[change] Eliminate redundancy of header HTML #314#1080
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:
📝 WalkthroughWalkthroughThe header component was refactored to remove duplicated desktop/mobile markup: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
kunalverma2512
force-pushed
the
eliminate-redundancy-of-header-html
branch
from
36e1be5 to
d6ef743
Compare
Test and Commit Message FailuresHello @kunalverma2512,
|
kunalverma2512
force-pushed
the
eliminate-redundancy-of-header-html
branch
from
d6ef743 to
7858dfd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/header/header.js`:
- Around line 80-91: The header language button markup (generated by
languages.map in languageButtons and the onClick setLanguage handler) no longer
includes the header-desktop-language-btn class; update the corresponding browser
tests to use the new selector shape by removing header-desktop-language-btn from
the button selectors (i.e., match button.header-language-btn and the specific
header-language-btn-<slug> classes such as header-language-btn-it and
header-language-btn-en) so the tests target the current DOM structure.
In `@client/components/header/header.test.js`:
- Around line 108-109: The test incorrectly expects isInternalLink to be called
three times even though props.header.links contains only one link; update the
expectation in the test to expect one call instead of three (change the
isInternalLink toHaveBeenCalledTimes assertion from 3 to 1) — this corresponds
to the single call performed inside the links.map() in the Header component
where isInternalLink is invoked for each entry in props.header.links.
🪄 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: 4b7bcb2a-4f7e-44cd-b1db-4b79f1c311c7
📒 Files selected for processing (3)
client/components/header/header.jsclient/components/header/header.test.jsclient/components/header/index.css
📜 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:
client/components/header/header.test.js
🧠 Learnings (4)
📚 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:
client/components/header/header.test.js
📚 Learning: 2026-03-25T00:22:16.841Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-25T00:22:16.841Z
Learning: In `openwisp/openwisp-wifi-login-pages` browser tests (e.g., `browser-test/mobile-verfication.test.js`), fixed delay waits (e.g., `driver.sleep()`) are intentionally used instead of `driver.wait()` for toast message animations. This is because the JS animation on toast messages does not expose a reliable DOM state to poll against, and testing confirmed that fixed waits are more robust in this context. Do not flag these as flaky-by-design without considering this justification.
Applied to files:
client/components/header/header.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:
client/components/header/header.test.js
📚 Learning: 2026-03-18T17:34:09.903Z
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:09.903Z
Learning: Do not rely on mocking or altering window.location in Jest/JSDOM for this project. Since window.location is non-configurable in the test environment and attempts to redefine or delete it can cause state leaks across tests, avoid hacks like delete window.location or Object.defineProperty(window, 'location', ...) in tests. Instead, migrate detection-related tests to React Testing Library (RTL) and use RTL-friendly patterns (e.g., render components and inspect behavior via UI, or mock network/detection logic with explicit dependency injection or module mocks). This pattern should apply to all test files under client/components (any .test.js) where window.location might be interacted with.
Applied to files:
client/components/header/header.test.js
🔇 Additional comments (7)
client/components/header/header.js (2)
93-128: Clean consolidation of nav links rendering.The unified
navLinkscomputation correctly handles:
shouldLinkBeShownfor auth/verification checksisInternalLinkfor routing decisions- Placeholder replacement with
{orgSlug}- Active state based on pathname comparison
130-186: Well-structured unified header layout.The refactored JSX correctly:
- Renders
languageButtonsin bothdesktop-languagesandmobile-languages-rowcontainers- Uses CSS classes (
mobile-hidden,display-flex) for responsive behavior- Preserves
header-mobile-menuclass for organization-specific styling compatibility (per context snippet fromorganizations/default/client_assets/index.css)client/components/header/header.test.js (2)
156-161: Test expectations correctly updated for unified header.
- 4 language buttons (2 in
.desktop-languages+ 2 in.mobile-languages-row) ✓- 2 active English buttons (one per container) ✓
- 0 active Italian buttons ✓
236-239: Language button interaction tests properly updated.Using
.at(0)and.at(1)correctly targets the duplicate Italian buttons in separate containers and validates thatsetLanguageis called for each click.Based on learnings: "Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior" - this change appropriately covers the new dual-button structure.
client/components/header/index.css (3)
117-122: Correct default-hidden state for mobile-only elements.Setting
display: noneby default for.header-hamburgerand.mobile-languages-rowensures they only appear within the mobile media query, delegating responsive behavior entirely to CSS as intended by the PR objectives.
159-164: Appropriate visibility toggling for responsive layout.
.desktop-languageshidden on mobile ✓.mobile-hiddenutility with!importantensures elements stay hidden regardless of inline styles ✓This enables the unified JSX to render both desktop and mobile language buttons once, with CSS controlling visibility.
169-191: Mobile nav styling correctly targets unified structure.The responsive rules for
.header-unified-navand.header-unified-nav > .header-linkproperly handle:
- Column layout with centered alignment
- Full-width links with appropriate spacing
- Language row visibility and centering
Test Snapshot Failures in Header ComponentHello @kunalverma2512, There are 4 test snapshot failures in the To fix this, you need to update the snapshots to reflect the current state of the component. Run the following command in your terminal: yarn run coverage -u |
kunalverma2512
force-pushed
the
eliminate-redundancy-of-header-html
branch
from
7858dfd to
4ff455d
Compare
Test Failure in Language ChangeHello @kunalverma2512,
|
kunalverma2512
force-pushed
the
eliminate-redundancy-of-header-html
branch
from
4ff455d to
4329f78
Compare
Test Failure in Password Expiration TestHello @kunalverma2512,
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
client/components/header/header.test.js (1)
108-109:⚠️ Potential issue | 🟠 MajorFix the expected call count for
isInternalLink— should be 1, not 3.The test sets up only one link in
props.header.links(lines 100-105), andisInternalLinkis called once per link in thelinks.map()at line 100 ofheader.js. The expected call count should be1, not3.🐛 Proposed fix
- expect(isInternalLink).toHaveBeenCalledTimes(3); + expect(isInternalLink).toHaveBeenCalledTimes(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/header/header.test.js` around lines 108 - 109, The test incorrectly expects isInternalLink to be called three times; update the assertion in header.test.js to expect a single call: change the expect for isInternalLink call count from 3 to 1 so it matches the single link in props.header.links and the single invocation in the links.map() inside header.js (where isInternalLink is called).
🤖 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 80-91: The language button class list in the languageButtons JSX
(inside the languages.map callback that renders buttons and calls setLanguage)
still includes "header-desktop-language-btn" even when those buttons are used in
mobile contexts; to address this, either (preferred for a quick follow-up)
replace "header-desktop-language-btn" with a neutral, unified class name (e.g.,
"header-language-btn-platform") across the map and where selectors/tests rely on
it, or if you must keep the existing class for backward-compatible test
selectors, add a one-line clarifying comment directly above the languageButtons
declaration explaining that "header-desktop-language-btn" is intentionally
preserved for backward compatibility with browser tests even though the buttons
are reused in mobile, so future cleanup can rename it safely.
In `@client/components/header/index.css`:
- Around line 197-204: Remove the redundant CSS override inside the media query:
the global rule .header-desktop-language-btn already sets display: inline-block,
so delete the block targeting .mobile-languages-row .header-desktop-language-btn
within the `@media` (max-width: 767px) section to avoid duplicate rules and keep
the stylesheet clean.
---
Duplicate comments:
In `@client/components/header/header.test.js`:
- Around line 108-109: The test incorrectly expects isInternalLink to be called
three times; update the assertion in header.test.js to expect a single call:
change the expect for isInternalLink call count from 3 to 1 so it matches the
single link in props.header.links and the single invocation in the links.map()
inside header.js (where isInternalLink is called).
🪄 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: 8bdf353d-c157-4916-a1bb-31c16f574412
⛔ Files ignored due to path filters (1)
client/components/header/__snapshots__/header.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
client/components/header/header.jsclient/components/header/header.test.jsclient/components/header/index.css
📜 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:
client/components/header/header.test.js
🧠 Learnings (5)
📚 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:
client/components/header/header.jsclient/components/header/header.test.js
📚 Learning: 2026-03-25T00:22:16.841Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-25T00:22:16.841Z
Learning: In `openwisp/openwisp-wifi-login-pages` browser tests (e.g., `browser-test/mobile-verfication.test.js`), fixed delay waits (e.g., `driver.sleep()`) are intentionally used instead of `driver.wait()` for toast message animations. This is because the JS animation on toast messages does not expose a reliable DOM state to poll against, and testing confirmed that fixed waits are more robust in this context. Do not flag these as flaky-by-design without considering this justification.
Applied to files:
client/components/header/header.jsclient/components/header/header.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: Bug Fixes: If the bug affects the user interface, a Selenium browser test should be included
Applied to files:
client/components/header/header.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:
client/components/header/header.test.js
📚 Learning: 2026-03-18T17:34:09.903Z
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:09.903Z
Learning: Do not rely on mocking or altering window.location in Jest/JSDOM for this project. Since window.location is non-configurable in the test environment and attempts to redefine or delete it can cause state leaks across tests, avoid hacks like delete window.location or Object.defineProperty(window, 'location', ...) in tests. Instead, migrate detection-related tests to React Testing Library (RTL) and use RTL-friendly patterns (e.g., render components and inspect behavior via UI, or mock network/detection logic with explicit dependency injection or module mocks). This pattern should apply to all test files under client/components (any .test.js) where window.location might be interacted with.
Applied to files:
client/components/header/header.test.js
🔇 Additional comments (7)
client/components/header/header.js (2)
93-128: LGTM! Navigation link rendering logic is correct.The unified
navLinkscomputation properly:
- Filters links based on
shouldLinkBeShownwith authentication/verification state- Uses
isInternalLinkcorrectly withlink.url(matching the function signature incheck-internal-links.js)- Conditionally renders
Linkvsabased on internal/external status- Applies active state when
pathnamematches the resolved URL
178-185: LGTM! Unified nav structure preserves organization theming.The
header-mobile-menuclass is retained alongside the newheader-unified-navclass, which maintains compatibility with organization-specific CSS overrides (e.g.,organizations/default/client_assets/index.cssline 148 targets.header-mobile-menufor background styling). Themobile-hiddenclass toggle correctly works with the CSS to show/hide on mobile.client/components/header/header.test.js (3)
153-161: LGTM! Test selectors correctly updated for unified structure.The test assertions properly reflect the new unified rendering:
- Line 154: Uses
.header-link(unified) instead of desktop-specific selector- Line 157: Expects 4 language buttons (2 languages × 2 locations = 4)
- Lines 160-161: Active state assertions account for both desktop and mobile instances
163-175: LGTM! Logo selector tests updated correctly.The selector change from
.header-logo-image.header-desktop-logo-imageto.header-logo-imagecorrectly matches the simplified class structure in the component.
235-240: LGTM! Language button interaction tests updated correctly.The test now uses
.at(0)and.at(1)to select specific buttons from the unified rendering, and the assertion progression (1 → 2 calls) correctly validates that both button instances triggersetLanguage.client/components/header/index.css (2)
67-78: LGTM! Unified nav styling properly replaces the previous desktop-specific selector.The
.header-unified-navselector correctly inherits the flexbox layout styling previously applied to.header-row-2-inner, maintaining the same max-width and alignment for both desktop and mobile contexts.
117-122: LGTM! Default hidden state for mobile-only elements.Setting
.header-hamburgerand.mobile-languages-rowtodisplay: noneby default ensures they're hidden on desktop and only revealed via the mobile media query.
Test Failure in Password Expired TestHello @kunalverma2512,
|
kunalverma2512
force-pushed
the
eliminate-redundancy-of-header-html
branch
2 times, most recently
from
ad5b926 to
9d7893d
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 106-131: The Link rendering has an extra trailing space in its
className template ("button ") causing inconsistent class attribute compared to
the anchor branch ("button"); update the Link JSX in the map (the Link element
that uses pathname === link.url.replace("{orgSlug}", orgSlug) ? "active" : "")
to remove the trailing space so both branches use "button" consistently.
🪄 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: 88e9398f-7062-48d3-864c-6c2e136414be
📒 Files selected for processing (2)
client/components/header/header.jsclient/components/header/index.css
📜 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 (7)
client/components/header/index.css (5)
67-78: LGTM!Clean refactor: unifying the
.header-unified-navand.sticky-container .innerselectors with shared flex layout properties is a good approach for consistent styling and maintainability.
117-122: LGTM!Properly hiding hamburger and mobile language row by default, with media query revealing them on mobile. This follows mobile-first/desktop-first responsive patterns correctly.
159-164: The!importantdeclaration is acceptable here for mobile hiding.Using
!importanton.mobile-hidden(line 163) ensures this utility class reliably hides elements on mobile regardless of other display rules. This is a valid use case for overriding dynamic inline styles or component-level rules.
169-184: LGTM!The mobile-specific styles for
.header-unified-navproperly override the desktop flex layout with column direction, centered alignment, and full-width link styling. Themargin-right: 0on line 183 correctly resets the desktop margin.
197-199: LGTM!This rule ensures language buttons display correctly as inline-block elements. The previous redundant media query override was correctly removed per past review feedback.
client/components/header/header.js (2)
79-94: LGTM!Good refactor: computing
languageButtonsonce and reusing it in both desktop and mobile contexts eliminates the previous duplication. The comment explaining theheader-desktop-language-btnclass retention for Selenium backward compatibility is helpful.
133-192: LGTM! Clean unified header structure.The refactored render method successfully:
- Eliminates duplicate desktop/mobile header markup
- Computes
navLinksandlanguageButtonsonce- Preserves
header-mobile-menuclass for organization-specific styling- Uses CSS-driven responsive behavior via
mobile-hiddenanddesktop-languagescontainersThis aligns well with the PR objectives of reducing DOM duplication while maintaining visual consistency.
Code Style FailureHello @kunalverma2512, The CI failed due to code style issues detected by Prettier in Fix: Run |
kunalverma2512
force-pushed
the
eliminate-redundancy-of-header-html
branch
from
9d7893d to
d556dfe
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
client/components/header/header.js (1)
106-131:⚠️ Potential issue | 🟡 MinorFix trailing space in className for consistency.
Line 112 has a trailing space after
button(button) while line 123 hasbuttonwithout the trailing space. This creates inconsistent whitespace in the rendered class attribute.🔧 Proposed fix
return ( <Link className={`header-link header-link-${index + 1} ${ pathname === link.url.replace("{orgSlug}", orgSlug) ? "active" : "" - } button `} + } button`} to={link.url.replace("{orgSlug}", orgSlug)} key={index} >🤖 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 106 - 131, The Link element in the header component returns className={`header-link header-link-${index + 1} ${ pathname === link.url.replace("{orgSlug}", orgSlug) ? "active" : "" } button ` which includes a trailing space after "button" causing inconsistent class attributes compared to the anchor branch; remove the trailing space so the className ends with "button" (update the JSX in the Link return where className is constructed, referencing link, index, pathname, orgSlug and getText to locate the exact spot).client/components/header/header.test.js (1)
108-109:⚠️ Potential issue | 🟡 MinorFix the expected call count for
isInternalLink.The test sets up only one link in
props.header.linksbut expectsisInternalLinkto be called 3 times. SinceisInternalLinkis called once per link in thelinks.map()at line 103 of header.js, and there's only 1 link, the expected call count should be 1.🔧 Proposed fix
wrapper = shallow(<Header {...props} />); - expect(isInternalLink).toHaveBeenCalledTimes(3); + expect(isInternalLink).toHaveBeenCalledTimes(1); expect(isInternalLink).toHaveBeenCalledWith("/default/login");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/header/header.test.js` around lines 108 - 109, The test's expectation for isInternalLink call count is wrong because props.header.links contains only one link; update the assertion in header.test.js to expect a single call (change expect(isInternalLink).toHaveBeenCalledTimes(3) to expect(isInternalLink).toHaveBeenCalledTimes(1)) while keeping the existing expect(isInternalLink).toHaveBeenCalledWith("/default/login") so it still verifies the argument from the links.map() invocation in header.js.
🤖 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/components/header/header.js`:
- Around line 106-131: The Link element in the header component returns
className={`header-link header-link-${index + 1} ${ pathname ===
link.url.replace("{orgSlug}", orgSlug) ? "active" : "" } button ` which includes
a trailing space after "button" causing inconsistent class attributes compared
to the anchor branch; remove the trailing space so the className ends with
"button" (update the JSX in the Link return where className is constructed,
referencing link, index, pathname, orgSlug and getText to locate the exact
spot).
In `@client/components/header/header.test.js`:
- Around line 108-109: The test's expectation for isInternalLink call count is
wrong because props.header.links contains only one link; update the assertion in
header.test.js to expect a single call (change
expect(isInternalLink).toHaveBeenCalledTimes(3) to
expect(isInternalLink).toHaveBeenCalledTimes(1)) while keeping the existing
expect(isInternalLink).toHaveBeenCalledWith("/default/login") so it still
verifies the argument from the links.map() invocation in header.js.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fdf49064-137d-4cc3-8489-f49ae385bbe4
⛔ Files ignored due to path filters (1)
client/components/header/__snapshots__/header.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
client/components/header/header.jsclient/components/header/header.test.jsclient/components/header/index.css
📜 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:
client/components/header/header.test.js
🧠 Learnings (4)
📚 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:
client/components/header/header.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:
client/components/header/header.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} : Features: Tests must be added with coverage that does not decrease significantly; for UI features, Selenium browser tests are preferred
Applied to files:
client/components/header/header.test.js
📚 Learning: 2026-03-18T17:34:09.903Z
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:09.903Z
Learning: Do not rely on mocking or altering window.location in Jest/JSDOM for this project. Since window.location is non-configurable in the test environment and attempts to redefine or delete it can cause state leaks across tests, avoid hacks like delete window.location or Object.defineProperty(window, 'location', ...) in tests. Instead, migrate detection-related tests to React Testing Library (RTL) and use RTL-friendly patterns (e.g., render components and inspect behavior via UI, or mock network/detection logic with explicit dependency injection or module mocks). This pattern should apply to all test files under client/components (any .test.js) where window.location might be interacted with.
Applied to files:
client/components/header/header.test.js
🔇 Additional comments (6)
client/components/header/header.js (1)
79-94: Good addition of backward compatibility comment.The comment clearly documents why
header-desktop-language-btnclass is preserved despite being used in both desktop and mobile contexts. This addresses the semantic naming concern while maintaining Selenium test compatibility.client/components/header/index.css (3)
67-78: LGTM!The
.header-unified-navselector correctly replaces the previous.header-row-2-innerwhile maintaining the same flexbox layout properties. The unified styling supports both desktop and mobile navigation in a single structure.
117-122: Good approach: default-hidden with mobile media query override.Setting
.header-hamburgerand.mobile-languages-rowtodisplay: noneby default and then revealing them in the mobile media query is a clean responsive pattern that avoids JavaScript-based visibility toggling.
197-199: LGTM!The
.header-desktop-language-btnrule is correctly placed outside the media query as a global style. The previously flagged redundant media query override has been removed.client/components/header/header.test.js (2)
156-161: LGTM!The updated expectations correctly reflect the unified header structure:
- 4 language buttons (2 languages × 2 render locations: desktop + mobile)
- 2 active English buttons (one in each location)
This aligns with the component rendering
languageButtonsin both.desktop-languagesand.mobile-languages-row.
235-240: LGTM!The interaction test correctly targets both Italian language button instances (
.at(0)and.at(1)) to verify that clicking either the desktop or mobile button triggers thesetLanguagecallback. This ensures both button locations are functional.
kunalverma2512
force-pushed
the
eliminate-redundancy-of-header-html
branch
from
d556dfe to
e173689
Compare
Test Failures in Browser TestsHello @kunalverma2512,
|
kunalverma2512
force-pushed
the
eliminate-redundancy-of-header-html
branch
from
e173689 to
a93921f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
client/components/header/header.test.js (1)
108-109:⚠️ Potential issue | 🟠 MajorFix the expected call count for
isInternalLink— this test will fail.The test configures
props.header.linkswith only 1 link (lines 100-105), but expectsisInternalLinkto be called 3 times. Looking atheader.js,isInternalLinkis called once per link insidelinks.map()(line 103). With 1 link, the expected call count should be 1, not 3.- expect(isInternalLink).toHaveBeenCalledTimes(3); + expect(isInternalLink).toHaveBeenCalledTimes(1);,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/header/header.test.js` around lines 108 - 109, The test's expected call count for isInternalLink is wrong: it configures props.header.links with a single link but asserts isInternalLink was called 3 times; update the assertion in client/components/header/header.test.js to expect 1 call (i.e., change expect(isInternalLink).toHaveBeenCalledTimes(3) to toHaveBeenCalledTimes(1)) since header.js calls isInternalLink once per entry in links.map().
🤖 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/components/header/header.test.js`:
- Around line 108-109: The test's expected call count for isInternalLink is
wrong: it configures props.header.links with a single link but asserts
isInternalLink was called 3 times; update the assertion in
client/components/header/header.test.js to expect 1 call (i.e., change
expect(isInternalLink).toHaveBeenCalledTimes(3) to toHaveBeenCalledTimes(1))
since header.js calls isInternalLink once per entry in links.map().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15756221-d348-4ace-96fc-5fd38ceceefb
⛔ Files ignored due to path filters (1)
client/components/header/__snapshots__/header.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
client/components/header/header.jsclient/components/header/header.test.jsclient/components/header/index.css
📜 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:
client/components/header/header.test.js
🧠 Learnings (5)
📚 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:
client/components/header/header.test.js
📚 Learning: 2026-03-25T00:22:16.841Z
Learnt from: nemesifier
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: :0-0
Timestamp: 2026-03-25T00:22:16.841Z
Learning: In `openwisp/openwisp-wifi-login-pages` browser tests (e.g., `browser-test/mobile-verfication.test.js`), fixed delay waits (e.g., `driver.sleep()`) are intentionally used instead of `driver.wait()` for toast message animations. This is because the JS animation on toast messages does not expose a reliable DOM state to poll against, and testing confirmed that fixed waits are more robust in this context. Do not flag these as flaky-by-design without considering this justification.
Applied to files:
client/components/header/header.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:
client/components/header/header.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} : Features: Tests must be added with coverage that does not decrease significantly; for UI features, Selenium browser tests are preferred
Applied to files:
client/components/header/header.test.js
📚 Learning: 2026-03-18T17:34:09.903Z
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:09.903Z
Learning: Do not rely on mocking or altering window.location in Jest/JSDOM for this project. Since window.location is non-configurable in the test environment and attempts to redefine or delete it can cause state leaks across tests, avoid hacks like delete window.location or Object.defineProperty(window, 'location', ...) in tests. Instead, migrate detection-related tests to React Testing Library (RTL) and use RTL-friendly patterns (e.g., render components and inspect behavior via UI, or mock network/detection logic with explicit dependency injection or module mocks). This pattern should apply to all test files under client/components (any .test.js) where window.location might be interacted with.
Applied to files:
client/components/header/header.test.js
🔇 Additional comments (11)
client/components/header/header.test.js (3)
156-161: LGTM!The test correctly expects 4 language buttons (2 languages × 2 render locations) and 2 active English buttons (one in desktop, one in mobile context). This aligns with the unified rendering approach where
languageButtonsis rendered in both.desktop-languagesand.mobile-languages-row.
235-240: LGTM!The interaction test correctly targets both Italian language button instances (
.at(0)and.at(1)) in the unified rendering structure and verifies thatsetLanguageis called the expected number of times.
153-155: LGTM!The selector update from desktop-specific to unified
.header-linkcorrectly reflects the refactored component structure.client/components/header/index.css (4)
67-78: LGTM!The unified
.header-unified-navselector correctly replaces the previous desktop-specific selector, matching the new DOM structure inheader.js. The flex layout rules are appropriate for the navigation container.
117-122: LGTM!The default
display: nonefor.header-hamburgerand.mobile-languages-rowcorrectly establishes the desktop baseline where these mobile-specific elements should be hidden.
159-164: LGTM!The mobile media query correctly hides
.desktop-languagesand uses!importanton.mobile-hiddento override any dynamically-applieddisplay-flexutility class.
197-199: LGTM!The
.header-desktop-language-btnrule placed outside the media query ensures consistent visibility across viewports for the language buttons.client/components/header/header.js (4)
79-94: LGTM!The backward-compatibility comment (lines 80-82) clearly documents why
header-desktop-language-btnis preserved despite being used in both viewports. ThelanguageButtonsarray is cleanly computed once and ready for reuse in both desktop and mobile locations.
96-131: LGTM!The unified
navLinkslogic correctly:
- Filters links using
shouldLinkBeShownbefore rendering- Uses
isInternalLinkto determine routing approach- Renders internal links as
<Link>with active state detection- Renders external links as
<a>with proper security attributes (target="_blank",rel="noreferrer noopener")
181-188: LGTM!The unified structure elegantly handles responsive behavior:
header-row-2toggles visibility viamobile-hiddenclass based on menu stateheader-unified-nav header-mobile-menudual class preserves organization-specific theming while applying unified nav stylesnavLinksandlanguageButtonsare rendered once and reused, eliminating the previous duplication
140-148: LGTM!The logo rendering correctly uses the unified
header-logo-imageclass without viewport-specific modifiers, simplifying the CSS selectors and DOM structure.
|
@nemesifier All checks have passed and the PR is approved. It is ready for merge. |
1 participant
Checklist
Reference to Existing Issue
Closes #314.
Description of Changes
I have refactored the Header component to fix the issue where the desktop and mobile navigation were being rendered as two completely separate blocks of HTML. By moving the nav links and language buttons into shared constants within the render method, I was able to unify the entire header into a single JSX structure that handles different screen sizes purely through CSS media queries. This makes the code a lot easier to maintain because we no longer have to update links in two different places, and it also slightly reduces the DOM weight for the browser. I specifically kept the .header-mobile-menu class on the container to make sure the organization-specific background colors and themes still apply correctly on mobile, so there are no visual regressions from the original design.
Screenshot
1.Before change
in web view
in phone view (Toggled opened)
2.After change
in web view
in phone view (Toggled opened)
