Reduce iOS workspace row swipe contention#6064
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughThis PR removes per-row minute-driven timestamp updates and simplifies the timestamp API by eliminating the injected ChangesWorkspace row timestamp and interaction simplification
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 reduces iOS swipe-gesture contention by removing the per-row
Confidence Score: 4/5Safe to merge with one concern worth resolving: the compact-stack push case could double-push the workspace detail view if gesture ordering between the SwiftUI TapGesture and UIKit's NavigationLink activation is not deterministic. The WorkspaceRow and timestamp changes are correct and well-scoped. The DeviceTreeView .push→.sidebar switch is a necessary fix. The WorkspaceNavigationRow push case attaches a simultaneousGesture(TapGesture) alongside a NavigationLink(value:). selectWorkspace guards with compactNavigationPath.last != id before setting the path, but that guard only protects against redundancy when the NavigationLink has already mutated the path first — if the SwiftUI gesture fires first (path still []), selectWorkspace sets [id] and the NavigationLink subsequently appends a second id, leaving [id, id] in the stack. Whether this ordering occurs in practice was tested on-device, but the ordering is not guaranteed by the SwiftUI/UIKit contract. WorkspaceNavigationRow.swift — specifically the .push branch of rowTarget where NavigationLink and simultaneousGesture(TapGesture) both mutate compactNavigationPath on the same tap. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant UIKit as UIKit (List)
participant NL as NavigationLink
participant TG as simultaneousGesture(TapGesture)
participant SW as selectWorkspace
participant Path as compactNavigationPath
User->>UIKit: tap row
par NavigationLink activation
UIKit->>NL: didSelectRow
NL->>Path: append(workspace.id) → [id]
and SwiftUI TapGesture
UIKit->>TG: tap ended
TG->>SW: selectWorkspace(id)
SW->>Path: "if last != id → set [id]"
end
Note over Path: Race: if TG fires before NL,<br/>Path becomes [id] then [id,id]
Reviews (9): Last reviewed commit: "Keep device tree workspace rows as butto..." | Re-trigger Greptile |
azooz2003-bit
force-pushed
the
feat-ios-swipe-stutter
branch
from
8e596ec to
a9f0510
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// Passive timestamp reference for the relative activity label. This avoids a | ||
| /// per-row `TimelineView` invalidating list rows while UIKit is tracking a | ||
| /// native swipe gesture. | ||
| var timestampReferenceDate: Date = .now |
There was a problem hiding this comment.
timestampReferenceDate defaults to Date.now at WorkspaceRow init time, but neither WorkspaceNavigationRow nor either of its callers (WorkspaceListView.workspaceRow(), DeviceTreeView) passes a live date. Without any periodic update mechanism, the label freezes at its initial value and only refreshes if some unrelated state change causes the parent to rebuild the row. In a quiet session — where no new workspace activity arrives — a row initialized at T+0 that shows "now" will still show "now" minutes later. The old TimelineView(.everyMinute) was the only guarantee of a minute-by-minute tick. The intended fix is for the list owner to host a single TimelineView(.everyMinute) and thread context.date into every WorkspaceNavigationRow as timestampReferenceDate, but that wiring is not present in this PR.
azooz2003-bit
force-pushed
the
feat-ios-swipe-stutter
branch
from
a9f0510 to
f5b94af
Compare
azooz2003-bit
force-pushed
the
feat-ios-swipe-stutter
branch
from
f5b94af to
bfb372c
Compare
azooz2003-bit
force-pushed
the
feat-ios-swipe-stutter
branch
from
bfb372c to
8d0aca3
Compare
azooz2003-bit
force-pushed
the
feat-ios-swipe-stutter
branch
from
8d0aca3 to
f979651
Compare
| case .push: | ||
| NavigationLink(value: workspace.id) { | ||
| rowLabel | ||
| } | ||
| .simultaneousGesture(TapGesture().onEnded { | ||
| selectWorkspace(workspace.id) | ||
| }) |
There was a problem hiding this comment.
NavigationLink(value: workspace.id) and simultaneousGesture(TapGesture().onEnded { selectWorkspace(workspace.id) }) both fire on the same tap. selectWorkspace (line 182–184 of WorkspaceShellView) guards with compactNavigationPath.last != id before setting the path to [id] — but that guard is only effective if the NavigationLink has already appended id to the path first. If the SwiftUI TapGesture fires before UIKit/NavigationStack processes the link activation (making the path still []), selectWorkspace sets compactNavigationPath = [id] and then the NavigationLink appends id again, yielding [id, id]. The NavigationStack would then push the workspace detail view twice, leaving a stale intermediate destination in the back-stack. Since simultaneousGesture makes the ordering undefined, this race is present even though it may not reproduce consistently on any given device.
1 participant
Summary
Verification
Note: the remaining physical-device-only slow-swipe hitch is not claimed fixed by this PR; follow-up profiling continues separately.