Fix angular input + API consistency#161
Open
benjavicente wants to merge 10 commits into
Open
Conversation
🦋 Changeset detectedLatest commit: e2eb851 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
View your CI Pipeline Execution ↗ for commit c8d1f5f
☁️ Nx Cloud last updated this comment at |
Contributor
|
ahh makes sense. Thank you for fixing this, I had missed it |
Member
|
I'll get this in soon |
Contributor
|
My effort to resolve the lockfile in the UI was a bad commit. Can you please resolve them locally? |
benjavicente
force-pushed
the
fix-angular-input
branch
from
b08d382 to
e2eb851
Compare
📝 WalkthroughWalkthrough
ChangesAngular pacer API and tooling
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/angular-pacer/src/queuer/injectQueuedValue.ts (1)
75-92:⚠️ Potential issue | 🟠 MajorThe 2-arg
initialValueoverload has a runtime detection bug.Line 77 never marks
hasInitialValuefor the 2-arg forminjectQueuedValue(signal, initialValue), so the initial value is treated as options (line 81) and skipped (line 91). This contradicts the overload declared at lines 49–60.While the codebase currently uses only the 3-arg form
injectQueuedValue(signal, options)with full test coverage, the dormant 2-arg overload exposes an API that breaks at runtime. Fix by either (a) correcting the detection logic to distinguish TValue from QueuerOptions, or (b) removing the unused 2-arg overload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/angular-pacer/src/queuer/injectQueuedValue.ts` around lines 75 - 92, The runtime detection for the 2-arg overload is wrong; change the hasInitialValue check to detect when the second argument (initialValueOrOptions) is present and is not a function (i.e. typeof initialValueOrOptions !== 'function') instead of inspecting initialOptionsOrSelector — update hasInitialValue to: (initialValueOrOptions !== undefined && typeof initialValueOrOptions !== 'function') || maybeSelector !== undefined, and keep the subsequent uses of initialOptions, selector, and queuedValue (symbols: initialOptionsOrSelector, maybeSelector, initialValueOrOptions, hasInitialValue, initialOptions, selector, queuedValue, injectQueuedValue) consistent with that corrected detection so the 2-arg overload treats the second param as the initial TValue rather than options.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/framework/angular/reference/interfaces/AsyncQueuedSignal.md`:
- Line 36: The addItem signature in the AsyncQueuedSignal interface should mark
the position and runOnItemsChange parameters as optional to match the
implementation; update the type signature for addItem to use position? and
runOnItemsChange? and update the corresponding parameter documentation entries
for position and runOnItemsChange in the AsyncQueuedSignal doc block so they
indicate optionality and align with packages/pacer/src/async-queuer.ts behavior.
In `@docs/framework/angular/reference/interfaces/QueuedSignal.md`:
- Line 36: Update the documented TypeScript signatures in the QueuedSignal
interface to mark parameters with defaults as optional: change addItem's
signature to reflect that position and runOnItemsChange are optional (with
defaults coming from packages/pacer/src/queuer.ts) and update the parameter
headers/descriptions accordingly; apply the same optional-parameter adjustments
to the other documented methods referenced around lines 58-64 so the docs match
the implementation in queuer.ts and clearly state default values for those
optional parameters.
In `@docs/framework/angular/reference/interfaces/QueuedValueSignal.md`:
- Around line 46-50: The example under the QueuedValueSignal interface uses the
wrong receiver name; update the usage examples to call queued.addItem(...)
instead of queuer.addItem(...). Locate the example block showing addItem and
replace calls to queuer.addItem with queued.addItem so the documented API
(QueuedValueSignal and its addItem method) matches actual usage.
In `@packages/angular-pacer/tsconfig.json`:
- Around line 3-5: The referenced tsconfig files (tsconfig.lib.json and
tsconfig.spec.json) are missing the required composite setting for TypeScript
project references; add "compilerOptions": { "composite": true } to both
tsconfig.lib.json and tsconfig.spec.json so they declare composite mode
(ensuring they can be referenced by the root tsconfig.json "references" array
and allowing tsc --build to succeed).
In `@packages/angular-pacer/tsconfig.spec.json`:
- Line 4: Update the test TypeScript config so it doesn't emit declaration
files: in tsconfig.spec.json change the compiler option "noEmit" from false to
true to override the root "declaration": true and prevent tsc from emitting
.d.ts during vitest runs; locate the "noEmit" setting in tsconfig.spec.json and
set it to true.
---
Outside diff comments:
In `@packages/angular-pacer/src/queuer/injectQueuedValue.ts`:
- Around line 75-92: The runtime detection for the 2-arg overload is wrong;
change the hasInitialValue check to detect when the second argument
(initialValueOrOptions) is present and is not a function (i.e. typeof
initialValueOrOptions !== 'function') instead of inspecting
initialOptionsOrSelector — update hasInitialValue to: (initialValueOrOptions !==
undefined && typeof initialValueOrOptions !== 'function') || maybeSelector !==
undefined, and keep the subsequent uses of initialOptions, selector, and
queuedValue (symbols: initialOptionsOrSelector, maybeSelector,
initialValueOrOptions, hasInitialValue, initialOptions, selector, queuedValue,
injectQueuedValue) consistent with that corrected detection so the 2-arg
overload treats the second param as the initial TValue rather than options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54576736-ac89-4730-81f5-a1148dfce5e4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.changeset/big-jobs-call.mddocs/framework/angular/reference/functions/injectQueuedValue.mddocs/framework/angular/reference/index.mddocs/framework/angular/reference/interfaces/AsyncQueuedSignal.mddocs/framework/angular/reference/interfaces/QueuedSignal.mddocs/framework/angular/reference/interfaces/QueuedValueSignal.mdexamples/angular/injectQueuedValue/src/app/app.htmlexamples/angular/injectQueuedValue/src/app/app.tsexamples/angular/injectQueuedValue/src/app/inputapp.tspackages/angular-pacer/eslint.config.jspackages/angular-pacer/package.jsonpackages/angular-pacer/src/queuer/injectQueuedValue.tspackages/angular-pacer/tests/queuer/injectQueuedValue.spec.tspackages/angular-pacer/tests/test-setup.tspackages/angular-pacer/tsconfig.base.jsonpackages/angular-pacer/tsconfig.docs.jsonpackages/angular-pacer/tsconfig.jsonpackages/angular-pacer/tsconfig.lib.jsonpackages/angular-pacer/tsconfig.spec.jsonpackages/angular-pacer/tsdown.config.tspackages/angular-pacer/vitest.config.ts
benjavicente
force-pushed
the
fix-angular-input
branch
from
e2eb851 to
55025ad
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Author
|
Rebased to newer changes and bumped some dependencies that had issues after the rebase. I added a small third thing: Now utilities use the type defined in the Angular package instead of the core one, so options like |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/angular-pacer/src/queuer/injectQueuedValue.ts`:
- Around line 76-82: The overload discriminator misdetects the two-argument form
of injectQueuedValue: change the hasInitialValue logic so the second argument
(initialOptionsOrSelector) is treated as an initialValue when maybeSelector is
undefined and the second arg does not look like AngularQueuerOptions; implement
a small type-guard (e.g., isAngularQueuerOptions) that checks for known option
keys (bufferSize/delay/etc.) and use it in the condition for hasInitialValue and
the subsequent selection of initialOptions vs initialValue (references:
hasInitialValue, initialOptionsOrSelector, maybeSelector, initialValueOrOptions,
selector, injectQueuedValue); apply the same discriminator fix to the analogous
block at lines 88–91 so two-argument calls use the provided initialValue rather
than treating it as options.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d03d4784-22b2-4e46-8b8e-5a9f8dcc1533
📒 Files selected for processing (41)
.changeset/big-jobs-call.mddocs/framework/angular/reference/functions/injectAsyncBatchedCallback.mddocs/framework/angular/reference/functions/injectAsyncDebouncedCallback.mddocs/framework/angular/reference/functions/injectAsyncQueuedSignal.mddocs/framework/angular/reference/functions/injectAsyncRateLimitedCallback.mddocs/framework/angular/reference/functions/injectAsyncThrottledCallback.mddocs/framework/angular/reference/functions/injectBatchedCallback.mddocs/framework/angular/reference/functions/injectDebouncedCallback.mddocs/framework/angular/reference/functions/injectDebouncedSignal.mddocs/framework/angular/reference/functions/injectDebouncedValue.mddocs/framework/angular/reference/functions/injectQueuedSignal.mddocs/framework/angular/reference/functions/injectQueuedValue.mddocs/framework/angular/reference/functions/injectRateLimitedCallback.mddocs/framework/angular/reference/functions/injectRateLimitedSignal.mddocs/framework/angular/reference/functions/injectRateLimitedValue.mddocs/framework/angular/reference/functions/injectThrottledCallback.mddocs/framework/angular/reference/functions/injectThrottledSignal.mddocs/framework/angular/reference/functions/injectThrottledValue.mddocs/framework/angular/reference/index.mddocs/framework/angular/reference/interfaces/AsyncQueuedSignal.mddocs/framework/angular/reference/interfaces/QueuedSignal.mddocs/framework/angular/reference/interfaces/QueuedValueSignal.mdexamples/angular/injectQueuedValue/src/app/app.htmlexamples/angular/injectQueuedValue/src/app/app.tsexamples/angular/injectQueuedValue/src/app/inputapp.tspackages/angular-pacer/eslint.config.jspackages/angular-pacer/package.jsonpackages/angular-pacer/src/async-batcher/injectAsyncBatchedCallback.tspackages/angular-pacer/src/async-debouncer/injectAsyncDebouncedCallback.tspackages/angular-pacer/src/async-queuer/injectAsyncQueuedSignal.tspackages/angular-pacer/src/async-rate-limiter/injectAsyncRateLimitedCallback.tspackages/angular-pacer/src/async-throttler/injectAsyncThrottledCallback.tspackages/angular-pacer/src/batcher/injectBatchedCallback.tspackages/angular-pacer/src/debouncer/injectDebouncedCallback.tspackages/angular-pacer/src/debouncer/injectDebouncedSignal.tspackages/angular-pacer/src/debouncer/injectDebouncedValue.tspackages/angular-pacer/src/queuer/injectQueuedSignal.tspackages/angular-pacer/src/queuer/injectQueuedValue.tspackages/angular-pacer/src/rate-limiter/injectRateLimitedCallback.tspackages/angular-pacer/src/rate-limiter/injectRateLimitedSignal.tspackages/angular-pacer/src/rate-limiter/injectRateLimitedValue.ts
✅ Files skipped from review due to trivial changes (9)
- packages/angular-pacer/src/debouncer/injectDebouncedCallback.ts
- docs/framework/angular/reference/index.md
- docs/framework/angular/reference/functions/injectAsyncRateLimitedCallback.md
- .changeset/big-jobs-call.md
- docs/framework/angular/reference/functions/injectAsyncQueuedSignal.md
- examples/angular/injectQueuedValue/src/app/inputapp.ts
- examples/angular/injectQueuedValue/src/app/app.ts
- docs/framework/angular/reference/functions/injectAsyncThrottledCallback.md
- docs/framework/angular/reference/functions/injectRateLimitedValue.md
🚧 Files skipped from review as they are similar to previous changes (6)
- examples/angular/injectQueuedValue/src/app/app.html
- packages/angular-pacer/package.json
- docs/framework/angular/reference/interfaces/QueuedSignal.md
- docs/framework/angular/reference/interfaces/AsyncQueuedSignal.md
- packages/angular-pacer/eslint.config.js
- docs/framework/angular/reference/functions/injectQueuedValue.md
Comment on lines
76
to
82
| const hasInitialValue = | ||
| (initialOptionsOrSelector !== undefined && !hasSelector) || | ||
| maybeSelector !== undefined | ||
|
|
||
| const initialValue = hasInitialValue | ||
| ? (initialValueOrOptions as TValue) | ||
| : value() | ||
| const initialOptions = hasInitialValue | ||
| ? (initialOptionsOrSelector as QueuerOptions<TValue>) | ||
| : (initialValueOrOptions as QueuerOptions<TValue>) | ||
| ? (initialOptionsOrSelector as AngularQueuerOptions<TValue, TSelected>) | ||
| : (initialValueOrOptions as AngularQueuerOptions<TValue, TSelected>) | ||
| const selector = hasInitialValue |
There was a problem hiding this comment.
Two-argument initialValue overload is misparsed and ignored.
hasInitialValue is never true for injectQueuedValue(value, initialValue) (no arg3/arg4), so the second argument is treated as options and the linked signal initializes from value() instead of initialValue.
Suggested discriminator fix
- const hasInitialValue =
- (initialOptionsOrSelector !== undefined && !hasSelector) ||
- maybeSelector !== undefined
+ const hasInitialValue =
+ maybeSelector !== undefined ||
+ (initialOptionsOrSelector !== undefined && !hasSelector) ||
+ // 2-arg form: injectQueuedValue(value, initialValue)
+ (initialOptionsOrSelector === undefined &&
+ (initialValueOrOptions === null ||
+ typeof initialValueOrOptions !== 'object'))Also applies to: 88-91
🤖 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 `@packages/angular-pacer/src/queuer/injectQueuedValue.ts` around lines 76 - 82,
The overload discriminator misdetects the two-argument form of
injectQueuedValue: change the hasInitialValue logic so the second argument
(initialOptionsOrSelector) is treated as an initialValue when maybeSelector is
undefined and the second arg does not look like AngularQueuerOptions; implement
a small type-guard (e.g., isAngularQueuerOptions) that checks for known option
keys (bufferSize/delay/etc.) and use it in the condition for hasInitialValue and
the subsequent selection of initialOptions vs initialValue (references:
hasInitialValue, initialOptionsOrSelector, maybeSelector, initialValueOrOptions,
selector, injectQueuedValue); apply the same discriminator fix to the analogous
block at lines 88–91 so two-argument calls use the provided initialValue rather
than treating it as options.
Author
|
Regarding CoreRabbit comment: it looks like |
benjavicente
force-pushed
the
fix-angular-input
branch
from
55025ad to
cf80673
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/framework/angular/reference/functions/injectAsyncQueuedSignal.md`:
- Line 26: Update the queued.queuer descriptions in
docs/framework/angular/reference/functions/injectAsyncQueuedSignal.md:26-26 and
docs/framework/angular/reference/functions/injectQueuedSignal.md:26-26, changing
“additional control method” to “additional control methods” in both locations.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5a14c49-1770-4a0d-9e77-427530e2da84
📒 Files selected for processing (4)
docs/framework/angular/reference/functions/injectAsyncQueuedSignal.mddocs/framework/angular/reference/functions/injectQueuedSignal.mdpackages/angular-pacer/src/async-queuer/injectAsyncQueuedSignal.tspackages/angular-pacer/src/queuer/injectQueuedSignal.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/angular-pacer/src/queuer/injectQueuedSignal.ts
- packages/angular-pacer/src/async-queuer/injectAsyncQueuedSignal.ts
| - `queued()`: Get the current queue items as an array | ||
| - `queued.addItem(...)`: Add an item to the queue | ||
| - `queued.queue`: The queuer instance with additional control methods | ||
| - `queued.queuer`: The queuer instance with additional control methods |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the repeated singular wording.
Both descriptions list multiple controls (start() and stop()) but say “additional control method.”
docs/framework/angular/reference/functions/injectAsyncQueuedSignal.md#L26-L26: change “additional control method” to “additional control methods.”docs/framework/angular/reference/functions/injectQueuedSignal.md#L26-L26: make the same pluralization correction.
🧰 Tools
🪛 LanguageTool
[grammar] ~26-~26: Ensure spelling is correct
Context: ...tem to the queue - queued.queuer: The queuer instance with additional control method...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
📍 Affects 2 files
docs/framework/angular/reference/functions/injectAsyncQueuedSignal.md#L26-L26(this comment)docs/framework/angular/reference/functions/injectQueuedSignal.md#L26-L26
🤖 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 `@docs/framework/angular/reference/functions/injectAsyncQueuedSignal.md` at
line 26, Update the queued.queuer descriptions in
docs/framework/angular/reference/functions/injectAsyncQueuedSignal.md:26-26 and
docs/framework/angular/reference/functions/injectQueuedSignal.md:26-26, changing
“additional control method” to “additional control methods” in both locations.
Source: Linters/SAST tools
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
3 participants
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.
🎯 Changes
This PR does 2 things, both in
injectQueuedValue:valuecould be called on component initialization, causinginjectQueuedValueto throw. I added a tests first to demonstrate the issue and then I added a commit to fix it. There was an example that tested against that error, but where the path where of thevalue()call wasn't reached (maybeSelectoris not undefined).pacer/examples/angular/injectQueuedValue/src/app/inputapp.ts
Lines 18 to 21 in b376c24
pacer/packages/angular-pacer/src/queuer/injectQueuedValue.ts
Lines 60 to 75 in b376c24
The
injectQueuedValuewas returning an array instead of a queued value. I guess that was a mistake, so I aligned the function to behave similar to the React api.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
injectQueuedValuenow returns a callable queued-value object (QueuedValueSignal) withaddItem()and aqueuercontrol handle (instead of the previous return shape).injectQueuedValue(including thequeuer-based queue-length access) and added documentation forQueuedValueSignal.injectQueuedValuecoverage for queueing behavior, timing, and Angular initialization.