fix(markdown): ensure custom style overrides precede theme colors#7441
fix(markdown): ensure custom style overrides precede theme colors#7441Shevilll wants to merge 4 commits into
Conversation
Validate the username field against the server's UTF8_User_Names_Validation setting (with the default pattern as a fallback) before submitting the profile. This prevents invalid characters such as spaces from reaching the server, which previously surfaced only as a generic save error, and shows an inline message explaining the allowed characters. Closes RocketChat#1682
Drop the stale isMasterDetail destructured from useAppSelector (the selector no longer returns it) so only the useMasterDetail() hook declares it, fixing the no-redeclare lint/build failure introduced by merging develop. Also annotate isValidUsername with an explicit boolean return type per the TS guideline.
WalkthroughMarkdown inline renderers now let optional ChangesMarkdown textStyle precedence
Profile username validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/containers/markdown/Markdown.textStyle.test.tsx`:
- Around line 64-67: The assertions in Markdown.textStyle.test.tsx only check
that a red style exists somewhere in the style array, so they do not protect the
override order in the Markdown rendering path. Update the expectations around
plainTextNode, linkNode, mentionNode, and hashtagNode to verify the effective
resolved color or that the textStyle override is applied in the correct
precedence relative to the internal theme style, using the relevant props.style
on the rendered nodes.
In `@app/i18n/locales/en.json`:
- Line 1005: The Username_invalid locale copy is too specific and can be
misleading when the server uses a custom UTF8_User_Names_Validation pattern.
Update the en.json entry for Username_invalid to a generic validation message
that does not list character rules, so it applies to both the fallback and
server-configured username regex. Keep the key unchanged and only revise the
wording in this locale string.
In `@app/views/ProfileView/index.tsx`:
- Around line 98-101: The username schema in ProfileView’s validation currently
always runs the UTF8_User_Names_Validation test, which blocks saving unrelated
profile changes for legacy or locked usernames. Update the yup rule in the
ProfileView form schema so the `valid-username` test is skipped when the
username is unchanged or when `Accounts_AllowUsernameChange` is false, and keep
the check only for editable username changes during submit validation.
🪄 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: 9c416ec7-c963-4e93-86f1-ae4b0a21c7cf
📒 Files selected for processing (7)
app/containers/markdown/Markdown.textStyle.test.tsxapp/containers/markdown/components/inline/Link.tsxapp/containers/markdown/components/mentions/AtMention.tsxapp/containers/markdown/components/mentions/Hashtag.tsxapp/i18n/locales/en.jsonapp/views/ProfileView/index.test.tsxapp/views/ProfileView/index.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/containers/markdown/Markdown.textStyle.test.tsxapp/containers/markdown/components/inline/Link.tsxapp/views/ProfileView/index.test.tsxapp/containers/markdown/components/mentions/Hashtag.tsxapp/containers/markdown/components/mentions/AtMention.tsxapp/views/ProfileView/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode enabled
Files:
app/containers/markdown/Markdown.textStyle.test.tsxapp/containers/markdown/components/inline/Link.tsxapp/views/ProfileView/index.test.tsxapp/containers/markdown/components/mentions/Hashtag.tsxapp/containers/markdown/components/mentions/AtMention.tsxapp/views/ProfileView/index.tsx
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses
Files:
app/containers/markdown/Markdown.textStyle.test.tsxapp/i18n/locales/en.jsonapp/containers/markdown/components/inline/Link.tsxapp/views/ProfileView/index.test.tsxapp/containers/markdown/components/mentions/Hashtag.tsxapp/containers/markdown/components/mentions/AtMention.tsxapp/views/ProfileView/index.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce ESLint rules from
@rocket.chat/eslint-configwith React, React Native, TypeScript, and Jest plugins
Files:
app/containers/markdown/Markdown.textStyle.test.tsxapp/containers/markdown/components/inline/Link.tsxapp/views/ProfileView/index.test.tsxapp/containers/markdown/components/mentions/Hashtag.tsxapp/containers/markdown/components/mentions/AtMention.tsxapp/views/ProfileView/index.tsx
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place reusable UI components in 'app/containers/' directory
Files:
app/containers/markdown/Markdown.textStyle.test.tsxapp/containers/markdown/components/inline/Link.tsxapp/containers/markdown/components/mentions/Hashtag.tsxapp/containers/markdown/components/mentions/AtMention.tsx
app/i18n/**/*.{ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Place internationalization (i18n) configuration in 'app/i18n/' directory with support for 40+ locales and RTL
Files:
app/i18n/locales/en.json
app/views/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place screen components in 'app/views/' directory
Files:
app/views/ProfileView/index.test.tsxapp/views/ProfileView/index.tsx
🧠 Learnings (2)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/containers/markdown/Markdown.textStyle.test.tsxapp/containers/markdown/components/inline/Link.tsxapp/views/ProfileView/index.test.tsxapp/containers/markdown/components/mentions/Hashtag.tsxapp/containers/markdown/components/mentions/AtMention.tsxapp/views/ProfileView/index.tsx
📚 Learning: 2026-06-24T22:58:43.390Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7157
File: app/views/MessagesView/index.tsx:392-392
Timestamp: 2026-06-24T22:58:43.390Z
Learning: When wrapping a React Native component (e.g., via `withSafeAreaInsets`) ensure `hoistNonReactStatics` is only required if the wrapped component actually defines static properties/methods that consumers rely on. If the component has no statics (as in `app/views/MessagesView/index.tsx`), you can omit `hoistNonReactStatics` for this case.
Applied to files:
app/views/ProfileView/index.test.tsxapp/views/ProfileView/index.tsx
🔇 Additional comments (4)
app/containers/markdown/components/inline/Link.tsx (1)
52-52: LGTM!app/containers/markdown/components/mentions/AtMention.tsx (1)
31-34: LGTM!Also applies to: 79-79, 88-88
app/containers/markdown/components/mentions/Hashtag.tsx (1)
60-71: LGTM!app/views/ProfileView/index.test.tsx (1)
12-12: LGTM!Also applies to: 73-74, 115-134
| expect(plainTextNode.props.style).toEqual(expect.arrayContaining([expect.objectContaining({ color: 'red' })])); | ||
| expect(linkNode.props.style).toEqual(expect.arrayContaining([expect.objectContaining({ color: 'red' })])); | ||
| expect(mentionNode.props.style).toEqual(expect.arrayContaining([expect.objectContaining({ color: 'red' })])); | ||
| expect(hashtagNode.props.style).toEqual(expect.arrayContaining([expect.objectContaining({ color: 'red' })])); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
Assert the resolved color priority, not just presence in the style array.
The current arrayContaining check only proves that a red color object appears somewhere in props.style. It does not verify that the passed textStyle takes visual precedence over the internal theme color. If the order were regressed (e.g., textStyle applied before the theme), the test would still pass while the component renders the wrong color. Refine the assertions to verify the effective resolved color or the specific position of the override object to ensure the fix is actually protected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/containers/markdown/Markdown.textStyle.test.tsx` around lines 64 - 67,
The assertions in Markdown.textStyle.test.tsx only check that a red style exists
somewhere in the style array, so they do not protect the override order in the
Markdown rendering path. Update the expectations around plainTextNode, linkNode,
mentionNode, and hashtagNode to verify the effective resolved color or that the
textStyle override is applied in the correct precedence relative to the internal
theme style, using the relevant props.style on the rendered nodes.
| "User_not_found_or": "User not found or incorrect password", | ||
| "User_sent_an_attachment": "{{user}} sent an attachment", | ||
| "Username": "Username", | ||
| "Username_invalid": "Use only letters, numbers, dots, hyphens and underscores", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make this error message generic.
Validation now follows the server-configured UTF8_User_Names_Validation, so this copy is only correct for the default fallback pattern. On workspaces with a custom username regex, the app will show misleading guidance.
Suggested fix
- "Username_invalid": "Use only letters, numbers, dots, hyphens and underscores",
+ "Username_invalid": "Enter a valid username",📝 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.
| "Username_invalid": "Use only letters, numbers, dots, hyphens and underscores", | |
| "Username_invalid": "Enter a valid username", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/i18n/locales/en.json` at line 1005, The Username_invalid locale copy is
too specific and can be misleading when the server uses a custom
UTF8_User_Names_Validation pattern. Update the en.json entry for
Username_invalid to a generic validation message that does not list character
rules, so it applies to both the fallback and server-configured username regex.
Keep the key unchanged and only revise the wording in this locale string.
| username: yup | ||
| .string() | ||
| .required(I18n.t('Username_required')) | ||
| .test('valid-username', I18n.t('Username_invalid'), value => isValidUsername(value ?? '', UTF8_User_Names_Validation)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Skip regex validation for unchanged or locked usernames.
This test now runs on every submit, so a legacy username that no longer matches UTF8_User_Names_Validation will block saving unrelated profile changes. That can happen even when Accounts_AllowUsernameChange is false, because validateFormInfo() validates the whole schema before submit.
Suggested fix
username: yup
.string()
.required(I18n.t('Username_required'))
- .test('valid-username', I18n.t('Username_invalid'), value => isValidUsername(value ?? '', UTF8_User_Names_Validation))
+ .test('valid-username', I18n.t('Username_invalid'), value => {
+ const nextUsername = value ?? '';
+ if (!Accounts_AllowUsernameChange || nextUsername === (user?.username ?? '')) {
+ return true;
+ }
+ return isValidUsername(nextUsername, UTF8_User_Names_Validation);
+ })📝 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.
| username: yup | |
| .string() | |
| .required(I18n.t('Username_required')) | |
| .test('valid-username', I18n.t('Username_invalid'), value => isValidUsername(value ?? '', UTF8_User_Names_Validation)) | |
| username: yup | |
| .string() | |
| .required(I18n.t('Username_required')) | |
| .test('valid-username', I18n.t('Username_invalid'), value => { | |
| const nextUsername = value ?? ''; | |
| if (!Accounts_AllowUsernameChange || nextUsername === (user?.username ?? '')) { | |
| return true; | |
| } | |
| return isValidUsername(nextUsername, UTF8_User_Names_Validation); | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/views/ProfileView/index.tsx` around lines 98 - 101, The username schema
in ProfileView’s validation currently always runs the UTF8_User_Names_Validation
test, which blocks saving unrelated profile changes for legacy or locked
usernames. Update the yup rule in the ProfileView form schema so the
`valid-username` test is skipped when the username is unchanged or when
`Accounts_AllowUsernameChange` is false, and keep the check only for editable
username changes during submit validation.
1 participant
Summary
This PR resolves the issue where custom text styles/colors rendering on formatted links, mentions, and hashtags inside Markdown components were being overwritten by default/theme-specific colors (Issue #7035).
Root Cause
In React Native, properties from style objects or arrays that appear later in a style array override earlier ones. Within
Link.tsx,AtMention.tsx, andHashtag.tsx, the conditional array spread oftextStyle(which propagates custom style overrides, such as custom font sizes or colors from parent components) was applied before the theme color object. Because of this ordering, default/theme-specific colors (e.g.,fontInfo,statusFontService,fontDefault) were overriding any custom colors supplied viatextStyle.Solution
Rearranged the style array elements to place the default/theme colors before
textStylespread. This ensures:textStylecorrectly override theme colors.Additionally, a comprehensive integration test case was added in
Markdown.textStyle.test.tsxto verify custom color propagation and prevent future regressions.Summary by CodeRabbit
New Features
Bug Fixes