feat: add device profiles#950
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughProfiles are added across persistence, services, state, APIs, permissions, playtime enforcement, scan commands, launch gating, platform data swapping, CLI, TUI, pairing roles, and documentation. ChangesProfiles feature implementation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
pkg/service/profiles/resolver.go (1)
94-103: 💤 Low valueConsider explicitly handling "0" as a special case for clarity.
The code currently relies on
time.ParseDurationerror handling to interpret the string"0"(without a unit suffix) as unlimited. While this works defensively, explicitly handling"0"would make the intent clearer and align with the documented behavior in line 35.♻️ Proposed refactor for explicit "0" handling
func parseLimit(s string) time.Duration { - if s == "" { + if s == "" || s == "0" { return 0 } d, err := time.ParseDuration(s) if err != nil { return 0 } return d }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/service/profiles/resolver.go` around lines 94 - 103, The parseLimit function currently relies on time.ParseDuration to treat the string "0" as an invalid duration and return 0; update parseLimit to explicitly handle the literal "0" (or possibly "0s") as a special case by checking if s == "0" (and/or s == "0s") at the top and returning 0 immediately, then fall through to time.ParseDuration for other values and keep the existing error handling that returns 0 on parse failure; this clarifies intent in parseLimit and aligns with the documented behavior.pkg/service/profiles/profiles_test.go (1)
108-131: ⚡ Quick winRate limiting test doesn't cover the TOCTOU race.
TestActivateByID_RateLimitedverifies single-threaded rate limiting behavior but won't catch the race condition flagged inprofiles.go#L311-L346. The race only manifests when multiple goroutines callActivateByIDconcurrently for the same profile.📝 Example concurrent test to catch the race
func TestActivateByID_ConcurrentRateLimiting(t *testing.T) { t.Parallel() svc, mockDB, _ := newTestService(t) mockDB.On("GetProfile", "profile-1").Return(pinProfile(t, "1234"), nil) var wg sync.WaitGroup errors := make(chan error, 10) // Fire 10 concurrent wrong-PIN attempts for range 10 { wg.Add(1) go func() { defer wg.Done() _, err := svc.ActivateByID("profile-1", "9999") errors <- err }() } wg.Wait() close(errors) incorrectCount := 0 rateLimitedCount := 0 for err := range errors { if errors.Is(err, ErrPINIncorrect) { incorrectCount++ } else if errors.Is(err, ErrPINRateLimited) { rateLimitedCount++ } } // After fixing the race, we expect at most pinAttemptLimit incorrect, // rest should be rate-limited assert.LessOrEqual(t, incorrectCount, pinAttemptLimit) assert.GreaterOrEqual(t, rateLimitedCount, 10-pinAttemptLimit) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/service/profiles/profiles_test.go` around lines 108 - 131, Add a concurrent test that reproduces the TOCTOU race by invoking svc.ActivateByID concurrently for the same profile: create TestActivateByID_ConcurrentRateLimiting (or augment TestActivateByID_RateLimited) to spin up multiple goroutines that call ActivateByID with a wrong PIN while mocking GetProfile to return the known PIN, collect errors into a channel, then assert that at most pinAttemptLimit calls return ErrPINIncorrect and the remainder return ErrPINRateLimited; this will surface the race in profiles.go handling around ActivateByID and the pin attempt window logic so you can fix the synchronization there.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/api/methods.md`:
- Line 2458: The "Profile object" heading is using too many hashes (shown as
"##### Profile object") and jumps from the previous "##" level; update that
heading to the correct level to follow the surrounding section hierarchy (e.g.,
change "##### Profile object" to "### Profile object" or the appropriate level
used by the nearby "Profiles" section) so the markdown outline/navigation is
consistent; locate the literal "Profile object" heading in docs/api/methods.md
and adjust the hash count to match the surrounding headings.
In `@docs/api/notifications.md`:
- Around line 420-423: The docs declare the notifications parameter "profile"
with type "object" but its description says it can be null; update the parameter
type to explicitly show it is nullable (use "object | null") so the contract is
unambiguous—edit the table row for the "profile" field in the notifications API
docs and change the Type column from "object" to "object | null" while keeping
the existing description.
In `@pkg/api/models/params.go`:
- Around line 183-190: The handler currently accepts both ProfileID and SwitchID
and silently prefers ProfileID; update the HandleProfilesSwitch request
validation to enforce mutual exclusivity per SwitchProfileParams: if both params
are non-nil/non-empty return a 400 error (with a clear message like "provide
exactly one of profileId or switchId") instead of proceeding, and add a
unit/integration test that posts a payload containing both profileId and
switchId and asserts the handler responds with the expected bad-request error;
touch the SwitchProfileParams handling branch in HandleProfilesSwitch to perform
this check early before any branch-specific logic.
In `@pkg/service/profiles/pin.go`:
- Around line 61-64: The pbkdf2.Key call is incorrect: use the signature
Key(password []byte, salt []byte, iter int, keyLen int, h func() hash.Hash)
[]byte; convert the pin string to []byte and pass parameters in that order
(password, salt, pinIterations, pinKeyLen, sha256.New), capture only the
returned []byte (no err) e.g. key := pbkdf2.Key([]byte(pin), salt,
pinIterations, pinKeyLen, sha256.New), then proceed with whatever
encoding/return the derived key instead of checking a non-existent error; update
any references in the same function (e.g., where key, err were used) to reflect
the single returned value.
- Around line 94-97: The pbkdf2.Key call in pkg/service/profiles/pin.go is using
the wrong parameter order and types and incorrectly expects an error; update the
call to use pbkdf2.Key with password as []byte(pin), salt as the []byte salt,
iterations as the iter int, key length as len(expected), and sha256.New as the
final parameter (pbkdf2.Key returns only []byte), then remove the err handling
around that call and adjust subsequent logic that compares the resulting key
(e.g., where `expected` is used) to use the returned []byte key.
In `@pkg/service/profiles/profiles.go`:
- Around line 311-346: The checkPIN function suffers a TOCTOU race: it computes
recent from s.pinAttempts under s.mu, releases the lock to call VerifyPIN, then
later reuses the stale recent when appending, causing lost attempts; fix by
re-loading and re-filtering the current attempts after reacquiring the lock
(i.e., after VerifyPIN and before appending) or by keeping the lock across the
VerifyPIN call if acceptable; specifically, in checkPIN re-acquire s.mu and
recompute attempts := s.pinAttempts[p.ProfileID] and recent = filtered attempts
using pinAttemptWindow before doing s.pinAttempts[p.ProfileID] = append(recent,
now), using the same pinAttemptLimit/pinAttemptWindow variables to enforce rate
limiting correctly.
---
Nitpick comments:
In `@pkg/service/profiles/profiles_test.go`:
- Around line 108-131: Add a concurrent test that reproduces the TOCTOU race by
invoking svc.ActivateByID concurrently for the same profile: create
TestActivateByID_ConcurrentRateLimiting (or augment
TestActivateByID_RateLimited) to spin up multiple goroutines that call
ActivateByID with a wrong PIN while mocking GetProfile to return the known PIN,
collect errors into a channel, then assert that at most pinAttemptLimit calls
return ErrPINIncorrect and the remainder return ErrPINRateLimited; this will
surface the race in profiles.go handling around ActivateByID and the pin attempt
window logic so you can fix the synchronization there.
In `@pkg/service/profiles/resolver.go`:
- Around line 94-103: The parseLimit function currently relies on
time.ParseDuration to treat the string "0" as an invalid duration and return 0;
update parseLimit to explicitly handle the literal "0" (or possibly "0s") as a
special case by checking if s == "0" (and/or s == "0s") at the top and returning
0 immediately, then fall through to time.ParseDuration for other values and keep
the existing error handling that returns 0 on parse failure; this clarifies
intent in parseLimit and aligns with the documented behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d7327b2-dfc7-41e1-851b-52a67c72b3db
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (52)
docs/ARCHITECTURE.mddocs/api/methods.mddocs/api/notifications.mdgo.modpkg/api/methods/profiles.gopkg/api/methods/profiles_test.gopkg/api/methods/settings.gopkg/api/models/models.gopkg/api/models/params.gopkg/api/models/requests/requests.gopkg/api/models/responses.gopkg/api/notifications/notifications.gopkg/api/server.gopkg/api/server_post_test.gopkg/api/server_startup_test.gopkg/api/server_ws_e2e_test.gopkg/api/ws_dispatcher_test.gopkg/config/config.gopkg/config/configprofiles.gopkg/config/configprofiles_test.gopkg/database/database.gopkg/database/userdb/media_history.gopkg/database/userdb/media_history_property_test.gopkg/database/userdb/media_history_test.gopkg/database/userdb/migrations/20260612000000_create_profiles_table.sqlpkg/database/userdb/profiles.gopkg/database/userdb/profiles_test.gopkg/platforms/platforms.gopkg/service/context.gopkg/service/media_history_tracker.gopkg/service/playtime/limits.gopkg/service/playtime/profile_limits_test.gopkg/service/playtime/provider.gopkg/service/profiles/pin.gopkg/service/profiles/pin_test.gopkg/service/profiles/profiles.gopkg/service/profiles/profiles_test.gopkg/service/profiles/resolver.gopkg/service/profiles/resolver_test.gopkg/service/profiles/switchid.gopkg/service/profiles/switchid_test.gopkg/service/profiles/wordlist.txtpkg/service/queues.gopkg/service/require_profile_test.gopkg/service/scan_behavior_test.gopkg/service/service.gopkg/service/state/active_profile_test.gopkg/service/state/state.gopkg/testing/helpers/db_mocks.gopkg/zapscript/commands.gopkg/zapscript/profile.gopkg/zapscript/profile_test.go
Comment on lines
+183
to
+190
| // SwitchProfileParams switches the device's active profile. Exactly one of | ||
| // ProfileID or SwitchID selects the target; both omitted (or null) means | ||
| // deactivate. PIN is required when the target profile has one set. | ||
| type SwitchProfileParams struct { | ||
| ProfileID *string `json:"profileId"` | ||
| SwitchID *string `json:"switchId"` | ||
| PIN *string `json:"pin"` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify that the profiles.switch handler validates mutual exclusivity of ProfileID and SwitchID.
# Search for the switch handler implementation
rg -n -A 20 'MethodProfilesSwitch|profiles\.switch' --type=go -g '!*_test.go'Repository: ZaparooProject/zaparoo-core
Length of output: 2791
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate the handler implementation
rg -n --type=go 'HandleProfilesSwitch' .
# 2) Inspect nearby code for validation (look for ProfileID/SwitchID checks)
rg -n --type=go -S 'ProfileID|SwitchID|profileId|switchId|SwitchProfileParams' pkg/api
# 3) If found, print a focused excerpt around the handler function
# (Use rg again to find the file path and then show the surrounding lines)
file="$(rg -l --type=go 'func\s+HandleProfilesSwitch' . || true)"
echo "handler_file=$file"
if [ -n "$file" ]; then
rg -n --type=go -A 200 'func\s+HandleProfilesSwitch' "$file"
fiRepository: ZaparooProject/zaparoo-core
Length of output: 6274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show switch-related tests around HandleProfilesSwitch
rg -n 'TestHandleProfilesSwitch' pkg/api/methods/profiles_test.go
echo "----"
sed -n '1,280p' pkg/api/methods/profiles_test.goRepository: ZaparooProject/zaparoo-core
Length of output: 7982
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the params struct definition
sed -n '140,220p' pkg/api/models/params.go
# Inspect validation helper implementation (where ValidateAndUnmarshal lives)
rg -n --type=go 'func\s+ValidateAndUnmarshal' -S pkg | headRepository: ZaparooProject/zaparoo-core
Length of output: 3642
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show validation helper implementation
sed -n '1,220p' pkg/api/validation/validator.goRepository: ZaparooProject/zaparoo-core
Length of output: 6513
Enforce ProfileID/SwitchID mutual exclusivity in profiles.switch handler
SwitchProfileParams docs say “Exactly one of ProfileID or SwitchID selects the target”, but HandleProfilesSwitch simply prioritizes profileId when both are non-nil/non-empty (it takes the ProfileID branch first) and does not error on conflicting inputs. Add validation to reject requests where both profileId and switchId are provided (and cover with a test).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/api/models/params.go` around lines 183 - 190, The handler currently
accepts both ProfileID and SwitchID and silently prefers ProfileID; update the
HandleProfilesSwitch request validation to enforce mutual exclusivity per
SwitchProfileParams: if both params are non-nil/non-empty return a 400 error
(with a clear message like "provide exactly one of profileId or switchId")
instead of proceeding, and add a unit/integration test that posts a payload
containing both profileId and switchId and asserts the handler responds with the
expected bad-request error; touch the SwitchProfileParams handling branch in
HandleProfilesSwitch to perform this check early before any branch-specific
logic.
Comment on lines
+61
to
+64
| key, err := pbkdf2.Key(sha256.New, pin, salt, pinIterations, pinKeyLen) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to derive PIN hash: %w", err) | ||
| } |
There was a problem hiding this comment.
Critical: Incorrect pbkdf2.Key function call.
The call to pbkdf2.Key has multiple issues:
- Wrong parameter order: The signature is
func Key(password, salt []byte, iter, keyLen int, h func() hash.Hash) []byte, but the code passes the hash function first. - Wrong type: The
pinparameter is astringbut must be[]byte. - No error return:
pbkdf2.Keyreturns only[]byte, not([]byte, error).
🔧 Proposed fix
- key, err := pbkdf2.Key(sha256.New, pin, salt, pinIterations, pinKeyLen)
- if err != nil {
- return "", fmt.Errorf("failed to derive PIN hash: %w", err)
- }
+ key := pbkdf2.Key([]byte(pin), salt, pinIterations, pinKeyLen, sha256.New)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| key, err := pbkdf2.Key(sha256.New, pin, salt, pinIterations, pinKeyLen) | |
| if err != nil { | |
| return "", fmt.Errorf("failed to derive PIN hash: %w", err) | |
| } | |
| key := pbkdf2.Key([]byte(pin), salt, pinIterations, pinKeyLen, sha256.New) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/service/profiles/pin.go` around lines 61 - 64, The pbkdf2.Key call is
incorrect: use the signature Key(password []byte, salt []byte, iter int, keyLen
int, h func() hash.Hash) []byte; convert the pin string to []byte and pass
parameters in that order (password, salt, pinIterations, pinKeyLen, sha256.New),
capture only the returned []byte (no err) e.g. key := pbkdf2.Key([]byte(pin),
salt, pinIterations, pinKeyLen, sha256.New), then proceed with whatever
encoding/return the derived key instead of checking a non-existent error; update
any references in the same function (e.g., where key, err were used) to reflect
the single returned value.
Comment on lines
+94
to
+97
| key, err := pbkdf2.Key(sha256.New, pin, salt, iterations, len(expected)) | ||
| if err != nil { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Critical: Incorrect pbkdf2.Key function call (same issue as HashPIN).
The call to pbkdf2.Key has the same issues as in HashPIN:
- Wrong parameter order: Hash function should be the last parameter.
- Wrong type:
pinis astringbut must be[]byte. - No error return:
pbkdf2.Keyreturns only[]byte.
🔧 Proposed fix
- key, err := pbkdf2.Key(sha256.New, pin, salt, iterations, len(expected))
- if err != nil {
- return false
- }
+ key := pbkdf2.Key([]byte(pin), salt, iterations, len(expected), sha256.New)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/service/profiles/pin.go` around lines 94 - 97, The pbkdf2.Key call in
pkg/service/profiles/pin.go is using the wrong parameter order and types and
incorrectly expects an error; update the call to use pbkdf2.Key with password as
[]byte(pin), salt as the []byte salt, iterations as the iter int, key length as
len(expected), and sha256.New as the final parameter (pbkdf2.Key returns only
[]byte), then remove the err handling around that call and adjust subsequent
logic that compares the resulting key (e.g., where `expected` is used) to use
the returned []byte key.
Comment on lines
+311
to
+346
| func (s *Service) checkPIN(p *database.Profile, pin string) error { | ||
| if p.PINHash == "" { | ||
| return nil | ||
| } | ||
| if pin == "" { | ||
| return ErrPINRequired | ||
| } | ||
|
|
||
| s.mu.Lock() | ||
| now := s.now() | ||
| attempts := s.pinAttempts[p.ProfileID] | ||
| recent := attempts[:0] | ||
| for _, at := range attempts { | ||
| if now.Sub(at) < pinAttemptWindow { | ||
| recent = append(recent, at) | ||
| } | ||
| } | ||
| if len(recent) >= pinAttemptLimit { | ||
| s.pinAttempts[p.ProfileID] = recent | ||
| s.mu.Unlock() | ||
| return ErrPINRateLimited | ||
| } | ||
| s.mu.Unlock() | ||
|
|
||
| if !VerifyPIN(pin, p.PINHash) { | ||
| s.mu.Lock() | ||
| s.pinAttempts[p.ProfileID] = append(recent, now) | ||
| s.mu.Unlock() | ||
| return ErrPINIncorrect | ||
| } | ||
|
|
||
| s.mu.Lock() | ||
| delete(s.pinAttempts, p.ProfileID) | ||
| s.mu.Unlock() | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Race condition in PIN rate limiting allows bypass.
The checkPIN method has a TOCTOU (time-of-check to time-of-use) race between lines 333 and 336. The recent slice is computed inside the first lock (lines 322–327), then the lock is released to call VerifyPIN (line 335), then reacquired (line 336) and the stale recent is appended to (line 337).
Race scenario:
- Goroutine A: locks, reads
attempts = [], filters torecent = [], unlocks, starts VerifyPIN (slow) - Goroutine B: locks, reads
attempts = [], filters torecent = [], unlocks, calls VerifyPIN (wrong PIN), locks, appends to map:attempts = [t1], unlocks - Goroutine A: finishes VerifyPIN (wrong PIN), locks, sets
attempts = append([], t2) = [t2], unlocks
Result: The t1 attempt is lost. Future checks see only [t2] instead of [t1, t2], allowing extra PIN attempts beyond the limit.
🔒 Proposed fix: re-filter attempts after acquiring the second lock
if !VerifyPIN(pin, p.PINHash) {
s.mu.Lock()
- s.pinAttempts[p.ProfileID] = append(recent, now)
+ now := s.now()
+ attempts := s.pinAttempts[p.ProfileID]
+ recent := attempts[:0]
+ for _, at := range attempts {
+ if now.Sub(at) < pinAttemptWindow {
+ recent = append(recent, at)
+ }
+ }
+ s.pinAttempts[p.ProfileID] = append(recent, now)
s.mu.Unlock()
return ErrPINIncorrect
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/service/profiles/profiles.go` around lines 311 - 346, The checkPIN
function suffers a TOCTOU race: it computes recent from s.pinAttempts under
s.mu, releases the lock to call VerifyPIN, then later reuses the stale recent
when appending, causing lost attempts; fix by re-loading and re-filtering the
current attempts after reacquiring the lock (i.e., after VerifyPIN and before
appending) or by keeping the lock across the VerifyPIN call if acceptable;
specifically, in checkPIN re-acquire s.mu and recompute attempts :=
s.pinAttempts[p.ProfileID] and recent = filtered attempts using pinAttemptWindow
before doing s.pinAttempts[p.ProfileID] = append(recent, now), using the same
pinAttemptLimit/pinAttemptWindow variables to enforce rate limiting correctly.
Resolves conflicts and adapts profiles to code that moved on main: - rename profiles migration to 20260714000000 (goose rejects out-of-order migrations; main shipped 20260624120000 after the branch point) - re-implement profile-scoped daily usage on the SumMediaPlayTimeForDay aggregate (profile-scoped variant) instead of the removed pagination loop - drop GetMediaHistoryByProfile paging API (superseded by the aggregate) - add profiles.active to the LimitsManager broker method filter so profile switches reach ResetSession - adapt userdb profile queries to the Conn atomic pointer (db.sql.Load()) - restore session from history after the active profile is restored - go-zapscript v0.15.0
…oles Reworks device profiles following review: - Switch IDs are bearer credentials: presenting one authorizes a PIN-free switch on every path. The profiles list only returns switch IDs to local/admin clients, the API switchId path drops its PIN check, and PINs protect only pick-from-list switching by profileId. - Playtime: the session resets only on profile identity change (rescans and edits no longer reset it); the limits context is pinned at media start, so deactivating mid-game keeps the launch profile's limits until the media stops; deactivation never wipes cooldown state; combo cards that switch profile before launching pass the require-for-launch gate. - Permissions: Role column on paired clients, chosen at pairing approval (default member), checked through a capability lookup (pkg/api/permissions). Profile CRUD and settings updates require the admin capability. Local connections default to admin; unpaired remote clients keep full access while service.encryption is off. - profiles.verify: check a profile PIN or switch ID without switching, sharing the switch path's PIN attempt rate limiter. - Rename the ZapScript command profile.switch to profile (go-zapscript v0.16.0). - PBKDF2 iterations 600k -> 50k, negative limit durations rejected, PIN attempt accounting under a single lock, activation serialized, and docs rewritten around the shared-profile framing and trust model.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/api/methods/profiles.go (1)
1-1: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win"Exactly one of profileId/switchId" is documented but never enforced, in both the handlers and the params structs.
HandleProfilesVerifyandHandleProfilesSwitchboth prioritizeprofileIdwhenswitchIdis also present instead of rejecting the ambiguous request, andVerifyProfileParams(like the pre-existingSwitchProfileParams) has no validation tag backing its "exactly one of" docstring.
pkg/api/methods/profiles.go#L216-225: inHandleProfilesVerify's switch statement, reject the request when bothparams.ProfileIDandparams.SwitchIDare non-empty instead of silently taking theProfileIDbranch.pkg/api/methods/profiles.go#L265-284: apply the same fix inHandleProfilesSwitch(previously flagged and still unresolved).pkg/api/models/params.go#L223-230: addvalidate:"excluded_with=SwitchID"/excluded_with=ProfileID"toVerifyProfileParams.ProfileID/SwitchID(and consider the same forSwitchProfileParams) so the contract is enforced declaratively rather than relying on handler code alone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/methods/profiles.go` at line 1, The profile handlers and parameter models do not enforce the documented exactly-one-of contract. Update HandleProfilesVerify and HandleProfilesSwitch to reject requests when both ProfileID and SwitchID are non-empty, while preserving their existing single-identifier branches; add reciprocal excluded_with validation tags to VerifyProfileParams and SwitchProfileParams fields so ProfileID and SwitchID cannot both be supplied.
🧹 Nitpick comments (5)
pkg/database/userdb/migrations/20260714010000_client_roles.sql (1)
4-4: 🗄️ Data Integrity & Integration | 🔵 TrivialConfirm the migration policy for existing clients.
DEFAULT 'member'backfills every pre-existing client as a member. With the newsettings.writeandprofiles.managegates, existing paired clients may immediately lose administrative capabilities, and no promotion path is shown in the reviewed clients API. Confirm this downgrade is intentional and provide a documented re-pair or role-update path if needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/database/userdb/migrations/20260714010000_client_roles.sql` at line 4, Update the Clients role migration and related client-management flow to preserve the intended administrative access for existing paired clients, either by assigning the appropriate initial role or explicitly confirming the member downgrade. If existing clients should retain elevated capabilities, add the corresponding role backfill and a documented re-pair or role-update path through the reviewed clients API.pkg/api/pairing_test.go (1)
127-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for admin and invalid pairing roles.
The helper currently asserts only
"member", and all updated pairing flows use that role. Add an"admin"case and an invalid-role case verifying fallback to"member"; these branches determine privileged capability assignment.As per coding guidelines,
**/*.gorequires tests for all new code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/pairing_test.go` around lines 127 - 131, Add test coverage alongside the existing pairing assertions for both an explicit "admin" role and an invalid role. Verify the admin flow preserves the admin role and privileged capability assignment, while the invalid-role flow falls back to "member"; reuse the existing pairing test setup and helpers rather than changing the current member-case assertions.Source: Coding guidelines
pkg/database/userdb/media_history.go (1)
493-517: 🚀 Performance & Scalability | 🔵 TrivialConsider an index on
MediaHistory.ProfileID(and/or a composite withEndTime).
SumMediaPlayTimeForDayByProfilenow filters byProfileIDon every call, and this looks like it will run on each session-limit check. If the migration adding this column doesn't already add a supporting index, this will full-scanMediaHistoryas it grows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/database/userdb/media_history.go` around lines 493 - 517, Update the migration that adds MediaHistory.ProfileID, or the relevant schema setup, to create a supporting index for the ProfileID filter used by sqlSumMediaPlayTimeForDay. Prefer a composite index beginning with ProfileID and including EndTime if consistent with existing database conventions, while preserving existing query behavior for unfiltered calls.pkg/database/database.go (1)
556-619: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew functions placed before several type declarations.
TagTypeDisplayRank,isFourDigitYear, andBuildTitleZapScriptare inserted ahead ofMediaWithFullPath, theScanStaged*/ScanReconcile*types, and theUserDBI/MediaDBIinterfaces further down the file. Move these functions below the type/const block they're mixed into.As per path instructions, "Define Go types and consts near the top of the file, before functions and methods."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/database/database.go` around lines 556 - 619, Move TagTypeDisplayRank, isFourDigitYear, and BuildTitleZapScript below the file’s type and const declarations, including MediaWithFullPath, ScanStaged*/ScanReconcile* types, and UserDBI/MediaDBI interfaces. Preserve their implementations and ordering relative to one another while keeping type and const definitions near the top of the file.Source: Path instructions
pkg/api/models/params.go (1)
223-230: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDocstring promises exclusivity that isn't validated here.
VerifyProfileParamsdocuments "Exactly one of ProfileID ... or SwitchID ... must be given" but carries novalidatetag enforcing it — the handler (HandleProfilesVerifyinpkg/api/methods/profiles.go) doesn't enforce it either. Consider avalidate:"excluded_with=SwitchID"/excluded_with=ProfileID"pair on these fields so the contract is enforced declaratively.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/models/params.go` around lines 223 - 230, Add validation tags to VerifyProfileParams.ProfileID and VerifyProfileParams.SwitchID so each excludes the other, enforcing the documented mutual exclusivity declaratively. Preserve PIN behavior and ensure the validation still permits either credential path while rejecting requests that provide both.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/api/methods/profiles.go`:
- Around line 216-225: Enforce that exactly one identifier is supplied in both
HandleProfilesVerify and HandleProfilesSwitch: reject requests where profileId
and switchId are both non-empty, while retaining the existing missing-identifier
error and single-identifier verification/switch behavior.
In `@pkg/database/userdb/clients_test.go`:
- Line 64: The SQL round-trip tests use an empty client role and do not verify
role persistence. Update newTestClient and its test cases to assign realistic
roles ("admin" and "member"), then assert got.Role alongside the existing client
fields in each affected read test, including the referenced lines.
---
Outside diff comments:
In `@pkg/api/methods/profiles.go`:
- Line 1: The profile handlers and parameter models do not enforce the
documented exactly-one-of contract. Update HandleProfilesVerify and
HandleProfilesSwitch to reject requests when both ProfileID and SwitchID are
non-empty, while preserving their existing single-identifier branches; add
reciprocal excluded_with validation tags to VerifyProfileParams and
SwitchProfileParams fields so ProfileID and SwitchID cannot both be supplied.
---
Nitpick comments:
In `@pkg/api/models/params.go`:
- Around line 223-230: Add validation tags to VerifyProfileParams.ProfileID and
VerifyProfileParams.SwitchID so each excludes the other, enforcing the
documented mutual exclusivity declaratively. Preserve PIN behavior and ensure
the validation still permits either credential path while rejecting requests
that provide both.
In `@pkg/api/pairing_test.go`:
- Around line 127-131: Add test coverage alongside the existing pairing
assertions for both an explicit "admin" role and an invalid role. Verify the
admin flow preserves the admin role and privileged capability assignment, while
the invalid-role flow falls back to "member"; reuse the existing pairing test
setup and helpers rather than changing the current member-case assertions.
In `@pkg/database/database.go`:
- Around line 556-619: Move TagTypeDisplayRank, isFourDigitYear, and
BuildTitleZapScript below the file’s type and const declarations, including
MediaWithFullPath, ScanStaged*/ScanReconcile* types, and UserDBI/MediaDBI
interfaces. Preserve their implementations and ordering relative to one another
while keeping type and const definitions near the top of the file.
In `@pkg/database/userdb/media_history.go`:
- Around line 493-517: Update the migration that adds MediaHistory.ProfileID, or
the relevant schema setup, to create a supporting index for the ProfileID filter
used by sqlSumMediaPlayTimeForDay. Prefer a composite index beginning with
ProfileID and including EndTime if consistent with existing database
conventions, while preserving existing query behavior for unfiltered calls.
In `@pkg/database/userdb/migrations/20260714010000_client_roles.sql`:
- Line 4: Update the Clients role migration and related client-management flow
to preserve the intended administrative access for existing paired clients,
either by assigning the appropriate initial role or explicitly confirming the
member downgrade. If existing clients should retain elevated capabilities, add
the corresponding role backfill and a documented re-pair or role-update path
through the reviewed clients API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b4e933c7-b44c-4255-93d3-322b79043511
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
docs/ARCHITECTURE.mddocs/api/methods.mdgo.modpkg/api/integration_encryption_test.gopkg/api/methods/clients.gopkg/api/methods/pairing.gopkg/api/methods/pairing_test.gopkg/api/methods/permissions.gopkg/api/methods/profiles.gopkg/api/methods/profiles_test.gopkg/api/methods/settings.gopkg/api/middleware/encryption.gopkg/api/models/models.gopkg/api/models/params.gopkg/api/models/requests/requests.gopkg/api/models/responses.gopkg/api/models/responses_test.gopkg/api/pairing.gopkg/api/pairing_fuzz_test.gopkg/api/pairing_test.gopkg/api/permissions/permissions.gopkg/api/permissions/permissions_test.gopkg/api/server.gopkg/api/server_startup_test.gopkg/api/ws_dispatcher_test.gopkg/config/config.gopkg/database/database.gopkg/database/userdb/clients_test.gopkg/database/userdb/media_history.gopkg/database/userdb/media_history_test.gopkg/database/userdb/migrations/20260714000000_create_profiles_table.sqlpkg/database/userdb/migrations/20260714010000_client_roles.sqlpkg/database/userdb/profiles.gopkg/database/userdb/profiles_test.gopkg/database/userdb/sql.go
🚧 Files skipped from review as they are similar to previous changes (11)
- pkg/api/models/requests/requests.go
- pkg/api/ws_dispatcher_test.go
- pkg/api/models/models.go
- pkg/api/models/responses.go
- docs/ARCHITECTURE.md
- pkg/api/server_startup_test.go
- pkg/api/server.go
- pkg/database/userdb/profiles.go
- pkg/config/config.go
- pkg/database/userdb/profiles_test.go
- pkg/database/userdb/media_history_test.go
Requests supplying both profileId and switchId to profiles.switch or profiles.verify are now rejected instead of silently preferring profileId, matching the params docs. Also assert client Role round trips in the userdb SQL tests, fix a heading level jump in the API docs, and mark the profiles.active notification payload as nullable.
Add profile and client roles, TUI management, recovery CLI actions, and last-used tracking. Implement transactional MiSTer profile-data swaps with rollback and mount recovery.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/service/profiles/profiles.go (1)
263-267: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSerialize active snapshot refreshes with activation.
Updatecan read profile A as active, race with activation of profile B, and then overwrite the in-memory snapshot back to A while the database records B. Guard the active check andSetActiveProfilecall withactivateMu.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/service/profiles/profiles.go` around lines 263 - 267, In Update, protect the active-profile check and the subsequent SetActiveProfile call with s.activateMu so snapshot refreshes cannot race with activation. Keep the existing ProfileID condition and snapshot update behavior unchanged while holding the lock.pkg/database/userdb/profiles_test.go (1)
270-270: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winConstruct test media paths with
filepath.Join.These slash-separated literals violate the repository’s cross-platform path rule.
Proposed fix
- MediaPath: "snes/game.sfc", + MediaPath: filepath.Join("snes", "game.sfc"), ... - MediaPath: "nes/other.nes", + MediaPath: filepath.Join("nes", "other.nes"),As per coding guidelines,
**/*.go: “Usefilepath.Joinfor path construction everywhere, including test files.”Also applies to: 288-288
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/database/userdb/profiles_test.go` at line 270, Update the test media path literals in the affected profile test cases, including the entries near MediaPath at both referenced locations, to construct paths with filepath.Join instead of slash-separated strings. Add or reuse the filepath import as needed while preserving the existing path components and test behavior.Source: Coding guidelines
🧹 Nitpick comments (5)
pkg/platforms/mister/profiledata.go (1)
52-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded/concatenated POSIX paths instead of
filepath.Join.
mountLedgerPath(65) is a raw literal anddeviceBinPath(69) is built via string concatenation; the same pattern recurs at line 236 (fmt.Sprintf("/media/usb%d", i)) and lines 346/394 (/proc/self/mountinfoliteral, duplicated frommounts.go).As per coding guidelines, "Use
filepath.Joinfor path construction everywhere, including test files — never hardcode POSIX-style paths like "/roms/snes/game.sfc" as string literals."♻️ Example fix for the two constants
- mountLedgerPath = "/run/zaparoo/mounts.json" + mountLedgerPath = "" // set via filepath.Join in an init/var, e.g.: + // var mountLedgerPath = filepath.Join("/run", "zaparoo", "mounts.json")- deviceBinPath = misterconfig.CoreConfigFolder + "/device.bin" + deviceBinPath = filepath.Join(misterconfig.CoreConfigFolder, "device.bin")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/platforms/mister/profiledata.go` around lines 52 - 73, Replace hardcoded or concatenated filesystem paths in profiledata.go with filepath.Join, including mountLedgerPath, deviceBinPath, the USB path construction around the USB-root loop, and the /proc/self/mountinfo references. Reuse the existing shared mountinfo path symbol from mounts.go where available, and preserve the current resolved paths and behavior.Source: Coding guidelines
pkg/platforms/mister/profiledata_test.go (1)
200-234: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded literal mount/pool paths in tests.
e.g.
"/media/fat/saves"(208),"/media/fat/zaparoo/profiles/"+kidA().ID+"/saves"(210) — built via string concatenation/literals rather thanfilepath.Join. The guideline explicitly calls out test files.As per coding guidelines, "Use
filepath.Joinfor path construction everywhere, including test files — never hardcode POSIX-style paths like "/roms/snes/game.sfc" as string literals."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/platforms/mister/profiledata_test.go` around lines 200 - 234, The TestApply_SDPlainDir test hardcodes POSIX mount and pool paths instead of constructing them portably. Update the path assertions and related mountsAt, DirExists, and filepath usage in TestApply_SDPlainDir to build paths with filepath.Join, including the base media, profiles, profile ID, and directory components; preserve the existing expected mount behavior.Source: Coding guidelines
pkg/platforms/mister/mounts.go (1)
61-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded
/proc/self/mountinfopath literal.Per coding guidelines, POSIX-style paths should never be hardcoded as string literals — construct via
filepath.Join. The same literal is repeated inprofiledata.go(WatchProfileData,mountInfoSum).As per coding guidelines, "Use
filepath.Joinfor path construction everywhere, including test files — never hardcode POSIX-style paths like "/roms/snes/game.sfc" as string literals."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/platforms/mister/mounts.go` around lines 61 - 67, Replace the hardcoded "/proc/self/mountinfo" path in sysMounter.Mounts, WatchProfileData, and mountInfoSum with filepath.Join components. Ensure all mountinfo reads and related path handling use the constructed path consistently, including tests if applicable.Source: Coding guidelines
pkg/ui/tui/profiles_test.go (1)
171-185: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReinvents
strings.Containsfor multiple substrings.
containsAllmanually scans for each substring;strings.Containsfrom the standard library does the same thing more simply.♻️ Simplify with `strings.Contains`
+import "strings" + func containsAll(s string, subs ...string) bool { for _, sub := range subs { - found := false - for i := 0; i+len(sub) <= len(s); i++ { - if s[i:i+len(sub)] == sub { - found = true - break - } - } - if !found { + if !strings.Contains(s, sub) { return false } } return true }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ui/tui/profiles_test.go` around lines 171 - 185, Update containsAll to use the standard-library strings.Contains for each substring instead of manually scanning string indices, preserving the existing behavior of returning false when any substring is absent and true when all are present.pkg/ui/tui/profiles.go (1)
43-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsts scattered through the file instead of grouped at the top.
profileListHelpand the overlay page-name const block sit between function definitions rather than before them.As per coding guidelines, "Define Go types and consts near the top of the file, before functions and methods."
♻️ Suggested reorganization
+const profileListHelp = "Select a profile to edit. Use New to create one or Switch to change active profile." + +// Overlay page names used by profile switching. +const ( + profileSwitchModalPage = "profile_switch_modal" + profilePINModalPage = "profile_pin_modal" + profilePINEditModalPage = "profile_pin_edit_modal" + profileSwitchIDModalPage = "profile_switch_id_modal" +) + func profileCardZapScript(switchID string) string { return "**" + zapscript.ZapScriptCmdProfile + ":" + switchID } - -const profileListHelp = "..." ... -// Overlay page names used by profile switching. -const ( - profileSwitchModalPage = "profile_switch_modal" - ... -)Also applies to: 80-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ui/tui/profiles.go` at line 43, Move the profileListHelp constant and the overlay page-name const block currently located between functions to the top-level declarations before all functions and methods in profiles.go, keeping their names and values unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/api/pairing.go`:
- Around line 456-460: Remove the count-based role reassignment in the pairing
completion flow around sess.role: do not set RoleAdmin when count is zero or
otherwise derive a role from the completion-time client count. Validate the
existing sess.role with permissions.ValidRole and persist it unchanged, while
retaining the existing fallback for invalid roles if required by the surrounding
logic.
In `@pkg/database/userdb/clients_test.go`:
- Around line 225-241: Replace the mock-only coverage in
TestSqlDeleteClient_LastAdminProtected with a real-database integration test
that creates two admin clients, verifies one can be deleted, then verifies
deleting the remaining admin returns ErrLastClientAdmin. Exercise the actual
sqlDeleteClient predicate and retain cleanup and assertions for both deletion
outcomes.
In `@pkg/platforms/mister/profiledata.go`:
- Around line 297-319: Update the ledger.add failure handling around
mountLedgerEntry so the in-memory ledger removes the entry regardless of whether
the compensating d.m.Unmount(plan.target) succeeds. Preserve the joined ledger
and unmount errors, but ensure the failed bind is not retained as an owned entry
when unmount also fails.
In `@pkg/service/profiles/dataswap.go`:
- Around line 221-232: Preserve switch ordering in
DataSwapCoordinator.onMediaStopped by synchronizing deferred-target enqueueing
with newer RequestSwitch calls under c.mu, or by adding sequence tracking that
rejects stale deferred requests. Ensure a concurrent direct switch to B cannot
be displaced by the older deferred target A, while retaining the effectiveRef
resolution and success signaling. Add a regression test covering this
interleaving.
In `@pkg/service/profiles/profiles.go`:
- Around line 274-285: The Delete method must fully deactivate the profile
before deletion completes. Coordinate the active-profile path with activateMu,
transactionally clear the persisted active-profile device state, update s.st via
SetActiveProfile(nil), and request the shared-profile data swap; preserve the
existing error return and locking behavior for non-active profiles.
In `@pkg/ui/tui/profiles.go`:
- Around line 611-618: The profile setup flow around GetProfiles must fail
closed on lookup errors rather than replacing the error response with an empty
ProfilesResponse. Update the profilesErr handling in this flow so adminSetup
does not become true and local admin actions still pass through
promptProfileManagement, matching BuildProfilesPage behavior while preserving
successful-response handling.
In `@pkg/ui/tui/settings_components.go`:
- Around line 444-456: Move the SettingsTextEditOptions type declaration to the
top section of settings_components.go, alongside the other type declarations and
before any functions or methods, without changing its fields or behavior.
- Around line 120-130: Update formatTextValue to pass the dynamic value through
tview.Escape before inserting it into either formatted output branch, while
leaving the theme styling unchanged. Move the SettingsTextEditOptions
declaration nearer the top of the file to comply with the Go declaration-order
convention.
---
Outside diff comments:
In `@pkg/database/userdb/profiles_test.go`:
- Line 270: Update the test media path literals in the affected profile test
cases, including the entries near MediaPath at both referenced locations, to
construct paths with filepath.Join instead of slash-separated strings. Add or
reuse the filepath import as needed while preserving the existing path
components and test behavior.
In `@pkg/service/profiles/profiles.go`:
- Around line 263-267: In Update, protect the active-profile check and the
subsequent SetActiveProfile call with s.activateMu so snapshot refreshes cannot
race with activation. Keep the existing ProfileID condition and snapshot update
behavior unchanged while holding the lock.
---
Nitpick comments:
In `@pkg/platforms/mister/mounts.go`:
- Around line 61-67: Replace the hardcoded "/proc/self/mountinfo" path in
sysMounter.Mounts, WatchProfileData, and mountInfoSum with filepath.Join
components. Ensure all mountinfo reads and related path handling use the
constructed path consistently, including tests if applicable.
In `@pkg/platforms/mister/profiledata_test.go`:
- Around line 200-234: The TestApply_SDPlainDir test hardcodes POSIX mount and
pool paths instead of constructing them portably. Update the path assertions and
related mountsAt, DirExists, and filepath usage in TestApply_SDPlainDir to build
paths with filepath.Join, including the base media, profiles, profile ID, and
directory components; preserve the existing expected mount behavior.
In `@pkg/platforms/mister/profiledata.go`:
- Around line 52-73: Replace hardcoded or concatenated filesystem paths in
profiledata.go with filepath.Join, including mountLedgerPath, deviceBinPath, the
USB path construction around the USB-root loop, and the /proc/self/mountinfo
references. Reuse the existing shared mountinfo path symbol from mounts.go where
available, and preserve the current resolved paths and behavior.
In `@pkg/ui/tui/profiles_test.go`:
- Around line 171-185: Update containsAll to use the standard-library
strings.Contains for each substring instead of manually scanning string indices,
preserving the existing behavior of returning false when any substring is absent
and true when all are present.
In `@pkg/ui/tui/profiles.go`:
- Line 43: Move the profileListHelp constant and the overlay page-name const
block currently located between functions to the top-level declarations before
all functions and methods in profiles.go, keeping their names and values
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b2261d9-55ef-43e9-aa8f-fd9b01e1f0c9
📒 Files selected for processing (54)
docs/ARCHITECTURE.mddocs/api/methods.mddocs/api/notifications.mdpkg/api/methods/clients.gopkg/api/methods/clients_test.gopkg/api/methods/pairing.gopkg/api/methods/pairing_test.gopkg/api/methods/permissions.gopkg/api/methods/profiles.gopkg/api/methods/profiles_test.gopkg/api/methods/settings.gopkg/api/methods/settings_test.gopkg/api/models/models.gopkg/api/models/params.gopkg/api/models/responses.gopkg/api/notifications/notifications.gopkg/api/pairing.gopkg/api/pairing_test.gopkg/cli/cli.gopkg/cli/profiles.gopkg/cli/profiles_test.gopkg/config/configprofiles.gopkg/database/database.gopkg/database/userdb/clients.gopkg/database/userdb/clients_test.gopkg/database/userdb/migrations/20260714020000_profile_roles.sqlpkg/database/userdb/migrations/20260714030000_profile_last_used.sqlpkg/database/userdb/profiles.gopkg/database/userdb/profiles_test.gopkg/database/userdb/sql.gopkg/platforms/mister/mounts.gopkg/platforms/mister/platform.gopkg/platforms/mister/profiledata.gopkg/platforms/mister/profiledata_test.gopkg/platforms/profiles.gopkg/service/profiles/dataswap.gopkg/service/profiles/dataswap_test.gopkg/service/profiles/profiles.gopkg/service/profiles/profiles_test.gopkg/service/require_profile_test.gopkg/service/service.gopkg/testing/helpers/db_mocks.gopkg/ui/tui/clients.gopkg/ui/tui/clients_test.gopkg/ui/tui/main.gopkg/ui/tui/mock_api_client_test.gopkg/ui/tui/profiles.gopkg/ui/tui/profiles_test.gopkg/ui/tui/settings.gopkg/ui/tui/settings_components.gopkg/ui/tui/settings_integration_test.gopkg/ui/tui/settings_service.gopkg/ui/tui/utils.gopkg/ui/tui/utils_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/api/methods/clients.go
- pkg/api/methods/permissions.go
- docs/api/notifications.md
- pkg/api/models/models.go
- pkg/api/methods/pairing.go
- pkg/service/require_profile_test.go
- pkg/service/service.go
- pkg/api/models/params.go
- docs/api/methods.md
- pkg/api/models/responses.go
- pkg/api/pairing_test.go
- pkg/api/methods/profiles.go
- pkg/api/methods/profiles_test.go
Comment on lines
+274
to
+285
| func (s *Service) Delete(profileID string) error { | ||
| s.manageMu.Lock() | ||
| defer s.manageMu.Unlock() | ||
|
|
||
| if err := s.db.UserDB.DeleteProfile(profileID); err != nil { | ||
| return fmt.Errorf("failed to delete profile: %w", err) | ||
| } | ||
|
|
||
| if active := s.st.ActiveProfile(); active != nil && active.ProfileID == profileID { | ||
| s.st.SetActiveProfile(nil) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Fully deactivate an active profile before completing deletion.
Deleting the active profile only clears the in-memory snapshot. The persisted active-profile device state remains dangling, and data swapping is never requested back to shared state. Coordinate deletion with activateMu and transactionally clear persisted state, update the snapshot, and request the shared-profile swap.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/service/profiles/profiles.go` around lines 274 - 285, The Delete method
must fully deactivate the profile before deletion completes. Coordinate the
active-profile path with activateMu, transactionally clear the persisted
active-profile device state, update s.st via SetActiveProfile(nil), and request
the shared-profile data swap; preserve the existing error return and locking
behavior for non-active profiles.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/ui/tui/settings_components.go (1)
192-202: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMove
settingsItembefore the functions.The expanded
settingsItemtype remains afterformatTextValue(Lines [134]-[146]). Move this declaration alongside the other top-level types, before functions and methods.As per coding guidelines,
**/*.go: Define Go types and consts near the top of the file, before functions and methods.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/ui/tui/settings_components.go` around lines 192 - 202, Move the settingsItem type declaration, including its fields, from its current location after formatTextValue to the top-level type section before any functions and methods in settings_components.go. Preserve the declaration unchanged and keep the existing functions and methods in their current order.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/api/pairing_test.go`:
- Around line 131-132: Update the pairing handshake test around runHandshake to
accept the expected role as an argument instead of hardcoding "member". Add a
successful StartPairing("admin") handshake case that passes "admin" to
runHandshake and verifies administrator-role propagation, while preserving the
existing member and invalid-role fallback coverage.
---
Outside diff comments:
In `@pkg/ui/tui/settings_components.go`:
- Around line 192-202: Move the settingsItem type declaration, including its
fields, from its current location after formatTextValue to the top-level type
section before any functions and methods in settings_components.go. Preserve the
declaration unchanged and keep the existing functions and methods in their
current order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ebe3ae9f-4b1d-48b9-a0a7-d7e3655fddcc
📒 Files selected for processing (15)
pkg/api/pairing.gopkg/api/pairing_test.gopkg/database/userdb/clients_test.gopkg/database/userdb/profiles_test.gopkg/platforms/mister/mounts.gopkg/platforms/mister/profiledata.gopkg/platforms/mister/profiledata_test.gopkg/service/profiles/dataswap.gopkg/service/profiles/dataswap_test.gopkg/service/profiles/profiles.gopkg/service/profiles/profiles_test.gopkg/ui/tui/profiles.gopkg/ui/tui/profiles_test.gopkg/ui/tui/settings_components.gopkg/ui/tui/settings_components_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- pkg/platforms/mister/mounts.go
- pkg/service/profiles/dataswap_test.go
- pkg/platforms/mister/profiledata.go
- pkg/service/profiles/dataswap.go
- pkg/database/userdb/clients_test.go
- pkg/ui/tui/profiles_test.go
- pkg/ui/tui/profiles.go
- pkg/platforms/mister/profiledata_test.go
- pkg/service/profiles/profiles.go
- pkg/service/profiles/profiles_test.go
- pkg/database/userdb/profiles_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds device profiles: named buckets of preferences and playtime limits, with no passwords or accounts. One profile is active per device at a time; the un-profiled state is the implicit shared profile — the device as it behaves when nobody is signed in (global-config limits, unattributed history).
Profiles
**profile:<switchId>/**profile.clearZapScript commands for card-based switching (go-zapscript v0.16.0).profileId.profiles.verify: check a PIN or switch ID without switching, so clients can gate their own UI items behind a credential. Shares the switch path's PIN rate limiter; grants nothing server-side.Playtime
LimitsProviderseam; daily usage accounting scoped to the active profile's attributed history (MediaHistory.ProfileID, fixed at launch).require_for_launchsetting stops the shared profile launching media; combo cards that switch profile before launching pass the gate.Permissions
Rolecolumn on paired clients (admin/member), chosen at pairing approval (default member), checked through a capability lookup (pkg/api/permissions). Profile CRUD and settings updates require the admin capability; local connections default to admin.service.encryptionis off, unpaired remote clients keep full API access (compatibility with the current app); enabling it requires pairing and makes member restrictions enforceable.Trust model
Household-appliance grade, documented in
docs/api/methods.md: protects against gaming playtime limits via a member client, via cards (switch IDs are possession tokens), or via rescans. Does not protect against OS access or admin clients.Design doc for the follow-up profile data swapping (saves/savestates/emulator config per profile) was written and this PR's switching semantics were checked against it; implementation is out of scope here, as are per-profile favourites, credentials, and history/stats API methods.
Summary by CodeRabbit
profiles.switch/clear behavior.profiles.datanotifications (including deferral while media runs).