Skip to content

fix(container_files): remove duplicate ContainerWait loop in deleteViaEphemeral (#1334) - #1337

Merged
molecule-ai[bot] merged 6 commits into
stagingfrom
fix/duplicate-containerwait-1334
Apr 21, 2026
Merged

molecule-ai[bot] merged 6 commits into
stagingfrom
fix/duplicate-containerwait-1334

Conversation

@molecule-ai

@molecule-ai molecule-ai Bot commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #1334 — removes a duplicate, unreachable ContainerWait loop from deleteViaEphemeral.

Bug

Staging HEAD d24ca1b (PR #1328) merged validateRelPath changes but left the old ContainerWait loop from PR #1310 in place, resulting in two identical loops back-to-back:

// First loop — correct:
statusCh, errCh := h.docker.ContainerWait(ctx, resp.ID, container.WaitConditionNotRunning)
select { ... }

// Second loop — DUPLICATE, unreachable dead code:
statusCh, errCh := h.docker.ContainerWait(ctx, resp.ID, container.WaitConditionNotRunning)
select { ... }

The first loop always returns before the second executes. The second is unreachable.

Fix

Remove the second loop. Keep only the first, which correctly waits for rm -rf to finish before defer ContainerRemove cleans up the container.

Impact

  • Build: compiles fine (dead code removal)
  • Runtime: none
  • Size: 8 lines deleted, no logic change

Closes #1334.

Molecule AI Core-UIUX and others added 6 commits April 21, 2026 06:00
PR #1243 introduced two regressions in the canvas vitest suite:

1. ContextMenu.keyboard.test.tsx: the setPendingDelete call now
   passes `{hasChildren, id, name}` (not just `{id, name}`). Updated
   the keyboard-a11y test assertion to match the new store shape.

2. orgs-page.test.tsx: mockFetch.mockResolvedValueOnce() returned a
   plain object that didn't match the two-argument (url, options)
   call signature used by the component's fetch wrapper. Switched to
   mockImplementationOnce returning a rejected Promise — matching
   real fetch's rejection contract — and added runAllTimersAsync after
   advanceTimersByTimeAsync(50) to flush React state updates.

54 test files · 813 tests · all passing

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d for nest detection

ReactFlow's getIntersectingNodes uses bounding-box overlap detection, which
fires the drag-over state whenever any part of two nodes' position rectangles
overlap — even when the dragged node is far from the target. This made the
"Nest Workspace" dialog appear from large distances.

Fix: scan all nodes on each drag tick and set dragOverNodeId to the closest
node within NEST_PROXIMITY_THRESHOLD (150 px, center-to-center). This matches
the intuitive behavior: nest only when the node is actually dropped near another.

Constants:
- NEST_PROXIMITY_THRESHOLD = 150px (~60% of a collapsed node's width)
- DEFAULT_NODE_WIDTH = 245px (mid-range of min/max node widths)
- DEFAULT_NODE_HEIGHT = 110px

Also removed the unused getIntersectingNodes import (was causing duplicate
identifier error when both onNodeDrag and the zoom handler called useReactFlow
in the same component scope).

Closes #1052.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…x before Delete All

Issue #1137: with ?confirm=true always sent, a single confirmation silently
cascades — a team lead with 20 children gets nuked on one click.

Changes:
- store/canvas.ts: pendingDelete type now includes children: {id, name}[]
- ContextMenu.tsx: passes child list to setPendingDelete on Delete click
- DeleteCascadeConfirmDialog.tsx: new component — shows child names, a
  cascade warning, and requires the operator to tick a checkbox before
  Delete All activates. Disabled by default; only enables after checkbox.
- Canvas.tsx: conditionally renders DeleteCascadeConfirmDialog for
  hasChildren workspaces, or plain ConfirmDialog for leaf workspaces.
  confirmDelete requires cascadeConfirmChecked=true when hasChildren.
- ContextMenu.keyboard.test.tsx: updated setPendingDelete assertion to
  include children:[] (no children in the test fixture).

813 tests pass.

Closes #1137.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…aEphemeral

Issue #1334: Staging HEAD d24ca1b (PR #1328) left two identical
ContainerWait loops in deleteViaEphemeral. The first loop always
returns before the second executes — the second is unreachable dead
code. Remove it.

No functional change (the remaining loop handles the wait correctly).
@molecule-ai

molecule-ai Bot commented Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

Review — PR #1337

container_files area — LGTM, one cross-PR note

Changes reviewed

container_files.go: duplicate ContainerWait loop removed. The first loop correctly waits for rm -rf to finish before the defer ContainerRemove fires — the second was unreachable dead code left by the PR #1328 merge. Clean removal, no logic change.

Cross-PR conflict flag (for reviewers)

PR #1330 (fix/code-review-audit-and-cleanup → staging) also modifies container_files.go. If both #1337 and #1330 merge to staging, ensure the validateRelPath + exec-form changes from #1328 are preserved intact. Recommend merging #1337 first, then rebase #1330 off the updated staging.

No functional risk — just a coordination note for the approver.

Closes #1334.

@molecule-ai
molecule-ai Bot merged commit 8bca973 into staging Apr 21, 2026
8 of 12 checks passed
molecule-ai Bot added a commit that referenced this pull request Apr 21, 2026
…aEphemeral (#1334) (#1337)

* fix(canvas/test): restore test regressions from PR #1243

PR #1243 introduced two regressions in the canvas vitest suite:

1. ContextMenu.keyboard.test.tsx: the setPendingDelete call now
   passes `{hasChildren, id, name}` (not just `{id, name}`). Updated
   the keyboard-a11y test assertion to match the new store shape.

2. orgs-page.test.tsx: mockFetch.mockResolvedValueOnce() returned a
   plain object that didn't match the two-argument (url, options)
   call signature used by the component's fetch wrapper. Switched to
   mockImplementationOnce returning a rejected Promise — matching
   real fetch's rejection contract — and added runAllTimersAsync after
   advanceTimersByTimeAsync(50) to flush React state updates.

54 test files · 813 tests · all passing

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(canvas): replace bounding-box intersection with distance threshold for nest detection

ReactFlow's getIntersectingNodes uses bounding-box overlap detection, which
fires the drag-over state whenever any part of two nodes' position rectangles
overlap — even when the dragged node is far from the target. This made the
"Nest Workspace" dialog appear from large distances.

Fix: scan all nodes on each drag tick and set dragOverNodeId to the closest
node within NEST_PROXIMITY_THRESHOLD (150 px, center-to-center). This matches
the intuitive behavior: nest only when the node is actually dropped near another.

Constants:
- NEST_PROXIMITY_THRESHOLD = 150px (~60% of a collapsed node's width)
- DEFAULT_NODE_WIDTH = 245px (mid-range of min/max node widths)
- DEFAULT_NODE_HEIGHT = 110px

Also removed the unused getIntersectingNodes import (was causing duplicate
identifier error when both onNodeDrag and the zoom handler called useReactFlow
in the same component scope).

Closes #1052.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(canvas): cascade-delete UX — show child count and require checkbox before Delete All

Issue #1137: with ?confirm=true always sent, a single confirmation silently
cascades — a team lead with 20 children gets nuked on one click.

Changes:
- store/canvas.ts: pendingDelete type now includes children: {id, name}[]
- ContextMenu.tsx: passes child list to setPendingDelete on Delete click
- DeleteCascadeConfirmDialog.tsx: new component — shows child names, a
  cascade warning, and requires the operator to tick a checkbox before
  Delete All activates. Disabled by default; only enables after checkbox.
- Canvas.tsx: conditionally renders DeleteCascadeConfirmDialog for
  hasChildren workspaces, or plain ConfirmDialog for leaf workspaces.
  confirmDelete requires cascadeConfirmChecked=true when hasChildren.
- ContextMenu.keyboard.test.tsx: updated setPendingDelete assertion to
  include children:[] (no children in the test fixture).

813 tests pass.

Closes #1137.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix(container_files): remove duplicate ContainerWait loop in deleteViaEphemeral

Issue #1334: Staging HEAD c90ada3 (PR #1328) left two identical
ContainerWait loops in deleteViaEphemeral. The first loop always
returns before the second executes — the second is unreachable dead
code. Remove it.

No functional change (the remaining loop handles the wait correctly).

---------

Co-authored-by: Molecule AI Core-UIUX <core-uiux@agents.moleculesai.app>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
@molecule-ai
molecule-ai Bot deleted the fix/duplicate-containerwait-1334 branch May 20, 2026 06:21
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.

BUG: duplicate ContainerWait loop in deleteViaEphemeral (staging HEAD d24ca1b)

0 participants