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

Pick the lowest-numbered open finding whose severity ≥ MEDIUM.
Re-read SPEC.md §5 (capacity math), §7 (audit), and §9 (phase log) to confirm the invariant the simplification must preserve.
Apply the rewrite, add/adjust tests, update SPEC.md if a public contract changes. Most findings here are pure internals — no spec touch required.
Commit; flip Status to fixed-in-<sha> and add a one-line note.
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:
```
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:

$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:

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,

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:

{% 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:

R02-N02 (template-clone for buildTaskRow) — kills a class of recurring bugs and is mechanically simple. Worth doing first.
R02-N03 (form-endpoint guard helper) — five lines added to SessionGuard, ~25 lines removed across controllers, security- relevant.
R02-N04 (debounce factory) — pure JS internal cleanup, lets future "saving…" indicators land in one place.
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.
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:

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

REVIEW_02.md 24 KB Verlauf Originalformat