Fix: Prevent "Powered by Ghost" from opening two tabs#24623
Fix: Prevent "Powered by Ghost" from opening two tabs#24623niranjan-uma-shankar wants to merge 3 commits intoTryGhost:mainfrom
Conversation
This change ensures that clicking on the Powered By link does not open two tabs.
WalkthroughRemoved the custom onClick handler from the anchor in apps/portal/src/components/common/PoweredBy.js that previously called window.open('https://ghost.org', '_blank'). The anchor now relies on href="https://ghost.org" with target="_blank" and rel="noopener noreferrer" and includes a data-test-powered-by attribute. Added a test in apps/portal/src/tests/portal-links.test.js that renders the portal signup, finds the "powered by ghost" link in the popup, and asserts href, target, rel, and absence of an onClick handler. No other component structure, content, or exports changed. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
niranjan-uma-shankar
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/portal/src/components/common/PoweredBy.js (1)
14-14: Accessibility: hide decorative SVG from screen readers.Mark the logo as decorative so the link’s accessible name is just “Powered by Ghost.”
- <GhostLogo /> + <GhostLogo aria-hidden="true" focusable="false" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/portal/src/components/common/PoweredBy.js(1 hunks)
🔇 Additional comments (1)
apps/portal/src/components/common/PoweredBy.js (1)
13-16: Fix looks good: removes double navigation and keeps tabnabbing protections.Relying on the anchor’s default behavior with target="_blank" and rel="noopener noreferrer" resolves the two-tab issue and preserves security best practices.
Adds a data-test-powered-by attribute to the external link to simplify E2E targeting.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/portal/src/tests/portal-links.test.js (2)
362-362: Make the rel assertion order-insensitive.Avoid brittleness if token order changes. Assert both tokens are present.
- expect(poweredByLink).toHaveAttribute('rel', 'noopener noreferrer'); + const rel = poweredByLink.getAttribute('rel') || ''; + expect(rel).toContain('noopener'); + expect(rel).toContain('noreferrer');
357-357: Prefer findByRole for async rendering in iframe.Using findByRole reduces flakiness if the link renders slightly after the iframe is ready.
- const poweredByLink = within(popupFrame.contentDocument).getByRole('link', {name: /powered by ghost/i}); + const poweredByLink = await within(popupFrame.contentDocument).findByRole('link', {name: /powered by ghost/i});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/portal/src/tests/portal-links.test.js(1 hunks)
🔇 Additional comments (1)
apps/portal/src/tests/portal-links.test.js (1)
347-355: Good coverage of the PoweredBy link semantics.Asserting presence, href, target, and rel on the link is solid and aligns with the fix.
| const poweredByLink = within(popupFrame.contentDocument).getByRole('link', {name: /powered by ghost/i}); | ||
| expect(poweredByLink).toBeInTheDocument(); | ||
| expect(poweredByLink).toHaveAttribute('href', 'https://ghost.org'); | ||
| expect(poweredByLink).toHaveAttribute('target', '_blank'); | ||
| expect(poweredByLink).toHaveAttribute('rel', 'noopener noreferrer'); | ||
| // Verify no onClick handler exists | ||
| expect(poweredByLink.onclick).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
Replace brittle onclick check with an interaction-based assertion (spy on iframe window.open).
element.onclick is null even when a framework attaches an onClick via addEventListener. This check won’t catch regressions. Instead, spy on popupFrame.contentWindow.open, click the link, and assert it wasn’t called.
- const poweredByLink = within(popupFrame.contentDocument).getByRole('link', {name: /powered by ghost/i});
+ const poweredByLink = within(popupFrame.contentDocument).getByRole('link', {name: /powered by ghost/i});
expect(poweredByLink).toBeInTheDocument();
expect(poweredByLink).toHaveAttribute('href', 'https://ghost.org');
expect(poweredByLink).toHaveAttribute('target', '_blank');
expect(poweredByLink).toHaveAttribute('rel', 'noopener noreferrer');
- // Verify no onClick handler exists
- expect(poweredByLink.onclick).toBeNull();
+ // Clicking the link should not invoke script-driven window.open (prevents "two tabs" regression)
+ const openSpy = jest.spyOn(popupFrame.contentWindow, 'open').mockImplementation(() => null);
+ fireEvent.click(poweredByLink);
+ expect(openSpy).not.toHaveBeenCalled();📝 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.
| const poweredByLink = within(popupFrame.contentDocument).getByRole('link', {name: /powered by ghost/i}); | |
| expect(poweredByLink).toBeInTheDocument(); | |
| expect(poweredByLink).toHaveAttribute('href', 'https://ghost.org'); | |
| expect(poweredByLink).toHaveAttribute('target', '_blank'); | |
| expect(poweredByLink).toHaveAttribute('rel', 'noopener noreferrer'); | |
| // Verify no onClick handler exists | |
| expect(poweredByLink.onclick).toBeNull(); | |
| }); | |
| const poweredByLink = within(popupFrame.contentDocument).getByRole('link', {name: /powered by ghost/i}); | |
| expect(poweredByLink).toBeInTheDocument(); | |
| expect(poweredByLink).toHaveAttribute('href', 'https://ghost.org'); | |
| expect(poweredByLink).toHaveAttribute('target', '_blank'); | |
| expect(poweredByLink).toHaveAttribute('rel', 'noopener noreferrer'); | |
| // Clicking the link should not invoke script-driven window.open (prevents "two tabs" regression) | |
| const openSpy = jest.spyOn(popupFrame.contentWindow, 'open').mockImplementation(() => null); | |
| fireEvent.click(poweredByLink); | |
| expect(openSpy).not.toHaveBeenCalled(); |
🤖 Prompt for AI Agents
In apps/portal/src/tests/portal-links.test.js around lines 357 to 364, replace
the brittle check of poweredByLink.onclick being null with an interaction-based
assertion: create a spy/mock on popupFrame.contentWindow.open (or
jest.spyOn(popupFrame.contentWindow, 'open')), simulate a user click on the
poweredByLink (use userEvent.click or element.click()), and assert that
contentWindow.open was not called; remove the onclick assertion and keep the
href/target/rel attribute expectations so the test verifies behavior rather than
presence of an onclick property.
2 participants
Summary
In Portal, clicking on "Powered by Ghost", in the overlay that appears in the signup modal, opens ghost.org in two tabs. Example:
Screen.Recording.2025-08-07.at.7.40.18.PM.mov
Root cause:
hrefattribute and anonClickhandler, leading to two tabs for a single click.Fix
onClickhandler.Testing Instructions