Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
|
/approve-spec bec2ad3 |
There was a problem hiding this comment.
Changes requested: one blocking finding.
packages/core/src/hooks/useMenuPress.ts:814adds its own documenttouchstartlistener, but Core routes document and window input listeners throughsubscribeInputinpackages/core/src/utils/inputStream.ts(architecture:react-component-runtimeINV16, andpnpm review:preflags this line), so subscribe there and stop the subscription at the firsttouchstartand on detach, asclickGuard.tsalready does for itstouchendsubscription.
The touch-identifier fix itself restores module:DropdownMenu/useMenuPress FR5: the new tests fail on main and pass on this branch. The inline notes on clickGuard.ts stay non-blocking: two comment wordings, and the "names no touches" wording in a comment and the Changeset, which the code applies only when changedTouches is missing. The description's authority line should cite FR5 and spec:AST-094 FR26 rather than FR10.
…tream The touchstart that names the lifting finger is read through subscribeInput, as Core's other document and window input is, and the subscription stops at the first touchstart and on detach. The guard's comment and the changeset say a touchend without a list of changed touches counts as the finger's own.
|
/approve-spec c2be3b6 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet.
The touchstart read now goes through subscribeInput, which resolves the earlier blocking finding, and the touch-identifier fix restores module:DropdownMenu/useMenuPress FR5: the new tests fail on the base and pass on this branch.
Optional, non-blocking:
packages/core/src/utils/clickGuard.ts:33-34: keep the two original header sentences together so "otherwise" still refers to the cancelledtouchend, and add the identifier sentence after them.packages/core/src/utils/clickGuard.ts:163: "its click" holds only once the guard disarms; "Another finger lifted: leave its touchend." says what the code does.- The description's authority line could cite FR5 and
spec:AST-094FR21 and FR26 rather than FR10.
There was a problem hiding this comment.
Changes requested: one blocking finding. The guard change itself works as described: red-first tests back it, and CI is green at this head.
- Miss: the changeset (
.changeset/menu-press-touch-identifier.md) and the description say another finger's tap was lost and the picking finger then made a stray click, but only a synthetic jsdom event order shows this (in Chromium each finger'stouchenddirectly follows its ownpointerup, no click is raised while two fingers are down, and in the order described the guard would still swallow the other finger's click), so please state the guarantee instead, for example "when a finger picks a menu row, the menu cancels only that finger'stouchend, so atouchendfrom another finger is never cancelled", and drop the lost-tap and stray-click sentence.
Non-blocking: the comment at clickGuard.ts:163 says the other finger's click is its own, but while the guard waits it still swallows that click. The description cites spec:AST-094 FR10, while FR21 and the useMenuPress module's FR5 govern this change.
… guard does The changeset states the guarantee (the guard cancels only the picking finger's touchend) instead of a lost tap and a stray click that only a synthetic event order shows. Two clickGuard comments now say what the guard does while it waits.
|
/approve-spec bd97219 |
There was a problem hiding this comment.
Changes requested: one blocking finding, a wording fix in the description. The guard change itself works as described: the new tests fail on the base and pass on this head, and a real finger tap in Chromium still cancels the pick's own touchend.
- Newly live check: the description says "A
touchendthat lists no touches still counts as the picking finger's own", butendsTouch(packages/core/src/utils/clickGuard.ts, lines 186-192) counts only a missingchangedTouchesas the pick's own and treats an empty list as another finger's, so please write "AtouchendwithoutchangedTouches" as the Changeset and the JSDoc already do, and use the same words in the test title atclickGuard.test.ts:309, or make an empty list count as the touch's own in code.
Approval also waits for CI: CI has not run on this head because the pull request is a draft, and it runs once the pull request is marked ready for review.
Claims and files checked
- Changeset, "the click guard cancels only that finger's
touchend, so atouchendfrom another finger is never cancelled": verified (clickGuard.ts:162-173,useMenuPress.test.tsx:667,clickGuard.test.ts:300). - Changeset, the listed surfaces (DropdownMenu, ContextMenu, Selector, TabList, TopNav, SideNav, Breadcrumbs): verified, each uses the menu press hook.
- Changeset, "reads the finger's identifier from the
touchstartthat follows itspointerdown": verified (useMenuPress.ts:732-747). - Changeset, "a
touchendwithoutchangedTouchesstill counts as the picking finger's": verified (clickGuard.ts:188-190,clickGuard.test.ts:309). - Description, today the guard cancels the next
touchendwhichever finger it belongs to: verified (baseclickGuard.ts:145-164). - Description, the picking finger's own lift is still cancelled: verified (
useMenuPress.test.tsx:667; Chromium tap). - Description,
spec:AST-094FR21 andmodule:DropdownMenu/useMenuPressFR5 govern: verified. - Description, the identifier is passed to
armClickGuard, which stays armed when another finger'stouchendcomes first: verified (useMenuPress.ts:620-637,clickGuard.test.ts:300-307). - Description, "a
touchendthat lists no touches still counts as the picking finger's own": false (clickGuard.ts:186-192). - Description, no record lines change: verified (no record in the diff).
- Description, red first with three named failing tests, then 76 passing: verified.
- Description, eslint on the changed files: verified.
- Description, no new public API, visual representation, default or interaction model: verified (no API rows; identical frames).
- Description, the failing tests and the fix keep vjeux's authorship: verified.
- Files:
.changeset/menu-press-touch-identifier.mdclear;packages/core/src/utils/clickGuard.tsclear;packages/core/src/utils/clickGuard.test.tstest title wording (above);packages/core/src/hooks/useMenuPress.tsclear;packages/core/src/hooks/useMenuPress.test.tsxclear.
…he code and the changeset do
|
/approve-spec 8720b0d |
There was a problem hiding this comment.
Approval waits for CI: the required checks were skipped on this head because the pull request is a draft.
No blocking finding: the guard change does what the description and the Changeset say, and the new tests cover both another finger's touchend and the pick's own.
Next step: mark the pull request ready for review so CI runs on this head.
Claims and files checked
- Claim, Changeset package and bump (
'@astryxdesign/core': patch): verified; every changed source file is inpackages/core. - Claim, DropdownMenu, ContextMenu, Selector, TabList, TopNav, SideNav and Breadcrumbs pick through the menu press model: verified (
DropdownMenu.tsx:1477,ContextMenu.tsx:771,Selector.tsx:1753,TabMenu.tsx:318,TopNavMenu.tsx:468,SideNavHeading.tsx:445,BreadcrumbItem.tsx:590). - Claim, the guard cancels only the picking finger's
touchend, another finger's is not cancelled, and the pick's own still is: verified (clickGuard.ts:161-173;useMenuPress.test.tsx:667-676). - Claim, the menu reads the identifier from the
touchstartafter itspointerdownand passes it toarmClickGuard: verified (useMenuPress.ts:732-746,:620-637). - Claim, the guard stays armed for the pick's touch when another finger's
touchendarrives first: verified (clickGuard.ts:162-165;clickGuard.test.ts:300-307). - Claim, a
touchendwithoutchangedTouchesstill counts as the picking finger's own: verified (clickGuard.ts:186-190;clickGuard.test.ts:309-314). - Claim, today the guard cancels the next
touchendit sees, whichever finger it belongs to: verified at the base (clickGuard.ts:145-164). - Claim,
spec:AST-094FR21 andmodule:DropdownMenu/useMenuPressFR5 govern the change and no record lines change: verified; no record file is in the diff. - Claim, smallest restoration (the option and
endsTouch, the identifier read and passed, a[fix]Changeset): verified. - Claim, red first, the three named tests fail on the base source: verified from the base guard, which has no identifier check; the fourth new test passes there.
- Claim, 76 tests pass and eslint is clean on the changed files: the two test files hold 76 cases; not yet shown at this head, since CI has not run.
- Claim, scope (one defect, no tag-along, no new public API, default or interaction model, no internal context in public text): verified.
- Claim, the test and fix commits keep their original author: verified from the commit history.
- File,
.changeset/menu-press-touch-identifier.md: clear. - File,
packages/core/src/utils/clickGuard.ts: clear. - File,
packages/core/src/hooks/useMenuPress.ts: clear. - File,
packages/core/src/utils/clickGuard.test.ts: clear. - File,
packages/core/src/hooks/useMenuPress.test.tsx: clear.
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because the pull request is still a draft.
The guard change does what the description and the Changeset say: only the picking finger's touchend is cancelled, another finger's passes, and the new tests cover both another finger's touchend and the pick's own. There is no blocking finding.
Next step: mark the pull request ready for review so CI runs on this head.
Claims and files checked
- Claim, patch bump for
@astryxdesign/core: verified; every changed source file is inpackages/core. - Claim, DropdownMenu, ContextMenu, Selector, TabList, TopNav, SideNav and Breadcrumbs pick through the menu press model: verified (
DropdownMenu.tsx:1477,ContextMenu.tsx:771,Selector.tsx:1753,TabMenu.tsx:318,TopNavMenu.tsx:468,SideNavHeading.tsx:445,BreadcrumbItem.tsx:590). - Claim, today the guard cancels the next
touchendit sees, whichever finger it belongs to: verified at the base (clickGuard.ts:145-163there). - Claim, the guard now cancels only the picking finger's
touchend, another finger's is not cancelled, and the pick's own still is: verified (clickGuard.ts:161-174;useMenuPress.test.tsx:667-676). - Claim, the menu reads the identifier from the
touchstartafter itspointerdownand passes it when arming the guard: verified (useMenuPress.ts:732-746,:620-637). - Claim, the guard stays armed for the pick's touch when another finger's
touchendarrives first: verified (clickGuard.ts:162-165;clickGuard.test.ts:300-307). - Claim, a
touchendwithoutchangedTouchesstill counts as the picking finger's own: verified (clickGuard.ts:186-192;clickGuard.test.ts:309-314). - Claim, the touch-presentation and menu-press records' click rules govern the change, the spec check passes, and no record line changes: verified; no record file is in the diff.
- Claim, smallest restoration (the option and its check, the identifier read and passed, a
[fix]Changeset): verified. - Claim, red first, the three named tests fail on the base source: verified from the base guard, which has no identifier check; the fourth new test passes there.
- Claim, 76 tests pass and eslint is clean on the changed files: the two test files hold 76 cases; the run itself is pending CI.
- Claim, scope (one defect, no tag-along, no new public API, default or interaction model, no internal context in public text): verified.
- Claim, the test and fix commits keep their original author: verified from the commit history.
- File,
.changeset/menu-press-touch-identifier.md: clear. - File,
packages/core/src/utils/clickGuard.ts: clear. - File,
packages/core/src/hooks/useMenuPress.ts: clear. - File,
packages/core/src/utils/clickGuard.test.ts: clear. - File,
packages/core/src/hooks/useMenuPress.test.tsx: clear.
There was a problem hiding this comment.
Changes requested: one blocking finding, a wording fix in the Changeset and the description. The guard change itself works as described once the menu has named the picking touch, and the new tests pass in CI.
- Miss: the Changeset and the description say another finger's
touchendis never cancelled, but when the menu cannot name the picking touch (itstouchstartlists several new touches and none carries the pointer's id, or it arrives before thepointerdown),touchOfPointerreturnsnulland the guard still cancels the nexttouchendof any finger (useMenuPress.ts:212,clickGuard.ts:162), so the text should say the old rule applies then.
Suggested wording is in the inline comment on the Changeset; the two comment-wording notes are optional.
Claims and files checked
- Claim, patch bump for
@astryxdesign/core: verified; every changed source file is inpackages/core. - Claim, DropdownMenu, ContextMenu, Selector, TabList, TopNav, SideNav and Breadcrumbs pick through the menu press model: verified (
DropdownMenu.tsx:1477,ContextMenu.tsx:771,Selector.tsx:1753,TabMenu.tsx:318,TopNavMenu.tsx:468,SideNavHeading.tsx:445,BreadcrumbItem.tsx:590). - Claim, today the guard cancels the next
touchendit sees, whichever finger it belongs to: verified at the base (clickGuard.ts:145-163there). - Claim, the guard now cancels only the picking finger's
touchend, so another finger's is never cancelled: false when the menu could not name the touch (useMenuPress.ts:212,clickGuard.ts:162). - Claim, the picking finger's own lift is still cancelled: verified (
clickGuard.ts:166-173;useMenuPress.test.tsx:667-676). - Claim, the menu reads the identifier from the
touchstartafter itspointerdownand passes it when arming the guard: verified (useMenuPress.ts:732-746,:620-637). - Claim, the guard stays armed for the pick's touch when another finger's
touchendarrives first: verified (clickGuard.ts:162-165;clickGuard.test.ts:300-307). - Claim, a
touchendwithoutchangedTouchesstill counts as the picking finger's own: verified (clickGuard.ts:186-192;clickGuard.test.ts:309-314). - Claim, the touch-presentation and menu-press click rules govern the change and no record line changes: verified; no record file is in the diff.
- Claim, smallest restoration (the option and its check, the identifier read and passed, a
[fix]Changeset): verified. - Claim, red first, the three named tests fail on the base source: verified by reading the base guard, which has no identifier check; the fourth new test passes there.
- Claim, 76 tests pass in the two test files and eslint is clean: the two files hold 76 cases at this head, and both files and lint pass in CI.
- Claim, scope (one defect, no tag-along, no new public API, default or interaction model, no internal context in public text): verified.
- Claim, both commits keep their original author: verified from the commit history.
- File,
.changeset/menu-press-touch-identifier.md: the blocking finding. - File,
packages/core/src/utils/clickGuard.ts: clear; one optional comment-wording note. - File,
packages/core/src/hooks/useMenuPress.ts: clear; one optional comment-wording note. - File,
packages/core/src/utils/clickGuard.test.ts: clear. - File,
packages/core/src/hooks/useMenuPress.test.tsx: clear.
… touchend passes Once the menu names the picking touch, another finger's touchend passes. A touchend without changedTouches, or a lift whose touch the menu could not name, keeps the old rule. Two comments reworded.
|
/approve-spec f598827 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because the pull request is still a draft.
The guard change does what the Changeset and the description now say: once the menu has named the picking touch, only that touch's touchend is cancelled, and a touch it could not name keeps the next-touchend rule. The earlier wording finding is fixed, and the new cases cover the named-touch path. One optional comment-wording suggestion is inline.
Claims and files checked
- Claim, patch bump for
@astryxdesign/core: verified; every changed source file is inpackages/core. - Claim, DropdownMenu, ContextMenu, Selector, TabList, TopNav, SideNav and Breadcrumbs pick through the menu press model: verified (
DropdownMenu.tsx:1477,ContextMenu.tsx:771,Selector.tsx:1753,TabMenu.tsx:318,TopNavMenu.tsx:468,SideNavHeading.tsx:445,BreadcrumbItem.tsx:590). - Claim, today the guard cancels the next
touchendit sees, whichever finger it belongs to: verified at the base (clickGuard.ts:145-163there). - Claim, once the menu has named the touch, the guard cancels only that finger's
touchendand another finger's passes: verified (clickGuard.ts:163-166;useMenuPress.test.tsx:667-684). - Claim, the picking finger's own lift is still cancelled: verified (
clickGuard.ts:167-174;useMenuPress.test.tsx:667-675). - Claim, the menu names the touch from the
touchstartafter itspointerdownand passes it when arming the guard: verified (useMenuPress.ts:732-746,:620-637). - Claim, the guard stays armed for the picking touch when another finger's
touchendarrives first: verified (clickGuard.ts:163-166;clickGuard.test.ts:300-307). - Claim, a
touchendwithoutchangedTouchescounts as the picking finger's own: verified (clickGuard.ts:187-193;clickGuard.test.ts:309-314). - Claim, a lift whose touch the menu could not name (several new touches with none carrying the pointer's id, or a
touchstartbefore thepointerdown) keeps the next-touchendrule: verified (useMenuPress.ts:205-212; no identifier reaches the guard at:623and:636). - Claim, the touch-presentation and menu-press click rules govern the change and no record line changes: verified; no record file is in the diff.
- Claim, smallest restoration (the option and its check, the identifier read and passed, a
[fix]Changeset): verified. - Claim, red first, the three named tests fail on the base source: verified by reading the base guard, which has no identifier check; the fourth new test passes there.
- Claim, 76 tests in the two test files pass and eslint is clean: the two files hold 76 cases; they and lint passed in CI on the previous revision, whose code differs from this head only in comments.
- Claim, scope (one defect, no tag-along, no new public API, default or interaction model, no internal context in public text): verified.
- Claim, both original commits keep their author: verified from the commit history.
- File,
.changeset/menu-press-touch-identifier.md: clear. - File,
packages/core/src/utils/clickGuard.ts: clear; one optional comment-wording suggestion. - File,
packages/core/src/hooks/useMenuPress.ts: clear. - File,
packages/core/src/utils/clickGuard.test.ts: clear. - File,
packages/core/src/hooks/useMenuPress.test.tsx: clear.
| // when a drill-in has put another row under the finger. When the menu names | ||
| // the touch by its identifier, another finger's `touchend` passes; a touch it | ||
| // could not name keeps the old rule, and the next `touchend` is cancelled. |
There was a problem hiding this comment.
Non-blocking (comment wording): the new sentence runs to 29 words, carries two topics and ends in the passive. Suggest:
// when a drill-in has put another row under the finger. When the menu names
// the touch, the guard cancels only that touch's `touchend`. Without a name,
// it cancels the next `touchend`.
User impact
When a finger picks a row in a DropdownMenu, ContextMenu, or a menu-press surface (Selector, TabList, TopNav, SideNav, Breadcrumbs), the menu cancels that finger's
touchendso the lift does not also click whatever lies under it. Today the click guard cancels the nexttouchendit sees, whichever finger it belongs to.After this change, the click guard cancels only the picking finger's
touchendonce the menu has named the touch, so another finger'stouchendpasses. The picking finger's own lift is still cancelled. AtouchendwithoutchangedTouches, or a lift whose touch the menu could not name (itstouchstartlists several new touches and none carries the pointer's id, or it came before thepointerdown), keeps the old rule: the nexttouchendis cancelled.Problem and solution fit
armClickGuard's touch path cancels the firsttouchendit sees, without checking which touch ended.touchendis outside it, whatever order a browser delivers.spec:AST-094FR21 andmodule:DropdownMenu/useMenuPressFR5: the press model swallows the click of the gesture it tracked, and only that gesture's.The menu press now reads the finger's
Touch.identifierfrom thetouchstartthat follows itspointerdown, and passes it toarmClickGuard. The guard cancels only atouchendthat ends that touch, and stays armed for it when another finger'stouchendarrives first. AtouchendwithoutchangedTouchesstill counts as the picking finger's own, so engines that omitchangedToucheskeep today's behaviour.Expected behavior and authority
spec:AST-094FR21 andmodule:DropdownMenu/useMenuPressFR5. Specs & Decisions gives a consistency stamp; no record lines change.Smallest restoration
clickGuard.ts: atouchIdentifieroption and anendsTouchcheck.useMenuPress.ts: read the touch identifier from the matchingtouchstart, and pass it when arming the guard.[fix]Changeset.Evidence
main's source, three tests fail:clickGuard.test.tsanduseMenuPress.test.tsx, 76 passed.Scope
Testing
vitest run packages/core/src/utils/clickGuard.test.ts packages/core/src/hooks/useMenuPress.test.tsx: 76 passed.Credit
The failing tests and the fix were written by vjeux; both commits keep his authorship.
(Posted by Cindy's agent on her behalf.)