Refactor: Extracted session utility functions from status component#1081
Refactor: Extracted session utility functions from status component#1081JarrarHaiderNemati wants to merge 4 commits intoopenwisp:masterfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 2 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded a new utility module at client/utils/session-utils.js exporting three helpers: getDuration(seconds) — converts seconds into a human-readable hours/minutes/seconds string; getDateTimeFormat(language, time_option, date) — formats a date via Intl.DateTimeFormat with a fallback to String(new Date(date)); getUserCheckFormattedValue(value, type) — parses integer input and, based on type, formats bytes via pretty-bytes, converts seconds via timeFromSeconds, or returns the original value. The module imports pretty-bytes and timeFromSeconds. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/utils/session-utils.js`:
- Around line 1-13: The file defines a custom getDuration(seconds) while also
importing timeFromSeconds from duration-formatter and using it elsewhere (e.g.,
getUserCheckFormattedValue); consolidate to a single formatter by removing the
custom getDuration and replacing its usages with timeFromSeconds (or vice
versa), ensuring the chosen formatter's output matches existing UI expectations;
update all references to getDuration in this module (and any exports) to call
timeFromSeconds with appropriate arguments or transform its output to match the
previous string format.
- Around line 4-13: Update getDuration to handle zero/negative inputs by
returning a user-friendly string instead of an empty string: inside the
getDuration function, after converting seconds to Number (variable number), add
a check (e.g., if number <= 0) that returns "0 secs" for zero/negative values;
otherwise keep the existing logic that computes h, m, s and builds
hDisplay/mDisplay/sDisplay so callers of getDuration receive "0 secs" instead of
an empty string.
- Around line 22-34: The file ends without a trailing newline which fails
ESLint; open the module containing getUserCheckFormattedValue
(client/utils/session-utils.js) and add a single newline character at the end of
the file (after the closing brace of getUserCheckFormattedValue) so the file
ends with a newline; save and re-run lint to confirm the warning is resolved.
- Around line 15-20: The parameter name time_option in getDateTimeFormat
violates camelCase; rename it to timeOption across the function signature and
all usages inside getDateTimeFormat, update any callers that pass that argument
to use the new identifier, and adjust any related tests or imports to reflect
the new parameter name so ESLint camelCase errors are resolved.
- Around line 4-34: Replace the duplicated implementations of getDuration,
getDateTimeFormat, and getUserCheckFormattedValue in the Status component with
imports from the extracted utilities file; import { getDuration,
getDateTimeFormat, getUserCheckFormattedValue } from
"client/utils/session-utils.js" at the top of status.js, remove the local
duplicate definitions (the functions currently present in status.js), and ensure
all local call sites within Status still reference the same function names so
they use the shared implementations.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 63b2fcba-e5e1-416f-9a42-4173fcc9740f
📒 Files selected for processing (1)
client/utils/session-utils.js
📜 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). (1)
- GitHub Check: Tests and Coverage
🧰 Additional context used
🪛 ESLint
client/utils/session-utils.js
[error] 15-15: Identifier 'time_option' is not in camel case.
(camelcase)
[error] 34-34: Newline required at end of file but not found.
(eol-last)
🔇 Additional comments (1)
client/utils/session-utils.js (1)
1-2: Verifyduration-formatter@1.1.1is properly configured as it cannot be found on the public npm registry.The
prettyBytesoptions (space,maximumFractionDigits) are valid forpretty-bytes@7.0.0and are actively used in the code. However,duration-formatter@1.1.1does not appear to exist in the public npm registry. Verify that this package is configured correctly—whether as a private npm package, monorepo package, or alternative source—and that thetimeFromSecondsexport is available as expected.
| import prettyBytes from "pretty-bytes"; | ||
| import {timeFromSeconds} from "duration-formatter"; | ||
|
|
||
| export const getDuration = (seconds) => { | ||
| const number = Number(seconds); | ||
| const h = Math.floor(number / 3600); | ||
| const m = Math.floor((number % 3600) / 60); | ||
| const s = Math.floor((number % 3600) % 60); | ||
| const hDisplay = h > 0 ? h + (h === 1 ? " hr " : " hrs ") : ""; | ||
| const mDisplay = m > 0 ? m + (m === 1 ? " min " : " mins ") : ""; | ||
| const sDisplay = s > 0 ? s + (s === 1 ? " sec " : " secs ") : ""; | ||
| return hDisplay + mDisplay + sDisplay; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider consolidating duration formatting approaches.
The file imports timeFromSeconds from duration-formatter (used in getUserCheckFormattedValue) but also defines a custom getDuration function. If both produce acceptable output formats, consider using one approach consistently to reduce redundancy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/utils/session-utils.js` around lines 1 - 13, The file defines a custom
getDuration(seconds) while also importing timeFromSeconds from
duration-formatter and using it elsewhere (e.g., getUserCheckFormattedValue);
consolidate to a single formatter by removing the custom getDuration and
replacing its usages with timeFromSeconds (or vice versa), ensuring the chosen
formatter's output matches existing UI expectations; update all references to
getDuration in this module (and any exports) to call timeFromSeconds with
appropriate arguments or transform its output to match the previous string
format.
| export const getDuration = (seconds) => { | ||
| const number = Number(seconds); | ||
| const h = Math.floor(number / 3600); | ||
| const m = Math.floor((number % 3600) / 60); | ||
| const s = Math.floor((number % 3600) % 60); | ||
| const hDisplay = h > 0 ? h + (h === 1 ? " hr " : " hrs ") : ""; | ||
| const mDisplay = m > 0 ? m + (m === 1 ? " min " : " mins ") : ""; | ||
| const sDisplay = s > 0 ? s + (s === 1 ? " sec " : " secs ") : ""; | ||
| return hDisplay + mDisplay + sDisplay; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Edge case: returns empty string for 0 or negative seconds.
When seconds is 0, all display conditions evaluate to false, resulting in an empty string. This matches the existing behavior in status.js, but consider whether returning "0 secs" would be more user-friendly for the zero case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/utils/session-utils.js` around lines 4 - 13, Update getDuration to
handle zero/negative inputs by returning a user-friendly string instead of an
empty string: inside the getDuration function, after converting seconds to
Number (variable number), add a check (e.g., if number <= 0) that returns "0
secs" for zero/negative values; otherwise keep the existing logic that computes
h, m, s and builds hDisplay/mDisplay/sDisplay so callers of getDuration receive
"0 secs" instead of an empty string.
| export const getDuration = (seconds) => { | ||
| const number = Number(seconds); | ||
| const h = Math.floor(number / 3600); | ||
| const m = Math.floor((number % 3600) / 60); | ||
| const s = Math.floor((number % 3600) % 60); | ||
| const hDisplay = h > 0 ? h + (h === 1 ? " hr " : " hrs ") : ""; | ||
| const mDisplay = m > 0 ? m + (m === 1 ? " min " : " mins ") : ""; | ||
| const sDisplay = s > 0 ? s + (s === 1 ? " sec " : " secs ") : ""; | ||
| return hDisplay + mDisplay + sDisplay; | ||
| }; | ||
|
|
||
| export const getDateTimeFormat = (language, time_option, date) => { | ||
| if (typeof Intl !== "undefined") { | ||
| return new Intl.DateTimeFormat(language, time_option).format(new Date(date)); | ||
| } | ||
| return String(new Date(date)); | ||
| }; | ||
|
|
||
| export const getUserCheckFormattedValue = (value, type) => { | ||
| const intValue = parseInt(value, 10); | ||
| switch (type) { | ||
| case "bytes": | ||
| return intValue === 0 | ||
| ? 0 | ||
| : prettyBytes(intValue, {space: true, maximumFractionDigits: 2}); | ||
| case "seconds": | ||
| return timeFromSeconds(intValue); | ||
| default: | ||
| return value; | ||
| } | ||
| }; No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if Status component imports the new session-utils
# Check for imports of session-utils in status.js
echo "=== Checking for session-utils imports in status.js ==="
rg -n "session-utils" client/components/status/status.js
# Check if the old method definitions still exist
echo -e "\n=== Checking if old getDuration method still exists ==="
rg -n "getDuration\s*=" client/components/status/status.js
echo -e "\n=== Checking if old getDateTimeFormat method still exists ==="
rg -n "getDateTimeFormat\s*=" client/components/status/status.js
echo -e "\n=== Checking if old getUserCheckFormattedValue method still exists ==="
rg -n "getUserCheckFormattedValue\s*=" client/components/status/status.jsRepository: openwisp/openwisp-wifi-login-pages
Length of output: 476
Update the Status component to use extracted utilities from session-utils.js
The getDuration, getDateTimeFormat, and getUserCheckFormattedValue methods are duplicated in status.js (lines 917, 929, 1192). Import and use the extracted utilities from session-utils.js to eliminate this duplication.
🧰 Tools
🪛 ESLint
[error] 15-15: Identifier 'time_option' is not in camel case.
(camelcase)
[error] 34-34: Newline required at end of file but not found.
(eol-last)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/utils/session-utils.js` around lines 4 - 34, Replace the duplicated
implementations of getDuration, getDateTimeFormat, and
getUserCheckFormattedValue in the Status component with imports from the
extracted utilities file; import { getDuration, getDateTimeFormat,
getUserCheckFormattedValue } from "client/utils/session-utils.js" at the top of
status.js, remove the local duplicate definitions (the functions currently
present in status.js), and ensure all local call sites within Status still
reference the same function names so they use the shared implementations.
Code Style FailuresHello @JarrarHaiderNemati,
|
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 `@client/utils/session-utils.js`:
- Around line 24-35: getUserCheckFormattedValue currently parses value to int
(intValue) but doesn't handle unparseable inputs (NaN); add a guard after
computing intValue (use Number.isNaN(intValue)) and for NaN return the original
value (or a safe fallback) instead of calling prettyBytes or timeFromSeconds so
those functions never receive NaN; update logic inside
getUserCheckFormattedValue to early-return on NaN before the switch or handle
NaN inside each "bytes" / "seconds" case referencing intValue, prettyBytes, and
timeFromSeconds.
- Around line 27-30: The "bytes" branch currently returns the number 0 when
intValue === 0, causing a type mismatch with non-zero results (strings); change
the "bytes" case to always return prettyBytes(intValue, {space: true,
maximumFractionDigits: 2}) (i.e., remove the ternary that returns 0) so both
zero and non-zero values produce a consistent string; update the case inside the
same switch handling formatting (the "case \"bytes\"" block) to use prettyBytes
for all values.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 221acb3b-e18c-493a-8926-2aae677b3708
📒 Files selected for processing (1)
client/utils/session-utils.js
📜 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). (1)
- GitHub Check: Tests and Coverage
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2026-03-06T23:26:11.526Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), real memory leaks are fixed by calling `clearTimeout(this.usageRetryTimeoutId)` and `window.removeEventListener("message", ...)` in `componentWillUnmount()`. These were actual leaks — the retry timeout could fire and the message handler could run after unmount.
Applied to files:
client/utils/session-utils.js
📚 Learning: 2026-03-06T23:26:11.526Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), `finalOperations()` has a single early return before setting up intervals/listeners to prevent the scenario where the component unmounts during async awaits. `safeSetState` is applied to success paths in `getUserRadiusSessions()`, `getUserRadiusUsage()`, `getPlansSuccessCallback()`, and `handlePostMessage()`.
Applied to files:
client/utils/session-utils.js
📚 Learning: 2026-03-06T23:26:11.526Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), the Status component uses `this.isComponentMounted` (set to `true` in the constructor, `false` in `componentWillUnmount`) together with a `safeSetState()` helper method that checks `isComponentMounted` before calling `setState()`. This is the preferred pattern to guard against "setState on unmounted component" warnings.
Applied to files:
client/utils/session-utils.js
📚 Learning: 2026-03-06T23:26:11.526Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-06T23:26:11.526Z
Learning: In client/components/status/status.js (openwisp-wifi-login-pages), catch blocks deliberately do NOT check `isComponentMounted` — console warnings from catch paths are considered harmless. Only real memory leaks (timers, event listeners) and "setState on unmounted" in success paths are fixed. The maintainer (nemesifier) explicitly wants to avoid over-engineering with defensive checks in every catch block.
Applied to files:
client/utils/session-utils.js
🪛 ESLint
client/utils/session-utils.js
[error] 15-15: Identifier 'time_option' is not in camel case.
(camelcase)
🪛 GitHub Check: QA-Checks
client/utils/session-utils.js
[failure] 15-15:
Identifier 'time_option' is not in camel case
🔇 Additional comments (3)
client/utils/session-utils.js (3)
15-22: Renametime_optiontotimeOptionto fix CI failure.The ESLint camelCase error is causing the GitHub Check to fail. This was noted in a previous review.
35-36: Add missing newline at end of file.ESLint requires a trailing newline. This was noted in a previous review.
4-13: LGTM for the extraction, with prior suggestions noted.The duration calculation logic is correct. Past review comments have already flagged the edge case for
0seconds returning an empty string and the potential consolidation withtimeFromSeconds. Those remain valid suggestions for follow-up.
| export const getUserCheckFormattedValue = (value, type) => { | ||
| const intValue = parseInt(value, 10); | ||
| switch (type) { | ||
| case "bytes": | ||
| return intValue === 0 | ||
| ? 0 | ||
| : prettyBytes(intValue, {space: true, maximumFractionDigits: 2}); | ||
| case "seconds": | ||
| return timeFromSeconds(intValue); | ||
| default: | ||
| return value; | ||
| } |
There was a problem hiding this comment.
Add validation for unparseable input.
If value cannot be parsed as an integer, parseInt(value, 10) returns NaN. Passing NaN to prettyBytes or timeFromSeconds may produce unexpected results or errors.
🛡️ Proposed fix with NaN guard
export const getUserCheckFormattedValue = (value, type) => {
const intValue = parseInt(value, 10);
+ if (Number.isNaN(intValue)) {
+ return value;
+ }
switch (type) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/utils/session-utils.js` around lines 24 - 35,
getUserCheckFormattedValue currently parses value to int (intValue) but doesn't
handle unparseable inputs (NaN); add a guard after computing intValue (use
Number.isNaN(intValue)) and for NaN return the original value (or a safe
fallback) instead of calling prettyBytes or timeFromSeconds so those functions
never receive NaN; update logic inside getUserCheckFormattedValue to
early-return on NaN before the switch or handle NaN inside each "bytes" /
"seconds" case referencing intValue, prettyBytes, and timeFromSeconds.
| case "bytes": | ||
| return intValue === 0 | ||
| ? 0 | ||
| : prettyBytes(intValue, {space: true, maximumFractionDigits: 2}); |
There was a problem hiding this comment.
Inconsistent return type for zero bytes.
The function returns the number 0 when intValue === 0, but returns a string (e.g., "1.5 kB") for non-zero values. This type inconsistency could cause subtle issues in UI rendering or comparisons.
prettyBytes(0, {space: true, maximumFractionDigits: 2}) returns "0 B", which would be more consistent.
🔧 Proposed fix
case "bytes":
- return intValue === 0
- ? 0
- : prettyBytes(intValue, {space: true, maximumFractionDigits: 2});
+ return prettyBytes(intValue, {space: true, maximumFractionDigits: 2});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/utils/session-utils.js` around lines 27 - 30, The "bytes" branch
currently returns the number 0 when intValue === 0, causing a type mismatch with
non-zero results (strings); change the "bytes" case to always return
prettyBytes(intValue, {space: true, maximumFractionDigits: 2}) (i.e., remove the
ternary that returns 0) so both zero and non-zero values produce a consistent
string; update the case inside the same switch handling formatting (the "case
\"bytes\"" block) to use prettyBytes for all values.
Test and Commit Message FailuresHello @JarrarHaiderNemati, Test Failure: Commit Message Failure: Fix:
Example: |
1 participant
@nemesifier
Extracted getDuration, getDateTimeFormat and getUserCheckFormattedValue from the Status component into a separate client/utils/session-utils.js utility file.
These are pure functions with no dependency on component state or lifecycle. Extracting them is a first step toward addressing #918 : separating concerns in the Status component to improve maintainability and testability. Each function can now be tested and reused independently.
Tests: All 91 status tests pass.
Am i on the right track ?