refactor(ui): centralise state via AppContext, fix code quality issues#118
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's GuideRefactors the renderer UI to centralize shared state in a new AppContext, improve selection performance with Set-based state and batch operations, preserve tab state via CSS-based visibility, replace alert dialogs with an inline ErrorBanner, extract a shared Spinner component, and address several SonarQube and code-quality issues in both renderer and main process code. Sequence diagram for SourceTab analyze flow via AppContextsequenceDiagram
actor User
participant SourceTab
participant AppContext
participant ElectronAPI as electronAPI
participant MainProcess
participant ErrorBanner
User->>SourceTab: click Process selected files
SourceTab->>AppContext: handleAnalyze()
AppContext->>AppContext: collect selectedFiles as array
AppContext->>AppContext: validate rootPath and selectedFiles
alt no rootPath or no valid files
AppContext->>AppContext: set appError
AppContext-->>SourceTab: return undefined
SourceTab->>ErrorBanner: renders error from context
else valid input
AppContext->>ElectronAPI: analyzeRepository({rootPath, configContent, selectedFiles})
ElectronAPI->>MainProcess: ipcMain.handle analyzeRepository
MainProcess-->>ElectronAPI: AnalyzeRepositoryResult
ElectronAPI-->>AppContext: AnalyzeRepositoryResult
AppContext->>AppContext: update analysisResultRef
AppContext->>AppContext: derive ProcessingOptions from configContent
AppContext->>ElectronAPI: processRepository({rootPath, filesInfo, options})
ElectronAPI->>MainProcess: ipcMain.handle processRepository
MainProcess-->>ElectronAPI: ProcessRepositoryResult
ElectronAPI-->>AppContext: ProcessRepositoryResult
AppContext->>AppContext: set processedResult
AppContext->>AppContext: set activeTab = processed
AppContext-->>SourceTab: resolve Promise
SourceTab->>ProcessedTab: rendered due to activeTab
end
Sequence diagram for FileTree batch select-all using Set-based statesequenceDiagram
actor User
participant FileTree
participant AppContext
User->>FileTree: toggle Select all checkbox
FileTree->>FileTree: compute allPaths = {files[], folders[]}
alt onBatchSelect provided
FileTree->>AppContext: onBatchSelect(files, folders, isSelected)
alt isSelected = true
AppContext->>AppContext: add valid files to selectedFiles Set
AppContext->>AppContext: add valid folders to selectedFolders Set
else isSelected = false
AppContext->>AppContext: delete files from selectedFiles Set
AppContext->>AppContext: delete folders from selectedFolders Set
end
else fallback per-item
loop each file in files
FileTree->>AppContext: handleFileSelect(filePath, isSelected)
end
loop each folder in folders
FileTree->>AppContext: handleFolderSelect(folderPath, isSelected)
end
end
AppContext-->>FileTree: updated selectedFiles, selectedFolders
FileTree->>FileTree: recompute select-all checked state via Set.size
Class diagram for centralised AppContext state and UI componentsclassDiagram
class AppProvider {
+AppProviderProps props
-TabId activeTab
-string rootPath
-DirectoryTreeItem[] directoryTree
-Set~string~ selectedFiles
-Set~string~ selectedFolders
-AnalyzeRepositoryResult? analysisResultRef
-ProcessRepositoryResult? processedResult
-ProcessingOptions processingOptions
-string configContent
-AppError? appError
+AppProvider(props)
}
class AppContext {
<<context>>
+TabId activeTab
+string rootPath
+DirectoryTreeItem[] directoryTree
+Set~string~ selectedFiles
+Set~string~ selectedFolders
+ProcessRepositoryResult? processedResult
+string configContent
+ProcessingOptions processingOptions
+AppError? appError
+switchTab(tab: TabId) void
+selectDirectory() Promise~void~
+refreshDirectoryTree() Promise~void~
+updateConfig(config: string) void
+handleFileSelect(filePath: string, isSelected: boolean) void
+handleFolderSelect(folderPath: string, isSelected: boolean) void
+handleBatchSelect(files: string[], folders: string[], isSelected: boolean) void
+handleAnalyze() Promise~AnalyzeRepositoryResult?|undefined~
+handleRefreshProcessed() Promise~ProcessRepositoryResult?|null~
+handleSaveOutput() Promise~void~
+dismissError() void
}
class useApp {
<<hook>>
+useApp() AppContextValue
}
class AppContent {
+AppContent()
-TabId activeTab
-string rootPath
-DirectoryTreeItem[] directoryTree
-Set~string~ selectedFiles
-Set~string~ selectedFolders
-ProcessRepositoryResult? processedResult
-string configContent
+render() JSX.Element
}
class ErrorBanner {
+ErrorBanner()
-AppError? appError
-dismissError() void
+render() JSX.Element|null
}
class ConfigTab {
+ConfigTabProps props
-ConfigFormState formState
-boolean isSaved
-string[] providerValidationErrors
-ProviderConnectionResult? providerTestResult
+saveConfig() void
+handleTestProviderConnection() Promise~void~
+render() JSX.Element
}
class ConfigFormState {
+boolean useCustomExcludes
+boolean useCustomIncludes
+boolean useGitignore
+boolean enableSecretScanning
+boolean excludeSuspiciousFiles
+boolean includeTreeView
+boolean showTokenCount
+ExportFormat exportFormat
+string fileExtensions
+string excludePatterns
+ProviderId|'' providerId
+string providerModel
+string providerApiKey
+string providerBaseUrl
}
class SourceTab {
+SourceTabProps props
-boolean isAnalyzing
-boolean isCalculating
-boolean showTokenCount
-TokenCache tokenCache
+onDirectorySelect() Promise~void~
+onFileSelect(filePath: string, isSelected: boolean) void
+onFolderSelect(folderPath: string, isSelected: boolean) void
+onBatchSelect(files: string[], folders: string[], isSelected: boolean) void
+onAnalyze() Promise~unknown~
+onRefreshTree() Promise~void~
+render() JSX.Element
}
class FileTree {
+FileTreeProps props
-DirectoryTreeItem[] items
-Set~string~ selectedFiles
-Set~string~ selectedFolders
+onFileSelect(filePath: string, isSelected: boolean) void
+onFolderSelect(folderPath: string, isSelected: boolean) void
+onBatchSelect(files: string[], folders: string[], isSelected: boolean) void
+render() JSX.Element
}
class ProcessedTab {
+ProcessedTabProps props
-boolean isRefreshing
+onSave() Promise~void~
+onRefresh() Promise~void~
+render() JSX.Element
}
class Spinner {
+string? className
+string? data-testid
+Spinner(props: SpinnerProps)
+render() SVGElement
}
class ProcessingOptions {
+boolean showTokenCount
+boolean includeTreeView
+ExportFormat exportFormat
}
class AppError {
+string message
+number timestamp
}
AppProvider --> AppContext : provides
useApp --> AppContext : consumes
AppProvider --> AppContent : wraps
AppContent --> ErrorBanner : renders
AppContent --> ConfigTab : renders
AppContent --> SourceTab : renders
AppContent --> ProcessedTab : renders
ConfigTab --> useApp : uses rootPath, selectDirectory, switchTab
ConfigTab --> ConfigFormState : manages via useReducer
SourceTab --> FileTree : renders
SourceTab --> Spinner : uses
SourceTab --> useApp : uses configContent via props from context
ProcessedTab --> Spinner : uses
FileTree --> useApp : indirect via handlers passed from context
AppContext ..> ProcessingOptions : uses
AppContext ..> AppError : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughCentralizes renderer state into a new AppContext, migrates selection arrays to Sets across UI, consolidates main-process directory traversal with a processEntry helper, adds a reusable Spinner component, and updates configuration handling to a reducer pattern; also removes one dependency and tweaks vault env config. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (UI)
participant App as App (React)
participant Context as AppContext
participant Electron as ElectronBridge
participant Main as Main Process
participant FS as Filesystem
User->>App: click Analyze / Select folder
App->>Context: dispatch action / call useApp API
Context->>Electron: analyzeRepository / processRepository
Electron->>Main: IPC -> handle analyze/process
Main->>FS: traverse directory (processEntry per item)
FS-->>Main: directory items / st_mode / symlink info
Main-->>Electron: traversal result (directoryTree)
Electron-->>Context: return directoryTree / processed results
Context-->>App: update state (directoryTree, processedResult)
App-->>User: render updated tree / results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Review Summary by QodoRefactor UI state management with AppContext, fix SonarQube issues, and improve performance
WalkthroughsDescription• **Refactored App.tsx** from 715-line god component to thin shell (179 lines) with centralized state management via AppContext provider • **Consolidated state management** in ConfigTab from 19 useState hooks to single useReducer with context integration • **Improved selection performance** in FileTree by converting from Array.includes() O(n) to Set.has() O(1) lookups with batch select-all support • **Preserved tab state** using CSS hidden class instead of unmount/remount pattern • **Replaced alert() dialogs** with dismissible ErrorBanner component for better UX • **Extracted shared Spinner component** to eliminate code duplication across SourceTab and ProcessedTab • **Eliminated window globals** (switchToTab, refreshDirectoryTree) and 500ms localStorage polling in favor of React Context data flow • **Fixed all 4 SonarQube issues**: cognitive complexity (S3776), Set usage (S7776), codePointAt for Unicode (S7758), type assertion (S4325) • **Added semantic HTML** with role='treeitem' and aria-selected attributes for accessibility • **Updated all unit tests** (318/318 passing) for Set-based props, context mocks, and error banner assertions Diagramflowchart LR
A["God Component<br/>App.tsx<br/>715 lines"] -->|"Extract state"| B["AppContext<br/>Centralized state"]
C["19 useState<br/>ConfigTab"] -->|"Consolidate"| D["useReducer<br/>+ Context"]
E["Array.includes<br/>O(n) selection"] -->|"Convert to Set"| F["Set.has<br/>O(1) lookup"]
G["alert() dialogs"] -->|"Replace with"| H["ErrorBanner<br/>Dismissible UI"]
I["Inline SVG<br/>Spinners"] -->|"Extract"| J["Spinner<br/>Component"]
K["Window globals<br/>+ localStorage"] -->|"Eliminate"| B
B --> L["4 SonarQube<br/>issues fixed"]
File Changes1. src/main/index.ts
|
Summary of ChangesHello @Mehdi-Bl, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant architectural refactor to centralize state management using React Context, enhancing the application's maintainability, performance, and user experience. It addresses several code quality issues, streamlines UI interactions, and improves the efficiency of file selection and error reporting across the application. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- AppContext.tsx is taking on a lot of responsibilities (error handling, config persistence, path utils, selection logic, IPC orchestration); consider extracting reusable helpers (e.g., path boundary utilities, error helpers, and maybe selection helpers) into separate modules to keep the provider focused and easier to maintain.
- In SourceTab, the token-counting effect relies on a derived
selectedFilesArraybut suppresses the dependency lint rule; you could avoid the eslint-disable by memoizingselectedFilesArraywithuseMemoand using that in the effect dependencies to keep the logic explicit and safer to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- AppContext.tsx is taking on a lot of responsibilities (error handling, config persistence, path utils, selection logic, IPC orchestration); consider extracting reusable helpers (e.g., path boundary utilities, error helpers, and maybe selection helpers) into separate modules to keep the provider focused and easier to maintain.
- In SourceTab, the token-counting effect relies on a derived `selectedFilesArray` but suppresses the dependency lint rule; you could avoid the eslint-disable by memoizing `selectedFilesArray` with `useMemo` and using that in the effect dependencies to keep the logic explicit and safer to maintain.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Dependency Review✅ No vulnerabilities or license issues found.Scanned Files
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent and comprehensive refactoring of the UI state management. Centralizing state in AppContext and removing the "god component" pattern in App.tsx is a huge improvement for maintainability and clarity. The switch to useReducer in ConfigTab, the performance optimizations using Set in FileTree, and the improved UX with the ErrorBanner and state preservation on tab switching are all fantastic changes. The code quality is high, and the accompanying test updates are thorough. I've found one minor opportunity for performance improvement.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@docs/plan/ui_fixes.md`:
- Around line 3-4: Update the test count in the doc line that reads "Three-agent
validated review (Claude Opus 4.6, Codex GPT-5.3, Gemini). All findings
confirmed against source. Lint: PASS, Tests: PASS (35 suites, 317 tests)." to
reflect the actual number reported in the PR (change "317 tests" to "318 tests")
so the plan and PR description match exactly.
In `@src/renderer/components/ConfigTab.tsx`:
- Around line 384-387: handleFolderSelect unconditionally calls
switchTab('source') even when selectDirectory() is a no-op on cancel; update
selectDirectory in AppContext (e.g., return Promise<boolean> or
Promise<string|null> from the selectDirectory function in AppContext.tsx) to
indicate whether a directory was selected, then modify handleFolderSelect to
await selectDirectory and only call switchTab('source') when the returned value
indicates success (true or a non-null path) to prevent switching tabs on cancel.
In `@src/renderer/components/FileTree.tsx`:
- Around line 248-264: The selectAllChecked memo can return true incorrectly
when selectedFiles.size equals totalFiles but contains stale paths; update the
logic in selectAllChecked to always validate against the current tree by
collecting current file paths (using collectFilePaths(items, validFilePaths))
and then verifying that every path in validFilePaths exists in selectedFiles
(short-circuiting on the first missing file) instead of relying on
selectedFiles.size; keep the dependency array [items, selectedFiles, totalFiles]
and ensure the function returns true only when all current file paths are
present in selectedFiles.
In `@src/renderer/context/AppContext.tsx`:
- Around line 232-259: The updater passed to setActiveTab inside switchTab
contains side effects (localStorage access, yaml.parse, setProcessingOptions,
setProcessedResult, and modifying analysisResultRef) which must be moved outside
the state-updater to keep the updater pure; refactor switchTab to read the
current tab from a stable ref (e.g., activeTabRef) or from the outer scoped
activeTab state to perform the prevTab === tab guard, then if the tab actually
changes call setActiveTab(tab) and run the side effects (read localStorage,
parse YAML, call setProcessingOptions with computed values using
normalizeExportFormat, and when tab === 'source' clear analysisResultRef.current
and call setProcessedResult(null)); ensure the updater passed to setActiveTab
only returns the new tab value and does not perform any I/O or state-setting
side effects.
🧹 Nitpick comments (7)
src/main/index.ts (1)
563-612:processEntrycleanly centralizes per-item logic.The extraction reduces duplication and cognitive complexity. One observation:
Lines 575–581: All symlinks are skipped, but the warning only logs for symlinks pointing outside the root. Symlinks within the root are silently dropped — this could surprise users whose projects rely on in-tree symlinks. Consider logging at
debuglevel for the in-root case too, or adding a brief inline comment explaining the intent.Optional: clarify silent symlink skip
if (isSymbolicLink) { const resolvedSymlinkPath = resolveRealPath(itemPath); if (!isPathWithinRoot(authorizedDirPath, resolvedSymlinkPath)) { console.warn(`Skipping symlink outside current root directory: ${itemPath}`); } + // All symlinks are excluded from the tree to avoid traversal cycles and path confusion. return null; }tests/unit/components/file-tree.test.tsx (1)
191-222: Missing test coverage for batch deselect path.The batch handler test (Line 166) only verifies the
selectcase (true). The "deselects all" test (Line 191) doesn't passonBatchSelect, so the batch deselect path (onBatchSelect(…, …, false)) is never exercised.Consider adding a test that passes
onBatchSelectwith all items pre-selected, clicks "Select All" to deselect, and assertsmockBatchSelectwas called withfalse.src/renderer/components/App.tsx (1)
12-31: ErrorBanner: clean and accessible inline component.Consider extracting it to its own file (e.g.,
components/ErrorBanner.tsx) for reusability and testability, especially since the PR summary mentions it as a distinct feature. This is optional given its small size.src/renderer/components/SourceTab.tsx (2)
84-84:selectedFilesArrayis recomputed on every render.The spread
[...selectedFiles]runs on each render, even whenselectedFileshasn't changed. Consider wrapping inuseMemoto avoid unnecessary allocations during unrelated re-renders (e.g.,isAnalyzingorisCalculatingstate changes).Proposed fix
- const selectedFilesArray = [...selectedFiles]; + const selectedFilesArray = useMemo(() => [...selectedFiles], [selectedFiles]);You'd need to add
useMemoto the existing import on Line 1.
361-370: "Clear selection" button uses per-item calls instead of batch.The
onBatchSelectprop is available but the clear button still iterates individually over both Sets. This misses the optimization thatonBatchSelectprovides (single state update vs N+M updates).Proposed fix
onClick={() => { - for (const filePath of selectedFiles) onFileSelect(filePath, false); - for (const folderPath of selectedFolders) onFolderSelect(folderPath, false); + onBatchSelect([...selectedFiles], [...selectedFolders], false); setTotalTokens(0);src/renderer/context/AppContext.tsx (2)
467-479: Duplicated processing-options-from-config parsing logic.The same YAML→
ProcessingOptionsextraction appears inhandleAnalyze(Lines 467–479),handleRefreshProcessed(Lines 528–540), andswitchTab(Lines 236–247). Consider extracting a shared helper:const parseProcessingOptions = (configStr: string | null): ProcessingOptions => { const defaults: ProcessingOptions = { showTokenCount: true, includeTreeView: false, exportFormat: 'markdown' }; if (!configStr) return defaults; try { const config = (yaml.parse(configStr) || {}) as ConfigObject; return { showTokenCount: config.show_token_count !== false, includeTreeView: config.include_tree_view === true, exportFormat: normalizeExportFormat(config.export_format), }; } catch { return defaults; } };Also applies to: 528-540
304-396: Nested helper functions (findFolder,getAllSubFolders,getAllFiles) insideuseCallback.These tree-traversal helpers are recreated whenever
rootPathordirectoryTreechanges. Since they're pure functions (aside from therootPathclosure for boundary checks), consider extracting them to module-level functions that acceptrootPathas a parameter. This improves readability and testability.
Code Review by Qodo
1. ErrorBanner shows raw Error.message
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Mehdi-Bl
force-pushed
the
fix/ui-code-organisation-review
branch
from
4dfb545 to
06cb22d
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/main/index.ts`:
- Around line 563-612: processEntry currently returns null for every symbolic
link even when the symlink resolves inside the authorized root; change the logic
so you only skip symlinks whose resolved path is outside the root by using
resolveRealPath(resolvedSymlinkPath) + isPathWithinRoot(authorizedDirPath,
resolvedSymlinkPath) and only return null for out-of-root targets, otherwise
proceed to treat the symlink target as the entry (use the resolved path for
subsequent readPathStats/walkFn calls and rely on visitedDirectoryRealPaths to
avoid cycles); if the original early-return was intentional for
defence-in-depth, add a clear comment above the symlink handling explaining that
choice instead of silently dropping all symlinks.
In `@src/renderer/components/ConfigTab.tsx`:
- Around line 178-193: initialFormState sets includeTreeView: true which
conflicts with AppContext's default processingOptions.includeTreeView: false
causing a brief UI flash; update initialFormState.includeTreeView to false (or
align AppContext.processingOptions.includeTreeView to true) so both defaults
match, and verify the form state and AppContext (symbols: initialFormState,
includeTreeView, AppContext, processingOptions.includeTreeView,
LOAD_FROM_CONFIG) are consistent to prevent the checkbox showing a different
state before LOAD_FROM_CONFIG runs.
In `@src/renderer/context/AppContext.tsx`:
- Around line 563-612: The symlink branch in processEntry currently always
returns null after resolving a symlink even when the symlink target lives inside
the repo; change it so the early return is conditional: resolve the symlink
target (using the existing resolved/target variables), compute its real path,
and if the targetRealPath is inside the rootRealPath proceed to process that
target (and use visitedDirectoryRealPaths to avoid cycles) but if it's outside
the root log/warn and then return null; alternatively, if skipping all symlinks
was intended, replace the dead warn-only branch with a clear comment explaining
that design decision and keep the unconditional return — reference processEntry
and visitedDirectoryRealPaths to locate the change.
- Around line 528-540: handleRefreshProcessed currently reads config from
localStorage (localStorage.getItem('configContent')) which can be stale or
sanitized; change it to use the React state configContent (same source used by
handleAnalyze) when building ProcessingOptions. Locate handleRefreshProcessed
and replace the localStorage parse with parsing yaml.parse(configContent || '')
(with the same fallback/normalize logic you use elsewhere), preserve the
try/catch and ensure options.showTokenCount, options.includeTreeView and
options.exportFormat are set with normalizeExportFormat; then call
setProcessingOptions(options) as before so the in-memory state is the single
source of truth.
In `@vault-agent-env.hcl`:
- Line 21: Update the HCL template to document and/or harden the fallback
behavior: add a clear comment above the contents line that references the
specific keys used (.Data.data.SONAR_TOKEN, .Data.data.sonar_token and the
generic .Data.data.token) and explain that .Data.data.token is a legacy generic
fallback that can silently return unrelated secrets (also mention the
DTRACK_API_KEY usage at the other similar block), and either remove the generic
`.Data.data.token` fallback or set error_on_missing_key = true if you want
strict failures; include the same comment/changes for the second instance that
references DTRACK_API_KEY so future maintainers know the expected Vault key
layout and the risks of the generic `token` key.
🧹 Nitpick comments (7)
vault-agent-env.hcl (1)
4-4: Hardcoded paths reduce portability across environments.Replacing the previous environment-variable references (
VAULT_TOKEN_FILE,VAULT_AGENT_TOKEN_SINK_FILE) with hardcoded paths ties this config to a specific devcontainer layout (/home/vscode/…). If this file is only consumed inside the devcontainer that's acceptable, but it will silently break in any other environment (CI runner, production host, different container image with a non-vscodeuser).Consider restoring the env-var indirection or at least documenting the assumption prominently.
Also applies to: 10-10
src/renderer/context/AppContext.tsx (2)
330-360: Quadratic array growth in recursive helpersgetAllSubFolders/getAllFiles.Lines 337 and 355 use spread (
[...files, ...getAllFiles(item)]) inside a loop, which copies the entire accumulated array on every directory entry. For a tree with n files and d nested directories this is O(n·d) allocations instead of O(n).Suggested refactor: push into a single array
const getAllFiles = (folder: DirectoryTreeItem): string[] => { if (!folder.children) return []; - let files: string[] = []; + const files: string[] = []; for (const item of folder.children ?? []) { if (item.type === 'file') { if (isPathWithinRootBoundary(item.path, rootPath)) { files.push(item.path); } } else if (item.type === 'directory') { - files = [...files, ...getAllFiles(item)]; + files.push(...getAllFiles(item)); } } return files; };Apply the same pattern to
getAllSubFolders(Line 337).
220-230:refreshDirectoryTreehasappWindow(globalThis) in its dependency array.
appWindowis assigned asglobalThis(Line 164) and never changes. Including it in theuseCallbackdependency array is harmless but misleading — it suggests the callback would be recreated if the window reference changed, which can't happen. The same applies toselectDirectory(Line 285) and theprevTabRefeffect (Line 269).src/renderer/components/App.tsx (1)
12-31: ErrorBanner: consider auto-dismiss or staleness handling.The banner relies solely on manual dismiss. If an error is shown while the user is on a different tab (tabs stay mounted now), it may go unnoticed or persist indefinitely. The
AppError.timestampfield is already available — you could use it to auto-dismiss after a timeout or visually fade stale errors.src/renderer/components/SourceTab.tsx (2)
84-84:selectedFilesArrayis spread from the Set on every render.
[...selectedFiles]runs on every render regardless of whetherselectedFileschanged. For large selections this is wasteful. Wrapping it inuseMemoscopes the work to actual changes:Suggested fix
- const selectedFilesArray = [...selectedFiles]; + const selectedFilesArray = useMemo(() => [...selectedFiles], [selectedFiles]);(Add
useMemoto the existing React import on Line 1.)
361-365: "Clear selection" bypassesonBatchSelect, falling back to per-item deselect.
handleSelectAllTogglein FileTree already short-circuits toonBatchSelectfor select/deselect-all. Here the clear button manually iterates both Sets and fires individual handlers. For consistency and performance, consider usingonBatchSelect:Suggested fix
onClick={() => { - for (const filePath of selectedFiles) onFileSelect(filePath, false); - for (const folderPath of selectedFolders) onFolderSelect(folderPath, false); + onBatchSelect([...selectedFiles], [...selectedFolders], false); setTotalTokens(0);tests/unit/components/app.test.tsx (1)
106-106: ConsiderPropTypes.instanceOf(Set)instead ofPropTypes.any.
PropTypes.anysilently accepts wrong types (e.g., a plain array), which could mask regressions in the prop contract. Since the production code now passes aSet, a tighter check would catch accidental reversions.Suggested change
- selectedFiles: PropTypes.any.isRequired, + selectedFiles: PropTypes.instanceOf(Set).isRequired,
- Replace 715-line App.tsx god component with thin shell + AppContext provider - Introduce useReducer in ConfigTab (19 useState → single reducer) - Convert selection arrays to Set<string> for O(1) lookups in FileTree - Add batch select handler for O(n) select-all instead of N state updates - Fix nested interactive elements: outer <button> → <div role="treeitem"> - Add role="tree" container for a11y compliance - Replace alert() calls with inline ErrorBanner component - Preserve tab state via CSS hidden class instead of conditional unmounting - Extract shared Spinner component, remove @heroicons/react dependency - Remove window.switchToTab / window.refreshDirectoryTree global mutations - Remove 500ms localStorage polling interval - Pipe configContent as prop to SourceTab to prevent stale reads - Fix show_token_count default mismatch (=== true → !== false) - Fix SonarQube S3776: extract processEntry helper to reduce walkDirectory cognitive complexity from 19 to within allowed limit - Fix SonarQube S7776: SUPPORTED_PROVIDER_IDS array → Set with .has() - Fix SonarQube S7758: charCodeAt → codePointAt - Fix SonarQube S4325: remove unnecessary type assertion - Update vault-agent-env.hcl template paths and secret fallbacks All tests pass (318 unit, 3 stress, 4 E2E). Lint 0 warnings. SonarQube 0 issues.
Mehdi-Bl
force-pushed
the
fix/ui-code-organisation-review
branch
from
06cb22d to
476dc00
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/renderer/context/AppContext.tsx`:
- Around line 235-258: The switchTab callback currently reads the serialized
config from localStorage via localStorage.getItem('configContent') instead of
using the in-memory React state; change switchTab to use the configContent state
value (the same state used by handleAnalyze) when building ProcessingOptions
(parse yaml.parse(configContent) if configContent is a YAML string or use the
already-parsed object) and remove the localStorage read; also add configContent
to switchTab's dependency array so the callback is recreated when state changes;
keep existing error handling and ensure you still respect sanitized api_key
handling elsewhere.
- Around line 176-214: The save-effect currently writes the placeholder '#
Loading configuration...' to localStorage on mount; update the effect that calls
localStorage.setItem('configContent', sanitizeConfigForStorage(configContent))
to skip saving when configContent is the initial placeholder (or while an
"isInitialMount" ref is true), e.g., add a guard checking configContent !== '#
Loading configuration...' (or use a useRef isInitialMount to ignore the first
render) before calling sanitizeConfigForStorage and localStorage.setItem;
reference the useEffect that depends on [configContent], the configContent
state, and the sanitizeConfigForStorage helper to locate and modify the code.
🧹 Nitpick comments (6)
src/renderer/components/App.tsx (1)
120-164: Tab panels preserve state correctly via CSShiddenclass.Using
absolute inset-0with ahiddenclass toggle is a solid approach to keep all tab components mounted while only showing the active one. This preserves scroll position, form state, and selection state across tab switches.One minor observation: each tab panel has an identical set of base classes. If these grow, consider extracting a small helper or constant to reduce duplication.
♻️ Optional: extract shared tab panel classes
+const tabPanelBaseClass = + 'absolute inset-0 overflow-y-auto bg-white dark:bg-gray-800 p-4 text-gray-900 dark:text-gray-100 transition-colors duration-200'; + <div role='tabpanel' id='tabpanel-config' aria-labelledby='tab-config' - className={`absolute inset-0 overflow-y-auto bg-white dark:bg-gray-800 p-4 text-gray-900 dark:text-gray-100 transition-colors duration-200 ${activeTab === 'config' ? '' : 'hidden'}`} + className={`${tabPanelBaseClass} ${activeTab === 'config' ? '' : 'hidden'}`} >src/renderer/components/SourceTab.tsx (1)
84-84:selectedFilesArrayis spread on every render.
[...selectedFiles]creates a new array on each render. It's used only inside the token-calculation effect, where it's correctly gated by theselectedFilesdependency. This is fine for typical selection sizes, but for very large repositories (thousands of files), consider memoizing withuseMemo:♻️ Optional: memoize the derived array
- const selectedFilesArray = [...selectedFiles]; + const selectedFilesArray = React.useMemo(() => [...selectedFiles], [selectedFiles]);src/renderer/context/AppContext.tsx (1)
306-398:handleFolderSelectinlines three recursive helpers — consider extracting.
findFolder,getAllSubFolders, andgetAllFilesare re-created on every invocation. They're also generally useful tree utilities. Extracting them as standalone functions (outside the component) would improve readability and allow reuse. TherootPathdependency can be passed as a parameter.This isn't urgent given the current scope, but it would reduce the cognitive complexity of this already large provider.
src/renderer/components/ConfigTab.tsx (1)
138-140:SET_FIELDaction value type is loosely typed at the reducer level.
ConfigFormAction'sSET_FIELDallowsvalue: ConfigFormState[keyof ConfigFormState], which is a union of all possible field types (boolean | string | ExportFormat | ProviderId | ''). This means the reducer itself won't catch a mismatch like setting a boolean field to a string. The genericsetFieldhelper at Line 396 correctly constrains this at call sites, but a directdispatch({ type: 'SET_FIELD', field: 'useGitignore', value: 'oops' })would compile without error.This is a minor type-safety gap since all dispatches go through
setField. No action needed now, but worth noting ifdispatchis ever exposed directly.src/renderer/components/FileTree.tsx (2)
83-99: Dual-focusable elements: thetreeitemdiv and the nested checkbox are both in tab order.With
tabIndex={0}on the outerdivand the<input type="checkbox">also natively focusable, keyboard users will encounter two tab stops per tree item. For large trees this significantly increases tab-stop count. ConsidertabIndex={-1}on the checkbox (since the div already handles keyboard selection via Enter/Space) or manage focus with roving tabindex on the tree container.This isn't a blocker — the current behavior is functional — but it's a common accessibility pattern to avoid redundant tab stops in composite widgets like trees.
218-236:getAllPathsuses recursive spread — fine for typical trees.
result.files.push(...subPaths.files)in a recursive function could theoretically hit argument-count limits forFunction.prototype.applyon extremely flat trees with hundreds of thousands of files. In practice this is unlikely for directory trees, but if you ever need to harden this, a simplefor...ofpush loop orArray.prototype.concatassignment would avoid the spread.
|
Validation update on review threads (post-
One stale thread references |
|
1 participant
Summary
AppContextprovider (179 lines)useStatehooks to singleuseReducerArray.includes()toSet.has()for O(1) lookups with batch select-allhiddenclass instead of unmount/remountalert()calls with dismissible inline UI@heroicons/reactdependency removedswitchToTab,refreshDirectoryTree) and 500ms localStorage polling eliminatedChanges
Files Changed (16)
New files:
src/renderer/context/AppContext.tsx— centralised state providersrc/renderer/components/icons/Spinner.tsx— shared spinner componentdocs/plan/ui_fixes.md— three-agent validated implementation planRewritten:
src/renderer/components/App.tsx— thin shell with ErrorBannersrc/renderer/components/ConfigTab.tsx— useReducer, context integrationsrc/renderer/components/FileTree.tsx— Set-based selection, batch select, role=treeitemsrc/renderer/components/SourceTab.tsx— Set props, configContent prop, SpinnerUpdated:
src/main/index.ts— SonarQube fixes (cognitive complexity, Set, codePointAt, type assertion)src/renderer/components/ProcessedTab.tsx— Spinner componentsrc/types/global.d.ts— removed window global declarationstests/unit/components/*.test.tsx— updated for Set-based props, context mocks, ErrorBannerTest plan
Summary by Sourcery
Refactor the renderer UI to centralize application state in a new React context, improve file selection performance and accessibility, and replace blocking alerts with inline error handling while resolving SonarQube findings.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
New Features
Improvements
Chores