Fix: unified frames no longer land stacked on insert#20
Merged
Conversation
A new "Drawing" (unified) diagram is created via createDiagramDocument(undefined, 'unified') from the home "New diagram" flow. That factory left both auto-layout frames at their model-default origin (0,0), so inserting a mind map and a flowchart dropped them on top of each other. The distinct origins only lived in createUnifiedDocument(), which the create flow never calls. Fix: seed the distinct frame origins (mind map 600,200 · flowchart 600,700) inside createDiagramDocument itself for a unified type, so every unified doc gets separated frames however it's built. createUnifiedDocument now just delegates. Frames stay freely movable afterwards. - schema.js: applyUnifiedFrameOrigins() called from createDiagramDocument when diagramType === unified; createUnifiedDocument delegates (no duplicate constants). - unifiedDocument.test.js: assert the plain factory (the actual create-flow path) also seeds the distinct origins — the existing test only covered createUnifiedDocument, which is why the stacking slipped through. Verified: vitest 101 pass; Playwright drive of the real app (create -> insert mind map -> insert flowchart) shows the two frames spatially separated instead of overlapping. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Confidence Score: 5/5Safe to merge — targeted fix with a direct regression test covering the exact code path that was broken. The change is small and focused: origin seeding moves one level up the call chain, createUnifiedDocument becomes a thin wrapper, and a new test guards the previously uncovered factory path. The existing 101-test suite still passes. No files require special attention. Reviews (1): Last reviewed commit: "Frappe Draw: unified frames no longer la..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+67
to
70
| if (document.mindmap) document.mindmap.origin = { x: 600, y: 200 } | ||
| if (document.flowchart) document.flowchart.origin = { x: 600, y: 700 } | ||
| return document | ||
| } |
There was a problem hiding this comment.
return document is unreferenced by the caller (mutation-in-place is what takes effect). Drop it to avoid implying the function is pure.
Suggested change
| if (document.mindmap) document.mindmap.origin = { x: 600, y: 200 } | |
| if (document.flowchart) document.flowchart.origin = { x: 600, y: 700 } | |
| return document | |
| } | |
| if (document.mindmap) document.mindmap.origin = { x: 600, y: 200 } | |
| if (document.flowchart) document.flowchart.origin = { x: 600, y: 700 } | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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
2 participants
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.
Problem (found during end-to-end QA)
Creating a new Drawing (unified) diagram and inserting a mind map + a flowchart dropped both frames at the same origin
(0,0), so they rendered on top of each other.Root cause: the home "New diagram" flow builds the doc via
createDiagramDocument(undefined, 'unified'), but the distinct frame origins only lived increateUnifiedDocument()— which the create flow never calls.Fix
Seed the distinct origins (mind map
600,200· flowchart600,700) insidecreateDiagramDocumentitself for a unified type, so every unified doc gets separated frames regardless of how it's constructed.createUnifiedDocumentnow just delegates. Frames remain freely movable.Tests
createDiagramDocument(undefined, 'unified')— the real create-flow path) seeds the distinct origins. The pre-existing test only exercisedcreateUnifiedDocument, which is exactly why the stacking slipped through.vitest run→ 101 pass.Verified in the running app
Playwright drive against the local bench: create → Insert → Mind map → Insert → Flowchart now shows the two frames spatially separated (before: overlapping). Screenshots captured; 9/9 QA checks green.
🤖 Generated with Claude Code