Skip to content

feat: edit a text selection with AI from the editor toolbar#166

Open
djdevpro wants to merge 2 commits into
erictli:mainfrom
djdevpro:feat/ai-edit-selection
Open

feat: edit a text selection with AI from the editor toolbar#166
djdevpro wants to merge 2 commits into
erictli:mainfrom
djdevpro:feat/ai-edit-selection

Conversation

@djdevpro
Copy link
Copy Markdown

@djdevpro djdevpro commented Jun 4, 2026

Adds a wand button to the editor toolbar that sends the selected text to an AI provider and replaces the selection inline. Addresses the per-selection editing part of #37 (the diff/review part is tracked separately in #56).

  • New ai_transform_text command: transforms a snippet through the existing provider CLIs and returns the edited text (no whole-note rewrite). Reasoning model "thinking" output is stripped before applying.
image
  • AiSelectionModal: customizable one-click presets plus a free instruction, an animated status while running, and in-modal errors.
  • Settings (Integrations): selectable provider cards to choose the default, per-provider model selection for Ollama and OpenCode (listed via their CLIs), and an editable, reorderable list of quick actions.
image

Summary by CodeRabbit

  • New Features

    • "Ask AI" editor button and modal to transform selected text with presets (Fix grammar, Summarize, Rephrase)
    • Support for multiple AI providers with configurable default provider and per-provider model selection
    • AI quick-actions editor with drag-and-drop reordering and editable presets
    • Settings UI updated to manage AI providers and model selection
  • Style

    • New text-shimmer animation and motion-reduction handling
  • New Icons

    • Added Sparkles and GripVertical icons

Adds a wand button to the editor toolbar that sends the selected text to an
AI provider and replaces the selection inline. Addresses the per-selection
editing part of erictli#37 (the diff/review part is tracked separately in erictli#56).

- New `ai_transform_text` command: transforms a snippet through the existing
  provider CLIs and returns the edited text (no whole-note rewrite). Reasoning
  model "thinking" output is stripped before applying.
- AiSelectionModal: customizable one-click presets plus a free instruction, an
  animated status while running, and in-modal errors.
- Settings (Integrations): selectable provider cards to choose the default,
  per-provider model selection for Ollama and OpenCode (listed via their CLIs),
  and an editable, reorderable list of quick actions.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 4, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR introduces a complete AI text transformation feature: new Tauri backend commands enumerate local AI models and execute text transformations, a frontend service layer wraps these commands, and an editor modal allows users to select providers and presets to transform selected text. A new settings UI configures default providers, per-provider models, and customizable quick-action presets.

Changes

AI Text Transformation Feature

Layer / File(s) Summary
Type Definitions and Data Models
src/types/note.ts, src-tauri/src/lib.rs, src/components/ai/presets.ts
Settings gains opencodeModel, defaultAiProvider, and aiSelectionPresets fields; Rust AiPreset struct and TypeScript AiPreset interface define the preset shape.
Tauri Backend: Model Enumeration and Text Transformation
src-tauri/src/lib.rs
ai_list_ollama_models and ai_list_opencode_models spawn external CLIs and parse model lists; ai_transform_text routes provider-specific transformations through a shared CLI helper and registers all commands in the invoke handler.
Frontend Service Layer: Tauri Invocation Wrappers
src/services/ai.ts
listOllamaModels(), listOpencodeModels(), and aiTransformSelection() wrap Tauri command invocations with clean TypeScript signatures.
UI Icons and CSS Animations
src/components/icons/index.tsx, src/App.css
GripVerticalIcon and SparklesIcon SVG components added; text-shimmer keyframes animation with reduced-motion support for visual feedback.
AI Selection Modal: Provider, Model, and Preset Selection
src/components/ai/AiSelectionModal.tsx
Modal loads available providers, selects a preferred default, fetches provider-specific models, manages guidance input and error states, submits transformations via the service layer, and strips thinking markup before returning results.
Editor Integration: AI Selection and Result Insertion
src/components/editor/Editor.tsx
Editor adds "Ask AI" format bar button, tracks selected text and bounds, wires AiSelectionModal, and inserts AI results back into the editor at the original selection range.
Quick Actions Editor: Preset Management and Drag-Reorder
src/components/settings/QuickActionsEditor.tsx
Settings component loads presets from Tauri, renders drag-and-drop reorderable list with inline editing, validates inputs, and persists changes back to settings.
AI Provider Settings: Default Provider and Per-Provider Model Selection
src/components/settings/ToolsSettingsSection.tsx
Settings UI loads default provider and model configuration from Tauri, fetches available model lists from backend, renders a two-column provider card grid with install CTAs and per-provider model selectors, and persists updates.

Sequence Diagram

sequenceDiagram
  participant User
  participant Editor
  participant AiSelectionModal
  participant AiService
  participant TauriBackend
  participant ExternalCLI

  User->>Editor: Selects text and clicks "Ask AI"
  Editor->>AiSelectionModal: Opens modal with selected text
  AiSelectionModal->>AiService: listOllamaModels() / listOpencodeModels()
  AiService->>TauriBackend: invoke ai_list_ollama_models
  TauriBackend->>ExternalCLI: spawn ollama list
  ExternalCLI-->>TauriBackend: stdout with model names
  TauriBackend-->>AiService: Vec<String>
  AiSelectionModal->>User: Render available models and presets
  User->>AiSelectionModal: Selects preset and clicks submit
  AiSelectionModal->>AiService: aiTransformSelection(provider, text, prompt, model)
  AiService->>TauriBackend: invoke ai_transform_text
  TauriBackend->>ExternalCLI: spawn provider CLI with stdin
  ExternalCLI-->>TauriBackend: transformed text in stdout
  TauriBackend-->>AiService: AiExecutionResult
  AiSelectionModal->>AiSelectionModal: Strip thinking markup
  AiSelectionModal->>Editor: onApply(transformed text)
  Editor->>Editor: Insert result at selection range
  Editor->>User: Display transformed text in document
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • erictli/scratch#93: Adds OpenCode provider support and CLI auto-discovery; closely related to the new Opencode model-listing and selection plumbing.
  • erictli/scratch#90: Extends execute_ai_cli helper used by the new ai_transform_text command for provider CLI execution.
  • erictli/scratch#31: Earlier changes to the Tauri AI execution flow that this PR builds upon for multi-provider routing.

Suggested reviewers

  • erictli

Poem

🐰 A sparkle shines when fingers dance,
Through text with AI's subtle glance,
Presets fold like tidy leaves,
Shimmering words on quiet eves,
Selection transforms into new chance.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 30.43% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the primary feature: enabling users to edit selected text with AI directly from the editor toolbar, which is the core focus of this PR.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src-tauri/src/lib.rs`:
- Around line 3567-3573: ai_transform_text currently reads app_config and errors
if notes_folder is unset before provider dispatch; move the notes_folder lookup
and the error (the block that reads state.app_config and accesses
app_config.notes_folder) into the OpenCode branch only so other providers are
not rejected when notes_folder is absent. Locate the initial folder extraction
(the let folder = { ... app_config.notes_folder.clone().ok_or("Notes folder not
set")? }) and remove it from the shared pre-dispatch logic, then perform that
same read/ok_or logic inside the OpenCode handling branch (the branch that runs
OpenCode-specific code), referencing the same symbols (state.app_config and
app_config.notes_folder) so only OpenCode requires the working directory; do the
same relocation for the similar block currently duplicated around the other
OpenCode section (the corresponding code near the second OpenCode handling).

In `@src/components/ai/AiSelectionModal.tsx`:
- Around line 108-118: The modal currently allows in-flight async results to
call onApply after the modal closes; update AiSelectionModal to ignore or cancel
those results by tracking the modal's open state with a ref (e.g., isOpenRef) or
an AbortController: set isOpenRef.current = true when open is true and set it
false in the useEffect cleanup (the effect that registers handleEscape) and any
other cleanup path, and before calling onApply in your async callbacks (the code
referenced around lines 120-147) check isOpenRef.current (or whether the Signal
is aborted) and skip calling onApply if the modal is closed; ensure existing
cleanup removes listeners (handleEscape) and aborts or marks inflight requests
so they cannot apply after close.

In `@src/components/editor/Editor.tsx`:
- Around line 1585-1598: Wrap the body of the applyAiSelection callback in a
try-catch: first try to parse with manager.parse(result) inside the try and
catch parse errors, then validate the aiSelection range before calling
editor.chain().focus().insertContentAt(...).run(); if insertContentAt throws
(e.g., invalid range because the doc changed) catch it, clear
setAiSelection(null) and surface user feedback (e.g., call the same
notification/error helper used elsewhere or processLogger.error) with the caught
error message; reference the applyAiSelection function, manager.parse,
editor.chain().insertContentAt, and setAiSelection when making the changes.

In `@src/components/settings/QuickActionsEditor.tsx`:
- Around line 167-169: QuickActionsEditor currently uses useSensors/useSensor
with PointerSensor (activationConstraint) for drag reordering, which excludes
keyboard users; add keyboard-accessible reordering by implementing a keyboard
sensor (e.g., useSensor(KeyboardSensor)) or by adding explicit move controls
(Up/Down buttons) that call the same reordering logic used by the drag handlers
(the function that updates the presets order in QuickActionsEditor). Wire the
KeyboardSensor into useSensors alongside PointerSensor (or wire the Up/Down
buttons to the existing onDragEnd/reorder function), ensure keyboard activation
& focus handling mirrors the pointer behavior, and update aria labels/keyboard
instructions for accessibility.
- Around line 184-196: The persist function swallows errors when saving
aiSelectionPresets which leaves the UI showing unsaved changes; update persist
to surface failures and restore the last persisted presets: call
invoke("get_settings") and invoke("update_settings") as now but catch errors to
(1) show a user-friendly error (e.g., toast or set an error state) and (2)
revert the in-memory/visible items back to the last persisted presets fetched
from invoke("get_settings") (or reload settings) if update_settings fails;
ensure references to persist, invoke("get_settings"), invoke("update_settings"),
and aiSelectionPresets are used to locate and implement the error handling and
revert logic.

In `@src/components/settings/ToolsSettingsSection.tsx`:
- Around line 300-307: The clickable provider card in ToolsSettingsSection.tsx
currently uses a div which is not keyboard-accessible; update the element (the
one keyed by provider that calls handleSetDefault(provider) and uses isDefault
for styling) to be a semantic interactive control—preferably a <button> or a
radio-style input—so it is focusable and activates with Enter/Space; if you must
keep a non-button element, add tabIndex={0}, role="button" or
role="radio"/aria-checked={isDefault}, an onKeyDown handler that calls
handleSetDefault(provider) for Enter/Space, and appropriate
aria-label/aria-pressed/aria-checked attributes to convey state and make it
keyboard and screen-reader accessible.
- Around line 160-191: The three handlers (handleSetDefault,
handleSetOllamaModel, handleSetOpencodeModel) are doing read-modify-write cycles
via invoke("get_settings")/invoke("update_settings") which can race and drop
concurrent changes (same problem exists for
QuickActionsEditor/aiSelectionPresets); fix by keeping a single shared Settings
snapshot in component state (e.g., load settings into a settings state on mount
and update that state via setSettings in each handler) and then call
invoke("update_settings", { newSettings: updatedSettings }) using that
single-source-of-truth, or alternatively implement and call field-specific
backend patch commands (e.g., update_settings_field) instead of replacing the
whole object to avoid full-object races. Ensure to update and reference the
settings state in setDefaultProvider, handleSetOllamaModel and
handleSetOpencodeModel so writes merge against the latest local snapshot rather
than reloading the whole Settings on each action.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 401692c2-8a8f-41e7-b66c-956be6fa5b02

📥 Commits

Reviewing files that changed from the base of the PR and between a638f02 and 02674bc.

📒 Files selected for processing (10)
  • src-tauri/src/lib.rs
  • src/App.css
  • src/components/ai/AiSelectionModal.tsx
  • src/components/ai/presets.ts
  • src/components/editor/Editor.tsx
  • src/components/icons/index.tsx
  • src/components/settings/QuickActionsEditor.tsx
  • src/components/settings/ToolsSettingsSection.tsx
  • src/services/ai.ts
  • src/types/note.ts

Comment thread src-tauri/src/lib.rs
Comment on lines +3567 to +3573
let folder = {
let app_config = state.app_config.read().expect("app_config read lock");
app_config
.notes_folder
.clone()
.ok_or("Notes folder not set")?
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Move notes-folder lookup into the OpenCode branch only.

ai_transform_text currently rejects all providers when no notes folder is configured, even though only the OpenCode path needs a working directory.

Suggested fix
 async fn ai_transform_text(
     provider: String,
     text: String,
     prompt: String,
     model: Option<String>,
     state: State<'_, AppState>,
 ) -> Result<AiExecutionResult, String> {
-    let folder = {
-        let app_config = state.app_config.read().expect("app_config read lock");
-        app_config
-            .notes_folder
-            .clone()
-            .ok_or("Notes folder not set")?
-    };
-
     let instructions = format!(
         "You are a Markdown editor. Edit the Markdown in <text> as requested in \
          <request>, and return ONLY the resulting Markdown, with no commentary, \
          explanation, or code fences.\n\n\
@@
         "opencode" => {
+            let folder = {
+                let app_config = state.app_config.read().expect("app_config read lock");
+                app_config
+                    .notes_folder
+                    .clone()
+                    .ok_or("Notes folder not set")?
+            };
             let mut args = vec!["run".to_string()];
             if let Some(m) = model.as_deref().map(str::trim).filter(|m| !m.is_empty()) {
                 args.push("--model".to_string());
                 args.push(m.to_string());
             }

Also applies to: 3608-3627

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src-tauri/src/lib.rs` around lines 3567 - 3573, ai_transform_text currently
reads app_config and errors if notes_folder is unset before provider dispatch;
move the notes_folder lookup and the error (the block that reads
state.app_config and accesses app_config.notes_folder) into the OpenCode branch
only so other providers are not rejected when notes_folder is absent. Locate the
initial folder extraction (the let folder = { ...
app_config.notes_folder.clone().ok_or("Notes folder not set")? }) and remove it
from the shared pre-dispatch logic, then perform that same read/ok_or logic
inside the OpenCode handling branch (the branch that runs OpenCode-specific
code), referencing the same symbols (state.app_config and
app_config.notes_folder) so only OpenCode requires the working directory; do the
same relocation for the similar block currently duplicated around the other
OpenCode section (the corresponding code near the second OpenCode handling).

Comment on lines +108 to +118
useEffect(() => {
if (!open) return;
const handleEscape = (e: globalThis.KeyboardEvent) => {
if (e.key === "Escape") {
e.preventDefault();
onClose();
}
};
window.addEventListener("keydown", handleEscape);
return () => window.removeEventListener("keydown", handleEscape);
}, [open, onClose]);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Ignore in-flight AI results after the modal closes.

A request started before close can still call onApply, which may replace text after the user has dismissed the dialog.

Suggested fix
 export function AiSelectionModal({
@@
 }: AiSelectionModalProps) {
+  const runTokenRef = useRef(0);
+  const openRef = useRef(open);
@@
   useEffect(() => {
+    openRef.current = open;
     if (!open) {
+      runTokenRef.current += 1; // invalidate any in-flight run
       setGuidance("");
       setError(null);
       return;
@@
   const run = async (instruction: string) => {
     const trimmed = instruction.trim();
     if (!provider || isRunning || !trimmed) return;
+    const token = ++runTokenRef.current;
 
     setError(null);
     setIsRunning(true);
     try {
       const result = await aiTransformSelection(
@@
         model || undefined,
       );
+      if (token !== runTokenRef.current || !openRef.current) return;
       if (result.success) {
         const cleaned = stripThinking(result.output);
         if (cleaned) {
           onApply(cleaned);
@@
     } catch (err) {
+      if (token !== runTokenRef.current || !openRef.current) return;
       setError(err instanceof Error ? err.message : "The AI request failed.");
     } finally {
-      setIsRunning(false);
+      if (token === runTokenRef.current && openRef.current) {
+        setIsRunning(false);
+      }
     }
   };

Also applies to: 120-147

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/ai/AiSelectionModal.tsx` around lines 108 - 118, The modal
currently allows in-flight async results to call onApply after the modal closes;
update AiSelectionModal to ignore or cancel those results by tracking the
modal's open state with a ref (e.g., isOpenRef) or an AbortController: set
isOpenRef.current = true when open is true and set it false in the useEffect
cleanup (the effect that registers handleEscape) and any other cleanup path, and
before calling onApply in your async callbacks (the code referenced around lines
120-147) check isOpenRef.current (or whether the Signal is aborted) and skip
calling onApply if the modal is closed; ensure existing cleanup removes
listeners (handleEscape) and aborts or marks inflight requests so they cannot
apply after close.

Comment on lines +1585 to +1598
const applyAiSelection = useCallback(
(result: string) => {
if (!editor || !aiSelection) return;
const manager = editor.storage.markdown?.manager;
const parsed = manager ? manager.parse(result) : result;
editor
.chain()
.focus()
.insertContentAt({ from: aiSelection.from, to: aiSelection.to }, parsed)
.run();
setAiSelection(null);
},
[editor, aiSelection],
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add error handling for markdown parsing and content insertion.

The applyAiSelection callback lacks error handling around manager.parse(result) and insertContentAt(). If markdown parsing fails or the selection range becomes invalid (user edited the document during AI processing), the operation fails silently with no user feedback.

Compare this to the paste handler (lines 1250-1258), which wraps manager.parse() in a try-catch block.

🛡️ Proposed fix to add error handling and user feedback
 const applyAiSelection = useCallback(
   (result: string) => {
     if (!editor || !aiSelection) return;
-    const manager = editor.storage.markdown?.manager;
-    const parsed = manager ? manager.parse(result) : result;
-    editor
-      .chain()
-      .focus()
-      .insertContentAt({ from: aiSelection.from, to: aiSelection.to }, parsed)
-      .run();
-    setAiSelection(null);
+    try {
+      const manager = editor.storage.markdown?.manager;
+      const parsed = manager ? manager.parse(result) : result;
+      const success = editor
+        .chain()
+        .focus()
+        .insertContentAt({ from: aiSelection.from, to: aiSelection.to }, parsed)
+        .run();
+      
+      if (!success) {
+        toast.error("Could not apply AI result. The selection may have changed.");
+        return;
+      }
+      setAiSelection(null);
+    } catch (error) {
+      console.error("Failed to apply AI result:", error);
+      toast.error("Failed to apply AI result");
+    }
   },
   [editor, aiSelection],
 );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const applyAiSelection = useCallback(
(result: string) => {
if (!editor || !aiSelection) return;
const manager = editor.storage.markdown?.manager;
const parsed = manager ? manager.parse(result) : result;
editor
.chain()
.focus()
.insertContentAt({ from: aiSelection.from, to: aiSelection.to }, parsed)
.run();
setAiSelection(null);
},
[editor, aiSelection],
);
const applyAiSelection = useCallback(
(result: string) => {
if (!editor || !aiSelection) return;
try {
const manager = editor.storage.markdown?.manager;
const parsed = manager ? manager.parse(result) : result;
const success = editor
.chain()
.focus()
.insertContentAt({ from: aiSelection.from, to: aiSelection.to }, parsed)
.run();
if (!success) {
toast.error("Could not apply AI result. The selection may have changed.");
return;
}
setAiSelection(null);
} catch (error) {
console.error("Failed to apply AI result:", error);
toast.error("Failed to apply AI result");
}
},
[editor, aiSelection],
);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/editor/Editor.tsx` around lines 1585 - 1598, Wrap the body of
the applyAiSelection callback in a try-catch: first try to parse with
manager.parse(result) inside the try and catch parse errors, then validate the
aiSelection range before calling
editor.chain().focus().insertContentAt(...).run(); if insertContentAt throws
(e.g., invalid range because the doc changed) catch it, clear
setAiSelection(null) and surface user feedback (e.g., call the same
notification/error helper used elsewhere or processLogger.error) with the caught
error message; reference the applyAiSelection function, manager.parse,
editor.chain().insertContentAt, and setAiSelection when making the changes.

Comment on lines +167 to +169
const sensors = useSensors(
useSensor(PointerSensor, { activationConstraint: { distance: 5 } }),
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add a keyboard path for reordering.

Only pointer dragging is wired here, so keyboard users can edit presets but cannot change their order. Add a keyboard-accessible reorder path, such as explicit move controls or equivalent keyboard interaction.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/settings/QuickActionsEditor.tsx` around lines 167 - 169,
QuickActionsEditor currently uses useSensors/useSensor with PointerSensor
(activationConstraint) for drag reordering, which excludes keyboard users; add
keyboard-accessible reordering by implementing a keyboard sensor (e.g.,
useSensor(KeyboardSensor)) or by adding explicit move controls (Up/Down buttons)
that call the same reordering logic used by the drag handlers (the function that
updates the presets order in QuickActionsEditor). Wire the KeyboardSensor into
useSensors alongside PointerSensor (or wire the Up/Down buttons to the existing
onDragEnd/reorder function), ensure keyboard activation & focus handling mirrors
the pointer behavior, and update aria labels/keyboard instructions for
accessibility.

Comment on lines +184 to +196
const persist = (next: Item[]) => {
const presets = next.map(({ label, instruction }) => ({
label,
instruction,
}));
invoke<Settings>("get_settings")
.then((s) =>
invoke("update_settings", {
newSettings: { ...s, aiSelectionPresets: presets },
}),
)
.catch(() => {});
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t hide failed preset saves.

This keeps the new preset order/content on screen even when persistence fails, so the user only discovers the loss after reopening settings. Surface the failure and revert or reload the last persisted presets instead of swallowing it.

As per coding guidelines, "Implement error handling with user-friendly messages".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/settings/QuickActionsEditor.tsx` around lines 184 - 196, The
persist function swallows errors when saving aiSelectionPresets which leaves the
UI showing unsaved changes; update persist to surface failures and restore the
last persisted presets: call invoke("get_settings") and
invoke("update_settings") as now but catch errors to (1) show a user-friendly
error (e.g., toast or set an error state) and (2) revert the in-memory/visible
items back to the last persisted presets fetched from invoke("get_settings") (or
reload settings) if update_settings fails; ensure references to persist,
invoke("get_settings"), invoke("update_settings"), and aiSelectionPresets are
used to locate and implement the error handling and revert logic.

Comment on lines +160 to +191
const handleSetDefault = (provider: AiProvider) => {
setDefaultProvider(provider);
invoke<Settings>("get_settings")
.then((s) =>
invoke("update_settings", {
newSettings: { ...s, defaultAiProvider: provider },
}),
)
.catch(() => {});
};

const handleSetOllamaModel = (model: string) => {
setOllamaModel(model);
invoke<Settings>("get_settings")
.then((s) =>
invoke("update_settings", {
newSettings: { ...s, ollamaModel: model },
}),
)
.catch(() => {});
};

const handleSetOpencodeModel = (model: string) => {
setOpencodeModel(model);
invoke<Settings>("get_settings")
.then((s) =>
invoke("update_settings", {
newSettings: { ...s, opencodeModel: model },
}),
)
.catch(() => {});
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Avoid full-settings read/modify/write races here.

Each handler reloads the entire Settings object and writes back a merged copy. QuickActionsEditor persists aiSelectionPresets the same way, and update_settings replaces the full object, so overlapping saves on this screen can drop whichever change loses the race. Keep one shared Settings snapshot in state or add field-specific backend patch commands instead of issuing independent full-object writes.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/settings/ToolsSettingsSection.tsx` around lines 160 - 191, The
three handlers (handleSetDefault, handleSetOllamaModel, handleSetOpencodeModel)
are doing read-modify-write cycles via
invoke("get_settings")/invoke("update_settings") which can race and drop
concurrent changes (same problem exists for
QuickActionsEditor/aiSelectionPresets); fix by keeping a single shared Settings
snapshot in component state (e.g., load settings into a settings state on mount
and update that state via setSettings in each handler) and then call
invoke("update_settings", { newSettings: updatedSettings }) using that
single-source-of-truth, or alternatively implement and call field-specific
backend patch commands (e.g., update_settings_field) instead of replacing the
whole object to avoid full-object races. Ensure to update and reference the
settings state in setDefaultProvider, handleSetOllamaModel and
handleSetOpencodeModel so writes merge against the latest local snapshot rather
than reloading the whole Settings on each action.

Comment on lines +300 to +307
<div
key={provider}
onClick={() => handleSetDefault(provider)}
className={`flex flex-col gap-3 p-4 rounded-xl border cursor-pointer transition-all min-h-33 ${
isDefault
? "border-text/30 bg-bg-muted shadow-lg"
: "border-border hover:border-text-muted/50"
}`}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Provider selection needs real keyboard semantics.

This is a clickable div, so keyboard users can’t focus it or toggle the default provider with Enter/Space. Render the installed cards as buttons/radio controls, or add the equivalent focus and keyboard behavior.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/settings/ToolsSettingsSection.tsx` around lines 300 - 307, The
clickable provider card in ToolsSettingsSection.tsx currently uses a div which
is not keyboard-accessible; update the element (the one keyed by provider that
calls handleSetDefault(provider) and uses isDefault for styling) to be a
semantic interactive control—preferably a <button> or a radio-style input—so it
is focusable and activates with Enter/Space; if you must keep a non-button
element, add tabIndex={0}, role="button" or
role="radio"/aria-checked={isDefault}, an onKeyDown handler that calls
handleSetDefault(provider) for Enter/Space, and appropriate
aria-label/aria-pressed/aria-checked attributes to convey state and make it
keyboard and screen-reader accessible.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
package.json (1)

26-26: ⚡ Quick win

Align Tiptap package versions in package.json (lockfile already resolves to the same minor).

  • @tiptap/extension-code is ^3.18.0, but package-lock.json resolves @tiptap/extension-code, @tiptap/core, and @tiptap/extension-code-block-lowlight all to 3.20.0, so this shouldn’t cause compatibility issues with the current lock.
  • For consistency/readability, consider updating @tiptap/extension-code’s range to match the ecosystem version you’re actually using (e.g., ^3.20.0).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@package.json` at line 26, The `@tiptap/extension-code` dependency in
package.json is using ^3.18.0 while package-lock.json resolves tiptap packages
to 3.20.0; update the version string for "`@tiptap/extension-code`" to ^3.20.0 in
package.json to match the resolved versions (keeping consistency with
`@tiptap/core` and `@tiptap/extension-code-block-lowlight`), then run your package
manager to refresh the lockfile (npm install or yarn install) so the lockfile
and package.json remain aligned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@package.json`:
- Line 26: The `@tiptap/extension-code` dependency in package.json is using
^3.18.0 while package-lock.json resolves tiptap packages to 3.20.0; update the
version string for "`@tiptap/extension-code`" to ^3.20.0 in package.json to match
the resolved versions (keeping consistency with `@tiptap/core` and
`@tiptap/extension-code-block-lowlight`), then run your package manager to refresh
the lockfile (npm install or yarn install) so the lockfile and package.json
remain aligned.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 73ab09d4-0b2c-401d-919f-b671f9d24a99

📥 Commits

Reviewing files that changed from the base of the PR and between 02674bc and da5cabd.

📒 Files selected for processing (2)
  • package.json
  • src/components/editor/Editor.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/components/editor/Editor.tsx

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.

1 participant