RTC: Change SyncConnectionModal to isSyncConnectionErrorHandled filter and drop IS_GUTENBERG_PLUGIN check#76853
Merged
ingeniumed merged 8 commits intotrunkfrom Apr 7, 2026
Merged
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ingeniumed
added
[Type] Enhancement
|
Size Change: +4 B (0%) Total Size: 7.74 MB 📦 View Changed
ℹ️ View Unchanged
|
|
Flaky tests detected in f2d5dc7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/24104844473
|
Contributor
|
The main downside of this approach is that we make the What if we instead just broadcast a signal with the error message, and allow a plugin to take charge of showing the error? Something like: const editorOverride = wp.hooks.applyFilters(
'editor.SyncConnectionError',
null,
errorCode
);
if ( null === editorOverride ) {
// Show modal normally
} else {
// Do nothing, a plugin has taken over displaying this error
}This should keep the connection error API simple and more flexible for consumers. What do you think? |
Contributor
Author
|
Summarizing the internal discussion that happened after the point about the current filter location locking in the
|
Contributor
Author
This would be similar to option 2 and the one I like as well. I'll see what I can whip up for this |
Simplify the sync connection error modal by removing the withFilters
component wrapper pattern in favor of a lightweight applyFilters gate.
Plugins can now signal they are handling a sync error by returning a
non-null value from the 'editor.SyncConnectionError' filter, which
suppresses the default modal. This avoids exposing modal props as a
permanent public API while remaining flexible for consumers.
Also merges DefaultSyncConnectionErrorModal into the parent component
since the separate component is no longer needed without withFilters,
and fixes a typo ("intial" -> "initial").
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ingeniumed
changed the title
Contributor
Author
I have re-done the code with the following logic:
I've also dropped the dual function logic that was in place to support the inner UI of the modal being filterable. That wasn't necessary anymore, and so it's a lot simpler as a result. I have also updated the description of the PR to reflect these new changes. Appreciate the feedback and resulting discussion that led to this - @alecgeatches, @chriszarate and @youknowriad. |
With withFilters removed in favor of applyFilters, the separate DefaultSyncConnectionErrorModal component and its props interface are no longer needed. Inline all modal rendering logic directly into SyncConnectionErrorModal for simplicity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
alecgeatches
reviewed
Apr 6, 2026
| * ``` | ||
| */ | ||
| const isSyncErrorHandledByPlugin = | ||
| applyFilters( 'editor.SyncConnectionError', null, error?.code ) !== |
Contributor
There was a problem hiding this comment.
I think I suggested null above, but could this be false (and below if ( isSyncErrorHandledByPlugin === false ))? false is the normal pattern when a filter is unimplemented (like pre_option_), and null almost seems like it might be a React component, but it isn't.
Contributor
There was a problem hiding this comment.
One more thing, is there anything weird that happens with this filter getting called on every SyncConnectionErrorModal() rerender?
Contributor
Author
There was a problem hiding this comment.
You are right. I had made that change locally, but I'm guessing during my multiple refactors I did I dropped this at some point. I'll add it in
alecgeatches
reviewed
Apr 6, 2026
| connectionStatus?.status === 'disconnected' && | ||
| connectionStatus.backgroundRetriesFailed | ||
| ) { | ||
| setShowModal( true ); |
Contributor
There was a problem hiding this comment.
@ingeniumed I have a concern here. If we don't have a 'disconnected' status, e.g. connection-limit-exceeded, then connectionStatus?.status === 'disconnected' is never true, and then we never setShowModal( true ), and then we never reach the editor.SyncConnectionError filter because we return null before that.
Contributor
Author
There was a problem hiding this comment.
This wasn't an issue because the disconnected status is the status we send out for any errors. The subsequent error code is what's changed - network offline, connection limit exceeded, etc.
It'll work for any unrecoverable error, which means no retry being allowed. If a retry is allowed its a recoverable error. So at the moment that means only two errors take advantage of this:
- CONNECTION_LIMIT_EXCEEDED
- DOCUMENT_SIZE_LIMIT_EXCEEDED
Contributor
Author
There was a problem hiding this comment.
I've improved this further in 99f8d69. I also found a regression that I had caused when it came to automatic retries vs manual retries that I have resolved.
Replace the withFilters component-wrapping pattern with a simpler applyFilters gate that lets plugins signal they handle specific sync connection errors. The filter only applies for unrecoverable errors (no retry available), keeping the default modal for transient failures. Also broadens modal visibility to any non-connected status, not just disconnected with exhausted retries, so errors like connection-limit-exceeded show the modal immediately. Includes an e2e test plugin demonstrating the filter for connection-limit-exceeded, using registerPlugin and a separate JS file following codebase conventions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
I have added in an e2e test that implements a plugin with this filter. The above is a screenshot of what it looks like to implement a simple modal for offering an upgrade option for collaboration. |
alecgeatches
reviewed
Apr 7, 2026
alecgeatches
approved these changes
Apr 7, 2026
Contributor
alecgeatches
left a comment
alecgeatches
left a comment
There was a problem hiding this comment.
Looking good! Thank you for the e2e plugin example. I added one nit above about the applyFilters() filter name and typing, but should be shippable otherwise.
maxschmeling
approved these changes
Apr 7, 2026
… check Address PR feedback: rename the filter from editor.SyncConnectionError to editor.isSyncConnectionErrorHandled for clarity, and use an explicit !== false check instead of a truthy check to match the WordPress pre_option_ pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ingeniumed
changed the title
github-actions
bot
removed
the
Backport to WP 7.0 Beta/RC
gutenbergplugin
pushed a commit
that referenced
this pull request
Apr 7, 2026
…r and drop IS_GUTENBERG_PLUGIN check (#76853) * Open up the sync modal filter outside of GB * Fix the type * Tweak the doc description * Replace withFilters with applyFilters for sync connection error modal Simplify the sync connection error modal by removing the withFilters component wrapper pattern in favor of a lightweight applyFilters gate. Plugins can now signal they are handling a sync error by returning a non-null value from the 'editor.SyncConnectionError' filter, which suppresses the default modal. This avoids exposing modal props as a permanent public API while remaining flexible for consumers. Also merges DefaultSyncConnectionErrorModal into the parent component since the separate component is no longer needed without withFilters, and fixes a typo ("intial" -> "initial"). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Inline DefaultSyncConnectionErrorModal into SyncConnectionErrorModal With withFilters removed in favor of applyFilters, the separate DefaultSyncConnectionErrorModal component and its props interface are no longer needed. Inline all modal rendering logic directly into SyncConnectionErrorModal for simplicity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add applyFilters gate for unrecoverable sync errors and e2e test Replace the withFilters component-wrapping pattern with a simpler applyFilters gate that lets plugins signal they handle specific sync connection errors. The filter only applies for unrecoverable errors (no retry available), keeping the default modal for transient failures. Also broadens modal visibility to any non-connected status, not just disconnected with exhausted retries, so errors like connection-limit-exceeded show the modal immediately. Includes an e2e test plugin demonstrating the filter for connection-limit-exceeded, using registerPlugin and a separate JS file following codebase conventions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Rename filter to editor.isSyncConnectionErrorHandled and use explicit check Address PR feedback: rename the filter from editor.SyncConnectionError to editor.isSyncConnectionErrorHandled for clarity, and use an explicit !== false check instead of a truthy check to match the WordPress pre_option_ pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: ingeniumed <ingeniumed@git.wordpress.org> Co-authored-by: alecgeatches <alecgeatches@git.wordpress.org> Co-authored-by: maxschmeling <maxschmeling@git.wordpress.org>
github-actions
bot
added
the
Backported to WP Core
|
I just cherry-picked this PR to the wp/7.0 branch to get it included in the next release: 2edee4a |
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
3 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.
What?
Change the
SyncConnectionModalfilter toisSyncConnectionErrorHandled, and make it a conditional modal rendering signal instead. This filter was introduced in #76554 for testing before 7.0's release.Why?
With the plugin check, Gutenberg is needed in order to customize the modal using the filter. In addition, this filter makes the contract between the props of the modal and the consumers immutable. This makes it quite inflexible for all parties involved.
How?
So instead, the following is being done:
IS_GUTENBERG_PLUGINcheck entirely.isSyncConnectionErrorHandledTesting Instructions
Use of AI Tools
Used Claude Code for implementation, testing and reviewing the code.