Skip to content

fix(uve): QA feedback on prevent breadcrumb from showing stale title during navigation#35856

Merged
adrianjm-dotCMS merged 11 commits into
mainfrom
35807-uve-breadcrumb-stale-title-qa-feedback
May 29, 2026
Merged

fix(uve): QA feedback on prevent breadcrumb from showing stale title during navigation#35856
adrianjm-dotCMS merged 11 commits into
mainfrom
35807-uve-breadcrumb-stale-title-qa-feedback

Conversation

@adrianjm-dotCMS
Copy link
Copy Markdown
Member

@adrianjm-dotCMS adrianjm-dotCMS commented May 28, 2026

Fixes #35807

What changed

dot-ema-shell.component.ts

  • Replaced untracked(() => this.uveStore.pageParams()?.url) with full URL construction via pageFriendlyParams() + normalizeQueryParams() + router.createUrlTree(), producing a proper clickable /dotAdmin/#/edit-page/content?... breadcrumb link instead of just the raw page path
  • Added early-return guard if (!this.uveStore.pageParams()) return null inside untracked to prevent a crash in headless configs where uveConfig.url is set — normalizeQueryParams calls new URL(params.clientHost) which throws if pageParams was cleared by resetPageParams() before the effect is torn down

dot-ema-shell.component.spec.ts

  • Updated 3 existing URL assertions to match the #/edit-page/content?... format; one uses stringMatching to verify the /dotAdmin/# prefix
  • Replaced the stale-data-during-LOADING test with a navigation accumulation test that explicitly asserts the breadcrumb is not called during LOADING and is called exactly once with the new page data after LOADED
  • Existing destroy-race test (resetPageParams() + cycle uveStatus) still covers the null-pageParams crash path

Test plan

  • Navigate between pages in the UVE — breadcrumb should show the correct title after each navigation
  • Click a breadcrumb link — should navigate back to the correct page in the editor
  • Open and close the UVE — no console errors

This PR fixes: #35807

adrianjm-dotCMS and others added 5 commits May 25, 2026 14:36
…g navigation

Guard $updateBreadcrumbEffect with status === UVE_STATUS.LOADED so the
breadcrumb only updates once the new page's data has fully resolved from
the API. Previously, keeping the previous pageAsset visible during fetch
(introduced in #35539) caused the effect to fire with stale title + new
URL, and addNewBreadcrumb's URL deduplication then blocked the correct
update from ever landing.

Closes #35807

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d optional chains

Add untracked() on pageParams to prevent TypeError when resetPageParams()
nulls the signal during ngOnDestroy before the effect is torn down.
Also remove dead optional chaining on page.title/page.identifier since
page is already narrowed truthy by the if-guard.

Adds a test covering the destroy path: resetPageParams() while status
is LOADED must not throw and must not call addNewBreadcrumb.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Move untracked url read inside the if-guard so it only runs when the
  breadcrumb is actually going to be updated
- Add inline comment explaining the dual purpose of untracked (per
  store CLAUDE.md rule on documenting non-obvious untracked usage)
- Fix destroy regression test to actually exercise the async teardown
  race: cycle uveStatus after resetPageParams() so the effect re-fires
  with pageParams = null, confirming no TypeError is thrown

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 28, 2026

Claude finished @adrianjm-dotCMS's task in 2m 54s —— View job


Review

  • Read PR context
  • Read changed files
  • Review for bugs, risks, edge cases
  • Post review

Overall: The refactor from effect() + untracked() to computed + signalMethod is a clean expression of intent (signalMethod is purpose-built for "react to a signal source"). The crash fix and URL-construction fix both look correct. A few notes worth a second look:

🟡 Latent bug papered over, not fixed

normalizeQueryParams (libs/portlets/edit-ema/portlet/src/lib/utils/index.ts:526-531) calls new URL(params.clientHost) whenever baseClientHost is truthy — with no guard against params.clientHost being undefined. The component now adds if (!this.uveStore.pageParams()) return as a defensive fence, but the utility itself remains fragile. Any other caller in a headless config that passes empty params will hit the same crash. Consider a params?.clientHost && guard in the util as a follow-up.

🟡 Breadcrumb URL captures the current sub-tab, not always /content

this.#router.createUrlTree([], { queryParams }) (line 222) builds the URL from the current router state. If the breadcrumb effect fires while the user is sitting on /edit-page/layout (e.g., after a page reload while on the layout tab), the breadcrumb's clickable URL becomes /dotAdmin/#/edit-page/layout?..., not /dotAdmin/#/edit-page/content?... as the PR description claims. The test (expect.stringMatching(/^\/dotAdmin\/#.*url=index/)) is loose enough not to catch this. Is this intentional (return user to where they were) or should the URL be pinned to content? Worth verifying in QA on layout/rules tabs.

🟡 Test name oversells the assertion

The new test is named "should replace breadcrumb on navigation, not accumulate stale entries" (spec.ts:1164), but the assertion is toHaveBeenCalledTimes(1) — that proves the component fires once with fresh data and not during LOADING, not that the breadcrumb list is replaced rather than appended. Replacement vs. accumulation is decided by GlobalStore.addNewBreadcrumb's shouldReplaceLastCrumb logic (breadcrumb.feature.ts:164), which is outside this PR. Either rename the test to match what it asserts (e.g., "should not call addNewBreadcrumb during LOADING and call it once after LOADED") or add a separate assertion against globalStore.breadcrumbs() to actually cover the "no stale accumulation" claim.

✅ Things that look right

  • The destroy-race test (resetPageParams() + cycle uveStatus) still covers the null-pageParams crash path — good.
  • signalMethod from @ngrx/signals 21.0.1 wraps the processor in untracked internally, so reading pageParams() / pageFriendlyParams() inside the body won't create unwanted reactive deps — the source is exclusively $breadcrumbPage.
  • $breadcrumbPage correctly gates on status === LOADED, so the LOADING → LOADED transition naturally fires the method once per navigation. Stale data during LOADING is filtered out.
  • Tests updated consistently with the new /dotAdmin/#... URL format.

Minor / nits

  • The Spanish comment block from the original was dropped — the new version is more readable, no objection.
  • Constructor wiring (this.$updateBreadcrumb(this.$breadcrumbPage)) is the standard signalMethod pattern and runs in injection context. ✓
    Branch

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Revised comments in the DotEmaShellComponent to enhance understanding of the breadcrumb functionality. The changes clarify the purpose of the untracked function, specifically its role in preventing unnecessary re-execution on URL changes and avoiding potential TypeErrors during component destruction.
Updated the DotEmaShellComponent tests to better handle loading states during navigation. Introduced a Subject to simulate pending requests, ensuring that breadcrumb actions are correctly suppressed while data is loading. This change improves the accuracy of the tests by reflecting the component's behavior during asynchronous data fetching.
…ponent

Updated the DotEmaShellComponent to ensure that breadcrumbs are only added when a valid URL is present. This change prevents potential errors when the page parameters are not available, enhancing the stability of breadcrumb functionality. Additionally, updated the corresponding test to match the new URL validation logic.
@adrianjm-dotCMS adrianjm-dotCMS changed the title fix(uve): prevent breadcrumb from showing stale title during navigation fix(uve): QA feedback on prevent breadcrumb from showing stale title during navigation May 28, 2026
@adrianjm-dotCMS adrianjm-dotCMS marked this pull request as ready for review May 28, 2026 19:04
Updated the DotEmaShellComponent to utilize signalMethod for breadcrumb updates, ensuring that breadcrumbs are added only when valid page data is available. This change improves the clarity of the breadcrumb logic and eliminates the need for untracked functions, enhancing overall component performance and maintainability.
@adrianjm-dotCMS adrianjm-dotCMS added this pull request to the merge queue May 29, 2026
Merged via the queue into main with commit 0811aa2 May 29, 2026
32 checks passed
@adrianjm-dotCMS adrianjm-dotCMS deleted the 35807-uve-breadcrumb-stale-title-qa-feedback branch May 29, 2026 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

UVE: dotAdmin header shows stale page title when navigating between pages in editor

2 participants