feat(session-sync): phased SW resync with experiment-ready config#573
feat(session-sync): phased SW resync with experiment-ready config#573Just-Insane wants to merge 8 commits intoSableClient:devfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds session re-sync phase controls (foreground + visible heartbeat with optional adaptive backoff/jitter) and introduces an experiment/config override pipeline so behavior can be rolled out via environment-scoped config.json overrides at build time.
Changes:
- Extend client config schema with
sessionSynctoggles andexperiments, plus helper APIs to deterministically bucket users into variants. - Update app visibility handling to optionally push the active session to the service worker on foreground/focus and on a visible heartbeat loop.
- Add a CI/build-time
config.jsonoverride injector and wire it into GitHub Actions (preview/production environments), plus update defaultconfig.json.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/app/pages/client/ClientRoot.tsx | Passes activeSession into useAppVisibility so SW session sync can include full session identity. |
| src/app/hooks/useClientConfig.ts | Adds sessionSync/experiments config shape and experiment variant selection helpers. |
| src/app/hooks/useAppVisibility.ts | Implements phased SW session pushes (foreground/focus + heartbeat + optional adaptive backoff/jitter). |
| scripts/inject-client-config.js | New build-time script to deep-merge env overrides into config.json. |
| knip.json | Adds the new script as a Knip entry. |
| config.json | Adds default experiments and sessionSync config blocks. |
| .github/workflows/cloudflare-web-preview.yml | Exposes env-scoped config override vars to preview deploy workflow. |
| .github/workflows/cloudflare-web-deploy.yml | Exposes env-scoped config override vars to plan/apply workflows (preview + production). |
| .github/actions/setup/action.yml | Runs the config override injector as part of build setup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Addressing the three Copilot review comments from this PR:
Added an
Added a
|
a1445fe to
fa6d468
Compare
Add sessionSync config block documentation to the installation guide, including all timing fields, the three phase flags, and how to combine with experiments.sessionSyncStrategy for a controlled rollout. Corresponds to SableClient/Sable#573
Add sessionSync config block documentation to the installation guide, including all timing fields, the three phase flags, and how to combine with experiments.sessionSyncStrategy for a controlled rollout. Corresponds to SableClient/Sable#573
9c568d8 to
09e1a89
Compare
Add sessionSync config block documentation to the installation guide, including all timing fields, the three phase flags, and how to combine with experiments.sessionSyncStrategy for a controlled rollout. Corresponds to SableClient/Sable#573
d6a42e6 to
af84b05
Compare
…, pass userId to SW
Add pagehide listener alongside visibilitychange so the SW gets a visible:false message when the app is backgrounded quickly on iOS Safari before visibilitychange can be delivered. Also add a 30 s TTL on the appIsVisible SW flag so that if the app suspended the JS context before either message reached the SW, the flag expires automatically rather than permanently suppressing incoming push notifications.
…sVisible flag
The existing TTL guard (APP_VISIBLE_TTL_MS) handles the case where
matchAll() returns zero clients. This commit improves the case where
matchAll() does return clients: their visibilityState is now used as the
authoritative signal rather than being OR-ed with the potentially-stale
appIsVisible flag.
On iOS Safari PWA the visibilitychange handler may fail to deliver
postMessage({ visible: false }) before iOS suspends the JS thread.
matchAll() still reports the correct visibilityState:'hidden' for the
backgrounded page, so trusting it directly prevents stale appIsVisible=true
from suppressing push notifications when the app is backgrounded.
…ns empty The TTL-gated appIsVisibleFresh fallback was designed to handle the iOS Safari PWA quirk where clients.matchAll() returns an empty list even when the app is visually open. In practice, the TTL window (30 s) meant any push that arrived within 30 seconds of the app being last visible was silently dropped — including the common case of backgrounding the app and immediately receiving a reply. pagehide + visibilitychange together are now much more reliable at delivering setAppVisible:false before the SW processes the push. When both events DO fail (iOS kills the JS context mid-event), clients.matchAll() ALSO tends to return the backgrounded client as visibilityState:'hidden', which is already handled correctly by the first branch. The clients.length === 0 path is therefore a triple-failure edge case where we simply cannot determine visibility. Erring toward showing the notification (a possible duplicate, handled gracefully by the in-app banner) is always better than silently dropping it. Removes appVisibleSetAt, APP_VISIBLE_TTL_MS, and the stale-flag fallback.
db3d901 to
56690f5
Compare
…sites not ready 'skipped' from pushSessionNow means mx/session/SW controller aren't ready yet, not that a real push attempt failed. Incrementing heartbeatFailuresRef in this case caused exponential backoff to build up during app startup, keeping the heartbeat interval large long after prerequisites became available. Only reset failures to 0 on a successful 'sent'; leave count unchanged on 'skipped' so startup latency never inflates the backoff. Resolves Copilot review comment on PR SableClient#573.
…sites not ready 'skipped' from pushSessionNow means mx/session/SW controller aren't ready yet, not that a real push attempt failed. Incrementing heartbeatFailuresRef in this case caused exponential backoff to build up during app startup, keeping the heartbeat interval large long after prerequisites became available. Only reset failures to 0 on a successful 'sent'; leave count unchanged on 'skipped' so startup latency never inflates the backoff. Resolves Copilot review comment on PR SableClient#573.
📝 Docs PR: SableClient/docs#9 — merge that when merging this.
PR dependency (stacked)
This branch is stacked on #572 — it has been rebased onto
feat/feature-flag-env-varswith no duplicate commits. Only the two session-sync–specific commits are unique to this PR. Please review/merge #572 first, then review this PR for session-sync behavior.Description
Adds phased service-worker session re-sync controls, gated behind an experiment flag with direct-config fallback.
Also fixes two bugs in the phase-flag path:
heartbeatFailuresRef.currentwas not reset to 0 when a foreground push succeeded. This left the heartbeat on an inflated backoff interval even after the SW controller was confirmed alive. It now resets to 0 on any successful foreground or focus push.preloadedSessiondouble cache read: the SW push handler was warmingpreloadedSessionby fetching from the cache, then immediately reading from the cache again on the first/syncinterception. The redundant read has been removed; the push handler now stores the session object directly.Fixes #
Type of change
Checklist:
AI disclosure:
The phased session-sync hooks/flags and config integration were partially AI assisted; I verified branch behavior and safe defaults. The heartbeat reset fix and preloadedSession dedup were partially AI assisted; I confirmed the fix was correct by tracing the backoff counter through the foreground → heartbeat tick path.
Feature Flags / Environment Setup
Set these GitHub Environment variables:
CLIENT_CONFIG_OVERRIDES_JSONCLIENT_CONFIG_OVERRIDES_STRICT(optional)How it works
Phase flags are derived in
useAppVisibilityfrom thesessionSyncStrategyexperiment:control(default)session-sync-heartbeatsession-sync-adaptiveUsers not in the experiment fall back to the
sessionSync.phase*booleans, which can be used for direct-config overrides (e.g. preview/staging where you want all users to get the feature without experiment bucketing).sessionSync config field reference
phase1ForegroundResyncfalsephase2VisibleHeartbeatfalsephase3AdaptiveBackoffJitterfalsephase2VisibleHeartbeatistrue.foregroundDebounceMs1500heartbeatIntervalMs600000resumeHeartbeatSuppressMs60000heartbeatMaxBackoffMs1800000Recommended initial values
preview— enable for all users directly (no bucketing):{ "sessionSync": { "phase1ForegroundResync": true, "phase2VisibleHeartbeat": true, "phase3AdaptiveBackoffJitter": true } }production— gradual rollout via experiment:{ "experiments": { "sessionSyncStrategy": { "enabled": true, "rolloutPercentage": 20, "controlVariant": "control", "variants": ["session-sync-heartbeat", "session-sync-adaptive"] } } }No environment variable setup is required for deployment to succeed; missing overrides are treated as no-op and checked-in defaults are used.