Fix auto-sync reconcile cadence#157
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors periodic full-reconcile scheduling in startAutoSync to use a health-aware timeout scheduler: adds ChangesHealth-aware periodic reconcile scheduling
Sequence DiagramsequenceDiagram
participant startAutoSync
participant WatcherSubscriptions
participant PeriodicScheduler
participant Reconcile
participant onError
startAutoSync->>WatcherSubscriptions: start mount & project subscribe
WatcherSubscriptions->>startAutoSync: settle (success or failure)
alt both healthy
startAutoSync->>PeriodicScheduler: schedule(healthyScanIntervalMs)
else degraded
startAutoSync->>PeriodicScheduler: schedule(scanIntervalMs)
startAutoSync->>onError: forward subscription error
end
PeriodicScheduler->>Reconcile: setTimeout -> run reconcile
Reconcile->>PeriodicScheduler: reschedule next tick
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Relayfile Eval ReviewRun: Passed: 4 | Needs human: 0 | Reviewable: 0 | Missing output: 0 | Failed: 0 | Skipped: 0 Human Review CasesNo reviewable human-review cases captured Relayfile output. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/local-mount/src/auto-sync.ts`:
- Around line 258-266: markWatcherDegraded currently forces rescheduling on
every error which can repeatedly push the safety reconcile out; change
markWatcherDegraded to only call reschedulePeriodicReconcile when the degraded
state transitions from false to true (i.e., if (!watcherDegraded) {
watcherDegraded = true; reschedulePeriodicReconcile(); }) and otherwise just
call onError(err); keep references to markWatcherDegraded,
reschedulePeriodicReconcile, watcherDegraded and periodicReconcileRunning when
making the change.
- Around line 97-107: The normalizeScanInterval function currently allows any
finite non-negative number; enforce the setTimeout maximum by rejecting values >
2_147_483_647 (2**31-1) so callers don't inadvertently pass an overflowed delay
that becomes 1ms; inside normalizeScanInterval (function name) after computing
interval, treat 0 and Infinity as before, then if interval > 2147483647 throw a
RangeError (similar to the existing RangeError message) so only values within
[0, 2147483647] or Infinity are accepted.
🪄 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: 00ae8060-dbb3-4506-923a-7b727a84708b
📒 Files selected for processing (5)
packages/local-mount/CHANGELOG.mdpackages/local-mount/README.mdpackages/local-mount/src/auto-sync-scheduler.test.tspackages/local-mount/src/auto-sync.tspackages/local-mount/src/mount.ts
|
Addressed the two CodeRabbit findings in 1b4e73f: repeated watcher errors now only reschedule on the first transition to degraded, and scan intervals above Node's setTimeout maximum are rejected with regression coverage. |
Summary
Closes #135
Verification
Note: root npm run test currently passes core, SDK, and local-mount, then fails in packages/file-observer because jsdom is not installed; that package is not touched by this PR and is not part of the current CI workflow.