Skip to content

Conversation

@dominiquekleeven
Copy link
Collaborator

@dominiquekleeven dominiquekleeven commented Oct 21, 2025

Closes #53 and #52

Changes

This PR replaces the current method of selecting assets and their attributes (dropdowns) with the or-attribute-picker component.

  • Added or-attribute-picker dependency.
  • Added custom-asset-attribute-picker.ts, which extends/overrides OrAssetAttributePicker.
  • Updated the manager config eventProviderType to WEBSOCKET so that the or-asset-tree component inside the attribute picker functions correctly, as it relies on WebSockets.
  • The service now locally updates the manager.displayRealm so that or-asset-tree knows which realm to query for assets.

Screenshots

Select target button

image

Select attributes dialog (attribute picker)

image

When an attribute has been selected

image Clicking on the Asset/Attribute will show the dialog again to modify it

Regressors

image Functions the same as the target, but has an extra meta item requirement for the attribute image

@dominiquekleeven dominiquekleeven marked this pull request as ready for review December 1, 2025 12:54
@dominiquekleeven dominiquekleeven requested a review from a team December 1, 2025 12:54
@wborn wborn requested a review from Copilot December 1, 2025 16:41
Copilot finished reviewing on behalf of wborn December 1, 2025 16:42
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR modernizes the asset/attribute selection UI by replacing dropdown components with the or-attribute-picker component. Key changes include adding the picker dependency, creating a custom extension for regressor-specific filtering, switching to WebSocket event provider, and updating the realm display context.

  • Replaces dropdown-based asset/attribute selection with or-attribute-picker dialog component
  • Adds custom filtering logic to show only attributes with required meta items for regressors
  • Updates event provider configuration from POLLING to WEBSOCKET for or-asset-tree compatibility

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
frontend/src/pages/pages-config-editor.ts Replaced dropdown inputs with dialog-based attribute picker, added asset loading methods, removed dropdown list state management
frontend/src/pages/app-layout.ts Sets manager.displayRealm to enable or-asset-tree realm queries
frontend/src/index.ts Changed eventProviderType from POLLING to WEBSOCKET
frontend/src/components/custom-asset-attribute-picker.ts New custom component extending OrAssetAttributePicker with predicted datapoints filtering
frontend/package.json Added @openremote/or-attribute-picker dependency
frontend/package-lock.json Updated dependency tree with new packages and version updates

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +260 to 273
// Load asset data for target
protected async loadTargetAsset() {
if (this.formData.target.asset_id && this.formData.target.attribute_name) {
try {
const response = await manager.rest.api.AssetResource.get(this.formData.target.asset_id);
this.targetAsset = response.data;
} catch (err) {
console.error(err);
this.targetAsset = null;
}
} else {
this.targetAsset = null;
}
}
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The console.error on line 267 logs the error without context. Consider logging a descriptive message like 'Failed to load target asset:' before the error to help with debugging.

Copilot uses AI. Check for mistakes.
Comment on lines +314 to 330
// Load asset data for regressor
protected async loadRegressorAsset(index: number) {
if (!this.formData.regressors) return;

const regressor = this.formData.regressors[index];
if (regressor.asset_id && regressor.attribute_name) {
try {
const response = await manager.rest.api.AssetResource.get(regressor.asset_id);
this.regressorAssets.set(index, response.data);
this.regressorAssets = new Map(this.regressorAssets);
} catch (err) {
console.error(err);
this.regressorAssets.delete(index);
this.regressorAssets = new Map(this.regressorAssets);
}
}
}
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The console.error on line 325 logs the error without context. Consider logging a descriptive message like 'Failed to load regressor asset at index ${index}:' before the error to improve debugging.

Copilot uses AI. Check for mistakes.
protected error: string | null = null;

@state()
protected targetAsset: any = null;
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The targetAsset property uses 'any' type. Consider defining a proper type interface for the asset object to improve type safety and code maintainability.

Copilot uses AI. Check for mistakes.
protected targetAsset: any = null;

@state()
protected regressorAssets: Map<number, any> = new Map();
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The Map value uses 'any' type. Consider defining a proper type interface for asset objects to improve type safety throughout the codebase.

Copilot uses AI. Check for mistakes.
Comment on lines +56 to +60
return "No attributes with 'has predicted data points' and either 'store data points' or 'agent link' configuration item found";
}
if (this.showOnlyHasPredictedDatapointsAttrs) {
// TODO: local translation
return "No attributes with 'has predicted data points' configuration item found";
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The TODO comment indicates this error message should use local translation keys instead of hard-coded strings. Consider adding translation keys to maintain consistency with the rest of the codebase.

Suggested change
return "No attributes with 'has predicted data points' and either 'store data points' or 'agent link' configuration item found";
}
if (this.showOnlyHasPredictedDatapointsAttrs) {
// TODO: local translation
return "No attributes with 'has predicted data points' configuration item found";
return 'noPredictedAndDatapointAttributes';
}
if (this.showOnlyHasPredictedDatapointsAttrs) {
// TODO: local translation
return 'noPredictedAttributes';

Copilot uses AI. Check for mistakes.
Comment on lines +56 to +60
return "No attributes with 'has predicted data points' and either 'store data points' or 'agent link' configuration item found";
}
if (this.showOnlyHasPredictedDatapointsAttrs) {
// TODO: local translation
return "No attributes with 'has predicted data points' configuration item found";

This comment was marked as resolved.

Object.values(asset.attributes ?? {}).forEach((attribute) => {
if (attribute.meta?.storeDataPoints !== true) {
delete asset.attributes?.[attribute.name as keyof typeof asset.attributes];
dialog.addEventListener(OrAssetAttributePickerPickedEvent.NAME, async (ev: any) => {
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The event parameter 'ev' uses 'any' type. Consider using the proper event type 'OrAssetAttributePickerPickedEvent' for better type safety.

Suggested change
dialog.addEventListener(OrAssetAttributePickerPickedEvent.NAME, async (ev: any) => {
dialog.addEventListener(OrAssetAttributePickerPickedEvent.NAME, async (ev: OrAssetAttributePickerPickedEvent) => {

Copilot uses AI. Check for mistakes.
document.body.style.overflow = '';
};

dialog.addEventListener(OrAssetAttributePickerPickedEvent.NAME, async (ev: any) => {
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

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

The event parameter 'ev' uses 'any' type. Consider using the proper event type 'OrAssetAttributePickerPickedEvent' for better type safety.

Suggested change
dialog.addEventListener(OrAssetAttributePickerPickedEvent.NAME, async (ev: any) => {
dialog.addEventListener(OrAssetAttributePickerPickedEvent.NAME, async (ev: OrAssetAttributePickerPickedEvent) => {

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use or-attribute-picker instead of dropdowns for selecting attributes

2 participants