fix(sonar): clear complexity findings and add locale screenshot drift checks#124
Conversation
Reviewer's GuideRefactors config tab state handling to reduce cognitive complexity, and extends the QA screenshot flow to deterministically capture locale-specific UI screenshots while keeping provider and locale behavior consistent and testable. Sequence diagram for refactored config save flow in ConfigTabsequenceDiagram
actor User
participant ConfigTab
participant ConfigTabHelpers
participant ProviderValidationHelpers
participant onConfigChange
User->>ConfigTab: trigger saveConfig(state)
ConfigTab->>ConfigTabHelpers: parseConfigContent(configContent)
ConfigTabHelpers-->>ConfigTab: ConfigObject config
ConfigTab->>ConfigTabHelpers: applyBaseConfigState(config, state)
ConfigTab->>ConfigTabHelpers: applyProviderConfigState(config, state, aiSurfacesEnabled, t)
ConfigTabHelpers->>ProviderValidationHelpers: getProviderValidationErrors(providerFields, t)
ProviderValidationHelpers-->>ConfigTabHelpers: validationErrors
ConfigTabHelpers-->>ConfigTab: ProviderConfigSaveResult providerResult
alt aiSurfacesEnabled and providerResult.hasValidationErrors
ConfigTab->>ConfigTab: setProviderValidationErrors(validationErrors)
ConfigTab->>ConfigTab: setProviderTestResult(failMessage)
ConfigTab->>onConfigChange: onConfigChange(updatedConfig)
ConfigTab->>ConfigTab: setIsSaved(false)
else no provider validation errors
ConfigTab->>ConfigTab: setProviderValidationErrors([])
ConfigTab->>onConfigChange: onConfigChange(updatedConfig)
ConfigTab->>ConfigTab: setIsSaved(true)
ConfigTab->>ConfigTab: schedule reset of isSaved
end
Sequence diagram for locale-specific QA screenshot capture flowsequenceDiagram
participant NodeScript as capture-ui-screenshot
participant Browser as PlaywrightPage
participant App as RendererApp
participant LanguageSelector as languageSelector
participant LocalStorage
NodeScript->>Browser: setupMockElectronApi(page)
Browser->>LocalStorage: setItem(app.locale, en)
NodeScript->>Browser: captureAppStateScreenshots(page)
Browser->>App: render app with initial locale en
NodeScript->>Browser: capture configDefault screenshot
NodeScript->>Browser: captureLocaleScreenshots(page)
NodeScript->>Browser: waitForSelector(languageSelector)
loop for each locale in SUPPORTED_LOCALES
NodeScript->>Browser: setLocaleAndWait(page, locale)
Browser->>LanguageSelector: selectOption(locale)
Browser->>LocalStorage: setItem(app.locale, locale)
Browser-->>NodeScript: locale selector and storage updated
NodeScript->>Browser: page.screenshot(path for locale)
end
NodeScript->>Browser: setLocaleAndWait(page, DEFAULT_LOCALE)
Browser->>LanguageSelector: selectOption(en)
Browser->>LocalStorage: setItem(app.locale, en)
NodeScript->>Browser: continue remaining screenshots
NodeScript->>NodeScript: log deterministic screenshot paths
Class diagram for refactored ConfigTab helpersclassDiagram
class ConfigFormState {
string fileExtensions
string excludePatterns
boolean useCustomExcludes
boolean useCustomIncludes
boolean useGitignore
boolean enableSecretScanning
boolean excludeSuspiciousFiles
boolean includeTreeView
boolean showTokenCount
string exportFormat
string providerId
string providerModel
string providerApiKey
string providerBaseUrl
}
class ConfigObject {
boolean use_custom_excludes
boolean use_custom_includes
boolean use_gitignore
boolean enable_secret_scanning
boolean exclude_suspicious_files
boolean include_tree_view
boolean show_token_count
string export_format
string[] include_extensions
string[] exclude_patterns
ProviderConfig provider
}
class ProviderConfig {
string id
string model
string api_key
string base_url
}
class ConfigTabHelpers {
+string toPlainTextList(value)
+string[] toTrimmedLines(value)
+ConfigFormState loadFormStateFromConfig(state, config, aiSurfacesEnabled)
+ConfigObject parseConfigContent(configContent)
+void applyBaseConfigState(config, state)
+ProviderConfigSaveResult applyProviderConfigState(config, state, aiSurfacesEnabled, translate)
+ProviderFormFields extractProviderFormFields(config, aiSurfacesEnabled)
}
class ProviderFormFields {
string providerId
string providerModel
string providerApiKey
string providerBaseUrl
}
class ProviderConfigSaveResult {
boolean hasValidationErrors
string[] validationErrors
}
class ProviderValidationHelpers {
+string[] getProviderValidationErrors(providerFields, translate)
+boolean hasProviderInput(providerFields)
+string trimToUndefined(value)
}
ConfigTabHelpers --> ConfigFormState
ConfigTabHelpers --> ConfigObject
ConfigTabHelpers --> ProviderConfig
ConfigTabHelpers --> ProviderFormFields
ConfigTabHelpers --> ProviderConfigSaveResult
ConfigTabHelpers ..> ProviderValidationHelpers
ConfigObject o--> ProviderConfig
Class diagram for updated i18n and feature flag utilitiesclassDiagram
class I18nSettings {
+SupportedLocale getInitialLocale()
+void persistLocale(locale)
+boolean isSupportedLocale(value)
+string normalizeLocale(value)
+Window getBrowserWindow()
}
class FeatureFlags {
+boolean isDevMode()
+boolean isAiSurfacesEnabled()
}
class Window {
LocalStorage localStorage
Navigator navigator
DevUtils devUtils
}
class LocalStorage {
+void setItem(key, value)
+string getItem(key)
}
class Navigator {
string[] languages
string language
}
class DevUtils {
boolean isDev
}
I18nSettings ..> Window
FeatureFlags ..> Window
Window o--> LocalStorage
Window o--> Navigator
Window o--> DevUtils
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughAdds locale-aware UI screenshot capture, refactors ConfigTab to modularize config parsing and provider validation, and replaces direct window usage with safer global/window access patterns in i18n and feature-flags modules. Changes
Sequence Diagram(s)sequenceDiagram
participant Script as Capture Script
participant App as Renderer/UI
participant Storage as LocalStorage
participant FS as Screenshot FS
Script->>App: navigate to app, open language selector
Script->>App: run setLocaleAndWait(locale)
App->>Storage: write selected locale to localStorage
Storage-->>App: confirm persistence
App-->>Script: locale applied (UI updated)
Script->>App: take locale-specific screenshot
App->>FS: save screenshot to `LOCALE_SCREENSHOT_PATHS[locale]`
FS-->>Script: write complete
Script->>App: repeat for next locale, then reset to DEFAULT_LOCALE
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.16.0)scripts/capture-ui-screenshot.js┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m src/renderer/feature-flags.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m �[1m Loading rules from local config...�[0m 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 QodoReduce complexity and add locale screenshot QA validation
WalkthroughsDescription• Refactor ConfigTab reducer to reduce cognitive complexity - Extract provider config logic into separate functions - Extract form state loading into dedicated helper - Simplify config parsing and application logic • Replace typeof checks with globalThis for better SSR compatibility - Update feature-flags and i18n settings modules - Add getBrowserWindow helper for consistent window access • Add deterministic locale screenshot captures for QA - Capture UI screenshots for en, es, fr, de locales - Add language selector automation and locale persistence - Update screenshot validation to include all locale variants Diagramflowchart LR
A["ConfigTab Reducer<br/>High Complexity"] -->|Extract Functions| B["Helper Functions<br/>Reduced Complexity"]
C["typeof window<br/>Checks"] -->|Replace| D["globalThis.window<br/>SSR Safe"]
E["QA Screenshot<br/>Config Only"] -->|Extend| F["QA Screenshot<br/>+ 4 Locales"]
B --> G["Sonar Quality Gate<br/>PASS"]
D --> G
F --> G
File Changes1. src/renderer/feature-flags.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 focuses on improving code quality and enhancing the UI testing infrastructure. It resolves existing code-smell issues by refactoring complex configuration handling logic into more manageable functions. Additionally, it significantly upgrades the QA process by introducing automated, locale-specific UI screenshot captures, ensuring consistent user experience across multiple languages. These changes collectively contribute to a more maintainable codebase and a more reliable testing suite. 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, and left some high level feedback:
- The locale screenshot handling duplicates the supported locale list across
SUPPORTED_LOCALESandLOCALE_SCREENSHOT_KEYS; consider deriving the screenshot key names fromSUPPORTED_LOCALESto avoid drift when adding or removing locales. - Now that
getBrowserWindowcentralizes thewindowaccess pattern ini18n/settings.ts, it may be worth reusing a similar helper (or this one if feasible) in other modules likefeature-flags.tsto keep environment checks consistent and reduce repetition.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The locale screenshot handling duplicates the supported locale list across `SUPPORTED_LOCALES` and `LOCALE_SCREENSHOT_KEYS`; consider deriving the screenshot key names from `SUPPORTED_LOCALES` to avoid drift when adding or removing locales.
- Now that `getBrowserWindow` centralizes the `window` access pattern in `i18n/settings.ts`, it may be worth reusing a similar helper (or this one if feasible) in other modules like `feature-flags.ts` to keep environment checks consistent and reduce repetition.
## Individual Comments
### Comment 1
<location> `scripts/capture-ui-screenshot.js:15-18` </location>
<code_context>
const PORT = Number(process.env.UI_SCREENSHOT_PORT || 4173);
const DEFAULT_SCREENSHOT_NAME = `ui-${process.platform}-${process.arch}.png`;
+const DEFAULT_LOCALE = 'en';
+const SUPPORTED_LOCALES = ['en', 'es', 'fr', 'de'];
const FIXED_MTIME = 1700000000000;
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid duplicating the set of supported locales to keep screenshots in sync with locales
This introduces a second source of truth for locales (`SUPPORTED_LOCALES` vs `LOCALE_SCREENSHOT_KEYS`). If a locale is added to `SUPPORTED_LOCALES` without updating `LOCALE_SCREENSHOT_KEYS`, `captureLocaleScreenshots` will throw when `SCREENSHOTS[screenshotKey]` is undefined. Consider deriving `LOCALE_SCREENSHOT_KEYS` from `SUPPORTED_LOCALES` or adding a validation step to ensure every locale has a corresponding screenshot key.
</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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/components/ConfigTab.tsx (1)
330-371:⚠️ Potential issue | 🟡 MinorSave flow: config is persisted even when provider validation fails.
When
providerResult.hasValidationErrorsistrue,onConfigChange(updatedConfig)still fires (line 354) — saving the base config with the provider section deleted — before the early return on line 358. This is likely intentional (base config changes shouldn't be lost due to provider errors), but it may confuse users who see the "fix before saving" message while their non-provider changes were actually saved. Consider whether the save feedback should differentiate between "provider not saved" and "base config saved."
🧹 Nitpick comments (5)
scripts/capture-ui-screenshot.js (2)
14-15: Locale constants are duplicated fromsrc/renderer/i18n/settings.ts.
DEFAULT_LOCALEandSUPPORTED_LOCALESare defined here and insrc/renderer/i18n/settings.ts. Since this is a CommonJS Node script that can't import the TS source directly, the duplication is understandable. Consider adding a comment referencing the canonical source so future changes stay in sync.📝 Suggested comment
-const DEFAULT_LOCALE = 'en'; -const SUPPORTED_LOCALES = ['en', 'es', 'fr', 'de']; +// Keep in sync with src/renderer/i18n/settings.ts +const DEFAULT_LOCALE = 'en'; +const SUPPORTED_LOCALES = ['en', 'es', 'fr', 'de'];
762-776: Explicit screenshot path list is an improvement over dynamic iteration.Using a hardcoded ordered list ensures deterministic log output and makes the expected set of screenshots visible at a glance. However, if new entries are added to the
SCREENSHOTSobject in the future, this list must be updated separately.Consider deriving this list from
SCREENSHOTSto keep them in sync automatically:📝 Alternative approach
- const screenshotPaths = [ - SCREENSHOTS.configDefault, - SCREENSHOTS.localeEn, - SCREENSHOTS.localeEs, - SCREENSHOTS.localeFr, - SCREENSHOTS.localeDe, - SCREENSHOTS.sourceTab, - SCREENSHOTS.sourceSelected, - SCREENSHOTS.sourceSelectedResized, - SCREENSHOTS.processedTab, - ]; + const screenshotPaths = Object.values(SCREENSHOTS);src/renderer/components/ConfigTab.tsx (2)
317-325: Inconsistent YAML parsing:useEffectbypasses the newparseConfigContenthelper.The
useEffecton mount/config-change usesyaml.parse(configContent)directly with its own try/catch, whilesaveConfig(line 332) uses the newly extractedparseConfigContent. Consider usingparseConfigContenthere as well for consistency and to centralize error handling.♻️ Proposed fix
useEffect(() => { - try { - const config = (yaml.parse(configContent) || {}) as ConfigObject; - dispatch({ type: 'LOAD_FROM_CONFIG', config, aiSurfacesEnabled }); - setProviderValidationErrors([]); - setProviderTestResult(null); - } catch (error) { - console.error('Error parsing config:', error); - } + const config = parseConfigContent(configContent); + dispatch({ type: 'LOAD_FROM_CONFIG', config, aiSurfacesEnabled }); + setProviderValidationErrors([]); + setProviderTestResult(null); }, [aiSurfacesEnabled, configContent]);
240-251:applyBaseConfigStatemutatesconfigin place — consider documenting intent.The
voidreturn type hints at mutation, but in a React codebase where immutability is the norm, a brief JSDoc comment (e.g.,/** Mutates config in place */) or renaming toapplyBaseConfigStateTo(config, ...)would make the side-effect explicit to future readers.src/renderer/i18n/index.ts (1)
33-35: Remove unnecessary alias for simpler code.The pattern
const i18nInstance = i18nfollowed byexport default i18nInstanceis functionally identical toexport default i18nand serves no purpose. No ESLint rule in the project configuration (import plugin, sonarjs, or any other active plugin) requires this indirect export pattern. Simplify by directly exporting the imported binding.
Code Review by Qodo
1. parseConfigContent logs raw error
|
![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 is a solid step forward in improving the application's robustness and test coverage. The refactoring in ConfigTab.tsx significantly reduces cognitive complexity by breaking down large functions into smaller, more manageable pieces, which is a great improvement for maintainability. The addition of locale-based screenshot testing is also a valuable enhancement for quality assurance across different languages. My review includes a few suggestions to further improve maintainability by reducing code duplication in the test script. Overall, these are excellent changes.
| const LOCALE_SCREENSHOT_KEYS = { | ||
| en: 'localeEn', | ||
| es: 'localeEs', | ||
| fr: 'localeFr', | ||
| de: 'localeDe', | ||
| }; |
There was a problem hiding this comment.
Similar to the SCREENSHOTS object, this mapping can be generated dynamically from SUPPORTED_LOCALES to avoid repetition and improve maintainability. This change, combined with the suggested change for SCREENSHOTS, would allow you to simply update the SUPPORTED_LOCALES array to add new languages to the screenshot test suite.
const LOCALE_SCREENSHOT_KEYS = SUPPORTED_LOCALES.reduce((acc, locale) => {
acc[locale] = `locale${locale.charAt(0).toUpperCase() + locale.slice(1)}`;
return acc;
}, {});| const screenshotPaths = [ | ||
| SCREENSHOTS.configDefault, | ||
| SCREENSHOTS.localeEn, | ||
| SCREENSHOTS.localeEs, | ||
| SCREENSHOTS.localeFr, | ||
| SCREENSHOTS.localeDe, | ||
| SCREENSHOTS.sourceTab, | ||
| SCREENSHOTS.sourceSelected, | ||
| SCREENSHOTS.sourceSelectedResized, | ||
| SCREENSHOTS.processedTab, | ||
| ]; |
There was a problem hiding this comment.
Instead of manually listing all screenshot paths, you can use Object.values(SCREENSHOTS) to get all paths from the SCREENSHOTS object. This simplifies the code and ensures that any new screenshots added to the SCREENSHOTS object are automatically included in the log output.
const screenshotPaths = Object.values(SCREENSHOTS);| if (globalThis.window === undefined) { | ||
| return false; | ||
| } | ||
|
|
||
| return window.devUtils?.isDev === true; | ||
| return globalThis.window.devUtils?.isDev === true; |
There was a problem hiding this comment.
Using globalThis.window is a good step towards universal code. However, you can simplify this by using optional chaining (?.), which makes the code more concise and readable.
return globalThis.window?.devUtils?.isDev === true;| const i18nInstance = i18n; | ||
|
|
||
| export default i18nInstance; |
There was a problem hiding this comment.
While creating an intermediate constant i18nInstance works, it's unnecessary. You can directly export the i18n object as the default export, which is a more common and direct pattern.
| const i18nInstance = i18n; | |
| export default i18nInstance; | |
| export default i18n; |
|
1 participant
Summary
src/renderer/components/ConfigTab.tsxen,es,fr,dein the QA screenshot flowSonar
make sonar) completed successfullyalert_statusisOK0(code smells0, bugs0, vulnerabilities0)Validation
npm run lintnpm test -- --runInBandnpm run qa:screenshotnpm run e2e:playwrightmake sonarSummary by Sourcery
Refactor config tab state handling and locale initialization for better robustness while extending QA UI screenshot coverage across locales.
Enhancements:
Tests:
Summary by CodeRabbit
Chores
Documentation