feat: add offline support with IndexedDB caching and service worker#66
Merged
Merged
Conversation
|
@StackFox is attempting to deploy a commit to the karan3431's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
🎉 Thanks for your contribution, @StackFox! Please make sure CI passes and the checklist in the PR template is complete. A maintainer will review this soon. — The AgroNavis team |
📝 WalkthroughWalkthroughAdds full PWA offline support: an IndexedDB persistence layer ( ChangesOffline PWA Support
Sequence Diagram(s)sequenceDiagram
participant Browser
participant ServiceWorker as Service Worker (sw.js)
participant farmApi
participant BackendAPI as Backend /api/*
participant IndexedDB
Browser->>farmApi: getFarms()
farmApi->>BackendAPI: GET /api/farms
alt Online — success
BackendAPI-->>farmApi: farm list
farmApi->>IndexedDB: saveOfflineFarms(farms)
farmApi-->>Browser: fresh farms
else Offline / network failure
BackendAPI-->>farmApi: error
farmApi->>IndexedDB: getOfflineFarms()
IndexedDB-->>farmApi: cached farms
farmApi-->>Browser: cached farms
end
Browser->>ServiceWorker: fetch /api/farms (Workbox CacheFirst)
ServiceWorker-->>Browser: cached or network response
Note over Browser: Goes offline
Browser->>Browser: offline event fires
Browser->>Browser: OfflineIndicator banner shown
Note over Browser: Back online
Browser->>Browser: online event fires
Browser->>Browser: OfflineIndicator banner hidden (delayed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
frontend/public/offline.html (1)
149-151: ⚡ Quick winMove inline handler to script block and check online status before reloading.
The inline
onclickattribute has two concerns:
- CSP compatibility: Inline event handlers can violate Content Security Policy directives, which many PWAs enforce for security.
- Poor UX when offline: Clicking "Try Again" while still offline will reload
offline.htmlagain, not reconnect to the main app. The button should check connectivity first or provide feedback.♻️ Refactor to script-based handler with online check
- <button onclick="window.location.reload()"> + <button id="retry-btn"> Try Again </button> </div> <script> + // Try Again button - check if online first + document.getElementById('retry-btn').addEventListener('click', () => { + if (navigator.onLine) { + window.location.href = '/'; + } else { + // Still offline - give user feedback + alert('Still offline. Please check your connection and try again.'); + } + }); + // Automatically redirect back to app when online🤖 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 `@frontend/public/offline.html` around lines 149 - 151, Remove the inline onclick attribute from the "Try Again" button and move the click handler to a script block at the bottom of the HTML file. In the script block, query the button element and attach an event listener that checks if the browser is online (using window.navigator.onLine) before calling window.location.reload(). If the device is offline, the handler should either display a message to the user or simply prevent the reload to avoid the poor UX of reloading offline.html again. This approach will satisfy Content Security Policy requirements and provide better user experience by avoiding unnecessary reloads while offline.frontend/src/components/OfflineIndicator.tsx (2)
16-23: 💤 Low valueSSR guards are redundant given dynamic import configuration.
The
typeof window === 'undefined'checks are unnecessary because this component is already dynamically imported with{ ssr: false }in_app.tsx. These guards add noise without providing additional safety.♻️ Simplify state initialization
- const [isOffline, setIsOffline] = useState(() => { - if (typeof window === 'undefined') return false; - return !navigator.onLine; - }); - const [show, setShow] = useState(() => { - if (typeof window === 'undefined') return false; - return !navigator.onLine; - }); + const [isOffline, setIsOffline] = useState(!navigator.onLine); + const [show, setShow] = useState(!navigator.onLine);🤖 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 `@frontend/src/components/OfflineIndicator.tsx` around lines 16 - 23, Remove the redundant `typeof window === 'undefined'` checks in both useState initializers for `isOffline` and `show` since the OfflineIndicator component is already dynamically imported with `{ ssr: false }` in `_app.tsx`, which guarantees it only runs in the browser. Simplify both state initializations to directly return `!navigator.onLine` without the SSR guard wrapper to reduce code noise.
14-50: ⚖️ Poor tradeoffConsider consolidating to a single state variable.
The component maintains two separate states (
isOfflineandshow) that largely track the same information. After fixing the render condition bug, you could simplify to a single state:
null: online, hidden'offline': offline, showing banner'reconnecting': connection restored, brief transition stateThis would make the component logic clearer and eliminate the confusion between the two states.
🤖 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 `@frontend/src/components/OfflineIndicator.tsx` around lines 14 - 50, Consolidate the two separate state variables isOffline and show into a single state variable that can represent three states: null (online, hidden), 'offline' (offline, showing banner), or 'reconnecting' (transition state after reconnection). Replace both useState calls with a single useState initialization that checks navigator.onLine and sets the appropriate initial value. Update the handleOnline event handler to set the state to 'reconnecting' instead of separately updating isOffline and show with a timeout, and update handleOffline to set the state to 'offline'. Finally, update the render condition to check the single state variable instead of the current !show || !isOffline logic, returning null when the state is null or 'reconnecting' and rendering the banner when the state is 'offline'.
🤖 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 `@frontend/next.config.mjs`:
- Around line 26-29: The urlPattern RegExp patterns in the Workbox cache
configuration (at lines 26, 42, and 58 with patterns like /^\/api\/farms/,
/^\/api\/crop-scans/, and /^\/api\//) will not match requests because Workbox
evaluates these patterns against the full request URL (including protocol and
domain) rather than just the path portion. Replace each of these RegExp
urlPattern definitions with a matchCallback function that extracts the pathname
from the request URL object and checks if it starts with the intended path
segment, for example using ({url}) => url.pathname.startsWith('/api/farms') for
the farms pattern and applying the same approach to the crop-scans and other API
patterns.
- Around line 23-71: The Workbox cache configuration in next.config.mjs caches
authenticated API responses in three caches (agronavis-api-farms,
agronavis-api-scans, agronavis-api-other) and an IndexedDB offline store exists
with farms, crop-scans, and fields stores, but these are not cleared on user
sign-out, creating a security vulnerability where subsequent users can access
previous users' cached data. Add cache and IndexedDB clearing logic to the
sign-out flow by updating the auth state change handler in either supabase.ts or
AuthContext.tsx to programmatically delete all three Workbox runtime caches by
name and clear the three IndexedDB stores when the user signs out.
In `@frontend/src/components/OfflineIndicator.tsx`:
- Around line 25-27: The OfflineIndicator component has a redundant state
initialization inside the useEffect hook that duplicates the initial state
already set in the useState declaration. Remove the
setIsOffline(!navigator.onLine) call from the useEffect hook since the isOffline
state is already properly initialized with the same value in the useState
initializer, eliminating the unnecessary re-render on mount.
- Around line 29-50: The render condition checking `!show || !isOffline` uses OR
logic which causes the component to unmount immediately when isOffline becomes
false in the handleOnline function, before the 300ms timeout can complete and
trigger the animation. Change the render condition from checking both states
with OR logic to only checking the `show` state with `if (!show) return null;`.
This allows the component to remain rendered during the 300ms delay set in
handleOnline, enabling the intended slide-out animation to display before the
component is hidden.
In `@frontend/src/lib/offlineStorage.ts`:
- Around line 73-94: The getDB function permanently caches the dbPromise without
error handling, so if the openDB call fails, subsequent calls reuse the same
rejected promise and never attempt to recover. Modify the dbPromise assignment
to include error handling that catches any errors from the openDB promise, logs
them appropriately, resets the dbPromise variable back to undefined (or null),
and then rethrows the error so that the next call to getDB will attempt to open
the database again instead of reusing the failed promise.
In `@frontend/src/utils/cropScanApi.ts`:
- Around line 69-70: The console.warn call in getScanHistory is logging the raw
error object which can expose sensitive information such as request headers and
configuration details. Extract and log only safe, non-sensitive information from
the error, such as the error message or HTTP status code, instead of passing the
entire error object to the console.warn statement.
- Around line 70-75: The condition checking the cached result from
getOfflineCropScans currently requires both that cached exists AND that it has
items before returning it. This causes empty arrays to be treated as cache
misses and throws an error instead of returning the valid empty result. Change
the condition to only verify that cached exists (is not null or undefined) by
removing the length check, so that empty cache arrays representing users with
zero historical scans are properly returned as valid offline data rather than
triggering an error.
In `@frontend/src/utils/farmApi.ts`:
- Around line 56-60: The offline fallback logic in farmApi.ts incorrectly gates
the cache return on both existence and non-empty length. When a successful sync
legitimately caches an empty list, subsequent offline calls will throw instead
of returning the empty array. In the error handling blocks around the two
locations mentioned (the condition at lines 56-58 and the similar condition at
lines 122-124), change the cache check from `if (cached && cached.length > 0)`
to `if (cached)` so that empty arrays are properly returned as the offline
fallback instead of throwing the error.
- Line 22: Remove the console.log(config) statement that logs the full Axios
request configuration, as it exposes sensitive information like auth headers.
Additionally, locate the error logging statements (around lines 54-55 and
120-121) in the error handler interceptors and replace the raw error object
logging with sanitized logging that only includes non-sensitive details such as
the error message, status code, or custom error messages. Ensure no auth
headers, request metadata, or full error objects are logged in any production
paths.
---
Nitpick comments:
In `@frontend/public/offline.html`:
- Around line 149-151: Remove the inline onclick attribute from the "Try Again"
button and move the click handler to a script block at the bottom of the HTML
file. In the script block, query the button element and attach an event listener
that checks if the browser is online (using window.navigator.onLine) before
calling window.location.reload(). If the device is offline, the handler should
either display a message to the user or simply prevent the reload to avoid the
poor UX of reloading offline.html again. This approach will satisfy Content
Security Policy requirements and provide better user experience by avoiding
unnecessary reloads while offline.
In `@frontend/src/components/OfflineIndicator.tsx`:
- Around line 16-23: Remove the redundant `typeof window === 'undefined'` checks
in both useState initializers for `isOffline` and `show` since the
OfflineIndicator component is already dynamically imported with `{ ssr: false }`
in `_app.tsx`, which guarantees it only runs in the browser. Simplify both state
initializations to directly return `!navigator.onLine` without the SSR guard
wrapper to reduce code noise.
- Around line 14-50: Consolidate the two separate state variables isOffline and
show into a single state variable that can represent three states: null (online,
hidden), 'offline' (offline, showing banner), or 'reconnecting' (transition
state after reconnection). Replace both useState calls with a single useState
initialization that checks navigator.onLine and sets the appropriate initial
value. Update the handleOnline event handler to set the state to 'reconnecting'
instead of separately updating isOffline and show with a timeout, and update
handleOffline to set the state to 'offline'. Finally, update the render
condition to check the single state variable instead of the current !show ||
!isOffline logic, returning null when the state is null or 'reconnecting' and
rendering the banner when the state is 'offline'.
🪄 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: cc772e61-8ac8-44ef-a722-75e11c3c992e
📒 Files selected for processing (7)
frontend/next.config.mjsfrontend/public/offline.htmlfrontend/src/components/OfflineIndicator.tsxfrontend/src/lib/offlineStorage.tsfrontend/src/pages/_app.tsxfrontend/src/utils/cropScanApi.tsfrontend/src/utils/farmApi.ts
StackFox
force-pushed
the
feat/offline-support
branch
from
827de8f to
49702ea
Compare
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 `@frontend/src/lib/supabase.ts`:
- Around line 64-67: The indexedDB.deleteDatabase() call in the conditional
block starting with if ('indexedDB' in window) returns an IDBOpenDBRequest which
is asynchronous, but the current code treats it as a fire-and-forget operation
without awaiting completion or handling its events. Wrap the
indexedDB.deleteDatabase() call in a Promise that resolves when the request's
onsuccess event fires or rejects when the onerror or onblocked events fire, then
await this Promise to ensure the offline database is fully deleted before
signOut() completes. This prevents abandoned database connections from blocking
the deletion and ensures offline data is properly cleaned up on sign-out.
🪄 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: 10a53dbe-f9e2-4fb9-bc50-29f5a2fe3554
📒 Files selected for processing (8)
frontend/next.config.mjsfrontend/public/offline.htmlfrontend/src/components/OfflineIndicator.tsxfrontend/src/lib/offlineStorage.tsfrontend/src/lib/supabase.tsfrontend/src/pages/_app.tsxfrontend/src/utils/cropScanApi.tsfrontend/src/utils/farmApi.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- frontend/public/offline.html
- frontend/src/pages/_app.tsx
- frontend/src/components/OfflineIndicator.tsx
- frontend/src/utils/cropScanApi.ts
- frontend/src/utils/farmApi.ts
- frontend/next.config.mjs
- frontend/src/lib/offlineStorage.ts
Comment on lines
+64
to
+67
| if ('indexedDB' in window) { | ||
| try { | ||
| indexedDB.deleteDatabase('agronavis-offline-db'); | ||
| } catch (e) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "supabase.ts" | grep frontendRepository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 111
🏁 Script executed:
find . -type f -name "offlineStorage.ts" | grep frontendRepository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 117
🏁 Script executed:
cat -n ./frontend/src/lib/supabase.tsRepository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 4399
🏁 Script executed:
cat -n ./frontend/src/lib/offlineStorage.ts | head -100Repository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 3388
🏁 Script executed:
# Search for getDB() usage to understand if open connections would be active
rg "getDB\(" ./frontend/src --type typescript --type tsx -A 2Repository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 115
🏁 Script executed:
# Verify IndexedDB API - check if there are any async/await patterns for IndexedDB operations in the codebase
rg "deleteDatabase|indexedDB\." ./frontend/src --type typescript --type tsx -B 2 -A 2Repository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 115
🏁 Script executed:
rg "getDB\(" ./frontend/src -A 2Repository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 1811
🏁 Script executed:
rg "deleteDatabase|indexedDB\." ./frontend/src -B 2 -A 2Repository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 403
🏁 Script executed:
# Check if signOut is awaited when called - look for usage patterns
rg "signOut\(" ./frontend/src -B 1 -A 1Repository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 713
🏁 Script executed:
# Verify the idb library being used and check its type definitions
rg "from 'idb'" ./frontend/src -B 1 -A 1Repository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 209
🏁 Script executed:
# Check if there are any other async IndexedDB operations in the codebase for pattern reference
rg "openDB|deleteDatabase|db\." ./frontend/src/lib/offlineStorage.ts | head -30Repository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 970
🏁 Script executed:
# Check package.json to confirm idb library version and capabilities
cat ./frontend/package.json | grep -A 5 '"idb"'Repository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 62
🏁 Script executed:
# Look for any tests or documentation about IndexedDB cleanup patterns
find ./frontend -name "*.md" -o -name "*.test.*" | xargs grep -l "IndexedDB\|deleteDatabase" 2>/dev/null || echo "No test/doc files found"Repository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 104
🏁 Script executed:
cat ./frontend/package.json | head -50Repository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 1517
🏁 Script executed:
# Check MDN documentation behavior by searching for any idb usage patterns or async handling
rg "idb\|IDBOpenDBRequest" ./frontend -A 3 -B 1Repository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 62
🏁 Script executed:
grep '"idb"' ./frontend/package.jsonRepository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 62
🏁 Script executed:
# Search for any idb library in package.json
cat ./frontend/package.json | grep -i idbRepository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 62
🏁 Script executed:
# Check if there's a root package.json with idb dependency
cat ./package.json 2>/dev/null | grep -A 2 '"idb"' || echo "Not in root"Repository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 92
🏁 Script executed:
# Check for monorepo structure or check all package.json files
find . -maxdepth 3 -name "package.json" -exec grep -l '"idb"' {} \;Repository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 62
🏁 Script executed:
# Check if idb is listed in the complete package.json
wc -l ./frontend/package.json && cat ./frontend/package.json | tail -30Repository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 994
🏁 Script executed:
# Let's check the import statement more carefully - maybe it's using native IndexedDB via 'idb' wrapper
rg "import.*idb|require.*idb" ./frontendRepository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 171
🏁 Script executed:
# Check if dbPromise is ever closed/cleared anywhere
rg "dbPromise.*=.*null|closeDB|db.close\(\)" ./frontend/srcRepository: jpdevhub/Agronavis-AI-Farm-Assistant
Length of output: 277
Await IndexedDB deletion; current call is fire-and-forget.
indexedDB.deleteDatabase(...) is async and returns an IDBOpenDBRequest. The current code does not await it, so signOut() resolves immediately while deletion remains pending or blocked by open DB connections (maintained by the module-level dbPromise in offlineStorage.ts). The try/catch only catches synchronous errors and does not handle the request's onerror, onsuccess, or onblocked events, leaving offline data potentially undeleted on sign-out.
Proposed fix
// Clear IndexedDB offline store (farm data, scans, fields)
if ('indexedDB' in window) {
try {
- indexedDB.deleteDatabase('agronavis-offline-db');
+ await new Promise<void>((resolve, reject) => {
+ const req = indexedDB.deleteDatabase('agronavis-offline-db');
+ req.onsuccess = () => resolve();
+ req.onerror = () => reject(req.error);
+ req.onblocked = () =>
+ reject(new Error('IndexedDB deletion blocked by open connections'));
+ });
} catch (e) {
console.warn('Failed to clear IndexedDB:', e);
}
}📝 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.
Suggested change
| if ('indexedDB' in window) { | |
| try { | |
| indexedDB.deleteDatabase('agronavis-offline-db'); | |
| } catch (e) { | |
| if ('indexedDB' in window) { | |
| try { | |
| await new Promise<void>((resolve, reject) => { | |
| const req = indexedDB.deleteDatabase('agronavis-offline-db'); | |
| req.onsuccess = () => resolve(); | |
| req.onerror = () => reject(req.error); | |
| req.onblocked = () => | |
| reject(new Error('IndexedDB deletion blocked by open connections')); | |
| }); | |
| } catch (e) { | |
| console.warn('Failed to clear IndexedDB:', e); | |
| } | |
| } |
🤖 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 `@frontend/src/lib/supabase.ts` around lines 64 - 67, The
indexedDB.deleteDatabase() call in the conditional block starting with if
('indexedDB' in window) returns an IDBOpenDBRequest which is asynchronous, but
the current code treats it as a fire-and-forget operation without awaiting
completion or handling its events. Wrap the indexedDB.deleteDatabase() call in a
Promise that resolves when the request's onsuccess event fires or rejects when
the onerror or onblocked events fire, then await this Promise to ensure the
offline database is fully deleted before signOut() completes. This prevents
abandoned database connections from blocking the deletion and ensures offline
data is properly cleaned up on sign-out.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Added full offline support with IndexedDB caching and service worker which helps farmers when they're out in the farms without an internet connection.
Related Issue
Closes #54
Changes
offlineStorage.ts): Created typed stores for farms, crop scans, and fields using theidbwrapper — data persists across sessionsfarmApi.ts,cropScanApi.ts): ModifiedgetFarms(),getFarmFields(), andgetScanHistory()to try network first, then fall back to IndexedDB when offlinenext.config.mjs): Added WorkboxruntimeCachingrules — CacheFirst for/api/farms*and/api/crop-scans*, NetworkFirst for other APIsOfflineIndicator.tsx): Red banner at top of screen when user is offline, persists across page reloadsoffline.html): Standalone HTML page shown when no cached content is available_app.tsx): Registered OfflineIndicator globally via dynamic import (SSR-safe)Testing
cd frontend && npm run build— compiles without errorsnpm run start→ open http://localhost:3000Checklist
.envvalues are committedSummary by CodeRabbit