chore(frontend): shadcn v4 upgrade, Alby theme polish, hub logo#2155
chore(frontend): shadcn v4 upgrade, Alby theme polish, hub logo#2155stackingsaunter wants to merge 5 commits intogetAlby:masterfrom
Conversation
- Refresh vendored UI primitives from shadcn registry (radix-ui, lucide, etc.) - Fix components.json tailwind.config for Tailwind v4 CLI - Preserve Hub splits: buttonVariants, badgeVariants, hybrid tooltip + TouchProvider - Keep custom alert/badge variants; sonner uses document dark class - Remove duplicate use-mobile.ts (use use-mobile.tsx only) - Card keeps shadow-sm to match pre-change product default Made-with: Cursor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMigrates many UI components from individual Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/package.json (1)
25-41:⚠️ Potential issue | 🟠 MajorComplete the Radix UI migration or remove the consolidated package.
The PR adds the consolidated
radix-uipackage (line 54) while individual@radix-ui/*packages (lines 25-41) remain. However, the migration is incomplete—two files still import from individual packages:
frontend/src/components/stepper.tsxfrontend/src/components/ui/custom/circle-progress.tsxEither migrate these imports to the consolidated package and remove the individual dependencies, or revert the
radix-uiaddition to maintain consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/package.json` around lines 25 - 41, The package.json shows both the consolidated "radix-ui" package and many individual "@radix-ui/*" packages; resolve this by either (A) migrating the two remaining imports in frontend/src/components/stepper.tsx and frontend/src/components/ui/custom/circle-progress.tsx to import from "radix-ui" and then remove the now-unneeded individual "@radix-ui/*" entries from package.json, or (B) revert/remove the consolidated "radix-ui" dependency and keep the existing individual "@radix-ui/*" entries, ensuring import paths in stepper.tsx and circle-progress.tsx match the chosen approach; update package.json and run install to verify no unresolved imports.
🧹 Nitpick comments (2)
frontend/src/components/ui/command.tsx (1)
53-53: Optional: extract the longCommandclass string for maintainability.The inline class list is quite dense; moving it to a local
const(orcva) would make future edits less error-prone.Refactor sketch
+const commandDialogContentClass = + "**:data-[slot=command-input-wrapper]:h-12 [&_[cmdk-group-heading]]:px-2 [&_[cmdk-group-heading]]:font-medium [&_[cmdk-group-heading]]:text-muted-foreground [&_[cmdk-group]]:px-2 [&_[cmdk-group]:not([hidden])_~[cmdk-group]]:pt-0 [&_[cmdk-input-wrapper]_svg]:h-5 [&_[cmdk-input-wrapper]_svg]:w-5 [&_[cmdk-input]]:h-12 [&_[cmdk-item]]:px-2 [&_[cmdk-item]]:py-3 [&_[cmdk-item]_svg]:h-5 [&_[cmdk-item]_svg]:w-5"; ... - <Command className="**:data-[slot=command-input-wrapper]:h-12 [&_[cmdk-group-heading]]:px-2 [&_[cmdk-group-heading]]:font-medium [&_[cmdk-group-heading]]:text-muted-foreground [&_[cmdk-group]]:px-2 [&_[cmdk-group]:not([hidden])_~[cmdk-group]]:pt-0 [&_[cmdk-input-wrapper]_svg]:h-5 [&_[cmdk-input-wrapper]_svg]:w-5 [&_[cmdk-input]]:h-12 [&_[cmdk-item]]:px-2 [&_[cmdk-item]]:py-3 [&_[cmdk-item]_svg]:h-5 [&_[cmdk-item]_svg]:w-5"> + <Command className={commandDialogContentClass}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ui/command.tsx` at line 53, The Command component's long inline className string should be extracted to a named constant (e.g., COMMAND_BASE_CLASSES or commandClasses) or a cva utility to improve maintainability; locate the <Command ...> usage in the file (symbol: Command) and replace the inline className value with the constant, defining that constant above the component so future edits only change the single variable rather than the long attribute string.frontend/src/components/ui/alert.tsx (1)
13-15: Remove duplicate variant utilities to reduce class noise.
[&>svg]:text-currentis already in the base class, andborderis also already present globally. Keeping these duplicates in variant strings isn’t harmful, but removing them will make maintenance clearer.♻️ Suggested cleanup
destructive: - "bg-card text-destructive *:data-[slot=alert-description]:text-destructive/90 [&>svg]:text-current", + "bg-card text-destructive *:data-[slot=alert-description]:text-destructive/90", warning: - "border-warning-foreground border text-warning-foreground *:data-[slot=alert-description]:text-warning-foreground/90 [&>svg]:text-current", + "border-warning-foreground text-warning-foreground *:data-[slot=alert-description]:text-warning-foreground/90",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ui/alert.tsx` around lines 13 - 15, Remove duplicate utilities from the alert variants: in the Alert component's variant definitions (the base class string and the variant map keys like "warning" and "destructive"), eliminate redundant "[&>svg]:text-current" from the "warning" and "destructive" variant values and remove the redundant "border" from the "warning" variant since those are already present in the shared/base class; update the variant strings accordingly so they only contain unique utilities.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/ui/alert-dialog.tsx`:
- Around line 131-145: The Tailwind selector in AlertDialogMedia's className is
invalid; replace the malformed `*:[svg:not([class*='size-'])]:size-8` with the
correct descendant form `[&_svg:not([class*='size-'])]:size-8` inside the
cn(...) call in the AlertDialogMedia component so child SVGs without a "size-"
class get size-8 applied.
In `@frontend/src/components/ui/field.tsx`:
- Around line 203-209: Replace the loose equality check and redundant optional
chaining for the deduped errors array: in the block that builds uniqueErrors
(the constant uniqueErrors created from new Map(...).values()), change the
conditional if (uniqueErrors?.length == 1) to use strict equality and no
optional chaining — i.e. reference uniqueErrors directly and use ===; keep
returning uniqueErrors[0]?.message (you may keep the inner optional chaining on
the message itself). This updates the check around uniqueErrors to if
(uniqueErrors.length === 1) while leaving the rest of the dedup logic unchanged.
In `@frontend/src/components/ui/popover.tsx`:
- Around line 56-63: PopoverTitle currently declares props as
React.ComponentProps<"h2"> but returns a <div>, causing a type mismatch; fix by
making the rendered element match the prop type: change the returned element to
an <h2> (keeping data-slot="popover-title", className={cn("font-medium",
className)} and {...props}) so h2-specific props are applied correctly, or
alternatively change the prop type to React.ComponentProps<"div"> if you intend
to keep a div—update the signature and ensure className and {...props} stay
intact on the chosen element.
---
Outside diff comments:
In `@frontend/package.json`:
- Around line 25-41: The package.json shows both the consolidated "radix-ui"
package and many individual "@radix-ui/*" packages; resolve this by either (A)
migrating the two remaining imports in frontend/src/components/stepper.tsx and
frontend/src/components/ui/custom/circle-progress.tsx to import from "radix-ui"
and then remove the now-unneeded individual "@radix-ui/*" entries from
package.json, or (B) revert/remove the consolidated "radix-ui" dependency and
keep the existing individual "@radix-ui/*" entries, ensuring import paths in
stepper.tsx and circle-progress.tsx match the chosen approach; update
package.json and run install to verify no unresolved imports.
---
Nitpick comments:
In `@frontend/src/components/ui/alert.tsx`:
- Around line 13-15: Remove duplicate utilities from the alert variants: in the
Alert component's variant definitions (the base class string and the variant map
keys like "warning" and "destructive"), eliminate redundant
"[&>svg]:text-current" from the "warning" and "destructive" variant values and
remove the redundant "border" from the "warning" variant since those are already
present in the shared/base class; update the variant strings accordingly so they
only contain unique utilities.
In `@frontend/src/components/ui/command.tsx`:
- Line 53: The Command component's long inline className string should be
extracted to a named constant (e.g., COMMAND_BASE_CLASSES or commandClasses) or
a cva utility to improve maintainability; locate the <Command ...> usage in the
file (symbol: Command) and replace the inline className value with the constant,
defining that constant above the component so future edits only change the
single variable rather than the long attribute string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e80baa84-97a7-4183-8282-c11cf91ceae1
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (38)
frontend/components.jsonfrontend/package.jsonfrontend/src/components/ui/accordion.tsxfrontend/src/components/ui/alert-dialog.tsxfrontend/src/components/ui/alert.tsxfrontend/src/components/ui/avatar.tsxfrontend/src/components/ui/badge.tsxfrontend/src/components/ui/badgeVariants.tsxfrontend/src/components/ui/breadcrumb.tsxfrontend/src/components/ui/button.tsxfrontend/src/components/ui/buttonVariants.tsxfrontend/src/components/ui/calendar.tsxfrontend/src/components/ui/card.tsxfrontend/src/components/ui/carousel.tsxfrontend/src/components/ui/checkbox.tsxfrontend/src/components/ui/command.tsxfrontend/src/components/ui/dialog.tsxfrontend/src/components/ui/dropdown-menu.tsxfrontend/src/components/ui/field.tsxfrontend/src/components/ui/input.tsxfrontend/src/components/ui/label.tsxfrontend/src/components/ui/navigation-menu.tsxfrontend/src/components/ui/pagination.tsxfrontend/src/components/ui/popover.tsxfrontend/src/components/ui/progress.tsxfrontend/src/components/ui/radio-group.tsxfrontend/src/components/ui/select.tsxfrontend/src/components/ui/separator.tsxfrontend/src/components/ui/sheet.tsxfrontend/src/components/ui/sidebar.tsxfrontend/src/components/ui/skeleton.tsxfrontend/src/components/ui/sonner.tsxfrontend/src/components/ui/switch.tsxfrontend/src/components/ui/table.tsxfrontend/src/components/ui/tabs.tsxfrontend/src/components/ui/textarea.tsxfrontend/src/components/ui/tooltip.tsxfrontend/src/hooks/use-mobile.ts
💤 Files with no reviewable changes (1)
- frontend/src/hooks/use-mobile.ts
| function AlertDialogMedia({ | ||
| className, | ||
| ...props | ||
| }: React.ComponentProps<typeof AlertDialogPrimitive.Action>) { | ||
| }: React.ComponentProps<"div">) { | ||
| return ( | ||
| <AlertDialogPrimitive.Action | ||
| className={cn(buttonVariants(), className)} | ||
| <div | ||
| data-slot="alert-dialog-media" | ||
| className={cn( | ||
| "mb-2 inline-flex size-16 items-center justify-center rounded-md bg-muted sm:group-data-[size=default]/alert-dialog-content:row-span-2 *:[svg:not([class*='size-'])]:size-8", | ||
| className | ||
| )} | ||
| {...props} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Tailwind CSS arbitrary variant syntax for targeting child SVG elements
💡 Result:
To target child SVG elements with Tailwind CSS arbitrary variants, apply classes like [&_svg]:fill-red-500 or [&>svg]:fill-blue-600 to the parent element. The & represents the parent, _svg targets descendant svg elements (space replaced by _), and >svg targets direct child svg elements. Example: div class="[&_svg]:fill-current [&_svg]:w-6 [&_svg]:h-6 hover:[&_svg]:fill-red-500" svg { ... } svg { ... } /div This generates CSS like .[&_svg]:fill-current svg { fill: currentColor; }. For direct children only, use [&>svg]:utility. Spaces in selectors become _ (e.g., [& svg]:utility). Tailwind v3.1+ supports arbitrary variants per official docs and GitHub PR #8299. Multiple sources confirm child targeting with [&_tag]:utility (e.g., posts from 2025, Stack Overflow).
Citations:
- 1: https://cekrem.github.io/posts/tailwind-targeting-child-elements/
- 2: Arbitrary variants tailwindlabs/tailwindcss#8299
- 3: https://agk.io/posts/arbitrary-values-variants-in-tailwindcss/
- 4: https://tailwindcss.com/docs/pseudo-class-variants
- 5: https://stackoverflow.com/questions/67119992/how-to-access-all-the-direct-children-of-a-div-in-tailwind-css
- 6: https://tailwindcss.com/docs/fill
- 7: Add
*variant for targeting direct children tailwindlabs/tailwindcss#12551
Invalid Tailwind CSS selector syntax for targeting child SVG elements.
The selector *:[svg:not([class*='size-'])]:size-8 is malformed. The *: prefix is not valid Tailwind syntax, and [svg:not(...)] conflates attribute selector brackets with element matching.
Use [&_svg:not([class*='size-'])]:size-8 instead, where & references the parent element, _ represents a descendant space, and :not([class*='size-']) filters out SVG elements that already have a class containing "size-".
Proposed fix
- "mb-2 inline-flex size-16 items-center justify-center rounded-md bg-muted sm:group-data-[size=default]/alert-dialog-content:row-span-2 *:[svg:not([class*='size-'])]:size-8",
+ "mb-2 inline-flex size-16 items-center justify-center rounded-md bg-muted sm:group-data-[size=default]/alert-dialog-content:row-span-2 [&_svg:not([class*='size-'])]:size-8",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/ui/alert-dialog.tsx` around lines 131 - 145, The
Tailwind selector in AlertDialogMedia's className is invalid; replace the
malformed `*:[svg:not([class*='size-'])]:size-8` with the correct descendant
form `[&_svg:not([class*='size-'])]:size-8` inside the cn(...) call in the
AlertDialogMedia component so child SVGs without a "size-" class get size-8
applied.
| const uniqueErrors = [ | ||
| ...new Map(errors.map((error) => [error?.message, error])).values(), | ||
| ]; | ||
|
|
||
| if (uniqueErrors?.length == 1) { | ||
| return uniqueErrors[0]?.message; | ||
| } |
There was a problem hiding this comment.
Use strict equality and simplify optional chaining.
Line 207 uses loose equality (==) instead of strict equality (===). Also, uniqueErrors is always an array (spread from Map.values()), so optional chaining is unnecessary.
Proposed fix
const uniqueErrors = [
...new Map(errors.map((error) => [error?.message, error])).values(),
];
- if (uniqueErrors?.length == 1) {
+ if (uniqueErrors.length === 1) {
return uniqueErrors[0]?.message;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/ui/field.tsx` around lines 203 - 209, Replace the
loose equality check and redundant optional chaining for the deduped errors
array: in the block that builds uniqueErrors (the constant uniqueErrors created
from new Map(...).values()), change the conditional if (uniqueErrors?.length ==
1) to use strict equality and no optional chaining — i.e. reference uniqueErrors
directly and use ===; keep returning uniqueErrors[0]?.message (you may keep the
inner optional chaining on the message itself). This updates the check around
uniqueErrors to if (uniqueErrors.length === 1) while leaving the rest of the
dedup logic unchanged.
| function PopoverTitle({ className, ...props }: React.ComponentProps<"h2">) { | ||
| return ( | ||
| <div | ||
| data-slot="popover-title" | ||
| className={cn("font-medium", className)} | ||
| {...props} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
Type mismatch: PopoverTitle is typed as <h2> but renders a <div>.
The function signature declares React.ComponentProps<"h2"> but the component renders a <div> element. This creates a type inconsistency where h2-specific props would be typed but not applied correctly.
Proposed fix
function PopoverTitle({ className, ...props }: React.ComponentProps<"h2">) {
return (
- <div
+ <h2
data-slot="popover-title"
className={cn("font-medium", className)}
{...props}
- />
+ />
);
}📝 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.
| function PopoverTitle({ className, ...props }: React.ComponentProps<"h2">) { | |
| return ( | |
| <div | |
| data-slot="popover-title" | |
| className={cn("font-medium", className)} | |
| {...props} | |
| /> | |
| ); | |
| function PopoverTitle({ className, ...props }: React.ComponentProps<"h2">) { | |
| return ( | |
| <h2 | |
| data-slot="popover-title" | |
| className={cn("font-medium", className)} | |
| {...props} | |
| /> | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/ui/popover.tsx` around lines 56 - 63, PopoverTitle
currently declares props as React.ComponentProps<"h2"> but returns a <div>,
causing a type mismatch; fix by making the rendered element match the prop type:
change the returned element to an <h2> (keeping data-slot="popover-title",
className={cn("font-medium", className)} and {...props}) so h2-specific props
are applied correctly, or alternatively change the prop type to
React.ComponentProps<"div"> if you intend to keep a div—update the signature and
ensure className and {...props} stay intact on the chosen element.
- Alby: lighter primary gradient, 1px stroke, stacked focus ring - Alby: neutral gray palette; dark primary button label matches light - Hub logo: default/alby light & dark fills; mono for other themes - Figtree for alby; map tailwind font-sans via --app-font-sans - Link buttons: data-slot/data-variant for themed primary styles Made-with: Cursor
stackingsaunter
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/ExternalLink.tsx`:
- Around line 5-12: The Props type currently exposes all AnchorHTMLAttributes
allowing callers to override component-owned attributes and to pass anchor-only
props into the non-HTTP <span> branch; change Props to explicitly omit "target",
"rel", and "onClick" (and keep omitting "href"|"className"|"children") from the
forwarded attributes so those are owned by the component (e.g. Props =
React.PropsWithChildren<{to: string; className?: string; onClick?:
React.MouseEventHandler}>&
Omit<React.AnchorHTMLAttributes<HTMLAnchorElement>,'href'|'className'|'children'|'target'|'rel'|'onClick'>),
and when rendering the non-HTTP branch use appropriate HTMLElement attributes
(React.HTMLAttributes<HTMLSpanElement>) rather than anchor-only attributes;
ensure the component applies its internal target/rel/onClick after the spread
or, better, relies on the typing omission so callers cannot override
target/rel/onClick for ExternalLink.
In `@frontend/src/components/icons/AlbyHubLogo.tsx`:
- Around line 61-70: The logic in logoPalette incorrectly uses invert ||
isDarkMode so invert forces the dark palette regardless of current mode; update
the darkAppearance calculation in logoPalette to flip relative to the current
mode (use an XOR-style check: darkAppearance = isDarkMode !== invert or
equivalently darkAppearance = invert ? !isDarkMode : isDarkMode) so that when
invert is true the palette is the opposite of isDarkMode and you still return
"default-alby-dark" or "default-alby-light" accordingly; leave the
isDefaultOrAlbyTheme early return unchanged.
In `@frontend/src/themes/default.css`:
- Around line 8-10: The root CSS variable --app-font-sans currently falls back
to serif; update its fallback to a proper sans-serif stack so Tailwind's
font-sans (which now references --app-font-sans) doesn't revert to serif when
"Inter var" is unavailable—replace the trailing serif fallback with a sensible
sans stack (e.g. system UI and common sans fonts) in the --app-font-sans
declaration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ca21131-240d-42f2-b870-b00baa974ffa
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
frontend/package.jsonfrontend/src/components/AppSidebar.tsxfrontend/src/components/ExternalLink.tsxfrontend/src/components/icons/AlbyHubLogo.tsxfrontend/src/components/layouts/TwoColumnFullScreenLayout.tsxfrontend/src/components/ui/custom/external-link-button.tsxfrontend/src/components/ui/custom/link-button.tsxfrontend/src/themes/alby.cssfrontend/src/themes/default.cssfrontend/src/themes/index.css
✅ Files skipped from review due to trivial changes (1)
- frontend/src/themes/index.css
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/package.json
| type Props = React.PropsWithChildren<{ | ||
| to: string; | ||
| className?: string; | ||
| children?: React.ReactNode; | ||
| }; | ||
| }> & | ||
| Omit< | ||
| React.AnchorHTMLAttributes<HTMLAnchorElement>, | ||
| "href" | "className" | "children" | ||
| >; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the ExternalLink component
find . -name "ExternalLink.tsx" -type fRepository: getAlby/hub
Length of output: 97
🏁 Script executed:
# Also find the external-link-button component
find . -name "*external-link*" -type fRepository: getAlby/hub
Length of output: 115
🏁 Script executed:
cat -n frontend/src/components/ExternalLink.tsxRepository: getAlby/hub
Length of output: 1062
🏁 Script executed:
cat -n frontend/src/components/ui/custom/external-link-button.tsxRepository: getAlby/hub
Length of output: 971
🏁 Script executed:
# Check for usages of ExternalLink to see what props are actually being passed
rg "ExternalLink" -A 5 -B 2 frontend/src --type tsx --type tsRepository: getAlby/hub
Length of output: 82
🏁 Script executed:
# Also check if there are any usages that pass anchor-specific attributes
rg "ExternalLink.*target|ExternalLink.*rel|ExternalLink.*download" frontend/src --type tsx --type tsRepository: getAlby/hub
Length of output: 82
🏁 Script executed:
# Check for usages of ExternalLink to see what props are actually being passed
rg "ExternalLink" -A 5 -B 2 frontend/srcRepository: getAlby/hub
Length of output: 50368
🏁 Script executed:
# Check the Link component from react-router-dom to understand what props it accepts
rg "from.*react-router" frontend/src -A 2 | head -20Repository: getAlby/hub
Length of output: 1567
🏁 Script executed:
# Verify the order of prop spreading and whether later props override earlier ones
# by checking if there are any TypeScript/React docs or examples showing override behavior
python3 << 'EOF'
# JSX prop override behavior:
# In JSX, when you have: <element attr1="value1" {...rest} />
# If rest.attr1 is also defined, rest.attr1 will override the literal "value1"
# This is standard JavaScript object merging behavior
# The spread operator expands rest into individual properties AFTER the literal attributes
# Example:
# <Link target="_blank" {...{target: "_self"}} />
# Results in: <Link target="_self" />
print("JSX prop spreading behavior verification:")
print("- Literal props before {...rest}: can be overridden by rest properties")
print("- In ExternalLink line 28: target='_blank' can be overridden by rest.target")
print("- In ExternalLink line 33: onClick={() => openLink(to)} can be overridden by rest.onClick")
print("\nThis is standard React behavior and the review comment is correct.")
EOFRepository: getAlby/hub
Length of output: 401
🏁 Script executed:
# Check if Link (from react-router-dom) has specific prop requirements
rg "Link.*from.*react-router" frontend/src -A 3 | head -20Repository: getAlby/hub
Length of output: 1676
Narrow Props to prevent overriding component-owned behavior.
The Props type accepts all React.AnchorHTMLAttributes<HTMLAnchorElement>, but the non-HTTP branch renders a <span>, making anchor-specific attributes semantically invalid. More critically, because {...rest} is spread after the built-in target/rel (line 28) and onClick (line 33), callers can override these component-owned attributes. Restrict the forwarded props to safe shared DOM attributes (aria-*, data-*, standard event handlers that don't conflict) and make onClick/target/rel non-overridable.
Also applies to: 28-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/ExternalLink.tsx` around lines 5 - 12, The Props type
currently exposes all AnchorHTMLAttributes allowing callers to override
component-owned attributes and to pass anchor-only props into the non-HTTP
<span> branch; change Props to explicitly omit "target", "rel", and "onClick"
(and keep omitting "href"|"className"|"children") from the forwarded attributes
so those are owned by the component (e.g. Props = React.PropsWithChildren<{to:
string; className?: string; onClick?: React.MouseEventHandler}>&
Omit<React.AnchorHTMLAttributes<HTMLAnchorElement>,'href'|'className'|'children'|'target'|'rel'|'onClick'>),
and when rendering the non-HTTP branch use appropriate HTMLElement attributes
(React.HTMLAttributes<HTMLSpanElement>) rather than anchor-only attributes;
ensure the component applies its internal target/rel/onClick after the spread
or, better, relies on the typing omission so callers cannot override
target/rel/onClick for ExternalLink.
| function logoPalette( | ||
| theme: string, | ||
| isDarkMode: boolean, | ||
| invert: boolean | ||
| ): LogoPalette { | ||
| if (!isDefaultOrAlbyTheme(theme)) { | ||
| return "mono"; | ||
| } | ||
| const darkAppearance = invert || isDarkMode; | ||
| return darkAppearance ? "default-alby-dark" : "default-alby-light"; |
There was a problem hiding this comment.
invert should flip the palette relative to the current mode.
frontend/src/components/layouts/TwoColumnFullScreenLayout.tsx renders the logo on bg-foreground with invert={true}. That surface is dark in light mode but light in dark mode, so invert || isDarkMode picks the dark palette in both cases. In dark mode the fullscreen logo becomes white/yellow on a light panel.
Suggested fix
- const darkAppearance = invert || isDarkMode;
+ const darkAppearance = invert ? !isDarkMode : isDarkMode;📝 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.
| function logoPalette( | |
| theme: string, | |
| isDarkMode: boolean, | |
| invert: boolean | |
| ): LogoPalette { | |
| if (!isDefaultOrAlbyTheme(theme)) { | |
| return "mono"; | |
| } | |
| const darkAppearance = invert || isDarkMode; | |
| return darkAppearance ? "default-alby-dark" : "default-alby-light"; | |
| function logoPalette( | |
| theme: string, | |
| isDarkMode: boolean, | |
| invert: boolean | |
| ): LogoPalette { | |
| if (!isDefaultOrAlbyTheme(theme)) { | |
| return "mono"; | |
| } | |
| const darkAppearance = invert ? !isDarkMode : isDarkMode; | |
| return darkAppearance ? "default-alby-dark" : "default-alby-light"; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/icons/AlbyHubLogo.tsx` around lines 61 - 70, The
logic in logoPalette incorrectly uses invert || isDarkMode so invert forces the
dark palette regardless of current mode; update the darkAppearance calculation
in logoPalette to flip relative to the current mode (use an XOR-style check:
darkAppearance = isDarkMode !== invert or equivalently darkAppearance = invert ?
!isDarkMode : isDarkMode) so that when invert is true the palette is the
opposite of isDarkMode and you still return "default-alby-dark" or
"default-alby-light" accordingly; leave the isDefaultOrAlbyTheme early return
unchanged.
| :root { | ||
| --font-sans: "Inter var", serif; | ||
| --app-font-sans: "Inter var", serif; | ||
| --radius: 0.625rem; |
There was a problem hiding this comment.
Use a sans fallback for --app-font-sans.
frontend/src/themes/index.css now routes Tailwind’s font-sans through this variable, so falling back to serif here will flip the default theme to serif text whenever Inter var is missing. A normal sans stack is safer.
Suggested fix
- --app-font-sans: "Inter var", serif;
+ --app-font-sans: "Inter var", ui-sans-serif, system-ui, sans-serif;📝 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.
| :root { | |
| --font-sans: "Inter var", serif; | |
| --app-font-sans: "Inter var", serif; | |
| --radius: 0.625rem; | |
| :root { | |
| --app-font-sans: "Inter var", ui-sans-serif, system-ui, sans-serif; | |
| --radius: 0.625rem; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/themes/default.css` around lines 8 - 10, The root CSS variable
--app-font-sans currently falls back to serif; update its fallback to a proper
sans-serif stack so Tailwind's font-sans (which now references --app-font-sans)
doesn't revert to serif when "Inter var" is unavailable—replace the trailing
serif fallback with a sensible sans stack (e.g. system UI and common sans fonts)
in the --app-font-sans declaration.
Select Xcode.app for CGO on macOS so Wails WebKit compiles. Skip Apple signing/notarize/DMG when the PR head is a fork (no secrets); upload an unsigned zip for Wails fork PRs. Set fail-fast false on the HTTP build matrix so Linux jobs finish if macOS fails. Made-with: Cursor
For workflow_call runs, require inputs.build-release for macOS signing. Fork PR logic unchanged. Made-with: Cursor
Revert workflow_call gating and fork unsigned logic. Remove Xcode selection step added for CGO builds. Made-with: Cursor
1 participant
Summary
shadcn / Tailwind v4
components.json, Tailwind v4–compatible setup).radix-ui,lucide-react, etc.).buttonVariants/badgeVariants, hybrid tooltip +TouchProvider, custom variants,sonnertied to documentdarkclass.shadow-smto match currentmasterdefault.Alby theme & branding
#ffdf6fring, stackedfocus-visiblering with theme--ring.@fontsource-variable/figtree; Tailwindfont-sansfollows per-theme--app-font-sans(default.css/index.css).Hub wordmark (
AlbyHubLogo)#202020/#ffc800light; white /#ffe480dark); other themes use monochromecurrentColor.invertforces dark-surface variant on default/alby (e.g. intro column).Link-as-button styling
ExternalLink/LinkButton/ExternalLinkButtonforwarddata-slot/data-variantso themed primary button CSS applies.Test plan
cd frontend && yarn lintSummary by CodeRabbit
New Features
Chores