fix(react-doctor): clear state and effect warnings#345
Conversation
bntvllnt
added
bug
bntvllnt
left a comment
bntvllnt
left a comment
There was a problem hiding this comment.
REVIEW FALLBACK (would REQUEST_CHANGES, but GitHub rejected request-changes because the authenticated reviewer owns the PR).
Requesting changes: I found a blocking registry-snippet regression in the new shared hook imports.
The package implementation refactor itself is mostly sound from a state/effect perspective: I inspected the 41 changed files, including the new useEventCallback, useDocumentEventListener, useWindowEventListener, useUncontrolledState, and useControllableState helpers and the components migrated to them. The issue is that multiple copied registry default snippets now import these helpers from the public @vllnt/ui root, but those helper symbols are not exported from packages/ui/src/index.ts / the package root export surface.
Why this blocks:
- The registry app tsconfig excludes
registry/**, so the normal registry typecheck does not cover copied snippets. - A targeted snippet typecheck reproduces the public-import failures, e.g.
Module '"@vllnt/ui"' has no exported member 'useControllableState',useEventCallback, anduseDocumentEventListener. - The affected snippets are copied consumer-facing code, so users who install/use these registry examples will get compile errors.
Evidence preserved in the Kanban workspace:
artifacts/pr345-hook-inventory.txt— all 41 changed files inventoried for hook usage.artifacts/validation/registry-snippet-public-import-tsc.txt— targeted snippet typecheck showing the missing public exports.artifacts/validation/registry-tsc-symlink.txt— normal registry tsc passed only becauseregistry/**is excluded.- Required gates attempted:
ui-lint.exit=1,ui-tsc.exit=1,workspace-build.exit=0,workspace-test-once.exit=1. The lint/tsc/test failures appear tied to existing/missing dependency resolution around form/react-hook-form in this symlinked detached worktree, while workspace build passed from cache.
Please either export these new hook helpers from the public @vllnt/ui root, or keep the registry snippets self-contained / avoid importing non-public helpers from @vllnt/ui.
|
|
||
| import { Bot, MessageSquarePlus, X } from "lucide-react"; | ||
|
|
||
| import { useControllableState } from "@vllnt/ui"; |
There was a problem hiding this comment.
This registry snippet now imports useControllableState from the public @vllnt/ui root, but that symbol is not exported by the package root (packages/ui/src/index.ts). The same pattern appears in the other migrated snippets for useEventCallback, useDocumentEventListener, useWindowEventListener, and useUncontrolledState. Because apps/registry/tsconfig.json excludes registry/**, the regular registry typecheck does not catch it; a targeted snippet typecheck fails with TS2305 missing-export errors. Please either export these helpers publicly or keep the registry snippets self-contained so copied examples compile for consumers.
bntvllnt
left a comment
bntvllnt
left a comment
There was a problem hiding this comment.
Review complete at ea54461. I would REQUEST_CHANGES, but GitHub rejects request-changes reviews on my own PR/auth identity. The original registry import blocker is fixed by exporting the helper hooks and the targeted registry typecheck passes, but the MapTimeline playback regression below remains blocking. Also note the app Vercel preview remains failed/canceled under the existing waiver; not treating that as code approval.
| if (next >= endYear) { | ||
| setYear(endYear); | ||
| setIsPlaying(false); | ||
| return; |
There was a problem hiding this comment.
Blocking: this return leaves isPlaying stuck at true after playback reaches endYear. The RAF loop stops because no next frame is scheduled, but MapTimelinePlayButton still renders the pause state (aria-pressed=true, pause label/icon), so users must click once to flip to stopped before they can play again. Please restore the previous setIsPlaying(false) behavior (or otherwise clear playing state) when clamping to endYear.
Keep the registry MapTimeline copy in parity with source behavior by clearing playback when RAF reaches the configured end year.
bntvllnt
left a comment
bntvllnt
left a comment
There was a problem hiding this comment.
REVIEW FALLBACK (would REQUEST_CHANGES, but GitHub rejects request-changes reviews on my own PR/auth identity).
Review — 1 blocking metadata finding
BLOCKING
- C1 — PR body/check evidence is stale for the current head.
- Evidence: current PR head is
767172124bd9c08ebb85285566ff8b98e253dfd1, but the PR body still saysCurrent head: 9a3d134e14486717928be663bcce50674609d949and says checks are re-running for9a3d134e14486717928be663bcce50674609d949. - Rule:
docs/agents/RULES.mdR3 — after every push, rewrite the PR body so validation commands, screenshots/recordings, CI status, and listed components match the current head; stale claims block ship. - Fix: refresh the PR body for
767172124bd9c08ebb85285566ff8b98e253dfd1, including the MapTimeline registry parity push and current check status.
- Evidence: current PR head is
VERIFIED CLEAN
- Reviewed the latest-push delta at
767172124bd9c08ebb85285566ff8b98e253dfd1:apps/registry/registry/default/map-timeline/map-timeline.tsxnow clearsisPlayingwhen RAF playback reachesendYear(setYear(endYear); setIsPlaying(false); return;). - Re-checked source/registry parity for MapTimeline playback behavior:
packages/ui/src/components/map-timeline/map-timeline.tsxand the registry copy now match for theuseTimelineStateandusePlaybackend-year stop behavior. The only full-file diff between source and registry is the expected import path (../../lib/utilsvs@vllnt/ui). - The focused regression test exists in
packages/ui/src/components/map-timeline/map-timeline.test.tsxand asserts that playback returns to the Play state after advancing beyond the end year.
VALIDATION
- Ran on reviewed head
767172124bd9c08ebb85285566ff8b98e253dfd1:git diff --check origin/main...HEAD— passpnpm -F @vllnt/ui exec vitest run src/components/map-timeline/map-timeline.test.tsx— pass, 9 testspnpm -F @vllnt/ui-registry exec tsc --noEmit --project tsconfig.json— pass
- Observed GitHub checks at review time: CodeQL, Build Storybook, Analyze (actions), Verify Stories, Scan codebase health, Analyze (javascript-typescript), and Enforce issue-linked PRs were passing; Storybook Tests, Visual Regression, and Quality Gates were still in progress/pending.
bntvllnt
left a comment
bntvllnt
left a comment
There was a problem hiding this comment.
Review — metadata blocker cleared
BLOCKING
- None.
VERIFIED CLEAN
- Re-fetched PR #345 live and confirmed the head is still
767172124bd9c08ebb85285566ff8b98e253dfd1onfix/271-state-effects. - The PR body no longer cites the stale
9a3d134ecurrent-head/check rerun evidence, the old mixedea54461check evidence, or the canceled Vercel note that caused the prior metadata blocker. - The remaining
ea54461mention is limited to the branch-history summary (Refreshes the branch on top of latest origin/main via merge commit ea54461), not stale CI/head evidence. - Current GitHub checks observed for the head are green: Quality Gates, Analyze (actions), Enforce issue-linked PRs, Verify Stories, Scan codebase health, Analyze (javascript-typescript), Build Storybook, Storybook Tests, Visual Regression, and CodeQL all report success.
- Live merge state is open, non-draft, mergeable, and clean.
VALIDATION
- Re-fetched live PR metadata/body/checks/reviews through GitHub CLI/API.
- Compared the refreshed body against the stale metadata strings from the prior blocker.
- No code, push, merge, approval, close, release, or production mutation was performed.
Verdict: COMMENT only. Approval is recommended from this gate, but final approval remains reserved for the maintainer/bntvllnt.
|
Preview ready · Updated 2026-05-20T16:10:03Z
Inspect
|
…sent timer on unmount
bntvllnt
left a comment
bntvllnt
left a comment
There was a problem hiding this comment.
REVIEW FALLBACK (would REQUEST_CHANGES, but GitHub does not allow request-changes reviews from the PR author account).
Blocking issues found at HEAD edda95c:
-
PR body is stale and violates the repo R3 rule that the PR body must match HEAD.
gh pr viewreports current headedda95c9234aac2c4b668e315c6d74eb79ab6376, but the body still saysCurrent head: 767172124bd9c08ebb85285566ff8b98e253dfd1and presents validation for that older revision. Please refresh the body/validation section for the actual current head. -
Inline findings below:
useControllableStatedoes not preserve theDispatch<SetStateAction<T>>semantics it exposes, and the PR adds an inline//comment in shipped component code.
Remote checks were green at review time. I did not merge or push anything.
| (nextValue) => { | ||
| const resolvedNextValue = | ||
| typeof nextValue === "function" | ||
| ? (nextValue as (currentValue: T) => T)(resolvedValue) |
There was a problem hiding this comment.
This helper returns Dispatch<SetStateAction<T>>, but functional updates are resolved against the render-captured resolvedValue before calling React state. In uncontrolled mode, two batched calls such as setValue(n => n + 1); setValue(n => n + 1); both compute from the same stale value and only increment once, unlike useState/Dispatch semantics promised by the type and docs. This line also introduces an as assertion in shipped TS, which the repo rules prohibit. Please preserve updater semantics by forwarding the updater to React state in the uncontrolled branch (and derive the onChange value from the current state there), or narrow the API so it does not claim Dispatch<SetStateAction<T>>.
|
|
||
| const closeTimerRef = useRef<null | ReturnType<typeof setTimeout>>(null); | ||
|
|
||
| // Clear close timer on unmount to avoid dispatching into an unmounted component. |
There was a problem hiding this comment.
Repo rules prohibit inline // comments in shipped TS/TSX. This PR adds one here (and the generated registry copy repeats it), so please remove it or make the code self-explanatory/TSDoc-only before merging.
| }; | ||
| }, [open]); | ||
|
|
||
| const handleClose = useCallback(() => { |
There was a problem hiding this comment.
Blocking: this new registry example does not dismiss itself in the default uncontrolled usage. handleClose only calls onOpenChange?.(false) after the animation, but open defaults to true and the render guard is still if (!open && !isAnimatingOut) return null. Consumers who install/use the registry component without controlling open will see Accept/Decline/Close animate out and then remain mounted. Please add internal closed state or use the shared controllable-state helper so the default registry path actually closes.
Jarvis review fallback: request changesFormal Blocking
Coverage / validation checked
The rest of the changed hook/state refactors did not surface additional blockers in this pass. |
|
Review result: fallback comment (GitHub rejected a formal APPROVE because this PR is authored by the authenticated account) CookieConsent remediation looks correct at current head 9d1a09b. What I checked:
Validation I ran locally from a detached review worktree:
CI evidence:
No blocking code findings from this review. |
1 participant
Summary
useControllableState,useUncontrolledState,useEventCallback,useDocumentEventListener,useWindowEventListener) so registry examples can import them from@vllnt/ui.origin/mainvia merge commitea54461.isPlayingatendYear, with focused button-state coverage.useControllableStatefunctional-updater regression by preserving latest uncontrolled state across batched updater calls, with focused regression coverage.//comments from CookieConsent source and registry copy.useControllableState; default uncontrolled Accept/Close now remove the dialog after the exit animation while controlledopen+onOpenChangeremains supported.Validation
Current head
9d1a09b6fa1e63a0c1e407aee967b392ebe88f44:git diff --check origin/main...HEADgit diff --checkpnpm -F @vllnt/ui exec vitest run src/components/cookie-consent/cookie-consent.test.tsx(1 file passed, 30 tests passed)pnpm -F @vllnt/ui exec tsc --noEmit --project tsconfig.build.jsonpnpm -F @vllnt/ui lintpnpm buildpnpm test:once(217 test files passed, 1219 tests passed; known jsdom navigation, ShareDialog Description, and InteractiveTimeline duplicate-key warnings still print)Prior validation retained from earlier heads:
pnpm -F @vllnt/ui exec vitest run src/lib/use-uncontrolled-state.test.tsx(1 test passed)pnpm -F @vllnt/ui exec vitest run src/components/map-timeline/map-timeline.test.tsx(9 tests passed)pnpm -F @vllnt/ui-registry exec tsc --noEmit --project tsconfig.jsonnpx --yes react-doctor . --offline --json(targeted react-doctor: state & effects — derived state, cascading setState, effect-event handlers (41 warnings) #271 state/effects rules = 0)npx --yes react-doctor . --diff origin/main --annotations --fail-on error --offline(exits 0; non-target warnings remain from other tracked issue families)packages/ui/src/index.ts.Review / CI
9d1a09b6fa1e63a0c1e407aee967b392ebe88f44.https://vllnt.aias failing; this is outside the repository Actions checks surfaced for the PR and appears unrelated to the code changes.Notes
useEffectEventbecause@vllnt/uipeers React>=18.0.0.401 Unauthorized), so the code mutation was delegated to a Hermes subagent in the dedicated PR worktree and then independently validated before the non-force push.Closes #271