Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces a comprehensive Goals feature for tracking custom analytics events and pageviews. It adds a complete backend module with entity, CRUD operations, and ClickHouse-backed analytics (stats, charts, sessions), paired with a full-featured frontend UI including list views, expandable charts, and create/edit modals integrated into the project dashboard. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Frontend UI
participant API as Frontend API
participant Controller as Goal Controller
participant Service as Goal Service
participant DB as TypeORM Repo
participant CH as ClickHouse
User->>UI: Create new goal
UI->>API: createGoal(goalData)
API->>Controller: POST /goal (validate, check permissions)
Controller->>Service: create(goalData)
Service->>DB: save Goal entity
DB-->>Service: Goal persisted
Service-->>Controller: Goal
Controller-->>API: 201 Goal response
API-->>UI: Goal created
User->>UI: View goal stats & chart
UI->>API: getGoalStats(goalId, period)
API->>Controller: GET /goal/:id/stats (resolve timezone)
Controller->>CH: Query conversions, sessions (filtered by goal type/metadata)
CH-->>Controller: Aggregated metrics
Controller->>Controller: Calculate conversion rate & trend
Controller-->>API: GoalStats
API-->>UI: Stats rendered
UI->>API: getGoalChart(goalId, period, timeBucket)
API->>Controller: GET /goal/:id/chart (group by time bucket)
Controller->>CH: Query time-series data (by timeBucket: day/hour/month)
CH-->>Controller: Time-bucketed conversions & sessions
Controller->>Controller: Format labels & align dates
Controller-->>API: GoalChartData { x, conversions, uniqueSessions }
API-->>UI: Chart data plotted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (21)
web/app/modals/NewFunnel.tsx (1)
131-131: Inconsistent Tailwind variant ordering.The className mixes
hover:dark:anddark:hover:orderings. For consistency with the other dark mode hover utilities in this same line (dark:hover:bg-slate-800,dark:hover:text-slate-300), consider usingdark:hover:border-slate-700/80.- 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 hover:dark:border-slate-700/80 dark:hover:bg-slate-800 dark:hover:text-slate-300' + 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'web/app/pages/Project/Settings/ProjectSettings.tsx (1)
1029-1029: Provide a complete label inselectedItemfor consistency.The
selectedItemprop has an emptylabelproperty, which is inconsistent with otherSelectusages in this file (lines 757, 937-939) where the full item object is provided. While thetitleprop handles display, the empty label may cause unexpected behavior in the Select component's internal selection matching logic.Apply this diff to find and use the complete item:
+ const difficultyItems = [ + { value: 2, label: t('project.settings.captcha.difficultyLevels.veryEasy') }, + { value: 3, label: t('project.settings.captcha.difficultyLevels.easy') }, + { value: 4, label: t('project.settings.captcha.difficultyLevels.medium') }, + { value: 5, label: t('project.settings.captcha.difficultyLevels.hard') }, + { value: 6, label: t('project.settings.captcha.difficultyLevels.veryHard') }, + ] <Select label={t('project.settings.captcha.difficulty')} hint={t('project.settings.captcha.difficultyHint')} className='lg:w-1/2' hintClassName='lg:w-2/3' - items={[ - { value: 2, label: t('project.settings.captcha.difficultyLevels.veryEasy') }, - { value: 3, label: t('project.settings.captcha.difficultyLevels.easy') }, - { value: 4, label: t('project.settings.captcha.difficultyLevels.medium') }, - { value: 5, label: t('project.settings.captcha.difficultyLevels.hard') }, - { value: 6, label: t('project.settings.captcha.difficultyLevels.veryHard') }, - ]} + items={difficultyItems} keyExtractor={(item) => String(item.value)} labelExtractor={(item) => item.label} - selectedItem={{ value: captchaDifficulty, label: '' }} + selectedItem={difficultyItems.find((item) => item.value === captchaDifficulty)} onSelect={(item: { value: number; label: string }) => { setCaptchaDifficulty(item.value) }}web/app/ui/ProgressRing.tsx (3)
60-60: Optionally guard against non‑finitevaluebefore clampingIf
valueever becomesNaNor otherwise non‑finite (e.g., from a division by zero upstream), it will propagate throughnormalizedValueand into the SVG attributes. Consider a small defensive guard to fall back to 0 in that case:- const normalizedValue = useMemo(() => Math.min(100, Math.max(0, value)), [value]) + const normalizedValue = useMemo(() => { + const safe = Number.isFinite(value) ? value : 0 + return Math.min(100, Math.max(0, safe)) + }, [value])This keeps the ring in a sane state even when upstream data is temporarily bad.
62-72: Clamp radius to avoid negative values whensize < strokeWidthIf a caller ever passes a
sizesmaller thanstrokeWidth,radiusbecomes negative, which can produce odd rendering across browsers. A tiny clamp keeps things safe with no behavioral change for normal inputs:- const r = (size - strokeWidth) / 2 + const r = Math.max(0, (size - strokeWidth) / 2)
circumferenceandstrokeDashoffsetthen gracefully collapse to 0 when the ring can’t be drawn.
79-111: Consider optional ARIA hooks for better accessibilityRight now the ring is purely visual; screen readers won’t get any progress information unless it’s duplicated elsewhere. Consider (in a later pass) adding optional props for accessibility, e.g. an
ariaLabelor alabel+ wiringrole="progressbar"witharia-valuenow/min/max, so consumers can opt into semantic progress when needed.web/app/styles/tailwind.css (1)
16-21: Unify height variables to avoid duplicated header constantsYou now have
--header-heightplus hard-coded74px/157pxin other min-height vars. To keep layouts consistent when header size changes, consider deriving--min-height-pageand--min-height-min-footerfrom--header-heightinstead of fixed pixel values.web/public/locales/en.json (2)
822-887: Dashboard security/goals labels look good; consider deduplicating description textThe new
dashboard.security,dashboard.goals, anddashboard.goalsDesckeys read clearly and match the new tab semantics. Sincedashboard.goalsDescandgoals.descriptiondescribe essentially the same concept, you might want to reuse a single translation string to avoid drift if the copy is updated in future.
953-1000: Goals locale block is comprehensive; minor wording consistency nits onlyThe
goalssection covers all the UI states (CRUD, filters, stats, sessions) and aligns well with the feature surface. Two tiny optional polish points:
add/addGoalcould be consolidated or named more explicitly based on usage (e.g. one for CTA button, one for modal title) to avoid confusion.- Ensure capitalization is consistent across labels (e.g. “Add goal” vs “Add Goal”) per your UI copy guidelines.
Functionally everything looks fine.
backend/migrations/mysql/2025_12_07_goals.sql (1)
1-14: Goals table schema matches the model; consider tightening projectId constraintsThe table shape (including
type,matchType,metadataFiltersJSON, andprojectIdFK withON DELETE CASCADE) matches the described Goal model well. Two potential hardening tweaks if they fit the domain:
- If every goal must belong to a project, consider making
projectIdNOT NULLto enforce that invariant at the DB level.- You’ll query goals by
projectIda lot; InnoDB will have an index for the FK, but it can be worth explicitly documenting/ensuring that in your migrations (and possibly adding a composite index like(projectId, active)if you often filter by both).Please verify these against your expected query patterns and CE vs cloud usage before changing the migration.
web/app/pages/Project/Goals/View/GoalSettingsModal.tsx (3)
56-72: Consider handling component unmount during async operation.If the modal is closed while
loadGoalis still in progress, state updates will be attempted on an unmounted component. Consider adding an abort mechanism or checking if the component is still mounted before updating state.const loadGoal = async () => { if (!goalId) return setIsLoading(true) try { const goal = await getGoal(goalId) + // If modal was closed during fetch, skip state updates + if (!isOpen) return setName(goal.name) setType(goal.type) setMatchType(goal.matchType) setValue(goal.value || '') setMetadataFilters(goal.metadataFilters || []) } catch (err: any) { toast.error(err?.message || 'Failed to load goal') onClose() } finally { setIsLoading(false) } }Alternatively, use an AbortController or a mounted ref pattern.
120-132: Consider validating metadata filters before submission.Empty metadata filter entries (with empty key or value) can be added and submitted. Consider filtering out incomplete entries or adding validation to prevent submitting filters with empty keys/values.
const handleSubmit = async (e: React.FormEvent) => { e.preventDefault() setIsSaving(true) + // Filter out incomplete metadata filters + const validFilters = metadataFilters.filter(f => f.key.trim() && f.value.trim()) try { if (isNew) { const data: CreateGoal = { pid: projectId, name, type, matchType, value: value || undefined, - metadataFilters: metadataFilters.length > 0 ? metadataFilters : undefined, + metadataFilters: validFilters.length > 0 ? validFilters : undefined, } await createGoal(data)
243-268: Consider adding accessible labels for metadata filter inputs.The metadata filter inputs lack
aria-labelor associated<label>elements, which may affect screen reader accessibility.<Input value={filter.key} onChange={(e) => updateMetadataFilter(index, 'key', e.target.value)} placeholder={t('goals.filterKey')} className='flex-1' + aria-label={`${t('goals.filterKey')} ${index + 1}`} /> <span className='text-gray-500'>=</span> <Input value={filter.value} onChange={(e) => updateMetadataFilter(index, 'value', e.target.value)} placeholder={t('goals.filterValue')} className='flex-1' + aria-label={`${t('goals.filterValue')} ${index + 1}`} />backend/apps/cloud/src/goal/dto/goal.dto.ts (1)
106-150: Consider adding validation decorators to response DTOs for documentation completeness.While
GoalDtoandGoalStatsDtoare response types and don't require runtime validation, adding validation decorators would improve Swagger documentation and provide consistency. This is optional since these are output DTOs.web/app/pages/Project/Goals/View/GoalsView.tsx (1)
524-530: Consider debouncing or batching stats loading for performance.When the date range changes,
loadGoalStatsis called for every goal in the list. With many goals, this could trigger numerous parallel API requests. Consider batching these requests or implementing a debounce.+import { useDebouncedCallback } from 'use-debounce' // Or batch the requests: useEffect(() => { // Load stats for all goals when goals change or date range changes - goals.forEach((goal) => { - loadGoalStats(goal.id) - }) + const loadAllStats = async () => { + await Promise.all(goals.map((goal) => loadGoalStats(goal.id))) + } + loadAllStats() // eslint-disable-next-line react-hooks/exhaustive-deps }, [goals, period, from, to, timezone])Using
Promise.allat least allows the browser to batch connection handling, though a dedicated batch endpoint would be more efficient.backend/apps/cloud/src/goal/entity/goal.entity.ts (1)
68-71: Add onDelete cascade behavior to the Project relation for referential integrity consistency.The
@ManyToOnerelation to Project lacks cascade delete behavior. The codebase consistently appliesonDelete: 'CASCADE'for similar child entities—for example,ProjectViewEntityspecifies{ onDelete: 'CASCADE' }when relating to Project. Without this, orphaned goals will remain in the database if a project is deleted.@ApiProperty({ type: () => Project }) -@ManyToOne(() => Project, project => project.goals) +@ManyToOne(() => Project, project => project.goals, { onDelete: 'CASCADE' }) @JoinColumn() project: Projectweb/app/lib/models/Goal.ts (1)
1-33: Unify Goal types across models and API, especiallyGoalChartDatanamingThis file defines the canonical Goal types, while
web/app/api/index.tsredefinesGoal,GoalStats,GoalChartData, andGoalSessionwith slightly different shapes (notably,GoalChartDatahere is a single point, but the API version is an x/conversions/uniqueSessions series). That duplication will drift and the shared name for different shapes is easy to misuse.Consider:
- Importing
Goal,GoalStats, andGoalSessionfrom here in the API layer instead of redefining them.- Renaming the API’s
{ x: string[]; conversions: number[]; ... }to something likeGoalChartSeriesand usingGoalChartData[]from this file as the per-point shape inside the UI.This keeps a single source of truth for models and avoids subtle type mismatches between API responses and view models.
backend/apps/cloud/src/goal/goal.service.ts (1)
1-72: Typecreate/updatewith DTOs instead ofPartial<Goal>The service methods are fine functionally, but
createandupdatecurrently acceptPartial<Goal>, which is quite permissive and bypasses the structure you already enforce at the controller/DTO level.To tighten things up, consider:
async create(goalData: CreateGoalDto): Promise<Goal> { ... } async update(id: string, goalData: UpdateGoalDto): Promise<UpdateResult> { ... }This keeps the service surface aligned with your DTO contracts and makes later changes (like adding non-persisted fields to
Goal) safer.web/app/pages/Project/View/ViewProject.tsx (1)
545-545: Goals refresh uses a trigger counter but isn’t tied to global loadingIn
refreshStats, whenactiveTab === PROJECT_TABS.goalsyou only bumpgoalsRefreshTrigger. That’s a clean, side-effect-free signal forGoalsViewto refetch.One small UX consideration: since goals fetching doesn’t touch
dataLoading, the global<LoadingBar />you added won’t appear during goal reloads; any loading indication will have to live insideGoalsView. If you want consistent behavior with other tabs, you may want to either:
- Expose a setter for
dataLoadingvia context for GoalsView, or- Let
ViewProjectContentown the goals fetch and keepGoalsViewdumb.Not blocking, just something to keep in mind.
Also applies to: 2620-2672, 3413-3416
web/app/api/index.ts (1)
1137-1282: Reuse shared Goal types and disambiguateGoalChartDataFunctionally, the new Goals API helpers are aligned with the rest of this file (same error handling, pagination shape, and axios usage). A couple of type-structure points are worth tightening:
Type duplication vs
~/lib/models/Goal
Goal,GoalStats, andGoalSessionare already defined inweb/app/lib/models/Goal.tswith the same fields. Redefining them here risks future drift.Suggestion:
import type { Goal, GoalStats, GoalSession } from '~/lib/models/Goal'and drop the local interface duplicates.
GoalChartDataname clash for different shapesHere,
GoalChartDatais a series:{ x: string[]; conversions: number[]; uniqueSessions: number[] }whereas in
~/lib/models/Goalit describes a single point:{ time: string; conversions: number; uniqueSessions: number }Sharing the same name for two incompatible shapes is error-prone.
Consider renaming this one to something like
GoalChartSeriesand typing the helper accordingly:export interface GoalChartSeries { x: string[] conversions: number[] uniqueSessions: number[] } export const getGoalChart = (...) => api .get(`/goal/${goalId}/chart`, { params: { ... } }) .then((response): { chart: GoalChartSeries } => response.data)Then the UI can map that into an array of the shared
GoalChartDatapoint type if needed.If you centralize types in
~/lib/models/Goaland use distinct names for series vs points, the goals feature will be much easier to maintain.backend/apps/cloud/src/goal/goal.controller.ts (2)
124-128: Address TypeScript type mismatch instead of suppressing.The
@ts-expect-errorindicates a type mismatch when mappingresults. Consider defining a proper return type or using a DTO transformation to avoid suppressing type checking.- // @ts-expect-error - result.results = _map(result.results, goal => ({ + const transformedResults = _map(result.results, goal => ({ ..._omit(goal, ['project']), pid: goal.project.id, - })) + })) as GoalDto[] - return result + return { + ...result, + results: transformedResults, + }
284-287: Remove unused_tableparameter.The
_tableparameter is never used in the method body. Remove it to avoid confusion.private buildGoalMatchCondition( goal: Goal, - _table: 'analytics' | 'customEV', ): { condition: string; params: Record<string, string> } {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
backend/apps/cloud/src/app.module.ts(2 hunks)backend/apps/cloud/src/goal/dto/goal.dto.ts(1 hunks)backend/apps/cloud/src/goal/entity/goal.entity.ts(1 hunks)backend/apps/cloud/src/goal/goal.controller.ts(1 hunks)backend/apps/cloud/src/goal/goal.module.ts(1 hunks)backend/apps/cloud/src/goal/goal.service.ts(1 hunks)backend/apps/cloud/src/project/entity/project.entity.ts(2 hunks)backend/migrations/mysql/2025_12_07_goals.sql(1 hunks)web/app/api/index.ts(1 hunks)web/app/lib/constants/index.ts(2 hunks)web/app/lib/models/Goal.ts(1 hunks)web/app/modals/NewFunnel.tsx(1 hunks)web/app/pages/Project/Goals/View/GoalSettingsModal.tsx(1 hunks)web/app/pages/Project/Goals/View/GoalsView.tsx(1 hunks)web/app/pages/Project/Goals/View/index.tsx(1 hunks)web/app/pages/Project/Settings/ProjectSettings.tsx(1 hunks)web/app/pages/Project/View/ViewProject.tsx(34 hunks)web/app/pages/Project/View/components/CaptchaView.tsx(2 hunks)web/app/pages/Project/View/components/ProjectSidebar.tsx(6 hunks)web/app/styles/tailwind.css(1 hunks)web/app/ui/ProgressRing.tsx(1 hunks)web/app/ui/Text.tsx(2 hunks)web/public/locales/en.json(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
backend/apps/cloud/src/project/entity/project.entity.ts (2)
web/app/api/index.ts (1)
Goal(1138-1148)web/app/lib/models/Goal.ts (1)
Goal(9-19)
web/app/ui/ProgressRing.tsx (1)
web/app/utils/generic.ts (1)
cn(150-152)
web/app/ui/Text.tsx (1)
web/app/utils/generic.ts (1)
cn(150-152)
web/app/pages/Project/View/ViewProject.tsx (2)
web/app/lib/constants/index.ts (1)
PROJECT_TABS(422-424)web/app/pages/Project/View/ViewProject.helpers.tsx (1)
getFormatDate(1944-1944)
web/app/pages/Project/Goals/View/GoalSettingsModal.tsx (1)
web/app/api/index.ts (4)
getGoal(1191-1197)CreateGoal(1199-1206)createGoal(1208-1214)updateGoal(1216-1222)
backend/apps/cloud/src/goal/dto/goal.dto.ts (2)
backend/apps/cloud/src/goal/entity/goal.entity.ts (1)
MetadataFilter(23-26)web/app/lib/models/Goal.ts (3)
MetadataFilter(4-7)GoalType(1-1)GoalMatchType(2-2)
backend/apps/cloud/src/goal/goal.service.ts (2)
web/app/api/index.ts (1)
Goal(1138-1148)web/app/lib/models/Goal.ts (1)
Goal(9-19)
web/app/pages/Project/View/components/ProjectSidebar.tsx (2)
web/app/ui/Text.tsx (1)
Text(51-75)web/app/lib/constants/index.ts (1)
PROJECT_TABS(422-424)
web/app/lib/models/Goal.ts (2)
backend/apps/cloud/src/goal/entity/goal.entity.ts (1)
MetadataFilter(23-26)web/app/api/index.ts (4)
Goal(1138-1148)GoalStats(1150-1156)GoalChartData(1158-1162)GoalSession(1164-1172)
🔇 Additional comments (38)
web/app/ui/Text.tsx (2)
17-17: Newcodeprop is a clean, backwards-compatible extensionAdding
code?: booleantoTextPropsis type-safe and optional, so it won’t break existing call sites while giving a clear, semantic flag for “code-like” text.
58-70: Conditional code styling is correctly scoped and mergedDestructuring
codeand omitting it from{...props}avoids leaking an invalid DOM attribute, and the conditional Tailwind classes are properly merged viacnalongside existing size/weight/colour/truncate handling. This is a straightforward, non-breaking enhancement.web/app/ui/ProgressRing.tsx (1)
5-50: Threshold logic and color mapping look solidThe
ColourThresholdsshape andgetColourClasseslogic cleanly partition values into green/amber/red without gaps or overlaps, and the defaults are reasonable. No changes needed here.web/app/pages/Project/View/components/CaptchaView.tsx (2)
15-15: LoadingBar import wiring looks correctImporting
LoadingBarfrom~/ui/LoadingBaris consistent with the usage below; no issues here.
172-187: Loading state split between initial load and refresh looks solidThe
hasExistingDataflag plus the two guards:
analyticsLoading && !hasExistingData→ full-page<Loader />only on initial loadisPanelsDataEmpty && !dataLoading→NoEventsonly when not actively fetchingdataLoading && hasExistingData→ inline<LoadingBar />while refreshing with prior datagive a clear, non-flickering UX and don’t introduce new state bugs given the current
panelsData/chartDatahandling.web/app/lib/constants/index.ts (1)
400-425: Confirm Goals availability for self‑hosted before exposing tabAdding
goalsto bothSELFHOSTED_PROJECT_TABSandPRODUCTION_PROJECT_TABSkeeps thePROJECT_TABStyping consistent and will surface the Goals tab everywhere. That’s good for parity, but it assumes the Goals backend and migrations are also enabled for Community Edition/self‑hosted deployments. Please double‑check that self‑hosted instances actually have the necessary API/migrations wired; otherwise consider gating thegoalstab by edition or feature flag.backend/apps/cloud/src/goal/goal.module.ts (1)
1-24: GoalModule wiring looks idiomatic; just keep an eye on circular depsImporting
TypeOrmModule.forFeature([Goal])along withProjectModule,AppLoggerModule,UserModule, andAnalyticsModule, and exportingGoalServiceis a standard NestJS pattern and should work fine. Please just verify thatProjectModule(or others) do not in turn importGoalModule, to avoid circular dependency warnings at runtime.web/app/pages/Project/Goals/View/index.tsx (1)
1-1: Barrel re‑export for GoalsView is clean and convenientRe‑exporting the default from
./GoalsViewkeeps the import path concise (.../Goals/View) and matches common project conventions. No issues here.backend/apps/cloud/src/project/entity/project.entity.ts (1)
12-12: Project–Goal relation looks correct; verify GDPR export & entity/table alignmentThe
@OneToMany(() => Goal, goal => goal.project)plusgoals: Goal[]aligns with the newgoal.projectIdFK and gives you the inverse navigation onProject. A couple of follow‑ups:
- Line 28 notes that modifying properties here may require updates to the GDPR data export email template. If goals should appear in a user’s export, ensure that template is updated accordingly (or explicitly decide they’re out of scope).
- Please also confirm that
Goalis mapped to the same table name and column (projectId) as defined in the2025_12_07_goals.sqlmigration so TypeORM doesn’t drift from the schema over time.Also applies to: 127-130
backend/apps/cloud/src/app.module.ts (1)
23-23: GoalModule registration in AppModule looks fine; confirm if it should be configurableImporting
GoalModuleand adding it to the sharedmodulesarray correctly wires the new feature into the cloud app. If Goals are meant to be always‑on for all cloud environments, this is perfect; if not, consider guarding it behind an env flag similar toENABLE_INTEGRATIONS.Also applies to: 81-81
web/app/pages/Project/Goals/View/GoalSettingsModal.tsx (1)
134-145: LGTM!Good use of Headless UI Dialog with proper transition effects and responsive styling. The dark mode support is well-implemented.
backend/apps/cloud/src/goal/dto/goal.dto.ts (3)
15-25: LGTM!The
MetadataFilterDtocorrectly implements theMetadataFilterinterface with proper validation decorators (@IsString,@IsNotEmpty) ensuring both key and value are non-empty strings.
27-62: LGTM!The
CreateGoalDtois well-structured with appropriate validation decorators. The nested validation formetadataFiltersusing@ValidateNested({ each: true })and@Type(() => MetadataFilterDto)ensures proper validation of array items.
64-104: LGTM!The
UpdateGoalDtocorrectly makes all fields optional for partial updates while maintaining the same validation rules. The inclusion of theactiveboolean field allows toggling goal status.web/app/pages/Project/View/components/ProjectSidebar.tsx (4)
25-25: LGTM!Good addition of the
goalscolor token toICON_COLORSmap, maintaining consistency with the existing color scheme for analytics tabs.
63-63: Verify the UX impact of always-expanded groups.The
CollapsibleGroupnow defaults toisExpanded = trueinstead of usinggroup.defaultExpanded. This means all groups will be expanded by default, which may affect the user experience if there are many groups/tabs.Was this intentional as part of the sidebar simplification? If some groups should be collapsed by default (e.g., Security), consider restoring the
defaultExpandedlogic or making it configurable.
174-189: LGTM!The Goals tab is correctly added to the
productAnalyticsTabsfilter, integrating it into the Product Analytics group alongside profiles, sessions, funnels, and errors.
191-198: LGTM!Renaming the CAPTCHA group to "Security" is a reasonable UX improvement that provides a more general category name that could accommodate future security-related tabs.
web/app/pages/Project/Goals/View/GoalsView.tsx (3)
253-260: LGTM!Good use of
useMemofor deriving the pattern display with match type prefixes. The conditional rendering handles null values appropriately.
700-709: LGTM!Good conditional rendering for pagination - only shown when there are multiple pages and no active filter. This prevents confusing pagination behavior when filtering locally.
534-540: The eslint-disable comment here is justified and necessary. The effect intentionally excludesgoalChartDatafrom dependencies to prevent infinite loops whenloadGoalChartDataupdates the state. The conditionexpandedGoalId && goalChartData[expandedGoalId]is designed to prevent unnecessary reloads, and the handlerhandleToggleExpand(line 513-514) manages initial data loading when a goal is expanded. The dependencies[period, from, to, timezone, timeBucket]correctly capture when chart data should be refreshed for the currently expanded goal. No infinite loop risk exists with this pattern.Likely an incorrect or invalid review comment.
backend/apps/cloud/src/goal/entity/goal.entity.ts (2)
12-21: LGTM!The enums
GoalTypeandGoalMatchTypeare well-defined with clear string values that align with the frontend type definitions inweb/app/lib/models/Goal.ts.
28-76: LGTM!The Goal entity is well-structured with:
- Proper UUID primary key generation
- Appropriate column lengths matching DTO validation
- Sensible defaults (matchType: EXACT, active: true)
- Nullable fields where appropriate (value, metadataFilters)
- ApiProperty decorators for Swagger documentation
web/app/pages/Project/View/ViewProject.tsx (7)
41-43: Goals tab and view wiring looks consistentThe new
TargetIcon,GoalsViewimport, goals tab entry (PROJECT_TABS.goals), and conditional<GoalsView />render are all wired in the same pattern as existing tabs (funnels, captcha, alerts). Passingperiod, andfrom/toformatted viagetFormatDate, plustimezone, aligns with how other analytic endpoints are called.Assuming
GoalsViewconsumesperiod/date range similarly to the goals endpoints in~/api, this integration looks good.Also applies to: 1503-1507, 1532-1535, 4304-4311, 170-170
392-416: Goals refresh trigger is correctly plumbed through contextAdding
goalsRefreshTriggertoViewProjectContextType, the default context, state (useState(0)), and provider value is consistent with the existingcaptchaRefreshTriggerpattern. This gives child components (likeGoalsView) a simple numeric “bump” they can watch for reloads without threading callbacks.No issues here.
Also applies to: 418-437, 539-546, 3413-3416
2026-2043: Session pagination reset + override logic is soundThe new
loadSessions(forcedSkip?, override?)and its usage look correct:
- On back from a session detail and in
refreshStatsfor the sessions tab, you:
- Reset
sessionsSkipto 0.- Call
loadSessions(0, true), which replaces the list instead of appending.- The guard using
sessionsRequestIdRefprevents late responses from stale calls from corrupting state.- The “auto-load first page” effect respects
skipNextSessionsAutoLoadRefso you don’t double-fetch when you’ve already explicitly reloaded.This should fix the previous “resume pagination from old offset” behavior without introducing obvious races.
Also applies to: 2062-2114, 2788-2791, 3965-3990
2116-2189: Profiles pagination reset mirrors sessions and looks correctSimilarly for profiles:
loadProfiles(forcedSkip?, override?)now supports replacing vs appending.- When:
- The profiles tab becomes active,
- The profile type filter changes, or
- You return from a profile detail,
you resetprofilesSkipto 0 and callloadProfiles(0, true).skipNextProfilesAutoLoadRefprevents the “auto-load” effect from firing a redundant request after a manual reset.This is a clear improvement over the prior pagination behavior.
Also applies to: 2794-2820, 2832-2844, 3895-3902
3023-3025: Resetting skip counters on period/filter/navigation changes is consistentThe various new
setSessionsSkip(0),setErrorsSkip(0), andsetProfilesSkip(0)calls (on period change, filter changes, and back-navigation from details) are consistent with the updated loaders that accept(forcedSkip, override).Together they ensure:
- New time ranges and filters start at the first page.
- Returning from a detail view doesn’t resume from an old offset.
The patterns are uniform across sessions, errors, and profiles and look good.
Also applies to: 785-796, 2030-2034, 2048-2052, 2805-2807, 2839-2842
3426-3431: Extended LoadingBar conditions improve perceived responsivenessShowing
<LoadingBar />when:
dataLoading && !isAutoRefreshing, or- A next page of sessions/errors/profiles is loading while the list is non-empty,
is a nice UX improvement: users see a subtle top bar when more data is coming, without blocking the initial skeleton-based loads. This logic matches how the list loaders work elsewhere in the component.
3270-3275: Layout switches to header-aware height utilities are coherentReplacing the older
min-h-min-footer/h-fullpatterns withmin-h-including-headerandh-including-headeracross the various early-return branches and the main layout keeps sidebar and content height consistent relative to the header.All usages (locked state, empty data states, errors-only state, and main view) have been updated in sync, so there’s no obvious mismatch.
Also applies to: 3288-3296, 3330-3338, 3366-3374, 3441-3458, 3295-3309, 3334-3348, 3385-3388
backend/apps/cloud/src/goal/goal.controller.ts (8)
66-93: LGTM!The endpoint correctly fetches the goal with relations, verifies user access via
allowedToView, and returns a clean response withpidinstead of nested project data.
215-260: LGTM!The update flow correctly validates ownership, whitelists update fields with
_pick, and returns fresh data after the update.
262-282: LGTM!The delete endpoint follows the standard pattern with proper authorization checks before deletion.
325-351: LGTM!The metadata condition builder correctly uses parameterized queries and ClickHouse array functions to safely filter by metadata key-value pairs.
353-485: LGTM!The stats endpoint correctly computes analytics metrics with proper handling of edge cases (division by zero via
|| 1fallback) and secure parameterized ClickHouse queries.
487-559: LGTM!The helper methods correctly handle time bucket granularities and date string generation for chart data alignment.
681-781: LGTM!The sessions endpoint efficiently uses
Promise.allfor parallel query execution and correctly aggregates session data with proper pagination.
47-53: No issue found. ThetimeBucketConversionmap is complete and correctly covers all possible values returned bygetLowestPossibleTimeBucket(). The function returns onlyMINUTE,HOUR,DAY,MONTH, orYEAR—never'week'. The map entries (minute, hour, day, month, year) match all possible return values, so the fallback to'toStartOfDay'will never be triggered by a missing 'week' entry.Likely an incorrect or invalid review comment.
| format: | ||
| // @ts-expect-error | ||
| timeFormat === TimeFormat['24-hour'] | ||
| ? (x: string) => d3.timeFormat(tbsFormatMapper24h[timeBucket])(x as unknown as Date) | ||
| : (x: string) => d3.timeFormat(tbsFormatMapper[timeBucket])(x as unknown as Date), | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find TimeFormat type definition
rg -n "TimeFormat" --type=ts -B 2 -A 10 | head -50Repository: Swetrix/swetrix
Length of output: 3056
🏁 Script executed:
# Check the specific file mentioned
head -20 web/app/lib/constants/index.tsRepository: Swetrix/swetrix
Length of output: 624
🏁 Script executed:
# Also check if TimeFormat is defined in the GoalsView file itself or imported
rg -n "TimeFormat" web/app/pages/Project/Goals/View/GoalsView.tsx -B 2 -A 2Repository: Swetrix/swetrix
Length of output: 977
Remove the unnecessary @ts-expect-error comment.
The same comparison timeFormat === TimeFormat['24-hour'] appears elsewhere in the file (lines 160, 179) without this suppression, indicating the code is valid TypeScript. The bracket notation on an enum is properly supported and doesn't produce type errors. Remove the comment on line 155 to maintain consistency.
🤖 Prompt for AI Agents
In web/app/pages/Project/Goals/View/GoalsView.tsx around lines 154 to 159,
remove the unnecessary "// @ts-expect-error" comment on line 155; the timeFormat
=== TimeFormat['24-hour'] comparison is valid TypeScript (as used elsewhere in
the file) so delete the suppression and keep the existing ternary format
function expressions intact, then run TypeScript/ESLint checks to confirm no
errors.
1 participant
Changes
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.