Revert "fix(ui): improve narration suppression and reduce flicker (#2…#24857
Revert "fix(ui): improve narration suppression and reduce flicker (#2…#24857gundermanc merged 1 commit intomainfrom
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 reverts a previous change that introduced complex narration suppression logic. The original implementation was intended to reduce UI flicker and improve narration, but it inadvertently caused valid messages to be hidden even when the narration feature was disabled. This revert restores the previous behavior to ensure message visibility. Highlights
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. Footnotes
|
|
Size Change: -529 B (0%) Total Size: 34 MB
ℹ️ View Unchanged
|
![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 simplifies the narration suppression logic in the MainContent component by reducing the number of rules and state variables used during history iteration. However, the new implementation introduces a bug where the toolGroupInTurn flag is not 'sticky' within a turn, potentially failing to suppress Gemini messages if multiple tool groups exist. Additionally, the PR removes the entire 'Narration Suppression' test suite, leaving the remaining logic without test coverage. It is recommended to fix the flag logic and retain/adapt the relevant tests.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/cli/src/ui/components/MainContent.tsx (100)
The logic for toolGroupInTurn is not sticky within a turn. Since the loop iterates backwards through history, if a turn contains multiple tool groups and a non-topic tool group appears after a topic tool group (chronologically), the toolGroupInTurn flag will be reset to false. This prevents preceding Gemini messages from being suppressed as intended. Making it sticky ensures consistent behavior. Additionally, please include a detailed comment explaining this logic to prevent incorrect refactoring, as it affects conditional rendering.
// Ensure toolGroupInTurn is sticky to maintain consistent narration suppression logic
toolGroupInTurn = toolGroupInTurn || item.tools.some((t) => isTopicTool(t.name));
References
- For complex layout calculations that depend on component rendering logic, add detailed comments explaining the logic to prevent incorrect refactoring.
packages/cli/src/ui/components/MainContent.test.tsx (735-885)
This PR deletes the entire "Narration Suppression" test suite. While some of the logic being reverted is removed from MainContent.tsx, the core logic for suppressing narration before topic tools is still present in the restored code. Deleting these tests entirely leaves the remaining feature without coverage. It is recommended to retain and adapt the tests that verify suppression. When adapting tests, ensure that if renderWithProviders is used, the unmount function is called at the end of the test.
References
- When using renderWithProviders in tests, the returned unmount function must be called at the end of the test to ensure proper cleanup and prevent resource leaks.
gemini-cli
bot
added
the
status/need-issue
2 participants
Reverts #24635 which appears to have caused some issues with disappearing messages even when the narration flag is disabled.