Skip to content

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

billybonks
Copy link
Contributor

@billybonks billybonks commented May 6, 2025

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:

  • Added an optional id field to the ResultsFile interface

Enhancements:

  • Simplified the data fetching logic in the Eval component
  • Extracted common data fetching and state setting logic into a reusable function
  • Removed redundant props and simplified component signature

Documentation:

  • Updated HTTP provider documentation with more detailed examples of response parsing

billybonks added 2 commits May 6, 2025 22:56
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
Copy link
Contributor

sourcery-ai bot commented May 6, 2025

Reviewer's Guide

The 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

Change Details Files
Refactored data fetching and state management in the Eval component for improved clarity and organization.
  • Simplified EvalOptions interface by removing props for preloaded data, recent evals, and default eval ID.
  • Extracted fetchRecentFileEvals and fetchEvalById data fetching functions out of the component body, making them regular async functions.
  • Introduced a setPageState callback to centralize the logic for setting eval data, configuration, author, and IDs.
  • Streamlined the main useEffect hook's logic for initializing and updating eval data, including conditional fetching based on fetchId, local websocket handling, and fetching recent evals.
  • Removed direct usage of preloadedData prop and simplified initial state for recentEvals and defaultEvalId.
src/app/src/pages/eval/components/Eval.tsx
Updated documentation for the HTTP provider's function parser.
  • Clarified that the transformResponse function can return either a string or a ProviderResponse object.
  • Added an example demonstrating transformResponse returning an object with output and tokenUsage.
  • Included TypeScript type definitions for ProviderResponse and TokenUsage within a details dropdown.
site/docs/providers/http.md
Added an id field to the ResultsFile type and ensured it is populated.
  • Added an optional id string field to the ResultsFile interface in src/types/index.ts.
  • Ensured the id field is set with this.id in the toResultsFile method in src/models/eval.ts.
src/models/eval.ts
src/types/index.ts

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

gru-agent bot commented May 6, 2025

TestGru Assignment

Summary

Link CommitId Status Reason
Detail 96e3543 🚫 Skipped No files need to be tested {"site/docs/providers/http.md":"File path does not match include patterns.","src/app/src/pages/eval/components/Eval.tsx":"File path does not match include patterns.","src/models/eval.ts":"File path does not match include patterns.","src/types/index.ts":"File path does not match include patterns."}

Tip

You can @gru-agent and leave your feedback. TestGru will make adjustments based on your input

Copy link
Contributor

gru-agent bot commented May 6, 2025

TestGru Assignment

Summary

Link CommitId Status Reason
Detail 96e3543 ✅ Finished

Files

File Pull Request
src/models/eval.ts 🟣 Merged #3904
src/types/index.ts 🔴 Closed #3905

Tip

You can @gru-agent and leave your feedback. TestGru will make adjustments based on your input

setLoaded(true);
setDefaultEvalId(defaultEvalId);
setEvalId(defaultEvalId);
});
} else {
Copy link
Contributor

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.

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
@billybonks billybonks force-pushed the feat/super-refactor branch from 495651d to e270eba Compare May 6, 2025 17:01
- 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.
@billybonks billybonks force-pushed the feat/super-refactor branch from e270eba to 701aa4e Compare May 6, 2025 17:03
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.

1 participant