refactor(renderer): split AppContext and ConfigTab concerns#131
Conversation
Reviewer's GuideRefactors renderer config and context logic by extracting provider/config form handling and tree/path/error helpers into dedicated utility modules, while updating AppContext and ConfigTab to consume these new abstractions without changing UI behavior. Class diagram for extracted config-tab form and provider utilitiesclassDiagram
direction LR
class ConfigTab {
+configContent: string
+onConfigChange(config: string): void
-formState: ConfigFormState
-dispatch(action: ConfigFormAction): void
-handleSave(): void
-handleTestConnection(): void
}
class ConfigFormModule {
<<module>>
+ConfigFormState
+ConfigFormAction
+initialFormState: ConfigFormState
+configFormReducer(state: ConfigFormState, action: ConfigFormAction) ConfigFormState
+parseConfigContent(configContent: string) ConfigObject
+applyBaseConfigState(config: ConfigObject, state: ConfigFormState) void
+applyProviderConfigState(config: ConfigObject, state: ConfigFormState, aiSurfacesEnabled: boolean, translate: function) ProviderConfigSaveResult
}
class ProviderUtilsModule {
<<module>>
+ProviderOption
+ProviderFields
+PROVIDER_OPTIONS: ProviderOption[]
+isSupportedProviderId(value: unknown) boolean
+trimToUndefined(value: string) string
+hasProviderInput(providerFields: ProviderFields) boolean
+getProviderValidationErrors(providerFields: ProviderFields, translate: function) string[]
}
class ConfigFormState {
+useCustomExcludes: boolean
+useCustomIncludes: boolean
+useGitignore: boolean
+enableSecretScanning: boolean
+excludeSuspiciousFiles: boolean
+includeTreeView: boolean
+showTokenCount: boolean
+exportFormat: ExportFormat
+fileExtensions: string
+excludePatterns: string
+providerId: ProviderId | ""
+providerModel: string
+providerApiKey: string
+providerBaseUrl: string
}
class ProviderConfigSaveResult {
+hasValidationErrors: boolean
+validationErrors: string[]
}
class ConfigObject {
}
class ProviderFields {
+providerId: ProviderId | ""
+providerModel: string
+providerApiKey: string
+providerBaseUrl: string
}
class ProviderOption {
+id: ProviderId
+label: string
+defaultBaseUrl: string
+requiresApiKey: boolean
}
class ExportFormat {
}
class ProviderId {
}
%% Relationships
ConfigTab --> ConfigFormModule : uses
ConfigTab --> ProviderUtilsModule : uses
ConfigFormModule --> ConfigFormState : defines
ConfigFormModule --> ProviderConfigSaveResult : returns
ConfigFormModule --> ConfigObject : parses
ConfigFormModule --> ProviderUtilsModule : uses
ConfigFormModule --> ExportFormat : uses
ProviderUtilsModule --> ProviderFields : uses
ProviderUtilsModule --> ProviderOption : defines
ProviderUtilsModule --> ProviderId : uses
Class diagram for extracted AppContext utility modulesclassDiagram
direction LR
class AppContextModule {
<<context>>
+AppProvider
+useApp()
-rootPath: string
-directoryTree: DirectoryTreeItem[]
-selectedFolders: string[]
-selectedFiles: string[]
-toggleFolderSelection(folderPath: string, isSelected: boolean) void
}
class TreeSelectionModule {
<<module>>
+findFolderByPath(items: DirectoryTreeItem[], targetPath: string) DirectoryTreeItem | null
+collectSubFoldersWithinBoundary(folder: DirectoryTreeItem, rootPath: string) string[]
+collectFilesWithinBoundary(folder: DirectoryTreeItem, rootPath: string) string[]
}
class PathBoundaryModule {
<<module>>
+normalizePathForBoundaryCheck(inputPath: string) string
+isPathWithinRootBoundary(candidatePath: string, rootPath: string) boolean
}
class ConfigStorageModule {
<<module>>
+INITIAL_CONFIG_PLACEHOLDER: string
+sanitizeConfigForStorage(configContent: string) string
}
class ErrorUtilsModule {
<<module>>
+ensureError(error: unknown) Error
}
class DirectoryTreeItem {
+path: string
+type: string
+children: DirectoryTreeItem[]
}
class ConfigObject {
+provider
}
AppContextModule --> TreeSelectionModule : uses
AppContextModule --> PathBoundaryModule : uses
AppContextModule --> ConfigStorageModule : uses
AppContextModule --> ErrorUtilsModule : uses
AppContextModule --> DirectoryTreeItem : manages
TreeSelectionModule --> DirectoryTreeItem : traverses
TreeSelectionModule --> PathBoundaryModule : uses
PathBoundaryModule <.. TreeSelectionModule : boundary checks
ConfigStorageModule --> ConfigObject : parses
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. 📝 WalkthroughWalkthroughThis PR refactors configuration management by extracting duplicated logic from ConfigTab.tsx and AppContext.tsx into modular utilities—including config form state management, provider validation, path boundary checks, and tree traversal—while maintaining existing behavior and public APIs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 renderer concerns into dedicated utility modules
WalkthroughsDescription• Extract form parsing and validation logic into dedicated modules • Extract renderer context utilities into separate files • Reduce mixed concerns in AppContext and ConfigTab components • Maintain existing UI behavior and feature flags unchanged Diagramflowchart LR
ConfigTab["ConfigTab.tsx"]
ConfigForm["config-form.ts"]
ProviderUtils["provider-utils.ts"]
AppContext["AppContext.tsx"]
ConfigStorage["config-storage.ts"]
ErrorUtils["error-utils.ts"]
PathBoundary["path-boundary.ts"]
TreeSelection["tree-selection.ts"]
ConfigTab -- "imports form logic" --> ConfigForm
ConfigTab -- "imports provider utilities" --> ProviderUtils
ConfigForm -- "uses provider validation" --> ProviderUtils
AppContext -- "imports config storage" --> ConfigStorage
AppContext -- "imports error handling" --> ErrorUtils
AppContext -- "imports path utilities" --> PathBoundary
AppContext -- "imports tree utilities" --> TreeSelection
TreeSelection -- "uses path boundary" --> PathBoundary
File Changes1. src/renderer/components/config-tab/config-form.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 significantly improves the modularity and maintainability of the renderer by systematically separating concerns within both the 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 found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/renderer/components/config-tab/config-form.ts:32-37` </location>
<code_context>
+ providerBaseUrl: string;
+};
+
+export type ConfigFormAction =
+ | {
+ type: 'SET_FIELD';
+ field: keyof ConfigFormState;
+ value: ConfigFormState[keyof ConfigFormState];
+ }
+ | { type: 'LOAD_FROM_CONFIG'; config: ConfigObject; aiSurfacesEnabled: boolean };
+
</code_context>
<issue_to_address>
**suggestion:** Tighten ConfigFormAction typing so `field` and `value` stay in sync
With `value` typed as `ConfigFormState[keyof ConfigFormState]`, TypeScript allows mismatches (e.g. dispatching a string to a boolean field). You can make this type‑safe by tying `field` and `value` together via a generic discriminated union:
```ts
export type ConfigFormAction =
| { type: 'LOAD_FROM_CONFIG'; config: ConfigObject; aiSurfacesEnabled: boolean }
| ({ type: 'SET_FIELD' } & {
[K in keyof ConfigFormState]: { field: K; value: ConfigFormState[K] }
}[keyof ConfigFormState]);
```
This ensures `value`’s type always matches the selected `field`, preventing incorrect dispatches at compile time.
</issue_to_address>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 FilesNone |
Code Review by Qodo
✅ 1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| export const parseConfigContent = (configContent: string): ConfigObject => { | ||
| try { | ||
| const parsedConfig = yaml.parse(configContent) as ConfigObject; | ||
| if (!parsedConfig || typeof parsedConfig !== 'object') { | ||
| return {}; | ||
| } | ||
| return parsedConfig; | ||
| } catch (error) { | ||
| console.error('Error parsing config content, using empty config:', error); | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
1. parseconfigcontent logs raw error 📘 Rule violation ⛨ Security
parseConfigContent() writes the caught YAML parsing error directly to console.error, which is unstructured and may include sensitive config details (e.g., API keys) in logs. This can leak secrets via logs and makes auditing/monitoring harder.
Agent Prompt
## Issue description
`parseConfigContent()` logs the raw caught YAML parsing error via `console.error(...)`. YAML parse errors can include contextual information that may reveal sensitive config values, and `console.*` output is unstructured.
## Issue Context
Config content may contain `provider.api_key` and other secrets. Logging should be structured/redacted and must not emit secrets.
## Fix Focus Areas
- src/renderer/components/config-tab/config-form.ts[123-133]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| export const sanitizeConfigForStorage = (configContent: string): string => { | ||
| try { | ||
| const parsedConfig = yaml.parse(configContent); | ||
| if (!parsedConfig || typeof parsedConfig !== 'object') { | ||
| return configContent; | ||
| } | ||
|
|
||
| const config = parsedConfig as ConfigObject; | ||
| if (!config.provider || typeof config.provider !== 'object' || !config.provider.api_key) { | ||
| return configContent; | ||
| } | ||
|
|
||
| const sanitizedProvider = { ...config.provider }; | ||
| delete sanitizedProvider.api_key; | ||
|
|
||
| const sanitizedConfig: ConfigObject = { ...config }; | ||
| const providerValues = Object.values(sanitizedProvider).filter((value) => value !== undefined); | ||
| if (providerValues.length === 0) { | ||
| delete sanitizedConfig.provider; | ||
| } else { | ||
| sanitizedConfig.provider = sanitizedProvider; | ||
| } | ||
|
|
||
| return yaml.stringify(sanitizedConfig); | ||
| } catch { | ||
| return configContent; | ||
| } |
There was a problem hiding this comment.
2. sanitizeconfigforstorage leaks api_key 📘 Rule violation ⛨ Security
sanitizeConfigForStorage() returns the original configContent on parse failures, which can persist secrets like provider.api_key to storage unredacted. This is insecure handling of sensitive data in an error/edge-case path.
Agent Prompt
## Issue description
`sanitizeConfigForStorage()` can return unsanitized `configContent` (including `provider.api_key`) when YAML parsing fails, causing secrets to be persisted/exposed in an edge-case path.
## Issue Context
This function is intended to sanitize secrets before storage; the error path currently bypasses redaction.
## Fix Focus Areas
- src/renderer/context/utils/config-storage.ts[7-33]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // Load form state from config prop | ||
| useEffect(() => { | ||
| try { | ||
| const config = (yaml.parse(configContent) || {}) as ConfigObject; | ||
| const config = parseConfigContent(configContent); | ||
| dispatch({ type: 'LOAD_FROM_CONFIG', config, aiSurfacesEnabled }); | ||
| setProviderValidationErrors([]); | ||
| setProviderTestResult(null); |
There was a problem hiding this comment.
3. Parse errors reset form 🐞 Bug ⛯ Reliability
ConfigTab’s load effect now uses parseConfigContent(), which catches YAML parse errors and returns
{}. This means invalid YAML no longer triggers the surrounding try/catch path and instead reloads
the form with defaults, clearing provider validation/test feedback and potentially confusing users
while editing broken YAML.
Agent Prompt
## Issue description
ConfigTab reloads form state from YAML using `parseConfigContent()`. Because `parseConfigContent()` swallows YAML parse errors and returns `{}`, the ConfigTab effect will dispatch `LOAD_FROM_CONFIG` even when YAML is invalid, resetting the form to defaults and clearing provider feedback.
## Issue Context
We want to preserve the previous behavior of *not* reloading the form on parse failure (or at least make the failure user-visible), rather than silently treating invalid YAML as an empty config.
## Fix Focus Areas
- src/renderer/components/config-tab/config-form.ts[123-134]
- src/renderer/components/ConfigTab.tsx[46-56]
## Suggested approach
1. Update `parseConfigContent` to either:
- return `null` (or `{ config: null, error }`) when parsing fails, OR
- rethrow after logging (and let callers handle).
2. In `ConfigTab`’s load effect, only dispatch `LOAD_FROM_CONFIG` when parsing succeeds; otherwise keep the existing form state and optionally set a UI-visible error state (instead of clearing provider validation/test result).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![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 pull request successfully refactors the codebase by splitting the concerns of AppContext and ConfigTab into dedicated utility modules, significantly improving code organization, maintainability, and testability. However, critical security vulnerabilities have been identified, including insecure deserialization of YAML from untrusted sources, which could lead to prototype pollution, and a high-severity Server-Side Request Forgery (SSRF) vulnerability due to insufficient validation of user-provided URLs for provider configuration. Additionally, there are suggestions for further improvement regarding performance optimization in the file collection logic and enhancing function purity.
| if (providerBaseUrl.trim()) { | ||
| try { | ||
| const parsedUrl = new URL(providerBaseUrl.trim()); | ||
| if (!['http:', 'https:'].includes(parsedUrl.protocol)) { | ||
| errors.push(translate('config.validation.baseUrlProtocol')); | ||
| } | ||
| } catch { | ||
| errors.push(translate('config.validation.baseUrlValid')); | ||
| } | ||
| } |
There was a problem hiding this comment.
The validation for the provider's baseUrl is insufficient. It only checks if the protocol is http: or https:, but it does not prevent requests to internal network addresses (e.g., localhost, 192.168.x.x, 10.x.x.x) or cloud metadata endpoints (e.g., 169.254.169.254). An attacker could convince a user to enter a malicious URL, which would cause the application's backend (Electron main process) to make a request to an arbitrary address on the user's local network. This can be used to scan the internal network, access non-public services, or exploit other vulnerabilities.
| default: | ||
| return state; | ||
| } | ||
| }; |
There was a problem hiding this comment.
The application uses yaml.parse() to deserialize YAML content that can be controlled by a user or loaded from localStorage. Untrusted data from localStorage should be treated as malicious. Parsing untrusted YAML content without protection against prototype pollution can allow an attacker to modify the prototype of core JavaScript objects. This can lead to application-wide security vulnerabilities, such as Denial of Service (DoS), or can be chained to achieve Cross-Site Scripting (XSS) or Remote Code Execution (RCE). An attacker would need to find a way to inject malicious YAML into localStorage (e.g., via an XSS vulnerability) to exploit this.
| }; | |
| const parsedConfig = yaml.parse(configContent, { schema: 'core' }) as ConfigObject; |
|
|
||
| export const sanitizeConfigForStorage = (configContent: string): string => { | ||
| try { | ||
| const parsedConfig = yaml.parse(configContent); |
There was a problem hiding this comment.
The application uses yaml.parse() to deserialize YAML content in the sanitizeConfigForStorage function. This content originates from localStorage or user input and is therefore untrusted. Parsing this untrusted YAML without protection against prototype pollution can allow an attacker to modify the prototype of core JavaScript objects, leading to potential application-wide vulnerabilities like DoS, XSS, or RCE.
| const parsedConfig = yaml.parse(configContent); | |
| const parsedConfig = yaml.parse(configContent, { schema: 'core' }); |
| hasValidationErrors: boolean; | ||
| validationErrors: string[]; | ||
| }; | ||
|
|
||
| export const applyProviderConfigState = ( | ||
| config: ConfigObject, | ||
| state: ConfigFormState, | ||
| aiSurfacesEnabled: boolean, | ||
| translate: (key: string) => string | ||
| ): ProviderConfigSaveResult => { | ||
| if (!aiSurfacesEnabled) { | ||
| return { hasValidationErrors: false, validationErrors: [] }; | ||
| } | ||
|
|
||
| const providerFields = { | ||
| providerId: state.providerId, | ||
| providerModel: state.providerModel, | ||
| providerApiKey: state.providerApiKey, | ||
| providerBaseUrl: state.providerBaseUrl, | ||
| }; | ||
| const validationErrors = getProviderValidationErrors(providerFields, translate); | ||
| const hasValidationErrors = validationErrors.length > 0; | ||
|
|
||
| if (hasValidationErrors) { | ||
| if (config.provider) { | ||
| delete config.provider; | ||
| } | ||
| return { hasValidationErrors, validationErrors }; | ||
| } | ||
|
|
||
| if (hasProviderInput(providerFields) && state.providerId) { | ||
| config.provider = { | ||
| id: state.providerId, | ||
| model: state.providerModel.trim(), | ||
| api_key: trimToUndefined(state.providerApiKey), | ||
| base_url: trimToUndefined(state.providerBaseUrl), | ||
| }; | ||
| return { hasValidationErrors, validationErrors }; | ||
| } | ||
|
|
||
| if (config.provider) { | ||
| delete config.provider; |
There was a problem hiding this comment.
The function applyProviderConfigState currently mutates the config object passed to it, which can lead to unexpected side effects. It would be more robust and easier to reason about if it were a pure function.
Consider refactoring it to return a new config object along with the validation results, instead of modifying it in place. This would align better with functional programming principles and improve predictability. A similar change could be applied to applyBaseConfigState for consistency.
| files.push(item.path); | ||
| } | ||
| } else if (item.type === 'directory') { | ||
| files = [...files, ...collectFilesWithinBoundary(item, rootPath)]; |
There was a problem hiding this comment.
Using spread syntax [...array1, ...array2] to concatenate arrays inside a loop can be inefficient for large directory trees, as it creates a new array in each iteration. A more performant approach is to mutate the existing array using push().
| files = [...files, ...collectFilesWithinBoundary(item, rootPath)]; | |
| files.push(...collectFilesWithinBoundary(item, rootPath)); |
|
Mehdi-Bl
deleted the
feat/renderer-concern-split-appcontext-configtab
branch
1 participant
Summary
AppContexthelper logic into dedicated renderer context utility modulesConfigTabprovider/form parsing and validation logic into dedicated helper modulesRefactor details
src/renderer/context/utils/{error-utils,config-storage,path-boundary,tree-selection}.tssrc/renderer/components/config-tab/{config-form,provider-utils}.tssrc/renderer/context/AppContext.tsxto consume extracted utilitiessrc/renderer/components/ConfigTab.tsxto consume extracted form/provider modulesValidation
npm run lintnpm test -- --runInBandnpm run qa:screenshotmake sonarNotes
cl -p ...) butclis not installed in this environmentSummary by CodeRabbit
Bug Fixes
Tests