Label a tool's input port even when it has only one - #2012
Conversation
Multi-input tools got a labelled row per port in #1983, but a single-input tool kept a bare dot. A bare dot says a connection goes here without saying what belongs on it, so a user facing Buffer had no more idea what to wire in than they had facing Raster Streams To Vector before its ports were named. Every input port on a tool node is now labelled. The synthetic `input` and `output` nodes stay bare: their single port carries the node's own kind as its name, so labelling it would print "Output" under a card already headed OUTPUT. Output ports are still labelled only when a tool has several, since one result port needs no telling apart. Two sizing corrections that go with it: - CARD_HEADER_HEIGHT was 36, but the rendered header (a 10px uppercase line over a 12px title inside the card's 8px padding) occupies 40. At 36 the first port row overlapped the tool name; that was invisible on a multi-input card because its rows start lower, and only showed up once one-row cards existed. - A labelled card is now always a little taller than a bare one. At the compact 64px a single row's text sat hard against the card's bottom edge. Verified in a browser: Buffer renders as a 70px card reading "Input layer", Clip and Raster Streams To Vector as 84px cards reading their two inputs, and the Input/Output nodes stay bare at 64px. Each label is centred on its dot to the pixel, clears the title, and sits inside the card. Wiring is unaffected: edges still terminate within 1px of their own port row.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe model builder now distinguishes tool and synthetic nodes during card layout. Tool input ports receive labels, while synthetic ports remain unlabeled. Canvas extents, edge anchors, card sizing, and port rendering use the updated geometry. ChangesModel builder layout
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change labels tool input ports and adjusts card sizing so labels remain readable without affecting wiring; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
🔍 Cloudflare PR preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
🔍 GitHub Pages PR preview
Note GitHub Pages built this preview successfully, but its serving edge returned HTTP 403 when checked. The links may still be propagating. |
- Keep a card's input and output dots on the same vertical band. Labelling every tool input moved the input side to row-based positions while an unlabelled single output stayed centred on the whole card, so a 1-in/1-out tool — the most common shape there is — had its two dots 14px apart and drew a visibly bent edge through a straight two-node pipeline. Both sides now share one port band: a labelled side fills it row by row, an unlabelled side spreads its dots down the same band instead of down the whole card. This goes slightly further than the suggestion of pinning the unlabelled side to the labelled side's first row. Centring on the band degenerates to exactly that for one row, and for a multi-input tool it puts the lone output midway between the inputs rather than level with the topmost one, which is how the edges want to meet it. Measured in a browser, dot offsets from the card top: before Buffer in=50 out=36 | Clip in=50,68 out=43 after Buffer in=50 out=50 | Clip in=50,68 out=59 The Input/Output data nodes are untouched at out=33. Edges still terminate within 1px of their own port row, and the labels still clear the title and sit inside the card.
Code reviewBugs: None found. Traced Security: None applicable — pure client-side layout/rendering logic, no user input parsing or injection surface. Performance: None — the added Quality: Left one minor inline nit — a stray, now-fully-redundant one-line doc comment (pre-existing from #1983) sits above CLAUDE.md: No violations — the file isn't one of the mirrored-constant modules requiring special sync checks, and no new user-facing strings were added without No automated test covers the new |
- Delete the orphaned "Where a port's connector dot sits" line above CARD_HEADER_HEIGHT. It documented portPosition, but #1983 inserted the card geometry constants between the two and left the comment stranded on the wrong declaration. portPosition now carries its own doc block, so the stray line is redundant as well as misplaced.
| const height = | ||
| labelIn || labelOut | ||
| ? Math.max(NODE_HEIGHT, CARD_HEADER_HEIGHT + rows * PORT_ROW_HEIGHT + 6) | ||
| ? Math.max(NODE_HEIGHT + 6, CARD_HEADER_HEIGHT + rows * PORT_ROW_HEIGHT + 8) | ||
| : NODE_HEIGHT; |
There was a problem hiding this comment.
Minor, low confidence: with labelIn now true for any single-input tool (previously only multi-input tools grew past NODE_HEIGHT), the great majority of tool cards will now render at 70px instead of 64px. findFreePosition in apps/geolibre-desktop/src/lib/model-graph-edit.ts still assumes every node is NODE_HEIGHT (64) tall when spacing newly-placed nodes (NODE_HEIGHT + NODE_GAP = 80px steps), so the intended 16px gap between a labelled single-input card and the node placed below it shrinks to ~10px. That's not an actual overlap for the single-input case (70 < 80), but it does erode the margin that comment block is relying on, and for a tool with 3+ inputs (height 102px vs the 80px step) it can still visibly overlap the next placed card — a pre-existing issue this change makes noticeably more common by widening which nodes it applies to. Worth a follow-up to make findFreePosition/auto-layout height-aware, even if out of scope for this PR.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
Follow-up to #1983.
Multi-input tools got a labelled row per port in that PR, but a single-input tool kept a bare dot. A bare dot says a connection goes here without saying what belongs on it, so a user facing Buffer had no more idea what to wire in than they had facing Raster Streams To Vector before its ports were named.
What changed
Every input port on a tool node is now labelled.
The synthetic
inputandoutputnodes stay bare: their single port carries the node's own kind as its name, so labelling it would print "Output" under a card already headed OUTPUT. Output ports are still labelled only when a tool has several, since one result port needs no telling apart.Two sizing corrections came with it:
CARD_HEADER_HEIGHTwas 36, but the rendered header (a 10px uppercase line over a 12px title inside the card's 8px padding) occupies 40. At 36 the first port row overlapped the tool name. That was invisible on a multi-input card because its rows start lower, and only surfaced once one-row cards existed.Verification
Driven in a real browser against the production build:
Input layerInput layer,Overlay (clip) layerd8_pntr,streams_rasterEach label is centred on its dot to the pixel, clears the title box, and sits inside the card. Wiring is unaffected: dragging onto the second row still connects
streams_rasterand keyboard-activating the first still connectsd8_pntr, both edges terminating within 1px of their own port row.6512 frontend tests pass; pre-commit clean.
Summary by CodeRabbit