forgejo_admin/pal-e-docs-app
1
0
Fork 0
Code Issues 4 Pull requests 1 Projects Releases Packages Wiki Activity Actions

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

New issue
Closed
opened 2026-03-27 01:17:49 +00:00 by forgejo_admin · 2 comments
forgejo_admin commented 2026-03-27 01:17:49 +00:00
Owner
Copy link

Type

Feature

Lineage

Discovered scope from QA review of PR #55 (issue #52).

Repo

forgejo_admin/pal-e-app

User Story

As pal-e-app
I want clean code with no stale references or dead interfaces
So that the codebase stays maintainable after the auth migration

Context

QA review of PR #55 (auth migration from Auth.js to keycloak-js) found 5 non-blocking nits. These are cleanup items that don't affect functionality but should be addressed for code quality.

File Targets

  • src/lib/keycloak.ts line 20 — misleading "check-sso" comment (no onLoad: 'check-sso' is actually passed). Remove or correct the comment.
  • src/lib/api-client.ts line 106 — dead NoteLink interface (defined but never imported). Remove it.
  • .env.example — stale references to old server-side env vars (PAL_E_DOCS_API_URL, PAL_E_DOCS_API_KEY, AUTH_*). Replace with new client-side VITE_* vars.
  • src/lib/api-client.ts — duplicate COLUMNS definition. Remove and import from $lib/columns instead (canonical source, already exported and used by 3 pages).
  • src/routes/+layout.svelte lines 171-198 — near-identical CSS for .nav-login-link and .nav-logout-btn. Consolidate into a shared class.

Acceptance Criteria

  • No misleading comments about check-sso in keycloak.ts
  • No dead interfaces in api-client.ts
  • .env.example reflects new client-side env vars (VITE_*)
  • COLUMNS imported from $lib/columns (single source of truth)
  • Login/logout button CSS consolidated

Test Expectations

  • npm run check passes
  • npm run build passes
  • Run command: npm run check && npm run build

Constraints

  • Small cleanup — no architectural changes
  • Must not break any existing functionality
  • Note: mcd-tracker-app has the same misleading check-sso comment — that's a separate discovered-scope issue, not in scope here

Checklist

  • PR opened
  • Tests pass
  • No unrelated changes

Related

  • forgejo_admin/pal-e-app#52 — parent migration issue
  • forgejo_admin/pal-e-app#55 — PR where nits were found
### Type Feature ### Lineage Discovered scope from QA review of PR #55 (issue #52). ### Repo `forgejo_admin/pal-e-app` ### User Story As pal-e-app I want clean code with no stale references or dead interfaces So that the codebase stays maintainable after the auth migration ### Context QA review of PR #55 (auth migration from Auth.js to keycloak-js) found 5 non-blocking nits. These are cleanup items that don't affect functionality but should be addressed for code quality. ### File Targets - `src/lib/keycloak.ts` line 20 — misleading "check-sso" comment (no onLoad: 'check-sso' is actually passed). Remove or correct the comment. - `src/lib/api-client.ts` line 106 — dead `NoteLink` interface (defined but never imported). Remove it. - `.env.example` — stale references to old server-side env vars (`PAL_E_DOCS_API_URL`, `PAL_E_DOCS_API_KEY`, `AUTH_*`). Replace with new client-side `VITE_*` vars. - `src/lib/api-client.ts` — duplicate `COLUMNS` definition. Remove and import from `$lib/columns` instead (canonical source, already exported and used by 3 pages). - `src/routes/+layout.svelte` lines 171-198 — near-identical CSS for `.nav-login-link` and `.nav-logout-btn`. Consolidate into a shared class. ### Acceptance Criteria - [ ] No misleading comments about check-sso in keycloak.ts - [ ] No dead interfaces in api-client.ts - [ ] .env.example reflects new client-side env vars (VITE_*) - [ ] COLUMNS imported from $lib/columns (single source of truth) - [ ] Login/logout button CSS consolidated ### Test Expectations - [ ] `npm run check` passes - [ ] `npm run build` passes - Run command: `npm run check && npm run build` ### Constraints - Small cleanup — no architectural changes - Must not break any existing functionality - Note: mcd-tracker-app has the same misleading check-sso comment — that's a separate discovered-scope issue, not in scope here ### Checklist - [ ] PR opened - [ ] Tests pass - [ ] No unrelated changes ### Related - `forgejo_admin/pal-e-app#52` — parent migration issue - `forgejo_admin/pal-e-app#55` — PR where nits were found
forgejo_admin commented 2026-03-27 03:31:41 +00:00
Author
Owner
Copy link

Scope Review: NEEDS_REFINEMENT

Review note: review-424-2026-03-26

All 5 file targets verified against codebase -- every nit confirmed real. Template complete, traceability triangle intact, no blockers.

Two refinements needed:

  • Add COLUMNS consolidation direction to File Targets: specify that api-client.ts should import from $lib/columns (the canonical source, already used by 3 pages) rather than maintaining its own private copy.
  • Blast radius: mcd-tracker-app/src/lib/keycloak.js has the identical misleading "check-sso" comment with no actual onLoad: 'check-sso'. Needs a separate discovered-scope issue on forgejo_admin/mcd-tracker-app.
## Scope Review: NEEDS_REFINEMENT Review note: `review-424-2026-03-26` All 5 file targets verified against codebase -- every nit confirmed real. Template complete, traceability triangle intact, no blockers. Two refinements needed: - **Add COLUMNS consolidation direction** to File Targets: specify that `api-client.ts` should import from `$lib/columns` (the canonical source, already used by 3 pages) rather than maintaining its own private copy. - **Blast radius**: `mcd-tracker-app/src/lib/keycloak.js` has the identical misleading "check-sso" comment with no actual `onLoad: 'check-sso'`. Needs a separate discovered-scope issue on `forgejo_admin/mcd-tracker-app`.
forgejo_admin commented 2026-03-27 03:47:08 +00:00
Author
Owner
Copy link

Scope Review: READY

Review note: review-424-2026-03-26
Re-review passed. Both prior issues resolved: COLUMNS direction now specified ($lib/columns), mcd-tracker blast radius explicitly scoped out in Constraints.

## Scope Review: READY Review note: `review-424-2026-03-26` Re-review passed. Both prior issues resolved: COLUMNS direction now specified ($lib/columns), mcd-tracker blast radius explicitly scoped out in Constraints.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
226-fix-wget-auth
No results found.
Labels
Clear labels   
domain:backend

   
domain:devops

   
domain:frontend
No labels
domain:backend
domain:devops
domain:frontend
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
forgejo_admin/pal-e-docs-app#56
No description provided.