refactor(tui): replace magic numbers with named constants for eval form fields#139
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaces magic numeric field indices with named constants/types for the evaluation TUI form across rendering, controllers, keyboard handling, and paste logic; adds FormField/EvalFormField constant sets and adjusts a cast when creating FormState. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tui/internal/tui/keyboard_controller.go (1)
149-163: Fix logic error: slash insertion broken for Judge Model field.The condition
m.evalState.currentField <= EvalFieldProtocol(line 149) only allows fields 0 and 1, but the switch statement (lines 152-161) handles bothEvalFieldAgentURL(0) andEvalFieldJudgeModel(3). This means the "/" character cannot be inserted into the Judge Model field because the condition fails whencurrentFieldis 3.🔎 Proposed fix
- if m.currentScreen == NewEvaluationScreen && m.evalState != nil && m.evalState.currentField <= EvalFieldProtocol { + if m.currentScreen == NewEvaluationScreen && m.evalState != nil && + (m.evalState.currentField == EvalFieldAgentURL || m.evalState.currentField == EvalFieldJudgeModel) { // Handle "/" character directly in text fields s := "/" switch m.evalState.currentField { case EvalFieldAgentURL:
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/tui/internal/screens/evaluations/form_view.go(1 hunks)packages/tui/internal/screens/evaluations/types.go(2 hunks)packages/tui/internal/tui/common_controller.go(1 hunks)packages/tui/internal/tui/eval_form_view.go(1 hunks)packages/tui/internal/tui/eval_types.go(2 hunks)packages/tui/internal/tui/form_controller.go(6 hunks)packages/tui/internal/tui/keyboard_controller.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/tui/internal/tui/form_controller.go (1)
packages/tui/internal/tui/eval_types.go (6)
EvalFieldAgentURL(32-32)EvalFieldJudgeModel(35-35)EvalFieldStartButton(37-37)EvalFieldProtocol(33-33)EvalFieldTransport(34-34)EvalFieldDeepTest(36-36)
packages/tui/internal/screens/evaluations/form_view.go (1)
packages/tui/internal/screens/evaluations/types.go (1)
FormFieldStartButton(12-12)
packages/tui/internal/tui/common_controller.go (1)
packages/tui/internal/tui/eval_types.go (2)
EvalFieldAgentURL(32-32)EvalFieldJudgeModel(35-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: codestyle
🔇 Additional comments (10)
packages/tui/internal/tui/eval_types.go (2)
28-41: LGTM! Well-structured constant definitions.The new
EvalFormFieldtype and associated constants provide clear, self-documenting field indices. IncludingEvalFieldCountfor bounds checking is a good practice.
69-70: LGTM! Type change improves type safety.Changing
currentFieldfrominttoEvalFormFieldprovides better type safety and makes the intent explicit.packages/tui/internal/screens/evaluations/form_view.go (1)
174-174: LGTM! Excellent use of named constant.Replacing the magic number
5withFormFieldStartButtonmakes the code self-documenting.packages/tui/internal/tui/eval_form_view.go (1)
35-35: LGTM! Type cast is appropriate here.The cast from
EvalFormFieldtointis necessary sinceFormState.CurrentFieldis defined asintin the evaluations package (to avoid import cycles).packages/tui/internal/tui/common_controller.go (1)
50-61: LGTM! Clear use of semantic constants.Replacing numeric literals with
EvalFieldAgentURLandEvalFieldJudgeModelmakes the paste handling logic much more readable.packages/tui/internal/tui/keyboard_controller.go (1)
284-294: LGTM! Correct use of field constants.The Enter key handling correctly uses
EvalFieldStartButtonandEvalFieldJudgeModelconstants, making the intent clear.packages/tui/internal/screens/evaluations/types.go (1)
3-13: LGTM! Well-documented constant mirroring.The mirrored
FormField*constants are a good solution to avoid import cycles while maintaining consistency. The comment clearly explains the relationship toEvalFormField.packages/tui/internal/tui/form_controller.go (3)
20-48: LGTM! Navigation logic correctly uses constants.The up/down navigation properly uses
EvalFieldStartButtonfor bounds checking andEvalFieldAgentURL/EvalFieldJudgeModelfor cursor positioning in text fields.
50-80: LGTM! Left/right handling correctly distinguishes field types.The code appropriately uses constants to differentiate between text fields (
EvalFieldAgentURL,EvalFieldJudgeModel) and dropdown fields (EvalFieldProtocol,EvalFieldTransport), applying the correct behavior for each.
82-132: LGTM! Special key and text input handling uses clear constants.All field-specific operations (Space for toggle, Tab for dialog, Backspace and character insertion for text fields) correctly use the named constants, making the code much more maintainable.
| // FormField represents the field indices for the evaluation form | ||
| // These constants mirror the EvalFormField constants in the tui package | ||
| // to avoid import cycles while maintaining consistency | ||
| const ( | ||
| FormFieldAgentURL = 0 | ||
| FormFieldProtocol = 1 | ||
| FormFieldTransport = 2 | ||
| FormFieldJudgeModel = 3 | ||
| FormFieldDeepTest = 4 | ||
| FormFieldStartButton = 5 | ||
| ) | ||
|
|
There was a problem hiding this comment.
Consider:
| // FormField represents the field indices for the evaluation form | |
| // These constants mirror the EvalFormField constants in the tui package | |
| // to avoid import cycles while maintaining consistency | |
| const ( | |
| FormFieldAgentURL = 0 | |
| FormFieldProtocol = 1 | |
| FormFieldTransport = 2 | |
| FormFieldJudgeModel = 3 | |
| FormFieldDeepTest = 4 | |
| FormFieldStartButton = 5 | |
| ) | |
| // FormField represents the field indices for the evaluation form | |
| // These constants mirror the EvalFormField constants in the tui package | |
| // to avoid import cycles while maintaining consistency | |
| type FormField int | |
| const ( | |
| FormFieldAgentURL FormField = iota | |
| FormFieldProtocol | |
| FormFieldTransport | |
| FormFieldJudgeModel | |
| FormFieldDeepTest | |
| FormFieldStartButton | |
| ) |
| const ( | ||
| EvalFieldAgentURL EvalFormField = 0 | ||
| EvalFieldProtocol EvalFormField = 1 | ||
| EvalFieldTransport EvalFormField = 2 | ||
| EvalFieldJudgeModel EvalFormField = 3 | ||
| EvalFieldDeepTest EvalFormField = 4 | ||
| EvalFieldStartButton EvalFormField = 5 | ||
|
|
||
| // EvalFieldCount is the total number of fields (for bounds checking) | ||
| EvalFieldCount = 6 | ||
| ) | ||
|
|
There was a problem hiding this comment.
Consider:
| const ( | |
| EvalFieldAgentURL EvalFormField = 0 | |
| EvalFieldProtocol EvalFormField = 1 | |
| EvalFieldTransport EvalFormField = 2 | |
| EvalFieldJudgeModel EvalFormField = 3 | |
| EvalFieldDeepTest EvalFormField = 4 | |
| EvalFieldStartButton EvalFormField = 5 | |
| // EvalFieldCount is the total number of fields (for bounds checking) | |
| EvalFieldCount = 6 | |
| ) | |
| const ( | |
| EvalFieldAgentURL EvalFormField = iota | |
| EvalFieldProtocol | |
| EvalFieldTransport | |
| EvalFieldJudgeModel | |
| EvalFieldDeepTest | |
| EvalFieldStartButton | |
| ) | |
| const EvalFieldCount = 6 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/tui/internal/screens/evaluations/types.go (1)
33-33: Consider changingCurrentFieldtoFormFieldtype for better type safety.While the comment correctly documents the use of
FormField*constants, the field itself remains typed asint. Changing it toFormFieldwould provide compile-time type checking and make misuse more difficult.🔎 Proposed type change
- CurrentField int // Use FormField* constants for field indices + CurrentField FormField // Field index in the evaluation formNote: This change may require explicit conversions (e.g.,
int(f.CurrentField)) in other parts of the codebase where integer operations are performed, but it would provide stronger type safety guarantees.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/tui/internal/screens/evaluations/form_view.gopackages/tui/internal/screens/evaluations/types.gopackages/tui/internal/tui/eval_types.go
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/tui/internal/screens/evaluations/form_view.go
- packages/tui/internal/tui/eval_types.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: codestyle
🔇 Additional comments (1)
packages/tui/internal/screens/evaluations/types.go (1)
3-15: LGTM! Clean implementation of form field constants.The
FormFieldtype and constants are well-defined using idiomatic Go withiota. The documentation clearly explains the rationale for mirroring constants to avoid import cycles, which is a standard Go pattern.
2 participants
Description
In the TUI, replacing magic numbers used for
EvaluationState.currentFieldwith named constants, making the code more readable and maintainableType of Change
Changes Made
The
currentFieldinteger was used with hardcoded values (0, 1, 2, 3, 4, 5) scattered across multiple files, making it difficult to understand which field each number represented and error-prone when making changesSolution
EvalFormFieldtype with named constants:EvalFieldAgentURL = 0
EvalFieldProtocol = 1
EvalFieldTransport = 2
EvalFieldJudgeModel = 3
EvalFieldDeepTest = 4
EvalFieldStartButton = 5
Related Issues/PRs