Fix snap offset for reroutes and subgraph IO#10229
Open
AustinMroz wants to merge 3 commits intomainfrom
Open
Conversation
🎨 Storybook: ✅ Built — View Storybook |
🎭 Playwright: ✅ 629 passed, 0 failed · 5 flaky📊 Browser Reports
|
Contributor
📝 WalkthroughWalkthroughThree files updated: LGraphCanvas.drawSnapGuide now accepts an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
📦 Bundle: 5 MB gzip 🔴 +93 BDetailsSummary
Category Glance App Entry Points — 22.7 kB (baseline 22.7 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.1 MB (baseline 1.1 MB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 75.9 kB (baseline 75.9 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed / 2 unchanged Panels & Settings — 461 kB (baseline 461 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed / 12 unchanged User & Accounts — 16.9 kB (baseline 16.9 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed / 2 unchanged Editors & Dialogs — 82 kB (baseline 82 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 59.3 kB (baseline 59.3 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed / 8 unchanged Data & Services — 2.91 MB (baseline 2.91 MB) • 🔴 +175 BStores, services, APIs, and repositories
Status: 14 added / 14 removed / 3 unchanged Utilities & Hooks — 322 kB (baseline 322 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 14 added / 14 removed / 8 unchanged Vendor & Third-Party — 9.78 MB (baseline 9.78 MB) • ⚪ 0 BExternal libraries and shared vendor chunks Status: 16 unchanged Other — 8.24 MB (baseline 8.24 MB) • ⚪ 0 BBundles that do not match a named category
Status: 53 added / 53 removed / 79 unchanged ⚡ Performance Report
All metrics
Historical variance (last 10 runs)
Trend (last 10 commits on main)
Raw data |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/litegraph/src/Reroute.ts (1)
428-437: Snapping logic is correct.The offset-based snapping technique properly aligns reroutes with node slot positions. Consider adding a brief comment explaining the 0.7 multiplier's purpose for future maintainers—the PR description notes this as a "magic constant" that may need revisiting.
,
💡 Optional: Add clarifying comment
snapToGrid(snapTo: number): boolean { if (!snapTo) return false + // Offset to align reroute center with slot vertical position const offsetY = LiteGraph.NODE_SLOT_HEIGHT * 0.7 const { pos } = this pos[0] = snapTo * Math.round(pos[0] / snapTo) pos[1] = snapTo * Math.round((pos[1] - offsetY) / snapTo) + offsetY return true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/Reroute.ts` around lines 428 - 437, The snapping math in snapToGrid uses a "magic constant" multiplier LiteGraph.NODE_SLOT_HEIGHT * 0.7 without explanation; add a short clarifying comment above the computation in the snapToGrid method describing why 0.7 is used (to offset the reroute to align with node slot centers/visual anchor) and note that it may need tuning if NODE_SLOT_HEIGHT changes; reference the snapToGrid function and the offsetY = LiteGraph.NODE_SLOT_HEIGHT * 0.7 calculation so reviewers can find and update the comment later.src/lib/litegraph/src/LGraphCanvas.ts (1)
5805-5808: Replace the unnamed slot-offset multiplier with a named constantLine 5805 introduces a hardcoded
0.7multiplier. Please extract it into a named constant so this snap behavior is self-documenting and easier to keep consistent with other slot-snap paths.♻️ Proposed refactor
+const SLOT_SNAP_OFFSET_MULTIPLIER = 0.7 ... - const offsetY = - pos[1] - - snapGuide[1] - - (offsetToSlot ? LiteGraph.NODE_SLOT_HEIGHT * 0.7 : 0) + const slotOffsetY = offsetToSlot + ? LiteGraph.NODE_SLOT_HEIGHT * SLOT_SNAP_OFFSET_MULTIPLIER + : 0 + const offsetY = pos[1] - snapGuide[1] - slotOffsetYAs per coding guidelines: "Write code that is expressive and self-documenting; avoid comments unless absolutely necessary."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/litegraph/src/LGraphCanvas.ts` around lines 5805 - 5808, The calculation for offsetY uses a magic number 0.7; replace it with a named constant (e.g., NODE_SLOT_OFFSET_RATIO or SLOT_OFFSET_MULTIPLIER) to make the snap behavior self-documenting. Introduce the constant near related graph/layout constants or at the top of LGraphCanvas, then update the expression in the offsetY calculation (which uses pos, snapGuide, offsetToSlot, and LiteGraph.NODE_SLOT_HEIGHT) to multiply by that constant instead of 0.7 so all slot-snap logic refers to a single, descriptive symbol.
🤖 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/lib/litegraph/src/LGraphCanvas.ts`:
- Around line 5805-5808: The calculation for offsetY uses a magic number 0.7;
replace it with a named constant (e.g., NODE_SLOT_OFFSET_RATIO or
SLOT_OFFSET_MULTIPLIER) to make the snap behavior self-documenting. Introduce
the constant near related graph/layout constants or at the top of LGraphCanvas,
then update the expression in the offsetY calculation (which uses pos,
snapGuide, offsetToSlot, and LiteGraph.NODE_SLOT_HEIGHT) to multiply by that
constant instead of 0.7 so all slot-snap logic refers to a single, descriptive
symbol.
In `@src/lib/litegraph/src/Reroute.ts`:
- Around line 428-437: The snapping math in snapToGrid uses a "magic constant"
multiplier LiteGraph.NODE_SLOT_HEIGHT * 0.7 without explanation; add a short
clarifying comment above the computation in the snapToGrid method describing why
0.7 is used (to offset the reroute to align with node slot centers/visual
anchor) and note that it may need tuning if NODE_SLOT_HEIGHT changes; reference
the snapToGrid function and the offsetY = LiteGraph.NODE_SLOT_HEIGHT * 0.7
calculation so reviewers can find and update the comment later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e1d4223b-1236-469e-b891-c63832205f1e
📒 Files selected for processing (3)
src/lib/litegraph/src/LGraphCanvas.tssrc/lib/litegraph/src/Reroute.tssrc/lib/litegraph/src/subgraph/SubgraphIONodeBase.ts
Should be safer for tests and is technically the smaller delta
dosubot
bot
added
the
size:S
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
1 participant
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.
When snapping to grid, reroutes and subgraph IO would snap at an awful y offset.
Regretfully, the PR leaves more magic constants than I would like. There's a hardcoded
0.7multiplier onNODE_SLOT_HEIGHTthat isn't defined as a constant, and it seems circular imports prevent constants being used to declare the subgraphIOroundedRadiusSee #8838 (Sorry for the delay. This change wound up being real simple)
┆Issue is synchronized with this Notion page by Unito