fix: App slash commands show UUID keys instead of descriptions#7072
fix: App slash commands show UUID keys instead of descriptions#7072divyanshu-patil wants to merge 5 commits intoRocketChat:developfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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)
WalkthroughAdds app translation support: new WatermelonDB table and model, REST endpoint type, a fetch-and-cache method, automatic fetching on login/language change, and translation lookup in slash-command autocomplete with fallbacks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginSaga as Login Saga
participant SDK
participant DB as WatermelonDB
participant Composer as MessageComposer
User->>LoginSaga: successful login / language change
LoginSaga->>SDK: GET apps.translations?language=xx
SDK-->>LoginSaga: { language, translations }
LoginSaga->>DB: write transaction (delete old rows, create new app_translations)
DB-->>LoginSaga: ack
User->>Composer: types "/"
Composer->>DB: query app_translations where language=xx and key=description_key
DB-->>Composer: translation record or none
Composer-->>User: display command list with resolved subtitles (translated or fallback)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 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 |
divyanshu-patil
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/containers/MessageComposer/hooks/useAutocomplete.ts`:
- Around line 155-161: In the useAutocomplete.ts branch handling command.appId,
change the lookup to first query appTranslationsCollection using the full
I18n.currentLocale() (the same locale key used when saving in
app/lib/methods/getSlashCommands.ts) and only if that returns no records, fall
back to querying with the stripped base language (appLang =
I18n.currentLocale().split('-')[0]); update the logic around variables
translationRecords and appLang so the primary fetch uses I18n.currentLocale()
and the secondary uses the base language to ensure saved translations like
"en-US" or "pt-BR" are found before falling back to the generic language key.
In `@app/lib/methods/getAppTranslations.ts`:
- Around line 42-51: The current logic in getAppTranslations uses
collection.query(Q.where('language', language)).fetchCount() and returns as soon
as any row exists, making app_translations write-once and never refreshed;
change this to detect staleness instead of presence: replace the
fetchCount-based early return with either (a) always call
fetchAndSaveTranslations(language, db) so translations are refreshed on each
sync, or (b) fetch the existing rows (collection.query(...).fetch()) and inspect
a freshness/version/timestamp field (e.g., updated_at or app_version) to decide
when to call fetchAndSaveTranslations; ensure fetchAndSaveTranslations performs
upserts (not inserts-only) so it can update existing records when new data
arrives.
In `@app/lib/methods/getSlashCommands.ts`:
- Around line 23-25: The prefetch of translations via
getAppTranslations(I18n.currentLocale()) must not abort the whole slash-command
sync if it fails; wrap the getAppTranslations call in its own try/catch (or
attach a .catch) and log or ignore the error so it does not propagate to the
outer catch that handles command persistence, then continue with the existing
slash-command sync/persistence logic (the code that persists slash commands
after the translation call). Ensure you reference getAppTranslations and the
surrounding slash-command sync/persistence block so only the translation fetch
is isolated.
- Around line 24-25: getSlashCommands.ts calls getAppTranslations with
I18n.currentLocale() (e.g. "en-US") which mismatches LanguageView's
base-language keys (e.g. "en"), causing cache misses; change the call to
normalize the locale to the base language via LanguageView before calling
getAppTranslations (e.g. derive base language from I18n.currentLocale() using
LanguageView’s helper or mapping and pass that base value to getAppTranslations)
so keys align with LanguageView and avoid duplication/unresolved descriptions.
In `@app/views/LanguageView/index.tsx`:
- Around line 86-88: The catch currently groups getAppTranslations() failures
with saveUserPreferences() failures; split these operations so translation fetch
errors are handled separately: call saveUserPreferences(...) and await it in its
own try/catch (or let it propagate to the existing success/failure flow), then
call getAppTranslations(appLang) in a separate try/catch block and handle/log
translation-specific errors (using the params.language-derived appLang and
getAppTranslations) without triggering the “saving preferences” error path or
telemetry for saveUserPreferences.
🪄 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: 716a7016-bd26-431e-964e-8918b5b6cea7
📒 Files selected for processing (10)
app/containers/MessageComposer/hooks/useAutocomplete.tsapp/definitions/rest/v1/appTranslations.tsapp/definitions/rest/v1/index.tsapp/lib/database/index.tsapp/lib/database/model/AppTranslation.tsapp/lib/database/model/migrations.jsapp/lib/database/schema/app.jsapp/lib/methods/getAppTranslations.tsapp/lib/methods/getSlashCommands.tsapp/views/LanguageView/index.tsx
📜 Review details
🔇 Additional comments (6)
app/definitions/rest/v1/appTranslations.ts (1)
1-8: Endpoint typing looks consistent with the new translations flow.
This definition cleanly exposes theapps.translationscontract for callers.app/definitions/rest/v1/index.ts (1)
24-24:Endpointscomposition update is correct.
The new endpoint group is properly registered in the global REST type map.Also applies to: 48-49
app/lib/database/model/AppTranslation.ts (1)
4-10: Model definition aligns with the new DB table contract.
tablename and fields match the migration/schema introduced in this PR.app/lib/database/model/migrations.js (1)
349-360: Migration step is well-formed and versioned correctly.
The new table creation matches the intended translation cache persistence flow.app/lib/database/index.ts (1)
28-28: Database model registration is correctly integrated.
AppTranslationis now available to WatermelonDB queries/writes in app scope.Also applies to: 64-65
app/lib/database/schema/app.js (1)
4-4: Schema update is consistent with migration/model changes.
Version bump andapp_translationstable definition are in sync.Also applies to: 289-296
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 `@app/sagas/login.js`:
- Around line 188-195: The fetchAppTranslationsFork currently reads locale from
I18n.currentLocale() before the user's preference is applied; change
fetchAppTranslationsFork to accept a language parameter (e.g.,
fetchAppTranslationsFork(lang)) and use that value when calling
getAppTranslations (falling back to I18n.currentLocale().split('-')[0] if lang
is falsy). Then update the place that forks it (the fork call near where
setLanguage(user?.language) is invoked) to pass user?.language so translations
are fetched using the stored user preference rather than the pre-update I18n
locale.
🪄 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: 57587415-9c62-4ef6-92ea-8df913596d72
📒 Files selected for processing (3)
app/lib/methods/getAppTranslations.tsapp/lib/methods/getSlashCommands.tsapp/sagas/login.js
✅ Files skipped from review due to trivial changes (1)
- app/lib/methods/getSlashCommands.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/lib/methods/getAppTranslations.ts
📜 Review details
🔇 Additional comments (2)
app/sagas/login.js (2)
42-42: LGTM!Import follows the existing pattern and correctly references the
getAppTranslationsmethod.
264-264: Fork placement follows established pattern.The fork is correctly placed alongside other data-fetching operations. The graceful fallback in
useAutocomplete.ts(per context snippet 3) handles cases where translations haven't arrived yet by showing a readable form of the description key.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/sagas/login.js (1)
180-188:⚠️ Potential issue | 🟠 MajorLanguage may be captured before user preference is applied.
fetchSlashCommandsForkis forked at line 256, butsetLanguage(user?.language)executes at line 264. At line 182,I18n.currentLocale()may not reflect the user's stored preference yet, causing incorrect translations to be fetched and cached.Pass the user's language directly to avoid the race:
Proposed fix
-const fetchSlashCommandsFork = function* fetchSlashCommandsFork() { +const fetchSlashCommandsFork = function* fetchSlashCommandsFork(userLanguage) { try { - const appLang = I18n.currentLocale().split('-')[0]; + const appLang = (userLanguage || I18n.currentLocale()).split('-')[0]; yield getAppTranslations(appLang); yield getSlashCommands(); } catch (e) { log(e); } };Then update the fork call at line 256:
- yield fork(fetchSlashCommandsFork); + yield fork(fetchSlashCommandsFork, user?.language);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/sagas/login.js` around lines 180 - 188, fetchSlashCommandsFork currently reads I18n.currentLocale() which can race with setLanguage(user?.language); change fetchSlashCommandsFork to accept a language parameter (e.g., fetchSlashCommandsFork(language)) and use that to call getAppTranslations(language) before getSlashCommands, then update the place where it's forked (the fork that currently calls fetchSlashCommandsFork) to pass the user's stored language (user?.language or a safe fallback) after setLanguage is determined; keep function name fetchSlashCommandsFork and calls to getAppTranslations and getSlashCommands unchanged besides using the passed-in language.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/sagas/login.js`:
- Around line 180-188: fetchSlashCommandsFork currently reads
I18n.currentLocale() which can race with setLanguage(user?.language); change
fetchSlashCommandsFork to accept a language parameter (e.g.,
fetchSlashCommandsFork(language)) and use that to call
getAppTranslations(language) before getSlashCommands, then update the place
where it's forked (the fork that currently calls fetchSlashCommandsFork) to pass
the user's stored language (user?.language or a safe fallback) after setLanguage
is determined; keep function name fetchSlashCommandsFork and calls to
getAppTranslations and getSlashCommands unchanged besides using the passed-in
language.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55944ced-26f1-4178-9e77-a651faed65cb
📒 Files selected for processing (1)
app/sagas/login.js
📜 Review details
🔇 Additional comments (1)
app/sagas/login.js (1)
42-42: LGTM!Import is correctly added for the new
getAppTranslationsmethod.
divyanshu-patil
force-pushed
the
fix/commands-description
branch
from
7af6f12 to
9b5517c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/sagas/login.js (1)
188-195:⚠️ Potential issue | 🟠 MajorFetch App translations using the user’s language, not pre-login locale.
At Line 190, language is read from
I18n.currentLocale()before Line 272 appliessetLanguage(user?.language). On devices where default locale differs from profile locale, translations are fetched under the wrong language and autocomplete lookup can miss.Proposed fix
-const fetchAppTranslationsFork = function* fetchAppTranslationsFork() { +const fetchAppTranslationsFork = function* fetchAppTranslationsFork(userLanguage) { try { - const appLang = I18n.currentLocale().split('-')[0]; + const appLang = (userLanguage || I18n.currentLocale()).split('-')[0]; yield getAppTranslations(appLang); } catch (e) { log(e); } }; @@ - yield fork(fetchAppTranslationsFork); + yield fork(fetchAppTranslationsFork, user?.language);Also applies to: 263-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/sagas/login.js` around lines 188 - 195, fetchAppTranslationsFork currently reads language from I18n.currentLocale() (causing translations to be fetched before the logged-in user's language is applied); change fetchAppTranslationsFork (and the other occurrence using I18n.currentLocale()) to use the authenticated user's language instead—e.g., read user?.language from the auth state (or accept a language param) and call getAppTranslations(userLanguage) after setLanguage(user?.language) has run; ensure getAppTranslations is invoked with that user language rather than I18n.currentLocale().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/sagas/login.js`:
- Around line 188-195: fetchAppTranslationsFork currently reads language from
I18n.currentLocale() (causing translations to be fetched before the logged-in
user's language is applied); change fetchAppTranslationsFork (and the other
occurrence using I18n.currentLocale()) to use the authenticated user's language
instead—e.g., read user?.language from the auth state (or accept a language
param) and call getAppTranslations(userLanguage) after
setLanguage(user?.language) has run; ensure getAppTranslations is invoked with
that user language rather than I18n.currentLocale().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f0ae6e98-62da-447d-91ad-00cce477c973
📒 Files selected for processing (1)
app/sagas/login.js
📜 Review details
🔇 Additional comments (1)
app/sagas/login.js (1)
42-42: Import wiring looks good.
getAppTranslationsis correctly imported and used by the new login-success fork flow.
1 participant
Proposed changes
Slash commands registered by Rocket.Chat Apps (e.g. Jitsi, Giphy) have their descriptions stored as dynamic i18n keys in the format
app-<uuid>.command_description. These keys are injected at runtime on the web client viaTAPi18nextbut were never exposed to the React Native client, causing the raw translation key to be displayed instead of the human-readable description.This PR introduces a new server-side REST endpoint
(GET /api/v1/apps.translations)that aggregates translation strings from all installed and enabled Apps, and a client-side pipeline that fetches, persists, and resolves these translations in WatermelonDB.Depends on
RocketChat/Rocket.Chat#39959
Issue(s)
closes #7071
How to test or reproduce
/Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit