feat: config-file multi-source sync with selectors and date filtering - #4
Merged
Conversation
Document sources[] include/exclude design for upstream Phase 2.6 selective sync; un-ignore docs/ so planning lives in-repo.
Autonomous decisions: - Name resolution verifies configured `name` against the Notion page title for each source `id`, and rejects duplicate names mapping to different ids — no workspace search yet, but bad/ambiguous names fail at load. - Stub orchestrator always prints the resolved plan; non-dry-run warns that multi-source sync is not implemented yet — Phase 1 preview only, no pull. - `defaultExclude` is merged into per-source exclude selectors at load time — matches load.ts scope for effective selector computation ahead of Phase 4 wiring.
Autonomous decisions: - Auto-init per-source index via ensureSyncIndex — config-driven runs should not require manual init per subdir - Sequential for-loop over sources — avoids parallel Notion API rate-limit blowups per plan - Injectable syncSource hook — keeps orchestration testable without mocking the Notion client
Autonomous decisions: - Keep defaultExclude separate from source exclude in EffectiveSelectors — required for correct precedence (source exclude glob beats defaultExclude glob) - Inline glob-to-regexp matcher in selector.ts — no new dependency; trailing /** matches the folder node itself, not just descendants - Include-override via shouldTraverseExcludedNode — excluded parents still fetch children when an include id or descendant-matching glob requires it
Autonomous decisions: - Module-level setters for concurrency/retry instead of threading through engine.ts — keeps Phase 4 scoped; single-root sync keeps built-in defaults - Dry-run shows merged effective exclude rather than separate exclude/defaultExclude lines — clearer fully-resolved plan
Autonomous decisions: - Config section after Commands — keeps Quick Start simple; advanced users find schema/options where CLI docs live - Example config uses planning-doc placeholder IDs — documents multi-source + exclude patterns without binding to a real workspace - Phase 2.6 checked only for selective sync — watch mode, backup, and rollback remain out of scope per plan
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 nonfx#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.
Use fs.readFile instead of Bun.file for global npm installs, and resolve titles for database roots during config load.
Notion linked database blocks cannot be retrieved via databases.retrieve; skip them instead of aborting multi-source sync.
Autonomous decisions: - Date.parse for validation — matches plan spec and accepts any ISO-8601 string the runtime parses, including date-only and full timestamps - Shared parseDateFilter helper for source and default fields — avoids duplicated validation logic while keeping path-specific error labels - Allow empty dateFilter objects — both after/before are optional per type; no runtime effect until Phase 2 wires intersection
Autonomous decisions: - intersectDateFilters kept private — intersection is exercised via computeEffectiveSelectors and resolveConfig tests only - defaultDateFilter surfaced on ResolvedConfig alongside defaultExclude — dry-run plan prints both global default and per-source effective range - Intersection uses Date.parse comparison on original ISO strings — preserves config formatting while picking later after / earlier before
Autonomous decisions: - Inclusive calendar-day bounds: after uses start-of-day, before uses end-of-day UTC - applyDateExclusion helper ORs date into excluded only; pruneChildren unchanged - tree.test.ts mocks client.ts via mock.module for integration coverage
Autonomous decisions: - Mock Notion client at file level (matching tree.test.ts) so sync() runs end-to-end without network - Use root edited 2026-07-01 and leaf edited 2026-06-15 so tightening after to 2026-06-20 excludes only the leaf while root stays in range - Dynamic-import engine/tree after mock.module so findStalePages cleanup is exercised via real sync(), not a unit test of the private helper
Document dateFilter/defaultDateFilter in config plan, README, and example config. Includes oxfmt pass on Phase 1-4 source files so check passes. Autonomous decisions: - Added date filtering section to README schema table and example JSON - Updated config-file-support-plan.md status to Implemented - Included oxfmt formatting fixes from prior phases in this commit
WalkthroughAdds config-driven multi-source Notion syncing with schema validation, title resolution, selector and date filtering, and dry-run planning. Tree construction prunes excluded subtrees while preserving explicitly included descendants, with configurable concurrency and retries. Sync supports per-source roots, outputs, indexes, and depth limits. CLI, documentation, examples, changelog, and tests are updated. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 5
🧹 Nitpick comments (5)
docs/config-file-support-plan.md (1)
31-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced dependency-chain block.
markdownlint flags this fence as MD040; use
textfor the diagram input.🤖 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 `@docs/config-file-support-plan.md` around lines 31 - 43, Update the fenced dependency-chain block in the documentation to specify the text language, preserving the diagram content unchanged.Source: Linters/SAST tools
README.md (1)
209-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument Claude API usage patterns.
Add a concise Claude API usage/example section, or link to a canonical guide. As per coding guidelines, “Document Claude API usage patterns and examples in project documentation.”
🤖 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 `@README.md` around lines 209 - 219, Update README.md by adding a concise section documenting Claude API usage patterns and examples, or link to the project’s canonical Claude API guide. Place it near the existing configuration documentation and keep the guidance focused on practical usage.Source: Coding guidelines
src/config/selector.ts (1)
63-166: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRecompiling regexes + re-partitioning per node = wasteful on big trees.
partitionSelectorsruns on everyresolveNodeDecision/shouldTraverseExcludedNodecall, andglobToRegExprecompiles each glob→RegExp for every node visited. On a large workspace that's O(nodes × globs) regex compiles. Precompute partitioned selectors once + cache compiled RegExp per pattern.Static analysis also flagged ReDoS at Line 110 (regex from variable). Patterns come from trusted config, so risk is low, but caching + a small
Map<string, RegExp>addresses both.🤖 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/config/selector.ts` around lines 63 - 166, Cache compiled glob regular expressions by pattern and reuse them from matchGlob instead of recompiling via globToRegExp for every node. Pre-partition EffectiveSelectors once per selection/traversal operation and pass or reuse that partitioned result in resolveNodeDecision and shouldTraverseExcludedNode, preserving the existing precedence and matching behavior.Source: Linters/SAST tools
src/notion/tree.ts (1)
254-261: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftPass task factories into
runWithConcurrency
childPromisesare already started here, sotreeConcurrencyonly throttles awaiting, not API request start. Same pattern exists in the other tree/block walkers too. SwitchrunWithConcurrencyto accept() => Promise<T>factories and start them inside the limiter.🤖 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/notion/tree.ts` around lines 254 - 261, Update runWithConcurrency and all tree/block walker call sites to accept Promise-returning task factories, starting each factory only after the concurrency limiter grants execution. In the current childPromises flow around buildPageTree and buildDatabaseTree, replace eagerly invoked promises with deferred callbacks and pass those factories directly to runWithConcurrency, preserving filtering and traversal behavior.src/notion/__tests__/tree.test.ts (1)
12-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared Notion-client test harness. Same
MockPage/makePage/maps/dateSelectors+mock.modulestub duplicated across both files, already drifting (engine addsfetchBlocks/createNotionClient). Pull into one helper to stop the drift.
src/notion/__tests__/tree.test.ts#L12-L82: importMockPage,makePage,dateSelectors, and the client-mock factory from the shared helper.src/sync/__tests__/engine.test.ts#L17-L93: same, extending the shared mock factory withfetchBlocks/createNotionClientoverrides instead of re-declaring.🤖 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/notion/__tests__/tree.test.ts` around lines 12 - 82, Extract the duplicated Notion test harness into a shared helper. In src/notion/__tests__/tree.test.ts lines 12-82, replace the local MockPage, makePage, maps, dateSelectors, and mock.module setup with imports and the shared client-mock factory. In src/sync/__tests__/engine.test.ts lines 17-93, make the same replacement and extend the shared mock factory with fetchBlocks and createNotionClient overrides, preserving each test’s existing behavior.
🤖 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 `@docs/config-file-support-plan.md`:
- Around line 3-6: Update the Scope statement in the config-file support plan to
remove the stale claim that TODO.md Phase 2.6 is currently unchecked, while
preserving the existing implemented status and scope details.
In `@src/cli.ts`:
- Around line 139-146: The sync command currently reaches syncFromConfig only
for explicit --config values, so default config discovery is skipped. In
src/cli.ts lines 139-146, route sync through the cwd default config when it
exists while preserving the legacy single-root behavior when it does not; update
the surrounding sync option handling without changing explicit config behavior.
In README.md lines 167-173, document the precise final auto-discovery behavior,
including the default config path and fallback behavior.
In `@src/config/schema.ts`:
- Around line 211-232: Update the per-source validation around the output value
in the source parsing function to reject absolute paths and any path traversal
that escapes the configured output root, while preserving valid subdirectory
paths. Track normalized output paths across sources and reject duplicates before
returning the source configuration, ensuring comparisons use the normalized
form.
- Around line 145-147: Update isValidDateString to validate the input against
the expected ISO-8601 lexical format before calling Date.parse, rejecting
formats such as slash-separated or month-name dates while preserving the
existing invalid-date rejection.
In `@src/config/selector.ts`:
- Around line 51-57: Update validateConfig and the selector construction
returning defaultExcludeGlobs so id-shaped defaultExclude values are either
rejected during validation or partitioned into an ID collection handled by
resolveNodeDecision; ensure no accepted IDs are silently passed only to
matchGlob.
---
Nitpick comments:
In `@docs/config-file-support-plan.md`:
- Around line 31-43: Update the fenced dependency-chain block in the
documentation to specify the text language, preserving the diagram content
unchanged.
In `@README.md`:
- Around line 209-219: Update README.md by adding a concise section documenting
Claude API usage patterns and examples, or link to the project’s canonical
Claude API guide. Place it near the existing configuration documentation and
keep the guidance focused on practical usage.
In `@src/config/selector.ts`:
- Around line 63-166: Cache compiled glob regular expressions by pattern and
reuse them from matchGlob instead of recompiling via globToRegExp for every
node. Pre-partition EffectiveSelectors once per selection/traversal operation
and pass or reuse that partitioned result in resolveNodeDecision and
shouldTraverseExcludedNode, preserving the existing precedence and matching
behavior.
In `@src/notion/__tests__/tree.test.ts`:
- Around line 12-82: Extract the duplicated Notion test harness into a shared
helper. In src/notion/__tests__/tree.test.ts lines 12-82, replace the local
MockPage, makePage, maps, dateSelectors, and mock.module setup with imports and
the shared client-mock factory. In src/sync/__tests__/engine.test.ts lines
17-93, make the same replacement and extend the shared mock factory with
fetchBlocks and createNotionClient overrides, preserving each test’s existing
behavior.
In `@src/notion/tree.ts`:
- Around line 254-261: Update runWithConcurrency and all tree/block walker call
sites to accept Promise-returning task factories, starting each factory only
after the concurrency limiter grants execution. In the current childPromises
flow around buildPageTree and buildDatabaseTree, replace eagerly invoked
promises with deferred callbacks and pass those factories directly to
runWithConcurrency, preserving filtering and traversal behavior.
🪄 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: 3c015694-186a-43de-8a8a-70a0772c4a35
📒 Files selected for processing (19)
.gitignoreCHANGELOG.mdREADME.mdTODO.mddocs/config-file-support-plan.mdnotion-rsync.config.example.jsonsrc/cli.tssrc/config/__tests__/load.test.tssrc/config/__tests__/schema.test.tssrc/config/__tests__/selector.test.tssrc/config/load.tssrc/config/schema.tssrc/config/selector.tssrc/markdown/writer.tssrc/notion/__tests__/tree.test.tssrc/notion/client.tssrc/notion/tree.tssrc/sync/__tests__/engine.test.tssrc/sync/engine.ts
💤 Files with no reviewable changes (1)
- .gitignore
- cli: fall back to ./notion-rsync.config.json when --config is omitted - schema: enforce ISO-8601 dateFilter strings, reject absolute/traversing or duplicate source output paths, reject id selectors in defaultExclude - docs: drop stale 'currently unchecked' status note
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cli.ts (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReconcile config-file probing with the file-I/O runtime contract.
existsSyncperforms file I/O, but the applicable rule requiresBun.file()/Bun.write()insrc/**/*.ts. Since this PR also adds Node runtime support, verify the supported runtime and use a compatible abstraction—or document an approved exception—before merging.As per coding guidelines,
src/**/*.tsmust useBun.file()andBun.write()for file I/O operations.Also applies to: 140-142
🤖 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/cli.ts` at line 8, Replace the existsSync-based config probing in the CLI flow with the approved runtime-compatible file abstraction, using Bun.file() where supported and the Node-compatible equivalent only if required by the documented runtime contract. Update the related config-file handling around the affected lines consistently, or document the approved exception if existsSync must remain.Source: Coding guidelines
🤖 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/cli.ts`:
- Line 8: Replace the existsSync-based config probing in the CLI flow with the
approved runtime-compatible file abstraction, using Bun.file() where supported
and the Node-compatible equivalent only if required by the documented runtime
contract. Update the related config-file handling around the affected lines
consistently, or document the approved exception if existsSync must remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e588fb2-1a10-4407-b606-28e358ebd1c9
📒 Files selected for processing (4)
README.mddocs/config-file-support-plan.mdsrc/cli.tssrc/config/schema.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- README.md
- docs/config-file-support-plan.md
- src/config/schema.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.cursor/environment.json:
- Around line 2-3: Pin the repository used by the install command in the
environment configuration to a reviewed tag or commit instead of cloning the
moving default branch. Verify the fetched revision matches the expected pinned
value before executing the tooling installer, while preserving the existing
repositoryDependencies entry and installation flow.
🪄 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 Plus
Run ID: f7cc20f1-1516-42e7-a5c6-f1d20fc553cd
📒 Files selected for processing (1)
.cursor/environment.json
Comment on lines
+2
to
+3
| "repositoryDependencies": ["github.com/crimsonsunset/cursor-config"], | ||
| "install": "rm -rf /tmp/cursor-config && git clone --depth 1 https://github.com/crimsonsunset/cursor-config.git /tmp/cursor-config && node /tmp/cursor-config/scripts/tooling/install.js" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-remote https://github.com/crimsonsunset/cursor-config.git HEAD
# Compare the returned commit with the reviewed commit before updating this file.Repository: nonfx/notion-sync
Length of output: 271
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if [ -f .cursor/environment.json ]; then
echo "--- .cursor/environment.json ---"
cat -n .cursor/environment.json
else
echo ".cursor/environment.json not found"
fi
printf '\n--- git ls-remote all refs for repository ---\n'
git ls-remote https://github.com/crimsonsunset/cursor-config.git || true
printf '\n--- try public JSON endpoints ---\n'
curl -fsSL https://api.github.com/repos/crimsonsunset/cursor-config | jq '{name,private,updated_at,latest_release}' || true
curl -fsSL https://raw.githubusercontent.com/crimsonsunset/cursor-config/main/scripts/tooling/install.js | sed -n '1,20p' || trueRepository: nonfx/notion-sync
Length of output: 772
Pin the remote dependency before executing it.
This clones the repository and immediately runs install.js without a pinned tag/commit, checksum, or provenance check. A compromised or changed upstream can execute arbitrary code during environment setup. Use a reviewed tag or commit and verify it before running the installer.
🤖 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 @.cursor/environment.json around lines 2 - 3, Pin the repository used by the
install command in the environment configuration to a reviewed tag or commit
instead of cloning the moving default branch. Verify the fetched revision
matches the expected pinned value before executing the tooling installer, while
preserving the existing repositoryDependencies entry and installation flow.
This was referenced Aug 3, 2026
Contributor
|
Thanks @crimsonsunset — this is a great contribution. The build-time pruning design (vs the post-filter Merged in aac9916 with a manual conflict resolution, since #6 (incremental pull — the same full-crawl scaling issue you flagged in your follow-up note) landed on main after you opened this. Notes on what changed during the merge:
Since #6 already added incremental block-fetch skipping via |
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.
Summary
Adds config-file-driven sync so a single run can pull multiple Notion roots (pages or databases) into different output directories, each with its own include/exclude selectors — plus a date filter on
last_edited_timeso a source can be scoped to recently-edited content only. Both features were designed via planning docs before implementation (linked below) and verified against a real multi-source personal Notion workspace, not just unit tests.Changes overview
Planned scope: config-file multi-source sync
src/config/schema.ts,src/config/load.ts): JSON config withsources[], per-sourceoutput,include/excludeselectors (Notion ID or glob), and global defaults that intersect with per-source overrides.src/cli.ts,src/sync/engine.ts): one CLI invocation syncs every configured source sequentially, each with its own independent sync index.src/notion/tree.ts,src/config/selector.ts): excluded subtrees are pruned during the crawl — not fetched at all — unless a descendant is explicitly re-included, in which case traversal continues without ever fetching the excluded ancestor's own content.Planned scope: date filtering
dateFilter: { after, before }(global default + per-source override, intersected) excludes pages bylast_edited_time, inclusive on both boundaries.findStalePages/removeStaleFilesinengine.tsneeded no changes: pages that fall out of date-filter scope on a re-sync are already treated as stale and cleaned up correctly.Added while implementing (discovered, not originally planned)
e412ed6) instead of throwing —databases.retrievedoesn't support linked-database views, and this surfaced immediately on a real workspace.1a88b11) — the original config schema assumed page-only roots; real usage needed database roots too.779c145) — fixed cascading exclusion leaking into unrelated branches when an include-override was nested under an excluded ancestor.Key technical decisions
last_edited_time(not a title/property date) since it's the one timestamp Notion guarantees is accurate and queryable on every page/database.Test plan
bun test)dateFilter(valid dates, malformed strings,after>before)last_edited_timeand ancestor paths for the specific pages expected to pass/fail each filterDocs
docs/config-file-support-plan.md— multi-source config design + phase breakdownREADME.mdandnotion-rsync.config.example.json— updated with the new config shape and date filter examplesCHANGELOG.md— updatedFollow-up (not in this PR)
Testing this PR's date filter against a real workspace surfaced a separate scaling issue worth flagging: every sync run does a full recursive crawl + full block-content fetch with no incremental skip, and the crawl concurrency limiter (
runWithConcurrency, cap of 2) throttles simultaneity, not requests/sec — so it doesn't actually keep the client under Notion's 3 req/sec average, which caused repeated 429s during testing on a content-heavy workspace. Planning a follow-up PR for: (1) an interval-based rate limiter ahead ofwithRetry()inclient.tsinstead of relying on concurrency alone, and (2) caching each page's reallast_edited_timein the sync index so re-syncs skip block-fetching unchanged pages entirely.Summary by CodeRabbit
New Features
--config(or auto-discoverednotion-rsync.config.json) with per-source include/exclude selectors, date filtering, max depth, concurrency/retry settings, and dry-run resolved plan output.Bug Fixes
Documentation
Tests
Chores
docs/.