fix(message-parser): avoid protocol duplication for invalid absolute URLs#39293
Conversation
|
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds an internal helper to detect absolute URL schemes and updates autoLink to treat sources with absolute schemes as already-valid hrefs; adds tests verifying autoLink preserves original protocol/port for invalid absolute URLs (e.g., out-of-range ports). Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/message-parser/src/utils.ts`:
- Line 78: The scheme regex in hasAbsoluteSchemePrefix currently allows a
leading digit and omits the dot character; update the regex used in
hasAbsoluteSchemePrefix to enforce RFC 3986: require the first character to be a
letter and allow letters, digits, plus, dot and hyphen for the remaining
characters (respecting the existing max-length logic), so it correctly detects
schemes like "com.example://". Replace the current pattern in
hasAbsoluteSchemePrefix with one that matches a leading letter followed by
allowed scheme chars and "://".
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/message-parser/src/utils.tspackages/message-parser/tests/url.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/message-parser/tests/url.test.tspackages/message-parser/src/utils.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
packages/message-parser/tests/url.test.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
packages/message-parser/tests/url.test.ts
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
packages/message-parser/tests/url.test.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/message-parser/tests/url.test.tspackages/message-parser/src/utils.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/message-parser/tests/url.test.tspackages/message-parser/src/utils.ts
🧬 Code graph analysis (1)
packages/message-parser/tests/url.test.ts (2)
packages/message-parser/src/utils.ts (3)
paragraph(27-27)link(80-83)autoLink(85-100)packages/message-parser/tests/helpers.ts (2)
paragraph(6-6)link(39-42)
🔇 Additional comments (3)
packages/message-parser/src/utils.ts (1)
97-97: Good fallback ordering for invalid absolute URLsLine 97 correctly preserves already-absolute sources before applying
//, which addresses the reported duplicated-protocol regression path.packages/message-parser/tests/url.test.ts (2)
28-29: Regression cases are well-targetedThese added parser cases cover the invalid-absolute-port scenarios that triggered protocol duplication and validate both root and path variants.
162-165: Helper-level regression coverage looks solidThe helper test explicitly guards invalid absolute port handling (including boundary-adjacent 65536), which strengthens future refactor safety.
|
Hey @Shreyas2004wagh, thank you for your contribution! 🙏 Your changes have been consolidated into #39853, where we merged all message-parser PRs together to make it easier to test and validate everything as a single unit. That PR has already been merged into |
2 participants
Summary
autoLinkeven whenURL()validation fails (for example, invalid ports).//to already-absolute inputs such ashttps://rocket.chat:99999.Problem
When
autoLinkreceived an absolute URL that failedURL()validation, it prepended//and produced malformed href values like//https://rocket.chat:99999.Testing
Parser build succeeded locally.
Verified parse output for:
https://rocket.chat:99999https://rocket.chat:99999/testhttps://rocket.chat:3000/testrocket.chat/testrocket.chattt/url_pathFix chore(message-parser): tracking issue for parser correctness, performance, and test hardening #39295
Summary by CodeRabbit