Shadcn Theming & Styling [added the requirements as stated] #883#1315
Shadcn Theming & Styling [added the requirements as stated] #883#1315Zedoman wants to merge 2 commits intoMail-0:stagingfrom
Conversation
WalkthroughThis change implements a comprehensive theming system for the mail application. It introduces database tables, API endpoints, UI components, and settings pages for creating, editing, deleting, sharing, and applying custom themes. Users can manage colors, fonts, spacing, shadows, and more, set themes per connection, and browse or copy public themes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant API
participant DB
User->>UI: Opens Appearance/Theme Settings
UI->>API: Fetches user's themes (GET /api/v1/themes)
API->>DB: Query themes by userId
DB-->>API: Return themes
API-->>UI: Return themes list
User->>UI: Edits/creates theme (colors, fonts, etc.)
UI->>API: Save theme (POST/PATCH /api/v1/themes)
API->>DB: Insert/update theme record
DB-->>API: Confirm save
API-->>UI: Return saved theme
User->>UI: Sets theme public or copies from marketplace
UI->>API: Update/copy theme (PATCH/POST /api/v1/themes/copy)
API->>DB: Copy/modify theme record
DB-->>API: Confirm copy/update
API-->>UI: Return new/updated theme
User->>UI: Assigns theme to connection
UI->>API: PATCH /api/v1/connections/[id]/theme
API->>DB: Update connection's themeId
DB-->>API: Confirm update
API-->>UI: Return updated connection
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@MrgSub Sir please check it once |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
🔭 Outside diff range comments (2)
apps/mail/app/(routes)/settings/connections/page.tsx (1)
125-126: Straylg:variant produces an invalid Tailwind class
"lg: grid gap-4 …"containslg:with no class following it, generating a zero-width rule and polluting the bundle.- <div className="lg: grid gap-4 sm:grid-cols-1 md:grid-cols-2"> + <div className="grid gap-4 sm:grid-cols-1 md:grid-cols-2 lg:grid-cols-3">apps/mail/app/globals.css (1)
54-88:--background: 1;is an invalid HSL valueA single number cannot be parsed as an HSL triplet; this will resolve to black in some browsers and break dark-mode contrast. Provide full
h s lcomponents, e.g.,--background: 240 3.7% 10%;.
♻️ Duplicate comments (2)
packages/tailwind-config/tailwind.config.ts (1)
13-40: Schema mismatch –secondarycolours exist here but not in DBThe Tailwind palette introduces
secondary/secondary-foreground, yet thetheme.colorsjsonb schema omits them (see previous comment).
Ensure both layers agree to prevent blank CSS variables in rendered themes.apps/server/src/routes/v1/themes/[id]/route.ts (1)
96-122:backgroundsin payload has no matching columnUpdating with a field that the table doesn’t possess will throw
column "backgrounds" of relation "theme" does not exist.
Either add the column (see schema comment) or drop it from the API contract.
🧹 Nitpick comments (29)
apps/server/src/db/migrations/meta/_journal.json (1)
207-220: Confirm intentional use ofbreakpoints: truefor new migrationsBoth newly-added migration entries keep the
breakpointsflag set totrue.
With Drizzle this prevents the migrator from automatically running later migrations past these points unless the flag is cleared manually.If these migrations aren’t meant to serve as a manual stop-point in CI / prod, consider switching the flag back to
falseafter validating them, otherwise deploy automation may stall.apps/server/src/trpc/index.ts (1)
14-15: Router file naming is off-patternAll existing routers follow
<entity>.ts(connectionsRouter,labelsRouter, …). IntroducingthemeRouterfrom./routes/themeRouterbreaks the pattern and can confuse future contributors.-import { themeRouter } from './routes/themeRouter'; +import { themeRouter } from './routes/theme';Same rename applies to the file itself. Functionality is fine; this is purely for consistency.
tsconfig.json (1)
2-7: Path alias may collide with existingbaseUrlsettingsMapping
"@/*"to"./*"is convenient, but:
- It resolves to repo root, not the individual package path inside the monorepo.
- Other package-level tsconfigs may already declare their own
"@/*"or rely on npm/TS-path exports.Double-check that IDE jump-to-definition and
tsc -bstill resolve the correct file when two packages export e.g.@/utils/logger.apps/server/src/trpc/trpc.ts (1)
11-14: Type alias shadowing & naming
type Connection = …lands in the type namespace, so it won’t collide with the runtimeconnectiontable, but sharing the same identifier easily confuses maintainers. Prefer a clearer alias such asConnectionModel.-type Connection = InferSelectModel<typeof connection>; +type ConnectionModel = InferSelectModel<typeof connection>;apps/mail/components/ui/font-selector.tsx (2)
10-17: Consider loading the selected Google fonts automatically.
GOOGLE_FONTSis a hard-coded list; the component does not inject<link>tags or@importstatements for the chosen font. Unless the application already pre-loads these families, users may see fallback fonts or FOUC.
Either document the expectation or add font-loading logic (e.g. [next/font/google]).
19-37: Nit: make the dropdown accessible & predictable.
SelectTriggerhas noaria-label, so screen-readers will read an empty control until a value is chosen. Pass thelabelprop toSelectTriggerasaria-label(or useLabel’shtmlForpointing to the trigger’sid) to improve accessibility.apps/mail/app/(routes)/settings/themes/marketplace/page.tsx (3)
12-14: Remove leftover comment & unused import hint.Line 13 (
// Use the correct import for Link in your project) appears to be a scaffolding note and should be deleted to avoid confusion.
22-29: Typings OK, but translated strings are hard-coded.All user-visible copy (
"Theme Marketplace","Browse and use themes…", etc.) bypassesuseTranslations(). Replace witht('…')keys to keep localisation consistent.
42-46: Invalidate the marketplace query after copying.Only
['themes']is invalidated; the marketplace continues to show stale data until reload. Add:queryClient.invalidateQueries({ queryKey: ['themes'] }); + queryClient.invalidateQueries({ queryKey: ['public-themes'] });apps/server/src/routes/v1/connections/[id]/theme/route.ts (1)
29-44: SQL check logic looks good but note potential perf hit.
select … where id = ? and (userId = ? or isPublic = true)fetches the whole row just for existence. A simplecount()could be cheaper, but not blocking.apps/server/src/routes/v1/themes/default/route.ts (1)
8-10: Fail fast whenDATABASE_URLis missingPassing an empty string to
postgres()will crash at runtime.
Return 500 immediately to surface mis-configuration:-const connectionString = process.env.DATABASE_URL || ''; +const connectionString = process.env.DATABASE_URL; +if (!connectionString) { + throw new Error('DATABASE_URL not set'); +}apps/mail/types/theme.ts (1)
75-116: Constant reuse: id"default"may collide with real DB idsIf a user creates a theme with id
"default"you risk primary-key conflicts when serialisingdefaultTheme.
Consider using a sentinel like"__default"ornull.apps/server/src/routes/v1/themes/favorites/route.ts (1)
11-13: SameDATABASE_URLempty-string fallback issue as indefault/route.ts.
Propagate the guard clause here as well.apps/mail/app/(routes)/settings/themes/page.tsx (2)
38-40:previewThemestate is updated but never read – dead code that adds cognitive overhead.
Remove it or integrate it into a live preview.
100-119: Consider addingrole="list"/role="listitem"and keyboard affordances for accessibility, and extract the card into a separate component once edit / delete actions are added to keep this page lean.apps/mail/app/(routes)/settings/connections/page.tsx (1)
70-77: Use localized strings for toast messagesHard-coding English strings bypasses the existing i18n system (
useTranslations). Surface the messages throught('…')to keep UX consistent.- toast.success('Theme updated successfully'); + toast.success(t('pages.settings.connections.themeUpdateSuccess')); … - toast.error('Failed to update theme'); + toast.error(t('pages.settings.connections.themeUpdateError'));apps/server/src/routes/v1/themes/route.ts (1)
17-63: LocalThemeDatatype duplicates domain modelAs with the copy route, re-declaring the full schema increases maintenance cost. Import a shared type or pick required columns (e.g.,
Insert<typeof themeTable>from Drizzle).apps/mail/components/theme/theme-editor.tsx (2)
34-43:parsePixelValuesilently assumes1rem = 16pxThis hard-coded conversion may mismatch the user’s root font size. Either read
window.getComputedStyle(document.documentElement).fontSizeor disallowremaltogether.
96-103:onCheckedChangeignores indeterminate stateRadix
Checkboxcan emit'indeterminate'. Comparing with=== trueleaves the theme stuck if that value surfaces. Convert to boolean withchecked === 'indeterminate' ? false : !!checked.apps/server/src/db/schema.ts (1)
137-141: Potential multiple defaults – add a DB-level uniqueness guard
isDefaultis toggled in application code, but the database allows many rows per user withis_default = true.
Enforce with a partial unique index (WHERE user_id = ? AND is_default) to guarantee integrity even if the app logic regresses.packages/tailwind-config/tailwind.config.ts (1)
46-54: Overwriting core radii may surprise plugin authorsRedefining
lg,md,smto CSS variables changes the meaning of Tailwind’s built-ins globally. Consider namespaced keys (e.g.,theme-lg) to avoid third-party style regressions.apps/server/src/routes/v1/themes/[id]/route.ts (4)
8-10: New DB instance per module – pool exhaustion aheadCreating a connection with
createDb(connectionString)at module scope for every route file circumvents pooling and Hot-Reload can spawn many clients.
Reuse a singleton or Drizzle’s built-in pool manager.
59-75: Query condition can be simplified & indexed
or(eq(id,id), and(isPublic=true, id=id))collapses to justeq(id,id).
Instead, filtereq(theme.id, id)and rely on the post-fetch authorization check. This also allows the primary-key index to be used directly.
123-125: Early return only when all fields empty
Object.keys(updateData).length === 1assumesupdatedAtis always present but might be omitted if object spread changes. Safer: compute the diff set first, bail if it’s empty, then addupdatedAt.
90-94: Authenticate first, parse laterConsider validating the JWT before reading the request body – avoids wasting CPU / parsing JSON if the caller is unauthenticated.
apps/mail/app/(auth)/login/login-client.tsx (1)
116-120: Missing error / loading handling for the themes query
useQuerycan error (network, TRPC failure). Right now the UI silently breaks. At minimum:const { data: themes = [], isLoading, error } = useQuery<Theme[]>(…); if (error) return <p className="text-red-500">Failed to load themes</p>;A tiny fallback avoids blank dropdowns and hard-to-debug console traces.
apps/server/src/trpc/routes/themeRouter.ts (1)
106-126: Client-supplied primary key – risky and unnecessary
createexpects the caller to sendid. Collisions or malicious overwrites become trivial.Generate the ID server-side (
gen_random_uuid()in SQL orcrypto.randomUUID()in JS) and remove the field from the input schema.apps/server/src/db/migrations/meta/0029_snapshot.json (1)
396-402: Out-of-date snapshot kept for historic referenceNo action needed, but ensure CI only generates one authoritative snapshot to avoid confusion during future diffs.
apps/mail/app/(routes)/settings/appearance/page.tsx (1)
149-161: Remove noisyconsole.logstatements before shippingLarge-scale logging (e.g. theme lists, colors, settings) leaks implementation details to the browser console, bloats bundles, and can unintentionally surface sensitive user data. Strip or guard these logs behind a debug flag before production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (37)
apps/mail/app/(auth)/login/login-client.tsx(7 hunks)apps/mail/app/(routes)/layout.tsx(1 hunks)apps/mail/app/(routes)/settings/appearance/page.tsx(3 hunks)apps/mail/app/(routes)/settings/connections/page.tsx(5 hunks)apps/mail/app/(routes)/settings/themes/marketplace/page.tsx(1 hunks)apps/mail/app/(routes)/settings/themes/page.tsx(1 hunks)apps/mail/app/+types/theme.ts(1 hunks)apps/mail/app/globals.css(1 hunks)apps/mail/components/theme/theme-editor.tsx(1 hunks)apps/mail/components/ui/color-picker.tsx(1 hunks)apps/mail/components/ui/font-selector.tsx(1 hunks)apps/mail/components/ui/range-input.tsx(1 hunks)apps/mail/components/ui/slider.tsx(1 hunks)apps/mail/locales/en.json(2 hunks)apps/mail/package.json(3 hunks)apps/mail/types/theme.ts(1 hunks)apps/mail/vite.config.ts(2 hunks)apps/server/package.json(4 hunks)apps/server/src/db/migrations/0029_eminent_theme.sql(1 hunks)apps/server/src/db/migrations/0030_exotic_elektra.sql(1 hunks)apps/server/src/db/migrations/meta/0029_snapshot.json(1 hunks)apps/server/src/db/migrations/meta/0030_snapshot.json(1 hunks)apps/server/src/db/migrations/meta/_journal.json(1 hunks)apps/server/src/db/schema.ts(2 hunks)apps/server/src/lib/utils.ts(2 hunks)apps/server/src/routes/v1/connections/[id]/theme/route.ts(1 hunks)apps/server/src/routes/v1/themes/[id]/route.ts(1 hunks)apps/server/src/routes/v1/themes/copy/route.ts(1 hunks)apps/server/src/routes/v1/themes/default/route.ts(1 hunks)apps/server/src/routes/v1/themes/favorites/route.ts(1 hunks)apps/server/src/routes/v1/themes/route.ts(1 hunks)apps/server/src/trpc/index.ts(2 hunks)apps/server/src/trpc/routes/themeRouter.ts(1 hunks)apps/server/src/trpc/trpc.ts(1 hunks)apps/server/tsconfig.json(1 hunks)packages/tailwind-config/tailwind.config.ts(1 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (12)
apps/server/src/trpc/index.ts (1)
apps/server/src/trpc/routes/themeRouter.ts (1)
themeRouter(6-247)
apps/mail/components/ui/slider.tsx (1)
apps/mail/lib/utils.ts (1)
cn(51-51)
apps/mail/app/(routes)/settings/themes/marketplace/page.tsx (3)
apps/mail/types/theme.ts (1)
ServerTheme(47-73)apps/mail/providers/query-provider.tsx (1)
trpcClient(86-103)apps/server/src/db/schema.ts (1)
theme(87-141)
apps/server/src/trpc/trpc.ts (2)
apps/server/src/db/schema.ts (1)
connection(143-163)apps/server/src/ctx.ts (1)
HonoContext(15-15)
apps/server/src/routes/v1/connections/[id]/theme/route.ts (3)
apps/server/src/db/index.ts (1)
createDb(5-9)apps/server/src/lib/utils.ts (1)
getAuthenticatedUserId(339-343)apps/server/src/db/schema.ts (1)
theme(87-141)
apps/server/src/routes/v1/themes/default/route.ts (3)
apps/server/src/db/index.ts (1)
createDb(5-9)apps/server/src/lib/utils.ts (1)
getAuthenticatedUserId(339-343)apps/server/src/db/schema.ts (1)
theme(87-141)
apps/mail/app/+types/theme.ts (1)
apps/mail/types/theme.ts (1)
Theme(1-45)
apps/server/src/lib/utils.ts (2)
apps/server/src/lib/server-utils.ts (2)
getActiveConnection(7-35)connectionToDriver(37-50)apps/server/src/db/schema.ts (1)
connection(143-163)
apps/mail/types/theme.ts (1)
apps/mail/app/+types/theme.ts (1)
Theme(1-28)
apps/mail/app/(routes)/settings/themes/page.tsx (3)
apps/mail/types/theme.ts (2)
defaultTheme(75-116)Theme(1-45)apps/mail/app/+types/theme.ts (1)
Theme(1-28)apps/server/src/db/schema.ts (1)
theme(87-141)
apps/mail/components/theme/theme-editor.tsx (4)
apps/mail/types/theme.ts (1)
Theme(1-45)apps/mail/components/ui/color-picker.tsx (1)
ColorPicker(12-37)apps/mail/components/ui/font-selector.tsx (1)
FontSelector(19-37)apps/mail/components/ui/range-input.tsx (1)
RangeInput(13-33)
packages/tailwind-config/tailwind.config.ts (1)
apps/mail/types/theme.ts (1)
defaultTheme(75-116)
🔇 Additional comments (15)
apps/server/src/trpc/index.ts (1)
32-33: Nice addition – theme routes now exposed through TRPCNo functional issues spotted with the registration. 👍
apps/mail/app/(routes)/layout.tsx (1)
10-14: Indent-only change looks goodPure whitespace / indentation tweak – no functional impact.
apps/mail/package.json (1)
26-28: [web_search]What are the breaking changes in @tanstack/react-query v5 migration guide compared to v4, specifically any renamed hooks?apps/mail/locales/en.json (1)
59-70: New i18n keys – no issuesKeys are unique and follow existing naming conventions.
Also applies to: 273-275
apps/mail/components/ui/slider.tsx (1)
12-24: Component looks solid – minimal, typed, themable.Forward-ref wiring, Radix composition and Tailwind classes are correct and match the rest of the design-system. No issues spotted.
apps/server/src/routes/v1/connections/[id]/theme/route.ts (1)
2-10: Import paths are one level too deep – file will not compile.From
routes/v1/connections/[id]/, the correct relative path tolib/utils(anddb/*) is../../../../, not../../../../../.-import { getAuthenticatedUserId } from '../../../../../lib/utils'; +import { getAuthenticatedUserId } from '../../../../lib/utils'; ... -import { - connection as connectionTable, - theme as themeTable -} from '../../../../../db/schema'; +import { + connection as connectionTable, + theme as themeTable, +} from '../../../../db/schema'; ... -import { createDb } from '../../../../../db/index'; +import { createDb } from '../../../../db/index';Likely an incorrect or invalid review comment.
apps/mail/components/ui/range-input.tsx (1)
4-11: ```shell
#!/bin/bash
echo "===== apps/mail/components/ui/label.tsx ====="
sed -n '1,200p' apps/mail/components/ui/label.tsx
echo ""
echo "===== apps/mail/components/ui/slider.tsx ====="
sed -n '1,200p' apps/mail/components/ui/slider.tsx</details> <details> <summary>apps/mail/types/theme.ts (1)</summary> `1-45`: ```shell #!/bin/bash set -e echo "----- apps/mail/app/+types/theme.ts -----" sed -n '1,200p' apps/mail/app/+types/theme.ts || echo "File not found or no content." echo echo "----- apps/mail/types/theme.ts -----" sed -n '1,200p' apps/mail/types/theme.ts || echo "File not found or no content."apps/server/src/lib/utils.ts (1)
373-376:connectionToDriverstub will break consumersDown-stream code expects a fully-featured driver. Either implement or at least throw
NotImplementedto avoid silent undefined behaviour.apps/mail/app/(auth)/login/login-client.tsx (1)
109-120: UnusedselectedThemestate & no UI hookup
selectedThemeis stored but never rendered or sent to the backend. Either:
- Render a
<Select>fed bythemesso the user can actually pick a theme.- Drop the state until it is required.
Keeping dead state increases cognitive load.
apps/server/src/db/migrations/0029_eminent_theme.sql (1)
68-95:copy_theme()relies ongen_random_uuid()extensionEnsure
pgcrypto(oruuid-osspdepending on PG version) is enabled in earlier migrations:CREATE EXTENSION IF NOT EXISTS "pgcrypto";Without it,
gen_random_uuid()will raisefunction does not existon clean installs.apps/server/src/db/migrations/meta/0030_snapshot.json (1)
618-631: Good to seebackgroundscolumn removedThis aligns the snapshot with the current TypeScript model and avoids the NOT NULL mismatch flagged in migration 0029.
apps/mail/app/(routes)/settings/appearance/page.tsx (1)
389-397:crypto.randomUUID()not universally supportedOlder Safari/Firefox ESR lack this API. Consider a polyfill (
uuidnpm) or feature-detect before use to avoid runtime errors in unsupported browsers.apps/server/src/db/migrations/0030_exotic_elektra.sql (2)
82-83: New uniquephone_numberconstraint may fail existing dataIf any duplicate phone numbers already exist, this migration will error and halt deployment. Validate data or add a cleanup step (
DELETE / UPDATE) before applying theUNIQUEconstraint.Also applies to: 91-92
55-61:access_tokennullable change: confirm downstream code handlesNULLDropping NOT NULL on
mail0_connection.access_tokencan propagatenullto OAuth flows. Audit codepaths that assume a string to avoid NPEs.
| "compilerOptions": { | ||
| "types": ["next/types"], | ||
| "moduleResolution": "node", | ||
| "esModuleInterop": true | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
❓ Verification inconclusive
"types": ["next/types"] may shadow Node globals on the server
apps/server is a pure Node/Hono service. Importing Next.js ambient types pulls browser-centric globals (Window, fetch, etc.) which can mask missing Node typings or leak DOM types into server code.
If you only wanted the Headers/Request Web-API types, prefer "types": ["undici-types"] or add Node explicitly:
"types": ["next/types"],
+ "types": ["node", "undici-types"],Please verify that no accidental DOM globals are now referenced in server code.
"types": ["next/types"] may shadow Node globals on the server
apps/server is a pure Node/Hono service. Importing Next.js ambient types pulls browser-centric globals (Window, fetch, etc.) which can mask missing Node typings or leak DOM types into server code.
If you only wanted the Headers/Request Web-API types, prefer "types": ["undici-types"] or add Node explicitly:
"types": ["next/types"],
+ "types": ["node", "undici-types"],Please verify that no accidental DOM globals are now referenced in server code.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/server/tsconfig.json lines 3 to 7, the "types" field currently includes
"next/types", which introduces browser-specific globals that can conflict with
Node.js types in this pure server environment. Replace "next/types" with
"undici-types" or explicitly add Node types to avoid importing DOM globals like
Window or fetch. After updating, review the server code to ensure no DOM globals
are mistakenly used.
| optimizeDeps: { | ||
| include: ['novel', '@tiptap/extension-placeholder'], | ||
| }, | ||
| } | ||
| }, | ||
| define: { | ||
| __dirname: JSON.stringify('/'), // Fallback for __dirname | ||
| }, |
There was a problem hiding this comment.
Hard-coding __dirname to '/' can break path-based logic
Any third-party library (or your own code) that joins paths relative to __dirname will now resolve to the filesystem root, which almost never exists in the browser bundle and may produce incorrect URLs.
If you only need to silence Vite warnings when a lib references __dirname, it’s safer to stub it to '' or remove usages via alias:
- define: {
- __dirname: JSON.stringify('/'), // Fallback for __dirname
- },
+ define: {
+ __dirname: '""',
+ },Please re-test modules that read static assets with path.join(__dirname, '...').
📝 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.
| optimizeDeps: { | |
| include: ['novel', '@tiptap/extension-placeholder'], | |
| }, | |
| } | |
| }, | |
| define: { | |
| __dirname: JSON.stringify('/'), // Fallback for __dirname | |
| }, | |
| optimizeDeps: { | |
| include: ['novel', '@tiptap/extension-placeholder'], | |
| } | |
| }, | |
| define: { | |
| __dirname: '""', | |
| }, |
🤖 Prompt for AI Agents
In apps/mail/vite.config.ts around lines 44 to 50, the __dirname is hard-coded
to '/' which can cause incorrect path resolutions in browser bundles. To fix
this, change the __dirname definition to an empty string '' or remove it
entirely if possible, and verify that any modules using path.join(__dirname,
'...') still correctly resolve static asset paths after this change.
| activeConnection?: Connection; // Add activeConnection | ||
| } & HonoVariables; |
There was a problem hiding this comment.
activeConnection is optional but later assumed non-null
activeDriverProcedure passes ctx.activeConnection straight into connectionToDriver without a null-check. If getActiveConnection() returns null (e.g. user has no connections), this will throw at runtime.
Guard or make the field non-optional:
- activeConnection?: ConnectionModel;
+ activeConnection: ConnectionModel | null;and add an early failure inside activeDriverProcedure.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/server/src/trpc/trpc.ts around lines 18 to 19, the activeConnection
field is marked optional but is used later without null checks, risking runtime
errors if it is null. To fix this, either make activeConnection non-optional by
ensuring it is always set or keep it optional but add a null check in
activeDriverProcedure before passing it to connectionToDriver. If null, handle
the error early by throwing or returning a failure to prevent runtime
exceptions.
| "dev": "NODE_OPTIONS='--experimental-vm-modules' wrangler dev --show-interactive-dev-session=false --experimental-vectorize-bind-to-prod --env local", | ||
| "deploy": "wrangler deploy", | ||
| "postinstall": "patch-package", | ||
| "types": "wrangler types --env local", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
*NODE_OPTIONS= assignment is nix-only
Scripts like "dev": "NODE_OPTIONS='--experimental-vm-modules' wrangler dev …" won’t run on Windows shells. Use cross-env for portability:
-"dev": "NODE_OPTIONS='--experimental-vm-modules' wrangler dev …",
+"dev": "cross-env NODE_OPTIONS=--experimental-vm-modules wrangler dev …",You already depend on cross-env elsewhere, so reuse it here.
📝 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.
| "dev": "NODE_OPTIONS='--experimental-vm-modules' wrangler dev --show-interactive-dev-session=false --experimental-vectorize-bind-to-prod --env local", | |
| "deploy": "wrangler deploy", | |
| "postinstall": "patch-package", | |
| "types": "wrangler types --env local", | |
| "dev": "cross-env NODE_OPTIONS=--experimental-vm-modules wrangler dev --show-interactive-dev-session=false --experimental-vectorize-bind-to-prod --env local", | |
| "deploy": "wrangler deploy", | |
| "postinstall": "patch-package", | |
| "types": "wrangler types --env local", |
🤖 Prompt for AI Agents
In apps/server/package.json lines 6 to 9, the "dev" script sets NODE_OPTIONS
using a syntax that only works on Unix-like systems, causing issues on Windows.
Replace the direct assignment of NODE_OPTIONS with the cross-env package by
prefixing the command with "cross-env NODE_OPTIONS=--experimental-vm-modules" to
ensure cross-platform compatibility. This change leverages the existing
cross-env dependency for environment variable setting portability.
| export function ColorPicker({ label, value, onChange }: ColorPickerProps) { | ||
| const [color, setColor] = useState(value); | ||
|
|
||
| return ( | ||
| <div className="flex flex-col gap-2"> | ||
| <Label>{label}</Label> | ||
| <Popover> | ||
| <PopoverTrigger> | ||
| <div | ||
| className="w-10 h-10 rounded-md border" | ||
| style={{ backgroundColor: color }} | ||
| /> | ||
| </PopoverTrigger> | ||
| <PopoverContent className="w-auto p-0"> | ||
| <HexColorPicker | ||
| color={color} | ||
| onChange={(newColor) => { | ||
| setColor(newColor); | ||
| onChange(newColor); | ||
| }} | ||
| /> | ||
| </PopoverContent> |
There was a problem hiding this comment.
Internal colour state gets stale when parent prop changes.
useState(value) initialises once; subsequent prop updates aren’t reflected, so the swatch can display an outdated colour when the parent resets/undoes. Sync via useEffect.
-import { useState } from 'react';
+import { useState, useEffect } from 'react';
...
- const [color, setColor] = useState(value);
+ const [color, setColor] = useState(value);
+
+ // Keep internal state in sync with external value
+ useEffect(() => {
+ setColor(value);
+ }, [value]);📝 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.
| export function ColorPicker({ label, value, onChange }: ColorPickerProps) { | |
| const [color, setColor] = useState(value); | |
| return ( | |
| <div className="flex flex-col gap-2"> | |
| <Label>{label}</Label> | |
| <Popover> | |
| <PopoverTrigger> | |
| <div | |
| className="w-10 h-10 rounded-md border" | |
| style={{ backgroundColor: color }} | |
| /> | |
| </PopoverTrigger> | |
| <PopoverContent className="w-auto p-0"> | |
| <HexColorPicker | |
| color={color} | |
| onChange={(newColor) => { | |
| setColor(newColor); | |
| onChange(newColor); | |
| }} | |
| /> | |
| </PopoverContent> | |
| import { useState, useEffect } from 'react'; | |
| // ...other imports | |
| export function ColorPicker({ label, value, onChange }: ColorPickerProps) { | |
| const [color, setColor] = useState(value); | |
| // Keep internal state in sync with external value | |
| useEffect(() => { | |
| setColor(value); | |
| }, [value]); | |
| return ( | |
| <div className="flex flex-col gap-2"> | |
| <Label>{label}</Label> | |
| <Popover> | |
| <PopoverTrigger> | |
| <div | |
| className="w-10 h-10 rounded-md border" | |
| style={{ backgroundColor: color }} | |
| /> | |
| </PopoverTrigger> | |
| <PopoverContent className="w-auto p-0"> | |
| <HexColorPicker | |
| color={color} | |
| onChange={(newColor) => { | |
| setColor(newColor); | |
| onChange(newColor); | |
| }} | |
| /> | |
| </PopoverContent> | |
| </Popover> | |
| </div> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In apps/mail/components/ui/color-picker.tsx around lines 12 to 33, the internal
color state is initialized from the prop value only once and does not update
when the parent prop changes, causing the displayed color to become stale. Fix
this by adding a useEffect hook that listens for changes to the value prop and
updates the internal color state accordingly to keep them in sync.
| // Optionally, remove themeId from any connections using this theme | ||
| await ctx.c.var.db | ||
| .update(connection) | ||
| .set({ themeId: null }) | ||
| .where(eq(connection.themeId, id)) | ||
| .execute(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Deleting a theme nulls themeId for every connection globally
After deletion you update all connections, regardless of owner.
Limit the scope to the current user’s connections, or rely on the FK ON DELETE SET NULL already in place.
- .where(eq(connection.themeId, id))
+ .where(
+ and(
+ eq(connection.themeId, id),
+ eq(connection.userId, ctx.sessionUser.id)
+ )
+ )📝 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.
| // Optionally, remove themeId from any connections using this theme | |
| await ctx.c.var.db | |
| .update(connection) | |
| .set({ themeId: null }) | |
| .where(eq(connection.themeId, id)) | |
| .execute(); | |
| // Optionally, remove themeId from any connections using this theme | |
| await ctx.c.var.db | |
| .update(connection) | |
| .set({ themeId: null }) | |
| .where( | |
| and( | |
| eq(connection.themeId, id), | |
| eq(connection.userId, ctx.sessionUser.id) | |
| ) | |
| ) | |
| .execute(); |
🤖 Prompt for AI Agents
In apps/server/src/trpc/routes/themeRouter.ts around lines 238 to 244, the code
updates all connections globally by setting themeId to null when a theme is
deleted. To fix this, restrict the update query to only affect connections owned
by the current user by adding a condition that filters connections by the user's
ID, or alternatively remove this manual update entirely and rely on the foreign
key constraint with ON DELETE SET NULL to handle it automatically.
| "backgrounds" jsonb NOT NULL, | ||
| "is_public" boolean DEFAULT false NOT NULL, | ||
| "is_default" boolean DEFAULT false NOT NULL, | ||
| "created_at" timestamp DEFAULT now() NOT NULL, | ||
| "updated_at" timestamp DEFAULT now() NOT NULL |
There was a problem hiding this comment.
backgrounds column is NOT NULL but the API never sends it
mail0_theme.backgrounds jsonb NOT NULL,
The themeRouter.create() & update() payloads have no backgrounds field, which will cause INSERT / UPDATE to fail until the next migration drops the column.
Either:
- Keep the column and include it in the router contract, or
- Drop / make it nullable in this very migration to avoid a broken intermediate state.
🤖 Prompt for AI Agents
In apps/server/src/db/migrations/0029_eminent_theme.sql around lines 11 to 15,
the "backgrounds" column is defined as NOT NULL but the API does not send this
field in create or update payloads, causing insert or update failures. To fix
this, either modify the migration to make the "backgrounds" column nullable or
remove it entirely in this migration, or update the API router contracts to
include and handle the "backgrounds" field consistently.
| useEffect(() => { | ||
| if (selectedTheme?.colors.primary && selectedTheme.colors.primary !== primaryColor) { | ||
| console.log("Selected theme primary color changed:", selectedTheme.colors.primary); | ||
| } | ||
| }, [selectedTheme, form, primaryColor]); | ||
|
|
||
| useEffect(() => { | ||
| const currentTheme = form.getValues("colorTheme"); | ||
| console.log("Current Selected Theme:", currentTheme); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
form.watch("colorTheme") in deps re-triggers effect every render
form.watch() returns a value on each call, so the dependency array changes every render, causing the heavy DOM-mutation effect to run continuously.
Cache with useWatch or read from state once:
const watchedTheme = useWatch({ control: form.control, name: "colorTheme" });
...
}, [watchedTheme, primaryColor, selectedTheme]);Also applies to: 220-221
🤖 Prompt for AI Agents
In apps/mail/app/(routes)/settings/appearance/page.tsx around lines 169 to 178,
the useEffect dependency array includes form.watch("colorTheme"), which returns
a new value on every render causing the effect to run continuously. Replace
form.watch("colorTheme") with a cached value using useWatch hook from
react-hook-form, e.g., const watchedTheme = useWatch({ control: form.control,
name: "colorTheme" }), and update the dependency array to use watchedTheme
instead. Apply the same fix around lines 220-221 where form.watch is used
similarly.
| // Removed secondary color application | ||
|
|
||
| document.documentElement.style.setProperty("--primary-color", primaryColor, "important"); | ||
| document.documentElement.style.setProperty("background-color", primaryColor, "important"); | ||
| document.documentElement.style.setProperty("--background", primaryColor, "important"); | ||
| document.body.style.backgroundColor = primaryColor; | ||
|
|
||
| const bodyFont = selectedTheme.fonts.body || (data?.settings as Settings)?.bodyFont || "Inter"; | ||
| const headingFont = selectedTheme.fonts.heading || (data?.settings as Settings)?.headingFont || "Inter"; | ||
| const baseSpacing = selectedTheme.spacing.default || "1rem"; | ||
| const cornerRadius = selectedTheme.radius.default || "0.5rem"; | ||
|
|
||
| console.log("Applying Body Font:", bodyFont); | ||
| console.log("Applying Heading Font:", headingFont); | ||
| console.log("Applying Base Spacing:", baseSpacing); | ||
| console.log("Applying Corner Radius:", cornerRadius); | ||
|
|
||
| document.documentElement.style.setProperty("--font-sans", `"${bodyFont}", var(--font-geist-sans)`, "important"); | ||
| document.documentElement.style.setProperty("--font-heading", `"${headingFont}", var(--font-geist-sans)`, "important"); | ||
| document.documentElement.style.setProperty("--spacing-base", baseSpacing, "important"); | ||
| document.documentElement.style.setProperty("--radius-base", cornerRadius, "important"); | ||
|
|
||
| document.documentElement.classList.remove("light", "dark"); | ||
| document.documentElement.setAttribute("data-theme", "custom"); | ||
| document.documentElement.style.color = "white"; | ||
| } else { |
There was a problem hiding this comment.
Hard-coding global colours risks unreadable UI & fails WCAG
Bluntly forcing background-color, --background, and global color:white for every custom theme ignores contrast requirements and overrides downstream components. A dark primary colour will render white text on a dark background unreadable.
Prefer limiting overrides to CSS variables only:
- document.documentElement.style.setProperty("background-color", primaryColor, "important");
- document.documentElement.style.color = "white";
+ document.documentElement.style.setProperty("--background", primaryColor);
+ // leave text colour derived from theme tokens, not forcedAdd automated a11y checks for custom themes.
📝 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.
| // Removed secondary color application | |
| document.documentElement.style.setProperty("--primary-color", primaryColor, "important"); | |
| document.documentElement.style.setProperty("background-color", primaryColor, "important"); | |
| document.documentElement.style.setProperty("--background", primaryColor, "important"); | |
| document.body.style.backgroundColor = primaryColor; | |
| const bodyFont = selectedTheme.fonts.body || (data?.settings as Settings)?.bodyFont || "Inter"; | |
| const headingFont = selectedTheme.fonts.heading || (data?.settings as Settings)?.headingFont || "Inter"; | |
| const baseSpacing = selectedTheme.spacing.default || "1rem"; | |
| const cornerRadius = selectedTheme.radius.default || "0.5rem"; | |
| console.log("Applying Body Font:", bodyFont); | |
| console.log("Applying Heading Font:", headingFont); | |
| console.log("Applying Base Spacing:", baseSpacing); | |
| console.log("Applying Corner Radius:", cornerRadius); | |
| document.documentElement.style.setProperty("--font-sans", `"${bodyFont}", var(--font-geist-sans)`, "important"); | |
| document.documentElement.style.setProperty("--font-heading", `"${headingFont}", var(--font-geist-sans)`, "important"); | |
| document.documentElement.style.setProperty("--spacing-base", baseSpacing, "important"); | |
| document.documentElement.style.setProperty("--radius-base", cornerRadius, "important"); | |
| document.documentElement.classList.remove("light", "dark"); | |
| document.documentElement.setAttribute("data-theme", "custom"); | |
| document.documentElement.style.color = "white"; | |
| } else { | |
| // Removed secondary color application | |
| document.documentElement.style.setProperty("--primary-color", primaryColor, "important"); | |
| document.documentElement.style.setProperty("--background", primaryColor); | |
| document.documentElement.style.setProperty("--background", primaryColor, "important"); | |
| document.body.style.backgroundColor = primaryColor; | |
| const bodyFont = selectedTheme.fonts.body || (data?.settings as Settings)?.bodyFont || "Inter"; | |
| const headingFont = selectedTheme.fonts.heading || (data?.settings as Settings)?.headingFont || "Inter"; | |
| const baseSpacing = selectedTheme.spacing.default || "1rem"; | |
| const cornerRadius = selectedTheme.radius.default || "0.5rem"; | |
| console.log("Applying Body Font:", bodyFont); | |
| console.log("Applying Heading Font:", headingFont); | |
| console.log("Applying Base Spacing:", baseSpacing); | |
| console.log("Applying Corner Radius:", cornerRadius); | |
| document.documentElement.style.setProperty("--font-sans", `"${bodyFont}", var(--font-geist-sans)`, "important"); | |
| document.documentElement.style.setProperty("--font-heading", `"${headingFont}", var(--font-geist-sans)`, "important"); | |
| document.documentElement.style.setProperty("--spacing-base", baseSpacing, "important"); | |
| document.documentElement.style.setProperty("--radius-base", cornerRadius, "important"); | |
| document.documentElement.classList.remove("light", "dark"); | |
| document.documentElement.setAttribute("data-theme", "custom"); | |
| // leave text colour derived from theme tokens, not forced | |
| } else { |
🤖 Prompt for AI Agents
In apps/mail/app/(routes)/settings/appearance/page.tsx between lines 181 and
206, the code directly sets global background-color and text color styles which
can cause accessibility issues by ignoring contrast requirements. To fix this,
remove the direct assignments to document.body.style.backgroundColor and
document.documentElement.style.color, and instead only set CSS custom properties
(variables) for colors. This allows downstream components to handle colors
appropriately and maintain readability. Additionally, consider adding automated
accessibility checks for custom themes to ensure compliance.
| CREATE TABLE "mail0_jwks" ( | ||
| "id" text PRIMARY KEY NOT NULL, | ||
| "public_key" text NOT NULL, | ||
| "private_key" text NOT NULL, | ||
| "created_at" timestamp NOT NULL | ||
| ); |
There was a problem hiding this comment.
Storing raw private keys in plain text is a security liability
mail0_jwks.private_key is text NOT NULL without encryption or at-rest protection. Compromise of the DB exposes all signing keys.
Encrypt the column (e.g., pgcrypto), move keys to a dedicated KMS, or store only key identifiers and fetch secrets from a vault.
🤖 Prompt for AI Agents
In apps/server/src/db/migrations/0030_exotic_elektra.sql lines 1 to 6, the
private_key column is stored as plain text, which is a security risk. To fix
this, do not store raw private keys directly in the database; instead, either
encrypt the private_key column using a method like pgcrypto, or better, remove
the private_key column and store only key identifiers in the table while
fetching the actual private keys securely from a dedicated key management
service or vault at runtime.
Please resolve CodeRabbit comments :) |
|
|
||
| type TrpcContext = { | ||
| c: Context<HonoContext>; | ||
| activeConnection?: Connection; // Add activeConnection |
There was a problem hiding this comment.
We don't need activeConnection in context, can you use getActiveConnection ?
| shortcut: shortcutRouter, | ||
| settings: settingsRouter, | ||
| user: userRouter, | ||
| voice: voiceRouter, |
| }; | ||
|
|
||
| // GET /api/v1/themes - List themes (user's, public, or favorites) | ||
| export async function GET(req: NextRequest) { |
There was a problem hiding this comment.
we don't use this pattern, please follow the pattern existing with trpc
|
|
||
| // Initialize db connection | ||
| const connectionString = process.env.DATABASE_URL || ''; | ||
| const db = createDb(connectionString); |
There was a problem hiding this comment.
deprecated, we don't use process.env anymore
| themeId, | ||
| createdAt: new Date() | ||
| }); | ||
| return NextResponse.json({ success: true }); |
| @@ -0,0 +1,61 @@ | |||
| export const runtime = 'nodejs'; | |||
We don't use Next anymore, most of your work is using next patterns and libraries... |
|
Ok sir doing the changes. |
2 participants
READ CAREFULLY THEN REMOVE
/claim #883
Remove bullet points that are not relevant.
PLEASE REFRAIN FROM USING AI TO WRITE YOUR CODE AND PR DESCRIPTION. IF YOU DO USE AI TO WRITE YOUR CODE PLEASE PROVIDE A DESCRIPTION AND REVIEW IT CAREFULLY. MAKE SURE YOU UNDERSTAND THE CODE YOU ARE SUBMITTING USING AI.
Resolves: #883
Shadcn Theming & Styling — Complete Implementation
Description
This PR enhances the theming functionality of the application by implementing the remaining requirements for the "Shadcn Theming & Styling" feature. It introduces the ability for users to mark themes as public and display them in a new Theme Marketplace page, where users can copy public themes to their own collection. Additionally, it resolves a TypeScript error in the Connections page related to the useTRPC hook, ensuring proper typing of the trpc client, and adds a theme selection dropdown to the Connections page for managing connection-specific themes. UI/UX improvements were also made to the Appearance page to enhance the visual experience while maintaining its existing functionality. Also the created theme will get stored in the db.
Features Added
Mark Themes as Public:
Added a "Make Theme Public" toggle in the ThemeEditor component on the Appearance page, allowing users to mark themes as public.
Public themes are visible in the Theme Marketplace for other users to discover and copy.
Theme Marketplace Page:
Created a new ThemeMarketplace.tsx page to list all public themes.
Each public theme is displayed in a card with a preview of its primary color, the creator’s ID, and a "Copy Theme" button.
Users can copy a public theme to their own collection, creating a new theme with the name (Copy).
Theme Selection Dropdown in Connections Page:
Added a theme selection dropdown to the Connections page, allowing users to assign a specific theme to each email connection.
The dropdown includes a "Default" option to reset the theme and a list of the user’s themes with a preview of the primary color.
Indicates the currently selected theme with a "Current" badge.
TypeScript Fix for useTRPC:
Resolved a TypeScript error ("Type 'never' has no call signatures") in the Connections page by properly typing the trpc client with the AppRouter type.
Ensured the useTRPC hook provides a typed trpc client, fixing the issue with trpc.theme.getThemes.query.
UI/UX Improvements:
Enhanced the Appearance page with animations (using framer-motion) for adding and removing themes.
Added a preview of the primary color next to each theme’s name in the "Custom Themes" section.
Applied a gradient style to the "Create Theme" button and added hover effects to theme cards for a more engaging experience.
Type of Change
Areas Affected
Testing Done
Describe the tests you've done:
Mark Theme as Public:
Toggled the "Make Theme Public" switch in the Appearance page and verified the isPublic field updates in the database.
Confirmed the theme appears in the Theme Marketplace.
Theme Marketplace:
Navigated to the Theme Marketplace page and verified public themes are listed.
Copied a public theme and confirmed a new theme is created with the correct name and isPublic: false.
Theme Selection in Connections Page:
Assigned a theme to a connection and verified the theme applies (via the /api/v1/connections/[id]/theme endpoint).
Reset the theme to default and confirmed the themeId is set to null.
Verified loading and empty states in the theme dropdown.
Appearance Page Enhancements:
Created and deleted themes to confirm animations work.
Verified the primary color preview and hover effects.
Security Considerations
For changes involving data or authentication:
Checklist
Screenshots/Recordings
Screen.Recording.2025-06-17.at.00.23.14.mp4
By submitting this pull request, I confirm that my contribution is made under the terms of the project's license.
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores
Database