Repository navigation
feat(cloud-sync): move the workspace sync engine into @deepnote/cloud-sync - #568
Conversation
…-sync
`@deepnote/cloud-sync` now exports `syncWorkspace(options)`. The caller passes
the root, token and API URL explicitly, supplies a conflict policy ('skip',
'override' or an async function) and receives progress through `onEvent`.
The library never reads `process`, `.env` or a TTY.
`deepnote sync` is a thin adapter over it (`runSync`): it resolves the root,
loads `.env`, resolves the token, turns `--on-conflict` into a policy (asking
through @inquirer/prompts on a terminal) and renders the events and result.
Flags, output, `-o json` and exit codes are unchanged.
The engine tests move to cloud-sync and are converted to the library API; the
CLI gets new adapter tests. A rejecting conflict policy cancels the run at the
existing checkpoints and `syncWorkspace` rejects with that same error.
Closes #563
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NZwreo9KDxDk4VuSXgzmC2
dinohamzic
left a comment
dinohamzic
left a comment
There was a problem hiding this comment.
The move makes sense, nothing blocking code wise, all READMEs also have no outdated references or content as a consequence of the move.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe workspace synchronization engine moves into Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant deepnoteSync
participant runSync
participant syncWorkspace
participant conflictPolicy
participant eventHandler
deepnoteSync->>runSync: pass sync arguments
runSync->>syncWorkspace: pass options and callbacks
syncWorkspace->>conflictPolicy: request conflict decision
conflictPolicy-->>syncWorkspace: return skip or override
syncWorkspace->>eventHandler: emit progress event
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The sync engine moves into 🚥 Pre-merge checks | ✅ 4 | ❌ 1 | ❓ 1❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)Full details: Docstring CoverageExplanation Docstring coverage is 76.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 4 files. (3 skipped: 3 unsupported.) Full details: Updates DocsExplanation The OSS repository documentation is updated:
Warning Some tools did not complete. Review the errors below. 🔧 LanguageToolLanguageTool checks were skipped: reviews.tools.languagetool.enabled_only requires at least one selection in enabled_rules or enabled_categories. Select rules/categories, set enabled_only to false to use the default rules, or set enabled to false to disable LanguageTool. 🔧 Biome (2.5.14)packages/cli/src/commands/sync.tsBiome could not lint this file: configuration resulted in errors. Check the repository's Biome configuration and plugins. packages/cloud-sync/package.jsonBiome could not lint this file: configuration resulted in errors. Check the repository's Biome configuration and plugins. packages/cloud-sync/src/index.tsBiome could not lint this file: configuration resulted in errors. Check the repository's Biome configuration and plugins.
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
2 participants
Summary
Closes #563.
deepnote synccould only be used as a CLI command: its engine resolved the root fromprocess.cwd(), loaded.envintoprocess.env, prompted through@inquirer/promptsand printed through the CLI output module. This moves the engine into@deepnote/cloud-syncassyncWorkspace(options), so other consumers (the local MCP server in #544, then the editor extension) can call it.deepnote syncbecomes a thin adapter over it.CLI behavior is unchanged: no change to flags, output,
-o jsonor exit codes.cli.tsis untouched.Changes
packages/cloud-sync/src/sync-workspace.ts(new): the engine, ported fromcommands/sync.ts, behind an explicit contract:WorkspaceSyncOptions(rootDir,baseUrl,token, flags,concurrency,onConflict,onEvent),WorkspaceSyncResult,SyncConflict(five kinds),SyncConflictPolicy('skip' | 'override' | async function) andSyncEvent(listing-projects,project-outcome,warning,file-transferred). It has noprocess.reference and no import fromcommander,chalk,ora,@inquirer/prompts,dotenvor@deepnote/cli.throwIfCancelledcheckpoints, a final manifest save is attempted (ignored if it fails), andsyncWorkspacerejects with that same error once every worker has settled. The CLI'sisPromptExitcheck is gone.concurrencythat is not a positive integer rejects withRangeErrorbeforerootDiris created or the API is called. The CLI keepsparseSyncConcurrencyand its message.packages/cli/src/commands/sync.ts: now the adapter.runSync(dir, options)resolves the root, creates it unless dry run, loads.env, resolves the token, picks the policy (ask+ TTY → aselectprompt viadescribeConflict;askwithout a TTY or under-o→'skip'plus the existing debug line) and maps events tolog/warn/debug.DEFAULT_SYNC_CONCURRENCYis re-exported socli.tsneeds no change.sync.test.tsmoves topackages/cloud-sync/src/sync-workspace.test.tswithgit mvand is converted to the library API (names and expected values unchanged;select/TTY mocks andconsoleassertions replaced by anonConflictvi.fnand collectedSyncEvents). New engine tests cover the cases listed in Move the workspace sync engine into @deepnote/cloud-sync #563.packages/cli/src/commands/sync.test.tsis a new adapter suite that mocks onlysyncWorkspace.@deepnote/blocksand@deepnote/database-integrationsadded to@deepnote/cloud-syncdependencies andexternalintsdown.config.ts;fflatemoved from the CLI's to cloud-sync'sdevDependencies. The lockfile diff contains only those changes.packages/cloud-sync/README.mddocumentssyncWorkspace(options, conflicts, events, cancellation). The pointer tocanonicalProjectHashinpackages/cloud/docs/project-import-contract.mdnow names its new location.Things to check
toDecisionis not in Move the workspace sync engine into @deepnote/cloud-sync #563. The library treats any policy answer other than the exact string'override'as'skip'. Without it, an untyped caller returningundefinedortruewould act asoverrideat the destructivepushProjectsites. Typed callers see no difference. It is documented onSyncConflictPolicyand tested.resolveConflict(the original delivered, then cleared).onEventis now consumer code; a throwing listener would otherwise leave the hold stuck.index.tsexports in Move the workspace sync engine into @deepnote/cloud-sync #563's removal list are removed (planProjectPaths,pathsOverlap,PlannedProjectPaths). The other six still have importers outside the package (packages/cli/src/utils/publish-mirror.ts,publish-streamlit-app.ts), so Move the workspace sync engine into @deepnote/cloud-sync #563's own condition ("whengrepfinds no importer") does not hold for them.skip); the rest keep their names and expected values. The new CLI adapter suite has 41 tests.importProjectwas sent skips the follow-upwriteProjectNotebooks(checkpoint atsync-workspace.ts:427), so the manifest baseline is not advanced and the next sync can report a false conflict. Worth a follow-up now that a library caller can trigger it.Refusing to prune…error still mentions--url, kept byte-identical so CLI output does not change. The behavior whenonEventitself throws is unspecified.Verification
Run on this branch:
pnpm typecheck,pnpm build,pnpm prettier:checkandpnpm spell-checkexit 0.pnpm biome:checkexits 0; its 8noConsolewarnings are inexamples/local-runner/*/serve.mjs, which this change does not touch.pnpm vitest run --bail 0: 186 files, 3447 tests, all passed, run with apythonsymlink onPATH. Without it, 3 tests inpackages/cli/src/commands/run.test.tsfail (they expectpythonEnv: 'python', and this host only haspython3); they pass with the symlink and fail the same way onmainper feat(cloud-sync): add @deepnote/cloud-sync with sync manifest and path modules #554.deepnote syncwith no token exits 2 withMissing authentication token.and empty stdout;--concurrency 0is still rejected by the CLI parser.fetch) and interactive TTY prompting by hand (covered by the mocked adapter tests).The change was reviewed with the
code-review-typescriptskill over three rounds; the findings that held up (missing tests for the policy contract,projectDirvsprojectName, flush order, final-save failure, and an exhaustiveness check on the event switch) are fixed in this PR.🤖 Generated with Claude Code
https://claude.ai/code/session_01NZwreo9KDxDk4VuSXgzmC2
Summary by CodeRabbit