Skip to content

improvement(ui): remove anti-patterns, fix follow-up auto-scroll, move CopyCodeButton to emcn - #4148

Merged
waleedlatif1 merged 16 commits into
stagingfrom
fix/resource-url
Apr 14, 2026
Merged

improvement(ui): remove anti-patterns, fix follow-up auto-scroll, move CopyCodeButton to emcn#4148
waleedlatif1 merged 16 commits into
stagingfrom
fix/resource-url

Conversation

@waleedlatif1

@waleedlatif1waleedlatif1 commented Apr 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fixed suggested follow-ups not scrolling into view after stream ends — added a brief post-stream MutationObserver in use-auto-scroll to catch the DOM expansion
  • Moved CopyCodeButton from components/ui into components/emcn/components/code, exported from the emcn barrel, nudged slightly right in code block headers
  • Removed dead code and anti-patterns, ran full cleanup pass

Type of Change

  • Bug fix

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel

vercelBot commented Apr 14, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
docsSkippedSkippedApr 14, 2026 4:57am

Request Review

@cursor

cursorBot commented Apr 14, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Touches execution-cancellation paths and adds a DB transaction that updates paused executions and execution logs, so mistakes could incorrectly mark executions as cancelled or impact observability. UI changes are low risk but rely on the new transient cancelling status and optimistic cache updates.

Overview
Execution cancellation now supports paused HITL runs. The cancel API (POST /api/workflows/[id]/executions/[executionId]/cancel) adds a fallback that cancels paused executions directly in the DB via PauseResumeManager.cancelPausedExecution, and emits execution:cancelled metadata/events when that path succeeds; the response now includes pausedCancelled, and server-side tests were expanded to cover auth/403 and this new cancel mode.

Logs UI gets better cancellation feedback. The logs view introduces a cancelling display status and an optimistic React Query update in useCancelExecution to immediately mark the targeted log as cancelling, and the context menu allows cancelling pending executions (not just running).

UI cleanup / component moves.CopyCodeButton is moved under emcn (exported from the barrel) and standardized to use the Button component; various small cleanups remove dead mappings and render/effect anti-patterns, and tooling bumps turbo to 2.9.6.

Reviewed by Cursor Bugbot for commit abf4b5a. Configure here.

@greptile-apps

greptile-appsBot commented Apr 14, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR removes anti-patterns, fixes follow-up auto-scroll in the Mothership chat by reverting the streaming guard on onOptionSelect, and relocates CopyCodeButton from components/ui into the emcn library. It also lands several HITL cancellation fixes (DB endedAt, fire-and-forget .catch() guards, and canvas event emission for paused executions) along with a matching test suite update.

Confidence Score: 5/5

  • Safe to merge — all remaining findings are P2 style suggestions with no correctness impact.
  • Previous P0/P1 concerns (missing endedAt, uncancellable observer timer, tilde-fence handling) are confirmed resolved. The two remaining findings are a TypeScript any annotation and a missing intermediate query-key factory method — neither affects runtime behaviour.
  • No files require special attention.

Important Files Changed

FilenameOverview
apps/sim/app/api/workflows/[id]/executions/[executionId]/cancel/route.tsHITL cancellation route: always attempts DB cancel alongside Redis/local-abort paths and emits canvas event only for the DB-only path. One error: any catch-block violates TypeScript rules.
apps/sim/app/api/workflows/[id]/executions/[executionId]/cancel/route.test.tsWell-structured tests using vi.hoisted() + static imports; covers all four cancellation outcomes and auth/authz edge cases cleanly.
apps/sim/hooks/queries/logs.tsSolid useCancelExecution mutation with proper optimistic update, onError rollback, and onSettled invalidation; one inline stats key prefix deviates from the factory pattern.
apps/sim/components/emcn/components/code/copy-code-button.tsxClean move of CopyCodeButton into the emcn library; timer cleanup via ref is correct and the component follows the emcn patterns properly.
apps/sim/app/workspace/[workspaceId]/home/components/mothership-chat/mothership-chat.tsxFollow-up scroll fix reverted to always passing onSubmit for the last message — removes the post-stream DOM expansion that was causing scroll issues. Layout constants extraction is clean.
apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/chat-content/chat-content.tsxStreaming reveal integration and special-tag rendering are well-structured; CopyCodeButton now sourced from emcn correctly.
apps/sim/app/workspace/[workspaceId]/logs/utils.tsUtility functions properly extracted and documented with TSDoc; StatusBadge and TriggerBadge use React.memo and displayName correctly.
apps/sim/lib/workflows/executor/human-in-the-loop-manager.tscancelPausedExecution now correctly sets endedAt: now alongside status: 'cancelled', fixing the running-filter bug flagged in a previous review thread.
apps/sim/lib/core/utils/browser-storage.tsWell-typed SSR-safe localStorage utilities with proper error handling and structured storage classes for landing-page data.

Sequence Diagram

sequenceDiagram
participant Client
participant CancelRoute as POST /cancel
participant Redis as markExecutionCancelled
participant LocalAbort as abortManualExecution
participant DB as cancelPausedExecution
participant EventBuffer
Client->>CancelRoute: POST cancel
CancelRoute->>Redis: markExecutionCancelled(executionId)
CancelRoute->>LocalAbort: abortManualExecution(executionId)
CancelRoute->>DB: cancelPausedExecution(executionId)
Note over DB: Sets status=cancelled, endedAt=now
alt durablyRecorded
CancelRoute-->>Client: success (Redis path)
else locallyAborted
CancelRoute-->>Client: success (in-process path)
else pausedCancelled
CancelRoute->>EventBuffer: setExecutionMeta + write execution:cancelled
CancelRoute-->>Client: success (DB direct path)
else none succeeded
CancelRoute-->>Client: "success=false"
end
Loading

Reviews (8): Last reviewed commit: "fix(hitl): add .catch() to fire-and-forg..." | Re-trigger Greptile

Comment threadapps/sim/hooks/use-auto-scroll.ts Outdated
Comment threadapps/sim/hooks/use-streaming-reveal.ts Outdated
Comment threadapps/sim/tailwind.config.ts
@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

@greptile

@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

@cursor review

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 77f2b27. Configure here.

@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

@greptile

@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

@cursor review

@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

@greptile

@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

@greptile

@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

@cursor review

Paused HITL executions are idle in the DB — they don't poll Redis or
run in-process, so the existing cancel signals had no effect. The DB
status stayed 'pending', causing the optimistic 'cancelling' update to
revert on refetch.
- Add PauseResumeManager.cancelPausedExecution: atomically sets
paused_executions.status and workflow_execution_logs.status to
'cancelled' inside a FOR UPDATE transaction
- Guard enqueueOrStartResume against resuming a cancelled execution
- Include pausedCancelled in the cancel route success check
- Mock PauseResumeManager.cancelPausedExecution to prevent DB calls
- Add pausedCancelled to all expected response objects
- Add test for HITL paused execution cancellation path
- Add missing auth/authz tests
- Switch to vi.hoisted pattern for all mocks
@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

@greptile

@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

@cursor review

Comment threadapps/sim/lib/workflows/executor/human-in-the-loop-manager.ts
Comment threadapps/sim/app/workspace/[workspaceId]/home/home.tsx Outdated
Without endedAt, the logs API running filter (isNull(endedAt)) would
keep cancelled paused executions in the running view indefinitely.
@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

@greptile

@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

@cursor review

Comment threadapps/sim/app/api/workflows/[id]/executions/[executionId]/cancel/route.ts Outdated
…aused execution
Paused HITL executions have no active SSE stream, so the canvas never
received the cancellation event. Now writes execution:cancelled to the
event buffer and updates the stream meta so the canvas reconnect path
picks it up and shows 'Execution Cancelled'.
…ellation
Wrap cancelPausedExecution in try/catch so a DB error does not mask
a prior successful Redis or in-process cancellation. Also move the
resource-collapse side effect in home.tsx to a useEffect to avoid the
stale closure on the resources array.
@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

@greptile

@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

@cursor review

Comment threadapps/sim/app/workspace/[workspaceId]/logs/utils.ts
@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

@greptile

@waleedlatif1

Copy link
Copy Markdown
CollaboratorAuthor

@cursor review

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit abf4b5a. Configure here.

@waleedlatif1
waleedlatif1 merged commit 9c1b0bc into stagingApr 14, 2026
14 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/resource-url branch April 14, 2026 05:04
Sign up for freeto 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.

1 participant

@waleedlatif1