dashboard: move Ctrl+S event listener from <Toolbar /> component to useAgentShortcuts() hook

[dimaMachina]

I think it make sense to move this listener, especially since we already have dedicated hook

@sarah-inkeep Also I noticed, undo/redo shortcuts doesn't work when I remove node/edges, should I create a ticket?

[changeset-bot]

🦋 Changeset detected

Latest commit: 40fb634

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages

Name	Type
@inkeep/agents-manage-ui	Major
@inkeep/agents-cli	Major
@inkeep/agents-manage-api	Major
@inkeep/agents-run-api	Major
@inkeep/agents-core	Major
@inkeep/agents-manage-mcp	Major
@inkeep/agents-sdk	Major
@inkeep/ai-sdk-provider	Major
@inkeep/create-agents	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[vercel]

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

Project	Deployment	Preview	Comments	Updated (UTC)
agents-manage-api	Ready	Preview	Comment	Nov 26, 2025 0:50am
agents-manage-ui	Ready	Preview	Comment	Nov 26, 2025 0:50am
agents-run-api	Ready	Preview	Comment	Nov 26, 2025 0:50am

1 Skipped Deployment

Project	Deployment	Preview	Comments	Updated (UTC)
agents-docs	Skipped			Nov 26, 2025 0:50am

[claude]

Claude finished @dimaMachina's task —— View job

---

Code Review: Move Ctrl+S Event Listener to useAgentShortcuts Hook

Todo List:

• Understand the current codebase and existing patterns
• Analyze each changed file and the refactoring approach
• Evaluate code quality, architecture, and potential issues
• Provide comprehensive review feedback
• Post final review

---

This PR successfully consolidates keyboard shortcut handling by moving the Ctrl+S save functionality from the Toolbar component into the existing useAgentShortcuts hook. The refactoring is well-executed and follows good separation of concerns principles.

✅ What Works Well

• Logical consolidation: Moving keyboard shortcuts to a centralized hook is the right architectural decision
• Consistent implementation: The Ctrl+S handler follows the same pattern as other shortcuts (undo/redo/delete)
• Proper cleanup: Event listeners are correctly managed with useEffect cleanup
• Cross-platform support: Maintains proper Mac/PC key handling with isMacOs() utility
• DOM interaction: Uses appropriate preventDefault() and programmatic button clicking

⚠️ Areas for Consideration

1. DOM Query Coupling & Brittleness

Severity: Medium

The document.querySelector('button#save-agent') approach creates tight coupling between the hook and DOM structure:

// Current approach - brittle
const el = document.querySelector<HTMLButtonElement>('button#save-agent');
el?.click();

Concerns:

• If the button ID changes, keyboard shortcuts break silently
• No compile-time safety for this relationship
• Makes component testing more complex
• Violates React's declarative paradigm by directly manipulating DOM

Potential Solutions:

1. Callback-based approach: Pass onSave callback to the hook
2. Context pattern: Use React Context to share save functionality
3. Ref forwarding: Use React refs to maintain the button reference

2. Inconsistent Button Reference Strategy

Severity: Low-Medium

The PR switches from a React ref approach to DOM querying, but doesn't explain why:

// Before: React ref (type-safe, React-idiomatic)
saveButtonRef.current?.click();

// After: DOM query (brittle, imperative)
document.querySelector<HTMLButtonElement>('button#save-agent')?.click();

The original ref-based approach was actually more robust because:

• Type-safe at compile time
• No dependency on DOM structure
• Better testability
• More idiomatic React

3. Single Responsibility & Hook Scope

Severity: Low

The useAgentShortcuts hook now handles two distinct categories of shortcuts:

• Flow-specific shortcuts: Undo/redo/delete (operate on React Flow nodes)
• Global app shortcuts: Save (operates on entire agent)

Consider whether this violates single responsibility principle and if the save shortcut might be better in a more general useGlobalShortcuts hook.

4. Missing Documentation & Testing Considerations

Severity: Low

• No JSDoc comments explaining the hook's expanded responsibilities
• The save-agent ID dependency isn't documented
• No indication of how this should be tested

🏗️ Architectural Recommendations

Option 1: Callback-Based Approach (Recommended)

export function useAgentShortcuts(onSave?: () => void) {
  // ... existing code ...
  
  if (meta && e.key === 's') {
    e.preventDefault();
    onSave?.();
    return;
  }
}

// Usage in Agent component:
useAgentShortcuts(onSubmit);

Option 2: Context Pattern

const AgentContext = createContext<{ onSave: () => void }>();

export function useAgentShortcuts() {
  const { onSave } = useContext(AgentContext);
  // ... rest of implementation
}

Option 3: Keep Ref Pattern (Safest for now)

Since the original ref approach was more robust, consider reverting to that pattern while keeping the centralized hook structure.

🔍 Additional Considerations

1. Error Boundaries: What happens if the button doesn't exist? Consider error handling
2. Performance: The current implementation is fine, no concerns
3. Accessibility: Keyboard shortcuts work well for accessibility
4. Future Extensibility: The current pattern scales well for additional shortcuts

📝 Minor Notes

• The changeset categorizes this as minor which seems appropriate
• The isMacOs utility change (agents-manage-ui/src/lib/utils.ts) appears to be a formatting fix

Final Recommendation

While this refactoring achieves the right architectural goal, I recommend addressing the DOM coupling issue before merging. The callback-based approach (Option 1) would make this change more robust and maintainable while preserving all the benefits of centralizing keyboard shortcuts.

---

[dimaMachina]

we don't need this since we check keys only for react-flow__node class which is never input, textarea or isContentEditable

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed in 7 days if no further activity occurs.

If this PR is still relevant:

• Rebase it on the latest main branch
• Add a comment explaining its current status
• Request a review if it's ready

Thank you for your contributions!