Add App Router error boundaries, Sentry monitoring, and PII scrubbing… - #118
Add App Router error boundaries, Sentry monitoring, and PII scrubbing…#118Victorjonah-prog wants to merge 8 commits into
Conversation
…t-progressive feat(pwa): add offline support and installable PWA with Serwist
…lder docs(readme): add live application screenshots
|
@Victorjonah-prog is attempting to deploy a commit to the Deen Bridge Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughSentry is integrated across browser, server, edge, CI, and Next.js builds. Error boundaries expose recovery actions and report IDs, sensitive event data is scrubbed, Stellar failures are captured, and PWA installation and offline caching support are added. ChangesApplication observability and recovery
Progressive web app support
Estimated code review effort: 4 (Complex) | ~55 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sentry.shared.config.js`:
- Around line 63-96: Replace the shallow value handling in scrubBreadcrumbs and
scrubExtra with the shared recursive redactor, preserving cookie-key removal and
string scrubbing at every nested level. Apply these same scrubbers to
breadcrumbs and extra data in sentryBeforeSendTransaction, matching the existing
sentryBeforeSend behavior for error events.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6fbe95d1-aaa6-4f6d-8062-97d07c080543
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
.env.example.github/workflows/ci.ymlapp/dashboard/courses/error.jsxapp/dashboard/error.jsxapp/dashboard/library/error.jsxapp/error.jsxapp/global-error.jsxcomponents/DebugAuthLogs.jsxhooks/useAuth.jshooks/useStellarDonation.jshooks/useStellarPayment.jsinstrumentation-client.jsinstrumentation.jslib/config/axios.config.jslib/config/req.header.config.jsnext.config.mjspackage.jsonsentry.edge.config.jssentry.server.config.jssentry.shared.config.js
💤 Files with no reviewable changes (3)
- components/DebugAuthLogs.jsx
- lib/config/req.header.config.js
- hooks/useAuth.js
| function scrubBreadcrumbs(breadcrumbs) { | ||
| if (!Array.isArray(breadcrumbs)) return breadcrumbs; | ||
| return breadcrumbs.map((crumb) => { | ||
| const cleaned = { ...crumb }; | ||
| if (cleaned.data) { | ||
| const data = { ...cleaned.data }; | ||
| delete data.cookies; | ||
| delete data.Cookie; | ||
| delete data.cookie; | ||
| if (data.headers) data.headers = scrubHeaders(data.headers); | ||
| if (data.url) data.url = scrubUrl(data.url); | ||
| if (typeof data.message === "string") data.message = scrubString(data.message); | ||
| cleaned.data = data; | ||
| } | ||
| if (typeof cleaned.message === "string") { | ||
| cleaned.message = scrubString(cleaned.message); | ||
| } | ||
| return cleaned; | ||
| }); | ||
| } | ||
|
|
||
| function scrubExtra(extra) { | ||
| if (!extra || typeof extra !== "object") return extra; | ||
| const cleaned = { ...extra }; | ||
| for (const key of SENSITIVE_COOKIE_KEYS) { | ||
| delete cleaned[key]; | ||
| } | ||
| delete cleaned.cookies; | ||
| delete cleaned.Cookie; | ||
| delete cleaned.cookie; | ||
| for (const [key, value] of Object.entries(cleaned)) { | ||
| if (typeof value === "string") cleaned[key] = scrubString(value); | ||
| } | ||
| return cleaned; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant region.
wc -l sentry.shared.config.js
cat -n sentry.shared.config.js | sed -n '1,220p'Repository: Deen-Bridge/dnb-frontend
Length of output: 5758
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the relevant symbols and call sites.
rg -n "scrubBreadcrumbs|scrubExtra|sentryBeforeSendTransaction|scrubHeaders|scrubUrl|scrubString|sendDefaultPii|breadcrumbs|extra|contexts" sentry.shared.config.jsRepository: Deen-Bridge/dnb-frontend
Length of output: 1405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the file around the reported lines, plus surrounding hook definitions.
sed -n '1,220p' sentry.shared.config.jsRepository: Deen-Bridge/dnb-frontend
Length of output: 4694
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the file outline first to locate the scrubber and hook definitions.
ast-grep outline sentry.shared.config.js --view expandedRepository: Deen-Bridge/dnb-frontend
Length of output: 744
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where these hooks are wired up and whether transaction events are routed through them.
rg -n "sentryBeforeSend(Transaction)?|beforeSendTransaction|beforeSend:|sendDefaultPii|Sentry\.init|integrations" .Repository: Deen-Bridge/dnb-frontend
Length of output: 1469
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any Sentry config files that import this module.
fd -a "sentry.*config.js|sentry.*config.ts|sentry.*config.mjs|sentry.*config.cjs" .Repository: Deen-Bridge/dnb-frontend
Length of output: 289
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where these hooks are wired up and whether transaction events are routed through them.
rg -n "sentryBeforeSend(Transaction)?|beforeSendTransaction|beforeSend:|sendDefaultPii|Sentry\\.init|integrations" .Repository: Deen-Bridge/dnb-frontend
Length of output: 1469
🌐 Web query:
Sentry JavaScript beforeSendTransaction breadcrumbs extra transaction event payload documentation
💡 Result:
In the Sentry JavaScript SDK, the beforeSendTransaction hook is a user-facing callback that allows you to modify or discard a transaction event before it is sent to Sentry [1]. Regarding the structure of transaction events and breadcrumbs: Transaction Event Payload Transactions are events enriched with span data [2]. When modifying a transaction within beforeSendTransaction, the event object typically includes properties such as transaction, contexts.trace, and spans. Because transactions represent a span tree (where the root node is the transaction itself), they must contain specific tracing information, such as the contexts.trace object [2]. Breadcrumbs within Transactions Transaction events can contain a breadcrumbs property [3]. This property is an array of breadcrumb objects or an object containing a values array [3]. Each breadcrumb object supports the following keys: type, category, message, level, timestamp, and data [4][3][5]. The data key is specifically intended for any additional, structured information you wish to include with the breadcrumb [4][3]. Modification in beforeSendTransaction You can access and modify the breadcrumbs array directly within the beforeSendTransaction hook to scrub sensitive data or add extra information [6]. Example Usage Sentry.init({ beforeSendTransaction(event) { // Example: Accessing and modifying breadcrumbs if (event.breadcrumbs) { event.breadcrumbs.forEach(breadcrumb => { // Modify or scrub data in the breadcrumb if (breadcrumb.data && breadcrumb.data.sensitiveInfo) { delete breadcrumb.data.sensitiveInfo; } }); } return event; // Return the modified event }, }); Important Notes 1. Guaranteed Last Execution: beforeSendTransaction is guaranteed to run last, after all other event processors have completed, ensuring you have the final version of the event [7]. 2. Dedicated Hook: You should not use beforeSend for transaction events; beforeSend is exclusively for error events [1]. 3. Breadcrumb Customization: If you need to filter or modify individual breadcrumbs as they are created (rather than when the transaction event is sent), consider using the beforeBreadcrumb hook instead [4][5].
Citations:
- 1: https://develop.sentry.dev/sdk/foundations/client/hooks/
- 2: https://develop.sentry.dev/sdk/foundations/envelopes/event-payloads/transaction/
- 3: https://develop.sentry.dev/sdk/foundations/envelopes/event-payloads/breadcrumbs/
- 4: https://docs.sentry.io/platforms/javascript/enriching-events/breadcrumbs/
- 5: https://docs.sentry.io/platforms/javascript/enriching-events/breadcrumbs.md
- 6: https://docs.sentry.io/platforms/javascript/guides/connect/data-management/sensitive-data.md
- 7: https://docs.sentry.io/platforms/javascript/configuration/apis.md
🌐 Web query:
Sentry transaction event payload extra property javascript beforeSendTransaction extra
💡 Result:
In Sentry's JavaScript SDK, you can interact with the extra property of a transaction event payload within the beforeSendTransaction hook by directly mutating the event object passed to it [1][2][3]. The beforeSendTransaction hook is a configuration option available in transaction mode that is guaranteed to run last, after all other event processors [4][5][1]. It receives the transaction event object as its first argument, which includes an extra property (if present) that holds an arbitrary mapping of additional metadata [6][7][1]. Example usage: Sentry.init({ dsn: "https://@o.ingest.sentry.io/", beforeSendTransaction(event) { // Access and mutate the extra property event.extra = {...event.extra, customKey: "customValue" }; // You can also delete or modify existing entries delete event.extra.sensitiveData; return event; }, }); Key points to consider: 1. Data Availability: The extra field is a standard part of the Sentry event payload schema [6][8]. If you intend to add data, ensure the object structure aligns with the expected format [6]. 2. Alternative Approaches: While beforeSendTransaction is useful for final modifications or scrubbing [9][3], you can also use addEventProcessor or scope.setExtra to enrich events with additional data throughout the transaction's lifecycle [4][5]. 3. UI Considerations: Note that depending on the Sentry version and configuration, how extra data is rendered in the Sentry UI can vary, and in some contexts, attaching data to individual spans may be preferred over attaching it to the top-level transaction [10]. 4. Stream Mode: If your SDK is configured in "stream mode" rather than "transaction mode," beforeSendTransaction will not be invoked [9][1]. In that case, you should use beforeSendSpan to manage data on spans instead [9].
Citations:
- 1: https://docs.sentry.io/platforms/javascript/configuration/filtering/
- 2: https://docs.sentry.io/platforms/javascript/guides/connect/enriching-events/transaction-name.md
- 3: https://docs.sentry.io/platforms/javascript/guides/connect/data-management/sensitive-data.md
- 4: https://docs.sentry.io/platforms/javascript/configuration/apis/
- 5: https://docs.sentry.io/platforms/javascript/enriching-events/event-processors/
- 6: https://develop.sentry.dev/sdk/foundations/transport/event-payloads/
- 7: https://develop.sentry.dev/sdk/foundations/envelopes/event-payloads/
- 8: https://docs.rs/sentry/latest/sentry/protocol/struct.Transaction.html
- 9: https://docs.sentry.io/platforms/javascript/data-management/sensitive-data.md
- 10: "Additional Data" event field not rendered in the UI. getsentry/sentry#81417
🌐 Web query:
Sentry transaction event payload extra breadcrumbs fields documentation
💡 Result:
In Sentry event payloads, breadcrumbs are structured as part of the Breadcrumbs Interface [1][2]. While the overall event payload may contain an extra field for arbitrary metadata [1], the breadcrumb objects themselves utilize a specific data field to store arbitrary associated information [3][4]. Breadcrumb Object Structure Each breadcrumb object within the breadcrumbs array typically contains the following fields [3][5]: - type: An optional string defining the breadcrumb type (e.g., default, navigation, http, info, ui) [3][6][7]. - category: An optional string describing the category of the event (e.g., sentry.transaction, console, ui.click) [3][6][7]. - message: An optional string message describing the event [3][5]. - level: An optional string indicating the severity level (e.g., info, error, debug) [3][7]. - timestamp: An optional timestamp indicating when the breadcrumb occurred (RFC 3339 or Unix timestamp) [3][7]. - data: An optional dictionary/object for arbitrary metadata [3][4][5]. The contents of this field depend on the breadcrumb type [3][7]. Important Considerations - Additional Fields: While you can include other keys in a breadcrumb object, Sentry will drop any fields that are not recognized or supported by the breadcrumbs interface during processing [4]. - Data Field Usage: The data field is the designated location for adding custom, structured information to a breadcrumb [4]. For certain types, such as http or navigation, specific keys within data (e.g., url, method, from, to) are expected [6][7]. - Transaction Events: Transactions can also contain breadcrumbs. Internally, breadcrumbs of type default with categories like sentry.transaction or sentry.event are displayed as transaction type breadcrumbs [3][6]. For developers needing to manipulate these fields, SDKs generally provide a before_breadcrumb hook, which allows you to inspect and modify breadcrumbs—including the data dictionary—before they are sent to Sentry [4].
Citations:
- 1: https://develop.sentry.dev/sdk/foundations/transport/event-payloads/
- 2: https://develop.sentry.dev/sdk/foundations/envelopes/event-payloads/
- 3: https://develop.sentry.dev/sdk/foundations/envelopes/event-payloads/breadcrumbs/
- 4: https://docs.sentry.io/platforms/python/enriching-events/breadcrumbs/
- 5: https://getsentry.github.io/relay/relay_event_schema/protocol/struct.Breadcrumb.html
- 6: https://develop.sentry.dev/sdk/foundations/state-management/scopes/breadcrumbs.md
- 7: https://develop.sentry.dev/sdk/foundations/state-management/scopes/breadcrumbs/
Scrub nested breadcrumb/extra values in both hooks
breadcrumbs.data and extra are only redacted one level deep here, so nested objects can still leak tokens or email addresses. sentryBeforeSendTransaction also skips breadcrumb/extra scrubbing entirely, so transaction events can bypass the same redaction path. Reuse a recursive redactor in both hooks to keep error and transaction payloads consistent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sentry.shared.config.js` around lines 63 - 96, Replace the shallow value
handling in scrubBreadcrumbs and scrubExtra with the shared recursive redactor,
preserving cookie-key removal and string scrubbing at every nested level. Apply
these same scrubbers to breadcrumbs and extra data in
sentryBeforeSendTransaction, matching the existing sentryBeforeSend behavior for
error events.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.npmrc (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid globally enabling
legacy-peer-deps.This makes npm ignore peer-dependency conflicts, potentially allowing an incompatible dependency tree and masking the real package-version issue. Prefer fixing dependency ranges or using this only in a narrowly documented, verified CI workaround.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.npmrc at line 1, Remove the global legacy-peer-deps setting from .npmrc. Resolve any resulting peer-dependency conflicts by correcting the affected package version ranges; only retain a narrowly scoped, documented CI workaround if dependency alignment cannot address the verified conflict.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/account/settings/page.jsx`:
- Around line 62-70: Update handleInstall so installable is reset when the
native prompt is dismissed as well as when it is accepted. Preserve clearing
deferredPrompt after userChoice, ensuring the banner cannot remain visible while
the prompt reference is unavailable.
In `@app/layout.js`:
- Around line 21-22: Update the manifest value in the layout metadata to
reference the actual `/manifest.webmanifest` endpoint instead of `/manifest`,
while leaving the surrounding icons configuration unchanged.
In `@app/sw.js`:
- Around line 15-148: Prefix every explicit runtime cacheName in the
runtimeCaching configuration with “serwist-”, including google-fonts,
static-font-assets, next-static-assets, next-static-images,
next-optimized-images, book-previews, cloudinary-images, pages, and
same-origin-others. Keep the existing cache strategies and expiration settings
unchanged so logout cleanup can remove all runtime caches through its serwist-
prefix filter.
- Around line 116-130: Update the HTML navigation matcher in the service worker
to inspect the response Content-Type rather than the incoming request header,
while retaining the sameOrigin restriction. Ensure same-origin HTML responses
are routed to the existing NetworkFirst “pages” cache and non-HTML resources
continue to use the fallback rule.
---
Nitpick comments:
In @.npmrc:
- Line 1: Remove the global legacy-peer-deps setting from .npmrc. Resolve any
resulting peer-dependency conflicts by correcting the affected package version
ranges; only retain a narrowly scoped, documented CI workaround if dependency
alignment cannot address the verified conflict.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8282331d-b58e-4beb-9eb3-8fb4b2d97ca3
⛔ Files ignored due to path filters (11)
docs/screenshots/courses.pngis excluded by!**/*.pngdocs/screenshots/dashboard.pngis excluded by!**/*.pngdocs/screenshots/landing.pngis excluded by!**/*.pngdocs/screenshots/library.pngis excluded by!**/*.pngdocs/screenshots/login.pngis excluded by!**/*.pngdocs/screenshots/wallet.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.jsonpublic/icons/icon-192x192-maskable.pngis excluded by!**/*.pngpublic/icons/icon-192x192.pngis excluded by!**/*.pngpublic/icons/icon-512x512-maskable.pngis excluded by!**/*.pngpublic/icons/icon-512x512.pngis excluded by!**/*.png
📒 Files selected for processing (12)
.npmrcREADME.mdapp/account/settings/page.jsxapp/layout.jsapp/manifest.jsapp/offline/page.jsxapp/sw.jshooks/useAuth.jsnext.config.mjspackage.jsonpublic/sw.jspublic/swe-worker-f61931bc2770d10b.js
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- hooks/useAuth.js
- next.config.mjs
| const handleInstall = async () => { | ||
| if (!deferredPrompt) return; | ||
| deferredPrompt.prompt(); | ||
| const result = await deferredPrompt.userChoice; | ||
| if (result.outcome === "accepted") { | ||
| setInstallable(false); | ||
| } | ||
| setDeferredPrompt(null); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Install banner keeps showing but silently stops working after a dismissed prompt.
installable is only reset when outcome === "accepted". If the user dismisses the native prompt, deferredPrompt is nulled but installable stays true, so a subsequent click on "Install" silently no-ops (the !deferredPrompt guard returns early). The banner should also hide (or re-arm) after a dismissal.
♻️ Proposed fix
const handleInstall = async () => {
if (!deferredPrompt) return;
deferredPrompt.prompt();
const result = await deferredPrompt.userChoice;
- if (result.outcome === "accepted") {
- setInstallable(false);
- }
+ setInstallable(false);
setDeferredPrompt(null);
};📝 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 handleInstall = async () => { | |
| if (!deferredPrompt) return; | |
| deferredPrompt.prompt(); | |
| const result = await deferredPrompt.userChoice; | |
| if (result.outcome === "accepted") { | |
| setInstallable(false); | |
| } | |
| setDeferredPrompt(null); | |
| }; | |
| const handleInstall = async () => { | |
| if (!deferredPrompt) return; | |
| deferredPrompt.prompt(); | |
| const result = await deferredPrompt.userChoice; | |
| setInstallable(false); | |
| setDeferredPrompt(null); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/account/settings/page.jsx` around lines 62 - 70, Update handleInstall so
installable is reset when the native prompt is dismissed as well as when it is
accepted. Preserve clearing deferredPrompt after userChoice, ensuring the banner
cannot remain visible while the prompt reference is unavailable.
| manifest: "/manifest", | ||
| icons: { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files 'app/layout.js' 'app/**/manifest.*' 'public/sw.js'
printf '\n== outline: app/layout.js ==\n'
ast-grep outline app/layout.js --view expanded || true
printf '\n== relevant layout lines ==\n'
sed -n '1,120p' app/layout.js
printf '\n== manifest files ==\n'
for f in $(git ls-files 'app/**/manifest.*'); do
echo "--- $f ---"
wc -l "$f"
sed -n '1,220p' "$f"
done
printf '\n== sw.js manifest references ==\n'
rg -n 'manifest|webmanifest' public/sw.js app -g '!**/node_modules/**' || trueRepository: Deen-Bridge/dnb-frontend
Length of output: 33106
Fix the manifest URL
app/manifest.js is served as /manifest.webmanifest, so manifest: "/manifest" points at the wrong path and breaks the manifest link for installability.
🐛 Proposed fix
- manifest: "/manifest",
+ manifest: "/manifest.webmanifest",📝 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.
| manifest: "/manifest", | |
| icons: { | |
| manifest: "/manifest.webmanifest", | |
| icons: { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/layout.js` around lines 21 - 22, Update the manifest value in the layout
metadata to reference the actual `/manifest.webmanifest` endpoint instead of
`/manifest`, while leaving the surrounding icons configuration unchanged.
| runtimeCaching: [ | ||
| { | ||
| matcher: /^https:\/\/fonts\.(?:gstatic|googleapis)\.com\/.*/i, | ||
| handler: new CacheFirst({ | ||
| cacheName: "google-fonts", | ||
| plugins: [ | ||
| new ExpirationPlugin({ | ||
| maxEntries: 8, | ||
| maxAgeSeconds: 365 * 24 * 60 * 60, | ||
| maxAgeFrom: "last-used", | ||
| }), | ||
| ], | ||
| }), | ||
| }, | ||
| { | ||
| matcher: /\.(?:eot|otf|ttc|ttf|woff|woff2|font\.css)$/i, | ||
| handler: new CacheFirst({ | ||
| cacheName: "static-font-assets", | ||
| plugins: [ | ||
| new ExpirationPlugin({ | ||
| maxEntries: 8, | ||
| maxAgeSeconds: 30 * 24 * 60 * 60, | ||
| maxAgeFrom: "last-used", | ||
| }), | ||
| ], | ||
| }), | ||
| }, | ||
| { | ||
| matcher: /\/_next\/static\/.+\.(?:js|css|json)$/i, | ||
| handler: new CacheFirst({ | ||
| cacheName: "next-static-assets", | ||
| plugins: [ | ||
| new ExpirationPlugin({ | ||
| maxEntries: 64, | ||
| maxAgeSeconds: 30 * 24 * 60 * 60, | ||
| maxAgeFrom: "last-used", | ||
| }), | ||
| ], | ||
| }), | ||
| }, | ||
| { | ||
| matcher: /\/_next\/static\/media\/.+\.(?:png|jpg|jpeg|gif|svg|ico|webp|avif)$/i, | ||
| handler: new CacheFirst({ | ||
| cacheName: "next-static-images", | ||
| plugins: [ | ||
| new ExpirationPlugin({ | ||
| maxEntries: 64, | ||
| maxAgeSeconds: 30 * 24 * 60 * 60, | ||
| maxAgeFrom: "last-used", | ||
| }), | ||
| ], | ||
| }), | ||
| }, | ||
| { | ||
| matcher: /\/_next\/image\?url=.+$/i, | ||
| handler: new StaleWhileRevalidate({ | ||
| cacheName: "next-optimized-images", | ||
| plugins: [ | ||
| new ExpirationPlugin({ | ||
| maxEntries: 64, | ||
| maxAgeSeconds: 30 * 24 * 60 * 60, | ||
| maxAgeFrom: "last-used", | ||
| }), | ||
| ], | ||
| }), | ||
| }, | ||
| { | ||
| matcher: /\/api\/books\/[^/]+\/preview/i, | ||
| handler: new StaleWhileRevalidate({ | ||
| cacheName: "book-previews", | ||
| plugins: [ | ||
| new ExpirationPlugin({ | ||
| maxEntries: 10, | ||
| maxAgeSeconds: 7 * 24 * 60 * 60, | ||
| maxAgeFrom: "last-used", | ||
| }), | ||
| ], | ||
| }), | ||
| }, | ||
| { | ||
| matcher: /^https:\/\/res\.cloudinary\.com\/.*/i, | ||
| handler: new StaleWhileRevalidate({ | ||
| cacheName: "cloudinary-images", | ||
| plugins: [ | ||
| new ExpirationPlugin({ | ||
| maxEntries: 48, | ||
| maxAgeSeconds: 7 * 24 * 60 * 60, | ||
| maxAgeFrom: "last-used", | ||
| }), | ||
| ], | ||
| }), | ||
| }, | ||
| { | ||
| matcher: ({ sameOrigin, url }) => | ||
| sameOrigin && url.pathname.startsWith("/api/"), | ||
| handler: new NetworkOnly(), | ||
| }, | ||
| { | ||
| matcher: ({ sameOrigin }) => !sameOrigin, | ||
| handler: new NetworkOnly(), | ||
| }, | ||
| { | ||
| matcher: ({ request, url, sameOrigin }) => | ||
| request.headers.get("Content-Type")?.includes("text/html") && | ||
| sameOrigin, | ||
| handler: new NetworkFirst({ | ||
| cacheName: "pages", | ||
| plugins: [ | ||
| new ExpirationPlugin({ | ||
| maxEntries: 32, | ||
| maxAgeSeconds: 24 * 60 * 60, | ||
| maxAgeFrom: "last-used", | ||
| }), | ||
| ], | ||
| }), | ||
| }, | ||
| { | ||
| matcher: ({ sameOrigin, url }) => sameOrigin, | ||
| handler: new NetworkFirst({ | ||
| cacheName: "same-origin-others", | ||
| plugins: [ | ||
| new ExpirationPlugin({ | ||
| maxEntries: 32, | ||
| maxAgeSeconds: 24 * 60 * 60, | ||
| maxAgeFrom: "last-used", | ||
| }), | ||
| ], | ||
| }), | ||
| }, | ||
| { | ||
| matcher: /.*/i, | ||
| handler: new NetworkOnly(), | ||
| }, | ||
| ], |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Runtime cache names aren't prefixed with serwist-, breaking the logout cleanup in hooks/useAuth.js.
Serwist's getRuntimeName returns the supplied cacheName verbatim whenever one is provided (confirmed in the compiled public/sw.js), so none of these caches ("google-fonts", "static-font-assets", "next-static-assets", "next-static-images", "next-optimized-images", "cloudinary-images", "pages", "same-origin-others") get a serwist- prefix — only the internal precache cache does.
Per the provided snippet, hooks/useAuth.js's logout handler only deletes caches matching name.startsWith("serwist-") || name === "book-previews". That means every runtime cache defined here except book-previews survives logout, including pages and same-origin-others, which can hold previously-authenticated HTML responses — a real risk on shared/public devices.
Fix at the root: either prefix these cache names consistently (e.g. serwist-pages, serwist-same-origin-others, ...) or update the logout filter in hooks/useAuth.js to enumerate/match all cache names actually used here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/sw.js` around lines 15 - 148, Prefix every explicit runtime cacheName in
the runtimeCaching configuration with “serwist-”, including google-fonts,
static-font-assets, next-static-assets, next-static-images,
next-optimized-images, book-previews, cloudinary-images, pages, and
same-origin-others. Keep the existing cache strategies and expiration settings
unchanged so logout cleanup can remove all runtime caches through its serwist-
prefix filter.
| { | ||
| matcher: ({ request, url, sameOrigin }) => | ||
| request.headers.get("Content-Type")?.includes("text/html") && | ||
| sameOrigin, | ||
| handler: new NetworkFirst({ | ||
| cacheName: "pages", | ||
| plugins: [ | ||
| new ExpirationPlugin({ | ||
| maxEntries: 32, | ||
| maxAgeSeconds: 24 * 60 * 60, | ||
| maxAgeFrom: "last-used", | ||
| }), | ||
| ], | ||
| }), | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
HTML-page matcher checks the wrong header — navigations never carry Content-Type.
request.headers.get("Content-Type") is checked on the incoming request, but GET/navigation requests don't send a Content-Type request header (that describes a request body, which GETs don't have). This condition is effectively always falsy, so the "pages" cache bucket is dead code and all same-origin HTML falls through to the "same-origin-others" rule below, diluting its 32-entry limit with mixed resource types.
♻️ Proposed fix
{
- matcher: ({ request, url, sameOrigin }) =>
- request.headers.get("Content-Type")?.includes("text/html") &&
- sameOrigin,
+ matcher: ({ request, sameOrigin }) =>
+ sameOrigin && request.destination === "document",
handler: new NetworkFirst({
cacheName: "pages",📝 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.
| { | |
| matcher: ({ request, url, sameOrigin }) => | |
| request.headers.get("Content-Type")?.includes("text/html") && | |
| sameOrigin, | |
| handler: new NetworkFirst({ | |
| cacheName: "pages", | |
| plugins: [ | |
| new ExpirationPlugin({ | |
| maxEntries: 32, | |
| maxAgeSeconds: 24 * 60 * 60, | |
| maxAgeFrom: "last-used", | |
| }), | |
| ], | |
| }), | |
| }, | |
| { | |
| matcher: ({ request, sameOrigin }) => | |
| sameOrigin && request.destination === "document", | |
| handler: new NetworkFirst({ | |
| cacheName: "pages", | |
| plugins: [ | |
| new ExpirationPlugin({ | |
| maxEntries: 32, | |
| maxAgeSeconds: 24 * 60 * 60, | |
| maxAgeFrom: "last-used", | |
| }), | |
| ], | |
| }), | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/sw.js` around lines 116 - 130, Update the HTML navigation matcher in the
service worker to inspect the response Content-Type rather than the incoming
request header, while retaining the sameOrigin restriction. Ensure same-origin
HTML responses are routed to the existing NetworkFirst “pages” cache and
non-HTML resources continue to use the fallback rule.
|
Strict review blocker: this branch conflicts with the base branch, |
|
Strict review blocker: this branch has merge conflicts with the base branch and/or failing deployment checks. Please rebase, resolve conflicts, and restore checks before requesting merge. |
|
Strict review blocker: this branch conflicts with the base branch, is failing, and changes have been requested. Please resolve these before requesting merge. |
|
@Victorjonah-prog this PR has merge conflicts with the |
3 participants
… (closes #109)
Summary by CodeRabbit