Repository navigation
Conversation
|
@viqtor is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
To use Codex here, create a Codex account and connect to github. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Focus Resize feature: persisted settings and Settings UI, managed-settings parser entries, Workspace animation and drag-override logic to interpolate divider positions toward per-pane target ratios, unit and end-to-end tests, and an Xcode project entry for the new test file. Changes
Sequence DiagramsequenceDiagram
autonumber
actor User
participant SettingsView as "SettingsView"
participant UserDefaults as "UserDefaults"
participant Workspace as "Workspace"
participant Timer as "Timer"
participant Dividers as "Dividers"
User->>SettingsView: Toggle focusResize / adjust ratio
SettingsView->>UserDefaults: Write enabled & ratio
UserDefaults->>Workspace: didChangeNotification (debounced)
Workspace->>Workspace: Recompute targets for current focused pane
User->>Workspace: Focus changes to Pane X
Workspace->>Workspace: Compute ancestor split targets
Workspace->>Timer: Start animation timer
loop Animation frames
Timer->>Workspace: Tick
Workspace->>Dividers: Apply interpolated divider positions toward targets
Dividers->>Workspace: Report geometry change
Workspace->>Workspace: Detect user-driven deviations -> record overrides
end
Workspace->>Timer: Cancel timer when complete
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly Related PRs
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)
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 |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/cmuxApp.swift`:
- Around line 4811-4815: Replace the hard-coded percent subtitle in the
SettingsCardRow so it's localized and locale-aware: instead of
"\(Int(focusResizeRatio * 100))%" (in the SettingsCardRow call referencing
focusResizeRatio), build the subtitle with String(localized:
"settings.app.focusResizeRatio.percent", defaultValue: "%d%%",
Int(focusResizeRatio * 100)) or use a NumberFormatter/Percent style formatted
value wrapped by String(localized: ...) to ensure the percent is localized and
uses the correct locale-aware formatting; update the localization key
accordingly.
In `@Sources/Workspace.swift`:
- Around line 12289-12309: The settings handler currently reacts to every
UserDefaults change; update installFocusResizeSettingsObserver /
focusResizeSettingsDidChange to track the last seen FocusResizeSettings state
(e.g., cached properties like focusResizeLastEnabled and focusResizeLastRatio),
read the current (enabled, ratio) from FocusResizeSettings, and only
schedule/trigger focusResizeApplyCurrentSettings when those values actually
change; also if enabled transitions to false cancel
focusResizeRatioDebounceWorkItem immediately and do not proceed (clear
focusResizeLastPaneId if needed) so unrelated defaults writes no longer replay
the animation and disabling stops any pending work.
- Around line 11928-11943: The code applies FocusResizeSettings.ratio() to every
ancestor split causing compounded sizing; fix by computing a per-ancestor factor
so the product of ancestor factors equals the desired final ratio: count the
ancestors returned by focusResizeFindAncestorSplits(ofPane:in:) (excluding any
in focusResizeDragOverrideSplits), compute f = pow(ratio,
1.0/CGFloat(ancestorCount)), and then set targetPosition = isFirst ? f : (1.0 -
f) when creating FocusResizeSplitTarget (use
focusResizeFindDividerPosition(forSplit:in:) to compare and the same threshold
logic). Ensure you handle ancestorCount == 0 and unchanged early-continues the
same way.
- Around line 12378-12427: The current start/stop flow snaps to the previous
animation's final positions because focusResizeStopAnimation always forces
setDividerPosition for any existing focusResizeAnimation; modify the logic so
starting a new animation does not apply stale final positions: change
focusResizeStopAnimation to accept a parameter (e.g., snapToFinal: Bool = true)
or add an overload, and have focusResizeStartAnimation call
focusResizeStopAnimation(snapToFinal: false) so it only invalidates the timer
and clears state without forcing setDividerPosition; leave
focusResizeStopAnimation(snapToFinal: true) behavior for user-cancel/finish
cases and ensure focusResizeTickAnimation still clears and calls stop with
snapToFinal: true when progress >= 1.0, referencing symbols
focusResizeStartAnimation, focusResizeStopAnimation, focusResizeAnimation,
focusResizeTimer, focusResizeTickAnimation and
bonsplitController.setDividerPosition.
In `@tests_v2/test_focus_resize.py`:
- Around line 205-206: The current cleanup catches all exceptions with "except
Exception: pass" in tests_v2/test_focus_resize.py which silently hides teardown
failures; replace that block so it does not swallow errors — either catch only
expected exceptions or change to "except Exception as e:" and then log the error
(e.g., logger.exception("Workspace teardown failed") or call
pytest.fail(f"Cleanup failed: {e}")) or re-raise the exception to surface the
failure; locate the exact "except Exception: pass" and update it to one of these
safer behaviors.
- Around line 54-62: The _disable_focus_resize() helper currently deletes
persistent settings and can wipe a developer's preexisting defaults; update the
pair of helpers so _enable_focus_resize(ratio) first reads and stores the
existing values (using _defaults_read or equivalent) for ENABLED_KEY and
RATIO_KEY before writing the test values, and then have _disable_focus_resize()
restore those stored values (re-writing previous values or deleting only if they
were originally absent) instead of unconditionally calling _defaults_delete;
reference the helper names _enable_focus_resize, _disable_focus_resize and the
underlying _defaults_write/_defaults_delete (and _defaults_read) to implement
save-then-restore semantics.
- Around line 181-191: The test currently skips the height assertion when
pane_b_id_3 is empty; instead make the presence of pane_b_id_3 a required
precondition and always run the check: replace the conditional guard around
pane_b_id_3 with an explicit assertion (use must or similar) that pane_b_id_3 is
non-empty, referencing pane_b_id_3 and workspace_panes, then call
_wait_for_ratio(pane_c_id, pane_b_id_3[0], "height", ...) and run the must(...)
assertion on c_height_ratio so the test fails if no B pane exists rather than
silently passing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96adc088-b376-441a-ba28-987cf8f4f1a4
📒 Files selected for processing (7)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/KeyboardShortcutSettingsFileStore.swiftSources/Workspace.swiftSources/cmuxApp.swiftcmuxTests/FocusResizeSettingsTests.swifttests_v2/test_focus_resize.py
Greptile SummaryAdds automatic focus-pane resizing: when a pane gains focus, ancestor split dividers animate to give it a configurable proportion (50–90%, default 75%) of each split's space. Settings (toggle + slider) are wired through
Confidence Score: 3/5Two P1 issues need fixing before merge: a duplicate test block and a settings-cleanup type bug that corrupts user defaults. Two P1 findings (duplicate test with missing intended coverage, bool-key restore type mismatch that silently corrupts user settings) pull the score to 3/5. The production Swift logic is sound — animation timer, drag-override tracking, and defaults.double(forKey:) fix are all correct — but the test file issues are real and actionable. tests_v2/test_focus_resize.py (duplicate block + cleanup bug), Sources/Workspace.swift (file size growth) Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant BonsplitDelegate as BonsplitDelegate (Workspace)
participant FocusResizeSettings
participant AnimationTimer as Timer (120 fps)
participant BonsplitController
User->>BonsplitDelegate: didFocusPane(_:)
BonsplitDelegate->>FocusResizeSettings: isEnabled()
FocusResizeSettings-->>BonsplitDelegate: true/false
alt enabled
BonsplitDelegate->>BonsplitDelegate: focusResizeBuildTargets(forPane:ratio:tree:)
note right of BonsplitDelegate: per-orientation sqrt(ratio) for same-axis ancestors
BonsplitDelegate->>BonsplitDelegate: focusResizeStartAnimation(targets:)
BonsplitDelegate->>AnimationTimer: RunLoop.add(timer, forMode: .common)
loop each tick until progress >= 1.0
AnimationTimer->>BonsplitDelegate: focusResizeTickAnimation()
BonsplitDelegate->>BonsplitController: setDividerPosition(_:forSplit:)
end
AnimationTimer->>BonsplitDelegate: focusResizeStopAnimation(snapToFinal: false)
end
User->>BonsplitDelegate: didMoveDivider(_:)
alt not animating and enabled
BonsplitDelegate->>BonsplitController: treeSnapshot()
BonsplitController-->>BonsplitDelegate: ExternalTreeNode
BonsplitDelegate->>BonsplitDelegate: detect drag divergence from target
BonsplitDelegate->>BonsplitDelegate: focusResizeDragOverrideSplits.insert(splitId)
end
User->>BonsplitDelegate: Settings slider moved
BonsplitDelegate->>BonsplitDelegate: focusResizeSettingsDidChange()
note right of BonsplitDelegate: debounced 0.3s via asyncAfter
BonsplitDelegate->>BonsplitDelegate: focusResizeApplyCurrentSettings()
Reviews (3): Last reviewed commit: "fix review feedback" | Re-trigger Greptile |
There was a problem hiding this comment.
5 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests_v2/test_focus_resize.py">
<violation number="1" location="tests_v2/test_focus_resize.py:60">
P2: Test writes persistent UserDefaults keys but cleanup deletes them instead of restoring pre-existing values, causing host-state side effects and weak test isolation.</violation>
<violation number="2" location="tests_v2/test_focus_resize.py:183">
P2: A key assertion in Test 3b is conditional, so the test can pass while silently skipping vertical resize validation when pane lookup fails.</violation>
</file>
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:11937">
P1: The configured ratio is applied independently to every ancestor split, causing the effective pane size to compound. For a pane nested under two splits at 0.75, it ends up at 0.75 × 0.75 ≈ 56% instead of 75%. Either limit the resize to the immediate parent split, or compute per-level targets so the final pane share matches the configured ratio.</violation>
<violation number="2" location="Sources/Workspace.swift:12279">
P2: Manual-drag override is inferred from generic geometry changes, so non-drag layout updates can disable future auto-resize for affected splits.</violation>
<violation number="3" location="Sources/Workspace.swift:12379">
P2: `focusResizeStartAnimation` calls `focusResizeStopAnimation()` which commits the previous animation's final target positions before the new animation begins. During rapid pane switches or slider drags, this causes a visible snap to the old target before animating to the new one. Stop the old animation without committing its final positions when starting a new one.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
tests_v2/test_focus_resize.py (3)
54-62:⚠️ Potential issue | 🟠 MajorRestore prior defaults instead of always deleting them
Line 59–61 currently deletes persisted keys unconditionally, and Line 270 always runs this path. This can wipe a developer’s preexisting local config after test execution.
Proposed fix
+def _defaults_read(key: str) -> str | None: + result = subprocess.run( + ["defaults", "read", BUNDLE_ID, key], + check=False, + capture_output=True, + text=True, + ) + if result.returncode != 0: + return None + return result.stdout.strip() + +def _restore_focus_resize_defaults(prev_enabled: str | None, prev_ratio: str | None) -> None: + if prev_enabled is None: + _defaults_delete(ENABLED_KEY) + else: + _defaults_write(ENABLED_KEY, "bool", prev_enabled) + if prev_ratio is None: + _defaults_delete(RATIO_KEY) + else: + _defaults_write(RATIO_KEY, "float", prev_ratio) + def _run_once(socket_path: str) -> int: workspace_id = "" + prev_enabled = _defaults_read(ENABLED_KEY) + prev_ratio = _defaults_read(RATIO_KEY) try: _enable_focus_resize(0.75) time.sleep(2.0) ... finally: - _disable_focus_resize() + _restore_focus_resize_defaults(prev_enabled, prev_ratio) if workspace_id: ...Also applies to: 270-271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_focus_resize.py` around lines 54 - 62, The test helpers currently unconditionally delete persisted keys in _disable_focus_resize, which can erase a developer's existing config; change the pattern so _enable_focus_resize captures and stores the previous values (using _defaults_read or equivalent) for ENABLED_KEY and RATIO_KEY before writing new ones, and then have _disable_focus_resize restore those saved values (re-write them if present or delete only if they were absent originally) rather than always calling _defaults_delete; update any callers (e.g., the teardown path at the noted location) to rely on this restore behavior so prior defaults are preserved.
181-191:⚠️ Potential issue | 🟠 MajorMake Test 3b preconditions strict; don’t skip the assertion
Line 183 conditionally skips the height check. If pane discovery fails, this test can pass without validating the intended behavior.
Proposed fix
- pane_b_id_3 = [pid for pid, _, _ in workspace_panes(client, workspace_id) - if pid != pane_a_id and pid != pane_c_id] - if pane_b_id_3: - c_height_ratio = _wait_for_ratio(client, pane_c_id, pane_b_id_3[0], "height", min_ratio=0.60, max_ratio=0.90) - must( - 0.60 < c_height_ratio < 0.90, - f"Test 3b FAIL: pane C should be ~75% height, got {c_height_ratio:.2%} " - f"(C_height={pane_extent(client, pane_c_id, 'height'):.0f}, " - f"B_height={pane_extent(client, pane_b_id_3[0], 'height'):.0f})", - ) - print(f" Test 3b PASS: pane C is {c_height_ratio:.1%} of height") + pane_b_candidates = [ + pid for pid, _, _ in workspace_panes(client, workspace_id) + if pid != pane_a_id and pid != pane_c_id + ] + must( + len(pane_b_candidates) == 1, + f"expected exactly one remaining sibling pane for Test 3b, got {len(pane_b_candidates)}", + ) + pane_b_id_3 = pane_b_candidates[0] + c_height_ratio = _wait_for_ratio(client, pane_c_id, pane_b_id_3, "height", min_ratio=0.60, max_ratio=0.90) + must( + 0.60 < c_height_ratio < 0.90, + f"Test 3b FAIL: pane C should be ~75% height, got {c_height_ratio:.2%} " + f"(C_height={pane_extent(client, pane_c_id, 'height'):.0f}, " + f"B_height={pane_extent(client, pane_b_id_3, 'height'):.0f})", + ) + print(f" Test 3b PASS: pane C is {c_height_ratio:.1%} of height")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_focus_resize.py` around lines 181 - 191, The test currently skips the height assertion when no alternate pane is found; change this to fail explicitly so the test cannot pass silently: replace the conditional "if pane_b_id_3:" with a precondition check using the existing test helpers (e.g., must(pane_b_id_3, 'Test 3b precondition FAIL: could not find pane B for height comparison')) and then run the _wait_for_ratio / must assertions (using pane_b_id_3[0]) as before; reference functions/vars: pane_b_id_3, workspace_panes, _wait_for_ratio, must, pane_extent.
275-276:⚠️ Potential issue | 🟡 MinorDon’t silently swallow cleanup errors
Line 275–276 hides teardown failures completely. This makes regressions/flakes much harder to diagnose.
Proposed fix
- except Exception: - pass + except Exception as exc: + print(f"WARN: cleanup close_workspace({workspace_id}) failed: {exc}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_focus_resize.py` around lines 275 - 276, The test currently swallows cleanup errors with the bare "except Exception: pass" which hides teardown failures; replace that block so cleanup exceptions are not silently ignored—catch the exception as "except Exception as e" and either log it with traceback (e.g., logging.exception or pytest.fail with the error) or re-raise after any necessary local cleanup so test infrastructure surfaces the failure; update the "except Exception: pass" occurrence in test_focus_resize (the try/except cleanup block) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests_v2/test_focus_resize.py`:
- Around line 54-62: The test helpers currently unconditionally delete persisted
keys in _disable_focus_resize, which can erase a developer's existing config;
change the pattern so _enable_focus_resize captures and stores the previous
values (using _defaults_read or equivalent) for ENABLED_KEY and RATIO_KEY before
writing new ones, and then have _disable_focus_resize restore those saved values
(re-write them if present or delete only if they were absent originally) rather
than always calling _defaults_delete; update any callers (e.g., the teardown
path at the noted location) to rely on this restore behavior so prior defaults
are preserved.
- Around line 181-191: The test currently skips the height assertion when no
alternate pane is found; change this to fail explicitly so the test cannot pass
silently: replace the conditional "if pane_b_id_3:" with a precondition check
using the existing test helpers (e.g., must(pane_b_id_3, 'Test 3b precondition
FAIL: could not find pane B for height comparison')) and then run the
_wait_for_ratio / must assertions (using pane_b_id_3[0]) as before; reference
functions/vars: pane_b_id_3, workspace_panes, _wait_for_ratio, must,
pane_extent.
- Around line 275-276: The test currently swallows cleanup errors with the bare
"except Exception: pass" which hides teardown failures; replace that block so
cleanup exceptions are not silently ignored—catch the exception as "except
Exception as e" and either log it with traceback (e.g., logging.exception or
pytest.fail with the error) or re-raise after any necessary local cleanup so
test infrastructure surfaces the failure; update the "except Exception: pass"
occurrence in test_focus_resize (the try/except cleanup block) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 754f050e-4b47-43c0-ae97-6ced5130f1d7
📒 Files selected for processing (4)
Sources/Workspace.swiftSources/cmuxApp.swiftcmuxTests/FocusResizeSettingsTests.swifttests_v2/test_focus_resize.py
🚧 Files skipped from review as they are similar to previous changes (3)
- cmuxTests/FocusResizeSettingsTests.swift
- Sources/Workspace.swift
- Sources/cmuxApp.swift
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:12314">
P2: Disabling focus-resize does not stop an in-flight animation, so divider movement can continue briefly after the feature is turned off.</violation>
</file>
<file name="tests_v2/test_focus_resize.py">
<violation number="1" location="tests_v2/test_focus_resize.py:250">
P2: Test 4 claims to verify pane C's total-width share, but it measures only C vs A (`C/(C+A)`), weakening the regression check and potentially letting the compounding bug pass.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests_v2/test_focus_resize.py (3)
186-196:⚠️ Potential issue | 🟠 MajorDon’t allow Test 3b to silently skip
Line 188 makes the height assertion optional. If pane discovery fails, this test can pass without validating one of its core checks. Make pane B presence a required precondition and always run the assertion.
Proposed fix
- pane_b_id_3 = [pid for pid, _, _ in workspace_panes(client, workspace_id) - if pid != pane_a_id and pid != pane_c_id] - if pane_b_id_3: - c_height_ratio = _wait_for_ratio(client, pane_c_id, pane_b_id_3[0], "height", min_ratio=0.60, max_ratio=0.90) - must( - 0.60 < c_height_ratio < 0.90, - f"Test 3b FAIL: pane C should be ~75% height, got {c_height_ratio:.2%} " - f"(C_height={pane_extent(client, pane_c_id, 'height'):.0f}, " - f"B_height={pane_extent(client, pane_b_id_3[0], 'height'):.0f})", - ) - print(f" Test 3b PASS: pane C is {c_height_ratio:.1%} of height") + pane_b_candidates = [ + pid for pid, _, _ in workspace_panes(client, workspace_id) + if pid != pane_a_id and pid != pane_c_id + ] + must( + len(pane_b_candidates) == 1, + f"expected exactly one remaining sibling pane for Test 3b, got {len(pane_b_candidates)}", + ) + pane_b_id_3 = pane_b_candidates[0] + c_height_ratio = _wait_for_ratio(client, pane_c_id, pane_b_id_3, "height", min_ratio=0.60, max_ratio=0.90) + must( + 0.60 < c_height_ratio < 0.90, + f"Test 3b FAIL: pane C should be ~75% height, got {c_height_ratio:.2%} " + f"(C_height={pane_extent(client, pane_c_id, 'height'):.0f}, " + f"B_height={pane_extent(client, pane_b_id_3, 'height'):.0f})", + ) + print(f" Test 3b PASS: pane C is {c_height_ratio:.1%} of height")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_focus_resize.py` around lines 186 - 196, The test currently skips the height assertion when pane_b_id_3 is empty; make presence of pane B a required precondition so the assertion always runs. Replace the conditional guard around pane_b_id_3 with an explicit check (e.g., call must(pane_b_id_3, "Test 3b FAIL: expected pane B to be present") or raise/assert) referencing pane_b_id_3, workspace_panes, pane_a_id, pane_c_id, then call _wait_for_ratio and must(...) as before (use pane_extent for the failure message) so the test fails loudly if pane B cannot be discovered.
277-281:⚠️ Potential issue | 🟡 MinorAvoid swallowing cleanup failures
Line 280 catches all exceptions and ignores them. That hides teardown regressions and makes failures harder to diagnose.
Proposed fix
- except Exception: - pass + except Exception as exc: + print(f"WARN: cleanup close_workspace({workspace_id}) failed: {exc}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_focus_resize.py` around lines 277 - 281, The cleanup block currently swallows all exceptions which hides teardown failures; update the try/except around the cmux(...) as cleanup and cleanup.close_workspace(workspace_id) so that you either let errors propagate (remove the broad except) or catch specific expected exceptions (e.g., OSError) and log the exception before re-raising; if you must not fail the test, at minimum catch Exception as e and call the test logger or pytest.fail/pytest.skip with the error message (use the cmux context, cleanup variable, and workspace_id identifiers to locate the code).
56-66:⚠️ Potential issue | 🟠 MajorRestore prior defaults instead of always deleting them
Line 63/Line 275 teardown currently removes keys unconditionally, which can wipe a developer’s existing local settings after test execution. Save current values before writing test values, then restore in
finally.Proposed fix
+def _defaults_read(key: str) -> str | None: + result = subprocess.run( + ["defaults", "read", BUNDLE_ID, key], + check=False, + capture_output=True, + text=True, + ) + if result.returncode != 0: + return None + return result.stdout.strip() + +def _restore_focus_resize_defaults(prev_enabled: str | None, prev_ratio: str | None) -> None: + if prev_enabled is None: + _defaults_delete(ENABLED_KEY) + else: + _defaults_write(ENABLED_KEY, "bool", prev_enabled) + if prev_ratio is None: + _defaults_delete(RATIO_KEY) + else: + _defaults_write(RATIO_KEY, "float", prev_ratio) + def _run_once(socket_path: str) -> int: """Run the full focus-resize test suite against a single cmux socket.""" workspace_id = "" + prev_enabled = _defaults_read(ENABLED_KEY) + prev_ratio = _defaults_read(RATIO_KEY) try: ... finally: - _disable_focus_resize() + _restore_focus_resize_defaults(prev_enabled, prev_ratio) if workspace_id: ...Also applies to: 274-276
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_focus_resize.py` around lines 56 - 66, Tests unconditionally delete UserDefaults keys in _disable_focus_resize which can wipe a developer's local settings; modify _enable_focus_resize to read and save existing values for ENABLED_KEY and RATIO_KEY before writing test values (using _defaults_write), and change teardown/_disable_focus_resize to restore those saved previous values in a finally block (re-applying via _defaults_write if present or removing via _defaults_delete if absent) so original defaults are preserved; reference functions _enable_focus_resize, _disable_focus_resize and helpers _defaults_write/_defaults_delete and keys ENABLED_KEY/RATIO_KEY when implementing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests_v2/test_focus_resize.py`:
- Around line 254-264: The assertion currently computes C/(C+A) via
_wait_for_ratio(client, pane_4c_id, pane_4a_id, ...) which lets regressions
pass; change the check to compute C relative to the total width (sum of all pane
extents) instead. Replace or supplement the _wait_for_ratio usage so you compute
total = sum(pane_extent(client, id, "width") for each pane id in the 4-pane
layout), then compute c_total_ratio = pane_extent(client, pane_4c_id, "width") /
total and assert must(0.60 < c_total_ratio < 0.90, ...) (or similar bounds
around 0.75). Update references to pane_4c_id, pane_4a_id, pane_extent, and must
accordingly so the test validates C vs full total instead of C vs A only.
---
Duplicate comments:
In `@tests_v2/test_focus_resize.py`:
- Around line 186-196: The test currently skips the height assertion when
pane_b_id_3 is empty; make presence of pane B a required precondition so the
assertion always runs. Replace the conditional guard around pane_b_id_3 with an
explicit check (e.g., call must(pane_b_id_3, "Test 3b FAIL: expected pane B to
be present") or raise/assert) referencing pane_b_id_3, workspace_panes,
pane_a_id, pane_c_id, then call _wait_for_ratio and must(...) as before (use
pane_extent for the failure message) so the test fails loudly if pane B cannot
be discovered.
- Around line 277-281: The cleanup block currently swallows all exceptions which
hides teardown failures; update the try/except around the cmux(...) as cleanup
and cleanup.close_workspace(workspace_id) so that you either let errors
propagate (remove the broad except) or catch specific expected exceptions (e.g.,
OSError) and log the exception before re-raising; if you must not fail the test,
at minimum catch Exception as e and call the test logger or
pytest.fail/pytest.skip with the error message (use the cmux context, cleanup
variable, and workspace_id identifiers to locate the code).
- Around line 56-66: Tests unconditionally delete UserDefaults keys in
_disable_focus_resize which can wipe a developer's local settings; modify
_enable_focus_resize to read and save existing values for ENABLED_KEY and
RATIO_KEY before writing test values (using _defaults_write), and change
teardown/_disable_focus_resize to restore those saved previous values in a
finally block (re-applying via _defaults_write if present or removing via
_defaults_delete if absent) so original defaults are preserved; reference
functions _enable_focus_resize, _disable_focus_resize and helpers
_defaults_write/_defaults_delete and keys ENABLED_KEY/RATIO_KEY when
implementing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 797bce18-f8d6-4ef7-9cca-0249d59ba2d4
📒 Files selected for processing (1)
tests_v2/test_focus_resize.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
tests_v2/test_focus_resize.py (3)
287-292:⚠️ Potential issue | 🟡 MinorDon't swallow workspace teardown failures.
The blanket
except Exception: passstill hides cleanup regressions and makes intermittent failures much harder to debug. At minimum, log the exception.Possible fix
- except Exception: - pass + except Exception as exc: + print(f"WARN: cleanup close_workspace({workspace_id}) failed: {exc}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_focus_resize.py` around lines 287 - 292, The current teardown swallows all errors in the block using "except Exception: pass", which hides failures; change this to "except Exception as e" and log the exception before continuing (e.g., use a module logger with logger.exception(...) or print the exception) for the cleanup around cmux(socket_path) and cleanup.close_workspace(workspace_id) so teardown failures are visible and debuggable.
186-196:⚠️ Potential issue | 🟠 MajorMake Test 3b fail when pane B is missing.
The
if pane_b_id_3:guard still lets the height assertion disappear entirely, so this scenario can pass without validating the second axis. Require exactly one remaining sibling pane and always run the check.Possible fix
- pane_b_id_3 = [pid for pid, _, _ in workspace_panes(client, workspace_id) - if pid != pane_a_id and pid != pane_c_id] - if pane_b_id_3: - c_height_ratio = _wait_for_ratio(client, pane_c_id, pane_b_id_3[0], "height", min_ratio=0.60, max_ratio=0.90) - must( - 0.60 < c_height_ratio < 0.90, - f"Test 3b FAIL: pane C should be ~75% height, got {c_height_ratio:.2%} " - f"(C_height={pane_extent(client, pane_c_id, 'height'):.0f}, " - f"B_height={pane_extent(client, pane_b_id_3[0], 'height'):.0f})", - ) - print(f" Test 3b PASS: pane C is {c_height_ratio:.1%} of height") + pane_b_candidates = [ + pid for pid, _, _ in workspace_panes(client, workspace_id) + if pid != pane_a_id and pid != pane_c_id + ] + must( + len(pane_b_candidates) == 1, + f"expected exactly one remaining sibling pane for Test 3b, got {len(pane_b_candidates)}", + ) + pane_b_id_3 = pane_b_candidates[0] + c_height_ratio = _wait_for_ratio(client, pane_c_id, pane_b_id_3, "height", min_ratio=0.60, max_ratio=0.90) + must( + 0.60 < c_height_ratio < 0.90, + f"Test 3b FAIL: pane C should be ~75% height, got {c_height_ratio:.2%} " + f"(C_height={pane_extent(client, pane_c_id, 'height'):.0f}, " + f"B_height={pane_extent(client, pane_b_id_3, 'height'):.0f})", + ) + print(f" Test 3b PASS: pane C is {c_height_ratio:.1%} of height")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_focus_resize.py` around lines 186 - 196, The test currently skips the height assertion when pane_b_id_3 is empty; change the guard so the test requires exactly one remaining sibling and always runs the check: replace the "if pane_b_id_3:" guard with a check/assert using pane_b_id_3 (e.g., require len(pane_b_id_3) == 1 via must or an explicit failure) and then call _wait_for_ratio(..., pane_b_id_3[0], "height", ...), followed by the existing must/print logic referencing pane_c_id, pane_b_id_3[0], _wait_for_ratio and pane_extent so the test fails when B is missing instead of silently passing.
56-65:⚠️ Potential issue | 🟠 MajorRestore prior defaults instead of always deleting them.
This still overwrites persistent app settings and then clears them, so any preexisting local configuration is lost after the run.
Possible fix
+def _defaults_read(key: str) -> str | None: + result = subprocess.run( + ["defaults", "read", BUNDLE_ID, key], + check=False, + capture_output=True, + text=True, + ) + if result.returncode != 0: + return None + return result.stdout.strip() + +def _restore_focus_resize_defaults(prev_enabled: str | None, prev_ratio: str | None) -> None: + if prev_enabled is None: + _defaults_delete(ENABLED_KEY) + else: + _defaults_write(ENABLED_KEY, "bool", prev_enabled) + + if prev_ratio is None: + _defaults_delete(RATIO_KEY) + else: + _defaults_write(RATIO_KEY, "float", prev_ratio) + def _run_once(socket_path: str) -> int: + prev_enabled = _defaults_read(ENABLED_KEY) + prev_ratio = _defaults_read(RATIO_KEY) try: _enable_focus_resize(0.75) ... finally: - _disable_focus_resize() + _restore_focus_resize_defaults(prev_enabled, prev_ratio)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_focus_resize.py` around lines 56 - 65, The tests currently set UserDefaults via _enable_focus_resize (calling _defaults_write) and then remove keys in _disable_focus_resize (calling _defaults_delete), which wipes any preexisting user configuration; change this to capture and restore prior values instead: when _enable_focus_resize runs, read existing values for ENABLED_KEY and RATIO_KEY (via whatever read helper exists or _defaults_read) and store them in a module-level variable (e.g., PREV_DEFAULTS keyed by those constants), then write the test values; update _disable_focus_resize to check PREV_DEFAULTS and restore each key with _defaults_write if a previous value existed or call _defaults_delete only if there was no prior value, ensuring original UserDefaults are preserved after the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests_v2/test_focus_resize.py`:
- Around line 27-31: DEFAULT_SOCKET_PATHS currently includes an untagged
fallback "/tmp/cmux-debug.sock" which can accidentally target a local DEV
instance; remove that fallback from the DEFAULT_SOCKET_PATHS list so the code
only uses CMUX_SOCKET env or explicit tagged socket paths (i.e., keep
os.environ.get("CMUX_SOCKET", "") and any tagged path entries, but delete the
"/tmp/cmux-debug.sock" element), and optionally add a brief comment near
DEFAULT_SOCKET_PATHS explaining that tests must use CMUX_SOCKET or a tagged
socket to avoid binding to an untagged DEV instance.
- Around line 97-101: The test currently calls _enable_focus_resize(0.75) after
the app is already running which can leave the app using its in-memory 50/50
defaults; move the settings change so it is applied before launching cmux (or
add an in-process settings hook/RPC that sets and returns the current
focus-resize value) and then explicitly poll/verify the new value via that RPC
or by reading the app's UserDefaults wrapper (e.g., via the same function that
_enable_focus_resize uses) to confirm the 0.75 value took effect before
proceeding with time.sleep() and the geometry assertions.
---
Duplicate comments:
In `@tests_v2/test_focus_resize.py`:
- Around line 287-292: The current teardown swallows all errors in the block
using "except Exception: pass", which hides failures; change this to "except
Exception as e" and log the exception before continuing (e.g., use a module
logger with logger.exception(...) or print the exception) for the cleanup around
cmux(socket_path) and cleanup.close_workspace(workspace_id) so teardown failures
are visible and debuggable.
- Around line 186-196: The test currently skips the height assertion when
pane_b_id_3 is empty; change the guard so the test requires exactly one
remaining sibling and always runs the check: replace the "if pane_b_id_3:" guard
with a check/assert using pane_b_id_3 (e.g., require len(pane_b_id_3) == 1 via
must or an explicit failure) and then call _wait_for_ratio(..., pane_b_id_3[0],
"height", ...), followed by the existing must/print logic referencing pane_c_id,
pane_b_id_3[0], _wait_for_ratio and pane_extent so the test fails when B is
missing instead of silently passing.
- Around line 56-65: The tests currently set UserDefaults via
_enable_focus_resize (calling _defaults_write) and then remove keys in
_disable_focus_resize (calling _defaults_delete), which wipes any preexisting
user configuration; change this to capture and restore prior values instead:
when _enable_focus_resize runs, read existing values for ENABLED_KEY and
RATIO_KEY (via whatever read helper exists or _defaults_read) and store them in
a module-level variable (e.g., PREV_DEFAULTS keyed by those constants), then
write the test values; update _disable_focus_resize to check PREV_DEFAULTS and
restore each key with _defaults_write if a previous value existed or call
_defaults_delete only if there was no prior value, ensuring original
UserDefaults are preserved after the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aaee569b-511a-42e9-bafe-9809728dacf4
📒 Files selected for processing (2)
Sources/Workspace.swifttests_v2/test_focus_resize.py
✅ Files skipped from review due to trivial changes (1)
- Sources/Workspace.swift
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests_v2/test_focus_resize.py (1)
125-125: Prefernext()over single-element slice.Using
next()is more idiomatic and will raiseStopIterationimmediately if no match is found, rather thanIndexError.Suggested change
- pane_a_id = [pid for pid, _, _ in panes if pid != pane_b_id][0] + pane_a_id = next(pid for pid, _, _ in panes if pid != pane_b_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_focus_resize.py` at line 125, Replace the single-element slice lookup for pane_a_id with next() to be more idiomatic and to raise StopIteration when no match exists: locate the assignment where pane_a_id is computed from panes and pane_b_id (pane_a_id = [pid for pid, _, _ in panes if pid != pane_b_id][0]) and change it to use next() over a generator expression (next(pid for pid, _, _ in panes if pid != pane_b_id)), preserving the same semantics but relying on next() to signal missing elements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests_v2/test_focus_resize.py`:
- Line 125: Replace the single-element slice lookup for pane_a_id with next() to
be more idiomatic and to raise StopIteration when no match exists: locate the
assignment where pane_a_id is computed from panes and pane_b_id (pane_a_id =
[pid for pid, _, _ in panes if pid != pane_b_id][0]) and change it to use next()
over a generator expression (next(pid for pid, _, _ in panes if pid !=
pane_b_id)), preserving the same semantics but relying on next() to signal
missing elements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b3af79a-40c9-4a8f-8ff0-4a4d17995d0e
📒 Files selected for processing (1)
tests_v2/test_focus_resize.py
c78e558 to
b8ab251
Compare
b8ab251 to
ec91a9c
Compare
|
@coderabbitai review |
|
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit. |
- Distribute ratio per orientation so same-axis ancestors don't compound - Filter settings observer to only react to focusResize key changes - Cancel pending debounce work immediately when feature is disabled - Add snapToFinal parameter to stopAnimation to avoid stale target snaps - Schedule animation timer with .common run loop node for draf tracking - Use defaults.double(forKey:) to handle Float-backed NSNumber coercion - Add Float-backed NSNUmber unit test - Add same-orientation nesting socket test for per-orientation factor
b8ea82a to
a5bb235
Compare
- Stop in-flight animation when disabling focus-resize - Guard both dimension in split test
| # Create a new workspace with A | (B | C) — two nested horizontal splits. | ||
| # Without per-orientation correction, C would get 0.75 * 0.75 = 56% of | ||
| # total width. With correction, each split gets sqrt(0.75) ≈ 0.866, so | ||
| # C gets ~75% of total width. | ||
| workspace_id = client.new_workspace() | ||
| client.select_workspace(workspace_id) | ||
| surfaces_4 = client.list_surfaces(workspace_id) | ||
| must(bool(surfaces_4), "workspace should have at least one surface") | ||
| surface_4a = surfaces_4[0][1] | ||
|
|
||
| wait_for_surface_command_roundtrip(client, workspace_id, surface_4a) | ||
|
|
||
| # First split: A | B | ||
| surface_4b = client.new_split("right") | ||
| wait_for( | ||
| lambda: len(workspace_panes(client, workspace_id)) >= 2, | ||
| timeout_s=4.0, | ||
| ) | ||
| time.sleep(0.5) | ||
|
|
||
| # Second split: A | (B | C) — split B to the right again | ||
| client.focus_surface(surface_4b) | ||
| time.sleep(0.3) | ||
| surface_4c = client.new_split("right") | ||
| wait_for( | ||
| lambda: len(workspace_panes(client, workspace_id)) >= 3, | ||
| timeout_s=4.0, | ||
| ) | ||
| time.sleep(0.5) | ||
|
|
||
| # Identify pane A by finding which pane contains surface_4a | ||
| pane_4a_id = None | ||
| for pid, _, _ in workspace_panes(client, workspace_id): | ||
| client.focus_pane(pid) | ||
| time.sleep(0.1) | ||
| surfs = client.list_pane_surfaces(pid) | ||
| for _, sid, _, _ in surfs: | ||
| if sid == surface_4a: | ||
| pane_4a_id = pid | ||
| break | ||
| if pane_4a_id: | ||
| break | ||
| must(pane_4a_id is not None, "Could not find pane A in test 4") | ||
|
|
||
| # Focus A first, then focus C to trigger resize | ||
| client.focus_surface(surface_4a) | ||
| time.sleep(0.5) | ||
| client.focus_surface(surface_4c) | ||
| time.sleep(0.3) | ||
| pane_4c_id = focused_pane_id(client, workspace_id) | ||
|
|
||
| # C should get ~75% of total width (not 56% from compounding). | ||
| # Measure C against the full container, not just C+A, since B also | ||
| # takes width and C/(C+A) would mask the compounding bug. | ||
| def _c_share_of_total() -> float: | ||
| panes_4 = layout_panes(client) | ||
| total_w = sum( | ||
| float((p.get("frame") or {}).get("width") or 0) | ||
| for p in panes_4 | ||
| ) | ||
| c_w = pane_extent(client, pane_4c_id, "width") | ||
| return c_w / total_w if total_w > 0 else 0.0 | ||
|
|
||
| wait_for( | ||
| lambda: _c_share_of_total() > 0.60, | ||
| timeout_s=5.0, | ||
| ) | ||
| c_total_ratio = _c_share_of_total() | ||
| must( | ||
| 0.60 < c_total_ratio < 0.90, | ||
| f"Test 4 FAIL: pane C should be ~75% of total width (per-orientation), " | ||
| f"got {c_total_ratio:.2%} " | ||
| f"(C={pane_extent(client, pane_4c_id, 'width'):.0f})", | ||
| ) | ||
| print(f" Test 4 PASS: pane C is {c_total_ratio:.1%} of total width " | ||
| f"(same-orientation nesting, per-orientation factor)") | ||
|
|
||
| client.close_workspace(workspace_id) | ||
| workspace_id = "" | ||
|
|
There was a problem hiding this comment.
Test 4 is duplicated verbatim — second block is dead code
Lines 311–390 are an exact copy of lines 230–309, including the # --- Test 4: Same-orientation nesting --- comment, variable names (surface_4a/b/c, pane_4a/c_id), _c_share_of_total closure, and assertions. A second workspace_id = "" cleanup is executed on an already-closed workspace. No additional scenario is exercised: any distinct Test 5 scenario the author had in mind (vertical-only layout, different ratio, or a disabling toggle mid-test) is silently absent. The duplicate should be removed or replaced with the intended distinct test.
| _defaults_write(key, "string", prev) | ||
| PREV_DEFAULTS.clear() | ||
|
|
There was a problem hiding this comment.
Restore uses wrong
defaults type for the boolean key
_defaults_read returns the raw CLI string representation of a bool. Passing that back through _defaults_write with type "string" writes an NSString instead of the original NSNumber bool. FocusResizeSettings.isEnabled() then calls defaults.object(forKey:) as? Bool, which returns nil for an NSString and silently falls back to defaultEnabled = false. A user who had focus-resize enabled before the test will find it disabled after cleanup.
The restore branch should apply type "bool" when writing ENABLED_KEY and type "float" when writing RATIO_KEY.
| } | ||
| } | ||
|
|
||
| // MARK: - Focus Resize | ||
|
|
||
| /// Split IDs where the user has manually dragged the divider (suppresses auto-resize until focus changes) | ||
| private var focusResizeDragOverrideSplits: Set<UUID> = [] | ||
|
|
||
| /// The last pane that triggered a focus-resize, used to detect focus changes for clearing drag overrides | ||
| private var focusResizeLastPaneId: UUID? | ||
|
|
||
| /// Active timer for focus-resize animation | ||
| private var focusResizeTimer: Timer? | ||
|
|
||
| /// Whether a focus-resize animation is currently running (suppresses drag-override detection) | ||
| private var focusResizeIsAnimating = false | ||
|
|
||
| /// Animation state for the in-flight focus-resize | ||
| private var focusResizeAnimation: FocusResizeAnimationState? | ||
|
|
||
| /// Debounced work item for live-preview when the ratio slider changes | ||
| private var focusResizeRatioDebounceWorkItem: DispatchWorkItem? | ||
|
|
||
| /// Observer for ratio UserDefaults changes | ||
| private var focusResizeRatioObserver: NSObjectProtocol? | ||
|
|
||
| /// Cached settings state to avoid reacting to unrelated UserDefaults changes | ||
| private var focusResizeLastEnabled: Bool = FocusResizeSettings.defaultEnabled | ||
| private var focusResizeLastRatio: Double = FocusResizeSettings.defaultRatio | ||
|
|
||
| private struct FocusResizeSplitTarget { | ||
| let splitId: UUID | ||
| let startPosition: CGFloat | ||
| let targetPosition: CGFloat | ||
| } | ||
|
|
||
| private struct FocusResizeAnimationState { | ||
| let splits: [FocusResizeSplitTarget] | ||
| let startTime: CFTimeInterval | ||
| let duration: CFTimeInterval | ||
| } | ||
|
|
||
| // MARK: - Initialization | ||
|
|
||
| private static func currentSplitButtonTooltips() -> BonsplitConfiguration.SplitButtonTooltips { |
There was a problem hiding this comment.
~275 lines added to a file already far beyond the 800-line threshold
The focus-resize feature adds 9 new mutable instance variables, 2 private structs, and 7 private functions directly to Workspace.swift (already referenced at line ~14 000 in prior review threads). The cmux-swift-file-package-boundaries rule flags additions of more than 250 lines to a file already over 800 lines unless an equivalent extraction happens in the same PR.
The focus-resize state machine — ancestor-split traversal, per-orientation ratio distribution, animation interpolation, drag-override tracking, and UserDefaults observation — has a stable domain noun, carries no AppKit/SwiftUI view types, and is already covered by unit tests in FocusResizeSettingsTests. A dedicated source file (FocusResizeController.swift) or small package target would give this logic a clear owner and an isolated test surface.
Rule Used: Flag Swift changes that add too much unrelated res... (source)
Summary
Automatically resize the focused pane.
Add config to toggle behaviour and size (50-90%) of active pane.
I often use multiple panes in my workflow and this helps focus on what I am currently working on while still seeing if other require attention.
Testing
cmuxTests/FocusResizeSettingsTests.swift,tests_v2/test_focus_resize.pyOpen multiple panes both horizontally and vertically to verify resize behaviour.
Toggle setting off to preserve existing behaviour.
Demo Video
For UI or behavior changes, include a short demo video (GitHub upload, Loom, or other direct link).
https://github.com/user-attachments/assets/d6adddc6-341d-4757-828f-192a5be67f9f
Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by CodeRabbit
New Features
Settings
Localization
Tests
Summary by cubic
Automatically resizes the focused pane on focus change with a smooth, non-jarring animation. Adds a settings toggle and size slider so you control how much space the active pane gets.
New Features
app.focusResizeandapp.focusResizeRatio. Stored asfocusResize.enabled/focusResize.ratioin UserDefaults with clamping (0.5–0.9) anddefaults.double(forKey:)to support Float-backed NSNumber.Bug Fixes
Written for commit 3522ece. Summary will update on new commits.