fix: adding validation when adding link to message composer#37573
fix: adding validation when adding link to message composer#37573divyanshu-patil wants to merge 13 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 |
|
|
|
WalkthroughURL validation was added to the message composer link modal: the form now validates required http/https URLs on change, shows inline error messages, and disables confirm until valid. Translation keys for the validation messages were added in en, es, hi-IN, and pt-BR. Changes
Sequence DiagramsequenceDiagram
actor User
participant Modal as AddLinkComposerActionModal
participant Form as React Hook Form
participant Validator as URL Pattern Check
participant UI as FieldError / Confirm Button
User->>Modal: Open modal & type URL
Modal->>Form: onChange -> update field value
Form->>Validator: Validate required + http/https pattern
alt Valid URL
Validator-->>Form: validation passed
Form-->>Modal: formState.isValid = true
Modal-->>UI: enable Confirm
else Invalid or empty
Validator-->>Form: validation failed (error message)
Form-->>Modal: formState.isValid = false
Modal-->>UI: show FieldError (translated message) & disable Confirm
end
User->>Modal: Click Confirm (only if enabled)
Modal->>Modal: Submit validated URL
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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: 4
🧹 Nitpick comments (3)
packages/i18n/src/locales/en.i18n.json (1)
5369-5369: Refactor is incomplete: code reference must also be updated.The suggested i18n key rename in
packages/i18n/src/locales/en.i18n.jsonis paired with a code reference atapps/meteor/app/ui-message/client/messageBox/AddLinkComposerActionModal.tsx:62that must be updated in tandem:- message: t("URL_must_start_with_'http://'_or_'https://'"), + message: t("URL_must_start_with_http_or_https"),Renaming only the i18n key without updating this reference will break the translation lookup. If this refactor proceeds, both changes are required together. Per learnings,
en.i18n.jsonis the only locale file to edit; others are managed by the translation pipeline.apps/meteor/app/ui-message/client/messageBox/AddLinkComposerActionModal.tsx (2)
56-70: URL regex behavior vs. error message—small UX mismatchThe pattern
^https?:\/\/.+$both enforceshttp:///https://and requires at least one non-newline character after://. That means an input likehttp://orhttps://will fail with the message “URL must start with 'http://' or 'https://'”, even though it does start that way. Also, the regex is case‑sensitive, soHTTPS://example.comwould be rejected.If you want stricter validation (no bare
http://) this is fine, but it might be worth either:
- Relaxing the pattern (e.g., allowing
*after://) or- Tweaking the message to reflect that a full URL (not just the scheme) is required and optionally documenting case sensitivity.
Up to you whether to adjust this now or leave as‑is.
1-1: HookFieldErrorto the URL input viaaria-describedbyfor accessibilityRight now the
FieldErroris rendered but not associated with the URL input, so screen readers may not announce the error when the field is focused. Given past ARIA issues fixed elsewhere in the app, it’d be good to wire this similarly. For example:- const urlField = useId(); + const urlField = useId(); + const urlFieldErrorId = `${urlField}-error`; ... - <Controller - control={control} - name='url' - rules={...} - render={({ field }) => <TextInput autoComplete='off' id={urlField} {...field} />} - /> + <Controller + control={control} + name='url' + rules={...} + render={({ field }) => ( + <TextInput + autoComplete='off' + id={urlField} + aria-describedby={urlFieldErrorId} + {...field} + /> + )} + /> ... - <FieldError>{formState.errors.url?.message}</FieldError> + <FieldError id={urlFieldErrorId}>{formState.errors.url?.message}</FieldError>(Assuming
FieldErrorforwardsidto the underlying element.) This keeps the label/field wiring you already have and makes the error text properly discoverable to assistive tech. Based on learnings.Also applies to: 54-56, 72-72
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/meteor/app/ui-message/client/messageBox/AddLinkComposerActionModal.tsx(4 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)packages/i18n/src/locales/es.i18n.json(1 hunks)packages/i18n/src/locales/hi-IN.i18n.json(2 hunks)packages/i18n/src/locales/pt-BR.i18n.json(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-18T17:32:33.969Z
Learnt from: aleksandernsilva
Repo: RocketChat/Rocket.Chat PR: 36974
File: apps/meteor/client/components/Omnichannel/OutboundMessage/components/OutboundMessageWizard/forms/MessageForm/MessageForm.tsx:124-129
Timestamp: 2025-09-18T17:32:33.969Z
Learning: The ARIA mismatch issue in MessageForm's template field (where FieldError id used templateId instead of messageFormId) was addressed in PR #36972 through refactoring the template field into a separate TemplateField component, which uses consistent templateFieldId for both aria-describedby and FieldError id.
Applied to files:
apps/meteor/app/ui-message/client/messageBox/AddLinkComposerActionModal.tsx
📚 Learning: 2025-11-19T12:32:29.673Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.673Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
packages/i18n/src/locales/es.i18n.jsonpackages/i18n/src/locales/pt-BR.i18n.jsonpackages/i18n/src/locales/en.i18n.jsonpackages/i18n/src/locales/hi-IN.i18n.json
🔇 Additional comments (3)
packages/i18n/src/locales/pt-BR.i18n.json (1)
5217-5218: Fix Portuguese grammar; key renaming requires code coordination.The PT-BR translation has a gender agreement error: "URL é obrigatório" should be "A URL é obrigatória" (URL is feminine in Portuguese).
If renaming the keys to avoid quotes (e.g.,
URL_must_start_with_http_or_https), coordinate:
- Rename in
packages/i18n/src/locales/en.i18n.json(lines 5368–5369)- Update code references in
apps/meteor/app/ui-message/client/messageBox/AddLinkComposerActionModal.tsx(lines 62, 66)- Other locales will update via the translation pipeline
⛔ Skipped due to learnings
Learnt from: d-gubert Repo: RocketChat/Rocket.Chat PR: 37547 File: packages/i18n/src/locales/en.i18n.json:634-634 Timestamp: 2025-11-19T12:32:29.673Z Learning: Repo: RocketChat/Rocket.Chat Context: i18n workflow Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.Learnt from: gabriellsh Repo: RocketChat/Rocket.Chat PR: 37419 File: packages/i18n/src/locales/en.i18n.json:918-921 Timestamp: 2025-11-19T18:20:07.683Z Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.Learnt from: gabriellsh Repo: RocketChat/Rocket.Chat PR: 37505 File: packages/i18n/src/locales/en.i18n.json:3765-3765 Timestamp: 2025-11-17T22:38:48.631Z Learning: Rocket.Chat i18n copy: Keep sentence case for the value of "Notification_Desktop_show_voice_calls" in packages/i18n/src/locales/en.i18n.json (“Show desktop notifications for voice calls”) per design directive; do not change to Title Case even if nearby labels differ.packages/i18n/src/locales/en.i18n.json (1)
5368-5368: Key looks good and consistent.“URL_is_required” follows existing patterns like “Email_is_required”. No changes needed.
apps/meteor/app/ui-message/client/messageBox/AddLinkComposerActionModal.tsx (1)
18-20: Form validity wiring withreact-hook-formis soundUsing
mode: 'onChange'andformState.isValidto driveconfirmDisabledgives immediate feedback whilehandleSubmitstill guards submission, so the Add button only activates when the URL passes validation. This looks correct and consistent with the stated UX goals.Also applies to: 43-43
| "URL_is_required": "La URL es obligatoria", | ||
| "URL_must_start_with_'http://'_or_'https://'": "La URL debe comenzar con 'http://' o 'https://'", |
There was a problem hiding this comment.
🧩 Analysis chain
Add new i18n keys only in English and avoid special characters in key names
- In this repo, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locales are populated via the external translation pipeline. Please remove the Spanish additions here and keep only the English source keys. Based on learnings
- Also, the key "URL_must_start_with_'http://'or'https://'" contains quotes and URL text in the key name, which is non‑idiomatic and risky for tooling. Prefer a slug like URL_must_start_with_http_or_https.
Apply this removal in es.i18n.json (translations will come via the pipeline):
- "URL_is_required": "La URL es obligatoria",
- "URL_must_start_with_'http://'_or_'https://'": "La URL debe comenzar con 'http://' o 'https://'",And in en.i18n.json, define stable keys such as:
- URL_is_required: "URL is required"
- URL_must_start_with_http_or_https: "URL must start with http:// or https://"
Can you confirm the translation pipeline is active for this branch so Spanish (and other locales) will sync automatically after adding the keys to en.i18n.json?
🏁 Script executed:
#!/bin/bash
# Check which locale files were modified in this PR
git diff --name-only HEAD~1 HEAD | grep -E 'i18n\.json' | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the full diff for both Spanish and English locale files
git diff HEAD~1 HEAD -- 'packages/i18n/src/locales/en.i18n.json' 'packages/i18n/src/locales/es.i18n.json' | head -100Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for existing keys with special characters (quotes, slashes) in any locale file
rg -n "\"[^\"]*['\\/][^\"]*\"\\s*:" packages/i18n/src/locales/ | head -20Length of output: 95
🏁 Script executed:
#!/bin/bash
# Check if URL-related keys exist anywhere in en.i18n.json
rg -n "URL" packages/i18n/src/locales/en.i18n.jsonLength of output: 18522
Remove Spanish translations—keys already exist in English and will sync via translation pipeline
The keys URL_is_required and URL_must_start_with_'http://'_or_'https://' already exist in packages/i18n/src/locales/en.i18n.json. Per the repo's i18n workflow, new translations should only be added to the English locale file; other locales are populated via the external translation pipeline. Remove these entries from es.i18n.json:
- "URL_is_required": "La URL es obligatoria",
- "URL_must_start_with_'http://'_or_'https://'": "La URL debe comenzar con 'http://' o 'https://'",The Spanish translations will be generated automatically once you confirm these keys are finalized in en.i18n.json.
📝 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.
| "URL_is_required": "La URL es obligatoria", | |
| "URL_must_start_with_'http://'_or_'https://'": "La URL debe comenzar con 'http://' o 'https://'", |
🤖 Prompt for AI Agents
In packages/i18n/src/locales/es.i18n.json around lines 3894-3895, remove the two
Spanish entries "URL_is_required" and
"URL_must_start_with_'http://'_or_'https://'" because these keys must be
maintained only in the English locale and other locales are populated by the
translation pipeline; ensure the keys remain present and correct in
packages/i18n/src/locales/en.i18n.json, save the file, and commit the deletion
(do not add or modify these keys in other locale files).
| "Test_Connection": "परीक्षण कनेक्शन", | ||
| "Test_Desktop_Notifications": "डेस्कटॉप सूचनाओं का परीक्षण करें", | ||
| "Test_LDAP_Search": "एलडीएपी खोज का परीक्षण करें", | ||
| "Text": "टेक्स्ट", |
There was a problem hiding this comment.
Do not add keys to non-English locale files (i18n workflow).
Per this repo’s i18n process, new keys should be added only to packages/i18n/src/locales/en.i18n.json; other locales are populated via the external translation pipeline and/or fall back to English. Please revert this addition in hi-IN and keep the source key in en only. Based on learnings
Apply: remove "Text" from hi-IN and rely on fallback until it lands via the translation pipeline.
🤖 Prompt for AI Agents
packages/i18n/src/locales/hi-IN.i18n.json around line 4456: The key "Text" was
added directly to the Hindi locale but per the repo i18n workflow new keys must
be added only to packages/i18n/src/locales/en.i18n.json and other locales should
wait for the translation pipeline or fallback to English; remove the "Text" key
entry from hi-IN.i18n.json (revert this addition) so the file matches the
canonical workflow and rely on fallback until the translation lands via the
pipeline.
| "URL_is_required": "यूआरएल आवश्यक है", | ||
| "URL_must_start_with_'http://'_or_'https://'": "यूआरएल की शुरुआत 'http://' या 'https://' से होनी चाहिए।", |
There was a problem hiding this comment.
🧩 Analysis chain
URL validation strings should not be added in hi-IN; keep only in en and let the pipeline translate.
- Move/remove these from hi-IN; add them (or keep them) in en.i18n.json only. Based on learnings
- Optional: prefer a more stable key (e.g., URL_must_start_with_scheme) with placeholders in the value instead of embedding 'http://' in the key.
To verify duplication and key placement across locales, run:
🏁 Script executed:
#!/bin/bash
# Check where the new keys appear
rg -nP --glob 'packages/i18n/src/locales/*.json' \
-e '"URL_is_required"\s*:' \
-e '"URL_must_start_with_\\'http://\\'_or_\\'https://\\'"\s*:' \
-e '"Text"\s*:'
# Ensure only en.i18n.json contains the new URL validation keys
rg -nP --glob 'packages/i18n/src/locales/*.json' '"URL_is_required"\s*:' | grep -v '/en.i18n.json' && echo "Found outside en (should be moved)" || echo "OK: only in en"
rg -nP --glob 'packages/i18n/src/locales/*.json' '"URL_must_start_with_\\'http://\\'_or_\\'https://\\'"\s*:' | grep -v '/en.i18n.json' && echo "Found outside en (should be moved)" || echo "OK: only in en"Length of output: 1051
Remove URL validation keys from hi-IN; keep only in en.i18n.json and let the translation pipeline handle other locales.
Per the i18n workflow, new translation keys should only be added to packages/i18n/src/locales/en.i18n.json. The verification confirms "URL_is_required" is currently in multiple locales (es, pt-BR, hi-IN, and en) when it should only be in en. Remove these keys from hi-IN.i18n.json at lines 4651–4652.
🤖 Prompt for AI Agents
In packages/i18n/src/locales/hi-IN.i18n.json around lines 4651–4652, remove the
two URL validation keys ("URL_is_required" and
"URL_must_start_with_'http://'_or_'https://'") so they exist only in
en.i18n.json; after deleting those two entries, ensure the surrounding JSON
remains valid (remove or adjust the trailing comma on the preceding or following
line as needed).
| "URL_is_required": "URL é obrigatório", | ||
| "URL_must_start_with_'http://'_or_'https://'": "A URL deve começar com 'http://' ou 'https://'", |
There was a problem hiding this comment.
Do not add new keys to non-English locales; revert these pt-BR insertions.
In this repo, new i18n keys must be introduced only in packages/i18n/src/locales/en.i18n.json. Other locales are populated via the external translation pipeline and fall back to English meanwhile. Please remove these additions from pt-BR and keep the keys only in en.i18n.json. Based on learnings
Apply this removal here:
- "URL_is_required": "URL é obrigatório",
- "URL_must_start_with_'http://'_or_'https://'": "A URL deve começar com 'http://' ou 'https://'",📝 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.
| "URL_is_required": "URL é obrigatório", | |
| "URL_must_start_with_'http://'_or_'https://'": "A URL deve começar com 'http://' ou 'https://'", |
🤖 Prompt for AI Agents
packages/i18n/src/locales/pt-BR.i18n.json lines 5217-5218: these two new keys
("URL_is_required" and "URL_must_start_with_'http://' _or_'https://'") were
wrongly added to the pt-BR locale; remove these entries from this file so pt-BR
stays unchanged and only en.i18n.json contains new keys. Ensure you delete both
key/value lines and leave surrounding JSON valid (no trailing commas), and
confirm the corresponding keys exist in packages/i18n/src/locales/en.i18n.json.
2 participants
Proposed changes (including videos or screenshots)
Issue(s)
closes #37572
Steps to test or reproduce
affected screens
Further comments
Summary by CodeRabbit
New Features
Localization
✏️ Tip: You can customize this high-level summary in your review settings.