Skip to content

fix(desktop): let the whole Settings pane scroll - #3453

Merged
Astro-Han merged 1 commit into
apache:mainfrom
chihumyum:fix/settings-page-scroll-area
Aug 22, 2026
Merged

fix(desktop): let the whole Settings pane scroll#3453
Astro-Han merged 1 commit into
apache:mainfrom
chihumyum:fix/settings-page-scroll-area

Conversation

@chihumyum

@chihumyum chihumyum commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Move vertical scroll ownership from the centered Settings content column to the whole rounded main pane.
  • Let wheel input over either wide gutter scroll the page and keep the native scrollbar at the pane's outer edge.
  • Preserve the existing 920px content width and mirror the production scroll structure in Storybook.
  • Add Electron E2E coverage for wide-gutter wheel scrolling and scroll ownership.

Before / after

before.mp4
after.mp4

Draft status

The current three-way merge against #3450 auto-merges the production TSX/CSS changes. Only apps/desktop/e2e/settings.spec.ts conflicts because both branches append a test at the end of the file; resolving it means keeping both tests.

Verification

  • npx biome check apps/desktop/e2e/settings.spec.ts apps/desktop/src/renderer/settings/settings-surface.tsx apps/desktop/src/renderer/styles/settings/nav-sidebar.css apps/desktop/stories/settings/provider-settings.stories.tsx
  • npm --workspace @maka/desktop run typecheck
  • npm --workspace @maka/desktop run e2e -- e2e/settings.spec.ts — 3/3 passed
  • npm --workspace @maka/desktop run build-storybook
  • npm --workspace @maka/desktop run smoke:storybook — 160/160 stories passed
  • git diff --check

Manual/native visual acceptance and screenshots were intentionally skipped for this draft.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope:

OpenAI Codex — investigated the scroll owner, implemented the renderer/CSS/Storybook changes, added Electron E2E coverage, verified the branch, checked overlap with #3450, and prepared this Draft PR. The human contributor reviewed the intended behavior and authorized publication.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

Move vertical scroll ownership from the centered content column to the rounded Settings main pane so wide gutters accept wheel input and the native scrollbar stays at the pane edge. Keep the page column width and mirror the production layout in Storybook.

Generated-by: Codex
@chihumyum
chihumyum marked this pull request as ready for review August 21, 2026 19:12

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Automated review of exact head b69a89b8a08ba5bd848ae97270d753d808195fff against current main@d62857a8357e9160926726a2a13096bc2dc2b91d.

The defect remains on main: .settingsMainPane clips overflow while Astryx LayoutContent owns overflow: auto, so the centered 920px column is the only scroll target and wide pane gutters do not receive wheel scrolling. This patch moves the single scroll owner to the rounded pane, opts the inner content out of scrolling, and keeps the Storybook frame structurally aligned. The E2E asserts both ownership and actual gutter-wheel behavior. I found no actionable P0-P2 defect; git diff --check is clean.

Required conclusions:

  1. Optimal for the actual problem: yes; one outer scroll owner fixes both inert gutters and scrollbar placement without adding event forwarding.
  2. Production code to delete: none beyond the replaced inner scroll ownership; none further identified.
  3. Tests to delete/replace: none identified.
  4. Deeper refactor: no.
  5. Ready to merge: not yet. No hosted test check is reported on this exact head, and automated review is not approval.
  6. Residual risks/gaps: the scroll-owner change is user-visible and still needs the promised human visual/native acceptance; the PR body also still describes itself as Draft even though GitHub currently marks it ready for review, and it records a pending rebase/combined-test resolution after #3450.

This affects user-visible layout, so CONTRIBUTING.md requires independent human judgment on the exact head.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, Clean and small fix.

@Astro-Han
Astro-Han merged commit f1f4e71 into apache:main Aug 22, 2026
1 check passed
@chihumyum
chihumyum deleted the fix/settings-page-scroll-area branch August 22, 2026 06:07
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.

3 participants