fix(ui): unify Ctrl+O expansion hint experience across buffer modes#21474
fix(ui): unify Ctrl+O expansion hint experience across buffer modes#21474
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the overflow hint mechanism to provide a more consistent and less intrusive user experience across different terminal buffer modes. By centralizing the 'Ctrl+O' expansion hint and intelligently managing inline hints, the system now offers a clearer way for users to interact with truncated content, especially in complex Alternate Screen Buffer environments. The changes streamline the overflow detection logic, making it more robust and easier to maintain. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully unifies the Ctrl+O expansion hint experience by centralizing overflow detection and display. The changes correctly delegate overflow reporting from ToolGroupMessage to child components like Scrollable, which now has an opt-in mechanism to report overflow to a global context. This simplifies the message components and provides a consistent user experience in both standard and alternate screen buffer modes. The tests have been updated accordingly, but one test file contains an incomplete test case that should be fixed to ensure full coverage of the new functionality and adhere to testing best practices, including proper cleanup when using renderWithProviders.
Note: Security Review did not run due to the size of the PR.
packages/cli/src/ui/components/messages/ToolOverflowConsistencyChecks.test.tsx
Show resolved
Hide resolved
|
Size Change: -2.48 kB (-0.01%) Total Size: 26 MB
ℹ️ View Unchanged
|
jwhelangoog
force-pushed
the
feature/consolidate-overflow-hints
branch
from
bd4ac98 to
31c9505
Compare
gemini-cli
bot
added
the
area/core
jwhelangoog
force-pushed
the
feature/consolidate-overflow-hints
branch
2 times, most recently
from
953c09e to
aea6305
Compare
packages/cli/src/ui/components/messages/ToolOverflowConsistencyChecks.test.tsx
Outdated
Show resolved
Hide resolved
| ) : ( | ||
| content | ||
| ); | ||
| return <OverflowProvider>{content}</OverflowProvider>; |
There was a problem hiding this comment.
shouldn't we be just returning content rather than always wrapping in an overflow provider?
There was a problem hiding this comment.
For FolderTrustDialog.tsx and the ToolConfirmationQueue.tsx, the wrapper helps prevent any overflowing elements w/in these drawers/dialogs from triggering the global hint. At least initially, it allows just their local ShowMoreLine components to render. Without the wrappers we get something like this when they have overflow: https://screenshot.googleplex.com/37BxeV57LD29Mds.png
There was a problem hiding this comment.
ok fine for now. please cleanup in a followup so we show the overflow hint with an appropriate message in the main area to keep this simple.
|
|
||
| import type React from 'react'; | ||
| import { useState, useRef, useCallback, useMemo, useLayoutEffect } from 'react'; | ||
| import { |
There was a problem hiding this comment.
what are these changes to scrollable trying to do? this all seems wrong and error prone and can hopefully just be removed.
There was a problem hiding this comment.
three hooks being added: useEffect, useId, and useOverflowAction to let Scrollable self-report that it has overflowing content; but only when reportOverflow is true. The flag gives the option for scrollables like the main output history to withhold 'overflow' signaling, so it doesn't trigger the global Ctrl+O hint.
| ) : ( | ||
| content | ||
| ); | ||
| return <OverflowProvider>{content}</OverflowProvider>; |
There was a problem hiding this comment.
why is this defaulting to the alternate buffer version rather than the regular version? these changes should be making alternate buffer mode more like regular mode as far as this goes not vice vesa.
jacob314
left a comment
jacob314
left a comment
There was a problem hiding this comment.
Approved after these comments are addressed. thanks for the cleanup! this makes the experience a lot cleaner in alternate buffer mode.
- Show Ctrl+O overflow hint in the composer toast display when overflow components exist in the output history (hint no longer appears in output history stream) - Continue using isolated inline hints for contextual UI (such as the Tool Confirmation views) - Opt-in global overflow reporting for ASB scrollable components
- Update AppContainer tests for asynchronous overflow reporting in ASB mode - Update MainContent snapshots reflecting removal of inline hints - Clean up obsolete tool result display overflow tests
jwhelangoog
force-pushed
the
feature/consolidate-overflow-hints
branch
from
aea6305 to
92c7bb6
Compare
- ShowMoreLine.tsx: relevant changes only - ToolOverflowConsistencyChecks.test: remove unncessary mock, add waitUntilReady() calls to ensure initial renders finish (avoid flakiness)
jwhelangoog
force-pushed
the
feature/consolidate-overflow-hints
branch
from
7f9b6b7 to
415e9e3
Compare
| ) : ( | ||
| content | ||
| ); | ||
| return <OverflowProvider>{content}</OverflowProvider>; |
There was a problem hiding this comment.
ok fine for now. please cleanup in a followup so we show the overflow hint with an appropriate message in the main area to keep this simple.
2 participants
Summary
This PR unifies the
Ctrl+Oexpansion hint experience between the Standard Terminal and Alternate Screen Buffer (ASB) modes.ShowMoreLinescomp) are maintained for contextual UI components (such as Tool Confirmation views) in both modes.Details
hasOverflowcalculations,useEffecthooks, and the localShowMoreLinescomponent rendering fromToolGroupMessage.tsx.ToolResultDisplay->Scrollable/MaxSizedBox), allowing overflow events to bubble up to the globalAppContainer.reportOverflowprop toScrollable.tsxto handle opt-in global overflow reporting viauseOverflowActions.reportOverflow={true}to theScrollablecomponent instance inToolResultDisplay.tsx.AppContainer.test.tsxto usewaitForto handle React's asynchronous state flush in ASB mode correctly.MainContent.test.tsxsnapshots to reflect the intended removal of the localShowMoreLinescomponent.Related Issues
Fixes #20455
How to Validate
npm run start.vimor setting the flag in config).lsor script output). Verify that the centralized "Ctrl+O" Toast hint appears.Pre-Merge Checklist