Repository navigation
fix(mobile): make swipe back smoother - #1793
Conversation
 |
| Layer / File(s) | Summary |
|---|---|
Pager scroll and pop behavior apps/desktop/src/mobile/use-back-pager.ts, apps/desktop/src/mobile/use-back-swipe.ts, apps/desktop/src/mobile/use-back-swipe.test.tsx |
Adds useBackPager to position pages and handle settled user scrolls. A qualifying scroll reveals the lower page and calls the pop callback. Removes the previous swipe hook and its listener tests. |
Pager integration and navigation apps/desktop/src/mobile/mobile-stack.tsx, apps/desktop/src/styles/index.css, docs/porting/reflect-mobile/app-shell-and-navigation.md |
Renders stack layers as scroll-snap pages and connects gesture pops to navigation. Adds pager touch-action rules and updates the porting notes. |
Pager interaction tests apps/desktop/src/mobile/mobile-screen.test.tsx |
Tests pager position, touch and mouse gestures, edge-strip taps, and navigation between stacked and cold-note routes. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant User
participant Pager
participant useBackPager
participant MobileStack
User->>Pager: Scroll toward the lower page
Pager->>useBackPager: Report scroll settling
useBackPager->>Pager: Reveal lower page at offset at or below 1 px
useBackPager->>MobileStack: Call onPop
MobileStack->>MobileStack: Call back or navigate to today
Merge Risk: 
8b21d
-
A canceled swipe can unexpectedly navigate back, and a resize during a drag can make the page jump. Both are bounded gesture issues, but they should be fixed or explicitly accepted before merging.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly describes the main change: improving mobile swipe-back behavior. It is concise and directly related to the native scroll pager implementation.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 6 functions across 3 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Reviewer's GuideMigrates mobile edge back navigation from React-driven pointer gestures to a compositor-friendly native scroll-snap pager, adding precise gesture routing for nested scrolling and taps, settle-time accessibility handling, mouse support, documentation, and comprehensive updated tests. Sequence diagram for a mobile edge swipe popsequenceDiagram
actor User
participant Edge as EdgeStrip
participant Pager as NativePager
participant Settle as watchScrollSettle
participant Below as RevealedPage
participant Router
User->>Edge: Start and drag horizontally
Edge->>Pager: Allow native horizontal scroll
Pager->>Pager: Snap to lower page
Pager->>Settle: Report settled scroll
Settle->>Below: Remove inert and aria-hidden
Settle->>Router: onPop()
Router-->>Pager: Re-render stack
Pager->>Pager: Pin scrollLeft to last page
Flow diagram for mobile gesture routingflowchart TD
Start[Gesture or tap in mobile screen]
Edge{Starts in left 20px edge strip?}
Horizontal{Inside data-scroll-x scroller?}
Vertical[Allow vertical pan]
Pager[Route horizontal drag to native pager]
Tap[Forward tap through strip]
Target[Focus field, click control, or place editor caret]
Start --> Edge
Edge -->|no| Vertical
Edge -->|yes| Horizontal
Horizontal -->|yes| Vertical
Horizontal -->|no horizontal drag| Pager
Horizontal -->|tap| Tap
Tap --> Target
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
8731ad0 to
0fb1dc9
Compare
0fb1dc9 to
c69bd03
Compare
c69bd03 to
9764ce2
Compare
ocavuebot
changed the title
9764ce2 to
cb9e434
Compare
# Conflicts: # apps/desktop/src/lib/scroll-settle.ts
# Conflicts: # pnpm-lock.yaml
# Conflicts: # pnpm-lock.yaml
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
# Conflicts: # apps/desktop/src/mobile/mobile-screen.test.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="apps/desktop/src/mobile/mobile-screen.test.tsx" line_range="979-983" />
<code_context>
- const stack = view.container.querySelector('.mobile-stack')!
- const card = stackLayers(view).at(-1)!
+ const scroller = pager(view)
+ await mouseDragFromStrip(view, 60)
+ await waitFor(() => expect(scroller.scrollLeft).toBe(scroller.clientWidth))
+ await new Promise((resolve) => setTimeout(resolve, 200))
+ expect(view.getByRole('heading').element().textContent).toBe('Edit note')
+ expect(stackLayers(view)).toHaveLength(2)
+ })
</code_context>
<issue_to_address>
**Ignored mouse drags pass snap-back test**
When the mouse-drag path ignores a short drag but does not forward its click, `it('snaps a short mouse drag back without passing its click to the back button')` only checks that the note remains visible and the pager ends at its starting offset. A handler that ignores the drag but suppresses its click passes without ever scrolling or snapping back.
Assert that the pager moves during the drag before verifying that it returns to the last page.
</issue_to_address>
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
ocavuebot
left a comment
ocavuebot
left a comment
There was a problem hiding this comment.
Independent review by Claude (model ID: claude-opus-5-5) at 8b21d18e282d43fd88d473bb79532b22bc139437.
Summary:
- Simplification (inline,
use-back-pager.ts:41):pageCountis not needed. "Last page" is the end of the scroll range. - Possible logic bug (inline,
use-back-pager.ts:116): the resize re-pin ignores a finger that is down. The drawer in #1794 guards this case. - Question:
watchScrollSettlecounts a wheel as a user scroll, andtouch-actiondoes not apply to wheel input. So on an iPad with a trackpad (or in the browser preview), a two-finger horizontal scroll anywhere on a stacked screen moves the pager, and a long one pops. The PR text says only the edge strip starts a back swipe. If this is intended (iPadOS has a trackpad back swipe), please write it in the PR text. If not, ignore a wheel for the pop. - Edge case (no action needed if accepted):
forwardStripTaponly focuses aninput. A checkbox or radioinputunder the 20px strip gets focus but does not toggle. Long press (text selection, link preview) is also lost under the strip. Both are probably fine trade-offs, but the PR text does not list them.
ocavue
left a comment
ocavue
left a comment
There was a problem hiding this comment.
AI model ID: gpt-6 (Codex).
Reviewed commit: 8b21d18e282d43fd88d473bb79532b22bc139437.
Independent source review of the pager, stack, CSS, and settlement helper. No local tests or device checks were run.
[P2, known behavior trade-off] The edge strip removes vertical scrolling from the leftmost 20px. At apps/desktop/src/mobile/mobile-stack.tsx:387, the full-height strip receives the gesture and allows only horizontal panning. A vertical drag starting there cannot scroll a long note or Settings. I read the existing response: changing it to pan-x pan-y alone cannot fix this, because the content scroller is a sibling. This is a real UX regression that the current architecture accepts, not a missing defensive guard. Please make that limitation explicit in the PR's user-facing behavior and manual checks. If preserving edge vertical scrolling is required, the hit region needs to participate in the content scroller's ancestor chain, or needs deliberate gesture forwarding.
Simplification recommendation: keep the synthetic tap forwarding narrowly scoped. It already reconstructs clicks, field focus, and editor caret placement, but cannot preserve the full native pointer interaction. Avoid extending it into a general input dispatcher with more target-specific fallbacks. Resolve the hit-region layout first if more edge interactions must work.
I found no additional high-confidence navigation-state bug in the current diff. The once-only pop guard and programmatic-scroll rejection serve concrete purposes; I would retain them.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Before/after recording with Chrome CPU throttling at 20x ( Gestures (CDP touch events, 390x844 viewport), two rounds:
a3_cpu20_base_vs_pr.mp4 |
# Conflicts: # apps/desktop/src/mobile/mobile-stack.tsx
ocavuebot
changed the title
Makes the native scroll-snap pager the mobile edge back swipe and deletes the pointer-driven one. There is no flag and no second path.
What changed
MobileStackrenders its layers as pages of ascroll-snap-type: x mandatoryscroller. Every page isposition: sticky; left: 0and the current page is last, so a swipe moves only the current card while the page below stays put.useBackPagerpinsscrollLeftto the last page before paint whenever the page list changes, and on resize.touch-action: pan-ythrough a zero-specificity rule in@layer base. A 20px full-height edge strip (data-pager-edge,touch-action: pan-x) is the only place where a horizontal drag reaches the pager. Horizontal scrollers carrydata-scroll-x(meowdown sets it on code blocks and math and Mermaid previews, feat: mark horizontal scrollers withdata-scroll-xprosekit/meowdown#752, released in 0.82.3; swipe rows set it too), and they and everything inside them opt back intoautowithoverscroll-behavior-x: contain.elementsFromPointandcaretRangeFromPointwith the strip's pointer events off): fields focus, controls click, and the editor gets a caret.watchScrollSettlecommits the pop when a user scroll settles atscrollLeft <= 1. The revealed page dropsinertandaria-hiddenat once, before React re-renders it as current. A scroll that no finger, wheel, or mouse drag started pins the pager back.dragHandle). A plain click on the strip still forwards; the click that ends a drag does not.linkOpacityToScroll. Button back, wiki-link push, and other programmatic push and pop keep the existing CSS keyframes.position: fixedelement inside a stacked screen (it would stay put while the card moves), anddata-scroll-xfor any horizontal scroller.What was deleted
mobile/use-back-swipe.tsand its test (the pointer hook with its non-passivetouchmoveblocker, per-movesetState, and settle transition).mobile-stack.tsx: the dragging and settling styles, the drag scrim, and thetransitionendlistener.lib/pointer-drag.tsstays (the image dismiss drag uses it).What users feel
Before and after
Chromium 156.0.8078.4 (Playwright, headless,
isMobile, 390x844), 4x CPU throttle, dev build, n = 7 per row after 1 warmup. Baseb83dca13(old hook; the old path is byte-identical at the PR's base26f9058a), PR9764ce22. Real CDPInput.dispatchTouchEventgestures from (8, 420): pop = slow drag to x 330 and release; cancel = drag to x 100, hold 300ms, release; fling = 5 moves to x 150, 8ms apart. Light note:notes/sarah-chen.md(seed). Heavy note: a generated 3000-line note. "During" is touchStart to touchEnd; "after" is the 1.5s after touchEnd. LoAF max is the max over reps; other columns are p50. Script and task time come from CDPPerformance.getMetricsdeltas over the drag, divided by the deliveredtouchmovecount.p95FrameMsandhitchTimeRatiouse the perf HUD formula on rAF times from touchStart to touchEnd + 500ms. Correct means the expected outcome happened exactly once (oneback()for a pop, none for a cancel).How to test by hand
On an iPhone (or iPad) build, open a note from Daily:
?platform=ios): press on the left 20px and drag right: it goes back. Click the strip over the Back button: it goes back once. Drag on note text: it selects text and never swipes.Written by Claude Opus 5.5.
Need help on this PR? Tag
@codesmith-botwith what you need.Summary by Sourcery
Adopt native scroll-snap paging for mobile edge back navigation to improve gesture responsiveness and preserve nested scrolling and underlying interactions.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores:
Summary by CodeRabbit