Fix iOS terminal picker menu flicker and blocked scrolling - #7959
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a reusable SwiftUI terminal picker menu with derived state and action closures, integrates it into ChangesTerminal picker menu
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WorkspaceDetailView
participant TerminalPickerMenu
participant MenuActionHandler
WorkspaceDetailView->>TerminalPickerMenu: provide picker value and action closures
TerminalPickerMenu->>MenuActionHandler: invoke selected terminal or menu action
MenuActionHandler->>WorkspaceDetailView: execute configured operation
Possibly related PRs
🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 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 |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
Greptile SummaryThis PR isolates the iOS terminal picker from frequent preview updates. The main changes are:
Confidence Score: 4/5The equatable menu can retain actions from an old workspace when the compared snapshot stays equal.
TerminalPickerMenu.swift and WorkspaceDetailView.swift Important Files Changed
Reviews (1): Last reviewed commit: "fix(ios): isolate terminal picker from p..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| nonisolated static func == (lhs: Self, rhs: Self) -> Bool { | ||
| lhs.value == rhs.value | ||
| } |
There was a problem hiding this comment.
The equality check ignores actions, although the rendered menu invokes those closures. If SwiftUI preserves this view identity while the workspace changes to one with an equal menu snapshot, .equatable() can retain closures from the previous WorkspaceDetailView; selecting a terminal can then close the browser or perform another action for the previous workspace. Include the action owner, such as the workspace ID, in the compared value.
Rule Used: Flag SwiftUI changes that can cause stale state, b... (source)
There was a problem hiding this comment.
Not reachable: terminal ids are globally unique surface ids, so a workspace change always changes the row ids and therefore the menu value; equal snapshots across workspaces cannot occur. The existing .onChange(of: terminalPickerLiveMembership) also resyncs the snapshot on any membership change, installing fresh closures. Within the single-render window the closures act via the live store reference keyed by unique terminal id, so they cannot target the wrong workspace.
There was a problem hiding this comment.
That's a sound argument. Globally unique terminal IDs ensure workspace switches always produce unequal TerminalPickerMenuValues, .equatable() never suppresses those updates, and the .onChange resync installs fresh closures before any interaction is possible. The stale-closure path I described isn't reachable given those invariants.
|
Re Greptile's stale-closure concern (actions excluded from Equatable): a workspace transition cannot keep the old closures live. Terminal ids are globally unique surface ids, so any workspace swap changes |
This branch was successfully deployed
1 participant
The terminal picker toolbar menu flickered while open: every live preview update from the Mac (~2Hz title/viewportFit churn in
MobileTerminalPreview) re-renderedWorkspaceDetailView, re-evaluated theMenucontent closure, and made iOS rebuild the presented UIMenu. Each rebuild cross-faded the menu text (the visible opacity pulse, measured at 3.5 luminance peak-to-peak with a dominant ~2Hz component) and cancelled in-flight scroll gestures, so an overflowing menu could not be scrolled.Fix: the picker now lives in
TerminalPickerMenu, anEquatablechild view applied with.equatable(). Its only inputs areTerminalPickerMenuValue(row snapshot, selection resolved from that same snapshot, capability/browser/chat flags) and aTerminalPickerMenuActionsclosure bundle excluded from equality. Preview/title/viewport churn produces an equal value, so SwiftUI skips the child body and the presented UIMenu is never rebuilt; membership changes, selection changes, browser/chat mode, and the New Workspace capability still change the value and update the open menu once. The existingsyncTerminalPickerRowsevent paths (tap, onAppear, membership onChange, selection onChange) are unchanged. DEBUG-only Logger/os_signpost diagnostics count content-builder evaluations and snapshot writes.Commits follow the red/green regression policy: the first commit adds
TerminalPickerMenuValueTestsagainst the not-yet-existing value seam (CI red), the second adds the fix (green). Tests cover title-only vs membership changes, selection resolution from snapshot rows, and the empty-snapshot first-open fallback.Simulator evidence (before/after video with per-frame luminance analysis) is being captured and will be posted on this PR.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Low Risk
Localized SwiftUI toolbar refactor with tests and DEBUG-only instrumentation; menu actions and sync behavior are preserved.
Overview
Fixes iOS terminal picker flicker and broken scrolling when live
MobileTerminalPreviewupdates (~2Hz title/viewport churn) kept rebuilding the nativeUIMenufromWorkspaceDetailView.The inline toolbar
Menuis replaced byTerminalPickerMenu, anEquatablechild with.equatable(). It only comparesTerminalPickerMenuValue(snapshot rows, selection resolved from those rows, browser/chat/capability flags);TerminalPickerMenuActionsclosures are excluded from equality. Title/viewport-only live churn leaves the value unchanged so SwiftUI skips the child body and the open menu is not rebuilt; membership, selection, mode, and capability changes still update once. ExistingsyncTerminalPickerRowstriggers (tap, appear, membership/selection) are unchanged.DEBUG adds Logger/os_signpost for menu content evaluation and snapshot writes.
TerminalPickerMenuValueTestslock snapshot-vs-live equality and selection resolution.Reviewed by Cursor Bugbot for commit 74daa96. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes the iOS terminal picker flicker and cancelled scrolling by moving the menu into an Equatable child with a stable snapshot. The menu no longer rebuilds during live preview/title/viewport updates, so scrolling stays smooth.
TerminalPickerMenuand applied.equatable()so equalTerminalPickerMenuValueskips recompute.TerminalPickerMenuValue(snapshot-based rows; selection resolved from the snapshot) andTerminalPickerMenuActions(excluded from equality).TerminalPickerMenuValueTeststo cover title-only vs membership changes, selection resolution, and empty-snapshot fallback.Written for commit 74daa96. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests