Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
community
meta-cla
Bot
added
the
CLA Signed
…field A press defers the time-option show past the click that ends it with a one-shot `document` click listener. Nothing removed that listener when the field unmounted first, so a press that never became a click left a watcher standing that answered the next click anywhere in the document with work for a field that was gone. The watchers now register through one helper, and unmounting drops every watcher still pending.
eef23e7 to
abab484
Compare
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| DateTimeInput · Storybook | 188.4 kB (+108 B) · 17.3 kB CSS (0) | 3,567 (+12) | ✓ · ✓ · ✓ |
Package size: @astryxdesign/core 6.61 MB packed (+738 B); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 19 rows, core 23 rows; run pnpm api:report.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
|
AI review status for this pull request.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested. The fix itself holds up: pending document click listeners are now removed when the field unmounts, the new test fails without the fix and passes with it, and behavior while the field is mounted is unchanged. Only the pull request description needs a correction:
- The User impact section says a leftover listener calls
timePopover.show()on the unmounted field, keeps its closure "for the life of the page" and accumulates "indefinitely", but in the case the new test reproduces a press leaves only the twotimePointerActiveRefreset listeners (the deferred-show listener is added and run by the same click) and the next click that reachesdocumentruns and removes all of them, so please reword it to that scope or add a reproduction for the broader claims.
CI has not run on this head yet; it waits for a maintainer's ci:run label. Two small comment-wording suggestions are inline and non-blocking.
[Automated review]
| // The watchers carrying that deferral are one-shot listeners on `document`, | ||
| // and they belong to this field. Unmounting takes them with it: a watcher | ||
| // left standing answers the next click anywhere in the document with work for | ||
| // a field that is gone. |
There was a problem hiding this comment.
Non-blocking (comment wording): the first sentence carries two topics, and "left standing" and "answers the next click ... with work" are idioms. A shorter version that keeps the reason:
// These one-shot `document` listeners belong to this field. Unmount removes
// them. Otherwise the next click runs work for a field that is gone.There was a problem hiding this comment.
Adopted in 5de3ece — the comment now reads exactly as suggested:
// These one-shot \`document\` listeners belong to this field. Unmount removes
// them. Otherwise the next click runs work for a field that is gone.| // The `once` option removes this listener as the browser delivers it; | ||
| // deleting here is what keeps the set from holding every past press. |
There was a problem hiding this comment.
Non-blocking (comment wording): two topics joined by a semicolon. Suggested:
// The `once` option removes the listener from the document. Delete it from
// the set too, or the set keeps every past press.There was a problem hiding this comment.
Adopted in 5de3ece:
// The \`once\` option removes the listener from the document. Delete it from
// the set too, or the set keeps every past press.| // The press defers its show to a one-shot `document` click watcher, and a | ||
| // press that ends without a click leaves that watcher standing. It belongs |
There was a problem hiding this comment.
The press itself does not defer the show: it installs the two listeners that end its deferral (markTimePointerActive, from the input and from the wrapper), and those are what this test counts. The deferred-show listener is added only by the input's click, and that same click runs it. Suggested opening: "A press installs one-shot document click listeners that end its deferral, and a press that ends without a click leaves them in place."
There was a problem hiding this comment.
You are right, and the test now says so. Adopted in 5de3ece, with the title widened to match what a press actually installs:
'drops the watchers of a press that never became a click when the field unmounts'
The opening follows your sentence, and the rest records the measurement behind it. I re-ran the interaction with the same spy the test uses (bubble-phase click on document, counting real deliveries):
- press only: installed 2, delivered 0 — the pair from
markTimePointerActiveon the input and on the wrapper. - press, then a click on the field: installed 3, delivered 3, 0 standing. The third is
showTimeOptions' deferred show, and the click that installs it reachesdocumentin the same dispatch, so it is consumed at once and cannot outlive the field. That is the case I wrongly folded into the description'stimePopover.show(). - two presses with no click: installed 4; one
document.bodyclick delivers all 4, and a second delivers nothing more. So the count is bounded by the next click, not unbounded —for the life of the pageandaccumulates indefinitelywere both wrong, and the User impact and harm paragraphs now carry the measured scope instead.
User impact now claims only what reproduces: the two reset watchers outlive the field, the work each does is timePointerActiveRef.current = false on a detached ref, and they are released by the next click that reaches document — or stay reachable for the rest of the page if none arrives.
The base witness changed shape with the corrected assertion, so it is re-quoted: git show origin/main:packages/core/src/DateTimeInput/DateTimeInput.tsx, then NO_COLOR=1 npx vitest run --project ui packages/core/src/DateTimeInput/DateTimeInput.test.tsx -t 'drops the watchers' fails at DateTimeInput.test.tsx:2322 with expected 2 to be +0 — the later click delivered 2 listeners of an unmounted field. With the fix it delivers 0. The three mutation arms were re-run on this head too: deleting the useEffect fails, deleting the removeEventListener loop fails, deleting watchers.delete(watcher) passes, exactly as stated.
…eaves pending Review asks for the narrower wording. The comments now say that a press installs two one-shot `document` listeners that end its deferral, and that the deferred show is not one of them: the input's own click installs it and delivers it in the same dispatch.
|
Short answer: the description overstated the defect, and 5de3ece corrects it. The fix itself is unchanged — no line of the source behavior moved, only comments and the test title. What I got wrong, and what measures now:
So the defect is the lifecycle rule, not a visible malfunction: Both non-blocking comment suggestions are adopted verbatim, and the test's opening follows your sentence. Renamed: Re-run on this head: Ready for a re-review. |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, and a maintainer adds the ci:run label so it runs.
The description now matches what the new test reproduces, and the fix holds up: the press's pending document click listeners are removed when the field unmounts, the test fails without the fix and passes with it, and behavior while the field is mounted is unchanged. Nothing else is needed from you.
[Automated review]
cixzhang
added
the
ci:run
github-actions
Bot
removed
the
needs:code-review
User impact
A
DateTimeInputwithtimeOptionIntervalset ends a pointer press's deferral with one-shotdocumentclick listeners, installed once each by the time input and by its wrapper. When the field unmounts before the next click arrives — the dialog holding it closes on the same gesture, a repeatable form row disappears, a wizard step changes — both listeners stay installed: measured, a press installs 2 and delivers 0, and onmainunmounting releases neither. The next click anywhere in the document then runs this field's work for a field that is gone, and if no further click reachesdocumentthe field's closure stays reachable for the rest of the page.The restoration gives the field ownership of its listeners, so no later click answers for a component that no longer exists. Its scope is measured rather than assumed: only the press's two reset watchers can be left standing (
timePointerActiveRef.current = falseon a detached ref) — the deferredtimePopover.show()is installed and delivered by the input's own click inside one dispatch, measured at 3 installed, 3 delivered, 0 standing — and nothing accumulates past a click, becauseoncedelivers every pending watcher on the next click that reachesdocument(two presses install 4; one document click delivers all 4).Problem and solution fit
Why does the defect occur? Light dismiss counts a press in a text input as a press outside the list (a text input cannot be an invoker), so the field defers the show, and its own "the press is over" reset, to
{once: true}listeners ondocument. Theonceoption means only a delivered click removes them, and nothing tracked them, so unmount had no way to release them.Why does that harm the task? The gesture the deferral exists to serve ends when the field goes away, but its two watchers outlive it. A click the user aimed at some other part of the app runs this field's handler instead, and until such a click arrives the page keeps the closure of a component nothing renders. Both bounds are measured above: the work an orphaned watcher does is a ref write, and the count a screen can hold is one pair per press-until-the-next-click.
Why does that harm matter here? The repo already states the rule and already tests it for the sibling field.
architecture:react-component-runtimeINV5 requires component-owned resources to be released at unmount, andTypeahead.test.tsx:561— 'removes its document listeners when the field unmounts with the list open' — is that promise enforced forBaseTypeahead. This is the same deferral pattern copied without the teardown, so the defect is an exception to the project's own documented convention rather than a design choice.Why does the narrow fix close the chain? Both watchers are now created in one place (
addTimeClickWatcher) and dropped in one place (clearTimeClickWatchers, run by the field's unmount cleanup). A press that never becomes a click can no longer leave anything behind, and a press that does become a click still behaves exactly as before because the browser's ownoncedelivery removes it first.Expected behavior and authority
architecture:react-component-runtimeINV5 — Resource ownership is complete (docs/architecture/react-component-runtime.md:137): "Every listener, timer, animation frame, observer, request, subscription, and imperative browser resource has one declared lifecycle owner. Component- or node-owned resources are removed, aborted, or released when disabled, replaced, or unmounted."docs/architecture/react-component-runtime.md:123): "When setup acquires, installs, or starts a resource, cleanup MUST release that resource."documentclick listener (packages/core/src/Layer/gestureCounter.ts:41), which this change deliberately leaves in place. The field's watchers are one per press, so they are component-owned.docs/specs/components/DateTimeInput/DateTimeInput.spec.md:138FR7 (authority: current) is the contract the deferral implements: "While open, the listbox stays open for a press on the time field or in the listbox and closes for a press elsewhere". This change does not alter that behavior; it only ends the deferral with the field.packages/core/src/Typeahead/Typeahead.test.tsx:561.Smallest restoration
PointerDateTimeFieldgains a ref-held set of its pendingdocumentclick watchers. The two sites that installed a raw{once: true}listener —markTimePointerActiveand the deferred branch ofshowTimeOptions— install throughaddTimeClickWatcherinstead, and one unmount cleanup (clearTimeClickWatchers) removes every watcher still pending. No prop, type, visual affordance, default or interaction model changes; the popover, list-open and close-on-press-elsewhere behavior is byte-for-byte the same while the field is mounted.Why not
subscribeInput?pnpm review:prereports, informationally, that Core owns document listeners and points atsubscribeInput(packages/core/src/utils/inputStream.ts). That owner cannot carry this deferral: its native listener sits on the window's capture phase, which its own header documents as running before every document listener (inputStream.ts:16), and the whole purpose of the watcher is to act once the press's click has already reached the list's outside-press watch. Routing the reset through the capture phase would run it before that watch judges the click — a behavior change this bug fix must not make. The other click-shaped owner,awaitBrowserClick(packages/core/src/utils/browserClickWait.ts:41), fires only for clicks inside one element and gives up afterBROWSER_CLICK_WAIT_MS(300 ms,browserClickWait.ts:25), which is again a different contract. Core already keeps a rawwindowclick listener where the phase is load-bearing and releases it explicitly rather than through the stream —clickAndReportadds it in the capture phase and removes it in afinallyatpackages/core/src/utils/browserClickWait.ts:115-119, inside the module the finding names. What INV5 asks for is a declared lifecycle owner, and that is what this change adds.Deliberately split out:
packages/core/src/Typeahead/BaseTypeahead.tsxshowLayer()has the identical shape and the same leak, and I have the same fix for it. Five open PRs already edit that file (#8778, #6360, #4329, #5999, #7910), andgit merge-tree --write-treeagainst aTypeahead-only arm of this change reports a conflict inpackages/core/src/Typeahead/BaseTypeahead.tsxfor four of them (rc=1 for #8778, #6360, #4329, #5999; rc=0 for #7910). None of the five adds a listener removal, so the defect is not being fixed by any of them. Bundling that file here would make this PR depend on their ordering, so it is a follow-up — as soon as the file is not contested, or a maintainer can apply the same three-part change directly.For this PR's file, the only open PRs touching
packages/core/src/DateTimeInput/DateTimeInput.tsxare #10129 (merges cleanly with this change, rc=0) and the draft #6133 (rc=1, conflicting in an unrelated region); neither removes adocumentlistener, so this restoration is not already in flight here.Evidence
git show origin/main:packages/core/src/DateTimeInput/DateTimeInput.tsx, thenNO_COLOR=1 npx vitest run --project ui packages/core/src/DateTimeInput/DateTimeInput.test.tsx -t 'drops the watchers') fails atDateTimeInput.test.tsx:2322withexpected 2 to be +0: after onepointerDownon the time field andunmount(), a click ondocument.bodydelivers 2 of the field's bubble-phasedocumentclick listeners. With the fix the same click delivers 0.NO_COLOR=1 npx vitest run --project ui packages/core/src/DateTimeInput/→ 300 passed (300) over 6 files.document.addEventListener/removeEventListenerwrapped for bubble-phaseclick, counting real deliveries). A press: installed 2, delivered 0. Press then unmount: with the fix, 2 removed and a laterdocument.bodyclick delivers 0; with only theuseEffectremoved, 2 stand and the later click delivers 2. Press then a click on the field: installed 3, delivered 3, 0 standing — the deferred show lives and dies inside one dispatch. Two presses with no click: installed 4; onedocument.bodyclick delivers all 4, and a second delivers nothing more.DateTimeInput.test.tsx:2081'keeps the time list open on a press in the time input, closes it on a press elsewhere' ✓ andDateTimeInput.test.tsx:2245'opens from the keyboard after a pointer press that never became a click' ✓.useEffectthat returnsclearTimeClickWatchers: test fails (the teardown is what is being asserted);document.removeEventListenerloop insideclearTimeClickWatchers: test fails;watchers.delete(watcher)bookkeeping inside the fired handler: test passes. Stated plainly, that line is not behavior-coverable:oncealready removes the listener from the document, so the delete only stops the set from holding every past press. It is memory bounding, and the test does not pretend to guard it.clicklistener the document is given and increments on the call, so it fails when a watcher survives, not when a mock misfires. Asserting teardown this way follows the repo's own isolation check,packages/core/src/utils/clickGuardTeardown.test.ts, which asserts that an armed click guard does not outlive the run that armed it.Scope
BaseTypeaheadhalf is split out, with the conflict measurements that motivated the split above.Testing
From the repo root (Node 24, pnpm 11):
NO_COLOR=1 npx vitest run --project ui packages/core/src/DateTimeInput/→ 300 passed (6 files)NO_COLOR=1 npx vitest run --project ui packages/core/src/DateTimeInput/DateTimeInput.test.tsx -t 'drops the watchers'→ fails on base withexpected 2 to be +0, passes with the fixcd packages/core && npx tsc --noEmit -p tsconfig.json→ exit 0npx eslint packages/core/src/DateTimeInput/DateTimeInput.tsx packages/core/src/DateTimeInput/DateTimeInput.test.tsx→ exit 0pnpm specs:for --diff origin/main→ the diff is governed byarchitecture:react-component-runtimeINV2, INV3, INV5, INV6 via theeffects-listeners-observersconcern; the two claims that decide it (INV5, INV3) are quoted above, and the fix satisfies the other two by leaving the deferral itself untouched (INV2: the deferred work still runs from the handler that caused it, never from an Effect)pnpm check:repo→ green (runs as the pre-commit hook for this branch's commit)