chore(admin-ui): changes required in Admin UI #2522
Conversation
syntrydy
added
comp-admin-ui
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughMultiple UI styling tweaks and spacing adjustments; Health now filters out "not present" services and no longer computes an aggregated overall status; Dashboard and DateRange converted to prop-driven, typed components with new dashboard hooks, types, constants, and RTK Query–based data wiring. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant DashboardPage
participant DateRange
participant Hooks as "useDashboard* hooks"
participant API as "RTK Query / API"
participant DashboardChart
Note over User,DashboardPage: Load dashboard or change date range
User->>DashboardPage: open page / change dates
DashboardPage->>Hooks: call useDashboardLicense/Clients/LockStats/Mau/Health
Hooks->>API: issue RTK Query requests (with cache/select)
API-->>Hooks: return query results
Hooks-->>DashboardPage: provide clients, license, lockStats, mauData, services
DashboardPage->>DateRange: pass startDate,endDate,onChange callbacks
DateRange-->>DashboardPage: emit onStartDateChange/onEndDateChange
DashboardPage->>DashboardChart: pass statData,startMonth,endMonth
DashboardChart->>DashboardChart: compute augmentedData (useMemo)
DashboardChart-->>User: render chart
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
admin-ui/plugins/admin/components/Audit/AuditListPage.tsx(1 hunks)admin-ui/plugins/admin/components/CustomScripts/CustomScriptListPage.tsx(1 hunks)admin-ui/plugins/admin/components/Health/HealthPage.tsx(2 hunks)admin-ui/plugins/admin/components/Health/hooks/useHealthStatus.ts(1 hunks)admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx(1 hunks)admin-ui/plugins/auth-server/components/Configuration/Keys/Jwks/JwkItem.tsx(1 hunks)admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx(0 hunks)admin-ui/plugins/auth-server/components/Sessions/SessionListPage.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- admin-ui/plugins/auth-server/components/Scopes/ScopeListPage.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
admin-ui/plugins/auth-server/components/Sessions/SessionListPage.tsx (1)
admin-ui/app/customColors.ts (1)
customColors(4-25)
admin-ui/plugins/admin/components/Health/HealthPage.tsx (2)
admin-ui/plugins/admin/components/Health/hooks/useHealthStatus.ts (1)
useHealthStatus(82-112)admin-ui/plugins/admin/components/Health/hooks/index.ts (1)
useHealthStatus(1-1)
admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx (1)
admin-ui/app/customColors.ts (1)
customColors(4-25)
admin-ui/plugins/admin/components/CustomScripts/CustomScriptListPage.tsx (1)
admin-ui/app/customColors.ts (1)
customColors(4-25)
🔇 Additional comments (5)
admin-ui/plugins/auth-server/components/Configuration/Keys/Jwks/JwkItem.tsx (1)
14-15: Verify the margin spacing value aligns with design requirements.The wrapper div adds
5pxbottom margin between key items. Based on the AI summary, this replaced a class that was likelymb-12(typically ~48px in most frameworks). The significant reduction from ~48px to 5px may result in insufficient visual separation between accordion items.Please confirm that
5pxis the intended spacing value per the design specification for objective #4 ("Add spacing between each key div in the Keys view").admin-ui/plugins/auth-server/components/Sessions/SessionListPage.tsx (1)
209-209: LGTM! Delete icon color updated for visual consistency.The delete icon color has been updated to
darkGray, aligning with the PR objective to make delete icons less prominent and match the background color scheme. This change is consistent with similar updates across other admin UI components.admin-ui/plugins/admin/components/Webhook/WebhookListPage.tsx (1)
79-81: LGTM! Delete icon styling updated consistently.The DeleteIcon component now applies
darkGraycolor by default while preserving any additional styles passed via props. This aligns with the PR objective to make delete icons less visually prominent.admin-ui/plugins/admin/components/CustomScripts/CustomScriptListPage.tsx (1)
338-341: LGTM! Delete icon color standardized.The delete action icon color has been updated to
darkGray, maintaining visual consistency with other admin UI components per the PR objectives.admin-ui/plugins/admin/components/Health/hooks/useHealthStatus.ts (1)
30-39: Verify whether "down" services should also be filtered.The filter now excludes services with status "not present" (case-insensitive), but the PR objective states: "Do not display services that are down or not present on the Dashboard or Health page." Currently, only "not present" services are filtered out, while "down" services remain visible.
Please clarify whether services with status "down" should also be filtered out. If so, the filter condition should be expanded:
.filter(([, status]) => { const statusStr = String(status).toLowerCase() return statusStr !== 'not present' && statusStr !== 'down' })Otherwise, consider updating the PR objective description to reflect that only "not present" services are being filtered.
syntrydy
changed the title
duttarnab
left a comment
duttarnab
left a comment
There was a problem hiding this comment.
- This point not addressed ---
The services which are down or not present should not be shown on Dashboard or Health page.
reference.
#service_admin_ui > System Status -- Down -> Not Configured @ 💬
There was a problem hiding this comment.
Actionable comments posted: 14
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
admin-ui/app/routes/Dashboards/Chart/DashboardChart.tsx(2 hunks)admin-ui/app/routes/Dashboards/Chart/TooltipDesign.tsx(1 hunks)admin-ui/app/routes/Dashboards/DashboardPage.tsx(8 hunks)admin-ui/app/routes/Dashboards/DateRange/index.tsx(3 hunks)admin-ui/app/routes/Dashboards/Reports/ReportCard.tsx(1 hunks)admin-ui/app/routes/Dashboards/Reports/ReportPiChartItem.tsx(2 hunks)admin-ui/app/routes/Dashboards/Reports/Reports.tsx(1 hunks)admin-ui/app/routes/Dashboards/constants.ts(1 hunks)admin-ui/app/routes/Dashboards/hooks/index.ts(1 hunks)admin-ui/app/routes/Dashboards/hooks/useDashboardClients.ts(1 hunks)admin-ui/app/routes/Dashboards/hooks/useDashboardLicense.ts(1 hunks)admin-ui/app/routes/Dashboards/hooks/useDashboardLockStats.ts(1 hunks)admin-ui/app/routes/Dashboards/types/DashboardTypes.ts(1 hunks)admin-ui/app/routes/Dashboards/types/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
admin-ui/app/routes/Dashboards/hooks/useDashboardClients.ts (2)
admin-ui/app/routes/Dashboards/hooks/index.ts (1)
useDashboardClients(2-2)admin-ui/app/routes/Dashboards/constants.ts (1)
DASHBOARD_CACHE_CONFIG(1-4)
admin-ui/app/routes/Dashboards/hooks/useDashboardLockStats.ts (3)
admin-ui/app/routes/Dashboards/types/DashboardTypes.ts (1)
LockStatEntry(6-10)admin-ui/app/routes/Dashboards/hooks/index.ts (1)
useDashboardLockStats(3-3)admin-ui/app/routes/Dashboards/constants.ts (1)
DASHBOARD_CACHE_CONFIG(1-4)
admin-ui/app/routes/Dashboards/Reports/ReportCard.tsx (1)
admin-ui/app/routes/Dashboards/types/DashboardTypes.ts (1)
ReportCardProps(17-22)
admin-ui/app/routes/Dashboards/Chart/TooltipDesign.tsx (1)
admin-ui/app/routes/Dashboards/types/DashboardTypes.ts (2)
TooltipPayloadItem(61-67)TooltipDesignProps(69-72)
admin-ui/app/routes/Dashboards/DateRange/index.tsx (1)
admin-ui/app/routes/Dashboards/types/DashboardTypes.ts (1)
DateRangeProps(48-53)
admin-ui/app/routes/Dashboards/Reports/ReportPiChartItem.tsx (2)
admin-ui/app/routes/Dashboards/types/DashboardTypes.ts (1)
ReportPiChartItemProps(24-26)admin-ui/app/customColors.ts (1)
customColors(4-25)
admin-ui/app/routes/Dashboards/Reports/Reports.tsx (3)
admin-ui/app/routes/Dashboards/constants.ts (1)
REPORTS_CACHE_CONFIG(6-9)admin-ui/plugins/auth-server/components/Clients/ClientBasicPanel.js (1)
isLoading(56-56)admin-ui/app/routes/Dashboards/types/DashboardTypes.ts (1)
ReportCardData(12-15)
🔇 Additional comments (25)
admin-ui/app/routes/Dashboards/Reports/ReportCard.tsx (1)
5-7: LGTM! Clean typing with shared types.The component now uses
ReportCardPropsfrom the shared types module, ensuring type consistency across the dashboard.admin-ui/app/routes/Dashboards/types/index.ts (1)
1-1: LGTM! Standard barrel export pattern.Establishes a clean public API surface for dashboard types.
admin-ui/app/routes/Dashboards/hooks/index.ts (1)
1-3: LGTM! Establishes clean hook API.The barrel export provides a single entry point for dashboard hooks.
admin-ui/app/routes/Dashboards/hooks/useDashboardClients.ts (1)
7-26: LGTM! Well-structured data fetching hook.The hook correctly:
- Gates fetching on auth token presence
- Applies appropriate cache configuration from
DASHBOARD_CACHE_CONFIG- Uses
useMemoto prevent unnecessary re-renders when derivingclients- Provides safe defaults for
clients(empty array) andtotalCount(0)admin-ui/app/routes/Dashboards/hooks/useDashboardLicense.ts (2)
19-30: LGTM! Clean license data fetching with transformation.The hook correctly:
- Gates fetching on auth token presence
- Applies cache configuration
- Uses
selectto transform data inline during fetch
7-17: No issues found—consuming code safely handles empty strings.The review comment requests verification of whether empty string defaults break consuming code. Analysis of all usages confirms the transformation is safe:
- LicenseDetailsPage.tsx (lines 81–83): Uses falsy checks (
!item.customerFirstName) which work identically for bothundefinedand empty strings.- DashboardPage.tsx (line 204): Explicitly uses OR operators (
field || '') to handle empty or falsy values.- Redux slice (licenseDetailsSlice.ts, lines 48–59): Already applies the same transformation, making this change consistent with existing behavior.
No consuming code relies on explicit
undefinedchecks (e.g.,field !== undefined), so the API contract change poses no risk of breakage.admin-ui/app/routes/Dashboards/Chart/TooltipDesign.tsx (3)
15-15: Good use of default parameters.Defaulting
payloadto an empty array in the function signature simplifies the logic.
17-21: LGTM! Explicit typing for translation map.The
Record<string, string>type forobjValuesclearly documents the structure.
25-25: Better conditional check.Using
payload.length > 0is more explicit and readable than relying on truthy coercion ofpayload.length.admin-ui/app/routes/Dashboards/hooks/useDashboardLockStats.ts (3)
8-18: LGTM! Safe transformation with proper defaults.The
transformLockStatsfunction correctly handles undefined/non-array inputs and maps to the expected type.
24-36: LGTM! Well-configured query with appropriate options.The hook correctly:
- Gates on both auth token and optional enabled flag
- Applies cache configuration
- Disables retries (appropriate for dashboard stats)
- Uses select to transform data inline
38-61: Verify intended behavior: "latest" vs aggregated stats.The
latestStatscomputation sums all entries when multiple exist (lines 54-60), but the name "latest" suggests the most recent entry. For time-series data, "latest" typically means the most recent data point, not an aggregate sum.If aggregation across all months is intended for a dashboard summary, consider renaming to
aggregatedStatsortotalStatsfor clarity. If the latest entry is needed, use the last element instead:- return { - monthly_active_users: data.reduce((sum, entry) => sum + (entry.monthly_active_users ?? 0), 0), - monthly_active_clients: data.reduce( - (sum, entry) => sum + (entry.monthly_active_clients ?? 0), - 0, - ), - } + // Return the most recent entry (assuming data is sorted chronologically) + const latest = data[data.length - 1] + return { + monthly_active_users: latest.monthly_active_users ?? 0, + monthly_active_clients: latest.monthly_active_clients ?? 0, + }Please verify with the team whether the current summation behavior is correct, or if it should return only the latest entry.
admin-ui/app/routes/Dashboards/Chart/DashboardChart.tsx (1)
55-68: LGTM!The BarChart rendering with
augmentedDataand the responsive container setup looks correct.admin-ui/app/routes/Dashboards/Reports/Reports.tsx (1)
25-34: LGTM - Query options with proper caching.The memoized
queryOptionswith token-based enablement and cache configuration aligns well with theREPORTS_CACHE_CONFIGconstants.admin-ui/app/routes/Dashboards/constants.ts (1)
1-9: LGTM - Cache configurations look reasonable.The stale and GC times are appropriately set for dashboard data freshness requirements.
admin-ui/app/routes/Dashboards/Reports/ReportPiChartItem.tsx (1)
16-64: LGTM!The TypeScript typing improvements enhance type safety. The use of
(_, index)in the map is appropriate since only the index is needed for color assignment.admin-ui/app/routes/Dashboards/types/DashboardTypes.ts (1)
1-4: LGTM - Well-structured type definitions.Good use of
PickforDashboardClientandDashboardAttributeto create minimal projections. The re-export pattern forLicenseResponse,MauStatEntry, andMauDateRangecentralizes type access nicely.Also applies to: 74-76
admin-ui/app/routes/Dashboards/DashboardPage.tsx (8)
5-6: LGTM!Clean import organization with proper type imports and hook dependencies.
Also applies to: 34-37
51-52: LGTM!Sensible default date range (3 months) for dashboard statistics.
91-126: LGTM!Good use of conditional data fetching to avoid unnecessary API calls when services are unavailable or permissions are lacking. The
enabledpattern properly gates network requests.
240-244: Verify filtering behavior aligns with PR objectives.The code filters out services with
'unknown'status (not present), but the PR objective states: "Do not show services that are down or not present." Should services withstatus === 'down'also be filtered out?return allServices.filter((serviceConfig) => { const status = getServiceStatus(serviceConfig.key) - return status !== 'unknown' + return status === 'up' || status === 'degraded' // Show only running/degraded services })
246-271: LGTM!Status helper functions are well-structured with clear responsibility separation.
273-320: LGTM!StatusCard is well-memoized and renders service status cards correctly based on the filtered
statusDetails.
344-374: LGTM!Loading state aggregation correctly includes all relevant loading flags. Date handlers are properly memoized.
482-513: LGTM!DateRange and DashboardChart components receive the correct props consistently in both mobile and desktop layouts.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin-ui/app/routes/Dashboards/Chart/TooltipDesign.tsx (1)
5-21: Handle unknowndataKeyvalues more defensivelyIf a new
dataKeyever appears that isn’t listed inobjValues,objValues[item.dataKey]will beundefinedand render asundefined : <value>. Consider a safe fallback such as the raw key or skipping unknown entries:- {objValues[item.dataKey]} : {item.payload[item.dataKey]} + {(objValues[item.dataKey] ?? item.dataKey)} : {item.payload[item.dataKey]}This keeps the tooltip readable even when keys drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
admin-ui/app/routes/Dashboards/Chart/DashboardChart.tsx(2 hunks)admin-ui/app/routes/Dashboards/Chart/TooltipDesign.tsx(1 hunks)admin-ui/app/routes/Dashboards/DashboardPage.tsx(8 hunks)admin-ui/app/routes/Dashboards/Reports/ReportPiChartItem.tsx(2 hunks)admin-ui/app/routes/Dashboards/Reports/Reports.tsx(1 hunks)admin-ui/app/routes/Dashboards/constants.ts(1 hunks)admin-ui/app/routes/Dashboards/types/DashboardTypes.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
admin-ui/app/routes/Dashboards/Reports/Reports.tsx (2)
admin-ui/app/routes/Dashboards/constants.ts (1)
REPORTS_CACHE_CONFIG(6-9)admin-ui/app/routes/Dashboards/types/DashboardTypes.ts (1)
ReportCardData(13-16)
admin-ui/app/routes/Dashboards/DashboardPage.tsx (6)
admin-ui/app/routes/Dashboards/hooks/useDashboardLicense.ts (1)
useDashboardLicense(19-30)admin-ui/app/routes/Dashboards/hooks/useDashboardClients.ts (1)
useDashboardClients(7-26)admin-ui/app/routes/Dashboards/hooks/useDashboardLockStats.ts (1)
useDashboardLockStats(24-68)admin-ui/app/routes/Dashboards/types/DashboardTypes.ts (1)
MauDateRange(5-5)admin-ui/app/utils/Util.ts (1)
formatDate(53-64)admin-ui/app/routes/Dashboards/DateRange/index.tsx (1)
DateRange(16-72)
admin-ui/app/routes/Dashboards/Chart/TooltipDesign.tsx (1)
admin-ui/app/routes/Dashboards/types/DashboardTypes.ts (1)
TooltipDesignProps(67-70)
admin-ui/app/routes/Dashboards/Chart/DashboardChart.tsx (2)
admin-ui/app/routes/Dashboards/types/DashboardTypes.ts (2)
DashboardChartProps(53-57)MauStatEntry(5-5)admin-ui/plugins/admin/helper/utils.js (1)
current(82-82)
admin-ui/app/routes/Dashboards/Reports/ReportPiChartItem.tsx (2)
admin-ui/app/routes/Dashboards/types/DashboardTypes.ts (2)
ReportPiChartItemProps(25-27)PieChartLabelProps(29-36)admin-ui/app/customColors.ts (1)
customColors(4-25)
🔇 Additional comments (6)
admin-ui/app/routes/Dashboards/Reports/ReportPiChartItem.tsx (1)
5-52: Typed props and label function look consistentUsing
ReportPiChartItemPropsfordataandPieChartLabelPropsforrenderCustomizedLabelaligns this component with the shared dashboard types and keeps the Recharts integration type‑safe. No issues spotted here.admin-ui/app/routes/Dashboards/Reports/Reports.tsx (1)
57-111: Nice reuse of memoized aggregates inReportCardpropsUsing
attributeData,clientData,scopeData, andscriptDataas the single source of truth and wiringupValue/downValuefrom those arrays avoids the duplicated.filter()work that was previously flagged. This keeps the render path cheap and the value semantics clear.admin-ui/app/routes/Dashboards/constants.ts (1)
1-37: Status normalization helpers look consistentLowercasing the input in
getStatusDisplay/getStatusColorand keeping all map keys lowercase gives you predictable lookups across variants likeRUNNING,Online, orNot present. Combined with the cache configs, this module is in good shape and ready for reuse across dashboard and health views.admin-ui/app/routes/Dashboards/types/DashboardTypes.ts (1)
5-74: Good centralization of dashboard‑specific typesPulling
ReportCard*,PieChartLabelProps,DashboardChartProps, tooltip types, and the dashboard client/attribute aliases into this shared module cleans up the components and removes the earlier duplication. The shapes line up with the usages inDashboardChart,TooltipDesign,ReportCard, andReportPiChartItem. Looks solid.admin-ui/app/routes/Dashboards/DashboardPage.tsx (2)
363-372: Date range wiring into the chart looks correctThe Dayjs state is kept non‑null and formatted as
'YYYYMM'before being passed toDashboardChart:const startMonth = startDate.format('YYYYMM') const endMonth = endDate.format('YYYYMM')This lines up with
DashboardChartPropsand the moment‑based augmentation logic inDashboardChart.tsx. No issues here.
218-243: Verify the filtering requirement for down/degraded services on DashboardThe stated objective "Do not show services that are down or not present on the Dashboard or Health page" cannot be verified from the codebase. The current implementation shows all non-unknown services, including 'down' and 'degraded' statuses. Additionally, the HealthPage component displays all services from
useHealthStatus()without filtering out down services. If this filtering requirement is real, it should be documented. If it's a requirement, both DashboardPage and HealthPage need updates. Note: 'not present' services are already filtered at theuseHealthStatushook level before reaching the components.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
admin-ui/app/routes/Dashboards/DashboardPage.tsx (1)
184-215: License validity formatting uses shared utility, but confirm input shapeSwitching the “validity period” display to
formatDate(license?.validityPeriod)centralizes formatting and keeps the user info block consistent with the rest of the app. AssumingvalidityPeriodis either a date string/number your util already handles orundefined, this is a safe change.If
validityPeriodcan be something non-date-like, consider adding a fallback insideformatDate(or here) to avoid showing"Invalid date"or similar artifacts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
admin-ui/app/routes/Dashboards/Chart/DashboardChart.tsx(2 hunks)admin-ui/app/routes/Dashboards/DashboardPage.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
admin-ui/app/routes/Dashboards/Chart/DashboardChart.tsx (1)
admin-ui/app/routes/Dashboards/types/DashboardTypes.ts (2)
DashboardChartProps(53-57)MauStatEntry(5-5)
admin-ui/app/routes/Dashboards/DashboardPage.tsx (5)
admin-ui/app/routes/Dashboards/hooks/useDashboardLicense.ts (1)
useDashboardLicense(19-30)admin-ui/app/routes/Dashboards/hooks/useDashboardClients.ts (1)
useDashboardClients(7-26)admin-ui/app/routes/Dashboards/hooks/useDashboardLockStats.ts (1)
useDashboardLockStats(24-68)admin-ui/app/routes/Dashboards/types/DashboardTypes.ts (1)
MauDateRange(5-5)admin-ui/app/routes/Dashboards/DateRange/index.tsx (1)
DateRange(16-72)
🔇 Additional comments (3)
admin-ui/app/routes/Dashboards/DashboardPage.tsx (3)
5-7: Hook-based data wiring is consistent and improves separation of concernsUsing dedicated hooks (
useDashboardLicense,useDashboardClients,useDashboardLockStats,useMauStats,useHealthStatus) plus explicit Auth/Cedar typing forRootStatekeeps this container focused on composition instead of data-fetch details. Theenabledflags on lock stats and MAU stats are also appropriately gated on token/permissions andisLockServiceAvailable.No functional issues spotted here; just ensure the hook return types (
license,clients,services,mauData) stay aligned with these destructured shapes as those hooks evolve.Also applies to: 20-20, 34-37, 91-104, 120-126
147-183: Lock stats summary integration is now safe and guardedThe updated
summaryDatablock only appends Jans Lock MAU entries whenlockDatahas entries andlockStatsis truthy, and it uses the aggregatedlatestStatsvalues. This addresses the previous risk of dereferencing an undefinedlockStatswhile still hiding the extra cards when no lock data is available.Behavior here aligns well with the conditional enablement of
useDashboardLockStats.
341-360: Blocking state now accounts for all dashboard data dependenciesThe expanded
isBlockingcondition correctly aggregates the loading flags for license, clients, MAU stats, health, lock stats, and Cedar initialization/permissions. This ensures the loader only disappears once all pieces of data needed for the page (and authorization) are resolved.One thing to double-check:
(!cedarInitialized && !permissions)assumespermissionsis falsy until fully available. Ifpermissionscan be a non-empty but still “incomplete” object, you may eventually want a more explicit readiness flag, but that’s outside the scope of this change.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
admin-ui/app/routes/Dashboards/Chart/DashboardChart.tsx(2 hunks)admin-ui/app/routes/Dashboards/DashboardPage.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
admin-ui/app/routes/Dashboards/DashboardPage.tsx (5)
admin-ui/app/routes/Dashboards/hooks/useDashboardLicense.ts (1)
useDashboardLicense(19-30)admin-ui/app/routes/Dashboards/hooks/useDashboardClients.ts (1)
useDashboardClients(7-26)admin-ui/app/routes/Dashboards/hooks/useDashboardLockStats.ts (1)
useDashboardLockStats(24-68)admin-ui/app/routes/Dashboards/types/DashboardTypes.ts (1)
MauDateRange(5-5)admin-ui/app/routes/Dashboards/DateRange/index.tsx (1)
DateRange(16-72)
🔇 Additional comments (6)
admin-ui/app/routes/Dashboards/Chart/DashboardChart.tsx (1)
16-55: LGTM: Well-implemented data augmentation with proper guards.The refactored component successfully addresses all previous review comments:
- Type consistency achieved by importing
DashboardChartPropsfrom the shared types module- Reversed date range guard prevents empty results (lines 27-29)
- Iterator cloning eliminates mutation issues (line 51:
current.clone().add(1, 'month'))- Map-based lookup achieves O(n) complexity instead of O(n²)
- Proper memoization ensures efficient re-renders
The implementation is clean, type-safe, and performant.
admin-ui/app/routes/Dashboards/DashboardPage.tsx (5)
166-182: LGTM: Null safety for lockStats access.The conditional now properly guards against undefined
lockStatsbefore accessing its properties, resolving the potential runtime error flagged in the previous review.
217-223: LGTM: Exact service name matching.The
includes()check has been replaced with exact equality (s.name === key), eliminating false positive matches from substring collisions.
128-145: LGTM: Summary now reflects the selected date range.The
mauCountandtokenCountnow derive from the user-selectedendDateinstead of the current calendar month, ensuring the summary cards align with the displayed chart data when historical ranges are selected.
362-384: LGTM: Date handlers prevent inverted ranges.The handlers automatically adjust the opposite bound when a user selects incompatible dates:
handleStartDateChangemovesendDateforward if the new start is laterhandleEndDateChangemovesstartDatebackward if the new end is earlierThis ensures
useMauStatsnever receives an inverted range.
504-507: LGTM: Safe mauData fallback prevents type mismatch.The
mauData ?? []fallback ensuresDashboardChartreceives a defined array even during transient loading states, satisfying the non-optionalstatDataprop type.Also applies to: 504-507, 514-517
Also applies to: 514-517
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
admin-ui/app/routes/Dashboards/DashboardPage.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
admin-ui/app/routes/Dashboards/DashboardPage.tsx (5)
admin-ui/app/routes/Dashboards/hooks/useDashboardLicense.ts (1)
useDashboardLicense(19-30)admin-ui/app/routes/Dashboards/hooks/useDashboardClients.ts (1)
useDashboardClients(7-26)admin-ui/app/routes/Dashboards/hooks/useDashboardLockStats.ts (1)
useDashboardLockStats(24-68)admin-ui/app/routes/Dashboards/types/DashboardTypes.ts (1)
MauDateRange(5-5)admin-ui/app/routes/Dashboards/DateRange/index.tsx (1)
DateRange(16-72)
🔇 Additional comments (7)
admin-ui/app/routes/Dashboards/DashboardPage.tsx (7)
5-6: LGTM: Import additions are well-structured.The new imports support the refactoring to hook-based data fetching and date range management. The dayjs library is appropriate for date manipulation, and the hook imports align with the RTK Query pattern shown in the related code snippets.
Also applies to: 20-20, 34-37
51-52: LGTM: Sensible default date range.The 3-month default range provides a reasonable time window for dashboard analytics while keeping the initial data load manageable.
91-127: Well-structured hook-based data fetching.The refactoring to RTK Query hooks is clean and follows best practices. The conditional enablement of lock stats based on service availability is a good optimization.
One consideration:
isLockServiceAvailableonly checks forstatus === 'up', excluding'degraded'. If lock stats should still be fetched when the service is degraded, update the check:const isLockServiceAvailable = useMemo(() => { const lockService = services.find((s) => s.name === 'jans-lock') - return lockService?.status === 'up' + return lockService?.status === 'up' || lockService?.status === 'degraded' }, [services])If the current behavior is intentional (only fetch when fully operational), no change is needed.
128-145: LGTM: MAU summary correctly tied to selected date range.The computation now properly derives
yearMonthfrom the user-selectedendDateinstead of the current date, ensuring the summary cards stay in sync with the chart when historical ranges are selected. The integer arithmetic is clean and the null handling is appropriate.
166-182: LGTM: Lock stats safely guarded.The null check for
lockStatsprevents runtime errors. TheuseDashboardLockStatshook (from related snippets) provides safe defaults, ensuringmonthly_active_usersandmonthly_active_clientsare always numbers.
342-384: LGTM: Comprehensive loading state and validated date range handlers.The
isBlockinglogic correctly aggregates all async operations. The date change handlers properly prevent inverted ranges by auto-adjusting the opposite bound, ensuringuseMauStatsalways receives a valid range.
386-387: LGTM: Chart and date range integration is correct.The formatted month strings (
YYYYMM) and props passed toDashboardChartandDateRangealign with their expected interfaces. ThemauData ?? []fallback ensures type safety during loading states.Also applies to: 495-527
| const getServiceStatus = useCallback( | ||
| (key: string) => { | ||
| if (key === 'db_status') return dbStatus | ||
| if (key === 'status') return serverStatus | ||
| return serverHealth[key] | ||
| const service = services.find((s) => s.name === key) | ||
| return service?.status ?? 'unknown' | ||
| }, | ||
| [serverHealth, dbStatus, serverStatus], | ||
| [services], | ||
| ) | ||
|
|
||
| const statusDetails = useMemo(() => { | ||
| const allServices = [ | ||
| { label: 'menus.oauthserver', key: 'jans-auth' }, | ||
| { label: 'dashboard.config_api', key: 'jans-config-api' }, | ||
| { label: 'dashboard.database_status', key: 'database' }, | ||
| { label: 'FIDO', key: 'jans-fido2' }, | ||
| { label: 'CASA', key: 'jans-casa' }, | ||
| { label: 'dashboard.key_cloak', key: 'keycloak' }, | ||
| { label: 'SCIM', key: 'jans-scim' }, | ||
| { label: 'dashboard.jans_lock', key: 'jans-lock' }, | ||
| ] | ||
|
|
||
| return allServices.filter((serviceConfig) => { | ||
| const status = getServiceStatus(serviceConfig.key) | ||
| return status === 'up' || status === 'degraded' | ||
| }) | ||
| }, [services, getServiceStatus]) | ||
|
|
||
| const getClassName = useCallback( | ||
| (key: string) => { | ||
| const value = getStatusValue(key) | ||
| if (!value) return classes.crossText | ||
| const statusUpper = String(value).toUpperCase() | ||
| return statusUpper === 'RUNNING' || statusUpper === 'ONLINE' | ||
| ? classes.checkText | ||
| : classes.crossText | ||
| const status = getServiceStatus(key) | ||
| return status === 'up' ? classes.checkText : classes.crossText | ||
| }, | ||
| [getStatusValue, classes.checkText, classes.crossText], | ||
| [getServiceStatus, classes.checkText, classes.crossText], | ||
| ) | ||
|
|
||
| const getStatusText = useCallback( | ||
| (key: string) => { | ||
| const value = getStatusValue(key) | ||
| if (!value) return 'Unknown' | ||
| return value | ||
| const status = getServiceStatus(key) | ||
| if (status === 'up') return 'Running' | ||
| if (status === 'down') return 'Down' | ||
| if (status === 'degraded') return 'Degraded' | ||
| return 'Unknown' | ||
| }, | ||
| [getStatusValue], | ||
| [getServiceStatus], | ||
| ) | ||
|
|
||
| const getStatusIcon = useCallback( | ||
| (key: string) => { | ||
| const value = getStatusValue(key) | ||
| if (!value) return CrossIcon | ||
| const statusUpper = String(value).toUpperCase() | ||
| return statusUpper === 'RUNNING' || statusUpper === 'ONLINE' ? CheckIcon : CrossIcon | ||
| const status = getServiceStatus(key) | ||
| return status === 'up' ? CheckIcon : CrossIcon | ||
| }, | ||
| [getStatusValue], | ||
| [getServiceStatus], | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Service filtering correctly implements PR objective.
The code now properly filters to show only 'up' or 'degraded' services, hiding 'down' and 'unknown' services as required. The exact name matching prevents false positives.
Note on icon display: getStatusIcon (lines 262-268) returns CrossIcon for degraded services. If degraded services should have a distinct visual indicator (e.g., a warning icon rather than a cross), consider adding a specific case:
const getStatusIcon = useCallback(
(key: string) => {
const status = getServiceStatus(key)
- return status === 'up' ? CheckIcon : CrossIcon
+ if (status === 'up') return CheckIcon
+ if (status === 'degraded') return WarningIcon // or appropriate icon
+ return CrossIcon
},
[getServiceStatus],
)If showing CrossIcon for degraded is intentional, no change needed.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
admin-ui/app/routes/Dashboards/DashboardPage.tsx lines 217-268: getStatusIcon
currently returns CheckIcon for 'up' and CrossIcon for all other statuses, which
makes degraded services show a cross; if degraded should have a distinct
indicator, add an explicit case that returns a WarningIcon for status ===
'degraded', update the function to handle 'up'/'degraded'/'down' explicitly, and
import WarningIcon at top of the file (or use the existing warning icon
component) so degraded services render the correct icon.
|
#2518
Closes #2518
Summary by CodeRabbit
Style
UI Changes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.