-
Notifications
You must be signed in to change notification settings - Fork 81
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
Browser and evals #358
base: dev
Are you sure you want to change the base?
Browser and evals #358
Conversation
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.
👍 Looks good to me! Reviewed everything up to 6dcb0ff in 1 minute and 5 seconds
More details
- Looked at
414
lines of code in6
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. app-server/src/api/v1/evals.rs:31
- Draft comment:
Consider usingunwrap_or("default")
instead ofunwrap_or_else(|| "default".to_string())
for better performance. - Reason this comment was not posted:
Confidence changes required:50%
The use ofunwrap_or_else
with a string allocation can be optimized by usingunwrap_or
with a string slice.
2. app-server/src/api/v1/evals.rs:65
- Draft comment:
Consider usingunwrap_or("default")
instead ofunwrap_or_else(|| "default".to_string())
for better performance. - Reason this comment was not posted:
Confidence changes required:50%
The use ofunwrap_or_else
with a string allocation can be optimized by usingunwrap_or
with a string slice.
3. app-server/src/main.rs:434
- Draft comment:
Consider using theactix_cors
middleware to handle CORS instead of manually setting headers. - Reason this comment was not posted:
Confidence changes required:50%
Theoptions_handler
function is manually setting CORS headers. It's better to use theactix_cors
middleware for handling CORS in a more robust and configurable way.
4. frontend/components/traces/session-player.tsx:44
- Draft comment:
Usenumber
instead ofNodeJS.Timeout
fordebounceTimerRef
to avoid type issues in a browser environment. - Reason this comment was not posted:
Confidence changes required:50%
ThedebounceTimerRef
is initialized withNodeJS.Timeout
, which is not correct for a browser environment. It should benumber
instead.
Workflow ID: wflow_aCRaB4mlg5ZMO6pQ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
) | ||
SETTINGS async_insert = 1, wait_for_async_insert = 0 | ||
VALUES ", |
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.
we already have that in global Clickhouse settings in main.rs
app-server/src/api/v1/evals.rs
Outdated
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.
Is this an intermediate state while we migrate from the previous evaluations? If not, why separate file?
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.
👍 Looks good to me! Incremental review on 802728e in 33 seconds
More details
- Looked at
172
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. app-server/src/main.rs:82
- Draft comment:
Consider documenting the reason for changing theGRPC_PAYLOAD_DECODING_LIMIT
from 100MB to 10MB for future reference and clarity. - Reason this comment was not posted:
Confidence changes required:50%
The change in payload limits is significant and should be documented for clarity and future reference.
2. app-server/src/main.rs:81
- Draft comment:
Consider documenting the reason for changing theHTTP_PAYLOAD_LIMIT
from 100MB to 5MB for future reference and clarity. - Reason this comment was not posted:
Confidence changes required:50%
The change in payload limits is significant and should be documented for clarity and future reference.
3. frontend/components/traces/session-player.tsx:154
- Draft comment:
Ensure that removingcleanupDanglingNodes
does not lead to memory leaks or other issues with dangling nodes. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_Mnz1iVxogIuhPsBb
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
👍 Looks good to me! Incremental review on 6a92202 in 13 seconds
More details
- Looked at
20
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/components/traces/session-player.tsx:6
- Draft comment:
Consider organizing the import statements by grouping external libraries first, followed by internal modules. This improves readability. - Reason this comment was not posted:
Confidence changes required:10%
The import statements should be organized for better readability.
Workflow ID: wflow_006WIK4PSZRytZnW
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
👍 Looks good to me! Incremental review on 3f4f858 in 32 seconds
More details
- Looked at
3157
lines of code in9
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. app-server/src/db/evaluations.rs:93
- Draft comment:
The 'index' column is introduced, but the existing 'index_in_batch' column is not removed or deprecated. Ensure that both columns are necessary and clarify their distinct purposes to avoid redundancy. - Reason this comment was not posted:
Comment did not seem useful.
2. frontend/app/api/projects/[projectId]/evaluations/[evaluationId]/route.ts:55
- Draft comment:
Ensure that the change in order of 'orderBy' clause to prioritize 'index' over 'createdAt' is consistent across all related queries to maintain the intended ordering logic. - Reason this comment was not posted:
Confidence changes required:50%
The PR changes the order of the 'orderBy' clause in SQL queries to prioritize 'index' over 'createdAt'. This change should be consistent across all related queries to ensure the intended ordering logic is applied everywhere.
3. frontend/app/project/[projectId]/evaluations/[evaluationId]/page.tsx:87
- Draft comment:
Ensure that the change in order of 'orderBy' clause to prioritize 'index' over 'createdAt' is consistent across all related queries to maintain the intended ordering logic. - Reason this comment was not posted:
Confidence changes required:50%
The PR changes the order of the 'orderBy' clause in SQL queries to prioritize 'index' over 'createdAt'. This change should be consistent across all related queries to ensure the intended ordering logic is applied everywhere.
Workflow ID: wflow_MhdvCDNcA4d8tILd
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
👍 Looks good to me! Incremental review on 015e8c5 in 59 seconds
More details
- Looked at
346
lines of code in8
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_UTubcbRAdTYapOHg
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
👍 Looks good to me! Incremental review on fc0cfe3 in 16 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
1
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/package.json:106
- Draft comment:
Ensure that the update torrweb-player
is compatible with other dependencies and does not introduce any breaking changes. - Reason this comment was not posted:
Confidence changes required:50%
The update torrweb-player
should be checked for compatibility with other dependencies.
Workflow ID: wflow_psTlsiW2UcYGURMy
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
This pull request enhances evaluation and browser session handling by adding new API endpoints, updating database schemas, and improving frontend components.
evals.rs
to handle evaluation-related API endpoints, includinginit_eval
andsave_eval_datapoints
.browser_sessions.rs
to fix SQL syntax.evaluations.rs
to includeindices
inset_evaluation_results()
.main.rs
andspans.rs
.index
column toevaluation_results
in0018_vengeful_violations.sql
.0018_snapshot.json
and_journal.json
.route.ts
files for browser sessions and evaluations to handle new API logic.evaluation.tsx
to support score columns and Supabase integration.session-player.tsx
for improved event handling and UI updates.layout.tsx
andpage.tsx
for project and evaluation pages.package.json
to userrweb-player
version2.0.0-alpha.18
.This description was created by for fc0cfe3. It will automatically update as commits are pushed.