-
Notifications
You must be signed in to change notification settings - Fork 149
feat(experiments): register PARALLEL_TOOL_EXECUTION flag (internal-only) #678
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ export const EXPERIMENT_IDS = { | |
| IMAGE_GENERATION: "imageGeneration", | ||
| RUN_SLASH_COMMAND: "runSlashCommand", | ||
| CUSTOM_TOOLS: "customTools", | ||
| PARALLEL_TOOL_EXECUTION: "parallelToolExecution", | ||
| } as const satisfies Record<string, ExperimentId> | ||
|
|
||
| type _AssertExperimentIds = AssertEqual<Equals<ExperimentId, Values<typeof EXPERIMENT_IDS>>> | ||
|
|
@@ -13,13 +14,17 @@ type ExperimentKey = Keys<typeof EXPERIMENT_IDS> | |
|
|
||
| interface ExperimentConfig { | ||
| enabled: boolean | ||
| /** Defaults to true; set to false to hide from the Settings panel. */ | ||
| showInSettings?: boolean | ||
| } | ||
|
|
||
| export const experimentConfigsMap: Record<ExperimentKey, ExperimentConfig> = { | ||
| PREVENT_FOCUS_DISRUPTION: { enabled: false }, | ||
| IMAGE_GENERATION: { enabled: false }, | ||
| RUN_SLASH_COMMAND: { enabled: false }, | ||
| CUSTOM_TOOLS: { enabled: false }, | ||
| // TODO: add i18n keys (settings:experimental.PARALLEL_TOOL_EXECUTION.name/.description) in the same PR that sets showInSettings: true | ||
| PARALLEL_TOOL_EXECUTION: { enabled: false, showInSettings: false }, | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Low] Translation key TODO for when visibility is enabled No translation key ( Impact: None today. When "Epic 4" flips Suggestion: Leave a TODO anchored to this flag (or note in issue #363) that translation entries must be added in the same PR that enables |
||
|
|
||
| export const experimentDefault = Object.fromEntries( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| import { render, screen } from "@testing-library/react" | ||
|
|
||
| import { experimentDefault } from "@roo/experiments" | ||
|
|
||
| import { ExperimentalSettings } from "../ExperimentalSettings" | ||
|
|
||
| vi.mock("@src/i18n/TranslationContext", () => ({ | ||
| useAppTranslation: () => ({ | ||
| t: (key: string) => key, | ||
| }), | ||
| })) | ||
|
|
||
| describe("ExperimentalSettings", () => { | ||
| const defaultProps = { | ||
| experiments: experimentDefault, | ||
| setExperimentEnabled: vi.fn(), | ||
| setImageGenerationProvider: vi.fn(), | ||
| setOpenRouterImageApiKey: vi.fn(), | ||
| setImageGenerationSelectedModel: vi.fn(), | ||
| } | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| }) | ||
|
|
||
| it("does not render internal-only experiment flags", () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Low] Optional: assert special-cased entries still render The test asserts Impact: Low — test coverage gap. The test's stated intent ("does not render internal-only experiment flags") implicitly promises that non-internal flags do render, but only the generic-path flags are checked. Suggestion: Optionally add assertions that the |
||
| render(<ExperimentalSettings {...defaultProps} />) | ||
|
|
||
| expect(screen.getByText("settings:experimental.PREVENT_FOCUS_DISRUPTION.name")).toBeInTheDocument() | ||
| expect(screen.getByText("settings:experimental.RUN_SLASH_COMMAND.name")).toBeInTheDocument() | ||
| expect(screen.getByText("settings:experimental.IMAGE_GENERATION.name")).toBeInTheDocument() | ||
| expect(screen.getByText("settings:experimental.CUSTOM_TOOLS.name")).toBeInTheDocument() | ||
| expect(screen.queryByText("settings:experimental.PARALLEL_TOOL_EXECUTION.name")).not.toBeInTheDocument() | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Low] Document the
showInSettingscontractThe
showInSettings !== falsefilter inExperimentalSettings.tsxrelies onundefined !== falsebeingtrue, so all pre-existing configs (which omit the field) remain visible. This is correct and backward-compatible, but the implicit "undefined = visible" contract isn't documented here.Impact: Maintainability — a future contributor could misread the optional field and assume omission hides the flag.
Suggestion: Add a one-line JSDoc, e.g.
/** Defaults to true; set to false to hide from the Settings panel. */, so the convention is self-documenting.