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 WalkthroughRefactored session management from Redis-based hashing to salted PSID derivation with SaltService. Introduced profile management system with ClickHouse-backed sessions table. Added profile-related endpoints and DTOs. Migrated analytics data to include profileId. Added new UI components for user/profile tracking. Changes
Sequence DiagramssequenceDiagram
participant Client
participant Controller as AnalyticsController
participant Service as AnalyticsService
participant SaltService
participant Redis
participant MySQL as MySQL/DB
participant ClickHouse
autonumber
Client->>Controller: POST /log (with pageview/event)
activate Controller
Controller->>Service: getSessionId(pid, userAgent, ip)
activate Service
Service->>SaltService: getSaltForSession()
activate SaltService
SaltService->>Redis: GET salt:daily
alt Cache Hit
Redis-->>SaltService: cached salt
else Cache Miss
SaltService->>MySQL: SELECT FROM salt WHERE rotation='daily'
alt Valid Salt Exists
MySQL-->>SaltService: existing salt
SaltService->>Redis: SET salt:daily (TTL 10min)
else Generate New
SaltService->>SaltService: generateSalt(bcrypt)
SaltService->>MySQL: UPSERT INTO salt
SaltService->>Redis: SET salt:daily (TTL 10min)
end
end
SaltService-->>Service: salt value
deactivate SaltService
Service->>Service: derivePsidFromInputs(pid, userAgent, ip, salt)
Service->>Service: getSessionKey(psid)
Service->>Redis: EXISTS ses:psid
alt Session Exists
Redis-->>Service: true
else New Session
Redis-->>Service: false
Service->>ClickHouse: INSERT INTO sessions(psid, pid, profileId, firstSeen, lastSeen, pageviews)
end
Service-->>Controller: { exists, psid }
deactivate Service
Controller->>Service: recordSession(psid, pid, profileId, isPageview)
activate Service
Service->>ClickHouse: UPDATE sessions (increment pageviews/events, update lastSeen)
Service->>Service: transformers apply profileId
ClickHouse-->>Service: ✓
Service-->>Controller: ✓
deactivate Service
Controller->>Service: extendSessionTTL(psid)
activate Service
Service->>Redis: EXPIRE ses:psid 30min
Redis-->>Service: ✓
Service-->>Controller: ✓
deactivate Service
Controller-->>Client: 200 OK
deactivate Controller
sequenceDiagram
participant User as Frontend User
participant UI as React Component
participant API as API Layer
participant Controller as AnalyticsController
participant Service as AnalyticsService
participant ClickHouse
autonumber
User->>UI: Select Profiles Tab
activate UI
UI->>API: getProfiles(pid, period, filters, take, skip, timezone)
activate API
API->>Controller: GET /profiles
activate Controller
Controller->>Service: getProfilesList(pid, filtersQuery, timezone, take, skip, profileType)
activate Service
Service->>ClickHouse: SELECT profileId, COUNT sessions, COUNT pageviews FROM sessions WHERE pid=? GROUP BY profileId LIMIT take OFFSET skip
ClickHouse-->>Service: profiles with metrics
Service-->>Controller: { profiles, appliedFilters, take, skip }
deactivate Service
Controller-->>API: 200 + profile data
deactivate Controller
API-->>UI: profiles array
deactivate API
UI->>UI: Render profile list
User->>UI: Click on profile row
UI->>API: getProfile(pid, profileId, period, timezone)
activate API
API->>Controller: GET /profiles/:profileId
activate Controller
Controller->>Service: getProfileDetails(pid, profileId, timezone)
activate Service
Service->>ClickHouse: SELECT details, avgDuration FROM sessions/analytics WHERE profileId=?
ClickHouse-->>Service: profile details
Service-->>Controller: ProfileDetails
deactivate Service
Controller->>Service: getProfileTopPages(pid, profileId)
Service->>ClickHouse: SELECT page, COUNT FROM analytics WHERE profileId=? GROUP BY page
ClickHouse-->>Service: top pages
Controller->>Service: getProfileActivityCalendar(pid, profileId, months)
Service->>ClickHouse: SELECT DATE(timestamp), COUNT FROM analytics WHERE profileId=? GROUP BY DATE(timestamp)
ClickHouse-->>Service: activity calendar
Controller->>Service: getProfileChartData(pid, profileId, timeBucket, from, to, timezone)
Service->>ClickHouse: SELECT timeBucket, pageviews, customEvents, errors FROM analytics WHERE profileId=? AND timestamp BETWEEN from AND to
ClickHouse-->>Service: chart data
Controller-->>API: { details, topPages, activityCalendar, chart }
deactivate Controller
API-->>UI: combined profile details
deactivate API
UI->>UI: Render UserDetails component with charts, stats, sessions
UI->>User: Display complete profile
deactivate UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
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 |
Blaumaus
changed the title
|
@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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/app/lib/constants/index.ts (1)
471-492: Fix SELFHOSTED_PROJECT_TABS key mismatch:usersshould beprofilesAt line 474,
SELFHOSTED_PROJECT_TABSdefines the key asusers, but code throughout the codebase (e.g., line 1472 in ViewProject.tsx) accessesPROJECT_TABS.profiles. This causes the profiles tab to have anundefinedid on self-hosted deployments.Change line 474 from
users: 'users'toprofiles: 'profiles'to align with production and all existing usages, or ensure all code branches onisSelfhostedbefore accessing environment-specific keys.
🧹 Nitpick comments (20)
web/app/ui/Tooltip.tsx (1)
36-40: LGTM! Configurable delay enhances flexibility.The addition of the
delayprop with a default value of 200 preserves backward compatibility while enabling callers to customize the tooltip delay. The implementation correctly forwards the delay toTooltipProvider.Optional observation: The prop name
delayshares its name with a utility function inweb/app/utils/generic.ts(line 147). While there's no actual collision since the utility isn't imported here, consider whether a more specific name liketooltipDelaymight reduce potential confusion for future maintainers.backend/apps/cloud/src/analytics/dto/pageviews.dto.ts (1)
31-37: OptionalprofileIdon PageviewsDto is consistent with the profiles modelThe new field and its Swagger/validator annotations look good; if there are any backend constraints (length, charset) on
profileId, consider documenting them here to keep clients aligned with storage/query expectations.backend/apps/community/src/project/entity/project.entity.ts (1)
6-26: SaltRotation enum + Project.saltRotation property align with the new rotation featureThe enum values and comments clearly document DAU/WAU/MAU semantics, and adding
saltRotation: SaltRotationonProjectmatches the new schema/defaults. If you expect to touch partially migrated data anywhere, consider makingsaltRotationoptional in the TS type until all environments are fully upgraded.backend/apps/cloud/src/analytics/dto/events.dto.ts (1)
90-97:profileIdon EventsDto matches the rest of the profile-aware DTOsThis optional field is wired consistently with the pageviews/error DTOs and clearly documented for API consumers; if there are backend limits on
profileId(length/charset), adding them to the description or via validators would make client behavior more predictable.backend/apps/cloud/src/analytics/entities/salt.entity.ts (1)
13-14: Consider adding an index onexpiresAtfor cleanup queries.If there will be scheduled cleanup of expired salts, an index on
expiresAtwould improve query performance. This is optional if the table remains small (one row per rotation type).backend/apps/cloud/src/analytics/dto/error.dto.ts (1)
35-42: LGTM! Consider adding@MaxLengthvalidation for consistency.The
profileIdfield is properly decorated. For consistency with other string fields in this DTO and to prevent excessively long profile IDs from being stored, consider adding a@MaxLengthconstraint.@ApiProperty({ example: 'user_12345', description: 'Optional profile ID for long-term user tracking. If not provided, one will be auto-generated.', }) @IsOptional() @IsString() + @MaxLength(128) profileId?: stringbackend/migrations/clickhouse/initialise_database.js (1)
151-164: LGTM! Well-designed sessions table with ReplacingMergeTree.The
ReplacingMergeTree(lastSeen)engine is appropriate for session tracking — it automatically deduplicates rows with the same(pid, psid)key, keeping the row with the latestlastSeentimestamp. The partition byfirstSeenmonth enables efficient data lifecycle management.Consider adding a TTL clause if sessions should be automatically cleaned up after a retention period:
TTL firstSeen + INTERVAL 2 YEARweb/app/pages/Project/Settings/ProjectSettings.tsx (1)
800-806: Type safety concern: casting toanyloses type checking.The
saltRotation: name as anycast bypasses TypeScript's type checking. Consider using a proper type assertion or defining the allowed values explicitly.- setSaltRotation={(name) => - setForm((prevForm) => ({ - ...prevForm, - saltRotation: name as any, - })) - } + setSaltRotation={(name: 'daily' | 'weekly' | 'monthly') => + setForm((prevForm) => ({ + ...prevForm, + saltRotation: name, + })) + }web/app/pages/Project/View/ViewProject.helpers.tsx (1)
1666-1677: Consider removing unusedshift+elistener.The
_SHORTCUTS_TABS_LISTENERSincludesshift+e, butSHORTCUTS_TABS_MAPdoesn't have an 'E' key mapping. If 'E' was previously used for something that's now removed, consider cleaning up the listener to avoid confusion.-const _SHORTCUTS_TABS_LISTENERS = 'shift+t, shift+p, shift+u, shift+s, shift+f, shift+e, shift+r' +const _SHORTCUTS_TABS_LISTENERS = 'shift+t, shift+p, shift+u, shift+s, shift+f, shift+r'web/app/pages/Project/Settings/tabs/General.tsx (1)
70-73: Consider fixing the type mismatch instead of suppressing.The
@ts-expect-erroranditem: anyinlabelExtractorindicate a type mismatch betweensaltRotationOptionsand whatSelectexpects. If theSelectcomponent'sitemsprop expects a different shape, consider either:
- Updating the
saltRotationOptionstype to match the expected interface- Adding proper generics to the
SelectcomponentThis would improve type safety and IDE support.
backend/apps/community/src/task-manager/task-manager.service.ts (1)
14-17: Consider adding error handling and logging.The cron job delegates to
saltService.regenerateExpiredSalts()but doesn't handle potential errors. If the salt regeneration fails, there's no logging or alerting.@Cron(CronExpression.EVERY_HOUR) async regenerateGlobalSalts() { - await this.saltService.regenerateExpiredSalts() + try { + await this.saltService.regenerateExpiredSalts() + } catch (error) { + this.logger.error(`[CRON WORKER](regenerateGlobalSalts) Error occurred: ${error}`) + } }web/app/utils/profileAvatars.tsx (1)
190-199: Consider handling empty or malformed profileId.If
profileIdis an empty string,generateDisplayNamewill always return the same name (first adjective + first noun). You may want to add a guard:export const getProfileDisplayName = (profileId: string, isIdentified: boolean): string => { + if (!profileId) { + return 'Unknown User' + } if (isIdentified) {backend/apps/cloud/src/task-manager/task-manager.service.ts (1)
689-692: Add error handling consistent with other cron jobs in this file.Other cron jobs in this service use
Promise.allSettled(...).catch()or try/catch blocks with logging. This job should follow the same pattern:@Cron(CronExpression.EVERY_HOUR) async regenerateGlobalSalts() { - await this.saltService.regenerateExpiredSalts() + try { + await this.saltService.regenerateExpiredSalts() + } catch (reason) { + this.logger.error( + `[CRON WORKER](regenerateGlobalSalts) Error occured: ${reason}`, + ) + } }backend/apps/cloud/src/analytics/dto/get-profile.dto.ts (1)
37-41: Add@IsOptional()decorators tofromandtofields for consistency.These fields are typed as optional with
?but lack the@IsOptional()decorator that other optional fields in this DTO use (e.g.,timezone,period). This inconsistency could lead to validation failures.@ApiProperty({ required: false }) + @IsOptional() from?: string @ApiProperty({ required: false }) + @IsOptional() to?: stringbackend/apps/cloud/src/analytics/salt.service.ts (1)
44-46: Consider using a higher bcrypt cost factor for production salts.While cost factor 10 is reasonable for development, production security-sensitive salts may benefit from a higher factor (12-14). However, since these salts are cached and regenerated infrequently, the performance impact would be minimal.
web/app/pages/Project/View/ViewProject.tsx (1)
3908-3941: EnsureUserDetailssafely handlesdetails={activeProfile}when it can benullIn the profile detail render,
UserDetailsis invoked withdetails={activeProfile}, butactiveProfileis explicitly set tonullon load failures and before initial data arrives. Make sureUserDetails’s prop typing and implementation allowdetailsto benull(or gate the render onactiveProfilebeing truthy) to avoid runtime errors.For example, either:
- Type
detailsasProfileDetails | nulland handle the empty state insideUserDetails, or- Wrap the component:
{activeProfile && <UserDetails details={activeProfile} .../>}.web/app/pages/Project/View/components/Users.tsx (1)
37-137: Profiles list & filter components look solid; consider a small a11y tweakThe
UserRow,Users, andUsersFiltercomponents are well structured and integrate cleanly with the new profiles flow (URL state, time formatting, badges, icons).One minor accessibility improvement: since
UsersFilter’s three buttons behave as toggleable filter pills, you could mark them as toggle buttons witharia-pressedso screen readers convey their state:- <button + <button type='button' onClick={() => onProfileTypeChange('all')} - className={filterButtonClass(profileType === 'all')} + className={filterButtonClass(profileType === 'all')} + aria-pressed={profileType === 'all'} > {t('project.allUsers')} </button> ... - <button + <button type='button' onClick={() => onProfileTypeChange('anonymous')} - className={filterButtonClass(profileType === 'anonymous')} + className={filterButtonClass(profileType === 'anonymous')} + aria-pressed={profileType === 'anonymous'} > {t('project.anonymous')} </button> ... - <button + <button type='button' onClick={() => onProfileTypeChange('identified')} - className={filterButtonClass(profileType === 'identified')} + className={filterButtonClass(profileType === 'identified')} + aria-pressed={profileType === 'identified'} > {t('project.identified')} </button>Purely optional, but improves assistive-tech behavior without changing visuals.
Also applies to: 140-181, 183-201
backend/apps/community/src/analytics/analytics.service.ts (1)
1124-1137: Consider using delimiters in hash input to prevent potential concatenation collisions.While unlikely in practice, concatenating strings without delimiters could theoretically lead to collisions (e.g.,
"abc" + "def"equals"ab" + "cdef"). Adding a delimiter would be more robust.derivePsidFromInputs( pid: string, userAgent: string, ip: string, salt: string, ): string { - const combined = `${userAgent}${ip}${pid}${salt}` + const combined = `${userAgent}|${ip}|${pid}|${salt}` return crypto .createHash('sha256') .update(combined) .digest() .readBigUInt64BE(0) .toString() }backend/apps/cloud/src/analytics/analytics.service.ts (2)
1682-1689: Session‑duration aggregation doesn’t fully honor filters/time windows and may overweight noisy sessionsA few details around
sdurcould surprise you:
- In the
period === 'all'path,duration_avgisavg(dateDiff('second', firstSeen, lastSeen)) FROM sessions FINAL WHERE pid = {pid}, i.e. it ignoresfiltersQuery. The counts (all/unique) are filtered; the duration is global for the project.- In the non‑
allpath, you constrainsessionsbypsid IN (SELECT DISTINCT psid FROM analytics ... BETWEEN groupFromUTC/groupToUTC ${filtersQuery})– good – but:
generateAnalyticsAggregationQueryjoinssessions_dataon(pid, psid)without deduping the subquery. Sessions with more pageviews in a bucket contribute their duration multiple times, biasing the bucket’s average toward noisy sessions.sessions_dataitself is not time‑bounded, so a long‑lived session’s full duration is reused for every bucket where it has at least one event.If you want
sdurto mean “average session duration for the same filtered/time‑bounded sessions”:
- Consider constraining
sessionsin theperiod === 'all'path via the samepsidset as analytics (or computing durations from analytics directly, as you already do ingetProfileDetails).- In
generateAnalyticsAggregationQuery, change the inner analytics subquery to dedupe by psid per bucket (e.g.SELECT DISTINCT pid, psid, tz_created ...) before joining tosessions_data, so each session contributes once per bucket.These aren’t hard correctness bugs, but they do make the reported
sdurharder to interpret.Also applies to: 1764-1778, 1794-1808, 2555-2582
4310-4401: Profile queries work but have mixed time/filter semantics and a couple of minor nitsA few things to be aware of in the new profile APIs:
- Time window in
getProfilesList: the CTEs overanalytics,customEV, anderrorsdon’t constraincreatedby{groupFrom}/{groupTo}at all – only${filtersQuery}is applied (and only toanalytics). If the UI expects “profiles active in the selected period”, this will currently return lifetime stats. If that’s not intentional, add acreated BETWEEN {groupFrom:String} AND {groupTo:String}clause consistent with the rest of the service.- Filter symmetry:
filtersQueryis only applied in theanalyticsCTE ofgetProfilesList/getProfileSessionsList, not in thecustomEV/errorsCTEs. That means pageviews respect filters while events/errors do not. If filters are meant to narrow “per‑profile events/errors” the same way, consider threading${filtersQuery}into those CTEs as well.- Session duration for profile sessions:
session_duration_aggingetProfileSessionsListreuses theavg(dateDiff('second', firstSeen, lastSeen))pattern from the global sessions list, without time bounds. This shares the same concerns as the main sessions duration comment – once sessions store multiple rows,sduris unlikely to reflect real session lengths.- Unused timezone args:
getProfilesListacceptssafeTimezonebut doesn’t use it, andgetProfileDetailsprefixes it with_to silence TS. If you don’t plan to timezone‑shift profile timestamps server‑side, you can drop these parameters to keep the signature lean.Functionally everything should run, but aligning time windows/filters across analytics, events, errors, and sessions will make the profile views much more predictable.
Also applies to: 4403-4515, 4517-4542, 4544-4575, 4577-4681, 4683-4787
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (45)
backend/apps/cloud/src/analytics/analytics.controller.ts(16 hunks)backend/apps/cloud/src/analytics/analytics.module.ts(1 hunks)backend/apps/cloud/src/analytics/analytics.service.ts(12 hunks)backend/apps/cloud/src/analytics/dto/error.dto.ts(1 hunks)backend/apps/cloud/src/analytics/dto/events.dto.ts(1 hunks)backend/apps/cloud/src/analytics/dto/get-profile.dto.ts(1 hunks)backend/apps/cloud/src/analytics/dto/get-profiles.dto.ts(1 hunks)backend/apps/cloud/src/analytics/dto/pageviews.dto.ts(1 hunks)backend/apps/cloud/src/analytics/entities/salt.entity.ts(1 hunks)backend/apps/cloud/src/analytics/salt.service.ts(1 hunks)backend/apps/cloud/src/analytics/utils/transformers.ts(6 hunks)backend/apps/cloud/src/common/constants.ts(0 hunks)backend/apps/cloud/src/project/entity/project.entity.ts(2 hunks)backend/apps/cloud/src/task-manager/task-manager.service.ts(3 hunks)backend/apps/community/src/analytics/analytics.controller.ts(11 hunks)backend/apps/community/src/analytics/analytics.module.ts(1 hunks)backend/apps/community/src/analytics/analytics.service.ts(11 hunks)backend/apps/community/src/analytics/entities/salt.entity.ts(1 hunks)backend/apps/community/src/analytics/salt.service.ts(1 hunks)backend/apps/community/src/common/constants.ts(0 hunks)backend/apps/community/src/common/utils.ts(1 hunks)backend/apps/community/src/project/entity/project.entity.ts(2 hunks)backend/apps/community/src/task-manager/task-manager.module.ts(1 hunks)backend/apps/community/src/task-manager/task-manager.service.ts(1 hunks)backend/migrations/clickhouse/2025_12_02_profile_id.js(1 hunks)backend/migrations/clickhouse/initialise_database.js(4 hunks)backend/migrations/clickhouse/initialise_selfhosted.js(1 hunks)backend/migrations/clickhouse/selfhosted_2025_11_30_salt_rotation.js(1 hunks)backend/migrations/clickhouse/selfhosted_2025_12_02_profile_id.js(1 hunks)backend/migrations/mysql/2025_11_30_salt_rotation.sql(1 hunks)backend/migrations/mysql/2025_12_05_salt_table.sql(1 hunks)web/app/api/index.ts(2 hunks)web/app/lib/constants/index.ts(2 hunks)web/app/lib/models/Project.ts(2 hunks)web/app/pages/Project/Settings/ProjectSettings.tsx(3 hunks)web/app/pages/Project/Settings/tabs/General.tsx(2 hunks)web/app/pages/Project/View/ViewProject.helpers.tsx(1 hunks)web/app/pages/Project/View/ViewProject.tsx(10 hunks)web/app/pages/Project/View/components/RefreshStatsButton.tsx(1 hunks)web/app/pages/Project/View/components/UserDetails.tsx(1 hunks)web/app/pages/Project/View/components/Users.tsx(1 hunks)web/app/ui/Tooltip.tsx(1 hunks)web/app/utils/profileAvatars.tsx(1 hunks)web/package.json(1 hunks)web/public/locales/en.json(3 hunks)
💤 Files with no reviewable changes (2)
- backend/apps/community/src/common/constants.ts
- backend/apps/cloud/src/common/constants.ts
🧰 Additional context used
🧬 Code graph analysis (18)
backend/migrations/clickhouse/selfhosted_2025_11_30_salt_rotation.js (1)
backend/migrations/clickhouse/setup.js (2)
dbName(107-107)queriesRunner(76-105)
web/app/ui/Tooltip.tsx (1)
web/app/utils/generic.ts (1)
delay(148-148)
backend/apps/cloud/src/analytics/dto/get-profile.dto.ts (1)
web/app/lib/constants/index.ts (1)
DEFAULT_TIMEZONE(349-349)
backend/apps/community/src/analytics/entities/salt.entity.ts (1)
backend/apps/community/src/analytics/salt.service.ts (1)
SaltRotationType(17-17)
backend/apps/community/src/analytics/analytics.module.ts (2)
backend/apps/cloud/src/analytics/analytics.module.ts (1)
Module(12-23)backend/apps/community/src/task-manager/task-manager.module.ts (1)
Module(6-11)
backend/apps/cloud/src/analytics/analytics.module.ts (2)
backend/apps/community/src/analytics/analytics.module.ts (1)
Module(11-17)backend/apps/community/src/task-manager/task-manager.module.ts (1)
Module(6-11)
backend/apps/cloud/src/analytics/entities/salt.entity.ts (1)
backend/apps/cloud/src/analytics/salt.service.ts (1)
SaltRotationType(17-17)
backend/migrations/clickhouse/initialise_database.js (1)
backend/migrations/clickhouse/setup.js (1)
dbName(107-107)
web/app/pages/Project/View/components/UserDetails.tsx (7)
web/app/lib/models/Project.ts (1)
Session(85-99)web/app/utils/generic.ts (4)
cn(150-152)getStringFromTime(81-91)getTimeFromSeconds(64-79)getLocaleDisplayName(134-146)web/app/lib/constants/index.ts (3)
BROWSER_LOGO_MAP(930-977)OS_LOGO_MAP(979-1005)OS_LOGO_MAP_DARK(1007-1012)web/app/providers/ThemeProvider.tsx (1)
useTheme(29-37)web/app/utils/profileAvatars.tsx (2)
getProfileDisplayName(190-199)ProfileAvatar(210-216)web/app/pages/Project/View/components/SessionChart.tsx (1)
SessionChart(24-52)web/app/pages/Project/View/components/Sessions.tsx (1)
Sessions(165-183)
web/app/pages/Project/View/ViewProject.helpers.tsx (1)
web/app/lib/constants/index.ts (1)
PROJECT_TABS(490-492)
backend/apps/cloud/src/analytics/salt.service.ts (3)
backend/apps/community/src/analytics/salt.service.ts (1)
SaltRotationType(17-17)backend/apps/community/src/common/constants.ts (1)
redis(105-105)backend/apps/cloud/src/common/constants.ts (1)
redis(170-170)
web/app/api/index.ts (2)
web/app/lib/models/Project.ts (3)
Profile(122-135)ProfileDetails(137-153)Session(85-99)web/app/pages/Project/View/interfaces/traffic.ts (1)
Filter(3-9)
web/app/pages/Project/View/components/Users.tsx (2)
web/app/utils/profileAvatars.tsx (2)
getProfileDisplayName(190-199)ProfileAvatar(210-216)web/app/ui/Badge.tsx (1)
Badge(11-34)
backend/apps/cloud/src/analytics/analytics.controller.ts (4)
backend/apps/cloud/src/analytics/utils/transformers.ts (1)
customEventTransformer(77-128)backend/apps/community/src/analytics/utils/transformers.ts (1)
customEventTransformer(75-124)backend/apps/cloud/src/analytics/analytics.service.ts (1)
getLowestPossibleTimeBucket(205-246)backend/apps/cloud/src/analytics/dto/get-profile.dto.ts (2)
GetProfileDto(8-42)GetProfileSessionsDto(44-66)
backend/apps/community/src/analytics/analytics.service.ts (2)
backend/apps/community/src/common/constants.ts (2)
redis(105-105)UNIQUE_SESSION_LIFE_TIME(109-109)backend/apps/cloud/src/common/constants.ts (2)
redis(170-170)UNIQUE_SESSION_LIFE_TIME(174-174)
web/app/pages/Project/View/ViewProject.tsx (5)
web/app/lib/models/Project.ts (3)
Profile(122-135)ProfileDetails(137-153)Session(85-99)web/app/lib/constants/index.ts (1)
PROJECT_TABS(490-492)web/app/api/index.ts (3)
getProfiles(669-695)getProfile(697-718)getProfileSessions(720-746)web/app/pages/Project/View/components/Users.tsx (2)
UsersFilter(145-181)Users(183-201)web/app/pages/Project/View/components/UserDetails.tsx (1)
UserDetails(195-402)
backend/apps/cloud/src/analytics/analytics.service.ts (3)
backend/apps/cloud/src/common/constants.ts (2)
redis(170-170)UNIQUE_SESSION_LIFE_TIME(174-174)backend/apps/community/src/common/utils.ts (1)
hash(79-85)backend/apps/cloud/src/common/utils.ts (1)
hash(22-28)
backend/apps/community/src/task-manager/task-manager.module.ts (2)
backend/apps/community/src/analytics/analytics.module.ts (1)
Module(11-17)backend/apps/cloud/src/analytics/analytics.module.ts (1)
Module(12-23)
🪛 Biome (2.1.2)
web/public/locales/en.json
[error] 1029-1029: The key noData was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
🔇 Additional comments (51)
web/app/pages/Project/View/components/RefreshStatsButton.tsx (1)
103-103: LGTM! Z-index addition fixes tooltip overlap.Adding
z-30ensures the tooltip appears above other elements, resolving potential stacking issues. The value is appropriate for tooltip layering.web/package.json (1)
39-39: Newboring-avatarsdependency looks appropriateThe dependency addition is straightforward and scoped; just ensure the lockfile is updated and CI/build output looks good (bundle size, SSR compatibility) once the new avatar UI is wired.
backend/apps/community/src/common/utils.ts (1)
115-128: AllowingsaltRotationupdates in ClickHouse utilities is correctAdding
'saltRotation'toALLOWED_KEYS(and therefore toCLICKHOUSE_PROJECT_UPDATABLE_KEYS) cleanly enables updating the rotation mode while keeping the existing filtering logic intact and type handling (INT8_COLUMNS) unchanged.backend/apps/cloud/src/analytics/utils/transformers.ts (1)
26-181: Extending transformers withprofileIdis consistent but tightens call-site requirementsAdding
profileIdas a required argument and field in the traffic/custom/error transformers cleanly aligns stored rows with the new profile dimension. Please double‑check that all call sites now pass a normalized, non‑PIIprofileIdand that the corresponding ClickHouse tables include a matchingprofileIdcolumn with compatible type.backend/migrations/clickhouse/initialise_selfhosted.js (1)
21-36: AddingsaltRotationto the self‑hosted ClickHouseprojecttable is aligned with the entityThe new
saltRotation String DEFAULT 'daily'column matches the Project entity and rotation model; ensure the standalone migrations for existing self‑hosted installations add the same column/default so upgrade paths stay consistent.backend/migrations/clickhouse/selfhosted_2025_11_30_salt_rotation.js (1)
1-8: LGTM! Migration follows established patterns.The migration correctly adds the
saltRotationcolumn with a sensible default. Note that ClickHouse usesStringtype while the MySQL migration usesENUM('daily', 'weekly', 'monthly')— this is acceptable since ClickHouse doesn't have a native enum equivalent for this use case, but application-level validation will be the enforcement point.backend/migrations/mysql/2025_11_30_salt_rotation.sql (1)
1-3: LGTM! Migration is well-documented.The migration correctly adds the
saltRotationcolumn with appropriate ENUM values matching theSaltRotationenum inproject.entity.ts. The comment clearly explains the column's purpose.backend/apps/cloud/src/project/entity/project.entity.ts (2)
27-31: LGTM! Well-documented enum with clear use cases.The
SaltRotationenum is properly defined with helpful comments explaining each rotation period's purpose (DAU, WAU, MAU tracking).
99-105: LGTM! Column properly configured.The
saltRotationproperty is correctly decorated with TypeORM and Swagger decorators, with an appropriate default value.backend/migrations/mysql/2025_12_05_salt_table.sql (1)
1-9: LGTM! Schema aligns with the Salt entity.The table structure correctly matches the
SaltTypeORM entity. The primary key onrotationensures only one active salt per rotation type, which is the intended design.backend/apps/cloud/src/analytics/analytics.module.ts (1)
1-21: LGTM! Module properly integrates Salt infrastructure.The
AnalyticsModulecorrectly registers theSaltentity with TypeORM and provides/exportsSaltService. This follows the same pattern as the community module.backend/migrations/clickhouse/initialise_database.js (1)
10-11: LGTM!profileIdcolumn added consistently.The
profileIdcolumn is correctly added asNullable(String)since not all events will have an associated profile.backend/apps/community/src/analytics/analytics.module.ts (1)
1-17: LGTM! Module configuration is correct and consistent with the cloud edition.The module properly registers the Salt entity with TypeORM, provides SaltService, and exports it for dependent modules. The structure aligns with the cloud edition's analytics module.
backend/apps/community/src/task-manager/task-manager.module.ts (1)
1-11: LGTM! Proper use offorwardReffor circular dependency resolution.The
forwardRef(() => AnalyticsModule)correctly handles the circular dependency between TaskManagerModule and AnalyticsModule, enabling TaskManagerService to access SaltService for salt regeneration workflows.backend/apps/community/src/analytics/entities/salt.entity.ts (1)
1-18: LGTM! Well-structured entity for salt persistence.The entity design is appropriate:
- Primary key on
rotationensures one salt per rotation typeexpiresAtenables expiration-based rotation logic- Timestamp columns use proper database defaults
backend/migrations/clickhouse/2025_12_02_profile_id.js (2)
29-42: Migration semantics note:lastSeenfor historical data reflects migration time.The migration correctly computes
firstSeenfrom the duration, butlastSeenis set tonow()(migration execution time) for all historical records. This is acceptable since the originalsession_durationstable didn't track exact timestamps, but be aware that historical session data will show the migration date aslastSeen.
44-45: Good practice: commented-out DROP TABLE for manual verification.The commented-out
DROP TABLEallows manual verification of migrated data before removing the source table. Consider adding a follow-up migration or documentation note to remind operators to drop the table after verification.web/app/lib/models/Project.ts (1)
122-153: LGTM! Well-structured Profile interfaces.The
ProfileandProfileDetailsinterfaces provide comprehensive typing for profile data with appropriate optional fields. The extension pattern (ProfileDetails extends Profile) is clean and maintainable.backend/migrations/clickhouse/selfhosted_2025_12_02_profile_id.js (1)
13-27: Sessions table schema looks well-designed.The
ReplacingMergeTree(lastSeen)engine withORDER BY (pid, psid)correctly enables upsert semantics for session updates. Partitioning bytoYYYYMM(firstSeen)is appropriate for time-series data.web/app/pages/Project/Settings/tabs/General.tsx (1)
65-78: Salt rotation selector implementation looks functional.The Select component is correctly wired up with the form state. Minor note: both
titleandselectedItemperform the samefindoperation—you could extract this to a variable if preferred, but it's not critical.web/app/utils/profileAvatars.tsx (2)
172-185: Hash function provides ~5,000 unique combinations.With 44 adjectives × 114 nouns = 5,016 unique display names. This may lead to collisions for projects with many anonymous users. Consider whether this level of uniqueness is acceptable for your use case, or if you need to expand the word lists or add a numeric suffix for disambiguation.
207-216: ProfileAvatar component is well-structured.Clean implementation using
boring-avatarswith deterministic avatars based onprofileId. The wrapper div allows flexible styling viaclassName.backend/apps/cloud/src/task-manager/task-manager.service.ts (1)
31-31: SaltService integration looks correct.The
SaltServiceis properly imported and injected following the existing dependency injection pattern in the constructor.Also applies to: 296-296
web/app/pages/Project/View/components/UserDetails.tsx (3)
42-140: Well-structured ActivityCalendar component with appropriate memoization.The component correctly uses
useMemofor expensive computations (countMap,weeks,monthLabels) with proper dependency arrays. The week/day iteration logic handles edge cases appropriately.
195-229: Good implementation of display name and date formatting with proper null handling.The component correctly handles the null
detailscase with a Loader fallback, and memoizes the display name computation.
376-396: Sessions loading state handles edge cases well.The conditional rendering properly handles the three states: loading with no sessions, empty sessions, and sessions with load-more capability.
backend/apps/community/src/analytics/analytics.controller.ts (3)
91-92: LGTM - Online visitors window constant is appropriately defined.The 5-minute window aligns with typical real-time analytics requirements for determining active sessions.
982-996: Heartbeat session handling correctly validates session existence before extending TTL.The flow properly checks for session existence and throws ForbiddenException if no session exists, preventing orphan heartbeats.
1022-1022:recordSessioncorrectly placed after session ID generation.The
isHeartbeat: trueflag appropriately marks this as a session-initiating pageview event.backend/apps/cloud/src/analytics/salt.service.ts (2)
103-131:regenerateExpiredSaltsimplementation is correct for scheduled tasks.The method properly iterates through rotation types, checks expiration, and persists new salts with Redis caching. Good logging for observability.
61-101: No race condition risk — rotation is the primary key.The
rotationfield is marked as@PrimaryColumn, making it the primary key of theSaltentity. This enforces uniqueness at the database level, preventing duplicate salts from being created under concurrent load. The repository's upsert operation (save()) will handle concurrent requests safely.Likely an incorrect or invalid review comment.
web/public/locales/en.json (1)
1212-1220: Salt rotation translation keys added correctly.The options clearly explain the behavior of each rotation period setting.
web/app/api/index.ts (1)
669-695: Profiles API client wiring looks consistent and type-safeThe three new helpers (
getProfiles,getProfile,getProfileSessions) mirror existing analytics/session APIs (encoding filters, forwardingx-password, and shaping responses toProfile/ProfileDetails/Session[]). URL encoding ofprofileIdand optional period/from/to handling look correct. No issues from my side here.Also applies to: 697-718, 720-746
backend/apps/community/src/analytics/salt.service.ts (1)
61-101: Therotationcolumn is already a primary key—no action neededThe
Saltentity definesrotationas@PrimaryColumn, ensuring only one row exists per rotation type. Thesave()andfindOne()operations are correct and will function as expected. No verification or changes required.Likely an incorrect or invalid review comment.
backend/apps/cloud/src/analytics/analytics.controller.ts (10)
77-78: LGTM!New DTO imports for profile-related endpoints are correctly added and align with the new profile management feature.
1163-1175: LGTM!The profileId generation and session recording flow for error logging is correctly implemented, following the same pattern as other endpoints.
1303-1315: LGTM!Profile ID generation and session recording for custom events correctly uses
isPageview: falseto distinguish from pageview events.
1379-1399: LGTM!Heartbeat endpoint correctly updates session activity and extends TTL for existing sessions.
1427-1434: LGTM!Pageview logging correctly generates profile ID and records the session with
isPageview: true.
1569-1575: LGTM!Noscript endpoint correctly handles profile ID generation without client-provided profileId, as JavaScript-disabled clients cannot send this parameter.
1849-1868: LGTM!The getSession endpoint is properly structured with access control checks and timezone handling.
2003-2015: LGTM!Good use of
Promise.allto parallelize fetching profile details, top pages, activity calendar, and chart data for better performance.
2075-2076: Consider whethercustomEVFilterAppliedshould be used for profile sessions.Similar to
getProfiles, the fourth return value fromgetFiltersQueryis not captured. If custom event filters should affect profile session queries, this may need adjustment.
1919-1920: No action needed. ThegetFiltersQuerymethod returns 4 values, butcustomEVFilterAppliedis not destructured ingetProfilesListby design—the same pattern is consistently used across all endpoints (getSessions,getErrors,getPage, andgetConversations), andcustomEVFilterAppliedis not used anywhere in the codebase. This is not an inconsistency specific to profile listing.Likely an incorrect or invalid review comment.
backend/apps/community/src/analytics/analytics.service.ts (5)
330-333: LGTM!SaltService is properly injected into the constructor following NestJS dependency injection patterns.
1439-1441: LGTM!The migration from
session_durationsto the newsessionstable withdateDiffcalculation is correctly implemented. TheFINALmodifier ensures merged rows are used.
2270-2275: LGTM!The sessions table join for duration calculation is correctly implemented with
FINALmodifier and proper join conditions.
3655-3656: LGTM!Session duration query correctly updated to use the new
sessionstable withFINALmodifier.
3857-3858: LGTM!The session duration aggregation in
getSessionsListcorrectly uses the newsessionstable with proper averaging logic.backend/apps/cloud/src/analytics/analytics.service.ts (2)
1115-1128: PSID and profile ID hashing approach looks solidUsing SHA‑256 → UInt64 for
psid/profile IDs with per‑purpose salts andanon_/usr_prefixes gives deterministic, non‑guessable identifiers while keeping ClickHouse types simple. The stripping of known prefixes fromuserSuppliedbefore hashing also avoids accidental collisions between anonymous and identified profiles.Also applies to: 1165-1200
3865-3900: Online user count query looks correct and reasonably defensiveThe 30‑minute “online” window combining analytics and custom events with
uniqExact(psid)and guarding onpsid IS NOT NULLis straightforward, and the try/catch fallback to0on errors is acceptable for a best‑effort metric.
backend/migrations/clickhouse/selfhosted_2025_12_02_profile_id.js
Outdated
Show resolved
Hide resolved
1 participant
#421
salt + pid + request info, instead of storing key-value pair in RedisTODOs:
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.