fix: improve sidebar animations#331
Conversation
… flash Replace the two separate SpacesSideBarView containers (HStack layout and SidebarHoverOverlayView) with a single instance rendered as an overlay. A Color.clear spacer animates width to push web content when pinned. The floating sidebar slides in/out via offset instead of being conditionally created/destroyed, preserving view identity and state. - Delete SidebarHoverOverlayView.swift (logic moved to WindowView) - Increase toggleSidebar animation from 0.1s to 0.2s for smoother transition - Animate hover dismissal when sidebar becomes pinned Co-Authored-By: Claude Opus 4.6 <[email protected]>
Replace branching if/else HStack with fixed-order ZStack layout so WebContent keeps stable view identity when swapping sidebar sides. Sidebar and AI panels slide under web content (zIndex layering) during the swap animation. When unpinned, sidebar floats above as before. - Add peekOverlay to HoverSidebarManager so the floating sidebar briefly appears on its new side for 2s after a position swap - Use layoutDirection environment to flip SidebarMenu tab placement - Wrap all position setters (context menu, settings) in withAnimation Co-Authored-By: Claude Opus 4.6 <[email protected]>
Matches Arc-style instant snap when pinning the sidebar while preserving the smooth ease-out animation when unpinning. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…sible Animate sidebar toggle in both directions (pin and unpin). When the floating sidebar is already showing via hover, pinning is instant since the sidebar is already visually present. Co-Authored-By: Claude Opus 4.6 <[email protected]>
…debar animation Use each tab's pageBackgroundColor instead of windowBackgroundColor for webview and split pane backgrounds. Switch sidebar pin/unpin animation to .smooth(duration: 0.1) for a snappier feel. Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR refactors sidebar rendering and interaction patterns by consolidating the hover overlay into a unified sidebar component, enhances the hover sidebar manager with peek functionality, updates sidebar toggle logic to account for floating state, adds smooth animations to sidebar position changes, improves keyboard handling in dialogs with tab-key blocking and shortcut hints, and adjusts background color propagation in split-view layouts. Changes
Sequence DiagramsequenceDiagram
participant User as User<br/>(Mouse Movement)
participant HSM as HoverSidebarManager
participant US as UnifiedSidebar
participant WC as WebContent
participant WS as WindowState
User->>HSM: Mouse moves to left edge (unpinned)
activate HSM
HSM->>HSM: Check if in sidebar zone<br/>(isMouseInsideSidebar)
alt During peek window
HSM->>HSM: Update cursor position only<br/>(return early)
else Normal operation
alt Mouse over sidebar
HSM->>HSM: Compute visibility<br/>isOverlayVisible = true
HSM->>US: Trigger animation
else Mouse away from sidebar
HSM->>HSM: Schedule auto-hide<br/>isOverlayVisible = false
HSM->>US: Trigger animation
end
end
deactivate HSM
opt Programmatic Peek (e.g., sidebar position changed)
HSM->>HSM: peekOverlay(for: 2.0)<br/>Set peekUntil deadline
HSM->>HSM: Force isOverlayVisible = true
HSM->>US: Show sidebar with animation
par
US->>US: Render with offset animation
US->>WC: Keep centered
and
HSM->>HSM: Wait for peek duration
HSM->>HSM: Clear peekUntil & resume<br/>normal mouse logic
end
end
US->>WS: Update sidebar position
WS->>WC: Adjust layout (leftWidth/rightWidth)
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🧹 Nitpick comments (1)
Nook/Components/Settings/Tabs/Appearance.swift (1)
32-39: Avoid stacking multiple animations for the samesidebarPositionwrite.Line 35 adds an explicit animation, but
App/Window/WindowView.swift(Line 222-227) already animates this value, andNavigation/Sidebar/SpacesSideBarView.swift(Line 263-278) also animates writes. Prefer a single animation owner to keep transitions predictable.Suggested adjustment
Picker( "Sidebar Position", selection: Binding( get: { settings.sidebarPosition }, set: { newValue in - withAnimation(.smooth(duration: 0.3)) { - settings.sidebarPosition = newValue - } + guard settings.sidebarPosition != newValue else { return } + settings.sidebarPosition = newValue } ) ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Nook/Components/Settings/Tabs/Appearance.swift` around lines 32 - 39, The selection Binding currently wraps writes to settings.sidebarPosition in withAnimation(.smooth(duration: 0.3)), which stacks with other animations that already animate sidebarPosition; remove the explicit animation in the setter so the binding simply assigns settings.sidebarPosition = newValue (or perform the assignment inside a non-animated Transaction) and let the single designated owner (WindowView or SpacesSideBarView) control the transition to avoid conflicting/stacked animations.
🤖 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/Components/Settings/Tabs/Appearance.swift`:
- Around line 32-39: The SettingsAppearanceTab struct is mutating shared state
(settings.sidebarPosition) from the Binding setter and must be confined to the
main actor; add the `@MainActor` annotation to the SettingsAppearanceTab
declaration so all its property accesses and the Binding setter run on the main
thread, then rebuild to ensure no other concurrency diagnostics remain for
methods or properties within SettingsAppearanceTab (referencing
SettingsAppearanceTab, settings.sidebarPosition, and the Binding selection
setter).
In `@Nook/Components/Sidebar/Menu/SidebarMenu.swift`:
- Line 54: Remove the layoutDirection environment override and instead implement
explicit child ordering so icons and text are not mirrored: in SidebarMenu
(where .environment(\.layoutDirection, nookSettings.sidebarPosition == .left ?
.leftToRight : .rightToLeft) is applied) delete that environment modifier and
conditionally order the HStack children based on nookSettings.sidebarPosition —
e.g., place the tabs view (or ForEach over tabs / SidebarTabView) before the
content view when sidebarPosition == .left, and reverse the order when ==
.right; if you need to reverse the tabs array use tabs.reversed() for display
only (do not mutate the source), and keep all other layout/alignments unchanged
so SF Symbols and text remain unmirrored.
In `@Nook/Managers/HoverSidebarManager/HoverSidebarManager.swift`:
- Line 35: The `@Published` property isMouseInsideSidebar in HoverSidebarManager
is being set on every pointer move causing excessive SwiftUI updates; change
assignments to only update when the value actually differs (e.g., compare new
value to current before setting or use a didSet/guard in the setter) so that
isMouseInsideSidebar only publishes when its boolean changes; apply the same fix
to the other occurrence referenced (the second assignment around line with the
other mouse-event handler) so both places check for equality before assigning.
---
Nitpick comments:
In `@Nook/Components/Settings/Tabs/Appearance.swift`:
- Around line 32-39: The selection Binding currently wraps writes to
settings.sidebarPosition in withAnimation(.smooth(duration: 0.3)), which stacks
with other animations that already animate sidebarPosition; remove the explicit
animation in the setter so the binding simply assigns settings.sidebarPosition =
newValue (or perform the assignment inside a non-animated Transaction) and let
the single designated owner (WindowView or SpacesSideBarView) control the
transition to avoid conflicting/stacked animations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09368d2b-fab0-4f94-b2d0-f294c51db824
📒 Files selected for processing (11)
App/Window/WindowView.swiftNavigation/Sidebar/SidebarHeader.swiftNavigation/Sidebar/SpacesSideBarView.swiftNook/Components/Settings/Tabs/Appearance.swiftNook/Components/Sidebar/Menu/SidebarMenu.swiftNook/Components/Sidebar/NavButtonsView.swiftNook/Components/Sidebar/SidebarHoverOverlayView.swiftNook/Components/WebsiteView/WebsiteView.swiftNook/Managers/BrowserManager/BrowserManager.swiftNook/Managers/HoverSidebarManager/HoverSidebarManager.swiftNook/Models/BrowserWindowState.swift
💤 Files with no reviewable changes (1)
- Nook/Components/Sidebar/SidebarHoverOverlayView.swift
| selection: Binding( | ||
| get: { settings.sidebarPosition }, | ||
| set: { newValue in | ||
| withAnimation(.smooth(duration: 0.3)) { | ||
| settings.sidebarPosition = newValue | ||
| } | ||
| } | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "Appearance.swift" | grep -i settingsRepository: nook-browser/Nook
Length of output: 109
🏁 Script executed:
cat -n ./Nook/Components/Settings/Tabs/Appearance.swift | head -50Repository: nook-browser/Nook
Length of output: 1972
🏁 Script executed:
wc -l ./Nook/Components/Settings/Tabs/Appearance.swiftRepository: nook-browser/Nook
Length of output: 112
Add @MainActor annotation to SettingsAppearanceTab struct to ensure thread-safe mutations of shared settings state.
The setter at lines 32–39 mutates settings.sidebarPosition, which is shared state. Per repository policy, all Swift code must be confined to the main actor for thread safety.
Suggested adjustment
+@MainActor
struct SettingsAppearanceTab: View {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Nook/Components/Settings/Tabs/Appearance.swift` around lines 32 - 39, The
SettingsAppearanceTab struct is mutating shared state (settings.sidebarPosition)
from the Binding setter and must be confined to the main actor; add the
`@MainActor` annotation to the SettingsAppearanceTab declaration so all its
property accesses and the Binding setter run on the main thread, then rebuild to
ensure no other concurrency diagnostics remain for methods or properties within
SettingsAppearanceTab (referencing SettingsAppearanceTab,
settings.sidebarPosition, and the Binding selection setter).
…ssary hover publishes Replace .environment(\.layoutDirection) override with explicit child ordering to prevent SF Symbols and text from being mirrored. Guard isMouseInsideSidebar updates to only publish when the value changes.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Nook/Managers/HoverSidebarManager/HoverSidebarManager.swift (1)
14-14:⚠️ Potential issue | 🟠 MajorAdd
@MainActortoHoverSidebarManagerto enforce thread-safe state access.Line 14 declares a non-isolated class that owns
@PublishedUI state and receives mutations from AppKit event monitor closures (lines 75–82) running on arbitrary threads. The repository requires all Swift code to be confined to@MainActor. Wrapping the monitor callbacks withTask {@mainactor}ensures thread-safe access.Proposed fix
+@MainActor final class HoverSidebarManager: ObservableObject { @@ - private func scheduleHandleMouseMovement() { - // Ensure main-actor work since we touch NSApp/window and main-actor BrowserManager - DispatchQueue.main.async { [weak self] in - self?.handleMouseMovementOnMain() - } - } + private func scheduleHandleMouseMovement() { + handleMouseMovementOnMain() + } @@ - localMonitor = NSEvent.addLocalMonitorForEvents(matching: [.mouseMoved, .leftMouseDragged, .rightMouseDragged]) { [weak self] event in - self?.scheduleHandleMouseMovement() + localMonitor = NSEvent.addLocalMonitorForEvents(matching: [.mouseMoved, .leftMouseDragged, .rightMouseDragged]) { [weak self] event in + Task { `@MainActor` in + self?.scheduleHandleMouseMovement() + } return event } @@ - globalMonitor = NSEvent.addGlobalMonitorForEvents(matching: [.mouseMoved, .leftMouseDragged, .rightMouseDragged]) { [weak self] _ in - self?.scheduleHandleMouseMovement() + globalMonitor = NSEvent.addGlobalMonitorForEvents(matching: [.mouseMoved, .leftMouseDragged, .rightMouseDragged]) { [weak self] _ in + Task { `@MainActor` in + self?.scheduleHandleMouseMovement() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Nook/Managers/HoverSidebarManager/HoverSidebarManager.swift` at line 14, Add `@MainActor` to the HoverSidebarManager class declaration to ensure all `@Published` UI state is confined to the main actor, and modify the AppKit event monitor callbacks (the closures that currently mutate HoverSidebarManager state) to perform their state changes inside Task { `@MainActor` in ... } so the mutations run on the main thread; update references to the class name HoverSidebarManager and the monitor callback closures to apply these changes.
🤖 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/Components/Sidebar/Menu/SidebarMenu.swift`:
- Around line 55-66: The SidebarMenu struct and its view content must be
confined to the main actor; add the `@MainActor` attribute to the SidebarMenu type
declaration (so the computed View property content and any child view usage like
SidebarMenuHistoryTab and SidebarMenuDownloadsTab run on the main thread).
Update the struct definition for SidebarMenu to be annotated with `@MainActor` and
ensure any related initializers or extensions for SidebarMenu are also
marked/moved under `@MainActor` as needed.
---
Outside diff comments:
In `@Nook/Managers/HoverSidebarManager/HoverSidebarManager.swift`:
- Line 14: Add `@MainActor` to the HoverSidebarManager class declaration to ensure
all `@Published` UI state is confined to the main actor, and modify the AppKit
event monitor callbacks (the closures that currently mutate HoverSidebarManager
state) to perform their state changes inside Task { `@MainActor` in ... } so the
mutations run on the main thread; update references to the class name
HoverSidebarManager and the monitor callback closures to apply these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c30f4094-0ebe-45b4-b903-2d779bb89a6b
📒 Files selected for processing (2)
Nook/Components/Sidebar/Menu/SidebarMenu.swiftNook/Managers/HoverSidebarManager/HoverSidebarManager.swift
| @ViewBuilder | ||
| private var content: some View { | ||
| VStack { | ||
| switch selectedTab { | ||
| case .history: | ||
| SidebarMenuHistoryTab() | ||
| case .downloads: | ||
| SidebarMenuDownloadsTab() | ||
| } | ||
| } | ||
| .frame(maxWidth: .infinity, maxHeight: .infinity) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the SidebarMenu struct definition and surrounding context
echo "=== Checking SidebarMenu.swift for `@MainActor` annotation ==="
cat -n Nook/Components/Sidebar/Menu/SidebarMenu.swift | head -80Repository: nook-browser/Nook
Length of output: 2544
Add @MainActor to confine this view to the main thread.
The SidebarMenu struct violates the codebase rule requiring all Swift code to be confined to @MainActor for thread safety. Annotate the struct definition at line 35.
Proposed fix
+@MainActor
struct SidebarMenu: View {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Nook/Components/Sidebar/Menu/SidebarMenu.swift` around lines 55 - 66, The
SidebarMenu struct and its view content must be confined to the main actor; add
the `@MainActor` attribute to the SidebarMenu type declaration (so the computed
View property content and any child view usage like SidebarMenuHistoryTab and
SidebarMenuDownloadsTab run on the main thread). Update the struct definition
for SidebarMenu to be annotated with `@MainActor` and ensure any related
initializers or extensions for SidebarMenu are also marked/moved under
`@MainActor` as needed.
|
@coderabbitai summarize your thoughts on the pr + any needed changes for me |
|
@AllDaGearNoIdea thanks so much for this PR. Good fix. Gonna review some before committing |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 23780 🏁 Script executed: Length of output: 12198
Overall AssessmentThis is a well-motivated refactor. The core idea — keeping a single ✅ What's Working Well
|
… keyboard input, closer visual design (nook-browser#329) * fix: allow quitting with Cmd+Q when "warn before quitting" is disabled Replace the "Force Quit App" menu item with a standard Cmd+Q that routes through showQuitDialog(). When "warn before quitting" is off, return .terminateNow immediately to avoid a deadlock in applicationShouldTerminate. * fix: prevent keyboard/mouse interaction with WebView while dialog is open and improve quit dialog design Resign WebView as first responder when a dialog appears so Enter/Escape reach dialog buttons. Disable hit testing on the WebView during dialog display. Add an NSEvent monitor to block Tab key and play the system beep. Update quit dialog copy and icon size. Add customIcon support to DialogButton with a KeycapLabel view for text-based shortcut hints. Simplify duplicate left-button branch in DialogFooter. * chore: trim verbose comments to match project style * fix: address review feedback on dialog focus and actor isolation Remove overly narrow WKWebView type check when resigning first responder. Drop unnecessary nonisolated(unsafe) from tabKeyMonitor. --------- Co-authored-by: AllDaGearNoIdea <[email protected]>
…solation, minor nits - Add .alwaysArrowCursor(when:) to floating sidebar panel so web views underneath don't override the cursor - Mark HoverSidebarManager as @mainactor for compiler-enforced isolation - Simplify dead cornerRadius availability check in WebContent - Extract magic number 14 into named floatingInset constant - Add comment noting dual animation paths for sidebar position Co-Authored-By: Claude Opus 4.6 <[email protected]>
3 participants
NOTE: I've unified sidebar into a single view instance. Previously everything was noticabley re-drawing everything.
When in "unpinned" sidebar mode, there's an animation between sides and it stays open for a couple of seconds to orient the user.
When in "pinned" mode, the toolbar content slides underneath the web-view column.
Switching between pinned/unpinned better matches Arc:
-there's a background colour on the web view area so the gap before content width catches up isn't noticeable
-if the user is hovering over the "unpinned" sidebar, we skip the animation when switching to pinned
-the animation is quicker and smoother: .smooth(duration: 0.1)
NOTE: I'm struggling to build with entitlements, so I can't test this with the opposing AI sidebar visible.
I used Claude Opus 4.6 for help with syntax and structure.
Summary by CodeRabbit
Release Notes
New Features
Improvements