[OPIK-2366] refactor reason field logic #3200
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the feedback score editing functionality to improve UI consistency and state management. The changes focus on better reason field editing, enhanced component key stability, and improved input control through hook enhancements.
- Refactored reason editing logic with dedicated toggle handler and synchronized text area values
- Enhanced component key stability to prevent unnecessary remounts
- Added programmatic input value control to the debounced value hook
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
useDebouncedValue.ts |
Added setInputValue method and removed initialValue parameter for better input control |
AddFeedbackScorePopover.tsx |
Updated component key to composite format for stability |
TraceAnnotateViewer.tsx |
Updated component key to composite format for stability |
AnnotateRow.tsx |
Refactored reason editing with toggle handler and improved value synchronization |
| export const useDebouncedValue = ({ | ||
| initialValue, | ||
| onDebouncedChange, | ||
| delay = 300, | ||
| onChange, | ||
| }: UseDebouncedValueArgs) => { |
There was a problem hiding this comment.
The setInputValue parameter is defined in the type but not destructured in the function parameters. This will cause the external setInputValue functionality to be unavailable.
Copilot uses AI. Check for mistakes.
| @@ -6,16 +6,14 @@ type UseDebouncedValueArgs = { | |||
| onDebouncedChange: (value: string) => void; | |||
| delay?: number; | |||
| onChange?: () => void; | |||
| setInputValue?: (value: string) => void; | |||
There was a problem hiding this comment.
The setInputValue parameter name conflicts with the internal state setter. Consider renaming to onSetInputValue or externalSetInputValue to avoid confusion and clearly indicate it's a callback.
| setInputValue?: (value: string) => void; | |
| onSetInputValue?: (value: string) => void; |
Copilot uses AI. Check for mistakes.
| @@ -93,7 +93,8 @@ const AnnotateRow: React.FunctionComponent<AnnotateRowProps> = ({ | |||
| value: reasonValue, | |||
| onChange: onReasonChange, | |||
| onReset: onReasonReset, | |||
| resetValue, | |||
| resetValue: resetReasonValue, | |||
| setInputValue: setReasonValue, | |||
| } = useDebouncedValue({ | |||
| initialValue: feedbackScore?.reason, | |||
There was a problem hiding this comment.
The initialValue parameter is being passed but was removed from the useDebouncedValue hook parameters. This will be ignored and could lead to unexpected behavior.
Copilot uses AI. Check for mistakes.
Details
This pull request refactors feedback score editing and copy functionality in the trace annotation components, improves key stability for feedback score editors, and enhances the
useDebouncedValuehook for better input control. These changes primarily address UI consistency, state management, and component reusability.Feedback Score Editing and Copy Functionality:
AnnotateRow.tsxto use a dedicated toggle handler (toggleEditReasonHandler) and ensure the text area value is always in sync with the underlying feedback score reason. This improves the reliability of editing and copying reasons.Component Key Stability:
keyprop forFeedbackScoresEditorin bothTraceAnnotateViewer.tsxandAddFeedbackScorePopover.tsxto a composite key format (no-span-id-traceId), preventing unnecessary component remounts and improving UI stability.Debounced Input Hook Enhancement:
setInputValuemethod to theuseDebouncedValuehook, allowing external components to programmatically update the input value. This supports more flexible and controlled input management in UI components.Change checklist
Issues
Testing
Documentation