Skip to content

Conversation

@waleedlatif1
Copy link
Collaborator

Summary

  • removed custom styling on tag dropdown popover
  • fixed execution ordering in terminal and loops entries

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)

…pover, fixed execution ordering in terminal and loops entries
@vercel
Copy link

vercel bot commented Feb 4, 2026

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Feb 4, 2026 2:12am

Request Review

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 4, 2026

Greptile Overview

Greptile Summary

This PR introduces a monotonically increasing executionOrder field to replace timestamp-based sorting for workflow execution logs. The change addresses race conditions where timestamp comparisons could produce incorrect ordering, especially in loops and parallel executions.

Key changes:

  • Added executionOrder field throughout the execution pipeline (executor, stores, terminal)
  • Replaced all timestamp-based sorting with executionOrder comparisons
  • Fixed iteration matching in console store to prevent cross-iteration output updates
  • Removed custom styling from tag dropdown back button, using PopoverItem component instead
  • Added special executionOrder values: 0 for validation errors (appear first), MAX_SAFE_INTEGER for execution errors (appear last)

Technical implementation:

  • getNextExecutionOrder(ctx) generates sequential integers (1, 2, 3...) via executionOrderCounter
  • Synthetic subflow/iteration entries use Math.min() of child executionOrder values for proper positioning
  • Iteration matching now checks both blockId and iterationCurrent to update correct entries

Confidence Score: 5/5

  • This PR is safe to merge with very low risk
  • The changes are well-structured and systematic, replacing timestamp-based sorting with a more reliable monotonically increasing counter. The implementation is consistent across all layers (executor, API, store, terminal). The iteration matching fix prevents a real bug where loop iterations could receive incorrect updates. No logical errors, security issues, or breaking changes detected.
  • No files require special attention

Important Files Changed

Filename Overview
apps/sim/executor/types.ts Added executionOrder field to BlockLog and ExecutionContext with getNextExecutionOrder helper function
apps/sim/executor/execution/block-executor.ts Updated to generate and pass executionOrder through block execution lifecycle
apps/sim/stores/terminal/console/store.ts Added iteration matching logic and startedAt update support
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/terminal/utils.ts Replaced all timestamp-based sorting with executionOrder, added executionOrder to synthetic entries
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-workflow-execution.ts Added executionOrder to console entries with special values for validation (0) and execution errors (MAX_SAFE_INTEGER)

Sequence Diagram

sequenceDiagram
    participant Client as Client/Frontend
    participant API as API Route
    participant Executor as Block Executor
    participant Orchestrator as Loop/Parallel Orchestrator
    participant Store as Console Store
    participant Terminal as Terminal UI

    Note over Executor: Initialize executionOrderCounter = 0
    
    Executor->>Executor: createBlockLog and getNextExecutionOrder returns 1
    
    Executor->>API: onBlockStart with executionOrder=1
    API->>Client: SSE BlockStartedEvent executionOrder 1
    Client->>Store: addConsole with executionOrder 1
    Store->>Terminal: Display block with order 1
    
    Executor->>Executor: Execute block
    Executor->>API: onBlockComplete with executionOrder 1
    API->>Client: SSE BlockCompletedEvent executionOrder 1
    Client->>Store: updateConsole with executionOrder 1
    
    Note over Orchestrator: Loop iteration 1 starts
    Orchestrator->>Executor: Execute block in iteration 1
    Executor->>Executor: getNextExecutionOrder returns 2
    Executor->>API: onBlockStart executionOrder 2 iterationCurrent 1
    API->>Client: SSE with executionOrder 2 and iterationCurrent 1
    Client->>Store: addConsole executionOrder 2 iterationCurrent 1
    
    Orchestrator->>Executor: Execute block in iteration 2
    Executor->>Executor: getNextExecutionOrder returns 3
    Executor->>API: onBlockStart executionOrder 3 iterationCurrent 2
    API->>Client: SSE with executionOrder 3 and iterationCurrent 2
    Client->>Store: addConsole executionOrder 3 iterationCurrent 2
    
    Note over Store: Match by blockId AND iterationCurrent
    Client->>Store: updateConsole for iterationCurrent 1
    Store->>Store: Find entry matching blockId and iterationCurrent 1
    Store->>Terminal: Update only iteration 1 entry
    
    Terminal->>Terminal: buildEntryTree sorts by executionOrder
    Terminal->>Terminal: Create synthetic parents using min executionOrder
    Terminal->>Terminal: Display in correct execution order
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

15 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 4, 2026

Additional Comments (2)

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/terminal/hooks/use-terminal-filters.ts
[P1] Sort config field no longer affects sorting

sortConfig.field is initialized to 'timestamp', but filterEntries() now always sorts by executionOrder (ignoring the selected field). This makes the UI sort selector misleading and can break expectations when users switch sort fields.

If the intent is to sort by executionOrder only, consider removing field from SortConfig/UI; otherwise branch on sortConfig.field and only use executionOrder when appropriate.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/terminal/hooks/use-terminal-filters.ts
Line: 20:23

Comment:
[P1] Sort config field no longer affects sorting

`sortConfig.field` is initialized to `'timestamp'`, but `filterEntries()` now always sorts by `executionOrder` (ignoring the selected field). This makes the UI sort selector misleading and can break expectations when users switch sort fields.

If the intent is to sort by `executionOrder` only, consider removing `field` from `SortConfig`/UI; otherwise branch on `sortConfig.field` and only use `executionOrder` when appropriate.

How can I resolve this? If you propose a fix, please make it concise.

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-workflow-execution.ts
[P2] startedAt is overwritten with completion-time value in console updates

In onBlockCompleted/onBlockError the startedAt variable is assigned from data.startedAt, and then passed into updateConsole. If data.startedAt is not the original start timestamp (or is omitted/incorrect), this will overwrite the entry’s start time in the store and skew duration/ordering display.

This seems especially likely given addConsole initially sets startedAt = new Date().toISOString() on block start, and later you’re updating it again on completion. If the server always provides the correct startedAt, it’s fine—otherwise consider only setting startedAt on the initial add, or only updating it when it was previously missing.

Also appears in the error path nearby (onBlockError).

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-workflow-execution.ts
Line: 987:993

Comment:
[P2] `startedAt` is overwritten with completion-time value in console updates

In `onBlockCompleted`/`onBlockError` the `startedAt` variable is assigned from `data.startedAt`, and then passed into `updateConsole`. If `data.startedAt` is not the original start timestamp (or is omitted/incorrect), this will overwrite the entry’s start time in the store and skew duration/ordering display.

This seems especially likely given `addConsole` initially sets `startedAt = new Date().toISOString()` on block start, and later you’re updating it again on completion. If the server always provides the correct `startedAt`, it’s fine—otherwise consider only setting `startedAt` on the initial add, or only updating it when it was previously missing.

Also appears in the error path nearby (`onBlockError`).

How can I resolve this? If you propose a fix, please make it concise.

@waleedlatif1
Copy link
Collaborator Author

@greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

5 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link

@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.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.

@waleedlatif1 waleedlatif1 merged commit 4ba2252 into staging Feb 4, 2026
12 checks passed
@waleedlatif1 waleedlatif1 deleted the improvement/tag-drop branch February 4, 2026 02:32
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.

2 participants