feat(date-picker): add date-picker - #55
Conversation
WalkthroughAdded Angular date-picker and date-range-picker primitives with value-accessor support, themed styles, package exports, dependency updates, documentation, previews, and a range-picker example. ChangesDate Picker Feature
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DocumentationPreview
participant MgnpDatePicker
participant MgnpDatePickerDateButton
participant NgpDatePicker
DocumentationPreview->>MgnpDatePicker: Render date picker
MgnpDatePicker->>NgpDatePicker: Provide picker state
DocumentationPreview->>MgnpDatePickerDateButton: Render date button
MgnpDatePickerDateButton->>NgpDatePicker: Delegate date selection
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
package.jsonOops! Something went wrong! :( ESLint: 9.39.4 TypeError: Error while loading rule ' packages/ng-primitives/package.jsonOops! Something went wrong! :( ESLint: 9.39.4 TypeError: Error while loading rule ' 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: 2
🤖 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/ng-primitives/_theme/components/date-picker.css`:
- Around line 11-25: Update the mgnp-date-picker-date-button styles to set an
explicit non-inline display value, such as flex, so size-full covers the entire
date cell. Apply the same display correction to the date target and
range-background styling referenced by this date-picker component.
In
`@packages/ng-primitives/date-picker/src/lib/mgnp-date-range-picker/mgnp-date-range-picker.ts`:
- Around line 17-52: Implement ControlValueAccessor on MgnpDateRangePicker to
match the existing provideValueAccessor registration: add writeValue,
registerOnChange, registerOnTouched, and setDisabledState, and connect them to
the injected state and range change outputs so form values, touch status, and
disabled state stay synchronized. If form support is not intended, remove
provideValueAccessor instead.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 27767583-99db-4e97-96ad-8140a78b6f5a
📒 Files selected for processing (24)
apps/documentation/src/app/components/app-quick-links.tsapps/documentation/src/app/directives/heading-anchor.tsapps/documentation/src/content/documentation/ng-primitives/date-picker.mdapps/documentation/src/examples/date-picker/range-picker.tsapps/documentation/src/previews/date-picker.tseslint.config.mjspackages/ng-primitives/_theme/components/date-picker.csspackages/ng-primitives/_theme/components/date-picker/color.csspackages/ng-primitives/_theme/mgnp.csspackages/ng-primitives/date-picker/README.mdpackages/ng-primitives/date-picker/ng-package.jsonpackages/ng-primitives/date-picker/src/index.tspackages/ng-primitives/date-picker/src/lib/mgnp-date-picker-cell-render/mgnp-date-picker-cell-render.tspackages/ng-primitives/date-picker/src/lib/mgnp-date-picker-cell/mgnp-date-picker-cell.tspackages/ng-primitives/date-picker/src/lib/mgnp-date-picker-date-button/mgnp-date-picker-date-button.tspackages/ng-primitives/date-picker/src/lib/mgnp-date-picker-grid/mgnp-date-picker-grid.tspackages/ng-primitives/date-picker/src/lib/mgnp-date-picker-header/mgnp-date-picker-header.tspackages/ng-primitives/date-picker/src/lib/mgnp-date-picker-label/mgnp-date-picker-label.tspackages/ng-primitives/date-picker/src/lib/mgnp-date-picker-next-month/mgnp-date-picker-next-month.tspackages/ng-primitives/date-picker/src/lib/mgnp-date-picker-previous-month/mgnp-date-picker-previous-month.tspackages/ng-primitives/date-picker/src/lib/mgnp-date-picker-row-render/mgnp-date-picker-row-render.tspackages/ng-primitives/date-picker/src/lib/mgnp-date-picker/mgnp-date-picker.tspackages/ng-primitives/date-picker/src/lib/mgnp-date-range-picker/mgnp-date-range-picker.tstsconfig.base.json
💤 Files with no reviewable changes (2)
- eslint.config.mjs
- apps/documentation/src/app/directives/heading-anchor.ts
| providers: [provideDateRangePickerState(), provideValueAccessor(MgnpDateRangePicker)], | ||
| host: { | ||
| class: 'mgnp-date-range-picker mgnp-c-date-range-picker', | ||
| 'data-mgnp-date-range-picker': '', | ||
| '[attr.data-mgnp-date-picker-color]': 'color()', | ||
| }, | ||
| hostDirectives: [ | ||
| { | ||
| directive: NgpDateRangePicker, | ||
| inputs: [ | ||
| 'ngpDateRangePickerMin:mgnpDateRangePickerMin', | ||
| 'ngpDateRangePickerMax:mgnpDateRangePickerMax', | ||
| 'ngpDateRangePickerDisabled:mgnpDateRangePickerDisabled', | ||
| 'ngpDateRangePickerDateDisabled:mgnpDateRangePickerDateDisabled', | ||
| 'ngpDateRangePickerFirstDayOfWeek:mgnpDateRangePickerFirstDayOfWeek', | ||
| 'ngpDateRangePickerStartDate:mgnpDateRangePickerStartDate', | ||
| 'ngpDateRangePickerDefaultStartDate:mgnpDateRangePickerDefaultStartDate', | ||
| 'ngpDateRangePickerEndDate:mgnpDateRangePickerEndDate', | ||
| 'ngpDateRangePickerDefaultEndDate:mgnpDateRangePickerDefaultEndDate', | ||
| 'ngpDateRangePickerFocusedDate:mgnpDateRangePickerFocusedDate', | ||
| 'ngpDateRangePickerDefaultFocusedDate:mgnpDateRangePickerDefaultFocusedDate', | ||
| ], | ||
| outputs: [ | ||
| 'ngpDateRangePickerStartDateChange:mgnpDateRangePickerStartDateChange', | ||
| 'ngpDateRangePickerEndDateChange:mgnpDateRangePickerEndDateChange', | ||
| 'ngpDateRangePickerFocusedDateChange:mgnpDateRangePickerFocusedDateChange', | ||
| ], | ||
| }, | ||
| ], | ||
| exportAs: 'mgnpDateRangePicker', | ||
| }) | ||
| export class MgnpDateRangePicker<T = Date> { | ||
| readonly state = injectDateRangePickerState<T>(); | ||
|
|
||
| readonly color = input<MgnpDateRangePickerColor>('ui'); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Compare the registered range accessor with the implemented date accessor.
ast-grep outline packages/ng-primitives/date-picker/src/lib/mgnp-date-picker/mgnp-date-picker.ts --items all
ast-grep outline packages/ng-primitives/date-picker/src/lib/mgnp-date-range-picker/mgnp-date-range-picker.ts --items all
rg -n -C 4 \
'provideValueAccessor|implements ControlValueAccessor|writeValue|registerOnChange|registerOnTouched|setDisabledState|DateChange' \
packages/ng-primitives/date-picker/src/libRepository: MGREMY/nx_source
Length of output: 10907
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== providers/utils files =="
fd -a 'providers|value.*accessor|utils' packages/ng-primitives | sed 's#^\./##' | head -50
echo "== locate provideValueAccessor =="
rg -n "function provideValueAccessor|export function provideValueAccessor|provideValueAccessor|NG_VALUE_ACCESSOR|ValueAccessor" packages/ng-primitives
echo "== date picker files with lines =="
sed -n '1,120p' packages/ng-primitives/date-picker/src/lib/mgnp-date-picker/mgnp-date-picker.ts
echo "---"
sed -n '1,90p' packages/ng-primitives/date-picker/src/lib/mgnp-date-range-picker/mgnp-date-range-picker.tsRepository: MGREMY/nx_source
Length of output: 9802
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package declarations =="
rg -n '"exports"|"ng-primitives/utils"|"date-picker"|provideValueAccessor|export const provideValueAccessor|export interface|export type (ChangeFn|TouchedFn)|NG_VALUE_ACCESSOR' package.json packages -g "package.json" -g "*.ts" | head -200
echo "== candidate source files =="
git ls-files packages | rg 'utils|value.*accessor|providers|date-range' | head -100
echo "== all provideValueAccessor definitions/usages across repo =="
rg -n -C 3 "provideValueAccessor|NG_VALUE_ACCESSOR" .Repository: MGREMY/nx_source
Length of output: 19921
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package json relevant:"
cat packages/ng-primitives/package.json
echo "== candidate source files exact:"
git ls-files packages/ng-primitives | rg '(^packages/ng-primitives/(utils|date-picker)|value|providers)'
echo "== direct source search for function definitions:"
rg -n -C 3 "provideValueAccessor|function provideValueAccessor|export .*provideValueAccessor" packages/ng-primitives -g "*.ts" || true
echo "== direct source search for Angular value accessor APIs:"
rg -n -C 2 "NG_VALUE_ACCESSOR|ValueAccessor|ControlValueAccessor|ChangeFn|TouchedFn" packages/ng-primitives -g "*.ts" || trueRepository: MGREMY/nx_source
Length of output: 50372
Implement the registered range value accessor.
provideValueAccessor(MgnpDateRangePicker) registers this directive for Angular form value binding, but the class does not implement ControlValueAccessor. Add writeValue, registerOnChange, registerOnTouched, and setDisabledState, then tie writeValue / change events to the range state, or remove the accessor if this directive should not support forms.
🤖 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/ng-primitives/date-picker/src/lib/mgnp-date-range-picker/mgnp-date-range-picker.ts`
around lines 17 - 52, Implement ControlValueAccessor on MgnpDateRangePicker to
match the existing provideValueAccessor registration: add writeValue,
registerOnChange, registerOnTouched, and setDisabledState, and connect them to
the injected state and range change outputs so form values, touch status, and
disabled state stay synchronized. If form support is not intended, remove
provideValueAccessor instead.
There was a problem hiding this comment.
@MGREMY, acknowledged. The finding depends on ng-primitives/ng-primitives#897.
I will keep it unresolved until that PR merges. Please update this thread after the upstream change is available.
You are interacting with an AI system.
There was a problem hiding this comment.
How would you implement this ? Since this must be a value accessor for two controls technically. Wouldn't it be good to require a group of two controls (like startDate, endDate) and manage them inside the component, or could this be done in another way ?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== upstream PR 897 metadata =='
gh pr view 897 --repo ng-primitives/ng-primitives \
--json number,title,state,body,files,url
echo '== upstream PR 897 changed-file patches =='
gh pr diff 897 --repo ng-primitives/ng-primitives --patch
echo '== local picker outlines =='
ast-grep outline packages/ng-primitives/date-picker/src/lib/mgnp-date-picker/mgnp-date-picker.ts --items all
ast-grep outline packages/ng-primitives/date-picker/src/lib/mgnp-date-range-picker/mgnp-date-range-picker.ts --items all
echo '== local picker and example form-related code =='
rg -n -C 5 \
'class MgnpDatePicker|class MgnpDateRangePicker|ControlValueAccessor|writeValue|registerOnChange|registerOnTouched|setDisabledState|mgnpDateRangePicker|mgnpDatePicker' \
packages/ng-primitives/date-picker apps/documentation/src/examples/date-pickerLength of output: 52055
@MGREMY, use one FormControl for one range value. Do not require a parent FormGroup.
A ControlValueAccessor connects one host element to one Angular form control. The date-range picker is one UI control, even though it contains two dates. Its form value should be an atomic range value, for example:
export interface MgnpDateRange<T> {
start: T | undefined;
end: T | undefined;
}Then consumers use:
range = new FormControl<MgnpDateRange<Date>>({
start: undefined,
end: undefined,
});This keeps the control reusable with both reactive forms and template-driven forms. It also lets validators operate on the range as a whole, such as start <= end.
After ng-primitives/ng-primitives#897 merges, I would implement the value accessor as follows:
writeValue(value)callsstate.setStart(value?.start, { emit: false })andstate.setEnd(value?.end, { emit: false }).- Subscribe to both
state().startDateChangeandstate().endDateChange. - On either event, call
onChangewith the current complete value:{ start: this.state().startDate(), end: this.state().endDate(), }
setDisabledState(isDisabled)callsstate().setDisabled(isDisabled).- Mark the control touched on a user interaction or when focus leaves the picker. Do not mark it touched from
writeValue.
The update methods must suppress emitted events during writeValue. Otherwise, a form model update can feed back into onChange.
A two-control FormGroup is still useful at the application level:
new FormGroup({
startDate: new FormControl<Date | undefined>(),
endDate: new FormControl<Date | undefined>(),
});However, that should be an optional adapter or application-owned composition. The picker directive should not require or mutate a parent FormGroup. A directive cannot cleanly act as two independent ControlValueAccessor instances on the same host element.
Until #897 merges, remove provideValueAccessor(MgnpDateRangePicker) or defer the range-picker form integration. The current registration advertises Angular forms support but cannot implement writeValue correctly without the upstream setters.
You are interacting with an AI system.
There was a problem hiding this comment.
@MGREMY, use a type alias here.
The range value is a small value object. It is not designed for declaration merging or consumer extension. A type also keeps the option open to model partial or complete ranges with unions later.
export type MgnpDateRange<T> = {
start: T | null;
end: T | null;
};Use the date-picker library’s existing range type instead if it exports one. Do not introduce a duplicate public type when the upstream API already defines the range contract.
Use an interface only if you intend consumers to extend the range contract through inheritance or declaration merging. Neither is needed for a ControlValueAccessor.
You are interacting with an AI system.
|
Waiting for this PR |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/ng-primitives/date-picker/src/lib/mgnp-date-range-picker/mgnp-date-range-picker.ts`:
- Around line 60-68: Rename the lifecycle method in the date-range picker from
contructor to constructor so Angular executes the subscription setup during
directive creation. Preserve both startDateChange and endDateChange
subscriptions and their existing onChangeFn callback behavior.
- Line 25: Replace the direct onTouchedFn call in the host focusout binding with
an onHandleFocusOut($event) method on mgnpDateRangePicker. In that method,
invoke onTouchedFn only when event.relatedTarget is not contained within the
host element, preserving focus transitions between internal range-date buttons
without marking the picker touched.
- Around line 70-72: Update MgnpDateRangePicker.writeValue to handle null or
undefined values by clearing both range endpoints instead of dereferencing the
value; retain the existing start/end assignments for non-null MgnpDateRange
values.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f7a9f39-60fa-4e23-b0a0-ef972e3ea250
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
package.jsonpackages/ng-primitives/_theme/components/date-picker.csspackages/ng-primitives/_theme/components/date-picker/color.csspackages/ng-primitives/date-picker/src/index.tspackages/ng-primitives/date-picker/src/lib/mgnp-date-range-picker/mgnp-date-range-picker.tspackages/ng-primitives/date-picker/src/lib/mgnp-date-range.tspackages/ng-primitives/package.jsonpnpm-workspace.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/ng-primitives/_theme/components/date-picker.css
- packages/ng-primitives/date-picker/src/index.ts
| class: 'mgnp-date-range-picker mgnp-c-date-range-picker', | ||
| 'data-mgnp-date-range-picker': '', | ||
| '[attr.data-mgnp-date-picker-color]': 'color()', | ||
| '(focusout)': 'onTouchedFn?.()', |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 \
'\(focusout\)|onTouchedFn|mgnpDateRangePicker|mgnpDatePickerDateButton' \
packages/ng-primitives/date-picker/src/lib -g '*.ts' || trueRepository: MGREMY/nx_source
Length of output: 13192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files 'packages/ng-primitives/date-picker/src/lib/mgnp-date-%picker.ts' 'packages/ng-primitives/date-picker/src/lib/*' | sed -n '1,200p'
printf '\nRange picker outline:\n'
ast-grep outline packages/ng-primitives/date-picker/src/lib/mgnp-date-range-picker/mgnp-date-range-picker.ts --view expanded || true
printf '\nDate picker outline:\n'
ast-grep outline packages/ng-primitives/date-picker/src/lib/mgnp-date-picker/mgnp-date-picker.ts --view expanded || true
printf '\nDate button outline:\n'
ast-grep outline packages/ng-primitives/date-picker/src/lib/mgnp-date-picker-date-button/mgnp-date-picker-date-button.ts --view expanded || true
printf '\nRelevant contents:\n'
cat -n packages/ng-primitives/date-picker/src/lib/mgnp-date-range-picker/mgnp-date-range-picker.ts | sed -n '1,110p'
printf '\n--- date picker ---\n'
cat -n packages/ng-primitives/date-picker/src/lib/mgnp-date-picker/mgnp-date-picker.ts | sed -n '1,110p'
printf '\n--- date button ---\n'
cat -n packages/ng-primitives/date-picker/src/lib/mgnp-date-picker-date-button/mgnp-date-picker-date-button.ts | sed -n '1,80p'Repository: MGREMY/nx_source
Length of output: 11037
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Search for NgpDateRangePicker / NgpDatePicker symbols:\n'
rg -n 'class Ngp(DateRangePicker|DatePicker)|export .*Ngp(DateRangePicker|DatePicker)|NgpDateRangePicker|NgpDatePicker' packages -g '*.ts' || true
printf '\nFocusout/touched handlers in repo:\n'
rg -n -C 3 'focusout|registerOnTouched|onTouchedFn|nextElement|document\.activeElement|contains\(.*focus' packages -g '*.ts' || true
printf '\nBehavioral probe for focusout event target vs current target:\n'
cat <<'EOF' > /tmp/focusout_probe.html
<!DOCTYPE html>
<meta charset="utf-8">
<div id="host" tabindex="0" style="border:1px solid black;padding:10px">
<button id="button1">1</button>
<button id="button2">2</button>
</div>
<script>
const log = [];
document.getElementById('host').addEventListener('focusout', (e) => {
log.push({
currentTargetOutside: !e.currentTarget.contains(e.relatedTarget),
currentTargetContainsRelatedTarget: e.currentTarget.contains(e.relatedTarget),
relatedTargetId: e.relatedTarget?.id ?? null
});
});
document.getElementById('button1').focus();
document.getElementById('button2').focus();
document.body.style.position = 'absolute';
document.body.style.left = '9999px';
document.body.focus();
document.getElementById('host').removeEventListener('focusout', () => {});
console.log(JSON.stringify(log));
</script>
EOF
# If a headless browser is unavailable, print the DOM-only representation of the same event semantics expected by Element.contains().
set +e
docker run --rm --network none alpine echo "docker unavailable" >/dev/null 2>&1 || true
printf '\nProbe file exists at /tmp/focusout_probe.html\n'Repository: MGREMY/nx_source
Length of output: 21989
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Ngp* classes definitions:\n'
rg -n -C 6 '`@Directive`|class NgpDateRangePicker|class NgpDatePicker|export interface NgpDateRangePicker|export interface NgpDatePicker' packages/ng-primitives -g '*.ts' | sed -n '1,260p'
printf '\nCalendar/grid/focus files:\n'
git ls-files packages/ng-primitives/date-picker/src/lib | grep -E 'cell|grid|row|month|button|range' | sed -n '1,200p'
printf '\nDate cell implementation:\n'
cat -n packages/ng-primitives/date-picker/src/lib/mgnp-date-picker-cell/mgnp-date-picker-cell.ts | sed -n '1,120p'
printf '\nDate range implementations:\n'
for f in packages/ng-primitives/date-picker/src/lib/mgnp-date-range*.ts; do
echo "--- $f"
cat -n "$f" | sed -n '1,160p'
doneRepository: MGREMY/nx_source
Length of output: 30586
Only call registerOnTouched when focus leaves the picker.
focusout also fires when focus moves between range-date buttons inside mgnpDateRangePicker, and the current listener immediately calls onTouchedFn. Add a host method that invokes the callback only when event.relatedTarget is not contained by the host element, and bind '(focusout)': 'onHandleFocusOut($event)'.
🤖 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/ng-primitives/date-picker/src/lib/mgnp-date-range-picker/mgnp-date-range-picker.ts`
at line 25, Replace the direct onTouchedFn call in the host focusout binding
with an onHandleFocusOut($event) method on mgnpDateRangePicker. In that method,
invoke onTouchedFn only when event.relatedTarget is not contained within the
host element, preserving focus transitions between internal range-date buttons
without marking the picker touched.
| contructor() { | ||
| this.state() | ||
| .startDateChange.pipe(takeUntilDestroyed()) | ||
| .subscribe((value) => this.onChangeFn?.({ start: value, end: this.state().endDate() })); | ||
|
|
||
| this.state() | ||
| .endDateChange.pipe(takeUntilDestroyed()) | ||
| .subscribe((value) => this.onChangeFn?.({ start: this.state().startDate(), end: value })); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='packages/ng-primitives/date-picker/src/lib/mgnp-date-range-picker/mgnp-date-range-picker.ts'
ast-grep outline "$FILE" --items all
ast-grep run --lang typescript --pattern 'contructor() { $$$ }' "$FILE"Repository: MGREMY/nx_source
Length of output: 1169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='packages/ng-primitives/date-picker/src/lib/mgnp-date-range-picker/mgnp-date-range-picker.ts'
printf '--- relevant file lines ---\n'
sed -n '55,75p' "$FILE" | cat -n
printf '\n--- contructor occurrences ---\n'
rg -n 'contructor|constructor' "$FILE" || true
printf '\n--- behavioral probe: TypeScript class constructor vs normal method ---\n'
node - <<'JS'
console.log('TypeScript constructor names are checked by the TypeScript compiler; a class method contructor is not equivalent to constructor.');
JSRepository: MGREMY/nx_source
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='packages/ng-primitives/date-picker/src/lib/mgnp-date-range-picker/mgnp-date-range-picker.ts'
echo '--- relevant file lines ---'
sed -n '55,75p' "$FILE" | cat -n
echo
echo '--- contructor occurrences ---'
rg -n 'contructor|constructor' "$FILE" || true
echo
echo '--- behavioral probe: TS constructor name rule ---'
node - <<'JS'
console.log('TypeScript class constructors must be named constructor; a class method named contructor is not a constructor.');
JSRepository: MGREMY/nx_source
Length of output: 1227
Rename contructor to constructor.
contructor is parsed as an ordinary method, so this subscription setup never runs when Angular creates the directive. Because the change callbacks are not subscribed, user date changes do not call the registered form change callback.
Proposed fix
- contructor() {
+ constructor() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| contructor() { | |
| this.state() | |
| .startDateChange.pipe(takeUntilDestroyed()) | |
| .subscribe((value) => this.onChangeFn?.({ start: value, end: this.state().endDate() })); | |
| this.state() | |
| .endDateChange.pipe(takeUntilDestroyed()) | |
| .subscribe((value) => this.onChangeFn?.({ start: this.state().startDate(), end: value })); | |
| } | |
| constructor() { | |
| this.state() | |
| .startDateChange.pipe(takeUntilDestroyed()) | |
| .subscribe((value) => this.onChangeFn?.({ start: value, end: this.state().endDate() })); | |
| this.state() | |
| .endDateChange.pipe(takeUntilDestroyed()) | |
| .subscribe((value) => this.onChangeFn?.({ start: this.state().startDate(), end: value })); | |
| } |
🤖 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/ng-primitives/date-picker/src/lib/mgnp-date-range-picker/mgnp-date-range-picker.ts`
around lines 60 - 68, Rename the lifecycle method in the date-range picker from
contructor to constructor so Angular executes the subscription setup during
directive creation. Preserve both startDateChange and endDateChange
subscriptions and their existing onChangeFn callback behavior.
| writeValue(value: MgnpDateRange<T>): void { | ||
| this.state().setStart(value.start); | ||
| this.state().setEnd(value.end); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 \
'new FormControl|\.reset\(|writeValue\(value:' \
packages apps -g '*.ts' || trueRepository: MGREMY/nx_source
Length of output: 9584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## candidate files"
fd -a 'mgnp-date-range-picker\.ts|date-range' packages/ng-primitives/date-picker || true
echo "## file outline"
ast-grep outline packages/ng-primitives/date-picker/src/lib/mgnp-date-range-picker/mgnp-date-range-picker.ts --view expanded || true
echo "## relevant source"
cat -n packages/ng-primitives/date-picker/src/lib/mgnp-date-range-picker/mgnp-date-range-picker.ts
echo "## date picker related usages and types"
rg -n -C 2 'MgnpDateRange|--date-range|dateRange|writeValue' packages/ng-primitives/date-picker -g '*.ts' || trueRepository: MGREMY/nx_source
Length of output: 24203
🌐 Web query:
Angular ControlValueAccessor writeValue allowed null value documentation
💡 Result:
In Angular, the ControlValueAccessor interface's writeValue(obj: any) method is designed to accept any value, including null [1][2]. The official Angular documentation does not explicitly forbid or warn against null values; rather, it defines the parameter type as any [1][3]. It is important to understand that your implementation of writeValue may receive a null value for several reasons: 1. Initial State: Form controls are often initialized with a default value of null, which triggers a call to writeValue [4][5][6]. 2. Form Resets: Calling the reset method on a FormControl will set its value to null, which also triggers writeValue [6]. 3. Lifecycle Behavior: Depending on the specific Angular version and the interaction between directives (such as NgModel or Reactive Forms), you may encounter scenarios where writeValue is called with null during initialization [4][7]. Because null is a valid value in the Angular forms system, your implementation of writeValue should be written defensively to handle it gracefully [4][6]. Ignoring null values entirely can cause issues, as a null value may represent a legitimate intentional state (e.g., clearing a selection or resetting a form) that your component needs to reflect in the UI [4]. Always ensure your component logic correctly interprets or sanitizes incoming null values to avoid errors or unexpected visual states [6].
Citations:
- 1: https://angular.dev/api/forms/ControlValueAccessor
- 2: https://github.com/angular/angular/blob/main/packages/forms/src/directives/control_value_accessor.ts
- 3: https://github.com/angular/angular/blob/e6953793/packages/forms/src/directives/control_value_accessor.ts
- 4: ValueAccessor.writeValue is being called twice, first time with a phantom null value angular/angular#14988
- 5: ValueAccessor.writeValue is being called twice, first time with a phantom null value angular/angular#14988
- 6: https://valor-software.com/articles/avoiding-common-pitfalls-with-controlvalueaccessors-in-angular
- 7: fix(forms): ValueAccessor.writeValue(...) sometimes called with null angular/angular#38140
Handle null values in MgnpDateRangePicker.writeValue.
Angular forms can pass null to ControlValueAccessor.writeValue during initialization or after reset(). The current implementation dereferences value.start and value.end unconditionally and can throw. Clear both range endpoints when writeValue receives null or undefined.
Proposed fix
- writeValue(value: MgnpDateRange<T>): void {
- this.state().setStart(value.start);
- this.state().setEnd(value.end);
+ writeValue(value: MgnpDateRange<T> | null | undefined): void {
+ this.state().setStart(value?.start);
+ this.state().setEnd(value?.end);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| writeValue(value: MgnpDateRange<T>): void { | |
| this.state().setStart(value.start); | |
| this.state().setEnd(value.end); | |
| writeValue(value: MgnpDateRange<T> | null | undefined): void { | |
| this.state().setStart(value?.start); | |
| this.state().setEnd(value?.end); | |
| } |
🤖 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/ng-primitives/date-picker/src/lib/mgnp-date-range-picker/mgnp-date-range-picker.ts`
around lines 70 - 72, Update MgnpDateRangePicker.writeValue to handle null or
undefined values by clearing both range endpoints instead of dereferencing the
value; retain the existing start/end assignments for non-null MgnpDateRange
values.
1 participant
Summary by CodeRabbit