Skip to content

feat(scheduling): coverage chart explainer — split demand vs floor, on-chart SPLH slider, arithmetic receipt - #650

Merged
jdelgado2002 merged 30 commits into
mainfrom
feature/coverage-chart-explainer
Jul 26, 2026
Merged

feat(scheduling): coverage chart explainer — split demand vs floor, on-chart SPLH slider, arithmetic receipt#650
jdelgado2002 merged 30 commits into
mainfrom
feature/coverage-chart-explainer

Conversation

@jdelgado2002

@jdelgado2002 jdelgado2002 commented Jul 24, 2026

Copy link
Copy Markdown
Member

What & why

The scheduling coverage panel told managers what was wrong (an Area/Delta toggle, a numeric strip, a separate verdict) but never why or what to do. It also had two correctness gaps: it folded raw sales-driven demand into the min_staff floor (so "you need more people because sales justify it" was indistinguishable from "you're just below the floor"), and it printed a misleading N/0 for hours with no sales history.

This rebuilds the panel into one chart that explains its own arithmetic, matching the approved interactive prototype. Six changes:

  1. Split demand-short from floor-short. Solid brick-red slice = scheduled < demand (sales justify more people); dashed amber = only the min_staff floor is unmet (scheduled ≥ demand but < max(demand, minStaff)). Different problems, different fixes — now visually distinct.
  2. On-chart SPLH target slider. The sales-per-labor-hour target moves out of Settings onto the chart. Dragging redraws the "needed" line live, with a live implied-labor-% readout, a red/green pill vs the labor target, and a notch on the track at the labor-consistent value.
  3. Pinned arithmetic "receipt". Replaces the floating tooltip — writes out the division ($503 avg sales ÷ $30 target = 17 people) and stays pinned beside the chart, surviving slider drags.
  4. Hatched "no sales history" hours instead of the misleading N/0.
  5. One plain-English verdict line distinguishing demand-short from floor-short.
  6. Collapsed the Area/Delta toggle + numeric strip + separate verdict into a single chart with a real people y-axis.

How it's built

  • Data model (pure, fully unit-tested): buildHourlyRecommendations now emits raw demand = ceil(avgSales / targetSplh) alongside recommendedStaff = max(demand, minStaff); CoverageHour carries demand + scheduledMax. No migration, no RLS change — purely additive.
  • Presentation logic lives in a new pure src/lib/coverageChartModel.ts (classifyHour, impliedLabor, laborConsistentSplh, buildReceipt, chartSummaryLabel) so the branch logic is unit-covered outside JSX (SonarCloud new-code gate).
  • Live preview + Save: dragging the slider redraws the whole panel live via the settingsOverrides slot useWeekStaffingSuggestions already exposes (zero hook changes). A Save button persists the target via updateSettings; Reset clears the preview.
  • Accessibility: the SVG chart is a role="toolbar" with roving-tabindex role="option" columns (ArrowLeft/Right selection), an sr-only rolled-up summary + understaffed-windows <ul>, and hatch/dash via <pattern> + hsl(var(--warning)) / hsl(var(--muted-foreground)) — semantic tokens only, no hex.

Save-gate role set — please confirm

The Save target button is gated to ['owner','manager','operations_manager']. Preview + Reset are available to everyone. This set was chosen deliberately (there's no prior precedent for gating a staffing-settings write) — flag here if you'd prefer a different set.

Testing

  • pgTAP: 1968 pass.
  • Unit: 587 files / 7384 tests pass. Data layer and coverageChartModel are directly unit-covered.
  • E2E: 157 pass, 12 pre-existing skips, 0 real failures. Fixed one genuine regression found in verify — the new slider's "Save" accessible name collided with the shift-popover "Save" (Playwright strict-mode); resolved with an aria-label, visible text unchanged.
  • typecheck / build: clean.

Review

Built via strict TDD (RED→GREEN→REFACTOR per task). Went through the multi-model review pass (Claude reviewers + Codex adversarial) and CodeRabbit iterations before verify.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Redesigned the scheduling coverage view as an interactive chart with demand and staffing-floor indicators.
    • Added accessible hour selection, keyboard navigation, understaffed-window summaries, and quick-add shift actions.
    • Added a pinned staffing receipt explaining demand, scheduled coverage, and shortfalls.
    • Added an SPLH slider with live labor projections, reset controls, and role-gated save functionality.
    • Improved coverage verdicts by distinguishing demand shortfalls, staffing-floor gaps, covered hours, and no-data periods.
  • Bug Fixes

    • Hours without sales history are now clearly identified instead of appearing as zero demand.
  • Tests

    • Added unit and end-to-end coverage for chart interactions, accessibility, staffing calculations, receipts, and slider behavior.

jdelgado2002 and others added 26 commits July 23, 2026 22:55
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…age chart spec

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Type-only change (task 1.1 of coverage-chart-explainer plan). Adds the
raw pre-floor demand field so downstream code (staffingCalculator,
coverageSummary, coverage chart) can distinguish demand-short from
floor-short hours. Population lands in the next task.
Threads raw demand = ceil(avgSales/targetSplh) (0 when sales/targetSplh
<= 0) through buildHourlyRecommendations and derives recommendedStaff
as max(demand, minStaff) directly, instead of delegating to
calculateRecommendedStaff. Behavior-preserving for recommendedStaff;
demand is new and lets the coverage chart split demand-short from
floor-short later in this feature.

Task 1.2 of coverage-chart-explainer plan.
summarizeCoverageHours now copies rec.demand into CoverageHour.demand
(null when no rec for that hour, back-compat when recs are omitted)
and computes scheduledMax as the per-hour maximum headcount alongside
the existing conservative scheduled=min. Both are additive fields that
lay the groundwork for the demand/floor split in CoverageChart — needed,
delta, and all other CoverageHour semantics are unchanged, so buildVerdict,
mergeUnderStaffedRange, and summarizeAreaCoverage (demand=null path) are
unaffected.

TDD: added 4 RED tests (demand copied per hour; demand null with no
recs; scheduledMax = max of in-hour samples; scheduledMax = 0 with no
coverage), confirmed failing, then GREEN. Full unit suite (7318 tests)
+ typecheck + lint clean on touched files.
New pure module for coverage-chart presentation helpers. classifyHour(h,
minStaff) buckets an hour into crit/floor/spare/ok/nodata: nodata when
h.demand is null; otherwise derives needed = max(demand, minStaff) and
compares against scheduled. Deliberately independent of h.needed so it
stays correct under a hypothetical minStaff (e.g. settings-form preview)
that hasn't round-tripped through the recommendation pipeline.

TDD: 9 RED tests covering the five buckets plus boundaries (scheduled ===
demand, demand < minStaff pulling needed up to the floor, zero/zero/zero),
confirmed failing (module didn't exist), then GREEN. Typecheck + lint
clean on touched files.
impliedLabor({ wage, splh, targetLaborPct }) -> { pct, overTarget } computes
the on-chart SPLH slider's live labor-% readout (pct = wage/splh*100),
flagging overTarget once pct clears targetLaborPct by more than a 0.05-point
tolerance so a target hit within float noise doesn't flash red.

laborConsistentSplh({ wage, targetLaborPct }) = wage / (targetLaborPct/100)
gives the SPLH value at which the slider's track notch is drawn — where a
manager's own labor goal puts the knob.

TDD: 8 RED tests (pct math, overTarget boundary at target+0.05 from both
sides, inverse wage/SPLH relationship, targetLaborPct=100 edge), confirmed
failing (functions didn't exist), then GREEN. Targeted suite
(coverageChartModel + coverageSummary + staffingCalculator, 68 tests),
project-wide typecheck, and lint on touched files all clean.
buildReceipt(h, { minStaff, weekdayKey, wage, lookbackWeeks }) builds the
pinned receipt's ordered ledger rows plus contextual asides for a single
coverage column, per the design doc's worked example.

- nodata (demand === null): no rows, single explanatory aside referencing
  weekdayKey/lookbackWeeks/scheduled (replaces the old "N / 0" reading).
- otherwise: Avg {weekday} sales / ÷ target / = demand / min staff / needed /
  scheduled / kind-specific closing row (Short on demand | Short on floor |
  On target | Covered), each row tagged with a tone (default/critical/
  warning/positive) for the UI to color.
- "÷ target" is the implied target (round(projectedSales/demand)), not the
  live SPLH slider value — CoverageHour doesn't carry that raw value (only
  the already-folded demand), and the implied figure keeps the "sales ÷
  target = demand" line on the receipt exactly self-consistent (no hidden
  Math.ceil to explain). Omitted when demand === 0 (a rec exists but
  avgSales <= 0, so there's nothing to divide).
- Asides: implied SPLH at the scheduled count (skipped at scheduled === 0
  to avoid a divide-by-zero), the mid-hour scheduledMax->scheduled note
  (only when they differ), and the floor-only explainer.

Reuses classifyHour internally so the receipt's kind can never drift from
the chart's own classification.

TDD: 7 RED tests added (nodata; crit/floor/ok/spare row sets incl. the
design doc's own $503/17-people/$30 example, which also validated the
"implied target" derivation against the doc's illustrative numbers;
mid-hour aside presence boundary; demand===0 edge omitting the ÷ target
row), confirmed failing (function didn't exist), then GREEN.

Targeted suite (coverageChartModel + coverageSummary + staffingCalculator +
coverageChart component test, 103 tests), project-wide typecheck, and lint
(touched files) clean. Full 7300+-test unit suite was invoked but stalled
in the sandbox background scheduler without producing output after several
minutes (same intermittent issue noted on commit 31e39de/task 2.2); this
change only adds one new pure export with zero edits to existing exported
behavior, so it was not blocking.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… + windows list

chartSummaryLabel(hours, minStaff) rolls a day's CoverageHour[] into the
chart's accessible summary: an "N short on demand, M at the floor over K
hours" string for the sr-only <p>, plus an ordered understaffedWindows list
(one entry per crit/floor hour) feeding a sr-only <ul aria-label="Understaffed
windows"> — porting CoverageStatusStrip's screen-reader gap enumeration
guarantee onto the new chart (design doc, Phase 2.5 resolutions #1-#2).

Classifies every hour via classifyHour (not h.delta/h.needed) so counts and
windows always agree with the chart/receipt's own minStaff-aware
classification, even under a live settings-form minStaff preview.

TDD: 4 RED tests (rollup counts across crit/floor/ok/spare/nodata; singular
"1 hour" boundary; zero-shorts boundary with empty windows list; per-window
label/order/exclusion), confirmed failing (export didn't exist), then GREEN.
Full unit suite (584 files, 7346 tests passed, 2 skipped) + project-wide
typecheck + lint (touched files) clean.
Stage 3.1 of the coverage chart explainer redesign. CoverageChart.tsx is
now a single SVG chart with a real people y-axis, replacing the old
AreaColumn/DeltaColumn/Legend two-view toggle:

- Per hour: a scheduled bar (--primary), a solid demand slice
  (--destructive) when short of raw demand (crit), a dashed/hatched
  floor slice (--warning) covering the gap up to the minStaff floor
  (crit/floor), and a hatched nodata ghost via an SVG <pattern> for
  hours with no sales history (replacing the old misleading "N / 0").
  A `floor N` rule line marks minStaff across the plot.
- New props: minStaff, selectedStartMin, onSelect, onQuickAdd (type
  only — its UI affordance is Stage 3.3); `view`/`targetSplh` removed.
- Accessibility: role="toolbar" container (not role="img", which would
  flatten the interactive subtree) with role="option" columns, roving
  tabIndex (selected=0, rest=-1), ArrowLeft/ArrowRight to move
  selection, and a pure exported `columnAriaLabel` helper for each
  column's one-line summary. A sr-only <p> carries the
  chartSummaryLabel() rollup and a sr-only
  <ul aria-label="Understaffed windows"> enumerates every crit/floor
  hour, porting CoverageStatusStrip's screen-reader gap guarantee onto
  the new chart.
- All fills/strokes use hsl(var(--token)) semantic colors only; the
  floor slice's dashed stroke and the nodata hatch pattern give each
  state a texture, not just a color.

ShiftTimelineTab.tsx gets the minimal compatibility shim needed for the
new prop contract: a local selectedStartMin state, minStaff via the
existing computeMinStaffFromCrew(min_crew, min_staff), and
onQuickAdd={handleGapClick} (reusing the existing gap-click handler).
The coverageView state, Area/Delta ToggleGroup, and CoverageStatusStrip
render are intentionally left in place — their removal is bundled with
the full Stage 5 wiring (sliderTarget, avgWage, receipt panel) per the
design doc's "E. ShiftTimelineTab.tsx wiring" section, out of scope
here.

TDD: tests/unit/coverageChart.test.tsx fully rewritten (25 tests) to
target the new contract — column layout/alignment, roving-tabindex
keyboard nav, demand/floor SVG slice rendering, the nodata hatch
pattern, the floor rule, semantic-color-only assertion, the sr-only
summary/list, the four-swatch legend, and columnAriaLabel's per-kind
wording. Confirmed RED (23 failed) against the old component, then
GREEN. Full unit suite (584 files, 7343 tests, 2 skipped) + project-wide
typecheck + lint (touched files) clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…melineLane

CoverageChart's y-axis gutter already carried the sticky left-0 z-10
w-[120px] classes from Stage 3.1's SVG rewrite (mirroring TimelineLane's
sticky label column, design doc §Design-review resolutions #3), but no
test pinned that contract. Add a data-testid to the gutter and cover:
the required class set, DOM order (gutter before the scrollable
toolbar/plot region), tick rendering scoped to the gutter, and that the
plot columns remain aligned to minToPct with the wrapper in place.

RED: 3 of 4 new tests failed (data-testid didn't exist) against the
existing markup. GREEN: added the data-testid only, no layout changes.
Full unit suite (585 files, 7347 tests) + typecheck + lint (touched
files) clean.
…umns

CoverageChart tracks hovered/focused startMin and reveals a small "+"
button over crit/floor columns (only when onQuickAdd is supplied),
calling onQuickAdd(startMin) on click without triggering column
selection — preserving the one-click quick-add parity with the old
CoverageStatusStrip gap-click (design doc resolution #5). Omitting
onQuickAdd keeps the affordance hidden entirely (back-compat).

TDD: 5 RED tests (hidden without onQuickAdd; reveals + calls onQuickAdd
for crit; same for floor; never renders for spare/ok/nodata; hides on
mouseleave; accessible label names the hour), confirmed failing, then
GREEN. Full unit suite (585 files, 7353 tests passed, 2 skipped) +
project-wide typecheck + lint (touched files) clean.
Native range input (25-120, step 5) with a live implied-labor readout
(`→ X% labor at $W/hr`) + red/green pill from `impliedLabor`, a notch on
the track at `laborConsistentSplh` labeling where the manager's own
labor goal sits, a Reset button, and a manager-only Save button gated
by a caller-computed `canSave` prop with an `isSaving` pending state.
`aria-valuetext` carries the implied labor % alongside the native
`aria-valuenow`. Purely presentational — holds no staffing state of its
own; `onChange`/`onSave`/`onReset` let the eventual `ShiftTimelineTab`
wiring (Stage 5) own the slider/override/save state and re-run the
staffing pipeline live.

TDD: 11 RED tests (range bounds/value; onChange on drag; readout text;
pill wording+styling at both sides of the target+0.05 threshold; notch
value/label; Reset click; Save hidden for canSave=false, visible+wired
for true; Saving pending state disables the button; aria-valuetext
content), confirmed failing (module didn't exist), then GREEN. Full
unit suite (586 files, 7364 tests passed, 2 skipped) + project-wide
typecheck + lint (touched files) clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-add + debounced aria-live)

New standalone panel rendering buildReceipt's ledger rows/asides for the
selected coverage-chart hour, with an "Add shift for this hour" action on
crit/floor hours (calls onQuickAdd(startMin); omitted onQuickAdd or an
ok/spare/nodata hour hides it, back-compat with the chart's own hover "+").

The visible ledger redraws live on every prop change (including SPLH-slider
drag frames once wired in Stage 5), but the aria-live="polite" region only
re-announces on commit (window pointerup/keyup) or on an actual hour-selection
change, per design doc §Design-review resolutions #4 — screen readers get one
coherent sentence per commit, not a stream of per-frame updates.

TDD: 11 RED tests (row/value rendering; nodata asides-only path; quick-add
present for crit/floor, calling onQuickAdd with startMin; absent for
ok/spare/nodata and when onQuickAdd is omitted; aria-live region content;
immediate announce on hour-selection change; frozen announce through a
same-hour "drag frame" until pointerup/keyup fires), confirmed failing (module
didn't exist), then GREEN. Full unit suite (587 files, 7375 tests passed, 2
skipped) + project-wide typecheck + lint (touched files) clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…eTab

Adds the SPLH slider's local preview state (design doc §B/§E, plan 5.1):
`sliderTarget` (null = follow saved settings) now feeds
`useWeekStaffingSuggestions`'s `settingsOverrides` argument via a new pure
`resolveSettingsOverrides` helper, replacing the hard-coded `null`. Also
destructures `updateSettings`/`isSaving` from the hook and derives `avgWage`
(computeAvgHourlyRateCents(employees)/100) alongside the existing `minStaff`
derivation — all four feed the SplhSlider/CoverageReceipt wiring landing in
the next task; none are rendered yet.

TDD: 3 RED tests for resolveSettingsOverrides (null passthrough, override
object, 0-is-not-null), confirmed failing (function didn't exist), then
GREEN. Full unit suite (588 files, 7378 tests passed, 2 skipped) +
project-wide typecheck + lint (touched files, 0 errors) clean.
…receipt (Stage 5.2)

ShiftTimelineTab now renders the coverage panel as a flex row: the
scrollable CoverageChart (header folds in CoverageVerdict/CoverageDemandInfo)
on the left, and a pinned CoverageReceipt outside the horizontal scroll on
the right (stacking below on mobile). Removes the coverageView state, the
Area/+/- ToggleGroup, and the CoverageStatusStrip render per design doc §E.

Quick-add entry point relocates from CoverageStatusStrip's gap cells to
CoverageReceipt's "Add shift for this hour" button (crit/floor hours only),
calling the same handleGapClick(startMin) — behavior unchanged, per §D.

Adds two pure helpers backing the receipt's default-hour selection:
- pickDefaultHour(hours, minStaff): worst crit hour by demand deficit
  (demand - scheduled), earliest startMin on ties, else first hour, else
  null for empty input.
- weekdayKeyForDay(day): noon-anchored weekday name for the receipt's
  "Avg {weekday} sales" row, matching the dayStringToDow DST-safety pattern.

Updates the one pre-existing overlay test whose click target was the
removed CoverageStatusStrip gap cell to go through the relocated
CoverageReceipt button instead, adding a `demand` field to its recommendation
fixture so the hour classifies as `crit` under the new demand-driven
classifyHour pipeline (previously only the legacy needed/delta pipeline
saw it as short).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…anager (Stage 5.3)

Mounts <SplhSlider> above the coverage chart in ShiftTimelineTab, wiring live
preview + Reset (available to every role) and Save (gated).

- Add SPLH_SAVE_ROLES = ['owner', 'manager', 'operations_manager'], mirroring
  the existing ad-hoc inline predicate already used in TimePunchesManager.tsx
  and Inventory.tsx (design doc §Design-review resolutions #6 — there's no
  shared role-gate helper for this style of check yet, so this follows the
  established convention rather than introducing a new one).
- canSaveSplhTarget reads selectedRestaurant?.role via useRestaurantContext()
  and is passed to SplhSlider's canSave prop, which hides the Save button
  entirely for non-gated roles while the range input (live preview) and Reset
  stay unconditionally available.
- handleSplhChange/handleSplhSave/handleSplhReset drive a local sliderTarget
  override; Save calls updateSettings({ target_splh }) then clears the
  override so subsequent renders read straight off activeSettings again.
- targetLaborPct sourced from activeSettings.target_labor_pct, falling back
  to the useStaffingSettings DEFAULTS value (22) before settings load.
- Slider is hidden entirely when there's no target to preview yet
  (targetSplh === null), matching the design doc's "No demand configured"
  state.

Tests (RED -> GREEN):
- New describe block in shiftTimelineTab.test.tsx: Save button shown for
  owner/manager/operations_manager, hidden (but preview+Reset kept) for
  staff/chef/kiosk/collaborator_accountant and for no-restaurant-selected,
  slider drag + Reset round-trip, Save persists via updateSettings, and the
  slider not rendering at all when activeSettings hasn't loaded.
- ShiftTimelineTab now calls useRestaurantContext() directly, so
  shiftTimelineTab.mobileLayout.test.tsx and shiftTimelineTabOverlay.test.tsx
  needed a `@/contexts/RestaurantContext` mock (defaulting to 'owner') to
  avoid the "must be used within a RestaurantProvider" throw.

Verified: full unit suite (588 files / 7397 tests, 2 skipped) green,
typecheck clean, lint clean on touched files.

Note for the eventual PR body (design doc requirement): the gated role set is
['owner', 'manager', 'operations_manager'].
…axis/chart/receipt (Stage 5.4)

The loading skeleton previously used generic bars unrelated to the new
coverage-panel layout (slider panel -> chart -> axis -> lanes, plus a
pinned receipt column). Reshaped it to mirror that structure exactly —
same lg:w-[320px] lg:shrink-0 receipt column, same top-to-bottom order
in the scrollable chart column — so nothing jump-shifts once real data
lands, per design doc resolution #9.

TDD: added a RED test asserting dedicated skeleton bands
(skeleton-splh-slider/-coverage-chart/-timeline-axis/-coverage-receipt),
the receipt band living in its own pinned column separate from the
chart/axis column, and slider-before-chart-before-axis DOM order;
confirmed failing against the old generic skeleton, then GREEN. Full
unit suite (589 files, 7398 tests passed, 2 skipped) + project-wide
typecheck + lint (touched files, 0 errors) clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Grep confirmed no production importer remained after Stage 5.2 folded its
gap-click/sr-only-enumeration duties into CoverageChart — only its own
dedicated test file still referenced it. Deleted both the component and
tests/unit/coverageStatusStrip.test.tsx, and cleaned up two now-stale
comments (coverageSummary.ts's formatCoverageHour doc, timelineComponents
.test.tsx's file-header note) that still named it as a live consumer.

TDD: confirmed the dedicated test suite green (14 tests) as baseline,
deleted the source file (RED — the test file's import failed to
resolve), then deleted the obsolete test file itself (GREEN). Full unit
suite (588 files, 7384 tests passed, 2 skipped) + project-wide typecheck
+ lint (touched files) clean.
…eededFor()

Math.max(demand, minStaff) was reimplemented 6 times across
coverageChartModel.ts (classifyHour, buildReceipt, chartSummaryLabel) and
CoverageChart.tsx (computePeak, columnAriaLabel, the SVG render loop).
Extracted a single exported neededFor(demand, minStaff) helper in
coverageChartModel.ts and pointed every call site at it. Pure refactor, no
behavior change — full unit suite (587 files, 7384 tests) unchanged.

Left staffingCalculator.ts's own Math.max(demand, minStaff) untouched: that's
a deliberately independent upstream computation (recommendedStaff), not the
presentation-layer "needed" derivation this helper centralizes.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ic, maintainability, codex

- Memoize resolveSettingsOverrides(sliderTarget) so useWeekStaffingSuggestions
  doesn't recompute the whole week's staffing pipeline on every unrelated
  re-render (e.g. rAF drag-draft frames) while an SPLH preview is active
  (performance + sound-logic).
- Feed CoverageChart's selectedStartMin from the resolved selectedHour
  (pickDefaultHour fallback) instead of the raw click state, so the chart's
  roving-tabindex column always agrees with whichever hour the pinned
  receipt is showing, including before any explicit click (sound-logic).
- Fix handleSplhSave to only clear the slider override if it still matches
  the value just persisted, so a newer in-flight drag isn't silently
  discarded by an in-flight Save's resolution (sound-logic).
- Remove the redundant outer pl-[120px] wrapper around CoverageChart —
  CoverageChart already renders its own internal w-[120px] y-axis gutter, so
  the extra wrapper double-offset the chart against the axis ticks/lanes
  below (codex). AreaCoverageStrips has no internal gutter, so its
  pl-[120px] wrapper is unaffected and stays.
- Delete calculateRecommendedStaff (staffingCalculator.ts) and its dedicated
  test block — dead code since task 1.2 inlined its logic at the only call
  site; grep confirms zero remaining production callers (maintainability).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…r 2)

- coverageSummary.ts: a staffing rec with projectedSales <= 0 has no demand
  target to derive from (buildHourlyRecommendations floors its own demand to
  0 in that case). summarizeCoverageHours was passing that 0 through as a
  real "0 people needed" target instead of nodata, contradicting the design
  doc's own nodata definition ("no rec, or projectedSales ≤ 0").
- coverageChartModel.ts: guard impliedLabor/laborConsistentSplh against a 0
  splh/targetLaborPct denominator so a misconfigured or pre-bounds-check
  value degrades to 0 instead of Infinity/NaN in the labor readout.
- tests: cover both fixes, add the missing negative-sales case in
  staffingCalculator.test.ts, and prefix the touched staffing-calculation
  tests with CRITICAL: per coding guidelines. Fixed a stale test fixture in
  shiftTimelineTab.test.tsx (Task 2d wiring rec was missing the required
  `demand` field, which the coverageSummary.ts fix surfaced as NaN render
  warnings).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sts (CodeRabbit iter 3)

Rename the added demand/floor test descriptions to the "should X when Y"
convention flagged by CodeRabbit for consistency and searchability.
The new SplhSlider renders a persistent "Save" button whose accessible
name collided with the shift-edit popover's own "Save" button, tripping a
Playwright strict-mode violation in timeline-edit-create.spec.ts. Add an
aria-label ("Save sales per labor hour target" / "Saving sales per labor
hour target"); visible text unchanged. Anchor the matching unit assertions
to /^save$/i so they target the popover button, not the slider.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds tests/e2e/coverage-chart-explainer.spec.ts covering the redesigned
coverage panel end-to-end against the real staffing pipeline:

- Seeds one $15/hr server + a mid-day shift + ~2 weeks of daily-spread
  sales so today's weekday has demand (~4/hr) against a lone scheduled
  server, guaranteeing demand-short (crit) columns.
- Asserts the single coverage chart (role="toolbar"), crit columns,
  the legend's demand-vs-floor split, the on-chart SPLH slider with its
  live implied-labor readout (default $60 → 25.0% at $15/hr) + over-target
  pill, the owner-visible Save button, and the pinned arithmetic receipt
  with its quick-add.
- Drives the slider via keyboard End/Home ($120 → 12.5% "On target";
  $25 → 60.0% "Over target") to prove the readout + pill update live.

Timezone pinned to UTC and wage fixed at exactly $15.00 for deterministic
percentages.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 24, 2026 19:56
@netlify

netlify Bot commented Jul 24, 2026

Copy link
Copy Markdown

Deploy Preview for easyshifthq failed.

Name Link
🔨 Latest commit 3b62e65
🔍 Latest deploy log https://app.netlify.com/projects/easyshifthq/deploys/6a6413f701953800088bbc25

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@jdelgado2002, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 47 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 86f26dcf-55a0-4e25-9dea-ff4bd64515eb

📥 Commits

Reviewing files that changed from the base of the PR and between fdc94e5 and 3b62e65.

📒 Files selected for processing (5)
  • src/components/scheduling/ShiftTimeline/CoverageChart.tsx
  • src/hooks/useWeekStaffingSuggestions.ts
  • tests/unit/coverageChart.test.tsx
  • tests/unit/shiftTimelineTab.test.tsx
  • tests/unit/useWeekStaffingSuggestions.pagination.test.ts
📝 Walkthrough

Walkthrough

The PR redesigns Timeline coverage around demand-aware staffing data, a unified accessible SVG chart, an SPLH preview slider, and a pinned arithmetic receipt. It updates verdict calculations, replaces the former status strip and tooltip flow, adds role-gated saving, and expands unit and E2E coverage.

Changes

Coverage chart explainer

Layer / File(s) Summary
Demand-aware coverage data contracts
src/lib/coverageSummary.ts, src/lib/staffingCalculator.ts, src/types/scheduling.ts, tests/unit/*
Hourly recommendations now expose raw demand; coverage summaries add demand and scheduledMax; verdicts distinguish demand-short and floor-only hours.
Coverage model, receipt, and labor calculations
src/lib/coverageChartModel.ts, src/components/scheduling/ShiftTimeline/CoverageReceipt.tsx, src/components/scheduling/ShiftTimeline/SplhSlider.tsx, tests/unit/*
Pure helpers classify hours, calculate labor values, build receipt content, and generate accessible summaries used by the receipt and slider.
Unified SVG coverage chart
src/components/scheduling/ShiftTimeline/CoverageChart.tsx, tests/unit/coverageChart.test.tsx
The chart renders demand/floor states, nodata patterns, y-axis ticks, legends, roving selection, quick-add controls, and screen-reader summaries.
Timeline wiring and selection flow
src/components/scheduling/ShiftTimeline/ShiftTimelineTab.tsx, src/components/scheduling/ShiftTimeline/CoverageVerdict.tsx, tests/unit/shiftTimelineTab*.tsx
Timeline state connects SPLH previews, role-gated save/reset actions, chart selection, pinned receipts, quick-add, revised verdict text, and the new layout.
End-to-end verification and supporting documentation
tests/e2e/coverage-chart-explainer.spec.ts, tests/unit/timelineComponents.test.tsx, docs/superpowers/...
Design and implementation plans describe the staged redesign, while deterministic E2E tests cover chart rendering, receipt actions, slider behavior, and labor readouts.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Manager
  participant ShiftTimelineTab
  participant CoverageChart
  participant CoverageReceipt
  participant StaffingSettings
  Manager->>ShiftTimelineTab: move SPLH slider
  ShiftTimelineTab->>StaffingSettings: request preview override
  StaffingSettings-->>ShiftTimelineTab: return updated recommendations
  ShiftTimelineTab->>CoverageChart: render updated coverage
  Manager->>CoverageChart: select hour
  CoverageChart->>ShiftTimelineTab: report selected startMin
  ShiftTimelineTab->>CoverageReceipt: update pinned receipt
  Manager->>CoverageReceipt: add shift for hour
  CoverageReceipt->>ShiftTimelineTab: invoke quick-add
Loading

Possibly related PRs

Suggested reviewers: copilot

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main redesign: demand vs floor split, SPLH slider, and arithmetic receipt.
Docstring Coverage ✅ Passed Docstring coverage is 81.82% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/coverage-chart-explainer

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.

@vercel

vercel Bot commented Jul 24, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
easyshifthq Ready Ready Preview, Comment Jul 25, 2026 1:41am

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2be3eef714

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +157 to +158
const impliedTarget = Math.round(projectedSales / demand);
rows.push({ label: '÷ target', value: `${fmtUsd(impliedTarget)}/hr`, tone: 'default' });

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Pass the actual SPLH target to the receipt

When projected sales are not exactly divisible by the selected target, this reverse-calculation reports a different target than the one driving the recommendation. For example, $200 sales at a $60 SPLH target produces demand 4 via ceil, but the receipt displays $50/hr; this makes the new arithmetic explanation misleading while the slider is at $60. Thread the active target SPLH into buildReceipt/CoverageReceipt and display that value instead.

Useful? React with 👍 / 👎.

Comment on lines +148 to 149
demand: rec && rec.projectedSales > 0 ? rec.demand : null,
delta: needed === null ? null : scheduled - needed,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Exclude no-data hours from the verdict

For a zero- or negative-sales recommendation with a nonzero staffing floor, this sets demand to null so the chart correctly renders the hour as “no sales history,” but leaves needed and a negative delta intact. buildVerdict still counts negative deltas, so the header can report the no-data hour as short-staffed (and name it as the biggest gap), contradicting the chart's no-target state. Clear or exclude the delta when the hour is classified as no-data.

Useful? React with 👍 / 👎.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Redesigns the Shift Timeline scheduling coverage panel into a single, self-explaining chart by separating sales-driven demand from the minimum-staff floor, adding an on-chart SPLH target slider (with role-gated Save), and replacing tooltips/status-strip UI with a pinned arithmetic “receipt”.

Changes:

  • Extend the staffing/recommendation data model to carry raw pre-floor demand and per-hour scheduledMax, and add pure chart/receipt helpers in coverageChartModel.
  • Replace the old coverage chart/strip UI with a single SVG chart (demand vs floor split, nodata hatching, roving-tabindex accessibility) plus a pinned CoverageReceipt and SplhSlider.
  • Add/adjust extensive unit + E2E tests covering the new model helpers and UI wiring, and remove the obsolete CoverageStatusStrip.

Reviewed changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/unit/timelineComponents.test.tsx Updates test-file commentary to reflect the new coverage components/tests.
tests/unit/staffingCalculator.test.ts Removes tests for deleted calculateRecommendedStaff; adds demand/floor behavior tests for buildHourlyRecommendations.
tests/unit/splhSlider.test.tsx New unit tests for SPLH slider bounds, interactions, ARIA, and Save/Reset behaviors.
tests/unit/shiftTimelineTabOverlay.test.tsx Updates overlay wiring test to use the receipt’s “Add shift for this hour” entry point; adds RestaurantContext mock.
tests/unit/shiftTimelineTab.test.tsx Updates coverage-panel wiring tests; adds receipt wiring + Save-gate authz tests; reshapes loading skeleton assertions.
tests/unit/shiftTimelineTab.mobileLayout.test.tsx Adds RestaurantContext mock due to new direct role check in ShiftTimelineTab.
tests/unit/resolveSettingsOverrides.test.ts New unit tests for resolveSettingsOverrides preview/null sentinel behavior.
tests/unit/pickDefaultHour.test.ts New unit tests for default-hour selection logic based on classifyHour.
tests/unit/coverageSummary.test.ts Extends coverage summary tests for demand threading and scheduledMax.
tests/unit/coverageStatusStrip.test.tsx Removes tests for deleted CoverageStatusStrip component.
tests/unit/coverageReceipt.test.tsx New unit tests for receipt rendering, quick-add, and aria-live commit semantics.
tests/unit/coverageChartModel.test.ts New unit tests for pure helpers: classifyHour, impliedLabor, laborConsistentSplh, buildReceipt, chartSummaryLabel.
tests/unit/coverageChart.test.tsx Replaces old Area/Delta chart tests with SVG chart + accessibility + quick-add + sticky gutter tests.
tests/e2e/coverage-chart-explainer.spec.ts New E2E specs validating chart/slider/receipt wiring with seeded sales + shifts.
src/types/scheduling.ts Adds demand to HourlyStaffingRecommendation.
src/lib/staffingCalculator.ts Refactors buildHourlyRecommendations to compute raw demand and derive recommendedStaff; removes calculateRecommendedStaff.
src/lib/coverageSummary.ts Adds scheduledMax and demand to CoverageHour; threads both through summarizeCoverageHours.
src/lib/coverageChartModel.ts New pure presentation/model helpers for chart classification, slider math, receipt building, and SR summary.
src/components/scheduling/ShiftTimeline/SplhSlider.tsx New on-chart SPLH slider component with implied labor readout, notch, Save/Reset UI.
src/components/scheduling/ShiftTimeline/ShiftTimelineTab.tsx Wires in slider preview overrides, role-gated Save, pinned receipt, and new chart props/layout; removes old toggle/strip.
src/components/scheduling/ShiftTimeline/CoverageStatusStrip.tsx Removes CoverageStatusStrip component (superseded by chart + receipt + SR enumeration).
src/components/scheduling/ShiftTimeline/CoverageReceipt.tsx New pinned arithmetic receipt panel with aria-live “commit” behavior and quick-add action.
src/components/scheduling/ShiftTimeline/CoverageChart.tsx Replaces old dual-view chart with single SVG chart, demand/floor split rendering, roving-tabindex columns, SR summary/list, and hover quick-add.
docs/superpowers/specs/2026-07-23-coverage-chart-explainer-design.md Adds detailed design spec for the coverage explainer redesign.
docs/superpowers/plans/2026-07-23-coverage-chart-explainer-plan.md Adds staged TDD plan for implementing the redesign.

Comment on lines +132 to +136
value={value}
onChange={(e) => onChange(Number(e.target.value))}
aria-label="Sales per labor hour target, in dollars"
aria-valuetext={`$${value}/hr → ${pctLabel}% labor`}
className="w-full accent-primary"
Comment on lines +289 to +293
// drag value, fed to `useWeekStaffingSuggestions` via `settingsOverrides` so
// the whole pipeline (chart/receipt/verdict) redraws off ONE source of
// truth. Not yet wired to a slider UI (Stage 4.1's `SplhSlider` render lands
// in the next task) — this only satisfies the state/plumbing half of the
// contract.
Comment on lines +104 to +107
useEffect(() => {
setAnnounced(announcementText(latestRef.current.receipt, latestRef.current.hour));

}, [hour.startMin]);
The shipped coverage panel diverged from the approved interactive prototype —
it read as an undifferentiated block chart with a terse status line. Rework the
four presentation surfaces to faithfully match the mock, translating its raw
hex/serif design language into the app's semantic tokens and typography:

- CoverageVerdict: one plain-English, two-clause sentence that distinguishes
  demand-short ("Sales justify N more people-hours…, worst at TIME") from
  floor-only ("Another M hours only trip the K-person floor — demand there is
  already met"), plus a category chips row (short on demand / at the floor only
  / covered / no sales history). Extend buildVerdict() with the split fields
  (demandShort*, floorOnly*, worstCrit, covered/nodataHours) that drive it.
- CoverageChart: discrete inset bars instead of one abutting block — scheduled
  fill + solid primary cap, a solid-red demand-short slice, a dashed-amber
  floor-only cap (texture, not colour alone), a foreground needed tick, and
  on-bar −N shortfall labels centred in the gap they name.
- CoverageReceipt: an "The arithmetic" eyebrow + hour header, the ledger, and
  the verdict set off below a divider as an emphasised total.
- SplhSlider: reframed as "Each labor hour should bring in $X → Y% labor",
  with the labor-consistent notch and $min/$max scale ends.

Semantic tokens only (destructive/warning/primary/success/muted-foreground) —
no raw hex. Unit tests updated for the new verdict shape and the relocated
dashed floor cap; typecheck, lint, and the 118 coverage unit tests are green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

src/components/scheduling/ShiftTimeline/CoverageReceipt.tsx:107

  • There’s an extra whitespace-only line inside this useEffect (line 106) that will show up as trailing whitespace noise and can trip lint/format rules. It should be removed.
  useEffect(() => {
    setAnnounced(announcementText(latestRef.current.receipt, latestRef.current.hour));
     
  }, [hour.startMin]);

src/components/scheduling/ShiftTimeline/ShiftTimelineTab.tsx:293

  • This comment says the SPLH slider state is “Not yet wired to a slider UI”, but this PR does render <SplhSlider> (later in this file). The comment should be updated so it doesn’t mislead future refactors.
  // drag value, fed to `useWeekStaffingSuggestions` via `settingsOverrides` so
  // the whole pipeline (chart/receipt/verdict) redraws off ONE source of
  // truth. Not yet wired to a slider UI (Stage 4.1's `SplhSlider` render lands
  // in the next task) — this only satisfies the state/plumbing half of the
  // contract.

Comment thread src/types/scheduling.ts
Comment on lines +317 to +318
/** Raw pre-floor demand: ceil(projectedSales / target_splh), 0 when projectedSales <= 0. */
demand: number;
Comment on lines +53 to +54
- No change to the staffing math itself (`calculateRecommendedStaff` stays
authoritative — `max(ceil(sales/splh), minStaff)`).

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 7

🧹 Nitpick comments (6)
src/components/scheduling/ShiftTimeline/SplhSlider.tsx (1)

132-143: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Clamp the value fed to the range input.

clampForDisplay is used for the notch but not for the input itself. A saved target_splh outside 25–120 (or off-step) makes the browser snap the thumb while the readout above still shows the raw number, and the first onChange then jumps unexpectedly. Passing clampForDisplay(value) to the input keeps thumb and readout consistent without changing what the caller persists.

🤖 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/components/scheduling/ShiftTimeline/SplhSlider.tsx` around lines 132 -
143, Update the range input in SplhSlider to use clampForDisplay(value) for its
value prop, keeping the caller-provided value unchanged for persistence while
ensuring the thumb matches the displayed readout.
tests/e2e/coverage-chart-explainer.spec.ts (1)

158-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer semantic selectors over data-testid/attribute locators where a role exists.

The receipt's quick-add and slider already use roles; the legend, pill, and [data-hour-col][data-kind="crit"] locators could use accessible names (e.g. the columns' aria-label contains "short N on demand"), keeping the spec resilient to markup changes.

As per path instructions for tests/e2e/**/*.spec.ts: "Use page.getByRole() and page.getByLabel() semantic selectors in E2E tests, NOT data-testid".

Also applies to: 164-166, 180-182

🤖 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 `@tests/e2e/coverage-chart-explainer.spec.ts` at line 158, Replace the
attribute-based locators in the coverage chart test, including critColumns and
the referenced legend and pill locators, with semantic page.getByRole() or
page.getByLabel() selectors. For the hour columns, target their accessible
aria-label names containing “short N on demand,” while preserving the existing
element-specific assertions and interactions.

Source: Path instructions

src/components/scheduling/ShiftTimeline/CoverageReceipt.tsx (1)

82-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move the ref write out of render.

latestRef.current = { receipt, hour } runs during render, so a discarded/replayed render can leak into the commit-time announcement. An effect keeps the same "latest value" behavior while staying pure.

♻️ Proposed change
   const latestRef = useRef({ receipt, hour });
-  latestRef.current = { receipt, hour };
+  useEffect(() => {
+    latestRef.current = { receipt, hour };
+  }, [receipt, hour]);

Note the hour.startMin effect then reads the just-committed ref; ordering holds because effects run in declaration order — declare this update effect first.

🤖 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/components/scheduling/ShiftTimeline/CoverageReceipt.tsx` around lines 82
- 107, Move the latestRef.current assignment into a useEffect declared before
the existing hour.startMin announcement effect, so the ref is updated after
commit while preserving access to the latest receipt and hour. Remove the
render-time assignment and keep the commit-event handler’s empty-dependency
behavior unchanged.

Source: Linters/SAST tools

src/components/scheduling/ShiftTimeline/ShiftTimelineTab.tsx (2)

506-510: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Import the shared DEFAULTS instead of re-hardcoding 22 / 4.

Both fallbacks duplicate useStaffingSettings' DEFAULTS.target_labor_pct / DEFAULTS.lookback_weeks (as the comments themselves acknowledge), so a change there silently drifts from what the slider pill and receipt display. Prefer importing the constant if it is exported (or exporting it).

Also applies to: 531-535

🤖 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/components/scheduling/ShiftTimeline/ShiftTimelineTab.tsx` around lines
506 - 510, Update the fallback values in the ShiftTimelineTab logic around
targetLaborPct and lookbackWeeks to use the shared DEFAULTS.target_labor_pct and
DEFAULTS.lookback_weeks constants instead of hard-coded 22 and 4. Import
DEFAULTS from its existing export, or export it from useStaffingSettings if
necessary, while preserving the current fallback behavior.

287-294: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Stale comment: the slider is now rendered.

SplhSlider is mounted at Line 960, so the "Not yet wired to a slider UI … lands in the next task" note is no longer true.

♻️ Suggested comment trim
   // the whole pipeline (chart/receipt/verdict) redraws off ONE source of
-  // truth. Not yet wired to a slider UI (Stage 4.1's `SplhSlider` render lands
-  // in the next task) — this only satisfies the state/plumbing half of the
-  // contract.
+  // truth. Rendered by the `SplhSlider` panel above the chart.
🤖 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/components/scheduling/ShiftTimeline/ShiftTimelineTab.tsx` around lines
287 - 294, The comment above sliderTarget is stale because SplhSlider is now
rendered. Remove the sentence claiming the state is not wired to a slider UI and
that SplhSlider will land in the next task, while preserving the accurate
explanation of sliderTarget and its settingsOverrides data flow.
tests/unit/shiftTimelineTab.test.tsx (1)

486-489: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Rewrite this test title — it reads as an unfinished thought.

The name argues with itself mid-sentence ("fully covered by... wait, only 1 scheduled total"), which makes failure output confusing. The body comment already explains the setup.

♻️ Suggested rename
-  it('CRITICAL: defaults the receipt to the worst crit hour (10 AM: demand 2, scheduled 1 — deficit 1) over the ok 11 AM hour (demand 17 fully covered by... wait, only 1 scheduled total) — uses the larger-deficit hour', () => {
+  it('CRITICAL: should default the receipt to the largest-deficit crit hour (11 AM, deficit 16) when several hours are crit', () => {

As per coding guidelines: "Use descriptive test names: 'should [expected behavior] when [condition]'".

🤖 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 `@tests/unit/shiftTimelineTab.test.tsx` around lines 486 - 489, Rename the test
case around pickDefaultHour to a concise “should [expected behavior] when
[condition]” description, removing the self-correction and setup details from
the title. Preserve the existing assertion and body comment explaining that the
larger-deficit 11 AM hour is selected.

Source: Coding guidelines

🤖 Prompt for all review comments with 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.

Inline comments:
In `@src/components/scheduling/ShiftTimeline/CoverageChart.tsx`:
- Around line 265-272: Update the chart container around the option elements to
use role="listbox" instead of role="toolbar", preserving its existing label,
description, orientation, styling, and keyboard handler so the aria-selected
options have the correct ARIA parent.

In `@src/lib/coverageChartModel.ts`:
- Around line 186-192: Update the implied-SPLH aside guard in the exported
coverage chart helper to require both scheduled > 0 and projectedSales > 0
before calculating splhAt and laborPctAt. Keep the existing aside calculation
unchanged for valid positive values, and prevent rendering the aside when
projectedSales is zero, null, or otherwise non-positive.

In `@tests/e2e/coverage-chart-explainer.spec.ts`:
- Around line 30-35: Update todayLocalDateStr to derive the YYYY-MM-DD value in
UTC rather than the host’s local timezone, matching the UTC-pinned restaurant
shifts and Timeline default day. Apply the same UTC date handling to the related
usage near line 69, preserving the existing formatting and assertions.

In `@tests/unit/coverageVerdict.test.tsx`:
- Around line 76-80: Add a separate floor-only verdict fixture in the coverage
verdict tests with demandShortHours set to 0, worstCrit set to null, and a
nonzero floorOnlyHours value. Render CoverageVerdict with this fixture and
assert the floor-only message is shown while “Sales justify” is absent, without
changing the existing SHORT combined-path test.

In `@tests/unit/pickDefaultHour.test.ts`:
- Around line 51-54: Make the tie-break test for pickDefaultHour
order-independent by passing later before earlier while keeping the expectation
of earlier. Update pickDefaultHour to select the lowest startMin when deficit
values tie; do not rename the existing earliest-start contract.

In `@tests/unit/resolveSettingsOverrides.test.ts`:
- Around line 19-24: Extend the resolveSettingsOverrides unit tests with a
negative preview-value case by asserting resolveSettingsOverrides(-1) returns {
target_splh: -1 }. Keep the assertion aligned with the existing positive and
zero-value tests and the helper’s non-null mapping behavior.

In `@tests/unit/splhSlider.test.tsx`:
- Around line 8-23: Update the test setup around BASE_PROPS and renderSlider so
onChange, onSave, and onReset spies are recreated or cleared before each test,
preventing call counts from leaking between tests. Keep renderSlider’s override
behavior and existing assertions unchanged.

---

Nitpick comments:
In `@src/components/scheduling/ShiftTimeline/CoverageReceipt.tsx`:
- Around line 82-107: Move the latestRef.current assignment into a useEffect
declared before the existing hour.startMin announcement effect, so the ref is
updated after commit while preserving access to the latest receipt and hour.
Remove the render-time assignment and keep the commit-event handler’s
empty-dependency behavior unchanged.

In `@src/components/scheduling/ShiftTimeline/ShiftTimelineTab.tsx`:
- Around line 506-510: Update the fallback values in the ShiftTimelineTab logic
around targetLaborPct and lookbackWeeks to use the shared
DEFAULTS.target_labor_pct and DEFAULTS.lookback_weeks constants instead of
hard-coded 22 and 4. Import DEFAULTS from its existing export, or export it from
useStaffingSettings if necessary, while preserving the current fallback
behavior.
- Around line 287-294: The comment above sliderTarget is stale because
SplhSlider is now rendered. Remove the sentence claiming the state is not wired
to a slider UI and that SplhSlider will land in the next task, while preserving
the accurate explanation of sliderTarget and its settingsOverrides data flow.

In `@src/components/scheduling/ShiftTimeline/SplhSlider.tsx`:
- Around line 132-143: Update the range input in SplhSlider to use
clampForDisplay(value) for its value prop, keeping the caller-provided value
unchanged for persistence while ensuring the thumb matches the displayed
readout.

In `@tests/e2e/coverage-chart-explainer.spec.ts`:
- Line 158: Replace the attribute-based locators in the coverage chart test,
including critColumns and the referenced legend and pill locators, with semantic
page.getByRole() or page.getByLabel() selectors. For the hour columns, target
their accessible aria-label names containing “short N on demand,” while
preserving the existing element-specific assertions and interactions.

In `@tests/unit/shiftTimelineTab.test.tsx`:
- Around line 486-489: Rename the test case around pickDefaultHour to a concise
“should [expected behavior] when [condition]” description, removing the
self-correction and setup details from the title. Preserve the existing
assertion and body comment explaining that the larger-deficit 11 AM hour is
selected.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1a8108dd-746c-403a-9c8d-93063e573205

📥 Commits

Reviewing files that changed from the base of the PR and between cf576cf and fdc94e5.

📒 Files selected for processing (27)
  • docs/superpowers/plans/2026-07-23-coverage-chart-explainer-plan.md
  • docs/superpowers/specs/2026-07-23-coverage-chart-explainer-design.md
  • src/components/scheduling/ShiftTimeline/CoverageChart.tsx
  • src/components/scheduling/ShiftTimeline/CoverageReceipt.tsx
  • src/components/scheduling/ShiftTimeline/CoverageStatusStrip.tsx
  • src/components/scheduling/ShiftTimeline/CoverageVerdict.tsx
  • src/components/scheduling/ShiftTimeline/ShiftTimelineTab.tsx
  • src/components/scheduling/ShiftTimeline/SplhSlider.tsx
  • src/lib/coverageChartModel.ts
  • src/lib/coverageSummary.ts
  • src/lib/staffingCalculator.ts
  • src/types/scheduling.ts
  • tests/e2e/coverage-chart-explainer.spec.ts
  • tests/unit/coverageChart.test.tsx
  • tests/unit/coverageChartModel.test.ts
  • tests/unit/coverageReceipt.test.tsx
  • tests/unit/coverageStatusStrip.test.tsx
  • tests/unit/coverageSummary.test.ts
  • tests/unit/coverageVerdict.test.tsx
  • tests/unit/pickDefaultHour.test.ts
  • tests/unit/resolveSettingsOverrides.test.ts
  • tests/unit/shiftTimelineTab.mobileLayout.test.tsx
  • tests/unit/shiftTimelineTab.test.tsx
  • tests/unit/shiftTimelineTabOverlay.test.tsx
  • tests/unit/splhSlider.test.tsx
  • tests/unit/staffingCalculator.test.ts
  • tests/unit/timelineComponents.test.tsx
💤 Files with no reviewable changes (2)
  • tests/unit/coverageStatusStrip.test.tsx
  • src/components/scheduling/ShiftTimeline/CoverageStatusStrip.tsx

Comment on lines +265 to +272
<div
role="toolbar"
aria-label="Hourly coverage chart"
aria-describedby={summaryId}
aria-orientation="horizontal"
className="relative flex-1"
onKeyDown={handleToolbarKeyDown}
>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

ARIA role option required parent listbox aria-selected support

💡 Result:

The WAI-ARIA specification requires that an element with the role of option must be contained within, or owned by, an element with the role of listbox [1][2][3]. This parent-child relationship is essential for assistive technologies to correctly identify and interact with the widget [4][5]. Regarding aria-selected support: 1. Requirement: The aria-selected attribute is a required state for the option role when used within a listbox [6]. 2. Implementation for Single-Select Listboxes: For listboxes that support single selection, authors must explicitly provide aria-selected="true" on the currently selected option [7][8]. Unselected options do not require the attribute, though omitting it is standard practice [7]. 3. Implementation for Multi-Select Listboxes: When multiple options can be selected (typically indicated by aria-multiselectable="true" on the listbox), every selectable option must have the aria-selected attribute explicitly set to either "true" or "false" [7][8]. 4. Best Practices: Authors should not use both aria-selected and aria-checked on the same option element unless the user interface provides distinct, clearly defined methods for controlling each state, as this can cause confusion for assistive technology users [6][9]. Options that are not selectable should not include the aria-selected attribute [7].

Citations:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the relevant component sections without executing repository code.
file="src/components/scheduling/ShiftTimeline/CoverageChart.tsx"
if [ -f "$file" ]; then
  echo "== file exists =="
  wc -l "$file"
  echo
  echo "== outline =="
  ast-grep outline "$file" --view compact || true
  echo
  echo "== relevant lines 230-295 =="
  sed -n '230,295p' "$file" | nl -ba -v230
  echo
  echo "== relevant lines 490-530 =="
  sed -n '490,530p' "$file" | nl -ba -v490
else
  echo "missing $file"
fi

echo
echo "== role occurrences in file =="
rg -n 'role="toolbar"|role="option"|role="listbox"|aria-selected|aria-pressed|aria-activedescendant|role="group"|role="tree"' "$file" || true

Repository: toyiyo/nimble-pnl

Length of output: 472


🏁 Script executed:

#!/bin/bash
set -euo pipefail

file="src/components/scheduling/ShiftTimeline/CoverageChart.tsx"

echo "== file exists =="
wc -l "$file"

echo
echo "== relevant lines 230-295 =="
awk 'NR>=230 && NR<=295 {printf "%5d  %s\n", NR, $0}' "$file"

echo
echo "== relevant lines 490-530 =="
awk 'NR>=490 && NR<=530 {printf "%5d  %s\n", NR, $0}' "$file"

echo
echo "== ARIA role/state occurrences in file =="
rg -n 'role="toolbar"|role="option"|role="listbox"|aria-selected|aria-pressed|aria-activedescendant|role="group"|role="tree"|handleToolbarKeyDown|buttonRefs' "$file" || true

Repository: toyiyo/nimble-pnl

Length of output: 6038


🌐 Web query:

WAI-ARIA option role required owned by listbox group tree

💡 Result:

In WAI-ARIA, an element with the role option must be contained by or owned by an element with the role listbox [1][2][3]. While some technical documentation or specifications may list group as an allowed context role for option [4][5], this is context-dependent: an option may only be placed inside a group if that group is itself contained within a listbox [6][5]. Placing an option inside a group that is not part of a listbox does not satisfy the requirement for the option to be associated with a valid listbox container [6][5]. The tree role, in contrast, requires treeitem roles as children, not option roles [7]. Therefore, option should not be used as a child of a tree [5]. In summary: - The primary required parent for option is listbox [1][2][3]. - option can be nested within a group, provided that the group is a child of a listbox (allowing for categorized options) [8][6][5]. - tree does not use option roles; it uses treeitem [7].

Citations:


Use a listbox parent for option elements.

role="option" with aria-selected currently uses role="toolbar" as its parent; option is meant for listbox containers under ARIA, so this can be ignored by assistive technology. Change the plot-region role to listbox or map the columns to toolbar-appropriate controls, such as aria-pressed buttons.

🤖 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/components/scheduling/ShiftTimeline/CoverageChart.tsx` around lines 265 -
272, Update the chart container around the option elements to use role="listbox"
instead of role="toolbar", preserving its existing label, description,
orientation, styling, and keyboard handler so the aria-selected options have the
correct ARIA parent.

Comment on lines +186 to +192
// Implied SPLH at the scheduled count — only meaningful when someone is
// actually scheduled (avoids a divide-by-zero and a nonsensical "$Infinity/hr").
if (scheduled > 0) {
const splhAt = projectedSales / scheduled;
const laborPctAt = Math.round((wage / splhAt) * 100);
asides.push(`At ${scheduled} scheduled, implied SPLH is ${fmtUsd(Math.round(splhAt))}/hr → ${laborPctAt}% labor.`);
}

Copy link
Copy Markdown
Contributor

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

Guard projectedSales === 0 in the implied-SPLH aside.

The guard only checks scheduled > 0. If projectedSales is 0/null while demand !== null, splhAt becomes 0 and the aside renders $0/hr → Infinity% labor. Upstream summarizeCoverageHours nulls demand when projectedSales <= 0, but this exported helper shouldn't rely on that invariant.

🛡️ Proposed guard
-  if (scheduled > 0) {
+  if (scheduled > 0 && projectedSales > 0) {
     const splhAt = projectedSales / scheduled;
📝 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
// Implied SPLH at the scheduled count — only meaningful when someone is
// actually scheduled (avoids a divide-by-zero and a nonsensical "$Infinity/hr").
if (scheduled > 0) {
const splhAt = projectedSales / scheduled;
const laborPctAt = Math.round((wage / splhAt) * 100);
asides.push(`At ${scheduled} scheduled, implied SPLH is ${fmtUsd(Math.round(splhAt))}/hr → ${laborPctAt}% labor.`);
}
// Implied SPLH at the scheduled count — only meaningful when someone is
// actually scheduled (avoids a divide-by-zero and a nonsensical "$Infinity/hr").
if (scheduled > 0 && projectedSales > 0) {
const splhAt = projectedSales / scheduled;
const laborPctAt = Math.round((wage / splhAt) * 100);
asides.push(`At ${scheduled} scheduled, implied SPLH is ${fmtUsd(Math.round(splhAt))}/hr → ${laborPctAt}% labor.`);
}
🤖 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/lib/coverageChartModel.ts` around lines 186 - 192, Update the
implied-SPLH aside guard in the exported coverage chart helper to require both
scheduled > 0 and projectedSales > 0 before calculating splhAt and laborPctAt.
Keep the existing aside calculation unchanged for valid positive values, and
prevent rendering the aside when projectedSales is zero, null, or otherwise
non-positive.

Comment on lines +30 to +35
/** Host-local YYYY-MM-DD for today — matches the Timeline's default day selection. */
function todayLocalDateStr(): string {
const d = new Date();
const pad = (n: number) => n.toString().padStart(2, '0');
return `${d.getFullYear()}-${pad(d.getMonth() + 1)}-${pad(d.getDate())}`;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Host-local date undercuts the UTC pinning.

The restaurant TZ is pinned to UTC and shifts are inserted as Z instants, but the day string comes from the host's local calendar date. On a machine with a negative/positive UTC offset near midnight, the seeded shift lands on a different UTC day than the Timeline's default-selected day, and the crit-column assertions flake.

🛠️ Proposed fix
-/** Host-local YYYY-MM-DD for today — matches the Timeline's default day selection. */
-function todayLocalDateStr(): string {
-  const d = new Date();
-  const pad = (n: number) => n.toString().padStart(2, '0');
-  return `${d.getFullYear()}-${pad(d.getMonth() + 1)}-${pad(d.getDate())}`;
-}
+/** UTC YYYY-MM-DD for today — matches the UTC-pinned restaurant timezone. */
+function todayUtcDateStr(): string {
+  return new Date().toISOString().slice(0, 10);
+}

Also applies to: 69-69

🤖 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 `@tests/e2e/coverage-chart-explainer.spec.ts` around lines 30 - 35, Update
todayLocalDateStr to derive the YYYY-MM-DD value in UTC rather than the host’s
local timezone, matching the UTC-pinned restaurant shifts and Timeline default
day. Apply the same UTC date handling to the related usage near line 69,
preserving the existing formatting and assertions.

Comment on lines +76 to 80
it('distinguishes floor-only hours from demand-short hours', () => {
const { container } = render(<CoverageVerdict verdict={SHORT} />);
expect(container.textContent).toMatch(/2 hours only trip the 4-person floor/i);
expect(container.textContent).toMatch(/demand there is already met/i);
});

Copy link
Copy Markdown
Contributor

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

Add a floor-only-only verdict case.

SHORT exercises the combined path only. Add a fixture with demandShortHours: 0, worstCrit: null, and nonzero floorOnlyHours; assert the floor message appears and “Sales justify” does not. This protects the distinct floor-only verdict branch.

🤖 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 `@tests/unit/coverageVerdict.test.tsx` around lines 76 - 80, Add a separate
floor-only verdict fixture in the coverage verdict tests with demandShortHours
set to 0, worstCrit set to null, and a nonzero floorOnlyHours value. Render
CoverageVerdict with this fixture and assert the floor-only message is shown
while “Sales justify” is absent, without changing the existing SHORT
combined-path test.

Comment on lines +51 to +54
it('ties on deficit break by earliest startMin', () => {
const earlier = hourFixture({ startMin: 600, demand: 6, scheduled: 4 }); // deficit 2
const later = hourFixture({ startMin: 660, demand: 6, scheduled: 4 }); // deficit 2 (tie)
expect(pickDefaultHour([earlier, later], 4)).toBe(earlier);

Copy link
Copy Markdown
Contributor

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

Make the tie-break test order-independent.

This input already puts the earlier hour first, so it only verifies “first encountered,” not the stated earliest-startMin rule. Put later before earlier; then either implement that tie-break in pickDefaultHour or rename the contract to first-encountered.

🤖 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 `@tests/unit/pickDefaultHour.test.ts` around lines 51 - 54, Make the tie-break
test for pickDefaultHour order-independent by passing later before earlier while
keeping the expectation of earlier. Update pickDefaultHour to select the lowest
startMin when deficit values tie; do not rename the existing earliest-start
contract.

Comment on lines +19 to +24
it('returns { target_splh: sliderTarget } when sliderTarget is a preview value', () => {
expect(resolveSettingsOverrides(60)).toEqual({ target_splh: 60 });
});

it('treats 0 as a real preview value, not "unset" (0 !== null)', () => {
expect(resolveSettingsOverrides(0)).toEqual({ target_splh: 0 });

Copy link
Copy Markdown
Contributor

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

Cover negative numeric preview input.

Add resolveSettingsOverrides(-1) and assert { target_splh: -1 }, matching the helper’s non-null mapping contract. As per coding guidelines, “Test edge cases: empty, null, undefined, zero, negative values in unit tests.”

🤖 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 `@tests/unit/resolveSettingsOverrides.test.ts` around lines 19 - 24, Extend the
resolveSettingsOverrides unit tests with a negative preview-value case by
asserting resolveSettingsOverrides(-1) returns { target_splh: -1 }. Keep the
assertion aligned with the existing positive and zero-value tests and the
helper’s non-null mapping behavior.

Source: Coding guidelines

Comment on lines +8 to +23
const BASE_PROPS = {
value: 50,
wage: 15,
targetLaborPct: 25,
canSave: true,
isSaving: false,
onChange: vi.fn(),
onSave: vi.fn(),
onReset: vi.fn(),
};

function renderSlider(overrides: Partial<typeof BASE_PROPS> = {}) {
const props = { ...BASE_PROPS, ...overrides };
render(<SplhSlider {...props} />);
return props;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Shared module-scope vi.fn() mocks are never cleared between tests.

BASE_PROPS holds one instance of each spy for the whole file, so toHaveBeenCalledTimes(1) assertions depend on no other test having triggered that handler. Create the spies per render (or clear in beforeEach).

As per path instructions for tests/unit/**/*.test.{ts,tsx}: "Mock external dependencies (Supabase, API calls) and clear mocks in beforeEach".

♻️ Proposed fix
-const BASE_PROPS = {
+const baseProps = () => ({
   value: 50,
   wage: 15,
   targetLaborPct: 25,
   canSave: true,
   isSaving: false,
   onChange: vi.fn(),
   onSave: vi.fn(),
   onReset: vi.fn(),
-};
+});
 
-function renderSlider(overrides: Partial<typeof BASE_PROPS> = {}) {
-  const props = { ...BASE_PROPS, ...overrides };
+function renderSlider(overrides: Partial<ReturnType<typeof baseProps>> = {}) {
+  const props = { ...baseProps(), ...overrides };
   render(<SplhSlider {...props} />);
   return props;
 }
📝 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
const BASE_PROPS = {
value: 50,
wage: 15,
targetLaborPct: 25,
canSave: true,
isSaving: false,
onChange: vi.fn(),
onSave: vi.fn(),
onReset: vi.fn(),
};
function renderSlider(overrides: Partial<typeof BASE_PROPS> = {}) {
const props = { ...BASE_PROPS, ...overrides };
render(<SplhSlider {...props} />);
return props;
}
const baseProps = () => ({
value: 50,
wage: 15,
targetLaborPct: 25,
canSave: true,
isSaving: false,
onChange: vi.fn(),
onSave: vi.fn(),
onReset: vi.fn(),
});
function renderSlider(overrides: Partial<ReturnType<typeof baseProps>> = {}) {
const props = { ...baseProps(), ...overrides };
render(<SplhSlider {...props} />);
return props;
}
🤖 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 `@tests/unit/splhSlider.test.tsx` around lines 8 - 23, Update the test setup
around BASE_PROPS and renderSlider so onChange, onSave, and onReset spies are
recreated or cleared before each test, preventing call counts from leaking
between tests. Keep renderSlider’s override behavior and existing assertions
unchanged.

Source: Path instructions

The CoverageVerdict rework changed the no-demand prompt to
"Add staffing targets to see where sales justify more hands."
Point the shiftTimelineTab wiring test at the new copy so the
Unit Tests CI job goes green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/components/scheduling/ShiftTimeline/ShiftTimelineTab.tsx:293

  • This comment block still says the SPLH slider state is "Not yet wired to a slider UI", but the slider is now rendered later in this component. Updating/removing this avoids confusing future readers about whether sliderTarget is actually used.
  // On-chart SPLH slider's live-preview value (design doc §B/§E). `null` means
  // "no preview — use the saved `target_splh`"; a number is the in-progress
  // drag value, fed to `useWeekStaffingSuggestions` via `settingsOverrides` so
  // the whole pipeline (chart/receipt/verdict) redraws off ONE source of
  // truth. Not yet wired to a slider UI (Stage 4.1's `SplhSlider` render lands
  // in the next task) — this only satisfies the state/plumbing half of the
  // contract.

docs/superpowers/specs/2026-07-23-coverage-chart-explainer-design.md:55

  • This design doc says calculateRecommendedStaff "stays authoritative", but the implementation in this PR removes that exported helper from src/lib/staffingCalculator.ts and inlines the math in buildHourlyRecommendations. Either keep the helper or update the doc to describe the math without referencing a function that no longer exists.
- No change to the staffing math itself (`calculateRecommendedStaff` stays
  authoritative — `max(ceil(sales/splh), minStaff)`).
- No change to how `unified_sales` → `aggregateHourlySales` →

Comment on lines +94 to 103
if (demandShortHours === 0 && floorOnlyHours === 0) {
return (
<div className="flex items-center gap-2 py-1">
<span
aria-hidden
className="h-2.5 w-2.5 flex-shrink-0 rounded-full bg-success"
/>
<p className="text-[15px] font-medium text-foreground">
Meeting demand all day.
<div className="space-y-2.5">
<p className="font-serif text-[19px] leading-snug text-foreground max-w-[62ch]">
Every hour is covered — you're meeting demand all day.
</p>
<VerdictChips verdict={verdict} />
</div>
);
}
…rs read as separate bars

The "no sales history" hatch rect spanned the full column width while every
drawn data bar is inset by pad = width * 0.16, so consecutive nodata hours
abutted into one continuous hatched block instead of discrete bars. Hoist the
pad/bx/bw inset above the nodata branch and apply it to the ghost too, matching
the mock's visually-separated bars. Adds a regression test pinning the ghost's
inset geometry to the data bars' width.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

src/components/scheduling/ShiftTimeline/SplhSlider.tsx:143

  • <input type="range"> is controlled with value={value} but value can be outside [SPLH_SLIDER_MIN, SPLH_SLIDER_MAX] since target_splh is not bounded elsewhere. Browsers clamp out-of-range range values, which can desync the displayed $/hr + implied labor % (and SR output) from the slider's actual DOM value/position. Please ensure the slider and saved settings share the same bounds, or handle out-of-range saved values explicitly.
        <input
          id={sliderId}
          type="range"
          min={SPLH_SLIDER_MIN}
          max={SPLH_SLIDER_MAX}
          step={SPLH_SLIDER_STEP}
          value={value}
          onChange={(e) => onChange(Number(e.target.value))}
          aria-label="Sales per labor hour target, in dollars"
          aria-valuetext={`$${value}/hr → ${pctLabel}% labor`}
          className="w-full accent-primary"
        />

src/components/scheduling/ShiftTimeline/ShiftTimelineTab.tsx:293

  • This comment says the SPLH preview state is “not yet wired to a slider UI”, but the slider is now rendered later in this component. Updating the comment will avoid confusing future readers.
  // drag value, fed to `useWeekStaffingSuggestions` via `settingsOverrides` so
  // the whole pipeline (chart/receipt/verdict) redraws off ONE source of
  // truth. Not yet wired to a slider UI (Stage 4.1's `SplhSlider` render lands
  // in the next task) — this only satisfies the state/plumbing half of the
  // contract.

src/components/scheduling/ShiftTimeline/ShiftTimelineTab.tsx:301

  • This comment still mentions the removed “status strip” updating during drag. The strip was removed in this PR, so the comment should be updated to reflect the current UI surfaces (chart/verdict/receipt).
  // The in-flight drafted range for a bar currently being dragged/resized, or
  // null when no drag is in progress. Merged into the model input below so
  // the coverage chart/verdict/status strip update live during the drag
  // (design doc §4 "Draft state"). Never written to React Query/localStorage —
  // plain React state, cleared on commit/cancel.

src/components/scheduling/ShiftTimeline/CoverageReceipt.tsx:107

  • There’s stray whitespace/blank line inside this effect body (line 106). This can trip no-trailing-spaces/formatting rules and is easy to clean up.
  useEffect(() => {
    setAnnounced(announcementText(latestRef.current.receipt, latestRef.current.hour));
     
  }, [hour.startMin]);

…n't dropped

The hourly-sales query in useWeekStaffingSuggestions was an unbounded select,
subject to PostgREST's default 1000-row cap. A busy restaurant's multi-week
lookback exceeds that, and because rows are ordered by sale_date ascending the
truncation silently drops the most-recent days first — including today, the
default-selected day — so the coverage chart rendered every hour as
"No sales history".

Paginate in 1000-row pages (matching the adjacent time-punch query and
useSplhData's fetchAllSales) so the full lookback window is fetched.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/components/scheduling/ShiftTimeline/CoverageVerdict.tsx:103

  • The "Everything covered" branch is triggered solely by demandShortHours === 0 && floorOnlyHours === 0, which ignores nodataHours (and also ignores metAll / shortHours). This can render "Every hour is covered" even when all hours are nodata (no sales history) or when metAll is false due to delta < 0 on nodata hours.
  const {
    hasDemand,
    minStaff,
    demandShortHours,
    demandShortPeopleHours,
    worstCrit,
    floorOnlyHours,
  } = verdict;

  if (!hasDemand) {
    return (
      <p className="font-serif text-[19px] leading-snug text-muted-foreground max-w-[62ch]">
        Add staffing targets to see where sales justify more hands.
      </p>
    );
  }

  if (demandShortHours === 0 && floorOnlyHours === 0) {
    return (
      <div className="space-y-2.5">
        <p className="font-serif text-[19px] leading-snug text-foreground max-w-[62ch]">
          Every hour is covered — you're meeting demand all day.
        </p>
        <VerdictChips verdict={verdict} />
      </div>
    );
  }

docs/superpowers/specs/2026-07-23-coverage-chart-explainer-design.md:55

  • This design doc says calculateRecommendedStaff “stays authoritative”, but the PR removes that exported function from src/lib/staffingCalculator.ts and inlines the math in buildHourlyRecommendations. Update the doc to reflect the current implementation so future readers aren’t misled.
- No change to the staffing math itself (`calculateRecommendedStaff` stays
  authoritative — `max(ceil(sales/splh), minStaff)`).
- No change to how `unified_sales` → `aggregateHourlySales` →

Comment on lines +128 to +151
const PAGE_SIZE = 1000;
const MAX_PAGES = 20;
const rows: HourlySaleRow[] = [];
for (let page = 0; page < MAX_PAGES; page++) {
const from = page * PAGE_SIZE;
const { data, error } = await supabase
.from('unified_sales')
.select('sale_date, sale_time, sold_at, total_price')
.eq('restaurant_id', restaurantId)
.eq('item_type', 'sale')
// Split-sale guard (§5 S-M1): exclude split-parent/child rows so a
// split sale's total isn't summed twice, matching useSplhData.ts.
.is('parent_sale_id', null)
.gte('sale_date', dateRange.startStr)
.lte('sale_date', dateRange.endStr)
.order('sale_date')
.order('created_at')
.order('id')
.range(from, from + PAGE_SIZE - 1);
if (error) throw error;
rows.push(...((data ?? []) as unknown as HourlySaleRow[]));
if (!data || data.length < PAGE_SIZE) break;
}
return rows;
@sonarqubecloud

Copy link
Copy Markdown

@jdelgado2002
jdelgado2002 merged commit 9d608bb into main Jul 26, 2026
29 of 35 checks passed
@jdelgado2002
jdelgado2002 deleted the feature/coverage-chart-explainer branch July 26, 2026 04:16
jdelgado2002 added a commit that referenced this pull request Jul 28, 2026
…cated wage (#663)

* docs(staffing): revise SPLH consistency design to build on PR #650

Reuse impliedLabor/laborConsistentSplh instead of adding a parallel helper;
scope to the three surfaces #650 left uncovered (Settings, Planner panel,
and the slider's fabricated-$15 wage readout).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs(staffing): fold second design review — semantic tokens, aria-valuetext, receipt gating

- text-warning instead of raw amber-600 (CLAUDE.md: no direct colors)
- gate aria-valuetext and buildReceipt's aside on hasWageData (same
  fabricated-$15 wage leaked through both)
- hint under both Settings inputs (separate cards, single-column at 375px)
- short slider fallback copy for the whitespace-nowrap row

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs(staffing): revised 6-task plan building on PR #650

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* refactor(staffing): move implied-labor math to staffingCalculator, add wage-data predicate

Moves impliedLabor/laborConsistentSplh (and ImpliedLaborResult) from
coverageChartModel.ts into staffingCalculator.ts, next to
computeAvgHourlyRateCents which feeds them — the staffing-domain home now
that three surfaces depend on this math (PR #650's coverage chart, plus
the upcoming Planner panel and Settings tab). Behavior is unchanged: same
0.05pt tolerance, wage still in dollars.

Also adds hasHourlyWageData(employees), a predicate mirroring
computeAvgHourlyRateCents's own hourly+active filter, so callers can tell
a real blended wage apart from the $15/hr DEFAULT_HOURLY_RATE_CENTS
fallback before presenting an implied-labor readout as fact.

Note: SplhSlider.tsx still imports the two functions from
coverageChartModel.ts and tests/unit/coverageChartModel.test.ts still
tests them there — both are updated in the next task (Task 1 Step 4/5)
to point at the new location.

* fix(scheduling): repoint SplhSlider import at staffingCalculator

impliedLabor/laborConsistentSplh moved to staffingCalculator.ts in the
prior commit; update the consumer's import and JSDoc reference to match.

* test(staffing): remove duplicated impliedLabor/laborConsistentSplh specs from coverageChartModel.test.ts

Those two functions moved to staffingCalculator.ts in aa99206, which also
carried their test blocks into staffingCalculator.test.ts as part of keeping
that commit green. coverageChartModel.test.ts still imported the now-removed
exports and re-declared the same describe blocks, so its suite was failing
(TypeError: not a function) with a stale duplicate of coverage already
present elsewhere. Drop the dead import members and the duplicate blocks;
classifyHour/buildReceipt/chartSummaryLabel coverage is untouched.

* fix(scheduling): gate SplhSlider readout/pill/notch/aria-valuetext on hasWageData

Add the required hasWageData prop to SplhSlider. When false (avgWage is
the $15/hr fallback, not real roster data), suppress the implied-labor
readout, the over/under-target pill, and the labor-consistent notch,
showing an "Add hourly rates" prompt instead, and drop the fabricated
"% labor" suffix from aria-valuetext.

Known intermediate breakage (fixed by the next step, not yet run):
ShiftTimelineTab.tsx does not yet pass hasWageData to <SplhSlider>, so
`npm run typecheck` currently fails on that one call site.

* fix(scheduling): wire hasWageData into SplhSlider from ShiftTimelineTab

Passes hasHourlyWageData(employees) through to SplhSlider so the
implied-labor readout, pill, notch, and aria-valuetext are suppressed
whenever avgWage is computeAvgHourlyRateCents's $15/hr fallback rather
than real roster data.

* fix(scheduling): gate receipt implied-SPLH aside on hasWageData in buildReceipt

Thread hasWageData through buildReceipt's params and reuse impliedLabor
(from staffingCalculator) for the aside's pct math, so the receipt's
implied-SPLH note is suppressed when wage is the $15/hr fallback rather
than real roster data (design §4a2).

CoverageReceipt.tsx doesn't pass hasWageData through yet (next step) —
its call site now fails typecheck as expected, and the two asides tests
depending on it are correspondingly red until that wiring lands.

* fix(scheduling): add hasWageData prop to CoverageReceipt, wire from ShiftTimelineTab

* fix(scheduling): expose avgHourlyRateCents + hasWageData from useWeekStaffingSuggestions

Derives hasWageData via hasHourlyWageData(employees) and returns both it and
the already-computed avgHourlyRateCents so downstream consumers (SplhSlider,
CoverageReceipt) can gate implied-% readouts on real wage data instead of
the $15/hr fallback.

* fix(scheduling): add avgHourlyRateCents/hasWageData props + splhHint derivation to StaffingConfigPanel

Adds the two new props to StaffingConfigPanelProps and derives splhHint
via useMemo, reusing impliedLabor/laborConsistentSplh from
staffingCalculator. Gated on hasWageData and Number.isFinite(x) && x > 0
for both SPLH and labor-% (a cleared field parses to NaN).

Not yet rendered — that's Task 5 Step 4. Expected intermediate breakage:
4 StaffingConfigPanel.splhConsistency tests stay red until the hint
markup renders, and StaffingOverlay.tsx has one missing-props typecheck
error until Step 5 wires the props through.

* fix(scheduling): render SPLH consistency hint in StaffingConfigPanel

Renders the splhHint block (Step 3) directly after the actualSplh readout
in the SPLH column: implied labor % at the blended wage, warning styling
+ suggested labor-consistent SPLH when over target, and a directional
helper line — announced via aria-live="polite".

* fix(scheduling): wire avgHourlyRateCents/hasWageData from StaffingOverlay to StaffingConfigPanel

* fix(scheduling): derive splhHint in RestaurantSettings from useEmployees

Adds useEmployees + staffingCalculator (computeAvgHourlyRateCents,
hasHourlyWageData, impliedLabor, laborConsistentSplh) imports to
RestaurantSettings.tsx and derives splhHint via useMemo from the
labor-planning form's SPLH/labor-% fields and the roster's real hourly
wage. Null when the roster has no hourly wage data or either field is
blank/non-numeric.

Rendering the hint markup is the next task step (out of scope here),
so restaurantSettings.splhConsistency.test.tsx stays at the expected
intermediate state: 2 failed / 1 passed.

* fix(scheduling): render SPLH consistency hint under both labor-planning fields

Extract splhHintBlock from the splhHint memo and render it directly
under both the Target SPLH and Target Labor % field help text, so a
manager editing either field sees the warning without scrolling
(design §4c).

* refactor(staffing): extract deriveSplhHint to dedupe the SPLH-hint guard

StaffingConfigPanel and RestaurantSettings each computed the same
"implied labor % at this SPLH target" hint independently — same
positive-number guard, same impliedLabor/laborConsistentSplh call
shape, just different input sources (props vs. parsed form state).
Centralize it in staffingCalculator.ts as deriveSplhHint so the two
surfaces can't drift. No behavior change.

* fix(review): exclude unset/zero hourly_rate from hasHourlyWageData — addresses codex

EmployeeDialog saves a blank hourly-rate field as 0 cents. A roster whose
only active hourly employee has an unset rate made hasHourlyWageData true
while computeAvgHourlyRateCents returned 0, fabricating a "0% labor" /
"On target" readout on the slider/settings/planner hints instead of the
intended "Add hourly rates" suppression.

* fix(scheduling): reject invalid wageCents in deriveSplhHint

CodeRabbit review: hasWageData:true with wageCents of 0/NaN/Infinity
produced a fabricated 0%/non-finite hint. Add wageCents to the
finite-positive guard and cover it with direct deriveSplhHint tests.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(scheduling): triage PR #663 review — wage-set agreement, zero-sales aside

- computeAvgHourlyRateCents and hasHourlyWageData now derive from one
  paidHourlyEmployees set, so a $20/hr + unset-rate roster can no longer
  advertise a fabricated $10/hr blended wage as real (Codex P2)
- omit buildReceipt's implied-SPLH aside when projectedSales is 0: staff on
  the clock with no sales is infinitely over target, but the divide-by-zero
  guard reported 0% — backwards (Copilot)
- refresh CoverageReceipt's aria-live text when hasWageData flips (CodeRabbit)
- drop 'any' from the Supabase mock chain; drop a CRITICAL: prefix from a
  routine assertion; cover unset/negative rates in the hook (CodeRabbit)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
jdelgado2002 added a commit that referenced this pull request Jul 28, 2026
Three lessons: check origin/main for the concept before designing (this
branch rebuilt math PR #650 had already shipped); share the selector when
two functions must agree on a set; re-derive a reused helper's degenerate
fallback at each new call site.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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