Skip to content

fix(template-builder): fix drag regression in template builder#35277

Merged
zJaaal merged 3 commits intomainfrom
fix/template-builder-drag-regression
Apr 9, 2026
Merged

fix(template-builder): fix drag regression in template builder#35277
zJaaal merged 3 commits intomainfrom
fix/template-builder-drag-regression

Conversation

@zJaaal
Copy link
Copy Markdown
Member

@zJaaal zJaaal commented Apr 9, 2026

Summary

  • Reverts the identify track function — removes the method entirely and uses row.id / box.id directly in @for track expressions. The previous ${id}-${index} approach caused Angular to treat widgets by position rather than identity, desyncing GridStack's DOM during column-between-row drags and leaving the source row empty.
  • Bumps gridstack 8.1.18.4.0
  • Guards against undefined oldChild in updateOldRows when a column has moved to a new X position after a drag

Related

Closes #33073

🤖 Generated with Claude Code


Note

Medium Risk
Changes how Angular tracks grid items and updates GridStack dependency, which can affect drag/drop rendering and layout state synchronization.

Overview
Fixes a template-builder drag regression by switching @for tracking to stable row.id/box.id (and removing the custom identify() key generator) so GridStack DOM/widget identity stays consistent during cross-row column drags.

Hardens updateOldRows merging to avoid crashing/losing state when a column’s previous X position can’t be found (oldChild?.id ?? newChild.id), and bumps gridstack from 8.1.1 to 8.4.0.

Reviewed by Cursor Bugbot for commit ab89c85. Bugbot is set up for automated code reviews on this repo. Configure here.

…ack to 8.4.0

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

❌ Issue Linking Required

This PR could not be linked to an issue. All PRs must be linked to an issue for tracking purposes.

How to fix this:

Option 1: Add keyword to PR body (Recommended - auto-removes this comment)
Edit this PR description and add one of these lines:

  • This PR fixes #123 or Fixes: #123

  • This PR closes #123 or Closes: #123

  • This PR resolves #123 or Resolves: #123

  • Other supported keywords: fix, fixed, close, closed, resolve, resolved
    Option 2: Link via GitHub UI (Note: won't clear the failed check)

  1. Go to the PR → Development section (right sidebar)

  2. Click "Link issue" and select an existing issue

  3. Push a new commit or re-run the workflow to clear the failed check
    Option 3: Use branch naming
    Create a new branch with one of these patterns:

  • 123-feature-description (number at start)

  • issue-123-feature-description (issue-number at start)

  • feature-issue-123 (issue-number anywhere)

Why is this required?

Issue linking ensures proper tracking, documentation, and helps maintain project history. It connects your code changes to the problem they solve.---

This comment was automatically generated by the issue linking workflow

@github-actions github-actions bot added the Area : Frontend PR changes Angular/TypeScript frontend code label Apr 9, 2026
@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 9, 2026

Claude finished @zJaaal's task in 3m 10s —— View job


PR Review

Tasks

  • Read changed files and analyze the PR
  • Post review feedback

Summary

The core fix (reverting identify in favor of row.id/box.id directly) is the right call — the index-based key in identify was the root cause of the drag regression. Three issues worth flagging:


Issues

1. track box.id — no guard for missing IDs

GridStackWidget.id is typed as id?: string (optional). The removed identify method was added to prevent Angular NG0955 duplicate-key errors when an item lacks an id. With:

@for (box of row.subGridOpts?.children; track box.id)

If any box has id === undefined, Angular uses undefined as the track key — two such boxes in the same row triggers NG0955. All code paths that create boxes appear to assign a uuid(), but the type does not enforce it. Consider track box.id ?? $index as a safe fallback.


2. updateOldRowsnewRow?.subGridOpts.children can still throw

template-builder.store.ts line ~419:

children: newRow?.subGridOpts.children.map((newChild) => {

If newRow is defined but newRow.subGridOpts is undefined, the optional chain on newRow does not protect the .children access — this throws a TypeError. The adjacent spread ...(newRow?.subGridOpts || {}) correctly guards the spread, but the .children.map() call does not. Should be newRow?.subGridOpts?.children?.map(...) ?? [].

Pre-existing issue, but the PR adds oldChild?.id ?? newChild.id in the same block so it is in scope.


3. oldRow.subGridOpts.children.find(...) — unguarded in same function

const oldChild = oldRow.subGridOpts.children.find(
    (oldChild) => oldChild.x === newChild.x
);

oldRow.subGridOpts is not null-checked. Pre-existing — worth fixing while touching this code.


4. gridstack 8.4.0 bump — changelog not referenced

Three minor versions jumped (8.1.1 to 8.4.0). The PR body does not mention what changed or which specific fix in 8.4.0 was needed.


Overall: The drag regression fix is correct and oldChild?.id is the right guard for the updateOldRows crash. The main remaining risk is the lack of type enforcement on box.id — low probability in practice since all creation paths use uuid(), but worth a defensive fallback.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@zJaaal
Copy link
Copy Markdown
Member Author

zJaaal commented Apr 9, 2026

Screen.Recording.2026-04-09.at.6.19.21.PM.mov

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 9, 2026

test body

Copy link
Copy Markdown

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is kicking off a free cloud agent to fix this issue. This run is complimentary, but you can enable autofix for all future PRs in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 473cf96. Configure here.

…directly in track

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@zJaaal zJaaal requested a review from KevinDavilaDotCMS April 9, 2026 21:32
@zJaaal zJaaal enabled auto-merge April 9, 2026 21:34
@zJaaal zJaaal added this pull request to the merge queue Apr 9, 2026
Merged via the queue into main with commit 0166416 Apr 9, 2026
42 checks passed
@zJaaal zJaaal deleted the fix/template-builder-drag-regression branch April 9, 2026 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Frontend PR changes Angular/TypeScript frontend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Upgrade RxJS from v6 to v7

4 participants