feat: WatchOS Quick Replies#6957
Open
divyanshu-patil wants to merge 55 commits into
Open
Conversation
Contributor
|
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:
WalkthroughAdds WatchOS quick replies: i18n keys and default setting, JS helpers and preferences for per-server storage and sync, a TurboModule + iOS implementation using MMKV and WCSession, Watch App data model and UI updates, and RN navigation/UI to manage and synchronize quick replies. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RNApp as React Native App
participant JS as JS State/Preferences
participant Bridge as TurboModule NativeWatchModule
participant iOS as iOS RCTWatchModule
participant Watch as Apple Watch (WCSession)
User->>RNApp: Add or manage quick replies
RNApp->>JS: persist replies per-server (UserPreferences.setArray)
RNApp->>JS: request sync (syncWatchOSQuickReplies)
JS->>Bridge: NativeWatchModule.syncQuickReplies()
Bridge->>iOS: invoke syncQuickReplies
iOS->>iOS: read MMKV currentServer & quickReplies, build payload
iOS->>Watch: updateApplicationContext (WCSession)
Watch-->>iOS: acknowledgement
iOS-->>Bridge: return success/error
Bridge-->>JS: propagate result
JS-->>RNApp: update UI status
sequenceDiagram
participant LoginFlow
participant Saga as login saga
participant State as App State
participant JS as sync helper
participant Bridge as TurboModule
LoginFlow->>Saga: login success
Saga->>State: read settings
Saga->>JS: syncWatchOSQuickRepliesWithServer(state)
JS->>Bridge: NativeWatchModule.syncQuickReplies()
Bridge-->>JS: return result
JS-->>Saga: sync result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
diegolmello
requested changes
Jan 30, 2026
Co-authored-by: Diego Mello <diegolmello@gmail.com>
Added comments for clarity on JSON conversion and error handling.
![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: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/views/UserPreferencesView/index.tsx (1)
37-41:⚠️ Potential issue | 🟠 Major
navigateToScreenalways logsUP_GO_USER_NOTIFICATION_PREF— wrong analytics event for WatchOS navigation.Every call to
navigateToScreenfires the notification-preference event regardless of the target. The new WatchOS Quick Replies item (line 123) will misattribute its navigation tap to the notification-preferences event in analytics. Either pass the event as a parameter, or calllogEventdirectly at each call site.♻️ Suggested fix
-const navigateToScreen = (screen: keyof ProfileStackParamList) => { - logEvent(events.UP_GO_USER_NOTIFICATION_PREF); - // `@ts-ignore` - navigation.navigate(screen); -}; +const navigateToScreen = (screen: keyof ProfileStackParamList, event?: string) => { + if (event) logEvent(event); + // `@ts-ignore` + navigation.navigate(screen); +};Then at the WatchOS call site:
-onPress={() => navigateToScreen('UserWatchOSQuickRepliesView')} +onPress={() => navigateToScreen('UserWatchOSQuickRepliesView', events.UP_GO_WATCHOS_QUICK_REPLIES)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/UserPreferencesView/index.tsx` around lines 37 - 41, navigateToScreen currently always logs events.UP_GO_USER_NOTIFICATION_PREF which misattributes analytics; change navigateToScreen to accept an analytics event parameter (e.g., event: string | undefined) or remove the log from it and instead call logEvent at each call site; update the WatchOS Quick Replies call site to pass the correct event or call logEvent(events.UP_GO_USER_WATCH_QUICK_REPLIES) directly before calling navigateToScreen (references: navigateToScreen function and the WatchOS Quick Replies navigation call).
🟡 Minor comments (9)
ios/RocketChat Watch App/Database/Entity/Server.swift-19-19 (1)
19-19:⚠️ Potential issue | 🟡 MinorTrailing semicolon (SwiftLint warning) and mixed indentation.
Line 38 has a trailing semicolon flagged by SwiftLint (
trailing_semicolon). Lines 19 and 32–40 also use 4-space indentation while the rest of the file uses tabs, which will cause noise in diffs and may fail CI linting.🔧 Proposed fix
- `@NSManaged` private var quickRepliesData: NSArray? + `@NSManaged` private var quickRepliesData: NSArray? ... - public var quickReplies: [String] { - get { - quickRepliesData as? [String] ?? [] - } - set { - quickRepliesData = newValue as NSArray; - } - } + public var quickReplies: [String] { + get { + quickRepliesData as? [String] ?? [] + } + set { + quickRepliesData = newValue as NSArray + } + }Also applies to: 32-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RocketChat` Watch App/Database/Entity/Server.swift at line 19, In Server.swift, remove the trailing semicolon and fix mixed indentation: change the property declaration for quickRepliesData (`@NSManaged` private var quickRepliesData: NSArray?) and the subsequent block around the other properties/methods in the Server class (the lines noted in the review) to use tabs instead of 4-space indentation and delete the stray semicolon found on the offending line (the SwiftLint trailing_semicolon). Ensure consistent tab indentation across the Server class so linting passes.app/lib/methods/getSettings.ts-26-26 (1)
26-26:⚠️ Potential issue | 🟡 Minor
Apple_Watch_Quick_ActionsinserverInfoKeyshas no matching handler inserverInfoUpdate— the entry is dead code.
serverInfoKeysfilters settings passed toserverInfoUpdate. ThereduceinsideserverInfoUpdate(lines 57–97) has no branch forApple_Watch_Quick_Actions, so the value is silently discarded there. The setting is still dispatched to Redux viaparsedSettingsregardless ofserverInfoKeysmembership, so adding it here has no functional effect.Either add a handler in
serverInfoUpdateif the intent is to persist this value to the server record, or remove it fromserverInfoKeysto avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/methods/getSettings.ts` at line 26, serverInfoKeys contains 'Apple_Watch_Quick_Actions' but serverInfoUpdate's reducer (function serverInfoUpdate) has no case for that key so the value is dropped; either remove 'Apple_Watch_Quick_Actions' from serverInfoKeys or add a handler in serverInfoUpdate's reduce to persist it to the server record. If you choose to persist, add a branch inside serverInfoUpdate's reduce that checks for 'Apple_Watch_Quick_Actions' and assigns the incoming value to the appropriate server info field on the accumulator (matching how other keys like 'Force_SSL' or 'Unsafe_Audio_Video' are handled) so it is included in the returned server update; if you choose to remove, delete 'Apple_Watch_Quick_Actions' from serverInfoKeys to avoid dead code.ios/RocketChat Watch App/Views/MessageComposerView.swift-33-34 (1)
33-34:⚠️ Potential issue | 🟡 Minor
ForEachidentity can collide when replies repeat.Line [33] uses
id: \.self; duplicate quick replies create non-unique IDs and unstable updates.Proposed fix
- ForEach(quickReplies, id: \.self) { reply in + ForEach(Array(quickReplies.enumerated()), id: \.offset) { _, reply in🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RocketChat` Watch App/Views/MessageComposerView.swift around lines 33 - 34, ForEach is using id: \.self which breaks when quickReplies contains duplicate strings; change the identity to a stable unique key (e.g., if Reply has an id property use ForEach(quickReplies, id: \.id) { reply in ... } or iterate indices like ForEach(quickReplies.indices, id: \.self) { idx in let reply = quickReplies[idx] ... } so each row has a stable, unique ID; update references inside the loop from reply to the indexed value if you choose the indices approach.app/views/UserWatchOSQuickRepliesView/index.tsx-58-60 (1)
58-60:⚠️ Potential issue | 🟡 MinorNormalize admin quick-reply seed values before saving.
Line [59] can introduce empty/whitespace replies from comma-separated config.
Proposed fix
- const replies = appleWatchReplies.split(','); + const replies = appleWatchReplies + .split(',') + .map(reply => reply.trim()) + .filter(Boolean); setQuickReplies(replies);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/UserWatchOSQuickRepliesView/index.tsx` around lines 58 - 60, When initializing quick replies in UserWatchOSQuickRepliesView, normalize the comma-separated admin seed (appleWatchReplies) before calling setQuickReplies to avoid empty or whitespace-only entries: split appleWatchReplies by comma, map each piece with trim(), filter out items with zero length (or falsy), and then pass the cleaned array to setQuickReplies so no blank replies are saved.app/lib/methods/userPreferences.ts-152-156 (1)
152-156:⚠️ Potential issue | 🟡 MinorValidate parsed payload is actually an array before casting.
Line [155] casts any JSON shape to
T[], which can surface runtime errors later.Proposed fix
getArray<T = unknown>(key: string): T[] | null { try { const jsonString = this.mmkv.getString(key); - return jsonString ? (JSON.parse(jsonString) as T[]) : null; + if (!jsonString) { + return null; + } + const parsed = JSON.parse(jsonString); + return Array.isArray(parsed) ? (parsed as T[]) : null; } catch { return null; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/methods/userPreferences.ts` around lines 152 - 156, The getArray method currently parses JSON and blindly casts to T[]; change getArray(key: string) to parse jsonString into a local variable (e.g., parsed), then verify Array.isArray(parsed) before casting and returning (return null or handle as invalid when not an array); ensure this uses the existing mmkv.getString(key) result and keep the try/catch, optionally logging a warning when the payload is not an array to aid debugging.app/lib/methods/WatchOSQuickReplies/syncWatchOSRepliesWithServer.ts-12-21 (1)
12-21:⚠️ Potential issue | 🟡 MinorNormalize quick replies before persistence to avoid blank entries.
Line [16] stores raw comma-split values; trailing commas or spaced values become empty/dirty entries.
Proposed fix
if (isFirstLogin && appleWatchReplies && typeof appleWatchReplies === 'string') { - if (server && appleWatchReplies) { - const quickRepliesMMKVKey = `${server}-${WATCHOS_QUICKREPLIES}`; - - const replies = appleWatchReplies.split(','); - UserPreferences.setArray(quickRepliesMMKVKey, replies); - } + const quickRepliesMMKVKey = `${server}-${WATCHOS_QUICKREPLIES}`; + const replies = appleWatchReplies + .split(',') + .map(reply => reply.trim()) + .filter(Boolean); + UserPreferences.setArray(quickRepliesMMKVKey, replies); } - if (server) { - syncWatchOSQuickReplies(); - } + syncWatchOSQuickReplies();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/methods/WatchOSQuickReplies/syncWatchOSRepliesWithServer.ts` around lines 12 - 21, The code stores raw comma-split appleWatchReplies which can create empty or whitespace-only entries; before calling UserPreferences.setArray with quickRepliesMMKVKey in syncWatchOSRepliesWithServer (where appleWatchReplies is processed and WATCHOS_QUICKREPLIES/quickRepliesMMKVKey is used), normalize the replies by splitting on ',' then trimming each item, filtering out empty strings (and optionally removing duplicates) and only pass the cleaned array to UserPreferences.setArray; keep the surrounding guards (isFirstLogin, server, typeof appleWatchReplies === 'string') intact and then call syncWatchOSQuickReplies as before.app/views/UserWatchOSQuickRepliesView/index.tsx-22-23 (1)
22-23:⚠️ Potential issue | 🟡 MinorAvoid writing preferences under a
null-*server key.If
currentServeris null, Line [23] stores data undernull-RC_WATCHOS_QUICKREPLIES, which can bleed data across sessions.Proposed fix
const currentServer = userPreferences.getString(CURRENT_SERVER); - const [quickReplies, setQuickReplies] = useUserPreferences<string[]>(`${currentServer}-${WATCHOS_QUICKREPLIES}`, []); + const quickRepliesKey = currentServer ? `${currentServer}-${WATCHOS_QUICKREPLIES}` : WATCHOS_QUICKREPLIES; + const [quickReplies, setQuickReplies] = useUserPreferences<string[]>(quickRepliesKey, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/UserWatchOSQuickRepliesView/index.tsx` around lines 22 - 23, The code constructs the preference key using currentServer which may be null, causing keys like "null-RC_WATCHOS_QUICKREPLIES"; fix by guarding the key construction before calling useUserPreferences: obtain currentServer via userPreferences.getString(CURRENT_SERVER), then build a safe key (e.g., if currentServer is truthy use `${currentServer}-${WATCHOS_QUICKREPLIES}` else use just `WATCHOS_QUICKREPLIES` or a stable default) and pass that safeKey to useUserPreferences<string[]>(safeKey, []); update references to currentServer/ WATCHOS_QUICKREPLIES/ useUserPreferences so preferences are never stored under a "null-*" prefix.app/lib/native/NativeWatchModule.ts-10-13 (1)
10-13:⚠️ Potential issue | 🟡 MinorDebug-only methods exposed in the production TurboModule interface.
getCurrentServerFromNative,getkey, andgetRepliesare annotated as// debug. Shipping these in the production bridge increases the attack surface (they expose raw MMKV keys and server URLs to JS). Consider gating them behind__DEV__or removing them before merge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/native/NativeWatchModule.ts` around lines 10 - 13, The interface exposes debug-only methods getCurrentServerFromNative, getkey, and getReplies on NativeWatchModule which should not be available in production; remove these methods from the production TurboModule interface or conditionally include them only in dev builds (e.g., guard their declarations/exports behind a __DEV__ check or a build-time flag) so that getCurrentServerFromNative, getkey, and getReplies are not present on the production bridge.ios/RCTWatchModule.mm-13-15 (1)
13-15:⚠️ Potential issue | 🟡 MinorFile-scope global variables without
staticrisk linker symbol collisions.
mmkvQuickRepliesKeyandmmkvCurrentServerKeyare declared at file scope withoutstatic, so they have external linkage. If another translation unit declares identically named globals, you'll get a duplicate-symbol linker error.Proposed fix
-NSString *mmkvQuickRepliesKey = @"RC_WATCHOS_QUICKREPLIES"; -NSString *mmkvCurrentServerKey = @"currentServer"; +static NSString *const mmkvQuickRepliesKey = @"RC_WATCHOS_QUICKREPLIES"; +static NSString *const mmkvCurrentServerKey = @"currentServer";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RCTWatchModule.mm` around lines 13 - 15, The two file-scope globals mmkvQuickRepliesKey and mmkvCurrentServerKey currently have external linkage and can cause duplicate-symbol linker errors; change their declarations to have internal linkage and be immutable by making them static and const (e.g., use static NSString * const for mmkvQuickRepliesKey and mmkvCurrentServerKey) so they are private to this translation unit and cannot be modified externally.
🧹 Nitpick comments (5)
app/stacks/InsideStack.tsx (1)
175-175: GuardUserWatchOSQuickRepliesViewscreen registration to iOS.The component calls
checkWatch()on mount (line 36), which directly invokesNativeWatchModulemethods (isWatchSupported(),isWatchPaired(),isWatchAppInstalled()) without error handling. SinceNativeWatchModuleis iOS-only, Android users who reach this screen (via deep link or navigation) will crash. Add a Platform.OS check to the screen registration or provide a no-op fallback for non-iOS platforms.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/stacks/InsideStack.tsx` at line 175, The UserWatchOSQuickRepliesView screen must be guarded for iOS because its component calls checkWatch() which uses NativeWatchModule (isWatchSupported(), isWatchPaired(), isWatchAppInstalled()) and will crash on Android; update the ProfileStack registration to only register UserWatchOSQuickRepliesView when Platform.OS === 'ios' (or register a safe no-op stub component for non-iOS) so navigation/deeplinks won't load the real component on Android, and alternatively add a defensive Platform.OS check inside UserWatchOSQuickRepliesView's checkWatch() as a secondary safeguard.app/views/UserWatchOSQuickRepliesView/index.tsx (1)
73-75: Prefer a stable, content-derived key over index-only for chips.Using index keys on editable lists can cause chip reuse glitches after deletion/reorder.
Proposed fix
- {quickReplies.map((reply, index) => ( - <Chip key={index} text={reply} onPress={() => removeQuickReply(reply)} /> + {quickReplies.map((reply, index) => ( + <Chip key={`${reply}-${index}`} text={reply} onPress={() => removeQuickReply(reply)} /> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/UserWatchOSQuickRepliesView/index.tsx` around lines 73 - 75, The list uses an unstable index key in quickReplies.map which can cause UI reuse glitches; change the key on the Chip inside quickReplies.map to a stable, content-derived identifier (e.g., use the reply string or a unique id property if replies are objects) instead of index — update the map in UserWatchOSQuickRepliesView to set key={reply} or key={reply.id} and ensure removeQuickReply and any state updates continue to operate on that same identifier.ios/RCTWatchModule.mm (1)
37-42:getMMKVallocates a newMMKVBridgeinstance on every call.This method is invoked from
getValueFromMMKV:, which is called multiple times withinsyncQuickReplies,getkey,getReplies, etc. Repeatedly allocating the bridge object is wasteful and may carry initialization overhead. Cache it as an instance variable initialized once ininit.Proposed fix
`@interface` RCTWatchModule () `@property`(nonatomic, strong) WCSession *session; +@property(nonatomic, strong) MMKVBridge *mmkv; `@end` ... - (id)init { if (self = [super init]) { _session = WCSession.defaultSession; + _mmkv = [[MMKVBridge alloc] initWithID:@"default" + cryptKey:nil + rootPath:nil]; } return self; } -- (MMKVBridge *)getMMKV { - MMKVBridge *mmkv = [[MMKVBridge alloc] initWithID:@"default" - cryptKey:nil - rootPath:nil]; - return mmkv; -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RCTWatchModule.mm` around lines 37 - 42, getMMKV currently creates a new MMKVBridge on every call (used by getValueFromMMKV, syncQuickReplies, getkey, getReplies), causing repeated initialization overhead; instead allocate and store a single MMKVBridge instance as an ivar/property when the module is initialized (in init) and have getMMKV return that cached instance; update the class to declare a MMKVBridge *cachedMMKV (or similar), initialize it in init (alloc/initWithID:cryptKey:rootPath:), and change getMMKV to return cachedMMKV (and handle nil-safety if initialization can fail).app/sagas/selectServer.ts (2)
201-207: Loose equality== nullis intentional but add a brief comment for clarity.The logic is correct: on first encounter the key is absent (
getBoolreturnsnull/undefined), so you persistfalse(consumed) and dispatchtrue. On subsequent launches it readsfalseand dispatchesfalse. However, the stored value beingfalseto mean "first start already happened" is counter-intuitive. A short inline comment would help future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/sagas/selectServer.ts` around lines 201 - 207, Add a short inline comment above the isFirstServerLogin check explaining that the loose null check (UserPreferences.getBool(`${server}-${FIRST_START}`) == null) intentionally detects an absent key, that we then persist false to mark the first-start as consumed via UserPreferences.setBool(`${server}-${FIRST_START}`, false), and that stored false therefore means "not first start" while dispatching setIsFirstServerLogin(true) for the initial run; reference the symbols UserPreferences.getBool, UserPreferences.setBool, FIRST_START, and setIsFirstServerLogin when adding the comment so future readers understand the inverse meaning of the stored boolean and why == null is used.
276-278: Selecting the entire Redux state into a saga is a code smell.
yield* appSelector(s => s)pulls the full state tree into the saga. This coupleshandleServerFinishAddto every slice of the store. Prefer selecting only the fieldssyncWatchOSQuickRepliesWithServeractually needs (e.g.,state.server,state.app,state.settings).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/sagas/selectServer.ts` around lines 276 - 278, The saga handleServerFinishAdd currently selects the entire Redux state via appSelector(s => s); change it to only select the specific slices needed by syncWatchOSQuickRepliesWithServer (for example select state.server, state.app and state.settings) instead of the full tree—either call appSelector to return an object with only those keys or make separate selectors for server/app/settings and yield them individually so handleServerFinishAdd no longer pulls the whole state.
🤖 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/lib/methods/WatchOSQuickReplies/getWatchStatus.ts`:
- Around line 9-15: checkWatch currently calls NativeWatchModule methods
directly and can throw if the native module is missing; wrap the body of
checkWatch (the calls to NativeWatchModule.isWatchSupported, isWatchPaired,
isWatchAppInstalled) in a try-catch and return a safe fallback IWatchStatus
(e.g., all false) on error, logging or swallowing the exception consistently
with syncReplies.ts; update checkWatch to return the fallback IWatchStatus when
NativeWatchModule access fails so the view cannot crash.
In `@app/lib/methods/WatchOSQuickReplies/syncReplies.ts`:
- Around line 7-17: Remove the console.log lines that print NativeWatchModule
payloads (calls to syncQuickReplies, isWatchSupported, isWatchPaired,
isWatchAppInstalled, getCurrentServerFromNative, getReplies, getkey) so
sensitive watch data is not written to device logs; instead call
NativeWatchModule.syncQuickReplies() and capture its result (or await it if
async), validate the result and return or propagate that real outcome (e.g.,
return the boolean/result or throw an error when sync indicates failure) rather
than always returning true in the try block; keep the catch block but ensure it
rethrows or returns a failing result after logging so callers receive the real
sync failure.
In `@app/lib/native/NativeWatchModule.ts`:
- Around line 4-14: The Spec methods getCurrentServerFromNative, getkey, and
getReplies are declared as returning string but native can return nil; update
the Spec to indicate nullable returns (change their return types to string |
null) so the TurboModule bridge and JS handle absent values safely, and add
brief null-checks where these methods are consumed; alternatively, if you prefer
guarding on native side, ensure the native implementations for
getCurrentServerFromNative/getkey/getReplies never return nil (return empty
string or sentinel) and keep the TS types unchanged—pick one strategy and apply
it consistently across the Spec and callers.
In `@app/reducers/app.ts`:
- Line 28: The reducer currently initializes isFirstServerLogin to true which
can cause a brief incorrect state before the selectServer saga reads MMKV and
dispatches the real value; change the initial state for isFirstServerLogin in
the reducer to false and let the selectServer saga (which reads MMKV and uses
yield put to update state) explicitly set the correct value so no code observes
a spurious true during cold start.
In `@app/views/UserPreferencesView/index.tsx`:
- Around line 119-128: Wrap the WatchOS quick replies UI in an iOS-only platform
guard so it only renders on iOS; locate the List.Section containing the
List.Item with title 'WatchOS_Quick_Replies' and testID
'preferences-view-watchos-quickreplies' (the item that calls
navigateToScreen('UserWatchOSQuickRepliesView')), and conditionally render that
section only when the platform is iOS (e.g., check Platform.OS === 'ios' or use
the project's isIOS helper) to prevent showing the dead navigation target on
Android.
In `@ios/RCTWatchModule.mm`:
- Around line 164-175: getReplies and getCurrentServerFromNative can return nil
when MMKV has no value, violating the non-nullable Spec; update both methods
(getReplies and getCurrentServerFromNative) to coalesce nil to a safe default
string (e.g., @"") before returning so the bridge never sends null to JS, and
ensure you reference the mmkvCurrentServerKey/mmkvQuickRepliesKey lookups and
any use of getValueFromMMKV to apply the same non-null fallback.
- Around line 158-163: The getkey method builds a malformed key when
currentServer is nil; update -[getkey] to guard the result of [self
getValueFromMMKV:mmkvCurrentServerKey] (same pattern used in getReplies and
syncQuickReplies) and when currentServer is nil return mmkvQuickRepliesKey (or
otherwise omit the "%@-" prefix) before calling stringWithFormat so you never
produce "(null)-RC_WATCHOS_QUICKREPLIES"; adjust the implementation in the
getkey method accordingly.
- Around line 19-24: The init method currently assigns WCSession.defaultSession
to _session but never activates it; update init (the - (id)init method that sets
_session) to set _session.delegate = self and call [_session activateSession]
(or [_session activate] on newer APIs) after assigning WCSession.defaultSession,
and ensure the class declares <WCSessionDelegate> conformance and implements the
required delegate method session:activationDidCompleteWithState:error: (plus any
relevant delegate stubs such as sessionDidBecomeInactive:/sessionDidDeactivate:
if needed) so session queries
(isPaired/isWatchAppInstalled/updateApplicationContext:error:) work correctly.
- Around line 88-116: The code currently throws std::runtime_error inside the
synchronous TurboModule flow (in RCTWatchModule.mm around the
updateApplicationContext handling), which will crash the app; replace all throw
std::runtime_error(...) usages in this method with RCTLogError(...) calls and an
early return path (or return a sensible value) so errors are logged instead of
thrown, remove the unreachable RCTLogError after the throw, and ensure NSError
*error checks from [_session updateApplicationContext:... error:&error] and the
JSON parse branch use the same log-and-return error handling; alternatively, if
you need to surface errors to JS, convert the API to a Promise-based method and
use reject rather than throwing.
In `@ios/RocketChat` Watch
App/Database/Default.xcdatamodeld/Default.xcdatamodel/contents:
- Line 15: DefaultDatabase in Database.swift currently loads the
NSPersistentContainer without migration options, so adding the new
quickRepliesData attribute will break existing stores; before calling
persistentContainer.loadPersistentStores(...) set migration options on
container.persistentStoreDescriptions (use
NSMigratePersistentStoresAutomaticallyOption and
NSInferMappingModelAutomaticallyOption with true as NSNumber) so lightweight
migration is enabled for the store, then call loadPersistentStores in the
existing initialization code (refer to DefaultDatabase and the
persistentStoreDescriptions/loadPersistentStores usage).
In `@ios/RocketChat` Watch App/Loaders/WatchSession.swift:
- Line 143: The DelayableWatchSession initializer ignores its delay parameter
because Timer.scheduledTimer is called with a hardcoded 1 second; update the
timer call in WatchSession/DelayableWatchSession to use the provided delay
variable (e.g., replace the literal 1 in Timer.scheduledTimer(withTimeInterval:
1, ...) with the delay parameter) so the constructed DelayableWatchSession
respects the configured delay.
- Around line 120-122: The failure branch in WatchSession.sendMessage (case
.failure where self.retries > 0) checks self.retries but never decrements it, so
configured retry budget is ignored; update the failure handling in the method
(the case handling around self.retries and
session.sendMessage/completionHandler) to decrement self.retries (e.g.,
self.retries -= 1) before re-attempting, and ensure the retry path still invokes
session.sendMessage(completionHandler:) with the updated retries value so
subsequent failures respect the remaining retry count.
- Around line 42-57: The sendMessage call in WatchSession.swift
(session.sendMessage(_:replyHandler:)) lacks an errorHandler so delivery
failures never invoke completionHandler; update the call to use
sendMessage(_:replyHandler:errorHandler:) and in the errorHandler bridge the
WCSession error into the same completion flow (e.g., call
completionHandler(.failure(.delivery(error))) or an appropriate WatchSession
error case), ensuring all paths (reply parsing in replyHandler and errors in
errorHandler) always call completionHandler and avoid leaving the caller
waiting; reference the existing completionHandler and WatchMessage decoding
logic so the error path mirrors the reply failure handling.
- Around line 68-88: The delegate method
session(_:didReceiveApplicationContext:) is called off the main thread but
mutates Core Data (setting server.quickReplies and calling serversDB.save());
wrap the Core Data updates and save call in DispatchQueue.main.async to ensure
main‑queue confinement for the viewContext (i.e., perform the
server.quickReplies = replies and serversDB.save() inside a
DispatchQueue.main.async block).
In `@ios/RocketChatRN.xcodeproj/project.pbxproj`:
- Line 2444: Remove the hard-coded contributor-specific DEVELOPMENT_TEAM =
B6FV7VP54P entries in the Xcode project file and replace them with a
shared/team-agnostic setting (e.g. remove the key or use a build setting
variable like DEVELOPMENT_TEAM = $(DEVELOPMENT_TEAM) or the org's canonical team
ID) so debug and release configs use consistent signing; update every occurrence
of the DEVELOPMENT_TEAM key in the project.pbxproj (the entries shown with
DEVELOPMENT_TEAM = B6FV7VP54P) to the chosen shared variable or remove the
per-contributor value to avoid committing personal team IDs.
---
Outside diff comments:
In `@app/views/UserPreferencesView/index.tsx`:
- Around line 37-41: navigateToScreen currently always logs
events.UP_GO_USER_NOTIFICATION_PREF which misattributes analytics; change
navigateToScreen to accept an analytics event parameter (e.g., event: string |
undefined) or remove the log from it and instead call logEvent at each call
site; update the WatchOS Quick Replies call site to pass the correct event or
call logEvent(events.UP_GO_USER_WATCH_QUICK_REPLIES) directly before calling
navigateToScreen (references: navigateToScreen function and the WatchOS Quick
Replies navigation call).
---
Minor comments:
In `@app/lib/methods/getSettings.ts`:
- Line 26: serverInfoKeys contains 'Apple_Watch_Quick_Actions' but
serverInfoUpdate's reducer (function serverInfoUpdate) has no case for that key
so the value is dropped; either remove 'Apple_Watch_Quick_Actions' from
serverInfoKeys or add a handler in serverInfoUpdate's reduce to persist it to
the server record. If you choose to persist, add a branch inside
serverInfoUpdate's reduce that checks for 'Apple_Watch_Quick_Actions' and
assigns the incoming value to the appropriate server info field on the
accumulator (matching how other keys like 'Force_SSL' or 'Unsafe_Audio_Video'
are handled) so it is included in the returned server update; if you choose to
remove, delete 'Apple_Watch_Quick_Actions' from serverInfoKeys to avoid dead
code.
In `@app/lib/methods/userPreferences.ts`:
- Around line 152-156: The getArray method currently parses JSON and blindly
casts to T[]; change getArray(key: string) to parse jsonString into a local
variable (e.g., parsed), then verify Array.isArray(parsed) before casting and
returning (return null or handle as invalid when not an array); ensure this uses
the existing mmkv.getString(key) result and keep the try/catch, optionally
logging a warning when the payload is not an array to aid debugging.
In `@app/lib/methods/WatchOSQuickReplies/syncWatchOSRepliesWithServer.ts`:
- Around line 12-21: The code stores raw comma-split appleWatchReplies which can
create empty or whitespace-only entries; before calling UserPreferences.setArray
with quickRepliesMMKVKey in syncWatchOSRepliesWithServer (where
appleWatchReplies is processed and WATCHOS_QUICKREPLIES/quickRepliesMMKVKey is
used), normalize the replies by splitting on ',' then trimming each item,
filtering out empty strings (and optionally removing duplicates) and only pass
the cleaned array to UserPreferences.setArray; keep the surrounding guards
(isFirstLogin, server, typeof appleWatchReplies === 'string') intact and then
call syncWatchOSQuickReplies as before.
In `@app/lib/native/NativeWatchModule.ts`:
- Around line 10-13: The interface exposes debug-only methods
getCurrentServerFromNative, getkey, and getReplies on NativeWatchModule which
should not be available in production; remove these methods from the production
TurboModule interface or conditionally include them only in dev builds (e.g.,
guard their declarations/exports behind a __DEV__ check or a build-time flag) so
that getCurrentServerFromNative, getkey, and getReplies are not present on the
production bridge.
In `@app/views/UserWatchOSQuickRepliesView/index.tsx`:
- Around line 58-60: When initializing quick replies in
UserWatchOSQuickRepliesView, normalize the comma-separated admin seed
(appleWatchReplies) before calling setQuickReplies to avoid empty or
whitespace-only entries: split appleWatchReplies by comma, map each piece with
trim(), filter out items with zero length (or falsy), and then pass the cleaned
array to setQuickReplies so no blank replies are saved.
- Around line 22-23: The code constructs the preference key using currentServer
which may be null, causing keys like "null-RC_WATCHOS_QUICKREPLIES"; fix by
guarding the key construction before calling useUserPreferences: obtain
currentServer via userPreferences.getString(CURRENT_SERVER), then build a safe
key (e.g., if currentServer is truthy use
`${currentServer}-${WATCHOS_QUICKREPLIES}` else use just `WATCHOS_QUICKREPLIES`
or a stable default) and pass that safeKey to
useUserPreferences<string[]>(safeKey, []); update references to currentServer/
WATCHOS_QUICKREPLIES/ useUserPreferences so preferences are never stored under a
"null-*" prefix.
In `@ios/RCTWatchModule.mm`:
- Around line 13-15: The two file-scope globals mmkvQuickRepliesKey and
mmkvCurrentServerKey currently have external linkage and can cause
duplicate-symbol linker errors; change their declarations to have internal
linkage and be immutable by making them static and const (e.g., use static
NSString * const for mmkvQuickRepliesKey and mmkvCurrentServerKey) so they are
private to this translation unit and cannot be modified externally.
In `@ios/RocketChat` Watch App/Database/Entity/Server.swift:
- Line 19: In Server.swift, remove the trailing semicolon and fix mixed
indentation: change the property declaration for quickRepliesData (`@NSManaged`
private var quickRepliesData: NSArray?) and the subsequent block around the
other properties/methods in the Server class (the lines noted in the review) to
use tabs instead of 4-space indentation and delete the stray semicolon found on
the offending line (the SwiftLint trailing_semicolon). Ensure consistent tab
indentation across the Server class so linting passes.
In `@ios/RocketChat` Watch App/Views/MessageComposerView.swift:
- Around line 33-34: ForEach is using id: \.self which breaks when quickReplies
contains duplicate strings; change the identity to a stable unique key (e.g., if
Reply has an id property use ForEach(quickReplies, id: \.id) { reply in ... } or
iterate indices like ForEach(quickReplies.indices, id: \.self) { idx in let
reply = quickReplies[idx] ... } so each row has a stable, unique ID; update
references inside the loop from reply to the indexed value if you choose the
indices approach.
---
Nitpick comments:
In `@app/sagas/selectServer.ts`:
- Around line 201-207: Add a short inline comment above the isFirstServerLogin
check explaining that the loose null check
(UserPreferences.getBool(`${server}-${FIRST_START}`) == null) intentionally
detects an absent key, that we then persist false to mark the first-start as
consumed via UserPreferences.setBool(`${server}-${FIRST_START}`, false), and
that stored false therefore means "not first start" while dispatching
setIsFirstServerLogin(true) for the initial run; reference the symbols
UserPreferences.getBool, UserPreferences.setBool, FIRST_START, and
setIsFirstServerLogin when adding the comment so future readers understand the
inverse meaning of the stored boolean and why == null is used.
- Around line 276-278: The saga handleServerFinishAdd currently selects the
entire Redux state via appSelector(s => s); change it to only select the
specific slices needed by syncWatchOSQuickRepliesWithServer (for example select
state.server, state.app and state.settings) instead of the full tree—either call
appSelector to return an object with only those keys or make separate selectors
for server/app/settings and yield them individually so handleServerFinishAdd no
longer pulls the whole state.
In `@app/stacks/InsideStack.tsx`:
- Line 175: The UserWatchOSQuickRepliesView screen must be guarded for iOS
because its component calls checkWatch() which uses NativeWatchModule
(isWatchSupported(), isWatchPaired(), isWatchAppInstalled()) and will crash on
Android; update the ProfileStack registration to only register
UserWatchOSQuickRepliesView when Platform.OS === 'ios' (or register a safe no-op
stub component for non-iOS) so navigation/deeplinks won't load the real
component on Android, and alternatively add a defensive Platform.OS check inside
UserWatchOSQuickRepliesView's checkWatch() as a secondary safeguard.
In `@app/views/UserWatchOSQuickRepliesView/index.tsx`:
- Around line 73-75: The list uses an unstable index key in quickReplies.map
which can cause UI reuse glitches; change the key on the Chip inside
quickReplies.map to a stable, content-derived identifier (e.g., use the reply
string or a unique id property if replies are objects) instead of index — update
the map in UserWatchOSQuickRepliesView to set key={reply} or key={reply.id} and
ensure removeQuickReply and any state updates continue to operate on that same
identifier.
In `@ios/RCTWatchModule.mm`:
- Around line 37-42: getMMKV currently creates a new MMKVBridge on every call
(used by getValueFromMMKV, syncQuickReplies, getkey, getReplies), causing
repeated initialization overhead; instead allocate and store a single MMKVBridge
instance as an ivar/property when the module is initialized (in init) and have
getMMKV return that cached instance; update the class to declare a MMKVBridge
*cachedMMKV (or similar), initialize it in init
(alloc/initWithID:cryptKey:rootPath:), and change getMMKV to return cachedMMKV
(and handle nil-safety if initialization can fail).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
app/actions/actionsTypes.tsapp/actions/app.tsapp/i18n/locales/en.jsonapp/lib/constants/defaultSettings.tsapp/lib/constants/keys.tsapp/lib/methods/WatchOSQuickReplies/getWatchStatus.tsapp/lib/methods/WatchOSQuickReplies/syncReplies.tsapp/lib/methods/WatchOSQuickReplies/syncWatchOSRepliesWithServer.tsapp/lib/methods/getSettings.tsapp/lib/methods/logout.tsapp/lib/methods/userPreferences.tsapp/lib/native/NativeWatchModule.tsapp/reducers/app.tsapp/sagas/selectServer.tsapp/stacks/InsideStack.tsxapp/stacks/types.tsapp/views/UserPreferencesView/index.tsxapp/views/UserWatchOSQuickRepliesView/index.tsxios/RCTWatchModule.hios/RCTWatchModule.mmios/RocketChat Watch App/Database/Default.xcdatamodeld/Default.xcdatamodel/contentsios/RocketChat Watch App/Database/Entity/Server.swiftios/RocketChat Watch App/Loaders/WatchSession.swiftios/RocketChat Watch App/Storage.swiftios/RocketChat Watch App/Views/MessageComposerView.swiftios/RocketChat Watch App/Views/MessageListView.swiftios/RocketChatRN.xcodeproj/project.pbxprojpackage.json
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-02-05T13:55:00.974Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6930
File: package.json:101-101
Timestamp: 2026-02-05T13:55:00.974Z
Learning: In this repository, the dependency on react-native-image-crop-picker should reference the RocketChat fork (RocketChat/react-native-image-crop-picker) with explicit commit pins, not the upstream ivpusic/react-native-image-crop-picker. Update package.json dependencies (and any lockfile) to point to the fork URL and a specific commit, ensuring edge-to-edge Android fixes are included. This pattern should apply to all package.json files in the repo that declare this dependency.
Applied to files:
package.json
🧬 Code graph analysis (6)
app/views/UserWatchOSQuickRepliesView/index.tsx (6)
app/stacks/types.ts (1)
ProfileStackParamList(190-203)app/lib/constants/keys.ts (2)
CURRENT_SERVER(30-30)WATCHOS_QUICKREPLIES(31-31)app/lib/methods/userPreferences.ts (1)
useUserPreferences(63-90)app/lib/methods/WatchOSQuickReplies/getWatchStatus.ts (1)
checkWatch(9-16)app/lib/methods/WatchOSQuickReplies/syncReplies.ts (1)
syncWatchOSQuickReplies(3-20)app/containers/TextInput/FormTextInput.tsx (1)
FormTextInput(103-250)
app/lib/methods/logout.ts (1)
app/lib/constants/keys.ts (1)
FIRST_START(27-27)
app/lib/methods/WatchOSQuickReplies/syncWatchOSRepliesWithServer.ts (3)
app/definitions/redux/index.ts (1)
IApplicationState(52-79)app/lib/constants/keys.ts (1)
WATCHOS_QUICKREPLIES(31-31)app/lib/methods/WatchOSQuickReplies/syncReplies.ts (1)
syncWatchOSQuickReplies(3-20)
app/sagas/selectServer.ts (4)
app/lib/constants/keys.ts (1)
FIRST_START(27-27)app/actions/app.ts (1)
setIsFirstServerLogin(82-87)app/lib/methods/WatchOSQuickReplies/syncReplies.ts (1)
syncWatchOSQuickReplies(3-20)app/actions/actionsTypes.ts (1)
SERVER(49-58)
app/reducers/app.ts (1)
app/actions/actionsTypes.ts (1)
APP(35-44)
app/actions/app.ts (1)
app/actions/actionsTypes.ts (1)
APP(35-44)
🪛 Clang (14.0.6)
ios/RCTWatchModule.h
[error] 1-1: 'Foundation/Foundation.h' file not found
(clang-diagnostic-error)
🪛 SwiftLint (0.63.2)
ios/RocketChat Watch App/Database/Entity/Server.swift
[Warning] 38-38: Lines should not have trailing semicolons
(trailing_semicolon)
🔇 Additional comments (17)
app/actions/actionsTypes.ts (1)
42-43: APP action type extension is consistent and safe.The new
SET_FIRST_LOGINaction type is added cleanly to the existingAPPrequest type map.app/lib/constants/keys.ts (1)
27-31: Key additions look correct.
FIRST_STARTandWATCHOS_QUICKREPLIESare clearly defined, and the coupling note with native code helps prevent drift.app/lib/methods/logout.ts (1)
10-17: Server cleanup now correctly includes first-login state.Including
FIRST_STARTinremoveServerKeyskeeps server-scoped preference cleanup coherent.Also applies to: 32-32
package.json (1)
209-218: Codegen config update is well-structured.
RocketChatSpecplus the iOSmodulesProvidermapping forWatchModuleis integrated cleanly.app/i18n/locales/en.json (1)
32-32: New localization entries look good.The added labels/descriptions are clear and match the WatchOS quick replies feature scope.
Also applies to: 981-982
ios/RocketChatRN.xcodeproj/project.pbxproj (1)
14-15:RCTWatchModuleproject wiring is complete and consistent.File references, group placement, and source build-phase entries are correctly added for both app targets.
Also applies to: 474-475, 735-736, 2052-2052, 2328-2328
ios/RocketChat Watch App/Storage.swift (1)
5-5: Storage key addition is clean.The new
quickReplieskey integrates cleanly with the existingStoragewrapper pattern.app/stacks/types.ts (1)
194-194: LGTM — route type correctly registered.
UserWatchOSQuickRepliesView: undefinedis consistent with other parameter-less routes inProfileStackParamList.app/lib/constants/defaultSettings.ts (1)
246-248: LGTM.
Apple_Watch_Quick_ActionswithvalueAsStringis consistent with other JSON-encoded settings in the map.ios/RocketChat Watch App/Views/MessageListView.swift (1)
78-82: LGTM —serverparameter correctly propagated.
serveris already a stored property onMessageListView, so passing it through toMessageComposerViewis safe and straightforward.app/lib/methods/getSettings.ts (1)
190-194: The try-catch is appropriate and effective here.syncWatchOSQuickRepliesWithServeris a synchronous function that does not return a Promise, so the try-catch block correctly handles any errors thrown during its execution.Note:
Apple_Watch_Quick_Actionsis included inserverInfoKeys(line 26) but is not handled in theserverInfoUpdatereduce function (lines 51–113). This setting is silently dropped and has no effect on server info updates. If the intent is to sync this setting with the server, add a handler in the reduce; otherwise, remove it fromserverInfoKeysto avoid confusion.ios/RCTWatchModule.h (1)
6-7: Header interface shape looks correct.The module declaration cleanly matches the generated spec protocol surface.
app/lib/methods/userPreferences.ts (1)
113-113: Good fix:getBoolnow preservesfalsecorrectly.Using
?? nullon Line [113] avoids converting storedfalseintonull.ios/RocketChat Watch App/Views/MessageComposerView.swift (1)
34-47: Availability gate is correct. The.glassEffectmodifier requires watchOS 26.0, which matches the#available(watchOS 26.0, *)guard on line 34. No changes needed.app/actions/app.ts (2)
24-28: LGTM!The new
ISetFirstServerLogininterface and its inclusion inTActionAppfollow the established pattern in this file consistently.
82-87: LGTM!The
setIsFirstServerLoginaction creator is consistent with the other action creators in this file and correctly referencesAPP.SET_FIRST_LOGIN.app/sagas/selectServer.ts (1)
276-291: BothsyncWatchOSQuickRepliesWithServerandsyncWatchOSQuickRepliesare synchronous functions — they do not return promises or useasync/await.syncWatchOSQuickRepliesWithServerexecutes synchronously and returnsvoid, whilesyncWatchOSQuickRepliesis synchronous, returns a boolean, and has its own internal error handling. Errors from synchronous function calls are properly caught by the surroundingtry/catchblock. No fix is needed.Likely an incorrect or invalid review comment.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (9)
ios/RCTWatchModule.mm (3)
19-24:WCSessionis never activated — all session queries return stale/incorrect values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RCTWatchModule.mm` around lines 19 - 24, The WCSession obtained in init is never activated so queries return stale values; in the init method of RCTWatchModule set _session = WCSession.defaultSession, assign its delegate to self (ensuring the class conforms to WCSessionDelegate) and call [_session activateSession] (or the modern equivalent activateSession) so the session becomes active and delivers up-to-date state and callbacks.
96-134: Throwingstd::runtime_errorfrom a synchronous TurboModule method will crash the app;RCTLogErrorafterthrowis dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RCTWatchModule.mm` around lines 96 - 134, The code currently throws std::runtime_error (e.g. in the JSON error branch, when updateApplicationContext fails, inside the catch block) which will crash a synchronous TurboModule method and makes the RCTLogError after throw dead code; replace all throws of std::runtime_error in this function with non-throwing error handling: log the error via RCTLogError (include e.what() or [[error localizedDescription] UTF8String]), perform any necessary cleanup, and return early (or propagate a proper NSError/callback/promise result depending on the surrounding API) instead of throwing; specifically change the branches that call throw std::runtime_error (the JSON parse branch, the updateApplicationContext failure branch, and inside catch around updateApplicationContext) to call RCTLogError and return, and remove any unreachable RCTLogError lines after throws.
96-100:⚠️ Potential issue | 🟡 MinorDead
elsebranch in the error ternary — always takes thelocalizedDescriptionpath.The enclosing
if (error)already guaranteeserroris non-nil, so the": "JSON parse error"fallback in the ternary is unreachable. Minor clean-up item.🧹 Proposed fix
- std::string message = - error ? [[error localizedDescription] UTF8String] - : "JSON parse error"; - throw std::runtime_error(message); + RCTLogError(@"JSON parse error: %@", [error localizedDescription]); + return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RCTWatchModule.mm` around lines 96 - 100, Inside the if (error) block in RCTWatchModule.mm the ternary using error is redundant because the enclosing if already guarantees error is non-null; replace the ternary that assigns std::string message with a direct extraction of the localizedDescription (e.g., use [[error localizedDescription] UTF8String]) so message is set unconditionally from error, and remove the unreachable "JSON parse error" fallback; update the variable assignment that currently references error ? ... : ... and keep the subsequent throw std::runtime_error(message) unchanged.app/lib/methods/WatchOSQuickReplies/getWatchStatus.ts (1)
17-22:checkWatch()calls native module methods without error handling — potential crash if the native side throws.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/methods/WatchOSQuickReplies/getWatchStatus.ts` around lines 17 - 22, The getWatchStatus/checkWatch implementation is calling NativeWatchModule.isWatchSupported(), isWatchPaired(), and isWatchAppInstalled() directly with no error handling; wrap those native calls in a try/catch inside the function (e.g., in getWatchStatus or checkWatch) and on error return a safe IWatchStatus fallback (all flags false) and optionally log the error via the existing logger; ensure the returned object shape stays IWatchStatus so callers don't crash when the native side throws.ios/RocketChat Watch App/Loaders/WatchSession.swift (5)
138-145:⚠️ Potential issue | 🟠 MajorTwo issues with
Timer.scheduledTimer: hardcoded interval ignoresdelay, and the timer may never fire when called off the main thread.
withTimeInterval: 1hardcodes the interval, so thedelayproperty injected viainithas no effect. (Previously flagged — )Timer.scheduledTimerschedules on the currentRunLoop. IfsendMessageis invoked from any thread other than the main thread (e.g., a WCSession delegate callback or a background dispatch), that thread has no activeRunLoop, the timer is silently not scheduled, andcompletionHandleris never called.🐛 Proposed fix
- Timer.scheduledTimer(withTimeInterval: 1, repeats: false) { - [weak self] _ in - guard let self else { - return - } - - self.session.sendMessage(completionHandler: completionHandler) - } + DispatchQueue.main.asyncAfter(deadline: .now() + delay) { [weak self] in + guard let self else { return } + self.session.sendMessage(completionHandler: completionHandler) + }
DispatchQueue.main.asyncAfteris run-loop–independent and always fires as expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RocketChat` Watch App/Loaders/WatchSession.swift around lines 138 - 145, Timer.scheduledTimer currently hardcodes withTimeInterval: 1 (ignoring the injected delay from init) and may not fire if called off the main thread because it schedules on the current RunLoop; replace the timer use in WatchSession.sendMessage (or the closure that calls self.session.sendMessage with completionHandler) with DispatchQueue.main.asyncAfter using the instance's delay property so the injected delay is respected and the callback always fires on the main queue, ensuring completionHandler is invoked reliably even when sendMessage is called from background threads.
68-85:⚠️ Potential issue | 🔴 CriticalCore Data
viewContextmutated off the main thread — thread-confinement violation.
didReceiveApplicationContextis delivered on WCSession's non-main serial delegate queue. Lines 83–84 write toserver.quickRepliesand callserversDB.save(), which inDatabase.swiftaccessescontainer.viewContext(anNSMainQueueConcurrencyTypecontext). This violates Core Data's queue-confinement rules and can crash.🐛 Proposed fix
func session( _ session: WCSession, didReceiveApplicationContext applicationContext: [String: Any] ) { - guard - let serverString = applicationContext["server"] as? String, - let replies = applicationContext["quickReplies"] as? [String], - let url = URL(string: serverString), - let server = serversDB.server(url: url) - else { - print("Server not found or invalid context") - return - } - - // store quick replies per server - server.quickReplies = replies - serversDB.save() + DispatchQueue.main.async { [weak self] in + guard let self else { return } + guard + let serverString = applicationContext["server"] as? String, + let replies = applicationContext["quickReplies"] as? [String], + let url = URL(string: serverString), + let server = self.serversDB.server(url: url) + else { return } + + server.quickReplies = replies + self.serversDB.save() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RocketChat` Watch App/Loaders/WatchSession.swift around lines 68 - 85, didReceiveApplicationContext is invoked on WCSession's background delegate queue but currently mutates Core Data's main viewContext by setting server.quickReplies and calling serversDB.save(), causing thread-confinement violations; fix by dispatching the Core Data write onto the main queue or use a background context: move the server.quickReplies assignment and serversDB.save() into DispatchQueue.main.async { ... } (or alternatively obtain a background context from serversDB.container.newBackgroundContext(), perform the mutation and save on that context), ensuring you reference session(_:didReceiveApplicationContext:), server.quickReplies and serversDB.save()/serversDB.container when applying the change.
115-116:⚠️ Potential issue | 🟠 MajorRetry count checked but never decremented — configured retry budget is silently ignored.
self.retries > 0is always evaluated against the fixed initializer value; the recursive call does not reduceremainingRetries, so after the first retry the budget is always non-zero and the session retries indefinitely (or until success).🐛 Proposed fix
func sendMessage( completionHandler: `@escaping` (Result<WatchMessage, ServersLoadingError>) -> Void ) { - session.sendMessage { [weak self] result in - guard let self else { return } - switch result { - case .success(let message): - completionHandler(.success(message)) - case .failure where self.retries > 0: - self.session.sendMessage(completionHandler: completionHandler) - case .failure(let error): - completionHandler(.failure(error)) - } - } + sendMessage(remainingRetries: retries, completionHandler: completionHandler) + } + + private func sendMessage( + remainingRetries: Int, + completionHandler: `@escaping` (Result<WatchMessage, ServersLoadingError>) -> Void + ) { + session.sendMessage { [weak self] result in + guard let self else { return } + switch result { + case .success(let message): + completionHandler(.success(message)) + case .failure where remainingRetries > 0: + self.sendMessage( + remainingRetries: remainingRetries - 1, + completionHandler: completionHandler + ) + case .failure(let error): + completionHandler(.failure(error)) + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RocketChat` Watch App/Loaders/WatchSession.swift around lines 115 - 116, The retry branch in WatchSession.sendMessage uses self.retries > 0 but never decrements the counter, causing infinite retries; update the retry logic to decrement the remaining retry budget before retrying (e.g., pass remainingRetries - 1 or set self.retries -= 1) so the recursive/reentrant call to session.sendMessage uses the reduced count; ensure you reference and adjust the same property used in the condition (retries or remainingRetries) and preserve the original completionHandler semantics.
42-57:⚠️ Potential issue | 🟠 MajorAdd
errorHandlertosendMessageto handle delivery failures.
session.sendMessage([:]) { ... }silently drops WCSession errors;completionHandleris never called on failure, leaving the caller in an indeterminate state.🐛 Proposed fix
- session.sendMessage([:]) { dictionary in + session.sendMessage([:], replyHandler: { dictionary in do { let data = try JSONSerialization.data(withJSONObject: dictionary) let message = try JSONDecoder().decode(WatchMessage.self, from: data) completionHandler(.success(message)) } catch { completionHandler(.failure(.undecodable(error))) } - } + }, errorHandler: { _ in + completionHandler(.failure(.unreachable)) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RocketChat` Watch App/Loaders/WatchSession.swift around lines 42 - 57, The current sendMessage call in WatchSession.swift uses only the replyHandler (session.sendMessage(_:)) so WCSession delivery errors are ignored and completionHandler isn't invoked on failure; update the call to use the sendMessage(_:replyHandler:errorHandler:) overload (provide an errorHandler closure) on the same session instance so any NSError from the message send is forwarded to the caller via completionHandler (e.g., call completionHandler(.failure(.sessionError(error))) or map to the existing failure enum) and keep the existing replyHandler behavior that decodes WatchMessage and calls completionHandler(.success(...)).
78-78:⚠️ Potential issue | 🟡 MinorRemove debug
- print("Server not found or invalid context")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RocketChat` Watch App/Loaders/WatchSession.swift at line 78, Remove the debug print("Server not found or invalid context") from WatchSession.swift (the WatchSession class / method that checks server context) and replace it with a production-appropriate logging call or error handling; e.g., remove the print statement and use the app's logger/OSLog or propagate an error/early return from the method instead so no debug prints remain in production code.
🧹 Nitpick comments (2)
app/lib/methods/WatchOSQuickReplies/syncWatchOSRepliesWithServer.ts (1)
23-25: Redundantif (server)guard —serveris already asserted non-falsy at line 12.♻️ Proposed fix
- if (server) { - syncWatchOSQuickReplies(); - } + syncWatchOSQuickReplies();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/methods/WatchOSQuickReplies/syncWatchOSRepliesWithServer.ts` around lines 23 - 25, The `if (server)` guard around the call to syncWatchOSQuickReplies() is redundant because `server` is already asserted non-falsy earlier in the function; remove the conditional and call syncWatchOSQuickReplies() directly (i.e., delete the `if (server) { ... }` block and leave a single call to syncWatchOSQuickReplies()), ensuring the surrounding flow still expects the earlier assertion of `server`.app/sagas/selectServer.ts (1)
277-295:shouldShowWatchAppOptions()is called twice — once here and again insidesyncWatchOSQuickRepliesWithServer.
shouldShowWatchAppOptions()queries native WatchConnectivity state (three native calls) on every invocation. The outer guard (line 284) and the inner guard insidesyncWatchOSQuickRepliesWithServerboth invoke it independently. Pass the result through or restructure to call it once.♻️ Proposed refactor
function* handleServerFinishAdd() { try { const state = yield* appSelector(s => s); const { server } = state.server; if (!server) return; - if (shouldShowWatchAppOptions()) { - syncWatchOSQuickRepliesWithServer(state); - syncWatchOSQuickReplies(); - } + // syncWatchOSQuickRepliesWithServer guards internally via shouldShowWatchAppOptions() + syncWatchOSQuickRepliesWithServer(state); + syncWatchOSQuickReplies(); } catch (e) { log(e); } }Alternatively, expose the check result from
syncWatchOSQuickRepliesWithServeras a return value so the outer call can decide whether to invokesyncWatchOSQuickReplies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/sagas/selectServer.ts` around lines 277 - 295, handleServerFinishAdd calls shouldShowWatchAppOptions twice (once in the outer guard and again inside syncWatchOSQuickRepliesWithServer) causing redundant native WatchConnectivity queries; fix by evaluating shouldShowWatchAppOptions once and either pass that boolean into syncWatchOSQuickRepliesWithServer (add a parameter, e.g. shouldShow) and have syncWatchOSQuickRepliesWithServer use the passed value instead of calling shouldShowWatchAppOptions, then conditionally call syncWatchOSQuickReplies based on the same boolean, or change syncWatchOSQuickRepliesWithServer to return the computed boolean so handleServerFinishAdd can decide whether to call syncWatchOSQuickReplies; update references to shouldShowWatchAppOptions in syncWatchOSQuickRepliesWithServer accordingly.
🤖 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/lib/methods/WatchOSQuickReplies/getWatchStatus.ts`:
- Line 29: Remove the debug console.log by deleting the console.log(watchStatus)
call in getWatchStatus.ts; locate the shouldShowWatchAppOptions() flow (where
watchStatus is evaluated) and either remove the console.log entirely or replace
it with a proper, gated logger (e.g., debug/info on an existing logging utility)
so it no longer prints on every evaluation—ensure you update references to
watchStatus in the getWatchStatus/getWatchStatus function to avoid
unused-variable warnings if you remove the log.
In `@app/lib/methods/WatchOSQuickReplies/syncWatchOSRepliesWithServer.ts`:
- Line 19: In syncWatchOSRepliesWithServer, the split into replies (const
replies = appleWatchReplies.split(',')) leaves leading/trailing whitespace on
items; change the logic that produces replies to trim each entry and drop empty
strings (e.g., map each split item with trim and filter out falsy/empty results)
so stored replies have no surrounding spaces and equality checks/Watch UI behave
correctly; update the variable handling around appleWatchReplies and replies
accordingly.
- Around line 15-22: Remove the redundant inner guard: inside the sync logic
where isFirstLogin, appleWatchReplies and server are already validated (the
outer if using isFirstLogin && appleWatchReplies && typeof appleWatchReplies ===
'string' and earlier server check), drop the nested "if (server &&
appleWatchReplies)" block and directly compute quickRepliesMMKVKey =
`${server}-${WATCHOS_QUICKREPLIES}` and call
UserPreferences.setArray(quickRepliesMMKVKey, replies); keeping the existing
replies = appleWatchReplies.split(',') flow and preserving WATCHOS_QUICKREPLIES
and UserPreferences usage.
In `@app/sagas/selectServer.ts`:
- Around line 202-208: The FIRST_START flag is being persisted too early: move
the UserPreferences.setBool(`${server}-${FIRST_START}`, false) call (currently
guarded by isFirstServerLogin == null) so it only executes after a confirmed
successful selection rather than before potential failures. Specifically, remove
the unconditional write in the isFirstServerLogin null branch in selectServer
saga and instead set the flag after dispatching selectServerSuccess (or perform
the write inside handleServerFinishAdd where success is guaranteed) so that
failed selectServer flows that call selectServerFailure do not mark the server
as "seen" and thereby prevent syncWatchOSRepliesWithServer from running on the
next attempt.
In `@ios/RCTWatchModule.mm`:
- Around line 69-71: The early-return guard in RCTWatchModule.mm is using && and
should use || so it returns whenever the watch is unsupported OR the watch app
is not installed; update the condition in the method containing this guard
(e.g., syncQuickReplies or the method showing the if (![self isWatchSupported]
&& ![self isWatchAppInstalled]) to use || instead of && to avoid calling
updateApplicationContext:error: when the app isn't installed.
---
Duplicate comments:
In `@app/lib/methods/WatchOSQuickReplies/getWatchStatus.ts`:
- Around line 17-22: The getWatchStatus/checkWatch implementation is calling
NativeWatchModule.isWatchSupported(), isWatchPaired(), and isWatchAppInstalled()
directly with no error handling; wrap those native calls in a try/catch inside
the function (e.g., in getWatchStatus or checkWatch) and on error return a safe
IWatchStatus fallback (all flags false) and optionally log the error via the
existing logger; ensure the returned object shape stays IWatchStatus so callers
don't crash when the native side throws.
In `@ios/RCTWatchModule.mm`:
- Around line 19-24: The WCSession obtained in init is never activated so
queries return stale values; in the init method of RCTWatchModule set _session =
WCSession.defaultSession, assign its delegate to self (ensuring the class
conforms to WCSessionDelegate) and call [_session activateSession] (or the
modern equivalent activateSession) so the session becomes active and delivers
up-to-date state and callbacks.
- Around line 96-134: The code currently throws std::runtime_error (e.g. in the
JSON error branch, when updateApplicationContext fails, inside the catch block)
which will crash a synchronous TurboModule method and makes the RCTLogError
after throw dead code; replace all throws of std::runtime_error in this function
with non-throwing error handling: log the error via RCTLogError (include
e.what() or [[error localizedDescription] UTF8String]), perform any necessary
cleanup, and return early (or propagate a proper NSError/callback/promise result
depending on the surrounding API) instead of throwing; specifically change the
branches that call throw std::runtime_error (the JSON parse branch, the
updateApplicationContext failure branch, and inside catch around
updateApplicationContext) to call RCTLogError and return, and remove any
unreachable RCTLogError lines after throws.
- Around line 96-100: Inside the if (error) block in RCTWatchModule.mm the
ternary using error is redundant because the enclosing if already guarantees
error is non-null; replace the ternary that assigns std::string message with a
direct extraction of the localizedDescription (e.g., use [[error
localizedDescription] UTF8String]) so message is set unconditionally from error,
and remove the unreachable "JSON parse error" fallback; update the variable
assignment that currently references error ? ... : ... and keep the subsequent
throw std::runtime_error(message) unchanged.
In `@ios/RocketChat` Watch App/Loaders/WatchSession.swift:
- Around line 138-145: Timer.scheduledTimer currently hardcodes
withTimeInterval: 1 (ignoring the injected delay from init) and may not fire if
called off the main thread because it schedules on the current RunLoop; replace
the timer use in WatchSession.sendMessage (or the closure that calls
self.session.sendMessage with completionHandler) with
DispatchQueue.main.asyncAfter using the instance's delay property so the
injected delay is respected and the callback always fires on the main queue,
ensuring completionHandler is invoked reliably even when sendMessage is called
from background threads.
- Around line 68-85: didReceiveApplicationContext is invoked on WCSession's
background delegate queue but currently mutates Core Data's main viewContext by
setting server.quickReplies and calling serversDB.save(), causing
thread-confinement violations; fix by dispatching the Core Data write onto the
main queue or use a background context: move the server.quickReplies assignment
and serversDB.save() into DispatchQueue.main.async { ... } (or alternatively
obtain a background context from serversDB.container.newBackgroundContext(),
perform the mutation and save on that context), ensuring you reference
session(_:didReceiveApplicationContext:), server.quickReplies and
serversDB.save()/serversDB.container when applying the change.
- Around line 115-116: The retry branch in WatchSession.sendMessage uses
self.retries > 0 but never decrements the counter, causing infinite retries;
update the retry logic to decrement the remaining retry budget before retrying
(e.g., pass remainingRetries - 1 or set self.retries -= 1) so the
recursive/reentrant call to session.sendMessage uses the reduced count; ensure
you reference and adjust the same property used in the condition (retries or
remainingRetries) and preserve the original completionHandler semantics.
- Around line 42-57: The current sendMessage call in WatchSession.swift uses
only the replyHandler (session.sendMessage(_:)) so WCSession delivery errors are
ignored and completionHandler isn't invoked on failure; update the call to use
the sendMessage(_:replyHandler:errorHandler:) overload (provide an errorHandler
closure) on the same session instance so any NSError from the message send is
forwarded to the caller via completionHandler (e.g., call
completionHandler(.failure(.sessionError(error))) or map to the existing failure
enum) and keep the existing replyHandler behavior that decodes WatchMessage and
calls completionHandler(.success(...)).
- Line 78: Remove the debug print("Server not found or invalid context") from
WatchSession.swift (the WatchSession class / method that checks server context)
and replace it with a production-appropriate logging call or error handling;
e.g., remove the print statement and use the app's logger/OSLog or propagate an
error/early return from the method instead so no debug prints remain in
production code.
---
Nitpick comments:
In `@app/lib/methods/WatchOSQuickReplies/syncWatchOSRepliesWithServer.ts`:
- Around line 23-25: The `if (server)` guard around the call to
syncWatchOSQuickReplies() is redundant because `server` is already asserted
non-falsy earlier in the function; remove the conditional and call
syncWatchOSQuickReplies() directly (i.e., delete the `if (server) { ... }` block
and leave a single call to syncWatchOSQuickReplies()), ensuring the surrounding
flow still expects the earlier assertion of `server`.
In `@app/sagas/selectServer.ts`:
- Around line 277-295: handleServerFinishAdd calls shouldShowWatchAppOptions
twice (once in the outer guard and again inside
syncWatchOSQuickRepliesWithServer) causing redundant native WatchConnectivity
queries; fix by evaluating shouldShowWatchAppOptions once and either pass that
boolean into syncWatchOSQuickRepliesWithServer (add a parameter, e.g.
shouldShow) and have syncWatchOSQuickRepliesWithServer use the passed value
instead of calling shouldShowWatchAppOptions, then conditionally call
syncWatchOSQuickReplies based on the same boolean, or change
syncWatchOSQuickRepliesWithServer to return the computed boolean so
handleServerFinishAdd can decide whether to call syncWatchOSQuickReplies; update
references to shouldShowWatchAppOptions in syncWatchOSQuickRepliesWithServer
accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/lib/methods/WatchOSQuickReplies/getWatchStatus.tsapp/lib/methods/WatchOSQuickReplies/syncReplies.tsapp/lib/methods/WatchOSQuickReplies/syncWatchOSRepliesWithServer.tsapp/lib/methods/getSettings.tsapp/lib/native/NativeWatchModule.tsapp/sagas/selectServer.tsapp/views/UserPreferencesView/index.tsxios/RCTWatchModule.mmios/RocketChat Watch App/Loaders/WatchSession.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- app/views/UserPreferencesView/index.tsx
- app/lib/methods/getSettings.ts
- app/lib/methods/WatchOSQuickReplies/syncReplies.ts
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (2)
app/lib/methods/WatchOSQuickReplies/syncWatchOSRepliesWithServer.ts (4)
app/definitions/redux/index.ts (1)
IApplicationState(52-79)app/lib/methods/WatchOSQuickReplies/getWatchStatus.ts (1)
shouldShowWatchAppOptions(25-32)app/lib/constants/keys.ts (1)
WATCHOS_QUICKREPLIES(31-31)app/lib/methods/WatchOSQuickReplies/syncReplies.ts (1)
syncWatchOSQuickReplies(4-13)
ios/RocketChat Watch App/Loaders/WatchSession.swift (2)
ios/RocketChat Watch App/Client/Extensions/Data+Extensions.swift (1)
decode(4-8)ios/RocketChat Watch App/Database/Database.swift (1)
save(44-50)
🔇 Additional comments (2)
app/lib/native/NativeWatchModule.ts (1)
4-13: LGTM — debug methods removed, Spec is clean.The three debug methods (
getCurrentServerFromNative,getkey,getReplies) that previously raised the nullable-return concern have been removed. The remaining four methods (syncQuickReplies,isWatchSupported,isWatchPaired,isWatchAppInstalled) all have correct, non-nullable return types aligned with the native implementations.ios/RocketChat Watch App/Loaders/WatchSession.swift (1)
47-50: Use the project'sData.decodehelper for consistency.While the current code will work correctly (WatchMessage contains no Date fields), using the
Data.decodehelper is consistent with the project's pattern of centralizing decoder configuration and simplifies the code.Suggested refactoring
- let data = try JSONSerialization.data( - withJSONObject: dictionary - ) - let message = try JSONDecoder().decode( - WatchMessage.self, - from: data - ) + let data = try JSONSerialization.data(withJSONObject: dictionary) + let message = try data.decode(WatchMessage.self)Likely an incorrect or invalid review comment.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
ios/RocketChat Watch App/Views/MessageComposerView.swift (2)
16-33:⚠️ Potential issue | 🟠 MajorRender
anchorIDonce at the actual bottom of the composer.
MessageListView.scrollTo(bottomAnchor)depends on this anchor being present after all composer content. Here it disappears in read-only rooms, and when quick replies exist it sits above them, so auto-scroll can stop before the true bottom.Also applies to: 35-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RocketChat` Watch App/Views/MessageComposerView.swift around lines 16 - 33, The bottom anchor (anchorID) must always be present after all composer content so MessageListView.scrollTo(bottomAnchor) can reliably scroll; update MessageComposerView to render the view with id(anchorID) unconditionally at the very end of the composer UI (outside the room.isReadOnly conditional and after any quick replies or extra views) so the anchor remains present for both read-only rooms and when quick replies are shown; locate usages of anchorID, the HStack block for read-only, and the VStack containing TextField/send handlers and move or duplicate the Color.clear.frame(height:1).id(anchorID) so it is always the last subview rendered.
37-55:⚠️ Potential issue | 🟠 MajorUse stable IDs and restore button semantics for quick replies.
id: \.selfwill collapse duplicate replies, and the currentTextpills still have no button semantics for VoiceOver. Keep the scroll-safeTextpattern, but key by index and add button traits/label.Based on learnings: In WatchOS SwiftUI code, prefer `Text` with `.onTapGesture` for tappable items inside scrollable content, and add `.accessibilityAddTraits(.isButton)` plus `.accessibilityLabel()` to preserve accessibility.🛠️ Proposed fix
- ForEach(quickReplies, id: \.self) { reply in + ForEach(quickReplies.indices, id: \.self) { index in + let reply = quickReplies[index] Text(reply) .font(.caption) .foregroundStyle(.white) .frame( maxWidth: .infinity, @@ .background( RoundedRectangle(cornerRadius: 10) .fill(Color.white.opacity(0.12)) ) .contentShape(Rectangle()) + .accessibilityAddTraits(.isButton) + .accessibilityLabel(reply) .onTapGesture { message = reply send() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RocketChat` Watch App/Views/MessageComposerView.swift around lines 37 - 55, The ForEach using id: \.self causes duplicate quickReplies to collapse and the tappable Text pills lack button semantics for VoiceOver; change the ForEach to iterate by stable index (e.g., iterate quickReplies.indices or enumerated offsets) to key items by index, access the reply via that index, keep the Text + .onTapGesture pattern that sets message and calls send(), and add accessibility by calling .accessibilityAddTraits(.isButton) and .accessibilityLabel(reply) on the Text so VoiceOver treats each pill as a button with the proper label.
🧹 Nitpick comments (1)
app/lib/methods/getSettings.ts (1)
25-25: KeepserverInfoKeysaligned withserverInfoUpdate().Line 25 adds
Apple_Watch_Quick_Actionsto the server-info subset, butserverInfoUpdate()never maps that_idintoinfo, so this entry is currently ignored on the server-record update path. Either persist it there as well or remove it fromserverInfoKeysto avoid a misleading no-op.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/methods/getSettings.ts` at line 25, serverInfoKeys now contains 'Apple_Watch_Quick_Actions' but serverInfoUpdate() does not map that _id into the info object, so the key is a no-op; either remove 'Apple_Watch_Quick_Actions' from serverInfoKeys in getSettings.ts or update serverInfoUpdate() to read the document field with _id 'Apple_Watch_Quick_Actions' and assign it into the info object (match the mapping pattern used for other keys) so the server-record update persists this value.
🤖 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 227-235: The one-shot sync in fetchWatchReplies races with
settings hydration; modify fetchWatchReplies (the generator function
fetchWatchRepliesFork) to wait for settings to be populated before calling
syncWatchOSQuickRepliesWithServer: either yield a take for the settings-loaded
action (e.g., GET_SETTINGS_SUCCESS or SETTINGS_HYDRATED) or poll/select in a
small loop with a short delay until state.settings.Apple_Watch_Quick_Actions
exists, then call syncWatchOSQuickRepliesWithServer(state); keep the try/catch
and ensure you still select the latest state before invoking the sync.
In `@ios/RCTWatchModule.mm`:
- Around line 120-128: The current syncQuickReplies() implementation in
RCTWatchModule.mm returns the joined user replies (replies) on success which can
start with "[ERROR]" and be misinterpreted as failure; change the success path
so that when success is true and error is nil you return a fixed success token
(e.g., "[OK]" or "OK") instead of the user-controlled string produced from
array/componentsJoinedByString:, keep the existing error branch unchanged (using
error/localizedDescription) and keep the joined replies available if you need to
surface them elsewhere but do not return them as the success indicator from
syncQuickReplies().
---
Duplicate comments:
In `@ios/RocketChat` Watch App/Views/MessageComposerView.swift:
- Around line 16-33: The bottom anchor (anchorID) must always be present after
all composer content so MessageListView.scrollTo(bottomAnchor) can reliably
scroll; update MessageComposerView to render the view with id(anchorID)
unconditionally at the very end of the composer UI (outside the room.isReadOnly
conditional and after any quick replies or extra views) so the anchor remains
present for both read-only rooms and when quick replies are shown; locate usages
of anchorID, the HStack block for read-only, and the VStack containing
TextField/send handlers and move or duplicate the
Color.clear.frame(height:1).id(anchorID) so it is always the last subview
rendered.
- Around line 37-55: The ForEach using id: \.self causes duplicate quickReplies
to collapse and the tappable Text pills lack button semantics for VoiceOver;
change the ForEach to iterate by stable index (e.g., iterate
quickReplies.indices or enumerated offsets) to key items by index, access the
reply via that index, keep the Text + .onTapGesture pattern that sets message
and calls send(), and add accessibility by calling
.accessibilityAddTraits(.isButton) and .accessibilityLabel(reply) on the Text so
VoiceOver treats each pill as a button with the proper label.
---
Nitpick comments:
In `@app/lib/methods/getSettings.ts`:
- Line 25: serverInfoKeys now contains 'Apple_Watch_Quick_Actions' but
serverInfoUpdate() does not map that _id into the info object, so the key is a
no-op; either remove 'Apple_Watch_Quick_Actions' from serverInfoKeys in
getSettings.ts or update serverInfoUpdate() to read the document field with _id
'Apple_Watch_Quick_Actions' and assign it into the info object (match the
mapping pattern used for other keys) so the server-record update persists this
value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c489fba-1bb4-47a7-a8f4-c7e4cdb4f0b4
📒 Files selected for processing (12)
app/i18n/locales/en.jsonapp/lib/methods/WatchOSQuickReplies/syncReplies.tsapp/lib/methods/WatchOSQuickReplies/syncWatchOSRepliesWithServer.tsapp/lib/methods/getSettings.tsapp/lib/native/NativeWatchModule.tsapp/sagas/login.jsapp/views/UserWatchOSQuickRepliesView/index.tsxios/RCTWatchModule.mmios/RocketChat Watch App/Loaders/WatchSession.swiftios/RocketChat Watch App/Views/MessageComposerView.swiftios/RocketChat Watch App/Views/MessageListView.swiftios/RocketChatRN.xcodeproj/project.pbxproj
🚧 Files skipped from review as they are similar to previous changes (4)
- app/lib/methods/WatchOSQuickReplies/syncWatchOSRepliesWithServer.ts
- app/views/UserWatchOSQuickRepliesView/index.tsx
- app/i18n/locales/en.json
- app/lib/native/NativeWatchModule.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RocketChat Watch App/Views/MessageComposerView.swift:37-55
Timestamp: 2026-03-04T20:13:17.288Z
Learning: In the WatchOS app (ios/RocketChat Watch App) for Rocket.Chat React Native, using SwiftUI `Button` inside a `ScrollView` on WatchOS causes accidental message sends because button tap targets can be triggered during scroll gestures. `Text` with `.onTapGesture` is the preferred pattern for tappable items in scroll views on WatchOS. To preserve accessibility, add `.accessibilityAddTraits(.isButton)` and `.accessibilityLabel()` to the `Text` element instead.
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RCTWatchModule.mm:19-24
Timestamp: 2026-03-05T06:06:19.755Z
Learning: In the Rocket.Chat React Native iOS app, `WCSession` (WatchConnectivity) is activated with its delegate in `ios/RocketChat Watch App/Loaders/WatchSession.swift`, not in `RCTWatchModule.mm`. Since `WCSession.defaultSession` is a singleton, activating it once in `WatchSession.swift` is sufficient; `RCTWatchModule.mm` does not need to re-activate or re-set the delegate.
📚 Learning: 2026-03-05T06:06:19.755Z
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RCTWatchModule.mm:19-24
Timestamp: 2026-03-05T06:06:19.755Z
Learning: In the Rocket.Chat React Native iOS app, `WCSession` (WatchConnectivity) is activated with its delegate in `ios/RocketChat Watch App/Loaders/WatchSession.swift`, not in `RCTWatchModule.mm`. Since `WCSession.defaultSession` is a singleton, activating it once in `WatchSession.swift` is sufficient; `RCTWatchModule.mm` does not need to re-activate or re-set the delegate.
Applied to files:
ios/RocketChat Watch App/Loaders/WatchSession.swiftios/RocketChatRN.xcodeproj/project.pbxproj
📚 Learning: 2026-03-04T20:13:17.288Z
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RocketChat Watch App/Views/MessageComposerView.swift:37-55
Timestamp: 2026-03-04T20:13:17.288Z
Learning: In the WatchOS app (ios/RocketChat Watch App) for Rocket.Chat React Native, using SwiftUI `Button` inside a `ScrollView` on WatchOS causes accidental message sends because button tap targets can be triggered during scroll gestures. `Text` with `.onTapGesture` is the preferred pattern for tappable items in scroll views on WatchOS. To preserve accessibility, add `.accessibilityAddTraits(.isButton)` and `.accessibilityLabel()` to the `Text` element instead.
Applied to files:
ios/RocketChat Watch App/Loaders/WatchSession.swiftios/RCTWatchModule.mm
📚 Learning: 2026-02-26T10:48:56.700Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7008
File: .github/workflows/maestro-ios.yml:90-93
Timestamp: 2026-02-26T10:48:56.700Z
Learning: In the Rocket.Chat.ReactNative repository's Maestro iOS workflow (.github/workflows/maestro-ios.yml), the `retry_on: timeout` configuration is intentionally used instead of `retry_on: any` because test failures are handled within the .github/scripts/run-maestro.sh script itself. Workflow-level retries should only occur when Maestro hangs (timeout), not on test failures.
Applied to files:
ios/RocketChat Watch App/Loaders/WatchSession.swift
📚 Learning: 2026-03-04T20:13:10.435Z
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RocketChat Watch App/Views/MessageComposerView.swift:37-55
Timestamp: 2026-03-04T20:13:10.435Z
Learning: In WatchOS SwiftUI code (e.g., iOS Watch App targets), prefer using Text with an onTapGesture inside ScrollView for tappable items instead of Button, to avoid accidental taps during scroll gestures. When using Text for tappable content, preserve accessibility by adding accessibilityAddTraits(.isButton) and provide an accessibilityLabel() to describe the action. Apply this pattern to WatchOS-related SwiftUI view files (notably those under ios/**/Views/) to reduce mis-taps and improve accessibility.
Applied to files:
ios/RocketChat Watch App/Views/MessageListView.swiftios/RocketChat Watch App/Views/MessageComposerView.swift
📚 Learning: 2025-12-17T15:56:22.578Z
Learnt from: OtavioStasiak
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6499
File: app/containers/ServerItem/index.tsx:34-36
Timestamp: 2025-12-17T15:56:22.578Z
Learning: In the Rocket.Chat React Native codebase, for radio button components on iOS, include the selection state ("Selected"/"Unselected") in the accessibilityLabel instead of using accessibilityState={{ checked: hasCheck }}, because iOS VoiceOver has known issues with accessibilityRole="radio" + accessibilityState that prevent correct state announcement.
Applied to files:
ios/RocketChat Watch App/Views/MessageComposerView.swiftios/RCTWatchModule.mm
📚 Learning: 2026-03-05T06:06:12.277Z
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RCTWatchModule.mm:19-24
Timestamp: 2026-03-05T06:06:12.277Z
Learning: Do not re-activate or reset the WCSession singleton in iOS Objective-C/Swift bridge modules. Ensure WCSession is activated and its delegate is set in a single, central place (e.g., ios/RocketChat Watch App/Loaders/WatchSession.swift) and avoid duplicating activation or delegate assignment in other iOS bridge files like ios/RCTWatchModule.mm. If WCSession is already activated via the central loader, relying on WCSession.defaultSession is sufficient and maintains a single session lifecycle.
Applied to files:
ios/RCTWatchModule.mm
🔇 Additional comments (7)
ios/RocketChat Watch App/Loaders/WatchSession.swift (7)
3-8: LGTM!The protocol definition is clean with proper multiline formatting for readability.
10-23: LGTM!The dependency injection pattern and WCSession initialization are correctly implemented. Based on learnings, this is the appropriate location to activate the WCSession delegate.
25-42: LGTM!The precondition checks for session state, device lock status, and reachability are comprehensive and provide appropriate error feedback.
94-94: Force unwrap on URL initialization can crash the Watch app.Line 94 uses
URL(string: serverString)!which will crash ifserverStringcontains an invalid URL format. SinceapplicationContextcomes from external input, this should be safely unwrapped.Proposed fix
- if let server = self.serversDB.server(url: URL(string: serverString)!) { + guard let serverURL = URL(string: serverString) else { + print("Invalid server URL") + return + } + + if let server = self.serversDB.server(url: serverURL) {
91-105: Good thread safety handling for Core Data access.The
DispatchQueue.main.asyncwrapper correctly ensures Core Data'sviewContextis accessed on the main queue. The pending quick replies storage usingUserDefaultswith key"pendingQuickReplies"is consistent with the reader inServersLoader.applyPendingQuickReplies.
108-121: LGTM!The default initializer correctly composes the decorator chain with
DelayableWatchSessionwrappingWatchSession. This matches the usage inServersLoaderwhereRetriableWatchSession()is instantiated with defaults.
144-167: LGTM!The delay decorator structure is clean with proper weak self handling in the timer callback.
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
ios/RocketChat Watch App/Views/MessageComposerView.swift (2)
35-56:⚠️ Potential issue | 🟠 MajorUse non-colliding IDs and restore button semantics for the quick-reply pills.
server.quickRepliesis just a[String], so duplicate reply text can be synced as-is;id: \.selfwill then collide. Also, these tappableTextviews are still exposed as static text to VoiceOver. Keep the scroll-safeTextpattern here, but switch theForEachID and add button traits plus an action-oriented label.Suggested fix
- ForEach(quickReplies, id: \.self) { reply in + ForEach(Array(quickReplies.enumerated()), id: \.offset) { _, reply in Text(reply) .font(.caption) .foregroundStyle(.white) .frame( maxWidth: .infinity, alignment: .leading ) .padding(.horizontal, 12) .padding(.vertical, 8) .background( RoundedRectangle(cornerRadius: 10) .fill(Color.white.opacity(0.12)) ) .contentShape(Rectangle()) + .accessibilityAddTraits(.isButton) + .accessibilityLabel("Send quick reply: \(reply)") .onTapGesture { message = reply send() } }Please sanity-check this with duplicate reply text configured and VoiceOver enabled in the Watch simulator.
Based on learnings: in WatchOS scroll views, keep
Text+.onTapGesturefor quick replies and add.accessibilityAddTraits(.isButton)with.accessibilityLabel().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RocketChat` Watch App/Views/MessageComposerView.swift around lines 35 - 56, ForEach is using id: \.self on quickReplies (a [String]) which will collide for duplicate texts and the tappable Texts are exposed as static text to VoiceOver; update the ForEach to use a stable unique id (e.g., enumerate quickReplies or map to (id: UUID, text: String)) instead of id: \.self, keep the current Text + .onTapGesture pattern to preserve scroll behavior, and add accessibility traits/label to each pill (call .accessibilityAddTraits(.isButton) and .accessibilityLabel(...) with an action-oriented label) so VoiceOver treats them as buttons; ensure the tap still sets message = reply and calls send() in the onTapGesture.
16-33:⚠️ Potential issue | 🟠 MajorRegression: keep
bottomAnchorat the real bottom in every composer state.
MessageListViewalways scrolls tobottomAnchor, but this ID is missing whenroom.isReadOnlyand it sits above the quick-reply pills when replies exist. That makes the shared scroll target disappear in one branch and stop above the actual bottom in the other.Suggested fix
- if room.isReadOnly { - HStack { - Spacer() - Text("This room is read only") - .font(.caption.bold()) - .foregroundStyle(.white) - .multilineTextAlignment(.center) - Spacer() - } - } else { - VStack(alignment: .leading, spacing: 4) { + VStack(alignment: .leading, spacing: 4) { + if room.isReadOnly { + HStack { + Spacer() + Text("This room is read only") + .font(.caption.bold()) + .foregroundStyle(.white) + .multilineTextAlignment(.center) + Spacer() + } + } else { TextField("Message", text: $message) .submitLabel(.send) .onSubmit(send) - - Color.clear - .frame(height: 1) - .id(anchorID) if !quickReplies.isEmpty { VStack(spacing: 6) { ForEach(quickReplies, id: \.self) { reply in Text(reply) .font(.caption) .foregroundStyle(.white) .frame( maxWidth: .infinity, alignment: .leading ) .padding(.horizontal, 12) .padding(.vertical, 8) .background( RoundedRectangle(cornerRadius: 10) .fill(Color.white.opacity(0.12)) ) .contentShape(Rectangle()) .onTapGesture { message = reply send() } } } .frame(maxWidth: .infinity) } } + Color.clear + .frame(height: 1) + .id(anchorID) }Also applies to: 35-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RocketChat` Watch App/Views/MessageComposerView.swift around lines 16 - 33, MessageListView scrolls to bottomAnchor but MessageComposerView currently omits/places that anchor incorrectly when room.isReadOnly or when quick-reply pills exist; update MessageComposerView so the shared bottom anchor (use the same bottomAnchor/anchorID identifier) is always present at the real visual bottom in both branches (readOnly and non-readOnly) — e.g., add a hidden view with .id(bottomAnchor or anchorID) after any quick-reply pills and at the end of the readOnly HStack branch so MessageListView can reliably scroll to the true bottom regardless of room.isReadOnly or replies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ios/RocketChat` Watch App/Views/MessageListView.swift:
- Around line 106-110: The auto-scroll handler uses proxy.scrollTo(bottomAnchor,
anchor: .center) which misaligns with the existing scrollToBottom(proxy:)
behavior that uses .bottom; update this handler (and the similar one at lines
~154-157) to either call scrollToBottom(proxy:) directly or change the anchor to
.bottom so new messages land at the bottom—look for messages.last?.ts change
handlers and replace proxy.scrollTo(bottomAnchor, anchor: .center) with a call
to scrollToBottom(proxy:) or proxy.scrollTo(bottomAnchor, anchor: .bottom).
- Around line 35-48: The empty-room branch currently hides the composer by
placing the “No messages” view in the top-level if/else inside MessageListView;
ensure MessageComposerView (or the quick-reply UI) is always rendered by moving
the empty-state into the ScrollView content or by rendering MessageComposerView
outside the messages.count == 0 conditional. Concretely, keep the
ScrollViewReader and its VStack/ForEach always present (rendering either
messages or the placeholder inside it) and place MessageComposerView below that
block (outside the if/else) so the composer is visible even when messages.count
== 0 (also apply the same change to the other occurrence around lines 91-101).
---
Duplicate comments:
In `@ios/RocketChat` Watch App/Views/MessageComposerView.swift:
- Around line 35-56: ForEach is using id: \.self on quickReplies (a [String])
which will collide for duplicate texts and the tappable Texts are exposed as
static text to VoiceOver; update the ForEach to use a stable unique id (e.g.,
enumerate quickReplies or map to (id: UUID, text: String)) instead of id:
\.self, keep the current Text + .onTapGesture pattern to preserve scroll
behavior, and add accessibility traits/label to each pill (call
.accessibilityAddTraits(.isButton) and .accessibilityLabel(...) with an
action-oriented label) so VoiceOver treats them as buttons; ensure the tap still
sets message = reply and calls send() in the onTapGesture.
- Around line 16-33: MessageListView scrolls to bottomAnchor but
MessageComposerView currently omits/places that anchor incorrectly when
room.isReadOnly or when quick-reply pills exist; update MessageComposerView so
the shared bottom anchor (use the same bottomAnchor/anchorID identifier) is
always present at the real visual bottom in both branches (readOnly and
non-readOnly) — e.g., add a hidden view with .id(bottomAnchor or anchorID) after
any quick-reply pills and at the end of the readOnly HStack branch so
MessageListView can reliably scroll to the true bottom regardless of
room.isReadOnly or replies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d05331e2-06ef-4930-b6b0-8f4cb75c8a5d
📒 Files selected for processing (5)
app/i18n/locales/en.jsonapp/sagas/login.jsios/RCTWatchModule.mmios/RocketChat Watch App/Views/MessageComposerView.swiftios/RocketChat Watch App/Views/MessageListView.swift
🚧 Files skipped from review as they are similar to previous changes (3)
- app/sagas/login.js
- app/i18n/locales/en.json
- ios/RCTWatchModule.mm
📜 Review details
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RocketChat Watch App/Views/MessageComposerView.swift:37-55
Timestamp: 2026-03-04T20:13:17.288Z
Learning: In the WatchOS app (ios/RocketChat Watch App) for Rocket.Chat React Native, using SwiftUI `Button` inside a `ScrollView` on WatchOS causes accidental message sends because button tap targets can be triggered during scroll gestures. `Text` with `.onTapGesture` is the preferred pattern for tappable items in scroll views on WatchOS. To preserve accessibility, add `.accessibilityAddTraits(.isButton)` and `.accessibilityLabel()` to the `Text` element instead.
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RCTWatchModule.mm:19-24
Timestamp: 2026-03-05T06:06:19.755Z
Learning: In the Rocket.Chat React Native iOS app, `WCSession` (WatchConnectivity) is activated with its delegate in `ios/RocketChat Watch App/Loaders/WatchSession.swift`, not in `RCTWatchModule.mm`. Since `WCSession.defaultSession` is a singleton, activating it once in `WatchSession.swift` is sufficient; `RCTWatchModule.mm` does not need to re-activate or re-set the delegate.
📚 Learning: 2026-03-04T20:13:10.435Z
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RocketChat Watch App/Views/MessageComposerView.swift:37-55
Timestamp: 2026-03-04T20:13:10.435Z
Learning: In WatchOS SwiftUI code (e.g., iOS Watch App targets), prefer using Text with an onTapGesture inside ScrollView for tappable items instead of Button, to avoid accidental taps during scroll gestures. When using Text for tappable content, preserve accessibility by adding accessibilityAddTraits(.isButton) and provide an accessibilityLabel() to describe the action. Apply this pattern to WatchOS-related SwiftUI view files (notably those under ios/**/Views/) to reduce mis-taps and improve accessibility.
Applied to files:
ios/RocketChat Watch App/Views/MessageComposerView.swiftios/RocketChat Watch App/Views/MessageListView.swift
📚 Learning: 2025-12-17T15:56:22.578Z
Learnt from: OtavioStasiak
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6499
File: app/containers/ServerItem/index.tsx:34-36
Timestamp: 2025-12-17T15:56:22.578Z
Learning: In the Rocket.Chat React Native codebase, for radio button components on iOS, include the selection state ("Selected"/"Unselected") in the accessibilityLabel instead of using accessibilityState={{ checked: hasCheck }}, because iOS VoiceOver has known issues with accessibilityRole="radio" + accessibilityState that prevent correct state announcement.
Applied to files:
ios/RocketChat Watch App/Views/MessageComposerView.swift
📚 Learning: 2026-03-05T06:06:19.755Z
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RCTWatchModule.mm:19-24
Timestamp: 2026-03-05T06:06:19.755Z
Learning: In the Rocket.Chat React Native iOS app, `WCSession` (WatchConnectivity) is activated with its delegate in `ios/RocketChat Watch App/Loaders/WatchSession.swift`, not in `RCTWatchModule.mm`. Since `WCSession.defaultSession` is a singleton, activating it once in `WatchSession.swift` is sufficient; `RCTWatchModule.mm` does not need to re-activate or re-set the delegate.
Applied to files:
ios/RocketChat Watch App/Views/MessageComposerView.swift
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
divyanshu-patil
commented
May 28, 2026
| #import <PushKit/PushKit.h> | ||
|
|
||
| #import <SSLPinning/SSLPinning.h> | ||
| #import <RocketChatSpec/RocketChatSpec.h> |
Author
There was a problem hiding this comment.
as turbomodules are used for multiple purposes this PR renamed Spec Name from SSLPinning to RocketChatSpec to avoid ambiguity
divyanshu-patil
commented
May 28, 2026
| }, | ||
| "codegenConfig": { | ||
| "name": "SSLPinning", | ||
| "name": "RocketChatSpec", |
Author
There was a problem hiding this comment.
as turbomodules are used for multiple purposes this PR renamed Spec Name from SSLPinning to RocketChatSpec to avoid ambiguity
Author
|
hello @diegolmello i have resolved all conflicts in this PR, can you take a look ? |
diegolmello
reviewed
May 28, 2026
diegolmello
left a comment
diegolmello
left a comment
Member
There was a problem hiding this comment.
Review — WatchOS Quick Replies
Two-axis review (standards + spec) against develop (merge-base 3961d4dea). Inline comments below; this body holds findings that have no single diff line to anchor to.
🔴 Headline bug — the first-login gate is dead
FIRST_START is never written anywhere in the diff (repo-wide grep: only imported + read). So getWatchOSRepliesForServer is always falsy → syncWatchOSRepliesWithServer re-seeds from server settings on every login, silently overwriting the user's local quick replies, and logout.ts never clears the key either. See inline notes on getWatchOSRepliesFromMMKV.ts and logout.ts. This was flagged independently by both review axes.
Scope creep — recommend splitting
The spec's only RN display item is "scroll-to-bottom anchor in MessageListView", which maps only to the watch app's ios/.../Views/MessageListView.swift. The RN MessageListView.tsx is not touched. Everything else in the RN message pipeline is unrelated to Apple Watch and looks like the separate "load 50 visible messages when system messages hidden" fix (commit 815ff6c3d):
loadMessagesForRoom.tsrewrite (hidden-system-message batching, async refactor)- new
HISTORY_UI_LOADER_PUSH/POPactions + reducer +room.tscreators useMessages.tsin-flight guard- ~470 lines of new tests (
loadMessagesForRoom.test.ts,useMessages.test.tsx,room.test.ts)
Incidental churn also in the diff: new Rocket.Chat.Watch.xcscheme, VoipModule.mm codegen rename SSLPinning→RocketChatSpec, AppDelegate.swift reordering, Gemfile.lock newline. Suggest moving the RN message-pipeline changes to their own PR.
Other spec gaps (no anchor line)
isWatchPairedis exposed by the native bridge /checkWatch()but never consulted inshouldShowWatchAppOptions(uses onlyisWatchSupported && isWatchAppInstalled).- Conditional visibility is non-reactive —
UserPreferencesViewgates the row on a synchronousshouldShowWatchAppOptions()evaluated once at render, with no state/subscription; it won't update if pairing/install status changes without a remount.
Reviewed with the two-axis /review flow. All line references verified against 74407994.
Author
|
@diegolmello can you review the changes? |
7 tasks
divyanshu-patil
commented
Jun 9, 2026
| } | ||
| } | ||
|
|
||
| - (nonnull NSNumber *)isWatchPaired { |
Author
There was a problem hiding this comment.
reserved for future, if we want to perform certain tasks or show conditional UI based on watchapp
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
2 participants
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.
Linked PR: RocketChat/Rocket.Chat#38911
Proposed changes
Apple Watch Quick Replies support
Apple_Watch_Quick_Actionswith related constants and localization keys.FIRST_START,WATCHOS_QUICKREPLIES) and default settings for managing quick replies.Quick Replies Management UI
Watch Sync & Status Handling
User Preferences Integration
UserPreferenceswithgetArrayandsetArray.Native iOS Bridge
RCTWatchModuleTurboModule exposing APIs to:WCSessioncommunication.Watch App Updates
UI Improvements
MessageListView.Localization
Issue(s)
How to test or reproduce
Screenshots
iOS
watchos-quickreplies.final.mp4
Android
Admin settings
Types of changes
Checklist
Further comments
Summary by CodeRabbit