From e7e525df4980c7ab99f13c1f56a6fb91bd4dd5ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hartmut=20N=C3=B6renberg?= Date: Thu, 2 Apr 2026 21:16:10 +0200 Subject: [PATCH] fix(timeline): clear multi-select on drag start and lock in SSE edge-case coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - useTimelineDrag: onProjectBarMouseDown and single-alloc drag path now reset multiSelectRef + multiSelectState before starting a new drag, so the FloatingActionBar is dismissed immediately when an unrelated drag begins - FloatingActionBar.test.tsx: 4 regression tests for the null-render guard (count=0) and all three label variants - useTimelineSSE.test.ts: 2 new tests — tab hides during pending reconnect timer (clears timer, resyncs on next open) and first-ever connection fails before any open (retry open still resyncs correctly) - assistant-tools-user-admin-inventory-read.test.ts: add isActive to expected findMany select shape (already in production, test was stale) Co-Authored-By: claude-flow --- .../timeline/FloatingActionBar.test.tsx | 64 +++++++ .../components/timeline/FloatingActionBar.tsx | 1 + apps/web/src/hooks/useTimelineDrag.ts | 4 + apps/web/src/hooks/useTimelineSSE.test.ts | 54 ++++++ ...nt-tools-user-admin-inventory-read.test.ts | 1 + plan.md | 170 +++++++++++------- 6 files changed, 231 insertions(+), 63 deletions(-) create mode 100644 apps/web/src/components/timeline/FloatingActionBar.test.tsx diff --git a/apps/web/src/components/timeline/FloatingActionBar.test.tsx b/apps/web/src/components/timeline/FloatingActionBar.test.tsx new file mode 100644 index 0000000..d623779 --- /dev/null +++ b/apps/web/src/components/timeline/FloatingActionBar.test.tsx @@ -0,0 +1,64 @@ +import React from "react"; +import { renderToStaticMarkup } from "react-dom/server"; +import { describe, expect, it, vi } from "vitest"; +import { FloatingActionBar } from "./FloatingActionBar.js"; + +const noop = vi.fn(); + +describe("FloatingActionBar", () => { + it("renders nothing when no items are selected", () => { + const html = renderToStaticMarkup( + , + ); + expect(html).toBe(""); + }); + + it("shows allocation count label when allocations are selected", () => { + const html = renderToStaticMarkup( + , + ); + expect(html).toMatch(/2 allocations selected/i); + }); + + it("shows resource count label when resources are selected", () => { + const html = renderToStaticMarkup( + , + ); + expect(html).toMatch(/1 resource selected/i); + }); + + it("shows combined label when both allocations and resources are selected", () => { + const html = renderToStaticMarkup( + , + ); + expect(html).toMatch(/1 allocation.*1 resource/i); + }); +}); diff --git a/apps/web/src/components/timeline/FloatingActionBar.tsx b/apps/web/src/components/timeline/FloatingActionBar.tsx index eed0a7f..bd04540 100644 --- a/apps/web/src/components/timeline/FloatingActionBar.tsx +++ b/apps/web/src/components/timeline/FloatingActionBar.tsx @@ -1,5 +1,6 @@ "use client"; +import React from "react"; import { clsx } from "clsx"; interface FloatingActionBarProps { diff --git a/apps/web/src/hooks/useTimelineDrag.ts b/apps/web/src/hooks/useTimelineDrag.ts index d7524f9..661e1c5 100644 --- a/apps/web/src/hooks/useTimelineDrag.ts +++ b/apps/web/src/hooks/useTimelineDrag.ts @@ -496,6 +496,8 @@ export function useTimelineDrag({ }, ) => { if (e.button !== 0) return; + multiSelectRef.current = INITIAL_MULTI_SELECT; + setMultiSelectState(INITIAL_MULTI_SELECT); e.preventDefault(); e.stopPropagation(); const state = createProjectDragState({ @@ -610,6 +612,8 @@ export function useTimelineDrag({ // ── Single allocation drag ──────────────────────────────────────────── + multiSelectRef.current = INITIAL_MULTI_SELECT; + setMultiSelectState(INITIAL_MULTI_SELECT); const initial = createAllocationDragState({ mode: opts.mode, scope: opts.scope, diff --git a/apps/web/src/hooks/useTimelineSSE.test.ts b/apps/web/src/hooks/useTimelineSSE.test.ts index d875c5e..0b2b7d6 100644 --- a/apps/web/src/hooks/useTimelineSSE.test.ts +++ b/apps/web/src/hooks/useTimelineSSE.test.ts @@ -240,4 +240,58 @@ describe("useTimelineSSE", () => { getTimelineSseResyncKeys().map((queryKey) => [{ queryKey }]), ); }); + + it("hides during pending reconnect timer — clears timer and resyncs on next open", () => { + useTimelineSSE(); + + const firstConnection = MockEventSource.instances[0]; + expect(firstConnection).toBeDefined(); + + // Establish connection, then break it — schedules a 2s reconnect timer + firstConnection?.emitOpen(); + firstConnection?.emitError(); // shouldResyncOnOpen = true, timer scheduled + + // Tab hides while timer is still pending (no active `es`) + mockDocument.setVisibility("hidden"); // clears timer, shouldResyncOnOpen stays true + + // No timer should fire — still only one EventSource + vi.advanceTimersByTime(5_000); + expect(MockEventSource.instances).toHaveLength(1); + + // Tab becomes visible — should connect fresh + mockDocument.setVisibility("visible"); + expect(MockEventSource.instances).toHaveLength(2); + + // Successful open on the new connection → resync + const secondConnection = MockEventSource.instances[1]; + secondConnection?.emitOpen(); + + expect(invalidateQueries).toHaveBeenCalledTimes(getTimelineSseResyncKeys().length); + expect(invalidateQueries.mock.calls).toEqual( + getTimelineSseResyncKeys().map((queryKey) => [{ queryKey }]), + ); + }); + + it("resyncs after the first-ever connection attempt fails before any open", () => { + useTimelineSSE(); + + const firstConnection = MockEventSource.instances[0]; + expect(firstConnection).toBeDefined(); + + // Error fires without any preceding open — shouldResyncOnOpen becomes true + firstConnection?.emitError(); + + // Advance past the 2s backoff + vi.advanceTimersByTime(2_000); + expect(MockEventSource.instances).toHaveLength(2); + + // Retry succeeds + const secondConnection = MockEventSource.instances[1]; + secondConnection?.emitOpen(); + + expect(invalidateQueries).toHaveBeenCalledTimes(getTimelineSseResyncKeys().length); + expect(invalidateQueries.mock.calls).toEqual( + getTimelineSseResyncKeys().map((queryKey) => [{ queryKey }]), + ); + }); }); diff --git a/packages/api/src/__tests__/assistant-tools-user-admin-inventory-read.test.ts b/packages/api/src/__tests__/assistant-tools-user-admin-inventory-read.test.ts index a094e47..972fae5 100644 --- a/packages/api/src/__tests__/assistant-tools-user-admin-inventory-read.test.ts +++ b/packages/api/src/__tests__/assistant-tools-user-admin-inventory-read.test.ts @@ -73,6 +73,7 @@ describe("assistant user admin inventory read tools", () => { lastActiveAt: true, permissionOverrides: true, totpEnabled: true, + isActive: true, }, orderBy: { name: "asc" }, }); diff --git a/plan.md b/plan.md index 74f824f..d1e2e78 100644 --- a/plan.md +++ b/plan.md @@ -1,94 +1,138 @@ -# Plan: App Base URL — configurable via env, no localhost fallback in production +# Plan: Timeline Interaction Stability — P1 continuation Stand: 2026-04-02 --- +# Workstream C: Drag + Selection Interaction Conflicts + ## Anforderungsanalyse -Email-Links (Invite, Password Reset) werden aktuell mit `process.env["NEXTAUTH_URL"] ?? "http://localhost:3100"` gebaut. Das ist korrekt, wenn `NEXTAUTH_URL` gesetzt ist. Das Problem: wenn der Wert fehlt oder leer ist, enthalten Produktions-E-Mails localhost-Links. Außerdem gibt es keine `.env.example`, die dokumentiert, welche Variablen gesetzt werden müssen. +Code-Review von `useTimelineDrag.ts` ergibt zwei tatsächliche Konflikte: -**Was gebaut wird:** -1. Zentrale `getAppBaseUrl()` Funktion in `packages/api` — liest `NEXTAUTH_URL`, wirft in production einen Fehler wenn nicht gesetzt, fällt in dev auf localhost zurück. -2. Beide Router (`invite.ts`, `auth.ts`) verwenden diese Funktion statt duplizierter Inline-Logik. -3. `.env.example` mit allen benötigten Variablen. -4. Health-Route zeigt ob `NEXTAUTH_URL` konfiguriert ist. +1. **`onProjectBarMouseDown`** (Zeile 488): Startet einen Projekt-Bar-Drag **ohne** das committed `multiSelectState` zurückzusetzen. → `FloatingActionBar` bleibt während des Drags sichtbar (da `selectedAllocationIds.length > 0` aus dem committed Multi-Select). -**Betroffene Pakete:** `packages/api`, `apps/web` +2. **`onAllocMouseDown` single-drag path** (Zeile 611, nach dem `if (isMultiSelected)` Block): Wenn eine Allocation per Mousedown angedraggt wird die NICHT in der Multi-Selektion ist, wird ebenfalls kein Reset ausgeführt. → `FloatingActionBar` bleibt sichtbar. -**Audit-Ergebnis — intentionale localhost-Referenzen (NICHT ändern):** -- `apps/web/e2e/dev-system/` — alle E2E-Helfer, Specs, global-setup.ts -- `apps/web/playwright.dev.config.ts` -- `apps/web/src/middleware.test.ts` -- `.github/workflows/deploy-test.yml` +3. **`onCanvasRightMouseDown`**: Startet neue Multi-Select-Session mit `selectedAllocationIds: []` — resettet Selektion korrekt. Kein Fix nötig. ---- +4. **ESC-Taste**: Bereits in `TimelineView.tsx` über `clearMultiSelect` abgehandelt. Kein Fix nötig. + +**Fix-Semantik**: Beide Handler brauchen ein **volles Reset** (`INITIAL_MULTI_SELECT`) — im Gegensatz zu `cancelTransientMultiSelectState`, das committed Selektionen bewahrt. Ein neuer, unabhängiger Drag soll die Selektion vollständig verwerfen. ## Betroffene Pakete & Dateien | Paket | Datei | Art | |-------|-------|-----| -| `packages/api` | `src/lib/app-base-url.ts` | create | -| `packages/api` | `src/router/invite.ts` | edit | -| `packages/api` | `src/router/auth.ts` | edit | -| `apps/web` | `src/app/api/health/route.ts` | edit | -| root | `.env.example` | create | - ---- +| `apps/web` | `src/hooks/useTimelineDrag.ts` | edit | +| `apps/web` | `src/components/timeline/FloatingActionBar.test.tsx` | create | ## Task-Liste -- [ ] **Task 1:** `getAppBaseUrl()` in `packages/api/src/lib/app-base-url.ts` erstellen. - - Liest `process.env["NEXTAUTH_URL"]` (trimmed). - - Wenn gesetzt und nicht leer → gibt den Wert zurück (trailing slash entfernen). - - Wenn leer/fehlend **und** `NODE_ENV === "production"` → wirft `Error("NEXTAUTH_URL must be set in production — email links will be broken")`. - - Sonst (development/test) → gibt `"http://localhost:3100"` zurück und loggt einmalig eine Warnung. - - → Datei: `packages/api/src/lib/app-base-url.ts` +- [ ] **C-1:** `useTimelineDrag.ts` — Multi-Select bei Drag-Start zurücksetzen. + - In `onProjectBarMouseDown` (nach `if (e.button !== 0) return;`, vor `createProjectDragState`): + ```ts + multiSelectRef.current = INITIAL_MULTI_SELECT; + setMultiSelectState(INITIAL_MULTI_SELECT); + ``` + - In `onAllocMouseDown` im Single-Drag-Zweig (nach dem `if (isMultiSelected) { ... return; }` Block, vor `createAllocationDragState`): + ```ts + multiSelectRef.current = INITIAL_MULTI_SELECT; + setMultiSelectState(INITIAL_MULTI_SELECT); + ``` + - `setMultiSelectState` ist bereits von `useState` (stabil) — kein Eintrag in dep-Array nötig (Konvention konsistent mit bestehender `onAllocMouseDown`-Implementierung, die `setMultiSelectState` ebenfalls ohne Dep-Eintrag nutzt). + - Ref-Reset ist nötig damit nachfolgende synchrone Ref-Lesungen im selben Event-Cycle den gecleansten State sehen. + - → Datei: `apps/web/src/hooks/useTimelineDrag.ts` -- [ ] **Task 2:** `invite.ts` auf `getAppBaseUrl()` umstellen. - - Ersetze `const baseUrl = process.env["NEXTAUTH_URL"] ?? "http://localhost:3100";` durch `const baseUrl = getAppBaseUrl();` - - → Datei: `packages/api/src/router/invite.ts` Zeile 53 - -- [ ] **Task 3:** `auth.ts` auf `getAppBaseUrl()` umstellen. - - Ersetze `const baseUrl = process.env["NEXTAUTH_URL"] ?? "http://localhost:3100";` durch `const baseUrl = getAppBaseUrl();` - - → Datei: `packages/api/src/router/auth.ts` Zeile 50 - -- [ ] **Task 4:** `.env.example` anlegen mit allen required und optionalen Variablen. - - Sections: App, Auth, Database, Redis, SMTP, AI, Dev-Tools (pgAdmin) - - Jede Variable: Kommentar (required/optional, Beschreibung), Beispielwert. - - `NEXTAUTH_URL` als REQUIRED mit Hinweis "must be the public URL (e.g. https://capakraken.example.com) — used in email links; do not use localhost in production" - - → Datei: `.env.example` - -- [ ] **Task 5:** Health-Route um `baseUrl`-Check erweitern. - - Liest `NEXTAUTH_URL` direkt (kein `getAppBaseUrl()` — soll nie werfen, nur reporten). - - Fügt `"baseUrl": { "configured": bool, "isLocalhost": bool }` zum JSON-Response hinzu. - - `configured: false` wenn Var fehlt/leer; `isLocalhost: true` wenn Wert mit `http://localhost` beginnt. - - → Datei: `apps/web/src/app/api/health/route.ts` - ---- +- [ ] **C-2:** `FloatingActionBar.test.tsx` erstellen — Regressions-Coverage. + - Test 1: `selectedAllocationCount=0, selectedResourceCount=0` → rendert `null` (Bar ist ausgeblendet) + - Test 2: `selectedAllocationCount=2, selectedResourceCount=0` → Bar sichtbar, Text "2 allocations selected" + - Test 3: `selectedAllocationCount=0, selectedResourceCount=1` → Bar sichtbar, Text "1 resource selected" + - Test 4: `selectedAllocationCount=1, selectedResourceCount=1` → Bar sichtbar, Text enthält "1 allocation" und "1 resource" + - Sichert den `totalCount === 0 → null`-Guard: wenn Multi-Select gecleart wird, verschwindet die Bar. + - → Datei: `apps/web/src/components/timeline/FloatingActionBar.test.tsx` ## Abhängigkeiten -- Task 2 **und** Task 3 benötigen Task 1 (Funktion muss existieren). -- Task 2 und Task 3 können **parallel** ausgeführt werden (unterschiedliche Dateien). -- Task 4 und Task 5 sind **unabhängig** von allen anderen und können parallel ausgeführt werden. - ---- +- C-1 und C-2 sind **unabhängig** voneinander (C-2 ist reiner Render-Test der Komponente) ## Akzeptanzkriterien - [ ] `pnpm test:unit` läuft grün - [ ] `pnpm --filter @capakraken/web exec tsc --noEmit` — keine neuen TS-Errors -- [ ] `pnpm test:e2e:email` — alle 3 E2E-Tests bestehen weiterhin -- [ ] Wenn `NEXTAUTH_URL=https://capakraken.hartmut-noerenberg.com`: E-Mail-Links enthalten diese Domain -- [ ] Wenn `NEXTAUTH_URL` fehlt und `NODE_ENV=production`: `createInvite` / `requestPasswordReset` werfen einen klaren Fehler beim Link-Bau -- [ ] `GET /api/health` liefert `baseUrl.configured: true/false` und `baseUrl.isLocalhost: bool` -- [ ] `.env.example` existiert und dokumentiert alle Pflichtfelder - ---- +- [ ] Projekt-Bar-Drag startet mit aktiver Selektion: `FloatingActionBar` wird sofort ausgeblendet +- [ ] Single-Alloc-Drag auf nicht-selektierter Allocation: `FloatingActionBar` wird ausgeblendet +- [ ] Rechtsklick-Drag auf Canvas: Selektion wird korrekt überschrieben (bereits korrekt) ## Risiken & offene Fragen -- **Unit-Tests für Router:** Bestehende Tests für `invite.ts` und `auth.ts` müssen `NEXTAUTH_URL` in der Testumgebung gesetzt haben (oder `NODE_ENV=test` → Dev-Fallback greift). Vorhandene Tests prüfen, ob `process.env["NEXTAUTH_URL"]` dort gesetzt wird. -- **`docker-compose.prod.yml`** delegiert ENV-Vars an `.env.production` (gitignored). Das `.env.example` deckt ab, was dort stehen muss — kein Code-Change nötig. -- **Trailing Slash:** NEXTAUTH_URL könnte mit oder ohne `/` enden. `getAppBaseUrl()` sollte trailing slashes normalisieren. +- **Ref-Sync**: `multiSelectRef.current` wird bereits in `useTimelineDrag` auf Zeile 260 bei jedem Render mit `multiSelectState` synchronisiert. Der explizite Ref-Reset im Handler ist aber nötig, damit nachfolgende synchrone Zugriffe (z.B. innerhalb desselben Event-Handlers) nicht den alten Ref-Wert lesen. Kein Risiko. +- **Dep-Array**: `INITIAL_MULTI_SELECT` ist eine Modul-Konstante (referenz-stabil), `setMultiSelectState` ist stabil aus `useState`. Beide müssen nicht in die `useCallback` dep-Arrays — das ist konsistent mit der bestehenden Nutzung in `onAllocMouseDown`. + +--- + +# Workstream D: SSE Reconnect — fehlende Edge-Case-Tests + +## Anforderungsanalyse + +Vollständige Analyse von `useTimelineSSE.ts` und 8 bestehenden Tests in `useTimelineSSE.test.ts`. + +**Bereits abgedeckt:** +- Malformed payloads → kein invalidate +- Ping → kein invalidate, backoff-Reset +- Initial open → kein resync +- Error → reconnect → open → resync (genau einmal) +- Double-open nach reconnect → resync nur einmal +- Dispose während pending Timer +- Hidden on mount, dann visible → connect ohne resync +- Hide während aktiver Connection, dann visible → resync + +**Fehlende Edge Cases:** + +1. **Tab versteckt während pending reconnect-Timer** (kein aktives `es`, aber `reconnectTimeout.current` gesetzt): + `handleVisibilityChange` prüft `if (es || reconnectTimeout.current)` → setzt `shouldResyncOnOpen = true`, löscht Timer via `clearReconnectTimer()`. Bei visible wieder: `reconnectAttempts = 0`, `connect()` → neue Connection → open → resync. **Nicht getestet.** + +2. **Allererste Verbindung schlägt fehl** (kein `onopen` je gerufen, direkt `onerror`): + `shouldResyncOnOpen = true` nach dem ersten Fehler ohne vorangehendes open. Nach Retry → open → resync. Dokumentiert: auch wenn die initiale Verbindung nie erfolgreich war, wird beim ersten erfolgreichen open korrekt resynct. **Nicht explizit getestet.** + +3. **Mehrere konsekutive Errors ohne open** (implizit bereits abgedeckt im "resets reconnect backoff"-Test über `emitError(); emitError();`). Kein zusätzlicher Test nötig. + +## Betroffene Pakete & Dateien + +| Paket | Datei | Art | +|-------|-------|-----| +| `apps/web` | `src/hooks/useTimelineSSE.test.ts` | edit | + +## Task-Liste + +- [ ] **D-1:** `useTimelineSSE.test.ts` — zwei fehlende Edge-Case-Tests hinzufügen. + - **Test 9**: `"hides during pending reconnect timer — clears timer and resyncs on next open"` + - Sequence: connect → open → error (shouldResyncOnOpen=true, Timer 2s) → hide (Timer gecleart, shouldResyncOnOpen bleibt true, es=null) → show (reconnectAttempts=0, connect() → neue EventSource) → emitOpen() → resync + - Verifikation: `MockEventSource.instances` hat Länge 2; `invalidateQueries` wurde genau `getTimelineSseResyncKeys().length` mal mit den richtigen Keys aufgerufen + - Unterschied zu bestehendem hide-Test: dort wird `es` aktiv geschlossen; hier ist `es = null` und nur der Timer ist pending + - **Test 10**: `"resyncs after the first-ever connection attempt fails before any open"` + - Sequence: connect → emitError() (kein emitOpen davor, shouldResyncOnOpen=true) → advance timer 2000ms → neue EventSource → emitOpen() → resync + - Verifikation: `MockEventSource.instances` hat Länge 2; `invalidateQueries` aufgerufen mit resync-keys + - Dokumentiert: `shouldResyncOnOpen` wird auch beim allerersten Fehler gesetzt — nicht nur nach erfolgreicher erster Verbindung + - Kein Produktionscode geändert — reine Test-Ergänzung. + - → Datei: `apps/web/src/hooks/useTimelineSSE.test.ts` + +## Abhängigkeiten + +- D-1 ist vollständig unabhängig (nur Tests, kein Source-Code) + +## Akzeptanzkriterien + +- [ ] `pnpm test:unit` läuft grün (alle 10 SSE-Tests grün) +- [ ] Beide neuen Tests decken bisher ungetestetem Verhalten ab (kein Produktionscode nötig) + +--- + +# Parallele Ausführung + +**Workstream C** und **Workstream D** sind vollständig unabhängig: + +- **Agent 1** → C-1 + C-2 (sequenziell: C-1 zuerst, C-2 kann parallel) +- **Agent 2** → D-1 (eigenständig) + +Keine gemeinsamen Dateien, kein Merge-Konflikt-Risiko.