Refactor tool pill styling and add cicada explorer support#46
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user interface and interaction of tool call pills within chat rows. It focuses on modernizing their visual presentation, improving the expand/collapse functionality for better readability, and adding robust event handling to create a more intuitive and accessible user experience. The changes are supported by new unit tests to validate the updated UI interactions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
getToolBubbleStyleandBottomChevronButtonrely on hard-coded colors (#FAFAFA,#E0E0E0) instead of design tokens, which will likely break dark mode/theming; consider switching these to existing theme variables or CSS classes used elsewhere for borders and backgrounds. - Both the header and expanded content sections in
ToolCallPillnow usehandleToggleMouseUp, which means clicking anywhere in the expanded result/args area will collapse the pill; if the intent is to only toggle from the header or chevron, limit the toggle handler to that region so users can interact with content without accidentally collapsing it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `getToolBubbleStyle` and `BottomChevronButton` rely on hard-coded colors (`#FAFAFA`, `#E0E0E0`) instead of design tokens, which will likely break dark mode/theming; consider switching these to existing theme variables or CSS classes used elsewhere for borders and backgrounds.
- Both the header and expanded content sections in `ToolCallPill` now use `handleToggleMouseUp`, which means clicking anywhere in the expanded result/args area will collapse the pill; if the intent is to only toggle from the header or chevron, limit the toggle handler to that region so users can interact with content without accidentally collapsing it.
## Individual Comments
### Comment 1
<location path="components/ToolCallPill.tsx" line_range="148-151" />
<code_context>
+ );
+}
+
+function hasSelectedText() {
+ if (typeof window === "undefined" || typeof window.getSelection !== "function") return false;
+ const selection = window.getSelection();
+ return !!selection && !selection.isCollapsed && selection.toString().trim().length > 0;
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Using global window selection to gate toggling can cause unexpected behavior when text is selected elsewhere on the page.
`hasSelectedText` uses `window.getSelection()` globally, so any non-collapsed selection anywhere on the page blocks pill toggling. This means selecting text elsewhere in the UI will prevent expand/collapse until the selection is cleared.
Consider scoping the check to the pill by passing the event/currentTarget into the helper and inspecting selection ranges (e.g., checking whether `range.commonAncestorContainer` or its parent is within the pill node). This keeps the behavior local to text selected inside the pill only.
</issue_to_address>
### Comment 2
<location path="components/ToolCallPill.tsx" line_range="207-216" />
<code_context>
<div
ref={scrollContainerRef}
onScroll={handleScroll}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Spawn pill `aria-expanded` may not accurately reflect the internal `open` state when the pill is pinned.
Because `aria-expanded` is tied to `open && !isPinned`, a pinned pill can still toggle its internal `open` state without any visual change or ARIA update. This desynchronizes user interaction, visual state, and accessibility state. Consider either preventing toggling while pinned (removing the `onKeyDown`/`onMouseUp`/`onTouchEnd` handlers) or updating both the rendered content and `aria-expanded` so they remain consistent when pinned.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the ToolCallPill and SpawnPill components to enhance their expand/collapse functionality and styling. Key changes include updating bubble styling logic, introducing new event handlers for keyboard, mouse, and touch interactions, and adding a BottomChevronButton for collapsing. Several hardcoded styling constants and related CSS classes were removed. A new test file was added for ToolCallPill to cover the new collapse behavior. The review comments suggest replacing hardcoded border and background colors with CSS variables for improved theming and maintainability.
| borderTop: expanded ? "1px solid #E0E0E0" : "none", | ||
| borderBottom: expanded ? "1px solid #E0E0E0" : "none", |
There was a problem hiding this comment.
The border color #E0E0E0 is hardcoded. To improve theming and maintainability, it's better to use a CSS variable from your design system. This will ensure the component adapts correctly to different themes, such as dark mode. Using a variable like hsl(var(--border)) would be consistent with Tailwind CSS conventions.
| borderTop: expanded ? "1px solid #E0E0E0" : "none", | |
| borderBottom: expanded ? "1px solid #E0E0E0" : "none", | |
| borderTop: expanded ? "1px solid hsl(var(--border))" : "none", | |
| borderBottom: expanded ? "1px solid hsl(var(--border))" : "none", |
| aria-label={label} | ||
| onClick={onToggle} | ||
| className="absolute left-1/2 z-10 flex h-5 w-16 -translate-x-1/2 cursor-pointer items-center justify-center text-foreground/45" | ||
| style={{ background: "#FAFAFA", bottom: "-0.625rem" }} |
There was a problem hiding this comment.
The background color #FAFAFA is hardcoded. This will not adapt to different themes (e.g., dark mode). Please use a CSS variable from your design system, for example hsl(var(--background)) or a more specific one for card backgrounds.
| style={{ background: "#FAFAFA", bottom: "-0.625rem" }} | |
| style={{ background: "hsl(var(--background))", bottom: "-0.625rem" }} |
|
Merged manually |
1 participant
Summary
Testing
pnpm test tests/ToolCallPill.test.tsxpnpm test tests/MessageRow.test.tsx