-
Notifications
You must be signed in to change notification settings - Fork 153
Add Span Embed #1077
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?
Add Span Embed #1077
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 94c7843 in 57 seconds. Click for details.
- Reviewed
217lines of code in4files - Skipped
0files when reviewing. - Skipped posting
4draft 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/shared/traces/[traceId]/spans/route.ts:16
- Draft comment:
Good error handling using ZodError. Consider optionally logging unexpected errors to aid debugging in production. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. frontend/app/blog/[slug]/page.tsx:60
- Draft comment:
Clear addition of TraceEmbed in MDX components. Ensure MDX content supplies the necessary props for proper rendering. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. frontend/components/embeds/trace-embed.tsx:24
- Draft comment:
Good use of useMemo to resolve the host. Ensure that the API endpoints support proper CORS when using a dynamic host value. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. frontend/components/shared/traces/trace-view.tsx:150
- Draft comment:
Integration of 'initialSpanId' and 'disableRouting' props works as expected. Consider adding inline documentation for these new props to clarify their purpose. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_OoRlxk45jaqa0SAG
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| setSelectedSpan({ ...span, collapsed: false }); | ||
| } | ||
| }, []); | ||
| }, [initialSpanId, searchParams, setSelectedSpan, spans]); |
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.
Bug: Span selection reset on URL parameter changes
The second useEffect has searchParams and spans in its dependency array, causing it to re-run whenever URL parameters change. When disableRouting is true (as in the embed component), user span selections are not persisted to the URL, so any searchParams change will reset the selection back to initialSpanId or the first span, losing the user's interactive selection. Previously this logic only ran on mount with an empty dependency array.
|
|
||
| if (!spansRes.ok) { | ||
| const text = await spansRes.text(); | ||
| throw new Error(text || "Failed to load spans."); |
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.
Bug: Error responses parsed as text display raw JSON
When API requests fail, the error responses are read using text() but the API endpoints return JSON objects like { error: "message" }. This causes the displayed error message to show raw JSON strings like {"error":"Trace not found"} to users instead of just the error message. The code should parse the JSON response and extract the error field.
Note
Adds a TraceEmbed component for MDX blogs and a shared spans API, updating TraceView to support embedding via initial span selection and disabled routing.
components/embeds/trace-embed.tsx: NewTraceEmbedthat fetches sharedtraceandspans, supportsspanId,host,previewOnly, and fixedheight, and rendersTraceViewwith routing disabled.app/api/shared/traces/[traceId]/spans/route.ts: NewGETendpoint returning shared spans withZodErrorand generic error handling.app/blog/[slug]/page.tsx: MDX components maptrace/Tracetags toTraceEmbedfor inline trace embeds.components/shared/traces/trace-view.tsx: AcceptsinitialSpanIdanddisableRouting; initializes selection frominitialSpanIdorspanIdquery; suppresses URL updates whendisableRoutingis true; minor effect dependency updates.Written by Cursor Bugbot for commit 94c7843. This will update automatically on new commits. Configure here.
Important
Adds
TraceEmbedcomponent for embedding trace views in blog posts, with new API route for fetching spans and updates toTraceViewfor initial span selection and routing control.TraceEmbedcomponent intrace-embed.tsxfor embedding trace views with props liketraceId,spanId,host, andpreviewOnly.GEThandler inroute.tsfor/api/shared/traces/[traceId]/spansto fetch spans bytraceId.ZodErrorwith 400 status and other errors with 500 status.page.tsxto includeTraceEmbedin blog posts, allowing<trace>and<Trace>tags.trace-view.tsxto acceptinitialSpanIdanddisableRoutingprops for initial span selection and routing control.This description was created by
for 94c7843. You can customize this summary. It will automatically update as commits are pushed.