fix: clean up 5 QA nits from auth migration PR #55 #61

Merged

forgejo_admin merged 1 commit from 56-cleanup-qa-nits into main

2026-03-27 04:09:22 +00:00

forgejo_admin commented

2026-03-27 03:58:56 +00:00

Owner

Summary

Addresses 5 non-blocking QA nits discovered during review of PR #55 (auth migration). All cleanup -- no functional or architectural changes.

Changes

src/lib/keycloak.ts: corrected misleading JSDoc comment that referenced check-sso when no onLoad mode is actually set
src/lib/api-client.ts: removed dead NoteLink interface (defined but never imported anywhere)
src/lib/api-client.ts: replaced duplicate COLUMNS array with import from $lib/columns (canonical source, already used by 3 pages)
.env.example: replaced stale server-side env vars (PAL_E_DOCS_API_URL, PAL_E_DOCS_API_KEY) with current client-side VITE_* vars
src/routes/+layout.svelte: consolidated identical .nav-login-link and .nav-logout-btn CSS into a shared selector

Test Plan

npm run check -- 0 errors, 1 pre-existing warning (a11y autofocus on search page)
npm run build -- clean build, adapter-static wrote site to build/
No behavioral changes -- login, logout, board columns, and nav styling all unchanged

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #56
forgejo_admin/pal-e-app#52 -- parent auth migration issue
forgejo_admin/pal-e-app#55 -- PR where nits were found

## Summary Addresses 5 non-blocking QA nits discovered during review of PR #55 (auth migration). All cleanup -- no functional or architectural changes. ## Changes - `src/lib/keycloak.ts`: corrected misleading JSDoc comment that referenced check-sso when no onLoad mode is actually set - `src/lib/api-client.ts`: removed dead NoteLink interface (defined but never imported anywhere) - `src/lib/api-client.ts`: replaced duplicate COLUMNS array with import from `$lib/columns` (canonical source, already used by 3 pages) - `.env.example`: replaced stale server-side env vars (PAL_E_DOCS_API_URL, PAL_E_DOCS_API_KEY) with current client-side VITE_* vars - `src/routes/+layout.svelte`: consolidated identical .nav-login-link and .nav-logout-btn CSS into a shared selector ## Test Plan - [x] `npm run check` -- 0 errors, 1 pre-existing warning (a11y autofocus on search page) - [x] `npm run build` -- clean build, adapter-static wrote site to build/ - [x] No behavioral changes -- login, logout, board columns, and nav styling all unchanged ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related Notes - Closes #56 - `forgejo_admin/pal-e-app#52` -- parent auth migration issue - `forgejo_admin/pal-e-app#55` -- PR where nits were found

forgejo_admin added 1 commit

2026-03-27 03:58:56 +00:00

fix: clean up 5 QA nits from auth migration PR #55

ci/woodpecker/pr/woodpecker Pipeline failed

Details

92b2f29f62

- keycloak.ts: correct misleading check-sso comment (no onLoad mode is set)
- api-client.ts: remove dead NoteLink interface (defined but never imported)
- api-client.ts: import COLUMNS from $lib/columns instead of duplicating
- .env.example: replace stale server-side vars with VITE_* client-side vars
- +layout.svelte: consolidate identical .nav-login-link/.nav-logout-btn CSS

Closes #56

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-27 03:59:34 +00:00

Author

Owner

QA Review -- PR #61

Scope: 4 files, +12/-39 lines. Pure cleanup, no behavioral changes.

File-by-file review

.env.example -- Stale server-side vars (PAL_E_DOCS_API_URL, PAL_E_DOCS_API_KEY) correctly replaced with VITE_* client-side vars. Comments updated. Keycloak defaults match keycloak.ts fallbacks.

src/lib/api-client.ts -- Two clean removals:

Duplicate COLUMNS array replaced with import { COLUMNS } from './columns'. Relative path is correct within $lib. The canonical $lib/columns.ts already exports COLUMNS and is used by 3 pages.
Dead NoteLink interface removed. Confirmed: defined but never imported anywhere in the codebase.

src/lib/keycloak.ts -- JSDoc comment corrected. Old text claimed "Uses check-sso" but keycloak.init() has no onLoad option set. New comment accurately describes the no-onLoad behavior.

src/routes/+layout.svelte -- .nav-login-link and .nav-logout-btn CSS consolidated into shared selectors (base + :hover). Duplicate .nav-login-link block fully removed. No style change -- both classes had identical declarations.

Checks

npm run check: 0 errors (1 pre-existing a11y warning)
npm run build: clean static build

Nits

None.

Discovered scope

None.

VERDICT: APPROVED

## QA Review -- PR #61 **Scope**: 4 files, +12/-39 lines. Pure cleanup, no behavioral changes. ### File-by-file review **`.env.example`** -- Stale server-side vars (`PAL_E_DOCS_API_URL`, `PAL_E_DOCS_API_KEY`) correctly replaced with `VITE_*` client-side vars. Comments updated. Keycloak defaults match `keycloak.ts` fallbacks. **`src/lib/api-client.ts`** -- Two clean removals: - Duplicate `COLUMNS` array replaced with `import { COLUMNS } from './columns'`. Relative path is correct within `$lib`. The canonical `$lib/columns.ts` already exports `COLUMNS` and is used by 3 pages. - Dead `NoteLink` interface removed. Confirmed: defined but never imported anywhere in the codebase. **`src/lib/keycloak.ts`** -- JSDoc comment corrected. Old text claimed "Uses check-sso" but `keycloak.init()` has no `onLoad` option set. New comment accurately describes the no-onLoad behavior. **`src/routes/+layout.svelte`** -- `.nav-login-link` and `.nav-logout-btn` CSS consolidated into shared selectors (base + `:hover`). Duplicate `.nav-login-link` block fully removed. No style change -- both classes had identical declarations. ### Checks - `npm run check`: 0 errors (1 pre-existing a11y warning) - `npm run build`: clean static build ### Nits None. ### Discovered scope None. **VERDICT: APPROVED**