fix(usage): meter comparability and unbounded loading - #8357
fix(usage): meter comparability and unbounded loading#8357talissoncosta wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
github-actions
Bot
added
the
front-end
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe change adds allowance comparability checks for billing periods and projects. It adds a timer hook that identifies loading requests lasting more than 20 seconds. The usage dashboard now renders stalled and error states and supports retry callbacks that refetch usage and subscription data. Usage meters now accept comparability, adjust copy and tone, and hide the allowance bar when comparison is not valid. Tests cover period comparability, non-comparable meter copy, and meter tones. Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: 🔵 Low · up to The usage page now avoids misleading comparisons and provides recovery for long-running requests, but some edge cases remain: omitted comparison settings can make the meter and chart disagree, changing filters can show the timeout state too early, and subscription metadata failures may leave users without a retry path. The PR is mergeable with explicit owner follow-up on these bounded issues. 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 |
Docker builds report
|
70ec520 to
202ccf8
Compare
✅ private-cloud · depot-ubuntu-latest-16 — run #19746 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
🗂️ Previous results✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19746 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19746 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19746 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19743 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #19743 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19743 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19743 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ private-cloud · depot-ubuntu-latest-16 — run #19731 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
✅ private-cloud · depot-ubuntu-latest-arm-16 — run #19731 (attempt 1)Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-arm-16 — run #19731 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
✅ oss · depot-ubuntu-latest-16 — run #19731 (attempt 1)Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8e46f0f1-f8fb-4bf7-be3e-438740499572
📒 Files selected for processing (13)
frontend/CHANGE_REQUEST_STUCK_BUTTON_INVESTIGATION.mdfrontend/CSS_TOKEN_GAPS_PLAN.mdfrontend/MV_IDENTITY_OVERRIDE_EDITOR_NOTES.mdfrontend/USAGE_DASHBOARD_DECISIONS.mdfrontend/USAGE_DASHBOARD_PARITY.mdfrontend/common/hooks/useTakingTooLong.tsfrontend/web/components/pages/usage/UsageDashboard.tsxfrontend/web/components/pages/usage/UsageDashboardPage.tsxfrontend/web/components/pages/usage/__tests__/utils.test.tsfrontend/web/components/pages/usage/components/UsageMeter/UsageMeter.tsxfrontend/web/components/pages/usage/components/UsageMeter/__tests__/utils.test.tsfrontend/web/components/pages/usage/components/UsageMeter/utils.tsfrontend/web/components/pages/usage/utils.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
|
|
||
| ## How to reproduce | ||
|
|
||
| ### Symptom — deterministic, no Zoox data needed |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add blank lines after the affected headings.
markdownlint-cli2 reports MD022 warnings because these headings are not followed by a blank line. Add one blank line after each heading at Lines 52, 60, 65, and 70.
Also applies to: 60-60, 65-65, 70-70
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 52-52: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
Source: Linters/SAST tools
|
|
||
| 1. Use a **change-request-enabled (versioned + 4-eyes) environment**. Staging envs don't have this by default — enable change requests on an env first. | ||
| 2. Open a feature's **Edit Feature** modal, make a value change, click **Create Change Request**, fill the dialog. | ||
| 3. In DevTools → Network, **block or set offline** the `POST …/features/workflows/change-requests/…` request (or throttle to force a timeout). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the actual create endpoint in the reproduction steps.
For a new change request, editFeatureStateChangeRequest sends a POST to environments/${environmentId}/create-change-request/. The features/workflows/change-requests/${req.id}/ endpoint is used for existing requests and uses PUT. The documented POST …/features/workflows/change-requests/… path matches neither branch, so the reproduction and response-capture steps can miss the failing request. This contract is defined in frontend/common/stores/feature-list-store.ts:525-752.
Also applies to: 61-61
| 2. Add a `font` group to `tokens.json` with `size` and `line-height` entries, | ||
| keyed on the role-free names and sourced from the existing values so nothing | ||
| moves visually: | ||
|
|
||
| ```jsonc | ||
| "font": { | ||
| "size": { | ||
| "xs": { "cssVar": "--font-xs-size", "light": "11px", "dark": "11px" }, | ||
| "sm": { "cssVar": "--font-sm-size", "light": "12px", "dark": "12px" }, | ||
| "md": { "cssVar": "--font-md-size", "light": "13px", "dark": "13px" }, | ||
| "base": { "cssVar": "--font-base-size", "light": "14px", "dark": "14px" } | ||
| // lg, xl, 2xl, 3xl, 4xl, 5xl follow | ||
| }, | ||
| "line-height": { | ||
| // one default per step | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Include direct font-variable consumers in the migration.
The proposed variables, such as --font-xs-size and --font-base-size, do not define the existing names listed in Lines 33-43, such as --font-body-sm-size and --font-caption-size. Migrating only the fs-* utility call sites will leave those component declarations invalid. Migrate those SCSS references or emit compatibility aliases, and add an acceptance check for unresolved --font-*-size references.
Proposed plan update
-5. Migrate the 137 `fs-regular` / `fs-caption` / `fs-captionSmall` /
- `fs-captionXSmall` call sites. Keep the old names as aliases while it is
- in flight, and delete them at the end so two scales never coexist for long.
+5. Migrate the 137 `fs-regular` / `fs-caption` / `fs-captionSmall` /
+ `fs-captionXSmall` call sites and the direct `--font-*` references listed
+ above. Keep the old names as aliases while it is in flight, and delete them
+ at the end so two scales never coexist for long.
+ Add an acceptance check that no component references an undefined
+ `--font-*-size` variable.Also applies to: 171-173
| a route-level component that fetches and renders. None of them is paired, so it | ||
| does not name the rendering half, but it names the fetching half exactly: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the fenced code block.
Line [186] opens a fenced code block without a language. Markdownlint MD040 reports this. Use text for this filename map, or the correct language identifier, so documentation lint passes.
This addresses the markdownlint warning reported for Line [186].
Proposed fix
-```
+```text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 186-186: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: efad4046-5647-40ef-9d40-cec84cc76248
📒 Files selected for processing (6)
frontend/common/hooks/useTakingTooLong.tsfrontend/web/components/pages/usage/UsageDashboard.tsxfrontend/web/components/pages/usage/UsageDashboardPage.tsxfrontend/web/components/pages/usage/components/UsageMeter/UsageMeter.tsxfrontend/web/components/pages/usage/components/UsageMeter/__tests__/utils.test.tsfrontend/web/components/pages/usage/components/UsageMeter/utils.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.
| return () => clearTimeout(timer) | ||
| }, [active, afterMs, attempt]) | ||
|
|
||
| return tooLong |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- frontend/common/hooks/useTakingTooLong.ts ---'
cat -n frontend/common/hooks/useTakingTooLong.ts
printf '%s\n' '--- UsageDashboard references ---'
rg -n -C 8 'useTakingTooLong|tooLong' frontend --glob '*.{ts,tsx}'Repository: Flagsmith/flagsmith
Length of output: 4719
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- UsageDashboard render branches ---'
sed -n '33,125p' frontend/web/components/pages/usage/UsageDashboard.tsx
printf '%s\n' '--- directly named hook tests/usages ---'
rg -n -C 5 'useTakingTooLong|STALLED_AFTER_MS|stalled' frontend --glob '*.{test,spec}.{ts,tsx}' --glob '*.{ts,tsx}'Repository: Flagsmith/flagsmith
Length of output: 11878
Return active && tooLong from useTakingTooLong.
When active changes to false after the timer expires, the hook can return the previous tooLong state for one render. UsageDashboard can therefore render the timeout state after loading completes. Add a regression test for this transition.
| import UsageMeter from './components/UsageMeter' | ||
| import UsageOverTime from './components/UsageOverTime' |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use permitted import aliases for the changed imports.
The changed imports use relative paths in files under frontend/.
frontend/web/components/pages/usage/UsageDashboard.tsx#L6-L7: importUsageMeterandUsageOverTimethrough theircomponents/aliases.frontend/web/components/pages/usage/UsageDashboardPage.tsx#L11-L18: import the addedisComparableToAllowancesymbol throughcomponents/pages/usage/utils.
As per coding guidelines, frontend/**/*.{js,jsx,ts,tsx} must “Use only common/, components/, and project/ import paths; do not use relative imports.”
📍 Affects 2 files
frontend/web/components/pages/usage/UsageDashboard.tsx#L6-L7(this comment)frontend/web/components/pages/usage/UsageDashboardPage.tsx#L11-L18
Source: Coding guidelines
| const { | ||
| data: subscriptionMeta, | ||
| isLoading: loadingLimit, | ||
| refetch: refetchLimit, | ||
| } = useGetSubscriptionMetadataQuery( | ||
| organisationId ? { id: organisationId } : skipToken, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- changed hunk ---'
git diff -- frontend/web/components/pages/usage/UsageDashboardPage.tsx
printf '%s\n' '--- page definitions and usage ---'
sed -n '1,180p' frontend/web/components/pages/usage/UsageDashboardPage.tsx
fd -i 'UsageDashboard' frontend
rg -n -C 5 'useGetSubscriptionMetadataQuery|isError=|Try again|loadingLimit|subscriptionMeta' frontend/web/components/pages/usage frontend/common frontend/components frontend/project 2>/dev/null || trueRepository: Flagsmith/flagsmith
Length of output: 17401
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- UsageDashboard contract ---'
sed -n '1,120p' frontend/web/components/pages/usage/UsageDashboard.tsx
printf '%s\n' '--- subscription metadata endpoint ---'
sed -n '1,80p' frontend/common/services/useSubscriptionMetadata.ts
printf '%s\n' '--- shared service base ---'
rg -n -C 6 'const service|createApi|fetchBaseQuery|baseQuery' frontend/common/services frontend/common | head -160Repository: Flagsmith/flagsmith
Length of output: 17024
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- RTK Query dependency and base service ---'
rg -n '"`@reduxjs/toolkit`"|reduxjs/toolkit' package.json frontend/package.json frontend/*/package.json 2>/dev/null || true
fd -i 'service' frontend/common -t f | head -40
fd -i 'package.json' . -t f -E node_modules -E dist -E build | head -30Repository: Flagsmith/flagsmith
Length of output: 334
Surface subscription metadata failures.
Destructure isError: limitFailed from useGetSubscriptionMetadataQuery and include it in UsageDashboard's isError prop. Otherwise, a failed request can render the dashboard without a plan limit or the Try again action.
The meter divided the filtered total by the organisation's 30-day allowance, so "Last 90 days" read roughly three times too high and a project filter measured one project against the whole allowance. The percentage now appears only when the total covers the same span and the same scope as the allowance. Otherwise the meter reports the count and labels the allowance as a 30-day figure. Fixes #8353 Fixes #8354 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The headline still took its colour from usagePercent even where the percentage itself had been withheld, so ninety days of usage could turn the number red on a comparison the meter had just disowned. The tone now follows the same rule as the copy, which also stops a self-hosted count, where there is no limit at all, rendering in success green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The chart took the same organisation-wide allowance, drew it as a reference line and titled itself "Usage vs plan limit", so a project filter or a ninety day window left it making the comparison the meter above had already refused. It now loses the line and the title together with the percentage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A request that never settles left the page on a loader with no way out. After twenty seconds it gives up and offers a retry. The retry carries two conditions worth naming. RTK keeps isFetching true across a refetch, so the timer needs an attempt key or it never restarts and the button does nothing. And retrying usage alone cannot recover a page whose organisation query was what failed, because that leaves the usage query skipped. Fixes #8355 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
67cfa22 to
838e421
Compare
When the headline is the total rather than a percentage, the panel read "1.5M API calls" on one side and "1.5M API calls used" on the other. The copy now owns the whole right hand block, so it shows the allowance instead of repeating the total, and disappears when there is no allowance to show. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
MeterCopy was exported but only ever used as a return type inside its own module. useTakingTooLong exported both a named binding and a default, which is the duplicate-export shape a dead code check flags; it is named only now, matching useScript and useProjectEnvironments. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/web/components/pages/usage/UsageDashboard.tsx (2)
15-15: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse one default for
comparable.When
comparableis omitted,UsageMeterdefaults it totrue, butUsageDashboardpassesundefinedtoUsageOverTime.limit. This gives callers inconsistent meter and chart behaviour. Defaultcomparabletotrueduring destructuring, or pass one normalised value to both children. Add a test for an omitted prop.
34-35: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset the stall timer when the usage-query identity changes.
When
billingPeriodorselectedProjectIdchanges whileisLoadingremains true,useTakingTooLongkeeps its existing timer because its dependencies exclude the query identity. The new request can therefore show the stalled state before 20 seconds have elapsed. Reset the timer for each query identity and add a test for this case.frontend/web/components/pages/usage/components/UsageMeter/utils.ts (1)
47-53: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the inline union into a named type.
Define a named type for
UsageTone | undefinedand use it as the return type ofmeterTone.As per coding guidelines, TypeScript files must extract inline union types into named types.
Proposed change
+type MeterTone = UsageTone | undefined + export const meterTone = ( total: number, limit: PlanLimit, comparable: boolean, warnAt: number, -): UsageTone | undefined => +): MeterTone =>Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 55a09ff8-c792-4d76-bb30-8e1622cc1086
📒 Files selected for processing (3)
frontend/common/hooks/useTakingTooLong.tsfrontend/web/components/pages/usage/UsageDashboard.tsxfrontend/web/components/pages/usage/components/UsageMeter/utils.ts
💤 Files with no reviewable changes (1)
- frontend/common/hooks/useTakingTooLong.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review.
Filtering by project made the meter drop its percentage, which threw away a real number to avoid a wording problem: a project's usage over the organisation's allowance is that project's share of what you are allowed. The meter now names the project and says the percentage is of your allowance rather than of plan consumed. Last 90 days still drops to a count, since ninety days against a thirty day allowance cannot be reworded into meaning. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Changes
Three fixes to the usage dashboard from #8320, all reported after it merged.
The meter compared numbers that do not belong together (#8353, #8354). The
total moved with both filters, the allowance never did, so "Last 90 days" read
about three times too high and picking a project measured that project against
the whole organisation's allowance.
max_api_callsis a 30-day figure, so apercentage only means something when the total covers the same 30 days and the
same organisation. When it does not, the meter now reports the count and says
the allowance is per 30 days, rather than a percentage that is wrong.
The page could spin for ever (#8355). A request that never settled left a
loader with no way out. After twenty seconds it gives up and offers a retry.
Fixes #8353
Fixes #8354
Fixes #8355
How did you test this code?
Unit tests cover both the comparability rule and the meter copy, 40 passing in
pages/usage.Worth checking on the deploy preview: