perf: reduce react-doctor performance warnings#340
Conversation
bntvllnt
added
react-doctor
# Conflicts: # packages/ui/src/components/conversation-thread/conversation-thread.tsx
|
|
||
| function sortEventsDesc(events: LiveFeedEvent[]): LiveFeedEvent[] { | ||
| return [...events].sort( | ||
| return events.toSorted( |
There was a problem hiding this comment.
This introduces Array.prototype.toSorted(), but @vllnt/ui is still built/published with an ES2020 target (packages/ui/tsup.config.ts and tsconfig.build.json). I verified the built dist preserves the .toSorted() calls, so consumers on ES2020-era runtimes/browsers will throw TypeError before these components render. Please replace this with a compatible copy+sort pattern such as [...events].sort(...) (same issue also appears in search-dialog and usage-breakdown), or explicitly raise/transpile the package runtime target.
| const [open, setOpen] = useState(false); | ||
|
|
||
| const sortedItems = [...items].sort((a, b) => a.title.localeCompare(b.title)); | ||
| const sortedItems = items.toSorted((a, b) => a.title.localeCompare(b.title)); |
There was a problem hiding this comment.
Same runtime-compatibility issue here: items.toSorted(...) is preserved in the published ES2020-targeted package output. Please use [...items].sort(...) or otherwise make the package runtime target/polyfill story explicit.
| const rankedItems = [...items].sort( | ||
| (left, right) => right.value - left.value, | ||
| ); | ||
| const rankedItems = items.toSorted((left, right) => right.value - left.value); |
There was a problem hiding this comment.
Same blocker here: items.toSorted(...) is ES2023 and is preserved in the built dist despite the package target being ES2020. Please switch this to a compatible copy+sort implementation or update the runtime contract.
|
Hermes review fallback for PR #340 GitHub would not allow this account to submit a REQUEST_CHANGES review because it reports this as the caller's own pull request, so I published the blocking findings as file-level review comments and am leaving this PR comment as the review-state fallback. Verdict: changes requested / blocked until fixed. Blocking issue:
These changed lines introduce Array.prototype.toSorted() into @vllnt/ui public component runtime code. The package is still built/published with an ES2020 target (packages/ui/tsup.config.ts and tsconfig.build.json), and I verified the built dist preserves .toSorted() calls. Consumers on ES2020-era runtimes/browsers can therefore throw TypeError before these components render. Please switch these back to a compatible copy+sort pattern (for example, [...items].sort(...)) or explicitly update/transpile the package runtime contract. Validation evidence from /home/ubuntu/ui/.worktrees/pr-340-review:
Coverage: reviewed all 59 changed files and all changed hunks. Machine-readable coverage ledger is stored in the Kanban workspace artifact pr340-review-artifact.json. |
Review — 0 code findings (0 blocking, 1 operational warning)BLOCKING
WARN
VERIFIED CLEAN
VALIDATION
Approval is recommended from the code-review side after the canceled Vercel status is resolved or explicitly waived. I am not casting APPROVE autonomously; final approval remains with bntvllnt. Note: because the authenticated GitHub account is also the PR author, this is posted as a PR comment rather than a formal GitHub review artifact. |
bntvllnt
left a comment
bntvllnt
left a comment
There was a problem hiding this comment.
@bntvllnt Review verdict: REQUEST_CHANGES recommended — one blocking regression
Head reviewed: 4e7a6d9518f37b90486089df7406ca0a6a59d871
BLOCKING
apps/registry/components/storybook-embed/storybook-embed.tsx: manual Storybook preview theme selection is no longer sticky. The mutation/media-query listeners are installed once andupdateTheme()still callssetPreviewTheme(...)even afterhasManualThemeSelectionRef.current = true, so later document theme / prefers-color-scheme changes can overwrite the user's manual preview selection.
PROCESS / STATUS
- GitHub Actions/code checks are green and
Vercel – storybookis SUCCESS. Vercel – ui.vllnt.ai: FAILUREis eligible for the PM stale-Vercel reviewer-triage waiver only; it is not merge/release approval.- Validation gap: I did not rerun full local workspace gates; this review used the live GitHub status rollup plus changed-file inspection.
| @@ -180,7 +179,7 @@ export function StorybookEmbed({ | |||
| observer.disconnect(); | |||
There was a problem hiding this comment.
Blocking: with the effect dependency changed to [], the observer/media-query listeners remain active after a user manually selects a preview theme. Later site/theme changes can still call setPreviewTheme(...) and overwrite the manual preview choice; the previous state dependency cleaned this up.
|
Preview build failed Build failed: build failed: exec: "buildctl": executable file not found in $PATH InspectCheck deployer logs: kubectl logs -n default deploy/deployer |
|
Hermes review fallback for PR #340 at d8ad904 GitHub would not allow this account to submit a REQUEST_CHANGES review, so I am posting the blocking review as a PR comment. Verdict: changes requested / blocked until fixed. Review — 1 finding (1 blocking, 0 warn)BLOCKING
VERIFIED CLEAN
VALIDATION
Verdict: changes requested / blocked on C1. The older |
bntvllnt
left a comment
bntvllnt
left a comment
There was a problem hiding this comment.
Review — 1 blocking finding
BLOCKING
C1 — registry changed-file lint is red- Evidence: current head
b06cda963178aacfd0df38bf4b6a6f743ed03dc2; targeted registry lint over the PR's changed registry.ts/.tsxfiles exits non-zero with errors in changed files. Concrete examples:apps/registry/app/components/[slug]/page.tsx:48introduces the repo-bannedparamsabbreviation,apps/registry/lib/sidebar-sections.ts:5leavesRegistryComponentunused and the newsections.pushblock is not Prettier-formatted, and the new loops inapps/registry/lib/stats.ts:60/:65violate the registry lint rule set. - Why it matters: the PR is explicitly about react-doctor/perf cleanup, but it leaves the touched registry source failing the repository's own lint/rule contract. That makes the branch not review-ready even though GitHub's CI checks are green.
- Fix: run the registry formatter/lint autofix path, then manually clean the remaining rule violations in changed registry files. In practice this means renaming
params/docPages, removing the unused import, fixing the indentation, and replacing newly introducedforloops where this package's functional lint rules require reduce/map.
- Evidence: current head
WARN
- None.
VERIFIED CLEAN
- Reviewed all 52 changed files in the PR diff.
- The previous Storybook theme override blocker is fixed: the observer callback now re-checks
hasManualThemeSelectionRef.current, so DOM/class mutations after a manual selection no longer overwrite the user's selected preview theme. - No remaining
.toSorted()usage was found in the touched registry/app/component paths. packages/uichanged-file lint passed locally.
VALIDATION
- GitHub checks: 13 pass / 1 neutral from preserved
gh pr checks --jsonartifact, including Quality Gates, react-doctor, Storybook tests, visual regression, builds, CodeQL, and preview deploy. - Local:
git diff --check origin/main...HEADpassed. - Local: searched touched paths for
.toSorted()/toSorted(— no matches. - Local:
pnpm -F @vllnt/ui exec eslint <changed packages/ui files>passed. - Local:
pnpm -F @vllnt/ui-registry exec eslint <changed apps/registry files>failed with changed-file lint errors; seechanged-files-eslint.txt/registry-targeted-eslint.txtin the Kanban artifact directory.
Verdict: REQUEST_CHANGES recommended. GitHub will not let this token formally request changes on its own PR, so this COMMENT review is the authoritative blocking handoff for this head.
| .map((item) => ({ | ||
| slug: item.name, | ||
| })); | ||
| return registry.items.reduce<{ slug: string }[]>((params, item) => { |
There was a problem hiding this comment.
Blocking: this changed line introduces params, which violates the repo abbreviation rule (unicorn/prevent-abbreviations). Rename it to parameters (or a more specific accumulator name) and rerun the registry lint/format gate before re-requesting review.
| label: string; | ||
| }[] = []; | ||
|
|
||
| for (const category of categoryOrder) { |
There was a problem hiding this comment.
Blocking: the changed registry files are not lint-clean. This file now leaves RegistryComponent unused, introduces a for loop rejected by the package lint rules, and the new sections.push object is not Prettier-formatted. Please run/fix the registry lint path across the whole changed-file set, not only this hunk.
| .map((slug) => REGISTRY.items.find((item) => item.name === slug)) | ||
| .filter((item): item is RegistryItem => item !== undefined); | ||
| const registryByName = new Map<string, RegistryItem>(); | ||
| for (const item of REGISTRY.items) { |
There was a problem hiding this comment.
Blocking: this new loop is one of the changed-file lint failures (functional/no-loop-statements). The PR should keep the touched registry files lint-clean; use the package-preferred reduce/map shape here and at the second new loop below.
|
PR #340 lint remediation handback Previous head: b06cda9 Changed files:
What changed:
Validation run locally from
Notes / remaining risks:
Ready for reviewer re-check. |
|
Hermes review fallback for PR #340 at 8ebb2c5 GitHub normally rejects formal Review — REQUEST_CHANGES recommended (2 blocking findings)BLOCKING
WARN
VERIFIED CLEAN
VALIDATION
|
1 participant
Summary
Array.prototype.toSortedin the public package component, default registry shim, and registry app llms route surfaces with ES2020-safe copy-and-sort patterns so sorting stays immutable without requiring ES2023 consumers.Manual-theme remediation
apps/registry/components/storybook-embed/storybook-embed.tsx:updateTheme()now checkshasManualThemeSelectionRef.currentbefore applying observer/media-query theme changes.Validation
pnpm -F @vllnt/ui lint-> passespnpm -F @vllnt/ui exec tsc --noEmit --project tsconfig.build.json-> passespnpm -F @vllnt/ui-registry exec eslint components/storybook-embed/storybook-embed.tsx-> passespnpm -F @vllnt/ui-registry exec tsc --noEmit --project tsconfig.json-> passesgit diff --check-> passespnpm build-> passespnpm test:once-> passes (216 files / 1215 tests; existing stderr warnings only)pnpm doctor:json-> previously passed on this PR before the one-line StorybookEmbed remediation; not rerun for this updaterg -n "toSorted\\(|\\.toSorted" packages/ui/src/components apps/registry/registry/default apps/registry/app-> previously no matches on this PR; not rerun for this updateCI status
b06cda963178aacfd0df38bf4b6a6f743ed03dc2Closes #270