fix(core): preserve shell execution config fields - #25130
Conversation
…ogle-gemini#24820) - Import uiTelemetryService and call hydrate when resuming via --resume - This ensures the session ID and metrics are correctly restored, so the quit summary shows the correct resume command Fixes google-gemini#24820
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 addresses an issue where specific shell execution configuration fields were being inadvertently dropped when the UI refreshed layout-related values. By updating the configuration setter to properly merge existing state and ensuring telemetry services are correctly hydrated during session resumption, the changes maintain runtime consistency for sandbox and background-completion settings. 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces telemetry hydration when resuming a session and updates the Config class to ensure that shell execution configuration fields are correctly preserved during updates. A review comment suggests simplifying the setShellExecutionConfig implementation by using a standard spread-based merge instead of the current redundant approach of spreading followed by explicit field assignments for every property.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/core/src/config/config.ts (3309-3341)
The setShellExecutionConfig method should consistently use a merge operation (e.g., spread syntax {...a, ...b}) instead of explicit field assignment. Explicitly listing every field after a spread is redundant and can lead to accidental overwrites or order-dependent logic when new fields are added to the configuration.
References
- When constructing an object from multiple data sources, consistently use a merge operation (e.g., spread syntax) instead of assignment to avoid accidental overwrites and order-dependent logic.
|
We already have #25113 |
|
Thank you for the guidance. I see that #25113 already addresses this by properly preserving shell config fields. I am closing this PR to reduce backlog and focus on active feedback. Appreciate the review! |
Fixes #25112
Summary
Preserve existing shell execution config fields when the UI refreshes layout-related values, so sandbox and background-completion settings are not dropped at runtime.
Validation
npm test packages/core/src/config/config.test.ts