-
Notifications
You must be signed in to change notification settings - Fork 389
[personal-wp] Add multi-tab coordination #3163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
a75d636 to
add3bab
Compare
1555eba to
4da5ad5
Compare
add3bab to
7cdbb30
Compare
4da5ad5 to
817712c
Compare
7cdbb30 to
c1b2abd
Compare
817712c to
7158776
Compare
c1b2abd to
c94e2d5
Compare
| const remoteUrl = getRemoteUrl(); | ||
| const scopedSiteUrl = `/scope:${encodeURIComponent(site.slug)}/`; | ||
|
|
||
| const dependentModeClient = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These methods seem like a less capable version of the original client methods, e.g. goTo()) covers for a few relevant corner-cases. I understand we need this because there's not a playgroundClient we could use to transform playground.pathToInternalUrl()? Could we somehow declare const playground = mainPlaygroundRunningInAnotherTab? e.g. using broadcast channels for communication? We may need a custom Comlink transport but could be doable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Admittedly I am not sure I understand this in-and-out enough but the way I understand it is that the PlaygroundClient basically fulfills navigational tasks for the iframe communication and php worker stuff. Could we refactor out those navigational tasks so that our dependentModeClient could have those and all php worker things would remain in the PlaygroundClient.
| * | ||
| * @param tabs - List of tabs to check | ||
| */ | ||
| export function requestStaleTabsShutdown(tabs: TabInfo[]): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comes from the worry that people will have super long running tabs. AFAIK we don't have a mechanism to check with the server if there is a new version of playground. This is mostly to avoid people sitting on stale and outdated tabs for months.
| * @param targetTabId - The tab ID to shut down | ||
| * @param reason - Why the tab should shut down | ||
| */ | ||
| export function requestTabShutdown( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only called once, in the function below. Can we either inline it or not export it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove the export.
| const ONE_DAY_MS = 24 * 60 * 60 * 1000; | ||
| const PING_TIMEOUT_MS = 150; | ||
|
|
||
| let channel: BroadcastChannel | null = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to name it: these are local variables that are not managed by redux even though this file lives in state/redux. It seems fine. I don't expect we'll need to run multiple redux stores that run independent tab coordination.
| let siteResetCallback: (() => void) | null = null; | ||
| let beforeUnloadHandler: (() => void) | null = null; | ||
|
|
||
| // Clean up on Vite HMR to prevent duplicate listeners |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is weird, is there a general way of expressing this that doesn't depend on vite or encode vite-specific APIs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a remainder of debugging some weirdnes, I think I can remove it.
| */ | ||
| export function useRemoteBackup() { | ||
| const clientInfo = usePlaygroundClientInfo(); | ||
| const activeSite = useActiveSite(); | ||
| const [isRequestingBackup, setIsRequestingBackup] = useState(false); | ||
|
|
||
| const isDependentMode = clientInfo?.isDependentMode ?? false; | ||
|
|
||
| const requestBackup = useCallback(async (): Promise<boolean> => { | ||
| if (!activeSite || !isDependentMode || isRequestingBackup) { | ||
| return false; | ||
| } | ||
|
|
||
| setIsRequestingBackup(true); | ||
| try { | ||
| const success = await requestRemoteBackup(activeSite.slug); | ||
| return success; | ||
| } finally { | ||
| setIsRequestingBackup(false); | ||
| } | ||
| }, [activeSite, isDependentMode, isRequestingBackup]); | ||
|
|
||
| return { | ||
| requestBackup, | ||
| isRequestingBackup, | ||
| isDependentMode, | ||
| canRequestBackup: isDependentMode && !!activeSite, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The structure seems pretty generic, maybe later on it will make sense to have a generic cross-tab client and a useCrossTabRPC( "backup" ) call? Let's watch how this evolves. Nothing blocking or actionable in this comment.
| // This callback is called when another tab requests to take over as main | ||
| // We switch to dependent mode without showing an error | ||
| const remoteUrl = getRemoteUrl(); | ||
| const scopedSiteUrl = `/scope:${encodeURIComponent(site.slug)}/`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a helper for this in
| export function setURLScope(url: URL | string, scope: string | null): URL { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, our scope is actually hardcoded to default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments, none of which need to be addressed before shipping this. Feel free to merge anytime.
7158776 to
b72b187
Compare
48a1290 to
55a7f29
Compare
- Remove export from requestTabShutdown (only used internally) - Remove Vite HMR cleanup code (dev-only, accept occasional HMR quirks)
55a7f29 to
45accb9
Compare
Based on #3162.
Motivation for the change, related issues
When users open the same persistent site in multiple browser tabs, OPFS (Origin Private File System) conflicts can occur because each tab tries to access the same storage. This PR adds a coordination system so only one tab owns the PHP worker while other tabs operate in "dependent mode".
Screenshots
Implementation details
tab-coordinator.ts): Uses BroadcastChannel API for cross-tab communication with ping/pong discovery, takeover requests, and backup delegationboot-site-client.ts): Checks for existing tabs before booting; yields to fresh main tabs or becomes dependent if one existsWorkerStatusIndicatorin address bar shows "Main" or "Dependent" badgeTabInfoWindowin overlay shows WordPress boot time and connected tabsisDependentMode: truein pong, so reloading main correctly re-establishes as mainTesting Instructions (or ideally a Blueprint)