feat(experiments): register PARALLEL_TOOL_EXECUTION flag (internal-only)#678
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new ChangesPARALLEL_TOOL_EXECUTION Experiment Flag
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| @@ -13,13 +14,15 @@ type ExperimentKey = Keys<typeof EXPERIMENT_IDS> | |||
|
|
|||
| interface ExperimentConfig { | |||
| enabled: boolean | |||
There was a problem hiding this comment.
[Low] Document the showInSettings contract
The showInSettings !== false filter in ExperimentalSettings.tsx relies on undefined !== false being true, 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.
| RUN_SLASH_COMMAND: { enabled: false }, | ||
| CUSTOM_TOOLS: { enabled: false }, | ||
| PARALLEL_TOOL_EXECUTION: { enabled: false, showInSettings: false }, | ||
| } |
There was a problem hiding this comment.
[Low] Translation key TODO for when visibility is enabled
No translation key (settings:experimental.PARALLEL_TOOL_EXECUTION.name / .description) was added to en/settings.json. This is harmless now because the flag is filtered out before render, so the key is never looked up.
Impact: None today. When "Epic 4" flips showInSettings to true (or removes the filter), the UI will render the raw key string instead of a human-readable label until translations are added. The repo's scripts/find-missing-i18n-key.js scans for referenced keys, and since the key is never referenced while hidden, it won't catch this gap at that transition.
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 showInSettings: true.
| vi.clearAllMocks() | ||
| }) | ||
|
|
||
| it("does not render internal-only experiment flags", () => { |
There was a problem hiding this comment.
[Low] Optional: assert special-cased entries still render
The test asserts PREVENT_FOCUS_DISRUPTION and RUN_SLASH_COMMAND render and PARALLEL_TOOL_EXECUTION does not, but doesn't assert that the special-cased IMAGE_GENERATION and CUSTOM_TOOLS entries still render. Those branches are exercised by the same .filter pipeline, so a regression that accidentally hid them wouldn't be caught here.
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 IMAGE_GENERATION and CUSTOM_TOOLS labels (or their rendered wrappers) are present, so the test guards the full filter behavior rather than a subset.
Related GitHub Issue
Closes: #363
Description
Registers the
PARALLEL_TOOL_EXECUTIONexperiment flag as an internal-only, preloaded flag (defaultfalse,showInSettings: false)."parallelToolExecution"toexperimentIdsandexperimentsSchemainpackages/types/src/experiment.tsPARALLEL_TOOL_EXECUTION: "parallelToolExecution"toEXPERIMENT_IDSand{ enabled: false, showInSettings: false }toexperimentConfigsMapinsrc/shared/experiments.tsshowInSettings?: booleantoExperimentConfiginterface;ExperimentalSettings.tsxfilters out configs whereshowInSettings === false, so the flag is wired up but invisible in the Settings UI until Epic 4 is readyTest Procedure
Both pass with no failures.
Pre-Submission Checklist
Summary by CodeRabbit
New Features
parallelToolExecution) and updated the experiment definitions and validation.Bug Fixes
Tests