Remove TOC.md files when toc setting is disabled#11
Conversation
Co-authored-by: fbosch <6979916+fbosch@users.noreply.github.com>
Co-authored-by: fbosch <6979916+fbosch@users.noreply.github.com>
Co-authored-by: fbosch <6979916+fbosch@users.noreply.github.com>
Co-authored-by: fbosch <6979916+fbosch@users.noreply.github.com>
commit: |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the
📝 WalkthroughWalkthroughReplaces the index.json feature with per-source TOC.md generation. Removes top-level Changes
Sequence Diagram(s)sequenceDiagram
participant Sync as RunSync
participant Plan as Plan/Lock
participant Toc as writeToc
participant FS as Filesystem
participant Manifest as ManifestReader
Sync->>Plan: prepare plan & results
Sync->>Toc: writeToc(cacheDir, configPath, lock, sources, results)
Toc->>Plan: iterate lock.sources
Toc->>FS: check sourceDir exists
alt sourceDir exists
Toc->>Manifest: read .manifest.jsonl
Manifest-->>Toc: list of file paths
Toc->>FS: read existing TOC.md (if any)
Toc->>FS: write or remove TOC.md based on toc flag & up-to-date check
else missing sourceDir
Toc-->>Sync: skip source
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sync.ts (1)
485-495:⚠️ Potential issue | 🟠 MajorMissing inheritance from
defaults.toc.The current check
source.toc === truedoesn't account for thedefaults.tocsetting. If a user setsdefaults: { toc: true }but doesn't explicitly settoc: trueon each source, no TOC will be generated becausesource.tocwill beundefined.This same issue exists in
src/toc.tsat line 87 whereconst sourceToc = source?.toc ?? falseshould inherit from defaults.🐛 Proposed fix
// Check if any source has TOC enabled - const anySourceHasToc = plan.sources.some((source) => source.toc === true); + const defaultToc = plan.defaults.toc ?? false; + const anySourceHasToc = plan.sources.some( + (source) => (source.toc ?? defaultToc) === true + ); if (anySourceHasToc) { await writeToc({ cacheDir: plan.cacheDir, configPath: plan.configPath, lock, sources: plan.sources, + defaults: plan.defaults, }); }And in
src/toc.ts, update thewriteTocfunction to accept and use defaults:export const writeToc = async (params: { cacheDir: string; configPath: string; lock: DocsCacheLock; sources: DocsCacheResolvedSource[]; + defaults: DocsCacheDefaults; }) => { // ... - const sourceToc = source?.toc ?? false; + const sourceToc = source?.toc ?? params.defaults.toc ?? false;
🤖 Fix all issues with AI agents
In `@src/paths.ts`:
- Around line 43-48: Remove the unused tocPath property from the cache layout
return object (stop returning tocPath alongside cacheDir and sourceDir) and
eliminate the duplicate DEFAULT_TOC_FILENAME constant by consolidating to a
single definition: delete the local DEFAULT_TOC_FILENAME in src/paths.ts and
import the canonical DEFAULT_TOC_FILENAME from the module that owns it (the
symbol DEFAULT_TOC_FILENAME in toc.ts) or move the single constant to a shared
module; ensure any remaining code references use the single exported
DEFAULT_TOC_FILENAME and that sourceDir remains the location for per-source
TOCs.
In `@src/toc.ts`:
- Line 7: DEFAULT_TOC_FILENAME is duplicated; remove the local definition in the
toc module and instead import the already exported DEFAULT_TOC_FILENAME from the
existing paths module (or re-export that symbol from this module if you need to
preserve the same export surface). Update imports/usages in this file to
reference the imported DEFAULT_TOC_FILENAME and delete the redundant declaration
to avoid inconsistency.
🧹 Nitpick comments (4)
src/paths.ts (1)
4-4: Duplicate constant export detected.
DEFAULT_TOC_FILENAMEis exported from bothsrc/paths.ts(line 4) andsrc/toc.ts(line 7). This duplication creates a maintenance risk—if one value is updated without the other, they will diverge. Consolidate to a single source of truth.♻️ Suggested approach
Either:
- Keep the constant only in
src/paths.tsand import it insrc/toc.ts, or- Keep it only in
src/toc.ts(since it's TOC-specific) and import it insrc/paths.tstests/sync-toc.test.js (1)
86-152: Consider adding global TOC absence check for consistency.This test validates source-level
toc: truebehavior but doesn't verify that the global TOC is absent, unlike the first test. For consistency and to ensure the PR objective (removing global TOC) holds in both scenarios, consider adding the same assertion.💡 Suggested addition
assert.ok(sourceToc.includes("- [README.md](./README.md)")); assert.ok(sourceToc.includes("- [guide.md](./guide.md)")); + + // Check global TOC does NOT exist + const globalTocPath = path.join(cacheDir, "TOC.md"); + await assert.rejects( + () => readFile(globalTocPath, "utf8"), + /ENOENT/, + "Global TOC should not exist", + ); });src/toc.ts (2)
39-56: Silent error handling may mask data issues.The catch block returns an empty array for all errors, including JSON parse failures from malformed manifest entries. This could silently mask data corruption. Consider distinguishing between file-not-found (acceptable) and parse errors (should log or propagate).
💡 Suggested improvement
const readManifest = async (sourceDir: string): Promise<string[]> => { const manifestPath = path.join(sourceDir, ".manifest.jsonl"); try { const raw = await readFile(manifestPath, "utf8"); const files: string[] = []; for (const line of raw.split("\n")) { if (line.trim()) { - const entry = JSON.parse(line); - if (entry.path) { - files.push(entry.path); + try { + const entry = JSON.parse(line); + if (entry.path) { + files.push(entry.path); + } + } catch { + // Skip malformed lines but continue processing } } } return files; - } catch { + } catch (error) { + // File not found is expected for sources without manifests + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return []; + } + throw error; } - return []; };
77-86: TocEntry contains unused fields.
fileCount,cachePath, andtargetDirare populated in the entry but not used bygenerateSourceToc. If these are intended for future use, consider adding a comment. Otherwise, they can be removed to reduce complexity.
Co-authored-by: fbosch <6979916+fbosch@users.noreply.github.com>
Co-authored-by: fbosch <6979916+fbosch@users.noreply.github.com>
Co-authored-by: fbosch <6979916+fbosch@users.noreply.github.com>
Co-authored-by: fbosch <6979916+fbosch@users.noreply.github.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 76-95: The README claims the `toc` default is false but the actual
default in code is true; update one to match the other. Either change the code
in src/config.ts where DEFAULT_CONFIG.defaults.toc is set (toggle it to false)
or update the README text to state `toc` defaults to `true` (and adjust the
example list entry), making sure to reference DEFAULT_CONFIG.defaults.toc and
the README entry for `toc` so the documentation and implementation stay
consistent.
In `@src/config.ts`:
- Line 86: The default value for the "toc" setting in src/config.ts is currently
set to true but the README documents the default as false; decide which behavior
you want and make them consistent: either change the "toc" property in the
config export/object to false (replace toc: true with toc: false) or update the
README to state the default is true, and ensure any related constants, tests, or
consumers that rely on the default (e.g., DEFAULT_CONFIG or exported config
object) are updated accordingly so the code and documentation match.
🧹 Nitpick comments (1)
src/toc.ts (1)
28-69: Normalize/encode manifest paths before rendering TOC links.
Raw paths can contain backslashes or spaces, which can flatten the tree and break Markdown links on some platforms. Consider normalizing to POSIX and encoding link targets.♻️ Suggested update
const createTocTree = (files: string[]): TocTree => { const root: TocTree = { dirs: new Map(), files: [] }; for (const file of files) { - const parts = file.split("/").filter(Boolean); + const normalizedPath = toPosixPath(file); + const parts = normalizedPath.split("/").filter(Boolean); if (parts.length === 0) { continue; } @@ - const name = parts[parts.length - 1]; - node.files.push({ name, path: file }); + const name = parts[parts.length - 1]; + node.files.push({ name, path: normalizedPath }); } return root; }; @@ for (const file of files) { - lines.push(`${indent}- [${file.name}](./${file.path})`); + const linkPath = encodeURI(file.path); + lines.push(`${indent}- [${file.name}](./${linkPath})`); } };
There was a problem hiding this comment.
Pull request overview
This PR replaces the legacy index.json generation feature with a new per-source TOC.md (table of contents) feature that lists all documentation files in a structured markdown format. The key improvement is that TOC files are now properly cleaned up when the toc setting is disabled.
Changes:
- Replaced global
indexconfig option withtocoption indefaultsand per-source configuration (defaults totrue) - Added
src/toc.tswith logic to generate per-source TOC.md files and clean them up when disabled - Updated sync process to always call
writeTocto handle both generation and cleanup - Removed legacy
src/index.tsand associatedindex.jsongeneration
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/toc.ts | New file implementing TOC generation and cleanup logic for per-source documentation indexes |
| src/sync.ts | Replaced writeIndex call with writeToc, now always invoked to handle cleanup |
| src/config.ts | Removed index field, added toc field to defaults and sources with default value true |
| src/config-schema.ts | Updated Zod schema to replace index with toc field |
| src/init.ts | Updated prompts to ask about TOC generation instead of index, with default true |
| src/paths.ts | Renamed constant from DEFAULT_INDEX_FILENAME to DEFAULT_TOC_FILENAME |
| src/add.ts | Removed handling of index field when adding sources |
| src/remove.ts | Removed handling of index field when removing sources |
| src/index.ts | Deleted file (legacy index.json generation) |
| docs.config.schema.json | Updated JSON schema to remove index and add toc field |
| README.md | Updated documentation to describe new TOC feature and remove index references |
| tests/sync-toc.test.js | New comprehensive test suite for TOC generation, removal, and optimization |
| tests/sync-index.test.js | Deleted file (legacy index.json tests) |
| tests/init.test.js | Updated tests to verify TOC prompt and config generation behavior |
| tests/edge-cases-validation.test.js | Updated validation tests for new toc field |
Comments suppressed due to low confidence (1)
src/remove.ts:69
- The initial value of config is unused, since it is always overwritten.
let config = DEFAULT_CONFIG;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| await rm(sourceTocPath, { force: true }); | ||
| } catch { | ||
| // Ignore errors if file doesn't exist | ||
| } |
There was a problem hiding this comment.
The try-catch block here is unnecessary. When the force: true option is passed to rm(), it will not throw an error if the file doesn't exist - it will simply succeed silently. The try-catch can be removed, and the code can just be:
await rm(sourceTocPath, { force: true });
| try { | |
| await rm(sourceTocPath, { force: true }); | |
| } catch { | |
| // Ignore errors if file doesn't exist | |
| } | |
| await rm(sourceTocPath, { force: true }); |
| }; | ||
|
|
||
| // Generate per-source TOC if the source has TOC enabled | ||
| const sourceToc = source?.toc ?? true; |
There was a problem hiding this comment.
When a source is removed from the config but still exists in the lock file (which happens until prune is run), this code will default sourceToc to true because source will be undefined. This means TOC files will be generated for sources that are no longer in the configuration.
Consider treating sources not present in the config as having toc: false to ensure their TOC files are cleaned up. For example:
const sourceToc = source ? (source.toc ?? true) : false;
| const sourceToc = source?.toc ?? true; | |
| const sourceToc = source ? (source.toc ?? true) : false; |
Disabling
tocon a source only prevented new TOC.md generation but left existing files orphaned in the cache.Changes
src/toc.ts: Added cleanup path to delete TOC.md whentoc: falsesrc/sync.ts: Always invokewriteTocinstead of conditionally skipping when no sources have TOC enabledtests/sync-toc.test.js: Added test verifying TOC.md removal on config changeImplementation
The conditional guard in sync that only called
writeTocwhenanySourceHasTocprevented cleanup from running when all sources disabled TOC. Removing this guard allows the function to handle both generation and removal.💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.