refactor: centralized experimental features #13227
Conversation
📝 WalkthroughWalkthroughThis change updates the handling of experimental configuration features in the renderer's preferences UI and related tests. It shifts the logic from treating experimental configurations as booleans to treating them as objects, updating the UI and test logic accordingly. The update and toggle operations now use new APIs ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were identified based on the objectives from the linked issues. Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (10)
🔇 Additional comments (5)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
1a38edb to
32fd0f6
Compare
There was a problem hiding this comment.
Hey @gastoner - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `packages/renderer/src/lib/preferences/PreferencesRenderingItemFormat.svelte:204` </location>
<code_context>
record={record}
checked={typeof givenValue === 'boolean' ? givenValue : !!recordValue}
onChange={onChange} />
+ {:else if record.type === 'object'}
+ <BooleanItem
+ record={record}
+ checked={!!recordValue}
+ onChange={onChange} />
{:else if record.type === 'number' || record.type === 'integer'}
{#if enableSlider && typeof record.maximum === 'number'}
</code_context>
<issue_to_address>
Rendering BooleanItem for 'object' types may be misleading.
If this is an intentional convention (e.g., for feature toggles), please document it clearly in the codebase to avoid confusion.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
{:else if record.type === 'object'}
<BooleanItem
record={record}
checked={!!recordValue}
onChange={onChange} />
=======
{:else if record.type === 'object'}
{/*
NOTE: Rendering BooleanItem for 'object' types is intentional.
This is used as a convention for feature toggles or similar boolean-like objects.
If you change this behavior, ensure you understand all usages of 'object' type in the preferences schema.
*/}
<BooleanItem
record={record}
checked={!!recordValue}
onChange={onChange} />
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| {:else if record.type === 'object'} | ||
| <BooleanItem | ||
| record={record} | ||
| checked={!!recordValue} | ||
| onChange={onChange} /> |
There was a problem hiding this comment.
suggestion: Rendering BooleanItem for 'object' types may be misleading.
If this is an intentional convention (e.g., for feature toggles), please document it clearly in the codebase to avoid confusion.
| {:else if record.type === 'object'} | |
| <BooleanItem | |
| record={record} | |
| checked={!!recordValue} | |
| onChange={onChange} /> | |
| {:else if record.type === 'object'} | |
| {/* | |
| NOTE: Rendering BooleanItem for 'object' types is intentional. | |
| This is used as a convention for feature toggles or similar boolean-like objects. | |
| If you change this behavior, ensure you understand all usages of 'object' type in the preferences schema. | |
| */} | |
| <BooleanItem | |
| record={record} | |
| checked={!!recordValue} | |
| onChange={onChange} /> |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/renderer/src/lib/preferences/PreferencesRenderingItemFormat.svelte (1)
102-118: Consider the implications of the JSON serialization hack.The
JSON.parse(JSON.stringify(recordValue))approach on lines 107-110 works for simple objects but could fail for:
- Objects with circular references
- Objects with functions or undefined values
- Objects with Date, RegExp, or other special types
Since experimental features are represented as empty objects
{}, this should work fine for now. However, consider documenting this limitation or using a more robust deep clone method if the object structure becomes more complex in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/main/src/plugin/configuration-impl.ts(1 hunks)packages/renderer/src/lib/preferences/ExperimentalPage.spec.ts(3 hunks)packages/renderer/src/lib/preferences/ExperimentalPage.svelte(2 hunks)packages/renderer/src/lib/preferences/PreferencesRenderingItemFormat.svelte(6 hunks)
🧠 Learnings (2)
📓 Common learnings
Learnt from: benoitf
PR: podman-desktop/podman-desktop#13043
File: packages/main/src/plugin/image-checker.ts:39-46
Timestamp: 2025-07-02T08:30:22.649Z
Learning: In Podman Desktop, benoitf prefers to add dependency injection annotations (@injectable, @inject) in separate PRs before implementing the actual container bindings and DI activation. This allows for incremental adoption of dependency injection.
packages/renderer/src/lib/preferences/ExperimentalPage.spec.ts (1)
Learnt from: benoitf
PR: #13043
File: packages/main/src/plugin/color-registry-inject.spec.ts:33-35
Timestamp: 2025-07-02T08:26:44.201Z
Learning: In test files for Podman Desktop, benoitf prefers partial mocking over complete mocking. Partial mocks that only implement the methods actually used in the test are sufficient and preferred over creating complete mock objects.
🧬 Code Graph Analysis (1)
packages/renderer/src/lib/preferences/ExperimentalPage.spec.ts (2)
packages/api/src/configuration/models.ts (1)
IConfigurationPropertyRecordedSchema(49-53)packages/api/src/configuration/constants.ts (1)
CONFIGURATION_DEFAULT_SCOPE(19-19)
🪛 GitHub Check: typecheck
packages/renderer/src/lib/preferences/ExperimentalPage.spec.ts
[failure] 31-31:
Property 'isExperimentalConfigurationEnabled' does not exist on type 'Window & typeof globalThis'.
[failure] 103-103:
Property 'isExperimentalConfigurationEnabled' does not exist on type 'Window & typeof globalThis'.
[failure] 93-93:
Property 'updateExperimentalConfigurationValue' does not exist on type 'Window & typeof globalThis'. Did you mean 'updateConfigurationValue'?
[failure] 116-116:
Property 'updateExperimentalConfigurationValue' does not exist on type 'Window & typeof globalThis'. Did you mean 'updateConfigurationValue'?
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: benoitf
PR: podman-desktop/podman-desktop#13043
File: packages/main/src/plugin/image-checker.ts:39-46
Timestamp: 2025-07-02T08:30:22.649Z
Learning: In Podman Desktop, benoitf prefers to add dependency injection annotations (@injectable, @inject) in separate PRs before implementing the actual container bindings and DI activation. This allows for incremental adoption of dependency injection.
packages/renderer/src/lib/preferences/ExperimentalPage.spec.ts (1)
Learnt from: benoitf
PR: #13043
File: packages/main/src/plugin/color-registry-inject.spec.ts:33-35
Timestamp: 2025-07-02T08:26:44.201Z
Learning: In test files for Podman Desktop, benoitf prefers partial mocking over complete mocking. Partial mocks that only implement the methods actually used in the test are sufficient and preferred over creating complete mock objects.
🧬 Code Graph Analysis (1)
packages/renderer/src/lib/preferences/ExperimentalPage.spec.ts (2)
packages/api/src/configuration/models.ts (1)
IConfigurationPropertyRecordedSchema(49-53)packages/api/src/configuration/constants.ts (1)
CONFIGURATION_DEFAULT_SCOPE(19-19)
🪛 GitHub Check: typecheck
packages/renderer/src/lib/preferences/ExperimentalPage.spec.ts
[failure] 31-31:
Property 'isExperimentalConfigurationEnabled' does not exist on type 'Window & typeof globalThis'.
[failure] 103-103:
Property 'isExperimentalConfigurationEnabled' does not exist on type 'Window & typeof globalThis'.
[failure] 93-93:
Property 'updateExperimentalConfigurationValue' does not exist on type 'Window & typeof globalThis'. Did you mean 'updateConfigurationValue'?
[failure] 116-116:
Property 'updateExperimentalConfigurationValue' does not exist on type 'Window & typeof globalThis'. Did you mean 'updateConfigurationValue'?
⏰ 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). (11)
- GitHub Check: smoke e2e tests (production)
- GitHub Check: smoke e2e tests (development)
- GitHub Check: linter, formatters
- GitHub Check: unit tests / windows-2025
- GitHub Check: unit tests / ubuntu-24.04
- GitHub Check: typecheck
- GitHub Check: unit tests / macos-15
- GitHub Check: macOS
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: build website
🔇 Additional comments (7)
packages/main/src/plugin/configuration-impl.ts (1)
70-94: Good catch! The deletion now correctly uses the computed key.The fix properly deletes
localKeyinstead ofsectionfrom the local view. This ensures that configuration keys with global section prefixes are correctly removed when setting values toundefined.packages/renderer/src/lib/preferences/ExperimentalPage.svelte (2)
23-29: LGTM! The update function correctly handles object-typed experimental configurations.The logic properly interprets object values as enabled states, aligning with the new experimental feature representation.
41-59: Good optimization to avoid redundant updates.The changes correctly:
- Check the current state before updating to avoid unnecessary API calls
- Use the new experimental configuration APIs
- Set empty object
{}to enable andundefinedto disable featurespackages/renderer/src/lib/preferences/ExperimentalPage.spec.ts (1)
28-51: Test changes correctly reflect the new experimental configuration handling.The updates properly:
- Mock the new
isExperimentalConfigurationEnabledmethod- Change configuration types from 'boolean' to 'object'
- Remove the default values (as experimental features don't have defaults)
packages/renderer/src/lib/preferences/PreferencesRenderingItemFormat.svelte (3)
86-100: Good handling of object-typed configurations as boolean toggles.The logic correctly converts object values to booleans for UI display while preserving the original value structure.
160-173: Well-structured boolean to object conversion for experimental features.The logic clearly handles the conversion:
true→{}(enable feature)false→undefined(disable feature)This aligns perfectly with the experimental feature representation.
204-208: Consistent UI representation for object-typed configurations.Using BooleanItem for object types provides a consistent toggle interface for experimental features.
| // mock false by default | ||
| vi.mocked(window.getConfigurationValue).mockResolvedValue(false); | ||
| // mock undefined by default (not enabled) | ||
| vi.mocked(window.isExperimentalConfigurationEnabled).mockResolvedValue(false); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address TypeScript errors for the new window methods.
The static analysis shows that isExperimentalConfigurationEnabled and updateExperimentalConfigurationValue are not defined on the Window interface. These methods need to be properly mocked in the test setup.
Also applies to: 93-93, 103-103, 116-116
🏁 Script executed:
#!/bin/bash
# Description: Check if these window methods are defined in the test setup files
# Search for test setup files that might define window mocks
fd -e ts -e js "vitest-setup|test-setup|setup" packages/renderer/src
# Search for existing window method definitions
rg -A 5 "window\.(getConfigurationValue|updateConfigurationValue)" packages/renderer/srcLength of output: 87264
Define and mock the new experimental window methods
TypeScript errors occur because isExperimentalConfigurationEnabled and updateExperimentalConfigurationValue aren’t declared on Window. Please:
- Add a global type declaration (e.g.
packages/renderer/src/global.d.ts):export {}; declare global { interface Window { /** Returns whether a given experimental flag (optional) is enabled */ isExperimentalConfigurationEnabled?(flag?: string): Promise<boolean>; /** Updates the value of an experimental flag */ updateExperimentalConfigurationValue?(flag: string, enabled: boolean): Promise<void>; } }
- In your Vitest setup file (e.g.
packages/renderer/vitest-setup.ts), mock both methods:import { vi } from 'vitest'; Object.defineProperty(window, 'isExperimentalConfigurationEnabled', { value: vi.fn().mockResolvedValue(false) }); Object.defineProperty(window, 'updateExperimentalConfigurationValue', { value: vi.fn().mockResolvedValue(undefined) });
- Remove the inline mocks in
ExperimentalPage.spec.tsnow that the globals exist (lines 31, 93, 103, 116).
This ensures both methods are recognized by TypeScript and properly faked in tests.
🧰 Tools
🪛 GitHub Check: typecheck
[failure] 31-31:
Property 'isExperimentalConfigurationEnabled' does not exist on type 'Window & typeof globalThis'.
🤖 Prompt for AI Agents
In packages/renderer/src/lib/preferences/ExperimentalPage.spec.ts at line 31,
TypeScript errors occur because the window methods
isExperimentalConfigurationEnabled and updateExperimentalConfigurationValue are
not declared on the Window interface. To fix this, add a global type declaration
for these methods in packages/renderer/src/global.d.ts, then mock both methods
in the Vitest setup file (e.g., packages/renderer/vitest-setup.ts) using
Object.defineProperty with vi.fn().mockResolvedValue. Finally, remove the inline
mocks from ExperimentalPage.spec.ts at lines 31, 93, 103, and 116 since the
global mocks now exist.
32fd0f6 to
9ce9f52
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
9ce9f52 to
91542fb
Compare
91542fb to
c060525
Compare
| if (value === undefined) { | ||
| if (localView[localKey]) { | ||
| delete localView[section]; | ||
| delete localView[localKey]; |
There was a problem hiding this comment.
Why this change? I cannot see where the affectation changed from section to localkey
There was a problem hiding this comment.
When seting some configuration to undefined e.g. kubernetes.statesExperimental the localKey === kubernetes.statesExperimentalbut thesection === statesExperimental` due to this this configuration is not found so we are "deleting" not existing config :(
So setting config value with undewined will do nothing.
I've didn't found any other way how to do this, maybe some bug or typo? Other config should work as expected
c060525 to
c717fd5
Compare
| recordValue = value; | ||
| // HACK: when updating experimental features (representated in settings.json by object) | ||
| // disabling this feature will set undefined as a value | ||
| // enabking will set empty object |
There was a problem hiding this comment.
| // enabking will set empty object | |
| // enabling will set empty object |
Signed-off-by: Evzen Gasta <evzen.ml@seznam.cz>
c717fd5 to
4401a69
Compare
What does this PR do?
Changes logic from storing boolean value to object/undefined in order to make work
#13223
#13224
#13225
#13226
Uses new experimental configuration manager #13316
More info in #13095
Screenshot / video of UI
What issues does this PR fix or reference?
Closes #13096
Needs rebase after #13316
THOSE PRs MUST be merged together:
#13225
#13224
#13223
#13226
#13227
How to test this PR?