Repository navigation
Add a browser-based visual layout workbench - #7
Conversation
|
Warning Review limit reached
Next review available in: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
📝 WalkthroughWalkthroughChangesThe PR adds a versioned layout sidecar model and core APIs for parsing, reconciliation, geometry application, routing, grouping, and z-order. The web app gains an interactive SVG layout editor with persistence, history, toolbar controls, and export integration. SVG and PPTX export behavior is also updated. Layout sidecar and core processing
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
LJAYi
changed the title
268dbef to
71aa583
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
packages/core/src/pptx.ts (1)
482-501: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExact-mode SVG embedding still leaves
foreignObjectlabels untouched
svgStringToPptxBuffer(..., { mode: "exact" })now sends the raw SVG intoexactSvgToPptxBuffer, andsanitizeSvgForEmbedding()only strips active/external content — it does not normalizeforeignObjectlabels. Mermaid node/edge labels can still disappear in PowerPoint/LibreOffice if the SVG importer dropsforeignObject. Restore the normalization step or make the limitation explicit.🤖 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/core/src/pptx.ts` around lines 482 - 501, Update the exact-mode branch of svgStringToPptxBuffer so the SVG passed to exactSvgToPptxBuffer is normalized to replace or convert foreignObject labels into importer-compatible content before embedding. Preserve parsed diagnostics and generated output handling, and ensure exact mode no longer sends raw foreignObject labels that PowerPoint or LibreOffice may drop.
🧹 Nitpick comments (2)
packages/core/src/exporters/svg.ts (1)
231-250: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
dominant-baseline:middlefor multi-line labels is not reliably honored across tools, and the multi-line centering test was removed without replacement.Adobe Illustrator does not respect
dominant-baseline="middle"and instead renders it as the defaultbaseline, producing misaligned text — confirmed by Adobe's own bug tracker: dominant-baseline="middle" is not respected, and is instead rendered as dominant-baseline="baseline", producing misaligned text, which is displayed aligned normally by Edge, Firefox, Chrome. This project already treats Illustrator SVG rendering fidelity as a real constraint (see the "Illustrator-invisible SVG" concern instroke-consistency.test.ts), so relying ondominant-baseline:middlefor label centering risks visibly broken multi-line label positioning when opened in Illustrator.Separately,
exporters.test.tspreviously had a test validating vertical-centering baseline behavior for multi-line text; per the line-range change details it was removed and not replaced with an equivalent multi-line assertion (the remainingDIAGRAMfixture only has single-line labels). Consider adding a multi-line label test to lock in the newtspan/dymath, and re-evaluate whether an explicity-per-tspanapproach (as before) would be safer thandominant-baselinefor cross-tool fidelity.🤖 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/core/src/exporters/svg.ts` around lines 231 - 250, Update textSvg to avoid relying on dominant-baseline:middle for vertical centering, which is not honored consistently by Illustrator. Emit explicit vertical positions for each tspan using the existing centered startY and lineHeight calculations, and remove the unsupported baseline dependency. Restore a multi-line assertion in the exporter tests that verifies the generated positions and centering math.apps/web/src/layout/editor.ts (1)
989-993: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant collision computation on the drag hot path.
applyLayout()already computes collisions viaupdateCollisions()(Line 684), andnotify()recomputes the same O(n²)collisionKeysover a freshly-built map. Both run on everypointermovewhile dragging. Store the count duringupdateCollisionsand reuse it here.♻️ Reuse the computed collision count
private updateCollisions( entries: Map<string, { bounds: LayoutBounds; mode: string }>, ): void { const collisions = collisionKeys(entries); + this.collisionCount = collisions.size; for (const [key, handle] of this.nodeHandles) { handle.element.classList.toggle("layout-node-collision", collisions.has(key)); } }Then reference
this.collisionCountinnotify()instead of recomputing (default it to0and reset inclear()).🤖 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 `@apps/web/src/layout/editor.ts` around lines 989 - 993, Eliminate the duplicate collision computation in notify() by adding a collisionCount field initialized to 0, assigning it from the collision result in updateCollisions(), and using this.collisionCount for the emitted count. Reset collisionCount to 0 in clear() so stale values are not retained.
🤖 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 `@packages/core/src/pptx.ts`:
- Around line 482-501: Update the exact-mode branch of svgStringToPptxBuffer so
the SVG passed to exactSvgToPptxBuffer is normalized to replace or convert
foreignObject labels into importer-compatible content before embedding. Preserve
parsed diagnostics and generated output handling, and ensure exact mode no
longer sends raw foreignObject labels that PowerPoint or LibreOffice may drop.
---
Nitpick comments:
In `@apps/web/src/layout/editor.ts`:
- Around line 989-993: Eliminate the duplicate collision computation in notify()
by adding a collisionCount field initialized to 0, assigning it from the
collision result in updateCollisions(), and using this.collisionCount for the
emitted count. Reset collisionCount to 0 in clear() so stale values are not
retained.
In `@packages/core/src/exporters/svg.ts`:
- Around line 231-250: Update textSvg to avoid relying on
dominant-baseline:middle for vertical centering, which is not honored
consistently by Illustrator. Emit explicit vertical positions for each tspan
using the existing centered startY and lineHeight calculations, and remove the
unsupported baseline dependency. Restore a multi-line assertion in the exporter
tests that verifies the generated positions and centering math.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ada37fed-8ff3-4e89-ac05-66ff1565a91f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
README.mdapps/web/package.jsonapps/web/src/layout/arrange.tsapps/web/src/layout/editor.tsapps/web/src/layout/history.tsapps/web/src/layout/index.tsapps/web/src/main.tsapps/web/src/styles.cssapps/web/src/svg-viewer.tsapps/web/tests/e2e/layout-editor.spec.tsapps/web/tests/e2e/workbench.spec.tsapps/web/tests/layout/arrange.test.tsapps/web/tests/layout/history.test.tsdocs/adr/0001-forward-only-unified-ir.mddocs/architecture.mddocs/roadmap.mdpackages/core/README.mdpackages/core/src/exporters/svg.tspackages/core/src/index.tspackages/core/src/layout/apply.tspackages/core/src/layout/identity.tspackages/core/src/layout/index.tspackages/core/src/layout/reconcile.tspackages/core/src/layout/sidecar.tspackages/core/src/layout/types.tspackages/core/src/pptx-connector-patch.tspackages/core/src/pptx.tspackages/core/src/stroke-style.tspackages/core/test/core.test.tspackages/core/test/exporters/exporters.test.tspackages/core/test/exporters/stroke-consistency.test.tspackages/core/test/layout.test.tspackages/core/test/pptx-groups-z-order.test.tspackages/core/test/public-api.test.ts
1 participant
Summary
Scope
This PR keeps the visual layout workbench isolated from the export foundation delivered in PR #6. Reverse conversion remains deferred.
Testing
Summary by CodeRabbit