chore(runner): removed all direct reads or writes to DocImpl, now everything goes via transactions #1427
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR removes all direct reads and writes to DocImpl, routing everything through transactions instead. This is a significant refactoring that affects the core data access layer of the system.
Key changes:
- Replaces legacy
getAsLegacyCellLink()calls with moderngetAsLink()andgetAsWriteRedirectLink()methods - Updates all cell operations to use transaction-based reads and writes instead of direct document access
- Refactors storage interfaces to use activity logs instead of legacy transaction logs
Reviewed Changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui/src/v1/components/common-plaid-link.ts | Updates method calls from legacy cell link format to modern format |
| packages/runner/test/*.test.ts | Extensive test updates to use new cell link and transaction APIs |
| packages/runner/src/storage/transaction-shim.ts | Major refactoring of storage transaction implementation and logging |
| packages/runner/src/storage/interface.ts | Updates interface definitions for transaction methods |
| packages/runner/src/*.ts | Core runtime files updated to use transaction-based operations |
packages/runner/test/cell.test.ts
Outdated
There was a problem hiding this comment.
The function name changed from isLegacyCellLink to isAnyCellLink but the test description still refers to 'legacy cell link'. Consider updating the test description to match the new function name.
There was a problem hiding this comment.
cubic found 2 issues across 30 files. Review them in cubic.dev
React with 👍 or 👎 to teach cubic. Tag @cubic-dev-ai to give specific feedback.
17124f0 to
f2f0418
Compare
packages/runner/src/cell.ts
Outdated
There was a problem hiding this comment.
suggest we create a unit test to show ignorereadforscheduling is respected (i can do this later)
packages/runner/src/cell.ts
Outdated
There was a problem hiding this comment.
this pattern is repeated a few times and also in query-result-proxy.ts, runner.ts. perhaps option for refactoring into a function that also takes in the failure option (runtime.edit() or undefined for example). but not a big deal.
packages/runner/src/cell.ts
Outdated
There was a problem hiding this comment.
suggest calling this readOnlyReason if we think we'll ever use it to show the reason, otherwise turn to boolean
(runner tests still broken)
- Refactored core runner modules to eliminate direct references to the `doc` object. - Updated builder, builtins, cell, data-updating, doc-map, link-utils, query-result-proxy, recipe-binding, runner, runtime, scheduler, sigil-types, storage, storage/transaction-shim, and uri-utils to use new abstractions or access patterns. - Adjusted related tests to align with the new approach and ensure coverage.
- updated read APIs to pass metadata along - use it to ignore certain reads in scheduler
- Move createSigilLink function from cell.ts to link-utils.ts as createSigilLinkFromParsedLink - Add support for base cell/link parameter to enable relative references - Add sanitizeSchemaForLinks function to remove asCell/asStream flags from schemas - Update all callers to use new function signature with options object - Add comprehensive tests for schema sanitization
54433d0 to
a0cd848
Compare
Summary by cubic
Removed all direct reads and writes to the internal
DocImplobject; all cell and document access now goes through transaction-based APIs.docaccess.