Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+83
to
+85
| onSelect={(currentValue) => { | ||
| setValue(currentValue); | ||
| setOpen(false); |
There was a problem hiding this comment.
clicking the currently selected option will set currentValue to empty string (cmdk toggle behavior), causing the button label to display undefined
Suggested change
| onSelect={(currentValue) => { | |
| setValue(currentValue); | |
| setOpen(false); | |
| onSelect={(currentValue) => { | |
| setValue(currentValue === value ? value : currentValue); | |
| setOpen(false); | |
| }} |
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.
Greptile Overview
Greptile Summary
This PR successfully adds a new analytics dashboard page with comprehensive filtering and visualization controls. The implementation includes a reusable date range picker component, risk view selector with visual indicators, and a filter system with real-time feedback.
Key additions:
/dashboard/analyticsroute with full dashboard UIDateRangePickercomponent with 30-day default range and dual-month calendar viewRiskViewSelectdropdown with visual status indicators for different risk viewsFiltersPopoverwith checkbox-based filtering and active filter displayMinor notes:
onClickhandler) - appears to be intentional scaffoldingConfidence Score: 4/5
analytics-overview.tsxis non-functional but this appears intentionalImportant Files Changed
File Analysis
AnalyticsOverview- clean implementation with no issuescomingSoonflag - clean and correct changeSequence Diagram
sequenceDiagram actor User participant Page as Analytics Page participant Overview as AnalyticsOverview participant RiskView as RiskViewSelect participant Filters as FiltersPopover participant DatePicker as DateRangePicker User->>Page: Navigate to /dashboard/analytics Page->>Overview: Render component Overview->>RiskView: Initialize (default: "risk-view") Overview->>Filters: Initialize (empty filters) Overview->>DatePicker: Initialize (last 30 days) User->>RiskView: Click to open dropdown RiskView->>RiskView: setOpen(true) User->>RiskView: Select view option RiskView->>RiskView: setValue(currentValue) RiskView->>RiskView: setOpen(false) User->>Filters: Click to open popover Filters->>Filters: setOpen(true) User->>Filters: Check/uncheck filter Filters->>Filters: handleFilterChange(filterId, checked) Filters->>Filters: Update selectedFilters state Filters->>Filters: Display active filters User->>DatePicker: Click to open calendar DatePicker->>DatePicker: setOpen(true) User->>DatePicker: Select date range DatePicker->>DatePicker: setDateRange(newRange) DatePicker->>DatePicker: Update button label User->>Overview: Click Export button Note over Overview: Export functionality not implementedGreptile Summary
This PR introduces the
/dashboard/analyticsroute and its full set of sub-components: anAnalyticsOverviewtoolbar, a revenue sparkline with dynamic date-range support, a Forecast vs Target chart, a Coverage Triage card, a Manager Action Queue, and a sortable Revenue Risk Ledger table. The navigation item is activated by removing thecomingSoonflag.Compared to the previous review, several issues have been fixed:
handleFilterTogglenow correctly uses a functional updater with a uniqueness guard, eliminating the stale-state duplicate-filter bug.buildRevenueChartDatanow properly accepts bothfromandto, so the chart window matches the selected range.comparatorLabelhas been changed from the hardcoded date string to the generic"vs previous period".Issues from previous reviews that remain unresolved in the current code:
RiskViewSelectstill passescurrentValuedirectly tosetValueon select — re-selecting the active option emits an empty string (cmdk toggle behaviour), clearing the button label.last:sm:pr-0 first:sm:pl-0variant ordering inLedgerStat(analytics-actions-risk-ledger.tsx:323) is still reversed; Tailwind JIT may not emit the correct CSS, leaving the border-adjacent columns with wrong padding at thesmbreakpoint.YAxisinanalytics-drivers-forecast-target.tsxstill declaresdomain={[0, "auto"]}withticks={[0, 50, 100, 150, 200]}; the auto ceiling (~115) means both the150and200ticks fall outside the domain and will be silently dropped by Recharts.analytics-drivers-coverage-triage.tsxand the Export button inanalytics-overview.tsxhave noonClickhandlers.id="date"on theDateRangePickertrigger will produce duplicate DOM IDs if the component is ever used more than once on a page.Confidence Score: 3/5
id="date"in DateRangePicker. None are data-corrupting or security-relevant, but they represent known UX/rendering bugs that have been explicitly pointed out and not yet resolved.analytics-drivers-forecast-target.tsx(YAxis ticks/domain mismatch) andanalytics-actions-risk-ledger.tsx(Tailwind variant ordering in LedgerStat).Important Files Changed
buildRevenueChartData(from, to), generic "vs previous period" comparator label). TheRiskViewSelectempty-string toggle edge case and the no-op Export button remain unresolved.last:sm:pr-0 first:sm:pl-0) on the LedgerStat wrapper that will produce incorrect padding at the sm breakpoint.domain={[0, "auto"]}(auto ceiling ≈ 115) butticks={[0, 50, 100, 150, 200]}— both the 150 and 200 tick values lie outside the computed domain and will be silently dropped by Recharts, leaving the Y-axis visually incomplete. This was flagged in a previous review and remains unresolved.onClickhandler (previously flagged). No other logic issues.id="date"on the trigger button is still present, which will cause duplicate DOM IDs if the component is used more than once on a page.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[User navigates to /dashboard/analytics] --> B[page.tsx] B --> C[AnalyticsOverview] B --> D[DriversForecastTarget] B --> E[DriversCoverageTriage] B --> F[ActionsManagerQueue] B --> G[ActionsRiskLedger] C --> C1[RiskViewSelect\ninternal state only] C --> C2[FiltersPopover\nselectedFilters state] C --> C3[DateRangePicker\ncontrolled via dateRange state] C --> C4[buildRevenueChartData\nfrom + to → random walk series] C --> C5[SummaryRow\nrevenue sparkline + risk metrics] C2 -->|onToggle| C2a[handleFilterToggle\nfunctional updater] C3 -->|onChange| C3a[handleDateRangeChange\nguards from+to both present] G --> G1[useReactTable\nSortingState: riskScore desc] G1 --> G2[ledgerColumns\n7 column defs]Last reviewed commit: 079e8d9