fix: add ROOT_URL fallback to getURL#38872
fix: add ROOT_URL fallback to getURL#38872aniruddhaadak80 wants to merge 2 commits intoRocketChat:developfrom
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 detailsConfiguration 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)
WalkthroughUpdated README label "Youtube" → "YouTube". Changed server URL helper to use Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant getURL
participant Settings
participant Env
Caller->>getURL: request absolute site URL
getURL->>Settings: read 'Site_Url'
alt Site_Url present
Settings-->>getURL: return Site_Url
else Site_Url missing
getURL->>Env: read ROOT_URL
alt ROOT_URL present
Env-->>getURL: return ROOT_URL
else ROOT_URL missing
Env-->>getURL: return empty string
end
end
getURL-->>Caller: return resolved site URL (or empty)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
aniruddhaadak80
force-pushed
the
fix/geturl-fallback-rooturl
branch
from
985e76a to
3bfeaaf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/utils/server/getURL.ts (1)
16-16: No test coverage for the new fallback pathThe PR description outlines a clear, testable scenario (unset
Site_Url, setROOT_URL, callgetURL()), but no unit test is added. Consider adding a test to the existinggetURLtest suite covering this fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/utils/server/getURL.ts` at line 16, Add a unit test to the existing getURL test suite that verifies the fallback path when Site_Url is unset: mock settings.get to return empty/undefined for 'Site_Url', set process.env.ROOT_URL to a test URL, call getURL(), and assert the returned value equals process.env.ROOT_URL; be sure to restore or clear process.env and any settings mocks after the test to avoid cross-test pollution and reference the getURL function and settings.get call in the test setup.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.mdapps/meteor/app/utils/server/getURL.ts
🧰 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:
apps/meteor/app/utils/server/getURL.ts
🧠 Learnings (2)
📚 Learning: 2026-01-08T15:03:59.621Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38068
File: apps/meteor/tests/data/apps/app-packages/README.md:14-16
Timestamp: 2026-01-08T15:03:59.621Z
Learning: For the RocketChat/Rocket.Chat repository, do not analyze or report formatting issues (such as hard tabs vs spaces, line breaks, etc.). The project relies on automated linting tools to enforce formatting standards.
Applied to files:
README.md
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
README.md
⏰ 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 (2)
README.md (1)
118-118: LGTM! Correct capitalization of "YouTube".apps/meteor/app/utils/server/getURL.ts (1)
16-16: Trailing slash normalization is already applied downstream — no fix needed.The concern about
ROOT_URLtrailing slashes causing double-slash URLs is already handled. In_getURL(lib/getURL.ts, line 52),siteUrlis normalized viartrim(trim(_site_url || ''), '/'), which removes trailing slashes before URL concatenation. The proposed fix to stripROOT_URLat line 16 is redundant.
🤖 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/utils/server/getURL.ts`:
- Line 16: Add a unit test to the existing getURL test suite that verifies the
fallback path when Site_Url is unset: mock settings.get to return
empty/undefined for 'Site_Url', set process.env.ROOT_URL to a test URL, call
getURL(), and assert the returned value equals process.env.ROOT_URL; be sure to
restore or clear process.env and any settings mocks after the test to avoid
cross-test pollution and reference the getURL function and settings.get call in
the test setup.
Updates the server-side getURL function to fallback to process.env.ROOT_URL when the Site_Url setting is not configured. This ensures valid URL generation during initial setup or in restricted environments where settings might be missing. Fixes RocketChat#38867
aniruddhaadak80
force-pushed
the
fix/geturl-fallback-rooturl
branch
from
3bfeaaf to
306b399
Compare
2 participants
Proposed changes
This PR adds a fallback to
process.env.ROOT_URLin thegetURLfunction when theSite_Urlsetting is not configured.Why?
Ensures that the application can still generate valid URLs based on the environment configuration, which is particularly useful during initial setup or in environments where settings haven't been fully populated yet.
Issue(s)
Fixes #38867
Steps to test or reproduce
Site_Urlis not set in the database.ROOT_URLis set in the environment.getURL()and observe that it returns a URL based onROOT_URLinstead of an empty string.Summary by CodeRabbit
Documentation
Bug Fixes