feat: email config features in theme ui#4698
Conversation
…center into theme-update-page
…center into theme-update-page
…itch-control-center into theme-update-page
|
Review changes with |
WalkthroughThis PR adds email configuration support to the theme system, introducing email asset management (logo), email preview rendering, tabbed theme creation/update interfaces, and refactored asset upload flows. Theme updates now support deletion and require org/merchant/profile validation context. New utility functions handle asset file selection/removal and conditional theme data body construction. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Form as Theme Update Form
participant Assets as Asset Processor<br/>(useProcessAssets)
participant API as Theme API
participant CDN as CDN/Asset Storage
participant UI as Theme Screen
User->>Form: Select theme & configure<br/>assets (logo, favicon, email logo)
User->>Form: Submit form
Form->>Assets: Process assets dict<br/>(~themeId)
alt Asset is String URL
Assets->>Assets: Store URL in urlsDict
else Asset is File
Assets->>API: POST upload asset<br/>(FormData with file)
API->>CDN: Store asset
CDN-->>API: Return storage path
Assets->>Assets: Build CDN URL<br/>from baseUrl/themeId/asset
Assets->>Assets: Store URL in urlsDict
end
Assets-->>Form: Return (urlsDict, emailLogoUrl)
Form->>API: Fetch current theme<br/>(internalSwitch validation)
API-->>Form: Return theme_data.settings
Form->>Form: Build request body<br/>(buildThemeDataBody)
Form->>API: PUT /theme<br/>(settings, urls, email_config)
API-->>UI: Success/Error response
UI->>User: Show toast & navigate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
src/Themes/ThemeLanding.res (1)
30-30: HardentoOptionagainst empty identifiers.
Right now only sentinel values are dropped; empty strings can still pass through asSome(""). Treat empty/whitespace IDs asNonetoo.Suggested update
-let toOption = (val, sentinel) => val->Option.flatMap(v => v == sentinel ? None : Some(v)) +let toOption = (val, sentinel) => + val->Option.flatMap(v => { + let trimmed = v->String.trim + trimmed == "" || trimmed == sentinel ? None : Some(trimmed) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Themes/ThemeLanding.res` at line 30, The toOption helper currently only filters out the sentinel but allows empty or whitespace strings; update toOption (val->Option.flatMap(...)) to also treat values that are empty or all-whitespace as None by trimming the string and returning None when the trimmed length is 0 (otherwise return Some of the original/trimmed value), so both sentinel and empty/whitespace identifiers are dropped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/RawFileInput.res`:
- Around line 11-16: The file input is currently made inaccessible by using
hidden=true and a plain label; remove the hidden attribute on the input
referenced by inputId and instead make the file control visually hidden but
still focusable (use a CSS "sr-only"/visually-hidden class that does not use
display:none or the hidden attribute), keep the existing onChange handler, add
an accessible name (aria-label) to the input, and make the visible trigger
keyboard-focusable—replace the plain label trigger with a real focusable button
(or keep the label but give it role="button" and onKeyDown that calls
document.getElementById(inputId).click()) so keyboard users can open the picker;
ensure buttonText, inputId, and onChange are preserved in RawFileInput.res.
In `@src/Themes/ThemeFeatureUtils.res`:
- Around line 45-61: buildThemeDataBody currently builds only "theme_data" and
optional "email_config", so theme renames are dropped; change its signature to
accept an extra ~themeName:string (callers should pass
valuesDict->getString("theme_name", "")) and add ("theme_name",
JSON.Encode.string(themeName)) to the top-level bodyEntries before returning
bodyEntries->getJsonFromArrayOfJson so the PUT payload includes the theme_name;
update ThemeUpdate.res call sites to pass valuesDict->getString("theme_name",
"").
In `@src/Themes/ThemeHelper.res`:
- Around line 516-539: The PUT currently overwrites existing theme_data.urls and
email_config because we only send urlsDict from processAssets; fetch the
currentThemeData (already loaded into currentThemeData) and extract existingUrls
=
currentThemeData->getDictFromJsonObject->getDictfromDict("theme_data")->getDictfromDict("urls")
and existingEmailConfig =
currentThemeData->getDictFromJsonObject->getDictfromDict("theme_data")->getDictfromDict("email_config")
(or similar keys used elsewhere), merge existingUrls with the new urlsDict (new
values override existing keys) and merge existingEmailConfig with any
emailConfigDict you build, then pass the merged maps into buildThemeDataBody (or
inject them into requestBody before calling updateDetails) so the PUT preserves
previously stored URLs and email config instead of clearing them.
In `@src/Themes/ThemeScreens/ThemePreview/ThemeMockEmail.res`:
- Around line 74-91: The preview currently renders a placeholder "Your Logo
Here" inside the dashed box instead of the uploaded logo because entity_logo_url
on emailConfig is parsed but never used; update ThemeMockEmail.res so the JSX
that renders the dashed container (the div using ReactDOM.Style.make and the
inner span with className `${body.xs.medium}`) conditionally renders an <img>
using emailConfig.entity_logo_url (with alt text and size constraints via the
same ReactDOM.Style.make) when present, and falls back to the existing span
placeholder when entity_logo_url is empty or invalid; ensure the img uses the
parsed URL directly from emailConfig.entity_logo_url and preserves the
surrounding layout and styles (border, borderColor, borderRadius, padding,
display, alignItems, justifyContent).
In `@src/Themes/ThemeScreens/ThemeSettings/ThemeSettingsHelper.res`:
- Around line 204-209: The remove button is icon-only and lacks an accessible
name; update the button element (the one calling onRemove) to provide an
accessible label derived from the existing label variable—e.g., set aria-label
to something like "Remove {label}" or add a visually-hidden span that reads
"Remove {label}" and reference it with aria-labelledby; ensure the Icon remains
decorative (aria-hidden="true") so assistive tech announces the descriptive
label instead of the icon.
- Around line 228-239: getDisplayUrl currently calls
DownloadUtils.createObjectURL inline during render (using
DownloadUtils.blobInstanceType), causing memory leaks and churn; change it to
memoize the generated object URL per file/key (e.g., a Map keyed by key or file
id) and return the cached URL from getDisplayUrl, create the object URL only
once when the blob is first seen, and ensure you revoke the URL via
URL.revokeObjectURL in the appropriate cleanup (component unmount or when the
file/blob changes) so blob URLs are not created on every render and are properly
released.
- Around line 244-265: Add the missing "Email logo" AssetField to the Icons
block so ThemeSettingsHelper renders the same assets promised by
ThemeUploadAssetsModal: insert an AssetField with label "Email logo",
displayUrl={getDisplayUrl("emailLogo")}, onFileChange={ev =>
onFileSelect("emailLogo", ev)}, onRemove={() => onRemove("emailLogo")},
accept=".png,.jpg,.jpeg" and a unique inputId like "emailLogoFileInput" and
include themeConfigVersion so the email branding asset can be uploaded and
removed via the existing handlers.
In `@src/Themes/ThemeScreens/ThemeUpdate/ThemeUpdate.res`:
- Around line 133-136: The form submit still includes the old logo because when
emailLogoUrl is None you do nothing; update the None branch so that
emailConfigDict explicitly clears the stored value (e.g., set "entity_logo_url"
to an empty string or remove the key) instead of leaving the original from
initialValues. Locate the switch on emailLogoUrl around the call to
valuesDict->getDictfromDict("email_config") and modify the None branch to call
the same Dict operation used in the Some branch (e.g.,
Dict.set("entity_logo_url", "")) so the cleared state is sent to the server.
- Around line 171-181: The emailLogoDisplayUrl currently creates a new blob URL
on every render (via DownloadUtils.createObjectURL and
assets->getvalFromDict("emailLogo")), causing leaked blob URLs; wrap the
blob-to-URL creation in React.useMemo keyed on the underlying blob value (the
result of assets->getvalFromDict("emailLogo") or its JSON.Decode output) to
reuse the same URL across renders, and add a React.useEffect that revokes the
URL (via DownloadUtils.revokeObjectURL or URL.revokeObjectURL) when the memoized
URL changes or the component unmounts so old blob URLs are released.
---
Nitpick comments:
In `@src/Themes/ThemeLanding.res`:
- Line 30: The toOption helper currently only filters out the sentinel but
allows empty or whitespace strings; update toOption (val->Option.flatMap(...))
to also treat values that are empty or all-whitespace as None by trimming the
string and returning None when the trimmed length is 0 (otherwise return Some of
the original/trimmed value), so both sentinel and empty/whitespace identifiers
are dropped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47ec3505-5df0-40e5-87cd-dc95ef58d49c
📒 Files selected for processing (19)
src/Themes/ThemeFeatureUtils.ressrc/Themes/ThemeHelper.ressrc/Themes/ThemeHooks.ressrc/Themes/ThemeLanding.ressrc/Themes/ThemeScreens/ThemeCreate/ThemeCreate.ressrc/Themes/ThemeScreens/ThemeList/ThemeList.ressrc/Themes/ThemeScreens/ThemeList/ThemeListEntity.ressrc/Themes/ThemeScreens/ThemeList/ThemeListHelper.ressrc/Themes/ThemeScreens/ThemePreview/ThemeMockEmail.ressrc/Themes/ThemeScreens/ThemePreview/ThemePreviewUtils.ressrc/Themes/ThemeScreens/ThemeSettings/EmailConfigSettings.ressrc/Themes/ThemeScreens/ThemeSettings/ThemeSettingsHelper.ressrc/Themes/ThemeScreens/ThemeUpdate/ThemeUpdate.ressrc/Themes/ThemeScreens/ThemeUpdate/ThemeUpdateHelper.ressrc/Themes/ThemeScreens/ThemeUpdate/ThemeUpdateType.ressrc/Themes/ThemeScreens/ThemeUpdate/ThemeUpdateUtils.ressrc/components/RawFileInput.ressrc/context/ThemeProvider.ressrc/entryPoints/HyperSwitchApp.res
| <input type_="file" accept hidden=true onChange id={inputId} /> | ||
| <label | ||
| htmlFor={inputId} | ||
| className={`inline-flex items-center justify-center px-4 py-2 ${body.sm.medium} text-nd_gray-700 bg-white border border-nd_gray-300 rounded-md hover:bg-nd_gray-50 cursor-pointer transition`}> | ||
| {React.string(buttonText)} | ||
| </label> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
File upload trigger is not keyboard-accessible.
Using hidden=true on the input and a plain label trigger can prevent keyboard-only users from opening the picker. Please switch to an accessible trigger pattern (focusable button/label + keyboard activation), and avoid hidden for the actual control.
Suggested direction
- <input type_="file" accept hidden=true onChange id={inputId} />
+ <input type_="file" accept onChange id={inputId} className="sr-only" />
<label
htmlFor={inputId}
- className={`inline-flex items-center justify-center px-4 py-2 ${body.sm.medium} text-nd_gray-700 bg-white border border-nd_gray-300 rounded-md hover:bg-nd_gray-50 cursor-pointer transition`}>
+ className={`inline-flex items-center justify-center px-4 py-2 ${body.sm.medium} text-nd_gray-700 bg-white border border-nd_gray-300 rounded-md hover:bg-nd_gray-50 cursor-pointer transition focus-visible:outline focus-visible:outline-2`}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/RawFileInput.res` around lines 11 - 16, The file input is
currently made inaccessible by using hidden=true and a plain label; remove the
hidden attribute on the input referenced by inputId and instead make the file
control visually hidden but still focusable (use a CSS "sr-only"/visually-hidden
class that does not use display:none or the hidden attribute), keep the existing
onChange handler, add an accessible name (aria-label) to the input, and make the
visible trigger keyboard-focusable—replace the plain label trigger with a real
focusable button (or keep the label but give it role="button" and onKeyDown that
calls document.getElementById(inputId).click()) so keyboard users can open the
picker; ensure buttonText, inputId, and onChange are preserved in
RawFileInput.res.
| let buildThemeDataBody = ( | ||
| ~settingsDict: Dict.t<JSON.t>, | ||
| ~urlsDict: Dict.t<JSON.t>, | ||
| ~emailConfigDict: option<Dict.t<JSON.t>>=?, | ||
| ) => { | ||
| open LogicUtils | ||
| let themeDataEntries = [("settings", settingsDict->JSON.Encode.object)] | ||
| if !(urlsDict->isEmptyDict) { | ||
| themeDataEntries->Array.push(("urls", urlsDict->JSON.Encode.object)) | ||
| } | ||
| let bodyEntries = [("theme_data", themeDataEntries->getJsonFromArrayOfJson)] | ||
| switch emailConfigDict { | ||
| | Some(dict) if !(dict->isEmptyDict) => | ||
| bodyEntries->Array.push(("email_config", dict->JSON.Encode.object)) | ||
| | _ => () | ||
| } | ||
| bodyEntries->getJsonFromArrayOfJson |
There was a problem hiding this comment.
Include theme_name in the update payload.
ThemeUpdate.res uses this helper as the whole PUT body, but this builder only emits theme_data and email_config. Any rename made in the update form is silently dropped, and a full-replace PUT can also clear the existing name.
Proposed fix
let buildThemeDataBody = (
+ ~themeName: string,
~settingsDict: Dict.t<JSON.t>,
~urlsDict: Dict.t<JSON.t>,
~emailConfigDict: option<Dict.t<JSON.t>>=?,
) => {
open LogicUtils
let themeDataEntries = [("settings", settingsDict->JSON.Encode.object)]
if !(urlsDict->isEmptyDict) {
themeDataEntries->Array.push(("urls", urlsDict->JSON.Encode.object))
}
- let bodyEntries = [("theme_data", themeDataEntries->getJsonFromArrayOfJson)]
+ let bodyEntries = [
+ ("theme_name", themeName->JSON.Encode.string),
+ ("theme_data", themeDataEntries->getJsonFromArrayOfJson),
+ ]
switch emailConfigDict {
| Some(dict) if !(dict->isEmptyDict) =>
bodyEntries->Array.push(("email_config", dict->JSON.Encode.object))
| _ => ()
}
bodyEntries->getJsonFromArrayOfJson
}Callers should pass valuesDict->getString("theme_name", "").
📝 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.
| let buildThemeDataBody = ( | |
| ~settingsDict: Dict.t<JSON.t>, | |
| ~urlsDict: Dict.t<JSON.t>, | |
| ~emailConfigDict: option<Dict.t<JSON.t>>=?, | |
| ) => { | |
| open LogicUtils | |
| let themeDataEntries = [("settings", settingsDict->JSON.Encode.object)] | |
| if !(urlsDict->isEmptyDict) { | |
| themeDataEntries->Array.push(("urls", urlsDict->JSON.Encode.object)) | |
| } | |
| let bodyEntries = [("theme_data", themeDataEntries->getJsonFromArrayOfJson)] | |
| switch emailConfigDict { | |
| | Some(dict) if !(dict->isEmptyDict) => | |
| bodyEntries->Array.push(("email_config", dict->JSON.Encode.object)) | |
| | _ => () | |
| } | |
| bodyEntries->getJsonFromArrayOfJson | |
| let buildThemeDataBody = ( | |
| ~themeName: string, | |
| ~settingsDict: Dict.t<JSON.t>, | |
| ~urlsDict: Dict.t<JSON.t>, | |
| ~emailConfigDict: option<Dict.t<JSON.t>>=?, | |
| ) => { | |
| open LogicUtils | |
| let themeDataEntries = [("settings", settingsDict->JSON.Encode.object)] | |
| if !(urlsDict->isEmptyDict) { | |
| themeDataEntries->Array.push(("urls", urlsDict->JSON.Encode.object)) | |
| } | |
| let bodyEntries = [ | |
| ("theme_name", themeName->JSON.Encode.string), | |
| ("theme_data", themeDataEntries->getJsonFromArrayOfJson), | |
| ] | |
| switch emailConfigDict { | |
| | Some(dict) if !(dict->isEmptyDict) => | |
| bodyEntries->Array.push(("email_config", dict->JSON.Encode.object)) | |
| | _ => () | |
| } | |
| bodyEntries->getJsonFromArrayOfJson | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Themes/ThemeFeatureUtils.res` around lines 45 - 61, buildThemeDataBody
currently builds only "theme_data" and optional "email_config", so theme renames
are dropped; change its signature to accept an extra ~themeName:string (callers
should pass valuesDict->getString("theme_name", "")) and add ("theme_name",
JSON.Encode.string(themeName)) to the top-level bodyEntries before returning
bodyEntries->getJsonFromArrayOfJson so the PUT payload includes the theme_name;
update ThemeUpdate.res call sites to pass valuesDict->getString("theme_name",
"").
| let (urlsDict, _) = await processAssets(~assets) | ||
|
|
||
| if !(urlsDict->isEmptyDict) { | ||
| let themeUrl = getURL( | ||
| ~entityName=V1(USERS), | ||
| ~methodType=Get, | ||
| ~id=Some(themeId), | ||
| ~userType=#THEME, | ||
| ) | ||
| let currentThemeData = await fetchDetails(themeUrl, ~version=UserInfoTypes.V1) | ||
| let settingsDict = | ||
| currentThemeData | ||
| ->getDictFromJsonObject | ||
| ->getDictfromDict("theme_data") | ||
| ->getDictfromDict("settings") | ||
|
|
||
| let requestBody = buildThemeDataBody(~settingsDict, ~urlsDict) | ||
| let updateUrl = getURL( | ||
| ~entityName=V1(USERS), | ||
| ~methodType=Put, | ||
| ~id=Some(themeId), | ||
| ~userType=#THEME, | ||
| ) | ||
| let _ = await updateDetails(updateUrl, requestBody, Put) |
There was a problem hiding this comment.
Preserve existing theme URLs and email config in this PUT.
processAssets only contains the files chosen in this modal, and buildThemeDataBody only serializes the urlsDict / emailConfigDict you pass in (src/Themes/ThemeFeatureUtils.res:26-62). This code reloads settings, but it does not merge the current theme_data.urls or forward the current email_config, so uploading one asset can clear previously saved branding data on a replace-style update.
Suggested fix
- let settingsDict =
- currentThemeData
- ->getDictFromJsonObject
- ->getDictfromDict("theme_data")
- ->getDictfromDict("settings")
-
- let requestBody = buildThemeDataBody(~settingsDict, ~urlsDict)
+ let currentThemeDict = currentThemeData->getDictFromJsonObject
+ let themeDataDict = currentThemeDict->getDictfromDict("theme_data")
+ let settingsDict = themeDataDict->getDictfromDict("settings")
+ let mergedUrlsDict = themeDataDict->getDictfromDict("urls")->Dict.copy
+ urlsDict
+ ->Dict.toArray
+ ->Array.forEach(((key, value)) => mergedUrlsDict->Dict.set(key, value))
+ let emailConfigDict = currentThemeDict->getDictfromDict("email_config")
+
+ let requestBody =
+ buildThemeDataBody(
+ ~settingsDict,
+ ~urlsDict=mergedUrlsDict,
+ ~emailConfigDict=?Some(emailConfigDict),
+ )📝 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.
| let (urlsDict, _) = await processAssets(~assets) | |
| if !(urlsDict->isEmptyDict) { | |
| let themeUrl = getURL( | |
| ~entityName=V1(USERS), | |
| ~methodType=Get, | |
| ~id=Some(themeId), | |
| ~userType=#THEME, | |
| ) | |
| let currentThemeData = await fetchDetails(themeUrl, ~version=UserInfoTypes.V1) | |
| let settingsDict = | |
| currentThemeData | |
| ->getDictFromJsonObject | |
| ->getDictfromDict("theme_data") | |
| ->getDictfromDict("settings") | |
| let requestBody = buildThemeDataBody(~settingsDict, ~urlsDict) | |
| let updateUrl = getURL( | |
| ~entityName=V1(USERS), | |
| ~methodType=Put, | |
| ~id=Some(themeId), | |
| ~userType=#THEME, | |
| ) | |
| let _ = await updateDetails(updateUrl, requestBody, Put) | |
| let (urlsDict, _) = await processAssets(~assets) | |
| if !(urlsDict->isEmptyDict) { | |
| let themeUrl = getURL( | |
| ~entityName=V1(USERS), | |
| ~methodType=Get, | |
| ~id=Some(themeId), | |
| ~userType=#THEME, | |
| ) | |
| let currentThemeData = await fetchDetails(themeUrl, ~version=UserInfoTypes.V1) | |
| let currentThemeDict = currentThemeData->getDictFromJsonObject | |
| let themeDataDict = currentThemeDict->getDictfromDict("theme_data") | |
| let settingsDict = themeDataDict->getDictfromDict("settings") | |
| let mergedUrlsDict = themeDataDict->getDictfromDict("urls")->Dict.copy | |
| urlsDict | |
| ->Dict.toArray | |
| ->Array.forEach(((key, value)) => mergedUrlsDict->Dict.set(key, value)) | |
| let emailConfigDict = currentThemeDict->getDictfromDict("email_config") | |
| let requestBody = | |
| buildThemeDataBody( | |
| ~settingsDict, | |
| ~urlsDict=mergedUrlsDict, | |
| ~emailConfigDict=?Some(emailConfigDict), | |
| ) | |
| let updateUrl = getURL( | |
| ~entityName=V1(USERS), | |
| ~methodType=Put, | |
| ~id=Some(themeId), | |
| ~userType=#THEME, | |
| ) | |
| let _ = await updateDetails(updateUrl, requestBody, Put) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Themes/ThemeHelper.res` around lines 516 - 539, The PUT currently
overwrites existing theme_data.urls and email_config because we only send
urlsDict from processAssets; fetch the currentThemeData (already loaded into
currentThemeData) and extract existingUrls =
currentThemeData->getDictFromJsonObject->getDictfromDict("theme_data")->getDictfromDict("urls")
and existingEmailConfig =
currentThemeData->getDictFromJsonObject->getDictfromDict("theme_data")->getDictfromDict("email_config")
(or similar keys used elsewhere), merge existingUrls with the new urlsDict (new
values override existing keys) and merge existingEmailConfig with any
emailConfigDict you build, then pass the merged maps into buildThemeDataBody (or
inject them into requestBody before calling updateDetails) so the PUT preserves
previously stored URLs and email config instead of clearing them.
| <div | ||
| style={ReactDOM.Style.make( | ||
| ~border="2px dashed", | ||
| ~borderColor=emailConfig.foreground_color, | ||
| ~opacity="0.3", | ||
| ~borderRadius="8px", | ||
| ~padding="8px 20px", | ||
| ~display="inline-flex", | ||
| ~alignItems="center", | ||
| ~justifyContent="center", | ||
| (), | ||
| )}> | ||
| <span | ||
| className={`${body.xs.medium}`} | ||
| style={ReactDOM.Style.make(~color=emailConfig.foreground_color, ~opacity="0.6", ())}> | ||
| {React.string("Your Logo Here")} | ||
| </span> | ||
| </div> |
There was a problem hiding this comment.
Email logo URL is ignored in preview.
entity_logo_url is parsed but never rendered, so users can’t validate the uploaded logo in the preview.
Suggested fix
- <div
- style={ReactDOM.Style.make(
- ~border="2px dashed",
- ~borderColor=emailConfig.foreground_color,
- ~opacity="0.3",
- ~borderRadius="8px",
- ~padding="8px 20px",
- ~display="inline-flex",
- ~alignItems="center",
- ~justifyContent="center",
- (),
- )}>
- <span
- className={`${body.xs.medium}`}
- style={ReactDOM.Style.make(~color=emailConfig.foreground_color, ~opacity="0.6", ())}>
- {React.string("Your Logo Here")}
- </span>
- </div>
+ {if emailConfig.entity_logo_url->String.trim == "" {
+ <div
+ style={ReactDOM.Style.make(
+ ~border="2px dashed",
+ ~borderColor=emailConfig.foreground_color,
+ ~opacity="0.3",
+ ~borderRadius="8px",
+ ~padding="8px 20px",
+ ~display="inline-flex",
+ ~alignItems="center",
+ ~justifyContent="center",
+ (),
+ )}>
+ <span
+ className={`${body.xs.medium}`}
+ style={ReactDOM.Style.make(~color=emailConfig.foreground_color, ~opacity="0.6", ())}>
+ {React.string("Your Logo Here")}
+ </span>
+ </div>
+ } else {
+ <img
+ src={emailConfig.entity_logo_url}
+ alt={`${emailConfig.entity_name} logo`}
+ style={ReactDOM.Style.make(~maxHeight="48px", ~objectFit="contain", ())}
+ />
+ }}📝 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.
| <div | |
| style={ReactDOM.Style.make( | |
| ~border="2px dashed", | |
| ~borderColor=emailConfig.foreground_color, | |
| ~opacity="0.3", | |
| ~borderRadius="8px", | |
| ~padding="8px 20px", | |
| ~display="inline-flex", | |
| ~alignItems="center", | |
| ~justifyContent="center", | |
| (), | |
| )}> | |
| <span | |
| className={`${body.xs.medium}`} | |
| style={ReactDOM.Style.make(~color=emailConfig.foreground_color, ~opacity="0.6", ())}> | |
| {React.string("Your Logo Here")} | |
| </span> | |
| </div> | |
| {if emailConfig.entity_logo_url->String.trim == "" { | |
| <div | |
| style={ReactDOM.Style.make( | |
| ~border="2px dashed", | |
| ~borderColor=emailConfig.foreground_color, | |
| ~opacity="0.3", | |
| ~borderRadius="8px", | |
| ~padding="8px 20px", | |
| ~display="inline-flex", | |
| ~alignItems="center", | |
| ~justifyContent="center", | |
| (), | |
| )}> | |
| <span | |
| className={`${body.xs.medium}`} | |
| style={ReactDOM.Style.make(~color=emailConfig.foreground_color, ~opacity="0.6", ())}> | |
| {React.string("Your Logo Here")} | |
| </span> | |
| </div> | |
| } else { | |
| <img | |
| src={emailConfig.entity_logo_url} | |
| alt={`${emailConfig.entity_name} logo`} | |
| style={ReactDOM.Style.make(~maxHeight="48px", ~objectFit="contain", ())} | |
| /> | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Themes/ThemeScreens/ThemePreview/ThemeMockEmail.res` around lines 74 -
91, The preview currently renders a placeholder "Your Logo Here" inside the
dashed box instead of the uploaded logo because entity_logo_url on emailConfig
is parsed but never used; update ThemeMockEmail.res so the JSX that renders the
dashed container (the div using ReactDOM.Style.make and the inner span with
className `${body.xs.medium}`) conditionally renders an <img> using
emailConfig.entity_logo_url (with alt text and size constraints via the same
ReactDOM.Style.make) when present, and falls back to the existing span
placeholder when entity_logo_url is empty or invalid; ensure the img uses the
parsed URL directly from emailConfig.entity_logo_url and preserves the
surrounding layout and styles (border, borderColor, borderRadius, padding,
display, alignItems, justifyContent).
| <button | ||
| type_="button" | ||
| onClick={_ => onRemove()} | ||
| className="p-2 hover:bg-nd_gray-100 rounded-md transition"> | ||
| <Icon name="nd-cross" size=16 className="text-nd_gray-500" /> | ||
| </button> |
There was a problem hiding this comment.
Give the remove button an accessible name.
This is an icon-only button, so assistive tech gets no meaningful label for the action. Please derive an accessible name from label so users can tell which asset they are removing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Themes/ThemeScreens/ThemeSettings/ThemeSettingsHelper.res` around lines
204 - 209, The remove button is icon-only and lacks an accessible name; update
the button element (the one calling onRemove) to provide an accessible label
derived from the existing label variable—e.g., set aria-label to something like
"Remove {label}" or add a visually-hidden span that reads "Remove {label}" and
reference it with aria-labelledby; ensure the Icon remains decorative
(aria-hidden="true") so assistive tech announces the descriptive label instead
of the icon.
| let getDisplayUrl = (key: string) => { | ||
| switch assets->LogicUtils.getvalFromDict(key) { | ||
| | Some(value) => | ||
| switch value->JSON.Decode.string { | ||
| | Some(url) => Some(url) | ||
| | None => | ||
| Some( | ||
| DownloadUtils.createObjectURL( | ||
| (value->Identity.jsonToAnyType: DownloadUtils.blobInstanceType), | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Avoid creating blob URLs during render.
DownloadUtils.createObjectURL is called inline on every render and never revoked. That leaks browser memory and causes the preview URL to churn whenever this component rerenders. Memoize the object URL per file and revoke it in cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Themes/ThemeScreens/ThemeSettings/ThemeSettingsHelper.res` around lines
228 - 239, getDisplayUrl currently calls DownloadUtils.createObjectURL inline
during render (using DownloadUtils.blobInstanceType), causing memory leaks and
churn; change it to memoize the generated object URL per file/key (e.g., a Map
keyed by key or file id) and return the cached URL from getDisplayUrl, create
the object URL only once when the blob is first seen, and ensure you revoke the
URL via URL.revokeObjectURL in the appropriate cleanup (component unmount or
when the file/blob changes) so blob URLs are not created on every render and are
properly released.
| <div className="flex flex-col gap-4"> | ||
| <div className={`${body.lg.semibold}`}> {React.string("Icons")} </div> | ||
| <div className="space-y-4"> | ||
| <AssetField | ||
| label="Logo" | ||
| displayUrl={getDisplayUrl("logo")} | ||
| onFileChange={ev => onFileSelect("logo", ev)} | ||
| onRemove={() => onRemove("logo")} | ||
| accept=".png,.jpg,.jpeg" | ||
| inputId="logoFileInput" | ||
| themeConfigVersion | ||
| /> | ||
| <AssetField | ||
| label="Favicon" | ||
| displayUrl={getDisplayUrl("favicon")} | ||
| onFileChange={ev => onFileSelect("favicon", ev)} | ||
| onRemove={() => onRemove("favicon")} | ||
| accept=".png,.ico,.jpg,.jpeg" | ||
| inputId="faviconFileInput" | ||
| themeConfigVersion | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
The shared icon picker is still missing the email logo field.
ThemeUploadAssetsModal now reuses IconSettings (src/Themes/ThemeHelper.res:577-579), and its copy explicitly promises email-logo upload on Line 573, but this component only renders Logo and Favicon. Right now there is no way to attach or remove the email branding asset from that flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Themes/ThemeScreens/ThemeSettings/ThemeSettingsHelper.res` around lines
244 - 265, Add the missing "Email logo" AssetField to the Icons block so
ThemeSettingsHelper renders the same assets promised by ThemeUploadAssetsModal:
insert an AssetField with label "Email logo",
displayUrl={getDisplayUrl("emailLogo")}, onFileChange={ev =>
onFileSelect("emailLogo", ev)}, onRemove={() => onRemove("emailLogo")},
accept=".png,.jpg,.jpeg" and a unique inputId like "emailLogoFileInput" and
include themeConfigVersion so the email branding asset can be uploaded and
removed via the existing handlers.
| let emailConfigDict = valuesDict->getDictfromDict("email_config") | ||
| switch emailLogoUrl { | ||
| | Some(url) => emailConfigDict->Dict.set("entity_logo_url", url) | ||
| | None => () |
There was a problem hiding this comment.
Clearing the email logo never reaches the server.
emailConfigDict still contains the original entity_logo_url from initialValues. When the user removes the asset, only assets changes; the None branch leaves that old value intact, so submit re-sends the deleted logo. Explicitly clear entity_logo_url when emailLogo has been removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Themes/ThemeScreens/ThemeUpdate/ThemeUpdate.res` around lines 133 - 136,
The form submit still includes the old logo because when emailLogoUrl is None
you do nothing; update the None branch so that emailConfigDict explicitly clears
the stored value (e.g., set "entity_logo_url" to an empty string or remove the
key) instead of leaving the original from initialValues. Locate the switch on
emailLogoUrl around the call to valuesDict->getDictfromDict("email_config") and
modify the None branch to call the same Dict operation used in the Some branch
(e.g., Dict.set("entity_logo_url", "")) so the cleared state is sent to the
server.
| let emailLogoDisplayUrl = switch assets->getvalFromDict("emailLogo") { | ||
| | Some(value) => | ||
| switch value->JSON.Decode.string { | ||
| | Some(url) => Some(url) | ||
| | None => | ||
| Some( | ||
| DownloadUtils.createObjectURL( | ||
| (value->Identity.jsonToAnyType: DownloadUtils.blobInstanceType), | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l src/Themes/ThemeScreens/ThemeUpdate/ThemeUpdate.resRepository: juspay/hyperswitch-control-center
Length of output: 132
🏁 Script executed:
sed -n '150,200p' src/Themes/ThemeScreens/ThemeUpdate/ThemeUpdate.resRepository: juspay/hyperswitch-control-center
Length of output: 1986
🏁 Script executed:
# Search for createObjectURL usage in the file
rg -n "createObjectURL" src/Themes/ThemeScreens/ThemeUpdate/ThemeUpdate.resRepository: juspay/hyperswitch-control-center
Length of output: 129
🏁 Script executed:
# Search for cleanup or revoke patterns in the file
rg -n "revoke|cleanup|useEffect|useLayoutEffect" src/Themes/ThemeScreens/ThemeUpdateThemeUpdate.resRepository: juspay/hyperswitch-control-center
Length of output: 167
🏁 Script executed:
# Search for cleanup/revoke in the entire file (fix typo)
rg -n "revoke|cleanup|useEffect|useLayoutEffect|useMemo" src/Themes/ThemeScreens/ThemeUpdate/ThemeUpdate.resRepository: juspay/hyperswitch-control-center
Length of output: 105
🏁 Script executed:
# Get more context - see the full component structure
sed -n '1,100p' src/Themes/ThemeScreens/ThemeUpdate/ThemeUpdate.resRepository: juspay/hyperswitch-control-center
Length of output: 3592
🏁 Script executed:
# Check if there's a React component definition and understand the structure
rg -n "@react\|component\|useState\|useCallback" src/Themes/ThemeScreens/ThemeUpdate/ThemeUpdate.resRepository: juspay/hyperswitch-control-center
Length of output: 59
🏁 Script executed:
# See lines around and after the problematic code
sed -n '160,220p' src/Themes/ThemeScreens/ThemeUpdate/ThemeUpdate.resRepository: juspay/hyperswitch-control-center
Length of output: 2831
🏁 Script executed:
# Check if there's any revoke logic in the file
rg -n -A5 -B5 "revoke" src/Themes/ThemeScreens/ThemeUpdate/ThemeUpdate.resRepository: juspay/hyperswitch-control-center
Length of output: 59
🏁 Script executed:
# Check DownloadUtils to understand createObjectURL
fd -n "DownloadUtils*" -type fRepository: juspay/hyperswitch-control-center
Length of output: 307
🏁 Script executed:
fd DownloadUtils -type fRepository: juspay/hyperswitch-control-center
Length of output: 247
🏁 Script executed:
# Look for DownloadUtils in the codebase
rg -l "DownloadUtils" | head -20Repository: juspay/hyperswitch-control-center
Length of output: 1465
🏁 Script executed:
rg "createObjectURL" --type res -A5 | head -40Repository: juspay/hyperswitch-control-center
Length of output: 104
🏁 Script executed:
# Find DownloadUtils module
fd DownloadUtilsRepository: juspay/hyperswitch-control-center
Length of output: 104
🏁 Script executed:
cat -n src/utils/DownloadUtils.resRepository: juspay/hyperswitch-control-center
Length of output: 2641
🏁 Script executed:
# Verify if there's any useMemo or cleanup related to emailLogoDisplayUrl
rg -n "useMemo\|useEffect.*emailLogo\|revokeObjectURL" src/Themes/ThemeScreens/ThemeUpdate/ThemeUpdate.resRepository: juspay/hyperswitch-control-center
Length of output: 59
🏁 Script executed:
# Check the full lifecycle of this component to see if there's any cleanup
sed -n '78,120p' src/Themes/ThemeScreens/ThemeUpdate/ThemeUpdate.resRepository: juspay/hyperswitch-control-center
Length of output: 1268
Memoize the preview URL and revoke it in cleanup.
The emailLogoDisplayUrl let binding (lines 171-181) creates a fresh blob URL via DownloadUtils.createObjectURL on every render. When assets state updates from file selection or form changes, the component re-renders and allocates a new URL without revoking the old one, causing memory leaks until page unload. Use React.useMemo to preserve the URL across renders and add a React.useEffect cleanup to revoke it when the component unmounts or the blob changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Themes/ThemeScreens/ThemeUpdate/ThemeUpdate.res` around lines 171 - 181,
The emailLogoDisplayUrl currently creates a new blob URL on every render (via
DownloadUtils.createObjectURL and assets->getvalFromDict("emailLogo")), causing
leaked blob URLs; wrap the blob-to-URL creation in React.useMemo keyed on the
underlying blob value (the result of assets->getvalFromDict("emailLogo") or its
JSON.Decode output) to reuse the same URL across renders, and add a
React.useEffect that revokes the URL (via DownloadUtils.revokeObjectURL or
URL.revokeObjectURL) when the memoized URL changes or the component unmounts so
old blob URLs are released.
muditbhutani
left a comment
muditbhutani
left a comment
There was a problem hiding this comment.
Merge Confidence: 2.5/5 - Moderate
"Needs attention - review carefully"
Assessment
- ✓ Good refactoring: parseThemeJson extraction eliminates ~80 lines of duplication in ThemeProvider
- ✓ Well-structured shared hook (useProcessAssets) consolidates asset upload logic across create and update flows
- ✓ OMP-aware routing correctly matches EntityScaffold's URL parsing pattern (id/profileId/merchantId/orgId)
- ✓ CSS cleanup (trailing spaces, redundant flex-row, unnecessary wrapper divs) is consistently applied
- ⚠ Blocking: Asset removal doesn't propagate to server — handleAssetRemove deletes from local dict but useProcessAssets never sends null/empty for removed URLs, making the remove button a no-op on save
- ⚠ Form values dict mutated in place during onSubmit (getDictfromDict returns a reference, then Dict.set modifies it) — fragile if React Final Form re-reads values
- ⚠ Email logo upload missing from ThemeCreate flow — only available on ThemeUpdate, creating an inconsistent user experience between create and edit
- ⚠ createObjectURL called without revokeObjectURL cleanup — minor memory leak on repeated file selections
Review: Add email config support to theme system with full update page implementation
This PR adds email configuration (colors, entity name, logo) as a new tab alongside dashboard config in both theme create and update flows, implements the previously-stub ThemeUpdate page with full CRUD operations, and refactors asset upload logic into a shared hook. The primary risk is a data integrity issue: removing an existing asset (logo/favicon/email logo) in the UI is silently lost on save because the removal isn't propagated to the server — the asset URL field is simply omitted from the PUT request rather than explicitly nulled. This should be fixed before merge. Secondary concerns include form state mutation during submit and missing email logo upload in the create flow.
| @@ -23,6 +23,44 @@ let getEntityTypeFromStep = (stepVariant: ThemeTypes.lineageSelectionSteps) => | |||
| | _ => "" | |||
There was a problem hiding this comment.
Review shared asset processing hook for correctness and data loss risk
Risk: HIGH
The new ThemeHooks.useProcessAssets hook centralizes asset upload logic previously duplicated in ThemeUploadAssetsModal. It handles three asset types (logo, favicon, emailLogo) and distinguishes between existing URL strings and new File blobs via JSON.Decode.string.
What changed: Asset upload was extracted from ThemeHelper.ThemeUploadAssetsModal into a reusable hook. The hook now also supports emailLogo as a third asset type. Both ThemeUploadAssetsModal and the new ThemeUpdate page consume it.
Risk: Asset removal doesn't propagate to the server. When a user removes an existing asset (e.g., logo) via handleAssetRemove, the key is deleted from the local assets dict. On submit, useProcessAssets won't find that key, so it won't add a URL to urlsDict. But buildThemeDataBody only includes URLs if urlsDict is non-empty — it doesn't explicitly null-out removed URLs. The server will retain the old URL. This means "remove logo" in the UI is a no-op on submit.
Verify: Trace the full remove-then-submit flow for an existing asset and confirm the server receives an explicit null or empty string for the removed URL field.
Diagram:
sequenceDiagram
participant U as User
participant UI as ThemeUpdate
participant Hook as useProcessAssets
participant API as Server
U->>UI: Remove existing logo
UI->>UI: handleAssetRemove("logo")
Note over UI: "logo" key deleted from assets dict
U->>UI: Click "Update Theme"
UI->>Hook: processAssets(~assets)
Note over Hook: No "logo" key found → skip
Hook-->>UI: (urlsDict={}, emailLogoUrl=None)
UI->>API: PUT /theme (no logoUrl field)
Note over API: Old logoUrl retained — removal lost
| @@ -27,6 +27,7 @@ let make = (~remainingPath) => { | |||
| | _ => setScreenState(_ => PageLoaderWrapper.Error("Error fetching theme list")) | |||
There was a problem hiding this comment.
Verify ThemeUpdate page with OMP-aware routing and context switching
Risk: MEDIUM
The ThemeUpdate component was previously a stub showing only a heading. This PR implements the full update flow: fetch theme by ID, populate form, handle asset uploads, submit updates, and delete themes. It also adds OMP (Org-Merchant-Profile) aware routing via renderCustomWithOMP and internalSwitch.
URL construction in ThemeListEntity.getShowLink builds URLs as /theme/{themeId}/{profileId}/{merchantId}/{orgId}. The EntityScaffold parses list{id, profileId, merchantId, orgId} and passes these to renderCustomWithOMP(themeId, profileId, merchantId, orgId). The segment order matches, so routing is correct.
Context switching: internalSwitch is called on mount to switch to the correct org/merchant/profile context before fetching theme data. If any of orgId, merchantId, profileId are None (from renderShow fallback path), this passes None to internalSwitch which should be safe.
Verify: Test the renderShow fallback path (L5 of ThemeLanding.res#H2) where all OMP params are None — confirm internalSwitch with all-None params doesn't error or switch to an unintended context.
Diagram:
flowchart LR
subgraph ThemeList
TL[ThemeList] -->|getShowLink| URL["/theme/{id}/{profile}/{merchant}/{org}"]
end
subgraph EntityScaffold
URL -->|parse list| ES["renderCustomWithOMP(id, profile, merchant, org)"]
end
subgraph ThemeUpdate
ES --> TU[ThemeUpdate component]
TU -->|1| IS[internalSwitch to OMP context]
IS -->|2| FETCH[GET /theme/{id}]
FETCH -->|3| FORM[Populate form + assets]
end
| @@ -50,17 +50,8 @@ let make = () => { | |||
| subTitle="Personalize your dashboard look with a live preview." | |||
There was a problem hiding this comment.
Review email config feature addition across create and update flows
Risk: MEDIUM
A new "Email Config" tab is added to both ThemeCreate and ThemeUpdate pages. This includes email settings (entity name, primary/foreground/background colors), an email logo asset, and a live preview via ThemeMockEmail.
What's new: EmailConfigSettings renders form fields under the email_config.* namespace. ThemeMockEmail reads form values via ReactFinalForm and renders an email preview. buildThemeDataBody conditionally includes email_config in the request body. ThemeUpdateUtils.themeBodyMapper parses the email_config field from the API response.
The create flow doesn't handle email logo assets. ThemeCreate uses the new EmailConfigSettings component for form fields but doesn't wire up any asset management for the email logo. The email logo upload only appears in the ThemeUpdate page. On create, the email logo URL field (entity_logo_url) will be whatever the default is (empty string), with no way for the user to upload one during initial creation.
Verify: Confirm whether the product intent is to only allow email logo uploads during theme update (post-creation), or if this is a gap in the create flow.
Diagram:
flowchart TD
subgraph ThemeCreate
TC[ThemeCreate] --> TAB1["Dashboard Config tab"]
TC --> TAB2["Email Config tab"]
TAB2 --> ECS[EmailConfigSettings form fields]
TAB2 --> TME[ThemeMockEmail preview]
TAB2 -.- NOUP["No email logo upload ⚠"]
end
subgraph ThemeUpdate
TU[ThemeUpdate] --> UTAB1["Dashboard Config tab"]
TU --> UTAB2["Email Config tab"]
UTAB2 --> UECS[EmailConfigSettings form fields]
UTAB2 --> LOGO["Email logo AssetField ✓"]
UTAB2 --> UTME[ThemeMockEmail preview]
end
| @@ -0,0 +1,28 @@ | |||
| open ThemeUpdateType | |||
There was a problem hiding this comment.
Verify parseThemeJson extraction and cleanup of ThemeProvider
Risk: LOW
The theme JSON parsing logic (~80 lines) was extracted from the configCustomDomainTheme callback in ThemeProvider into a standalone parseThemeJson function. This is a pure refactor — the function is now reused by both ThemeProvider.configCustomDomainTheme and ThemeUpdateUtils.themeBodyMapper.
The extraction is clean: identical logic, no behavioral changes. The configCustomDomainTheme callback now simply calls parseThemeJson and passes the result to Window.appendStyle.
Verify: Confirm parseThemeJson handles missing/empty JSON gracefully (it uses getDictfromDict which returns empty dicts for missing keys, and getString with defaults — this looks safe).
Diagram:
flowchart LR
subgraph Before
CCT["configCustomDomainTheme"] -->|inline 80 lines| WAS["Window.appendStyle"]
end
subgraph After
PTJ["parseThemeJson()"] --> CCT2["configCustomDomainTheme"]
PTJ --> TBM["themeBodyMapper"]
CCT2 --> WAS2["Window.appendStyle"]
end
| } | ||
| } | ||
|
|
||
| let handleAssetRemove = (setAssets, key) => { |
There was a problem hiding this comment.
Feedback: Asset removal silently lost on save
Severity: 🔴 Blocking
Agent System:
handleAssetRemove deletes the key from the assets dict. useProcessAssets only processes keys that exist, so removed assets are never sent to the server. The server retains the old URL.
Evidence: handleAssetRemove removes key → processAssets skips missing keys → buildThemeDataBody only includes non-empty urlsDict
| @@ -1,9 +1,253 @@ | |||
| @react.component | |||
| let make = (~themeId="") => { | |||
| let make = (~themeId, ~orgId, ~merchantId, ~profileId) => { | |||
There was a problem hiding this comment.
Feedback: ThemeUpdate signature change
Severity: 🔵 Nice to Have
Agent System:
ThemeUpdate.make changed from optional themeId with default to required themeId plus three new required params. All call sites in this PR are updated (ThemeLanding.res).
Evidence: Old: let make = (~themeId="") => { ... New: let make = (~themeId, ~orgId, ~merchantId, ~profileId) => {
| } | ||
| } | ||
|
|
||
| React.useEffect(() => { |
There was a problem hiding this comment.
Feedback: useEffect dependency array may miss prop changes
Severity: 🔵 Nice to Have
Agent Claude:
The useEffect has an empty dependency array [], but references themeId, orgId, merchantId, and profileId from props. If this component is reused with different props (e.g., navigating between themes without unmounting), it won't refetch.
The key={themeId} on the <Form> at L168 forces a remount on themeId change, which mitigates this for themeId — but if orgId/merchantId/profileId change independently, the effect won't re-run.
Recommendation: Add the relevant props to the dependency array, or confirm the component always fully remounts on prop changes.
| let (urlsDict, emailLogoUrl) = await processAssets(~assets) | ||
|
|
||
| let settingsDict = valuesDict->getDictfromDict("theme_data")->getDictfromDict("settings") | ||
| let emailConfigDict = valuesDict->getDictfromDict("email_config") |
There was a problem hiding this comment.
Feedback: Mutating form values dict in place during submit
Severity: 🔵 Nice to Have
Agent Claude:
getDictfromDict returns a mutable reference to the nested dict within valuesDict. The subsequent Dict.set("entity_logo_url", url) at L141 mutates the form values in place. While this may work because the form values aren't read again after this point, mutating form state is fragile — if React Final Form re-renders or the form values are used elsewhere, this could cause subtle bugs.
Recommendation: Copy the dict before mutating: let emailConfigDict = valuesDict->getDictfromDict("email_config")->Dict.copy.
| let updateUrl = getURL( | ||
| ~entityName=V1(USERS), | ||
| ~methodType=Put, | ||
| ~id=Some(`${themeId}`), |
There was a problem hiding this comment.
Feedback: Unnecessary template literal wrapping a string variable
Severity: ⚪ Nitpick
Agent Claude:
`${themeId}` is a redundant template literal — themeId is already a string. Use themeId directly.
~id=Some(themeId),| let updateUrl = getURL( | ||
| ~entityName=V1(USERS), | ||
| ~methodType=Put, | ||
| ~id=Some(`${themeId}`), |
There was a problem hiding this comment.
Feedback: Unnecessary string interpolation of themeId
Severity: ⚪ Nitpick
Agent Claude:
~id=Some(\${themeId}`)is a redundant string interpolation —themeIdis already a string. Use~id=Some(themeId)` directly, which is how it's used elsewhere in this same file (e.g., L47, L95).
muditbhutani
left a comment
muditbhutani
left a comment
There was a problem hiding this comment.
Merge Confidence: 2.5/5 - Moderate
"Needs attention - review carefully"
Assessment
- ✓ Good refactoring: parseThemeJson extraction eliminates ~80 lines of duplication in ThemeProvider
- ✓ Well-structured shared hook (useProcessAssets) consolidates asset upload logic across create and update flows
- ✓ OMP-aware routing correctly matches EntityScaffold's URL parsing pattern (id/profileId/merchantId/orgId)
- ✓ CSS cleanup (trailing spaces, redundant flex-row, unnecessary wrapper divs) is consistently applied
- ⚠ Blocking: Asset removal doesn't propagate to server — handleAssetRemove deletes from local dict but useProcessAssets never sends null/empty for removed URLs, making the remove button a no-op on save
- ⚠ Form values dict mutated in place during onSubmit (getDictfromDict returns a reference, then Dict.set modifies it) — fragile if React Final Form re-reads values
- ⚠ Email logo upload missing from ThemeCreate flow — only available on ThemeUpdate, creating an inconsistent user experience between create and edit
- ⚠ createObjectURL called without revokeObjectURL cleanup — minor memory leak on repeated file selections
Review: Add email config support to theme system with full update page implementation
This PR adds email configuration (colors, entity name, logo) as a new tab alongside dashboard config in both theme create and update flows, implements the previously-stub ThemeUpdate page with full CRUD operations, and refactors asset upload logic into a shared hook. The primary risk is a data integrity issue: removing an existing asset (logo/favicon/email logo) in the UI is silently lost on save because the removal isn't propagated to the server — the asset URL field is simply omitted from the PUT request rather than explicitly nulled. This should be fixed before merge. Secondary concerns include form state mutation during submit and missing email logo upload in the create flow.
| @@ -23,6 +23,44 @@ let getEntityTypeFromStep = (stepVariant: ThemeTypes.lineageSelectionSteps) => | |||
| | _ => "" | |||
There was a problem hiding this comment.
Review shared asset processing hook for correctness and data loss risk
Risk: HIGH
The new ThemeHooks.useProcessAssets hook centralizes asset upload logic previously duplicated in ThemeUploadAssetsModal. It handles three asset types (logo, favicon, emailLogo) and distinguishes between existing URL strings and new File blobs via JSON.Decode.string.
What changed: Asset upload was extracted from ThemeHelper.ThemeUploadAssetsModal into a reusable hook. The hook now also supports emailLogo as a third asset type. Both ThemeUploadAssetsModal and the new ThemeUpdate page consume it.
Risk: Asset removal doesn't propagate to the server. When a user removes an existing asset (e.g., logo) via handleAssetRemove, the key is deleted from the local assets dict. On submit, useProcessAssets won't find that key, so it won't add a URL to urlsDict. But buildThemeDataBody only includes URLs if urlsDict is non-empty — it doesn't explicitly null-out removed URLs. The server will retain the old URL. This means "remove logo" in the UI is a no-op on submit.
Verify: Trace the full remove-then-submit flow for an existing asset and confirm the server receives an explicit null or empty string for the removed URL field.
Diagram:
sequenceDiagram
participant U as User
participant UI as ThemeUpdate
participant Hook as useProcessAssets
participant API as Server
U->>UI: Remove existing logo
UI->>UI: handleAssetRemove("logo")
Note over UI: "logo" key deleted from assets dict
U->>UI: Click "Update Theme"
UI->>Hook: processAssets(~assets)
Note over Hook: No "logo" key found → skip
Hook-->>UI: (urlsDict={}, emailLogoUrl=None)
UI->>API: PUT /theme (no logoUrl field)
Note over API: Old logoUrl retained — removal lost
| @@ -27,6 +27,7 @@ let make = (~remainingPath) => { | |||
| | _ => setScreenState(_ => PageLoaderWrapper.Error("Error fetching theme list")) | |||
There was a problem hiding this comment.
Verify ThemeUpdate page with OMP-aware routing and context switching
Risk: MEDIUM
The ThemeUpdate component was previously a stub showing only a heading. This PR implements the full update flow: fetch theme by ID, populate form, handle asset uploads, submit updates, and delete themes. It also adds OMP (Org-Merchant-Profile) aware routing via renderCustomWithOMP and internalSwitch.
URL construction in ThemeListEntity.getShowLink builds URLs as /theme/{themeId}/{profileId}/{merchantId}/{orgId}. The EntityScaffold parses list{id, profileId, merchantId, orgId} and passes these to renderCustomWithOMP(themeId, profileId, merchantId, orgId). The segment order matches, so routing is correct.
Context switching: internalSwitch is called on mount to switch to the correct org/merchant/profile context before fetching theme data. If any of orgId, merchantId, profileId are None (from renderShow fallback path), this passes None to internalSwitch which should be safe.
Verify: Test the renderShow fallback path (L5 of ThemeLanding.res#H2) where all OMP params are None — confirm internalSwitch with all-None params doesn't error or switch to an unintended context.
Diagram:
flowchart LR
subgraph ThemeList
TL[ThemeList] -->|getShowLink| URL["/theme/{id}/{profile}/{merchant}/{org}"]
end
subgraph EntityScaffold
URL -->|parse list| ES["renderCustomWithOMP(id, profile, merchant, org)"]
end
subgraph ThemeUpdate
ES --> TU[ThemeUpdate component]
TU -->|1| IS[internalSwitch to OMP context]
IS -->|2| FETCH[GET /theme/{id}]
FETCH -->|3| FORM[Populate form + assets]
end
| @@ -50,17 +50,8 @@ let make = () => { | |||
| subTitle="Personalize your dashboard look with a live preview." | |||
There was a problem hiding this comment.
Review email config feature addition across create and update flows
Risk: MEDIUM
A new "Email Config" tab is added to both ThemeCreate and ThemeUpdate pages. This includes email settings (entity name, primary/foreground/background colors), an email logo asset, and a live preview via ThemeMockEmail.
What's new: EmailConfigSettings renders form fields under the email_config.* namespace. ThemeMockEmail reads form values via ReactFinalForm and renders an email preview. buildThemeDataBody conditionally includes email_config in the request body. ThemeUpdateUtils.themeBodyMapper parses the email_config field from the API response.
The create flow doesn't handle email logo assets. ThemeCreate uses the new EmailConfigSettings component for form fields but doesn't wire up any asset management for the email logo. The email logo upload only appears in the ThemeUpdate page. On create, the email logo URL field (entity_logo_url) will be whatever the default is (empty string), with no way for the user to upload one during initial creation.
Verify: Confirm whether the product intent is to only allow email logo uploads during theme update (post-creation), or if this is a gap in the create flow.
Diagram:
flowchart TD
subgraph ThemeCreate
TC[ThemeCreate] --> TAB1["Dashboard Config tab"]
TC --> TAB2["Email Config tab"]
TAB2 --> ECS[EmailConfigSettings form fields]
TAB2 --> TME[ThemeMockEmail preview]
TAB2 -.- NOUP["No email logo upload ⚠"]
end
subgraph ThemeUpdate
TU[ThemeUpdate] --> UTAB1["Dashboard Config tab"]
TU --> UTAB2["Email Config tab"]
UTAB2 --> UECS[EmailConfigSettings form fields]
UTAB2 --> LOGO["Email logo AssetField ✓"]
UTAB2 --> UTME[ThemeMockEmail preview]
end
| @@ -0,0 +1,28 @@ | |||
| open ThemeUpdateType | |||
There was a problem hiding this comment.
Verify parseThemeJson extraction and cleanup of ThemeProvider
Risk: LOW
The theme JSON parsing logic (~80 lines) was extracted from the configCustomDomainTheme callback in ThemeProvider into a standalone parseThemeJson function. This is a pure refactor — the function is now reused by both ThemeProvider.configCustomDomainTheme and ThemeUpdateUtils.themeBodyMapper.
The extraction is clean: identical logic, no behavioral changes. The configCustomDomainTheme callback now simply calls parseThemeJson and passes the result to Window.appendStyle.
Verify: Confirm parseThemeJson handles missing/empty JSON gracefully (it uses getDictfromDict which returns empty dicts for missing keys, and getString with defaults — this looks safe).
Diagram:
flowchart LR
subgraph Before
CCT["configCustomDomainTheme"] -->|inline 80 lines| WAS["Window.appendStyle"]
end
subgraph After
PTJ["parseThemeJson()"] --> CCT2["configCustomDomainTheme"]
PTJ --> TBM["themeBodyMapper"]
CCT2 --> WAS2["Window.appendStyle"]
end
| } | ||
| } | ||
|
|
||
| let handleAssetRemove = (setAssets, key) => { |
There was a problem hiding this comment.
Feedback: Asset removal silently lost on save
Severity: 🔴 Blocking
Agent System:
handleAssetRemove deletes the key from the assets dict. useProcessAssets only processes keys that exist, so removed assets are never sent to the server. The server retains the old URL.
Evidence: handleAssetRemove removes key → processAssets skips missing keys → buildThemeDataBody only includes non-empty urlsDict
| @@ -1,9 +1,253 @@ | |||
| @react.component | |||
| let make = (~themeId="") => { | |||
| let make = (~themeId, ~orgId, ~merchantId, ~profileId) => { | |||
There was a problem hiding this comment.
Feedback: ThemeUpdate signature change
Severity: 🔵 Nice to Have
Agent System:
ThemeUpdate.make changed from optional themeId with default to required themeId plus three new required params. All call sites in this PR are updated (ThemeLanding.res).
Evidence: Old: let make = (~themeId="") => { ... New: let make = (~themeId, ~orgId, ~merchantId, ~profileId) => {
| } | ||
| } | ||
|
|
||
| React.useEffect(() => { |
There was a problem hiding this comment.
Feedback: useEffect dependency array may miss prop changes
Severity: 🔵 Nice to Have
Agent Claude:
The useEffect has an empty dependency array [], but references themeId, orgId, merchantId, and profileId from props. If this component is reused with different props (e.g., navigating between themes without unmounting), it won't refetch.
The key={themeId} on the <Form> at L168 forces a remount on themeId change, which mitigates this for themeId — but if orgId/merchantId/profileId change independently, the effect won't re-run.
Recommendation: Add the relevant props to the dependency array, or confirm the component always fully remounts on prop changes.
| let (urlsDict, emailLogoUrl) = await processAssets(~assets) | ||
|
|
||
| let settingsDict = valuesDict->getDictfromDict("theme_data")->getDictfromDict("settings") | ||
| let emailConfigDict = valuesDict->getDictfromDict("email_config") |
There was a problem hiding this comment.
Feedback: Mutating form values dict in place during submit
Severity: 🔵 Nice to Have
Agent Claude:
getDictfromDict returns a mutable reference to the nested dict within valuesDict. The subsequent Dict.set("entity_logo_url", url) at L141 mutates the form values in place. While this may work because the form values aren't read again after this point, mutating form state is fragile — if React Final Form re-renders or the form values are used elsewhere, this could cause subtle bugs.
Recommendation: Copy the dict before mutating: let emailConfigDict = valuesDict->getDictfromDict("email_config")->Dict.copy.
| let updateUrl = getURL( | ||
| ~entityName=V1(USERS), | ||
| ~methodType=Put, | ||
| ~id=Some(`${themeId}`), |
There was a problem hiding this comment.
Feedback: Unnecessary template literal wrapping a string variable
Severity: ⚪ Nitpick
Agent Claude:
`${themeId}` is a redundant template literal — themeId is already a string. Use themeId directly.
~id=Some(themeId),| let updateUrl = getURL( | ||
| ~entityName=V1(USERS), | ||
| ~methodType=Put, | ||
| ~id=Some(`${themeId}`), |
There was a problem hiding this comment.
Feedback: Unnecessary string interpolation of themeId
Severity: ⚪ Nitpick
Agent Claude:
~id=Some(\${themeId}`)is a redundant string interpolation —themeIdis already a string. Use~id=Some(themeId)` directly, which is how it's used elsewhere in this same file (e.g., L47, L95).
3 participants
Type of Change
Description
Screen.Recording.2026-04-13.at.3.50.03.PM.mov
Motivation and Context
How did you test it?
Where to test it?
Checklist
npm run re:buildSummary by CodeRabbit
Release Notes