Feature/chart days filter #206
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA time-range filter capability has been added to the user subscription update chart. The Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds a time-range filter to the Subscription Client Distribution chart, allowing administrators to view client statistics filtered by specific time periods (24 hours, 7 days, 30 days) or all-time data.
Changes:
- Added a
daysparameter to the subscription chart API endpoint and supporting infrastructure - Implemented a time-range filter UI component in the dashboard with proper TypeScript type safety
- Added comprehensive translations for the new filter options across 4 languages
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| dashboard/src/service/api/index.ts | Added optional days parameter to GetUsersSubUpdateChartParams type |
| dashboard/src/components/charts/user-sub-update-pie-chart.tsx | Added time-range filter UI with Select component, resolved TypeScript implicit any errors with explicit type annotations, integrated days filter into chart data fetching |
| dashboard/public/statics/locales/en.json | Added English translations for time-range filter labels and options |
| dashboard/public/statics/locales/ru.json | Added Russian translations for time-range filter labels and options |
| dashboard/public/statics/locales/zh.json | Added Chinese translations for time-range filter labels and options |
| dashboard/public/statics/locales/fa.json | Added Farsi translations for time-range filter labels and options |
| app/routers/user.py | Added days query parameter to get_users_sub_update_chart endpoint |
| app/operation/user.py | Updated get_users_sub_update_chart to accept and pass days parameter to CRUD layer |
| app/db/crud/user.py | Implemented time-based filtering in get_users_subscription_agent_counts using days parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🤖 Fix all issues with AI agents
In @app/db/crud/user.py:
- Around line 880-891: The function get_users_subscription_agent_counts
currently uses a truthiness check on days (if days:) which treats days=0 as
falsy and skips the time filter; change that conditional to an explicit None
check (if days is not None:) so callers passing days=0 apply the intended
filter, and leave the rest of the logic (calculating since and adding the where
clause on UserSubscriptionUpdate.created_at) unchanged.
In @dashboard/src/service/api/index.ts:
- Around line 86-90: GetUsersSubUpdateChartParams currently allows null (e.g.,
days?: number | null) which causes orvalFetcher to send "null" in the query; fix
by either removing nullability in the OpenAPI schema/type (change days?: number
| null to days?: number) so generated types no longer permit null, or add a
null-filter step in orvalFetcher before calling ofetch that iterates request
params (including username, admin_id, days) and deletes any key whose value is
null or undefined so queries never include "null" strings.
🧹 Nitpick comments (2)
app/routers/user.py (1)
134-142: Consider adding validation for thedaysparameter.The
daysparameter accepts any integer, including negative values, which would result in a future timestamp causing no records to be returned. Consider adding validation constraints using FastAPI'sQuerywithge=0(greater than or equal to 0).Proposed fix
- days: int | None = Query(None), + days: int | None = Query(None, ge=0, description="Filter by number of days (0 or greater)"),dashboard/src/components/charts/user-sub-update-pie-chart.tsx (1)
21-26: Type definition has redundant fields.
SegmentWithColorextendsUserSubscriptionUpdateChartSegmentwhich already containscountandpercentagefields. Re-declaring them is redundant.Proposed fix
type SegmentWithColor = UserSubscriptionUpdateChartSegment & { key: string color: string - count: number - percentage: number }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/db/crud/user.pyapp/operation/user.pyapp/routers/user.pydashboard/public/statics/locales/en.jsondashboard/public/statics/locales/fa.jsondashboard/public/statics/locales/ru.jsondashboard/public/statics/locales/zh.jsondashboard/src/components/charts/user-sub-update-pie-chart.tsxdashboard/src/service/api/index.ts
🧰 Additional context used
🧬 Code graph analysis (4)
app/routers/user.py (1)
app/operation/user.py (1)
get_users_sub_update_chart(695-717)
app/db/crud/user.py (1)
app/db/models.py (1)
UserSubscriptionUpdate(285-292)
app/operation/user.py (3)
dashboard/src/service/api/index.ts (1)
UserSubscriptionUpdateChart(544-547)app/models/user.py (1)
UserSubscriptionUpdateChart(155-157)app/db/crud/user.py (1)
get_users_subscription_agent_counts(880-903)
dashboard/src/components/charts/user-sub-update-pie-chart.tsx (2)
dashboard/src/service/api/index.ts (2)
UserSubscriptionUpdateChartSegment(538-542)AdminDetails(2052-2068)dashboard/src/components/ui/chart.tsx (1)
ChartConfig(9-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CodeQL analysis (python)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (javascript-typescript)
🔇 Additional comments (11)
dashboard/public/statics/locales/en.json (1)
1275-1280: LGTM!The new localization keys for the time-range filter are clear, consistent with existing naming conventions, and provide appropriate English translations for the subscription client distribution chart feature.
dashboard/public/statics/locales/zh.json (1)
1225-1230: LGTM!The Chinese translations are accurate and idiomatic. The keys are consistent with the additions in other locale files, properly supporting the new time-range filter feature.
dashboard/public/statics/locales/fa.json (1)
1120-1125: LGTM!The Persian translations are accurate and correctly use Persian numerals (۲۴, ۷, ۳۰), which is consistent with the localization conventions used elsewhere in this file. The keys properly support the new time-range filter feature.
dashboard/public/statics/locales/ru.json (1)
29-34: LGTM!The Russian translations for the new days filter feature are grammatically correct and consistent with the existing locale structure. The translation for
daysFilter1as "За последние 24 часа" (Last 24 hours) is a reasonable choice that conveys the same meaning as "1 day" to users.app/operation/user.py (1)
695-717: LGTM. Thedaysparameter is correctly threaded through both code paths.Parameter is properly propagated to
get_users_subscription_agent_countsin both branches—when username is specified and when using admin_id. The change is backward compatible withNoneas the default.Minor note: The CRUD layer uses
if days:which treatsdays=0as falsy (no filter). While this edge case is unlikely in practice, the router currently lacks validation to restrict non-positive values if you want to make the API contract explicit.app/db/crud/user.py (1)
892-903: LGTM!The existing filtering logic for
user_idandadmin_idremains correct, and thedaysparameter integrates cleanly without affecting the existing behavior.dashboard/src/components/charts/user-sub-update-pie-chart.tsx (5)
147-164: LGTM!The params memo correctly handles the new
daysfilter, parsing and validating the value before including it in the API payload. The dependency array is properly updated.
172-203: LGTM!The segments memo properly maps API data to typed
SegmentWithColorobjects with defensive handling for potentially undefined values. The explicit type annotations improve code clarity.
246-260: LGTM!The days filter UI is well-implemented with proper internationalization support and responsive layout. The filter options (1, 7, 30 days, and all time) provide reasonable time-range choices for users.
261-278: LGTM!The admin filter is properly refactored with explicit type annotations and integrates well with the new layout alongside the days filter.
205-237: LGTM!The additional type annotations throughout the component improve type safety and code readability without changing functionality.
1 participant
Added a time-range filter (
days) to the Subscription Client Distribution chart. This allows admins to filter client statistics by 7, 30 days, or view all-time data.Changes:
get_users_subscription_agent_countsCRUD and user stats operations to support the optionaldaysparameter.Selectcomponent touser-sub-update-pie-chart.tsxfor period selection.anyerrors in the chart component by using proper interfaces from the API service.daysparameter.Summary by CodeRabbit
Release Notes
New Features
Internationalization
✏️ Tip: You can customize this high-level summary in your review settings.