Conversation
|
Warning Rate limit exceeded@DevlinRocha has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 59 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis pull request addresses multiple enhancements and clarifications across the codebase. In the service worker, error handling around network fetches has been improved with a try…catch block. A resource list generation script has been simplified by removing cache array constructions. The API server’s CORS configuration now permits an additional origin. The Vite dependency version in the frontend has been updated, and several blog-related components have been modified to pass context-specific error messages to the DefaultNotFound component, which itself now accepts an optional message prop. Changes
Possibly related PRs
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Deploying devlinrocha with
|
| Latest commit: |
74b697e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://582ba582.devlinrocha.pages.dev |
| Branch Preview URL: | https://staging.devlinrocha.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/frontend/public/offline.html(1 hunks)apps/frontend/public/service-worker.js(1 hunks)apps/frontend/scripts/generate-resource-list.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: devlinrocha
🔇 Additional comments (3)
apps/frontend/scripts/generate-resource-list.js (1)
17-17: Good addition of offline.html to the cache listAdding the offline page to the routesToCache array ensures it will be available when users are offline, which perfectly aligns with the PR objective.
apps/frontend/public/offline.html (1)
1-78: Nice offline page design with helpful UI elementsThe offline page has a clean, minimalist design that clearly communicates the network status to users. The emoticon adds a human touch, and providing both home and refresh options gives users clear actions to take.
apps/frontend/public/service-worker.js (1)
96-106: Good implementation of offline fallbackThe try-catch block effectively handles network failures and provides the offline.html page as a fallback. This creates a more robust user experience when the network is unavailable.
The error logging is thorough and will be helpful for debugging network issues, and using the cached offline page as a fallback ensures users always see something useful.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/frontend/public/offline.html(1 hunks)apps/frontend/public/service-worker.js(1 hunks)apps/frontend/scripts/generate-resource-list.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: devlinrocha
🔇 Additional comments (1)
apps/frontend/scripts/generate-resource-list.js (1)
17-17: Offline page added to cache listAdding
/offline.htmlto theroutesToCachearray ensures that the offline page will be available when the user is offline, which is essential for the improved offline user experience.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/frontend/public/service-worker.js (1)
43-52: 💡 Verification agent🧩 Analysis chain
Verify offline.html is included in the cache list.
Ensure that 'offline.html' is properly included in the RESOURCE_LIST that's being cached during the service worker installation. According to the summary, it was added to the routesToCache array in generate-resource-list.js, but we should verify this.
Run the following to check if offline.html is included in the resources to be cached:
🏁 Script executed:
#!/bin/bash # Check if offline.html is included in resource-list.js echo "Checking if offline.html is included in RESOURCE_LIST..." if grep -q "'/offline.html'" apps/frontend/public/resource-list.js; then echo "✅ '/offline.html' is included in resource-list.js" else echo "❌ '/offline.html' is NOT found in resource-list.js" fiLength of output: 382
Action Required: Verify Offline Fallback Caching
Our automated check couldn’t locate the expected resource file at
apps/frontend/public/resource-list.js, so we weren't able to verify if/offline.htmlis included in the cached resources. Please confirm the following:
- Verify whether the file that defines
RESOURCE_LISTis generated or located at a different path. If it’s generated (e.g., bygenerate-resource-list.js), ensure that the generated resource list includes/offline.html.- Update the service worker or caching configuration accordingly if the resource file’s location or generation logic has changed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/public/service-worker.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: devlinrocha
🔇 Additional comments (1)
apps/frontend/public/service-worker.js (1)
96-111: Good error handling implementation with room for improvement.The implementation of try/catch for network requests with a fallback to the offline page is a solid approach. This ensures users will see a friendly offline message rather than a browser error.
Two suggestions that would enhance this implementation:
- Add request type checking before returning the offline HTML page, as returning HTML for non-HTML resources (like images, CSS, JS) could cause issues
- Provide a text-based fallback response if the offline page isn't found in cache
try { return await handleFetch(event.request, cache) } catch (error) { console.error( `Failed to fetch from network for ${event.request.url}:`, error ) const offline = await caches.match('/offline.html') + // Only return the offline page for navigation or HTML requests + if (event.request.mode === 'navigate' || + (event.request.headers.get('accept') && + event.request.headers.get('accept').includes('text/html'))) { + if (offline) { return new Response(offline, { status: 503, statusText: 'Service Unavailable', headers: { 'Content-Type': 'text/html' }, }) + } + } + // Return a plain text response for non-HTML resources or if offline page not found + return new Response('Network error, resource unavailable', { + status: 503, + statusText: 'Service Unavailable', + headers: { 'Content-Type': 'text/plain' } + }) }
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/public/service-worker.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: devlinrocha
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/frontend/public/service-worker.js (1)
111-118: 🛠️ Refactor suggestionAdd null check and request type verification before serving offline page
The current implementation doesn't check if the offline page exists in the cache and serves HTML content for all request types (including images, CSS, JS), which can cause issues.
-const offlineResponse = await cache.match('/offline') -const offlineBody = await offlineResponse.text() - -return new Response(offlineBody, { - status: 503, - statusText: 'Service Unavailable', - headers: { 'Content-Type': 'text/html' }, -}) +// Only return HTML for navigation or HTML requests +if (event.request.mode === 'navigate' || + (event.request.headers.get('accept') && + event.request.headers.get('accept').includes('text/html'))) { + + const offlineResponse = await cache.match('/offline') + if (offlineResponse) { + const offlineBody = await offlineResponse.text() + return new Response(offlineBody, { + status: 503, + statusText: 'Service Unavailable', + headers: { 'Content-Type': 'text/html' }, + }) + } +} + +// For non-HTML requests or if offline page isn't available, return a simple error response +return new Response('Network error', { + status: 503, + statusText: 'Service Unavailable' +})This implementation:
- Checks if the request is for HTML content before returning HTML
- Checks if the offline page exists in the cache
- Provides appropriate responses for different content types
🧰 Tools
🪛 GitHub Actions: CI/CD Pipeline
[error] Command failed with exit code 1: vite build && node scripts/generate-resource-list
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/frontend/public/service-worker.js(3 hunks)apps/frontend/scripts/generate-resource-list.js(1 hunks)apps/frontend/src/assets/offline.html(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI/CD Pipeline
apps/frontend/scripts/generate-resource-list.js
[error] 30-30: Error: ENOENT: no such file or directory, open '/home/runner/work/portfolio/portfolio/apps/frontend/dist/offline.html'
[error] Command failed with exit code 1: vite build && node scripts/generate-resource-list
apps/frontend/public/service-worker.js
[error] Command failed with exit code 1: vite build && node scripts/generate-resource-list
apps/frontend/src/assets/offline.html
[error] Command failed with exit code 1: vite build && node scripts/generate-resource-list
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: devlinrocha
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/src/assets/offline.html(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI/CD Pipeline
apps/frontend/src/assets/offline.html
[error] Command failed with exit code 1: vite build && node scripts/generate-resource-list
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: devlinrocha
🔇 Additional comments (1)
apps/frontend/src/assets/offline.html (1)
1-7: Standard HTML Structure Confirmed
The file correctly implements the basic HTML structure including the DOCTYPE,<html lang="en">, meta charset, viewport meta tag, and a<title>declaration. This addresses previous concerns regarding browser compatibility.🧰 Tools
🪛 GitHub Actions: CI/CD Pipeline
[error] Command failed with exit code 1: vite build && node scripts/generate-resource-list
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/scripts/generate-resource-list.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: devlinrocha
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/public/service-worker.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: devlinrocha
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/public/service-worker.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: devlinrocha
🔇 Additional comments (2)
apps/frontend/public/service-worker.js (2)
48-50: Good addition of offline page caching during installationThe implementation correctly fetches and caches the offline HTML page during service worker installation. This ensures the offline experience will be available even if the user goes offline before navigating to any pages.
107-114: Enhance offline fallback logic with content type checking and null handling.The error handling for network failures is good, but there are two issues that should be addressed:
- There's no null check before using
offlineResponse- The code returns HTML content for all request types, which can cause issues for non-HTML resources like images, CSS, and JavaScript files
try { return await handleFetch(event.request, cache) } catch (error) { console.error( `Failed to fetch from network for ${event.request.url}:`, error ) const offlineResponse = await cache.match('/offline') - const offlineBody = await offlineResponse.text() - - return new Response(offlineBody, { - status: 503, - statusText: 'Service Unavailable', - headers: { 'Content-Type': 'text/html' }, - }) + + // Only return HTML for navigation or HTML requests + if (event.request.mode === 'navigate' || + (event.request.headers.get('accept') && + event.request.headers.get('accept').includes('text/html'))) { + + if (offlineResponse) { + const offlineBody = await offlineResponse.text() + return new Response(offlineBody, { + status: 503, + statusText: 'Service Unavailable', + headers: { 'Content-Type': 'text/html' }, + }) + } + } + + // For non-HTML requests or if offline page is not available + return new Response('Network error', { + status: 503, + statusText: 'Service Unavailable' + }) }This implementation addresses both issues by:
- Checking if the request is for HTML content before returning HTML
- Adding a null check for the offline response
- Providing an appropriate fallback for other content types
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/frontend/public/service-worker.js (1)
43-52: 🛠️ Refactor suggestionAdd offline.html to the precached resources
To ensure the offline page is available when needed, you should add it to the resources that are cached during service worker installation.
addEventListener('install', (event) => { event.waitUntil( (async () => { const cache = await getCache() await cache.addAll(RESOURCE_LIST) + // Ensure offline.html is cached + await cache.add('/offline.html') })() ) skipWaiting() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/public/service-worker.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: devlinrocha
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/public/service-worker.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: devlinrocha
🔇 Additional comments (2)
apps/frontend/public/service-worker.js (2)
99-115: Enhance the offline fallback logic for non-HTML requests and add null checks.This code always returns HTML for any failed network request, even if the request is for static assets like images or scripts. Also, if
/offlineisn’t cached or is unavailable, callingofflineResponse.text()will fail. Checking if the request is HTML (e.g., viaevent.request.mode === 'navigate'orAcceptheaders) and verifyingofflineResponseis not null before returning it can prevent unexpected errors.
48-50:Details
❓ Verification inconclusive
Ensure that
/offlineis served as intended.You're fetching from
/offlineand caching the response here. If the application intends to serve an HTML file at/offline, confirm that the server (or hosting setup) indeed responds with an offline page. Otherwise, consider referencing/offline.htmlor adjusting your server routes/bundler configuration to avoid 404 errors or unexpected content types.Below is a script to locate references to “/offline” and verify any route or file references:
🏁 Script executed:
#!/bin/bash # Searching for references to '/offline' in the repository rg --context 5 '/offline'Length of output: 1646
Action: Verify
/offlineEndpoint ConfigurationWe're fetching from
/offlineand caching its response in the service worker. Please confirm that this endpoint reliably serves the intended offline content (typically an HTML page) when there is no network connectivity. If the intended offline fallback is an HTML file (for example, named/offline.html), update the service worker code or server configuration accordingly to avoid 404 errors or content mismatches.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/frontend/public/service-worker.js (1)
99-115: 🛠️ Refactor suggestionAdd content type checking before returning HTML response.
The current implementation returns HTML content for all request types, which can cause issues for non-HTML resources like images, CSS, and JavaScript files.
try { return await handleFetch(event.request, cache) } catch (error) { console.error( `Failed to fetch from network for ${event.request.url}:`, error ) + // Only return HTML for navigation or HTML requests + if (event.request.mode === 'navigate' || + (event.request.headers.get('accept') && + event.request.headers.get('accept').includes('text/html'))) { const offlineResponse = await cache.match('/offline') if (offlineResponse) { const offlineBody = await offlineResponse.text() return new Response(offlineBody, { status: 503, statusText: 'Service Unavailable', headers: { 'Content-Type': 'text/html' }, }) } + + // Fallback if offline page isn't in cache + return new Response( + '<main style="text-align: center; font-family: system-ui;"><h1>Network offline</h1><a href="/">Go home</a></main>', + { + status: 503, + statusText: 'Service Unavailable', + headers: { 'Content-Type': 'text/html' }, + } + ) + } + + // For non-HTML requests + return new Response('Network error', { + status: 503, + statusText: 'Service Unavailable' + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/frontend/public/service-worker.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages: devlinrocha
DevlinRocha
force-pushed
the
staging
branch
3 times, most recently
from
e8664bf to
74b697e
Compare
1 participant
createoffline.htmlfile to cache and respond with service workerviteto6.2.4<DefaultNotFound />takes inmessageprop