feat(nav): open the mobile nav drawer by swipe on every page - #7133
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of Diff reviewed end-to-end: hook rewrite, shell binding, chat-page claim, doc section, and the three test suites all match the description's claims (composedPath chain, dialog/touch-action/owns-swipe ownership, lock-time suppression, settle-commit for the prop lag). The problem is user-reported, the ownership protocol fails open with the rationale argued and documented in Design-Verdict: PASS One shell-bound gesture with a fail-open, documented page-claim protocol is the right shape; root causes fixed, alternatives argued, no one-way doors. [DESIGN-REVIEWED] 8db6b59 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- website/src/hooks/useDrawerSwipe.ts:762 -- checking False positive or not applicable? A repository writer can comment: |
UX Review (Fable 5) — ✅ PASSUX-level review of UX-Verdict: PASS One invisible accelerator, correctly yielded to scrollers, widgets, dialogs, and chat's own drawers — the hamburger stays the discoverable path, so nothing is lost and nothing lies. [UX-REVIEWED] 8db6b59 |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of All repo facts are verified. Composing the review. First-Principles-Verdict: CONCERNS Everything here traces to a named reporter or a platform rule — except the What this change shipsIntent: let a phone user open the nav drawer by swipe on any page — declared an ADDITION, honestly ("an ergonomics addition, not a repair").
Demo media under WatchThe change reuses the existing Subtractions
[FIRST-PRINCIPLES-REVIEWED] 8db6b59 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsNon-blocking: re-opening the nav drawer mid-close snaps the panel backward before it follows the finger. FINDING — website/src/hooks/useDrawerSwipe.ts:1163 — during the ~200-300ms close settle the panel is visibly partway (say −100) but [OPUS-REVIEWED] 8db6b59 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
eb9335f to
0373a94
Compare
0373a94 to
9de5b86
Compare
Disposition — UX CONCERNS and First Principles CONCERNS on
|
9de5b86 to
0d94648
Compare
0d94648 to
ca8a52a
Compare
Disposition — UX CONCERNS on
|
Disposition — First Principles CONCERNS on
|
ca8a52a to
3aec8eb
Compare
3aec8eb to
7b74984
Compare
7b74984 to
7405b64
Compare
7405b64 to
24e1ad3
Compare
24e1ad3 to
59d2ad9
Compare
The drawer had one way in, the hamburger, while every other mobile panel is
swipeable -- and it already carried everything a gesture needs except the
binding. It cannot simply be a second instance: the chat page's rightward drag
belongs to its sessions drawer, and two instances on one side arm on the same
touch and fight for the same direction.
Bind one instance on the dashboard shell, the common ancestor of <main>, the
panel and the scrim. <main> is the wrong root: the panel and scrim are fixed
SIBLINGS of it, so a gesture rooted there opens the drawer but never sees the
touch that should close it. Widening the root does not widen what arms --
dialogs portal to document.body, and a page declares the sides it owns with
data-owns-swipe on the element it binds. The hook walks from the touch target
up to but not including its own root, so one attribute serves both instances:
the claim is below the shell and IS the page's own root. It fails open, because
a missing attribute should cost a visible conflict rather than silently killing
the gesture everywhere.
Bind the panel live while here. It read mobileNavX.get() into an inline
transform at render time, which is correct only while a tap is the sole mover: a
MotionValue does not re-render React, so a drag wrote the value every frame
while the DOM moved once, and the panel completed only on release. The hook's
own comment documented that premise -- "having no gesture" -- which adding a
gesture invalidated. motion.nav with style={{ x }}, matching the sessions
drawer and the right overlay; the compositor settle still runs through the ref.
The scrim had the same defect as a literal opacity:0 and now derives from the
panel's offset over the drawer's own travel.
59d2ad9 to
8db6b59
Compare
|
Disposition for the two CONCERNS on UX — FIXED, and it was a real defect in the common caseThe finding is right, and its evidence is my own comment: the locked gesture's Took the first suggested remedy: an ENDED suppression is now released as soon as a new touchstart arrives, because a fresh finger means any pending click belongs to that touch rather than to the finished drag. The 350ms timer stays as the backstop for the case where a click never comes and no further touch follows. Preferred over the alternative (only swallow a click whose target sits under the release point) because the ordering it relies on is fixed rather than geometric: a release's synthesized click is dispatched before any subsequent touchstart, so this can only narrow the window, and it fails toward letting a click through instead of eating one the user meant. Hit-testing the release point would need the same fail-open reasoning plus coordinates. Mutation-verified: removing the release makes the new test red. One mutation I am reporting rather than papering over — relaxing the guard from First Principles — ACCEPTED as a real design argument, DEFERRED, and it needs the repo owner's callNot rebutting this. The premise is correct on every point I checked: the hook does know its bindings at bind time, the attribute does duplicate that, the drift failure mode is real rather than theoretical (this PR already fixes one instance of it — the Deferring it here for reasons of blast radius rather than merit:
So: worth doing, not worth folding into this PR at this point. @Rayrayxu owns whether it becomes a follow-up — I have deliberately not filed an issue for it unilaterally. |
|
Follow-up filed for the First Principles finding: #7210 — replace |
Problem
Requested by @Rayrayxu while using the dashboard on a phone, after the mobile swipe gestures shipped in #7073 made the chat panels draggable: the nav drawer was then the one mobile panel still reachable only by tapping the hamburger. The same reporter verified the finished gesture on-device (and found the two defects fixed below — a panel that did not track the finger, and a drawer that could not be swiped shut).
That is the requirement, and it is worth stating plainly rather than resting on symmetry with the other panels: "everything else is swipeable" is an analogy, not a need. The hamburger did work, so this is an ergonomics addition, not a repair — the drawer already carried everything a gesture needs (registered targets, a distance-derived settle,
openMobileNav/closeMobileNavDrawer) except the binding.Reaching it by swipe is not a matter of adding a second instance, though, because the chat page's own rightward drag already belongs to its sessions drawer. An app-wide gesture and a page-level one on the same side arm on the same touch and fight for the same direction.
What changed
One gesture, bound on the shell.
useDrawerSwipenow runs on[data-testid="dashboard-shell"]— the common ancestor of<main>, the drawer panel and the scrim.<main>looks like the natural root and is the wrong one: the panel and scrim arefixedsiblings of it, so a gesture rooted there can open the drawer but never sees the touch that should close it. The finger lands on the scrim, and the listener is on an element the scrim is not inside.Widening the root does not widen what arms. Dialogs render through
createPortaltodocument.body, so they sit outside the element entirely; the chat page's three overlays (content, sessions drawer, activity panel) all sit inside the element that claims the sides, verified by walking their ancestor chains rather than assumed.A page claims the sides it owns.
data-owns-swipe="left right"on the element the chat page binds its own gestures to. The hook walks from the touch target up to but not including its own root, which is what lets one attribute serve both instances: the claim is strictly below the shell (so the app-wide instance stands down) and is the page instance's own root (so the page proceeds). A naive ancestor walk finds the claim on the page's own element and silently disables the sessions drawer — that case is the first test in the file.The mechanism fails open: no attribute means the app-wide gesture works. A page that forgets to declare gets a visible conflict; the inverse default would let one missing attribute kill the gesture dashboard-wide with nothing to see. The claim is therefore gated on the same condition as the bindings (
embedded ? undefined : 'left right') — an embedded chat renders inside the shell at full width on mobile and binds nothing there, so an unconditional claim defeated that default from the one place that declares.A panel with a gesture must be bound live. The nav panel read
mobileNavX.get()into an inline transform at render time. A MotionValue deliberately does not re-render React, so a drag wrote the value every frame while the DOM moved only on the single re-render the gesture's ownsetDraggingcauses: the panel came out a little, froze, and completed only on release when the settle took over. Correct while the tap was its only mover — and the hook's own comment documented that premise ("the nav drawer is a plain<nav>… having no gesture"), which is exactly what adding a gesture invalidated. It is nowmotion.navwithstyle={{ x: mobileNavX }}, matching the sessions drawer and the right overlay. The compositor settle still runs throughmobileNavPanelRef; framer and that animation coexist here as they already do for the other two, becausetakeOverDraweradopts and cancels whatever is running before either writes.The scrim had the same defect in its other half: a literal
opacity: 0, so it could not dim with the finger (the tap path looked right because the compositor settle animates the scrim in lockstep). It now derives from the panel's offset, divided by the drawer's own travel, so the dim reaches 0 exactly as the panel clears the edge.The hook comment that named the nav drawer as the gestureless plain-
<nav>case is rewritten in the same commit, since that is no longer true of any panel it serves.Ownership is declined, never contested. Three defects found on-device after the
above, all of them the same shape — the gesture claiming a touch that already belonged
to something else — and all reported by @Rayrayxu:
A drag over a code block or table opened a panel. The hook deferred to a
horizontal scroller only while that scroller could still reveal content in the drag's
direction, the handoff you would give a scrollable PARENT. A freshly rendered code
block sits at
scrollLeft: 0, so the first rightward drag on it had nothing to revealand summoned the drawer; the mirror case is a table already scrolled to its right
edge. Deference is now unconditional and decided at touchstart.
That fix did not reach a finished chat code block, because the scroller was never
FOUND.
e.targetread outside a shadow root is retargeted to the host, and a completeblock renders through
@pierre/diffs, whosediffs-containercarries the overflow onan element inside its shadow root — so walking
parentElementsaw a host with nothingto scroll. The chain is now built from
composedPath(), which crosses the boundary.The page kept scrolling under the moving drawer, and a release could fire a button.
The four touch listeners are
passive: true— what keeps a touch that never becomes agesture on the browser's scroll fast path — and a passive listener may not
preventDefault(). Suppression is therefore attached only once the gesture LOCKS: anon-passive
touchmovefor the rest of the gesture plus a one-shot capture-phaseclickswallower for the release, both onwindow, both released when it ends. Theswallower is not redundant with
preventDefault: a touch that BEGAN on a button andthen moved still fires its click.
Swiping the drawer shut and immediately swiping it back open failed, intermittently
and with the direction perfectly clean. The gesture judged itself against the
openprop, which LAGS: the consumer learns the new state from
onSettle, called in thesettle animation's completion callback, so for the whole ~200-300ms of a closing slide
the prop still says open. A re-opening drag started in that window read as an opening
drag on an already-open panel and was declined outright. A settle now commits its own
target the moment it starts, and the prop is adopted when it CHANGES — the authority
for a panel opened by tap rather than by gesture. Both halves are load-bearing:
without the first, re-opening is declined; without the second, a hamburger-opened
drawer cannot be dragged shut.
The last of those exposed a fourth, and it is the reason this is framed as declining
rather than winning: the browser decides ownership first, and once it has committed a
touch to a scroller nothing takes it back —
preventDefault()is ignored. A diagonaldrag was where its rule and this hook's disagreed: a dy just under dx passed the "is this
vertical?" test while dy alone had already started a scroll, so the drawer arrived to
find the page moving under it. A gesture whose vertical drift reaches 8px (deliberately
below the 10px axis lock) is now abandoned instead of fought for.
Reading the platform's own answer looks better and is not safe to act on: an engine marks
a touchmove non-cancelable once it owns the touch, but
cancelableis false by defaulton a synthetic event and is not guaranteed true for an ordinary touchmove delivered to a
passive listener — and one false reading abandons every gesture, spending the whole
feature to fix an occasional drag. That was not reasoned around, it was measured: the
attempt reddened seven existing tests. A displacement threshold is engine-independent and
fails toward keeping the gesture.
Evidence
Same recording as mp4
recorded from 3aec8eb5 · isolated pod at 390x844 · real touch via CDP Input.dispatchTouchEvent · real server, no fixturesThe clip predates the gesture-ownership round below and is deliberately not re-pinned to HEAD, which did not produce it. What it demonstrates is unchanged: those drags are horizontal, and a horizontal drag still opens the drawer. What changed since is which touches the gesture DECLINES.
In order: the Schedule page — a surface that had no gesture at all — where a rightward drag from mid-screen pulls the drawer in tracking the finger; a leftward drag taking it back out; then one gesture that goes out part way, holds, reverses and releases, which is the beat a render-time snapshot cannot produce; and finally the chat page, where the same rightward drag still opens the sessions drawer, because that page claims both sides.
The blue dot and trail are a recording aid injected by the capture scenario, not app UI. They are painted from the real touch events rather than written per step by the driver, which would add a round-trip to the intervals the release-speed window is measured over.
Tests
drawerSwipeOwnership.test.ts(new, 8 cases) covers the claim: the claiming page keeps its own gesture, an instance rooted above stands down, only the claimed sides are suppressed,leftydoes not satisfy a claim onleft, a claim on the instance's own root is ignored, and — the case<main>could not serve — a sibling of the claiming page still arms, which is the closing drag.Eleven mutations on the original mechanism, each verified to redden: the claim check removed; the walk including its own root (self-suppression); substring instead of side-list matching; the side ignored so any claim suppresses everything; the binding regressed to
<main>;onGestureOpendropped so the drawer never mounts mid-gesture; the panel regressed to a render-time snapshot (the reported defect); the scrim regressed to a literal opacity; the scrim divided by the viewport instead of the panel's travel; the chat page's claim removed; and the gesture bound below<main>.Seventeen more cover the ownership round: five on the shadow-boundary scroller search (the
composedPath()call dropped, the chain excluding its own root, the ownership walk no longer excluding it, the deference deleted, and the fixture regressed to a non-composed event), eight on the page suppression (no suppression at all, touchmove not prevented, the click not swallowed, the swallower never released, no timeout fallback, suppression not ended, not released on unbind, and the fixture regressed to a non-cancelable event), and four on the diagonal decline (the rule removed, its threshold raised above the axis lock so it cannot fire, lowered to 1px so it rejects every real swipe, and the decline weakened from abandoning the gesture to merely postponing it). Two more cover the state lag: the settle no longer committing its target (the reported re-open failure), and the prop change no longer being adopted. The second of those SURVIVED the first version of its test, which simulated a prop change by remounting — a fresh mount initialises correctly whether or not adoption exists, so it could not fail; rewritten withrenderHookinitialPropsplus a realrerender, it reddens.Two of those fixture mutations are there because the tests they guard were INERT when first written. The shadow test passed while nothing ran, because the fixture's events lacked
composed: trueand so never escaped the shadow root; the scroll-suppression test passed whilepreventDefault()was correctly skipped, because the fixture's events were notcancelable. Both now carry a mutation that reverts the fixture, so a test that stops exercising its subject fails instead of passing quietly.tsc -b, eslint anddocs-lintclean; the drawer, ownership, two-panel, settle and compositor suites green locally (120 cases). The full frontend suite runs in CI on this head.Why no linked issue: reported directly during device testing.
A modal layer owns every touch inside it, read from its
role. A dialog is notnecessarily portaled out of the shell -- the changelog and update-error overlays are plain
fixed inset-0JSX inside it (the shell element spansApp.tsx2635-3878 and both sitbetween) -- so a horizontal drag across one pulled the nav drawer out BEHIND the dialog.
The hook now stands down for
role="dialog"/role="alertdialog"in the chain, read as arule rather than a list of overlays. Only those two roles count: treating any
roleasownership would hand away most of the page, which is the third mutation.
A suppression that has ENDED is not reused. It stays parked for ~350ms waiting to eat
the release's click, with its touchmove listener already removed -- and that window is
exactly the "swipe shut, swipe straight back open" beat the settle-commit fix just made
reachable, so inheriting it left the second of two quick drags with no scroll suppression
at all. A new lock releases it and installs a fresh one.
A drag widget needs no attribute either. The hook also yields to any element in the chain whose computed
touch-actionisnone— the platform's own declaration that it took touch handling from the browser. Sliders, resize handles, column splitters and pinch-zoom canvases are not horizontally scrollable (so scroller deference misses them) and run on POINTER events, whosepreventDefaultdoes not stop the touch stream reaching an ancestor listener. There are around a dozen such widget families insrc/today, so a per-widget attribute would be a list that goes stale on the next one. Only a fullnonecounts: the root setspan-x pan-yunder a coarse pointer, and treating that as ownership would kill the gesture everywhere.