Skip to content

fix: app-tab Rule-8 sweep — roster/stats/pipeline/scorecard/qualifier become real phone layouts #801

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
njrini99-code merged 1 commit into from
Jul 10, 2026
Merged

fix: app-tab Rule-8 sweep — roster/stats/pipeline/scorecard/qualifier become real phone layouts #801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/app/baseball/(dashboard)/dashboard/dev-plan/DevPlanClient.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,9 +559,15 @@ export default function DevPlanClient() {
</PaperCard>
) : null}

{/* Goals with Tabs */}
{/* Goals with Tabs. The four icon+label+badge triggers are
whitespace-nowrap and sum past the ~338px content width at
390px, and TabsList has no scroll handling of its own — so
below md the list rides a contained edge-bled scroller
(matching the page's px-4 shell) instead of overflowing the
page. md+ is untouched (overflow-visible, no bleed). */}
<Tabs defaultValue="active" value={activeTab} onChange={setActiveTab}>
<TabsList className="mb-4">
<div className="-mx-4 mb-4 overflow-x-auto px-4 [scrollbar-width:none] [&::-webkit-scrollbar]:hidden md:mx-0 md:overflow-visible md:px-0">
<TabsList>
Comment on lines +562 to +570

@coderabbitai coderabbitai Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Match the tab scroller to the responsive page shell (DevPlanClient.tsx:569).

The page switches to sm:px-6, but this wrapper remains -mx-4 px-4 until md. At 640–767px, the tabs therefore do not align with the shell edge. Add sm:-mx-6 sm:px-6.

As per path instructions, mobile screens must use consistent page padding and spacing.

Also applies to: 584-584

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/baseball/`(dashboard)/dashboard/dev-plan/DevPlanClient.tsx around
lines 562 - 570, Update the responsive tab scroller wrapper inside the Goals
Tabs section of DevPlanClient by adding sm:-mx-6 sm:px-6 alongside the existing
-mx-4 px-4 classes, preserving the md overrides. Apply the same responsive
padding adjustment to the related wrapper at the additionally referenced
location.

Source: Path instructions

<TabsTrigger value="active" icon={<IconTarget size={16} />} badge={activeCount > 0 ? activeCount : undefined}>
Active
</TabsTrigger>
Expand All @@ -575,6 +581,7 @@ export default function DevPlanClient() {
All
</TabsTrigger>
</TabsList>
</div>

<TabsContent value="active">
<GoalsList
Expand Down
199 changes: 192 additions & 7 deletions src/app/baseball/(dashboard)/dashboard/pipeline/PipelineClient.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ import { PlayerDetailModal } from '@/components/coach/PlayerDetailModal';
import { PlayerPeekPanel } from '@/components/panels/PlayerPeekPanel';
import { PositionPlanner } from '@/components/baseball/position-planner';
import { ConfirmDialog } from '@/components/ui/confirm-dialog';
import { IconUsers, IconLayoutGrid, IconList, IconTarget, IconTrash, IconTrendingUp, IconChevronDown, IconChevronUp } from '@/components/icons';
import { IconUsers, IconLayoutGrid, IconList, IconTarget, IconTrash, IconTrendingUp, IconChevronDown, IconChevronUp, IconChevronRight } from '@/components/icons';
import { useWatchlist } from '@/hooks/use-watchlist';
import { useAuth } from '@/hooks/use-auth';
import { useToast } from '@/components/ui/sonner';
Expand Down Expand Up @@ -537,10 +537,15 @@ function PipelineListRow({
onClick={onOpenPeek}
className="h-auto min-h-0 justify-start gap-3 rounded-fw-sm px-2 py-1 text-left font-normal"
>
<Avatar src={item.player?.avatar_url} name={name} size="md" />
<div className="min-w-0">
<span className="block truncate font-annual text-body-lg text-text-primary">{name}</span>
<span className="text-eyebrow text-text-tertiary">{item.player?.high_school_name || 'No school'}</span>
{/* ONE pre-composed flex child — Button wraps children in a bare
<span>, so Avatar + div as siblings stack vertically (see
Button's CHILDREN CONTRACT doc). Same pattern as the card row. */}
<div className="flex min-w-0 flex-1 items-center gap-3">
<Avatar src={item.player?.avatar_url} name={name} size="md" />
<div className="min-w-0">
<span className="block truncate font-annual text-body-lg text-text-primary">{name}</span>
<span className="text-eyebrow text-text-tertiary">{item.player?.high_school_name || 'No school'}</span>
</div>
</div>
</Button>
</td>
Expand Down Expand Up @@ -607,6 +612,147 @@ function PipelineListRow({
);
}

// Rule 8 (docs/MOBILE_DOCTRINE.md) — a `min-w` table wrapped only in
// `overflow-x-auto` is not the phone treatment on a reading surface. Below
// `lg` each row becomes a composed card (identity + position/grad-year +
// status + updated + tap-through/actions), mirroring the `hidden lg:block` /
// `lg:hidden` split already shipped in AcademicsClient.tsx:291/294 in this
// same app. The desktop `<table>` above stays byte-identical; this card
// consumes the exact same row props/callbacks — no new read/write path.
function PipelineListCard({
item,
focused,
selected,
editing,
noteValue,
onFocus,
onOpenPeek,
onToggleSelect,
onStatusChange,
onStartEditNote,
onNoteValueChange,
onSaveNote,
onCancelNote,
onViewProfile,
onRemove,
}: PipelineListRowProps) {
const name = getFullName(item.player?.first_name, item.player?.last_name);
const location = item.player?.city && item.player?.state ? `${item.player.city}, ${item.player.state}` : 'N/A';

return (
<PaperCard
onClick={onFocus}
className={cn(
'p-4',
pressableClass({ ink: 'pursuit', lift: true }),
focused && 'ring-2 ring-inset ring-pursuit',
)}
Comment on lines +643 to +649

@coderabbitai coderabbitai Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Do not make the card container a keyboardless clickable <div> (PipelineClient.tsx:643-649).

PaperCard defaults to a div, yet it receives onClick={onFocus} and has no keyboard semantics. Remove the wrapper click and invoke onFocus from the relevant controls, or provide an accessible interaction model that does not conflict with the nested controls.

As per path instructions, clickable non-button elements must support Enter/Space keyboard handling and visible focus behavior.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/baseball/`(dashboard)/dashboard/pipeline/PipelineClient.tsx around
lines 643 - 649, The PaperCard in the pipeline item is a clickable div without
keyboard semantics, conflicting with its nested controls. Remove
onClick={onFocus} from PaperCard and invoke onFocus from the relevant
interactive controls, or convert the container to an accessible interactive
element with Enter/Space handling and visible focus styling while preserving
nested-control behavior.

Source: Path instructions

>
<div className="flex items-start gap-3">
{/* Long-press multi-select isn't available on a card row — a compact,
always-visible checkbox keeps single-tap selection reachable. */}
<Checkbox checked={selected} onCheckedChange={onToggleSelect} aria-label={`Select ${name}`} className="mt-1 shrink-0" />
<Button
variant="ghost"
onClick={onOpenPeek}
rightIcon={<IconChevronRight size={16} aria-hidden className="text-text-tertiary" />}
className="h-auto min-h-0 flex-1 justify-between gap-3 rounded-fw-sm px-2 py-1 text-left font-normal"
>
{/* Fairway's <Button> renders non-icon children inside a single bare
<span> (no className) as a direct child of its inline-flex
container — that span gets blockified, and an inline Avatar
followed by a block-level name/school div inside it triggers
anonymous-block-box generation (Avatar stacks above the name
instead of beside it). An explicit flex wrapper here establishes
its own flex formatting context so Avatar + text lay out as a
real identity row, matching the Avatar+name idiom used elsewhere
(MessagesClient.tsx, AcademicsClient.tsx) instead of relying on
the Button's auto-wrapping children slot. */}
<div className="flex min-w-0 flex-1 items-center gap-3">
<Avatar src={item.player?.avatar_url} name={name} size="md" />
<div className="min-w-0 flex-1">
<span className="block truncate font-annual text-body-lg text-text-primary">{name}</span>
<span className="block truncate text-eyebrow text-text-tertiary">{item.player?.high_school_name || 'No school'}</span>
</div>
</div>
</Button>
Comment on lines +655 to +678

@coderabbitai coderabbitai Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Let nested controls own Enter/Space (PipelineClient.tsx:655-678,738-750).

The document handler at PipelineClient.tsx:1028-1035 excludes only input, textarea, and select elements. It therefore intercepts Enter/Space on these new buttons and checkboxes, opening the peek panel or toggling row selection instead of activating the focused control. Ignore events inside button, a, [role="button"], and editable elements before handling list navigation.

As per path instructions, components handling user input must preserve normal keyboard activation for interactive controls.

Also applies to: 738-750

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/baseball/`(dashboard)/dashboard/pipeline/PipelineClient.tsx around
lines 655 - 678, Update the document-level keyboard handler near the list
navigation logic to return early when the event target is inside a button, link,
element with role="button", or an editable control, in addition to the existing
input, textarea, and select checks. Use this guard before handling Enter/Space
so nested controls in the PipelineClient button and checkbox sections retain
their native keyboard activation.

Source: Path instructions

</div>

<div className="mt-3 flex flex-wrap items-center gap-x-2 gap-y-1 font-annual text-body-sm text-text-secondary">
<span>{item.player?.primary_position || '—'}</span>
<span aria-hidden className="text-text-tertiary">·</span>
<span>{item.player?.grad_year ?? '—'}</span>
<span aria-hidden className="text-text-tertiary">·</span>
<span className="truncate">{location}</span>
</div>

<HairlineRule ink="hairline" className="my-3" />

<div className="flex items-center justify-between gap-3">
<div className="w-40">
<Select
size="sm"
aria-label={`Change stage for ${name}`}
value={item.pipeline_stage ?? 'watchlist'}
onValueChange={(v) => v && onStatusChange(v as PipelineStage)}
options={statusOptions}
/>
</div>
<span className="shrink-0 font-annual text-eyebrow uppercase tracking-[0.1em] text-text-tertiary">
{formatDate(item.updated_at)}
</span>
</div>

<div className="mt-3">
{editing ? (
<div className="space-y-2">
<TextArea
size="sm"
value={noteValue}
onChange={(e) => onNoteValueChange(e.target.value)}
placeholder="Add notes about this player…"
rows={3}
className="w-full resize-none focus-visible:ring-pursuit"
/>
<div className="flex gap-2">
<Button size="sm" onClick={onSaveNote} className="min-h-[44px] flex-1">
Save
</Button>
<Button variant="ghost" size="sm" onClick={onCancelNote} className="min-h-[44px] flex-1">
Cancel
</Button>
Comment on lines +718 to +723

@coderabbitai coderabbitai Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use shared touch-target sizes instead of arbitrary heights (PipelineClient.tsx:718-723,738-750).

Replace min-h-[44px] with the existing shared Button/IconButton size contract so mobile controls do not introduce one-off control heights.

As per path instructions, do not introduce one-off spacing, radius, icon sizes, or control heights.

Also applies to: 738-750

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/baseball/`(dashboard)/dashboard/pipeline/PipelineClient.tsx around
lines 718 - 723, Replace the arbitrary min-h-[44px] classes on the Save, Cancel,
and controls referenced around the note actions in PipelineClient with the
existing shared Button/IconButton size variants or contract. Remove one-off
control height styling while preserving the intended mobile touch target and
layout.

Source: Path instructions

</div>
</div>
) : (
<Button
variant="ghost"
size="sm"
onClick={onStartEditNote}
className="h-auto min-h-0 w-full justify-start truncate px-2 py-1 text-left font-normal underline decoration-[color:var(--hairline)] underline-offset-2"
>
{item.notes ? (item.notes.length > 48 ? `${item.notes.slice(0, 48)}…` : item.notes) : 'Add note'}
</Button>
)}
</div>

<div className="mt-3 flex items-center gap-2">
<Button variant="secondary" size="sm" onClick={onViewProfile} className="min-h-[44px] flex-1">
View profile
</Button>
<IconButton
variant="danger"
size="sm"
aria-label={`Remove ${name} from pipeline`}
onClick={onRemove}
className="min-h-[44px] min-w-[44px]"
>
<IconTrash size={16} />
</IconButton>
</div>
</PaperCard>
);
}

// ─── Page ───────────────────────────────────────────────────────────────────

export default function PipelinePage() {
Expand Down Expand Up @@ -1173,14 +1319,53 @@ export default function PipelinePage() {
/>
) : (
<div className="space-y-3">
<p className="text-eyebrow uppercase tracking-[0.14em] text-text-tertiary">
<p className="hidden font-annual text-eyebrow uppercase tracking-[0.14em] text-text-tertiary lg:block">
<kbd className="rounded-fw-sm border border-[color:var(--hairline)] bg-surface-sunken px-1.5 py-0.5 font-mono text-text-secondary">j</kbd>/
<kbd className="rounded-fw-sm border border-[color:var(--hairline)] bg-surface-sunken px-1.5 py-0.5 font-mono text-text-secondary">k</kbd> navigate ·{' '}
<kbd className="rounded-fw-sm border border-[color:var(--hairline)] bg-surface-sunken px-1.5 py-0.5 font-mono text-text-secondary">Enter</kbd> view ·{' '}
<kbd className="rounded-fw-sm border border-[color:var(--hairline)] bg-surface-sunken px-1.5 py-0.5 font-mono text-text-secondary">x</kbd> select
</p>

<PaperCard className="overflow-hidden p-0">
{/* Rule 8 — cards below `lg`, byte-identical table at `lg`+. The
thead's "select all" checkbox has no card-list equivalent, so
a lightweight select-all/deselect-all row replaces it here
rather than dropping the capability on phone. */}
<div className="flex items-center justify-between gap-3 lg:hidden">
<span className="font-annual text-eyebrow uppercase tracking-[0.14em] text-text-tertiary">
{filteredWatchlist.length} player{filteredWatchlist.length !== 1 ? 's' : ''}
</span>
<Button variant="ghost" size="sm" onClick={toggleSelectAll}>
{selectedPlayers.size === filteredWatchlist.length && filteredWatchlist.length > 0 ? 'Deselect all' : 'Select all'}
</Button>
Comment on lines +1337 to +1339

@qodo-code-review qodo-code-review Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

2. Select-all set mismatch 🐞 Bug ≡ Correctness

PipelinePage’s new mobile select-all/deselect-all row decides “all selected” via
selectedPlayers.size === filteredWatchlist.length, which can be true even when the selected IDs
differ from the filtered IDs. This can show the wrong label and cause toggleSelectAll to clear or
replace unrelated selections when filters change.
Agent Prompt 
### Issue description
The new mobile select-all/deselect-all UI uses a cardinality comparison (`selectedPlayers.size === filteredWatchlist.length`) to determine whether all filtered players are selected. Because `selectedPlayers` persists independently of filtering, this can misclassify the state (same size, different elements) and the action can clear/overwrite selections that are not part of the current filtered list.

### Issue Context
- `selectedPlayers` is maintained as a Set and is not cleared when `filterTab`, `positionFilter`, or `gradYearFilter` changes.
- `filteredWatchlist` is recomputed from those filters.
- The mobile header label and the desktop header checkbox both use the same size-based check.

### Fix Focus Areas
- src/app/baseball/(dashboard)/dashboard/pipeline/PipelineClient.tsx[970-976]
- src/app/baseball/(dashboard)/dashboard/pipeline/PipelineClient.tsx[1333-1340]

### What to change
1. Compute an `allFilteredSelected` boolean via set inclusion, e.g.:
   - `filteredWatchlist.length > 0 && filteredWatchlist.every(i => selectedPlayers.has(i.id))`
2. Update the mobile button label (and desktop checkbox checked state) to use `allFilteredSelected`.
3. Update `toggleSelectAll` to act on the filtered IDs set-wise:
   - If `allFilteredSelected`, remove only the filtered IDs from `selectedPlayers` (don’t necessarily clear everything).
   - Else, add missing filtered IDs to `selectedPlayers` (don’t necessarily replace the entire set).
   This avoids wiping selections that are outside the current filtered subset.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment on lines +1333 to +1339

@coderabbitai coderabbitai Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Scope “select all” to visible filtered rows (PipelineClient.tsx:1333-1339).

toggleSelectAll() at PipelineClient.tsx:970-975 compares the global selection size with filteredWatchlist.length, then replaces the selection with only visible IDs. With selections retained across filters, the button can show the wrong action and silently drop hidden selections. Compare the visible selected count and add/remove only the currently filtered IDs.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/app/baseball/`(dashboard)/dashboard/pipeline/PipelineClient.tsx around
lines 1333 - 1339, Update toggleSelectAll in PipelineClient.tsx to scope
selection changes to filteredWatchlist: count selected IDs that are currently
visible, deselect only those visible IDs when all visible rows are selected,
otherwise add all visible IDs while preserving hidden selections. Update the
button label condition to use the visible selected count rather than
selectedPlayers.size.

</div>

<div className="space-y-3 lg:hidden">
{filteredWatchlist.map((item, index) => (
<PipelineListCard
key={item.id}
item={item}
focused={focusedIndex === index}
selected={selectedPlayers.has(item.id)}
editing={editingNote === item.id}
noteValue={noteValue}
onFocus={() => setFocusedIndex(index)}
onOpenPeek={() => setPeekPlayerId(item.player?.id || null)}
onToggleSelect={() => togglePlayerSelection(item.id)}
onStatusChange={(stage) => handleStatusChange(item.id, stage)}
onStartEditNote={() => startEditingNote(item.id, item.notes)}
onNoteValueChange={setNoteValue}
onSaveNote={() => handleSaveNote(item.id)}
onCancelNote={() => {
setEditingNote(null);
setNoteValue('');
}}
onViewProfile={() => item.player && setSelectedPlayer(item.player)}
onRemove={() => setRemoveConfirm(item.id)}
/>
))}
</div>

<PaperCard className="hidden overflow-hidden p-0 lg:block">
<div className="overflow-x-auto">
<table className="w-full">
<thead>
Expand Down
74 changes: 57 additions & 17 deletions src/app/baseball/(dashboard)/dashboard/roster/RosterFairway.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,24 @@ function buildWallStats(agg: BaseballPlayerAggregates | undefined, leader: boole
];
}

// Phone read (doctrine Rule 8): a coach triages the wall by contact (AVG)
// and overall production (OPS) — OBP/SLG/SESS live on the player detail page
// this row already taps through to. Labels render inline (PlayerRowStat's
// "standalone row" mode) since there's no shared `PlayerRowPlateHeader` at
// this width to carry the column labels instead.
function buildWallStatsMobile(agg: BaseballPlayerAggregates | undefined, leader: boolean): PlayerRowStat[] {
if (!agg) {
return [
{ label: 'AVG', value: EM_DASH },
{ label: 'OPS', value: EM_DASH },
];
}
return [
{ label: 'AVG', value: agg.career_avg == null ? EM_DASH : formatRate(agg.career_avg, 3) },
{ label: 'OPS', value: agg.career_ops == null ? EM_DASH : formatRate(agg.career_ops, 3), leader },
];
}

function RosterWall({
members,
aggregates,
Expand All @@ -136,25 +154,47 @@ function RosterWall({
}, [members, aggregates]);

return (
<div className="overflow-x-auto">
<div className="min-w-[680px]">
<PlayerRowPlateHeader columns={WALL_COLUMNS} />
<div className="flex flex-col">
{members.map((member, i) => (
<Reveal key={member.id} staggerIndex={Math.min(i, 10)}>
<PlayerRowPlate
firstName={member.player.first_name ?? ''}
lastName={member.player.last_name ?? ''}
jerseyNumber={member.jersey_number ?? undefined}
position={member.player.primary_position ?? undefined}
stats={buildWallStats(aggregates[member.player.id], member.player.id === opsLeaderId)}
onClick={() => onSelect(member.player.id)}
/>
</Reveal>
))}
<>
{/* Below md (Rule 8): identity + the two stats a coach actually
triages by, full-width rows, tap-through to the detail page for
the rest of the record book — never the five-column table's
horizontal scroll on a reading surface. */}
<div className="flex flex-col md:hidden">
{members.map((member, i) => (
<Reveal key={member.id} staggerIndex={Math.min(i, 10)}>
<PlayerRowPlate
firstName={member.player.first_name ?? ''}
lastName={member.player.last_name ?? ''}
jerseyNumber={member.jersey_number ?? undefined}
position={member.player.primary_position ?? undefined}
stats={buildWallStatsMobile(aggregates[member.player.id], member.player.id === opsLeaderId)}
onClick={() => onSelect(member.player.id)}
/>
</Reveal>
))}
</div>

{/* md+: the full five-column record book, unchanged. */}
<div className="hidden overflow-x-auto md:block">
<div className="min-w-[680px]">
<PlayerRowPlateHeader columns={WALL_COLUMNS} />
<div className="flex flex-col">
{members.map((member, i) => (
<Reveal key={member.id} staggerIndex={Math.min(i, 10)}>
<PlayerRowPlate
firstName={member.player.first_name ?? ''}
lastName={member.player.last_name ?? ''}
jerseyNumber={member.jersey_number ?? undefined}
position={member.player.primary_position ?? undefined}
stats={buildWallStats(aggregates[member.player.id], member.player.id === opsLeaderId)}
onClick={() => onSelect(member.player.id)}
/>
</Reveal>
))}
</div>
</div>
</div>
</div>
</>
);
}

Expand Down
Loading
Loading