-
Notifications
You must be signed in to change notification settings - Fork 4
feat: use bug icon for run link #28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughAdds a new React component IconBug at src/assets/SvgIcons/IconBug.tsx that renders a FontAwesome bug icon. Several existing SVG icon components (IconCopy, IconThumbsUp, IconThumbsDown) were refactored to use FontAwesomeIcon with a default size of 12. IncomingMsg/FeedbackButtons were updated: IconBug imported; FeedbackButtons props now include showRunLink: boolean and data.web_url?: string, and when showRunLink is true and web_url exists a "Run Details" external link with IconBug is rendered. package.json and .npmrc were updated to add FontAwesome dependencies and registry config. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
| const FeedbackButtons = ({ | ||
| data, | ||
| body, | ||
| showRunLink, | ||
| }: { | ||
| data: { | ||
| buttons: ReplyButton[]; | ||
| bot_message_id: string; | ||
| web_url?: string; | ||
| }; | ||
| body: string; | ||
| showRunLink: boolean; | ||
| }) => { | ||
| const { buttons, bot_message_id } = data; | ||
| const locationModalRef = useRef<LocationModalRef | null>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correctness: The diff adds web_url to the data type and showRunLink parameter, but the caller at line 296 passes props?.data which has type any. This creates a type safety gap where web_url might be undefined at runtime despite being used conditionally at line 163. The any type on props.data masks potential runtime errors when showRunLink is true but web_url is missing.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
File: src/widgets/copilot/components/Messages/IncomingMsg.tsx
Lines: 53-67 and caller at line 296
Problem: The FeedbackButtons component now expects `data.web_url?: string` and `showRunLink: boolean`, but the caller (IncomingMsg component at line 296) passes `props.data` typed as `any`. This creates a type safety issue where `web_url` could be undefined at runtime when `showRunLink` is true.
Fix Instructions:
1. Define a proper interface for the IncomingMsg props.data that includes the web_url field
2. Replace `data: any` in the IncomingMsg props type (line 257) with a properly typed interface
3. Ensure the interface includes: `web_url?: string` along with other required fields like `buttons`, `bot_message_id`, `output_text`, etc.
4. This will provide compile-time safety that web_url is properly handled when showRunLink is enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/assets/SvgIcons/IconBug.tsx`:
- Around line 3-16: The IconBug component is passing the size prop into the
<svg> via {...props}, causing an invalid SVG attribute warning; fix by
destructuring size out of props (e.g., const { size = 14, ...rest } = props) and
then use height={size} width={size} while spreading {...rest} onto the <svg>
(leave SvgIcon wrapper unchanged), ensuring size is not forwarded to the DOM.
🧹 Nitpick comments (1)
src/widgets/copilot/components/Messages/IncomingMsg.tsx (1)
161-167: Add tooltip for consistency with other action buttons.The copy button above uses
GooeyTooltipto provide context ("Copy Message"). The bug icon lacks a tooltip, leaving users unclear about its purpose. Consider wrapping it similarly for consistency and accessibility.♻️ Proposed fix
{showRunLink && data?.web_url && ( - <a href={data?.web_url} target="_blank" rel="noopener noreferrer"> - <IconButton className="text-muted"> - <IconBug size={12} /> - </IconButton> - </a> + <GooeyTooltip text="Run Details"> + <a href={data?.web_url} target="_blank" rel="noopener noreferrer"> + <IconButton className="text-muted"> + <IconBug size={12} /> + </IconButton> + </a> + </GooeyTooltip> )}
1b2d488 to
544075b
Compare
|
Re-review triggered for new commit 544075b. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/widgets/copilot/components/Messages/IncomingMsg.tsx`:
- Around line 161-167: The IconButton is being nested inside an anchor which
produces invalid HTML and accessibility issues; update the render for the
run-link so the interactive element is a single element (either render
IconButton as an anchor if it supports an as prop or replace it with a plain
anchor-styled control) and add a GooeyTooltip to match the Copy button.
Specifically, change the block that checks showRunLink and data?.web_url to
render a single interactive element (use IconButton with as="a" or an <a> with
role/button semantics) and wrap that element with GooeyTooltip, keeping the
IconBug icon inside and preserving target="_blank" rel="noopener noreferrer".
| {showRunLink && data?.web_url && ( | ||
| <a href={data?.web_url} target="_blank" rel="noopener noreferrer"> | ||
| <IconButton className="text-muted"> | ||
| <IconBug size={12} /> | ||
| </IconButton> | ||
| </a> | ||
| )} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid nesting interactive elements and add tooltip for consistency.
Wrapping <IconButton> (likely renders a <button>) inside <a> creates invalid HTML and accessibility issues. Also, the Copy button above has a GooeyTooltip wrapper for consistency.
🛠️ Proposed fix
{showRunLink && data?.web_url && (
- <a href={data?.web_url} target="_blank" rel="noopener noreferrer">
+ <GooeyTooltip text="View Run Details">
+ <a href={data.web_url} target="_blank" rel="noopener noreferrer" className="text-muted" style={{ display: 'inline-flex' }}>
+ <IconBug size={12} />
+ </a>
+ </GooeyTooltip>
- <IconButton className="text-muted">
- <IconBug size={12} />
- </IconButton>
- </a>
)}Alternatively, if IconButton supports an as prop or similar pattern, render it as an anchor directly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {showRunLink && data?.web_url && ( | |
| <a href={data?.web_url} target="_blank" rel="noopener noreferrer"> | |
| <IconButton className="text-muted"> | |
| <IconBug size={12} /> | |
| </IconButton> | |
| </a> | |
| )} | |
| {showRunLink && data?.web_url && ( | |
| <GooeyTooltip text="View Run Details"> | |
| <a href={data.web_url} target="_blank" rel="noopener noreferrer" className="text-muted" style={{ display: 'inline-flex' }}> | |
| <IconBug size={12} /> | |
| </a> | |
| </GooeyTooltip> | |
| )} |
🤖 Prompt for AI Agents
In `@src/widgets/copilot/components/Messages/IncomingMsg.tsx` around lines 161 -
167, The IconButton is being nested inside an anchor which produces invalid HTML
and accessibility issues; update the render for the run-link so the interactive
element is a single element (either render IconButton as an anchor if it
supports an as prop or replace it with a plain anchor-styled control) and add a
GooeyTooltip to match the Copy button. Specifically, change the block that
checks showRunLink and data?.web_url to render a single interactive element (use
IconButton with as="a" or an <a> with role/button semantics) and wrap that
element with GooeyTooltip, keeping the IconBug icon inside and preserving
target="_blank" rel="noopener noreferrer".
544075b to
9b8482a
Compare
|
Re-review triggered for new commit 9b8482a. |
|
✅ Review completed for commit f8597c7. |
|
✅ Review completed for commit 9bb0f2b. |
Legal Boilerplate
Look, I get it. The entity doing business as “Gooey.AI” and/or “Dara.network” was incorporated in the State of Delaware in 2020 as Dara Network Inc. and is gonna need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Dara Network Inc can use, modify, copy, and redistribute my contributions, under its choice of terms.