LNK-0000: Feature/admin UI playwright e2e - #1773
Open
arianamihailescu wants to merge 16 commits into
Open
Conversation
Two-tier Playwright setup selected via projects: - mocked (default): runs against ng serve with all /api traffic served from typed fixtures; 17 tests over auth routing, dashboard, tenant list, facility create, generate-report, reports dashboard, validation config, health, and API error handling. Unmocked /api calls fail tests loudly. - live: 4 shape-based smoke tests against the docker compose stack (admin-ui :8066, BFF :8063 by default), including a self-cleaning facility roundtrip; wired into the backend-e2e-tests CI job to reuse the already-running stack, with report artifact and fail-gate. Also adds Web/ to the workflow change filter, data-testid attributes on the tenant dashboard, and Playwright output paths to .gitignore. Claude-Session: https://claude.ai/code/session_01KacgjtXmKzba8j1pkBN1Gi
The mocked suite passed while a TypeError fired on every render of the query dispatch panel. Playwright does not fail on console errors, so an Angular lifecycle throw left the page looking fine and the assertions went green. Add a console-error tripwire to the mocked fixture, mirroring the existing unmatched-/api one: collect console.error plus uncaught pageerrors, dedupe (lifecycle errors re-fire on every change detection pass), and fail the test naming them. Resource-load failures stay ignored, since api.unmatched already covers genuinely missing fixtures. Specs opt out with allowConsoleErrors, or narrow the ignore list with allowedConsoleErrors. It caught two real defects: - query-dispatch-config-form ngOnChanges dereferenced `item` outside both of its guards, throwing whenever the viewOnly input changed before the config had loaded. - LoadingIndicatorComponent raised NG0100. LoaderInterceptor calls show() synchronously as a request is issued, so a component fetching from a lifecycle hook emits mid-change-detection, after the shell's indicator has already been checked. Deferred by a tick and moved onto the async pipe. Both confirmed fixed in a browser against the real BFF, not only under mocks. Also here: - query-dispatch-create.spec.ts and its fixture, covering the create, validation and edit-mode branches of the query dispatch panel. Its endpoint paths and POST shape were verified against the live BFF. - generate-report.spec.ts read the call log synchronously right after the click, racing the POST and failing intermittently under parallel load. It polls now, the way query-dispatch-create already did. - api-errors.spec.ts opts out of the tripwire: driving the failure path is its whole purpose. Claude-Session: https://claude.ai/code/session_01CwowpQtD6yYfmiV9Zxrv54
Drives the running Admin.UI through the Playwright MCP server to explore flows unscripted, report defects with source file:line, and draft specs that follow the e2e harness conventions. Encodes the environment traps that cost real time to find: the 8066 container serves a prebuilt bundle, so verifying a source edit means ng serve against the real BFF rather than a container rebuild; config panels fetch lazily on expand; Material accordion and dialog selector shapes; and PowerShell needing -UseBasicParsing, without which Invoke-WebRequest failures masquerade as HTTP errors. Also ignore .playwright-mcp/, where the MCP server writes its snapshots, screenshots and console logs. Claude-Session: https://claude.ai/code/session_01CwowpQtD6yYfmiV9Zxrv54
Only --project=live ran in CI, inside backend-e2e-tests, so it needed the whole compose stack up and healthy before any UI test executed. The 40 mocked specs — the fast, deterministic tier, and the one carrying the console-error tripwire — never ran at all. Add admin-ui-mocked-tests: plain ubuntu-latest, no backend. Playwright boots its own ng serve and intercepts every /api call, so the tier is cheap enough to gate every pull request. Same `changes` gate as the other jobs; a job skipped by its `if` still reports as passing, so PRs that touch nothing relevant are unaffected. playwright.config.ts has emitted test-results/junit.xml under CI all along with nothing to consume it — dorny/test-reporter now renders it as a check run, and the artifact carries the HTML report plus the traces and screenshots retained on failure, so a red run reproduces locally via `npx playwright show-trace`. Reporting is continue-on-error: it needs checks:write, and a permissions problem must never turn a green suite red. The test step itself is the gate. Note action inputs are repo-relative — defaults.run.working-directory applies to `run` steps only, not to the reporter and upload paths. Claude-Session: https://claude.ai/code/session_01CwowpQtD6yYfmiV9Zxrv54
The mocked tier asserts against fixtures, so nothing in it can detect backend
drift: rename an endpoint or a field in the BFF and all 40 specs stay green
while the UI breaks. Close that gap where it belongs — in the live tier.
api-contracts.live.spec.ts covers every endpoint the mocked tier fakes, shape
only: the paged envelope and the fields each screen actually reads, the census
and query dispatch round-trips (404 -> POST -> refetch), and measure
definitions. The query dispatch case pins the exact assumption
query-dispatch-create.spec.ts mocks — the POST omits `event`, because Angular
leaves disabled controls out of form.value, and the backend defaults it to
Discharge on the way back out. If that stops being true the live tier goes red
instead of the mocked tier staying green.
Every test creates its own facility and deletes it in a finally, so there are
no seeded-data assumptions and nothing leaks into the dotnet categories that
run after it in CI.
CI builds its stack fresh, and a fresh stack has no measure definitions while
this BFF answers an empty collection with 204 and no body. The measure
definition check therefore accepts 204 and only asserts shape when a body is
present — asserting 200 outright would have passed locally and failed every
CI run.
One characterisation test records current behaviour rather than desired:
/facility/list matches facilityName only and 204s when empty, so the tenant
dashboard's Facility filter cannot find anything by facility id. When the BFF
learns to match ids that test should start failing — update it then, because
the UI filter will finally work.
Fix facility-roundtrip, which was green for the wrong reason. It searched by
facility id and accepted either a table row or an autocomplete option. It never
passed through the row: the id filter returns 204 and the table renders "No
tenants found". What it matched was an option from the *unfiltered* lookup that
ngOnInit's startWith('') fires, which renders the id in every option. So the
assertion held with search completely broken — and search is broken for ids. It
now searches by name, the only thing FacilityQueries matches, and asserts the
row, so it fails if either the search or the table breaks. Confirmed by
reverting the term to the id: the amended test fails where the old one passed.
Run the live tier with --workers=1. These specs create and delete real
facilities, and in parallel their cleanup DELETEs collide: facility-roundtrip
failed 2 of 2 full runs and passed alone every time. Serial is also faster
(11.6s vs ~26s) since the contention was never useful work. The flag lives in
the npm script so CI and local runs cannot drift apart.
Verified against the docker compose stack: mocked 40/40, live 11/11, no test
facilities left behind.
Claude-Session: https://claude.ai/code/session_01CwowpQtD6yYfmiV9Zxrv54
The Playwright MCP server writes an accessibility-tree snapshot per browser action into .playwright-mcp/. Twenty-four of them were committed by accident, adding 6,551 lines — roughly seventy percent of this branch's diff. They have no value in the repository: no spec imports them, their [ref=...] handles are per-session identifiers that mean nothing in any other run, and they describe a build that predates the fixes on this branch. They are the same category as playwright-report/ and test-results/, already ignored. .playwright-mcp/ is now in .gitignore, but that only stops new files; these were already tracked. Removed from the index only, so they stay on disk. Claude-Session: https://claude.ai/code/session_01CwowpQtD6yYfmiV9Zxrv54
Contributor
📝 WalkthroughWalkthroughAdds a Playwright-based Admin UI E2E framework with mocked, live, and demo tiers, shared fixtures and page objects, CI jobs, and API contract coverage. It also adds stable UI selectors and updates loading-indicator and query-dispatch component state handling. ChangesAdmin UI E2E coverage
Angular runtime fixes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Playwright
participant AdminUI
participant AdminBFF
participant Database
Playwright->>AdminUI: Navigate to live route
AdminUI->>AdminBFF: Request UI API data
AdminBFF->>Database: Read or persist facility data
Database-->>AdminBFF: Return response data
AdminBFF-->>AdminUI: Return API response
AdminUI-->>Playwright: Render page and status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
arianamihailescu
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
.github/workflows/tests.yaml (1)
361-362: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable persisted checkout credentials for the test job.
npm ciexecutes dependency lifecycle scripts after checkout. The default checkout configuration leaves the job token in local Git config; this job does not need Git authentication after checkout.Proposed fix
- name: Checkout code uses: actions/checkout@v4 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/tests.yaml around lines 361 - 362, Update the test job’s actions/checkout@v4 step to disable persisted credentials by setting persist-credentials to false, while leaving the checkout behavior otherwise unchanged.Source: Linters/SAST tools
Web/Admin.UI/src/app/components/core/loading-indicator/loading-indicator.component.ts (1)
12-30: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd focused XUnit coverage for deferred loading emissions.
delay(0)changes the observable timing contract. Add a small test with a mockedLoadingService.isLoadingthat verifies true/false emissions update the template throughAsyncPipe; keep it network-free. The reported Playwright suites do not directly cover this component behavior.As per path instructions, keep the unit test small, XUnit-based, and free of network activity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Admin.UI/src/app/components/core/loading-indicator/loading-indicator.component.ts` around lines 12 - 30, Add a focused XUnit unit test for LoadingIndicatorComponent that mocks LoadingService.isLoading and verifies true and false emissions update the rendered template through AsyncPipe after the deferred delay(0) emission. Keep the test small, isolated, and network-free, using the existing component test conventions.Source: Path instructions
Web/Admin.UI/src/app/components/query-dispatch/query-dispatch-config-form/query-dispatch-config-form.component.ts (2)
98-100: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the pre-
itemlifecycle ordering with a focused XUnit test.Test the case where
viewOnlychanges beforeitem, then verify that a lateritemchange populates the schedules and facility ID without throwing. Keep the test isolated from network activity.As per path instructions, add focused XUnit coverage for this input-ordering behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Admin.UI/src/app/components/query-dispatch/query-dispatch-config-form/query-dispatch-config-form.component.ts` around lines 98 - 100, Add focused XUnit coverage for the component’s input-ordering behavior around setDispatchSchedules: change viewOnly before item, then assign item and verify schedules and facility ID populate without throwing. Keep the test isolated from network activity and reuse existing component/test setup utilities.Source: Path instructions
98-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the
iteminput type with its runtime contract.This code now explicitly handles
itembeing undefined, but Line 38 still uses@Input() item!, which suppresses strict-null checking. Make the input optional and retain the guards so future accesses cannot reintroduce the pre-item crash.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Admin.UI/src/app/components/query-dispatch/query-dispatch-config-form/query-dispatch-config-form.component.ts` around lines 98 - 100, Update the item input declaration in QueryDispatchConfigFormComponent to be optional instead of using the non-null assertion, matching the existing item?.dispatchSchedules guard and setDispatchSchedules fallback. Preserve the guards for all accesses so sibling input changes remain safe before item is initialized.Web/Admin.UI/e2e/support/api-mock.ts (1)
37-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd focused branch coverage for the shared E2E harness.
Web/Admin.UI/e2e/support/api-mock.ts#L37-L50: cover exact, wildcard, function, status, JSON, and unmatched handler dispatch.Web/Admin.UI/e2e/support/api-mock.ts#L67-L75: cover exact versus prefix handler resolution.Web/Admin.UI/e2e/support/live.ts#L10-L53: cover URL modes, reachable/unreachable backend responses, and API failure collection.Web/Admin.UI/e2e/support/test.ts#L45-L87: cover allowlists, opt-outs, and unmatched-call failure behavior.Keep tests small and mocked; provide the required XUnit coverage or document the applicable equivalent test location. As per path instructions, “If/Else or Switch/Case blocks are introduced or modified — ensure each branch has a corresponding unit test,” and unit tests must not make network calls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Admin.UI/e2e/support/api-mock.ts` around lines 37 - 50, Add focused, fully mocked unit coverage for the shared E2E harness: in Web/Admin.UI/e2e/support/api-mock.ts lines 37-50, cover exact, wildcard, function, status, JSON, and unmatched dispatch; in lines 67-75, cover exact versus prefix resolution; in Web/Admin.UI/e2e/support/live.ts lines 10-53, cover URL modes, reachable and unreachable backends, and API failure collection; and in Web/Admin.UI/e2e/support/test.ts lines 45-87, cover allowlists, opt-outs, and unmatched-call failures. Provide the required XUnit coverage or document the applicable equivalent test location, and ensure tests make no network calls.Source: Path instructions
Web/Admin.UI/src/app/components/tenant/tenant-dashboard/tenant-dashboard.component.html (1)
7-12: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueMark the create action as non-submit.
This button opens a dialog; add
type="button"so it cannot submit an enclosing form.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Admin.UI/src/app/components/tenant/tenant-dashboard/tenant-dashboard.component.html` around lines 7 - 12, Update the create-action button invoking showCreateFacilityDialog() to include type="button", preventing it from submitting any enclosing form while preserving its existing dialog behavior.Source: Learnings
Web/Admin.UI/e2e/live/smoke.live.spec.ts (1)
35-41: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHealth monitor smoke test doesn't check for API failures, unlike its siblings.
Both other tests in this file assert
expect(failures).toEqual([])viawatchApiFailures. This test only checks visual rendering, so a failing/monitorAPI call wouldn't be caught here even though the file's stated goal is "real API responds 2xx."✅ Suggested addition
test('health monitor renders real service statuses', async ({ page }) => { + const failures = watchApiFailures(page); + await page.goto('/monitor/health'); await expect(page.locator('mat-toolbar', { hasText: 'Service Health Status' })).toBeVisible({ timeout: 20_000 }); // At least one service should report in a running stack. await expect(page.locator('table tr[mat-row]').first()).toBeVisible({ timeout: 20_000 }); + expect(failures).toEqual([]); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Admin.UI/e2e/live/smoke.live.spec.ts` around lines 35 - 41, Update the health monitor smoke test around “health monitor renders real service statuses” to use the existing watchApiFailures helper and assert that the collected failures equal an empty array. Start monitoring before navigating to /monitor/health, retain the existing rendering assertions, and verify no API failures after the page has loaded.Web/Admin.UI/e2e/live/api-contracts.live.spec.ts (1)
56-61: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCleanup assertions inside
finallyblocks can swallow the real test failure — repeats across both live spec files. Every facility/resource cleanup in these files putsexpect([...]).toContain(del.status())directly inside afinallythat follows atrywhich can itself throw; if both throw, JS discards the original exception and only the cleanup failure surfaces, hiding the real root cause.
Web/Admin.UI/e2e/live/api-contracts.live.spec.ts#L56-L61: inwithFacility, replace the inlineexpectondel.status()with a non-throwing check (e.g.console.warnon unexpected status, or.catch()the delete call) so cleanup never masksbody(facilityId)failures.Web/Admin.UI/e2e/live/api-contracts.live.spec.ts#L154-L157: apply the same non-throwing cleanup pattern to the query-dispatch configuration delete.Web/Admin.UI/e2e/live/api-contracts.live.spec.ts#L182-L185: apply the same non-throwing cleanup pattern to the census configuration delete.Web/Admin.UI/e2e/live/facility-roundtrip.live.spec.ts#L55-L59: apply the same non-throwing cleanup pattern to the facility delete in this spec'sfinally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Admin.UI/e2e/live/api-contracts.live.spec.ts` around lines 56 - 61, The cleanup assertions in finally blocks can mask the original test failure; update withFacility in Web/Admin.UI/e2e/live/api-contracts.live.spec.ts at lines 56-61, the query-dispatch cleanup at lines 154-157, the census cleanup at lines 182-185, and the facility cleanup in Web/Admin.UI/e2e/live/facility-roundtrip.live.spec.ts at lines 55-59 to use non-throwing delete handling, such as warning on unexpected statuses or catching delete errors, so cleanup never replaces body or test failures.Web/Admin.UI/e2e/demo/facility-walkthrough.demo.spec.ts (1)
40-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover both environment-controlled branches.
Extract the pause/keep-open decisions into a testable seam and add focused XUnit coverage for pause vs. skip and keep-open vs. close behavior. Keep external browser interactions mocked. As per path instructions, “If/Else or Switch/Case blocks are introduced or modified — ensure each branch has a corresponding unit test,” and “No network activity … should appear in unit tests.”
Also applies to: 166-169
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Admin.UI/e2e/demo/facility-walkthrough.demo.spec.ts` around lines 40 - 47, Refactor the environment-controlled pause and keep-open decisions around stage and the related teardown flow into testable helper seams, preserving their current behavior. Add focused XUnit tests covering both pause versus skip and keep-open versus close branches, using mocked browser interactions and ensuring the tests perform no network activity.Source: Path instructions
Web/Admin.UI/e2e/mocked/generate-report.spec.ts (1)
43-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen date payload assertions.
toBeTruthy()only confirms the fields are non-empty, not that they reflect the entered Custom range (6/1/2026–6/30/2026). A regression that submits the wrong dates would still pass.♻️ Proposed stronger assertion
- expect(payload.startDate).toBeTruthy(); - expect(payload.endDate).toBeTruthy(); + expect(payload.startDate).toContain('2026-06-01'); + expect(payload.endDate).toContain('2026-06-30');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Admin.UI/e2e/mocked/generate-report.spec.ts` around lines 43 - 46, Strengthen the payload assertions in the custom-range test by verifying payload.startDate and payload.endDate equal the expected values for 6/1/2026 through 6/30/2026, rather than only checking truthiness. Keep the existing reportTypes and bypassSubmission assertions unchanged.Web/Admin.UI/e2e/mocked/facility-edit.spec.ts (1)
21-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
mockFacilityEdithelper across two mocked specs. Both files re-implement the same baseline mock (facility, measure-definition, normalization-operations) for the facility-edit screen; the copies have already started to drift (only one takes an optionalconfigoverride).
Web/Admin.UI/e2e/mocked/facility-edit.spec.ts#L21-L25: keep this parameterized version and move it into a shared support module (e.g.e2e/support/mocks/facility-edit.ts) exportingmockFacilityEdit(api, config?).Web/Admin.UI/e2e/mocked/query-dispatch-create.spec.ts#L17-L22: delete this local copy and import the shared helper instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Admin.UI/e2e/mocked/facility-edit.spec.ts` around lines 21 - 25, The duplicated mockFacilityEdit helper should be centralized while preserving its optional config override. In Web/Admin.UI/e2e/mocked/facility-edit.spec.ts:21-25, move the parameterized mockFacilityEdit implementation into a shared support module and export it; in Web/Admin.UI/e2e/mocked/query-dispatch-create.spec.ts:17-22, remove the local copy and import the shared helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/admin-ui-walkthrough/SKILL.md:
- Line 34: Update the Markdown command fences to include language identifiers:
mark the MCP tool list, tool-call examples, and Grep example fences in
.claude/skills/admin-ui-walkthrough/SKILL.md at lines 34, 53, and 86 as text,
and mark the npm command fence in Web/Admin.UI/e2e/README.md at line 12 as
powershell or bash.
In `@Web/Admin.UI/e2e/fixtures/facility-detail.ts`:
- Around line 44-50: Update pagedLocationMappings and the corresponding paging
fixture helpers at Web/Admin.UI/e2e/fixtures/facility-detail.ts lines 76-82 and
89-93, plus the helper at Web/Admin.UI/e2e/fixtures/report-summaries.ts lines
34-38, to calculate totalPages from records.length using the existing page size
of 10 instead of always returning 1; keep the remaining metadata unchanged.
In `@Web/Admin.UI/e2e/live/facility-roundtrip.live.spec.ts`:
- Around line 61-63: Update the deletion verification after the facility delete
request to poll GET requests for facilityId until the resource is absent,
matching the existing creation polling pattern. Assert success only after the
response is no longer 200, while preserving the current failure message and
bounded retry behavior.
In `@Web/Admin.UI/e2e/support/live.ts`:
- Around line 10-16: Normalize the UI_E2E_API_URL value in apiBaseUrl before
returning it, removing trailing slash characters so appended paths produce a
single separator. Preserve the existing environment-variable precedence and
fallback behavior.
In
`@Web/Admin.UI/src/app/components/tenant/facility-view/facility-view.component.html`:
- Around line 6-7: In the facility-view template, fix the markup around the
facility-view-title paragraph by closing the existing title <p> element instead
of opening a second paragraph. Preserve the facilityConfig?.facilityName binding
and surrounding structure.
---
Nitpick comments:
In @.github/workflows/tests.yaml:
- Around line 361-362: Update the test job’s actions/checkout@v4 step to disable
persisted credentials by setting persist-credentials to false, while leaving the
checkout behavior otherwise unchanged.
In `@Web/Admin.UI/e2e/demo/facility-walkthrough.demo.spec.ts`:
- Around line 40-47: Refactor the environment-controlled pause and keep-open
decisions around stage and the related teardown flow into testable helper seams,
preserving their current behavior. Add focused XUnit tests covering both pause
versus skip and keep-open versus close branches, using mocked browser
interactions and ensuring the tests perform no network activity.
In `@Web/Admin.UI/e2e/live/api-contracts.live.spec.ts`:
- Around line 56-61: The cleanup assertions in finally blocks can mask the
original test failure; update withFacility in
Web/Admin.UI/e2e/live/api-contracts.live.spec.ts at lines 56-61, the
query-dispatch cleanup at lines 154-157, the census cleanup at lines 182-185,
and the facility cleanup in
Web/Admin.UI/e2e/live/facility-roundtrip.live.spec.ts at lines 55-59 to use
non-throwing delete handling, such as warning on unexpected statuses or catching
delete errors, so cleanup never replaces body or test failures.
In `@Web/Admin.UI/e2e/live/smoke.live.spec.ts`:
- Around line 35-41: Update the health monitor smoke test around “health monitor
renders real service statuses” to use the existing watchApiFailures helper and
assert that the collected failures equal an empty array. Start monitoring before
navigating to /monitor/health, retain the existing rendering assertions, and
verify no API failures after the page has loaded.
In `@Web/Admin.UI/e2e/mocked/facility-edit.spec.ts`:
- Around line 21-25: The duplicated mockFacilityEdit helper should be
centralized while preserving its optional config override. In
Web/Admin.UI/e2e/mocked/facility-edit.spec.ts:21-25, move the parameterized
mockFacilityEdit implementation into a shared support module and export it; in
Web/Admin.UI/e2e/mocked/query-dispatch-create.spec.ts:17-22, remove the local
copy and import the shared helper.
In `@Web/Admin.UI/e2e/mocked/generate-report.spec.ts`:
- Around line 43-46: Strengthen the payload assertions in the custom-range test
by verifying payload.startDate and payload.endDate equal the expected values for
6/1/2026 through 6/30/2026, rather than only checking truthiness. Keep the
existing reportTypes and bypassSubmission assertions unchanged.
In `@Web/Admin.UI/e2e/support/api-mock.ts`:
- Around line 37-50: Add focused, fully mocked unit coverage for the shared E2E
harness: in Web/Admin.UI/e2e/support/api-mock.ts lines 37-50, cover exact,
wildcard, function, status, JSON, and unmatched dispatch; in lines 67-75, cover
exact versus prefix resolution; in Web/Admin.UI/e2e/support/live.ts lines 10-53,
cover URL modes, reachable and unreachable backends, and API failure collection;
and in Web/Admin.UI/e2e/support/test.ts lines 45-87, cover allowlists, opt-outs,
and unmatched-call failures. Provide the required XUnit coverage or document the
applicable equivalent test location, and ensure tests make no network calls.
In
`@Web/Admin.UI/src/app/components/core/loading-indicator/loading-indicator.component.ts`:
- Around line 12-30: Add a focused XUnit unit test for LoadingIndicatorComponent
that mocks LoadingService.isLoading and verifies true and false emissions update
the rendered template through AsyncPipe after the deferred delay(0) emission.
Keep the test small, isolated, and network-free, using the existing component
test conventions.
In
`@Web/Admin.UI/src/app/components/query-dispatch/query-dispatch-config-form/query-dispatch-config-form.component.ts`:
- Around line 98-100: Add focused XUnit coverage for the component’s
input-ordering behavior around setDispatchSchedules: change viewOnly before
item, then assign item and verify schedules and facility ID populate without
throwing. Keep the test isolated from network activity and reuse existing
component/test setup utilities.
- Around line 98-100: Update the item input declaration in
QueryDispatchConfigFormComponent to be optional instead of using the non-null
assertion, matching the existing item?.dispatchSchedules guard and
setDispatchSchedules fallback. Preserve the guards for all accesses so sibling
input changes remain safe before item is initialized.
In
`@Web/Admin.UI/src/app/components/tenant/tenant-dashboard/tenant-dashboard.component.html`:
- Around line 7-12: Update the create-action button invoking
showCreateFacilityDialog() to include type="button", preventing it from
submitting any enclosing form while preserving its existing dialog behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 541f4c5b-8317-474e-85ca-01f41e166e26
⛔ Files ignored due to path filters (1)
Web/Admin.UI/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (42)
.claude/skills/admin-ui-walkthrough/SKILL.md.github/workflows/tests.yaml.gitignoreWeb/Admin.UI/e2e/README.mdWeb/Admin.UI/e2e/demo/facility-walkthrough.demo.spec.tsWeb/Admin.UI/e2e/fixtures/facilities.tsWeb/Admin.UI/e2e/fixtures/facility-detail.tsWeb/Admin.UI/e2e/fixtures/health.tsWeb/Admin.UI/e2e/fixtures/measure-defs.tsWeb/Admin.UI/e2e/fixtures/report-summaries.tsWeb/Admin.UI/e2e/live/api-contracts.live.spec.tsWeb/Admin.UI/e2e/live/facility-roundtrip.live.spec.tsWeb/Admin.UI/e2e/live/smoke.live.spec.tsWeb/Admin.UI/e2e/mocked/api-errors.spec.tsWeb/Admin.UI/e2e/mocked/auth-routing.spec.tsWeb/Admin.UI/e2e/mocked/dashboard.spec.tsWeb/Admin.UI/e2e/mocked/facility-create.spec.tsWeb/Admin.UI/e2e/mocked/facility-edit.spec.tsWeb/Admin.UI/e2e/mocked/facility-view.spec.tsWeb/Admin.UI/e2e/mocked/generate-report.spec.tsWeb/Admin.UI/e2e/mocked/health.spec.tsWeb/Admin.UI/e2e/mocked/query-dispatch-create.spec.tsWeb/Admin.UI/e2e/mocked/reports-dashboard.spec.tsWeb/Admin.UI/e2e/mocked/tenant-list.spec.tsWeb/Admin.UI/e2e/mocked/validation-config.spec.tsWeb/Admin.UI/e2e/support/api-mock.tsWeb/Admin.UI/e2e/support/app-config.tsWeb/Admin.UI/e2e/support/live.tsWeb/Admin.UI/e2e/support/pages/facility-edit.page.tsWeb/Admin.UI/e2e/support/pages/facility-view.page.tsWeb/Admin.UI/e2e/support/pages/nav.page.tsWeb/Admin.UI/e2e/support/pages/tenant-dashboard.page.tsWeb/Admin.UI/e2e/support/test.tsWeb/Admin.UI/e2e/tools/capture-fixtures.mdWeb/Admin.UI/package.jsonWeb/Admin.UI/playwright.config.tsWeb/Admin.UI/src/app/components/core/loading-indicator/loading-indicator.component.htmlWeb/Admin.UI/src/app/components/core/loading-indicator/loading-indicator.component.tsWeb/Admin.UI/src/app/components/query-dispatch/query-dispatch-config-form/query-dispatch-config-form.component.tsWeb/Admin.UI/src/app/components/tenant/facility-edit/facility-edit.component.htmlWeb/Admin.UI/src/app/components/tenant/facility-view/facility-view.component.htmlWeb/Admin.UI/src/app/components/tenant/tenant-dashboard/tenant-dashboard.component.html
facility-roundtrip verified deletion with a single immediate GET while the creation check right above it polls, because creation is not immediately readable on a busy stack. Deletion is no different: poll for absence the same way, so a delete still in flight cannot fail a test that is actually fine. The paging fixtures returned totalPages: 1 regardless of how many records they were handed, so any spec supplying more than a page of data got metadata that contradicted itself. Derive it, matching the form facilities.ts already used correctly. apiBaseUrl returned UI_E2E_API_URL verbatim; every caller appends a path, so a value ending in a slash produced .../api//info, which not every reverse proxy normalises. Trim it. facility-view.component.html opened a second <p> instead of closing the title paragraph. The testid the page object and several specs assert on lives on that element, so the mocked tier was re-run to confirm the DOM change is inert. Label the markdown code fences in SKILL.md and the e2e README for the linter. Verified: typecheck clean, mocked 40/40, live 11/11 against the compose stack, no test facilities left behind. Claude-Session: https://claude.ai/code/session_01CwowpQtD6yYfmiV9Zxrv54
Contributor
Author
|
@coderabbitai resolve |
Contributor
✅ Action performedComments resolved. Approval is disabled; enable |
The walkthrough stopped at the facility edit screen. Carry it through the rest of the journey: create a facility from the tenant dashboard, find it in the refreshed list, open it, and give it a census configuration and a query dispatch schedule. That needs mocks with state rather than static fixtures. The facility list, the lookup behind the autocomplete and GET /api/facility/:id now read from one mutable array, so the facility created mid-walkthrough appears in the table and can then be opened — a static fixture would send the app to a facility the list never gained. Census and query dispatch each keep a per-facility store, so the seeded facility answers 200 while the new one 404s, and that 404 is exactly what makes each panel offer to create a configuration. The query dispatch store defaults the event to Discharge on write, because the control is disabled and Angular leaves disabled controls out of form.value — mirroring what the backend does, as confirmed against the live BFF. Verified with E2E_KEEP_OPEN=0 so it runs straight through instead of holding the browser open: passes in 15s with pacing disabled. Claude-Session: https://claude.ai/code/session_01CwowpQtD6yYfmiV9Zxrv54
The job shared the `changes` filter with the backend suites, so a DotNet- or Java-only pull request spent two and a half minutes running Playwright specs that stub every /api call and therefore cannot be affected by a backend change. Add a second output to the `changes` job — GitHub has no per-job paths filter, only workflow-level — computed from Web/Admin.UI/ plus this workflow file. The workflow counts deliberately: a change to it should exercise the job it defines. Only the mocked tier narrows. The live tier keeps the broad gate, because it runs against a real stack where backend changes genuinely can break it. Skipped jobs report a skipped conclusion, which required status checks treat as passing, so a backend-only pull request is not left waiting on a check that never runs. Verified: the workflow parses and all six jobs resolve, and the two predicates were exercised against nine change sets — Admin.UI sources, this workflow, DotNet-only, Java-only, another app under Web/, docker-compose, docs-only and a mixed DotNet + Admin.UI set all classify as intended. Claude-Session: https://claude.ai/code/session_01CwowpQtD6yYfmiV9Zxrv54
The tenant dashboard's single search box fed two endpoints with different matching rules: the autocomplete matched a substring while the paged table matched the name exactly. Typing part of a name listed the facility in the dropdown while the table underneath reported "No tenants found", and typing a facility ID matched nothing at all in either, even though the dropdown displays IDs beneath each name. Rename FacilityNameContains to PartialMatch and widen it to match the term against both FacilityName and FacilityId, then opt the paged endpoint in alongside the autocomplete. Exact-name lookups keep their old behaviour, which GetAsync's SingleOrDefault relies on. This stayed hidden because nearly every seeded facility uses the same value for its ID and its name, making the two searches indistinguishable. Verified against the live stack: partial name "MyFacil" and the ID-only fragment "demo-tenant" both return their facility, and the two endpoints now agree. Claude-Session: https://claude.ai/code/session_01FcrHDu3n6F5w9823Taw9DG
The reports dashboard had two tests that only checked rows rendered. Both screens filter, sort and page server-side, so almost all of their behaviour lives in the request they send rather than in the DOM. Assert on that request: the reports dashboard debounces typing into one call, appends a repeated status parameter per selection rather than joining them, widens the reporting-period To date to the end of that local day, and resets to page one when the sort column changes. The audit dashboard submits on Enter instead of debouncing, sends the facility id while displaying its name, and keeps its filters across a refresh. Two tests characterise behaviour rather than endorse it: audit never sends sortBy, because searchLogs drops the parameter getAuditLogs passes it, and a report whose delete returns 409 gets its own "Report In Progress" dialog. Both should start failing if that changes. Add page objects for each screen so the selector workarounds have somewhere to live -- the Material paginator's page-size select sits under a touch target that swallows clicks, and the audit notes panel is a sibling row that would otherwise inflate the row count. Give the reports table, header and empty state test ids, and give the delete and restore icon buttons aria-labels. Those two buttons had only a matTooltip, so they had no accessible name for a screen reader. 64 mocked tests pass. Claude-Session: https://claude.ai/code/session_01STFKxmDaJenKVny6WUoWKo
The previous commit taught /facility/list to match facility ids as well as names, which made this characterisation test fail exactly as its own comment said it should: it asserted that an id search finds nothing and 204s. Assert the current contract instead -- searching a name fragment and searching an id both return 200, and both return a map containing the facility. The 204-on-empty case moves into its own test rather than disappearing; the old test only covered it as a side effect of the id search never matching, so folding the two together would have silently dropped it. Also correct a comment in facility-roundtrip that claimed typing an id filters everything out. That test still passes -- it searches by name -- but the explanation next to it was no longer true. Claude-Session: https://claude.ai/code/session_01STFKxmDaJenKVny6WUoWKo
arianamihailescu
added a commit
that referenced
this pull request
Aug 4, 2026
* LEGLINK-620: record the vendor signing-key UI design Captures the decisions behind associating a Key Vault secret with a vendor, so the reasoning survives outside a chat log: why the association is vendor-scoped rather than facility-scoped (Veradigm needs a different key generation algorithm than Epic and Cerner), and why the stored value is the Key Vault secret id rather than a JWKS kid, which LEGLINK-63's title conflates. Also records what this ticket deliberately cannot do. The update endpoint that persists a secret id is owned by neither LEGLINK-620 nor LEGLINK-743, so the UI is built against a single isolated service method. LEGLINK-63's audit-trail acceptance criterion is produced from backend managers onto Kafka and no UI change can satisfy it. Claude-Session: https://claude.ai/code/session_01STFKxmDaJenKVny6WUoWKo * LEGLINK-620: associate a Key Vault secret with a vendor in the UI Link signs the JWT that Data Acquisition presents to Epic and Cerner during client-credentials auth. LEGLINK-14 moved the PEM into Key Vault; this makes the association explicit and vendor-scoped, because Veradigm needs a different key generation algorithm and so cannot share a key. Add an edit path to Vendor Management. The dashboard grows a Secret ID column that reads "Not set" when empty and a per-row edit action; the form grows a JWT / Authentication panel holding the Key Vault Secret ID, expanded when a vendor already has one so existing configuration is visible without hunting. An emptied box travels as undefined rather than "", so clearing the association reads as absent rather than set-to-empty. No update endpoint exists yet. The Vendor model is moving out of Normalization into Tenant under LEGLINK-743, whose acceptance criteria cover list, add and delete but not update, so this operation is owned by neither ticket. VendorService.updateVendor is the single place that knows the route: when the contract lands, that method is the only edit required. A config-flagged dual path was considered and rejected as permanent complexity bought against a decision expected within days. Two defects in files this already touches: createVendor was typed as IVendorConfigModel while callers pass a name string, which interpolated "[object Object]" into the URL for anything else; and getVendors never cleared its loading flag on success. Deferred: the mocked Playwright spec the design calls for. That harness arrives with PR #1773, which is not yet merged into dev, so there is nowhere on this branch for the spec to live. LEGLINK-63's audit-trail criterion is also outstanding -- audit events are produced from backend managers onto Kafka, so it belongs with the update endpoint rather than here. 16 unit specs pass; the app builds clean. Claude-Session: https://claude.ai/code/session_01STFKxmDaJenKVny6WUoWKo * LEGLINK-620: report a failed vendor save once, and clear a key explicitly Two defects found reviewing 4b894e1 against the design. ErrorHandlingService raises its own toastr before rethrowing, so once the save paths began emitting failure to the dialog -- which shows a snackbar and stays open so the admin's input survives -- one failed save reported itself twice, toastr bottom-full-width and snackbar top-right. Saves now route through handleSaveError, which suppresses the toastr and leaves the dialog as the single surface. List and delete keep theirs, having no dialog to carry the news. The rethrown error still carries the sanitized message either way. Clearing a key sent secretId as undefined, which JSON.stringify drops, so the field never reached the wire. An absent field reads as "leave unchanged" to any endpoint with partial-update semantics, which would have made clearing an association succeed visibly and do nothing. It now travels as an explicit null, and the design's open items record that the backend must honour null as "remove the association" when the contract is settled. Adds vendor.service.spec.ts, the service having had no direct coverage: the update route and body, a cleared key surviving serialization, name escaping in the create route, and the toastr suppressed for saves but kept for list and delete. Both new behaviours fail against the previous code -- args[1] was absent rather than false, and secretId was undefined rather than null. 21 unit specs pass; the app builds clean. Claude-Session: https://claude.ai/code/session_01CX9BzMrPhTzGSakXYDaAVa * LEGLINK-620: cover the vendor create failure branch A failed createVendor had no test. Assert it emits a single failure with the error message and does not fall through into the update path. Claude-Session: https://claude.ai/code/session_01TDPYGwkQDCXConLTuyU4is * LEGLINK-620: gate vendor editing until an update endpoint exists VendorController exposes list, add and delete only -- no PUT -- so the edit dialog added on this branch would save into a 404. Put the edit button and onEdit behind a vendorEditEnabled config flag, shipped off, following the existing AppConfig boolean pattern. Flip it once the update contract, including clearing secretId with null, is confirmed. Claude-Session: https://claude.ai/code/session_01TDPYGwkQDCXConLTuyU4is
arianamihailescu
added a commit
that referenced
this pull request
Aug 5, 2026
* LEGLINK-620: record the vendor signing-key UI design Captures the decisions behind associating a Key Vault secret with a vendor, so the reasoning survives outside a chat log: why the association is vendor-scoped rather than facility-scoped (Veradigm needs a different key generation algorithm than Epic and Cerner), and why the stored value is the Key Vault secret id rather than a JWKS kid, which LEGLINK-63's title conflates. Also records what this ticket deliberately cannot do. The update endpoint that persists a secret id is owned by neither LEGLINK-620 nor LEGLINK-743, so the UI is built against a single isolated service method. LEGLINK-63's audit-trail acceptance criterion is produced from backend managers onto Kafka and no UI change can satisfy it. Claude-Session: https://claude.ai/code/session_01STFKxmDaJenKVny6WUoWKo * LEGLINK-620: associate a Key Vault secret with a vendor in the UI Link signs the JWT that Data Acquisition presents to Epic and Cerner during client-credentials auth. LEGLINK-14 moved the PEM into Key Vault; this makes the association explicit and vendor-scoped, because Veradigm needs a different key generation algorithm and so cannot share a key. Add an edit path to Vendor Management. The dashboard grows a Secret ID column that reads "Not set" when empty and a per-row edit action; the form grows a JWT / Authentication panel holding the Key Vault Secret ID, expanded when a vendor already has one so existing configuration is visible without hunting. An emptied box travels as undefined rather than "", so clearing the association reads as absent rather than set-to-empty. No update endpoint exists yet. The Vendor model is moving out of Normalization into Tenant under LEGLINK-743, whose acceptance criteria cover list, add and delete but not update, so this operation is owned by neither ticket. VendorService.updateVendor is the single place that knows the route: when the contract lands, that method is the only edit required. A config-flagged dual path was considered and rejected as permanent complexity bought against a decision expected within days. Two defects in files this already touches: createVendor was typed as IVendorConfigModel while callers pass a name string, which interpolated "[object Object]" into the URL for anything else; and getVendors never cleared its loading flag on success. Deferred: the mocked Playwright spec the design calls for. That harness arrives with PR #1773, which is not yet merged into dev, so there is nowhere on this branch for the spec to live. LEGLINK-63's audit-trail criterion is also outstanding -- audit events are produced from backend managers onto Kafka, so it belongs with the update endpoint rather than here. 16 unit specs pass; the app builds clean. Claude-Session: https://claude.ai/code/session_01STFKxmDaJenKVny6WUoWKo * LEGLINK-620: report a failed vendor save once, and clear a key explicitly Two defects found reviewing 4b894e1 against the design. ErrorHandlingService raises its own toastr before rethrowing, so once the save paths began emitting failure to the dialog -- which shows a snackbar and stays open so the admin's input survives -- one failed save reported itself twice, toastr bottom-full-width and snackbar top-right. Saves now route through handleSaveError, which suppresses the toastr and leaves the dialog as the single surface. List and delete keep theirs, having no dialog to carry the news. The rethrown error still carries the sanitized message either way. Clearing a key sent secretId as undefined, which JSON.stringify drops, so the field never reached the wire. An absent field reads as "leave unchanged" to any endpoint with partial-update semantics, which would have made clearing an association succeed visibly and do nothing. It now travels as an explicit null, and the design's open items record that the backend must honour null as "remove the association" when the contract is settled. Adds vendor.service.spec.ts, the service having had no direct coverage: the update route and body, a cleared key surviving serialization, name escaping in the create route, and the toastr suppressed for saves but kept for list and delete. Both new behaviours fail against the previous code -- args[1] was absent rather than false, and secretId was undefined rather than null. 21 unit specs pass; the app builds clean. Claude-Session: https://claude.ai/code/session_01CX9BzMrPhTzGSakXYDaAVa * LEGLINK-620: cover the vendor create failure branch A failed createVendor had no test. Assert it emits a single failure with the error message and does not fall through into the update path. Claude-Session: https://claude.ai/code/session_01TDPYGwkQDCXConLTuyU4is * LEGLINK-620: gate vendor editing until an update endpoint exists VendorController exposes list, add and delete only -- no PUT -- so the edit dialog added on this branch would save into a 404. Put the edit button and onEdit behind a vendorEditEnabled config flag, shipped off, following the existing AppConfig boolean pattern. Flip it once the update contract, including clearing secretId with null, is confirmed. Claude-Session: https://claude.ai/code/session_01TDPYGwkQDCXConLTuyU4is * LEGLINK-566: design for validating a vendor secret id against Key Vault Validation lives on Admin.BFF, which already holds an ISecretManager and is independent of the Vendor model's move to Tenant. Adds ISecretInspector and PemSigningKeyValidator to Shared; the UI warns inline on blur and on save without ever blocking the save. Claude-Session: https://claude.ai/code/session_01TDPYGwkQDCXConLTuyU4is * LEGLINK-566: implementation plan for vendor secret id validation Eight tasks, TDD throughout: characterize EpicAuth's PKCS#8 behavior, then PemSigningKeyValidator and ISecretInspector in Shared, the Admin.BFF endpoint, the Angular service call, and the form's blur/save warnings. Claude-Session: https://claude.ai/code/session_01TDPYGwkQDCXConLTuyU4is * LEGLINK-620: store a vendor's Key Vault signing key secret id Vendor moved into Tenant under LEGLINK-743 carrying only Id and Name, so there was nowhere to record the Key Vault secret holding a vendor's PEM signing key. LEGLINK-63 scopes that key to the vendor rather than the facility, because the key generation algorithm differs by EHR. Stored as a JSON column rather than a plain one so later vendor-level auth settings need no migration, following the AuthenticationConfiguration precedent in DataAcquisitionDbContext. Only the signing key lives here: TokenUrl, Audience and ClientId are per-EHR-instance and stay on the facility's authentication configuration, where EpicAuth already reads them. The value converter carries an explicit ValueComparer. Without one EF snapshots the property by reference, so mutating a field on the existing instance is never detected and SaveChanges writes nothing -- a test covers that specifically. Update treats a missing authentication object as "leave unchanged", matching how Name already behaves, so a caller that omits it cannot wipe a configured key. Clearing a key means sending the object with a null inside it. Vendor versions expose the parent vendor's settings as a read-only projection, so consumers holding only a vendor version id -- as Data Acquisition will -- can resolve the key in one call while writes stay on the vendor. EpicAuth is untouched: it still derives {facilityId}-pem, so nothing changes at runtime until the fallback rule between vendor and facility keys is settled. Claude-Session: https://claude.ai/code/session_01QoUHyt1ALkruCbuYxHSXiB * LEGLINK-620: point the vendor screens at the Tenant API LEGLINK-743 deleted Normalization's VendorController when it moved the model to Tenant, but left this service calling the old routes. Every vendor operation in Admin.UI has been hitting a controller that no longer exists. The API nests the signing key under authentication; the vendor screens work with a flat secretId. Translating at the gateway keeps that difference out of the components, so a second vendor-level setting only touches this file. Create now carries the secret id. The add form has always shown the field, but the create branch sent the name alone, so anything typed there was silently discarded behind a success message. Both write paths build the same payload before branching, which is what stops them drifting apart again. authentication is always sent, including when the key is being cleared: the Tenant manager reads an absent object as "leave unchanged", so omitting it would make a clear no-op. Null inside the object is what removes the association. The vendorEditEnabled flag is gone with it. It existed only to keep the edit button hidden while no update endpoint existed, and PUT /api/vendor/{id} now does, so the gate has nothing left to protect. Claude-Session: https://claude.ai/code/session_01QoUHyt1ALkruCbuYxHSXiB * LEGLINK-620: drop the superpowers design and plan docs The vendor signing-key design and the secret-id validation design and plan were working notes for this change, not reference material the repository needs to carry. They stay recoverable through history. Claude-Session: https://claude.ai/code/session_01QoUHyt1ALkruCbuYxHSXiB * LEGLINK-620: Add Secret Key to Vendor screen * LEGLINK-620: revert local environment files off the branch The previous commit swept in five files that belong to a local development setup rather than to this change. Both appsettings.Development.json files in particular replaced the committed SQLEXPRESS defaults with a machine-specific SQL Server instance and a plaintext sa password, which would have become the checked-in default for everyone. Restores all five to their dev contents so the branch carries only the vendor signing key work. The AGENTS.md documentation, the check_health.sh compose fix and the Admin.UI Dockerfile npm layer-caching fix are worth landing, but each on its own terms rather than inside this ticket. Claude-Session: https://claude.ai/code/session_01QoUHyt1ALkruCbuYxHSXiB
arianamihailescu
added a commit
that referenced
this pull request
Aug 5, 2026
* LEGLINK-620: record the vendor signing-key UI design Captures the decisions behind associating a Key Vault secret with a vendor, so the reasoning survives outside a chat log: why the association is vendor-scoped rather than facility-scoped (Veradigm needs a different key generation algorithm than Epic and Cerner), and why the stored value is the Key Vault secret id rather than a JWKS kid, which LEGLINK-63's title conflates. Also records what this ticket deliberately cannot do. The update endpoint that persists a secret id is owned by neither LEGLINK-620 nor LEGLINK-743, so the UI is built against a single isolated service method. LEGLINK-63's audit-trail acceptance criterion is produced from backend managers onto Kafka and no UI change can satisfy it. Claude-Session: https://claude.ai/code/session_01STFKxmDaJenKVny6WUoWKo * LEGLINK-620: associate a Key Vault secret with a vendor in the UI Link signs the JWT that Data Acquisition presents to Epic and Cerner during client-credentials auth. LEGLINK-14 moved the PEM into Key Vault; this makes the association explicit and vendor-scoped, because Veradigm needs a different key generation algorithm and so cannot share a key. Add an edit path to Vendor Management. The dashboard grows a Secret ID column that reads "Not set" when empty and a per-row edit action; the form grows a JWT / Authentication panel holding the Key Vault Secret ID, expanded when a vendor already has one so existing configuration is visible without hunting. An emptied box travels as undefined rather than "", so clearing the association reads as absent rather than set-to-empty. No update endpoint exists yet. The Vendor model is moving out of Normalization into Tenant under LEGLINK-743, whose acceptance criteria cover list, add and delete but not update, so this operation is owned by neither ticket. VendorService.updateVendor is the single place that knows the route: when the contract lands, that method is the only edit required. A config-flagged dual path was considered and rejected as permanent complexity bought against a decision expected within days. Two defects in files this already touches: createVendor was typed as IVendorConfigModel while callers pass a name string, which interpolated "[object Object]" into the URL for anything else; and getVendors never cleared its loading flag on success. Deferred: the mocked Playwright spec the design calls for. That harness arrives with PR #1773, which is not yet merged into dev, so there is nowhere on this branch for the spec to live. LEGLINK-63's audit-trail criterion is also outstanding -- audit events are produced from backend managers onto Kafka, so it belongs with the update endpoint rather than here. 16 unit specs pass; the app builds clean. Claude-Session: https://claude.ai/code/session_01STFKxmDaJenKVny6WUoWKo * LEGLINK-620: report a failed vendor save once, and clear a key explicitly Two defects found reviewing 4b894e1 against the design. ErrorHandlingService raises its own toastr before rethrowing, so once the save paths began emitting failure to the dialog -- which shows a snackbar and stays open so the admin's input survives -- one failed save reported itself twice, toastr bottom-full-width and snackbar top-right. Saves now route through handleSaveError, which suppresses the toastr and leaves the dialog as the single surface. List and delete keep theirs, having no dialog to carry the news. The rethrown error still carries the sanitized message either way. Clearing a key sent secretId as undefined, which JSON.stringify drops, so the field never reached the wire. An absent field reads as "leave unchanged" to any endpoint with partial-update semantics, which would have made clearing an association succeed visibly and do nothing. It now travels as an explicit null, and the design's open items record that the backend must honour null as "remove the association" when the contract is settled. Adds vendor.service.spec.ts, the service having had no direct coverage: the update route and body, a cleared key surviving serialization, name escaping in the create route, and the toastr suppressed for saves but kept for list and delete. Both new behaviours fail against the previous code -- args[1] was absent rather than false, and secretId was undefined rather than null. 21 unit specs pass; the app builds clean. Claude-Session: https://claude.ai/code/session_01CX9BzMrPhTzGSakXYDaAVa * LEGLINK-620: cover the vendor create failure branch A failed createVendor had no test. Assert it emits a single failure with the error message and does not fall through into the update path. Claude-Session: https://claude.ai/code/session_01TDPYGwkQDCXConLTuyU4is * LEGLINK-620: gate vendor editing until an update endpoint exists VendorController exposes list, add and delete only -- no PUT -- so the edit dialog added on this branch would save into a 404. Put the edit button and onEdit behind a vendorEditEnabled config flag, shipped off, following the existing AppConfig boolean pattern. Flip it once the update contract, including clearing secretId with null, is confirmed. Claude-Session: https://claude.ai/code/session_01TDPYGwkQDCXConLTuyU4is * LEGLINK-566: design for validating a vendor secret id against Key Vault Validation lives on Admin.BFF, which already holds an ISecretManager and is independent of the Vendor model's move to Tenant. Adds ISecretInspector and PemSigningKeyValidator to Shared; the UI warns inline on blur and on save without ever blocking the save. Claude-Session: https://claude.ai/code/session_01TDPYGwkQDCXConLTuyU4is * LEGLINK-566: implementation plan for vendor secret id validation Eight tasks, TDD throughout: characterize EpicAuth's PKCS#8 behavior, then PemSigningKeyValidator and ISecretInspector in Shared, the Admin.BFF endpoint, the Angular service call, and the form's blur/save warnings. Claude-Session: https://claude.ai/code/session_01TDPYGwkQDCXConLTuyU4is * LEGLINK-620: store a vendor's Key Vault signing key secret id Vendor moved into Tenant under LEGLINK-743 carrying only Id and Name, so there was nowhere to record the Key Vault secret holding a vendor's PEM signing key. LEGLINK-63 scopes that key to the vendor rather than the facility, because the key generation algorithm differs by EHR. Stored as a JSON column rather than a plain one so later vendor-level auth settings need no migration, following the AuthenticationConfiguration precedent in DataAcquisitionDbContext. Only the signing key lives here: TokenUrl, Audience and ClientId are per-EHR-instance and stay on the facility's authentication configuration, where EpicAuth already reads them. The value converter carries an explicit ValueComparer. Without one EF snapshots the property by reference, so mutating a field on the existing instance is never detected and SaveChanges writes nothing -- a test covers that specifically. Update treats a missing authentication object as "leave unchanged", matching how Name already behaves, so a caller that omits it cannot wipe a configured key. Clearing a key means sending the object with a null inside it. Vendor versions expose the parent vendor's settings as a read-only projection, so consumers holding only a vendor version id -- as Data Acquisition will -- can resolve the key in one call while writes stay on the vendor. EpicAuth is untouched: it still derives {facilityId}-pem, so nothing changes at runtime until the fallback rule between vendor and facility keys is settled. Claude-Session: https://claude.ai/code/session_01QoUHyt1ALkruCbuYxHSXiB * LEGLINK-620: point the vendor screens at the Tenant API LEGLINK-743 deleted Normalization's VendorController when it moved the model to Tenant, but left this service calling the old routes. Every vendor operation in Admin.UI has been hitting a controller that no longer exists. The API nests the signing key under authentication; the vendor screens work with a flat secretId. Translating at the gateway keeps that difference out of the components, so a second vendor-level setting only touches this file. Create now carries the secret id. The add form has always shown the field, but the create branch sent the name alone, so anything typed there was silently discarded behind a success message. Both write paths build the same payload before branching, which is what stops them drifting apart again. authentication is always sent, including when the key is being cleared: the Tenant manager reads an absent object as "leave unchanged", so omitting it would make a clear no-op. Null inside the object is what removes the association. The vendorEditEnabled flag is gone with it. It existed only to keep the edit button hidden while no update endpoint existed, and PUT /api/vendor/{id} now does, so the gate has nothing left to protect. Claude-Session: https://claude.ai/code/session_01QoUHyt1ALkruCbuYxHSXiB * LEGLINK-620: drop the superpowers design and plan docs The vendor signing-key design and the secret-id validation design and plan were working notes for this change, not reference material the repository needs to carry. They stay recoverable through history. Claude-Session: https://claude.ai/code/session_01QoUHyt1ALkruCbuYxHSXiB * LEGLINK-620: Add Secret Key to Vendor screen * LEGLINK-620: revert local environment files off the branch The previous commit swept in five files that belong to a local development setup rather than to this change. Both appsettings.Development.json files in particular replaced the committed SQLEXPRESS defaults with a machine-specific SQL Server instance and a plaintext sa password, which would have become the checked-in default for everyone. Restores all five to their dev contents so the branch carries only the vendor signing key work. The AGENTS.md documentation, the check_health.sh compose fix and the Admin.UI Dockerfile npm layer-caching fix are worth landing, but each on its own terms rather than inside this ticket. Claude-Session: https://claude.ai/code/session_01QoUHyt1ALkruCbuYxHSXiB * LEGLINK-620: validate the signing key secret id before persisting it POST and PUT passed the administrator-supplied identifier straight to VendorManager, so an empty, whitespace-only, oversized or otherwise unusable value was stored and only surfaced later as a signing-key resolution failure, far from the admin who typed it. The rule is Azure's documented constraint for a Key Vault object name: 1 to 127 characters of letters, digits and dashes. A value outside it can never resolve, since AzureKeyVaultSecretManager calls the SDK's name-based overload. Note this is the object name, not the object identifier URI an admin may copy from the portal -- that carries a vault host and a pinned version, and pinning a version would quietly stop key rotation from taking effect. Implemented as IValidatableObject on VendorAuthenticationSettings rather than as checks at the two controller call sites: it covers both verbs from one place and matches AuthenticationConfigurationModel. With [ApiController] and no SuppressModelStateInvalidFilter, an invalid value returns 400 with the failure keyed to Authentication.SigningKeySecretId. Null stays valid -- it is how a caller clears the association. Claude-Session: https://claude.ai/code/session_01QoUHyt1ALkruCbuYxHSXiB * LEGLINK-620: check the secret id in the form and show why it was rejected The API now rejects a secret id Key Vault cannot resolve, but the admin only saw "An error occured in our API" plus a trace id: ErrorHandlingService prefers the ProblemDetails `detail`, which is generic, over `errors`, which carries the reason. The vendor form now reads `errors` and shows that instead. The form also applies Azure's rule for an object name -- 1 to 127 characters of letters, digits and dashes -- so a bad value never leaves the browser. The message names the likely mistake: pasting the portal's Secret Identifier URI, which carries a vault host and a pinned version, rather than the secret's name. Validation runs against the trimmed value, so blanking the field still reads as "clear the association" rather than as a malformed name. A pre-existing test covering that clearing behaviour is what caught it. Existing form-level guards already stop an invalid submit: the dialog disables Save while the form is invalid, and submitConfiguration returns early. Scoped to this form. ErrorHandlingService swallows the same `errors` block for every other screen in Admin.UI, which is worth fixing on its own terms. Claude-Session: https://claude.ai/code/session_01QoUHyt1ALkruCbuYxHSXiB
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🛠️ Description of Changes
Adds an end-to-end test suite for Admin.UI with two tiers, wires it into CI, and fixes three defects the suite surfaced.
🧪 Testing Performed
Automated suites
Mocked tier: 40/40 passing, run repeatedly through the change set (npm run e2e). Boots its own ng serve; no backend required.
Live tier: 11/11 passing against the docker compose stack — UI on :8066, BFF on :8063 — via npm run e2e:live, the same command CI invokes. Stack health confirmed before each run so passes weren't masking a down service.
Manual verification in a real browser
I have written or updated unit tests to cover my changes
Coverage: 100.0%
📓 Documentation Updated
Please update any relevant sections in the project documentation that were impacted by the changes in the PR.
Summary by CodeRabbit
New Features
Bug Fixes
Usability