refactor(platform): improve ui#475
Conversation
|
Too many files changed for review. ( |
📝 WalkthroughWalkthroughThis pull request introduces substantial UI and backend refactoring across the platform. It removes offset-based pagination infrastructure from Convex, replacing it with cursor-based and count-based approaches. Form components gain optional description props with accessibility support. Table configurations are updated with new column metadata (headerLabel) and header rendering changes. Routes receive SEO metadata via a new seo utility. Locale detection is enhanced with Accept-Language header parsing and resolution. Skeleton components are simplified by removing pagination support. Several table features are refactored to use component composition patterns instead of monolithic containers (e.g., ApiKeys, Teams settings pages). Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
services/platform/lib/pagination/types.ts (1)
168-169:⚠️ Potential issue | 🟡 MinorStale JSDoc:
clearAllstill references "reset pagination".Pagination state has been removed from this module, but the comment on
clearAllstill says "Clear all filters and reset pagination." Update to match the new scope.📝 Proposed fix
- /** Clear all filters and reset pagination */ + /** Clear all filters and sorting */services/platform/convex/lib/pagination/types.ts (1)
15-58: 🧹 Nitpick | 🔵 Trivial
DEFAULT_PAGE_SIZEis exported twice — once via the wildcard on Line 15 and again explicitly on Line 58.The
export *on line 15 already re-exportsDEFAULT_PAGE_SIZEfrom the shared schemas module. The explicitexport { DEFAULT_PAGE_SIZE }on line 58 is redundant. Consider removing line 58 to avoid confusion.♻️ Remove redundant export
-export { DEFAULT_PAGE_SIZE };services/platform/convex/lib/pagination/helpers.ts (1)
44-47:⚠️ Potential issue | 🟠 MajorPre-existing:
maxScanItemscutoff silently reportsisDone: trueeven when more data may exist.When the loop breaks at
scanned >= maxScanItems(line 45),hasMoreremainsfalse, so the caller receivesisDone: trueand stops paginating — potentially missing rows beyond the scan window. Consider settinghasMore = true(or adding atruncatedflag) when the scan limit is hit to avoid silent data loss for clients.This is pre-existing behavior, not introduced by this PR, but worth addressing given the public API surface reduction that may increase reliance on this function.
🐛 Proposed fix
if (scanned >= maxScanItems) { + hasMore = true; break; } } + + // Also account for scan-limit hit before cursor was even found + // or while skipping filtered itemsNote: the same
scanned >= maxScanItemscheck on line 31 (inside the filter branch) also breaks without signalling that more data may exist. Both exit paths should sethasMore = true.if (filter && !filter(item)) { if (scanned >= maxScanItems) { + hasMore = true; break; } continue; }services/platform/app/features/settings/integrations/components/shopify-integration-dialog.tsx (2)
60-67: 🧹 Nitpick | 🔵 TrivialDuplicate toast descriptions for connect vs. update success.
Both branches of the ternary produce the identical description string (
integrations.connectedTo). Thetitleis correctly differentiated (updateSuccessfulvsconnectionSuccessful), but the description likely should differ too — or if the same text is intentional, collapse the ternary.Proposed fix
toast({ title: isConnected ? t('integrations.updateSuccessful') : t('integrations.connectionSuccessful'), - description: isConnected - ? t('integrations.connectedTo', { provider: 'Shopify' }) - : t('integrations.connectedTo', { provider: 'Shopify' }), + description: t('integrations.connectedTo', { provider: 'Shopify' }), variant: 'success', });
69-79:⚠️ Potential issue | 🟡 MinorWrong i18n key in the connect/update error handler.
failedToDisconnectis used as the error description when a connect or update attempt fails. This will show a misleading message to the user (e.g., "Failed to disconnect from Shopify" when they were trying to connect).The proposed key
integrations.failedToConnectdoes not exist in the i18n files. Use an appropriate existing key instead, such asintegrations.failedToTestConnection("Failed to test connection"), or add thefailedToConnectkey to your i18n configuration.Example fix using existing key
} catch { // Keep dialog open and surface error without leaking credentials toast({ title: isConnected ? t('integrations.updateFailed') : t('integrations.connectionTestFailed'), - description: t('integrations.failedToDisconnect', { + description: t('integrations.failedToTestConnection', { provider: 'Shopify', }), variant: 'destructive', });services/platform/app/features/settings/integrations/components/circuly-integration-dialog.tsx (1)
126-137:⚠️ Potential issue | 🟡 MinorWrong i18n key —
failedToDisconnectused in the connect error handler.Same issue as in the Shopify dialog: the
descriptionin the connect/update catch block usesintegrations.failedToDisconnect, which is misleading when the operation that failed was a connect or update.Proposed fix
} catch (error) { console.error('Failed to connect to Circuly:', error); toast({ title: isConnected ? t('integrations.updateFailed') : t('integrations.connectionTestFailed'), - description: t('integrations.failedToDisconnect', { + description: t('integrations.failedToConnect', { provider: 'Circuly', }), variant: 'destructive', });services/platform/app/hooks/use-locale.ts (1)
28-28:⚠️ Potential issue | 🟠 MajorFix default locale to match
config.ts.
useLocaledefaults to'en-US', but the canonicaldefaultLocaleinservices/platform/lib/i18n/config.tsis'en'. Callers that rely on the hook's default parameter will get inconsistent locale resolution. Import and use the config constant instead.Suggested fix
+import { defaultLocale } from '@/lib/i18n/config'; import { loadDayjsLocale } from '@/lib/utils/date/format'; import { isValidLocale } from '@/lib/utils/intl/is-valid-locale'; import { parseAcceptLanguage } from '@/lib/utils/intl/parse-accept-language'; import { resolveLocale } from '@/lib/utils/intl/resolve-locale'; -export function useLocale(defaultLocale = 'en-US') { +export function useLocale(fallbackLocale = defaultLocale) {services/platform/app/hooks/use-list-page.ts (1)
214-227: 🧹 Nitpick | 🔵 TrivialPrefetch logic looks solid.
The approach of checking
remainingAfterIncrement <= pageSizebefore the buffer is exhausted, combined with the 3× batch size, should significantly reduce perceived latency during scrolling.One observation:
handleLoadMoredepends ondataSource(the full object), which likely has a new identity on every render from the paginated query hook. This means the callback is recreated frequently. Consider destructuring the specific fields (dataSource.status,dataSource.loadMore) into stable references if you observe unnecessaryIntersectionObserverteardown/setup cycles downstream.services/platform/app/components/ui/data-table/column-builders.tsx (1)
258-264: 🧹 Nitpick | 🔵 TrivialPre-existing:
truncateoption is silently ignored whenclassNameis provided.When
options?.classNameis set, the nullish coalescing operator short-circuits, andoptions?.truncatehas no effect. This is pre-existing behavior, but worth noting iftruncateis expected to work independently ofclassName.♻️ Possible improvement
cell: ({ row }) => { const value = row.original[accessorKey]; const text = value ? String(value) : (options?.emptyText ?? '-'); return ( <span - className={ - options?.className ?? - `text-muted-foreground text-xs ${options?.truncate ? 'block max-w-sm truncate' : ''}` - } + className={cn( + options?.className ?? 'text-muted-foreground text-xs', + options?.truncate && 'block max-w-sm truncate', + )} > {text} </span> ); },
🤖 Fix all issues with AI agents
In `@services/platform/app/components/ui/forms/form-section.tsx`:
- Around line 22-37: The container in FormSection should be exposed as an
accessible group: import React's useId and generate an id (e.g., const id =
useId()) then set the root div (the one using cn('flex flex-col gap-3',
className)) to role="group" and conditionally add aria-labelledby={label ?
`${id}-label` : undefined} and aria-describedby={description ? `${id}-desc` :
undefined}; update the label span to have id={`${id}-label`} and the Description
component to have id={`${id}-desc`} so screen readers associate the
label/description with the children while preserving existing props (children,
className).
In `@services/platform/app/components/ui/forms/json-input.tsx`:
- Around line 391-393: The Description element is rendered without an id and not
linked to the interactive control, so create a descriptionId (derived from the
component id prop or generated) and pass it as the id prop to <Description>,
then add aria-describedby="{descriptionId}" to the interactive controls (the
<Textarea> used in this component and the JSON viewer container element) so
screen readers associate the description with the input; also update the prop
type for description from string to ReactNode to match other form components.
In `@services/platform/app/components/ui/forms/switch.tsx`:
- Around line 62-91: The two render paths render the switch on different sides;
make them consistent by changing the label-only branch to mirror the description
branch layout: render a container (e.g., <div className="flex items-center
justify-between">) with the Label (htmlFor={id}, required={required}) on the
left and the switchElement on the right, keeping the same className/cursor
behavior as the description branch and preserving accessibility IDs (id and
descriptionId) and props used in the switchElement so the visual position
doesn't flip when description is added or removed.
In
`@services/platform/app/features/automations/components/automation-navigation.tsx`:
- Around line 224-229: The loading skeleton is hidden on mobile because of
"hidden md:flex"; add a mobile-visible placeholder so users on small screens get
feedback: keep the existing desktop skeleton (the block using isLoading and
Skeleton) and add a separate element visible only on mobile (e.g., a div with
className "flex md:hidden" containing appropriately sized Skeletons) so the
action area shows a compact skeleton while isLoading is true; ensure the mobile
dropdown logic that currently checks !isLoading remains unchanged so the
dropdown still waits for load, but replace the hidden skeleton with a mobile
skeleton tied to the same isLoading condition (referencing isLoading and the
Skeleton components in automation-navigation.tsx).
In
`@services/platform/app/features/settings/integrations/components/circuly-integration-dialog.tsx`:
- Line 223: Replace the misuse of FormSection (which renders
label+description+children) with the simpler Description component: import
Description alongside other form atoms, change the JSX where FormSection is used
with only a description prop to instead render
<Description>{t('integrations.circuly.syncingData')}</Description>, and remove
the now-unused FormSection import if it’s no longer referenced (check the
circuly-integration-dialog.tsx component for other FormSection usages before
deleting).
In `@services/platform/app/routes/__root.tsx`:
- Around line 22-30: The head() meta property is using an unnecessary spread of
the seo() result; update the head implementation to assign meta: seo({...})
directly (replace the current meta: () => ({ meta: [...seo({ title: 'Tale',
description: '...'} )] }) pattern) so it matches other routes and avoids
creating a redundant array copy; modify the head function that references
seo(...) accordingly.
In `@services/platform/app/routes/dashboard/`$id/_knowledge/websites.tsx:
- Around line 50-61: The code in this route inlines the loading/filter checks
while other routes extract hasServerFilters and isInitialLoading for clarity;
refactor the conditional by creating named boolean variables (e.g.,
hasServerFilters = Boolean(search.status) or however other routes derive it, and
isInitialLoading = paginatedResult.status === 'LoadingFirstPage' &&
!search.status) and then use those variables in the early returns for
WebsitesEmptyState and WebsitesTableSkeleton (referencing paginatedResult,
search, count, organizationId, WebsitesEmptyState, and WebsitesTableSkeleton) so
the logic matches the customers/vendors/products pages for readability and
consistency.
In `@services/platform/app/routes/dashboard/`$id/automations/$amId.tsx:
- Around line 58-63: The head() currently calls seo() with hardcoded English
strings (title/description) which can't use i18n hooks; instead, make SEO
locale-aware by computing localized meta in the route loader (or route context)
and returning it as part of loaderData, then change head() to read those
localized values from loaderData (or route context) and pass them into seo() —
update the loader that corresponds to this route to expose the locale and
localized title/description and update head() to use those loaderData fields
rather than hardcoded strings (refer to head(), seo(), and the route loader).
In `@services/platform/app/routes/dashboard/`$id/settings/teams.tsx:
- Around line 40-51: TeamsContent currently passes organizationId but calls
useTeams() without it; change useTeams to accept an organizationId parameter
(mirror useApiKeys pattern) and update its implementation to use the passed
organizationId instead of calling useOrganizationId() internally, then update
TeamsContent to call useTeams(organizationId); also update any other call sites
of useTeams (or make the param optional for backwards compatibility) and ensure
components like TeamsTableSkeleton, TeamsEmptyState, and TeamsTable still
receive organizationId as before.
In `@services/platform/lib/utils/intl/parse-accept-language.test.ts`:
- Around line 54-57: The test name claims negatives are clamped but the regex in
parseAcceptLanguage (currently /^q\s*=\s*(\d+(?:\.\d+)?)$/) doesn't match a
leading '-' so "-0.1" is treated as unparseable and falls back to default 1;
either update parseAcceptLanguage to accept an optional leading sign and then
clamp the parsed quality to the 0–1 range (modify the quality parser inside
parseAcceptLanguage to use a regex like allowing an optional '-' and then coerce
via Math.max(0, Math.min(1, parsed))) or change the test description to reflect
that unparseable qualities default to 1; reference parseAcceptLanguage and the
regex when making the change.
In `@services/platform/lib/utils/intl/resolve-locale.test.ts`:
- Around line 24-26: The test name for the case using resolveLocale is
misleading: it claims to "fall back to en-US" but the assertion expects 'en'
because resolveLocale first checks isValidLocale('en') and returns it directly;
update the test to either (a) change the description to reflect that
resolveLocale returns 'en' for a bare 'en' input (e.g., "returns 'en' for bare
'en' when valid"), or (b) if you intend to assert the en-US fallback, adjust the
test setup so isValidLocale('en') is false (or remove 'en' from input) and then
assert 'en-US'; reference resolveLocale and isValidLocale when making the
change.
In `@services/platform/lib/utils/seo.ts`:
- Around line 1-21: The seo utility currently hardcodes og:type and omits
Twitter cards; update the SeoParams type and seo function to accept an optional
ogType (e.g., ogType?: string) and use it when pushing the { name: 'og:type',
content: ... } tag (fall back to 'website' if not provided), and add optional
Twitter meta tags (e.g., { name: 'twitter:card', content: 'summary_large_image'
}, { name: 'twitter:title', content: title }, and if description present { name:
'twitter:description', content: description }) into the tags array so both Open
Graph type and Twitter card metadata are supported.
In `@services/platform/server.js`:
- Around line 41-51: The injected Accept-Language (and envConfig) JSON is
vulnerable to XSS because JSON.stringify doesn't escape sequences like
"</script>" or "<!--"; update the replacement logic around indexHtmlTemplate so
after JSON.stringify(acceptLanguage) and JSON.stringify(envConfig) you run a
sanitizer that escapes dangerous HTML/script-breaking sequences (e.g. replace
"</" with "<\/" and "<!--" with "<\\!--" or similar) before inserting into the
window.__ACCEPT_LANGUAGE__ and window.__ENV__ replacements; keep the
sanitization helper local (e.g., escapeForScript) and use it when constructing
the two replacement strings to ensure the script context cannot be broken by
user-controlled headers.
In `@services/platform/vite-plugins/inject-accept-language.ts`:
- Around line 28-31: The injected Accept-Language value is
JSON.stringify(acceptLanguage) which does not escape the sequence "</script>",
allowing a dev-only self-XSS; to harden it, create a safe string from
JSON.stringify(acceptLanguage) that escapes any "</script>" sequences (e.g.,
replace "</script>" with "<\/script>" in the stringified value) and use that
safe string in the chunk.replace call (the replacement around injected /
chunk.replace and acceptLanguage).
| return ( | ||
| <div className={cn('flex flex-col gap-3', className)}> | ||
| {(label || description) && ( | ||
| <div className="flex flex-col gap-1"> | ||
| {label && ( | ||
| <span className="text-muted-foreground text-xs font-medium md:text-sm"> | ||
| {label} | ||
| </span> | ||
| )} | ||
| {description && ( | ||
| <Description className="text-xs">{description}</Description> | ||
| )} | ||
| </div> | ||
| )} | ||
| {children} | ||
| </div> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding role="group" with aria-labelledby for accessible grouping.
Since FormSection is meant to label a group of controls, screen readers won't associate the label/description with the children unless the container is marked as a group. A <fieldset>/<legend> or role="group" with aria-labelledby would improve the experience.
♿ Proposed accessibility improvement
export function FormSection({
label,
description,
children,
className,
}: FormSectionProps) {
+ const id = React.useId();
+ const labelId = `${id}-label`;
+ const descId = `${id}-desc`;
+
return (
- <div className={cn('flex flex-col gap-3', className)}>
+ <div
+ role="group"
+ aria-labelledby={label ? labelId : undefined}
+ aria-describedby={description ? descId : undefined}
+ className={cn('flex flex-col gap-3', className)}
+ >
{(label || description) && (
<div className="flex flex-col gap-1">
{label && (
- <span className="text-muted-foreground text-xs font-medium md:text-sm">
+ <span id={labelId} className="text-muted-foreground text-xs font-medium md:text-sm">
{label}
</span>
)}
{description && (
- <Description className="text-xs">{description}</Description>
+ <Description id={descId} className="text-xs">{description}</Description>
)}
</div>
)}
{children}
</div>
);
}This would require adding import React from 'react' (or import { useId } from 'react').
🤖 Prompt for AI Agents
In `@services/platform/app/components/ui/forms/form-section.tsx` around lines 22 -
37, The container in FormSection should be exposed as an accessible group:
import React's useId and generate an id (e.g., const id = useId()) then set the
root div (the one using cn('flex flex-col gap-3', className)) to role="group"
and conditionally add aria-labelledby={label ? `${id}-label` : undefined} and
aria-describedby={description ? `${id}-desc` : undefined}; update the label span
to have id={`${id}-label`} and the Description component to have
id={`${id}-desc`} so screen readers associate the label/description with the
children while preserving existing props (children, className).
| {description && ( | ||
| <p className="text-muted-foreground text-xs">{description}</p> | ||
| <Description className="text-xs">{description}</Description> | ||
| )} |
There was a problem hiding this comment.
Missing aria-describedby linkage for the description.
Other form components in this PR (Checkbox, Input, Textarea) generate a descriptionId, pass it as id to the <Description> element, and wire aria-describedby on the interactive control. This component renders the Description without an id and without linking it to the <Textarea> or JSON viewer, so screen readers won't associate the description with the input.
Additionally, description is typed as string here (line 40) while the other form components type it as ReactNode — consider aligning for consistency.
🛡️ Proposed fix
Generate a descriptionId (e.g., alongside the existing id prop) and wire it through:
export function JsonInput({
...
id,
}: JsonInputProps) {
+ const descriptionId = id ? `${id}-description` : undefined;
...Then on the <Textarea> (line 300–323) and the JSON viewer container, add aria-describedby:
<Textarea
ref={textareaRef}
+ aria-describedby={description && descriptionId ? descriptionId : undefined}
...
/>And pass the id to the Description:
- <Description className="text-xs">{description}</Description>
+ <Description id={descriptionId} className="text-xs">{description}</Description>🤖 Prompt for AI Agents
In `@services/platform/app/components/ui/forms/json-input.tsx` around lines 391 -
393, The Description element is rendered without an id and not linked to the
interactive control, so create a descriptionId (derived from the component id
prop or generated) and pass it as the id prop to <Description>, then add
aria-describedby="{descriptionId}" to the interactive controls (the <Textarea>
used in this component and the JSON viewer container element) so screen readers
associate the description with the input; also update the prop type for
description from string to ReactNode to match other form components.
| if (description) { | ||
| return ( | ||
| <div className="flex flex-col gap-1.5"> | ||
| <div className="flex items-center justify-between"> | ||
| {label && ( | ||
| <Label | ||
| htmlFor={id} | ||
| required={required} | ||
| className="cursor-pointer" | ||
| > | ||
| {label} | ||
| </Label> | ||
| )} | ||
| {switchElement} | ||
| </div> | ||
| <Description id={descriptionId} className="text-xs"> | ||
| {description} | ||
| </Description> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <div className="flex items-center gap-2"> | ||
| {switchElement} | ||
| <Label htmlFor={id} required={required} className="cursor-pointer"> | ||
| {label} | ||
| </Label> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent switch position relative to label across rendering paths.
When description is present (line 62 branch), the layout is justify-between placing the label on the left and switch on the right. When only label is present (line 84 branch), the switch is rendered before the label (left side). This means adding or removing a description prop flips the switch's visual position, which could be jarring in forms that mix both variants.
Consider aligning the two layouts so the switch is consistently on the same side of the label:
Proposed fix — label-only layout matches the description layout
return (
- <div className="flex items-center gap-2">
- {switchElement}
- <Label htmlFor={id} required={required} className="cursor-pointer">
+ <div className="flex items-center justify-between">
+ <Label htmlFor={id} required={required} className="cursor-pointer">
{label}
</Label>
+ {switchElement}
</div>
);🤖 Prompt for AI Agents
In `@services/platform/app/components/ui/forms/switch.tsx` around lines 62 - 91,
The two render paths render the switch on different sides; make them consistent
by changing the label-only branch to mirror the description branch layout:
render a container (e.g., <div className="flex items-center justify-between">)
with the Label (htmlFor={id}, required={required}) on the left and the
switchElement on the right, keeping the same className/cursor behavior as the
description branch and preserving accessibility IDs (id and descriptionId) and
props used in the switchElement so the visual position doesn't flip when
description is added or removed.
| {isLoading && ( | ||
| <div className="hidden items-center gap-2 md:flex"> | ||
| <Skeleton className="h-8 w-12 rounded-md" /> | ||
| <Skeleton className="h-8 w-20 rounded-md" /> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Loading skeleton is hidden on mobile.
The skeleton placeholder (Lines 225-228) uses hidden md:flex, so mobile users see no loading indicator in the actions area while isLoading is true. Meanwhile, the mobile dropdown (Line 303) is also gated by !isLoading. Consider adding a mobile-visible skeleton or at minimum a small placeholder so mobile users get visual feedback.
🤖 Prompt for AI Agents
In
`@services/platform/app/features/automations/components/automation-navigation.tsx`
around lines 224 - 229, The loading skeleton is hidden on mobile because of
"hidden md:flex"; add a mobile-visible placeholder so users on small screens get
feedback: keep the existing desktop skeleton (the block using isLoading and
Skeleton) and add a separate element visible only on mobile (e.g., a div with
className "flex md:hidden" containing appropriately sized Skeletons) so the
action area shows a compact skeleton while isLoading is true; ensure the mobile
dropdown logic that currently checks !isLoading remains unchanged so the
dropdown still waits for load, but replace the hidden skeleton with a mobile
skeleton tied to the same isLoading condition (referencing isLoading and the
Skeleton components in automation-navigation.tsx).
| <Description className="text-xs"> | ||
| {t('integrations.circuly.syncingData')} | ||
| </Description> | ||
| <FormSection description={t('integrations.circuly.syncingData')} /> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
FormSection used as a standalone description wrapper (no label or children).
FormSection is a container that renders a label, description header, and then {children}. Using it with only description and no children or label works but feels like a misuse of the component's intent. A plain <Description> would be more semantically appropriate here.
Proposed fix
Import Description directly:
-import { FormSection } from '@/app/components/ui/forms/form-section';
+import { Description } from '@/app/components/ui/forms/description';Then at line 223:
- <FormSection description={t('integrations.circuly.syncingData')} />
+ <Description className="text-xs">
+ {t('integrations.circuly.syncingData')}
+ </Description>🤖 Prompt for AI Agents
In
`@services/platform/app/features/settings/integrations/components/circuly-integration-dialog.tsx`
at line 223, Replace the misuse of FormSection (which renders
label+description+children) with the simpler Description component: import
Description alongside other form atoms, change the JSX where FormSection is used
with only a description prop to instead render
<Description>{t('integrations.circuly.syncingData')}</Description>, and remove
the now-unused FormSection import if it’s no longer referenced (check the
circuly-integration-dialog.tsx component for other FormSection usages before
deleting).
| it('clamps quality to 0–1 range', () => { | ||
| const result = parseAcceptLanguage('en;q=1.5, de;q=-0.1'); | ||
| expect(result).toEqual(['en', 'de']); | ||
| }); |
There was a problem hiding this comment.
Negative quality value isn't actually clamped — it's silently ignored by the regex.
The regex in parseAcceptLanguage (/^q\s*=\s*(\d+(?:\.\d+)?)$/) doesn't match negative values like -0.1, so de;q=-0.1 falls through to the default quality of 1 rather than being clamped to 0. The test still passes, but it's testing "unparseable quality defaults to 1" rather than "negative quality is clamped to 0". Consider adding a comment to clarify the actual behavior, or adjusting the test name.
🤖 Prompt for AI Agents
In `@services/platform/lib/utils/intl/parse-accept-language.test.ts` around lines
54 - 57, The test name claims negatives are clamped but the regex in
parseAcceptLanguage (currently /^q\s*=\s*(\d+(?:\.\d+)?)$/) doesn't match a
leading '-' so "-0.1" is treated as unparseable and falls back to default 1;
either update parseAcceptLanguage to accept an optional leading sign and then
clamp the parsed quality to the 0–1 range (modify the quality parser inside
parseAcceptLanguage to use a regex like allowing an optional '-' and then coerce
via Math.max(0, Math.min(1, parsed))) or change the test description to reflect
that unparseable qualities default to 1; reference parseAcceptLanguage and the
regex when making the change.
| it('falls back to en-US for bare "en" via en fallback', () => { | ||
| expect(resolveLocale(['en'], 'de-DE')).toBe('en'); | ||
| }); |
There was a problem hiding this comment.
Misleading test description — the assertion contradicts the name.
The test is named 'falls back to en-US for bare "en" via en fallback' but asserts the result is 'en', not 'en-US'. If isValidLocale('en') returns true, the function returns 'en' directly on the first check (line 16 of resolve-locale.ts) and never reaches the en-US fallback branch. Either the description should be updated to reflect what's actually being tested, or the assertion is wrong.
Suggested description fix
- it('falls back to en-US for bare "en" via en fallback', () => {
+ it('resolves bare "en" directly when it is a valid locale', () => {
expect(resolveLocale(['en'], 'de-DE')).toBe('en');
});🤖 Prompt for AI Agents
In `@services/platform/lib/utils/intl/resolve-locale.test.ts` around lines 24 -
26, The test name for the case using resolveLocale is misleading: it claims to
"fall back to en-US" but the assertion expects 'en' because resolveLocale first
checks isValidLocale('en') and returns it directly; update the test to either
(a) change the description to reflect that resolveLocale returns 'en' for a bare
'en' input (e.g., "returns 'en' for bare 'en' when valid"), or (b) if you intend
to assert the en-US fallback, adjust the test setup so isValidLocale('en') is
false (or remove 'en' from input) and then assert 'en-US'; reference
resolveLocale and isValidLocale when making the change.
| interface SeoParams { | ||
| title: string; | ||
| description?: string; | ||
| } | ||
|
|
||
| export function seo({ title, description }: SeoParams) { | ||
| const tags: Array<Record<string, string>> = [ | ||
| { title }, | ||
| { name: 'og:title', content: title }, | ||
| { name: 'og:type', content: 'website' }, | ||
| ]; | ||
|
|
||
| if (description) { | ||
| tags.push( | ||
| { name: 'description', content: description }, | ||
| { name: 'og:description', content: description }, | ||
| ); | ||
| } | ||
|
|
||
| return tags; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clean utility — consider a few SEO enhancements for completeness.
The implementation is correct and works well with TanStack Router's head API. A couple of optional improvements:
og:typeis hardcoded to'website'— fine for most pages, but you may want to make it configurable if you later have blog/article pages.- Consider adding
twitter:card/twitter:title/twitter:descriptionmeta tags for better social sharing coverage.
🤖 Prompt for AI Agents
In `@services/platform/lib/utils/seo.ts` around lines 1 - 21, The seo utility
currently hardcodes og:type and omits Twitter cards; update the SeoParams type
and seo function to accept an optional ogType (e.g., ogType?: string) and use it
when pushing the { name: 'og:type', content: ... } tag (fall back to 'website'
if not provided), and add optional Twitter meta tags (e.g., { name:
'twitter:card', content: 'summary_large_image' }, { name: 'twitter:title',
content: title }, and if description present { name: 'twitter:description',
content: description }) into the tags array so both Open Graph type and Twitter
card metadata are supported.
| const acceptLanguage = req.headers['accept-language'] || ''; | ||
|
|
||
| const html = indexHtmlTemplate | ||
| .replace( | ||
| /window\.__ENV__\s*=\s*['"]__ENV_PLACEHOLDER__['"];/, | ||
| `window.__ENV__ = ${JSON.stringify(envConfig)};`, | ||
| ) | ||
| .replace( | ||
| /window\.__ACCEPT_LANGUAGE__\s*=\s*['"]__ACCEPT_LANGUAGE_PLACEHOLDER__['"];/, | ||
| `window.__ACCEPT_LANGUAGE__ = ${JSON.stringify(acceptLanguage)};`, | ||
| ); |
There was a problem hiding this comment.
XSS risk: JSON.stringify does not escape </script> sequences.
An attacker can send a crafted Accept-Language header containing </script><script>alert(1)</script>. JSON.stringify escapes quotes and backslashes but does not escape </ or <!--, so the injected value can break out of the <script> tag context.
The same risk applies to envConfig on line 46, though that is derived from server-controlled env vars, making exploitation less likely. The Accept-Language header is fully client-controlled.
🔒 Proposed fix: sanitize JSON output for HTML script context
Add a helper that escapes dangerous sequences after JSON.stringify:
+function htmlSafeJsonStringify(value) {
+ return JSON.stringify(value)
+ .replace(/</g, '\\u003c')
+ .replace(/>/g, '\\u003e')
+ .replace(/&/g, '\\u0026')
+ .replace(/'/g, '\\u0027');
+}
+
app.get('{*path}', (req, res) => {
// ...
const html = indexHtmlTemplate
.replace(
/window\.__ENV__\s*=\s*['"]__ENV_PLACEHOLDER__['"];/,
- `window.__ENV__ = ${JSON.stringify(envConfig)};`,
+ `window.__ENV__ = ${htmlSafeJsonStringify(envConfig)};`,
)
.replace(
/window\.__ACCEPT_LANGUAGE__\s*=\s*['"]__ACCEPT_LANGUAGE_PLACEHOLDER__['"];/,
- `window.__ACCEPT_LANGUAGE__ = ${JSON.stringify(acceptLanguage)};`,
+ `window.__ACCEPT_LANGUAGE__ = ${htmlSafeJsonStringify(acceptLanguage)};`,
);🤖 Prompt for AI Agents
In `@services/platform/server.js` around lines 41 - 51, The injected
Accept-Language (and envConfig) JSON is vulnerable to XSS because JSON.stringify
doesn't escape sequences like "</script>" or "<!--"; update the replacement
logic around indexHtmlTemplate so after JSON.stringify(acceptLanguage) and
JSON.stringify(envConfig) you run a sanitizer that escapes dangerous
HTML/script-breaking sequences (e.g. replace "</" with "<\/" and "<!--" with
"<\\!--" or similar) before inserting into the window.__ACCEPT_LANGUAGE__ and
window.__ENV__ replacements; keep the sanitization helper local (e.g.,
escapeForScript) and use it when constructing the two replacement strings to
ensure the script context cannot be broken by user-controlled headers.
| const injected = chunk.replace( | ||
| "'__ACCEPT_LANGUAGE_PLACEHOLDER__'", | ||
| JSON.stringify(acceptLanguage), | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor XSS vector via </script> in Accept-Language header (dev-only, self-XSS).
JSON.stringify doesn't escape forward slashes, so a crafted Accept-Language value containing </script> would break out of the inline <script> tag in the HTML. Since this is dev-only and the header originates from the developer's own browser, the risk is negligible (self-XSS). However, if you want defense-in-depth, you can escape it:
Optional hardening
const injected = chunk.replace(
"'__ACCEPT_LANGUAGE_PLACEHOLDER__'",
- JSON.stringify(acceptLanguage),
+ JSON.stringify(acceptLanguage).replace(/</g, '\\u003c'),
);🤖 Prompt for AI Agents
In `@services/platform/vite-plugins/inject-accept-language.ts` around lines 28 -
31, The injected Accept-Language value is JSON.stringify(acceptLanguage) which
does not escape the sequence "</script>", allowing a dev-only self-XSS; to
harden it, create a safe string from JSON.stringify(acceptLanguage) that escapes
any "</script>" sequences (e.g., replace "</script>" with "<\/script>" in the
stringified value) and use that safe string in the chunk.replace call (the
replacement around injected / chunk.replace and acceptLanguage).
1 participant
Summary by CodeRabbit
New Features
Improvements