Skip to content

fix(sidebar): preserve WebKit workspace height when Agents folds - #3255

Open
Haeminway1 wants to merge 3 commits into
devswha:mainfrom
Haeminway1:fix/mobile-sidebar-split-mask
Open

Haeminway1 wants to merge 3 commits into
devswha:mainfrom
Haeminway1:fix/mobile-sidebar-split-mask

Conversation

@Haeminway1

Copy link
Copy Markdown
Contributor

Change

Fix the workspace list collapsing to roughly one visible row in WebKit when the PCs/Agents divider is dragged down to fold Agents. The divider and footer remain in place, so the remaining sidebar looks blank. The edge fade makes the surviving first row look covered, but disabling/changing the mask is not the root fix.

The PCs section is a flex item capped at max-height: max-content. Its inner flex column is incorrectly capped in WebKit. Use a single minmax(0, 1fr) grid row for that inner section instead. This retains its intrinsic content height and permits scrolling when a saved allocation is smaller. The outer flex proportions, measured minima, saved divider allocation, edge fades, and footer docking are unchanged. In particular, the existing long-list half-sidebar allocation test still passes; changing the outer basis to auto did not preserve that contract and was discarded.

Preserves the existing VS Code-style view folding/restoring interaction documented in DESIGN.md; this is a layout repair, not a new interaction or visual redesign. No code was adapted from another project. Includes a changelog fragment and regression assertions for actual visible workspace rows, default/collapsed/reloaded/reopened layouts, both section orders, light/dark, desktop/phone, DPR 3, and a shorter viewport.

Related alternative: #3218 addresses the same WebKit failure with an outer 0% basis. I found that PR while checking for overlap before submission. This PR instead repairs the inner layout and leaves the outer allocation untouched; these are alternative fixes, not changes that need to be stacked or both merged.

Measured evidence

All evidence used an isolated synthetic App demo, never live user sessions. Linux WebKit 26.6 is not a physical iPhone or iOS Simulator.

Mobile 393 × 852, DPR 3, Agents folded Before Grid fix
WebKit workspace list clientHeight 46 px 320 px
WebKit workspace content scrollHeight 320 px 320 px
WebKit PCs section height 54 px 328 px
Chromium workspace list clientHeight 320 px 320 px
Divider top / footer top 696 / 792 px 696 / 792 px

The new layout assertion fails on the original CSS. A separate focused browser measurement exercised drag-to-fold, reload with the saved fold, and resize from 844 to 620 px across Chromium/WebKit × 390/1280 px × light/dark × both section orders: 16 scenarios passed, first/last workspace rows remained visible, footer stayed within the shell, and no page errors occurred in those focused scenarios. Folded list height was 320 px on phone and 270 px on desktop, matching content height.

Validation

  • PASS bun run check fast with CI-pinned Bun 1.4.2: workflow syntax, generated types/themes, typecheck, build, 12,916 unit tests passed; 50 skipped. The first run on host Bun 1.4.0 failed the unrelated voice upload filename assertion; rerunning on the repository's pinned runtime passed without changing that code or test.
  • PASS bun run check run bun scripts/sidebar-split-demo-regression.ts (Chromium, all 8 combinations).
  • PASS bun run check run bun scripts/sidebar-split-access-demo-regression.ts.
  • PASS bun run check run bun scripts/sidebar-heads-demo-regression.ts.
  • PASS bun run check run bun scripts/sidebar-spacing-demo-regression.ts, including the long-list default half-sidebar allocation.
  • PASS the separate focused 16-scenario collapse/reload/resize measurement described above.
  • EXPECTED FAIL: new WebKit default-height assertion with the original inner flex CSS, before the fix.
  • Not fully green: OVIS_TEST_BROWSER=webkit bun run check run bun scripts/sidebar-split-demo-regression.ts passes the tested layout/fold assertions but its later legacy scenarios intermittently fail the retained no browser errors assertion with ResizeObserver loop completed with undelivered notifications. Diagnostic traces include virtual-chat resize callbacks; fix(sidebar): keep workspace rows visible in Safari #3218 also reports the legacy warning on unchanged main. No error filtering, assertion suppression, or unrelated observer changes are included in this PR.
  • git diff --check passes.
  • Browser checks ran on this Linux host, not the CI runner image (Docker/Podman unavailable). Full integration, native Windows, physical iPhone, and iOS Simulator checks were not run. No deployment or live service restart was performed.

@github-actions

github-actions Bot commented Nov 7, 2026

Copy link
Copy Markdown

Thank you for your contribution! Before we can merge this PR, every commit author needs to sign our Contributor License Agreement (CLA).

To sign the CLA, comment on this PR with:

I have read the CLA Document and I hereby sign the CLA

You sign once, and it covers your later pull requests. Comment recheck to run the check again.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

Explain Change →

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

  • Source mode: Auto
  • Repository: devswha/ovis
  • Branch: main
  • File: .coderabbit.yaml
  • Commit: d5b8333
  • Layers: Repository YAML
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 18ede9d1-8486-4b8e-8413-1545da66af92

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review

No actionable comments were generated in the recent review.

Summary

Summary by CodeRabbit

  • Bug Fixes
    • Fixed a WebKit layout issue so the PC workspace list retains its content height and remains visible when Agents is folded.
    • The list continues to display its rows after reopening Agents, reloading the page, and resizing the viewport.

The PCs sidebar now uses a zero-minimum grid row instead of a flex column. The regression script adds WebKit coverage for PC list height and visibility when Agents is folded, including reload and viewport checks.

Changes: PC list height

Layer / File(s) Summary
Sidebar layout and documented behavior
src/components/SidebarSections.css, DESIGN.md, changelog.d/sidebar-webkit-folded-list-height.md
.pcs-sidebar now uses a single grid row with a zero minimum, and the .machine-list flex sizing rule is removed. The documentation and changelog describe the WebKit behavior and note that outer flex allocation and saved split remain unchanged.
WebKit regression coverage
scripts/sidebar-split-demo-regression.ts
The script can run in Playwright WebKit. It measures PC list layout and checks row visibility and folded-state behavior across reload, reopening Agents, and a narrower viewport.

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

Change: Bug fix

Possibly related issues

  • devswha/ovis#3217: The issue reports WebKit shrinking the PCs list and clipping rows. This PR tests the folded-Agents case, but does not establish a fix for the issue’s expanded-Agents case.

Possibly related PRs

  • devswha/ovis#3218: It targets the same WebKit PCs-list sizing defect and extends the same regression script with WebKit coverage; its flex-basis change is an alternative layout fix.
  • devswha/ovis#2944: It introduced the PCs/Agents split and the regression script that this PR changes.

Suggested reviewers: duytbp, devswha

Merge Risk: Minimal · previous review ee55c

No actionable code risk remains established for this layout fix. Normal checks and the outstanding CLA signature requirement still apply before merging.

Pre-merge checks: Passed 5

Check name Status Explanation
Title check Passed The title names the main change: it fixes sidebar workspace height in WebKit when Agents folds. It matches the raw summary, the PR objectives, and the CSS change to the .pcs-sidebar grid. It is specif…
Description check Passed The description follows the template. It has a Change section that explains the WebKit collapse, the root cause, and the grid fix. It also has a Validation section that lists the checks run with resul…
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
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.
Full details: Title check

Explanation

The title names the main change: it fixes sidebar workspace height in WebKit when Agents folds. It matches the raw summary, the PR objectives, and the CSS change to the .pcs-sidebar grid. It is specific and under 72 characters.

Full details: Description check

Explanation

The description follows the template. It has a Change section that explains the WebKit collapse, the root cause, and the grid fix. It also has a Validation section that lists the checks run with results, the expected failure, the skipped checks, and the one regression command that is not fully green. The template asks for a linked issue for features, but this is a bug fix, so that does not apply. The description also points to the changelog fragment.

Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files. (3 skipped: 3 unsupported.)

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

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

Recent review info

Run configuration

  • Source mode: Auto
  • Repository: devswha/ovis
  • Branch: main
  • File: .coderabbit.yaml
  • Commit: d5b8333
  • Layers: Repository YAML
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 06fc9da7-1de2-4776-b5a0-66202d6438da
Commits

Reviewing files that changed from the base of the PR and between d5b8333 and ee55c7c.

Files selected for processing: 4

  • DESIGN.md
  • changelog.d/sidebar-webkit-folded-list-height.md
  • scripts/sidebar-split-demo-regression.ts
  • src/components/SidebarSections.css

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

@devswha

devswha commented Nov 7, 2026

Copy link
Copy Markdown
Owner

Thanks for the careful fix and the WebKit measurements, @Haeminway1! One small thing before this can merge: the CLA check is still pending. Please sign it by replying with the sentence from the CLA bot comment above. We'll keep reviewing in the meantime and push any follow-up fixes to this branch ourselves.

@devswha

devswha commented Nov 7, 2026

Copy link
Copy Markdown
Owner

Thanks again @Haeminway1. We reviewed this, confirmed the bug on main, and pushed two commits to your branch (no force push):

  • ec15897b merges current main in.
  • 6129020f test(sidebar): CI runs this script in Chromium only, and Chromium shows the rows with either layout, so the new assertions could not catch a revert there. The default case now also checks that .pcs-sidebar computes to display: grid; with main's CSS this now fails in Chromium as well.

Verification in the CI runner image: bun run check fast passes, and all 16 scripts/sidebar-*-regression.ts pass (Chromium). A WebKit comparison of main, this PR and #3218 (many agents expanded, Agents folded, 390/1280, both section orders) shows main at 0-1 visible workspace rows, while both fixes show identical heights. A full OVIS_TEST_BROWSER=webkit run passes every new fold/reload/reopen/620px assertion; it then stops on the existing ResizeObserver loop warning in the older scenarios, as you noted.

This branch has not been deployed

No deployments
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.

2 participants