Repository navigation
fix(cli): improve inspect concurrency performance hints - #101
Merged
Merged
Conversation
This reverts commit ed7d9fe.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (8)
Summary by CodeRabbit
WalkthroughThis PR renames archive index commands from build/clear to enable/disable, adds Sequence Diagram(s)sequenceDiagram
participant CLI as archive CLI
participant Args as args parser
participant ArchiveView as archive-view
participant Stores as document serial store
participant Cursor as continuation cursor
CLI->>Args: parse --reverse / --query / index action
Args-->>CLI: validated options and action
CLI->>ArchiveView: list/get/evidence/related with order
ArchiveView->>Stores: listDocumentOrders()
Stores-->>ArchiveView: serialId -> documentOrder map
ArchiveView-->>CLI: ordered results and evidence
CLI->>Cursor: create/read continuation cursor with order
Cursor-->>CLI: serialized cursor payload
sequenceDiagram
participant Facade as chapter/import/digest
participant Serial as SerialGeneration
participant Document as document store
participant Fragments as stored fragments
Facade->>Document: writeSerialSource(stream)
Document->>Fragments: persist fragments
Facade->>Serial: buildTopologyInto(serialId, options)
Serial->>Fragments: list stored fragments
Fragments-->>Serial: batches with startSentenceIndex
Serial-->>Facade: topology ready
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
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/archive/query/archive-view.ts (1)
2988-3074: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winThread
documentOrdersthrough evidence previews to avoid repeated queries.
hydrateFindHitEvidencefans out into preview helpers that each re-readdocument.serials.listDocumentOrders().listArchiveCollectionalready computes that map once, so pass it through tocreateSourceEvidencePreview/filterAndSortSourceEvidenceRangesByFtsQueryinstead of doing one DB query per hit.🤖 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 `@src/archive/query/archive-view.ts` around lines 2988 - 3074, hydrateFindHitEvidence is still causing repeated document order lookups inside the evidence preview path. Thread the precomputed documentOrders map from listArchiveCollection through hydrateFindHitEvidence into the preview helpers it calls, especially createMentionLinkEvidencePreview and createMentionEvidencePreview, and then pass it onward to createSourceEvidencePreview/filterAndSortSourceEvidenceRangesByFtsQuery so they stop re-reading document.serials.listDocumentOrders() per hit.
🧹 Nitpick comments (2)
src/facade/chapter-build.ts (1)
575-579: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
ensureCreated()runs after the firstcreateDraft().
fragments.ensureCreated()(Line 579) is invoked only afterserial.createDraft()(Line 575). If draft creation touches the fragments directory before it exists, this ordering is fragile. Consider ensuring the directory first.♻️ Suggested reorder
const fragments = new Fragments(documentPath); const serial = fragments.getSerial(chapterId); + await fragments.ensureCreated(); let draft = await serial.createDraft(); let draftWordsCount = 0; let hasSentences = false; - - await fragments.ensureCreated();🤖 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 `@src/facade/chapter-build.ts` around lines 575 - 579, The draft setup in chapter-build is ordering the fragment initialization too late: `serial.createDraft()` runs before `fragments.ensureCreated()`, which can be fragile if draft creation depends on the fragments directory existing. Update the setup flow so `fragments.ensureCreated()` is called before `serial.createDraft()`, keeping the rest of the draft initialization (`draftWordsCount`, `hasSentences`, and `draft`) unchanged.src/wikipage/cache.ts (1)
216-266: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnconditional
VACUUMon any expired-row deletion may be costly for the disambiguation cache.
VACUUMis triggered wheneverexpired > 0(Line 242-253), regardless of how much data is deleted relative to total DB size. Sincedisambiguation_cachestores full page text, this table can grow large, andVACUUMrewrites the entire file — meaning even a single expired row on a large cache triggers a full-file rewrite on every GC pass. Consider gatingVACUUMon a minimum freed-row ratio/size, or switching to SQLite's incremental vacuum mode to amortize this cost.♻️ Example: only vacuum when a meaningful fraction of rows were removed
- if (!context.dryRun && expired > 0) { + if (!context.dryRun && expired > 0 && expired / scanned >= 0.1) { await database.transaction(async () => {🤖 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 `@src/wikipage/cache.ts` around lines 216 - 266, The GC flow in runWikipageCacheGc unconditionally runs VACUUM whenever any expired rows are removed, which can cause expensive full-file rewrites for large wikipage caches. Update the logic around countExpiredWikipageCacheRows, the delete transaction, and the database.run("VACUUM") call so vacuuming only happens when a meaningful amount of data was reclaimed (for example by row/size threshold) or use SQLite incremental vacuum instead. Keep the existing delete behavior and result accounting in GcContext/GcJobResult intact.
🤖 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 `@src/cli/args.ts`:
- Around line 1010-1034: The archive reverse-flag validation is incomplete for
index URI parsing, allowing `wikg://<archive>/index get --reverse` to be
silently accepted and ignored. Update `parseArchiveIndexUriArguments()` to
explicitly validate `values.reverse` with `rejectArchiveBooleanFlag(action,
"--reverse", values.reverse, helpRoute)` so the flag is rejected for unsupported
index actions, and keep `rejectUnsupportedArchiveReverse()` aligned with the
same action handling in `rejectUnsupportedArchiveReverse()` and
`parseArchiveIndexUriArguments()`.
In `@src/wikipage/cache.ts`:
- Around line 255-261: The dry-run result in the cache GC path is reporting
actual freed bytes instead of candidate savings, so update the logic in the
cache cleanup flow around the `readFileSize`/return block to distinguish dry-run
from real execution. Use the same `removed`/`scanned` counters, but when dry-run
is enabled compute `freedBytes` as an estimate of the space that would be
reclaimed (for example by deriving it from the candidate removals rather than
comparing `beforeBytes` and `afterBytes`), while keeping the existing actual
before/after size calculation for non-dry-run runs. Make sure the returned
object from this method reports `freedBytes` consistently with the documented
candidate freed disk space behavior.
---
Outside diff comments:
In `@src/archive/query/archive-view.ts`:
- Around line 2988-3074: hydrateFindHitEvidence is still causing repeated
document order lookups inside the evidence preview path. Thread the precomputed
documentOrders map from listArchiveCollection through hydrateFindHitEvidence
into the preview helpers it calls, especially createMentionLinkEvidencePreview
and createMentionEvidencePreview, and then pass it onward to
createSourceEvidencePreview/filterAndSortSourceEvidenceRangesByFtsQuery so they
stop re-reading document.serials.listDocumentOrders() per hit.
---
Nitpick comments:
In `@src/facade/chapter-build.ts`:
- Around line 575-579: The draft setup in chapter-build is ordering the fragment
initialization too late: `serial.createDraft()` runs before
`fragments.ensureCreated()`, which can be fragile if draft creation depends on
the fragments directory existing. Update the setup flow so
`fragments.ensureCreated()` is called before `serial.createDraft()`, keeping the
rest of the draft initialization (`draftWordsCount`, `hasSentences`, and
`draft`) unchanged.
In `@src/wikipage/cache.ts`:
- Around line 216-266: The GC flow in runWikipageCacheGc unconditionally runs
VACUUM whenever any expired rows are removed, which can cause expensive
full-file rewrites for large wikipage caches. Update the logic around
countExpiredWikipageCacheRows, the delete transaction, and the
database.run("VACUUM") call so vacuuming only happens when a meaningful amount
of data was reclaimed (for example by row/size threshold) or use SQLite
incremental vacuum instead. Keep the existing delete behavior and result
accounting in GcContext/GcJobResult intact.
🪄 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: CHILL
Plan: Pro
Run ID: 814e90dc-7c06-40dc-8c45-45fdf72834dc
📒 Files selected for processing (36)
data/help/commands/gc.jinjadata/help/commands/predicate.jinjadata/help/commands/uri.jinjadata/help/topics/format.jinjadata/help/topics/readiness.jinjadata/help/topics/recipe.jinjadata/help/topics/uri.jinjasrc/archive/query/archive-view.tssrc/archive/query/continuation-cursor.tssrc/cli/archive-index.tssrc/cli/archive.tssrc/cli/args.tssrc/cli/generation-planning.tssrc/cli/help.tssrc/cli/main.tssrc/cli/queue.tssrc/document/document.tssrc/document/schema.tssrc/document/stores.tssrc/document/types.tssrc/facade/chapter-build.tssrc/facade/chapter.tssrc/facade/digest.tssrc/facade/import.tssrc/gc/runner.tssrc/serial.tssrc/wikipage/cache.tssrc/wikipage/index.tstest/archive/query/archive-view.test.tstest/cli/archive.test.tstest/cli/args.test.tstest/cli/main.test.tstest/cli/queue.test.tstest/facade/chapter-graph.test.tstest/gc/gc.test.tstest/serial.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/legacy-sdpub/upgrade.ts (1)
947-958: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse SQLite identifier quoting instead of
JSON.stringify.
JSON.stringify(tableName)is not SQL identifier escaping, so this helper can generate malformed PRAGMA SQL for table names containing quotes. Keep the helper safe if it is reused beyond the current"serials"literal.Proposed hardening
+function quoteSqlIdentifier(identifier: string): string { + return `"${identifier.replace(/"/g, '""')}"`; +} + async function listTableColumns( database: Database, tableName: string, ): Promise<ReadonlySet<string>> { const columns = await database.queryAll( - `PRAGMA table_info(${JSON.stringify(tableName)})`, + `PRAGMA table_info(${quoteSqlIdentifier(tableName)})`, undefined, (row) => String(row.name), );🤖 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 `@src/legacy-sdpub/upgrade.ts` around lines 947 - 958, The listTableColumns helper currently builds PRAGMA SQL with JSON.stringify(tableName), which is not valid SQLite identifier quoting and can break for table names containing quotes. Update listTableColumns to use proper SQLite identifier escaping/quoting when constructing the PRAGMA table_info query, keeping the logic in database.queryAll safe even if tableName is not a fixed literal like "serials".
🤖 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.
Nitpick comments:
In `@src/legacy-sdpub/upgrade.ts`:
- Around line 947-958: The listTableColumns helper currently builds PRAGMA SQL
with JSON.stringify(tableName), which is not valid SQLite identifier quoting and
can break for table names containing quotes. Update listTableColumns to use
proper SQLite identifier escaping/quoting when constructing the PRAGMA
table_info query, keeping the logic in database.queryAll safe even if tableName
is not a fixed literal like "serials".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4fd8518-3270-45e9-88a8-8972d94feb88
📒 Files selected for processing (8)
src/archive/query/archive-view.tssrc/cli/archive.tssrc/cli/queue.tssrc/document/schema.tssrc/legacy-sdpub/upgrade.tstest/cli/archive.test.tstest/document/stores.test.tstest/document/wiki-graph-schema.test.ts
💤 Files with no reviewable changes (1)
- src/document/schema.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/cli/archive.test.ts
- src/cli/archive.ts
- src/archive/query/archive-view.ts
Moskize91
changed the title
This was referenced Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tests