Add animate parameter to splitPane to skip entry animation#25
Add animate parameter to splitPane to skip entry animation#25shouryamaanjain wants to merge 3 commits intomanaflow-ai:mainfrom
Conversation
Allows the host application to distinguish user-initiated tab closes (clicking the X button) from internal/programmatic closes, so it can decide whether closing the last surface should also close the workspace. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When restoring session state, splits should be created without entry animation so that the saved divider positions are applied immediately rather than being overridden by the animation code which forces 0.5.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughAdds an optional Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (1)
Sources/Bonsplit/Public/BonsplitController.swift (1)
54-57: Clarify callback contract foronTabCloseRequest.Consider documenting that this hook is a request signal and can fire even if the subsequent close is vetoed, so consumers don’t treat it as a guaranteed close event.
✍️ Suggested doc update
/// Called when the user explicitly requests to close a tab from the tab strip UI. /// Internal host-driven closes should not use this hook. +/// Note: this is a request notification and may fire even if the close is later vetoed. `@ObservationIgnored` public var onTabCloseRequest: ((_ tabId: TabID, _ paneId: PaneID) -> Void)?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Bonsplit/Public/BonsplitController.swift` around lines 54 - 57, Update the doc comment for the onTabCloseRequest property to state clearly that this is a user-initiated "request" signal and not a guaranteed close event: mention that handlers receive (tabId: TabID, paneId: PaneID), that the close can be vetoed by the host or other logic, and consumers must not assume the tab is closed after this callback fires (they should observe actual close events/state changes instead); modify the comment on the `@ObservationIgnored` public var onTabCloseRequest to include that clarification and any guidance about side-effects or asynchronous veto handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Bonsplit/Public/BonsplitController.swift`:
- Around line 423-425: Add documentation for the new animate parameter in the
public API comment for the BonsplitController method that takes insertFirst:
Bool, animate: Bool = true -> PaneID?; update the method's docblock to include a
brief `@param` (or similar) entry for animate describing that true performs the
restore with animation, false restores without animation, and that the default
is true so callers can opt out of animation when needed. Ensure the doc text
uses the parameter name animate and mentions the method returns a PaneID? so
callers can understand restore/no-animation behavior without reading
implementation.
---
Nitpick comments:
In `@Sources/Bonsplit/Public/BonsplitController.swift`:
- Around line 54-57: Update the doc comment for the onTabCloseRequest property
to state clearly that this is a user-initiated "request" signal and not a
guaranteed close event: mention that handlers receive (tabId: TabID, paneId:
PaneID), that the close can be vetoed by the host or other logic, and consumers
must not assume the tab is closed after this callback fires (they should observe
actual close events/state changes instead); modify the comment on the
`@ObservationIgnored` public var onTabCloseRequest to include that clarification
and any guidance about side-effects or asynchronous veto handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d8e0431-5b46-4f57-a9cc-a124c3ac231b
📒 Files selected for processing (3)
Sources/Bonsplit/Internal/Controllers/SplitViewController.swiftSources/Bonsplit/Internal/Views/TabBarView.swiftSources/Bonsplit/Public/BonsplitController.swift
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Bonsplit/Public/BonsplitController.swift (1)
518-523: API inconsistency:movingTabvariant lacksanimateparameter.The
splitPane(_:orientation:withTab:insertFirst:animate:)variant exposes animation control via theanimateparameter, but thesplitPane(_:orientation:movingTab:insertFirst:)variant does not, and will always use the defaultanimate: truefrom the internal call.This is likely intentional—the
movingTabvariant is used for drag operations where animation is expected—but the API is asymmetric. If future use cases (e.g., session restore) require non-animated moves, this variant would need the sameanimateparameter for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Bonsplit/Public/BonsplitController.swift` around lines 518 - 523, The movingTab variant splitPane(_:orientation:movingTab:insertFirst:) is missing the animate parameter and always uses the default animate:true; modify the API to accept an animate: Bool (defaulting to true for backward compatibility) on splitPane(_:orientation:movingTab:insertFirst:animate:) and forward that value to internalController.splitPaneWithTab(..., insertFirst: insertFirst, animate: animate); also update any callers of splitPane(_:orientation:movingTab:insertFirst:) to pass the animate flag where appropriate or rely on the default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Bonsplit/Public/BonsplitController.swift`:
- Around line 518-523: The movingTab variant
splitPane(_:orientation:movingTab:insertFirst:) is missing the animate parameter
and always uses the default animate:true; modify the API to accept an animate:
Bool (defaulting to true for backward compatibility) on
splitPane(_:orientation:movingTab:insertFirst:animate:) and forward that value
to internalController.splitPaneWithTab(..., insertFirst: insertFirst, animate:
animate); also update any callers of
splitPane(_:orientation:movingTab:insertFirst:) to pass the animate flag where
appropriate or rely on the default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a068090-1192-4d29-a2cf-119c3b5502e8
📒 Files selected for processing (1)
Sources/Bonsplit/Public/BonsplitController.swift
2 participants
Companion to manaflow-ai/cmux#1433 — fixes manaflow-ai/cmux#1376
Summary
animate: Bool = trueparameter tosplitPane(_:orientation:withTab:insertFirst:)and its internal implementationfalse,SplitStateis created withanimationOrigin: nilso the entry animation is skippedSummary by CodeRabbit
Release Notes