Skip to content

Fix desktop sidebar hosting layout glitch#5781

Merged
kodjima33 merged 1 commit intomainfrom
fix/sidebar-hosting-followup
Mar 18, 2026
Merged

Fix desktop sidebar hosting layout glitch#5781
kodjima33 merged 1 commit intomainfrom
fix/sidebar-hosting-followup

Conversation

@kodjima33
Copy link
Copy Markdown
Collaborator

Summary

  • remove the custom click-through host wrapper from the desktop sidebar
  • keep the existing settings fade/hit-testing behavior without the extra hosting layer
  • address the sidebar icon/label desync still visible after v0.11.130

Verification

  • cd desktop && xcrun swift build -c debug --package-path Desktop

@kodjima33 kodjima33 merged commit ebbe75c into main Mar 18, 2026
2 checks passed
@kodjima33 kodjima33 deleted the fix/sidebar-hosting-followup branch March 18, 2026 06:59
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 18, 2026

Greptile Summary

This PR removes a single .clickThrough(enabled: !isInSettings) modifier from the SidebarView in DesktopHomeView, eliminating the extra NSViewRepresentable (ClickThroughView) hosting layer that was previously wrapping the sidebar. The intent is to fix a sidebar icon/label desync layout glitch introduced by that extra AppKit/SwiftUI boundary while preserving the settings-mode hide/hit-test behaviour via the existing .opacity and .allowsHitTesting modifiers.

  • The settings-mode fade and hit-testing behaviour (opacity + allowsHitTesting) is correctly preserved.
  • The activation-click behaviour provided by ClickThroughHostingView — specifically acceptsFirstMouse(for:) → true and the pending-click re-dispatch mechanism — is not preserved. The sidebar will now require two clicks to interact with when the app window is not in focus (first click focuses the window; second click selects the item), which is a potential UX regression.
  • The special-case guard in disableMinSizeComputation that skips ClickThroughHostingView is now dead code and the associated comment is stale; both should be cleaned up.

Confidence Score: 3/5

  • Safe to merge if the loss of single-click activation on a non-key window is an accepted trade-off; otherwise the activation-click regression should be addressed first.
  • The change is a one-line removal that fixes a documented layout glitch, and the settings-mode behaviour is correctly preserved. However, ClickThroughHostingView.acceptsFirstMouse and the pending-click re-dispatch logic (which ensured sidebar items could be activated in a single click on a non-focused window) are silently lost. This is a real behavioural regression that the PR description does not acknowledge. Additionally, stale dead-code remains in disableMinSizeComputation.
  • desktop/Desktop/Sources/MainWindow/DesktopHomeView.swift — specifically the disableMinSizeComputation dead-code block and the sidebar interaction behaviour at lines 538–544.

Important Files Changed

Filename Overview
desktop/Desktop/Sources/MainWindow/DesktopHomeView.swift Removes the .clickThrough(enabled: !isInSettings) modifier from SidebarView. Fixes the layout glitch caused by the extra NSViewRepresentable boundary, but leaves stale dead-code in disableMinSizeComputation and may regress "accept first mouse" / activation-click behaviour for the sidebar.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[User clicks on Sidebar] --> B{Window is key?}

    subgraph BEFORE["Before PR (with .clickThrough)"]
        B1{Window is key?} -->|Yes| C1[ClickThroughHostingView\nhitTest passes through]
        B1 -->|No| D1[acceptsFirstMouse = true\nCaptures pending click location]
        D1 --> E1[windowDidBecomeKey fires]
        E1 --> F1[Synthetic click re-sent\nSidebar item selected ✓]
        C1 --> G1[allowsHitTesting check\nSwiftUI dispatches event\nSidebar item selected ✓]
    end

    subgraph AFTER["After PR (without .clickThrough)"]
        B2{Window is key?} -->|Yes| C2[Standard NSHostingView\nhitTest]
        B2 -->|No| D2[acceptsFirstMouse = false\ndefault NSHostingView]
        D2 --> E2[First click only activates window\nSidebar item NOT selected ✗]
        C2 --> G2[allowsHitTesting check\nSwiftUI dispatches event\nSidebar item selected ✓]
    end

    B --> B1
    B --> B2
Loading

Comments Outside Diff (1)

  1. desktop/Desktop/Sources/MainWindow/DesktopHomeView.swift, line 347-360 (link)

    P2 Stale dead-code in disableMinSizeComputation

    The guard on lines 355–360 that skips ClickThroughHostingView was added specifically because it wrapped the sidebar and needed its intrinsicContentSize intact for .fixedSize() to compute the correct sidebar width. Now that .clickThrough(…) has been removed from the sidebar, no ClickThroughHostingView will be found in this traversal for the sidebar path, making the check dead code.

    The accompanying comment on lines 347–348 is also now misleading. Consider removing both the guard block and updating (or removing) the comment to reflect the new layout approach, otherwise future readers may be confused about a guard that never fires for its documented reason.

      private static func disableMinSizeComputation(in window: NSWindow) {
        func visit(_ view: NSView) {
          if let hosting = view as? any HostingSizingConfigurable {
            let before = hosting.sizingOptions
            hosting.sizingOptions = []
            log("DesktopHomeView: Set sizingOptions on \(type(of: view)): \(before) → []")
          }
          for subview in view.subviews {
            visit(subview)
          }
        }
        if let contentView = window.contentView {
          visit(contentView)
        }
      }

Last reviewed commit: "Fix desktop sidebar ..."

Comment on lines 543 to 544
.opacity(isInSettings ? 0 : 1)
.allowsHitTesting(!isInSettings)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Loss of "accept first mouse" / activation-click behaviour

ClickThroughHostingView served two purposes that are not replicated by .allowsHitTesting:

  1. acceptsFirstMouse(for:) → true — allows the first click on the sidebar (when the app window is not the key window) to both activate the window and select the sidebar item in a single gesture. A plain NSHostingView returns false here, so without the wrapper the first click on the sidebar in a non-key window will only focus the window; a second click is then required to actually select an item.

  2. Pending-click re-dispatch — the windowDidBecomeKey handler in ClickThroughHostingView captured the original click location and synthetically re-sent it after the window became key, guaranteeing the sidebar item was activated regardless of the window-focus sequence.

Both mechanisms are lost with this removal. .allowsHitTesting(!isInSettings) only controls SwiftUI-level event dispatch after AppKit has already decided the NSView is the hit target; it does not replace acceptsFirstMouse or the re-dispatch logic.

If the activation-click regression is acceptable as a trade-off for fixing the layout glitch, that should be documented explicitly in the code or PR.

Glucksberg pushed a commit to Glucksberg/omi-local that referenced this pull request Apr 28, 2026
## Summary
- remove the custom click-through host wrapper from the desktop sidebar
- keep the existing settings fade/hit-testing behavior without the extra
hosting layer
- address the sidebar icon/label desync still visible after v0.11.130

## Verification
- cd desktop && xcrun swift build -c debug --package-path Desktop
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant