Add incremental build support and watch mode to leadtype generate#119
Conversation
- Introduced `--watch` option for `leadtype generate`, enabling automatic rebuilds on changes to documentation sources and configuration files. - Implemented incremental builds by caching file inputs and skipping unchanged files, improving performance during repeated runs. - Updated the library API to support a new `cache` option in `convertAllMdx`, allowing for dependency tracking and cache invalidation. - Enhanced documentation to reflect these new features, including detailed explanations of the caching mechanism and watch mode behavior. These changes significantly enhance the efficiency and usability of the `leadtype` CLI for documentation generation.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds ChangesWatch mode and incremental builds
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant runGenerateCommand
participant executeGenerate
participant convertAllMdx
participant watchInputs
User->>runGenerateCommand: leadtype generate --watch
runGenerateCommand->>executeGenerate: initial run
executeGenerate->>convertAllMdx: convert with cache options
convertAllMdx-->>executeGenerate: { cached, pruned, outputs }
executeGenerate->>watchInputs: arm watcher with watchPaths
watchInputs-->>runGenerateCommand: debounced change batch
runGenerateCommand->>executeGenerate: rerun
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — adds --watch/-w and default incremental builds to leadtype generate, with a content-hash manifest cache, dependency tracking for <include> targets and type-table TypeScript sources, and stale-output pruning.
- CLI flags & docs —
parseGenerateArgsgainswatchandforce; help text anddocs/reference/cli.mdxdocument incremental builds and watch mode;.changesetmarks aminorbump. - Incremental cache (
convert/incremental.ts) — per-file manifest undernode_modules/.cache/leadtype/, recordingsourceHash, per-dependency hashes, serialized gitenrichment, and the output path. Every manifest miss (missing, corrupt, version or fingerprint mismatch) degrades to a full rebuild rather than erroring; the save path writes-then-renames to stay crash-safe. convertAllMdxcaching — skips a file only when its source hash, git enrichment, and all dependency hashes match and the prior output still exists;pruneStaleOutputsdeletes outputs of deleted sources, scoped strictly insideoutDir.- Dependency protocol — reuses the
_compiler.addDependencyfile-data channel (already emitted by the include plugin) and extendsTypeTableOptionswithonDependencyso type-table TypeScript reads invalidate the cache too. - Cache fingerprint —
resolveConvertCachekeys the cache on leadtype version, docs-config content hash, and conversion-relevant flags; caching is skipped entirely when there is nonode_modules/. - Watch loop (
cli/watch.ts,runGenerateWatch) — debounced recursivefs.watch(fine givenengines >= 22), ignoresoutDir/.git/node_modules/.DS_Store, serializes reruns, re-arms when a config edit changes the docs-source set, and stops cleanly onSIGINT/SIGTERM.
I verified the load-bearing seam: downstream site artifacts (search index, llms.txt, agents) read the converted markdown from outDir on disk rather than from convertAllMdx's return value, so a cached skip that leaves the output in place stays consistent — no stale-artifact bug. Dependency paths are recorded as real source paths (mapped back through gitSourcePath), so they survive the temp-mirror churn between runs. Test coverage is strong. The points below are informational only; nothing blocks merge.
ℹ️ --force is sticky across every watch rebuild
In watch mode each rebuild re-invokes generation with the original parsed args, so leadtype generate --watch --force reconverts every file on every change for the life of the session rather than forcing once and then rebuilding incrementally. That is defensible (force means force), but a user reaching for "clean start, then fast iteration" gets a full rebuild on each keystroke-triggered change instead — worth a deliberate decision.
Technical details
# `--force` is sticky across every watch rebuild
## Affected sites
- `packages/leadtype/src/cli/generate.ts` — `runGenerateWatch` → `rerun` calls `executeGenerate(args, io)` with the same `args.force` on every change; `resolveConvertCache` sets `force: true` each run, so `previousEntries` is always `{}`.
## Required outcome
- Decide whether `--force` should apply only to the initial watch build (then fall back to incremental) or intentionally to every rebuild. Current behavior is the latter.
## Suggested approach (optional)
- If one-shot force is desired: clear `args.force` (or pass a per-run flag) after the first `executeGenerate` in `runGenerateWatch`, leaving subsequent reruns incremental.
## Open questions for the human
- Is "force every rebuild in watch" intended, or should force be first-build-only? The CLI docs currently say "restart with `--force`", which reads as one-shot.ℹ️ No incremental test for type-table source invalidation
The changeset calls out type-table TypeScript sources as a newly tracked dependency, and the plugin now reports them through onDependency. The incremental test suite covers source edits, include-target edits, pruning, force, fingerprint, and corrupt-manifest recovery — but not the type-table path, so a regression in getFileDependencyReporter or the extractTypeFromFile dependency reporting would pass CI silently.
Technical details
# No incremental test for type-table source invalidation
## Affected sites
- `packages/leadtype/src/convert/incremental.test.ts` — has "rebuilds a file when its include target changes" but no equivalent for a type-table `.ts` source change.
- `packages/leadtype/src/markdown/plugins/type-table.ts:236-254`, `798-826` — the new dependency-reporting code paths (fresh extraction and cached-extraction branch) are untested for cache invalidation.
## Required outcome
- Add an incremental test asserting that editing a `.ts` file consumed by a type table rebuilds the page that renders it (and leaves an unrelated page cached), mirroring the existing include-target test.ℹ️ Nitpicks
packages/leadtype/src/convert/incremental.ts:125—saveConvertCacheManifestuses a fixed temp path${filePath}.tmp; two concurrentgenerateprocesses on one project (e.g. a--watchsession plus a manual run) could race on it. Impact is bounded (a corrupt manifest just triggers a rebuild), but a pid- ormkdtemp-suffixed temp name would remove the race.
Claude Opus ｜ 𝕏
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/leadtype/src/cli/generate.ts (1)
2629-2722: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWatch session can't detect a docs directory created after startup.
When
executeGeneratefails because a configured docs directory doesn't exist yet, the returnedwatchPathsstill includes that nonexistent path.watchInputs(seewatch.ts) silently skips watching paths that don't exist at arm-time, so if the user subsequently creates the missing docs directory, no change event fires for it — only edits to the config file (if it resolved) would trigger a re-arm. The watch session is then stuck requiring a manual restart, with no in-app hint.🩹 Suggested mitigation: fall back to watching the nearest existing ancestor
+function nearestExistingAncestor(target: string): string { + let dir = target; + while (!existsSync(dir)) { + const parent = path.dirname(dir); + if (parent === dir) { + return dir; + } + dir = parent; + } + return dir; +}Use
nearestExistingAncestor(candidate)when buildingwatchPathsfor any docs directory that doesn't yet exist, so its parent's creation events are observed and can trigger a re-check.🤖 Prompt for 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. In `@packages/leadtype/src/cli/generate.ts` around lines 2629 - 2722, The watch path selection in executeGenerate returns a nonexistent docs directory directly, so watchInputs never arms a watcher for it and later creation is missed. Update executeGenerate to detect missing docsDirs and replace each missing path in watchPaths with nearestExistingAncestor(candidate) so the parent directory creation can retrigger the generate flow. Keep the change localized around the docs directory existence check and watchPaths assembly, using nearestExistingAncestor and executeGenerate as the key symbols.
🤖 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/generate.ts`:
- Around line 2432-2483: The incremental cache is only enabled when
resolveConvertCache finds a node_modules directly under srcDir, which disables
caching for hoisted workspace layouts. Update resolveConvertCache in generate.ts
to locate the nearest usable node_modules by walking up from opts.srcDir
(Node-style resolution) instead of checking only the immediate child, while
keeping the existing cacheId/fingerprint logic and the ConvertCacheOptions file
path generation intact.
- Around line 2547-2607: The watch rerun path in runGenerateWatch keeps reusing
the same args object, so --force remains enabled for every rebuild and bypasses
the cache indefinitely. Update the watch flow so the force flag is only honored
for the initial executeGenerate call, then clear or override args.force before
subsequent reruns triggered by scheduleRerun/rerun. Make the change in
runGenerateWatch and keep the behavior aligned with resolveConvertCache so
incremental watch rebuilds can use the cache after the first pass.
In `@packages/leadtype/src/convert/convert.ts`:
- Around line 1357-1372: The dependency hashing in convert() is still done
sequentially inside the cacheState.nextEntries block, which unnecessarily
serializes I/O for dependencyPaths. Update the hashing logic near deps and
hashFileCached to mirror depsUnchanged by collecting all dependency hash
promises and awaiting them together with Promise.all, then build the deps record
from the resolved results while preserving the empty-string fallback for
unreadable or missing files.
- Around line 1270-1284: The cache manifest handling in convert.ts is dropping
prune history whenever config.cache.force is set or loadConvertCacheManifest
returns null on a fingerprint mismatch, which causes stale outputs to survive
forever. Update the cacheState setup so prune candidates are loaded
independently of force/fingerprint invalidation, and keep a separate history
source for pruneStaleOutputs instead of reusing previousEntries for both reuse
checks and pruning. Add a regression test in incremental.test.ts covering a
deleted source whose output is pruned across a force run or
fingerprint-invalidated run.
In `@packages/leadtype/src/convert/incremental.test.ts`:
- Around line 1-246: Add a test in incremental.test.ts that covers pruning after
a force run or fingerprint change using createCacheProject and runConvert. The
issue is that the current prune test only exercises the normal cache path, so it
misses the case where cacheState.previousEntries is cleared by force/fingerprint
invalidation and deleted outputs may not be removed. Reuse the existing helpers
(runConvert, tamper, existsSync, rm) to delete a source after an initial
conversion, rerun with force or a different fingerprint, and assert the deleted
file’s output is pruned while the remaining output stays intact.
In `@packages/leadtype/src/convert/incremental.ts`:
- Around line 79-116: The manifest validator in isManifestShape only checks that
entries exists as an object, so malformed ConvertCacheEntry data can slip
through and later crash pruneStaleOutputs when it reads entry.output. Tighten
isManifestShape in incremental.ts to validate every entry’s shape, including
required string/object fields such as sourceHash, deps, enrichment, and output,
and only return true when all entries match ConvertCacheEntry. Keep
loadConvertCacheManifest’s behavior unchanged so invalid manifests still fall
back to null and a full rebuild.
---
Outside diff comments:
In `@packages/leadtype/src/cli/generate.ts`:
- Around line 2629-2722: The watch path selection in executeGenerate returns a
nonexistent docs directory directly, so watchInputs never arms a watcher for it
and later creation is missed. Update executeGenerate to detect missing docsDirs
and replace each missing path in watchPaths with
nearestExistingAncestor(candidate) so the parent directory creation can
retrigger the generate flow. Keep the change localized around the docs directory
existence check and watchPaths assembly, using nearestExistingAncestor and
executeGenerate as the key symbols.
🪄 Autofix (Beta)
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
Run ID: 487a745a-9c1b-47a1-b20d-6a660a72251e
📒 Files selected for processing (9)
.changeset/watch-mode-incremental-builds.mddocs/reference/cli.mdxpackages/leadtype/src/cli/generate.tspackages/leadtype/src/cli/watch.test.tspackages/leadtype/src/cli/watch.tspackages/leadtype/src/convert/convert.tspackages/leadtype/src/convert/incremental.test.tspackages/leadtype/src/convert/incremental.tspackages/leadtype/src/markdown/plugins/type-table.ts
📜 Review details
🧰 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/watch.test.tspackages/leadtype/src/cli/watch.tspackages/leadtype/src/convert/incremental.test.tspackages/leadtype/src/convert/incremental.tspackages/leadtype/src/markdown/plugins/type-table.tspackages/leadtype/src/convert/convert.tspackages/leadtype/src/cli/generate.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/watch.test.tspackages/leadtype/src/cli/watch.tspackages/leadtype/src/convert/incremental.test.tspackages/leadtype/src/convert/incremental.tspackages/leadtype/src/markdown/plugins/type-table.tspackages/leadtype/src/convert/convert.tspackages/leadtype/src/cli/generate.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/watch.test.tspackages/leadtype/src/convert/incremental.test.ts
🧠 Learnings (1)
📚 Learning: 2026-06-09T18:30:08.038Z
Learnt from: KayleeWilliams
Repo: inthhq/leadtype PR: 97
File: .changeset/search-prototype-safety-and-scaling.md:5-5
Timestamp: 2026-06-09T18:30:08.038Z
Learning: In this repo, `.changeset/*.md` files must not start the body with an H1/first-line heading (`#`) immediately after the YAML frontmatter. The changesets tool inlines the body as bullet entries into `CHANGELOG.md` during release, and a leading `#` heading would break the generated changelog format. As a result, MD041 (`first-line-heading`) warnings for files under `.changeset/` are expected false positives and should be ignored.
Applied to files:
.changeset/watch-mode-incremental-builds.md
🪛 ast-grep (0.44.0)
packages/leadtype/src/convert/convert.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)
[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)
[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)
[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)
[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)
[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)
[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)
[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)
[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)
[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)
🪛 markdownlint-cli2 (0.22.1)
.changeset/watch-mode-incremental-builds.md
[warning] 5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (13)
.changeset/watch-mode-incremental-builds.md (1)
1-12: 📐 Maintainability & Code QualityMD041 warning is an expected false positive for this repo's changeset files.
Based on learnings,
.changeset/*.mdfiles intentionally must not start with an H1 heading immediately after frontmatter, since the changesets tool inlines the body as bullet entries intoCHANGELOG.md; a leading#would break that format. The static-analysis MD041 hint here should be ignored.Source: Learnings
packages/leadtype/src/cli/watch.ts (1)
1-133: LGTM!packages/leadtype/src/cli/watch.test.ts (1)
1-158: LGTM!packages/leadtype/src/cli/generate.ts (5)
1-11: LGTM!Also applies to: 77-77
183-186: LGTM!Also applies to: 354-366, 396-397, 445-448
2401-2413: LGTM!
2536-2540: LGTM!Also applies to: 2609-2634, 2694-2699, 2721-2721, 2779-2789, 3161-3165
2414-2430: 🗄️ Data Integrity & IntegrationDrop the self-reference warning for
leadtype/package.json.packages/leadtype/package.jsonalready exports"./package.json", socreateRequire(import.meta.url)can resolve it and this fallback won’t be triggered by package exports alone.> Likely an incorrect or invalid review comment.docs/reference/cli.mdx (1)
60-79: LGTM!packages/leadtype/src/convert/convert.ts (2)
1458-1497: 🎯 Functional CorrectnessCovered by the pruning-history comment above.
The
entry.outputtraversal-guard here is solid, but this function's inputs need the fix proposed on Lines 1270-1284 (using history-preserving prune candidates and per-entry validation) to avoid silently losing prune opportunities and to avoid crashing on malformed manifest entries.
3-3: LGTM!Also applies to: 37-47, 347-352, 374-379, 842-852, 1286-1356, 1401-1452
packages/leadtype/src/convert/incremental.ts (1)
1-77: LGTM!Also applies to: 118-147
packages/leadtype/src/markdown/plugins/type-table.ts (1)
192-212: LGTM!Also applies to: 236-255, 910-932, 1091-1113
Resolves conflicts with the recently merged prune (#125) and redirect tracking (#126) work: incremental-cache writes now go through writeFileAtomic (concurrent-generate safety from #121), the cache option composes with prune/pruneKeep on the generate convert call, and the lock-acquisition failure path returns the new GenerateOutcome shape.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the delta since the prior pullfrog review is the merge of main (commit 95f17679), which integrates three main-side features with the incremental cache introduced earlier in this PR.
- Atomic output writes — the cache write path in
convertMdxBatchnow useswriteFileAtomic(write-then-rename) instead ofwriteFile; theexistsSync(outputPath)reuse check is unaffected. prune: truealongsidecache—convertAllMdxis now invoked with both the incrementalcacheand redirect-tracking'sprune/pruneKeepwhenredirectsEnabled.- Per-
outDirgenerate lock —acquireGenerateLockis taken inexecuteGenerateand released in itsfinally, so each watch rerun acquires and releases it fresh rather than holding across the session. docs/paths.lock.jsonhash refresh — mechanical lockfile updates carried in by the merge.
I traced the load-bearing interaction between the incremental cache and the merged features. pruneAfterBatch builds its keep-set from the full mdxFiles source list rather than the files actually written this run, so a cached (skipped) output is never treated as orphaned. Redirect tracking reads its page set from the on-disk agent-readability manifest (agentReadability.manifest.pages) and reads emitted .md content from disk, so cached skips stay visible to rename/deletion detection while deleted sources still get pruned (by both pruneStaleOutputs and pruneOrphanedOutputs) so detection fires. The two prune paths overlap when both are active, but that is redundant work, not a correctness problem. Nothing here blocks merge.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/leadtype/src/convert/convert.ts (2)
1520-1528: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential dependency hashing — still not parallelized (previously flagged).
Each dependency path is hashed one at a time in a
for...ofloop, unlikedepsUnchangedwhich parallelizes viaPromise.all. For pages with many dependencies (e.g. type-table pages), this serializes I/O unnecessarily.⚡ Suggested fix
if (cacheState && sourceHash) { - const deps: Record<string, string> = {}; - for (const dependencyPath of dependencyPaths ?? []) { - deps[dependencyPath] = - (await hashFileCached(dependencyPath, cacheState.hashCache)) ?? ""; - } + const deps = Object.fromEntries( + await Promise.all( + Array.from(dependencyPaths ?? [], async (dependencyPath) => [ + dependencyPath, + (await hashFileCached(dependencyPath, cacheState.hashCache)) ?? + "", + ]) + ) + );🤖 Prompt for 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. In `@packages/leadtype/src/convert/convert.ts` around lines 1520 - 1528, The dependency hashing in convert.ts is still sequential inside the cacheState/sourceHash block, which slows pages with many dependency paths. Update the dependency collection in the `convert` flow to hash all `dependencyPaths` in parallel, following the `depsUnchanged` pattern with `Promise.all`, while preserving the current empty-string fallback for unreadable or missing files. Keep the `deps` object construction tied to the same `hashFileCached` and `cacheState.hashCache` usage so the rest of the logic remains unchanged.
1433-1443: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPruning history is still lost when the cache is invalidated (
--force/ fingerprint mismatch) — previously flagged, unresolved.
previousEntriesis{}wheneverconfig.cache.forceis true or the fingerprint mismatches (Line 1436-1443), and this same (now-empty)previousEntriesis whatpruneStaleOutputsuses at Line 1571-1576 to decide what to prune. Any entry for a source deleted before a force/fingerprint-invalidated run is dropped from the manifest with no chance to prune its output — leaving an orphaned.mdfile inoutDirpermanently (e.g. after any leadtype version bump combined with a previously-removed doc).This is the same issue raised on a prior commit; the code is unchanged.
♻️ Suggested fix: load prune candidates independently of force/fingerprint
+// in incremental.ts +export async function loadConvertCacheEntriesForPrune( + filePath: string +): Promise<Record<string, ConvertCacheEntry>> { + ... +}const cacheState = config.cache ? { options: config.cache, previousEntries: config.cache.force ? {} : (( await loadConvertCacheManifest( config.cache.file, config.cache.fingerprint ) )?.entries ?? {}), + pruneEntries: await loadConvertCacheEntriesForPrune(config.cache.file), nextEntries: {} as Record<string, ConvertCacheEntry>, hashCache: createFileHashCache(), } : undefined;pruned = await pruneStaleOutputs( - cacheState.previousEntries, + cacheState.pruneEntries, cacheState.nextEntries, currentKeys, outDir );Also applies to: 1571-1576
🤖 Prompt for 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. In `@packages/leadtype/src/convert/convert.ts` around lines 1433 - 1443, Pruning history is still being lost when the cache is invalidated, because `cacheState.previousEntries` in `convert.ts` is reset to an empty object on `config.cache.force` or fingerprint mismatch and that same value is later consumed by `pruneStaleOutputs`. Update the cache handling so stale-prune candidates are loaded independently of the force/fingerprint invalidation path, and ensure `pruneStaleOutputs` can still see prior manifest entries even when the active cache is treated as invalid. Use the existing `loadConvertCacheManifest`, `cacheState`, and `pruneStaleOutputs` flow to preserve old entries for pruning while still invalidating cache reuse.
🤖 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/generate.ts`:
- Around line 2228-2274: The pruning logic has a safety issue where the second
fg call that retrieves mirroredFiles can include files from other mounts if
output directories overlap or nest, then incorrectly delete them as stale when
comparing against currentFiles. When calling fg to get mirroredFiles from the
targetDir, add an ignore pattern to exclude other mount paths from the search
result, or alternatively filter the staleFiles computation to only consider
files that could have come from the current mount's source subtree. This ensures
that files belonging to other mounts are never mistakenly pruned as stale.
In `@packages/leadtype/src/convert/convert.ts`:
- Around line 1246-1288: The orphan-pruning and empty-directory cleanup logic in
pruneOrphanedOutputs duplicates the same upward-walk rmdir behavior used by
removeEmptyMirrorDirs. Extract the shared empty-parent cleanup into a common
internal helper (for example in internal/atomic-fs.ts) and have both
pruneOrphanedOutputs and removeEmptyMirrorDirs call it so the behavior stays
consistent and doesn’t drift across files.
---
Duplicate comments:
In `@packages/leadtype/src/convert/convert.ts`:
- Around line 1520-1528: The dependency hashing in convert.ts is still
sequential inside the cacheState/sourceHash block, which slows pages with many
dependency paths. Update the dependency collection in the `convert` flow to hash
all `dependencyPaths` in parallel, following the `depsUnchanged` pattern with
`Promise.all`, while preserving the current empty-string fallback for unreadable
or missing files. Keep the `deps` object construction tied to the same
`hashFileCached` and `cacheState.hashCache` usage so the rest of the logic
remains unchanged.
- Around line 1433-1443: Pruning history is still being lost when the cache is
invalidated, because `cacheState.previousEntries` in `convert.ts` is reset to an
empty object on `config.cache.force` or fingerprint mismatch and that same value
is later consumed by `pruneStaleOutputs`. Update the cache handling so
stale-prune candidates are loaded independently of the force/fingerprint
invalidation path, and ensure `pruneStaleOutputs` can still see prior manifest
entries even when the active cache is treated as invalid. Use the existing
`loadConvertCacheManifest`, `cacheState`, and `pruneStaleOutputs` flow to
preserve old entries for pruning while still invalidating cache reuse.
🪄 Autofix (Beta)
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
Run ID: a7715a88-699e-432c-a4ba-9722f4ea8b8f
📒 Files selected for processing (3)
docs/paths.lock.jsonpackages/leadtype/src/cli/generate.tspackages/leadtype/src/convert/convert.ts
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: CI / 0_Validate & test.txt: Add incremental build support and watch mode to leadtype generate
Conclusion: failure
Current runner version: '2.335.1'
##[group]Runner Image Provisioner
Hosted Compute Agent
Version: 20260624.560
Commit: 925d229a51159bc391ae97e54a2dd1fe20af789d
Build Date:
Worker ID: {84a1cb75-1d54-4cab-9b27-98228a50521a}
Azure Region: eastus2
##[endgroup]
##[group]Operating System
Ubuntu
24.04.4
LTS
##[endgroup]
##[group]Runner Image
Image: ubuntu-24.04
Version: 20260628.225.1
Included Software: https://github.com/actions/runner-images/blob/ubuntu24/20260628.225/images/ubuntu/Ubuntu2404-Readme.md
Image Release: https://github.com/actions/runner-images/releases/tag/ubuntu24%2F20260628.225
##[endgroup]
##[group]GITHUB_TOKEN Permissions
Contents: read
Metadata: read
##[endgroup]
Secret source: Actions
Prepare workflow directory
Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v4' (SHA:34e114876b0b11c390a56381ad16ebd13914f8d5)
Download action repository 'oven-sh/setup-bun@v2' (SHA:0c5077e51419868618aeaa5fe8019c62421857d6)
Complete job name: Validate & test
Node 20 is being deprecated. This workflow is running with Node 24 by default. If you need to temporarily use Node 20, you can set the ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION=true environment variable. For more information see: https://github.blog/changelog/2025-09-19-deprecation-of-node-20-on-github-actions-runners/
##[group]Run actions/checkout@v4
with:
repository: inthhq/leadtype
***REDACTED***
ssh-strict: true
ssh-user: git
persist-credentials: true
clean: true
sparse-checkout-cone-mode: true
fetch-depth: 1
fetch-tags: false
show-progress: true
lfs: false
submodules: false
set-safe-directory: true
##[endgroup]
Syncing repository: inthhq/leadtype
##[group]Getting Git version info
Working directory is '/home/runner/work/leadtype/leadtype'
[command]/usr/bin/git version
git version 2.54.0
##[endgroup]
Temporarily overriding HOME='/home/runner/work/_temp/655fcd59-6728-4f10-9224-59414...
GitHub Actions: CI / Validate & test: Add incremental build support and watch mode to leadtype generate
Conclusion: failure
##[group]Run bun run --filter leadtype test
�[36;1mbun run --filter leadtype test�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
leadtype test:
leadtype test: �[1m�[30m�[46m RUN �[49m�[39m�[22m �[36mv4.1.5 �[39m�[90m/home/runner/work/leadtype/leadtype/packages/leadtype�[39m
leadtype test:
leadtype test: �[32m✓�[39m src/llm/llm.test.ts �[2m(�[22m�[2m86 tests�[22m�[2m)�[22m�[33m 324�[2mms�[22m�[39m
leadtype test: �[32m✓�[39m src/openapi/openapi.test.ts �[2m(�[22m�[2m32 tests�[22m�[2m)�[22m�[33m 421�[2mms�[22m�[39m
leadtype test: Converted 1 docs in 19 ms
leadtype test: Converted 2 docs in 7 ms
leadtype test: Converted 2 docs in 7 ms
leadtype test: Pruned 1 orphaned .md file(s) from /tmp/leadtype-convert-WAk0Iw/public
leadtype test: Converted 2 docs in 3 ms
leadtype test: Converted 1 docs in 2 ms
leadtype test: Pruned 2 orphaned .md file(s) from /tmp/leadtype-convert-CV8zhC/public
leadtype test: Converted 1 docs in 3 ms
leadtype test: Pruned 1 orphaned .md file(s) from /tmp/leadtype-convert-kXjOYm/public
leadtype test: Converted 1 docs in 1 ms
leadtype test: Pruned 1 orphaned .md file(s) from /tmp/leadtype-convert-2NrDCJ/public
leadtype test: Converted 1 docs in 2 ms
leadtype test: Error: failed to process /tmp/leadtype-convert-v4MQe3/docs/broken.mdx: 3:1: Unexpected character after `<`, expected a valid JSX tag (note: to create a link in MDX, use `[text](url)`) (mdx-jsx:unexpected-character)
leadtype test: Converted 1 docs in 3 ms (1 failed)
leadtype test: Warning: prune skipped: 1 file(s) failed to convert, so the expected output set is incomplete.
leadtype test: Warning: prune skipped: no .mdx sources under /tmp/leadtype-convert-fM7b8H/empty-docs — refusing to treat every output in /tmp/leadtype-convert-fM7b8H/public as orphaned. Check srcDir if this is unexpected.
leadtype test: Converted 3 docs in 11 ms
leadtype test: Converted 2 docs in 4 ms
leadtype test: Converted 1 docs in 1 ms
leadtype test: �[32m✓�[39m src/convert/convert.test...
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/convert/convert.tspackages/leadtype/src/cli/generate.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/convert/convert.tspackages/leadtype/src/cli/generate.ts
🪛 ast-grep (0.44.1)
packages/leadtype/src/convert/convert.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)
[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)
[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)
[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)
[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)
[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)
[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)
[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)
[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)
🔇 Additional comments (7)
packages/leadtype/src/cli/generate.ts (3)
2974-2992: 🎯 Functional CorrectnessPlease confirm past-review concerns on
resolveConvertCache/watch reruns were addressed.Two previously-raised concerns on this file's cache/watch wiring aren't visible in the current diff context (the
resolveConvertCacheandrunGenerateWatchbodies fall in the unchanged 2524-2747 range not shown here):
- Cache silently disabled for hoisted monorepo
node_moduleslayouts (only checkssrcDir/node_modulesdirectly).--watch --forcekeeps forcing a full rebuild on every rerun, not just the first, sinceargsis never mutated between reruns.Since the underlying functions aren't part of this review's visible context, please confirm whether these were addressed in the current implementation.
#!/bin/bash # Inspect current resolveConvertCache / runGenerateWatch implementations. rg -nP -A30 'function resolveConvertCache' packages/leadtype/src/cli/generate.ts rg -nP -A40 'function runGenerateWatch' packages/leadtype/src/cli/generate.ts
1-29: LGTM!Also applies to: 77-86, 202-263, 305-329, 1281-1411, 1872-1936, 2396-2457, 2520-2523
2748-2862: LGTM!Also applies to: 3394-3403
packages/leadtype/src/convert/convert.ts (3)
43-53: LGTM!Also applies to: 370-376, 397-402, 1140-1204
1294-1359: LGTM!Also applies to: 1564-1591
1433-1443: 🎯 Functional CorrectnessNo cache fingerprint gap The build-wide fingerprint already hashes the full docs config file, so config-only changes to
markdownTransforms,frontmatterSchema,typeTableBasePath/typeTableStrict, andtransformersinvalidate the cache.> Likely an incorrect or invalid review comment.docs/paths.lock.json (1)
1-218: LGTM!
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/leadtype/src/convert/convert.ts (2)
1520-1528: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential dependency hashing — still not parallelized (previously flagged).
Each dependency path is hashed one at a time in a
for...ofloop, unlikedepsUnchangedwhich parallelizes viaPromise.all. For pages with many dependencies (e.g. type-table pages), this serializes I/O unnecessarily.⚡ Suggested fix
if (cacheState && sourceHash) { - const deps: Record<string, string> = {}; - for (const dependencyPath of dependencyPaths ?? []) { - deps[dependencyPath] = - (await hashFileCached(dependencyPath, cacheState.hashCache)) ?? ""; - } + const deps = Object.fromEntries( + await Promise.all( + Array.from(dependencyPaths ?? [], async (dependencyPath) => [ + dependencyPath, + (await hashFileCached(dependencyPath, cacheState.hashCache)) ?? + "", + ]) + ) + );🤖 Prompt for 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. In `@packages/leadtype/src/convert/convert.ts` around lines 1520 - 1528, The dependency hashing in convert.ts is still sequential inside the cacheState/sourceHash block, which slows pages with many dependency paths. Update the dependency collection in the `convert` flow to hash all `dependencyPaths` in parallel, following the `depsUnchanged` pattern with `Promise.all`, while preserving the current empty-string fallback for unreadable or missing files. Keep the `deps` object construction tied to the same `hashFileCached` and `cacheState.hashCache` usage so the rest of the logic remains unchanged.
1433-1443: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPruning history is still lost when the cache is invalidated (
--force/ fingerprint mismatch) — previously flagged, unresolved.
previousEntriesis{}wheneverconfig.cache.forceis true or the fingerprint mismatches (Line 1436-1443), and this same (now-empty)previousEntriesis whatpruneStaleOutputsuses at Line 1571-1576 to decide what to prune. Any entry for a source deleted before a force/fingerprint-invalidated run is dropped from the manifest with no chance to prune its output — leaving an orphaned.mdfile inoutDirpermanently (e.g. after any leadtype version bump combined with a previously-removed doc).This is the same issue raised on a prior commit; the code is unchanged.
♻️ Suggested fix: load prune candidates independently of force/fingerprint
+// in incremental.ts +export async function loadConvertCacheEntriesForPrune( + filePath: string +): Promise<Record<string, ConvertCacheEntry>> { + ... +}const cacheState = config.cache ? { options: config.cache, previousEntries: config.cache.force ? {} : (( await loadConvertCacheManifest( config.cache.file, config.cache.fingerprint ) )?.entries ?? {}), + pruneEntries: await loadConvertCacheEntriesForPrune(config.cache.file), nextEntries: {} as Record<string, ConvertCacheEntry>, hashCache: createFileHashCache(), } : undefined;pruned = await pruneStaleOutputs( - cacheState.previousEntries, + cacheState.pruneEntries, cacheState.nextEntries, currentKeys, outDir );Also applies to: 1571-1576
🤖 Prompt for 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. In `@packages/leadtype/src/convert/convert.ts` around lines 1433 - 1443, Pruning history is still being lost when the cache is invalidated, because `cacheState.previousEntries` in `convert.ts` is reset to an empty object on `config.cache.force` or fingerprint mismatch and that same value is later consumed by `pruneStaleOutputs`. Update the cache handling so stale-prune candidates are loaded independently of the force/fingerprint invalidation path, and ensure `pruneStaleOutputs` can still see prior manifest entries even when the active cache is treated as invalid. Use the existing `loadConvertCacheManifest`, `cacheState`, and `pruneStaleOutputs` flow to preserve old entries for pruning while still invalidating cache reuse.
🤖 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/generate.ts`:
- Around line 2228-2274: The pruning logic has a safety issue where the second
fg call that retrieves mirroredFiles can include files from other mounts if
output directories overlap or nest, then incorrectly delete them as stale when
comparing against currentFiles. When calling fg to get mirroredFiles from the
targetDir, add an ignore pattern to exclude other mount paths from the search
result, or alternatively filter the staleFiles computation to only consider
files that could have come from the current mount's source subtree. This ensures
that files belonging to other mounts are never mistakenly pruned as stale.
In `@packages/leadtype/src/convert/convert.ts`:
- Around line 1246-1288: The orphan-pruning and empty-directory cleanup logic in
pruneOrphanedOutputs duplicates the same upward-walk rmdir behavior used by
removeEmptyMirrorDirs. Extract the shared empty-parent cleanup into a common
internal helper (for example in internal/atomic-fs.ts) and have both
pruneOrphanedOutputs and removeEmptyMirrorDirs call it so the behavior stays
consistent and doesn’t drift across files.
---
Duplicate comments:
In `@packages/leadtype/src/convert/convert.ts`:
- Around line 1520-1528: The dependency hashing in convert.ts is still
sequential inside the cacheState/sourceHash block, which slows pages with many
dependency paths. Update the dependency collection in the `convert` flow to hash
all `dependencyPaths` in parallel, following the `depsUnchanged` pattern with
`Promise.all`, while preserving the current empty-string fallback for unreadable
or missing files. Keep the `deps` object construction tied to the same
`hashFileCached` and `cacheState.hashCache` usage so the rest of the logic
remains unchanged.
- Around line 1433-1443: Pruning history is still being lost when the cache is
invalidated, because `cacheState.previousEntries` in `convert.ts` is reset to an
empty object on `config.cache.force` or fingerprint mismatch and that same value
is later consumed by `pruneStaleOutputs`. Update the cache handling so
stale-prune candidates are loaded independently of the force/fingerprint
invalidation path, and ensure `pruneStaleOutputs` can still see prior manifest
entries even when the active cache is treated as invalid. Use the existing
`loadConvertCacheManifest`, `cacheState`, and `pruneStaleOutputs` flow to
preserve old entries for pruning while still invalidating cache reuse.
🪄 Autofix (Beta)
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
Run ID: a7715a88-699e-432c-a4ba-9722f4ea8b8f
📒 Files selected for processing (3)
docs/paths.lock.jsonpackages/leadtype/src/cli/generate.tspackages/leadtype/src/convert/convert.ts
📜 Review details
🔇 Additional comments (7)
packages/leadtype/src/cli/generate.ts (3)
2974-2992: 🎯 Functional CorrectnessPlease confirm past-review concerns on
resolveConvertCache/watch reruns were addressed.Two previously-raised concerns on this file's cache/watch wiring aren't visible in the current diff context (the
resolveConvertCacheandrunGenerateWatchbodies fall in the unchanged 2524-2747 range not shown here):
- Cache silently disabled for hoisted monorepo
node_moduleslayouts (only checkssrcDir/node_modulesdirectly).--watch --forcekeeps forcing a full rebuild on every rerun, not just the first, sinceargsis never mutated between reruns.Since the underlying functions aren't part of this review's visible context, please confirm whether these were addressed in the current implementation.
#!/bin/bash # Inspect current resolveConvertCache / runGenerateWatch implementations. rg -nP -A30 'function resolveConvertCache' packages/leadtype/src/cli/generate.ts rg -nP -A40 'function runGenerateWatch' packages/leadtype/src/cli/generate.ts
1-29: LGTM!Also applies to: 77-86, 202-263, 305-329, 1281-1411, 1872-1936, 2396-2457, 2520-2523
2748-2862: LGTM!Also applies to: 3394-3403
packages/leadtype/src/convert/convert.ts (3)
43-53: LGTM!Also applies to: 370-376, 397-402, 1140-1204
1294-1359: LGTM!Also applies to: 1564-1591
1433-1443: 🎯 Functional CorrectnessNo cache fingerprint gap The build-wide fingerprint already hashes the full docs config file, so config-only changes to
markdownTransforms,frontmatterSchema,typeTableBasePath/typeTableStrict, andtransformersinvalidate the cache.> Likely an incorrect or invalid review comment.docs/paths.lock.json (1)
1-218: LGTM!
🛑 Comments failed to post (2)
packages/leadtype/src/cli/generate.ts (1)
2228-2274: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check whether mount urlPrefix/targetDir exclusivity is validated anywhere. rg -nP -C5 'function\s+outputDirForUrlPrefix|function\s+validateDocsMounts' packages/leadtype/srcRepository: inthhq/leadtype
Length of output: 1714
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the mount validation and mirror-pruning implementation. sed -n '867,980p' packages/leadtype/src/cli/generate.ts printf '\n---\n' sed -n '2190,2295p' packages/leadtype/src/cli/generate.ts printf '\n=== search for mount overlap / exclusivity checks ===\n' rg -n --context 3 'validateDocsMounts|outputDirForUrlPrefix|targetDir|urlPrefix|mirror|staleFiles|removeEmptyMirrorDirs|copyMountedMarkdownMirrors' packages/leadtype/src/cli/generate.tsRepository: inthhq/leadtype
Length of output: 19856
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect how collections and mounts are resolved and whether overlapping URL prefixes are rejected. sed -n '1979,2079p' packages/leadtype/src/cli/generate.ts printf '\n---\n' sed -n '1200,1465p' packages/leadtype/src/cli/generate.ts printf '\n=== overlap-related searches ===\n' rg -n --context 2 'seenUrlPrefixes|seenMounts|multiple collections share URL prefix|mounts|pathPrefixForUrlPrefix|normalizeUrlPrefix|normalizeDocsPath' packages/leadtype/src/cli/generate.tsRepository: inthhq/leadtype
Length of output: 22305
Pruning needs an ownership boundary. Collections-level uniqueness checks don’t cover
source.mounts/metadata.mounts, sofg("**/*.md", { cwd: targetDir })can sweep in another mount’s files and delete them as stale if output dirs overlap or nest. Add an output-dir exclusivity check or prune from a per-mount manifest instead.🤖 Prompt for 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. In `@packages/leadtype/src/cli/generate.ts` around lines 2228 - 2274, The pruning logic has a safety issue where the second fg call that retrieves mirroredFiles can include files from other mounts if output directories overlap or nest, then incorrectly delete them as stale when comparing against currentFiles. When calling fg to get mirroredFiles from the targetDir, add an ignore pattern to exclude other mount paths from the search result, or alternatively filter the staleFiles computation to only consider files that could have come from the current mount's source subtree. This ensures that files belonging to other mounts are never mistakenly pruned as stale.packages/leadtype/src/convert/convert.ts (1)
1246-1288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate "prune stale files + remove empty parent dirs" logic vs.
generate.ts.
pruneOrphanedOutputs/its empty-dir cleanup loop (Lines 1272-1285) closely mirrorsremoveEmptyMirrorDirsinpackages/leadtype/src/cli/generate.ts(Lines 2276-2303) — same upward-walk-and-rmdir-until-non-empty pattern, duplicated across two files. Consider extracting a shared internal helper (e.g. ininternal/atomic-fs.ts) to avoid the two implementations drifting.🧰 Tools
🪛 ast-grep (0.44.1)
[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)
🤖 Prompt for 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. In `@packages/leadtype/src/convert/convert.ts` around lines 1246 - 1288, The orphan-pruning and empty-directory cleanup logic in pruneOrphanedOutputs duplicates the same upward-walk rmdir behavior used by removeEmptyMirrorDirs. Extract the shared empty-parent cleanup into a common internal helper (for example in internal/atomic-fs.ts) and have both pruneOrphanedOutputs and removeEmptyMirrorDirs call it so the behavior stays consistent and doesn’t drift across files.
…on, hoisted node_modules, watch force semantics - Stale-output pruning now works from the on-disk manifest even when --force or a fingerprint change disables entry reuse; previously a source deleted before an invalidated run left its output orphaned forever (the rewritten manifest lost the entry). Tests cover both invalidation paths and fail against the previous wiring. - isManifestShape validates every cache entry, so a partially-written or hand-edited entry rejects the whole manifest (full rebuild) instead of crashing pruning on path.resolve(undefined). - The cache manifest now lives in the nearest node_modules walking up from --src, so hoisted monorepo subpackages get caching too. - --watch --force forces only the initial build; watch-triggered rebuilds stay incremental. Docs updated for both behavior changes.
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
--watchoption forleadtype generate, enabling automatic rebuilds on changes to documentation sources and configuration files.cacheoption inconvertAllMdx, allowing for dependency tracking and cache invalidation.These changes significantly enhance the efficiency and usability of the
leadtypeCLI for documentation generation.