Conversation
Signed-off-by: prxt6529 <prxt@6529.io>
Signed-off-by: prxt6529 <prxt@6529.io>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the React-Bootstrap HeaderShare modal with a custom Tailwind-based overlay/dialog (enter/exit lifecycle, Escape-key, scroll-lock), removes the SCSS module, refactors tab/subtab rendering and QR/core link helpers, updates URL copy/tooltip logic, and updates tests to expect "Connection" text. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HeaderShare as HeaderShare(Component)
participant QRGen as QRGenerator
participant Clipboard
participant DOM
User->>HeaderShare: open "Connection" modal
HeaderShare->>DOM: apply scroll-lock, render overlay/dialog (shouldRender)
HeaderShare->>HeaderShare: requestAnimationFrame -> set isVisible (enter animation)
alt QR tab selected
HeaderShare->>QRGen: generate QR (SVG/bitmap)
QRGen-->>HeaderShare: QR payload
HeaderShare->>DOM: render QR image
else Core/Apps tab selected
HeaderShare->>HeaderShare: compute links
HeaderShare->>HeaderShare: fetch app versions via Promise.allSettled
HeaderShare->>DOM: render core link + apps list
end
User->>HeaderShare: Click copy URL
HeaderShare->>Clipboard: writeText(url)
Clipboard-->>HeaderShare: success/failure
HeaderShare->>DOM: show tooltip / copied state
User->>HeaderShare: Press Escape or click overlay
HeaderShare->>HeaderShare: start exit timeout -> unset isVisible
HeaderShare->>DOM: remove overlay, restore scroll
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
components/header/share/HeaderShare.tsx (5)
633-646: Sequential fetches could be parallelized for faster loading.The
for...ofloop fetches each OS's YAML file sequentially. UsingPromise.allSettledwould fetch all three in parallel, reducing total wait time.♻️ Parallelize version fetches
const loadVersions = async () => { - const versions: OSInfo[] = []; - for (const osConfig of osConfigs.filter((config) => config.enabled)) { - try { - const ymlData = await fetchYml(osConfig.url); - versions.push({ ...osConfig, version: ymlData.version }); - } catch (error) { - console.error( - `Failed to fetch or process ${osConfig.displayName}:`, - error - ); - } - } - setVersions(versions); + const enabledConfigs = osConfigs.filter((config) => config.enabled); + const results = await Promise.allSettled( + enabledConfigs.map(async (osConfig) => { + const ymlData = await fetchYml(osConfig.url); + return { ...osConfig, version: ymlData.version }; + }) + ); + const versions = results + .filter((r): r is PromiseFulfilledResult<OSInfo> => r.status === "fulfilled") + .map((r) => r.value); + setVersions(versions); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/share/HeaderShare.tsx` around lines 633 - 646, The loadVersions function currently fetches each OS YAML sequentially using a for...of loop which slows loading; change it to run fetchYml for all enabled osConfigs in parallel (use Promise.allSettled on an array mapped from osConfigs.filter(c => c.enabled).map(c => fetchYml(c.url))). After settling, iterate results to build the versions array (include only successful responses and keep the original osConfig metadata) and log failures, then call setVersions(versions). Ensure you still merge version into each osConfig when pushing to versions.
432-442: Consider addingaria-labelledbyfor improved screen reader support.The dialog has
role="dialog"andaria-modal="true", which is good. Addingaria-labelledbypointing to a heading element would improve accessibility by announcing the dialog's purpose to screen reader users.♿ Optional: Add dialog labeling
<div role="dialog" aria-modal="true" + aria-labelledby="share-modal-title" data-testid="header-share-modal" ... > <div className="tw-flex tw-flex-col tw-gap-2 tw-p-3"> + <h2 id="share-modal-title" className="tw-sr-only">Share Options</h2> <ModalMenu🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/share/HeaderShare.tsx` around lines 432 - 442, Add an aria-labelledby attribute to the dialog container (the div with role="dialog" in HeaderShare.tsx) and ensure a corresponding heading element inside the modal uses that id so screen readers announce the dialog title; update the div's attributes to include aria-labelledby="header-share-title" (or another unique id) and add/modify the modal heading element (e.g., an h2/h3 used as the modal title) to have id="header-share-title" and meaningful text.
473-480: Consider simplifying nested ternary for readability.The
subTabCountcalculation with nested ternaries is hard to parse at a glance.♻️ Extract to helper or use if-else for clarity
- const subTabCount = - activeTab === Mode.NAVIGATE ? (!isElectron ? 3 : 2) : !isElectron ? 2 : 1; + const getSubTabCount = () => { + if (activeTab === Mode.NAVIGATE) { + return isElectron ? 2 : 3; + } + return isElectron ? 1 : 2; + }; + const subTabCount = getSubTabCount();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/share/HeaderShare.tsx` around lines 473 - 480, The nested ternary assigning subTabCount is hard to read; replace it with an explicit conditional (e.g., a small helper or if/else or switch) that computes subTabCount based on activeTab and isElectron to improve clarity. Locate the expression assigning subTabCount (using activeTab, Mode, and isElectron) and refactor into a named function or a few clear if/else branches that return 3/2/1 as appropriate so the logic is easier to follow and maintain.
539-563: Sub-tab buttons lackdisabledattribute when active.Unlike the top-level tabs (lines 503, 512, 520), the sub-tab buttons don't disable themselves when already active. This allows unnecessary state updates when users click the currently active sub-tab.
♻️ Add disabled state to sub-tab buttons
<button type="button" + disabled={activeSubTab === SubMode.APP} className={getMenuButtonClass(activeSubTab === SubMode.APP)} onClick={() => onTabChange(activeTab, SubMode.APP)} >Apply similar changes to the Browser and 6529 Desktop buttons.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/share/HeaderShare.tsx` around lines 539 - 563, Sub-tab buttons (those using getMenuButtonClass and controlled by activeSubTab/SubMode) should mirror top-level tabs by setting the disabled attribute when already active to avoid redundant state updates; update the APP button to include disabled={activeSubTab === SubMode.APP} and likewise add disabled={activeSubTab === SubMode.BROWSER} to the Browser button (inside the activeTab === Mode.NAVIGATE block) and disabled={activeSubTab === SubMode.CORE} to the 6529 Desktop button (the !isElectron block), keeping the existing onClick handlers and class logic unchanged.
225-244: Consider stabilizingonClosereference to avoid effect re-runs.The
onCloseprop is in the dependency array, but it's defined inline in the parent (onClose={() => setShowQRModal(false)}), creating a new function reference on each render. This causes unnecessary effect re-runs, repeatedly adding/removing the event listener.♻️ Wrap onKeyDown with useCallback to avoid re-subscribing
+import { useEffect, useState, useCallback } from "react"; -import { useEffect, useState } from "react";Then stabilize the handler:
useEffect(() => { if (!shouldRender) { return; } const previousOverflow = document.body.style.overflow; - const onKeyDown = (event: KeyboardEvent) => { - if (event.key === "Escape") { - onClose(); - } - }; document.body.style.overflow = "hidden"; - window.addEventListener("keydown", onKeyDown); + window.addEventListener("keydown", handleKeyDown); return () => { document.body.style.overflow = previousOverflow; - window.removeEventListener("keydown", onKeyDown); + window.removeEventListener("keydown", handleKeyDown); }; - }, [shouldRender, onClose]); + }, [shouldRender, handleKeyDown]);Add above the effect:
const handleKeyDown = useCallback((event: KeyboardEvent) => { if (event.key === "Escape") { onClose(); } }, [onClose]);Alternatively, the parent components (
HeaderShareandWebSidebarUser) can wrap theironClosehandlers withuseCallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/share/HeaderShare.tsx` around lines 225 - 244, The effect re-subscribes because onClose is recreated by the parent; to fix, stabilize the handler used in the effect: inside HeaderShare create a stable handleKeyDown (e.g., useCallback handleKeyDown that checks event.key === "Escape" and calls onClose) and use handleKeyDown in the useEffect dependency array instead of onClose, or alternatively memoize the parent's onClose (in components like WebSidebarUser) with useCallback so its reference doesn't change; update the effect to add/remove window listener for handleKeyDown and keep document.body.style.overflow logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@components/header/share/HeaderShare.tsx`:
- Around line 633-646: The loadVersions function currently fetches each OS YAML
sequentially using a for...of loop which slows loading; change it to run
fetchYml for all enabled osConfigs in parallel (use Promise.allSettled on an
array mapped from osConfigs.filter(c => c.enabled).map(c => fetchYml(c.url))).
After settling, iterate results to build the versions array (include only
successful responses and keep the original osConfig metadata) and log failures,
then call setVersions(versions). Ensure you still merge version into each
osConfig when pushing to versions.
- Around line 432-442: Add an aria-labelledby attribute to the dialog container
(the div with role="dialog" in HeaderShare.tsx) and ensure a corresponding
heading element inside the modal uses that id so screen readers announce the
dialog title; update the div's attributes to include
aria-labelledby="header-share-title" (or another unique id) and add/modify the
modal heading element (e.g., an h2/h3 used as the modal title) to have
id="header-share-title" and meaningful text.
- Around line 473-480: The nested ternary assigning subTabCount is hard to read;
replace it with an explicit conditional (e.g., a small helper or if/else or
switch) that computes subTabCount based on activeTab and isElectron to improve
clarity. Locate the expression assigning subTabCount (using activeTab, Mode, and
isElectron) and refactor into a named function or a few clear if/else branches
that return 3/2/1 as appropriate so the logic is easier to follow and maintain.
- Around line 539-563: Sub-tab buttons (those using getMenuButtonClass and
controlled by activeSubTab/SubMode) should mirror top-level tabs by setting the
disabled attribute when already active to avoid redundant state updates; update
the APP button to include disabled={activeSubTab === SubMode.APP} and likewise
add disabled={activeSubTab === SubMode.BROWSER} to the Browser button (inside
the activeTab === Mode.NAVIGATE block) and disabled={activeSubTab ===
SubMode.CORE} to the 6529 Desktop button (the !isElectron block), keeping the
existing onClick handlers and class logic unchanged.
- Around line 225-244: The effect re-subscribes because onClose is recreated by
the parent; to fix, stabilize the handler used in the effect: inside HeaderShare
create a stable handleKeyDown (e.g., useCallback handleKeyDown that checks
event.key === "Escape" and calls onClose) and use handleKeyDown in the useEffect
dependency array instead of onClose, or alternatively memoize the parent's
onClose (in components like WebSidebarUser) with useCallback so its reference
doesn't change; update the effect to add/remove window listener for
handleKeyDown and keep document.body.style.overflow logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a0ebe1c-728d-4464-bb7d-82b9b9b1c04b
📒 Files selected for processing (3)
__tests__/components/header/share/HeaderShare.test.tsxcomponents/header/share/HeaderShare.module.scsscomponents/header/share/HeaderShare.tsx
💤 Files with no reviewable changes (1)
- components/header/share/HeaderShare.module.scss
There was a problem hiding this comment.
🧹 Nitpick comments (5)
components/header/share/HeaderShare.tsx (5)
212-216: Missing dependencies in useEffect may cause stale QR codes.The effect calls
generateSourceswhich usespathnameandsearchParams, but these aren't in the dependency array. If the URL changes while the modal is open (e.g., via programmatic navigation), the QR codes will show stale data.If this is intentional (only generate on open), consider adding a comment. Otherwise, add the missing dependencies:
useEffect(() => { if (show) { generateSources(getRefreshToken(), getWalletAddress(), getWalletRole()); } - }, [show]); + }, [show, pathname, searchParams]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/share/HeaderShare.tsx` around lines 212 - 216, The useEffect that calls generateSources when the modal opens only depends on show, but generateSources reads pathname and searchParams so URL changes while the modal is open can produce stale QR codes; update the dependency array of the useEffect to include pathname and searchParams (or explicitly document intent with a comment if you truly only want to run on open) so that generateSources(getRefreshToken(), getWalletAddress(), getWalletRole()) re-runs when the URL changes while the modal is visible.
615-640: MoveosConfigsoutside component and fix variable shadowing.
osConfigsis a static configuration array recreated on every render. Move it outside the component.The local variable
versions(line 657) shadows the state variableversions(line 642), which can cause confusion.♻️ Suggested refactor
+ const OS_CONFIGS: OSInfo[] = [ + { + name: "windows", + url: "https://6529bucket.s3.eu-west-1.amazonaws.com/6529-core-app/win/latest.yml", + displayName: "Windows", + downloadPath: "6529-core-app/win/links", + image: "/windows.png", + enabled: true, + }, + // ... other configs + ]; function CoreAppsDownload() { // ... interfaces remain inside if they're only used here - const osConfigs: OSInfo[] = [...]; const [versions, setVersions] = useState<OSInfo[]>([]); useEffect(() => { // ... const loadVersions = async () => { - const versions: OSInfo[] = []; - for (const osConfig of osConfigs.filter((config) => config.enabled)) { + const loadedVersions: OSInfo[] = []; + for (const osConfig of OS_CONFIGS.filter((config) => config.enabled)) { // ... - versions.push({ ... }); + loadedVersions.push({ ... }); } - setVersions(versions); + setVersions(loadedVersions); };Also applies to: 656-669
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/share/HeaderShare.tsx` around lines 615 - 640, Move the static osConfigs array out of the HeaderShare component to module scope (declare it as a top-level const so it's created once and imported/used by the component) and update any references inside the component to use that top-level osConfigs; also resolve the variable shadowing by renaming the local variable named versions (used around the fetch/parse logic) to something like fetchedVersions or localVersions and update all uses in functions like the fetch/parse handlers so they don't shadow the state variable versions declared with useState.
242-261: Potential scroll-lock race condition with concurrent modals.The
previousOverflowcapture-and-restore pattern can fail when multiple modals are open. If HeaderQRModal opens while another modal (e.g.,WaveLeaderboardCurationDropModal) is already open,previousOverflowcaptures"hidden". When HeaderQRModal closes first, it restores"hidden", which is correct. But if the other modal closes first, it restores its captured value (also"hidden"or empty), potentially leaving scroll locked or unlocked prematurely.Additionally,
onClosein the dependency array will cause the effect to re-run if the parent doesn't memoize the callback, leading to listener churn.Consider:
- Using a shared scroll-lock utility with reference counting
- Wrapping
onClosein a ref to avoid effect re-runs♻️ Suggested fix using a ref for onClose
+ const onCloseRef = useRef(onClose); + useEffect(() => { + onCloseRef.current = onClose; + }, [onClose]); useEffect(() => { if (!shouldRender) { return; } const previousOverflow = document.body.style.overflow; const onKeyDown = (event: KeyboardEvent) => { if (event.key === "Escape") { - onClose(); + onCloseRef.current(); } }; document.body.style.overflow = "hidden"; globalThis.addEventListener("keydown", onKeyDown); return () => { document.body.style.overflow = previousOverflow; globalThis.removeEventListener("keydown", onKeyDown); }; - }, [shouldRender, onClose]); + }, [shouldRender]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/share/HeaderShare.tsx` around lines 242 - 261, The effect in HeaderShare.tsx captures document.body.style.overflow into previousOverflow which races when multiple modals are open and also re-runs when onClose changes; replace the direct capture/restore with a shared scroll-lock counter utility (e.g., export lockScroll() to increment and set overflow="hidden" and unlockScroll() to decrement and restore only when counter reaches 0) and call lockScroll() when shouldRender becomes true and unlockScroll() in the cleanup; also avoid effect churn by storing the onClose callback in a ref (e.g., onCloseRef.current) and use that ref inside the onKeyDown handler so the effect's dependency list can be reduced to [shouldRender] while still calling the latest onClose.
459-466: Consider adding focus trap for accessibility.The
<dialog open>element with theopenattribute bypasses the nativeshowModal()API, which provides automatic focus trapping. Currently, keyboard focus can escape the modal to elements behind it, which is an accessibility concern for keyboard-only users.Consider using a focus-trap library or implementing focus cycling similar to
WaveLeaderboardCurationDropModal(see context snippet 1, which handles Tab key navigation).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/share/HeaderShare.tsx` around lines 459 - 466, The modal in HeaderShare.tsx currently renders a <dialog open> (in the JSX around the dialog with className using isVisible) but lacks focus trapping; add a focus trap so keyboard focus cannot escape while the share modal is open by either integrating a focus-trap library (e.g., focus-trap or react-focus-lock) around the dialog or implementing the Tab-cycling logic used in WaveLeaderboardCurationDropModal: on open, set focus to the first focusable element (use a ref to the modal root), register a keydown listener that intercepts Tab/Shift+Tab to move focus between the first and last focusable elements, and remove listeners on close; ensure listeners are attached/detached in the same component (useEffect) and reference the dialog root ref and isVisible state so focus trapping is only active while the modal is visible.
562-586: Sub-tab buttons missingdisabledattribute for consistency.The top-level tab buttons (lines 526, 535, 543) include
disabled={activeTab === Mode.X}when active, but the sub-tab buttons don't have correspondingdisabledattributes. This creates inconsistent behavior and accessibility semantics.♻️ Suggested fix
<button type="button" + disabled={activeSubTab === SubMode.APP} className={getMenuButtonClass(activeSubTab === SubMode.APP)} onClick={() => onTabChange(activeTab, SubMode.APP)} > <span>6529 Mobile</span> </button> {activeTab === Mode.NAVIGATE && ( <button type="button" + disabled={activeSubTab === SubMode.BROWSER} className={getMenuButtonClass(activeSubTab === SubMode.BROWSER)} onClick={() => onTabChange(activeTab, SubMode.BROWSER)} > <span>Browser</span> </button> )} {!isElectron && ( <button type="button" + disabled={activeSubTab === SubMode.CORE} className={getMenuButtonClass(activeSubTab === SubMode.CORE)} onClick={() => onTabChange(activeTab, SubMode.CORE)} > <span>6529 Desktop</span> </button> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/share/HeaderShare.tsx` around lines 562 - 586, The sub-tab buttons (the ones rendering SubMode.APP, SubMode.BROWSER, SubMode.CORE) are missing a disabled prop — add a disabled attribute to each sub-tab button that mirrors the same condition used on its corresponding top-level tab (i.e., use the same expression involving activeTab and the appropriate Mode constant that the top-level buttons use), keeping existing props like className={getMenuButtonClass(...)} and onClick={() => onTabChange(activeTab, SubMode.X)} unchanged; reference activeTab, Mode, SubMode, getMenuButtonClass, onTabChange and isElectron to locate and update those three button elements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@components/header/share/HeaderShare.tsx`:
- Around line 212-216: The useEffect that calls generateSources when the modal
opens only depends on show, but generateSources reads pathname and searchParams
so URL changes while the modal is open can produce stale QR codes; update the
dependency array of the useEffect to include pathname and searchParams (or
explicitly document intent with a comment if you truly only want to run on open)
so that generateSources(getRefreshToken(), getWalletAddress(), getWalletRole())
re-runs when the URL changes while the modal is visible.
- Around line 615-640: Move the static osConfigs array out of the HeaderShare
component to module scope (declare it as a top-level const so it's created once
and imported/used by the component) and update any references inside the
component to use that top-level osConfigs; also resolve the variable shadowing
by renaming the local variable named versions (used around the fetch/parse
logic) to something like fetchedVersions or localVersions and update all uses in
functions like the fetch/parse handlers so they don't shadow the state variable
versions declared with useState.
- Around line 242-261: The effect in HeaderShare.tsx captures
document.body.style.overflow into previousOverflow which races when multiple
modals are open and also re-runs when onClose changes; replace the direct
capture/restore with a shared scroll-lock counter utility (e.g., export
lockScroll() to increment and set overflow="hidden" and unlockScroll() to
decrement and restore only when counter reaches 0) and call lockScroll() when
shouldRender becomes true and unlockScroll() in the cleanup; also avoid effect
churn by storing the onClose callback in a ref (e.g., onCloseRef.current) and
use that ref inside the onKeyDown handler so the effect's dependency list can be
reduced to [shouldRender] while still calling the latest onClose.
- Around line 459-466: The modal in HeaderShare.tsx currently renders a <dialog
open> (in the JSX around the dialog with className using isVisible) but lacks
focus trapping; add a focus trap so keyboard focus cannot escape while the share
modal is open by either integrating a focus-trap library (e.g., focus-trap or
react-focus-lock) around the dialog or implementing the Tab-cycling logic used
in WaveLeaderboardCurationDropModal: on open, set focus to the first focusable
element (use a ref to the modal root), register a keydown listener that
intercepts Tab/Shift+Tab to move focus between the first and last focusable
elements, and remove listeners on close; ensure listeners are attached/detached
in the same component (useEffect) and reference the dialog root ref and
isVisible state so focus trapping is only active while the modal is visible.
- Around line 562-586: The sub-tab buttons (the ones rendering SubMode.APP,
SubMode.BROWSER, SubMode.CORE) are missing a disabled prop — add a disabled
attribute to each sub-tab button that mirrors the same condition used on its
corresponding top-level tab (i.e., use the same expression involving activeTab
and the appropriate Mode constant that the top-level buttons use), keeping
existing props like className={getMenuButtonClass(...)} and onClick={() =>
onTabChange(activeTab, SubMode.X)} unchanged; reference activeTab, Mode,
SubMode, getMenuButtonClass, onTabChange and isElectron to locate and update
those three button elements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4318433-a938-45d8-97a3-d3731aff6b72
📒 Files selected for processing (1)
components/header/share/HeaderShare.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/header/share/HeaderShare.tsx (1)
229-239:⚠️ Potential issue | 🟡 MinorMissing
isAuthenticatedin dependency array.The effect uses
isAuthenticatedto setactiveTabbut it's not listed as a dependency. If authentication state changes while the modal is closed, reopening could show stale tab selection.🔧 Proposed fix
useEffect(() => { setActiveTab(isAuthenticated ? Mode.SHARE : Mode.NAVIGATE); setActiveSubTab(SubMode.APP); if (show) return; const timer = setTimeout(() => { setNavigateBrowserSrc(""); setNavigateAppSrc(""); setShareConnectionSrc(""); }, 150); return () => clearTimeout(timer); - }, [show]); + }, [show, isAuthenticated]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/share/HeaderShare.tsx` around lines 229 - 239, The effect in HeaderShare.tsx reads isAuthenticated to decide the initial tab (calling setActiveTab(isAuthenticated ? Mode.SHARE : Mode.NAVIGATE)) but only lists show in its dependency array, which can leave stale state; update the useEffect dependency list to include isAuthenticated so the effect reruns when authentication changes, keeping the activeTab in sync with isAuthenticated while preserving the existing behavior that resets subtab and clears sources when show is false (references: useEffect, setActiveTab, Mode.SHARE, Mode.NAVIGATE, setActiveSubTab, SubMode.APP, show, isAuthenticated).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/header/share/HeaderShare.tsx`:
- Around line 153-157: The Escape key handler handleEscapeKeyDown in
HeaderShare.tsx should stop the event bubbling so other global Escape listeners
don't also run; update handleEscapeKeyDown to call event.stopPropagation() (and
optionally event.preventDefault()) before invoking onCloseRef.current() so the
modal/action closes and the key event doesn't propagate to TermsOfServiceModal,
WaveDropActionsAddReaction, or other global listeners.
---
Outside diff comments:
In `@components/header/share/HeaderShare.tsx`:
- Around line 229-239: The effect in HeaderShare.tsx reads isAuthenticated to
decide the initial tab (calling setActiveTab(isAuthenticated ? Mode.SHARE :
Mode.NAVIGATE)) but only lists show in its dependency array, which can leave
stale state; update the useEffect dependency list to include isAuthenticated so
the effect reruns when authentication changes, keeping the activeTab in sync
with isAuthenticated while preserving the existing behavior that resets subtab
and clears sources when show is false (references: useEffect, setActiveTab,
Mode.SHARE, Mode.NAVIGATE, setActiveSubTab, SubMode.APP, show, isAuthenticated).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e67e61c-70cd-4841-ab48-fb46b5ddf61b
📒 Files selected for processing (1)
components/header/share/HeaderShare.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/header/share/HeaderShare.tsx (1)
204-222:⚠️ Potential issue | 🟡 MinorAdd error handling for QR code generation.
The
QRCode.toDataURLpromises lack.catch()handlers. If generation fails (e.g., invalid input), the unhandled rejection could cause issues.🛡️ Proposed fix
QRCode.toDataURL(browserUrl, { width: 500, margin: 0 }).then( (dataUrl: string) => { setNavigateBrowserSrc(dataUrl); } - ); + ).catch((error: unknown) => { + console.error("Failed to generate browser QR code", error); + }); QRCode.toDataURL(appUrl, { width: 500, margin: 0 }).then( (dataUrl: string) => { setNavigateAppSrc(dataUrl); } - ); + ).catch((error: unknown) => { + console.error("Failed to generate app QR code", error); + }); if (shareConnectionAppUrl) { QRCode.toDataURL(shareConnectionAppUrl, { width: 500, margin: 0 }).then( (dataUrl: string) => { setShareConnectionSrc(dataUrl); } - ); + ).catch((error: unknown) => { + console.error("Failed to generate share connection QR code", error); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/share/HeaderShare.tsx` around lines 204 - 222, The QRCode.toDataURL calls lack error handling and can produce unhandled promise rejections; wrap each QRCode.toDataURL invocation in proper error handling (either add .catch(...) to each promise or convert to async/await with try/catch) and log or handle errors before setting state; specifically update the calls that setNavigateBrowserSrc, setNavigateAppSrc, and setShareConnectionSrc (only when shareConnectionAppUrl is present) to catch failures and avoid calling the setters on error, and include a concise log or fallback behavior in the catch block.
🧹 Nitpick comments (3)
components/header/share/HeaderShare.tsx (3)
636-661: Consider moving static config outside the component.
osConfigsis static data recreated on every render. Moving it to module scope would be slightly more efficient and clearer about its immutable nature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/share/HeaderShare.tsx` around lines 636 - 661, The osConfigs array is defined inside the HeaderShare component and is recreated on every render; move this immutable static configuration to module scope by declaring osConfigs (the OSInfo[] constant) outside the HeaderShare component (top-level in HeaderShare.tsx) so the component uses the shared constant rather than recreating it each render; update any references inside HeaderShare to the relocated osConfigs and keep the shape and names (name, url, displayName, downloadPath, image, enabled) unchanged.
473-481: Consider improving dialog accessibility.Using
<dialog open>instead ofshowModal()bypasses native focus trapping and inert behavior. Consider addingaria-modal="true"and implementing focus management (trap focus within the dialog while open, restore focus to trigger element on close).♿ Proposed aria-modal addition
<dialog open + aria-modal="true" aria-labelledby="header-share-title" data-testid="header-share-modal"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/share/HeaderShare.tsx` around lines 473 - 481, The dialog element in HeaderShare.tsx is using <dialog open> which bypasses native focus trapping; update the modal markup and behavior in the HeaderShare component to add aria-modal="true" to the dialog, implement JS focus management (trap focus within the dialog while isVisible is true and cycle Tab/Shift+Tab), save and restore the previously focused trigger element when opening/closing, and ensure Escape closes the dialog and returns focus; locate the dialog JSX and the isVisible state (and any open/close handlers) to wire the focus-trap logic and cleanup on unmount/close.
255-269: Scroll-lock may conflict with other modals.The save/restore pattern for
document.body.style.overflowcan break when multiple modals are open simultaneously. Per the codebase,HeaderSearchModal,UserPageDropModal, andArtistPreviewModalall manipulate body overflow. If two modals overlap in their open states, the first to close will restore a stale value.Consider using a reference-counted scroll-lock utility or a shared context to coordinate body overflow across all modals.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/share/HeaderShare.tsx` around lines 255 - 269, The current useEffect in HeaderShare.tsx directly saves/restores document.body.style.overflow (when shouldRender is true) which can clobber other modals; replace that with a shared reference-counted scroll-lock utility (or context) and call its lock() when opening and unlock() on cleanup instead of saving previousOverflow. Update the effect that references shouldRender and handleEscapeKeyDown to call ScrollLock.lock() when shouldRender becomes true and ScrollLock.unlock() in the return cleanup, keep the keydown add/remove logic unchanged, and ensure the utility tracks a counter so only the final unlock restores the overflow style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/header/share/HeaderShare.tsx`:
- Around line 45-50: getSubTabCount currently requires a strict boolean but
you're passing the result of useElectron (boolean | undefined); fix by handling
the undefined case before calling getSubTabCount — either coerce the hook value
to a boolean (e.g., !!isElectron) where you call getSubTabCount, or update
getSubTabCount signature to accept boolean | undefined and treat undefined the
same as false inside the function (use a default false branch). Reference:
getSubTabCount and the useElectron call site (where the value is passed).
---
Outside diff comments:
In `@components/header/share/HeaderShare.tsx`:
- Around line 204-222: The QRCode.toDataURL calls lack error handling and can
produce unhandled promise rejections; wrap each QRCode.toDataURL invocation in
proper error handling (either add .catch(...) to each promise or convert to
async/await with try/catch) and log or handle errors before setting state;
specifically update the calls that setNavigateBrowserSrc, setNavigateAppSrc, and
setShareConnectionSrc (only when shareConnectionAppUrl is present) to catch
failures and avoid calling the setters on error, and include a concise log or
fallback behavior in the catch block.
---
Nitpick comments:
In `@components/header/share/HeaderShare.tsx`:
- Around line 636-661: The osConfigs array is defined inside the HeaderShare
component and is recreated on every render; move this immutable static
configuration to module scope by declaring osConfigs (the OSInfo[] constant)
outside the HeaderShare component (top-level in HeaderShare.tsx) so the
component uses the shared constant rather than recreating it each render; update
any references inside HeaderShare to the relocated osConfigs and keep the shape
and names (name, url, displayName, downloadPath, image, enabled) unchanged.
- Around line 473-481: The dialog element in HeaderShare.tsx is using <dialog
open> which bypasses native focus trapping; update the modal markup and behavior
in the HeaderShare component to add aria-modal="true" to the dialog, implement
JS focus management (trap focus within the dialog while isVisible is true and
cycle Tab/Shift+Tab), save and restore the previously focused trigger element
when opening/closing, and ensure Escape closes the dialog and returns focus;
locate the dialog JSX and the isVisible state (and any open/close handlers) to
wire the focus-trap logic and cleanup on unmount/close.
- Around line 255-269: The current useEffect in HeaderShare.tsx directly
saves/restores document.body.style.overflow (when shouldRender is true) which
can clobber other modals; replace that with a shared reference-counted
scroll-lock utility (or context) and call its lock() when opening and unlock()
on cleanup instead of saving previousOverflow. Update the effect that references
shouldRender and handleEscapeKeyDown to call ScrollLock.lock() when shouldRender
becomes true and ScrollLock.unlock() in the return cleanup, keep the keydown
add/remove logic unchanged, and ensure the utility tracks a counter so only the
final unlock restores the overflow style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67b346cd-cf5a-4953-96c9-667a22e9d4d4
📒 Files selected for processing (1)
components/header/share/HeaderShare.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/header/share/HeaderShare.tsx (2)
352-364:⚠️ Potential issue | 🟠 MajorEncode and clear share-connection URLs atomically.
These links are built from raw query-string concatenation, and the fallback path only clears the QR image. If
refreshTokencontains reserved characters, the deep link can become invalid; and if a later render no longer has a validwalletAddress, the previous session’sshareConnectionAppUrl/shareConnectionCoreUrlstays copyable.Suggested fix
if (refreshToken && walletAddress) { - shareConnectionAppUrl = `${appScheme}://${DeepLinkScope.SHARE_CONNECTION}?token=${refreshToken}&address=${walletAddress}`; - shareConnectionCoreUrl = `${coreScheme}://${DeepLinkScope.NAVIGATE}/accept-connection-sharing?token=${refreshToken}&address=${walletAddress}`; - - if (role) { - shareConnectionAppUrl += `&role=${role}`; - shareConnectionCoreUrl += `&role=${role}`; - } + const params = new URLSearchParams({ + token: refreshToken, + address: walletAddress, + }); + if (role) { + params.set("role", role); + } + + shareConnectionAppUrl = `${appScheme}://${DeepLinkScope.SHARE_CONNECTION}?${params.toString()}`; + shareConnectionCoreUrl = `${coreScheme}://${DeepLinkScope.NAVIGATE}/accept-connection-sharing?${params.toString()}`; + setShareConnectionAppUrl(shareConnectionAppUrl); setShareConnectionCoreUrl(shareConnectionCoreUrl); } else { + setShareConnectionAppUrl(""); + setShareConnectionCoreUrl(""); setShareConnectionSrc(""); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/share/HeaderShare.tsx` around lines 352 - 364, The code builds deep links by raw string concatenation which can break if refreshToken/walletAddress/role contain reserved characters and currently only clears the QR fallback; update the logic in the block that references refreshToken, walletAddress, role, DeepLinkScope.SHARE_CONNECTION and DeepLinkScope.NAVIGATE so you first compute local shareConnectionAppUrl and shareConnectionCoreUrl using encodeURIComponent for token, address, and role, then call setShareConnectionAppUrl and setShareConnectionCoreUrl together; in the else branch ensure you clear BOTH setShareConnectionAppUrl("") and setShareConnectionCoreUrl("") so stale URLs can’t be copied.
396-400:⚠️ Potential issue | 🟠 MajorRefresh the share sources when the route changes.
generateSources()depends onpathnameandsearchParams, but this effect only reruns onshow. If the user navigates while the modal stays open, the QR code and copied URL can point to the previous page.Suggested fix
+ const searchParamsString = searchParams?.toString() ?? ""; + useEffect(() => { if (show) { generateSources(getRefreshToken(), getWalletAddress(), getWalletRole()); } - }, [show]); + }, [show, pathname, searchParamsString]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/header/share/HeaderShare.tsx` around lines 396 - 400, The effect that calls generateSources (inside the useEffect block) only depends on show, but generateSources uses the current pathname and searchParams so the share data can become stale when navigating; update the dependency array of the useEffect that references generateSources/getRefreshToken/getWalletAddress/getWalletRole to also include pathname and searchParams (the route values from next/navigation) so the effect reruns and regenerates the QR/copy URL whenever the route changes while the modal is open.
🧹 Nitpick comments (1)
components/header/share/HeaderShare.tsx (1)
823-866: Avoid refetching the manifest files on every tab toggle.
CoreAppsDownloadis mounted only for the Apps/Desktop branch, so leaving and re-entering that sub-tab reruns all three network requests. Caching these results inHeaderQRModalor at module scope would make the modal feel much snappier and cut duplicate traffic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@components/header/share/HeaderShare.tsx`:
- Around line 352-364: The code builds deep links by raw string concatenation
which can break if refreshToken/walletAddress/role contain reserved characters
and currently only clears the QR fallback; update the logic in the block that
references refreshToken, walletAddress, role, DeepLinkScope.SHARE_CONNECTION and
DeepLinkScope.NAVIGATE so you first compute local shareConnectionAppUrl and
shareConnectionCoreUrl using encodeURIComponent for token, address, and role,
then call setShareConnectionAppUrl and setShareConnectionCoreUrl together; in
the else branch ensure you clear BOTH setShareConnectionAppUrl("") and
setShareConnectionCoreUrl("") so stale URLs can’t be copied.
- Around line 396-400: The effect that calls generateSources (inside the
useEffect block) only depends on show, but generateSources uses the current
pathname and searchParams so the share data can become stale when navigating;
update the dependency array of the useEffect that references
generateSources/getRefreshToken/getWalletAddress/getWalletRole to also include
pathname and searchParams (the route values from next/navigation) so the effect
reruns and regenerates the QR/copy URL whenever the route changes while the
modal is open.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e6b0810-26d0-4c4a-9476-90d55bea2c61
📒 Files selected for processing (1)
components/header/share/HeaderShare.tsx
|
ragnep
approved these changes
Mar 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit