Skip to content

Conversation

@dmnktoe
Copy link
Owner

@dmnktoe dmnktoe commented Jan 7, 2026

This pull request introduces a new, centralized z-index scale to the design tokens and refactors all usages of hardcoded z-index values across the codebase to use these named tokens. This ensures consistent stacking order for UI elements, improves maintainability, and enhances clarity when working with overlays, widgets, banners, and other layered components. Additionally, some improvements to widget and menu styling are included.

Design Tokens & Theming

  • Added a comprehensive zIndex scale to @httpjpg/tokens, providing named z-index values for all UI stacking needs (e.g., modal, header, floating, cookieBanner, etc.), along with TypeScript types for safety.
  • Exported zIndex and related types from the tokens package and integrated them into the Panda CSS config and the complete tokens object. [1] [2] [3] [4] [5]

Refactoring z-index Usage

  • Replaced all hardcoded z-index values in UI components (e.g., Header, MobileMenuButton, MobileMenuContent, CustomCursor, PreviewNotificationContent, NowPlayingWidgetComponent, PSNCard, CookieBanner) with the new named tokens from zIndex, ensuring consistent stacking order throughout the app. [1] [2] [3] [4] [5] [6] [7] [8] [9]

Widget and Menu Styling Improvements

  • Improved the NowPlayingWidgetComponent to use the zIndex.floating token and adjusted its structure for better pointer event handling. [1] [2]
  • Enhanced MobileMenuContent with better overflow handling, maximum width constraints, and more robust layout for mobile navigation. [1] [2] [3]

Storyblok Work List Component Enhancements

  • Upgraded SbWorkList to support new props for divider customization (gap, showDividers, dividerPattern, dividerColor, dividerSpacing), improved error handling and fallback rendering, and ensured more robust data transformation for work items. [1] [2] [3] [4]

These changes collectively make the UI more maintainable, visually consistent, and easier to reason about when developing new features or debugging stacking issues.

Summary by CodeRabbit

  • New Features

    • Work list components now support configurable dividers with customizable gap and spacing options
    • Added error messaging and fallback display for missing work items
  • Improvements

    • Mobile menu layout enhanced with improved text truncation and overflow handling
    • UI stacking and layering optimized for consistent component display
    • Improved work data handling with better defaults and error states

✏️ Tip: You can customize this high-level summary in your review settings.

@vercel
Copy link

vercel bot commented Jan 7, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
httpjpg Ready Ready Preview, Comment Jan 7, 2026 10:52am
httpjpg-docs Ready Ready Preview, Comment Jan 7, 2026 10:52am
httpjpg-storybook Ready Ready Preview, Comment Jan 7, 2026 10:52am

@coderabbitai
Copy link

coderabbitai bot commented Jan 7, 2026

Walkthrough

This pull request migrates the codebase from hard-coded numeric z-index values to a centralized, typed design token system. A new z-index token module with 21 named levels is introduced and integrated across components, CSS utilities, and design systems. Additionally, some components receive layout refinements and improved data handling.

Changes

Cohort / File(s) Summary
Z-Index Token System (Core)
packages/tokens/src/z-index.ts, packages/tokens/src/index.ts, packages/ui/panda.config.ts, packages/ui/styles.css
New z-index token module with 21 typed levels (hide, base, docked, widget, header, floating, etc.); re-exported from tokens package; integrated into Panda CSS theme; CSS variables and utility classes (.z_preview, .z_widget, etc.) introduced in styles.css to centralize z-index management.
Preview & Notification Components
apps/web/components/ui/preview-notification.tsx, packages/ui/src/components/image-preview/image-preview.tsx
Replaced numeric z-index values (9999) with string token ("preview") for consistent stacking.
Interactive Widget Components
apps/web/components/widgets/now-playing-widget.tsx, apps/web/components/widgets/psn-card.tsx, packages/now-playing/src/now-playing*.tsx
Updated z-index handling; now-playing-widget introduces full-screen fixed overlay container with pointerEvents management; z-index values replaced with tokens or removed entirely.
Header & Navigation Components
packages/ui/src/components/header/header.tsx, packages/ui/src/components/header/mobile-menu-*.tsx, packages/ui/src/components/nav-link/nav-link.tsx
Header and mobile menu button z-index values replaced with tokens ("header", "mobileMenuButton"); mobile-menu-content refactored with max-width constraints, overflow handling, and text truncation across elements; nav-link adds relative positioning and z-index: 1 to both internal and external links.
Visual Effects Components
packages/ui/src/components/mouse-trail/mouse-trail.tsx, packages/ui/src/components/custom-cursor/custom-cursor.tsx
Numeric z-index values (9998, 999999) replaced with shared token-based values ("mouseEffects", "cursor").
Work Display Components
packages/ui/src/components/work-card/work-card.tsx, packages/ui/src/components/work-list/work-list.tsx, packages/storyblok-ui/src/components/work-list/SbWorkList.tsx
WorkCard z-index increased from 10 to 20; WorkList replaces VStack wrapper with fragment for list items, affecting key assignment; SbWorkList enriched with divider configuration (gap, showDividers, dividerPattern, dividerColor, dividerSpacing), improved work item filtering and caching, robust data transformation with fallback handling, and error state rendering.
Cookie Management
packages/consent/src/components/cookie-banner.tsx
Replaced hard-coded z-index (99999) with token-based value (zIndex.cookieBanner).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Hops with glee through stacking layers,
Z-index tokens, no more cares!
From magic numbers to a named design,
The hierarchy is crystal-clear and fine—
Float, dock, and modal arrange with grace,
A token system takes its rightful place.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'fix: zindexes' is vague and uses imprecise language that doesn't clearly convey the primary change of centralizing z-index tokens and refactoring hardcoded values. Consider a more descriptive title like 'feat: centralize z-index tokens and refactor hardcoded values' to better reflect that this introduces a new design token system rather than fixing an existing bug.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In @apps/web/components/widgets/now-playing-widget.tsx:
- Around line 35-65: The outer full-viewport <button> (with onClick=handleClick)
is semantically and accessibly wrong; replace it with a non-interactive Box (or
div) that uses the component's css prop for the overlay positioning/zIndex
instead of inline styles, move the onClick/handleClick and focusable behavior to
the inner interactive container that wraps NowPlaying (give that wrapper
tabIndex=0 or make it a real button/anchor and keep pointerEvents enabled),
ensure only the NowPlaying area is keyboard-focusable (no giant focus ring on
the viewport), preserve zIndex/inset positioning and the NowPlaying props
(title/artist/artwork/isPlaying/isLoading/autoExtractColor/vibrantColor/textColor)
and remove pointerEvents: "none" from the outer element.

In @packages/ui/src/components/work-card/work-card.tsx:
- Around line 320-322: The Box currently uses a hardcoded zIndex value instead
of the centralized token; import zIndex from "@httpjpg/tokens" and replace the
hardcoded value in the Box css prop with zIndex.docked (update the import
section and change the Box css={{ zIndex: 20 }} to use zIndex.docked) so the
component (work-card.tsx, Box) uses the tokenized value.

In @packages/ui/src/components/work-list/work-list.tsx:
- Around line 127-132: The fragment returned around each mapped item currently
uses the shorthand <>...</> while the key is applied to the WorkCard; move the
key to the top-level fragment by replacing the shorthand with an explicit
React.Fragment and give that fragment the key (e.g., React.Fragment
key={work.slug || index}); keep WorkCard and Divider props unchanged and remove
the key from WorkCard.
🧹 Nitpick comments (8)
packages/ui/src/components/mouse-trail/mouse-trail.tsx (1)

130-130: Use imported zIndex token for type safety.

The component uses a string literal "mouseEffects" instead of importing the token from @httpjpg/tokens. This bypasses TypeScript's type checking and could lead to runtime errors if the key is mistyped.

♻️ Recommended refactor for type safety
+"use client";
+
+import { useEffect, useRef, useState } from "react";
+import type { SystemStyleObject } from "styled-system/types";
+import { Box } from "../box/box";
+import { zIndex } from "@httpjpg/tokens";

 export function MouseTrail({
   character = "✦",
   count = 20,
   lifetime = 1000,
   size = "24px",
   color = "black",
   css: cssProp,
 }: MouseTrailProps) {
   // ...
   
   return (
     <Box
       css={{
         position: "fixed",
         top: 0,
         left: 0,
         w: "100%",
         h: "100%",
         pointerEvents: "none",
-        zIndex: "mouseEffects",
+        zIndex: zIndex.mouseEffects,
         ...cssProp,
       }}
     >

This matches the pattern used in custom-cursor.tsx and provides autocomplete and compile-time validation.

packages/ui/src/components/image-preview/image-preview.tsx (1)

117-117: Use imported zIndex token for type safety.

Similar to mouse-trail.tsx, this component uses a string literal "preview" instead of importing the token. This bypasses type checking and increases the risk of errors.

♻️ Recommended refactor for type safety

Add the import at the top of the file:

 "use client";
 
 import { useEffect, useRef, useState } from "react";
+import { zIndex } from "@httpjpg/tokens";

Then use the imported token:

   return (
     <div
       ref={containerRef}
       style={{
         position: "fixed",
         top: 0,
         left: 0,
         width: `${width}px`,
         height: `${height}px`,
         transform: "translate(-1000px, -1000px)",
         pointerEvents: "none",
-        zIndex: "preview",
+        zIndex: zIndex.preview,
       }}
     >

This provides type safety and matches the pattern used in custom-cursor.tsx.

packages/ui/src/components/nav-link/nav-link.tsx (1)

101-106: Inconsistent with token migration goals.

The PR introduces a centralized z-index token system to replace hardcoded values, but this change adds a new hardcoded zIndex: 1. While this might be intentional for local component-level stacking (ensuring nav links render above their siblings), it creates inconsistency in the codebase.

Consider using a token like zIndex.base + 1 or documenting why this local z-index is appropriate and shouldn't use the token system.

♻️ Option: Use a token for consistency

If this is truly meant to be a minimal local z-index, you could define a token for it or use an existing one. For now, adding a comment would help:

         style={{
           position: "relative",
-          zIndex: 1,
+          zIndex: 1, // Local stacking context for nav link pseudo-elements
           cursor:
packages/tokens/src/z-index.ts (2)

50-54: floating token is out of order and may cause confusion.

The floating: 10 token is placed after header: 80 in the source but has a lower z-index value. This makes the scale harder to read and maintain. Consider reordering tokens by their numeric values for clarity, or grouping related tokens together with clear section comments.


1-11: JSDoc mentions "20-step increments" but values don't follow this pattern.

The header comment states the scale "Uses 20-step increments for clear hierarchy," but many values deviate from this (e.g., 10, 300, 320, 340, 360, 380). Consider updating the documentation to accurately describe the actual scale strategy, or reorganize values to match the documented pattern.

packages/storyblok-ui/src/components/work-list/SbWorkList.tsx (3)

60-60: Consider replacing any[] with a specific type.

Using any[] for lastWorkItemsRef bypasses TypeScript's type checking. Define a type for the work item shape to catch potential type mismatches early.

♻️ Suggested type definition
interface TransformedWorkItem {
  id: string;
  slug: string;
  title: string;
  description: ReactNode;
  images: Array<{
    url: string;
    alt: string;
    copyright?: string;
    focus?: string;
    videoUrl?: string;
  }>;
  date: string;
  baseUrl: string;
}

// Then use:
const lastWorkItemsRef = useRef<TransformedWorkItem[]>([]);

130-137: Silent error handling may hide issues during development.

The empty catch block silently swallows errors from renderStoryblokRichText. Consider logging in development mode to help debug content issues.

♻️ Suggested improvement
       try {
         description = renderStoryblokRichText(item.content.description);
-      } catch {
+      } catch (error) {
         // Silently fail - description is optional
+        if (process.env.NODE_ENV === "development") {
+          console.warn(`Failed to render description for ${item.slug}:`, error);
+        }
       }

81-99: Duplicate dividerProps construction.

The dividerProps object is constructed identically in two places (lines 87-96 and 190-199). Consider extracting this to a variable to reduce duplication and ensure consistency.

♻️ Suggested refactor
+ const dividerPropsConfig = showDividersValue
+   ? {
+       variant: "ascii" as const,
+       pattern: dividerPattern || "⋆。°✩ ・ ✦ ・ ✧ ・ ✦ ・ ✩°。⋆",
+       color: mapColorToToken(dividerColor) || "neutral.200",
+       spacing: dividerSpacing || "3",
+     }
+   : undefined;

  // Then use dividerPropsConfig in both WorkList instances

Also applies to: 186-200

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ece7503 and df97413.

📒 Files selected for processing (20)
  • apps/web/components/ui/preview-notification.tsx
  • apps/web/components/widgets/now-playing-widget.tsx
  • apps/web/components/widgets/psn-card.tsx
  • packages/consent/src/components/cookie-banner.tsx
  • packages/now-playing/src/now-playing-loading.tsx
  • packages/now-playing/src/now-playing.tsx
  • packages/storyblok-ui/src/components/work-list/SbWorkList.tsx
  • packages/tokens/src/index.ts
  • packages/tokens/src/z-index.ts
  • packages/ui/panda.config.ts
  • packages/ui/src/components/custom-cursor/custom-cursor.tsx
  • packages/ui/src/components/header/header.tsx
  • packages/ui/src/components/header/mobile-menu-button.tsx
  • packages/ui/src/components/header/mobile-menu-content.tsx
  • packages/ui/src/components/image-preview/image-preview.tsx
  • packages/ui/src/components/mouse-trail/mouse-trail.tsx
  • packages/ui/src/components/nav-link/nav-link.tsx
  • packages/ui/src/components/work-card/work-card.tsx
  • packages/ui/src/components/work-list/work-list.tsx
  • packages/ui/styles.css
💤 Files with no reviewable changes (2)
  • packages/now-playing/src/now-playing.tsx
  • packages/now-playing/src/now-playing-loading.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}

⚙️ CodeRabbit configuration file

Review the Typescript and React code for conformity with best practices in React, and Typescript. Highlight any deviations.

Files:

  • packages/ui/src/components/work-card/work-card.tsx
  • packages/ui/src/components/nav-link/nav-link.tsx
  • apps/web/components/widgets/psn-card.tsx
  • packages/tokens/src/z-index.ts
  • packages/tokens/src/index.ts
  • packages/ui/src/components/header/mobile-menu-content.tsx
  • packages/ui/src/components/custom-cursor/custom-cursor.tsx
  • packages/ui/panda.config.ts
  • apps/web/components/widgets/now-playing-widget.tsx
  • packages/consent/src/components/cookie-banner.tsx
  • packages/ui/src/components/header/header.tsx
  • packages/ui/src/components/work-list/work-list.tsx
  • apps/web/components/ui/preview-notification.tsx
  • packages/storyblok-ui/src/components/work-list/SbWorkList.tsx
  • packages/ui/src/components/header/mobile-menu-button.tsx
  • packages/ui/src/components/image-preview/image-preview.tsx
  • packages/ui/src/components/mouse-trail/mouse-trail.tsx
🧬 Code graph analysis (8)
packages/ui/src/components/nav-link/nav-link.tsx (5)
packages/ui/src/components/link/link.tsx (2)
  • css (77-145)
  • LinkProps (8-33)
apps/storybook/.storybook/next-link-mock.tsx (2)
  • props (21-41)
  • LinkProps (8-18)
apps/storybook/stories/Link.stories.tsx (1)
  • Box (124-148)
apps/storybook/stories/NavLink.stories.tsx (2)
  • Box (142-164)
  • Box (171-193)
packages/ui/src/components/header/navigation.tsx (1)
  • work (89-107)
apps/web/components/widgets/psn-card.tsx (3)
packages/ui/src/components/stack/stack.tsx (1)
  • css (82-118)
apps/storybook/stories/AnimateInView.stories.tsx (1)
  • Box (218-263)
packages/ui/src/components/box/box.tsx (1)
  • C (142-165)
packages/tokens/src/z-index.ts (1)
packages/tokens/src/index.ts (3)
  • zIndex (25-25)
  • ZIndexKey (25-25)
  • ZIndexValue (25-25)
packages/tokens/src/index.ts (1)
packages/tokens/scripts/generate-css-vars.ts (1)
  • generateCSSVariables (31-52)
apps/web/components/widgets/now-playing-widget.tsx (1)
packages/tokens/src/z-index.ts (1)
  • zIndex (13-139)
packages/consent/src/components/cookie-banner.tsx (2)
packages/tokens/src/index.ts (1)
  • zIndex (25-25)
packages/tokens/src/z-index.ts (1)
  • zIndex (13-139)
packages/ui/src/components/work-list/work-list.tsx (2)
packages/ui/src/components/work-card/work-card.tsx (1)
  • WorkCard (280-345)
packages/ui/src/components/work-card/index.ts (1)
  • WorkCard (2-2)
packages/storyblok-ui/src/components/work-list/SbWorkList.tsx (3)
packages/ui/src/components/work-list/work-list.tsx (1)
  • WorkList (108-139)
packages/storyblok-ui/src/lib/token-mapping.ts (1)
  • mapColorToToken (19-62)
packages/storyblok-richtext/src/richtext.tsx (1)
  • renderStoryblokRichText (253-286)
🔇 Additional comments (18)
packages/tokens/src/index.ts (1)

25-25: LGTM! Clean token export integration.

The zIndex token is properly exported with its types and integrated into the consolidated tokens object, following the established pattern used by other design tokens.

Also applies to: 35-35, 55-55

packages/ui/styles.css (1)

783-803: LGTM! Clear z-index hierarchy established.

The CSS variables create a well-organized stacking context with semantic naming and appropriate spacing between levels. The range from -1 (hide) to 380 (cursor) provides sufficient granularity for UI layering.

packages/ui/src/components/custom-cursor/custom-cursor.tsx (1)

3-3: LGTM! Exemplary token usage pattern.

This component correctly imports and uses the zIndex token, providing type safety and autocomplete. This pattern should be followed by other components in the codebase.

Also applies to: 203-203, 229-229

packages/ui/src/components/header/header.tsx (1)

97-97: LGTM!

The z-index has been correctly migrated to use the "header" token from the centralized z-index scale, replacing the hardcoded value.

packages/consent/src/components/cookie-banner.tsx (1)

3-3: LGTM!

The z-index has been correctly migrated from the hardcoded value 99999 to use the centralized token zIndex.cookieBanner (360), with the proper import added. This follows the PR's objective of standardizing z-index values across the codebase.

Also applies to: 121-121

apps/web/components/ui/preview-notification.tsx (1)

38-38: LGTM!

The z-index has been correctly migrated from the hardcoded value 9999 to use the token "preview" (340), aligning with the centralized z-index system.

Note: This file uses the string token pattern (zIndex: "preview"), while cookie-banner.tsx uses the imported object pattern (zIndex: zIndex.cookieBanner). Both approaches are valid, but consider standardizing the pattern across the codebase for consistency.

apps/web/components/widgets/psn-card.tsx (1)

41-41: LGTM! Clean z-index token migration.

The migration from numeric 40 to the "widget" token aligns with the centralized z-index system. This ensures consistent stacking order for widget-level elements across the codebase.

packages/ui/src/components/header/mobile-menu-button.tsx (1)

23-23: LGTM! Z-index token migration.

Successfully migrated to the "mobileMenuButton" token, maintaining the component's fixed positioning on mobile and relative positioning on desktop.

packages/ui/panda.config.ts (2)

21-21: LGTM! Z-index token import.

Added zIndex to the imports from the centralized tokens package.


254-254: LGTM! Z-index tokens integrated into Panda theme.

The zIndex tokens are properly transformed and exposed in the Panda CSS theme, following the same pattern as other design tokens. This enables consistent z-index usage across all components using the Panda CSS css prop.

packages/tokens/src/z-index.ts (2)

72-120: Duplicate z-index values may cause stacking conflicts.

Several tokens share identical z-index values:

  • mobileMenu (300) and tooltip (300) at lines 72 and 114
  • mobileMenuButton (320) and mouseEffects (320) at lines 78 and 120
  • banner (240) and modal (240) at lines 84 and 96

If these elements appear together, their stacking order becomes unpredictable (depends on DOM order). Verify this is intentional, or adjust values to ensure distinct stacking when these elements coexist.


141-142: LGTM on type exports.

The ZIndexKey and ZIndexValue type exports provide good type safety for consumers of this token module.

packages/ui/src/components/header/mobile-menu-content.tsx (3)

29-43: Good adoption of z-index token.

The migration from a hardcoded z-index to the "mobileMenu" token aligns well with the centralized z-index scale. This improves maintainability and ensures consistent stacking across components.


78-90: Good overflow handling for scrollable content.

The combination of overflow: "hidden", overflowY: "auto", and minW: 0 correctly enables vertical scrolling while preventing horizontal overflow issues in flex containers.


99-118: Text truncation applied consistently.

The ellipsis truncation pattern (overflow: "hidden", textOverflow: "ellipsis", whiteSpace: "nowrap") is correctly applied to nav links, personal work items, and client work items. This ensures long titles don't break the layout.

packages/storyblok-ui/src/components/work-list/SbWorkList.tsx (3)

118-164: Good robust data transformation with fallbacks.

The work item transformation includes sensible defaults: title fallback chain, comprehensive date selection, safe image filtering, and video detection. The null filtering with NonNullable type guard is correct.


78-115: Good caching strategy for Visual Editor stability.

The lastWorkItemsRef caching mechanism ensures the UI remains stable during Storyblok Visual Editor reloads when stories are temporarily unresolved. The conditional check work && work.length > 0 correctly distinguishes between "loading" and "empty" states.


152-158: This concern is unfounded. The Slideshow component checks if (image.videoUrl) before rendering, which ensures videos are rendered via SlideshowVideoSlide (using the videoUrl property) while images are rendered via the Image component (using the url property). The empty URL string for videos is intentionally set but never used—it's never passed to the Image component because the videoUrl condition is checked first. No downstream issues will occur.

Comment on lines +35 to 65
<button
type="button"
onClick={handleClick}
style={{
all: "unset",
position: "fixed",
inset: 0,
pointerEvents: "none",
zIndex: zIndex.floating,
}}
>
<div style={{ pointerEvents: "auto" }}>
<NowPlaying
title={isLoading ? "Loading..." : data?.title || "╱╱ #welovemusic ╱╱"}
artist={
isLoading ? "..." : data?.artist || "⋄ ⋄ ⋄ (spotify(none)) ⋄ ⋄ ⋄"
}
artwork={
data?.artwork ||
"data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' width='100' height='100'%3E%3Crect fill='%23a3a3a3' width='100' height='100'/%3E%3Ctext x='50' y='50' font-family='monospace' font-size='40' text-anchor='middle' dy='.3em' fill='white'%3E♪%3C/text%3E%3C/svg%3E"
}
isPlaying={data?.isPlaying || false}
isLoading={isLoading}
autoExtractColor={!!data && !isLoading}
vibrantColor={
!data || isLoading ? "rgba(163, 163, 163, 0.6)" : undefined
}
textColor={!data || isLoading ? "white" : undefined}
/>
</div>
</button>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Reconsider the full-viewport button element for accessibility and semantics.

Wrapping the entire viewport in a <button> element creates several concerns:

  1. Semantic HTML: A button spanning the full screen with pointerEvents: none is semantically incorrect, even though pointer events are disabled on the outer element.

  2. Keyboard accessibility: The button remains focusable, which means keyboard users could tab to it and see a focus ring on the entire viewport, creating a confusing experience.

  3. Code consistency: Inline styles are used here instead of the Box component's css prop pattern used throughout the codebase.

♿ Proposed architectural improvements

Option 1: Non-interactive outer container (recommended)

+ import { Box } from "@httpjpg/ui";
+
  const content = (
-    <button
-      type="button"
-      onClick={handleClick}
-      style={{
-        all: "unset",
+    <Box
+      css={{
         position: "fixed",
         inset: 0,
         pointerEvents: "none",
         zIndex: zIndex.floating,
       }}
     >
-      <div style={{ pointerEvents: "auto" }}>
+      <Box
+        as="button"
+        type="button"
+        onClick={handleClick}
+        css={{ 
+          all: "unset",
+          pointerEvents: "auto",
+          cursor: "pointer",
+        }}
+      >
         <NowPlaying
           title={isLoading ? "Loading..." : data?.title || "╱╱ #welovemusic ╱╱"}
           artist={
             isLoading ? "..." : data?.artist || "⋄ ⋄ ⋄ (spotify(none)) ⋄ ⋄ ⋄"
           }
           {/* ...rest of props... */}
         />
-      </div>
-    </button>
+      </Box>
+    </Box>
   );

This approach:

  • Uses a non-interactive Box for the viewport overlay
  • Makes only the NowPlaying widget area interactive and focusable
  • Follows the codebase's Box component pattern
  • Maintains proper keyboard navigation semantics
📝 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.

Suggested change
<button
type="button"
onClick={handleClick}
style={{
all: "unset",
position: "fixed",
inset: 0,
pointerEvents: "none",
zIndex: zIndex.floating,
}}
>
<div style={{ pointerEvents: "auto" }}>
<NowPlaying
title={isLoading ? "Loading..." : data?.title || "╱╱ #welovemusic ╱╱"}
artist={
isLoading ? "..." : data?.artist || "⋄ ⋄ ⋄ (spotify(none)) ⋄ ⋄ ⋄"
}
artwork={
data?.artwork ||
"data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' width='100' height='100'%3E%3Crect fill='%23a3a3a3' width='100' height='100'/%3E%3Ctext x='50' y='50' font-family='monospace' font-size='40' text-anchor='middle' dy='.3em' fill='white'%3E♪%3C/text%3E%3C/svg%3E"
}
isPlaying={data?.isPlaying || false}
isLoading={isLoading}
autoExtractColor={!!data && !isLoading}
vibrantColor={
!data || isLoading ? "rgba(163, 163, 163, 0.6)" : undefined
}
textColor={!data || isLoading ? "white" : undefined}
/>
</div>
</button>
<Box
css={{
position: "fixed",
inset: 0,
pointerEvents: "none",
zIndex: zIndex.floating,
}}
>
<Box
as="button"
type="button"
onClick={handleClick}
css={{
all: "unset",
pointerEvents: "auto",
cursor: "pointer",
}}
>
<NowPlaying
title={isLoading ? "Loading..." : data?.title || "╱╱ #welovemusic ╱╱"}
artist={
isLoading ? "..." : data?.artist || "⋄ ⋄ ⋄ (spotify(none)) ⋄ ⋄ ⋄"
}
artwork={
data?.artwork ||
"data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' width='100' height='100'%3E%3Crect fill='%23a3a3a3' width='100' height='100'/%3E%3Ctext x='50' y='50' font-family='monospace' font-size='40' text-anchor='middle' dy='.3em' fill='white'%3E♪%3C/text%3E%3C/svg%3E"
}
isPlaying={data?.isPlaying || false}
isLoading={isLoading}
autoExtractColor={!!data && !isLoading}
vibrantColor={
!data || isLoading ? "rgba(163, 163, 163, 0.6)" : undefined
}
textColor={!data || isLoading ? "white" : undefined}
/>
</Box>
</Box>
🤖 Prompt for AI Agents
In @apps/web/components/widgets/now-playing-widget.tsx around lines 35 - 65, The
outer full-viewport <button> (with onClick=handleClick) is semantically and
accessibly wrong; replace it with a non-interactive Box (or div) that uses the
component's css prop for the overlay positioning/zIndex instead of inline
styles, move the onClick/handleClick and focusable behavior to the inner
interactive container that wraps NowPlaying (give that wrapper tabIndex=0 or
make it a real button/anchor and keep pointerEvents enabled), ensure only the
NowPlaying area is keyboard-focusable (no giant focus ring on the viewport),
preserve zIndex/inset positioning and the NowPlaying props
(title/artist/artwork/isPlaying/isLoading/autoExtractColor/vibrantColor/textColor)
and remove pointerEvents: "none" from the outer element.

Comment on lines +320 to +322
<Box css={{ zIndex: 20 }}>
{" "}
{/* @httpjpg/tokens zIndex.docked */}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Incomplete token migration.

The comment indicates this should use zIndex.docked from @httpjpg/tokens, but the implementation still uses the hardcoded value 20. To complete the migration to the centralized token system, import and use the token directly.

♻️ Complete the token migration

Add the import at the top of the file:

+"use client";
+
+import { zIndex } from "@httpjpg/tokens";
 import type { ReactNode } from "react";

Then apply this change to use the token:

-        <Box css={{ zIndex: 20 }}>
-          {" "}
-          {/* @httpjpg/tokens zIndex.docked */}
+        <Box css={{ zIndex: zIndex.docked }}>
           <Box
🤖 Prompt for AI Agents
In @packages/ui/src/components/work-card/work-card.tsx around lines 320 - 322,
The Box currently uses a hardcoded zIndex value instead of the centralized
token; import zIndex from "@httpjpg/tokens" and replace the hardcoded value in
the Box css prop with zIndex.docked (update the import section and change the
Box css={{ zIndex: 20 }} to use zIndex.docked) so the component (work-card.tsx,
Box) uses the tokenized value.

Comment on lines +127 to +132
<>
<WorkCard key={work.slug || index} {...work} />
{showDividers && index < works.length - 1 && (
<Divider orientation="horizontal" {...dividerProps} />
)}
</VStack>
</>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

React key must be on the fragment, not the child element.

The key prop is currently on WorkCard, but React requires the key on the top-level element returned from .map(). Since a fragment is the outer element, it needs the key. The shorthand fragment syntax <>...</> doesn't support keys, so you must use the explicit React.Fragment syntax.

🔑 Proposed fix: Move key to React.Fragment
+import { Fragment } from "react";
+
 // ... rest of imports

         {works.map((work, index) => (
-          <>
-            <WorkCard key={work.slug || index} {...work} />
+          <Fragment key={work.slug || index}>
+            <WorkCard {...work} />
             {showDividers && index < works.length - 1 && (
               <Divider orientation="horizontal" {...dividerProps} />
             )}
-          </>
+          </Fragment>
         ))}
🤖 Prompt for AI Agents
In @packages/ui/src/components/work-list/work-list.tsx around lines 127 - 132,
The fragment returned around each mapped item currently uses the shorthand
<>...</> while the key is applied to the WorkCard; move the key to the top-level
fragment by replacing the shorthand with an explicit React.Fragment and give
that fragment the key (e.g., React.Fragment key={work.slug || index}); keep
WorkCard and Divider props unchanged and remove the key from WorkCard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant