Skip to content

Revert "Implement GutenbergKit ViewModel Architecture (#22087)" #22127

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

Merged
merged 1 commit into from
Aug 14, 2025

Conversation

oguzkocer
Copy link
Contributor

Summary

Reverts the GutenbergKitViewModel implementation as it may create configuration change issues and adds complexity without clear benefits.

Note that this was a clean revert, so it doesn't include any additional changes.

Reason

The ViewModel approach mixes static configuration with dynamic content and could cause state synchronization problems. The Bundle-based approach is simpler and more appropriate for passing initialization parameters.

Testing Instructions

Check for any regressions.

@oguzkocer oguzkocer added this to the 26.1 milestone Aug 14, 2025
@oguzkocer oguzkocer added Posting/Editing Gutenberg Editing and display of Gutenberg blocks. labels Aug 14, 2025
@dangermattic
Copy link
Collaborator

dangermattic commented Aug 14, 2025

1 Warning
⚠️ This PR is assigned to the milestone 26.1. The due date for this milestone has already passed.
Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

Copy link

@wpmobilebot
Copy link
Contributor

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr22127-b637fad
Commitb637fad
Direct Downloadwordpress-prototype-build-pr22127-b637fad.apk
Note: Google Login is not supported on these builds.

@oguzkocer oguzkocer marked this pull request as ready for review August 14, 2025 04:30
@oguzkocer oguzkocer requested a review from dcalhoun August 14, 2025 04:30
@wpmobilebot
Copy link
Contributor

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr22127-b637fad
Commitb637fad
Direct Downloadjetpack-prototype-build-pr22127-b637fad.apk
Note: Google Login is not supported on these builds.

Copy link

codecov bot commented Aug 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 39.43%. Comparing base (cbbde75) to head (b637fad).
⚠️ Report is 1 commits behind head on release/26.1.

Additional details and impacted files
@@              Coverage Diff              @@
##           release/26.1   #22127   +/-   ##
=============================================
  Coverage         39.43%   39.43%           
=============================================
  Files              2149     2149           
  Lines            101872   101872           
  Branches          15604    15604           
=============================================
  Hits              40177    40177           
  Misses            58126    58126           
  Partials           3569     3569           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not encounter any regressions while smoke testing GutenbergKit.

The ViewModel approach mixes static configuration with dynamic content and could cause state synchronization problems. The Bundle-based approach is simpler and more appropriate for passing initialization parameters.

For my own understanding, would you mind sharing an explicit example(s) of synchronization problems? Are they severe or prevalent enough to target the beta branch?

@oguzkocer
Copy link
Contributor Author

For my own understanding, would you mind sharing an explicit example(s) of synchronization problems? Are they severe or prevalent enough to target the beta branch?

@dcalhoun This is a precautionary revert, rather than addressing a bug I've found. While working on EditPostActivity, and specifically that regression you've reported, I am better understanding how the pieces fit together.

The way GutenbergKitViewModel is used at the moment creates architectural concerns, because we don't use the reactive nature of the live data and only hold the settings. The problem is that those settings include static settings, such as the siteUrl, locale etc, but also dynamic ones such as postTitle, postContent etc. The activity life cycles, fragment restorations and the fact that view models survive configuration changes mean that there are possible cases where the View Model may contain stale data. This is probably preventable, and may not be happening at the moment, but we'd need to pay extra attention to it. On the other hand, the previous solution did not have these issues and since the view model is not currently adding any value, I believe it's best to revert it.

As for whether to target the frozen branch or not, I think there is a trade-off:

  • If we revert, it's a clean one and we only go back to where we were just prior to code freeze. However, we don't get the testing benefit from the beta cycle.
  • If we don't revert, we use the beta tested version, but we might be introducing edge cases. If we revert this in trunk, the data that we collect for this version - for example bug reports - will not be as valuable because there will always be the possibility of the View Model change being the root cause.

Considering this is behind an experimental flag, I believe it'd be best to revert this in the frozen branch. In my opinion, we want to be as close to the final version as possible in every release we do for this experimental feature to get the most out of this period.

What do you think?

@dcalhoun
Copy link
Member

@oguzkocer thank you for elaborating on the context and rationale. I agree, it makes sense to revert in the release branch.

@oguzkocer oguzkocer merged commit 205b7da into release/26.1 Aug 14, 2025
30 checks passed
@oguzkocer oguzkocer deleted the revert/remove-gutenbergkit-viewmodel branch August 14, 2025 20:32
oguzkocer added a commit that referenced this pull request Aug 15, 2025
oguzkocer added a commit that referenced this pull request Aug 15, 2025
* Revert "Implement GutenbergKit ViewModel Architecture (#22087)" (#22127)

This reverts commit 890d484.

* Fix `GutenbergKit` editor issues during dark mode toggle (#22135)

* Remove isEditorStarted flag to fix content loss during activity restart

The isEditorStarted flag was preventing editor reinitialization during
activity restarts, but this caused content loss because:

1. Activity restart triggers new fragment creation
2. isEditorStarted gets reset to false in onDestroy()
3. startWithEditorSettings() gets called but always rebuilds from original data
4. Current editor content is lost instead of being preserved

Root cause: The flag was designed to prevent duplicate initialization within
a single fragment instance, but during activity restarts, we WANT the editor
to reinitialize with the current content, not block reinitialization entirely.

Changes:
- Remove isEditorStarted field and related state management
- Remove isEditorStarted checks in startWithEditorSettings()
- Allow editor to reinitialize after activity restart
- Fixes content loss during dark mode toggle

* Fix double ViewPager setup during auth state changes

Prevent duplicate editor initialization by:
1. Only calling fetchWpComCookies() if not already in Success state
2. Adding ViewPager state check to prevent duplicate setup
3. Using distinctUntilChanged() to prevent duplicate LiveData emissions
4. Consolidating Success handling logic in handleSuccessfulAuth()

Changes:
- Check auth state before calling fetchWpComCookies()
- Add ViewPager adapter check with error logging for duplicate setup attempts
- Add distinctUntilChanged() to auth state observer
- Extract handleSuccessfulAuth() for consistent Success state handling
- Remove debug logging from production code

* Update translations

* Update translations

* Bump version number

* Update WordPress metadata translations for 26.1

* Update Jetpack metadata translations for 26.1

---------

Co-authored-by: Oguz Kocer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gutenberg Editing and display of Gutenberg blocks. Posting/Editing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants