Skip to content

Enforce divider range for default splits and drifted dividers - #169

Merged
austinywang merged 1 commit into
mainfrom
coderabbit-167-divider-range
Jul 10, 2026
Merged

austinywang merged 1 commit into
mainfrom
coderabbit-167-divider-range

Conversation

@austinywang

@austinywang austinywang commented Jul 10, 2026 •

Copy link
Copy Markdown

Addresses the two remaining CodeRabbit findings on #167:

  1. Default (nil) divider position bypasses the configured range — normalizedInitialDividerPosition now clamps the implicit 0.5 default into dividerPositionRange instead of passing nil through to the internal hardcoded fallback.
  2. Pre-clamped comparison masks out-of-range dividers — syncPosition now compares the raw currentDividerPixels / availableSize ratio, so a divider that physically drifted outside the configured range (non-drag AppKit resize) is re-asserted to the model position instead of early-returning.

Both fixes verified red/green: testDefaultSplitPositionRespectsConfiguredDividerRange fails at 0.5 and testSyncRestoresDividerThatDriftedOutsideConfiguredRange fails at 0.2 without the fixes. Full suite: 184/184 pass.

🤖 Generated with Claude Code


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.


Summary by cubic

Enforces dividerPositionRange for default split position and restores dividers that drift outside the range after non-drag resizes. Prevents bypassing the configured range and keeps the view in sync with the model.

  • Bug Fixes
    • Clamp the implicit 0.5 default into dividerPositionRange in normalizedInitialDividerPosition.
    • In syncPosition, compare the raw ratio (currentDividerPixels / availableSize) and re-assert the clamped model position when the divider drifts out of range.

Written for commit 4b46851. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Divider positions that drift outside the configured range are now automatically restored.
    • Default divider placement now respects configured position limits, including when no initial position is provided.
  • Tests

    • Added coverage for restoring out-of-range dividers and clamping default positions to configured limits.

Two follow-ups to #167's dividerPositionRange contract:

- A nil initialDividerPosition fell through to the internal 0.5 fallback,
  bypassing a configured range that excludes 0.5. The default is now
  clamped exactly like an explicit position.
- syncPosition compared a pre-clamped current ratio, so a divider that
  physically drifted outside the range (non-drag AppKit resize) satisfied
  the early return and stayed out of range. The comparison now uses the
  raw pixel ratio so the re-assert path restores the model position.
@coderabbitai

coderabbitai Bot commented Jul 10, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Divider initialization now respects configured bounds, while synchronization corrects physical divider drift outside those bounds without changing the stored model position.

Changes

Divider range correction

Layer / File(s) Summary
Initial divider position normalization
Sources/Bonsplit/Public/BonsplitController.swift, Tests/BonsplitTests/EmbeddedConfigurationTests.swift
Initial and default divider positions are clamped to dividerPositionRange, with coverage for a constrained default range.
Out-of-range divider resynchronization
Sources/Bonsplit/Internal/Views/SplitContainerView.swift, Tests/BonsplitTests/BonsplitTests.swift
Synchronization compares the un-clamped physical ratio and restores dividers that drift outside the configured range without modifying the stored divider position.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the two main fixes: clamping default split positions and restoring drifted dividers into range.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch coderabbit-167-divider-range

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jul 10, 2026

Copy link
Copy Markdown

Greptile Summary

This PR keeps split dividers inside the configured range.

  • Clamps the implicit default divider position before it reaches the internal split controller.
  • Compares the raw divider ratio during split-view sync so drifted dividers are restored.
  • Adds tests for default-range clamping and non-drag divider drift.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.
  • The sync path still guards zero-size layouts and clamps the final AppKit divider write.
  • The default-position change is private and affects only local split creation call sites.

Important Files Changed

Filename Overview
Sources/Bonsplit/Internal/Views/SplitContainerView.swift Uses the raw physical divider ratio for sync comparisons while keeping the existing guarded, clamped write path.
Sources/Bonsplit/Public/BonsplitController.swift Clamps nil split positions to the configured range before calling the internal controller.
Tests/BonsplitTests/BonsplitTests.swift Adds coverage for restoring a divider that drifted outside the configured range.
Tests/BonsplitTests/EmbeddedConfigurationTests.swift Adds coverage for default split positions with a narrowed divider range.

Reviews (1): Last reviewed commit: "Enforce divider range for default splits..." | Re-trigger Greptile

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@Sources/Bonsplit/Public/BonsplitController.swift`:
- Around line 601-608: Update the movingTab split path to pass the normalized
divider position into splitPaneWithTab, using
normalizedInitialDividerPosition(_:) before the call, so configured
dividerPositionRange is enforced instead of relying on the internal 0.5 default.
🪄 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: 3d993cd4-ae76-4fcc-bed9-446217a5ad17

📥 Commits

Reviewing files that changed from the base of the PR and between 9a06689 and 4b46851.

📒 Files selected for processing (4)
  • Sources/Bonsplit/Internal/Views/SplitContainerView.swift
  • Sources/Bonsplit/Public/BonsplitController.swift
  • Tests/BonsplitTests/BonsplitTests.swift
  • Tests/BonsplitTests/EmbeddedConfigurationTests.swift

Comment thread Sources/Bonsplit/Public/BonsplitController.swift
@austinywang
austinywang merged commit 9ba9e71 into main Jul 10, 2026
6 checks passed
@austinywang
austinywang deleted the coderabbit-167-divider-range branch July 10, 2026 01:33
hhsw2015 pushed a commit to hhsw2015/bonsplit that referenced this pull request Jul 10, 2026
Follow-up to manaflow-ai#169: the movingTab overload called splitPaneWithTab without
an initialDividerPosition, so the internal 0.5 default bypassed a
configured dividerPositionRange that excludes it. Forward the clamped
default like the tab-creating overloads.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant