MemberJunction / MemberJunction/MJ

graph-view: 3 ui-layers violations (NavigationService from Explorer) + 11 direct UUID comparisons

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

Description

`@AN-BC` — surfaced while triaging the `next` install break (#3849). The `adopted standards` gate reports three UI-layering violations in the `graph-view` component added by #3842:

```
packages/Angular/Generic/graph-view/src/lib/components/graph-view.component.ts:18
imports "@memberjunction/ng-shared" — ng-shared is Explorer (NavigationService /
BaseResourceComponent) — that is L3
imports "NavigationService" — navigation is L3 — emit an intent event instead
packages/Angular/Generic/graph-view/package.json
declares "@memberjunction/ng-shared" — a "widgets" package must not depend on it
→ https://github.com/MemberJunction/MJ/blob/next/guides/UI_LAYERING_GUIDE.md
```

`packages/Angular/Generic/**` is L1. Per the layering guide, nothing below L3 imports an Explorer package — so as it stands the component can only be used inside Explorer, which is the opposite of what its location advertises.

## This is not dead code, and the fix is not free

Worth stating plainly, because the two lines look interchangeable at a glance and they are not:

```ts
// graph-view.component.ts — NavigateToEntity(), lines ~795-805
if (this.navService) {
const pk = CompositeKey.FromID(node.ID);
this.navService.OpenEntityRecord(entityName, pk); // ← PERFORMS the navigation
}
this.NodeNavigated.emit(new NodeNavigatedEventArgs(node, entityName, node.ID)); // ← REPORTS it
```

`NavigationService.OpenEntityRecord` (`navigation.service.ts:464`) opens a record tab through the workspace manager — resolving the active app, its color, the URL segment, and deduping against existing tabs. `NodeNavigated` only notifies. A host wired to the event alone gets a notification and no navigation.

And `inject(NavigationService, { optional: true })` (line 516) is clearly deliberate: navigate automatically inside Explorer, degrade gracefully anywhere else. Removing it is a real API change, not cleanup.

## Proposed fix, and what it costs

Move the navigation decision to L3:

- Drop the `NavigationService` import (line 18), the optional inject (line 516), the `if` block (lines 800-803), and `@memberjunction/ng-shared` from `graph-view/package.json`.
- Hosts own navigation by wiring `(NodeNavigated)` → `NavigationService.OpenEntityRecord`. The event already carries everything needed — node, entity name, ID — and `BeforeNodeNavigate` (line 553) still gives hosts a cancel hook.

**The cost:** clicking a node stops navigating by default. Every Explorer host embedding `` must wire the event or the component looks broken.

**Why now is the cheap moment:** nothing consumes it yet. The only reference to `graph-view` outside its own package is a path mapping in `packages/MJExplorer/tsconfig.json` — no template uses ``, and nothing handles `NodeNavigated`. So the change is observably a no-op today. That stops being true the moment someone embeds it.

If the automatic behavior is worth keeping, the layer-correct shape is a thin L3 wrapper in an Explorer package that hosts the widget and wires the event — the widget stays reusable, Explorer keeps the convenience.

## Why this didn't stop the merge

`adopted standards` is not a required check. The ruleset on `next` requires exactly one — `Check migrations` — so #3842 merged with this, `Run unit tests`, `Integration (SQL Server)`, and `Build new versions of NPM packages` all red. Not a criticism of that PR so much as a note that the gate currently has no teeth; worth a separate conversation about which checks should be required.

## Knock-on

- The DOM coverage ratchet was one over its gate because `graph-view.component.ts` has no spec. Deferred in `ca61460086` (PR #3876) rather than specced, precisely because this issue is likely to change the component's navigation contract — a spec written against today's behavior would need rewriting. **When this lands, remove the deferral from `scripts/dom-test-report.mjs` and backfill a real DOM spec.**
- PR #3876 fixes the install breaks and the ratchet; it does not touch this, deliberately.

## Not in scope

`NavigateToEntity` also hardcodes BizApps entity names as its fallback (`'MJ_BizApps_Common: People'` / `'MJ_BizApps_Common: Organizations'`) inside a package under `Generic`. The standards gate can't see it because it's a string, not an import. Deliberately left alone for now — recorded here only so it isn't mistaken for an oversight later.

## Acceptance criteria

- [ ] `node packages/Standards/bin/run.js check` reports no `ui-layers` violations for `graph-view`
- [ ] `@memberjunction/ng-shared` removed from `packages/Angular/Generic/graph-view/package.json`
- [ ] Navigation behavior either moved to a host/L3 wrapper or consciously dropped, with the decision recorded
- [ ] `scripts/dom-test-report.mjs` deferral for `graph-view.component.ts` removed and a DOM spec added
- [ ] `pnpm-lock.yaml` regenerated if the dependency change alters it

## References

- #3842 — the PR that added `graph-view`
- #3876 — install + ratchet fix (does not address this)
- #3849 — the `next` install break this was found under
- `guides/UI_LAYERING_GUIDE.md`

Contributor guide

Open the contributing guide

Research direction

Start with guides/UI_LAYERING_GUIDE.md, packages/Angular/Generic/graph-view/src/lib/components/graph-view.component.ts, and its package.json; run node packages/Standards/bin/run.js check to confirm the current violations. Trace NodeNavigated, BeforeNodeNavigate, and the NavigationService call before choosing whether a host wrapper is needed. Done includes a DOM spec, removal of the scripts/dom-test-report.mjs deferral, and a clean layering check.

Written by the indexing model from the issue text.

Assessment

Tech stack
angular, typescript
Domain
frontend, testing-qa
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.