Add backend to store user settings in the DB #958
Conversation
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
Contributor
WalkthroughThis PR adds user settings management by introducing two new authenticated endpoints ( Changes
Sequence DiagramsequenceDiagram
actor User
participant Handler as API Handler
participant Service as UserService
participant Repo as UsersRepository
participant DB as Database
autonumber
rect rgb(200,230,255)
Note over User,Handler: GET /v1/users/self/settings
User->>Handler: GET + Bearer token
Handler->>Service: GetSettings(ctx, userID)
Service->>Repo: GetSettings(ctx, userID)
Repo->>DB: SELECT settings FROM users WHERE id=...
DB-->>Repo: settings JSON
Repo-->>Service: settings
Service-->>Handler: settings
Handler->>User: 200 OK (Cache-Control: no-store) + wrapped settings
end
rect rgb(230,200,255)
Note over User,Handler: PUT /v1/users/self/settings
User->>Handler: PUT + Bearer token + payload
Handler->>Handler: cap body (64KB) and decode JSON
alt decode error
Handler-->>User: 400 Bad Request
else success
Handler->>Service: SetSettings(ctx, userID, settings)
Service->>Repo: SetSettings(ctx, userID, settings)
Repo->>DB: UPDATE users SET settings=... WHERE id=...
DB-->>Repo: ok
Repo-->>Service: ok
Service->>Handler: ok
Handler->>Service: GetSettings(ctx, userID)
Service->>Repo: GetSettings(ctx, userID)
Repo->>DB: SELECT settings ...
DB-->>Repo: updated settings
Repo-->>Service: settings
Service-->>Handler: settings
Handler->>User: 200 OK (Cache-Control: no-store) + wrapped updated settings
end
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Security Recommendations
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
backend/app/api/routes.go (1)
87-88: LGTM on route wiring and auth scope.Endpoints are correctly protected by userMW with RoleUser. Paths are consistent with existing “self” resources.
Security recommendation:
- These endpoints return per-user data; consider adding Cache-Control: no-store at handler level (or via a global middleware for “/users/self/*”) to prevent proxy/browser caching of personalized settings.
[security]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (75)
backend/app/api/static/docs/docs.gois excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/swagger.jsonis excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/swagger.yamlis excluded by!backend/app/api/static/docs/**backend/go.sumis excluded by!**/*.sumbackend/internal/data/ent/attachment.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/attachment_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authroles.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authroles_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authroles_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authroles_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authroles_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authtokens.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authtokens_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authtokens_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authtokens_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/authtokens_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/client.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/ent.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/group_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/groupinvitationtoken_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/has_id.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/item.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/item_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/item_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/item_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/item_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/itemfield.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/itemfield_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/itemfield_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/itemfield_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/itemfield_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/label.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/label_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/label_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/label_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/label_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/location.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/location_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/location_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/location_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/location_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/maintenanceentry.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/maintenanceentry_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/maintenanceentry_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/maintenanceentry_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/maintenanceentry_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/migrate/schema.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/mutation.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/notifier.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/notifier_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/notifier_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/notifier_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/notifier_update.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/runtime/runtime.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/user.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user/user.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user/where.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_delete.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_query.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_update.gois excluded by!backend/internal/data/ent/**docs/en/api/openapi-2.0.jsonis excluded by!docs/en/api/**docs/en/api/openapi-2.0.yamlis excluded by!docs/en/api/**
📒 Files selected for processing (4)
backend/app/api/handlers/v1/v1_ctrl_user.go(2 hunks)backend/app/api/routes.go(1 hunks)backend/go.mod(2 hunks)backend/internal/core/services/service_user.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/app/api/handlers/v1/v1_ctrl_user.go (3)
backend/app/api/handlers/v1/controller.go (2)
V1Controller(67-77)Wrap(33-35)backend/internal/core/services/contexts.go (2)
UseUserCtx(53-58)Context(19-30)backend/internal/sys/validate/errors.go (1)
NewRequestError(57-59)
backend/internal/core/services/service_user.go (1)
backend/internal/core/services/contexts.go (1)
Context(19-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Frontend Tests / Integration Tests
- GitHub Check: Backend Server Tests / Go
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm/v7)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
backend/internal/core/services/service_user.go (2)
11-11: Import of schema for UserSettings is appropriate.No issues spotted; keeps service signatures aligned with repository types.
265-271: Verify ownership, behavior, and schema of UserSettings before merging
- In backend/internal/data/repo/repo_users.go you’ll see:
• GetSettings → r.db.User.Get(ctx, uid) (fetch by ID only)
• SetSettings → r.db.User.UpdateOneID(uid).SetSettings(settings).Exec(ctx)
These enforce “by‐ID” semantics at the DB level.- In the service layer (backend/internal/core/services/service_user.go), both methods are thin wrappers; make sure every call site (e.g. handlers) passes exactly the authenticated user’s UID (actor.ID) and never an arbitrary UUID.
- Document the update semantics of SetSettings clearly: it entirely replaces the stored blob rather than merging. Clients must know whether they need to send the full settings or partial patches.
- Locate and review the definition of schema.UserSettings (in your schema package, e.g. backend/internal/core/schema):
• If it’s a JSON‐backed map, enforce a strict key whitelist or shape validation so that no unexpected fields (or scripts) can be stored and later rendered (mitigates stored XSS or config injection).
• Add a maximum‐size validation on the settings payload to prevent denial-of-service via oversized blobs.Security recommendation (always):
- Validate shape/allowed keys at the service or repo boundary.
- Enforce a size limit on the settings JSON before persisting.
backend/app/api/handlers/v1/v1_ctrl_user.go (1)
12-12: Importing schema here is correct and keeps handler payloads typed.No action needed.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
tankerkiller125
force-pushed
the
mk/db-settings
branch
from
2de8267 to
f835a34
Compare
Deploying homebox-docs with
|
| Latest commit: |
782aa6a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a0bf075a.homebox-docs.pages.dev |
| Branch Preview URL: | https://mk-db-settings.homebox-docs.pages.dev |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/internal/data/migrations/sqlite3/20250823141400_user_settings.sql (1)
1-2: Consider adding a Down migration for rollback support.While this additive schema change works without a Down migration, including one (
-- +goose DownwithALTER TABLE users DROP COLUMN settings;) would provide a cleaner rollback path during development or in case of deployment issues.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (20)
backend/app/api/static/docs/docs.gois excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/openapi-3.jsonis excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/openapi-3.yamlis excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/swagger.jsonis excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/swagger.yamlis excluded by!backend/app/api/static/docs/**backend/go.sumis excluded by!**/*.sumbackend/internal/data/ent/migrate/schema.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/mutation.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/user.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user/user.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user/where.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_update.gois excluded by!backend/internal/data/ent/**docs/en/api/openapi-2.0.jsonis excluded by!docs/en/api/**docs/en/api/openapi-2.0.yamlis excluded by!docs/en/api/**docs/en/api/openapi-3.0.jsonis excluded by!docs/en/api/**docs/en/api/openapi-3.0.yamlis excluded by!docs/en/api/**docs/en/api/swagger-2.0.jsonis excluded by!docs/en/api/**docs/en/api/swagger-2.0.yamlis excluded by!docs/en/api/**
📒 Files selected for processing (6)
backend/app/api/handlers/v1/v1_ctrl_user.gobackend/app/api/routes.gobackend/internal/core/services/service_user.gobackend/internal/data/migrations/postgres/20250823141600_user_settings.sqlbackend/internal/data/migrations/sqlite3/20250823141400_user_settings.sqlbackend/internal/data/repo/repo_users.go
✅ Files skipped from review due to trivial changes (1)
- backend/internal/data/migrations/postgres/20250823141600_user_settings.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/api/routes.go
🧰 Additional context used
📓 Path-based instructions (2)
backend/**/*.go
📄 CodeRabbit inference engine (.github/instructions/code.instructions.md)
Backend Go code must pass golangci-lint validation with no errors (6-minute timeout in CI)
Files:
backend/app/api/handlers/v1/v1_ctrl_user.gobackend/internal/core/services/service_user.gobackend/internal/data/repo/repo_users.go
backend/app/api/handlers/**/*.go
📄 CodeRabbit inference engine (.github/instructions/code.instructions.md)
API handlers must be placed in
backend/app/api/handlers/directory and include Swagger documentation comments
Files:
backend/app/api/handlers/v1/v1_ctrl_user.go
🧠 Learnings (17)
📚 Learning: 2025-07-06T18:51:05.524Z
Learnt from: mcarbonne
Repo: sysadminsmedia/homebox PR: 869
File: backend/internal/data/migrations/sqlite3/20250706190000_fix_old_timestamps.sql:0-0
Timestamp: 2025-07-06T18:51:05.524Z
Learning: Goose database migrations are automatically wrapped in transactions by the migration framework, eliminating the need for explicit BEGIN TRANSACTION and COMMIT statements in migration scripts.
Applied to files:
backend/internal/data/migrations/sqlite3/20250823141400_user_settings.sql
📚 Learning: 2025-12-29T16:29:02.445Z
Learnt from: CR
Repo: sysadminsmedia/homebox PR: 0
File: .github/instructions/backend-app-api-handlers.instructions.md:0-0
Timestamp: 2025-12-29T16:29:02.445Z
Learning: Applies to /backend/app/api/handlers/v1/v1_ctrl_*.go : All handlers must extract context using `services.NewContext(r.Context(), ctrl.CurrentUser(r))` for auth and multi-tenancy
Applied to files:
backend/app/api/handlers/v1/v1_ctrl_user.go
📚 Learning: 2025-12-29T16:29:02.445Z
Learnt from: CR
Repo: sysadminsmedia/homebox PR: 0
File: .github/instructions/backend-app-api-handlers.instructions.md:0-0
Timestamp: 2025-12-29T16:29:02.445Z
Learning: Applies to /backend/app/api/handlers/v1/v1_ctrl_*.go : Every handler must have Swagger comments for API doc generation, including Summary, Tags, Router, and Security Bearer
Applied to files:
backend/app/api/handlers/v1/v1_ctrl_user.go
📚 Learning: 2025-12-29T16:29:02.445Z
Learnt from: CR
Repo: sysadminsmedia/homebox PR: 0
File: .github/instructions/backend-app-api-handlers.instructions.md:0-0
Timestamp: 2025-12-29T16:29:02.445Z
Learning: Applies to /backend/app/api/handlers/v1/v1_ctrl_*.go : Handler methods must be defined on V1Controller struct and return `errchain.HandlerFunc`
Applied to files:
backend/app/api/handlers/v1/v1_ctrl_user.go
📚 Learning: 2025-12-29T16:29:13.502Z
Learnt from: CR
Repo: sysadminsmedia/homebox PR: 0
File: .github/instructions/backend-internal-core-services.instructions.md:0-0
Timestamp: 2025-12-29T16:29:13.502Z
Learning: Applies to /backend/app/api/handlers/v1/*.go : Handlers should create service contexts using `services.NewContext(r.Context(), ctrl.CurrentUser(r))` and call services, not repositories directly
Applied to files:
backend/app/api/handlers/v1/v1_ctrl_user.gobackend/internal/core/services/service_user.go
📚 Learning: 2025-12-29T16:29:02.445Z
Learnt from: CR
Repo: sysadminsmedia/homebox PR: 0
File: .github/instructions/backend-app-api-handlers.instructions.md:0-0
Timestamp: 2025-12-29T16:29:02.445Z
Learning: Applies to /backend/app/api/handlers/v1/v1_ctrl_*.go : Swagger documentation must include Param definitions for all path parameters, query parameters, and request bodies
Applied to files:
backend/app/api/handlers/v1/v1_ctrl_user.go
📚 Learning: 2025-12-29T16:29:51.634Z
Learnt from: CR
Repo: sysadminsmedia/homebox PR: 0
File: .github/instructions/code.instructions.md:0-0
Timestamp: 2025-12-29T16:29:51.634Z
Learning: Applies to backend/internal/data/ent/schema/*.go : Backend schema definitions must be edited in `backend/internal/data/ent/schema/` directory (files ending with .go like `backend/internal/data/ent/schema/*.go`)
Applied to files:
backend/app/api/handlers/v1/v1_ctrl_user.gobackend/internal/core/services/service_user.gobackend/internal/data/repo/repo_users.go
📚 Learning: 2025-12-29T16:29:27.462Z
Learnt from: CR
Repo: sysadminsmedia/homebox PR: 0
File: .github/instructions/backend-internal-data.instructions.md:0-0
Timestamp: 2025-12-29T16:29:27.462Z
Learning: Applies to backend/internal/data/ent/schema/*.go : Always edit schema files in `ent/schema/` - these define your database entities. Use `mixins.BaseMixin{}` for `id`, `created_at`, `updated_at` fields; use `mixins.DetailsMixin{}` for `name` and `description` fields; use `GroupMixin{ref: "items"}` to link entities to groups; add indexes for frequently queried fields
Applied to files:
backend/app/api/handlers/v1/v1_ctrl_user.gobackend/internal/core/services/service_user.gobackend/internal/data/repo/repo_users.go
📚 Learning: 2025-12-29T16:29:51.634Z
Learnt from: CR
Repo: sysadminsmedia/homebox PR: 0
File: .github/instructions/code.instructions.md:0-0
Timestamp: 2025-12-29T16:29:51.634Z
Learning: Applies to backend/internal/data/ent/**/*.go : Never edit generated code in `backend/internal/data/ent/` directly; instead, edit source schema definitions in `backend/internal/data/ent/schema/` and run `task generate`
Applied to files:
backend/app/api/handlers/v1/v1_ctrl_user.gobackend/internal/core/services/service_user.gobackend/internal/data/repo/repo_users.go
📚 Learning: 2025-12-29T16:29:27.462Z
Learnt from: CR
Repo: sysadminsmedia/homebox PR: 0
File: .github/instructions/backend-internal-data.instructions.md:0-0
Timestamp: 2025-12-29T16:29:27.462Z
Learning: Applies to backend/internal/data/ent/schema/*.go : Use Ent field builders with appropriate constraints: `field.String().NotEmpty()`, `field.Int().Default(value)`, `field.Bool().Default(false)` to enforce data integrity at the database layer
Applied to files:
backend/app/api/handlers/v1/v1_ctrl_user.gobackend/internal/core/services/service_user.gobackend/internal/data/repo/repo_users.go
📚 Learning: 2025-12-29T16:29:51.634Z
Learnt from: CR
Repo: sysadminsmedia/homebox PR: 0
File: .github/instructions/code.instructions.md:0-0
Timestamp: 2025-12-29T16:29:51.634Z
Learning: Data access must use the Repository pattern via implementations in `backend/internal/data/repo/`
Applied to files:
backend/app/api/handlers/v1/v1_ctrl_user.gobackend/internal/core/services/service_user.gobackend/internal/data/repo/repo_users.go
📚 Learning: 2025-12-29T16:29:13.502Z
Learnt from: CR
Repo: sysadminsmedia/homebox PR: 0
File: .github/instructions/backend-internal-core-services.instructions.md:0-0
Timestamp: 2025-12-29T16:29:13.502Z
Learning: Applies to /backend/internal/core/services/service_*.go : Service structs should contain repository access via `repo *repo.AllRepos` field and optional configuration like feature flags
Applied to files:
backend/app/api/handlers/v1/v1_ctrl_user.gobackend/internal/core/services/service_user.gobackend/internal/data/repo/repo_users.go
📚 Learning: 2025-12-29T16:29:27.462Z
Learnt from: CR
Repo: sysadminsmedia/homebox PR: 0
File: .github/instructions/backend-internal-data.instructions.md:0-0
Timestamp: 2025-12-29T16:29:27.462Z
Learning: Applies to backend/internal/data/ent/schema/*.go : Define edges (relationships) in both related schema files with proper `Ref()` references to maintain relationship consistency
Applied to files:
backend/app/api/handlers/v1/v1_ctrl_user.gobackend/internal/core/services/service_user.gobackend/internal/data/repo/repo_users.go
📚 Learning: 2025-12-29T16:29:13.502Z
Learnt from: CR
Repo: sysadminsmedia/homebox PR: 0
File: .github/instructions/backend-internal-core-services.instructions.md:0-0
Timestamp: 2025-12-29T16:29:13.502Z
Learning: Applies to /backend/internal/core/services/**/*.go : Services should NOT construct SQL queries directly; all database access must go through repository methods
Applied to files:
backend/app/api/handlers/v1/v1_ctrl_user.go
📚 Learning: 2025-12-29T16:29:13.502Z
Learnt from: CR
Repo: sysadminsmedia/homebox PR: 0
File: .github/instructions/backend-internal-core-services.instructions.md:0-0
Timestamp: 2025-12-29T16:29:13.502Z
Learning: Applies to /backend/internal/core/services/**/*.go : Orchestrate multiple repository calls within service methods when a single operation requires multiple data access steps, with proper rollback on failure
Applied to files:
backend/app/api/handlers/v1/v1_ctrl_user.gobackend/internal/core/services/service_user.go
📚 Learning: 2025-12-29T16:29:13.502Z
Learnt from: CR
Repo: sysadminsmedia/homebox PR: 0
File: .github/instructions/backend-internal-core-services.instructions.md:0-0
Timestamp: 2025-12-29T16:29:13.502Z
Learning: Applies to /backend/internal/core/services/**/*.go : Service methods should validate input parameters before calling repository methods
Applied to files:
backend/app/api/handlers/v1/v1_ctrl_user.go
📚 Learning: 2025-12-29T16:29:27.462Z
Learnt from: CR
Repo: sysadminsmedia/homebox PR: 0
File: .github/instructions/backend-internal-data.instructions.md:0-0
Timestamp: 2025-12-29T16:29:27.462Z
Learning: Applies to backend/internal/data/repo/*_test.go : Repository tests should use `enttest` for in-memory SQLite and test both SQLite and PostgreSQL implementations
Applied to files:
backend/internal/core/services/service_user.gobackend/internal/data/repo/repo_users.go
🧬 Code graph analysis (2)
backend/app/api/handlers/v1/v1_ctrl_user.go (3)
backend/app/api/handlers/v1/controller.go (2)
V1Controller(68-79)Wrap(34-36)backend/internal/core/services/contexts.go (2)
UseUserCtx(53-58)Context(19-30)backend/internal/sys/validate/errors.go (1)
NewRequestError(57-59)
backend/internal/core/services/service_user.go (1)
backend/internal/core/services/contexts.go (1)
Context(19-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 2/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 4/4
- GitHub Check: Frontend Tests / Integration Tests PGSQL 15
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 1/4
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 3/4
- GitHub Check: Frontend Tests / Integration Tests PGSQL 17
- GitHub Check: Frontend Tests / Integration Tests PGSQL 16
- GitHub Check: Frontend Tests / Integration Tests
- GitHub Check: Backend Server Tests / Go
- GitHub Check: build (linux/amd64, ubuntu-latest)
- GitHub Check: build (linux/arm64, ubuntu-24.04-arm)
- GitHub Check: build (linux/amd64, ubuntu-latest)
- GitHub Check: build (linux/arm64, ubuntu-24.04-arm)
- GitHub Check: build (linux/amd64, ubuntu-latest)
- GitHub Check: build (linux/arm64, ubuntu-24.04-arm)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (go)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
backend/internal/data/repo/repo_users.go (2)
180-182: LGTM!The SetSettings method follows the established repository pattern and is consistent with similar methods like ChangePassword (line 177).
184-190: LGTM!The GetSettings method follows the existing repository pattern of retrieving the full entity and extracting the required field, consistent with other methods in this file.
backend/internal/core/services/service_user.go (2)
374-376: LGTM!The GetSettings method appropriately delegates to the repository layer with no additional logic needed for this read operation.
378-380: LGTM!The SetSettings method appropriately delegates to the repository layer. Input validation occurs during JSON decoding at the handler level (line 160 in v1_ctrl_user.go), which is consistent with the service pattern in this codebase.
backend/app/api/handlers/v1/v1_ctrl_user.go (2)
125-144: LGTM!The GET settings handler correctly:
- Retrieves the authenticated user from context
- Sets appropriate Cache-Control headers to prevent caching of personalized data
- Handles errors properly
- Follows Swagger documentation standards
146-178: LGTM!The PUT settings handler implements appropriate security measures:
- 64KB body size limit prevents DoS attacks via large payloads
- Cache-Control header prevents caching of personalized data
- Proper error handling and logging
- Follows the established pattern of re-fetching data after updates (consistent with HandleUserSelfUpdate, line 93)
The Swagger documentation correctly reflects the API contract.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/internal/data/migrations/sqlite3/20260103121200_user_settings.sql (2)
1-2: Consider adding NOT NULL constraint and ensure application-layer validation.Since a default value of '{}' is provided, adding a NOT NULL constraint would prevent NULL values and make the schema more explicit:
-- +goose Up -ALTER TABLE users ADD COLUMN settings JSON DEFAULT '{}'; +ALTER TABLE users ADD COLUMN settings JSON NOT NULL DEFAULT '{}';Security recommendation: Ensure that the application layer validates and sanitizes all user settings data before storing it in this column. JSON columns accept arbitrary data structures, so implement:
- Schema validation for allowed settings keys and value types
- Size limits to prevent storage abuse (consider max JSON document size)
- Sanitization of any user-provided values that might be reflected in the UI
1-2: Consider adding a down migration for reversibility.Adding a
-- +goose Downsection allows the migration to be rolled back if needed:-- +goose Up ALTER TABLE users ADD COLUMN settings JSON DEFAULT '{}'; + +-- +goose Down +ALTER TABLE users DROP COLUMN settings;backend/internal/data/migrations/postgres/20260103121100_user_settings.sql (2)
1-2: Consider adding NOT NULL constraint and ensure application-layer validation.Since a default value of '{}' is provided, adding a NOT NULL constraint would prevent NULL values and make the schema more explicit:
-- +goose Up -ALTER TABLE users ADD COLUMN settings JSONB DEFAULT '{}'; +ALTER TABLE users ADD COLUMN settings JSONB NOT NULL DEFAULT '{}';Security recommendation: Ensure that the application layer validates and sanitizes all user settings data before storing it in this column. JSONB columns accept arbitrary data structures, so implement:
- Schema validation for allowed settings keys and value types
- Size limits to prevent storage abuse (consider max JSON document size)
- Sanitization of any user-provided values that might be reflected in the UI
1-2: Consider adding a down migration for reversibility.Adding a
-- +goose Downsection allows the migration to be rolled back if needed:-- +goose Up ALTER TABLE users ADD COLUMN settings JSONB DEFAULT '{}'; + +-- +goose Down +ALTER TABLE users DROP COLUMN settings;
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/internal/data/migrations/postgres/20260103121100_user_settings.sqlbackend/internal/data/migrations/sqlite3/20260103121200_user_settings.sql
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-06T18:51:05.524Z
Learnt from: mcarbonne
Repo: sysadminsmedia/homebox PR: 869
File: backend/internal/data/migrations/sqlite3/20250706190000_fix_old_timestamps.sql:0-0
Timestamp: 2025-07-06T18:51:05.524Z
Learning: Goose database migrations are automatically wrapped in transactions by the migration framework, eliminating the need for explicit BEGIN TRANSACTION and COMMIT statements in migration scripts.
Applied to files:
backend/internal/data/migrations/postgres/20260103121100_user_settings.sqlbackend/internal/data/migrations/sqlite3/20260103121200_user_settings.sql
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Frontend Tests / Integration Tests
- GitHub Check: build (linux/arm64, ubuntu-24.04-arm)
- GitHub Check: Frontend Tests / Integration Tests PGSQL 17
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 1/4
- GitHub Check: Frontend Tests / Integration Tests PGSQL 16
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 4/4
- GitHub Check: Backend Server Tests / Go
- GitHub Check: build (linux/arm64, ubuntu-24.04-arm)
- GitHub Check: build (linux/amd64, ubuntu-latest)
- GitHub Check: build (linux/arm64, ubuntu-24.04-arm)
- GitHub Check: build (linux/amd64, ubuntu-latest)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (go)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/internal/data/repo/repo_users.go (1)
183-189: Return an empty map instead of nil for uninitialized settings.When settings haven't been initialized in the database, this method returns
(nil, nil). This forces callers to handle both nil checks and can lead to nil pointer errors. Following Go conventions, consider returning an empty map when settings are nil.🔎 Proposed refactor for consistent nil handling
func (r *UserRepository) GetSettings(ctx context.Context, uid uuid.UUID) (map[string]interface{}, error) { usr, err := r.db.User.Get(ctx, uid) if err != nil { return nil, err } + if usr.Settings == nil { + return make(map[string]interface{}), nil + } return usr.Settings, nil }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (16)
backend/app/api/static/docs/docs.gois excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/openapi-3.jsonis excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/openapi-3.yamlis excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/swagger.jsonis excluded by!backend/app/api/static/docs/**backend/app/api/static/docs/swagger.yamlis excluded by!backend/app/api/static/docs/**backend/internal/data/ent/mutation.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/schema/user.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_create.gois excluded by!backend/internal/data/ent/**backend/internal/data/ent/user_update.gois excluded by!backend/internal/data/ent/**docs/en/api/openapi-2.0.jsonis excluded by!docs/en/api/**docs/en/api/openapi-2.0.yamlis excluded by!docs/en/api/**docs/en/api/openapi-3.0.jsonis excluded by!docs/en/api/**docs/en/api/openapi-3.0.yamlis excluded by!docs/en/api/**docs/en/api/swagger-2.0.jsonis excluded by!docs/en/api/**docs/en/api/swagger-2.0.yamlis excluded by!docs/en/api/**
📒 Files selected for processing (4)
backend/app/api/handlers/v1/v1_ctrl_user.gobackend/internal/core/services/service_user.gobackend/internal/data/repo/repo_users.gofrontend/lib/api/types/data-contracts.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/api/handlers/v1/v1_ctrl_user.go
- backend/internal/core/services/service_user.go
🧰 Additional context used
📓 Path-based instructions (6)
frontend/lib/api/types/**/*.ts
📄 CodeRabbit inference engine (.github/instructions/code.instructions.md)
Never manually edit generated TypeScript API types in
frontend/lib/api/types/; these are auto-generated from the backend and should be regenerated usingtask generateNever edit generated types in
lib/api/types/- they are auto-generated from backend Swagger docs. Runtask generateafter backend API changes to regenerate
Files:
frontend/lib/api/types/data-contracts.ts
frontend/**/*.{ts,tsx,vue,js,jsx}
📄 CodeRabbit inference engine (.github/instructions/code.instructions.md)
Frontend TypeScript/Vue code must pass ESLint with a maximum of 1 warning in CI (
pnpm run lint:ci)
Files:
frontend/lib/api/types/data-contracts.ts
frontend/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (.github/instructions/code.instructions.md)
Frontend code must pass TypeScript type checking with
task ui:checkbefore PR submission
Files:
frontend/lib/api/types/data-contracts.ts
frontend/**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (.github/instructions/code.instructions.md)
Use Tailwind CSS utility classes for styling in Vue components instead of custom CSS
Files:
frontend/lib/api/types/data-contracts.ts
**/*.{ts,vue}
⚙️ CodeRabbit configuration file
**/*.{ts,vue}: Check for hardcoded strings in UI components that should be translatable.
Look for:
- String literals in Vue components (e.g. Click me)
- Alert messages, error messages, and user-facing text
- Placeholder text and labels
Files:
frontend/lib/api/types/data-contracts.ts
backend/**/*.go
📄 CodeRabbit inference engine (.github/instructions/code.instructions.md)
Backend Go code must pass golangci-lint validation with no errors (6-minute timeout in CI)
Files:
backend/internal/data/repo/repo_users.go
🧠 Learnings (1)
📚 Learning: 2025-12-29T16:29:13.502Z
Learnt from: CR
Repo: sysadminsmedia/homebox PR: 0
File: .github/instructions/backend-internal-core-services.instructions.md:0-0
Timestamp: 2025-12-29T16:29:13.502Z
Learning: Applies to /backend/internal/core/services/service_*.go : Service structs should contain repository access via `repo *repo.AllRepos` field and optional configuration like feature flags
Applied to files:
backend/internal/data/repo/repo_users.go
🧬 Code graph analysis (1)
backend/internal/data/repo/repo_users.go (1)
backend/internal/core/services/contexts.go (1)
Context(19-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: End-to-End Playwright Tests / E2E Playwright Testing 1/4
- GitHub Check: Frontend Tests / Integration Tests PGSQL 17
- GitHub Check: Frontend Tests / Integration Tests PGSQL 16
- GitHub Check: Backend Server Tests / Go
- GitHub Check: Frontend Tests / Lint
- GitHub Check: build (linux/arm64, ubuntu-24.04-arm)
- GitHub Check: build (linux/amd64, ubuntu-latest)
- GitHub Check: build (linux/arm64, ubuntu-24.04-arm)
- GitHub Check: build (linux/amd64, ubuntu-latest)
- GitHub Check: build (linux/arm64, ubuntu-24.04-arm)
- GitHub Check: build (linux/amd64, ubuntu-latest)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (go)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
frontend/lib/api/types/data-contracts.ts (1)
530-531: Generated type definition looks correct.This change properly reflects the backend's addition of a settings field to the user entity. Since this is an auto-generated file, ensure it was regenerated using
task generateafter the backend schema changes.Note: The
Record<string, any>type allows maximum flexibility for arbitrary user settings but sacrifices type safety. This is acceptable for generated code representing a dynamic JSON field.
Comment on lines
+179
to
+181
| func (r *UserRepository) SetSettings(ctx context.Context, uid uuid.UUID, settings map[string]interface{}) error { | ||
| return r.db.User.UpdateOneID(uid).SetSettings(settings).Exec(ctx) | ||
| } |
Contributor
There was a problem hiding this comment.
Consider adding size limits to prevent DoS attacks.
The method accepts arbitrary JSON settings without validation or size constraints. While the flexible approach aligns with the PR's goal of accepting any JSON input, unbounded settings could pose a denial-of-service risk through storage exhaustion or performance degradation.
Consider adding a maximum size check:
- Validate the serialized JSON size before persistence
- Define reasonable limits based on expected use cases (e.g., 1MB)
- Return a descriptive error if limits are exceeded
🔎 Example implementation with size validation
+import (
+ "encoding/json"
+ "fmt"
+)
+
+const maxSettingsSize = 1024 * 1024 // 1MB
+
func (r *UserRepository) SetSettings(ctx context.Context, uid uuid.UUID, settings map[string]interface{}) error {
+ // Validate settings size to prevent DoS
+ data, err := json.Marshal(settings)
+ if err != nil {
+ return fmt.Errorf("invalid settings format: %w", err)
+ }
+ if len(data) > maxSettingsSize {
+ return fmt.Errorf("settings exceed maximum size of %d bytes", maxSettingsSize)
+ }
+
return r.db.User.UpdateOneID(uid).SetSettings(settings).Exec(ctx)
}📝 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
| func (r *UserRepository) SetSettings(ctx context.Context, uid uuid.UUID, settings map[string]interface{}) error { | |
| return r.db.User.UpdateOneID(uid).SetSettings(settings).Exec(ctx) | |
| } | |
| import ( | |
| "encoding/json" | |
| "fmt" | |
| ) | |
| const maxSettingsSize = 1024 * 1024 // 1MB | |
| func (r *UserRepository) SetSettings(ctx context.Context, uid uuid.UUID, settings map[string]interface{}) error { | |
| // Validate settings size to prevent DoS | |
| data, err := json.Marshal(settings) | |
| if err != nil { | |
| return fmt.Errorf("invalid settings format: %w", err) | |
| } | |
| if len(data) > maxSettingsSize { | |
| return fmt.Errorf("settings exceed maximum size of %d bytes", maxSettingsSize) | |
| } | |
| return r.db.User.UpdateOneID(uid).SetSettings(settings).Exec(ctx) | |
| } |
🤖 Prompt for AI Agents
In backend/internal/data/repo/repo_users.go around lines 179 to 181, the
SetSettings method accepts unbounded JSON which can enable DoS via large
payloads; before calling UpdateOneID(...).SetSettings(...).Exec(ctx) serialize
the settings (e.g., json.Marshal) and check the byte length against a defined
max (suggest ~1MB or a configurable constant), return a clear validation error
if the size exceeds the limit, and only proceed to persist when within bounds;
ensure the size check runs early and that the error returned is descriptive and
uses existing error types/handling conventions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What type of PR is this?
What this PR does / why we need it:
Moves the user settings to the DB, so that they follow the user across devices and browsers (instead of the current local browser storage)
Which issue(s) this PR fixes:
I'm sure we have an issue for this, I just can't find it at the moment
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.