Feat allow chat during inference#245
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new INFERRING parsing status and propagates it through enums, polling, service logic, and UI: banners, progress components, step advancement, and parse API payloads (filters). Also adds a duplicate FileTree effect that may emit file counts twice. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChatPage as Chat Page
participant Poller as Poller (5s)
participant Service as BranchAndRepositoryService
participant API as Parse API
User->>ChatPage: Open chat / start parse (with filters)
ChatPage->>Service: parseRepo(repo, branch, filters)
Service->>API: POST /api/v1/parse {..., filters}
API-->>Service: { status: INFERRING | READY | ERROR }
alt status == INFERRING
Service-->>ChatPage: INFERRING
ChatPage->>ChatPage: Show INFERRING banner, allow writing
ChatPage->>Poller: Start 5s polling
loop every 5s
Poller->>Service: pollParsingStatus()
Service-->>Poller: { status: INFERRING }
Poller->>ChatPage: Update UI (still INFERRING)
end
else status == READY
Service-->>ChatPage: READY
ChatPage->>ChatPage: Close banner, advance to step 2 (once)
else status == ERROR
Service-->>ChatPage: ERROR
ChatPage->>ChatPage: Show error state
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/agent-creation/ParsingProgress.tsx (1)
6-14: Remove duplicateParsingStatusEnumdefinition.This enum is already defined in
lib/Constants.tsxand should be imported from there instead of being redefined. Having duplicate definitions can lead to inconsistencies if one is updated without the other.Apply this diff:
import React from "react"; import { Button } from "@/components/ui/button"; import { Loader, CheckCircle, AlertCircle, RefreshCw } from "lucide-react"; +import { ParsingStatusEnum } from "@/lib/Constants"; -// Enum for parsing status -export enum ParsingStatusEnum { - SUBMITTED = "submitted", - PROCESSING = "processing", - CLONED = "cloned", - PARSED = "parsed", - INFERRING = "inferring", - READY = "ready", - ERROR = "error" -}
🧹 Nitpick comments (17)
components/agent-creation/ParsingProgress.tsx (1)
97-106: Extract nested ternary into a helper function for readability.The deeply nested ternary for progress width is hard to read and maintain. As flagged by SonarCloud, extracting this into a helper improves clarity.
+const getProgressWidth = (status: ParsingStatusEnum | null): string => { + switch (status) { + case ParsingStatusEnum.SUBMITTED: return '10%'; + case ParsingStatusEnum.PROCESSING: return '30%'; + case ParsingStatusEnum.CLONED: return '60%'; + case ParsingStatusEnum.PARSED: return '80%'; + case ParsingStatusEnum.INFERRING: return '95%'; + default: return '0%'; + } +}; // Then in the JSX: - <div - className="bg-primary h-1.5 rounded-full transition-all duration-500" - style={{ - width: status === ParsingStatusEnum.SUBMITTED ? '10%' : - status === ParsingStatusEnum.PROCESSING ? '30%' : - status === ParsingStatusEnum.CLONED ? '60%' : - status === ParsingStatusEnum.PARSED ? '80%' : - status === ParsingStatusEnum.INFERRING ? '95%' : '0%' - }} - ></div> + <div + className="bg-primary h-1.5 rounded-full transition-all duration-500" + style={{ width: getProgressWidth(status) }} + ></div>services/BranchAndRepositoryService.ts (2)
13-13: Avoid usinganytype forfiltersparameter.Using
anybypasses TypeScript's type checking. Consider defining a proper interface for the filters structure.+interface ParseFilters { + exclude_dirs?: string[]; + exclude_files?: string[]; + // Add other filter properties as needed +} -static async parseRepo(repo_name: string, branch_name: string, filters?: any) { +static async parseRepo(repo_name: string, branch_name: string, filters?: ParseFilters) {
200-215: Remove unusedgetStatusMessagefunction.The
getStatusMessagefunction is defined insidepollParsingStatusbut its return value is never used. Either remove it or use it for logging/status updates.- const getStatusMessage = (status: string) => { - switch (status) { - case ParsingStatusEnum.SUBMITTED: - return "Cloning your repository"; - case ParsingStatusEnum.CLONED: - return "Parsing your code"; - case ParsingStatusEnum.PARSED: - return "Understanding your codebase"; - case ParsingStatusEnum.INFERRING: - return "Enriching with AI insights"; - case ParsingStatusEnum.ERROR: - return "Error"; - default: - return status; - } - };app/(main)/newchat/components/ParsingProgress.tsx (1)
6-10: Consider usingParsingStatusEnumtype forstatusprop.The
statusprop is typed asstring, but since you're comparing againstParsingStatusEnumvalues, using the enum type (or a union with"loading") would provide better type safety.interface ParsingProgressProps { - status: string; + status: ParsingStatusEnum | "loading" | ""; projectId?: string; onRetry?: () => void; }app/(main)/chat/[chatId]/page.tsx (1)
230-230: Consider extracting the magic number for banner height.The
40pxoffset is a magic number that couples this calculation to the banner's actual height. If the banner height changes, this will break silently.+const INFERRING_BANNER_HEIGHT = 40; // px - matches the INFERRING status banner height -<div className={parsingStatus === ParsingStatusEnum.INFERRING ? "h-[calc(90vh-40px)]" : "h-[calc(90vh)]"}> +<div className={parsingStatus === ParsingStatusEnum.INFERRING ? `h-[calc(90vh-${INFERRING_BANNER_HEIGHT}px)]` : "h-[calc(90vh)]"}>Alternatively, consider using
flex-1withoverflow-hiddento let the content area fill remaining space naturally.app/(main)/newchat/components/FileSelector.tsx (4)
1-21: Remove unused import and consolidate lucide-react imports.The
BranchAndRepositoryServiceimport is unused andlucide-reactis imported twice on separate lines.Apply this diff:
-import React, { useState, useEffect } from "react"; -import { X, Plus, Info, Folder, FileText, Hash, Regex, Trash2 } from "lucide-react"; +import React, { useState, useEffect } from "react"; +import { + X, Plus, Info, Folder, FileText, Hash, Regex, Trash2, + ChevronDown, ChevronRight, FolderTree, Settings2 +} from "lucide-react"; import { Input } from "@/components/ui/input"; import { Badge } from "@/components/ui/badge"; import { Button } from "@/components/ui/button"; @@ -15,9 +17,7 @@ TooltipProvider, TooltipTrigger, } from "@/components/ui/tooltip"; -import { ChevronDown, ChevronRight, FolderTree, Settings2 } from "lucide-react"; import { cn } from "@/lib/utils"; import FileTree, { calculateFileCounts } from "./FileTree"; -import BranchAndRepositoryService from "@/services/BranchAndRepositoryService";
196-211: Use extension value as key instead of array index.Using array index as React key can cause rendering issues when items are removed or reordered. Since extensions are unique strings, use them directly as keys.
- {filters.excluded_extensions.map((ext, index) => ( + {filters.excluded_extensions.map((ext) => ( <Badge - key={index} + key={ext} variant="secondary" className="gap-1 pr-1 text-xs bg-purple-50 text-purple-700 hover:bg-purple-100" > {ext} <X className="h-3 w-3 cursor-pointer hover:text-red-500" - onClick={() => onRemoveExtension(index)} + onClick={() => onRemoveExtension(filters.excluded_extensions.indexOf(ext))} /> </Badge> ))}
269-294: Use directory path as key instead of array index.Directory paths are unique identifiers and should be used as keys for stable rendering.
- {filters.excluded_directories.map((dir, index) => ( - <TooltipProvider key={index} delayDuration={300}> + {filters.excluded_directories.map((dir) => ( + <TooltipProvider key={dir} delayDuration={300}> <Tooltip> <TooltipTrigger asChild> <Badge variant="secondary" className="gap-1 pr-1 text-xs bg-blue-50 text-blue-700 hover:bg-blue-100 cursor-default" > {dir.length > 25 ? `...${dir.slice(-22)}` : dir} <X className="h-3 w-3 cursor-pointer hover:text-red-500" - onClick={() => onRemoveDirectory(index)} + onClick={() => onRemoveDirectory(filters.excluded_directories.indexOf(dir))} /> </Badge> </TooltipTrigger>
355-357: Consider using proper type forvalueparameter.Using
anyreduces type safety. Consider using a union type for better type checking.- const updateFilter = (key: keyof ParseFilters, value: any) => { + const updateFilter = (key: keyof ParseFilters, value: string[]) => { setFilters({ ...filters, [key]: value }); };app/(main)/newchat/components/step1.tsx (1)
342-371: Consider adding cleanup for the async effect.If
repoNameorbranchNamechanges rapidly or the component unmounts during the fetch, this could attempt to update state on a stale/unmounted component.useEffect(() => { + let isCancelled = false; + const fetchExistingFilters = async () => { // Reset filters first when repo/branch changes const defaultFilters = { excluded_directories: [], excluded_files: [], excluded_extensions: [], }; setFilters(defaultFilters); if (repoName && branchName) { try { const statusResponse = await BranchAndRepositoryService.checkParsingStatus(repoName, branchName); - if (statusResponse?.current_filters) { + if (!isCancelled && statusResponse?.current_filters) { const existingFilters = statusResponse.current_filters; setFilters({ excluded_directories: existingFilters.excluded_directories || [], excluded_files: existingFilters.excluded_files || [], excluded_extensions: existingFilters.excluded_extensions || [], }); } } catch (error) { console.error("Error fetching existing filters:", error); } } }; fetchExistingFilters(); + + return () => { + isCancelled = true; + }; }, [repoName, branchName]);app/(main)/newchat/components/FileTree.tsx (7)
6-6: Remove unusedcnimport.The
cnutility is imported but not used in this file.-import { cn } from "@/lib/utils";
15-20: Consider extractingParseFiltersto a shared types file.Duplicating the interface risks type drift between files. Extract to a shared location like
@/types/parsing.tsthat both components can import.Create a new file
types/parsing.ts:export interface ParseFilters { excluded_directories: string[]; excluded_files: string[]; excluded_extensions: string[]; }Then update imports in both
FileSelector.tsxandFileTree.tsx:import { ParseFilters } from "@/types/parsing";
370-393: Consider usingreplaceAllfor cleaner code.Modern JavaScript
replaceAllis more readable thanreplacewith regex for global substitutions. Also consider usingString.rawfor the regex pattern.// Convert glob pattern to regex // Escape special regex characters except * and ** - let regexPattern = pattern - .replace(/[.+^${}()|[\]\\]/g, '\\$&') // Escape special chars - .replace(/\*\*/g, '{{GLOBSTAR}}') // Temp placeholder for ** - .replace(/\*/g, '[^/]*') // * matches anything except / - .replace(/{{GLOBSTAR}}/g, '.*'); // ** matches anything including / + let regexPattern = pattern + .replaceAll(/[.+^${}()|[\]\\]/g, String.raw`\$&`) // Escape special chars + .replaceAll('**', '{{GLOBSTAR}}') // Temp placeholder for ** + .replaceAll('*', '[^/]*') // * matches anything except / + .replaceAll('{{GLOBSTAR}}', '.*'); // ** matches anything including /
549-597: The toggle logic is clear but consider extracting helpers for maintainability.While the complexity is only slightly over the threshold, the distinct add/remove operations for directories vs files could be extracted into helper functions for better readability.
764-835: Consider extracting render branches into separate functions.The nested ternaries for pattern mode, search mode, and tree mode are difficult to follow. Extract into named render functions for clarity.
// Add these helper functions inside the component: const renderPatternPreview = () => { if (debouncedPatternQuery !== patternQuery) { return ( <div className="flex justify-center p-4"> <Loader2 className="h-4 w-4 animate-spin text-gray-400" /> </div> ); } if (patternMatches.length === 0) { return ( <div className="text-gray-500 p-4 text-sm text-center"> No files matching pattern "{patternQuery}" </div> ); } return ( <div className="space-y-0.5"> {/* ... pattern matches content ... */} </div> ); }; const renderSearchResults = () => { /* ... */ }; const renderTreeView = () => { /* ... */ }; // Then in the render: <div className="flex-1 overflow-y-auto p-1"> {showPatternMode && patternQuery ? renderPatternPreview() : searchQuery ? renderSearchResults() : renderTreeView() } </div>
278-284: Consider splitting the component to reduce complexity.The
FileTreecomponent has high cognitive complexity (41 vs 15 allowed) due to multiple concerns: data fetching, search debouncing, pattern matching, and multiple render modes. Consider extracting custom hooks likeuseFileTreeData,useDebouncedSearch, andusePatternMatching.
572-577: Useelse ifinstead of nestedifinelseblock.} else { - // Add file if not already present - if (node.path && !newFilters.excluded_files.includes(node.path)) { - newFilters.excluded_files = [...newFilters.excluded_files, node.path]; - } + } else if (node.path && !newFilters.excluded_files.includes(node.path)) { + // Add file if not already present + newFilters.excluded_files = [...newFilters.excluded_files, node.path]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/(main)/chat/[chatId]/page.tsx(3 hunks)app/(main)/newchat/components/FileSelector.tsx(1 hunks)app/(main)/newchat/components/FileTree.tsx(1 hunks)app/(main)/newchat/components/ParsingProgress.tsx(3 hunks)app/(main)/newchat/components/step1.tsx(8 hunks)components/agent-creation/ParsingProgress.tsx(4 hunks)lib/Constants.tsx(1 hunks)services/BranchAndRepositoryService.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use PascalCase for component names and camelCase for utility names
Order imports: React/Next imports first, then components, then utilities
Use Next.js error boundaries, optional chaining, and nullish coalescing for error handling
Files:
lib/Constants.tsxapp/(main)/newchat/components/ParsingProgress.tsxcomponents/agent-creation/ParsingProgress.tsxapp/(main)/chat/[chatId]/page.tsxapp/(main)/newchat/components/FileTree.tsxapp/(main)/newchat/components/FileSelector.tsxservices/BranchAndRepositoryService.tsapp/(main)/newchat/components/step1.tsx
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.tsx: Use Tailwind CSS for styling and thecnutility for conditional classes
Use functional components with TypeScript FC types
Prefer destructuring props at the component level
Use the "use client" directive for client components
Files:
lib/Constants.tsxapp/(main)/newchat/components/ParsingProgress.tsxcomponents/agent-creation/ParsingProgress.tsxapp/(main)/chat/[chatId]/page.tsxapp/(main)/newchat/components/FileTree.tsxapp/(main)/newchat/components/FileSelector.tsxapp/(main)/newchat/components/step1.tsx
🧠 Learnings (2)
📚 Learning: 2025-07-22T09:20:21.427Z
Learnt from: CR
Repo: potpie-ai/potpie-ui PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-22T09:20:21.427Z
Learning: Organize files using feature-based structure compatible with Next.js App Router
Applied to files:
app/(main)/newchat/components/FileTree.tsx
📚 Learning: 2025-07-22T09:20:21.427Z
Learnt from: CR
Repo: potpie-ai/potpie-ui PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-22T09:20:21.427Z
Learning: Applies to **/*.tsx : Use functional components with TypeScript FC types
Applied to files:
app/(main)/newchat/components/FileSelector.tsx
🧬 Code graph analysis (3)
app/(main)/chat/[chatId]/page.tsx (1)
services/BranchAndRepositoryService.ts (1)
BranchAndRepositoryService(11-249)
app/(main)/newchat/components/FileTree.tsx (2)
app/(main)/newchat/components/FileSelector.tsx (1)
ParseFilters(23-27)services/BranchAndRepositoryService.ts (1)
BranchAndRepositoryService(11-249)
app/(main)/newchat/components/step1.tsx (3)
app/(main)/newchat/components/FileSelector.tsx (1)
ParseFilters(23-27)app/(main)/newchat/components/FileTree.tsx (1)
ParseFilters(16-20)services/BranchAndRepositoryService.ts (1)
BranchAndRepositoryService(11-249)
🪛 GitHub Check: SonarCloud Code Analysis
components/agent-creation/ParsingProgress.tsx
[warning] 103-104: Extract this nested ternary operation into an independent statement.
app/(main)/newchat/components/FileTree.tsx
[warning] 807-807: Unexpected negated condition.
[warning] 781-781: Extract this nested ternary operation into an independent statement.
[failure] 284-284: Refactor this function to reduce its Cognitive Complexity from 41 to the 15 allowed.
[warning] 794-820: Extract this nested ternary operation into an independent statement.
[warning] 807-807: Extract this nested ternary operation into an independent statement.
[warning] 768-768: Unexpected negated condition.
[warning] 376-376: Prefer String#replaceAll() over String#replace().
[warning] 798-820: Extract this nested ternary operation into an independent statement.
[warning] 781-781: Unexpected negated condition.
[warning] 6-6: Remove this unused import of 'cn'.
[warning] 574-574: 'If' statement should not be the only statement in 'else' block
[warning] 391-391: Prefer String#replaceAll() over String#replace().
[warning] 768-791: Extract this nested ternary operation into an independent statement.
[warning] 222-222: Avoid non-native interactive elements. If using native HTML is not possible, add an appropriate role and support for tabbing, mouse, keyboard, and touch inputs to an interactive content element.
[failure] 600-600: Refactor this function to reduce its Cognitive Complexity from 18 to the 15 allowed.
[warning] 222-222: Visible, non-interactive elements with click handlers must have at least one keyboard listener.
[warning] 374-374: Prefer String#replaceAll() over String#replace().
[warning] 373-373: String.raw should be used to avoid escaping \.
[failure] 549-549: Refactor this function to reduce its Cognitive Complexity from 16 to the 15 allowed.
[warning] 373-373: Prefer String#replaceAll() over String#replace().
app/(main)/newchat/components/FileSelector.tsx
[warning] 388-388: Avoid non-native interactive elements. If using native HTML is not possible, add an appropriate role and support for tabbing, mouse, keyboard, and touch inputs to an interactive content element.
[warning] 21-21: Remove this unused import of 'BranchAndRepositoryService'.
[warning] 388-388: Visible, non-interactive elements with click handlers must have at least one keyboard listener.
[warning] 199-199: Do not use Array index in keys
[warning] 18-18: 'lucide-react' imported multiple times.
[warning] 271-271: Do not use Array index in keys
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (12)
services/BranchAndRepositoryService.ts (1)
217-229: LGTM on the step advancement logic.The
hasAdvancedToStep2flag correctly ensures the step is only advanced once when transitioning to INFERRING or READY. This prevents multiple unnecessary calls tosetChatStep(2).lib/Constants.tsx (1)
30-38: LGTM on the newINFERRINGenum value.The addition of
INFERRINGtoParsingStatusEnumcorrectly extends the parsing state machine to support the new AI enrichment phase.app/(main)/newchat/components/ParsingProgress.tsx (1)
72-80: LGTM on the INFERRING state UI.The visual treatment with the animated
Sparklesicon and the helpful "(You can start chatting now!)" message clearly communicates that users can proceed while AI enrichment continues.app/(main)/chat/[chatId]/page.tsx (2)
173-189: LGTM on the INFERRING polling implementation.The polling logic correctly:
- Only runs when status is
INFERRINGandprojectIdexists- Cleans up the interval on unmount or dependency change
- Handles errors gracefully without breaking the UI
219-229: LGTM on the INFERRING status banner.The banner provides clear visual feedback with appropriate styling for both light and dark modes, and the messaging effectively communicates that users can continue working.
app/(main)/newchat/components/FileSelector.tsx (1)
37-119: LGTM!The
TagInputcomponent is well-structured with proper keyboard handling for Enter key submission and click-based addition. The suggestions feature with visual feedback for already-selected items is a nice UX touch.app/(main)/newchat/components/step1.tsx (5)
60-60: LGTM!The import correctly brings in both the
FileSelectorcomponent and itsParseFilterstype, enabling proper typing throughout the component.
100-104: LGTM!The filters state is properly initialized with empty arrays matching the
ParseFiltersinterface structure.
153-160: LGTM!The logic correctly handles both
READYandINFERRINGstates - advancing to step 2 in both cases while only skipping the polling forREADY. This enables chat during inference as intended by the PR.
184-204: LGTM!The local repo parsing correctly includes filters in the payload and mirrors the INFERRING state handling from
parseRepo, maintaining consistency between both parsing flows.
652-653: LGTM!The
FileSelectorcomponent is properly integrated with all required props. TheisParsingderivation correctly disables interaction when any parsing status is set.app/(main)/newchat/components/FileTree.tsx (1)
524-547: LGTM!The data fetching logic correctly handles loading states, errors, and transforms the API response with proper path building and empty directory filtering.
There was a problem hiding this comment.
Pull request overview
This PR implements a feature that allows users to start chatting during the AI inference phase of repository parsing. Previously, users had to wait until parsing was completely finished. Now, once the code is parsed (but while AI enrichment is still happening), users can begin interacting with their codebase. Additionally, the PR introduces comprehensive file filtering capabilities, allowing users to exclude specific directories, files, or file extensions from parsing.
Key Changes:
- Added
INFERRINGstatus to the parsing lifecycle, enabling chat functionality before full completion - Implemented file/folder exclusion UI with tree view, search, and pattern-based filtering
- Enhanced polling logic to transition users to chat step during the
INFERRINGphase while continuing to poll forREADYstatus
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| services/BranchAndRepositoryService.ts | Added getRepoStructure and checkParsingStatus methods; updated parseRepo and pollParsingStatus to support filters and INFERRING status |
| lib/Constants.tsx | Added INFERRING enum value to ParsingStatusEnum |
| components/agent-creation/ParsingProgress.tsx | Updated to display INFERRING status with appropriate progress bar and messaging |
| app/(main)/newchat/components/ParsingProgress.tsx | Added dedicated UI for INFERRING status with Sparkles icon and user-friendly messaging |
| app/(main)/newchat/components/step1.tsx | Integrated filter state management and fetching of existing filters; updated parsing logic to handle INFERRING status |
| app/(main)/newchat/components/FileTree.tsx | New component providing file tree visualization with search, pattern matching, and selection capabilities |
| app/(main)/newchat/components/FileSelector.tsx | New collapsible component providing UI for managing file exclusions with extensions, patterns, and tree selection |
| app/(main)/chat/[chatId]/page.tsx | Added banner for INFERRING status; implemented polling to detect READY transition; enabled chat during INFERRING |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| }, [isParsing]); | ||
|
|
||
| const updateFilter = (key: keyof ParseFilters, value: any) => { |
There was a problem hiding this comment.
The value parameter type is any, which bypasses type safety. Since this function updates filter values, it should be typed more specifically. The value should be typed as string[] since all filter properties in ParseFilters are string arrays. This will provide better type checking and prevent potential bugs.
| const updateFilter = (key: keyof ParseFilters, value: any) => { | |
| const updateFilter = (key: keyof ParseFilters, value: string[]) => { |
| } | ||
| } | ||
|
|
||
| static async checkParsingStatus(repoName: string, branchName: string, filters?: any) { |
There was a problem hiding this comment.
The filters parameter type is defined as any, which bypasses type safety. This should be typed as ParseFilters or a similar interface to ensure type safety and better IDE support. Consider importing the ParseFilters interface from the appropriate location or defining it in this service.
| useEffect(() => { | ||
| if (parsingStatus !== ParsingStatusEnum.INFERRING || !projectId) return; | ||
|
|
||
| const pollInterval = setInterval(async () => { | ||
| try { | ||
| const currentStatus = await BranchAndRepositoryService.getParsingStatus(projectId); | ||
| if (currentStatus === ParsingStatusEnum.READY) { | ||
| setParsingStatus(ParsingStatusEnum.READY); | ||
| } | ||
| } catch (error) { | ||
| console.error("Error polling parsing status:", error); | ||
| } | ||
| }, 5000); // Poll every 5 seconds | ||
|
|
||
| return () => clearInterval(pollInterval); | ||
| }, [parsingStatus, projectId]); |
There was a problem hiding this comment.
The polling interval cleanup could fail if the status changes to READY while a poll is in progress. When the status becomes READY, the interval is cleared but if getParsingStatus is currently executing, it could still update the status after cleanup. Consider tracking the interval ID in state and cleaning it up in a cleanup function, or using a ref to track if the component is still mounted.
Zingzy
force-pushed
the
feat-allow-chat-during-inderence
branch
from
141f89d to
3756d31
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/(main)/newchat/components/step1.tsx (1)
138-148: FixparseRepocall:filterspassed twice and argument list misaligned
BranchAndRepositoryService.parseRepoaccepts(repo_name, branch_name, filters?), but here it’s called with four arguments, passingfilterstwice:const parseResponse = await BranchAndRepositoryService.parseRepo( repo_name, branch_name, filters, filters );This will fail TypeScript checking and is misleading at runtime.
Update to pass
filtersonce:- const parseResponse = await BranchAndRepositoryService.parseRepo( - repo_name, - branch_name, - filters, - filters - ); + const parseResponse = await BranchAndRepositoryService.parseRepo( + repo_name, + branch_name, + filters + );
♻️ Duplicate comments (1)
services/BranchAndRepositoryService.ts (1)
13-27: Type thefiltersparameter instead ofanyin parse/check methodsBoth
parseRepo(Line 13) andcheckParsingStatus(Line 172) acceptfilters?: any. This defeats TypeScript’s type safety, especially sincefiltersis modeled asParseFilterselsewhere.Consider introducing a shared
ParseFilterstype in a common module and using it here:static async parseRepo( repo_name: string, branch_name: string, filters?: ParseFilters ) { /* ... */ } static async checkParsingStatus( repoName: string, branchName: string, filters?: ParseFilters ) { /* ... */ }This will tighten IDE support and reduce runtime shape mismatches between client and backend.
Also applies to: 172-187
🧹 Nitpick comments (3)
services/BranchAndRepositoryService.ts (1)
189-247: INFERRING handling + one‑time step‑2 advance look good, butgetStatusMessageis now unusedThe new
hasAdvancedToStep2flag and conditionif ( (parsingStatus === ParsingStatusEnum.INFERRING || parsingStatus === ParsingStatusEnum.READY) && !hasAdvancedToStep2 )correctly ensure
setChatStep(2)is fired once when parsing reaches INFERRING or READY, which aligns with “chat during inference” behavior.However,
getStatusMessage(Lines 200–215) is no longer used;setParsingStatusis called with the raw status string instead. Either:
- Remove
getStatusMessageto avoid dead code, or- Call
setParsingStatus(getStatusMessage(parsingStatus))if the UI should receive human‑readable messages.Right now it’s just unused overhead.
app/(main)/chat/[chatId]/page.tsx (1)
173-189: Avoid redundant polling of parsing status while INFERRINGThis effect starts its own 5‑second
setIntervalto callgetParsingStatuswheneverparsingStatus === INFERRING:useEffect(() => { if (parsingStatus !== ParsingStatusEnum.INFERRING || !projectId) return; const pollInterval = setInterval(async () => { const currentStatus = await BranchAndRepositoryService.getParsingStatus(projectId); if (currentStatus === ParsingStatusEnum.READY) { setParsingStatus(ParsingStatusEnum.READY); } }, 5000); return () => clearInterval(pollInterval); }, [parsingStatus, projectId]);But
parseRepoalready callsBranchAndRepositoryService.pollParsingStatus, which itself polls every 5 seconds until READY/ERROR/timeout and updatesparsingStatus. This effect therefore duplicates network calls and READY‑transition logic.Consider relying solely on
pollParsingStatusand removing this local interval to keep polling behavior centralized and easier to reason about.app/(main)/newchat/components/step1.tsx (1)
155-162: INFERRING / READY shortcut to step 2 matches new flow, though slightly redundant with service pollThe added logic in both
parseRepoandparseLocalRepo:// Advance to step 2 immediately if already READY or INFERRING if ( initialStatus === ParsingStatusEnum.READY || initialStatus === ParsingStatusEnum.INFERRING ) { setParsingStatus(initialStatus); setChatStep(2); if (initialStatus === ParsingStatusEnum.READY) { return; } }
- Correctly jumps the UI to step 2 when the backend already reports READY or INFERRING.
- Still lets
pollParsingStatusrun when INFERRING so you eventually land on READY.Given
pollParsingStatusnow also advances to step 2 on INFERRING/READY, this shortcut is slightly redundant but harmless and arguably clearer for readers who expect an immediate step‑2 transition on the first response.Also applies to: 199-206
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/(main)/chat/[chatId]/page.tsx(3 hunks)app/(main)/newchat/components/FileTree.tsx(1 hunks)app/(main)/newchat/components/ParsingProgress.tsx(3 hunks)app/(main)/newchat/components/step1.tsx(4 hunks)components/agent-creation/ParsingProgress.tsx(4 hunks)lib/Constants.tsx(1 hunks)services/BranchAndRepositoryService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- components/agent-creation/ParsingProgress.tsx
- app/(main)/newchat/components/FileTree.tsx
- app/(main)/newchat/components/ParsingProgress.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use PascalCase for component names and camelCase for utility names
Order imports: React/Next imports first, then components, then utilities
Use Next.js error boundaries, optional chaining, and nullish coalescing for error handling
Files:
services/BranchAndRepositoryService.tsapp/(main)/chat/[chatId]/page.tsxlib/Constants.tsxapp/(main)/newchat/components/step1.tsx
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.tsx: Use Tailwind CSS for styling and thecnutility for conditional classes
Use functional components with TypeScript FC types
Prefer destructuring props at the component level
Use the "use client" directive for client components
Files:
app/(main)/chat/[chatId]/page.tsxlib/Constants.tsxapp/(main)/newchat/components/step1.tsx
🧬 Code graph analysis (1)
app/(main)/chat/[chatId]/page.tsx (1)
services/BranchAndRepositoryService.ts (1)
BranchAndRepositoryService(11-249)
🪛 Biome (2.1.2)
app/(main)/newchat/components/step1.tsx
[error] 61-61: Shouldn't redeclare 'FileSelector'. Consider to delete it or rename it.
'FileSelector' is defined here:
(lint/suspicious/noRedeclare)
[error] 61-61: Shouldn't redeclare 'ParseFilters'. Consider to delete it or rename it.
'ParseFilters' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (4)
lib/Constants.tsx (1)
30-38: INFERRING status enum addition looks consistentThe new
ParsingStatusEnum.INFERRING = "inferring"value is well placed and matches usages elsewhere (polling, UI, gating). No issues from this change alone.app/(main)/chat/[chatId]/page.tsx (2)
156-171: Dialog behavior for INFERRING vs READY is consistent with new statusTreating
ParsingStatusEnum.INFERRINGlike READY for dialog visibility (closing the dialog in both cases) while still triggeringparseRepoonly for non‑READY / non‑INFERRING states keeps the UX coherent and avoids redundant polling kicks.
219-235: INFERRING banner, layout adjustment, and write gating align with “chat during inference”
- The INFERRING banner with
Sparklescommunicates enrichment clearly without interfering with the navbar.- Adjusting the content height only when INFERRING avoids layout jump otherwise.
writeDisabled={parsingStatus !== READY && parsingStatus !== ParsingStatusEnum.INFERRING}correctly enables chat once parsing reaches INFERRING or READY.These changes meet the “allow chat during inference” goal cleanly.
app/(main)/newchat/components/step1.tsx (1)
345-373: UsingcheckParsingStatusto pre‑load existing filters is reasonableThe new effect:
const statusResponse = await BranchAndRepositoryService.checkParsingStatus(repoName, branchName); if (statusResponse?.current_filters) { // … setFilters({ ... }); }is a good way to hydrate
filtersfrom server state when repo/branch change, while resetting to defaults first. Returningnullon error in the service and swallowing fetch errors here keeps the UI resilient.No issues from this addition.
| import FileSelector, { ParseFilters } from "./FileSelector"; | ||
| import FileSelector, { ParseFilters } from "./FileSelector"; | ||
|
|
There was a problem hiding this comment.
Remove duplicate FileSelector / ParseFilters import
FileSelector, { ParseFilters } is imported twice, which will cause a lint/compile error:
import FileSelector, { ParseFilters } from "./FileSelector";
import FileSelector, { ParseFilters } from "./FileSelector";Keep only a single import:
import FileSelector, { ParseFilters } from "./FileSelector";
-import FileSelector, { ParseFilters } from "./FileSelector";🧰 Tools
🪛 Biome (2.1.2)
[error] 61-61: Shouldn't redeclare 'FileSelector'. Consider to delete it or rename it.
'FileSelector' is defined here:
(lint/suspicious/noRedeclare)
[error] 61-61: Shouldn't redeclare 'ParseFilters'. Consider to delete it or rename it.
'ParseFilters' is defined here:
(lint/suspicious/noRedeclare)
🤖 Prompt for AI Agents
In app/(main)/newchat/components/step1.tsx around lines 60 to 62, there is a
duplicated import statement for FileSelector and ParseFilters which will cause a
lint/compile error; remove the redundant line so only a single import of
FileSelector, { ParseFilters } from "./FileSelector" remains and ensure any
unused import references are handled.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/(main)/chat/[chatId]/page.tsx (1)
173-189: Duplicate polling mechanism detected.This effect adds a second polling mechanism that overlaps with the existing
parseRepofunction (lines 60-79). Both pollgetParsingStatusevery 5 seconds:
- The existing
parseRepousesBranchAndRepositoryService.pollParsingStatus, which already includes 5-second interval polling with additional features (max duration, step advancement, error handling)- This new effect creates a simpler but redundant poll
This duplication results in concurrent API calls to the same endpoint and adds unnecessary complexity.
Consider consolidating the polling logic:
- // Poll for status updates when in INFERRING state to detect READY transition - useEffect(() => { - if (parsingStatus !== ParsingStatusEnum.INFERRING || !projectId) return; - - const pollInterval = setInterval(async () => { - try { - const currentStatus = await BranchAndRepositoryService.getParsingStatus(projectId); - if (currentStatus === ParsingStatusEnum.READY) { - setParsingStatus(ParsingStatusEnum.READY); - } - } catch (error) { - console.error("Error polling parsing status:", error); - } - }, 5000); // Poll every 5 seconds - - return () => clearInterval(pollInterval); - }, [parsingStatus, projectId]);The existing
parseRepomechanism should handle INFERRING→READY transitions automatically through the service-level polling.Note: The cleanup race condition mentioned in the past review comment also applies to this polling implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/(main)/chat/[chatId]/page.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use PascalCase for component names and camelCase for utility names
Order imports: React/Next imports first, then components, then utilities
Use Next.js error boundaries, optional chaining, and nullish coalescing for error handling
Files:
app/(main)/chat/[chatId]/page.tsx
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.tsx: Use Tailwind CSS for styling and thecnutility for conditional classes
Use functional components with TypeScript FC types
Prefer destructuring props at the component level
Use the "use client" directive for client components
Files:
app/(main)/chat/[chatId]/page.tsx
🧬 Code graph analysis (1)
app/(main)/chat/[chatId]/page.tsx (1)
services/BranchAndRepositoryService.ts (1)
BranchAndRepositoryService(11-249)
🔇 Additional comments (4)
app/(main)/chat/[chatId]/page.tsx (4)
16-16: LGTM!The Sparkles icon import is correctly added and used in the INFERRING status banner.
156-171: LGTM!The dialog state logic correctly treats INFERRING status as equivalent to READY, allowing the parsing dialog to close and enabling users to chat during the inference phase.
217-227: LGTM!The INFERRING status banner is well-implemented with appropriate styling, dark mode support, and clear user messaging. The animated Sparkles icon provides good visual feedback.
232-232: LGTM!The
writeDisabledlogic correctly enables chat input during both READY and INFERRING states, aligning with the PR objective to allow users to chat while inference is in progress.
| return () => clearInterval(pollInterval); | ||
| }, [parsingStatus, projectId]); | ||
|
|
||
| if (Error.isError) |
There was a problem hiding this comment.
Critical bug: incorrect error state check.
Line 191 references Error.isError, but Error is the JavaScript Error class, not the state variable. This condition will always evaluate to undefined, breaking error handling.
Apply this diff:
- if (Error.isError)
+ if (errorState.isError)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (Error.isError) | |
| if (errorState.isError) |
🤖 Prompt for AI Agents
In app/(main)/chat/[chatId]/page.tsx around line 191, the condition uses the
global Error class (Error.isError) which is incorrect; replace it with the
component's error state or object check (for example use the local boolean state
variable isError: if (isError) or check the actual error object: if (error !=
null) / if (error instanceof Error)). Ensure the referenced state/prop (isError
or error) is defined in scope before using it.
| </div> | ||
| </div> | ||
| )} | ||
| <div className={parsingStatus === ParsingStatusEnum.INFERRING ? "h-[calc(90vh-40px)]" : "h-[calc(90vh)]"}> |
There was a problem hiding this comment.
Hardcoded height offset may not match actual banner height.
The calculation uses a fixed 40px offset for the INFERRING banner, but the actual banner height depends on padding (py-2.5), border, font size, and content. This could cause minor layout inconsistencies or spacing issues if the banner renders at a different height.
Consider using CSS Grid or Flexbox to dynamically allocate space, or measure the banner height with a ref:
- <div className={parsingStatus === ParsingStatusEnum.INFERRING ? "h-[calc(90vh-40px)]" : "h-[calc(90vh)]"}>
+ <div className="flex-1 overflow-hidden">Then adjust the parent container to use flex flex-col layout (which is already present at line 211).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className={parsingStatus === ParsingStatusEnum.INFERRING ? "h-[calc(90vh-40px)]" : "h-[calc(90vh)]"}> | |
| <div className="flex-1 overflow-hidden"> |
🤖 Prompt for AI Agents
In app/(main)/chat/[chatId]/page.tsx around line 228, the container height is
computed with a hardcoded 40px offset for the INFERRING banner which can be
wrong; change the layout to rely on flexbox so the banner's variable height is
handled automatically — make the parent a flex column (ensure the element at
~line 211 has `flex flex-col` and the content area uses `flex-1 overflow-auto`
instead of manual calc heights) or alternatively measure the banner height with
a ref and subtract that dynamic value when computing the content area height;
implement one of these two approaches so the content area size adapts to the
banner’s actual rendered height.
|
Summary by CodeRabbit
New Features
Bug Fixes / UX
✏️ Tip: You can customize this high-level summary in your review settings.