Skip to content

Conversation

@chintankavathia
Copy link
Member

@chintankavathia chintankavathia commented Dec 9, 2025

initCompleted has no functional use but it was used just for handling tests. adjusted tests with immediate promise resolver so we don't have to rely on explicit initCompleted event subscriptions.

Refactor: Remove initCompleted output event

Removes the unused initCompleted output event from SiWidgetHostComponent that served no functional purpose and was only used for testing. Tests have been refactored to eliminate their dependency on this event by using Jasmine clock ticks, fixture.whenStable(), and Promise-based control flow instead of event subscriptions.

Changes:

  • Removed initCompleted output declaration and its two emission sites from SiWidgetHostComponent
  • Refactored widget host tests to use Jasmine clock installation/ticking, fixture.whenStable(), and explicit change detection instead of subscribing to initialization events
  • Refactored grid tests to use async/await patterns with fixture.whenStable() for better control flow
  • Updated test widget module loader to use Promise.resolve() for synchronous immediate resolution instead of dynamic imports

@github-actions
Copy link

github-actions bot commented Dec 9, 2025

@chintankavathia chintankavathia marked this pull request as ready for review December 9, 2025 09:39
@chintankavathia chintankavathia requested a review from a team as a code owner December 9, 2025 09:39
@coderabbitai
Copy link

coderabbitai bot commented Dec 9, 2025

Walkthrough

The PR makes three focused changes: (1) SiGridComponent.addWidgetInstance signature extended to accept an optional payload ({ widgetId: string, payload?: any }); (2) SiWidgetHostComponent removes the public initCompleted output and all emissions of that event; (3) tests updated — si-widget-host tests converted from callback/subscription style to async/await with Jasmine clock/whenStable, and the test-widget loader changed from a dynamic import to a synchronous Promise.resolve of a pre-imported module.

Possibly related PRs

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: removing the initCompleted output event from the SiWidgetHostComponent, which is the primary focus of the pull request.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor/dashboards/remove/initCompleted

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (1)

71-78: Same clock/whenStable considerations as previous test

This test uses the same jasmine.clock().install(); tick(0); await fixture.whenStable(); pattern. The same comments apply here:

  • Wrap clock usage in try/finally to guarantee uninstall() even on failure.
  • Consider adding an explicit markForCheck() + detectChanges() after stabilization to avoid reliance on implicit initial change detection.
📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c1240bb and 532e964.

📒 Files selected for processing (4)
  • projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1 hunks)
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (2 hunks)
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.ts (0 hunks)
  • projects/dashboards-ng/test/test-widget/test-widget.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.ts
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:953-964
Timestamp: 2025-12-04T05:50:17.637Z
Learning: In the siemens/element repository, when making tree-view tests zoneless-ready, jasmine.clock() should be used to mimic setTimeouts even in zoneless mode, as the component implementation relies on setTimeout calls that need to be controlled in tests.
📚 Learning: 2025-12-08T11:24:45.247Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts:67-85
Timestamp: 2025-12-08T11:24:45.247Z
Learning: In select lazy options tests (projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts), jasmine.clock() cannot control RxJS debounceTime in zoneless mode. Real setTimeout waits must be used instead for tests involving search debouncing.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
  • projects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-05T08:00:38.407Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1151
File: projects/element-ng/typeahead/si-typeahead.directive.spec.ts:114-121
Timestamp: 2025-12-05T08:00:38.407Z
Learning: In the typeahead test suite (projects/element-ng/typeahead/si-typeahead.directive.spec.ts), jasmine.clock() must be installed in beforeAll and uninstalled in afterAll (not beforeEach/afterEach) because using per-test clock installation causes schedulerFunction-related issues in these specific tests.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
  • projects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-04T05:50:17.637Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:953-964
Timestamp: 2025-12-04T05:50:17.637Z
Learning: In the siemens/element repository, when making tree-view tests zoneless-ready, jasmine.clock() should be used to mimic setTimeouts even in zoneless mode, as the component implementation relies on setTimeout calls that need to be controlled in tests.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
  • projects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-08T11:25:51.566Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:511-516
Timestamp: 2025-12-08T11:25:51.566Z
Learning: In the select component overflow tests (projects/element-ng/select/si-select.component.spec.ts), jasmine.clock() cannot be used to control the timing for overflow item detection. Real setTimeout waits must be used instead, likely due to ResizeObserver or debounced layout calculations that jasmine.clock() cannot control in zoneless mode.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-01T14:12:11.111Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1104
File: projects/element-ng/datepicker/components/si-day-selection.component.spec.ts:198-202
Timestamp: 2025-12-01T14:12:11.111Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), prefer `await fixture.whenStable()` over `fixture.detectChanges()` after dispatching events or triggering async operations, as `whenStable()` waits for all asynchronous operations to complete and better mimics the production environment.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
  • projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-01T04:01:27.365Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1099
File: projects/element-ng/accordion/si-accordion.component.spec.ts:39-43
Timestamp: 2025-12-01T04:01:27.365Z
Learning: With Angular 20 and esbuild, synchronous `TestBed.configureTestingModule(...).compileComponents()` calls work correctly even when components use external templates (templateUrl), because esbuild pre-compiles templates at build time. The async/await pattern is not required in this scenario.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
  • projects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-04T05:50:38.735Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:1244-1245
Timestamp: 2025-12-04T05:50:38.735Z
Learning: In zoneless Angular tests with OnPush components, calling `await runOnPushChangeDetection(fixture)` followed by `await fixture.whenStable()` is NOT redundant. The `runOnPushChangeDetection` helper forces change detection to run for OnPush components, while `whenStable()` waits for all async operations to complete. Both are necessary and serve different purposes.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
  • projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-08T11:25:20.839Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:204-205
Timestamp: 2025-12-08T11:25:20.839Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), calling fixture.changeDetectorRef.markForCheck() before fixture.detectChanges() is required even for components using the default change detection strategy (not OnPush). This differs from zone.js-based testing where markForCheck is primarily needed only for OnPush components.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
🧬 Code graph analysis (1)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1)
projects/dashboards-ng/test/test-widget/test-widget.ts (1)
  • TEST_WIDGET (17-35)
🔇 Additional comments (4)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1)

218-235: Passing TEST_WIDGET.payload into addWidgetInstance is consistent with the new API

Using:

component.addWidgetInstance({ widgetId: TEST_WIDGET.id, payload: TEST_WIDGET.payload });

correctly exercises the new payload support on addWidgetInstance and keeps the test data aligned with TEST_WIDGET. No issues spotted here.

projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (3)

121-135: setupEditable() test with widget edit actions looks solid

This test:

  • Drives async initialization with jasmine.clock().tick(0) + await fixture.whenStable(), which is appropriate for setTimeout-based flows in zoneless mode (per prior learnings).
  • Explicitly calls fixture.changeDetectorRef.markForCheck(); fixture.detectChanges(); before invoking component.setupEditable(true).
  • Verifies both the merged primaryActions (including the widget’s own edit action) and the widgetInstance!.editable flag.

The sequence and assertions look correct and in line with the component’s expected behavior.


137-150: setupEditable() test without widget edit actions correctly covers fallback behavior

By clearing component.widgetInstance!.primaryEditActions and then calling setupEditable(true), this test validates the fallback to only the grid’s default edit/remove actions and checks widgetInstance!.editable:

  • Async + clock handling matches the previous test.
  • Assertions on primaryActions.length === 2 and ordering (editAction, removeAction) look appropriate.

The non-null assertion on widgetInstance! is acceptable here given prior initialization in the same setup path, but if that initialization flow is ever made lazier, this test is where a failure would show up first.


63-69: Clock + whenStable pattern is reasonable; consider explicit detectChanges and try/finally

Using jasmine.clock().install(); jasmine.clock().tick(0); await fixture.whenStable(); is a sane way to drive setTimeout-based initialization in zoneless tests and aligns with patterns used elsewhere in this repo.

Two small robustness suggestions:

  • Make the clock teardown exception-safe:
jasmine.clock().install();
try {
  jasmine.clock().tick(0);
  await fixture.whenStable();
  expect(component.widgetHost().length).toBe(1);
} finally {
  jasmine.clock().uninstall();
}
  • You currently never call fixture.changeDetectorRef.markForCheck(); fixture.detectChanges(); in this test. If the widget host instantiation ever depends on an explicit change detection pass instead of just the initial one from createComponent, this may turn flaky. Consider adding an explicit detectChanges after stabilization.
⛔ Skipped due to learnings
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:953-964
Timestamp: 2025-12-04T05:50:17.637Z
Learning: In the siemens/element repository, when making tree-view tests zoneless-ready, jasmine.clock() should be used to mimic setTimeouts even in zoneless mode, as the component implementation relies on setTimeout calls that need to be controlled in tests.
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:511-516
Timestamp: 2025-12-08T11:25:51.566Z
Learning: In the select component overflow tests (projects/element-ng/select/si-select.component.spec.ts), jasmine.clock() cannot be used to control the timing for overflow item detection. Real setTimeout waits must be used instead, likely due to ResizeObserver or debounced layout calculations that jasmine.clock() cannot control in zoneless mode.
Learnt from: chintankavathia
Repo: siemens/element PR: 1104
File: projects/element-ng/datepicker/components/si-day-selection.component.spec.ts:198-202
Timestamp: 2025-12-01T14:12:11.111Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), prefer `await fixture.whenStable()` over `fixture.detectChanges()` after dispatching events or triggering async operations, as `whenStable()` waits for all asynchronous operations to complete and better mimics the production environment.
Learnt from: chintankavathia
Repo: siemens/element PR: 1151
File: projects/element-ng/typeahead/si-typeahead.directive.spec.ts:114-121
Timestamp: 2025-12-05T08:00:38.407Z
Learning: In the typeahead test suite (projects/element-ng/typeahead/si-typeahead.directive.spec.ts), jasmine.clock() must be installed in beforeAll and uninstalled in afterAll (not beforeEach/afterEach) because using per-test clock installation causes schedulerFunction-related issues in these specific tests.
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts:67-85
Timestamp: 2025-12-08T11:24:45.247Z
Learning: In select lazy options tests (projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts), jasmine.clock() cannot control RxJS debounceTime in zoneless mode. Real setTimeout waits must be used instead for tests involving search debouncing.
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:1244-1245
Timestamp: 2025-12-04T05:50:38.735Z
Learning: In zoneless Angular tests with OnPush components, calling `await runOnPushChangeDetection(fixture)` followed by `await fixture.whenStable()` is NOT redundant. The `runOnPushChangeDetection` helper forces change detection to run for OnPush components, while `whenStable()` waits for all async operations to complete. Both are necessary and serve different purposes.
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:204-205
Timestamp: 2025-12-08T11:25:20.839Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), calling fixture.changeDetectorRef.markForCheck() before fixture.detectChanges() is required even for components using the default change detection strategy (not OnPush). This differs from zone.js-based testing where markForCheck is primarily needed only for OnPush components.
Learnt from: chintankavathia
Repo: siemens/element PR: 1099
File: projects/element-ng/accordion/si-accordion.component.spec.ts:39-43
Timestamp: 2025-12-01T04:01:27.365Z
Learning: With Angular 20 and esbuild, synchronous `TestBed.configureTestingModule(...).compileComponents()` calls work correctly even when components use external templates (templateUrl), because esbuild pre-compiles templates at build time. The async/await pattern is not required in this scenario.

@chintankavathia chintankavathia force-pushed the refactor/dashboards/remove/initCompleted branch from 532e964 to 79b97cb Compare December 9, 2025 11:28
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

♻️ Duplicate comments (2)
projects/dashboards-ng/test/test-widget/test-widget.ts (1)

6-11: Changes already reviewed.

The typo fix and static import approach were already covered in a previous review comment.

projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (1)

71-78: Verify initialization without explicit detectChanges.

Similar to the previous test, this test doesn't explicitly call fixture.detectChanges() before checking the widget host state. Verify that the absence of widgets is correctly detected without explicit change detection triggering.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 532e964 and 79b97cb.

📒 Files selected for processing (4)
  • projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1 hunks)
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (2 hunks)
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.ts (0 hunks)
  • projects/dashboards-ng/test/test-widget/test-widget.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.ts
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-12-04T05:50:38.735Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:1244-1245
Timestamp: 2025-12-04T05:50:38.735Z
Learning: In zoneless Angular tests with OnPush components, calling `await runOnPushChangeDetection(fixture)` followed by `await fixture.whenStable()` is NOT redundant. The `runOnPushChangeDetection` helper forces change detection to run for OnPush components, while `whenStable()` waits for all async operations to complete. Both are necessary and serve different purposes.

Applied to files:

  • projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-01T14:12:11.111Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1104
File: projects/element-ng/datepicker/components/si-day-selection.component.spec.ts:198-202
Timestamp: 2025-12-01T14:12:11.111Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), prefer `await fixture.whenStable()` over `fixture.detectChanges()` after dispatching events or triggering async operations, as `whenStable()` waits for all asynchronous operations to complete and better mimics the production environment.

Applied to files:

  • projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-05T08:00:38.407Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1151
File: projects/element-ng/typeahead/si-typeahead.directive.spec.ts:114-121
Timestamp: 2025-12-05T08:00:38.407Z
Learning: In the typeahead test suite (projects/element-ng/typeahead/si-typeahead.directive.spec.ts), jasmine.clock() must be installed in beforeAll and uninstalled in afterAll (not beforeEach/afterEach) because using per-test clock installation causes schedulerFunction-related issues in these specific tests.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
  • projects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-04T05:50:17.637Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:953-964
Timestamp: 2025-12-04T05:50:17.637Z
Learning: In the siemens/element repository, when making tree-view tests zoneless-ready, jasmine.clock() should be used to mimic setTimeouts even in zoneless mode, as the component implementation relies on setTimeout calls that need to be controlled in tests.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
  • projects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-08T11:24:45.247Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts:67-85
Timestamp: 2025-12-08T11:24:45.247Z
Learning: In select lazy options tests (projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts), jasmine.clock() cannot control RxJS debounceTime in zoneless mode. Real setTimeout waits must be used instead for tests involving search debouncing.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
  • projects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-08T11:25:51.566Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:511-516
Timestamp: 2025-12-08T11:25:51.566Z
Learning: In the select component overflow tests (projects/element-ng/select/si-select.component.spec.ts), jasmine.clock() cannot be used to control the timing for overflow item detection. Real setTimeout waits must be used instead, likely due to ResizeObserver or debounced layout calculations that jasmine.clock() cannot control in zoneless mode.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-01T04:01:27.365Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1099
File: projects/element-ng/accordion/si-accordion.component.spec.ts:39-43
Timestamp: 2025-12-01T04:01:27.365Z
Learning: With Angular 20 and esbuild, synchronous `TestBed.configureTestingModule(...).compileComponents()` calls work correctly even when components use external templates (templateUrl), because esbuild pre-compiles templates at build time. The async/await pattern is not required in this scenario.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
  • projects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-08T11:25:20.839Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:204-205
Timestamp: 2025-12-08T11:25:20.839Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), calling fixture.changeDetectorRef.markForCheck() before fixture.detectChanges() is required even for components using the default change detection strategy (not OnPush). This differs from zone.js-based testing where markForCheck is primarily needed only for OnPush components.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
🧬 Code graph analysis (1)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1)
projects/dashboards-ng/test/test-widget/test-widget.ts (1)
  • TEST_WIDGET (17-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build
🔇 Additional comments (1)
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (1)

121-135: Async pattern looks good.

The test correctly:

  1. Installs Jasmine clock and advances to trigger initialization
  2. Waits for stability to allow widget instantiation to complete
  3. Explicitly triggers change detection with markForCheck() and detectChanges()
  4. Calls setupEditable(true) after initialization is complete
  5. Verifies the edit actions are properly configured

This pattern properly handles the removal of the initCompleted event by relying on stabilization instead.

@chintankavathia chintankavathia force-pushed the refactor/dashboards/remove/initCompleted branch from 79b97cb to 1216a94 Compare December 9, 2025 12:56
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (1)
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (1)

138-151: Add initial fixture.detectChanges() and address unresolved past comment on explicit change detection.

This test has two issues:

  1. Missing initial fixture.detectChanges(): The test omits the initial change detection call before installing the Jasmine clock, inconsistent with the first test pattern (line 64).

  2. Unaddressed past comment: The previous review correctly noted that explicit change detection should be added after await fixture.whenStable() and before mutating component.widgetInstance!.primaryEditActions at line 142. This ensures the component view/model are synchronized before state mutations and subsequent assertions.

Apply this diff to address both issues:

 it('should setup default edit actions without widgets edit actions', async () => {
+  fixture.detectChanges();
   jasmine.clock().install();
   jasmine.clock().tick(0);
   await fixture.whenStable();
+  fixture.changeDetectorRef.markForCheck();
+  fixture.detectChanges();
   component.widgetInstance!.primaryEditActions = undefined;
   expect(component.primaryActions.length).toBe(0);

   component.setupEditable(true);
   expect(component.primaryActions.length).toBe(2);
   expect(component.primaryActions[0]).toBe(component.editAction);
   expect(component.primaryActions[1]).toBe(component.removeAction);
   expect(component.widgetInstance!.editable).toBeTrue();
   jasmine.clock().uninstall();
 });

Based on learnings, in zoneless Angular tests, explicit change detection is important for state synchronization even with default change detection strategy components.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 79b97cb and 1216a94.

📒 Files selected for processing (4)
  • projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1 hunks)
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (2 hunks)
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.ts (0 hunks)
  • projects/dashboards-ng/test/test-widget/test-widget.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.ts
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-12-01T04:01:27.365Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1099
File: projects/element-ng/accordion/si-accordion.component.spec.ts:39-43
Timestamp: 2025-12-01T04:01:27.365Z
Learning: With Angular 20 and esbuild, synchronous `TestBed.configureTestingModule(...).compileComponents()` calls work correctly even when components use external templates (templateUrl), because esbuild pre-compiles templates at build time. The async/await pattern is not required in this scenario.

Applied to files:

  • projects/dashboards-ng/test/test-widget/test-widget.ts
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-08T11:24:45.247Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts:67-85
Timestamp: 2025-12-08T11:24:45.247Z
Learning: In select lazy options tests (projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts), jasmine.clock() cannot control RxJS debounceTime in zoneless mode. Real setTimeout waits must be used instead for tests involving search debouncing.

Applied to files:

  • projects/dashboards-ng/test/test-widget/test-widget.ts
  • projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-04T05:50:17.637Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:953-964
Timestamp: 2025-12-04T05:50:17.637Z
Learning: In the siemens/element repository, when making tree-view tests zoneless-ready, jasmine.clock() should be used to mimic setTimeouts even in zoneless mode, as the component implementation relies on setTimeout calls that need to be controlled in tests.

Applied to files:

  • projects/dashboards-ng/test/test-widget/test-widget.ts
  • projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-08T11:25:51.566Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:511-516
Timestamp: 2025-12-08T11:25:51.566Z
Learning: In the select component overflow tests (projects/element-ng/select/si-select.component.spec.ts), jasmine.clock() cannot be used to control the timing for overflow item detection. Real setTimeout waits must be used instead, likely due to ResizeObserver or debounced layout calculations that jasmine.clock() cannot control in zoneless mode.

Applied to files:

  • projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-04T05:50:38.735Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:1244-1245
Timestamp: 2025-12-04T05:50:38.735Z
Learning: In zoneless Angular tests with OnPush components, calling `await runOnPushChangeDetection(fixture)` followed by `await fixture.whenStable()` is NOT redundant. The `runOnPushChangeDetection` helper forces change detection to run for OnPush components, while `whenStable()` waits for all async operations to complete. Both are necessary and serve different purposes.

Applied to files:

  • projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-01T14:12:11.111Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1104
File: projects/element-ng/datepicker/components/si-day-selection.component.spec.ts:198-202
Timestamp: 2025-12-01T14:12:11.111Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), prefer `await fixture.whenStable()` over `fixture.detectChanges()` after dispatching events or triggering async operations, as `whenStable()` waits for all asynchronous operations to complete and better mimics the production environment.

Applied to files:

  • projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-05T08:00:38.407Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1151
File: projects/element-ng/typeahead/si-typeahead.directive.spec.ts:114-121
Timestamp: 2025-12-05T08:00:38.407Z
Learning: In the typeahead test suite (projects/element-ng/typeahead/si-typeahead.directive.spec.ts), jasmine.clock() must be installed in beforeAll and uninstalled in afterAll (not beforeEach/afterEach) because using per-test clock installation causes schedulerFunction-related issues in these specific tests.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-08T11:25:20.839Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:204-205
Timestamp: 2025-12-08T11:25:20.839Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), calling fixture.changeDetectorRef.markForCheck() before fixture.detectChanges() is required even for components using the default change detection strategy (not OnPush). This differs from zone.js-based testing where markForCheck is primarily needed only for OnPush components.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-02T06:50:25.778Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1104
File: projects/element-ng/datepicker/components/si-day-selection.component.spec.ts:19-19
Timestamp: 2025-12-02T06:50:25.778Z
Learning: The `runOnPushChangeDetection` helper is located at `projects/element-ng/test-helpers/change-detection.helper.ts` and is shared across the element-ng project, not scoped under individual feature modules like datepicker.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
🧬 Code graph analysis (1)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1)
projects/dashboards-ng/test/test-widget/test-widget.ts (1)
  • TEST_WIDGET (17-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build
🔇 Additional comments (1)
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (1)

63-70: LGTM! Proper initialization pattern established.

This test correctly calls fixture.detectChanges() before installing the Jasmine clock, ensuring component initialization runs before async timing control begins. This pattern should be followed consistently across all tests in this suite.

Copy link
Member

@spike-rabbit spike-rabbit left a comment

Choose a reason for hiding this comment

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

👍

`initCompleted` has no functional use but it was used just for handling tests.
adjusted tests with immediate promise resolver so we don't have to rely on explicit `initCompleted` event subscriptions.
@spike-rabbit spike-rabbit force-pushed the refactor/dashboards/remove/initCompleted branch from 1216a94 to d39cd13 Compare December 12, 2025 06:03
@spike-rabbit spike-rabbit enabled auto-merge (rebase) December 12, 2025 06:03
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
projects/dashboards-ng/test/test-widget/test-widget.ts (1)

5-15: Tighten moduleLoader typing (avoid Promise<any>) to catch export drift early.

Right now loaderFunction returns Promise<any>, which can hide broken exports from projects/dashboards-ng/test/test-widget/index. Consider typing it to the module namespace.

-import { Widget, WidgetConfig } from '@siemens/dashboards-ng';
+import { Widget, WidgetConfig } from '@siemens/dashboards-ng';
 import * as testWidgetModule from 'projects/dashboards-ng/test/test-widget/index';

-const loaderFunction = (name: string): Promise<any> => {
+type TestWidgetModule = typeof testWidgetModule;
+const loaderFunction = (name: string): Promise<TestWidgetModule> => {
   if (name === 'TestWidgetComponent' || name === 'TestWidgetEditorComponent') {
     // immediately resolve so we don't have to deal with timers in tests
     return Promise.resolve(testWidgetModule);
   } else {
     throw new Error(`Unknown component to be loaded ${name}`);
   }
 };
♻️ Duplicate comments (3)
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (3)

72-79: Add initial fixture.detectChanges() before clock (zoneless init won’t run otherwise).

Without an initial fixture.detectChanges(), ngOnInit/widget attachment may never execute in zoneless tests, making this test prone to false positives/flakes.

 it('should not create widget instance without widget', async () => {
   gridService.widgetCatalog.set([]);
+  fixture.detectChanges();
   jasmine.clock().install();
   jasmine.clock().tick(0);
   await fixture.whenStable();
   expect(component.widgetHost().length).toBe(0);
   jasmine.clock().uninstall();
 });

122-136: Run initial change detection before installing clock in #setupEditable() tests.

These tests depend on widget instantiation; in zoneless mode that typically requires an initial fixture.detectChanges() before any clock/tick/whenStable flow.

 it('should setup default edit actions with widgets edit actions', async () => {
+  fixture.detectChanges();
   jasmine.clock().install();
   jasmine.clock().tick(0);
   await fixture.whenStable();
   expect(component.primaryActions.length).toBe(0);
   fixture.changeDetectorRef.markForCheck();
   fixture.detectChanges();
   component.setupEditable(true);
   expect(component.primaryActions.length).toBe(3);
   expect((component.primaryActions[0] as MenuItem).title).toBe('Hello User');
   expect(component.primaryActions[1]).toBe(component.editAction);
   expect(component.primaryActions[2]).toBe(component.removeAction);
   expect(component.widgetInstance!.editable).toBeTrue();
   jasmine.clock().uninstall();
 });

138-151: Add CD sync before mutating widgetInstance in zoneless mode (and add initial detectChanges).

This test mutates component.widgetInstance!.primaryEditActions after whenStable() but without syncing change detection; in zoneless runs this can be flaky. Also add the initial fixture.detectChanges() like the other tests.

 it('should setup default edit actions without widgets edit actions', async () => {
+  fixture.detectChanges();
   jasmine.clock().install();
   jasmine.clock().tick(0);
   await fixture.whenStable();
+  fixture.changeDetectorRef.markForCheck();
+  fixture.detectChanges();
   component.widgetInstance!.primaryEditActions = undefined;
   expect(component.primaryActions.length).toBe(0);

   component.setupEditable(true);
   expect(component.primaryActions.length).toBe(2);
   expect(component.primaryActions[0]).toBe(component.editAction);
   expect(component.primaryActions[1]).toBe(component.removeAction);
   expect(component.widgetInstance!.editable).toBeTrue();
   jasmine.clock().uninstall();
 });
📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1216a94 and d39cd13.

📒 Files selected for processing (4)
  • projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1 hunks)
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (2 hunks)
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.ts (0 hunks)
  • projects/dashboards-ng/test/test-widget/test-widget.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.ts
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:953-964
Timestamp: 2025-12-04T05:50:17.637Z
Learning: In the siemens/element repository, when making tree-view tests zoneless-ready, jasmine.clock() should be used to mimic setTimeouts even in zoneless mode, as the component implementation relies on setTimeout calls that need to be controlled in tests.
📚 Learning: 2025-12-11T10:08:54.597Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1151
File: projects/element-ng/auto-collapsable-list/si-auto-collapsable-list.directive.spec.ts:83-104
Timestamp: 2025-12-11T10:08:54.597Z
Learning: In the auto-collapsable-list directive tests (projects/element-ng/auto-collapsable-list/si-auto-collapsable-list.directive.spec.ts), jasmine.clock() successfully controls the directive's setTimeout-based layout logic in zoneless mode. Mock timers work fine here, unlike in select component overflow tests where real setTimeout waits are required.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
  • projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-08T11:24:45.247Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts:67-85
Timestamp: 2025-12-08T11:24:45.247Z
Learning: In select lazy options tests (projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts), jasmine.clock() cannot control RxJS debounceTime in zoneless mode. Real setTimeout waits must be used instead for tests involving search debouncing.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
  • projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
  • projects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-08T11:25:51.566Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:511-516
Timestamp: 2025-12-08T11:25:51.566Z
Learning: In the select component overflow tests (projects/element-ng/select/si-select.component.spec.ts), jasmine.clock() cannot be used to control the timing for overflow item detection. Real setTimeout waits must be used instead, likely due to ResizeObserver or debounced layout calculations that jasmine.clock() cannot control in zoneless mode.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
  • projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-04T05:50:17.637Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:953-964
Timestamp: 2025-12-04T05:50:17.637Z
Learning: In the siemens/element repository, when making tree-view tests zoneless-ready, jasmine.clock() should be used to mimic setTimeouts even in zoneless mode, as the component implementation relies on setTimeout calls that need to be controlled in tests.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
  • projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
  • projects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-01T14:12:11.111Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1104
File: projects/element-ng/datepicker/components/si-day-selection.component.spec.ts:198-202
Timestamp: 2025-12-01T14:12:11.111Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), prefer `await fixture.whenStable()` over `fixture.detectChanges()` after dispatching events or triggering async operations, as `whenStable()` waits for all asynchronous operations to complete and better mimics the production environment.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
  • projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-05T08:00:38.407Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1151
File: projects/element-ng/typeahead/si-typeahead.directive.spec.ts:114-121
Timestamp: 2025-12-05T08:00:38.407Z
Learning: In the typeahead test suite (projects/element-ng/typeahead/si-typeahead.directive.spec.ts), jasmine.clock() must be installed in beforeAll and uninstalled in afterAll (not beforeEach/afterEach) because using per-test clock installation causes schedulerFunction-related issues in these specific tests.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
📚 Learning: 2025-12-04T05:50:38.735Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:1244-1245
Timestamp: 2025-12-04T05:50:38.735Z
Learning: In zoneless Angular tests with OnPush components, calling `await runOnPushChangeDetection(fixture)` followed by `await fixture.whenStable()` is NOT redundant. The `runOnPushChangeDetection` helper forces change detection to run for OnPush components, while `whenStable()` waits for all async operations to complete. Both are necessary and serve different purposes.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
  • projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-01T04:01:27.365Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1099
File: projects/element-ng/accordion/si-accordion.component.spec.ts:39-43
Timestamp: 2025-12-01T04:01:27.365Z
Learning: With Angular 20 and esbuild, synchronous `TestBed.configureTestingModule(...).compileComponents()` calls work correctly even when components use external templates (templateUrl), because esbuild pre-compiles templates at build time. The async/await pattern is not required in this scenario.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
  • projects/dashboards-ng/test/test-widget/test-widget.ts
📚 Learning: 2025-12-08T11:25:20.839Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:204-205
Timestamp: 2025-12-08T11:25:20.839Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), calling fixture.changeDetectorRef.markForCheck() before fixture.detectChanges() is required even for components using the default change detection strategy (not OnPush). This differs from zone.js-based testing where markForCheck is primarily needed only for OnPush components.

Applied to files:

  • projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts
🧬 Code graph analysis (2)
projects/dashboards-ng/src/components/widget-host/si-widget-host.component.spec.ts (2)
playwright/support/test-helpers.ts (1)
  • expect (18-18)
projects/element-ng/docs.ts (1)
  • MenuItem (111-111)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1)
projects/dashboards-ng/test/test-widget/test-widget.ts (1)
  • TEST_WIDGET (17-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build
🔇 Additional comments (1)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1)

218-235: Passing payload here is justified; ensure widget templates tolerate missing payload elsewhere.

This test now triggers full widget rendering (await fixture.whenStable()), so providing TEST_WIDGET.payload prevents template errors. Just ensure production widgets handle payload being absent (since it’s optional in the API) and avoid implicitly making it required via template access without null guards.

@github-actions
Copy link

Code Coverage

@spike-rabbit spike-rabbit merged commit f9b8cc0 into main Dec 12, 2025
11 checks passed
@spike-rabbit spike-rabbit deleted the refactor/dashboards/remove/initCompleted branch December 12, 2025 06:15
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.

3 participants