Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
graphieros
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds alt-text generation and copy-to-clipboard for trend line charts by wiring an Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
♻️ Duplicate comments (2)
lunaria/files/fr-FR.json (1)
377-390: Duplicate of the FR wording fix already flagged in the source locale file.No additional action here beyond keeping this mirror in sync with
i18n/locales/fr-FR.json.lunaria/files/en-GB.json (1)
387-387: Duplicate of the English copy tweak already flagged ini18n/locales/en.json.Please keep this en-GB mirror aligned when applying that wording update.
🧹 Nitpick comments (2)
app/composables/useCharts.ts (1)
706-707: Consider a defensive cap for copied alt-text length.The generated text is currently unbounded; adding a hard cap helps meet the shareability target for services with character limits.
♻️ Suggested refinement
- return (isSinglePackage ? singlePackageText : compareText) + generalAnalysis + const text = (isSinglePackage ? singlePackageText : compareText) + generalAnalysis + return text.length > 2000 ? `${text.slice(0, 1999)}…` : textapp/utils/charts.ts (1)
471-474: PreferunknownoveranyinTrendLineDatasetextra fields.The current index signature removes type checks for all extra properties.
unknownkeeps flexibility without dropping safety.♻️ Proposed refinement
export type TrendLineDataset = { lines: VueUiXyDatasetLineItem[] - [key: string]: any + [key: string]: unknown } | nullAs per coding guidelines "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
app/components/Package/TrendsChart.vueapp/composables/useCharts.tsapp/utils/charts.tsi18n/locales/en.jsoni18n/locales/fr-FR.jsoni18n/schema.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.jsonlunaria/files/fr-FR.jsonpackage.jsontest/unit/app/utils/charts.spec.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/utils/charts.ts (3)
247-252: Prefer direct index access over an accumulator loop for the single-element case.When
n === 1, iterating the array with a mutable accumulator is roundabout. As per the coding guidelines, prefer type-safe index access.♻️ Suggested simplification
- if (n === 1) { - let onlyValue = 0 - for (const entry of indexedValues) { - onlyValue = entry.value - } - - return { - mean: onlyValue, + if (n === 1) { + const onlyValue = indexedValues[0]?.value ?? 0 + return { + mean: onlyValue,
471-474:[key: string]: anyindex signature undermines type safety forTrendLineDataset.Any property access on a
TrendLineDatasetwill resolve toany, bypassing TypeScript checks entirely. If the intent is to accommodate extra vue-data-ui properties, consider a narrower escape hatch.♻️ Suggested narrowing
export type TrendLineDataset = { lines: VueUiXyDatasetLineItem[] - [key: string]: any + // Additional vue-data-ui dataset fields are not consumed here } | nullIf other properties are genuinely required downstream, document the specific fields instead of using a blanket
anyindex.
514-516: Unnecessary double castas unknown as string.
ChartTimeGranularityis'daily' | 'weekly' | 'monthly' | 'yearly', which is directly assignable tostring. The intermediateas unknownis redundant.♻️ Suggested simplification
- granularityKeyByGranularity[config.granularity as unknown as string] ?? + granularityKeyByGranularity[config.granularity] ??
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/components/Package/TrendsChart.vueapp/utils/charts.tsi18n/locales/en.jsoni18n/locales/fr-FR.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.jsonlunaria/files/fr-FR.json
🚧 Files skipped from review as they are similar to previous changes (5)
- lunaria/files/en-GB.json
- i18n/locales/fr-FR.json
- i18n/locales/en.json
- app/components/Package/TrendsChart.vue
- lunaria/files/fr-FR.json
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/utils/charts.ts (1)
533-534:⚠️ Potential issue | 🟡 Minor
?? 0fallback injects a bare number into a string slot, rendering"0"in alt text.
formattedDatasetValuesisArray<string[]>, so when the index is out of range the fallback should be a displayable string, not0.🐛 Proposed fix
- start_value: config.formattedDatasetValues[i]?.[0] ?? 0, - end_value: config.formattedDatasetValues[i]?.at(-1) ?? 0, + start_value: config.formattedDatasetValues[i]?.[0] ?? '—', + end_value: config.formattedDatasetValues[i]?.at(-1) ?? '—',
🧹 Nitpick comments (2)
app/utils/charts.ts (2)
496-501:hasEstimationadded to each analysis item is never read.The per-item
hasEstimation: config.hasEstimationspread into everyanalysisentry is unused inside the.map()callback; the estimation notice is built fromconfig.hasEstimationdirectly at line 547. This is dead data on every analysis object.♻️ Suggested removal
const analysis = dataset.lines.map(({ name, series }) => ({ name, ...computeLineChartAnalysis(series), dates: config.formattedDates, - hasEstimation: config.hasEstimation, }))
503-511: Tighten the granularity map type toRecord<ChartTimeGranularity, string>.
config.granularityis alreadyChartTimeGranularity('daily' | 'weekly' | 'monthly' | 'yearly'). UsingRecord<ChartTimeGranularity, string>makes the map exhaustive, lets the compiler catch missing/misspelled keys, and renders the?? 'package.trends.granularity_weekly'fallback unnecessary.♻️ Suggested change
- const granularityKeyByGranularity: Record<string, string> = { + const granularityKeyByGranularity: Record<ChartTimeGranularity, string> = { daily: 'package.trends.granularity_daily', weekly: 'package.trends.granularity_weekly', monthly: 'package.trends.granularity_monthly', yearly: 'package.trends.granularity_yearly', } - const granularityKey = - granularityKeyByGranularity[config.granularity] ?? 'package.trends.granularity_weekly' + const granularityKey = granularityKeyByGranularity[config.granularity]
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/utils/charts.tsi18n/locales/en.jsoni18n/locales/fr-FR.jsoni18n/schema.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.jsonlunaria/files/fr-FR.json
🚧 Files skipped from review as they are similar to previous changes (4)
- i18n/schema.json
- i18n/locales/en.json
- lunaria/files/en-GB.json
- lunaria/files/en-US.json
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/utils/charts.ts (1)
172-407: Extract smaller focused helpers from these long functions to reduce responsibilities.
computeLineChartAnalysis(236 lines) andcreateAltTextForTrendLineChart(80 lines) both exceed the 50-line guideline. Consider splitting them into focused units:
- computeLineChartAnalysis: Extract statistical calculations (mean, standard deviation), winsorization logic, regression computation, and interpretation rules into separate helpers.
- createAltTextForTrendLineChart: Extract data transformation, configuration lookup, and text assembly into separate utilities.
This improves maintainability, testability, and readability.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/utils/charts.tsi18n/locales/en.jsoni18n/locales/fr-FR.jsoni18n/schema.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.jsonlunaria/files/fr-FR.json
🚧 Files skipped from review as they are similar to previous changes (2)
- lunaria/files/en-GB.json
- lunaria/files/en-US.json
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/utils/charts.tstest/unit/app/utils/charts.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/utils/charts.ts
| it('upgrades trend from none to weak when relativeSlope is high enough', () => { | ||
| // Many points, huge growth + noise to keep linearity low but direction strong. | ||
| const values: Array<number | null> = [] | ||
| for (let i = 0; i < 25; i += 1) { | ||
| values.push(i * 1000 + (i % 2 === 0 ? 0 : 8000)) | ||
| } | ||
|
|
||
| const result = computeLineChartAnalysis(values) | ||
|
|
||
| expect(result.slope).toBeGreaterThan(0) | ||
| expect(result.rSquared).not.toBeNull() | ||
| expect(result.interpretation.trend === 'weak' || result.interpretation.trend === 'strong').toBe( | ||
| true, | ||
| ) | ||
| }) |
There was a problem hiding this comment.
The “none → weak” case is under-constrained and can miss regressions.
This test currently allows weak or strong without proving the baseline was none, so it does not fully validate what the title claims.
Suggested tightening
it('upgrades trend from none to weak when relativeSlope is high enough', () => {
@@
const result = computeLineChartAnalysis(values)
+ const baseTrend = computeBaseTrend(result.rSquared)
+ expect(baseTrend).toBe('none')
expect(result.slope).toBeGreaterThan(0)
expect(result.rSquared).not.toBeNull()
- expect(result.interpretation.trend === 'weak' || result.interpretation.trend === 'strong').toBe(
- true,
- )
+ expect(result.interpretation.trend).toBe('weak')
})
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
2 participants
Resolves #1264
The feature is only applied on the trends charts (modal & compare page) and works for single or multiple series.
We can iterate on other charts (versions).
A new 'Copy alt text' button is added in the chart's contextual menu:
Enregistrement.de.l.ecran.2026-02-25.a.13.11.03.mov