chore: normalize urls extraction for messages#38795
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
/jira ARCH-1935 |
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/lib/server/functions/extractUrlsFromMessageAST.ts">
<violation number="1" location="apps/meteor/app/lib/server/functions/extractUrlsFromMessageAST.ts:23">
P2: Protocol-relative URLs from LINK nodes are pushed without normalization, but tests expect `//` links to be converted to `https://`. Normalize before pushing so schema-less links generate previews correctly.</violation>
</file>
<file name="apps/meteor/app/lib/server/functions/parseUrlsInMessage.ts">
<violation number="1" location="apps/meteor/app/lib/server/functions/parseUrlsInMessage.ts:32">
P2: This conditional skips clearing message.urls when no URLs are found, leaving stale URLs on edits that remove links. Assign message.urls even for an empty list so old previews are cleared.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38795 +/- ##
===========================================
- Coverage 70.58% 70.55% -0.03%
===========================================
Files 3182 3182
Lines 112442 112483 +41
Branches 20378 20376 -2
===========================================
- Hits 79365 79361 -4
- Misses 31025 31071 +46
+ Partials 2052 2051 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
b579743 to
0747a55
Compare
Co-authored-by: ggazzo <5263975+ggazzo@users.noreply.github.com>
Co-authored-by: ggazzo <5263975+ggazzo@users.noreply.github.com>
Co-authored-by: ggazzo <5263975+ggazzo@users.noreply.github.com>
…or URL extraction from message AST. Update parseUrlsInMessage to utilize new URL extraction logic.
…logic for oembed iframe generation
Co-authored-by: ggazzo <5263975+ggazzo@users.noreply.github.com>
Co-authored-by: ggazzo <5263975+ggazzo@users.noreply.github.com>
… and message processing. Update insertMessage to assign parsed URLs directly to the message object. Enhance parseUrlsInMessage to return structured URL data and clean up unused code. Adjust MessageService to utilize the updated URL parsing logic.
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.ts">
<violation number="1" location="apps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.ts:141">
P2: Remove debug console logging from this server-side hook to avoid leaking message data and adding noisy logs in production.
(Based on your team's feedback about avoiding console statements in production code.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/meteor/server/services/messages/service.ts">
<violation number="1" location="apps/meteor/server/services/messages/service.ts:239">
P2: Unconditionally assigning `message.urls` breaks the `parseUrls=false` behavior by adding an empty `urls` property. Keep `urls` unset when `parseUrls` is explicitly false.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
apps/meteor/server/services/messages/hooks/BeforeSaveJumpToMessage.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/lib/server/functions/parseUrlsInMessage.ts">
<violation number="1" location="apps/meteor/app/lib/server/functions/parseUrlsInMessage.ts:16">
P2: `parseUrlsInMessage` no longer honors `message.parseUrls === false`, so callers that explicitly disable URL parsing (e.g., webhook payloads with attachments) will still have URLs extracted. This is a behavioral regression; add the guard back so `parseUrls` is respected.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…sage object. Update MessageService to always parse URLs, simplifying the logic by removing the conditional check for parseUrls.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/server/services/messages/service.ts">
<violation number="1" location="apps/meteor/server/services/messages/service.ts:239">
P2: The `parseUrls` flag is no longer honored. `message.urls` is now set even when callers explicitly send `parseUrls: false`, which breaks the documented API behavior and existing tests expecting no `urls` field.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…lag in beforeSave calls, enhancing URL processing flexibility.
435a8d4 to
e4a7ee9
Compare
…tions for async oembed generation.
|
@sampaiodiego fyi |
URL parsing for link previews was called externally in
sendMessageandupdateMessagefunctions. This moves theparseUrlsInMessagecall intoMessage.beforeSaveto consolidate message preparation logic.https://rocketchat.atlassian.net/browse/ARCH-1970
Changes
previewUrlsparameter tobeforeSavemethod signatureparseUrlsInMessage(message, previewUrls)internally after markdown parsingparseUrlsInMessagecalls, now passpreviewUrlstobeforeSaveExample
Before:
After:
All message processing now flows through a single lifecycle point. Existing callers without
previewUrlscontinue to work unchanged.💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.