Repository navigation
Common UI components refactoring & accesibility improvements - #525
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughCentralizes Button styling into a new variant/size API and Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Blaumaus
marked this pull request as ready for review
April 30, 2026 02:11
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (5)
web/app/ui/Pagination.tsx (2)
132-143: ⚡ Quick winAvoid duplicating the DOTS jump-page calculation.
middlePageis computed inonClickand recomputed again foraria-label. Extract it once and reuse it so click behavior and announced target cannot drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/ui/Pagination.tsx` around lines 132 - 143, The DOTS jump computes the same middle page twice which can drift; compute the middle page once and reuse it for both the click handler and aria-label. In the Pagination component, extract the calculation of middlePage (from paginationRange[index - 1] and paginationRange[index + 1]) into a single const (e.g., middlePage) in the surrounding scope for that DOTS element, then call setPage(middlePage) in the onClick and use that same middlePage value in the aria-label string; update references to paginationRange, setPage, onClick, and aria-label accordingly so they share the single computed middlePage.
111-112: 🏗️ Heavy liftLocalize the new ARIA labels instead of hardcoding English.
Line 111 (
'Pagination'), Line 142 (Jump to page ...), and Line 157 (Page ...) bypasst(...). This creates mixed-language screen reader output in localized UIs.Also applies to: 142-143, 157-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/ui/Pagination.tsx` around lines 111 - 112, The ARIA labels in the Pagination component are hardcoded English strings; replace the literals "'Pagination'", "Jump to page ..." and "Page ..." with localized strings via the existing i18n translator (e.g., call t('pagination.ariaLabel') for the container, t('pagination.jumpTo', { page }) for the jump input aria-label, and t('pagination.page', { page }) for page labels). Update the import/use of the translator in the Pagination component (e.g., useTranslation or t from the project's i18n helper) and add appropriate i18n keys with interpolation for the page number (pagination.ariaLabel, pagination.jumpTo, pagination.page) so screen readers receive localized text.web/app/pages/UserSettings/components/Organisations.tsx (1)
105-113: ⚡ Quick winGive reject a destructive variant for safer UX.
At Line 106 and Line 112, reject and accept both appear primary-styled. Adding a destructive variant to reject preserves action contrast.
Proposed tweak
<Button + variant='danger-outline' size='xs' className='mr-2' onClick={() => setShowDeleteModal(true)} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/pages/UserSettings/components/Organisations.tsx` around lines 105 - 113, The "reject" Button currently uses the same primary styling as "accept"; update the first Button (the one that calls setShowDeleteModal(true)) to use a destructive variant (e.g., add variant="destructive") so the reject action is visually distinct and safer; leave the accept Button (onAccept, isLoading) styled as primary or unchanged. This change targets the Button element that calls setShowDeleteModal and the nearby onAccept/isLoading usage for context.web/app/pages/UserSettings/components/ProjectList.tsx (1)
103-112: ⚡ Quick winKeep reject action visually distinct from accept.
At Line 104 and Line 111, both actions render as primary-styled buttons. This weakens destructive-action affordance and increases misclick risk.
Proposed tweak
- <Button - size='xs' - className='mr-2' - onClick={() => setShowDeleteModal(true)} - loading={isPending} - > + <Button + variant='danger-outline' + size='xs' + className='mr-2' + onClick={() => setShowDeleteModal(true)} + loading={isPending} + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/pages/UserSettings/components/ProjectList.tsx` around lines 103 - 112, The reject and accept buttons in ProjectList are visually similar; change the reject button (the one that calls setShowDeleteModal(true) and uses isPending) to a distinct destructive/secondary style (e.g., use the component's danger/secondary/ghost variant or a different color prop) while leaving the accept button (onAccept, isPending) as primary; ensure any className or size props remain but update the Button variant/intent for the reject action so it clearly contrasts with the accept action.web/app/ui/StatusPage.tsx (1)
68-75: ⚡ Quick winUse the shared
buttonClasses()helper here.
web/app/ui/Button.tsxalready exportsbuttonClasses()for exactly this case. Re-encoding the primary/secondary button styles inStatusPagemeans this page can drift the next time the Button API or focus treatment changes.♻️ Proposed refactor
import { Link } from '~/ui/Link' +import { buttonClasses } from '~/ui/Button' import { cn } from '~/utils/generic' @@ -const baseActionClasses = - 'inline-flex items-center justify-center rounded-md px-4 py-2 text-sm font-medium transition-[background-color,color,transform] duration-150 ease-out active:scale-[0.985] focus-visible:ring-2 focus-visible:ring-offset-2 focus-visible:outline-hidden' - -const primaryActionClasses = - 'bg-slate-900 text-white shadow-xs hover:bg-slate-700 focus-visible:ring-slate-900 dark:bg-slate-100 dark:text-slate-900 dark:hover:bg-white dark:focus-visible:ring-slate-300 dark:focus-visible:ring-offset-slate-950' - -const secondaryActionClasses = - 'bg-white text-gray-700 ring-1 ring-inset ring-gray-300 hover:bg-gray-50 focus-visible:ring-slate-900 dark:bg-slate-900 dark:text-gray-200 dark:ring-slate-700/80 dark:hover:bg-slate-800 dark:focus-visible:ring-slate-300 dark:focus-visible:ring-offset-slate-950' - @@ - className={cn( - baseActionClasses, - action.primary - ? primaryActionClasses - : secondaryActionClasses, - )} + className={buttonClasses({ + variant: action.primary ? 'primary' : 'secondary', + })} @@ - className={cn( - baseActionClasses, - action.primary - ? primaryActionClasses - : secondaryActionClasses, - )} + className={buttonClasses({ + variant: action.primary ? 'primary' : 'secondary', + })}Also applies to: 145-149, 160-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/ui/StatusPage.tsx` around lines 68 - 75, StatusPage.tsx duplicates button styling via baseActionClasses, primaryActionClasses, and secondaryActionClasses; replace those duplicated class strings by importing and using the shared buttonClasses() helper from Button.tsx (call buttonClasses({ variant: 'primary' }) and buttonClasses({ variant: 'secondary' }) or the appropriate API) and remove the three constants; update all usages (including the other occurrences noted around lines ~145-149 and ~160-163) to call buttonClasses() so the page stays in sync with the central Button styles and focus behaviors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/app/pages/Project/Settings/Alerts/ProjectAlertsView.tsx`:
- Around line 427-451: The no-permission fallback in ProjectAlertsView currently
shows a signup CTA even when an authenticated non-owner hits the block; update
the rendering logic around the Link that uses routes.signup so it only renders
the signup CTA for unauthenticated users (e.g., check the app's auth state such
as currentUser or isAuthenticated from context/props) and for authenticated
users replace it with a more appropriate action (like a disabled button, an
upgrade/ownership info link, or nothing). Locate the block guarded by
canManageAlerts and conditionally render the Link to routes.signup based on the
auth check, leaving the rest of the markup (icons/text) unchanged.
In `@web/app/pages/Project/View/components/RefreshStatsButton.tsx`:
- Around line 100-105: The Button disabled prop only checks authLoading but
handleManualRefresh also no-ops when dataLoading or isRefreshing; update the
Button in RefreshStatsButton to set disabled to (authLoading || dataLoading ||
isRefreshing) so the UI matches the click guard in handleManualRefresh and
prevents a clickable control that does nothing.
In `@web/app/ui/Combobox.tsx`:
- Around line 69-72: The hardcoded aria-label on the ComboboxButton ("Toggle
options") needs to be replaced with a localized string; update the
ComboboxButton in the Combobox component to use the app's i18n translation
(e.g., call the existing translation hook/function used in this file such as t
or useTranslation) and pass a translation key like "combobox.toggleOptions" (or
the matching key in your locale files) instead of the hardcoded text so the
aria-label is localized for assistive technologies.
In `@web/app/ui/FileUpload.tsx`:
- Around line 119-124: The icon wrapper still includes group-hover styles even
when the dropzone is disabled; update the className construction in
FileUpload.tsx (the JSX element using cn with dragOver) to include the
group-hover:* classes only when !isDisabled, i.e., conditionally append the
hover-related class strings behind a !isDisabled check so that when isDisabled
is true no group-hover styles are applied; reference the className usage, the
dragOver variable and isDisabled prop to locate and change the conditional
strings.
In `@web/app/ui/Input.tsx`:
- Around line 95-101: The password-visibility toggle button still responds to
clicks when the input is disabled/readOnly; update the button in Input.tsx (the
element using onClick, aria-pressed, tabIndex) to be non-interactive when
disabled or readOnly by adding disabled={disabled || readOnly} and
aria-disabled={disabled || readOnly}, and guard the onClick handler to return
early when disabled || readOnly before calling setShowPassword(!showPassword);
this ensures the toggle cannot be activated while the input is
disabled/readOnly.
In `@web/app/ui/MultiProgress.tsx`:
- Around line 23-29: The MultiProgress component currently sets
role='progressbar' on the wrapper but exposes no accessible name or value;
either remove the role to make the bar purely decorative or add props to surface
ARIA metadata so callers can supply semantics. Update MultiProgress to accept
optional props such as aria-label, aria-labelledby, aria-valuenow,
aria-valuemin, aria-valuemax (and/or an isDecorative boolean) and only render
role='progressbar' when a name/value are provided; otherwise omit role (or set
aria-hidden) so screen readers aren't given an unlabeled control. Ensure the
change is applied where role and className are set and document the new props in
the component API.
In `@web/app/ui/MultiSelect.tsx`:
- Around line 168-178: The listbox div (identified by id={listboxId},
role='listbox' and controlled by isOpen) is only opacity-hidden and remains in
the accessibility tree and tabbable when closed; change the render so the
listbox is removed from the DOM when closed (either by conditional rendering
based on isOpen or by replacing the wrapper with a transition component that
unmounts after exit) so it cannot receive Tab focus while collapsed; apply the
same change to the other popup block that uses isOpen (the similar wrapper
around lines 190-198) to ensure both popups are unmounted when closed and keep
any close animation by using an unmount-on-exit transition.
In `@web/app/ui/PasswordStrength.tsx`:
- Around line 85-94: The progressbar is missing aria-valuetext so screen readers
only get a number; update the PasswordStrength component to set aria-valuetext
to the human-readable strength label (the same localized label shown visually)
instead of only numeric state. Use the existing strength label source (e.g. the
value derived from currentConfig such as currentConfig.label or the localized
string returned by t(...)) and pass it into the div as
aria-valuetext={strengthLabel} while keeping
aria-valuenow={currentConfig.segments} so assistive tech reads both the number
and the textual strength.
In `@web/app/ui/ProgressRing.tsx`:
- Around line 94-99: The progressbar can be rendered without an accessible name
and announces completion early because aria-label is optional and aria-valuenow
uses the rounded value; in ProgressRing update the accessibility attributes so
aria-label always has a fallback (e.g., use ariaLabel || `Progress:
${Math.floor(value)}%` or similar descriptive string) and set aria-valuenow to a
non-inflated numeric value (use the raw/clamped value or Math.floor(value)
instead of rounded) to avoid prematurely announcing 100; apply the same fix for
the other instance referenced (the attributes around lines 132-133) and keep
references to the ProgressRing props (value, rounded, ariaLabel) when making the
change.
In `@web/app/ui/Switch.tsx`:
- Around line 53-67: Thumb contrast in dark mode is lost because the thumb's
static classes ('pointer-events-none inline-block size-3 ... bg-white') always
set it white while the checked dark-mode track uses a light color; update the
thumb class logic in Switch.tsx (the JSX element using the 'pointer-events-none
inline-block ...' class and the checked boolean) to conditionally apply a
dark-mode thumb color when checked (e.g., add a conditional 'dark:...' class or
use cx to apply a different thumb background when checked) so the thumb
contrasts against the checked dark-mode track and maintains visible state.
---
Nitpick comments:
In `@web/app/pages/UserSettings/components/Organisations.tsx`:
- Around line 105-113: The "reject" Button currently uses the same primary
styling as "accept"; update the first Button (the one that calls
setShowDeleteModal(true)) to use a destructive variant (e.g., add
variant="destructive") so the reject action is visually distinct and safer;
leave the accept Button (onAccept, isLoading) styled as primary or unchanged.
This change targets the Button element that calls setShowDeleteModal and the
nearby onAccept/isLoading usage for context.
In `@web/app/pages/UserSettings/components/ProjectList.tsx`:
- Around line 103-112: The reject and accept buttons in ProjectList are visually
similar; change the reject button (the one that calls setShowDeleteModal(true)
and uses isPending) to a distinct destructive/secondary style (e.g., use the
component's danger/secondary/ghost variant or a different color prop) while
leaving the accept button (onAccept, isPending) as primary; ensure any className
or size props remain but update the Button variant/intent for the reject action
so it clearly contrasts with the accept action.
In `@web/app/ui/Pagination.tsx`:
- Around line 132-143: The DOTS jump computes the same middle page twice which
can drift; compute the middle page once and reuse it for both the click handler
and aria-label. In the Pagination component, extract the calculation of
middlePage (from paginationRange[index - 1] and paginationRange[index + 1]) into
a single const (e.g., middlePage) in the surrounding scope for that DOTS
element, then call setPage(middlePage) in the onClick and use that same
middlePage value in the aria-label string; update references to paginationRange,
setPage, onClick, and aria-label accordingly so they share the single computed
middlePage.
- Around line 111-112: The ARIA labels in the Pagination component are hardcoded
English strings; replace the literals "'Pagination'", "Jump to page ..." and
"Page ..." with localized strings via the existing i18n translator (e.g., call
t('pagination.ariaLabel') for the container, t('pagination.jumpTo', { page })
for the jump input aria-label, and t('pagination.page', { page }) for page
labels). Update the import/use of the translator in the Pagination component
(e.g., useTranslation or t from the project's i18n helper) and add appropriate
i18n keys with interpolation for the page number (pagination.ariaLabel,
pagination.jumpTo, pagination.page) so screen readers receive localized text.
In `@web/app/ui/StatusPage.tsx`:
- Around line 68-75: StatusPage.tsx duplicates button styling via
baseActionClasses, primaryActionClasses, and secondaryActionClasses; replace
those duplicated class strings by importing and using the shared buttonClasses()
helper from Button.tsx (call buttonClasses({ variant: 'primary' }) and
buttonClasses({ variant: 'secondary' }) or the appropriate API) and remove the
three constants; update all usages (including the other occurrences noted around
lines ~145-149 and ~160-163) to call buttonClasses() so the page stays in sync
with the central Button styles and focus behaviors.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7d11e94-0329-4ad4-b890-369a4973b8bc
📒 Files selected for processing (92)
web/app/components/GithubAuth.tsxweb/app/components/GoogleAuth.tsxweb/app/components/NotificationChannels/EnableWebPushButton.tsxweb/app/components/NotificationChannels/NotificationChannels.tsxweb/app/components/OIDCAuth.tsxweb/app/components/pricing/BillingPricing.tsxweb/app/modals/AnnotationModal.tsxweb/app/modals/NewFunnel.tsxweb/app/pages/Auth/CreateNewPassword/CreateNewPassword.tsxweb/app/pages/Auth/ForgotPassword/ForgotPassword.tsxweb/app/pages/Auth/Signin/Signin.tsxweb/app/pages/Auth/Signup/InvitationSignup.tsxweb/app/pages/Auth/Signup/Signup.tsxweb/app/pages/Captcha/CaptchaDemo.tsxweb/app/pages/Captcha/index.tsxweb/app/pages/Dashboard/Dashboard.tsxweb/app/pages/Dashboard/NoProjects.tsxweb/app/pages/Dashboard/PeriodSelector.tsxweb/app/pages/Dashboard/SortSelector.tsxweb/app/pages/Onboarding/Onboarding.tsxweb/app/pages/Organisations/Organisations.tsxweb/app/pages/Organisations/Settings/People.tsxweb/app/pages/Organisations/Settings/Projects.tsxweb/app/pages/Organisations/Settings/index.tsxweb/app/pages/Project/Settings/Alerts/AlertTemplateEditor.tsxweb/app/pages/Project/Settings/Alerts/ProjectAlertsSettings.tsxweb/app/pages/Project/Settings/Alerts/ProjectAlertsView.tsxweb/app/pages/Project/Settings/Annotations.tsxweb/app/pages/Project/Settings/Emails/Emails.tsxweb/app/pages/Project/Settings/People/People.tsxweb/app/pages/Project/Settings/ProjectSettings.tsxweb/app/pages/Project/Settings/components/DataImportTab.tsxweb/app/pages/Project/Settings/components/ProxyDomainsTab.tsxweb/app/pages/Project/Settings/tabs/DangerZone.tsxweb/app/pages/Project/Settings/tabs/Revenue.tsxweb/app/pages/Project/View/ViewProject.tsxweb/app/pages/Project/View/components/ChartTypeSwitcher.tsxweb/app/pages/Project/View/components/DashboardHeader.tsxweb/app/pages/Project/View/components/PasswordRequiredModal.tsxweb/app/pages/Project/View/components/RefreshStatsButton.tsxweb/app/pages/Project/tabs/AskAI/AskAIView.tsxweb/app/pages/Project/tabs/Errors/ErrorDetails.tsxweb/app/pages/Project/tabs/Experiments/ExperimentResults.tsxweb/app/pages/Project/tabs/Experiments/ExperimentSettingsModal.tsxweb/app/pages/Project/tabs/Experiments/ExperimentsView.tsxweb/app/pages/Project/tabs/FeatureFlags/FeatureFlagSettingsModal.tsxweb/app/pages/Project/tabs/FeatureFlags/FeatureFlagsView.tsxweb/app/pages/Project/tabs/Funnels/FunnelsView.tsxweb/app/pages/Project/tabs/Goals/GoalSettingsModal.tsxweb/app/pages/Project/tabs/Goals/GoalsView.tsxweb/app/pages/Project/tabs/Profiles/ProfileDetails.tsxweb/app/pages/Subscribe/Subscribe.tsxweb/app/pages/UserSettings/UserSettings.tsxweb/app/pages/UserSettings/components/Organisations.tsxweb/app/pages/UserSettings/components/ProjectList.tsxweb/app/pages/UserSettings/components/Socialisations.tsxweb/app/pages/UserSettings/components/TwoFA.tsxweb/app/routes/tools.conversion-rate-calculator.tsxweb/app/routes/tools.ctr-calculator.tsxweb/app/routes/tools.ip-lookup.tsxweb/app/routes/tools.percentage-calculator.tsxweb/app/routes/tools.qr-code-generator.tsxweb/app/routes/tools.robots-txt-generator.tsxweb/app/routes/tools.schema-markup-generator.tsxweb/app/routes/tools.sitemap-validator.tsxweb/app/routes/tools.utm-generator.tsxweb/app/ui/Alert.tsxweb/app/ui/Badge.tsxweb/app/ui/Button.tsxweb/app/ui/Checkbox.tsxweb/app/ui/CodeBlock.tsxweb/app/ui/Combobox.tsxweb/app/ui/Drawer.tsxweb/app/ui/Dropdown.tsxweb/app/ui/FAQ.tsxweb/app/ui/FileUpload.tsxweb/app/ui/Input.tsxweb/app/ui/Loader.tsxweb/app/ui/LoadingBar.tsxweb/app/ui/Modal.tsxweb/app/ui/MultiProgress.tsxweb/app/ui/MultiSelect.tsxweb/app/ui/Pagination.tsxweb/app/ui/PasswordStrength.tsxweb/app/ui/ProgressRing.tsxweb/app/ui/RadioCardGroup.tsxweb/app/ui/Select.tsxweb/app/ui/StatusPage.tsxweb/app/ui/Switch.tsxweb/app/ui/Text.tsxweb/app/ui/Textarea.tsxweb/app/ui/Tooltip.tsx
💤 Files with no reviewable changes (4)
- web/app/routes/tools.ip-lookup.tsx
- web/app/pages/Project/tabs/Profiles/ProfileDetails.tsx
- web/app/routes/tools.qr-code-generator.tsx
- web/app/routes/tools.sitemap-validator.tsx
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/app/pages/Project/View/components/RefreshStatsButton.tsx (1)
55-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnsure refresh state always resets on failed refresh calls.
Both auto and manual paths can leave
isRefreshingstucktrueifonRefresh(...)throws, and the auto path can also produce an unhandled rejected promise. Wrap both awaits withtry/finally(and catch/log in auto path) so UI state is always restored.Suggested fix
if ( newProgress >= 100 && !hasTriggeredRefresh.current && !isRefreshing && !authLoading && !dataLoading ) { hasTriggeredRefresh.current = true ;(async () => { setIsRefreshing(true) - await onRefresh(false) - setIsRefreshing(false) - // Reset timer after refresh completes - startTimeRef.current = Date.now() - hasTriggeredRefresh.current = false - setProgress(0) + try { + await onRefresh(false) + } catch (error) { + console.error(error) + } finally { + setIsRefreshing(false) + // Reset timer after refresh completes + startTimeRef.current = Date.now() + hasTriggeredRefresh.current = false + setProgress(0) + } })() } @@ const handleManualRefresh = async () => { if (authLoading || dataLoading || isRefreshing) return setIsRefreshing(true) startTimeRef.current = Date.now() hasTriggeredRefresh.current = false setProgress(0) - await onRefresh(true) - setIsRefreshing(false) + try { + await onRefresh(true) + } finally { + setIsRefreshing(false) + } }Also applies to: 74-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/pages/Project/View/components/RefreshStatsButton.tsx` around lines 55 - 63, The refresh logic can leave isRefreshing stuck true and produce unhandled rejections if onRefresh throws; wrap the await calls to onRefresh(false) (manual path inside the IIFE) and the auto-refresh await (the block around lines using startTimeRef/hasTriggeredRefresh/setProgress) in try/finally so setIsRefreshing(false), startTimeRef.current reset, hasTriggeredRefresh.current = false and setProgress(0) always run, and additionally catch/log errors in the auto path to avoid unhandled promise rejections; locate the logic around the anonymous async IIFE and the auto-refresh block that calls onRefresh and apply try/finally (with a catch in the auto path) to restore UI state on failure.web/app/root.tsx (1)
255-275:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd disclosure ARIA state for crash stack toggle.
The show/hide stack button should expose expanded state and control target so screen readers can understand the toggle behavior.
Suggested fix
- <button + <button type='button' onClick={() => setCrashStackShown((prev) => !prev)} + aria-expanded={crashStackShown} + aria-controls='error-boundary-stack-trace' className='flex cursor-pointer items-center justify-center text-base text-gray-800 hover:underline dark:text-gray-300' > @@ - <span className='text-sm whitespace-pre-line text-gray-600 dark:text-gray-400'> + <span + id='error-boundary-stack-trace' + className='text-sm whitespace-pre-line text-gray-600 dark:text-gray-400' + > {error.stack} </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/root.tsx` around lines 255 - 275, The crash stack toggle button (using setCrashStackShown and crashStackShown) lacks ARIA disclosure attributes—add aria-expanded={crashStackShown} to the button and give the stack container (the span rendering error.stack) a stable id (e.g., crash-stack) and add aria-controls pointing to that id on the button; ensure the controlled span has role="region" or aria-hidden appropriately so screen readers can detect the expanded/collapsed state and target.web/app/routes/_index.tsx (1)
335-345:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake live-demo overlay keyboard-accessible (not hover-only).
The CTA reveal is tied to
group-hover; addgroup-focus-withinso keyboard focus can reveal the overlay too.Suggested fix
- <div className='pointer-events-none absolute inset-0 flex items-center justify-center rounded-2xl bg-slate-900/40 opacity-0 backdrop-blur-[2px] transition-opacity duration-200 group-hover:pointer-events-auto group-hover:opacity-100'> + <div className='pointer-events-none absolute inset-0 flex items-center justify-center rounded-2xl bg-slate-900/40 opacity-0 backdrop-blur-[2px] transition-opacity duration-200 group-hover:pointer-events-auto group-hover:opacity-100 group-focus-within:pointer-events-auto group-focus-within:opacity-100'> <a href={LIVE_DEMO_URL} target='_blank' rel='noopener noreferrer' - className='pointer-events-auto inline-flex items-center rounded-md bg-white px-4 py-2 text-sm font-semibold text-gray-900 shadow-sm ring-1 ring-black/10 transition-all hover:bg-gray-50 dark:bg-slate-950 dark:text-white dark:ring-white/10 dark:hover:bg-slate-900' + className='pointer-events-auto inline-flex items-center rounded-md bg-white px-4 py-2 text-sm font-semibold text-gray-900 shadow-sm ring-1 ring-black/10 transition-all hover:bg-gray-50 focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-indigo-500 dark:bg-slate-950 dark:text-white dark:ring-white/10 dark:hover:bg-slate-900' aria-label={`${t('main.seeLiveDemo')} (opens in a new tab)`} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/routes/_index.tsx` around lines 335 - 345, The overlay reveal is currently only triggered by hover via the "group-hover" tailwind classes on the overlay div; update that overlay's class list (the div containing the CTA anchor and ArrowRightIcon) to also include the "group-focus-within" variants (e.g., add group-focus-within:pointer-events-auto and group-focus-within:opacity-100) so keyboard focus on the live demo link (LIVE_DEMO_URL anchor) will reveal the overlay as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/app/modals/AnnotationModal.tsx`:
- Around line 125-135: The label rendered by the Text component is not linked to
the Datepicker input; generate a unique id (e.g., via React's useId or a small
helper) and set that id as the label's htmlFor (or aria-labelledby) and pass the
same id into the Datepicker's input prop (or id/inputId/inputProps.id depending
on Datepicker API) so the label and input are programmatically associated;
update both places where you render the date label + <Datepicker> (the Text
label and the corresponding Datepicker component) to use the shared id and, if
Datepicker only exposes an inputRef, set aria-labelledby on the input to the
label id.
- Around line 110-115: The Save button no longer prevents clicks while an async
save is in progress and _onSubmit lacks a loading short-circuit, which allows
duplicate submissions; add an early return in _onSubmit like `if (loading)
return` to short-circuit when a save is in progress, and/or restore the loading
check in the Button's disabled prop (ensure the Button rendering that uses props
size='lg' onClick={_onSubmit} includes `disabled={!date || !text.trim() ||
!allowedToManage || loading}`); apply the same guard pattern to the other
save/button instance that uses the same _onSubmit handler.
In `@web/app/pages/ErrorTracking/index.tsx`:
- Around line 157-160: The current map rendering uses the localized string as
React key (key={item}) which can collide; in the mapping inside ErrorTracking's
render where _map(t('errors.fast.list', { returnObjects: true }), (item) => ...)
replace the key with a stable unique value (e.g., include the map index or an
existing id) by changing the iterator to (item, idx) and using a key like
`error-fast-${idx}` or item.id if items are objects, and keep the Text component
and content identical.
In `@web/app/pages/Organisations/Settings/People.tsx`:
- Around line 173-180: The remove action button (Button with onClick={() =>
onRemove(member)}) is icon-only and lacks an accessible name; add an aria-label
to the Button (for example aria-label={`Remove ${member.name || 'member'}`}) so
screen readers announce the action, and mark the TrashIcon decorative by passing
aria-hidden={true} (or role="img" aria-hidden) to the TrashIcon component so it
isn't announced twice.
In `@web/app/routes/tools.meta-tags-generator.tsx`:
- Around line 276-283: The rendering uses new URL(...) directly on data.url
which can throw for malformed values and crash the page; update the hostname
extraction (where new URL(...) is invoked for rendering the Text paragraph) to
safely parse the input—either wrap the URL construction in a try/catch or add a
small helper like getHostnameFromUrl(url) that returns a default ('example.com'
or '') on error or when url is falsy; replace the inline new URL(...) expression
in the component with a call to that safe parser so invalid input no longer
throws during render.
In `@web/app/ui/MultiSelect.tsx`:
- Around line 139-143: Replace usage of itemKey(items[activeIndex]) when
building DOM ids with a deterministic index-based id: construct activeOptionId
(and the option id generation used elsewhere around the option rendering logic)
from optionIdPrefix + the item's index (e.g., `${optionIdPrefix}-${index}`)
instead of itemKey(item) so ids are valid, unique and stable for
aria-activedescendant; update any references to option ids in the same component
to use this index-based scheme and ensure activeIndex is used to derive the id
consistently.
In `@web/app/ui/ProgressRing.tsx`:
- Around line 86-99: The computed displayValue and ARIA attributes use
normalizedValue directly and can become NaN/Infinity; update the logic around
normalizedValue and displayValue in ProgressRing.tsx to guard non-finite inputs
(use Number.isFinite(normalizedValue) ? Math.floor(normalizedValue) : 0) and
then use that safe displayValue for aria-valuenow and aria-label (or omit
aria-valuenow when not finite) so the component never renders "NaN%" or an
invalid ARIA value; modify the code that defines displayValue and the JSX
attributes referencing it accordingly (look for normalizedValue, displayValue,
aria-valuenow, aria-label).
---
Outside diff comments:
In `@web/app/pages/Project/View/components/RefreshStatsButton.tsx`:
- Around line 55-63: The refresh logic can leave isRefreshing stuck true and
produce unhandled rejections if onRefresh throws; wrap the await calls to
onRefresh(false) (manual path inside the IIFE) and the auto-refresh await (the
block around lines using startTimeRef/hasTriggeredRefresh/setProgress) in
try/finally so setIsRefreshing(false), startTimeRef.current reset,
hasTriggeredRefresh.current = false and setProgress(0) always run, and
additionally catch/log errors in the auto path to avoid unhandled promise
rejections; locate the logic around the anonymous async IIFE and the
auto-refresh block that calls onRefresh and apply try/finally (with a catch in
the auto path) to restore UI state on failure.
In `@web/app/root.tsx`:
- Around line 255-275: The crash stack toggle button (using setCrashStackShown
and crashStackShown) lacks ARIA disclosure attributes—add
aria-expanded={crashStackShown} to the button and give the stack container (the
span rendering error.stack) a stable id (e.g., crash-stack) and add
aria-controls pointing to that id on the button; ensure the controlled span has
role="region" or aria-hidden appropriately so screen readers can detect the
expanded/collapsed state and target.
In `@web/app/routes/_index.tsx`:
- Around line 335-345: The overlay reveal is currently only triggered by hover
via the "group-hover" tailwind classes on the overlay div; update that overlay's
class list (the div containing the CTA anchor and ArrowRightIcon) to also
include the "group-focus-within" variants (e.g., add
group-focus-within:pointer-events-auto and group-focus-within:opacity-100) so
keyboard focus on the live demo link (LIVE_DEMO_URL anchor) will reveal the
overlay as well.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f9c84ac8-7420-43d3-be56-bf3e462e3f99
📒 Files selected for processing (91)
web/app/components/ExitIntentPopup.tsxweb/app/components/Footer/index.tsxweb/app/components/Header/index.tsxweb/app/components/marketing/DitchGoogle.tsxweb/app/components/marketing/FAQ.tsxweb/app/components/pricing/BillingPricing.tsxweb/app/components/pricing/MarketingPricing.tsxweb/app/modals/AnnotationModal.tsxweb/app/modals/ViewProjectHotkeys.tsxweb/app/pages/Auth/CreateNewPassword/CreateNewPassword.tsxweb/app/pages/Auth/ForgotPassword/ForgotPassword.tsxweb/app/pages/Auth/Signup/InvitationSignup.tsxweb/app/pages/Blog/Post.tsxweb/app/pages/Contact/index.tsxweb/app/pages/ErrorTracking/index.tsxweb/app/pages/Landings/Marketers.tsxweb/app/pages/Landings/SmallBusinesses.tsxweb/app/pages/Landings/Startups.tsxweb/app/pages/NotFound/index.tsxweb/app/pages/OpenStartup/index.tsxweb/app/pages/Organisations/Settings/People.tsxweb/app/pages/Organisations/Settings/Projects.tsxweb/app/pages/Organisations/Settings/index.tsxweb/app/pages/Performance/index.tsxweb/app/pages/Project/Settings/Alerts/ProjectAlertsSettings.tsxweb/app/pages/Project/Settings/Alerts/ProjectAlertsView.tsxweb/app/pages/Project/Settings/Annotations.tsxweb/app/pages/Project/Settings/Emails/Emails.tsxweb/app/pages/Project/Settings/People/People.tsxweb/app/pages/Project/Settings/ProjectSettings.tsxweb/app/pages/Project/Settings/tabs/AccessSettings.tsxweb/app/pages/Project/Settings/tabs/DangerZone.tsxweb/app/pages/Project/Settings/tabs/General.tsxweb/app/pages/Project/Settings/tabs/Shields.tsxweb/app/pages/Project/View/components/AddAViewModal.tsxweb/app/pages/Project/View/components/LiveVisitorsDropdown.tsxweb/app/pages/Project/View/components/PasswordRequiredModal.tsxweb/app/pages/Project/View/components/RefreshStatsButton.tsxweb/app/pages/Project/View/components/SearchFilters.tsxweb/app/pages/Project/tabs/AskAI/AIChart.tsxweb/app/pages/Project/tabs/AskAI/AskAIView.tsxweb/app/pages/Project/tabs/Errors/ErrorDetails.tsxweb/app/pages/Project/tabs/Errors/NoErrorDetails.tsxweb/app/pages/Project/tabs/Experiments/ExperimentSettingsModal.tsxweb/app/pages/Project/tabs/Experiments/ExperimentsView.tsxweb/app/pages/Project/tabs/FeatureFlags/FeatureFlagsView.tsxweb/app/pages/Project/tabs/Funnels/FunnelsView.tsxweb/app/pages/Project/tabs/Goals/GoalSettingsModal.tsxweb/app/pages/Project/tabs/Goals/GoalsView.tsxweb/app/pages/Project/tabs/Sessions/NoSessionDetails.tsxweb/app/pages/Project/tabs/Sessions/SessionDetailView.tsxweb/app/pages/Project/tabs/Traffic/TrafficView.tsxweb/app/pages/Project/tabs/Traffic/UserFlow.tsxweb/app/pages/UserSettings/UserSettings.tsxweb/app/pages/UserSettings/components/Organisations.tsxweb/app/pages/UserSettings/components/ProjectList.tsxweb/app/pages/UserSettings/components/TwoFA.tsxweb/app/root.tsxweb/app/routes/_index.tsxweb/app/routes/blog._index.tsxweb/app/routes/google-analytics-alternative.tsxweb/app/routes/pay.tsxweb/app/routes/tools.ab-test-calculator.tsxweb/app/routes/tools.ad-cost-calculator.tsxweb/app/routes/tools.conversion-rate-calculator.tsxweb/app/routes/tools.ctr-calculator.tsxweb/app/routes/tools.ltv-calculator.tsxweb/app/routes/tools.meta-tags-generator.tsxweb/app/routes/tools.og-preview.tsxweb/app/routes/tools.percentage-calculator.tsxweb/app/routes/tools.qr-code-generator.tsxweb/app/routes/tools.robots-txt-generator.tsxweb/app/routes/tools.roi-calculator.tsxweb/app/routes/tools.schema-markup-generator.tsxweb/app/routes/tools.utm-generator.tsxweb/app/ui/Combobox.tsxweb/app/ui/FileUpload.tsxweb/app/ui/Input.tsxweb/app/ui/MultiProgress.tsxweb/app/ui/MultiSelect.tsxweb/app/ui/Pagination.tsxweb/app/ui/PasswordStrength.tsxweb/app/ui/ProgressRing.tsxweb/app/ui/StatusPage.tsxweb/app/ui/Switch.tsxweb/app/ui/Text.tsxweb/public/locales/de.jsonweb/public/locales/en.jsonweb/public/locales/fr.jsonweb/public/locales/pl.jsonweb/public/locales/uk.json
✅ Files skipped from review due to trivial changes (11)
- web/public/locales/en.json
- web/app/pages/Project/Settings/tabs/General.tsx
- web/public/locales/pl.json
- web/public/locales/uk.json
- web/app/pages/Project/Settings/tabs/Shields.tsx
- web/public/locales/de.json
- web/public/locales/fr.json
- web/app/pages/Project/View/components/AddAViewModal.tsx
- web/app/components/Header/index.tsx
- web/app/pages/Project/Settings/tabs/AccessSettings.tsx
- web/app/routes/tools.utm-generator.tsx
🚧 Files skipped from review as they are similar to previous changes (20)
- web/app/pages/Auth/CreateNewPassword/CreateNewPassword.tsx
- web/app/pages/Project/tabs/Goals/GoalSettingsModal.tsx
- web/app/pages/Auth/ForgotPassword/ForgotPassword.tsx
- web/app/pages/Project/tabs/Errors/ErrorDetails.tsx
- web/app/pages/Project/View/components/PasswordRequiredModal.tsx
- web/app/pages/Organisations/Settings/index.tsx
- web/app/pages/Project/tabs/Funnels/FunnelsView.tsx
- web/app/components/pricing/BillingPricing.tsx
- web/app/ui/Text.tsx
- web/app/pages/Project/Settings/People/People.tsx
- web/app/routes/tools.percentage-calculator.tsx
- web/app/pages/Project/Settings/Alerts/ProjectAlertsView.tsx
- web/app/pages/Organisations/Settings/Projects.tsx
- web/app/ui/StatusPage.tsx
- web/app/ui/Input.tsx
- web/app/ui/Pagination.tsx
- web/app/pages/Project/tabs/Experiments/ExperimentSettingsModal.tsx
- web/app/pages/Project/tabs/AskAI/AskAIView.tsx
- web/app/ui/Combobox.tsx
- web/app/routes/tools.conversion-rate-calculator.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
web/app/ui/ProgressRing.tsx (1)
63-72:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNon-finite
valuestill propagates into ring math.At Line 64,
Math.min/Math.maxdoes not sanitizeNaN, sonormalizedValuecan still becomeNaN. That reaches Line 71 and produces an invalidstrokeDashoffset, breaking visual progress rendering despite the laterdisplayValueguard.Proposed fix
- const normalizedValue = useMemo( - () => Math.min(100, Math.max(0, value)), - [value], - ) + const normalizedValue = useMemo(() => { + if (!Number.isFinite(value)) return 0 + return Math.min(100, Math.max(0, value)) + }, [value])#!/bin/bash # Verify where normalizedValue is computed and where it flows into rendering math/ARIA. rg -n -C2 'normalizedValue|strokeDashoffset|aria-valuenow|displayValue' web/app/ui/ProgressRing.tsx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/ui/ProgressRing.tsx` around lines 63 - 72, normalizedValue can become NaN because Math.min/Math.max don't guard non-finite inputs; update the useMemo that computes normalizedValue to first coerce/sanitize value (use Number.isFinite or similar) into a finite number (e.g., default to 0 or clamp to bounds) and then apply Math.max/Math.min, so stroke math in the subsequent useMemo (radius, circumference, strokeDashoffset) always sees a finite numeric normalizedValue; ensure the dependency array still references value and that displayValue/aria-valuenow logic continues to use the sanitized normalizedValue.
🧹 Nitpick comments (1)
web/app/pages/Organisations/Settings/People.tsx (1)
429-435: Remove unsafe role cast by narrowing invite form role type.On Line 433,
(form.role as 'admin' | 'viewer')casts aRoletype (which includes'owner') to a narrower union. This bypasses type safety—if the invite form ever accidentally assigns'owner'toform.role, the cast silently allows it through. Create a dedicatedInviteRole = 'admin' | 'viewer'type and use it for the form state instead:+type InviteRole = 'admin' | 'viewer' + const [form, setForm] = useState<{ email: string - role: Role + role: InviteRole }>({ email: '', role: 'viewer', }) @@ - <RadioCardGroup<'admin' | 'viewer'> + <RadioCardGroup<InviteRole> name='role' label={t('project.settings.role')} className='mt-4' - value={(form.role as 'admin' | 'viewer') || null} + value={form.role} onChange={(role) => setForm((prev) => ({ ...prev, role }))} error={errors.role}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/pages/Organisations/Settings/People.tsx` around lines 429 - 435, The code unsafely casts form.role to 'admin'|'viewer' for RadioCardGroup; define a new InviteRole = 'admin' | 'viewer' type and change the invite form state so its role property is InviteRole (replace usages of the broader Role type for the invite form), then remove the cast in RadioCardGroup value and ensure onChange calls setForm((prev) => ({ ...prev, role })) use the narrowed InviteRole type; update any helpers/initial state that construct the invite form to use InviteRole so assigning 'owner' becomes a type error instead of a silent cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/app/pages/Project/View/components/RefreshStatsButton.tsx`:
- Around line 84-88: The manual refresh path in handleManualRefresh currently
awaits onRefresh(true) inside a try/finally but does not catch errors, causing
unhandled promise rejections; update handleManualRefresh to mirror the
auto-refresh logic by wrapping the await onRefresh(true) in a try/catch/finally
(or add a .catch) and log errors using the same logger used by the auto-refresh
path (e.g., processLogger or console.error) before calling
setIsRefreshing(false) in the finally block so errors are handled and state is
always reset.
In `@web/app/root.tsx`:
- Around line 275-279: The region element with id 'crash-stack' (role='region')
is unlabeled for assistive tech; add an accessible name by adding either an
aria-label (e.g., aria-label="Crash stack trace" or similar) or aria-labelledby
pointing to a nearby heading element that describes the content, ensuring the
label is concise and descriptive and updating any related heading id if using
aria-labelledby.
---
Duplicate comments:
In `@web/app/ui/ProgressRing.tsx`:
- Around line 63-72: normalizedValue can become NaN because Math.min/Math.max
don't guard non-finite inputs; update the useMemo that computes normalizedValue
to first coerce/sanitize value (use Number.isFinite or similar) into a finite
number (e.g., default to 0 or clamp to bounds) and then apply Math.max/Math.min,
so stroke math in the subsequent useMemo (radius, circumference,
strokeDashoffset) always sees a finite numeric normalizedValue; ensure the
dependency array still references value and that displayValue/aria-valuenow
logic continues to use the sanitized normalizedValue.
---
Nitpick comments:
In `@web/app/pages/Organisations/Settings/People.tsx`:
- Around line 429-435: The code unsafely casts form.role to 'admin'|'viewer' for
RadioCardGroup; define a new InviteRole = 'admin' | 'viewer' type and change the
invite form state so its role property is InviteRole (replace usages of the
broader Role type for the invite form), then remove the cast in RadioCardGroup
value and ensure onChange calls setForm((prev) => ({ ...prev, role })) use the
narrowed InviteRole type; update any helpers/initial state that construct the
invite form to use InviteRole so assigning 'owner' becomes a type error instead
of a silent cast.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8bc736d4-8316-4b86-b748-f91478dbdca8
📒 Files selected for processing (10)
web/app/modals/AnnotationModal.tsxweb/app/pages/ErrorTracking/index.tsxweb/app/pages/Organisations/Settings/People.tsxweb/app/pages/Project/View/components/RefreshStatsButton.tsxweb/app/root.tsxweb/app/routes/_index.tsxweb/app/routes/tools.meta-tags-generator.tsxweb/app/ui/Datepicker.tsxweb/app/ui/MultiSelect.tsxweb/app/ui/ProgressRing.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- web/app/routes/_index.tsx
- web/app/ui/MultiSelect.tsx
- web/app/pages/ErrorTracking/index.tsx
3 of 6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
If applicable, please describe what changes were made in this pull request.
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
New Features
UI/Style Updates
Accessibility Improvements