Implement v18 property projection reads#99
Merged
Conversation
📝 WalkthroughWalkthroughImplements V18 property projection: runtime-backed legacy key/value nouns and visible record types, deterministic NodePropertyProjection and EdgePropertyProjection services, QueryReads refactor to consume projections, unit tests, and docs/changelog updates. ChangesV18 Property Projection Implementation
Sequence Diagram(s)sequenceDiagram
participant Client as QueryReads
participant Projection as Node/EdgePropertyProjection
participant State as WarpState
participant Record as Visible{Node|Edge}PropertyRecord
Client->>Projection: request projected props (forNode/forEdge/fromState)
Projection->>State: iterate state.prop / decode legacy keys
State-->>Projection: LWW registers
Projection->>Projection: filter/validate keys, birth-event checks
Projection->>Record: construct Visible property records
Projection-->>Client: frozen, sorted Visible property records
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Release Preflight
If you tag this commit as |
Member
Author
|
@codex Self-review findings for PR #99.
|
Member
Author
|
@codex Resolution summary for the self-review findings.
Verification run locally before push and again through the pre-push firewall:
|
Release Preflight
If you tag this commit as |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/design/0176-v18-node-property-projection/v18-node-property-projection.md (1)
107-107:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winVerification path doesn't match observed output.
Line 107 specifies
test/unit/domain/graph/NodePropertyProjection.test.ts, but the observed output at line 117 shows the test is actually located attest/unit/domain/services/NodePropertyProjection.test.ts. The verification command should reflect the actual file location.📝 Proposed fix
-npx vitest run test/unit/domain/graph/NodePropertyProjection.test.ts --reporter=verbose +npx vitest run test/unit/domain/services/NodePropertyProjection.test.ts --reporter=verbose🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/0176-v18-node-property-projection/v18-node-property-projection.md` at line 107, Update the verification command to point to the actual test file location: replace the path test/unit/domain/graph/NodePropertyProjection.test.ts with test/unit/domain/services/NodePropertyProjection.test.ts in the npx vitest run invocation so the command runs the NodePropertyProjection.test.ts under the services directory.docs/design/0177-v18-edge-property-projection/v18-edge-property-projection.md (1)
105-105:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winVerification path doesn't match observed output.
Line 105 specifies
test/unit/domain/graph/EdgePropertyProjection.test.ts, but the observed output at line 115 shows the test is located attest/unit/domain/services/EdgePropertyProjection.test.ts. The verification command should use the correct path.📝 Proposed fix
-npx vitest run test/unit/domain/graph/EdgePropertyProjection.test.ts --reporter=verbose +npx vitest run test/unit/domain/services/EdgePropertyProjection.test.ts --reporter=verbose🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/0177-v18-edge-property-projection/v18-edge-property-projection.md` at line 105, The verification command references the wrong test path; update the npx vitest run command in EdgePropertyProjection documentation to point to the actual test file location by replacing test/unit/domain/graph/EdgePropertyProjection.test.ts with test/unit/domain/services/EdgePropertyProjection.test.ts so the command runs the observed test (EdgePropertyProjection.test.ts) in the services folder.
🧹 Nitpick comments (2)
src/domain/services/NodePropertyProjection.ts (1)
38-45: ⚖️ Poor tradeoffTargeted reads allocate all visible property records.
forNodecallsfromState()which iterates and constructs all visible node property records, then filters to the requested owner. For graphs with many nodes and properties, this allocates and discards records unnecessarily.♻️ Potential optimization: targeted iteration path
Add a targeted iteration path that constructs only records matching the requested node ID:
static forNode(state: WarpState, nodeId: string | NodeId): readonly VisibleNodePropertyRecord[] { const checkedState = requireWarpState(state); const owner = nodeRecordForProjectionTarget(checkedState, nodeId); if (owner === null) { return Object.freeze([]); } const targetNodeIdStr = owner.id.toString(); const records: VisibleNodePropertyRecord[] = []; // Iterate only once and construct only matching records for (const [encodedKey, register] of checkedState.prop) { if (!encodedKey.startsWith(targetNodeIdStr)) continue; const record = nodePropertyRecordForOwnerRegister(owner, encodedKey, register); if (record !== null) { records.push(record); } } records.sort(compareNodePropertyRecords); return Object.freeze(records); }Keep
fromState()for whole-graph enumeration.Based on learnings: Reviewer self-review flagged that forNode()/forEdge() call fromState() and filter complete result set, causing targeted reads to allocate every visible property record instead of constructing only requested-owner records.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/services/NodePropertyProjection.ts` around lines 38 - 45, forNode currently builds the entire visible-property projection via fromState() and then filters, causing wasteful allocations; change it to a targeted iteration: after resolving checkedState with requireWarpState and the owner via nodeRecordForProjectionTarget, iterate checkedState.prop and only process entries whose encoded key begins with owner.id.toString(), call nodePropertyRecordForOwnerRegister(owner, encodedKey, register) for matches, collect non-null records, sort with compareNodePropertyRecords, freeze and return; keep fromState() unchanged for full-graph enumeration.src/domain/services/EdgePropertyProjection.ts (1)
57-67: ⚖️ Poor tradeoffTargeted reads allocate all visible property records.
forEdgedelegates tofromState()via the code path at line 66, which iterates and constructs all visible edge property records before filtering to the requested owner. For graphs with many edges and properties, this allocates and discards records unnecessarily.Same performance pattern as
NodePropertyProjection.forNode. Consider adding a targeted iteration path that constructs only records matching the requested edge (filter by encoded key prefix early in the loop). KeepfromState()for whole-graph enumeration.Based on learnings: Reviewer self-review flagged that forNode()/forEdge() call fromState() and filter complete result set, causing targeted reads to allocate every visible property record instead of constructing only requested-owner records.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/services/EdgePropertyProjection.ts` around lines 57 - 67, forEdge currently delegates to forEdgeRecord/fromState and causes allocation of all visible property records then filters by owner; implement a targeted iteration that only constructs records for the requested edge by scanning the state/store with the encoded owner key prefix and building VisibleEdgePropertyRecord on-the-fly. Modify EdgePropertyProjection.forEdge to call a new helper (e.g. forEdgeTargeted or forEdgeFromStatePrefix) that uses requireWarpState(state) and edgeRecordForProjectionTarget(edge) to compute the prefix, then iterates the underlying store/entries producing only matching records; keep the existing fromState() intact for full-graph enumeration and mirror the same change pattern used for NodePropertyProjection.forNode to maintain symmetry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/domain/services/EdgePropertyProjection.ts`:
- Around line 109-118: The tolerant resolver edgeRecordForProjectionTarget
currently calls EdgeRecord.fromLegacyEdge(edge) which throws on malformed
NodeId/EdgeTypeId; change it to avoid constructing EdgeRecord unless inputs are
valid by wrapping the conversion in a try-catch (or validate fields first) so
malformed/invalid legacy edges cause the function to return null instead of
throwing; specifically update edgeRecordForProjectionTarget to catch errors from
EdgeRecord.fromLegacyEdge(edge) and return null on any exception, then call
state.getEdgeRecord(...) only when conversion succeeds.
- Around line 130-138: The loop in EdgePropertyProjection that calls
EdgeRecord.fromLegacyEdge(keyParts) can throw on malformed identifiers and
should skip invalid entries instead of throwing; update the projection (e.g., in
fromState or the method iterating state.prop) to either validate keyParts.from,
keyParts.to and keyParts.label before calling EdgeRecord.fromLegacyEdge or wrap
the construction and subsequent
state.getEdgeRecord(EdgeRecord.fromLegacyEdge(...).id) in a try-catch that
returns/continues null for that entry; ensure you still call encodeEdgeKey and
visibleEdgeRegister as before but gracefully ignore and continue on malformed
keys so a single bad entry does not abort the full projection.
In `@src/domain/services/NodePropertyProjection.ts`:
- Around line 177-186: Duplicate compareStrings implementation in
NodePropertyProjection.ts and EdgePropertyProjection.ts; extract it to a single
exported utility (e.g., export function compareStrings(left: string, right:
string): number) in a shared module such as src/domain/utils/StringComparison.ts
(or an existing string utilities module), replace the local implementations in
NodePropertyProjection and EdgePropertyProjection with imports from that new
module, and run type checks to ensure both projection files reference the same
exported symbol.
- Around line 83-88: The function nodeRecordForProjectionTarget currently defers
to state.getNodeRecord(nodeId) which can throw for malformed NodeId values;
update nodeRecordForProjectionTarget to catch exceptions from
state.getNodeRecord (or validate nodeId for empty/NUL/reserved prefixes before
calling) and return null on error so it remains tolerant for public query
APIs—refer to nodeRecordForProjectionTarget, state.getNodeRecord, and NodeId
(and note state.hasNodeRecord behavior) when making the change.
---
Outside diff comments:
In
`@docs/design/0176-v18-node-property-projection/v18-node-property-projection.md`:
- Line 107: Update the verification command to point to the actual test file
location: replace the path test/unit/domain/graph/NodePropertyProjection.test.ts
with test/unit/domain/services/NodePropertyProjection.test.ts in the npx vitest
run invocation so the command runs the NodePropertyProjection.test.ts under the
services directory.
In
`@docs/design/0177-v18-edge-property-projection/v18-edge-property-projection.md`:
- Line 105: The verification command references the wrong test path; update the
npx vitest run command in EdgePropertyProjection documentation to point to the
actual test file location by replacing
test/unit/domain/graph/EdgePropertyProjection.test.ts with
test/unit/domain/services/EdgePropertyProjection.test.ts so the command runs the
observed test (EdgePropertyProjection.test.ts) in the services folder.
---
Nitpick comments:
In `@src/domain/services/EdgePropertyProjection.ts`:
- Around line 57-67: forEdge currently delegates to forEdgeRecord/fromState and
causes allocation of all visible property records then filters by owner;
implement a targeted iteration that only constructs records for the requested
edge by scanning the state/store with the encoded owner key prefix and building
VisibleEdgePropertyRecord on-the-fly. Modify EdgePropertyProjection.forEdge to
call a new helper (e.g. forEdgeTargeted or forEdgeFromStatePrefix) that uses
requireWarpState(state) and edgeRecordForProjectionTarget(edge) to compute the
prefix, then iterates the underlying store/entries producing only matching
records; keep the existing fromState() intact for full-graph enumeration and
mirror the same change pattern used for NodePropertyProjection.forNode to
maintain symmetry.
In `@src/domain/services/NodePropertyProjection.ts`:
- Around line 38-45: forNode currently builds the entire visible-property
projection via fromState() and then filters, causing wasteful allocations;
change it to a targeted iteration: after resolving checkedState with
requireWarpState and the owner via nodeRecordForProjectionTarget, iterate
checkedState.prop and only process entries whose encoded key begins with
owner.id.toString(), call nodePropertyRecordForOwnerRegister(owner, encodedKey,
register) for matches, collect non-null records, sort with
compareNodePropertyRecords, freeze and return; keep fromState() unchanged for
full-graph enumeration.
🪄 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: d9d52abc-6d17-4d71-9a92-2c025de1cb97
📒 Files selected for processing (26)
CHANGELOG.mddocs/BEARING.mddocs/design/0174-v18-post-25-property-projection-runway/v18-post-25-property-projection-runway.mddocs/design/0175-v18-legacy-property-projection-nouns/v18-legacy-property-projection-nouns.mddocs/design/0176-v18-node-property-projection/v18-node-property-projection.mddocs/design/0177-v18-edge-property-projection/v18-edge-property-projection.mddocs/design/0178-v18-query-property-projection-reads/v18-query-property-projection-reads.mdsrc/domain/graph/LegacyContentPropertyKeys.tssrc/domain/graph/LegacyEdgePropertyKey.tssrc/domain/graph/LegacyNodePropertyKey.tssrc/domain/graph/LegacyPropertyKeyClassification.tssrc/domain/graph/LegacyPropertyProjection.tssrc/domain/graph/LegacyPropertyValue.tssrc/domain/graph/VisibleEdgePropertyRecord.tssrc/domain/graph/VisibleNodePropertyRecord.tssrc/domain/graph/publicGraphSubstrate.tssrc/domain/services/EdgePropertyProjection.tssrc/domain/services/KeyCodec.tssrc/domain/services/LegacyPropertyProjectionTarget.tssrc/domain/services/NodePropertyProjection.tssrc/domain/services/controllers/QueryReads.tssrc/domain/types/PropValue.tstest/unit/domain/graph/LegacyPropertyProjection.test.tstest/unit/domain/services/EdgePropertyProjection.test.tstest/unit/domain/services/NodePropertyProjection.test.tstest/unit/domain/services/QueryReadsPropertyProjection.test.ts
Member
Author
|
@codex spot check please: can you review the latest follow-up commits ( |
Member
Author
|
@coderabbitai Feedback resolution pushed in
Verification:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me verify the key changes from 🏁 Script executed: Length of output: 24639
One non-blocking observation: The implementation is solid. Approving. [approve] |
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/design/0176-v18-node-property-projection/v18-node-property-projection.md (1)
114-125:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRefresh the verification transcript to match current test output.
The
Observed GREENsnippet still showsTests 2 passed (2), but the current suite in this slice includes additional tests. Please update the captured output so the design record stays accurate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/0176-v18-node-property-projection/v18-node-property-projection.md` around lines 114 - 125, Update the "Observed GREEN" transcript to reflect the current test run output: re-run the test command for NodePropertyProjection (npx vitest run test/unit/domain/services/NodePropertyProjection.test.ts --reporter=verbose) and replace the outdated "Tests 2 passed (2)" line and any other stale lines with the actual current output from that run, and also ensure subsequent linter/typecheck commands shown (eslint, npm run typecheck, npm run lint:sludge) reflect their current statuses for NodePropertyProjection.ts and NodePropertyProjection.test.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@docs/design/0176-v18-node-property-projection/v18-node-property-projection.md`:
- Around line 114-125: Update the "Observed GREEN" transcript to reflect the
current test run output: re-run the test command for NodePropertyProjection (npx
vitest run test/unit/domain/services/NodePropertyProjection.test.ts
--reporter=verbose) and replace the outdated "Tests 2 passed (2)" line and any
other stale lines with the actual current output from that run, and also ensure
subsequent linter/typecheck commands shown (eslint, npm run typecheck, npm run
lint:sludge) reflect their current statuses for NodePropertyProjection.ts and
NodePropertyProjection.test.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a7b0132-b8c4-4555-a52b-3339f17c4b6e
📒 Files selected for processing (9)
CHANGELOG.mddocs/design/0176-v18-node-property-projection/v18-node-property-projection.mddocs/design/0177-v18-edge-property-projection/v18-edge-property-projection.mdsrc/domain/services/EdgePropertyProjection.tssrc/domain/services/NodePropertyProjection.tssrc/domain/utils/StringComparison.tstest/unit/domain/services/EdgePropertyProjection.test.tstest/unit/domain/services/NodePropertyProjection.test.tstest/unit/domain/services/QueryReadsPropertyProjection.test.ts
✅ Files skipped from review due to trivial changes (3)
- src/domain/utils/StringComparison.ts
- CHANGELOG.md
- docs/design/0177-v18-edge-property-projection/v18-edge-property-projection.md
Member
Author
|
@coderabbitai Follow-up doc feedback resolved in
Verification for this docs-only follow-up:
|
|
Tip For best results, initiate chat on the files or code changes.
PR is ready to merge. 🐇 ✨ |
Release Preflight
If you tag this commit as |
Member
Author
|
@codex Code Lawyer audit activity summary for this branch.
Verification just completed locally:
|
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
WarpStatefactsVerification
ADR checks
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests