Open
Conversation
Pull PROTO_op-type-class-hierarchy (cool-ideas) and CC_op-types-typedef-to-class (bad-code) into cycle 0009. Hill: replace 8 typedef ops with frozen class hierarchy for runtime identity, constructor validation, and instanceof dispatch.
Base Op class (abstract) + 8 concrete subclasses: NodeAdd, NodeRemove, EdgeAdd, EdgeRemove, NodePropSet, EdgePropSet, PropSet, BlobValue. Each class: constructor validation, Object.freeze, instanceof dispatch. Edge types use options objects (max-params compliance). Shared validators in validate.js. Cycle 0009 ��� op-type-class-hierarchy
Factory functions in WarpTypesV2.js now delegate to Op class constructors. OpNormalizer.normalizeRawOp() and lowerCanonicalOp() already return class instances via the factory functions. Updated WarpTypesV2.test.js: real Dot instances replace plain objects, encoded dot strings replace object arrays for observedDots. 15 new factory-integration tests + 35 updated existing tests. Cycle 0009 — op-type-class-hierarchy
11 new tests verifying class-instance ops work through all three reducer paths (applyFast, applyWithReceipt, applyWithDiff) and OP_STRATEGIES dispatch. No source changes needed — the string-keyed strategy Map already dispatches correctly for class instances since they carry the same .type field. Cycle 0009 — op-type-class-hierarchy
Cycle 0009 — op-type-class-hierarchy
Witness: hill met — all 8 agent questions answered YES (Q9 deferred). All 3 human questions answered YES. Hard gates: noCoordination 7/7, 5504/5504 tests, lint clean. Retro: fast RED→GREEN, edge options objects caught by lint, existing tests had wrong Dot field names hidden by any-casts. New backlog: - PROTO_op-consumer-instanceof-migration (up-next, S) - PROTO_cbor-op-hydration (up-next, M) - CC_versionvector-constructor-no-validation (bad-code, S) Cycle 0009 — op-type-class-hierarchy
📝 WalkthroughWalkthroughThis PR replaces plain-object op typedefs with a frozen Op class hierarchy (8 concrete subclasses), updates WarpTypesV2 factories to construct those classes, adds validation helpers and 97 tests, and documents design/retro/backlog items. CHANGELOG also notes auto-materialize-on-remove behavior for createPatch(). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
test/unit/domain/types/WarpTypesV2.test.js (1)
442-447: Use distinctDotinstances per add-op fixture.This test currently reuses one
Dotfor bothNodeAddandEdgeAdd. Separate dots keep fixtures aligned with causal-identity expectations and avoid normalizing an invalid pattern.Proposed test tweak
- const dot = new Dot('w', 1); + const nodeDot = new Dot('w', 1); + const edgeDot = new Dot('w', 2); const ops = [ - createNodeAddV2('n1', dot), + createNodeAddV2('n1', nodeDot), createNodeRemoveV2('n1', []), - createEdgeAddV2('n1', 'n2', 'r', dot), + createEdgeAddV2('n1', 'n2', 'r', edgeDot), createEdgeRemoveV2('n1', 'n2', 'r', []), createPropSetV2('n1', 'k', 'v'), ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/types/WarpTypesV2.test.js` around lines 442 - 447, The test reuses one Dot instance for both NodeAdd and EdgeAdd which violates causal-identity expectations; update the ops fixture so createNodeAddV2 and createEdgeAddV2 each receive their own distinct Dot (e.g., two Dot(...) instances) instead of sharing the same dot variable; locate the ops array construction around the calls to createNodeAddV2, createNodeRemoveV2, createEdgeAddV2, and createEdgeRemoveV2 and replace the single shared Dot with separate Dot instances for the add operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/0009-op-type-class-hierarchy/op-type-class-hierarchy.md`:
- Around line 31-32: The design doc claims JoinReducer.OP_STRATEGIES dispatches
by instanceof but the implementation uses string-key lookup via op.type; update
the design doc text (references: JoinReducer.OP_STRATEGIES, JoinReducer.js line
showing OP_STRATEGIES.get(op.type)) to accurately state that strategy lookup is
performed by the `.type` string on operation instances, or explicitly document
that an instanceof-based dispatch approach is planned/deferred and not currently
used; ensure the doc references the `.type` property on class instances and
remove or rephrase any lines asserting instanceof dispatch.
In `@src/domain/types/ops/EdgeRemove.js`:
- Around line 36-40: The container-level assertArray call in the EdgeRemove
constructor is not enough—after assertArray(observedDots, 'EdgeRemove',
'observedDots') you must validate each element is a non-empty encoded dot string
before freezing; iterate observedDots and run the appropriate element validator
(e.g., an existing assertEncodedDot or a new small helper) on each entry
(include context 'EdgeRemove' and 'observedDots' and the element index in error
messages), then set this.observedDots = Object.freeze([...observedDots]).
In `@src/domain/types/ops/NodePropSet.js`:
- Around line 8-9: NodePropSet's constructor currently uses assertNonEmptyString
but does not reject NUL bytes for the identifiers; update the validation in the
NodePropSet constructor (the class/function named NodePropSet) to check both the
node and key parameters for banned NUL bytes (e.g., string.includes('\0')) and
throw/raise the same validation error when found; reuse or extend
assertNonEmptyString (or add a small helper) so both node and key are rejected
on construction if they contain '\0' (also apply the same check to the other
validation locations referenced in the comment around lines 33-35).
In `@src/domain/types/ops/NodeRemove.js`:
- Around line 29-31: The constructor for NodeRemove only asserts observedDots is
an array but doesn't validate elements; update the NodeRemove constructor to
iterate observedDots and validate each entry is a non-empty string (e.g., use or
add an assertString/assertNonEmpty helper) before freezing the array, and throw
a clear error mentioning 'NodeRemove' and 'observedDots' if any element is
invalid so downstream remove processing cannot receive bad entries.
In `@src/domain/types/ops/PropSet.js`:
- Around line 12-13: PropSet currently only calls assertNonEmptyString for the
key, which allows NUL bytes; update the PropSet constructor (or the helper it
uses) to reject any key containing '\0' by adding a NUL-byte check alongside the
existing assertNonEmptyString validation (e.g., throw if key.includes('\0'));
ensure the same change is applied for all PropSet key validations mentioned
(lines ~37-39) so every code path that sets PropSet.key enforces non-empty and
no NUL bytes, referencing the PropSet constructor and assertNonEmptyString
helper for where to add the check.
In `@test/unit/domain/types/ops/factory-integration.test.js`:
- Line 15: Remove the unused import by deleting the BlobValue import statement
(the "import BlobValue from ...BlobValue.js" line) from the test file; ensure
there are no remaining references to the BlobValue symbol in the file and run
the tests/linter to confirm the unused-import warning is resolved.
- Around line 120-129: The test's `canonical` variable is typed as a union so
TypeScript can't see `node`/`key` even after the runtime `toBeInstanceOf` check;
narrow the type before accessing properties by casting `canonical` to
`NodePropSet` (e.g., use a type assertion like `canonical as NodePropSet` or
assign `const nodeProp = canonical as NodePropSet`) and then assert
`nodeProp.node`, `nodeProp.key`, and `nodeProp.value`; keep using
`normalizeRawOp`, `NodePropSet`, and the `canonical` variable names to locate
the change.
---
Nitpick comments:
In `@test/unit/domain/types/WarpTypesV2.test.js`:
- Around line 442-447: The test reuses one Dot instance for both NodeAdd and
EdgeAdd which violates causal-identity expectations; update the ops fixture so
createNodeAddV2 and createEdgeAddV2 each receive their own distinct Dot (e.g.,
two Dot(...) instances) instead of sharing the same dot variable; locate the ops
array construction around the calls to createNodeAddV2, createNodeRemoveV2,
createEdgeAddV2, and createEdgeRemoveV2 and replace the single shared Dot with
separate Dot instances for the add operations.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba3cdbe1-183c-4e0d-b5dd-cac2ae425741
📒 Files selected for processing (25)
CHANGELOG.mddocs/design/0009-op-type-class-hierarchy/op-type-class-hierarchy.mddocs/design/0009-op-type-class-hierarchy/witness.mddocs/method/backlog/bad-code/CC_op-types-typedef-to-class.mddocs/method/backlog/bad-code/CC_versionvector-constructor-no-validation.mddocs/method/backlog/cool-ideas/PROTO_op-type-class-hierarchy.mddocs/method/backlog/up-next/PROTO_cbor-op-hydration.mddocs/method/backlog/up-next/PROTO_op-consumer-instanceof-migration.mddocs/method/retro/0009-op-type-class-hierarchy/retro.mdsrc/domain/types/WarpTypesV2.jssrc/domain/types/ops/BlobValue.jssrc/domain/types/ops/EdgeAdd.jssrc/domain/types/ops/EdgePropSet.jssrc/domain/types/ops/EdgeRemove.jssrc/domain/types/ops/NodeAdd.jssrc/domain/types/ops/NodePropSet.jssrc/domain/types/ops/NodeRemove.jssrc/domain/types/ops/Op.jssrc/domain/types/ops/PropSet.jssrc/domain/types/ops/validate.jstest/unit/domain/types/WarpTypesV2.test.jstest/unit/domain/types/ops/Op.test.jstest/unit/domain/types/ops/factory-integration.test.jstest/unit/domain/types/ops/reducer-integration.test.jstest/unit/domain/types/ops/validate.test.js
💤 Files with no reviewable changes (2)
- docs/method/backlog/bad-code/CC_op-types-typedef-to-class.md
- docs/method/backlog/cool-ideas/PROTO_op-type-class-hierarchy.md
Comment on lines
+36
to
+40
| assertArray(observedDots, 'EdgeRemove', 'observedDots'); | ||
| this.from = from; | ||
| this.to = to; | ||
| this.label = label; | ||
| this.observedDots = Object.freeze([...observedDots]); |
There was a problem hiding this comment.
observedDots needs element-level validation.
At Line 36, container validation alone is insufficient. Validate each entry as a non-empty encoded dot string before freezing.
Proposed fix
constructor({ from, to, label, observedDots }) {
super('EdgeRemove');
assertNonEmptyString(from, 'EdgeRemove', 'from');
assertNonEmptyString(to, 'EdgeRemove', 'to');
assertNonEmptyString(label, 'EdgeRemove', 'label');
assertArray(observedDots, 'EdgeRemove', 'observedDots');
+ for (let i = 0; i < observedDots.length; i += 1) {
+ assertNonEmptyString(observedDots[i], 'EdgeRemove', `observedDots[${i}]`);
+ }
this.from = from;
this.to = to;
this.label = label;
this.observedDots = Object.freeze([...observedDots]);
Object.freeze(this);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/types/ops/EdgeRemove.js` around lines 36 - 40, The container-level
assertArray call in the EdgeRemove constructor is not enough—after
assertArray(observedDots, 'EdgeRemove', 'observedDots') you must validate each
element is a non-empty encoded dot string before freezing; iterate observedDots
and run the appropriate element validator (e.g., an existing assertEncodedDot or
a new small helper) on each entry (include context 'EdgeRemove' and
'observedDots' and the element index in error messages), then set
this.observedDots = Object.freeze([...observedDots]).
Comment on lines
+29
to
+31
| assertArray(observedDots, 'NodeRemove', 'observedDots'); | ||
| this.node = node; | ||
| this.observedDots = Object.freeze([...observedDots]); |
There was a problem hiding this comment.
Validate each observedDots entry, not just the container.
At Line 29, only array shape is checked. Non-string entries can pass construction and fail semantically during remove processing. Please validate each element as a non-empty string.
Proposed fix
import Op from './Op.js';
import { assertNonEmptyString, assertArray } from './validate.js';
@@
constructor(node, observedDots) {
super('NodeRemove');
assertNonEmptyString(node, 'NodeRemove', 'node');
assertArray(observedDots, 'NodeRemove', 'observedDots');
+ for (let i = 0; i < observedDots.length; i += 1) {
+ assertNonEmptyString(observedDots[i], 'NodeRemove', `observedDots[${i}]`);
+ }
this.node = node;
this.observedDots = Object.freeze([...observedDots]);
Object.freeze(this);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assertArray(observedDots, 'NodeRemove', 'observedDots'); | |
| this.node = node; | |
| this.observedDots = Object.freeze([...observedDots]); | |
| assertArray(observedDots, 'NodeRemove', 'observedDots'); | |
| for (let i = 0; i < observedDots.length; i += 1) { | |
| assertNonEmptyString(observedDots[i], 'NodeRemove', `observedDots[${i}]`); | |
| } | |
| this.node = node; | |
| this.observedDots = Object.freeze([...observedDots]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/types/ops/NodeRemove.js` around lines 29 - 31, The constructor for
NodeRemove only asserts observedDots is an array but doesn't validate elements;
update the NodeRemove constructor to iterate observedDots and validate each
entry is a non-empty string (e.g., use or add an assertString/assertNonEmpty
helper) before freezing the array, and throw a clear error mentioning
'NodeRemove' and 'observedDots' if any element is invalid so downstream remove
processing cannot receive bad entries.
Comment on lines
+12
to
+13
| import { assertNonEmptyString } from './validate.js'; | ||
|
|
There was a problem hiding this comment.
Enforce NUL-byte validation for PropSet.key.
key is only checked for non-empty string. That permits NUL bytes, which conflicts with the shared identifier constraints and weakens constructor boundary validation.
🔧 Proposed fix
-import { assertNonEmptyString } from './validate.js';
+import { assertNonEmptyString, assertNoBannedBytes } from './validate.js';
@@
assertNonEmptyString(node, 'PropSet', 'node');
assertNonEmptyString(key, 'PropSet', 'key');
+ assertNoBannedBytes(key, 'PropSet', 'key');
this.node = node;Also applies to: 37-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/types/ops/PropSet.js` around lines 12 - 13, PropSet currently only
calls assertNonEmptyString for the key, which allows NUL bytes; update the
PropSet constructor (or the helper it uses) to reject any key containing '\0' by
adding a NUL-byte check alongside the existing assertNonEmptyString validation
(e.g., throw if key.includes('\0')); ensure the same change is applied for all
PropSet key validations mentioned (lines ~37-39) so every code path that sets
PropSet.key enforces non-empty and no NUL bytes, referencing the PropSet
constructor and assertNonEmptyString helper for where to add the check.
| import NodePropSet from '../../../../../src/domain/types/ops/NodePropSet.js'; | ||
| import EdgePropSet from '../../../../../src/domain/types/ops/EdgePropSet.js'; | ||
| import PropSetClass from '../../../../../src/domain/types/ops/PropSet.js'; | ||
| import BlobValue from '../../../../../src/domain/types/ops/BlobValue.js'; |
There was a problem hiding this comment.
Remove unused BlobValue import.
The static analysis correctly identifies that BlobValue is imported but never used in this test file.
🧹 Proposed fix
-import BlobValue from '../../../../../src/domain/types/ops/BlobValue.js';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import BlobValue from '../../../../../src/domain/types/ops/BlobValue.js'; |
🧰 Tools
🪛 GitHub Check: preflight
[failure] 15-15:
'BlobValue' is declared but its value is never read.
🪛 GitHub Check: type-firewall
[failure] 15-15:
'BlobValue' is declared but its value is never read.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/domain/types/ops/factory-integration.test.js` at line 15, Remove
the unused import by deleting the BlobValue import statement (the "import
BlobValue from ...BlobValue.js" line) from the test file; ensure there are no
remaining references to the BlobValue symbol in the file and run the
tests/linter to confirm the unused-import warning is resolved.
Comment on lines
+120
to
+129
| it('normalizeRawOp converts PropSet (node) to NodePropSet instance', () => { | ||
| const raw = createPropSetV2('user:alice', 'name', 'Alice'); | ||
| const canonical = normalizeRawOp(raw); | ||
|
|
||
| expect(canonical).toBeInstanceOf(NodePropSet); | ||
| expect(canonical.type).toBe('NodePropSet'); | ||
| expect(canonical.node).toBe('user:alice'); | ||
| expect(canonical.key).toBe('name'); | ||
| expect(canonical.value).toBe('Alice'); | ||
| }); |
There was a problem hiding this comment.
Add type narrowing to fix TypeScript errors on canonical properties.
The static analysis reports that canonical.node and canonical.key don't exist on CanonicalOpV2 | { type: string; }. After confirming canonical is a NodePropSet instance (line 124), TypeScript needs help narrowing the type.
🔧 Proposed fix using type assertion
it('normalizeRawOp converts PropSet (node) to NodePropSet instance', () => {
const raw = createPropSetV2('user:alice', 'name', 'Alice');
const canonical = normalizeRawOp(raw);
expect(canonical).toBeInstanceOf(NodePropSet);
- expect(canonical.type).toBe('NodePropSet');
- expect(canonical.node).toBe('user:alice');
- expect(canonical.key).toBe('name');
- expect(canonical.value).toBe('Alice');
+ const np = /** `@type` {import('../../../../../src/domain/types/ops/NodePropSet.js').default} */ (canonical);
+ expect(np.type).toBe('NodePropSet');
+ expect(np.node).toBe('user:alice');
+ expect(np.key).toBe('name');
+ expect(np.value).toBe('Alice');
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('normalizeRawOp converts PropSet (node) to NodePropSet instance', () => { | |
| const raw = createPropSetV2('user:alice', 'name', 'Alice'); | |
| const canonical = normalizeRawOp(raw); | |
| expect(canonical).toBeInstanceOf(NodePropSet); | |
| expect(canonical.type).toBe('NodePropSet'); | |
| expect(canonical.node).toBe('user:alice'); | |
| expect(canonical.key).toBe('name'); | |
| expect(canonical.value).toBe('Alice'); | |
| }); | |
| it('normalizeRawOp converts PropSet (node) to NodePropSet instance', () => { | |
| const raw = createPropSetV2('user:alice', 'name', 'Alice'); | |
| const canonical = normalizeRawOp(raw); | |
| expect(canonical).toBeInstanceOf(NodePropSet); | |
| const np = /** `@type` {import('../../../../../src/domain/types/ops/NodePropSet.js').default} */ (canonical); | |
| expect(np.type).toBe('NodePropSet'); | |
| expect(np.node).toBe('user:alice'); | |
| expect(np.key).toBe('name'); | |
| expect(np.value).toBe('Alice'); | |
| }); |
🧰 Tools
🪛 GitHub Check: preflight
[failure] 127-127:
Property 'key' does not exist on type 'CanonicalOpV2 | { type: string; }'.
[failure] 126-126:
Property 'node' does not exist on type 'CanonicalOpV2 | { type: string; }'.
🪛 GitHub Check: type-firewall
[failure] 127-127:
Property 'key' does not exist on type 'CanonicalOpV2 | { type: string; }'.
[failure] 126-126:
Property 'node' does not exist on type 'CanonicalOpV2 | { type: string; }'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/domain/types/ops/factory-integration.test.js` around lines 120 -
129, The test's `canonical` variable is typed as a union so TypeScript can't see
`node`/`key` even after the runtime `toBeInstanceOf` check; narrow the type
before accessing properties by casting `canonical` to `NodePropSet` (e.g., use a
type assertion like `canonical as NodePropSet` or assign `const nodeProp =
canonical as NodePropSet`) and then assert `nodeProp.node`, `nodeProp.key`, and
`nodeProp.value`; keep using `normalizeRawOp`, `NodePropSet`, and the
`canonical` variable names to locate the change.
Code review (12 issues, 0 remaining): C1/M2/m1/m2: Add assertNoReservedBytes to all op constructors — rejects NUL (\x00) and \x01 prefix in identifiers. Matches PatchBuilderV2._assertNoReservedBytes. PropSet exempted (legitimately carries \x01-prefixed nodes for edge prop wire encoding). M1: Design doc Status DESIGN → HILL MET. Added Delivered vs. Deferred table clarifying which scope items shipped. n1: Blank line between frontmatter fields in design doc. n2: Witness references test descriptions instead of line numbers. n3: Retro file count 9 → 10. n4: WarpTypesV2 import alias PropSetOpClass → PropSetClass. 15 new validation tests. 5519 total tests passing. Cycle 0009 — op-type-class-hierarchy
Use case: mirroring git history into a warp graph. HEAD snapshot is fast, but full history backfill is O(commits). Explores edge-based chronological ordering, checkpoint seeding, wormhole compression, and lazy materialization with continuation tokens. Cycle 0009 — op-type-class-hierarchy
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/domain/types/WarpTypesV2.js`:
- Around line 160-162: The JSDoc `@returns` annotations still reference
plain-object typedefs (e.g., OpV2NodeAdd) while the factory functions like
createNodeAddV2, createNodeRemoveV2, createEdgeAddV2, createEdgeRemoveV2,
createPropSetV2, createNodePropSetV2, and createEdgePropSetV2 now return class
instances (NodeAddClass, NodeRemoveClass, EdgeAddClass, EdgeRemoveClass,
PropSetClass, NodePropSetClass, EdgePropSetClass); update each function's
`@returns` tag to reference the corresponding class type (e.g., `@returns`
{NodeAddClass}) OR change the typedefs to alias the classes (e.g., `@typedef`
{NodeAddClass} OpV2NodeAdd) so TypeScript sees them as compatible.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43fbdd8b-5655-418b-9c1d-4e366037d012
📒 Files selected for processing (15)
docs/design/0009-op-type-class-hierarchy/op-type-class-hierarchy.mddocs/design/0009-op-type-class-hierarchy/witness.mddocs/method/backlog/cool-ideas/PROTO_incremental-history-backfill.mddocs/method/retro/0009-op-type-class-hierarchy/retro.mdsrc/domain/types/WarpTypesV2.jssrc/domain/types/ops/BlobValue.jssrc/domain/types/ops/EdgeAdd.jssrc/domain/types/ops/EdgePropSet.jssrc/domain/types/ops/EdgeRemove.jssrc/domain/types/ops/NodeAdd.jssrc/domain/types/ops/NodePropSet.jssrc/domain/types/ops/NodeRemove.jssrc/domain/types/ops/validate.jstest/unit/domain/types/ops/Op.test.jstest/unit/domain/types/ops/validate.test.js
✅ Files skipped from review due to trivial changes (6)
- src/domain/types/ops/NodePropSet.js
- docs/method/backlog/cool-ideas/PROTO_incremental-history-backfill.md
- docs/method/retro/0009-op-type-class-hierarchy/retro.md
- test/unit/domain/types/ops/validate.test.js
- docs/design/0009-op-type-class-hierarchy/witness.md
- docs/design/0009-op-type-class-hierarchy/op-type-class-hierarchy.md
🚧 Files skipped from review as they are similar to previous changes (6)
- src/domain/types/ops/EdgeAdd.js
- src/domain/types/ops/EdgePropSet.js
- src/domain/types/ops/NodeRemove.js
- src/domain/types/ops/validate.js
- src/domain/types/ops/EdgeRemove.js
- test/unit/domain/types/ops/Op.test.js
Comment on lines
160
to
162
| export function createNodeAddV2(node, dot) { | ||
| return { type: 'NodeAdd', node, dot }; | ||
| return new NodeAddClass(node, dot); | ||
| } |
There was a problem hiding this comment.
TypeScript type mismatch causing pipeline failure.
The factory functions now return class instances, but the @returns JSDoc annotations still reference the plain-object typedefs (e.g., OpV2NodeAdd). TypeScript sees these as incompatible because class instances have a prototype chain and additional structure that doesn't match the typedef's plain object shape.
Update the return type annotations to reference the class types, or adjust the typedefs to describe the classes.
Proposed fix: Update `@returns` to reference class types
/**
* Creates a NodeAdd operation with a dot
* `@param` {NodeId} node - Node ID to add
* `@param` {Dot} dot - Causal identifier for this add
- * `@returns` {OpV2NodeAdd} NodeAdd operation
+ * `@returns` {NodeAddClass} NodeAdd operation
*/
export function createNodeAddV2(node, dot) {
return new NodeAddClass(node, dot);
}Apply the same pattern to the other factory functions:
createNodeRemoveV2:@returns {NodeRemoveClass}createEdgeAddV2:@returns {EdgeAddClass}createEdgeRemoveV2:@returns {EdgeRemoveClass}createPropSetV2:@returns {PropSetClass}createNodePropSetV2:@returns {NodePropSetClass}createEdgePropSetV2:@returns {EdgePropSetClass}
Alternatively, if the typedefs are used elsewhere and need to remain as the "public API type", you could update them to use @typedef {NodeAddClass} OpV2NodeAdd so the typedef aliases the class.
Also applies to: 170-172, 182-184, 194-196, 207-209, 218-220, 231-233
🧰 Tools
🪛 GitHub Actions: Release Preflight (PR)
[error] 161-161: Typecheck failed (tsc --noEmit). TS2322: Type 'NodeAdd' is not assignable to type 'OpV2NodeAdd'.
🪛 GitHub Check: preflight
[failure] 161-161:
Type 'NodeAdd' is not assignable to type 'OpV2NodeAdd'.
🪛 GitHub Check: type-firewall
[failure] 161-161:
Type 'NodeAdd' is not assignable to type 'OpV2NodeAdd'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/types/WarpTypesV2.js` around lines 160 - 162, The JSDoc `@returns`
annotations still reference plain-object typedefs (e.g., OpV2NodeAdd) while the
factory functions like createNodeAddV2, createNodeRemoveV2, createEdgeAddV2,
createEdgeRemoveV2, createPropSetV2, createNodePropSetV2, and
createEdgePropSetV2 now return class instances (NodeAddClass, NodeRemoveClass,
EdgeAddClass, EdgeRemoveClass, PropSetClass, NodePropSetClass,
EdgePropSetClass); update each function's `@returns` tag to reference the
corresponding class type (e.g., `@returns` {NodeAddClass}) OR change the typedefs
to alias the classes (e.g., `@typedef` {NodeAddClass} OpV2NodeAdd) so TypeScript
sees them as compatible.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NodeAdd,NodeRemove,EdgeAdd,EdgeRemove,NodePropSet,EdgePropSet,PropSet,BlobValue) promoted to frozen classes with constructor validation andinstanceofdispatchOpclass (abstract) provides shared runtime identityWarpTypesV2.jsdelegate to class constructorsWhat's deferred
instanceofmigration (MessageSchemaDetector, presenter, TickReceipt) — backlogup-next/up-next/Test plan
Summary by CodeRabbit
New Features
createPatch()auto-materializes whenautoMaterializeis enabled, somaterialize()is not required before remove operations.Bug Fixes
Tests