Skip to content

Conversation

@ThomasK33
Copy link
Member

@ThomasK33 ThomasK33 commented Oct 7, 2025

Summary

  • add a "Set Thinking Effort…" command palette action that offers off/low/medium/high options
  • persist and broadcast the selected thinking level so chat input stays in sync
  • confirm the behavior with unit coverage for the new command source

Testing

  • bun typecheck

Generated with cmux

@ammario
Copy link
Member

ammario commented Oct 8, 2025

I'd like to make this a direct key-bind -- we should probably talk about how to standardize the interface

@ThomasK33 ThomasK33 force-pushed the command-thinking branch 2 times, most recently from fbaae0d to 1944979 Compare October 8, 2025 15:06
Copy link
Member

@ammario ammario left a comment

Choose a reason for hiding this comment

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

Left comments if we go this approach but still think a direct keybind that toggles between last thinking mode and off is probably best UX.

};

window.addEventListener("cmux:setThinkingLevel", handler as EventListener);
return () => window.removeEventListener("cmux:setThinkingLevel", handler as EventListener);
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this duplication / coupling with the underlying implementation of usePersistedState, which has its own listening system.

Copy link
Member Author

Choose a reason for hiding this comment

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

✅ Fixed by using updatePersistedState() utility and enabling listener in ThinkingProvider. The event handler now only handles UI feedback (toast), not state management.

detail: { workspaceId, level: normalized, source: "command-palette" },
})
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Same here re coupling

@ThomasK33
Copy link
Member Author

Addressed Review Comments

Fixed the coupling issues with usePersistedState internals as requested:

Changes Made

  1. Created updatePersistedState() utility - Encapsulates the localStorage write and event dispatch logic that was previously duplicated in App.tsx

  2. Enabled listener in ThinkingProvider - Added { listener: true } option to usePersistedState so the context automatically picks up changes from external sources

  3. Removed state management from event handler - The cmux:setThinkingLevel event handler in ChatInput no longer calls setThinkingLevel() since the context updates automatically

  4. Renamed event for clarity - Changed from cmux:setThinkingLevel to cmux:thinkingLevelToast to make it clear this event is only for UI feedback (showing the toast), not state management

Result

  • No more duplication of usePersistedState logic
  • No more coupling between App.tsx and the internal implementation of usePersistedState
  • State updates flow through the proper channel (localStorage + storage-change event)
  • UI feedback (toasts) is handled separately via a dedicated event

All checks passing ✅

}

// Dispatch custom event for same-tab synchronization
const customEvent = new CustomEvent(`storage-change:${key}`, {
Copy link
Member

Choose a reason for hiding this comment

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

constant duplication

src/App.tsx Outdated
// Dispatch toast notification event for UI feedback
if (typeof window !== "undefined") {
window.dispatchEvent(
new CustomEvent("cmux:thinkingLevelToast", {
Copy link
Member

Choose a reason for hiding this comment

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

constant duplication

src/App.tsx Outdated
}

const normalized = THINKING_LEVELS.includes(level) ? level : "off";
const key = `thinkingLevel:${workspaceId}`;
Copy link
Member

Choose a reason for hiding this comment

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

constant duplication again -- common LLM problem needs to be actively prompted against

import type { WorkspaceSelection } from "./components/ProjectSidebar";
import ProjectSidebar from "./components/ProjectSidebar";
import NewWorkspaceModal from "./components/NewWorkspaceModal";
import { AIView } from "./components/AIView";
Copy link
Member

Choose a reason for hiding this comment

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

sidenote: App.tsx is becoming a monster but that can be handled elsewhere

@ThomasK33
Copy link
Member Author

✅ All Review Comments Addressed

1. Coupling with usePersistedState internals

Comments on: src/components/ChatInput.tsx:473 and src/App.tsx:320

Fixed:

  • Created updatePersistedState() utility function that encapsulates the localStorage write and event dispatch logic (commit 07485b3)
  • Enabled listener: true option in ThinkingProvider so it automatically picks up external changes
  • Removed setThinkingLevel() call from ChatInput event handler - now only shows toast for UI feedback
  • App.tsx now uses the utility function instead of manually duplicating the persistence logic

2. Constant duplication

Comments on: src/hooks/usePersistedState.ts:27, src/App.tsx:308, src/App.tsx:299

Fixed (commit 60387a8):

  • Created src/constants/events.ts with CUSTOM_EVENTS object for all custom event names:
    • THINKING_LEVEL_TOAST
    • INSERT_TO_CHAT_INPUT
    • OPEN_MODEL_SELECTOR
  • Created src/constants/storage.ts with getThinkingLevelKey() helper
  • Added getStorageChangeEvent() helper for the storage-change event pattern
  • Replaced all hardcoded event strings and storage key patterns throughout:
    • App.tsx
    • ChatInput.tsx
    • CommandPalette.tsx
    • ThinkingContext.tsx
    • usePersistedState.ts
    • sources.ts

3. Sidenote: App.tsx size

Comment on: src/App.tsx:10

Acknowledged: This can be handled in a separate refactoring PR. The current PR stays focused on the thinking level feature.


All checks passing ✅

@ThomasK33
Copy link
Member Author

In reply to review comment on src/components/ChatInput.tsx:

Fixed: Removed the coupling with usePersistedState internals. The event handler no longer calls setThinkingLevel() - that's now handled automatically by ThinkingProvider's listener. The event only triggers the toast for UI feedback.

@ThomasK33
Copy link
Member Author

In reply to review comment on src/App.tsx line 315:

Fixed: Replaced the manual localStorage/event dispatch with updatePersistedState() utility function that encapsulates this logic. No more coupling with usePersistedState internals.

@ThomasK33
Copy link
Member Author

In reply to review comments about constant duplication:

Fixed: All magic strings have been extracted to constants:

  • Created src/constants/events.ts with CUSTOM_EVENTS for event names
  • Created src/constants/storage.ts with getThinkingLevelKey() helper
  • Added getStorageChangeEvent() for storage-change events
  • Replaced all hardcoded strings throughout the codebase

Single source of truth established for all string literals.

Expose command palette action to set thinking effort per workspace.
Use shared storage helpers to persist levels and dispatch toast events.
Add custom event constants to replace literals across UI code.
@ThomasK33 ThomasK33 enabled auto-merge October 9, 2025 09:35
@ThomasK33 ThomasK33 added this pull request to the merge queue Oct 9, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 9, 2025
@ThomasK33 ThomasK33 added this pull request to the merge queue Oct 9, 2025
Merged via the queue into main with commit 5bff20c Oct 9, 2025
6 checks passed
@ThomasK33 ThomasK33 deleted the command-thinking branch October 9, 2025 09:51
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