-
Notifications
You must be signed in to change notification settings - Fork 7
RG-T117 chat feature flag #262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| import { api } from '../common/client'; | ||
|
|
||
| const FEATURE_TOGGLES = '/FeatureToggles'; | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Feature toggle evaluation (department-scoped, any authenticated user). | ||
| // Backed by the v4 FeatureToggles API; keys live in Resgrid.Model.FeatureFlagKeys. | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| export interface FeatureToggleData { | ||
| Key: string; | ||
| Enabled: boolean; | ||
| Value?: string | null; | ||
| ValueType?: string | null; | ||
| Source?: string | null; | ||
| } | ||
|
|
||
| export interface FeatureTogglesResult { | ||
| Data?: FeatureToggleData[]; | ||
| StateHash?: string; | ||
| } | ||
|
|
||
| export interface FeatureToggleResult { | ||
| Data?: FeatureToggleData; | ||
| } | ||
|
|
||
| /** 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unhandled network exception: api.get at src/api/feature-flags/feature-flags.ts:35 throws transient and non-transient errors that propagate without enrichment or mapping to application-level errors. Wrap the call in try/catch, log with structured context including the flag operation name, and rethrow or return a typed error. Kody rule violation: Add try-catch blocks for external calls Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
| return response.data; | ||
| }; | ||
|
|
||
| /** Lightweight enabled-only check for a single flag. */ | ||
| export const getFeatureFlagState = async (key: string, signal?: AbortSignal) => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing Promise contract documentation: getFeatureFlagState at src/api/feature-flags/feature-flags.ts:28 omits @returns and rejection conditions, leaving callers without resolve/reject semantics for correct await usage. Add Kody rule violation: Document async/Promise behavior and errors Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
| const response = await api.get<FeatureToggleResult>(`${FEATURE_TOGGLES}/GetState`, { params: { key }, signal }); | ||
| return response.data; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,7 @@ import { bluetoothAudioService } from '@/services/bluetooth-audio.service'; | |
| import { usePushNotifications } from '@/services/push-notification'; | ||
| import { useCoreStore } from '@/stores/app/core-store'; | ||
| import { useCallsStore } from '@/stores/calls/store'; | ||
| import { FeatureFlagKeys, featureFlagsStore } from '@/stores/feature-flags/store'; | ||
| import { useRolesStore } from '@/stores/roles/store'; | ||
| import { securityStore } from '@/stores/security/store'; | ||
| import { useSignalRStore } from '@/stores/signalr/signalr-store'; | ||
|
|
@@ -164,19 +165,27 @@ export default function TabLayout() { | |
|
|
||
| // These fetches are independent of each other — run in parallel to cut | ||
| // time-to-interactive (previously 8+ serial network hops). | ||
| await Promise.all([useRolesStore.getState().init(), useCallsStore.getState().init(), useWeatherAlertsStore.getState().init(), securityStore.getState().getRights()]); | ||
| await Promise.all([useRolesStore.getState().init(), useCallsStore.getState().init(), useWeatherAlertsStore.getState().init(), securityStore.getState().getRights(), featureFlagsStore.getState().fetchFlags()]); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Promise.all misuse for independent batch operations violates the team's partial-failure handling rule: if any single init() rejects (useRolesStore, useCallsStore, useWeatherAlertsStore, securityStore, or featureFlagsStore), the remaining initializations are aborted. Replace Promise.all with Promise.allSettled and handle per-item results. Kody rule violation: Use Promise.allSettled for batch operations with partial failures Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
|
|
||
| // SignalR needs config (EventingUrl) and rights (DepartmentId) — both | ||
| // available now. The two hub connects are independent. | ||
| await Promise.all([useSignalRStore.getState().connectUpdateHub(), useSignalRStore.getState().connectGeolocationHub()]); | ||
|
|
||
| // Connect the realtime chat hub (best-effort; chat may be disabled per department) | ||
| try { | ||
| await useSignalRStore.getState().connectChatHub(); | ||
| } catch (error) { | ||
| logger.warn({ | ||
| message: 'Failed to connect SignalR chat hub during initialization', | ||
| context: { error, platform: Platform.OS }, | ||
| // Connect the realtime chat hub only when the Chat.System feature flag is on for | ||
| // this department; when it is off every chat surface stays hidden. | ||
| if (featureFlagsStore.getState().isEnabled(FeatureFlagKeys.ChatSystem)) { | ||
| // Best-effort: chat hub failure should not block app initialization. | ||
| try { | ||
| await useSignalRStore.getState().connectChatHub(); | ||
| } catch (error) { | ||
| logger.warn({ | ||
| message: 'Failed to connect SignalR chat hub during initialization', | ||
| context: { error, platform: Platform.OS }, | ||
| }); | ||
| } | ||
| } else { | ||
| logger.info({ | ||
| message: 'Chat disabled by feature flag; skipping chat hub connection', | ||
| }); | ||
|
Comment on lines
+168
to
189
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect every chat-hub connection call and lifecycle implementation.
rg -nP -C 6 '\bconnectChatHub\s*\(' src
rg -nP -C 8 '\buseSignalRLifecycle\s*\(' srcRepository: Resgrid/Unit Length of output: 24296 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== use-signalr-lifecycle.ts relevant sections =="
sed -n '1,170p' src/hooks/use-signalr-lifecycle.ts
echo
echo "== signalr store files =="
fd -a 'signalr-store|signalr' src/stores src/components src/app src/hooks | sed 's#^\./##' | head -80
echo
echo "== chat disable/feature flag references =="
rg -n -C 4 'ChatSystem|connectChatHub|disconnectChatHub|isChatHubConnected|hubNames|ChatHub' srcRepository: Resgrid/Unit Length of output: 19189 Prevent
🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import { useFocusEffect } from 'expo-router'; | ||
| import { Redirect, useFocusEffect } from 'expo-router'; | ||
| import { RefreshCw, Send, Sparkles } from 'lucide-react-native'; | ||
| import React, { useCallback, useMemo, useState } from 'react'; | ||
| import { useTranslation } from 'react-i18next'; | ||
|
|
@@ -14,28 +14,33 @@ import { HStack } from '@/components/ui/hstack'; | |
| import { Input, InputField } from '@/components/ui/input'; | ||
| import { KeyboardAvoidingView } from '@/components/ui/keyboard-avoiding-view'; | ||
| import { Pressable } from '@/components/ui/pressable'; | ||
| import { Spinner } from '@/components/ui/spinner'; | ||
| import { Text } from '@/components/ui/text'; | ||
| import { VStack } from '@/components/ui/vstack'; | ||
| import { type ChatMessageResultData } from '@/models/v4/chat'; | ||
| import useAuthStore from '@/stores/auth/store'; | ||
| import { useChatStore } from '@/stores/chat/store'; | ||
| import { useChatSystemStatus } from '@/stores/feature-flags/store'; | ||
|
|
||
| export default function ChatbotScreen() { | ||
| const { t } = useTranslation(); | ||
| const currentUserId = useAuthStore((s) => s.userId); | ||
| const chatStatus = useChatSystemStatus(); | ||
| const isChatEnabled = chatStatus === 'enabled'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Magic string vulnerability across Kody rule violation: Use enums instead of magic strings Prompt for LLMTalk to Kody by mentioning @kody Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction. |
||
| const chatbotChannelId = useChatStore((s) => s.chatbotChannelId); | ||
| const chatbotTyping = useChatStore((s) => s.chatbotTyping); | ||
| const messages = useChatStore((s) => (chatbotChannelId ? s.messagesByChannel[chatbotChannelId] : undefined)); | ||
| const [text, setText] = useState(''); | ||
|
|
||
| useFocusEffect( | ||
| useCallback(() => { | ||
| if (!isChatEnabled) return; | ||
| const store = useChatStore.getState(); | ||
| void store.initChatbot(); | ||
| return () => { | ||
| useChatStore.getState().setActiveChannel(null); | ||
| }; | ||
| }, []) | ||
| }, [isChatEnabled]) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| ); | ||
|
|
||
| // Keep the assistant channel active while viewing so incoming messages don't inflate unread. | ||
|
|
@@ -61,6 +66,21 @@ export default function ChatbotScreen() { | |
| [currentUserId] | ||
| ); | ||
|
|
||
| // Chat.System flag not yet resolved: wait instead of redirecting away from a valid route. | ||
| if (chatStatus === 'unknown') { | ||
| return ( | ||
| <Box className="size-full flex-1 items-center justify-center bg-background-0"> | ||
| <FocusAwareStatusBar /> | ||
| <Spinner /> | ||
| </Box> | ||
| ); | ||
| } | ||
|
|
||
| // Chat.System feature flag off: the assistant rides on the chat system, hide it too. | ||
| if (chatStatus === 'disabled') { | ||
| return <Redirect href="/(app)" />; | ||
| } | ||
|
|
||
| return ( | ||
| <Box className="size-full flex-1 bg-background-0"> | ||
| <FocusAwareStatusBar /> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unhandled rejection: the awaited api.get call at src/api/feature-flags/feature-flags.ts:35 propagates network failures, 5xx responses, and aborts to callers with no context. Wrap the await in try/catch (or attach .catch), log the error, and rethrow or return a safe default.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.