Skip to content

Conversation

@waleedlatif1
Copy link
Collaborator

Summary

  • fixed global commands provider to follow best practices
    • fixed bug where after hard refresh, the use-workflow-execution hook required activeWorkflowId and it wasn't avaiable yet, so because of the impl we had before it would only init once and not have the workflow Id so the keyboard entry wouldn't run. now we follow the same patterns as dedicated libraries like use-keyboard-shortcut

https://epicreact.dev/the-latest-ref-pattern-in-react/

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
Copy link

vercel bot commented Dec 24, 2025

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

1 Skipped Deployment
Project Deployment Review Updated (UTC)
docs Skipped Skipped Dec 24, 2025 8:18am

@waleedlatif1 waleedlatif1 marked this pull request as ready for review December 24, 2025 07:56
@waleedlatif1 waleedlatif1 changed the title fix(shortcut): fixed global commands provider to follow best practices fix(shortcut): fixed global keyboard commands provider to follow latest ref pattern Dec 24, 2025
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 24, 2025

Greptile Summary

Fixed keyboard shortcut bug by implementing the "latest ref pattern" in useRegisterGlobalCommands. Previously, command handlers were registered once on mount and captured stale closures - after a hard refresh, activeWorkflowId wasn't available yet, so the keyboard shortcut wouldn't work. Now handlers are wrapped to look up the current command from a ref on each invocation, ensuring fresh closures with up-to-date dependencies are always called.

Key changes:

  • Wrapped command handlers to use commandsRef.current for fresh closure access
  • Removed excessive JSDoc comments and logging statements for cleaner code
  • Reordered cancelWorkflow before runWorkflow in panel.tsx
  • Minor styling adjustment in chat.tsx (Tailwind class order)

Confidence Score: 4/5

  • Safe to merge with minor optimization opportunity
  • The latest ref pattern implementation correctly solves the stale closure bug. The logic is sound - wrapping handlers to look up fresh commands from a ref ensures up-to-date closures are called. Minor inefficiency: evaluating commands() on every render instead of only in the effect, but this doesn't affect correctness. The other files have trivial, safe changes.
  • Pay attention to global-commands-provider.tsx - consider the performance optimization suggestion

Important Files Changed

Filename Overview
apps/sim/app/workspace/[workspaceId]/providers/global-commands-provider.tsx Implements latest ref pattern to capture fresh command handlers; removed excessive logging and documentation
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/chat/chat.tsx Reordered Tailwind classes for better readability (background classes first)
apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/panel.tsx Moved cancelWorkflow definition before runWorkflow and removed redundant comment

Sequence Diagram

sequenceDiagram
    participant Panel as Panel Component
    participant Hook as useRegisterGlobalCommands
    participant Ref as commandsRef
    participant Provider as GlobalCommandsProvider
    participant Browser as Browser KeyDown
    
    Panel->>Hook: useRegisterGlobalCommands(() => commands)
    Hook->>Ref: Create commandsRef
    Note over Hook: On mount (useEffect)
    Hook->>Hook: Evaluate commands()
    Hook->>Ref: Store commands in commandsRef.current
    Hook->>Hook: Wrap each handler
    Note over Hook: Wrapper captures cmd.id<br/>but calls commandsRef.current handler
    Hook->>Provider: ctx.register(wrappedCommands)
    Provider->>Provider: Store in registryRef
    
    Note over Panel,Browser: Later: User presses Mod+Enter
    Browser->>Provider: keydown event
    Provider->>Provider: Match shortcut in registryRef
    Provider->>Hook: Call wrapped handler
    Hook->>Ref: Look up fresh command by id
    Ref->>Panel: Execute latest handler (runWorkflow/cancelWorkflow)
    
    Note over Hook,Ref: Latest Ref Pattern ensures<br/>fresh closures are called
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.

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

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

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@waleedlatif1 waleedlatif1 merged commit 1145f5c into staging Dec 24, 2025
11 checks passed
@waleedlatif1 waleedlatif1 deleted the fix/lag branch December 24, 2025 08:25
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