[change:ux] Make progress bar in status page more user friendly #863#1015
[change:ux] Make progress bar in status page more user friendly #863#1015PRoIISHAAN wants to merge 15 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a daily "usage" UI to the status page: new CSS for a usage-overview widget and a React implementation using react-circular-progressbar to render radial progress bars. The Status component gains helpers to format used/remaining values, compute reset time remaining, and choose colors/icons; it renders per-check usage cards, optional daily reset messaging, and an upgrade CTA. The feature is enabled by default in internals/config/default.yml, package.json updated, and multiple i18n keys/translations were added. Sequence Diagram(s)sequenceDiagram
participant Browser
participant StatusComponent
participant API_Server
participant RADIUS_Backend
Browser->>StatusComponent: Load status page
StatusComponent->>API_Server: GET /status (request usage checks)
API_Server->>RADIUS_Backend: Query counters and reset timestamps
RADIUS_Backend-->>API_Server: Return counters + reset info
API_Server-->>StatusComponent: Return usage checks + reset timestamps
StatusComponent->>StatusComponent: compute colors, icons, remaining time, formatted values
StatusComponent->>Browser: render usage-overview (radial progress bars, labels, reset line)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 4
🤖 Fix all issues with AI agents
In `@client/components/status/status.js`:
- Around line 1381-1463: The JSX duplicates nearly identical blocks and uses a
nested ternary; extract a helper method (e.g., renderUsageCheck) on the
component class that accepts (check, color, icon, label) and renders the shared
markup while calling existing helpers getUserCheckFormattedValue and
getUserCheckUsedValue; then replace the nested ternary in render with two simple
checks (check.type === "seconds" and check.type === "bytes") that call
renderUsageCheck with timerIcon/"Time" and dataIcon/"Data" respectively,
removing the nested ternary and eliminating the duplicated JSX.
- Around line 17-20: ESLint flagged import order: the external module imports
(react-circular-progressbar's CircularProgressbarWithChildren and buildStyles)
must come before the local import from ../../utils/get-language-headers; reorder
the imports so the lines importing CircularProgressbarWithChildren and
buildStyles appear above the import of get-language-headers (referenced symbol:
CircularProgressbarWithChildren, buildStyles, and the local module import
../../utils/get-language-headers).
- Around line 1260-1283: The getUsageColorAndIcons function computes
usagePercentage as (result / value) * 100 and can divide by zero; update
getUsageColorAndIcons to defensively handle value === 0 (or non-numeric) before
performing the division—coerce value/result to numbers, if value is 0 return a
safe default color/icons (e.g., the low-usage/green set) or a clearly defined
fallback object, otherwise compute usagePercentage and keep the existing
threshold logic; reference getUsageColorAndIcons and its value/result parameters
when making the change.
- Around line 1358-1368: In the Status component render block in
client/components/status/status.js, replace hardcoded English UI strings with
the i18n helper (e.g., wrap them in t`` or gettext()), specifically: change the
"Daily usage overview" text inside the div with className
"usage-overview-title", the paragraph "Track how much time and data you've used
today", and all occurrences of the labels "Time", "Data", and the word
"remaining" within the same render output; ensure you leave conditional logic
using radiusUsageSpinner, this.getSpinner(), settings.subscriptions, and
userPlan.name unchanged and only wrap the literal strings so the component uses
translated text.
🧹 Nitpick comments (3)
client/components/status/index.css (1)
136-142: Consider adding responsive styles for the usage boxes.The
.usage-boxhas fixed dimensions (width: 174px,height: 236px), but there are no responsive adjustments in the media query for small screens (lines 209-220). On narrow viewports (< 500px), two usage boxes side-by-side would overflow.Consider adding responsive rules to allow the boxes to resize or stack on smaller screens.
💡 Suggested responsive adjustment
`@media` screen and (min-width: 0px) and (max-width: 500px) { /* existing rules... */ .usage-box { width: 100%; max-width: 174px; height: auto; min-height: 200px; } .usage-checks-container { flex-direction: column; } }client/components/status/status.js (2)
1469-1474: Minor:getResetTimeRemainingcalled twice.The method is called once in the condition (line 1470) and again in the template literal (line 1472). Consider storing the result in a variable.
💡 Suggested optimization
{(() => { const resetTime = userChecks[0]?.reset && this.getResetTimeRemaining(userChecks[0].reset); return resetTime && ( <div className="usage-reset-info"> {t`*Daily limits reset in ${resetTime}`} </div> ); })()}Or compute
resetTimeRemainingonce before the return statement.
1384-1385: Alt text should be translatable for accessibility.The
altattributes ("Timer Icon", "Data Icon") are hardcoded English strings. For proper i18n/a11y support, these should use translation functions.-<img src={timerIcon} alt="Timer Icon" /> +<img src={timerIcon} alt={t`TIMER_ICON_ALT`} />
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
client/components/status/__snapshots__/status.test.js.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
client/components/status/index.cssclient/components/status/status.jsinternals/config/default.ymlpackage.json
🧰 Additional context used
🧬 Code graph analysis (1)
client/components/status/status.js (1)
client/reducers/plan-exhausted.js (1)
planExhausted(3-10)
🪛 ESLint
client/components/status/status.js
[error] 17-20: react-circular-progressbar import should occur before import of ../../utils/get-language-headers
(import/order)
[error] 1362-1362: ' can be escaped with ', ‘, ', ’.
(react/no-unescaped-entities)
[error] 1381-1463: Do not nest ternary expressions.
(no-nested-ternary)
⏰ 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). (1)
- GitHub Check: Tests and Coverage
🔇 Additional comments (6)
package.json (1)
34-34: LGTM!The
react-circular-progressbarlibrary is a well-maintained choice for this feature. The version and placement in dependencies is appropriate.internals/config/default.yml (1)
104-104: Verify the intent to enable radius usage by default.Enabling
radius_usage_enabled: trueby default will display the new usage overview UI for all organizations. This changes the default behavior for new and existing deployments that haven't explicitly configured this setting.Per the linked issue
#863, this appears intentional ("consider enabling the feature by default"), but please confirm this is the desired rollout strategy.client/components/status/index.css (1)
144-208: LGTM!The layout and typography styles for the usage overview section are well-structured with appropriate use of flexbox.
client/components/status/status.js (3)
1168-1197: LGTM with minor observation.The remaining value calculation and formatting logic is correct. Note that the time formatting logic (hours/minutes calculation) is duplicated in
getUserCheckUsedValuebelow - consider extracting to a shared helper if this pattern grows.
1199-1240: LGTM!The "used of total" formatting logic correctly caps usage at the total value and handles all type variants appropriately.
1242-1258: LGTM!The reset time calculation correctly handles edge cases (null timestamp, past timestamps) and provides human-readable remaining time.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@client/components/status/status.js`:
- Around line 1229-1237: The template-literal strings using t`` in
client/components/status/status.js (the `return t\`${usedFormatted} used of
${totalFormatted}\`` / `return t\`${usedFormatted} used of ${totalFormatted}\``
/ `return t\`${used} used of ${intValue}\`` occurrences inside the switch
handling formats) are not present in our .po files; run the translation
extraction to add them to all language catalogs (e.g. run the provided yarn
translations-update commands for each i18n/*.po or run the bulk update to sync
all files) so the "used of" entries are extracted and translations are available
before deployment.
🧹 Nitpick comments (3)
client/components/status/status.js (3)
1173-1191: Time abbreviations may not localize correctly.The time formatting uses embedded English abbreviations ("h", "m") in template literals. While
t\`` is used, different languages have different abbreviations for hours/minutes (e.g., German uses "Std"/"Min", French uses "h"/"min"). Consider using translation keys with placeholders:// Example approach const hours = ...; const minutes = ...; if (hours > 0 && minutes > 0) { return t`TIME_HOURS_MINUTES`.replace('{hours}', hours).replace('{minutes}', minutes); }Or leverage the existing
Intl.RelativeTimeFormatAPI for proper locale-aware formatting.Also,
t\0`` on lines 1174 and 1178 is unnecessary since "0" is language-neutral.
1418-1423: Type inconsistency forcheck.valuecomparison.Line 1421 compares
check.value === "0"(string), whilegetUsageColorAndIconsat line 1261 converts it withNumber(value). Ifcheck.valuecan be either string or number, consider normalizing:- if (check.value === "0") { + if (Number(check.value) === 0) {This ensures consistent behavior regardless of the API response type.
1451-1456: Missing translation key for reset time message.Line 1454's string
*Daily limits reset in ${...}has embedded English that won't be translated. Add a translation key likeDAILY_LIMITS_RESET_INto the .po files:- {t`*Daily limits reset in ${this.getResetTimeRemaining(userChecks[0].reset)}`} + {t`DAILY_LIMITS_RESET_IN`.replace('{time}', this.getResetTimeRemaining(userChecks[0].reset))}And add corresponding entries in .po files:
msgid "DAILY_LIMITS_RESET_IN" msgstr "*Tägliche Limits werden in {time} zurückgesetzt"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
client/components/status/index.cssclient/components/status/status.jsi18n/de.poi18n/en.poi18n/es.poi18n/fur.poi18n/it.poi18n/ru.poi18n/sl.po
✅ Files skipped from review due to trivial changes (1)
- i18n/sl.po
⏰ 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). (1)
- GitHub Check: Tests and Coverage
🔇 Additional comments (10)
i18n/fur.po (1)
183-202: Translation additions look good.
Thanks for adding the new usage strings here; this aligns with the UI changes.i18n/ru.po (1)
189-207: Translation additions look good.
The new usage overview strings are consistent with the UI updates.i18n/en.po (1)
182-200: Translation additions look good.
These strings match the new usage overview UI.i18n/it.po (1)
184-202: Translation additions look good.
New usage strings are consistent and complete.i18n/es.po (1)
188-206: Translation additions look good.
These new usage labels fit the updated status UI.client/components/status/index.css (1)
144-149: No change needed. The currentalign-items: startis the modern Box Alignment standard, supported across all modern browsers since 2021-2022. Switching toflex-startwould move away from the current standard. Unless your project explicitly targets IE11 (which doesn't appear to be the case based on your configuration), keep the current code as-is.Likely an incorrect or invalid review comment.
i18n/de.po (1)
183-201: LGTM!The German translations for the new usage overview UI elements are accurate and consistent with the existing translation style in this file.
client/components/status/status.js (3)
4-18: LGTM!The import reorganization addresses the previous ESLint import order concern. External dependencies are now properly ordered before local imports.
1259-1294: LGTM!The division-by-zero concern from the previous review is addressed with the guard at line 1265. The color thresholds (green ≤50%, yellow ≤80%, red >80%) provide clear visual feedback for usage levels.
1296-1331: LGTM!Good refactor extracting the repeated JSX into
renderUsageCheckContent, addressing the previous nested ternary concern. The component is well-structured with proper styling.Minor note: Line 1299's alt text
${label} Iconhas "Icon" hardcoded. For complete i18n, consider a translation key, though this is low priority since screen readers typically handle this adequately.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@client/components/status/status.js`:
- Around line 1166-1239: Both getUserCheckFormattedValue and
getUserCheckUsedValue currently use parseInt on value/result and assume valid
numbers; add defensive guards by converting with Number(...) (or retain parseInt
but then validate) and checking Number.isFinite(...) for intValue and intResult
before using them. If either conversion is not finite, return a safe fallback
(e.g., t`0` or a localized "N/A" string) and avoid calling prettyBytes or
formatTime; update references inside getUserCheckFormattedValue (type
"bytes"/"seconds" and default) and getUserCheckUsedValue (used calculation,
bytes branch using prettyBytes, seconds branch using formatTime, and default) to
short-circuit when inputs are invalid.
- Around line 1418-1446: Normalize and validate quota values before rendering:
at the top of the userChecks.map callback convert check.value to a number (e.g.,
const valueNum = Number(check.value)), then if (!Number.isFinite(valueNum) ||
valueNum <= 0) return null; call this.getUsageColorAndIcons with valueNum
(instead of raw check.value) and pass the normalized valueNum into
this.renderUsageCheckContent so the progress component receives a finite
positive number.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
client/components/status/status.jsi18n/de.poi18n/en.poi18n/es.poi18n/fur.poi18n/it.poi18n/ru.poi18n/sl.po
🚧 Files skipped from review as they are similar to previous changes (5)
- i18n/sl.po
- i18n/de.po
- i18n/fur.po
- i18n/ru.po
- i18n/en.po
🧰 Additional context used
🧬 Code graph analysis (1)
client/components/status/status.js (1)
client/reducers/plan-exhausted.js (1)
planExhausted(3-10)
⏰ 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). (1)
- GitHub Check: Tests and Coverage
🔇 Additional comments (2)
i18n/it.po (1)
184-208: Translation additions look good.The new usage overview keys and labels align with the updated status UI.
i18n/es.po (1)
188-213: LGTM for the new usage translations.These entries correctly cover the new daily-usage UI labels.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@client/components/status/status.js`:
- Around line 1472-1477: The template literal used in the status component for
the reset message (the expression t`*Daily limits reset in
${this.getResetTimeRemaining(userChecks[0].reset)}` inside the usage-reset-info
render) lacks a msgid in the .po files; add a translation entry like "*Daily
limits reset in %s" to all .po files (and update your i18n extraction if used)
so the string uses the translation instead of falling back to English, ensuring
the placeholder matches getResetTimeRemaining output formatting.
🧹 Nitpick comments (1)
client/components/status/status.js (1)
1166-1249: LGTM - defensive guards properly implemented.The methods now correctly validate inputs using
Number()andNumber.isFinite()checks before performing calculations. Both methods returnt\N/A`for invalid inputs, preventingNaN` propagation.Optional: Consider extracting the duplicated time formatting logic (hours/minutes calculation pattern appearing in both
getUserCheckFormattedValueandgetUserCheckUsedValue) into a shared helper to reduce duplication.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
client/components/status/__snapshots__/status.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
client/components/status/status.jsi18n/de.poi18n/en.poi18n/es.poi18n/fur.poi18n/it.poi18n/ru.poi18n/sl.po
🚧 Files skipped from review as they are similar to previous changes (4)
- i18n/sl.po
- i18n/ru.po
- i18n/en.po
- i18n/fur.po
⏰ 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). (1)
- GitHub Check: Tests and Coverage
🔇 Additional comments (7)
i18n/de.po (1)
183-212: LGTM!The new German translation entries for the daily usage overview feature are complete and appropriately translated. All required keys (
DAILY_USAGE_OVERVIEW,DAILY_USAGE_OVERVIEW_DESCRIPTION,USAGE_TIME,USAGE_DATA,USAGE_REMAINING,USAGE_USED_OF,N/A) are present with proper German translations.i18n/es.po (1)
188-217: LGTM!The Spanish translation entries are complete and linguistically appropriate. All new keys for the usage overview feature are properly translated.
i18n/it.po (1)
184-213: LGTM!The Italian translation entries are complete and properly translated. All required keys for the daily usage overview feature are present.
client/components/status/status.js (4)
1-18: LGTM!The imports are now properly organized with the
react-circular-progressbarimport placed correctly among other external library imports.
1269-1304: LGTM - division by zero guard implemented.The method now properly handles the case when
valueis 0 or non-numeric by returning the green (low usage) color scheme as a safe default before the division occurs.
1306-1341: LGTM - well-structured helper method.The extracted
renderUsageCheckContenthelper cleanly encapsulates the repeated JSX for rendering usage check cards, addressing the previous nested ternary concern.
1416-1470: LGTM - proper validation and normalization of usage data.The render logic now properly:
- Validates
valueNumandresultNumusingNumber.isFinite()checks- Filters out invalid or zero quota values
- Creates a normalized check object with numeric values before passing to the progress bar
- Uses proper i18n functions for all user-facing strings
This addresses the previous concerns about filtering invalid quota values.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
github-project-automation
Bot
moved this to To do (general)
in OpenWISP Contributor's Board
github-project-automation
Bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/components/status/status.js`:
- Around line 1252-1266: getResetTimeRemaining currently returns "0m" for
sub‑minute values and "Xh 0m" when minutes are zero; update it to match the
app's time formatting rules by first checking secondsRemaining < 60 and
returning t`TIME_LESS_THAN_MINUTE`, and when hours > 0 only include the minutes
portion if minutes > 0 (otherwise return just `${hours}${t`TIME_HOUR_ABBR`}`);
keep using the same translation tokens (t`TIME_LESS_THAN_MINUTE`,
t`TIME_HOUR_ABBR`, t`TIME_MINUTE_ABBR`) and ensure the function still returns
null for non-positive or missing resetTimestamp.
In `@i18n/de.po`:
- Around line 209-212: The German translation for the "N/A" placeholder is
incorrect; update the msgstr for msgid "N/A" in i18n/de.po (currently "N/V") to
the appropriate German text per UX guidelines—e.g., replace "N/V" with "Nicht
verfügbar" or the preferred abbreviation "k. A." so the status placeholder in
client/components/status/status.js displays a standard German string.
- Around line 209-238: Update the German translations for the time-related
msgids to use standard German abbreviations and spacing: change msgid
"TIME_HOUR_ABBR" msgstr to "Std.", change msgid "TIME_MINUTE_ABBR" msgstr to
"Min.", and change msgid "TIME_LESS_THAN_MINUTE" msgstr to "<1 Min."; adjust
these msgstr values in i18n/de.po where those msgid entries appear.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
client/components/status/__snapshots__/status.test.js.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
client/components/status/status.jsi18n/de.poi18n/en.poi18n/es.poi18n/fur.poi18n/it.poi18n/ru.poi18n/sl.popackage.json
📜 Review details
⏰ 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). (2)
- GitHub Check: Tests and Coverage
- GitHub Check: QA-Checks
🔇 Additional comments (10)
package.json (1)
33-33: No review comments for this change.i18n/fur.po (1)
183-238: No review comments for this change.i18n/en.po (1)
182-238: No review comments for this change.i18n/it.po (1)
184-239: No review comments for this change.i18n/sl.po (1)
181-236: No review comments for this change.i18n/ru.po (1)
189-244: No review comments for this change.client/components/status/status.js (3)
4-18: No review comments for this change.
1167-1249: Defensive formatting and localized output look good.
1416-1499: Usage overview rendering looks solid with proper normalization.i18n/es.po (1)
188-243: No review comments for this change.
|
@PRoIISHAAN you were asking what to do with the extra space right I asked our UX team. |
Yes, what did they say? |
|
@avocadodavinci if you could chime in here it would be very helpful! |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@PRoIISHAAN we can use the horizontal bar for the desktop view and radial for narrow screens, see the figma design.
|
Hi @PRoIISHAAN 👋, This pull request has been marked as stale due to 48 days of inactivity after changes were requested. As a result, the linked issue(s) have been unassigned from you to allow other contributors to work on it. However, you can still continue working on this PR! If you push new commits or respond to the review feedback:
If you need more time or have questions about the requested changes, please let us know. We're happy to help! 🤝 If there's no further activity within 12 more days, this PR will be automatically closed (but can be reopened anytime). |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Changes ReviewedThe latest commit contains only minor formatting improvements in the test file: Test Formatting (
Overall PR AssessmentAll previous functionality remains intact. The PR successfully implements the user-friendly progress bar redesign with:
Files Reviewed (1 file)
Reviewed by kimi-k2.5 · 96,932 tokens |
3 participants
Checklist
Reference to Existing Issue
Closes #863
Description of Changes
Added react-circular-progressbar package for circular progress visualization
Replaced linear bars with circular progress indicators (CircularProgressbarWithChildren)
Added new CSS classes for the redesigned usage overview section
New Helper Methods in status.js:
getUserCheckFormattedValue() - Updated to calculate and format remaining quota
getUserCheckUsedValue() - New method to format "used of total" display
getResetTimeRemaining() - New method to calculate time until quota reset
getUsageColorAndIcons() - New method to determine color and icons based on usage percentage
Screenshot