[fix] Fixed indefinite loading on mixed content exception #602#1063
[fix] Fixed indefinite loading on mixed content exception #602#1063prathmeshkulkarni-coder wants to merge 7 commits into
Conversation
When the application is served via HTTPS but the portal API uses HTTP, the browser blocks the request. This fix catches the exception, clears the loading overlay, and notifies the user via a toast message. Fixes openwisp#602
|
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 new utility Sequence Diagram(s)sequenceDiagram
participant UI as Client UI
participant Status as Status Component
participant Check as checkMixedContent
participant Browser as Browser
participant Server as Server
UI->>Status: submitCaptivePortalForm(formRef)
Status->>Check: checkMixedContent(formRef.current.action)
alt Mixed Content detected (HTTPS page, http: action)
Check-->>Status: throw MixedContentError
Status->>UI: setLoading(false)
Status->>UI: toast.error("Security/Network Error: ...")
Status->>Status: console.error(error)
Status-->>UI: return false
else No mixed content
Status->>Browser: formRef.current.submit()
Browser->>Server: POST to actionUrl
Server-->>Browser: response
Browser-->>Status: navigation/response handling
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Test Failures and Linting ErrorsHello @prathmeshkulkarni-coder,
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/components/status/status.js (1)
933-952:⚠️ Potential issue | 🔴 CriticalCritical: Missing
returnin catch block causes loading state to be immediately re-enabled.After catching the mixed content error:
- Line 945 sets
setLoading(false)- Line 949 (outside the if block) sets
setLoading(true)— this overrides the fix- Lines 950-952 continue to make API calls
The user will still see an indefinite loading overlay, defeating the purpose of this fix.
Additionally, the error handling is inconsistent with the other two locations — missing the "Security/Network Error:" prefix and
console.errorlogging.Proposed fix
try { const formAction = new URL(this.logoutFormRef.current.action); if ( window.location.protocol === "https:" && formAction.protocol === "http:" ) { throw new Error( "Mixed Content: Cannot submit insecure HTTP form from a secure HTTPS page.", ); } this.logoutFormRef.current.submit(); } catch (error) { setLoading(false); - toast.error(error.message); + toast.error(`Security/Network Error: ${error.message}`); + console.error("Mixed Content Exception:", error); + return; } } setLoading(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/status/status.js` around lines 933 - 952, The catch block for the logout flow (around logoutFormRef.current.submit()) fails to stop further execution and re-enables loading; modify the catch to log the error with console.error, prefix the toast message with "Security/Network Error: " (consistent with other handlers), call setLoading(false) if not already, and then return immediately to prevent executing getUserPastRadiusSessions, getUserActiveRadiusSessions and the later setLoading(true); ensure the unique symbols referenced are logoutFormRef, setLoading, toast.error, getUserPastRadiusSessions and getUserActiveRadiusSessions so the early-return and logging are applied in the same catch scope where the mixed-content error is thrown.
🤖 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 237-252: Extract the repeated mixed-content validation into a
single helper method (e.g., submitFormWithMixedContentCheck(form)) that accepts
an HTMLFormElement, performs the URL/protocol check (throwing the existing Error
message when page is HTTPS but form.action is HTTP), and calls form.submit() on
success; then replace the duplicated blocks in componentDidMount, handleLogout,
and handleSessionLogout to call this helper with this.loginFormRef.current and
keep the existing try/catch behavior (this.context.setLoading(false),
toast.error(...), console.error(...)) at each call site for consistent error
handling.
- Line 618: Replace the direct property call this.context.setLoading(false) with
the already-destructured setLoading(false) to use the local variable from the
earlier destructuring; update the call in the method where setLoading was
extracted so it calls setLoading(false) instead of
this.context.setLoading(false).
- Line 249: Replace the direct usage of this.context.setLoading with the already
destructured setLoading variable to satisfy the lint rule and maintain
consistency; locate where setLoading was destructured (earlier in the same
module) and change the call this.context.setLoading(false) to setLoading(false)
(ensure there are no naming conflicts and that setLoading is in scope where it's
used).
---
Outside diff comments:
In `@client/components/status/status.js`:
- Around line 933-952: The catch block for the logout flow (around
logoutFormRef.current.submit()) fails to stop further execution and re-enables
loading; modify the catch to log the error with console.error, prefix the toast
message with "Security/Network Error: " (consistent with other handlers), call
setLoading(false) if not already, and then return immediately to prevent
executing getUserPastRadiusSessions, getUserActiveRadiusSessions and the later
setLoading(true); ensure the unique symbols referenced are logoutFormRef,
setLoading, toast.error, getUserPastRadiusSessions and
getUserActiveRadiusSessions so the early-return and logging are applied in the
same catch scope where the mixed-content error is thrown.
🪄 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: ae2df772-856b-438b-ac39-c4267a2039a1
📒 Files selected for processing (1)
client/components/status/status.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), 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.
📚 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), 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
📚 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
🪛 GitHub Check: QA-Checks
client/components/status/status.js
[failure] 249-249:
Must use destructuring context assignment
[failure] 618-618:
Must use destructuring context assignment
Test Failures in Status ComponentHello @prathmeshkulkarni-coder, The tests for the Specifically, the following tests are failing:
To fix these failures, you will need to examine the logic within the |
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/components/status/status.js`:
- Around line 929-935: The catch block for the logout flow should match other
sites: add a console.error call with a label (e.g., console.error("Mixed Content
Exception:", error)) for debugging and change the toast message to include the
same prefix ("Security/Network Error: " + error.message) before calling
setLoading(false) and toast.error; locate the try/catch around
checkMixedContent(this.logoutFormRef.current.action) /
this.logoutFormRef.current.submit() and update the catch to log the error and
prepend the "Security/Network Error:" prefix when calling toast.error to ensure
consistency with the other handlers.
🪄 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: db45f045-365d-4009-8b76-a58040b7b72a
📒 Files selected for processing (1)
client/components/status/status.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), `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), 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
📚 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
🔇 Additional comments (3)
client/components/status/status.js (3)
41-51: LGTM! Helper correctly detects mixed content.The helper function properly identifies the HTTPS→HTTP mixed content scenario that browsers block. One edge case to note: if
actionUrlis empty or invalid,new URL(actionUrl)will throw aTypeError, but this is acceptable since the surrounding try-catch will still handle it gracefully and an empty action URL indicates a configuration error that should surface.
249-256: LGTM! Properly guards login form submission.The try-catch correctly prevents the indefinite loading state by calling
setLoading(false)when mixed content is detected, and provides clear user feedback via toast notification.
610-617: LGTM! Consistent error handling for logout form.The pattern matches the login form handling, ensuring the loading state is properly cleared and the user receives feedback on mixed content errors.
prathmeshkulkarni-coder
force-pushed
the
fix/602-mixed-content-exception
branch
from
129bbbd to
afdf213
Compare
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 (1)
client/components/status/status.js (1)
930-943:⚠️ Potential issue | 🟠 MajorControl flow bug:
setLoading(true)at line 939 executes after error handling.When a mixed content error is caught,
setLoading(false)is called at line 934, but thensetLoading(true)at line 939 executes unconditionally afterward, re-enabling the loading spinner and negating the fix.The
setLoading(true)and subsequent session fetching should either be moved inside the try block (before the check) or the catch block should return early.🐛 Proposed fix: return early on error
try { checkMixedContent(this.logoutFormRef.current.action); this.logoutFormRef.current.submit(); } catch (error) { setLoading(false); toast.error(`Security/Network Error: ${error.message}`); console.error("Mixed Content Exception:", error); + return; } } setLoading(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/status/status.js` around lines 930 - 943, The catch block for the mixed content check currently calls setLoading(false) but execution continues and unconditionally calls setLoading(true) and session fetching; update the control flow so that when checkMixedContent throws you return early from the function (i.e., add a return in the catch) or move setLoading(true) and the calls to getUserPastRadiusSessions and getUserActiveRadiusSessions inside the try block after the check; locate the logic around checkMixedContent(this.logoutFormRef.current.action), the catch that calls setLoading(false), and the later setLoading(true)/getUserPastRadiusSessions/getUserActiveRadiusSessions calls and ensure you either return from the catch or relocate the setLoading(true)+session fetches so they only run on success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@client/components/status/status.js`:
- Around line 930-943: The catch block for the mixed content check currently
calls setLoading(false) but execution continues and unconditionally calls
setLoading(true) and session fetching; update the control flow so that when
checkMixedContent throws you return early from the function (i.e., add a return
in the catch) or move setLoading(true) and the calls to
getUserPastRadiusSessions and getUserActiveRadiusSessions inside the try block
after the check; locate the logic around
checkMixedContent(this.logoutFormRef.current.action), the catch that calls
setLoading(false), and the later
setLoading(true)/getUserPastRadiusSessions/getUserActiveRadiusSessions calls and
ensure you either return from the catch or relocate the setLoading(true)+session
fetches so they only run on success.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 184fc1ce-e79d-45e4-84db-4f4c902c4d90
📒 Files selected for processing (1)
client/components/status/status.js
📜 Review details
🧰 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), `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), 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
📚 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
🔇 Additional comments (3)
client/components/status/status.js (3)
41-52: LGTM! Clean defensive implementation.The function correctly validates mixed content scenarios with appropriate guards for falsy or non-string inputs. The
startsWith("http:")approach is simple and sufficient since form action URLs from the DOM are browser-normalized.
250-257: LGTM! Proper error handling for mixed content detection.The try-catch correctly wraps the form submission, and the error handling appropriately clears the loading state and provides user feedback via toast notification.
611-618: LGTM! Consistent error handling with other call sites.The error handling pattern matches the componentDidMount implementation, maintaining consistency across the codebase.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
- Extracted protocol validation into submitWithMixedContentCheck helper. - Used destructured setLoading to satisfy 'Must use destructuring' rule. - Fixed duplicated logic in componentDidMount, handleLogout, and handleSessionLogout. Addresses review feedback for openwisp#602
prathmeshkulkarni-coder
force-pushed
the
fix/602-mixed-content-exception
branch
from
afdf213 to
beb9d7c
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Regression test is missing.
Commit Message Format FailureHello @prathmeshkulkarni-coder, The commit message is not in the correct format. The issue number must be present in the commit title. Fix: Example of correct format: |
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/components/status/status.js`:
- Line 264: The mixed-content guard checkMixedContent defined in status.js must
be extracted to a shared utility (e.g., create
client/utils/checkMixedContent.js) and then imported where used; update
submitCaptivePortalForm (in status.js) to call the new shared check and modify
the radius realms submission flow in login.js (the function that currently
submits the radius_realms form directly) to either call the shared
checkMixedContent before submitting or use the same wrapper used by
submitCaptivePortalForm so HTTPS pages cannot post to HTTP actions; ensure you
update imports in both client/components/status/status.js and
client/components/login/login.js and keep the original check logic and
return/early-exit behavior intact.
In `@client/components/status/status.test.js`:
- Around line 2890-2916: The test currently triggers the catch via a mocked
submit() error instead of exercising the mixed-content check; update the test
for submitCaptivePortalForm so checkMixedContent actually runs by setting
window.location.protocol = "https:" (or mocking it) and ensuring the
mockFormRef.current.action is "http://..." so checkMixedContent throws, then
call wrapper.instance().submitCaptivePortalForm(mockFormRef) and assert the same
catch behavior, and finally restore the original window.location.protocol after
the test; refer to submitCaptivePortalForm and checkMixedContent to locate where
to adjust the test setup.
🪄 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: 4185d8a1-200d-4411-ab41-ad5b0c264f02
📒 Files selected for processing (2)
client/components/status/status.jsclient/components/status/status.test.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), 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), `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), 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.jsclient/components/status/status.test.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
🔇 Additional comments (4)
client/components/status/status.js (4)
41-52: LGTM!The
checkMixedContentfunction correctly detects mixed content by checking if the page is served over HTTPS while the form action URL uses HTTP. The guard conditions (actionUrl &&,typeof actionUrl === "string") properly handle edge cases.
106-118: LGTM!The helper method consolidates mixed-content handling with proper error recovery: resets loading state, shows user-friendly toast, logs for debugging, and returns a boolean for callers that need to know the outcome. This addresses the duplicate code concern from past reviews.
618-618: LGTM!Correctly uses the new wrapper for logout form submission.
930-932: LGTM!Good use of the return value to exit early if the mixed-content check fails, preventing the subsequent loading state and session fetching logic from executing.
prathmeshkulkarni-coder
force-pushed
the
fix/602-mixed-content-exception
branch
from
78cc226 to
b3f2e8a
Compare
QA and Test FailuresHello @prathmeshkulkarni-coder,
|
prathmeshkulkarni-coder
force-pushed
the
fix/602-mixed-content-exception
branch
from
b3f2e8a to
41016d1
Compare
ESLint Unused Variable FailureHello @prathmeshkulkarni-coder,
|
prathmeshkulkarni-coder
force-pushed
the
fix/602-mixed-content-exception
branch
2 times, most recently
from
4f05a9a to
ea68fa5
Compare
|
@nemesifier I added test case and some coderabbit review . |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
prathmeshkulkarni-coder
force-pushed
the
fix/602-mixed-content-exception
branch
from
91eb275 to
29c2bbc
Compare
Browser Test TimeoutHello @prathmeshkulkarni-coder, The CI job failed because the browser test timed out while waiting for Action: This appears to be a transient infrastructure issue. The CI has been automatically restarted. If the issue persists, please investigate the application's startup logs for any errors. |
…nwisp#602 - Consolidated duplicated error handling into submitWithMixedContentCheck helper. - Added a regression test case to verify loading state reset and toast notification. - Fixed linting by using destructured setLoading throughout the status component. Fixes openwisp#602
prathmeshkulkarni-coder
force-pushed
the
fix/602-mixed-content-exception
branch
from
5e6d8a9 to
4617ce2
Compare
3 participants
When the application is served via HTTPS but the portal API uses HTTP, the browser blocks the request. This fix catches the exception, clears the loading overlay, and notifies the user via a toast message.
Fixes #602
Checklist
Reference to Existing Issue
Closes #602
Description of Changes
The Issue: When the application is served over HTTPS but the captive portal endpoints are served via HTTP, the browser's Security Policy blocks the insecure form submission. Previously, because form.submit() is a native browser navigation action (a "fire and forget" mechanism that doesn't return a Promise), it swallowed the Mixed Content network exception. Because the error wasn't caught by the try/catch block, the setLoading(false) was never triggered, leaving the user with an indefinite loading overlay and no error feedback.
The Solution: Instead of replacing the native submit() with an AJAX fetch() (which would break the required iframe onLoad handling and duplicate the captive portal authentication payloads), this PR implements a surgical URL Protocol Validator immediately before the form submission is triggered.
This ensures the user is accurately notified of the security exception while keeping the legacy Captive Portal iframe injection flows perfectly intact.
Screenshot
Please include any relevant screenshots.