[WEB-4246] Analytics minor improvements#7194
Conversation
WalkthroughA new generic type for export configuration is introduced in the analytics types. Analytics table components are refactored to use column-level export metadata and delegate CSV export logic to a new shared utility function. Minor UI logic is updated for conditional labeling, type definitions are extended for future tab configuration, and translation keys for "number of {entity}" are added in multiple locales. Additional UI and chart components gain optional props and improved configurability. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TableComponent
participant exportCSV Utility
participant CSVGenerator
participant Downloader
User->>TableComponent: Clicks "Export CSV"
TableComponent->>exportCSV Utility: Calls exportCSV(rows, columns, workspaceSlug)
exportCSV Utility->>CSVGenerator: Generates CSV string from rows and column export metadata
CSVGenerator-->>exportCSV Utility: Returns CSV string
exportCSV Utility->>Downloader: Triggers CSV file download
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
web/core/components/analytics/work-items/workitems-insight-table.tsx (1)
188-202: Remove commented code.The large block of commented-out CSV export logic should be removed since it's been replaced by the centralized
exportCSVutility. Keeping dead code reduces maintainability and can cause confusion.- // const exportCSV = useCallback( - // (rows: Row<AnalyticsTableDataMap["work-items"]>[]) => { - // const rowData = rows.map((row) => { - // const headers = columns.map((col) => col.meta?.export?.key).filter((v) => v != undefined); - // const cells = headers.reduce((acc, header) => { - // const cell = columns.find((col) => col.meta?.export?.key === header)?.meta?.export?.value(row); - // return { ...acc, [header]: cell }; - // }, {}); - // return cells; - // }); - // const csv = generateCsv(csvConfig(workspaceSlug))(rowData); - // download(csvConfig(workspaceSlug))(csv); - // }, - // [columns, workspaceSlug] - // );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/types/src/analytics.d.ts(2 hunks)web/ce/components/analytics/tabs.ts(1 hunks)web/core/components/analytics/export.ts(1 hunks)web/core/components/analytics/work-items/created-vs-resolved.tsx(1 hunks)web/core/components/analytics/work-items/priority-chart.tsx(6 hunks)web/core/components/analytics/work-items/workitems-insight-table.tsx(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
web/core/components/analytics/work-items/created-vs-resolved.tsx (1)
packages/i18n/src/store/index.ts (1)
t(211-232)
web/core/components/analytics/export.ts (2)
web/core/store/router.store.ts (1)
workspaceSlug(69-71)web/core/components/analytics/config.ts (1)
csvConfig(3-9)
🪛 Biome (1.9.4)
web/core/components/analytics/export.ts
[error] 10-10: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (10)
web/ce/components/analytics/tabs.ts (1)
8-8: LGTM! Clean interface extension for future use.The optional
isExtendedproperty is a well-structured addition that prepares the tab configuration for future functionality without breaking existing code.packages/types/src/analytics.d.ts (2)
3-3: LGTM! Clean import addition.The
Rowimport from@tanstack/react-tableproperly supports the new export type definition.
63-67: LGTM! Well-designed generic export configuration type.The
ExportConfig<T>type is well-structured with:
- Strong typing for the value extraction function
- Flexible generic parameter for different data types
- Optional label for customizable export headers
This provides a solid foundation for the centralized export functionality.
web/core/components/analytics/work-items/created-vs-resolved.tsx (1)
107-107: LGTM! Improved contextual labeling.The conditional y-axis label properly adapts to show "epics" or "work_items" based on the
isEpicflag, improving user experience with context-appropriate labeling.web/core/components/analytics/work-items/priority-chart.tsx (2)
156-175: LGTM! Well-structured export metadata for columns.The export metadata is properly configured with appropriate keys, labels, and value extraction functions. The implementation correctly supports the centralized export functionality.
232-232: LGTM! Clean integration with centralized export function.The refactored CSV export properly delegates to the shared
exportCSVutility with all required parameters, improving code maintainability and consistency.web/core/components/analytics/work-items/workitems-insight-table.tsx (4)
1-2: LGTM: Import additions are appropriate.The new imports for
ColumnDef,Row,RowData,ExportConfig, and the centralizedexportCSVutility support the refactoring objectives effectively.Also applies to: 9-9, 19-19
24-28: LGTM: Module augmentation enhances type safety.The module augmentation properly extends the
ColumnMetainterface to include export configuration, providing type safety for the export metadata across all columns.
68-186: LGTM: Column definitions with export metadata are well-structured.The enhanced column definitions consistently implement export metadata with appropriate key mapping and value extraction functions. The null safety handling (using optional chaining and nullish coalescing) is properly implemented across all columns.
212-212: LGTM: Export function integration is correct.The updated
onExportprop properly delegates to the centralizedexportCSVutility with the correct parameters (rows, columns, workspaceSlug).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
web/core/components/analytics/insight-table/root.tsx (1)
32-32: Consider improving the empty state search placeholder UX.The current implementation shows "0 [headerText]" when there's no data, which might be confusing to users. Consider showing a more descriptive placeholder like "Search in [headerText]" or "No [headerText] available" for better UX.
- searchPlaceholder={`${data?.length || 0} ${headerText}`} + searchPlaceholder={data?.length ? `${data.length} ${headerText}` : `Search ${headerText}`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
web/core/components/analytics/config.ts(0 hunks)web/core/components/analytics/export.ts(1 hunks)web/core/components/analytics/insight-table/root.tsx(1 hunks)web/core/components/analytics/work-items/workitems-insight-table.tsx(4 hunks)
💤 Files with no reviewable changes (1)
- web/core/components/analytics/config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/core/components/analytics/export.ts
- web/core/components/analytics/work-items/workitems-insight-table.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
web/core/components/analytics/insight-table/root.tsx (2)
29-32: Good refactoring to ensure consistent rendering.The change to always render
DataTablewithdata || []fallback provides better consistency and aligns well with the centralized export logic refactoring. The optional chaining in the search placeholder is also a good defensive programming practice.
33-42:Details
❓ Verification inconclusive
Verify export button UX for empty data scenarios.
The export button is now always visible, even when there's no data to export. This could confuse users who click export on empty tables. Consider either:
- Conditionally disabling the button when data is empty
- Ensuring the export function handles empty data gracefully with appropriate user feedback
🏁 Script executed:
#!/bin/bash # Verify how the DataTable component handles empty arrays and if export functionality provides appropriate feedback for empty data # Check DataTable implementation for empty state handling ast-grep --pattern 'function DataTable($$$) { $$$ }' # Check if there are any empty state handlers or messages rg -A 10 -B 5 "empty.*state|no.*data|empty.*array" --type tsxLength of output: 148
Verify export button UX for empty data scenarios
I wasn’t able to locate any empty‐state handling in the
Tablecomponent or theonExportimplementation. Please manually verify that:
- The export button is disabled or hidden when
table.getFilteredRowModel().rowsis empty- Or that invoking export on an empty array shows clear, user-friendly feedback rather than producing an empty file or silent failure
…lytics-minor-improvements
… y-axis label to use new translation
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (4)
web/core/components/analytics/insight-table/data-table.tsx (2)
66-72: 🛠️ Refactor suggestion
table.getHeaderGroups()?.[0]?.headers?.[0]?.idis still a brittle way to pick the “searchable” columnGrabbing “the very first column of the very first header group” implicitly couples the search-bar logic to a particular table layout.
As soon as the consumer adds a row-selection column, a drag handle, or re-orders columns, the search bar will silently target the wrong column (or none at all).Consider passing an explicit
searchColumnId(or a predicate) via props and fall back to the first data column only when it’s not provided.
95-101:⚠️ Potential issue
getColumnis called with a possibly-undefined id — this breaks strict TypeScript and can throw at runtime
table.getColumn(undefined)violates the TanStack typings (expectsstring | number).
Wrap it with a guard instead of relying on?.after the call:- value={table.getColumn(table.getHeaderGroups()?.[0]?.headers?.[0]?.id)?.getFilterValue() as string} + const columnId = table.getHeaderGroups()?.[0]?.headers?.[0]?.id; + value={columnId ? (table.getColumn(columnId)?.getFilterValue() as string) : ""}This also gives you the
columnIdvariable you already need in the next two handlers, eliminating duplication.packages/propel/src/charts/line-chart/root.tsx (1)
120-127:⚠️ Potential issueTreat
0as a validdxvalue – switch||to nullish-coalescing
yAxis.dx || -16overrides an explicit0with-16because0is falsy.
If consumers legitimately passdx: 0to centre the label, the override will bite them.- dx: yAxis.dx || -16, + dx: yAxis.dx ?? -16,Use
??(or an explicit check) so onlynull/undefinedfall back.
Replicate the same change in other chart components for consistency.packages/propel/src/charts/scatter-chart/root.tsx (1)
104-113:⚠️ Potential issueSame
dxfalsy-value bug as in LineChartExact same issue –
0cannot be passed. Apply the??operator.- dx: yAxis.dx || -16, + dx: yAxis.dx ?? -16,
🧹 Nitpick comments (7)
web/core/components/analytics/insight-table/data-table.tsx (1)
96-97: Hard-coded placeholder defeats i18n & prop customisation
placeholder="Search"ignores both localisation (t(...)) and thesearchPlaceholderprop already passed in.
Reuse one of those instead:- placeholder="Search" + placeholder={searchPlaceholder}web/core/components/analytics/overview/project-insights.tsx (1)
34-46: Cache-key construction logic is duplicated across analytics views
Several analytics components build long template-string keys manually. Over time they tend to diverge (ordering, delimiter choices, missing fields) and hurt cache hit-rates. Consider extracting a small helper, e.g.buildRadarChartKey({ chart, workspace, duration, projects, cycle, module, peek }), so future refactors touch one place only.-`radar-chart-project-insights-${workspaceSlug}-${selectedDuration}-${selectedProjects}-${selectedCycle}-${selectedModule}-${isPeekView}` +buildRadarChartKey({ + chart: "project-insights", + workspace: workspaceSlug, + duration: selectedDuration, + projects: selectedProjects, + cycle: selectedCycle, + module: selectedModule, + peek: isPeekView, +})This keeps keys deterministic, testable, and easier to update.
packages/propel/src/charts/scatter-chart/root.tsx (1)
128-150: Avoid recreating the tooltip render function on every renderInline arrow functions inside JSX generate a new reference each render, forcing Recharts to re-mount the tooltip and causing unnecessary work. Memoise or extract the callback.
- content={({ active, label, payload }) => - customTooltipContent ? ( - customTooltipContent({ active, label, payload }) - ) : ( - <CustomTooltip - …props - /> - ) - } + content={tooltipRenderer}and above the return:
const tooltipRenderer = useCallback( ({ active, label, payload }) => customTooltipContent ? customTooltipContent({ active, label, payload }) : ( <CustomTooltip active={active} activeKey={activePoint} label={label} payload={payload} itemKeys={itemKeys} itemLabels={itemLabels} itemDotColors={itemDotColors} /> ), [customTooltipContent, activePoint, itemKeys, itemLabels, itemDotColors], );Small perf win, especially on large datasets.
web/core/components/analytics/trend-piece.tsx (4)
11-13: Prop additions are sound – please document defaults
trendIconVisibleandvariantgreatly improve flexibility; however, they are now part of the public API and should be reflected in component documentation / Storybook so downstream consumers understand their behaviour and defaults (true,"simple").
34-47: Dark-mode / contrast not handled for new variants
outlinedandtintedhard-codetext-green-500/text-red-500(and matching border / bg). These colours are optimised for light backgrounds but become AA-contrast failures on dark themes. Consider adding explicitdark:classes (e.g.dark:text-green-400) or pulling colours from a theme token to keep accessibility intact.
56-61: Class string could be pre-computed for perf / readability
cn()is called on every render. Given thatpercentage,variant, andsizeseldom change, memoising the computed class string or pushing it intovariants/sizeConfigreduces work in large lists. Not critical, but worth considering.
63-64: Guard icon rendering with fragment instead of logical-andThe
&&pattern occasionally trips TypeScript when the left operand isn’t strictly boolean. Wrapping the icon expression in a fragment avoids unintended coercion:{trendIconVisible && ( isPositive ? <TrendingUp className={config.icon} /> : <TrendingDown className={config.icon} /> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
packages/constants/src/module.ts(2 hunks)packages/i18n/src/locales/cs/translations.json(1 hunks)packages/i18n/src/locales/de/translations.json(1 hunks)packages/i18n/src/locales/en/translations.json(2 hunks)packages/i18n/src/locales/es/translations.json(1 hunks)packages/i18n/src/locales/fr/translations.json(1 hunks)packages/i18n/src/locales/id/translations.json(1 hunks)packages/i18n/src/locales/it/translations.json(1 hunks)packages/i18n/src/locales/ja/translations.json(1 hunks)packages/i18n/src/locales/ko/translations.json(1 hunks)packages/i18n/src/locales/pl/translations.json(1 hunks)packages/i18n/src/locales/pt-BR/translations.json(1 hunks)packages/i18n/src/locales/ro/translations.json(1 hunks)packages/i18n/src/locales/ru/translations.json(1 hunks)packages/i18n/src/locales/sk/translations.json(1 hunks)packages/i18n/src/locales/tr-TR/translations.json(1 hunks)packages/i18n/src/locales/ua/translations.json(1 hunks)packages/i18n/src/locales/vi-VN/translations.json(1 hunks)packages/i18n/src/locales/zh-CN/translations.json(1 hunks)packages/i18n/src/locales/zh-TW/translations.json(1 hunks)packages/propel/src/charts/line-chart/root.tsx(1 hunks)packages/propel/src/charts/scatter-chart/root.tsx(4 hunks)packages/types/src/analytics.d.ts(3 hunks)packages/types/src/charts/index.d.ts(10 hunks)packages/ui/src/icons/cycle/helper.tsx(1 hunks)packages/ui/src/icons/cycle/index.ts(1 hunks)web/core/components/analytics/empty-state.tsx(1 hunks)web/core/components/analytics/insight-table/data-table.tsx(1 hunks)web/core/components/analytics/overview/project-insights.tsx(1 hunks)web/core/components/analytics/trend-piece.tsx(2 hunks)web/core/components/analytics/work-items/priority-chart.tsx(7 hunks)
✅ Files skipped from review due to trivial changes (24)
- web/core/components/analytics/empty-state.tsx
- packages/i18n/src/locales/id/translations.json
- packages/i18n/src/locales/de/translations.json
- packages/ui/src/icons/cycle/helper.tsx
- packages/types/src/charts/index.d.ts
- packages/i18n/src/locales/it/translations.json
- packages/i18n/src/locales/ua/translations.json
- packages/i18n/src/locales/ru/translations.json
- packages/i18n/src/locales/pl/translations.json
- packages/ui/src/icons/cycle/index.ts
- packages/i18n/src/locales/cs/translations.json
- packages/i18n/src/locales/sk/translations.json
- packages/i18n/src/locales/en/translations.json
- packages/i18n/src/locales/fr/translations.json
- packages/i18n/src/locales/ko/translations.json
- packages/i18n/src/locales/ja/translations.json
- packages/i18n/src/locales/vi-VN/translations.json
- packages/constants/src/module.ts
- packages/i18n/src/locales/tr-TR/translations.json
- packages/i18n/src/locales/ro/translations.json
- packages/i18n/src/locales/pt-BR/translations.json
- packages/i18n/src/locales/zh-TW/translations.json
- packages/i18n/src/locales/es/translations.json
- packages/i18n/src/locales/zh-CN/translations.json
🚧 Files skipped from review as they are similar to previous changes (2)
- web/core/components/analytics/work-items/priority-chart.tsx
- packages/types/src/analytics.d.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
web/core/components/analytics/overview/project-insights.tsx (1)
web/core/store/router.store.ts (1)
workspaceSlug(69-71)
packages/propel/src/charts/scatter-chart/root.tsx (1)
packages/propel/src/charts/components/tooltip.tsx (1)
CustomTooltip(17-59)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
web/core/components/analytics/overview/project-insights.tsx (1)
35-35: More specific SWR cache-key – good improvement
Adding theproject-insightsqualifier reduces the risk of key collisions with other radar-chart requests and keeps the cache namespace clearer.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
web/core/components/analytics/trend-piece.tsx (1)
34-50: Neutral (0 %) case still treated as negative — same concern as prior review.Previous feedback about rendering a muted, icon-less neutral state was not addressed; 0 % will inherit
"atrisk"styling.
Consider introducing an early return or a dedicated"neutral"style to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/core/components/analytics/trend-piece.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript)
…ese locales in translations.json files
* chore: updated label for epics
* chore: improved export logic
* refactor: move csvConfig to export.ts and clean up export logic
* refactor: remove unused CSV export logic from WorkItemsInsightTable component
* refactor: streamline data handling in InsightTable component for improved rendering
* feat: add translation for "No. of {entity}" and update priority chart y-axis label to use new translation
* refactor: cleaned up some component and added utilitites
* feat: add "at_risk" translation to multiple languages in translations.json files
* refactor: update TrendPiece component to use new status variants for analytics
* fix: adjust TrendPiece component logic for on-track and off-track status
* refactor: use nullish coalescing operator for yAxis.dx in line and scatter charts
* feat: add "at_risk" translation to various languages in translations.json files
* feat: add "no_of" translation to various languages in translations.json files
* feat: update "at_risk" translation in Ukrainian, Vietnamese, and Chinese locales in translations.json files
3 participants
Description
Type of Change
Summary by CodeRabbit
New Features
Enhancements
Refactor