Fix inactive tab content rendering on top of active tab in sam-tabs-next - #668
Conversation
Add a shared Playwright component-render harness in test-app (routed
gallery shell with a /tabs route, BrowserAnimationsModule, deep-imported
SamTabsNextModule) that can actually render library components from the
raw source tree, then use it to reproduce and fix the tab-overlap bug.
- test-app/tsconfig.json + tsconfig.app.json: paths mapping resolving
@gsa-sam/sam-ui-elements to the root src/ tree, plus skipLibCheck
- app.module.ts: routed shell (/ -> HomeComponent, /tabs ->
TabsGalleryComponent), BrowserAnimationsModule (not Noop), imports
SamTabsNextModule
- app.component: reduced to a lean <router-outlet> shell; original
placeholder content moved to new HomeComponent so smoke.spec.ts still
has a home page to assert against
- New TabsGalleryComponent renders <sam-tabs-next> with two tabs of real
content, at the /tabs route
- test-app/angular.json: swapped build/serve to
@angular-builders/custom-esbuild (still esbuild/Vite under the hood,
no webpack) so a plugin can be registered
- New test-app/esbuild/dedupe-angular-plugin.ts: forces @angular/*,
rxjs, and zone.js to resolve from test-app/node_modules regardless of
which physical source tree does the importing. Without this, files
under root src/ui-kit and files under test-app/src/app resolve two
separate copies of @angular/core into the same bundle, which breaks
Angular's DI context tracking and throws NG0203 as soon as any
root-tree component is instantiated. Mirrors the resolve.dedupe
workaround vitest.config.mts already applies for the Vitest path.
- New test-app/e2e/tabs.spec.ts: navigates to /tabs, switches tabs, and
asserts the previously-active tab's content is hidden immediately
(tight 100ms window, since the animation eventually detaches content
regardless of the CSS bug, so a default-timeout assertion would not
have caught the regression)
The actual bug: tab-group.scss's .mat-tab-body { display: block; } is
an author-stylesheet rule, which always wins the cascade over the
browser's built-in [hidden] { display: none; } user-agent rule that
tab-group.html relies on to hide inactive tab bodies. Fixed by adding
an explicit &[hidden] { display: none; } rule so [hidden] wins.
Confirmed the new e2e spec fails against the unpatched CSS (reproduces
the bug) and passes after the fix. Existing tab-group.spec.ts and
tab-header.spec.ts (ink-bar, keyboard nav) unaffected.
Match the rest of test-app/package.json, which pins every dependency exactly (no ^/~ ranges).
test-app compiles the library straight from the root src/ tree (see the paths mapping in test-app/tsconfig.json), so those library source files' @angular/*, @angular/cdk, rxjs, and zone.js imports need the root node_modules to be present -- they're peerDependencies of the root package, not test-app's own dependencies. The e2e workflow only ran `npm install --prefix test-app`, so on a clean runner (no root node_modules) esbuild couldn't resolve any of those imports from library source files, and the Playwright webServer never came up before the 120s timeout. Verified locally by removing both node_modules trees and reinstalling exactly as the workflow now does; npm run test:e2e passes.
fpigeonjr
left a comment
fpigeonjr
left a comment
There was a problem hiding this comment.
Review: ✅ Approve
| Result | ⛔ blocking | 💡 suggestion | |
|---|---|---|---|
| ✅ Approve | 0 | 5 | 3 |
Note
The CSS fix and the harness are both correct — I verified the red/green transition and that the entering tab's centering animation still interpolates. The findings below are all about the dedupe plugin's resolution strategy and the harness's durability for #666/#583.
Must fix before merge
No blocking findings.
Should verify before merge
🟡 require.resolve forces rxjs to its CommonJS entry, inflating the bundle ~42% and permanently adding "not ESM" build warnings — test-app/esbuild/dedupe-angular-plugin.ts:56
require.resolve applies Node's require condition, so rxjs resolves to dist/cjs/index.js rather than the ESM entry esbuild would normally pick — which is exactly why every build now logs two Module 'rxjs' ... is not ESM / optimization bailouts warnings. Measured on this branch: as committed, main.js is 474.58 kB with 2 warnings; swapping the onResolve body to esbuild's own build.resolve() (with a pluginData re-entry guard) gives 334.29 kB, 0 warnings, still a single rxjs copy and a single @angular/core, and e2e still passes. That's the strictly better option — it keeps the dedupe and the ESM entry, unlike simply dropping rxjs from the list (350.63 kB, but two rxjs copies creep back in).
🟡 The dedupe list has already drifted from vitest.config.mts, and the next gallery route may re-trigger NG0203 — test-app/esbuild/dedupe-angular-plugin.ts:25
The plugin's list is the Vitest resolve.dedupe list minus @fortawesome/angular-fontawesome and @fortawesome/fontawesome-svg-core. Those two are in the Vitest list because library source needs them deduped; the moment a gallery route renders a FontAwesome-using component, the esbuild path hits the same duplicate-instance failure this plugin exists to prevent — and the symptom (NG0203) is famously hard to trace back here. Since the doc comment already names vitest.config.mts as the sibling, export the array from one module and import it in both, so drift becomes impossible.
🟡 --no-package-lock on the new root install makes the e2e job non-reproducible against unbounded peer ranges — .github/workflows/e2e.yml:26
The root package declares its Angular stack in peerDependencies with ranges including rxjs: >=7.5.0 and @angular/cdk: ^19.2.15, and --no-package-lock discards the pins in package-lock.json (which currently resolves @angular/core to 19.2.25) and re-resolves them fresh on every run. An upstream rxjs 8 release would silently enter this build; ci.yml avoids that by using npm ci for both workspaces. Worth switching to npm ci here. Note the reason a root install is needed at all is AOT typechecking, not bundling — the CI failure was TS2307 ... [plugin angular-compiler], and the esbuild plugin only rewrites the bundler's module graph. If a @angular/* entry in the paths map can satisfy the compiler instead, the root install step and the version-skew surface both disappear; that seems worth a look before locking this in.
🟡 The regression gate races a 100 ms timer instead of asserting the property under test — test-app/e2e/tabs.spec.ts:26
not.toBeVisible({ timeout: 100 }) is correct today (I confirmed it fails on the unfixed SCSS and passes on the fix), but it couples the gate to how fast the runner processes the click plus change detection — a loaded CI runner that needs >100 ms to flip [hidden] produces a false red. The issue explicitly asked for deterministic assertions ("Assert toBeVisible(), computed style, and bounding boxes — deterministic, and it states intent"), and the deterministic form is right there: assert getComputedStyle on the leaving md-tab-body is none, or that the two bodies' bounding boxes don't overlap. That states the cascade intent directly and removes the timing coupling.
🟡 The harness convention this PR exists to establish isn't written down anywhere — AGENTS.md
The issue's framing was that getting the convention right makes #666 "add a /datepicker route plus a spec" — but nothing in AGENTS.md or test-app/README.md records the one-route-per-component URL convention, the paths mapping, or, most importantly, that esbuild/dedupe-angular-plugin.ts exists and why. AGENTS.md's Testing section already documents the Vitest alias ordering and the @gsa-sam/icons alias for exactly this reason; the next person to add a route will otherwise rediscover NG0203 from scratch. The PR's own "documentation update" checkbox is still unchecked.
💡 Suggestions and notes
🔵 zone.js in the dedupe list is dead weight and quietly papers over a version skew — test-app/esbuild/dedupe-angular-plugin.ts:36
No file under src/ imports zone.js (it only enters via the polyfills array, which is already resolved from test-app), so the entry does nothing. It is not harmless, though: root has 0.15.1 and test-app has 0.16.2, so the entry's only real effect is to hide that skew rather than surface it.
🔵 home.component.spec.ts is a verbatim copy of the old AppComponent spec, including "should create the app" — test-app/src/app/home/home.component.spec.ts:9
The move is fine, but the test names now describe a component that no longer does this ("the app" is the router shell). Renaming to "should create" / "should render the title in an h1" keeps the spec honest, and the empty home.component.css carried along by the rename could go with it.
🔵 extract-i18n is now on a third builder family — test-app/angular.json:53
build and serve moved to @angular-builders/custom-esbuild, but extract-i18n went to @angular/build. Nothing exercises that target, so it's cosmetic — aligning all three on one family just removes a "why is this one different?" question later.
Verified, no finding:
- The
&[hidden]fix makes the leaving tabdisplay: nonesynchronously, which means its 500 ms slide-out is no longer rendered. I checked this doesn't leak an attached portal —translateTab.donestill fires and_portalHost.detach()still runs (leaving body reportshasContent: falseafter settle). - Entering-tab centering animation still runs post-fix: content transform interpolates
matrix(1,0,0,1,17.32,0)→matrix(1,0,0,1,0,0)over ~500 ms. npm run validate:publishpasses;tab-group.scssis not inscripts/consumer-deep-imports.json, so no consumer deep-import contract is touched.- Neither
coverage-floor.jsonnoreslint-baseline.jsonwas edited, per the ratchet rules inAGENTS.md. - Spec AC all met:
paths+skipLibCheck, single/tabsroute,BrowserAnimationsModule(not Noop),smoke.spec.tsstill passes, and nostyles/scriptsadded toangular.jsonas the issue required. - Ink-bar position and keyboard nav are still only covered by jsdom unit specs, which can't see real-browser layout — the same limitation that motivated this harness. Now that the harness exists, those are cheap follow-up assertions rather than a gap in this PR.
Recommendation
✅ Approve — the cascade fix is correct and properly gated by a spec I confirmed is red before it and green after, and every acceptance criterion in #665 is met. None of the findings above block merge; the build.resolve() swap and the shared dedupe list are the two I'd most want picked up, either here or as immediate follow-ups, since both affect the durability of the harness for #666 and #583.
This review was drafted by @fpigeonjr's coding agent.
- test-app/esbuild/dedupe-angular-plugin.ts: use esbuild's own
build.resolve() (with a pluginData re-entry guard) instead of Node's
require.resolve, which forced the "require" export condition and
picked rxjs's CommonJS entry over its ESM one. main.js: 474.58 kB (2
"not ESM" warnings) -> 334.29 kB (0 warnings), still a single copy
of rxjs and @angular/core.
- test-app/dedupe-packages.ts: extract the dedupe package list into a
single shared module imported by both vitest.config.mts and
dedupe-angular-plugin.ts, so the two configs can no longer drift
(the esbuild list was missing @fortawesome/angular-fontawesome and
@fortawesome/fontawesome-svg-core, which vitest.config.mts already
depended on). Dropped zone.js from the shared list per review: no
src/ file imports it, it only enters via the polyfills array, which
esbuild already resolves from test-app.
- .github/workflows/e2e.yml: swap npm install --no-package-lock for
npm ci on both workspaces, matching ci.yml, so the e2e job installs
the exact pinned dependency versions instead of re-resolving them
against unbounded peerDependency ranges on every run.
- test-app/e2e/tabs.spec.ts: replace the timing-coupled
not.toBeVisible({ timeout: 100 }) assertion with a deterministic
toHaveCSS("display", "none") check on the leaving tab body -
asserts the cascade property GH-665's fix establishes directly,
with no race against how fast the runner applies [hidden].
- AGENTS.md: document the Playwright component-render harness
convention (one route per component, the tsconfig paths mapping,
BrowserAnimationsModule, and why dedupe-angular-plugin.ts exists)
so the next gallery route (e.g. #666) doesn't have to rediscover
NG0203 from scratch.
- home.component.spec.ts/.ts: rename test names off the old
AppComponent copy ("should create the app" -> "should create",
"should render title in a h1 tag" -> "should render the title in an
h1") and drop the empty home.component.css that came along with
the rename.
Verified locally: npm --prefix test-app run test:e2e passes (and
fails against the unpatched tab-group.scss, confirming the new
assertion still gates the regression); npm --prefix test-app test
(1377 tests) and npm run coverage:check both still pass; npm run
build (test-app) still succeeds with the smaller esbuild bundle.
|
Addressed all 5 "should verify" findings from the review in 1f2500d:
Also took the two 💡 suggestions:
Left the Re-verified after all changes: |
master has been red since #670 merged. The failure is the coverage-floor gate, not a test failure — all 1377 specs pass: statements 88.57% (floor 88.73%) branches 78.57% (floor 78.65%) functions 85.74% (floor 85.90%) lines 88.55% (floor 88.72%) Cause is a merge-order collision between #668 and #670. #668 (merged 11:21) added the Playwright component-render harness, including two test-app build-tooling files — dedupe-packages.ts and esbuild/dedupe-angular-plugin.ts. Both run inside the bundler, so no spec can ever execute them, but coverage.all walks the project and they weren't in coverage.exclude, so they landed in the report at 0% (14 statements, 4 branches, 4 functions, 14 lines of pure denominator). #670 (merged 11:24) then locked the floor at coverage measured on a branch cut from 081088f — before #668 — so its denominator never saw those files. Each PR was green on its own; master got the union. Extend coverage.exclude to cover both files, matching the rationale already documented for src/main.ts, app.module.ts, environments/** and playwright.config.ts: keep harness/build scaffolding that has nothing to do with the library under test out of the denominator. Verified against the failing run's coverage artifact that dropping exactly these two files restores statements 88.73 / branches 78.65 / functions 85.90 / lines 88.72 — at or above every floor, so coverage-floor.json is untouched (and per AGENTS.md, lowering the floor to go green isn't an option). Refs #637
Description
Two parts, per the issue:
1. Shared Playwright component-render harness in
test-app.test-appas committed onmastercannot render any library component (nopathsmapping, no routing,BrowserModuleonly, a single placeholder smoke spec). Added:tsconfig.json/tsconfig.app.json:pathsmapping@gsa-sam/sam-ui-elements→ rootsrc/, plusskipLibCheckapp.module.ts:/→ newHomeComponent(holds the original CLI placeholder content, sosmoke.spec.tsstill has something to assert against),/tabs→ newTabsGalleryComponentrendering<sam-tabs-next>with two tabs of real contentBrowserAnimationsModule(notNoopAnimationsModule), sincetab-body.tshas a realtranslateTabanimation trigger that a browser test needs to exercise for realSamTabsNextModuleimported via the deep path@gsa-sam/sam-ui-elements/src/ui-kit/experimental/tabs(the package root barrel doesn't export it)test-app/angular.jsonswappedbuild/serveto@angular-builders/custom-esbuild(still esbuild/Vite under the hood — not a webpack regression) so a custom resolution plugin could be registeredtest-app/esbuild/dedupe-angular-plugin.ts: forces@angular/*,rxjs, andzone.jsto resolve from a single copy (test-app/node_modules) no matter which physical source tree (rootsrc/ui-kit/...vstest-app/src/app/...) does the importing. Without this, the two trees resolve two separate copies of@angular/coreinto one bundle, which splits Angular's DI-context tracking and throwsNG0203the instant any root-tree component is instantiated. This mirrors theresolve.dedupeworkaroundvitest.config.mtsalready applies for the Vitest/unit-test path — same problem, now also covered on the app-serve/build path.2. The tab overlap bug itself.
tab-group.scss's.mat-tab-body { display: block; }is an author-stylesheet rule, which always wins the cascade over the browser's built-in[hidden] { display: none; }user-agent rule thattab-group.htmlrelies on ([hidden]="selectedIndex !== i") to hide inactive tab bodies — so the inactive tab's absolutely-positioned content never actually gets hidden and renders stacked on top of the active tab. Fixed with an explicit&[hidden] { display: none; }rule so[hidden]wins.Motivation and Context
Closes #665
Type of Change (Select One and Apply Label)
bugfixlabelenhancementlabelbreakinglabelmaintenancelabelHow to Test
npm ci && npm ci --prefix test-appnpm --prefix test-app run test:e2e— runssmoke.spec.tsand the newtabs.spec.ts; both should passgit stash -- src/ui-kit/experimental/tabs/tab-group.scssthen re-runnpm --prefix test-app run test:e2e—tabs.spec.tsfails (previously-active tab content still visible immediately after switching tabs);git stash popto restore the fixnpm --prefix test-app test— full unit suite (1377 tests), including existingtab-group.spec.tsandtab-header.spec.ts(ink-bar, keyboard nav), all passcd test-app && npm start, openhttp://localhost:4200/tabs, click between "Tab One" / "Tab Two" — only the active tab's content should be visible, with the slide animation still playingExpected result:
test:e2e, unit tests, lint baseline, coverage floor, and format checks all pass; only the active tab's content is ever visible in the browser, with no regression to the ink-bar position, keyboard navigation, or the centering/slide animation.Screenshots (if appropriate)
N/A — behavioral/CSS fix, best verified via the new e2e spec and the manual repro steps above rather than a static screenshot.
Checklist
gh-<number>-<slug>)format:checkpasses (npm run format:check)lintpasses (npm run lint)buildpasses (cd test-app && npm run build)cd test-app && npm test)