jacobh2
014503e89b
The planner knew exactly how many semesterdagar a parent was about to forfeit and never placed a single day to prevent it. It also had no weekend-wrap logic at all: REASON_VACATION_WRAP_WEEKEND was stamped on every tier-3 interior vacation day, including a lone Tuesday in February. Two changes. Spend better. A workday adjacent to a free stretch, or in the middle of a gap of three or fewer fenced by free days, now takes vacation -- _vacation_bridge_reason, consulted for every parent type. This departs from spec 6.1's "wrapping suppressed for hourly": the FP-preservation argument is salary-type independent, only the pay argument was hourly-moot. In a both-parents window, both now take vacation on such a date rather than one paying with FP. After funding, reallocate_vacation_by_value swaps vacation onto higher-ranked dates 1-for-1 with FP days, so which dates hold vacation improves while how many never drops -- greedy first, re-timed after, never withheld. An invariant test asserts the count can only rise. Name the losses. A new pass proposes concrete dates for vacation that would otherwise expire, current year only, ranked red-day bridge then weekend bridge then interior, later dates first. They ride on PlanProposal.rescue_days, kept off `days` so nothing downstream commits them, and surface on the Dashboard with accept-one, accept-all (one batched /api/plan/apply, not N) and dismiss. Dismissal reuses the tip_dismissal table, keyed on (parent, year) identity rather than the group's dates -- those are recomputed from the whole plan's residual on every propose, so date-hashing made dismissal self-defeating. Dismiss does not bump the plan version; it changes no data and must not trigger a write. Measured on the seeded demo: 12 forfeiting days for one parent now yield 12 proposed dates, all Fridays or Mondays. Total vacation placed is unchanged at 203 days, confirming the reallocation only re-times. Three defects here were found by driving a browser, while 2273 unit tests passed -- all three lived in the seam between layers that unit tests mock away. Adds API-level tests over the real request sequence. Rate limiting is now off in tests (one file consumed the whole registration budget) with the suite's first real 429 assertion. PROPOSE_VS_SYNC_BUG.md documents a pre-existing, unfixed issue found along the way: Dashboard figures swing on the first sync with no user action. Backend 946 tests, frontend 1341, coverage above all gates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZaX6xbuMEMXuyKA1fq2Mm
78 lines
3.8 KiB
Markdown
78 lines
3.8 KiB
Markdown
# Bug: Dashboard figures swing on the first plan sync
|
|
|
|
**Status:** open, not fixed. Found 2026-09-20 during browser verification of the vacation-rescue
|
|
feature. **Pre-existing** — not introduced by that work.
|
|
|
|
## Symptom
|
|
|
|
The vacation figures on the Dashboard change substantially the first time
|
|
`POST /api/plan/sync` runs, with **zero user interaction** — an ordinary page reload is enough.
|
|
|
|
Measured on a freshly seeded demo workspace (`backend/scripts/seed_demo.py`), 2026-09-20:
|
|
|
|
| Figure | Before sync | After sync |
|
|
|---|---|---|
|
|
| Total "vacation at risk of being lost" | 178 | 24 |
|
|
| Sam 2026 — vacation left | 23 | 17 |
|
|
| Sam 2026 — expiring | 18 | 12 |
|
|
| Sam 2027 — expiring | 20 | 2 |
|
|
| Sam 2028 — expiring | 20 | 3 |
|
|
|
|
## Cause
|
|
|
|
Two different numbers are both correct, for different questions, and the UI shows them in
|
|
sequence without saying which is which:
|
|
|
|
1. `usePlanProposal` (`frontend/src/hooks/usePlanProposal.ts`) initially renders the result of
|
|
`POST /api/plan/propose` — a **read-only** projection that writes nothing.
|
|
2. `PlanSyncProvider` (`frontend/src/context/PlanSyncProvider.tsx`) then runs
|
|
`POST /api/plan/sync`, which **persists** the proposed days via
|
|
`PlanningService.sync_plan` (`backend/app/services/planning_service.py`).
|
|
3. Once those days exist in `planned_leave_day`, `AllocationInputLoader` counts them as committed
|
|
usage when it builds `vacation_forfeiting`, so the next forecast is much smaller.
|
|
4. `usePlanProposal` permanently prefers `PlanSyncProvider.latestProposal` once any sync has
|
|
completed, so the post-sync figure is what sticks.
|
|
|
|
So the pre-sync figure answers *"what would be lost if nothing were planned?"* and the post-sync
|
|
figure answers *"what will be lost given the plan we just wrote?"*. Only the second is what the
|
|
user means by "days I'm about to lose".
|
|
|
|
## Why it matters
|
|
|
|
It reads as a bug in whatever the user just did. During verification, the vacation-rescue
|
|
**dismiss** action was initially blamed for it — dismiss was the first Dashboard control to call
|
|
`bumpPlanVersion()`, so it triggered the sync and appeared to move the numbers.
|
|
|
|
Dismiss was **exonerated** by capturing figures immediately before and after the click: with
|
|
dismiss no longer bumping the plan version, the figures are byte-identical across a dismiss
|
|
(178 / 23 / 18 before and after). The swing only ever came from the sync.
|
|
|
|
## Suggested direction (not decided)
|
|
|
|
Options, roughly in increasing order of effort:
|
|
|
|
1. Don't render vacation figures until the first sync has completed — show the existing loading
|
|
skeleton instead. Smallest change; makes the transient state invisible rather than explained.
|
|
2. Have the Dashboard sync before first paint rather than propose-then-sync, so only one number is
|
|
ever shown.
|
|
3. Reconsider whether `sync_plan` should write on a plain page load at all. It is a persisting
|
|
write triggered by navigation, which is what makes a read of the Dashboard change stored data.
|
|
|
|
Option 3 is the real question and is the largest; 1 is a stopgap that would stop the user seeing a
|
|
figure that is about to change by 7x.
|
|
|
|
## Reproduction
|
|
|
|
1. Seed a fresh demo workspace and log in as `demo@example.com`.
|
|
2. Open the Dashboard and read the "vacation at risk" total **immediately**, before the background
|
|
sync completes (it is quick — capture it fast, or throttle the network).
|
|
3. Reload the page and read it again.
|
|
|
|
## Related, probably not a real defect
|
|
|
|
Chrome's network panel reports `POST /api/plan/rescue-days/dismiss` as **503** while uvicorn's own
|
|
access log shows **204 No Content** for the same call, with one log line per click and no retries
|
|
reaching the server. Server behaviour is verifiably correct. Looks like an artifact of the
|
|
browser extension's request tracking, possibly around React 18 dev-mode effect double-invocation.
|
|
Recorded only so it isn't re-investigated from scratch.
|