Skip to content

refactor(components): inline Item composition and enforce component boundaries#289

Merged
andrewck24 merged 12 commits intodevfrom
refactor/component-architecture
Mar 31, 2026
Merged

refactor(components): inline Item composition and enforce component boundaries#289
andrewck24 merged 12 commits intodevfrom
refactor/component-architecture

Conversation

@andrewck24
Copy link
Copy Markdown
Owner

@andrewck24 andrewck24 commented Mar 27, 2026

Summary

This PR finishes the component-architecture refactor around the actual landed shape, not the earlier wrapper-based plan.

  • Install and adopt Shadcn Item as the base row primitive in src/components/ui/item.tsx
  • Remove the PersonItem and TeamItem wrapper layer, and compose Item primitives directly in players, roster, menu, and invitations
  • Add src/components/ui/avatar.tsx plus ItemAvatar for the only shared domain-agnostic avatar boilerplate
  • Keep the invitations row fully navigable except for the inline action buttons, and document that overlay-link pattern in docs/architecture.md
  • Relocate LoadingCourt, panel, and flip-words, and delete the unused sheet.tsx
  • Replace the old custom/list-item Storybook coverage with src/stories/item.stories.tsx
  • Sync the archived component-architecture artifacts and related storybook-modernization docs to match the final implementation
  • Clean up the local Spectra command and skill files that were updated alongside this branch

Review Fixes

  • Fix the invalid Tailwind arbitrary variant selector in ui/item.tsx
  • Restore the missing React import in ui/skeleton.tsx
  • Add inline per-item error feedback for failed invitation rejection
  • Align the menu loading placeholder with the shared Skeleton component
  • Document the overlay-link item form so the invitations pattern is explicit instead of implicit

Verification

  • npm test
  • npx tsc --noEmit
  • npm run lint
  • npm run build

摘要

這個 PR 完成 component-architecture 重構的最終落地版本,而不是早期保留 wrapper 的方案。

  • 以 Shadcn Item 作為基礎 row primitive
  • 移除 PersonItem / TeamItem wrapper,改由各 consumer 直接組合 Item primitives
  • 新增 src/components/ui/avatar.tsxItemAvatar,只保留通用 avatar boilerplate
  • 保留 invitations 的 overlay-link pattern,讓整列可導頁但 action buttons 仍可單獨操作,並在 docs/architecture.md 明確記錄
  • 搬移 LoadingCourtpanelflip-words,刪除未使用的 sheet.tsx
  • 將 Storybook coverage 改為 src/stories/item.stories.tsx
  • 同步 archived component-architecturestorybook-modernization 文件內容,讓規格與實作一致
  • 一併整理這個 branch 內更新過的 Spectra command / skill 檔案

- Install Shadcn Item via npx shadcn@latest add item to src/components/ui/item.tsx
- Remove custom list-item compound component (superseded by Shadcn Item)
- Write Storybook stories for Shadcn Item covering two item forms,
  action footer pattern, variant/size combinations
…tives

- Rewrite PersonItem as thin wrapper over ItemMedia/ItemContent/ItemTitle
- Remove href/onClick/action props; consumers use Item asChild for navigation
- Add co-located PersonItemSkeleton export with data-testid markers
- Install shadcn Skeleton component
- Update players/list.tsx to navigable form (Item asChild + Link)
- Update roster-list.tsx to static with actions form (Item + ItemActions)
- Rewrite tests: data-testid for structure queries, Link instead of <a>
- Rewrite TeamItem as thin wrapper over ItemMedia/ItemContent/ItemTitle
- Remove href/onClick/action/className props; consumers use Item asChild
- Add co-located TeamItemSkeleton export with data-testid markers
- Update invitations to action footer pattern (Item asChild + accept/reject footer)
- Update menu to navigable form (Item asChild + button with className)
- Rewrite tests: data-testid for structure queries, Link/button via asChild
- Move LoadingCourt from custom/loading/court.tsx to custom/court/index.tsx
- Export as named export alongside other court primitives
- Update team/lineup and record/index imports accordingly
- Delete custom/loading/court.tsx
- Delete unused ui/sheet.tsx (zero consumers)
- Relocate ui/panel.tsx to custom/panel/index.tsx, update 9 import paths
- Relocate ui/flip-words.tsx to landing/flip-words.tsx, update hero.tsx import
- Fix hero.test.tsx mock path for flip-words relocation
- Create docs/architecture.md with layer boundary table (ui/custom/domain)
- Document testing conventions: data-testid usage, Link vs <a>, no container.querySelector
- Note iterative expansion policy for future changes
Move person-item and team-item to custom/list-item/ subdirectory,
mirror test structure under custom/__tests__/list-item/, and update
all consumer import paths.

Also use Array.from loop for LoadingCourt placeholder cards for
consistency with RecordCourt pattern, and update architecture docs
to reflect subdirectory grouping convention.
@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 27, 2026

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

Project Deployment Actions Updated (UTC)
volleybro Ready Ready Preview, Comment Mar 31, 2026 7:13pm

@claude
Copy link
Copy Markdown

claude Bot commented Mar 27, 2026

Claude finished @andrewck24's task in 3m 12s —— View job


PR Review: component architecture refactor

  • Gather context & understand request
  • Review ui/item.tsx — new Shadcn Item primitive
  • Review custom/list-item/person-item.tsx & team-item.tsx
  • Review consumer updates (user/invitations, user/menu, team/players/list, record/new/roster-list)
  • Review test files
  • Review Storybook stories
  • Review architecture docs
  • Post final review

Overall this is a well-structured refactor. The compound component pattern for Item, the asChild polymorphism, and the two-form consumer convention (navigable vs. static-with-actions) are clean and consistent. Test coverage including axe a11y checks is excellent. A few issues worth addressing before merge:


Issues

1. Invalid Tailwind arbitrary-variant syntax — ui/item.tsx:34 (likely a bug)

"... [a]:transition-colors [a]:hover:bg-accent/50"

Tailwind arbitrary variants require & to reference the element itself, and _ for descendant selectors. [a]: is not valid Tailwind syntax and will generate no CSS. The intent appears to be styling anchor descendants of Item on hover.

Should be:

"... [&_a]:transition-colors [&_a]:hover:bg-accent/50"

Fix this →


2. Missing React import in ui/skeleton.tsx — TypeScript error

skeleton.tsx uses React.HTMLAttributes<HTMLDivElement> (line 3) without importing React. All other ui/ components use import * as React from "react". With the project's tsconfig.json types array not including a global React reference, npx tsc --noEmit should flag this.

// skeleton.tsx — add at the top
import * as React from "react"

Fix this →


3. TeamItem loading state uses raw Tailwind instead of <Skeleton>custom/list-item/team-item.tsx:27–30

// team-item.tsx — inconsistent loading state
<span
 
  className="inline-block h-4 w-24 animate-pulse rounded bg-muted"
/>

PersonItemSkeleton and TeamItemSkeleton both use the <Skeleton> component (which is already imported in this file). The inline loading state inside TeamItem bypasses that abstraction, producing a slightly different pulse color (bg-muted vs. bg-primary/10 in Skeleton).


4. handleReject silently drops per-item errors — user/invitations/index.tsx:54–67

handleAccept stores error messages in errorMap and renders them inline per invitation row. handleReject only calls showErrorToast on failure — if the rejection API call fails, the user has no per-item feedback and the invitation row stays on-screen without indication of what went wrong.

const handleReject = async (playerId: string): Promise<void> => {
  try { ... }
  catch (err) {
    // Missing: setErrorMap((prev) => ({ ...prev, [playerId]: getErrorMessage(err) }));
    showErrorToast(err, toast);
  }
};

Nits

ItemHeader and ItemFooter have identical class strings (flex basis-full items-center justify-between gap-2). If they're intentionally the same right now but semantically distinct, a comment explaining the distinction would help future contributors avoid collapsing them.

TeamItem test gap: No test for team?.name being undefined after loading completes (i.e., team fetch resolves but name is missing). The component would render an empty ItemTitle, which is silent. Worth a test case.


What's good

  • Compound component pattern and asChild polymorphism are correctly applied
  • Two-form convention (navigable asChild + Link vs. static Item + ItemActions) is well-documented and consistently followed across all 4 updated consumers
  • Axe accessibility tests on both forms — good practice
  • data-testid markers on structural skeleton elements
  • ActionFooterPattern story documents the invitations UX clearly
  • docs/architecture.md layer boundary table is a useful reference
  • LoadingCourt relocation into custom/court/index.tsx as a named export alongside Court is a clean colocation

- Deleted the Spectra: Discuss, Ingest, and Propose commands as they are no longer needed.
- Updated the Spectra Apply skill to include a preflight check for missing files and adjusted the configuration file references from `openspec/config.yaml` to `.spectra.yaml`.
- Enhanced the Spectra Ask skill description for clarity.
- Revised the Spectra Debug skill to reference the new configuration file.
- Improved the Spectra Discuss skill to include a structured approach for discussions based on codebase scouting.
- Adjusted the Spectra Ingest and Propose skills to reference the new configuration file for parallel task markers.
Remove the PersonItem and TeamItem wrapper layer along with the related tests and legacy list-item story.

Compose Item primitives directly in players, roster, menu, and invitations, while keeping the invitation overlay-link pattern so the row stays clickable except for the action buttons.

Add the shared avatar primitive, move Storybook coverage to item.stories.tsx, and sync the archived component-architecture and storybook-modernization docs to match the final implementation.
@claude
Copy link
Copy Markdown

claude Bot commented Mar 31, 2026

Claude finished @andrewck24's task in 3m 18s —— View job


PR Review: component architecture refactor

  • Gather context & understand request
  • Review ui/item.tsx — Tailwind syntax & compound component
  • Review ui/skeleton.tsx — React import
  • Review custom/list-item/ — components inlined into consumers (per architecture docs)
  • Review user/invitations/index.tsx — error handling
  • Review consumer updates, test files, and stories
  • Post final review

The overall refactor is clean — compound component pattern, asChild polymorphism, and consumer colocation are well-executed. Three issues from the previous review remain unaddressed, and I found two additional points.


Unresolved from previous review

1. Invalid Tailwind arbitrary-variant syntax — ui/item.tsx:35 (still broken)

"... [a]:transition-colors [a]:hover:bg-accent/50"

[a]: is not valid Tailwind arbitrary-variant syntax and generates no CSS. The correct form uses & to anchor the selector:

"... [&_a]:transition-colors [&_a]:hover:bg-accent/50"

Fix this →


2. Missing React import — ui/skeleton.tsx:6 (still broken)

React.HTMLAttributes<HTMLDivElement> is used on line 6 without importing React. Every other ui/ component uses import * as React from "react".

// Add at the top of skeleton.tsx
import * as React from "react"

Fix this →


3. handleReject missing per-item error feedback — user/invitations/index.tsx:82–95 (still present)

handleAccept writes to errorMap on failure; handleReject only calls showErrorToast. If rejection fails, the row stays on screen with no inline indication.

catch (err) {
  // Missing: setErrorMap((prev) => ({ ...prev, [playerId]: getErrorMessage(err) }));
  showErrorToast(err, toast);
}

Fix this →


New findings

4. InvitationItem uses an undocumented third navigation pattern — user/invitations/index.tsx:137–176

The component puts an absolute inset-0 z-0 link behind action buttons with relative z-10. This is a third form not covered by the two-form convention in docs/architecture.md, and it's fragile: clickability of the buttons depends entirely on z-index stacking, and hover state on the item fires when the pointer is over any button (potentially triggering the link's hover style while the user is focused on the button).

The InvitationOverlayPattern story documents the pattern visually, but docs/architecture.md only describes the two canonical forms. Either document this as an explicit "third form for items that need both navigation and inline actions" with the z-index contract, or reconsider the structure (e.g., navigate on team name click only, without the full-item link overlay).

5. Raw animate-pulse span in user/menu/index.tsx:42 — minor inconsistency

<span className="h-6 w-[16rem] animate-pulse rounded-md bg-muted" />

Every other loading state in this PR uses <Skeleton>. This one uses bg-muted directly, which produces a different shade from Skeleton's bg-primary/10. Low priority, but worth aligning if Skeleton's color is intentional.


Resolved since last review

  • TeamItem loading stateuser/menu/index.tsx:147–150 now uses <Skeleton> correctly ✓
  • ItemHeader / ItemFooter class strings — now intentionally differ (justify-between vs justify-start) ✓

What's good

  • Component inlining per docs/architecture.md "Item Composition Rule" is well-reasoned — removes thin wrapper layer without losing reuse
  • asChild polymorphism correctly applied across all consumers
  • ItemAvatar promoted to ui/item.tsx as a domain-agnostic primitive is the right call
  • InvitationOverlayPattern story is a good safety net even if it's an unusual pattern
  • LoadingCourt colocation into custom/court/index.tsx as a named export is clean
  • Architecture doc layer boundary table is useful

Correct the Tailwind anchor selector in ui/item.tsx and restore the missing React import in ui/skeleton.tsx.

Add inline reject error feedback for invitation rows, document the overlay-link item form in docs/architecture.md, and align the menu loading placeholder with the shared Skeleton component.
@claude
Copy link
Copy Markdown

claude Bot commented Mar 31, 2026

Claude finished @andrewck24's task in 2m 56s —— View job


PR Review: component architecture refactor

  • Gather context & understand request
  • Check ui/item.tsx — Tailwind arbitrary-variant syntax
  • Check ui/skeleton.tsx — React import
  • Check user/invitations/index.tsx — handleReject error handling & overlay pattern
  • Check user/menu/index.tsx — animate-pulse inconsistency
  • Post final review

All four issues from the previous review have been resolved. One new finding from this pass.


Resolved since last review

  • Tailwind arbitrary-variant syntax[&_a]:transition-colors [&_a]:hover:bg-accent/50 is now correct in ui/item.tsx:35
  • Missing React importimport * as React from "react" present at ui/skeleton.tsx:1
  • handleReject missing per-item error feedbacksetErrorMap call added in the catch block at user/invitations/index.tsx:93
  • Overlay-link pattern undocumenteddocs/architecture.md lines 28–34 now document the third form with its z-index contract ✓
  • Raw animate-pulse span in user/menu/index.tsx — replaced with <Skeleton>

New finding

Error message paragraph sits below the navigation overlay — user/invitations/index.tsx:171–175

The action buttons in ItemFooter escape the overlay via relative z-10, but the per-item error paragraph that appears below them does not:

{errorMap[player._id] && (
  <p className="px-3 pb-2 text-sm text-destructive">
    {errorMap[player._id]}
  </p>
)}

The NextLink overlay is absolute inset-0 z-0. Unpositioned elements sit below positioned elements in the same stacking context, so the overlay intercepts pointer events on this <p>. A user clicking the error message text would navigate to the team page instead of being able to interact with the text.

Add relative z-10 to keep it above the overlay, consistent with ItemFooter:

{errorMap[player._id] && (
  <p className="relative z-10 px-3 pb-2 text-sm text-destructive">
    {errorMap[player._id]}
  </p>
)}

Fix this →


What's good

All five previously flagged issues have been addressed cleanly. The overlay-link contract in docs/architecture.md is well-written and sufficient to guide future contributors. The error map symmetry between handleAccept and handleReject is now correct. This PR is in good shape to merge once the error paragraph stacking is fixed.

@andrewck24 andrewck24 changed the title refactor(components): component architecture — Shadcn Item as base primitive and layer boundary enforcement refactor(components): inline Item composition and enforce component boundaries Mar 31, 2026
… of toast

Remove errorMap state and inline <p> error messages from InvitationItem.
Errors are already handled by showErrorToast. Also delete unused
team/invitation-list.tsx and its tests (dead code).
@claude
Copy link
Copy Markdown

claude Bot commented Mar 31, 2026

Claude finished @andrewck24's task in 2m 9s —— View job


PR Review: component architecture refactor

  • Gather context & understand request
  • Check user/invitations/index.tsx — error paragraph z-index & error handling
  • Check ui/item.tsx & ui/skeleton.tsx — all previous issues
  • Post final review

All issues from prior reviews have been resolved. One note on the latest approach.


Resolved since last review

  • Error paragraph z-index — resolved by removing the errorMap pattern entirely; the conditional <p> no longer exists, so there's nothing to intercept clicks ✓
  • All five previously flagged issues from earlier rounds remain fixed ✓

Note on the latest commit (171a146)

The inline errorMap display was removed from handleAccept as well, making both handlers symmetric — both call showErrorToast on failure with no per-item state. The commit message ("remove duplicate inline error display in favor of toast") captures the intent clearly.

Trade-off: Toasts disappear automatically; if a user misses a toast or triggers errors on multiple rows quickly, they won't see persistent per-item feedback. For invitation accept/reject — a low-frequency, single-action flow — toast-only is a reasonable call. If the product ever needs persistent error feedback here, errorMap is straightforward to restore.

The symmetry between the two handlers is better than the previous asymmetric state. No objection.


Status

This PR is ready to merge. All previously identified issues are resolved, the compound component pattern and asChild polymorphism are correctly implemented, and the overlay-link contract is documented.


@andrewck24 andrewck24 merged commit 44960bc into dev Mar 31, 2026
7 checks passed
@andrewck24 andrewck24 deleted the refactor/component-architecture branch March 31, 2026 19:16
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 4, 2026

🎉 This PR is included in version 0.12.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant