Conversation
WalkthroughThis set of changes introduces a new portal-based component system for rendering floating UI elements, such as popovers and sidebars, into separate browser windows using Electron. It adds comprehensive documentation and implementation for the Changes
Sequence Diagram(s)sequenceDiagram
participant ReactApp
participant PortalComponent
participant ElectronPreload
participant ElectronMain
participant TabbedBrowserWindow
participant WebContentsView
ReactApp->>PortalComponent: Mount with props (x, y, width, height, zIndex, visible)
PortalComponent->>ElectronPreload: setComponentWindowBounds(componentId, bounds)
PortalComponent->>ElectronPreload: setComponentWindowZIndex(componentId, zIndex)
PortalComponent->>ElectronPreload: setComponentWindowVisible(componentId, visible)
ElectronPreload->>ElectronMain: IPC: setComponentWindowBounds
ElectronMain->>TabbedBrowserWindow: Find/Create WebContentsView for componentId
TabbedBrowserWindow->>WebContentsView: Set bounds/z-index/visibility
ReactApp->>PortalComponent: Unmount
PortalComponent->>ElectronPreload: setComponentWindowVisible(componentId, false)
ElectronPreload->>ElectronMain: IPC: setComponentWindowVisible
ElectronMain->>TabbedBrowserWindow: Hide WebContentsView
sequenceDiagram
participant BrowserSidebar
participant PortalComponent
participant MainWindow
participant ElectronPreload
participant ElectronMain
BrowserSidebar->>PortalComponent: Render floating sidebar (variant: floating)
PortalComponent->>ElectronPreload: setComponentWindowBounds (sidebar window)
ElectronPreload->>ElectronMain: IPC: setComponentWindowBounds
ElectronMain->>MainWindow: Add/Update sidebar as overlay window
BrowserSidebar->>PortalComponent: Unmount on close/exit
PortalComponent->>ElectronPreload: setComponentWindowVisible (sidebar window, false)
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (22)
vite/src/main.tsx (1)
6-8: Consider adding a comment explaining StrictMode toggleThe StrictMode toggle is a useful feature for development and debugging, especially when working with portal components that may have complex rendering behaviors.
+// Toggle StrictMode for development - can help debug issues with portal components const STRICT_MODE_ENABLED = true;vite/src/components/providers/spaces-provider.tsx (1)
124-137: Good use of portals for style injection.Moving the style element to document.head using React's
createPortalis a better approach than rendering it inline with the component tree. This separates style concerns from the component hierarchy.For optimization, consider memoizing the stylesheet to prevent recreation on every render:
- // Stylesheet Portal - const stylesheet = ( - <style> - {currentSpace - ? ` - :root { - --space-background-start: ${bgStart}; - --space-background-end: ${bgEnd}; - } -` - : ""} - </style> - ); + // Stylesheet Portal + const stylesheet = React.useMemo(() => ( + <style> + {currentSpace + ? ` + :root { + --space-background-start: ${bgStart}; + --space-background-end: ${bgEnd}; + } +` + : ""} + </style> + ), [currentSpace, bgStart, bgEnd]);electron/browser/components/omnibox.ts (1)
210-224: Good bounds validation to keep omnibox within window.The implementation now properly ensures the omnibox stays within the parent window's bounds by clamping the coordinates. This prevents UI elements from being positioned off-screen.
Consider adding debug logging when bounds are adjusted to help with troubleshooting:
if (bounds) { const windowBounds = parentWindow.getBounds(); const newBounds: Electron.Rectangle = { x: Math.min(bounds.x, windowBounds.width - bounds.width), y: Math.min(bounds.y, windowBounds.height - bounds.height), width: bounds.width, height: bounds.height }; + // Log if bounds were adjusted + if (newBounds.x !== bounds.x || newBounds.y !== bounds.y) { + debugPrint("OMNIBOX", `Adjusted bounds from (${bounds.x}, ${bounds.y}) to (${newBounds.x}, ${newBounds.y}) to keep within window`); + } omnibox.setBounds(newBounds); } else { omnibox.setBounds(null); }vite/src/lib/utils.ts (1)
15-34: Improved clipboard implementation with fallback mechanism.Good implementation of a fallback mechanism for clipboard operations. The code now tries the web API first and falls back to IPC if needed, with proper success tracking.
Consider adding type guard checks before calling the fallback:
if (!writeSuccess) { + if (typeof flow?.app?.writeTextToClipboard === 'function') { writeSuccess = await copyTextToClipboardFallback(text); + } }vite/src/components/browser-ui/sidebar/hover-detector.tsx (1)
26-35: Consider accessibility improvements for the hover detector.The component creates an invisible interactive area that might cause issues for screen readers or keyboard navigation.
<div ref={ref} className={cn( "remove-app-drag absolute top-0 w-2 h-full overflow-hidden z-50", side === "left" ? "left-0" : "right-0" )} + aria-hidden="true" + role="none" />Also consider whether the z-index of 50 might conflict with other high z-index elements in the application.
vite/src/hooks/use-sidebar-resize.tsx (1)
94-95: Well-implemented direction-aware calculation.The direction inversion for right-side resizing is correctly implemented. The comment helps explain the purpose of the change.
Consider renaming the variable to something like
directionAwareDeltato make it even clearer that this value accounts for the sidebar's position:-// Calculate new width in pixels with right side inversion if needed -const deltaWidth = isRightSide ? startX.current - e.clientX : e.clientX - startX.current; +// Calculate direction-aware delta based on sidebar position +const directionAwareDelta = isRightSide ? startX.current - e.clientX : e.clientX - startX.current;electron/browser/window.ts (1)
184-184: Portal component windows initialization.This replaces the previous glance modal system with the new portal-based component system for floating UI elements.
Consider adding a brief comment explaining what the initialization does, to make the code more self-documenting:
-initializePortalComponentWindows(this); +// Initialize the portal component system for floating UI elements (popovers, sidebars, etc.) +initializePortalComponentWindows(this);vite/src/lib/flow/interfaces/browser/interface.ts (3)
24-27: Well-structured component window bounds API.The
setComponentWindowBoundsmethod provides a clean API for positioning portal component windows.Consider enhancing the documentation with more details:
/** * Sets the bounds of a component window + * @param componentId Unique identifier for the component window + * @param bounds Position and size of the component window */
29-32: Clear z-index control API.The
setComponentWindowZIndexmethod allows proper stacking of multiple portal component windows.Consider enhancing the documentation with more details:
/** * Sets the z-index of a component window + * @param componentId Unique identifier for the component window + * @param zIndex Stacking order for the component window (higher values appear on top) */
34-37: Good visibility control API.The
setComponentWindowVisiblemethod allows showing and hiding portal component windows without recreating them.Consider enhancing the documentation with more details:
/** * Sets the visibility of a component window + * @param componentId Unique identifier for the component window + * @param visible Whether the component window should be visible */electron/preload.ts (1)
302-305: Clipboard helper should surface errors and return a boolean.
app:write-text-to-clipboardcould theoretically fail (for example on Wayland when the seat is locked).
Returning aPromise<boolean>– or at least swallowing errors and logging – would make this call site safer and debuggable instead of silently failing.- writeTextToClipboard: (text: string) => { - return ipcRenderer.send("app:write-text-to-clipboard", text); - } + writeTextToClipboard: async (text: string): Promise<boolean> => { + try { + await ipcRenderer.invoke("app:write-text-to-clipboard", text); + return true; + } catch (err) { + console.error("[clipboard] failed:", err); + return false; + } + }vite/src/components/browser-ui/main.tsx (1)
27-31:sidesetting won’t react to runtime setting changes.
getSettingis called only during the initial render, so changing the Sidebar Side setting in the UI won’t move the sidebar until a full reload.
Wrapping the call in auseEffector – easier – using the reactive accessor that the settings provider probably offers would keep the UI in sync.vite/src/components/portal/popover.tsx (1)
33-57: Effective portal integration with animations.The
PortalPopoverContentcomponent:
- Successfully integrates with the portal system using
PortalComponent- Correctly positions content using portal context coordinates
- Implements smooth fade-out animation with
AnimatePresence- Properly styles the popover arrow
One suggestion to consider:
Consider extracting the animation configuration into constants or props to make it more customizable:
- <motion.div initial={{ opacity: 1 }} exit={{ opacity: 0 }} transition={{ duration: 0.15 }}> + <motion.div + initial={{ opacity: 1 }} + exit={{ opacity: 0 }} + transition={{ duration: props.animationDuration ?? 0.15 }}>vite/src/hooks/use-copy-styles.ts (1)
106-128: Robust style change detection with MutationObserver.The implementation properly:
- Performs initial style copying
- Sets up a MutationObserver to detect style changes
- Configures the observer with appropriate options
- Checks if the container is still valid before applying changes
- Cleans up the observer on unmount or when the container window changes
Consider adding error handling for edge cases:
// Initial style copy -copyStyles(); +try { + copyStyles(); +} catch (error) { + console.error("Failed to copy styles to portal:", error); +} // Set up a MutationObserver to watch for style changes in the main document const styleObserver = new MutationObserver(() => { if (containerWin && !containerWin.closed) { - copyStyles(); + try { + copyStyles(); + } catch (error) { + console.error("Failed to update styles in portal:", error); + } } });vite/src/components/portal/portal.tsx (2)
8-11: ID generation is prone to collision & debugging pain
Math.random().toString(36)gives only ~1 M distinct 8-char strings.
Replacing it withcrypto.randomUUID()(Electron/Chromium ≥ 92) or a nanoid
eliminates collision risk and makes logs/searches easier.
60-67:window.openlacks feature flags & initial sizingWithout
width,height,noopener,noreferrerortoolbar=0, the popup
behaves inconsistently across platforms (and may inherit the opener’s JS
context, defeating isolation).Consider:
-const containerWin = window.open("about:blank", windowName, `componentId=${newComponentId}`); +const features = [ + `popup`, + `noopener`, + `noreferrer`, + `width=${Math.round(widthInPixels)}`, + `height=${Math.round(heightInPixels)}`, +]; +const containerWin = window.open("about:blank", windowName, features.join(","));vite/src/hooks/use-css-size-to-pixels.tsx (1)
78-88: Observer may stay attached ifcssSizeStringloses%
parentObserveris created only when the string initially contains%.
If later renders switch to a non-percentage value the observer continues to
run unnecessarily. Store the observer in a ref and disconnect whenever the
condition becomes false.vite/src/hooks/use-bounding-rect.tsx (1)
20-48:updateRectruns outside React’s batching
requestAnimationFrame(updateRect)is good, but insideupdateRectyou invoke
setRectpotentially many times in quick succession because scroll, resize and
mutation events fire frequently. Wrapping the body ofupdateRectwith
unstable_batchedUpdates(React DOM export) or throttling at the event
handler level would reduce re-renders.vite/src/lib/css.ts (2)
55-65: Returning0on invalid input can mask bugsThe early-exit branch converts malformed strings and even server-side rendering (no
window) into a valid numeric result (0). This makes silent failures hard to detect downstream.Consider returning
NaN(and documenting that) or throwing for clearly invalid input; callers can then decide how to degrade gracefully.- console.warn(`cssSizeToPixels: Invalid input "${cssSizeString}". Returning 0.`); - return 0; + console.warn(`cssSizeToPixels: Invalid input "${cssSizeString}".`); + return Number.NaN;
172-195: Constant-allocate%property lookup tables
widthProps/heightPropsarrays are rebuilt on every invocation, allocating new arrays and hampering engine optimisation. Moving them outside the function (or making themconst Sets) eliminates per-call allocations and speeds up hot paths, especially if this util is used in layout-intensive code like resize observers.- const widthProps = [ + // hoist outside cssSizeToPixels: + // const WIDTH_PROPS = new Set([...]); + const widthProps = WIDTH_PROPS;docs/components/portal.md (2)
5-6: Tiny wording nit – drop “of”“rendered outside of the main DOM hierarchy”
The phrase “outside the main DOM hierarchy” is already clear. Removing “of” tightens the prose.-The Portal component allows content to be rendered outside of the main DOM hierarchy +The Portal component allows content to be rendered outside the main DOM hierarchy🧰 Tools
🪛 LanguageTool
[style] ~5-~5: This phrase is redundant. Consider using “outside”.
Context: ...component allows content to be rendered outside of the main DOM hierarchy into a separate ...(OUTSIDE_OF)
91-107: List punctuation is inconsistentA few bullet descriptions end with periods while most others do not (e.g., lines 91 & 105). Standardising punctuation improves readability—pick one style (period or none) and apply it consistently.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~91-~91: Loose punctuation mark.
Context: ...ns -initializePortalComponentWindows: Sets up IPC handlers and manages compon...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~105-~105: Loose punctuation mark.
Context: ...interface:set-component-window-bounds: Updates position and dimensions of comp...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~105-~105: You might be missing the article “the” here.
Context: ...e:set-component-window-bounds: Updates position and dimensions of component windows -...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
docs/components/portal.md(1 hunks)docs/references/view-indexes.md(1 hunks)electron/browser/components/glance-modal.ts(0 hunks)electron/browser/components/omnibox.ts(1 hunks)electron/browser/components/portal-component-windows.ts(1 hunks)electron/browser/tabs/tab.ts(0 hunks)electron/browser/utility/hot-reload.ts(1 hunks)electron/browser/view-manager.ts(2 hunks)electron/browser/window.ts(3 hunks)electron/ipc/app/app.ts(2 hunks)electron/modules/basic-settings.ts(3 hunks)electron/modules/output.ts(1 hunks)electron/preload.ts(2 hunks)vite/src/App.tsx(0 hunks)vite/src/components/browser-ui/browser-action.tsx(3 hunks)vite/src/components/browser-ui/browser-content.tsx(2 hunks)vite/src/components/browser-ui/browser-sidebar.tsx(3 hunks)vite/src/components/browser-ui/main.tsx(4 hunks)vite/src/components/browser-ui/sidebar/content/space-sidebar.tsx(1 hunks)vite/src/components/browser-ui/sidebar/header/action-buttons.tsx(1 hunks)vite/src/components/browser-ui/sidebar/header/navigation-buttons.tsx(4 hunks)vite/src/components/browser-ui/sidebar/header/window-controls.tsx(1 hunks)vite/src/components/browser-ui/sidebar/hover-detector.tsx(1 hunks)vite/src/components/browser-ui/sidebar/spaces-switcher.tsx(1 hunks)vite/src/components/portal/popover.tsx(1 hunks)vite/src/components/portal/portal.tsx(1 hunks)vite/src/components/providers/spaces-provider.tsx(3 hunks)vite/src/components/settings/sections/general/basic-settings-cards.tsx(1 hunks)vite/src/components/ui/popover.tsx(3 hunks)vite/src/components/ui/resizable-sidebar.tsx(3 hunks)vite/src/hooks/use-bounding-rect.tsx(1 hunks)vite/src/hooks/use-copy-styles.ts(1 hunks)vite/src/hooks/use-css-size-to-pixels.tsx(1 hunks)vite/src/hooks/use-sidebar-resize.tsx(4 hunks)vite/src/index.css(1 hunks)vite/src/lib/css.ts(1 hunks)vite/src/lib/flow/interfaces/app/app.ts(1 hunks)vite/src/lib/flow/interfaces/browser/interface.ts(2 hunks)vite/src/lib/utils.ts(1 hunks)vite/src/main.tsx(1 hunks)vite/src/routes/glance-modal/page.tsx(0 hunks)vite/src/routes/glance-modal/route.tsx(0 hunks)
💤 Files with no reviewable changes (5)
- vite/src/App.tsx
- vite/src/routes/glance-modal/route.tsx
- vite/src/routes/glance-modal/page.tsx
- electron/browser/tabs/tab.ts
- electron/browser/components/glance-modal.ts
🧰 Additional context used
🧬 Code Graph Analysis (13)
vite/src/components/browser-ui/sidebar/header/action-buttons.tsx (1)
vite/src/components/ui/resizable-sidebar.tsx (1)
SidebarGroup(722-722)
vite/src/lib/flow/interfaces/browser/interface.ts (1)
vite/src/lib/flow/types.ts (1)
PageBounds(1-6)
vite/src/components/browser-ui/sidebar/header/navigation-buttons.tsx (4)
vite/src/components/providers/spaces-provider.tsx (1)
useSpaces(19-25)vite/src/lib/utils.ts (1)
cn(5-7)vite/src/components/portal/popover.tsx (1)
PortalPopover(59-62)vite/src/components/ui/popover.tsx (1)
PopoverTrigger(44-44)
vite/src/components/settings/sections/general/basic-settings-cards.tsx (1)
vite/src/components/ui/card.tsx (1)
CardContent(56-56)
vite/src/components/browser-ui/browser-content.tsx (2)
vite/src/hooks/use-bounding-rect.tsx (1)
useBoundingRect(8-116)vite/src/lib/flow/types.ts (1)
PageBounds(1-6)
vite/src/components/ui/resizable-sidebar.tsx (1)
vite/src/lib/utils.ts (1)
cn(5-7)
electron/browser/components/portal-component-windows.ts (3)
electron/browser/window.ts (2)
TabbedBrowserWindow(28-249)destroy(206-237)electron/modules/output.ts (1)
debugPrint(19-25)electron/browser/view-manager.ts (1)
destroy(45-60)
vite/src/components/browser-ui/main.tsx (6)
vite/src/components/ui/resizable-sidebar.tsx (2)
useSidebar(742-742)SidebarInset(728-728)vite/src/components/ui/sidebar.tsx (2)
useSidebar(675-675)SidebarInset(661-661)vite/src/components/providers/settings-provider.tsx (1)
useSettings(13-19)vite/src/components/browser-ui/browser-sidebar.tsx (1)
BrowserSidebar(157-266)vite/src/lib/utils.ts (1)
cn(5-7)vite/src/components/browser-ui/sidebar/hover-detector.tsx (1)
SidebarHoverDetector(5-35)
vite/src/hooks/use-bounding-rect.tsx (1)
vite/src/components/portal/portal.tsx (1)
usePortalContext(26-29)
vite/src/components/browser-ui/sidebar/hover-detector.tsx (2)
vite/src/components/browser-ui/main.tsx (1)
SidebarSide(18-18)vite/src/lib/utils.ts (1)
cn(5-7)
vite/src/components/portal/popover.tsx (2)
vite/src/components/ui/popover.tsx (2)
Popover(44-44)PopoverContent(44-44)vite/src/components/portal/portal.tsx (2)
usePortalContext(26-29)PortalComponent(43-159)
vite/src/components/browser-ui/browser-action.tsx (3)
vite/src/components/portal/popover.tsx (1)
PortalPopover(59-62)vite/src/lib/utils.ts (1)
cn(5-7)vite/src/components/browser-ui/browser-sidebar.tsx (1)
SIDEBAR_HOVER_COLOR(32-33)
vite/src/hooks/use-css-size-to-pixels.tsx (1)
vite/src/lib/css.ts (1)
cssSizeToPixels(50-260)
🪛 LanguageTool
docs/components/portal.md
[style] ~5-~5: This phrase is redundant. Consider using “outside”.
Context: ...component allows content to be rendered outside of the main DOM hierarchy into a separate ...
(OUTSIDE_OF)
[uncategorized] ~91-~91: Loose punctuation mark.
Context: ...ns - initializePortalComponentWindows: Sets up IPC handlers and manages compon...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~105-~105: Loose punctuation mark.
Context: ... interface:set-component-window-bounds: Updates position and dimensions of comp...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~105-~105: You might be missing the article “the” here.
Context: ...e:set-component-window-bounds: Updates position and dimensions of component windows - ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~154-~154: Consider an alternative adverb to strengthen your wording.
Context: ...c CSS features - Window objects must be properly cleaned up to prevent resource leaks - ...
(PROPERLY_THOROUGHLY)
🔇 Additional comments (59)
vite/src/index.css (1)
2-2: Minor syntax update to align with project standards.The quote style for the tailwindcss-animate plugin was changed from single to double quotes, which aligns with common CSS quoting conventions and maintains consistency with the other @plugin directive on line 3.
electron/modules/basic-settings.ts (3)
42-42: Setting name now visible in UI.Making the sidebar collapse mode setting name visible in the UI improves clarity for users when accessing sidebar settings.
57-74: Good implementation of the new sidebar side setting.The new sidebar side setting is well-structured with clear options. Marking the "Right" option as experimental is a good practice to set appropriate user expectations for a feature that may still be evolving.
127-129: Well-organized settings card update.The settings card has been appropriately updated with a more general title and subtitle that encompasses both sidebar settings. The settings array properly includes both the new sidebarSide and existing sidebarCollapseMode options.
vite/src/components/browser-ui/sidebar/header/action-buttons.tsx (1)
158-158: Added horizontal padding improves UI spacing.The added
px-1class provides better horizontal spacing within the sidebar group, improving the visual appearance of the navigation controls.vite/src/components/settings/sections/general/basic-settings-cards.tsx (1)
49-49: Improved layout with flex layout classes 👍Adding flex layout classes to
CardContentcreates a more consistent vertical spacing between settings items, improving the visual organization of the settings cards.electron/modules/output.ts (1)
13-14: Added debug area for portal componentsThis addition is well-structured and follows the established pattern for debug areas. The new debug flag will be useful for troubleshooting the newly introduced portal component system.
electron/browser/utility/hot-reload.ts (1)
42-42: Increased minimum throttling timeout for better stabilityIncreasing the minimum timeout from 100ms to 300ms should help reduce resource contention during hot-reloading. This is a reasonable adjustment that balances development speed with system stability.
vite/src/lib/flow/interfaces/app/app.ts (1)
21-24: Added clipboard write functionalityThis is a clean extension to the FlowAppAPI interface with proper documentation. The method signature is simple and clear, following the pattern of other methods in this interface.
vite/src/components/browser-ui/sidebar/content/space-sidebar.tsx (1)
47-47: Good adjustment to prevent initial animation flashSetting
initial={false}on AnimatePresence prevents unwanted animations when the sidebar first loads, creating a smoother user experience for content that's already present on mount.electron/ipc/app/app.ts (2)
1-1: Correctly imported clipboard API from ElectronGood addition of the clipboard module import alongside the existing app module.
11-13: Well-implemented clipboard write handlerThe IPC handler for clipboard writing is clean and follows Electron's standard patterns. This is essential functionality for the portal component system to interact with the system clipboard across windows.
vite/src/components/browser-ui/sidebar/spaces-switcher.tsx (1)
22-22: Improved icon color contrast and consistencyThe updated color classes provide better contrast in both light and dark modes while maintaining visual hierarchy between active and inactive states.
vite/src/main.tsx (2)
1-1: Good import renaming for clarityRenaming StrictMode to ReactStrictMode improves code clarity, especially with the conditional usage pattern.
10-14: Clean implementation of conditional StrictModeThe conditional wrapping using either ReactStrictMode or Fragment is a clean approach that maintains the same component structure regardless of the StrictMode setting.
vite/src/components/providers/spaces-provider.tsx (1)
149-149: Good implementation of portal for stylesheet.Using
createPortalto inject styles into document.head is the correct approach for global styles.vite/src/components/browser-ui/sidebar/header/window-controls.tsx (3)
1-7: LGTM - Component setup with hook integration.The component properly uses the
useBoundingRecthook to manage window control positioning.
8-15: LGTM - Window button positioning implementation.The effect correctly updates window button position whenever the titlebar bounds change.
26-27: LGTM - Simple div element for positioning reference.The component renders a minimal div with appropriate sizing and positioning.
vite/src/components/ui/popover.tsx (3)
5-5: LGTM - Import Fragment for conditionally disabling portal.Good addition of the Fragment import to support the new portal conditional rendering.
19-22: LGTM - Portal rendering flexibility with backward compatibility.The addition of the optional
portalprop with a default value oftruemaintains backward compatibility while allowing for more flexible rendering strategies.
25-36: Good implementation of conditional portal wrapping.The component now properly wraps its content in either a Portal or Fragment based on the prop value.
vite/src/components/browser-ui/browser-content.tsx (4)
1-1: Updated imports to use the new bounding rect hook.Good removal of unnecessary imports and addition of the
useBoundingRecthook import.Also applies to: 5-5
12-12: Good simplification with useBoundingRect hook.Using the
useBoundingRecthook reduces code complexity by abstracting the dimension tracking logic.
14-24: Well-refactored effect for updating page bounds.The effect is correctly simplified to use the
rectvalue from the hook and only runs when needed. The code is now cleaner while maintaining the same functionality.
35-38: Properly updated debug display to use rect from hook.The debug display code is correctly updated to use the new
rectobject. Good conditional check for both the debug flag and the rect existence.vite/src/components/browser-ui/browser-action.tsx (2)
6-6: Updated imports for portal-based popover components.Good update to the imports, keeping only the
PopoverTriggerfrom UI popover while adding the newPortalPopoverfrom the portal components.Also applies to: 12-12
165-165: Replaced popover implementation with portal-based components.The switch to portal-based popover components is a good improvement. This will help prevent CSS inheritance issues and improve stacking context management by rendering content in a separate DOM node.
Also applies to: 171-171, 201-202
vite/src/components/browser-ui/sidebar/hover-detector.tsx (3)
1-4: Good imports for the new SidebarHoverDetector component.The imports are clean and appropriate for the component's functionality.
5-11: Well-structured component with clean props interface.The component definition is well structured with clear props typing and good use of
useCallbackfor event handler optimization.
12-24: Properly implemented event listener with cleanup.The event listener is correctly set up with proper cleanup in the effect's return function, which prevents memory leaks.
vite/src/hooks/use-sidebar-resize.tsx (3)
12-12: Good addition of right-side support parameter.The new
isRightSideproperty enables the sidebar to be positioned and resized from either the left or right side of the window, which aligns with the sidebar positioning feature mentioned in the PR objectives.
43-43: Clear default value implementation.Setting
isRightSidetofalseby default ensures backward compatibility with existing code that doesn't specify this parameter.
155-165: Properly updated dependencies array.The dependencies array correctly includes the new
isRightSideparameter to ensure the effect is rerun when this property changes.electron/browser/window.ts (2)
12-12: Good import of portal component windows initialization.This import brings in the new portal system initialization, replacing the previous glance modal approach.
77-78: Proactive window button visibility handling.Hiding the window buttons before component mount prevents UI flickering where window buttons might briefly appear before being controlled by the UI components.
vite/src/components/browser-ui/sidebar/header/navigation-buttons.tsx (3)
7-10: Good migration to portal-based popover system.The new imports support the transition from dropdown menus to the portal-based popover system, enabling floating UI elements that can render outside the DOM hierarchy.
40-40: Theme awareness implementation.Adding space theme awareness ensures visual consistency between the main UI and popovers.
69-70: Well-implemented theme class injection.Using the
cnutility to conditionally apply the dark theme class based on the current space theme is a clean approach.vite/src/lib/flow/interfaces/browser/interface.ts (1)
1-1: Good type import organization.Importing
PageBoundsfrom flow types promotes code reuse and type consistency across the application.vite/src/components/ui/resizable-sidebar.tsx (3)
228-230: Improved sidebar visibility for all screen sizes.The sidebar is now visible by default on all screen sizes by replacing the
"hidden md:block"classes with a dynamic class name using thecnutility. This change aligns with the portal component system by making the sidebar more consistent across different display contexts.
302-304: Added right-side support for sidebar resizing.The
isRightSideprop with a default value offalseenables the sidebar to be positioned and resized from either the left or right side of the screen. This is a valuable enhancement for the portal components system.
315-316: Properly passing new resize parameters to the hook.The implementation correctly passes both the
setIsDraggingRailandisRightSideparameters to theuseSidebarResizehook, enabling side-specific dragging behavior.vite/src/components/portal/popover.tsx (5)
1-6: Good choice of dependencies for the portal popover system.The imports are well-organized and include all necessary dependencies for creating a portal-based popover: positioning context, UI components, React hooks, and animation libraries.
7-12: Well-defined context type for popover state management.The
PopoverContextTypeinterface clearly defines the contract for the popover state management with proper TypeScript typing.
14-31: Clean implementation of controlled/uncontrolled component pattern.The
PortalPopoverRootcomponent correctly implements both controlled and uncontrolled usage patterns by:
- Using internal state when props aren't provided
- Respecting external state when provided
- Properly passing state to the context and underlying component
This follows React best practices for flexible component API design.
59-62: Clean object export pattern.Using the object pattern for
PortalPopoverexport provides a clean, organized API that groups related components together while maintaining their separation of concerns.
64-71: Helpful error for hook usage.The
usePopoverhook includes proper context validation with a descriptive error message when used outside of aPortalPopover.Rootcomponent, which will help developers use the API correctly.electron/browser/view-manager.ts (4)
1-5: Enhanced type safety with WebContentsView.Refining the type from generic
Viewto specificWebContentsViewimproves type safety and makes the API more precise for the portal component system's needs.
12-16: Automatic cleanup on view destruction prevents memory leaks.Adding an event listener to the
webContents.destroyedevent that automatically triggers view removal is an excellent enhancement to prevent memory leaks and simplify client code.
20-26: Flexible view removal with parent handling option.The enhanced
removeViewmethod with the optionaldontRemoveFromParentparameter allows for more flexible cleanup scenarios, particularly when a view is already being removed elsewhere (like during the destruction event).
41-43: Type consistency in getViewZIndex.Updating
getViewZIndexto acceptWebContentsViewmaintains type consistency throughout the class API.vite/src/hooks/use-copy-styles.ts (3)
3-6: Well-documented custom hook.The JSDoc comment clearly explains the purpose of the hook, making it easy for other developers to understand and use.
7-17: Proper hook structure with dependency array.The hook follows React best practices by:
- Using
useEffectwith a properly defined dependency array- Including an early return when the container window is null
- Defining local variables within the effect scope
22-105: Comprehensive style synchronization logic.The style copying implementation:
- Handles both
<style>and<link>elements correctly- Matches existing styles to prevent duplicates
- Updates attributes when they differ
- Removes styles that don't exist in the main document
- Properly clones nodes with their attributes
This thorough approach ensures consistent styling between the main window and portal windows.
vite/src/components/portal/portal.tsx (2)
93-95:useCopyStylesshould be guarded against a null windowWhen the first render runs,
containerWinRef.currentisnull, so the hook
executes with an invalid argument. If the hook doesn’t handle that internally
it will throw.-// Use the hook for style copying -useCopyStyles(containerWinRef.current); +if (containerWinRef.current) { + useCopyStyles(containerWinRef.current); +}
120-131: Verify thatflow.interfaceis always availableThe calls to
flow.interface.*run on every prop change. If the component is
rendered in a non-Electron context (SSR, unit tests, storybook), these calls
will explode. Please add a runtime guard or mock.vite/src/lib/css.ts (1)
164-205: Missing fall-through for%when parent width/height isautoIf the parent’s computed width/height is
auto,parsePixelValuereturnsNaNand the function falls back to0. This is reasonable, but callers have no way of distinguishing unresolvable from zero. Consider bubbling upNaNor adding an optionaldefaultValueparameter so consumers can handle this case more explicitly.vite/src/components/browser-ui/browser-sidebar.tsx (1)
251-258: Potential prop-type mismatch:widthpassed as number
PortalComponentdocs (and typings inportal.tsx) indicatewidthexpects a CSS string ("300px"etc.). Here a numericwidthfromuseSidebar()is forwarded directly. IfPortalComponentstill enforces astring, this will break type-checking and runtime conversion insidecssSizeToPixels.Please verify:
- Is
widtha number (e.g.,300) or a CSS string?- Does
PortalComponentacceptnumber | string?Adjust as needed, e.g.:
- width={width} + width={`${width}px`}
vite/src/components/browser-ui/sidebar/header/window-controls.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
vite/src/components/browser-ui/browser-sidebar.tsx (2)
56-85:⚠️ Potential issueFix timer cleanup logic in useSidebarHover hook
The current implementation has a logical issue where it attempts to clear a timeout from within its own callback, which doesn't prevent memory leaks or state updates after unmount. The timeout should be cleared before setting a new one.
Apply this fix:
function useSidebarHover(setIsHoveringSidebar: (isHovering: boolean) => void) { const isHoveringSidebarRef = useRef(false); const timeoutIdRef = useRef<NodeJS.Timeout | null>(null); const handleMouseEnter = useCallback(() => { isHoveringSidebarRef.current = true; + if (timeoutIdRef.current) { + clearTimeout(timeoutIdRef.current); + } timeoutIdRef.current = setTimeout(() => { - if (timeoutIdRef.current) { - clearTimeout(timeoutIdRef.current); - } if (isHoveringSidebarRef.current) { setIsHoveringSidebar(true); } }, 100); }, [setIsHoveringSidebar]); const handleMouseLeave = useCallback(() => { isHoveringSidebarRef.current = false; + if (timeoutIdRef.current) { + clearTimeout(timeoutIdRef.current); + } timeoutIdRef.current = setTimeout(() => { - if (timeoutIdRef.current) { - clearTimeout(timeoutIdRef.current); - } if (!isHoveringSidebarRef.current) { setIsHoveringSidebar(false); } }, 100); }, [setIsHoveringSidebar]); return { handleMouseEnter, handleMouseLeave }; }
191-197:⚠️ Potential issueNumeric
xvalues should includepxfor motion springThe
sideOffsetis constructed as a string without the "px" unit. This can cause animation issues as Motion may not interpret bare numbers in strings correctly.Convert to a number (preferred) or append "px":
-const sideOffset = side === "left" ? `-${width}` : `${width}`; +const sideOffset: number = side === "left" ? -width : width; const animationProps = { initial: { x: sideOffset, originX: side === "left" ? 0 : 1 }, animate: { x: 0 }, exit: { x: sideOffset }, transition: { type: "spring", damping: 30, stiffness: 400 } };
🧹 Nitpick comments (4)
vite/src/components/providers/minimal-toast-provider.tsx (4)
27-81: Consider accessibility improvements for the toast container.The toast implementation works well, but is missing accessibility attributes that would make it more usable for screen readers.
<motion.div key={currentMessage} initial={{ opacity: 0, scale: 0.9 }} animate={{ opacity: 1, scale: 1 }} exit={{ opacity: 0, scale: 0.9 }} transition={{ duration: 0.3 }} - className="box-border border border-border bg-space-background-start rounded-lg w-full h-full flex items-center justify-center" + className="box-border border border-border bg-space-background-start rounded-lg w-full h-full flex items-center justify-center" + role="alert" + aria-live="polite" onClick={removeToast} >
48-55: Consider more flexible positioning for the toast.The current width/height values of "10%" and "6%" may lead to inconsistent sizing across different viewport sizes. A fixed size or more adaptable approach might improve consistency.
<PortalComponent visible={isVisible} x={sidebarSide === "left" ? "100vw" : "0vw"} y={0} - width={"10%"} - height={"6%"} + width={"auto"} + height={"auto"} anchorX={sidebarSide === "left" ? "right" : "left"} >
83-113: Add cleanup for timeout on component unmount.The
MinimalToastProvidershould clean up any pending timeouts when it unmounts to prevent potential memory leaks.export function MinimalToastProvider({ children, sidebarSide }: ToastProviderProps) { const [currentMessage, setCurrentMessage] = useState<string | null>(null); const timeoutIdRef = useRef<NodeJS.Timeout | null>(null); + useEffect(() => { + // Cleanup function to clear timeout on unmount + return () => { + if (timeoutIdRef.current) { + clearTimeout(timeoutIdRef.current); + } + }; + }, []); const removeToast = () => { // Remove timeout if (timeoutIdRef.current) { clearTimeout(timeoutIdRef.current); timeoutIdRef.current = null; } // Remove message setCurrentMessage(null); };
98-105: Consider adding support for multiple toast messages.The current implementation replaces any existing toast with a new one. For a more robust notification system, you might want to consider a queue-based approach that can display multiple notifications.
This is an optional enhancement for future consideration. The current implementation makes sense for a minimal toast provider, but if you need to support multiple notifications in the future, you would need to modify the state to store an array of messages rather than a single message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/references/view-indexes.md(1 hunks)vite/src/components/browser-ui/browser-sidebar.tsx(2 hunks)vite/src/components/browser-ui/main.tsx(4 hunks)vite/src/components/browser-ui/sidebar/header/address-bar/copy-link-button.tsx(3 hunks)vite/src/components/browser-ui/sidebar/header/window-controls.tsx(1 hunks)vite/src/components/omnibox/main.tsx(1 hunks)vite/src/components/providers/minimal-toast-provider.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- vite/src/components/omnibox/main.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/references/view-indexes.md
- vite/src/components/browser-ui/sidebar/header/window-controls.tsx
- vite/src/components/browser-ui/main.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
vite/src/components/browser-ui/sidebar/header/address-bar/copy-link-button.tsx (1)
vite/src/components/providers/minimal-toast-provider.tsx (1)
useToast(14-20)
🔇 Additional comments (10)
vite/src/components/browser-ui/sidebar/header/address-bar/copy-link-button.tsx (3)
3-3: Good addition of the useToast hook import.This import properly brings in the toast functionality from the new minimal toast provider, allowing for user feedback when copy actions occur.
56-57: Good destructuring pattern for the toast hook.The destructuring syntax for
useToastis clean and follows React best practices.
69-69: Great user feedback enhancement with toast notification.Adding a toast notification after successful clipboard operations improves user experience by providing clear feedback. This complements the visual indicator (check icon) that's already implemented.
vite/src/components/providers/minimal-toast-provider.tsx (2)
1-7: Clean import structure with good organization.The imports are well-organized, bringing in the necessary dependencies for the portal component, React functionality, animations, and utilities.
8-20: Well-implemented context and hook pattern.The toast context and hook follow React best practices with proper error handling when used outside a provider. This enforces correct usage throughout the application.
vite/src/components/browser-ui/browser-sidebar.tsx (5)
20-23: Well-structured import organizationGood job on organizing imports by functionality - adding the portal components, animation libraries, and window controls necessary for the new floating sidebar feature.
35-53: Good extraction of animation logic into custom hookThe
useSidebarAnimationhook effectively manages mounting/unmounting logic for animated components, improving separation of concerns and reusability.
87-123: Good component extraction for improved modularityBreaking down the sidebar into
SidebarFooterContentandSidebarHeaderContentcomponents enhances maintainability and readability by separating concerns.
177-188: Improved event listener handlingGood implementation of the sidebar toggle event listener with proper cleanup in the useEffect return function, ensuring no memory leaks.
258-270: Well-implemented portal component integrationThe conditional rendering using
PortalComponentfor the floating variant is a clean approach that properly handles positioning based on the sidebar side.
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores