Fix/ai link loss - #458
Merged
Merged
Conversation
AI grammar fix rewrote the draft as plain text, so links in the reply box were dropped. It now sends the editor HTML and asks the model to keep tags. Conversation transcripts, the AI agent history and copilot context also stripped link URLs when converting HTML to text, so the model never saw them. They now keep links as "text ( url )".
Contributor
📝 WalkthroughWalkthroughAI responses are stored as markdown and returned as HTML. Valid outer Markdown code fences are removed. HTML-to-text conversion now preserves link URLs for transcripts and quote processing. ChangesAI content normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Provider
participant Completion
participant Copilot
participant ConversationStore
participant ConversationPanel
Provider->>Completion: return generated response
Completion->>Completion: strip valid outer code fence
Completion->>Completion: normalize response as HTML fragment
Completion->>Copilot: return normalized response
Copilot->>ConversationStore: persist markdown response
Copilot->>ConversationPanel: return HTML response
ConversationStore->>ConversationPanel: provide history
ConversationPanel->>ConversationPanel: convert assistant markdown to HTML
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/ai.go`:
- Around line 439-444: Update handleGetCopilotMessages so assistant messages
that already contain legacy HTML are detected or marked before Markdown2HTML is
applied, preserving their existing markup while still converting Markdown
assistant content. Use the message format metadata or established legacy-row
detection mechanism rather than treating every assistant message as Markdown.
In `@frontend/apps/main/src/features/conversation/ReplyBox.vue`:
- Around line 225-226: Update the htmlContent assignment in ReplyBox.vue to
avoid treating arbitrary angle-bracketed text, including email addresses, as
HTML. Prefer an explicit API response-format indicator when available; otherwise
use a strict HTML fragment matcher that only accepts valid expected tags, and
add a regression case ensuring text such as “Contact <customer@example.com>”
remains plain text.
In `@internal/ai/ai.go`:
- Line 43: The codeFenceOpenRe matcher in internal/ai/ai.go at lines 43-43 must
accept punctuation-bearing language labels such as c++ and text/plain while
still rejecting additional opening-line content; update the pattern accordingly.
Add matching test cases in internal/ai/ai_test.go at lines 11-50 covering both
labels.
🪄 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 Plus
Run ID: 87dc77d1-6927-4420-aa63-4864d48c32b8
📒 Files selected for processing (13)
cmd/ai.gofrontend/apps/main/src/features/conversation/ReplyBox.vueinternal/ai/agent.gointernal/ai/ai.gointernal/ai/ai_test.gointernal/ai/copilot.gointernal/aiagent/worker.gointernal/conversation/models/models.gointernal/conversation/models/models_test.gointernal/stringutil/emailquote.gointernal/stringutil/emailquote_test.gointernal/stringutil/stringutil.gointernal/stringutil/stringutil_test.go
Transcripts flattened links to "text ( url )", so the model copied that shape and drafts came back with the URL in brackets instead of a link. HTML to text for the LLM now writes "[text](url)", which Markdown2HTML turns back into an anchor.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/stringutil/htmllinks.go`:
- Around line 41-42: Update the href filtering condition in the HTML link
conversion logic to detect the cid: scheme case-insensitively, preserving
plain-text handling for values such as CID:image-1. Add a test case covering an
uppercase CID: prefix alongside the existing href exclusion tests.
🪄 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 Plus
Run ID: eec7587d-7ae4-4a3c-9313-7632e78999f0
📒 Files selected for processing (8)
internal/aiagent/worker.gointernal/conversation/models/models.gointernal/conversation/models/models_test.gointernal/stringutil/emailquote.gointernal/stringutil/emailquote_test.gointernal/stringutil/htmllinks.gointernal/stringutil/stringutil.gointernal/stringutil/stringutil_test.go
💤 Files with no reviewable changes (1)
- internal/stringutil/stringutil.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/stringutil/emailquote_test.go
- internal/aiagent/worker.go
- internal/stringutil/emailquote.go
- internal/conversation/models/models.go
- internal/conversation/models/models_test.go
Comment on lines
+41
to
+42
| if href == "" || strings.HasPrefix(href, "#") || strings.HasPrefix(href, "cid:") { | ||
| return "" |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle cid: schemes without case sensitivity.
Line 41 only excludes lowercase cid: values. URI schemes are case-insensitive, so CID:image-1 becomes a Markdown link instead of remaining plain text. Use a case-insensitive prefix check. Add a CID: test case.
Proposed fix
- if href == "" || strings.HasPrefix(href, "#") || strings.HasPrefix(href, "cid:") {
+ if href == "" || strings.HasPrefix(href, "#") ||
+ (len(href) >= len("cid:") && strings.EqualFold(href[:len("cid:")], "cid:")) {
return ""
}📝 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
| if href == "" || strings.HasPrefix(href, "#") || strings.HasPrefix(href, "cid:") { | |
| return "" | |
| if href == "" || strings.HasPrefix(href, "#") || | |
| (len(href) >= len("cid:") && strings.EqualFold(href[:len("cid:")], "cid:")) { | |
| return "" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/stringutil/htmllinks.go` around lines 41 - 42, Update the href
filtering condition in the HTML link conversion logic to detect the cid: scheme
case-insensitively, preserving plain-text handling for values such as
CID:image-1. Add a test case covering an uppercase CID: prefix alongside the
existing href exclusion tests.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/stringutil/htmllinks.go (1)
44-51: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEscape other Markdown-significant characters in link text.
markdownLinkescapes only[and]in the anchor text. If the text contains_,*, or`, the returned Markdown can be misinterpreted as emphasis, strong emphasis, or code spans when re-rendered, corrupting the visible link text. Extend the replacer to cover these characters too.♻️ Proposed fix
- return "[" + strings.NewReplacer("[", `\[`, "]", `\]`).Replace(text) + "](" + href + ")" + escaper := strings.NewReplacer("\\", `\\`, "[", `\[`, "]", `\]`, "_", `\_`, "*", `\*`, "`", "\\`") + return "[" + escaper.Replace(text) + "](" + href + ")"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/stringutil/htmllinks.go` around lines 44 - 51, The strings.NewReplacer call in the return statement of markdownLink only escapes brackets but allows other Markdown-significant characters like underscore, asterisk, and backtick to pass through unescaped, causing potential misinterpretation when the link text is re-rendered. Extend the strings.NewReplacer to include escape sequences for underscore, asterisk, and backtick characters in addition to the existing bracket escaping to preserve the intended link text appearance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/init.go`:
- Around line 871-879: Change initI18n to return the initialization error
instead of calling log.Fatalf for file-read or loadI18nLang failures, and update
every caller to handle the returned error. Preserve fatal startup behavior at
the startup caller, while handleUpdateGeneralSettings must return an appropriate
HTTP error response and leave the running server alive when a runtime language
update fails.
- Around line 871-873: Validate req.Lang in handleUpdateGeneralSettings against
the available i18n language files before calling app.setting.Update(req),
rejecting unsupported values. In initI18n, validate the persisted app.lang
similarly and fall back to defLang (or reject it) before constructing the
/i18n/{lang}.json path.
---
Nitpick comments:
In `@internal/stringutil/htmllinks.go`:
- Around line 44-51: The strings.NewReplacer call in the return statement of
markdownLink only escapes brackets but allows other Markdown-significant
characters like underscore, asterisk, and backtick to pass through unescaped,
causing potential misinterpretation when the link text is re-rendered. Extend
the strings.NewReplacer to include escape sequences for underscore, asterisk,
and backtick characters in addition to the existing bracket escaping to preserve
the intended link text appearance.
🪄 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 Plus
Run ID: a2085f34-e768-403f-931b-fb1bed23432b
📒 Files selected for processing (9)
cmd/init.gointernal/aiagent/worker.gointernal/conversation/models/models.gointernal/conversation/models/models_test.gointernal/stringutil/emailquote.gointernal/stringutil/emailquote_test.gointernal/stringutil/htmllinks.gointernal/stringutil/stringutil.gointernal/stringutil/stringutil_test.go
💤 Files with no reviewable changes (1)
- internal/stringutil/stringutil.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/conversation/models/models.go
- internal/stringutil/emailquote.go
- internal/stringutil/emailquote_test.go
- internal/conversation/models/models_test.go
- internal/aiagent/worker.go
- internal/stringutil/stringutil_test.go
Comment on lines
+871
to
+873
| lang := cmp.Or(ko.String("app.lang"), defLang) | ||
| log.Printf("loading i18n language file: %s", lang) | ||
| if _, err := fs.Read("/i18n/" + lang + ".json"); err != nil { |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether app.lang is validated against a known language list before persistence.
rg -n -C5 '"lang"|Lang\b' internal/ cmd/ --type=goRepository: abhinavxd/libredesk
Length of output: 5842
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^cmd/(init|settings|i18n|handlers)\.go$|internal/settings|internal/setting)'
echo
echo "== settings model and handlers =="
cat -n internal/setting/models/models.go
echo
cat -n cmd/settings.go | sed -n '1,120p'
echo
echo "== setting update implementation candidates =="
rg -n -C4 'type .*Setting|func .*Update|UpdateSettings|settings.*Update|ko\.String\("app\.lang"\)|app\.lang' internal cmd --type=go
echo
echo "== handlers i18n relevant =="
cat -n cmd/i18n.go | sed -n '1,180p'Repository: abhinavxd/libredesk
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== internal setting implementation =="
cat -n internal/setting/setting.go | sed -n '1,220p'
echo
echo "== setting SQL for app.lang =="
cat -n internal/setting/queries.sql | sed -n '1,220p'
echo
echo "== available lang endpoint/handlers =="
cat -n cmd/i18n.go | sed -n '33,75p'
echo
echo "== migrations mentioning app.lang and lang map =="
cat -n internal/migrations/v2.3.0.go | sed -n '1,120p'
rg -n "langMap|en-US|en_GB|de-DE|fr-FR|es-ES|pt-PT|zh-CN|zh-Hans|ar-SA|tr-TR|ru-RU" internal migrations cmd --type=go --iglob '*.go'Repository: abhinavxd/libredesk
Length of output: 11913
Validate app.lang before saving it to settings.
handleUpdateGeneralSettings accepts any req.Lang value and passes it to app.setting.Update(req), which only marshals and persists the JSON without checking supported i18n codes. Validate req.Lang against the available lang files before calling Update, and have initI18n fall back to defLang or reject an invalid persisted value instead of using it directly in "/i18n/" + lang + ".json".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/init.go` around lines 871 - 873, Validate req.Lang in
handleUpdateGeneralSettings against the available i18n language files before
calling app.setting.Update(req), rejecting unsupported values. In initI18n,
validate the persisted app.lang similarly and fall back to defLang (or reject
it) before constructing the /i18n/{lang}.json path.
Comment on lines
+871
to
879
| lang := cmp.Or(ko.String("app.lang"), defLang) | ||
| log.Printf("loading i18n language file: %s", lang) | ||
| if _, err := fs.Read("/i18n/" + lang + ".json"); err != nil { | ||
| log.Fatalf("error reading i18n language file `%s` : %v", lang, err) | ||
| } | ||
| i18n, err := i18n.New(file.ReadBytes()) | ||
| i18n, err := loadI18nLang(lang, fs) | ||
| if err != nil { | ||
| log.Fatalf("error initializing i18n: %v", err) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
log.Fatalf inside initI18n can crash the running server on a runtime settings update.
initI18n calls log.Fatalf when the file read fails (Line 874) or when loadI18nLang fails (Line 878). log.Fatalf calls os.Exit(1) and terminates the whole process.
cmd/settings.go calls app.i18n = initI18n(app.fs) from handleUpdateGeneralSettings whenever an admin changes the language setting at runtime. If the corresponding /i18n/{lang}.json file is missing, unreadable, or malformed at that point, the entire server process exits, not just the request. This turns a single misconfigured admin update into a full outage for all users.
Change initI18n to return an error to its callers, and let each caller decide the behavior: fatal at startup, but return an HTTP error response (without crashing) from handleUpdateGeneralSettings.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/init.go` around lines 871 - 879, Change initI18n to return the
initialization error instead of calling log.Fatalf for file-read or loadI18nLang
failures, and update every caller to handle the returned error. Preserve fatal
startup behavior at the startup caller, while handleUpdateGeneralSettings must
return an appropriate HTTP error response and leave the running server alive
when a runtime language update fails.
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 by CodeRabbit
New Features
Bug Fixes