fix(frontend): clarify channel adoption statistics#1847
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds new localization keys for adoption/channel stats, refactors the stats overview UI into "latest snapshot" and "selected period" sections with new formatters and computed properties, and tightens the stats API timeframe defaults and bounds from 14/30 to 3/7 days respectively. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant Database
User->>Frontend: select period (days = 1|3|7)
Frontend->>Frontend: update days, compute selectedPeriodLabel
Frontend->>Backend: fetchStats(days)
Backend->>Backend: clamp days to [1,7], compute startDate/endDate
Backend->>Database: query stats between startDate and endDate
Database-->>Backend: return time-series & metaCounts
Backend-->>Frontend: return datasets, labels, currentVersion
Frontend->>Frontend: compute currentVersionDataset, periodSummary
Frontend-->>User: render latest snapshot & selected period cards, chart, help text
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/pages/app/[app].channel.[channel].statistics.vue (3)
667-725: Consider handling nullperiodSummarymore gracefully.When
periodSummaryisnull(e.g., no data for the current version in the selected period), the "Selected period" cards still render with0.0%values and-dates. This could confuse users into thinking there's actual zero adoption rather than missing data.Consider adding a conditional to show a "No period data available" message when
periodSummaryis null, similar to how the chart handlesstats.totals.total_devices === 0.💡 Suggested approach
+ <div v-if="!periodSummary" class="p-4 text-center text-slate-500 dark:text-slate-400"> + {{ t('no-data-available') }} + </div> + + <div v-else class="grid grid-cols-1 gap-4 sm:grid-cols-3"> - <div class="grid grid-cols-1 gap-4 sm:grid-cols-3"> <!-- ... cards ... --> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/app/`[app].channel.[channel].statistics.vue around lines 667 - 725, The cards always show zeros when periodSummary is null, which is misleading; update the template around selectedPeriodLabel to check periodSummary and, if null, render a single centered "No period data available" message (or similar) instead of the three metric cards; otherwise render the existing card group that uses periodSummary?.startShare, periodSummary?.changeShare, periodSummary?.peakShare and helpers formatPercent/formatSignedPoints and stats.currentVersion as before. Ensure the conditional uses the same truthiness check the chart uses (e.g., periodSummary === null or periodSummary?.startCount === undefined) so the UI consistently hides metrics when no period data exists.
268-269: Optional: Redundant length check.The check
if (normalizedShares.length === 0)at line 268 is unreachable becauselabels.length === 0already returnsnullat line 253, andnormalizedSharesis derived fromlabels.map(...).♻️ Remove redundant check
const normalizedCounts = labels.map((_, index) => { const rawValue = dataset.metaCounts?.[index] const numeric = typeof rawValue === 'number' ? rawValue : Number(rawValue) return Number.isFinite(numeric) ? Math.max(0, Math.round(numeric)) : 0 }) - if (normalizedShares.length === 0) - return null - const startShare = normalizedShares[0] ?? 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/app/`[app].channel.[channel].statistics.vue around lines 268 - 269, The guard `if (normalizedShares.length === 0) return null` is redundant because `labels.length === 0` already returns null earlier and `normalizedShares` is derived from `labels.map(...)`; remove that unreachable check (the `normalizedShares` length check) to simplify the control flow while keeping the existing early-return on `labels.length === 0` intact — search for `labels`, `normalizedShares`, and the `labels.length === 0` return to locate and remove the redundant `normalizedShares` check.
221-227: Minor:formatSignedPoints(0)will display "+0.0 pts".The condition
value > 0means exactly zero gets a+prefix, resulting in "+0.0 pts". This is likely unintentional from a UX perspective.💡 Suggested fix to show "0.0 pts" for zero
function formatSignedPoints(value: number) { if (!Number.isFinite(value)) - return `0.0 ${t('points-short')}` + return '-' - const sign = value > 0 ? '+' : '' + const sign = value > 0 ? '+' : value < 0 ? '' : '' return `${sign}${value.toFixed(1)} ${t('points-short')}` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/app/`[app].channel.[channel].statistics.vue around lines 221 - 227, formatSignedPoints currently can prefix zero with a plus; update the sign logic in function formatSignedPoints so zero shows no "+": compute sign as an explicit three-way check (if value === 0 -> '', else if value > 0 -> '+', else -> ''), then return `${sign}${value.toFixed(1)} ${t('points-short')}` so negatives keep their built-in "-" from toFixed and zero is "0.0 pts".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/pages/app/`[app].channel.[channel].statistics.vue:
- Around line 667-725: The cards always show zeros when periodSummary is null,
which is misleading; update the template around selectedPeriodLabel to check
periodSummary and, if null, render a single centered "No period data available"
message (or similar) instead of the three metric cards; otherwise render the
existing card group that uses periodSummary?.startShare,
periodSummary?.changeShare, periodSummary?.peakShare and helpers
formatPercent/formatSignedPoints and stats.currentVersion as before. Ensure the
conditional uses the same truthiness check the chart uses (e.g., periodSummary
=== null or periodSummary?.startCount === undefined) so the UI consistently
hides metrics when no period data exists.
- Around line 268-269: The guard `if (normalizedShares.length === 0) return
null` is redundant because `labels.length === 0` already returns null earlier
and `normalizedShares` is derived from `labels.map(...)`; remove that
unreachable check (the `normalizedShares` length check) to simplify the control
flow while keeping the existing early-return on `labels.length === 0` intact —
search for `labels`, `normalizedShares`, and the `labels.length === 0` return to
locate and remove the redundant `normalizedShares` check.
- Around line 221-227: formatSignedPoints currently can prefix zero with a plus;
update the sign logic in function formatSignedPoints so zero shows no "+":
compute sign as an explicit three-way check (if value === 0 -> '', else if value
> 0 -> '+', else -> ''), then return `${sign}${value.toFixed(1)}
${t('points-short')}` so negatives keep their built-in "-" from toFixed and zero
is "0.0 pts".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53cd33ba-fb72-400b-8287-c99e2b24ff88
📒 Files selected for processing (2)
messages/en.jsonsrc/pages/app/[app].channel.[channel].statistics.vue
|
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary (AI generated)
Motivation (AI generated)
Users were reading the adoption cards as if they all represented the same time scope. The page mixed a latest snapshot number with a chart over a selectable range, which made the values look inconsistent even when the underlying data was correct.
Business Impact (AI generated)
This reduces confusion on a customer-facing analytics page, which should improve trust in Capgo's update adoption reporting and lower support friction around rollout visibility.
Test Plan (AI generated)
Generated with AI
Summary by CodeRabbit
New Features
Documentation
Chores