Conversation
WalkthroughRestores the Create Wave modal on the Discover page, adds ticket TKT-0021 to codex state, replaces Link-based navigation in WaveItem and WaveItemDropped with button/handler-driven routing (supporting modifier/middle-click), adds a next/navigation mock in tests, and sets an experimental MCP server env var for dev. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Waves as Waves.tsx
participant ModalState as Modal State
participant CreateWaveModal
participant CreateDMModal
User->>Waves: Open Discover page
Waves->>ModalState: read isWaveModalOpen, isDirectMessageModalOpen
alt isWaveModalOpen
Waves->>CreateWaveModal: Render (isWaveModalOpen=true)
User->>CreateWaveModal: Interact (open/close/form)
else isDirectMessageModalOpen
Waves->>CreateDMModal: Render (isDirectMessageModalOpen=true)
User->>CreateDMModal: Interact
else
Waves->>Waves: Render main content only
end
sequenceDiagram
participant User
participant WaveItem as WaveItem.tsx
participant Router
User->>WaveItem: Click / middle-click / modifier-click
alt Standard click (no modifiers)
WaveItem->>Router: router.push(waveUrl)
else Middle-click or Ctrl/Meta
WaveItem->>WaveItem: window.open(waveUrl, "_blank")
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
__tests__/components/waves/list/WaveItemDropped.test.tsx (1)
5-5: Router mock looks good; consider centralizing and preparing for interaction assertions.This aligns with the new router.push-based navigation. Optional: move this mock to a shared test util and add a window.open mock for future click/aux-click tests. You can also drop the unused next/link mock in this file.
components/waves/list/WaveItemDropped.tsx (2)
11-25: Support Shift+Click to match link semantics.Buttons don’t open a new tab on Shift+Click. Add shiftKey to match anchor behavior.
Apply this diff:
- if (event.metaKey || event.ctrlKey) { + if (event.metaKey || event.ctrlKey || event.shiftKey) { event.preventDefault(); event.stopPropagation(); window.open(href, "_blank", "noopener,noreferrer"); return; }Optionally, extract this open-in-new-tab check into a shared helper to reuse across WaveItem and here.
27-38: Verify onAuxClick support in target browsers.onAuxClick isn’t uniformly fired in some older browsers. If you need widest support, add a middle-button check on onMouseDown/onMouseUp as a fallback.
components/waves/list/WaveItem.tsx (3)
178-194: Match full link semantics and dedupe navigation helpers.
- Add Shift+Click support like anchors.
- Logic duplicates WaveItemDropped; consider a small shared util (e.g., openInNewTabIfModifier(event, href)).
Apply this diff:
- if (event.metaKey || event.ctrlKey) { + if (event.metaKey || event.ctrlKey || event.shiftKey) { event.preventDefault(); event.stopPropagation(); window.open(authorHref, "_blank", "noopener,noreferrer"); return; }Also applies to: 196-206
220-238: Avoid nesting interactive controls inside a Link (invalid HTML, a11y risk).CardContainer renders a Link around the entire card; this inserts a as a descendant of an , which HTML disallows (interactive descendants). Screen readers may announce unpredictably, and some browsers may mis-handle events.
Recommended restructure (non-breaking UX):
- Render CardContainer as a non-link container (
) when it contains nested interactive children, and handle meta/ctrl/middle in code; or- Use an absolutely-positioned Link overlay as a sibling inside the container, with interactive children above it (z-index), avoiding nesting.
If you go with the first approach, adjust handleCardClick to open a new tab when no anchor is present:
- if (event.button === 1 || event.metaKey || event.ctrlKey) { - return; - } + if (event.button === 1 || event.metaKey || event.ctrlKey || event.shiftKey) { + if (!(event.currentTarget instanceof HTMLAnchorElement)) { + event.preventDefault(); + event.stopPropagation(); + window.open(waveHref, "_blank", "noopener,noreferrer"); + } + return; + }I can draft a small CardContainer variant that switches to
when nested interactive content is present, if helpful.
321-323: Non-link title is fine; card remains the primary tap target.Minor note: the file still uses Heroicons. Our TSX guideline prefers FontAwesome; consider standardizing icons in a follow-up across components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
__tests__/components/waves/list/WaveItemDropped.test.tsx(1 hunks)codex/tickets/TKT-0021.md(1 hunks)components/waves/list/WaveItem.tsx(5 hunks)components/waves/list/WaveItemDropped.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- codex/tickets/TKT-0021.md
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Do not include any comments in the code
Use react-query for data fetching
Always add readonly before propsUse TypeScript across the codebase
Files:
components/waves/list/WaveItemDropped.tsxcomponents/waves/list/WaveItem.tsx__tests__/components/waves/list/WaveItemDropped.test.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks for UI components
Files:
components/waves/list/WaveItemDropped.tsxcomponents/waves/list/WaveItem.tsx__tests__/components/waves/list/WaveItemDropped.test.tsx
__tests__/**
📄 CodeRabbit inference engine (tests/AGENTS.md)
Place Jest test suites under the
__tests__directory mirroring source folders (e.g., components, contexts, hooks, utils)
Files:
__tests__/components/waves/list/WaveItemDropped.test.tsx
__tests__/components/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (tests/AGENTS.md)
Use
@testing-library/reactand@testing-library/user-eventfor React component tests
Files:
__tests__/components/waves/list/WaveItemDropped.test.tsx
**/__tests__/**
📄 CodeRabbit inference engine (AGENTS.md)
Place tests in
__tests__directories when organizing test suites
Files:
__tests__/components/waves/list/WaveItemDropped.test.tsx
**/*.test.tsx
📄 CodeRabbit inference engine (AGENTS.md)
When colocating tests with components, name them
ComponentName.test.tsx
Files:
__tests__/components/waves/list/WaveItemDropped.test.tsx
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Mock external dependencies and APIs in tests
Files:
__tests__/components/waves/list/WaveItemDropped.test.tsx
🧬 Code graph analysis (1)
components/waves/list/WaveItemDropped.tsx (1)
generated/models/ApiWave.ts (1)
ApiWave(27-177)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
components/waves/list/WaveItemDropped.tsx (1)
79-93: A11y and interaction setup looks solid.Button semantics + aria-label + data-wave-item-interactive with preventDefault/stopPropagation is correct for nested interactive containment.
If you plan to add keyboard tests, ensure Space/Enter activate this button and do not trigger card navigation.
components/waves/list/WaveItem.tsx (2)
63-63: Underline removal via class is fine.tw-no-underline on the interactive card container keeps styling consistent without inline styles.
258-264: Good containment: stop navigation when clicking nested interactive.preventDefault + stopPropagation on interactive-child clicks avoids accidental card nav.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
components/waves/list/WaveItem.tsx (1)
178-206: Code duplication with WaveItemDropped.tsx handlers.These navigation handlers duplicate the logic in
WaveItemDropped.tsx(lines 11-38). See the earlier comment on that file for the suggested shared helper extraction.
🧹 Nitpick comments (1)
components/waves/list/WaveItemDropped.tsx (1)
11-38: Extract shared navigation handlers to reduce duplication.These click and auxiliary-click handlers are nearly identical to
handleAuthorClickandhandleAuthorAuxClickinWaveItem.tsx(lines 178-206). Both implement the same pattern: handle modifier keys for new tabs, middle-click for new tabs, and fallback torouter.push.Consider extracting a shared helper:
function createNavigationHandlers(router: ReturnType<typeof useRouter>, href: string) { const handleClick = useCallback( (event: MouseEvent<HTMLButtonElement>) => { if (event.metaKey || event.ctrlKey) { event.preventDefault(); event.stopPropagation(); window.open(href, "_blank", "noopener,noreferrer"); return; } event.preventDefault(); event.stopPropagation(); router.push(href); }, [href, router] ); const handleAuxClick = useCallback( (event: MouseEvent<HTMLButtonElement>) => { if (event.button !== 1) { return; } event.preventDefault(); event.stopPropagation(); window.open(href, "_blank", "noopener,noreferrer"); }, [href] ); return { handleClick, handleAuxClick }; }Then use it in both components:
const { handleClick, handleAuxClick } = createNavigationHandlers(router, contributorHref);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/waves/list/WaveItem.tsx(5 hunks)components/waves/list/WaveItemDropped.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Do not include any comments in the code
Use react-query for data fetching
Always add readonly before propsUse TypeScript across the codebase
Files:
components/waves/list/WaveItem.tsxcomponents/waves/list/WaveItemDropped.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks for UI components
Files:
components/waves/list/WaveItem.tsxcomponents/waves/list/WaveItemDropped.tsx
🧬 Code graph analysis (1)
components/waves/list/WaveItemDropped.tsx (1)
generated/models/ApiWave.ts (1)
ApiWave(27-177)
🔇 Additional comments (6)
components/waves/list/WaveItemDropped.tsx (2)
1-2: LGTM: Imports support the new navigation pattern.The React hooks and Next.js router imports are appropriate for the button-based navigation handlers added below.
Note: The missing "use client" directive has already been flagged in a previous review.
79-92: LGTM: Button-based navigation with proper accessibility.The button implementation correctly:
- Uses
type="button"to prevent form submission- Adds
data-wave-item-interactivefor parent click handler coordination- Includes descriptive
aria-label- Applies appropriate styling to remove default button appearance
components/waves/list/WaveItem.tsx (4)
63-63: LGTM: Removes default link underline.The
tw-no-underlineclass appropriately removes text decoration from the card link, maintaining clean visual styling.
220-237: LGTM: Author button with proper interaction handlers.The button-based author navigation correctly:
- Implements the same navigation pattern as contributor buttons
- Includes
data-wave-item-interactivefor parent handler coordination- Provides accessible labeling
- Maintains visual consistency with
text-leftalignment
257-264: LGTM: Proper delegation between nested interactive elements and card navigation.The updated logic correctly handles three scenarios:
- Nested interactive elements (lines 258-261): Prevents card navigation when clicking nested buttons, stopping event propagation
- Modifier/middle-clicks (line 262): Allows default Link behavior for opening in new tabs
- Normal clicks (lines 265-268): Uses router.push for in-app navigation
This coordination ensures nested buttons (author, contributors, follow) work independently while preserving the card's link functionality.
321-323: LGTM: Wave name as non-interactive element.Changing the wave name from a Link to a span simplifies the interaction model by consolidating navigation to the card level. The
group-hoverstyling still provides visual feedback, maintaining good UX.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/waves/list/WaveItemDropped.tsx (1)
81-94: Consider adding keyboard focus indicator.The interactive button lacks visible focus styles for keyboard navigation. Consider adding focus-visible styles to improve accessibility.
Apply this diff to add keyboard focus indicator:
<button type="button" data-wave-item-interactive="true" onClick={handleContributorClick(contributorHref)} onAuxClick={handleContributorAuxClick(contributorHref)} - className="tw-cursor-pointer tw-bg-transparent tw-border-none tw-p-0 tw-m-0 tw-inline-flex" + className="tw-cursor-pointer tw-bg-transparent tw-border-none tw-p-0 tw-m-0 tw-inline-flex focus-visible:tw-outline focus-visible:tw-outline-2 focus-visible:tw-outline-primary-400 focus-visible:tw-rounded-lg" aria-label={ c.contributor_identity ? `View @${c.contributor_identity}` : "View contributor profile" } >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
codex/tickets/TKT-0021.md(1 hunks)components/waves/list/WaveItemDropped.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- codex/tickets/TKT-0021.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Do not include any comments in the code
Use react-query for data fetching
Always add readonly before propsUse TypeScript across the codebase
Files:
components/waves/list/WaveItemDropped.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks for UI components
Files:
components/waves/list/WaveItemDropped.tsx
🧬 Code graph analysis (1)
components/waves/list/WaveItemDropped.tsx (1)
generated/models/ApiWave.ts (1)
ApiWave(27-177)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
components/waves/list/WaveItemDropped.tsx (3)
1-1: LGTM: "use client" directive properly added.The addition of the "use client" directive resolves the previous review concern and correctly enables client-side hooks (useRouter, useCallback) in the App Router.
13-27: LGTM: Click handler properly implements modifier-key navigation.The handleContributorClick implementation correctly:
- Detects modifier keys to open in new tab
- Prevents default behavior and stops propagation
- Uses router.push for standard navigation
- Is properly memoized with router dependency
29-40: LGTM: Middle-click handler properly implemented.The handleContributorAuxClick correctly handles middle-click (button === 1) navigation with appropriate event prevention and new-tab behavior using secure window.open parameters.
2 participants
Summary by CodeRabbit
Bug Fixes
New Features / Interaction
UI Changes
Documentation
Tests