Skip to content

Always use topological sort for published pages#22896

Open
oguzkocer wants to merge 3 commits into
trunkfrom
fix/page-list-topological-sort-for-all-pages
Open

Always use topological sort for published pages#22896
oguzkocer wants to merge 3 commits into
trunkfrom
fix/page-list-topological-sort-for-all-pages

Conversation

@oguzkocer
Copy link
Copy Markdown
Contributor

Description

#10476 introduced MAX_TOPOLOGICAL_PAGE_COUNT = 100 to cap the topological (hierarchical) sort and fall back to sorting by publish date for sites with 100+ published pages. This was done to match Calypso's behavior at the time.

The date-based fallback produced a completely different ordering than what users see on the web — pages lost their parent/child hierarchy and indentation, and edited pages could jump to the top of the list due to transient local state during save.

This commit:

  • Removes MAX_TOPOLOGICAL_PAGE_COUNT and the date-based fallback so all published pages are sorted hierarchically regardless of count.
  • Rewrites topologicalSort to build a parentId → children lookup map in a single groupBy pass then walk the tree once, instead of re-scanning the full page list on every recursive call.

To Test

  • Compare the page list in the app with web UI. Prior to this change, if you test a site with more than 100 pages or change the MAX_TOPOLOGICAL_PAGE_COUNT to something like 2, you can verify that the ordering is completely different from the web.

Although I don't know why Calypso had the 100 pages limit, maybe due to network request page sizing, I don't believe it makes sense in our case because we fetch all the pages before we show it to the user.

I couldn't find a compelling reason to keep this behavior. The only consideration I could think of was the performance cost, because the current implementation is O(n^2) but this PR improves that to O(n).

Fixes CMM-2065

PR #10476 introduced `MAX_TOPOLOGICAL_PAGE_COUNT = 100` to cap
the topological (hierarchical) sort and fall back to sorting by
publish date for sites with 100+ published pages. This was done
to match Calypso's behavior at the time.

The date-based fallback produced a completely different ordering
than what users see on the web — pages lost their parent/child
hierarchy and indentation, and edited pages could jump to the
top of the list due to transient local state during save.

This commit:

- Removes `MAX_TOPOLOGICAL_PAGE_COUNT` and the date-based
  fallback so all published pages are sorted hierarchically
  regardless of count.
- Rewrites `topologicalSort` to build a `parentId → children`
  lookup map in a single `groupBy` pass then walk the tree once,
  instead of re-scanning the full page list on every recursive
  call.
@oguzkocer oguzkocer added this to the 26.8 milestone May 26, 2026
@dangermattic
Copy link
Copy Markdown
Collaborator

dangermattic commented May 26, 2026

1 Warning
⚠️ This PR is assigned to the milestone 26.8. 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

@wpmobilebot
Copy link
Copy Markdown
Contributor

wpmobilebot commented May 26, 2026

App Icon📲 You can test the changes from this Pull Request in WordPress Android by scanning the QR code below to install the corresponding build.

App NameWordPress Android
Build TypeDebug
Versionpr22896-6580efc
Build Number1488
Application IDorg.wordpress.android.prealpha
Commit6580efc
Installation URL3hl52ofkna4sg
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot
Copy link
Copy Markdown
Contributor

wpmobilebot commented May 26, 2026

App Icon📲 You can test the changes from this Pull Request in Jetpack Android by scanning the QR code below to install the corresponding build.

App NameJetpack Android
Build TypeDebug
Versionpr22896-6580efc
Build Number1488
Application IDcom.jetpack.android.prealpha
Commit6580efc
Installation URL184lhpc2noqho
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot
Copy link
Copy Markdown
Contributor

🤖 Build Failure Analysis

This build has failures. Claude has analyzed them - check the build annotations for details.

The `sorts 100 or more pages by date DESC` test validated the
date-based fallback that no longer exists. The remaining
topological sort test is renamed since it now applies to all
page counts.
@codecov
Copy link
Copy Markdown

codecov Bot commented May 26, 2026

Codecov Report

❌ Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 37.32%. Comparing base (82411a0) to head (6580efc).

Files with missing lines Patch % Lines
...press/android/viewmodel/pages/PageListViewModel.kt 81.25% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk   #22896      +/-   ##
==========================================
- Coverage   37.32%   37.32%   -0.01%     
==========================================
  Files        2320     2320              
  Lines      124578   124576       -2     
  Branches    16926    16925       -1     
==========================================
- Hits        46498    46495       -3     
  Misses      74319    74319              
- Partials     3761     3762       +1     

☔ 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.

@oguzkocer oguzkocer marked this pull request as ready for review May 26, 2026 08:30
@oguzkocer oguzkocer requested review from a team and adalpari and removed request for a team May 26, 2026 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants