chore: turn on the checks that were inert - #1106
Draft
teallarson wants to merge 8 commits into
Draft
Conversation
Two isolated behavior fixes. Static rendering: the root layout awaited headers() to read "x-pathname" and derive a locale. Awaiting headers() in the root layout opts the entire route tree out of static rendering, so every page — including all 117 toolkit pages, which are pure functions of committed JSON — was server-rendered on demand. The derived locale was always "en": proxy.ts redirects every non-English locale to /en and getPreferredLocale returns "en" unconditionally. The site paid full dynamic rendering to compute a constant. This is not an i18n change. TranslationBanner and the dictionary plumbing stay in place; restoring real i18n means an app/[lang]/ route segment, which is the correct Next pattern regardless. Sitemap: app/sitemap.ts skips any directory whose name contains "[", which is right for directory walking but meant all 117 toolkit pages were absent from sitemap.xml — the largest content section on the site. Merges in listValidIntegrationLinks() from app/_lib/toolkit-static-params.ts, the same enumeration the integrations index uses, and dedupes against the authored partner pages the disk walk already finds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eliminates the duplication that lets these subsystems drift.
Redirects were a 909-line array inside next.config.ts, so three consumers
regex-parsed the config as text — one of them carrying a second "reversed"
regex that existed only because a human might write {destination, source}
instead of {source, destination}, a problem that only exists when you parse
text instead of importing data. The array now lives in a typed redirects.ts
and every consumer imports it. Two further consumers that also parsed the
config as text (scripts/update-internal-links.ts and
tests/integration-index-links.test.ts) would have silently found zero
redirects, so they move to the import too. --auto-fix now appends to the
data file, and its six scattered "Auto-added redirects" marker blocks
collapse to one append point. The resolved array is byte-identical: 156
entries, same order.
check-redirects-utils.ts had 425 lines of tests but was imported by nothing
except its own test file, while the code that actually runs — the
pre-commit hook's scripts/check-redirects.ts — kept private copies of the
same six helpers. The tests guarded a copy while the shipping
implementation was untested. The module moves to scripts/lib/ and the
shipping script now imports it.
Toolkit primitives (data dir, toKebabCase, normalizeToolkitId, the category
list, the *Api heuristic, docsLink→slug) existed in 2-7 copies, one pair
carrying a "must stay in sync" comment. They collapse into
toolkit-docs-generator/src/shared/, which both halves can import. All seven
data-dir consumers now honor TOOLKIT_DATA_DIR; previously only two did.
The Node-only path resolution lives in its own module because client
components reach the primitives through the integrations index, and a
node:* import anywhere in that graph fails the webpack browser build.
The consumer-side contract was a four-field duck-check that never verified
tools was an array, followed by an unchecked cast — while toToolkitSummary
immediately calls data.tools.map(). The generator's Zod schemas are now the
single shared contract, the 522-line hand-written mirror is z.infer, and
zod moves to dependencies because it enters the app's runtime path.
Corruption is now loud and absence stays quiet: three catch blocks treated
missing, unparseable, and schema-invalid identically, so a malformed file
from the nightly PR silently dropped a toolkit and 404'd. Unparseable or
invalid now throws with the file path and the Zod issues, failing the
build. An unrecognized category throws instead of being coerced to
"others", which had no route directory and would have made every toolkit
in a new category a clickable card pointing at a 404.
One cache()-wrapped loader replaces 11 full passes over the data directory
per build, and removes the scan-every-file miss path that a burst of
unknown IDs could otherwise trigger at request time.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds a typecheck script covering ~18k lines that nothing checked: root tests/, and the generator's scripts/ and tests/. scripts/ already had a tsconfig and already passed — it was simply wired to no command. The generator's tsconfig excluded **/*.test.ts, which is what hid most of the errors. Fixing the 50 surfaced errors needed no suppressions and no loosened compiler options: six were .ts import specifiers that tsx tolerates and tsc does not, the rest were incomplete test fixtures and genuine exactOptionalPropertyTypes / noUncheckedIndexedAccess findings. test.yaml now runs it. The generator's vitest config set 80% coverage thresholds and enabled typecheck, but nothing pointed at it — the root config's default glob picked those tests up and ran them under root settings. It could not have worked if it had been wired up: @vitest/coverage-v8 is not installed, so the thresholds could never execute, and running under it breaks four suites for want of an @ alias. Deleting it stops the file implying guarantees it never provided. The six app-lib tests move to root tests/, where the @ alias means they no longer reach through ../../../. The pre-commit hook stashed unstaged work and re-formatted staged files after lint-staged had already formatted them — its own comment conceded the redundancy. That block also swallowed formatter failures by assigning FORMAT_EXIT_CODE=0 unconditionally, and called sha256sum, which stock macOS does not have, so under set -e a contributor without coreutils could not commit at all. 222 lines to 125. The generator fabricated metadata when the design system had none: category "development", an iconUrl, and a docsLink under /en/mcp-servers/, signalled only by a console warning inside an automated PR. One committed record runs on it today. Missing metadata now fails under --require-complete and names the toolkit; without the flag the omission reaches the run log rather than only stdout. The fabricated docsLink prefix now matches the route shape the other 116 records use, and the placeholder is marked hidden so a guessed category cannot file a toolkit under the wrong sidebar section. --require-complete was previously unreachable in --all mode, which is the mode CI uses: a pre-filter added those toolkits to the skip list before the merger ever saw them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
The scheduled workflow uses both flags together; metadata-missing toolkits were excluded from change detection and the no-op path never validated them. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
@cursor review |
…to agent/resolve-pr-1106
…to agent/resolve-pr-1106
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR 4 of 4. Stacked on #1105. Turns on checks that currently exist but do nothing, and removes the last fragility. Deliberately last: T4.1's typecheck would otherwise have surfaced errors in code that PRs 1–3 delete or rewrite.
A typecheck script, covering ~18k previously unchecked lines
Nothing type-checked root
tests/(~5.8k lines) or the generator'sscripts/(~1.3k) andtests/(~11.9k).scripts/tsconfig.jsonalready existed and already passed — it was simply wired to no command. There was notypecheckscript at all.A measurement note worth recording: my first count of the damage said 12 errors. That was wrong — the probe config inherited the generator tsconfig's
exclude: ["**/*.test.ts"], which hid the generator's entire test bucket. The real number was ~50. That same blanket exclude is what had to be removed to makeinclude: ["tests/**/*"]mean anything, and it's the reason this went unnoticed for so long.All ~50 fixed with type-level solutions — zero suppressions (
ts-ignore/ts-expect-error/biome-ignorecount is 24 on this branch and 24 onmain) and no loosened compiler options:TS5097—.tsimport specifiers thattsxtolerates andtscrejects. Changed to.js, matching the NodeNext conventionsrc/already uses. Verified a generator script still runs under tsx afterwards.noUncheckedIndexedAccessand 1 ×exactOptionalPropertyTypes— genuine findings, fixed at the call site.isComingSoon,isHidden,inferrable,documentationChunks, …), partialResponsemocks now typed against real signatures, and aOmit<Partial<X>, "metadata">that was accidentally re-requiring every metadata field.tsconfig.jsongainstests/**; the generator'srootDirmoves from./srcto.soscripts/andtests/can joininclude(nothing consumes the generator'sdist/). One generator test that importsapp/_libvia the@alias is checked under the root config instead.Coverage proven, not assumed. A deliberate type error was introduced into each of the seven areas one at a time and confirmed caught —
app/,lib/,scripts/,tests/,toolkit-docs-generator/{src,scripts,tests}. I independently re-verified three of them. A typecheck script that silently skips a directory is worse than none..github/workflows/test.yamlnow runs it, between lint and build.The generator's vitest config was inert — and could not have worked
toolkit-docs-generator/vitest.config.tsset 80% coverage thresholds on lines/functions/branches/statements plustypecheck: { enabled: true }, but nothing referenced it: nopackage.jsonscript, no CI workflow, no--config. The root config's default glob picked those tests up and ran them under root settings, so the guarantees were decorative.It turns out wiring it up wasn't an option either. Running it directly shows
@vitest/coverage-v8is not an installed dependency — the thresholds could never have executed — and four suites fail under it for want of the@alias that only the root config defines, plus aroot:override that breaks path resolution. Making it real would mean a new devDependency, an alias, and a path fix. Deleting it is the honest choice: it stops implying guarantees that were never true.The six
toolkit-docs-generator/tests/app-lib/files move to roottests/. They testedapp/_lib/*through../../../app/_lib/...precisely because the generator's config has no@alias; they now import via@/. Same collected test count before and after — the full per-test name list diffs to zero, only the six paths changed.The pre-commit hook
222 → 125 lines. The last ~85 stashed unstaged work and ran
ultracite fixper staged file afterlint-stagedhad already formatted the same files — the hook's own comment conceded it. Two defects went with it:FORMAT_EXIT_CODE=0was assigned unconditionally after the format loop, so formatter failures were swallowed and the hook always reported success.sha256sumis GNU coreutils and absent on stock macOS. Underset -ethe command substitution aborts the hook — it worked here only because coreutils happens to be installed. A new macOS contributor could not commit.The merge/rebase detection block went too: it existed solely so the stash block could skip itself. Verified in a scratch repo: formatting still applies on commit, unstaged work survives a commit with staged changes, mid-merge commits succeed, and a deliberately failing formatter now fails the commit instead of passing silently.
The generator no longer fabricates metadata silently
getDefaultMetadatainventedcategory: "development", aniconUrl, and adocsLinkunder/en/mcp-servers/when design-system metadata was missing — signalled only by aconsole.warninside an automated nightly PR.This is live:
cursoragentsapi.jsonruns on it today. It is 1 of 117 records; the other 116 use/en/resources/integrations/. Nothing is broken right now, but the hardcoded category means any future toolkit with missing metadata gets silently filed in the wrong sidebar section under a wrong canonical URL — and PR #1105 removed the"others"catch-all, so a fabricated-but-valid category is now the one bad value that nothing can catch.--require-complete(already passed in CI) and names every affected toolkit.--require-completewas previously unreachable in--allmode — the mode CI actually uses. A pre-filter added missing-metadata toolkits to the skip list before the merger ever saw them. That's why this never fired.toolkitsWithDefaultMetadata=<ids>), not just stdout.docsLinknow matches the real route shape, and the placeholder is markedisHidden: trueso a guessed category can't route anything.Verified with the mock source (no Engine or Anthropic credentials needed), writing to a scratch directory: with the flag, exit 1 naming
Orphan; without it, exit 0 with the omission inruns.log.Adjacent gap left alone:
--providersmode has the same silent-skip pre-filter. CI doesn't use it.Verification
pnpm typecheckpnpm lintpnpm testpnpm build🤖 Generated with Claude Code