refactor: simplify sandbox resource-settings helpers (#693 follow-up)#720
Conversation
Follow-up cleanup to #693. Two behavior-preserving changes: - web: the cpu/memory override logic duplicated the inheritance-guard with a heavier, parallel mechanism (four helpers incl. a 7-arg function) than the adjacent child-session fields. Collapsed to two small helpers; the inherit/override/null tri-state is unchanged. All 25 existing sandbox-settings tests pass as-is. - control-plane: added a regression test pinning the getResolvedConfig merge-time normalizeSandboxSettings pass -- the only enforcement of maxConcurrent <= maxTotal when global and repo settings are each individually valid but the merged result is not. No behavior change. Modal mapping and all normalization passes untouched.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a regression test for sandbox config normalization and refactors resource (CPU cores, memory) override handling in the settings editor with new semantic helpers distinguishing inheritance ( ChangesConfig Normalization Test
Resource Override/Inheritance Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
![open-inspect[bot]](https://avatars.githubusercontent.com/u/2492022?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Reviewed PR #720, refactor: simplify sandbox resource-settings helpers (#693 follow-up) by ColeMurray. The PR changes 2 files with 45 additions and 92 deletions, simplifying the web sandbox resource helper flow and adding a control-plane regression test for merge-time sandbox normalization.
Critical Issues
None found.
Suggestions
None.
Nitpicks
None.
Positive Feedback
The resource helper refactor keeps the repo/global tri-state behavior compact while preserving the important distinction between inherited values, explicit numeric overrides, and explicit null provider-default overrides.
The new control-plane regression test targets a subtle merge-only invariant and clearly documents why the resolve-time normalization pass is necessary.
Targeted verification passed: web sandbox settings tests passed (25/25) and control-plane integration settings tests passed (87/87).
Questions
None.
Verdict
Approve: ready to merge.
There was a problem hiding this comment.
[deep review] No blocking maintainability findings. This follow-up deletes a heavier resource-settings helper structure without adding new branching, keeps the tri-state repo/global behavior covered by existing tests, and the new control-plane regression test is placed at the merge boundary it is pinning. Focused tests passed locally: npm test -w @open-inspect/web -- sandbox-settings.test.tsx and npm test -w @open-inspect/control-plane -- integration-settings.test.ts sandbox/settings.test.ts.
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
1 participant
What
Follow-up cleanup to #693 (configurable Modal sandbox CPU/memory). Two behavior-preserving changes:
sandbox-settings.tsx): the cpu/memory override logic was implemented with a heavier, parallel mechanism than the adjacent child-session-limit fields — four helpers including a 7-argumentapplyResourcePayloadand a 3-field intermediate object. Collapsed to two small helpers (resourceDisplayValue,resourcePayloadValue); the inherit / explicit-override / explicit-null(provider-default) tri-state is unchanged.integration-settings.test.ts): added a regression test pinning thegetResolvedConfigmerge-timenormalizeSandboxSettingspass.Why
The resource fields and the child-session-limit fields solve the same inheritance-guard problem; #693 introduced a second, heavier pattern for it. This narrows them and removes the bespoke indirection (net −47 lines).
The regression test closes a gap: the merge-time normalize in
getResolvedConfigis the only enforcement ofmaxConcurrent <= maxTotalwhen the global defaults and the repo override are each individually valid but the merged result is not. It had no committed test, so a future refactor could silently drop it. The test is mutation-verified (it fails if that normalize pass is removed).Behavior
No behavior change. The
prior !== undefinedreasoning relies on stored JSON values never beingundefined, so an inherited-only field is skipped and an explicitnulloverride is preserved — exactly as before. Modal's_resource_kwargsmapping and every normalization pass are untouched.Testing
sandbox-settingstests pass unchangedintegration-settingsunit suite passes, +1 new regression test (mutation-verified)Summary by CodeRabbit
Tests
Refactor