Skip to content

feat: add workspace branding service and endpoints - #377

Open
pikann wants to merge 3 commits into
masterfrom
feature/add-workspace-brading-service
Open

feat: add workspace branding service and endpoints#377
pikann wants to merge 3 commits into
masterfrom
feature/add-workspace-brading-service

Conversation

@pikann

@pikann pikann commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds an admin-only Workspace Branding settings page (/admin/settings) so a self-hosted instance can be re-skinned without a rebuild:

  • Logo & favicon — uploaded through the existing avatar pipeline (presigned upload, server-side resize).
  • Brand name — replaces "Paca" as the browser tab title and the wordmark shown next to the logo (sidebar, login page). The "OSS" badge on the login hero hides itself once a custom brand name is set.
  • Primary color — chosen from a curated preset palette (not a free-form picker) for light/dark mode, applied everywhere the accent color shows up: buttons, the sidebar's active nav item, focus rings, links, and the rich-text editor's selection highlight.

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 new settings.write permission (granted to the default ADMIN role). The fetched branding is cached in localStorage so 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

  • New singleton workspace_settings table (migration 000035) and settings domain/service/repository/handler, reusing attachmentdom.AvatarService for logo/favicon uploads instead of new image-handling code.
  • PATCH /admin/settings updates brand name + colors together; POST/DELETE .../logo|favicon/avatar/* handle uploads, mirroring the existing user/agent/project avatar endpoints.
  • New settings.write permission wired into internal/platform/authz.
  • Service + handler test coverage in 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>, and document.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 + localStorage cache for the branding query.
  • New strings translated across all 9 supported locales (i18n/locales/*/admin.json).

Test plan

  • go build ./..., go vet ./..., go test ./..., golangci-lint run all pass
  • tsc -b --noEmit and biome check pass
  • Manually verified against a running dev stack: uploaded a logo/favicon, switched primary color presets, set a brand name — confirmed propagation to the sidebar, login page (logged out), browser tab favicon/title, and that the "OSS" badge disappears once branded
  • Confirmed the localStorage branding cache paints immediately on reload and revalidates via a single background refetch
  • Confirmed the consolidated migration applies cleanly against an already-migrated dev database (schema/data verified via psql \d workspace_settings)

- 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.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 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: 0 plus 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 by getBranding is 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?

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread services/api/internal/service/settings/settings_service.go Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 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 in settings_repository.go, so concurrent logo/favicon/color writes can no longer discard each other's changes.
  • Removed Update from the settings Repository interface and routed every singleton mutation (CompleteImageUpload, RemoveImage, UpdateSettings) through WithLock, 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 whose WithLock holds a mutex across the callback to mirror the row lock.
  • Exported hexToRgb/rgbToHex/foregroundFor/darken from branding-effects.tsx and added branding-effects.test.ts unit coverage (assertions verified against the implementation).
  • Consolidated the sidebar's repeated inactive-nav-item class strings into NAV_ITEM_INACTIVE_CLASS, aligning two /60 stragglers with the shared /80 + dark variant.
  • Added router tests asserting settings.write gates PATCH /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.

Pullfrog  | Fix it ➔View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues in this revision — the UpdatedBy audit 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/RemoveImage now take an updatedBy parameter, record it on the singleton row (no-op removals intentionally leave UpdatedBy untouched), 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 — setBrandingQueryData in settings-api.ts patches both the React Query cache and the paca:branding-cache localStorage snapshot, and BrandingSettings.tsx routes its logo/favicon cache updates and the brand-save onSuccess through it. The new settings-api.test.ts suite 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/).

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant