[chores] Updated Twitter reference to X#1046
Conversation
📝 WalkthroughWalkthroughThe changes replace Twitter branding with X across test fixtures and organization configuration: alt text and CSS classes ( Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Ayush4958
changed the title
Multiple Snapshot Failures and Babel WarningsHello @Ayush4958 and @nemesifier, There are multiple test failures due to snapshot mismatches and several warnings related to Babel configuration. 1. Test Failures (Snapshot Mismatches)
2. Babel Configuration Warnings
|
Code Review SummaryStatus: No Issues Found | Recommendation: Merge This PR successfully rebrands Twitter to X across the codebase. All changes are consistent and complete. Files Reviewed (7 files)
Other Observations
|
Commit Message FailureHello @Ayush4958, There's an issue with your commit message format. OpenWISP requires a specific format for commit messages to ensure consistency and clarity. Explanation: The CI pipeline detected that the commit message does not adhere to the expected format. Specifically, it's missing the required structure for the commit title, body, and footer, which includes referencing an issue number. Remediation: Please reformat your commit message according to the following convention: Example: Please update your commit message and push the changes again. |
Commit Message FailureHello @Ayush4958, The CI pipeline failed because the commit message does not adhere to the project's conventions. Specifically, the commit message is missing the required closing keyword and issue number in the footer. Explanation: Remediation: For instance, if your commit fixes issue #1046, a valid commit message would look like this: |
Commit Message FailureHello @Ayush4958, Explanation: Remediation: For your specific commit, it should look something like this: |
Commit Message Format ViolationHello @Ayush4958, Explanation: Remediation: Here's an example of the correct format: |
Commit Message FailureHello @Ayush4958, The CI pipeline failed because the commit message does not adhere to the expected OpenWISP format. Explanation: Remediation: For example, based on your commit, a corrected message might be: |
…enwisp#1046 Updated snapshots to match current rendered output after recent changes. Fixes openwisp#1046
|
@nemesifier , thanks ... |
|
Hi @nemesifier , |
|
@Ayush4958 thanks, I am busy testing other PRs which are more critical, please leave this open and as soon as I double check with manual testing I will merge. We don't merge PRs without manual verification and this takes time. Thanks. |
|
Okay , Understandable now , |
nemesifier
changed the title
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
A new SVG is being added, but there's already an existing one which should be renamed instead: organizations/default/client_assets/twitter.svg.
| alt="X" | ||
| class="contact-image " | ||
| src="/assets/default/twitter.svg" | ||
| src="/assets/default/X.svg" |
There was a problem hiding this comment.
| src="/assets/default/X.svg" | |
| src="/assets/default/x.svg" |
Applies to all other occurrences
| <a | ||
| className="link " | ||
| href="https://twitter.com/openwisp" | ||
| href="https://X.com/openwisp" |
There was a problem hiding this comment.
| href="https://X.com/openwisp" | |
| href="https://x.com/openwisp" |
applies to all other occurrences
There was a problem hiding this comment.
Sorry late answering , I had made the changes
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/contact-box/contact.test.js`:
- Around line 15-19: Update the test in contact.test.js to assert the actual
rendered link metadata for the X entry instead of only checking the
presence/count of the `.link.X` CSS class: locate the test that uses the X
fixture (the object with keys alt, icon, url, authenticated, css) and add
assertions that the rendered anchor element with class `.link.X` has the correct
href attribute equal to the fixture's url and that the image (or accessible
element) uses the expected alt text from the fixture; also add similar href/alt
assertions in the other related tests that currently only check `.link.X` (the
ones referenced around the other assertions) so the behavior is validated beyond
CSS class matching.
🪄 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: a5a1f45a-53a3-4c5a-ac5e-57683b768911
⛔ Files ignored due to path filters (4)
client/components/contact-box/__snapshots__/contact.test.js.snapis excluded by!**/*.snapclient/components/login/__snapshots__/login.test.js.snapis excluded by!**/*.snapclient/components/password-confirm/__snapshots__/password-confirm.test.js.snapis excluded by!**/*.snaporganizations/default/client_assets/x.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
client/components/contact-box/contact.test.js
📜 Review details
🧰 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/contact-box/contact.test.js
🧠 Learnings (5)
📓 Common learnings
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
📚 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/contact-box/contact.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/contact-box/contact.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/contact-box/contact.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/contact-box/contact.test.js
…penwisp#1046 Updated snapshots for the login, password-confirm, and contact-box components to reflect recent changes. This ensures that the UI tests pass with the latest UI elements. Fixes openwisp#1046 Signed-off-by: Ayush <bhandariayush935@gmail.com>
…penwisp#1046 Updated snapshots for the login, password-confirm, and contact-box components to reflect recent changes. This ensures that the UI tests pass with the latest UI elements. Fixes openwisp#1046 Signed-off-by: Ayush <bhandariayush935@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/test-config.json (1)
158-169:⚠️ Potential issue | 🔴 CriticalAdd missing
cssproperty to the X social link.Similar to the issue in
organizations/default/default.yml, this test configuration is missing thecssproperty that the contact component requires to generate CSS class selectors. Without this property, tests will fail when they expect.link.xselectors.🔧 Proposed fix to add the css property
"social_links": [ { "alt": "x", "icon": "x.svg", - "url": "https://x.com/openwisp" + "url": "https://x.com/openwisp", + "css": "x" }, { "alt": "facebook", "icon": "facebook.svg", - "url": "https://facebook.com/openwisp" + "url": "https://facebook.com/openwisp", + "css": "facebook" } ]Based on the component implementation at
client/components/contact-box/contact.js:45,52which useslink.cssto generate the CSS class.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/test-config.json` around lines 158 - 169, The X social link in the social_links array is missing the css property required by the contact component (which reads link.css to build class selectors); update the X entry in the social_links array so it includes a css property (e.g., "css": "x") matching the alt/icon identifier used to produce the .link.x selector so tests expecting that selector will pass.
♻️ Duplicate comments (2)
client/components/contact-box/contact.test.js (2)
64-73:⚠️ Potential issue | 🟡 MinorAssert X link metadata directly (not only class presence).
The test only verifies CSS class presence but doesn't validate the actual rendered link attributes (href, alt). As noted in a previous review, tests should assert the X link's metadata to properly validate the Twitter → X migration.
✅ Suggested test hardening
it("should render without authenticated links when not authenticated", () => { props = createTestProps(); props.contactPage.social_links = links; props.isAuthenticated = false; const wrapper = shallow(<Contact {...props} />); expect(wrapper.find(".contact-image")).toHaveLength(2); expect(wrapper.find(".link.google")).toHaveLength(1); expect(wrapper.find(".link.facebook")).toHaveLength(1); - expect(wrapper.find(".link.x")).toHaveLength(0); + // X link should be hidden when not authenticated + expect(wrapper.find(".link.x")).toHaveLength(0); });As per coding guidelines: "Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/contact-box/contact.test.js` around lines 64 - 73, Update the test "should render without authenticated links when not authenticated" in contact.test.js to assert the X link's rendered attributes (not just its class). After rendering Contact with props.contactPage.social_links = links and isAuthenticated = false, locate the X link (wrapper.find(".link.x") or the anchor element within it) and assert its href and alt/title/text match the expected values coming from the links test fixture (the links variable) so the X (Twitter→X) migration is verified end-to-end.
75-84:⚠️ Potential issue | 🟡 MinorAssert X link metadata directly (not only class presence).
The test only verifies CSS class count but doesn't validate that the rendered X link has the correct
hrefandaltattributes. This leaves the Twitter → X migration incompletely tested.✅ Suggested test hardening
it("should render with authenticated links when authenticated", () => { props = createTestProps(); props.contactPage.social_links = links; props.isAuthenticated = true; const wrapper = shallow(<Contact {...props} />); expect(wrapper.find(".contact-image")).toHaveLength(2); expect(wrapper.find(".link.google")).toHaveLength(1); - expect(wrapper.find(".link.x")).toHaveLength(1); + const xLink = wrapper.find(".link.x"); + expect(xLink).toHaveLength(1); + expect(xLink.at(0).prop("href")).toBe("https://x.com/openwisp"); + const xImage = xLink.find(".contact-image"); + expect(xImage).toHaveLength(1); + expect(xImage.at(0).prop("alt")).toBe("x"); expect(wrapper.find(".link.facebook")).toHaveLength(0); });As per coding guidelines: "Tests must be updated to cover non-trivial changes and ensure proper validation of modified behavior".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/components/contact-box/contact.test.js` around lines 75 - 84, The test "should render with authenticated links when authenticated" only checks element counts; update it to assert the X link's actual attributes by locating the rendered X link for the Contact component (use the test's props.contactPage.social_links / links fixture and the .link.x selector used in the test) and verify its href and alt/text match the expected values from the links fixture (e.g., links.x.url and links.x.alt), in addition to the existing class presence assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@organizations/default/default.yml`:
- Around line 134-137: Add the missing css property to the X social link entry
so the contact component can generate the correct CSS class (it currently only
has alt/icon/url). Update the social link object (the X entry) to include css:
"x" so client/components/contact-box/contact.js (which reads link.css) will
produce className="link x" and satisfy the tests referenced in contact.test.js.
---
Outside diff comments:
In `@client/test-config.json`:
- Around line 158-169: The X social link in the social_links array is missing
the css property required by the contact component (which reads link.css to
build class selectors); update the X entry in the social_links array so it
includes a css property (e.g., "css": "x") matching the alt/icon identifier used
to produce the .link.x selector so tests expecting that selector will pass.
---
Duplicate comments:
In `@client/components/contact-box/contact.test.js`:
- Around line 64-73: Update the test "should render without authenticated links
when not authenticated" in contact.test.js to assert the X link's rendered
attributes (not just its class). After rendering Contact with
props.contactPage.social_links = links and isAuthenticated = false, locate the X
link (wrapper.find(".link.x") or the anchor element within it) and assert its
href and alt/title/text match the expected values coming from the links test
fixture (the links variable) so the X (Twitter→X) migration is verified
end-to-end.
- Around line 75-84: The test "should render with authenticated links when
authenticated" only checks element counts; update it to assert the X link's
actual attributes by locating the rendered X link for the Contact component (use
the test's props.contactPage.social_links / links fixture and the .link.x
selector used in the test) and verify its href and alt/text match the expected
values from the links fixture (e.g., links.x.url and links.x.alt), in addition
to the existing class presence assertions.
🪄 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: db1effd1-f56e-4e10-9211-2daa9162bcd0
⛔ Files ignored due to path filters (4)
client/components/contact-box/__snapshots__/contact.test.js.snapis excluded by!**/*.snapclient/components/login/__snapshots__/login.test.js.snapis excluded by!**/*.snapclient/components/mobile-phone-change/__snapshots__/mobile-phone-change.test.js.snapis excluded by!**/*.snapclient/components/password-confirm/__snapshots__/password-confirm.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
client/components/contact-box/contact.test.jsclient/test-config.jsonorganizations/default/default.yml
📜 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/contact-box/contact.test.js
🧠 Learnings (3)
📓 Common learnings
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: Changes: PR description must include screenshots or screen recordings for any UI changes
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
📚 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/contact-box/contact.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/contact-box/contact.test.js
🔇 Additional comments (2)
client/components/contact-box/contact.test.js (1)
15-19: LGTM! Mock data structure is correct.The test mock properly includes the
cssproperty that the component expects. However, note that the actual configuration inorganizations/default/default.ymlis missing this property (see review comment for that file).organizations/default/default.yml (1)
163-165: The X social link configuration is correct as-is.The login component does not use a
cssproperty. It applies a hardcoded className of"social-link button full"to all social links. The X link entry contains all required properties (text, icon, and url) that the login component actually expects.> Likely an incorrect or invalid review comment.
Code Style FailureHello @Ayush4958,
|
2 participants
Checklist
Reference to Existing Issue
Closes #1043
Description of Changes
Added X Social in the place where previously Twitter social was used
Screenshot