MemberJunction / MemberJunction/MJ

Follow-ups from #3108: error-path NotifyLoadComplete, attachView hazard audit, regression test, docs

Open
#3,144 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TSQL
Stars
29
Forks
6
Avg merge
1d 8h
Merged PRs (30d)
308

Description

Follow-up from the review of #3108 (fixes #3106) — non-blocking items the reviewer flagged that shouldn't evaporate after merge.

## 1. Error-path hardening for `NotifyLoadComplete()`
Both `BaseDashboard.ngOnInit()` and the newly-async `BaseDashboard.Refresh()` skip `NotifyLoadComplete()` if `loadData()` rejects. A dashboard that hits a load error still won't render its error state, and the shell's loading screen hangs indefinitely.
- Wrap both call sites in `try/finally` with `NotifyLoadComplete()` in the `finally`.
- File: `packages/Angular/Explorer/shared/src/lib/base-dashboard.ts`

## 2. Audit other `ApplicationRef.attachView()` sites for the same hazard class
#3108 fixed the CD-scheduler blind spot for resource components hosted via `createComponent()` + `attachView()` in the tab container. At least ~11 other sites share the same hosting pattern and are exposed to the identical silent-no-rerender bug on async state changes outside the Angular zone/CD tick:
- Dialog services: `dialog.service`, `confirm-dialog`, `feedback-dialog`, `form-presenter.service`, `create-agent.service`
- The AI test harness window manager

(`dashboard-viewer` is already covered since it hosts `BaseDashboard` subclasses, which inherit the fix via `NotifyLoadComplete`.)

Audit each site for async content updates that could land on a detached/cached view and need an equivalent `markForCheck()`/`RefreshView()` call.

## 3. Commit the Playwright regression probe
The root-cause diagnosis in #3108 used a live `window.ng` introspection probe (attach confirmed, ticks running, view skipped → post-fix DOM/instance sync validated) described in the PR discussion but never checked into the repo. CD-scheduler behavior needs a real browser to test meaningfully — add it to the browser regression suite so this doesn't regress silently on a future Angular CD change (this bug's root cause already shifted once, in Angular 17.1/18).

## 4. Document the `RefreshView()` rule
The existing `NotifyLoadComplete()` requirement is documented, but the new rule this PR establishes — "async state changes delivered outside the load lifecycle (e.g. via `OnQueryParamsChanged`, or any handler not triggered from inside the attached view) need an explicit `this.RefreshView()` / `markForCheck()` call" — is currently only discoverable by reading the diff. Document it next to the `NotifyLoadComplete()` rule in:
- `packages/Angular/Explorer/dashboards/CLAUDE.md`
- `guides/DASHBOARD_BEST_PRACTICES.md`

---
Source: review discussion on #3108, https://github.com/MemberJunction/MJ/pull/3108#pullrequestreview (2026-07-09 and 2026-07-13 reviews by @rkihm-BC)

Contributor guide

Open the contributing guide

Research direction

Start with packages/Angular/Explorer/shared/src/lib/base-dashboard.ts and verify the two loadData error paths against the existing NotifyLoadComplete rule. Then audit the listed ApplicationRef.attachView sites, inspect the browser regression suite for the Playwright probe, and read the existing guidance in the two named documentation files. Done means error states finish loading, affected async views are refreshed, the regression probe is committed, and both guides state the RefreshView rule.

Written by the indexing model from the issue text.

Assessment

Tech stack
angular, playwright, typescript
Domain
documentation, frontend, testing
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.