Organization branding in the desktop editor (API + UI)#1783
Organization branding in the desktop editor (API + UI)#1783richiemcilroy merged 32 commits intomainfrom
Conversation
brin-security-scanner
Bot
added
the
contributor:verified
|
Paragon Review Skipped Hi @richiemcilroy! Your Polarity credit balance is insufficient to complete this review. Please visit https://app.paragon.run to finish your review. |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| throw new OrganizationBrandingValidationError("Invalid logo data"); | ||
| } | ||
|
|
||
| const buffer = Buffer.from(logo.data, "base64"); |
There was a problem hiding this comment.
Minor DoS hardening: Buffer.from will allocate based on the full base64 input. Since you already validate the charset, you can preflight size with Buffer.byteLength(..., "base64") and only decode if it’s within the limit.
| const buffer = Buffer.from(logo.data, "base64"); | |
| const decodedLength = Buffer.byteLength(logo.data, "base64"); | |
| if (decodedLength === 0) { | |
| throw new OrganizationBrandingValidationError("Logo file is empty"); | |
| } | |
| if (decodedLength > MAX_ORGANIZATION_LOGO_BYTES) { | |
| throw new OrganizationBrandingValidationError( | |
| "Logo file must be less than 1MB", | |
| ); | |
| } | |
| const buffer = Buffer.from(logo.data, "base64"); |
| } | ||
|
|
||
| const organization = DesktopOrganizationSchema.parse(response.body); | ||
| await commands.updateAuthPlan(); |
There was a problem hiding this comment.
If updateAuthPlan() fails (network hiccup, etc) after the PATCH succeeded, this currently throws and makes the UI look like the branding update failed. Might be nicer to return the updated org and treat the refresh as best-effort.
| await commands.updateAuthPlan(); | |
| const organization = DesktopOrganizationSchema.parse(response.body); | |
| try { | |
| await commands.updateAuthPlan(); | |
| const auth = (await authStore.get()) as CachedAuthStore | null; | |
| markOrganizationRefreshSuccess(auth?.user_id ?? null); | |
| } catch (error: unknown) { | |
| const auth = (await authStore.get()) as CachedAuthStore | null; | |
| markOrganizationRefreshFailure(auth?.user_id ?? null); | |
| console.error(error); | |
| } | |
| return organization; |
| export async function encodeFileAsBase64(file: File) { | ||
| const bytes = new Uint8Array(await file.arrayBuffer()); | ||
| const chunkSize = 0x8000; | ||
| let binary = ""; |
There was a problem hiding this comment.
binary += ... in a loop can get pretty slow (quadratic-ish) for larger files. Since logos can be ~1MB, pushing chunks into an array and joining is usually cheaper.
| let binary = ""; | |
| const chunkSize = 0x8000; | |
| const chunks: string[] = []; | |
| for (let index = 0; index < bytes.length; index += chunkSize) { | |
| chunks.push( | |
| String.fromCharCode(...bytes.subarray(index, index + chunkSize)), | |
| ); | |
| } | |
| return btoa(chunks.join("")); |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const organization = DesktopOrganizationSchema.parse(response.body); | ||
| await commands.updateAuthPlan(); | ||
| const auth = (await authStore.get()) as CachedAuthStore | null; | ||
| markOrganizationRefreshSuccess(auth?.user_id ?? null); | ||
| return organization; | ||
| } |
There was a problem hiding this comment.
markOrganizationRefreshSuccess is called immediately after commands.updateAuthPlan() writes to the Tauri store, but before any SolidJS authStore.createQuery() instance has called auth.refetch(). Because shouldRefreshOrganizations checks hasRecentOrganizationRefresh first, every active createDesktopOrganizationsQuery instance (one in OrganizationDropdown, another in ConfigSidebar) sees the module-level timestamp as "fresh" and skips its auto-refresh effect. The result: organizations() stays stale across both instances, so the brand-color swatches fed into the gradient editor, captions tab, keyboard tab, and text-segment config all continue showing the pre-save colors until the 45-minute TTL expires.
Compare with refresh(), which explicitly awaits auth.refetch() after commands.updateAuthPlan() before marking success. The same ordering is needed here — or the onSaved callback in OrganizationDropdown should call organizationSelection.refresh() instead of only persisting the selected ID.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/utils/organization-branding.ts
Line: 413-418
Comment:
**Stale brand colors after save**
`markOrganizationRefreshSuccess` is called immediately after `commands.updateAuthPlan()` writes to the Tauri store, but before any SolidJS `authStore.createQuery()` instance has called `auth.refetch()`. Because `shouldRefreshOrganizations` checks `hasRecentOrganizationRefresh` first, every active `createDesktopOrganizationsQuery` instance (one in `OrganizationDropdown`, another in `ConfigSidebar`) sees the module-level timestamp as "fresh" and skips its auto-refresh effect. The result: `organizations()` stays stale across both instances, so the brand-color swatches fed into the gradient editor, captions tab, keyboard tab, and text-segment config all continue showing the pre-save colors until the 45-minute TTL expires.
Compare with `refresh()`, which explicitly awaits `auth.refetch()` after `commands.updateAuthPlan()` before marking success. The same ordering is needed here — or the `onSaved` callback in `OrganizationDropdown` should call `organizationSelection.refresh()` instead of only persisting the selected ID.
How can I resolve this? If you propose a fix, please make it concise.| .from(organizations) | ||
| .leftJoin( | ||
| organizationMembers, | ||
| and( | ||
| eq(organizationMembers.organizationId, organizations.id), | ||
| eq(organizationMembers.userId, user.id), | ||
| ), |
There was a problem hiding this comment.
The metadata (brand colors) update is committed to the database before applyOrganizationLogoUpdate runs. If the logo upload or removal throws — storage error, network timeout, DB write failure — the response is a 500 but the color change is already persisted. The client receives an error and may retry the full operation, potentially applying the color update a second time or getting into a state where colors are updated but the logo action was never applied. Wrapping both operations in a single transaction (or reversing the order so the logo is applied first) would prevent this partial-write window.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/api/desktop/[...route]/root.ts
Line: 491-497
Comment:
**Non-atomic colors + logo update**
The metadata (brand colors) update is committed to the database before `applyOrganizationLogoUpdate` runs. If the logo upload or removal throws — storage error, network timeout, DB write failure — the response is a 500 but the color change is already persisted. The client receives an error and may retry the full operation, potentially applying the color update a second time or getting into a state where colors are updated but the logo action was never applied. Wrapping both operations in a single transaction (or reversing the order so the logo is applied first) would prevent this partial-write window.
How can I resolve this? If you propose a fix, please make it concise.| const ORGANIZATION_BRAND_COLOR_KEYS = [ | ||
| "primary", | ||
| "secondary", | ||
| "accent", | ||
| "background", | ||
| ] as const; | ||
|
|
||
| type OrganizationBrandColorKey = (typeof ORGANIZATION_BRAND_COLOR_KEYS)[number]; |
There was a problem hiding this comment.
ORGANIZATION_BRAND_COLOR_KEYS and OrganizationBrandColorKey are already exported from ~/utils/organization-branding but are re-declared locally here, creating a silent drift risk if one copy is updated.
| const ORGANIZATION_BRAND_COLOR_KEYS = [ | |
| "primary", | |
| "secondary", | |
| "accent", | |
| "background", | |
| ] as const; | |
| type OrganizationBrandColorKey = (typeof ORGANIZATION_BRAND_COLOR_KEYS)[number]; | |
| import { | |
| ORGANIZATION_BRAND_COLOR_KEYS, | |
| type OrganizationBrandColorKey, | |
| } from "~/utils/organization-branding"; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/routes/editor/OrganizationDropdown.tsx
Line: 46-53
Comment:
`ORGANIZATION_BRAND_COLOR_KEYS` and `OrganizationBrandColorKey` are already exported from `~/utils/organization-branding` but are re-declared locally here, creating a silent drift risk if one copy is updated.
```suggestion
import {
ORGANIZATION_BRAND_COLOR_KEYS,
type OrganizationBrandColorKey,
} from "~/utils/organization-branding";
```
How can I resolve this? If you propose a fix, please make it concise.
|
hey @greptileai please re-review the pr |
1 participant
This PR adds organization-level brand colors (and logo updates) end-to-end for the desktop app, backed by new authenticated desktop API routes on the web app.
Greptile Summary
This PR adds end-to-end organization branding (brand colors + logo) to the desktop editor. It introduces new authenticated API routes on the web side, a shared contract package for schemas, a desktop utility layer with reactive caching and a 45-minute TTL, and UI wiring that injects brand-color swatches into every color picker across the editor.
root.ts,organization-branding.ts): NewGET /organizationsandPATCH /organizations/:id/brandingendpoints backed by logo magic-byte validation, base64 decode, and metadata-merging helpers.organization-branding.ts): Module-level refresh deduplication,onKeyChange-driven reactive queries, and an optimistic cache write on successful PATCH confirmed by a fullupdateAuthPlancall.OrganizationDropdown,BrandColorsDropdown,ConfigSidebar, et al.): New org-selector and brand-settings dialog in the editor header; brand-color swatches propagated to all color pickers across the background, gradient, captions, keyboard, and text-segment tabs.Confidence Score: 5/5
Safe to merge — all findings are non-blocking quality observations.
Server-side auth and permission checks are correct, logo upload is validated with magic bytes and size limits, the reactive cache uses
onKeyChangeto keep all component instances in sync, and the Zod contract schemas are shared cleanly. All three findings are narrow and do not affect the correctness of the happy path.apps/web/app/api/desktop/[...route]/root.ts(membership filter on the PATCH query) andapps/desktop/src/utils/organization-branding.ts(failure-path flag logic inupdateOrganizationBranding).Security Review
root.tsPATCH endpoint): Any authenticated user can distinguish a live organization from a non-existent/tombstoned one by probing arbitrary UUIDs — 403 means active, 404 means absent. The GET/organizationsendpoint restricts to membership; the PATCH query does not. Impact is low given UUID IDs, but a membership filter before the auth check would close the gap.Important Files Changed
OrganizationBrandColorsstruct and extendsOrganizationwithrole,canEditBrand,iconUrl, andbrandColors; all new fields use#[serde(default)]so existing cached data deserialises safely.organizations_updated_at: Option<i32>stamped on every successful org fetch; theas i32cast mirrors existing code but will overflow in 2038./organizationsand PATCH/organizations/:id/branding; the PATCH query lacks a membership filter, enabling authenticated users to probe org existence via 404/403.updateOrganizationBrandingincorrectly triggersmarkOrganizationRefreshFailurewith a null userId whenupdateAuthPlanfails after a successful optimistic write.onKeyChangeensures selection changes propagate automatically across independent query instances.Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(web): apply organization logo upload..." | Re-trigger Greptile