# REVIEW_02 — Simplification & maintainability audit

Date: 2026-05-08
Scope: full repo at `main` (commit `1dba877`), against SPEC.md as of the
same SHA. Sister document to `doc/REVIEW_01.md` — that one captures
security and "fishy patterns"; this one captures **overcomplicated
solutions that could be rewritten more simply with no functional change.**

This document is meant to be referenced on each refactor loop alongside
SPEC.md and REVIEW_01.md. Findings are ordered by severity (highest
first) and each carries:

- **ID**: stable handle (`R02-Nxx`) — quote when committing a fix.
- **Severity**: HIGH / MEDIUM / LOW. Severity here measures
  _maintenance pain_, not security. HIGH = a real bug surface (already
  caused regressions, or two-place edits routinely diverge); MEDIUM =
  meaningful churn / readability / tested-once-then-forgotten; LOW =
  cosmetic / churn for churn's sake unless the area is being touched
  anyway.
- **Status**: `open` / `accepted-by-design` / `fixed-in-<sha>`.
- **Where**: file paths + line ranges (current; will shift after edits).
- **What**: the pattern that's overcomplicated.
- **Why it's worth fixing**: what breaks today, or what the next
  contributor will trip over.
- **Suggested rewrite**: shape of the change, not a full diff. Keep the
  spec invariants intact: vanilla JS, strict CSP, Twig auto-escape,
  audit-row-per-mutation, capacity math under §5.

When a finding is closed, set Status to `fixed-in-<sha>` and append a
short note. Do not delete entries — they're history, same contract as
REVIEW_01.

---

## How to use this with /loop

> **Before starting on any finding, read `SPEC.md` end-to-end.** It is
> the contract these refactors must preserve — vanilla JS, strict CSP,
> Twig auto-escape, audit-row-per-mutation, the capacity math in §5, the
> audit logging rules in §7, and the build-phase log in §9 (which often
> records *why* the "complicated" pattern landed that way). A
> simplification that silently breaks any of these is a regression, not
> a fix. Most findings here are pure internals so SPEC.md will not need
> editing — but you can only know that after reading it.

1. Read `SPEC.md` in full (at minimum §5, §7, §9). Skip this step and
   you will land R02-N02-style hotfixes a week later.
2. Pick the lowest-numbered `open` finding whose severity ≥ MEDIUM.
3. Apply the rewrite, add/adjust tests, update SPEC.md if a public
   contract changes.
4. Commit; flip Status to `fixed-in-<sha>` and add a one-line note.
5. Commit the doc update separately.

---

## HIGH

### R02-N01 — Capacity formula duplicated in PHP and JS
- **Severity**: HIGH (already a documented two-place-edit hazard).
- **Status**: open.
- **Where**:
  - `src/Services/CapacityCalculator.php` — authoritative implementation.
  - `public/assets/js/sprint-planner.js` lines 172–222 — `recomputeRow()`
    + `applyServerCapacity()` reproduce the same `round_half`,
    `after_reserves`, `committed_p1`, `available` chain on the client.
  - SPEC.md §5 calls this out explicitly: _"Runs identically in
    `App\Services\CapacityCalculator` (PHP) and in `sprint-planner.js`
    (JS). Any edit must touch both."_
- **What**: The capacity rule (the core domain invariant of the app)
  lives in two places. The JS copy exists so the on-screen totals
  update during typing without waiting for the server round-trip. Every
  PATCH response already returns the freshly computed `per_worker`
  values though, and `applyServerCapacity()` is called immediately
  after each save (lines 249, 559, 612).
- **Why it's worth fixing**: any future change to capacity (a new
  reserve type, a different rounding rule, priority-3 tasks, anything)
  must be made twice. The invariant is the kind of thing that gets
  changed during product evolution; missing one side has produced subtle
  display bugs in similar projects historically.
- **Suggested rewrite**: drop `recomputeRow()`'s arithmetic. Two paths:
  - **Path A (preferred):** keep typing-responsive feel by relying on
    the existing 400 ms debounce — the user usually pauses before
    expecting Available to update anyway. Render `Available` from
    server values only; on edit, optionally show a "saving…" indicator.
  - **Path B:** keep an immediate visual update but make it a *plain
    sum* (no reserve arithmetic) so the formula stays single-source.
    Server PATCH response is authoritative for Reserves/Available.
  Either way the JS file shrinks by ~30 lines and the spec's "any edit
  must touch both" line in §5 goes away.

### R02-N02 — `buildTaskRow()` mirrors the Twig partial by hand
- **Severity**: HIGH (already the source of two phase-bound hotfixes).
- **Status**: fixed-in-d5a09ff. Row markup extracted to
  `views/sprints/_task_row.twig`; `_task_list.twig` includes it once per
  task and once more inside a hidden `<template
  data-task-row-template>` (admin-only). `buildTaskRow` now clones the
  template and only populates the per-task fields — ~150 lines of
  hand-rolled DOM gone, along with the `ownerChoices()` /
  `sprintWorkerHeaders()` helpers that only fed it. TwigViewTest pins
  the template's presence (admin path) and absence (read-only path).
- **Where**: `public/assets/js/sprint-planner.js` ~lines 361–510 —
  `buildTaskRow(task, ...)`.
  - Twig source of truth: `views/sprints/_task_list.twig` lines 200–283.
  - Historical regressions documented in SPEC.md §9 Phase 10:
    - hotfix `7c298d3` — owner dropdown empty in JS-built rows because
      `ownerChoices()` scraped the pre-Phase-10 `[data-owner-filter]
      option` selector that no longer existed.
    - hotfix `23ab365` — JS-built cells missed `data-col`, breaking the
      Columns dropdown and Phase 13 focus auto-hide.
  - Plus the `whitespace-nowrap` re-mirror in the "Task table polish"
    commit `f204611`.
- **What**: when an admin clicks "+ Add task", the new row is built in
  JS to avoid a page reload. The function manually emits ~15 data
  attributes + ~10 CSS classes that the Twig partial already emits on
  server-rendered rows. Every time the partial changes, this function
  silently drifts.
- **Why it's worth fixing**: it's a recurring foot-gun. The partial is
  the spec for what a task row looks like; `buildTaskRow` is a
  forgetful copy that has bitten the project at least three times.
- **Suggested rewrite**: server-rendered hidden `<template
  data-task-row-template>` block at the bottom of `_task_list.twig`
  containing one rendered row with placeholder values (or just the
  blank shell — the JS already fills the data). On task creation:
  ```js
  const tr = template.content.firstElementChild.cloneNode(true);
  tr.dataset.taskId = data.task.id;
  // …apply server-returned task fields
  ```
  Eliminates the manual mirror. The template renders inside the
  existing autoescape, so no XSS surface change. ~80 LoC removed from
  `sprint-planner.js`. New test: assert the template element exists in
  `_task_list.twig` (drift guard, same shape as the autoescape pin
  R01-N21).

### R02-N03 — Form-endpoint admin+CSRF guard inlined ~7×
- **Severity**: HIGH (the JSON path already has the helper; form path
  doesn't, and it's the same boilerplate). Real fix is small but stops
  ongoing copy-paste.
- **Status**: open.
- **Where**:
  - `src/Controllers/SprintController.php::create` lines 91–99
  - `…::delete` lines ~1020–1026
  - `src/Controllers/WorkerController.php::create`/`::update`
  - `src/Controllers/UserController.php::update`/`::tombstone`
  - `src/Controllers/SettingsController.php::update`
  - `src/Controllers/ImportController.php::upload`/`::commit`
  Existing parallel: `SprintController::gateJsonAdmin()` at line
  1169 — used 8× across the JSON endpoints (lines 349, 510, 589, 649,
  705, 762, 806, 928). The pattern works. The form path just lacks the
  twin.
- **What**: every form-handling action begins with the same three
  lines:
  ```php
  $actor = SessionGuard::requireAdmin($this->users);
  if ($actor instanceof Response) { return $actor; }
  if (!SessionGuard::verifyCsrf($req)) { return Response::text('CSRF token invalid', 403); }
  ```
- **Why it's worth fixing**: ~7 sites, three lines each, all identical.
  More importantly: a developer adding a new form endpoint can forget
  the CSRF check or the admin gate and the omission won't be obvious in
  review (the surrounding code looks fine). Both the JSON
  (`gateJsonAdmin` returning a JSON envelope) and the form (a
  `Response::text` 403) flavours are reasonable; missing either is a
  vulnerability.
- **Suggested rewrite**: add `SessionGuard::requireAdminForm(Request,
  UserRepository): User|Response` mirroring `requireAdminJson` (already
  at SessionGuard:256). It runs both the admin check and CSRF, returns
  the user on success or a 403 text response on either failure. Each
  controller method drops 4 lines to one. `gateJsonAdmin` in
  `SprintController` can also move into `SessionGuard` for symmetry —
  it's not sprint-specific.

---

## MEDIUM

### R02-N04 — Three near-identical debounced-batch-save pipelines
- **Severity**: MEDIUM.
- **Status**: open.
- **Where**: `public/assets/js/sprint-planner.js`
  - Arbeitstage cells: lines 228–258 (`pendingCells`/`cellDebounce`/
    `queueCell`/`flushCells`)
  - Task assignment days: ~lines 590–615 (`pendingAssign`/`assignTimers`)
  - Task assignment status: ~lines 644–668 (`pendingStatus`/
    `statusTimers`)
  Plus a fourth, simpler one in `sprint-settings.js` lines 94–141 for
  meta + week masks.
- **What**: each pipeline is the same shape — accumulator Map +
  per-key debounce timer + flush function that PATCHes a JSON envelope
  and applies the server response. The only differences are URL,
  payload shape, and post-save UI hooks (which row total to refresh).
- **Why it's worth fixing**: every behaviour change ("show
  saving-spinner"/"retry on network error"/"coalesce two pending writes
  to the same cell") has to be applied 3×. Already happened during
  Phase 18 — the status pipeline is structurally identical to the days
  pipeline but written in parallel.
- **Suggested rewrite**: a small factory in `sprint-planner.js`'s
  helper section:
  ```js
  function makeBatchSaver(url, debounceMs, onSuccess) {
      const pending = new Map();
      let timer = null;
      function flush() {
          if (pending.size === 0) return;
          const items = Array.from(pending.values());
          pending.clear();
          request('PATCH', url, items)
              .then(onSuccess)
              .catch((e) => flash(e.message, true));
      }
      return {
          queue(key, item) {
              pending.set(key, item);
              clearTimeout(timer);
              timer = setTimeout(flush, debounceMs);
          },
      };
  }
  ```
  Three callers, each ~5 lines. ~80 LoC saved across both JS files. The
  cell-popover-status pipeline keeps its own onSuccess closure; the
  rest is shared.

### R02-N05 — `SprintController::updateMeta()` mixes 4 concerns in 144 lines
- **Severity**: MEDIUM.
- **Status**: open.
- **Where**: `src/Controllers/SprintController.php::updateMeta` lines
  346–489.
- **What**: a single method does (a) JSON body parsing, (b) date
  validation, (c) reserve fraction validation, (d) detection of which
  fields actually changed, (e) the auto-resync of `sprint_weeks` when
  start_date/end_date moves (Phase 21), (f) the audit emission, (g) the
  envelope construction. Reading top-to-bottom is hard.
- **Why it's worth fixing**: the date-resync flow is the most
  consequential change in Phase 21, and it's tucked inside this
  method. The next time someone touches it (e.g. to add a "shrink also
  trims tasks" rule) they'll need to mentally re-trace the whole
  surrounding flow.
- **Suggested rewrite**: extract three private methods, each pure
  (returns a value or throws):
  - `parseAndValidateMetaPatch(Request): array` — returns the
    field-set or a `Response` on failure, like `gateJsonAdmin`.
  - `applyDateRangeChange(Sprint, string $start, string $end): array`
    — does the realign + audit, returns the diff descriptor for the
    response envelope.
  - `applyReserveChange(Sprint, float $reserve): bool` — same shape.
  The method body becomes ~30 lines of orchestration. Tests can call
  the helpers directly without spinning up a full transaction.

### R02-N06 — localStorage key registry duplicated between init/setItem/Reset/`:beamer`
- **Severity**: MEDIUM.
- **Status**: open.
- **Where**: `public/assets/js/sprint-planner.js`
  - Five keys: `ownerFilterKey` (~line 1515), `focusKey` (~1565),
    `statusFilterKey` (~1596), `columnsKey` (~1740), `tabKey` (~1989).
  - Each is built inline as `'sp:' + sprintId + ':<name>' +
    keySuffix` (the `:beamer` namespacing for the present view).
  - Reset button handler clears them by hand (~lines 1851–1864).
- **What**: adding a new persisted filter is a five-step ritual: pick a
  name, build the key, init from localStorage, persist on change, add
  to Reset. Step 5 is the easy one to forget — the symptom is "Reset
  doesn't clear my new filter." Two-place edit (Reset + the persistence
  site) per state slot.
- **Why it's worth fixing**: a small registry removes the second
  edit. Pure cleanup; no behaviour change.
- **Suggested rewrite**: at the top of the IIFE,
  ```js
  const stateKeys = ['ownerFilter', 'focusWorker', 'statusFilter',
                     'hiddenCols', 'tab'];
  const key = (n) => `sp:${sprintId}:${n}${keySuffix}`;
  function resetState() { stateKeys.forEach(n => localStorage.removeItem(key(n))); }
  ```
  Reset becomes one line. New filters add one entry to `stateKeys`.

### R02-N07 — Form-field markup repeated ~20× across views
- **Severity**: MEDIUM (medium because every view-touch nibbles at the
  problem; small individually, large in aggregate).
- **Status**: open.
- **Where**: across `views/sprints/new.twig`, `views/sprints/settings.twig`,
  `views/workers/index.twig`, `views/users/index.twig`,
  `views/settings/index.twig`, `views/auth/local.twig`, the Phase 20
  `views/sprints/import_*.twig`. Roughly 20 instances of the
  `<label>…<input class="mt-1 block w-full rounded-md border-slate-300
  border shadow-sm px-3 py-2 …">` pattern + optional hint span.
- **What**: every form field hand-rolls the same 4-line label/input/
  hint block. Tailwind class strings are duplicated verbatim (or — worse
  — _almost_ verbatim, with a stray class missing in 1–2 spots).
- **Why it's worth fixing**: any visual tweak (focus ring colour, dark
  mode text, error state styling) requires touching 20+ sites. Already
  happens for dark-mode adjustments — the Phase 16 commit notes
  "every view file gets a systematic `dark:` sweep."
- **Suggested rewrite**: a `views/_macros/form.twig` (or inline at top
  of `layout.twig`) with:
  ```twig
  {% macro field(name, label, type='text', value='', hint=null, attrs={}) %}
    <label class="block">
      <span class="text-sm font-medium ...">{{ label }}</span>
      <input type="{{ type }}" name="{{ name }}" value="{{ value }}"
             class="mt-1 block w-full rounded-md border-slate-300 ..."
             {% for k, v in attrs %} {{ k }}="{{ v }}"{% endfor %}>
      {% if hint %}<span class="text-xs ...">{{ hint }}</span>{% endif %}
    </label>
  {% endmacro %}
  ```
  Saves ~80 lines. Same auto-escape applies. Audit each replacement
  with the existing TwigViewTest smoke renders.

### R02-N08 — Three filter dropdowns hand-coded with identical scaffolding
- **Severity**: MEDIUM.
- **Status**: open.
- **Where**: `views/sprints/_task_list.twig`
  - Owners dropdown: lines 31–58
  - Status dropdown: lines 61–90
  - Columns dropdown: lines 103–132
  Each is `<div data-X-root>` + button + panel + clear button + option
  list, with identical Tailwind class strings.
- **What**: the structural shell (positioning, classes, open/close
  trigger pair) is the same in all three; only the option list differs.
  A new dropdown filter (e.g. "Status of last update") adds ~30 lines
  of mechanical scaffolding before any real content.
- **Why it's worth fixing**: a `{% macro filter_dropdown(id, label,
  triggerText) %}` that yields a slot for the option list would cut
  ~60 lines. The dropdown-close-on-outside-click logic in
  `sprint-planner.js` (lines that listen for `data-*-root`) already
  treats them uniformly — the markup should follow.
- **Suggested rewrite**: macro accepting `id` (used as `data-{id}-root`,
  `data-{id}-trigger`, `data-{id}-dropdown`), `triggerLabel`,
  `headerLabel`, and renders the three rendered to `{% block options %}`.

### R02-N09 — Tailwind class soup repeated for buttons/inputs/alerts
- **Severity**: MEDIUM.
- **Status**: open.
- **Where**: scattered across `views/**/*.twig`. Notable cases:
  - Secondary button class string (~35 chars + dark variants) appears
    ≥4× across `show.twig`, `present.twig`, settings, audit.
  - Number-input class (`w-14 rounded border …`) appears 3× across
    `show.twig` and `_task_list.twig`.
  - Error/warning alert classes appear ≥4× across `home.twig`,
    `show.twig`, `settings.twig`, `users/index.twig`.
- **What**: the same ~10 inline class strings repeat verbatim. The
  `.cell-popover` / `.task-menu` / `.task-modal` blocks already exist
  in `assets/css/input.css` under `@layer components` (per SPEC.md
  Phase 17/18/22 notes), proving the team is comfortable with `@apply`-
  based component classes when the markup gets too thick — these other
  recurrences just haven't been promoted yet.
- **Why it's worth fixing**: visual drift between "supposedly the same"
  buttons. Easier theming. Less to read.
- **Suggested rewrite**: add `@layer components` blocks for
  `.btn-secondary`, `.input-number`, `.alert-error`, `.alert-warning`
  in `assets/css/input.css`. Replace inline duplicates. ~30 individual
  copies eliminated; central styling for review.

### R02-N10 — `public/index.php` does ~60 lines of inline service wiring
- **Severity**: MEDIUM.
- **Status**: open.
- **Where**: `public/index.php` lines 158–196.
- **What**: 11 repository constructors + 9 controllers, each with 4–12
  args, instantiated by hand in a flat block. Reading the file
  top-to-bottom you have to step over the wiring to reach the dispatch
  logic.
- **Why it's worth fixing**: every new repository or controller touches
  this block. SPEC.md `§3` already lists every dependency, so a small
  factory class is a one-time cost. Today the bootstrap mixes safety-
  critical work (early `FatalErrorHandler::register`, schema-pending
  503, HTTPS redirect) with mechanical wiring. Separating them makes
  the safety-critical sequence easier to audit (which is itself a
  REVIEW_01 concern: R01-N13 had to register the handler twice to cover
  the wiring window).
- **Suggested rewrite**: `App\Container::build(PDO $pdo, View $view):
  array` returning `['controllers' => [...], 'services' => [...]]`.
  `index.php` becomes ~25 lines: env + autoload + early handler +
  schema check + `$container = Container::build(...)` + router setup.
  No DI library required — same plain `new` calls, just relocated.
  Tests don't change (they construct individual controllers directly).

---

## LOW

### R02-N11 — `recordForRequest()` always passes `$req, $actor` as the trailing pair
- **Severity**: LOW (named args make it readable; bulk reduction is the
  win).
- **Status**: open.
- **Where**: 50+ call sites across `SprintController.php`,
  `TaskController.php`, `WorkerController.php`, `UserController.php`,
  `ImportController.php`, `SettingsController.php`.
- **What**: every audit emission inside a controller method is 8 lines
  with two positional invariants (`req: $req, actor: $actor`).
- **Why it's worth fixing**: the controller already has both — they're
  the per-request constants of the call. A `$this->record('CREATE',
  'sprint', $id, $before, $after, $req, $actor)` style helper, or
  better, a per-request audit closure built once at the top of each
  method (`$audit = $this->auditFor($req, $actor); $audit('CREATE',
  'sprint', $id, null, $snap);`) — would shrink the bulk by ~30%.
- **Suggested rewrite**: thin protected method on a base controller, or
  a request-scoped closure. Spec §7's audit contract is unaffected.

### R02-N12 — Field whitelists declared inline in `TaskController::update`
- **Severity**: LOW.
- **Status**: open.
- **Where**: `src/Controllers/TaskController.php` lines 133–181.
- **What**: an if/elseif cascade walks `title`, `priority`,
  `owner_worker_id`, `description`, `url` with bespoke validation per
  field. URL validation (regex + length) inline at lines 170–181.
- **Why it's worth fixing**: each field's rule is buried in the cascade.
  Adding a sixth field (already happened twice in Phase 22) keeps
  growing this method.
- **Suggested rewrite**: a static array `private const UPDATABLE = ['title' =>
  ['string', 1, 200], 'priority' => ['int_in', [1,2]],
  'owner_worker_id' => ['nullable_int'], 'description' => ['string',
  0, 8000], 'url' => ['url_or_empty', 2048]];` with a small
  `validateField($name, $value)` switch on the type tag. Same per-
  field semantics, table-driven, easier to extend.

### R02-N13 — Date validation duplicated in `create()` and `updateMeta()`
- **Severity**: LOW.
- **Status**: open.
- **Where**: `src/Controllers/SprintController.php` lines 110–124
  (`create()`) and ~lines 373–401 (`updateMeta()`).
- **What**: both methods strict-parse `Y-m-d` with
  `DateTimeImmutable::createFromFormat`, check `$endD < $startD`, and
  call `weeksBetween(...)` to enforce the 26-week ceiling.
  R01-N12's `validateDateFilters()` in `AuditController` is yet another
  variant of the same idea (round-trip strict parse).
- **Why it's worth fixing**: 3× similar parse-and-validate. A shared
  `Domain\DateRange::parse(string, string): self|array` would let the
  controllers do `$range = DateRange::parse($start, $end); if
  (is_array($range)) return $errorRedirect($range['error']);`.
- **Suggested rewrite**: small value object in `src/Domain/`. Static
  helpers `weeksBetween`, `isIsoDate`, `validateDateFilters` collapse
  into one type. Tests for it are already half-written across the
  three call sites.

---

## Things that look complex but are NOT findings

Listed so a future reviewer doesn't re-flag them.

- **The cell popover (`sprint-planner.js` ~lines 787–885).** SPEC.md
  Phase 17 documents seven iterations of a number-stepper popover that
  was eventually deleted entirely. The _current_ popover (commit
  `10ea4b8`) is a different one — slider + status pills, ~100 lines,
  uses element-local pointer events with a 250 ms grace timer. That
  scar tissue is worth preserving: the previous attempts at
  document-level pointer tracking failed in production. Don't refactor
  this without reading the Phase 17 saga first.
- **`buildTaskRow` itself was scaffolded once in vanilla JS** when the
  team migrated off jQuery in Phase 19. The right fix is the
  template-clone approach in R02-N02; rewriting the inner helpers is
  not worth it.
- **Twig macro `capacity_table` is called twice on the same page**
  (Phase "tabs"). That is _already_ the simplification — a macro for
  what would otherwise be 50 lines of duplicated table.
- **The audit-row-per-cascade rule** (SPEC.md §7) — every cascade
  controller (`TaskController::delete`, `SprintController::removeWorker`,
  `SprintController::delete`, etc.) snapshots descendants before the
  FK fires. This _is_ verbose, but the verbosity is the audit
  contract. Don't try to "abstract" it; the explicit shape is the
  point.
- **The R01-N04 `SESSION_SECRET` removal** demonstrates the team's
  preference for "delete the dead config" over "wire it up." Apply the
  same posture to any future "the var is documented but unused" find:
  delete first.

---

## Suggested order to attack

If a single refactor sprint is allotted, the highest leverage-per-hour
sequence:

1. **R02-N02** (template-clone for `buildTaskRow`) — kills a class of
   recurring bugs and is mechanically simple. Worth doing first.
2. **R02-N03** (form-endpoint guard helper) — five lines added to
   `SessionGuard`, ~25 lines removed across controllers, security-
   relevant.
3. **R02-N04** (debounce factory) — pure JS internal cleanup, lets
   future "saving…" indicators land in one place.
4. **R02-N01** (capacity formula) — biggest semantic win, but worth
   landing _after_ the JS-side debounce factory because Path A's "let
   the server be authoritative" works cleanly with the factory.
5. **R02-N07 / R02-N08 / R02-N09** (Twig macros + component classes) —
   pure markup cleanup; can be done piecemeal as views are touched for
   other reasons. Don't churn 10 view files in one PR for purely
   cosmetic reasons.

R02-N05/N06/N10 and the LOW findings are good "while I'm in here"
opportunistic cleanups, not standalone work items.

---

## Maintenance contract

Same posture as REVIEW_01:

1. Pick a finding with severity ≥ MEDIUM that has Status `open`.
2. Apply the rewrite. Adjust tests. Make sure SPEC.md still describes
   reality (most findings here are pure internals — no spec touch
   needed).
3. Commit code first; commit the doc update separately, marking Status
   `fixed-in-<sha>` with a one-line note.
4. Don't delete entries. The history is part of the document's value.