diff --git a/ARCHITECTURE-DECISIONS.md b/ARCHITECTURE-DECISIONS.md index 3d4bc70b18..14dd61a02f 100644 --- a/ARCHITECTURE-DECISIONS.md +++ b/ARCHITECTURE-DECISIONS.md @@ -69,6 +69,55 @@ This document tracks significant architectural decisions and patterns in the Sup --- +### 3. Sync Package Boundary Direction + +**Status**: ✅ Active (since May 2026) + +**Decision**: Operation-log sync code is split by dependency direction: +`src/app` composes host-specific wiring, `@sp/sync-providers` owns bundled +provider implementations, and `@sp/sync-core` owns framework-agnostic reusable +sync primitives. + +**Rationale**: + +- Keeps reusable sync algorithms independent of Angular, NgRx, app models, and + provider implementations +- Prevents provider IDs, app action/entity enums, validation schemas, UI, OAuth, + and platform bridges from leaking into the core engine package +- Gives boundary lint a clear rule: packages never import app code, and + providers consume only public sync-core exports + +**Implementation**: + +- ESLint rejects Angular, NgRx, app, shared-schema, sync-core deep imports, and + dynamic imports inside package sources +- `@sp/sync-core` has no runtime dependencies and owns vector-clock algorithms + used by client/server compatibility paths +- `packages/shared-schema` compatibility-re-exports generic vector-clock + algorithms from `@sp/sync-core`; `@sp/sync-core` must not import + `@sp/shared-schema` +- `@sp/sync-providers` depends on public `@sp/sync-core` plus provider runtime + helpers, while app factories inject credentials, platform bridges, validators, + OAuth routing, and config + +**Documentation**: [`docs/sync-and-op-log/package-boundaries.md`](docs/sync-and-op-log/package-boundaries.md) + +**Key Files**: + +- [`packages/sync-core/src/index.ts`](packages/sync-core/src/index.ts) - Core public API +- [`packages/sync-providers/src/index.ts`](packages/sync-providers/src/index.ts) - Provider public API +- [`eslint.config.js`](eslint.config.js) - Package boundary enforcement +- [`src/app/op-log/sync-providers/sync-providers.factory.ts`](src/app/op-log/sync-providers/sync-providers.factory.ts) - App-side provider composition + +**When to Update This Pattern**: + +- Moving sync code between app and packages +- Adding a package export or dependency +- Adding a provider implementation or plugin-facing provider contract +- Changing vector-clock ownership or shared-schema compatibility + +--- + ## How to Use This Document ### When Making Architectural Changes diff --git a/docs/long-term-plans/sync-core-extraction-plan.md b/docs/long-term-plans/sync-core-extraction-plan.md index af98a296d3..c4a2c542e2 100644 --- a/docs/long-term-plans/sync-core-extraction-plan.md +++ b/docs/long-term-plans/sync-core-extraction-plan.md @@ -9,7 +9,11 @@ > envelope types, PKCE helpers, the native-HTTP retry helpers, the shared > provider error classes, and the bundled Dropbox, WebDAV + Nextcloud, > SuperSync, and LocalFile providers behind injected platform/credential/file -> ports. Next up: PR 6 final boundary hardening and documentation.** +> ports. PR 6 final boundary hardening is present, including boundary +> lint/grep, manifest and public-export audit, a package-boundary architecture +> note, and package build/test verification. PR 7 optional polish is present: +> PKCE is package-owned, the deprecated provider alias is retired, and duplicate +> package-boundary lint patterns are trimmed.** **Goal:** Carve the sync engine out of `src/app/op-log/` into a reusable, framework-agnostic, **domain-agnostic** `@sp/sync-core` package, plus a sibling @@ -99,20 +103,28 @@ first slice: ## Current Branch Snapshot -The branch already contains the first package boundary and part of the PR 2 -groundwork: +The branch now contains the current package boundary for this extraction slice: - `packages/sync-core/` exists and is exposed through the `@sp/sync-core` path alias. -- `npm run sync-core:build` runs the package build, and `prepare` builds - `sync-core`, `shared-schema`, then `plugin-api`. +- `packages/sync-providers/` exists and is exposed through the + `@sp/sync-providers` path alias. +- `npm run sync-core:build` and `npm run sync-providers:build` run the package + builds, and `prepare` builds `sync-core`, `sync-providers`, `shared-schema`, + then `plugin-api`. - `eslint.config.js` applies `no-restricted-imports` and a dynamic-import ban to - `packages/sync-core/**/*.ts`. -- The package currently exports operation primitives, apply types, LWW helper - factory, full-state op-type helper factory, entity-key helpers, - host-configured sync-file prefix helpers, generic error-message helpers, - `SyncStateCorruptedError`, entity-registry contracts, and the privacy-aware - logger port. + `packages/sync-core/**/*.ts` and `packages/sync-providers/**/*.ts`. +- `@sp/sync-core` currently exports generic operation/apply primitives, + vector-clock algorithms, full-state op-type helper factories, entity-key and + sync-file-prefix helpers, compression/error helpers, import-filter decisions, + conflict-resolution helpers, upload/download/replay/remote-apply planning + helpers, entity-registry contracts, app-side orchestration ports, + `SyncStateCorruptedError`, and the privacy-aware logger port. +- `@sp/sync-providers` currently exports provider-neutral contracts, provider + ports, file-based sync envelope types, provider-shared errors, PKCE and retry + helpers, safe logging helpers, provider-owned string constants, and the + bundled Dropbox, WebDAV + Nextcloud, SuperSync, and LocalFile provider + classes. - The app registry now has `buildEntityRegistry()` and an `ENTITY_REGISTRY` injection token. Existing helper functions still read the app-side `ENTITY_CONFIGS` singleton for compatibility. @@ -214,9 +226,9 @@ Suggested next order: 3. Treat the current PR 4a/4b/4c port, small-helper, and replay-coordinator slices as complete for this round; keep the Angular `OperationApplierService` shell app-side unless a later port proves another small extraction safe. -4. Treat PR 5 provider lift as complete for this branch. Continue with PR 6: - final boundary hardening, manifest/public-export audits, and a short - architecture note for package dependency direction. +4. Treat PR 5 provider lift and PR 6 final boundary hardening as complete for + this branch. The next work is optional post-provider-lift polish or merge + verification outside this extraction slice. ## PR 1 - Thin First Slice (#7546) @@ -298,9 +310,10 @@ Each previously-public symbol path keeps working via thin shims: - Update the PR description if it still says `action-types.enum.ts` or `provider.const.ts` moved into `@sp/sync-core`; the code correctly keeps them app-side. -- Fix comments that imply `sync-core` depends on `shared-schema`. The build may - run after `shared-schema`, but the package dependency direction must remain - absent. +- Resolved in PR 3a: vector-clock ownership moved to `@sp/sync-core`, and + `packages/shared-schema` now compatibility-re-exports those algorithms from + `@sp/sync-core`. The forbidden direction remains + `@sp/sync-core -> @sp/shared-schema`. - Resolved in follow-up: `FULL_STATE_OP_TYPES` is now app-configured via `createFullStateOpTypeHelpers()`. @@ -1053,7 +1066,7 @@ Shipped as two commits behind a shared design doc (`docs/plans/2026-05-12-pr5-dropbox-slice.md`) plus a post-review cleanup pass: -- **PR 5a — provider error classes.** Twelve provider-shared error +- **Dropbox slice A — provider error classes.** Twelve provider-shared error classes (`AuthFailSPError`, `InvalidDataSPError`, `HttpNotOkAPIError`, `NoRevAPIError`, `RemoteFileNotFoundAPIError`, `MissingCredentialsSPError`, `MissingRefreshTokenAPIError`, @@ -1079,7 +1092,7 @@ cleanup pass: passed raw `Authorization` headers through `additionalLog`. - Package gained `"sideEffects": false` so consumers that only import error classes can tree-shake through the barrel. -- **PR 5b — Dropbox provider proper.** `Dropbox`, `DropboxApi`, and +- **Dropbox slice B — Dropbox provider proper.** `Dropbox`, `DropboxApi`, and `DropboxFileMetadata` moved into `packages/sync-providers/src/file-based/dropbox/` behind three new injected ports: @@ -1140,12 +1153,12 @@ a shared design doc (`docs/plans/2026-05-12-pr5-webdav-slice.md`) with multi-review consensus, followed by tightening commits that fold post-review findings: -- **PR 6a — shared log helpers.** Promoted `errorMeta(e, extra)` and +- **WebDAV slice A — shared log helpers.** Promoted `errorMeta(e, extra)` and `urlPathOnly(url)` from `dropbox-api.ts:88-104` into `packages/sync-providers/src/log/error-meta.ts` so WebDAV could adopt them without copy-paste. Exported from the package barrel. Dropbox imports updated. No behavior change. -- **PR 6b — WebDAV + Nextcloud provider proper.** `webdav-base-provider.ts`, +- **WebDAV slice B — WebDAV + Nextcloud provider proper.** `webdav-base-provider.ts`, `webdav-api.ts`, `webdav-xml-parser.ts`, `webdav-http-adapter.ts`, `webdav.const.ts`, `webdav.model.ts`, `webdav.ts`, `nextcloud.ts`, and `nextcloud.model.ts` (plus their co-located specs) moved into @@ -1205,7 +1218,9 @@ browsers`, W2) restored "Failed to fetch" / "NetworkError" / no-retry behavior keeps the slice scope a refactor. Trivially addable later as a per-call-site `maxRetries` argument on the reused `NativeHttpExecutor` port. -- **Spec count delta.** Package spec count went from 103 (post PR 5b) to 177. PR 6b added 53 webdav specs (Jasmine → Vitest one-to-one move). +- **Spec count delta.** Package spec count went from 103 (post Dropbox slice B) + to 177. WebDAV slice B added 53 webdav specs (Jasmine → Vitest one-to-one + move). Two follow-up commits added 13 namespace/server-format specs (the `getElementsByTagNameNS('*', name)` PROPFIND parsing path, `:href` variants, server-format compatibility — `restore WebDAV @@ -1243,7 +1258,7 @@ Shipped behind the shared design doc (`docs/plans/2026-05-12-pr7-super-sync-slice.md`) with multi-review consensus, then tightened by follow-up commits from review findings: -- **PR 7a - retryable upload helper.** Promoted the broad-pattern +- **SuperSync slice A - retryable upload helper.** Promoted the broad-pattern operation-upload retry predicate from `src/app/op-log/sync/sync-error-utils.ts` into `packages/sync-providers/src/http/retryable-upload-error.ts` as @@ -1252,7 +1267,7 @@ consensus, then tightened by follow-up commits from review findings: `operation-log-upload.service.ts` kept its import surface unchanged. This helper remains distinct from the native-code-aware `isTransientNetworkError` in `native-http-retry.ts`. -- **PR 7b - SuperSync provider proper.** `super-sync.ts`, +- **SuperSync slice B - SuperSync provider proper.** `super-sync.ts`, `super-sync.model.ts`, and the SuperSync provider spec moved into `packages/sync-providers/src/super-sync/`. The spec was converted from Jasmine to Vitest. `SyncProviderId.SuperSync` was replaced in @@ -1339,15 +1354,30 @@ Shipped the LocalFile final slice: ### Remaining Slice Plan -PR 5 is complete for this branch. Continue with PR 6 final boundary -hardening: +PR 5 and PR 6 final boundary hardening are complete for this branch: -1. Recheck package boundary rules and forbidden import greps for both - `sync-core` and `sync-providers`. -2. Audit manifests/runtime deps now that all bundled providers moved. -3. Audit the `@sp/sync-providers` public barrel for app-owned concepts and - consider whether tiered exports should remain polish-only. -4. Add the short package-boundary architecture note described in PR 6. +- Boundary rules were rechecked with direct ESLint on + `packages/sync-core/**/*.ts` and `packages/sync-providers/**/*.ts`; the + forbidden-import grep for Angular, NgRx, `src/app`, `@sp/shared-schema`, and + sync-core deep imports returned no source matches. +- Manifest/runtime dependency audit: `@sp/sync-core` has no runtime + dependencies and now declares `"sideEffects": false`; `@sp/sync-providers` + runtime deps remain limited to public `@sp/sync-core` plus `hash-wasm`, with + `@xmldom/xmldom` test-only in `devDependencies`. +- Public barrel audit: `@sp/sync-providers` exports provider classes, + provider-owned string constants, contracts, ports, and shared helpers, but no + app-owned `SyncProviderId`, provider lists, OAuth routing, UI config, storage + prefixes, or `@sp/shared-schema` validators. `@sp/sync-core` still carries + the explicitly-deprecated full-state op compatibility exports and + host-defined `OpType.SyncImport` / `BackupImport` / `Repair` strings noted + above; reusable hosts should use `createFullStateOpTypeHelpers()` instead of + those defaults. Tiered provider exports remain polish-only until size or + consumer pressure justifies the manifest surface. +- Added `docs/sync-and-op-log/package-boundaries.md`, linked it from the sync + docs README, and recorded the boundary direction in `ARCHITECTURE-DECISIONS.md`. + +Remaining merge gates outside this extraction slice: selected provider E2E +smoke tests as needed. ### Verification @@ -1371,51 +1401,83 @@ This is now a final audit rather than the first boundary rule. - Add a small architecture note that explains the package boundaries and allowed dependency direction. +### Current State + +Status: implemented for this branch. + +- `docs/sync-and-op-log/package-boundaries.md` documents the allowed dependency + direction: app composition may consume both packages, + `@sp/sync-providers` may consume only public `@sp/sync-core`, and + `@sp/sync-core` stays independent of Angular, NgRx, app code, + `@sp/shared-schema`, and provider implementations. +- `docs/sync-and-op-log/README.md` now points to the package-boundary note and + its key-file section reflects the current package/app split rather than the + pre-extraction app-local provider layout. +- `ARCHITECTURE-DECISIONS.md` records the package boundary direction as an + active architectural decision. +- `packages/sync-core/package.json` now declares `"sideEffects": false`, + matching `@sp/sync-providers` and the package's no-runtime-side-effect + surface. +- Manifest and public-export audit found no app-owned provider enums, provider + lists, storage prefixes, OAuth routing, UI config, shared-schema validators, + or sync-core deep imports in package public APIs. The known exception remains + the deprecated `@sp/sync-core` full-state op compatibility exports plus the + host-defined `OpType.SyncImport` / `BackupImport` / `Repair` strings retained + for existing consumers. + ### Verification -- `npm run lint`. -- Boundary grep for both packages. -- Package builds from a clean install. -- Full app unit tests and selected sync E2E. +- `npm ci` completed, including the prepare-time builds for sync-core, + sync-providers, shared-schema, and plugin-api. +- `npx eslint "packages/sync-core/**/*.ts" "packages/sync-providers/**/*.ts"` + passed. +- `npm run lint` passed. +- Boundary grep for both packages returned no forbidden source imports. +- `npm run sync-core:build` passed. +- `npm run sync-providers:build` passed. +- Local run on May 13, 2026: `npm run packages:test` passed with sync-core 156 + tests and sync-providers 302 tests. +- Full app unit tests passed in the PR 7 verification below; selected sync E2E + remain merge-level verification as needed. --- ## Optional Polish (Post-Provider Lift) -Non-blocking cleanups surfaced during the PR 5 provider lift. None of these -change behaviour or boundaries — they remove duplication, tighten tests, and -retire deprecated aliases once consumers have migrated. +Status: implemented for this branch. -### Candidates +Non-blocking cleanups surfaced during the PR 5 provider lift. These do not +change behaviour or boundaries; they remove duplication, tighten tests, and +retire deprecated aliases after consumers have migrated. -- **Consolidate PKCE helpers.** `packages/sync-providers/src/pkce.ts` currently - duplicates `src/app/util/pkce.util.ts`. The package needs to stand alone, so - the duplication is intentional during the scaffold, but the remaining - consumers (`src/app/plugins/oauth/plugin-oauth.service.ts`, - `src/app/plugins/oauth/pkce.util.spec.ts`, - `src/app/op-log/sync-providers/file-based/dropbox/dropbox-auth-helper.spec.ts`) - should migrate onto `@sp/sync-providers` so `src/app/util/pkce.util.ts` can be - deleted. Drift between the two implementations is the risk this resolves. -- **Drop the dead `_length` arg in `generatePKCECodes`.** Pre-existing from the - original helper; the parameter is unused. Either remove it (single call site) - or document why it is kept for API compatibility. -- **Tighten the PKCE verifier-length assertion.** `tests/pkce.spec.ts` bounds - the verifier with `toBeLessThanOrEqual(128)`, but a 32-byte random buffer - always encodes to exactly 43 base64url chars. Replace with an exact length - check so the test actually constrains the output. -- **Retire `SyncProviderServiceInterface` alias.** Marked `@deprecated` in - `src/app/op-log/sync-providers/provider.interface.ts`. Sweep callers to - `SyncProviderBase` / `FileSyncProvider` and remove the alias. -- **Trim duplicate ESLint pattern depths.** The `packages/sync-providers/**` - block in `eslint.config.js` lists `../sync-core/**`, `../../sync-core/**`, and - `**/sync-core/**` (plus shared-schema equivalents). The `**/...` form already - covers the relative variants; collapse for readability. +### Current State + +- PKCE is package-owned: app OAuth code imports `generateCodeVerifier` and + `generateCodeChallenge` from `@sp/sync-providers`, and the app-local + `pkce.util` shim/spec/helper were removed. +- The dead `_length` parameter was removed from `generatePKCECodes()`, and the + Dropbox call site now uses the zero-argument helper. +- Package PKCE tests now assert the exact 43-character verifier length produced + by the default 32 random bytes. +- The deprecated `SyncProviderServiceInterface` alias was removed. File-provider + references now use `FileSyncProvider`, and generic/operation-capable provider + references now use `SyncProviderBase` plus `OperationSyncCapable` where + needed. +- The `packages/sync-providers/**` ESLint boundary override no longer repeats + relative sync-core/shared-schema pattern depths already covered by the + `**/...` forms. ### Verification -- `npm run lint`, `npm test`, `npm run packages:test`. -- Grep for `pkce.util` and `SyncProviderServiceInterface` after the cleanup — - both should return zero hits outside of `packages/sync-providers`. +- `npm run packages:test` passed with sync-core 156 tests and sync-providers + 302 tests. +- `npm run sync-core:build` passed. +- `npm run sync-providers:build` passed. +- `npm test` passed, including the full Karma run and the Los Angeles timezone + run. +- `npm run lint` passed. +- Source grep for `pkce.util` and `SyncProviderServiceInterface` returned zero + hits under `src` and `packages`. --- @@ -1432,7 +1494,7 @@ retire deprecated aliases once consumers have migrated. | **4c** | Revisit `OperationApplierService` extraction | High | Narrow replay coordinator present | | **5** | Lift providers into `@sp/sync-providers` | Medium-High | Provider deps stay out of core | | **6** | Final boundary hardening and architecture note | Low | Audit and lock down | -| **7** | Optional polish: dedupe PKCE, retire deprecated aliases | Low | Non-blocking cleanup | +| **7** | Optional polish: dedupe PKCE, retire deprecated aliases | Low | Present on branch | After the final PR, `@sp/sync-core` should be the domain-agnostic sync engine and abstractions, `@sp/sync-providers` should contain bundled provider diff --git a/docs/long-term-plans/sync-provider-plugins.md b/docs/long-term-plans/sync-provider-plugins.md index 6fb8f6865c..f483cdd8c8 100644 --- a/docs/long-term-plans/sync-provider-plugins.md +++ b/docs/long-term-plans/sync-provider-plugins.md @@ -73,13 +73,13 @@ Same constraints as `persistDataSynced()` (1 MB limit, rate limiting), but data ### 3. PluginSyncProviderAdapter -New class in `src/app/plugins/` that wraps plugin callbacks into `SyncProviderServiceInterface`: +New class in `src/app/plugins/` that wraps plugin callbacks into `FileSyncProvider`: ``` src/app/plugins/plugin-sync-provider-adapter.ts ``` -- Implements `SyncProviderServiceInterface` +- Implements `FileSyncProvider` - Delegates file operations to plugin callbacks via `PluginBridgeService` - `privateCfg` uses a no-op credential store (plugin manages its own creds) - `isReady()` delegates to the plugin's `isReady()` callback @@ -144,7 +144,7 @@ Changes: | `src/app/plugins/plugin-api.ts` | Implement new API methods | | `src/app/plugins/plugin-bridge.service.ts` | Add bridge methods for sync provider registration and local data persistence | | `src/app/plugins/plugin-cleanup.service.ts` | Unregister sync provider on plugin disable/unload | -| **New**: `src/app/plugins/plugin-sync-provider-adapter.ts` | Adapter wrapping plugin callbacks → `SyncProviderServiceInterface` | +| **New**: `src/app/plugins/plugin-sync-provider-adapter.ts` | Adapter wrapping plugin callbacks → `FileSyncProvider` | | `src/app/op-log/sync-providers/provider-manager.service.ts` | Add `registerPluginProvider()` / `unregisterPluginProvider()`, dynamic provider list | | `src/app/op-log/sync-providers/provider.const.ts` | Support dynamic plugin provider IDs alongside the enum | | `src/app/features/config/form-cfgs/sync-form.const.ts` | Dynamic provider dropdown, "Configure" button for plugin providers | @@ -155,7 +155,7 @@ Changes: **`src/app/plugins/plugin-sync-provider-adapter.ts`** -Thin adapter that implements `SyncProviderServiceInterface` by delegating to plugin callbacks. ~50-80 lines. +Thin adapter that implements `FileSyncProvider` by delegating to plugin callbacks. ~50-80 lines. ## Verification Plan diff --git a/docs/sync-and-op-log/README.md b/docs/sync-and-op-log/README.md index 370a018446..07fda89ea5 100644 --- a/docs/sync-and-op-log/README.md +++ b/docs/sync-and-op-log/README.md @@ -11,6 +11,7 @@ This directory contains the architectural documentation for Super Productivity's | Understand the overall architecture | [operation-log-architecture.md](./operation-log-architecture.md) | | See visual diagrams | [diagrams/](./diagrams/) (split by topic) | | Learn the design rules | [operation-rules.md](./operation-rules.md) | +| Understand package boundaries | [package-boundaries.md](./package-boundaries.md) | | Understand file-based sync | [diagrams/04-file-based-sync.md](./diagrams/04-file-based-sync.md) | | Understand SuperSync encryption | [supersync-encryption-architecture.md](./supersync-encryption-architecture.md) | @@ -23,6 +24,7 @@ This directory contains the architectural documentation for Super Productivity's | [operation-log-architecture.md](./operation-log-architecture.md) | Comprehensive architecture reference covering Parts A-F: Local Persistence, File-Based Sync, Server Sync, Validation & Repair, Smart Archive Handling, and Atomic State Consistency | Active | | [diagrams/](./diagrams/) | Mermaid diagrams split by topic (local persistence, server sync, file-based sync, etc.) | Active | | [operation-rules.md](./operation-rules.md) | Design rules and guidelines for the operation log store and operations | Active | +| [package-boundaries.md](./package-boundaries.md) | Dependency direction and ownership boundaries for `@sp/sync-core`, `@sp/sync-providers`, and app sync wiring | Active | ### Sync Architecture @@ -117,20 +119,33 @@ This prevents duplicate side effects when syncing operations from other clients. ## Key Files -### Sync Providers +### Sync Packages + +``` +packages/sync-core/src/ +├── operation.types.ts # Generic operation primitives +├── vector-clock.ts # Compare/merge/prune algorithms +├── conflict-resolution.ts # Pure conflict helpers +├── replay-coordinator.ts # Generic remote replay ordering +└── ports.ts # App-side orchestration contracts + +packages/sync-providers/src/ +├── super-sync/ # SuperSync provider implementation +├── file-based/dropbox/ # Dropbox provider implementation +├── file-based/webdav/ # WebDAV + Nextcloud providers +├── file-based/local-file/ # LocalFile provider classes +└── provider.types.ts # Provider-neutral contracts +``` + +### App Sync Wiring ``` src/app/op-log/sync-providers/ -├── super-sync/ # SuperSync server provider -├── file-based/ # File-based providers -│ ├── file-based-sync-adapter.service.ts # Unified adapter for file providers -│ ├── file-based-sync.types.ts # FileBasedSyncData types -│ ├── webdav/ # WebDAV provider -│ ├── dropbox/ # Dropbox provider -│ └── local-file/ # Local file sync provider -├── provider-manager.service.ts # Provider activation/management +├── sync-providers.factory.ts # Composes app deps into package providers +├── credential-store.service.ts # OAuth/credential storage implementation ├── wrapped-provider.service.ts # Provider wrapper with encryption -└── credential-store.service.ts # OAuth/credential storage +├── file-based/ # File-based adapter + app shims +└── super-sync/ # SuperSync app shims + validators ``` ### Core Operation Log diff --git a/docs/sync-and-op-log/package-boundaries.md b/docs/sync-and-op-log/package-boundaries.md new file mode 100644 index 0000000000..d50a2634b2 --- /dev/null +++ b/docs/sync-and-op-log/package-boundaries.md @@ -0,0 +1,165 @@ +# Sync Package Boundaries + +**Status:** Active +**Last Updated:** May 13, 2026 + +This note documents the package split used by the operation-log sync stack. The +goal is to keep reusable sync logic framework-agnostic while leaving Super +Productivity domain wiring in the app. + +## Dependency Direction + +Allowed direction: + +```text +src/app + -> @sp/sync-providers + -> @sp/sync-core + +src/app + -> @sp/sync-core + +packages/shared-schema + -> @sp/sync-core +``` + +Rules: + +- `@sp/sync-core` must not import Angular, NgRx, `src/app`, `@sp/shared-schema`, + `@sp/sync-providers`, or provider-specific code. +- `@sp/sync-providers` may import only public `@sp/sync-core` exports. It must + not deep-import `@sp/sync-core/*`, Angular, NgRx, `src/app`, or + `@sp/shared-schema`. +- The app may import both packages and is responsible for Angular dependency + injection, NgRx, Electron/Capacitor bridges, config UI, OAuth routing, and + Super Productivity-specific model wiring. +- `packages/shared-schema` depends on `@sp/sync-core` only for compatibility + re-exports of generic vector-clock algorithms. + +The `packages/shared-schema -> @sp/sync-core` edge is deliberate compatibility +coupling. Vector-clock compare/merge/prune algorithms moved to `@sp/sync-core` +so sync-core, client wrappers, and server/shared consumers use one +implementation. `packages/shared-schema` re-exports those algorithms to preserve +legacy import paths while consumers migrate; do not add new sync-engine logic to +`@sp/shared-schema`, and remove the compatibility edge once no consumers need +it. + +## Ownership + +`@sp/sync-core` owns reusable sync-engine primitives: + +- generic operation and apply types; +- vector-clock compare, merge, and prune algorithms; +- pure conflict, import-filter, upload/download, replay, compression, and + prefix helpers; +- structural entity-registry contracts; +- app-facing port contracts and the privacy-aware `SyncLogger` interface. + +`@sp/sync-providers` owns bundled provider implementations and provider-neutral +contracts: + +- Dropbox, WebDAV, Nextcloud, SuperSync, and LocalFile provider classes; +- file-based sync envelope types and provider response contracts; +- provider-owned file envelope constants such as `sync-data.json` and + file-sync version keys; +- credential, file-adapter, platform-info, web-fetch, native-HTTP, storage, and + response-validator ports; +- provider-shared error classes, PKCE helpers, retry helpers, and safe logging + metadata helpers. + +Cross-provider utilities belong in `@sp/sync-providers` when they are reusable +by provider implementations but not by the generic engine. Existing examples are +provider-shared error classes, PKCE, retry predicates, native-HTTP retry, and +safe log-metadata helpers. + +New bundled providers should follow the Dropbox/WebDAV/SuperSync/LocalFile +pattern: put provider-owned protocol logic and provider-neutral contracts in +`@sp/sync-providers`, then compose app-only credentials, platform bridges, +validators, OAuth routing, and UI config in thin app-side factories. If a +provider is app-specific or plugin-provided rather than bundled, implement it +app-side against the provider contracts instead of widening the package surface. + +`src/app` owns host-specific configuration and choreography: + +- `ActionType`, `ENTITY_TYPES`, `SyncProviderId`, provider lists, and storage + prefixes such as `REMOTE_FILE_CONTENT_PREFIX` and `PRIVATE_CFG_PREFIX`; +- entity registry construction from feature reducers/selectors; +- wrapped full-state payload shape, import reasons, repair payloads, and + validation against `@sp/shared-schema`; +- Angular services, NgRx dispatch/replay conversion, local-action filtering, + hydration windows, archive side effects, provider factories, OAuth callbacks, + config dialogs, and platform bridge implementations. + +`packages/shared-schema` owns Super Productivity schema contracts and validators +that are shared between app and server. In this boundary it should stay +SP-coupled and should not become a dependency of `@sp/sync-core` or +`@sp/sync-providers`. + +## Public Exports + +Package consumers should import from package barrels only: + +```ts +import { compareVectorClocks } from '@sp/sync-core'; +import { Dropbox, PROVIDER_ID_DROPBOX } from '@sp/sync-providers'; +``` + +Do not import from package internals such as `@sp/sync-core/src/*`, +`@sp/sync-providers/src/*`, or `dist/*`. If a host needs a symbol, promote it to +the package barrel deliberately and check that it is not app-owned. + +The `@sp/sync-providers` barrel intentionally exports provider classes and +provider-owned string constants, but not app enums such as `SyncProviderId`. +Internal helpers such as WebDAV API/adapter classes stay unexported unless a +second host needs them. Tiered provider exports can be added later if bundle +size or consumer ergonomics justify the extra manifest surface. + +`@sp/sync-core` still exports deprecated full-state op compatibility defaults +and host-defined `OpType.SyncImport` / `BackupImport` / `Repair` strings for +existing consumers. New reusable hosts should provide their own full-state +operation strings through `createFullStateOpTypeHelpers()`. + +## Privacy Boundary + +Package logging must use `SyncLogger` and safe structured metadata only. +`SyncLogger` is a privacy-aware port shape; it does not sanitize arbitrary +metadata. Call sites are responsible for passing already-scrubbed values, and +enforcement is currently code review plus focused tests. + +IDs, counts, action strings, entity types, provider IDs, and error names/codes +are acceptable. URL metadata is acceptable only after the caller strips query +strings, fragments, credentials, tokens, raw response bodies, and user-provided +path segments such as file names, emails, share IDs, or folder names. Prefer +coarse path templates, provider operation names, host-only values, or a +provider-owned relative path category over raw URL paths. + +Full entities, operation payloads, task titles, note text, raw provider +responses, credentials, headers, and encryption material must stay out of +exportable logs. A lint rule for unsafe direct logging remains a possible +follow-up; until then, new movable/provider code should use `SyncLogger` and +tests should assert privacy-sensitive catch paths. + +## Tests + +The ESLint package-boundary overrides apply to all TypeScript files under +`packages/sync-core/**` and `packages/sync-providers/**`, including tests. Tests +may import their own package internals through relative paths for white-box +coverage. `@sp/sync-providers` tests may import public `@sp/sync-core` exports, +but should not import `@sp/sync-core` internals or sync-core test helpers. + +## Verification + +Before moving code across these boundaries, run: + +```bash +npm run lint +npm run sync-core:build +npm run sync-providers:build +npm run packages:test +``` + +For a quick boundary spot-check, use: + +```bash +rg -n "from ['\"](@angular|@ngrx|@sp/shared-schema|src/app|@sp/sync-core/src|@sp/sync-core/)|import\(['\"](@angular|@ngrx|@sp/shared-schema|src/app|@sp/sync-core/src|@sp/sync-core/)" packages/sync-core/src packages/sync-providers/src +``` diff --git a/eslint.config.js b/eslint.config.js index 1f9dfc0de7..972bf1ba09 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -186,14 +186,6 @@ module.exports = tseslint.config( '@ngrx/*', '@sp/shared-schema/*', '@sp/sync-core/*', - '../sync-core/*', - '../sync-core/**', - '../shared-schema/*', - '../shared-schema/**', - '../../shared-schema/*', - '../../shared-schema/**', - '../../sync-core/*', - '../../sync-core/**', '**/shared-schema/*', '**/shared-schema/**', '**/sync-core/*', diff --git a/packages/sync-core/package.json b/packages/sync-core/package.json index 271cd340a2..88c0aa5a7a 100644 --- a/packages/sync-core/package.json +++ b/packages/sync-core/package.json @@ -5,6 +5,7 @@ "main": "dist/index.js", "module": "dist/index.mjs", "types": "dist/index.d.ts", + "sideEffects": false, "exports": { ".": { "import": { diff --git a/packages/sync-providers/src/file-based/dropbox/dropbox.ts b/packages/sync-providers/src/file-based/dropbox/dropbox.ts index aa6361927d..2fc01c6987 100644 --- a/packages/sync-providers/src/file-based/dropbox/dropbox.ts +++ b/packages/sync-providers/src/file-based/dropbox/dropbox.ts @@ -328,7 +328,7 @@ export class Dropbox implements FileSyncProvider< // auth button) share one PKCE generation instead of racing. if (!this._pkcePromise) { const inFlight = (async () => { - const { codeVerifier, codeChallenge } = await generatePKCECodes(128); + const { codeVerifier, codeChallenge } = await generatePKCECodes(); let authCodeUrl = `${DROPBOX_AUTH_URL}` + `?response_type=code&client_id=${this._appKey}` + diff --git a/packages/sync-providers/src/pkce.ts b/packages/sync-providers/src/pkce.ts index b736b1be45..2f829637d7 100644 --- a/packages/sync-providers/src/pkce.ts +++ b/packages/sync-providers/src/pkce.ts @@ -108,7 +108,6 @@ export const generateCodeChallenge = async ( }; export const generatePKCECodes = async ( - _length: number, options: GeneratePkceCodesOptions = {}, ): Promise<{ codeVerifier: string; codeChallenge: string }> => { const codeVerifier = generateCodeVerifier(options); diff --git a/packages/sync-providers/tests/pkce.spec.ts b/packages/sync-providers/tests/pkce.spec.ts index 8bd03ecf60..446a6f3de3 100644 --- a/packages/sync-providers/tests/pkce.spec.ts +++ b/packages/sync-providers/tests/pkce.spec.ts @@ -20,8 +20,7 @@ describe('PKCE utilities', () => { const verifier = generateCodeVerifier({ crypto: createDeterministicCrypto() }); expect(verifier).toMatch(/^[A-Za-z0-9\-_]+$/); - expect(verifier.length).toBeGreaterThanOrEqual(43); - expect(verifier.length).toBeLessThanOrEqual(128); + expect(verifier.length).toBe(43); }); it('generates the RFC 7636 S256 challenge for a known verifier', async () => { @@ -66,7 +65,7 @@ describe('PKCE utilities', () => { }); it('generates a verifier and matching challenge pair', async () => { - const result = await generatePKCECodes(128, { + const result = await generatePKCECodes({ crypto: createDeterministicCrypto(), }); diff --git a/src/app/imex/sync/file-based-encryption.service.spec.ts b/src/app/imex/sync/file-based-encryption.service.spec.ts index 015134495b..fc35cef7f4 100644 --- a/src/app/imex/sync/file-based-encryption.service.spec.ts +++ b/src/app/imex/sync/file-based-encryption.service.spec.ts @@ -10,7 +10,10 @@ import { import { FileBasedSyncAdapterService } from '../../op-log/sync-providers/file-based/file-based-sync-adapter.service'; import { GlobalConfigService } from '../../features/config/global-config.service'; import { SyncProviderId } from '../../op-log/sync-providers/provider.const'; -import { SyncProviderServiceInterface } from '../../op-log/sync-providers/provider.interface'; +import { + FileSyncProvider, + SyncProviderBase, +} from '../../op-log/sync-providers/provider.interface'; describe('FileBasedEncryptionService', () => { let service: FileBasedEncryptionService; @@ -20,7 +23,7 @@ describe('FileBasedEncryptionService', () => { let mockClientIdProvider: jasmine.SpyObj; let mockFileBasedAdapter: jasmine.SpyObj; let mockGlobalConfigService: jasmine.SpyObj; - let mockProvider: SyncProviderServiceInterface; + let mockProvider: FileSyncProvider; let mockAdapter: { uploadSnapshot: jasmine.Spy; setLastServerSeq: jasmine.Spy; @@ -41,7 +44,7 @@ describe('FileBasedEncryptionService', () => { maxConcurrentRequests: 1, privateCfg: { load: jasmine.createSpy('load').and.resolveTo(mockExistingCfg), - } as unknown as SyncProviderServiceInterface['privateCfg'], + } as unknown as FileSyncProvider['privateCfg'], isReady: jasmine.createSpy('isReady').and.resolveTo(true), setPrivateCfg: jasmine.createSpy('setPrivateCfg').and.resolveTo(), getFileRev: jasmine.createSpy('getFileRev'), @@ -128,16 +131,12 @@ describe('FileBasedEncryptionService', () => { }); it('should throw for non-file-based provider (SuperSync)', async () => { - const superSyncProvider: SyncProviderServiceInterface = { + const superSyncProvider: SyncProviderBase = { id: SyncProviderId.SuperSync, maxConcurrentRequests: 1, privateCfg: {} as never, isReady: jasmine.createSpy('isReady').and.resolveTo(true), setPrivateCfg: jasmine.createSpy('setPrivateCfg'), - getFileRev: jasmine.createSpy('getFileRev'), - downloadFile: jasmine.createSpy('downloadFile'), - uploadFile: jasmine.createSpy('uploadFile'), - removeFile: jasmine.createSpy('removeFile'), }; mockProviderManager.getActiveProvider.and.returnValue(superSyncProvider); diff --git a/src/app/imex/sync/import-encryption-handler.service.spec.ts b/src/app/imex/sync/import-encryption-handler.service.spec.ts index a9cc72374b..aaa6b6a085 100644 --- a/src/app/imex/sync/import-encryption-handler.service.spec.ts +++ b/src/app/imex/sync/import-encryption-handler.service.spec.ts @@ -5,7 +5,7 @@ import { SyncProviderManager } from '../../op-log/sync-providers/provider-manage import { SyncProviderId } from '../../op-log/sync-providers/provider.const'; import { OperationSyncCapable, - SyncProviderServiceInterface, + SyncProviderBase, } from '../../op-log/sync-providers/provider.interface'; import { SuperSyncPrivateCfg } from '../../op-log/sync-providers/super-sync/super-sync.model'; import { AppDataComplete } from '../../op-log/model/model-config'; @@ -15,7 +15,7 @@ describe('ImportEncryptionHandlerService', () => { let mockSnapshotUploadService: jasmine.SpyObj; let mockProviderManager: jasmine.SpyObj; let mockSyncProvider: jasmine.SpyObj< - SyncProviderServiceInterface & OperationSyncCapable + SyncProviderBase & OperationSyncCapable >; const createMockImportedData = ( diff --git a/src/app/imex/sync/snapshot-upload.service.spec.ts b/src/app/imex/sync/snapshot-upload.service.spec.ts index 209cd47582..6f9557ac12 100644 --- a/src/app/imex/sync/snapshot-upload.service.spec.ts +++ b/src/app/imex/sync/snapshot-upload.service.spec.ts @@ -7,7 +7,7 @@ import { CLIENT_ID_PROVIDER } from '../../op-log/util/client-id.provider'; import { SyncProviderId } from '../../op-log/sync-providers/provider.const'; import { OperationSyncCapable, - SyncProviderServiceInterface, + SyncProviderBase, } from '../../op-log/sync-providers/provider.interface'; import { OperationEncryptionService } from '../../op-log/sync/operation-encryption.service'; import { SuperSyncPrivateCfg } from '../../op-log/sync-providers/super-sync/super-sync.model'; @@ -23,7 +23,7 @@ describe('SnapshotUploadService', () => { }; let mockEncryptionService: jasmine.SpyObj; let mockSyncProvider: jasmine.SpyObj< - SyncProviderServiceInterface & OperationSyncCapable + SyncProviderBase & OperationSyncCapable >; const mockExistingCfg: SuperSyncPrivateCfg = { diff --git a/src/app/imex/sync/supersync-encryption-toggle.service.spec.ts b/src/app/imex/sync/supersync-encryption-toggle.service.spec.ts index 3922d16d49..04a0367ad0 100644 --- a/src/app/imex/sync/supersync-encryption-toggle.service.spec.ts +++ b/src/app/imex/sync/supersync-encryption-toggle.service.spec.ts @@ -5,7 +5,7 @@ import { SyncProviderManager } from '../../op-log/sync-providers/provider-manage import { SyncProviderId } from '../../op-log/sync-providers/provider.const'; import { OperationSyncCapable, - SyncProviderServiceInterface, + SyncProviderBase, } from '../../op-log/sync-providers/provider.interface'; import { SuperSyncPrivateCfg } from '../../op-log/sync-providers/super-sync/super-sync.model'; @@ -14,7 +14,7 @@ describe('SuperSyncEncryptionToggleService', () => { let mockSnapshotUploadService: jasmine.SpyObj; let mockProviderManager: jasmine.SpyObj; let mockSyncProvider: jasmine.SpyObj< - SyncProviderServiceInterface & OperationSyncCapable + SyncProviderBase & OperationSyncCapable >; const mockExistingCfg: SuperSyncPrivateCfg = { diff --git a/src/app/op-log/sync-exports.ts b/src/app/op-log/sync-exports.ts index 915d2e81d1..0b7fd88018 100644 --- a/src/app/op-log/sync-exports.ts +++ b/src/app/op-log/sync-exports.ts @@ -56,7 +56,6 @@ export { export { SyncProviderBase, FileSyncProvider, - SyncProviderServiceInterface, isFileSyncProvider, } from './sync-providers/provider.interface'; diff --git a/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.spec.ts b/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.spec.ts index 095b6a798d..969ba0dbfb 100644 --- a/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.spec.ts +++ b/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.spec.ts @@ -2,7 +2,7 @@ import { TestBed } from '@angular/core/testing'; import { FileBasedSyncAdapterService } from './file-based-sync-adapter.service'; import { SyncProviderId } from '../provider.const'; import { - SyncProviderServiceInterface, + FileSyncProvider, OperationSyncCapable, SyncOperation, } from '../provider.interface'; @@ -21,7 +21,7 @@ import { StateSnapshotService } from '../../backup/state-snapshot.service'; describe('FileBasedSyncAdapterService', () => { let service: FileBasedSyncAdapterService; - let mockProvider: jasmine.SpyObj>; + let mockProvider: jasmine.SpyObj>; let mockArchiveDbAdapter: jasmine.SpyObj; let mockStateSnapshotService: jasmine.SpyObj; let adapter: OperationSyncCapable; diff --git a/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.ts b/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.ts index 009d12b01c..def763692c 100644 --- a/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.ts +++ b/src/app/op-log/sync-providers/file-based/file-based-sync-adapter.service.ts @@ -1,7 +1,7 @@ import { inject, Injectable } from '@angular/core'; import { SyncProviderId } from '../provider.const'; import { - SyncProviderServiceInterface, + FileSyncProvider, OperationSyncCapable, SyncOperation, OpUploadResponse, @@ -230,7 +230,7 @@ export class FileBasedSyncAdapterService { * @returns Object implementing OperationSyncCapable interface */ createAdapter( - provider: SyncProviderServiceInterface, + provider: FileSyncProvider, encryptAndCompressCfg: EncryptAndCompressCfg, encryptKey: string | undefined, ): OperationSyncCapable<'fileSnapshotOps'> { @@ -321,7 +321,7 @@ export class FileBasedSyncAdapterService { * Gets the current sync state from cache or by downloading. */ private async _getCurrentSyncState( - provider: SyncProviderServiceInterface, + provider: FileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, providerKey: string, @@ -437,7 +437,7 @@ export class FileBasedSyncAdapterService { * can download the concurrent ops and retry with a consistent snapshot). */ private async _uploadWithMismatchFallback( - provider: SyncProviderServiceInterface, + provider: FileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, newData: FileBasedSyncData, @@ -505,7 +505,7 @@ export class FileBasedSyncAdapterService { } private async _uploadOps( - provider: SyncProviderServiceInterface, + provider: FileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, ops: SyncOperation[], @@ -588,7 +588,7 @@ export class FileBasedSyncAdapterService { // ═══════════════════════════════════════════════════════════════════════════ private async _downloadOps( - provider: SyncProviderServiceInterface, + provider: FileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, sinceSeq: number, @@ -761,7 +761,7 @@ export class FileBasedSyncAdapterService { // ═══════════════════════════════════════════════════════════════════════════ private async _uploadSnapshot( - provider: SyncProviderServiceInterface, + provider: FileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, _state: unknown, @@ -837,7 +837,7 @@ export class FileBasedSyncAdapterService { // ═══════════════════════════════════════════════════════════════════════════ private async _deleteAllData( - provider: SyncProviderServiceInterface, + provider: FileSyncProvider, ): Promise<{ success: boolean }> { const providerKey = this._getProviderKey(provider); @@ -908,7 +908,7 @@ export class FileBasedSyncAdapterService { * @returns The sync data and its revision (ETag) for conditional upload */ private async _downloadSyncFile( - provider: SyncProviderServiceInterface, + provider: FileSyncProvider, cfg: EncryptAndCompressCfg, encryptKey: string | undefined, ): Promise<{ data: FileBasedSyncData; rev: string }> { @@ -962,9 +962,7 @@ export class FileBasedSyncAdapterService { /** * Gets a unique key for a provider (for storing per-provider state). */ - private _getProviderKey( - provider: SyncProviderServiceInterface, - ): string { + private _getProviderKey(provider: FileSyncProvider): string { return `${provider.id}`; } diff --git a/src/app/op-log/sync-providers/provider.interface.ts b/src/app/op-log/sync-providers/provider.interface.ts index 471392fc8c..53e76a27d4 100644 --- a/src/app/op-log/sync-providers/provider.interface.ts +++ b/src/app/op-log/sync-providers/provider.interface.ts @@ -37,13 +37,6 @@ export type FileSyncProvider = GenericFileSyncProvid PrivateCfgByProviderId >; -/** - * @deprecated Use `SyncProviderBase` for generic provider references - * or `FileSyncProvider` for file-based providers. Kept as alias for backward compatibility. - */ -export type SyncProviderServiceInterface = - FileSyncProvider; - export const isFileSyncProvider = ( provider: SyncProviderBase, ): provider is FileSyncProvider => { diff --git a/src/app/op-log/sync-providers/wrapped-provider.service.spec.ts b/src/app/op-log/sync-providers/wrapped-provider.service.spec.ts index a6a6f5a889..b5c4cd4378 100644 --- a/src/app/op-log/sync-providers/wrapped-provider.service.spec.ts +++ b/src/app/op-log/sync-providers/wrapped-provider.service.spec.ts @@ -4,7 +4,11 @@ import { WrappedProviderService } from './wrapped-provider.service'; import { SyncProviderManager } from './provider-manager.service'; import { FileBasedSyncAdapterService } from './file-based/file-based-sync-adapter.service'; import { SyncProviderId } from './provider.const'; -import { SyncProviderServiceInterface, OperationSyncCapable } from './provider.interface'; +import { + FileSyncProvider, + OperationSyncCapable, + SyncProviderBase, +} from './provider.interface'; describe('WrappedProviderService', () => { let service: WrappedProviderService; @@ -12,10 +16,22 @@ describe('WrappedProviderService', () => { let mockFileBasedAdapter: jasmine.SpyObj; let providerConfigChanged$: Subject; - const createMockProvider = ( + const createMockBaseProvider = ( id: SyncProviderId, - supportsOperationSync = false, - ): jasmine.SpyObj> => { + ): jasmine.SpyObj> => { + return jasmine.createSpyObj('SyncProvider', ['isReady'], { + id, + privateCfg: { + load: jasmine + .createSpy('load') + .and.returnValue(Promise.resolve({ encryptKey: 'test-key' })), + }, + }); + }; + + const createMockFileProvider = ( + id: SyncProviderId, + ): jasmine.SpyObj> => { const provider = jasmine.createSpyObj('SyncProvider', ['isReady'], { id, privateCfg: { @@ -24,14 +40,17 @@ describe('WrappedProviderService', () => { .and.returnValue(Promise.resolve({ encryptKey: 'test-key' })), }, }); - if (supportsOperationSync) { - const operationProvider = provider as unknown as { - supportsOperationSync: true; - providerMode: 'superSyncOps'; - }; - operationProvider.supportsOperationSync = true; - operationProvider.providerMode = 'superSyncOps'; - } + return provider; + }; + + const createMockOperationSyncProvider = (): jasmine.SpyObj< + SyncProviderBase & OperationSyncCapable<'superSyncOps'> + > => { + const provider = createMockBaseProvider(SyncProviderId.SuperSync) as jasmine.SpyObj< + SyncProviderBase & OperationSyncCapable<'superSyncOps'> + >; + provider.supportsOperationSync = true; + provider.providerMode = 'superSyncOps'; return provider; }; @@ -81,7 +100,7 @@ describe('WrappedProviderService', () => { }); it('should return SuperSync provider as-is (already implements OperationSyncCapable)', async () => { - const superSyncProvider = createMockProvider(SyncProviderId.SuperSync, true); + const superSyncProvider = createMockOperationSyncProvider(); const result = await service.getOperationSyncCapable(superSyncProvider); @@ -90,7 +109,7 @@ describe('WrappedProviderService', () => { }); it('should wrap Dropbox provider with FileBasedSyncAdapterService', async () => { - const dropboxProvider = createMockProvider(SyncProviderId.Dropbox, false); + const dropboxProvider = createMockFileProvider(SyncProviderId.Dropbox); const mockAdapter = createMockSyncCapableAdapter(); mockFileBasedAdapter.createAdapter.and.returnValue(mockAdapter); @@ -105,7 +124,7 @@ describe('WrappedProviderService', () => { }); it('should wrap WebDAV provider with FileBasedSyncAdapterService', async () => { - const webdavProvider = createMockProvider(SyncProviderId.WebDAV, false); + const webdavProvider = createMockFileProvider(SyncProviderId.WebDAV); const mockAdapter = createMockSyncCapableAdapter(); mockFileBasedAdapter.createAdapter.and.returnValue(mockAdapter); @@ -116,7 +135,7 @@ describe('WrappedProviderService', () => { }); it('should wrap LocalFile provider with FileBasedSyncAdapterService', async () => { - const localFileProvider = createMockProvider(SyncProviderId.LocalFile, false); + const localFileProvider = createMockFileProvider(SyncProviderId.LocalFile); const mockAdapter = createMockSyncCapableAdapter(); mockFileBasedAdapter.createAdapter.and.returnValue(mockAdapter); @@ -127,7 +146,7 @@ describe('WrappedProviderService', () => { }); it('should cache wrapped adapters per provider ID', async () => { - const dropboxProvider = createMockProvider(SyncProviderId.Dropbox, false); + const dropboxProvider = createMockFileProvider(SyncProviderId.Dropbox); const mockAdapter = createMockSyncCapableAdapter(); mockFileBasedAdapter.createAdapter.and.returnValue(mockAdapter); @@ -142,7 +161,7 @@ describe('WrappedProviderService', () => { }); it('should handle provider without encryptKey', async () => { - const dropboxProvider = createMockProvider(SyncProviderId.Dropbox, false); + const dropboxProvider = createMockFileProvider(SyncProviderId.Dropbox); (dropboxProvider.privateCfg.load as jasmine.Spy).and.returnValue( Promise.resolve(null), ); @@ -161,7 +180,7 @@ describe('WrappedProviderService', () => { describe('clearCache', () => { it('should clear cached adapters', async () => { - const dropboxProvider = createMockProvider(SyncProviderId.Dropbox, false); + const dropboxProvider = createMockFileProvider(SyncProviderId.Dropbox); const mockAdapter1 = createMockSyncCapableAdapter(); const mockAdapter2 = createMockSyncCapableAdapter(); mockFileBasedAdapter.createAdapter.and.returnValues(mockAdapter1, mockAdapter2); @@ -182,7 +201,7 @@ describe('WrappedProviderService', () => { describe('auto-invalidation on config change', () => { it('should auto-clear cache when providerConfigChanged$ emits', async () => { - const dropboxProvider = createMockProvider(SyncProviderId.Dropbox, false); + const dropboxProvider = createMockFileProvider(SyncProviderId.Dropbox); const mockAdapter1 = createMockSyncCapableAdapter(); const mockAdapter2 = createMockSyncCapableAdapter(); mockFileBasedAdapter.createAdapter.and.returnValues(mockAdapter1, mockAdapter2); diff --git a/src/app/op-log/sync/operation-log-download.service.spec.ts b/src/app/op-log/sync/operation-log-download.service.spec.ts index 3675a39ba7..5315e002ae 100644 --- a/src/app/op-log/sync/operation-log-download.service.spec.ts +++ b/src/app/op-log/sync/operation-log-download.service.spec.ts @@ -4,7 +4,7 @@ import { OperationLogStoreService } from '../persistence/operation-log-store.ser import { LockService } from './lock.service'; import { SnackService } from '../../core/snack/snack.service'; import { - SyncProviderServiceInterface, + SyncProviderBase, OperationSyncCapable, } from '../sync-providers/provider.interface'; import { SyncProviderId } from '../sync-providers/provider.const'; @@ -82,7 +82,7 @@ describe('OperationLogDownloadService', () => { describe('API-based sync', () => { let mockApiProvider: jasmine.SpyObj< - SyncProviderServiceInterface & OperationSyncCapable + SyncProviderBase & OperationSyncCapable >; beforeEach(() => { diff --git a/src/app/op-log/sync/operation-log-upload.service.spec.ts b/src/app/op-log/sync/operation-log-upload.service.spec.ts index da0ee9520a..9a9dc1bc23 100644 --- a/src/app/op-log/sync/operation-log-upload.service.spec.ts +++ b/src/app/op-log/sync/operation-log-upload.service.spec.ts @@ -3,7 +3,7 @@ import { OperationLogUploadService } from './operation-log-upload.service'; import { OperationLogStoreService } from '../persistence/operation-log-store.service'; import { LockService } from './lock.service'; import { - SyncProviderServiceInterface, + SyncProviderBase, OperationSyncCapable, } from '../sync-providers/provider.interface'; import { SyncProviderId } from '../sync-providers/provider.const'; @@ -88,7 +88,7 @@ describe('OperationLogUploadService', () => { describe('API-based sync', () => { let mockApiProvider: jasmine.SpyObj< - SyncProviderServiceInterface & OperationSyncCapable + SyncProviderBase & OperationSyncCapable >; beforeEach(() => { @@ -509,7 +509,7 @@ describe('OperationLogUploadService', () => { describe('full-state operation routing', () => { let mockApiProvider: jasmine.SpyObj< - SyncProviderServiceInterface & OperationSyncCapable + SyncProviderBase & OperationSyncCapable >; const createFullStateEntry = ( @@ -1143,7 +1143,7 @@ describe('OperationLogUploadService', () => { describe('error handling and recovery', () => { let mockApiProvider: jasmine.SpyObj< - SyncProviderServiceInterface & OperationSyncCapable + SyncProviderBase & OperationSyncCapable >; beforeEach(() => { @@ -1324,7 +1324,7 @@ describe('OperationLogUploadService', () => { * server migration and create duplicate SYNC_IMPORT operations. */ let mockApiProvider: jasmine.SpyObj< - SyncProviderServiceInterface & OperationSyncCapable + SyncProviderBase & OperationSyncCapable >; beforeEach(() => { diff --git a/src/app/op-log/sync/server-migration.service.spec.ts b/src/app/op-log/sync/server-migration.service.spec.ts index 54305de1e6..05b70303ef 100644 --- a/src/app/op-log/sync/server-migration.service.spec.ts +++ b/src/app/op-log/sync/server-migration.service.spec.ts @@ -11,7 +11,7 @@ import { StateSnapshotService } from '../backup/state-snapshot.service'; import { SnackService } from '../../core/snack/snack.service'; import { UserInputWaitStateService } from '../../imex/sync/user-input-wait-state.service'; import { - SyncProviderServiceInterface, + SyncProviderBase, OperationSyncCapable, } from '../sync-providers/provider.interface'; import { SyncProviderId } from '../sync-providers/provider.const'; @@ -34,8 +34,7 @@ describe('ServerMigrationService', () => { let defaultProvider: OperationSyncProvider; // Type for operation-sync-capable provider - type OperationSyncProvider = SyncProviderServiceInterface & - OperationSyncCapable; + type OperationSyncProvider = SyncProviderBase & OperationSyncCapable; // Mock sync provider that supports operations const createMockSyncProvider = (): OperationSyncProvider => { diff --git a/src/app/op-log/testing/integration/helpers/mock-file-provider.helper.ts b/src/app/op-log/testing/integration/helpers/mock-file-provider.helper.ts index 0560342c67..d9118fd45b 100644 --- a/src/app/op-log/testing/integration/helpers/mock-file-provider.helper.ts +++ b/src/app/op-log/testing/integration/helpers/mock-file-provider.helper.ts @@ -1,5 +1,5 @@ import { - SyncProviderServiceInterface, + FileSyncProvider, FileRevResponse, FileDownloadResponse, } from '../../../sync-providers/provider.interface'; @@ -60,7 +60,7 @@ const createMockCredentialStore = < }; /** - * Mock implementation of SyncProviderServiceInterface for integration testing. + * Mock implementation of FileSyncProvider for integration testing. * * Simulates a file-based storage (like Dropbox/WebDAV) by storing data in memory. * This allows testing FileBasedSyncAdapterService without real network calls. @@ -88,7 +88,7 @@ const createMockCredentialStore = < * ); * ``` */ -export class MockFileProvider implements SyncProviderServiceInterface { +export class MockFileProvider implements FileSyncProvider { readonly id: SyncProviderId; readonly isUploadForcePossible = true; readonly maxConcurrentRequests = 4; diff --git a/src/app/op-log/testing/integration/service-logic.integration.spec.ts b/src/app/op-log/testing/integration/service-logic.integration.spec.ts index ffc5a275df..e74a0967ed 100644 --- a/src/app/op-log/testing/integration/service-logic.integration.spec.ts +++ b/src/app/op-log/testing/integration/service-logic.integration.spec.ts @@ -12,7 +12,7 @@ import { ValidateStateService } from '../../validation/validate-state.service'; import { RepairOperationService } from '../../validation/repair-operation.service'; import { StateSnapshotService } from '../../backup/state-snapshot.service'; import { - SyncProviderServiceInterface, + SyncProviderBase, OperationSyncCapable, OpUploadResponse, OpDownloadResponse, @@ -58,7 +58,7 @@ import { SyncImportFilterService } from '../../sync/sync-import-filter.service'; // Mock Sync Provider that supports operation sync class MockOperationSyncProvider - implements SyncProviderServiceInterface, OperationSyncCapable + implements SyncProviderBase, OperationSyncCapable { id = SyncProviderId.SuperSync; supportsOperationSync = true; diff --git a/src/app/plugins/oauth/pkce.util.spec.ts b/src/app/plugins/oauth/pkce.util.spec.ts deleted file mode 100644 index 8b7ec73b4a..0000000000 --- a/src/app/plugins/oauth/pkce.util.spec.ts +++ /dev/null @@ -1,38 +0,0 @@ -import { generateCodeVerifier, generateCodeChallenge } from './pkce.util'; - -describe('PKCE utilities', () => { - describe('generateCodeVerifier', () => { - it('should return a string of 43-128 characters', () => { - const verifier = generateCodeVerifier(); - expect(verifier.length).toBeGreaterThanOrEqual(43); - expect(verifier.length).toBeLessThanOrEqual(128); - }); - - it('should only contain URL-safe characters', () => { - const verifier = generateCodeVerifier(); - expect(verifier).toMatch(/^[A-Za-z0-9\-._~]+$/); - }); - - it('should generate unique values', () => { - const v1 = generateCodeVerifier(); - const v2 = generateCodeVerifier(); - expect(v1).not.toEqual(v2); - }); - }); - - describe('generateCodeChallenge', () => { - it('should return a base64url-encoded SHA-256 hash', async () => { - const verifier = generateCodeVerifier(); - const challenge = await generateCodeChallenge(verifier); - // base64url: A-Z, a-z, 0-9, -, _ (no = padding) - expect(challenge).toMatch(/^[A-Za-z0-9\-_]+$/); - }); - - it('should produce a consistent hash for the same input', async () => { - const verifier = 'test-verifier-string-for-pkce'; - const c1 = await generateCodeChallenge(verifier); - const c2 = await generateCodeChallenge(verifier); - expect(c1).toEqual(c2); - }); - }); -}); diff --git a/src/app/plugins/oauth/pkce.util.ts b/src/app/plugins/oauth/pkce.util.ts deleted file mode 100644 index e004b32258..0000000000 --- a/src/app/plugins/oauth/pkce.util.ts +++ /dev/null @@ -1 +0,0 @@ -export { generateCodeVerifier, generateCodeChallenge } from '../../util/pkce.util'; diff --git a/src/app/plugins/oauth/plugin-oauth.service.ts b/src/app/plugins/oauth/plugin-oauth.service.ts index 3fffa9cf41..1309c0cc76 100644 --- a/src/app/plugins/oauth/plugin-oauth.service.ts +++ b/src/app/plugins/oauth/plugin-oauth.service.ts @@ -2,8 +2,8 @@ import { Injectable, inject } from '@angular/core'; import { HttpClient, HttpHeaders } from '@angular/common/http'; import { firstValueFrom, Subject } from 'rxjs'; import { OAuthFlowConfig, OAuthTokenResult } from '@super-productivity/plugin-api'; +import { generateCodeChallenge, generateCodeVerifier } from '@sp/sync-providers'; import { PluginOAuthTokens } from './plugin-oauth.model'; -import { generateCodeVerifier, generateCodeChallenge } from './pkce.util'; import { IS_ELECTRON } from '../../app.constants'; import { IS_NATIVE_PLATFORM, IS_ANDROID_NATIVE } from '../../util/is-native-platform'; import { PluginLog } from '../../core/log'; diff --git a/src/app/util/pkce.util.ts b/src/app/util/pkce.util.ts deleted file mode 100644 index b2da55e13d..0000000000 --- a/src/app/util/pkce.util.ts +++ /dev/null @@ -1,44 +0,0 @@ -// Shared PKCE (Proof Key for Code Exchange) utility. -// Handles environments where crypto.subtle is unavailable (e.g., Android Capacitor -// on http://localhost) by falling back to hash-wasm. -// @see https://www.chromium.org/blink/webcrypto - -import { sha256 as hashWasmSha256 } from 'hash-wasm'; - -const base64UrlEncode = (buffer: Uint8Array): string => { - const base64 = btoa(String.fromCharCode(...buffer)); - return base64.replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/, ''); -}; - -/** - * SHA-256 hash with automatic fallback for insecure contexts. - * Uses crypto.subtle when available, otherwise falls back to hash-wasm. - */ -const sha256 = async (data: Uint8Array): Promise => { - if (typeof crypto !== 'undefined' && crypto.subtle != null) { - return crypto.subtle.digest('SHA-256', data as BufferSource); - } - - // Fallback to hash-wasm for insecure contexts (e.g., Android Capacitor on http://localhost). - // hash-wasm is already a dependency (used for Argon2id encryption). - const hexHash = await hashWasmSha256(data); - const bytes = new Uint8Array(hexHash.length / 2); - for (let i = 0; i < hexHash.length; i += 2) { - bytes[i / 2] = parseInt(hexHash.slice(i, i + 2), 16); - } - return bytes.buffer; -}; - -/** Generate a cryptographically random code verifier (base64url-encoded). */ -export const generateCodeVerifier = (): string => { - const array = new Uint8Array(32); - crypto.getRandomValues(array); - return base64UrlEncode(array); -}; - -/** Generate an S256 code challenge from a code verifier. */ -export const generateCodeChallenge = async (verifier: string): Promise => { - const data = new TextEncoder().encode(verifier); - const digest = await sha256(data); - return base64UrlEncode(new Uint8Array(digest)); -};