Rework journeys#592
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughJourney analytics now reports continuation and path-length coverage, exposes node/link breakdowns, and renders these details in an expanded Sankey and table UI. Journey controls persist in URL parameters, while funnel and journey endpoints tolerate selected analytics lookup failures. ChangesJourney analytics expansion
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant JourneysView
participant AnalyticsController
participant AnalyticsService
participant ClickHouse
JourneysView->>AnalyticsController: request journeys
AnalyticsController->>AnalyticsService: fetch flow and node/link details
AnalyticsService->>ClickHouse: execute journey aggregation queries
ClickHouse-->>AnalyticsService: return paths, histogram, nodes, and links
AnalyticsService-->>AnalyticsController: return journey data
AnalyticsController-->>JourneysView: render flow and details
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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)
1206-1234: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a deterministic tie-breaker to journey ranking
getJourneysandgetJourneyNodeDetailsare called together in the same response, so tiedvaluecounts can make the journeys list and Sankey details pick different paths. Addpath ASCto bothrow_number()ORDER BYclauses in:
backend/apps/cloud/src/analytics/analytics.service.ts#L1206-L1234backend/apps/cloud/src/analytics/analytics.service.ts#L1351-L1363backend/apps/community/src/analytics/analytics.service.ts#L1009-L1037backend/apps/community/src/analytics/analytics.service.ts#L1154-L1166🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/cloud/src/analytics/analytics.service.ts` around lines 1206 - 1234, Make journey ranking deterministic by adding path ASC as the secondary sort key after value DESC in both row_number() ORDER BY clauses at backend/apps/cloud/src/analytics/analytics.service.ts:1206-1234 and :1351-1363, and backend/apps/community/src/analytics/analytics.service.ts:1009-1037 and :1154-1166. Apply the same tie-breaker consistently in getJourneys and getJourneyNodeDetails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/apps/cloud/src/analytics/v2/analytics-v2.service.ts`:
- Around line 1707-1720: Replace the this.logger.log call in the
getFunnelTimeToConvert catch block with the error-level logging method,
preserving the existing reason and contextual message so failures are emitted in
production.
In `@web/app/pages/Project/tabs/Journeys/JourneysView.tsx`:
- Around line 1197-1200: Update the labelTextColor configuration in the journey
node rendering to choose a contrasting color for light-theme nodes based on each
node’s kind or background luminance, ensuring labels remain readable on
saturated blue, purple, and similar backgrounds while preserving the existing
dark-theme behavior.
- Around line 703-725: Remove the global hiddenPaths calculation and its
hidden-path count from the node.kind === 'other' tooltip in JourneysView. Keep
the existing “Other paths” label, step, and session count unchanged; do not
display a count that is not scoped to the current step.
- Around line 260-277: Add an effect in JourneysView that watches searchParams
and synchronizes steps, journeysCount, and view from the current URL values
using the same clamping and defaults as their initializers. Ensure changed query
parameters update the local controls and downstream debounced/fetched data while
preserving existing values for valid chart/table views.
---
Outside diff comments:
In `@backend/apps/cloud/src/analytics/analytics.service.ts`:
- Around line 1206-1234: Make journey ranking deterministic by adding path ASC
as the secondary sort key after value DESC in both row_number() ORDER BY clauses
at backend/apps/cloud/src/analytics/analytics.service.ts:1206-1234 and
:1351-1363, and
backend/apps/community/src/analytics/analytics.service.ts:1009-1037 and
:1154-1166. Apply the same tie-breaker consistently in getJourneys and
getJourneyNodeDetails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7dfe59e9-444f-45e5-a67c-fb485f82e827
📒 Files selected for processing (12)
backend/apps/cloud/src/analytics/analytics.controller.tsbackend/apps/cloud/src/analytics/analytics.service.tsbackend/apps/cloud/src/analytics/interfaces/index.tsbackend/apps/cloud/src/analytics/v2/analytics-v2.service.tsbackend/apps/community/src/analytics/analytics.controller.tsbackend/apps/community/src/analytics/analytics.service.tsbackend/apps/community/src/analytics/interfaces/index.tsbackend/apps/community/src/analytics/v2/analytics-v2.service.tsweb/app/api/api.server.tsweb/app/pages/Project/View/ViewProject.helpers.tsxweb/app/pages/Project/tabs/Journeys/JourneysView.tsxweb/public/locales/en.json
Comment on lines
1707
to
1720
| (async () => { | ||
| timeToConvert = await this.analyticsService.getFunnelTimeToConvert( | ||
| steps, | ||
| params, | ||
| filters.filtersQuery, | ||
| ) | ||
| try { | ||
| timeToConvert = await this.analyticsService.getFunnelTimeToConvert( | ||
| steps, | ||
| params, | ||
| filters.filtersQuery, | ||
| ) | ||
| } catch (reason) { | ||
| this.logger.log( | ||
| reason, | ||
| 'AnalyticsV2Service -> getFunnelTimeToConvert', | ||
| ) | ||
| } | ||
| })(), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Failure is logged with .log(), which is silently dropped in production.
Based on learnings, AppLoggerService.log(value, route, forceLog=false) only emits when isDevelopment is true or forceLog is explicitly set — so a getFunnelTimeToConvert failure here produces no log entry in production, unlike the V1 controllers' equivalent handling of this same failure (which use this.logger.error(...)). This weakens the ability to detect/debug the very failures this resilience wrapping is meant to tolerate.
🔧 Proposed fix
} catch (reason) {
- this.logger.log(
- reason,
- 'AnalyticsV2Service -> getFunnelTimeToConvert',
- )
+ this.logger.error(
+ reason,
+ 'AnalyticsV2Service -> getFunnelTimeToConvert',
+ )
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| (async () => { | |
| timeToConvert = await this.analyticsService.getFunnelTimeToConvert( | |
| steps, | |
| params, | |
| filters.filtersQuery, | |
| ) | |
| try { | |
| timeToConvert = await this.analyticsService.getFunnelTimeToConvert( | |
| steps, | |
| params, | |
| filters.filtersQuery, | |
| ) | |
| } catch (reason) { | |
| this.logger.log( | |
| reason, | |
| 'AnalyticsV2Service -> getFunnelTimeToConvert', | |
| ) | |
| } | |
| })(), | |
| (async () => { | |
| try { | |
| timeToConvert = await this.analyticsService.getFunnelTimeToConvert( | |
| steps, | |
| params, | |
| filters.filtersQuery, | |
| ) | |
| } catch (reason) { | |
| this.logger.error( | |
| reason, | |
| 'AnalyticsV2Service -> getFunnelTimeToConvert', | |
| ) | |
| } | |
| })(), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/apps/cloud/src/analytics/v2/analytics-v2.service.ts` around lines
1707 - 1720, Replace the this.logger.log call in the getFunnelTimeToConvert
catch block with the error-level logging method, preserving the existing reason
and contextual message so failures are emitted in production.
Source: Learnings
Comment on lines
+260
to
+277
| const [searchParams, setSearchParams] = useSearchParams() | ||
|
|
||
| const [steps, setSteps] = useState(DEFAULT_STEPS) | ||
| const [journeysCount, setJourneysCount] = useState(DEFAULT_JOURNEYS) | ||
| const [steps, setSteps] = useState(() => { | ||
| const value = Number(searchParams.get('steps')) | ||
| return Number.isFinite(value) && value ? clampSteps(value) : DEFAULT_STEPS | ||
| }) | ||
| const [journeysCount, setJourneysCount] = useState(() => { | ||
| const value = Number(searchParams.get('journeys')) | ||
| return Number.isFinite(value) && value | ||
| ? clampJourneys(value) | ||
| : DEFAULT_JOURNEYS | ||
| }) | ||
| const [view, setView] = useState<'chart' | 'table'>(() => | ||
| searchParams.get('view') === 'table' ? 'table' : 'chart', | ||
| ) | ||
| const [debouncedParams, setDebouncedParams] = useState({ | ||
| steps: DEFAULT_STEPS, | ||
| journeys: DEFAULT_JOURNEYS, | ||
| steps, | ||
| journeys: journeysCount, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Synchronize local controls when the URL changes after mount.
These initializers only read searchParams once. Browser navigation or in-app navigation to another steps, journeys, or view query leaves the controls and fetched data stale relative to the URL. Add an effect that parses and applies changed search parameters.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/app/pages/Project/tabs/Journeys/JourneysView.tsx` around lines 260 - 277,
Add an effect in JourneysView that watches searchParams and synchronizes steps,
journeysCount, and view from the current URL values using the same clamping and
defaults as their initializers. Ensure changed query parameters update the local
controls and downstream debounced/fetched data while preserving existing values
for valid chart/table views.
Comment on lines
+703
to
+725
| if (node.kind === 'other') { | ||
| const hiddenPaths = Math.max( | ||
| (coverage?.totalPaths || 0) - (coverage?.drawnPaths || 0), | ||
| 0, | ||
| ) | ||
|
|
||
| return ( | ||
| <div className={tooltipContainerClassName}> | ||
| <div className='flex items-center justify-between gap-6'> | ||
| <div className='flex min-w-0 items-center gap-1.5'> | ||
| <span className='truncate font-semibold text-gray-900 dark:text-gray-50'> | ||
| {t('project.journeys.otherPaths')} | ||
| </span> | ||
| <span className='shrink-0 text-gray-700 dark:text-gray-200'> | ||
| · {t('project.journeys.stepX', { x: node.step + 1 })} | ||
| </span> | ||
| </div> | ||
| <span className='shrink-0 font-mono font-semibold text-gray-900 tabular-nums dark:text-gray-50'> | ||
| {node.sessions.toLocaleString()} | ||
| </span> | ||
| </div> | ||
| <p className={`mt-1 ${tooltipLabelClassName}`}> | ||
| {t('project.journeys.hiddenPathsCount', { count: hiddenPaths })} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not show the global hidden-path count for each step.
hiddenPaths counts all omitted paths, including paths that never reached this node’s step. Later “Other paths” tooltips therefore overstate how many paths contribute to node.sessions. Use per-step path counts or remove the count from this tooltip.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/app/pages/Project/tabs/Journeys/JourneysView.tsx` around lines 703 - 725,
Remove the global hiddenPaths calculation and its hidden-path count from the
node.kind === 'other' tooltip in JourneysView. Keep the existing “Other paths”
label, step, and session count unchanged; do not display a count that is not
scoped to the current step.
Comment on lines
+1197
to
+1200
| labelPosition='inside' | ||
| labelOrientation='horizontal' | ||
| labelPadding={10} | ||
| labelTextColor={theme === 'dark' ? '#e2e8f0' : '#334155'} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use contrasting label colors for light-theme nodes.
The dark #334155 text is rendered inside saturated blue, purple, and similar nodes, making several labels difficult to read. Select label color by node kind/background luminance or adjust the palette.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/app/pages/Project/tabs/Journeys/JourneysView.tsx` around lines 1197 -
1200, Update the labelTextColor configuration in the journey node rendering to
choose a contrasting color for light-theme nodes based on each node’s kind or
background luminance, ensuring labels remain readable on saturated blue, purple,
and similar backgrounds while preserving the existing dark-theme behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
If applicable, please describe what changes were made in this pull request.
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
New Features
continuedPast), total path counts, and per-length histograms, plus richer node/link breakdowns.Bug Fixes