feat: add workspace branding service and endpoints - #377
Conversation
- Implemented settings service for managing workspace branding, including logo and favicon uploads, brand name, and primary colors. - Created HTTP handler for branding endpoints, supporting both public and admin functionalities. - Added DTOs for branding responses and update requests. - Developed tests for the settings service and HTTP handler to ensure functionality and error handling. - Created SQL migration to add workspace_settings table for storing branding information.
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Reviewed changes
Single-commit PR adding instance-wide workspace branding: a singleton workspace_settings row (migration 000035), a new settings domain (entity / repo / service / DTOs / handler), a public GET /api/v1/branding read plus admin PATCH /settings and logo/favicon upload endpoints gated by a new settings.write permission (seeded onto ADMIN, wildcard-covered for SUPER_ADMIN, reconciled for existing installs on startup), and a frontend that applies the brand via BrandingEffects (CSS variables, favicon, <title>), swaps the login/sidebar wordmarks and logos, adds an admin settings page, and translates all copy across the 9 locales. Image handling correctly reuses the existing avatar service (size caps, decode-dimension bounds, owner-kind key-prefix verification), and both service and handler tests are meaningful rather than theatre.
ℹ️ Nitpicks
- Logo/favicon URLs are the avatar service's 1-hour presigned GET URLs.
staleTime: 0plus refetch-on-mount/focus keeps them fresh across normal navigation, but a tab left idle in the background (most likely the pre-login page, which has no auth to push a refresh) will show a broken logo/favicon for the remainder of the hour after expiry. Consistent with how avatars behave across the app, so not blocking — just worth being aware of for a login page that users often leave open. - After a logo/favicon upload, only the React Query cache is updated (
updateImageCache); the localStorage branding cache written bygetBrandingis not, so a hard refresh paints the stale cached logo for one round-trip before the refetch lands. Transient and cosmetic.
Technical details
# Presigned branding URL expiry in idle sessions
## Affected sites
- apps/web/src/components/app-shell/branding-effects.tsx — `faviconUrl` effect only
re-runs when the derived URL string changes; an idle tab never re-resolves it.
- apps/web/src/hooks/use-branding.ts / lib/settings-api.ts — `staleTime: 0` relies on
mount/focus to refetch; no timer re-resolves URLs mid-session.
- services/api/internal/service/attachment/avatar_service.go:35 — `avatarURLTTL = 1h`.
## Required outcome
- Ensure a long-idle branded page does not display a broken asset after the presigned
URL expires (optional; matches existing avatar behavior app-wide, so acceptable as-is).
## Open questions for the human
- Is a hard 1h ceiling acceptable for an instance-wide asset shown on the login page?DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
… concurrent updates
There was a problem hiding this comment.
ℹ️ No new issues in this revision — the lock-based concurrency fix is well executed and its regression test is genuine.
Reviewed changes
- Replaced the unlocked read-modify-write path with a
WithLock(SELECT ... FOR UPDATE) row lock insettings_repository.go, so concurrent logo/favicon/color writes can no longer discard each other's changes. - Removed
Updatefrom the settingsRepositoryinterface and routed every singleton mutation (CompleteImageUpload,RemoveImage,UpdateSettings) throughWithLock, holding the transaction only for the DB read/write and ordering storage deletion after commit — readers never observe a committed row pointing at a deleted object. - Added
TestWithLock_SerializesConcurrentCallers(100 rounds of concurrent logo upload + settings update, asserting both writes survive) plus a fake repo whoseWithLockholds a mutex across the callback to mirror the row lock. - Exported
hexToRgb/rgbToHex/foregroundFor/darkenfrombranding-effects.tsxand addedbranding-effects.test.tsunit coverage (assertions verified against the implementation). - Consolidated the sidebar's repeated inactive-nav-item class strings into
NAV_ITEM_INACTIVE_CLASS, aligning two/60stragglers with the shared/80+ dark variant. - Added router tests asserting
settings.writegatesPATCH /admin/settings(403 without, 200 with).
The locking design is sound: the lock never spans object-store I/O, the fake-repo contract structurally enforces WithLock usage (the old Update no longer exists), and the old Get+Update code could actually leak the intermediate winner's newly-uploaded objects — now cleaned up correctly by the serialized caller.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
…oads and removals
There was a problem hiding this comment.
✅ No new issues in this revision — the
UpdatedByaudit gap and the localStorage cache-drift nitpick flagged in review are both addressed cleanly, with genuine coverage.
Reviewed changes
Since the prior pullfrog review (f0ecaea0), this revision adds commit 66269f9, which:
- Threaded the acting user through image mutation —
CompleteImageUpload/RemoveImagenow take anupdatedByparameter, record it on the singleton row (no-op removals intentionally leaveUpdatedByuntouched), and the complete-upload/delete handlers forward the claims subject. New service assertions and handler tests (TestCompleteLogoUpload_PassesActingUserIDToService,TestDeleteFavicon_PassesActingUserIDToService) verify the user ID actually reaches the service rather than being dropped. - Co-synced the frontend branding caches —
setBrandingQueryDatainsettings-api.tspatches both the React Query cache and thepaca:branding-cachelocalStorage snapshot, andBrandingSettings.tsxroutes its logo/favicon cache updates and the brand-saveonSuccessthrough it. The newsettings-api.test.tssuite covers the fetch-cache write, patch propagation across both caches, and the no-cached-value/no-write path.
The updatedBy pointer capture is safe (the closure is invoked synchronously inside WithLock and consumed by updateRow before that call returns), and all settings Go tests pass (go test ./internal/service/settings/ ./internal/transport/http/handler/).
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Summary
Adds an admin-only Workspace Branding settings page (
/admin/settings) so a self-hosted instance can be re-skinned without a rebuild:Branding is public (
GET /branding, unauthenticated) since it has to render before login and on every page load; only the admin write endpoints require the newsettings.writepermission (granted to the defaultADMINrole). The fetched branding is cached inlocalStorageso a hard reload paints instantly from the last-known value instead of flashing back to defaults, then revalidates in the background right after mount.Changes
Backend
workspace_settingstable (migration000035) andsettingsdomain/service/repository/handler, reusingattachmentdom.AvatarServicefor logo/favicon uploads instead of new image-handling code.PATCH /admin/settingsupdates brand name + colors together;POST/DELETE .../logo|favicon/avatar/*handle uploads, mirroring the existing user/agent/project avatar endpoints.settings.writepermission wired intointernal/platform/authz.settings_service_test.go/settings_handler_test.go.Frontend
components/admin/settings/BrandingSettings.tsx— the admin form (logo/favicon upload, brand name field, color preset picker).components/app-shell/branding-effects.tsx— applies fetched branding to CSS variables, the favicon<link>, anddocument.title; mounted at the route root so it also applies on the pre-login page.app-sidebar.tsx,BrandPanel.tsx,LoginFormPanel.tsx— consume the brand name/logo; sidebar nav item text switched from tracking the primary color to a neutral grey.lib/settings-api.ts— API client +localStoragecache for the branding query.i18n/locales/*/admin.json).Test plan
go build ./...,go vet ./...,go test ./...,golangci-lint runall passtsc -b --noEmitandbiome checkpasslocalStoragebranding cache paints immediately on reload and revalidates via a single background refetchpsql \d workspace_settings)