feat(dashboards-ng): custom id provider for widget instance#1049
feat(dashboards-ng): custom id provider for widget instance#1049
Conversation
📝 WalkthroughWalkthroughReplaces ad-hoc "new widget" ID logic with a pluggable Sequence Diagram(s)sequenceDiagram
participant User
participant Grid as SiGridComponent
participant Provider as SI_WIDGET_ID_PROVIDER
participant Storage as SiWidgetStorage
rect rgb(230, 245, 255)
Note over Grid,Provider: Add widget -> transient ID generation
User->>Grid: addWidgetInstance(widget)
Grid->>Provider: generateWidgetId(widget, dashboardId)
Provider-->>Grid: transientId
Grid->>Grid: store instance with transientId
end
rect rgb(240, 255, 230)
Note over Grid,Storage: Save -> partition & persist
User->>Grid: save()
Grid->>Grid: partition -> modifiedWidgets, addedWidgets, removedWidgets
Grid->>Storage: save(modifiedWidgets, addedWidgets, removedWidgets, dashboardId)
Storage->>Provider: generateWidgetId(each addedWidget, dashboardId)
Provider-->>Storage: resolvedIds
Storage->>Storage: merge modified + added(with resolvedIds) and persist
Storage-->>Grid: Observable<WidgetConfig[]> (persisted result)
end
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
chintankavathia
force-pushed
the
feat/dashboards/widget-id-provider
branch
from
7264d1d to
292a059
Compare
|
Documentation. Coverage Reports: |
chintankavathia
added
the
breaking-changes
![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
🧹 Nitpick comments (2)
projects/dashboards-demo/src/app/app-widget-id-provider.ts (1)
7-20: Consider clarifying the demo's intent.The implementation demonstrates the basic usage pattern correctly. A few observations:
- The explicit
stringreturn type annotation on line 8 is more restrictive than the abstract signature (string | Promise<string>), which is valid in TypeScript but could be omitted for flexibility.Math.random()provides adequate uniqueness for a demo but may lead to collisions in production scenarios. Consider adding a comment clarifying this is for demonstration purposes.- The commented async example is helpful documentation.
If this is intended as a reference implementation, consider adding a comment:
export class AppWidgetIdProvider extends SiWidgetIdProvider { + // Note: This is a demo implementation. For production use, consider UUIDs or server-generated IDs. override widgetIdResolver(widgetId: string, dashboardId: string): string { return (dashboardId ?? '') + Math.random().toString(36).substring(2, 9);projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
46-54: Consider documenting collision risk for high-volume scenarios.The default implementation provides a simple and fast ID generation using
Math.random(). For most use cases this is adequate, but in high-volume production scenarios, collisions are theoretically possible.Consider adding a JSDoc comment to set expectations:
/** * The default implementation of the {@link SiWidgetIdProvider} which - * generates random widget ids. + * generates random alphanumeric widget IDs using Math.random(). + * For production systems requiring guaranteed uniqueness or security, + * consider implementing a custom provider using UUIDs or server-generated IDs. */ @Injectable({ providedIn: 'root' }) export class SiWidgetDefaultIdProvider extends SiWidgetIdProvider {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
projects/dashboards-demo/src/app/app-widget-id-provider.ts(1 hunks)projects/dashboards-demo/src/app/app.config.ts(2 hunks)projects/dashboards-ng/README.md(1 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts(2 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.ts(5 hunks)projects/dashboards-ng/src/model/si-widget-id-provider.ts(1 hunks)projects/dashboards-ng/src/model/si-widget-storage.ts(3 hunks)projects/dashboards-ng/src/public-api.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
projects/dashboards-ng/src/components/grid/si-grid.component.ts (2)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
projects/dashboards-demo/src/app/app.config.ts (2)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-demo/src/app/app-widget-id-provider.ts (1)
AppWidgetIdProvider(7-20)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (2)
projects/dashboards-ng/src/model/si-widget-storage.ts (1)
SiDefaultWidgetStorage(87-167)projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)
projects/dashboards-ng/src/model/si-widget-storage.ts (2)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
🔇 Additional comments (20)
projects/dashboards-ng/src/public-api.ts (1)
15-15: LGTM!The export properly exposes the new widget ID provider API to consumers of the library, following the existing export pattern.
projects/dashboards-demo/src/app/app.config.ts (3)
10-15: LGTM!The import statement correctly adds
SI_WIDGET_ID_PROVIDERto the existing imports from@siemens/dashboards-ng, maintaining a clean and organized import structure.
19-19: LGTM!The import of the custom
AppWidgetIdProviderimplementation is properly placed and follows the project's import organization conventions.
39-39: LGTM!The DI provider registration correctly wires
SI_WIDGET_ID_PROVIDERto use the customAppWidgetIdProviderclass, demonstrating the proper usage pattern for consumers of the library.projects/dashboards-ng/README.md (2)
187-196: LGTM!The introduction to the widget instance ID generation system is clear and well-structured, accurately describing the default behavior and the purpose of the provider system.
237-251: LGTM!The provider registration examples correctly demonstrate both standalone and module-based approaches, and the fallback behavior note is an important detail for users to understand the system's resilience.
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (3)
14-14: LGTM!The import of
SI_WIDGET_ID_PROVIDERis correctly added to support the new test suite for custom ID resolver functionality.
22-22: LGTM!The removal of
NEW_WIDGET_PREFIXfrom the import statement aligns with the breaking change that removes the temporary ID mechanism in favor of the new provider system.
226-268: LGTM!The new test suite properly validates the custom ID resolver functionality through dependency injection. The test:
- Correctly configures a custom
SI_WIDGET_ID_PROVIDERwith inline implementation- Verifies that the custom resolver is invoked when adding widget instances
- Validates the expected ID pattern (
'custom-id-default') is generatedThis provides good coverage for the new pluggable ID provider feature.
projects/dashboards-ng/src/model/si-widget-storage.ts (4)
7-7: LGTM!The import change from
oftofromis correct, as the updated implementation returnsfrom(Promise.all(newWidgets))to handle asynchronous ID resolution.
10-10: LGTM!The imports of
SI_WIDGET_ID_PROVIDERandSiWidgetDefaultIdProviderare correctly added to support the new injectable ID provider system with fallback behavior.
89-90: LGTM!The injection of both the configurable
idProviderviaSI_WIDGET_ID_PROVIDERand thedefaultIdProviderfor fallback is the correct approach to support custom providers while maintaining resilience.
138-151: LGTM!The
updatemethod correctly handles both resolvedWidgetConfig[]andPromise<WidgetConfig>[]arrays. UsingPromise.allensures all widget IDs are resolved before:
- Updating the BehaviorSubject with the resolved configurations
- Persisting to storage
This maintains consistency between the in-memory observable and persisted state.
projects/dashboards-ng/src/components/grid/si-grid.component.ts (5)
26-29: LGTM!The import of
SI_WIDGET_ID_PROVIDERandSiWidgetDefaultIdProvidercorrectly adds the dependencies needed for the new pluggable ID provider system.
55-56: LGTM!The dependency injection of both providers follows the established pattern: the configurable provider via the token, and the default provider for fallback scenarios.
302-328: LGTM - Breaking change properly implemented.The refactored
addWidgetInstancemethod correctly implements asynchronous ID resolution:
- Returns
Promise<void>(documented breaking change)- Handles both synchronous (
string) and asynchronous (Promise<string>) ID resolution- Includes fallback to
defaultWidgetIdProvideron error, ensuring widgets can always be added- Properly tracks transient widgets before persistence
The error handling silently falls back to the default provider, which is appropriate since it ensures the operation succeeds while the error context isn't critical for end users.
255-258: LGTM!The simplified save logic correctly reflects the new ID provider system where widgets receive their final IDs immediately upon creation, eliminating the need to filter or replace temporary IDs before persistence.
341-343: LGTM - Important correctness fix.The guard correctly prevents transient (unsaved) widgets from being added to
markedForRemoval. Since transient widgets haven't been persisted yet, they should simply be removed from the visible list without triggering a delete operation in the storage layer.This ensures the storage layer only receives deletion requests for widgets that actually exist in persistence.
projects/dashboards-ng/src/model/si-widget-id-provider.ts (2)
9-24: LGTM!The
SI_WIDGET_ID_PROVIDERinjection token is well-designed:
- Provides clear documentation with usage example
- Uses
providedIn: 'root'for singleton behavior- Includes a sensible default factory that injects
SiWidgetDefaultIdProvider- Allows easy customization through DI
26-40: LGTM!The abstract
SiWidgetIdProviderclass provides a clean and flexible contract:
- Union return type (
string | Promise<string>) supports both synchronous and asynchronous ID generation- Type-safe parameter using
WidgetConfig['widgetId']- Clear JSDoc documentation
- Optional
dashboardIdparameter enables context-aware ID generation
chintankavathia
force-pushed
the
feat/dashboards/widget-id-provider
branch
from
292a059 to
b295ac9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1)
14-22: Custom ID resolver wiring in tests looks good; consider adding async/fallback coverageThe new spec suite correctly:
- Overrides
SI_WIDGET_ID_PROVIDERwith a custom implementation.- Verifies that
addWidgetInstanceuses the custom resolver and produces the expectedcustom-id-defaultID whendashboardIdis absent.To more fully exercise the new ID-provider contract, you might also:
- Add a test where
widgetIdResolverreturns aPromise<string>to cover the async path.- Add a test where the custom resolver rejects/throws and verify fallback to
SiWidgetDefaultIdProvider(or at least that the grid still assigns a valid ID).This would guard against regressions in the more complex async + fallback logic.
Also applies to: 226-268
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
projects/dashboards-demo/src/app/app-widget-id-provider.ts(1 hunks)projects/dashboards-demo/src/app/app.config.ts(2 hunks)projects/dashboards-ng/README.md(1 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts(2 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.ts(5 hunks)projects/dashboards-ng/src/model/si-widget-id-provider.ts(1 hunks)projects/dashboards-ng/src/model/si-widget-storage.ts(3 hunks)projects/dashboards-ng/src/public-api.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- projects/dashboards-ng/README.md
- projects/dashboards-ng/src/model/si-widget-id-provider.ts
- projects/dashboards-demo/src/app/app.config.ts
🧰 Additional context used
🧬 Code graph analysis (3)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (3)
projects/dashboards-ng/src/model/si-widget-storage.ts (1)
SiDefaultWidgetStorage(87-167)projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/configuration.ts (1)
SI_DASHBOARD_CONFIGURATION(21-27)
projects/dashboards-ng/src/components/grid/si-grid.component.ts (2)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
projects/dashboards-ng/src/model/si-widget-storage.ts (2)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
⏰ 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 (2)
projects/dashboards-ng/src/public-api.ts (1)
15-15: Public re-export of ID provider looks correctRe-exporting
./model/si-widget-id-providerfrom the dashboards-ng public API aligns with the new DI-based ID provider contract and keeps the surface consistent with other model exports. No issues here.projects/dashboards-ng/src/components/grid/si-grid.component.ts (1)
336-343: Guard against marking transient widgets as removedThe added check in
removeWidgetInstanceto skip adding a widget tomarkedForRemovalif it’s present intransientWidgetInstancesis a good fix: it ensures widgets that were never persisted aren’t treated as “to be removed” on save, which matches the semantics oftransientWidgetInstances.No changes needed here.
projects/dashboards-ng/src/components/grid/si-grid.component.ts
Outdated
Show resolved
Hide resolved
chintankavathia
force-pushed
the
feat/dashboards/widget-id-provider
branch
from
b295ac9 to
22041d2
Compare
There was a problem hiding this comment.
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/src/components/grid/si-grid.component.ts (1)
302-328: Address async/await mismatch inaddWidgetInstancecallers.The review concern is confirmed and critical.
addWidgetInstanceis nowasync, but the main production caller does not await it:
- si-flexible-dashboard.component.ts:270:
this.grid().addWidgetInstance(widgetConfig);must be awaited- si-flexible-dashboard.component.spec.ts:82: The mock returns
voidinstead ofPromise<void>, creating a type inconsistency- si-grid.component.spec.ts (lines 137, 150, 163, 182, 199, 260): Test calls should await the Promise to properly handle async behavior
Without awaiting, there is a race condition risk: if the ID resolver is slow, subsequent operations may proceed before the widget is fully added.
♻️ Duplicate comments (1)
projects/dashboards-ng/src/model/si-widget-storage.ts (1)
118-136: Handleupdate()Promise rejections to avoid unhandled errors.The async
save()flow is fine, butupdate()still callsPromise.all(newConfigs).then(...)without a.catch(). If any of the per-widget promises reject, or ifstorage.setItem(...)throws inside thethenhandler, this results in an unhandled Promise rejection and the error is not surfaced via the returnedObservable.Consider at least adding a catch to
update()so these failures are handled (and optionally logged), e.g.:protected update( newConfigs: WidgetConfig[] | Promise<WidgetConfig>[], dashboardId?: string ): void { const widgets$ = this.load(dashboardId) as BehaviorSubject<WidgetConfig[]>; - Promise.all(newConfigs).then(widgetConfigs => { - widgets$.next(widgetConfigs); - if (!dashboardId) { - this.storage.setItem(STORAGE_KEY, JSON.stringify(widgetConfigs)); - } else { - this.storage.setItem(`${STORAGE_KEY}-${dashboardId}`, JSON.stringify(widgetConfigs)); - } - }); + Promise.all(newConfigs) + .then(widgetConfigs => { + widgets$.next(widgetConfigs); + if (!dashboardId) { + this.storage.setItem(STORAGE_KEY, JSON.stringify(widgetConfigs)); + } else { + this.storage.setItem(`${STORAGE_KEY}-${dashboardId}`, JSON.stringify(widgetConfigs)); + } + }) + .catch(err => { + // Avoid unhandled rejections; you may want to replace this with your logging strategy. + console.error('Updating widget storage failed', err); + }); }This keeps the external API the same while preventing unhandled rejections and making storage-layer failures observable.
Also applies to: 138-150
🧹 Nitpick comments (1)
projects/dashboards-ng/README.md (1)
204-207: Add missing imports in the async provider example.In the
ServerWidgetIdProvidersnippet you useinject(HttpClient)but only importInjectableandSiWidgetIdProviderplusfirstValueFrom. For completeness and copy‑paste‑ability, consider adding:import { inject, Injectable } from '@angular/core'; import { HttpClient } from '@angular/common/http'; import { SiWidgetIdProvider } from '@siemens/dashboards-ng'; import { firstValueFrom } from 'rxjs';Also applies to: 222-227
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
projects/dashboards-demo/src/app/app-widget-id-provider.ts(1 hunks)projects/dashboards-demo/src/app/app.config.ts(2 hunks)projects/dashboards-ng/README.md(1 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts(2 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.ts(5 hunks)projects/dashboards-ng/src/model/si-widget-id-provider.ts(1 hunks)projects/dashboards-ng/src/model/si-widget-storage.ts(3 hunks)projects/dashboards-ng/src/public-api.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- projects/dashboards-ng/src/public-api.ts
- projects/dashboards-demo/src/app/app-widget-id-provider.ts
- projects/dashboards-ng/src/model/si-widget-id-provider.ts
- projects/dashboards-demo/src/app/app.config.ts
🧰 Additional context used
🧬 Code graph analysis (3)
projects/dashboards-ng/src/components/grid/si-grid.component.ts (2)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (2)
projects/dashboards-ng/src/model/si-widget-storage.ts (1)
SiDefaultWidgetStorage(87-167)projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)
projects/dashboards-ng/src/model/si-widget-storage.ts (2)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
⏰ 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 (2)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1)
226-267: Custom ID resolver test wiring looks correct.The second describe block cleanly overrides
SI_WIDGET_ID_PROVIDERand verifies thataddWidgetInstanceuses it to produce the expectedcustom-<widgetId>-defaultID. This gives good coverage of the DI wiring for custom ID providers.projects/dashboards-ng/src/components/grid/si-grid.component.ts (1)
248-259: Saving with updated positions looks good.Updating widget positions via
updateWidgetPositionsjust before callingwidgetStorage.saveensures the persisted layout always reflects the latest grid positions, even if no further grid events occurred. This aligns the saved state with what the user sees.
chintankavathia
force-pushed
the
feat/dashboards/widget-id-provider
branch
from
22041d2 to
364d23c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
21-24: Provider abstraction and token wiring look solid; optionaloverridenitThe
SI_WIDGET_ID_PROVIDERtoken defaulting toSiWidgetDefaultIdProviderand the abstractSiWidgetIdProvider.widgetIdResolvercontract are consistent with how the grid and storage use ID providers. The default implementation is straightforward and appropriate for random IDs.Consider marking
widgetIdResolverinSiWidgetDefaultIdProviderwith theoverridekeyword to guard against accidental signature drift, but otherwise this file looks good as-is.Also applies to: 29-40, 46-53
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
projects/dashboards-demo/src/app/app-widget-id-provider.ts(1 hunks)projects/dashboards-demo/src/app/app.config.ts(2 hunks)projects/dashboards-ng/README.md(1 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts(2 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.ts(5 hunks)projects/dashboards-ng/src/model/si-widget-id-provider.ts(1 hunks)projects/dashboards-ng/src/model/si-widget-storage.ts(3 hunks)projects/dashboards-ng/src/public-api.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- projects/dashboards-demo/src/app/app-widget-id-provider.ts
- projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
- projects/dashboards-demo/src/app/app.config.ts
🧰 Additional context used
🧬 Code graph analysis (3)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
projects/dashboards-ng/src/model/si-widget-storage.ts (2)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
projects/dashboards-ng/src/components/grid/si-grid.component.ts (2)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
⏰ 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 (7)
projects/dashboards-ng/src/public-api.ts (1)
10-16: Public export for widget ID provider looks correctRe‑exporting
./model/si-widget-id-providerhere cleanly exposesSI_WIDGET_ID_PROVIDER/SiWidgetIdProvider/SiWidgetDefaultIdProviderfor consumers and matches the usage shown in the README examples. No issues from an API-surface perspective.projects/dashboards-ng/src/model/si-widget-storage.ts (2)
7-11: ID provider injection into default storage is consistentImporting
SI_WIDGET_ID_PROVIDER/SiWidgetDefaultIdProviderand injecting them inSiDefaultWidgetStorageensures the storage layer uses the same pluggable ID generation mechanism as the grid, which keeps persistence and UI aligned. No issues with this wiring.Also applies to: 88-90
118-136: Async ID resolution and persistence flow look correctThe
saveimplementation correctly handles both synchronous and Promise‑basedwidgetIdResolverresults, with a fallback to the default provider when the returned Promise rejects, andupdatewaits for all IDs to resolve before emitting and writing toStorage. This matches the documented behavior and maintains a coherent async save path.Also applies to: 138-150
projects/dashboards-ng/src/components/grid/si-grid.component.ts (4)
26-29: Grid’s dependency on the pluggable ID provider is wired correctlyImporting and injecting
SI_WIDGET_ID_PROVIDERplusSiWidgetDefaultIdProviderhere keeps the grid aligned with the storage’s ID resolution strategy. The DI usage is straightforward and matches the new provider model.Also applies to: 55-56
255-256: Save now delegates current layout and removals cleanly to storageRecomputing widget positions via
updateWidgetPositionsand then callingsave(widgets, this.markedForRemoval, this.dashboardId())ensures persistence uses the latest layout plus the explicit removal list. This keeps the grid’s visible state and stored state in sync.Also applies to: 258-258
302-321: AsyncaddWidgetInstancecorrectly supports sync/async ID providers with fallback
addWidgetInstancenow accepts providers that return either a string or a Promise, and falls back todefaultWidgetIdProviderwhen the returned Promise rejects, before inserting the new widget and recomputing positions. Given your stated contract that synchronous throws from custom providers should surface, this implementation is consistent with both the storage logic and the README wording.
341-343: Avoiding marking transient widgets for removal is a good fixThe extra guard against
transientWidgetInstancesprevents widgets that were never persisted from being added tomarkedForRemoval, which avoids emitting bogus “removals” to storage implementations that rely on that list.
chintankavathia
force-pushed
the
feat/dashboards/widget-id-provider
branch
from
364d23c to
babfaf0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
projects/dashboards-ng/src/model/si-widget-storage.ts (1)
124-126: Simplify by removing no-op.then().The
.then()call with no arguments on line 125 is a no-op that doesn't affect behavior but reduces clarity.Apply this diff:
- const resolvedId = await id - .then() - .catch(err => this.defaultIdProvider.widgetIdResolver(widget.widgetId, dashboardId)); + const resolvedId = await id + .catch(err => this.defaultIdProvider.widgetIdResolver(widget.widgetId, dashboardId));projects/dashboards-ng/src/components/grid/si-grid.component.ts (1)
309-311: Simplify by removing identity function.then(i => i).The
.then(i => i)on line 310 is an identity function that doesn't transform the value, making it redundant and reducing code clarity.Apply this diff:
- resolvedId = await id - .then(i => i) - .catch(err => { + resolvedId = await id.catch(err => { // In case of an error, we still need to return some id so fallback to default return this.defaultWidgetIdProvider.widgetIdResolver( widgetInstanceConfig.widgetId, this.dashboardId() ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
projects/dashboards-demo/src/app/app-widget-id-provider.ts(1 hunks)projects/dashboards-demo/src/app/app.config.ts(2 hunks)projects/dashboards-ng/README.md(1 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts(2 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.ts(5 hunks)projects/dashboards-ng/src/model/si-widget-id-provider.ts(1 hunks)projects/dashboards-ng/src/model/si-widget-storage.ts(3 hunks)projects/dashboards-ng/src/public-api.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- projects/dashboards-demo/src/app/app-widget-id-provider.ts
- projects/dashboards-ng/src/model/si-widget-id-provider.ts
- projects/dashboards-ng/src/public-api.ts
- projects/dashboards-demo/src/app/app.config.ts
🧰 Additional context used
🧬 Code graph analysis (3)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (3)
projects/dashboards-ng/src/model/si-widget-storage.ts (1)
SiDefaultWidgetStorage(87-167)projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/configuration.ts (1)
SI_DASHBOARD_CONFIGURATION(21-27)
projects/dashboards-ng/src/model/si-widget-storage.ts (2)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
projects/dashboards-ng/src/components/grid/si-grid.component.ts (2)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
🔇 Additional comments (5)
projects/dashboards-ng/README.md (1)
187-253: Documentation is comprehensive and accurate.The new "Widget instance ID generation" section thoroughly covers:
- Default random ID generation via
SiWidgetDefaultIdProvider- Custom provider implementation (both synchronous and asynchronous patterns)
- Proper registration in standalone and module-based apps
- Fallback behavior when custom providers fail
The code examples are syntactically correct and align with the implementation. All previously flagged issues (abstract class terminology,
firstValueFromusage, fallback wording) have been addressed.projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1)
226-268: Test suite correctly validates custom ID provider integration.The new test suite properly configures a custom
widgetIdResolvervia DI and asserts that the generated widget ID follows the expected pattern (custom-id-default). This validates that the grid component correctly delegates ID generation to the injected provider.projects/dashboards-ng/src/model/si-widget-storage.ts (1)
118-135: Implementation correctly handles async ID resolution with fallback.The
savemethod properly delegates ID generation to the injected provider and handles both synchronous (string) and asynchronous (Promise) return values. The fallback todefaultIdProvideron promise rejection ensures widgets can always be added.projects/dashboards-ng/src/components/grid/si-grid.component.ts (2)
302-321: Async ID resolution with fallback correctly implemented.The method properly delegates to the injected
widgetIdProviderand handles both synchronous and asynchronous ID generation. The fallback todefaultWidgetIdProvideron promise rejection ensures widgets can always be added even if the custom provider fails.
341-343: Guard correctly prevents transient widgets from being marked for removal.This check ensures that widgets which haven't been persisted yet (transient widgets) are simply discarded rather than being added to
markedForRemoval, avoiding unnecessary operations during save.
chintankavathia
force-pushed
the
feat/dashboards/widget-id-provider
branch
2 times, most recently
from
e13ecce to
f143835
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
51-53: Consider collision risk and unused parameters in default ID generation.The default implementation generates IDs using
Math.random().toString(36).substring(2, 9), which produces ~7-character alphanumeric strings. While suitable for prototyping and demos, this approach has some considerations:
- Collision probability: With base-36 and 7 characters, you have ~78 billion possible IDs. For typical dashboard usage (tens to hundreds of widgets), collision probability is very low but non-zero.
- Unused parameters: The
widgetIdanddashboardIdparameters are not used in ID generation. While this is intentional (random IDs don't depend on context), consider whether the ID could benefit from incorporating these values (e.g.,${dashboardId}-${widgetId}-${randomPart}) to guarantee uniqueness within a dashboard or make IDs more traceable.The current implementation is acceptable for the default provider, but document that production applications should consider providing a custom implementation if stronger guarantees are needed (e.g., UUID v4, server-generated IDs, or context-aware prefixing).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
projects/dashboards-demo/src/app/app-widget-id-provider.ts(1 hunks)projects/dashboards-demo/src/app/app.config.ts(2 hunks)projects/dashboards-ng/README.md(1 hunks)projects/dashboards-ng/src/components/flexible-dashboard/si-flexible-dashboard.component.spec.ts(1 hunks)projects/dashboards-ng/src/components/flexible-dashboard/si-flexible-dashboard.component.ts(1 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts(7 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.ts(5 hunks)projects/dashboards-ng/src/model/si-widget-id-provider.ts(1 hunks)projects/dashboards-ng/src/model/si-widget-storage.ts(3 hunks)projects/dashboards-ng/src/public-api.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- projects/dashboards-demo/src/app/app-widget-id-provider.ts
- projects/dashboards-ng/README.md
- projects/dashboards-demo/src/app/app.config.ts
🧰 Additional context used
🧬 Code graph analysis (5)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (3)
projects/dashboards-ng/test/test-widget/test-widget.ts (1)
TEST_WIDGET(15-33)projects/dashboards-ng/src/model/si-widget-storage.ts (1)
SiDefaultWidgetStorage(87-167)projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)
projects/dashboards-ng/src/components/grid/si-grid.component.ts (2)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
projects/dashboards-ng/src/components/flexible-dashboard/si-flexible-dashboard.component.spec.ts (1)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
projects/dashboards-ng/src/model/si-widget-storage.ts (2)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
⏰ 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 (14)
projects/dashboards-ng/src/public-api.ts (1)
15-15: LGTM! Public API export enables custom ID provider implementations.The new export correctly surfaces the ID provider infrastructure (token, abstract class, and default implementation) to library consumers, enabling them to provide custom widget ID generation logic as intended by the PR.
projects/dashboards-ng/src/components/flexible-dashboard/si-flexible-dashboard.component.ts (1)
265-272: LGTM! Async handler correctly awaits the updatedaddWidgetInstancesignature.The catalog closed event subscription callback is now properly async and awaits the
addWidgetInstancecall, which aligns with the updated method signature that returnsPromise<void>.projects/dashboards-ng/src/components/flexible-dashboard/si-flexible-dashboard.component.spec.ts (1)
82-84: LGTM! Mock signature correctly reflects async change.The test mock's
addWidgetInstancesignature is properly updated to returnPromise<void>, aligning with the production code change.projects/dashboards-ng/src/model/si-widget-id-provider.ts (2)
21-24: LGTM! Well-designed injection token with sensible defaults.The token correctly uses
providedIn: 'root'and provides a factory that injects the default implementation, allowing consumers to override with custom providers while maintaining a safe fallback.
29-40: LGTM! Flexible abstract contract supports both sync and async ID generation.The abstract class correctly defines the contract with a return type of
string | Promise<string>, enabling both synchronous (fast) and asynchronous (e.g., API-based) ID providers. The method signature appropriately acceptswidgetIdand optionaldashboardIdfor context-aware ID generation.projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (2)
136-147: LGTM! Test properly updated for async widget addition.The test correctly uses
async/awaitto handle the updatedaddWidgetInstancesignature and validates unique ID generation for multiple widgets.
225-267: LGTM! Excellent integration test for custom ID providers.This new test suite effectively validates that:
- Custom ID providers can be injected via
SI_WIDGET_ID_PROVIDER- The custom resolver is invoked during widget creation
- Generated IDs follow the expected custom pattern
This provides good coverage of the new pluggable ID provider feature.
projects/dashboards-ng/src/components/grid/si-grid.component.ts (4)
55-56: LGTM! ID providers correctly injected.The component properly injects both the configurable
SI_WIDGET_ID_PROVIDERand theSiWidgetDefaultIdProviderfallback, enabling custom ID generation with a safe default.
302-328: Async ID resolution correctly implemented with fallback.The method properly:
- Accepts
Omit<WidgetConfig, 'id'>to enforce that callers don't provide IDs- Invokes the custom ID provider with context (
widgetId,dashboardId)- Handles both sync (string) and async (Promise) return values
- Falls back to default provider on Promise rejection
- Returns
Promise<void>to signal async completionNote: As discussed in past review comments, synchronous throws from
widgetIdResolverwill propagate as rejections (standard async function behavior), which is acceptable per the architectural decision that custom providers should handle their own sync errors.
255-258: Save flow simplified correctly—no more temporary ID filtering.The removal of NEW_WIDGET_PREFIX filtering aligns with the new architecture where widgets receive final IDs immediately via the provider. The save flow now simply updates positions and delegates to storage, which is cleaner and more straightforward.
341-343: LGTM! Transient widget check prevents unnecessary persistence.The guard correctly prevents adding transient (not-yet-saved) widgets to
markedForRemoval, avoiding the persistence of widgets that were never saved in the first place. This is a sensible optimization.projects/dashboards-ng/src/model/si-widget-storage.ts (3)
89-90: LGTM! ID providers correctly injected.The storage class properly injects both the configurable
SI_WIDGET_ID_PROVIDERand the fallbackSiWidgetDefaultIdProvider, enabling custom ID generation with a safe default.
118-135: Async ID resolution correctly implemented with fallback.The save flow properly:
- Maps each widget without an ID to an async operation that resolves its ID
- Invokes the custom ID provider with context
- Handles both sync (string) and async (Promise) return values
- Falls back to default provider on Promise rejection via
.catch()- Updates storage and returns an Observable from
Promise.allThis correctly implements the new provider-based ID resolution architecture.
138-151: Updated signature correctly handles async ID resolution.The
update()method signature now acceptsWidgetConfig[] | Promise<WidgetConfig>[]and usesPromise.allto resolve all widgets before persisting to storage. This properly supports the async ID resolution flow introduced by the provider system.Note: The
Promise.allat line 143 does not have a.catch()handler. As discussed in past review comments, this architectural decision places error handling responsibility on the caller (via the Observable returned fromsave()).
timowolf
left a comment
timowolf
left a comment
There was a problem hiding this comment.
In many cases, the final persisted id of a resource is returned by the API, when creating the entity on a HTTP POST. Upates need a HTTP PUT. With this change, the storage layer does not get the information of which widgets are new and which are updated.
The save method takes actually three parameter (updated, new, deleted), but first two are merged and separated by removing the id in the old implementation.
Maybe that was not so bad, maybe there is a better way of doing it.
chintankavathia
force-pushed
the
feat/dashboards/widget-id-provider
branch
from
f143835 to
4c0834f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
projects/dashboards-demo/src/app/app-widget-storage.ts (1)
51-57: Consider aligningsaveAsDefaultswith the new ID provider pattern.The
saveAsDefaultsmethod still generates IDs directly usingMath.random().toString(36).substring(2, 9)instead of using the injectedSiWidgetIdProvider. While this may be intentional for the demo, it could cause confusion as it bypasses the custom ID provider mechanism.If you want consistency with the new provider pattern, consider injecting and using the widget ID provider here as well.
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (2)
149-157: Unnecessaryasyncmarker.The test function is marked as
asyncbut doesn't contain anyawaitexpressions. Either remove theasynckeyword or useawaitif async operations are intended.- it('#removeWidget() shall remove WidgetConfig from visible widgets', async () => { + it('#removeWidget() shall remove WidgetConfig from visible widgets', () => {
271-278: Consider more robust async waiting.Using
setTimeout(resolve, 0)to wait for async operations is fragile and depends on microtask timing. Consider usingfixture.whenStable()or wrapping the test infakeAsyncwithtick()for more deterministic behavior.component.save(); // wait for async save operation to complete - await new Promise(resolve => setTimeout(resolve, 0)); + await fixture.whenStable(); const widgets = component.visibleWidgetInstances$.value; expect(widgets[0].id).toBe('custom-id-default');projects/dashboards-ng/src/model/si-widget-storage.ts (1)
131-134: Remove redundant.then()call.Calling
.then()with no arguments is a no-op that just creates an unnecessary intermediate promise.} else { - const resolvedId = await id - .then() - .catch(err => this.defaultIdProvider.widgetIdResolver(widget, dashboardId)); + const resolvedId = await id.catch(err => + this.defaultIdProvider.widgetIdResolver(widget, dashboardId) + ); return { ...widget, id: resolvedId }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
projects/dashboards-demo/src/app/app-widget-id-provider.ts(1 hunks)projects/dashboards-demo/src/app/app-widget-storage.ts(1 hunks)projects/dashboards-demo/src/app/app.config.ts(2 hunks)projects/dashboards-ng/README.md(1 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts(5 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.ts(5 hunks)projects/dashboards-ng/src/model/si-widget-id-provider.ts(1 hunks)projects/dashboards-ng/src/model/si-widget-storage.ts(4 hunks)projects/dashboards-ng/src/public-api.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- projects/dashboards-demo/src/app/app-widget-id-provider.ts
- projects/dashboards-ng/src/public-api.ts
🧰 Additional context used
🧬 Code graph analysis (4)
projects/dashboards-demo/src/app/app.config.ts (2)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-demo/src/app/app-widget-id-provider.ts (1)
AppWidgetIdProvider(7-28)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (3)
projects/dashboards-ng/src/model/si-widget-storage.ts (1)
SiDefaultWidgetStorage(94-177)projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/configuration.ts (1)
SI_DASHBOARD_CONFIGURATION(21-27)
projects/dashboards-ng/src/components/grid/si-grid.component.ts (1)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
⏰ 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 (15)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (3)
1-24: Well-structured injection token with factory default.The
InjectionTokenwithprovidedIn: 'root'and factory pattern correctly establishes the DI boundary. This allows consumers to override while ensuring a default is always available.
29-48: Consider documenting the transient vs permanent ID distinction more explicitly.The abstract class design is clean. The
transientWidgetIdResolverprovides a sensible default, and allowingwidgetIdResolverto returnPromise<string>enables async ID generation (e.g., from a backend).One observation: the
widgetparameter intransientWidgetIdResolveris unused in the default implementation. This is acceptable for API flexibility, but consider adding a brief JSDoc note explaining when consumers might want to use it (e.g., generating IDs based on widget type).
54-61: Default provider implementation is correct.The
SiWidgetDefaultIdProvidercorrectly extends the abstract class and provides synchronous random ID generation. TheprovidedIn: 'root'ensures singleton behavior.projects/dashboards-demo/src/app/app.config.ts (2)
10-15: Imports correctly updated.The new imports for
SI_WIDGET_ID_PROVIDERfrom@siemens/dashboards-ngare properly added alongside existing dashboard imports.
39-39: Provider registration demonstrates correct usage.This demonstrates the intended pattern for consumers to override the default ID provider. The registration using
useClassis appropriate here.projects/dashboards-ng/README.md (1)
237-253: Provider registration documentation is clear and complete.The examples for both standalone and module-based applications correctly demonstrate the DI registration pattern. The fallback behavior note is helpful for consumers.
projects/dashboards-demo/src/app/app-widget-storage.ts (1)
119-129: Save method signature correctly updated to match new storage API.The updated signature separating
modifiedWidgetsandaddedWidgetsaligns with the new provider-based ID resolution flow. Thesuper.save()call correctly propagates all parameters.projects/dashboards-ng/src/components/grid/si-grid.component.ts (4)
52-52: Provider injection is correctly implemented.The
SI_WIDGET_ID_PROVIDERinjection follows standard Angular DI patterns and integrates cleanly with the existing service injections.
251-258: Save logic correctly partitions modified and added widgets.The implementation efficiently uses a
Setfor O(1) lookups when partitioning widgets. The separation ofmodifiedWidgetsandaddedWidgetsaligns with the new storage API contract.
302-314: Transient ID generation is synchronous by design.Using
transientWidgetIdResolver(which is always synchronous) for new widgets during edit mode is a good design choice. This avoids async complexity in the UI flow while still allowing custom ID generation at save time viawidgetIdResolver.
327-329: Guard prevents transient widgets from being marked for removal.This correctly avoids persisting removal operations for widgets that were never persisted in the first place. The logic is sound.
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1)
215-215: LGTM!The updated
save()call correctly uses the new signature withmodifiedWidgets, emptyaddedWidgets, and emptyremovedWidgetsarrays.projects/dashboards-ng/src/model/si-widget-storage.ts (3)
36-47: LGTM!The updated JSDoc clearly documents the new signature separating
modifiedWidgetsandaddedWidgets, which aligns well with the new ID provider architecture where only added widgets need ID resolution.
96-97: LGTM!Injecting both the custom ID provider and the default provider enables proper fallback behavior when custom ID resolution fails.
148-161: LGTM!The
updatemethod correctly handles both synchronous and async widget configurations by usingPromise.allbefore persisting to storage.
chintankavathia
force-pushed
the
feat/dashboards/widget-id-provider
branch
from
4c0834f to
495e5ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
projects/dashboards-ng/README.md (1)
228-237: Fix parameter name mismatch in async example.Line 228 declares the parameter as
widgetIdResolver(widgetId: WidgetConfig, ...), but line 231 referenceswidget.widgetId, which doesn't match. The parameter should be namedwidgetto be consistent with the sync example (line 210) and to make the code functional.Apply this diff to fix the parameter naming:
- override async widgetIdResolver(widgetId: WidgetConfig, dashboardId?: string): Promise<string> { + override async widgetIdResolver(widget: WidgetConfig, dashboardId?: string): Promise<string> { const response = await firstValueFrom( this.http.post<{ id: string }>('/api/widgets/generate-id', { widgetId: widget.widgetId,
🧹 Nitpick comments (1)
projects/dashboards-ng/src/model/si-widget-storage.ts (1)
126-136: Remove unnecessary.then()call.The
.then()on line 132 is called without a callback, making it a no-op that only adds noise.const resolvedId = await id - .then() .catch(err => this.defaultIdProvider.widgetIdResolver(widget, dashboardId));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
projects/dashboards-demo/src/app/app-widget-id-provider.ts(1 hunks)projects/dashboards-demo/src/app/app-widget-storage.ts(1 hunks)projects/dashboards-demo/src/app/app.config.ts(2 hunks)projects/dashboards-ng/README.md(1 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts(5 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.ts(5 hunks)projects/dashboards-ng/src/model/si-widget-id-provider.ts(1 hunks)projects/dashboards-ng/src/model/si-widget-storage.ts(4 hunks)projects/dashboards-ng/src/public-api.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- projects/dashboards-ng/src/public-api.ts
- projects/dashboards-ng/src/model/si-widget-id-provider.ts
- projects/dashboards-demo/src/app/app.config.ts
🧰 Additional context used
🧬 Code graph analysis (5)
projects/dashboards-ng/src/components/grid/si-grid.component.ts (1)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)
projects/dashboards-demo/src/app/app-widget-storage.ts (1)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (4)
projects/dashboards-ng/src/model/si-widget-storage.ts (1)
SiDefaultWidgetStorage(94-177)projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)projects/dashboards-ng/src/model/configuration.ts (1)
SI_DASHBOARD_CONFIGURATION(21-27)
projects/dashboards-ng/src/model/si-widget-storage.ts (2)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)
projects/dashboards-demo/src/app/app-widget-id-provider.ts (1)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
⏰ 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 (11)
projects/dashboards-demo/src/app/app-widget-storage.ts (1)
119-129: LGTM! Storage API signature updated correctly.The
save()method signature aligns with the updatedSiWidgetStorageabstract class, properly separating modified and added widgets. The delegation tosuper.save()with all parameters is correct.projects/dashboards-demo/src/app/app-widget-id-provider.ts (2)
7-19: LGTM! Clean implementation of custom ID provider.The
widgetIdResolvercorrectly handles the optionaldashboardIdparameter with nullish coalescing. The commented async example provides helpful guidance for consumers who need Promise-based ID generation.
21-27: Delegation to base implementation is appropriate.The
transientWidgetIdResolvercorrectly delegates to the base class, which is a sensible default. The comment indicates this is intentional and can be customized.projects/dashboards-ng/src/components/grid/si-grid.component.ts (3)
251-258: Clean separation of modified and added widgets.The logic correctly uses a
Setto efficiently distinguish between modified (existing) and added (transient) widgets before calling storage. This aligns well with the new storage API contract.
302-306: Transient ID generation using provider.The
addWidgetInstancemethod correctly usestransientWidgetIdResolverfor temporary IDs. The final ID will be resolved by the storage layer duringsave(), which provides clean separation of concerns.
327-329: Good guard to prevent marking transient widgets for removal.This check correctly avoids adding widgets that were never persisted to
markedForRemoval. Transient widgets don't need backend removal since they only exist in the current session.projects/dashboards-ng/src/model/si-widget-storage.ts (2)
138-145: Async ID resolution and aggregation looks correct.The implementation correctly:
- Wraps modified widgets in resolved promises
- Combines with async-resolved added widgets
- Returns an Observable that emits once all IDs are resolved
148-161: Update method signature change handles async resolution.The
updatemethod now correctly accepts both synchronousWidgetConfig[]and asynchronousPromise<WidgetConfig>[]arrays, allowing it to defer persistence until all IDs are resolved.projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (3)
225-259: Well-structured test suite for custom ID resolver.The new test suite correctly:
- Provides a custom
SI_WIDGET_ID_PROVIDERvia DI- Uses correct parameter types matching the provider interface (
widget: WidgetConfigandwidget: Omit<WidgetConfig, 'id'>)- Sets up the testing module consistently with the main suite
261-278: Good test coverage for two-phase ID resolution.This test effectively validates the provider-based ID generation flow:
- Pre-save: widget has transient ID (
transient-custom-id-default)- Post-save: widget has final ID (
custom-id-default)This confirms the separation between
transientWidgetIdResolver(used in grid) andwidgetIdResolver(used in storage during save).
215-215: Updated save() call aligns with new API.The test correctly passes the new signature:
save([widgetConfig], [], [], myDashboardId)where the second array represents added widgets (empty here sincewidgetConfigalready has an ID).
chintankavathia
force-pushed
the
feat/dashboards/widget-id-provider
branch
from
495e5ce to
96de60a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1)
225-279: Custom ID provider test suite exercises both transient and final IDsThe “SiGridComponent with custom id resolver” block wires
SI_WIDGET_ID_PROVIDERwith correctly typed resolver functions and verifies that transient IDs and persisted IDs follow the expectedtransient-custom-*andcustom-*patterns, giving good coverage of the new provider flow.As a tiny cleanup, you could drop the unused
{ provide: SiWidgetStorage, useClass: SiDefaultWidgetStorage }provider in this describe block, sinceSiGridComponentuses theSI_WIDGET_STOREtoken instead.projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
5-62: ID provider abstraction and default implementation are coherent
SI_WIDGET_ID_PROVIDER,SiWidgetIdProvider, andSiWidgetDefaultIdProviderdefine a clear extension point for widget ID generation (including optional async resolution) and integrate cleanly with the rest of the code that now injects this token.If you touch this file again, you might consider tightening the JSDoc text (there’s a stray
*before@example) and optionally mentioning that the default implementations currently ignoredashboardIdso consumers know they need a custom provider when IDs must be dashboard‑scoped.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
projects/dashboards-demo/src/app/app-widget-id-provider.ts(1 hunks)projects/dashboards-demo/src/app/app-widget-storage.ts(1 hunks)projects/dashboards-demo/src/app/app.config.ts(2 hunks)projects/dashboards-ng/README.md(1 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts(5 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.ts(5 hunks)projects/dashboards-ng/src/model/si-widget-id-provider.ts(1 hunks)projects/dashboards-ng/src/model/si-widget-storage.ts(4 hunks)projects/dashboards-ng/src/public-api.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- projects/dashboards-ng/README.md
🧰 Additional context used
🧬 Code graph analysis (6)
projects/dashboards-ng/src/components/grid/si-grid.component.ts (1)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)
projects/dashboards-demo/src/app/app.config.ts (2)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-demo/src/app/app-widget-id-provider.ts (1)
AppWidgetIdProvider(7-28)
projects/dashboards-demo/src/app/app-widget-storage.ts (1)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
projects/dashboards-demo/src/app/app-widget-id-provider.ts (1)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (4)
projects/dashboards-ng/src/model/si-widget-storage.ts (1)
SiDefaultWidgetStorage(94-177)projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)projects/dashboards-ng/src/model/configuration.ts (1)
SI_DASHBOARD_CONFIGURATION(21-27)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
🔇 Additional comments (11)
projects/dashboards-ng/src/public-api.ts (1)
15-15: Expose ID provider types via public APIRe‑exporting
./model/si-widget-id-providerhere is correct and keeps the new ID provider contract available to consumers.projects/dashboards-demo/src/app/app.config.ts (1)
10-15: Custom widget ID provider is wired correctly in demo app configThe added imports and
{ provide: SI_WIDGET_ID_PROVIDER, useClass: AppWidgetIdProvider }registration cleanly hook the demo into the new ID provider mechanism while keeping existing store/config wiring intact.Also applies to: 19-19, 39-39
projects/dashboards-demo/src/app/app-widget-storage.ts (1)
119-129: Updated save signature correctly delegates to base storageThe override matches the new
(modifiedWidgets, addedWidgets, removedWidgets?, dashboardId?)contract and cleanly forwards arguments toSiDefaultWidgetStorage.savewhile preserving the demo’s artificial delay behavior.projects/dashboards-demo/src/app/app-widget-id-provider.ts (1)
5-28: Demo ID provider correctly implementsSiWidgetIdProviderThe
AppWidgetIdProvideroverride signatures match the base contract and provide a clear example of custom final IDs plus reuse of the default transient ID behavior; this is a good, minimal demo implementation.projects/dashboards-ng/src/components/grid/si-grid.component.ts (4)
26-27: InjectingSI_WIDGET_ID_PROVIDERinto grid is consistent with the new ID abstractionThe new import and
widgetIdProviderinjection integrate the grid cleanly with the configurable ID provider without affecting existing signals or store wiring.Also applies to: 52-52
251-259: Splitting widgets into modified vs added before save is logically soundUpdating positions once and then partitioning via
transientWidgetIdsgivesmodifiedWidgetsandaddedWidgetsthat together represent the current visible state, matching the newSiWidgetStorage.save(modified, added, removed, dashboardId)contract.
302-307: Using the transient ID resolver for new widgets matches the provider contract
addWidgetInstancenow delegates ID creation towidgetIdProvider.transientWidgetIdResolver, ensuring custom providers control transient IDs and keeping the method synchronous as designed.
327-329: Avoiding removal tracking for transient widgets is correctThe added guard prevents never‑persisted transient instances from being added to
markedForRemoval, so storage only receives true deletions of previously persisted widgets.projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1)
149-157: Tests correctly reflect new save contract and grid behaviorThe updated specs use the new
save(modified, added, removed, dashboardId)signature and still validateremoveWidget,handleGridEvent, and dashboardId‑driven reload behavior under the new storage semantics.Also applies to: 198-209, 211-221
projects/dashboards-ng/src/model/si-widget-storage.ts (2)
7-11: RevisedSiWidgetStorage.savecontract matches new widget lifecycleThe abstract
save(modifiedWidgets, addedWidgets, removedWidgets?, dashboardId?)plus updated documentation aligns with how the grid now separates modified vs newly added widgets and gives storage implementations all necessary context for persistence and removals.Also applies to: 35-55
95-106: Default storage correctly integrates the ID provider into async save flow
SiDefaultWidgetStoragenow injects the configurable ID provider and uses it to resolve IDs foraddedWidgets(including Promise‑returning resolvers), combines them withmodifiedWidgets, and updates both in‑memory subjects and backing storage afterPromise.allhas resolved.Also applies to: 120-146, 148-161
timowolf
left a comment
timowolf
left a comment
There was a problem hiding this comment.
Many thanks. I think we only need one generateId function that might be overwritten by injection. See my comments.
chintankavathia
force-pushed
the
feat/dashboards/widget-id-provider
branch
from
96de60a to
4c02328
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (2)
149-157:asyncdeclaration is unnecessary here.The test function is marked
asyncbut doesn't contain anyawaitexpressions. Consider removingasyncfor clarity.🔧 Suggested fix:
- it('#removeWidget() shall remove WidgetConfig from visible widgets', async () => { + it('#removeWidget() shall remove WidgetConfig from visible widgets', () => {
234-241: Mock type could be more precise.The mock's
generateWidgetIdparameter type isWidgetConfigbut theSiWidgetIdProviderinterface declares it asOmit<WidgetConfig, 'id'>. While structurally compatible, using the exact type would better document the contract.🔧 Suggested fix:
{ provide: SI_WIDGET_ID_PROVIDER, useValue: { - generateWidgetId: (widget: WidgetConfig, dashboardId?: string) => { + generateWidgetId: (widget: Omit<WidgetConfig, 'id'>, dashboardId?: string) => { return `custom-${widget.widgetId}-${dashboardId ?? 'default'}`; } } },
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
projects/dashboards-demo/src/app/app-widget-id-provider.ts(1 hunks)projects/dashboards-demo/src/app/app-widget-storage.ts(1 hunks)projects/dashboards-demo/src/app/app.config.ts(2 hunks)projects/dashboards-ng/README.md(1 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts(5 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.ts(5 hunks)projects/dashboards-ng/src/model/si-widget-id-provider.ts(1 hunks)projects/dashboards-ng/src/model/si-widget-storage.ts(4 hunks)projects/dashboards-ng/src/public-api.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- projects/dashboards-demo/src/app/app-widget-storage.ts
- projects/dashboards-ng/src/components/grid/si-grid.component.ts
- projects/dashboards-ng/README.md
- projects/dashboards-demo/src/app/app.config.ts
- projects/dashboards-ng/src/public-api.ts
- projects/dashboards-demo/src/app/app-widget-id-provider.ts
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-12-17T04:34:55.597Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1173
File: src/app/examples/si-charts/interactive/interactive.ts:5-5
Timestamp: 2025-12-17T04:34:55.597Z
Learning: Guideline: In Angular projects using OnPush change detection (zoneless mode), you only need to call ChangeDetectorRef.markForCheck() for asynchronous operations (e.g., setTimeout, setInterval, Promises, Observables) where a change is not automatically detected. Event handlers triggered by Angular template bindings (click, input, etc.) will trigger change detection automatically and do not require explicit markForCheck() calls. Apply this broadly to TS files within Angular components/services that use OnPush; use markForCheck() selectively for async work where change detection wouldn’t run otherwise.
Applied to files:
projects/dashboards-ng/src/model/si-widget-id-provider.tsprojects/dashboards-ng/src/model/si-widget-storage.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-15T07:17:06.981Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/widget-instance-editor-dialog/si-widget-instance-editor-dialog.component.spec.ts:37-49
Timestamp: 2025-12-15T07:17:06.981Z
Learning: In widget-instance-editor-dialog component tests (projects/dashboards-ng/src/components/widget-instance-editor-dialog/si-widget-instance-editor-dialog.component.spec.ts), real setTimeout waits (e.g., `await new Promise(resolve => setTimeout(resolve, 0))`) must be used instead of `fixture.whenStable()` when testing component initialization. This is likely due to the component's lifecycle behavior or modal initialization timing that fixture.whenStable() cannot properly wait for in zoneless mode.
Applied to files:
projects/dashboards-ng/src/model/si-widget-storage.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-17T06:59:43.496Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1036
File: projects/dashboards-ng/src/widget-loader.ts:176-182
Timestamp: 2025-12-17T06:59:43.496Z
Learning: In projects/dashboards-ng/src/widget-loader.ts, when checking `factory.moduleName`, there's no need to also check for `factory.moduleLoader` because the TypeScript type system (ModuleOptions type) enforces that `moduleLoader` must be present whenever `moduleName` is provided.
Applied to files:
projects/dashboards-ng/src/model/si-widget-storage.ts
📚 Learning: 2025-12-15T07:16:53.762Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts:92-105
Timestamp: 2025-12-15T07:16:53.762Z
Learning: In gridstack-wrapper component tests (projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts), real setTimeout waits must be used instead of fixture.whenStable() to avoid injector destroyed errors during mount/unmount operations in zoneless mode. The GridStack timing-dependent operations require actual async delays.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-15T07:16:32.082Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts:69-80
Timestamp: 2025-12-15T07:16:32.082Z
Learning: In gridstack-wrapper component tests (projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts), real setTimeout waits (e.g., `await new Promise(resolve => setTimeout(resolve, 0))`) must be used instead of `fixture.whenStable()` when testing grid item rendering and initialization. This is likely due to gridstack library's initialization timing or lifecycle behavior that fixture.whenStable() cannot properly wait for in zoneless mode.
Applied to files:
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/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-11T10:09:01.564Z
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:09:01.564Z
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/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-08T11:25:51.584Z
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.584Z
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
📚 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
📚 Learning: 2025-12-08T11:24:45.272Z
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.272Z
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/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-15T10:05:59.100Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1193
File: projects/element-ng/search-bar/si-search-bar.component.spec.ts:21-28
Timestamp: 2025-12-15T10:05:59.100Z
Learning: In tests that rely on Jasmine's fake clock to control time-based RxJS operators (e.g., debounceTime), use jasmine.clock().install() together with jasmine.clock().mockDate() to enable precise timing control. Without mockDate, scheduler-based operators may not respond to the fake clock. Apply this guidance to spec files (e.g., any file named *.spec.ts or *.spec.js) that test time-sensitive behavior, ensuring you install and mockDate before advancing time in tests.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
🧬 Code graph analysis (2)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (3)
projects/dashboards-ng/src/model/si-widget-storage.ts (1)
SiDefaultWidgetStorage(94-159)projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
🔇 Additional comments (7)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (2)
9-24: Well-structured injection token with sensible defaults.The token is correctly configured with
providedIn: 'root'and a factory that injects the default provider, enabling tree-shaking while providing a clean override mechanism.
26-38: Clean abstract class design.The abstract class properly defines the contract for custom ID providers with appropriate parameter types.
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (2)
14-22: Clean import updates for the new ID provider feature.The imports correctly bring in
SI_WIDGET_ID_PROVIDERand remove the obsoleteNEW_WIDGET_PREFIXdependency.
258-267: Good test coverage for custom ID provider integration.The test validates the end-to-end flow of custom ID generation, ensuring the provider is correctly wired and the expected ID format is produced.
projects/dashboards-ng/src/model/si-widget-storage.ts (3)
94-104: Clean dependency injection for the ID provider.Using field initialization with
inject()is the modern Angular pattern and correctly wires the provider for ID generation.
119-133: Solid implementation of the new save signature.The logic correctly:
- Maps over
addedWidgetsto generate IDs via the provider- Combines resolved added widgets with modified widgets
- Delegates to
update()for persistenceNote that
removedWidgetsis intentionally unused here since this default implementation replaces the entire widget list — the removed widgets are implicitly excluded. Custom implementations can use this parameter for optimized delta-based persistence.
135-143: Streamlined update method.The refactored
updatemethod cleanly accepts resolvedWidgetConfig[]and handles both BehaviorSubject emission and storage persistence.
chintankavathia
force-pushed
the
feat/dashboards/widget-id-provider
branch
from
4c02328 to
c7a2b5a
Compare
chintankavathia
force-pushed
the
feat/dashboards/widget-id-provider
branch
from
c7a2b5a to
459ba75
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
46-58: Fix documentation/implementation mismatch for ID format.The JSDoc on line 54 states the format is
${dashboardId}-${widget.widgetId}-${crypto.randomUUID()}, but the implementation on line 57 produces${dashboardId ?? ''}${widget.widgetId}-${crypto.randomUUID()}— missing the hyphen separator betweendashboardIdandwidget.widgetIdwhendashboardIdis provided.For example, with
dashboardId="dash1"andwidgetId="widget1":
- Documentation implies:
dash1-widget1-<uuid>- Code produces:
dash1widget1-<uuid>🔎 Apply this diff to add the hyphen separator:
override generateWidgetId(widget: Omit<WidgetConfig, 'id'>, dashboardId?: string): string { - return `${dashboardId ?? ''}${widget.widgetId}-${crypto.randomUUID()}`; + return `${dashboardId ? `${dashboardId}-` : ''}${widget.widgetId}-${crypto.randomUUID()}`; }Alternatively, update the JSDoc to match the current implementation if this behavior is intentional.
projects/dashboards-ng/src/model/si-widget-storage.ts (1)
36-47: Update JSDoc to reference the correct method name.Line 37 references
SiWidgetIdProvider.transientWidgetIdResolver, but the actual method in theSiWidgetIdProviderinterface isgenerateWidgetId. This creates confusion about the API.🔎 Apply this diff to correct the method reference:
- * generated through the `SiWidgetIdProvider.transientWidgetIdResolver` implementation + * generated through the `SiWidgetIdProvider.generateWidgetId` implementation
🧹 Nitpick comments (1)
projects/dashboards-ng/src/model/si-widget-storage.ts (1)
96-96: Verify if injectedidProvideris intentionally unused.The
idProvideris injected on line 96 but doesn't appear to be referenced anywhere in the implementation. Based on the PR objectives, IDs are generated by the grid component before reaching storage, so the provider may not be needed here.If this field is not used and not planned for future use, consider removing it to avoid confusion:
- private idProvider = inject(SI_WIDGET_ID_PROVIDER);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
projects/dashboards-demo/src/app/app-widget-id-provider.ts(1 hunks)projects/dashboards-demo/src/app/app-widget-storage.ts(1 hunks)projects/dashboards-demo/src/app/app.config.ts(2 hunks)projects/dashboards-ng/README.md(1 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts(5 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.ts(5 hunks)projects/dashboards-ng/src/model/si-widget-id-provider.ts(1 hunks)projects/dashboards-ng/src/model/si-widget-storage.ts(4 hunks)projects/dashboards-ng/src/public-api.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- projects/dashboards-ng/src/public-api.ts
- projects/dashboards-demo/src/app/app.config.ts
- projects/dashboards-demo/src/app/app-widget-id-provider.ts
- projects/dashboards-ng/README.md
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-12-15T07:17:06.981Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/widget-instance-editor-dialog/si-widget-instance-editor-dialog.component.spec.ts:37-49
Timestamp: 2025-12-15T07:17:06.981Z
Learning: In widget-instance-editor-dialog component tests (projects/dashboards-ng/src/components/widget-instance-editor-dialog/si-widget-instance-editor-dialog.component.spec.ts), real setTimeout waits (e.g., `await new Promise(resolve => setTimeout(resolve, 0))`) must be used instead of `fixture.whenStable()` when testing component initialization. This is likely due to the component's lifecycle behavior or modal initialization timing that fixture.whenStable() cannot properly wait for in zoneless mode.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.tsprojects/dashboards-ng/src/model/si-widget-storage.ts
📚 Learning: 2025-12-15T07:16:32.082Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts:69-80
Timestamp: 2025-12-15T07:16:32.082Z
Learning: In gridstack-wrapper component tests (projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts), real setTimeout waits (e.g., `await new Promise(resolve => setTimeout(resolve, 0))`) must be used instead of `fixture.whenStable()` when testing grid item rendering and initialization. This is likely due to gridstack library's initialization timing or lifecycle behavior that fixture.whenStable() cannot properly wait for in zoneless mode.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-15T07:16:53.762Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts:92-105
Timestamp: 2025-12-15T07:16:53.762Z
Learning: In gridstack-wrapper component tests (projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts), real setTimeout waits must be used instead of fixture.whenStable() to avoid injector destroyed errors during mount/unmount operations in zoneless mode. The GridStack timing-dependent operations require actual async delays.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-17T04:34:55.597Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1173
File: src/app/examples/si-charts/interactive/interactive.ts:5-5
Timestamp: 2025-12-17T04:34:55.597Z
Learning: Guideline: In Angular projects using OnPush change detection (zoneless mode), you only need to call ChangeDetectorRef.markForCheck() for asynchronous operations (e.g., setTimeout, setInterval, Promises, Observables) where a change is not automatically detected. Event handlers triggered by Angular template bindings (click, input, etc.) will trigger change detection automatically and do not require explicit markForCheck() calls. Apply this broadly to TS files within Angular components/services that use OnPush; use markForCheck() selectively for async work where change detection wouldn’t run otherwise.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.tsprojects/dashboards-ng/src/model/si-widget-id-provider.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.tsprojects/dashboards-demo/src/app/app-widget-storage.tsprojects/dashboards-ng/src/model/si-widget-storage.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/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-11T10:09:01.564Z
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:09:01.564Z
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/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-08T11:25:51.584Z
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.584Z
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
📚 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
📚 Learning: 2025-12-08T11:24:45.272Z
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.272Z
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/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-15T10:05:59.100Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1193
File: projects/element-ng/search-bar/si-search-bar.component.spec.ts:21-28
Timestamp: 2025-12-15T10:05:59.100Z
Learning: In tests that rely on Jasmine's fake clock to control time-based RxJS operators (e.g., debounceTime), use jasmine.clock().install() together with jasmine.clock().mockDate() to enable precise timing control. Without mockDate, scheduler-based operators may not respond to the fake clock. Apply this guidance to spec files (e.g., any file named *.spec.ts or *.spec.js) that test time-sensitive behavior, ensuring you install and mockDate before advancing time in tests.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-17T06:59:43.496Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1036
File: projects/dashboards-ng/src/widget-loader.ts:176-182
Timestamp: 2025-12-17T06:59:43.496Z
Learning: In projects/dashboards-ng/src/widget-loader.ts, when checking `factory.moduleName`, there's no need to also check for `factory.moduleLoader` because the TypeScript type system (ModuleOptions type) enforces that `moduleLoader` must be present whenever `moduleName` is provided.
Applied to files:
projects/dashboards-ng/src/model/si-widget-storage.ts
🧬 Code graph analysis (4)
projects/dashboards-ng/src/components/grid/si-grid.component.ts (1)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (4)
projects/dashboards-ng/src/model/si-widget-storage.ts (1)
SiDefaultWidgetStorage(94-154)projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)projects/dashboards-ng/src/model/configuration.ts (1)
SI_DASHBOARD_CONFIGURATION(21-27)
projects/dashboards-demo/src/app/app-widget-storage.ts (1)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
🔇 Additional comments (10)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
26-38: LGTM! Clear abstraction for widget ID generation.The abstract class provides a clean contract for custom ID providers, with appropriate typing using
Omit<WidgetConfig, 'id'>to ensure new widgets don't have IDs yet.projects/dashboards-demo/src/app/app-widget-storage.ts (1)
119-129: LGTM! Signature correctly updated for new storage API.The save method properly separates
modifiedWidgetsandaddedWidgetsparameters and passes them to the base implementation. The demo delay logic is appropriately preserved.projects/dashboards-ng/src/components/grid/si-grid.component.ts (4)
26-26: LGTM! Proper DI setup for widget ID provider.The component correctly imports and injects the
SI_WIDGET_ID_PROVIDERtoken to support custom ID generation.Also applies to: 52-52
251-258: LGTM! Clean partition logic for modified vs added widgets.The save method correctly partitions widgets into
modifiedWidgets(existing) andaddedWidgets(transient) using a Set-based lookup, eliminating the previous prefix-based approach. The storage API call properly passes both arrays.
302-311: LGTM! Provider-based ID generation properly integrated.The method now uses
widgetIdProvider.generateWidgetId()to generate IDs for new widget instances, replacing the previous counter-based approach. The generated ID is correctly assigned and the widget is tracked intransientWidgetInstances.
319-333: LGTM! Correct handling of transient vs persisted widget removal.The method properly checks if a widget is transient before adding it to
markedForRemoval. Transient widgets (not yet saved) are simply removed from the visible list, while persisted widgets are marked for deletion from storage.projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (2)
211-222: LGTM! Test correctly updated for new storage API.The test properly calls
widgetStorage.save()with the new signature, passing the widget as a modified widget (first parameter) and empty arrays for added and removed widgets.
225-268: LGTM! Comprehensive test for custom ID provider integration.The new test suite properly:
- Configures a custom
SI_WIDGET_ID_PROVIDERwith a mock implementation- Verifies that widget IDs are generated using the custom provider
- Checks the expected ID format (
custom-id-default)The mock signature correctly matches the
SiWidgetIdProviderinterface.projects/dashboards-ng/src/model/si-widget-storage.ts (2)
119-128: LGTM! Save implementation correctly handles separated widget lists.The method properly accepts
modifiedWidgetsandaddedWidgetsas separate parameters, combines them for persistence, and returns the merged list. The implementation is clean and straightforward.
130-138: LGTM! Update method correctly persists widget configurations.The method properly updates the BehaviorSubject and persists to storage, with correct handling of both default and dashboard-specific storage keys.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
46-58: Documentation does not match implementation format.The JSDoc at line 54 states the format is
${dashboardId}-${widget.widgetId}-${crypto.randomUUID()}, but the implementation at line 57 produces${dashboardId ?? ''}${widget.widgetId}-${crypto.randomUUID()}— missing the hyphen betweendashboardIdandwidget.widgetIdwhendashboardIdis provided.For
dashboardId="dash1"andwidgetId="widget1":
- Doc implies:
dash1-widget1-<uuid>- Code produces:
dash1widget1-<uuid>Either update the implementation to add the separator or fix the JSDoc to match.
🔧 Suggested fix — add hyphen separator when dashboardId is present:
override generateWidgetId(widget: Omit<WidgetConfig, 'id'>, dashboardId?: string): string { - return `${dashboardId ?? ''}${widget.widgetId}-${crypto.randomUUID()}`; + return `${dashboardId ? `${dashboardId}-` : ''}${widget.widgetId}-${crypto.randomUUID()}`; }projects/dashboards-ng/src/model/si-widget-storage.ts (1)
36-47: Address outstanding documentation issues.Multiple reviewers have flagged issues with this documentation that still need to be addressed:
Line 37: References non-existent
SiWidgetIdProvider.transientWidgetIdResolver(should refer togenerateWidgetIdor clarify that IDs are assigned beforesave()is called).Lines 38-39: States "it is in the responsibility of the implementor to set the ids of the new widgets," but the signature shows
addedWidgets: WidgetConfig[]which already have IDs assigned. This is contradictory.Lines 39-41: The sentence about checking for removed widgets is unclear (as noted by timowolf).
Per timowolf's suggestion, consider simplifying this to clarify that IDs are assigned earlier in the flow and that the method receives widgets that are already identified.
🧹 Nitpick comments (2)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (1)
234-241: Mock parameter type should useOmit<WidgetConfig, 'id'>.The mock's
generateWidgetIdsignature useswidget: WidgetConfig, but theSiWidgetIdProviderabstract class defines the parameter aswidget: Omit<WidgetConfig, 'id'>. While this works at runtime (TypeScript structural typing), the type annotation should match the interface for clarity and type safety.🔧 Suggested fix:
{ provide: SI_WIDGET_ID_PROVIDER, useValue: { - generateWidgetId: (widget: WidgetConfig, dashboardId?: string) => { + generateWidgetId: (widget: Omit<WidgetConfig, 'id'>, dashboardId?: string) => { return `custom-${widget.widgetId}-${dashboardId ?? 'default'}`; } } },projects/dashboards-demo/src/app/app-widget-storage.ts (1)
51-57: Consider using the ID provider for consistency.The
saveAsDefaultsmethod generates IDs usingMath.random().toString(36).substring(2, 9)rather than the injectedSI_WIDGET_ID_PROVIDER. While this is demo code and may be intentional, using the provider would ensure consistent ID generation across the application.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
projects/dashboards-demo/src/app/app-widget-id-provider.ts(1 hunks)projects/dashboards-demo/src/app/app-widget-storage.ts(1 hunks)projects/dashboards-demo/src/app/app.config.ts(2 hunks)projects/dashboards-ng/README.md(1 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts(5 hunks)projects/dashboards-ng/src/components/grid/si-grid.component.ts(5 hunks)projects/dashboards-ng/src/model/si-widget-id-provider.ts(1 hunks)projects/dashboards-ng/src/model/si-widget-storage.ts(4 hunks)projects/dashboards-ng/src/public-api.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- projects/dashboards-demo/src/app/app.config.ts
- projects/dashboards-ng/README.md
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-12-17T04:34:55.597Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1173
File: src/app/examples/si-charts/interactive/interactive.ts:5-5
Timestamp: 2025-12-17T04:34:55.597Z
Learning: Guideline: In Angular projects using OnPush change detection (zoneless mode), you only need to call ChangeDetectorRef.markForCheck() for asynchronous operations (e.g., setTimeout, setInterval, Promises, Observables) where a change is not automatically detected. Event handlers triggered by Angular template bindings (click, input, etc.) will trigger change detection automatically and do not require explicit markForCheck() calls. Apply this broadly to TS files within Angular components/services that use OnPush; use markForCheck() selectively for async work where change detection wouldn’t run otherwise.
Applied to files:
projects/dashboards-ng/src/public-api.tsprojects/dashboards-demo/src/app/app-widget-storage.tsprojects/dashboards-ng/src/model/si-widget-id-provider.tsprojects/dashboards-ng/src/components/grid/si-grid.component.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.tsprojects/dashboards-ng/src/model/si-widget-storage.tsprojects/dashboards-demo/src/app/app-widget-id-provider.ts
📚 Learning: 2025-12-15T07:17:06.981Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/widget-instance-editor-dialog/si-widget-instance-editor-dialog.component.spec.ts:37-49
Timestamp: 2025-12-15T07:17:06.981Z
Learning: In widget-instance-editor-dialog component tests (projects/dashboards-ng/src/components/widget-instance-editor-dialog/si-widget-instance-editor-dialog.component.spec.ts), real setTimeout waits (e.g., `await new Promise(resolve => setTimeout(resolve, 0))`) must be used instead of `fixture.whenStable()` when testing component initialization. This is likely due to the component's lifecycle behavior or modal initialization timing that fixture.whenStable() cannot properly wait for in zoneless mode.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.tsprojects/dashboards-ng/src/model/si-widget-storage.ts
📚 Learning: 2025-12-15T07:16:32.082Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts:69-80
Timestamp: 2025-12-15T07:16:32.082Z
Learning: In gridstack-wrapper component tests (projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts), real setTimeout waits (e.g., `await new Promise(resolve => setTimeout(resolve, 0))`) must be used instead of `fixture.whenStable()` when testing grid item rendering and initialization. This is likely due to gridstack library's initialization timing or lifecycle behavior that fixture.whenStable() cannot properly wait for in zoneless mode.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-15T07:16:53.762Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts:92-105
Timestamp: 2025-12-15T07:16:53.762Z
Learning: In gridstack-wrapper component tests (projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts), real setTimeout waits must be used instead of fixture.whenStable() to avoid injector destroyed errors during mount/unmount operations in zoneless mode. The GridStack timing-dependent operations require actual async delays.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.tsprojects/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/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-11T10:09:01.564Z
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:09:01.564Z
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/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-08T11:25:51.584Z
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.584Z
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
📚 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
📚 Learning: 2025-12-08T11:24:45.272Z
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.272Z
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/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-15T10:05:59.100Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1193
File: projects/element-ng/search-bar/si-search-bar.component.spec.ts:21-28
Timestamp: 2025-12-15T10:05:59.100Z
Learning: In tests that rely on Jasmine's fake clock to control time-based RxJS operators (e.g., debounceTime), use jasmine.clock().install() together with jasmine.clock().mockDate() to enable precise timing control. Without mockDate, scheduler-based operators may not respond to the fake clock. Apply this guidance to spec files (e.g., any file named *.spec.ts or *.spec.js) that test time-sensitive behavior, ensuring you install and mockDate before advancing time in tests.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-17T06:59:43.496Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1036
File: projects/dashboards-ng/src/widget-loader.ts:176-182
Timestamp: 2025-12-17T06:59:43.496Z
Learning: In projects/dashboards-ng/src/widget-loader.ts, when checking `factory.moduleName`, there's no need to also check for `factory.moduleLoader` because the TypeScript type system (ModuleOptions type) enforces that `moduleLoader` must be present whenever `moduleName` is provided.
Applied to files:
projects/dashboards-ng/src/model/si-widget-storage.tsprojects/dashboards-demo/src/app/app-widget-id-provider.ts
🧬 Code graph analysis (5)
projects/dashboards-demo/src/app/app-widget-storage.ts (1)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
projects/dashboards-ng/src/components/grid/si-grid.component.ts (1)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (3)
projects/dashboards-ng/src/model/si-widget-storage.ts (1)
SiDefaultWidgetStorage(94-154)projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)
projects/dashboards-ng/src/model/si-widget-storage.ts (2)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(115-177)projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)
🔇 Additional comments (11)
projects/dashboards-demo/src/app/app-widget-id-provider.ts (1)
1-5: LGTM!The imports are appropriate and the copyright header is correct.
projects/dashboards-ng/src/public-api.ts (1)
15-15: LGTM!The new export for
si-widget-id-provideris correctly placed alongside other model exports, enabling consumers to provide custom widget ID generation logic.projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
21-24: LGTM!The injection token setup with a factory defaulting to
SiWidgetDefaultIdProvideris a clean pattern that allows consumers to override while providing sensible defaults.projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (2)
258-267: LGTM!The test properly validates that the custom ID provider is used for widget ID generation, verifying the expected
custom-id-defaultformat. Good coverage for the new provider injection feature.
211-222: LGTM!The
save()call correctly uses the new signature with separatedmodifiedWidgets,addedWidgets, andremovedWidgetsparameters.projects/dashboards-ng/src/components/grid/si-grid.component.ts (3)
251-258: LGTM!The save flow correctly partitions widgets into
modifiedWidgets(existing persisted widgets) andaddedWidgets(transient widgets added in edit mode) using a Set for efficient lookup. This cleanly separates the concerns for the updated storage API.
302-311: LGTM!The
addWidgetInstancemethod cleanly integrates with the widget ID provider for synchronous ID generation. The approach aligns with the discussion in past reviews where synchronous errors from custom providers should fail fast for immediate visibility during development.
319-333: LGTM!The logic correctly avoids marking transient widgets for removal since they were never persisted to the backend. Only widgets that exist in
persistedWidgetInstances(i.e., not intransientWidgetInstances) should be added tomarkedForRemoval.projects/dashboards-demo/src/app/app-widget-storage.ts (1)
119-129: LGTM!The
save()method signature is correctly updated to match the newSiDefaultWidgetStorageAPI, properly forwardingmodifiedWidgets,addedWidgets, andremovedWidgetsto the parent implementation while maintaining the demo's simulated delay behavior.projects/dashboards-ng/src/model/si-widget-storage.ts (2)
119-128: LGTM - simplified persistence implementation.The implementation correctly merges
addedWidgetsandmodifiedWidgetsfor persistence without performing ID generation. This aligns with the design decision (per timowolf's feedback) that ID generation happens earlier in the flow, and by the timesave()is called, all widgets already have their IDs assigned.
130-138: LGTM - cleaner synchronous update.The
update()method has been simplified to acceptWidgetConfig[]directly instead of dealing with async operations. This removes the complexity around Promise resolution and error handling, making the code cleaner and more maintainable.
chintankavathia
force-pushed
the
feat/dashboards/widget-id-provider
branch
6 times, most recently
from
08ae388 to
bfd67b8
Compare
timowolf
force-pushed
the
feat/dashboards/widget-id-provider
branch
from
bfd67b8 to
1e5e3cf
Compare
kfenner
force-pushed
the
feat/dashboards/widget-id-provider
branch
from
1e5e3cf to
28ce4c9
Compare
|
@timowolf @spike-rabbit Looks like the goldens are outdated: |
chintankavathia
force-pushed
the
feat/dashboards/widget-id-provider
branch
from
28ce4c9 to
1199318
Compare
- Add `SiWidgetIdProvider` abstract class and injection token for custom widget ID generation - Implement `SiWidgetDefaultIdProvider` with random ID generation as default - Update `SiGridComponent` to use widget ID provider - Update `SiDefaultWidgetStorage` to support custom ID provider for new widgets - Add documentation in README.md for widget instance ID generation - Add example `AppWidgetIdProvider` implementation in dashboards-demo - Remove hardcoded `NEW_WIDGET_PREFIX` and counter-based ID generation - Add unit tests for custom ID provider integration BREAKING CHANGE: New widgets are now assigned IDs upon creation using the `SiWidgetIdProvider.generateWidgetId`, rather than using temporary `NEW_WIDGET_PREFIX` IDs that are replaced on save. The `save()` method signature in the abstract class `SiWidgetStorage` has been modified to separate existing widgets from new widgets: Before: ``` abstract save( widgets: (WidgetConfig | Omit<WidgetConfig, 'id'>)[], removedWidgets?: WidgetConfig[], dashboardId?: string ): Observable<WidgetConfig[]>; ``` After: ``` abstract save( modifiedWidgets: WidgetConfig[], addedWidgets: WidgetConfig[], removedWidgets?: WidgetConfig[], dashboardId?: string ): Observable<WidgetConfig[]>; ``` `addedWidgets` array will contain new widgets with ids generated by `SiWidgetIdProvider.generateWidgetId`.
kfenner
force-pushed
the
feat/dashboards/widget-id-provider
branch
from
1199318 to
3bb7737
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/dashboards-demo/src/app/app-widget-storage.ts (1)
52-54: Ad-hoc ID generation bypasses the new ID provider infrastructure.This code directly mutates widget IDs using
Math.random()instead of using theSI_WIDGET_ID_PROVIDERintroduced in this PR. This is inconsistent with the new pluggable ID provider pattern and bypasses any custom ID generation logic (like theAppWidgetIdProviderregistered in the demo app).Consider injecting
SI_WIDGET_ID_PROVIDERand using it to generate IDs:🔎 Suggested approach using the ID provider
Inject the provider in the constructor:
+import { inject } from '@angular/core'; +import { SI_WIDGET_ID_PROVIDER } from '@siemens/dashboards-ng'; + export class AppWidgetStorage extends SiDefaultWidgetStorage implements SiWidgetStorage { + private idProvider = inject(SI_WIDGET_ID_PROVIDER); private secondaryMenuItems = new BehaviorSubject<DashboardToolbarItem[]>([]);Then use it in
saveAsDefaults:saveAsDefaults = (grid: SiGridComponent): Observable<void> => { - grid.transientWidgetInstances.forEach( - widget => (widget.id = Math.random().toString(36).substring(2, 9)) - ); + grid.transientWidgetInstances.forEach(widget => { + widget.id = this.idProvider.generateWidgetId(widget, grid.dashboardId()); + }); this.storage.setItem(CUSTOM_DEFAULTS, JSON.stringify(grid.visibleWidgetInstances$.value)); return of(); };
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
api-goldens/dashboards-ng/index.api.mdprojects/dashboards-demo/src/app/app-widget-id-provider.tsprojects/dashboards-demo/src/app/app-widget-storage.tsprojects/dashboards-demo/src/app/app.config.tsprojects/dashboards-ng/README.mdprojects/dashboards-ng/src/components/grid/si-grid.component.spec.tsprojects/dashboards-ng/src/components/grid/si-grid.component.tsprojects/dashboards-ng/src/model/si-widget-id-provider.tsprojects/dashboards-ng/src/model/si-widget-storage.tsprojects/dashboards-ng/src/public-api.ts
🧰 Additional context used
🧠 Learnings (15)
📚 Learning: 2025-12-17T06:59:51.485Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1036
File: projects/dashboards-ng/src/widget-loader.ts:176-182
Timestamp: 2025-12-17T06:59:51.485Z
Learning: In projects/dashboards-ng/src/widget-loader.ts, when checking `factory.moduleName`, there's no need to also check for `factory.moduleLoader` because the TypeScript type system (ModuleOptions type) enforces that `moduleLoader` must be present whenever `moduleName` is provided.
Applied to files:
projects/dashboards-demo/src/app/app-widget-id-provider.tsprojects/dashboards-ng/README.mdprojects/dashboards-ng/src/model/si-widget-storage.ts
📚 Learning: 2025-12-17T04:34:55.597Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1173
File: src/app/examples/si-charts/interactive/interactive.ts:5-5
Timestamp: 2025-12-17T04:34:55.597Z
Learning: Guideline: In Angular projects using OnPush change detection (zoneless mode), you only need to call ChangeDetectorRef.markForCheck() for asynchronous operations (e.g., setTimeout, setInterval, Promises, Observables) where a change is not automatically detected. Event handlers triggered by Angular template bindings (click, input, etc.) will trigger change detection automatically and do not require explicit markForCheck() calls. Apply this broadly to TS files within Angular components/services that use OnPush; use markForCheck() selectively for async work where change detection wouldn’t run otherwise.
Applied to files:
projects/dashboards-demo/src/app/app-widget-id-provider.tsprojects/dashboards-ng/src/public-api.tsprojects/dashboards-demo/src/app/app-widget-storage.tsprojects/dashboards-ng/src/components/grid/si-grid.component.spec.tsprojects/dashboards-ng/src/components/grid/si-grid.component.tsprojects/dashboards-ng/src/model/si-widget-id-provider.tsprojects/dashboards-demo/src/app/app.config.tsprojects/dashboards-ng/src/model/si-widget-storage.ts
📚 Learning: 2025-12-15T07:16:53.762Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts:92-105
Timestamp: 2025-12-15T07:16:53.762Z
Learning: In gridstack-wrapper component tests (projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts), real setTimeout waits must be used instead of fixture.whenStable() to avoid injector destroyed errors during mount/unmount operations in zoneless mode. The GridStack timing-dependent operations require actual async delays.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.spec.tsprojects/dashboards-ng/src/components/grid/si-grid.component.ts
📚 Learning: 2025-12-15T07:16:32.082Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts:69-80
Timestamp: 2025-12-15T07:16:32.082Z
Learning: In gridstack-wrapper component tests (projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts), real setTimeout waits (e.g., `await new Promise(resolve => setTimeout(resolve, 0))`) must be used instead of `fixture.whenStable()` when testing grid item rendering and initialization. This is likely due to gridstack library's initialization timing or lifecycle behavior that fixture.whenStable() cannot properly wait for in zoneless mode.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.spec.tsprojects/dashboards-ng/src/components/grid/si-grid.component.ts
📚 Learning: 2025-12-15T07:17:06.981Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/widget-instance-editor-dialog/si-widget-instance-editor-dialog.component.spec.ts:37-49
Timestamp: 2025-12-15T07:17:06.981Z
Learning: In widget-instance-editor-dialog component tests (projects/dashboards-ng/src/components/widget-instance-editor-dialog/si-widget-instance-editor-dialog.component.spec.ts), real setTimeout waits (e.g., `await new Promise(resolve => setTimeout(resolve, 0))`) must be used instead of `fixture.whenStable()` when testing component initialization. This is likely due to the component's lifecycle behavior or modal initialization timing that fixture.whenStable() cannot properly wait for in zoneless mode.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.spec.tsprojects/dashboards-ng/src/components/grid/si-grid.component.tsprojects/dashboards-ng/README.mdprojects/dashboards-ng/src/model/si-widget-storage.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/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-11T10:09:01.564Z
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:09:01.564Z
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/grid/si-grid.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
📚 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
📚 Learning: 2025-12-08T11:25:51.584Z
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.584Z
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
📚 Learning: 2025-12-15T10:05:59.100Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1193
File: projects/element-ng/search-bar/si-search-bar.component.spec.ts:21-28
Timestamp: 2025-12-15T10:05:59.100Z
Learning: In tests that rely on Jasmine's fake clock to control time-based RxJS operators (e.g., debounceTime), use jasmine.clock().install() together with jasmine.clock().mockDate() to enable precise timing control. Without mockDate, scheduler-based operators may not respond to the fake clock. Apply this guidance to spec files (e.g., any file named *.spec.ts or *.spec.js) that test time-sensitive behavior, ensuring you install and mockDate before advancing time in tests.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts
📚 Learning: 2025-12-22T13:21:01.053Z
Learnt from: mistrykaran91
Repo: siemens/element PR: 1234
File: projects/element-ng/ag-grid/provider.ts:74-81
Timestamp: 2025-12-22T13:21:01.053Z
Learning: In the siemens/element repository, configuration providers use custom string tokens with useValue for consistency (e.g., provideSiDatatableConfig uses { provide: 'configuration', useValue: {...} }). The AG Grid provider follows this same pattern with { provide: 'ag-grid-options', useValue: provideGlobalGridOptions({...}) }.
Applied to files:
projects/dashboards-ng/src/components/grid/si-grid.component.tsprojects/dashboards-ng/src/model/si-widget-id-provider.tsapi-goldens/dashboards-ng/index.api.mdprojects/dashboards-demo/src/app/app.config.ts
📚 Learning: 2025-12-23T09:24:35.163Z
Learnt from: spike-rabbit
Repo: siemens/element PR: 1235
File: api-goldens/dashboards-ng/translate/index.api.md:12-84
Timestamp: 2025-12-23T09:24:35.163Z
Learning: In the siemens/element repository, do not review files under the api-goldens/ directory (e.g., api-goldens/**/index.api.md) since they are auto-generated by API Extractor. Exclude these from review checks and focus on source files that are not auto-generated.
Applied to files:
api-goldens/dashboards-ng/index.api.md
📚 Learning: 2025-12-23T09:24:57.843Z
Learnt from: spike-rabbit
Repo: siemens/element PR: 1235
File: api-goldens/dashboards-ng/index.api.md:324-325
Timestamp: 2025-12-23T09:24:57.843Z
Learning: Do not review files in the api-goldens directory, as they are auto-generated API reports produced by API Extractor.
Applied to files:
api-goldens/dashboards-ng/index.api.md
📚 Learning: 2025-12-23T09:24:40.395Z
Learnt from: spike-rabbit
Repo: siemens/element PR: 1235
File: api-goldens/dashboards-ng/module-federation/index.api.md:7-11
Timestamp: 2025-12-23T09:24:40.395Z
Learning: In the siemens/element repository, do not review any files under the api-goldens directory, as they are auto-generated API reports produced by API Extractor. These MD files should be excluded from review unless explicitly overridden.
Applied to files:
api-goldens/dashboards-ng/index.api.md
🧬 Code graph analysis (7)
projects/dashboards-demo/src/app/app-widget-id-provider.ts (1)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(129-191)
projects/dashboards-demo/src/app/app-widget-storage.ts (1)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(129-191)
projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (4)
projects/dashboards-ng/src/model/si-widget-storage.ts (1)
SiDefaultWidgetStorage(94-153)projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(129-191)projects/dashboards-ng/src/model/configuration.ts (1)
SI_DASHBOARD_CONFIGURATION(21-27)
projects/dashboards-ng/src/components/grid/si-grid.component.ts (1)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(129-191)
projects/dashboards-demo/src/app/app.config.ts (2)
projects/dashboards-ng/src/model/si-widget-id-provider.ts (1)
SI_WIDGET_ID_PROVIDER(21-24)projects/dashboards-demo/src/app/app-widget-id-provider.ts (1)
AppWidgetIdProvider(7-11)
projects/dashboards-ng/src/model/si-widget-storage.ts (1)
projects/dashboards-ng/src/model/widgets.model.ts (1)
WidgetConfig(129-191)
⏰ 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 (15)
projects/dashboards-ng/src/model/si-widget-storage.ts (1)
35-54: LGTM! Clean separation of modified and added widgets.The updated
save()signature clearly separatesmodifiedWidgets(existing widgets) fromaddedWidgets(new widgets with provider-generated IDs), improving clarity and making the persistence contract explicit. The documentation accurately describes the responsibility for backend ID reconciliation when applicable.projects/dashboards-ng/src/public-api.ts (1)
15-15: LGTM! Public API surface correctly expanded.The export of
si-widget-id-providermakes the new provider infrastructure available to library consumers, enabling custom ID generation implementations as documented in the README.projects/dashboards-ng/README.md (1)
187-232: LGTM! Comprehensive and accurate documentation.The new "Widget instance ID generation" section effectively documents:
- The default UUID-based ID generation using
crypto.randomUUID()- How to implement custom ID providers by extending
SiWidgetIdProvider- Provider registration for both standalone and module-based Angular applications
- Correct method signature:
generateWidgetId(widget: WidgetConfig, dashboardId?: string): stringThe examples are clear and actionable for library consumers.
projects/dashboards-demo/src/app/app.config.ts (1)
39-39: LGTM! Provider registration correctly demonstrates custom ID provider usage.The registration of
AppWidgetIdProviderviaSI_WIDGET_ID_PROVIDERfollows Angular DI best practices and effectively demonstrates how library consumers can override the default ID generation behavior.projects/dashboards-ng/src/model/si-widget-id-provider.ts (3)
21-24: LGTM! Injection token with sensible default factory.The
SI_WIDGET_ID_PROVIDERtoken correctly provides a default factory that injectsSiWidgetDefaultIdProvider, ensuring the library works out-of-the-box while remaining extensible for custom implementations.
29-38: LGTM! Clean abstract contract for ID generation.The abstract
SiWidgetIdProviderclass defines a clear, type-safe contract for widget ID generation. The signaturegenerateWidgetId(widget: Omit<WidgetConfig, 'id'>, dashboardId?: string): stringappropriately accepts widgets without IDs and optional dashboard context.
44-59: LGTM! Secure default implementation with comprehensive documentation.The
SiWidgetDefaultIdProviderusescrypto.randomUUID()to generate RFC4122 version 4 UUIDs with 122 bits of entropy, providing cryptographically secure uniqueness. The JSDoc accurately documents the algorithm and format. The implementation intentionally ignores the widget and dashboardId parameters, which is appropriate for a generic default that prioritizes uniqueness over context.projects/dashboards-ng/src/components/grid/si-grid.component.spec.ts (3)
21-29: LGTM! Test imports updated for provider-based ID generation.The imports correctly include
SI_WIDGET_ID_PROVIDERto support testing the new provider-based ID generation infrastructure.
226-226: LGTM! Storage API call correctly uses new signature.The
save()call properly separates parameters:modifiedWidgets(containing the persisted widgetConfig),addedWidgets(empty array),removedWidgets(empty array), anddashboardId. This aligns with the new storage API contract.
236-280: LGTM! Comprehensive test coverage for custom ID provider integration.The new test suite effectively validates:
- Custom provider registration via
SI_WIDGET_ID_PROVIDER- ID generation using the custom
generateWidgetIdimplementation- Integration with the grid component's
addWidgetInstanceflow- Correct ID format:
"custom-id-default"(matching the mock implementation)The mock provider correctly implements the
generateWidgetId(widget: WidgetConfig, dashboardId?: string)signature, addressing previous review feedback.projects/dashboards-ng/src/components/grid/si-grid.component.ts (4)
26-26: LGTM! Provider injection correctly implemented.The component properly imports and injects
SI_WIDGET_ID_PROVIDER, enabling pluggable ID generation while respecting the DI token's default factory fallback.Also applies to: 52-52
251-258: LGTM! Clean separation of transient and persisted widgets for save.The implementation correctly:
- Updates widget positions before persistence
- Creates a Set of transient widget IDs for efficient lookup
- Filters
modifiedWidgets(existing, non-transient)- Filters
addedWidgets(transient, newly added)- Calls
save()with the new signature:save(modifiedWidgets, addedWidgets, markedForRemoval, dashboardId())This separation aligns perfectly with the updated storage API contract and enables backend systems to handle new vs. existing widgets differently (e.g., INSERT vs. UPDATE).
302-311: LGTM! ID generation correctly delegated to provider.Line 303 properly delegates ID generation to
widgetIdProvider.generateWidgetId(widgetInstanceConfig, this.dashboardId()), passing the widget config without an ID and the optional dashboard context. The generated ID is assigned to the new widget, which is then tracked intransientWidgetInstancesfor later separation during save.
319-333: LGTM! Transient widget removal correctly handled.Lines 324-326 correctly check whether the widget being removed is transient (newly added, not persisted). Only non-transient widgets are added to
markedForRemovalfor backend deletion, preventing unnecessary removal operations for widgets that were never persisted.projects/dashboards-demo/src/app/app-widget-storage.ts (1)
119-129: LGTM! Signature correctly updated to separate modified and added widgets.The updated
savemethod signature correctly splits widgets intomodifiedWidgetsandaddedWidgetsarrays, aligning with the new storage API. The call tosuper.save()properly passes all parameters through, and the delay behavior is preserved.
|
|
3 participants
SiWidgetIdProviderabstract class and injection token for custom widget ID generationSiWidgetDefaultIdProviderwith random ID generation as defaultSiGridComponentto use widget ID providerSiDefaultWidgetStorageto support custom ID provider for new widgetsAppWidgetIdProviderimplementation in dashboards-demoNEW_WIDGET_PREFIXand counter-based ID generationBREAKING CHANGE: New widgets are now assigned transient IDs upon creation using the
SiWidgetIdProvider.transientWidgetIdResolver, rather than using temporaryNEW_WIDGET_PREFIXIDs that are replaced on save.The
save()method signature in the abstract classSiWidgetStoragehas been modified to separate existing widgets from new widgets:Before:
After:
addedWidgetsarray will contain new widgets with ids generated bySiWidgetIdProvider.transientWidgetIdResolver.Diagram
Summary by CodeRabbit
New Features
Breaking Changes / Persistence
Code
Docs & Tests