Skip to content

feat(signal-slice): optimize subscription management - #700

Open
endlacer wants to merge 1 commit into
ngxtension:mainfrom
endlacer:feature/signal-slice-dead-code
Open

feat(signal-slice): optimize subscription management#700
endlacer wants to merge 1 commit into
ngxtension:mainfrom
endlacer:feature/signal-slice-dead-code

Conversation

@endlacer

@endlacer endlacer commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Problem

Calling a function-form actionSource (e.g. increaseAge: (state, $) => $.pipe(...)) with a plain, non-Observable value created a brand-new subscription to the action's already-connected, shared pipeline on every call, in addition to the one permanent subscription connect() makes once at slice-creation time. Each of these extra subscriptions was wired to takeUntilDestroyed(destroyRef) and only released when the owning injector was destroyed — never between calls — so live subscriptions accumulated without bound for the lifetime of the slice.

Root cause

The offending .subscribe() call in addReducerProperties used to have real handlers: it fed an effectTrigger Subject that powered the experimental actionEffects API (added in #154). When actionEffects was removed in #361, the next/error handlers were stripped out but the bare .subscribe() call was left behind - a leftover with nothing listening, serving no purpose beyond registering a new teardown per call.

Fix

Deleted the dead resubscription block and the now-unused observableFromActionSource parameter it depended on. State updates are unaffected - they were already handled entirely by the permanent connect(state, sharedObservable) subscription made once at slice creation.

Testing

  • Added a regression test asserting that repeated calls to a plain-value action don't register additional DestroyRef.onDestroy teardowns beyond the one made at slice creation.
  • Verified the test fails against the old code (i.e. it actually catches the leak) and passes with the fix.
  • Full signal-slice suite (28 tests) and ngxtension lint pass.

No public API changes.

@nx-cloud

nx-cloud Bot commented Jul 8, 2026

Copy link
Copy Markdown

View your CI Pipeline Execution ↗ for commit 753d88d

Command Status Duration Result
nx affected --target=build --parallel=3 --exclu... ✅ Succeeded <1s View ↗
nx affected --target=test --parallel=3 --exclud... ✅ Succeeded 5s View ↗
nx affected --target=lint --parallel=3 ✅ Succeeded 31s View ↗
nx-cloud record -- nx format:check ✅ Succeeded 2s View ↗

💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗


☁️ Nx Cloud last updated this comment at 2026-07-08 06:47:34 UTC

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request optimizes subscription handling in signalSlice by removing redundant subscriptions to action sources and preventing multiple permanent subscription teardowns when actions are called with plain values. A unit test has been added to verify this behavior. The feedback suggests further cleanup by removing the now-redundant share() operator on the observable and restoring the Jest spy at the end of the new test to prevent mock pollution.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines 209 to 212
destroyRef,
subject,
subs,
sharedObservable,
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Since sharedObservable is no longer passed to addReducerProperties, it is only subscribed to once by connect(state, sharedObservable) at line 204. This makes the share() operator on line 203 redundant. You can clean this up by removing const sharedObservable = observable.pipe(share()); and passing observable directly to connect(state, observable).

state.increaseAge(1);
state.increaseAge(1);

expect(onDestroySpy.mock.calls.length).toEqual(baseline);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

It is a good practice to restore Jest spies at the end of the test to prevent potential mock pollution or side effects in other tests within the same suite.

				expect(onDestroySpy.mock.calls.length).toEqual(baseline);
				onDestroySpy.mockRestore();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant