Repository navigation
Add tab context menu open availability hook - #158
Conversation
📝 WalkthroughWalkthroughThis PR adds an optional fork-conversation availability provider on ChangesFork Conversation Availability Override
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 introduces a lazy, open-time hook (
Confidence Score: 4/5The change is safe to merge; the new hook is additive, the override logic in makeMenu is straightforward, and existing behaviour is preserved when the provider returns nil. The core plumbing is correct and well-scoped. The only gaps are a sparse doc comment on the new public API and the absence of unit tests exercising the true/false override paths — both are quality concerns rather than functional defects. Tests/BonsplitTests/BonsplitTests.swift would benefit from two new test cases covering the non-nil provider return values. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Host
participant BonsplitController
participant TabBarView
participant TabItemView
participant TabContextMenuBuilder
Note over Host,TabBarView: SwiftUI render (frequent)
Host->>BonsplitController: tabContextForkConversationAvailabilityProvider(tabId, paneId)
BonsplitController-->>TabBarView: Bool (canForkConversation)
TabBarView->>TabItemView: "TabContextMenuState{canForkConversation}"
Note over Host,TabContextMenuBuilder: NSMenu opens (lazy, infrequent)
TabItemView->>TabContextMenuBuilder: makeMenu(snapshot)
TabContextMenuBuilder->>Host: forkConversationOpenAvailabilityProvider()
Host-->>TabContextMenuBuilder: "Bool? (nil = preserve, true/false = override)"
TabContextMenuBuilder-->>TabItemView: NSMenu with updated canForkConversation
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Host
participant BonsplitController
participant TabBarView
participant TabItemView
participant TabContextMenuBuilder
Note over Host,TabBarView: SwiftUI render (frequent)
Host->>BonsplitController: tabContextForkConversationAvailabilityProvider(tabId, paneId)
BonsplitController-->>TabBarView: Bool (canForkConversation)
TabBarView->>TabItemView: "TabContextMenuState{canForkConversation}"
Note over Host,TabContextMenuBuilder: NSMenu opens (lazy, infrequent)
TabItemView->>TabContextMenuBuilder: makeMenu(snapshot)
TabContextMenuBuilder->>Host: forkConversationOpenAvailabilityProvider()
Host-->>TabContextMenuBuilder: "Bool? (nil = preserve, true/false = override)"
TabContextMenuBuilder-->>TabItemView: NSMenu with updated canForkConversation
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// Host-provided check evaluated only when the tab context menu is opening. | ||
| @ObservationIgnored public var tabContextForkConversationOpenAvailabilityProvider: ((TabID, PaneID) -> Bool)? |
There was a problem hiding this comment.
tabContextForkConversationOpenAvailabilityProvider doesn't explain what nil, true, or false return values mean, or how this interacts with the existing tabContextForkConversationAvailabilityProvider. The adjacent provider has a clear contract ("Return true to enable the item, false (or omit the provider) to hide it"); this one should be equally explicit so callers understand that nil is a no-op that defers to the build-time check.
| /// Host-provided check evaluated only when the tab context menu is opening. | |
| @ObservationIgnored public var tabContextForkConversationOpenAvailabilityProvider: ((TabID, PaneID) -> Bool)? | |
| /// Host-provided check evaluated only when the tab context menu is opening (i.e. on | |
| /// `NSMenu` open, not during SwiftUI rendering). Return `true` to enable the "Fork | |
| /// Conversation" item, `false` to hide it, or `nil` to leave the value produced by | |
| /// `tabContextForkConversationAvailabilityProvider` unchanged. | |
| @ObservationIgnored public var tabContextForkConversationOpenAvailabilityProvider: ((TabID, PaneID) -> Bool)? |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Tests/BonsplitTests/BonsplitTests.swift (1)
1676-1678: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the actual override behavior.
All four updated snapshots use
forkConversationOpenAvailabilityProvider: { nil }, which per the builder overridesstate.canForkConversationonly "if let canForkConversation = snapshot.forkConversationOpenAvailabilityProvider()" means none of these tests exercise the non-nil override path — the new feature's core behavior is untested.Consider adding a test where the provider returns a non-nil value that flips
canForkConversation(e.g., state hascanForkConversation: falsebut provider returnstrue, then assert the "Fork Conversation" item appears), to validate the override actually takes effect.Also applies to: 1724-1726, 1759-1761, 1796-1798
🤖 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 `@Tests/BonsplitTests/BonsplitTests.swift` around lines 1676 - 1678, The updated snapshot tests only cover the nil provider path, so the actual fork availability override behavior in the snapshot builder is still untested. Add at least one test around the relevant snapshot setup that uses forkConversationOpenAvailabilityProvider with a non-nil value to override state.canForkConversation, then assert the resulting menu includes the "Fork Conversation" item when the override flips the value. Use the existing snapshot builder and the updated snapshot assertions to verify the non-nil override path in addition to the current nil cases.Sources/Bonsplit/Public/BonsplitController.swift (1)
80-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify precedence and threading contract in the doc comment.
The new provider's doc comment doesn't state (a) that it takes precedence over
tabContextForkConversationAvailabilityProviderwhen set, or (b) that it runs synchronously on the main thread while theNSMenuis being constructed (i.e., any slow/probing work here will visibly delay the context menu opening). Given the whole point of this hook is to let hosts do "expensive or probing checks," a short note warning that the closure should complete quickly (no blocking I/O without a timeout) would help downstream implementers avoid UI hangs.📝 Suggested doc tweak
- /// Host-provided check evaluated only when the tab context menu is opening. + /// Host-provided check evaluated only when the tab context menu is opening. + /// When non-nil, this overrides `tabContextForkConversationAvailabilityProvider` + /// for the item's visibility. Called synchronously on the main thread while the + /// `NSMenu` is being built, so implementations should avoid long-running or + /// blocking work. `@ObservationIgnored` public var tabContextForkConversationOpenAvailabilityProvider: ((TabID, PaneID) -> Bool)?🤖 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 `@Sources/Bonsplit/Public/BonsplitController.swift` around lines 80 - 87, Update the doc comment for tabContextForkConversationOpenAvailabilityProvider in BonsplitController to state that it takes precedence over tabContextForkConversationAvailabilityProvider when both are present. Also document that this closure is invoked synchronously on the main thread during NSMenu construction, so it must return quickly and avoid blocking work or unbounded I/O to prevent menu-opening UI hangs.
🤖 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.
Nitpick comments:
In `@Sources/Bonsplit/Public/BonsplitController.swift`:
- Around line 80-87: Update the doc comment for
tabContextForkConversationOpenAvailabilityProvider in BonsplitController to
state that it takes precedence over
tabContextForkConversationAvailabilityProvider when both are present. Also
document that this closure is invoked synchronously on the main thread during
NSMenu construction, so it must return quickly and avoid blocking work or
unbounded I/O to prevent menu-opening UI hangs.
In `@Tests/BonsplitTests/BonsplitTests.swift`:
- Around line 1676-1678: The updated snapshot tests only cover the nil provider
path, so the actual fork availability override behavior in the snapshot builder
is still untested. Add at least one test around the relevant snapshot setup that
uses forkConversationOpenAvailabilityProvider with a non-nil value to override
state.canForkConversation, then assert the resulting menu includes the "Fork
Conversation" item when the override flips the value. Use the existing snapshot
builder and the updated snapshot assertions to verify the non-nil override path
in addition to the current nil cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c22a49a3-0031-4b08-8d3c-0635cde09611
📒 Files selected for processing (4)
Sources/Bonsplit/Internal/Views/TabBarView.swiftSources/Bonsplit/Internal/Views/TabItemView.swiftSources/Bonsplit/Public/BonsplitController.swiftTests/BonsplitTests/BonsplitTests.swift
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Tests/BonsplitTests/BonsplitTests.swift (1)
1818-1856: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage for the false-override case; consider adding the true-escalation case too.
This test nicely locks in "visible but disabled" behavior when the provider returns
falsewhilestate.canForkConversationistrue. Consider adding a companion test withcanForkConversation: falseandforkConversationOpenAvailabilityProvider: { true }to pin down whether the menu should become visible/enabled in that case — this directly relates to the open question about the escalation path inTabContextMenuBuilder.makeMenu.🤖 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 `@Tests/BonsplitTests/BonsplitTests.swift` around lines 1818 - 1856, Add a companion test for the escalation path in TabContextMenuBuilder.makeMenu: cover the case where TabContextMenuState.canForkConversation is false but forkConversationOpenAvailabilityProvider returns true, so the menu’s Fork Conversation items behavior is pinned down when availability becomes true. Reuse the existing TabContextMenuSnapshot and TabContextMenuActionTarget setup, then assert whether the main Fork Conversation item, the Fork Conversation To submenu, and its destination items are visible and/or enabled as expected for that true-override case.Sources/Bonsplit/Internal/Views/TabItemView.swift (1)
1144-1146: 🚀 Performance & Scalability | 🔵 TrivialBlocking/expensive provider runs synchronously during menu build.
Per the PR objective this provider exists to run "expensive or probing checks" at open-time instead of during SwiftUI rendering. It's now invoked synchronously inside
makeMenu, which is presumably called from the main-thread menu-opening path. If a host implementation performs real I/O/probing here, this still blocks the main thread — just at a different point (menu open) rather than during render. Worth calling out in documentation/API contract that the provider must be fast/non-blocking, or consider a timeout/async-safe contract.🤖 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 `@Sources/Bonsplit/Internal/Views/TabItemView.swift` around lines 1144 - 1146, The open-time provider call in makeMenu is still executed synchronously on the menu-opening path, so expensive I/O or probing can block the main thread. Update the contract around forkConversationOpenAvailabilityProvider in TabItemView to require a fast, non-blocking check at open time, and if needed adjust the API/design so slow work is performed asynchronously or cached before menu construction. Preserve the existing state.canForkConversation and forkConversationEnabled logic, but ensure the provider is safe to invoke from makeMenu without UI stalls.
🤖 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.
Nitpick comments:
In `@Sources/Bonsplit/Internal/Views/TabItemView.swift`:
- Around line 1144-1146: The open-time provider call in makeMenu is still
executed synchronously on the menu-opening path, so expensive I/O or probing can
block the main thread. Update the contract around
forkConversationOpenAvailabilityProvider in TabItemView to require a fast,
non-blocking check at open time, and if needed adjust the API/design so slow
work is performed asynchronously or cached before menu construction. Preserve
the existing state.canForkConversation and forkConversationEnabled logic, but
ensure the provider is safe to invoke from makeMenu without UI stalls.
In `@Tests/BonsplitTests/BonsplitTests.swift`:
- Around line 1818-1856: Add a companion test for the escalation path in
TabContextMenuBuilder.makeMenu: cover the case where
TabContextMenuState.canForkConversation is false but
forkConversationOpenAvailabilityProvider returns true, so the menu’s Fork
Conversation items behavior is pinned down when availability becomes true. Reuse
the existing TabContextMenuSnapshot and TabContextMenuActionTarget setup, then
assert whether the main Fork Conversation item, the Fork Conversation To
submenu, and its destination items are visible and/or enabled as expected for
that true-override case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd329596-0d97-42e2-94a8-789204058ff5
📒 Files selected for processing (2)
Sources/Bonsplit/Internal/Views/TabItemView.swiftTests/BonsplitTests/BonsplitTests.swift
1 participant
Adds an open-time tab context menu availability hook so hosts can run expensive/probing checks only when an NSMenu is opening, not while SwiftUI renders tab rows.\n\nNeeded by manaflow-ai/cmux#7352 for Fork Conversation availability.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds an open-time availability hook for the tab context menu so hosts can run checks only when the menu opens. Keeps “Fork Conversation” visible (but disabled) while availability refreshes, and supports
manaflow-ai/cmux#7352.tabContextForkConversationOpenAvailabilityProvideronBonsplitController; runs when the menu opens.canForkConversationmutable; tests updated.Written for commit 222f9a7. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes