Implement SSR / proxy API requests through RR loaders or actions#459
Implement SSR / proxy API requests through RR loaders or actions#459
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughCentralizes server API access with a typed server client, migrates many frontend flows to react‑router fetcher/loader/deferred patterns and proxy hooks, adds a streaming AI proxy route (/api/ai), changes captcha secret returns to Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Browser
participant RemixAction as /api/ai (Remix action)
participant ServerClient as api.server (streamingServerFetch)
participant AIBackend as AI Backend
Client->>RemixAction: POST /api/ai { pid, messages, timezone }
RemixAction->>ServerClient: streamingServerFetch("ai/{pid}/chat", POST, body, forward cookies)
ServerClient->>AIBackend: POST ai/{pid}/chat (streaming)
AIBackend-->>ServerClient: SSE chunks
ServerClient-->>RemixAction: proxied stream chunks
RemixAction-->>Client: text/event-stream (SSE-compatible)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (15)
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 |
Blaumaus
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/app/pages/Project/Settings/ProjectSettings.tsx (2)
1-1: Fix code formatting before merging.The pipeline reports code style issues. Please run
prettier --write web/app/pages/Project/Settings/ProjectSettings.tsxto fix formatting inconsistencies.#!/bin/bash # Run prettier to fix formatting cd web && npx prettier --write app/pages/Project/Settings/ProjectSettings.tsx
123-126: Includepidin the useEffect dependency array or document why it's safe to exclude.The effect calls
getFiltersList()which uses bothpidandfilterType, but onlyfilterTypeis listed in the dependency array. AlthoughfetchFiltersis stable (passed from parent),pidcan change if the parent component receives a different project ID. Either addpidto[filterType, pid]or add a comment explaining why this pattern is safe (e.g., "modal always closes on project navigation").
🤖 Fix all issues with AI agents
In @web/app/pages/Organisations/Organisations.tsx:
- Around line 76-93: Refactor the fetcher response handling in the useEffect
that reads fetcher.data to simplify control flow and make error feedback
consistent: switch on fetcher.data?.intent (or use clear if/early-return
branches) and for 'create-organisation' handle success by calling
updateUrlParams(page, debouncedSearch), toast.success(...), and
closeNewOrganisationModal(); on failure handle field validation errors
(fetcher.data?.fieldErrors?.name) by calling setNewOrganisationError(...) and
toast.error(...) and handle generic fetcher.data?.error similarly
(setNewOrganisationError(...) + toast.error(...)); for 'accept-invitation' keep
the success path (updateUrlParams + toast.success) and otherwise fall back to a
single toast.error(fetcher.data.error) for unknown errors; use fetcher.data,
intent, fieldErrors.name, setNewOrganisationError, updateUrlParams,
closeNewOrganisationModal, and toast in the revised logic.
- Around line 77-78: The current post-fetcher logic calls updateUrlParams(page,
debouncedSearch) to refresh data; replace that with the revalidation pattern:
import useRevalidator, call const { revalidate } = useRevalidator() at component
top, and call revalidate() inside the branches where fetcher.data?.success &&
fetcher.data?.intent === 'create-organisation' (and the analogous delete/edit
branch) instead of updateUrlParams; also decide whether to preserve the current
page or reset to page 1 after creation and implement that UX choice by
conditionally setting page state before calling revalidate if needed.
🧹 Nitpick comments (2)
web/app/pages/Project/Settings/ProjectSettings.tsx (2)
495-509: Clarify the exhaustive-deps suppressions.Both effects disable the exhaustive-deps rule without documenting why
gscFetcheris excluded. While the fetcher reference fromuseFetcher()is stable across renders, adding a comment explaining this would improve maintainability.📝 Suggested comment additions
useEffect(() => { if (gscPropertiesPending && gscFetcher.state === 'idle') { setGscPropertiesPending(false) gscFetcher.submit({ intent: 'gsc-properties' }, { method: 'post' }) } - // eslint-disable-next-line react-hooks/exhaustive-deps + // gscFetcher ref is stable from useFetcher + // eslint-disable-next-line react-hooks/exhaustive-deps }, [gscPropertiesPending, gscFetcher.state]) // Initial GSC status fetch useEffect(() => { if (isSelfhosted || gscInitialized.current) return gscInitialized.current = true gscFetcher.submit({ intent: 'gsc-status' }, { method: 'post' }) + // gscFetcher ref is stable; run once on mount // eslint-disable-next-line react-hooks/exhaustive-deps }, [])
828-828: Consider removing type casts for better type safety.The
as anycasts at lines 828 and 844 bypass TypeScript's type checking. These could potentially be improved:
- Line 828: The cast maintains the literal type
'off' | 'basic'. Consider using a type assertion to the specific union type instead.- Line 844: This cast may indicate a type mismatch between
Formand whatAccessSettingsexpects. Consider aligning the types or creating a proper type conversion function.♻️ Potential improvements
For line 828:
setBotsLevel={(name) => setForm((prevForm) => ({ ...prevForm, - // cast to maintain allowed literal types - botsProtectionLevel: name as any, + botsProtectionLevel: name as 'off' | 'basic', })) }For line 844, investigate the type mismatch between
FormandAccessSettings's expected prop type, and create a proper adapter if needed.Also applies to: 844-844
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web/app/pages/Organisations/Organisations.tsxweb/app/pages/Project/Settings/ProjectSettings.tsxweb/app/routes/projects.settings.$id.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
web/app/pages/Project/Settings/ProjectSettings.tsx (5)
web/app/providers/AuthProvider.tsx (1)
useAuth(129-137)web/app/lib/models/Project.ts (1)
Project(181-213)web/app/routes/projects.settings.$id.tsx (1)
ProjectSettingsActionData(51-71)web/app/hooks/useAnalyticsProxy.ts (1)
useFiltersProxy(536-593)web/app/lib/constants/index.ts (2)
isBrowser(290-290)isSelfhosted(309-309)
🪛 GitHub Actions: Pull Request Validation Workflow
web/app/pages/Project/Settings/ProjectSettings.tsx
[warning] 1-1: Code style issues found in the file. Run 'prettier --write' to fix.
🔇 Additional comments (8)
web/app/pages/Project/Settings/ProjectSettings.tsx (6)
1-91: LGTM! Clean migration to react-router patterns.The imports and type definitions are well-structured. The addition of
fetchFilterstoModalMessagePropsenables dynamic filter loading through the proxy hook, which is a good separation of concerns.
264-433: Well-structured state initialization with proper race-condition guards.The use of refs (
gscPropertiesPending,lastHandledGscData,gscInitialized) to coordinate GSC status and properties fetching prevents race conditions during initialization. The fallback chain forsharableLinkensures SSR compatibility.
458-473: Excellent URL validation for security.The validation at lines 458-468 properly checks protocol, credentials, and hostname to prevent potential SSRF or open redirect vulnerabilities. The safe construction ensures only valid HTTPS URLs to
accounts.google.comare allowed.
512-549: Solid fetcher response handling with proper state management.The intent-based action handling is clean and maintainable. Line 519's merge strategy (
{ ...prev, ...updatedProject }) correctly preserves client-only fields likerolethat aren't returned by the backend.
551-640: Well-structured action handlers with proper guards.All handlers correctly check
fetcher.state === 'submitting'to prevent duplicate submissions and use FormData with explicitintentfields for type-safe action routing. The validation inhandleReset(lines 626-639) ensures required data is present before submission.
1085-1085: No issues found. The prop namesetResetting={isResetting}at line 1085 correctly matches the DangerZone component's interface, which definessetResetting: booleanas an expected prop. The code is correct as written.Likely an incorrect or invalid review comment.
web/app/pages/Organisations/Organisations.tsx (2)
26-47: Well-structured migration to loader/fetcher pattern.The refactoring from direct API calls to React Router's loader/fetcher pattern is well-executed:
- Proper initialization of UI state from loader data with fallbacks
- Correct derivation of loading state from
fetcher.state- Good use of optional chaining to handle potentially undefined loader data
- Clean separation between server data (from loader) and local UI state (search, modals)
The approach aligns with React Router best practices for data fetching and mutations.
100-108: Proper double-submission prevention.The guard at Line 100-102 correctly prevents double submissions by checking
fetcher.state === 'submitting'. This works in conjunction with theisNewOrganisationLoadingstate (Line 42) used to disable the modal's submit button, providing defense-in-depth against race conditions.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @web/app/pages/Dashboard/ProjectCard.tsx:
- Around line 167-196: The effect uses variables that can become stale and
listens to global fetcher.data across instances; include all referenced values
(isPinned, shareId, user, mergeUser, refetchProjects, t, id, and fetcher.data)
in the effect dependency array instead of disabling exhaustive-deps, and add a
guard so the handler only runs for responses targeting this card (e.g., check
fetcher.data.project?.id === id or fetcher.formData/projectId === id) before
applying optimistic reverts, to ensure per-instance isolation and fresh
closures.
- Line 432: The skeleton's container div currently uses 'mt-5.5' for non-list
view which mismatches the real ProjectCard's 'mt-4' in list view; update the
className expression in the skeleton (the div with cx('flex shrink-0 gap-5',
viewMode === 'list' ? 'ml-4' : 'mt-5.5 px-4 pb-4')) to use 'mt-4' instead of
'mt-5.5' so spacing matches the actual ProjectCard (viewMode handling remains
the same).
- Line 335: The rotation utility used in the className for the icon is invalid
(using rotate-30); update the conditional in the JSX where className={cx('size-5
transition-transform', localIsPinned && 'rotate-30')} to use Tailwind's
arbitrary rotation syntax 'rotate-[30deg]' instead so the icon actually rotates
30 degrees when localIsPinned is true (look for the cx call that composes
classes around localIsPinned in ProjectCard's icon).
🧹 Nitpick comments (4)
web/app/pages/Dashboard/Dashboard.tsx (2)
188-200: Consider guarding against duplicate effect executions.The effect could potentially fire multiple times if
tchanges (e.g., language switch) whilefetcher.datastill holds the same response, leading to duplicate toast notifications andrefetchProjectscalls.Consider tracking whether the response has been handled:
♻️ Proposed guard against duplicate handling
+ const [handledFetcherKey, setHandledFetcherKey] = useState<string | null>(null) useEffect(() => { + // Prevent duplicate handling of the same fetcher response + const fetcherKey = fetcher.data ? JSON.stringify(fetcher.data) : null + if (fetcherKey && fetcherKey === handledFetcherKey) return + if (fetcher.data?.success && fetcher.data?.intent === 'create-project') { + setHandledFetcherKey(fetcherKey) refetchProjects() toast.success(t('project.settings.created')) closeNewProjectModal() } else if (fetcher.data?.error && fetcher.data?.intent === 'create-project') { + setHandledFetcherKey(fetcherKey) setNewProjectError(fetcher.data.error) toast.error(fetcher.data.error) } else if (fetcher.data?.fieldErrors?.name && fetcher.data?.intent === 'create-project') { + setHandledFetcherKey(fetcherKey) setNewProjectError(fetcher.data.fieldErrors.name) } - }, [fetcher.data, t]) // eslint-disable-line react-hooks/exhaustive-deps + }, [fetcher.data, t, handledFetcherKey]) // eslint-disable-line react-hooks/exhaustive-deps
448-458: Consider simplifying thekeyprop.If
project.idis guaranteed to be unique (which it should be for database entities), includingproject.namein the key is redundant and could cause unnecessary re-mounts if the project is renamed.♻️ Simplified key
- key={`${project.id}-${project.name}`} + key={project.id}web/app/pages/Dashboard/ProjectCard.tsx (2)
28-37: SSR-safe initialization looks correct.The initial
falsestate and queueMicrotask pattern ensure the first client render matches the server output, preventing hydration mismatches. However, React 19 has improved hydration behavior that tolerates certain client-only values. Consider testing whether the simpler immediatesetIsTouchDevice(mediaQuery.matches)in the effect works without hydration warnings in your React 19 setup.
340-351: Reconsider replacing Link with button + programmatic navigation.Replacing
Linkwithbutton+navigate()loses several built-in benefits:
- Right-click "Open in new tab/window"
- Browser status bar preview of destination
- Assistive technology link semantics
- Cmd/Ctrl+click to open in new tab
If you need to prevent navigation bubbling or add custom logic, you can keep
Linkand useonClickto calle.preventDefault()conditionally, thennavigate()only when needed.♻️ Alternative approach preserving Link semantics
- <button - type='button' + <Link + to={_replace(routes.project_settings, ':id', id)} className='rounded-md border border-transparent p-1.5 text-gray-800 transition-colors hover:border-gray-300 hover:bg-gray-50 hover:text-gray-900 dark:text-slate-400 dark:hover:border-slate-700/80 dark:hover:bg-slate-800 dark:hover:text-slate-300' onClick={(e) => { e.preventDefault() e.stopPropagation() - navigate(_replace(routes.project_settings, ':id', id)) }} aria-label={`${t('project.settings.settings')} ${name}`} > <Settings2Icon className='size-5' strokeWidth={1.5} /> - </button> + </Link>This preserves Link's accessibility while preventing the parent Link's navigation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web/app/pages/Dashboard/Dashboard.tsxweb/app/pages/Dashboard/ProjectCard.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
web/app/pages/Dashboard/ProjectCard.tsx (1)
web/app/routes/dashboard.tsx (1)
DashboardActionData(23-32)
🪛 GitHub Actions: Pull Request Validation Workflow
web/app/pages/Dashboard/ProjectCard.tsx
[warning] 1-1: Code style issues found in file. Run 'prettier --write' to fix.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (15)
web/app/pages/Dashboard/Dashboard.tsx (10)
7-34: LGTM! Clean import organization.The imports are well-organized, separating React, router, and application-specific dependencies. The migration to React Router v7 hooks (
useFetcher,useNavigation,useRevalidator) and proxy hooks for analytics data is consistent with the SSR/proxy architecture goals.
46-93: LGTM! Well-structured hook initialization.The component properly leverages React Router v7's data patterns:
useLoaderDatafor SSR-compatible initial datauseNavigationanduseRevalidatorfor loading stateuseFetcherfor mutations without navigationThe separation of
isLoading(for data fetching) andisNewProjectLoading(line 202, for project creation) is appropriate.
130-140: LGTM! URL parameter management is straightforward.The
updateURLfunction correctly manages search params, removing empty values to keep URLs clean. The falsy check works well for the current use cases (page numbers, sort options, periods) where"0"or empty strings aren't valid values.
204-220: LGTM! Clean fetcher-based form submission.The project creation flow correctly uses React Router's fetcher pattern with
FormDataand anintentfield for action routing. The defensiveDEFAULT_PROJECT_NAMEfallback is good practice even though validation should prevent empty names.
278-298: LGTM! Overall stats fetching is well-structured.The effect correctly fetches stats when relevant dependencies change. The null check before
setOverallStatsis good defensive coding. Note the same callback stability consideration applies tofetchOverallStatsas mentioned forfetchLiveVisitors.
246-253: LGTM! Debounced search sync is correctly implemented.The comparison with
currentSearchprevents unnecessary URL updates and infinite loops. Resetting to page 1 on search change is the correct UX behavior.
95-102: LGTM! Clean URL parameter derivation.The memoized values correctly derive state from URL search params with sensible defaults. The
sortByvalidation againstSORT_OPTIONSprevents injection of invalid sort values.
234-236: LGTM! Proper use of revalidator.Using
revalidator.revalidate()is the idiomatic React Router v7 approach to refresh loader data after mutations. The wrapper function provides a clean abstraction.
256-276: > Likely an incorrect or invalid review comment.
538-538: Route-level authentication is properly enforced.The
withAuthenticationHOC removal is safe — both the loader and action functions callredirectIfNotAuthenticated(request), ensuring unauthenticated users are redirected before the component renders. Thememo()wrapper is appropriate for optimizing component re-renders with stable props.web/app/pages/Dashboard/ProjectCard.tsx (5)
53-53: Props signature change is correct.The return type change from
Promise<void>tovoidaligns with the fetcher-driven workflow whererefetchProjectslikely triggers a state update rather than an async operation.
130-131: LGTM!Standard React Router fetcher and navigation setup with correct typing.
155-164: LGTM!Solid optimistic UI pattern. The local state tracks immediate user actions while the sync effect ensures server updates are reflected.
198-215: LGTM!Clean migration to fetcher pattern with proper optimistic updates and double-submission prevention.
253-263: LGTM!Proper validation and fetcher submission for accepting project shares.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/community/src/analytics/analytics.controller.ts (1)
1248-1335: Critical:noscript()useslogDTObut it’s no longer defined (runtime crash).
logDTOis referenced multiple times (validate / generateAndStoreSessionId / generateProfileId / recordSessionActivity / trafficTransformer) but the variable is missing, so this endpoint will throw on every non-bot request.Proposed fix
async noscript( @Query() data: NoscriptDto, @Headers() headers, @Ip() reqIP, @Response() res, ) { const { 'user-agent': userAgent, origin } = headers const { pid } = data const isBot = await this.analyticsService.isBot(pid, userAgent) if (isBot) { res.writeHead(200, { 'Content-Type': 'image/gif' }) return res.end(TRANSPARENT_GIF_BUFFER, 'binary') } + const logDTO: PageviewsDto = { pid } + const ip = getIPFromHeaders(headers) || reqIP || '' const project = await this.analyticsService.validate(logDTO, origin, ip)
🧹 Nitpick comments (2)
backend/apps/cloud/src/analytics/analytics.controller.ts (1)
242-245: Avoid logging persistent identifiers / user-provided content verbatim (privacy + log-forging risk).Several new logs include stable identifiers (
profileId,psid/sessionId) and user-controlled strings (e.g.,pg,message,metrics,customEvents). Consider redacting/hashing IDs and truncating/normalizing user-provided fields (and/or droppingerrorDTO.messageentirely) to reduce PII exposure, log injection, and noisy/expensive logs.Also applies to: 412-413, 481-482, 503-504, 525-526, 547-551, 572-576, 617-618, 663-667, 806-810, 907-908, 985-989, 1032-1033, 1094-1095, 1189-1193, 1307-1311, 1418-1419, 1469-1470, 1610-1611, 1681-1685, 1769-1773, 1848-1852, 1914-1918, 2007-2011, 2062-2063, 2109-2113, 2188-2192, 2275-2279, 2356-2360, 2440-2444, 2471-2475
backend/apps/community/src/analytics/analytics.controller.ts (1)
223-226: Same note as Cloud: consider redacting/truncating IDs + user-controlled fields in logs.Also applies to: 406-407, 467-468, 487-488, 507-508, 547-548, 591-592, 682-683, 714-715, 768-769, 827-828, 849-850, 893-894, 990-994, 1101-1102, 1151-1152, 1292-1293, 1361-1362, 1427-1428, 1464-1465, 1547-1548, 1567-1568, 1610-1614, 1723-1724, 1797-1798, 1858-1859, 1946-1947, 2013-2014, 2087-2088, 2169-2170, 2240-2241, 2268-2269
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/cloud/src/analytics/analytics.controller.tsbackend/apps/community/src/analytics/analytics.controller.ts
🧰 Additional context used
🪛 GitHub Actions: Pull Request Validation Workflow
backend/apps/community/src/analytics/analytics.controller.ts
[error] 591-591: Prettier formatting error. Replace ``pid:·${pid},·period:·${period},·measure:·${measure},·'GET·/analytics/performance' with `⏎······`pid:·${pid},·period:·${period},·measure:·${measure}`,⏎······'GET·/analytics/performance',⏎····` (prettier/prettier)
🪛 GitHub Check: Backend Checks (22.x.x)
backend/apps/community/src/analytics/analytics.controller.ts
[failure] 591-591:
Replace ``pid:·${pid},·period:·${period},·measure:·${measure},·'GET·/analytics/performance' with `⏎······`pid:·${pid},·period:·${period},·measure:·${measure}`,⏎······'GET·/analytics/performance',⏎····`
[failure] 682-682:
Replace ``pid:·${pid},·period:·${period},·measure:·${measure},·'GET·/analytics/performance/chart' with `⏎······`pid:·${pid},·period:·${period},·measure:·${measure}`,⏎······'GET·/analytics/performance/chart',⏎····`
[failure] 714-714:
Replace ``pid:·${pid},·period:·${period},·'GET·/analytics/user-flow' with `⏎······`pid:·${pid},·period:·${period}`,⏎······'GET·/analytics/user-flow',⏎····`
[failure] 827-827:
Replace ``pidsArray:·${pidsArray},·measure:·${measure},·'GET·/analytics/performance/birdseye' with `⏎······`pidsArray:·${pidsArray},·measure:·${measure}`,⏎······'GET·/analytics/performance/birdseye',⏎····`
[failure] 1361-1361:
Replace ``pid:·${pid},·period:·${period},·take:·${take},·skip:·${skip},·'GET·/analytics/sessions' with `⏎······`pid:·${pid},·period:·${period},·take:·${take},·skip:·${skip}`,⏎······'GET·/analytics/sessions',⏎····`
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
backend/apps/cloud/src/analytics/analytics.controller.ts (1)
242-245: Logging placement is sensible (after access checks).Also applies to: 412-413, 525-526, 617-618, 663-667, 806-810, 1681-1685, 1769-1773, 2007-2011
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/cloud/src/analytics/analytics.service.ts (1)
4901-4979: Profiles aggregation: guardsessionsCountagainst psid=0 and strengthenany()with conditional functions; critical: time-range parameters not applied to query
countDistinct(psid)skips NULLs but not0values—sessions with psid=0 inflatesessionsCount. Other queries in this codebase explicitly useAND psid != 0(lines 1465, 1476), confirming psid can be 0. Additionally, controller passesgroupFromUTCandgroupToUTCto the query but the WHERE clause never appliesAND created BETWEEN {groupFrom:String} AND {groupTo:String}, unlike comparable queries (e.g., lines 297, 300). This omission means profiles are unfiltered by the requested time range, breaking reporting accuracy.For robustness,
any(cc/os/br/dv)across the UNION should prefer non-empty dimensions.Proposed fix: apply time-range filter and use conditional aggregate functions
WITH all_profile_data AS ( SELECT profileId, psid, cc, os, br, dv, created, 1 AS isPageview, 0 AS isEvent FROM analytics WHERE pid = {pid:FixedString(12)} AND profileId IS NOT NULL AND profileId != '' AND psid != 0 AND created BETWEEN {groupFrom:String} AND {groupTo:String} ${profileTypeFilter} ${filtersQuery} UNION ALL SELECT profileId, psid, cc, os, br, dv, created, 0 AS isPageview, 1 AS isEvent FROM customEV WHERE pid = {pid:FixedString(12)} AND profileId IS NOT NULL AND profileId != '' AND psid != 0 AND created BETWEEN {groupFrom:String} AND {groupTo:String} ${profileTypeFilter} ${filtersQuery} ), profile_aggregated AS ( SELECT profileId, sum(isPageview) AS pageviewsCount, - countDistinct(psid) AS sessionsCount, + uniqExactIf(psid, psid != 0) AS sessionsCount, sum(isEvent) AS eventsCount, min(created) AS firstSeen, max(created) AS lastSeen, - any(cc) AS cc_agg, - any(os) AS os_agg, - any(br) AS br_agg, - any(dv) AS dv_agg + anyIf(cc, cc IS NOT NULL AND cc != '') AS cc_agg, + anyIf(os, os IS NOT NULL AND os != '') AS os_agg, + anyIf(br, br IS NOT NULL AND br != '') AS br_agg, + anyIf(dv, dv IS NOT NULL AND dv != '') AS dv_agg FROM all_profile_data GROUP BY profileId ), profile_errors AS ( SELECT profileId, count() AS errorsCount FROM errors WHERE pid = {pid:FixedString(12)} AND profileId IS NOT NULL AND profileId != '' + AND created BETWEEN {groupFrom:String} AND {groupTo:String} ${profileTypeFilter} GROUP BY profileId )
🤖 Fix all issues with AI agents
In @backend/apps/cloud/src/analytics/analytics.service.ts:
- Around line 4706-4754: The filtered branch (when customEVFilterApplied)
currently builds primaryEventsSubquery only from customEV, causing
distinct_sessions_filtered to compute sessionStart/lastActivity from the
filtered custom events and thereby shrink/shift sessions; change the logic so
session boundaries still come from the full analytics events while retaining
customEV for filtering—e.g., in the customEVFilterApplied path, keep
primaryEventsSubquery selecting customEV rows for membership but also include
(via UNION ALL or a join/subquery) the relevant analytics rows per psid/pid (or
compute min/max analytics.created per psid/pid) so distinct_sessions_filtered
uses analytics-derived sessionStart and lastActivity for ordering/live checks
while filtersQuery is applied only to select sessions by custom events.
In @web/app/pages/Project/tabs/Traffic/TrafficView.tsx:
- Around line 109-125: TrafficDataResolver currently calls the React "use" hook
on loader promises which will throw on rejection and unmount the tree; wrap its
usage in an error boundary so rejected promises are caught and rendered
gracefully. Implement or reuse an ErrorBoundary (e.g., TrafficErrorBoundary) and
render <TrafficErrorBoundary><TrafficDataResolver ... /></TrafficErrorBoundary>
(or wrap the internals of TrafficDataResolver) so that any thrown errors from
use(trafficDataPromise) / use(overallStatsPromise) / use(trafficComparePromise)
/ use(overallComparePromise) / use(customEventsPromise) are caught and a
user-friendly fallback UI is shown instead of unmounting the app. Ensure the
boundary uses getDerivedStateFromError / componentDidCatch (or React
ErrorBoundary equivalent) and returns a clear fallback like "Failed to load
traffic data. Please try again."
- Around line 530-593: The loadRevenueData callback can run before
baseChartData.x is populated causing incorrect alignment; update loadRevenueData
to early-return when baseChartData?.x is missing or empty (check chart.x
existence/length) before attempting to align series, so setRevenueOverlay(null)
and exit early if the base chart x-axis is not yet available (references:
loadRevenueData, baseChartData, setRevenueOverlay).
🧹 Nitpick comments (4)
backend/apps/community/src/analytics/analytics.service.ts (1)
4258-4306: LGTM! Clean implementation of dynamic data source selection.The conditional construction of
primaryEventsSubquerycorrectly handles two scenarios:
- When
customEVFilterAppliedis true: queries only thecustomEVtable (relevant data source)- When false: combines both
analyticsandcustomEVtables viaUNION ALLThe use of
UNION ALL(rather thanUNION) is appropriate here since deduplication is not needed, avoiding unnecessary performance overhead. Thepsid IS NOT NULLconstraints (Lines 4272, 4288, 4302) ensure data integrity across both branches.Operational consideration: For large-scale deployments, monitor the performance of the
UNION ALLoperation in the non-filtered path. Consider adding appropriate indexes on(pid, psid, created)for bothanalyticsandcustomEVtables if query performance becomes a concern.web/app/pages/Project/tabs/Traffic/TrafficView.tsx (3)
260-265: Document the dynamic chartData shape.The
chartDataobject has a dynamically merged shape (base chart + optional revenue fields). This explains why line 904 requires a type assertion. Consider documenting this in a type or comment to make the dynamic nature explicit to future maintainers.📝 Optional: Document dynamic shape
Add a type comment:
+ // chartData shape: baseChartData + optional revenue overlay { revenue: number[], refundsAmount: number[] } const chartData = useMemo(() => { if (revenueOverlay) { return { ...baseChartData, ...revenueOverlay } } return baseChartData }, [baseChartData, revenueOverlay])
686-692: Consider includingrevalidatorin the dependency array.While
revalidator.revalidateis typically stable, React's exhaustive-deps rule generally expects the revalidator object itself to be in the dependencies. The current eslint-disable might hide this.♻️ Recommended: Add revalidator to dependencies
useEffect(() => { if (trafficRefreshTrigger > 0) { revalidator.revalidate() } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [trafficRefreshTrigger]) + }, [trafficRefreshTrigger, revalidator])
109-135: Overall architecture: Strong SSR migration with minor concerns.The refactor successfully migrates to a loader-based SSR pattern with React 19 features. The use of
useLoaderData, deferred promises, and theusehook is well-structured. However, please address:
- Missing error boundary for promise rejections (lines 109-125)
- Potential race condition between revenue data and base chart data (lines 530-593)
- Type safety with dynamic chartData shape requiring assertions
The URL-based state management for custom events and the revalidator pattern for refreshes are excellent choices for SSR compatibility.
Also applies to: 212-254
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/cloud/src/analytics/analytics.service.tsbackend/apps/community/src/analytics/analytics.service.tsweb/app/pages/Project/tabs/Traffic/TrafficView.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
backend/apps/community/src/analytics/analytics.service.ts (2)
4163-4184: LGTM! Well-implemented fallback for custom-events-only sessions.The addition of the customEV fallback query correctly handles sessions that contain only custom events without any pageviews in the analytics table. The
AND psid IS NOT NULLconstraint on Line 4169 is essential for data integrity, ensuring only valid session IDs are retrieved.This implementation aligns with the PR objective of supporting custom-events-only sessions.
5180-5258: Elegant refactor of profile aggregation using unified data source.The new approach consolidates profile data from multiple sources into a single unified flow:
all_profile_datacombines analytics and customEV tables with discriminator flags (isPageview/isEvent)profile_aggregatedsums these flags to compute counts:sum(isPageview)for pageviews,sum(isEvent)for eventsThis is both more maintainable (single data path) and likely more performant (one
UNION ALL+ aggregation vs. multiple separate queries).The aggregation logic is mathematically sound:
- Analytics rows contribute
isPageview=1, isEvent=0- CustomEV rows contribute
isPageview=0, isEvent=1- Summing gives accurate counts for each type
Verification recommendation: Given this is a significant refactor of query structure, ensure integration/E2E tests validate that:
- Profile counts (pageviews, events, sessions) match the previous implementation
- Edge cases are covered (profiles with only analytics, only customEV, or both)
- Performance is acceptable for profiles with large event volumes
web/app/pages/Project/tabs/Traffic/TrafficView.tsx (8)
212-223: LGTM: Clean data derivation from loader.The panelsData is correctly derived from deferred traffic data with proper null handling and memoization.
225-242: LGTM: Well-structured data derivation.The separation between
baseChartData(from loader) andchartData(merged with revenue) is clean, and the use ofrevalidator.statefor loading indication is the correct pattern for loader-based fetching.
284-288: LGTM: URL-based state for SSR compatibility.Reading custom events from URL parameters is the correct approach for SSR, ensuring state is preserved across navigation and refreshes.
500-520: LGTM: Correct URL parameter management.The toggle logic for custom events correctly adds/removes events from the URL parameter and cleans up when empty.
595-628: LGTM: Metadata fetching correctly uses proxy hooks.The migration to proxy hooks for custom events and property metadata is well-structured, with proper handling of custom date ranges and null safety.
631-679: LGTM: Keywords loading properly uses proxy and cleanup.The GSC keywords loading correctly uses the proxy hook and includes proper cleanup checks to prevent state updates after unmount.
1295-1297: LGTM: Clean export wrapper pattern.Assigning
TrafficViewWrappertoTrafficViewbefore exporting maintains the original export name while transparently adding Suspense and loader resolution.
95-95: Verify all call sites forloadProjectViewsprop signature change.The signature changed from
() => Promise<void>to(forced?: boolean) => void. Ensure all parent components passing this prop are updated to match the new signature and handle the synchronous return type instead of awaiting a promise.backend/apps/cloud/src/analytics/analytics.service.ts (1)
4611-4632: Schema verification confirms all fallback columns exist incustomEV—no issue hereThe
customEVtable includes all columns referenced in the fallback query (dv, br, brv, os, osv, lc, ref, so, me, ca, te, co, cc, rg, ct, profileId). BothcustomEVandanalyticstables have identical schema definitions in the migrations, so this fallback is safe.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/app/pages/Project/View/components/ProjectSidebar.tsx (1)
102-263: Fix CI:react/display-namefor memoizedCollapsibleGroup.The memo-wrapped anonymous component triggers the ESLint rule (and currently fails the pipeline). Name the inner function (or set
displayName) so React DevTools / lint can identify it.Proposed fix (name the memoized component)
-const CollapsibleGroup: React.FC<{ +const CollapsibleGroup: React.FC<{ group: TabGroup activeTab: ProjectTabKey onTabChange: (tabId: keyof typeof PROJECT_TABS) => void projectId: string dataLoading?: boolean searchParams: URLSearchParams isCollapsed?: boolean onMobileClose?: () => void -}> = memo(({ group, activeTab, onTabChange, projectId, dataLoading, searchParams, isCollapsed, onMobileClose }) => { +}> = memo(function CollapsibleGroup({ + group, + activeTab, + onTabChange, + projectId, + dataLoading, + searchParams, + isCollapsed, + onMobileClose, +}) { const [isExpanded, setIsExpanded] = useState(() => getGroupExpandedState(group.id)) ... -}) +})web/app/pages/Project/View/Panels.tsx (1)
397-502: Handleperiod === 'custom'with missingdateRangebefore proxy call.Currently
periodValuebecomes'custom'whenperiod === 'custom'butdateRangeis falsy (Line 465-469), and that value is passed to the proxy (Line 470-477). If the backend expects either a valid named period orperiod:''withfrom/to, this can break.Possible guard
@@ try { - const isCustomPeriod = period === 'custom' && dateRange + const isCustomPeriod = period === 'custom' && dateRange + if (period === 'custom' && !dateRange) { + setStackedChart(null) + return + } const from = isCustomPeriod ? getFormatDate(dateRange![0]) : '' const to = isCustomPeriod ? getFormatDate(dateRange![1]) : '' const periodValue = isCustomPeriod ? '' : period
🤖 Fix all issues with AI agents
In @web/app/pages/Project/tabs/Errors/ErrorsView.tsx:
- Around line 478-490: The code asserts deferredData.errorDetails.details as
SwetrixErrorDetails which is incorrect because ErrorDetailsResponse.details does
not include stackTrace; remove the improper type assertion on
activeError.details (in the activeError useMemo) and either leave it untyped
(infer its actual shape from ErrorDetailsResponse) or introduce a proper
union/type that matches ErrorDetailsResponse.details and SwetrixErrorDetails,
then update usages (e.g., ErrorDetails.tsx checks for stackTrace) to guard
against optional stackTrace via runtime checks rather than forcing the
SwetrixErrorDetails type.
In @web/app/pages/Project/tabs/Experiments/ExperimentsView.tsx:
- Line 78: The Experiment type in api.server.ts is missing the
filterInternalUsers: boolean field which causes a mismatch with the local
Experiment interface in ExperimentsView.tsx and the backend DTOs; update the
Experiment interface in api.server.ts (the exported Experiment type used by the
client) to include filterInternalUsers: boolean so the generated API types match
the backend/controller/DTOs and prevent TypeScript runtime/type errors.
In @web/app/pages/Project/tabs/Funnels/FunnelsView.tsx:
- Around line 125-163: The post-mutation handler updates the funnels list but
doesn't revalidate the loader that provides the active funnel analytics, so
charts can stay stale; update the useEffect that handles fetcher responses to,
when handling 'update-funnel' or 'delete-funnel' (and when fetcher.data includes
the affected funnel id), detect if that id equals the currently viewed funnel
(e.g., funnelData?.id or selectedFunnelId) and then trigger a loader
revalidation for that funnel — for example submit a fetcher request with an
intent like 'get-funnel' and the funnelId (or call your existing revalidation
helper) to reload funnel analytics after mergeProject; keep the existing
toast/UX flow but add this conditional revalidation step referencing fetcher,
fetcher.data, intent, mergeProject, and funnelData/selectedFunnelId.
- Around line 78-90: The code casts FunnelDataResponse.funnel to
AnalyticsFunnel[] which hides a real shape mismatch (server returns
AnalyticsFunnelStep[] with dropoffPerc while the component state expects
AnalyticsFunnel), so remove the unsafe `as AnalyticsFunnel[]` casts and either
(a) change the component state type to accept AnalyticsFunnelStep[] (update the
useState generic and all references to funnelAnalytics/funnel), or (b)
explicitly map FunnelDataResponse.funnel to the expected AnalyticsFunnel shape
(strip or transform dropoffPerc) before calling setFunnelAnalytics; apply this
fix at the useState initializer and in the other places where setFunnelAnalytics
or similar casts occur (references: FunnelDataResponse.funnel,
AnalyticsFunnelStep, AnalyticsFunnel, funnelAnalytics, setFunnelAnalytics).
In @web/app/pages/Project/tabs/Goals/GoalsView.tsx:
- Around line 517-548: The component leaves a sticky error because setError is
only called on failure; clear the error when starting a new fetch and on
successful responses. In loadGoals (function loadGoals) call setError(undefined)
before fetcher.submit to reset previous errors; in the useEffect that handles
fetcher responses (the block checking fetcher.data?.intent ===
'get-project-goals'), add setError(undefined) inside the success branch (where
setGoals and setTotal are called) so successful loads clear any prior error;
keep the existing error branch that calls setError(fetcher.data.error) for
failures.
In @web/app/pages/Project/tabs/Sessions/SessionsView.tsx:
- Around line 146-154: The effect that syncs loader data (useEffect watching
revalidator.state and deferredData.sessionsData) can clobber in-flight proxy
pagination results; modify the update so that when deferredData.sessionsData
changes you cancel or invalidate any pending proxy requests (e.g., abort the
fetch or bump a request token) before calling setSessions,
setSessionsSkip(SESSIONS_TAKE) and setCanLoadMoreSessions; alternatively track
loader vs proxy pagination state separately and only reset the proxy pagination
if the response token matches the current loader token to avoid applying stale
proxy responses.
- Line 112: The variable sessionsLoading is a boolean (const sessionsLoading =
revalidator.state === 'loading' || sessionsProxy.isLoading) but the code still
checks sessionsLoading === null at the locations surrounding the render logic;
remove those null comparisons and update the conditional logic to use the
boolean (e.g., if (sessionsLoading) or if (!sessionsLoading)) so all checks
reference the boolean sessionsLoading; search for any occurrences of
sessionsLoading === null (including the checks around lines where rendering
occurs) and delete or replace them with the appropriate boolean condition.
- Around line 99-110: The code force-casts deferredData.sessionDetails.details
to SessionDetailsType in the activeSession useMemo, hiding a real shape mismatch
between SessionDetailsResponse.details and the SessionDetails/SessionDetailsType
expected fields; instead of using "as unknown as SessionDetailsType",
map/transform deferredData.sessionDetails.details into the correct shape (or
extend the response type) so fields like osv, brv, lc, etc. are populated from
the API values (or provide defaults), update the construction inside useMemo
(referencing activeSession, useMemo, deferredData.sessionDetails,
SessionDetailsResponse.details, SessionDetailsType/SessionDetails) to perform a
proper conversion or validation before returning the ActiveSession object.
In @web/app/pages/Project/View/ViewProject.tsx:
- Around line 419-457: The useEffect handling viewsLoadFetcher can leave
projectViewsLoading true when the fetcher becomes 'idle' but has no data; update
the effect for viewsLoadFetcher (the anonymous useEffect watching
viewsLoadFetcher.state and viewsLoadFetcher.data) to always call
setProjectViewsLoading(false) as soon as viewsLoadFetcher.state === 'idle'
before branching on viewsLoadFetcher.data, and in the no-data branch explicitly
clear or leave project views (e.g. setProjectViews([]) or no-op) and/or surface
an error via toast; this ensures loadProjectViews and projectViewsLoading cannot
get stuck when viewsLoadFetcher finishes without data while keeping the existing
success and error branches intact.
🧹 Nitpick comments (23)
web/app/pages/Project/tabs/FeatureFlags/FeatureFlagsView.tsx (5)
467-469: Review deduplication and mounted ref patterns.The
processedActionRefpattern (line 467) is used to prevent duplicate action processing, but it's never reset except inline. Consider whether this could cause issues if the component remains mounted across many actions.The
isMountedRefpattern (line 469) helps prevent state updates after unmount, but there's still a potential race condition window between the async operation completing and the ref check.Consider using AbortController for cleaner async cleanup:
useEffect(() => { const abortController = new AbortController() return () => abortController.abort() }, [])
523-536: Add error handling for malformed fetcher responses.Line 528 performs a type assertion without validation. If the backend returns unexpected data structure, this could cause runtime errors.
🛡️ Add response validation
useEffect(() => { if (listFetcher.data?.intent === 'get-project-feature-flags' && listFetcher.state === 'idle') { if (isMountedRef.current) { if (listFetcher.data.success && listFetcher.data.data) { - const result = listFetcher.data.data as { results: ProjectFeatureFlag[]; total: number } - setFlags(result.results) - setTotal(result.total) + const result = listFetcher.data.data as { results: ProjectFeatureFlag[]; total: number } + if (Array.isArray(result.results) && typeof result.total === 'number') { + setFlags(result.results) + setTotal(result.total) + } else { + console.error('Unexpected data structure from list fetcher') + setError('Invalid response format') + } } else if (listFetcher.data.error) { setError(listFetcher.data.error) } } } }, [listFetcher.data, listFetcher.state])
539-567: Action deduplication pattern is fragile.The deduplication key at line 542 uses
JSON.stringify(actionFetcher.data.data), which could fail or produce unreliable keys if the data contains non-serializable values, undefined properties, or has property order variations.💡 More robust deduplication
Consider using a simpler, more reliable key:
-const actionKey = `${actionFetcher.data.intent}-${JSON.stringify(actionFetcher.data.data)}` +// For delete: just use the flagId +// For update: use flagId + enabled state +const actionKey = actionFetcher.data.intent === 'delete-feature-flag' + ? `delete-${actionFetcher.data.data?.flagId}` + : `update-${actionFetcher.data.data?.flagId}-${actionFetcher.data.data?.enabled}`Alternatively, use a timestamp-based or counter-based approach if you need to allow the same action to be processed multiple times intentionally.
600-660: Consider user feedback for failed analytics loads.Errors in
loadFlagStats(line 609) andloadFlagProfiles(line 648) are logged to console but not shown to users. If analytics fail to load, users see no indication of the problem, which could be confusing.Consider adding subtle error indicators or retry mechanisms:
// Option 1: Add error state to show in UI const [statsError, setStatsError] = useState<Record<string, boolean>>({}) // In catch block: setStatsError((prev) => ({ ...prev, [flagId]: true })) // Option 2: Show toast only for persistent failures // (requires tracking retry count)Alternatively, if silent failure is intended for auxiliary data, this is acceptable as-is.
519-520: Review exhaustive-deps suppressions periodically.Multiple effects have exhaustive-deps warnings disabled. While these appear intentional to avoid infinite loops or stale closures, they should be reviewed periodically to ensure they remain correct as the code evolves.
Consider adding comments explaining why each suppression is necessary:
// eslint-disable-next-line react-hooks/exhaustive-deps // Intentionally depends only on submit function to avoid recreating callback [listFetcher.submit],Also applies to: 698-699, 719-720, 726-727, 733-734
web/app/pages/Project/tabs/Captcha/CaptchaView.tsx (1)
60-60: The isMountedRef pattern works correctly.The implementation successfully prevents state updates after unmount. However, the
useEffectat lines 90-95 that setsisMountedRef.current = trueis redundant since the ref is already initialized totrueat line 60.💡 Optional: Consider using AbortController for modern async handling
Modern React favors
AbortControllerto cancel in-flight requests rather than theisMountedRefpattern:const CaptchaView = ({ projectId }: CaptchaViewProps) => { ... - const isMountedRef = useRef(true) const captchaProxy = useCaptchaProxy() ... - useEffect(() => { - isMountedRef.current = true - return () => { - isMountedRef.current = false - } - }, []) const loadCaptcha = async () => { + const abortController = new AbortController() setDataLoading(true) try { ... const data = await captchaProxy.fetchCaptcha(projectId, { timeBucket, period: period === 'custom' && dateRange ? '' : period, filters, from: period === 'custom' && dateRange ? from : '', to: period === 'custom' && dateRange ? to : '', + signal: abortController.signal, }) - if (!isMountedRef.current) return ... } catch (reason) { + if (abortController.signal.aborted) return - if (!isMountedRef.current) return ... } } useEffect(() => { + const controller = new AbortController() loadCaptcha() + return () => controller.abort() }, [filters, dateRange, period, timeBucket, projectId])Note: This assumes
captchaProxy.fetchCaptchaaccepts anAbortSignaloption. If not, the current isMountedRef approach is acceptable.Also applies to: 90-95, 126-126, 157-157
web/app/pages/Project/View/components/ProjectSidebar.tsx (1)
120-128: Optional: drop theexhaustive-depssuppression (or justify it).The effect reads
isExpandedbut omits it from deps; it’s probably fine here, but it’s also low-risk to include and remove the suppression to avoid future footguns.web/app/pages/Project/tabs/Sessions/SessionsView.tsx (1)
193-199: Review the disabled exhaustive-deps rule.The
react-hooks/exhaustive-depsrule is disabled. Whilerevalidator.revalidateis likely stable, explicitly including it in the dependency array (or documenting why it's safe to omit) would be clearer.♻️ Optional improvement
useEffect(() => { if (sessionsRefreshTrigger > 0) { revalidator.revalidate() } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [sessionsRefreshTrigger]) + }, [sessionsRefreshTrigger, revalidator])web/app/pages/Project/tabs/Experiments/ExperimentsView.tsx (4)
51-89: Import types from api.server.ts instead of duplicating them.These type definitions (
ExperimentStatus,ExposureTrigger,MultipleVariantHandling,FeatureFlagMode,ExperimentVariant,Experiment) duplicate the types already defined inweb/app/api/api.server.ts(lines 1325-1358). Maintaining duplicate type definitions creates two sources of truth and increases maintenance burden.♻️ Refactor to import types
+import type { + Experiment, + ExperimentStatus, + ExperimentVariant, + ExposureTrigger, + MultipleVariantHandling, + FeatureFlagMode, +} from '~/api/api.server' + const DEFAULT_EXPERIMENTS_TAKE = 20 - -type ExperimentStatus = 'draft' | 'running' | 'paused' | 'completed' - -type ExposureTrigger = 'feature_flag' | 'custom_event' - -type MultipleVariantHandling = 'exclude' | 'first_exposure' - -type FeatureFlagMode = 'create' | 'link' - -interface ExperimentVariant { - id?: string - name: string - key: string - description?: string | null - rolloutPercentage: number - isControl: boolean -} - -interface Experiment { - id: string - name: string - description: string | null - hypothesis: string | null - status: ExperimentStatus - // Exposure criteria - exposureTrigger: ExposureTrigger - customEventName: string | null - multipleVariantHandling: MultipleVariantHandling - filterInternalUsers: boolean - // Feature flag configuration - featureFlagMode: FeatureFlagMode - featureFlagKey: string | null - startedAt: string | null - endedAt: string | null - pid: string - goalId: string | null - featureFlagId: string | null - variants: ExperimentVariant[] - created: string -}
378-393: Consider adding request tracking to prevent race conditions.The list fetcher response handler doesn't track request IDs or timestamps, which could lead to race conditions if responses arrive out of order. For example, if a user quickly changes pages or filters, an older response might overwrite newer data.
Consider one of these approaches:
- Add a request ID to each submission and verify it in the response
- Track the latest request timestamp and ignore stale responses
- Use React Router's built-in navigation state to cancel stale requests
Example approach with request ID:
const requestIdRef = useRef(0) const loadExperiments = useCallback( (take: number, skip: number, showLoading = true, search?: string) => { if (showLoading) { setIsLoading(true) } requestIdRef.current += 1 const currentRequestId = requestIdRef.current listFetcher.submit( { intent: 'get-project-experiments', requestId: String(currentRequestId), take: String(take), skip: String(skip), search: search || '', }, { method: 'POST' }, ) }, [listFetcher.submit], ) // In the response handler useEffect: useEffect(() => { if (listFetcher.data?.intent === 'get-project-experiments') { const responseId = Number(listFetcher.data.requestId) if (responseId !== requestIdRef.current) { // Stale response, ignore it return } // ... rest of handler } }, [listFetcher.data])
395-435: Simplify action response handler and extract repeated reload logic.This useEffect has several issues:
- Code duplication: Lines 409, 416, 423, and 430 all call
loadExperimentswith identical parameters- Fragile deduplication: The uniqueSuffix logic (lines 401-402) assumes
requestIdortimestampexist inresponseData, which may not be guaranteed- Complex dependency array: The large dependency array (line 435) makes this effect prone to re-execution
♻️ Refactor to reduce duplication and simplify
useEffect(() => { if (!actionFetcher.data || actionFetcher.state !== 'idle') return const { intent, success, error } = actionFetcher.data - const responseData = actionFetcher.data as Record<string, unknown> - const uniqueSuffix = responseData.requestId ?? responseData.timestamp ?? actionFetcher.state - const actionKey = `${intent}-${success ?? false}-${error ?? ''}-${uniqueSuffix}-${JSON.stringify(data)}` + // Use a simpler unique key based on fetcher's key (React Router provides this) + const actionKey = `${intent}-${success}-${error}-${actionFetcher.key}` if (processedActionRef.current === actionKey) return processedActionRef.current = actionKey + const reloadList = () => { + loadExperiments(DEFAULT_EXPERIMENTS_TAKE, (page - 1) * DEFAULT_EXPERIMENTS_TAKE, true, filterQuery || undefined) + } + + const actionMessages: Record<string, { success: string; error?: string }> = { + 'delete-experiment': { success: t('experiments.deleted') }, + 'start-experiment': { success: t('experiments.started') }, + 'pause-experiment': { success: t('experiments.paused') }, + 'complete-experiment': { success: t('experiments.completed') }, + } + + if (intent && actionMessages[intent]) { + if (success) { + toast.success(actionMessages[intent].success) + reloadList() + } else if (error) { + toast.error(error) + } + } - if (actionFetcher.data.intent === 'delete-experiment') { - if (actionFetcher.data.success) { - toast.success(t('experiments.deleted')) - loadExperiments(DEFAULT_EXPERIMENTS_TAKE, (page - 1) * DEFAULT_EXPERIMENTS_TAKE, true, filterQuery || undefined) - } else if (actionFetcher.data.error) { - toast.error(actionFetcher.data.error) - } - } else if (actionFetcher.data.intent === 'start-experiment') { - // ... similar blocks - } }, [actionFetcher.data, actionFetcher.state, t, loadExperiments, page, filterQuery])Note: If
actionFetcher.keyis not available, you could track a request counter in the action handlers instead of relying on fields that may not exist in the response.
505-535: Event handlers correctly submit actions via fetcher.The pattern of resetting
processedActionRefbefore each submission ensures the response handler can process the result. The implementation is consistent across all action handlers.Optional: Consider extracting the repeated pattern into a helper:
const createActionHandler = useCallback( (intent: string) => (experimentId: string) => { processedActionRef.current = null actionFetcher.submit({ intent, experimentId }, { method: 'POST' }) }, [actionFetcher], ) const handleDeleteExperiment = createActionHandler('delete-experiment') const handleStartExperiment = createActionHandler('start-experiment') const handlePauseExperiment = createActionHandler('pause-experiment') const handleCompleteExperiment = createActionHandler('complete-experiment')web/app/pages/Project/tabs/Performance/PerformanceView.tsx (2)
126-141: Improve type safety for state declarations.The
panelsDataandchartDatastate variables useanytypes. Consider using more specific types derived fromPerformanceDataResponseto improve type safety and catch potential errors at compile time.💡 Suggested typing improvements
- const [panelsData, setPanelsData] = useState<any>(() => { + const [panelsData, setPanelsData] = useState<{ types?: string[]; data: Record<string, { name: string; count: number }[]> }>(() => { if (deferredData.perfData?.params) { return { types: _keys(deferredData.perfData.params), data: deferredData.perfData.params, } } return { data: {} } }) - const [chartData, setChartData] = useState<any>(() => deferredData.perfData?.chart || {}) + const [chartData, setChartData] = useState<PerformanceDataResponse['chart'] | {}>(() => deferredData.perfData?.chart || {})
181-182: Document or conditionally disable comparison UI.The comparison mode is stubbed with empty defaults (see comment on line 83), but the UI still renders comparison-related components (lines 441, 453). Consider either:
- Conditionally hiding comparison UI elements when SSR data is in use, or
- Adding a TODO comment here to track the incomplete implementation
This ensures users aren't confused by non-functional UI elements.
web/app/pages/Project/tabs/Traffic/TrafficView.tsx (3)
530-593: Consider improving type safety and simplifying alignment logic.The revenue data loading and alignment logic is functional but has a few areas for improvement:
Line 558: Type assertion
as { x?: string[] }suggests thebaseChartDatatype doesn't include thexproperty. Consider updating the type definition instead of casting.Lines 562-576: The x-axis alignment logic could be simplified and made more readable by extracting it into a helper function.
♻️ Proposed refactor to improve readability
Extract the alignment logic into a helper function:
// Helper function to align revenue data to chart x-axis function alignRevenueToChart( chartX: string[], revenueX: string[], revenueY: any[], refunds: any[] ): { revenue: number[]; refundsAmount: number[] } { // Exact match - no alignment needed if (revenueX.length === chartX.length && revenueX[0] === chartX[0]) { return { revenue: revenueY.map((v) => Number(v ?? 0)), refundsAmount: refunds.map((v) => Number(v ?? 0)), } } // Map-based alignment for different x-axes const revenueByX = new Map(revenueX.map((x, i) => [x, Number(revenueY[i] ?? 0)])) const refundsByX = new Map(revenueX.map((x, i) => [x, Number(refunds[i] ?? 0)])) return { revenue: chartX.map((x) => revenueByX.get(x) ?? 0), refundsAmount: chartX.map((x) => refundsByX.get(x) ?? 0), } } // Then in loadRevenueData: if (Array.isArray(chart?.x) && chart.x.length > 0 && Array.isArray(revX) && revX.length > 0) { const aligned = alignRevenueToChart(chart.x, revX, revY, revRefunds) setRevenueOverlay(aligned) }
682-684: Optional: Remove redundant dependency.The effect includes both
loadRevenueDataandfetchRevenueDatain the dependency array. SinceloadRevenueDataalready depends onfetchRevenueData(line 592), includingfetchRevenueDatahere is redundant.♻️ Simplify dependency array
useEffect(() => { loadRevenueData() -}, [loadRevenueData, fetchRevenueData]) +}, [loadRevenueData])
904-904: Improve type safety by defining proper chartData type.The type assertion
(chartData as any).xbypasses TypeScript's type checking. SincechartDatais derived from mergingbaseChartDataandrevenueOverlay, consider defining a proper interface that includes thexproperty.♻️ Define proper type for chartData
Define an interface at the top of the file:
interface ChartData { x?: string[] unique?: number[] total?: number[] revenue?: number[] refundsAmount?: number[] // ... other properties }Then update the useMemo at lines 260-265:
-const chartData = useMemo(() => { +const chartData: ChartData = useMemo(() => { if (revenueOverlay) { return { ...baseChartData, ...revenueOverlay } } return baseChartData }, [baseChartData, revenueOverlay])And remove the type assertion at line 904:
-<div onContextMenu={(e) => handleChartContextMenu(e, (chartData as any).x)} className='relative'> +<div onContextMenu={(e) => handleChartContextMenu(e, chartData.x)} className='relative'>web/app/pages/Project/tabs/Errors/ErrorsView.tsx (3)
469-474: Minor: Inconsistent initial skip value logic.The initial
errorsSkipis set todeferredData.errorsData?.errors?.length || 0, but the effect at line 527 immediately overrides it toERRORS_TAKE. Consider initializing toERRORS_TAKEdirectly for consistency, or document why the actual length is used initially.
555-570: Consider guarding against concurrent proxy fetches.The guard
if (errorsLoading) returncheckserrorsLoadingwhich is tied torevalidator.state, noterrorsProxy.isLoading. If the user rapidly clicks "Load More", concurrent proxy fetches could occur. Consider also checkingerrorsProxy.isLoading:🔧 Suggested fix
const loadMoreErrors = useCallback(() => { - if (errorsLoading) return + if (errorsLoading || errorsProxy.isLoading) return errorsProxy.fetchErrors(id, {
1019-1019: Dead code:overviewLoadingis never null.
overviewLoadingis defined asrevalidator.state === 'loading'(line 465), which is always a boolean. TheoverviewLoading === nullcheck is dead code.🧹 Suggested cleanup
-if ((overviewLoading === null || overviewLoading) && !overview && _isEmpty(errors)) { +if (overviewLoading && !overview && _isEmpty(errors)) {web/app/pages/Project/tabs/Profiles/ProfilesView.tsx (1)
74-96: Type safety: avoidas unknown as ...by typing proxy responses.Casting
(dataProfiles?.profiles || []) as unknown as Profile[]and similar for sessions/details makes it easy for subtle API contract drift to become runtime bugs (especially with the new proxy layer).If the proxy hooks can return
ProxyResponse<T>already, consider makingfetchProfiles/fetchProfile/fetchProfileSessionsgeneric/typed so local state setters don’t need unsafe casts.Also applies to: 98-116, 189-217
web/app/pages/Project/tabs/Goals/GoalsView.tsx (1)
456-457: Consider separate fetchers for “list” vs “delete” intents.Reusing one
fetcherfor both list pagination/search and delete increases coupling and makes it easier to introduce state edge-cases (e.g., delete in-flight blocking list loads due tofetcher.state !== 'idle').Also applies to: 517-533, 675-695
web/app/pages/Project/tabs/Funnels/FunnelsView.tsx (1)
218-236: TheactiveFunnelsync effect relies on an exhaustive-deps disable; consider restructuring to avoid stale reads.
Right now it only keys offactiveFunnel?.idwhile readingproject,deferredData.funnelData, andrevalidator; it’s easy to miss updates when those change without an id change.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
web/app/pages/Project/View/Panels.tsxweb/app/pages/Project/View/ViewProject.tsxweb/app/pages/Project/View/components/ProjectSidebar.tsxweb/app/pages/Project/tabs/Captcha/CaptchaView.tsxweb/app/pages/Project/tabs/Errors/ErrorsView.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/GoalsView.tsxweb/app/pages/Project/tabs/Performance/PerformanceView.tsxweb/app/pages/Project/tabs/Profiles/ProfilesView.tsxweb/app/pages/Project/tabs/Sessions/SessionsView.tsxweb/app/pages/Project/tabs/Traffic/TrafficView.tsx
🧰 Additional context used
🧬 Code graph analysis (8)
web/app/pages/Project/tabs/Captcha/CaptchaView.tsx (3)
web/app/pages/Project/View/ViewProject.tsx (2)
RefreshTriggersContext(225-225)ViewProjectContext(224-224)web/app/hooks/useAnalyticsProxy.ts (1)
useCaptchaProxy(230-258)web/app/pages/Project/View/ViewProject.helpers.tsx (1)
getFormatDate(2204-2204)
web/app/pages/Project/tabs/Experiments/ExperimentsView.tsx (3)
web/app/api/api.server.ts (5)
ExposureTrigger(1326-1326)MultipleVariantHandling(1328-1328)FeatureFlagMode(1330-1330)ExperimentVariant(1332-1339)Experiment(1341-1359)web/app/pages/Project/View/ViewProject.tsx (2)
useRefreshTriggers(232-235)useViewProjectContext(227-230)web/app/routes/projects.$id.tsx (1)
ProjectViewActionData(370-375)
web/app/pages/Project/View/Panels.tsx (3)
web/app/hooks/useAnalyticsProxy.ts (1)
useProjectDataCustomEventsProxy(715-740)web/app/lib/constants/index.ts (1)
PROJECT_TABS(408-410)web/app/pages/Project/View/ViewProject.tsx (1)
useViewProjectContext(227-230)
web/app/pages/Project/tabs/Performance/PerformanceView.tsx (5)
web/app/api/api.server.ts (2)
PerformanceDataResponse(694-709)PerformanceOverallObject(735-741)web/app/routes/projects.$id.tsx (1)
ProjectLoaderData(151-179)web/app/providers/CurrentProjectProvider.tsx (1)
useCurrentProject(235-243)web/app/pages/Project/View/ViewProject.tsx (1)
useRefreshTriggers(232-235)web/app/pages/Project/View/ViewProject.helpers.tsx (2)
CHART_METRICS_MAPPING_PERF(2212-2212)CHART_MEASURES_MAPPING_PERF(2220-2220)
web/app/pages/Project/tabs/Errors/ErrorsView.tsx (5)
web/app/api/api.server.ts (3)
ErrorsResponse(891-893)ErrorDetailsResponse(921-942)ErrorOverviewResponse(970-984)web/app/routes/projects.$id.tsx (2)
ProjectLoaderData(151-179)ProjectViewActionData(370-375)web/app/hooks/useAnalyticsProxy.ts (1)
useErrorsProxy(82-108)web/app/pages/Project/View/ViewProject.tsx (2)
useRefreshTriggers(232-235)useViewProjectContext(227-230)web/app/lib/models/Project.ts (2)
SwetrixError(74-84)SwetrixErrorDetails(86-91)
web/app/pages/Project/tabs/Traffic/TrafficView.tsx (5)
web/app/hooks/useAnalyticsProxy.ts (4)
useCustomEventsMetadataProxy(595-620)usePropertyMetadataProxy(622-647)useGSCKeywordsProxy(772-800)useRevenueProxy(802-848)web/app/pages/Project/View/ViewProject.tsx (1)
useRefreshTriggers(232-235)web/app/lib/constants/index.ts (1)
isSelfhosted(309-309)web/app/pages/Project/View/ViewProject.helpers.tsx (1)
getFormatDate(2204-2204)backend/apps/cloud/src/analytics/analytics.service.ts (1)
getCustomEventMetadata(4047-4140)
web/app/pages/Project/tabs/Funnels/FunnelsView.tsx (3)
web/app/api/api.server.ts (1)
FunnelDataResponse(1241-1244)web/app/routes/projects.$id.tsx (2)
ProjectLoaderData(151-179)ProjectViewActionData(370-375)web/app/pages/Project/View/ViewProject.tsx (2)
useRefreshTriggers(232-235)useViewProjectContext(227-230)
web/app/pages/Project/tabs/Goals/GoalsView.tsx (5)
web/app/api/api.server.ts (2)
GoalsResponse(1133-1136)Goal(1121-1131)web/app/routes/projects.$id.tsx (1)
ProjectLoaderData(151-179)web/app/providers/CurrentProjectProvider.tsx (1)
useCurrentProject(235-243)web/app/pages/Project/View/ViewProject.tsx (2)
useRefreshTriggers(232-235)useViewProjectContext(227-230)web/app/hooks/useAnalyticsProxy.ts (2)
useGoalStatsProxy(170-198)useGoalChartProxy(200-228)
🪛 GitHub Actions: Pull Request Validation Workflow
web/app/pages/Project/View/components/ProjectSidebar.tsx
[error] 111-111: eslint: react/display-name - Component definition is missing display name.
🪛 GitHub Check: Web Checks (22.x.x)
web/app/pages/Project/View/components/ProjectSidebar.tsx
[failure] 111-111:
Component definition is missing display name
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (49)
web/app/pages/Project/tabs/FeatureFlags/FeatureFlagsView.tsx (6)
23-33: LGTM: SSR imports properly structured.The new imports correctly support the loader/fetcher pattern and analytics proxies.
52-53: LGTM: Pagination constants well-defined.
424-442: LGTM: Correct Suspense and deferred data pattern.The wrapper/resolver pattern correctly implements React's
use()hook for promise unwrapping within a Suspense boundary.
493-498: State synchronization logic looks correct.The effect properly syncs loader data to local state when not in search mode, avoiding conflicts between loader-provided and fetcher-provided data.
760-774: LGTM: Action handlers properly structured.Both
handleDeleteFlagandhandleToggleFlagcorrectly resetprocessedActionRefbefore submitting to ensure the action is processed.
894-896: LGTM: Public API preserved with SSR wrapper.The default export maintains backward compatibility while implementing the SSR pattern internally.
web/app/pages/Project/tabs/Captcha/CaptchaView.tsx (1)
110-124: LGTM! Custom date range handling is correct.The conditional parameter logic properly handles two modes:
- Custom range mode (
period === 'custom'anddateRangeexists): sends emptyperiodwith populatedfrom/todates- Preset period mode (all other cases): sends the
periodvalue with emptyfrom/toThis ensures the backend receives mutually exclusive parameters and avoids ambiguity.
web/app/pages/Project/View/components/ProjectSidebar.tsx (1)
623-640: No concerns here.tabsis already memoized withuseMemodependencies[t, projectQueryTabs, allowedToManage]and uses spread operators to create new arrays, ensuring reference stability.searchParamsis never mutated in-place—all usages create freshURLSearchParamsinstances before modifications. The memo comparator's assumptions are valid.Likely an incorrect or invalid review comment.
web/app/pages/Project/tabs/Sessions/SessionsView.tsx (7)
52-55: LGTM!The
DeferredSessionsDatainterface correctly models the deferred loader data with nullable types.
67-75: LGTM!The wrapper correctly establishes a Suspense boundary with an appropriate loading fallback.
92-97: LGTM!State initialization properly uses lazy initialization and safely handles missing loader data.
156-167: LGTM!The proxy response handling correctly implements pagination by appending sessions and updating pagination state. Error handling is appropriately included.
169-191: LGTM!The
loadMoreSessionsfunction correctly constructs request parameters and uses the proxy for pagination with proper loading guards.
201-202: LGTM!The session detail loading state correctly derives from the revalidator when viewing a specific session.
277-279: LGTM!The export pattern maintains backward compatibility while delegating to the new wrapper implementation.
web/app/pages/Project/tabs/Experiments/ExperimentsView.tsx (3)
20-20: LGTM! New imports support the fetcher-based refactor.The added imports for
useFetcher,useRefreshTriggers, andProjectViewActionDataare correctly used throughout the component.Also applies to: 24-24, 26-26
322-323: LGTM! Fetcher setup and context hooks are correctly initialized.The dual-fetcher pattern (one for listing, one for actions) is a good approach to separate concerns and avoid state conflicts.
Also applies to: 327-328
358-376: LGTM! Fetcher-based data loading is correctly implemented.The refactored
loadExperimentscorrectly submits to the list fetcher with appropriate parameters.web/app/pages/Project/tabs/Performance/PerformanceView.tsx (7)
61-69: LGTM!The Suspense boundary with fallback correctly handles the asynchronous data loading from the
use()hook inPerfDataResolver. This follows the SSR/loader pattern described in the PR objectives.
75-78: LGTM!The component correctly uses
useRevalidatorfor loader-based refresh patterns and receivesdeferredDatafrom the wrapper, aligning with the SSR architecture.
146-161: LGTM!The effect correctly syncs loader-provided data to local state when the revalidator completes, and sets loading state appropriately. The dependency array is correct for this use case.
297-302: LGTM!The refresh trigger correctly uses
revalidator.revalidate()to refetch data through the loader instead of making direct API calls. This aligns with the SSR/loader architecture introduced in this PR.
679-679: LGTM!The export structure maintains the same public API while delegating to the new wrapper component, ensuring backward compatibility for consumers of this module.
164-165: Verify URL parameter defaults match server loader expectations.The client-side defaults for
perfMetricandmeasureare set toCHART_METRICS_MAPPING_PERF.timingandCHART_MEASURES_MAPPING_PERF.medianrespectively. Confirm that the server-side loader inprojects.$id.tsxuses the same defaults when these URL parameters are absent, to prevent SSR/client hydration mismatches.
52-59: Error handling is properly configured.An error boundary exists at the app root level (
web/app/root.tsx, lines 86-105) and will catch any errors thrown by theuse()hook ifperfDataPromiseorperfOverallStatsPromisereject. The Suspense boundary with theLoaderfallback handles the pending state. The implementation is correct.web/app/pages/Project/tabs/Traffic/TrafficView.tsx (6)
212-270: LGTM! Clean loader-driven data flow.The data derivation from deferred loader data is well-structured with proper memoization. The separation of base chart data and revenue overlay, followed by merging, keeps concerns separated and makes the data flow easier to follow.
285-288: LGTM! URL-based state management is SSR-friendly.Migrating custom events selection to URL parameters is a solid improvement that makes state shareable and SSR-compatible. The implementation properly handles adding/removing events from the comma-separated list.
Also applies to: 500-520
595-628: LGTM! Metadata fetching properly migrated to proxy hooks.Both
getCustomEventMetadataand_getPropertyMetadatacorrectly use the new proxy hooks and handle custom vs. predefined periods appropriately.
838-850: No issue found. Theclasses.labelprop is valid for the Checkbox component—it's properly defined in the component's prop interface and correctly applies styling to the checkbox container element.
95-95: No breaking change found—all call sites are compatible with the current signature.The
loadProjectViewssignature(forced?: boolean) => voidis the current state, and all call sites (in TrafficHeaderActions.tsx and ViewProject.tsx) use it without awaiting or chaining.then(). The function is called directly asloadProjectViews()orloadProjectViews(true), which is compatible with the void return type. There are no incompatible usages to update.Likely an incorrect or invalid review comment.
109-125: Theusehook is stable in React 19.2.3 and the error handling is already in place. A rootErrorBoundary()is exported inroot.tsxthat catches promise rejections. The current architecture—Suspense with a fallback handling pending state, combined with the root-level error boundary catching errors—follows the standard React Router pattern and is sufficient. No additional error boundary is required.Likely an incorrect or invalid review comment.
web/app/pages/Project/tabs/Errors/ErrorsView.tsx (12)
400-404: LGTM - Well-defined deferred data interface.The interface correctly types the resolved promise data from the loader, matching the
ProjectLoaderDatastructure.
406-418: LGTM - Correct use of React'suse()hook.The conditional
use()calls are valid sinceuse()is explicitly designed to be called conditionally, unlike other hooks. The Suspense boundary in the wrapper ensures proper suspension handling.
420-426: LGTM - Clean Suspense composition.The wrapper correctly establishes a Suspense boundary for the deferred data resolution, with an appropriate fallback.
447-449: LGTM - Proper fetcher state management pattern.Using refs to track pending status and last handled data prevents duplicate processing of fetcher results, which is a correct pattern for handling async state transitions.
519-534: LGTM - One-time initialization pattern.The
initialDataProcessed.currentref correctly ensures initialization logic runs only once per mount.
536-553: LGTM - Proper revalidation state synchronization.The effect correctly resets to the first page of errors when revalidation completes, which is expected behavior when filters or date ranges change.
601-623: LGTM - Robust fetcher response handling.The
lastHandledStatusData.currentguard effectively prevents duplicate processing on re-renders, and thependingStatusUpdate.currentcorrectly tracks the intended state change.
625-643: LGTM - Clean mutation pattern with fetcher.The intent-based form submission pattern is idiomatic for Remix/React Router actions, with proper guards against concurrent submissions.
664-670: LGTM - Correct external refresh trigger handling.The guard
errorsRefreshTrigger > 0prevents revalidation on mount, andrevalidatoris correctly omitted from deps as it provides a stable reference.
1128-1128: LGTM - Correct delegation to proxy-based pagination.
1146-1148: LGTM - Backward-compatible export.The alias maintains the existing export interface while internally switching to the new wrapper implementation.
19-35: LGTM - Imports align with deferred data pattern.The
use()hook from React 19 is correctly imported for unwrapping promises within Suspense boundaries. The project uses React 19.2.3, which fully supports this feature. The hook is actively used on lines 413-415 for deferred data loading. Router hooks and proxy imports properly support the new SSR/loader-based architecture.web/app/pages/Project/View/Panels.tsx (1)
1326-1342: Nice:activeTab/dataLoadingnow flow from context into Panel/DetailsTable.This makes the numeric formatting branch (
PROJECT_TABS.traffic=>nFormatter) deterministic from the current view context without extra plumbing at call sites.Also applies to: 1370-1371, 1670-1676, 1922-1923
web/app/pages/Project/View/ViewProject.tsx (1)
150-235: Good split: refresh triggers moved to a dedicated context/provider.This reduces churn in
ViewProjectContextTypeand avoids rerender pressure on unrelated consumers when refresh counters change.Also applies to: 1039-1064, 1205-1207, 1444-1446
web/app/pages/Project/tabs/Goals/GoalsView.tsx (1)
20-23: No action required. The codebase uses React 19.2.3, which has stable support for theuse()hook. The implementation at lines 425-444 correctly usesuse(goalsDataPromise)within a Suspense boundary, following the proper React 19 pattern for unwrapping promises from loaders.web/app/pages/Project/tabs/Funnels/FunnelsView.tsx (3)
165-187: Verify password-protected project behavior for create/update/delete intents.
get-funnelsincludes{ password: projectPassword }, but create/update/delete do not—if actions require password for embedded/password flows, these intents may regress.
422-424: Wrapper-based default export looks fine.
Makes the Suspense boundary explicit and keepsFunnelsViewInnerfocused on UI/state.
7-46: Remove this review—theuse()hook is correctly implemented and is the standard pattern in this app.React 19.2.3 and React Router 7.11.0 both natively support and recommend the
use()hook for consuming deferred loader data. The codebase consistently uses this pattern across all view components (Traffic, Performance, Sessions, Goals, FeatureFlags, Errors, Alerts), makingFunnelsViewcompliant with the established architecture. Switching to<Await>would deviate from project conventions without benefit.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @web/app/api/api.server.ts:
- Around line 1525-1532: The query construction double-encodes profileId: remove
the encodeURIComponent wrapper around profileId in the URLSearchParams call (the
const params = new URLSearchParams({...}) block) so URLSearchParams performs the
encoding once; make the same change inside getProfileSessionsServer (the
analogous URLSearchParams usage) to avoid double-encoding and lookup failures
for IDs with special characters.
- Around line 132-140: The retry branch assumes retryResponse is JSON and
directly calls retryResponse.json(); modify the retry path (where retryResponse
and refreshResult.cookies are used) to inspect
retryResponse.headers.get("content-type") (or similar) and only call .json()
when the content-type indicates JSON, otherwise call .text() and set data
accordingly so the returned object mirrors the non-retry path (data, error:
null, status: retryResponse.status, cookies: refreshResult.cookies). Ensure you
handle empty bodies consistently and preserve the same error/null typing as the
non-retry handling.
- Around line 236-260: The streamingServerFetch code path must mirror
serverFetch’s behavior when token refresh fails: after tryRefreshToken returns a
failed result, clear the auth cookies before returning so the client isn't left
in a stale auth state. Concretely, inside the if ((response.status === 401 ||
response.status === 403) && !skipAuth) branch, handle the non-success case from
tryRefreshToken by appending the same "Set-Cookie" headers used to clear auth
(the logic used in serverFetch to expire/clear access and refresh tokens) to the
response you return (use the same cookie names/values or call the same
clear-cookie helper if one exists), then return that response. Ensure you
reference tryRefreshToken, refreshResult, and the response/finalResponse objects
when adding the clear-cookie headers so behavior is consistent with serverFetch.
In @web/app/pages/Project/tabs/Errors/ErrorsView.tsx:
- Around line 536-553: The effect in ErrorsView uses deferredData as a
dependency but deferredData is recreated each render by ErrorsDataResolver,
causing unnecessary runs; either stabilize deferredData by wrapping it in
useMemo inside ErrorsDataResolver (so the object identity only changes when its
contained data changes) or remove deferredData from the useEffect dependency
array in ErrorsView and rely only on revalidator.state to trigger the sync;
update the code referencing deferredData and ErrorsDataResolver accordingly,
ensuring setErrors, setErrorsSkip, setCanLoadMoreErrors and setErrorsLoading
still run when actual deferredData.errorsData changes.
In @web/app/pages/Project/tabs/Sessions/SessionsView.tsx:
- Around line 163-174: The effect handling sessionsProxy may append duplicate
sessions if loader revalidation resets sessions while a proxy response is
in-flight; update the effect so when adding newSessions from sessionsProxy.data
you deduplicate against current sessions (e.g., compute ids from existing
sessions state setSessions and only append items whose id is not already
present) and still update sessionsSkip and canLoadMoreSessions accordingly;
additionally when loader data arrives (where you reset sessions and sessionsSkip
to SESSIONS_TAKE) ensure you clear or ignore stale proxy responses by either
resetting any proxy state if available or comparing a last-updated marker before
applying sessionsProxy results so only responses newer than the loader are
appended.
🧹 Nitpick comments (10)
web/app/pages/Project/tabs/Sessions/SessionsView.tsx (3)
100-110: Type assertion bypasses type safety.The double cast
as unknown as SessionDetailsTypeon line 103 suggests a structural mismatch betweenSessionDetailsResponse.detailsandSessionDetailsType. This can mask runtime errors if the types diverge.Consider aligning the server response type with the client-side type, or adding a mapping function that validates/transforms the shape explicitly.
116-123:isMountedRefis unused.The ref is declared and cleaned up but never read. This appears to be dead code left over from a previous implementation where async callbacks checked mount status before updating state.
🧹 Remove unused ref
- const isMountedRef = useRef(true) - // Track if we've ever shown actual content to prevent NoSessions flash during exit animation const hasShownContentRef = useRef(false)And remove the cleanup effect at lines 145-151:
- // Cleanup on unmount - useEffect(() => { - isMountedRef.current = true - return () => { - isMountedRef.current = false - } - }, [])
253-256: Redundant null check onsessionsLoading.
sessionsLoadingis derived asrevalidator.state === 'loading' || sessionsProxy.isLoading(line 112), which is always a boolean. ThesessionsLoading === nullcheck here and on line 266 will never be true.🧹 Simplify conditions
- {(sessionsLoading === null || sessionsLoading) && _isEmpty(sessions) ? <Loader /> : null} - {typeof sessionsLoading === 'boolean' && !sessionsLoading && _isEmpty(sessions) && !hasShownContentRef.current ? ( + {sessionsLoading && _isEmpty(sessions) ? <Loader /> : null} + {!sessionsLoading && _isEmpty(sessions) && !hasShownContentRef.current ? ( <NoSessions filters={filters} /> ) : null}Also at line 266:
{ - 'cursor-not-allowed opacity-50': sessionsLoading || sessionsLoading === null, + 'cursor-not-allowed opacity-50': sessionsLoading, hidden: sessionsLoading && _isEmpty(sessions), },web/app/pages/Project/tabs/Performance/PerformanceView.tsx (1)
143-154: Render-phase side effect: consider moving ref mutation to an effect.Mutating
hasShownContentRef.currentduring render (lines 149-151) is a side effect that could cause issues in React's concurrent mode. While it's idempotent in this case, it's safer to move this to auseEffect.♻️ Suggested refactor
- // Track if we've ever shown actual content to prevent NoEvents flash during exit animation - const hasShownContentRef = useRef(false) - - const isPanelsDataEmptyRaw = _isEmpty(panelsData.data) - - // Track when we've shown content to prevent NoEvents flash during exit animation - if (!isPanelsDataEmptyRaw) { - hasShownContentRef.current = true - } - - // Don't show NoEvents if we've previously shown content (prevents flash during tab switch) - const isPanelsDataEmpty = isPanelsDataEmptyRaw && !hasShownContentRef.current + // Track if we've ever shown actual content to prevent NoEvents flash during exit animation + const hasShownContentRef = useRef(false) + + const isPanelsDataEmptyRaw = _isEmpty(panelsData.data) + + // Track when we've shown content to prevent NoEvents flash during exit animation + useEffect(() => { + if (!isPanelsDataEmptyRaw) { + hasShownContentRef.current = true + } + }, [isPanelsDataEmptyRaw]) + + // Don't show NoEvents if we've previously shown content (prevents flash during tab switch) + const isPanelsDataEmpty = isPanelsDataEmptyRaw && !hasShownContentRef.currentweb/app/api/api.server.ts (1)
652-652: Manual JSON array construction is fragile.Building the JSON array manually with string interpolation could produce malformed JSON if a
pidcontains special characters (e.g., quotes, backslashes). The same pattern appears ingetPerformanceOverallStatsServer(line 758) andgetLiveVisitorsServer(line 1790).Proposed fix
- const pidsParam = `[${pids.map((pid) => `"${pid}"`).join(',')}]` + const pidsParam = JSON.stringify(pids)Apply similar change to lines 758 and 1790.
web/app/pages/Project/tabs/Errors/ErrorsView.tsx (2)
469-474: Minor inconsistency inerrorsSkipinitialization.The initial state for
errorsSkipis set to the length of loaded errors (line 471), but the effect on line 527 resets it toERRORS_TAKE. This inconsistency doesn't cause runtime issues since the effect runs immediately, but it could be cleaner to initialize directly toERRORS_TAKEfor clarity.♻️ Suggested fix
- const [errorsSkip, setErrorsSkip] = useState(() => deferredData.errorsData?.errors?.length || 0) + const [errorsSkip, setErrorsSkip] = useState(ERRORS_TAKE)
622-623: Verify the eslint-disable is intentional.The exhaustive deps rule is disabled here. The omitted dependencies (
updateStatusInErrors,revalidator) are likely stable references, but it's worth confirming this is intentional to avoid stale closure bugs.Consider documenting why deps are omitted or wrapping
updateStatusInErrorswithuseCallbackthat has stable dependencies (it already hasactiveError?.details?.eid).web/app/pages/Project/tabs/Traffic/TrafficView.tsx (3)
479-482: Confusing array spreading into object—consider clearer logic.Spreading
activeChartMetricsCustomEvents(an array) into an object produces{0: 'eventA', 1: 'eventB', ...}, which works because non-empty strings are truthy. However, this relies on subtle JavaScript behavior and obscures the intent.♻️ Suggested clearer implementation
const checkIfAllMetricsAreDisabled = useMemo( - () => !_some({ ...activeChartMetrics, ...activeChartMetricsCustomEvents }, (value) => value), + () => !_some(activeChartMetrics, (value) => value) && activeChartMetricsCustomEvents.length === 0, [activeChartMetrics, activeChartMetricsCustomEvents], )
590-592: Silent error catch—add logging for consistency.This catch block silently swallows errors, unlike similar handlers in the file (e.g., line 346 logs
[ERROR] (fetchRevenueStatus)). Consider logging the error to aid debugging.♻️ Suggested fix
- } catch { + } catch (error) { + console.error('[ERROR] (loadRevenueData) Fetching revenue data failed', error) setRevenueOverlay(null) }
915-915: Type assertion toanybypasses type safety.Consider defining a proper type for
chartDatathat includes thexproperty, rather than usingas any.♻️ Suggested approach
Define an interface for the chart data shape:
interface ChartData { x?: string[] // ... other chart properties revenue?: number[] refundsAmount?: number[] }Then use it in the useMemo:
-const chartData = useMemo(() => { +const chartData = useMemo((): ChartData => { if (revenueOverlay) { return { ...baseChartData, ...revenueOverlay } } return baseChartData -}, [baseChartData, revenueOverlay]) +}, [baseChartData, revenueOverlay]) as ChartDataThis allows you to remove the
as anycast:-<div onContextMenu={(e) => handleChartContextMenu(e, (chartData as any).x)} className='relative'> +<div onContextMenu={(e) => handleChartContextMenu(e, chartData.x)} className='relative'>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
web/app/api/api.server.tsweb/app/pages/Project/tabs/Errors/ErrorsView.tsxweb/app/pages/Project/tabs/Performance/PerformanceView.tsxweb/app/pages/Project/tabs/Sessions/SessionsView.tsxweb/app/pages/Project/tabs/Traffic/TrafficView.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
web/app/pages/Project/tabs/Performance/PerformanceView.tsx (6)
web/app/api/api.server.ts (2)
PerformanceDataResponse(704-719)PerformanceOverallObject(745-751)web/app/routes/projects.$id.tsx (1)
ProjectLoaderData(151-179)web/app/providers/CurrentProjectProvider.tsx (1)
useCurrentProject(235-243)web/app/pages/Project/View/ViewProject.tsx (1)
useRefreshTriggers(232-235)web/app/lib/models/Project.ts (1)
OverallPerformanceObject(30-44)web/app/pages/Project/View/ViewProject.helpers.tsx (2)
CHART_METRICS_MAPPING_PERF(2212-2212)CHART_MEASURES_MAPPING_PERF(2220-2220)
🔇 Additional comments (22)
web/app/pages/Project/tabs/Performance/PerformanceView.tsx (8)
47-50: LGTM!The
DeferredPerfDatainterface is well-defined and properly typed with nullable fields to handle cases where loader data may not be available.
52-59: LGTM!The
PerfDataResolvercorrectly uses React 19'suse()hook to unwrap promises from loader data within a Suspense boundary. The conditional check before callinguse()properly handles cases where the promises may not exist.
61-69: LGTM!Clean wrapper composition with proper Suspense boundary to handle the promise suspension from
use(). TheLoaderfallback maintains consistent UX during data resolution.
156-172: LGTM!The effect properly synchronizes local state with loader data when the revalidator completes. The conditional checks ensure state is only updated when meaningful data exists, and the loading state provides appropriate UX feedback.
191-193: Acknowledged: Compare mode deferred for future SSR implementation.The empty placeholders for
overallCompareandchartDataCompareare appropriate for now. Consider tracking this as a follow-up task if period comparison is a key feature.
307-313: LGTM!Clean integration with the refresh trigger system. Using
revalidator.revalidate()correctly delegates data refresh to the loader, maintaining the SSR-friendly pattern.
690-692: LGTM!The export assignment maintains backward compatibility while clearly indicating the component restructuring.
174-189: The implementation is correct as-is.The loader in
web/app/routes/projects.$id.tsxreadsperfMetricandmeasurefrom URL params (lines 231-232) and uses them directly for server-side data fetching when the performance tab is active (lines 296-300). The params are passed togetPerfDataServerandgetPerformanceOverallStatsServer, which fetch different data based on these values. Therefore, triggering loader revalidation viasetSearchParamsis necessary and correct—not unnecessary.web/app/api/api.server.ts (1)
1-12: Well-structured centralized API client.The file provides a comprehensive, strongly-typed server API surface with:
- Unified error handling and response typing via
ServerFetchResult<T>- Automatic token refresh with cookie propagation
- Client IP and User-Agent forwarding for proper request attribution
- Consistent password header handling for protected projects
Good foundation for SSR/proxy architecture.
web/app/pages/Project/tabs/Errors/ErrorsView.tsx (8)
19-30: LGTM!The imports are correctly updated for the new loader-driven SSR pattern. The
usehook from React 19 and the react-router hooks (useFetcher,useLoaderData,useRevalidator) are appropriate for this implementation.
400-418: LGTM!The
ErrorsDataResolvercomponent correctly leverages React 19'suse()hook, which unlike other hooks can be called conditionally. The null checks before callinguse()properly handle cases where the loader might not return certain promises.
420-426: LGTM!The Suspense boundary is correctly positioned to catch suspensions from the
use()calls inErrorsDataResolver. This enables proper streaming/deferred data hydration.
601-623: LGTM!The error status update handling correctly uses a ref guard (
lastHandledStatusData) to prevent reprocessing stale fetcher data. ThependingStatusUpdateref provides a clean way to track the expected state change without optimistic UI complexity.
680-690: Good fix for the flashing banner issue.The
hasShownContentRefpattern effectively prevents the NoEvents banner from flashing during tab switches or revalidation. This aligns with the commit message "fix: flashing no events banner".One consideration: since the ref never resets, if errors are genuinely cleared (e.g., all resolved and filtered out), the user won't see the NoEvents state until component remounts. This is likely acceptable given the UX improvement.
1028-1040: LGTM!The loading state handling correctly distinguishes between initial load (show full-page loader) and subsequent revalidation (show loading bar while preserving existing content). This provides a smooth user experience.
1156-1158: LGTM!The export pattern maintains backward compatibility by preserving the
ErrorsViewname while internally switching to the wrapper implementation. This ensures existing imports continue to work without changes.
572-583: The concern about stale data is not valid. TheerrorsProxy.datareference changes on each API fetch becausefetchErrorscallssetData(result.data)with a fresh response object from the server. The effect dependency onerrorsProxy.datais appropriate and will correctly trigger for each new pagination response.Regarding deduplication: the
if (errorsLoading) returnguard inloadMoreErrorsprevents concurrent requests, and each paginated API call returns distinct results based on theskipoffset. No practical scenario for duplicates exists here.The code is working as intended—no changes needed.
web/app/pages/Project/tabs/Traffic/TrafficView.tsx (5)
270-281: Good pattern for preventing UI flash during tab switches.The
hasShownContentRefpattern correctly prevents the "NoEvents" component from flashing when switching away from a tab that has data. This addresses the "flashing no events banner" issue mentioned in the PR.
127-135: Clean wrapper composition with Suspense.Good pattern for separating data resolution from rendering. The
TrafficDataResolverhandles promise unwrapping viause(), whileTrafficViewWrapperprovides the Suspense boundary with a loading fallback.
511-531: URL-driven custom events toggle is well implemented.Clean logic for managing the comma-separated
customEventsURL parameter with proper cleanup when the list becomes empty.
215-256: Well-structured data derivation from loader.Good use of
useMemowith appropriate dependencies and safe fallbacks for null/undefined data. ThedataLoadingstate correctly derives fromrevalidator.state.
109-125: Error handling for rejected promises is properly configured at the root level through theErrorBoundaryexported fromroot.tsx. This is the standard pattern in Remix applications with React 19'suse()hook. When promises reject, the thrown errors are caught by the root error boundary. Combined with theSuspensewrapper inTrafficViewWrapper(which handles loading states), this architecture correctly handles both suspension and rejection cases.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
web/app/pages/Dashboard/ProjectCard.tsx (1)
410-444: Pipeline failure:ProjectCardSkeletonis unused.The CI pipeline reports that
ProjectCardSkeletonis exported but never used. Either remove this component if it's no longer needed, or ensure it's imported where the skeleton loading state is required (likely in the parent Dashboard component).#!/bin/bash # Check if ProjectCardSkeleton is imported anywhere in the codebase rg 'ProjectCardSkeleton' --type ts --type tsx -lweb/app/pages/Project/tabs/Profiles/ProfilesView.tsx (1)
260-271: Potential race:loadProfilesmay be silently skipped after navigation.When navigating away from profile detail (line 264-268),
loadProfiles(0, true)is called. However, if a previous profiles load is still in progress (profilesLoading === true), this call will return early at lines 72-74 without loading data.The user might end up with stale data if they quickly navigate back from a profile while a previous load was pending.
Consider either:
- Canceling the in-flight request and starting a new one, or
- Queuing the load request to execute after the current one completes
web/app/lib/models/Project.ts (1)
86-91: Remove unusedSwetrixErrorDetailsinterface.As flagged by the pipeline, this interface is no longer used. The
ErrorDetailscomponent now usesErrorDetailsResponse['details']from~/api/api.serverinstead. Remove to eliminate dead code.🧹 Suggested fix
-export interface SwetrixErrorDetails extends SwetrixError { - colno: number - lineno: number - first_seen: string - stackTrace?: string -}web/app/pages/Project/Settings/ProjectSettings.tsx (1)
1087-1095: Prop naming inconsistency.The prop
setResettingfollows the naming convention for setter functions (likesetShowTransfer,setShowReset,setShowDelete), but it is declared as abooleantype in theDangerZonecomponent interface. This is inconsistent with the adjacentisDeleting={isDeleting}pattern.Rename it to
isResettingfor consistency with the convention whereis*prefixes indicate boolean state values andset*prefixes indicate setter functions. This requires updating both the prop call site and the component's prop interface.🔧 Suggested fix
<DangerZone setShowTransfer={setShowTransfer} setShowReset={setShowReset} setShowDelete={setShowDelete} isDeleting={isDeleting} - setResetting={isResetting} + isResetting={isResetting} />Also update the
DangerZonePropsinterface inweb/app/pages/Project/Settings/tabs/DangerZone.tsx(line 12):- setResetting: boolean + isResetting: boolean
🤖 Fix all issues with AI agents
In @web/app/pages/Dashboard/ProjectCard.tsx:
- Line 342: The PinIcon toggle uses an invalid Tailwind class `rotate-30`;
revert it to the arbitrary value syntax `rotate-[30deg]` in the JSX where
PinIcon is rendered (see the className using cx with localIsPinned) so the icon
actually rotates 30 degrees when localIsPinned is true; ensure the string uses
square-bracket arbitrary syntax `rotate-[30deg]` rather than `rotate-30`.
In @web/app/pages/Project/Settings/Emails/Emails.tsx:
- Around line 385-386: The form reset currently calls setForm({ email: '',
reportFrequency: '' }) which uses an empty string for reportFrequency; change
the reset to use the same default used during initialization
(reportFrequencyForEmailsOptions[3].value) so the modal reopens with the same
pre-selected radio and consistent validation—update the setForm call (and any
other form reset paths) to set reportFrequency:
reportFrequencyForEmailsOptions[3].value while keeping email: ''.
In @web/app/pages/Project/tabs/Profiles/ProfilesView.tsx:
- Line 101: The current double-cast in ProfilesView.tsx masks an API/component
type mismatch; replace the cast with a proper transformation that maps API
fields to the component Profile shape (map id -> profileId, isAnonymous ->
isIdentified as the inverse, pageviews -> pageviewsCount, sessions ->
sessionsCount, customEvents -> eventsCount, errors -> errorsCount). Implement a
small mapper function (e.g., transformApiProfile or mapApiProfileToUI) and use
it when building profilesList instead of "(dataProfiles?.profiles || []) as
unknown as Profile[]", and update any related types so the component consumes
the correctly typed profiles.
In @web/app/pages/Project/tabs/Sessions/SessionsView.tsx:
- Around line 180-185: hasShownContentRef is set to true whenever sessions is
non-empty and never reset, which prevents NoSessions from appearing after
filters produce zero results; update the logic around hasShownContentRef (used
in SessionsView) to reset to false when the loaded sessions become empty (e.g.,
on loader/response update where sessions === [] or _isEmpty(sessions) is true)
or switch to a time-window/animation-based flag (set true only during the exit
animation and clear after the animation completes) so that NoSessions can render
correctly when filters yield no results.
In @web/app/pages/Project/View/ViewProject.tsx:
- Around line 446-460: The effect prematurely sets projectViewsLoading to false
whenever viewsLoadFetcher.state === 'idle', breaking the initial-load guard in
loadProjectViews; change the useEffect so setProjectViewsLoading(false) is only
called after actual response data/error is present (i.e., inside the if
(viewsLoadFetcher.data) block or after handling success/error), leaving the
initial null value intact while no data has arrived; update references to the
effect handling around viewsLoadFetcher.data,
setProjectViews((viewsLoadFetcher.data.data as ProjectView[]) || []), and the
error/toast branch so loading is turned off only when you have a concrete
result.
🧹 Nitpick comments (33)
web/app/pages/Project/View/components/ProjectSidebar.tsx (1)
625-640: Consider the effectiveness of reference equality comparisons.The custom equality function correctly handles
searchParamsby converting to string. However, the following props use reference equality which may reduce memoization effectiveness:
tabs(line 636): If the parent recreates this array on each render, memo won't prevent re-rendersonTabChange/onMobileClose(lines 637-638): Parent must wrap these inuseCallbackfor memo to be effectiveThis isn't a bug—the component will function correctly regardless—but you may want to verify that parent components properly memoize these values, or consider using a deep comparison for
tabsif the array contents are what matter.web/app/hooks/useAnnotations.ts (1)
197-215: Minor redundancy:setAnnotationToEdit(undefined)called twice on success path.When the mutation succeeds,
closeAnnotationModal()(line 208) already setsannotationToEdittoundefined(line 193), making the call at line 213 redundant.♻️ Suggested fix
} setPendingAction(null) - setAnnotationToEdit(undefined) } }, [fetcher.state, fetcher.data, pendingAction, loadAnnotations, t, closeAnnotationModal])web/app/pages/Project/tabs/Funnels/FunnelsView.tsx (3)
69-69: Remove unusedisMountedRefcode.The
isMountedRefis declared and maintained but never checked anywhere in the component. This appears to be leftover from the previous implementation that used direct API calls. With the fetcher-based approach, React Router handles cleanup automatically.🧹 Proposed fix to remove dead code
- const isMountedRef = useRef(true) const fetcher = useFetcher<ProjectViewActionData>() const lastHandledData = useRef<ProjectViewActionData | null>(null)- // Cleanup on unmount - useEffect(() => { - isMountedRef.current = true - return () => { - isMountedRef.current = false - } - }, []) - // Handle fetcher responsesAlso applies to: 118-123
169-169: Simplify redundant dependency array entries.The dependency array includes both
fetcherand its propertiesfetcher.state/fetcher.data. Sincefetcheris a stable object from React Router, you only needfetcher.stateandfetcher.datafor reactivity—or justfetcherif you prefer simplicity.♻️ Proposed simplification
- }, [fetcher.state, fetcher.data, t, id, projectPassword, fetcher, mergeProject, activeFunnel?.id, revalidator]) + }, [fetcher.state, fetcher.data, t, id, projectPassword, mergeProject, activeFunnel?.id, revalidator])
326-333: Remove misleadingasync/awaitfrom onSubmit handlers.The
onFunnelCreateandonFunnelEditfunctions callfetcher.submitwhich is synchronous and returnsvoid. Theasync/awaithere is semantically misleading—the promise resolves immediately without waiting for the actual action to complete.♻️ Proposed fix
- onSubmit={async (name: string, steps: string[]) => { + onSubmit={(name: string, steps: string[]) => { if (funnelToEdit) { - await onFunnelEdit(funnelToEdit.id, name, steps) + onFunnelEdit(funnelToEdit.id, name, steps) return } - await onFunnelCreate(name, steps) + onFunnelCreate(name, steps) }}Apply the same change to the other
NewFunnelcomponents at lines 366-373 and 414-421.web/app/pages/Project/tabs/FeatureFlags/FeatureFlagSettingsModal.tsx (2)
95-116: Consider stabilizing callback identity to reduce re-renders.Including
filterValuesCacheandloadingColumnsin the dependency array causesfetchFilterValuesto be recreated on every cache update, which can trigger downstream effects unnecessarily. The early-return guard prevents redundant fetches, so this is not a functional bug.♻️ Optional: Use refs for mutable state checks
+ const filterValuesCacheRef = useRef(filterValuesCache) + filterValuesCacheRef.current = filterValuesCache + const loadingColumnsRef = useRef(loadingColumns) + loadingColumnsRef.current = loadingColumns const fetchFilterValues = useCallback( async (column: string) => { const cacheKey = `${projectId}-${column}` - if (filterValuesCache[cacheKey] || loadingColumns.has(column)) return + if (filterValuesCacheRef.current[cacheKey] || loadingColumnsRef.current.has(column)) return // ... rest of function }, - [projectId, filterValuesCache, loadingColumns, fetchFilters], + [projectId, fetchFilters], )
240-241: Consider replacing@ts-ignorewith explicit type handling.While the
@ts-ignoreworks, it suppresses all type errors on that line. A type assertion or explicit field handling would be safer.♻️ Optional: Explicit type handling
} else { - // @ts-ignore - TypeScript doesn't like dynamic field assignment - updated[index][field] = value + (updated[index] as Record<string, string | boolean>)[field] = value }Or handle each field explicitly:
} else if (field === 'filter') { updated[index].filter = value as string } else if (field === 'isExclusive') { updated[index].isExclusive = value as boolean }web/app/pages/Project/View/ViewProject.tsx (1)
423-444: Consider clearing stale data before new submissions.The
loadProjectViewscallback correctly guards against concurrent submissions. However, whenforcedistrueand you callfetcher.submit(), theviewsLoadFetcher.datafrom the previous request remains until the new response arrives. If the component re-renders while the fetcher is in-flight, stale data could briefly cause unexpected behavior in consumers.Consider resetting the fetcher data or adding a submission-in-progress flag to distinguish between "idle with fresh data" and "idle before any data":
♻️ Suggested improvement
const loadProjectViews = useCallback( (forced?: boolean) => { if (!forced && projectViewsLoading !== null) { return } if (viewsLoadFetcher.state !== 'idle') { return } setProjectViewsLoading(true) + // Optional: track that we're expecting new data + // This helps distinguish "idle with stale data" from "idle with fresh data" const formData = new FormData() formData.append('intent', 'get-project-views') if (projectPassword) { formData.append('password', projectPassword) } viewsLoadFetcher.submit(formData, { method: 'POST' }) }, [projectViewsLoading, viewsLoadFetcher, projectPassword], )backend/apps/cloud/src/analytics/analytics.service.ts (1)
4934-4981: Unified profile data aggregation with potential behavioral change.The refactored query now applies
filtersQueryto bothanalyticsandcustomEVtables (lines 4950, 4967). This is a behavioral change that may affect filter behavior.Additionally, profile metadata (
cc,os,br,dv) now usesany()across both data sources. For deterministic results, consider usingargMin(cc, created)or similar to consistently pick metadata from the earliest event.♻️ Optional: Deterministic metadata selection
profile_aggregated AS ( SELECT profileId, sum(isPageview) AS pageviewsCount, countDistinct(psid) AS sessionsCount, sum(isEvent) AS eventsCount, min(created) AS firstSeen, max(created) AS lastSeen, - any(cc) AS cc_agg, - any(os) AS os_agg, - any(br) AS br_agg, - any(dv) AS dv_agg + argMin(cc, created) AS cc_agg, + argMin(os, created) AS os_agg, + argMin(br, created) AS br_agg, + argMin(dv, created) AS dv_agg FROM all_profile_data GROUP BY profileId ),web/app/pages/Project/tabs/Goals/GoalsView.tsx (2)
454-454: Unused variable_searchParams.The
_searchParamsvariable is destructured fromuseSearchParams()but never used in the component. Consider removing this declaration if search params aren't needed, or use them if they were intended for some purpose.Suggested fix
- const [_searchParams] = useSearchParams()If search params are needed elsewhere (e.g., for initial filter state), ensure they are being used appropriately.
615-621: Consider memoizing stats loading to avoid redundant fetches.When the
goalsarray changes (e.g., after pagination), this effect loads stats for all goals in the list, even if some stats were previously fetched. While this ensures fresh data, it could result in redundant API calls during navigation.Consider tracking which goals have had their stats loaded for the current date range to skip unnecessary fetches, or accept this as intentional refresh behavior.
web/app/pages/Project/Settings/Emails/Emails.tsx (2)
300-310: Consider removing unnecessarysetTimeoutwrappers.React 18+ automatically batches state updates, making
setTimeout(..., 0)unnecessary for this purpose. These wrappers add cognitive overhead without benefit.Suggested simplification
useEffect(() => { if (loadFetcher.data?.intent === 'get-subscribers') { if (loadFetcher.data.success) { - setTimeout(() => { - setEmails(loadFetcher.data!.subscribers || []) - setPagination((old) => ({ ...old, total: loadFetcher.data!.subscribersCount || 0 })) - }, 0) + setEmails(loadFetcher.data.subscribers || []) + setPagination((old) => ({ ...old, total: loadFetcher.data!.subscribersCount || 0 })) } - setTimeout(() => setLoading(false), 0) + setLoading(false) } }, [loadFetcher.data])
168-172: Consider extracting repeated action path.The action path
/projects/settings/${projectId}appears in 4 places (lines 171, 296, 382, 416). Extracting it to a constant reduces duplication and maintenance burden.Example extraction
// In Emails component: const actionPath = `/projects/settings/${projectId}` // Then use: fetcher.submit({ ... }, { method: 'post', action: actionPath })For
EmailList, the path could be passed as a prop or computed similarly.web/app/pages/Project/Settings/People/People.tsx (3)
55-74: Inconsistent revalidation pattern with parent component.This component explicitly calls
revalidator.revalidate()after a successful role change (line 60), but the parentPeoplecomponent doesn't useuseRevalidatoror explicitly revalidate after successfulshare-projectordelete-share-useractions.React Router's fetcher should auto-revalidate loaders after action completion, so the explicit call here may be redundant—or if it's needed, the parent should do the same for consistency.
Consider either:
- Removing the explicit
revalidate()call here if auto-revalidation suffices, or- Adding explicit revalidation in the
Peoplecomponent for consistency.
238-238: Consider documenting thesetTimeoutwrapper.The
setTimeout(() => validate(), 0)deferral is likely to ensureformstate updates are flushed before validation runs. A brief comment would clarify the intent for future maintainers.
359-359: SimplifyisLoadingexpression to avoid returningundefined.The current ternary returns
true,false, orundefined. Using a logical&&produces a cleaner boolean-only result.♻️ Suggested simplification
- isLoading={isSubmitting ? fetcher.formData?.get('intent') === 'delete-share-user' : undefined} + isLoading={isSubmitting && fetcher.formData?.get('intent') === 'delete-share-user'}web/app/pages/Organisations/Organisations.tsx (2)
75-112: Consider tracking processed fetcher responses to prevent edge-case duplicate handling.The effect depends on
fetcher.data, which persists after the fetcher completes. In most cases this works correctly, but if the component re-renders andfetcher.datareference changes without the content being "new" (e.g., due to serialization behavior), the effect could re-trigger, causing duplicate toasts or unexpected modal behavior.A common robust pattern is to track whether the current response has been processed:
Suggested approach using a processed ref
+ const processedFetcherData = useRef<typeof fetcher.data>(null) + // Handle fetcher responses useEffect(() => { - if (!fetcher.data) return + if (!fetcher.data || fetcher.data === processedFetcherData.current) return + processedFetcherData.current = fetcher.data const { intent, success, error, fieldErrors } = fetcher.data // ... rest of switch statement - }, [fetcher.data, t, revalidate, closeNewOrganisationModal]) + }, [fetcher.data, t, revalidate, closeNewOrganisationModal])
118-127: Use the existingisNewOrganisationLoadingvariable for consistency.Line 119 duplicates the logic already captured in
isNewOrganisationLoading(line 43).Suggested fix
const onCreateOrganisation = () => { - if (fetcher.state === 'submitting') { + if (isNewOrganisationLoading) { return }web/app/pages/Dashboard/ProjectCard.tsx (2)
32-34: Redundantwindowcheck insideuseEffect.
useEffectonly runs on the client, so thetypeof window === 'undefined'check is unnecessary. Consider removing it for cleaner code.Suggested simplification
useEffect(() => { - if (typeof window === 'undefined') { - return - } - const mediaQuery = window.matchMedia('(hover: none)') queueMicrotask(() => setIsTouchDevice(mediaQuery.matches)) const handler = (e: MediaQueryListEvent) => setIsTouchDevice(e.matches) mediaQuery.addEventListener('change', handler) return () => mediaQuery.removeEventListener('change', handler) }, [])
261-271: Consider memoizingonAcceptfor consistency.
handlePinToggleis wrapped withuseCallback, butonAcceptis not. For consistency and to prevent potential unnecessary re-renders of theModalcomponent, consider memoizingonAcceptas well.Suggested change
- const onAccept = () => { + const onAccept = useCallback(() => { if (!shareId) { toast.error(t('apiNotifications.acceptInvitationError')) return } const formData = new FormData() formData.set('intent', 'accept-project-share') formData.set('shareId', shareId) fetcher.submit(formData, { method: 'post' }) - } + }, [shareId, t, fetcher])web/app/pages/Project/tabs/Sessions/SessionsView.tsx (1)
32-59: Consider i18n for error boundary messages.The error boundary uses hardcoded English strings ("Failed to load sessions", "Reload page", "Support") while the rest of the component uses
useTranslation. Class components can't use hooks, but you could pass translated strings via props or use a different pattern.Additionally, consider adding
componentDidCatchfor error logging/monitoring.web/app/pages/Project/tabs/Profiles/ProfilesView.tsx (3)
35-37: Proxy hooks' internal state is unused.The proxy hooks (
useProfilesProxy,useProfileProxy,useProfileSessionsProxy) each maintain their owndata,error, andisLoadingstate internally, but this component only uses thefetch*methods and manages its own local state. This is fine for the request-ID tracking pattern, but the unused hook state (e.g.,profilesProxy.data,profilesProxy.error,profilesProxy.isLoading) adds minor overhead.Consider whether a simpler approach (direct fetch functions without hook state) might be cleaner, or document that the hook's internal state is intentionally unused here.
158-162: Error handling relies on null check but doesn't surface errors to users.When
fetchProfilereturnsnull(on error), the code skips settingactiveProfilebut doesn't inform the user. The proxy hook setserrorinternally, but this component doesn't read or display it.Consider either:
- Using the proxy's
errorstate to show an error message, or- Explicitly handling the null case with user feedback
💡 Suggested approach
- if (requestId === profileRequestIdRef.current && isMountedRef.current && data) { + if (requestId === profileRequestIdRef.current && isMountedRef.current) { + if (!data) { + // Handle error case - show toast or set error state + console.warn('[WARN](loadProfile) No data returned for profile:', profileId) + setActiveProfile(null) + return + } setActiveProfile(data as unknown as ProfileDetailsType)
81-87: Duplicated date range parameter logic across three functions.The pattern for computing
from/tobased onperiodanddateRangeis repeated identically inloadProfiles,loadProfile, andloadProfileSessionsData. This violates DRY and increases maintenance burden.♻️ Suggested refactor: Extract helper function
// Add helper at module level or inside component const getDateRangeParams = (period: string, dateRange: [Date, Date] | null) => { if (period === 'custom' && dateRange) { return { period: '', from: getFormatDate(dateRange[0]), to: getFormatDate(dateRange[1]), } } return { period, from: '', to: '' } } // Then in each load function: const { period: periodParam, from, to } = getDateRangeParams(period, dateRange)Also applies to: 143-149, 185-191
web/app/api/api.server.ts (3)
207-278: Streaming requests lack timeout protection.Unlike
serverFetch,streamingServerFetchuses rawfetchwithout timeout. Long-running or hung streaming connections could block indefinitely.Consider whether an initial connection timeout is appropriate here, or document that callers must handle timeouts at a higher level.
663-686: Consider usingJSON.stringifyfor safer array serialization.Manual string construction works but
JSON.stringify(pids)would be more robust against edge cases (e.g., pids containing special characters).♻️ Suggested change
export async function getOverallStatsServer( request: Request, pids: string[], params: AnalyticsParams & { includeChart?: boolean }, ): Promise<ServerFetchResult<Record<string, OverallObject>>> { - const pidsParam = `[${pids.map((pid) => `"${pid}"`).join(',')}]` + const pidsParam = JSON.stringify(pids) const queryParams = new URLSearchParams() queryParams.append('pids', pidsParam)
766-792: Same manual JSON array construction pattern.For consistency, consider using
JSON.stringify(pids)here as well.web/app/pages/Project/tabs/Errors/ErrorDetails.tsx (1)
22-29:projectPasswordprop is declared but unused.The
projectPasswordprop remains in the interface (line 28) but is not destructured in the component signature (lines 117-123) and is no longer used since the proxy hook handles password internally. Consider removing it from the interface to avoid confusion.♻️ Suggested fix
interface ErrorDetailsProps { details: ErrorDetailsResponse['details'] period?: string from?: string to?: string timeBucket?: string - projectPassword?: string }web/app/pages/Project/tabs/Errors/ErrorsView.tsx (1)
828-837: Remove unusedprojectPasswordprop from ErrorDetails.
projectPasswordis passed but no longer used byErrorDetails(see earlier review comment on that file). Remove to align with the component's actual interface.♻️ Suggested fix
{activeError?.details ? ( <ErrorDetails details={activeError.details} period={period} from={dateRange ? getFormatDate(dateRange[0]) : undefined} to={dateRange ? getFormatDate(dateRange[1]) : undefined} timeBucket={timeBucket} - projectPassword={projectPassword} /> ) : null}web/app/pages/Project/Settings/ProjectSettings.tsx (1)
1057-1063: Consider more specific loading state.The loading prop uses
fetcher.state === 'submitting'which will show loading for any pending submission, not just the captcha difficulty update. For consistency with the GSC buttons' more precise loading states, consider checking the specific intent.♻️ Optional: More precise loading state
<Button type='button' className='mt-4' - loading={fetcher.state === 'submitting'} + loading={fetcher.state === 'submitting' && fetcher.formData?.get('intent') === 'update-project'} onClick={() => {web/app/pages/Organisations/Settings/index.tsx (3)
82-85: Error handling closes delete modal unconditionally.The error branch calls
setShowDelete(false)for all error cases, not just delete-organisation errors. While harmless when the modal isn't open, this is semantically imprecise.Consider scoping the modal close to delete-specific errors:
Suggested refinement
} else if (fetcher.data?.error) { + const failedIntent = fetcher.formData?.get('intent') toast.error(fetcher.data.error) - setShowDelete(false) + if (failedIntent === 'delete-organisation') { + setShowDelete(false) + } }
223-226: Remove commented-out code or add a tracking reference.Dead code should be removed to keep the codebase clean. If the transfer feature is planned for later, consider creating an issue and adding a reference comment.
Suggested cleanup
- {/* <Button onClick={() => setShowTransfer(true)} semiDanger semiSmall> - <ArrowLeftRight className='mr-1 h-5 w-5' /> - {t('project.settings.transfer')} - </Button> */}
209-211: Button component could be improved to support polymorphicasprop with proper type inference.The
@ts-expect-errorsuppression is necessary becauseButtonPropsextendsReact.ComponentPropsWithoutRef<typeof HeadlessButton>without a generic type parameter, preventing TypeScript from inferring the correct props when usingas={Link}.This pattern appears in at least two locations (here and in
PasswordRequiredModal.tsx). Consider refactoring the Button component to accept a generic type:interface ButtonProps<T extends React.ElementType = typeof HeadlessButton> extends React.ComponentPropsWithoutRef<T> { // existing custom props... } const Button = React.forwardRef<HTMLButtonElement, ButtonProps>(({ ... }, ref) => { // implementation }) as <T extends React.ElementType = typeof HeadlessButton>( props: ButtonProps<T> & React.RefAttributes<HTMLButtonElement> ) => React.ReactElementThis would allow proper type checking of the
toprop when usingas={Link}without suppression.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
backend/apps/cloud/src/analytics/analytics.service.tsbackend/apps/community/src/analytics/analytics.controller.tsweb/app/api/api.server.tsweb/app/hooks/useAnnotations.tsweb/app/lib/models/Project.tsweb/app/pages/Dashboard/ProjectCard.tsxweb/app/pages/Organisations/Organisations.tsxweb/app/pages/Organisations/Settings/index.tsxweb/app/pages/Project/Settings/Emails/Emails.tsxweb/app/pages/Project/Settings/People/People.tsxweb/app/pages/Project/Settings/ProjectSettings.tsxweb/app/pages/Project/View/ViewProject.tsxweb/app/pages/Project/View/components/ProjectSidebar.tsxweb/app/pages/Project/tabs/Errors/ErrorDetails.tsxweb/app/pages/Project/tabs/Errors/ErrorsView.tsxweb/app/pages/Project/tabs/FeatureFlags/FeatureFlagSettingsModal.tsxweb/app/pages/Project/tabs/Funnels/FunnelsView.tsxweb/app/pages/Project/tabs/Goals/GoalsView.tsxweb/app/pages/Project/tabs/Profiles/ProfilesView.tsxweb/app/pages/Project/tabs/Sessions/SessionsView.tsxweb/app/pages/Project/tabs/Traffic/TrafficView.tsxweb/app/pages/UserSettings/UserSettings.tsxweb/app/providers/AuthProvider.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/community/src/analytics/analytics.controller.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-10T19:29:45.232Z
Learnt from: Blaumaus
Repo: Swetrix/swetrix PR: 459
File: web/app/hooks/useAnnotations.ts:161-175
Timestamp: 2026-01-10T19:29:45.232Z
Learning: In the Swetrix codebase, password should be sent only for read operations (GET). For all create, update, delete (write) requests, the password parameter must not be included. This rule applies to all files under web/app/hooks (and similar modules) that perform API requests; ensure requests use password only when the operation is a read (GET) and omit it for write operations.
Applied to files:
web/app/hooks/useAnnotations.ts
🧬 Code graph analysis (12)
web/app/pages/Project/Settings/ProjectSettings.tsx (5)
web/app/providers/AuthProvider.tsx (1)
useAuth(131-139)web/app/lib/models/Project.ts (1)
Project(182-214)web/app/routes/projects.settings.$id.tsx (1)
ProjectSettingsActionData(51-71)web/app/hooks/useAnalyticsProxy.ts (1)
useFiltersProxy(536-593)web/app/lib/constants/index.ts (2)
isBrowser(290-290)isSelfhosted(309-309)
web/app/pages/Project/tabs/Profiles/ProfilesView.tsx (3)
web/app/pages/Project/View/ViewProject.tsx (2)
useRefreshTriggers(232-235)useViewProjectContext(227-230)web/app/hooks/useAnalyticsProxy.ts (3)
useProfilesProxy(410-441)useProfileProxy(443-471)useProfileSessionsProxy(473-504)web/app/pages/Project/View/ViewProject.helpers.tsx (1)
getFormatDate(2204-2204)
web/app/pages/Organisations/Organisations.tsx (2)
web/app/routes/organisations._index.tsx (2)
OrganisationsLoaderData(20-26)OrganisationsActionData(63-71)web/app/lib/constants/index.ts (1)
ENTRIES_PER_PAGE_DASHBOARD(437-437)
web/app/pages/Dashboard/ProjectCard.tsx (1)
web/app/routes/dashboard.tsx (1)
DashboardActionData(23-32)
web/app/pages/Project/tabs/Funnels/FunnelsView.tsx (6)
web/app/api/api.server.ts (1)
FunnelDataResponse(1252-1263)web/app/routes/projects.$id.tsx (2)
ProjectLoaderData(151-179)ProjectViewActionData(370-375)web/app/providers/CurrentProjectProvider.tsx (2)
useCurrentProject(235-243)useProjectPassword(36-45)web/app/providers/AuthProvider.tsx (1)
useAuth(131-139)web/app/pages/Project/View/ViewProject.tsx (2)
useRefreshTriggers(232-235)useViewProjectContext(227-230)web/app/lib/models/Project.ts (2)
Funnel(60-65)AnalyticsFunnel(172-180)
web/app/pages/Project/tabs/Goals/GoalsView.tsx (5)
web/app/api/api.server.ts (2)
GoalsResponse(1154-1157)Goal(1142-1152)web/app/routes/projects.$id.tsx (2)
ProjectLoaderData(151-179)ProjectViewActionData(370-375)web/app/providers/CurrentProjectProvider.tsx (1)
useCurrentProject(235-243)web/app/pages/Project/View/ViewProject.tsx (2)
useRefreshTriggers(232-235)useViewProjectContext(227-230)web/app/hooks/useAnalyticsProxy.ts (2)
useGoalStatsProxy(170-198)useGoalChartProxy(200-228)
web/app/pages/Project/tabs/Errors/ErrorsView.tsx (8)
web/app/api/api.server.ts (3)
ErrorsResponse(912-914)ErrorDetailsResponse(941-965)ErrorOverviewResponse(992-1006)web/app/routes/projects.$id.tsx (1)
ProjectLoaderData(151-179)web/app/providers/CurrentProjectProvider.tsx (1)
useCurrentProject(235-243)web/app/hooks/useAnalyticsProxy.ts (1)
useErrorsProxy(82-108)web/app/pages/Project/View/ViewProject.tsx (2)
useRefreshTriggers(232-235)useViewProjectContext(227-230)web/app/pages/Project/View/ViewProject.helpers.tsx (2)
getFormatDate(2204-2204)typeNameMapping(2206-2206)web/app/pages/Project/View/utils/filters.tsx (1)
ERROR_FILTERS_MAPPING(12-14)web/app/lib/models/Project.ts (1)
SwetrixError(74-84)
web/app/pages/Project/tabs/FeatureFlags/FeatureFlagSettingsModal.tsx (3)
web/app/routes/projects.$id.tsx (1)
ProjectViewActionData(370-375)web/app/hooks/useAnalyticsProxy.ts (1)
useFiltersProxy(536-593)web/app/api/api.server.ts (1)
ProjectFeatureFlag(1041-1051)
web/app/pages/Project/Settings/People/People.tsx (3)
web/app/routes/projects.settings.$id.tsx (1)
ProjectSettingsActionData(51-71)web/app/providers/AuthProvider.tsx (1)
useAuth(131-139)web/app/lib/models/Project.ts (1)
ShareOwnerProject(51-58)
web/app/pages/Project/View/ViewProject.tsx (3)
web/app/routes/projects.$id.tsx (1)
ProjectViewActionData(370-375)web/app/pages/Project/View/interfaces/traffic.ts (1)
ProjectView(26-31)web/app/lib/constants/index.ts (4)
PROJECT_TABS(408-410)whitelist(344-344)languageFlag(361-367)languages(353-359)
web/app/pages/Project/tabs/Errors/ErrorDetails.tsx (2)
web/app/api/api.server.ts (1)
ErrorDetailsResponse(941-965)web/app/hooks/useAnalyticsProxy.ts (1)
useErrorSessionsProxy(649-677)
web/app/hooks/useAnnotations.ts (2)
web/app/lib/models/Project.ts (1)
Annotation(67-72)web/app/routes/projects.$id.tsx (1)
ProjectViewActionData(370-375)
🪛 GitHub Actions: Pull Request Validation Workflow
web/app/lib/models/Project.ts
[error] 86-86: Unused exported interface: SwetrixErrorDetails.
web/app/pages/Dashboard/ProjectCard.tsx
[error] 410-410: Unused export: ProjectCardSkeleton (component) is declared but never used.
There was a problem hiding this comment.
Actionable comments posted: 6
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/Dashboard/ProjectCard.tsx (1)
129-204: Remove the fragileformDataguard that will never execute success/error handling.
In React Router,fetcher.formDatais cleared the moment the submission completes andfetcher.datais populated. Since this effect waits forfetcher.datato be set (i.e., submission is complete),formData.get()will always beundefined, causing the guard to return early and skip all success/error handling (no toast, no refetch, no revert). Because eachProjectCardhas its ownuseFetcher()instance, this guard is unnecessary—remove it entirely.Proposed fix
useEffect(() => { if (!fetcher.data) return - // Guard: only process responses for this specific project card - const submittedProjectId = fetcher.formData?.get('projectId') - const submittedShareId = fetcher.formData?.get('shareId') - const isForThisCard = submittedProjectId === id || submittedShareId === shareId - if (!isForThisCard) return - if (fetcher.data.success) { const { intent } = fetcher.data if (intent === 'pin-project') { toast.success(t('dashboard.pinned')) refetchProjects() } else if (intent === 'unpin-project') { toast.success(t('dashboard.unpinned')) refetchProjects() } else if (intent === 'accept-project-share') { mergeUser({ sharedProjects: user?.sharedProjects?.map((item) => { if (item.id === shareId) { return { ...item, isAccessConfirmed: true } } return item }), }) refetchProjects() toast.success(t('apiNotifications.acceptInvitation')) } } else if (fetcher.data.error) { // Revert optimistic update on error if (fetcher.data.intent === 'pin-project' || fetcher.data.intent === 'unpin-project') { setLocalIsPinned(isPinned) } toast.error(fetcher.data.error) } - }, [fetcher.data, fetcher.formData, isPinned, shareId, user, mergeUser, refetchProjects, t, id]) + }, [fetcher.data, isPinned, shareId, user, mergeUser, refetchProjects, t])web/app/pages/Auth/Signin/Signin.tsx (1)
146-180: Infinite polling loop without an exit condition for timeout.The
while (true)loop polls for SSO completion but only exits whenauthWindow.closedor on successful authentication. If the auth window remains open but the backend never returns valid credentials (e.g., network issues, backend failure), this loop runs indefinitely, potentially consuming resources.Consider adding a maximum retry count or timeout based on
expiresInto prevent unbounded polling.Suggested improvement
+ const maxAttempts = Math.ceil(expiresIn / HASH_CHECK_FREQUENCY) + let attempts = 0 + - while (true) { + while (attempts < maxAttempts) { await delay(HASH_CHECK_FREQUENCY) + attempts++ try { const { user, totalMonthlyEvents } = await getJWTBySSOHash(uuid, provider, !dontRemember) // ... rest of success handling } catch { // Authentication is not finished yet } if (authWindow.closed) { setIsSsoLoading(false) return } } + + // Timeout reached + authWindow.close() + setIsSsoLoading(false) + toast.error(t('apiNotifications.socialisationAuthGenericError'))web/app/pages/Auth/Signup/Signup.tsx (1)
127-161: Same infinite polling loop issue.The SSO polling loop in
onSsoLoginhas the same unbounded iteration issue as inSignin.tsx. Consider adding a maximum retry count or timeout.
🤖 Fix all issues with AI agents
In @web/app/hooks/useAnalyticsProxy.ts:
- Around line 648-656: The fetchLiveVisitors function in useLiveVisitorsProxy
currently sends projectId: pids[0] || '' which can send an empty string to the
server; add an explicit early-return at the start of fetchLiveVisitors to handle
empty or missing pids (e.g., if (!pids || pids.length === 0) return null or
throw a clear Error) so the POST to /api/analytics is not made with an invalid
projectId, and update any callers/types as needed to accept the null/error
result; reference fetchLiveVisitors and useLiveVisitorsProxy when applying this
change.
- Around line 114-134: In the fetch payloads sent from useAnalyticsProxy (e.g.,
the fetchStats callback defined in useCallback) remove the redundant "projectId:
''" property so the POST body only includes { action, flagId/other identifiers,
params } (omit the projectId key entirely); apply the same change to the other
fetch calls in this file (the callbacks around lines noted in the review) so
none of the request bodies include projectId: ''.
- Around line 34-46: Replace the local AnalyticsParams interface in
useAnalyticsProxy.ts with an import of the server-side AnalyticsParams contract
and then create a small client-only extension for UI fields (e.g., type
ClientAnalyticsParams = ServerAnalyticsParams & { resultFilter?: string;
search?: string }), ensuring you remove any locally duplicated optionalization
of server-required fields and do NOT add a password field to the client type
since the server reads that from the cookie.
In @web/app/pages/Dashboard/ProjectCard.tsx:
- Around line 260-270: The onAccept handler currently allows double-submits;
guard it the same way as the pin flow by returning early when a submission is
already in-flight and disable the accept control while submitting: inside
onAccept, check fetcher.state (or fetcher.submission) and if it's not 'idle'
return early before creating formData/submit; additionally disable the accept
button/modal trigger when fetcher.state !== 'idle' so users cannot click while
the request is pending (references: onAccept, fetcher.submit, fetcher.state,
shareId, formData).
- Around line 156-222: The isPinning guard currently only checks fetcher.state
=== 'submitting', allowing double-submit during the subsequent revalidation;
update the isPinning computation to treat both 'submitting' and 'loading' as
busy (e.g., isPinning = fetcher.state === 'submitting' || fetcher.state ===
'loading') so handlePinToggle (and any UI that uses isPinning) blocks clicks
during revalidation; ensure you adjust any related dependency usage if necessary
(symbols: isPinning, fetcher.state, handlePinToggle, localIsPinned).
🧹 Nitpick comments (7)
web/app/pages/Dashboard/ProjectCard.tsx (1)
26-44: Guard against setState after unmount inuseIsTouchDevice(microtask).
queueMicrotask(() => setIsTouchDevice(...))can run after unmount. Consider a mounted flag or just set directly in the effect (still hydration-safe with initialfalse).Proposed fix
const useIsTouchDevice = () => { // Important for SSR hydration: first client render must match server output. const [isTouchDevice, setIsTouchDevice] = useState(false) useEffect(() => { if (typeof window === 'undefined') { return } + let mounted = true const mediaQuery = window.matchMedia('(hover: none)') - queueMicrotask(() => setIsTouchDevice(mediaQuery.matches)) + queueMicrotask(() => { + if (mounted) setIsTouchDevice(mediaQuery.matches) + }) const handler = (e: MediaQueryListEvent) => setIsTouchDevice(e.matches) mediaQuery.addEventListener('change', handler) - return () => mediaQuery.removeEventListener('change', handler) + return () => { + mounted = false + mediaQuery.removeEventListener('change', handler) + } }, []) return isTouchDevice }web/app/hooks/useAnalyticsProxy.ts (1)
505-616: Inconsistent error contract (some hooksthrow, others seterrorand return null).
This forces callers to handle both patterns and makes composition harder. Pick one convention per layer (either “throw” everywhere, or “never throw; return{data,error}everywhere”).Also applies to: 648-682, 771-817, 819-840
web/app/pages/Auth/Signin/Signin.tsx (2)
90-97: SSO loading state set to false before it's set to true.On line 95,
setIsSsoLoading(false)is called when the auth window fails to open, but at this pointisSsoLoadinghasn't been set totrueyet (that happens on line 99). This is a no-op but indicates inverted logic. The early return is correct, but thesetIsSsoLoading(false)call is unnecessary here.Suggested fix
if (!authWindow) { toast.error(t('apiNotifications.socialisationAuthGenericError')) - setIsSsoLoading(false) return }
143-144: Potential issue:setTimeoutwith method reference may lose context.Passing
authWindow.closedirectly tosetTimeoutcan cause issues ifcloseexpectsthisto beauthWindow. While this typically works in modern browsers, it's safer to wrap it in an arrow function.Suggested fix
- setTimeout(authWindow.close, expiresIn) + setTimeout(() => authWindow.close(), expiresIn)web/app/pages/Auth/Signup/Signup.tsx (3)
75-82: Same issue:setIsSsoLoading(false)called before loading state is set.Same pattern as in
Signin.tsx— the loading state is reset before it's actually set totrue.Suggested fix
if (!authWindow) { toast.error(t('apiNotifications.socialisationAuthGenericError')) - setIsSsoLoading(false) return }
124-125: Same issue:setTimeoutwith method reference.Same pattern as Signin.tsx — wrap in arrow function for safety.
Suggested fix
- setTimeout(authWindow.close, expiresIn) + setTimeout(() => authWindow.close(), expiresIn)
75-167: Consider extracting the duplicated SSO login flow.The
onSsoLoginfunction is nearly identical betweenSignin.tsxandSignup.tsx. This is a significant amount of duplicated logic (~90 lines) that could be extracted into a shared hook (e.g.,useSsoLogin) to improve maintainability and ensure bug fixes apply to both flows.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
web/app/hooks/useAnalyticsProxy.tsweb/app/lib/models/Project.tsweb/app/pages/Auth/Signin/Signin.tsxweb/app/pages/Auth/Signup/Signup.tsxweb/app/pages/Dashboard/ProjectCard.tsxweb/app/utils/accessToken.tsweb/app/utils/refreshToken.tsweb/app/utils/session.server.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-10T19:29:45.232Z
Learnt from: Blaumaus
Repo: Swetrix/swetrix PR: 459
File: web/app/hooks/useAnnotations.ts:161-175
Timestamp: 2026-01-10T19:29:45.232Z
Learning: In the Swetrix codebase, password should be sent only for read operations (GET). For all create, update, delete (write) requests, the password parameter must not be included. This rule applies to all files under web/app/hooks (and similar modules) that perform API requests; ensure requests use password only when the operation is a read (GET) and omit it for write operations.
Applied to files:
web/app/hooks/useAnalyticsProxy.ts
🧬 Code graph analysis (3)
web/app/pages/Auth/Signin/Signin.tsx (5)
web/app/routes/login.tsx (1)
LoginActionData(26-36)web/app/hooks/useAuthProxy.ts (1)
useAuthProxy(10-121)web/app/api/api.server.ts (1)
SSOProvider(1872-1872)web/app/lib/models/Auth.ts (1)
SSOProvider(10-10)web/app/utils/generic.ts (1)
openBrowserWindow(99-105)
web/app/pages/Dashboard/ProjectCard.tsx (1)
web/app/routes/dashboard.tsx (1)
DashboardActionData(23-32)
web/app/hooks/useAnalyticsProxy.ts (1)
web/app/api/api.server.ts (3)
AnalyticsParams(625-634)AnalyticsFilter(583-588)OverallObject(613-623)
🔇 Additional comments (9)
web/app/lib/models/Project.ts (2)
165-173: LGTM!The addition of
dropoffPercis consistent with the existing field naming conventions (eventsPerc,dropoffPercStep) and logically completes the funnel analytics data model.
74-84: No broken imports detected from type removals.Verification confirms that
LiveStatsandSwetrixErrorDetailswere successfully removed fromProject.ts.LiveStatswas migrated toapi.server.tsand all consumers (api.analytics.tsanduseAnalyticsProxy.ts) correctly import from the new location.SwetrixErrorDetailshas no remaining usages in the codebase. No broken imports or TypeScript errors will result from these removals.web/app/pages/Dashboard/ProjectCard.tsx (1)
343-356: Settings button-in-Link navigation looks good.
Preventing default +navigate(...)preserves the card link while allowing an accessible secondary action.web/app/pages/Auth/Signin/Signin.tsx (3)
321-370: LGTM on the Form migration.The migration to React Router's
Formcomponent withuseActionDatafor error handling anduseNavigationfor loading states follows the recommended patterns for React Router v7. The field error clearing viaclearedErrorsSet provides good UX by allowing users to dismiss errors on input change.
350-351: Server-side parsing is already handled correctly.The form value is correctly parsed in
web/app/routes/login.tsxusingformData.get('dontRemember') === 'true', avoiding any truthy/falsy pitfalls with the string'false'.
188-202: 2FA submission properly includes user context via JWT token—no changes needed.The
_submit2FAfunction correctly passes user context through the session. The frontend callsserverFetch(request, '2fa/authenticate', ...)which extracts the access token from the request object and includes it in the Authorization header. The backend endpoint's@Auth()decorator and@CurrentUserId()parameter extract the user ID directly from this JWT token. Email and password re-submission is neither needed nor expected after the initial login—JWT tokens are the standard mechanism for maintaining authentication context across requests, including for both regular login and SSO-triggered 2FA flows.Likely an incorrect or invalid review comment.
web/app/pages/Auth/Signup/Signup.tsx (3)
238-240: Hidden inputs for checkbox values are correctly implemented.The hidden inputs properly pass boolean values as strings for server-side processing, matching the pattern used in
Signin.tsx. This is the correct approach for Headless UI checkboxes that don't submit natively.
210-316: Form implementation looks good.The migration to React Router's
Formcomponent with action-based submission and field error handling follows best practices. ThehandleFormSubmitcallback correctly resets cleared errors before submission.
406-406: LGTM on the memoization.Adding
memo()wrapper is appropriate for this component to prevent unnecessary re-renders when parent components update.
1 participant
Changes
Also fixes:
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Removals
✏️ Tip: You can customize this high-level summary in your review settings.