feat: rework button grid interactions - #4434
Open
Julusian wants to merge 39 commits into
Open
Conversation
The grid cell fired its click on pointerdown, so a touch that was meant to scroll the grid opened the editor before the finger had moved. On the single-panel layout that is a full-screen context switch to back out of. `.button-control` also sets `touch-action: none`, which left the grid barely scrollable by touch at all, since buttons cover nearly the whole canvas. Add a grid-specific cell component. A tap now commits on release, and only when the pointer stayed put; the browser is allowed to pan, and the pointercancel it fires when it takes a gesture over is read as "that was not a tap". ButtonPreview is deliberately left alone rather than parameterised. It emulates a physical surface, where a touch is always a press and must never be reinterpreted, and it carries a set of hard-won Android workarounds. The markup and CSS are shared; only the gestures differ. Press mode keeps firing on pointerdown - press and release map to real button-down/up on the control - and holds `touch-action: none` so a scroll cannot steal a press. The gesture handlers reach the cell through a new per-grid-instance context rather than being threaded down as props, which is the seam the rest of the interaction rework hangs off. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
The grid had three mechanisms racing to interpret a click: an imperative interceptor from the action bar under the grid, a global "is shift held" flag, and the default select path. Which one won depended on state spread across three components, and shift - the one modifier every grid UI uses for range-select - was spent on firing buttons for real. Replace all three with one active tool that alone decides what a tap means. Tools are objects rather than a flag, so each owns its own phases: "back" means unwind one step of a half-finished copy, or drop the selection, depending on which tool is asking. Escape is now "one step back, then out" rather than a jump to nothing. Copy/move/swap/delete keep the two-tap flow and its wording exactly - it is still the best way to do this on a touchscreen - but they now stay armed afterwards, so repeated work no longer means re-arming between each one. They also capture their sources rather than reading the selection as they go, which is what lets a transfer survive changing page, while a selection (which belongs to one page) is dropped. Chrome goes down, not up. The bar under the grid, the heading, the paragraph of explanation and the permanent keyboard-hints alert are all gone; the tools move into a palette above the grid, and wipe page / recreate navigation buttons move into a page menu, out of reach but not hidden. What is left is one toolbar plus a context bar that keeps its height whether or not it has anything to say. Press mode gets a banner in the space the heading used to occupy, since a dull tint on the grid was far too easy to miss for something that runs real actions on real hardware, and it is never restored on load. Selection lives in a per-grid store rather than component state, built on useSyncExternalStore. Each cell subscribes to one boolean about itself, so changing the selection re-renders the two cells that changed rather than all of them. Being per-instance rather than app-wide is also what would let two grids sit side by side later. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
Selection was one button, full stop, so rearranging a page meant repeating the same two-tap dance for every button in it. Add the three gestures everyone already knows: shift-click extends a rectangle, ctrl/cmd-click toggles one cell, and dragging across the grid rubber-bands. Shift is only free to mean this because hot pressing moved to its own tool. The arrow keys grow the same way - shift extends from the anchor, ctrl walks the focus without disturbing the selection, space picks out the focused cell - which is also why focus and selection are tracked separately rather than as one thing. Arrow navigation now scrolls the focus into view. It never did, which was survivable when the selection was a single button you had just clicked, and is not once the keyboard is the way you build a selection. The transfer tools already worked in terms of "the sources", so they generalise to regions for free: a copied block keeps its shape, anchored at the tapped cell, and goes out as one atomic request. With more than one button selected the right-hand panel shows the region as the shape it actually is, gaps left as holes, rather than a list of coordinates nobody can picture. It only takes over the panel on a wide screen, where the grid stays visible - on a narrow one it would replace the grid mid-gesture. Marquee is left to the mouse. On touch a drag scrolls the grid, which matters far more than rubber-banding with a finger. Also replaces the page-number fixup that reassigned a variable mid-render (and navigated from a setTimeout) with a resolved value plus an effect, since the keyboard handler now needs to know which page it is looking at. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
Nothing on the grid could be dragged: cells were drop targets for presets and otherwise inert, so rearranging a page meant the pick-then-place tools even when the buttons were right next to each other. Buttons now drag. Which ones depends on the tool: in select only an already-selected button drags, so dragging anywhere else can still rubber-band, and the new arrange tool lets any button drag for when dragging should only ever mean dragging. Press mode allows none of it - a drag must never swallow a press that is about to fire real actions. The cell the drag started from lands under the cursor and the rest of the selection keeps its offset from it, so there is no anchoring rule to learn: what the ghost shows under the pointer is where it goes. Dropping one button onto another trades places, the way rearranging icons does everywhere else; a region cannot do that without scattering what it displaced, so it overwrites and asks first. A region that would hang off the edge is refused outright rather than dropping the buttons that happen to fit. The ghost draws the region with its gaps left as holes. It costs nothing beyond the elements, because image subscriptions are already shared with the grid. Also adds middle-drag to pan and ctrl+wheel to zoom, both of which the grid wanted long before it could be dragged on. Alt-to-copy is deliberately absent. It would need a global modifier listener, which is exactly what hot pressing used to be and exactly why it was moved; copy stays an explicit tool. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
They are two views of the same store, so the thing worth checking is that picking a tool in one shows up in the other - the hint appearing and following the tool to its next step, cancel unwinding one step before leaving, and the selection bar arming the matching tool. Also gives the tool buttons an explicit aria-label. Their text label collapses when the toolbar is narrow, which left them as icons with no accessible name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
…ne row Ctrl-clicking builds up a scattered selection and rubber-banding is faster still, but neither is available to a finger: a touch drag has to stay as scrolling the grid, and there is no ctrl key. So multi-select becomes a mode you enter, where every plain tap adds or removes a button - which works the same way with a mouse, for anyone who would rather not hold a chord. It keeps whatever is already selected when entered, so it can be reached for part-way through picking things out, and shift still extends a range. That made an eighth tool, and running the real thing showed what the tests could not: the palette was already wrapping onto a second row, and the selection bar onto a third. Four bands of chrome above the grid, which is exactly what this rework was meant to remove. Two fixes. The palette's collapse threshold was guessed too low, so the labels "fit" by wrapping - it is now measured from how wide the labelled row actually gets, and the active tool keeps its label even when the rest collapse, so which mode the grid is in is never left to be inferred from an icon. And the selection bar no longer repeats copy/move/swap. Those are the tools in the palette directly above, which already pick up the selection - that is the whole point of the tool model, so a second copy of them was pure chrome. Delete stays, because it is the exception: it has no destination to pick, so the palette's delete tool works tap-by-tap and clearing a whole selection belongs next to the count it acts on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
The previous fix swapped one guessed pixel threshold for another. A fixed number in JS cannot answer this: it is measured at one font size and one zoom level, so a reader with larger text gets labels the row no longer has room for, and it says nothing about widths below the icons-only row either. Hand the decision to CSS, where it belongs. A container query on the toolbar asks about the toolbar's own width - the panel it sits in does not track the viewport, so a media query would be answering a different question - and the threshold is in rem, so it moves with the reader's font size instead of promising room that a larger font has taken away. Where container queries are unsupported the labels simply stay collapsed, which is the safe direction to fail. Below that, the active tool keeps its label so the current mode is still named; below that again even it goes, since it is the one thing tipping a ~440px panel into a second row, and the active tool is already the only coloured one. The group divider is gone. It was a flex item, so it stranded itself at whatever width the row happened to wrap at; a wider gap between the groups says the same thing and cannot dangle. It also means a wrap lands on the group boundary, so a phone gets modes on one row and tools on the next rather than an arbitrary split. Measured across panel widths from 277px to 894px: one row everywhere except a phone, where eight full-size tap targets cannot fit one row and two tidy rows is the right answer. No horizontal overflow at any width. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
The grid's tool palette was a row of ordinary Buttons, which read as a row of buttons rather than as a toolbar. The layered button editor already had the right visual language for this - a bordered strip of flat icon buttons with the active one in the accent colour - but it was private to that component, so matching it by eye would have meant a second copy of the same rules, free to drift. Extract it instead. Toolbar.Root/Button/Separator/Group in Components, with the CSS that used to live in EditButton.css, parameterised by orientation and size: the style editor keeps its vertical 'sm' rail unchanged, and the grid gets the same thing horizontally at a size that can still be hit with a thumb. All that is left in EditButton.css is where the rail sits relative to the canvas. Toolbar.Group exists for the wrapping case: without it a toolbar narrow enough to wrap breaks between arbitrary buttons, which on a phone left a lone delete button on a second row. With it the break falls between the modes and the pick-then-place tools. Tone is a property of the button rather than a colour chosen at the call site, so press and delete stay red for the reason that they are live or irreversible. Verified in a browser: the style editor's rail is unchanged at 34x288, and the grid toolbar is a single row at every panel width measured from 337px up, with the phone the only two-row case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
…utton Copy and move were off by one step. Clicking a button to look at it selects it, and the transfer tools took any selection as their source, so arming Copy went straight to "where do you want it?" - and the tap the user meant as the source became the destination, copying whatever they had last looked at. Only a deliberate selection of several buttons is a statement of intent now; one selected button is just the one you were looking at. Delete gained the mirror of that rule, so a multiple selection can be cleared by touch, where there is no delete key. The rest is what running it showed: The toolbar spans the panel and carries its own status line, so it reads as a toolbar rather than a huddle of buttons, and the row below is no longer an empty band that looks like stray padding. Idle it names the current mode rather than sitting blank; cancel keeps its place, disabled, rather than appearing and reflowing the bar. Press mode's warning moved into that status. As a banner above the toolbar it pushed the whole grid down the page the moment it appeared, which is a strange thing for a warning to do. The toolbar is 42px in every state now. Per-button labels are gone rather than shown only for the active tool, which read as an odd one-off. They also could not earn their place: all eight need 883px, which leaves nothing for a status message on any real panel. The status names the active tool at every width instead, which is what the labels were for. Buttons were 38px around a 12px icon, which is why they looked over-padded - 32px around a 16px icon reads as a toolbar button. The group divider no longer stretches the full height, where it looked like the bar had been padded out. The page menu had none of the app's dropdown styling, because all of it is scoped to two specific positioner classes and I had used neither. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
… heading Dragging with the move tool armed drew a selection box. The marquee was offered by the grid regardless of which tool was active, so a drag that meant "nothing here" still rubber-banded, and the box outlived the gesture. Only the selecting tools offer it now. The same pointerdown also started a marquee behind a button being dragged, so a drop both moved the button and selected the region it crossed; a drag in progress now cancels it. Restore the heading and the paragraph above the grid. Removing them was my call and it was wrong - it left this page not matching the others, and orphaned the help icon into the page-picker row where it had no business being. It goes back beside the heading, which is what it was styled for. The right-click menu now acts on the selection when the click lands inside one, naming the count so it is clear what is about to happen. Clicking outside a selection is still about the button under the cursor and leaves the selection alone. Press and abort stay per-button either way - firing five buttons from a menu entry is not what anyone means by it. The selection preview drew its buttons as a fraction of the panel width, so selecting a column produced four enormous soft tiles and a scrollbar. They cap at 72px now, the size the grid itself draws at and the size the previews are rendered for. The toolbar's status drops to its own row when the bar is too narrow to hold it beside the buttons, rather than shrinking until it needs an ellipsis. Its basis is fixed rather than sized from its content, so which row it lands on depends on the width available and not on how long the current message happens to be. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
…low it Dragging marked only the cell under the cursor, which says nothing about where the rest of a region ends up or whether the drop will be allowed at all - both worth knowing before letting go. The whole landing region now lights up while the drag is in flight, in red when it would be refused for hanging off the grid. The preview and the drop resolve the drag through the same call, so the two cannot disagree about where the buttons were going. planGridDrop describes a refused drop rather than returning null for it, since the preview needs to draw the thing it is refusing. Dropping a preset still marks every cell as a target, because that is news; dragging a button around the grid no longer does, because every cell is a target and saying so buried the part that mattered. After a move the old positions stayed selected, pointing at where the buttons used to be. The selection follows them to where they landed now. That lives in the transfer action rather than in the tool, so it holds for dragging, for the tap-tap flow and for pasting alike - the drag path had never touched the selection at all, which is where this was most obvious. The selection preview was capped at 72px, which the panel had room to beat. It fills the width available up to twice the size the grid draws at, and the cap is on the whole grid rather than each column, so it can never want a scrollbar. The status and the cancel button are kept together, so a bar too narrow to hold them beside the tools moves both to the next row instead of leaving the message on one row and a lone button stranded on the next. Checked from 360px to 2560px: they share a row at every width. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
The group divider was sized as a percentage of the toolbar's height, which is the thing the toolbar is still working out. It settled on 40px against a 32px row, so the first line came out taller than the height the toolbar had reserved for it, and everything after that hung a few pixels below the bar - the status background and the cancel button most visibly. Size it from the button instead. Nothing then depends on a height that depends on it, and the toolbar grows to hold however many lines it needs: on a 267px panel that is three, with the tools on two rows and the message on a third, rather than two rows and an overflow. Cancel is enabled in press mode. Escape already left press mode, so the button that means the same thing had no business being greyed out. Measured from 280px to 600px: no overflow at any width, and every row sits inside the bar. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
Arming a transfer tool with several buttons selected captured them as its sources but left them selected as well - two copies of the same thing. Anything that cleared one left the other behind: deselecting from the right-hand panel emptied the selection while the tool carried on holding the same buttons and asking where to put them. One owner at a time. Picking them up now clears the selection, so the panel and its deselect button go with it and there is nothing left to half-clear. Backing out hands them back rather than dropping them, since misclicking a tool should not cost you the selection you built up to use it. That tab only exists while it has something to show, so it could be the tab you were on and then stop existing - which left the panel blank. It now falls back to Pages when the last of its content goes away. Clearing a multiple selection any other way had the same hole. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
The drop preview outlined the cells a drag would land on, which tells you something is arriving but not what. Lining a large block up means seeing which button lands where, so each destination now draws that button's own image over the cell. A swap describes both ends, since it displaces something as well as placing something - showing only half of that is showing the easy half. Pasting could put buttons outside the grid, where nothing can reach them, with no warning at all. Both paste paths now go through one place that checks first: a paste that would not fit is refused and says so, and one that would replace something asks and names the count. Refusing rather than pasting the part that fits is deliberate - a cut whose destination was dropped would take the source with it and leave nothing behind. The backend checks the bounds too. The UI owns the geometry, but a location off the grid is not something to take on trust from arithmetic done elsewhere. Clicking an empty cell with delete armed asked whether to clear nothing. It is ignored now, and so is picking an empty source to copy or move - there is nothing to carry, and it would quietly wipe whatever it was dropped on. Swap still takes either end, since trading with an empty cell is how a button gets moved into one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
…ing else Ctrl+C and Ctrl+X marked their buttons on the grid and then said nothing about it. The toolbar reported the current tool, cancel sat greyed out, and escape did not touch it - so mixing the keyboard with the toolbar could leave buttons marked with no way to find out why or to clear them. The clipboard is now part of the same in-progress state as everything else. The toolbar says what is held and what to do with it, cancel is offered while it is, and escape unwinds it - after any selection, so each press does one visible thing rather than clearing everything at once. It is reported ahead of the selection count on purpose. A pending cut is an operation waiting to happen where a selection is only a highlight, and it is the half that was invisible, which is how this went unnoticed in the first place. The clipboard is deliberately still not the same thing as an armed tool: it survives a page change, it stays put so a copy can be pasted more than once, and the right-click menu pastes from it. Sharing the state would cost all three. Sharing the way out of it was the actual problem. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
Picking the source for a copy, move or swap meant tapping one button, so acting on a block meant selecting it first with a different tool and then arming the one you wanted. Dragging a box now picks the sources directly, and under delete it picks what to clear. Whether a box means anything is the tool's business rather than a flag on it, because for a transfer it depends on where the tool has got to: a box while it is asking what to take, and none once it is asking where to put them, where one would mean nothing and leave a stray rectangle behind. Press mode still offers none at all. A box over nothing but empty cells is a stray drag rather than a choice, so it is ignored - the gaps inside a region are still part of its shape and travel with it. Delete counts only the cells that hold something, so the confirmation names the number of buttons going rather than the size of the box. The rectangle geometry moves out of the store, so the tools can reach it without importing the store that owns them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
… click Arming Copy/Move/Swap and picking a source told you "Where do you want it?" and nothing else. The buttons are anchored by the top-left of what was picked up, which is impossible to work out from a box dragged bottom-right to top-left: you have to click the new home of a button that isn't the one you started the box from. The click that commits was the first sign of what it actually did. The grid now reports the hovered cell, and a transfer tool holding buttons draws them there through the same ghost machinery a drag uses - real button images, both ends of a swap, and a red outline when the region would hang off the grid. It also means an armed tool is visibly armed while the pointer is over the grid, rather than being mistaken for one you had already left. Placing off the grid is now refused rather than putting the part that fits somewhere and losing the rest, matching what dropping a region off the edge and pasting past the edge already do. The tool keeps hold of the buttons, so the cost is one more tap somewhere with more room. `buildTransferPairs` and `previewPlacements` move to GridGeometry, which is where the tools can reach them without a cycle back through the store. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
… ask before overwriting Two ways to lose a button without being asked. Copying a region with gaps in it stamped the gaps down too, clearing whatever they landed on. The gaps in a region are its shape, not its contents: they decide where the buttons around them land, and then have nothing of their own to place. Copying a hole over a button is not a way to delete that button, so they now leave what they pass over alone. A swap is unchanged - trading with an empty cell is how a button is moved into one, and it destroys nothing either way. The backend applies the same rule, so no caller can ask for the old behaviour. Overwriting was confirmed when dropping a region and when pasting, but not when placing with the toolbar tools - the flow most likely to be carrying several buttons at once. Rather than adding a third copy of the question, every way of moving buttons about now goes through one `transfer` that drops the empty sources, refuses a placement that would fall off the grid, and asks before replacing anything. It reports back only once the transfer has actually been sent, so a tool that was cancelled at the prompt is still holding its buttons, and a cut is only spent on a paste that happened. The count in the prompt is of buttons actually being replaced, so the gaps in a region are not counted, and neither is a cell the region is vacating anyway. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
… the cursor Two things about placing a region. Getting the selection wrong meant starting the tool again. Shift and ctrl build a selection, so they now revise one that has been picked up: ctrl takes a button in or out, shift extends a rectangle from wherever the pick started, and a shift/ctrl box adds another region to what is in hand. Taking the last button away goes back to asking for a source rather than holding nothing while still claiming to be waiting for a destination. While a modifier is held the landing spot is not drawn, since the next click revises rather than places. A plain box still means nothing while a tool is holding buttons, so a stray drag cannot replace the selection built up to be placed. That gate moves from a boolean the grid subscribes to into `allowsMarquee(additive)`, asked as the drag starts - so a box that would mean nothing is never drawn, and the grid stops re-rendering every time a tool changes phase. The region also anchored by its top-left, which put the cursor at a corner of whatever it was carrying and everything else below and right of it. It now centres on the cursor: "put it here" needs no rule, where "its top-left corner goes here" does. Pasting keeps the top-left - its destination is named rather than pointed at, there is no ghost to show which cell the answer was measured from, and centring a 3x3 on the top-left cell of the grid would need a row above the grid and be refused. The two agree exactly for a single button. Also: a cell that cannot be dragged no longer reports itself as `aria-disabled`. dnd-kit marks whatever holds a disabled draggable's ref that way, and in select mode that is every unselected button on the grid - all of them perfectly clickable. The ref is withheld instead of passed disabled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
A transfer tool declines to take a single selected button as its source - the one you last looked at is not a choice, and taking it would silently make your first tap the destination. It left that button selected, though, so the grid showed a highlighted button the tool was not holding: it looks picked up and behaves as though it is not. A modifier click then made no sense. Meaning "and this one too", it had nothing to add to, so it ignored both the modifier and the highlighted button and picked up whatever was clicked - reading as the tool trying to place rather than extend. So: arming clears a selection it does not take, and a modifier click with nothing in hand takes the selection as its starting point before applying the modifier, which is the state Escape hands back. Picking up clears the selection too, so the two are never both non-empty. The landing spot also blanked whenever shift or ctrl went down, on the grounds that the next click would revise rather than place. That is true, but it means no ghost at all for the whole time a selection is being built up with the keyboard - which reads as the tool having lost its grip on the buttons. It stays put now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
…cross a tool switch Restores the landing spot going quiet while shift or ctrl is held - it was right, and the report it looked broken in turned out to be a stale bundle. But "nothing" is a poor answer for shift, which is the case where what the click does is least obvious: a rectangle measured from an anchor that is not drawn anywhere. So shift now previews the buttons it would take, dashed in the same colour that marks what is already in hand - the same fix as the landing ghost, applied to the other half of what a click can mean. Ctrl still shows nothing, since the cell under it is as likely to be leaving the set as joining it. Switching between copy, move and swap also let go of everything, so deciding a copy should have been a move meant picking the buttons out again. What a tool had picked up is now handed to the one replacing it, which decides what that means: another transfer keeps holding them, however few - the choice was deliberate and the switch is not undoing it - delete asks about them the way it asks about a selection, and press mode puts them down, since a highlight there only misleads. Anything else hands them back as a selection rather than dropping them, so no tool switch quietly costs you the set you built up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
…fore the drag ghost Shift previewed the buttons it would take; ctrl showed nothing, on the grounds that the cell under it is as likely to be leaving the held set as joining it. That is a reason to draw two things, not none - so the preview is now a diff against what is held, and each cell it would change says which way. Joining is dashed in the picked-up colour against the solid outline of what is already held; leaving is the same dashed outline, dimmed. Shift gets the same treatment for free: pulling the rectangle back towards its anchor now shows the buttons that would drop out of it, which it did not before. What the click would leave in hand is worked out in one place for both the preview and the click itself, so the two cannot disagree about it. Also: dragging a button showed dnd-kit's own clone of the cell for a couple of frames before the DragOverlay mounted and replaced it with the region ghost - long enough to see the plain button flash under the cursor first. The clone carries the position collision detection reads, so turning the feedback off outright stops drops resolving at all; it is hidden instead. A preset, whose clone *is* its feedback, is untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
…wap whole regions on drop Dropping one button onto another traded places; dropping a region overwrote, on the grounds that the displaced buttons had nowhere sensible to go. They do: where the dragged ones came from, exactly as for a single button. Each cell trades with the one it landed on, so a region drop now swaps too, with nothing lost and nothing to confirm. The exception is a region nudged onto part of itself, where a cell would be both something's source and something else's destination and cannot be both ends of a trade - that still moves, and overwrites the cells beyond the region it lands on. The shift/ctrl preview also only existed for the transfer tools. It now covers select, multi-select and arrange, drawn against the selection instead of the buttons in hand: shift shows the rectangle it would reach, ctrl the one button it would add or drop. Select stays quiet without a modifier, since a plain click replaces the selection outright and lighting up everything about to go says more about what is being left behind than what is being chosen; multi-select speaks up for every hover, because every tap there does something. The outline takes the colour of the set it would join - the selection's, or the picked-up colour of what a transfer holds - so it reads against the solid outline already on the buttons that are in that set. The preview redraws for the cell just clicked rather than clearing, so "one more click would undo this" is visible without moving the pointer first; and what a click would leave selected is worked out in one place for both the preview and the click. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
…ap that empties The ghosts were drawn at 75% over the cell they land on, so what you saw was the two blended into a third thing that was neither. Solid now: what is drawn is what you would get. That also gives a swap somewhere to draw its other half. Trading with an empty cell is how a button is moved into one, and half of that is a cell becoming empty - which was invisible, because a ghost with no image was not drawn at all and the cell went on showing the button that was about to leave it. A ghost is now drawn for any cell something is heading to, and with no image it paints an empty button. Copy and move need nothing here. An empty source carries nothing and is dropped before it reaches the preview, so no destination of theirs ever empties; a move's own sources do empty, but those are the buttons in hand and are already marked as such - blanking them would make the grid look like the move had already happened. Also restores the drag ghost's styles, which an over-broad edit to this file deleted several commits ago: it had been rendering as an unstyled block, so a region dragged around the grid was not laid out as a region at all. Back, minus its own transparency, and centred on the pointer rather than up and to the left of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
…nge opening the editor Escape only worked while the focus was inside the grid. Switching to a tab on the right - which is a normal thing to do mid-transfer, to go and look something up - left it doing nothing until you clicked back near the grid, and being stuck holding a set of buttons is exactly the state it is there to get you out of. Something you press to be sure you are not about to do anything has to work when you press it. It now listens for the whole page, stepping aside for anything with a nearer claim on the key: a text field, and an open dialog or menu, which is what Escape closes while one is up. The tools stay armed through a change of focus rather than being torn down by one - going to fetch a preset and coming back to find the buttons put down would be its own trap, and the toolbar says what is armed the whole time. Also, tapping in the Arrange tool opened the button in the editor, since it inherits the default tool's tap. A tap there is for picking out what to drag, and being taken to the editor by a stray one is a long way to go - on a single-column layout it replaces the grid being rearranged with the button that was not meant to be tapped. It selects and stays put now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
The Arrange tool lets any button be dragged, which included the ones that are not there: an empty cell picked up and followed the cursor, and dropping it did nothing, because there was nothing to put down. The same held for an empty cell that happened to be selected. Nothing to pick up, so nothing to drag. The cell is no longer registered as a draggable at all, so there is no gesture to start rather than one that goes nowhere - and a drag begun on empty space still rubber-bands, as it did. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
The rules that stop a transfer quietly destroying something - drop the gaps in
a region, refuse anything that would land off the grid, ask before replacing -
were decided inline in a 650-line component, so nothing tested them. Every one
of them was reported as a bug before it was written; none of them had a
regression test.
They move into `planGridTransferRequest`, which reports what a transfer would
do and leaves applying, refusing and asking to the caller. `transfer` and the
paste path both ask it now, rather than each working out the off-grid case for
itself.
Also covers the droppable id round-trip, which had none, and found that a
malformed id parsed as page 0 rather than being rejected: `Number('')` is 0 and
`Number.isInteger(0)` is true. Every droppable in the app passes through the
same drag monitor, so it is matched whole now instead of split and coerced.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
The parts of this that decide what a gesture means were mostly covered but not entirely, and the gaps were all the same shape: the states nothing reaches by accident. Moving the focus with nothing focused, clearing an empty clipboard, wrapping round the edge of the grid, a box drawn over nothing, a gesture a tool has no use for, a second finger moving while the first is held, the end of a swap that empties. The context hooks had no tests at all, which mattered more than the number suggested: every cell reads the store through them, and the reason they are one hook per answer is so an unaffected cell skips the re-render. That is now asserted rather than assumed. Three things turned out to be untestable because they were unreachable, and are gone rather than covered: two store getters nothing read, and the argument that a tool holding no buttons might ask what a click would do to them. The toolbar's "no such tool" fallback goes the same way - every id has an entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
…ghost Brings the rest of the grid's own components under test: the selection preview and the actions it shares with the bar above the grid, the page menu's confirmations, and what the drag ghost draws for a region with holes. Includes the mutation continuations that were previously only asserted not to throw - that a press is released again, and that a failure is reported rather than escaping as an unhandled rejection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
Pulls the canvas geometry out of ButtonInfiniteGrid - which cell a point is in, how far to scroll to reveal one, and which cells are worth drawing - so the fiddly parts are testable on their own, and collapses the two copies of the reveal maths into one function. Covers the rest through the component: the rubber-band's threshold and its modifiers, middle-button panning, what is reported under the cursor, and which cells offer a drag. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
The buttons page had grown to hold the tool actions, the keyboard, the drag-drop monitor and the page-number resolution alongside its layout, none of which could be tested without rendering the whole page - editor, router and all. Each is now its own hook or module, tested directly: what a transfer is allowed to do quietly, what a paste anchors to and explains, every key the grid answers to, and what a drag released over it does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
…focus Covers what the panel wires up: the tools it hands a dragged box and a hovered cell to, keeping the focused cell in view, page stepping, and ctrl+wheel zoom. Writing that turned up setSelection notifying before it moved the focus, so the grid never scrolled to a selection it was given - after a transfer, a drop, or Escape handing one back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
…he selection Covers which page the URL resolves to, where the grid goes at each width, and which of the editor and the selection panel answers for what is selected - with the grid, the editor and the other tabs stood in for. Two things fell out of writing it. The store took every setViewPage as a page change, so a re-render of the page around it dropped the selection under the select tool; it now only passes on an actual change. And the page could never resolve to no page at all, so the states written for that are gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
It only ever redrew what the grid beside it was already showing, and carried a second copy of actions the toolbar already offers - so selecting a few buttons swapped out the editor for something that explained less than the grid did. A multi-selection no longer touches the right panel at all: the editor keeps showing whichever button you opened, and what is selected stays in the bar above the grid. The slot is free for something that earns it - editing style across the whole selection is the obvious candidate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
The editor edits one button, and the one it was opened on is not necessarily even in the selection - so leaving it up beside a highlighted block invites the idea that what you type there lands on all of them. The tab goes for the length of a multiple selection and comes back on the same button afterwards, so passing through a selection costs you nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017iVPGapnD6w1ii3Gor6kQg
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThis PR replaces local button-grid interaction state with a store-driven editing flow. It adds multi-selection, transfer tools, drag previews, keyboard handling, page actions, and grid rendering hooks. It also removes the old hot-press store path. ChangesButton grid editing flow
Poem
Merge Risk: 🟡 Moderate · up to This PR reworks button-grid multiselect, keyboard navigation, and transfer/clear interactions, but the current behavior can silently hide rejected operations, scroll the browser unexpectedly, or clear selections during page navigation. These bounded correctness and usability risks should be fixed or explicitly accepted before merging. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy the linked objectives [ Full details: Out of Scope Changes checkExplanation The changes are within scope. The new grid state, interaction tools, drag-and-drop behavior, page actions, context-menu integration, keyboard handling, styling, and focused tests directly support the button grid interaction rework and linked issue objectives. ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
webui/src/Buttons/index.tsx (1)
72-72: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTwo sources for the page count — probably worth picking one.
Line 72 uses
pages.pageCountto resolve the viewed page, and line 176 passespages.data.lengthtouseGridKeyboard. If those two ever disagree, PageUp/PageDown can wrap to a page number thatresolveViewedPagethen corrects, which reads as a skipped page. Reusing thepageCountvariable on line 176 keeps both in step.♻️ Suggested tidy-up
- pageCount: pages.data.length, + pageCount,Also applies to: 176-176
webui/src/Buttons/ButtonGridToolbar.tsx (1)
195-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a compile-time exhaustiveness check for tool definitions
GridToolIdcurrently matches all entries inNAVIGATION_TOOLSandTRANSFER_TOOLS. A future ID can still makefindreturnundefined, causingactiveTool.labelto throw during render. ARecord<GridToolId, ToolDefinition>object orsatisfiescheck would enforce the keys; castingObject.fromEntries(...) as Record<...>would not.webui/src/Buttons/useGridToolActions.ts (1)
62-74: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOptional: consider surfacing transfer failures to the user.
Thanks for pulling every path through one place — this reads really nicely. One small thought:
apply()updates the selection, callsonGridChanged(), and callsonApplied()before the mutation settles, and a rejection only reachesconsole.error. If the server refuses a move, the user sees the selection jump to cells that never received anything, with no message. A toast or aconfirmRefmessage on failure would close that gap. Same idea applies toclearButtonsat Line 147.Happy to leave this for a follow-up if you prefer to keep this PR focused.
webui/src/Buttons/useGridKeyboard.ts (1)
108-123: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSmall suggestion: call
e.preventDefault()forPageUp/PageDown.The arrow keys and space already claim their keys, but these two do not. The grid panel is a scrolling container, so the browser can page-scroll it at the same time as the handler changes the page. Adding
e.preventDefault()once the focus check passes would keep the two from fighting.✨ Suggested tweak
case 'PageUp': { const focus = store.focus if (!focus) return + e.preventDefault() const newPageNumber = focus.pageNumber >= pageCount ? 1 : focus.pageNumber + 1 setPageNumber(newPageNumber) store.moveFocusToPage(newPageNumber) return } case 'PageDown': { const focus = store.focus if (!focus) return + e.preventDefault() const newPageNumber = focus.pageNumber <= 1 ? pageCount : focus.pageNumber - 1 setPageNumber(newPageNumber) store.moveFocusToPage(newPageNumber) return }webui/src/Buttons/__tests__/ButtonGridStore.test.ts (1)
19-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThanks for the thorough coverage here — one small consolidation idea.
makeActionsduplicatesmakeGridActionsinwebui/src/Buttons/__tests__/gridViewTestHelpers.tsx(lines 19-31), down to the same defaults and comment. IfGridToolActionsgains a member later, both stubs need the same edit. Reusing the shared helper would keep the stubbed contract in one place.♻️ Optional: reuse the shared helper
-import type { GridToolActions } from '../GridTools/index.js' +import type { GridToolActions } from '../GridTools/index.js' +import { makeGridActions } from './gridViewTestHelpers.js' @@ -function makeActions(): GridToolActions { - return { - openEditor: vi.fn(), - press: vi.fn(), - // The real one asks before replacing anything, and only reports back once it has happened - transfer: vi.fn((_operation, _pairs, onApplied: () => void) => onApplied()), - clearButtons: vi.fn(), - // Tests act on a grid where every cell holds a button unless they say otherwise - isOccupied: vi.fn(() => true), - pasteAt: vi.fn(), - fitsOnGrid: vi.fn(() => true), - } -} +const makeActions = (): GridToolActions => makeGridActions()Note the helper file is
.tsx, so importing it into a.tstest pulls in the React dependency. SplittingmakeGridActionsinto a plain.tsmodule would avoid that if you would rather keep this test free of JSX imports.webui/src/Buttons/GridPageNavigation.ts (1)
11-17: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a safe session-storage getter
SafeStorage.tsexports only setters. Add a guarded getter and use it ingetLastViewedPage(). Direct access towindow.sessionStoragecan throw when storage is blocked and interrupt page resolution.webui/src/Buttons/GridButtonIcons.tsx (2)
57-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider dropping
imagefromButtonGridIcon's props.
ButtonGridIconresolves the image itself on line 60 and overwrites whatever the caller passed on line 66. The prop is still required, so callers must supply a value that is always discarded — the test on line 268 ofGridButtonIcons.test.tsxpassesimage={null}purely for that reason.Also, both
memowrappers use the inner nameButtonGridIcon, which makes the two hard to tell apart in React DevTools.♻️ Suggested tidy-up
-type ButtonGridIconProps = ButtonGridIconBaseProps +type ButtonGridIconProps = Omit<ButtonGridIconBaseProps, 'image'> -export const ButtonGridIcon = memo(function ButtonGridIcon({ ...props }: ButtonGridIconProps) { +export const ButtonGridIcon = memo(function ButtonGridIconWithImage({ ...props }: ButtonGridIconProps) { const { image, isUsed } = useButtonImageForLocation({ - pageNumber: Number(props.pageNumber), + pageNumber: props.pageNumber, column: props.column, row: props.row, })The
memo(function ButtonGridIconon line 32 could becomeButtonGridIconBasetoo.
21-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the newly added optional parameters with the surrounding API convention.
ButtonGridIconleaves callback props optional while its sibling interface models absence explicitly, and the test helpers defaultoverridesinstead of requiring an explicit none case. Making these parameters required and passingundefinedornullat call sites would keep the contracts consistent.Source: Coding guidelines
webui/src/Components/ButtonPreview.css (1)
78-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo comments describe the same rule here.
The first block (lines 78-80) talks about blurring the dragged preset, but it now sits above
.grid-drag-source; the preset rule moved down to line 87 with its own comment on 81-82. Removing the stale one would keep the file readable.webui/src/Buttons/__tests__/GridButtonDroppableId.test.ts (1)
36-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOne more case worth pinning: negative rows and columns.
The parser's doc comment says rows and columns may be negative because a grid's bounds need not start at zero, and the regex allows
-?. Nothing here locks that in, so a future tightening of the pattern would pass the suite and silently break offset grids.🧪 Suggested extra test
it('refuses an id whose parts are not whole numbers', () => { expect(parseGridButtonDroppableId('gridbtn:1:x:3')).toBeNull() expect(parseGridButtonDroppableId('gridbtn:1:2:3.5')).toBeNull() expect(parseGridButtonDroppableId('gridbtn::2:3')).toBeNull() }) + + it('accepts cells left of and above the origin, since a grid need not start at 0/0', () => { + expect(parseGridButtonDroppableId(makeGridButtonDroppableId(1, -3, -2))).toEqual({ + pageNumber: 1, + column: -3, + row: -2, + }) + })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c7be3dc-69bc-45ce-abb3-e1a1712e6dc0
📒 Files selected for processing (61)
webui/src/App.tsxwebui/src/Buttons/ActionRecorder/ButtonPicker.tsxwebui/src/Buttons/ButtonGridActions.tsxwebui/src/Buttons/ButtonGridPageMenu.tsxwebui/src/Buttons/ButtonGridPanel.csswebui/src/Buttons/ButtonGridPanel.tsxwebui/src/Buttons/ButtonGridStore.tswebui/src/Buttons/ButtonGridToolbar.tsxwebui/src/Buttons/ButtonGridViewContext.tsxwebui/src/Buttons/ButtonInfiniteGrid.tsxwebui/src/Buttons/GridButtonDragItem.tswebui/src/Buttons/GridButtonDragOverlay.tsxwebui/src/Buttons/GridButtonDroppableId.tswebui/src/Buttons/GridButtonIcons.tsxwebui/src/Buttons/GridButtonPreview.tsxwebui/src/Buttons/GridCanvasGeometry.tswebui/src/Buttons/GridDragDrop.tswebui/src/Buttons/GridGeometry.tswebui/src/Buttons/GridPageNavigation.tswebui/src/Buttons/GridTools/ArrangeTool.tswebui/src/Buttons/GridTools/DeleteTool.tswebui/src/Buttons/GridTools/MultiSelectTool.tswebui/src/Buttons/GridTools/PressTool.tswebui/src/Buttons/GridTools/SelectTool.tswebui/src/Buttons/GridTools/TransferTool.tswebui/src/Buttons/GridTools/index.tswebui/src/Buttons/GridTools/types.tswebui/src/Buttons/GridTransferRequest.tswebui/src/Buttons/__tests__/ButtonGridPageMenu.test.tsxwebui/src/Buttons/__tests__/ButtonGridPanel.test.tsxwebui/src/Buttons/__tests__/ButtonGridStore.test.tswebui/src/Buttons/__tests__/ButtonGridToolbar.test.tsxwebui/src/Buttons/__tests__/ButtonGridViewContext.test.tsxwebui/src/Buttons/__tests__/ButtonInfiniteGrid.test.tsxwebui/src/Buttons/__tests__/ButtonsPage.test.tsxwebui/src/Buttons/__tests__/GridButtonDragOverlay.test.tsxwebui/src/Buttons/__tests__/GridButtonDroppableId.test.tswebui/src/Buttons/__tests__/GridButtonIcons.test.tsxwebui/src/Buttons/__tests__/GridButtonPreview.test.tsxwebui/src/Buttons/__tests__/GridCanvasGeometry.test.tswebui/src/Buttons/__tests__/GridDragDrop.test.tswebui/src/Buttons/__tests__/GridPageNavigation.test.tswebui/src/Buttons/__tests__/GridTransferRequest.test.tswebui/src/Buttons/__tests__/gridViewTestHelpers.tsxwebui/src/Buttons/__tests__/useButtonContextMenu.test.tsxwebui/src/Buttons/__tests__/useGridDropMonitor.test.tsxwebui/src/Buttons/__tests__/useGridKeyboard.test.tsxwebui/src/Buttons/__tests__/useGridToolActions.test.tsxwebui/src/Buttons/index.tsxwebui/src/Buttons/useButtonContextMenu.tsxwebui/src/Buttons/useGridDropMonitor.tswebui/src/Buttons/useGridKeyboard.tswebui/src/Buttons/useGridToolActions.tswebui/src/Components/ButtonPreview.csswebui/src/Components/ButtonPreview.tsxwebui/src/Components/ContextMenu.csswebui/src/ContextData.tsxwebui/src/ImportExport/Import/Page.tsxwebui/src/Stores/RootAppStore.tsxwebui/src/Stores/ViewControlStore.tsxwebui/src/tailwind.css
💤 Files with no reviewable changes (4)
- webui/src/Stores/ViewControlStore.tsx
- webui/src/ContextData.tsx
- webui/src/Buttons/ButtonGridActions.tsx
- webui/src/Stores/RootAppStore.tsx
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
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
2 participants
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.
Multiselect, drag and drop etc
Closes #2834, closes #1124
Summary by CodeRabbit
New Features
Bug Fixes
Tests