[change] Extracted storage logic from Status component #918#1070
Open
kunalverma2512 wants to merge 2 commits into
Open
[change] Extracted storage logic from Status component #918#1070kunalverma2512 wants to merge 2 commits into
kunalverma2512 wants to merge 2 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:
📝 WalkthroughWalkthroughThis pull request extracts captive-portal storage logic from the Status component into a new utility module Sequence Diagram(s)sequenceDiagram
participant Status
participant CaptivePortalStorage
participant Cookies
participant LocalStorage
Status->>CaptivePortalStorage: storeValue(captivePortalSyncAuth, key, value, cookies)
alt captivePortalSyncAuth falsy
CaptivePortalStorage-->>Status: return immediately
else
CaptivePortalStorage->>LocalStorage: localStorage.setItem(key, value)
CaptivePortalStorage->>Cookies: cookies.set(key, value, {path: "/", maxAge:60})
CaptivePortalStorage-->>Status: return
end
Status->>CaptivePortalStorage: resolveStoredValue(captivePortalSyncAuth, key, fallback, cookies)
alt captivePortalSyncAuth falsy
CaptivePortalStorage-->>Status: return fallback
else
CaptivePortalStorage->>Cookies: cookies.get(key)
alt cookie exists
CaptivePortalStorage->>LocalStorage: localStorage.removeItem(key)
CaptivePortalStorage-->>Status: return (cookieValue === true or "true")
else
CaptivePortalStorage->>LocalStorage: localStorage.getItem(key)
alt localStorage value exists
CaptivePortalStorage->>LocalStorage: localStorage.removeItem(key)
CaptivePortalStorage-->>Status: return (value === "true")
else
CaptivePortalStorage-->>Status: return fallback
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 1
🤖 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/captive-portal-storage.js`:
- Line 31: The JSDoc parameter name `cookieKey` mismatches the actual function
parameter `key`; update the JSDoc for the function in
client/utils/captive-portal-storage.js so the `@param` name matches the real
parameter (`key`) and its description, ensuring IDE/tooling show correct hints
for that function (replace `cookieKey` with `key` in the `@param` line and keep
the rest of the description unchanged).
🪄 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: 975726d1-72c4-4a96-86c6-05e5632cea31
📒 Files selected for processing (2)
client/components/status/status.jsclient/utils/captive-portal-storage.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 (5)
📓 Common learnings
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.
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()`.
📚 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/components/status/status.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/components/status/status.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), 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/components/status/status.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/components/status/status.js
🔇 Additional comments (6)
client/utils/captive-portal-storage.js (2)
18-24: Good extraction of write-path storage logic.The helper keeps the sync-auth gate and dual-write behavior intact, which makes the Status call sites cleaner without changing flow.
35-58: Read precedence and cleanup behavior look correct.Cookie-first resolution with localStorage fallback and post-read cleanup is consistent and easy to reason about.
client/components/status/status.js (4)
40-43: Import wiring is clean and localized.Good move to centralize captive-portal storage behavior in a utility module.
160-171:mustLogin/mustLogoutresolution refactor preserves flow.Using
resolveStoredValue(...)here keeps the previous decision path intact while reducing component complexity.
234-239: All write-path call sites were updated consistently.The
storeValue(...)replacement is applied in login, upgrade, and logout paths in a consistent way.Also applies to: 501-506, 589-594
706-711: Logout iframe read-path update is consistent with extracted helper usage.This keeps the sync-auth logout restoration logic centralized and maintainable.
Based on learnings, the established
client/components/status/status.jslifecycle/mounted-safety pattern should remain unchanged unless directly needed; this refactor does so.
Commit Message Format FailureHello @kunalverma2512, The commit message is missing the issue number in the body, even though it's present in the title. Fix: Example: |
kunalverma2512
force-pushed
the
refactor-status-storage-918
branch
2 times, most recently
from
5000bc4 to
8f988ba
Compare
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/captive-portal-storage.js`:
- Line 59: The file ends with the closing token "};" (the end of the exported
object/function) but lacks a trailing newline; open
client/utils/captive-portal-storage.js, move the cursor to the end after the
final "};" and add a single newline character so the file ends with a blank
line, satisfying ESLint's newline-at-end-of-file rule.
- Around line 46-56: The cookie and localStorage branches return inconsistent
types (cookieValue vs localStorageValue === "true"); update the logic so both
branches return the same type (preferably boolean). In the code handling cookies
(cookieValue from cookies.get(key)) and localStorage
(localStorage.getItem(key)), coerce both values to boolean consistently—e.g.,
for cookieValue check its type and treat string "true" as true otherwise use
Boolean(cookieValue), and for localStorageValue continue using localStorageValue
=== "true"—so callers of cookies.get(key)/localStorage.getItem(key) via this
helper always receive a boolean.
🪄 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: 47215e8f-6c58-4b5e-95c4-0405b56954b6
📒 Files selected for processing (2)
client/components/status/status.jsclient/utils/captive-portal-storage.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 (5)
📓 Common learnings
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.
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()`.
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.
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.
📚 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/components/status/status.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/components/status/status.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), 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/components/status/status.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/components/status/status.js
🪛 ESLint
client/utils/captive-portal-storage.js
[error] 59-59: Newline required at end of file but not found.
(eol-last)
🔇 Additional comments (6)
client/components/status/status.js (6)
40-43: LGTM!The import is well-structured and correctly brings in both utility functions from the new module.
160-171: LGTM!Both
resolveStoredValuecalls correctly pass the required parameters and maintain the original fallback logic usinguserMustLoginanduserMustLogoutfromuserData.
234-239: LGTM!The
storeValuecall correctly persistsmustLogin = falsebefore form submission, preventing duplicate logins on page reload in synchronous auth mode.
501-506: LGTM!The
storeValuecall correctly setsmustLogin = truebefore redirecting to payment, ensuring the user regains internet access upon return.
589-594: LGTM!The
storeValuecall correctly persistsmustLogout = truebefore form submission, ensuring the logout state is maintained across page reloads.
704-712: LGTM!The
resolveStoredValuecall correctly checks for the stored logout flag inhandleLogoutIframe, with an appropriate fallback offalse. The integration with the existingloggedOutstate check is preserved.
Prettier Code Style FailuresHello @kunalverma2512,
|
kunalverma2512
force-pushed
the
refactor-status-storage-918
branch
from
8f988ba to
d22afcb
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
client/utils/captive-portal-storage.js (1)
46-56:⚠️ Potential issue | 🟠 MajorNormalize
resolveStoredValuereturn type to boolean in both branches.Line 49 returns raw cookie value, while Line 55 returns a boolean. If cookie values are returned as strings (e.g.
"false"), callers may treat them as truthy and mis-handle login/logout flags.♻️ Proposed fix
const cookieValue = cookies.get(key); if (cookieValue !== undefined) { localStorage.removeItem(key); - return cookieValue; + return cookieValue === true || cookieValue === "true"; } const localStorageValue = localStorage.getItem(key); if (localStorageValue !== null) { localStorage.removeItem(key); return localStorageValue === "true"; }In react-cookie/universal-cookie, what value type does cookies.get("key") return when cookies.set("key", false) or cookies.set("key", true) is used?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/utils/captive-portal-storage.js` around lines 46 - 56, The cookie branch returns the raw cookie value (which may be the string "false") while the localStorage branch coerces to boolean; update resolveStoredValue to always return a boolean by coercing cookieValue to a boolean before returning: when cookies.get(key) yields a value, remove the localStorage key then convert the cookieValue to a boolean (e.g., treat the string "true" as true and everything else as false, or use a safe JSON/boolean coercion) so both branches consistently return boolean values; reference cookies.get, localStorage.getItem, localStorage.removeItem and resolveStoredValue to find where to apply the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@client/utils/captive-portal-storage.js`:
- Around line 46-56: The cookie branch returns the raw cookie value (which may
be the string "false") while the localStorage branch coerces to boolean; update
resolveStoredValue to always return a boolean by coercing cookieValue to a
boolean before returning: when cookies.get(key) yields a value, remove the
localStorage key then convert the cookieValue to a boolean (e.g., treat the
string "true" as true and everything else as false, or use a safe JSON/boolean
coercion) so both branches consistently return boolean values; reference
cookies.get, localStorage.getItem, localStorage.removeItem and
resolveStoredValue to find where to apply the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a2e686b2-b673-411c-9fa9-c5e50e937e3f
📒 Files selected for processing (2)
client/components/status/status.jsclient/utils/captive-portal-storage.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 (5)
📓 Common learnings
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.
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()`.
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.
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.
📚 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/components/status/status.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/components/status/status.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), 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/components/status/status.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/components/status/status.js
🔇 Additional comments (2)
client/components/status/status.js (1)
160-171: Call-site migration to storage helpers is consistent and behavior-preserving.The replacement of in-class storage calls with utility helpers is applied coherently across login/logout flows and stored-flag resolution points.
Also applies to: 234-239, 501-506, 589-594, 706-711
client/utils/captive-portal-storage.js (1)
18-24:storeValueimplementation is clean and correctly gated by sync-auth mode.Writes are properly skipped when synchronous captive-portal auth is disabled.
kunalverma2512
force-pushed
the
refactor-status-storage-918
branch
from
d22afcb to
8c0275c
Compare
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/captive-portal-storage.js`:
- Around line 36-59: Add unit tests for resolveStoredValue covering four
behaviors: (1) when captivePortalSyncAuth is false it returns the fallback; (2)
when cookies.get(key) exists it takes precedence over localStorage, coerces
boolean values (true and "true") and removes the localStorage entry; (3) when
cookies.get(key) is undefined but localStorage.getItem(key) returns "true" it
returns true and removes the localStorage key; and (4) when neither cookie nor
localStorage present it returns the fallback. In each test, stub/mock
cookies.get, localStorage.getItem, and localStorage.removeItem, call
resolveStoredValue(captivePortalSyncAuth, key, fallback, cookies) and assert the
returned boolean and that localStorage.removeItem was called when reading from
storage; include cases for cookie value true and "true" and ensure cookies takes
precedence over localStorage.
🪄 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: c6ca0bb4-e8a2-4ad1-82c4-488a184178d5
📒 Files selected for processing (2)
client/components/status/status.jsclient/utils/captive-portal-storage.js
📜 Review details
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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()`.
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.
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.
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.
📚 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/components/status/status.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/components/status/status.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), 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/components/status/status.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/components/status/status.js
🔇 Additional comments (2)
client/utils/captive-portal-storage.js (1)
18-24: Extraction is clean and preserves the intended storage resolution behavior.The helper split is solid: sync-auth gating, cookie-first precedence, and localStorage cleanup are all applied consistently.
Also applies to: 36-59
client/components/status/status.js (1)
40-43: Storage-helper migration is consistent across all touched flows.The new utility is wired correctly in login/logout/payment paths, and the key/fallback usage matches previous intent while reducing class-level duplication.
Also applies to: 160-171, 234-239, 501-506, 589-594, 706-711
kunalverma2512
force-pushed
the
refactor-status-storage-918
branch
from
8c0275c to
08372c5
Compare
|
Kilo Code Review could not run — your account is out of credits. Add credits at app.kilo.ai to enable reviews on this change. |
This commit extracts the storage logic from the Status component to improve modularity and testability. Closes openwisp#918
QA and Linting FailuresHello @kunalverma2512,
|
kunalverma2512
force-pushed
the
refactor-status-storage-918
branch
from
08372c5 to
c74d184
Compare
|
Hi @kunalverma2512 👋, This pull request has been marked as stale due to 50 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 10 more days, this PR will be automatically closed (but can be reopened anytime). |
Author
|
@nemesifier All checks have passed and the PR is approved. It is ready for merge. |
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
2 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.
Checklist
Reference to Existing Issue
Related to #918.
Description of Changes
This PR contributes to the ongoing refactoring of the Status component (#918). Following the "surgical" approach discussed in the issue, I have extracted the storage-related logic into a dedicated utility file.
Key Changes
client/utils/captive-portal-storage.jsto house thestoreValueandresolveStoredValuelogic.client/components/status/status.jsto import and use these utilities.Statuscomponent by approximately 50 lines.Verification
npm run setupto clear environment dependencies.npm run lint: Passed with zero errors.npm test: All 369 tests passed, verifying that the refactoring did not break existing behavior.npm run format.Screenshot
No UI changes were made as this is a pure codebase maintainability improvement.