fix(react-doctor): clear misc architecture warnings#346
Conversation
bntvllnt
added
react-doctor
bntvllnt
left a comment
bntvllnt
left a comment
There was a problem hiding this comment.
Review — request changes recommended
I reviewed PR #346 at head 5fd2fc36968c4bf750fb8a615b10ea88f0b8e7e5 against origin/main.
BLOCKING
-
C1 — ContentIntro public API was renamed instead of preserved- Evidence:
ContentIntroPropsnow requiresintroContentand renders{introContent};renderIntroContentis gone from source, docs, story, and registry copy. - Why it matters: this is a published component API. Existing consumers using
renderIntroContentwill fail type-checking after upgrade even though this PR is scoped as react-doctor warning cleanup. - Fix: keep
renderIntroContentas a deprecated fallback, or support both props while migrating examples/docs.
- Evidence:
-
C2 — Slideshow public render prop was replaced with a component prop- Evidence:
SlideshowPropsnow requiresSectionContent;renderContentis gone from source, docs, story, and registry copy. - Why it matters: this breaks existing consumers that pass
renderContent={(section) => ...}. The repo rules require legacy prop behavior not to change unintentionally. - Fix: keep
renderContentcompatibility or support bothrenderContentandSectionContentwith a clear migration path.
- Evidence:
-
C3 — code component children contracts were narrowed- Evidence:
CodeBlockProps.childrenandCodePlaygroundProps.childrenchanged fromReactNodetostring/string?;CodeBlockalso removedextractTextFromChildren. - Why it matters: MDX/JSX/numeric/array children that were previously accepted by the public types, and in
CodeBlockpreviously converted to text, now break or lose behavior. - Fix: preserve the old
ReactNodecontract/extraction behavior, or add a new explicitcodeprop without removing existing children compatibility.
- Evidence:
VERIFIED CLEAN
- PR head/worktree matched:
5fd2fc36968c4bf750fb8a615b10ea88f0b8e7e5. - All 30 changed files were covered through direct inspection and focused subreview; source and generated registry copies are internally in parity for the new names.
- Remote GitHub checks were green when captured: Storybook, Quality Gates, issue-linked PR gate, codebase health scan, and Vercel previews.
VALIDATION
- Ran locally:
git diff --check origin/main...HEAD— clean. - Ran targeted grep/evidence capture for renamed public props and source/registry parity.
- Did not rerun full
pnpmgates locally; relied on captured green remote checks plus diff/evidence inspection.
Note: the authenticated reviewer is also the PR author, so GitHub may reject a formal REQUEST_CHANGES review. If that happens, treat this same payload as the authoritative fallback review for the current head.
| /** Estimated time to complete */ | ||
| estimatedTime: string; | ||
| /** Rendered introduction content */ | ||
| introContent: ReactNode; |
There was a problem hiding this comment.
Blocking: this renames the published ContentIntro API from renderIntroContent to required introContent instead of preserving compatibility. Existing consumers using the old render prop will fail type-checking on upgrade. Please keep renderIntroContent as a deprecated fallback or support both props while migrating docs/examples.
| /** Render function for section content */ | ||
| renderContent: (section: SlideshowSection) => ReactNode; | ||
| /** Component used to render the current section content */ | ||
| SectionContent: ComponentType<SlideshowSectionContentProps>; |
There was a problem hiding this comment.
Blocking: this replaces the published renderContent render prop with required SectionContent. That breaks existing Slideshow consumers even though the PR is scoped as react-doctor warning cleanup. Please preserve renderContent compatibility or support both APIs with a migration path.
|
|
||
| type CodeBlockProps = { | ||
| children: ReactNode; | ||
| children?: string; |
There was a problem hiding this comment.
Blocking: CodeBlock used to accept ReactNode children and extract text from strings, numbers, arrays, and nested elements. Narrowing this to string and rendering children ?? "" breaks existing MDX/JSX-shaped children. Please preserve the ReactNode contract/extraction behavior or add a separate explicit code prop without removing the old behavior.
|
|
||
| export type CodePlaygroundProps = { | ||
| children: ReactNode; | ||
| children?: string; |
There was a problem hiding this comment.
Blocking: this narrows the public CodePlaygroundProps.children type from ReactNode to string?. Even though the runtime only displayed string children before, this is still a public type break for existing consumers. Please preserve the old type contract or introduce a compatible migration path.
|
|
||
| import type { ReactNode } from "react"; | ||
|
|
||
| import { useDocumentEventListener } from "@vllnt/ui"; |
There was a problem hiding this comment.
Blocking: this registry copy now imports useDocumentEventListener from @vllnt/ui, but the registry item still declares @vllnt/ui@^0.2.1 and packages/ui/package.json is still 0.2.1. The currently published @vllnt/ui@0.2.1 does not export this hook, so shadcn add .../content-intro.json can install a component that immediately fails to compile for consumers until a future package version exists and is resolved. Keep this helper local in the registry copy, or bump/publish and point the registry dependency at a version that actually contains the export.
| import type { ComponentType, ReactNode } from "react"; | ||
| import { createPortal } from "react-dom"; | ||
|
|
||
| import { useDocumentEventListener } from "@vllnt/ui"; |
There was a problem hiding this comment.
Same registry compatibility blocker here: the copied Slideshow component imports the new useDocumentEventListener export from @vllnt/ui, while registry.json still advertises @vllnt/ui@^0.2.1; published 0.2.1 has no such export. This replaces the fixed render-prop API break with a registry-install break. Make the registry copy self-contained for the hook or update the dependency/versioning so the installed package definitely exports it.
| @@ -33,9 +33,9 @@ const sections = [ | |||
|
|
|||
| <ContentIntro | |||
There was a problem hiding this comment.
Blocking docs/API issue: the public usage snippet omits required completedSections, even though the API table below correctly marks it required. This gives consumers a copy-paste example that does not satisfy ContentIntroProps. Add completedSections={new Set()} (or equivalent state) so the docs match the current public contract.
| return <p>{section.title}</p> | ||
| } | ||
|
|
||
| <Slideshow |
There was a problem hiding this comment.
Blocking docs/API issue: this public usage snippet omits required completedSections and passes currentIndex="0" as a string while the prop is typed as number. Since this PR is specifically about public API compatibility and docs/MDX freshness, the example needs to be copy-paste valid against SlideshowProps (completedSections={new Set()}, currentIndex={0}, etc.).
|
Formal Review — 4 blocking findingsBLOCKING
WARN
VERIFIED CLEAN
VALIDATION
|
bntvllnt
left a comment
bntvllnt
left a comment
There was a problem hiding this comment.
Review — 0 findings (current head)
BLOCKING
- None found in the remediation delta.
WARN
W1 — full registry lint remains blocked by unrelated tooling debt- Evidence:
pnpm -F @vllnt/ui-registry lintstill exits 2 before the touched registry copies, atapps/registry/app/report/report-bug-form.tsx:73, witheslint-plugin-jsx-a11y/minimatchruntimeTypeError: (0 , _minimatch.default) is not a function. - Why it matters: this is still a validation gap for the registry app as a whole, but it is not evidence of a regression in this remediation because the targeted registry typecheck/lint and all current GitHub checks are green.
- Evidence:
VERIFIED CLEAN
- Reviewed current head
e84c03866daf9f43df9aa9866814926098bd37bdagainst previous reviewed head15adb082e80f24d56991cede602b4af61cef88f9; the new remediation delta is limited to the two registry copies plus the two MDX docs. apps/registry/registry/default/content-intro/content-intro.tsxno longer importsuseDocumentEventListenerfrom@vllnt/ui; it defines the document listener helper locally and imports onlycn/Buttonfrom@vllnt/ui.apps/registry/registry/default/slideshow/slideshow.tsxno longer importsuseDocumentEventListeneroruseMountedfrom@vllnt/ui; both helpers are local and the only@vllnt/uiimports arecn/CompletionDialog.- Public MDX examples now include the required props:
ContentIntroincludescompletedSections={new Set()}, andSlideshowincludes bothcompletedSections={new Set()}andcurrentIndex={0}. - PR metadata is current enough for this head: body calls out the registry export-contract remediation, the MDX required-prop fix, validation evidence, and
Closes #277. - Current GitHub checks are all passing: Quality Gates, PR Issue Link, Verify Stories, Scan codebase health, CodeQL, Build Storybook, Storybook Tests, and Visual Regression.
VALIDATION
- Ran:
git diff --check— PASS. - Ran:
pnpm -F @vllnt/ui lint— PASS. - Ran:
pnpm -F @vllnt/ui exec tsc --noEmit --project tsconfig.build.json— PASS. - Ran:
pnpm -F @vllnt/ui-registry exec tsc --noEmit --project tsconfig.json— PASS. - Ran:
pnpm -F @vllnt/ui-registry exec eslint registry/default/content-intro/content-intro.tsx registry/default/slideshow/slideshow.tsx— PASS with package-config ignore warnings only. - Ran:
npm pack @vllnt/ui@0.2.1plus export-file probe — PASS for the relevant contract: root exportscnand re-exports components includingButton/CompletionDialog; no root export foruseDocumentEventListener/useMountedwas found. - Ran:
pnpm -F @vllnt/ui-registry lint— blocked by the unrelated existingeslint-plugin-jsx-a11y/minimatchruntime error noted above.
Manual approval is recommended, but final approval remains reserved for bntvllnt.
|
Preview ready · Updated 2026-05-20T16:10:16Z
Inspect
|
…rect
React 17+ registers synthetic onWheel handlers as passive, making
preventDefault() a no-op and emitting a console warning. Restore the
native addEventListener({ passive: false }) pattern using a ref on the
scroll container so preventDefault() actually cancels the scroll event.
Also add a TSDoc note to useDocumentEventListener warning callers that
the options parameter must be a stable reference to avoid re-registering
the listener on every render.
|
|
||
| import type { ReactNode } from "react"; | ||
|
|
||
| import { useDocumentEventListener } from "@vllnt/ui"; |
There was a problem hiding this comment.
This registry copy now imports useDocumentEventListener from @vllnt/ui, but the registry entry still advertises @vllnt/ui@^0.2.1; I verified the currently published 0.2.1 package does not expose this root export. Consumers installing this registry item can therefore get a copied component that fails to compile/import. Please either keep the hook local to the registry copy or bump/publish/pin the dependency to a version that actually contains the export.
Review — REQUEST_CHANGESI reviewed PR #346 at current head BLOCKING
WARN
VERIFIED CLEAN
Because GitHub may reject a formal |
bntvllnt
left a comment
bntvllnt
left a comment
There was a problem hiding this comment.
Review — completion-dialog registry remediation at 700c836
BLOCKING
- None in the bounded completion-dialog remediation. The original registry installability blocker is resolved.
WARN
W1 — vllnt-pilot status artifact still leaves the PR UNSTABLE- Evidence: the current head has one nameless
vllnt-pilotcheck run failing withBuildKit reported error: exec: "buildctl": executable file not found in $PATH(https://github.com/vllnt/ui/runs/76871625654), while the later namedvllnt-pilot / preview deploycheck succeeded and producedhttps://pr-346-ui-registry.preview.vllnt.ai. - Interpretation: this is an infrastructure/deployer check artifact, not a completion-dialog code regression, but it is still visible in GitHub's status rollup and keeps
mergeStateStatusatUNSTABLEuntil rerun/cleared/routed.
- Evidence: the current head has one nameless
VERIFIED CLEAN
apps/registry/registry/default/completion-dialog/completion-dialog.tsxno longer importsuseDocumentEventListenerfrom@vllnt/ui; it imports only published primitivescnandButtonfrom@vllnt/uiand defines the small document event helper locally.apps/registry/registry.jsonstill advertisescompletion-dialogwithdependencies: ["@vllnt/ui@^0.2.1"]and noregistryDependencies, matching the intended install contract.npm pack @vllnt/ui@0.2.1confirmsButton,cn, andCompletionDialogare exported, whileuseDocumentEventListeneranduseMountedare not exported. The registry copy no longer depends on those unpublished hooks.- The PR body now names current head
700c83645da13fb44731643f0309f4ff6ff61d2d, states the hook-inlining remediation, includes the current registry typecheck/diff-check evidence, and accurately calls out the known@vllnt/ui-registry lintblocker.
VALIDATION
- Reviewed live PR #346 at head
700c83645da13fb44731643f0309f4ff6ff61d2d. - Ran
git diff --check origin/main...HEAD— passed. - Ran
pnpm -F @vllnt/ui-registry exec tsc --noEmit --project tsconfig.json— passed. - Ran
pnpm -F @vllnt/ui-registry lint— reproduced the PR-body-described existingeslint-plugin-jsx-a11y/minimatchruntime error before reaching the touched registry copy. - Re-fetched GitHub checks: all named Actions/deploy checks are passing; only the nameless
vllnt-pilotcheck run is failing as described above.
Verdict: no code changes requested for the completion-dialog remediation. Manual approval/merge should wait for the visible nameless vllnt-pilot failure to be cleared or explicitly accepted as an infra artifact.
1 participant
Summary
renderIntroContentandrenderContentprops still work, whileintroContent/SectionContentremain the preferred non-render-prop APIs.ReactNodechildren compatibility forCodeBlockandCodePlaygroundwith safe text extraction, plus additivecodeprops for explicit string input.content-intro,slideshow, andcompletion-dialogregistry copies no longer import unpublished event/mount hooks from@vllnt/ui@^0.2.1; those registry copies inline the small local hook helpers while continuing to import published primitives such ascn,Button, and component exports.700c83645da13fb44731643f0309f4ff6ff61d2d.Validation
git diff --check— passed at current head before push.pnpm -F @vllnt/ui-registry exec tsc --noEmit --project tsconfig.json— passed at current head before push.apps/registry/registry/default/completion-dialog/completion-dialog.tsxno longer importsuseDocumentEventListenerfrom@vllnt/ui.registry/default/completion-dialog/completion-dialog.tsx— passed.pnpm -F @vllnt/ui-registry lint— still blocked before reaching the touched registry copy by the existingeslint-plugin-jsx-a11y/minimatchruntime error inapps/registry/app/report/report-bug-form.tsx:73.npm pack @vllnt/ui@0.2.1showedcn,Button, andCompletionDialogare present, whileuseDocumentEventListeneranduseMountedare absent; the registry copies therefore inline those hooks instead of importing them from the advertised dependency.pnpm -F @vllnt/ui exec vitest run src/components/content-intro/content-intro.test.tsx src/components/slideshow/slideshow.test.tsx src/components/code-block/code-block.test.tsx src/components/code-playground/code-playground.test.tsx(4 files, 14 tests),pnpm -F @vllnt/ui lint,pnpm -F @vllnt/ui exec tsc --noEmit --project tsconfig.build.json,pnpm build,pnpm test:once(218 files, 1222 tests),pnpm dlx react-doctor . --offline --json,pnpm dlx react-doctor . --diff origin/main --annotations --fail-on error --offline, andgit diff --stat origin/main...HEAD.Closes #277