Skip to content

fix(DesignerV2): Fixed issue with dd fallback on initial load#8943

Merged
rllyy97 merged 4 commits intomainfrom
riley/dd-loading-fallback-fix
Mar 19, 2026
Merged

fix(DesignerV2): Fixed issue with dd fallback on initial load#8943
rllyy97 merged 4 commits intomainfrom
riley/dd-loading-fallback-fix

Conversation

@rllyy97
Copy link
Contributor

@rllyy97 rllyy97 commented Mar 19, 2026

Commit Type

  • feature - New functionality
  • fix - Bug fix
  • refactor - Code restructuring without behavior change
  • perf - Performance improvement
  • docs - Documentation update
  • test - Test-related changes
  • chore - Maintenance/tooling

Risk Level

  • Low - Minor changes, limited scope
  • Medium - Moderate changes, some user impact
  • High - Major changes, significant user/system impact

What & Why

Iteration on this feature: #8918
Our fallback logic was not getting triggered on initial load of dynamic data.
This fixes that issue.

Impact of Change

  • Users: Existing dynamic data values are now properly handled during loading states
  • Developers: N/A
  • System: N/A

Test Plan

  • Unit tests added/updated
  • E2E tests added/updated
  • Manual testing completed
  • Tested in:

Contributors

@rllyy97

Screenshots/Videos

N/A

@rllyy97 rllyy97 marked this pull request as ready for review March 19, 2026 16:51
Copilot AI review requested due to automatic review settings March 19, 2026 16:51
@rllyy97 rllyy97 added the risk:low Low risk change with minimal impact label Mar 19, 2026
@rllyy97 rllyy97 enabled auto-merge (squash) March 19, 2026 16:55
@rllyy97 rllyy97 disabled auto-merge March 19, 2026 16:57
@rllyy97 rllyy97 enabled auto-merge (squash) March 19, 2026 16:57
Copy link
Contributor

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

Fixes a regression where dynamic data fallback logic wasn’t triggered on initial load, causing original dynamic values to be lost on save while dynamic inputs were still unavailable.

Changes:

  • Expand fallback condition to also trigger when a node indicates dynamic inputs are expected (not just when a DynamicInputs error exists).
  • Add a unit test covering the “dynamic inputs not yet available” scenario to ensure original inputs are preserved.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File Description
libs/designer-v2/src/lib/core/actions/bjsworkflow/serializer.ts Updates fallback merge condition to preserve original inputs when dynamic inputs aren’t available yet.
libs/designer-v2/src/lib/core/actions/bjsworkflow/test/serializeOperationDynamicMerge.spec.ts Adds a test asserting original dynamic values are preserved during initial dynamic-load states.

@github-actions
Copy link

github-actions bot commented Mar 19, 2026

🤖 AI PR Validation Report

PR Review Results

Thank you for your submission! Here's detailed feedback on your PR title and body compliance:

⚠️ PR Title

  • Current: fix(DesignerV2): Fixed issue with dd fallback on initial load
  • Issue: "dd" is ambiguous (likely "dynamic data"). Title could be more specific and follow conventional present-tense style after the scope prefix (e.g., use preserve/preserves/preserve rather than Fixed). Consider also normalizing the scope casing to lowercase (designer-v2).
  • Recommendation: Use a clearer, present-tense title that explicitly mentions dynamic data and the behavior changed. Examples:
    • fix(designer-v2): preserve dynamic data fallback on initial load
    • fix(designer-v2): trigger dynamic data fallback on initial load when schema isn't available

Commit Type

  • Properly selected (fix).
  • Only one commit type is selected which is correct.

⚠️ Risk Level

  • The PR body and labels indicate Low risk and a risk:low label is present.
  • Assessment: I recommend risk:medium instead. Reason: the changes touch serialization logic, operation metadata state transitions (dynamicLoadStatus), reducers, and the helper that dispatches load status actions. These are cross-cutting state/behavior changes that affect serialization and save behavior across dynamic inputs; although unit tests were added, the behavioral surface area warrants a slightly higher risk classification.

What & Why

  • Current: "Iteration on this feature: https://github.com/Azure/LogicAppsUX/pull/8918\nOur fallback logic was not getting triggered on initial load of dynamic data.\nThis fixes that issue."
  • Issue: The section is brief and does not describe the concrete approach taken (e.g., added dynamicLoadStatus handling, updated serializer to preserve inputs, added reducer action updateNodeDynamicInputLoadStatus).
  • Recommendation: Expand to include a short list of the key changes, for example:
    • Adds/uses DynamicLoadStatus to track dynamic input lifecycle (loading/succeeded/failed).
    • Serializer now preserves original definition inputs when dynamic inputs are not yet available or in-progress and no stash exists.
    • Adds updateNodeDynamicInputLoadStatus action and related reducer transitions; helper dispatches LOADING/FAILED statuses.
    • Added unit tests covering these flows.

⚠️ Impact of Change

  • The impact section is present but lists "N/A" for Developers/System which is not accurate.
  • Recommendation: Clarify who and what is affected:
    • Users: Dynamic data values will be preserved on initial load/saves where dynamic schema hasn't loaded yet — user-facing bugfix.
    • Developers: API/behavior change in operation serialization and new state transitions (dynamicLoadStatus), plus a new action updateNodeDynamicInputLoadStatus that may be used by other code. Callers that previously relied on different assumptions may need to be aware.
    • System: Serialization/save flow may behave differently for nodes with dynamic parameters; ensure integration points that rely on serialization are validated.

Test Plan

  • Assessment: The PR claims unit tests added/updated and manual testing completed. The diff includes multiple unit test updates/additions validating the new behaviors (serializeOperation tests, operationMetadataSlice tests, helper tests), so this matches the claim.
  • Recommendation: If there are any important integration/e2e scenarios that exercise saving workflows with dynamic inputs across services, consider adding E2E coverage or mention why manual testing is sufficient.

⚠️ Contributors

  • Current: @rllyy97 listed.
  • Assessment: Contributors section is present. If others reviewed or helped, consider adding them for credit; if none, this is fine.

Screenshots/Videos

  • Current: N/A — appropriate for a non-visual change.

Summary Table

Section Status Recommendation
Title ⚠️ Make title explicit (avoid "dd") and use present-tense and consistent scope casing
Commit Type None
Risk Level ⚠️ Change to risk:medium in body and label (see rationale)
What & Why Expand to list key implementation changes briefly
Impact of Change ⚠️ Update Developers/System fields to reflect state/action changes
Test Plan Consider E2E or explain why not required
Contributors ⚠️ Add additional contributors if applicable
Screenshots/Videos None needed

Final Message
Please update the PR as recommended above. Key actionable items:

  • Update the PR title to be explicit (e.g., fix(designer-v2): preserve dynamic data fallback on initial load).
  • Consider updating the Risk Level in the PR body and label to Medium to reflect cross-cutting behavioral changes (state & serialization). If you disagree with Medium, add notes in the PR explaining why Low is appropriate (e.g., extremely isolated change with low surface area and full test coverage).
  • Expand the "What & Why" to briefly list the files/behaviors changed so reviewers understand the approach without scanning the diff.
  • Update the Impact section to list Developer/System impacts (new action, state transitions, serialization behavior).
  • If there are integration or E2E scenarios that should be validated (save/serialize flows across dynamic inputs), either add tests or describe manual test scope clearly.

Thanks — overall the PR body is largely complete and unit tests were added. I recommend the label be bumped to risk:medium and the title clarified before merging.


Last updated: Thu, 19 Mar 2026 20:34:23 GMT

@github-actions
Copy link

github-actions bot commented Mar 19, 2026

📊 Coverage Check

The following changed files need attention:

⚠️ libs/designer-v2/src/lib/core/actions/bjsworkflow/serializer.ts - 39% covered (needs improvement)
⚠️ libs/designer-v2/src/lib/core/state/operation/operationMetadataSlice.ts - 38% covered (needs improvement)
⚠️ libs/designer-v2/src/lib/core/utils/parameters/helper.ts - 42% covered (needs improvement)

Please add tests for the uncovered files before merging.

@rllyy97 rllyy97 merged commit 66b74b3 into main Mar 19, 2026
14 of 15 checks passed
@rllyy97 rllyy97 deleted the riley/dd-loading-fallback-fix branch March 19, 2026 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-validated risk:low Low risk change with minimal impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants