Fix three pre-existing breakages in the example apps and task graph - #166
Fix three pre-existing breakages in the example apps and task graph#166KayleeWilliams wants to merge 4 commits into
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 46 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Astro example adds ChangesAstro native parser and validation updates
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
ℹ️ Both fixes are sound — a few rough edges inline, none blocking.
Reviewed changes — full review of 888dab2: the Astro native-binding workaround and the .gitignore generalization.
- Astro build fix — a local Vite plugin (
astro.config.mjs6-33, registered at 36-38) markssatteriand@bruits/satteri-*external from aresolveIdhook, keeping leadtype's napi-rs MDX parser out of the prerender chunk. satterideclared on the app — added toapps/astro-example/package.jsondependenciesat^0.9.3, so the externalized import resolves fromdist/under bun's isolated layout..gitignoregeneralization — the fiveapps/sveltekit-example/static/*entries become elevenapps/*/static/*entries mirroring theapps/*/public/*list, plus a comment on why two lists exist.bun.lock— records the new dependency and incidentally correctspackages/leadtype0.3.1 → 0.4.2.
I verified the .gitignore half end-to-end and it is correct and complete. Out roots are public for astro/fumadocs/next/nuxt and static for sveltekit only (c15t and tanstack ignore public/ wholesale locally), so apps/*/public/ + apps/*/static/ is the right pair. Every out-root artifact is now covered under both roots — feeds/schema.jsonl and schema-map.xml (src/nlweb/artifacts.ts:8-9), root mcp.json (src/mcp/card.ts:228), .well-known/, both llms*.txt, robots, both sitemaps — with search-index.json / agent-readability.json covered by docs/ and the configured /changelog/*.xml feeds covered by changelog/. Nothing tracked sits under the broadened glob (apps/astro-example/public/styles.css is the only tracked file there, matched by no pattern).
I also confirmed the plugin's blast radius is safe even though it applies to every Vite environment: the client graph cannot reach satteri, since the app's only client-side leadtype imports are leadtype/webmcp and leadtype/search/client, whose Rollup entries transitively reach none of the four satteri importers.
ℹ️ Nothing builds the example apps in CI, so both of these will regress silently
ci.yml builds, lints, typechecks and tests packages/leadtype only — no job runs astro build or any other example build. That is precisely why both of these breakages sat in main unnoticed until someone ran bun run build by hand, and why nothing stops either from coming back. The .gitignore half is self-healing once merged, but the Astro plugin is a load-bearing workaround with no test pinning it.
Technical details
# Example app builds are not covered by CI
## Affected sites
- `.github/workflows/ci.yml:32-53` — the `validate` job runs `bun run --filter leadtype build`, `lint`, `check-types`, `test`. Every step is scoped to `packages/leadtype`; no example app is ever built.
- `apps/astro-example/astro.config.mjs:24-38` (new) — the workaround this PR adds has no regression guard. If a future refactor drops the plugin, or `leadtype`'s import graph changes so `satteri` arrives by another path, CI stays green.
## Required outcome
- A merge-blocking signal that fails when an example app can no longer build, covering at minimum the Astro example that this PR just repaired.
## Suggested approach (optional)
- Add a job (or matrix leg) that runs `bun run --filter astro-example build` after the `leadtype` build + `Link CLI` steps. Note the example's `build` script already chains `bun run --filter leadtype build && bun run docs:generate`, so it is close to self-contained.
- A matrix over the example apps is the fuller version, but it is meaningfully slower — one Astro leg buys most of the protection for this specific PR.
## Open questions for the human
- Is leaving the examples out of CI a deliberate cost/runtime tradeoff? If so, this is fine to close — but then the Astro workaround is worth a comment noting it is unguarded.
- Would you rather this land as a follow-up issue than expand the scope of this PR?ℹ️ Nitpicks
bun.lock:228also correctspackages/leadtype0.3.1 → 0.4.2, drift left behind by release commitb5857b4rather than anything in this PR. Harmless to carry here, but worth knowing it rode along givenci.yml:30andrelease.yml:55both runbun install --frozen-lockfile.apps/{next,nuxt,sveltekit}-example/test-results/.last-run.jsonare tracked in git — Playwright output committed in exactly the way this PR's.gitignorecomment warns about.test-results/is ignored only inapps/tanstack/.gitignore:6, and those three apps have no local.gitignore. Pre-existing and out of scope, but adjacent enough to mention; note it needsgit rm --cachedas well as an ignore entry, so it is not a one-line fix.
Claude Opus | 𝕏
There was a problem hiding this comment.
Important
The check-types half of the race is fixed correctly, but the identical rm -rf dist race is still live in turbo run build.
Reviewed changes — delta since the prior review of 888dab2; the new commit 81adabb is the task-graph fix.
- Moved the leadtype build into turbo's task graph —
check-typesbecomesdependsOn: ["^build", "^check-types"](turbo.json:16-18), sopackages/leadtype/distis produced once ahead of every dependent. - Dropped the per-app self-build — the
bun run --filter leadtype build &&prefix comes out of all seven apps'check-typesscripts. - Refreshed a tracked generated artifact —
apps/next-example/app/generated/docs-mdx-map.tsis re-sorted and gains nine doc routes (mdxSlugs41 → 50), which also widensgenerateStaticParams.
I confirmed the diagnosis rather than taking it from the description: packages/leadtype/rollup.config.ts:66-71 has a cleanDist plugin doing rm("dist", { recursive: true, force: true }) in buildStart, and apps read dist by literal path — apps/next-example's docs:generate runs bun ../../packages/leadtype/dist/cli.js — so concurrent self-builds really do delete dist out from under a sibling mid-read.
The turbo semantics are right. ^build and ^check-types reference only upstream workspace dependencies, so packages/leadtype's own check-types node gets an empty prerequisite set — no self-build, no cycle. Nothing outside turbo invokes an app's check-types either: ci.yml:50 runs bun run --filter leadtype check-types only, .husky/pre-commit runs ultracite fix plus bun test, and root package.json:21 (turbo run check-types) is the sole in-repo consumer. Direct bun run --filter <app> check-types on a clean checkout does now fail, but that is the intended consequence of moving ordering into turbo, not a regression to undo — re-adding the prefix would reintroduce the race.
The docs-mdx-map.ts refresh is genuine generator output, not a hand edit: apps/next-example/scripts/build-mdx-map.mjs sorts with .sort(), the new file is sorted and the old one was not.
⚠️ turbo run build still lets seven parallel self-builds rm -rf the same dist
check-types is fixed; build is untouched. turbo.json:5-9 already had dependsOn: ["^build"], and all seven apps/*/package.json build scripts still begin with bun run --filter leadtype build. Turbo dedupes its own packages/leadtype#build node, not a raw shell command sitting inside a sibling task's script body — so after leadtype builds once, seven concurrent apps/*#build tasks each re-invoke rollup, each hitting cleanDist while a sibling is reading dist/cli.js. That is the same failure this PR exists to fix, in the task where a wiped dist is most likely to be observed.
Technical details
# The `dist` race is fixed for `check-types` but left live for `build`, `dev`, `mcp` and `test:e2e`
## Affected sites
- `turbo.json:5-9` — the `build` task (unmodified) is `dependsOn: ["^build"]`, exactly the shape that made the `check-types` prefix redundant. Nothing was removed from the app scripts to match.
- `apps/astro-example/package.json:9`, `apps/c15t-example/package.json:9`, `apps/fumadocs-example/package.json:9`, `apps/next-example/package.json:10`, `apps/nuxt-example/package.json:9`, `apps/sveltekit-example/package.json:9`, `apps/tanstack/package.json:8` — every `build` script still opens with `bun run --filter leadtype build &&`. None of these lines are in the diff.
- `apps/*/package.json` `dev` (all seven) plus `apps/tanstack/package.json:11` (`mcp`) and `:13` (`test:e2e`) carry the same prefix. `dev` is lower risk in practice because root `package.json:18` filters it to `tanstack` alone.
- `packages/leadtype/rollup.config.ts:66-71` — `cleanDist` runs `rm("dist", { recursive: true, force: true })` in `buildStart`, so each redundant invocation opens a window where `dist` does not exist.
- `packages/leadtype/package.json` `build` is `rollup -c ... && bun run --silent docs:generate` — a plain bun subprocess when reached via `--filter`, invisible to turbo's graph and cache, so there is no dedup or locking.
## Required outcome
- `turbo run build` must not be able to delete `packages/leadtype/dist` while a sibling app task is reading it — the same guarantee `81adabb` establishes for `check-types`.
- Whatever is decided, the `turbo.json` comment should describe the state the repo is actually in.
## Suggested approach (optional)
- Drop the `bun run --filter leadtype build &&` prefix from the seven `build` scripts and rely on the existing `^build`, mirroring what this commit did for `check-types`. `dev`, `mcp` and `test:e2e` are the same one-line change if you want them consistent.
## Open questions for the human
- Is the prefix kept in `build` deliberately, so `bun run --filter <app> build` stays self-sufficient for someone who bypasses turbo? That goal and race-freedom are in direct conflict, and `check-types` has now picked the other side — worth making the split explicit rather than incidental.
- No CI job runs `turbo run build` or `turbo run check-types`, so neither the fix nor the remaining hazard is exercised anywhere. Does that change how much you want to invest here versus in the CI gap?ℹ️ Nitpicks
apps/c15t-example/README.md:25-26still says "Thedev,build, andcheck-typesscripts all rebuild the localleadtypepackage", which is no longer true forcheck-types. It is the only README in the repo making that claim.apps/next-example/app/generated/docs-mdx-map.tsis stampedGenerated by scripts/build-mdx-map.mjs — do not editand is rewritten bydocs:preparebefore everybuildandcheck-types, yet it is tracked — so it re-dirties the working tree on every run, which is how it came to ride along in a commit about the task graph. Same hygiene family as this PR's.gitignorechange, and worth deciding whether it should be ignored instead. The refresh itself is a real improvement: nine doc routes were missing fromgenerateStaticParamsonmain.
Claude Opus | 𝕏
There was a problem hiding this comment.
Important
Three of the four comment corrections are right. The fourth replaces an imprecise claim with a false one — and the false claim is the reason the remaining dist race won't get fixed.
Reviewed changes — delta since the prior review of 81adabb; the new commit 39223d4 rewrites four comments and deletes one dead code path. Only astro.config.mjs and turbo.json changed.
- Corrected the Astro plugin's causation comment — the inaccurate
noExternalattribution is gone, replaced by "monorepo-specific:leadtypeis a linked workspace package, so it is bundled rather than externalized", and the record thatresolve.externalplus Astro 6'senvironments.ssr/environments.prerenderequivalents were both tried is retained. - Deleted the unreachable
@bruits/satteri-resolver arm — the hook is nowif (id === NATIVE_PARSER)andNATIVE_PARSER_BINDINGis removed (astro.config.mjs:38). - Documented the
satteriversion coupling — a comment atastro.config.mjs:6-9explains that externalizing makes the import resolve to this app's copy, so leadtype's range must be kept in step. - Rewrote the
turbo.jsoncheck-typescomment — the count is corrected to seven and the scripts still carrying the self-build prefix are now named.
I verified the resolver-arm deletion is behavior-preserving rather than taking it on the comment's word. satteri/dist/index.js:13-14 statically imports only the #binding subpath, which maps to dist/binding.js; that file reaches its platform binding through createRequire(import.meta.url) followed by a runtime require('@bruits/satteri-*') — a call expression Rollup's static specifier discovery cannot see. Independently, a resolveId returning { external: true } bypasses the load/transform pipeline entirely, so index.js is never parsed once satteri is external. Nothing in the repo, in leadtype's built dist, or in packages/leadtype/src references a binding id statically. The arm was dead before this PR, and removing it is safe.
The count in the turbo.json comment is also now correct: exactly seven app check-types scripts carried the prefix, and none carry it after 81adabb.
ℹ️ Nitpicks
apps/astro-example/astro.config.mjs:6-9— the new coupling comment sits aboveconst NATIVE_PARSER = "satteri", a module specifier rather than a version range, so "Kept in step withpackages/leadtype/package.json's range" reads as though this line held the range. Someone bumpingsatteriatapps/astro-example/package.json:23never passes it.package.jsonis strict JSON and cannot carry the note itself, so there may be nowhere strictly better — but naming the dependency line explicitly in the wording would close the gap.
Claude Opus | 𝕏
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@turbo.json`:
- Around line 13-20: Remove the nested `bun run --filter leadtype build`
invocation from each app’s `build` script so app builds no longer independently
rebuild `leadtype`. Rely on the root Turbo `build` dependency configuration,
including the existing `^build` behavior, to build `leadtype` once before
dependents.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6ab5f081-dee6-4b56-b2aa-e67ea1d3a17c
📒 Files selected for processing (2)
apps/astro-example/astro.config.mjsturbo.json
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: pullfrog
🔇 Additional comments (1)
apps/astro-example/astro.config.mjs (1)
6-9: LGTM!Also applies to: 21-43, 45-48
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/leadtype/src/cli.test.ts`:
- Around line 2203-2234: Update the temporary-directory setup around runCli to
set TMPDIR, TMP, and TEMP to privateTmpDir, then restore or delete each variable
according to its original value in the finally block. Keep the existing leak
scan unchanged so it checks the directory actually used by the CLI across
platforms.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 283c4945-9be8-47b7-ac7f-7ce6207e70bb
📒 Files selected for processing (1)
packages/leadtype/src/cli.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: pullfrog
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Preferunknownoveranywhen the type is genuinely unknown
Use const assertions (as const) for immutable values and literal types
Leverage TypeScript's type narrowing instead of type assertions
Files:
packages/leadtype/src/cli.test.ts
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use meaningful variable names instead of magic numbers - extract constants with descriptive names
Use arrow functions for callbacks and short functions
Preferfor...ofloops over.forEach()and indexedforloops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Useconstby default,letonly when reassignment is needed, nevervar
Alwaysawaitpromises in async functions - don't forget to use the return value
Useasync/awaitsyntax instead of promise chains for better readability
Handle errors appropriately in async code with try-catch blocks
Don't use async functions as Promise executors
Removeconsole.log,debugger, andalertstatements from production code
ThrowErrorobjects with descriptive messages, not strings or other values
Usetry-catchblocks meaningfully - don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Don't useeval()or assign directly todocument.cookie
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Use descriptive names for functions, variables, and types for meaningful naming
Add comments for complex logic, but prefer self-documenting code
Files:
packages/leadtype/src/cli.test.ts
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write assertions insideit()ortest()blocks
Avoid done callbacks in async tests - use async/await instead
Don't use.onlyor.skipin committed code
Keep test suites reasonably flat - avoid excessivedescribenesting
Files:
packages/leadtype/src/cli.test.ts
🪛 ast-grep (0.45.0)
packages/leadtype/src/cli.test.ts
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
There was a problem hiding this comment.
ℹ️ The test change is sound and genuinely falsifiable — two nitpicks, neither blocking.
Reviewed changes — delta since the prior review of 39223d4; the new commit add9551 touches packages/leadtype/src/cli.test.ts and nothing else.
- Scoped the temp-dir leak assertion to a private
TMPDIR— the test "cleans up mirrored sources when the generate pipeline fails" no longer snapshotsleadtype-generate-*in the sharedos.tmpdir()before and afterrunCliand diffs the two sets. It creates a private dir, pointsprocess.env.TMPDIRat it for the duration of the CLI call, and globs only that dir. - Restored the env in a
finally— including the originally-unset case, so the mutation cannot outlive therunClicall.
I mutation-tested the assertion rather than trusting that the override lands: patching createSourceMirror's cleanup (packages/leadtype/src/cli/generate.ts:2553) to a no-op makes the test fail with + ["/tmp/leadtype-cli-XXXX/leadtype-generate-XXXX/"]. So the override is effective and the assertion can still catch the leak it exists to catch — the usual failure mode for this refactor, where the glob silently starts pointing at an always-empty directory, is not present here.
No coverage is lost relative to the old glob either. Every tmpdir() call site under src/ sits inside a function rather than a module-level constant (cli/generate.ts:2244,2531, internal/generate-lock.ts:73, source/index.ts:439, openapi/index.ts:2114), so all of them re-read process.env.TMPDIR per call, and this test's code path spawns no subprocess that could inherit a stale env. The comment's stated flake mechanism also checks out: generateLockPath returns leadtype-generate-<key>.lock and tryAcquire creates it with mkdir (internal/generate-lock.ts:167), so despite the suffix it is a directory and did match the old glob's onlyDirectories: true — a concurrent worker's lock landing inside the before/after window really was misattributable. And the env mutation cannot leak: tests in this file are sequential (no .concurrent), and vitest workers hold independent process.env copies. privateTmpDir itself comes from createTempDir() (cli.test.ts:97), so afterEach (cli.test.ts:156) already removes it.
ℹ️ Nitpicks
packages/leadtype/src/cli.test.ts:2204—os.tmpdir()readsTEMP/TMPon Windows and ignoresTMPDIRentirely, so thereprivateTmpDirstays empty andexpect(leakedTempDirs).toEqual([])passes vacuously rather than failing. CI is ubuntu-only (ci.yml:19) so there is no merge-gate impact, but a Windows contributor loses the assertion silently. SettingTMP/TEMPalongsideTMPDIRwould close it if the repo cares about that platform.- The PR title and body still describe "three pre-existing breakages in the example apps and task graph". This commit is a fourth change, and a test-flake fix in
packages/leadtypeis neither an example app nor the task graph — worth a line in the body so the merge commit records why it rode along.
Note on scope: add9551 does not touch turbo.json, so the open thread on turbo.json:16-18 from the previous review is unaffected and stays open.
Claude Opus | 𝕏
Two pre-existing breakages, both surfaced by running the monorepo build.
`apps/astro-example` could not build at all. `satteri` — leadtype's MDX parser —
is a napi-rs module whose loader picks a platform binding with a bare
`require("@bruits/satteri-darwin-arm64")`. Bundled into Astro's prerender
chunk that require resolves relative to `dist/.prerender/chunks/`, where the
binding isn't reachable, so static generation died with "Cannot find native
binding".
Declaring it in `resolve.external` does not work: `leadtype` is a workspace
package, so Astro's dependency crawl marks it `noExternal`, and noExternal
wins. A `resolveId` hook is unambiguous. Externalizing it also means the app
has to depend on it directly — under bun's isolated layout a transitive dep is
not resolvable from `apps/astro-example/dist/` — so `satteri` is now an
explicit dependency at the version leadtype pins.
The SvelteKit example generates into `static/` rather than `public/`, and the
two ignore lists had drifted: `feeds/`, `mcp.json`, `robots.txt`,
`schema-map.xml`, and both sitemaps were ignored under `public/` but not under
`static/`, so `bun run build` left six untracked build artifacts behind. The
lists now mirror each other and both use `apps/*/`, so a future example with
either output root is covered.
`bun run check-types` failed on main, and failed a *different* app each run — tanstack, then nuxt-example, then sveltekit-example. Not a type error: every example app's `check-types` script began with `bun run --filter leadtype build`, which rm -rf's and rebuilds `packages/leadtype/dist`. Turbo runs the tasks in parallel, so eight of those raced on one directory and whichever app was reading dist when another deleted it lost. Turbo already knows the dependency; the scripts were working around it. Adding `^build` to the `check-types` task builds leadtype once, ahead of every dependent, and the per-app self-build comes out. Verified with three consecutive full runs: 9 successful, 9 total, each time.
The Astro attribution was wrong, and wrong in the way that matters — it told the next reader the cause was Astro's dependency crawl marking `leadtype` noExternal, which cannot happen: that crawl matches only packages with an astro dep, an astro keyword, or an `astro-` name prefix, and leadtype has none. The comment now says what is actually true and checkable — this is monorepo-specific because `leadtype` is a linked workspace package — and records that the per-environment Vite keys were tried too, so nobody repeats that. The `@bruits/satteri-` arm of the resolver was unreachable: the loader reaches its platform binding through `createRequire`, so those ids are runtime CJS calls Vite never resolves, and once `satteri` is external its loader is not parsed at all. Verified by deleting it — 51 pages, exit 0 — so it is deleted. The satteri dependency now carries a note that externalizing the import makes it resolve to the app's copy rather than leadtype's, so the two ranges have to move together. The turbo comment said eight scripts; it was seven — the ninth task in turbo's output is leadtype's own `build`. It also read as though the prefix were gone repo-wide, when `build`, `dev`, `mcp`, and `test:e2e` still carry it. The race is fixed for `check-types`, which is the one turbo fans out.
The previous wording said other prefix-bearing scripts are not run concurrently by turbo. That is false for `build`: after packages/leadtype#build, the seven app builds are siblings. Keep the count at seven, and record that the race is fixed for check-types only. Also point bun.lock's workspace version at the package's current 0.4.3 after rebasing onto main.
add9551 to
5bf1235
Compare
|
Rebased onto None of the three original fixes had landed: Also dropped the later |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — delta since the prior review of add9551: the branch was rebased onto main at 76af4e4 and 5bf1235 corrects the turbo.json comment. Both open threads from earlier reviews are now resolved.
- Corrected the
check-typescomment's concurrency claim —turbo.json:18-21no longer says the other scripts "are not run concurrently by turbo". It now states thatturbo run buildfans the seven app builds out as siblings afterpackages/leadtype#build, so those nested rebuilds can still race, and that the fix is scoped tocheck-typesrather than repo-wide. - Rebased onto current
main— thepackages/leadtype/src/cli.test.tstemp-dir fix and theapps/next-example/app/generated/docs-mdx-map.tsrefresh both left the PR, bringing it back to the three changes the title and body describe. bun.lockversion correction moved with it —packages/leadtypenow reads 0.3.1 → 0.4.3, matchingpackages/leadtype/package.jsonexactly.
The rewritten comment is accurate, which is the thing the previous review blocked on. I re-checked each clause: turbo.json:5-9 still gives build only dependsOn: ["^build"], all seven build scripts still open with bun run --filter leadtype build &&, and root package.json:17 is an unfiltered turbo run build — so "those nested rebuilds can still race" is exactly right, and the count of seven matches the seven apps and seven now-prefix-free check-types scripts. Your note on the thread that the prefix is kept deliberately so standalone bun run --filter <app> build stays self-sufficient answers the design question that section was really asking; treating it as a separate change is a reasonable call.
The rebase does not stale anything. satteri is still imported from five files under packages/leadtype/src/, so the Astro workaround is still load-bearing, and apps/astro-example/package.json:23 is still identical to packages/leadtype/package.json:212 at ^0.9.3. The test-flake fix that dropped out was superseded rather than lost — main now narrows the same glob to source-mirror dirs at cli.test.ts:2292-2305.
I also re-verified the .gitignore half against the newer main's generator rather than assuming the earlier check still held, since roughly thirty commits of docs-pipeline work landed underneath it. Every write rooted at the CLI's out dir still lands inside the eleven ignored entries: docs/i18n-manifest.json (cli/generate.ts:1239), .well-known/agent-card.json (llm/skills.ts:243), both llms*.txt and their .well-known/ twins, docs/agent-readability.json plus .well-known/api-catalog, robots and both sitemaps (llm/llm.ts), feeds/schema.jsonl and schema-map.xml (nlweb/artifacts.ts:89-90), and mcp.json plus the two MCP cards (mcp/card.ts:227-229). The two top-level writers that would escape the list — generateAgentsMd's AGENTS.md and bundle-mode's root SKILL.md — are both gated behind args.bundle (generate.ts:1975), which no app passes.
Claude Opus | 𝕏

Three pre-existing breakages, all surfaced by running
bun run buildandbun run check-typesacross the monorepo while working on #157. Independent of that stack, so this is a standalone PR offmain.1.
bun run check-typesfailed onmain, differently each runtanstack, then nuxt-example, then sveltekit-example. Not a type error — every example app's
check-typesscript began withbun run --filter leadtype build, whichrm -rfs and rebuildspackages/leadtype/dist. Turbo runs the tasks in parallel, so eight of those raced on one directory and whichever app was readingdistwhen another deleted it lost.Turbo already knows the dependency; the scripts were working around it. Adding
^buildto thecheck-typestask builds leadtype once, ahead of every dependent, and the per-app self-build comes out.Verified with three consecutive full runs: 9 successful, 9 total, each time.
2.
apps/astro-examplecould not build at allsatteri— leadtype's MDX parser — is a napi-rs module whose loader picks a platform binding with a barerequire("@bruits/satteri-darwin-arm64"). Bundled into Astro's prerender chunk, that require resolves relative todist/.prerender/chunks/, where the binding isn't reachable:Two things were needed, and the first alone isn't enough:
resolveIdhook rather thanresolve.external.leadtypeis a workspace package, so Astro's dependency crawl marks itnoExternal— and noExternal wins. Astro 6's Vite 7 environment keys (environments.ssr/environments.prerender) don't get around that either; I tried both.satterias an explicit dependency of the app. Under bun's isolated node_modules layout a transitive dep isn't resolvable fromapps/astro-example/dist/, so externalizing without declaring just moves the failure toCannot find package 'satteri'. Pinned at the version leadtype uses.Verified: 51 pages built, exit 0.
3. SvelteKit ignore drift
The SvelteKit example generates into
static/rather thanpublic/. The two ignore lists had drifted —feeds/,mcp.json,robots.txt,schema-map.xml, and both sitemaps were ignored underpublic/but not understatic/— sobun run buildleft six untracked build artifacts in the tree, onegit add -Aaway from being committed.The lists now mirror each other and both use
apps/*/, so a future example with either output root is covered without a third list to keep in sync.