refactor(web): move the IconButton call sites to Opal Button - #14163
Open
raunakab wants to merge 7 commits into
Open
refactor(web): move the IconButton call sites to Opal Button#14163raunakab wants to merge 7 commits into
raunakab wants to merge 7 commits into
Conversation
The first eight call sites are a straight prop translation: the boolean flags become the string enums Opal takes. tertiary/internal -> prominence small -> size transient -> interaction toolTipPosition -> tooltipSide Two sites also passed a className, which Opal Button refuses by design. Both were positioning the button rather than restyling it, so the class moves to the parent: `shrink-0` to a wrapper, and `ml-2` becomes the row's own gap and padding. The sites that override Opal's own sizing, border or hover still use IconButton and are left for later.
The four card actions hid themselves with `hidden group-hover/AgentCard:flex`, which Opal Button cannot carry. Hoverable is the sanctioned way to say the same thing, so the card gets a Hoverable.Root and each action becomes a Hoverable.Item. The pin action is the one that is not uniform: a pinned agent shows its pin at rest and only an unpinned one waits for hover, so only that case is wrapped. The button itself is declared once and rendered either way. Hoverable also reveals on focus-visible, so these actions are now reachable by keyboard, which the hover-only classes never allowed.
Two more rows whose actions were hidden with group-hover classes that Opal Button cannot carry. The file row was already a replace-on-hover in all but name: the extension label at rest, the open-file action on hover. It now says so, and the two halves no longer have to agree about which class hides which. The action row keeps its Tailwind group, because the source-count spans still use it. Its toggle has three states, not two, so the source-count case now simply does not render the button rather than layering `hidden!` over the hover classes.
The chat row and the craft session row carried the same class, which encoded three states at once: pinned open while its popover is open, revealed on hover otherwise, and never shown while the row is being renamed. Hoverable.Root takes an `interaction`, so the open-popover case is now the Root's job. Hover is the Item's default. The renaming case is the one Hoverable has no word for, so the menu simply is not rendered then. NameStep was already inside a Hoverable.Item and only needed the button swapped.
The call site moved to Opal Button in 41f2bea, but this import used a relative path and so was missed when the others were cleaned up. Nothing referenced it.
The send button only needed `iconClassName` for its loading spinner, and wrapping the icon component says the same thing — the pattern the file picker already uses. Nothing else about it was custom. The rename button sized itself with `h-6 w-6`, which is exactly what size="sm" gives (--height-line-label is 1.5rem). Its resting opacity moves to a wrapper, since that dims the button rather than restyling it. BaseInputBar keeps IconButton for its stop button, whose 1.5px border has no Opal equivalent.
Contributor
|
Full-stack Preview (frontend + backend)
Sign in with GitHub as an |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 17 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
|
Preview Deployment
|
Contributor
Greptile SummaryThis PR migrates IconButton call sites to Opal Button and replaces custom hover classes with Hoverable primitives.
Confidence Score: 4/5The sidebar hitbox regression should be fixed before merging because hidden menu controls intercept normal row navigation. Opacity-based hiding leaves both migrated sidebar menu triggers clickable above their navigation overlays, so right-edge row clicks open menus instead of selecting sessions. Files Needing Attention: web/src/sections/sidebar/ChatButton.tsx; web/src/app/craft/components/SideBar.tsx Important Files Changed
Prompt To Fix All With AI### Issue 1
web/src/sections/sidebar/ChatButton.tsx:412-419
**Hidden menu intercepts navigation**
When a user clicks the right edge of a row while its menu icon is hidden, the opacity-zero trigger remains hit-testable above the row overlay. It opens the actions popover and stops propagation instead of navigating to the selected chat; the same pattern affects Craft session rows.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "refactor(web): move two more IconButtons..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
🖼️ Visual Regression Report
|
An `appear-on-hover` item only set `opacity: 0`, so at rest it was invisible but still hit-testable. In the sidebar that put a transparent popover trigger above the row's navigation overlay, which is `z-99` to the actions slot's `z-100`: clicking the right edge of a row opened the menu instead of opening the chat. This is why the classes it replaced worked — both `display: none` and `visibility: hidden` take the element out of hit-testing, and opacity does not. Every opacity rule now moves `pointer-events` with it. Local mode is deliberately excluded. It reveals on its own hover, so an item that could not be hovered could never be revealed. Also forwards `style` to the send button's spinner. Opal's iconWrapper sizes icons through `style`, not classes, so dropping it left the loader at its own default size. LineItem sizes with classes instead, which is why the file picker's loader does not need this.
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.
Description
Moves the
IconButtoncall sites onto Opal'sButton. 16 of 21 files are done; the last five are listed below and are deliberately left alone.Opal
ButtonrefusesclassNameby design (InteractiveStatelessProps extends WithoutStyles<...>), so each site had to say what its class was actually for:tertiary/internal→prominence,small→size,transient→interaction,toolTipPosition→tooltipSide.action,danger,onHoverandhrefturned out to be unused across all 29 call sites.Hoverable. Nine of the classes werehidden group-hover/X:flexor the CSS equivalent. Those are nowHoverable.Root+Hoverable.Item, which also reveals onfocus-visible— so these actions are keyboard-reachable for the first time.shrink-0moved to a wrapper;ml-2became the row's owngap-2 pl-2, which also follows the padding-over-margin rule.iconClassName→ a wrapped icon component, the idiomFilePickerPopoveralready used for its spinner.Two behaviours are worth a look rather than a read:
Hoverable'sappear-on-hoveris opacity-based, so a hidden item still occupies layout space.ActionLineItemusedinvisibleand matches exactly.AgentCardusedhidden, so its card actions now reserve their space instead of reflowing the row on hover. I think that is an improvement, but it is a visual change.ActionCardHeader'sh-6 w-6maps tosize="sm"exactly (--height-line-labelis1.5rem), but Opal's internal padding for that size differs slightly from the oldp-2.Not migrated
Five sites override Opal's own appearance, which it has no prop for. Each needs a design decision, so they still use
IconButton:LLMProviderCardhover:bg-transparent— no prominence suppresses the hover backgroundBaseInputBar(stop)border-[1.5px] border-border-02— Opal auto-borders onlyprominence="secondary", at its own widthInputImagew-5! h-5! p-0.5! rounded-04!ChatPanelrounded-full p-2.5! bg-*!— a round custom-coloured buttonAttachmentButton<button className="attachment-item">; already invalid HTML, andButtonpreserves itIconButton.tsxand its stories can be deleted once those five are resolved.Screenshots + Videos
Button changes were all mechanical. Should be no UI/UX changes here.
Additional Options
Summary by cubic
Replaces most
IconButtonusages with@opal/componentsButtonand migrates hover-only reveals to@opal/coreHoverablefor keyboard access. Old behavior used Tailwindhidden/group-hover; new behavior usesHoverableon hover and focus-visible. Also fixes invisible hover items intercepting clicks by toggling pointer-events inHoverableCSS.tertiary/internal→prominence,small→size,transient→interaction,toolTipPosition→tooltipSide). Removed unusedaction,danger,onHover,href; wrapped icons instead oficonClassName; moved positioning classes to parents.hidden/invisible group-hoverwithHoverable. File picker now uses replace-on-hover (extension label ↔ open-file button). Sidebar row menus pin open while their popover is open; they do not render while renaming.ActionLineItemrenders the toggle inline or as a hover item; when showing source counts it omits the toggle.Hoverablenow sets pointer-events: none when items are hidden (local mode excluded) to prevent invisible triggers from catching clicks (notably in sidebar rows).AgentCardactions reserve space instead of reflowing on hover; small buttons use Opal padding.ActionCardHeadermapsh-6 w-6tosize="sm". The send button wraps its spinner icon to size and spin correctly.Not migrated
LLMProviderCard: suppress hover background.BaseInputBar(stop): 1.5px border not expressible in Opal.InputImage: custom size and padding.ChatPanel: round, custom-colored button.AttachmentButton: nested in another<button>; invalid HTML retained.Written for commit 0cf0f15. Summary will update on new commits.