Skip to content

fix(guest-list): wrap New Event button in li so ul only contains li children - #296

Draft
dccakes wants to merge 1 commit into
mainfrom
claude/sharp-feynman-gvk4b0
Draft

dccakes wants to merge 1 commit into
mainfrom
claude/sharp-feynman-gvk4b0

Conversation

@dccakes

@dccakes dccakes commented Sep 8, 2026

Copy link
Copy Markdown
Owner

Daily Product Improvement

Summary

Fixes an HTML validity and accessibility bug in EventsTabs (src/components/guest-list/event-tabs.tsx): the "+ New Event" <Button> was rendered as a direct child of the <ul>, which the HTML spec does not allow — a <ul> may only contain <li> elements as direct children. The button is now wrapped in a <li>.


Recommendations Reviewed

Area: src/components/guest-list/event-tabs.tsx
Issue: <Button> ("+ New Event") rendered as a direct child of <ul>, violating the HTML spec. Browsers are forgiving but the ARIA list structure is broken for screen readers — assistive technology expects only list items as children, so the button appears outside the list in the accessibility tree.
Recommendation: Wrap the button in a <li> element.
Type: Frontend cleanup / Accessibility
Priority: Medium
Effort: Small


Area: src/app/(authenicated)/events/_components/event-card.tsx
Issue: When event.collectRsvp is true, an "RSVPs" <Badge> is shown in the card header alongside the "Collect RSVPs" toggle — these two elements always reflect the same state, making the badge purely redundant.
Recommendation: Remove the badge; the Switch state already communicates whether RSVPs are collected.
Type: UX/UI
Priority: Low
Effort: Small


Area: src/app/(authenicated)/events/_components/events-page-client.tsx
Issue: The "General RSVP questions" button is disabled={websiteQuestions === null} while the query is loading, with no tooltip or spinner to explain why. Users see an unresponsive button with no indication it will become available.
Recommendation: Show a loading spinner or a title tooltip on the button while the query resolves.
Type: UX/UI
Priority: Medium
Effort: Small


Area: src/app/(authenicated)/events/_components/events-page-client.tsx (lines 237–244)
Issue: The if (isLoading && initialEvents.length === 0) branch is unreachable. When initialData is provided to a tRPC query, isLoading is never true on the first render — the query starts satisfied. Future developers may rely on this branch and be surprised.
Recommendation: Remove the dead branch.
Type: Frontend cleanup
Priority: Low
Effort: Small


Area: src/app/(authenicated)/vendors/page.tsx (lines 27–29)
Issue: if (vendors === null) { redirect('/') } is unreachable — the catch block already redirects, so this path is only reached after a successful assignment.
Recommendation: Remove the dead null check.
Type: Frontend cleanup
Priority: Low
Effort: Small


Area: src/app/(authenicated)/vendors/page.tsx and src/app/(authenicated)/budget/page.tsx
Issue: Both pages catch all errors (including transient network failures) and redirect silently to /. Users lose context with no explanation.
Recommendation: Use the route-level error.tsx for unexpected errors rather than a silent redirect.
Type: UX/UI / Backend cleanup
Priority: Medium
Effort: Medium


Selected Improvement

Wrap the "+ New Event" <Button> in a <li> in EventsTabs.

Why This Was Selected

  • The HTML spec requires <ul> to contain only <li> elements as direct children. The button was the sole violator in this component.
  • Screen readers announce <ul> contents as a list; a bare <button> among <li> elements breaks that structure in the accessibility tree.
  • The fix is a two-line wrapper with zero behavior change.
  • No other open PR covers this component.

Changes Made

  • Wrapped the <Button variant='ghost' size='sm'>+ New Event</Button> in <li> in EventsTabs.
  • No styling changes were needed — the parent <ul> is flex items-center gap-5, so the <li> naturally becomes a flex item.

Files Changed

  • src/components/guest-list/event-tabs.tsx

Verification

  • Biome lint: no errors, no new warnings on the changed file.
  • TypeScript: no new type errors (pre-existing baseUrl deprecation warning unrelated to this change).
  • Jest: all 2080 tests passed.
  • Diff reviewed — only the <li> wrapper added; all button props and styling unchanged.

Future Recommendations

Recommendation: Remove redundant "RSVPs" badge from EventCard — the Collect RSVPs toggle already communicates the same state.
Priority: Low
Effort: Small
Reason not included: Low priority, different area; keeps this PR focused.
Should become GitHub issue: No

Recommendation: Add loading indicator to "General RSVP questions" button while websiteQuestions is loading.
Priority: Medium
Effort: Small
Reason not included: Different component and concern; better reviewed separately.
Should become GitHub issue: No

Recommendation: Remove unreachable isLoading branch in events-page-client.tsx.
Priority: Low
Effort: Small
Reason not included: Low priority dead-code cleanup; small enough for a follow-up daily run.
Should become GitHub issue: No

Recommendation: Fix silent redirect-on-catch in vendors and budget page server components — use error.tsx instead.
Priority: Medium
Effort: Medium
Reason not included: Touches error-handling architecture across two pages; warrants its own focused review.
Should become GitHub issue: Yes

GitHub Issues Created or Proposed

No issues created in this run. The silent-redirect-on-catch pattern in vendors and budget pages is Medium priority and Medium effort — it warrants a tracked issue, but requires a decision on the preferred error handling strategy (error.tsx vs. toast vs. inline error state) before implementing.


🤖 Generated with Claude Code

https://claude.ai/code/session_01XDWDzCQ5ob6jVrz8tEpqrX


Generated by Claude Code

…hildren

The "New Event" Button rendered as a direct child of the ul in EventsTabs,
violating the HTML spec (ul may only contain li elements as direct children)
and breaking the accessibility tree for screen readers. Wrap it in a li to
restore valid HTML structure.

Co-Authored-By: AgenticDiego <noreply@carvallo.io>
Claude-Session: https://claude.ai/code/session_01XDWDzCQ5ob6jVrz8tEpqrX
@vercel

vercel Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

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

Project Deployment Actions Updated
scv Error Error Sep 8, 2026 2:22pm UTC

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Jest Test Coverage

Coverage Summary

Lines Statements Branches Functions
Coverage: 83%
83.11% (36264/43629) 82.14% (3813/4642) 69.49% (1073/1544)

This branch had an error being deployed

1 failed deployment
Preview — 7cb012b7 Deployed Sep 8, 2026 by vercel[bot]
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.

2 participants