NAS-143957 / 27.0.0-BETA.1 / Let an error dialog name itself, and the shell render its title - #14177
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -1,10 +1,5 @@ | |||
| <tn-dialog-shell testId="error" [title]="''" [ariaLabel]="error.title | translate"> | |||
| <tn-dialog-shell [testId]="error.testId || 'error'" [title]="error.title | translate"> | |||
There was a problem hiding this comment.
BLOCKER — the title moves into a header that a global rule hides, so every error dialog now renders with no title at all.
src/assets/styles/other/_tn-styles.scss:365-368 is still in the tree, unchanged by this PR:
ix-error-dialog .tn-dialog__header,
ix-info-dialog .tn-dialog__header {
display: none;
}.tn-dialog__title lives inside .tn-dialog__header — truenas-connect-status-modal.component.scss:6-11 says so explicitly ("The shell header lays the title out flex:1 with the close (X) button after it"). So passing a real [title] renders the heading into a subtree that is display: none, while the <h1 class="err-title-row"> that used to carry the title in the body is deleted. Failing input: any error dialog in a built app — a failed API call, or the Max Concurrent Calls diagnostic itself — shows the message with no heading above it.
Neither test layer can see it:
- Jest loads no global stylesheet, so
TnDialogHarness.getTitle()reads the text node and passes. - Playwright's
filter({ has: ... })matches on DOM presence, not visibility, so the E2E harness findsdialog-title-concurrent-callsinside the hidden header and the suite stays green.
The collapse rule and its two companion comments (error-dialog.component.scss:16-18, info-dialog.component.scss:6-8) are the same workaround this PR is unwinding — _tn-styles.scss:357-364 marks it TEMP "until @truenas/ui-components ships an icon/custom title for tn-dialog-shell", NAS-141021. Unwinding it means dropping ix-error-dialog from that selector in the same change (and deciding what happens to ix-info-dialog, which still renders its own body heading and needs the rule kept).
| * harness dismisses it rather than failing. | ||
| */ | ||
| export const concurrentCallsDialogTitle = 'Max Concurrent Calls'; | ||
| export const concurrentCallsDialogTitle = '[data-test="dialog-title-concurrent-calls"]'; |
There was a problem hiding this comment.
LOW — the name no longer describes the value. concurrentCallsDialogTitle reads as the dialog's title text at both use sites, and it now holds an attribute selector; fixtures.ts:77 passes it to page.locator(), where a reader following the name would expect hasText. Worth renaming to match what it is:
| export const concurrentCallsDialogTitle = '[data-test="dialog-title-concurrent-calls"]'; | |
| export const concurrentCallsDialogTitleId = '[data-test="dialog-title-concurrent-calls"]'; |
(with the import and use in fixtures.ts following.)
| stackTrace?: string; | ||
| logs?: Job; | ||
| icon?: string; | ||
| actions?: ErrorReportAction[]; |
There was a problem hiding this comment.
LOW — removing icon leaves the two error dialogs disagreeing. error-template.component.html:2 (the multi-error dialog) still renders alert-circle beside each error title, and _tn-styles.scss:226-229 still styles .error-warning-icon for it. After this change a single error has no icon and the same error in a list of two does — and the connectivity cue the PR description flags (cloud-off on ECONNRESET / ETIMEDOUT / ENETUNREACH) is gone from the one place a user most often sees it.
Not a correctness problem, and the PR calls the behaviour change out, so this is a note rather than an objection: whichever way it lands, the two dialogs should land the same way.
|
|
||
| it('shows error title', () => { | ||
| expect(spectator.query('.err-title')).toHaveText(baseError.title); | ||
| it('shows error title', async () => { |
There was a problem hiding this comment.
LOW — nothing here covers testId, which is the contract the whole PR exists to create. The only assertion that testId: 'concurrent-calls' becomes dialog-title-concurrent-calls is the E2E harness, which runs against an appliance and not on every PR — so a library change to how tn-dialog-shell composes that id would land silently and surface as a hung E2E run. A sibling case in this block would pin it:
it('names the dialog after the error when a testId is given', async () => {
spectator = createComponent({
providers: [{ provide: DIALOG_DATA, useValue: { ...baseError, testId: 'concurrent-calls' } as ErrorReport }],
});
expect(spectator.query('[data-test="dialog-title-concurrent-calls"]')).toExist();
});
|
Four findings: all LOW. A human review is requested — not for anything wrong, but for the user-visible icon removal noted below. ✨ Replacing prose matching with an opt-in LOW — LOW — LOW — LOW — Why a human: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #14177 +/- ##
==========================================
- Coverage 89.27% 89.26% -0.01%
==========================================
Files 1876 1876
Lines 70495 70493 -2
Branches 9307 9306 -1
==========================================
- Hits 62931 62927 -4
- Misses 7564 7566 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
bugclerk
changed the title
| // the shell's header, outside the message block — but reading the whole | ||
| // dialog to get it would quote its buttons too. | ||
| const details = (await Promise.all( | ||
| [errorDialogHeading, errorDialogBody].map((part) => page.locator(part).innerText()), |
There was a problem hiding this comment.
LOW — Both parts are scoped to ix-error-dialog, but the gate above (errorDialogClose = [data-test="button-close-error-dialog"]) is not: multi-error-dialog.component.html uses the same testId="close-error-dialog" on its footer button. So if middleware returns several errors at sign-in, ix-multi-error-dialog renders, closeButton.isVisible() is true, and neither ix-error-dialog h2 nor ix-error-dialog [role="alertdialog"] ever resolves — both innerText() calls sit on the default timeout and throw a Playwright "waiting for locator" error, so the crafted "Sign-in as … failed with a middleware error" message (and the rate-limit hint under it) never gets thrown. That's exactly the R6.2 legibility this block exists to provide.
The old getByRole('alertdialog') had the same hole, so this isn't a regression — but these are the lines being rewritten, so it's a cheap moment to close it: give each innerText() a short explicit timeout and fall back to page.locator(errorDialog).innerText() (or add ix-multi-error-dialog to the two part selectors) rather than letting a quotable failure turn into a timeout. 🙂
| * is noise in the one place a failure has to be legible (R6.2). | ||
| */ | ||
| export const errorDialog = 'ix-error-dialog'; | ||
| export const errorDialogHeading = 'ix-error-dialog h2'; |
There was a problem hiding this comment.
LOW — The constant these two replace carried an explicit "documented exception to R5.1" paragraph; the new block drops it, and these are tag/attribute selectors (h2, [role="alertdialog"]) rather than data-test — precisely what R5.1 lists as not allowed (e2e/docs/status.md:182). Either restate the exception here as the old doc did, or better: the heading now has a data-test, courtesy of this very PR, so it can just use it —
| export const errorDialogHeading = 'ix-error-dialog h2'; | |
| export const errorDialogHeading = 'ix-error-dialog [data-test^="dialog-title-"]'; |
which also stops depending on the library's choice of heading tag. errorDialogBody has no id to point at, so that one genuinely is an exception and is worth saying so in the comment.
| // library change to how the shell composes it fails in this repo instead | ||
| // of surfacing as a hung suite against an appliance. | ||
| expect(spectator.query('tn-dialog-shell h2')) | ||
| .toHaveAttribute('data-test', 'dialog-title-concurrent-calls'); |
There was a problem hiding this comment.
LOW — Nice idea, but the comment claims slightly more than the assertion delivers. What is pinned here is the shell's composition rule (dialog-title- + base), which is the library-change case the comment describes — that part works. What isn't pinned is the value: the spec supplies its own 'concurrent-calls' literal, so renaming testId in websocket-handler.service.ts:279 leaves this test green while concurrentCallsDialogTitleId in e2e/support/constants.ts stops matching. Both sides document the coupling, and the failure is loud (the dialog stays up and awaitAdminShell reports it), so this is minor — but either import the value from the service here, or trim the comment to claim only the composition.
| @@ -1,10 +1,5 @@ | |||
| <tn-dialog-shell testId="error" [title]="''" [ariaLabel]="error.title | translate"> | |||
| <tn-dialog-shell [testId]="error.testId || 'error'" [title]="error.title | translate"> | |||
| <div id="err-md-content" class="err-content" role="alertdialog"> | |||
There was a problem hiding this comment.
LOW — Now that the heading has moved up to the shell, this role="alertdialog" wraps only the message: a nested dialog role inside the shell's own dialog container, with no aria-label/aria-labelledby of its own. It was already odd before, but it at least contained the <h1> title; a screen reader now enters a second, unnamed alert dialog holding just body text.
Worth considering dropping the role (the shell owns the dialog semantics and, per tn-fallback-labels.provider.ts, names the container from [title] — which this PR correctly relies on). Note that e2e/support/constants.ts now points errorDialogBody at this attribute, so the two changes go together; a data-test on the wrapper would decouple them.
There was a problem hiding this comment.
Needs a human review. 4 finding(s): 4 LOW.
Nothing blocks, but this change is one a person should decide on:
- src/app/services/errors/error-parser.service.ts removes icon: cloud-off from the three network-error branches, so network errors lose their distinct connectivity cue
- src/app/modules/dialog/components/error-dialog/error-dialog.component.html removes the alert-circle heading icon, so every error dialog loses its at-a-glance severity cue
Superseded by the review of cadc67b.
|
This PR has been merged and conversations have been locked. |
This branch was successfully deployed
3 participants
Changes:
Every error dialog opens the same component with the same
testId="error", so nothing could tell one from another except its wording. The E2E harness had to match the development build's "Max Concurrent Calls" diagnostic by its prose in order to dismiss it — a selector that breaks the moment the message is reworded or translated.Two changes close that:
ErrorReportgains an optionaltestId, bound totn-dialog-shell. Unset, a dialog keeps the sharederrorbase it has always had; set, it names itself.websocket-handler.service.tspassesconcurrent-callsfor the concurrency diagnostic.error-dialog.component.htmlwas passing[title]="''"and rendering its own<h1>— an icon beside the title — so the shell's heading, and thedialog-title-*id that comes with it, never existed. That heading is now the shell's, and the harness matches[data-test="dialog-title-concurrent-calls"]instead of prose.Removing the custom heading makes three things dead, all removed: the
iconfield onErrorReport, its styles, and the[ariaLabel]binding (with a real title the shell pointsaria-labelledbyat its own heading).Needs
@truenas/ui-components>= 0.7.8, already on master, fordialog-title-<base>(iXsystems/truenas-ui-components#319).Testing:
data-testvalues — nothing removed or changed; two new ones appear only when a caller opts in viatestId:dialog-title-<testId>testId—dialog-title-concurrent-callstodaybutton-close-<testId>button-close-error-dialog(the footer Close) andbutton-close-errorare untouched, so nothing downstream moves.Unit tests: the error-dialog spec now asserts the title through
TnDialogHarness.getTitle()rather than a CSS class — 17 passing. Three icon tests were deleted along with the behaviour. The error-parser spec drops threeiconexpectations — 22 passing.Full E2E suite against a TrueNAS v27 nightly from a golden snapshot: 26 passed, 2 skipped. The two skips are deliberate
test.skipcalls (the HTTPS-warning assertion cannot hold under thebranchprofile; S3 versioning needs theS3_VERSIONINGentitlement).One behaviour change worth a reviewer's eye: network errors lose their distinct icon.
ECONNRESET,ETIMEDOUTandENETUNREACHseticon: cloud-offto mark themselves as connectivity rather than appliance problems, and now show no icon like every other error. The "Network Error" title, the hint, and the "Network Settings" action are unchanged, so nothing is unsayable — but the at-a-glance cue is gone.Downstream
dialog-title-*/button-close-*ids for opted-in error dialogs; removes none. The E2E harness now matches the concurrency dialog by id instead of its wording. Any suite asserting the error dialog's icon or its.err-titleelement needs updating — in tree, only the two specs changed here did.