Skip to content

feat(engine-settings): add 'number' variant to engine setting fields#875

Merged
aaight merged 1 commit intodevfrom
feature/numeric-engine-setting-fields
Mar 15, 2026
Merged

feat(engine-settings): add 'number' variant to engine setting fields#875
aaight merged 1 commit intodevfrom
feature/numeric-engine-setting-fields

Conversation

@aaight
Copy link
Copy Markdown
Collaborator

@aaight aaight commented Mar 15, 2026

Summary

  • Adds min, max, and step optional props to the 'number' variant of AgentEngineSettingField in src/backends/types.ts (the TODO comment about Story chore: add .nvmrc for Node.js 22 #2 is now resolved)
  • Updates the frontend EngineSettingField type in engine-settings-fields.tsx to mirror the backend with min/max/step
  • Implements <Input type="number"> rendering for number fields in EngineSettingsFields, with inherit/default placeholder and value stored as a number (cleared to undefined when empty)
  • Extracts field rendering into a FieldControl helper component to keep cognitive complexity within the linter limit

Closes Trello card: https://trello.com/c/f7eciwO5/392-as-a-developer-i-want-numeric-engine-setting-fields-so-that-thinkingbudgettokens-can-be-configured-in-the-dashboard

Work item: https://trello.com/c/69b6cf2712b4af253a3e3dd1

Test plan

  • All 277 unit test files pass (5035 tests)
  • npm run lint — zero errors/warnings
  • npm run typecheck — zero errors
  • Existing select and boolean fields render unchanged
  • Number fields render <Input type="number"> with optional min/max/step constraints
  • Empty input clears the field to undefined (inherit behaviour)
  • Valid numeric input is stored as a number, not a string

🤖 Generated with Claude Code

🕵️ claude-code · claude-sonnet-4-6 · run details

Copy link
Copy Markdown
Collaborator

@nhopeatall nhopeatall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

LGTM — Clean implementation of numeric engine setting fields. The type additions are symmetric between backend and frontend, the FieldControl extraction is a good refactor, and the number parsing logic is correct.

Should Fix (non-blocking)

  • Stale TODO comments: src/backends/catalog.ts (lines 70-71) and src/backends/claude-code/settings.ts (lines 8-10) still reference "Story #2" and state that numeric field rendering isn't implemented yet. Since this PR implements it, those comments should be cleaned up in a follow-up.

  • No client-side min/max enforcement in onChange handler: The <Input type="number"> passes min/max/step as HTML attributes, but browser type="number" inputs don't actually prevent out-of-range values from being typed — they only affect the stepper and :invalid pseudo-class. The onChange handler accepts any valid number regardless of min/max. This is mitigated by the Zod schema on the backend (z.number().int().positive() for thinkingBudgetTokens), so it's not a correctness bug, but adding client-side clamping/validation would improve UX by giving immediate feedback. Worth considering for a follow-up.

  • Catalog doesn't use new min/max/step props: The thinkingBudgetTokens field in catalog.ts doesn't specify min, max, or step, even though the Zod schema requires int().positive(). Adding min: 1 and step: 1 to the catalog entry would make the UI stepper and constraints match the backend validation — good for a follow-up.

Inline Notes

  • web/src/components/settings/engine-settings-fields.tsx:106 — Nitpick: Number(trimmed) accepts "Infinity" and "-Infinity" (these are not NaN), though HTML type="number" inputs prevent typing these in practice. If this component ever receives programmatic input, you may want Number.isFinite(parsed) instead of !Number.isNaN(parsed).

🕵️ claude-code · claude-opus-4-6 · run details

@aaight aaight merged commit f9c1d50 into dev Mar 15, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants