Commit graph

5 commits

Author SHA1 Message Date
Johannes Millan
24a691bed5
fix(sync): surface native WebDAV errors (#8058) 2026-06-06 15:09:46 +02:00
Johannes Millan
087b9dd43f
refactor(sync): post-extraction review cleanup of @sp/sync-core and @sp/sync-providers (#7595)
* refactor(sync): tighten extracted package surfaces

Combined polish from the post-extraction review:

- sync-core: strip NgRx-shaped types from EntityConfig/EntityRegistry;
  expose host extensions via generic param. Move StateSelector,
  PropsStateSelector, SelectByIdFactory, SelectById, EntityUpdateLike,
  EntityAdapterLike to a new app-side entity-registry-host.types.ts.
- sync-core: mark OpType.SyncImport/BackupImport/Repair as @deprecated;
  hosts should use createFullStateOpTypeHelpers().
- sync-providers: resolve provider.types.ts vs provider-types.ts
  duplication; inline implementation into the dashed canonical name.
- sync-providers: drop unused root barrel and "." export; consumers
  already use focused subpath barrels (/dropbox, /webdav, etc.).
- sync-providers: replace wildcard "@sp/sync-providers/*" tsconfig path
  alias with 11 explicit subpath entries matching package.json exports;
  deep-internal imports now fail at typecheck.
- sync-providers: move @sp/sync-core from dependencies to
  peerDependencies (kept in devDependencies for tests).
- both packages: add composite: true to enable project references;
  introduce tsconfig.build.json overlay so tsup DTS bundler still works.
- gitignore: ignore **/*.tsbuildinfo composite outputs.

* refactor(sync-core): prune 47 unused barrel exports

Removes exports with zero consumers outside the package. Source files
are unchanged; only the public barrel is trimmed. Covers compression
helper classes, sync-file-prefix error/config types, replay coordinator
internals, remote-apply result types, upload/download planning option
and plan types, ports misc, conflict-resolution helper types, and
sync-import-filter decision types.

* refactor(sync-core): drop unused encryption migration path

decryptWithMigration and DecryptResult had no host consumer; they
exposed a structural-migration entry point ("here is your ciphertext
re-encrypted under Argon2id") that nothing in the codebase reads. The
side-channel setLegacyKdfWarningHandler — which IS used — stays.

encryptWithDerivedKey/decryptWithDerivedKey lose their export keyword
and remain as module-internal helpers; encrypt/decrypt/encryptBatch/
decryptBatch still call them. Wire format and legacy-fallback semantics
are unchanged, so existing ciphertext continues to decrypt.

Test imports for compression and sync-file-prefix specs now go via
their source files instead of the trimmed barrel.

* fix(sync-providers): bound dropbox token refresh to single retry; share md5 rev helper

The five hand-rolled token-refresh blocks in Dropbox.{getFileRev,
downloadFile, uploadFile, removeFile, listFiles} recursed on themselves
after refresh. If the post-refresh call still saw a token error (real
case: the refresh token itself was revoked), the recursion would not
terminate. Consolidated into a single _withTokenRefresh helper that
attempts the call, refreshes once on a token error, retries once, then
lets the outer 401 classifier surface AuthFailSPError.

Same log message, same _isTokenError discriminator, same refresh call.
Same five sites still apply their post-call non-token error mapping
(NoRev, InvalidData, RemoteFileNotFound, path-not-found swallow, etc.).

Also extracts md5 content-rev computation duplicated between
LocalFileSyncBase._getLocalRev and WebdavApi._computeContentHash into a
shared file-based/content-rev.ts; both call sites preserve their own
error wrapping at the boundary.

* refactor(sync): split oversized super-sync and conflict-resolution

sync-providers: extract request-ID hashing from super-sync.ts (1017 ->
918 lines) into a new request-id.ts. The helpers were free functions
already in disguise (none referenced this), so the move is mechanical.
HTTP plumbing (_doWebFetch/_doNativeFetch/_fetchApi*) stays as private
methods — it transitively touches 12 instance members and would need
either a wide context object or a separate http-client collaborator
class to extract cleanly. Left as a follow-up.

sync-core: split conflict-resolution.ts into three cohesive files:
- entity-frontier.ts now owns buildEntityFrontier and
  adjustForClockCorruption (per-entity vector-clock domain).
- extractEntityFromPayload and extractUpdateChanges move to
  operation.types.ts next to the existing extractActionPayload.
- conflict-resolution.ts keeps deep-equality, LWW planning,
  partitioning, and identical-conflict detection.

Public barrel exports unchanged; tests now import the moved symbols
from their new homes.

* refactor(sync-core): drop redundant OperationStorePort

OperationStorePort overlapped with RemoteOperationApplyStorePort on the
two state-transition methods (markSynced/markApplied,
markRejected/markFailed) and had zero non-structural consumers — the
only implementer was OperationLogStoreService, which already exposes
the three methods as its own public surface. Removing the port leaves
the service contract intact and removes the verb-pair confusion noted
in the post-extraction review.

Spec contract test still drives the same state transitions; only the
local typing of the test fixture changes from the deleted interface to
Pick<OperationLogStoreService, ...>.

* refactor(sync-providers): decouple SuperSync provider from SP-specific host

Two coupling leaks the package shouldn't carry:

1. SUPER_SYNC_DEFAULT_BASE_URL was an implicit fallback inside
   SuperSyncProvider — an SP-specific URL baked into a "framework-
   agnostic" package. Make defaultBaseUrl a required SuperSyncDeps
   field; the host factory supplies the SP default. The constant stays
   exported as a suggested default for hosts targeting the SP-hosted
   server.

2. Consumers that wanted the WebSocket path had to do
   `provider as unknown as SuperSyncProvider` to call
   getWebSocketParams. Introduce SuperSyncWebSocketAccess interface +
   isSuperSyncWebSocketAccess structural guard; SuperSyncProvider
   implements it. sync-wrapper.service drops its cast in favor of the
   guard.

super-sync-restore.service still casts to SuperSyncProvider for the
restore path — same pattern would solve it, but out of scope here.

* test(sync-providers): extract shared test helpers and prefer barrels

Adds tests/helpers/sync-logger.ts and tests/helpers/credential-store.ts
to centralize the noopLogger and CredentialStore mocks that were copy-
pasted across 8 spec files. createStatefulCredentialStore covers the
"load/upsert/clear with state" cases; createMockCredentialStore covers
bare vi.fn() ports. Spec sites that needed a unique mockResolvedValue
chain it after the helper, preserving behavior 1:1.

Also migrates 5 spec files from deep ../src/<file> paths to the
matching sub-barrel (../src/webdav, /http, /super-sync, /platform) for
symbols already exported there. No new barrel exports added — internal
types (WebDavHttpAdapter, WebdavApi, DropboxApi, etc.) stay on deep
paths because they are intentionally not part of the public surface.

super-sync.spec.ts keeps its own credential/logger mocks (special
__asPort wrapper and vi.spyOn against the live NOOP_SYNC_LOGGER) that
the generic helpers cannot reproduce without bloat.

* test(sync): pin vector-clock pruning, error-meta privacy, and sync-import edges

Fills three test gaps surfaced by the post-extraction review:

- vector-clock pruning correctness across clocks: 4 cases pinning that
  pruning legitimately flips GREATER_THAN to CONCURRENT/LESS_THAN when
  the dropped keys are still present in the comparison clock. This is
  the documented behavior (compareVectorClocks is intentionally not
  pruning-aware); the protocol handles flips server-side via the
  rejected-ops retry loop. preserveClientIds case also covered.

- error-meta privacy boundary: 22 new cases covering urlPathOnly (strip
  query/fragment/userinfo, preserve host+path+port, leave non-URLs
  intact) and errorMeta (no leakage of headers, response bodies, OAuth
  tokens, signed-URL params, user emails, or attached error fields).
  Real negative assertions (.not.toContain), not shape checks.

- sync-import-filter edge cases: 8 cases covering empty clocks on
  either side, op clock listing the import client at 0, same-client
  with equal counter (pinning the strict-greater-than boundary), and
  different-client knowledge above the import counter.

sync-core 195 -> 207 tests, sync-providers 319 -> 341 tests; no
production code changed.

* style(sync-core): format sync-file-prefix.spec import line

* fix(sync): address package review feedback
2026-05-14 13:06:08 +02:00
Johannes Millan
87f092bee3 fix(sync): correct error class names, JSDoc, and missed cache-clear
Findings from a multi-agent review of the recent sync extraction:

- Seven error classes in @sp/sync-providers shipped with a leading
  space in `name`, and `UploadRevToMatchMismatchAPIError` was further
  truncated to ' UploadRevToMatchMismatchAP'. Consumers use instanceof
  so runtime behavior was preserved, but stack traces, error envelopes,
  and structured-log meta carried the broken names. Added a regression
  test asserting instance.name matches ErrCtor.name for all 14 classes.
- @sp/sync-core decryptBatch JSDoc claimed Argon2 errors are never
  silently masked as legacy fallbacks, contradicting the actual
  catch-and-decryptLegacy path. The fallback is part of the public
  wire-format contract; rewrote the comment to match the implementation
  and reference the module-level wire-format spec.
- SuperSyncEncryptionToggleService.enable/disableEncryption promised
  "Clear cache on success" but never called clearSessionKeyCache().
  Added the call on the success path in both methods, matching the
  pattern used by EncryptionPasswordChangeService and
  FileBasedEncryptionService.
- Removed packages/sync-core/tests/ports.spec.ts — 57 LOC of
  vi.fn().mockResolvedValue type-assertion tests with no production
  code under test. Port shapes are enforced at the host via
  `implements` clauses with real behavioral coverage in sibling specs.
2026-05-13 19:49:26 +02:00
Johannes Millan
e4f9a4e2e5 refactor(sync-providers): extract local file provider 2026-05-13 11:36:19 +02:00
Johannes Millan
91e53bb488 refactor(sync-providers): move provider error classes
Lift 12 provider-shared error classes (AuthFailSPError, InvalidDataSPError,
HttpNotOkAPIError, NoRevAPIError, RemoteFileNotFoundAPIError,
MissingCredentialsSPError, MissingRefreshTokenAPIError,
TooManyRequestsAPIError, UploadRevToMatchMismatchAPIError,
PotentialCorsError, RemoteFileChangedUnexpectedly, EmptyRemoteBodySPError)
plus AdditionalLogErrorBase and extractErrorMessage into
@sp/sync-providers. App-side sync-errors.ts becomes a re-export shim
so existing call sites and instanceof checks keep working.

The moved AdditionalLogErrorBase drops its constructor-time
OP_LOG_SYNC_LOGGER.log side effect (Option A from the slice design):
privacy responsibility shifts entirely to catch-site logging via the
injected SyncLogger port. A new app-side identity spec asserts the
constructor identity is preserved across import paths so future bundler
or tsconfig drift can't silently break instanceof catches.

HttpNotOkAPIError splits its parsed body excerpt off .message onto a
new opt-in .detail field; getErrorTxt forwards .detail to UI surfaces
so user-visible toasts remain unchanged while privacy-aware logger
paths see only "HTTP <status> <statusText>".

TooManyRequestsAPIError's constructor is narrowed to accept only
{ status, retryAfter?, path? } — closing a latent bearer-token leak
where Dropbox's _handleErrorResponse passed the raw Authorization
header through additionalLog. Callers in dropbox-api and
webdav-http-adapter updated accordingly.

Package gains "sideEffects": false to unlock tree-shaking through the
barrel for consumers that import only error classes.

Slice design and round-2 multi-review findings documented in
docs/plans/2026-05-12-pr5-dropbox-slice.md.
2026-05-12 20:32:08 +02:00