pal-e-docs

Review: Cleanup: QA nits from PR #55 auth migration

review-424-2026-03-26 Review

review ready
Project

Verdict: READY

Re-review after refinements. Both issues from prior NEEDS_REFINEMENT review have been addressed.

Template Completeness

  • [x] Type -- Feature
  • [x] Lineage -- "Discovered scope from QA review of PR #55 (issue #52)"
  • [x] Repo -- forgejo_admin/pal-e-app
  • [x] User Story
  • [x] Context
  • [x] File Targets
  • [x] Acceptance Criteria
  • [x] Test Expectations
  • [x] Constraints
  • [x] Checklist
  • [x] Related
All required sections for the Feature template are present. Template is complete.

Traceability

  • [x] story:spa-convention label -- present on board item
  • [x] arch:app label -- present on board item
  • [x] Forgejo issue -- forgejo_admin/pal-e-app#56, state: open
  • [x] scope:discovered label -- correctly identifies this as discovered scope from PR #55 QA

File Targets

  • [x] src/lib/keycloak.ts line 20 -- VERIFIED: Misleading "Uses check-sso" comment present. init() call (lines 26-29) does NOT pass onLoad: 'check-sso'.
  • [x] src/lib/api-client.ts line 106 (NoteLink) -- VERIFIED: NoteLink interface defined at lines 106-111. Only appears in api-client.ts, never imported by any other file. Dead code confirmed.
  • [x] .env.example -- VERIFIED: Contains stale server-side vars PAL_E_DOCS_API_URL and PAL_E_DOCS_API_KEY. No VITE_* vars present. The old X-PaleDocs-Token/API key pattern is dead after auth migration.
  • [x] src/lib/api-client.ts line 27 + src/lib/columns.ts line 8 (COLUMNS duplicate) -- VERIFIED: Both files define identical COLUMNS arrays. Issue now specifies direction: "Remove and import from $lib/columns instead (canonical source, already exported and used by 3 pages)." FIX CONFIRMED.
  • [x] src/routes/+layout.svelte lines 171-198 (nav CSS) -- VERIFIED: .nav-logout-btn (lines 171-184) and .nav-login-link (lines 186-199) have identical CSS properties. Should consolidate.

Repo Placement

OK. Issue filed on forgejo_admin/pal-e-app, all file targets in pal-e-app repo. Single-repo scope.

Dependencies

  • Parent issue #52 (auth migration) -- board item #413, done. No blocker.
  • PR #55 (where nits were found) -- merged and closed. No blocker.
  • Issue #53 (adapter-static) -- board item #414, done. No blocker.
  • Issue #58 (deployment overlay) -- board item #427, todo. Independent scope.
  • No items in in_progress block this ticket.

Acceptance Criteria

  • [x] "No misleading comments about check-sso" -- Verifiable by grep.
  • [x] "No dead interfaces in api-client.ts" -- Verifiable by grep.
  • [x] ".env.example reflects new client-side env vars (VITE_*)" -- Verifiable by reading file.
  • [x] "COLUMNS imported from $lib/columns (single source of truth)" -- Verifiable by grep. Direction now specified: import from $lib/columns. FIX CONFIRMED.
  • [x] "Login/logout button CSS consolidated" -- Verifiable by grep.
All criteria are agent-verifiable. Test commands (npm run check && npm run build) are real.

Blast Radius

  • mcd-tracker-app: ~/mcd-tracker-app/src/lib/keycloak.js line 22 has identical misleading "Uses check-sso" comment. Explicitly scoped out in Constraints section: "that's a separate discovered-scope issue, not in scope here." FIX CONFIRMED.
  • westside-app is correct: Actually uses onLoad: 'check-sso', so its comment is accurate. No action needed.
  • No other sibling apps have duplicate COLUMNS or dead NoteLink patterns.

Recommendation

No action needed. Both issues from the prior NEEDS_REFINEMENT review have been resolved:
  • COLUMNS consolidation direction missing -- Now specifies: import from $lib/columns (canonical source).
  • mcd-tracker blast radius undocumented -- Now noted in Constraints as out-of-scope.
Ticket is ready to move from todo to next_up.