chore: remove oauthRedirectUri override (dead code after Meteor OAuth removal)#40354
Open
billywithaw1lly wants to merge 6 commits intoRocketChat:developfrom
Open
chore: remove oauthRedirectUri override (dead code after Meteor OAuth removal)#40354billywithaw1lly wants to merge 6 commits intoRocketChat:developfrom
billywithaw1lly wants to merge 6 commits intoRocketChat:developfrom
Conversation
Contributor
|
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 |
🦋 Changeset detectedLatest commit: 42a7f04 The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
WalkthroughThis change removes dead code following Meteor OAuth and DDP layer removal, including the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Suggested labels
🚥 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/honest-plants-heal.md:
- Around line 1-5: Update the changeset body to be explicit that this is a
non-functional, non-breaking change by adding a sentence like "This change only
removes a dead-code override (oauthRedirectUri) and does not affect runtime
behavior or public APIs." Also include a reference to the related issue/PR
(e.g., "#40346 / PR `#40354`") for traceability; keep the existing front-matter
('@rocket.chat/meteor': patch) unchanged and ensure the markdown remains concise
and audit-friendly.
In @.changeset/tame-falcons-watch.md:
- Around line 1-5: The changeset message only lists removing useCorsSSLConfig
hook and the absoluteUrl startup patch but omits deletion of the
oauthRedirectUri override; update the .changeset/tame-falcons-watch.md body so
the description explicitly lists oauthRedirectUri alongside useCorsSSLConfig and
the absoluteUrl startup patch (or reword it to cover all three removals) to
ensure release notes match the PR scope and summary.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 961d3031-cc93-4b48-b226-72aa41371706
📒 Files selected for processing (8)
.changeset/honest-plants-heal.md.changeset/tame-falcons-watch.mdapps/meteor/client/meteor/overrides/index.tsapps/meteor/client/meteor/overrides/oauthRedirectUri.tsapps/meteor/client/meteor/startup/absoluteUrl.tsapps/meteor/client/meteor/startup/index.tsapps/meteor/client/views/root/AppLayout.tsxapps/meteor/client/views/root/hooks/useCorsSSLConfig.ts
💤 Files with no reviewable changes (6)
- apps/meteor/client/meteor/startup/index.ts
- apps/meteor/client/meteor/overrides/index.ts
- apps/meteor/client/views/root/AppLayout.tsx
- apps/meteor/client/meteor/startup/absoluteUrl.ts
- apps/meteor/client/views/root/hooks/useCorsSSLConfig.ts
- apps/meteor/client/meteor/overrides/oauthRedirectUri.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 38623
File: apps/meteor/app/lib/server/functions/cleanRoomHistory.ts:146-149
Timestamp: 2026-04-18T12:32:53.425Z
Learning: In `apps/meteor/app/lib/server/functions/cleanRoomHistory.ts` (PR `#38623`), the read-receipt cleanup (both `ReadReceipts.removeByMessageIds` and `ReadReceiptsArchive.removeByMessageIds`) is intentionally only performed in the limited prune path (`limit && selectedMessageIds`). The unlimited/delete-all path (`limit === 0`) deliberately skips cleaning up orphaned read receipts in both hot and cold storage — this is by design. Do not flag this as a bug or missing cleanup in future reviews.
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 40268
File: apps/meteor/client/startup/startup.ts:48-56
Timestamp: 2026-04-29T19:33:29.434Z
Learning: In `apps/meteor/client/startup/startup.ts`, the heuristic `!userIdStore.getState() && localStorage.getItem('Meteor.loginToken') === null` calling `removeLocalUserData()` (which calls `localStorage.clear()`) is intentional. The maintainer (tassoevan) has confirmed this is acceptable even though it fires on fresh first-time visits with no prior login, not just on expired-token resume scenarios. Do not flag this as destructive or suggest narrowing it to specific E2EE keys in future reviews.
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
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.
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
.changeset/honest-plants-heal.md.changeset/tame-falcons-watch.md
📚 Learning: 2026-03-14T14:58:58.834Z
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Applied to files:
.changeset/honest-plants-heal.md.changeset/tame-falcons-watch.md
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
.changeset/honest-plants-heal.md.changeset/tame-falcons-watch.md
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
.changeset/honest-plants-heal.md.changeset/tame-falcons-watch.md
📚 Learning: 2026-04-10T21:17:22.932Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40096
File: apps/meteor/ee/server/apps/lib/redactor.ts:3-17
Timestamp: 2026-04-10T21:17:22.932Z
Learning: In RocketChat/Rocket.Chat, `X-User-Id` / `x-user-id` headers must NOT be added to redaction paths in apps log redaction (e.g., `apps/meteor/ee/server/apps/lib/redactor.ts`). The maintainer (d-gubert) has confirmed that X-User-Id is an identifier, not a credential — its presence in logs is useful for diagnostics, and `X-Auth-Token` is the only header that constitutes a real secret. Do not suggest redacting X-User-Id in future reviews of this area.
Applied to files:
.changeset/honest-plants-heal.md
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.
Applied to files:
.changeset/honest-plants-heal.md.changeset/tame-falcons-watch.md
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
.changeset/tame-falcons-watch.md
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
.changeset/tame-falcons-watch.md
📚 Learning: 2026-04-27T16:19:02.889Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 40324
File: apps/meteor/client/providers/ServerProvider.tsx:20-20
Timestamp: 2026-04-27T16:19:02.889Z
Learning: In RocketChat/Rocket.Chat, `ServerContextValue.absoluteUrl` in `packages/ui-contexts/src/ServerContext.ts` intentionally keeps the more restrictive signature `(path: string) => string`, even though the underlying `absoluteUrl` helper in `apps/meteor/client/lib/absoluteUrl.ts` accepts optional `path` and `options` parameters. Do not suggest widening the context type to match the helper's full signature.
Applied to files:
.changeset/tame-falcons-watch.md
📚 Learning: 2026-04-14T21:10:36.233Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36292
File: apps/meteor/client/hooks/useHasValidLocationHash.ts:7-12
Timestamp: 2026-04-14T21:10:36.233Z
Learning: In the RocketChat/Rocket.Chat repository, adding JSDoc-style comments to React hooks (e.g., files under apps/meteor/client/hooks/) is considered a good pattern and should not be flagged as a violation of the "avoid code comments in the implementation" guideline. The guideline against code comments does not apply to JSDoc documentation on exported hooks.
Applied to files:
.changeset/tame-falcons-watch.md
Comment on lines
+1
to
+5
| --- | ||
| '@rocket.chat/meteor': patch | ||
| --- | ||
|
|
||
| Remove oauthRedirectUri override (dead code after Meteor OAuth stack removal) |
Contributor
There was a problem hiding this comment.
Make the changeset note more explicit (non-functional + reference).
The front matter looks correct, but the body is extremely brief. Please consider expanding it to explicitly state this is non-breaking / no functional changes, and optionally reference the relevant issue/PR (e.g., #40346 / PR #40354) so release notes are self-contained and auditable later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.changeset/honest-plants-heal.md around lines 1 - 5, Update the changeset
body to be explicit that this is a non-functional, non-breaking change by adding
a sentence like "This change only removes a dead-code override
(oauthRedirectUri) and does not affect runtime behavior or public APIs." Also
include a reference to the related issue/PR (e.g., "#40346 / PR `#40354`") for
traceability; keep the existing front-matter ('@rocket.chat/meteor': patch)
unchanged and ensure the markdown remains concise and audit-friendly.
Comment on lines
+1
to
+5
| --- | ||
| '@rocket.chat/meteor': patch | ||
| --- | ||
|
|
||
| Remove useCorsSSLConfig hook and absoluteUrl startup patch (dead code after Meteor DDP removal) |
Contributor
There was a problem hiding this comment.
Update changeset description to match the full PR scope.
This changeset body only mentions removing useCorsSSLConfig and the absoluteUrl startup patch, but the PR summary/objectives also include deleting the oauthRedirectUri override. To avoid incomplete/misleading release notes, update line 5 to mention oauthRedirectUri as well (or otherwise ensure the text matches exactly what this PR removes).
Suggested change
-Remove useCorsSSLConfig hook and absoluteUrl startup patch (dead code after Meteor DDP removal)
+Remove dead Meteor OAuth/DDP compatibility patches: `oauthRedirectUri` override, `useCorsSSLConfig` hook, and `absoluteUrl` startup patch📝 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
| --- | |
| '@rocket.chat/meteor': patch | |
| --- | |
| Remove useCorsSSLConfig hook and absoluteUrl startup patch (dead code after Meteor DDP removal) | |
| --- | |
| '@rocket.chat/meteor': patch | |
| --- | |
| Remove dead Meteor OAuth/DDP compatibility patches: `oauthRedirectUri` override, `useCorsSSLConfig` hook, and `absoluteUrl` startup patch |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.changeset/tame-falcons-watch.md around lines 1 - 5, The changeset message
only lists removing useCorsSSLConfig hook and the absoluteUrl startup patch but
omits deletion of the oauthRedirectUri override; update the
.changeset/tame-falcons-watch.md body so the description explicitly lists
oauthRedirectUri alongside useCorsSSLConfig and the absoluteUrl startup patch
(or reword it to cover all three removals) to ensure release notes match the PR
scope and summary.
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.
Remove the oauthRedirectUri override — it monkey-patched meteor/oauth solely for backwards compatibility with pre-2.3 clients. Once Meteor's OAuth stack is removed, this file has no purpose. The file contains a TODO comment explicitly marking it for removal.
apps/meteor/client/meteor/overrides/oauthRedirectUri.tsapps/meteor/client/meteor/overrides/index.tsCloses #40346
No functional changes — this is dead code removal. Verify no remaining references by running:
grep -r "oauthRedirectUri" apps/meteor/client/
Straightforward cleanup. No behavior changes expected.
Summary by CodeRabbit