feat: add PWA app shell and offline upload queue for mobile#307
Open
SOHALIYAJAY wants to merge 1 commit into
Open
feat: add PWA app shell and offline upload queue for mobile#307SOHALIYAJAY wants to merge 1 commit into
SOHALIYAJAY wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR implements offline upload capability with a PWA shell. It adds an IndexedDB-backed queue for storing files selected while offline, a service worker for PWA registration, automatic queue flushing when connectivity returns, and supporting UI states ("draft" status) to show queued uploads to the user. ChangesOffline Upload Queue with PWA Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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. 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 |
PR Context Summary
Suggested issue links
Use |
Contributor
ApprovabilityVerdict: Needs human review Unable to check for correctness in 4d96b28. This PR introduces a new PWA feature with offline upload queuing, service worker registration, and IndexedDB storage. New user-facing capabilities like offline workflows and background sync warrant human review. You can customize Macroscope's approvability policy. Learn more. |
Author
|
now i solved the issue, please review it, and merge it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
frontend/src/app/upload/page.tsx (2)
59-61: ⚡ Quick winDraft status display is correct, but
getStatusClasses()doesn't handle draft state.
getDisplayStatus()correctly returns"draft (offline)"for draft items (lines 59-61), butgetStatusClasses()(lines 137-151) has no case forprocessingState === "draft". Draft items will fall through to the default"accent-badge status-default"styling (line 150), which may not provide appropriate visual distinction from queued items. Users may not clearly see that these items are in a different (offline-queued) state.Add a specific style case for draft items in
getStatusClasses()to improve visual clarity.♻️ Optional improvement: add draft-specific styling
function getStatusClasses(item: UploadListItem) { + if (item.processingState === "draft") { + return "accent-badge status-pending"; + } if (item.status === "duplicate") { return "accent-badge status-pending"; }🤖 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/app/upload/page.tsx` around lines 59 - 61, getStatusClasses() is missing a branch for processingState === "draft", so draft items fall through to the default styling; add a specific case in getStatusClasses() that checks processingState === "draft" and returns a dedicated CSS class (e.g., "accent-badge status-draft" or similar) to match the display returned by getDisplayStatus() and visually distinguish offline/queued drafts from other states; update any corresponding mapping or switch inside getStatusClasses() (referencing the function name getStatusClasses and the processingState === "draft" condition used in getDisplayStatus) and ensure the new class name is used in the component's JSX/CSS so the styling takes effect.
342-342: ⚖️ Poor tradeoff
navigator.onLineis unreliable for detecting backend connectivity.Line 342 uses
navigator.onLineto decide whether to queue files offline or upload immediately. However,navigator.onLineonly indicates whether the device has a network connection, not whether the Find backend is reachable. The user might have internet access but the backend might be down, unreachable due to firewall rules, or on a different network segment. In these cases, files will attempt to upload immediately and fail, instead of being queued for later retry.While there's no perfect solution, consider detecting upload failures and offering to queue failed uploads for retry, or implement a backend health check before deciding to upload vs queue.
🤖 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/app/upload/page.tsx` at line 342, The check using navigator.onLine is unreliable for backend reachability; replace the direct reference to navigator.onLine in the upload decision (the conditional that currently uses navigator.onLine) with logic that attempts a short backend health check or proceeds to upload and treats any network/fetch failure as offline: call a lightweight endpoint (e.g., GET /api/health) or try the actual upload via the existing upload handler (e.g., uploadFiles or handleUpload) and on any fetch/network error or non-2xx response, call the offline enqueue function (e.g., queueFiles or enqueueUpload) and surface a retry option in the UI; ensure failed upload promises are caught, move the files to the offline queue, and update status so users can retry later.frontend/src/app/providers.tsx (1)
43-45: ⚡ Quick winService worker registration failure is silently ignored.
Line 44 uses
voidto intentionally ignore the registration promise, meaning any registration failure (e.g., SW script syntax error, browser security restrictions, or loading issues) will be completely silent. While SW registration failure is non-fatal to the app, it will prevent PWA installation and offline queueing from working, leaving users confused about why those features are unavailable.Consider adding
.catch()to log registration errors to the console for debugging, or use a toast notification if the user should be informed.♻️ Recommended fix: log registration errors
if ("serviceWorker" in navigator) { - void navigator.serviceWorker.register("/sw.js"); + navigator.serviceWorker.register("/sw.js").catch((err) => { + console.error("Service worker registration failed:", err); + }); }🤖 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/app/providers.tsx` around lines 43 - 45, The service worker registration promise is ignored (void navigator.serviceWorker.register("/sw.js")) so failures are silent; update the registration call that uses navigator.serviceWorker.register to attach a .catch handler that logs the error (e.g., via console.error or an app logger) and optionally handle success logging; ensure the change is applied inside the same conditional branch that checks "serviceWorker" in navigator so registration errors are surfaced for debugging or user notification.frontend/src/lib/uploadQueue.ts (1)
15-24: ⚡ Quick winCache the database connection to avoid reopening on every operation.
openDb()is called by every CRUD function (enqueue,getQueue,updateStatus,dequeue), causing repeatedindexedDB.open()calls. IndexedDB connections are heavyweight; repeatedly opening and not closing them can degrade performance and may exhaust browser resources under concurrent usage.Consider caching the database handle in a module-level promise so all operations share a single connection, or explicitly close the database after each transaction completes.
♻️ Recommended fix: cache the database connection
+let dbPromise: Promise<IDBDatabase> | null = null; + function openDb(): Promise<IDBDatabase> { + if (dbPromise) return dbPromise; + dbPromise = new Promise((resolve, reject) => { - return new Promise((resolve, reject) => { const req = indexedDB.open(DB_NAME, DB_VERSION); req.onupgradeneeded = () => { req.result.createObjectStore(STORE, { keyPath: "id" }); }; req.onsuccess = () => resolve(req.result); req.onerror = () => reject(req.error); }); + return dbPromise; }🤖 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/uploadQueue.ts` around lines 15 - 24, openDb() currently calls indexedDB.open() every time (used by enqueue, getQueue, updateStatus, dequeue), which is expensive; cache a single module-level Promise for the DB connection (e.g., let dbPromise: Promise<IDBDatabase> | null) and have openDb() initialize that promise once and return it for all callers so all CRUD functions reuse the same connection; ensure the cached DB handles version upgrades/close by listening for db.onversionchange (or clearing dbPromise on close/error) so the cache can be replaced when needed.
🤖 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/public/sw.js`:
- Around line 1-3: The service worker currently forwards every request in the
fetch handler (self.addEventListener("fetch", (e) =>
e.respondWith(fetch(e.request)))) so there is no app-shell precaching or runtime
caching; update the SW to implement precaching and a cache-first runtime
strategy for the app shell by adding an install handler that opens a named cache
(use caches.open), adds the static asset list (precache) via cache.addAll or
integration with the repo's suggested tool like `@serwist/next`, adjust activate
to clean old caches, and replace the fetch handler with logic that tries
caches.match(e.request) first (serve cached app-shell assets) then falls back to
fetch and populates runtime cache on success; if you intentionally only want
installability, add a comment in sw.js clarifying that offline app-shell caching
is out of scope.
In `@frontend/src/app/upload/page.tsx`:
- Around line 342-356: The code enqueues offline files but incorrectly marks
them with status: "uploaded" — update the upload status handling by adding a
"draft" state to the UploadResult["status"] union in frontend/src/lib/api.ts and
use that new value when creating draft list entries in the offline branch
(replace status: "uploaded" in the block that calls enqueue and
setUploadedFiles). Ensure UploadListItem consumers (e.g., the trackedUploads
useMemo, jobStatus logic, and activeJobs filters) treat "draft" as
not-yet-uploaded so they exclude drafts from uploaded/active counts and progress
calculations.
- Around line 342-356: The code casts a partial object to UploadListItem in the
offline branch (inside the loop over acceptedFiles where enqueue(file) and
setUploadedFiles are used), which bypasses type safety and can cause runtime
errors when UI consumes fields like media_id/job_id/hash; change the queued item
type to an explicit partial/draft type (e.g., DraftUploadItem or use
Partial<UploadListItem>) instead of as UploadListItem, update setUploadedFiles
call to store that draft type, and then update any UI rendering or consumers
that access UploadListItem fields (check for presence of result.media_id,
job_id, hash, etc.) to handle nullable/undefined values safely.
- Around line 308-334: The catch block in flush updates the IndexedDB status to
"failed" but doesn't update the uploadedFiles React state, so the UI still shows
"draft"; modify the catch in flush to call setUploadedFiles and map over
uploadedFiles to replace any entry that matches the queue item (use
item.filename and/or item.id) and has processingState === "draft" to set
processingState (or status) to "failed" (and include any failure metadata),
ensuring the UI reflects the failed state; reference the flush function,
updateStatus(item.id, "failed"), uploadedFiles and setUploadedFiles so you
update the in-memory state after the IndexedDB change.
- Around line 308-334: The flush() flow currently calls getQueue(),
updateStatus(), and dequeue() without guarding for IndexedDB errors; wrap calls
to getQueue(), each updateStatus(item.id, ...), and dequeue(item.id) in
try-catch blocks inside flush(), log errors (console.error or a user
notification) and ensure status consistency (e.g., if updateStatus(item.id,
"pending") succeeds but uploadMutation or dequeue fails, revert status back to
"draft" or "failed" as appropriate); reference the flush function and the calls
to getQueue, updateStatus, dequeue, uploadMutation.mutateAsync, and
setUploadedFiles when applying these changes.
- Around line 308-334: The flush function can run concurrently causing
double-uploads; add a guard (e.g., a module-level boolean or React ref like
isFlushingRef) checked at the start of flush and set to true before awaiting
async work, then set back to false in a finally block so concurrent invocations
(initial void flush(), online event handler, network flaps) return early while
another run is active; update the useEffect to close over the same guard so the
event listener and initial call share it and keep existing calls to getQueue,
updateStatus, uploadMutation.mutateAsync, dequeue, and setUploadedFiles
unchanged except for honoring the guard to prevent re-entrancy.
- Around line 308-334: The flush logic in useEffect (function flush) incorrectly
removes draft entries by matching filename, which can collide; add an optional
queueItemId field to the UploadListItem type, populate that queueItemId when
creating/enqueuing items in onDrop (use the id returned by the IndexedDB enqueue
call), and then in flush (after successful upload and dequeue) update
setUploadedFiles to filter by queueItemId === item.id (or remove the entry whose
queueItemId matches) instead of matching on filename and processingState; ensure
enqueue/dequeue/updateStatus calls return/accept the same item.id so the mapping
is reliable.
In `@frontend/src/lib/uploadQueue.ts`:
- Around line 26-42: The enqueue function currently builds QueueItem.id as
`${Date.now()}-${file.name}`, which can collide; update enqueue to generate a
robust unique id (e.g., call crypto.randomUUID() or append a random/counter
suffix) when constructing the QueueItem so each item saved by
tx.objectStore(STORE).put(item) cannot overwrite another; change the id
assignment inside enqueue (and any code that reads/relies on this id) to use the
new UUID-based value while keeping the rest of QueueItem fields the same.
- Around line 54-67: The updateStatus function currently resolves the Promise
inside req.onsuccess right after calling store.put(), which returns before the
IndexedDB transaction commits; change it to wait for the transaction to finish
by moving the resolve() into tx.oncomplete and add tx.onabort/tx.onerror
handlers to reject the Promise on failure (mirror the pattern used in
enqueue()/dequeue()); keep using openDb(), STORE, tx, store, req and ensure
req.onsuccess only performs the put() and does not resolve the outer Promise.
---
Nitpick comments:
In `@frontend/src/app/providers.tsx`:
- Around line 43-45: The service worker registration promise is ignored (void
navigator.serviceWorker.register("/sw.js")) so failures are silent; update the
registration call that uses navigator.serviceWorker.register to attach a .catch
handler that logs the error (e.g., via console.error or an app logger) and
optionally handle success logging; ensure the change is applied inside the same
conditional branch that checks "serviceWorker" in navigator so registration
errors are surfaced for debugging or user notification.
In `@frontend/src/app/upload/page.tsx`:
- Around line 59-61: getStatusClasses() is missing a branch for processingState
=== "draft", so draft items fall through to the default styling; add a specific
case in getStatusClasses() that checks processingState === "draft" and returns a
dedicated CSS class (e.g., "accent-badge status-draft" or similar) to match the
display returned by getDisplayStatus() and visually distinguish offline/queued
drafts from other states; update any corresponding mapping or switch inside
getStatusClasses() (referencing the function name getStatusClasses and the
processingState === "draft" condition used in getDisplayStatus) and ensure the
new class name is used in the component's JSX/CSS so the styling takes effect.
- Line 342: The check using navigator.onLine is unreliable for backend
reachability; replace the direct reference to navigator.onLine in the upload
decision (the conditional that currently uses navigator.onLine) with logic that
attempts a short backend health check or proceeds to upload and treats any
network/fetch failure as offline: call a lightweight endpoint (e.g., GET
/api/health) or try the actual upload via the existing upload handler (e.g.,
uploadFiles or handleUpload) and on any fetch/network error or non-2xx response,
call the offline enqueue function (e.g., queueFiles or enqueueUpload) and
surface a retry option in the UI; ensure failed upload promises are caught, move
the files to the offline queue, and update status so users can retry later.
In `@frontend/src/lib/uploadQueue.ts`:
- Around line 15-24: openDb() currently calls indexedDB.open() every time (used
by enqueue, getQueue, updateStatus, dequeue), which is expensive; cache a single
module-level Promise for the DB connection (e.g., let dbPromise:
Promise<IDBDatabase> | null) and have openDb() initialize that promise once and
return it for all callers so all CRUD functions reuse the same connection;
ensure the cached DB handles version upgrades/close by listening for
db.onversionchange (or clearing dbPromise on close/error) so the cache can be
replaced when needed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b94e706-82cc-4128-9dad-e238d797ecec
📒 Files selected for processing (7)
frontend/package.jsonfrontend/public/manifest.jsonfrontend/public/sw.jsfrontend/src/__tests__/uploadQueue.test.tsfrontend/src/app/providers.tsxfrontend/src/app/upload/page.tsxfrontend/src/lib/uploadQueue.ts
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
1 participant
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
Adds the initial PWA foundation for mobile usage by introducing an installable app shell, service worker support, and an IndexedDB-backed offline upload queue. Uploads selected while offline are stored locally and can be retried when connectivity is restored.
Fixes #259
Type of change
What changed
Screenshots / recordings (for UI changes)
Attach screenshots demonstrating:
How to test
Install dependencies:
Run the application:
Verify PWA behavior:
Verify offline upload queue:
Restore network connectivity:
Run project checks:
npm run lint npm run build npm testChecklist
GSSoC'26 checklist
Summary by CodeRabbit
New Features
Tests
Chores