fix: add rowGap to FilterByText to fix spacing on mobile view#39963
Open
adwait50 wants to merge 3 commits into
Open
fix: add rowGap to FilterByText to fix spacing on mobile view#39963adwait50 wants to merge 3 commits into
adwait50 wants to merge 3 commits into
Conversation
… is enabled When Accounts_ManuallyApproveNewUsers is enabled, new users created via custom OAuth (e.g. Authentik, generic OIDC) were left with active: false because the custom OAuth path never explicitly set user.active before insertion, unlike SAML which handles this correctly. The onCreateUserAsync hook only sets active if user.active is undefined, so by setting it explicitly in the validateNewUser callback we ensure OAuth users get the correct active status matching the setting. Fixes RocketChat#38722
When the screen shrinks to mobile/tablet view, the filter dropdowns
wrap onto new lines but had no vertical spacing between rows, making
the marketplace filters and search bar look congested.
Added rowGap={4} to the FilterByText wrapper Box so wrapped rows
have consistent spacing on smaller screens.
Fixes RocketChat#38613
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 |
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdded an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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. 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: 1
🧹 Nitpick comments (1)
apps/meteor/app/custom-oauth/server/custom_oauth_server.js (1)
474-477: Remove inline implementation comments from this block.These comments should live in the PR description/commit message instead of implementation code.
Proposed cleanup
- // Ensure OAuth users respect the ManuallyApproveNewUsers setting. - // Without this, new OAuth users are created with active: false (pending) - // because onCreateUserAsync only sets active if user.active is undefined, - // and the custom OAuth path never sets it explicitly (unlike SAML). if (user.active === undefined) { user.active = !settings.get('Accounts_ManuallyApproveNewUsers'); }As per coding guidelines, "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/custom-oauth/server/custom_oauth_server.js` around lines 474 - 477, Remove the inline implementation comments in the oauth-manual-approval block in custom_oauth_server.js: delete the three-line explanatory comment that mentions ManuallyApproveNewUsers, onCreateUserAsync, and SAML so implementation code contains no developer-facing rationale; move this explanation to the PR description/commit message instead and leave only necessary code or a minimal one-line comment if absolutely required by style rules.
🤖 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/components/FilterByText.tsx`:
- Line 26: The Box in the FilterByText component uses an unsupported direct prop
rowGap; update the Box JSX (the element rendering the form in FilterByText.tsx)
to remove the rowGap prop and pass it inside the style prop instead (e.g.,
style={{ rowGap: '4px' }}) while keeping mb, mi, is='form',
onSubmit={handleFormSubmit}, display, flexWrap and alignItems unchanged so the
layout behavior is preserved.
---
Nitpick comments:
In `@apps/meteor/app/custom-oauth/server/custom_oauth_server.js`:
- Around line 474-477: Remove the inline implementation comments in the
oauth-manual-approval block in custom_oauth_server.js: delete the three-line
explanatory comment that mentions ManuallyApproveNewUsers, onCreateUserAsync,
and SAML so implementation code contains no developer-facing rationale; move
this explanation to the PR description/commit message instead and leave only
necessary code or a minimal one-line comment if absolutely required by style
rules.
🪄 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: 8b41c8ea-0890-4318-bf6c-52a3085539c6
📒 Files selected for processing (2)
apps/meteor/app/custom-oauth/server/custom_oauth_server.jsapps/meteor/client/components/FilterByText.tsx
📜 Review details
🧰 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/components/FilterByText.tsxapps/meteor/app/custom-oauth/server/custom_oauth_server.js
🧠 Learnings (6)
📓 Common learnings
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.
📚 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/components/FilterByText.tsx
📚 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/components/FilterByText.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/app/custom-oauth/server/custom_oauth_server.js
📚 Learning: 2026-03-15T14:31:23.493Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:891-899
Timestamp: 2026-03-15T14:31:23.493Z
Learning: In RocketChat/Rocket.Chat, `IUser.inactiveReason` in `packages/core-typings/src/IUser.ts` is typed as `'deactivated' | 'pending_approval' | 'idle_too_long'` (optional, no `null`), but the database stores `null` for newly created users. The Typia-generated `$ref: '#/components/schemas/IUser'` schema therefore correctly rejects `null` for `inactiveReason`. This causes the test "should create a new user with default roles" to fail when response validation is active (TEST_MODE). The fix is to add `| null` to `inactiveReason` in core-typings and rebuild Typia schemas in a separate PR. Do not flag this test failure as a bug introduced by the users.create OpenAPI migration (PR `#39647`). Do not suggest inlining a custom schema to work around it, as migration rules require using `$ref` when a Typia schema exists.
Applied to files:
apps/meteor/app/custom-oauth/server/custom_oauth_server.js
📚 Learning: 2026-03-09T23:46:52.173Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39492
File: apps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts:22-24
Timestamp: 2026-03-09T23:46:52.173Z
Learning: In `apps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts`, the `oAuth2ServerAuth` function's `authorization` field in `partialRequest` is exclusively expected to carry Bearer tokens. Basic authentication is not supported in this OAuth flow, so there is no need to guard against non-Bearer schemes when extracting the token from the `Authorization` header.
Applied to files:
apps/meteor/app/custom-oauth/server/custom_oauth_server.js
🔇 Additional comments (2)
apps/meteor/app/custom-oauth/server/custom_oauth_server.js (1)
469-480: Please confirm this auth-path change belongs in this PR.The stated PR objective is a Marketplace mobile spacing fix, but this block changes OAuth new-user activation behavior. Since this is security-sensitive account-creation logic, it should be explicitly justified here or moved to a dedicated PR for safer review and rollback.
apps/meteor/client/components/FilterByText.tsx (1)
1-1: AI summary describes unrelated changes not present in this file.The AI-generated summary mentions OAuth user validation changes (
Accounts.validateNewUserguard) that are not present in this file. Only theFilterByTextlayout update withrowGap={4}is visible here.
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.
Description
When the screen shrinks to mobile/tablet view, the filter dropdowns
in the Marketplace page wrap onto new lines but had no vertical
spacing between rows, making filters and search bar look congested.
Root Cause
FilterByText.tsxusesflexWrap='wrap'on the wrapperBoxbuthad no
rowGapset, so wrapped rows had zero vertical spacing.Fix
Added
rowGap={4}to the wrapperBoxinFilterByText.tsx.Fixes #38613
Summary by CodeRabbit
Bug Fixes
Style