diff --git a/docs/planning/retrospectives/epic-9-retro-2026-03-10.md b/docs/planning/retrospectives/epic-9-retro-2026-03-10.md new file mode 100644 index 0000000..0701678 --- /dev/null +++ b/docs/planning/retrospectives/epic-9-retro-2026-03-10.md @@ -0,0 +1,241 @@ +# Epic 9 Retrospective: Bug Fixes & Core Workflow Stability + +**Date:** 2026-03-10 +**Facilitator:** Bob (Scrum Master) +**Participants:** Simon (Project Lead), Alice (Product Owner), Charlie (Senior Dev), Dana (QA Engineer), Elena (Junior Dev) + +--- + +## Epic Summary + +| Metric | Value | +|--------|-------| +| **Epic Name** | Bug Fixes & Core Workflow Stability | +| **Stories Completed** | 4/4 (100%) | +| **Story Points** | 14 (5 + 5 + 2 + 2) | +| **Estimated Duration** | 7-10 days | +| **Actual Duration** | ~4 days (Mar 6 - Mar 9) | +| **Review Findings** | 0 HIGH, 0 MEDIUM, 5 LOW (all advisory) | +| **Production Incidents** | 0 | +| **Blockers Encountered** | 0 | +| **CI Failures on Merge** | 4 (3 fix commits required) | +| **Tests Added** | 15 new tests (141 → 156) | + +**Stories Delivered:** +1. Story 9.1: Fix Application Status Transitions & Add Draft Status — Root cause: missing frontend UI (not backend bug). Added status dropdown to detail page, Draft migration, status select on create form. +2. Story 9.2: Fix Interview Status Tracking & Dashboard Display — Fixed dashboard count (query interviews table directly), added interview status column, auto-status transition on interview creation, color-coded badges. +3. Story 9.3: Fix Session Expiry Redirect & Re-authentication — Fixed all 3 signOut call sites to use `signOut({ redirect: false })` + manual navigation, added isSigningOut race condition guard, callbackUrl validation. +4. Story 9.4: Application Form Error Display — Wired error display to 6 missing fields, added formError banner for non-validation errors, backend-to-frontend field name mapping. + +**Business Outcomes:** +- Application status transitions work via UI dropdown +- Dashboard shows accurate interview counts from interviews table +- Creating an interview auto-transitions application status to "Interview" +- Interview status badges (scheduled/completed/cancelled/awaiting outcome) display across all views +- Application form shows inline errors on all fields and general error banner +- Session expiry redirects to clean URL (partial — re-authentication still broken) + +--- + +## What Went Well + +### 1. Execution Speed +Delivered 14 story points in ~4 days against a 7-10 day estimate. Third consecutive epic finishing under estimate. Established patterns made execution fast and predictable. + +### 2. Clean Reviews +3 of 4 stories approved on first review. Only Story 9.3 required one round of changes (3 LOW items, all resolved same day). Zero HIGH or MEDIUM findings across the entire epic. + +### 3. Root Cause Investigation Discipline +Story 9.1's thorough investigation revealed the "status transition bug" was actually a missing frontend feature. This insight carried forward to Story 9.2, allowing the team to focus on the real fix rather than chasing phantom backend bugs. + +### 4. Test Commitment Maintained +15 new tests added across all 4 stories (141 → 156). Tests-per-story commitment from Epic 7 retro fully maintained for the third consecutive epic. + +### 5. Deploy Before Retro +Epic 9 deployed to production before retrospective (Epic 8 action item fulfilled). + +--- + +## What Could Improve + +### 1. CI Broke 4 Times on Merge (Priority: HIGH) + +**Issue:** PR 54 required 3 fix commits after 4 CI failures: +- TypeScript index signature error on `statusVariantMap` — `pnpm dev` didn't catch what `pnpm build` (strict mode in Docker) catches +- Backend test DB schema missing `rate_limits` table and `status` column — CI test schema is manually maintained and wasn't updated with new migrations + +**Impact:** Wasted CI minutes, delayed merge, required additional commits to fix. + +**Root Cause:** Two gaps: (1) local dev (`pnpm dev`) is more lenient than production build (`pnpm build`), (2) CI test database uses a static schema that must be manually updated for each migration. + +**Resolution:** +- Dev agent runs `pnpm build` locally before pushing +- Automate CI test DB schema to run migrations instead of static schema + +### 2. Visual Quality Not Caught in Review (Priority: MEDIUM) + +**Issue:** Story 9.1 shipped a status dropdown with poor UI that Simon had to fix during Story 9.2. Code review checked correctness but not visual output. + +**Impact:** Rework required in subsequent story. + +**Root Cause:** Review process checks code patterns and logic but doesn't include rendering the UI. + +**Resolution:** Add visual review step — every UI story's review includes a screenshot or manual render check before approval. + +### 3. Session Expiry Re-authentication Still Broken (Priority: HIGH) + +**Issue:** Production smoke testing during this retrospective revealed that while Story 9.3 fixed the redirect URL format (no CSRF tokens), **re-authentication fails** when credentials are submitted from `/login?error=SessionExpired&callbackUrl=%2Fapplications`. Same credentials work fine on clean `/login`. + +**Impact:** Users who experience session expiry cannot log back in without manually navigating to `/login`. + +**Resolution:** Must fix before starting Epic 10. The `error` or `callbackUrl` URL params may be interfering with NextAuth's credential sign-in handler. + +### 4. Duplicate CI Runs (Priority: LOW) + +**Issue:** CI workflow triggers on both `push` and `pull_request` without branch filtering, causing duplicate runs on every feature branch push that has an open PR. + +**Impact:** Wasted GitHub Actions minutes. + +**Resolution:** Scope `push` trigger to `main` only in `.github/workflows/ci.yml`. + +--- + +## Epic 8 Action Items Follow-Through + +| # | Action | Status | Evidence | +|---|--------|--------|----------| +| 1 | Draft → design → refine workflow | N/A | Bug fix epic, no feature design needed | +| 2 | Deploy before retro | ✅ Done | Epic 9 deployed to production before retro | +| 3 | Five-state UI checklist | N/A | Bug fix epic, no new UI screens | +| 4 | Tests required per story | ✅ Done | 15 new tests across all 4 stories | +| 5 | ARIA lint rules in CI | ❌ Not done | Still pending from Epic 7 | + +--- + +## Technical Debt + +### From Epic 9 Reviews (LOW) + +| # | Item | Source | +|---|------|--------| +| 1 | Duplicated `getInterviewDisplayStatus` in `columns.tsx` and `interview-card-list.tsx` | Story 9.2 | +| 2 | `getApplicationStatuses()` cache has no invalidation | Story 9.1 | +| 3 | Dashboard `statusCounts` map doesn't initialize "draft" | Story 9.1 | +| 4 | Test 3.1 in Story 9.4 lacks DOM rendering assertions for field-level errors | Story 9.4 | + +### Carry-Forward from Previous Epics + +| # | Item | Source | +|---|------|--------| +| 1 | Touch target audit (44px minimum) | Epic 7 | +| 2 | Empty state components | Epic 7 | +| 3 | Form inputMode/autoComplete completeness | Epic 7 | +| 4 | ARIA lint rules in CI | Epic 7 | + +--- + +## Key Insights + +### 1. Production Smoke Testing Catches What Automated Tests Miss +The retrospective's production smoke test found 2 issues (session expiry re-auth failure, Draft status missing) that passed all 156 automated tests and all code reviews. Always test the deployed artifact. + +### 2. "Bug vs Feature" Labeling Doesn't Matter +2 of 4 "bugs" were actually missing features. From the user's perspective, broken functionality is broken regardless of whether it's a code defect or a design gap. Focus on fixing what's broken. + +### 3. Local Dev Environment Diverges from CI/Production +`pnpm dev` is more lenient than `pnpm build`. CI test schema is static while production uses migrations. These gaps cause avoidable CI failures. + +### 4. Visual Review is a Gap +Code review catches logic and pattern issues but not visual quality. UI stories need a render check as part of the review process. + +--- + +## Action Items + +### Process Improvements + +| # | Action | Owner | Deadline | Success Criteria | +|---|--------|-------|----------|-----------------| +| 1 | Run `pnpm build` locally before pushing | Dev agent | Starting Epic 10 | Zero build-only CI failures | +| 2 | Add visual review step to story reviews | Bob + Simon | Before Epic 10 stories ready-for-dev | UI stories include render check | +| 3 | Automate CI test DB schema (run migrations) | Charlie | Before Epic 10 | New migrations don't break CI | +| 4 | Fix duplicate CI runs (scope push to main) | Charlie | Before Epic 10 | Single CI run per feature branch push | + +### Carry-Forward + +| # | Action | Status | +|---|--------|--------| +| 1 | Draft → design → refine workflow | Active — apply to Story 10.2 | +| 2 | Tests required per story | ✅ Continuing | +| 3 | ARIA lint rules in CI | Active — pending since Epic 7 | + +--- + +## Critical Path + +| # | Item | Owner | Priority | +|---|------|-------|----------| +| 1 | **Fix session expiry re-authentication** — credentials login fails on `/login?error=SessionExpired&callbackUrl=...` | Dev agent | **BLOCKER** — must fix before Epic 10 | + +--- + +## Production Smoke Test Results + +Conducted during retrospective using Chrome DevTools MCP against jobditto.com. + +| # | Test | Result | Notes | +|---|------|--------|-------| +| 1 | Create application with status selection | ✅ Pass | Status dropdown works | +| 2 | Change status via detail page dropdown | ✅ Pass | Saved → Applied, toast confirmed | +| 3 | Dashboard interview count accurate | ✅ Pass | Shows correct count | +| 4 | Auto-status transition on interview creation | ✅ Pass | Applied → Interview automatically | +| 5 | Interview "Awaiting Outcome" display | ✅ Pass | Past date + no outcome renders correctly | +| 6 | Session expiry re-authentication | ❌ FAIL | Credentials rejected on session expiry URL | +| 7 | Inline field error display | ✅ Pass | Invalid URL shows error with role="alert" | +| 8 | Error clears on correction | ✅ Pass | Fixing input clears error, enables submit | + +--- + +## Readiness Assessment + +| Area | Status | Notes | +|------|--------|-------| +| Testing & Quality | ✅ Good | 156 tests passing, production smoke tested | +| Deployment | ✅ Deployed | Live on jobditto.com | +| Stakeholder Acceptance | ✅ Accepted | Simon tested and approved | +| Technical Health | ⚠️ Issue | Session expiry re-auth broken in production | +| Unresolved Blockers | ⚠️ 1 Blocker | Session expiry fix required before Epic 10 | + +--- + +## Next Steps + +1. **Fix session expiry re-authentication** — BLOCKER for Epic 10 +2. **Fix CI: automate test DB schema + scope push trigger to main** +3. **Add visual review step to story review process** +4. **Begin Epic 10: Security & Access Hardening** once blocker resolved + +--- + +## Project Journey + +| Epic | Name | Stories | +|------|------|---------| +| 1 | Enhanced Application Management | 6 | +| 2 | Deep Interview Management | 12 | +| 0.1 | Design System Update | 1 | +| 3 | Technical Assessment Tracking | 8 | +| 4 | Workflow Automation & Timeline | 6 | +| 5 | Search, Discovery & Data Management | 5 | +| 6 | Polish, Performance & Production Readiness | 10 | +| 7 | CI/CD Pipeline & Production Deployment | 3 | +| 8 | Multi-Provider Authentication & Account Security | 3 | +| 9 | Bug Fixes & Core Workflow Stability | 4 | +| **Total** | | **58 stories** | + +**Production:** jobditto.com | **Status:** Live + +--- + +_This retrospective was facilitated using the BMad Method retrospective workflow._ +_Format: BMad Retrospective Workflow v1.0_ diff --git a/docs/planning/sprint-status.yaml b/docs/planning/sprint-status.yaml index 2c7317a..438aab9 100644 --- a/docs/planning/sprint-status.yaml +++ b/docs/planning/sprint-status.yaml @@ -135,6 +135,7 @@ development_status: 9-2-fix-interview-status-tracking-and-dashboard: done 9-3-fix-session-expiry-redirect: done 9-4-application-form-error-display: done + epic-9-retrospective: done # Epic 10: Security & Access Hardening epic-10: backlog diff --git a/frontend/src/app/(auth)/login/__tests__/page.test.tsx b/frontend/src/app/(auth)/login/__tests__/page.test.tsx index ade3f6f..34cc9bd 100644 --- a/frontend/src/app/(auth)/login/__tests__/page.test.tsx +++ b/frontend/src/app/(auth)/login/__tests__/page.test.tsx @@ -5,10 +5,11 @@ import LoginPage from "../page"; const mockPush = jest.fn(); const mockSignIn = jest.fn(); +let mockSearchParams = new URLSearchParams(); jest.mock("next/navigation", () => ({ useRouter: () => ({ push: mockPush }), - useSearchParams: () => new URLSearchParams(), + useSearchParams: () => mockSearchParams, })); jest.mock("next-auth/react", () => ({ @@ -33,6 +34,7 @@ jest.mock("../../components/oauth-buttons", () => ({ describe("LoginPage", () => { beforeEach(() => { jest.clearAllMocks(); + mockSearchParams = new URLSearchParams(); }); it("renders email and password fields", () => { @@ -100,6 +102,7 @@ describe("LoginPage", () => { email: "test@example.com", password: "password123", redirect: false, + callbackUrl: "/", }); }); }); @@ -133,4 +136,76 @@ describe("LoginPage", () => { ).toBeInTheDocument(); }); }); + + it("passes valid internal callbackUrl from URL params to signIn", async () => { + mockSearchParams = new URLSearchParams("callbackUrl=/applications"); + mockSignIn.mockResolvedValue({ error: null }); + const user = userEvent.setup(); + render(); + + await user.type(screen.getByLabelText(/email/i), "test@example.com"); + await user.type(screen.getByLabelText(/password/i), "password123"); + await user.click(screen.getByRole("button", { name: /sign in/i })); + + await waitFor(() => { + expect(mockSignIn).toHaveBeenCalledWith("credentials", { + email: "test@example.com", + password: "password123", + redirect: false, + callbackUrl: "/applications", + }); + expect(mockPush).toHaveBeenCalledWith("/applications"); + }); + }); + + it("sanitizes external callbackUrl to / to prevent open redirect", async () => { + mockSearchParams = new URLSearchParams("callbackUrl=//evil.com"); + mockSignIn.mockResolvedValue({ error: null }); + const user = userEvent.setup(); + render(); + + await user.type(screen.getByLabelText(/email/i), "test@example.com"); + await user.type(screen.getByLabelText(/password/i), "password123"); + await user.click(screen.getByRole("button", { name: /sign in/i })); + + await waitFor(() => { + expect(mockSignIn).toHaveBeenCalledWith("credentials", { + email: "test@example.com", + password: "password123", + redirect: false, + callbackUrl: "/", + }); + expect(mockPush).toHaveBeenCalledWith("/"); + }); + }); + + it("sanitizes absolute external callbackUrl to /", async () => { + mockSearchParams = new URLSearchParams("callbackUrl=https://evil.com"); + mockSignIn.mockResolvedValue({ error: null }); + const user = userEvent.setup(); + render(); + + await user.type(screen.getByLabelText(/email/i), "test@example.com"); + await user.type(screen.getByLabelText(/password/i), "password123"); + await user.click(screen.getByRole("button", { name: /sign in/i })); + + await waitFor(() => { + expect(mockSignIn).toHaveBeenCalledWith("credentials", { + email: "test@example.com", + password: "password123", + redirect: false, + callbackUrl: "/", + }); + expect(mockPush).toHaveBeenCalledWith("/"); + }); + }); + + it("shows session expired message when error=SessionExpired in URL", () => { + mockSearchParams = new URLSearchParams("error=SessionExpired&callbackUrl=/applications"); + render(); + + expect( + screen.getByText("Your session has expired. Please sign in again."), + ).toBeInTheDocument(); + }); }); diff --git a/frontend/src/app/(auth)/login/page.tsx b/frontend/src/app/(auth)/login/page.tsx index 3a382dc..95ff040 100644 --- a/frontend/src/app/(auth)/login/page.tsx +++ b/frontend/src/app/(auth)/login/page.tsx @@ -32,7 +32,7 @@ const LoginForm = () => { const urlError = searchParams.get('error'); const rawCallbackUrl = searchParams.get('callbackUrl'); - const callbackUrl = rawCallbackUrl?.startsWith('/') ? rawCallbackUrl : '/'; + const callbackUrl = rawCallbackUrl?.startsWith('/') && !rawCallbackUrl.startsWith('//') ? rawCallbackUrl : '/'; useEffect(() => { if (urlError === 'SessionExpired') { @@ -45,6 +45,7 @@ const LoginForm = () => { email: data.email, password: data.password, redirect: false, + callbackUrl, }); if (result?.error) {