Conversation
WalkthroughThis pull request refactors the popover infrastructure and sidebar UI components from a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Build artifacts for all platforms are ready! 🚀Download the artifacts for: One-line installer (Unstable):bunx flow-debug-build@1.2.1 --open 25229045599(execution 25229045599 / attempt 1) |
Greptile SummaryThis PR replaces the Radix UI-based popover with a
Confidence Score: 3/5Not safe to merge as-is due to the nested interactive element bug in ExtensionGridTile, though SiteControls is currently commented out. One clear P1 (nested button in button), one P2 with potential real rendering consequence (portal container timing), and a misleading type in BubbleEvent. The P1 is in new code that isn't yet wired up (SiteControls is commented out), which slightly limits immediate user impact but doesn't change the validity of the bug. site-controls/extensions.tsx (nested button), portal/popover.tsx (portalContainerRef timing), bubble-event.tsx (KeyboardEvent hardcoding) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Popover open = true] --> B[portal/popover.tsx\nPopoverContent renders]
B --> C[useDelayedOpenValue\ndelayedOpen = true]
C --> D{delayedOpen?}
D -- false --> E[return null\nunmount tree]
D -- true --> F[portalContainerRef = useRef null]
F --> G[BasePopoverContent\nportalContainer = ref]
F --> H[PortalComponent\nportalBodyRef = ref]
H --> I[useEffect\nref.current = portal.window.body]
G --> J{ref.current?}
J -- null on first render --> K[Portals to main\ndocument.body]
J -- set after effect --> L[Portals to overlay\nwindow body]
I -.->|ref mutation no re-render| J
M[User triggers popover] --> N[Popover / PopoverTrigger\nui/popover.tsx]
N --> O[portal/popover.tsx\nPopover sets context open]
O --> A
P[BubbleEvent] --> Q[document keydown listener]
Q --> R[dispatches KeyboardEvent\nto Command ref]
R --> S[cmdk handles\narrow key navigation]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| <button | ||
| className={cn( | ||
| "relative flex items-center justify-center rounded-md group/button", | ||
| "w-11.5 h-8 shrink-0", | ||
| "bg-black/10 dark:bg-white/10", | ||
| "hover:bg-black/20 dark:hover:bg-white/20", | ||
| "transition-colors duration-150", | ||
| className | ||
| )} | ||
| {...props} | ||
| > | ||
| {children} | ||
| </button> | ||
| ); | ||
| } |
There was a problem hiding this comment.
<button> inside <button> — invalid HTML
ExtensionButtonContainer renders a <button> element, and ExtensionGridTile places another <button> (the pin overlay) as a direct child. The HTML spec forbids interactive content inside <button>, so browsers implicitly close the outer button before the inner one is reached. This means clicks on the pin icon may not reach the outer onClick handler as expected, and event behaviour between the two buttons becomes browser-dependent.
Consider rendering the outer element as a <div> with role="button" / tabIndex={0} / onKeyDown, or using a CSS-only overlay (e.g., <span> with pointer-events:auto) for the pin affordance instead of a nested <button>.
| const handler = (event: DocumentEventMap[EventType]) => { | ||
| if (bubbleTarget.contains(event.target as Node)) return; | ||
| const cloned = new KeyboardEvent(event.type, event); | ||
| bubbleTarget.dispatchEvent(cloned); |
There was a problem hiding this comment.
KeyboardEvent hardcoded for generic event type
The component is typed generically as EventType extends keyof DocumentEventMap, accepting any event name, but the handler always constructs a new KeyboardEvent(event.type, event). If eventType is ever anything other than "keydown" / "keyup" / "keypress", a KeyboardEvent is dispatched instead of the correct event subtype (e.g., MouseEvent, WheelEvent), which will silently deliver the wrong event to listeners and break any handler inspecting mouse-specific properties.
Either narrow the type to "keydown" | "keyup" | "keypress" to match the implementation, or make the event constructor dynamic (new (event.constructor as typeof Event)(event.type, event)).
| export function PopoverContent({ ...props }: Omit<React.ComponentProps<typeof BasePopoverContent>, "portalContainer">) { | ||
| const { open } = usePopover(); | ||
| const delayedOpen = useDelayedOpenValue(open, 200); | ||
| const portalContainerRef = useRef<HTMLDivElement>(null); | ||
| if (!delayedOpen) return null; | ||
|
|
||
| return ( | ||
| <AnimatePresence mode="wait"> | ||
| {open && ( | ||
| <motion.div initial={{ opacity: 1 }} exit={{ opacity: 0 }} transition={{ duration: 0.15 }}> | ||
| <PortalComponent autoFocus className="w-screen h-screen absolute top-0 left-0" zIndex={ViewLayer.POPOVER}> | ||
| <PopoverContent {...props} portal={false}> | ||
| <PopoverArrow className="fill-popover h-2 w-4 outline-hidden stroke-border" /> | ||
| {children} | ||
| </PopoverContent> | ||
| </PortalComponent> | ||
| </motion.div> | ||
| )} | ||
| </AnimatePresence> | ||
| <> | ||
| <BasePopoverContent {...props} portalContainer={portalContainerRef} /> | ||
| <PortalComponent | ||
| visible={delayedOpen} | ||
| autoFocus | ||
| className="w-screen h-screen absolute top-0 left-0" | ||
| zIndex={ViewLayer.POPOVER} | ||
| portalBodyRef={portalContainerRef} | ||
| /> | ||
| </> | ||
| ); | ||
| } |
There was a problem hiding this comment.
portalContainerRef is null on first render
portalContainerRef starts as null. BasePopoverContent receives it immediately as portalContainer, but PortalComponent only sets portalContainerRef.current = portal.window.document.body inside a useEffect — which runs after the first paint. Since a ref mutation does not trigger a re-render, base-ui's portal never learns about the updated container, and the popover content will render into the main window's document.body rather than the overlay window.
One approach: gate the BasePopoverContent render on portalContainerRef.current being non-null by tracking it in a state variable updated after the portal body is ready.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/components/portal/popover.tsx (1)
90-97:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale error message references removed API.
The error message says
"usePopover must be used within a PortalPopover.Root"butPortalPopover.Rootno longer exists in the codebase. The component is now justPopover.🐛 Proposed fix
export const usePopover = () => { const context = useContext(PopoverContext); if (!context) { - throw new Error("usePopover must be used within a PortalPopover.Root"); + throw new Error("usePopover must be used within a Popover"); } return context; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/portal/popover.tsx` around lines 90 - 97, The error thrown in usePopover references a non-existent component name; update the message in the usePopover hook (which checks PopoverContext) to reference the current component name (Popover) instead of "PortalPopover.Root". Locate the usePopover function and replace the stale error text so it reads something like "usePopover must be used within a Popover" (or similar) to match the Popover component API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/contributing/dependencies.md`:
- Line 84: The renderer dependency list is out of alphabetical order because
`@base-ui/react` was appended after `@chenglou/pretext`; reorder the entries so the
list is A–Z sorted—move the `@base-ui/react` line to the correct alphabetical
position before/after the appropriate entries (e.g., place `@base-ui/react` in
proper order relative to `@chenglou/pretext`) so the renderer dependency section
complies with the documented sorting rule.
In
`@src/renderer/src/components/browser-ui/browser-sidebar/_components/browser-action-list.tsx`:
- Around line 171-183: The PopoverTrigger used for the icon-only button (the
PopoverTrigger wrapping PuzzleIcon) lacks an accessible name; update the
PopoverTrigger to provide one (e.g., add aria-label="Open extensions menu" or
aria-labelledby pointing to a visually-hidden text) so screen readers can
identify the control; ensure the label is meaningful (e.g., "Open extensions
menu" or "Extensions") and attach it to the PopoverTrigger that contains
PuzzleIcon to make the trigger discoverable and operable by assistive tech.
In
`@src/renderer/src/components/browser-ui/browser-sidebar/_components/navigation-controls.tsx`:
- Around line 154-167: The popover for history (PopoverContent / CommandList)
currently grows indefinitely; limit its height and make the list scrollable so
older entries remain reachable. Update the PopoverContent (or the CommandList
wrapper) to include a max-height (e.g., max-h-64 / max-h-[calc(...)] or
equivalent token) and overflow-y-auto while preserving existing padding and
truncate behavior on each CommandItem; ensure the scrollable container wraps the
mapped entries (entries.map) so onSelect/onActivateHistory still works as
before.
In
`@src/renderer/src/components/browser-ui/browser-sidebar/_components/site-controls/extensions.tsx`:
- Around line 151-160: The onContextMenu handler currently returns the result of
onActivated("contextmenu") even though onActivated returns void and React
ignores handler return values; remove the misleading return and simply call
onActivated("contextmenu") after stopping propagation. Update the onContextMenu
useCallback (and keep onClick as-is) so it invokes onActivated("contextmenu")
without returning its value.
- Around line 63-79: The isError flag set by setIsError is not cleared when the
iconUrl changes (e.g., when action.id or activeTabId updates), causing a
persistent PuzzleIcon fallback; fix this by importing and using React's
useEffect in the component and add an effect that watches the computed iconUrl
(or its constituents like action.id and activeTabId) and calls setIsError(false)
whenever it changes so the image reload can attempt again; ensure you reference
the existing isError/setIsError state and the iconUrl variable in the effect
dependency list.
- Around line 8-22: Export the existing ExtensionAction and Action type
definitions from browser-action-provider (add export to their declarations in
browser-action-provider.tsx), remove the duplicate local definitions in
extensions.tsx and browser-action-list.tsx, and import the types from the
provider into those components (import { ExtensionAction, Action } from
'browser-action-provider' or the correct module export). Ensure the exported
names exactly match the identifiers used in extensions.tsx and
browser-action-list.tsx so existing usages (tabs, id, popup, iconModified, etc.)
compile without changes.
In
`@src/renderer/src/components/browser-ui/browser-sidebar/_components/site-controls/index.tsx`:
- Around line 54-60: The Button instances using className with the hardcoded
"text-white" (see the two Button elements containing LockIcon and EllipsisIcon
and the cn(...) calls) should use theme-aware tokens instead (e.g.,
"text-foreground" or a popover-specific token like "text-popover-foreground") so
text/icons remain visible if the surrounding variant or theme changes; update
both Button className strings to replace "text-white" with the chosen token and
ensure the icons inherit color (use currentColor-compatible icon classes) so
they follow the new foreground color.
In `@src/renderer/src/components/portal/popover.tsx`:
- Around line 52-65: The useEffect in popover.tsx currently returns an
unnecessary empty cleanup when value === true; remove the `return () => {};` so
the effect simply sets setDelayedValue(value) and returns nothing in that
branch, leaving the existing timeout/clearTimeout cleanup in the else branch
intact; locate the useEffect that references value, delay, and setDelayedValue
to apply this change.
In `@src/renderer/src/components/portal/portal.tsx`:
- Around line 67-71: The effect in portal.tsx sets portalBodyRef.current to
portal.window.document.body but never clears it, leaving a stale reference when
the portal/window unmounts or closes; update the useEffect that reads portal and
portalBodyRef so that when portal or portal.window is falsy you set
portalBodyRef.current = null (guarding that portalBodyRef exists) and also
return a cleanup function that clears portalBodyRef.current = null on unmount,
ensuring callers don't keep a detached document body; refer to the existing
useEffect handling portal/window and the portalBodyRef symbol to implement these
changes.
---
Outside diff comments:
In `@src/renderer/src/components/portal/popover.tsx`:
- Around line 90-97: The error thrown in usePopover references a non-existent
component name; update the message in the usePopover hook (which checks
PopoverContext) to reference the current component name (Popover) instead of
"PortalPopover.Root". Locate the usePopover function and replace the stale error
text so it reads something like "usePopover must be used within a Popover" (or
similar) to match the Popover component API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e68541db-98f8-4127-bbdf-e9f8719982cd
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
docs/contributing/dependencies.mdpackage.jsonsrc/renderer/src/components/browser-ui/browser-sidebar/_components/address-bar.tsxsrc/renderer/src/components/browser-ui/browser-sidebar/_components/bottom/bottom-extras-menu.tsxsrc/renderer/src/components/browser-ui/browser-sidebar/_components/browser-action-list.tsxsrc/renderer/src/components/browser-ui/browser-sidebar/_components/navigation-controls.tsxsrc/renderer/src/components/browser-ui/browser-sidebar/_components/site-controls/extensions.tsxsrc/renderer/src/components/browser-ui/browser-sidebar/_components/site-controls/index.tsxsrc/renderer/src/components/logic/bubble-event.tsxsrc/renderer/src/components/portal/popover.tsxsrc/renderer/src/components/portal/portal.tsxsrc/renderer/src/components/ui/popover-listbox.tsxsrc/renderer/src/components/ui/popover.tsxsrc/renderer/src/routes/extensions/page.tsx
💤 Files with no reviewable changes (1)
- src/renderer/src/components/ui/popover-listbox.tsx
| - nuqs - Use for managing query parameters in the URL. | ||
| - @tanstack/react-query - React Query for the frontend. | ||
| - @chenglou/pretext - Calculate text width for the frontend. | ||
| - @base-ui/react - UI Components for the frontend. |
There was a problem hiding this comment.
Keep this list A–Z sorted.
@base-ui/react was appended after @chenglou/pretext, so the renderer dependency section no longer matches the ordering rule documented at the top of this file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/contributing/dependencies.md` at line 84, The renderer dependency list
is out of alphabetical order because `@base-ui/react` was appended after
`@chenglou/pretext`; reorder the entries so the list is A–Z sorted—move the
`@base-ui/react` line to the correct alphabetical position before/after the
appropriate entries (e.g., place `@base-ui/react` in proper order relative to
`@chenglou/pretext`) so the renderer dependency section complies with the
documented sorting rule.
| <PopoverTrigger | ||
| className={cn( | ||
| "size-6 flex items-center justify-center rounded-md", | ||
| "hover:bg-black/15 dark:hover:bg-white/20", | ||
| "transition-colors duration-150", | ||
| "relative shrink-0" | ||
| )} | ||
| onClick={(event) => { | ||
| event.stopPropagation(); | ||
| }} | ||
| > | ||
| <PuzzleIcon strokeWidth={2} className="size-4 text-black/80 dark:text-white/80" /> | ||
| </PopoverTrigger> |
There was a problem hiding this comment.
Add an accessible name to this icon-only trigger.
Right now this will be exposed as an unlabeled button, which makes the extensions menu hard to discover and operate with assistive tech.
Proposed fix
<PopoverTrigger
+ aria-label="Open extensions menu"
+ title="Open extensions menu"
className={cn(
"size-6 flex items-center justify-center rounded-md",
"hover:bg-black/15 dark:hover:bg-white/20",📝 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.
| <PopoverTrigger | |
| className={cn( | |
| "size-6 flex items-center justify-center rounded-md", | |
| "hover:bg-black/15 dark:hover:bg-white/20", | |
| "transition-colors duration-150", | |
| "relative shrink-0" | |
| )} | |
| onClick={(event) => { | |
| event.stopPropagation(); | |
| }} | |
| > | |
| <PuzzleIcon strokeWidth={2} className="size-4 text-black/80 dark:text-white/80" /> | |
| </PopoverTrigger> | |
| <PopoverTrigger | |
| aria-label="Open extensions menu" | |
| title="Open extensions menu" | |
| className={cn( | |
| "size-6 flex items-center justify-center rounded-md", | |
| "hover:bg-black/15 dark:hover:bg-white/20", | |
| "transition-colors duration-150", | |
| "relative shrink-0" | |
| )} | |
| onClick={(event) => { | |
| event.stopPropagation(); | |
| }} | |
| > | |
| <PuzzleIcon strokeWidth={2} className="size-4 text-black/80 dark:text-white/80" /> | |
| </PopoverTrigger> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/renderer/src/components/browser-ui/browser-sidebar/_components/browser-action-list.tsx`
around lines 171 - 183, The PopoverTrigger used for the icon-only button (the
PopoverTrigger wrapping PuzzleIcon) lacks an accessible name; update the
PopoverTrigger to provide one (e.g., add aria-label="Open extensions menu" or
aria-labelledby pointing to a visually-hidden text) so screen readers can
identify the control; ensure the label is meaningful (e.g., "Open extensions
menu" or "Extensions") and attach it to the PopoverTrigger that contains
PuzzleIcon to make the trigger discoverable and operable by assistive tech.
| <PopoverContent className={cn("w-56 p-2 select-none")} positionerClassName={spaceInjectedClasses}> | ||
| <Command ref={commandRef} loop> | ||
| <BubbleEvent targetRef={commandRef} eventType="keydown" /> | ||
| <CommandList> | ||
| {entries.map((entry) => ( | ||
| <CommandItem | ||
| key={entry.index} | ||
| value={entry.index.toString()} | ||
| onSelect={() => onActivateHistory(entry)} | ||
| > | ||
| <span className="truncate">{entry.title || entry.url}</span> | ||
| </CommandItem> | ||
| ))} | ||
| </CommandList> |
There was a problem hiding this comment.
Cap and scroll the history list.
A tab can easily have more history entries than fit on screen. In the current shape the popover grows indefinitely, so older entries can end up off-screen and unreachable.
Proposed fix
<PopoverContent className={cn("w-56 p-2 select-none")} positionerClassName={spaceInjectedClasses}>
<Command ref={commandRef} loop>
<BubbleEvent targetRef={commandRef} eventType="keydown" />
- <CommandList>
- {entries.map((entry) => (
- <CommandItem
- key={entry.index}
- value={entry.index.toString()}
- onSelect={() => onActivateHistory(entry)}
- >
- <span className="truncate">{entry.title || entry.url}</span>
- </CommandItem>
- ))}
- </CommandList>
+ <div className="max-h-80 overflow-y-auto">
+ <CommandList>
+ {entries.map((entry) => (
+ <CommandItem
+ key={entry.index}
+ value={entry.index.toString()}
+ onSelect={() => onActivateHistory(entry)}
+ >
+ <span className="truncate">{entry.title || entry.url}</span>
+ </CommandItem>
+ ))}
+ </CommandList>
+ </div>
</Command>
</PopoverContent>📝 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.
| <PopoverContent className={cn("w-56 p-2 select-none")} positionerClassName={spaceInjectedClasses}> | |
| <Command ref={commandRef} loop> | |
| <BubbleEvent targetRef={commandRef} eventType="keydown" /> | |
| <CommandList> | |
| {entries.map((entry) => ( | |
| <CommandItem | |
| key={entry.index} | |
| value={entry.index.toString()} | |
| onSelect={() => onActivateHistory(entry)} | |
| > | |
| <span className="truncate">{entry.title || entry.url}</span> | |
| </CommandItem> | |
| ))} | |
| </CommandList> | |
| <PopoverContent className={cn("w-56 p-2 select-none")} positionerClassName={spaceInjectedClasses}> | |
| <Command ref={commandRef} loop> | |
| <BubbleEvent targetRef={commandRef} eventType="keydown" /> | |
| <div className="max-h-80 overflow-y-auto"> | |
| <CommandList> | |
| {entries.map((entry) => ( | |
| <CommandItem | |
| key={entry.index} | |
| value={entry.index.toString()} | |
| onSelect={() => onActivateHistory(entry)} | |
| > | |
| <span className="truncate">{entry.title || entry.url}</span> | |
| </CommandItem> | |
| ))} | |
| </CommandList> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/renderer/src/components/browser-ui/browser-sidebar/_components/navigation-controls.tsx`
around lines 154 - 167, The popover for history (PopoverContent / CommandList)
currently grows indefinitely; limit its height and make the list scrollable so
older entries remain reachable. Update the PopoverContent (or the CommandList
wrapper) to include a max-height (e.g., max-h-64 / max-h-[calc(...)] or
equivalent token) and overflow-y-auto while preserving existing padding and
truncate behavior on each CommandItem; ensure the scrollable container wraps the
mapped entries (entries.map) so onSelect/onActivateHistory still works as
before.
| interface ExtensionAction { | ||
| color?: string; | ||
| text?: string; | ||
| title?: string; | ||
| icon?: chrome.browserAction.TabIconDetails; | ||
| popup?: string; | ||
| iconModified?: number; | ||
| } | ||
|
|
||
| interface Action { | ||
| id: string; | ||
| title: string; | ||
| popup: string; | ||
| tabs: Record<string, ExtensionAction>; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Action/ExtensionAction types are exported from the provider
rg -n "export (type|interface) (Action|ExtensionAction)" src/renderer/src/components/providers/browser-action-provider.tsxRepository: MultiboxLabs/flow-browser
Length of output: 51
🏁 Script executed:
# First, check if the extensions.tsx file exists and its imports
head -30 "src/renderer/src/components/browser-ui/browser-sidebar/_components/site-controls/extensions.tsx"Repository: MultiboxLabs/flow-browser
Length of output: 1052
🏁 Script executed:
# Search for where Action and ExtensionAction types are defined in the codebase
rg -n "(export )?(type|interface) (Action|ExtensionAction)" --type ts --type tsxRepository: MultiboxLabs/flow-browser
Length of output: 97
🏁 Script executed:
# Check if browser-action-provider.tsx exists and what it contains
find . -name "*browser-action-provider*" -type fRepository: MultiboxLabs/flow-browser
Length of output: 137
🏁 Script executed:
# Search for useBrowserAction hook to understand where it's defined
rg -n "useBrowserAction" --type ts --type tsx -A 2Repository: MultiboxLabs/flow-browser
Length of output: 97
🏁 Script executed:
# Read the browser-action-provider.tsx to see what types it exports
cat "src/renderer/src/components/providers/browser-action-provider.tsx"Repository: MultiboxLabs/flow-browser
Length of output: 5236
🏁 Script executed:
# Search for Action and ExtensionAction definitions without type filter
rg -n "(export )?(type|interface) (Action|ExtensionAction)"Repository: MultiboxLabs/flow-browser
Length of output: 920
Export the Action and ExtensionAction types from the browser-action-provider and import them instead of redefining locally.
These types are currently defined in the provider but not exported. They're being redefined identically in both extensions.tsx and browser-action-list.tsx, which violates DRY and risks drift. Export them from browser-action-provider.tsx and import them in the components that use them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/renderer/src/components/browser-ui/browser-sidebar/_components/site-controls/extensions.tsx`
around lines 8 - 22, Export the existing ExtensionAction and Action type
definitions from browser-action-provider (add export to their declarations in
browser-action-provider.tsx), remove the duplicate local definitions in
extensions.tsx and browser-action-list.tsx, and import the types from the
provider into those components (import { ExtensionAction, Action } from
'browser-action-provider' or the correct module export). Ensure the exported
names exactly match the identifiers used in extensions.tsx and
browser-action-list.tsx so existing usages (tabs, id, popup, iconModified, etc.)
compile without changes.
| const { iconModified } = { ...action, ...tabInfo }; | ||
| const [isError, setIsError] = useState(false); | ||
| const iconSize = 32; | ||
| const resizeType = 2; | ||
| const timeParam = iconModified ? `&t=${iconModified}` : ""; | ||
| const iconUrl = `crx://extension-icon/${action.id}/${iconSize}/${resizeType}?tabId=${activeTabId}${timeParam}&partition=${encodeURIComponent(partitionId)}`; | ||
|
|
||
| if (isError) { | ||
| return <PuzzleIcon className="size-4 shrink-0" />; | ||
| } | ||
|
|
||
| return ( | ||
| <svg className="size-4 shrink-0"> | ||
| {/* eslint-disable-next-line react/no-unknown-property */} | ||
| <image href={iconUrl} className="size-4 object-contain" onError={() => setIsError(true)} /> | ||
| </svg> | ||
| ); |
There was a problem hiding this comment.
isError state persists incorrectly when icon URL changes.
The isError state is never reset when action.id or activeTabId changes. If an icon fails to load, then the user switches tabs or extensions, the error state persists and shows the PuzzleIcon fallback for a different (potentially valid) extension icon.
🐛 Proposed fix: Reset error state when icon URL changes
function BrowserActionIcon({
action,
activeTabId,
tabInfo,
partitionId
}: {
action: Action;
activeTabId: number;
tabInfo: ExtensionAction | null;
partitionId: string;
}) {
const { iconModified } = { ...action, ...tabInfo };
const [isError, setIsError] = useState(false);
const iconSize = 32;
const resizeType = 2;
const timeParam = iconModified ? `&t=${iconModified}` : "";
const iconUrl = `crx://extension-icon/${action.id}/${iconSize}/${resizeType}?tabId=${activeTabId}${timeParam}&partition=${encodeURIComponent(partitionId)}`;
+ // Reset error state when iconUrl changes
+ useEffect(() => {
+ setIsError(false);
+ }, [iconUrl]);
+
if (isError) {
return <PuzzleIcon className="size-4 shrink-0" />;
}This requires adding useEffect to the imports on line 6.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/renderer/src/components/browser-ui/browser-sidebar/_components/site-controls/extensions.tsx`
around lines 63 - 79, The isError flag set by setIsError is not cleared when the
iconUrl changes (e.g., when action.id or activeTabId updates), causing a
persistent PuzzleIcon fallback; fix this by importing and using React's
useEffect in the component and add an effect that watches the computed iconUrl
(or its constituents like action.id and activeTabId) and calls setIsError(false)
whenever it changes so the image reload can attempt again; ensure you reference
the existing isError/setIsError state and the iconUrl variable in the effect
dependency list.
| const onClick = useCallback(() => onActivated("click"), [onActivated]); | ||
|
|
||
| const onContextMenu = useCallback( | ||
| (event: MouseEvent<HTMLButtonElement>) => { | ||
| event.stopPropagation(); | ||
| event.nativeEvent.stopImmediatePropagation(); | ||
| return onActivated("contextmenu"); | ||
| }, | ||
| [onActivated] | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Return value from context menu handler is unused.
Line 157 returns the result of onActivated("contextmenu"), but onActivated returns void and React's onContextMenu handler ignores the return value. The return statement is harmless but misleading.
♻️ Suggested cleanup
const onContextMenu = useCallback(
(event: MouseEvent<HTMLButtonElement>) => {
event.stopPropagation();
event.nativeEvent.stopImmediatePropagation();
- return onActivated("contextmenu");
+ onActivated("contextmenu");
},
[onActivated]
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/renderer/src/components/browser-ui/browser-sidebar/_components/site-controls/extensions.tsx`
around lines 151 - 160, The onContextMenu handler currently returns the result
of onActivated("contextmenu") even though onActivated returns void and React
ignores handler return values; remove the misleading return and simply call
onActivated("contextmenu") after stopping propagation. Update the onContextMenu
useCallback (and keep onClick as-is) so it invokes onActivated("contextmenu")
without returning its value.
| <Button className={cn("justify-start rounded-md", "text-white h-8", "bg-white/5 hover:bg-white/10")}> | ||
| <LockIcon className="size-4" /> | ||
| <span className="font-medium truncate">Secure</span> | ||
| </Button> | ||
| <Button className={cn("justify-center rounded-full", "text-white size-8", "bg-white/5 hover:bg-white/10")}> | ||
| <EllipsisIcon className="size-4" /> | ||
| </Button> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Hardcoded text-white may break in light mode context.
The utility buttons use text-white unconditionally, but the popover uses variant="translucent" which forces dark mode. If the variant logic changes or these buttons are reused elsewhere, the white text won't be visible on light backgrounds.
Consider using theme-aware colors like text-foreground or text-popover-foreground for better maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/renderer/src/components/browser-ui/browser-sidebar/_components/site-controls/index.tsx`
around lines 54 - 60, The Button instances using className with the hardcoded
"text-white" (see the two Button elements containing LockIcon and EllipsisIcon
and the cn(...) calls) should use theme-aware tokens instead (e.g.,
"text-foreground" or a popover-specific token like "text-popover-foreground") so
text/icons remain visible if the surrounding variant or theme changes; update
both Button className strings to replace "text-white" with the chosen token and
ensure the icons inherit color (use currentColor-compatible icon classes) so
they follow the new foreground color.
| useEffect(() => { | ||
| if (value === true) { | ||
| setDelayedValue(value); | ||
| return () => {}; | ||
| } else { | ||
| const timer = setTimeout(() => { | ||
| setDelayedValue(value); | ||
| }, delay); | ||
|
|
||
| return () => { | ||
| clearTimeout(timer); | ||
| }; | ||
| } | ||
| }, [value, delay]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Simplify cleanup function in effect.
The empty cleanup function return () => {}; on line 55 is unnecessary. When no cleanup is needed, the effect can simply not return anything.
♻️ Suggested simplification
useEffect(() => {
if (value === true) {
setDelayedValue(value);
- return () => {};
+ return;
} else {
const timer = setTimeout(() => {
setDelayedValue(value);
}, delay);
return () => {
clearTimeout(timer);
};
}
}, [value, delay]);📝 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.
| useEffect(() => { | |
| if (value === true) { | |
| setDelayedValue(value); | |
| return () => {}; | |
| } else { | |
| const timer = setTimeout(() => { | |
| setDelayedValue(value); | |
| }, delay); | |
| return () => { | |
| clearTimeout(timer); | |
| }; | |
| } | |
| }, [value, delay]); | |
| useEffect(() => { | |
| if (value === true) { | |
| setDelayedValue(value); | |
| return; | |
| } else { | |
| const timer = setTimeout(() => { | |
| setDelayedValue(value); | |
| }, delay); | |
| return () => { | |
| clearTimeout(timer); | |
| }; | |
| } | |
| }, [value, delay]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/components/portal/popover.tsx` around lines 52 - 65, The
useEffect in popover.tsx currently returns an unnecessary empty cleanup when
value === true; remove the `return () => {};` so the effect simply sets
setDelayedValue(value) and returns nothing in that branch, leaving the existing
timeout/clearTimeout cleanup in the else branch intact; locate the useEffect
that references value, delay, and setDelayedValue to apply this change.
| useEffect(() => { | ||
| if (!portal?.window) return; | ||
| if (!portalBodyRef) return; | ||
| portalBodyRef.current = portal.window.document.body; | ||
| }, [portal, portalBodyRef]); |
There was a problem hiding this comment.
Clear portalBodyRef when the portal disappears.
This effect only writes the portal body once. If the portal window closes or this component unmounts, callers keep a stale body reference and can mount follow-up overlays into a detached window.
Proposed fix
useEffect(() => {
- if (!portal?.window) return;
- if (!portalBodyRef) return;
- portalBodyRef.current = portal.window.document.body;
+ if (!portalBodyRef) return;
+ portalBodyRef.current = portal?.window?.document.body ?? null;
+
+ return () => {
+ portalBodyRef.current = null;
+ };
}, [portal, portalBodyRef]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/src/components/portal/portal.tsx` around lines 67 - 71, The
effect in portal.tsx sets portalBodyRef.current to portal.window.document.body
but never clears it, leaving a stale reference when the portal/window unmounts
or closes; update the useEffect that reads portal and portalBodyRef so that when
portal or portal.window is falsy you set portalBodyRef.current = null (guarding
that portalBodyRef exists) and also return a cleanup function that clears
portalBodyRef.current = null on unmount, ensuring callers don't keep a detached
document body; refer to the existing useEffect handling portal/window and the
portalBodyRef symbol to implement these changes.
1 participant
Summary by CodeRabbit
Release Notes