Conversation
This comment has been minimized.
This comment has been minimized.
|
Warning Review limit reached
Next review available in: 19 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdded API-backed feature flags with persisted Zustand state. The ChangesFeature flag chat gating
Audio migration guidance
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant TabLayout
participant FeatureFlagStore
participant FeatureFlagAPI
participant SignalRStore
participant ChatScreen
TabLayout->>FeatureFlagStore: fetch feature flags
FeatureFlagStore->>FeatureFlagAPI: getAllFeatureFlags()
FeatureFlagAPI-->>FeatureFlagStore: ChatSystem state
alt ChatSystem enabled
TabLayout->>SignalRStore: connectChatHub()
ChatScreen->>FeatureFlagStore: read chat status
ChatScreen->>SignalRStore: fetch chat data
else ChatSystem disabled
TabLayout->>SignalRStore: skip chat connection
ChatScreen-->>ChatScreen: redirect to /home
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/chat/[channelId].tsx (1)
69-78: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate the remaining channel requests.
Lines 69-78 skip channel activation, but the presence request at Lines 82-94 and
markChannelReadat Lines 97-101 still run when chat is disabled. A disabled deep link can therefore read presence or update chat state before navigation completes.Add
isChatEnabledguards and dependencies to both effects.Proposed guard changes
useEffect(() => { + if (!isChatEnabled) return; const ids = (members ?? []).map((m) => m.UserId).filter((id): id is string => !!id && id !== currentUserId); if (ids.length === 0) return; // ... -}, [members, currentUserId]); +}, [members, currentUserId, isChatEnabled]); useEffect(() => { - if (channelId && inverted.length > 0) { + if (isChatEnabled && channelId && inverted.length > 0) { void useChatStore.getState().markChannelRead(channelId); } -}, [channelId, inverted.length]); +}, [channelId, inverted.length, isChatEnabled]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/chat/`[channelId].tsx around lines 69 - 78, Update the presence-request effect and the markChannelRead effect in the chat channel component to return early when isChatEnabled is false, matching the existing guard around setActiveChannel and channel loading. Add isChatEnabled to both effects’ dependency arrays while preserving their current behavior when chat is enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/audio-stream-refactoring.md`:
- Around line 9-10: Update the later Overview, Key Changes, and Installation
sections in the audio-stream refactoring document to consistently describe the
expo-audio SDK 56 migration using createAudioPlayer, setAudioModeAsync,
AudioPlayer, and playbackStatusUpdate; remove or explicitly label any remaining
expo-av guidance as an SDK 54 baseline.
- Around line 3-5: Update the “Expo SDK 56 migration requirement” guidance to
sequence the expo-audio dependency update during the SDK 56 upgrade rather than
before it, and describe alignment through the SDK upgrade process. If specifying
a package version, use the tested SDK 56-compatible expo-audio version;
otherwise remove the unsupported prerequisite-version wording while retaining
the expo-av replacement requirement.
In `@src/stores/feature-flags/store.ts`:
- Around line 62-66: Separate unresolved feature flags from disabled flags by
adding non-persisted current-department resolution state alongside the enabled
value in useFeatureFlag within src/stores/feature-flags/store.ts#L62-L66. Update
the redirect/rendering logic in src/app/(app)/chat.tsx#L109-L112,
src/app/(app)/chatbot.tsx#L68-L71, src/app/chat/[channelId].tsx#L239-L242, and
src/app/chat/thread/[messageId].tsx#L101-L104 to defer redirects while
unresolved, using ternary rendering for pending, enabled, and disabled states;
the store change is the root fix and all listed route sites require
corresponding updates.
- Around line 55-58: Scope feature-flag persistence to the current
DepartmentCode, or clear the feature-flags store before logout and department
changes. Update the feature-flags store around the feature-flags-storage
configuration and ensure stale flags cannot survive identity changes or fetch
failures for a new department.
---
Outside diff comments:
In `@src/app/chat/`[channelId].tsx:
- Around line 69-78: Update the presence-request effect and the markChannelRead
effect in the chat channel component to return early when isChatEnabled is
false, matching the existing guard around setActiveChannel and channel loading.
Add isChatEnabled to both effects’ dependency arrays while preserving their
current behavior when chat is enabled.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0aaa6653-a6c7-4828-bf10-32dd7eb6935c
📒 Files selected for processing (9)
docs/audio-stream-refactoring.mdsrc/api/feature-flags/feature-flags.tssrc/app/(app)/_layout.tsxsrc/app/(app)/chat.tsxsrc/app/(app)/chatbot.tsxsrc/app/chat/[channelId].tsxsrc/app/chat/thread/[messageId].tsxsrc/components/sidebar/side-menu.tsxsrc/stores/feature-flags/store.ts
| ## Expo SDK 56 migration requirement | ||
|
|
||
| Before upgrading Dispatch to Expo SDK 56, upgrade `expo-audio` to the SDK 56-compatible version and replace all remaining `expo-av` audio usage with `expo-audio`. SDK 56 no longer provides the legacy Expo Modules Core header required by `expo-av` 16, so leaving `expo-av` installed can break the iOS archive build. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate document and relevant docs/config:"
fd -a 'audio-stream-refactoring\.md|app\.json|app\.config\.(js|ts)|package\.json|expo\.config\.(js|ts)|babel\.config' . | sed 's#^\./##' | head -200
echo
echo "Show audio-stream-reffactoring.md context:"
if [ -f docs/audio-stream-refactoring.md ]; then
nl -ba docs/audio-stream-refactoring.md | sed -n '1,220p'
fi
echo
echo "Find expo versions and audio packages:"
rg -n '"expo"|"react-native"|"expo-audio"|"expo-av"|"babel-preset-expo"|SDK 56|SDK 54' -S --glob '!node_modules' --glob '!dist' --glob '!build' .Repository: Resgrid/Dispatch
Length of output: 433
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Audio-refactoring context:"
awk '{ printf "%6d %s\n", NR, $0 }' docs/audio-stream-refactoring.md | sed -n '1,220p'
echo
echo "Package/Expo config snippets:"
for f in app.config.ts babel.config.js package.json; do
echo "--- $f ---"
awk '{ printf "%6d %s\n", NR, $0 }' "$f" | sed -n '1,220p'
done
echo
echo "Search relevant strings:"
rg -n '"expo"|"react-native"|"expo-audio"|"expo-av"|"babel-preset-expo"|SDK 56|SDK 54' -S --glob '!node_modules' --glob '!dist' --glob '!build' .Repository: Resgrid/Dispatch
Length of output: 27844
🌐 Web query:
Expo SDK 54 expo-audio 1.1 installable expo-audio 16 expo-audio SDK 56 compatibility expo doc
💡 Result:
The expo-audio library is fully compatible with and designed for modern Expo SDKs, including SDK 54 and SDK 56. 1. Expo SDK 54: expo-audio 1.1 is compatible and was the standard for audio in SDK 54 [1][2]. During the SDK 54 lifecycle, users were encouraged to migrate to expo-audio from the deprecated expo-av [3][4]. You can install the version appropriate for your SDK using the command: npx expo install expo-audio [5]. 2. Expo SDK 56: expo-audio version 56.x is the version associated with SDK 56 [6][7]. The library received significant updates in SDK 56, including a new useAudioStream hook and various live-streaming improvements [8][9]. Important Note on Versions: In the Expo ecosystem, you should not manually choose a version number like 16 for expo-audio unless it specifically aligns with your SDK. Instead, always use the command npx expo install expo-audio [5]. This command ensures that the Expo CLI automatically installs the specific version of the package that is tested and compatible with your current Expo SDK version [5]. Using version 16 with SDK 56 is likely incorrect, as the package versions for expo-audio follow the SDK release numbering (e.g., version 56.x for SDK 56) [6][7]. Always verify your dependencies using npx expo-doctor after installation [5].
Citations:
- 1: https://docs.expo.dev/versions/v54.0.0/sdk/audio
- 2: https://github.com/expo/expo/blob/sdk-54/packages/expo-audio/CHANGELOG.md
- 3: https://expo.dev/changelog/sdk-54?gad_campaignid=23330869816
- 4: https://docs.expo.dev/versions/v54.0.0/sdk/audio-av/
- 5: https://github.com/expo/fyi/blob/main/resolving-dependency-issues.md
- 6: https://github.com/expo/expo/blob/main/packages/expo-audio/CHANGELOG.md
- 7: https://github.com/expo/expo/blob/HEAD/packages/expo-audio/CHANGELOG.md
- 8: https://expo.dev/changelog/sdk-56-beta
- 9: https://dev.to/expo/expo-sdk-56-5eb5
🌐 Web query:
docs.expo.dev "expo-audio" "Expo SDK 56"
💡 Result:
In Expo SDK 56, the expo-audio library is officially supported and includes several key updates [1][2]. Key features and improvements for expo-audio in SDK 56 include: - New useAudioStream Hook: This hook provides real-time access to microphone buffer data, enabling new possibilities for audio processing [1][3]. - Live-Stream Enhancements: - An isLiveStream option is now available for the lock-screen on iOS [1][2]. - A playsInSilentMode option has been added for Android [1][3]. - The AudioStatus object has been updated to include new fields: isLive, currentOffsetFromLive, and error [1][2]. You can access the official documentation for this version at https://docs.expo.dev/versions/v56.0.0/sdk/audio/ [1][2].
Citations:
- 1: https://expo.dev/changelog/sdk-56
- 2: https://dev.to/expo/expo-sdk-56-5eb5
- 3: https://medium.com/@onix_react/whats-new-in-expo-sdk-56-63f704fc8426
🌐 Web query:
Expo docs "Before upgrading" "During the upgrade" package compatibility SDK 56 expo install
💡 Result:
To upgrade your project to Expo SDK 56, follow these recommended practices regarding preparation, package compatibility, and the installation process [1][2][3]. Preparation and Best Practices Before starting the upgrade, it is strongly recommended that you read the official SDK 56 changelog and breaking changes documentation [1][4]. This allows you to identify necessary configuration adjustments specific to your application before beginning the actual upgrade process [1]. Additionally, use a dedicated git branch for the upgrade to isolate changes [2][3]. If you are still using Expo Go for development, it is recommended to migrate to development builds first, as this provides a more stable baseline for testing and debugging during and after the upgrade [5]. It is also advised to upgrade one major change at a time (e.g., upgrading the SDK version first, then enabling the New Architecture separately) to make it easier to isolate and troubleshoot any issues [5][4][6]. Package Compatibility and Installation During the upgrade, you should prioritize ensuring that your dependencies are compatible with the new SDK version [2][3]. The primary command for updating your Expo-related packages is [2][3]: npx expo install expo@^56.0.0 After updating the core expo package, run the package-aware installer to automatically align your remaining dependencies with the requirements of SDK 56 [2][3]: npx expo install --fix Post-Upgrade Verification Once the installation is complete, it is crucial to verify your project's health. The recommended next step is to run Expo Doctor, which is designed to detect mismatched versions, incompatible dependencies, and configuration issues [2][3]: npx expo-doctor If your project utilizes Expo Router, be aware that SDK 56 includes internal migration work; if you encounter issues, you may need to use the provided codemod to handle transitions between file-based routing and React Navigation imports [2][3]: npx expo-codemod sdk-56-expo-router-react-navigation-replace src Finally, rebuild your development builds and test thoroughly before deploying to production [2][3].
Citations:
- 1: https://expo.dev/blog/upgrading-to-sdk-56
- 2: https://paddyb.com/tutorials/expo-56-upgrade.html
- 3: https://signalandsyntax.netlify.app/blog/expo-sdk-56-beta-react-native
- 4: https://expo.dev/blog/upgrading-to-sdk-55
- 5: https://github.com/expo/fyi/blob/main/troubleshooting-sdk-upgrades.md
- 6: https://expo.dev/blog/upgrading-to-expo-sdk-52
Sequence the SDK 56 package update with the SDK 56 upgrade.
Expo’s SDK upgrade process updates Expo packages with the SDK and uses npx expo install --fix to align dependencies. Keep this guidance focused on the SDK upgrade, or change “Before upgrading” to “During the SDK 56 upgrade” and add the tested expo-audio package version.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/audio-stream-refactoring.md` around lines 3 - 5, Update the “Expo SDK 56
migration requirement” guidance to sequence the expo-audio dependency update
during the SDK 56 upgrade rather than before it, and describe alignment through
the SDK upgrade process. If specifying a package version, use the tested SDK
56-compatible expo-audio version; otherwise remove the unsupported
prerequisite-version wording while retaining the expo-av replacement
requirement.
Source: MCP tools
| - Migrate `src/hooks/use-ptt.ts`, `src/components/calls/call-audio-modal.tsx`, `src/stores/app/audio-stream-store.ts`, and `src/services/audio.service.ts` to `createAudioPlayer`, `setAudioModeAsync`, `AudioPlayer`, and `playbackStatusUpdate`. | ||
| - Remove `expo-av` from `package.json`, the lockfile, tests/mocks, and the Expo Doctor exclusion after no imports remain. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
if [ -f docs/audio-stream-refactoring.md ]; then
wc -l docs/audio-stream-refactoring.md
sed -n '1,220p' docs/audio-stream-refactoring.md
else
echo "docs/audio-stream-refactoring.md not found"
fi
echo
echo "== expo-av mentions in docs/audio-stream-refactoring.md =="
rg -n "expo-av|expo-audio|audio-av|createAudioPlayer|setAudioModeAsync|AudioPlayer|playbackStatusUpdate|Migrate|Remove" docs/audio-stream-refactoring.md || trueRepository: Resgrid/Dispatch
Length of output: 7247
🌐 Web query:
Expo SDK 56 expo-av expo-audio expo-audio SDK 56 createAudioPlayer AudioPlayer
💡 Result:
In Expo SDK 56, the expo-audio library is the standard tool for audio playback and recording, having replaced the deprecated expo-av package [1][2]. To work with audio, you primarily use the useAudioPlayer hook or, in advanced scenarios where the player must persist beyond a component's lifecycle, the createAudioPlayer function [3][4]. 1. useAudioPlayer(source, options): This is the recommended approach for most use cases [3]. It is a React hook that automatically manages the lifecycle of the AudioPlayer instance, ensuring that resources are properly disposed of when the component unmounts [5][6]. 2. createAudioPlayer(source, options): This function allows you to create an AudioPlayer instance manually [3][5]. Because this instance is not automatically managed by a component's lifecycle, it is your responsibility to call the release method on the player when it is no longer needed to prevent memory leaks [3][4]. Key differences when migrating from expo-av include: - Time is measured in seconds rather than milliseconds [7]. - Audio loads immediately upon creation [7]. - There is no automatic reset to the beginning when playback finishes; if you wish to replay, you must manually call player.seekTo(0) before calling play [7][8]. For additional functionality, expo-audio also provides the useAudioRecorder hook for recording [7][3].
Citations:
- 1: https://expo.dev/blog/upgrading-to-sdk-55
- 2: https://github.com/expo/skills/blob/main/plugins/expo/skills/upgrading-expo/SKILL.md
- 3: https://docs.expo.dev/versions/latest/sdk/audio/
- 4: https://github.com/expo/expo/blob/main/docs/pages/versions/unversioned/sdk/audio.mdx
- 5: https://github.com/expo/expo/blob/073ace8d/packages/expo-audio/src/ExpoAudio.ts
- 6: https://github.com/expo/expo/blob/97172ac0/packages/expo-audio/build/ExpoAudio.d.ts
- 7: https://github.com/expo/skills/blob/main/plugins/expo/skills/expo-upgrade/references/expo-av-to-audio.md
- 8: https://docs.expo.dev/versions/v53.0.0/sdk/audio/
Align expo-av references with the migration goal.
Lines 9-10 remove expo-av, but later sections describe expo-av, the Overview, the Key Changes, and the Installation steps still instruct using expo-av. Rewrite those sections to describe the expo-audio SDK 56 implementation, or label them as an SDK 54 baseline only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/audio-stream-refactoring.md` around lines 9 - 10, Update the later
Overview, Key Changes, and Installation sections in the audio-stream refactoring
document to consistently describe the expo-audio SDK 56 migration using
createAudioPlayer, setAudioModeAsync, AudioPlayer, and playbackStatusUpdate;
remove or explicitly label any remaining expo-av guidance as an SDK 54 baseline.
Source: MCP tools
| { | ||
| name: 'feature-flags-storage', | ||
| storage: createJSONStorage(() => zustandStorage), | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate persistence reset or rehydration calls for this store.
rg -n -C 3 'feature-flags-storage|featureFlagsStore\.(persist\.)?(clearStorage|rehydrate)|featureFlagsStore\.setState' src
# Inspect authentication and department-transition paths for a feature-flag reset.
rg -n -C 3 '\b(signOut|logout|signedOut|DepartmentCode)\b' srcRepository: Resgrid/Dispatch
Length of output: 24927
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== feature-flags store outline =="
ast-grep outline src/stores/feature-flags/store.ts --view expanded || true
echo "== feature-flags store contents =="
cat -n src/stores/feature-flags/store.ts
echo "== feature-flags imports/usages =="
rg -n -C 4 'featureFlagsStore|feature.*flags|Flags|Chat\.System' src --glob '!src/translations/**'
echo "== auth store relevant logout path =="
sed -n '120,175p' src/stores/auth/store.tsx | cat -n
echo "== any clearStorage/rehydrate / storage reset =="
rg -n 'clearStorage|rehydrate\(\)|setPersist|persist\.clearStorage|name:\s*feature-flags-storage' src --glob '!src/translations/**'Repository: Resgrid/Dispatch
Length of output: 21012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== app/_layout.tsx relevant initialization and gating =="
sed -n '330,410p' src/app/(' '(app))/_layout.tsx | cat -n
sed -n '520,555p' src/app/(' '(app))/_layout.tsx | cat -n
echo "== security/store identity change helpers and DepartmentCode state =="
cat -n src/stores/security/store.ts
echo "== all DepartmentCode identifiers =="
rg -n -C 3 'DepartmentCode|departmentCode|setDepartment|Department' src/stores src/services src/app src/components \
--glob '!src/translations/**' --glob '!src/__tests__/**' \
| head -n 220
echo "== imports/features around auth/store and security =="
rg -n -C 3 'useSecurityStore|securityStore|useAuthStore|useAuth|logout\(|signOut' src/app src/hooks src/stores src/components \
--glob '!src/translations/**' --glob '!src/__tests__/**' \
| head -n 260
echo "== targeted reset calls =="
rg -n 'clearStorage|featureFlagsStore\.(persist\.)?(clearStorage|rehydrate)|featureFlagsStore\.setState\(|logout\(\)\s*$|signOut\(\)\s*$' src --glob '!src/translations/**'Repository: Resgrid/Dispatch
Length of output: 264
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== app/_layout.tsx relevant initialization and gating =="
sed -n '330,410p' 'src/app/(app)/_layout.tsx' | cat -n
sed -n '520,555p' 'src/app/(app)/_layout.tsx' | cat -n
echo "== security/store identity change helpers and DepartmentCode state =="
cat -n src/stores/security/store.ts
echo "== all DepartmentCode identifiers =="
rg -n -C 3 'DepartmentCode|departmentCode|setDepartment|Department' src/stores src/services src/app src/components \
--glob '!src/translations/**' --glob '!src/__tests__/**' \
| head -n 220
echo "== imports/features around auth/store and security =="
rg -n -C 3 'useSecurityStore|securityStore|useAuthStore|useAuth|logout\(|signOut' src/app src/hooks src/stores src/components \
--glob '!src/translations/**' --glob '!src/__tests__/**' \
| head -n 260
echo "== targeted reset calls =="
rg -n 'clearStorage|featureFlagsStore\.(persist\.)?(clearStorage|rehydrate)|featureFlagsStore\.setState\(|logout\(\)\s*$|signOut\(\)\s*$' src --glob '!src/translations/**'Repository: Resgrid/Dispatch
Length of output: 44059
Reset or scope feature-flag persistence on sign-out and department changes.
feature-flags-storage is device-wide, while getAllFeatureFlags() evaluates flags for the current department. src/app/(app)/_layout.tsx fetches flags after rights, but saveFlags preserves the previous flags on fetch failure and src/stores/auth/store.tsx logout does not clear them. A prior department can leave Chat.System: true for the next department. Use a DepartmentCode-scoped storage key or clear this store before identity changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/stores/feature-flags/store.ts` around lines 55 - 58, Scope feature-flag
persistence to the current DepartmentCode, or clear the feature-flags store
before logout and department changes. Update the feature-flags store around the
feature-flags-storage configuration and ensure stale flags cannot survive
identity changes or fetch failures for a new department.
|
|
||
| /** Evaluates every active flag for the caller's department. */ | ||
| export const getAllFeatureFlags = async (signal?: AbortSignal) => { | ||
| const response = await api.get<FeatureTogglesResult>(`${FEATURE_TOGGLES}/GetAll`, { signal }); |
There was a problem hiding this comment.
Unguarded external HTTP call via api.get violates Rule [27] by lacking contextual error mapping. Wrap the call in a try/catch block, attach the operation name and endpoint, and map errors to a feature-flags domain error or safe default.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File src/api/feature-flags/feature-flags.ts:
Line 29:
Unguarded external HTTP call via `api.get` violates Rule [27] by lacking contextual error mapping. Wrap the call in a try/catch block, attach the operation name and endpoint, and map errors to a feature-flags domain error or safe default.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // Connect the realtime chat hub (best-effort; chat may be disabled per department) | ||
| try { | ||
| await useSignalRStore.getState().connectChatHub(); | ||
| // Connect the realtime chat hub only when the Chat.System feature flag is on for |
There was a problem hiding this comment.
Feature flag bypass occurs in useSignalRLifecycle, which unconditionally reconnects the chat hub on app resume via signalRStore.connectChatHub() at use-signalr-lifecycle.ts:121, defeating the Chat.System gate at _layout.tsx:182. Thread the feature flag into the resume reconnect path in handleAppResume to skip connectChatHub() when featureFlagsStore.getState().isEnabled(FeatureFlagKeys.ChatSystem) is false.
// Gate must also be enforced in use-signalr-lifecycle.ts handleAppResume:
// const hubs = [signalRStore.connectUpdateHub(), signalRStore.connectGeolocationHub()];
// if (featureFlagsStore.getState().isEnabled(FeatureFlagKeys.ChatSystem)) {
// hubs.push(signalRStore.connectChatHub());
// }
// const results = await Promise.allSettled(hubs);Prompt for LLM
File src/app/(app)/_layout.tsx:
Line 180:
Feature flag bypass occurs in `useSignalRLifecycle`, which unconditionally reconnects the chat hub on app resume via `signalRStore.connectChatHub()` at `use-signalr-lifecycle.ts:121`, defeating the `Chat.System` gate at `_layout.tsx:182`. Thread the feature flag into the resume reconnect path in `handleAppResume` to skip `connectChatHub()` when `featureFlagsStore.getState().isEnabled(FeatureFlagKeys.ChatSystem)` is false.
Suggested Code:
// Gate must also be enforced in use-signalr-lifecycle.ts handleAppResume:
// const hubs = [signalRStore.connectUpdateHub(), signalRStore.connectGeolocationHub()];
// if (featureFlagsStore.getState().isEnabled(FeatureFlagKeys.ChatSystem)) {
// hubs.push(signalRStore.connectChatHub());
// }
// const results = await Promise.allSettled(hubs);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| context: { platform: Platform.OS }, | ||
| }); | ||
|
|
||
| await featureFlagsStore.getState().fetchFlags(); |
There was a problem hiding this comment.
Unguarded awaited fetchFlags() call risks unhandled rejections that fail app initialization. Wrap the call in a try/catch block and log the error context to comply with Rule [1].
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File src/app/(app)/_layout.tsx:
Line 158:
Unguarded awaited `fetchFlags()` call risks unhandled rejections that fail app initialization. Wrap the call in a try/catch block and log the error context to comply with Rule [1].
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| // Chat.System feature flag off: no chat for this department. | ||
| if (!isChatEnabled) { | ||
| return <Redirect href={'/home' as Href} />; |
There was a problem hiding this comment.
Magic string '/home' for a finite application route reduces maintainability and risks typos. Define route names as a Route enum or const tuple instead of using raw strings.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File src/app/(app)/chat.tsx:
Line 111:
Magic string '/home' for a finite application route reduces maintainability and risks typos. Define route names as a Route enum or const tuple instead of using raw strings.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const menuItems = getMenuItems(t); | ||
| const isChatEnabled = useIsChatEnabled(); | ||
| // Chat and the assistant are gated by the Chat.System feature flag. | ||
| const menuItems = getMenuItems(t).filter((item) => (item.id === 'chat' || item.id === 'assistant' ? isChatEnabled : true)); |
There was a problem hiding this comment.
Duplicated inline string literals 'chat' and 'assistant' as menu-item identifiers risk drift across getMenuItems and other components. Define these shared keys as centralized constants, such as MenuItemIds.Chat and MenuItemIds.Assistant, to ensure consistency.
Kody rule violation: Centralize string constants
Prompt for LLM
File src/components/sidebar/side-menu.tsx:
Line 100:
Duplicated inline string literals 'chat' and 'assistant' as menu-item identifiers risk drift across `getMenuItems` and other components. Define these shared keys as centralized constants, such as `MenuItemIds.Chat` and `MenuItemIds.Assistant`, to ensure consistency.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| flags: {}, | ||
| isLoaded: false, | ||
| error: null, | ||
| fetchFlags: async () => { |
There was a problem hiding this comment.
Missing JSDoc on the async fetchFlags method hides its failure semantics, specifically that it swallows errors and sets the error state rather than rejecting. Add a JSDoc block to document the resolve value, rejection conditions, and await usage to satisfy Rule [22].
Kody rule violation: Document async/Promise behavior and errors
Prompt for LLM
File src/stores/feature-flags/store.ts:
Line 34:
Missing JSDoc on the async `fetchFlags` method hides its failure semantics, specifically that it swallows errors and sets the `error` state rather than rejecting. Add a JSDoc block to document the resolve value, rejection conditions, and await usage to satisfy Rule [22].
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stores/feature-flags/store.ts (1)
66-91: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winBind feature-flag responses to the active identity.
A request can complete after its user or department changes. The store can then persist flags for a previous identity, including unscoped flags with
identityKey: null.
src/stores/feature-flags/store.ts#L66-L91: commit results only when the captured non-null identity still matches the active identity and the request is the latest fetch.src/stores/feature-flags/store.ts#L23-L47: treat an absent identity as unresolved for new API results. Do not persist unscoped flag data.src/stores/feature-flags/__tests__/store.test.ts#L66-L173: add deferred-promise tests for identity changes during successful and failed requests.As per coding guidelines, generate tests for all generated logic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stores/feature-flags/store.ts` around lines 66 - 91, Update src/stores/feature-flags/store.ts:66-91 in fetchFlags to commit success or failure results only when the captured non-null identity still matches the active identity and the request remains the latest fetch; otherwise ignore the response. Update src/stores/feature-flags/store.ts:23-47 so absent identities are treated as unresolved for new API results and unscoped flags are never persisted. Add deferred-promise tests in src/stores/feature-flags/__tests__/store.test.ts:66-173 covering identity changes during both successful and failed requests.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/stores/feature-flags/__tests__/store.test.ts`:
- Around line 66-173: Add a fetch race test in the fetchFlags suite using a
deferred getAllFeatureFlags promise: start fetchFlags under one identity, change
identity before resolving the promise, then resolve with flags and await the
original fetch. Verify the stale response cannot update flags or identityKey,
and cover the generated identity-transition logic with the relevant assertions.
In `@src/stores/signalr/signalr-store.ts`:
- Around line 625-631: Update the disabled-feature guard in connectChatHub to
call disconnectChatHub before returning when ChatSystem is disabled. Preserve
the existing log and ensure the disconnect occurs for already-connected hubs as
well as inactive ones.
---
Outside diff comments:
In `@src/stores/feature-flags/store.ts`:
- Around line 66-91: Update src/stores/feature-flags/store.ts:66-91 in
fetchFlags to commit success or failure results only when the captured non-null
identity still matches the active identity and the request remains the latest
fetch; otherwise ignore the response. Update
src/stores/feature-flags/store.ts:23-47 so absent identities are treated as
unresolved for new API results and unscoped flags are never persisted. Add
deferred-promise tests in
src/stores/feature-flags/__tests__/store.test.ts:66-173 covering identity
changes during both successful and failed requests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 196a1d42-0c74-4a3e-a0ef-70733f9c3a15
📒 Files selected for processing (7)
src/app/(app)/chat.tsxsrc/app/(app)/chatbot.tsxsrc/app/chat/[channelId].tsxsrc/app/chat/thread/[messageId].tsxsrc/stores/feature-flags/__tests__/store.test.tssrc/stores/feature-flags/store.tssrc/stores/signalr/signalr-store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/chat/[channelId].tsx
| describe('fetchFlags', () => { | ||
| it('should store flags and stamp the current identity on success', async () => { | ||
| getAllFeatureFlags.mockResolvedValue({ | ||
| Data: [{ Key: FeatureFlagKeys.ChatSystem, Enabled: true, Value: null }], | ||
| }); | ||
|
|
||
| await featureFlagsStore.getState().fetchFlags(); | ||
|
|
||
| const state = featureFlagsStore.getState(); | ||
| expect(state.flags[FeatureFlagKeys.ChatSystem]).toEqual({ enabled: true, value: null }); | ||
| expect(state.isLoaded).toBe(true); | ||
| expect(state.error).toBeNull(); | ||
| expect(state.identityKey).toBe('user-1:dept-1'); | ||
| }); | ||
|
|
||
| it('should keep persisted flags on failure for the same identity', async () => { | ||
| featureFlagsStore.setState({ | ||
| flags: { [FeatureFlagKeys.ChatSystem]: { enabled: true, value: null } }, | ||
| isLoaded: true, | ||
| identityKey: 'user-1:dept-1', | ||
| }); | ||
| getAllFeatureFlags.mockRejectedValue(new Error('network down')); | ||
|
|
||
| await featureFlagsStore.getState().fetchFlags(); | ||
|
|
||
| const state = featureFlagsStore.getState(); | ||
| expect(state.flags[FeatureFlagKeys.ChatSystem]?.enabled).toBe(true); | ||
| expect(state.identityKey).toBe('user-1:dept-1'); | ||
| expect(state.error).toBe('network down'); | ||
| }); | ||
|
|
||
| it('should clear flags from a different department before fetching so a failed fetch cannot reuse them', async () => { | ||
| featureFlagsStore.setState({ | ||
| flags: { [FeatureFlagKeys.ChatSystem]: { enabled: true, value: null } }, | ||
| isLoaded: true, | ||
| identityKey: 'user-1:dept-old', | ||
| }); | ||
| getAllFeatureFlags.mockRejectedValue(new Error('network down')); | ||
|
|
||
| await featureFlagsStore.getState().fetchFlags(); | ||
|
|
||
| const state = featureFlagsStore.getState(); | ||
| expect(state.flags).toEqual({}); | ||
| // Fail-closed: the failed fetch still resolves the flags so consumers stop waiting. | ||
| expect(state.isLoaded).toBe(true); | ||
| expect(state.identityKey).toBeNull(); | ||
| }); | ||
|
|
||
| it('should clear flags from a different account before fetching', async () => { | ||
| featureFlagsStore.setState({ | ||
| flags: { [FeatureFlagKeys.ChatSystem]: { enabled: true, value: null } }, | ||
| isLoaded: true, | ||
| identityKey: 'user-other:dept-1', | ||
| }); | ||
| getAllFeatureFlags.mockRejectedValue(new Error('network down')); | ||
|
|
||
| await featureFlagsStore.getState().fetchFlags(); | ||
|
|
||
| expect(featureFlagsStore.getState().flags).toEqual({}); | ||
| }); | ||
|
|
||
| it('should replace another identity flags with fresh ones on success', async () => { | ||
| featureFlagsStore.setState({ | ||
| flags: { [FeatureFlagKeys.ChatSystem]: { enabled: true, value: null } }, | ||
| isLoaded: true, | ||
| identityKey: 'user-other:dept-other', | ||
| }); | ||
| getAllFeatureFlags.mockResolvedValue({ | ||
| Data: [{ Key: FeatureFlagKeys.ChatSystem, Enabled: false, Value: null }], | ||
| }); | ||
|
|
||
| await featureFlagsStore.getState().fetchFlags(); | ||
|
|
||
| const state = featureFlagsStore.getState(); | ||
| expect(state.flags[FeatureFlagKeys.ChatSystem]?.enabled).toBe(false); | ||
| expect(state.identityKey).toBe('user-1:dept-1'); | ||
| }); | ||
|
|
||
| it('should keep flags on failure when department is unknown but the user matches', async () => { | ||
| setIdentity('user-1', null); | ||
| featureFlagsStore.setState({ | ||
| flags: { [FeatureFlagKeys.ChatSystem]: { enabled: true, value: null } }, | ||
| isLoaded: true, | ||
| identityKey: 'user-1:dept-1', | ||
| }); | ||
| getAllFeatureFlags.mockRejectedValue(new Error('network down')); | ||
|
|
||
| await featureFlagsStore.getState().fetchFlags(); | ||
|
|
||
| const state = featureFlagsStore.getState(); | ||
| expect(state.flags[FeatureFlagKeys.ChatSystem]?.enabled).toBe(true); | ||
| expect(state.identityKey).toBe('user-1:dept-1'); | ||
| }); | ||
|
|
||
| it('should clear flags when department is unknown and the user differs', async () => { | ||
| setIdentity('user-2', null); | ||
| featureFlagsStore.setState({ | ||
| flags: { [FeatureFlagKeys.ChatSystem]: { enabled: true, value: null } }, | ||
| isLoaded: true, | ||
| identityKey: 'user-1:dept-1', | ||
| }); | ||
| getAllFeatureFlags.mockRejectedValue(new Error('network down')); | ||
|
|
||
| await featureFlagsStore.getState().fetchFlags(); | ||
|
|
||
| expect(featureFlagsStore.getState().flags).toEqual({}); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add an identity-transition test for pending fetches.
The tests change identity only before fetchFlags() starts. Add deferred API promises, change the identity before resolution, and verify that the earlier response cannot set flags or identityKey.
As per coding guidelines, generate tests for all generated logic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/stores/feature-flags/__tests__/store.test.ts` around lines 66 - 173, Add
a fetch race test in the fetchFlags suite using a deferred getAllFeatureFlags
promise: start fetchFlags under one identity, change identity before resolving
the promise, then resolve with flags and await the original fetch. Verify the
stale response cannot update flags or identityKey, and cover the generated
identity-transition logic with the relevant assertions.
Source: Coding guidelines
| const currentUserId = useAuthStore((s) => s.userId); | ||
| const isModerator = !!securityStore((s) => s.rights)?.IsAdmin; | ||
| const chatStatus = useChatSystemStatus(); | ||
| const isChatEnabled = chatStatus === 'enabled'; |
There was a problem hiding this comment.
Magic string comparison introduces silent failures if the store value changes or a typo occurs. Define a const tuple like const ChatSystemStatus = { Enabled: 'enabled', Unknown: 'unknown', Disabled: 'disabled' } as const in the feature-flags store and reference ChatSystemStatus.Enabled to enforce compile-time safety.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File src/app/chat/[channelId].tsx:
Line 43:
Magic string comparison introduces silent failures if the store value changes or a typo occurs. Define a const tuple like `const ChatSystemStatus = { Enabled: 'enabled', Unknown: 'unknown', Disabled: 'disabled' } as const` in the feature-flags store and reference `ChatSystemStatus.Enabled` to enforce compile-time safety.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| // Chat.System feature flag off: block deep links (push notifications, stale routes). | ||
| if (chatStatus === 'disabled') { | ||
| return <Redirect href={'/home' as Href} />; |
There was a problem hiding this comment.
Hardcoded route path '/home' scatters definitions, causing renames or typos to evade compile-time checks. Reference a centralized routes constant, such as ROUTES.HOME or AppRoutes.Home, defined in a single navigation module.
Kody rule violation: Centralize string constants
Prompt for LLM
File src/app/chat/[channelId].tsx:
Line 254:
Hardcoded route path '/home' scatters definitions, causing renames or typos to evade compile-time checks. Reference a centralized routes constant, such as `ROUTES.HOME` or `AppRoutes.Home`, defined in a single navigation module.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| export const useIsChatEnabled = () => useFeatureFlag(FeatureFlagKeys.ChatSystem); | ||
|
|
||
| export type FeatureFlagStatus = 'unknown' | 'enabled' | 'disabled'; |
There was a problem hiding this comment.
Duplicate source of truth: The type FeatureFlagStatus manually duplicates the runtime string literals ('unknown','enabled','disabled') returned on lines 118 and 120. Declare const FEATURE_FLAG_STATUSES = ['unknown','enabled','disabled'] as const; and derive type FeatureFlagStatus = typeof FEATURE_FLAG_STATUSES[number]; to synchronize definitions.
Kody rule violation: Derive TypeScript types from validation schemas
Prompt for LLM
File src/stores/feature-flags/store.ts:
Line 109:
Duplicate source of truth: The type `FeatureFlagStatus` manually duplicates the runtime string literals ('unknown','enabled','disabled') returned on lines 118 and 120. Declare `const FEATURE_FLAG_STATUSES = ['unknown','enabled','disabled'] as const;` and derive `type FeatureFlagStatus = typeof FEATURE_FLAG_STATUSES[number];` to synchronize definitions.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/app/chat/[channelId].tsx (1)
70-79: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate every chat side effect with
isChatEnabled.This effect is gated, but the presence request at Lines 83-95 and the read update at Lines 97-102 still run from cached state while chat is
unknownordisabled. AddisChatEnabledguards and dependencies to both effects so a disabled department does not send chat requests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/chat/`[channelId].tsx around lines 70 - 79, Gate the presence-request effect and read-update effect alongside the existing channel effect by returning early unless isChatEnabled is true. Add isChatEnabled to both effects’ dependency arrays, while preserving their existing behavior when chat is enabled.src/app/chat/thread/[messageId].tsx (2)
36-41: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winShow a user-visible error when thread loading fails.
The
catchblock only logs the failure. A network or offline error leaves the same empty state as a thread with no replies, without a retry or status message. Add translated error feedback and a retry action, or expose the cached/offline state.As per coding guidelines, “Handle errors gracefully and provide user feedback” and “Implement proper offline support.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/chat/thread/`[messageId].tsx around lines 36 - 41, Update the thread-loading flow in the useEffect around getThread so failures set user-visible translated error state instead of only logging, while retaining the messageId context in logger.error. Render that state with a retry action that re-invokes thread loading, or clearly expose the cached/offline state, and preserve the existing empty-replies behavior for successful responses.Source: Coding guidelines
36-41: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep fetched replies scoped to the active
messageId.When this screen instance receives a new
messageId,fetchedReplieskeeps the previous thread's data. The request also writes its result without checking that it is still current. Replies from one thread can therefore appear in another thread, and a failed request can leave the wrong replies visible.Clear the state when
messageIdchanges and ignore stale responses.Proposed fix
useEffect(() => { - if (!messageId || !isChatEnabled) return; + if (!messageId || !isChatEnabled) { + setFetchedReplies([]); + return; + } + let cancelled = false; + setFetchedReplies([]); + getThread(messageId, undefined, 50) - .then((response) => setFetchedReplies(response.Data ?? [])) + .then((response) => { + if (!cancelled) setFetchedReplies(response.Data ?? []); + }) .catch((error) => logger.error({ message: 'chat: failed to load thread', context: { error, messageId } })); + + return () => { + cancelled = true; + }; }, [messageId, isChatEnabled]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/chat/thread/`[messageId].tsx around lines 36 - 41, Update the useEffect tied to messageId and isChatEnabled to clear fetchedReplies when the active messageId changes, and ignore both successful and failed results from requests started for an earlier messageId. Ensure only the current thread’s response can update state or leave existing replies visible.
🧹 Nitpick comments (2)
src/components/chat/message-actions-sheet.tsx (1)
31-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
React.FCfor these prop-bearing components.
src/components/chat/message-actions-sheet.tsx#L31-L31: declareMessageActionsSheetasReact.FC<MessageActionsSheetProps>.src/components/chat/message-composer.tsx#L32-L32: declareMessageComposerasReact.FC<MessageComposerProps>.As per coding guidelines, “Utilize
React.FCfor defining functional components with props.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/chat/message-actions-sheet.tsx` at line 31, Update MessageActionsSheet in src/components/chat/message-actions-sheet.tsx:31 to use React.FC<MessageActionsSheetProps>, and update MessageComposer in src/components/chat/message-composer.tsx:32 to use React.FC<MessageComposerProps>, preserving their existing props and behavior.Source: Coding guidelines
src/app/(app)/chatbot.tsx (1)
76-76: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse a stable no-op reaction callback.
renderItemcreates a newonToggleReactionfunction for every item render. Define a typed no-op callback outside the component, then pass that callback.As per coding guidelines, “Avoid anonymous functions in
renderItemor event handlers to prevent re-renders.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`(app)/chatbot.tsx at line 76, Define a typed, stable no-op reaction callback outside the component, then replace the inline onToggleReaction function in renderItem’s MessageBubble with that shared callback. Preserve the existing no-op behavior while avoiding a new function allocation on each item render.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/`(app)/chatbot.tsx:
- Around line 188-193: Update the edit-message save handler around
setEditMessage and the editMessage call so an empty editText.trim() cannot
silently close the sheet: disable the Save action while trimmed text is empty or
display a validation message and keep the editor open. Preserve saving only when
editMessage, chatbotChannelId, and non-empty trimmed text are present.
In `@src/app/chat/`[channelId].tsx:
- Around line 257-260: Resolve or fetch the channel metadata before the screen’s
initialization and rendering flow, then apply the ChatChannelType.Chatbot
redirect once channel is known. Update the channel-loading logic around the
existing channel check so cold and stale deep links cannot render the generic
conversation before redirecting to the dedicated chatbot route.
---
Outside diff comments:
In `@src/app/chat/`[channelId].tsx:
- Around line 70-79: Gate the presence-request effect and read-update effect
alongside the existing channel effect by returning early unless isChatEnabled is
true. Add isChatEnabled to both effects’ dependency arrays, while preserving
their existing behavior when chat is enabled.
In `@src/app/chat/thread/`[messageId].tsx:
- Around line 36-41: Update the thread-loading flow in the useEffect around
getThread so failures set user-visible translated error state instead of only
logging, while retaining the messageId context in logger.error. Render that
state with a retry action that re-invokes thread loading, or clearly expose the
cached/offline state, and preserve the existing empty-replies behavior for
successful responses.
- Around line 36-41: Update the useEffect tied to messageId and isChatEnabled to
clear fetchedReplies when the active messageId changes, and ignore both
successful and failed results from requests started for an earlier messageId.
Ensure only the current thread’s response can update state or leave existing
replies visible.
---
Nitpick comments:
In `@src/app/`(app)/chatbot.tsx:
- Line 76: Define a typed, stable no-op reaction callback outside the component,
then replace the inline onToggleReaction function in renderItem’s MessageBubble
with that shared callback. Preserve the existing no-op behavior while avoiding a
new function allocation on each item render.
In `@src/components/chat/message-actions-sheet.tsx`:
- Line 31: Update MessageActionsSheet in
src/components/chat/message-actions-sheet.tsx:31 to use
React.FC<MessageActionsSheetProps>, and update MessageComposer in
src/components/chat/message-composer.tsx:32 to use
React.FC<MessageComposerProps>, preserving their existing props and behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 19ce0c0d-2d80-47da-9432-aca46f62062e
📒 Files selected for processing (10)
src/api/chat/chatbot.tssrc/app/(app)/chat.tsxsrc/app/(app)/chatbot.tsxsrc/app/chat/[channelId].tsxsrc/app/chat/thread/[messageId].tsxsrc/components/chat/message-actions-sheet.tsxsrc/components/chat/message-composer.tsxsrc/models/v4/chat/chatbotModels.tssrc/stores/chat/store.tssrc/stores/signalr/signalr-store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/stores/signalr/signalr-store.ts
| onPress={() => { | ||
| if (editMessage && chatbotChannelId && editText.trim()) { | ||
| void useChatStore.getState().editMessage(editMessage.ChatMessageId, chatbotChannelId, editText.trim()); | ||
| } | ||
| setEditMessage(null); | ||
| }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not silently discard an empty edit.
If editText.trim() is empty, the handler closes the sheet without saving or user feedback. Disable Save until text is non-empty, or show a validation message.
As per coding guidelines, “Handle errors gracefully and provide user feedback.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/`(app)/chatbot.tsx around lines 188 - 193, Update the edit-message
save handler around setEditMessage and the editMessage call so an empty
editText.trim() cannot silently close the sheet: disable the Save action while
trimmed text is empty or display a validation message and keep the editor open.
Preserve saving only when editMessage, chatbotChannelId, and non-empty trimmed
text are present.
Source: Coding guidelines
| const renderItem = useCallback( | ||
| ({ item }: { item: ChatMessageResultData }) => ( | ||
| <MessageBubble message={item} isOwn={!!item.SenderUserId && item.SenderUserId === currentUserId} showSender={false} currentUserId={currentUserId} onLongPress={() => undefined} onToggleReaction={() => undefined} /> | ||
| <MessageBubble message={item} isOwn={!!item.SenderUserId && item.SenderUserId === currentUserId} showSender={false} currentUserId={currentUserId} onLongPress={setActionsMessage} onToggleReaction={() => undefined} /> |
There was a problem hiding this comment.
Unnecessary re-renders occur in src/app/(app)/chatbot.tsx (lines 154, 158-161, 164, 168-171, 175, 188), src/app/chat/thread/[messageId].tsx:134, src/components/chat/message-actions-sheet.tsx:99, and src/components/chat/message-composer.tsx:138 because inline arrow functions and .bind() calls in JSX props create new function instances on every render. Move these function definitions outside the render method.
Kody rule violation: Avoid using .bind() or arrow functions in JSX props
Prompt for LLM
File src/app/(app)/chatbot.tsx:
Line 76:
Unnecessary re-renders occur in `src/app/(app)/chatbot.tsx` (lines 154, 158-161, 164, 168-171, 175, 188), `src/app/chat/thread/[messageId].tsx:134`, `src/components/chat/message-actions-sheet.tsx:99`, and `src/components/chat/message-composer.tsx:138` because inline arrow functions and `.bind()` calls in JSX props create new function instances on every render. Move these function definitions outside the render method.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| className="bg-primary-600" | ||
| onPress={() => { | ||
| if (editMessage && chatbotChannelId && editText.trim()) { | ||
| void useChatStore.getState().editMessage(editMessage.ChatMessageId, chatbotChannelId, editText.trim()); |
There was a problem hiding this comment.
Unhandled promise rejection risk exists in src/app/(app)/chatbot.tsx (lines 154, 161) because the promise returned by editMessage is discarded with the void operator, silently swallowing errors. Wrap the call in a try/catch block or chain a .catch() handler to display an error toast on failure.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File src/app/(app)/chatbot.tsx:
Line 190:
Unhandled promise rejection risk exists in `src/app/(app)/chatbot.tsx` (lines 154, 161) because the promise returned by `editMessage` is discarded with the `void` operator, silently swallowing errors. Wrap the call in a try/catch block or chain a `.catch()` handler to display an error toast on failure.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| </Pressable> | ||
| {allowUrgent ? ( | ||
| <Pressable className="p-2" onPress={() => setUrgent((prev) => !prev)} disabled={disabled} accessibilityLabel={t('chat.urgent')}> | ||
| <AlertTriangle size={22} color={urgent ? '#dc2626' : '#6b7280'} /> |
There was a problem hiding this comment.
Hardcoded hex color literals '#dc2626' and '#6b7280' in src/components/chat/message-composer.tsx duplicate values and make palette or theme changes error-prone. Extract these repeated strings into named constants like COLOR_ERROR and COLOR_MUTED within a centralized theme module.
Kody rule violation: Centralize string constants
Prompt for LLM
File src/components/chat/message-composer.tsx:
Line 139:
Hardcoded hex color literals `'#dc2626'` and `'#6b7280'` in `src/components/chat/message-composer.tsx` duplicate values and make palette or theme changes error-prone. Extract these repeated strings into named constants like `COLOR_ERROR` and `COLOR_MUTED` within a centralized theme module.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| void useChatStore | ||
| .getState() | ||
| .fetchChannels() | ||
| .finally(() => setResolveAttempted(true)); |
There was a problem hiding this comment.
Unhandled promise rejection occurs if fetchChannels() rejects because the void promise chain relies solely on .finally(). Add a .catch() handler to swallow and log the error before executing the finally block.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File src/app/chat/[channelId].tsx:
Line 76 to 79:
Unhandled promise rejection occurs if `fetchChannels()` rejects because the `void` promise chain relies solely on `.finally()`. Add a `.catch()` handler to swallow and log the error before executing the finally block.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| void useChatStore | ||
| .getState() | ||
| .fetchChannels() | ||
| .finally(() => setResolveAttempted(true)); |
There was a problem hiding this comment.
Unguarded network call to fetchChannels() silently propagates failures without context mapping. Wrap the invocation in a try/catch to log the error via logger.error and map it to application-level errors.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File src/app/chat/[channelId].tsx:
Line 76 to 79:
Unguarded network call to `fetchChannels()` silently propagates failures without context mapping. Wrap the invocation in a `try/catch` to log the error via `logger.error` and map it to application-level errors.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| try { | ||
| return await Clipboard.setStringAsync(text); | ||
| } catch { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Falsy return value from Clipboard.setStringAsync causes native callers in [channelId].tsx:351 and chatbot.tsx:161 to falsely trigger the "copy unavailable" toast on success. Await the call separately and return an explicit boolean to reflect the actual state.
try {
await Clipboard.setStringAsync(text);
return true;
} catch {
return false;
}Prompt for LLM
File src/components/chat/chat-utils.ts:
Line 123 to 127:
Falsy return value from `Clipboard.setStringAsync` causes native callers in `[channelId].tsx:351` and `chatbot.tsx:161` to falsely trigger the "copy unavailable" toast on success. Await the call separately and return an explicit boolean to reflect the actual state.
Suggested Code:
try {
await Clipboard.setStringAsync(text);
return true;
} catch {
return false;
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } catch { | ||
| return false; |
There was a problem hiding this comment.
Silent exception swallowing in the Clipboard.setStringAsync catch block causes native clipboard write failures to become invisible during debugging. Log the captured error with context via logger.error before returning false.
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File src/components/chat/chat-utils.ts:
Line 125 to 126:
Silent exception swallowing in the `Clipboard.setStringAsync` catch block causes native clipboard write failures to become invisible during debugging. Log the captured error with context via `logger.error` before returning `false`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
2 participants
Pull Request Description
This PR introduces a department-scoped feature flag system to the Resgrid Dispatch mobile app, with the Chat system as the first feature gated behind it.
What was added
Feature Flags API Client (
src/api/feature-flags/feature-flags.ts): New API integration to evaluate feature toggles from the v4 FeatureToggles API, supporting both bulk retrieval (GetAll) and single-flag state checks (GetState).Feature Flags Store (
src/stores/feature-flags/store.ts): A persisted Zustand store that fetches and caches feature flags. It retains flags on fetch failure to keep gating stable while offline, and unknown flags default to disabled until the server confirms them.Chat Feature Gating: The
Chat.Systemflag controls all chat surfaces:_layout.tsx): Feature flags are fetched after security rights load; the SignalR chat hub only connects when chat is enabled.chat.tsx,chatbot.tsx,chat/[channelId].tsx,chat/thread/[messageId].tsx): Redirect to home when chat is disabled, and skip data-fetching effects.side-menu.tsx): Chat and Assistant menu items are hidden when the flag is off.Documentation: Updated audio stream refactoring docs with Expo SDK 56 migration guidance for
expo-audio/expo-av.Summary by CodeRabbit