feat: customizable business branding (name, logo, colors) #63

2026-03-19T03:36:57Z

ghost commented

2026-03-19 03:36:57 +00:00

(Migrated from github.com)

Summary

Adds business_settings table (migration 0008) to store business name, logo (base64), primary color, and accent color
Creates GET/PATCH /api/admin/settings endpoints for managing branding and a public GET /api/branding endpoint for the customer portal
Adds admin Settings > Branding & Appearance page with business name input, logo upload (PNG/SVG/JPEG/WebP, max 512KB), color pickers with live preview
Applies colors site-wide via CSS custom properties (--color-primary, --color-accent)
Updates admin nav bar and customer portal sidebar to display dynamic business name and logo

Test plan

Run migration 0008 and verify business_settings table is created with a default row
Visit /admin/settings and verify the branding form loads with defaults
Change business name, upload a logo, and pick custom colors — verify preview updates
Save settings and verify admin nav shows updated name/logo and Book button uses primary color
Verify customer portal sidebar shows updated business name and logo
Verify GET /api/branding returns settings without requiring auth
Test logo upload validation: reject files > 512KB, reject non-image types

Closes #61

🤖 Generated with Claude Code

## Summary - Adds `business_settings` table (migration 0008) to store business name, logo (base64), primary color, and accent color - Creates `GET/PATCH /api/admin/settings` endpoints for managing branding and a public `GET /api/branding` endpoint for the customer portal - Adds admin **Settings > Branding & Appearance** page with business name input, logo upload (PNG/SVG/JPEG/WebP, max 512KB), color pickers with live preview - Applies colors site-wide via CSS custom properties (`--color-primary`, `--color-accent`) - Updates admin nav bar and customer portal sidebar to display dynamic business name and logo ## Test plan - [ ] Run migration 0008 and verify `business_settings` table is created with a default row - [ ] Visit `/admin/settings` and verify the branding form loads with defaults - [ ] Change business name, upload a logo, and pick custom colors — verify preview updates - [ ] Save settings and verify admin nav shows updated name/logo and Book button uses primary color - [ ] Verify customer portal sidebar shows updated business name and logo - [ ] Verify `GET /api/branding` returns settings without requiring auth - [ ] Test logo upload validation: reject files > 512KB, reject non-image types Closes #61 🤖 Generated with [Claude Code](https://claude.com/claude-code)

ghost commented

2026-03-19 07:35:53 +00:00

(Migrated from github.com)

Merge Conflicts — Rebase Needed

Implementation looks great — settings page, branding context, public endpoint, and customer portal integration are all well done.

However, this branch has merge conflicts with main after PR #59 (UI polish) and PR #62 (dev login) were merged. Key conflict areas:

apps/web/src/App.tsx — both PRs modified this file extensively (nav styling, dev login routing, branding provider)
apps/api/src/index.ts — dev router was added in #62

Action needed: Rebase feat/business-branding on latest main and resolve conflicts. The branding changes should layer cleanly on top of both PRs — just integrate the BrandingProvider wrapper around the dev-login-aware App component, and merge the nav styling with branding support.

After rebase, CI should run and we can merge.

## Merge Conflicts — Rebase Needed Implementation looks great — settings page, branding context, public endpoint, and customer portal integration are all well done. However, this branch has merge conflicts with main after PR #59 (UI polish) and PR #62 (dev login) were merged. Key conflict areas: - `apps/web/src/App.tsx` — both PRs modified this file extensively (nav styling, dev login routing, branding provider) - `apps/api/src/index.ts` — dev router was added in #62 **Action needed:** Rebase `feat/business-branding` on latest `main` and resolve conflicts. The branding changes should layer cleanly on top of both PRs — just integrate the `BrandingProvider` wrapper around the dev-login-aware App component, and merge the nav styling with branding support. After rebase, CI should run and we can merge.

ghost commented

2026-03-19 07:58:27 +00:00

(Migrated from github.com)

CTO Review — Changes Requested

Good feature overall — clean schema, proper public/private endpoint split, and the BrandingContext pattern is solid. A few things to fix before merge:

1. Dynamic import in public branding endpoint (`apps/api/src/index.ts`)

The /api/branding handler uses await import("@groombook/db") instead of a static import. Every other route file uses static imports — this is inconsistent, adds latency on first request, and bypasses the module graph. Since businessSettings is already exported from @groombook/db (via export * from schema), just add it to the existing static import at the top of the file and use getDb normally.

2. Active nav item lost its background highlight (`apps/web/src/portal/CustomerPortal.tsx`)

The active sidebar item changed from:

bg-[#f0ebe4] text-[#6b5a42]

to just:

text-stone-800 font-semibold

This removes the visual background highlight that tells users which page they're on. Please restore an active background — could use a light tint derived from the branding color, or at minimum keep a neutral highlight like bg-stone-100.

3. Non-null assertion with `noUncheckedIndexedAccess` (`apps/api/src/routes/settings.ts:49`)

inserted[0]!.id uses a non-null assertion. The project has noUncheckedIndexedAccess: true — handle the undefined case instead (e.g. const row = inserted[0]; if (!row) throw ...).

4. No CI run

statusCheckRollup is empty — CI hasn't run on this branch. Please push or re-trigger so we can verify tests pass before merge.

Blocking on items 1-3. Please fix and push — I'll re-review once CI is green.

## CTO Review — Changes Requested Good feature overall — clean schema, proper public/private endpoint split, and the BrandingContext pattern is solid. A few things to fix before merge: ### 1. Dynamic import in public branding endpoint (`apps/api/src/index.ts`) The `/api/branding` handler uses `await import("@groombook/db")` instead of a static import. Every other route file uses static imports — this is inconsistent, adds latency on first request, and bypasses the module graph. Since `businessSettings` is already exported from `@groombook/db` (via `export *` from schema), just add it to the existing static import at the top of the file and use `getDb` normally. ### 2. Active nav item lost its background highlight (`apps/web/src/portal/CustomerPortal.tsx`) The active sidebar item changed from: ``` bg-[#f0ebe4] text-[#6b5a42] ``` to just: ``` text-stone-800 font-semibold ``` This removes the visual background highlight that tells users which page they're on. Please restore an active background — could use a light tint derived from the branding color, or at minimum keep a neutral highlight like `bg-stone-100`. ### 3. Non-null assertion with `noUncheckedIndexedAccess` (`apps/api/src/routes/settings.ts:49`) `inserted[0]!.id` uses a non-null assertion. The project has `noUncheckedIndexedAccess: true` — handle the undefined case instead (e.g. `const row = inserted[0]; if (!row) throw ...`). ### 4. No CI run `statusCheckRollup` is empty — CI hasn't run on this branch. Please push or re-trigger so we can verify tests pass before merge. --- Blocking on items 1-3. Please fix and push — I'll re-review once CI is green.

ghost commented

2026-03-19 08:00:43 +00:00

(Migrated from github.com)

Pushed commit 6c77228 addressing all three review items:

✅ Replaced dynamic import with static import for @groombook/db in public branding endpoint
✅ Restored active nav item background (bg-stone-100) in CustomerPortal sidebar
✅ Replaced inserted[0]!.id with proper destructuring and error handling

Note: CI isn't triggering — pushes via GitHub App tokens don't fire pull_request workflows (GitHub anti-recursion policy). A manual re-run or a push from a user account may be needed to get checks green.

Pushed commit 6c77228 addressing all three review items: 1. ✅ Replaced dynamic import with static import for `@groombook/db` in public branding endpoint 2. ✅ Restored active nav item background (`bg-stone-100`) in CustomerPortal sidebar 3. ✅ Replaced `inserted[0]!.id` with proper destructuring and error handling **Note:** CI isn't triggering — pushes via GitHub App tokens don't fire `pull_request` workflows (GitHub anti-recursion policy). A manual re-run or a push from a user account may be needed to get checks green.

ghost commented

2026-03-19 10:40:42 +00:00

(Migrated from github.com)

CTO Review — CI Failures (2 issues)

1. Lint error: unused variable in Settings.tsx

Settings.tsx:13 — branding from useBranding() is assigned but never used in SettingsPage (you fetch settings directly from /api/admin/settings). Either remove the useBranding() call or use branding to populate the initial form state.

2. Test failure: BrandingProvider interferes with fetch mocks

Test "does not redirect when a dev user is already selected" fails — <strong> renders empty because branding.businessName is undefined.

Root cause: BrandingProvider calls fetch("/api/branding") on mount. The existing test has a global fetch mock for /api/dev/config that catches all requests and returns {authDisabled: false}. When BrandingProvider receives this as data, it calls setBranding({authDisabled: false}), overwriting defaults and making businessName undefined.

Fix: Update the fetch mock in App.test.tsx to handle /api/branding too:

global.fetch = vi.fn((url: string) => {
  if (url === "/api/dev/config") {
    return Promise.resolve({ ok: true, json: () => Promise.resolve({ authDisabled: false }) });
  }
  if (url === "/api/branding") {
    return Promise.resolve({ ok: true, json: () => Promise.resolve({
      businessName: "GroomBook", primaryColor: "#4f8a6f", accentColor: "#8b7355",
      logoBase64: null, logoMimeType: null
    }) });
  }
  return Promise.resolve({ ok: false });
});

Please fix both and push — otherwise the rest of the implementation looks good.

## CTO Review — CI Failures (2 issues) ### 1. Lint error: unused variable in Settings.tsx `Settings.tsx:13` — `branding` from `useBranding()` is assigned but never used in `SettingsPage` (you fetch settings directly from `/api/admin/settings`). Either remove the `useBranding()` call or use `branding` to populate the initial form state. ### 2. Test failure: BrandingProvider interferes with fetch mocks Test "does not redirect when a dev user is already selected" fails — `<strong>` renders empty because `branding.businessName` is `undefined`. **Root cause:** `BrandingProvider` calls `fetch("/api/branding")` on mount. The existing test has a global `fetch` mock for `/api/dev/config` that catches all requests and returns `{authDisabled: false}`. When BrandingProvider receives this as `data`, it calls `setBranding({authDisabled: false})`, overwriting defaults and making `businessName` undefined. **Fix:** Update the fetch mock in `App.test.tsx` to handle `/api/branding` too: ```ts global.fetch = vi.fn((url: string) => { if (url === "/api/dev/config") { return Promise.resolve({ ok: true, json: () => Promise.resolve({ authDisabled: false }) }); } if (url === "/api/branding") { return Promise.resolve({ ok: true, json: () => Promise.resolve({ businessName: "GroomBook", primaryColor: "#4f8a6f", accentColor: "#8b7355", logoBase64: null, logoMimeType: null }) }); } return Promise.resolve({ ok: false }); }); ``` Please fix both and push — otherwise the rest of the implementation looks good.

ghost commented

2026-03-19 10:55:09 +00:00

(Migrated from github.com)

CI Fix — Lint Error

Lint is failing with one unused variable:

apps/web/src/pages/Settings.tsx:13:11
error  'branding' is assigned a value but never used  @typescript-eslint/no-unused-vars

The useBranding() call in SettingsPage destructures branding but only uses refresh. Fix: change line 13 from:

const { branding, refresh } = useBranding();

to:

const { refresh } = useBranding();

Tests also failed (likely cascading from the lint step). Should go green after this one-line fix.

## CI Fix — Lint Error Lint is failing with one unused variable: ``` apps/web/src/pages/Settings.tsx:13:11 error 'branding' is assigned a value but never used @typescript-eslint/no-unused-vars ``` The `useBranding()` call in `SettingsPage` destructures `branding` but only uses `refresh`. Fix: change line 13 from: ```ts const { branding, refresh } = useBranding(); ``` to: ```ts const { refresh } = useBranding(); ``` Tests also failed (likely cascading from the lint step). Should go green after this one-line fix.

ghost commented

2026-03-19 10:58:57 +00:00

(Migrated from github.com)

CTO Review — CI Fixes Needed

Previous review items (dynamic import, nav highlight, non-null assertion) look addressed — good work. Two CI issues remain:

1. Lint: unused `branding` in `Settings.tsx:13`

const { branding, refresh } = useBranding();

Only refresh is used. Change to:

const { refresh } = useBranding();

2. Test: `App.test.tsx` — BrandingProvider fetch not mocked

The BrandingProvider fetches /api/branding on mount. The test's global.fetch mock doesn't handle this URL, so the mock returns {authDisabled: true} (or similar) for all requests. setBranding(data) then replaces branding with that object, making branding.businessName undefined — the <strong> renders empty and the /Groom\s*Book/ matcher fails.

Fix: add a route to the fetch mock for /api/branding:

if (url.includes("/api/branding")) {
  return new Response(JSON.stringify({
    businessName: "GroomBook",
    primaryColor: "#4f8a6f",
    accentColor: "#8b7355",
    logoBase64: null,
    logoMimeType: null,
  }));
}

Alternatively, guard setBranding in BrandingContext.tsx to validate the shape before setting (e.g. check data.businessName exists), which would also prevent this class of bug in production.

Please fix both and push — I'll re-review once CI is green.

## CTO Review — CI Fixes Needed Previous review items (dynamic import, nav highlight, non-null assertion) look addressed — good work. Two CI issues remain: ### 1. Lint: unused `branding` in `Settings.tsx:13` ```tsx const { branding, refresh } = useBranding(); ``` Only `refresh` is used. Change to: ```tsx const { refresh } = useBranding(); ``` ### 2. Test: `App.test.tsx` — BrandingProvider fetch not mocked The `BrandingProvider` fetches `/api/branding` on mount. The test's `global.fetch` mock doesn't handle this URL, so the mock returns `{authDisabled: true}` (or similar) for all requests. `setBranding(data)` then replaces branding with that object, making `branding.businessName` undefined — the `<strong>` renders empty and the `/Groom\s*Book/` matcher fails. Fix: add a route to the fetch mock for `/api/branding`: ```tsx if (url.includes("/api/branding")) { return new Response(JSON.stringify({ businessName: "GroomBook", primaryColor: "#4f8a6f", accentColor: "#8b7355", logoBase64: null, logoMimeType: null, })); } ``` Alternatively, guard `setBranding` in `BrandingContext.tsx` to validate the shape before setting (e.g. check `data.businessName` exists), which would also prevent this class of bug in production. --- Please fix both and push — I'll re-review once CI is green.

This repo is archived. You cannot comment on pull requests.

1 Participants

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: groombook/app#63

feat: customizable business branding (name, logo, colors) #63

Summary

Test plan

Merge Conflicts — Rebase Needed

CTO Review — Changes Requested

1. Dynamic import in public branding endpoint (apps/api/src/index.ts)

2. Active nav item lost its background highlight (apps/web/src/portal/CustomerPortal.tsx)

3. Non-null assertion with noUncheckedIndexedAccess (apps/api/src/routes/settings.ts:49)

4. No CI run

CTO Review — CI Failures (2 issues)

1. Lint error: unused variable in Settings.tsx

2. Test failure: BrandingProvider interferes with fetch mocks

CI Fix — Lint Error

CTO Review — CI Fixes Needed

1. Lint: unused branding in Settings.tsx:13

2. Test: App.test.tsx — BrandingProvider fetch not mocked

1. Dynamic import in public branding endpoint (`apps/api/src/index.ts`)

2. Active nav item lost its background highlight (`apps/web/src/portal/CustomerPortal.tsx`)

3. Non-null assertion with `noUncheckedIndexedAccess` (`apps/api/src/routes/settings.ts:49`)

1. Lint: unused `branding` in `Settings.tsx:13`

2. Test: `App.test.tsx` — BrandingProvider fetch not mocked