feat(frontend): offline support via service worker - #571
Open
ebrahimgamdiwala wants to merge 19 commits into
Open
feat(frontend): offline support via service worker#571ebrahimgamdiwala wants to merge 19 commits into
ebrahimgamdiwala wants to merge 19 commits into
Conversation
Precache the app shell and serve cached data when the network is unavailable: - gameplan-sw.js: service worker that precaches built assets listed in a build-time manifest and serves them offline. - vite.config.ts: offlineAssetManifest() plugin emits gameplan-offline-assets.json (CSS/JS/font URLs) at build time. - offline.ts: registers the service worker and exposes isBrowserOffline() / isNetworkError() helpers. - data layer: add staleOnError + cacheKey to useList/useDoc so cached responses are served on network failure. - router.ts: hydrate community/space data from cache and skip route validation (NotFound) when offline or on network errors.
Show an unobtrusive pill while the browser is offline, and refetch feeds, unread counts, and open discussion timelines once connectivity returns, so content posted by others while offline shows up without a manual reload.
…en network is unreliable navigator.onLine can briefly lag the real network state right after a reload, so the home-route decision was racing communities/spaces IndexedDB hydration and occasionally sending an offline reload to onboarding instead of the cached feed. Treat a resource that already failed with a network error the same as isBrowserOffline() when deciding whether to wait for the cache.
…lists; scope offline caches per user Add a friendly "can't load this while offline" state (with retry) for a never-visited discussion or space discussion list, instead of a blank or silently-empty screen. Also scope every offline-cached list/doc/call key to the session user (cacheKey: [..., session.user], matching the existing drafts.ts pattern), so a second account signing into the same browser can't read the previous account's cached data before its own permission-checked fetch resolves — review finding from PR frappe#516. users.ts reads the session user straight from the cookie rather than importing session.ts: session.ts itself imports users.ts before assigning its `session` export, so importing it back from users.ts at module scope threw on boot.
frappe-ui's useList sends fields/filters/start/limit as GET query-string values, but get_list type-hinted fields/filters as dict and start/limit implicitly as int, so Frappe's own request coercion rejected the strings before the function body ran. Parse fields/filters with frappe.parse_json and coerce start/limit with cint, matching how the builtin /api/v2/document/<doctype> list route and gp_discussion.api.get_discussions already handle this.
…son profiles Moves the People list from the legacy Options-API resource (no offline persistence, not scoped per user) to a data/people.ts useList singleton with cacheKey ['People', session.user]. Reworks ProfileBento's card fetch onto useCall with cacheKey ['ProfileBento', personId, session.user] so it can resolve on failure instead of hanging forever, and fixes a broken relative API URL that made bento cards fail for everyone (online included) - useCall takes its url verbatim, unlike call()'s automatic /api/method/ prefix. People.vue, PersonProfile.vue, PersonProfileProfile.vue, PersonProfilePosts/Replies.vue all get an explicit offline/network failure state (OfflineContentFallback, with retry) distinct from a genuinely empty list or a real 404 - a failed fetch used to render as "0 members", an infinite skeleton, or a misleading NotFound.
…for offline Idle-delayed after login (and again on reconnect), warms the People list, every enabled member's GP User Profile doc, their bento cards, and their avatar bytes (loaded through an <img> element so the service worker's image cache picks them up) through a small worker pool - so the People page and any member's profile render offline even for a member never directly visited this session. Posts/replies are intentionally left out; those pages fall back to the honest offline-fallback state instead.
… update flow Shared-computer safety (PR frappe#516's review finding): wipe every offline cache (SW shell/runtime caches + idb-keyval) on logout, and on detecting a different user's session cookie at boot (guardAgainstUserSwitch). Plain logout deliberately leaves gameplan-drafts alone so the same person can recover an in-progress draft after logging back in; a detected switch to a different user clears drafts too. Also adds an update flow: the service worker no longer force-activates a new version under an open tab (no more unconditional skipWaiting on install); instead the app shows a "new version available" toast with a Refresh action once an update finishes installing, and reloads once the new worker takes control.
guardAgainstUserSwitch clears the service worker's SHELL_CACHE on a detected user switch, but nothing repopulated it until the next successful online navigation to /g. If the browser went offline before that happened, even a reload of the page already open failed with net::ERR_FAILED instead of falling back to the offline UI. Add a WARM_SHELL_CACHE message the page sends once the switch-triggered clear resolves (known to be online at that point, since a user just logged in), scoped separately from CLEAR_USER_CACHES so a plain logout still leaves the shell cache empty as intended. Bump CACHE_VERSION v6->v7 since the SW's message handling changed.
Migrates the offline-mode Playwright suite (12 stories: US1-US8, P1-P3) from a throwaway /tmp harness into frontend/tests/offline so it survives reboots and can gate regressions. Seeded-content coupling and creds are now env-overridable via config.js instead of hardcoded. Adds playwright as a devDependency and a yarn test:offline script.
guardAgainstUserSwitch used to kick off clearOfflineCaches() in a fire-and-forget Promise.all().then() and return synchronously. Two consequences, both flagged by review (PR frappe#516, round 4): - session.ts's login handler hard-navigates the instant it sees `true` back from the guard, which could tear the page down mid-clear. - The marker (localStorage's last-seen-user) was written unconditionally, so a switch that got cut off still looked "handled" on the next boot - a shared browser could keep serving the previous user's SHELL_CACHE. guardAgainstUserSwitch is now async: the clear (and the marker write, which now happens only after the clear settles) are awaited, and session.ts's login handler awaits the guard before its hard-navigate. Also hardens clearServiceWorkerCaches' worker lookup: guardAgainstUserSwitch runs before this module's own service worker registration, so a plain getRegistration() could legitimately find nothing yet even though an earlier browser session's worker (and its stale SHELL_CACHE) is still around. It now falls back to a bounded wait on navigator.serviceWorker.ready when a worker is expected to exist, instead of treating "not registered on this page load yet" as "nothing to clear". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ull space router.ts's offline/network-error fallback let navigation continue when a space or community couldn't be resolved (isRouteValidationUnavailable), so a deep link to a genuine-but-uncached space or community rendered downstream page components with space/community === null instead of either a wrongful NotFound or a working page. Flagged by review (PR frappe#516, round 3, escalated P2 -> P1: "Missing Space Proceeds"). Both branches now redirect to a new OfflineUnavailable page that says honestly that the content isn't cached yet, with a retry action - matching the pattern OfflineContentFallback.vue already uses for a failed discussion/space-list fetch. Also adds backend regression tests for GP User Profile's get_list - the only backend change in this PR (parsing fields/filters/start/limit as GET query-string values) had no test coverage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Posting a comment, a poll, or publishing a new discussion while offline
hit the network and surfaced a raw "TypeError: Failed to fetch" instead
of failing gracefully. Disable the relevant submit buttons outright when
isOnline is false, rather than letting the attempt happen and reporting
the error after the fact:
- CommentsArea.vue: the comment and poll submit buttons' existing
`disabled` bindings now also check isOnline. submitComment/submitPoll
themselves are guarded too, since ctrl/cmd+Enter reaches submitComment
directly and bypasses the disabled button.
- DiscussionHeader.vue: the "Publish" button's disabled condition
(previously just isComposerEditable) is now a canPublish computed that
also requires isOnline, and its tooltip explains why ("You're offline"
alongside the existing "Draft is loading" case). The draft body and
space selector are untouched and stay editable offline - only the
final publish step needs a network round trip.
- useNewDiscussion.ts's publish() gets the same isOnline check as a
backstop, in case it's ever reached another way.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replaces the floating pill (fixed, top-center, rounded, translucent shadow) with a full-width bar pinned to the true top of the viewport - gray (bg-surface-gray-8, matching the pill's own tone and ReadOnlyBanner.vue's status-message convention), with a wifi-off icon and "Network offline. Showing saved content." The pill only needed z-index to float over content; this banner needs to not overlap anything, so going offline pushes the app's own chrome down instead of covering it: MobileShell and DesktopShell (frappe-ui) both expose a `data-slot` attribute as a public styling hook, and index.css uses a `data-offline` attribute on <html> (toggled by OfflineIndicator.vue, same pattern as useCursorStyle.ts's data-cursor) to add padding-top equal to the banner's height to both. MobileShell is `fixed inset-0`, so this only shrinks its scroll region; DesktopShell is normal flow, so its whole row (rail + sidebar + content) shifts down together. The banner itself stays at a modest z-[60] - above normal content, below toasts/dialogs, so either still displays correctly over it if opened while offline. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Maintainer review: bg-surface-gray-8 + text-ink-white looked fine in light mode but broke in dark mode, and switching the background to the requested bg-surface-gray-3 would have broken light mode instead - frappe-ui's surface-gray-N tokens invert which raw shade they resolve to per theme (gray-8 is a medium-dark gray in light mode, a light gray in dark mode), while ink-white is a fixed color that doesn't follow. Replaced with ink-gray-N tokens throughout, which invert the same way the surface token does, so contrast holds in both themes without a dark: variant needed. Verified via frappe-ui's generated color tokens (tailwind/generated/colors.json) that gray-3/ink-gray-5/7/8 stay legible against each other in both lightMode and darkMode. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
 The compatibility requirement must be satisfied before merging. Findings
Prompt To Fix All With AI### Issue 1
frontend/src/components/ProfileBento/profileBentoSource.ts:54-59
The exported `getProfileBentoCards` function was removed without a compatibility alias, breaking external imports. This violates the repository requirement to preserve public APIs, so it must be addressed before merging.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
ebrahimgamdiwala
marked this pull request as draft
September 9, 2026 09:38
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
clearServiceWorkerCaches resolved on ANY message from the worker,
discarding the { ok: true | false } payload gameplan-sw.js's
CLEAR_USER_CACHES handler actually sends - a reported failure (or the
2s no-response timeout) was silently treated the same as success, and
guardAgainstUserSwitch would still mark the switch as "handled," so a
genuinely failed clear was never retried on a later boot.
- clearServiceWorkerCaches now resolves to the worker's real answer,
and falls back to deleting the same caches directly via the page's
own Cache Storage API when the worker doesn't confirm one - not
registered yet, unsupported, timed out, or an explicit
{ ok: false }. Matched by the gameplan-sw.js cache-name prefix
(duplicated as a constant, same as the message-type strings already
are - the worker runs in a separate script/global scope), excluding
the content-addressed asset cache, so this fallback doesn't need the
worker's cooperation at all.
- clearOfflineCaches now resolves to whether every store (service
worker caches + IndexedDB) actually confirmed it cleared, instead of
Promise<void>.
- guardAgainstUserSwitch only writes the last-seen-user marker once
clearOfflineCaches confirms success; on failure it returns without
touching the marker, so the same mismatch is seen - and the clear
retried - the next time this runs.
Reported in PR review (frappe#571).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ebrahimgamdiwala
marked this pull request as ready for review
September 9, 2026 10:03
…allback requestWorkerClear() can reject, not just resolve false: postMessage throws synchronously (auto-rejecting the wrapping Promise) if the worker became redundant between the registration lookup and the send, and the lookup itself (getActiveWorker -> getRegistration/ready) can reject too. clearServiceWorkerCaches awaited it with no catch, so a rejection skipped clearCachesDirectly() entirely and propagated out of clearOfflineCaches - breaking session.ts's logout redirect (no try/catch there), and in guardAgainstUserSwitch's login path, skipping the one thing (the direct Cache Storage fallback) that could have cleared the previous user's caches immediately instead of only on a later retry. clearServiceWorkerCaches now catches requestWorkerClear() and falls through to clearCachesDirectly() on any failure, not just a resolved `false` - the function can no longer reject at all. Reported in PR review (frappe#571). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ebrahimgamdiwala
marked this pull request as draft
September 9, 2026 10:12
ebrahimgamdiwala
marked this pull request as ready for review
September 9, 2026 10:13
Comment on lines
54
to
59
| function getLoadResultFromResponse(response: ProfileBentoResponse): ProfileBentoLoadResult { | ||
| return { | ||
| cards: response.cards || [], | ||
| isDefault: response.is_default, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The exported getProfileBentoCards function was removed without a compatibility alias, breaking external imports. This violates the repository requirement to preserve public APIs, so it must be addressed before merging.
Context Used: Guidelines for reviewing Frappe Framework applicat... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/components/ProfileBento/profileBentoSource.ts
Line: 54-59
Comment:
**Exported API Removed**
The exported `getProfileBentoCards` function was removed without a compatibility alias, breaking external imports. This violates the repository requirement to preserve public APIs, so it must be addressed before merging.
**Context Used:** Guidelines for reviewing Frappe Framework applicat... ([source](https://github.com/frappe/skills/blob/main/skills/quality-code-review/SKILL.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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.
Continuation of #516
netchampfaris's original PR (#516) was handed over to me to take over and finish. Per the maintainer's request, this is opened as a separate PR from my fork rather than pushing to the original branch, since I don't have write access to this repo. Commit history from #516 is preserved intact — nothing rebased or rewritten, only a
developmerge plus new commits on top.What's in #516
Read-only offline support via a service worker: the app shell loads without network, and previously-cached content (spaces, discussions, people, profiles) renders from IndexedDB when a fetch fails. Full details in the original PR description.
What's new since #516
Addressing review findings that were still open, plus a couple of UX fixes found during manual testing:
guardAgainstUserSwitchnow awaits the cache clear and only writes the "last seen user" marker once it actually completes, instead of firing it and returning immediately. Also hardens the service-worker lookup to retry vianavigator.serviceWorker.readyinstead of treating "not registered yet" as "nothing to clear."space === null. It now routes to a newOfflineUnavailablepage with an honest "not available offline" message and a retry action.TypeError: Failed to fetch.ink-gray/surface-graytokens so it holds contrast in both light and dark mode.GP User Profile.get_list's GET query-param parsing (the PR's one backend change), which had no test coverage before.develop(was 77 commits behind).Testing
bench --site <disposable site> run-tests --app gameplan— 1112 tests, 0 failures🤖 Generated with Claude Code