feat(agents): sticky header + compact model params layout#8
Closed
bashrusakh wants to merge 8 commits into
Closed
Conversation
Exported package-manager helpers used by openchamber update Added regression coverage for the update-available path
Gate passive auto-follow on active (working/settling) state so idle layout churn from virtualizer re-measurement no longer re-pins the viewport to the bottom. Render default-open tool bodies synchronously on mount so the virtualizer measures the real row height up front instead of growing a frame later and lurching scroll past several messages.
Give touch surfaces a larger, viewport-relative head start for loading older history so an in-flight fetch completes before the finger reaches the top. Raise the mobile virtualizer overscan so fast flings stay populated instead of leaving blank gaps, and drop the fixed itemSize hint so virtua auto-estimates row heights from measured sizes instead of a flat constant.
…ts (openchamber#1791)" This reverts commit 30c8250.
Redesign the mobile composer model and agent buttons as borderless, full-bleed labels that hug their content, truncate with an ellipsis when space is tight, and show the provider logo inline before the model name. Tighten the footer action buttons (sessions / attach / auto-accept) so they sit close together, with a small left inset on the group. In the mobile model selection overlay, make the thinking-variant control text-only with a chevron, vertically center the variant and favorite controls in each row, and place the provider logo inline with the model name.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughVersion 1.13.7 release comprising: active-session gating in ChangesChat Scroll Stability and Mobile UI
Add-Provider Sentinel Preservation
AgentsPage Layout and i18n
CLI Update Command Export
Version Bump and Changelogs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 🔧 OpenGrep (1.23.0)packages/ui/src/lib/i18n/messages/uk.settings.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.18][ERROR]: unable to find a config; path packages/ui/src/lib/i18n/messages/zh-CN.settings.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.16][ERROR]: unable to find a config; path packages/ui/src/lib/i18n/messages/zh-TW.settings.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.16][ERROR]: unable to find a config; path Comment |
- Sticky header with agent name and always-visible Save button - Compact Model & Parameters: 2 rows (Model+Variant, Sampling+TopP) - Shortened variant label to 'Thinking' in all locales - Added missing i18n keys for all locales - Restored data-settings-item attrs for settings search - Removed unused isMobile/useDeviceInfo
bashrusakh
force-pushed
the
issue-1702-wise-narwhal
branch
from
7b22220 to
04dfa5b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/server/lib/package-manager.js (1)
772-783: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winWhitelist
pmbefore building the shell command.resolvePackageManagerCommand()falls back to the original value, andquoteCommand()leaves non-whitespace strings unchanged, so an unexpectedpmcan still reachspawnSync(..., shell: true). Reject unknown package managers here or switch to an executable + args form.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/web/server/lib/package-manager.js` around lines 772 - 783, executeUpdate currently builds a shell command from pm and passes it to spawnSync with shell enabled, so an unexpected package manager value can still be executed. Add validation in executeUpdate (or before getUpdateCommand/resolvePackageManagerCommand is used) to allow only known package managers, or refactor the spawnSync call to use an executable plus args instead of a shell string. Keep the fix localized around executeUpdate, getUpdateCommand, and resolvePackageManagerCommand/quoteCommand so invalid pm values cannot reach the shell.Source: Linters/SAST tools
🧹 Nitpick comments (1)
packages/ui/src/stores/useConfigStore.ts (1)
34-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare one sentinel constant across UI, store, and tests.
The store, providers page, model controls, and tests all rely on the exact
"__add_provider__"value. Since persistence now treats it specially, export/import a single constant to avoid silent drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/stores/useConfigStore.ts` around lines 34 - 35, The special add-provider sentinel is duplicated as the exact "__add_provider__" string across useConfigStore, the providers page, model controls, and tests, so introduce and export a single shared constant for this value and update all call sites to use it. Make sure the store’s persistence logic, any UI selection checks, and the related tests reference the same constant so the sentinel cannot drift silently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/src/lib/i18n/messages/es.settings.ts`:
- Line 393: The localized label for settings.agents.page.field.variant in
es.settings.ts no longer matches the updated English meaning for “Thinking.”
Update the Spanish translation in the messages map to the equivalent of
“Thinking” (for example, “Pensamiento”) so it stays consistent with the other
locales and the new English wording.
In `@packages/ui/src/lib/i18n/messages/fr.settings.ts`:
- Line 382: The localized label for settings.agents.page.field.variant in the
French settings messages is still using an outdated translation that no longer
matches the updated English wording. Update the value in fr.settings.ts from the
current variant text to the French equivalent of “Thinking,” and keep it aligned
with the existing variantTooltip terminology in the same locale file so the UI
wording stays consistent across languages.
In `@packages/ui/src/stores/useConfigStore.ts`:
- Around line 197-199: The helper preserveAddProviderSelection is only used in
one place, but the loadProviders failure fallback still overwrites
parsed.providerId directly and can close the add-provider flow after a failed
refresh. Update the remaining automatic overwrite path in loadProviders so that
settingsDefaultModel is applied through preserveAddProviderSelection, keeping
ADD_PROVIDER_SENTINEL when the current selection is the add-provider state and
only replacing it otherwise.
In `@packages/ui/src/styles/mobile.css`:
- Around line 88-96: The .composer-mobile-actions button rule is overriding the
mobile touch-target minimum and shrinking the footer action buttons too far.
Update the mobile.css styles so the session / attach / auto-accept buttons keep
the 36px minimum touch size defined by the global mobile button rule, and avoid
using !important or explicit widths that bypass that floor. Use the
composer-mobile-actions selector to preserve spacing, but let the buttons remain
at the mobile minimum tap target.
---
Outside diff comments:
In `@packages/web/server/lib/package-manager.js`:
- Around line 772-783: executeUpdate currently builds a shell command from pm
and passes it to spawnSync with shell enabled, so an unexpected package manager
value can still be executed. Add validation in executeUpdate (or before
getUpdateCommand/resolvePackageManagerCommand is used) to allow only known
package managers, or refactor the spawnSync call to use an executable plus args
instead of a shell string. Keep the fix localized around executeUpdate,
getUpdateCommand, and resolvePackageManagerCommand/quoteCommand so invalid pm
values cannot reach the shell.
---
Nitpick comments:
In `@packages/ui/src/stores/useConfigStore.ts`:
- Around line 34-35: The special add-provider sentinel is duplicated as the
exact "__add_provider__" string across useConfigStore, the providers page, model
controls, and tests, so introduce and export a single shared constant for this
value and update all call sites to use it. Make sure the store’s persistence
logic, any UI selection checks, and the related tests reference the same
constant so the sentinel cannot drift silently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9dd5c8c9-ba6d-4066-ab7b-0d768cb0048e
📒 Files selected for processing (33)
.github/workflows/release-desktop-smoke.ymlCHANGELOG.mdpackage.jsonpackages/electron/package.jsonpackages/ui/package.jsonpackages/ui/src/components/chat/ChatInput.tsxpackages/ui/src/components/chat/MessageList.tsxpackages/ui/src/components/chat/MobileAgentButton.tsxpackages/ui/src/components/chat/MobileModelButton.tsxpackages/ui/src/components/chat/ModelControls.tsxpackages/ui/src/components/chat/hooks/useChatTimelineController.tspackages/ui/src/components/chat/message/parts/ToolPart.tsxpackages/ui/src/components/sections/agents/AgentsPage.tsxpackages/ui/src/hooks/useChatAutoFollow.tspackages/ui/src/lib/i18n/messages/en.settings.tspackages/ui/src/lib/i18n/messages/es.settings.tspackages/ui/src/lib/i18n/messages/fr.settings.tspackages/ui/src/lib/i18n/messages/ja.settings.tspackages/ui/src/lib/i18n/messages/ko.settings.tspackages/ui/src/lib/i18n/messages/pl.settings.tspackages/ui/src/lib/i18n/messages/pt-BR.settings.tspackages/ui/src/lib/i18n/messages/uk.settings.tspackages/ui/src/lib/i18n/messages/zh-CN.settings.tspackages/ui/src/lib/i18n/messages/zh-TW.settings.tspackages/ui/src/stores/useConfigStore.test.tspackages/ui/src/stores/useConfigStore.tspackages/ui/src/styles/mobile.csspackages/vscode/CHANGELOG.mdpackages/vscode/package.jsonpackages/web/bin/lib/commands-update.test.jspackages/web/package.jsonpackages/web/server/lib/package-manager.jspackages/web/server/lib/package-manager.test.js
| "settings.agents.page.field.overrideModel": "Sobrescribir modelo", | ||
| "settings.agents.page.field.variant": "Variante de pensamiento", | ||
| "settings.agents.page.field.sampling": "Muestreo", | ||
| "settings.agents.page.field.variant": "Variante", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
variant label does not match the new English "Thinking" wording.
English changed this label to 'Thinking' (and pl/zh-TW were updated to the equivalent "Thinking"). The Spanish value here is 'Variante' ("Variant"), so the displayed label semantics diverge across locales. Per the commit notes ("some locale labels were corrected to match the English 'Thinking' label"), this likely should be something like 'Pensamiento'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/lib/i18n/messages/es.settings.ts` at line 393, The localized
label for settings.agents.page.field.variant in es.settings.ts no longer matches
the updated English meaning for “Thinking.” Update the Spanish translation in
the messages map to the equivalent of “Thinking” (for example, “Pensamiento”) so
it stays consistent with the other locales and the new English wording.
| 'settings.agents.page.field.overrideModel': 'Remplacer le modèle', | ||
| 'settings.agents.page.field.variant': 'Variante de réflexion', | ||
| 'settings.agents.page.field.sampling': 'Échantillonnage', | ||
| 'settings.agents.page.field.variant': 'Variante', |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
variant label diverges from the new English "Thinking" wording.
English is now 'Thinking' and pl/zh-TW were aligned accordingly, but this French value is 'Variante' ("Variant"). To keep the label consistent across locales, consider 'Réflexion' (matching the existing variantTooltip which already refers to "raisonnement/réflexion").
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/lib/i18n/messages/fr.settings.ts` at line 382, The localized
label for settings.agents.page.field.variant in the French settings messages is
still using an outdated translation that no longer matches the updated English
wording. Update the value in fr.settings.ts from the current variant text to the
French equivalent of “Thinking,” and keep it aligned with the existing
variantTooltip terminology in the same locale file so the UI wording stays
consistent across languages.
Comment on lines
+197
to
+199
| const preserveAddProviderSelection = (currentSelectedProviderId: string | undefined, nextProviderId: string): string => ( | ||
| currentSelectedProviderId === ADD_PROVIDER_SENTINEL ? ADD_PROVIDER_SENTINEL : nextProviderId | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use this helper in the remaining automatic overwrite path.
Line 1702 and Line 1707 still assign parsed.providerId directly in the loadProviders failure fallback, so a failed background refresh can still close the add-provider flow when it applies settingsDefaultModel.
Proposed fix
- nextState.selectedProviderId = parsed.providerId;
+ const nextSelectedProviderId = preserveAddProviderSelection(
+ state.selectedProviderId,
+ parsed.providerId,
+ );
+ nextState.selectedProviderId = nextSelectedProviderId;
nextSnapshot.currentProviderId = parsed.providerId;
nextSnapshot.currentModelId = parsed.modelId;
nextSnapshot.currentVariant = currentVariant;
- nextSnapshot.selectedProviderId = parsed.providerId;
+ nextSnapshot.selectedProviderId = nextSelectedProviderId;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const preserveAddProviderSelection = (currentSelectedProviderId: string | undefined, nextProviderId: string): string => ( | |
| currentSelectedProviderId === ADD_PROVIDER_SENTINEL ? ADD_PROVIDER_SENTINEL : nextProviderId | |
| ); | |
| const nextSelectedProviderId = preserveAddProviderSelection( | |
| state.selectedProviderId, | |
| parsed.providerId, | |
| ); | |
| nextState.selectedProviderId = nextSelectedProviderId; | |
| nextSnapshot.currentProviderId = parsed.providerId; | |
| nextSnapshot.currentModelId = parsed.modelId; | |
| nextSnapshot.currentVariant = currentVariant; | |
| nextSnapshot.selectedProviderId = nextSelectedProviderId; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/stores/useConfigStore.ts` around lines 197 - 199, The helper
preserveAddProviderSelection is only used in one place, but the loadProviders
failure fallback still overwrites parsed.providerId directly and can close the
add-provider flow after a failed refresh. Update the remaining automatic
overwrite path in loadProviders so that settingsDefaultModel is applied through
preserveAddProviderSelection, keeping ADD_PROVIDER_SENTINEL when the current
selection is the add-provider state and only replacing it otherwise.
Comment on lines
+88
to
+96
| /* Composer footer action buttons (sessions / attach / auto-accept): hug the | ||
| icon so the group stays tight. The container only renders in the mobile | ||
| JSX, so no pointer/runtime gating is needed; !important overrides both the | ||
| w-8 utility and the global mobile touch-target min-width regardless of | ||
| cascade layer or specificity. */ | ||
| .composer-mobile-actions button { | ||
| width: 1.5rem !important; | ||
| min-width: 0 !important; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the footer action tap targets at the mobile minimum.
Lines 94-95 shrink every button in this group to 24px wide and explicitly bypass the 36px mobile floor defined a few lines above. That makes the session / attach / auto-accept actions materially harder to hit on touch screens.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/styles/mobile.css` around lines 88 - 96, The
.composer-mobile-actions button rule is overriding the mobile touch-target
minimum and shrinking the footer action buttons too far. Update the mobile.css
styles so the session / attach / auto-accept buttons keep the 36px minimum touch
size defined by the global mobile button rule, and avoid using !important or
explicit widths that bypass that floor. Use the composer-mobile-actions selector
to preserve spacing, but let the buttons remain at the mobile minimum tap
target.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Agent settings editor layout redesign — phase 1. Closes openchamber#1702.
Changes
data-settings-itemattributes for variant/temperature/top-pisMobile/useDeviceInfoFiles changed
packages/ui/src/components/sections/agents/AgentsPage.tsxpackages/ui/src/lib/i18n/messages/*.settings.tsValidation
bun run type-checkinpackages/ui— passesbun run build— passesSummary by CodeRabbit
New Features
Bug Fixes