Skip to content

Fix remaining RxJS unbound-operator call sites in sidenav - #677

Merged
fpigeonjr merged 2 commits into
masterfrom
gh-661-fix-remaining-rxjs-unbound-operator-call-sites-in-
Sep 2, 2026
Merged

Fix remaining RxJS unbound-operator call sites in sidenav#677
fpigeonjr merged 2 commits into
masterfrom
gh-661-fix-remaining-rxjs-unbound-operator-call-sites-in-

Conversation

@fpigeonjr

Copy link
Copy Markdown
Contributor

Description

Fixes the last two remaining RxJS 5 "unbound operator" call sites in the library, both in MdSidenavContainer (src/ui-kit/experimental/patterns/layout/components/sidenav/sidenav.ts):

  • Enabling transitions after _sidenavs.changes emits (a sidenav is added/removed dynamically)
  • Re-validating drawers after a sidenav's onAlignChanged emits (a drawer's align changes at runtime)

Both used the form first.call(this._ngZone.onMicrotaskEmpty).subscribe(...). Under RxJS 7 (this repo declares rxjs >=7.5.0), an operator imported from rxjs/operators is a factory that returns an OperatorFunction, so calling it with .call(observable) returns a function, not an Observable — the chained .subscribe(...) then throws TypeError: ...subscribe is not a function at runtime.

Unlike the three sites already fixed in #655 and #660, these two are not dead code: they sit inside subscribe callbacks that only run when a sidenav is added/removed dynamically or a drawer's align changes at runtime, so static sidenav usage never reached them — which is also why #660 covered this file to ~85% without tripping either one.

Converted both to observable.pipe(first()).subscribe(...).

Also adds an ESLint no-restricted-syntax rule banning the <identifier>.call(observable).subscribe(...) shape, so this defect class can't silently regress. The selector is scoped tightly enough that it doesn't flag unrelated .call() usage elsewhere in the codebase (Object.prototype.toString.call, Array.prototype.slice.call, a plain callback's callback.call(context, ...args)), all confirmed clean via a targeted eslint run. Verified the rule fires as an error against a deliberately reintroduced violation, then removed the scratch fixture.

Because the repo now has zero violations of the pattern, eslint-baseline.json did not need to move (warnings actually measured lower: 1575 vs. the 1619 root baseline ceiling). coverage-floor.json is untouched; measured coverage is above the floor on every metric.

Motivation and Context

Closes #661

Type of Change (Select One and Apply Label)

  • Bug fix (non-breaking change which fixes an issue) → Apply bugfix label
  • New feature (non-breaking change which adds functionality) → Apply enhancement label
  • Breaking change (fix or feature that would cause existing functionality to change) → Apply breaking label
  • Documentation / configuration update → Apply maintenance label

How to Test

  1. npm ci && npm ci --prefix test-app
  2. npm run format:check — expect pass
  3. npm run lint — expect pass (root warnings 1575 ≤ baseline 1619, 0 errors)
  4. cd test-app && npm run lint — expect pass (4 warnings ≤ baseline 4, 0 errors)
  5. cd test-app && npm test — expect all specs green, including the three new specs in sidenav.spec.ts covering: a dynamically added sidenav (enables transitions once the microtask queue empties), a dynamically removed sidenav, and a runtime align change (re-validates drawers without throwing)
  6. npm run coverage:check — expect pass, coverage-floor.json unchanged
  7. cd test-app && npm run build — expect a clean Angular build

Expected result: All commands above exit 0. Before this fix, adding/removing a sidenav dynamically or changing a drawer's align at runtime under RxJS 7 would throw TypeError: ...subscribe is not a function from inside MdSidenavContainer; after the fix these paths run without throwing.

Screenshots (if appropriate)

N/A — backend/logic changes only (no visual changes).

Checklist

  • Branch name follows convention (e.g. gh-<number>-<slug>)
  • PR title starts with a verb in the imperative mood
  • I have self-reviewed my own code
  • format:check passes (npm run format:check)
  • lint passes (npm run lint)
  • build passes (cd test-app && npm run build)
  • Tests pass and coverage is reported (cd test-app && npm test)
  • If this change requires a documentation update, I have updated it accordingly
  • If there are dependent changes, they have been merged and published in downstream modules

Both first.call(this._ngZone.onMicrotaskEmpty) sites in
MdSidenavContainer (enabling transitions after _sidenavs.changes, and
re-validating drawers after onAlignChanged) used the RxJS 5 "unbound
operator" pattern. Under RxJS 7, first imported from rxjs/operators is
a factory that returns an OperatorFunction, so calling it with
.call(observable) returns a function, not an Observable -- the chained
.subscribe(...) then throws TypeError at runtime. Unlike the three
sites already fixed in #655 and #660, these two are not dead code:
they sit inside subscribe callbacks that only run when a sidenav is
added/removed dynamically or a drawer's align changes at runtime, so
static usage never reached them.

Switched both to observable.pipe(first()).subscribe(...).

Added specs exercising the previously-unreachable paths: a dynamically
added/removed sidenav (drives _sidenavs.changes) and a runtime align
change (drives onAlignChanged), each asserting detectChanges() does
not throw and that the gated callback (_enableTransitions /
_validateDrawers) actually runs once the microtask queue empties.

Added an ESLint no-restricted-syntax rule banning the
<identifier>.call(observable).subscribe(...) shape so the pattern
can't silently regress. The selector is scoped tightly enough to
avoid flagging unrelated .call() usage elsewhere in the codebase
(Object.prototype.toString.call, Array.prototype.slice.call, a plain
callback's callback.call(context, ...)), all confirmed clean via a
targeted eslint run. Verified the rule fires as an error against a
deliberately reintroduced violation.

Because the repo now has zero violations of the pattern,
eslint-baseline.json needed no change (warnings actually dropped:
1619 -> 1575 root baseline headroom). coverage-floor.json is
untouched; measured coverage is above the floor on every metric.

Closes #661

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The newly added async specs use await Promise.resolve() (and include an outdated comment) where fixture.whenStable() is the more reliable, idiomatic way to wait for NgZone stability, reducing flakiness risk.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes the remaining RxJS 5-style “unbound operator” usages in the experimental sidenav container by switching to RxJS 7+ pipe(...) operator usage, adds regression coverage in the sidenav specs, and introduces an ESLint guard to prevent reintroducing the problematic .call(observable).subscribe(...) pattern.

Changes:

  • Replace first.call(this._ngZone.onMicrotaskEmpty) with this._ngZone.onMicrotaskEmpty.pipe(first()) in MdSidenavContainer.
  • Add unit specs covering dynamic sidenav add/remove and runtime align changes to exercise the previously-unreached code paths.
  • Add an ESLint no-restricted-syntax rule to error on the unbound-operator call shape.
File summaries
File Description
src/ui-kit/experimental/patterns/layout/components/sidenav/sidenav.ts Converts two remaining unbound-operator call sites to pipe(first()) to be compatible with RxJS 7+.
src/ui-kit/experimental/patterns/layout/components/sidenav/sidenav.spec.ts Adds tests for dynamic sidenav changes and align changes to cover the fixed paths and prevent regression.
eslint.config.mjs Adds a restricted-syntax selector to block reintroduction of the unbound-operator .call(...).subscribe(...) pattern.
Review details

Suppressed comments (3)

src/ui-kit/experimental/patterns/layout/components/sidenav/sidenav.spec.ts:260

  • await Promise.resolve() does not guarantee the Angular fixture is stable (it only schedules one microtask). Use await fixture.whenStable() to ensure NgZone.onMicrotaskEmpty has a chance to emit and to reduce test flakiness.
    await Promise.resolve();

src/ui-kit/experimental/patterns/layout/components/sidenav/sidenav.spec.ts:267

  • Same as above: prefer await fixture.whenStable() over await Promise.resolve() when the intent is to wait for zone microtasks to flush / onMicrotaskEmpty to run.
    await Promise.resolve();

src/ui-kit/experimental/patterns/layout/components/sidenav/sidenav.spec.ts:285

  • The comment references the pre-fix first(this._ngZone.onMicrotaskEmpty) form, but the implementation is now onMicrotaskEmpty.pipe(first()); also, use fixture.whenStable() to reliably wait for the microtask-empty callback rather than a single resolved Promise.
    // The container waits for the microtask queue to be empty (via
    // first(this._ngZone.onMicrotaskEmpty)) before re-validating, since both
    // drawers may be swapping sides at the same time.
    await Promise.resolve();
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/ui-kit/experimental/patterns/layout/components/sidenav/sidenav.spec.ts Outdated
Address PR review feedback: await Promise.resolve() only flushes a
single microtask and isn't a reliable proxy for NgZone/Angular
stability, which could make these specs flaky. Switch to
fixture.whenStable() in all four call sites (dynamic add, dynamic
remove x2, align change), and update the stale in-code comment that
still referenced the pre-fix first(this._ngZone.onMicrotaskEmpty)
form to describe the actual onMicrotaskEmpty.pipe(first()) shape.
@fpigeonjr
fpigeonjr marked this pull request as ready for review September 2, 2026 19:55
@fpigeonjr
fpigeonjr requested a review from a team as a code owner September 2, 2026 19:55
@fpigeonjr
fpigeonjr merged commit 595d3ec into master Sep 2, 2026
7 checks passed
fpigeonjr added a commit that referenced this pull request Sep 3, 2026
- Rebase branch tip onto latest #675 (gh-582) so this PR's diff/baseline
  reflect only #586's type-safety changes, not #675's still-open autofix
  set. #675 is CLEAN/MERGEABLE and independently reviewed; stacking is the
  documented convention for this repo's slice-based PRs.
- Retarget PR base to gh-582-apply-safe-eslint-autofixes-and-lower-the-baseline
  so the GitHub diff matches (rebase alone doesn't move a PR's base).
- Drop the #674 datepicker outside-click fix and #677 sidenav RxJS fix
  commits from this branch entirely (dropped during rebase) -- both are
  unrelated bugfixes already merged to master via their own PRs; they were
  only present here as inherited ancestry from an earlier base choice, not
  something this PR should carry or take credit for.
- pagination.component.ts: options.value widened to 'string | number' --
  the template already supports numeric option values via attribute
  binding; the prior 'any[]' allowed this and the fix's 'string' literal
  would have narrowed a supported case.
- date-time-display.pipe.ts: transform() parameter widened to include
  'undefined' explicitly -- the implementation's own guard branch handles
  undefined and the pipe previously accepted it; the stricter signature
  would have been a source-compatibility break for existing callers.
- types.ts: HistoryNodeType.queryParams value type widened to include
  readonly arrays and null, matching Angular Router's actual accepted
  queryParams value shapes (repeated params, param removal) instead of
  the narrower 'string | number | boolean' which would reject valid
  existing usage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix remaining RxJS unbound-operator call sites in sidenav and guard against regression

3 participants