Skip to content

Conversation

@tanislavivanov
Copy link

There’s a new Theme Color picker, added Info plist property to enable sites which don’t support http to be able to load. Also fixed the Sidebar button’s padding to be flush with others.
image
image

Introduces a ThemeColorPicker in settings, allowing users to customize the browser's primary and accent colors, with persistence via SettingsStore. Adds a new keyboard shortcut and command for 'Search in Current Tab', updating LauncherView and app state to support searching in the current tab instead of opening a new one. Updates Theme struct to support custom colors, refines version display, and changes the downloads icon. Also lowers the macOS deployment target to 14.0.
Removed unused keys from Info.plist and added NSAppTransportSecurity to allow arbitrary loads (allows http-only sites to be loaded). Also added extra top padding to the sidebar in HomeView for improved layout.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request introduces a customizable theme color picker, enables HTTP site loading support, implements a "Search in Current Tab" feature, and includes several minor UI refinements. The deployment target has been lowered to macOS 14.0 to increase compatibility.

  • Added a new Theme Color Picker in settings allowing users to customize primary and accent colors
  • Implemented "Search in Current Tab" functionality with keyboard shortcut (Cmd+Shift+G)
  • Added NSAllowsArbitraryLoads to Info.plist to enable HTTP site loading
  • Updated deployment target from macOS 15.0 to 14.0
  • Minor UI improvements: adjusted sidebar button padding, changed download icon, and updated branding text

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
project.yml Lowered deployment target to macOS 14.0 for broader compatibility
ora/oraApp.swift Added launcherSearchInCurrentTab state to support new search mode
ora/UI/LinkPreview.swift Updated version string format to include "v" prefix
ora/UI/HomeView.swift Added extra top padding to sidebar button for improved alignment
ora/OraRoot.swift Modified launcher notification handling to support search-in-current-tab mode
ora/OraCommands.swift Added new menu command for searching in current tab with keyboard shortcut
ora/Modules/Sidebar/DownloadsWidget.swift Changed download icon from "arrow.down" to "arrow.down.circle"
ora/Modules/Settings/Sections/ThemeColorPicker.swift New file implementing customizable theme color picker UI
ora/Modules/Settings/Sections/GeneralSettingsView.swift Integrated theme color picker into general settings
ora/Modules/Launcher/LauncherView.swift Implemented logic to search in current tab vs opening new tab
ora/Info.plist Removed duplicate CFBundle keys and added NSAppTransportSecurity for HTTP support
ora/Common/Utils/SettingsStore.swift Added persistence for theme primary and accent colors
ora/Common/Constants/Theme.swift Updated theme to support custom colors with user preferences
ora/Common/Constants/KeyboardShortcuts.swift Added keyboard shortcut definition for search-in-current-tab feature

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +64 to +65
<key>NSAllowsArbitraryLoads</key>
<true/>
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting NSAllowsArbitraryLoads to true disables App Transport Security (ATS) and allows all HTTP connections, which poses significant security risks. This makes the app vulnerable to man-in-the-middle attacks and exposes user data to unencrypted transmission.

While the PR description mentions enabling "sites which don't support http", this setting is too permissive. Consider a more secure approach:

  • Use NSExceptionDomains to allow HTTP only for specific domains that require it
  • Or use NSAllowsLocalNetworking if the goal is to support local development/testing
  • Document which specific sites require this and why HTTPS alternatives aren't available

This is especially important for a browser application that handles user credentials and sensitive data.

Suggested change
<key>NSAllowsArbitraryLoads</key>
<true/>
<!--
For security, do not allow arbitrary HTTP loads.
Instead, specify only the domains that require HTTP below.
Replace "example.com" with the actual domain(s) that require HTTP,
and document the reason for each.
-->
<key>NSExceptionDomains</key>
<dict>
<key>example.com</key>
<dict>
<key>NSExceptionAllowsInsecureHTTPLoads</key>
<true/>
<key>NSIncludesSubdomains</key>
<true/>
<!-- Reason: [REPLACE WITH EXPLANATION WHY HTTPS IS NOT AVAILABLE] -->
</dict>
<!-- Add more domains as needed -->
</dict>

Copilot uses AI. Check for mistakes.
Comment on lines +70 to +98
@State private var isUsingCustom = false

var body: some View {
HStack(spacing: 12) {
VStack(alignment: .leading, spacing: 4) {
Text(title)
.font(.subheadline)
.fontWeight(.medium)
Text(description)
.font(.caption2)
.foregroundColor(.secondary)
}

Spacer()

ColorPicker("", selection: $color, supportsOpacity: false)
.labelsHidden()
.frame(width: 50)

Button {
onReset()
isUsingCustom = false
} label: {
Text("Reset")
.font(.caption)
}
.buttonStyle(.plain)
.foregroundColor(.secondary)
.disabled(!isUsingCustom)
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The isUsingCustom state is a local variable in ColorPickerRow, but it should be initialized based on whether a custom color is set in settings. Currently, it's only updated in onAppear and onChange, but if the binding's initial value differs from default, there could be a brief moment where the Reset button state is incorrect.

Additionally, the disabled state logic doesn't account for the scenario where the user manually changes the color picker back to the default color - the Reset button should become disabled, but isUsingCustom will still be true until the onChange fires.

Consider using a computed property instead:

var isUsingCustom: Bool {
    let defaultHex = defaultColor.toHex() ?? ""
    let currentHex = color.toHex() ?? ""
    return defaultHex.lowercased() != currentHex.lowercased()
}

This ensures the state is always accurate without needing to manage it separately.

Copilot uses AI. Check for mistakes.
Comment on lines 23 to 26
var primaryDark: Color {
Color(hex: "#63411D")
// Use the same color as primary for both light and dark modes
return primary
}
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change to make primaryDark return the same color as primary breaks existing theme logic. Several computed properties depend on primaryDark having a different color value:

  • activeTabBackground (line 56): Uses self.primaryDark.opacity(0.8) in light mode
  • mutedBackground (line 60): Uses self.primaryDark.opacity(0.1) in light mode
  • invertedSolidWindowBackgroundColor (line 52): Swaps primary and primaryDark based on color scheme

With primaryDark now returning the same color as primary, these will produce unexpected visual results. For example, invertedSolidWindowBackgroundColor will always return the same color regardless of the color scheme.

If the intent is to use a single customizable color, these dependent properties should be updated to use different opacity levels or alternative color derivation methods to maintain visual distinction.

Copilot uses AI. Check for mistakes.
.onChange(of: appState.showLauncher) { _, newValue in
isVisible = newValue
if !newValue {
// Reset the flag when launcher is closed
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] When the launcher closes, the search input field is not being cleared. The input state variable in LauncherView retains its value, so when the launcher is reopened, the previous search text will still be visible.

Consider clearing the input when the launcher is closed:

.onChange(of: appState.showLauncher) { _, newValue in
    isVisible = newValue
    if !newValue {
        input = ""  // Clear input when closing
        match = nil  // Also clear any matched engine
        appState.launcherSearchInCurrentTab = false
    }
}

This provides a cleaner UX where each launcher invocation starts fresh.

Suggested change
// Reset the flag when launcher is closed
// Clear input and match when launcher is closed for a fresh start
input = ""
match = nil

Copilot uses AI. Check for mistakes.
Comment on lines +130 to +135
// Only open if not already open, or close if already open
if appState.showLauncher {
appState.showLauncher = false
} else {
appState.showLauncher = true
}
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The toggle logic for showLauncher has changed in a subtle but potentially problematic way. Previously, it used .toggle() which would automatically toggle the value. Now it explicitly checks if appState.showLauncher and sets it to false, or else sets it to true.

However, when the "Search in Current Tab" command is triggered while the launcher is already open, this logic will:

  1. Set launcherSearchInCurrentTab = true
  2. Close the launcher (because showLauncher was true)

This means the user would have to invoke the command twice - once to close the existing launcher, and again to open it with the search-in-current-tab mode. This is confusing UX.

Consider either:

  • Always opening the launcher when searchInCurrentTab is true (don't close if already open, just update the mode)
  • Or ensuring the launcher properly responds to the mode change without needing to close/reopen
Suggested change
// Only open if not already open, or close if already open
if appState.showLauncher {
appState.showLauncher = false
} else {
appState.showLauncher = true
}
// Always open the launcher and update the mode
appState.showLauncher = true

Copilot uses AI. Check for mistakes.
<key>LSApplicationCategoryType</key>
<string>public.app-category.web-browser</string>
<key>LSMinimumSystemVersion</key>
<string>13.0</string>
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deployment target has been lowered to macOS 14.0, but the LSMinimumSystemVersion in Info.plist is set to 13.0. These values should be aligned for consistency. Consider updating LSMinimumSystemVersion to match the deployment target of 14.0, or ensure there's a specific reason for the mismatch.

Suggested change
<string>13.0</string>
<string>14.0</string>

Copilot uses AI. Check for mistakes.
@yonaries
Copy link
Contributor

yonaries commented Dec 1, 2025

hey, thank you for the contribution. we were thinking to have the theme picker to be like arc/zen etc with three way color wheel. we would appreciate it if you can pivot to that. the noise dial knob is not necessary to have.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants