refactor: rename isURL to isAbsoluteURL for clarity#38821
Open
amitb0ra wants to merge 8 commits intoRocketChat:developfrom
Open
refactor: rename isURL to isAbsoluteURL for clarity#38821amitb0ra wants to merge 8 commits intoRocketChat:developfrom
amitb0ra wants to merge 8 commits intoRocketChat:developfrom
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (2)📚 Learning: 2025-09-19T15:15:04.642ZApplied to files:
📚 Learning: 2026-01-17T01:51:47.764ZApplied to files:
🧬 Code graph analysis (2)apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (1)
apps/meteor/app/custom-oauth/server/custom_oauth_server.js (1)
⏰ 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)
🔇 Additional comments (3)
WalkthroughThe PR centralizes absolute-URL validation by introducing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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. 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 |
KevLehman
reviewed
Feb 19, 2026
Member
KevLehman
left a comment
KevLehman
left a comment
There was a problem hiding this comment.
Can you move the isAbsoluteUrl to the @rocket.chat/tools package, create some tests, and then use the imported function from there in all places?
After that, remove the files that are currently exporting isAbsoluteUrl.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 9 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/custom-oauth/server/custom_oauth_server.js">
<violation number="1" location="apps/meteor/app/custom-oauth/server/custom_oauth_server.js:100">
P1: Relative identityPath values are no longer normalized to absolute URLs, and addAutopublishFields is now incorrectly gated by the identityPath check with no type guard. This looks like a regression from the rename that can break OAuth identity fetches and mis-handle autopublish fields.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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
🧹 Nitpick comments (2)
apps/meteor/tests/unit/lib/utils/isAbsoluteURL.spec.ts (1)
6-14: Consider addinghttp://andjavascript:test cases.The test suite covers
https://but nothttp://explicitly (thes?regex handles both, but an explicit test documents the contract). More importantly,javascript:is absent —sendMessage.tsrelies onisAbsoluteURLreturningfalseforjavascript:URLs as part of its XSS guard; an explicit test case would lock in that security-sensitive behavior.✅ Suggested additions to testCases
const testCases = [ ['/', false], ['test', false], ['test/test', false], ['.', false], ['./test', false], + ['http://rocket.chat', true], ['https://rocket.chat', true], ['data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==', true], + ['javascript:alert(1)', false], + ['//example.com', false], ] as const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/unit/lib/utils/isAbsoluteURL.spec.ts` around lines 6 - 14, Add explicit test cases to the testCases constant in isAbsoluteURL.spec.ts to cover "http://" and "javascript:" inputs: keep isAbsoluteURL behavior (expect true for "http://..." and false for "javascript:...") so the test documents the intended contract and prevents regressions that could affect sendMessage.ts's XSS guard; update the tuple list in testCases and ensure the assertions still use the isAbsoluteURL function.apps/meteor/tests/unit/lib/utils/isURL.spec.ts (1)
3-3: Confusing import path and duplicate test coverage.
isAbsoluteURLis being imported fromisURL.ts(Line 3), which is the old path. All test cases here are identical to those inisAbsoluteURL.spec.ts, creating redundant coverage. OnceisURL.tsis cleaned up (re-export or deletion), this file should be removed as the canonical coverage lives inisAbsoluteURL.spec.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/unit/lib/utils/isURL.spec.ts` at line 3, The tests in this file duplicate the canonical isAbsoluteURL tests and import isAbsoluteURL from the old isURL.ts; either update the import to use the canonical module (import isAbsoluteURL from the new/lib utils module that hosts the function) or, preferably, after you clean up isURL.ts (by re-exporting or deleting it), delete this duplicate test file entirely and rely on isAbsoluteURL.spec.ts for coverage—search for the symbol isAbsoluteURL and the tests in isAbsoluteURL.spec.ts to confirm coverage before removing this file.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/meteor/app/custom-oauth/server/custom_oauth_server.jsapps/meteor/app/lib/server/functions/sendMessage.tsapps/meteor/app/utils/lib/getURL.tsapps/meteor/client/lib/customOAuth/CustomOAuth.tsapps/meteor/lib/utils/isAbsoluteURL.tsapps/meteor/lib/utils/isURL.tsapps/meteor/server/services/messages/hooks/AfterSaveOEmbed.tsapps/meteor/tests/unit/lib/utils/isAbsoluteURL.spec.tsapps/meteor/tests/unit/lib/utils/isURL.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
apps/meteor/lib/utils/isAbsoluteURL.tsapps/meteor/tests/unit/lib/utils/isAbsoluteURL.spec.tsapps/meteor/client/lib/customOAuth/CustomOAuth.tsapps/meteor/app/custom-oauth/server/custom_oauth_server.jsapps/meteor/lib/utils/isURL.tsapps/meteor/app/utils/lib/getURL.tsapps/meteor/app/lib/server/functions/sendMessage.tsapps/meteor/tests/unit/lib/utils/isURL.spec.tsapps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/tests/unit/lib/utils/isAbsoluteURL.spec.tsapps/meteor/tests/unit/lib/utils/isURL.spec.ts
🧠 Learnings (12)
📚 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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/unit/lib/utils/isAbsoluteURL.spec.tsapps/meteor/tests/unit/lib/utils/isURL.spec.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 : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/unit/lib/utils/isAbsoluteURL.spec.tsapps/meteor/tests/unit/lib/utils/isURL.spec.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 : Group related tests in the same file
Applied to files:
apps/meteor/tests/unit/lib/utils/isAbsoluteURL.spec.tsapps/meteor/tests/unit/lib/utils/isURL.spec.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 : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/unit/lib/utils/isAbsoluteURL.spec.tsapps/meteor/tests/unit/lib/utils/isURL.spec.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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/unit/lib/utils/isAbsoluteURL.spec.tsapps/meteor/tests/unit/lib/utils/isURL.spec.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:
apps/meteor/tests/unit/lib/utils/isAbsoluteURL.spec.tsapps/meteor/tests/unit/lib/utils/isURL.spec.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/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/unit/lib/utils/isAbsoluteURL.spec.tsapps/meteor/tests/unit/lib/utils/isURL.spec.ts
📚 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:
apps/meteor/tests/unit/lib/utils/isAbsoluteURL.spec.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/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/unit/lib/utils/isAbsoluteURL.spec.tsapps/meteor/tests/unit/lib/utils/isURL.spec.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 : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/tests/unit/lib/utils/isAbsoluteURL.spec.ts
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
apps/meteor/tests/unit/lib/utils/isAbsoluteURL.spec.tsapps/meteor/tests/unit/lib/utils/isURL.spec.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/lib/customOAuth/CustomOAuth.ts
🧬 Code graph analysis (6)
apps/meteor/app/custom-oauth/server/custom_oauth_server.js (1)
apps/meteor/lib/utils/isAbsoluteURL.ts (1)
isAbsoluteURL(1-1)
apps/meteor/lib/utils/isURL.ts (1)
apps/meteor/lib/utils/isAbsoluteURL.ts (1)
isAbsoluteURL(1-1)
apps/meteor/app/utils/lib/getURL.ts (2)
apps/meteor/lib/utils/isURL.ts (1)
isAbsoluteURL(1-1)apps/meteor/lib/utils/isAbsoluteURL.ts (1)
isAbsoluteURL(1-1)
apps/meteor/app/lib/server/functions/sendMessage.ts (3)
apps/meteor/lib/utils/isURL.ts (1)
isAbsoluteURL(1-1)apps/meteor/lib/utils/isAbsoluteURL.ts (1)
isAbsoluteURL(1-1)apps/meteor/app/file-upload/server/lib/FileUpload.ts (1)
FileUpload(113-727)
apps/meteor/tests/unit/lib/utils/isURL.spec.ts (1)
apps/meteor/lib/utils/isURL.ts (1)
isAbsoluteURL(1-1)
apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (2)
apps/meteor/lib/utils/isURL.ts (1)
isAbsoluteURL(1-1)apps/meteor/lib/utils/isAbsoluteURL.ts (1)
isAbsoluteURL(1-1)
🔇 Additional comments (7)
apps/meteor/lib/utils/isAbsoluteURL.ts (1)
1-1: LGTM — clean, correct implementation.The regex correctly anchors to the start and covers
http://,https://, anddata:schemes.apps/meteor/client/lib/customOAuth/CustomOAuth.ts (1)
8-8: LGTM — correct import swap.apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts (1)
21-21: LGTM — correct import swap.apps/meteor/app/utils/lib/getURL.ts (1)
3-3: LGTM — correct import swap.apps/meteor/app/lib/server/functions/sendMessage.ts (1)
9-9: LGTM — correct import swap; XSS guards and validation logic unchanged.apps/meteor/app/custom-oauth/server/custom_oauth_server.js (2)
13-13: LGTM — import path and symbol match the new utility.
96-98: LGTM —tokenPathnormalization is correct.The guard and concatenation pattern matches the intent;
this.tokenPathis used verbatim infetch()at line 135, so absolute URL normalization is required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/custom-oauth/server/custom_oauth_server.js`:
- Around line 100-102: Normalize this.identityPath the same way tokenPath is
normalized: if this.identityPath is not an absolute URL, prepend this.serverURL
so fetch(this.identityPath) never runs with a bare relative path; also call
Accounts.addAutopublishFields guarded and independently of whether identityPath
is absolute by checking if options && options.addAutopublishFields is truthy
before invoking Accounts.addAutopublishFields(options.addAutopublishFields) so
providers that supply an absolute identityPath still register autopublish fields
and you avoid passing undefined to Accounts.addAutopublishFields.
- Around line 96-103: Re-indent the block that handles absolute URLs and the
Accounts.addAutopublishFields call inside the configure() method so it matches
the method-body indentation used elsewhere (use the same 2-tab/method-body level
as lines 56–94); specifically adjust the lines that reference this.tokenPath,
this.identityPath and Accounts.addAutopublishFields so their indentation aligns
with the rest of configure(), and verify the surrounding braces remain correctly
paired after reformatting.
In `@apps/meteor/lib/utils/isURL.ts`:
- Line 1: Replace the duplicate implementation in isURL.ts with a re-export of
the canonical function from isAbsoluteURL.ts: remove the local isAbsoluteURL
implementation and export the symbol imported from './isAbsoluteURL' (so
consumers like isURL.spec.ts continue to import isAbsoluteURL from isURL.ts but
the actual implementation lives only in isAbsoluteURL.ts). Ensure the exported
name matches the original (isAbsoluteURL) and update any local imports if
necessary.
---
Nitpick comments:
In `@apps/meteor/tests/unit/lib/utils/isAbsoluteURL.spec.ts`:
- Around line 6-14: Add explicit test cases to the testCases constant in
isAbsoluteURL.spec.ts to cover "http://" and "javascript:" inputs: keep
isAbsoluteURL behavior (expect true for "http://..." and false for
"javascript:...") so the test documents the intended contract and prevents
regressions that could affect sendMessage.ts's XSS guard; update the tuple list
in testCases and ensure the assertions still use the isAbsoluteURL function.
In `@apps/meteor/tests/unit/lib/utils/isURL.spec.ts`:
- Line 3: The tests in this file duplicate the canonical isAbsoluteURL tests and
import isAbsoluteURL from the old isURL.ts; either update the import to use the
canonical module (import isAbsoluteURL from the new/lib utils module that hosts
the function) or, preferably, after you clean up isURL.ts (by re-exporting or
deleting it), delete this duplicate test file entirely and rely on
isAbsoluteURL.spec.ts for coverage—search for the symbol isAbsoluteURL and the
tests in isAbsoluteURL.spec.ts to confirm coverage before removing this file.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/meteor/app/lib/server/functions/sendMessage.ts (1)
7-10:@rocket.chat/toolsimport is out of order — move it with the other external package imports.Line 9 inserts an external/scoped-package import between relative imports. It should be co-located with the other
@rocket.chat/*imports (lines 1–5).♻️ Proposed fix
import { AppEvents, Apps } from '@rocket.chat/apps'; import { Message } from '@rocket.chat/core-services'; import type { IMessage, IRoom } from '@rocket.chat/core-typings'; import { Messages } from '@rocket.chat/models'; +import { isAbsoluteURL } from '@rocket.chat/tools'; import { Match, check } from 'meteor/check'; import { parseUrlsInMessage } from './parseUrlsInMessage'; import { isRelativeURL } from '../../../../lib/utils/isRelativeURL'; -import { isAbsoluteURL } from '@rocket.chat/tools'; import { hasPermissionAsync } from '../../../authorization/server/functions/hasPermission';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/lib/server/functions/sendMessage.ts` around lines 7 - 10, The import of isAbsoluteURL from '@rocket.chat/tools' is misplaced between relative imports; move the import of isAbsoluteURL so that it is grouped with other external/@rocket.chat package imports (keeping parseUrlsInMessage and isRelativeURL as local relative imports together and hasPermissionAsync with internal server imports) to maintain consistent import ordering in sendMessage.ts.packages/tools/src/isAbsoluteURL.spec.ts (1)
4-15: Add an empty-string edge case and a case-sensitivity test.The current suite doesn't cover two useful boundary inputs:
''(empty string) → should returnfalse'HTTPS://example.com'→ returnsfalsebecause the regex is case-sensitive; worth documenting explicitly♻️ Suggested additions to the false cases
test.each([ ['/', false], ['test', false], ['test/test', false], ['.', false], ['./test', false], ['/absolute/path', false], ['relative/path?query=1', false], ['ftp://example.com', false], + ['', false], + ['HTTPS://example.com', false], ])('should return false for non-absolute URL %# (%s)', (input, expected) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tools/src/isAbsoluteURL.spec.ts` around lines 4 - 15, Add two additional negative test cases to the isAbsoluteURL test data: an empty string ('') expecting false and an uppercase-scheme URL ('HTTPS://example.com') expecting false to capture the current case-sensitive behavior; update the test.each array in isAbsoluteURL.spec.ts (the table used by the test that calls isAbsoluteURL(...)) to include ['' , false] and ['HTTPS://example.com', false] so these edge cases are asserted.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/meteor/app/custom-oauth/server/custom_oauth_server.jsapps/meteor/app/lib/server/functions/sendMessage.tsapps/meteor/app/utils/lib/getURL.tsapps/meteor/client/lib/customOAuth/CustomOAuth.tsapps/meteor/lib/utils/isURL.tsapps/meteor/server/services/messages/hooks/AfterSaveOEmbed.tsapps/meteor/tests/unit/lib/utils/isURL.spec.tspackages/tools/src/index.tspackages/tools/src/isAbsoluteURL.spec.tspackages/tools/src/isAbsoluteURL.ts
💤 Files with no reviewable changes (2)
- apps/meteor/lib/utils/isURL.ts
- apps/meteor/tests/unit/lib/utils/isURL.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/meteor/app/custom-oauth/server/custom_oauth_server.js
- apps/meteor/app/utils/lib/getURL.ts
- apps/meteor/client/lib/customOAuth/CustomOAuth.ts
- apps/meteor/server/services/messages/hooks/AfterSaveOEmbed.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/tools/src/isAbsoluteURL.spec.tsapps/meteor/app/lib/server/functions/sendMessage.tspackages/tools/src/isAbsoluteURL.tspackages/tools/src/index.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
packages/tools/src/isAbsoluteURL.spec.ts
🧠 Learnings (9)
📚 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 : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
packages/tools/src/isAbsoluteURL.spec.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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
packages/tools/src/isAbsoluteURL.spec.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 : Group related tests in the same file
Applied to files:
packages/tools/src/isAbsoluteURL.spec.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 : Maintain test isolation between test cases in Playwright tests
Applied to files:
packages/tools/src/isAbsoluteURL.spec.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 **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
packages/tools/src/isAbsoluteURL.spec.ts
📚 Learning: 2025-12-10T21:00:43.645Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:43.645Z
Learning: Adopt the monorepo-wide Jest testMatch pattern: <rootDir>/src/**/*.spec.{ts,js,mjs} (represented here as '**/src/**/*.spec.{ts,js,mjs}') to ensure spec files under any package's src directory are picked up consistently across all packages in the Rocket.Chat monorepo. Apply this pattern in jest.config.ts for all relevant packages to maintain uniform test discovery.
Applied to files:
packages/tools/src/isAbsoluteURL.spec.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/tools/src/isAbsoluteURL.spec.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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
packages/tools/src/isAbsoluteURL.spec.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/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
packages/tools/src/isAbsoluteURL.spec.ts
🧬 Code graph analysis (2)
packages/tools/src/isAbsoluteURL.spec.ts (1)
packages/tools/src/isAbsoluteURL.ts (1)
isAbsoluteURL(1-1)
apps/meteor/app/lib/server/functions/sendMessage.ts (2)
packages/tools/src/isAbsoluteURL.ts (1)
isAbsoluteURL(1-1)apps/meteor/app/file-upload/server/lib/FileUpload.ts (1)
FileUpload(113-727)
⏰ 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 comments (3)
packages/tools/src/isAbsoluteURL.ts (1)
1-1: LGTM!The implementation correctly captures the stated behavior: prefix-matching
http://,https://, anddata:schemes. The type signature and lack of code comments align with the project guidelines.apps/meteor/app/lib/server/functions/sendMessage.ts (1)
36-36: LGTM — functional replacement is correct.Both
validFullURLParam(Line 36) andvalidPartialURLParam(Line 50) correctly swapisURLforisAbsoluteURLwith identical semantics. No behavioral change.Also applies to: 50-50
packages/tools/src/index.ts (1)
19-19: LGTM — consistent with existing barrel export pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/app/lib/server/functions/sendMessage.ts`:
- Around line 7-10: The import of isAbsoluteURL from '@rocket.chat/tools' is
misplaced between relative imports; move the import of isAbsoluteURL so that it
is grouped with other external/@rocket.chat package imports (keeping
parseUrlsInMessage and isRelativeURL as local relative imports together and
hasPermissionAsync with internal server imports) to maintain consistent import
ordering in sendMessage.ts.
In `@packages/tools/src/isAbsoluteURL.spec.ts`:
- Around line 4-15: Add two additional negative test cases to the isAbsoluteURL
test data: an empty string ('') expecting false and an uppercase-scheme URL
('HTTPS://example.com') expecting false to capture the current case-sensitive
behavior; update the test.each array in isAbsoluteURL.spec.ts (the table used by
the test that calls isAbsoluteURL(...)) to include ['' , false] and
['HTTPS://example.com', false] so these edge cases are asserted.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38821 +/- ##
===========================================
- Coverage 70.56% 70.52% -0.05%
===========================================
Files 3182 3182
Lines 112471 112472 +1
Branches 20355 20354 -1
===========================================
- Hits 79369 79320 -49
- Misses 31050 31101 +51
+ 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:
|
Member
|
Sadly CI is failing |
Author
fixed now :) |
KevLehman
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
KevLehman
approved these changes
Feb 20, 2026
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
2 participants
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.
Proposed changes (including videos or screenshots)
This PR addresses an existing
@todocomment inapps/meteor/lib/utils/isURL.tsthat explicitly requested the function be renamed to better reflect its purpose:The function
isURLonly checks whether a string starts withhttps://,http://, ordata:— meaning it specifically validates absolute URLs, not URLs in general (e.g., relative URLs like/pathor./filereturnfalse). The nameisURLis misleading and could cause confusion, especially since a sibling utilityisRelativeURLalready exists in the codebase.Issue(s)
Resolves inline todo in
apps/meteor/lib/utils/isURL.ts:Steps to test or reproduce
isURLimport fromlib/utils/isURL— there should be noneisAbsoluteURLto determine if a path is absolute or needs a server URL prefix)Summary by CodeRabbit
Refactor
Tests