-
-
Notifications
You must be signed in to change notification settings - Fork 527
refactor: eval.tsx to make it easier to digest how data is set. #3902
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
as far as i can see none fo this code is being used preloaded data was orginally used using what is now known as fetchId 2 years ago 4ebb21d recentEvals is now loaded using fetchRecentEvals defaultEvalId is now being set during one of the load sequances I also cant see where this component is being used outside of src/app/src/pages/eval/page.tsx
also return from hook is meant to be cleanup
Reviewer's GuideThe pull request refactors the Eval.tsx component to simplify data fetching and state management logic. This is achieved by extracting data fetching functions, introducing a unified state update callback, and streamlining the main data loading effect. Additionally, the documentation for the HTTP provider's function parser is updated, and an 'id' field is added to the ResultsFile type and its construction. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
TestGru AssignmentSummary
Tip You can |
setLoaded(true); | ||
setDefaultEvalId(defaultEvalId); | ||
setEvalId(defaultEvalId); | ||
}); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The socket connection created when running locally isn’t cleaned up. Add a cleanup function (e.g., socket.disconnect()
in the useEffect
return) to prevent potential memory leaks.
96e3543
to
495651d
Compare
it is marked as optional because of SharedResults which is being used in a function called handleLegacyResults (maybe can be removed) and in redteam report is also using it but its the same api as eval /results/${id} so we can probably easily change that type. once both changes are done we can remove the optional
495651d
to
e270eba
Compare
- Colocate data setting logic within a single hook for better readability. - Extract API call functions out of the component as they no longer depend on hooks. - Introduce a reusable callback to handle state updates based on `ResultsFile`. - Adjust error handling: set `failed` state when data is undefined i would prefer on thrown errors but to minimize changes. - Refactor data fetching flow: 1. Fetch data by `fetchId` if provided. 2. Fetch all evaluations for the dropdown menu. 3. Use the first evaluation from the dropdown if `fetchId` is not provided and not running locally. 4. Otherwise, rely on socket updates.
e270eba
to
701aa4e
Compare
please review by commits, i have explained all rational in the messages. original conversation #3825 and dependent on #3900
i cant seem to change the base branch to my fork sadly, so leaving in draft for now
Summary by Sourcery
Refactor the Eval component to simplify data fetching and state management, improving code readability and reducing complexity.
New Features:
id
field to the ResultsFile interfaceEnhancements:
Documentation: