🦋 Changeset detected

Latest commit: a83602d

The changes in this PR will be included in the next version bump.

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

No actionable comments were generated in the recent review. 🎉

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 help to get the list of available commands and usage tips.}

@sraphaz Hey Raphael, can you review this one?

Review: fix: person shape renders as ellipse in Draw.io export (#2682)

We contributed the initial Draw.io export/import feature to LikeC4 and were asked to review this PR. We take that responsibility seriously and have reviewed the change with care for correctness, round-trip integrity, and alignment with the project’s guidelines.

🌟 Recognition

Thank you for addressing #2679 and for the clear PR description and commit messages.

What this PR does: Elements with shape person in LikeC4 were exporting to Draw.io as shape=umlActor. The UML actor shape is a stick figure inside an ellipse; with our export’s HTML content rendering (overflow=fill;html=1;), the HTML fills the ellipse and hides the figure, so users only saw an ellipse. This PR switches the export to Draw.io’s built-in shape=actor (a person silhouette that works correctly with HTML content) and updates the parser so that re-imported diagrams using shape=actor are still recognized as actors. The fix is minimal, well-scoped, and includes the quote-handling improvement in findOpenTagEnd for robustness.

We recognize the care put into the implementation, the explanation of the root cause, and the inclusion of the parser change so that export and import stay in sync.

🔍 Our view on the integrity of the solution

From the perspective of maintainability and round-trip behavior:

✅Correctness: The change fixes the reported bug. Export now produces a shape that renders as intended in Draw.io, and import still classifies those cells as actor, so behavior is correct.
✅ Minimal scope: Only the person/actor path is touched; the rest of the Draw.io pipeline is unchanged. This matches the repo’s preference for focused, single-purpose changes.
✅ Round-trip behavior: After this PR, “export person → edit in Draw.io → re-import” yields name = actor 'title' as expected. Semantically, the round-trip is already correct because inferKind returns actor for shape=actor.
✅ Tests and snapshots: Running pnpm vitest run packages/generators/src/drawio/generate-drawio.spec.ts (and the parse-drawio specs) is appropriate; any snapshots that include actor/person elements should be updated to expect shape=actor instead of shape=umlActor in the generated XML.

🎉 We consider the solution sound and ready to merge from a correctness and integrity standpoint.

⚙️ Technical review

✅Export (generate-drawio.ts)
Replacing shape=umlActor;verticalLabelPosition=bottom;verticalAlign=top; with shape=actor; for the person shape is the right fix. The simpler style string avoids the ellipse-vs-HTML conflict and matches Draw.io’s recommended shape for this use case. Dropping the label-position modifiers is consistent with the new shape.

✅Import (parse-drawio.ts)
Extending inferKind so that shape=actor is recognized (e.g. s.includes('umlactor') || s.includes('shape=person') || s.includes('shape=actor')) ensures that cells exported with the new format are still classified as actor on re-import. Without this, they would fall through to container. The change is in the right place and keeps export and import aligned.

✅Quote handling in findOpenTagEnd
Handling both single- and double-quoted attributes so that a > inside a quoted value does not end the tag improves robustness and avoids misparsing in edge cases. This is a solid, defensive improvement.

✅Conventions
The change follows the repo’s TypeScript style, keeps the change local to the person/actor path, and fits the existing structure of drawioShape and inferKind. Commit messages follow the conventional style used in the project.

💡 Optional enhancement: explicit shape person on re-import (round-trip fidelity)

We support merging this PR as-is. If you are open to a small, optional improvement for round-trip fidelity, we suggest the following.

Current behavior after this PR:

• Export: person → shape=actor.
• Re-import: shape=actor → inferKind → actor → emitted as name = actor 'title' (and optionally a body). The element is correct; in LikeC4, actor implies the person shape by default.

Possible improvement:
Today, inferShape() in parse-drawio.ts only returns 'cylinder', 'document', or 'rectangle'. It does not return 'person' for actor-style cells. So when we re-import a .drawio that contains an actor (with shape=actor), we do not emit an explicit style { shape person } in the .c4 source. The diagram still renders correctly because the default for actor is the person shape, but the source would be more faithful to the diagram if we emitted the shape explicitly.

Suggestion: Extend inferShape() to return 'person' when the cell’s style indicates an actor shape, so that re-imported actor cells get an explicit style { shape person } in the emitted .c4. That would make the round-trip symmetric: export person → shape=actor; import shape=actor → actor with explicit shape person.

Concrete change (optional): In inferShape() in parse-drawio.ts, add a branch before the existing shape checks, for example:

function inferShape(style: string | undefined): string | undefined {
  if (!style) return undefined
  const s = style.toLowerCase()
  // Actor/person shape (export uses shape=actor; legacy may have shape=person or umlactor)
  if (s.includes('shape=actor') || s.includes('shape=person') || s.includes('umlactor')) return 'person'
  if (s.includes('shape=cylinder') || s.includes('cylinder3')) return 'cylinder'
  // ... rest unchanged
}

Then pushElementStyleBlock would emit shape person for such cells when building the element body, and the generated .c4 would explicitly reflect the shape. This is purely additive and does not change semantics; it only makes the round-trip more explicit. If you prefer to keep this PR minimal, we are happy to propose this as a small follow-up PR after merge.

🎁 Responsibility and care

In this review we aimed to:

• Verify correctness: Confirm that the fix resolves the issue and does not introduce regressions in export or import.
• Check round-trip integrity: Ensure that export and import stay aligned so that “export → edit in Draw.io → re-import” preserves intent.
• Respect project norms: Focused change, clear commits, tests run, and no unnecessary scope creep.
• Propose improvements only where useful: The optional inferShape suggestion is for extra fidelity, not a requirement for approval.

We are satisfied with the solution and recommend merging. We are grateful for the fix and for the opportunity to review; we are happy to follow up with a small PR for the optional inferShape enhancement if that would be helpful.