Skip to content

Add History type and merge-commit model to graph-storage#4238

Open
TrueDoctor wants to merge 4 commits into
masterfrom
upstream-graph-storage-history
Open

Add History type and merge-commit model to graph-storage#4238
TrueDoctor wants to merge 4 commits into
masterfrom
upstream-graph-storage-history

Conversation

@TrueDoctor

Copy link
Copy Markdown
Member

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the history tracking mechanism in the CRDT graph storage by introducing a dedicated History struct to manage the append-only DAG of committed deltas in topological order. It updates Rev to wrap NonZeroU128 for niche optimization, changes Delta to reference a single primary parent (with an optional Merge variant for divergent branches), and updates Session and Document accordingly. Feedback on the changes suggests avoiding the unstable let_chains feature in session.rs to maintain stable Rust compatibility, and using push instead of merge for single deltas to prevent redundant topological sorting overhead.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread document/graph-storage/src/session.rs
Comment thread document/graph-storage/src/session.rs Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found and verified against the latest diff

Confidence score: 3/5

  • In document/graph-storage/src/document.rs, the history traversal used by restoration ignores secondary parents from merges, so valid deltas may be skipped and merged documents can fail apply/restore with not-in-history or missing-target errors; include merge secondary parents in traversal (and add a merge-history regression test) before merging.

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread document/graph-storage/src/document.rs Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 7 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant