fix(offline): resume rehydrated paused mutations, stop logout cache leak, offline affordances (PR #120 QA)
Address six QA findings on the offline-sync feature: 1. HIGH — silent data loss: a paused mutation persisted to IndexedDB and reloaded while still offline never resumed on reconnect. Seed TanStack onlineManager from navigator.onLine at boot (it defaults to online:true and only flips on events, so a cold-boot-offline tab wrongly believed it was online and never got a true online transition), and call resumePausedMutations() in PersistQueryClientProvider onSuccess after the persister rehydrates (defaults are registered before, so the restored mutation has a mutationFn). New offline-resume.test.ts reproduces the full persist -> reload -> reconnect path. 2. MEDIUM (security) — logout did not durably clear gitmost-rq-cache: the throttled persister re-wrote the key ~1s after del() with the still-in-memory snapshot, resurrecting the previous user's data. Freeze the persister (persistClient becomes a no-op) before clearing/deleting so neither the clear()-triggered nor any in-flight write can repopulate the key; re-enable afterwards for the next sign-in session. 3. MEDIUM (UX) — offline create spun forever: the create-note button awaited a mutateAsync that stays pending while paused. Detect offline, fire-and-forget the (queued) mutation, show a "saved offline" notice, and gate the spinner on !isPaused so it no longer hangs. 4. LOW — an uncached page opened offline showed the generic "Error fetching page data." instead of the offline fallback (offline fetch yields no HTTP status). Render OfflineFallback when navigator is offline or the error has no status. 5. LOW — logout teardown threw "Cannot read properties of null (reading 'settings')" in full-editor.tsx: optional-chain the (transiently null) user. 6. Tab title "Untitled": investigated — the tab-title derivation in page.tsx is byte-identical to develop and already reads page.title from REST/cache (the recommended source); live edits keep it in sync via updatePageData. Not a tab-title-derivation regression introduced by this PR; no change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -82,14 +82,18 @@ export function FullEditor({
|
||||
// AI title generation is gated by the general AI chat flag (the same toggle
|
||||
// that enables the chat agent); the server enforces it too (#199).
|
||||
const isTitleGenEnabled = workspace?.settings?.ai?.chat === true;
|
||||
const fullPageWidth = user.settings?.preferences?.fullPageWidth;
|
||||
// `user` can momentarily be null during logout teardown (the currentUser atom
|
||||
// is reset before this subtree unmounts). Optional-chain every access so the
|
||||
// teardown render does not throw "Cannot read properties of null (reading
|
||||
// 'settings')".
|
||||
const fullPageWidth = user?.settings?.preferences?.fullPageWidth;
|
||||
const editorToolbarEnabled =
|
||||
user.settings?.preferences?.editorToolbar ?? false;
|
||||
user?.settings?.preferences?.editorToolbar ?? false;
|
||||
const [currentPageEditMode, setCurrentPageEditMode] = useAtom(
|
||||
currentPageEditModeAtom,
|
||||
);
|
||||
const userPageEditMode =
|
||||
user.settings?.preferences?.pageEditMode ?? PageEditMode.Edit;
|
||||
user?.settings?.preferences?.pageEditMode ?? PageEditMode.Edit;
|
||||
const isEditMode = currentPageEditMode === PageEditMode.Edit;
|
||||
|
||||
// Single shared Y.Doc + HocuspocusProvider for both the title and body
|
||||
|
||||
@@ -3,6 +3,8 @@ import { IconHourglass, IconPlus } from "@tabler/icons-react";
|
||||
import { ReactNode } from "react";
|
||||
import { useNavigate } from "react-router-dom";
|
||||
import { useTranslation } from "react-i18next";
|
||||
import { onlineManager } from "@tanstack/react-query";
|
||||
import { notifications } from "@mantine/notifications";
|
||||
import { useGetSpacesQuery } from "@/features/space/queries/space-query.ts";
|
||||
import { useCreatePageMutation } from "@/features/page/queries/page-query.ts";
|
||||
import { buildPageUrl } from "@/features/page/page.utils.ts";
|
||||
@@ -36,21 +38,39 @@ function CreateNoteButton({
|
||||
const createPageMutation = useCreatePageMutation();
|
||||
|
||||
const createNote = async (space: ISpace) => {
|
||||
// `spaceId`/`temporary` are accepted by the create-page endpoint but are
|
||||
// not part of the shared `IPageInput` type; cast to satisfy the mutation
|
||||
// signature.
|
||||
const variables = {
|
||||
spaceId: space.id,
|
||||
...(temporary ? { temporary: true } : {}),
|
||||
} as any;
|
||||
|
||||
if (!onlineManager.isOnline()) {
|
||||
// Offline: the create is PAUSED and queued — its promise will not resolve
|
||||
// until we are back online, so awaiting it here would spin the button
|
||||
// forever. Fire it without awaiting (it persists and replays on reconnect)
|
||||
// and tell the user it was saved offline instead of leaving a dead spinner.
|
||||
createPageMutation.mutate(variables);
|
||||
notifications.show({
|
||||
color: "blue",
|
||||
message: t("You're offline. This note will be created once you reconnect."),
|
||||
});
|
||||
return;
|
||||
}
|
||||
|
||||
try {
|
||||
// `spaceId`/`temporary` are accepted by the create-page endpoint but are
|
||||
// not part of the shared `IPageInput` type; cast to satisfy the mutation
|
||||
// signature.
|
||||
const createdPage = await createPageMutation.mutateAsync({
|
||||
spaceId: space.id,
|
||||
...(temporary ? { temporary: true } : {}),
|
||||
} as any);
|
||||
const createdPage = await createPageMutation.mutateAsync(variables);
|
||||
navigate(buildPageUrl(space.slug, createdPage.slugId, createdPage.title));
|
||||
} catch {
|
||||
// useCreatePageMutation already surfaces a red notification on error.
|
||||
}
|
||||
};
|
||||
|
||||
const isPending = createPageMutation.isPending;
|
||||
// A paused (offline) mutation stays `isPending`, so gate the spinner on it NOT
|
||||
// being paused — otherwise the button would spin forever after an offline
|
||||
// create. The offline path above gives its own "saved offline" feedback.
|
||||
const isPending = createPageMutation.isPending && !createPageMutation.isPaused;
|
||||
|
||||
// Exactly one writable space → create directly, no picker needed.
|
||||
if (writableSpaces.length === 1) {
|
||||
|
||||
@@ -1,7 +1,11 @@
|
||||
import { del } from "idb-keyval";
|
||||
|
||||
import { queryClient } from "@/main.tsx";
|
||||
import { OFFLINE_CACHE_KEY } from "./query-persister";
|
||||
import {
|
||||
OFFLINE_CACHE_KEY,
|
||||
freezeOfflinePersistence,
|
||||
unfreezeOfflinePersistence,
|
||||
} from "./query-persister";
|
||||
import { PAGE_YDOC_NAME_PREFIX } from "@/features/editor/page-ydoc-name";
|
||||
|
||||
/**
|
||||
@@ -31,19 +35,27 @@ import { PAGE_YDOC_NAME_PREFIX } from "@/features/editor/page-ydoc-name";
|
||||
* service-worker-capable browsers).
|
||||
*/
|
||||
export async function clearOfflineCache(): Promise<void> {
|
||||
// 1a. Drop the in-memory query cache immediately.
|
||||
try {
|
||||
queryClient.clear();
|
||||
} catch {
|
||||
// best-effort: ignore in-memory cache reset failures
|
||||
}
|
||||
// Freeze the throttled persister BEFORE touching the cache so the
|
||||
// queryClient.clear() below cannot trigger a late re-write of the (still
|
||||
// nearly-full) dehydrated snapshot after we del() the key — which would
|
||||
// otherwise resurrect the previous user's persisted data in IndexedDB.
|
||||
// Re-enabled in `finally` so the next (sign-in) session persists normally.
|
||||
freezeOfflinePersistence();
|
||||
|
||||
// 1b. Delete the persisted RQ cache from IndexedDB.
|
||||
try {
|
||||
await del(OFFLINE_CACHE_KEY);
|
||||
} catch {
|
||||
// best-effort: ignore persisted-cache deletion failures
|
||||
}
|
||||
// 1a. Drop the in-memory query cache immediately.
|
||||
try {
|
||||
queryClient.clear();
|
||||
} catch {
|
||||
// best-effort: ignore in-memory cache reset failures
|
||||
}
|
||||
|
||||
// 1b. Delete the persisted RQ cache from IndexedDB.
|
||||
try {
|
||||
await del(OFFLINE_CACHE_KEY);
|
||||
} catch {
|
||||
// best-effort: ignore persisted-cache deletion failures
|
||||
}
|
||||
|
||||
// 2. Delete the Yjs page IndexedDB databases (`page.<id>`).
|
||||
// `indexedDB.databases()` is not implemented everywhere (e.g. Firefox); when
|
||||
@@ -91,4 +103,10 @@ export async function clearOfflineCache(): Promise<void> {
|
||||
} catch {
|
||||
// best-effort: ignore Cache Storage failures
|
||||
}
|
||||
} finally {
|
||||
// Re-enable persistence for the next session (sign-in continues running in
|
||||
// the same tab; logout reloads via window.location.replace, so this is a
|
||||
// harmless no-op there).
|
||||
unfreezeOfflinePersistence();
|
||||
}
|
||||
}
|
||||
|
||||
128
apps/client/src/features/offline/offline-resume.test.ts
Normal file
128
apps/client/src/features/offline/offline-resume.test.ts
Normal file
@@ -0,0 +1,128 @@
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||
import { QueryClient, onlineManager } from "@tanstack/react-query";
|
||||
import {
|
||||
persistQueryClientRestore,
|
||||
persistQueryClientSave,
|
||||
} from "@tanstack/react-query-persist-client";
|
||||
|
||||
// Stub the network services so a replayed mutation hits a spy, not the network.
|
||||
const h = vi.hoisted(() => ({
|
||||
createPage: vi.fn(),
|
||||
movePage: vi.fn(),
|
||||
createComment: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock("@/features/page/services/page-service", () => ({
|
||||
createPage: h.createPage,
|
||||
movePage: h.movePage,
|
||||
}));
|
||||
vi.mock("@/features/comment/services/comment-service", () => ({
|
||||
createComment: h.createComment,
|
||||
}));
|
||||
vi.mock("@/features/page/queries/page-query", () => ({
|
||||
invalidateOnCreatePage: vi.fn(),
|
||||
}));
|
||||
|
||||
// In-memory idb-keyval so the REAL queryPersister round-trips through a fake
|
||||
// store (the actual persist -> reload -> restore path, not a hand-built blob).
|
||||
const store = new Map<string, string>();
|
||||
vi.mock("idb-keyval", () => ({
|
||||
get: vi.fn((k: string) => Promise.resolve(store.get(k) ?? undefined)),
|
||||
set: vi.fn((k: string, v: string) => {
|
||||
store.set(k, v);
|
||||
return Promise.resolve();
|
||||
}),
|
||||
del: vi.fn((k: string) => {
|
||||
store.delete(k);
|
||||
return Promise.resolve();
|
||||
}),
|
||||
}));
|
||||
|
||||
import { queryPersister } from "./query-persister";
|
||||
import {
|
||||
offlineMutationKeys,
|
||||
registerOfflineMutationDefaults,
|
||||
} from "./offline-mutations";
|
||||
|
||||
const BUSTER = "test-buster";
|
||||
|
||||
beforeEach(() => {
|
||||
store.clear();
|
||||
h.createPage.mockReset().mockResolvedValue({ id: "new-page" });
|
||||
h.movePage.mockReset().mockResolvedValue(undefined);
|
||||
h.createComment.mockReset().mockResolvedValue({ id: "new-comment" });
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
// onlineManager is a global singleton; leave it in the default online state.
|
||||
onlineManager.setOnline(true);
|
||||
});
|
||||
|
||||
describe("offline paused-mutation resume across a reload", () => {
|
||||
// This is the #120 silent-data-loss reproduction: a paused mutation persisted
|
||||
// to IndexedDB while offline, then the tab RELOADS while still offline, must
|
||||
// resume on reconnect. It exercises the real persister round-trip plus the two
|
||||
// boot-time fixes the app wiring relies on:
|
||||
// (a) onlineManager seeded to the real offline state so the later reconnect
|
||||
// is a true offline->online transition that auto-resumes, and
|
||||
// (b) resumePausedMutations() called after the persister restores (what the
|
||||
// PersistQueryClientProvider onSuccess does), with mutation defaults
|
||||
// registered BEFORE the resume so the rehydrated mutation has a fn.
|
||||
it("replays a rehydrated paused create on reconnect (mutationFn fires)", async () => {
|
||||
// --- Tab 1, OFFLINE: user creates a page; it pauses and gets persisted. ---
|
||||
onlineManager.setOnline(false); // (a) boot seeded offline
|
||||
|
||||
const client1 = new QueryClient();
|
||||
registerOfflineMutationDefaults(client1);
|
||||
const observer = client1.getMutationCache().build(client1, {
|
||||
mutationKey: offlineMutationKeys.createPage,
|
||||
});
|
||||
observer.state.isPaused = true;
|
||||
observer.state.status = "pending";
|
||||
observer.state.variables = { spaceId: "s1", title: "Offline page" };
|
||||
|
||||
await persistQueryClientSave({
|
||||
// Cast: persist-client-core and react-query may resolve to different
|
||||
// @tanstack/query-core copies whose QueryClient brands are nominally
|
||||
// incompatible (see query-persister.ts). Structurally identical at runtime.
|
||||
queryClient: client1 as any,
|
||||
persister: queryPersister,
|
||||
buster: BUSTER,
|
||||
dehydrateOptions: { shouldDehydrateMutation: () => true },
|
||||
});
|
||||
// The paused mutation is now in the persisted store.
|
||||
expect(store.size).toBe(1);
|
||||
|
||||
// --- RELOAD while still offline: fresh client restores from the SAME
|
||||
// persister. Defaults are registered BEFORE restore/resume. ---
|
||||
const client2 = new QueryClient();
|
||||
registerOfflineMutationDefaults(client2);
|
||||
client2.mount(); // subscribes to onlineManager (auto-resume on reconnect)
|
||||
|
||||
await persistQueryClientRestore({
|
||||
queryClient: client2 as any,
|
||||
persister: queryPersister,
|
||||
buster: BUSTER,
|
||||
});
|
||||
expect(client2.getMutationCache().getAll()).toHaveLength(1);
|
||||
|
||||
// (b) onSuccess wiring resumes after restore — but we are still OFFLINE, so
|
||||
// the mutation must stay paused and NOT fire yet.
|
||||
await client2.resumePausedMutations();
|
||||
expect(h.createPage).not.toHaveBeenCalled();
|
||||
|
||||
// --- RECONNECT: the offline->online transition auto-resumes the paused
|
||||
// mutation and its registered default mutationFn finally fires. ---
|
||||
onlineManager.setOnline(true);
|
||||
|
||||
await vi.waitFor(() => {
|
||||
expect(h.createPage).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
expect(h.createPage).toHaveBeenCalledWith({
|
||||
spaceId: "s1",
|
||||
title: "Offline page",
|
||||
});
|
||||
|
||||
client2.unmount();
|
||||
});
|
||||
});
|
||||
@@ -1,7 +1,19 @@
|
||||
import { describe, it, expect } from "vitest";
|
||||
import { describe, it, expect, vi, afterEach } from "vitest";
|
||||
|
||||
// In-memory idb-keyval so we can observe whether the persister actually writes.
|
||||
const h = vi.hoisted(() => ({
|
||||
get: vi.fn(() => Promise.resolve(undefined)),
|
||||
set: vi.fn(() => Promise.resolve()),
|
||||
del: vi.fn(() => Promise.resolve()),
|
||||
}));
|
||||
vi.mock("idb-keyval", () => h);
|
||||
|
||||
import {
|
||||
shouldDehydrateOfflineQuery,
|
||||
OFFLINE_PERSIST_ROOTS,
|
||||
queryPersister,
|
||||
freezeOfflinePersistence,
|
||||
unfreezeOfflinePersistence,
|
||||
} from "./query-persister";
|
||||
|
||||
// Small helper to build the structural query shape the predicate reads.
|
||||
@@ -87,3 +99,30 @@ describe("OFFLINE_PERSIST_ROOTS", () => {
|
||||
expect(OFFLINE_PERSIST_ROOTS.has("trash")).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("freeze/unfreeze persistence (logout no-late-write guard)", () => {
|
||||
const dummyClient = {
|
||||
timestamp: Date.now(),
|
||||
buster: "",
|
||||
clientState: { mutations: [], queries: [] },
|
||||
} as any;
|
||||
|
||||
afterEach(() => {
|
||||
// Always leave persistence enabled so other tests/sessions persist normally.
|
||||
unfreezeOfflinePersistence();
|
||||
h.set.mockClear();
|
||||
});
|
||||
|
||||
it("does NOT write to storage while frozen", async () => {
|
||||
freezeOfflinePersistence();
|
||||
await queryPersister.persistClient(dummyClient);
|
||||
expect(h.set).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("resumes writing to storage once unfrozen", async () => {
|
||||
freezeOfflinePersistence();
|
||||
unfreezeOfflinePersistence();
|
||||
await queryPersister.persistClient(dummyClient);
|
||||
expect(h.set).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -23,12 +23,38 @@ const idbStorage = {
|
||||
removeItem: (key: string) => del(key),
|
||||
};
|
||||
|
||||
export const queryPersister = createAsyncStoragePersister({
|
||||
const basePersister = createAsyncStoragePersister({
|
||||
storage: idbStorage,
|
||||
key: OFFLINE_CACHE_KEY,
|
||||
throttleTime: 1000,
|
||||
});
|
||||
|
||||
// When frozen, persistClient becomes a no-op so no new dehydrated snapshot is
|
||||
// written to IndexedDB. This closes a logout data-leak race: clearing the cache
|
||||
// (queryClient.clear()) fires `removed` cache events, each of which the persist
|
||||
// subscription turns into a throttled persistClient call. The FIRST such call
|
||||
// dehydrates a still-nearly-full snapshot and its async write can land AFTER the
|
||||
// del() that clears the key, resurrecting the previous user's data (~180KB) in
|
||||
// IndexedDB. Freezing before clear()/del() prevents any such rewrite. Re-enabled
|
||||
// afterwards so the next (sign-in) session persists normally. See
|
||||
// clear-offline-cache.ts.
|
||||
let persistFrozen = false;
|
||||
|
||||
export function freezeOfflinePersistence(): void {
|
||||
persistFrozen = true;
|
||||
}
|
||||
|
||||
export function unfreezeOfflinePersistence(): void {
|
||||
persistFrozen = false;
|
||||
}
|
||||
|
||||
export const queryPersister = {
|
||||
persistClient: (persistedClient: Parameters<typeof basePersister.persistClient>[0]) =>
|
||||
persistFrozen ? Promise.resolve() : basePersister.persistClient(persistedClient),
|
||||
restoreClient: () => basePersister.restoreClient(),
|
||||
removeClient: () => basePersister.removeClient(),
|
||||
};
|
||||
|
||||
// Only navigation/read query roots are persisted for offline reading.
|
||||
// Volatile/auth queries (collab tokens, trash lists) are intentionally excluded.
|
||||
//
|
||||
|
||||
@@ -11,7 +11,7 @@ import { MantineProvider } from "@mantine/core";
|
||||
import { BrowserRouter } from "react-router-dom";
|
||||
import { ModalsProvider } from "@mantine/modals";
|
||||
import { Notifications } from "@mantine/notifications";
|
||||
import { QueryClient } from "@tanstack/react-query";
|
||||
import { QueryClient, onlineManager } from "@tanstack/react-query";
|
||||
import { PersistQueryClientProvider } from "@tanstack/react-query-persist-client";
|
||||
import { HelmetProvider } from "react-helmet-async";
|
||||
import "./i18n";
|
||||
@@ -47,9 +47,21 @@ export const queryClient = new QueryClient({
|
||||
// Register default mutationFns for the offline-relevant structural mutations so
|
||||
// a paused mutation restored from IndexedDB after an offline reload still has a
|
||||
// mutationFn and is replayed by resumePausedMutations() on reconnect (instead
|
||||
// of silently no-op'ing and dropping the offline create/move/comment).
|
||||
// of silently no-op'ing and dropping the offline create/move/comment). MUST run
|
||||
// before any resumePausedMutations() so rehydrated paused mutations have a fn.
|
||||
registerOfflineMutationDefaults(queryClient);
|
||||
|
||||
// Seed TanStack Query's onlineManager from the REAL connectivity state at boot.
|
||||
// It defaults to `online: true` and only flips on window online/offline events,
|
||||
// so a tab that COLD-BOOTS offline would wrongly believe it is online: paused
|
||||
// mutations restored from IndexedDB would never get a later offline->online
|
||||
// transition to trigger their replay, and the offline UI affordances could not
|
||||
// tell they are offline. Seeding here makes the first real `online` event a true
|
||||
// transition that auto-resumes the rehydrated paused mutations (#120 data loss).
|
||||
if (typeof navigator !== "undefined" && "onLine" in navigator) {
|
||||
onlineManager.setOnline(navigator.onLine);
|
||||
}
|
||||
|
||||
if (isCloud() && isPostHogEnabled) {
|
||||
posthog.init(getPostHogKey(), {
|
||||
api_host: getPostHogHost(),
|
||||
@@ -76,6 +88,16 @@ root.render(
|
||||
shouldDehydrateQuery: shouldDehydrateOfflineQuery,
|
||||
},
|
||||
}}
|
||||
// After the persister finishes rehydrating, replay any paused
|
||||
// mutations restored from IndexedDB. If we are back online this fires
|
||||
// them immediately; if still offline they stay paused and TanStack's
|
||||
// onlineManager auto-resumes them on the next online transition (which
|
||||
// is now a true transition thanks to the onlineManager seeding above).
|
||||
// Without this, a paused mutation persisted while offline and then
|
||||
// reloaded would never resume and the user's work would be lost (#120).
|
||||
onSuccess={() => {
|
||||
queryClient.resumePausedMutations();
|
||||
}}
|
||||
>
|
||||
<Notifications position="bottom-center" limit={3} zIndex={10000} />
|
||||
{/* Skip SW registration inside the Capacitor native WebView — the
|
||||
|
||||
@@ -9,6 +9,7 @@ import { useGetSpaceBySlugQuery } from "@/features/space/queries/space-query.ts"
|
||||
import { useTranslation } from "react-i18next";
|
||||
import React from "react";
|
||||
import { EmptyState } from "@/components/ui/empty-state.tsx";
|
||||
import { OfflineFallback } from "@/features/offline/offline-fallback.tsx";
|
||||
import { IconAlertTriangle, IconFileOff } from "@tabler/icons-react";
|
||||
import { Button } from "@mantine/core";
|
||||
import { Link } from "react-router-dom";
|
||||
@@ -62,7 +63,19 @@ function PageContent({ pageSlug }: { pageSlug: string | undefined }) {
|
||||
}
|
||||
|
||||
if (isError || !page) {
|
||||
if ([401, 403, 404].includes(error?.["status"])) {
|
||||
// An offline fetch of a page that was never saved for offline use yields a
|
||||
// network error with NO HTTP status (status is undefined), which would
|
||||
// otherwise fall through to the generic "Error fetching page data." state.
|
||||
// When we are offline (or the failure is a network error with no status),
|
||||
// show the dedicated "You're offline — this page isn't saved for offline"
|
||||
// fallback instead, so the user understands why the page won't load.
|
||||
const httpStatus = error?.["status"];
|
||||
const isOffline =
|
||||
typeof navigator !== "undefined" && navigator.onLine === false;
|
||||
if (isOffline || (isError && httpStatus == null)) {
|
||||
return <OfflineFallback />;
|
||||
}
|
||||
if ([401, 403, 404].includes(httpStatus)) {
|
||||
return (
|
||||
<EmptyState
|
||||
icon={IconFileOff}
|
||||
|
||||
Reference in New Issue
Block a user