Add "Edit Draft" AI rewrite button with dedicated prompt#40
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 47 minutes and 42 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a nullable Changes
Sequence DiagramsequenceDiagram
participant User
participant Client as Browser (JS)
participant Server as FreeScoutGPTController
participant DB as Database (GPTSettings)
participant OpenAI as OpenAI API
User->>Client: Click "Edit Draft"
Client->>Client: getReplyDraftText() (strip HTML)
Client->>Server: POST /freescoutgpt/edit-draft {mailbox_id, text, skip_client_data}
Server->>DB: Load GPTSettings (includes message_edit_prompt)
DB-->>Server: Return settings
Server->>OpenAI: Chat completion request (model, prompt = message_edit_prompt + user draft)
OpenAI-->>Server: Edited text
Server-->>Client: Respond { answer: edited text }
Client->>Client: setReplyDraftText(edited text)
Client->>User: Editor updated with edited draft
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@Http/Controllers/FreeScoutGPTController.php`:
- Around line 247-249: The $skipClientData flag is only declared inside the
Responses API branch so the standard chat flow still sends client data; move the
declaration of $skipClientData = $request->boolean('skip_client_data') to the
top of the method (or at least before the Responses API conditional) so it's
available to both flows, then use that variable in the existing checks (e.g.,
the if that currently reads $settings->client_data_enabled && !$skipClientData
and any later logic that reads $skipClientData when building
$customerName/$client data) in FreeScoutGPTController to ensure the privacy flag
is honored in both paths.
In `@Providers/FreeScoutGPTServiceProvider.php`:
- Around line 75-95: The injected prompt strings (e.g., $message_edit_prompt,
$responses_api_prompt, $start_message, $modifyPrompt, etc.) are embedded raw
into the JS object freescoutGPTData and can break the script or allow XSS;
replace the manual string concatenation with safe JSON serialization: build a
PHP array/object containing all keys (copiedToClipboard, updateAvailable,
version, start_message, message_edit_prompt, responses_api_prompt, modifyPrompt,
send, editDraft, nothingToEdit, moduleDisabled) and output it via json_encode so
values are properly escaped for JavaScript and cannot inject backticks, ${}, or
</script>.
In `@Public/js/module.js`:
- Around line 200-207: The setReplyDraftText function is injecting
server-provided text into Summernote as HTML without escaping special
characters, allowing XSS; update setReplyDraftText to HTML-escape &, <, >, " and
' (or use a DOM/text-node-based sanitizer) before converting newlines to <br>
and calling $('#body').summernote('code', ...), or use Summernote's safe text
API if available; target the setReplyDraftText function and the
$('#body').summernote('code', ...) call to ensure the value is escaped/sanitized
prior to insertion.
- Around line 234-237: The success handler currently calls
setReplyDraftText(response.answer) unconditionally; update the success function
that includes setReplyDraftText and $(".gpteditbutton i").removeClass(...) to
first validate that response and response.answer exist and are the expected type
(e.g., typeof response.answer === "string" and response.answer.length > 0)
before calling setReplyDraftText; if validation fails, avoid updating the editor
(or pass a safe default like an empty string) and optionally log or handle the
unexpected response so the UI state change (removing the fa-beat-fade class)
still occurs safely.
🧹 Nitpick comments (2)
Http/Controllers/FreeScoutGPTController.php (1)
16-22: Remove or use the unusedstripHtmlToTexthelper.
Line 16–22 is currently unused (static analysis flagged it). Either delete it or wire it intoeditDraftif HTML stripping is intended.🧹 Remove if not needed
- private function stripHtmlToText($html) - { - if (!is_string($html)) { - return ''; - } - return trim(strip_tags($html)); - }Public/js/module.js (1)
224-254: Consider adding a timeout for the AI request.AI API calls can take a significant amount of time (30+ seconds). Without a timeout, the UI could remain in a loading state indefinitely if the server or AI service becomes unresponsive. Adding a timeout with appropriate user feedback would improve reliability.
⏱️ Suggested timeout configuration
$.ajax({ url: '/freescoutgpt/edit-draft', method: 'POST', dataType: 'json', + timeout: 60000, // 60 second timeout headers: csrfToken ? { 'X-CSRF-TOKEN': csrfToken } : {}, data: { mailbox_id: mailbox_id, text: text, skip_client_data: 1 }, // ... error: function(xhr) { $(".gpteditbutton i").removeClass("fa-beat-fade"); + if (xhr && xhr.statusText === 'timeout') { + showFloatingAlert('error', 'Request timed out. Please try again.'); + return; + } // ... rest of error handling } });
|
@ku9nov There are a few issues with the code that CodeRabbit has identified, are you able to address these? I will review in more detail as well. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Public/js/module.js`:
- Around line 189-199: getReplyDraftText currently converts the editor HTML to
plain text using tmp.textContent which collapses <br> and paragraph boundaries;
update getReplyDraftText to preserve line breaks by first transforming the DOM:
replace/insert newline markers for <br> elements and for block-level elements
(e.g., p, div, li) before reading text, then read the node text (tmp.textContent
or tmp.innerText) and trim; locate the getReplyDraftText function and apply this
DOM-to-text conversion after tmp.innerHTML = html and before returning the
trimmed string so drafts keep their original line breaks.
🧹 Nitpick comments (1)
Http/Controllers/FreeScoutGPTController.php (1)
16-22: UsestripHtmlToTextwhen normalizing the draft input.
The helper is currently unused; applying it ineditDraftboth strips HTML and clears the unused-method warning.♻️ Suggested wiring
- $draftText = (string)$request->get('text', ''); - $draftText = trim($draftText); + $draftText = $this->stripHtmlToText($request->get('text', ''));Also applies to: 472-488
| function getReplyDraftText() { | ||
| if (!$('#body').length || typeof $('#body').summernote !== 'function') { | ||
| return ''; | ||
| } | ||
| const html = $('#body').summernote('code') || ''; | ||
| // Convert HTML to plain text for the editing request. | ||
| const tmp = document.createElement('div'); | ||
| tmp.innerHTML = html; | ||
| return (tmp.textContent || tmp.innerText || '').trim(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Preserve line breaks when extracting draft text.
Using textContent first can collapse <br>/paragraphs, flattening the draft before editing.
📝 Suggested tweak
- return (tmp.textContent || tmp.innerText || '').trim();
+ return (tmp.innerText || tmp.textContent || '').trim();🤖 Prompt for AI Agents
In `@Public/js/module.js` around lines 189 - 199, getReplyDraftText currently
converts the editor HTML to plain text using tmp.textContent which collapses
<br> and paragraph boundaries; update getReplyDraftText to preserve line breaks
by first transforming the DOM: replace/insert newline markers for <br> elements
and for block-level elements (e.g., p, div, li) before reading text, then read
the node text (tmp.textContent or tmp.innerText) and trim; locate the
getReplyDraftText function and apply this DOM-to-text conversion after
tmp.innerHTML = html and before returning the trimmed string so drafts keep
their original line breaks.
There was a problem hiding this comment.
Switching the order to innerText does not reliably preserve line breaks and is browser-dependent.
For deterministic behavior, explicit handling of <br> / block elements would be required.
Keeping the current implementation for now.
There was a problem hiding this comment.
@ku9nov, you're absolutely right. innerText strips <br> tags without replacement, and its behavior varies significantly across browsers, while textContent adheres to the W3C standard and is more performant since it doesn't trigger reflows.
For reliable line break preservation, you'd indeed need to explicitly process <br> and block-level elements in the DOM before extracting text. Keeping the current implementation is a sensible choice for consistency and cross-browser compatibility.
✏️ Learnings added
Learnt from: ku9nov
Repo: presswizards/FreeScoutGPT PR: 40
File: Public/js/module.js:189-199
Timestamp: 2026-01-29T10:29:43.858Z
Learning: In the FreeScoutGPT module file `Public/js/module.js`, the `getReplyDraftText()` function uses `textContent` (not `innerText`) to extract text from the Summernote editor for consistency and cross-browser compatibility, accepting that line breaks may be collapsed. Switching to `innerText` is not preferred due to its browser-dependent behavior.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Hey @presswizards |
|
@ku9nov Ok we'll dig into it more, do some testing, and ensure it's working well across the our test installations, etc. |
|
@ku9nov This branch has conflicts that must be resolved. Can you look into this more, it may be needed to rework the pull request against the latest commit. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Http/Controllers/FreeScoutGPTController.php (1)
362-362:⚠️ Potential issue | 🟡 MinorInconsistent
$skipClientDatahandling in Infomaniak API path.The Infomaniak API path doesn't pass
$skipClientDatatofetchArticlesContext(), causing it to default tofalse. This means theskip_client_datarequest flag is ignored for Infomaniak, and client data will always be included whenclient_data_enabledis true—inconsistent with the Responses API path (line 278) that correctly passes the flag.🐛 Proposed fix to pass $skipClientData
- $fetchResult = $this->fetchArticlesContext($articleUrls, $settings, $request); + $fetchResult = $this->fetchArticlesContext($articleUrls, $settings, $request, $skipClientData);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Http/Controllers/FreeScoutGPTController.php` at line 362, The Infomaniak API branch in FreeScoutGPTController calls fetchArticlesContext($articleUrls, $settings, $request) without forwarding the $skipClientData flag, causing skip_client_data to be ignored for Infomaniak; update that call to pass the existing $skipClientData variable (matching the Responses API path which calls fetchArticlesContext(..., $skipClientData) around the other branch) so fetchArticlesContext receives and respects the skip-client-data behavior.
🧹 Nitpick comments (1)
Http/Controllers/FreeScoutGPTController.php (1)
16-22: Remove unused private method.The static analysis correctly identifies that
stripHtmlToText()is never called anywhere in this controller. This appears to be dead code that should be removed.🧹 Proposed fix to remove dead code
- private function stripHtmlToText($html) - { - if (!is_string($html)) { - return ''; - } - return trim(strip_tags($html)); - } -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Http/Controllers/FreeScoutGPTController.php` around lines 16 - 22, The private method stripHtmlToText in FreeScoutGPTController is dead code and should be removed; locate the method definition (private function stripHtmlToText($html)) inside the FreeScoutGPTController class, confirm there are no callers (search for stripHtmlToText), and delete the entire method declaration and its body to clean up the controller.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Http/Controllers/FreeScoutGPTController.php`:
- Around line 623-633: The OpenAI chatCompletions API call in
FreeScoutGPTController is not wrapped in error handling; wrap the call to
$openaiClient->chatCompletions()->create(...) in a try-catch (catch \Throwable
or \Exception and optionally the Tectalic OpenAI exception type), log the
exception (use the controller's logger or Laravel's Log::error) including the
exception message and context, and return a JSON error response (e.g.,
Response::json(['error' => 'OpenAI request failed', 'details' =>
$e->getMessage()], 500)) when an exception occurs; keep the existing successful
return of the answer unchanged.
---
Outside diff comments:
In `@Http/Controllers/FreeScoutGPTController.php`:
- Line 362: The Infomaniak API branch in FreeScoutGPTController calls
fetchArticlesContext($articleUrls, $settings, $request) without forwarding the
$skipClientData flag, causing skip_client_data to be ignored for Infomaniak;
update that call to pass the existing $skipClientData variable (matching the
Responses API path which calls fetchArticlesContext(..., $skipClientData) around
the other branch) so fetchArticlesContext receives and respects the
skip-client-data behavior.
---
Nitpick comments:
In `@Http/Controllers/FreeScoutGPTController.php`:
- Around line 16-22: The private method stripHtmlToText in
FreeScoutGPTController is dead code and should be removed; locate the method
definition (private function stripHtmlToText($html)) inside the
FreeScoutGPTController class, confirm there are no callers (search for
stripHtmlToText), and delete the entire method declaration and its body to clean
up the controller.
🪄 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: b4cc2d49-bcb8-4904-b5b9-f78de2397b1f
📒 Files selected for processing (5)
Entities/GPTSettings.phpHttp/Controllers/FreeScoutGPTController.phpHttp/routes.phpPublic/js/module.jsResources/views/settings.blade.php
✅ Files skipped from review due to trivial changes (1)
- Entities/GPTSettings.php
🚧 Files skipped from review as they are similar to previous changes (1)
- Http/routes.php
|
Hey @presswizards Both key flows were checked and are working:
|
|
@ku9nov I’ll review and do some testing of this more soon and look at merging once complete. |
2 participants
This PR adds a new Edit Draft action to the reply editor, allowing agents to send their current reply text to AI for grammar, clarity, and tone improvements without generating a new response from the customer message.
Fixes request behavior:
Summary by CodeRabbit
New Features
Improvements