fix(plugin-basic-ui): Fix scroll behavior of AppScreen #626
Conversation
🦋 Changeset detectedLatest commit: 29e0c21 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
WalkthroughShifts scroll operations from the outer paper container to a new inner content container ref ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant AS as AppScreen
participant Ctx as AppScreen Context
participant IC as Inner Content (paperContentRef)
U->>AS: Click "Top" / call scrollToTop()
AS->>Ctx: invoke context scroll function
Ctx->>IC: scrollTo({ top: 0 })
Note right of IC #DDEBF7: Scroll target changed to inner content element (paperContentRef)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Deploying stackflow-demo with
|
| Latest commit: |
29e0c21
|
| Status: | ⚡️ Build in progress... |
commit: |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
stackflow-docs | 29e0c21 | Commit Preview URL | Aug 22 2025, 02:38 PM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
extensions/plugin-basic-ui/src/components/AppScreen.tsx (5)
78-78: Good ref split; verify paper content is the actual scroll container.Adding a dedicated
paperContentRefis a clean fix to decouple transform/motion onpaperReffrom scroll. Please double-checkcss.paperContentsets scrolling semantics (overflow and sizing), otherwise the new scroll target will be inert on some layouts.If not already defined, consider the following in
AppScreen.css:.paperContent { overflow: auto; /* Ensures the inner container spans the available viewport area */ block-size: 100%; /* Mobile momentum scrolling */ -webkit-overflow-scrolling: touch; /* Prevent scroll chaining during swipe-back */ overscroll-behavior: contain; }
205-208: Respect prefers-reduced-motion and add a safe fallback target.Small UX/accessibility improvement and resilience if
paperContentRefis null or non-scrollable at runtime.Apply this diff:
- paperContentRef.current?.scroll({ - top: 0, - behavior: "smooth", - }); + const $el = paperContentRef.current ?? paperRef.current; + if ($el) { + const prefersReducedMotion = + typeof window !== "undefined" && + window.matchMedia?.("(prefers-reduced-motion: reduce)")?.matches; + $el.scroll({ + top: 0, + behavior: prefersReducedMotion ? "auto" : "smooth", + }); + }
221-225: Mirror reduced-motion handling in the public scroll API and add fallback.Keeps context scrolling behavior consistent with the AppBar handler and avoids no-ops when the inner ref isn’t ready.
Apply this diff:
- paperContentRef?.current?.scroll({ - top, - behavior: "smooth", - }); + const $el = paperContentRef.current ?? paperRef.current; + if ($el) { + const prefersReducedMotion = + typeof window !== "undefined" && + window.matchMedia?.("(prefers-reduced-motion: reduce)")?.matches; + $el.scroll({ + top, + behavior: prefersReducedMotion ? "auto" : "smooth", + }); + }
233-234: Nit: remove stable ref object from deps.
paperContentRefobject identity is stable; including it inuseMemodeps is unnecessary noise.Apply this diff:
- [paperContentRef, zIndexDim, zIndexPaper, zIndexEdge, zIndexAppBar], + [zIndexDim, zIndexPaper, zIndexEdge, zIndexAppBar],
298-304: Expose paper content as a named part and make it optionally focusable.
- Adding
data-partaligns with other elements (paper,edge) and helps testing/diagnostics.- Optional
tabIndex={-1}can help programmatically moving focus into the scroll region when needed (e.g., after navigation).Apply this diff:
- <div - ref={paperContentRef} - className={css.paperContent({ hasAppBar })} - > + <div + ref={paperContentRef} + className={css.paperContent({ hasAppBar })} + data-part="paperContent" + tabIndex={-1} + > {children} </div>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
extensions/plugin-basic-ui/src/components/AppScreen.tsx(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build all packages
- GitHub Check: Create PR or release packages
- GitHub Check: Check the TypeScript typings
- GitHub Check: Check whether the written test passes normally
- GitHub Check: Workers Builds: stackflow-docs
No description provided.