feat: admin players is_public toggle (#122) #125

Merged

forgejo_admin merged 1 commit from 122-admin-visibility-toggle into main

2026-03-27 21:25:12 +00:00

forgejo_admin commented

2026-03-27 21:11:12 +00:00

Owner

Summary

Add is_public visibility toggle to each player row on the admin players page
Toggle calls PATCH /admin/players/{id}/visibility with {"is_public": true/false}
Shows transient feedback: saving, saved, or failed

Changes

src/routes/(app)/admin/players/+page.svelte: Replaced full-row <a> link with <div> card-row so toggle click does not navigate. Player name remains a clickable link. Added toggle switch per row with async PATCH call, optimistic local state update, and scoped CSS for track/thumb/label using existing design tokens (no Tailwind).

Test Plan

Tests pass locally (build verified)
Navigate to /admin/players while authenticated as admin
Each player row shows a toggle on the right reflecting is_public state
Clicking toggle shows "..." then "Saved" on success, or "Failed" on error
Toggle track turns green when public, gray when hidden
Player name link still navigates to detail page
Search and filter tabs work unchanged

Review Checklist

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

Closes #122
project-westside -- westside-app admin tooling

## Summary - Add is_public visibility toggle to each player row on the admin players page - Toggle calls PATCH /admin/players/{id}/visibility with {"is_public": true/false} - Shows transient feedback: saving, saved, or failed ## Changes - `src/routes/(app)/admin/players/+page.svelte`: Replaced full-row `<a>` link with `<div>` card-row so toggle click does not navigate. Player name remains a clickable link. Added toggle switch per row with async PATCH call, optimistic local state update, and scoped CSS for track/thumb/label using existing design tokens (no Tailwind). ## Test Plan - [ ] Tests pass locally (build verified) - [ ] Navigate to /admin/players while authenticated as admin - [ ] Each player row shows a toggle on the right reflecting is_public state - [ ] Clicking toggle shows "..." then "Saved" on success, or "Failed" on error - [ ] Toggle track turns green when public, gray when hidden - [ ] Player name link still navigates to detail page - [ ] Search and filter tabs work unchanged ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive ## Related Notes - Closes #122 - `project-westside` -- westside-app admin tooling

forgejo_admin added 1 commit

2026-03-27 21:11:12 +00:00

feat: add is_public visibility toggle to admin players page

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

792e92d895

Each player row now has a toggle switch that PATCHes
/admin/players/{id}/visibility to flip is_public on/off.
Visual feedback shows saving/saved/failed states.

Closes #122

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

forgejo_admin commented

2026-03-27 21:11:46 +00:00

Author

Owner

QA Review -- PR #125

Diff Analysis (1 file, +130/-3)

src/routes/(app)/admin/players/+page.svelte

toggleVisibility() -- async PATCH with proper state machine (saving/saved/error/null). Local state only updates on success, not optimistic. Timeout clears feedback after 1.5s (success) / 2.5s (error). Correct pattern.
Row structure -- <a> wrapper correctly replaced with <div>. Player name becomes the link instead, preventing toggle clicks from navigating.
Accessibility -- aria-label, aria-pressed, focus-visible outline, disabled during save. All present.
CSS -- Scoped <style> block. Uses existing design tokens (--color-green, --color-gray-*, --color-blue, --color-white). No Tailwind. Toggle dimensions (40x22px track, 14px thumb) are appropriately sized for mobile touch targets.
No secrets, no unrelated file changes.

Nits

None.

VERDICT: APPROVED

## QA Review -- PR #125 ### Diff Analysis (1 file, +130/-3) **`src/routes/(app)/admin/players/+page.svelte`** - `toggleVisibility()` -- async PATCH with proper state machine (saving/saved/error/null). Local state only updates on success, not optimistic. Timeout clears feedback after 1.5s (success) / 2.5s (error). Correct pattern. - Row structure -- `<a>` wrapper correctly replaced with `<div>`. Player name becomes the link instead, preventing toggle clicks from navigating. - Accessibility -- `aria-label`, `aria-pressed`, `focus-visible` outline, `disabled` during save. All present. - CSS -- Scoped `<style>` block. Uses existing design tokens (`--color-green`, `--color-gray-*`, `--color-blue`, `--color-white`). No Tailwind. Toggle dimensions (40x22px track, 14px thumb) are appropriately sized for mobile touch targets. - No secrets, no unrelated file changes. ### Nits None. ### VERDICT: APPROVED

forgejo_admin commented

2026-03-27 21:16:23 +00:00

Author

Owner

PR #125 Review

DOMAIN REVIEW

Tech stack: SvelteKit 5 (Svelte 5 runes), vanilla CSS with design tokens, Keycloak auth via apiFetch.

API integration -- Correct. The toggleVisibility function calls PATCH /admin/players/{id}/visibility with { is_public: boolean } payload via apiFetch, which automatically attaches the Bearer token from Keycloak. This matches the auth pattern used by every other admin page in the repo (/admin/dashboard, /admin/players GET, etc.). The Content-Type: application/json header is set by apiFetch by default.

State management -- Sound. This is NOT an optimistic update (despite the PR description saying "optimistic"). Local state (allPlayers) is only mutated AFTER the API call succeeds (line 73-74 is inside try, after await). On failure, the toggle stays in its original position and shows "Failed" for 2.5s. This is the safer pattern -- good choice.

CSS -- All variables (--color-green, --color-gray-800, --color-gray-600, --color-gray-200, --color-gray-400, --color-white, --color-blue) are confirmed defined in src/app.css. No Tailwind. No magic numbers -- toggle dimensions (40x22px, 14px thumb, 18px translateX) are internally consistent. Scoped <style> block keeps styles local.

Layout change -- The <a class="card-row"> wrapper was correctly replaced with a <div class="card-row">, moving the link to just the player name. This prevents the toggle click from triggering navigation. The .card-row class in app.css uses display: flex with align-items: flex-start, and the toggle cell uses margin-left: auto to push right -- this is the correct flexbox pattern.

Accessibility -- Good foundation:

aria-label="Toggle public visibility for {player.name}" -- descriptive, per-player
aria-pressed correctly reflects is_public state
disabled during saving prevents double-clicks
:focus-visible outline for keyboard users

BLOCKERS

None.

Test coverage note: This repo has zero test infrastructure (no vitest, no playwright, no test files). The "new functionality with zero test coverage" blocker criterion does not apply here -- there is no testing framework to write tests against. This is a pre-existing repo-level gap, not a PR-level omission. The Test Plan in the PR body is manual and appropriate given the current state.

NITS

Missing role="switch": The toggle button semantically acts as a switch. Adding role="switch" alongside the existing aria-pressed would give screen readers the correct widget type announcement. Currently it announces as a button with pressed state, which is functional but not ideal.
cursor: pointer on .card-row: The global .card-row class in app.css (line 1467) sets cursor: pointer. Now that the row is a <div> instead of an <a>, the entire row shows a pointer cursor but is not itself clickable. Consider adding a scoped override: .card-row { cursor: default; } in the component's <style> block so the pointer only appears on the name link and toggle button.
Inline styles on the name link: style="text-decoration:none;color:inherit;" on the <a> tag could be a scoped CSS class (e.g., .player-link) for consistency with the rest of the file's approach.
PR body says "optimistic local state update": The implementation is actually a success-first update (state mutates after API confirmation). The PR description is slightly misleading -- minor, but worth correcting for documentation accuracy.

SOP COMPLIANCE

Branch named after issue (122-admin-visibility-toggle references #122)
PR body has Summary, Changes, Test Plan, Related sections
Related references project-westside and closes #122
No secrets committed
No unnecessary file changes (single file, tightly scoped)
Commit message is descriptive (feat: admin players is_public toggle (#122))

PROCESS OBSERVATIONS

Scope: Extremely tight -- 1 file, 130 additions, 3 deletions. Clean feature addition with no side effects.
Change failure risk: Low. The toggle is additive (new UI element on existing page), the API call is behind auth, and failure states are handled gracefully.
Testing gap: westside-app has no test framework. This is a repo-level technical debt item, not a PR-level concern. Worth tracking as a separate Forgejo issue if not already tracked.
DORA impact: No deployment frequency concern. The change is self-contained and low risk.

VERDICT: APPROVED

## PR #125 Review ### DOMAIN REVIEW **Tech stack**: SvelteKit 5 (Svelte 5 runes), vanilla CSS with design tokens, Keycloak auth via `apiFetch`. **API integration** -- Correct. The `toggleVisibility` function calls `PATCH /admin/players/{id}/visibility` with `{ is_public: boolean }` payload via `apiFetch`, which automatically attaches the Bearer token from Keycloak. This matches the auth pattern used by every other admin page in the repo (`/admin/dashboard`, `/admin/players` GET, etc.). The `Content-Type: application/json` header is set by `apiFetch` by default. **State management** -- Sound. This is NOT an optimistic update (despite the PR description saying "optimistic"). Local state (`allPlayers`) is only mutated AFTER the API call succeeds (line 73-74 is inside `try`, after `await`). On failure, the toggle stays in its original position and shows "Failed" for 2.5s. This is the safer pattern -- good choice. **CSS** -- All variables (`--color-green`, `--color-gray-800`, `--color-gray-600`, `--color-gray-200`, `--color-gray-400`, `--color-white`, `--color-blue`) are confirmed defined in `src/app.css`. No Tailwind. No magic numbers -- toggle dimensions (40x22px, 14px thumb, 18px translateX) are internally consistent. Scoped `<style>` block keeps styles local. **Layout change** -- The `<a class="card-row">` wrapper was correctly replaced with a `<div class="card-row">`, moving the link to just the player name. This prevents the toggle click from triggering navigation. The `.card-row` class in `app.css` uses `display: flex` with `align-items: flex-start`, and the toggle cell uses `margin-left: auto` to push right -- this is the correct flexbox pattern. **Accessibility** -- Good foundation: - `aria-label="Toggle public visibility for {player.name}"` -- descriptive, per-player - `aria-pressed` correctly reflects `is_public` state - `disabled` during saving prevents double-clicks - `:focus-visible` outline for keyboard users ### BLOCKERS None. **Test coverage note**: This repo has zero test infrastructure (no vitest, no playwright, no test files). The "new functionality with zero test coverage" blocker criterion does not apply here -- there is no testing framework to write tests against. This is a pre-existing repo-level gap, not a PR-level omission. The Test Plan in the PR body is manual and appropriate given the current state. ### NITS 1. **Missing `role="switch"`**: The toggle button semantically acts as a switch. Adding `role="switch"` alongside the existing `aria-pressed` would give screen readers the correct widget type announcement. Currently it announces as a button with pressed state, which is functional but not ideal. 2. **`cursor: pointer` on `.card-row`**: The global `.card-row` class in `app.css` (line 1467) sets `cursor: pointer`. Now that the row is a `<div>` instead of an `<a>`, the entire row shows a pointer cursor but is not itself clickable. Consider adding a scoped override: `.card-row { cursor: default; }` in the component's `<style>` block so the pointer only appears on the name link and toggle button. 3. **Inline styles on the name link**: `style="text-decoration:none;color:inherit;"` on the `<a>` tag could be a scoped CSS class (e.g., `.player-link`) for consistency with the rest of the file's approach. 4. **PR body says "optimistic local state update"**: The implementation is actually a success-first update (state mutates after API confirmation). The PR description is slightly misleading -- minor, but worth correcting for documentation accuracy. ### SOP COMPLIANCE - [x] Branch named after issue (`122-admin-visibility-toggle` references #122) - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related references `project-westside` and closes #122 - [x] No secrets committed - [x] No unnecessary file changes (single file, tightly scoped) - [x] Commit message is descriptive (`feat: admin players is_public toggle (#122)`) ### PROCESS OBSERVATIONS - **Scope**: Extremely tight -- 1 file, 130 additions, 3 deletions. Clean feature addition with no side effects. - **Change failure risk**: Low. The toggle is additive (new UI element on existing page), the API call is behind auth, and failure states are handled gracefully. - **Testing gap**: westside-app has no test framework. This is a repo-level technical debt item, not a PR-level concern. Worth tracking as a separate Forgejo issue if not already tracked. - **DORA impact**: No deployment frequency concern. The change is self-contained and low risk. ### VERDICT: APPROVED