Feat: Optimize collision resolution using depth-based partitioning#264
Feat: Optimize collision resolution using depth-based partitioning#264itvi-1234 wants to merge 7 commits into
Conversation
Signed-off-by: itvi-1234 <rjsumit71@gmail.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 47 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughBoxes are bucketed by Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
@AgniveshChaubey , could you please review it , Lemme know if any changes needed 🚀 |
Preview Deployed!
Last updated at 2026-04-03T09:03:04Z |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/resolveCollisions.ts (1)
57-87: Please codify the “depth == collision column” assumption.This loop now ignores collisions between different depths entirely. That’s safe only if every
resolveCollisionscaller guarantees distinct depths never share x-space; otherwise adjacent-depth wide nodes can now overlap without ever being separated. A small regression test or inline invariant note here would make that contract explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/resolveCollisions.ts` around lines 57 - 87, The loop assumes boxes in each depth bucket represent a single collision column; make that contract explicit and enforced: in resolveCollisions ensure depthBuckets is keyed by node depth and assert or guard that A.depth === B.depth before computing/handling overlaps (or throw/log a descriptive error if not), add a small regression test that creates two wide nodes at adjacent depths with overlapping x-ranges to verify the assertion triggers (or that callers pre-bucket by depth), and include an inline comment at the start of the nested loops referencing depthBuckets and the overlapThreshold to document the invariant; reference symbols: resolveCollisions, depthBuckets, overlapThreshold, A, B, moved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/utils/resolveCollisions.ts`:
- Around line 57-87: The loop assumes boxes in each depth bucket represent a
single collision column; make that contract explicit and enforced: in
resolveCollisions ensure depthBuckets is keyed by node depth and assert or guard
that A.depth === B.depth before computing/handling overlaps (or throw/log a
descriptive error if not), add a small regression test that creates two wide
nodes at adjacent depths with overlapping x-ranges to verify the assertion
triggers (or that callers pre-bucket by depth), and include an inline comment at
the start of the nested loops referencing depthBuckets and the overlapThreshold
to document the invariant; reference symbols: resolveCollisions, depthBuckets,
overlapThreshold, A, B, moved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64564782-48d7-45ce-9bcc-7ad761f18e0c
📒 Files selected for processing (2)
.changeset/little-bushes-pick.mdsrc/utils/resolveCollisions.ts
Signed-off-by: itvi-1234 <rjsumit71@gmail.com>
Signed-off-by: itvi-1234 <rjsumit71@gmail.com>
Signed-off-by: itvi-1234 <rjsumit71@gmail.com>
Signed-off-by: itvi-1234 <rjsumit71@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/utils/resolveCollisions.ts (1)
99-103: Add a comment documenting the depth-based horizontal separation assumption.The collision resolution algorithm only checks collisions within the same depth bucket and between adjacent depths (lines 103 and 108). This optimization relies on the depth-based X-offset in
GraphView.tsx—specifically the formula(NODE_WIDTH + HORIZONTAL_GAP) * node.depthwith NODE_WIDTH=172 and HORIZONTAL_GAP=150—which ensures non-adjacent depths are sufficiently separated horizontally (322 pixels apart) to prevent collisions. While this assumption is mathematically sound, documenting it in the code will help future maintainers understand the implicit coupling between this collision resolution strategy and the layout pipeline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/resolveCollisions.ts` around lines 99 - 103, Add a clarifying comment near the depth-based collision logic around depthBuckets and resolveBoxPairs explaining the optimization: mention that collisions are only checked within the same depth bucket and between adjacent depths because GraphView.tsx uses an X-offset of (NODE_WIDTH + HORIZONTAL_GAP) * node.depth (with NODE_WIDTH=172 and HORIZONTAL_GAP=150), which guarantees non-adjacent depths are at least 322px apart and cannot collide; reference depthBuckets, resolveBoxPairs, GraphView.tsx, NODE_WIDTH and HORIZONTAL_GAP in the comment so future maintainers know this function relies on that layout assumption.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/resolveCollisions.ts`:
- Line 91: The for-loop in resolveCollisions.ts inside the resolveCollisions
function uses "for (let iter = 0; iter <= maxIterations; iter++)" causing one
extra iteration; change the loop condition to stop after maxIterations
iterations (e.g., use "iter < maxIterations") or rename the parameter to reflect
the inclusive behavior, and update any callers/tests if needed so the function
performs exactly maxIterations iterations; ensure the unique symbol "iter" and
the loop in resolveCollisions are adjusted consistently.
---
Nitpick comments:
In `@src/utils/resolveCollisions.ts`:
- Around line 99-103: Add a clarifying comment near the depth-based collision
logic around depthBuckets and resolveBoxPairs explaining the optimization:
mention that collisions are only checked within the same depth bucket and
between adjacent depths because GraphView.tsx uses an X-offset of (NODE_WIDTH +
HORIZONTAL_GAP) * node.depth (with NODE_WIDTH=172 and HORIZONTAL_GAP=150), which
guarantees non-adjacent depths are at least 322px apart and cannot collide;
reference depthBuckets, resolveBoxPairs, GraphView.tsx, NODE_WIDTH and
HORIZONTAL_GAP in the comment so future maintainers know this function relies on
that layout assumption.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1aa6157-a1ff-48f2-9a45-43b18c7d9ca0
📒 Files selected for processing (1)
src/utils/resolveCollisions.ts
Signed-off-by: itvi-1234 <rjsumit71@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/utils/resolveCollisions.ts (1)
96-96:⚠️ Potential issue | 🟡 MinorLoop still runs
maxIterations + 1passes.
Line 96usesiter <= maxIterations, somaxIterations: 0still performs one relaxation pass and every other value gets one extra iteration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/resolveCollisions.ts` at line 96, The for-loop in resolveCollisions uses "for (let iter = 0; iter <= maxIterations; iter++)" which makes it run one extra pass (maxIterations + 1); change the loop condition to "iter < maxIterations" (or adjust the initialization if you want 1-based counts) so that when maxIterations is 0 no relaxation pass runs. Update the loop condition referencing iter and maxIterations inside the resolveCollisions function accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/resolveCollisions.ts`:
- Around line 47-52: The current depth-bucketing in resolveCollisions groups
boxes by box.node.depth and only compares a bucket with depth+1, which misses
geometric overlaps when a wide box spans further depths; update the collision
resolution in resolveCollisions (the depthBuckets Map logic and the loop that
compares buckets around lines where depthBuckets is used) to first compute each
box's x-range from its x/width and only compare buckets whose overall x-ranges
intersect (i.e., gate comparisons by actual x-range overlap rather than assuming
only depth+1 can collide), expanding the neighbor-depth checks when ranges
overlap; also apply the same fix to the similar logic around the other
occurrence (the code noted at 99-109) and add a regression test that constructs
a wide node spanning depth+2 to assert it gets collision-resolved.
---
Duplicate comments:
In `@src/utils/resolveCollisions.ts`:
- Line 96: The for-loop in resolveCollisions uses "for (let iter = 0; iter <=
maxIterations; iter++)" which makes it run one extra pass (maxIterations + 1);
change the loop condition to "iter < maxIterations" (or adjust the
initialization if you want 1-based counts) so that when maxIterations is 0 no
relaxation pass runs. Update the loop condition referencing iter and
maxIterations inside the resolveCollisions function accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d2b7be4-41c7-48cd-bb4f-ba6fa5abbd8f
📒 Files selected for processing (1)
src/utils/resolveCollisions.ts
Signed-off-by: itvi-1234 <rjsumit71@gmail.com>
|
Hey agnivesh , Also In simple words let's Imagine a classroom with 60 students. Previously, we were comparing every student against all other 59 students to check for space, even if they were sitting in the first row and the other was in the last. This led to thousands of unnecessary calculations (O(n²) complexity), causing the UI to lag significantly on large schemas. Why it’s better: Instead of blindly checking every pair, the algorithm now skips calculations for nodes that are horizontally far apart. It only does the heavy lifting when nodes are geometrically close enough to actually collide. |
|
Hey agnivesh , any direction or views regarding this PR |
|
Hi, reviewing this would be a bit time consuming as I myself have to thoroughly go through it first... since I just copy pasted it previously. I'm calling it a day for now, so, I'll check this by next weekend. |
|
let’s connect next weekend. 🚀 |
2 participants
Description: This PR optimizes the resolveCollisions algorithm by reducing its time complexity from$O(n^2)$ to $O(k \cdot (n/k)^2)$ , where $k$ is the number of columns (depths).
closes issue #240
Changes:
Implemented Depth-Based Partitioning: Instead of comparing every node pair globally, nodes are now grouped into "buckets" based on their depth.Localized Resolution: Collision checks are only performed between nodes within the same bucket/column.Summary by CodeRabbit
Chores
Refactor