Fix GraphQL Schema Builder checkbox state persistence issue when toggling panel position between bottom and side, and improve horizontal scrolling UX. #3920
Conversation
… toggling panel position between bottom and side, and improve horizontal scrolling UX.
WalkthroughThis PR modifies three files to address layout and component state preservation issues. In Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
![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 (2)
app/src/componentsV2/BottomSheet/components/BottomSheetLayout/components/SplitPaneLayout/SplitPaneLayout.tsx (1)
28-35: Direction-change effect looks correct; consider tightening dependencies / duplicationThe new effect correctly re-applies split sizes when
splitDirection/ placement changes, which should avoid remounts and preserve child state as intended. A couple of small cleanups you might consider:
- The logic in this effect duplicates the mount‑only effect above. You could unify into a single effect that depends on
isSheetPlacedAtBottom(and optionallyinitialSizes) so the sizing logic lives in one place.splitDirectionis derived fromisSheetPlacedAtBottom, so including both in the dependency array is redundant; depending onisSheetPlacedAtBottom(andinitialSizesif you want to react to size changes) is sufficient.- Since
initialSizesis an array prop, if callers pass a new array literal each render, this effect will run and callsetSizesevery render. That’s harmless but slightly noisy; if that’s a concern, you could either document thatinitialSizesshould be memoized or drop it from deps and only recompute on placement changes.Behaviorally this is fine; the above is just to keep the effect minimal and avoid unnecessary
setSizescalls.app/src/features/apiClient/screens/apiClient/components/views/graphql/components/SchemaBuilder/schemaBuilder.scss (1)
82-89: Hidden horizontal scrollbar UX/accessibility and overflow change look reasonable; verify cross‑platform behaviorThe combination of:
overflow-x: auto; overflow-y: hidden;with scrollbar styles that fully hide the horizontal scrollbar on the variable editor title bar, and- Removing
overflow: hidden !importantfrom the second child under.graphiql-explorer-rootshould improve horizontal scroll space and prevent content clipping, while keeping visuals clean.
Two minor points to double‑check:
- Discoverability & keyboard access – With scrollbars fully hidden, ensure users can still reliably discover and use horizontal scrolling (e.g., via trackpad, mouse wheel + Shift, keyboard) and that focus/keyboard navigation doesn’t get cut off for long variable names on common OS/browser combos (Chrome/Firefox/Edge on macOS/Windows).
- Layout in constrained heights – With the
overflow: hiddenremoved, verify there’s no unexpected vertical overflow or double‑scrolling inside the GraphQL explorer when nested in the bottom sheet with minimal height.The added
padding: 0 12pxon.toolbar-buttonaligns with the rest of the spacing and looks good.Also applies to: 111-111, 220-221
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/componentsV2/BottomSheet/components/BottomSheetLayout/components/SplitPaneLayout/SplitPaneLayout.tsx(1 hunks)app/src/features/apiClient/screens/apiClient/apiClient.scss(1 hunks)app/src/features/apiClient/screens/apiClient/components/views/graphql/components/SchemaBuilder/schemaBuilder.scss(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: nafees87n
Repo: requestly/requestly PR: 3655
File: app/src/features/apiClient/screens/apiClient/components/views/components/Collection/components/CollectionRunnerView/components/RunResultView/RunResultContainer/RunResultContainer.tsx:426-426
Timestamp: 2025-10-10T11:08:23.369Z
Learning: In the file `app/src/features/apiClient/screens/apiClient/components/views/components/Collection/components/CollectionRunnerView/components/RunResultView/RunResultContainer/RunResultContainer.tsx`, the `destroyInactiveTabPane={true}` setting is intentional. The loss of state (expandedKeys, scroll position) when switching tabs is acceptable for now and will be addressed in future work.
📚 Learning: 2025-10-10T11:08:23.369Z
Learnt from: nafees87n
Repo: requestly/requestly PR: 3655
File: app/src/features/apiClient/screens/apiClient/components/views/components/Collection/components/CollectionRunnerView/components/RunResultView/RunResultContainer/RunResultContainer.tsx:426-426
Timestamp: 2025-10-10T11:08:23.369Z
Learning: In the file `app/src/features/apiClient/screens/apiClient/components/views/components/Collection/components/CollectionRunnerView/components/RunResultView/RunResultContainer/RunResultContainer.tsx`, the `destroyInactiveTabPane={true}` setting is intentional. The loss of state (expandedKeys, scroll position) when switching tabs is acceptable for now and will be addressed in future work.
Applied to files:
app/src/componentsV2/BottomSheet/components/BottomSheetLayout/components/SplitPaneLayout/SplitPaneLayout.tsx
🔇 Additional comments (1)
app/src/features/apiClient/screens/apiClient/apiClient.scss (1)
74-85: Confirm that switching toheight: 100%doesn’t regress vertical layout in edge casesChanging
.api-client-bodyfrom a fixed offset (calc(100% - 43px)) toheight: 100%withflex: autoshould generally play nicer with flex layouts, but it also changes how space is distributed when headers/footers or bottom sheets are present. Please sanity‑check:
- Small viewport heights (e.g., short laptop screens).
- Cases where the bottom sheet is open and the GraphQL view is active.
Specifically verify that the tabs/content area doesn’t get clipped or overflow unexpectedly in those states.
1 participant
Closes issue: #3823
Summary of changes:
Root Cause:
The
SplitPaneLayoutcomponent was usingkey={splitDirection}on theSplitcomponent, causing React to remount the entire component tree (includingSchemaBuilder) when the panel position changed. This resulted in loss of internal state maintained bygraphiql-explorer'sExplorercomponent.Solution:
keyprop from theSplitcomponent to prevent unnecessary remountinguseEffecthook to handle direction changes dynamically while preserving component state🎥 Demo Video:
requestly_overflow_issue.mp4
Files Changed:
app/src/componentsV2/BottomSheet/components/BottomSheetLayout/components/SplitPaneLayout/SplitPaneLayout.tsxkey={splitDirection}prop to prevent remountinguseEffectfor dynamic direction handlingapp/src/features/apiClient/screens/apiClient/apiClient.scss.api-client-bodyheight fromcalc(100% - 43px)toheight: 100%for better flexbox handlingapp/src/features/apiClient/screens/apiClient/components/views/graphql/components/SchemaBuilder/schemaBuilder.scssSummary by CodeRabbit
Release Notes
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.