feat(sync): incremental pull — fetch only changed pages - #6
Merged
Conversation
Closes #5. The tree walk (buildTree) already carries every page's real last_edited_time and costs seconds; the block fetch is the expensive part. Diff the tree against the index before fetching: - planSync classifies each page: skip only when last_edited, title, and computed path all match the index AND the file exists on disk; anything else (edited, renamed, moved via ancestor rename, missing, new) re-fetches. fetchBlocksFiltered (already used by --pages) fetches just the changed set. - The writer takes skipIds: skipped pages keep their bytes, still reserve filenames for sibling dedup, and still appear in results (written: false) so index bookkeeping and stale-removal stay correct. - The index now records each page's REAL last_edited_time instead of the sync time (engine + partial) — the stamping bug that made incremental diffing impossible. Pre-incremental indexes match nothing, so the first run after upgrading re-fetches everything once and heals. - sync --force restores unconditional re-fetch. Also fixes a latent writer bug the new tests exposed: uniqueFilename compared bare filenames against a set of full paths, so same-title sibling leaves never deduped and silently overwrote each other; dedup now runs on the full path (per-directory uniqueness). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe CLI adds Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Contributor
Author
|
Live validation against stance.ai's real mirror (221 pages, scratch copy, real
Marking ready for review. |
vizkid
marked this pull request as ready for review
August 3, 2026 08:22
![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
🧹 Nitpick comments (3)
src/sync/__tests__/incremental.test.ts (1)
31-119: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSolid planner coverage.
One gap: no test asserts that a page missing from the tree but present in the index becomes stale under mixed dash formats. That is the
findStalePagesnormalization change, and it touches file deletion. A regression there deletes live docs.🧪 Suggested extra test
it("treats a dashed index key as live when the tree uses the undashed id", () => { const dashed = "2c3e4ea0-0265-8010-980c-d9e2fab0643e"; const undashed = dashed.replace(/-/g, ""); // exercise findStalePages via an exported helper or sync-level test // expect: no stale entry for `dashed` when results contain `undashed` });🤖 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/sync/__tests__/incremental.test.ts` around lines 31 - 119, Add regression coverage for findStalePages to verify dashed and undashed page IDs are normalized before stale detection: when the index contains the dashed ID and the tree contains the equivalent undashed ID, the entry must not be marked stale or deleted. Exercise the exported helper or sync-level path and assert no stale result for the live dashed entry.src/markdown/writer.ts (1)
93-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood catch on the full-path dedup.
usedFilenamesstores full paths, so the old bare-filename compare never matched. Both passes now agree, which keepscomputeLinkMapandwritePageTreeconsistent.One nit: the leaf path logic is now duplicated in
buildLinkMapandwritePageRecursive. A sharedleafPath(dirPath, title, used)helper would stop the two passes drifting again.Also applies to: 150-150
🤖 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/markdown/writer.ts` around lines 93 - 96, Extract the duplicated leaf-path construction from buildLinkMap and writePageRecursive into a shared leafPath(dirPath, title, used) helper. Have both call it with the same directory, page title, and used-path set, preserving slugification, “.md” suffixing, and full-path deduplication.src/sync/engine.ts (1)
171-195: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winIndex keys drift between formats across runs.
pagesis keyed byresult.pageId, which is the tree node ID.partial.tskeys entries by the format already present in the index. So a full sync and a partial sync can write two entries for the same page in different dash formats.
findStalePagesnormalizes, so no file is wrongly deleted. But the index can hold duplicate entries. Normalize the key here to keep one canonical format.♻️ Normalize index keys
- for (const [pageId, result] of results) { + for (const [rawPageId, result] of results) { + const pageId = normalizeId(rawPageId); if (result.written) {🤖 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/sync/engine.ts` around lines 171 - 195, Normalize each page ID before assigning entries in the pages map within the index-update loop, using the same canonical format as lookupPageState and partial sync. Keep the existing PageState selection and lastEdited behavior unchanged, but replace the raw result/page ID key so full and partial syncs cannot create duplicate entries for one page.
🤖 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/sync/__tests__/incremental.test.ts`:
- Around line 23-25: Reformat the node helper declaration in the node function
so it complies with the repository’s oxfmt line-width rules. Run bun run format
or oxfmt without --check and commit the formatter’s output, without changing the
helper’s behavior.
In `@src/sync/engine.ts`:
- Around line 55-91: Update planSync to account for path changes of existing
pages: identify pages whose computed path differs from oldState.path, then
ensure pages that could contain links to those moved or renamed pages are not
left in unchangedIds (the minimal safe behavior is to promote all otherwise
unchanged pages to changed). Preserve newCount and normal unchanged
classification when no page path has moved.
---
Nitpick comments:
In `@src/markdown/writer.ts`:
- Around line 93-96: Extract the duplicated leaf-path construction from
buildLinkMap and writePageRecursive into a shared leafPath(dirPath, title, used)
helper. Have both call it with the same directory, page title, and used-path
set, preserving slugification, “.md” suffixing, and full-path deduplication.
In `@src/sync/__tests__/incremental.test.ts`:
- Around line 31-119: Add regression coverage for findStalePages to verify
dashed and undashed page IDs are normalized before stale detection: when the
index contains the dashed ID and the tree contains the equivalent undashed ID,
the entry must not be marked stale or deleted. Exercise the exported helper or
sync-level path and assert no stale result for the live dashed entry.
In `@src/sync/engine.ts`:
- Around line 171-195: Normalize each page ID before assigning entries in the
pages map within the index-update loop, using the same canonical format as
lookupPageState and partial sync. Keep the existing PageState selection and
lastEdited behavior unchanged, but replace the raw result/page ID key so full
and partial syncs cannot create duplicate entries for one page.
🪄 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: 220c62ed-f8de-4758-949a-5ca32640e2ab
📒 Files selected for processing (5)
src/cli.tssrc/markdown/writer.tssrc/sync/__tests__/incremental.test.tssrc/sync/engine.tssrc/sync/partial.ts
A skipped page's rendered content embeds relative paths to the pages it links to, so it goes stale when a target is renamed/moved (or a previously-unresolvable target now exists). The writer now records each page's outgoing notion:// link IDs in the index (PageState.links), and the planner demotes an otherwise-unchanged page to changed when any of its targets' paths differ from last sync. Absent on pre-incremental indexes, which re-fetch everything once anyway. Also fixes the CI formatting failure on the new test file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
Both findings addressed: formatting fixed, and the stale-outgoing-links edge is closed properly — the index now records each page's outgoing link IDs ( @coderabbitai resolve |
|
✅ Action performedComments resolved and changes approved. |
NikhilVerma
approved these changes
Aug 4, 2026
NikhilVerma
pushed a commit
that referenced
this pull request
Aug 4, 2026
Real-workspace testing against a live Notion fixture tree surfaced two pruning bugs from Phase 3: - shouldTraverseExcludedNode checked "does any include-id exist in the source" globally, forcing traversal into every excluded subtree (e.g. an unrelated Archive folder) whenever the source had ANY include-id override configured elsewhere. - resolveNodeDecision had no ancestor-exclusion cascade, so a sibling of a buried include-override target (with no selector match of its own) silently defaulted back to "include" once its excluded parent was traversed. Fix: - resolveNodeDecision takes an ancestorExcluded flag; an unmatched node under an excluded ancestor now defaults to "exclude" instead of "include". - shouldTraverseExcludedNode + a new shared, mutable pendingIncludeIds set (computePendingIncludeIds) scope id-based override search to targets not yet found, so traversal stops once every configured include id has actually been located. - PageNode gets an `excluded` flag; fetchAllBlocks skips block-fetch for excluded nodes, and writer.ts skips writing an excluded node's own content while still writing non-excluded descendants nested under its directory. Verified against a real Notion workspace fixture tree (multi-source, id exclude, glob exclude via defaultExclude, buried-keeper include-override, sibling non-leak, idempotent re-run). Autonomous decisions: - Excluded-but-traversed ancestor still creates its directory (for descendant paths) but writes no index.md of its own — matches Decision #6's steer toward a separate sources[] entry for genuinely clean output, this is the fallback path for buried keepers. - Skipped a full fake-Client tree.ts integration test in favor of the real E2E Notion run (no existing client-mock infra in this repo) — selector.ts unit tests cover the pure logic, the live run covers end-to-end wiring.
NikhilVerma
added a commit
that referenced
this pull request
Aug 4, 2026
…ltering Merged with conflict resolution against the incremental-sync work (#6) that landed on main after this PR was opened: - writer.ts: adapted excluded-node handling to the new options-object writePageRecursive signature. - engine.ts: kept both incremental planning (force/planSync) and config-driven options (rootPageId/selectors/maxDepth); excluded nodes are filtered out of the sync plan since they're never written/indexed. - tree.ts: fetchBlocksFiltered now skips excluded nodes, matching fetchAllBlocks, so excluded-but-traversed pages don't burn API calls. - cli.ts: explicit --pages opts out of config auto-discovery so the flag isn't silently ignored when a notion-rsync.config.json exists. Dropped from the contribution (not part of the feature): - .cursor/environment.json (contributor's personal Cursor setup hook that executes a script cloned from an external repo) - .gitignore un-ignore of docs/ (docs/ is the default sync output dir) - docs/config-file-support-plan.md (fork-internal planning doc) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6 tasks
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
2 participants
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.
Closes #5.
What
syncnow diffs the page tree against the index before fetching content, and only fetches pages that actually changed. The tree walk (buildTree) already carries every page's reallast_edited_timeand costs seconds — the block fetch is the expensive part, and it now runs only for the changed set via the existingfetchBlocksFiltered.Skip rule (
planSync, pure + unit-tested): a page is skipped only when itslast_edited_time, title, and computed path all match the index, and the file exists on disk. Edited, renamed, moved (ancestor rename shifts the whole subtree's paths), missing, or new pages all re-fetch.sync --forcerestores unconditional re-fetch.The stamping bug: the index recorded
lastEdited: new Date()— the sync time — making incremental diffing impossible (every entry in a real index carries the identical timestamp). It now records the page's reallast_edited_time(engine + partial sync). Pre-incremental indexes match nothing, so the first run after upgrading re-fetches everything once and heals the index; no migration needed.Writer: takes
skipIds— skipped pages keep their bytes, still reserve filenames for sibling dedup, and still appear in results (written: false) so index bookkeeping and stale-file removal stay correct.Latent bug found by the new tests
uniqueFilenamecompared bare filenames against a set of full paths, so same-title sibling leaves never deduped — the second silently overwrote the first. Dedup now runs on the full path (uniqueness per directory). Covered by a regression test.Tests
bun test: 142 pass / 0 fail (10 new: 8 planner scenarios incl. the pre-incremental-index migration path, plus writer skip-behavior tests against a real temp dir — sentinel bytes survive, mtime untouched, sibling dedup preserved,computeLinkMappaths proven identical towritePageTreeoutput).oxlintandtsc --noEmitclean.Draft until
Live validation against stance.ai's real 219-page mirror (heal run → no-op run timing) — blocked momentarily on a 1Password unlock; numbers will be posted here before marking ready.
Summary by CodeRabbit
New Features
Bug Fixes