Conversation
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughThe PR adds a feature-flags API and persisted Zustand store. App initialization loads flags before chat connection. Chat screens, chatbot flows, and sidebar entries use ChangesChat feature flag integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AppLayout
participant featureFlagsStore
participant ChatSignalRHub
participant ChatScreens
AppLayout->>featureFlagsStore: fetchFlags()
featureFlagsStore-->>AppLayout: Return ChatSystem status
AppLayout->>ChatSignalRHub: connectChatHub() when enabled
ChatScreens->>featureFlagsStore: Read ChatSystem status
ChatScreens->>ChatScreens: Show spinner, redirect, or run chat operations
Possibly related PRs
🚥 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: 2
🧹 Nitpick comments (5)
src/api/feature-flags/feature-flags.ts (3)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the configured alias for the API client.
Replace the relative import with
@/api/common/client.As per coding guidelines,
src/**/*.{ts,tsx}must use configured path aliases instead of relative imports.🤖 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` at line 1, Update the API client import in feature-flags.ts to use the configured "`@/api/common/client`" alias instead of the relative path, preserving the existing api symbol.Source: Coding guidelines
28-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the API endpoint abstraction.
Define both requests through
createApiEndpointorcreateCachedApiEndpoint. Do not callapi.getdirectly from this API module.As per coding guidelines,
src/api/**/*.tsmust define API endpoints throughcreateApiEndpointorcreateCachedApiEndpoint.🤖 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, Replace the direct api.get calls in getAllFeatureFlags and getFeatureFlagState with endpoint definitions created through createApiEndpoint or createCachedApiEndpoint, then invoke those abstractions while preserving the existing paths, response types, parameters, and AbortSignal support.Source: Coding guidelines
10-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove feature-toggle response models to the v4 model layer.
Move
FeatureToggleData,FeatureTogglesResult, andFeatureToggleResultintosrc/models/v4/, organized by domain. Import those models withimport type.As per coding guidelines, API response models must remain in
src/models/v4/, organized by domain.🤖 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 10 - 25, Move the FeatureToggleData, FeatureTogglesResult, and FeatureToggleResult interfaces out of the feature-flags API module into the appropriate domain-organized files under src/models/v4/. Update the feature-flags implementation to import these models using import type, preserving their existing shapes and usage.Source: Coding guidelines
src/app/(app)/chat.tsx (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeparate
Hrefinto a type-only import.
src/app/(app)/chat.tsx#L1-L1: importHrefwithimport typeafter value imports.src/app/chat/[channelId].tsx#L2-L2: importHrefwithimport typeafter value imports.As per coding guidelines, type-only imports must use
import type, and type imports must follow value imports.🤖 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)/chat.tsx at line 1, Update the imports in src/app/(app)/chat.tsx at lines 1-1 and src/app/chat/[channelId].tsx at lines 2-2 so Href is removed from value imports and added via a separate import type statement after the remaining value imports.Source: Coding guidelines
src/stores/feature-flags/store.ts (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the configured alias for storage.
Replace
../../lib/storagewith@/lib/storage.As per coding guidelines,
src/**/*.{ts,tsx}must use configured path aliases instead of relative imports.🤖 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 storage import in the feature-flag store to use the configured "`@/lib/storage`" alias instead of the relative "../../lib/storage" path, while leaving the zustandStorage usage unchanged.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/store.ts`:
- Around line 56-59: Update the Zustand persistence configuration for
feature-flags-storage to scope persisted data by the authenticated department
identity or clear it whenever that identity changes, preventing reuse across
sessions. Add a partialize option that persists only flags and excludes isLoaded
and error, while preserving the existing storage behavior.
- Around line 63-67: The feature-flag hook currently conflates unresolved flags
with disabled flags, causing premature chat redirects. In
src/stores/feature-flags/store.ts lines 63-67, update useFeatureFlag to expose
an unresolved state while preserving an explicitly disabled result; in
src/app/(app)/chat.tsx lines 109-112, src/app/(app)/chatbot.tsx lines 67-70,
src/app/chat/[channelId].tsx lines 254-257, and
src/app/chat/thread/[messageId].tsx lines 106-109, defer redirects until the
flag resolves as disabled, while continuing to allow fetches and chat actions
only when the flag is explicitly enabled.
---
Nitpick comments:
In `@src/api/feature-flags/feature-flags.ts`:
- Line 1: Update the API client import in feature-flags.ts to use the configured
"`@/api/common/client`" alias instead of the relative path, preserving the
existing api symbol.
- Around line 28-36: Replace the direct api.get calls in getAllFeatureFlags and
getFeatureFlagState with endpoint definitions created through createApiEndpoint
or createCachedApiEndpoint, then invoke those abstractions while preserving the
existing paths, response types, parameters, and AbortSignal support.
- Around line 10-25: Move the FeatureToggleData, FeatureTogglesResult, and
FeatureToggleResult interfaces out of the feature-flags API module into the
appropriate domain-organized files under src/models/v4/. Update the
feature-flags implementation to import these models using import type,
preserving their existing shapes and usage.
In `@src/app/`(app)/chat.tsx:
- Line 1: Update the imports in src/app/(app)/chat.tsx at lines 1-1 and
src/app/chat/[channelId].tsx at lines 2-2 so Href is removed from value imports
and added via a separate import type statement after the remaining value
imports.
In `@src/stores/feature-flags/store.ts`:
- Line 7: Update the storage import in the feature-flag store to use the
configured "`@/lib/storage`" alias instead of the relative "../../lib/storage"
path, while leaving the zustandStorage usage 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: bc276eb6-58e5-4db0-a6f1-d29a0d1a9084
📒 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
| { | ||
| name: 'feature-flags-storage', | ||
| storage: createJSONStorage(() => zustandStorage), | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Scope and reduce persisted feature-flag state.
feature-flags-storage persists department-scoped flags under one device-wide key. After an account or department change, or after a failed refresh, the new session can reuse the prior department's Chat.System value.
Scope the storage by authenticated department identity, or clear it on identity changes. Add partialize so persistence retains only flags; do not persist isLoaded or error.
As per coding guidelines, src/stores/**/*.ts must use Zustand persistence with 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 56 - 59, Update the Zustand
persistence configuration for feature-flags-storage to scope persisted data by
the authenticated department identity or clear it whenever that identity
changes, preventing reuse across sessions. Add a partialize option that persists
only flags and excludes isLoaded and error, while preserving the existing
storage behavior.
Source: Coding guidelines
|
|
||
| /** 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 promise rejection: the awaited api.get call at src/api/feature-flags/feature-flags.ts:35 propagates to the caller with no structured context on network errors or non-2xx responses. Wrap the call in try { ... } catch (e) { /* log with context or rethrow mapped error */ }.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File src/api/feature-flags/feature-flags.ts:
Line 29:
Unhandled promise rejection: the awaited `api.get` call at `src/api/feature-flags/feature-flags.ts:35` propagates to the caller with no structured context on network errors or non-2xx responses. Wrap the call in `try { ... } catch (e) { /* log with context or rethrow mapped error */ }`.
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 exception: the external HTTP call at src/api/feature-flags/feature-flags.ts:35 is not wrapped in try/catch, so network/file/external failures propagate unhandled with no context or application-level error mapping. Wrap the call in try { ... } catch (e) { /* add context (endpoint, operation) and map to app 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 exception: the external HTTP call at `src/api/feature-flags/feature-flags.ts:35` is not wrapped in try/catch, so network/file/external failures propagate unhandled with no context or application-level error mapping. Wrap the call in `try { ... } catch (e) { /* add context (endpoint, operation) and map to app error */ }`.
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) => { |
There was a problem hiding this comment.
Incomplete JSDoc: getAllFeatureFlags (line 27) omits @returns {Promise<FeatureTogglesResult>} and rejection conditions, leaving callers without knowledge of the resolved type, rejection scenarios, or that it must be used with await. Add @returns {Promise<FeatureTogglesResult>} and document rejection scenarios (e.g., network failure, auth error).
Kody rule violation: Document async/Promise behavior and errors
Prompt for LLM
File src/api/feature-flags/feature-flags.ts:
Line 28:
Incomplete JSDoc: `getAllFeatureFlags` (line 27) omits `@returns {Promise<FeatureTogglesResult>}` and rejection conditions, leaving callers without knowledge of the resolved type, rejection scenarios, or that it must be used with `await`. Add `@returns {Promise<FeatureTogglesResult>}` and document rejection scenarios (e.g., network failure, auth error).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const isChatEnabled = useIsChatEnabled(); | ||
|
|
||
| // Chat.System feature flag off: hide the chat and assistant entries entirely. | ||
| const menuItems = MENU_ITEMS.filter((item) => (item.key === 'chat' || item.key === 'chatbot' ? isChatEnabled : true)); |
There was a problem hiding this comment.
Magic strings: the literals 'chat' and 'chatbot' are used as inline item-key comparisons but are shared domain identifiers also referenced in MENU_ITEMS definitions and route configs. Define constants (e.g., MENU_KEY.CHAT = 'chat', MENU_KEY.CHATBOT = 'chatbot') or a const tuple and reference them in both the filter and the MENU_ITEMS array.
Kody rule violation: Centralize string constants
Prompt for LLM
File src/components/sidebar/sidebar-content.tsx:
Line 42:
Magic strings: the literals `'chat'` and `'chatbot'` are used as inline item-key comparisons but are shared domain identifiers also referenced in `MENU_ITEMS` definitions and route configs. Define constants (e.g., `MENU_KEY.CHAT = 'chat'`, `MENU_KEY.CHATBOT = 'chatbot'`) or a const tuple and reference them in both the filter and the `MENU_ITEMS` array.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const isChatEnabled = useIsChatEnabled(); | ||
|
|
||
| // Chat.System feature flag off: hide the chat and assistant entries entirely. | ||
| const menuItems = MENU_ITEMS.filter((item) => (item.key === 'chat' || item.key === 'chatbot' ? isChatEnabled : true)); |
There was a problem hiding this comment.
Magic strings: the literals 'chat' and 'chatbot' represent a finite set of menu item keys but lack compile-time safety, making refactoring error-prone. Declare an enum or as-const tuple (e.g., const MENU_KEYS = ['chat','chatbot',...] as const) and compare against MENU_KEYS.Chat / MENU_KEYS.Chatbot instead of raw strings.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File src/components/sidebar/sidebar-content.tsx:
Line 42:
Magic strings: the literals `'chat'` and `'chatbot'` represent a finite set of menu item keys but lack compile-time safety, making refactoring error-prone. Declare an enum or as-const tuple (e.g., `const MENU_KEYS = ['chat','chatbot',...] as const`) and compare against `MENU_KEYS.Chat` / `MENU_KEYS.Chatbot` instead of raw strings.
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.
Security vulnerability: resetAllStores() in app-reset.service.ts omits featureFlagsStore, so the in-memory zustand state retains the previous department's flags after logout even though the MMKV key is deleted. Since fetchFlags() preserves existing flags on network failure (store.ts:46), an offline re-login as a different department leaves the previous department's Chat.System flag active — register the store in resetAllStores() by adding featureFlagsStore.setState({ flags: {}, isLoaded: false, error: null });.
// Add to resetAllStores() in src/services/app-reset.service.ts:
// 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:
Security vulnerability: `resetAllStores()` in `app-reset.service.ts` omits `featureFlagsStore`, so the in-memory zustand state retains the previous department's flags after logout even though the MMKV key is deleted. Since `fetchFlags()` preserves existing flags on network failure (`store.ts:46`), an offline re-login as a different department leaves the previous department's `Chat.System` flag active — register the store in `resetAllStores()` by adding `featureFlagsStore.setState({ flags: {}, isLoaded: false, error: null });`.
Suggested Code:
// Add to resetAllStores() in src/services/app-reset.service.ts:
// 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.
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: 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: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftInvalidate active feature-flag requests during reset.
fetchFlags()can complete afterresetAllStores()clears the store. Its success or failure path can then restore the prior session state and temporarily enableChat.Systemfor the wrong session.
src/stores/feature-flags/store.ts#L66-L91: associate each fetch with a request generation and apply success or failure state only when that generation remains current. Add a deferred-promise test for this case.src/services/app-reset.service.ts#L205-L205: invalidate the active feature-flag request generation before resetting the store state.🤖 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, Associate each fetch started by fetchFlags in src/stores/feature-flags/store.ts:66-91 with a request generation, and apply its success or failure state only if that generation is still current; add a deferred-promise test covering reset during an active fetch. In src/services/app-reset.service.ts:205-205, invalidate the active feature-flag request generation before resetting store state.
🧹 Nitpick comments (3)
src/stores/feature-flags/__tests__/store.test.ts (3)
43-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace untyped
requirecalls.
require()returnsany. Use typed Jest mocks or typed alias imports forgetAllFeatureFlags,useAuthStore, andsecurityStore.As per coding guidelines,
**/*.{ts,tsx}must use strict TypeScript and never useany.🤖 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 43 - 45, Replace the untyped require calls for getAllFeatureFlags, useAuthStore, and securityStore with typed ES module imports or properly typed Jest mock imports. Preserve each symbol’s existing default or named import shape, and ensure no any-typed require usage remains in the test.Source: Coding guidelines
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare the storage mock before importing the store.
Line 3 imports
../storebefore Line 21 mocks its storage dependency. Move native storage mocks before store imports so tests cannot initialize the real storage implementation.As per coding guidelines,
src/**/__tests__/**/*.{ts,tsx}must mock native modules before imports.Also applies to: 21-27
🤖 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 1 - 3, Move the native storage mock setup ahead of the `../store` import in `store.test.ts`, ensuring the mock is declared before `FeatureFlagKeys`, `featureFlagsStore`, and `useChatSystemStatus` are imported. Preserve the existing mock behavior while preventing the store from initializing the real storage implementation.Source: Coding guidelines
175-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnmount each rendered hook.
Each
renderHook()call creates a mounted subscription to the Zustand store. Callunmount()during test cleanup.As per coding guidelines,
src/**/__tests__/**/*.{ts,tsx}must always callunmount()during cleanup.🤖 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 - 212, Update each useChatSystemStatus test’s renderHook usage to retain the returned unmount function and invoke it during cleanup before the test completes, including both hooks created in the enabled/disabled test. Ensure every mounted hook subscription is explicitly unmounted while preserving the existing assertions.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/store.ts`:
- Around line 8-9: Replace all listed internal relative imports with configured
aliases: in src/stores/feature-flags/store.ts lines 8-9, alias the auth and
security store imports; in src/stores/feature-flags/__tests__/store.test.ts
lines 3, 21, 30, and 37, alias the feature-flags store, storage, auth-store, and
security-store imports; and in src/stores/signalr/signalr-store.ts line 11,
alias the feature-flags store import. Use the existing `@/stores/`... and
`@/lib/storage` paths without changing behavior.
In `@src/stores/signalr/signalr-store.ts`:
- Around line 424-429: Add a test in signalr-store.test.ts for connectChatHub()
with featureFlagsStore.isEnabled(FeatureFlagKeys.ChatSystem) mocked to false;
assert signalRService.connectToHubWithEventingUrl is not called and
isChatHubConnected remains false.
---
Outside diff comments:
In `@src/stores/feature-flags/store.ts`:
- Around line 66-91: Associate each fetch started by fetchFlags in
src/stores/feature-flags/store.ts:66-91 with a request generation, and apply its
success or failure state only if that generation is still current; add a
deferred-promise test covering reset during an active fetch. In
src/services/app-reset.service.ts:205-205, invalidate the active feature-flag
request generation before resetting store state.
---
Nitpick comments:
In `@src/stores/feature-flags/__tests__/store.test.ts`:
- Around line 43-45: Replace the untyped require calls for getAllFeatureFlags,
useAuthStore, and securityStore with typed ES module imports or properly typed
Jest mock imports. Preserve each symbol’s existing default or named import
shape, and ensure no any-typed require usage remains in the test.
- Around line 1-3: Move the native storage mock setup ahead of the `../store`
import in `store.test.ts`, ensuring the mock is declared before
`FeatureFlagKeys`, `featureFlagsStore`, and `useChatSystemStatus` are imported.
Preserve the existing mock behavior while preventing the store from initializing
the real storage implementation.
- Around line 175-212: Update each useChatSystemStatus test’s renderHook usage
to retain the returned unmount function and invoke it during cleanup before the
test completes, including both hooks created in the enabled/disabled test.
Ensure every mounted hook subscription is explicitly unmounted while preserving
the existing assertions.
🪄 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: 3c89b632-dc87-4cd7-ac74-f043d934e994
📒 Files selected for processing (9)
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/signalr-store.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/(app)/chat.tsx
- src/app/(app)/chatbot.tsx
| import useAuthStore from '../auth/store'; | ||
| import { securityStore } from '../security/store'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use configured aliases for internal imports.
These new internal imports bypass the configured aliases.
src/stores/feature-flags/store.ts#L8-L9: replace the auth and security relative imports with@/stores/...aliases.src/stores/feature-flags/__tests__/store.test.ts#L3-L3: replace../storewith the feature-flags store alias.src/stores/feature-flags/__tests__/store.test.ts#L21-L21: replace the storage mock path with@/lib/storage.src/stores/feature-flags/__tests__/store.test.ts#L30-L30: replace the auth-store mock path with its alias.src/stores/feature-flags/__tests__/store.test.ts#L37-L37: replace the security-store mock path with its alias.src/stores/signalr/signalr-store.ts#L11-L11: replace the feature-flags store relative import with its alias.
As per coding guidelines, src/**/*.{ts,tsx} must use configured path aliases (@/*, @env, @assets/*) instead of relative imports.
📍 Affects 3 files
src/stores/feature-flags/store.ts#L8-L9(this comment)src/stores/feature-flags/__tests__/store.test.ts#L3-L3src/stores/feature-flags/__tests__/store.test.ts#L21-L21src/stores/feature-flags/__tests__/store.test.ts#L30-L30src/stores/feature-flags/__tests__/store.test.ts#L37-L37src/stores/signalr/signalr-store.ts#L11-L11
🤖 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 all listed
internal relative imports with configured aliases: in
src/stores/feature-flags/store.ts lines 8-9, alias the auth and security store
imports; in src/stores/feature-flags/__tests__/store.test.ts lines 3, 21, 30,
and 37, alias the feature-flags store, storage, auth-store, and security-store
imports; and in src/stores/signalr/signalr-store.ts line 11, alias the
feature-flags store import. Use the existing `@/stores/`... and `@/lib/storage`
paths without changing behavior.
Source: Coding guidelines
| // Guard here so every call path (init, app-resume reconnect) honors the flag. | ||
| if (!featureFlagsStore.getState().isEnabled(FeatureFlagKeys.ChatSystem)) { | ||
| logger.info({ message: 'Chat disabled by feature flag; skipping chat hub connection' }); | ||
| return; | ||
| } | ||
|
|
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 6 'connectChatHub|ChatSystem|connectToHubWithEventingUrl|isChatHubConnected' \
src/stores/signalr/__tests__/signalr-store.test.tsRepository: Resgrid/IC
Length of output: 3112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate signalr store and tests"
git ls-files | rg 'src/stores/signalr/(signalr-store|.*test).*\.ts$'
echo
echo "Relevant test file around feature flag/chat-related tests"
wc -l src/stores/signalr/__tests__/signalr-store.test.ts
rg -n -C 8 'chat|Chat|connectChatHub|connectUpdateHub|connectGeolocationHub|isEnabled|ChatSystem' src/stores/signalr/__tests__/signalr-store.test.ts
echo
echo "signalr-store relevant implementation around connectChatHub"
rg -n -C 20 'connectChatHub|FeatureFlagKeys\.ChatSystem|isChatHubConnected|connectToHubWithEventingUrl' src/stores/signalr/signalr-store.tsRepository: Resgrid/IC
Length of output: 16943
Add a test for disabled chat-hub connection.
signalr-store.test.ts covers the update and geolocation hubs, but it does not cover connectChatHub() when Chat.System is disabled. Add a test that mocks featureFlagsStore.isEnabled(FeatureFlagKeys.ChatSystem) to false, calls connectChatHub(), and asserts signalRService.connectToHubWithEventingUrl is not called while isChatHubConnected remains false.
🤖 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/signalr-store.ts` around lines 424 - 429, Add a test in
signalr-store.test.ts for connectChatHub() with
featureFlagsStore.isEnabled(FeatureFlagKeys.ChatSystem) mocked to false; assert
signalRService.connectToHubWithEventingUrl is not called and isChatHubConnected
remains false.
Source: Coding guidelines
| const [fabOpen, setFabOpen] = useState(false); | ||
| const [newMode, setNewMode] = useState<'dm' | 'group' | null>(null); | ||
| const chatStatus = useChatSystemStatus(); | ||
| const isChatEnabled = chatStatus === 'enabled'; |
There was a problem hiding this comment.
Hardcoded string literal 'enabled' represents a finite set of chat system statuses and risks typos. Export named constants like CHAT_STATUS from the feature-flags store and compare against CHAT_STATUS.ENABLED.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File src/app/(app)/chat.tsx:
Line 92:
Hardcoded string literal 'enabled' represents a finite set of chat system statuses and risks typos. Export named constants like `CHAT_STATUS` from the feature-flags store and compare against `CHAT_STATUS.ENABLED`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const channelMessages = useChatStore((s) => (channelId ? s.messagesByChannel[channelId] : undefined)); | ||
| const [fetchedReplies, setFetchedReplies] = useState<ChatMessageResultData[]>([]); | ||
| const chatStatus = useChatSystemStatus(); | ||
| const isChatEnabled = chatStatus === 'enabled'; |
There was a problem hiding this comment.
Hardcoded string literal 'enabled' checks chat system status inline, creating synchronization risks across components. Define a constant like CHAT_STATUS.ENABLED or derive it from the feature-flag store's return type.
Kody rule violation: Centralize string constants
Prompt for LLM
File src/app/chat/thread/[messageId].tsx:
Line 32:
Hardcoded string literal 'enabled' checks chat system status inline, creating synchronization risks across components. Define a constant like `CHAT_STATUS.ENABLED` or derive it from the feature-flag store's return type.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| if (!persistedKey) { | ||
| return false; | ||
| } | ||
| const userId = useAuthStore.getState().userId; |
There was a problem hiding this comment.
Duplicated store-access logic fetches userId identically in getCurrentIdentityKey and isPersistedIdentityStale. Extract a shared helper like getCurrentIdentity() to centralize identity retrieval.
Kody rule violation: Extract duplicated logic into functions
Prompt for LLM
File src/stores/feature-flags/store.ts:
Line 39:
Duplicated store-access logic fetches `userId` identically in `getCurrentIdentityKey` and `isPersistedIdentityStale`. Extract a shared helper like `getCurrentIdentity()` to centralize identity retrieval.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Summary
This PR introduces a feature flag system and uses it to gate the entire Chat feature behind a department-scoped
Chat.Systemflag. When the flag is disabled for a department, all chat functionality is hidden and its background services are skipped.What was added
src/api/feature-flags/feature-flags.ts): API functions (getAllFeatureFlags,getFeatureFlagState) that call the v4FeatureTogglesendpoints.src/stores/feature-flags/store.ts): A persisted Zustand store that fetches, caches, and exposes feature flags. Provides reactive hooks (useFeatureFlag,useIsChatEnabled). Unknown flags default to disabled, and cached flags are retained on fetch failure for offline stability.What was changed
_layout.tsx): Feature flags are now fetched during startup. The SignalR chat hub only connects when theChatSystemflag is enabled.chat.tsx,chatbot.tsx,chat/[channelId].tsx,chat/thread/[messageId].tsx): Each screen checksuseIsChatEnabled()and redirects to the home screen if chat is disabled. Data-fetching effects are also guarded by the flag.sidebar-content.tsx): The Chat and Assistant/Bot menu entries are filtered out when the flag is off.Functional impact
Departments that don't have the
Chat.Systemfeature flag enabled will no longer see chat-related UI, won't connect to the chat SignalR hub, and won't trigger chat data fetches — effectively removing chat from the app experience for those departments.Summary by CodeRabbit