Conversation
📝 WalkthroughWalkthroughThis PR applies several targeted improvements: adding HTTP security policy headers via Netlify configuration, standardizing URLs to HTTPS, updating the blog route with a trailing slash, restructuring footer social link markup, adjusting a blog heading level, refining focus-visibility styling, and introducing a service worker for cleanup. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
gatsby-config.js (1)
205-205: Remove the deprecatedFeature-Policyheader (line 205).Both
Permissions-Policy(line 202) andFeature-Policy(line 205) are present and configure identical restrictions for camera, microphone, and geolocation. TheFeature-Policyheader is deprecated and should be removed;Permissions-Policyis the modern standard and takes precedence when both are present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gatsby-config.js` at line 205, Remove the deprecated Feature-Policy header entry by deleting the string "Feature-Policy: camera 'none'; microphone 'none'; geolocation 'none'" from the headers list in gatsby-config.js so only the modern "Permissions-Policy" header remains (it's the duplicate at/near the Permissions-Policy entry); ensure no other occurrences of "Feature-Policy" remain in the headers array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@static/sw.js`:
- Around line 3-7: The activate handler currently doesn't accept the event param
or use event.waitUntil, and it uses forEach which ignores the promises from
client.navigate; change the listener to accept the event (e.g., (event) => { ...
}) and call event.waitUntil(...) with a promise chain that awaits
self.clients.matchAll({type: 'window'}) then maps clients to
client.navigate(client.url), uses Promise.all(...) to wait for all navigations,
and finally chains self.registration.unregister() so unregister runs after
navigations complete; reference the existing addEventListener('activate'),
self.clients.matchAll, client.navigate, event.waitUntil, and
self.registration.unregister when making this change.
---
Nitpick comments:
In `@gatsby-config.js`:
- Line 205: Remove the deprecated Feature-Policy header entry by deleting the
string "Feature-Policy: camera 'none'; microphone 'none'; geolocation 'none'"
from the headers list in gatsby-config.js so only the modern
"Permissions-Policy" header remains (it's the duplicate at/near the
Permissions-Policy entry); ensure no other occurrences of "Feature-Policy"
remain in the headers array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 645f6aab-75f7-44a1-8025-f32710ea6464
📒 Files selected for processing (8)
gatsby-config.jssrc/components/pages/blog/hero/hero.jsxsrc/components/pages/home/email-editor/data/email-content.jssrc/components/shared/footer/footer.jsxsrc/constants/links.jssrc/data/pages/inbox/index.jsxsrc/styles/maily.cssstatic/sw.js
| self.addEventListener('activate', () => { | ||
| self.clients.matchAll({ type: 'window' }).then((clients) => { | ||
| clients.forEach((client) => client.navigate(client.url)); | ||
| }); | ||
| self.registration.unregister(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify lifecycle handling and async patterns in service worker
rg -n "addEventListener\\('install'|addEventListener\\('activate'|waitUntil|forEach\\(|map\\(|navigate\\(|unregister\\(" static/sw.jsRepository: novuhq/website
Length of output: 263
🏁 Script executed:
cat -n static/sw.jsRepository: novuhq/website
Length of output: 439
activate event handler must use event.waitUntil() to delay termination during cleanup.
The handler doesn't extend the service worker's lifetime for async operations. Without event.waitUntil(), the browser may terminate the service worker before matchAll(), navigate(), and unregister() complete. Additionally, forEach() ignores promises returned by client.navigate() calls, so the async operations aren't properly awaited. The unregister() call executes immediately without waiting for all navigations to finish.
Use map() + Promise.all() to aggregate promises from all navigate() calls, and chain unregister() to execute after navigations complete—all wrapped in event.waitUntil().
🔧 Suggested fix
-self.addEventListener('activate', () => {
- self.clients.matchAll({ type: 'window' }).then((clients) => {
- clients.forEach((client) => client.navigate(client.url));
- });
- self.registration.unregister();
-});
+self.addEventListener('activate', (event) => {
+ event.waitUntil(
+ self.clients
+ .matchAll({ type: 'window' })
+ .then((clients) => Promise.all(clients.map((client) => client.navigate(client.url))))
+ .then(() => self.registration.unregister())
+ );
+});📝 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.
| self.addEventListener('activate', () => { | |
| self.clients.matchAll({ type: 'window' }).then((clients) => { | |
| clients.forEach((client) => client.navigate(client.url)); | |
| }); | |
| self.registration.unregister(); | |
| self.addEventListener('activate', (event) => { | |
| event.waitUntil( | |
| self.clients | |
| .matchAll({ type: 'window' }) | |
| .then((clients) => Promise.all(clients.map((client) => client.navigate(client.url)))) | |
| .then(() => self.registration.unregister()) | |
| ); | |
| }); |
🧰 Tools
🪛 Biome (2.4.7)
[error] 5-5: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@static/sw.js` around lines 3 - 7, The activate handler currently doesn't
accept the event param or use event.waitUntil, and it uses forEach which ignores
the promises from client.navigate; change the listener to accept the event
(e.g., (event) => { ... }) and call event.waitUntil(...) with a promise chain
that awaits self.clients.matchAll({type: 'window'}) then maps clients to
client.navigate(client.url), uses Promise.all(...) to wait for all navigations,
and finally chains self.registration.unregister() so unregister runs after
navigations complete; reference the existing addEventListener('activate'),
self.clients.matchAll, client.navigate, event.waitUntil, and
self.registration.unregister when making this change.
1 participant
Summary
SEO audit fixes — Round 2 based on SquirrelScan 730-page audit. Targets list structure, insecure URLs, and focus visibility issues in the Gatsby site.
Round 1 (already merged)
Round 2 (this PR)
<Link>elements in<li>inside footer<ul>— they were direct children without list item wrappers (fixes 518 pages)http://URLs tohttps://—go.novu.co/dashboard(2 occurrences) andAmazon.comlink in email template.tippy-box * { outline: none !important }to exclude:focus-visibleelements, preserving keyboard focus indicatorsFiles changed
List structure
src/components/shared/footer/footer.jsx— social links wrapped in<li>HTTP → HTTPS
src/data/pages/inbox/index.jsx—http://go.novu.co/dashboard→https://(2 occurrences)src/components/pages/home/email-editor/data/email-content.js—http://Amazon.com→https://Focus visible
src/styles/maily.css—*→*:not(:focus-visible)in.tippy-boxoutline overrideSummary by CodeRabbit
Security
Accessibility
Infrastructure