Purpose
Bundle of lower-impact candidates from the 2026-06-19 refactor review. Each needs a go/no-go evaluation before we commit — break out into its own issue once we decide to implement. The high-impact items were split out separately: #504, #505, #506.
All evidence below was validated against the code on 2026-06-19. Two of these (#1, #2) only misbehave under multi-worker deployment — production currently runs single-worker uvicorn (server/scripts/start.sh, no --workers/gunicorn), and both are already acknowledged in-code, so they're latent/forward-looking, not active bugs.
Candidates
1. Kiosk pairing nonce cache is process-local + IP-keyed (Low-Med, latent)
server/app/api/kiosk.py:42 _pair_nonces is an in-memory dict keyed by client IP; /pair-challenge overwrites per-IP (:119); /pair pops by IP before comparing the nonce (:133).- Edge cases: two kiosks behind one NAT clobber each other; a single failed nonce attempt consumes the valid challenge (pop precedes compare). TTL is 10s and pairing is rare/manual, so impact is small today.
- Comment at
:39-41 already flags the single-worker assumption. Eval question: will we ever need multi-worker, or multi-kiosk-per-NAT? If yes → DB-backed KioskPairChallenge keyed by nonce hash with client_ip / expires_at / used_at, keeping single-use semantics.
2. LLM recommendation rate limit is a per-process cache (Low, latent)
server/app/api/events.py:137 _llm_rate_limit_cache (module-level, carries a FIXME); slowapi reads it at :1027 (lambda: f"{_llm_rate_limit_cache['value']}/minute") before the endpoint body refreshes it from DB at :1042. The first request after an admin change lags by one and self-heals on a single worker.- Eval question: keep admin-tunable (then it needs a shared store/TTL for multi-worker), or make it a deploy/env constant and drop the dynamic limit?
3. Bridge & Electron bridge duplicate HTTP retry/backoff (Low-Med)
- Byte-identical constants in
bridge/src/bridge.ts:18-22 and bridge-app/src/main/bridge-runner.ts:29-33; both implement fetchWithTimeout / postWithRetry / circuit-breaker / DELETE-retry loops.
- Eval question: worth a shared helper via the existing
@bridge/* alias? Keep the Electron-specific 401 handling. Duplication is small and the two artifacts diverge legitimately, so this is convenience, not load-bearing.
4. Setbuilder reorder/document math split between a component and a module (Low-Med)
buildReorderedIds (drag, locked-slot anchor) lives in the React component dashboard/app/(dj)/setbuilder/components/BuilderWorkspace.tsx:40-55; the parallel buildMovedIds (one-step up/down, same invariant) is in the pure reorderMath.ts:12-27. Tests import the pure helper from the component (__tests__/buildReorderedIds.test.ts:2). Pure doc helpers (insertPoolTrackIntoDocument, lockSlotsInDocument) also live in the component.- Approach if pursued: move pure helpers into focused modules (
reorderMath.ts, documentMath.ts). These are parallel implementations of one invariant, not trivially mergeable into a single function — don't over-abstract.
5. Magic numbers worth promoting (Low-Med)
210s default track length duplicated cross-stack: server/app/services/setbuilder/pass1_deterministic.py:21 (AVG_TRACK_LENGTH_SEC) and dashboard/app/(dj)/setbuilder/components/types.ts:43 (DEFAULT_TRACK_DURATION_SEC) — a domain contract.- Export caps hardcoded at
10000 (server/app/api/events.py:141,144) — silent partial export; consider a setting or an explicit truncation indicator.
- Public list page size
100 repeats across join/collect/event pages while backend has DEFAULT_PAGE_SIZE and frontend exports PUBLIC_PAGE_MAX = 500 (dashboard/lib/api.ts:245); a PUBLIC_PAGE_DEFAULT would align clients. Do not promote CSS literals or algorithm weights.
6. Pydantic mutable-default style cleanup (style only)
list[str] = [] defaults in server/app/schemas/user.py:18 and server/app/schemas/recommendation.py (28-37, 55-59, 74). Pydantic v2 deep-copies these, so it's not a bug; Field(default_factory=list) is clearer and already used in schemas/setbuilder.py.
Note
Keep all of the above behavior-preserving and avoid broad abstractions (per SECURITY.md invariants + house DRY-without-over-abstraction rules).
Source: .worktrees/refactor-review-2026-06-19/docs/reviews/2026-06-19-refactor-review-handoff.md. Surfaced by Codex's refactor review; validated against the code by Claude.
Purpose
Bundle of lower-impact candidates from the 2026-06-19 refactor review. Each needs a go/no-go evaluation before we commit — break out into its own issue once we decide to implement. The high-impact items were split out separately: #504, #505, #506.
All evidence below was validated against the code on 2026-06-19. Two of these (#1, #2) only misbehave under multi-worker deployment — production currently runs single-worker uvicorn (
server/scripts/start.sh, no--workers/gunicorn), and both are already acknowledged in-code, so they're latent/forward-looking, not active bugs.Candidates
1. Kiosk pairing nonce cache is process-local + IP-keyed (Low-Med, latent)
server/app/api/kiosk.py:42_pair_noncesis an in-memory dict keyed by client IP;/pair-challengeoverwrites per-IP (:119);/pairpops by IP before comparing the nonce (:133).:39-41already flags the single-worker assumption. Eval question: will we ever need multi-worker, or multi-kiosk-per-NAT? If yes → DB-backedKioskPairChallengekeyed by nonce hash withclient_ip/expires_at/used_at, keeping single-use semantics.2. LLM recommendation rate limit is a per-process cache (Low, latent)
server/app/api/events.py:137_llm_rate_limit_cache(module-level, carries aFIXME); slowapi reads it at:1027(lambda: f"{_llm_rate_limit_cache['value']}/minute") before the endpoint body refreshes it from DB at:1042. The first request after an admin change lags by one and self-heals on a single worker.3. Bridge & Electron bridge duplicate HTTP retry/backoff (Low-Med)
bridge/src/bridge.ts:18-22andbridge-app/src/main/bridge-runner.ts:29-33; both implementfetchWithTimeout/postWithRetry/ circuit-breaker / DELETE-retry loops.@bridge/*alias? Keep the Electron-specific 401 handling. Duplication is small and the two artifacts diverge legitimately, so this is convenience, not load-bearing.4. Setbuilder reorder/document math split between a component and a module (Low-Med)
buildReorderedIds(drag, locked-slot anchor) lives in the React componentdashboard/app/(dj)/setbuilder/components/BuilderWorkspace.tsx:40-55; the parallelbuildMovedIds(one-step up/down, same invariant) is in the purereorderMath.ts:12-27. Tests import the pure helper from the component (__tests__/buildReorderedIds.test.ts:2). Pure doc helpers (insertPoolTrackIntoDocument,lockSlotsInDocument) also live in the component.reorderMath.ts,documentMath.ts). These are parallel implementations of one invariant, not trivially mergeable into a single function — don't over-abstract.5. Magic numbers worth promoting (Low-Med)
210s default track length duplicated cross-stack:server/app/services/setbuilder/pass1_deterministic.py:21(AVG_TRACK_LENGTH_SEC) anddashboard/app/(dj)/setbuilder/components/types.ts:43(DEFAULT_TRACK_DURATION_SEC) — a domain contract.10000(server/app/api/events.py:141,144) — silent partial export; consider a setting or an explicit truncation indicator.100repeats across join/collect/event pages while backend hasDEFAULT_PAGE_SIZEand frontend exportsPUBLIC_PAGE_MAX = 500(dashboard/lib/api.ts:245); aPUBLIC_PAGE_DEFAULTwould align clients. Do not promote CSS literals or algorithm weights.6. Pydantic mutable-default style cleanup (style only)
list[str] = []defaults inserver/app/schemas/user.py:18andserver/app/schemas/recommendation.py(28-37, 55-59, 74). Pydantic v2 deep-copies these, so it's not a bug;Field(default_factory=list)is clearer and already used inschemas/setbuilder.py.Note
Keep all of the above behavior-preserving and avoid broad abstractions (per
SECURITY.mdinvariants + house DRY-without-over-abstraction rules).Source:
.worktrees/refactor-review-2026-06-19/docs/reviews/2026-06-19-refactor-review-handoff.md. Surfaced by Codex's refactor review; validated against the code by Claude.