Dalgo AI chat implementation - Frontend #162
Conversation
WalkthroughAdds an organization settings page and OrganizationSettings component, a dashboard AI chat feature (floating trigger, selectable charts, streamed right-side chat panel), API hooks for AI status and dashboard chat, a permissions hook, and permission-gated navigation for Organization Settings. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant D as Dashboard View
participant CTr as Chat Trigger
participant CP as Chat Panel
participant API as Backend API
U->>D: Open dashboard
Note right of D: Optional: select chart for AI context
U->>CTr: Click chat button
CTr->>CTr: Check aiEnabled & isPublicMode
alt AI Disabled or Public Mode
CTr->>U: Show toast / no-op
else AI Enabled
CTr->>CP: Open panel (isOpen=true)
CP->>API: POST /api/ai/dashboard/{id}/context (loadContext)
API-->>CP: Return context (charts, sample_data, summary)
U->>CP: Send message
CP->>API: POST /api/ai/dashboard/{id}/chat (streaming)
loop Streaming chunks
API-->>CP: data: ... (chunk)
CP->>CP: Append streamed assistant content
end
CP->>U: Display final response + metadata
end
sequenceDiagram
participant U as User
participant Chart as Chart Container
participant D as Dashboard
participant CP as Chat Panel
participant API as Backend
U->>Chart: Click chart (select)
Chart->>D: set selectedChartForChat
D->>Chart: Render selected ring + "Selected for AI"
D->>CP: Pass selectedChartId prop
CP->>API: Include selected_chart_id in requests
API-->>CP: Responses may reference selected chart
U->>Chart: Click again (deselect)
Chart->>D: Clear selection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #162 +/- ##
==========================================
+ Coverage 1.72% 13.53% +11.81%
==========================================
Files 131 210 +79
Lines 3767 13960 +10193
Branches 1062 5026 +3964
==========================================
+ Hits 65 1890 +1825
- Misses 3702 12065 +8363
- Partials 0 5 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
components/dashboard/dashboard-native-view.tsx (1)
283-285: Chart selection for AI works but may conflict with chart interactionsWrapping the whole chart tile in a
cursor-pointerdiv and togglingselectedChartForChaton any click is straightforward, but this will also fire when users interact with the chart itself (e.g., click-to-filter, tooltips with clicks). That may cause accidental AI selection/deselection.Consider narrowing the selection click target (e.g., a small overlay button/badge) or checking for modifier keys to avoid interfering with normal chart interactions.
Also applies to: 545-578
hooks/usePermissions.ts (1)
1-45: Centralized permissions hook looks solid
usePermissionsprovides a clear, reusable API (hasPermission,hasAnyPermission,hasAllPermissions,isAccountManager) over the auth store and should simplify permission checks across the app. Memoization oncurrentOrgUser?.permissionsandnew_role_slugis appropriate for typical immutable store updates.If the auth store ever mutates the
permissionsarray in place instead of replacing it, you may want to switch theuseMemodependencies to the wholecurrentOrgUserobject to ensure reactivity.components/settings/organization.tsx (1)
158-169: Align AI logging acknowledgment behavior with UI messagingThe dependency logic in
handleInputChangeensures that turning offai_data_sharing_enabledalso clearsai_logging_acknowledged, and the main “I acknowledge AI logging” switch is disabled when data sharing is off. However, the Info button in the “AI Logging Notice” alert togglesai_logging_acknowledgeddirectly, even when data sharing is disabled.Given the helper text (“Enable data sharing first to acknowledge AI logging”), this can lead to a state where:
ai_data_sharing_enabled === falseandai_logging_acknowledged === trueConsider one of:
- Removing the
onClicktoggle from the Info button so it’s purely informational, or- Respecting the same gating there (no-op when
!settings.ai_data_sharing_enabled), or- Updating the copy to reflect that logging can be acknowledged independently of data sharing if that’s actually intended.
Also applies to: 401-434, 471-492
components/dashboard/dashboard-chat-trigger.tsx (1)
13-151: AI chat trigger gating is correct; consider trimming unused activity stateThe trigger correctly:
- Hides itself for public dashboards, and
- Checks
aiEnabledon click, showing a toast instead of opening chat when AI is disabled.
hasNewActivityis currently never set totrue, so the notification ring / badge effects never appear. You can either wire this up via a callback fromEnhancedDashboardChat(e.g., on new assistant message) or remove the state + related class logic to keep the component lean.components/main-layout.tsx (1)
27-28: Fix transformType parameter type toTransformType | undefinedThe permission-gated "Org Settings" nav item is wired correctly and integrates cleanly with existing navigation rendering. However, verification confirms the first suggestion:
transformTypeis declared asstring | undefined(line 98) but compared against theTransformType.UIenum (line 191), causing a type mismatch. Change the parameter type toTransformType | undefinedto align with actual usage.The permission slug
can_manage_org_settingsappears consistently only once in the codebase at line 224; backend verification of this slug would require checking the backend code separately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
app/settings/organization/page.tsx(1 hunks)components/dashboard/dashboard-chat-trigger.tsx(1 hunks)components/dashboard/dashboard-native-view.tsx(5 hunks)components/dashboard/enhanced-dashboard-chat.tsx(1 hunks)components/main-layout.tsx(5 hunks)components/settings/organization.tsx(1 hunks)hooks/api/useAIStatus.ts(1 hunks)hooks/api/useDashboardChat.ts(1 hunks)hooks/usePermissions.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Define TypeScript interfaces/types for API responses to ensure type safety
Use the @/* path alias for imports instead of deep relative paths
Files:
hooks/usePermissions.tscomponents/settings/organization.tsxcomponents/main-layout.tsxcomponents/dashboard/dashboard-chat-trigger.tsxhooks/api/useAIStatus.tscomponents/dashboard/dashboard-native-view.tsxhooks/api/useDashboardChat.tscomponents/dashboard/enhanced-dashboard-chat.tsxapp/settings/organization/page.tsx
{components,app}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{components,app}/**/*.{ts,tsx}: Use Tailwind utility-first styling, CVA for component variants, and the cn() utility for class merging
Guard browser-only APIs with typeof window !== 'undefined' in client-side code
Files:
components/settings/organization.tsxcomponents/main-layout.tsxcomponents/dashboard/dashboard-chat-trigger.tsxcomponents/dashboard/dashboard-native-view.tsxcomponents/dashboard/enhanced-dashboard-chat.tsxapp/settings/organization/page.tsx
hooks/api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Add new SWR-based API hooks under hooks/api/ and start new features by creating an API hook first
Files:
hooks/api/useAIStatus.tshooks/api/useDashboardChat.ts
app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Create Next.js App Router pages and related route files under the app/ directory following App Router conventions
Files:
app/settings/organization/page.tsx
🧠 Learnings (4)
📚 Learning: 2025-10-13T07:21:21.746Z
Learnt from: CR
Repo: DalgoT4D/webapp_v2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-13T07:21:21.746Z
Learning: Applies to hooks/api/**/*.{ts,tsx} : Add new SWR-based API hooks under hooks/api/ and start new features by creating an API hook first
Applied to files:
hooks/usePermissions.tshooks/api/useAIStatus.tshooks/api/useDashboardChat.ts
📚 Learning: 2025-10-13T07:21:21.746Z
Learnt from: CR
Repo: DalgoT4D/webapp_v2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-13T07:21:21.746Z
Learning: Applies to components/ui/**/*.{ts,tsx} : Build reusable UI on top of Radix UI primitives in components/ui/
Applied to files:
components/main-layout.tsx
📚 Learning: 2025-10-13T07:21:21.746Z
Learnt from: CR
Repo: DalgoT4D/webapp_v2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-13T07:21:21.746Z
Learning: Applies to components/charts/**/*.{ts,tsx} : Implement charts following existing patterns in components/charts/, selecting the appropriate library (ECharts/Nivo/Recharts) and supporting PNG/PDF export
Applied to files:
components/dashboard/dashboard-native-view.tsx
📚 Learning: 2025-10-13T07:21:21.746Z
Learnt from: CR
Repo: DalgoT4D/webapp_v2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-13T07:21:21.746Z
Learning: Applies to app/**/*.{ts,tsx} : Create Next.js App Router pages and related route files under the app/ directory following App Router conventions
Applied to files:
app/settings/organization/page.tsx
🧬 Code graph analysis (8)
components/settings/organization.tsx (3)
lib/api.ts (2)
apiGet(191-193)apiPut(205-211)hooks/use-toast.ts (1)
toast(23-37)test-integration.js (1)
headers(12-16)
components/main-layout.tsx (1)
hooks/usePermissions.ts (1)
usePermissions(7-45)
components/dashboard/dashboard-chat-trigger.tsx (4)
hooks/api/useAIStatus.ts (1)
useAIStatus(11-30)hooks/use-toast.ts (1)
toast(23-37)lib/utils.ts (1)
cn(4-6)components/dashboard/enhanced-dashboard-chat.tsx (1)
EnhancedDashboardChat(92-710)
hooks/api/useAIStatus.ts (1)
lib/api.ts (1)
apiGet(191-193)
components/dashboard/dashboard-native-view.tsx (2)
lib/utils.ts (1)
cn(4-6)components/dashboard/dashboard-chat-trigger.tsx (1)
DashboardChatTrigger(21-153)
hooks/api/useDashboardChat.ts (2)
lib/api.ts (1)
apiPost(196-202)lib/config.ts (1)
API_BASE_URL(9-9)
components/dashboard/enhanced-dashboard-chat.tsx (2)
hooks/api/useAIStatus.ts (1)
useAIStatus(11-30)hooks/api/useDashboardChat.ts (3)
useDashboardChat(78-269)ChatMessage(4-18)DashboardChatSettings(20-25)
app/settings/organization/page.tsx (1)
components/settings/organization.tsx (1)
OrganizationSettings(38-552)
🪛 ast-grep (0.40.0)
components/dashboard/enhanced-dashboard-chat.tsx
[warning] 79-79: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
components/dashboard/enhanced-dashboard-chat.tsx
[error] 80-80: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (6)
app/settings/organization/page.tsx (1)
1-5: Org settings page wiring looks goodSimple App Router page that cleanly delegates to
OrganizationSettings; imports and export shape look correct.components/dashboard/dashboard-native-view.tsx (1)
85-85: AI chat trigger integration looks consistent with view modesImporting and rendering
DashboardChatTriggerwithdashboardId,dashboardTitle,selectedChartId, andisPublicModecleanly wires the dashboard view into the AI chat flow. Relying onisPublicModeinside the trigger to short‑circuit rendering keeps public dashboards free of the chat UI without complicating this component further.Also applies to: 1344-1350
hooks/api/useAIStatus.ts (1)
1-29: SWR AI status hook is clean and type-safe
AIStatusResponseplus the SWR wrapper give a clear, minimal contract (aiEnabled,dataSharingEnabled,loggingAcknowledged,isLoading,error,refetch). Defaulting booleans tofalsewhendatais undefined is a safe fallback.components/settings/organization.tsx (1)
38-156: Org settings + AI consent UI is cohesive and robustThe organization settings page covers name, website, logo upload (with type/size validation and org header propagation), and AI consent (data sharing + logging) with clear messaging and error handling. The load/save flows handle nulls gracefully and synchronize local state from API responses, and the “just saved” visual feedback is a nice UX touch.
No functional issues stand out beyond the AI logging toggle nuance noted above.
Also applies to: 171-261, 263-551
components/dashboard/enhanced-dashboard-chat.tsx (1)
84-709: Overall chat UX and AI gating behavior are well thought outThe chat panel:
- Honors
aiEnabledbefore sending messages,- Seeds settings from org data sharing status,
- Provides context loading, settings, quick actions, and clear feedback on errors,
- Integrates chart focus via
selectedChartIdand annotates metadata (charts analyzed, data included, token usage).Aside from the unsafe HTML injection noted separately, the state management and API usage (
useDashboardChat,useAIStatus) look consistent and aligned with the PR goals.hooks/api/useDashboardChat.ts (1)
195-207: Ensure streaming fetch carries all shared headersBy hand-crafting this
fetch, we only forwardx-dalgo-organd skip the rest of the default headers thatapiFetchinjects (e.g., the user identifier). If the backend relies on those to meter tokens per organization and user—one of this PR’s stated goals—the streaming path will miss the user context. Please confirm the exact header set and either reuseapiFetch(or its header builder) or explicitly include the same headers here so we don’t lose per-user attribution.
| // Component to format AI responses with better styling | ||
| const FormattedMessage = ({ content }: { content: string }) => { | ||
| // Split content by lines and apply formatting | ||
| const formatContent = (text: string) => { | ||
| return ( | ||
| text | ||
| // Convert **bold** to actual bold | ||
| .replace(/\*\*(.*?)\*\*/g, '<strong>$1</strong>') | ||
| // Convert ## headings to styled headings | ||
| .replace( | ||
| /^## (.*$)/gm, | ||
| '<h3 class="font-semibold text-gray-900 mb-2 mt-3 flex items-center gap-2">$1</h3>' | ||
| ) | ||
| // Convert > callouts to styled callouts | ||
| .replace( | ||
| /^> (.*$)/gm, | ||
| '<div class="bg-blue-50 border-l-4 border-blue-500 p-3 my-2 rounded-r"><p class="text-blue-700 font-medium">$1</p></div>' | ||
| ) | ||
| // Convert bullet points with emojis to styled lists | ||
| .replace(/^- (.*$)/gm, '<li class="ml-4 mb-1">$1</li>') | ||
| // Convert numbered lists | ||
| .replace(/^(\d+)\. (.*$)/gm, '<li class="ml-4 mb-2 font-medium">$2</li>') | ||
| // Handle line breaks | ||
| .replace(/\n/g, '<br/>') | ||
| ); | ||
| }; | ||
|
|
||
| const formattedContent = formatContent(content); | ||
|
|
||
| return ( | ||
| <div className="formatted-message" dangerouslySetInnerHTML={{ __html: formattedContent }} /> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Remove raw HTML injection from chat messages to avoid XSS
FormattedMessage constructs HTML strings and injects them with dangerouslySetInnerHTML. Because:
- User messages are rendered through this same component, and
- AI responses are also untrusted text,
this creates a direct XSS vector (users can trivially type <script> or similar and have it executed), matching the static analysis warnings.
A safer immediate fix is to drop the raw HTML injection and let React escape the content while still preserving line breaks:
-// Component to format AI responses with better styling
-const FormattedMessage = ({ content }: { content: string }) => {
- // Split content by lines and apply formatting
- const formatContent = (text: string) => {
- return (
- text
- // Convert **bold** to actual bold
- .replace(/\*\*(.*?)\*\*/g, '<strong>$1</strong>')
- // Convert ## headings to styled headings
- .replace(
- /^## (.*$)/gm,
- '<h3 class="font-semibold text-gray-900 mb-2 mt-3 flex items-center gap-2">$1</h3>'
- )
- // Convert > callouts to styled callouts
- .replace(
- /^> (.*$)/gm,
- '<div class="bg-blue-50 border-l-4 border-blue-500 p-3 my-2 rounded-r"><p class="text-blue-700 font-medium">$1</p></div>'
- )
- // Convert bullet points with emojis to styled lists
- .replace(/^- (.*$)/gm, '<li class="ml-4 mb-1">$1</li>')
- // Convert numbered lists
- .replace(/^(\d+)\. (.*$)/gm, '<li class="ml-4 mb-2 font-medium">$2</li>')
- // Handle line breaks
- .replace(/\n/g, '<br/>')
- );
- };
-
- const formattedContent = formatContent(content);
-
- return (
- <div className="formatted-message" dangerouslySetInnerHTML={{ __html: formattedContent }} />
- );
-};
+// Component to format AI responses safely without raw HTML injection
+const FormattedMessage = ({ content }: { content: string }) => {
+ // Preserve line breaks but let React escape the content
+ return (
+ <div className="formatted-message whitespace-pre-line">
+ {content}
+ </div>
+ );
+};This removes the XSS risk while keeping the door open to later introduce richer formatting via a markdown renderer or a sanitizer (e.g., DOMPurify) if needed.
Also applies to: 490-518
🧰 Tools
🪛 ast-grep (0.40.0)
[warning] 79-79: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
[error] 80-80: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🤖 Prompt for AI Agents
components/dashboard/enhanced-dashboard-chat.tsx lines 50-82 (and similarly
490-518): the component currently builds raw HTML strings and uses
dangerouslySetInnerHTML, which allows XSS; change it to stop producing HTML
strings and instead return safe React elements by escaping content and rendering
line breaks and simple formatting as JSX. Specifically: remove all string
.replace(...) that injects HTML, do not use dangerouslySetInnerHTML, split the
input text on newlines and map each line to a <p> or fragment (or map list-like
lines to plain text list items wrapped in React elements) so React escapes
content automatically; preserve visual breaks by inserting <br/> or wrapping
lines in blocks, and defer any HTML/markdown rendering to a sanitized markdown
renderer (e.g., DOMPurify + markdown-it) if/when added later.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
components/settings/organization.tsx (3)
64-76: Type API responses for org settings and logo upload instead of relying on implicitany
responsefromapiGet/apiPutandresultfromresponse.json()are treated as untyped objects, soresponse.res,response.success,result.success, andresult.filenameare effectivelyany. This makes it easy for backend changes (field renames, optional fields, etc.) to silently break the UI.Per the guideline to type API responses, consider:
Defining response interfaces such as:
interface OrgSettingsResponse { success: boolean; message?: string; res?: OrgSettings | null; } interface UploadLogoResponse { success: boolean; message?: string; filename?: string; }Either:
- Generically typing
apiGet/apiPutso you can writeawait apiGet<OrgSettingsResponse>(...), or- Casting at the call site:
const response = (await apiGet('/api/org-settings/')) as OrgSettingsResponse;and similarly forUploadLogoResponse.This will give you compile‑time coverage on
response.res.organization_logo_filename,ai_settings_accepted_*, andresult.filenameand help catch API drift early. As per coding guidelines, define TypeScript interfaces/types for API responses.Also applies to: 106-121, 213-233
193-201: GuardlocalStorageaccess when buildingx-dalgo-orgheader
handleLogoUploadaccesseslocalStoragedirectly:const selectedOrg = localStorage.getItem('selectedOrg');Even though this runs in an event handler on a client component, the project guideline is to guard browser‑only APIs with
typeof window !== 'undefined'. This also makes the code more robust in tests or any non‑browser execution context.A small adjustment keeps the behavior while following the guideline:
- const selectedOrg = localStorage.getItem('selectedOrg'); + const selectedOrg = + typeof window !== 'undefined' ? window.localStorage.getItem('selectedOrg') : null;As per coding guidelines, this keeps browser-specific APIs explicitly guarded.
371-377: Use acn()utility for conditional classNames instead of string templatesYou’re conditionally appending Tailwind classes via string interpolation:
className={`flex items-center ... rounded-md cursor-pointer ${ isUploadingLogo ? 'opacity-50 cursor-not-allowed' : '' }`}and:
className={`flex items-center justify-between p-4 border rounded-lg ${ !settings.ai_data_sharing_enabled ? 'opacity-50' : '' }`}Per the guideline to use
cn()for class merging, consider switching to something like:className={cn( 'flex items-center gap-2 px-4 py-2 border border-input bg-background hover:bg-accent hover:text-accent-foreground rounded-md cursor-pointer', { 'opacity-50 cursor-not-allowed': isUploadingLogo }, )}and:
className={cn( 'flex items-center justify-between p-4 border rounded-lg', { 'opacity-50': !settings.ai_data_sharing_enabled }, )}This keeps the classes readable and scales better as conditions grow.
Also applies to: 475-477
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
components/settings/organization.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{components,app}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{components,app}/**/*.{ts,tsx}: Use Tailwind utility-first styling, CVA for component variants, and the cn() utility for class merging
Guard browser-only APIs with typeof window !== 'undefined' in client-side code
Files:
components/settings/organization.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Define TypeScript interfaces/types for API responses to ensure type safety
Use the @/* path alias for imports instead of deep relative paths
Files:
components/settings/organization.tsx
🧬 Code graph analysis (1)
components/settings/organization.tsx (2)
lib/api.ts (2)
apiGet(191-193)apiPut(205-211)hooks/use-toast.ts (1)
toast(23-37)
| const handleInputChange = (field: keyof OrgSettings, value: string | boolean) => { | ||
| setSettings((prev) => { | ||
| const newSettings = { ...prev, [field]: value }; | ||
|
|
||
| // Dependency: If data sharing is disabled, also disable logging acknowledgment | ||
| if (field === 'ai_data_sharing_enabled' && !value) { | ||
| newSettings.ai_logging_acknowledged = false; | ||
| } | ||
|
|
||
| return newSettings; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Enforce invariant: logging cannot be acknowledged when data sharing is disabled
The Info button in the AI Logging notice toggles ai_logging_acknowledged regardless of ai_data_sharing_enabled, while the dedicated switch is disabled and the copy says “Enable data sharing first…”. This lets you end up with ai_logging_acknowledged === true and ai_data_sharing_enabled === false, and that state can be persisted via saveSettings, which is at odds with the intended consent flow and UI messaging.
Also, handleInputChange is typed as (field: keyof OrgSettings, value: string | boolean), but the function is currently only used for the two boolean flags. As the interface grows, it becomes easy to accidentally pass incompatible values to other fields.
Consider:
- Enforcing the invariant at the UI level by disabling the Info button when data sharing is off and/or short-circuiting its
onClickwhen!settings.ai_data_sharing_enabled. - Optionally reinforcing the invariant inside
handleInputChangesoai_logging_acknowledgedcan never be set totruewhileai_data_sharing_enabledisfalse. - Narrowing
handleInputChangeto only handle the boolean AI flags (or using field-specific helpers) to avoid accidental misuse later.
Example minimal UI change:
- <Button
- variant="ghost"
- size="sm"
- onClick={() =>
- handleInputChange(
- 'ai_logging_acknowledged',
- !settings.ai_logging_acknowledged
- )
- }
- >
+ <Button
+ variant="ghost"
+ size="sm"
+ disabled={!settings.ai_data_sharing_enabled}
+ onClick={() => {
+ if (!settings.ai_data_sharing_enabled) return;
+ handleInputChange(
+ 'ai_logging_acknowledged',
+ !settings.ai_logging_acknowledged
+ );
+ }}
+ >Also applies to: 405-438, 440-496
🤖 Prompt for AI Agents
In components/settings/organization.tsx around lines 150-161 (also review blocks
at 405-438 and 440-496): the UI currently allows toggling
ai_logging_acknowledged even when ai_data_sharing_enabled is false, and the
generic handleInputChange typed as (field: keyof OrgSettings, value: string |
boolean) can accept inappropriate fields/values; update the UI to disable the
Info button (and short-circuit its onClick) when
settings.ai_data_sharing_enabled is false so users cannot acknowledge logging
without enabling data sharing, and inside handleInputChange enforce the
invariant by preventing ai_logging_acknowledged from being set true if
ai_data_sharing_enabled is false (e.g., if field === 'ai_logging_acknowledged'
&& value === true && !prev.ai_data_sharing_enabled then return prev), and
tighten the handler signature to only accept the two boolean AI flags (or add
separate boolean-specific setters) to avoid accidental misuse as the settings
interface grows.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.