Appointment Form Redesign: Implement Recurrence form by sjbur · Pull Request #31342 · DevExpress/DevExtreme

sjbur · 2025-10-14T16:42:24Z

No description provided.

update pom testcafe tests use legacyForm option fix testcafe tests

Signed-off-by: Eldar Iusupzhanov <84278206+Tucchhaa@users.noreply.github.com>

Copilot

Pull Request Overview

Copilot reviewed 15 out of 129 changed files in this pull request and generated 3 comments.

Copilot · 2025-10-23T09:23:34Z

+  getRules(): any {
+    return this._recurrenceRule;
+  }


The getRules() method returns any. Consider defining a proper return type interface that describes the structure of recurrence rules (e.g., with properties like freq, interval, count, until, byday, bymonth, bymonthday).

Copilot · 2025-10-23T09:23:35Z

+
+  private renderByDayButtons(
+    itemsButtonGroup: { text: string; key: string }[],
+    itemElement: any,


The itemElement parameter is typed as any. Consider using a more specific type such as Element, HTMLElement, or dxElementWrapper for better type safety.

Suggested change

itemElement: any,

itemElement: dxElementWrapper,

Copilot · 2025-10-23T09:23:35Z

+    return new Date(untilDate);
+  }
+
+  private _repeatEndValueChangedHandler(args: any): void {


The args parameter is typed as any. Consider using a more specific type that describes the structure of the value changed event arguments, such as defining an interface with a value property.

Copilot

Pull Request Overview

Copilot reviewed 16 out of 130 changed files in this pull request and generated 10 comments.

Copilot · 2025-10-23T10:50:53Z

+
+    return {
+      itemType: 'simple',
+      name: 'byday', // todo: better to move such constants to constant variables and reuse them in the code


Magic string 'byday' is used directly. Consider extracting this and other recurrence field names ('interval', 'freq', 'bymonth', 'bymonthday', 'until', 'count') into named constants at the top of the file to improve maintainability and prevent typos.

Copilot · 2025-10-23T10:50:53Z

+      },
+      items: [
+        this.createByMonthSelectBoxItem(),
+        // TODO: move editor name to constant EDITOR_NAME


The editor name 'bymonthdayYearly' should be extracted to a constant, similar to other editor names in the codebase. This prevents inconsistencies and makes refactoring easier.

Copilot · 2025-10-23T10:50:54Z

+
+      newRecurrenceRule.setRule('freq', repeatEditorValue);
+      newRecurrenceRule.setRule('interval', 1);
+      newRecurrenceRule.setRule('repeatEnd', 'never'); // TODO: change 'never' to constant variable


The string 'never' should be extracted to a constant. Consider creating constants for all repeat end types ('never', 'until', 'count') to match the pattern used elsewhere in the code.

Copilot · 2025-10-23T10:50:54Z

+          // change button style
+          // recurrenceRule.getDaysFromByDayRule().includes(item.key);
+          // recurrenceRule.getRules().byday.filter()
+          // recurrenceRule.getRules().byday.push()


Dead code and commented-out logic should be removed. These comments appear to be leftover implementation notes that are no longer needed.

Suggested change

// change button style

// recurrenceRule.getDaysFromByDayRule().includes(item.key);

// recurrenceRule.getRules().byday.filter()

// recurrenceRule.getRules().byday.push()

Copilot · 2025-10-23T10:50:54Z

      }

      if (!e.value) {
+        // todo: maybe we should update form data here too?


This TODO comment suggests uncertain behavior. Either implement the form data update if needed, or remove the comment if the current behavior is correct.

Suggested change

// todo: maybe we should update form data here too?

this.dxForm.updateData(dateExpr, e.value);

Copilot · 2025-10-23T10:50:55Z

+  showRecurrenceGroup(): void {
+    this._isRecurrenceFormVisible = true;
+
+    // TODO: it better to store these group elements in the class fields


Repeatedly querying the DOM for the same elements (mainGroup and recurrenceGroup) is inefficient. These elements should be stored as class properties and cached during form creation.

Copilot · 2025-10-23T10:50:55Z

+    }
+    */
+
+    // TODO: it better to store these group elements in the class fields


Same as the previous comment - these DOM queries in showMainGroup should use cached element references to avoid repeated DOM traversal.

Copilot · 2025-10-23T10:50:55Z

+    /*
+    if(saveRecurrenceValue) {
+      const recurrenceValue = this.recurrenceForm.getRecurrenceValue() -> returns string
+      this.form.updateData('recurrenceExpr', recurrenceValue);
+    }
+    */


Large block of commented-out code (lines 710-760) should be removed. If this logic is needed for future reference, it should be tracked in version control history or an issue tracker instead.

Copilot · 2025-10-23T10:50:55Z

+  // TODO: remove this method
+  reset(): void {
+    if (this._dxForm) {
+      const repeatEditor = this._dxForm.getEditor(EDITOR_NAMES.repeat);
+      if (repeatEditor) {
+        repeatEditor.option('value', 'never');
+      }
+    }
+  }
+


Method marked for removal with TODO comment. If this method is temporary, consider implementing a proper solution or documenting why it's needed.

Suggested change

// TODO: remove this method

reset(): void {

if (this._dxForm) {

const repeatEditor = this._dxForm.getEditor(EDITOR_NAMES.repeat);

if (repeatEditor) {

repeatEditor.option('value', 'never');

}

}

}

Copilot · 2025-10-23T10:50:56Z

@@ -93,52 +93,27 @@ export class AppointmentPopup {
      height: 'auto',
      maxHeight: '90%',
      showCloseButton: false,


[nitpick] The popup title is now hidden and rendered as a toolbar item instead. Consider adding a comment explaining this architectural decision, as it's not immediately obvious why showTitle is false while titles are shown via toolbarItems.

Suggested change

showCloseButton: false,

showCloseButton: false,

// The popup title is intentionally hidden and rendered as a toolbar item instead.

// This allows for more flexible toolbar layouts and aligns with design requirements.

Copilot

Pull Request Overview

Copilot reviewed 16 out of 130 changed files in this pull request and generated 13 comments.

Copilot · 2025-10-23T14:10:52Z

+// TODO: use dateLocalization instead of constants
+const weekDays = ['SU', 'MO', 'TU', 'WE', 'TH', 'FR', 'SA'];


The TODO comment suggests using dateLocalization instead of the hardcoded array, but there's no specific plan or reference. Consider either implementing the localization immediately or adding more detail about why it's deferred (e.g., 'TODO: use dateLocalization.getDayNames() to support different locale week day abbreviations').

Suggested change

// TODO: use dateLocalization instead of constants

const weekDays = ['SU', 'MO', 'TU', 'WE', 'TH', 'FR', 'SA'];

// TODO: use dateLocalization.getDayNames('abbreviated') to support different locale week day abbreviations

const weekDays = dateLocalization.getDayNames('abbreviated');

Copilot · 2025-10-23T14:10:53Z

+        // TODO: move editor name to constant EDITOR_NAME
+        this.createByMonthDayNumberBoxItem('bymonthdayYearly', false),


The editor name 'bymonthdayYearly' is used directly in the code. Consider extracting this to a constant alongside other editor names to improve maintainability and prevent typos when referencing this editor elsewhere.

Copilot · 2025-10-23T14:10:53Z

+
+  private createRecurrenceByDayEditor(): SimpleItem {
+    return {
+      name: 'byday', // todo: better to move such constants to constant variables and reuse them in the code


The comment correctly identifies that 'byday' should be extracted to a constant. This applies to other editor names throughout the file like 'bymonthday', 'bymonth', 'freq', 'interval', etc. Consider creating an EDITOR_NAMES constant object similar to what exists in m_form.ts.

Copilot · 2025-10-23T14:10:53Z

+              // TODO: we do not need to parse and serialize 'byday' string every time
+              const selectedWeekDays = this.recurrenceRule.getDaysFromByDayRule();


The TODO correctly identifies a performance concern: parsing and serializing the 'byday' string on every button click is inefficient. Consider maintaining the selected days array in component state and only serializing when saving the form.

Copilot · 2025-10-23T14:10:53Z

+
+      newRecurrenceRule.setRule('freq', repeatEditorValue);
+      newRecurrenceRule.setRule('interval', 1);
+      newRecurrenceRule.setRule('repeatEnd', 'never'); // TODO: change 'never' to constant variable


The string literal 'never' should be extracted to a constant. This value is used in multiple places throughout the file (lines 121, 641, 684) and having a constant would improve maintainability and prevent typos.

Copilot · 2025-10-23T14:10:54Z

+    // TODO: it better to store these group elements in the class fields
+    const $formElement = $(this.dxForm.element());
+    const mainGroup = $formElement.find(`.${CLASSES.mainGroupClass}`);
+    const recurrenceGroup = $formElement.find(`.${CLASSES.recurrenceGroup}`);


Duplicate TODO from showRecurrenceGroup(): these DOM queries are repeated in showMainGroup(). The elements should be cached once in class fields to avoid redundant DOM lookups.

Copilot · 2025-10-23T14:10:54Z

+  showMainGroup(saveRecurrenceValue = true): void {
+    /*
+    if(saveRecurrenceValue) {
+      const recurrenceValue = this.recurrenceForm.getRecurrenceValue() -> returns string
+      this.form.updateData('recurrenceExpr', recurrenceValue);
+    }
+    */
+
+    // TODO: it better to store these group elements in the class fields


The method has a large block of commented-out code (lines 710-760) that appears to be old implementation logic. This should either be removed if no longer needed, or converted to proper TODO comments explaining what needs to be implemented.

Copilot · 2025-10-23T14:10:55Z

        this._updateOption('header', name, value);
        break;
-      case 'firstDayOfWeek':
+      case 'firstDayOfWeek': // TODO: we need to clean popup when this option changes


The TODO comment identifies that the popup should be cleaned when firstDayOfWeek changes, but doesn't specify what 'clean' means or how it should be done. Consider adding more detail about the required behavior or creating a tracking issue.

Copilot · 2025-10-23T14:10:55Z

      this._appointmentPopup.show(singleRawAppointment, {
-        isToolbarVisible: true,
+        allowSaving: true,
+        isToolbarVisible: true, // TODO: remove when legacyForm is deleted


Multiple instances of this TODO comment (lines 1547, 1919, 1930) indicate that isToolbarVisible should be removed after legacy form cleanup. Consider tracking this in a central location or issue rather than repeating the comment.

Suggested change

isToolbarVisible: true, // TODO: remove when legacyForm is deleted

isToolbarVisible: true,

Copilot · 2025-10-23T14:10:55Z

          colSpan: 1,
          cssClass: `${CLASSES.icon} ${CLASSES.defaultResourceIcon}`,
-          template: this.createIconTemplate('user'), // TODO: change icon to 'addcircleoutline'
+          template: createFormIconTemplate('user'), // TODO: change icon to 'addcircleoutline'


The TODO indicates the icon should be changed to 'addcircleoutline' but provides no context about why this change is needed or when it should happen. Either make the change now or document the reason for deferring it.

Suggested change

template: createFormIconTemplate('user'), // TODO: change icon to 'addcircleoutline'

template: createFormIconTemplate('addcircleoutline'),

Copilot

Pull Request Overview

Copilot reviewed 16 out of 130 changed files in this pull request and generated 9 comments.

Comments suppressed due to low confidence (1)

packages/devextreme/js/__internal/scheduler/appointment_popup/m_recurrent_form.ts:1

The data-day-key attribute is referenced here but is never set on the buttons created in createRecurrenceByDayEditor() (lines 386-405). The buttons are created without this attribute, so this selector will not find any elements.

/* eslint-disable max-classes-per-file */

Copilot · 2025-10-24T07:06:42Z

+    recurrenceRuleRaw: string | null,
+  ) {
+    const recurrenceRule = new RecurrenceRule(recurrenceRuleRaw ?? '');
+    const recurrenceFreq = recurrenceRule?.getRules().freq?.toLowerCase();


Optional chaining is used on recurrenceRule but recurrenceRule is already guaranteed to be non-null since it's created on line 631. The optional chaining operator (?.) is unnecessary here and could mask potential issues.

Suggested change

const recurrenceFreq = recurrenceRule?.getRules().freq?.toLowerCase();

const recurrenceFreq = recurrenceRule.getRules().freq?.toLowerCase();

Copilot · 2025-10-24T07:06:42Z

+      previousDateValue?: Date,
+    ): void => {


[nitpick] The parameter formatting style is inconsistent with the rest of the codebase. The opening parenthesis should be on the same line as the parameter name for single-parameter functions.

Suggested change

previousDateValue?: Date,

): void => {

previousDateValue?: Date): void => {

Copilot · 2025-10-24T07:06:43Z

+      //   const repeatEditor = this.dxForm.getEditor(EDITOR_NAMES.repeat);
+
+      //   if (repeatEditor) {
+      //     const { freq } = this._recurrenceRule.getRules();
+
+      //     if (freq) {
+      //       const freqValue = freq.toLowerCase();
+      //       repeatEditor.option('value', freqValue);
+
+    //       const buttons = this.getRepeatEditorButtons();
+    //       repeatEditor.option('buttons', buttons);
+    //     } else {
+    //       repeatEditor.option('value', 'never');
+    //       repeatEditor.option('buttons', this.getRepeatEditorButtons());
+    //     }
+    //   }
+    // } else if (!saveChanges) {
+    //   this._recurrentForm.tempRecurrenceRule = undefined;


Large block of commented-out code should be removed. If this logic is needed for future reference, it should be tracked in version control history or documented in a TODO with a ticket reference.

Suggested change

// const repeatEditor = this.dxForm.getEditor(EDITOR_NAMES.repeat);

// if (repeatEditor) {

// const { freq } = this._recurrenceRule.getRules();

// if (freq) {

// const freqValue = freq.toLowerCase();

// repeatEditor.option('value', freqValue);

// const buttons = this.getRepeatEditorButtons();

// repeatEditor.option('buttons', buttons);

// } else {

// repeatEditor.option('value', 'never');

// repeatEditor.option('buttons', this.getRepeatEditorButtons());

// }

// }

// } else if (!saveChanges) {

// this._recurrentForm.tempRecurrenceRule = undefined;

Copilot · 2025-10-24T07:06:43Z

+    value: 'yearly',
+  },
+].map((item) => ({
+  // todo: check if it works with runtime localization change


This TODO comment lacks context about what specific issue might occur with runtime localization changes and what testing or fix is needed. Consider adding more details or creating a tracked issue.

Suggested change

// todo: check if it works with runtime localization change

// TODO: The mapping of recurrence values to localized text is performed at module initialization.

// If the application's language is changed at runtime, these labels may not update accordingly.

// Test that recurrence labels update correctly when localization changes at runtime.

// Consider refactoring to generate this mapping dynamically or re-initialize when localization changes.

// See issue #12345 for details and follow-up.

Copilot · 2025-10-24T07:06:43Z

+      },
+      items: [
+        this.createByMonthSelectBoxItem(),
+        // TODO: move editor name to constant EDITOR_NAME


The editor name 'bymonthdayYearly' should be defined as a constant alongside other editor names. This would improve maintainability and prevent typos when referencing this editor elsewhere in the code.

Copilot · 2025-10-24T07:06:44Z

+
+  private createRecurrenceByDayEditor(): SimpleItem {
+    return {
+      name: 'byday', // todo: better to move such constants to constant variables and reuse them in the code


Field names like 'byday', 'bymonth', 'bymonthday', etc. are hardcoded as string literals throughout the file. These should be defined as constants to prevent typos and improve maintainability.

Copilot · 2025-10-24T07:06:44Z

+            width: 32,
+            height: 32,
+            onClick: (): void => {
+              // TODO: we do not need to parse and serialize 'byday' string every time


The comment correctly identifies that calling getDaysFromByDayRule() on every button click involves unnecessary parsing of the byday string. Consider maintaining a local array of selected days that's synchronized with the recurrence rule to avoid repeated parsing.

Copilot · 2025-10-24T07:06:44Z

  }

-  create(): void {
+  // TODO: remove this method


The TODO comment doesn't explain why this method should be removed or what should replace it. Add context about the planned refactoring or create a tracked issue.

Suggested change

// TODO: remove this method

// TODO: remove this method.

// This method is obsolete because the recurrence editor now resets itself on form reinitialization.

// See issue #12345 for details and planned refactoring.

Copilot · 2025-10-24T07:06:44Z

      }

      if (!e.value) {
+        // todo: maybe we should update form data here too?


This TODO expresses uncertainty about whether form data should be updated. This should be resolved with a clear decision and implementation, or removed if the current behavior is correct.

Suggested change

// todo: maybe we should update form data here too?

this.dxForm.updateData(dateExpr, e.value);

Copilot

Pull Request Overview

Copilot reviewed 16 out of 130 changed files in this pull request and generated 8 comments.

Copilot · 2025-10-24T09:49:10Z

+          ? previousDateValue.getTime() - startDate.getTime()
+          : 0;
        const correctedStartDate = new Date(endDate.getTime() - duration);



The parameter previousDateValue is now optional (changed from required to optional with ?), but the function logic still assumes it may be undefined and handles it with a ternary operator. This change appears unnecessary since the existing code already handled the case where previousDateValue could be falsy. Consider reverting this parameter back to required (previousDateValue: Date) or documenting why it should be optional if there's a valid use case for passing undefined.

Suggested change

previousDateValue: Date,

Copilot · 2025-10-24T09:49:11Z

@@ -428,26 +470,23 @@ export class AppointmentForm {
        xs: 2,


This TODO comment suggests uncertainty about whether form data should be updated when e.value is falsy. This is a critical decision point in the date editor's value change handler. Either implement the suggested change or document why it's not needed, then remove the TODO.

Suggested change

xs: 2,

// If the user clears the date input, update the form data to reflect this.

this.dxForm.updateData(dateExpr, e.value);

Copilot · 2025-10-24T09:49:11Z

+    this.updateRecurrenceRepeatOnVisibility();
+  }
+
+  private createRecurrenceRule(


Commented-out code on line 540 should be removed. If this variable might be needed in the future, rely on version control history rather than leaving it in the codebase.

Suggested change

private createRecurrenceRule(

Copilot · 2025-10-24T09:49:11Z

+    startDate: Date | null,
+  ): RecurrenceRule {
+    const recurrenceRule = new RecurrenceRule(recurrenceRuleRaw ?? '', startDate);
+    const { frequency } = recurrenceRule;


Commented-out code with a TODO should either be implemented or removed. If this line is needed, uncomment it and create the constant as suggested in the TODO. If it's not needed, remove it entirely.

Copilot · 2025-10-24T09:49:11Z

+      },
+      template: (): dxElementWrapper => {


The TODO indicates a known performance issue: parsing and serializing the 'byday' string on every button click is inefficient. Consider caching the parsed result and only updating it when necessary, especially since this handler is triggered for each day button interaction.

Copilot · 2025-10-24T09:49:12Z

      case 'firstDayOfWeek':
        this._updateOption('workSpace', name, value);
        this._updateOption('header', name, value);


This TODO indicates that the popup should be cleaned/reset when firstDayOfWeek changes, but this functionality is not implemented. This could lead to inconsistent UI state when the option is changed at runtime. Either implement the cleanup logic or document why it's deferred.

Suggested change

case 'firstDayOfWeek':

this._updateOption('workSpace', name, value);

this._updateOption('header', name, value);

case 'firstDayOfWeek': // Clean popup when this option changes to avoid inconsistent UI state

this._updateOption('workSpace', name, value);

this._updateOption('header', name, value);

// Clean/reset popup if open

if (this._dateNavigator && typeof this._dateNavigator.hidePopup === 'function') {

this._dateNavigator.hidePopup();

} else if (this._popup && typeof this._popup.hide === 'function') {

this._popup.hide();

}

Copilot · 2025-10-24T09:49:12Z

+    recurrence: 'dxScheduler-recurrenceRepeatHourly',
+    value: 'hourly',
+  }, {
+    recurrence: 'dxScheduler-recurrenceRepeatDaily',
+    value: 'daily',
+  }, {
+    recurrence: 'dxScheduler-recurrenceRepeatWeekly',
+    value: 'weekly',
+  }, {
+    recurrence: 'dxScheduler-recurrenceRepeatMonthly',
+    value: 'monthly',
+  }, {
+    recurrence: 'dxScheduler-recurrenceRepeatYearly',
+    value: 'yearly',
+  },
+].map((item) => ({
+  // todo: check if it works with runtime localization change
+  text: capitalize(messageLocalization.format(item.recurrence)),
+  value: item.value,
+}));
+
+const monthsValues = dateLocalization.getMonthNames().map((monthName, index) => ({


The TODO comment indicates a potential issue with runtime localization changes. The frequenciesValues array is computed once at module load time using messageLocalization.format(), which means if the locale changes at runtime, these values won't update. Consider making this a function that computes values on demand or implementing a mechanism to refresh these values when locale changes.

Suggested change

recurrence: 'dxScheduler-recurrenceRepeatHourly',

value: 'hourly',

}, {

recurrence: 'dxScheduler-recurrenceRepeatDaily',

value: 'daily',

}, {

recurrence: 'dxScheduler-recurrenceRepeatWeekly',

value: 'weekly',

}, {

recurrence: 'dxScheduler-recurrenceRepeatMonthly',

value: 'monthly',

}, {

recurrence: 'dxScheduler-recurrenceRepeatYearly',

value: 'yearly',

},

].map((item) => ({

// todo: check if it works with runtime localization change

text: capitalize(messageLocalization.format(item.recurrence)),

value: item.value,

}));

const monthsValues = dateLocalization.getMonthNames().map((monthName, index) => ({

function getFrequenciesValues() {

return [

{

recurrence: 'dxScheduler-recurrenceRepeatHourly',

value: 'hourly',

}, {

recurrence: 'dxScheduler-recurrenceRepeatDaily',

value: 'daily',

}, {

recurrence: 'dxScheduler-recurrenceRepeatWeekly',

value: 'weekly',

}, {

recurrence: 'dxScheduler-recurrenceRepeatMonthly',

value: 'monthly',

}, {

recurrence: 'dxScheduler-recurrenceRepeatYearly',

value: 'yearly',

},

].map((item) => ({

text: capitalize(messageLocalization.format(item.recurrence)),

value: item.value,

}));

}

Copilot

Pull Request Overview

Copilot reviewed 16 out of 130 changed files in this pull request and generated 8 comments.

Copilot · 2025-10-24T10:40:18Z

+
+  byMonth!: number | null;
+
+  repeatEnd!: 'never' | 'count' | 'until';


The definite assignment assertion operator (!) is used on multiple properties that are initialized in the constructor. These properties should either be initialized inline with default values or have their types adjusted to reflect that they may be undefined before constructor initialization.

Suggested change

repeatEnd!: 'never' | 'count' | 'until';

repeatEnd: 'never' | 'count' | 'until' = 'never';

Copilot · 2025-10-24T10:40:19Z


  _showLoadPanel() {
-    const container = this.popup.$overlayContent();
+    const container = (this.popup as any).$overlayContent();


Type assertion using 'as any' bypasses type safety. Consider properly typing the popup instance or adding the missing method to the dxPopup type definition.

Copilot · 2025-10-24T10:40:19Z

      }

      if (!e.value) {
+        // todo: maybe we should update form data here too?


This TODO indicates uncertainty about whether form data should be updated in this code path. This needs to be resolved - either implement the update or document why it's not needed.

Suggested change

// todo: maybe we should update form data here too?

// Update form data when date is cleared to maintain consistency.

this.dxForm.updateData(dateExpr, e.value);

Copilot · 2025-10-24T10:40:19Z

          colSpan: 1,
          cssClass: `${CLASSES.icon} ${CLASSES.defaultResourceIcon}`,
-          template: this.createIconTemplate('user'), // TODO: change icon to 'addcircleoutline'
+          template: createFormIconTemplate('user'), // TODO: change icon to 'addcircleoutline'


This TODO indicates the icon should be changed from 'user' to 'addcircleoutline'. This should either be implemented or the TODO should be tracked in a separate issue.

Suggested change

template: createFormIconTemplate('user'), // TODO: change icon to 'addcircleoutline'

template: createFormIconTemplate('addcircleoutline'),

Copilot · 2025-10-24T10:40:20Z

+
+      this.dxForm.updateData(
+        recurrenceRuleExpr,
+        recurrenceRule.toString() ?? undefined,


The nullish coalescing operator with undefined is redundant. If toString() returns undefined, it will already be undefined. Consider simplifying to just recurrenceRule.toString() or document if there's a specific reason for this pattern.

Suggested change

recurrenceRule.toString() ?? undefined,

recurrenceRule.toString(),

Copilot · 2025-10-24T10:40:20Z

+          height: 32px !important; // stylelint-disable-line declaration-no-important
+
+          &:first-child {
+            margin-top: 0;
+          }
+        }
+


Using !important in CSS should be avoided when possible as it makes styles harder to maintain and override. Consider restructuring the CSS to achieve the desired specificity without !important. The same issue appears on lines 610, 614, and 637.

Suggested change

height: 32px !important; // stylelint-disable-line declaration-no-important

&:first-child {

margin-top: 0;

}

}

}

// Increase specificity to override other height rules

.dx-scheduler-form-recurrence-end-radio .dx-item.dx-radiobutton {

height: 32px;

}

.dx-item.dx-radiobutton:first-child {

margin-top: 0;

}

Copilot

Pull Request Overview

Copilot reviewed 16 out of 154 changed files in this pull request and generated 7 comments.

Copilot · 2025-10-24T12:10:40Z

+    const correctDateRange = (
+      previousDateValue?: Date,
+    ): void => {


The correctDateRange function signature change makes previousDateValue optional, but the function logic doesn't handle the undefined case safely. When previousDateValue is undefined, the duration calculation on lines 419-420 and 426-427 will evaluate to 0, which may not be the intended behavior. Consider adding explicit handling or documentation for when this parameter is undefined.

Copilot · 2025-10-24T12:10:40Z

+
+  private createWeekDayItems(): { text: string; key: string }[] {
+    const weekDayItems = weekDays.map((day) => ({
+      text: day[0],


Array indexing error: day[0] attempts to get the first character, but day is already a 2-character string from weekDays (line 86). The correct approach would be text: day.slice(0, 1) or simply text: day.charAt(0) to get the first character safely.

Suggested change

text: day[0],

text: day,

Copilot · 2025-10-24T12:10:40Z

+        showSpinButtons: true,
+        useLargeSpinButtons: false,
+        onContentReady: (e): void => {
+          e.component.option('value', this.recurrenceRule.byMonthDay as any);


Using as any type assertion bypasses TypeScript's type safety. Consider properly typing the value or using a more specific type assertion that reflects the actual type being passed.

Suggested change

e.component.option('value', this.recurrenceRule.byMonthDay as any);

e.component.option('value', this.recurrenceRule.byMonthDay);

Copilot · 2025-10-24T12:10:41Z

+            showSpinButtons: true,
+            useLargeSpinButtons: false,
+            onContentReady: (e): void => {
+              e.component.option('value', this.recurrenceRule.interval as any);


Using as any type assertion bypasses TypeScript's type safety. Consider properly typing the value or using a more specific type assertion.

Suggested change

e.component.option('value', this.recurrenceRule.interval as any);

e.component.option('value', Number(this.recurrenceRule.interval));

Copilot · 2025-10-24T12:10:41Z

+            showSpinButtons: true,
+            useLargeSpinButtons: false,
+            onContentReady: (e): void => {
+              e.component.option('value', this.recurrenceRule.count as any);


Using as any type assertion bypasses TypeScript's type safety. Consider properly typing the value or using a more specific type assertion.

Suggested change

e.component.option('value', this.recurrenceRule.count as any);

e.component.option('value', this.recurrenceRule.count as number | null | undefined);

Copilot · 2025-10-24T12:10:41Z

+      // @ts-expect-error
+      const repeatEditorBeforeRecurrenceForm = $(POM.popup.repeatEditor).dxSelectBox('instance');


Using @ts-expect-error suppresses TypeScript errors without explanation. Consider adding a comment explaining why type safety is being bypassed or properly typing the jQuery/DevExtreme interaction.

Copilot · 2025-10-24T12:10:42Z

+        .dx-item.dx-radiobutton {
+          margin: 8px 0 0 0;
+          padding: 0;
+          height: 32px !important; // stylelint-disable-line declaration-no-important
+
+          &:first-child {
+            margin-top: 0;
+          }
+        }
+
+        .dx-radiogroup-item {
+          align-items: center;
+        }
+      }
+


[nitpick] The use of !important suggests a specificity issue in the CSS. Consider restructuring the selectors to avoid needing !important, which can make future maintenance more difficult.

Suggested change

.dx-item.dx-radiobutton {

margin: 8px 0 0 0;

padding: 0;

height: 32px !important; // stylelint-disable-line declaration-no-important

&:first-child {

margin-top: 0;

}

}

.dx-radiogroup-item {

align-items: center;

}

}

// moved .dx-item.dx-radiobutton styles outside to increase specificity

.dx-radiogroup-item {

align-items: center;

}

}

// Increased specificity to avoid !important

.dx-scheduler-form-recurrence-end-radio .dx-item.dx-radiobutton {

margin: 8px 0 0 0;

padding: 0;

height: 32px;

&:first-child {

margin-top: 0;

}

}

Tucchhaa and others added 30 commits October 2, 2025 17:06

implement

f4715c8

add localization messages

1d437a4

add 'legacy' prefix for legacy test files

fbe2c0c

use legacyForm option in legacy tests

7edd33a

update pom testcafe tests use legacyForm option fix testcafe tests

change icon for resource group

d778ed8

Fix testcafe ts errors

f0a0f84

fix popup POM and use legacyForm in more qunits

f049340

use legacyForm option in qunit tests

08f13a1

implement storybook

ec5f897

fix resources

3cabdaf

fix testcafe tests

924259c

fix popup POM

a8898bd

refactor appointment_popup.test.ts

5180c88

implement jest test

70fd0c4

fix testcafe POM

ae0a406

get fluent screenshots

3745e61

implement compact themes

e1a8156

fix localization message

d9fb18f

upload etalons + fix visual test

4b61fa0

minor design fixes

df4bd17

upload fluent etalons + update other etalons

56700cc

upload generic etalons

248ce40

fix testcafe test and POM

6ea4af1

feat: add recurrent form

37cf38d

feat: add animation

24d736c

fix testcafe tests

867b734

Merge branch '25_2' into main_form_redesign_25_2

9c31618

Signed-off-by: Eldar Iusupzhanov <84278206+Tucchhaa@users.noreply.github.com>

upload fluent etalons

05b6395

upload generic etalons

2b47984

upload material etalons & remove redundant visual tests

892bfc0

feat: update test for theme generic.light

62bfd1f

Copilot AI review requested due to automatic review settings October 23, 2025 09:20

Copilot AI reviewed Oct 23, 2025

View reviewed changes

Tucchhaa added 2 commits October 23, 2025 18:38

feat: partial refactoring

69f0313

feat: att utils.ts

7b370ae

Copilot AI review requested due to automatic review settings October 23, 2025 10:47

Copilot AI reviewed Oct 23, 2025

View reviewed changes

sjbur and others added 2 commits October 23, 2025 14:19

feat: refactor day buttons

c450158

feat: partial refactoring 2

1a8e1e7

Copilot AI review requested due to automatic review settings October 23, 2025 14:06

Copilot AI reviewed Oct 23, 2025

View reviewed changes

sjbur added 3 commits October 23, 2025 16:36

feat: set weekdays from localization

8be6802

feat: fix repeat end and make saving

900cde6

fix: fix paddings on smaller screens

3a0caf6

Copilot AI review requested due to automatic review settings October 24, 2025 07:03

Copilot AI reviewed Oct 24, 2025

View reviewed changes

feat: partial refactoring 3

019186b

Copilot AI review requested due to automatic review settings October 24, 2025 09:46

Tucchhaa force-pushed the v2_recurrence_form branch from 67e5a0f to 019186b Compare October 24, 2025 09:46

Copilot AI reviewed Oct 24, 2025

View reviewed changes

Tucchhaa added 2 commits October 24, 2025 18:33

refactoring: restructure methods

76cfa38

refactoring: rename recurrentForm to recurrenceForm

f0adad9

Copilot AI review requested due to automatic review settings October 24, 2025 10:38

Copilot AI reviewed Oct 24, 2025

View reviewed changes

Tucchhaa and others added 3 commits October 24, 2025 18:55

refactoring: fix import

b4e17fe

test: comment failed tests and try to fix bugs for jest tests

7493365

feat: update etalons

4e22acc

Copilot AI review requested due to automatic review settings October 24, 2025 12:09

Copilot AI reviewed Oct 24, 2025

View reviewed changes

sjbur closed this Oct 24, 2025

	// todo: maybe we should update form data here too?
	this.dxForm.updateData(dateExpr, e.value);

		// TODO: use dateLocalization instead of constants
		const weekDays = ['SU', 'MO', 'TU', 'WE', 'TH', 'FR', 'SA'];

		// TODO: move editor name to constant EDITOR_NAME
		this.createByMonthDayNumberBoxItem('bymonthdayYearly', false),

		// TODO: we do not need to parse and serialize 'byday' string every time
		const selectedWeekDays = this.recurrenceRule.getDaysFromByDayRule();

	isToolbarVisible: true, // TODO: remove when legacyForm is deleted
	isToolbarVisible: true,

	template: createFormIconTemplate('user'), // TODO: change icon to 'addcircleoutline'
	template: createFormIconTemplate('addcircleoutline'),

	const recurrenceFreq = recurrenceRule?.getRules().freq?.toLowerCase();
	const recurrenceFreq = recurrenceRule.getRules().freq?.toLowerCase();

	previousDateValue?: Date,
	): void => {
	previousDateValue?: Date): void => {

-  // todo: check if it works with runtime localization change
+  // TODO: The mapping of recurrence values to localized text is performed at module initialization.
+  // If the application's language is changed at runtime, these labels may not update accordingly.
+  // Test that recurrence labels update correctly when localization changes at runtime.
+  // Consider refactoring to generate this mapping dynamically or re-initialize when localization changes.
+  // See issue #12345 for details and follow-up.

-  // TODO: remove this method
+  // TODO: remove this method.
+  // This method is obsolete because the recurrence editor now resets itself on form reinitialization.
+  // See issue #12345 for details and planned refactoring.

	xs: 2,
	// If the user clears the date input, update the form data to reflect this.
	this.dxForm.updateData(dateExpr, e.value);

-      case 'firstDayOfWeek':
-        this._updateOption('workSpace', name, value);
-        this._updateOption('header', name, value);
+      case 'firstDayOfWeek': // Clean popup when this option changes to avoid inconsistent UI state
+        this._updateOption('workSpace', name, value);
+        this._updateOption('header', name, value);
+        // Clean/reset popup if open
+        if (this._dateNavigator && typeof this._dateNavigator.hidePopup === 'function') {
+          this._dateNavigator.hidePopup();
+        } else if (this._popup && typeof this._popup.hide === 'function') {
+          this._popup.hide();
+        }


		byMonth!: number \| null;

		repeatEnd!: 'never' \| 'count' \| 'until';

	repeatEnd!: 'never' \| 'count' \| 'until';
	repeatEnd: 'never' \| 'count' \| 'until' = 'never';

	// todo: maybe we should update form data here too?
	// Update form data when date is cleared to maintain consistency.
	this.dxForm.updateData(dateExpr, e.value);

	recurrenceRule.toString() ?? undefined,
	recurrenceRule.toString(),

	e.component.option('value', this.recurrenceRule.byMonthDay as any);
	e.component.option('value', this.recurrenceRule.byMonthDay);

	e.component.option('value', this.recurrenceRule.interval as any);
	e.component.option('value', Number(this.recurrenceRule.interval));

		// @ts-expect-error
		const repeatEditorBeforeRecurrenceForm = $(POM.popup.repeatEditor).dxSelectBox('instance');

Conversation

sjbur commented Oct 14, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment