feat(gitGraph): add multi parent merge#7950
Open
WhiteFox0-0 wants to merge 6 commits into
Open
Conversation
🦋 Changeset detectedLatest commit: dc89e47 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
@mermaid-js/examples
mermaid
@mermaid-js/layout-elk
@mermaid-js/layout-tidy-tree
@mermaid-js/mermaid-zenuml
@mermaid-js/parser
@mermaid-js/tiny
commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughgitGraph merge handling now accepts multiple source branches in one statement. The grammar, types, parser mapping, AST merge logic, tests, demos, and changesets were updated to use ChangesMulti-branch merge support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant LangiumGrammar as gitGraph.langium
participant gitGraphParser as gitGraphParser
participant gitGraphAst as gitGraphAst
participant Demo as git.html
LangiumGrammar->>gitGraphParser: parse Merge branches array
gitGraphParser->>gitGraphAst: mergeDB.branches
gitGraphAst->>gitGraphAst: validate branches and build parents list
Demo->>Demo: render multi-branch merge examples
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7950 +/- ##
===========================================
+ Coverage 77.46% 77.54% +0.08%
===========================================
Files 564 564
Lines 74906 74919 +13
Branches 12661 12664 +3
===========================================
+ Hits 58026 58097 +71
+ Misses 15881 15823 -58
Partials 999 999
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
demos/git.html (1)
202-258: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew multi-branch merge examples look correct but TB/BT are less thorough than LR.
The LR example demonstrates a 3-way octopus merge (
merge feature1, feature2, feature3), while the TB and BT examples only demonstrate a 2-way merge (merge feature1, feature2). Consider extending TB/BT to also cover the 3-branch scenario for consistency and to give users a complete picture of octopus-merge rendering across all diagram orientations.🤖 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 `@demos/git.html` around lines 202 - 258, The multi-branch merge examples are inconsistent because the LR gitGraph demonstrates a 3-branch octopus merge while the TB and BT variants only show 2-branch merges. Update the Mermaid snippets in the multiple branch merge section so the TB and BT diagrams match the LR example by including the third feature branch and merging all three branches, keeping the examples aligned across orientations in the relevant gitGraph blocks.packages/parser/tests/gitGraph.test.ts (1)
128-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest title doesn't match the syntax under test.
The test is titled "should parse multiple branches separated by single space" but the input
merge feature1, feature2is comma-separated, not space-separated.✏️ Suggested rename
- it('should parse multiple branches separated by single space', () => { + it('should parse multiple branches separated by a comma', () => { const result = parse(`gitGraph\n merge feature1, feature2\n`);🤖 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 `@packages/parser/tests/gitGraph.test.ts` around lines 128 - 132, The test title in gitGraph.test.ts does not match the syntax being parsed: the Merge parsing case uses comma-separated branches via parse and Merge.branches, not single-space separation. Rename the test to describe comma-separated branches so it accurately reflects the input and expected output.packages/mermaid/src/diagrams/git/gitGraphAst.ts (1)
230-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCustom-id collision check is re-evaluated unnecessarily on every branch iteration.
customIdandstate.records.commitsdon't change between iterations, so this check yields the same result each time the loop runs; hoisting it above theforloop would avoid redundant work for merges with many branches.♻️ Proposed refactor: hoist the customId check out of the loop
const currentBranchCheck = state.records.branches.get(state.records.currBranch); const currentCommit = currentBranchCheck ? state.records.commits.get(currentBranchCheck) : undefined; + if (customId && state.records.commits.has(customId)) { + const error: any = new Error( + 'Incorrect usage of "merge". Commit with id:' + customId + ' already exists, use different custom id' + ); + error.hash = { + text: `merge ${otherBranches.join(' ')} ${customId} ${overrideType} ${customTags?.join(' ')}`, + token: `merge ${otherBranches.join(' ')} ${customId} ${overrideType} ${customTags?.join(' ')}`, + expected: [`merge ${otherBranches.join(' ')} ${customId}_UNIQUE ${overrideType} ${customTags?.join(' ')}`], + }; + throw error; + } + const verifiedBranches: string[] = []; for (const branch of otherBranches) { ... - if (customId && state.records.commits.has(customId)) { - const error: any = new Error( - 'Incorrect usage of "merge". Commit with id:' + - customId + - ' already exists, use different custom id' - ); - error.hash = { - text: `merge ${otherBranch} ${customId} ${overrideType} ${customTags?.join(' ')}`, - token: `merge ${otherBranch} ${customId} ${overrideType} ${customTags?.join(' ')}`, - expected: [ - `merge ${otherBranch} ${customId}_UNIQUE ${overrideType} ${customTags?.join(' ')}`, - ], - }; - - throw error; - }🤖 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 `@packages/mermaid/src/diagrams/git/gitGraphAst.ts` around lines 230 - 245, The custom-id collision check in the merge handling logic is being repeated on every branch iteration even though `customId` and `state.records.commits` are unchanged. Move the `customId && state.records.commits.has(customId)` validation, including the `Error` construction and `error.hash` setup, to before the `for` loop in the merge path so it runs once per merge instead of once per branch. Use the surrounding merge function in `gitGraphAst` to place the guard at the earliest point where `customId` is available.
🤖 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 @.changeset/tender-adults-see.md:
- Line 6: Fix the user-facing changelog typo in the changeset entry: replace
“muli-parent” with “multi-parent” in the release note text so the summary for
gitGraph is spelled correctly. This is a simple content-only update in the
changeset markdown, with no code changes needed.
In `@packages/mermaid/src/diagrams/git/gitGraphAst.ts`:
- Around line 175-187: The new self-merge guard in gitGraphAst’s merge handling
is redundant and overrides the existing structured error path. Remove the plain
Error check that compares currentCommit.branch to otherBranch, and keep the
state.records.currBranch === otherBranch branch so the merge() failure continues
to emit the structured error with error.hash metadata used by downstream error
handling.
---
Nitpick comments:
In `@demos/git.html`:
- Around line 202-258: The multi-branch merge examples are inconsistent because
the LR gitGraph demonstrates a 3-branch octopus merge while the TB and BT
variants only show 2-branch merges. Update the Mermaid snippets in the multiple
branch merge section so the TB and BT diagrams match the LR example by including
the third feature branch and merging all three branches, keeping the examples
aligned across orientations in the relevant gitGraph blocks.
In `@packages/mermaid/src/diagrams/git/gitGraphAst.ts`:
- Around line 230-245: The custom-id collision check in the merge handling logic
is being repeated on every branch iteration even though `customId` and
`state.records.commits` are unchanged. Move the `customId &&
state.records.commits.has(customId)` validation, including the `Error`
construction and `error.hash` setup, to before the `for` loop in the merge path
so it runs once per merge instead of once per branch. Use the surrounding merge
function in `gitGraphAst` to place the guard at the earliest point where
`customId` is available.
In `@packages/parser/tests/gitGraph.test.ts`:
- Around line 128-132: The test title in gitGraph.test.ts does not match the
syntax being parsed: the Merge parsing case uses comma-separated branches via
parse and Merge.branches, not single-space separation. Rename the test to
describe comma-separated branches so it accurately reflects the input and
expected output.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cb24213c-800f-4fab-b16b-338c805dd48c
📒 Files selected for processing (8)
.changeset/tender-adults-see.mddemos/git.htmlpackages/mermaid/src/diagrams/git/gitGraph.spec.tspackages/mermaid/src/diagrams/git/gitGraphAst.tspackages/mermaid/src/diagrams/git/gitGraphParser.tspackages/mermaid/src/diagrams/git/gitGraphTypes.tspackages/parser/src/language/gitGraph/gitGraph.langiumpackages/parser/tests/gitGraph.test.ts
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
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
Added support for octopus (multi-parent) merges in gitGraph. Updated the parser, database to allow multiple branches to merge into a single commit node
Resolves #7462
📏 Design Decisions
Extended the existing gitGraph model to support octopus (multi-parent) merges by allowing multiple parent relationships for a single commit. Which can be use like:
merge [branch_1], [branch_2] ...This matches the git octopus merge
Example
📋 Tasks
Make sure you
MERMAID_RELEASE_VERSIONis used for all new features.pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.Added octopus (multi-parent) merge support to
gitGraph, allowing a single merge commit node to accept multiple parent branches viamerge feature1, feature2, ...(resolving#7462).Highlights:
gitGraphgrammar and parser to parsemergestatements with one-or-more branch references separated by commas.gitGraphtypes and AST/DB models to represent merges asbranches: string[](instead of a singlebranch).parents: [currentHead, ...verifiedBranches].demos/git.htmlwith new “Multiple branch merge graph” examples (left-to-right, top-to-bottom, bottom-to-top).mermaidand@mermaid-js/parserminor version bumps and documenting the new feature.