Skip to content

feat: Include content block in the search embedding - EXO-88570 - #562

Open
mkrout wants to merge 1 commit into
developfrom
develop-89159
Open

feat: Include content block in the search embedding - EXO-88570#562
mkrout wants to merge 1 commit into
developfrom
develop-89159

Conversation

@mkrout

@mkrout mkrout commented Aug 11, 2026

Copy link
Copy Markdown
Member

This commit broadcasts page layout/permissions update events for reactive indexing.

@boubaker

Copy link
Copy Markdown
Member

Review — eXIP 7.3.0.13 "Page Searching" (Layout part)

This PR is one of three implementing the specification. The full cross-repo review lives on Meeds-io/social#5974 — please read it there:

👉 Meeds-io/social#5974 (comment)

It covers social#5974, layout#562 (this PR) and notes#1747 as one changeset, ordered most-severe first.

What concerns this PR specifically

🟠 HighPR build is red. layout-service fails to compile with 4 × cannot find symbol on io.meeds.social.cms.plugin.PageUrlResolver / io.meeds.social.cms.service.PageUrlResolverService. This is merge-ordering: social#5974 has to publish its social-component-api snapshot first. Worth re-running once it lands, so the green build is on record before merge.

🟡 MediumOnly 2 of ~7 page-mutating paths broadcast. PageLayoutService.java:363 and :437 broadcast, but createPage (:220), clonePage (:256), updatePageApplicationPreferences (:296/:303) and restorePageLayout (:378) all persist page state silently. updatePageApplicationPreferences is the sharpest: the Single Note View portlet preference is what names the CMSSetting, so changing it swaps which note the page shows while the index keeps serving the old content.

🟠 High (cross-repo)Nothing ever unindexes. No production unindex call exists anywhere across the three PRs, so a deleted page keeps a live ES document carrying its old permissions. The spec assigns this to Layout — "Existing page lifecycle events (LayoutService) trigger unindex on page deletion" — so the page-deletion event Social needs is missing from this PR. Details in the main review.

Nice touch on LayoutPageUrlResolver: routing primitives genuinely do live in webui, and resolving the nav node here rather than exposing that dependency to Social is the right call. LayoutPageUrlResolverTest covers the null-root, no-match, match and throwing paths.

Classification: N1 for the changeset as a whole (ACL/permission filtering and a trust boundary on the Social side). This PR should not merge ahead of, or independently of, social#5974.

🤖 Generated with Claude Code

@boubaker

Copy link
Copy Markdown
Member

Review round 2 — eXIP 7.3.0.13 "Page Searching" (Layout part)

Full cross-repo re-review on Meeds-io/social#5974:

👉 Meeds-io/social#5974 (comment)

(round 1: Meeds-io/social#5974 (comment))

This PR is unchanged at 2a3df76 since round 1, while social#5974 moved forward — which changed the significance of one finding here.

Round 1 status for this PR

# Sev Finding Status
1 🟠 PR build red (cannot find symbol PageUrlResolver ×4) Merge ordering only — needs social#5974's social-component-api snapshot published, then re-run
2 🟡 Only 2 of ~7 page-mutating paths broadcast Open — and now escalated, see below
3 🟠 Nothing ever unindexes (cross-repo) ⚠️ Partial — Social added a reconcile path, but it depends on events this PR doesn't broadcast

🟠 High (escalated from 🟡) — the missing broadcasts now make a shipped Social feature inert

PageLayoutService.java:296-332

    layoutService.save(application.getState(), portletPreferences);
    // no broadcast — yet this is what repoints a Single Note View at another note
  }

Social's new revision added unindexDetachedBlocks, whose javadoc states it handles a block "removed from its layout, or its content-block portlet preference repointed elsewhere". The second half never happens: updatePageApplicationPreferences is the method that repoints that preference, and it broadcasts nothing. createPage (:220) and restorePageLayout (:378) are silent too — so a page created from a template already carrying a content block, or restored to its shipped layout, never enters the search index at all.

In round 1 this was a 🟡 gap in event coverage. Now that Social ships code whose documented behaviour depends on these events, it's a 🟠: the feature reads as implemented and does nothing.

Fix: broadcast PAGE_UPDATED_EVENT from updatePageApplicationPreferences, createPage and restorePageLayout, alongside the two existing broadcasts at :363 and :437.


🟡 Medium — page deletion (cross-repo, no change needed here after all)

Round 1 suggested Layout should add a page-deletion broadcast. Correction: it doesn't need to. The portal already broadcasts LayoutService.PAGE_REMOVED (org.exoplatform.portal.config.DataStorage.pageRemoved), covering every deletion path including the bulk removePages(siteKey) in SiteLayoutService.deleteSite. Social can listen to it directly — details in the main review. Nothing to do in this PR for that item.


LayoutPageUrlResolver remains a clean piece of work: resolving the nav node here rather than exposing gatein's webui routing primitives to Social is the right call, and its test covers the null-root, no-match, match and throwing paths.

Classification: N1 for the changeset as a whole. This PR should not merge ahead of, or independently of, social#5974.

🤖 Generated with Claude Code

@mkrout

mkrout commented Aug 12, 2026

Copy link
Copy Markdown
Member Author

Fixes applied — response to round 2 review

Pushed at b1f3d5a4. Addresses finding B from social#5974's round 2 review (cross-repo review, this PR's part).

B. 🟠 Detached-block path unreachable — ✅ Fixed

PageLayoutService.createPage(), updatePageApplicationPreferences() and restorePageLayout() now broadcast PAGE_UPDATED_EVENT, same as updatePageLayout()/updatePagePermissions() already did. updatePageApplicationPreferences was the sharpest gap — it's what repoints a Single Note View's portlet preference, exactly the case the social-side listener's javadoc claimed to handle but couldn't reach.

Tests: added verify(listenerService).broadcast(...) assertions to the three corresponding PageLayoutServiceTest cases. Full layout-service suite green.

Unrelated discovery, now resolved

While verifying this locally, layout-service failed to compile against the checked-out gatein-portalImportMode.RESTORE_DEFAULTS and UserPortalConfigService.isDefaultPage(PageKey) (used by restorePageLayout, pre-existing code from EXO-88526/EXO-88530) don't exist on that snapshot. Root cause: the local gatein-portal checkout was 4 commits behind origin/develop. Fast-forwarded to catch up — no conflict with this PR's files, unrelated to the fixes above, just noting it in case others hit the same stale-snapshot issue.

🤖 Generated with Claude Code

@boubaker

Copy link
Copy Markdown
Member

Review round 3 — eXIP 7.3.0.13 "Page Searching" (Layout part)

Full cross-repo review on Meeds-io/social#5974:

👉 Meeds-io/social#5974 (comment)

(earlier rounds: 1 · 2)

Re-reviewed at b1f3d5a. All findings against this PR are now resolved.

Status

# Sev Finding Status
1 🟠 Only 2 of ~7 page-mutating paths broadcast — leaving Social's unindexDetachedBlocks unable to fire for the case its javadoc names FixedcreatePage (:250), updatePageApplicationPreferences (:330) and restorePageLayout (:397) now broadcast PAGE_UPDATED_EVENT, each with a verify(listenerService).broadcast(...) assertion in PageLayoutServiceTest
2 🟡 Page deletion had no broadcast Resolved elsewhere — correctly, via Meeds-io/portal#1303 rather than here. Routing it through the portal's own remove(Page) covers site deletion and node deletion too, which a Layout-side broadcast would have missed
3 🟠 PR build red Merge ordering only — the four errors are all PageUrlResolver / PageUrlResolverService, awaiting social#5974's social-component-api snapshot

The three broadcasts landed exactly where they matter: updatePageApplicationPreferences is what repoints a Single Note View's portlet preference at a different note, and without it Social's reconcile path was documented-but-dead. Adding the assertion to the existing tests rather than writing new ones keeps the diff honest and small.

LayoutPageUrlResolver remains unchanged and is still the right shape — resolving the nav node here rather than exposing gatein's webui routing primitives to Social.

Merge order

portal#1303 → social#5974 → layout#562 / notes#1747

Nothing further needed in this PR; re-run CI once social publishes.

Classification: N1 for the changeset as a whole (ACL filtering and a trust boundary on the Social side, plus core event broadcasting in portal). This PR should not merge ahead of, or independently of, social#5974.

🤖 Generated with Claude Code

@mkrout
mkrout force-pushed the develop-89159 branch 2 times, most recently from 01490d1 to 3b34ff1 Compare August 13, 2026 17:00
…P7.3.0.13

This commit broadcasts page layout/permissions update events for reactive indexing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants