Skip to content

refactor: implement custom diagram layout algorithm and remove elkjs dependency#14

Merged
CoolSpring8 merged 3 commits into
mainfrom
replace-elk-with-custom
Nov 21, 2025
Merged

refactor: implement custom diagram layout algorithm and remove elkjs dependency#14
CoolSpring8 merged 3 commits into
mainfrom
replace-elk-with-custom

Conversation

@CoolSpring8
Copy link
Copy Markdown
Owner

@CoolSpring8 CoolSpring8 commented Nov 21, 2025

  • Eliminated the elkjs library from package.json and bun.lock.
  • Refactored DiagramView to remove references to elkjs, simplifying the layout logic.
  • Adjusted column gap in DiagramView for improved spacing.

Summary by CodeRabbit

  • Chores

    • Removed an unused external layout dependency from project configuration.
  • Refactor

    • Reworked diagram layout to compute positions internally with lane-based, deterministic placement and no-graph handling.
    • Increased horizontal spacing between diagram columns.
    • Simplified edge/node construction and enhanced active-path highlighting for clearer visuals.

✏️ Tip: You can customize this high-level summary in your review settings.

…dependency

- Eliminated the elkjs library from package.json and bun.lock.
- Refactored DiagramView to remove references to elkjs, simplifying the layout logic.
- Adjusted column gap in DiagramView for improved spacing.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Nov 21, 2025

Walkthrough

Migrates diagram layout from ELK to an in-component deterministic lane-based layout, removes the elkjs dependency and its ambient type declaration, and updates layout computation, node/edge construction, and spacing logic accordingly.

Changes

Cohort / File(s) Summary
Dependency removal
\package.json``
Removed elkjs from dependencies.
Layout engine replacement
\src/components/DiagramView.tsx``
Replaced ELK-based layout with an in-component lane-aware layout: early exit for no internal nodes, deterministic sorting (depth → lane → createdAt → id), lane allocation/fallback/expansion, computed x/y positions, direct Node/Edge mapping, updated active/inactive styling, columnGap changed from 64 → 80, and removed ELK error/cancel flow.
Type definitions cleanup
\src/types/elk.d.ts``
Removed ambient module declaration for "elkjs/lib/elk.bundled.js" (export re-exports for elkjs deleted).

Sequence Diagram

sequenceDiagram
    participant DV as DiagramView
    participant Data as Source Nodes/Edges
    participant Layout as In-component Layout
    participant Render as Renderer

    DV->>Data: Inspect nodes (internal vs external)
    alt no internal nodes
        DV->>DV: Clear layoutNodes/layoutEdges
        DV->>Render: Render empty/early exit
    else has internal nodes
        DV->>Layout: Allocate lanes (with fallback/expand)
        Layout-->>DV: Lane assignments
        DV->>Layout: Sort nodes (depth→lane→createdAt→id)
        Layout-->>DV: Sorted nodes
        DV->>Layout: Compute x (lane) / y (depth) positions (colGap=80)
        Layout-->>DV: Positioned nodes
        DV->>DV: Build Node & Edge entries with active/inactive styling
        DV->>Render: Render positioned nodes & edges
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Focus review on: src/components/DiagramView.tsx (lane allocation, sorting, position math, active-path styling, performance for large graphs).
  • Verify package change and types removal don't leave dangling imports/usages of elkjs.
  • Confirm visual/regression tests or snapshots for spacing change (64 → 80).

Possibly related PRs

  • fix: render DiagramView minimap nodes #11 — Changes DiagramView node handling (adds onNodesChange/applyNodeChanges and minimap color callbacks); likely touches the same component state flows and complements layout replacement.

Poem

🐰 We hopped from ELK to lanes so neat,
Placed each node with a steady beat,
Depths and lanes in tidy rows,
No bundled crate where wild code grows,
A little hop, a tidy feat!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: implementing a custom diagram layout algorithm and removing the elkjs dependency, which are the primary objectives of the PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch replace-elk-with-custom

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/components/DiagramView.tsx (1)

201-305: Custom layout effect is sound; consider minor refactors for clarity

The new layout effect correctly:

  • Bails out cleanly when there are no internal nodes, resetting layoutNodes/layoutEdges.
  • Produces a deterministic ordering by (depth, lane, createdAt, id).
  • Uses laneAssignments/depthByNode consistently to compute positions and active-path styling for nodes and edges.

Two optional refinements you might consider:

  • The getLane helper is only expected to allocate in pathological cases (since laneAssignments already gives every node a lane). You could turn that path into a stricter dev-only assertion to catch inconsistencies earlier, rather than silently assigning a new lane.
  • To keep the comparator obviously side‑effect‑free, you could precompute lanes for all internalNodes before sort (e.g., build a flat Map<id, lane> and use it read‑only inside the comparator). The current approach is functionally fine but a small refactor would make the intent clearer.

Otherwise the in‑component layout logic looks coherent and matches the stated PR goals.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7fa1c80 and fd44e59.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (3)
  • package.json (0 hunks)
  • src/components/DiagramView.tsx (2 hunks)
  • src/types/elk.d.ts (0 hunks)
💤 Files with no reviewable changes (2)
  • package.json
  • src/types/elk.d.ts
🔇 Additional comments (1)
src/components/DiagramView.tsx (1)

29-31: Column gap change looks good and consistent with layout math

The updated columnGap = 80 is wired correctly into the position formula (x: lane * (boxSize.width + columnGap)), so the only impact is the intended horizontal spacing tweak; no behavioural issues here.

…ance

- Introduced a laneLookup map to cache lane values for internal nodes, reducing redundant function calls.
- Updated sorting logic to utilize cached lane values, enhancing the efficiency of node ordering.
- Adjusted lane retrieval in node rendering to use the new lookup, maintaining existing functionality.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/components/DiagramView.tsx (2)

201-235: Empty-graph guard and lane fallback logic look robust

The new effect correctly:

  • Short-circuits to an empty layout when there are no treeNodes, avoiding stale nodes/edges.
  • Clones laneAssignments and derives nextLane from the current maximum, then uses getLane to lazily allocate lanes for any node missing an assignment, with a dev‑only warning. This should keep layout stable even while the tree is in a transient/inconsistent state.

If you want to trim a tiny bit of duplication, you could skip the extra laneLookup map and just rely on laneById directly in the sorter and node mapping, since getLane already guarantees every node ends up with an entry.


237-307: Deterministic sort and Node/Edge construction are correct and type-safe

The (depth, lane, createdAt, id) sort yields a stable, predictable ordering, and using depth/lane to compute position gives a clean grid layout. The Node/Edge objects:

  • Respect React Flow’s expectations (id, position, data, style, source, target, type, animated).
  • Apply active-path styling via activePathIds in a straightforward way.
  • Use satisfies Node / satisfies Edge to keep the typings honest.

If this JSX label block grows further, consider extracting it into a small presentational component for readability, but it’s perfectly fine as-is.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fd44e59 and 42036af.

📒 Files selected for processing (1)
  • src/components/DiagramView.tsx (2 hunks)
🔇 Additional comments (1)
src/components/DiagramView.tsx (1)

29-30: Column gap increase is safe and improves readability

Bumping columnGap to 80 cleanly widens horizontal spacing without affecting layout logic (positions are already computed from boxSize.width + columnGap), so this is a low‑risk, purely visual tweak.

@CoolSpring8 CoolSpring8 merged commit bbc79b8 into main Nov 21, 2025
1 check passed
@CoolSpring8 CoolSpring8 deleted the replace-elk-with-custom branch November 21, 2025 14:59
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