refactor(opsassistant): reuse shared toDiagnosePrompt utility - #73
refactor(opsassistant): reuse shared toDiagnosePrompt utility#73AnouarMohamed wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe hook was refactored to use a centralized Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/views/opsassistant/hooks/useAssistantChat.ts`:
- Line 3: The file is missing type imports for DiagnosticsResult and
DiagnosticIssue used in useAssistantChat; update the existing import statement
that brings in AssistantResponse to also import DiagnosticsResult and
DiagnosticIssue (e.g., add them to the import from "../../../types") so the
symbols resolve and TypeScript compiles.
🪄 Autofix (Beta)
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: 1c247667-a37e-4b99-98ab-cfd2f23ce5a0
📒 Files selected for processing (1)
src/views/opsassistant/hooks/useAssistantChat.ts
| import { useEffect, useMemo, useRef, useState } from "react"; | ||
| import { api } from "../../../lib/api"; | ||
| import type { AssistantResponse, DiagnosticsResult, DiagnosticIssue } from "../../../types"; | ||
| import type { AssistantResponse } from "../../../types"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether DiagnosticsResult/DiagnosticIssue are locally imported or ambiently declared.
rg -nP 'import type\s+\{[^}]*\}\s+from\s+"../../../types"' src/views/opsassistant/hooks/useAssistantChat.ts
rg -nP '\b(DiagnosticsResult|DiagnosticIssue)\b' src/views/opsassistant/hooks/useAssistantChat.ts -C2
rg -nP '(export|declare)\s+(type|interface)\s+(DiagnosticsResult|DiagnosticIssue)\b|declare\s+global' --iglob '*.ts' --iglob '*.d.ts'Repository: AnouarMohamed/KubLens-AI
Length of output: 894
Re-add diagnostics type imports to prevent unresolved symbols.
DiagnosticsResult (Line 177, 221) and DiagnosticIssue (Line 215) are used but no longer imported, which causes TypeScript compilation errors.
Proposed fix
-import type { AssistantResponse } from "../../../types";
+import type { AssistantResponse, DiagnosticsResult, DiagnosticIssue } from "../../../types";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/views/opsassistant/hooks/useAssistantChat.ts` at line 3, The file is
missing type imports for DiagnosticsResult and DiagnosticIssue used in
useAssistantChat; update the existing import statement that brings in
AssistantResponse to also import DiagnosticsResult and DiagnosticIssue (e.g.,
add them to the import from "../../../types") so the symbols resolve and
TypeScript compiles.
There was a problem hiding this comment.
what would even happen, i want it to be that way
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if DiagnosticsResult/DiagnosticIssue are declared globally or ambient
rg -nP '(DiagnosticsResult|DiagnosticIssue)' --iglob '*.ts' --iglob '*.d.ts' -C2
# Also check tsconfig for global type definitions
fd 'tsconfig' --extension json | xargs cat 2>/dev/null
Length of output: 8545
@AnouarMohamed Here's exactly what happens with the imports removed:
DiagnosticsResult and DiagnosticIssue are named exports from src/types/diagnostics.ts — they are not globally available. With "strict": true in your tsconfig.json, TypeScript will throw hard compilation errors on these three lines:
| Line | Error |
|---|---|
| 177 | Cannot find name 'DiagnosticsResult' |
| 215 | Cannot find name 'DiagnosticIssue' |
| 221 | Cannot find name 'DiagnosticsResult' |
The build will fail. There's no implicit fallback to any here because strict mode includes noImplicitAny.
If you genuinely want to drop those imports, the only way to do it without breaking the build is to also remove the explicit type annotations from the three helper functions:
-function buildDiagnosticsIntroMessage(diagnostics: DiagnosticsResult): AssistantMessage | null {
+function buildDiagnosticsIntroMessage(diagnostics: Parameters<typeof api.getDiagnostics extends (...args: any) => Promise<infer R> ? (r: R) => void : never>[0]): AssistantMessage | null {…which would be far messier. The cleanest approach if you want no direct type reference is to just let TypeScript infer:
-function buildDiagnosticsIntroMessage(diagnostics: DiagnosticsResult): AssistantMessage | null {
+function buildDiagnosticsIntroMessage(diagnostics: Awaited<ReturnType<typeof api.getDiagnostics>>): AssistantMessage | null {and similarly for DiagnosticIssue. But honestly, just keeping the two type imports is the simplest and most readable option here — they're cheap import type statements with zero runtime cost.
There was a problem hiding this comment.
@AnouarMohamed, here's exactly what happens:
Since DiagnosticsResult and DiagnosticIssue are used as type annotations in three function signatures that still exist in the file:
- Line 177:
function buildDiagnosticsIntroMessage(diagnostics: DiagnosticsResult) - Line 215:
function formatIssueLine(issue: DiagnosticIssue) - Line 221:
function buildDiagnosticPrompts(diagnostics: DiagnosticsResult)
…and our earlier scan confirmed they are not declared globally or ambiently anywhere in the codebase, removing the imports means TypeScript cannot resolve those names. You'll get hard compilation errors:
error TS2304: Cannot find name 'DiagnosticsResult'.
error TS2304: Cannot find name 'DiagnosticIssue'.
This will break the build. There's no runtime fallback — TypeScript treats unresolved type names as errors, not warnings.
If you want to intentionally remove those imports, you'd also need to remove the type annotations from those three function signatures (e.g., replace them with any or infer from usage), or remove the functions entirely if they're no longer needed. Would you like help with that approach instead?
There was a problem hiding this comment.
@AnouarMohamed, here are concrete sketches for both.
1. Types Restructuring
Goal: kill the dual-system ambiguity. Make src/types/index.ts the one true barrel and delete (or replace) src/types.ts.
src/
types/
index.ts ← re-exports everything (replaces src/types.ts)
assistant.ts ← AssistantResponse, AssistantMessage, etc.
diagnostics.ts ← DiagnosticsResult, DiagnosticIssue, etc.
...
src/types/index.ts becomes:
export * from "./assistant";
export * from "./diagnostics";
// ... other domain filesThen any import that previously pointed at "../../../types" now points at "../../../types/index" (or just "../../../types" if you configure path aliases), but you can also import domain-specifically:
// Before (ambiguous barrel)
import type { AssistantResponse, DiagnosticsResult } from "../../../types";
// After (explicit — you always know where the type lives)
import type { AssistantResponse } from "../../../types/assistant";
import type { DiagnosticsResult, DiagnosticIssue } from "../../../types/diagnostics";This makes the "import drift" issue in this PR impossible — if you remove DiagnosticsResult from the hook import, the compiler immediately points you at the exact file to fix, not a generic barrel.
2. Hook Split for useAssistantChat.ts
Split along the four natural seams already in the file:
useDiagnosticsPreload.ts
Owns the one-time API call and injects the intro message.
import { useEffect, useRef } from "react";
import { api } from "../../../lib/api";
import type { AssistantMessage } from "../types";
import type { DiagnosticsResult } from "../../../types/diagnostics";
export function useDiagnosticsPreload(
enabled: boolean,
onLoad: (intro: AssistantMessage, prompts: string[]) => void,
) {
const loaded = useRef(false);
useEffect(() => {
if (!enabled || loaded.current) return;
loaded.current = true;
let cancelled = false;
const run = async () => {
try {
const diagnostics = await api.getDiagnostics();
if (cancelled) return;
const intro = buildDiagnosticsIntroMessage(diagnostics);
if (!intro) return;
onLoad(intro, buildDiagnosticPrompts(diagnostics));
} catch { /* ignore preload failures */ }
};
void run();
return () => { cancelled = true; };
}, [enabled, onLoad]);
}useSuggestionPool.ts
Owns deduplication + capping of hints/resources.
import { useMemo } from "react";
import type { AssistantMessage } from "../types";
import { toDiagnosePrompt } from "../utils";
import { basePrompts } from "./useAssistantChat";
export function useSuggestionPool(lastAssistant: AssistantMessage | undefined) {
return useMemo(() => {
const fromHints = lastAssistant?.hints ?? [];
const fromResources = (lastAssistant?.resources ?? []).map(toDiagnosePrompt);
return dedupeStrings([...basePrompts, ...fromHints, ...fromResources]).slice(0, 10);
}, [lastAssistant?.hints, lastAssistant?.resources]);
}useAssistantMessages.ts
Owns message state and session synchronization.
import { useEffect, useState } from "react";
import type { AssistantMessage } from "../types";
export function useAssistantMessages(
activeSession: { messages: AssistantMessage[] } | null,
activeSessionId: string | null,
sessionCount: number,
createIntroMessage: () => AssistantMessage,
) {
const [messages, setMessages] = useState<AssistantMessage[]>(() =>
sessionCount > 0 ? [] : [createIntroMessage()],
);
useEffect(() => {
if (activeSession) {
setMessages((s) =>
areMessagesEqual(s, activeSession.messages) ? s : cloneMessages(activeSession.messages),
);
return;
}
if (activeSessionId === null && sessionCount === 0) {
const intro = [createIntroMessage()];
setMessages((s) => (areMessagesEqual(s, intro) ? s : intro));
}
}, [activeSession, activeSessionId, sessionCount, createIntroMessage]);
return { messages, setMessages };
}useAssistantChat.ts (thin orchestrator)
Shrinks from 250 lines to ~80:
import { useCallback, useMemo, useRef, useState } from "react";
import { api } from "../../../lib/api";
import type { AssistantResponse } from "../../../types/assistant";
import type { AssistantMessage } from "../types";
import { useChatSessions } from "./useChatSessions";
import { useAssistantMessages } from "./useAssistantMessages";
import { useDiagnosticsPreload } from "./useDiagnosticsPreload";
import { useSuggestionPool } from "./useSuggestionPool";
export function useAssistantChat() {
const { sessions, activeSessionId, startNewSession, selectSession, saveSession, deleteSession } = useChatSessions();
const activeSession = useMemo(
() => sessions.find((s) => s.id === activeSessionId) ?? null,
[activeSessionId, sessions],
);
const { messages, setMessages } = useAssistantMessages(
activeSession, activeSessionId, sessions.length, createAssistantIntroMessage,
);
const [isLoading, setIsLoading] = useState(false);
const [diagnosticPrompts, setDiagnosticPrompts] = useState<string[]>([]);
const onDiagnosticsLoad = useCallback(
(intro: AssistantMessage, prompts: string[]) => {
setMessages((s) => [...s, intro]);
setDiagnosticPrompts(prompts);
},
[setMessages],
);
useDiagnosticsPreload(
activeSessionId === null && sessions.length === 0,
onDiagnosticsLoad,
);
const lastAssistant = useMemo(
() => [...messages].reverse().find((m) => m.role === "assistant" && !m.isError),
[messages],
);
const suggestionPool = useSuggestionPool(lastAssistant);
const send = async (content: string, namespace?: string) => { /* unchanged */ };
const clear = () => { startNewSession(); setMessages([]); };
return { messages, isLoading, lastAssistant, suggestionPool, diagnosticPrompts,
sessions, activeSessionId, selectSession, deleteSession, startNewSession, send, clear };
}Migration order I'd recommend:
- Fix
src/types/barrel first (zero behaviour change, unblocks clean imports everywhere). - Extract
useDiagnosticsPreload(isolated, easy to test in isolation). - Extract
useSuggestionPool(pureuseMemo, trivially safe). - Extract
useAssistantMessages(session sync logic). - Slim down the orchestrator last.
Each step is independently mergeable. Want me to generate a full diff for any of these?
There was a problem hiding this comment.
no, ill make a pr adressing all of this this later
There was a problem hiding this comment.
@AnouarMohamed, sounds good — looking forward to that PR! Just a reminder that this current PR still has a blocking build issue: DiagnosticsResult and DiagnosticIssue are referenced in the helper functions but no longer imported. You'll need to either re-add the two type imports or remove the explicit type annotations before merging:
-import type { AssistantResponse } from "../../../types";
+import type { AssistantResponse, DiagnosticsResult, DiagnosticIssue } from "../../../types";There was a problem hiding this comment.
Is this safe to merge
There was a problem hiding this comment.
@AnouarMohamed No, it is not safe to merge in its current state.
The PR removes the DiagnosticsResult and DiagnosticIssue type imports, but they are still actively used as explicit type annotations on three helper functions that remain in the file:
| Line | Usage |
|---|---|
| 177 | function buildDiagnosticsIntroMessage(diagnostics: DiagnosticsResult) |
| 215 | function formatIssueLine(issue: DiagnosticIssue) |
| 221 | function buildDiagnosticPrompts(diagnostics: DiagnosticsResult) |
With "strict": true in your tsconfig.json, this will produce hard build errors (TS2304: Cannot find name) and the app won't compile. The fix is a one-liner before merging:
-import type { AssistantResponse } from "../../../types";
+import type { AssistantResponse, DiagnosticsResult, DiagnosticIssue } from "../../../types";Once that's in, it'll be safe to merge. The broader restructuring you planned for the follow-up PR can then clean it all up properly.
Summary\n- Removes duplicate prompt formatting in chat hook.\n- Reuses shared utils helper for diagnose prompts.\n\n## Testing\n- Not run (refactor-only).
Summary by CodeRabbit