Conversation
- Fix side padding to 14px for consistency with rest of product - Add white background to tab content area - Remove duplicate refresh button from Usage by Deployment table - Wire global refresh to also trigger deployment data refetch - Add sort on Last Run column in deployment usage table - Upgrade Subscription tab with area chart, summary stats, and last 30 active days window Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
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:
WalkthroughAdds a parent-controlled ref-based refetch hook for the deployment usage table (child registers its internal refetch on a passed ref), removes the table's manual refresh UI, updates "Last Run" sorting, and introduces layout wrapping and extensive "V2" subscription usage styling and spacing adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dashboard as "MetricsDashboard\n(container)"
participant Table as "DeploymentUsageTable\n(LLMUsageTable)"
participant Hook as "useDeploymentUsage\n(hooks/API)"
participant Backend as "Backend/API"
Dashboard->>Dashboard: handleRefresh()
Dashboard->>Hook: refetchOverview()
Dashboard->>Hook: refetchChart()
Dashboard->>Hook: refetchActivity()
Dashboard->>Hook: refetchSubscription()
Dashboard->>Table: deploymentRefetchRef.current?()
Table->>Hook: refetch()
Hook->>Backend: fetch data
Backend-->>Hook: data
Hook-->>Table: data
Hook-->>Dashboard: data (overview/chart/activity/subscription)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/src/components/metrics-dashboard/MetricsDashboard.css (1)
4-17: Consider centralizing the new 14px spacing as a CSS variable.
14pxis now repeated across dashboard/topbar/responsive rules. A shared token would reduce drift in future tweaks.♻️ Optional refactor
+.metrics-dashboard { + --metrics-page-gutter: 14px; +} .metrics-dashboard { - padding: 14px; + padding: var(--metrics-page-gutter); } .metrics-topbar { - margin: -14px -14px 20px -14px; - padding: 12px 14px; + margin: calc(-1 * var(--metrics-page-gutter)) calc(-1 * var(--metrics-page-gutter)) 20px calc(-1 * var(--metrics-page-gutter)); + padding: 12px var(--metrics-page-gutter); } `@media` (max-width: 768px) { .metrics-topbar { - margin: -14px -14px 16px -14px; - padding: 12px 14px; + margin: calc(-1 * var(--metrics-page-gutter)) calc(-1 * var(--metrics-page-gutter)) 16px calc(-1 * var(--metrics-page-gutter)); + padding: 12px var(--metrics-page-gutter); } .metrics-dashboard { - padding: 14px; + padding: var(--metrics-page-gutter); } }Also applies to: 238-253
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/metrics-dashboard/MetricsDashboard.css` around lines 4 - 17, Introduce a shared CSS variable for the repeated 14px spacing and replace the literal values with that token: add something like --spacing-sm: 14px at a root or container scope (e.g., :root or .metrics-dashboard) and update occurrences in .metrics-dashboard, .metrics-topbar and the related responsive rules to use var(--spacing-sm) for padding/margin calculations (including the margin: -14px ... and padding: 12px 14px and the height calc that uses 64px if applicable); ensure the variable name is descriptive (e.g., --spacing-sm) and update all other instances mentioned (lines ~238-253) so spacing is centralized.frontend/src/components/metrics-dashboard/LLMUsageTable.jsx (1)
127-132: Add cleanup to ref assignments for defensive consistency, though not currently necessary.The
refetchRef.currentassignment at line 130 should include a cleanup function, even thoughDeploymentUsageTableis not destroyed during tab switches (Tabs lacksdestroyInactiveTabPaneconfig). If the Tabs configuration changes to destroy inactive tabs in the future, this cleanup becomes essential to prevent stale function references. For consistency with React patterns, cleanup should be added:🩹 Suggested fix
useEffect(() => { if (refetchRef) { refetchRef.current = refetch; } + return () => { + if (refetchRef) { + refetchRef.current = null; + } + }; }, [refetch, refetchRef]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/metrics-dashboard/LLMUsageTable.jsx` around lines 127 - 132, The useEffect that exposes refetch via refetchRef should add a cleanup to avoid leaving a stale reference: inside the useEffect that assigns refetchRef.current = refetch (in LLMUsageTable.jsx), return a cleanup function that clears refetchRef.current (e.g., set to undefined or null) when the component unmounts or deps change; keep the existing dependency array [refetch, refetchRef] and only modify the effect to set the ref on mount/update and clear it in the returned cleanup function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/components/metrics-dashboard/LLMUsageTable.jsx`:
- Around line 127-132: The useEffect that exposes refetch via refetchRef should
add a cleanup to avoid leaving a stale reference: inside the useEffect that
assigns refetchRef.current = refetch (in LLMUsageTable.jsx), return a cleanup
function that clears refetchRef.current (e.g., set to undefined or null) when
the component unmounts or deps change; keep the existing dependency array
[refetch, refetchRef] and only modify the effect to set the ref on mount/update
and clear it in the returned cleanup function.
In `@frontend/src/components/metrics-dashboard/MetricsDashboard.css`:
- Around line 4-17: Introduce a shared CSS variable for the repeated 14px
spacing and replace the literal values with that token: add something like
--spacing-sm: 14px at a root or container scope (e.g., :root or
.metrics-dashboard) and update occurrences in .metrics-dashboard,
.metrics-topbar and the related responsive rules to use var(--spacing-sm) for
padding/margin calculations (including the margin: -14px ... and padding: 12px
14px and the height calc that uses 64px if applicable); ensure the variable name
is descriptive (e.g., --spacing-sm) and update all other instances mentioned
(lines ~238-253) so spacing is centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c51500bf-40b0-4a7e-b830-51a0da4fcb46
📒 Files selected for processing (3)
frontend/src/components/metrics-dashboard/LLMUsageTable.jsxfrontend/src/components/metrics-dashboard/MetricsDashboard.cssfrontend/src/components/metrics-dashboard/MetricsDashboard.jsx
Contributor
|
| Filename | Overview |
|---|---|
| frontend/src/components/metrics-dashboard/LLMUsageTable.jsx | Adds refetchRef wiring, compact number formatting, Last Run sort, and Executions column redesign; one unused Tag import remains |
| frontend/src/components/metrics-dashboard/MetricsDashboard.jsx | Adds deploymentRefetchRef + global refresh wiring; layout reorganisation looks correct |
| frontend/src/components/metrics-dashboard/MetricsDashboard.css | Side padding fixed to 14px, white tab-content background added, new compact execution span styles |
| frontend/src/components/custom-tools/header/Header.jsx | Formatting-only change: single-line return expanded to braced block |
Sequence Diagram
sequenceDiagram
participant User
participant MetricsDashboard
participant DeploymentUsageTable
participant useDeploymentUsage
MetricsDashboard->>DeploymentUsageTable: render(refetchRef=deploymentRefetchRef)
DeploymentUsageTable->>useDeploymentUsage: invoke hook
useDeploymentUsage-->>DeploymentUsageTable: { data, loading, error, refetch }
DeploymentUsageTable->>DeploymentUsageTable: useEffect — deploymentRefetchRef.current = refetch
Note over DeploymentUsageTable: cleanup sets current = null on unmount
User->>MetricsDashboard: click global Refresh button
MetricsDashboard->>MetricsDashboard: handleRefresh()
MetricsDashboard->>MetricsDashboard: refetchOverview / refetchChart / refetchActivity / refetchSubscription
MetricsDashboard->>DeploymentUsageTable: deploymentRefetchRef.current?.()
DeploymentUsageTable->>useDeploymentUsage: refetch()
useDeploymentUsage-->>DeploymentUsageTable: fresh data
Prompt To Fix All With AI
This is a comment left during a code review.
Path: frontend/src/components/metrics-dashboard/LLMUsageTable.jsx
Line: 13
Comment:
**Unused `Tag` import**
The `Tag` component is imported but is no longer used anywhere in this file. The Executions column was refactored from `<Tag color="success">` / `<Tag color="error">` to plain `<span>` elements with CSS classes (`.execution-success`, `.execution-error`), leaving this import as dead code.
```suggestion
Table,
Tabs,
Tooltip,
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (8): Last reviewed commit: "UN-3380 [FIX] Fix biome: CSS selector fo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Add cleanup function to refetchRef useEffect to prevent stale closures
- Narrow refetchRef PropTypes from object to shape({ current: func })
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
athul-rs
requested review from
Deepak-Kesavan,
chandrasekharan-zipstack,
jaseemjaskp and
vishnuszipstack
chandrasekharan-zipstack
approved these changes
Apr 5, 2026
Contributor
vishnuszipstack
left a comment
vishnuszipstack
left a comment
There was a problem hiding this comment.
@athul-rs why using - ve values in margin?
vishnuszipstack
approved these changes
Apr 6, 2026
…nd-layout Follows the same pattern as API Deployments and other pages where the entire content area (topbar, tabs, tab content) sits inside a white container box. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/metrics-dashboard/MetricsDashboard.css`:
- Line 4: Update the dashboard wrapper paddings from 12px to 14px to match the
new horizontal gutter used by the topbar/tabs: find the two occurrences
currently using "padding: 12px" (the top wrapper near the file start and the
other occurrence around the end of the file) and change them to "padding: 14px"
so left/right alignment across dashboard wrappers is consistent.
- Around line 55-57: The current CSS uses descendant selectors that match any
.ant-tabs inside .metrics-dashboard (including nested DeploymentUsageTable
tabs); update the selectors so they only target the top-level dashboard tabs by
using direct child combinators: change rules that reference .metrics-dashboard
.ant-tabs and .ant-tabs-nav to use .metrics-dashboard > .ant-tabs >
.ant-tabs-nav (and the analogous selector for the other rule at lines 60–62),
keeping the same declarations but narrowing the selector to the top-level
.ant-tabs only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e8f54b3-f6ad-4a51-b3c3-52ac62b6ea00
📒 Files selected for processing (1)
frontend/src/components/metrics-dashboard/MetricsDashboard.css
- Tokens: show as 12.6M/4.6K with full value on hover - Executions: inline compact display instead of spilling Tag components - Tabs nav alignment with dashboard header padding Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/src/components/metrics-dashboard/LLMUsageTable.jsx (2)
31-32: Consider nullish coalescing for clearer intent.SonarCloud flags
value || 0as non-idiomatic. Since the goal is to handlenull/undefinedfrom API responses while preserving actual0values correctly,??is more precise than||(which would also coerceNaNorfalse, though unlikely here).♻️ Suggested fix
function formatCompactNumber(value) { - const num = value || 0; + const num = value ?? 0; if (num >= 1_000_000_000) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/metrics-dashboard/LLMUsageTable.jsx` around lines 31 - 32, The helper function formatCompactNumber uses `value || 0`, which incorrectly treats falsy but valid zeros; change the fallback to the nullish coalescing operator so it only replaces null/undefined: update the assignment in formatCompactNumber (const num = value || 0) to use `??` (const num = value ?? 0) so real 0 values are preserved while still defaulting when the API returns null/undefined.
149-159: Ref-based refetch pattern is functional but consideruseImperativeHandlefor stricter typing.The implementation correctly exposes the
refetchfunction to the parent via ref assignment. The cleanup on unmount prevents stale calls. This works well.For a more idiomatic React approach,
useImperativeHandlewithforwardRefis the canonical pattern for exposing imperative methods to parents, but the current approach is simpler and avoids theforwardRefcomplexity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/metrics-dashboard/LLMUsageTable.jsx` around lines 149 - 159, The current pattern assigns refetch onto refetchRef inside useEffect (useEffect, refetchRef, refetch) which works but is not the idiomatic React way; refactor LLMUsageTable to use forwardRef and useImperativeHandle to expose a typed imperative API (e.g., expose a ref object with a refetch() method) instead of direct ref assignment, implement forwardRef around the component, call useImperativeHandle(ref, () => ({ refetch }), [refetch]) and remove the manual cleanup logic in the existing useEffect so the parent receives a stricter, well-typed handle.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/components/metrics-dashboard/LLMUsageTable.jsx`:
- Around line 31-32: The helper function formatCompactNumber uses `value || 0`,
which incorrectly treats falsy but valid zeros; change the fallback to the
nullish coalescing operator so it only replaces null/undefined: update the
assignment in formatCompactNumber (const num = value || 0) to use `??` (const
num = value ?? 0) so real 0 values are preserved while still defaulting when the
API returns null/undefined.
- Around line 149-159: The current pattern assigns refetch onto refetchRef
inside useEffect (useEffect, refetchRef, refetch) which works but is not the
idiomatic React way; refactor LLMUsageTable to use forwardRef and
useImperativeHandle to expose a typed imperative API (e.g., expose a ref object
with a refetch() method) instead of direct ref assignment, implement forwardRef
around the component, call useImperativeHandle(ref, () => ({ refetch }),
[refetch]) and remove the manual cleanup logic in the existing useEffect so the
parent receives a stricter, well-typed handle.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9166b772-d995-4dcc-a61a-ace2a63a445d
📒 Files selected for processing (2)
frontend/src/components/metrics-dashboard/LLMUsageTable.jsxfrontend/src/components/metrics-dashboard/MetricsDashboard.css
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/metrics-dashboard/MetricsDashboard.css
Use direct child selectors on .metrics-dashboard-container to prevent padding from leaking into the nested deployment-type-tabs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tement Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
|
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
3 participants
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.
What
Why
How
.ant-tabs-contentuseRef+deploymentRefetchRefto wire global refresh to deployment tablerefetchRefprop, added sort on Last Run columnCan this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.