refactor(invite): decouple global session state management side-effects from useValidateInviteQuery hook#39954
Open
thekishandev wants to merge 1 commit intoRocketChat:developfrom
Conversation
…ts from useValidateInviteQuery hook
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 |
|
Contributor
WalkthroughThis change decouples global session state management side-effects from the invite validation query hook. The session state update logic—based on registration form settings—is moved from 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}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the invite flow to remove global session/UI side-effects from the useValidateInviteQuery data-fetching hook, relocating that behavior into the InvitePage component to better align with React Query separation-of-concerns.
Changes:
- Removed
useSessionDispatch('loginDefaultState')anduseSetting('Accounts_RegistrationForm')usage fromuseValidateInviteQuery. - Added an
InvitePageeffect intended to setloginDefaultStateafter invite validation succeeds. - Removed the stale FIXME comment tied to the previous architecture.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/meteor/client/views/invite/hooks/useValidateInviteQuery.ts | Removes global session/setting side-effects from the query function, keeping it focused on validation + toast on failure. |
| apps/meteor/client/views/invite/InvitePage.tsx | Adds session dispatch logic in the UI layer tied to the invite query lifecycle. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+27
to
+35
| useEffect(() => { | ||
| if (validateInvite.isSuccess && validateInvite.data && token) { | ||
| if (registrationForm !== 'Disabled') { | ||
| setLoginDefaultState('invite-register'); | ||
| } else { | ||
| setLoginDefaultState('login'); | ||
| } | ||
| } | ||
| }, [validateInvite.isSuccess, validateInvite.data, token, registrationForm, setLoginDefaultState]); |
There was a problem hiding this comment.
setLoginDefaultState is dispatched in a useEffect, which runs after <LoginPage /> (and thus RegistrationPageRouter) has already mounted. RegistrationPageRouter reads useSession('loginDefaultState') only to initialize useLoginRouter via useState(...), so updates to the session after mount won't change the active route. This can cause the invite flow to still land on the default login route instead of invite-register. Consider passing defaultRoute into <LoginPage /> based on registrationForm/invite validity (or otherwise ensure the session value is set before RegistrationPageRouter mounts).
![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: 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 `@apps/meteor/client/views/invite/InvitePage.tsx`:
- Around line 27-35: When validateInvite succeeds the effect sets session state
asynchronously causing a flash of LoginPage; make the session decision
synchronous or gate rendering: inside InvitePage, derive a local desiredDefault
= validateInvite.isSuccess && validateInvite.data && token && registrationForm
!== 'Disabled' ? 'invite-register' : 'login' and pass that into
RegistrationPageRouter or useSession setter before rendering (or render a
short-loading placeholder until setLoginDefaultState has been applied) so
RegistrationPageRouter never reads an undefined/old loginDefaultState; refer to
validateInvite, setLoginDefaultState, InvitePage, LoginPage,
RegistrationPageRouter and useSession when implementing the change.
🪄 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: cfe9ce65-2539-49d8-b8cb-9176b6144405
📒 Files selected for processing (2)
apps/meteor/client/views/invite/InvitePage.tsxapps/meteor/client/views/invite/hooks/useValidateInviteQuery.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). (2)
- GitHub Check: Agent
- 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:
apps/meteor/client/views/invite/InvitePage.tsxapps/meteor/client/views/invite/hooks/useValidateInviteQuery.ts
🧠 Learnings (13)
📓 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.
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/client/views/invite/InvitePage.tsxapps/meteor/client/views/invite/hooks/useValidateInviteQuery.ts
📚 Learning: 2026-03-18T16:08:17.800Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39590
File: apps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsx:97-99
Timestamp: 2026-03-18T16:08:17.800Z
Learning: In `apps/meteor/client/views/omnichannel/contactInfo/EditContactInfo.tsx`, `reValidateMode: 'onBlur'` is intentionally used (not 'onChange') because the `validateEmailFormat` and `validatePhone` functions are async and call the `checkExistenceEndpoint` API to check for duplicates. Using 'onChange' would trigger excessive network requests on every keystroke. The combination of `mode: 'onSubmit'` with `reValidateMode: 'onBlur'` is a deliberate design decision to minimize API calls while still providing revalidation feedback.
Applied to files:
apps/meteor/client/views/invite/InvitePage.tsx
📚 Learning: 2026-03-15T14:31:28.969Z
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.
Applied to files:
apps/meteor/client/views/invite/InvitePage.tsxapps/meteor/client/views/invite/hooks/useValidateInviteQuery.ts
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/views/invite/InvitePage.tsx
📚 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:
apps/meteor/client/views/invite/hooks/useValidateInviteQuery.ts
📚 Learning: 2026-03-16T23:33:15.721Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:15.721Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs (e.g., PR `#39676` for users.register in apps/meteor/app/api/server/v1/users.ts), calls to `this.parseJsonQuery()` inside migrated handlers are intentionally preserved without adding a corresponding `query` AJV schema to the route options. Adding query-param schemas for the `fields`/`sort`/`query` parameters consumed by `parseJsonQuery()` is a separate cross-cutting concern shared by many endpoints (e.g., users.create, users.update, users.list) and is explicitly out of scope for individual endpoint migration PRs. Do not flag the absence of a `query` schema for `parseJsonQuery()` usage as a violation of OpenAPI/AJV contract during migration reviews.
Applied to files:
apps/meteor/client/views/invite/hooks/useValidateInviteQuery.ts
📚 Learning: 2026-03-16T21:50:42.118Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Applied to files:
apps/meteor/client/views/invite/hooks/useValidateInviteQuery.ts
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/client/views/invite/hooks/useValidateInviteQuery.ts
📚 Learning: 2026-03-11T16:46:55.955Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39535
File: apps/meteor/app/apps/server/bridges/livechat.ts:249-249
Timestamp: 2026-03-11T16:46:55.955Z
Learning: In `apps/meteor/app/apps/server/bridges/livechat.ts`, `createVisitor()` intentionally does not propagate `externalIds` to `registerData`. This is by design: the method is deprecated (JSDoc: `deprecated Use createAndReturnVisitor instead. Note: This method does not support externalIds.`) to push callers toward `createAndReturnVisitor()`, which does support `externalIds`. Do not flag the missing `externalIds` field in `createVisitor()` as a bug.
Applied to files:
apps/meteor/client/views/invite/hooks/useValidateInviteQuery.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/views/invite/hooks/useValidateInviteQuery.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:
apps/meteor/client/views/invite/hooks/useValidateInviteQuery.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:
apps/meteor/client/views/invite/hooks/useValidateInviteQuery.ts
🔇 Additional comments (2)
apps/meteor/client/views/invite/hooks/useValidateInviteQuery.ts (1)
1-34: Clean separation of concerns achieved.The hook now performs pure data-fetching without side effects. The removal of
useSessionDispatchanduseSettingaligns with the PR objective to decouple state management from the query layer.apps/meteor/client/views/invite/InvitePage.tsx (1)
53-55: VerifyvalidateInvite.datatype handling.The condition
validateInvite.dataevaluates tofalsewhen the query returnsfalse(invalid token or error). This correctly prevents rendering<LoginPage />for invalid tokens. The logic aligns with the hook's return values.
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
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
Extracts global UI session side-effects from the data-fetching layer, closing out an outstanding architectural technical debt marker.
Root Cause / Problem
Currently,
useValidateInviteQuerywas dispatching global session state side-effects (setLoginDefaultState) directly inside its asynchronousqueryFn. This violates typical React Query patterns by strictly entangling data-fetching with volatile UI state mutation.Solution / Behavior
useSessionDispatchanduseSettingfrom theuseValidateInviteQueryfetch logic.setLoginDefaultStatedispatch execution directly into the consuming UI component (InvitePage.tsx), binding it correctly to the query'sisSuccesslifecycle via a native ReactuseEffect.// FIXME: decouple this state managementcomment since the architecture is now correctly decoupled.Issue(s)
Closes #39953
Validation & Testing
yarn lintlocally (0 errors)Type of change
Summary by CodeRabbit