feat: in-app AI Chat panel (issue #209, Phase 8) - #252
Merged
Conversation
Adds a QML-based AI Chat dock panel that lets users control the editor with natural language. Commands are interpreted by the local LLM, which emits <tool_call> blocks that are dispatched through MCPServer and fed back into the conversation (agentic loop, up to 5 rounds). - AIChatManager singleton: bridges LLMManager (generation) and MCPServer (tool execution); QML_SINGLETON exposed as AIChatPanel 1.0 - LLMManager::generateText(): generic text-completion method reused by AIChatManager (vs generateMaterial() which has material-specific logic) - MCPServer::buildToolsList() promoted to public API so AIChatManager can build the system prompt from live tool definitions - AIChatPanel.qml: role-coloured message bubbles, streaming footer with animated thinking dots, Shift+Enter for newline, Enter to send - AI menu: "AI Chat…" action shows the dock; separator before settings - Dock auto-hides on launch; MCPServer wired to AIChatManager whenever the server is started or set externally Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an in-app AI chat QML panel and a QML-singleton C++ backend (AIChatManager) that streams LLM output, maintains chat history, parses/executes JSON tool-call responses via MCPServer, and integrates with LLMManager, MCPServer, MainWindow, build, and QML resources. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant QML as AIChatPanel (QML)
participant ACM as AIChatManager
participant LLM as LLMManager
participant MCP as MCPServer
User->>QML: Enter message / press Send
QML->>ACM: sendMessage(text)
ACM->>ACM: append user message, reset tool-loop state
ACM->>LLM: generateText(systemPrompt, conversation, maxTokensOverride)
LLM-->>ACM: streamingText (partial updates)
ACM->>QML: streamingTextChanged -> update UI
LLM-->>ACM: generationCompleted(fullText)
ACM->>ACM: sanitize & parse assistant text for JSON tool-call
alt tool call present and loop < max
ACM->>MCP: callTool(name, args)
MCP-->>ACM: tool result
ACM->>ACM: append tool result as tool message
ACM->>LLM: generateText(systemPrompt, updatedConversation)
end
ACM->>ACM: append final assistant message to history
ACM->>QML: messagesChanged -> UI renders new messages
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17b0dd084f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+424
to
+426
| QMetaObject::invokeMethod(m_worker, [this, systemPrompt, userPrompt]() { | ||
| m_worker->generate(systemPrompt, userPrompt); | ||
| }, Qt::QueuedConnection); |
There was a problem hiding this comment.
generateText() enqueues a normal m_worker->generate(...) call, but completions still flow through onWorkerGenerationCompleted(), which unconditionally runs cleanupGeneratedScript() + validateMaterialScript() and retry prompts for Ogre materials. In the AI chat flow this means ordinary assistant replies are treated as invalid (e.g., not starting with material), retried with material-specific correction prompts, and returned as the wrong content after retries. Please add a generation mode/path so generic chat text bypasses material-script validation/rewrite.
Useful? React with 👍 / 👎.
| color: sendBtnArea.containsMouse | ||
| ? PropertiesPanelController.accentColor | ||
| : PropertiesPanelController.buttonColor | ||
| enabled: AIChatManager.modelAvailable && !AIChatManager.isGenerating |
There was a problem hiding this comment.
The send/stop button is disabled whenever AIChatManager.isGenerating is true, but that is exactly when the click handler is supposed to call AIChatManager.stopGeneration(). Because the button (and its MouseArea) is disabled in that state, users cannot cancel an in-flight response and must wait for completion/error.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
qml/AIChatPanel.qml (1)
169-174: Animation stagger may not produce intended wave effect.The
PauseAnimationat the end of the loop means all dots start simultaneously and pause differently before the next iteration. For a proper sequential "wave" pattern, consider moving the pause before the fade-in or using a delayed start withrunning: falseand a Timer.♻️ Alternative approach for staggered animation
SequentialAnimation on opacity { loops: Animation.Infinite + PauseAnimation { duration: index * 150 } NumberAnimation { to: 1.0; duration: 400 } NumberAnimation { to: 0.3; duration: 400 } - PauseAnimation { duration: index * 150 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qml/AIChatPanel.qml` around lines 169 - 174, The SequentialAnimation on opacity currently places PauseAnimation at the end so all dots animate in sync then pause; to create a staggered wave move the PauseAnimation before the fade-in step (i.e., place PauseAnimation { duration: index * 150 } as the first child of the SequentialAnimation) so each dot delays before starting its NumberAnimation, or alternatively replace the end pause with a per-dot Timer/delayed start (using running: false toggled by a Timer) tied to index to achieve the intended stagger; update the SequentialAnimation children (SequentialAnimation, NumberAnimation, PauseAnimation) accordingly to maintain loops: Animation.Infinite.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qml/AIChatPanel.qml`:
- Around line 229-257: The send button is disabled while
AIChatManager.isGenerating, making the MouseArea onClicked branch that calls
AIChatManager.stopGeneration() unreachable; update the enabled logic so clicks
are allowed during generation (e.g., change the Rectangle `sendBtn` enabled
predicate to only depend on AIChatManager.modelAvailable or set
`sendBtnArea.enabled` to `AIChatManager.modelAvailable` while keeping `sendBtn`
visual state driven by `AIChatManager.isGenerating`) so that `sendBtnArea` can
invoke `stopGeneration()` when the icon shows "■" and otherwise call `doSend()`.
- Around line 127-131: The footer Item is hidden when
AIChatManager.streamingText is empty, preventing the thinking-dots that rely on
isGenerating from appearing; change the footer's visible binding to be true when
AIChatManager.streamingText.length > 0 OR AIChatManager.isGenerating, and adjust
the height expression (now using streamBubble.height + 4) to use that same OR
condition so the footer reserves space when isGenerating is true (but can still
collapse when neither streamingText nor isGenerating are set).
In `@src/AIChatManager.cpp`:
- Around line 174-190: onGenerationCompleted() currently calls
MCPServer::callTool() synchronously on the UI thread (freezing the app); change
this to run the tool call asynchronously (e.g., QtConcurrent::run or move a
worker to a QThread) and marshal the result back to the main thread before
updating UI state. Specifically: dispatch MCPServer::callTool(toolName,
toolArgs) off the UI thread, capture its QJsonObject result in the worker, then
post the result back to the main thread (via signal/slot or
QMetaObject::invokeMethod) where you perform SentryReporter::addBreadcrumb,
build the resultText, call appendMessage("tool", ...), increment m_toolLoopDepth
and finally call startGeneration(buildSystemPrompt(),
buildConversationPrompt()). Ensure any UI-only operations (appendMessage,
startGeneration) run on the main thread and that error/timeout handling for the
async task resumes generation or aborts gracefully.
- Around line 34-40: The current connections to
LLMManager::generationProgress/generationCompleted/generationError/generationStopped
are global and receive events from other generators; modify AIChatManager to
scope callbacks by a generation/request token: when starting a chat generation
capture the returned generationId/requestToken from LLMManager (or add one to
the start method), store it in a member like m_currentGenerationId, and replace
the direct signal-slot connects to
generationProgress/generationCompleted/generationError/generationStopped with
handlers (either connected lambdas or slot wrappers) that first check the
signal’s generationId matches m_currentGenerationId before mutating
m_streamingText, appending chat messages, or calling
executeToolCallsAndContinue; ensure generationStopped and generationError
similarly check the token so unrelated material generation cannot interfere.
In `@src/LLMManager.cpp`:
- Around line 418-427: generateText currently dispatches text generations but
there's no request-type tracking, so onWorkerGenerationCompleted always runs the
material-script cleanup/validation/retry path and can misclassify normal chat
replies (e.g., "<tool_call>{...}</tool_call>") as materials; add a request-type
flag or enum on LLMManager (e.g., CurrentRequestType { None, TextChat,
MaterialGeneration }), set it to TextChat inside generateText before invoking
m_worker->generate, and update onWorkerGenerationCompleted to check this flag:
if the request type is TextChat, skip the material-only cleanup/validation/retry
code and forward the full generated text to AIChatManager instead; ensure the
flag is cleared/reset after completion or error so subsequent requests behave
correctly.
In `@src/mainwindow.cpp`:
- Line 1544: AIChatManager currently only receives m_mcpServer when the HTTP
server is started; change initialization so an internal MCPServer instance (or
the existing m_mcpServer object) is created/assigned for chat unconditionally
and passed to AIChatManager::instance()->setMcpServer(m_mcpServer) during main
window startup, while keeping startHttp() as an opt-in call that only enables
the external HTTP listener; ensure the same unconditional setMcpServer call is
applied in both places where m_mcpServer is handed to AIChatManager (the current
setMcpServer invocation and the other occurrence around startHttp()) so chat
tooling and editor commands work even when MCP/enabled is false but the HTTP
listener remains optional.
---
Nitpick comments:
In `@qml/AIChatPanel.qml`:
- Around line 169-174: The SequentialAnimation on opacity currently places
PauseAnimation at the end so all dots animate in sync then pause; to create a
staggered wave move the PauseAnimation before the fade-in step (i.e., place
PauseAnimation { duration: index * 150 } as the first child of the
SequentialAnimation) so each dot delays before starting its NumberAnimation, or
alternatively replace the end pause with a per-dot Timer/delayed start (using
running: false toggled by a Timer) tied to index to achieve the intended
stagger; update the SequentialAnimation children (SequentialAnimation,
NumberAnimation, PauseAnimation) accordingly to maintain loops:
Animation.Infinite.
🪄 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: 2cadd71b-85d1-489f-ba8d-e483b464e3fc
📒 Files selected for processing (10)
qml/AIChatPanel.qmlsrc/AIChatManager.cppsrc/AIChatManager.hsrc/CMakeLists.txtsrc/LLMManager.cppsrc/LLMManager.hsrc/MCPServer.hsrc/mainwindow.cppsrc/mainwindow.hsrc/qml_resources.qrc
Comment on lines
+34
to
+40
| auto* llm = LLMManager::instance(); | ||
| connect(llm, &LLMManager::generationProgress, this, &AIChatManager::onGenerationProgress); | ||
| connect(llm, &LLMManager::generationCompleted, this, &AIChatManager::onGenerationCompleted); | ||
| connect(llm, &LLMManager::generationError, this, &AIChatManager::onGenerationError); | ||
| connect(llm, &LLMManager::generationStopped, this, &AIChatManager::onGenerationStopped); | ||
| connect(llm, &LLMManager::modelLoadedChanged, this, &AIChatManager::modelAvailableChanged); | ||
| } |
There was a problem hiding this comment.
Scope chat callbacks to the generation that started them.
LLMManager is a shared singleton, so these generationProgress/Completed/Error/Stopped connections also receive events from other consumers such as material generation. That lets an unrelated request overwrite m_streamingText, append chat messages, and even trigger executeToolCallsAndContinue() here. Chat needs a request token or dedicated signal path before accepting callbacks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/AIChatManager.cpp` around lines 34 - 40, The current connections to
LLMManager::generationProgress/generationCompleted/generationError/generationStopped
are global and receive events from other generators; modify AIChatManager to
scope callbacks by a generation/request token: when starting a chat generation
capture the returned generationId/requestToken from LLMManager (or add one to
the start method), store it in a member like m_currentGenerationId, and replace
the direct signal-slot connects to
generationProgress/generationCompleted/generationError/generationStopped with
handlers (either connected lambdas or slot wrappers) that first check the
signal’s generationId matches m_currentGenerationId before mutating
m_streamingText, appending chat messages, or calling
executeToolCallsAndContinue; ensure generationStopped and generationError
similarly check the token so unrelated material generation cannot interfere.
Comment on lines
+174
to
+190
| SentryReporter::addBreadcrumb("ai.tool_call", toolName); | ||
| QJsonObject result = m_mcpServer->callTool(toolName, toolArgs); | ||
|
|
||
| QString resultText; | ||
| QJsonArray content = result["content"].toArray(); | ||
| if (!content.isEmpty()) | ||
| resultText = content.first().toObject()["text"].toString(); | ||
| else | ||
| resultText = QJsonDocument(result).toJson(QJsonDocument::Compact); | ||
|
|
||
| QString toolEntry = QString("[Tool: %1]\n%2").arg(toolName, resultText.trimmed()); | ||
| appendMessage("tool", toolEntry, true); | ||
| } | ||
|
|
||
| // Feed tool results back to the LLM for a follow-up response | ||
| ++m_toolLoopDepth; | ||
| startGeneration(buildSystemPrompt(), buildConversationPrompt()); |
There was a problem hiding this comment.
Don't run MCP tools inline on the UI thread.
onGenerationCompleted() executes callTool() synchronously, and MCPServer::callTool() dispatches heavyweight operations like mesh I/O and screenshots on the calling thread. That will freeze the editor and stall the chat UI until the tool returns. Please move the tool step to an async/deferred path and only resume generation once it completes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/AIChatManager.cpp` around lines 174 - 190, onGenerationCompleted()
currently calls MCPServer::callTool() synchronously on the UI thread (freezing
the app); change this to run the tool call asynchronously (e.g.,
QtConcurrent::run or move a worker to a QThread) and marshal the result back to
the main thread before updating UI state. Specifically: dispatch
MCPServer::callTool(toolName, toolArgs) off the UI thread, capture its
QJsonObject result in the worker, then post the result back to the main thread
(via signal/slot or QMetaObject::invokeMethod) where you perform
SentryReporter::addBreadcrumb, build the resultText, call appendMessage("tool",
...), increment m_toolLoopDepth and finally call
startGeneration(buildSystemPrompt(), buildConversationPrompt()). Ensure any
UI-only operations (appendMessage, startGeneration) run on the main thread and
that error/timeout handling for the async task resumes generation or aborts
gracefully.
| if (!m_mcpServer) { | ||
| m_mcpServer = new MCPServer(this); | ||
| m_mcpServer->setMainWindow(this); | ||
| AIChatManager::instance()->setMcpServer(m_mcpServer); |
There was a problem hiding this comment.
Decouple chat tooling from the HTTP server setting.
These are currently the only paths that hand an MCPServer to AIChatManager. If MCP/enabled is false on startup, the chat prompt is built without tools and editor commands never execute, even though the PR objective/test plan expect in-app actions like mesh scaling to work. Please attach an internal MCPServer for chat regardless of startHttp(), and keep the HTTP listener opt-in on top of that.
Also applies to: 1573-1573
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mainwindow.cpp` at line 1544, AIChatManager currently only receives
m_mcpServer when the HTTP server is started; change initialization so an
internal MCPServer instance (or the existing m_mcpServer object) is
created/assigned for chat unconditionally and passed to
AIChatManager::instance()->setMcpServer(m_mcpServer) during main window startup,
while keeping startHttp() as an opt-in call that only enables the external HTTP
listener; ensure the same unconditional setMcpServer call is applied in both
places where m_mcpServer is handed to AIChatManager (the current setMcpServer
invocation and the other occurrence around startHttp()) so chat tooling and
editor commands work even when MCP/enabled is false but the HTTP listener
remains optional.
- Root cause of tool calls not executing: LLMManager.onWorkerGenerationCompleted ran validateMaterialScript() on all completions, causing <tool_call> responses to fail validation and get retried with the wrong material system prompt. Fixed by adding m_rawTextMode flag that bypasses cleanup/validation when generateText() (i.e. AI Chat) is active. - Expose AIChatManager.currentModelName property (delegates to LLMManager), connected to currentModelNameChanged so it updates when model is switched in AI Settings. - Show model filename in chat header (truncated to 160px); updates live on model switch without restarting the panel. - Improve system prompt: include full parameter descriptions per tool so the LLM knows valid values (e.g. create_primitive type: sphere/cube/plane/...) - Reduce kMaxToolLoops 5→3 to limit runaway agentic loops. - Set dock minimum height 350px + resizeDocks 400px vertical on first open. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tion - LLMWorker: use return (not break) after decode error so generationCompleted never fires after generationError — prevents the partial text from hitting material validation/retry in LLMManager and producing spurious material output - AIChatManager: truncate conversation history to last 10 messages before building prompt, preventing KV cache overflow that caused decode errors - QML: replace Text with readOnly TextEdit in message bubbles and streaming footer — users can now select and copy any chat text with the mouse Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
TextEdit does not auto-size like Text — it needs an explicit height set to contentHeight, and parent height cannot be derived from an anchored TextEdit child the way it can from Text.implicitHeight. Switch the message bubble to a Column layout so height flows naturally: Column sizes to its children, bubble = column.height + padding. Set TextEdit.height = contentHeight explicitly on both the message delegate and the streaming footer. Also fix streaming bubble height: streamLabel.contentHeight + 28 instead of streamLabel.implicitHeight (which is 0 for TextEdit). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
contentHeight resolves to 0 when TextEdit width is established through an anchor chain that includes the parent whose height depends on contentHeight. Break the cycle by positioning TextEdit with explicit x/y/width (pixel values, not anchors), so contentHeight has a stable width to compute against. Bubble height = msgLabel.y + contentHeight + 8. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
qml/AIChatPanel.qml (2)
233-240:⚠️ Potential issue | 🔴 CriticalKeep the stop button clickable during generation.
The control is disabled exactly when
AIChatManager.isGeneratingflips true, so the stop branch inonClickedcan never run. Leave it enabled whenever a model is available.💡 Suggested fix
Rectangle { id: sendBtn anchors { right: parent.right; verticalCenter: parent.verticalCenter; rightMargin: 8 } width: 32; height: 32; radius: 4 color: sendBtnArea.containsMouse ? PropertiesPanelController.accentColor : PropertiesPanelController.buttonColor - enabled: AIChatManager.modelAvailable && !AIChatManager.isGenerating + enabled: AIChatManager.modelAvailableAlso applies to: 249-259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qml/AIChatPanel.qml` around lines 233 - 240, The send button (Rectangle id sendBtn) is being disabled while AIChatManager.isGenerating, preventing the stop branch in its onClicked handler from ever running; change its enabled expression to only check AIChatManager.modelAvailable (remove the && !AIChatManager.isGenerating) so the button remains clickable during generation, and ensure the onClicked logic still checks AIChatManager.isGenerating to trigger the stop flow; apply the same change to the other similar Rectangle (the duplicate at lines 249-259).
131-135:⚠️ Potential issue | 🟠 MajorShow the streaming footer before the first token arrives.
The footer collapses whenever
streamingTextis empty, so the thinking dots at Lines 161-165 never become visible. Keep the footer visible whileAIChatManager.isGeneratingis true.💡 Suggested fix
footer: Item { width: messageList.width - 16 - height: AIChatManager.streamingText.length > 0 ? streamBubble.height + 4 : 0 - visible: AIChatManager.streamingText.length > 0 + height: (AIChatManager.isGenerating || AIChatManager.streamingText.length > 0) ? streamBubble.height + 4 : 0 + visible: AIChatManager.isGenerating || AIChatManager.streamingText.length > 0Also applies to: 161-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qml/AIChatPanel.qml` around lines 131 - 135, The footer currently hides when AIChatManager.streamingText is empty, preventing the "thinking" dots from appearing; update the footer's visibility and height logic in the footer Item so it remains visible while AIChatManager.isGenerating is true (i.e., use visible: AIChatManager.isGenerating || AIChatManager.streamingText.length > 0) and compute height to use streamBubble.height + 4 when either isGenerating or streamingText has content, ensuring the streamBubble area is reserved before the first token arrives.src/mainwindow.cpp (1)
1543-1547:⚠️ Potential issue | 🟠 MajorTool-enabled chat is still coupled to starting the HTTP listener.
These are still the only paths that ever hand an
MCPServertoAIChatManager. WhenMCP/enabledis false on startup, the chat panel comes up without tools, so editor commands like the mesh-scaling example never execute. Initialize/pass an internalMCPServerunconditionally and keepstartHttp()as the opt-in external layer on top.Also applies to: 1574-1575
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 1543 - 1547, The code only creates and assigns an MCPServer to AIChatManager when m_mcpServer is null and implicitly tied to starting the HTTP listener; change initialization so an internal MCPServer is always created and passed to AIChatManager (create m_mcpServer = new MCPServer(this); m_mcpServer->setMainWindow(this); AIChatManager::instance()->setMcpServer(m_mcpServer); unconditionally during MainWindow construction/initialization), and keep startHttp() as a separate opt-in method that only starts the external HTTP listener on the existing m_mcpServer rather than gating MCPServer creation; update any other creation sites (e.g., the block around lines 1574-1575) to follow the same pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/AIChatManager.cpp`:
- Around line 146-160: The fallback branch currently appends the raw
assistantText (leaking <tool_call> markup) when toolBlocks exist but m_mcpServer
is null or m_toolLoopDepth >= kMaxToolLoops; change the appendMessage call to
use visibleText.trimmed() instead of assistantText.trimmed(), and if visibleText
is empty after stripping tool markup emit a short human-readable fallback (e.g.
"Unable to process tool output") via appendMessage("assistant", ...); keep the
existing state updates (m_isGenerating = false; m_toolLoopDepth = 0; emit
isGeneratingChanged()) and ensure this logic is applied in the same branch that
checks toolBlocks, m_mcpServer, and m_toolLoopDepth.
---
Duplicate comments:
In `@qml/AIChatPanel.qml`:
- Around line 233-240: The send button (Rectangle id sendBtn) is being disabled
while AIChatManager.isGenerating, preventing the stop branch in its onClicked
handler from ever running; change its enabled expression to only check
AIChatManager.modelAvailable (remove the && !AIChatManager.isGenerating) so the
button remains clickable during generation, and ensure the onClicked logic still
checks AIChatManager.isGenerating to trigger the stop flow; apply the same
change to the other similar Rectangle (the duplicate at lines 249-259).
- Around line 131-135: The footer currently hides when
AIChatManager.streamingText is empty, preventing the "thinking" dots from
appearing; update the footer's visibility and height logic in the footer Item so
it remains visible while AIChatManager.isGenerating is true (i.e., use visible:
AIChatManager.isGenerating || AIChatManager.streamingText.length > 0) and
compute height to use streamBubble.height + 4 when either isGenerating or
streamingText has content, ensuring the streamBubble area is reserved before the
first token arrives.
In `@src/mainwindow.cpp`:
- Around line 1543-1547: The code only creates and assigns an MCPServer to
AIChatManager when m_mcpServer is null and implicitly tied to starting the HTTP
listener; change initialization so an internal MCPServer is always created and
passed to AIChatManager (create m_mcpServer = new MCPServer(this);
m_mcpServer->setMainWindow(this);
AIChatManager::instance()->setMcpServer(m_mcpServer); unconditionally during
MainWindow construction/initialization), and keep startHttp() as a separate
opt-in method that only starts the external HTTP listener on the existing
m_mcpServer rather than gating MCPServer creation; update any other creation
sites (e.g., the block around lines 1574-1575) to follow the same pattern.
🪄 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: a16614a7-38cc-45fe-bfee-6bc58725ee3d
📒 Files selected for processing (6)
qml/AIChatPanel.qmlsrc/AIChatManager.cppsrc/AIChatManager.hsrc/LLMManager.cppsrc/LLMManager.hsrc/mainwindow.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/LLMManager.cpp
- src/AIChatManager.h
Comment on lines
+146
to
+160
| // Separate visible text from tool call markers | ||
| QString visibleText = assistantText; | ||
| visibleText.replace(re, QString()).simplified(); | ||
|
|
||
| QRegularExpressionMatchIterator it = re.globalMatch(assistantText); | ||
| QStringList toolBlocks; | ||
| while (it.hasNext()) | ||
| toolBlocks << it.next().captured(1).trimmed(); | ||
|
|
||
| if (toolBlocks.isEmpty() || !m_mcpServer || m_toolLoopDepth >= kMaxToolLoops) { | ||
| // Plain response — add to history and finish | ||
| appendMessage("assistant", assistantText.trimmed()); | ||
| m_isGenerating = false; | ||
| m_toolLoopDepth = 0; | ||
| emit isGeneratingChanged(); |
There was a problem hiding this comment.
Don't leak <tool_call> markup on the fallback path.
When tool blocks exist but m_mcpServer is null or the loop cap is hit, this branch appends assistantText verbatim, so the user sees the raw XML/JSON protocol instead of a normal reply. Use visibleText here, or emit a short fallback message when there is no human-readable narration.
💡 Suggested fix
- if (toolBlocks.isEmpty() || !m_mcpServer || m_toolLoopDepth >= kMaxToolLoops) {
- // Plain response — add to history and finish
- appendMessage("assistant", assistantText.trimmed());
+ if (toolBlocks.isEmpty()) {
+ appendMessage("assistant", assistantText.trimmed());
+ m_isGenerating = false;
+ m_toolLoopDepth = 0;
+ emit isGeneratingChanged();
+ return;
+ }
+
+ if (!m_mcpServer || m_toolLoopDepth >= kMaxToolLoops) {
+ appendMessage(
+ "assistant",
+ visibleText.trimmed().isEmpty()
+ ? QStringLiteral("I couldn't execute that editor action.")
+ : visibleText.trimmed());
m_isGenerating = false;
m_toolLoopDepth = 0;
emit isGeneratingChanged();
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/AIChatManager.cpp` around lines 146 - 160, The fallback branch currently
appends the raw assistantText (leaking <tool_call> markup) when toolBlocks exist
but m_mcpServer is null or m_toolLoopDepth >= kMaxToolLoops; change the
appendMessage call to use visibleText.trimmed() instead of
assistantText.trimmed(), and if visibleText is empty after stripping tool markup
emit a short human-readable fallback (e.g. "Unable to process tool output") via
appendMessage("assistant", ...); keep the existing state updates (m_isGenerating
= false; m_toolLoopDepth = 0; emit isGeneratingChanged()) and ensure this logic
is applied in the same branch that checks toolBlocks, m_mcpServer, and
m_toolLoopDepth.
|
|
||
| // Generation | ||
| Q_INVOKABLE void generateMaterial(const QString &prompt, const QString ¤tMaterial = QString(), const QStringList &availableTextures = QStringList()); | ||
| Q_INVOKABLE void generateText(const QString &systemPrompt, const QString &userPrompt); |
There was a problem hiding this comment.
Make generation routing per-request, not manager-global.
Lines 101 and 178 add a second generation flow, but routing is still done with one shared generation* signal channel and one shared m_rawTextMode bit. That lets one request steal another request's callbacks or completion path, e.g. material output reaching chat or chat output bypassing material cleanup/validation. Please carry request id/type with each queued job instead of storing mode on the singleton.
Also applies to: 178-178
…bles TextEdit.contentHeight/implicitHeight is unreliable for ListView delegate sizing because QML resolves bindings lazily and the width chain may not be established before contentHeight is evaluated. Use the overlay pattern instead: - Text (reliable implicitHeight) drives the bubble layout - Invisible TextEdit (color: transparent) is layered exactly over the Text with the same x/y/width/height — handles mouse selection only - Selected text is rendered at selectedTextColor so it stays readable through the selection highlight - Bubble height = msgLabel.y + msgLabel.implicitHeight + 8 (Text) Same fix applied to the streaming footer. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t overlay Previous attempts changed the height formula, breaking the layout. Go back to the exact original: bubble.height = msgLabel.implicitHeight + 28 (+28 = roleLabel ~13px + topMargins + bottom padding), msgLabel is a plain Text with normal anchors — implicitHeight computed reliably. TextEdit (transparent, selection-only) is added separately with: height: msgLabel.implicitHeight (borrowed from Text, never circular) same anchors as msgLabel This keeps the Text driving all layout; TextEdit is just a hit-test layer. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
TextEdit (QtQuick) does not have a background property — that belongs to TextArea/TextField (QtQuick.Controls). The invalid assignment caused a QML engine error that prevented the delegate from rendering, showing a blank area instead of messages. Removed background:null from both overlay TextEdit instances; the TextArea input field retains it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Root cause: after executing tool calls, the follow-up generation used the full system prompt (all tool definitions, ~800 tokens) plus the growing conversation history. The model would often emit another tool call in the follow-up, repeating the cycle until hitting the context limit and erroring. - Replace full system prompt with a minimal summary-only prompt for follow-up generations: "write ONE short sentence, no tool calls" - Drop kMaxToolLoops 3→1: one round of tool execution + one summary is the right shape; complex multi-step tasks can be chained by the user - Shrink history window for follow-ups: 6 messages instead of 10 to keep the summary prompt + tool result well within context size Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Stop button: - LLMWorker resets m_stopRequested=false at the start of each generate(), so a stop pressed during tool execution was silently cleared when the follow-up generation started. Added m_stopRequested flag to AIChatManager that is checked in startGeneration() before invoking LLMManager — if set, abort immediately and reset all generating state. Tool call parsing: - The model sometimes outputs `tool_call> (backtick) instead of <tool_call> because it confuses the < with a markdown code-fence opening. Broadened regex from <tool_call> to \W?tool_call> to accept any non-word prefix. - Updated system prompt to put JSON inline on the same line as the tags (no newline between <tool_call> and the JSON) to reduce markdown confusion. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Models like Phi-3/Llama-3/Mistral echo back <|assistant|> and similar special tokens as literal text in their output, which then gets stored in conversation history and re-fed, growing the prompt each turn and corrupting the context. Added cleanGeneratedText() helper called on every completed and in-progress generation: - Removes <|...|> tokens via regex (covers Phi-3, Llama-3, Mistral, etc.) - Truncates output at the first hallucinated "User:" / "Human:" line (model generating the next user turn as part of its own response) Applied to both onGenerationCompleted and onGenerationProgress so the streaming preview is also clean. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The model keeps mangling the tag format (<tool_call>, `tool_call>,
tool_call}, ...). All regex attempts to match the opening tag break
on the next variation.
Root fix: abandon tag-based extraction entirely.
- extractToolJsonBlocks() scans the raw text for balanced { } blocks,
parses each with QJsonDocument, and keeps those with both "name" and
"arguments" keys — these are tool calls regardless of surrounding text.
- System prompt changed to instruct the model to output a bare JSON
object on its own line (no tags), which is far more reliably produced.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
qml/AIChatPanel.qml (2)
146-149:⚠️ Potential issue | 🟠 MajorKeep the footer visible during the pre-token “thinking” state.
The footer still collapses when
streamingTextis empty, so the dots never get laid out and the panel goes blank until the first token arrives.Also applies to: 189-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qml/AIChatPanel.qml` around lines 146 - 149, The footer Item currently hides when AIChatManager.streamingText is empty causing the pre-token "thinking" dots to never render; update the footer's visible and height logic to also consider the pre-token thinking flag (e.g., AIChatManager.isThinking or equivalent) so the footer stays visible and sized while thinking. Specifically, in the footer Item (and the duplicate block at the other location) change visible to something like "AIChatManager.streamingText.length > 0 || AIChatManager.isThinking" and set height to use "streamBubble.height + 4" when either streamingText has content or isThinking is true so the dots can be laid out before the first token arrives.
268-287:⚠️ Potential issue | 🔴 CriticalMake the stop button clickable while generation is active.
sendBtn.enabledbecomesfalseexactly when the handler wants to callAIChatManager.stopGeneration(), so the"■"state is unreachable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qml/AIChatPanel.qml` around lines 268 - 287, The send button is disabled while generation is active so the stop action can't be triggered; update the enabled expression so the control stays clickable during generation (e.g. change the enabled binding from AIChatManager.modelAvailable && !AIChatManager.isGenerating to a condition that allows clicks when generating, such as AIChatManager.modelAvailable || AIChatManager.isGenerating), leaving the existing MouseArea onClicked handler that calls AIChatManager.stopGeneration() when AIChatManager.isGenerating intact.src/AIChatManager.cpp (3)
221-255:⚠️ Potential issue | 🟠 MajorKeep tool execution off the GUI thread.
m_mcpServer->callTool()still runs inline here. Heavy tools will freeze the dock and make stop/clear unresponsive until the call returns; resume the chat only after an async/deferred tool result comes back.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AIChatManager.cpp` around lines 221 - 255, The loop executes m_mcpServer->callTool() on the GUI thread causing freezes; move the tool call and JSON parsing off the GUI thread (e.g., QtConcurrent::run or a worker QThread) and perform SentryReporter::addBreadcrumb before dispatch; marshal results back to the GUI thread via signals/slots or QMetaObject::invokeMethod(Qt::QueuedConnection) to call appendMessage and to increment m_toolLoopDepth and call startGeneration only after the async result arrives; ensure you preserve the same resultText extraction logic (from result["content"]) when handling the returned QJsonObject and that any GUI updates (appendMessage, startGeneration) run on the main thread.
34-38:⚠️ Potential issue | 🟠 MajorScope generation callbacks to the chat request that started them.
LLMManageris shared. These slots will still consume progress/completion/error signals from other features using the same singleton, which can overwrite chat state or kick off tool execution for someone else’s request.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AIChatManager.cpp` around lines 34 - 38, The LLMManager signals are currently connected globally to AIChatManager slots (connect(llm, &LLMManager::generationProgress, this, &AIChatManager::onGenerationProgress) etc.), so progress/completion/errors from other requests can affect this chat; change the connections to be request-scoped by using per-request identifiers and guarded callbacks: capture the chat's requestId (e.g., AIChatManager::currentRequestId or a newly stored requestId) in a lambda or wrapper when calling connect to check the incoming signal's requestId before invoking AIChatManager::onGenerationProgress/onGenerationCompleted/onGenerationError/onGenerationStopped, and store the QMetaObject::Connection handles so you explicitly disconnect those connections when the request completes or stops to avoid cross-request signal delivery.
198-214:⚠️ Potential issue | 🟠 MajorDon’t surface raw tool protocol on the fallback path.
When
m_mcpServeris null,toolBlocksis forced empty, and when the loop cap is hit this branch still appendsassistantTextverbatim. Users will see the raw JSON tool call instead of narration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AIChatManager.cpp` around lines 198 - 214, The fallback path currently forces toolBlocks empty when m_mcpServer is null and then appends assistantText verbatim, which can expose raw tool JSON; change AIChatManager::executeToolCallsAndContinue so toolBlocks is always computed from assistantText (remove the m_mcpServer ? ... : ... ternary and call extractToolJsonBlocks unconditionally) and use visibleText.trimmed() (not assistantText) when calling appendMessage("assistant", ...) in the plain-response / loop-cap branch so any extracted JSON blocks are stripped before appending.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qml/AIChatPanel.qml`:
- Around line 231-241: The footer container's height uses
inputField.implicitHeight (Math.max(40, inputField.implicitHeight + 16)) which
allows the row to grow past the editor's 80px cap; change the container height
to use the same capped value as the editor (e.g. Math.max(40,
Math.min(inputField.implicitHeight, 80) + 16) or Math.max(40, inputField.height
+ 16)) so the inputRow/container and the TextArea (inputField) use the same 80px
cap; update the height expression in the container that wraps TextArea (the
block that currently sets height: Math.max(40, inputField.implicitHeight + 16))
accordingly.
In `@src/AIChatManager.cpp`:
- Around line 172-195: The extractor extractToolJsonBlocks treats every '{' and
'}' as structural which breaks when braces appear inside JSON strings; modify
the scanning loop to track JSON string context and escapes (e.g., maintain a
bool inString and a prevEscape flag), only increment/decrement depth when not
inString, and toggle inString on encountering a double-quote that is not
escaped; ensure escaped quotes (backslash) are handled so braces inside string
values (like "text":"}") are ignored and valid JSON blocks are correctly parsed
by QJsonDocument::fromJson.
In `@src/AIChatManager.h`:
- Around line 39-40: AIChatManager currently stores a raw MCPServer* in
m_mcpServer (set via setMcpServer) which can become dangling; change m_mcpServer
to QPointer<MCPServer> (include <QPointer>) and update setMcpServer(MCPServer*
server) to assign to that QPointer, or if you prefer keep a raw pointer connect
the server's destroyed() signal inside setMcpServer to a slot/lambda that clears
m_mcpServer; ensure any uses like onGenerationCompleted() →
executeToolCallsAndContinue() check the QPointer (or nullified pointer) before
dereferencing.
---
Duplicate comments:
In `@qml/AIChatPanel.qml`:
- Around line 146-149: The footer Item currently hides when
AIChatManager.streamingText is empty causing the pre-token "thinking" dots to
never render; update the footer's visible and height logic to also consider the
pre-token thinking flag (e.g., AIChatManager.isThinking or equivalent) so the
footer stays visible and sized while thinking. Specifically, in the footer Item
(and the duplicate block at the other location) change visible to something like
"AIChatManager.streamingText.length > 0 || AIChatManager.isThinking" and set
height to use "streamBubble.height + 4" when either streamingText has content or
isThinking is true so the dots can be laid out before the first token arrives.
- Around line 268-287: The send button is disabled while generation is active so
the stop action can't be triggered; update the enabled expression so the control
stays clickable during generation (e.g. change the enabled binding from
AIChatManager.modelAvailable && !AIChatManager.isGenerating to a condition that
allows clicks when generating, such as AIChatManager.modelAvailable ||
AIChatManager.isGenerating), leaving the existing MouseArea onClicked handler
that calls AIChatManager.stopGeneration() when AIChatManager.isGenerating
intact.
In `@src/AIChatManager.cpp`:
- Around line 221-255: The loop executes m_mcpServer->callTool() on the GUI
thread causing freezes; move the tool call and JSON parsing off the GUI thread
(e.g., QtConcurrent::run or a worker QThread) and perform
SentryReporter::addBreadcrumb before dispatch; marshal results back to the GUI
thread via signals/slots or QMetaObject::invokeMethod(Qt::QueuedConnection) to
call appendMessage and to increment m_toolLoopDepth and call startGeneration
only after the async result arrives; ensure you preserve the same resultText
extraction logic (from result["content"]) when handling the returned QJsonObject
and that any GUI updates (appendMessage, startGeneration) run on the main
thread.
- Around line 34-38: The LLMManager signals are currently connected globally to
AIChatManager slots (connect(llm, &LLMManager::generationProgress, this,
&AIChatManager::onGenerationProgress) etc.), so progress/completion/errors from
other requests can affect this chat; change the connections to be request-scoped
by using per-request identifiers and guarded callbacks: capture the chat's
requestId (e.g., AIChatManager::currentRequestId or a newly stored requestId) in
a lambda or wrapper when calling connect to check the incoming signal's
requestId before invoking
AIChatManager::onGenerationProgress/onGenerationCompleted/onGenerationError/onGenerationStopped,
and store the QMetaObject::Connection handles so you explicitly disconnect those
connections when the request completes or stops to avoid cross-request signal
delivery.
- Around line 198-214: The fallback path currently forces toolBlocks empty when
m_mcpServer is null and then appends assistantText verbatim, which can expose
raw tool JSON; change AIChatManager::executeToolCallsAndContinue so toolBlocks
is always computed from assistantText (remove the m_mcpServer ? ... : ...
ternary and call extractToolJsonBlocks unconditionally) and use
visibleText.trimmed() (not assistantText) when calling
appendMessage("assistant", ...) in the plain-response / loop-cap branch so any
extracted JSON blocks are stripped before appending.
🪄 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: 708d0286-b7b8-4304-85dd-4fea749ae68d
📒 Files selected for processing (4)
qml/AIChatPanel.qmlsrc/AIChatManager.cppsrc/AIChatManager.hsrc/LLMWorker.cpp
The model was mimicking the tool-result display format it saw in history:
[Tool: create_primitive]
Arguments: {"name": "box", "type": "cube"}
This matched neither the primary JSON scanner (no "arguments" key) nor
the old tag regex, so no tool was ever called.
Two fixes:
1. buildConversationPrompt() now prefixes tool results with "RESULT: "
instead of showing the bare [Tool: name] block. This prevents the
model from learning to use the result format as a call format.
2. extractToolJsonBlocks() now has a fallback QRegularExpression that
matches "[Tool: X]\nArguments: {Y}" and reconstructs the canonical
{"name": "X", "arguments": Y} JSON, so any past-tense responses
already in flight are still parsed and executed correctly.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
With kMaxToolLoops=1 and a summary-only follow-up, the model had no chance to recover from tool errors (e.g. "No name provided"). Now: - kMaxToolLoops raised to 4 - Intermediate loops (depth < max) use the full system prompt so the model can call more tools — e.g. get_scene_info to find the right name, then retry the original action - Only the final loop forces a plain summary (no tools), capping runaway chains - History window increased to 12 for agentic loops so the model can see prior errors in context; shrinks to 6 only for the final summary - System prompt adds rule 3: "If a tool returns an error, call get_scene_info or list_* to find correct names, then retry" Example: "move the box left" with nothing selected → loop 1: transform_mesh fails "no name/selection" loop 2: get_scene_info returns nodes ["wood_box", ...] loop 3: transform_mesh name="wood_box" translate_x=-1 → success loop 4: summary "Moved wood_box left by 1 unit." Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/AIChatManager.cpp (4)
223-234:⚠️ Potential issue | 🟠 MajorFallback response path can still leak raw
<tool_call>/JSON protocol text.At Line 223, extraction is skipped when
m_mcpServeris null, and at Line 233 rawassistantTextis appended in the fallback branch. This can surface internal tool-call markup to users instead of clean narration.Suggested fallback cleanup
- QStringList toolBlocks = m_mcpServer ? extractToolJsonBlocks(assistantText) : QStringList{}; + QStringList toolBlocks = extractToolJsonBlocks(assistantText); @@ - if (toolBlocks.isEmpty() || m_toolLoopDepth >= kMaxToolLoops) { - // Plain response — add to history and finish - appendMessage("assistant", assistantText.trimmed()); + if (toolBlocks.isEmpty() || !m_mcpServer || m_toolLoopDepth >= kMaxToolLoops) { + const QString cleaned = visibleText.trimmed(); + appendMessage("assistant", + cleaned.isEmpty() ? QStringLiteral("I couldn't execute that editor action.") : cleaned); m_isGenerating = false; m_toolLoopDepth = 0; emit isGeneratingChanged(); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AIChatManager.cpp` around lines 223 - 234, The fallback path can append raw assistantText (including <tool_call>/JSON) when m_mcpServer is null or tool loop limits are hit; change the fallback so appendMessage("assistant", ...) uses the sanitized visibleText (the result of extractToolJsonBlocks/visibleText.simplified()) instead of raw assistantText, and ensure visibleText is computed regardless of m_mcpServer (call extractToolJsonBlocks when needed or treat toolBlocks as empty and still strip known tool markers from assistantText) so internal protocol markup never reaches users (adjust logic around m_toolLoopDepth/kMaxToolLoops and the appendMessage call).
180-186:⚠️ Potential issue | 🟠 MajorTool-call JSON scanning still breaks on braces inside quoted strings.
Line 181-Line 184 count
{/}without string/escape awareness, so valid payloads like{"arguments":{"text":"}"}}can terminate early and be dropped.Suggested parser hardening
- int depth = 0, j = i; - while (j < len) { - if (text[j] == QLatin1Char('{')) ++depth; - else if (text[j] == QLatin1Char('}')) { if (--depth == 0) break; } - ++j; - } + int depth = 0, j = i; + bool inString = false; + bool escaped = false; + while (j < len) { + const QChar ch = text[j]; + if (escaped) { + escaped = false; + } else if (inString && ch == QLatin1Char('\\')) { + escaped = true; + } else if (ch == QLatin1Char('"')) { + inString = !inString; + } else if (!inString && ch == QLatin1Char('{')) { + ++depth; + } else if (!inString && ch == QLatin1Char('}')) { + if (--depth == 0) break; + } + ++j; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AIChatManager.cpp` around lines 180 - 186, The brace-matching loop in AIChatManager.cpp (variables text, j, len, i, depth) incorrectly counts braces inside JSON strings; update the while loop to track string/escape state so braces are only counted when not inside a quoted string: add a bool inString and bool escape (toggle inString on unescaped double-quote characters, set/clear escape when seeing backslashes) and only increment/decrement depth for '{' and '}' when inString is false; ensure the loop handles escaped quotes properly so payloads like {"arguments":{"text":"}"}} are not terminated early.
35-38:⚠️ Potential issue | 🟠 MajorScope LLM callbacks to the active chat generation.
Line 35-Line 38 connect to global
LLMManagergeneration signals without request scoping. A non-chat generation can still mutatem_streamingText, append chat messages, or trigger tool execution in this manager.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AIChatManager.cpp` around lines 35 - 38, The current connects to LLMManager signals (generationProgress, generationCompleted, generationError, generationStopped) are global and let any generation mutate AIChatManager state (m_streamingText, appended messages, tool execution); change these to scoped connections that filter events to the active chat generation: use the functor/lambda overloads of connect to capture this and m_activeGenerationId (or the per-request id passed by LLMManager) and only invoke the existing handlers (or inline the handler logic) when the incoming generation's requestId matches the active id, or alternatively update the slots (onGenerationProgress/onGenerationCompleted/onGenerationError/onGenerationStopped) to accept a requestId and early-return if it doesn't match; ensure any mutation of m_streamingText or chat message append only happens for the matched request id.
257-257:⚠️ Potential issue | 🟠 MajorAvoid synchronous MCP tool execution on the UI thread.
Line 257 calls
m_mcpServer->callTool(...)inline during completion handling. Heavy tools will block the main thread, freezing the editor and chat panel until completion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AIChatManager.cpp` at line 257, The synchronous call m_mcpServer->callTool(toolName, toolArgs) is being executed on the UI thread; move the call off the main thread (e.g., use QtConcurrent::run, QThread or a worker object) so heavy tool execution doesn’t block the UI, then deliver the resulting QJsonObject back to the UI thread via a queued signal/slot or QMetaObject::invokeMethod with Qt::QueuedConnection and process the result there; update the code that currently reads m_mcpServer->callTool(...) to instead kick off the asynchronous task and handle the returned result in the completion slot/callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/AIChatManager.cpp`:
- Around line 223-234: The fallback path can append raw assistantText (including
<tool_call>/JSON) when m_mcpServer is null or tool loop limits are hit; change
the fallback so appendMessage("assistant", ...) uses the sanitized visibleText
(the result of extractToolJsonBlocks/visibleText.simplified()) instead of raw
assistantText, and ensure visibleText is computed regardless of m_mcpServer
(call extractToolJsonBlocks when needed or treat toolBlocks as empty and still
strip known tool markers from assistantText) so internal protocol markup never
reaches users (adjust logic around m_toolLoopDepth/kMaxToolLoops and the
appendMessage call).
- Around line 180-186: The brace-matching loop in AIChatManager.cpp (variables
text, j, len, i, depth) incorrectly counts braces inside JSON strings; update
the while loop to track string/escape state so braces are only counted when not
inside a quoted string: add a bool inString and bool escape (toggle inString on
unescaped double-quote characters, set/clear escape when seeing backslashes) and
only increment/decrement depth for '{' and '}' when inString is false; ensure
the loop handles escaped quotes properly so payloads like
{"arguments":{"text":"}"}} are not terminated early.
- Around line 35-38: The current connects to LLMManager signals
(generationProgress, generationCompleted, generationError, generationStopped)
are global and let any generation mutate AIChatManager state (m_streamingText,
appended messages, tool execution); change these to scoped connections that
filter events to the active chat generation: use the functor/lambda overloads of
connect to capture this and m_activeGenerationId (or the per-request id passed
by LLMManager) and only invoke the existing handlers (or inline the handler
logic) when the incoming generation's requestId matches the active id, or
alternatively update the slots
(onGenerationProgress/onGenerationCompleted/onGenerationError/onGenerationStopped)
to accept a requestId and early-return if it doesn't match; ensure any mutation
of m_streamingText or chat message append only happens for the matched request
id.
- Line 257: The synchronous call m_mcpServer->callTool(toolName, toolArgs) is
being executed on the UI thread; move the call off the main thread (e.g., use
QtConcurrent::run, QThread or a worker object) so heavy tool execution doesn’t
block the UI, then deliver the resulting QJsonObject back to the UI thread via a
queued signal/slot or QMetaObject::invokeMethod with Qt::QueuedConnection and
process the result there; update the code that currently reads
m_mcpServer->callTool(...) to instead kick off the asynchronous task and handle
the returned result in the completion slot/callback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76eae06d-79a0-4ed6-ab91-772fd22ec7a2
📒 Files selected for processing (2)
src/AIChatManager.cppsrc/AIChatManager.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/AIChatManager.h
- Rewrite AIChatManager system prompt: simpler one-response-at-a-time format with no multi-round examples (they confused 3B model into outputting all rounds at once); inject current scene state for RAG-style context - Add cleanGeneratedText truncations: at <after result> and EXAMPLE( patterns to stop model echoing future rounds from examples - Truncate stored assistant history at end of first JSON block so history stays clean and model doesn't see its own hallucinated future steps - Fix apply_material example param names: mesh/material not mesh_name/material_name - After successful tool call, continue with full buildSystemPrompt() so model can plan next steps (was using short summaryPrompt which had no tool list) - Add KV cache prefix reuse in LLMWorker: skip re-prefilling shared token prefix between consecutive calls (llama_memory_seq_rm); speeds up 2nd+ calls - Add AI Chat toolbar button (✨) next to primitives for quick panel access - Rewrite AIChatPanel.qml: single selectable TextEdit with buildHtml() instead of per-bubble ListView; right-align user messages; thinking dots always visible - Add loop detection via canonical tool call signatures; de-duplicate history on loop; enforce 1 tool per response (trim multi-tool responses) - Raise kMaxToolLoops to 10 for complex multi-step tasks - Add create_material tool to MCPServer for AI-driven material creation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- create_material: accept flat diffuse/ambient/specular arrays at top level (in addition to nested 'colors' object) — model naturally uses flat format - create_material: update schema to show flat [R,G,B] params, consistent with modify_material so the model uses the same format for both - apply_material: accept mesh_name/material_name/entity as fallback aliases alongside the canonical mesh/material params — prevents "Material name required" error when model uses slightly different parameter names - AIChatManager system prompt: put static content (tool list) before dynamic scene state so llama.cpp KV cache prefix reuse works across calls - Filter list_materials output more aggressively (DefaultSettings, GUI_, NormalVisualizer, etc.) to keep scene section compact - Add <after result>/EXAMPLE( truncation to cleanGeneratedText to stop model from echoing example conversation templates - Truncate stored assistant history at end of first JSON block so history stays clean for subsequent rounds Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
With a 4096-token context and ~3000-token system prompt, only ~1000 tokens remain for conversation. The create_material result includes a full Ogre material script (~100 tokens). Keep only the first line of each tool result in history when the full result exceeds 120 chars — the summary line is sufficient for the model to understand what happened. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When the conversation grows past maxHistory=8 messages and the window slides, the original user request gets dropped. The model then loses context about what it was asked to do. Fix: always prepend the first user message with a '...' separator when start > 0 so the goal is always visible. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Without a cap, the 3B model uses all available output tokens (~1000) to dump 20+ JSON tool calls in one response. 400 tokens is enough for one Thought line + one JSON call + a Done confirmation, which is exactly what we want. Also clarify system prompt: 'box'/'cube' = ONE create_primitive(type=cube). Do not compose a box from sphere + cube parts. The 3-step wooden box sequence is: create_primitive(cube) → create_material(brown) → apply_material. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Focus: - Set Qt::StrongFocus on the QQuickWidget so a single click inside the dock gives keyboard focus without needing to click the viewport first - Add onActiveFocusChanged on root Rectangle to forward focus to inputField whenever the panel gains active focus (returning from another app window) - Add focus: true on inputField as the default focus target in the panel - Replace onSelectedTextChanged focus snap with onActiveFocusChanged so focus only returns to inputField when msgEdit loses focus, not during text selection Copy/paste: - Intercept Ctrl/Cmd+C in msgEdit to copy selectedText (plain text) instead of the default RichText HTML that Qt copies from a TextEdit in Rich mode Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix transform_mesh schema: expose 'name' param (was missing, causing "no name provided" errors when the model didn't know to include it) - Add coordinate axis guidance to system prompt so move/front/left commands map correctly (X=right/left, Y=up/down, Z=back/front) - Add recent files to RAG context so AI can suggest loading known files - Strip trailing '|' stop-token artifacts emitted by Qwen models - Token cap 400→300 to prevent multi-call dumps while keeping headroom - Better system prompt rules: simple primitive names, no repeat after success, transform errors must not trigger create_primitive fallback - Include 'Done:' message correctly scoped to current turn only - Inject recent files (up to 10) from QSettings into scene section Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the free-text "Thought + JSON" format with a mandatory structured
JSON envelope for all model responses:
{"thought": "...", "command": "tool_name", "arguments": {...},
"remaining": ["next_step"], "response": "..."}
This makes parsing deterministic — malformed responses are detected
immediately and retried (up to 2 attempts). The "remaining" array forces
the model to explicitly track its multi-step plan.
Key changes:
- executeToolCallsAndContinue: rewritten to parse structured JSON with
4 fallback extraction methods (direct, prepend {, extract block, both)
- buildSystemPrompt: new format spec with JSON schema and field rules;
wooden-box example shown as 3 concrete JSON objects
- buildConversationPrompt: primer changed to "Assistant: {" to strongly
guide JSON output
- AIChatPanel.qml: render "command" key as [calling X], "response" key
as the final visible message, legacy "name" key still supported
- Token cap raised to 500 (structured envelope is ~100 tokens larger)
- m_jsonRetryCount tracks malformed-response retries across rounds
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove concrete wooden-box JSON example from system prompt — the 3B model was replaying it as a template after every tool call, creating unwanted boxes and duplicate Wood materials - Enforce remaining=[] server-side: when the model declares no remaining steps, force the conversation to end immediately instead of asking for another round (prevents runaway tool loops) - Strengthen Y-axis wording: "'on top of' or 'above' = increase Y. NEVER use Z for up/down" - Add explicit rule: ONLY create what the user asked for, ONE primitive per request, never create extras Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
New filesystem tools (read-only): - list_files: browse directories with optional glob filter, returns file names/sizes/types, caps at 200 entries. Defaults to home dir. - read_file: read text files up to 500 lines / 1 MB. Rejects known binary formats (images, meshes, archives, etc.) by extension. Both are read-only — no write/delete capability. Safe for the local AI assistant to find mesh files, textures, and configs on disk. Also fixes Z-axis direction in the AI system prompt: Z=front(+)/back(-), 'in front' = +Z Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t files New tools: - search_files: recursive file search with glob pattern and depth limit. "Find *.fbx in Downloads" now works. AI chat improvements: - Filter tool list to 15 core tools (was 39) — smaller models were overwhelmed and couldn't find transform_mesh in the noise - Recent files now show full absolute paths (was filename only) so the model can directly pass them to load_mesh. "Open the latest file" works. - Replace abstract axis descriptions with concrete position examples: 'move up 3' → [0,3,0], 'on the floor' → Y=0, 'forward' → Z+ Small models follow examples better than abstract rules. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
New MCP tools: - camera_control: set position, target, zoom, frame selection on the active viewport (uses TransformOperator's active widget, not always Viewport 1) - get_camera_info: read camera position, direction, orientation - search_files: recursive file search with glob pattern and depth limit AI chat improvements: - Add task separation rule: each user message is independent — model must not carry over actions from previous messages (e.g. re-applying old materials to newly loaded meshes) - Fix Z-axis: front = -Z, back = +Z (matches Ogre right-handed coords) Also adds TransformOperator::getActiveWidget() accessor. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Focus fix: - Listen to QApplication::focusChanged and forward focus from the dock container to the QQuickWidget when they don't match. Fixes the macOS issue where clicking the chat input after Alt-Tab requires two clicks. Model list: - Remove 4 near-duplicates (Gemma 2 2B, Qwen Coder 3B/7B, Phi-3.5 Mini) - Add 3 larger models for high-end hardware: Qwen 2.5 14B (~9.4GB), Gemma 3 27B (~17GB), Qwen 2.5 32B (~20GB) - Clean list: 9 models from 0.9GB to 20GB with no family overlap Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nd tests Release prep for AI Chat panel feature: - Bump version 2.20.2 → 2.21.0 - Remove all chatLog debug prints from AIChatManager.cpp (9 fprintf calls) - Add delete_entity MCP tool: proper node destruction matching the UI's Delete key behavior (deselect → destroySceneNode), not just hide/scale - Update docs/index.html: tool count 29 → 40, add AI Chat feature card - Add 18 new test cases for filesystem tools (list_files, search_files, read_file), delete_entity, and camera tools (error paths) - Update AllToolNamesAreRecognized: 27 → 40 tools Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Use QPointer<MCPServer> instead of raw pointer to prevent dangling reference if MCPServer is destroyed while generation is in-flight - Cap inputRow height to match the 80px TextArea limit (prevents unbounded footer growth on long prompts) - Remove dead extractToolJsonBlocks() function — no longer used since the structured JSON format parses the full response as a single object Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MCPServer.cpp (1)
791-805:⚠️ Potential issue | 🔴 CriticalDon't call
Manager::getEntities()fromapply_material.
Manager::getEntities()insrc/Manager.cppstillstatic_casts every attached object toOgre::Entity*. In scenes withManualObjects or other non-entity attachments, this path can crash before the safer fallback ever runs. Reuse the samegetMovableType() == "Entity"iteration thattoolGetSceneInfo()already uses nearby.🔧 Safer lookup sketch
- QList<Ogre::Entity*>& entities = mgr->getEntities(); - for (Ogre::Entity* entity : entities) { - if (entity && QString::fromStdString(entity->getName()) == meshName) { - entity->setMaterialName(materialName.toStdString()); - appliedTo << QString::fromStdString(entity->getName()); - found = true; - break; - } - } + for (Ogre::SceneNode* node : mgr->getSceneNodes()) { + if (!node) continue; + for (int i = 0; i < static_cast<int>(node->numAttachedObjects()); ++i) { + Ogre::MovableObject* obj = node->getAttachedObject(i); + if (!obj || obj->getMovableType() != "Entity") continue; + Ogre::Entity* entity = static_cast<Ogre::Entity*>(obj); + if (QString::fromStdString(entity->getName()) != meshName) continue; + entity->setMaterialName(materialName.toStdString()); + appliedTo << QString::fromStdString(entity->getName()); + found = true; + break; + } + if (found) break; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MCPServer.cpp` around lines 791 - 805, The call in apply_material that uses Manager::getEntities() is unsafe because Manager::getEntities() static_casts all attached objects to Ogre::Entity*, which can crash for non-Entity attachments; modify apply_material to avoid getEntities() and instead iterate attached objects like toolGetSceneInfo does by checking each movable's getMovableType() == "Entity" before casting, then compare the entity name to meshName and setMaterialName (use the same logic for appliedTo and found). Update any loop over entities in apply_material to use the safe getMovableType() check and dynamic cast only after confirming the type.
♻️ Duplicate comments (4)
src/AIChatManager.cpp (2)
36-43:⚠️ Potential issue | 🟠 MajorFilter these callbacks to the chat request that started them.
LLMManageris a shared singleton, so these connections also receive material-generation events and late completions from older chats. That lets unrelated work overwritem_streamingText, append messages into this panel, and even runexecuteToolCallsAndContinue()on non-chat output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AIChatManager.cpp` around lines 36 - 43, The connected LLMManager signals (generationProgress, generationCompleted, generationError, generationStopped, modelLoadedChanged, currentModelNameChanged) are global and are delivering events from other chats; update AIChatManager to ignore events not belonging to the chat that started them by filtering on the chat's request identifier or sender before handling: have the slots onGenerationProgress/onGenerationCompleted/onGenerationError/onGenerationStopped verify the incoming event’s requestId (or compare sender() to the instance that started the request) and return early for mismatched requests, or store and check a currentRequestId in AIChatManager when initiating generation so only matching events update m_streamingText, append messages, or call executeToolCallsAndContinue(); keep the same signal connections but add this request-scoped guard inside the referenced slot methods.
216-227:⚠️ Potential issue | 🟠 MajorTreat braces inside JSON strings as data, not structure.
extractFirstJsonBlock()counts every{and}as structural. Valid replies like{"arguments":{"text":"}"}}get cut early and fall into the malformed-JSON retry path.🐛 Proposed fix
auto extractFirstJsonBlock = [](const QString& text, int searchFrom = 0) -> QString { int start = text.indexOf('{', searchFrom); if (start < 0) return {}; int depth = 0; + bool inString = false; + bool escaped = false; for (int i = start; i < text.length(); ++i) { - if (text[i] == '{') ++depth; - else if (text[i] == '}') { + const QChar ch = text[i]; + if (escaped) { + escaped = false; + continue; + } + if (inString && ch == QLatin1Char('\\')) { + escaped = true; + continue; + } + if (ch == QLatin1Char('"')) { + inString = !inString; + continue; + } + if (!inString && ch == QLatin1Char('{')) ++depth; + else if (!inString && ch == QLatin1Char('}')) { if (--depth == 0) return text.mid(start, i - start + 1); } } return {}; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AIChatManager.cpp` around lines 216 - 227, extractFirstJsonBlock currently treats every '{' and '}' as structural, so braces inside JSON strings break parsing; update extractFirstJsonBlock to track string state and escape sequences: when iterating from the found start index maintain a boolean inString that flips on unescaped '"' characters and treat backslashes as escape markers (skip the next char or use an escaped flag) so that '{' and '}' are only counted when inString is false; keep the existing depth logic and return the substring when depth hits zero, and still return empty on failure.src/mainwindow.cpp (1)
1573-1576:⚠️ Potential issue | 🟠 MajorKeep chat tooling available when the HTTP listener is off.
AIChatManageronly receives anMCPServeron this path, so a normal startup withMCP/enabled = falseleaves the chat prompt tool-less and editor commands never execute. That breaks the in-app control flow this PR is adding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mainwindow.cpp` around lines 1573 - 1576, When MCP is disabled the code never gives AIChatManager an MCPServer, leaving chat tooling without a backend; ensure AIChatManager::instance()->setMcpServer(...) is always called by either moving that call out of the if-block or by creating/assigning a lightweight fallback (e.g., a Null/Local MCPServer implementation) and passing it to AIChatManager. Update the creation logic around m_mcpServer and MCPServer so that setMcpServer receives a valid object (m_mcpServer or the fallback) even when the HTTP listener is off.src/LLMManager.cpp (1)
460-469:⚠️ Potential issue | 🟠 MajorDon't route completions through one shared
m_rawTextModeflag.
generateMaterial()andgenerateText()both enqueue work onto the same worker, but this path still flips one manager-global bit to decide how the next completion is interpreted. If a chat request is queued behind an older material request, the older job can complete withm_rawTextMode == trueand skip material cleanup/validation entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/LLMManager.cpp` around lines 460 - 469, generateText and generateMaterial are racing on the manager-global m_rawTextMode flag which causes queued jobs to observe the wrong mode; stop using m_rawTextMode and instead pass an explicit mode parameter with each queued task to the worker (e.g., extend the worker->generate(...) signature or create a GenerateRequest struct that includes systemPrompt, userPrompt, maxTokensOverride and a mode/enum like RawText vs Material), update LLMManager::generateText and generateMaterial to enqueue the mode-bound request via QMetaObject::invokeMethod (capturing the mode in the lambda) and update the worker implementation to branch on the passed-in mode so each completion performs its own proper cleanup/validation without relying on a shared member flag.
🧹 Nitpick comments (1)
docs/index.html (1)
590-590: Avoid duplicating hardcoded tool-count values in docs copy.
40is now repeated in multiple places and can drift from reality as tools evolve. Consider generating this number during docs build or replacing it with non-numeric wording.Also applies to: 862-862
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/index.html` at line 590, The paragraph element with class "feature-desc" contains a hardcoded tool count ("40") that is duplicated and can drift; update the docs build/template so the count is not hardcoded — either inject a generated tools_count variable into the template or replace the numeric phrase with non-numeric wording like "dozens of tools"; target the <p class="feature-desc"> string (and the duplicate at the other occurrence) to consume the template variable (e.g., tools_count) or the revised copy during the docs build.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qml/AIChatPanel.qml`:
- Around line 129-136: The thinkingRow's visible binding currently uses only
AIChatManager.isGenerating so the "no tokens yet" dots remain during streaming;
change its visible expression to hide once streaming begins (e.g., visible:
AIChatManager.isGenerating && !AIChatManager.isStreaming or use the appropriate
flag like AIChatManager.hasNoTokens) so thinkingRow (id: thinkingRow) only shows
before streaming starts.
In `@src/LLMManager.cpp`:
- Around line 401-421: The QFileDialog calls in browseForModelsDirectory() and
browseForModelFile() use nullptr as parent which can make dialogs invisible on
macOS; change them to use a valid QWidget parent (for example the main window or
the LLMManager's top-level QWidget) instead of nullptr, and, like the macOS
workaround in mainwindow, if these are invoked from QML ensure you call the
dialog inside QTimer::singleShot(0, ...) with that QWidget parent so the dialog
is shown reliably; keep the rest of the logic (calling setModelsDirectory and
loadModelFromPath) unchanged.
In `@src/LLMSettingsWidget.cpp`:
- Around line 403-417: The onLoadFromFileClicked handler only disables
m_loadButton but leaves m_loadFromFileButton enabled, allowing overlapping
loads; disable m_loadFromFileButton at the start of onLoadFromFileClicked
alongside m_loadButton, and ensure both m_loadButton and m_loadFromFileButton
are re-enabled in the model load completion/failure handlers (e.g., the slot(s)
that handle load success/error such as onModelLoaded/onModelLoadFailed or the
signal from LLMManager used for load completion). Use the same button references
(m_loadButton, m_loadFromFileButton) and hook re-enabling into the existing
LLMManager load-complete/error signal handlers so buttons are reliably
re-enabled after the async load finishes.
In `@src/LLMWorker.cpp`:
- Around line 289-305: The bug is that setSettings() calls cleanupContext() and
initializeContext() which clear the KV cache but leaves m_prevTokens still
populated, causing generate() to skip decoding tokens that are no longer cached;
to fix it, after cleanupContext() (and before any new context/initialization) in
setSettings(), clear the previous-token state by calling m_prevTokens.clear() so
the invariant that m_prevTokens reflects the live cache holds, ensuring
generate() and the KV-cache prefix logic (commonLen calculation in generate())
behave correctly.
In `@src/MCPServer_test.cpp`:
- Around line 4270-4282: The test ReadFile_BinaryFileRejected currently writes
plain text "fake png" so binary detection may be skipped; update the QFile usage
(QFile f) in that test to write real binary bytes (e.g., the PNG signature 0x89
0x50 0x4E 0x47 0x0D 0x0A 0x1A 0x0A or other non-printable bytes) using a
QByteArray of those bytes and f.write(...), keeping the rest of the fixture
(QTemporaryDir, args["path"], server->callTool("read_file", args), and
assertions) unchanged so the file is genuinely binary and the
isError/getResultText checks still run.
In `@src/MCPServer.cpp`:
- Around line 2400-2407: Selection removal currently only calls
SelectionSet::getSingleton()->removeOne(node) which leaves raw Ogre::Entity* and
Ogre::SubEntity* pointers dangling; before calling
Manager::getSingleton()->destroySceneNode(node) iterate the node's attached
objects/entities/subentities (e.g., via the SceneNode API / attached objects
list), and for each attached Ogre::Entity* and each Ogre::SubEntity* call the
appropriate SelectionSet::removeOne(...) overload to remove those selections,
then remove the node itself as you already do; ensure you use
SelectionSet::getSingleton() to access removeOne for node, Entity, and SubEntity
before destroySceneNode(node).
- Around line 2237-2239: The new filesystem access defaults to the user's home
and allows arbitrary reads via args["path"] and the local HTTP tools exposed by
startHttp(), so restrict and validate paths: change the default path from
QDir::homePath() to the active project/workspace root (use your project root
getter), normalize/sanitize the requested path with QDir::cleanPath and
QFileInfo, and enforce that the resolved absolute path is a descendant of the
project root before proceeding; additionally gate these endpoints behind
explicit opt-in or authentication checks in startHttp() (e.g., require a
configured allow-files flag or an auth token) and return a permission-denied
error for disallowed paths or unauthenticated requests to prevent local-file
disclosure.
- Around line 617-623: The else branch currently hardcodes shininess to 32.0 so
a provided shininess input is ignored; modify the else branch that calls
pass->setSpecular(0.5, 0.5, 0.5, 1.0) to also call
pass->setShininess(resolveNumber("shininess", 32.0)) instead of the hardcoded
32.0 so resolveNumber("shininess", ...) is honored even when
resolveColor("specular") is empty; look for resolveColor, resolveNumber,
pass->setSpecular and pass->setShininess to locate the code to change.
- Around line 2509-2521: The tool schema for "create_material" exposes an
unsupported properties["script"] field which causes the model to emit script
payloads that toolCreateMaterial() ignores; remove the script entry from the
inputSchema (i.e., delete properties["script"] from the properties QJsonObject
before assigning inputSchema["properties"]) or alternatively update
toolCreateMaterial() to explicitly validate and reject "script" inputs, but the
preferred fix is to drop the properties["script"] line in the
buildToolDefinition call for "create_material" so the prompt does not advertise
unsupported parameters.
---
Outside diff comments:
In `@src/MCPServer.cpp`:
- Around line 791-805: The call in apply_material that uses
Manager::getEntities() is unsafe because Manager::getEntities() static_casts all
attached objects to Ogre::Entity*, which can crash for non-Entity attachments;
modify apply_material to avoid getEntities() and instead iterate attached
objects like toolGetSceneInfo does by checking each movable's getMovableType()
== "Entity" before casting, then compare the entity name to meshName and
setMaterialName (use the same logic for appliedTo and found). Update any loop
over entities in apply_material to use the safe getMovableType() check and
dynamic cast only after confirming the type.
---
Duplicate comments:
In `@src/AIChatManager.cpp`:
- Around line 36-43: The connected LLMManager signals (generationProgress,
generationCompleted, generationError, generationStopped, modelLoadedChanged,
currentModelNameChanged) are global and are delivering events from other chats;
update AIChatManager to ignore events not belonging to the chat that started
them by filtering on the chat's request identifier or sender before handling:
have the slots
onGenerationProgress/onGenerationCompleted/onGenerationError/onGenerationStopped
verify the incoming event’s requestId (or compare sender() to the instance that
started the request) and return early for mismatched requests, or store and
check a currentRequestId in AIChatManager when initiating generation so only
matching events update m_streamingText, append messages, or call
executeToolCallsAndContinue(); keep the same signal connections but add this
request-scoped guard inside the referenced slot methods.
- Around line 216-227: extractFirstJsonBlock currently treats every '{' and '}'
as structural, so braces inside JSON strings break parsing; update
extractFirstJsonBlock to track string state and escape sequences: when iterating
from the found start index maintain a boolean inString that flips on unescaped
'"' characters and treat backslashes as escape markers (skip the next char or
use an escaped flag) so that '{' and '}' are only counted when inString is
false; keep the existing depth logic and return the substring when depth hits
zero, and still return empty on failure.
In `@src/LLMManager.cpp`:
- Around line 460-469: generateText and generateMaterial are racing on the
manager-global m_rawTextMode flag which causes queued jobs to observe the wrong
mode; stop using m_rawTextMode and instead pass an explicit mode parameter with
each queued task to the worker (e.g., extend the worker->generate(...) signature
or create a GenerateRequest struct that includes systemPrompt, userPrompt,
maxTokensOverride and a mode/enum like RawText vs Material), update
LLMManager::generateText and generateMaterial to enqueue the mode-bound request
via QMetaObject::invokeMethod (capturing the mode in the lambda) and update the
worker implementation to branch on the passed-in mode so each completion
performs its own proper cleanup/validation without relying on a shared member
flag.
In `@src/mainwindow.cpp`:
- Around line 1573-1576: When MCP is disabled the code never gives AIChatManager
an MCPServer, leaving chat tooling without a backend; ensure
AIChatManager::instance()->setMcpServer(...) is always called by either moving
that call out of the if-block or by creating/assigning a lightweight fallback
(e.g., a Null/Local MCPServer implementation) and passing it to AIChatManager.
Update the creation logic around m_mcpServer and MCPServer so that setMcpServer
receives a valid object (m_mcpServer or the fallback) even when the HTTP
listener is off.
---
Nitpick comments:
In `@docs/index.html`:
- Line 590: The paragraph element with class "feature-desc" contains a hardcoded
tool count ("40") that is duplicated and can drift; update the docs
build/template so the count is not hardcoded — either inject a generated
tools_count variable into the template or replace the numeric phrase with
non-numeric wording like "dozens of tools"; target the <p class="feature-desc">
string (and the duplicate at the other occurrence) to consume the template
variable (e.g., tools_count) or the revised copy during the docs build.
🪄 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: 5c924ecb-9eab-421e-8ca5-c359f2a86e6f
📒 Files selected for processing (17)
CMakeLists.txtdocs/index.htmlqml/AIChatPanel.qmlqml/AISettingsDialog.qmlsrc/AIChatManager.cppsrc/AIChatManager.hsrc/LLMManager.cppsrc/LLMManager.hsrc/LLMSettingsWidget.cppsrc/LLMSettingsWidget.hsrc/LLMWorker.cppsrc/LLMWorker.hsrc/MCPServer.cppsrc/MCPServer.hsrc/MCPServer_test.cppsrc/TransformOperator.hsrc/mainwindow.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MCPServer.h
Comment on lines
+129
to
+136
| // ---- Thinking dots (no tokens yet) ---- | ||
| Row { | ||
| id: thinkingRow | ||
| anchors { bottom: inputRow.top; left: parent.left; leftMargin: 12; bottomMargin: 6 } | ||
| height: visible ? 14 : 0 | ||
| spacing: 4 | ||
| visible: AIChatManager.isGenerating | ||
|
|
There was a problem hiding this comment.
Hide the thinking dots once streaming starts.
This row is labeled "no tokens yet", but visible only checks isGenerating, so the dots stay on-screen during the whole streamed reply.
🐛 Proposed fix
- visible: AIChatManager.isGenerating
+ visible: AIChatManager.isGenerating && AIChatManager.streamingText.length === 0📝 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
| // ---- Thinking dots (no tokens yet) ---- | |
| Row { | |
| id: thinkingRow | |
| anchors { bottom: inputRow.top; left: parent.left; leftMargin: 12; bottomMargin: 6 } | |
| height: visible ? 14 : 0 | |
| spacing: 4 | |
| visible: AIChatManager.isGenerating | |
| // ---- Thinking dots (no tokens yet) ---- | |
| Row { | |
| id: thinkingRow | |
| anchors { bottom: inputRow.top; left: parent.left; leftMargin: 12; bottomMargin: 6 } | |
| height: visible ? 14 : 0 | |
| spacing: 4 | |
| visible: AIChatManager.isGenerating && AIChatManager.streamingText.length === 0 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/AIChatPanel.qml` around lines 129 - 136, The thinkingRow's visible
binding currently uses only AIChatManager.isGenerating so the "no tokens yet"
dots remain during streaming; change its visible expression to hide once
streaming begins (e.g., visible: AIChatManager.isGenerating &&
!AIChatManager.isStreaming or use the appropriate flag like
AIChatManager.hasNoTokens) so thinkingRow (id: thinkingRow) only shows before
streaming starts.
Comment on lines
+401
to
+421
| void LLMManager::browseForModelsDirectory() | ||
| { | ||
| QString dir = QFileDialog::getExistingDirectory( | ||
| nullptr, | ||
| "Select Models Directory", | ||
| m_modelsDirectory | ||
| ); | ||
| if (!dir.isEmpty()) | ||
| setModelsDirectory(dir); | ||
| } | ||
|
|
||
| void LLMManager::browseForModelFile() | ||
| { | ||
| QString file = QFileDialog::getOpenFileName( | ||
| nullptr, | ||
| "Load AI Model", | ||
| m_modelsDirectory, | ||
| "GGUF models (*.gguf);;Binary models (*.bin);;All files (*)" | ||
| ); | ||
| if (!file.isEmpty()) | ||
| loadModelFromPath(file); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- New dialog helpers in LLMManager ---\n'
sed -n '401,421p' src/LLMManager.cpp
printf '\n--- Existing macOS QFileDialog workaround in MainWindow ---\n'
sed -n '316,329p' src/mainwindow.cppRepository: fernandotonon/QtMeshEditor
Length of output: 165
🏁 Script executed:
sed -n '401,421p' src/LLMManager.cpp && echo "---" && sed -n '316,329p' src/mainwindow.cppRepository: fernandotonon/QtMeshEditor
Length of output: 1568
Pass a valid widget parent to these dialogs.
These helpers invoke QFileDialog with nullptr, but the codebase already documents that QFileDialog opened from QML context requires a proper parent widget on macOS to appear reliably. See the macOS workaround in src/mainwindow.cpp (lines 316–329), which passes this as the parent and defers the dialog via QTimer::singleShot(0, ...) to avoid invisible dialogs on macOS.
Update browseForModelsDirectory() and browseForModelFile() to supply a valid parent widget instead of nullptr.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/LLMManager.cpp` around lines 401 - 421, The QFileDialog calls in
browseForModelsDirectory() and browseForModelFile() use nullptr as parent which
can make dialogs invisible on macOS; change them to use a valid QWidget parent
(for example the main window or the LLMManager's top-level QWidget) instead of
nullptr, and, like the macOS workaround in mainwindow, if these are invoked from
QML ensure you call the dialog inside QTimer::singleShot(0, ...) with that
QWidget parent so the dialog is shown reliably; keep the rest of the logic
(calling setModelsDirectory and loadModelFromPath) unchanged.
Comment on lines
+403
to
+417
| void LLMSettingsWidget::onLoadFromFileClicked() | ||
| { | ||
| QString file = QFileDialog::getOpenFileName( | ||
| nullptr, | ||
| "Load AI Model", | ||
| LLMManager::instance()->modelsDirectory(), | ||
| "GGUF models (*.gguf);;Binary models (*.bin);;All files (*)" | ||
| ); | ||
| if (file.isEmpty()) | ||
| return; | ||
| m_loadButton->setEnabled(false); | ||
| m_statusLabel->setText("Loading model..."); | ||
| m_statusLabel->setStyleSheet("color: orange;"); | ||
| LLMManager::instance()->loadModelFromPath(file); | ||
| } |
There was a problem hiding this comment.
Disable the new “Load from file...” trigger during active load.
Right now only m_loadButton is disabled. m_loadFromFileButton remains clickable, so users can enqueue overlapping model loads.
Suggested fix
void LLMSettingsWidget::onLoadFromFileClicked()
{
@@
- m_loadButton->setEnabled(false);
+ m_loadButton->setEnabled(false);
+ m_loadFromFileButton->setEnabled(false);
@@
}
void LLMSettingsWidget::onModelLoadCompleted(const QString &modelName)
{
@@
m_loadButton->setEnabled(true);
+ m_loadFromFileButton->setEnabled(true);
@@
}
void LLMSettingsWidget::onModelLoadError(const QString &error)
{
@@
m_loadButton->setEnabled(true);
+ m_loadFromFileButton->setEnabled(true);
@@
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/LLMSettingsWidget.cpp` around lines 403 - 417, The onLoadFromFileClicked
handler only disables m_loadButton but leaves m_loadFromFileButton enabled,
allowing overlapping loads; disable m_loadFromFileButton at the start of
onLoadFromFileClicked alongside m_loadButton, and ensure both m_loadButton and
m_loadFromFileButton are re-enabled in the model load completion/failure
handlers (e.g., the slot(s) that handle load success/error such as
onModelLoaded/onModelLoadFailed or the signal from LLMManager used for load
completion). Use the same button references (m_loadButton, m_loadFromFileButton)
and hook re-enabling into the existing LLMManager load-complete/error signal
handlers so buttons are reliably re-enabled after the async load finishes.
Comment on lines
+289
to
+305
| // KV-cache prefix reuse: if the new prompt shares a prefix with the previous | ||
| // call's tokens, only decode the NEW suffix — the shared prefix is already in cache. | ||
| size_t commonLen = 0; | ||
| for (size_t i = 0; i < std::min(tokens.size(), m_prevTokens.size()); ++i) { | ||
| if (tokens[i] == m_prevTokens[i]) ++commonLen; | ||
| else break; | ||
| } | ||
| // Never reuse a prefix that covers the full previous prompt — we need at least | ||
| // the new suffix tokens to give the model something new to respond to. | ||
| if (commonLen >= tokens.size()) commonLen = 0; | ||
|
|
||
| llama_memory_t mem = llama_get_memory(m_ctx); | ||
| if (mem) { | ||
| llama_memory_clear(mem, false); | ||
| if (commonLen == 0) { | ||
| llama_memory_clear(mem, false); // full reset | ||
| } else { | ||
| llama_memory_seq_rm(mem, 0, (llama_pos)commonLen, -1); // trim suffix |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- setSettings() context rebuild ---\n'
sed -n '216,228p' src/LLMWorker.cpp
printf '\n--- KV prefix reuse path ---\n'
sed -n '289,334p' src/LLMWorker.cpp
printf '\n--- All cached-prefix invalidation sites ---\n'
rg -n "m_prevTokens\\.clear|cleanupContext\\(|initializeContext\\(" src/LLMWorker.cppRepository: fernandotonon/QtMeshEditor
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== setSettings() context rebuild (lines 216-228) ==="
sed -n '216,228p' src/LLMWorker.cpp
echo ""
echo "=== KV prefix reuse path (lines 289-334) ==="
sed -n '289,334p' src/LLMWorker.cpp
echo ""
echo "=== All m_prevTokens.clear() callsites ==="
rg -n "m_prevTokens\.clear" src/LLMWorker.cpp
echo ""
echo "=== All cleanupContext/initializeContext callsites ==="
rg -n "cleanupContext\(\)|initializeContext\(\)" src/LLMWorker.cppRepository: fernandotonon/QtMeshEditor
Length of output: 2647
🏁 Script executed:
sed -n '185,205p' src/LLMWorker.cppRepository: fernandotonon/QtMeshEditor
Length of output: 644
🏁 Script executed:
sed -n '235,340p' src/LLMWorker.cpp | head -50Repository: fernandotonon/QtMeshEditor
Length of output: 1809
🏁 Script executed:
sed -n '280,345p' src/LLMWorker.cppRepository: fernandotonon/QtMeshEditor
Length of output: 2798
Clear m_prevTokens when context is rebuilt in setSettings().
When setSettings() calls cleanupContext() and initializeContext(), it empties the KV cache but leaves m_prevTokens populated. The next generate() call then skips decoding tokens that it incorrectly assumes are cached, but they're not—resulting in incorrect output.
Add m_prevTokens.clear(); in setSettings() after cleanupContext() (line 224) to maintain the invariant that m_prevTokens matches the live cache state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/LLMWorker.cpp` around lines 289 - 305, The bug is that setSettings()
calls cleanupContext() and initializeContext() which clear the KV cache but
leaves m_prevTokens still populated, causing generate() to skip decoding tokens
that are no longer cached; to fix it, after cleanupContext() (and before any new
context/initialization) in setSettings(), clear the previous-token state by
calling m_prevTokens.clear() so the invariant that m_prevTokens reflects the
live cache holds, ensuring generate() and the KV-cache prefix logic (commonLen
calculation in generate()) behave correctly.
Comment on lines
+4270
to
+4282
| TEST_F(MCPServerTest, ReadFile_BinaryFileRejected) | ||
| { | ||
| QTemporaryDir tmpDir; | ||
| ASSERT_TRUE(tmpDir.isValid()); | ||
| QFile f(tmpDir.filePath("image.png")); | ||
| f.open(QIODevice::WriteOnly); f.write("fake png"); f.close(); | ||
|
|
||
| QJsonObject args; | ||
| args["path"] = tmpDir.filePath("image.png"); | ||
| QJsonObject result = server->callTool("read_file", args); | ||
| EXPECT_TRUE(isError(result)); | ||
| EXPECT_TRUE(getResultText(result).contains("binary")); | ||
| } |
There was a problem hiding this comment.
ReadFile_BinaryFileRejected should write actual binary bytes.
The fixture currently writes plain text ("fake png"), which can miss regressions in binary-content detection.
Suggested test fixture tweak
TEST_F(MCPServerTest, ReadFile_BinaryFileRejected)
{
@@
- QFile f(tmpDir.filePath("image.png"));
- f.open(QIODevice::WriteOnly); f.write("fake png"); f.close();
+ QFile f(tmpDir.filePath("image.png"));
+ ASSERT_TRUE(f.open(QIODevice::WriteOnly));
+ QByteArray bytes;
+ bytes.append(char(0x89));
+ bytes.append("PNG\r\n\x1A\n", 7);
+ bytes.append(char(0x00));
+ ASSERT_GT(f.write(bytes), 0);
+ f.close();
@@
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MCPServer_test.cpp` around lines 4270 - 4282, The test
ReadFile_BinaryFileRejected currently writes plain text "fake png" so binary
detection may be skipped; update the QFile usage (QFile f) in that test to write
real binary bytes (e.g., the PNG signature 0x89 0x50 0x4E 0x47 0x0D 0x0A 0x1A
0x0A or other non-printable bytes) using a QByteArray of those bytes and
f.write(...), keeping the rest of the fixture (QTemporaryDir, args["path"],
server->callTool("read_file", args), and assertions) unchanged so the file is
genuinely binary and the isError/getResultText checks still run.
Comment on lines
+2237
to
+2239
| QString path = args["path"].toString(); | ||
| if (path.isEmpty()) | ||
| path = QDir::homePath(); |
There was a problem hiding this comment.
Scope these filesystem tools before exposing them through MCP/HTTP.
These additions default to home-directory enumeration and allow arbitrary file reads. In the same class, startHttp() serves tool calls over an unauthenticated listener bound to QHostAddress::Any, so enabling HTTP now turns this into local-file disclosure for any reachable client. Please restrict these tools to the active project/workspace or gate them behind explicit auth/opt-in before release.
Also applies to: 2284-2287, 2344-2373
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MCPServer.cpp` around lines 2237 - 2239, The new filesystem access
defaults to the user's home and allows arbitrary reads via args["path"] and the
local HTTP tools exposed by startHttp(), so restrict and validate paths: change
the default path from QDir::homePath() to the active project/workspace root (use
your project root getter), normalize/sanitize the requested path with
QDir::cleanPath and QFileInfo, and enforce that the resolved absolute path is a
descendant of the project root before proceeding; additionally gate these
endpoints behind explicit opt-in or authentication checks in startHttp() (e.g.,
require a configured allow-files flag or an auth token) and return a
permission-denied error for disallowed paths or unauthenticated requests to
prevent local-file disclosure.
Comment on lines
+2509
to
+2521
| properties["name"] = QJsonObject{{"type", "string"}, {"description", "Name of the new material"}}; | ||
| properties["script"] = QJsonObject{{"type", "string"}, {"description", "Optional: full Ogre3D material script (overrides color params)"}}; | ||
| properties["ambient"] = QJsonObject{{"type", "array"}, {"description", "Ambient color [R, G, B] (0.0-1.0)"}}; | ||
| properties["diffuse"] = QJsonObject{{"type", "array"}, {"description", "Diffuse color [R, G, B] (0.0-1.0)"}}; | ||
| properties["specular"] = QJsonObject{{"type", "array"}, {"description", "Specular color [R, G, B] (0.0-1.0)"}}; | ||
| properties["shininess"] = QJsonObject{{"type", "number"}, {"description", "Specular shininess (1-128)"}}; | ||
| properties["emissive"] = QJsonObject{{"type", "array"}, {"description", "Emissive/glow color [R, G, B] (0.0-1.0)"}}; | ||
| inputSchema["properties"] = properties; | ||
| inputSchema["required"] = QJsonArray{"name"}; | ||
|
|
||
| tools.append(buildToolDefinition( | ||
| "create_material", | ||
| "Create a new Ogre3D material. Provide either a full Ogre material script via 'script', or set individual colors (ambient, diffuse, specular, emissive) via 'colors'. The material can then be applied to a mesh with apply_material.", | ||
| "Create a new Ogre3D material with optional colors. Colors are [R,G,B] arrays (0.0-1.0). Apply the result to a mesh with apply_material.", |
There was a problem hiding this comment.
Remove the unsupported script field from the tool schema.
The chat prompt is built from buildToolsList(), so this new field will cause the model to emit script payloads that toolCreateMaterial() silently ignores. Either drop the field or explicitly reject it in toolCreateMaterial() until there is real support.
🔧 Minimal fix
QJsonObject properties;
properties["name"] = QJsonObject{{"type", "string"}, {"description", "Name of the new material"}};
- properties["script"] = QJsonObject{{"type", "string"}, {"description", "Optional: full Ogre3D material script (overrides color params)"}};
properties["ambient"] = QJsonObject{{"type", "array"}, {"description", "Ambient color [R, G, B] (0.0-1.0)"}};
properties["diffuse"] = QJsonObject{{"type", "array"}, {"description", "Diffuse color [R, G, B] (0.0-1.0)"}};
properties["specular"] = QJsonObject{{"type", "array"}, {"description", "Specular color [R, G, B] (0.0-1.0)"}};📝 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
| properties["name"] = QJsonObject{{"type", "string"}, {"description", "Name of the new material"}}; | |
| properties["script"] = QJsonObject{{"type", "string"}, {"description", "Optional: full Ogre3D material script (overrides color params)"}}; | |
| properties["ambient"] = QJsonObject{{"type", "array"}, {"description", "Ambient color [R, G, B] (0.0-1.0)"}}; | |
| properties["diffuse"] = QJsonObject{{"type", "array"}, {"description", "Diffuse color [R, G, B] (0.0-1.0)"}}; | |
| properties["specular"] = QJsonObject{{"type", "array"}, {"description", "Specular color [R, G, B] (0.0-1.0)"}}; | |
| properties["shininess"] = QJsonObject{{"type", "number"}, {"description", "Specular shininess (1-128)"}}; | |
| properties["emissive"] = QJsonObject{{"type", "array"}, {"description", "Emissive/glow color [R, G, B] (0.0-1.0)"}}; | |
| inputSchema["properties"] = properties; | |
| inputSchema["required"] = QJsonArray{"name"}; | |
| tools.append(buildToolDefinition( | |
| "create_material", | |
| "Create a new Ogre3D material. Provide either a full Ogre material script via 'script', or set individual colors (ambient, diffuse, specular, emissive) via 'colors'. The material can then be applied to a mesh with apply_material.", | |
| "Create a new Ogre3D material with optional colors. Colors are [R,G,B] arrays (0.0-1.0). Apply the result to a mesh with apply_material.", | |
| properties["name"] = QJsonObject{{"type", "string"}, {"description", "Name of the new material"}}; | |
| properties["ambient"] = QJsonObject{{"type", "array"}, {"description", "Ambient color [R, G, B] (0.0-1.0)"}}; | |
| properties["diffuse"] = QJsonObject{{"type", "array"}, {"description", "Diffuse color [R, G, B] (0.0-1.0)"}}; | |
| properties["specular"] = QJsonObject{{"type", "array"}, {"description", "Specular color [R, G, B] (0.0-1.0)"}}; | |
| properties["shininess"] = QJsonObject{{"type", "number"}, {"description", "Specular shininess (1-128)"}}; | |
| properties["emissive"] = QJsonObject{{"type", "array"}, {"description", "Emissive/glow color [R, G, B] (0.0-1.0)"}}; | |
| inputSchema["properties"] = properties; | |
| inputSchema["required"] = QJsonArray{"name"}; | |
| tools.append(buildToolDefinition( | |
| "create_material", | |
| "Create a new Ogre3D material with optional colors. Colors are [R,G,B] arrays (0.0-1.0). Apply the result to a mesh with apply_material.", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MCPServer.cpp` around lines 2509 - 2521, The tool schema for
"create_material" exposes an unsupported properties["script"] field which causes
the model to emit script payloads that toolCreateMaterial() ignores; remove the
script entry from the inputSchema (i.e., delete properties["script"] from the
properties QJsonObject before assigning inputSchema["properties"]) or
alternatively update toolCreateMaterial() to explicitly validate and reject
"script" inputs, but the preferred fix is to drop the properties["script"] line
in the buildToolDefinition call for "create_material" so the prompt does not
advertise unsupported parameters.
The MaterialEditorQML test executables link mainwindow.cpp which references AIChatManager, but AIChatManager.cpp/.h was missing from TEST_SRC_FILES and TEST_HEADER_FILES, causing linker errors on CI. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AIChatManagerQML_SINGLETON that bridgesLLMManager(local LLM) andMCPServer(tool execution) for a natural-language chat interfaceAIChatPanel.qmldock panel: role-coloured message bubbles, streaming footer with animated thinking dots, Shift+Enter for newline/Enter to sendLLMManager::generateText()— generic completion method (reused by AIChatManager alongside the existing material-specificgenerateMaterial())MCPServer::buildToolsList()to public so AIChatManager can build its system prompt from live tool definitionsAgentic loop: LLM responses containing
<tool_call>{"name":...,"arguments":{...}}</tool_call>blocks are parsed, dispatched viaMCPServer::callTool(), and tool results fed back into the conversation for a follow-up response (up to 5 rounds).Closes #209
Test plan
transform_meshwith scale 2×🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation