Skip to content

Conversation

ghsolomon
Copy link
Contributor

@ghsolomon ghsolomon commented May 19, 2025

Hoist P/R Checklist

Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.

  • Caught up with develop branch as of last change.
  • Added CHANGELOG entry, or determined not required.
  • Reviewed for breaking changes, added breaking-change label + CHANGELOG if so.
  • Updated doc comments / prop-types, or determined not required.
  • Reviewed and tested on Mobile, or determined not required.
  • Created Toolbox branch / PR, or determined not required.

If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.

Pull request reviewers: when merging this P/R, please consider using a squash commit to
collapse multiple intermediate commits into a single commit representing the overall feature
change. This helps keep the commit log clean and easy to scan across releases. PRs containing a
single commit should be rebased when possible.

Copy link

@Copilot 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 improves persistence for both the DashCanvas and Grid components by refining state loading, provider registration, and state comparison functionality. Key changes include updates to state loading in DashCanvasModel, enhanced provider registration in persistence providers, and a custom equality check in grid column state persistence.

Reviewed Changes

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

Show a summary per file
File Description
desktop/cmp/dash/canvas/DashCanvasModel.ts Refactored state loading and view model updates with new ID generation and improved matching logic.
desktop/cmp/dash/DashViewModel.ts Enhanced view state update methods to propagate state changes to registered providers.
core/persist/provider/ViewManagerProvider.ts Replaced direct array manipulation with register/unregister methods for provider management.
core/persist/provider/DashViewProvider.ts Updated provider destruction to use the new unregisterProvider method.
core/persist/PersistenceProvider.ts Added lastRead caching and a pushStateToTarget method to prevent unnecessary state writes.
cmp/viewmanager/ViewManagerModel.ts Changed provider visibility and updated state push logic to ensure a clean provider reference.
cmp/grid/impl/InitPersist.ts Introduced a PersistableColumnState class with a custom equality check that filters out hidden cols.
CHANGELOG.md Updated changelog to document the persistence improvements and bug fixes.
Comments suppressed due to low confidence (2)

desktop/cmp/dash/canvas/DashCanvasModel.ts:409

  • Consider preserving existing state IDs (if available) instead of generating new ones on every load. This would help in matching persisted state with existing view models for incremental updates.
state = state.map(it => ({id: this.genViewId(), ...it}));

cmp/grid/impl/InitPersist.ts:80

  • [nitpick] Add documentation to clarify why hidden columns are filtered out in the equality check. This will help future maintainers understand the rationale behind this behavior.
class PersistableColumnState extends PersistableState<ColumnState[]> {

@ghsolomon
Copy link
Contributor Author

Still need to work on giving DashContainerModel the same ability to set individual view states as DashCanvasModel

@ghsolomon ghsolomon requested a review from Copilot May 30, 2025 13:49
Copy link

@Copilot 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 improves persistence handling for DashCanvas and grid column states while streamlining provider registration and state push mechanisms.

  • Introduces new state mapping with generated IDs and layout assignment in DashCanvasModel.
  • Integrates provider registration/unregistration to ensure consistent state updates across DashViewModel and persistence providers.
  • Implements a custom PersistableColumnState for grid models to filter out hidden columns when comparing state.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
desktop/cmp/dash/canvas/DashCanvasModel.ts Updates state mapping for items and layout assignment using new lodash methods.
desktop/cmp/dash/DashViewModel.ts Consolidates view state updates and registers state providers.
core/persist/provider/ViewManagerProvider.ts Refactors provider registration and adds lastReadTime for state reads.
core/persist/provider/DashViewProvider.ts Adjusts provider registration and unregistering during destruction.
core/persist/PersistenceProvider.ts Introduces lastRead caching and a pushStateToTarget method refactor.
cmp/viewmanager/ViewManagerModel.ts Ensures provider registration and state push logic is streamlined.
cmp/grid/impl/InitPersist.ts Adds a custom PersistableColumnState class that filters hidden columns.
CHANGELOG.md Documents the bug fix and new persistence improvements.
Comments suppressed due to low confidence (2)

desktop/cmp/dash/canvas/DashCanvasModel.ts:443

  • [nitpick] The property 'i' used in the layout object is ambiguous. Consider renaming it to 'id' for clarity and consistency with the rest of the code.
this.setLayout(state.map(it => ({i: it.id, ...it.layout})));

cmp/viewmanager/ViewManagerModel.ts:602

  • [nitpick] The removal of updating 'lastPushed' may affect state synchronization logic. Please verify that this change is intentional and that alternative handling for stale state is in place.
this.lastPushed = Date.now();

this.logWarn(`Unknown viewSpecId [${viewSpecId}] found in state - skipping.`);
}
});
state = state.map(it => ({id: this.genViewId(), ...it}));
Copy link
Preview

Copilot AI May 30, 2025

Choose a reason for hiding this comment

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

Generating a new id for each state item overwrites any existing id. Please confirm that this behavior is intentional to avoid breaking any persistence matching logic.

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.

ColumnState becomes unintentionally dirty when there are hidden columns
1 participant