Repository navigation
Fix AppKit sidebar shortcut hint animation - #8589
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe shortcut hint pill now uses separate material and shadow views, guarded visibility animations, reduced-motion handling, identity-aware reuse behavior, smaller fonts, capsule sizing, disabled hit-testing, and expanded tests. ChangesSidebar shortcut hint pill
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant SidebarRow
participant SidebarShortcutHintPillView
participant CALayer
SidebarRow->>SidebarShortcutHintPillView: Configure hint text and identity
SidebarShortcutHintPillView->>SidebarShortcutHintPillView: Set revealed state
SidebarShortcutHintPillView->>CALayer: Animate opacity
CALayer-->>SidebarShortcutHintPillView: Complete guarded transition
SidebarShortcutHintPillView->>CALayer: Hide after valid fade-out
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (23 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR improves command-hold shortcut hints in the AppKit sidebar. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (6): Last reviewed commit: "Reset shortcut hints in cell reuse lifec..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| NSAnimationContext.runAnimationGroup({ context in | ||
| context.duration = ShortcutHintAnimation.visibilityDuration | ||
| context.timingFunction = CAMediaTimingFunction(name: .easeOut) | ||
| context.allowsImplicitAnimation = true | ||
| animator().alphaValue = value | ||
| }, completionHandler: completion) |
There was a problem hiding this comment.
When macOS Reduce Motion is enabled, this new path still fades every shortcut pill for 120 ms on reveal and dismissal. Check NSWorkspace.shared.accessibilityDisplayShouldReduceMotion and apply the final alpha and hidden state immediately for that configuration.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e7b84b785
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| animateAlpha(to: 0) { [weak self] in | ||
| guard let self, | ||
| self.visibilityGeneration == generation, | ||
| !self.isRevealed else { return } | ||
| self.isHidden = true |
There was a problem hiding this comment.
When command-hold hints are dismissed while the pointer is over the trailing control, the row immediately reveals the plus/close button, but this branch keeps the hint pill isHidden == false until the fade-out completion. Because the pill is added after those controls in the AppKit row subview order and its trailing frame overlaps their frame, AppKit hit-testing will land on the fading pill/material subview instead of the newly revealed button for the animation duration, swallowing an immediate click. Override the pill to return nil from hitTest (or otherwise disable hit testing as soon as it starts hiding) while keeping the visual fade.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/SidebarAppKitRowCellTests.swift`:
- Around line 259-269: Update shortcutHintPillKeepsVisibleDuringFadeOut to
replace the fixed 180 ms sleep with a bounded polling wait for pill.isHidden to
become true. Keep the immediate visibility assertion, and ensure the polling has
a timeout so the test remains deterministic without relying on the animation
duration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f409f967-b8e5-4300-a85e-e7fcc73303a4
📒 Files selected for processing (3)
Sources/ShortcutHintPill.swiftSources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swiftcmuxTests/SidebarAppKitRowCellTests.swift
3e7b84b to
4563f8c
Compare
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cmuxTests/SidebarAppKitRowCellTests.swift (1)
259-273: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winFixed-sleep flakiness from the prior review is resolved, but this assertion is still coupled to the host's reduced-motion setting.
#expect(!pill.isHidden)right afterconfigure(text: nil, ...)assumessetRevealedtakes the animated path. IfNSWorkspace.shared.accessibilityDisplayShouldReduceMotionistrueon the runner,configurehides synchronously and this assertion fails immediately. See the consolidated root-cause note onSidebarGroupHeaderRowView.swift(setRevealed).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/SidebarAppKitRowCellTests.swift` around lines 259 - 273, The test shortcutHintPillKeepsVisibleDuringFadeOut must not assume animation is enabled. Account for the host’s reduced-motion setting before asserting immediate visibility, or conditionally skip the animated fade-out assertions when reduced motion is enabled, while preserving the existing eventual-hidden verification for the animated path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/SidebarAppKitRowCellTests.swift`:
- Around line 275-285: The test
shortcutHintPillUsesExplicitOpacityAnimationInsideDisabledTransaction
incorrectly requires an animation even when reduced motion disables animations.
Update the assertion to accommodate the immediate-visibility path used by
configure, while still validating the expected final opacity/state and
preserving animation verification when animations are enabled.
In `@Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swift`:
- Around line 671-695: The reduced-motion check in setRevealed is not
injectable, making visibility tests depend on the host environment. In
Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swift#L671-L695, add
a reduceMotionProvider closure defaulting to
NSWorkspace.shared.accessibilityDisplayShouldReduceMotion and branch on it; in
cmuxTests/SidebarAppKitRowCellTests.swift#L259-L273 and `#L275-L285`, construct
the pill with the provider forced to false, and add a companion true-provider
test verifying synchronous hiding and no animation key.
---
Duplicate comments:
In `@cmuxTests/SidebarAppKitRowCellTests.swift`:
- Around line 259-273: The test shortcutHintPillKeepsVisibleDuringFadeOut must
not assume animation is enabled. Account for the host’s reduced-motion setting
before asserting immediate visibility, or conditionally skip the animated
fade-out assertions when reduced motion is enabled, while preserving the
existing eventual-hidden verification for the animated path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76226f10-3a08-4d14-995c-45ae0d1a20c6
📒 Files selected for processing (4)
Sources/ShortcutHintPill.swiftSources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swiftcmuxTests/SidebarAppKitRowCellTests.swift
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35daa59a1e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/SidebarAppKitRowCellTests.swift`:
- Around line 319-329: The test shortcutHintPillClipsMaterialToItsCapsule should
also verify the pill’s outer layer: assert that its masksToBounds remains false
and its shadowPath is non-nil, while preserving the existing material clipping
and capsule corner-radius assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 85fb9b6b-573e-45a9-b811-d89d5d4dc2b8
📒 Files selected for processing (2)
Sources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swiftcmuxTests/SidebarAppKitRowCellTests.swift
| @Test | ||
| func shortcutHintPillClipsMaterialToItsCapsule() throws { | ||
| let pill = SidebarShortcutHintPillView() | ||
| pill.frame = NSRect(x: 0, y: 0, width: 36, height: 18) | ||
| pill.configure(text: "⌘1", fontSize: 10, emphasis: 1) | ||
| pill.layoutSubtreeIfNeeded() | ||
|
|
||
| let material = try #require(Self.descendants(of: pill).compactMap { $0 as? NSVisualEffectView }.first) | ||
| #expect(material.layer?.masksToBounds == true) | ||
| #expect(material.layer?.cornerRadius == pill.bounds.height / 2) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Also verify that the outer shadow remains unclipped.
This test only checks the inner material view. Add assertions that the pill’s outer layer remains unclipped and retains its shadowPath, so a regression in shadow preservation cannot pass unnoticed.
Suggested assertions
`#expect`(material.layer?.masksToBounds == true)
`#expect`(material.layer?.cornerRadius == pill.bounds.height / 2)
+ `#expect`(pill.layer?.masksToBounds == false)
+ `#expect`(pill.layer?.shadowPath != nil)📝 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.
| @Test | |
| func shortcutHintPillClipsMaterialToItsCapsule() throws { | |
| let pill = SidebarShortcutHintPillView() | |
| pill.frame = NSRect(x: 0, y: 0, width: 36, height: 18) | |
| pill.configure(text: "⌘1", fontSize: 10, emphasis: 1) | |
| pill.layoutSubtreeIfNeeded() | |
| let material = try #require(Self.descendants(of: pill).compactMap { $0 as? NSVisualEffectView }.first) | |
| #expect(material.layer?.masksToBounds == true) | |
| #expect(material.layer?.cornerRadius == pill.bounds.height / 2) | |
| } | |
| `@Test` | |
| func shortcutHintPillClipsMaterialToItsCapsule() throws { | |
| let pill = SidebarShortcutHintPillView() | |
| pill.frame = NSRect(x: 0, y: 0, width: 36, height: 18) | |
| pill.configure(text: "⌘1", fontSize: 10, emphasis: 1) | |
| pill.layoutSubtreeIfNeeded() | |
| let material = try `#require`(Self.descendants(of: pill).compactMap { $0 as? NSVisualEffectView }.first) | |
| `#expect`(material.layer?.masksToBounds == true) | |
| `#expect`(material.layer?.cornerRadius == pill.bounds.height / 2) | |
| `#expect`(pill.layer?.masksToBounds == false) | |
| `#expect`(pill.layer?.shadowPath != nil) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmuxTests/SidebarAppKitRowCellTests.swift` around lines 319 - 329, The test
shortcutHintPillClipsMaterialToItsCapsule should also verify the pill’s outer
layer: assert that its masksToBounds remains false and its shadowPath is
non-nil, while preserving the existing material clipping and capsule
corner-radius assertions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swift`:
- Around line 601-612: Remove the injectable reduceMotionProvider closure and
initializer parameter from the sidebar group header view, keeping the
accessibility motion setting read privately from NSWorkspace in production code.
Extract the visibility-policy decision into a pure testable helper or equivalent
so tests no longer need to override host state, while preserving the existing
reduced-motion behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb1001f5-4127-4523-904c-c6e667ceaee3
📒 Files selected for processing (3)
Sources/Sidebar/AppKitList/Cells/SidebarGroupHeaderRowView.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowCellView.swiftcmuxTests/SidebarAppKitRowCellTests.swift
1 participant
Summary
Testing
Task
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes the AppKit sidebar command-hold shortcut hints with smooth reveal/dismiss, correct capsule clipping with preserved shadow, and compact sizing. Resets hints on reuse of workspace and group header cells (including same-workspace reuse), with tests for fades, clipping, padding, reduced motion, and reuse.
visibilityDuration; pill stays visible during fade-out, then hides; deterministic reduced-motion policy; works inside disabled Core Animation actions.shadowPath; innerNSVisualEffectViewclips material and border.prepareForReuse/resetForReuse()andrepresentedIdentityto prevent stale hints.Written for commit 8b3b87b. Summary will update on new commits.
Summary by CodeRabbit