Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 370783eac1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export interface SettingDefinition { | ||
| key: string; | ||
| /** Conditions that must all hold, or this parameter does nothing and is not shown. */ | ||
| dependsOn?: SettingDependency[]; |
There was a problem hiding this comment.
When an operator declares dependsOn in customParams.paramDefinitions, endpointSchema parses the entry through paramDefinitionSchema, which does not include this new property, so Zod strips it before the configuration reaches the client. Consequently, the advertised dependency support silently works only for built-in definitions; add a corresponding nested field to paramDefinitionSchema so it can be configured in librechat.yaml.
AGENTS.md reference: AGENTS.md:L66-L68
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d9d6e16: the dependsOn rule is removed from SettingDefinition and every definition, so nothing is advertised that paramDefinitionSchema cannot carry. The panel no longer hides parameters by a companion toggle.
| }, | ||
| reasoning_summary: { | ||
| key: 'reasoning_summary', | ||
| dependsOn: [{ key: 'useResponsesApi', equals: true }], |
There was a problem hiding this comment.
For a custom OpenAI-compatible endpoint configured with reasoningFormat: reasoningObject, applyReasoningConfig deliberately sends reasoning_summary, reasoning_mode, and reasoning_context through modelKwargs.reasoning without requiring useResponsesApi; these unconditional dependencies now hide controls that previously worked unless the user enables a transport mode the gateway may not support. Availability needs to account for the endpoint's reasoning format rather than treating useResponsesApi as the sole capability signal.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
danny-avila
added
the
🗺️ Chat UI Shell
370783e to
d29830e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d29830ee35
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * dropped before grouping, so a section that has nothing live disappears with it | ||
| * rather than standing empty. */ | ||
| const sections = useMemo( |
There was a problem hiding this comment.
When a parameter cannot render for reasons outside dependsOn, it still creates a visible section here. For example, Bedrock's region definition has no options of its own, availableRegions is optional, and DynamicCombobox returns null when that list is empty; because grouping happens first and the component is later placed inside an unconditional wrapper, such deployments now display an empty “Advanced” heading and blank grid cell. Filter these controls before grouping (or otherwise suppress a section when all of its renderers return nothing).
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d9d6e16: region options are filled in before grouping and a combobox or dropdown with no choices is dropped there (hasControl), so no section is built around a control that renders nothing. Covered by groups.spec.
| </section> | ||
| ); | ||
| })} | ||
| <div className="mt-5 flex gap-2"> |
There was a problem hiding this comment.
In locales with a long com_endpoint_save_as_preset translation, putting both actions in this single narrow flex row causes horizontal overflow. The shared Button primitive applies whitespace-nowrap, and these flex items retain their min-content widths; for example, the Spanish “Guardar como configuración preestablecida” label cannot fit alongside Reset in the roughly 300 px parameter sidebar. Keep the actions stacked at constrained widths or otherwise provide a bounded localized-label treatment.
Useful? React with 👍 / 👎.
The panel rendered a provider's parameter list in the order the definitions happen to arrive in, so it asked about temperature, then reasoning, then whether to resend files, with nothing to say which belonged together. The keys are filed under six headings now, with a count beside each of what this conversation has changed inside it, which is what the owner scans for before reaching for Reset. A key this map has never heard of, which is what a deployment's own customParams look like, keeps its place in the last section instead of being guessed at or dropped. Every parameter stays on screen. A settings panel is read as much as it is edited, and a disclosure trades seeing the current state at a glance for vertical space that costs nothing to find elsewhere: the headings are captions rather than buttons, a control is measured against the row it sits in rather than padded to a fixed block, and only free text takes the full width. A number, a toggle or a choice says the same thing in half of a 300px panel, so two sit side by side and the narrow ones in a section come first, leaving one ragged edge at the bottom instead of a gap beside every full-width field. Reset and Save As Preset share the closing row. Fifteen parameters occupy 954px where the same fifteen took 1243px with four of the six sections shut. Grouping alone would still show controls that do nothing. A definition can now declare what it rides along with: OpenAI's reasoning summary, mode and context travel inside the reasoning object the Responses API takes and are dropped by the server without it; a thinking budget is read only while thinking is enabled; a cache lifetime only while the cache is being written. Those five declare it, the panel judges each one on the companion's effective value, and a section left with nothing live disappears rather than standing empty. The rule lives on the definition rather than in the panel so a deployment defining its own parameters can state one too. The custom instructions field caps its own rows. It is sized by a shadow copy of itself, measured as the panel mounts and before the sidebar has settled on a width, and that measurement can come back as one word per line; a CSS max-height was hiding the wrong answer rather than correcting it. No parameter changes what it sends. This drops controls for values the server was already discarding.
…ntrols Hiding a parameter behind a companion toggle judged it by a rule the client cannot see all of: a reasoningObject endpoint sends the reasoning fields without the Responses API, adaptive-thinking Claude models ignore the budget however thinking is set, and a deployment could not declare the rule in librechat.yaml at all. The panel groups every parameter again and leaves out only a choice with no options, such as Bedrock's region when no regions are configured, so no section stands empty. Reset and Save As Preset share a row while their labels fit and stack when a translation is too long for the panel.
A Spanish Save As Preset label measures 338px on its own against a 282px row, so stacking the two actions still ran it past the panel. The labels now wrap inside their buttons.
d29830e to
10b5ffd
Compare
berry-13
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10b5ffd66c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (value == null || value === '') { | ||
| return false; | ||
| } | ||
| if (Array.isArray(value)) { | ||
| return value.length > 0 && JSON.stringify(value) !== JSON.stringify(setting.default); |
There was a problem hiding this comment.
When an operator gives a text or tags parameter a nonempty default through customParams.paramDefinitions, clearing that control stores '' or [], but these branches always report it as untouched even though it differs from setting.default. The section badge therefore disappears after a user explicitly clears the configured default; compare empty values against the default and reserve the untouched case for an absent value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d405406: only an absent value reads as untouched now; clearing a text or tags parameter whose configured default is nonempty counts as a change. Covered by groups.spec.
| <div | ||
| className={cn( | ||
| 'flex flex-col items-center justify-start gap-6', | ||
| 'flex h-full flex-col items-center justify-start gap-6', |
There was a problem hiding this comment.
These dynamic controls are also reused by the desktop preset editors, whose two flex columns are stretched to the height of a single grid row (for example, Endpoints/Settings/OpenAI.tsx has three controls in the left column and many more in the right). Adding h-full to every shared renderer makes each left-column child expand or shrink against that tall column; together with the new full-height content-between trigger, the model/name/prompt labels and controls can be separated by hundreds of pixels and substantially bloat the Save As Preset dialog. Apply this equal-height layout from the grouped panel wrapper or behind a panel-specific prop instead.
AGENTS.md reference: AGENTS.md:L81-L86
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d405406: h-full is off the shared Dynamic* roots and the panel's grid cells stretch their controls instead (*:h-full), so the preset editors keep natural heights. preset-dialog-controls-keep-labels-close measures label-to-field gaps in the Edit Preset dialog: with the old roots it failed on desktop (Model label 355px above its field), and it passes on e6d7b22.
| {changed > 0 && ( | ||
| <span className="bg-surface-tertiary text-text-primary shrink-0 rounded-full px-1.5 text-xs font-normal tracking-normal normal-case tabular-nums"> | ||
| {changed} | ||
| </span> |
There was a problem hiding this comment.
For a screen-reader user, this badge only changes the section's accessible name from something like “Reasoning” to “Reasoning 1”; nothing identifies the number as a count of modified parameters. The new state indicator therefore conveys its meaning visually but not accessibly. Add a localized accessible phrase such as “1 modified parameter” while hiding the decorative numeral from the heading name if necessary.
AGENTS.md reference: AGENTS.md:L31-L34
Useful? React with 👍 / 👎.
…ounts mean The shared parameter controls stretched to their parent's height, which the preset editors' tall columns turned into labels far above their fields; the panel's cells now stretch their controls instead. The change count beside a heading is read out as a number of changed settings, and clearing a default a deployment configured counts as a change.
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This branch has not been deployed
2 participants
Summary
Part 5 of 6 of the quieter-layout stack. Its base, #16246, has merged, so this now targets
canarydirectly.The parameters panel rendered a provider's parameters in the order the definitions arrived, with nothing to say which belonged together. Keys are now filed under six headings, each with a count of what this conversation has changed in it. A key the map does not know, such as a deployment's own
customParams, keeps its place in the last section. Every parameter stays visible: narrow controls sit two to a row and only free text takes the full width. Fifteen parameters now take 954px, down from 1243px.A choice control with nothing to choose, such as Bedrock's region when no regions are configured, is left out before grouping so no section stands empty. Reset and Save As Preset share a row while their labels fit, and stack and wrap when a translation is longer than the panel. No parameter changes what it sends.
Type of change
Testing
Tested environments/configuration:
canaryAutomated tests:
e2e/specs/mock/scenarios/grouped-model-params.spec.ts: sections with change counts, a parameter staying on screen when a related toggle is off, and the actions staying inside the panel in Spanishnpx jestoverclient/src/components/SidePanel/Parameters: grouping, empty choice controls, change countsnpx tsc --noEmitinclient,npx eslinton every changed file including theshadcn/*design rulesScreenshots / recordings
Before is
canary; after is the top of this stack, so an image can also show changes from later PRs in the chain. Chromium, 1440x900 desktop and 390x844 mobile.Risk / compatibility
Parameter definitions and stored conversation values are unchanged; only the panel's layout and which empty controls it renders differ.
Checklist