Fix the default configs when adding a new provider#1383
Open
Fix the default configs when adding a new provider#1383
Conversation
📝 WalkthroughWalkthroughIntroduces per-provider default accelerator configs and separates supported_accelerators from the raw JSON config. Updates provider creation/edit flows and UI: JSON editor hidden for SLURM and LOCAL, supported accelerators managed separately, and SLURM defaults parsed earlier. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/renderer/components/Team/ProviderDetailsModal.tsx (1)
274-290:parsedConfig: anyviolates the no-anycoding guideline.Define a typed interface instead of relying on
any. Since the config shape varies by provider, a discriminated union or a minimal shared type keeps the compiler useful:♻️ Suggested refactor
+interface ProviderConfig { + supported_accelerators?: string[]; + [key: string]: unknown; // open for provider-specific fields +} + const saveProvider = async () => { setLoading(true); try { - let parsedConfig: any; + let parsedConfig: ProviderConfig; if (type === 'slurm') { parsedConfig = buildSlurmConfig(); } else if (type === 'local') {As per coding guidelines, "Avoid using
anytype in TypeScript; define interfaces for all props and API responses to ensure type safety."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/Team/ProviderDetailsModal.tsx` around lines 274 - 290, The code declares parsedConfig: any which violates the no-any rule; replace it with a typed interface or discriminated union that covers provider-specific shapes (e.g., SlurmConfig for buildSlurmConfig(), LocalConfig for local provider, and GenericProviderConfig for other providers) and use that type for parsedConfig in ProviderDetailsModal; ensure the code that assigns parsedConfig (branches referencing buildSlurmConfig, supportedAccelerators, and the config prop) narrows the union correctly (via a discriminator like type === 'slurm' | 'local' | 'other' or type guards) so the compiler knows when supported_accelerators can be assigned and when JSON.parse(config) returns the expected interface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/Team/ProviderDetailsModal.tsx`:
- Around line 277-282: When handling type === 'local' in the save path,
parsedConfig is reset to {} which drops any existing fields from the current
config state; instead seed parsedConfig from the existing config (merge config
into parsedConfig) and then set/override supported_accelerators only if
supportedAccelerators.length > 0 so extra keys (e.g. base_url or future fields)
are preserved; update the block that builds parsedConfig for local providers
(referencing parsedConfig, supportedAccelerators and the component's config
state populated by the useEffect) to merge rather than replace.
---
Nitpick comments:
In `@src/renderer/components/Team/ProviderDetailsModal.tsx`:
- Around line 274-290: The code declares parsedConfig: any which violates the
no-any rule; replace it with a typed interface or discriminated union that
covers provider-specific shapes (e.g., SlurmConfig for buildSlurmConfig(),
LocalConfig for local provider, and GenericProviderConfig for other providers)
and use that type for parsedConfig in ProviderDetailsModal; ensure the code that
assigns parsedConfig (branches referencing buildSlurmConfig,
supportedAccelerators, and the config prop) narrows the union correctly (via a
discriminator like type === 'slurm' | 'local' | 'other' or type guards) so the
compiler knows when supported_accelerators can be assigned and when
JSON.parse(config) returns the expected interface.
dadmobile
previously approved these changes
Feb 24, 2026
deep1401
dismissed
dadmobile’s stale review
Dismissing so you can check again @dadmobile
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/renderer/components/Team/ProviderDetailsModal.tsx (1)
285-290:⚠️ Potential issue | 🟡 MinorAvoid dropping existing local provider config fields on save.
Editing a local provider with extra config keys will still discard them becauseparsedConfigis reset to{}.🛡️ Proposed fix
- parsedConfig = {}; + parsedConfig = + typeof config === 'string' && config + ? JSON.parse(config) + : config || {}; if (supportedAccelerators.length > 0) { parsedConfig.supported_accelerators = supportedAccelerators; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/Team/ProviderDetailsModal.tsx` around lines 285 - 290, The code in ProviderDetailsModal.tsx resets parsedConfig to {} for type === 'local', dropping any existing local provider fields; instead preserve and merge existing config and only set/overwrite supported_accelerators when supportedAccelerators is non-empty. Update the logic around parsedConfig (the variable assigned inside the type === 'local' branch) to start from the existing parsedConfig/object and merge in supported_accelerators (e.g., copy existing keys and add supported_accelerators) rather than replacing the whole object so extra config keys are retained on save.
🧹 Nitpick comments (1)
src/renderer/components/Team/ProviderDetailsModal.tsx (1)
210-225: Add isolated tests for default accelerator init and SLURM default parsing.
These behaviors are now central to the add‑provider flow and should be covered in component tests.As per coding guidelines, "Write unit tests for all utility functions and complex hooks in the frontend" and "Test frontend components in isolation where possible".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/Team/ProviderDetailsModal.tsx` around lines 210 - 225, Add isolated unit tests for ProviderDetailsModal to cover two behaviors: (1) initialization of supported accelerators by verifying that when DEFAULT_SUPPORTED_ACCELERATORS has an entry for the provider type the component calls setSupportedAccelerators with that value (and falls back to an empty array when absent), and (2) SLURM default parsing by asserting that when type === 'slurm' and defaultConfig is valid JSON the parseSlurmConfig logic is invoked (and that parse errors are ignored). Locate tests around the ProviderDetailsModal component, mock or stub DEFAULT_SUPPORTED_ACCELERATORS, provide controlled defaultConfig strings, and spy on setSupportedAccelerators and parseSlurmConfig to assert expected calls and fallbacks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/renderer/components/Team/ProviderDetailsModal.tsx`:
- Around line 285-290: The code in ProviderDetailsModal.tsx resets parsedConfig
to {} for type === 'local', dropping any existing local provider fields; instead
preserve and merge existing config and only set/overwrite supported_accelerators
when supportedAccelerators is non-empty. Update the logic around parsedConfig
(the variable assigned inside the type === 'local' branch) to start from the
existing parsedConfig/object and merge in supported_accelerators (e.g., copy
existing keys and add supported_accelerators) rather than replacing the whole
object so extra config keys are retained on save.
---
Nitpick comments:
In `@src/renderer/components/Team/ProviderDetailsModal.tsx`:
- Around line 210-225: Add isolated unit tests for ProviderDetailsModal to cover
two behaviors: (1) initialization of supported accelerators by verifying that
when DEFAULT_SUPPORTED_ACCELERATORS has an entry for the provider type the
component calls setSupportedAccelerators with that value (and falls back to an
empty array when absent), and (2) SLURM default parsing by asserting that when
type === 'slurm' and defaultConfig is valid JSON the parseSlurmConfig logic is
invoked (and that parse errors are ignored). Locate tests around the
ProviderDetailsModal component, mock or stub DEFAULT_SUPPORTED_ACCELERATORS,
provide controlled defaultConfig strings, and spy on setSupportedAccelerators
and parseSlurmConfig to assert expected calls and fallbacks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes