fix(newsletters): correct stale project context on deep links (#1570) - #2224
fix(newsletters): correct stale project context on deep links (#1570)#2224audigregorie wants to merge 8 commits into
Conversation
- newsletter-access.guard: resolve the route's :projectUid (own paramMap, then the child snapshot at the lens mounts) and authorize against that project; fall back to ?project= then active context only when the param is absent or the uid lookup fails - newsletter-manage/analytics: reconcile active context from routeProjectUid via applyEntityProjectContext + computeIsFoundation so page chrome stops showing a stale project over the route's data - e2e: newsletter edit deep-link cases (context switch + guard authorization) in project-context-deep-link.spec.ts Signed-off-by: Audi Young <audi.mycloud@gmail.com>
Signed-off-by: Audi Young <audi.mycloud@gmail.com>
PR SummaryMedium Risk Overview
Coverage adds guard and util unit tests, Playwright cases for edit/analytics/guard with stale cookies, and monorepo Vitest config scoping so root/package test runs do not pick up Angular specs. Reviewed by Cursor Bugbot for commit b05b4b9. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Essentials Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
Pull request overview
Fixes newsletter deep links so authorization and page context follow the route’s project.
Changes:
- Prioritizes
:projectUidin the newsletter access guard. - Reconciles active project context on edit and analytics pages.
- Adds guard unit tests and deep-link E2E coverage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
entity-project-context.util.ts |
Adds route-project context reconciliation. |
newsletter-access.guard.ts |
Authorizes against the route project first. |
newsletter-access.guard.spec.ts |
Tests guard resolution and redirects. |
newsletter-manage.component.ts |
Reconciles context on edit pages. |
newsletter-analytics.component.ts |
Reconciles context on analytics pages. |
project-context-deep-link.spec.ts |
Tests stale-context newsletter deep links. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-2224.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
#1570) Signed-off-by: Audi Young <audi.mycloud@gmail.com>
…-1570) Signed-off-by: Audi Young <audi.mycloud@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
apps/lfx-one/src/app/shared/guards/newsletter-access.guard.ts:56
- The legacy list/create path now adds
_notice=accesson denial, whereas it previously redirected silently with onlyproject. That changes visible behavior on routes without:projectUid, contradicting #1570’s success criterion that create and list pages behave exactly as before. Keep the notice on the route-UID denial branch, but preserve the old query params incheckWriterAccessand update its legacy test expectation.
Signed-off-by: Audi Young <audi.mycloud@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
apps/lfx-one/src/app/shared/utils/entity-project-context.util.ts:198
- No test covers the full-context comparison added here: all new stale-cookie cases use a different UID. A regression back to UID-only equality would therefore pass while a cookie with the correct UID but stale name/logo still renders outdated chrome—the exact edge case this branch handles. Add a case with the same UID and stale metadata and assert the resolved project replaces it.
if (
projectContextService.activeRouteLensKind() === (resolved.isFoundation ? 'foundation' : 'project') &&
isSameProjectContext(projectContextService.activeContext(), resolved.context)
) {
audigregorie
left a comment
audigregorie
left a comment
There was a problem hiding this comment.
Code Review Summary
Solid, well-scoped fix: the route's :projectUid is now authoritative for both authorization (guard) and page chrome (reconciliation), with the legacy slug chain kept as a documented fallback. I verified the subtle claims rather than taking them on faith — the mount-level firstChild resolution matches app.routes.ts, the guard/page share the getProject(uid, false) shareReplay cache (no extra request), _notice: 'access' is whitelisted in AppComponent, and the e2e stubs genuinely satisfy computeIsFoundation and Playwright's reverse-registration ordering. Local checks: yarn check-types ✅, targeted ESLint ✅, new guard spec 13/13 ✅; CI all green.
One Minor thread below (comment convention in the new e2e case). Nothing blocking.
What's done well
- Degradation philosophy documented at every decision point (null uid → legacy chain instead of denial; relation-gated null → context untouched; ED fail-open) — comments explain why, not just what.
- Layered tests: 13 guard unit tests covering every branch, plus e2e cases for both pages with the deliberate "analytics regression would pass edit-route cases" separation.
- The comment claims hold up under verification — mount-level guard invocation shape, cache sharing, MainLayout re-assert ordering, and the negative-assertion
waitForTimeoutpattern all match the surrounding codebase.
Signed-off-by: Audi Young <audi.mycloud@gmail.com>
Signed-off-by: Audi Young <audi.mycloud@gmail.com>
luismoriguerra
left a comment
luismoriguerra
left a comment
There was a problem hiding this comment.
Approved — no blocking findings. Reviewed 101d7f56bda4.
Build, format:check, lint:check and shared type-check passed; all 6,051 existing tests passed. Browser E2E not re-run (credentials absent).
Non-blocking: cached-null UID fallback retains stale context; list/create now show an access toast; matching-context suppression can leave a stale ?project= (targeted helper reproduction). Details inline. Merge readiness: confirm the fallback/toast scope exceptions to #1570.
Nit (summary only): update the body’s “without NavigationEnd wiring” and “legacy chain otherwise unchanged” claims.
| // `_notice: 'access'` mirrors writerGuard's denial convention — AppComponent turns it | ||
| // into the generic "Access Denied" toast (survives the SSR redirect, unlike a | ||
| // guard-side MessageService.add, which has no DOM on the server). | ||
| return router.createUrlTree([overviewPath], { queryParams: { project: slug, _notice: 'access' } }); |
There was a problem hiding this comment.
Non-blocking: _notice=access also changes list/create denials from silent redirects to toasts, contrary to #1570’s unchanged-behavior criterion. Either retain the legacy redirect here or document acceptance of this UX change.
There was a problem hiding this comment.
Documenting acceptance of this UX change: legacy-chain denials (list/create) now surface the same "Access Denied" toast writerGuard uses app-wide via _notice: 'access', instead of redirecting silently — a silent redirect read as a broken link. Also corrected the PR body, which had claimed the legacy chain was unchanged, and added a Behavior changes row for it.
Signed-off-by: Audi Young <audi.mycloud@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b05b4b9. Configure here.
| return; | ||
| } | ||
| // Mirror syncEntityProjectContext: only write ?project= to the URL when already present. | ||
| const syncUrl = 'project' in urlParams; |
There was a problem hiding this comment.
Stale project query never repaired
Medium Severity
The new urlAgrees fall-through still cannot repair a stale ?project= param. Both the activation apply and the NavigationEnd re-apply call setProject/setFoundation while router.getCurrentNavigation() is still set, so syncProjectQueryParam no-ops. In Angular 20 the current navigation is cleared only after NavigationEnd subscribers finish, so the old slug remains for the session.
Reviewed by Cursor Bugbot for commit b05b4b9. Configure here.
| // null only ever comes from the catchError above — evict failures so the next | ||
| // caller retries instead of replaying a transient error for the rest of the | ||
| // session (GH-1570: a poisoned entry would pin the newsletter guard and route | ||
| // reconciliation to a stale context). Concurrent subscribers still share this | ||
| // emission; only future lookups re-fetch. | ||
| if (!project) { | ||
| this.projectCache.delete(cacheKey); |
| // Mirror syncEntityProjectContext: only write ?project= to the URL when already present. | ||
| const syncUrl = 'project' in urlParams; | ||
| applyEntityProjectContext(projectContextService, resolved.context, resolved.isFoundation, syncUrl); |
Summary
Newsletter edit and analytics pages could show — and authorize against — the wrong project when opened from a deep link. The URL carries the owning project as
:projectUid, but both the access guard and the page chrome (name, logo, sidebar) followed the?project=query param or a cookie-restored active context, which can be stale when a link is shared or the user switched projects since it was created. This PR makes the route's:projectUidauthoritative for both authorization and page context, with the legacy slug chain kept as a fallback.Resolves #1570
Behavior changes
Newsletter edit/analytics access check
Project shown on newsletter edit/analytics pages
Newsletter list/create access denial
Technical changes
apps/lfx-one/src/app/shared/guards/newsletter-access.guard.ts— Newsletter access guard:projectUidparam first (ownparamMap, thenfirstChildfor the lens-mount invocation), then the legacy?project=→ active-context slug chain.:projectUidresolves, writer access is checked directly on the resolved project; denial redirects to the lens-appropriate overview with the resolved slug and the_notice: 'access'query param (mirroringwriterGuard's toast convention)._notice: 'access', surfacing the same "Access Denied" toast aswriterGuardinstead of redirecting silently.apps/lfx-one/src/app/shared/services/project.service.ts— Project servicegetProjectno longer caches thenullproduced by its error path: a failed lookup is evicted so the next caller retries instead of replaying a transient error for the rest of the session (review feedback — a poisoned entry could pin the guard and route reconciliation to a stale context).apps/lfx-one/src/app/shared/utils/entity-project-context.util.ts— Shared context reconciliationreconcileRouteProjectContexthelper: the uid-only variant of the existing entity-signal syncs, for routes whose URL carries the owning project as:projectUidbut have no enriched entity payload.activeContextUid()so the correction self-heals after later route-lens re-asserts, withoutNavigationEndwiring; re-applies hit theshareReplay-cachedgetProject(the guard already resolved the same uid), so the happy path costs no extra request.?project=is only written to the URL when already present, mirroringsyncEntityProjectContext.?project=falls through so theNavigationEndre-apply repairs it once no navigation is in flight (review feedback — activation-time applies can't repair the URL, and suppressing on context alone left the old slug for the session).apps/lfx-one/src/app/modules/newsletters/newsletter-manage/newsletter-manage.component.ts, apps/lfx-one/src/app/modules/newsletters/newsletter-analytics/newsletter-analytics.component.ts— Newsletter pagesreconcileRouteProjectContextonce from the constructor sodisplayName/logoUrl/sidebar follow the route's project instead of a stale cookie-restored context.apps/lfx-one/src/app/shared/guards/newsletter-access.guard.spec.ts— Guard unit tests:projectUidauthorization over stale query param/context, child-snapshot resolution at the lens mount, null-lookup degradation to the legacy chain, denial with resolved slug + access notice, legacy-chain preference order, fail-closed lookup failure, and lens-appropriate overview redirects.apps/lfx-one/e2e/project-context-deep-link.spec.ts— E2E tests:projectUidrather than the stale context.