-
Notifications
You must be signed in to change notification settings - Fork 135
feat: add events table #801
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: dev
Are you sure you want to change the base?
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.
Important
Looks good to me! 👍
Reviewed everything up to 625233d in 1 minute and 24 seconds. Click for details.
- Reviewed
510
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
6
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/app/api/projects/[projectId]/events/route.ts:9
- Draft comment:
Consider parsing numeric query params (pageSize, pageNumber) to numbers before validation if the schema expects numbers. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% 1. The code uses Zod for validation which can handle type coercion. 2. Without seeing GetEventsSchema, we don't know if these fields are even meant to be numbers. 3. Even if they are numbers, Zod's built-in coercion is a common and valid approach. 4. Pre-parsing isn't necessarily better - it would add complexity and potential error cases. I don't have visibility into GetEventsSchema to know if these fields should be numbers. The current approach using Zod's type coercion might be intentional and perfectly valid. While pre-parsing could work, it would add complexity without clear benefit, and the current approach using Zod's validation capabilities is a common pattern. Delete the comment. Without seeing GetEventsSchema we can't be certain this is an issue, and the current approach using Zod's validation is a valid pattern.
2. frontend/components/traces/events-table/columns.tsx:45
- Draft comment:
Consider extracting the GUID prefix stripping logic into a helper function to avoid duplication. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
3. frontend/components/traces/events-table/index.tsx:93
- Draft comment:
Include the getEvents callback in the useEffect dependency array to ensure updates are correctly captured. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is technically correct - React's exhaustive-deps rule would flag this. However, getEvents is already memoized with useCallback and includes all the dependencies that would trigger a re-render. Adding getEvents to the dependency array would be redundant since its dependencies are already covered. The current implementation is actually cleaner. The comment follows React's rules strictly, but might be overly pedantic. The current implementation is already stable and won't cause issues. While technically correct, following this suggestion would add complexity without providing any real benefit since the dependencies are already properly managed. The comment should be removed as it suggests a change that would add complexity without practical benefit, given that the dependencies are already properly managed through useCallback.
4. frontend/lib/actions/events/index.ts:36
- Draft comment:
Verify that the SQL tag safely parameterizes the interpolated search string to prevent SQL injection. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify something, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue in the code. Therefore, it should be removed.
5. frontend/lib/actions/events/utils.ts:10
- Draft comment:
Ensure 'filter.value' is always a string to avoid runtime errors in processEventAttributeFilter. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a "verify/ensure" type comment which we should generally avoid. The type FilterDef should already define the type of value. Without seeing the FilterDef type definition, we can't be certain there's an actual issue. The code looks like it expects strings and uses string operations, which suggests the types are probably already correct. I might be wrong about FilterDef - what if it allows non-string values? The runtime error concern could be valid. Even if FilterDef allows non-strings, this would be a type system issue that should be caught in TypeScript compilation, not something that needs a PR comment. Delete the comment. It asks for verification rather than pointing out a clear issue, and type safety concerns should be handled by TypeScript, not PR comments.
6. frontend/lib/db/utils.ts:64
- Draft comment:
Consider extracting hardcoded column names (e.g., 'start_time', 'end_time') into constants for better maintainability. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_hiPAjDbIjIb56FOP
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed b4bf86d in 1 minute and 25 seconds. Click for details.
- Reviewed
400
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
6
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/app/api/projects/[projectId]/events/route.ts:35
- Draft comment:
Consider verifying that session.user.apiKey exists before calling getEvents to avoid potential undefined errors. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is about code that was just added in this diff. While it's true that user.apiKey could potentially be undefined, we don't have enough context to know if this is actually possible. We don't know the type of session.user or what authOptions guarantees. The comment is speculative without strong evidence that this is actually a problem. The comment could be pointing out a real type safety issue that could cause runtime errors. TypeScript types might allow undefined here. Without seeing the type definitions or auth setup, we can't be certain this is a real issue. The auth system likely guarantees apiKey exists when session exists. Delete the comment as it's speculative without strong evidence that apiKey could actually be undefined in practice.
2. frontend/components/traces/events-table/columns.tsx:35
- Draft comment:
The regex used for stripping the UUID prefix is duplicated; consider extracting it to a shared constant for maintainability. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
3. frontend/components/traces/events-table/index.tsx:92
- Draft comment:
Using JSON.stringify(filter) in the useEffect dependency array can be unstable; consider a stable dependency (e.g., a joined string) to avoid unnecessary re-renders. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. frontend/lib/actions/events/index.ts:73
- Draft comment:
Consider logging errors inside the JSON.parse catch block when transforming attributes for better troubleshooting. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
5. frontend/lib/actions/events/utils.ts:42
- Draft comment:
Unsupported filter columns/operators are silently skipped; add a comment to clarify if this behavior is intentional. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
6. frontend/lib/actions/sql/index.ts:11
- Draft comment:
Typo: The error message for the apiKey field lacks a period at the end, unlike the 'Query is required.' message. Consider changing it to 'API key is required.' for consistency. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_BoUBqOy1YS8bf2aR
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed cd199a0 in 51 seconds. Click for details.
- Reviewed
283
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
3
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/app/api/projects/[projectId]/events/route.ts:13
- Draft comment:
Verify that omitting 'projectId' from GetEventsSchema is intentional. The projectId is later merged (line 26) – add a comment for clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%
<= threshold50%
The comment is asking the author to verify if omitting 'projectId' is intentional, which is not allowed. However, it also suggests adding a comment for clarity, which is a valid suggestion. The comment is partially useful.
2. frontend/lib/actions/events/utils.ts:4
- Draft comment:
Ensure 'id' column data is processed consistently. 'span_id' is normalized (lines 4-5) but 'id' in buildColumnFilter (line 48) isn’t; confirm if additional normalization/validation for 'id' is needed. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
3. frontend/lib/actions/events/utils.ts:11
- Draft comment:
The buildDateFilters function returns immediately when 'pastHours' is provided, ignoring startTime/endTime. Confirm that this precedence is intended, or document the logic. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
Workflow ID: wflow_8BdVvNU036z2Kvac
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Important
Add events table feature with backend API, frontend components, and query logic for fetching and displaying project events.
GET
endpoint inroute.ts
to fetch events for a project, handling authentication and query parameter parsing.EventsTable
component inevents-table/index.tsx
for displaying events with pagination, filtering, and search.events-table/columns.tsx
for event attributes like ID, Name, Span ID, etc.EventsTable
intotraces.tsx
with a new "Events" tab.getEvents
function inevents/index.ts
to query events from the database with filters and pagination.events/utils.ts
for building SQL queries with filters and date ranges.GetEventsSchema
inevents/index.ts
for validating event query parameters.PaginationSchema
incommon/types.ts
to limit page size between 1 and 500.This description was created by
for cd199a0. You can customize this summary. It will automatically update as commits are pushed.