Add onboarding flow with Safari and Dia import#312
Merged
baginski11 merged 2 commits intodevfrom Feb 21, 2026
Merged
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a full onboarding flow (8 stages) with a TransitionView, onboarding UI components and shader transition; extends import support for Arc/Dia/Safari with parsers and async import paths; persists onboarding/tab layout settings and adjusts app/window startup chrome to avoid UI flashes. Changes
Sequence Diagram(s)sequenceDiagram
participant AppDelegate
participant UserDefaults
participant TransitionView
participant OnboardingView
participant BrowserManager
participant ImportManager
participant ContentView
AppDelegate->>UserDefaults: read settings.didFinishOnboarding
alt onboarding not finished
AppDelegate->>AppDelegate: apply immediate window chrome (hide titlebar/buttons, size/center)
AppDelegate->>TransitionView: present(showB=false)
TransitionView->>OnboardingView: render onboarding UI
User->>OnboardingView: complete stages / request import
OnboardingView->>BrowserManager: importArcData/importDiaData/importSafariData (async)
BrowserManager->>ImportManager: parse export (Dia/Safari)
ImportManager-->>BrowserManager: return parsed tabs/bookmarks/history
BrowserManager->>BrowserManager: create spaces, create folders, add/move tabs
OnboardingView->>UserDefaults: save tabLayout & didFinishOnboarding
OnboardingView->>TransitionView: signal completion (showB=true)
TransitionView->>ContentView: animate shader transition → show content
else onboarding finished
AppDelegate->>TransitionView: present(showB=true)
TransitionView->>ContentView: show content directly
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (16)
Nook/Assets.xcassets/Browser Logos/dia-logo.imageset/Contents.json (1)
1-21: Consider renaming generic asset filename."Frame 255.png" appears to be a raw export name from a design tool. Consider renaming to "dia-logo.png" for consistency with the imageset name and to avoid potential issues with spaces in filenames.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Nook/Assets.xcassets/Browser` Logos/dia-logo.imageset/Contents.json around lines 1 - 21, The imageset Contents.json references a generic export name "Frame 255.png" inside dia-logo.imageset which contains spaces and doesn't match the imageset; rename the actual asset file to a consistent name like "dia-logo.png", update the "filename" value in Contents.json to "dia-logo.png" for the 1x entry (and add corresponding filenames for 2x/3x if present), and ensure the renamed image file is added to the imageset bundle so Xcode finds it at build time.Onboarding/Components/RoundedSpinner.swift (1)
15-17: HardcodedColor.blackmay not be visible in dark mode.The spinner uses
Color.blackwhich will be invisible or hard to see against dark backgrounds. Consider using a semantic color like.primaryor a custom color from your asset catalog that adapts to light/dark mode.♻️ Proposed fix
.stroke( - Color.black, + Color.primary, style: StrokeStyle(lineWidth: 2.5, lineCap: .round) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Onboarding/Components/RoundedSpinner.swift` around lines 15 - 17, The spinner's stroke uses a hardcoded Color.black which can be hard to see in dark mode; update the stroke in RoundedSpinner (the .stroke call with StrokeStyle(lineWidth: 2.5, lineCap: .round)) to use a semantic/adaptive color such as Color.primary or a named asset color that supports light/dark mode (or Color(UIColor.label) on iOS) so the spinner remains visible across appearances.Nook/Assets.xcassets/Browser Logos/arc-logo.imageset/Contents.json (1)
1-21: Consider adding 2x/3x assets and using consistent naming.The 2x and 3x scale entries are placeholders without filenames, which may result in blurry images on Retina displays. Additionally, "Frame-2.png" is a generic filename—consider renaming to something descriptive like "arc-logo.png" or "arc-logo@1x.png" for maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Nook/Assets.xcassets/Browser` Logos/arc-logo.imageset/Contents.json around lines 1 - 21, Update Contents.json's images array so each scale entry includes a proper filename (replace the placeholder entries with "filename" fields) and use consistent, descriptive names instead of "Frame-2.png" (e.g., rename to "arc-logo@1x.png" and add matching "arc-logo@2x.png" and "arc-logo@3x.png" files); modify the "filename" values in the images array (the existing "Frame-2.png" entry and the two entries missing filename keys) to "arc-logo@1x.png", "arc-logo@2x.png", and "arc-logo@3x.png" respectively so Retina displays get correct assets and naming is maintainable.Onboarding/Components/OnboardingUtils.swift (1)
8-9: Unused import.
AppKitis imported but not used in this file. Only SwiftUI types are referenced.🧹 Proposed fix
import SwiftUI -import AppKit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Onboarding/Components/OnboardingUtils.swift` around lines 8 - 9, Remove the unused AppKit import from Onboarding/Components/OnboardingUtils.swift: the file only references SwiftUI types (import SwiftUI), so delete the line "import AppKit" to eliminate the unused import and any related compiler warnings; verify that no AppKit symbols are referenced in functions or types in this file (e.g., any helpers in OnboardingUtils).Nook/Managers/ImportManager/Dia.swift (2)
94-101: Consider validating URL format before import.The code checks for empty URLs but doesn't validate URL format. Malformed URLs (e.g., missing scheme, invalid characters) could cause issues downstream when the browser attempts to load them.
♻️ Add URL validation
let url = webContent._0.url - guard !url.isEmpty else { return nil } + guard !url.isEmpty, + URL(string: url) != nil else { return nil } return DiaImportTab(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Nook/Managers/ImportManager/Dia.swift` around lines 94 - 101, Replace the simple empty check on webContent._0.url with a proper URL validation: trim whitespace, ensure URL(string: url) yields a non-nil URL and that its scheme is http or https (or otherwise acceptable), and only then construct and return the DiaImportTab; if validation fails return nil. Update the guard that currently checks "!url.isEmpty" to perform these checks around webContent._0.url before calling DiaImportTab so malformed URLs are filtered out early.
68-77: Consider addingSendableconformance for async usage.Since
ImportManager.importDiaData()is async (per the AI summary), markingDiaImportResultandDiaImportTabasSendableensures safe cross-actor usage and silences Swift 6 concurrency warnings.♻️ Add Sendable conformance
-struct DiaImportResult { +struct DiaImportResult: Sendable { let favoriteTabs: [DiaImportTab] let windowTabs: [DiaImportTab] } -struct DiaImportTab { +struct DiaImportTab: Sendable { let id: String let title: String let url: String }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Nook/Managers/ImportManager/Dia.swift` around lines 68 - 77, Mark DiaImportResult and DiaImportTab as Sendable to allow safe cross-actor usage from async code: update the struct declarations for DiaImportResult and DiaImportTab to conform to Sendable (or `@unchecked` Sendable only if you have non-Sendable stored properties), ensuring their stored properties remain Sendable (String is Sendable) so ImportManager.importDiaData() can be used without Swift concurrency warnings.Onboarding/Components/StageFooter.swift (1)
66-77: Redundant guard insideonBackaction.The check
if currentStage > 0 { onBack() }on line 68 is redundant since the entire button is already hidden whencurrentStage == 0(line 66). Also, consider usingcurrentStage != 0for clarity.♻️ Simplified version
- if !(currentStage == 0) { + if currentStage != 0 { Button { - if currentStage > 0 { onBack() } + onBack() } label: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Onboarding/Components/StageFooter.swift` around lines 66 - 77, The back Button currently checks currentStage in two places: the outer guard if !(currentStage == 0) and again inside the Button action with if currentStage > 0 { onBack() }; remove the redundant inner guard and call onBack() directly in the Button action, and consider changing the outer condition to currentStage != 0 for clarity (references: currentStage, onBack, StageFooter.swift Button/secondaryText).Onboarding/Stages/AdBlockerStage.swift (1)
25-48: Consider extracting sharedlayoutOptioncomponent.This
layoutOptionfunction is nearly identical acrossAdBlockerStage,AiChatStage,TabLayoutStage, andURLBarStage. The only variation is the binding being updated. A reusable component accepting a generic selection callback would reduce duplication.♻️ Example shared component
// Shared component in Onboarding/Components/ struct OnboardingOptionButton: View { let image: String let label: String let isSelected: Bool let action: () -> Void var body: some View { VStack(spacing: 12) { Button(action: action) { Image(image) .resizable() .scaledToFit() .frame(width: 180, height: 140) .clipShape(RoundedRectangle(cornerRadius: 8)) .overlay { RoundedRectangle(cornerRadius: 8) .stroke(.black.opacity(0.2), lineWidth: isSelected ? 4 : 0) } .animation(.easeInOut(duration: 0.1), value: isSelected) } .buttonStyle(.plain) Text(label) .font(.system(size: 14, weight: .semibold)) .foregroundStyle(.white) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Onboarding/Stages/AdBlockerStage.swift` around lines 25 - 48, Extract the duplicated UI into a reusable View (e.g., OnboardingOptionButton) and replace the local layoutOption implementations: create a new struct OnboardingOptionButton with parameters image: String, label: String, isSelected: Bool, and action: () -> Void that renders the VStack/Button/Image/Text and the selection overlay/animation; then update AdBlockerStage's layoutOption call to instantiate OnboardingOptionButton passing image, label, isSelected: (adBlockerEnabled == enabled) and action: { adBlockerEnabled = enabled } (and do the same replacement in AiChatStage, TabLayoutStage, URLBarStage), keeping the original styling/animation and .buttonStyle(.plain) behavior.Onboarding/Stages/BackgroundStage.swift (2)
20-35: Consider renaming "Arc" label for clarity.The material label "Arc" for
.hudWindowat Line 21 may be confusing since Arc is a browser name. Consider using a more descriptive label like "HUD" or "Floating" that describes the visual appearance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Onboarding/Stages/BackgroundStage.swift` around lines 20 - 35, The label for the MaterialItem with id "hud" currently uses "Arc" which is ambiguous; update the materials array entry (the .init with id: "hud", material: .hudWindow) to a clearer label such as "HUD" or "Floating" (or another descriptive term), and ensure any user-facing strings or localization keys that reference that label are updated to match the new wording.
43-62: Consider usingLazyVGridfor cleaner grid layout.The current stride-based approach works but
LazyVGridwould be more idiomatic SwiftUI for grid layouts.♻️ Optional refactor using LazyVGrid
+ let columns = Array(repeating: GridItem(.flexible(), spacing: 8), count: 3) + var body: some View { VStack(spacing: 24) { Text("Background") .font(.system(size: 22, weight: .semibold)) .foregroundStyle(.white) - VStack(alignment: .center, spacing: 8) { - ForEach(Array(stride(from: 0, to: materials.count, by: 3)), id: \.self) { i in - HStack(alignment: .center, spacing: 8) { - ForEach(materials[i..<min(i + 3, materials.count)]) { item in - Button { - selectedMaterial = item.material - } label: { - Text(item.label) - .font(.system(size: 12, weight: .semibold)) - .foregroundStyle(selectedMaterial == item.material ? .black : .black.opacity(0.6)) - .padding(.vertical, 10) - .padding(.horizontal, 20) - .background(selectedMaterial == item.material ? .white : .white.opacity(0.6)) - .clipShape(RoundedRectangle(cornerRadius: 12)) - } - .buttonStyle(.plain) - } - } + LazyVGrid(columns: columns, spacing: 8) { + ForEach(materials) { item in + Button { + selectedMaterial = item.material + } label: { + Text(item.label) + .font(.system(size: 12, weight: .semibold)) + .foregroundStyle(selectedMaterial == item.material ? .black : .black.opacity(0.6)) + .padding(.vertical, 10) + .padding(.horizontal, 20) + .background(selectedMaterial == item.material ? .white : .white.opacity(0.6)) + .clipShape(RoundedRectangle(cornerRadius: 12)) + } + .buttonStyle(.plain) } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Onboarding/Stages/BackgroundStage.swift` around lines 43 - 62, Replace the manual stride-based grid in BackgroundStage with a LazyVGrid: define an array of three GridItem(.flexible()) and use LazyVGrid(columns: gridItems, spacing: 8) to iterate over materials directly (instead of the outer ForEach/stride and inner slice ForEach). Preserve the Button behavior and styling (updating selectedMaterial, Text(item.label), font, foregroundStyle, padding, background, clipShape, and .buttonStyle(.plain)) so the visual and selection logic remains identical while simplifying layout code.Onboarding/Stages/ImportStage.swift (1)
49-68: Redundant enabled check in button action.The
if enabledcheck at Line 53-55 is redundant since the button is already disabled via.disabled(!enabled)at Line 67. The action will never be called when disabled.♻️ Remove redundant check
`@ViewBuilder` private func browserButton(image: String, browser: Browsers) -> some View { let enabled = browser.isImplemented Button { - if enabled { - selectedBrowser = browser - } + selectedBrowser = browser } label: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Onboarding/Stages/ImportStage.swift` around lines 49 - 68, The action inside browserButton redundantly checks "if enabled" even though the Button is disabled via .disabled(!enabled); remove the conditional and simply set selectedBrowser = browser in the Button action block. Update the closure in browserButton(image:browser:) to directly assign selectedBrowser, keeping the existing .disabled(!enabled) and ScaleButtonStyle(), and ensure no other logic depends on the removed guard variable "enabled".App/AppDelegate.swift (1)
60-77: Consider whether hiding other applications is the desired UX.
NSApp.hideOtherApplications(nil)at Line 75 might feel intrusive to users who are multitasking during onboarding. This is a design decision, but worth confirming it's intentional.The window chrome hiding and sizing logic for onboarding looks correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@App/AppDelegate.swift` around lines 60 - 77, The call to NSApp.hideOtherApplications(nil) during onboarding can be intrusive; remove it or gate it behind an explicit opt-in so onboarding doesn't hide other apps unexpectedly. Update the onboarding branch where didFinishOnboarding is checked (the block that calls window.setContentSize(...) and NSApp.activate(...)) to either delete the NSApp.hideOtherApplications(nil) call or wrap it in a conditional (e.g., a userDefaults flag or an onboarding option) so the behavior is only executed when explicitly enabled.Nook/Managers/ImportManager/Safari.swift (2)
139-176: Nested folder tracking may produce unexpected results.The
currentFolderis reset tonilwhen encountering</DL>, but Safari's bookmark export uses nested<DL>tags for folder hierarchies. This means bookmarks in nested folders will lose their parent folder context when the inner folder closes.For basic onboarding import purposes, this may be acceptable since only the immediate folder name is captured. Consider documenting this limitation if nested folder preservation becomes important later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Nook/Managers/ImportManager/Safari.swift` around lines 139 - 176, The parser in parseSafariBookmarks currently uses a single currentFolder variable and sets it to nil on every "</DL>", which loses parent folder context for nested <DL> hierarchies; change to a stack-based approach: maintain a folderStack (push the folder name when you parse a "<DT><H3...>" folder header and set currentFolder = folderStack.last), and pop the stack when encountering a "</DL>" to restore the parent folder; update uses of currentFolder (and the bookmark append) to read the top of the stack instead of a single variable so nested folders retain correct parent context.
178-212: Duplicated OAuth/auth URL filtering logic.The filtering for OAuth and auth URLs appears twice - once in the
guardblock for entries without titles (lines 186-188) and again for entries with titles (lines 201-202). Consider extracting this into a helper function for maintainability.🧹 Proposed refactor
+private func isAuthRelatedURL(_ url: String) -> Bool { + url.contains("accounts.google.com") || + url.contains("auth0.") || + url.contains("/oauth") +} + func parseSafariHistory(from fileURL: URL) throws -> [SafariImportHistoryEntry] { let data = try Data(contentsOf: fileURL) let decoded = try JSONDecoder().decode(SafariHistoryFile.self, from: data) return decoded.history.compactMap { entry in + // Filter out authentication-related URLs + if isAuthRelatedURL(entry.url) { return nil } + guard let title = entry.title, !title.isEmpty else { guard let url = URL(string: entry.url), - (url.scheme == "http" || url.scheme == "https"), - !entry.url.contains("accounts.google.com"), - !entry.url.contains("auth0."), - !entry.url.contains("/oauth") else { + (url.scheme == "http" || url.scheme == "https") else { return nil } let displayTitle = url.host ?? entry.url let date = Date(timeIntervalSince1970: Double(entry.time_usec) / 1_000_000) return SafariImportHistoryEntry( url: entry.url, title: displayTitle, visitDate: date, visitCount: entry.visit_count ) } - if entry.url.contains("accounts.google.com/") && entry.url.contains("oauth") { return nil } - if entry.url.contains("auth0.") { return nil } - let date = Date(timeIntervalSince1970: Double(entry.time_usec) / 1_000_000)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Nook/Managers/ImportManager/Safari.swift` around lines 178 - 212, The duplicate OAuth/auth URL checks in parseSafariHistory should be extracted into a small helper (e.g., isOAuthOrAuthURL(urlString: String) -> Bool) and used in both branches to determine filtering; update parseSafariHistory to call that helper instead of repeating the checks (ensure the helper validates scheme when needed and checks for "accounts.google.com", "auth0.", and "/oauth" against the entry.url), and keep existing behavior when deciding to return nil or construct SafariImportHistoryEntry.Onboarding/Stages/SafariImportFlow.swift (1)
396-420: Import counts reflect expected items, not actual results.The
importedBookmarkCountandimportedHistoryCountare set fromexportContentscounts (the expected values) rather than the actual import result. If some items fail to import, the displayed counts could be misleading to the user.Consider capturing actual counts from the import result if
BrowserManager.importSafariDatacould return them, or document this behavior if approximate counts are acceptable for onboarding UX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Onboarding/Stages/SafariImportFlow.swift` around lines 396 - 420, The counts shown after import are taken from exportContents (expected totals) rather than the actual import outcome; update browserManager.importSafariData to return an import result (e.g., a struct with importedBookmarkCount and importedHistoryCount) and change startImport to await and use that returned result to set importedBookmarkCount and importedHistoryCount instead of using exportContents.bookmarkCount/historyCount (or, if you decide to keep the current API, explicitly document in SafariImportFlow.swift that the displayed counts are expected counts and may differ from actual imported items). Ensure you update references to importSafariData and the assignment in startImport so the UI reflects actual imported counts.Settings/NookSettingsService.swift (1)
400-401: Duplicate imports.
AppKitandFoundationare already imported at the top of the file (lines 9-10). These duplicate imports are unnecessary.🧹 Proposed fix to remove duplicate imports
-import AppKit -import Foundation -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Settings/NookSettingsService.swift` around lines 400 - 401, Remove the duplicate import statements for AppKit and Foundation in Settings/NookSettingsService.swift by deleting the repeated import lines (the second occurrences) so only the initial top-level imports remain; locate the duplicate "import AppKit" and "import Foundation" entries and remove them, leaving the single original imports that are already declared earlier in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Nook/Assets.xcassets/adblocker-off.imageset/Contents.json`:
- Around line 3-6: The imageset "adblocker-off.imageset" references the wrong
image file: change the "filename" value that currently says "adblocker-on.png"
to the correct "adblocker-off.png" inside the Contents.json for the
adblocker-off imageset (and verify any other scale entries like "1x"/"2x"/"3x"
reference matching "adblocker-off" filenames so the off-state icon displays
correctly).
In `@Nook/Assets.xcassets/adblocker-on.imageset/Contents` 2.json:
- Around line 1-21: This file is an invalid duplicate ("Contents 2.json") and
wrongly references adblocker-off.png inside the adblocker-on imageset; delete
the "Contents 2.json" file from the asset catalog (or replace it by renaming to
Contents.json if intended) and update the actual adblocker-on imageset's
Contents.json so the image entry references "adblocker-on.png" (ensure the
imageset identifier is "adblocker-on" and the filename field is corrected for
all scales).
In `@Nook/Assets.xcassets/adblocker-on.imageset/Contents.json`:
- Line 4: The adblocker icon PNGs are swapped between imageset folders: move the
actual files so adblocker-off.png is physically placed into
adblocker-off.imageset and adblocker-on.png into adblocker-on.imageset, then
update each imageset's Contents.json (adblocker-on.imageset/Contents.json and
adblocker-off.imageset/Contents.json) to reference the correct filename for
their image entry; do not only change the filename in Contents.json without
moving the files, and verify the filenames match exactly (adblocker-on.png /
adblocker-off.png).
In `@Nook/Assets.xcassets/ai-chat-on.imageset/Contents` 2.json:
- Around line 1-21: Delete the unrecognized file named "Contents 2.json" inside
the ai-chat-on.imageset asset folder (the file shown in the diff) because Xcode
only reads "Contents.json"; remove the file from the repository (git rm and
commit) so the asset catalog contains only the valid Contents.json and the image
entries.
In `@Nook/Assets.xcassets/ai-chat-on.imageset/Contents.json`:
- Around line 8-15: Contents.json for the ai-chat-on.imageset is missing the
"filename" entries for the 2x and 3x variants, causing 1x to be upscaled; open
Nook/Assets.xcassets/ai-chat-on.imageset/Contents.json and add "filename":
"ai-chat-on@2x.png" to the object with "scale":"2x" and "filename":
"ai-chat-on@3x.png" to the object with "scale":"3x", and ensure the
corresponding image files ai-chat-on@2x.png and ai-chat-on@3x.png are added to
the asset catalog so Retina displays use the correct raster assets.
In `@Nook/Assets.xcassets/Browser` Logos/Contents 2.json:
- Around line 1-6: Remove the invalid duplicate asset file "Contents 2.json"
inside the Browser Logos asset catalog; this file is not recognized by Xcode
(asset catalogs must use "Contents.json") so delete "Contents 2.json" from the
repo and ensure only the valid "Contents.json" remains to avoid confusion and
ignored files.
In `@Nook/Assets.xcassets/Browser` Logos/safari-logo.imageset/Contents.json:
- Around line 8-15: Contents.json currently has "scale":"2x" and "scale":"3x"
entries without "filename" values, so Retina displays will upscale the 1x asset
and render a blurry Safari logo; fix by adding proper 2x and 3x image files to
the asset catalog and update the Contents.json entries for the 2x and 3x items
to include the correct "filename" properties (e.g., safari@2x.png,
safari@3x.png) matching the actual asset names, and repeat the same for other
browser logo asset sets to ensure high-DPI support across onboarding.
In `@Nook/Assets.xcassets/sidebar.imageset/Contents` 2.json:
- Around line 1-21: The file "sidebar.imageset/Contents 2.json" is misnamed and
will be ignored by Xcode; rename it to "Contents.json" (replacing or merging
with any existing Contents.json in the same imageset) so the asset catalog
manifest is recognized, and ensure the "images" array entries (for 1x/2x/3x)
have the correct "filename" fields and idiom/scale values preserved after the
merge.
In `@Nook/Assets.xcassets/sidebar.imageset/Contents.json`:
- Around line 8-15: The Contents.json currently lists empty 2x and 3x image
slots ("scale": "2x" and "scale": "3x") which causes 1x images to be upscaled on
Retina displays; either add the matching high-resolution assets (filenames and
actual `@2x/`@3x images) for the sidebar image set or remove those two entries so
the asset catalog no longer advertises 2x/3x variants; locate the
"sidebar.imageset" entries in Contents.json and update the objects with correct
"filename" fields for the 2x/3x scales or delete those objects if no higher-res
images exist.
In `@Nook/Assets.xcassets/top-of-window.imageset/Contents` 2.json:
- Around line 1-21: Remove the duplicate file named "Contents 2.json" from the
top-of-window.imageset and then edit the actual Contents.json in that imageset:
either remove the 2x and 3x entries from the "images" array if you only want 1x
support, or add filenames "topofwindow@2x.png" and "topofwindow@3x.png" to the
corresponding 2x and 3x objects (and add the matching image files to the
imageset). Ensure the "filename" keys for the 2x/3x entries match the actual
asset filenames and keep the existing topofwindow.png as the 1x entry.
In `@Nook/Assets.xcassets/url-in-sidebar.imageset/Contents` 2.json:
- Around line 1-21: Delete the duplicate asset catalog file named
"url-in-sidebar.imageset/Contents 2.json" (the unwanted "Contents 2.json"
artifact) so only the valid "Contents.json" remains in the
url-in-sidebar.imageset; ensure you remove the file from the commit/PR and
verify the asset catalog builds cleanly in Xcode.
In `@Nook/Assets.xcassets/url-top-of-website.imageset/Contents` 2.json:
- Around line 1-21: The duplicate asset catalog file "Contents 2.json" inside
url-top-of-website.imageset should be removed because Xcode only uses
Contents.json; delete the file "Contents 2.json" (the duplicate of
Contents.json) from the url-top-of-website.imageset so only the canonical
Contents.json remains and the asset catalog is unambiguous.
In `@Nook/Managers/BrowserManager/BrowserManager.swift`:
- Around line 2248-2290: The space lookup by name in importArcData is unsafe
because tabManager.spaces.first(where: { $0.name == space.title }) can return an
existing space with the same name; instead capture and use the return value from
tabManager.createSpace directly (or change createSpace to return the created
Space if it currently doesn't) and use that returned instance (e.g., assign
createdSpace = tabManager.createSpace(...)) when creating tabs, pinning, and
folders so you operate on the exact newly-created space rather than searching by
name.
- Around line 2351-2362: History entries are being added with profileId: nil
which can cause cross-profile leakage; update the BrowserManager code that
iterates result.history and call historyManager.addVisit with profileId:
currentProfile?.id (instead of nil) so imported entries are explicitly tied to
the active profile; locate the loop over result.history and replace the
profileId argument in historyManager.addVisit to use currentProfile?.id, keeping
other fields (url, title, timestamp, tabId) unchanged.
In `@Nook/Managers/KeyboardShortcutManager/KeyboardShortcutManager.swift`:
- Around line 185-194: The validation currently accepts Return/Enter without
modifiers while executeShortcut (in KeyboardShortcutManager) ignores unmodified
Return/Enter; update the validation logic in isValidKeyCombination (or the
method that validates default shortcuts) to mirror executeShortcut by rejecting
keyCode 36 and 76 when no modifier flags are present—i.e., detect event.keyCode
== 36 || 76 and ensure event.modifierFlags contains at least one of .command,
.option, .control, .shift before marking the combination valid; alternatively,
if you want to allow explicitly configured unmodified Return, add a clear
conditional flag on the shortcut model and only bypass this rule when that flag
is set.
In `@Nook/Models/Tab/Tab.swift`:
- Around line 449-456: The if branch that removes boost scripts currently clears
user scripts without restoring non-boost scripts; update the removal branch in
Tab.swift to mirror the else branch by building boostScriptSources from
currentBoostScripts and then iterating allScripts to call
userContentController.addUserScript(script) for any script whose source is not
in boostScriptSources (i.e., restore non-boost scripts), ensuring you use the
same symbols currentBoostScripts, boostScriptSources, allScripts and
userContentController.addUserScript as in the existing code.
In `@Onboarding/OnboardingView.swift`:
- Around line 158-174: The goBack() handler has a copy-paste bug: when
currentStage == 1 it increments (currentStage += 1) instead of moving back;
update the branch in goBack() (the if currentStage == 1 case) to decrement the
stage (e.g., currentStage -= 1 or set currentStage = 0) inside the same
withAnimation(.easeInOut(duration: 0.25)) block so pressing back from stage 1
actually goes to the previous stage.
In `@Onboarding/Stages/FInalStage.swift`:
- Around line 1-6: Rename the file from "FInalStage.swift" to "FinalStage.swift"
to match the struct name and correct capitalization; ensure the declared
type/struct FinalStage remains unchanged and update any imports or references if
your project tooling caches the old filename so all references to FinalStage
resolve correctly.
---
Duplicate comments:
In `@Onboarding/Stages/AiChatStage.swift`:
- Around line 10-48: This duplicates the layoutOption view already flagged in
AdBlockerStage.swift; refactor by extracting the repeated layoutOption logic
into a shared reusable View (e.g., a SelectionOptionView or OptionTile) and
replace the private func layoutOption(image:label:enabled:) in AiChatStage with
that shared component, keeping the same API (image, label, enabled binding/prop
and the tap handler that sets aiChatEnabled) so both AiChatStage and
AdBlockerStage can instantiate the common View instead of duplicating code.
---
Nitpick comments:
In `@App/AppDelegate.swift`:
- Around line 60-77: The call to NSApp.hideOtherApplications(nil) during
onboarding can be intrusive; remove it or gate it behind an explicit opt-in so
onboarding doesn't hide other apps unexpectedly. Update the onboarding branch
where didFinishOnboarding is checked (the block that calls
window.setContentSize(...) and NSApp.activate(...)) to either delete the
NSApp.hideOtherApplications(nil) call or wrap it in a conditional (e.g., a
userDefaults flag or an onboarding option) so the behavior is only executed when
explicitly enabled.
In `@Nook/Assets.xcassets/Browser` Logos/arc-logo.imageset/Contents.json:
- Around line 1-21: Update Contents.json's images array so each scale entry
includes a proper filename (replace the placeholder entries with "filename"
fields) and use consistent, descriptive names instead of "Frame-2.png" (e.g.,
rename to "arc-logo@1x.png" and add matching "arc-logo@2x.png" and
"arc-logo@3x.png" files); modify the "filename" values in the images array (the
existing "Frame-2.png" entry and the two entries missing filename keys) to
"arc-logo@1x.png", "arc-logo@2x.png", and "arc-logo@3x.png" respectively so
Retina displays get correct assets and naming is maintainable.
In `@Nook/Assets.xcassets/Browser` Logos/dia-logo.imageset/Contents.json:
- Around line 1-21: The imageset Contents.json references a generic export name
"Frame 255.png" inside dia-logo.imageset which contains spaces and doesn't match
the imageset; rename the actual asset file to a consistent name like
"dia-logo.png", update the "filename" value in Contents.json to "dia-logo.png"
for the 1x entry (and add corresponding filenames for 2x/3x if present), and
ensure the renamed image file is added to the imageset bundle so Xcode finds it
at build time.
In `@Nook/Managers/ImportManager/Dia.swift`:
- Around line 94-101: Replace the simple empty check on webContent._0.url with a
proper URL validation: trim whitespace, ensure URL(string: url) yields a non-nil
URL and that its scheme is http or https (or otherwise acceptable), and only
then construct and return the DiaImportTab; if validation fails return nil.
Update the guard that currently checks "!url.isEmpty" to perform these checks
around webContent._0.url before calling DiaImportTab so malformed URLs are
filtered out early.
- Around line 68-77: Mark DiaImportResult and DiaImportTab as Sendable to allow
safe cross-actor usage from async code: update the struct declarations for
DiaImportResult and DiaImportTab to conform to Sendable (or `@unchecked` Sendable
only if you have non-Sendable stored properties), ensuring their stored
properties remain Sendable (String is Sendable) so ImportManager.importDiaData()
can be used without Swift concurrency warnings.
In `@Nook/Managers/ImportManager/Safari.swift`:
- Around line 139-176: The parser in parseSafariBookmarks currently uses a
single currentFolder variable and sets it to nil on every "</DL>", which loses
parent folder context for nested <DL> hierarchies; change to a stack-based
approach: maintain a folderStack (push the folder name when you parse a
"<DT><H3...>" folder header and set currentFolder = folderStack.last), and pop
the stack when encountering a "</DL>" to restore the parent folder; update uses
of currentFolder (and the bookmark append) to read the top of the stack instead
of a single variable so nested folders retain correct parent context.
- Around line 178-212: The duplicate OAuth/auth URL checks in parseSafariHistory
should be extracted into a small helper (e.g., isOAuthOrAuthURL(urlString:
String) -> Bool) and used in both branches to determine filtering; update
parseSafariHistory to call that helper instead of repeating the checks (ensure
the helper validates scheme when needed and checks for "accounts.google.com",
"auth0.", and "/oauth" against the entry.url), and keep existing behavior when
deciding to return nil or construct SafariImportHistoryEntry.
In `@Onboarding/Components/OnboardingUtils.swift`:
- Around line 8-9: Remove the unused AppKit import from
Onboarding/Components/OnboardingUtils.swift: the file only references SwiftUI
types (import SwiftUI), so delete the line "import AppKit" to eliminate the
unused import and any related compiler warnings; verify that no AppKit symbols
are referenced in functions or types in this file (e.g., any helpers in
OnboardingUtils).
In `@Onboarding/Components/RoundedSpinner.swift`:
- Around line 15-17: The spinner's stroke uses a hardcoded Color.black which can
be hard to see in dark mode; update the stroke in RoundedSpinner (the .stroke
call with StrokeStyle(lineWidth: 2.5, lineCap: .round)) to use a
semantic/adaptive color such as Color.primary or a named asset color that
supports light/dark mode (or Color(UIColor.label) on iOS) so the spinner remains
visible across appearances.
In `@Onboarding/Components/StageFooter.swift`:
- Around line 66-77: The back Button currently checks currentStage in two
places: the outer guard if !(currentStage == 0) and again inside the Button
action with if currentStage > 0 { onBack() }; remove the redundant inner guard
and call onBack() directly in the Button action, and consider changing the outer
condition to currentStage != 0 for clarity (references: currentStage, onBack,
StageFooter.swift Button/secondaryText).
In `@Onboarding/Stages/AdBlockerStage.swift`:
- Around line 25-48: Extract the duplicated UI into a reusable View (e.g.,
OnboardingOptionButton) and replace the local layoutOption implementations:
create a new struct OnboardingOptionButton with parameters image: String, label:
String, isSelected: Bool, and action: () -> Void that renders the
VStack/Button/Image/Text and the selection overlay/animation; then update
AdBlockerStage's layoutOption call to instantiate OnboardingOptionButton passing
image, label, isSelected: (adBlockerEnabled == enabled) and action: {
adBlockerEnabled = enabled } (and do the same replacement in AiChatStage,
TabLayoutStage, URLBarStage), keeping the original styling/animation and
.buttonStyle(.plain) behavior.
In `@Onboarding/Stages/BackgroundStage.swift`:
- Around line 20-35: The label for the MaterialItem with id "hud" currently uses
"Arc" which is ambiguous; update the materials array entry (the .init with id:
"hud", material: .hudWindow) to a clearer label such as "HUD" or "Floating" (or
another descriptive term), and ensure any user-facing strings or localization
keys that reference that label are updated to match the new wording.
- Around line 43-62: Replace the manual stride-based grid in BackgroundStage
with a LazyVGrid: define an array of three GridItem(.flexible()) and use
LazyVGrid(columns: gridItems, spacing: 8) to iterate over materials directly
(instead of the outer ForEach/stride and inner slice ForEach). Preserve the
Button behavior and styling (updating selectedMaterial, Text(item.label), font,
foregroundStyle, padding, background, clipShape, and .buttonStyle(.plain)) so
the visual and selection logic remains identical while simplifying layout code.
In `@Onboarding/Stages/ImportStage.swift`:
- Around line 49-68: The action inside browserButton redundantly checks "if
enabled" even though the Button is disabled via .disabled(!enabled); remove the
conditional and simply set selectedBrowser = browser in the Button action block.
Update the closure in browserButton(image:browser:) to directly assign
selectedBrowser, keeping the existing .disabled(!enabled) and
ScaleButtonStyle(), and ensure no other logic depends on the removed guard
variable "enabled".
In `@Onboarding/Stages/SafariImportFlow.swift`:
- Around line 396-420: The counts shown after import are taken from
exportContents (expected totals) rather than the actual import outcome; update
browserManager.importSafariData to return an import result (e.g., a struct with
importedBookmarkCount and importedHistoryCount) and change startImport to await
and use that returned result to set importedBookmarkCount and
importedHistoryCount instead of using exportContents.bookmarkCount/historyCount
(or, if you decide to keep the current API, explicitly document in
SafariImportFlow.swift that the displayed counts are expected counts and may
differ from actual imported items). Ensure you update references to
importSafariData and the assignment in startImport so the UI reflects actual
imported counts.
In `@Settings/NookSettingsService.swift`:
- Around line 400-401: Remove the duplicate import statements for AppKit and
Foundation in Settings/NookSettingsService.swift by deleting the repeated import
lines (the second occurrences) so only the initial top-level imports remain;
locate the duplicate "import AppKit" and "import Foundation" entries and remove
them, leaving the single original imports that are already declared earlier in
the file.
Comment on lines
3
to
6
| { | ||
| "filename" : "adblocker-on.png", | ||
| "idiom" : "universal", | ||
| "scale" : "1x" |
There was a problem hiding this comment.
Wrong asset filename: "adblocker-off" imageset references "adblocker-on.png".
The imageset is for the "off" state but the 1x entry references "adblocker-on.png". This will cause the wrong icon to display when adblocker is off.
🐛 Proposed fix
{
- "filename" : "adblocker-on.png",
+ "filename" : "adblocker-off.png",
"idiom" : "universal",
"scale" : "1x"
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| { | |
| "filename" : "adblocker-on.png", | |
| "idiom" : "universal", | |
| "scale" : "1x" | |
| { | |
| "filename" : "adblocker-off.png", | |
| "idiom" : "universal", | |
| "scale" : "1x" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Nook/Assets.xcassets/adblocker-off.imageset/Contents.json` around lines 3 -
6, The imageset "adblocker-off.imageset" references the wrong image file: change
the "filename" value that currently says "adblocker-on.png" to the correct
"adblocker-off.png" inside the Contents.json for the adblocker-off imageset (and
verify any other scale entries like "1x"/"2x"/"3x" reference matching
"adblocker-off" filenames so the off-state icon displays correctly).
Comment on lines
1
to
21
| { | ||
| "images" : [ | ||
| { | ||
| "filename" : "adblocker-off.png", | ||
| "idiom" : "universal", | ||
| "scale" : "1x" | ||
| }, | ||
| { | ||
| "idiom" : "universal", | ||
| "scale" : "2x" | ||
| }, | ||
| { | ||
| "idiom" : "universal", | ||
| "scale" : "3x" | ||
| } | ||
| ], | ||
| "info" : { | ||
| "author" : "xcode", | ||
| "version" : 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Invalid file and incorrect asset reference.
Two issues with this file:
- Xcode ignores "Contents 2.json" – Asset catalogs only recognize
Contents.json. This file will have no effect. - Wrong filename – The imageset is
adblocker-onbut referencesadblocker-off.png(Line 4). This will display the wrong icon if the file were recognized.
This file should likely be deleted, and the correct Contents.json should reference the appropriate adblocker-on.png asset.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Nook/Assets.xcassets/adblocker-on.imageset/Contents` 2.json around lines 1 -
21, This file is an invalid duplicate ("Contents 2.json") and wrongly references
adblocker-off.png inside the adblocker-on imageset; delete the "Contents 2.json"
file from the asset catalog (or replace it by renaming to Contents.json if
intended) and update the actual adblocker-on imageset's Contents.json so the
image entry references "adblocker-on.png" (ensure the imageset identifier is
"adblocker-on" and the filename field is corrected for all scales).
| { | ||
| "images" : [ | ||
| { | ||
| "filename" : "adblocker-off.png", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what image files exist in the adblocker-on.imageset directory
echo "=== Files in adblocker-on.imageset ==="
find "Nook/Assets.xcassets/adblocker-on.imageset" -type f 2>/dev/null | sort
echo ""
echo "=== Files in adblocker-off.imageset ==="
find "Nook/Assets.xcassets/adblocker-off.imageset" -type f 2>/dev/null | sort
echo ""
echo "=== Contents.json content of adblocker-on.imageset ==="
cat "Nook/Assets.xcassets/adblocker-on.imageset/Contents.json" 2>/dev/null
echo ""
echo "=== Contents.json content of adblocker-off.imageset ==="
cat "Nook/Assets.xcassets/adblocker-off.imageset/Contents.json" 2>/dev/nullRepository: nook-browser/Nook
Length of output: 1170
Critical: Image files are swapped between imageset directories.
The adblocker-on.imageset directory contains adblocker-off.png, while adblocker-off.imageset contains adblocker-on.png. This will display the wrong icon state. The image files must be moved to their correct directories:
- Move
adblocker-off.pngfromadblocker-on.imagesettoadblocker-off.imageset - Move
adblocker-on.pngfromadblocker-off.imagesettoadblocker-on.imageset - Update both
Contents.jsonfiles to reference the correct filenames after the move
Changing only the filename in Contents.json without moving files will create a broken reference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Nook/Assets.xcassets/adblocker-on.imageset/Contents.json` at line 4, The
adblocker icon PNGs are swapped between imageset folders: move the actual files
so adblocker-off.png is physically placed into adblocker-off.imageset and
adblocker-on.png into adblocker-on.imageset, then update each imageset's
Contents.json (adblocker-on.imageset/Contents.json and
adblocker-off.imageset/Contents.json) to reference the correct filename for
their image entry; do not only change the filename in Contents.json without
moving the files, and verify the filenames match exactly (adblocker-on.png /
adblocker-off.png).
Comment on lines
1
to
21
| { | ||
| "images" : [ | ||
| { | ||
| "filename" : "ai-chat-on.png", | ||
| "idiom" : "universal", | ||
| "scale" : "1x" | ||
| }, | ||
| { | ||
| "idiom" : "universal", | ||
| "scale" : "2x" | ||
| }, | ||
| { | ||
| "idiom" : "universal", | ||
| "scale" : "3x" | ||
| } | ||
| ], | ||
| "info" : { | ||
| "author" : "xcode", | ||
| "version" : 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove unrecognized file.
Contents 2.json is not read by Xcode—only Contents.json is valid for asset catalogs. This file should be deleted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Nook/Assets.xcassets/ai-chat-on.imageset/Contents` 2.json around lines 1 -
21, Delete the unrecognized file named "Contents 2.json" inside the
ai-chat-on.imageset asset folder (the file shown in the diff) because Xcode only
reads "Contents.json"; remove the file from the repository (git rm and commit)
so the asset catalog contains only the valid Contents.json and the image
entries.
Comment on lines
+8
to
+15
| { | ||
| "idiom" : "universal", | ||
| "scale" : "2x" | ||
| }, | ||
| { | ||
| "idiom" : "universal", | ||
| "scale" : "3x" | ||
| } |
There was a problem hiding this comment.
Missing 2x and 3x image assets.
The 2x and 3x scale variants have no filename specified. On Retina displays, the 1x asset will be upscaled, resulting in blurry images. Consider adding ai-chat-on@2x.png and ai-chat-on@3x.png for crisp rendering on high-DPI screens.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Nook/Assets.xcassets/ai-chat-on.imageset/Contents.json` around lines 8 - 15,
Contents.json for the ai-chat-on.imageset is missing the "filename" entries for
the 2x and 3x variants, causing 1x to be upscaled; open
Nook/Assets.xcassets/ai-chat-on.imageset/Contents.json and add "filename":
"ai-chat-on@2x.png" to the object with "scale":"2x" and "filename":
"ai-chat-on@3x.png" to the object with "scale":"3x", and ensure the
corresponding image files ai-chat-on@2x.png and ai-chat-on@3x.png are added to
the asset catalog so Retina displays use the correct raster assets.
Comment on lines
+2351
to
2362
| if !result.history.isEmpty { | ||
| for entry in result.history { | ||
| guard let url = URL(string: entry.url) else { continue } | ||
| historyManager.addVisit( | ||
| url: url, | ||
| title: entry.title, | ||
| timestamp: entry.visitDate, | ||
| tabId: nil, | ||
| profileId: nil | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
History entries imported with nil profileId may cause cross-profile leakage.
At Lines 2354-2360, history entries are added with profileId: nil. Based on the codebase context (see migrateUnassignedDataToDefaultProfile), nil profileId entries get migrated to the default profile, but it would be more explicit to use currentProfile?.id directly.
🛡️ Proposed fix
for entry in result.history {
guard let url = URL(string: entry.url) else { continue }
historyManager.addVisit(
url: url,
title: entry.title,
timestamp: entry.visitDate,
tabId: nil,
- profileId: nil
+ profileId: currentProfile?.id
)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Nook/Managers/BrowserManager/BrowserManager.swift` around lines 2351 - 2362,
History entries are being added with profileId: nil which can cause
cross-profile leakage; update the BrowserManager code that iterates
result.history and call historyManager.addVisit with profileId:
currentProfile?.id (instead of nil) so imported entries are explicitly tied to
the active profile; locate the loop over result.history and replace the
profileId argument in historyManager.addVisit to use currentProfile?.id, keeping
other fields (url, title, timestamp, tabId) unchanged.
Comment on lines
449
to
456
| let boostScriptSources = Set(currentBoostScripts.map { $0.source }) | ||
| for script in allScripts { | ||
| if !boostScriptSources.contains(script.source) { | ||
| userContentController.addUserScript(script) | ||
| } | ||
| } | ||
| // for script in allScripts { | ||
| // if !boostScriptSources.contains(script.source) { | ||
| // userContentController.addUserScript(script) | ||
| // } | ||
| // } | ||
| // MARK: This causes the browser to crash when loading boosted pages | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file and surrounding context
fd -e swift Tab.swiftRepository: nook-browser/Nook
Length of output: 304
🏁 Script executed:
# Read the specific section with broader context
head -500 Nook/Models/Tab/Tab.swift | tail -100Repository: nook-browser/Nook
Length of output: 4758
🏁 Script executed:
# Search for setupBoostUserScript and related methods
rg -n "setupBoostUserScript|removeAllUserScripts|addUserScript|userScripts" --type=swift -A 3 -B 3Repository: nook-browser/Nook
Length of output: 15700
Restore non-boost scripts when removing boost scripts.
The if branch removes all user scripts but never restores non-boost ones, causing them to disappear for the remainder of the session. The else branch below already has the correct pattern—apply the same approach to the if block.
Proposed fix
if !currentBoostScripts.isEmpty {
// Remove only the boost scripts we previously added
// Compare by source content since WKUserScript doesn't conform to Equatable
let allScripts = userContentController.userScripts
userContentController.removeAllUserScripts()
// Re-add only non-boost scripts (those not in our tracked list)
let boostScriptSources = Set(currentBoostScripts.map { $0.source })
-// for script in allScripts {
-// if !boostScriptSources.contains(script.source) {
-// userContentController.addUserScript(script)
-// }
-// }
-// MARK: This causes the browser to crash when loading boosted pages
+ for script in allScripts {
+ if !boostScriptSources.contains(script.source) {
+ userContentController.addUserScript(script)
+ }
+ }
currentBoostScripts.removeAll()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let boostScriptSources = Set(currentBoostScripts.map { $0.source }) | |
| for script in allScripts { | |
| if !boostScriptSources.contains(script.source) { | |
| userContentController.addUserScript(script) | |
| } | |
| } | |
| // for script in allScripts { | |
| // if !boostScriptSources.contains(script.source) { | |
| // userContentController.addUserScript(script) | |
| // } | |
| // } | |
| // MARK: This causes the browser to crash when loading boosted pages | |
| let boostScriptSources = Set(currentBoostScripts.map { $0.source }) | |
| for script in allScripts { | |
| if !boostScriptSources.contains(script.source) { | |
| userContentController.addUserScript(script) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Nook/Models/Tab/Tab.swift` around lines 449 - 456, The if branch that removes
boost scripts currently clears user scripts without restoring non-boost scripts;
update the removal branch in Tab.swift to mirror the else branch by building
boostScriptSources from currentBoostScripts and then iterating allScripts to
call userContentController.addUserScript(script) for any script whose source is
not in boostScriptSources (i.e., restore non-boost scripts), ensuring you use
the same symbols currentBoostScripts, boostScriptSources, allScripts and
userContentController.addUserScript as in the existing code.
Comment on lines
+1
to
+6
| // | ||
| // FInalStage.swift | ||
| // Nook | ||
| // | ||
| // Created by Maciek Bagiński on 19/02/2026. | ||
| // |
There was a problem hiding this comment.
Filename typo: "FInalStage.swift" should be "FinalStage.swift".
The filename has an incorrect capitalization ("FInalStage" with capital 'I' instead of lowercase 'i'). This creates inconsistency between the filename and the struct name FinalStage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Onboarding/Stages/FInalStage.swift` around lines 1 - 6, Rename the file from
"FInalStage.swift" to "FinalStage.swift" to match the struct name and correct
capitalization; ensure the declared type/struct FinalStage remains unchanged and
update any imports or references if your project tooling caches the old filename
so all references to FinalStage resolve correctly.
baginski11
force-pushed
the
feat/onboarding-safari-import
branch
from
d99e950 to
4cf8d85
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
Onboarding/Stages/TabLayoutStage.swift (1)
25-47: Consider extracting the repeated layout-option UI into a shared component.
layoutOptionis structurally identical to helpers inAiChatStage,AdBlockerStage, andURLBarStage. A shared component would reduce duplication and keep selection styling consistent across stages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Onboarding/Stages/TabLayoutStage.swift` around lines 25 - 47, Extract the repeated UI into a reusable SwiftUI view (e.g., LayoutOptionView) that takes parameters for image: String, label: String, layout: TabLayout and a Binding<TabLayout> for the current selection instead of duplicating the logic in layoutOption; move the image/overlay/selection/animation/style logic from the current private func layoutOption and the identical helpers in AiChatStage, AdBlockerStage, and URLBarStage into that new view, then replace each existing helper call with the new LayoutOptionView and pass $selectedLayout so selection updates correctly and styling remains consistent across stages.Nook/Managers/ImportManager/ImportManager.swift (1)
30-55: Consider offloading file I/O off the main actor.Both new async methods do synchronous disk reads and JSON parsing. If they’re called from the main actor, onboarding can hitch. Either ensure they’re invoked from a background task or move the heavy work off-main inside these methods.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Nook/Managers/ImportManager/ImportManager.swift` around lines 30 - 55, The methods importDiaData and importSafariData perform synchronous disk I/O and JSON parsing which can block the main actor; move those heavy operations onto a background task inside each method (e.g., wrap FileManager checks, parseDiaData(from:), validateSafariExport(at:), and parseSafariExport(...) inside a Task.detached or DispatchQueue.global block and await its result) so the public async methods remain safe to call from the main actor; ensure you capture and rethrow/convert errors inside that background task and return the same DiaImportResult/SafariImportResult shapes on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Nook/Assets.xcassets/Browser` Logos/firefox-logo.imageset/Contents.json:
- Around line 8-15: The Contents.json currently lists image entries with
"idiom":"universal" and "scale":"2x"/"3x" but no "filename" values, which causes
Retina devices to upsample the 1x asset; fix by either adding the actual `@2x` and
`@3x` PNG files and populate their corresponding entries' "filename" fields in
Contents.json (match the base asset name with `@2x/`@3x suffix), or if you only
intend a single 1x image, remove the objects that have "scale":"2x" and
"scale":"3x" so only the 1x entry remains.
In `@Nook/Managers/BrowserManager/BrowserManager.swift`:
- Around line 2284-2288: The loop that adds top tabs force-unwraps
self.tabManager.spaces.first, which can crash if no spaces exist; change the
logic in the BrowserManager loop over result.topTabs to safely unwrap or obtain
a valid space (e.g., use guard let space = self.tabManager.spaces.first else {
/* skip or create a new space */ continue } ), then call
self.tabManager.createNewTab(url: topTab.url, in: space) and
self.tabManager.addToEssentials(tab) using that safe space; avoid using the
forced unwrap and handle the empty-space case (skip adding or create a new space
via the appropriate TabManager API).
In `@Nook/Managers/ImportManager/Safari.swift`:
- Around line 67-103: In validateSafariExport(at:), if History.json exists but
fails JSON decoding or metadata validation the function currently returns nil
even when Bookmarks.html is present; change the logic so that after attempting
to load/validate History.json (using JSONDecoder to SafariHistoryFile for
historyURL) a failure does not immediately return nil but instead: if
hasBookmarks is true compute bookmarkCount via countBookmarks(at: bookmarksURL)
and return a SafariExportContents with hasHistory true?no (set hasHistory to
false), historyCount 0 and the bookmark info; otherwise (no bookmarks) return
nil. Update the block that references historyURL/SafariHistoryFile/JSONDecoder
to fall through to this bookmark-only return on decode/validation failure.
- Around line 139-176: The folder-tracking logic in parseSafariBookmarks
currently uses a single currentFolder and sets it to nil on every "</DL>", which
loses parent folder context for nested folders; replace this with a stack (e.g.,
currentFolderStack) inside parseSafariBookmarks: push the folder name onto the
stack when you parse a "<DT><H3" (use the same extraction logic that assigns
currentFolder), use the top of the stack for the SafariBookmark.folder when
parsing "<DT><A HREF=", and pop the stack when encountering "</DL>" so nested
folders restore their parent folder correctly (set folder to stack.last or nil
when stack is empty).
In `@Onboarding/Components/TransitionView.swift`:
- Around line 11-19: The WindowReader currently calls onChange every update
which can trigger state mutations and a render loop; modify WindowReader to
track the last observed window (use a Coordinator with a weak var lastWindow:
NSWindow?) created via makeCoordinator(), then in updateNSView(_:,context:) get
guard let window = nsView.window else { context.coordinator.lastWindow = nil;
return } and only dispatch onChange if context.coordinator.lastWindow !==
window, updating context.coordinator.lastWindow = window after calling onChange;
keep the DispatchQueue.main.async call but gate it behind that inequality check
to prevent repeated callbacks.
In `@Onboarding/Components/ViewTransition.metal`:
- Around line 25-31: Guard against divide-by-zero by clamping size before any
division: create a safeSize (clamped to a small epsilon, e.g. 1e-6) and use it
instead of size when computing uv, aspect and maxDist; update uses in the
uv/aspect/dist/maxDist calculations (the variables uv, aspect, dist, maxDist,
distorted, mask and the smoothstep call in ViewTransition.metal) so no division
by zero or NaNs occur during initial layout.
In `@Onboarding/OnboardingView.swift`:
- Around line 107-139: The advance() function may increment currentStage to 8
after calling applySettings() when currentStage == 7; update advance() (the
currentStage handling logic in the advance method) to stop further processing
after applySettings() — e.g., call applySettings() and immediately return (or
otherwise prevent the later currentStage += 1 path) so currentStage remains
within the 0...7 range and no transient invalid UI state occurs.
In `@Onboarding/Stages/ImportStage.swift`:
- Around line 50-67: The button highlights even for disabled browsers because
the background check uses selectedBrowser == browser regardless of enabled
state; update the logic in browserButton(image:browser:) so the highlight only
shows when the browser is implemented (e.g., use selectedBrowser == browser &&
browser.isImplemented) or ensure selectedBrowser is cleared/normalized upstream
for non-implemented Browsers; reference selectedBrowser and
Browsers.isImplemented when making the change so disabled cases no longer render
the selected background.
---
Duplicate comments:
In `@Nook/Assets.xcassets/sidebar.imageset/Contents.json`:
- Around line 8-15: The Contents.json declares image entries with "scale" : "2x"
and "scale" : "3x" but no "filename" fields, causing 1x assets to be upscaled on
Retina; update the sidebar.imageset Contents.json by either adding matching
high-resolution filenames (e.g., sidebar@2x.png and sidebar@3x.png) to the 2x/3x
entries and ensure those image files are added to the asset bundle, or remove
the 2x/3x entries entirely if you only ship a single scale; modify the entries
that contain "scale" : "2x" and "scale" : "3x" accordingly so the asset catalog
references actual files or omits the empty slots.
In `@Nook/Managers/BrowserManager/BrowserManager.swift`:
- Around line 2351-2360: The loop that imports Safari history (iterating over
result.history) calls historyManager.addVisit with profileId: nil which risks
merging entries into the default profile; change that call to pass the active
profile's identifier (e.g. use the BrowserManager's currentProfile.id or
activeProfileId property) so each imported entry is bound to the current profile
(update the historyManager.addVisit call site inside the for entry in
result.history loop to supply the active profileId instead of nil).
- Around line 2251-2261: The loop currently calls
tabManager.createSpace(name:icon:) and then re-looks up the created Space by
matching name on tabManager.spaces (risking wrong match if a space with the same
name exists); change createSpace to return the created Space (or add an overload
that returns Space) and update this call to capture that returned Space (e.g.,
let createdSpace = tabManager.createSpace(...)) and use it directly instead of
searching tabManager.spaces by name; update any callers of createSpace to handle
the new return value or provide a void overload if needed, and remove the guard
that searches by name.
---
Nitpick comments:
In `@Nook/Managers/ImportManager/ImportManager.swift`:
- Around line 30-55: The methods importDiaData and importSafariData perform
synchronous disk I/O and JSON parsing which can block the main actor; move those
heavy operations onto a background task inside each method (e.g., wrap
FileManager checks, parseDiaData(from:), validateSafariExport(at:), and
parseSafariExport(...) inside a Task.detached or DispatchQueue.global block and
await its result) so the public async methods remain safe to call from the main
actor; ensure you capture and rethrow/convert errors inside that background task
and return the same DiaImportResult/SafariImportResult shapes on failure.
In `@Onboarding/Stages/TabLayoutStage.swift`:
- Around line 25-47: Extract the repeated UI into a reusable SwiftUI view (e.g.,
LayoutOptionView) that takes parameters for image: String, label: String,
layout: TabLayout and a Binding<TabLayout> for the current selection instead of
duplicating the logic in layoutOption; move the
image/overlay/selection/animation/style logic from the current private func
layoutOption and the identical helpers in AiChatStage, AdBlockerStage, and
URLBarStage into that new view, then replace each existing helper call with the
new LayoutOptionView and pass $selectedLayout so selection updates correctly and
styling remains consistent across stages.
Comment on lines
+8
to
+15
| { | ||
| "idiom" : "universal", | ||
| "scale" : "2x" | ||
| }, | ||
| { | ||
| "idiom" : "universal", | ||
| "scale" : "3x" | ||
| } |
There was a problem hiding this comment.
Provide 2x/3x image files (or remove those entries).
The 2x/3x slots have no filenames, so Retina devices will scale the 1x image and the logo may look blurry. Consider adding the @2x/@3x PNGs or deleting those entries if only 1x is intended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Nook/Assets.xcassets/Browser` Logos/firefox-logo.imageset/Contents.json
around lines 8 - 15, The Contents.json currently lists image entries with
"idiom":"universal" and "scale":"2x"/"3x" but no "filename" values, which causes
Retina devices to upsample the 1x asset; fix by either adding the actual `@2x` and
`@3x` PNG files and populate their corresponding entries' "filename" fields in
Contents.json (match the base asset name with `@2x/`@3x suffix), or if you only
intend a single 1x image, remove the objects that have "scale":"2x" and
"scale":"3x" so only the 1x entry remains.
Comment on lines
+67
to
+103
| func validateSafariExport(at directoryURL: URL) -> SafariExportContents? { | ||
| let fm = FileManager.default | ||
|
|
||
| let bookmarksURL = directoryURL.appendingPathComponent("Bookmarks.html") | ||
| let historyURL = directoryURL.appendingPathComponent("History.json") | ||
|
|
||
| let hasBookmarks = fm.fileExists(atPath: bookmarksURL.path) | ||
| let hasHistory = fm.fileExists(atPath: historyURL.path) | ||
|
|
||
| guard hasBookmarks || hasHistory else { return nil } | ||
|
|
||
| if hasHistory { | ||
| guard let data = try? Data(contentsOf: historyURL), | ||
| let json = try? JSONDecoder().decode(SafariHistoryFile.self, from: data), | ||
| json.metadata.browser_name == "Safari" | ||
| else { return nil } | ||
|
|
||
| let bookmarkCount = hasBookmarks ? countBookmarks(at: bookmarksURL) : 0 | ||
|
|
||
| return SafariExportContents( | ||
| directoryURL: directoryURL, | ||
| hasBookmarks: hasBookmarks, | ||
| hasHistory: hasHistory, | ||
| bookmarkCount: bookmarkCount, | ||
| historyCount: json.history.count | ||
| ) | ||
| } | ||
|
|
||
| let bookmarkCount = hasBookmarks ? countBookmarks(at: bookmarksURL) : 0 | ||
| return SafariExportContents( | ||
| directoryURL: directoryURL, | ||
| hasBookmarks: hasBookmarks, | ||
| hasHistory: hasHistory, | ||
| bookmarkCount: bookmarkCount, | ||
| historyCount: 0 | ||
| ) | ||
| } |
There was a problem hiding this comment.
Allow bookmark-only imports when History.json is invalid.
If History.json exists but fails validation, the function returns nil even when bookmarks are present. That blocks a valid bookmark-only import.
🐛 Proposed fix
if hasHistory {
guard let data = try? Data(contentsOf: historyURL),
let json = try? JSONDecoder().decode(SafariHistoryFile.self, from: data),
json.metadata.browser_name == "Safari"
- else { return nil }
+ else {
+ if hasBookmarks {
+ let bookmarkCount = countBookmarks(at: bookmarksURL)
+ return SafariExportContents(
+ directoryURL: directoryURL,
+ hasBookmarks: true,
+ hasHistory: false,
+ bookmarkCount: bookmarkCount,
+ historyCount: 0
+ )
+ }
+ return nil
+ }
let bookmarkCount = hasBookmarks ? countBookmarks(at: bookmarksURL) : 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Nook/Managers/ImportManager/Safari.swift` around lines 67 - 103, In
validateSafariExport(at:), if History.json exists but fails JSON decoding or
metadata validation the function currently returns nil even when Bookmarks.html
is present; change the logic so that after attempting to load/validate
History.json (using JSONDecoder to SafariHistoryFile for historyURL) a failure
does not immediately return nil but instead: if hasBookmarks is true compute
bookmarkCount via countBookmarks(at: bookmarksURL) and return a
SafariExportContents with hasHistory true?no (set hasHistory to false),
historyCount 0 and the bookmark info; otherwise (no bookmarks) return nil.
Update the block that references historyURL/SafariHistoryFile/JSONDecoder to
fall through to this bookmark-only return on decode/validation failure.
Comment on lines
+139
to
+176
| func parseSafariBookmarks(from fileURL: URL) throws -> [SafariBookmark] { | ||
| let html = try String(contentsOf: fileURL, encoding: .utf8) | ||
| var bookmarks: [SafariBookmark] = [] | ||
| var currentFolder: String? = nil | ||
|
|
||
| for line in html.components(separatedBy: "\n") { | ||
| let trimmed = line.trimmingCharacters(in: .whitespaces) | ||
|
|
||
| if trimmed.hasPrefix("<DT><H3") { | ||
| if let startRange = trimmed.range(of: ">", range: trimmed.index(after: trimmed.startIndex)..<trimmed.endIndex), | ||
| let endRange = trimmed.range(of: "</H3>", options: .caseInsensitive) { | ||
| currentFolder = String(trimmed[startRange.upperBound..<endRange.lowerBound]) | ||
| } | ||
| } | ||
|
|
||
| if trimmed.hasPrefix("<DT><A HREF=") { | ||
| if let hrefStart = trimmed.range(of: "HREF=\"", options: .caseInsensitive), | ||
| let hrefEnd = trimmed.range(of: "\"", range: hrefStart.upperBound..<trimmed.endIndex), | ||
| let titleStart = trimmed.range(of: ">", range: hrefEnd.upperBound..<trimmed.endIndex), | ||
| let titleEnd = trimmed.range(of: "</A>", options: .caseInsensitive) { | ||
| let url = String(trimmed[hrefStart.upperBound..<hrefEnd.lowerBound]) | ||
| let title = String(trimmed[titleStart.upperBound..<titleEnd.lowerBound]) | ||
|
|
||
| bookmarks.append(SafariBookmark( | ||
| title: title, | ||
| url: url, | ||
| folder: currentFolder | ||
| )) | ||
| } | ||
| } | ||
|
|
||
| if trimmed.hasPrefix("</DL>") { | ||
| currentFolder = nil | ||
| } | ||
| } | ||
|
|
||
| return bookmarks | ||
| } |
There was a problem hiding this comment.
Folder tracking drops parent context for nested folders.
Resetting currentFolder to nil on every </DL> loses the parent folder when nested folders close, so some bookmarks get flattened unintentionally. A small stack keeps nesting consistent.
🐛 Proposed fix
- var currentFolder: String? = nil
+ var folderStack: [String] = []
@@
- currentFolder = String(trimmed[startRange.upperBound..<endRange.lowerBound])
+ folderStack.append(String(trimmed[startRange.upperBound..<endRange.lowerBound]))
}
}
@@
- bookmarks.append(SafariBookmark(
+ bookmarks.append(SafariBookmark(
title: title,
url: url,
- folder: currentFolder
+ folder: folderStack.last
))
}
}
if trimmed.hasPrefix("</DL>") {
- currentFolder = nil
+ if !folderStack.isEmpty { folderStack.removeLast() }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Nook/Managers/ImportManager/Safari.swift` around lines 139 - 176, The
folder-tracking logic in parseSafariBookmarks currently uses a single
currentFolder and sets it to nil on every "</DL>", which loses parent folder
context for nested folders; replace this with a stack (e.g., currentFolderStack)
inside parseSafariBookmarks: push the folder name onto the stack when you parse
a "<DT><H3" (use the same extraction logic that assigns currentFolder), use the
top of the stack for the SafariBookmark.folder when parsing "<DT><A HREF=", and
pop the stack when encountering "</DL>" so nested folders restore their parent
folder correctly (set folder to stack.last or nil when stack is empty).
Comment on lines
+11
to
+19
| private struct WindowReader: NSViewRepresentable { | ||
| let onChange: (NSWindow) -> Void | ||
|
|
||
| func makeNSView(context: Context) -> NSView { NSView() } | ||
|
|
||
| func updateNSView(_ nsView: NSView, context: Context) { | ||
| guard let window = nsView.window else { return } | ||
| DispatchQueue.main.async { self.onChange(window) } | ||
| } |
There was a problem hiding this comment.
Prevent a view update loop in WindowReader.
updateNSView schedules onChange on every update, and onChange mutates state, which can cause a continuous render/update loop. Gate the callback on window changes.
🔧 Suggested fix
private struct WindowReader: NSViewRepresentable {
let onChange: (NSWindow) -> Void
+
+ class Coordinator {
+ var window: NSWindow?
+ }
+
+ func makeCoordinator() -> Coordinator { Coordinator() }
func makeNSView(context: Context) -> NSView { NSView() }
func updateNSView(_ nsView: NSView, context: Context) {
guard let window = nsView.window else { return }
+ guard context.coordinator.window !== window else { return }
+ context.coordinator.window = window
DispatchQueue.main.async { self.onChange(window) }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private struct WindowReader: NSViewRepresentable { | |
| let onChange: (NSWindow) -> Void | |
| func makeNSView(context: Context) -> NSView { NSView() } | |
| func updateNSView(_ nsView: NSView, context: Context) { | |
| guard let window = nsView.window else { return } | |
| DispatchQueue.main.async { self.onChange(window) } | |
| } | |
| private struct WindowReader: NSViewRepresentable { | |
| let onChange: (NSWindow) -> Void | |
| class Coordinator { | |
| var window: NSWindow? | |
| } | |
| func makeCoordinator() -> Coordinator { Coordinator() } | |
| func makeNSView(context: Context) -> NSView { NSView() } | |
| func updateNSView(_ nsView: NSView, context: Context) { | |
| guard let window = nsView.window else { return } | |
| guard context.coordinator.window !== window else { return } | |
| context.coordinator.window = window | |
| DispatchQueue.main.async { self.onChange(window) } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Onboarding/Components/TransitionView.swift` around lines 11 - 19, The
WindowReader currently calls onChange every update which can trigger state
mutations and a render loop; modify WindowReader to track the last observed
window (use a Coordinator with a weak var lastWindow: NSWindow?) created via
makeCoordinator(), then in updateNSView(_:,context:) get guard let window =
nsView.window else { context.coordinator.lastWindow = nil; return } and only
dispatch onChange if context.coordinator.lastWindow !== window, updating
context.coordinator.lastWindow = window after calling onChange; keep the
DispatchQueue.main.async call but gate it behind that inequality check to
prevent repeated callbacks.
Comment on lines
+25
to
+31
| float2 uv = position / size; | ||
| float aspect = size.x / size.y; | ||
| float dist = length((uv - 0.5) * float2(aspect, 1.0)); | ||
| float maxDist = length(float2(0.5 * aspect, 0.5)); | ||
| float n = noise(uv * 6.0 + progress * 2.0); | ||
| float distorted = (dist / maxDist) + (n - 0.5) * 0.12; | ||
| float mask = smoothstep(progress - 0.12, progress + 0.12, distorted); |
There was a problem hiding this comment.
Guard against zero-size inputs to avoid NaNs.
If size.x or size.y is 0 (e.g., during initial layout), the division can produce NaNs and cause a blank/garbled transition. Clamping to a minimum size prevents this edge-case.
🔧 Suggested fix
- float2 uv = position / size;
- float aspect = size.x / size.y;
- float dist = length((uv - 0.5) * float2(aspect, 1.0));
- float maxDist = length(float2(0.5 * aspect, 0.5));
+ float2 safeSize = max(size, float2(1.0));
+ float2 uv = position / safeSize;
+ float aspect = safeSize.x / safeSize.y;
+ float dist = length((uv - 0.5) * float2(aspect, 1.0));
+ float maxDist = length(float2(0.5 * aspect, 0.5));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| float2 uv = position / size; | |
| float aspect = size.x / size.y; | |
| float dist = length((uv - 0.5) * float2(aspect, 1.0)); | |
| float maxDist = length(float2(0.5 * aspect, 0.5)); | |
| float n = noise(uv * 6.0 + progress * 2.0); | |
| float distorted = (dist / maxDist) + (n - 0.5) * 0.12; | |
| float mask = smoothstep(progress - 0.12, progress + 0.12, distorted); | |
| float2 safeSize = max(size, float2(1.0)); | |
| float2 uv = position / safeSize; | |
| float aspect = safeSize.x / safeSize.y; | |
| float dist = length((uv - 0.5) * float2(aspect, 1.0)); | |
| float maxDist = length(float2(0.5 * aspect, 0.5)); | |
| float n = noise(uv * 6.0 + progress * 2.0); | |
| float distorted = (dist / maxDist) + (n - 0.5) * 0.12; | |
| float mask = smoothstep(progress - 0.12, progress + 0.12, distorted); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Onboarding/Components/ViewTransition.metal` around lines 25 - 31, Guard
against divide-by-zero by clamping size before any division: create a safeSize
(clamped to a small epsilon, e.g. 1e-6) and use it instead of size when
computing uv, aspect and maxDist; update uses in the uv/aspect/dist/maxDist
calculations (the variables uv, aspect, dist, maxDist, distorted, mask and the
smoothstep call in ViewTransition.metal) so no division by zero or NaNs occur
during initial layout.
Comment on lines
+107
to
+139
| private func advance() { | ||
| guard currentStage < 8 else { return } | ||
| if currentStage == 7 { | ||
| applySettings() | ||
| } | ||
|
|
||
| // When tab layout is topOfWindow, skip URL bar stage (forced to top of website) | ||
| if currentStage == 4 && selectedTabLayout == .topOfWindow { | ||
| withAnimation(.easeInOut(duration: 0.25)) { | ||
| currentStage = 6 | ||
| } | ||
| return | ||
| } | ||
|
|
||
| if currentStage == 1 && selectedBrowser == .safari { | ||
| withAnimation(.easeInOut(duration: 0.25)) { | ||
| showSafariImportFlow = true | ||
| } | ||
| } else if currentStage == 1 { | ||
| withAnimation(.easeInOut(duration: 0.25)) { | ||
| isLoading = true | ||
| } | ||
| performImport { | ||
| withAnimation(.easeInOut(duration: 0.25)) { | ||
| currentStage += 1 | ||
| isLoading = false | ||
| } | ||
| } | ||
| } else { | ||
| withAnimation(.easeInOut(duration: 0.25)) { | ||
| currentStage += 1 | ||
| } | ||
| } |
There was a problem hiding this comment.
Prevent advancing past the final stage.
currentStage == 7 calls applySettings() but then continues and increments to 8. Keeping the stage within 0...7 avoids transient invalid UI state.
🐛 Proposed fix
private func advance() {
guard currentStage < 8 else { return }
if currentStage == 7 {
applySettings()
+ return
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Onboarding/OnboardingView.swift` around lines 107 - 139, The advance()
function may increment currentStage to 8 after calling applySettings() when
currentStage == 7; update advance() (the currentStage handling logic in the
advance method) to stop further processing after applySettings() — e.g., call
applySettings() and immediately return (or otherwise prevent the later
currentStage += 1 path) so currentStage remains within the 0...7 range and no
transient invalid UI state occurs.
Comment on lines
+50
to
+67
| private func browserButton(image: String, browser: Browsers) -> some View { | ||
| let enabled = browser.isImplemented | ||
| Button { | ||
| if enabled { | ||
| selectedBrowser = browser | ||
| } | ||
| } label: { | ||
| Image(image) | ||
| .resizable() | ||
| .scaledToFit() | ||
| .frame(width: 17, height: 17) | ||
| .frame(width: 44, height: 44) | ||
| .background(selectedBrowser == browser ? .white : .white.opacity(enabled ? 0.5 : 0.2)) | ||
| .clipShape(RoundedRectangle(cornerRadius: 12)) | ||
| .animation(.easeInOut(duration: 0.1), value: selectedBrowser == browser) | ||
| } | ||
| .buttonStyle(ScaleButtonStyle()) | ||
| .disabled(!enabled) |
There was a problem hiding this comment.
Disabled browsers can still appear selected.
If selectedBrowser is a non-implemented case, the button renders the selected background even though it’s disabled, which is confusing. Consider only highlighting when enabled or normalize the selection upstream.
🔧 Suggested fix
- .background(selectedBrowser == browser ? .white : .white.opacity(enabled ? 0.5 : 0.2))
+ .background(
+ selectedBrowser == browser && enabled
+ ? .white
+ : .white.opacity(enabled ? 0.5 : 0.2)
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Onboarding/Stages/ImportStage.swift` around lines 50 - 67, The button
highlights even for disabled browsers because the background check uses
selectedBrowser == browser regardless of enabled state; update the logic in
browserButton(image:browser:) so the highlight only shows when the browser is
implemented (e.g., use selectedBrowser == browser && browser.isImplemented) or
ensure selectedBrowser is cleared/normalized upstream for non-implemented
Browsers; reference selectedBrowser and Browsers.isImplemented when making the
change so disabled cases no longer render the selected background.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add onboarding flow with safari and dia import
Summary by CodeRabbit
New Features
Improvements