Conversation
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughThe PR adds typed feature-flag API helpers and an identity-aware persisted Zustand store. App startup loads flags and gates SignalR connection. Chat screens, routes, data loading, chatbot initialization, sidebar navigation, and app reset use feature-flag state. ChangesChat feature-flag gating
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AppLayout
participant featureFlagsStore
participant ChatSignalR
participant ChatRoute
participant Sidebar
AppLayout->>featureFlagsStore: fetch ChatSystem flag
featureFlagsStore-->>AppLayout: return feature-flag status
AppLayout->>ChatSignalR: connect when enabled
ChatRoute->>featureFlagsStore: read ChatSystem status
ChatRoute->>ChatSignalR: load chat data when enabled
Sidebar->>featureFlagsStore: read ChatSystem status
Sidebar-->>Sidebar: render chat navigation when enabled
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/stores/feature-flags/store.ts (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the configured source alias.
Replace the relative storage import with
@/lib/storage.Proposed fix
-import { zustandStorage } from '../../lib/storage'; +import { zustandStorage } from '`@/lib/storage`';As per coding guidelines, use path aliases from
tsconfig.jsoninstead of relative paths insrc/.🤖 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` at line 7, Update the import in the feature-flags store to use the configured `@/lib/storage` alias instead of the relative ../../lib/storage path, leaving the imported zustandStorage symbol unchanged.Source: Coding guidelines
src/api/feature-flags/feature-flags.ts (1)
28-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required API endpoint factory.
These functions call
api.get()directly. UsecreateApiEndpointorcreateCachedApiEndpointwith typed response generics. This preserves the API-module contract for request handling and caching.🤖 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/api/feature-flags/feature-flags.ts` around lines 28 - 36, Update getAllFeatureFlags and getFeatureFlagState to use the required createApiEndpoint or createCachedApiEndpoint factories with their existing typed response generics instead of calling api.get directly. Preserve the current endpoint paths, query parameters, abort signal handling, and return values.Source: Coding guidelines
src/components/sidebar/sidebar-content.tsx (1)
68-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a ternary for conditional rendering.
Replace the
&&expression withisChatEnabled ? <HStack ... /> : null.Proposed fix
- {isChatEnabled && ( + {isChatEnabled ? ( <HStack space="md"> <Button variant="outline" action="secondary" size="md" className="flex-1" onPress={handleNavigateToChat}> <MessagesSquare size={18} color="`#2563eb`" /> <ButtonText className="ml-2">{t('tabs.chat')}</ButtonText> </Button> <Button variant="outline" action="secondary" size="md" className="flex-1" onPress={handleNavigateToAssistant}> <Sparkles size={18} color="`#7c3aed`" /> <ButtonText className="ml-2">{t('tabs.assistant')}</ButtonText> </Button> </HStack> - )} + ) : null}As per coding guidelines, use the conditional operator for conditional rendering instead of
&&.🤖 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/sidebar/sidebar-content.tsx` around lines 68 - 80, Update the conditional rendering in the sidebar content around isChatEnabled to use a ternary expression that renders the existing HStack when enabled and null otherwise, preserving the current navigation buttons and handlers.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)/_layout.tsx:
- Around line 168-189: The useSignalRLifecycle chat-hub lifecycle bypasses the
Chat.System feature gate. Update useSignalRLifecycle so resume connects ChatHub
and background disconnects include ChatHub only when featureFlagsStore reports
FeatureFlagKeys.ChatSystem enabled; preserve existing lifecycle handling for the
other hubs.
In `@src/app/`(app)/chatbot.tsx:
- Around line 33-41: Gate all chat lifecycle effects on isChatEnabled. In
src/app/(app)/chatbot.tsx lines 33-41, ensure the active-channel focus effect
skips both initialization and cleanup setActiveChannel mutations when disabled.
In src/app/chat/[channelId].tsx lines 70-81, add isChatEnabled guards to the
presence and mark-read effects, and cancel any in-flight presence loading when
chat is disabled.
In `@src/stores/feature-flags/store.ts`:
- Around line 29-59: Scope the persisted cache in featureFlagsStore to the
authenticated department so flags cannot bleed across department changes or
sign-out; use department identity in the persistence key or clear the store
during those transitions. Configure persist with partialize to retain only
cacheable flags, excluding isLoaded and error, while preserving the existing
failure behavior for the in-memory flags.
- Around line 63-67: The useFeatureFlag and fetchFlags flow must distinguish
unresolved flags from an explicitly disabled flag. Expose the current flag value
together with a resolved/loading state, set that state on both successful and
failed fetches, and update useIsChatEnabled consumers so chat redirects and
effects wait until the current department flag load has resolved.
---
Nitpick comments:
In `@src/api/feature-flags/feature-flags.ts`:
- Around line 28-36: Update getAllFeatureFlags and getFeatureFlagState to use
the required createApiEndpoint or createCachedApiEndpoint factories with their
existing typed response generics instead of calling api.get directly. Preserve
the current endpoint paths, query parameters, abort signal handling, and return
values.
In `@src/components/sidebar/sidebar-content.tsx`:
- Around line 68-80: Update the conditional rendering in the sidebar content
around isChatEnabled to use a ternary expression that renders the existing
HStack when enabled and null otherwise, preserving the current navigation
buttons and handlers.
In `@src/stores/feature-flags/store.ts`:
- Line 7: Update the import in the feature-flags store to use the configured
`@/lib/storage` alias instead of the relative ../../lib/storage path, leaving the
imported zustandStorage symbol unchanged.
🪄 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: 1606ed4c-faf9-4235-adfd-03f11d8cdf0e
📒 Files selected for processing (8)
src/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/sidebar-content.tsxsrc/stores/feature-flags/store.ts
| await Promise.all([useRolesStore.getState().init(), useCallsStore.getState().init(), useWeatherAlertsStore.getState().init(), securityStore.getState().getRights(), featureFlagsStore.getState().fetchFlags()]); | ||
|
|
||
| // 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', | ||
| }); |
There was a problem hiding this comment.
🎯 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 useSignalRLifecycle from reconnecting the chat hub when Chat.System is disabled.
useSignalRLifecycle runs connectChatHub() on resume and includes ChatHub in disconnects during background. That bypasses the Chat.System gate used in _layout.tsx; only add/remove the chat hub when the feature flag is true.
🤖 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)/_layout.tsx around lines 168 - 189, The useSignalRLifecycle
chat-hub lifecycle bypasses the Chat.System feature gate. Update
useSignalRLifecycle so resume connects ChatHub and background disconnects
include ChatHub only when featureFlagsStore reports FeatureFlagKeys.ChatSystem
enabled; preserve existing lifecycle handling for the other hubs.
| export const featureFlagsStore = create<FeatureFlagsState>()( | ||
| persist( | ||
| (set, get) => ({ | ||
| flags: {}, | ||
| isLoaded: false, | ||
| error: null, | ||
| fetchFlags: async () => { | ||
| try { | ||
| const response = await getAllFeatureFlags(); | ||
| const flags: Record<string, FeatureFlagEntry> = {}; | ||
| for (const flag of response?.Data ?? []) { | ||
| if (flag?.Key) { | ||
| flags[flag.Key] = { enabled: !!flag.Enabled, value: flag.Value ?? null }; | ||
| } | ||
| } | ||
| set({ flags, isLoaded: true, error: null }); | ||
| } catch (error) { | ||
| // Keep any persisted flags on failure so gating stays stable while offline. | ||
| logger.error({ | ||
| message: 'Failed to fetch feature flags', | ||
| context: { error }, | ||
| }); | ||
| set({ error: error instanceof Error ? error.message : 'Failed to fetch feature flags' }); | ||
| } | ||
| }, | ||
| isEnabled: (key: string, defaultValue = false) => get().flags[key]?.enabled ?? defaultValue, | ||
| }), | ||
| { | ||
| name: 'feature-flags-storage', | ||
| storage: createJSONStorage(() => zustandStorage), | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Scope persisted flags to the authenticated department.
Line 57 stores department-scoped flags under one device-wide key. Line 46 retains those values when refresh fails. A user who changes departments can then inherit the prior department's Chat.System value, which can expose chat navigation and start the chat hub incorrectly.
Clear this state on sign-out and department changes, or namespace the persisted cache by department identity. Persist only cacheable flag data with partialize; do not persist isLoaded or error.
As per coding guidelines, Zustand persistence must use partialize to exclude transient state such as loading and error flags.
🤖 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 29 - 59, Scope the persisted
cache in featureFlagsStore to the authenticated department so flags cannot bleed
across department changes or sign-out; use department identity in the
persistence key or clear the store during those transitions. Configure persist
with partialize to retain only cacheable flags, excluding isLoaded and error,
while preserving the existing failure behavior for the in-memory flags.
Source: Coding guidelines
| // Reactive hook; components re-render when the flag changes. Unknown flags default to disabled | ||
| // so gated features stay hidden until the server confirms them. | ||
| export const useFeatureFlag = (key: string, defaultValue = false) => featureFlagsStore((state) => state.flags[key]?.enabled ?? defaultValue); | ||
|
|
||
| export const useIsChatEnabled = () => useFeatureFlag(FeatureFlagKeys.ChatSystem); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not treat an unresolved flag as disabled.
useFeatureFlag() returns false before fetchFlags() completes. The app layout fetches flags during asynchronous initialization, while the chat routes immediately redirect when this hook returns false. A direct link to an enabled chat route can therefore redirect to /(app) before the current department flags load.
Expose a resolved state with the flag value. Defer chat redirects and chat effects until the current flag load has resolved. Set that state on both successful and failed fetches.
🤖 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 63 - 67, The useFeatureFlag
and fetchFlags flow must distinguish unresolved flags from an explicitly
disabled flag. Expose the current flag value together with a resolved/loading
state, set that state on both successful and failed fetches, and update
useIsChatEnabled consumers so chat redirects and effects wait until the current
department flag load has resolved.
|
|
||
| /** 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.
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
File src/api/feature-flags/feature-flags.ts:
Line 29:
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.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| /** 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.
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 LLM
File src/api/feature-flags/feature-flags.ts:
Line 29:
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.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| }; | ||
|
|
||
| /** Lightweight enabled-only check for a single flag. */ | ||
| export const getFeatureFlagState = async (key: string, signal?: AbortSignal) => { |
There was a problem hiding this comment.
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 @returns {Promise<FeatureToggleResult|undefined>} and document rejection conditions (network errors, unknown key, aborts).
Kody rule violation: Document async/Promise behavior and errors
Prompt for LLM
File src/api/feature-flags/feature-flags.ts:
Line 34:
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 `@returns {Promise<FeatureToggleResult|undefined>}` and document rejection conditions (network errors, unknown key, aborts).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| // 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.
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 LLM
File src/app/(app)/_layout.tsx:
Line 168:
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.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| isEnabled: (key: string, defaultValue?: boolean) => boolean; | ||
| } | ||
|
|
||
| export const featureFlagsStore = create<FeatureFlagsState>()( |
There was a problem hiding this comment.
Stale state leak across user sessions: featureFlagsStore is omitted from resetAllStores() in src/services/app-reset.service.ts (lines 258-319), so the previous user's flags persist in-memory after logout and briefly surface for the next user until fetchFlags() completes during re-login. Add featureFlagsStore.setState({ flags: {}, isLoaded: false, error: null }) alongside the existing department-scoped store resets at lines 310-314.
// In app-reset.service.ts resetAllStores(), alongside other department-scoped resets:
// import { featureFlagsStore } from '@/stores/feature-flags/store';
// featureFlagsStore.setState({ flags: {}, isLoaded: false, error: null });Prompt for LLM
File src/stores/feature-flags/store.ts:
Line 29:
Stale state leak across user sessions: featureFlagsStore is omitted from resetAllStores() in src/services/app-reset.service.ts (lines 258-319), so the previous user's flags persist in-memory after logout and briefly surface for the next user until fetchFlags() completes during re-login. Add `featureFlagsStore.setState({ flags: {}, isLoaded: false, error: null })` alongside the existing department-scoped store resets at lines 310-314.
Suggested Code:
// In app-reset.service.ts resetAllStores(), alongside other department-scoped resets:
// import { featureFlagsStore } from '@/stores/feature-flags/store';
// featureFlagsStore.setState({ flags: {}, isLoaded: false, error: null });
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| isEnabled: (key: string, defaultValue?: boolean) => boolean; | ||
| } | ||
|
|
||
| export const featureFlagsStore = create<FeatureFlagsState>()( |
There was a problem hiding this comment.
Cross-account isolation violation: featureFlagsStore is omitted from resetAllStores() (src/services/app-reset.service.ts:258), and since logout performs an in-place navigation without a JS bundle reload, the previous user's flags persist in module-level zustand state — surfacing disabled features (e.g., Chat.System) to the next user if their fetchFlags() fails offline. Export an INITIAL_FEATURE_FLAGS_STATE constant and call featureFlagsStore.setState() alongside the other department-scoped resets to restore the auth/store.tsx:172-174 isolation guarantee.
// In app-reset.service.ts resetAllStores(), alongside the other department-scoped stores:
// featureFlagsStore.setState({ flags: {}, isLoaded: false, error: null });
//
// To keep it encapsulated, add to the store:
// export const INITIAL_FEATURE_FLAGS_STATE = { flags: {}, isLoaded: false, error: null };
// and call featureFlagsStore.setState(INITIAL_FEATURE_FLAGS_STATE); during reset.Prompt for LLM
File src/stores/feature-flags/store.ts:
Line 29:
Cross-account isolation violation: featureFlagsStore is omitted from resetAllStores() (src/services/app-reset.service.ts:258), and since logout performs an in-place navigation without a JS bundle reload, the previous user's flags persist in module-level zustand state — surfacing disabled features (e.g., Chat.System) to the next user if their fetchFlags() fails offline. Export an INITIAL_FEATURE_FLAGS_STATE constant and call featureFlagsStore.setState() alongside the other department-scoped resets to restore the auth/store.tsx:172-174 isolation guarantee.
Suggested Code:
// In app-reset.service.ts resetAllStores(), alongside the other department-scoped stores:
// featureFlagsStore.setState({ flags: {}, isLoaded: false, error: null });
//
// To keep it encapsulated, add to the store:
// export const INITIAL_FEATURE_FLAGS_STATE = { flags: {}, isLoaded: false, error: null };
// and call featureFlagsStore.setState(INITIAL_FEATURE_FLAGS_STATE); during reset.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| logger.error({ | ||
| message: 'Failed to fetch feature flags', | ||
| context: { error }, | ||
| }); |
There was a problem hiding this comment.
Unstructured log fields: the error log embeds the operation name inside the message string and omits structured identifiers (trace ID, flag keys), violating Rule 3's SIEM-searchability requirement. Add a dedicated operation field and include identifiers — e.g., logger.error({ message: 'Failed to fetch feature flags', operation: 'fetchFlags', context: { error, traceId } }).
Kody rule violation: Include error context in structured logs
Prompt for LLM
File src/stores/feature-flags/store.ts:
Line 47 to 50:
Unstructured log fields: the error log embeds the operation name inside the message string and omits structured identifiers (trace ID, flag keys), violating Rule 3's SIEM-searchability requirement. Add a dedicated `operation` field and include identifiers — e.g., `logger.error({ message: 'Failed to fetch feature flags', operation: 'fetchFlags', context: { error, traceId } })`.
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:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/stores/feature-flags/__tests__/store.test.ts (1)
175-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnmount each hook test.
Each
renderHook()call leaves the mounted hook active. Destructureunmountand call it after the assertions in each test.As per coding guidelines, “Always call
unmount()in tests to clean up after assertions.”🤖 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 175 - 211, Update each useChatSystemStatus test to destructure unmount from its renderHook result and call unmount after the assertions; in the enabled/disabled test, unmount each separately rendered hook before or after updating state while preserving the existing assertions.Source: Coding guidelines
src/stores/feature-flags/store.ts (1)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse configured aliases for these source-module references.
Replace relative module specifiers with the configured
@/*aliases.
src/stores/feature-flags/store.ts#L8-L9: import the auth and security stores through@/stores/....src/stores/signalr/signalr-store.ts#L10-L10: import the feature-flag store through@/stores/feature-flags/store.src/stores/feature-flags/__tests__/store.test.ts#L3-L3: import the feature-flag store through its@/stores/...alias.src/stores/feature-flags/__tests__/store.test.ts#L21-L21: mock storage through@/lib/storage.src/stores/feature-flags/__tests__/store.test.ts#L30-L30: mock auth through@/stores/auth/store.src/stores/feature-flags/__tests__/store.test.ts#L37-L37: mock security through@/stores/security/store.src/stores/feature-flags/__tests__/store.test.ts#L43-L45: retrieve mocks through the same alias specifiers.As per coding guidelines, “Use path aliases from tsconfig.json (
@/*,@env,@assets/*) instead of relative paths.”🤖 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 8 - 9, Replace the relative imports and mock references with configured `@/`* aliases: in src/stores/feature-flags/store.ts:8-9 use `@/stores/auth/store` and `@/stores/security/store`; in src/stores/signalr/signalr-store.ts:10 use `@/stores/feature-flags/store`; and in src/stores/feature-flags/__tests__/store.test.ts:3,21,30,37,43-45 use the corresponding `@/stores/feature-flags/store`, `@/lib/storage`, `@/stores/auth/store`, and `@/stores/security/store` specifiers consistently for imports, mocks, and mock retrieval.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/signalr/__tests__/signalr-store.test.ts`:
- Around line 51-59: Add a test for connectChatHub using the mocked
featureFlagsStore to return false for Chat.System, then assert
connectToHubWithEventingUrl is not called and isChatHubConnected remains false.
Keep the existing enabled and handling-path coverage unchanged.
---
Nitpick comments:
In `@src/stores/feature-flags/__tests__/store.test.ts`:
- Around line 175-211: Update each useChatSystemStatus test to destructure
unmount from its renderHook result and call unmount after the assertions; in the
enabled/disabled test, unmount each separately rendered hook before or after
updating state while preserving the existing assertions.
In `@src/stores/feature-flags/store.ts`:
- Around line 8-9: Replace the relative imports and mock references with
configured `@/`* aliases: in src/stores/feature-flags/store.ts:8-9 use
`@/stores/auth/store` and `@/stores/security/store`; in
src/stores/signalr/signalr-store.ts:10 use `@/stores/feature-flags/store`; and in
src/stores/feature-flags/__tests__/store.test.ts:3,21,30,37,43-45 use the
corresponding `@/stores/feature-flags/store`, `@/lib/storage`, `@/stores/auth/store`,
and `@/stores/security/store` specifiers consistently for imports, mocks, and mock
retrieval.
🪄 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: 3eb655bf-0955-4dd5-8766-2d1daba2445b
📒 Files selected for processing (11)
src/app/(app)/chat.tsxsrc/app/(app)/chatbot.tsxsrc/app/chat/[channelId].tsxsrc/app/chat/thread/[messageId].tsxsrc/services/__tests__/app-reset.service.test.tssrc/services/app-reset.service.tssrc/stores/feature-flags/__tests__/store.test.tssrc/stores/feature-flags/store.tssrc/stores/signalr/__tests__/signalr-store.test.tssrc/stores/signalr/__tests__/zz-dbg.test.tssrc/stores/signalr/signalr-store.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/chat/[channelId].tsx
- src/app/chat/thread/[messageId].tsx
| // Feature flags: default the chat flag to enabled so connectChatHub is not short-circuited. | ||
| jest.mock('../../feature-flags/store', () => ({ | ||
| FeatureFlagKeys: { ChatSystem: 'Chat.System' }, | ||
| featureFlagsStore: { | ||
| getState: jest.fn(() => ({ | ||
| isEnabled: jest.fn(() => true), | ||
| })), | ||
| }, | ||
| })); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'connectChatHub|isEnabled|ChatSystem|connectToHubWithEventingUrl|isChatHubConnected' src/stores/signalr/__tests__/signalr-store.test.tsRepository: Resgrid/Unit
Length of output: 2800
Add disabled Chat.System coverage for connectChatHub.
The current mock enables Chat.System for every test, and the returned matches only assert the enabled/handling paths. Add a case that sets featureFlagsStore.getState().isEnabled() to false for the default/hub flag and asserts that connectToHubWithEventingUrl() is not called and isChatHubConnected does not become true.
🤖 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/signalr/__tests__/signalr-store.test.ts` around lines 51 - 59, Add
a test for connectChatHub using the mocked featureFlagsStore to return false for
Chat.System, then assert connectToHubWithEventingUrl is not called and
isChatHubConnected remains false. Keep the existing enabled and handling-path
coverage unchanged.
Source: Coding guidelines
|
Approve |
| const { t } = useTranslation(); | ||
| const currentUserId = useAuthStore((s) => s.userId); | ||
| const chatStatus = useChatSystemStatus(); | ||
| const isChatEnabled = chatStatus === 'enabled'; |
There was a problem hiding this comment.
Magic string vulnerability across src/app/(app)/chatbot.tsx and src/app/(app)/chat.tsx compares the chat system status against the raw string literal 'enabled', risking silent failures from typos. Define an enum such as enum ChatSystemStatus { Enabled = 'enabled', Disabled = 'disabled', Unknown = 'unknown' } and compare against ChatSystemStatus.Enabled to centralize domain knowledge and enforce compile-time checks.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File src/app/(app)/chatbot.tsx:
Line 29:
Magic string vulnerability across `src/app/(app)/chatbot.tsx` and `src/app/(app)/chat.tsx` compares the chat system status against the raw string literal 'enabled', risking silent failures from typos. Define an enum such as `enum ChatSystemStatus { Enabled = 'enabled', Disabled = 'disabled', Unknown = 'unknown' }` and compare against `ChatSystemStatus.Enabled` to centralize domain knowledge and enforce compile-time checks.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const userId = useAuthStore.getState().userId; | ||
| const departmentId = securityStore.getState().rights?.DepartmentId; |
There was a problem hiding this comment.
Duplicated state access in src/stores/feature-flags/store.ts re-fetches userId and departmentId exactly as getCurrentIdentityKey does at lines 25-26, risking implementation drift. Extract a helper such as getIdentityParts(): { userId: string | null; departmentId: string | null } or reuse getCurrentIdentityKey() to synchronize auth and security store access.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File src/stores/feature-flags/store.ts:
Line 39 to 40:
Duplicated state access in `src/stores/feature-flags/store.ts` re-fetches `userId` and `departmentId` exactly as `getCurrentIdentityKey` does at lines 25-26, risking implementation drift. Extract a helper such as `getIdentityParts(): { userId: string | null; departmentId: string | null }` or reuse `getCurrentIdentityKey()` to synchronize auth and security store access.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| return { securityStore: mockSecurityStore }; | ||
| }); | ||
| jest.mock('../../feature-flags/store', () => ({ | ||
| FeatureFlagKeys: { ChatSystem: 'Chat.System' }, |
There was a problem hiding this comment.
Hardcoded mock constant in src/stores/signalr/__tests__/zz-dbg.test.ts duplicates the string literal 'Chat.System' for FeatureFlagKeys.ChatSystem, risking silent test drift and violating Rule [6]. Use const actual = jest.requireActual('../../feature-flags/store') to pull the real FeatureFlagKeys into the mock factory, then spread ...actual.FeatureFlagKeys while only overriding featureFlagsStore.
Kody rule violation: Centralize string constants
Prompt for LLM
File src/stores/signalr/__tests__/zz-dbg.test.ts:
Line 35:
Hardcoded mock constant in `src/stores/signalr/__tests__/zz-dbg.test.ts` duplicates the string literal 'Chat.System' for `FeatureFlagKeys.ChatSystem`, risking silent test drift and violating Rule [6]. Use `const actual = jest.requireActual('../../feature-flags/store')` to pull the real `FeatureFlagKeys` into the mock factory, then spread `...actual.FeatureFlagKeys` while only overriding `featureFlagsStore`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
This pull request introduces a feature flag system and uses it to gate the entire chat feature behind a department-scoped
Chat.Systemtoggle.Feature flag infrastructure:
useFeatureFlag,useIsChatEnabled) for components.Chat gating by feature flag:
Chat.Systemis enabled for the department.In summary, when the
Chat.Systemfeature flag is disabled for a department, the app fully hides and deactivates all chat functionality—navigation entry points, screen access, data loading, and realtime hub connections.Summary by CodeRabbit