Skip to content

feat(nav): open the mobile nav drawer by swipe on every page - #7133

Merged
buluoray merged 1 commit into
mainfrom
feat/global-nav-swipe
Aug 31, 2026
Merged

feat(nav): open the mobile nav drawer by swipe on every page#7133
buluoray merged 1 commit into
mainfrom
feat/global-nav-swipe

Conversation

@buluoray

@buluoray buluoray commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

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. useDrawerSwipe now 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 are fixed siblings 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 createPortal to document.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 own setDragging causes: 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 now motion.nav with style={{ x: mobileNavX }}, matching the sessions drawer and the right overlay. The compositor settle still runs through mobileNavPanelRef; framer and that animation coexist here as they already do for the other two, because takeOverDrawer adopts 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 reveal
    and 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.target read outside a shadow root is retargeted to the host, and a complete
    block renders through @pierre/diffs, whose diffs-container carries the overflow on
    an element inside its shadow root — so walking parentElement saw a host with nothing
    to 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 a
    gesture on the browser's scroll fast path — and a passive listener may not
    preventDefault(). Suppression is therefore attached only once the gesture LOCKS: a
    non-passive touchmove for the rest of the gesture plus a one-shot capture-phase
    click swallower for the release, both on window, both released when it ends. The
    swallower is not redundant with preventDefault: a touch that BEGAN on a button and
    then 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 open
    prop, which LAGS: the consumer learns the new state from onSettle, called in the
    settle 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 diagonal
drag 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 cancelable is false by default
on 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

On the Schedule page a rightward drag pulls the nav drawer in under the finger with the scrim dimming in step, a leftward drag takes it back out, a drag reversed mid-gesture returns it, and on the chat page the same rightward drag still opens the sessions drawer instead

Same recording as mp4

recorded from 3aec8eb5 · isolated pod at 390x844 · real touch via CDP Input.dispatchTouchEvent · real server, no fixtures

The 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, lefty does not satisfy a claim on left, 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>; onGestureOpen dropped 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 with renderHook initialProps plus a real rerender, 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: true and so never escaped the shadow root; the scroll-suppression test passed while preventDefault() was correctly skipped, because the fixture's events were not cancelable. 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 and docs-lint clean; 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 not
necessarily portaled out of the shell -- the changelog and update-error overlays are plain
fixed inset-0 JSX inside it (the shell element spans App.tsx 2635-3878 and both sit
between) -- 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 a
rule rather than a list of overlays. Only those two roles count: treating any role as
ownership 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-action is none — 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, whose preventDefault does not stop the touch stream reaching an ancestor listener. There are around a dozen such widget families in src/ today, so a per-widget attribute would be a list that goes stale on the next one. Only a full none counts: the root sets pan-x pan-y under a coarse pointer, and treating that as ownership would kill the gesture everywhere.

@buluoray
buluoray requested a review from a team August 30, 2026 22:43
@buluoray
buluoray requested a review from a team as a code owner August 30, 2026 22:43
@buluoray
buluoray requested a review from chenmingwei23 August 30, 2026 22:43
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 30, 2026
@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 8db6b591e8fa2fe108bab5b3ec43051d9a0cf983 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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 page-layout.md in the same commit, the fix is at root cause (live MotionValue binding, ownership decided at touchstart), and it's frontend-only and fully reversible.

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

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 8db6b591e8fa2fe108bab5b3ec43051d9a0cf983 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- website/src/hooks/useDrawerSwipe.ts:762 -- checking "role" only on ancestors misses the changelog backdrop, so swiping it opens navigation behind the modal -> Fix: make the changed ownership check recognize a backdrop containing an immediate modal dialog. (origin: validation)
[GPT-REVIEWED] 8db6b59

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 8db6b591e8fa2fe108bab5b3ec43051d9a0cf983: <one-sentence reason>

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

UX-level review of 8db6b591e8fa2fe108bab5b3ec43051d9a0cf983 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 8db6b591e8fa2fe108bab5b3ec43051d9a0cf983 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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 data-owns-swipe side grammar, which generalizes for a consumer that doesn't exist.

What this change ships

Intent: let a phone user open the nav drawer by swipe on any page — declared an ADDITION, honestly ("an ergonomics addition, not a repair").

  1. Nav drawer opens by rightward swipe on every page — justified (named requester)
  2. Drawer panel and scrim now track the finger during a drag — justified (on-device defect)
  3. Scrim dims proportionally with drag progress — justified (same defect)
  4. Drags over horizontal scrollers never summon a panel, even at scroll ends — changed default, declared, justified
  5. Drags over shadow-DOM code blocks scroll the code, not the drawer — justified (reported defect)
  6. Page stops scrolling under a locked drag; release can't fire a button — justified (reported defect)
  7. Diagonal drags are ceded to the browser's scroll — justified (platform ownership rule)
  8. Swipe-shut-then-reopen works mid-settle — justified, fixed at cause (in-hook, all three consumers)
  9. In-shell dialogs and touch-action:none widgets own their touches — justified (App.tsx:3234, 3254 verified)
  10. New data-owns-swipe per-side page contract — one consumer, generalized

Demo media under temp-screenshots/ follows the PR-template convention; not a rider.

Watch

The change reuses the existing useDrawerSwipe/registerDrawerTargets/openMobileNav machinery rather than adding a parallel gesture path — the delete-option was taken. The one premise risk is item 10, below.

Subtractions

  • Shrink data-owns-swipe from a side list to presence-only. Grepped data-owns-swipe|ownsSwipe across website/src: one producer (ChatPage.tsx:7132), one value ever written ('left right'), and the only app-wide instance is left-anchored (useDrawerSwipe(shellRef, …) takes the side = 'left' default), so the 'right' token gates nothing and claim.split(/\s+/).includes(side) in useDrawerSwipe.ts:752 reduces to a boolean. The "suppresses only the sides actually claimed" test pins behavior no shipped instance can reach; delete it with the grammar.

[FIRST-PRINCIPLES-REVIEWED] 8db6b59

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 8db6b591e8fa2fe108bab5b3ec43051d9a0cf983 — this comment is updated in place on each push.

Review details

Non-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 settle() has committed openRef.current = false; the re-open drag then locks with gestureBase.current = openRef.current ? 0 : closedOffset() and the if (!openRef.current) branch does x.set(closedOffset()), so takeOverDrawer's adopted ~−100 is overwritten and the panel jumps to clampOffset(closedOffset() + dx) ≈ −218, then lags the full travel behind the finger — a reverse-snap on this feature's core "swipe shut, swipe straight back open" beat (reachable via both the gesture-close settle and the scrim-tap closeMobileNavDrawer compositor close) → Fix: when locking a re-open while a settle is still presenting a partial offset, base gestureBase on the panel's current offset (x.get()) rather than unconditionally closedOffset(), and skip the offscreen seat when the panel is already mounted and visible.

[OPUS-REVIEWED] 8db6b59

Verdict parsed from the review's SHA-scoped output markers for commit 8db6b591e8fa2fe108bab5b3ec43051d9a0cf983.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 8db6b591e8fa2fe108bab5b3ec43051d9a0cf983: <one-sentence reason>

@buluoray
buluoray force-pushed the feat/global-nav-swipe branch from eb9335f to 0373a94 Compare August 30, 2026 23:10
@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Aug 30, 2026
@buluoray
buluoray force-pushed the feat/global-nav-swipe branch from 0373a94 to 9de5b86 Compare August 30, 2026 23:38
@buluoray

Copy link
Copy Markdown
Contributor Author

Disposition — UX CONCERNS and First Principles CONCERNS on eb9335f5d

Both lanes landed on the same defect independently. I verified it against source before changing anything, and it is real and reachable.

Fixed — the claim outlived its ownership

ChatPage authored data-owns-swipe="left right" unconditionally while both of its instances are gated enabled: isMobile && !embedded && …. The reachable path is not the /embed/* routes (those render in a branch with no shell, so there is no instance to suppress) but the three in-dashboard embeds, which do render inside the shell at full width on mobile:

  • src/components/ArtifactChatPanel.tsx:115
  • src/apps/papyrus/CoAuthorPanel.tsx:93
  • src/app-sdk/ChatPanel.tsx:34

There the claim suppressed the shell's nav swipe while binding nothing — a dead gesture across the whole screen. Worse than the friction: it defeated the fail-open default from the one place that declares, which is the property the PR body claims. Both reviews are right that a static claim over conditional ownership is the bug, not the symptom.

Adopted verbatim: data-owns-swipe={embedded ? undefined : 'left right'}. Not additionally gated on isMobile, and deliberately so — the app-wide instance is mobile-only, so a desktop claim suppresses nothing, and adding the term would imply the attribute carries a guarantee about a case it cannot affect.

Two mutations verified to redden: the claim made unconditional again, and the claim inverted (declared only when embedded). website/docs/page-layout.md now states the ownership rule beside the contract, and the PR body's fail-open paragraph is corrected to name this case rather than asserting the property unqualified.

Declined — collapsing the side-list grammar to attribute presence

First Principles is factually right about today's counts: one claim ever authored, both sides at once, and one above-page instance which is left-side, so split(/\s+/).includes(side) distinguishes nothing in production right now. I am keeping it, for two reasons that are not "a future page might".

It reuses an existing concept rather than inventing one. side is already DrawerSwipeOptions.side, the hook's own option, with the same left/right vocabulary and the same meaning (the edge a panel is anchored to). The attribute names the thing the hook already names; presence-only would be a second, coarser vocabulary for the same distinction.

Presence-only fails closed in exactly the direction this design rejects. A page that owns only one side has no way to say so, so the day an above-page right-side panel appears, every existing claim silently suppresses it too — the same silent-kill the fail-open default exists to prevent, reintroduced at a different layer. The cost of keeping it is one .includes and one sentence of documentation; the cost of removing it is a migration touching every claimer plus a window where the failure is invisible.

Happy to be overruled if you would rather ship the smaller surface — it is a one-line change either way.

@buluoray
buluoray enabled auto-merge (squash) August 30, 2026 23:42
@buluoray
buluoray force-pushed the feat/global-nav-swipe branch from 9de5b86 to 0d94648 Compare August 30, 2026 23:58
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 31, 2026
@buluoray
buluoray force-pushed the feat/global-nav-swipe branch from 0d94648 to ca8a52a Compare August 31, 2026 00:26
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 31, 2026
@buluoray

Copy link
Copy Markdown
Contributor Author

Disposition — UX CONCERNS on 0d94648b9, and the twice-raised subtraction

Fixed — and the class is larger than the one widget

UX is right that a rightward drag on the settings Slider pulled the nav drawer out mid-adjustment. Verified against source: the track carries role="slider" and touch-none, is driven by pointer events, and its preventDefault() is on pointerdown — pointer and touch are separate streams, so the touch stream still reaches the shell listener, passes the axis lock, finds no horizontal scroller and no claim, and arms.

I took the generalized form rather than the per-widget attribute, because counting the instances changes the shape of the fix: touch-action: none appears 28 times across src/, and every one of them is a widget that owns a drag — ResizeHandle, ColumnSplitter, BottomTerminalPanel, DetailPanel, SessionGridLayout, FileBrowserRail, ChatInput's resize handle, DiagramLightbox and MarkdownRenderer's pinch-zoom targets, useSceneInteraction, the Slider. So this is not one surface that forgot an attribute; it is roughly a dozen widget families that would each have to remember one, forever.

dragOwnedBelow now yields to any element in the chain whose computed touch-action is none. That is the platform's own declaration that the element took touch handling from the browser — the same kind of rule as deferring to a horizontal scroller, not a bespoke opt-out. Only a full none counts: the root sets pan-x pan-y under a coarse pointer to switch page zoom off, and treating that as ownership would kill the gesture everywhere.

Five cases pinned behaviourally (widget on an ancestor, widget as the touch target itself, pan-y must NOT suppress, none on the instance's own root must not self-suppress, and the closing drag still arming), with three mutations verified to redden: the check removed, any touch-action treated as ownership, and — caught by mutation rather than by review — an inline-style read I had added on a false premise. I claimed jsdom could not resolve the class; the mutation showed getComputedStyle covers inline in jsdom too, so the extra read was dead code justified by a wrong comment, and it is gone.

Docs and PR body updated: the contract is a rule about ownership, not an attribute, so both now say so.

The side-list subtraction, raised a second time

First Principles' counts are correct and unchanged: one claimant, both sides, one above-page instance on the left, so per-side matching distinguishes nothing shipped today. My reason for keeping it also hasn't changed — side is already DrawerSwipeOptions.side, so the attribute mirrors an existing concept, and presence-only fails CLOSED for a future above-page right-side instance, which is the failure direction this design specifically rejects.

Two rounds, two defensible positions, and the cost is one .includes either way. Rather than re-argue it a third time I am leaving the call to @Rayrayxu: say the word and I will collapse it to attribute presence in one commit. Note that today's fix makes the argument for subtraction slightly stronger, not weaker — the widget class is handled by touch-action, so no second attribute consumer arrived.

@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Aug 31, 2026
@buluoray

Copy link
Copy Markdown
Contributor Author

Disposition — First Principles CONCERNS on ca8a52ad6

Fixed by stating the provenance, not by code

The premise finding is fair and it is a defect in the description, not the diff: I justified the capability by symmetry ("every other mobile panel is swipeable"), which is an analogy, and the review is right that no need was named.

The real provenance is stronger than the analogy and was simply missing: this was requested by @Rayrayxu while using the dashboard on a phone, directly after #7073 made the chat panels draggable — which left the nav drawer as the one mobile panel still tap-only. The same reporter verified the finished gesture on-device, and that pass is what surfaced the two defects fixed in this PR (a panel that did not track the finger, and a drawer that could not be swiped shut). The body now says this, and says plainly that the hamburger did work, so this is an ergonomics addition rather than a repair.

Body edit only — the SHA is unchanged, so this comment will read against ca8a52ad6 on the next pass.

Also noting the review independently corroborated the generalization I based the slider fix on: it grepped 23 non-test files carrying touch-none|touchAction and confirmed the widget-family count, and found 3 production useDrawerSwipe( instances all reusing the one hook.

The side-list subtraction — third raising, awaiting @Rayrayxu

Same subtraction as eb9335f5d and 0d94648b9, with the counts unchanged and correct. I declined it twice with reasoning (side already exists as DrawerSwipeOptions.side; presence-only fails CLOSED for a future above-page right-side instance) and escalated it in issuecomment-5472231193 rather than re-litigating.

I am not going to argue it a third time, and I am not going to implement it unilaterally either — it is a judgment call between two defensible positions with a one-line cost either way, and it belongs to the human. @Rayrayxu: say "collapse it" and it becomes a bare data-owns-swipe presence check in one commit.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 31, 2026
@buluoray
buluoray force-pushed the feat/global-nav-swipe branch from ca8a52a to 3aec8eb Compare August 31, 2026 01:11
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 31, 2026
@buluoray
buluoray force-pushed the feat/global-nav-swipe branch from 3aec8eb to 7b74984 Compare August 31, 2026 02:10
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: passed Eligible automated validation passed for the current revision readiness: checking Automated validation is still running labels Aug 31, 2026
@buluoray
buluoray force-pushed the feat/global-nav-swipe branch from 7b74984 to 7405b64 Compare August 31, 2026 03:31
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 31, 2026
@buluoray
buluoray force-pushed the feat/global-nav-swipe branch from 7405b64 to 24e1ad3 Compare August 31, 2026 03:54
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 31, 2026
@buluoray
buluoray force-pushed the feat/global-nav-swipe branch from 24e1ad3 to 59d2ad9 Compare August 31, 2026 04:31
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 31, 2026
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.
@buluoray
buluoray force-pushed the feat/global-nav-swipe branch from 59d2ad9 to 8db6b59 Compare August 31, 2026 04:44
@buluoray

Copy link
Copy Markdown
Contributor Author

Disposition for the two CONCERNS on 59d2ad946. Head is now 8db6b591e.

UX — FIXED, and it was a real defect in the common case

The finding is right, and its evidence is my own comment: the locked gesture's preventDefault() already suppresses the synthesized click for a drag over non-interactive content, so nothing ever arrived to disarm the swallower. It stayed armed for the full CLICK_SWALLOW_MS and the next tap — the user's real one — was what it ate. Not a rare path: that window sits exactly on this feature's core beat, swipe the drawer open and immediately tap something in it.

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 ended() to any live suppression does not redden anything, because the only way a touchstart fires mid-drag is a second finger, which abandon()s and ends the suppression anyway. The ended() check is defensive precision with no observable difference, so there is no honest test to pin it, and I did not invent one.

First Principles — ACCEPTED as a real design argument, DEFERRED, and it needs the repo owner's call

Not 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 embedded gate), and registerDrawerTargets at useDrawerSwipe.ts:434 is genuine in-file precedent for the module-registry pattern. A registry keyed on (root, side) would also delete the awkward part of the current design — the "walk up to but NOT including my own root" asymmetry — because an instance would compare against other instances' roots instead of needing the walk to exclude itself.

Deferring it here for reasons of blast radius rather than merit:

  • It is not a defect fix. The mechanism works, fails open by design, and its one desync has been fixed and pinned.
  • It replaces a declarative contract with mutable module state whose correctness then depends on mount/unmount lifecycle — StrictMode double-invocation, stale entries on unmount, and ordering between an app-wide instance and a page instance mounting in the same commit. registerDrawerTargets is precedent for the pattern, not evidence that this particular lifecycle is safe; that needs its own verification.
  • It would touch App.tsx, ChatPage.tsx, the hook, delete the adjacency/synchrony tests and rewrite the doc's placement rules — and this PR's gesture behaviour has been verified on a real device across several rounds. Swapping the ownership mechanism underneath that invalidates the verification, so it wants its own PR and its own device pass.

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.

@buluoray

Copy link
Copy Markdown
Contributor Author

Follow-up filed for the First Principles finding: #7210 — replace data-owns-swipe with a registry of bound roots. It captures the argument (the attribute duplicates what the hook knows at bind time; the drift failure mode is real, this PR fixed one instance of it; registerDrawerTargets is in-file precedent), the shape, and the four lifecycle risks that have to be verified before it is safe (StrictMode double-invocation, stale entries — which would invert today's fail-OPEN default — same-commit mount ordering, and behavioural equivalence for the sibling-scrim closing drag). Not in this PR, per the reasoning in my disposition above.

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 31, 2026
@buluoray
buluoray merged commit 83e267a into main Aug 31, 2026
76 of 78 checks passed
@buluoray
buluoray deleted the feat/global-nav-swipe branch August 31, 2026 05:45
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants