[change] Fix header duplication #314#1068
Conversation
…penwisp#918 Refactored the componentDidMount logic in the status component to improve readability and simplify session handling flow. Cleared redundant logic to make the lifecycle flow clearer. Fixes openwisp#918
Reduced duplicated logic between desktop and mobile headers. Fixes openwisp#314
📝 WalkthroughWalkthroughAdded a helper method Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (2 passed)
✨ 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 |
ESLint and Test FailuresHello @kankaa16,
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/components/header/header.js (1)
162-192: 🧹 Nitpick | 🔵 TrivialConsider using
renderLinkItemfor desktop links to complete the deduplication.The PR objective is to eliminate header HTML duplication, but desktop links still use inline rendering while mobile uses the new
renderLinkItemhelper. Once the active class logic is added torenderLinkItem, you can replace this block with:{links && links.map((link, index) => this.renderLinkItem(link, index, false), )}This would fully achieve the stated goal of consolidating desktop and mobile header logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/header/header.js` around lines 162 - 192, Desktop header link rendering duplicates mobile logic instead of using the existing renderLinkItem helper; update renderLinkItem to handle the desktop-specific "active" class (use pathname and orgSlug to compare to link.url with "{orgSlug}" replaced) and respect shouldLinkBeShown/isInternalLink/internalLinks logic, then replace the inline desktop mapping block with: {links && links.map((link, index) => this.renderLinkItem(link, index, false))} so both mobile and desktop use the same renderLinkItem path (refer to renderLinkItem, shouldLinkBeShown, isInternalLink, internalLinks, getText, pathname, orgSlug).
🤖 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/header/header.js`:
- Around line 63-97: renderLinkItem is missing the active-state logic and leaves
pathname unused, causing mobile links to lose the "active" class and triggering
an ESLint unused-variable error; update renderLinkItem (the method on the
component) to compute const resolvedUrl = link.url.replace("{orgSlug}", orgSlug)
and include the same active class logic used by the desktop link (append "
active" when pathname === resolvedUrl) in the className for both the <Link> and
<a> branches, use resolvedUrl for the to/href props, and add a blank line before
the renderLinkItem method to satisfy the lines-between-class-members rule.
In `@client/components/status/status.js`:
- Around line 117-121: The current code in status.js clears the
`${orgSlug}_macaddr` cookie whenever the URL lacks the macaddr param, which
loses the persisted NAS mapping used by handleLogout() and
logoutIfCurrentRadiusSessionIsInactive(); change the logic in the macaddr
handling block to only set/overwrite the cookie when a new macaddr value is
present and do not remove the cookie when macaddr is absent (i.e., drop the
cookies.remove(...) branch); rely on the existing logout flow in
client/actions/logout.js to clear the cookie on explicit logout.
- Around line 193-205: prepareUserInfo currently only schedules setState and
returns immediately, so awaiting it doesn't guarantee state is committed; change
prepareUserInfo to return a Promise that resolves in the setState callback (use
safeSetState where appropriate) and have it return a boolean indicating whether
the user is active; then update callers (e.g., handleUserAuthentication path
that does await this.prepareUserInfo()) to await the result and short-circuit
when it returns false (so do not proceed to handleLoginFlow or queued
handleLogout). Reference prepareUserInfo, safeSetState,
handleUserAuthentication, handleLogout, and handleLoginFlow when making these
changes.
- Around line 605-611: The code rereads React state immediately after awaiting
getUserActiveRadiusSessions, which only schedules setStateSafe, so use the
fetched result instead of this.state to avoid stale data; update
getUserActiveRadiusSessions/getUserRadiusSessions to return the fetched rows
(ensure it returns response.data after calling this.setStateSafe) and then in
the caller replace reads of this.state.sessionsToLogout /
this.state.activeSessions with the returned payload (e.g., use the returned
sessionsToLogout or activeSessions from the awaited result) so the logout path
uses the freshly fetched rows.
---
Outside diff comments:
In `@client/components/header/header.js`:
- Around line 162-192: Desktop header link rendering duplicates mobile logic
instead of using the existing renderLinkItem helper; update renderLinkItem to
handle the desktop-specific "active" class (use pathname and orgSlug to compare
to link.url with "{orgSlug}" replaced) and respect
shouldLinkBeShown/isInternalLink/internalLinks logic, then replace the inline
desktop mapping block with: {links && links.map((link, index) =>
this.renderLinkItem(link, index, false))} so both mobile and desktop use the
same renderLinkItem path (refer to renderLinkItem, shouldLinkBeShown,
isInternalLink, internalLinks, getText, pathname, orgSlug).
🪄 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: b0a74f34-f437-4b53-9ea6-d68289ec3a66
⛔ Files ignored due to path filters (1)
client/components/header/__snapshots__/header.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
client/components/header/header.jsclient/components/header/index.cssclient/components/status/status.jsconfig/__tests__/add-org.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
📓 Path-based instructions (1)
**/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go}
📄 CodeRabbit inference engine (Custom checks)
**/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go}: Bug Fixes: Tests must be deterministic and not flaky, avoiding dependencies on timing, sleeps, specific timezones, system time, randomness without fixed seeds, race conditions, concurrency timing, network access, external services, filesystem state, environment-specific configuration, execution order, shared global state, hardcoded ports, or unawaited async operations
Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Features: Tests must be added with coverage that does not decrease significantly; for UI features, Selenium browser tests are preferred
General: Ensure tests cover the happy path, error paths, boundary values, and unusual inputs
Files:
config/__tests__/add-org.test.js
🧠 Learnings (7)
📓 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: prathmeshkulkarni-coder
Repo: openwisp/openwisp-wifi-login-pages PR: 1063
File: client/components/status/status.test.js:2890-2916
Timestamp: 2026-03-18T17:34:17.347Z
Learning: In openwisp/openwisp-wifi-login-pages, JSDOM marks `window.location` as non-configurable (configurable: false) in the Jest/Enzyme test environment used by this project. Object.defineProperty on window.location throws a TypeError. Hacks like `delete window.location` cause state leaks across sequential tests. The maintainer (prathmeshkulkarni-coder) considers the window.location mocking approach infeasible and plans to migrate detection tests to RTL instead.
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : Changes: Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior
Applied to files:
config/__tests__/add-org.test.js
📚 Learning: 2026-03-19T22:44:00.665Z
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Applies to **/*.{test,spec}.{js,ts,jsx,tsx,py,java,rb,go} : Bug Fixes: Tests must be deterministic and not flaky, avoiding dependencies on timing, sleeps, specific timezones, system time, randomness without fixed seeds, race conditions, concurrency timing, network access, external services, filesystem state, environment-specific configuration, execution order, shared global state, hardcoded ports, or unawaited async operations
Applied to files:
config/__tests__/add-org.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), `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/components/header/header.js
[error] 63-97: Expected blank line between class members.
(lines-between-class-members)
[error] 66-66: 'pathname' is assigned a value but never used.
(no-unused-vars)
client/components/status/status.js
[error] 610-610: Must use destructuring state assignment
(react/destructuring-assignment)
🪛 GitHub Check: QA-Checks
client/components/header/header.js
[failure] 66-66:
'pathname' is assigned a value but never used
[failure] 63-63:
Expected blank line between class members
client/components/status/status.js
[failure] 610-610:
Must use destructuring state assignment
🔇 Additional comments (4)
config/__tests__/add-org.test.js (2)
8-9: Good extraction of CLI script path into a single constant.This avoids repeated inline path construction and keeps command invocations consistent.
43-46: Directnodeinvocation improves test reliability and consistency.Switching all
spawnSynccalls tonode <resolved-script>removes package-manager indirection and keeps behavior stable across environments.Also applies to: 62-64, 73-75, 81-83, 91-92
client/components/header/index.css (1)
182-192: LGTM!The responsive utility classes are well-implemented with consistent breakpoints (767px/768px boundary) matching the existing media queries. The
!importantis appropriate for utility classes that need to reliably override component styles.client/components/header/header.js (1)
240-243: LGTM on the refactoring approach.The delegation to
renderLinkItemis clean and correctly passesisMobile = trueto apply mobile-specific styling. The consolidation pattern is good — just needs the active class fix in the helper method.
| if (macaddr) { | ||
| cookies.set(`${orgSlug}_macaddr`, macaddr, {path: "/"}); | ||
| } else { | ||
| cookies.remove(`${orgSlug}_macaddr`, {path: "/"}); | ||
| } |
There was a problem hiding this comment.
Don't clear the persisted MAC cookie on query-less status-page loads.
This branch removes ${orgSlug}_macaddr whenever the URL lacks macaddr_param_name, but that cookie is still used later by handleLogout() and logoutIfCurrentRadiusSessionIsInactive() to identify the current NAS session, and client/actions/logout.js already clears it on real logout. A normal reload/direct visit will therefore lose the current-session mapping and can break auto-logout or target the wrong session. Only overwrite the cookie when a new MAC is actually supplied.
Suggested fix
- if (macaddr) {
- cookies.set(`${orgSlug}_macaddr`, macaddr, {path: "/"});
- } else {
- cookies.remove(`${orgSlug}_macaddr`, {path: "/"});
- }
+ if (macaddr) {
+ cookies.set(`${orgSlug}_macaddr`, macaddr, {path: "/"});
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (macaddr) { | |
| cookies.set(`${orgSlug}_macaddr`, macaddr, {path: "/"}); | |
| } else { | |
| cookies.remove(`${orgSlug}_macaddr`, {path: "/"}); | |
| } | |
| if (macaddr) { | |
| cookies.set(`${orgSlug}_macaddr`, macaddr, {path: "/"}); | |
| } |
🤖 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 117 - 121, The current code
in status.js clears the `${orgSlug}_macaddr` cookie whenever the URL lacks the
macaddr param, which loses the persisted NAS mapping used by handleLogout() and
logoutIfCurrentRadiusSessionIsInactive(); change the logic in the macaddr
handling block to only set/overwrite the cookie when a new macaddr value is
present and do not remove the cookie when macaddr is absent (i.e., drop the
cookies.remove(...) branch); rely on the existing logout flow in
client/actions/logout.js to clear the cookie on explicit logout.
| await this.getUserActiveRadiusSessions(params); | ||
| const {sessionsToLogout} = this.state; | ||
|
|
||
| let {sessionsToLogout} = this.state; | ||
|
|
||
| if (!sessionsToLogout || sessionsToLogout.length === 0) { | ||
| sessionsToLogout = this.state.activeSessions || []; | ||
| } |
There was a problem hiding this comment.
This fallback still rereads stale session state right after the fetch.
getUserRadiusSessions() only schedules setStateSafe(options), so after Line 605 the arrays read here can still be the pre-request values. In that case this branch falls through to the local logout path and never submits the captive-portal logout form, leaving the NAS session alive. Use the fetched rows directly instead of rereading React state here; that also removes the current react/destructuring-assignment failure on Line 610.
Suggested fix
- await this.getUserActiveRadiusSessions(params);
-
- let {sessionsToLogout} = this.state;
- if (!sessionsToLogout || sessionsToLogout.length === 0) {
- sessionsToLogout = this.state.activeSessions || [];
- }
+ const fetchedActiveSessions = await this.getUserActiveRadiusSessions(params);
+
+ let {sessionsToLogout} = this.state;
+ if (!sessionsToLogout || sessionsToLogout.length === 0) {
+ sessionsToLogout = fetchedActiveSessions || [];
+ }async getUserRadiusSessions(params) {
// ...
this.setStateSafe(options);
return response.data;
}🧰 Tools
🪛 ESLint
[error] 610-610: Must use destructuring state assignment
(react/destructuring-assignment)
🪛 GitHub Check: QA-Checks
[failure] 610-610:
Must use destructuring state assignment
🤖 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 605 - 611, The code rereads
React state immediately after awaiting getUserActiveRadiusSessions, which only
schedules setStateSafe, so use the fetched result instead of this.state to avoid
stale data; update getUserActiveRadiusSessions/getUserRadiusSessions to return
the fetched rows (ensure it returns response.data after calling
this.setStateSafe) and then in the caller replace reads of
this.state.sessionsToLogout / this.state.activeSessions with the returned
payload (e.g., use the returned sessionsToLogout or activeSessions from the
awaited result) so the logout path uses the freshly fetched rows.
Reduced duplicated logic between desktop and mobile headers. Fixes openwisp#314
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/header/header.js (1)
167-197: 🛠️ Refactor suggestion | 🟠 MajorDesktop section still duplicates the logic that
renderLinkItemencapsulates.The PR objective is to eliminate header duplication, but the desktop section retains inline mapping with the same
shouldLinkBeShown,isInternalLink, and conditional<Link>/<a>logic thatrenderLinkItemalready handles. Consider using the helper here as well.♻️ Proposed refactor to fully eliminate duplication
<div className="header-row-2"> <div className="header-row-2-inner"> - {links && - links.map((link, index) => { - if (!shouldLinkBeShown(link, isAuthenticated, userData)) { - return null; - } - if ( - isInternalLink(link.url) && - (internalLinks.indexOf(link.url) < 0 || !isAuthenticated) - ) { - return ( - <Link - className={`header-link header-desktop-link header-link-${index + 1} ${pathname === link.url.replace("{orgSlug}", orgSlug) ? "active" : ""} button`} - to={link.url.replace("{orgSlug}", orgSlug)} - key={index} - > - {getText(link.text, language)} - </Link> - ); - } - return ( - <a - href={link.url} - className={`header-link header-desktop-link header-link-${index + 1} button`} - target="_blank" - rel="noreferrer noopener" - key={link.url} - > - {getText(link.text, language)} - </a> - ); - })} + {links && + links.map((link, index) => + this.renderLinkItem(link, index, false), + )} </div> </div>This also allows removing the now-unused
internalLinksandpathnamevariables declared at lines 116-117 withinrender(), since they would only be used insiderenderLinkItem.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/header/header.js` around lines 167 - 197, The desktop mapping duplicates renderLinkItem's logic; replace the inline links.map callback in the desktop section with calls to the existing renderLinkItem(link, index) helper (ensuring it receives the same context: isAuthenticated, userData, orgSlug, language) so the conditional shouldLinkBeShown, isInternalLink and Link/<a> branching is centralized in renderLinkItem; after switching to renderLinkItem, remove the now-unused internalLinks and pathname variables declared earlier in render() to eliminate dead code.
🤖 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/header/header.js`:
- Around line 167-197: The desktop mapping duplicates renderLinkItem's logic;
replace the inline links.map callback in the desktop section with calls to the
existing renderLinkItem(link, index) helper (ensuring it receives the same
context: isAuthenticated, userData, orgSlug, language) so the conditional
shouldLinkBeShown, isInternalLink and Link/<a> branching is centralized in
renderLinkItem; after switching to renderLinkItem, remove the now-unused
internalLinks and pathname variables declared earlier in render() to eliminate
dead code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2b5eb5ea-7545-417d-a0e3-d3a1d3a104e2
📒 Files selected for processing (2)
client/components/header/header.jsclient/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), 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), 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
🔇 Additional comments (3)
client/components/header/header.js (2)
64-101: LGTM! Past review issues are addressed.The
renderLinkItemhelper correctly:
- Uses
pathnameto computeisActive(line 73)- Applies the
activeclass to internal<Link>elements (line 81)- Includes the required blank line before the method (line 63)
External
<a>elements correctly omit the active class since external URLs won't matchlocation.pathname.
246-248: LGTM!Clean delegation to the helper method with
isMobile = true, properly applying the mobile-specific class variant.client/components/status/status.js (1)
748-767: Defensive origin validation looks good.The changes correctly harden
handlePostMessage:
event.data || {}prevents destructuring errors on null/undefined data- Try/catch with optional chaining (
action?.trim()) safely handles missing, empty, or malformed action URLs- When
actionOriginisnull, the check correctly falls through towindow.location.originonlyThis pattern aligns with the similar origin-checking approach in
payment-process.js(lines 57-62) while being more defensive with the try/catch wrapper.
Reduced duplicated logic between desktop and mobile headers. Fixes openwisp#314
Reduced duplicated logic between desktop and mobile headers. Fixes openwisp#314
Reduced duplicated logic between desktop and mobile headers. Fixes openwisp#314
ESLint Unused Variables FailureHello @kankaa16, The following variables are assigned a value but never used in
Fix: Remove these unused variables or use them in the component. |
kankaa16
force-pushed
the
fix-header-duplication
branch
from
45ce2b9 to
09d2ecc
Compare
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/header/header.js`:
- Around line 76-77: The route check and href rendering are using link.url
instead of the resolved path, which lets templated routes like {orgSlug} bypass
internalLinks rules and leak into rendered hrefs; update the logic in the header
component so you compute resolvedUrl (the templated path) once and use it
everywhere: replace uses of link.url in the internalLinks.indexOf(...) check and
in the anchor href with resolvedUrl, keeping the isInternalLink(resolvedUrl) and
isAuthenticated conditions intact so templated internal links are correctly
recognized and rendered.
🪄 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: 6d156efa-0180-4913-9aeb-29bbe2a3b50e
📒 Files selected for processing (1)
client/components/header/header.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 comments (1)
client/components/header/header.js (1)
166-168: Good refactor: shared link rendering removes duplicated branching cleanly.Both desktop and mobile now call the same rendering path, which improves maintainability and keeps behavior aligned.
Also applies to: 217-219
1 participant
Checklist
Reference to Existing Issue
Related to #314
Description of Changes
Reduced duplicated logic between desktop and mobile headers.
No functional changes.