Stack Trace and Breadcrumbs table polish#112
Conversation
🦋 Changeset detectedLatest commit: ec9bf7b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
4f53f2d to
f5b4a61
Compare
MikeShi42
left a comment
There was a problem hiding this comment.
Mainly just style thoughts, two other things:
Do you think we can switch displayedTab in the LogSidePanel to show the trace view if it's an exception by default? The parsed property tab is still kind of useful (mainly for us for debugging I suspect 😅) but the trace view is much better.
Minor scope creep, do you think you could add a ?? '[]' fallback case in the <ExceptionSubpanel breadcrumbs={JSON.parse... call? It looks like those properties might not exist sometimes.
I can create a follow up ticket otherwise.
| tr { | ||
| td { | ||
| padding: 6px 12px; | ||
| border-bottom: 1px solid $slate-950; |
There was a problem hiding this comment.
thoughts on leaving out the border for some of the network panel stuff? it feels a bit cleaner/less busy
| }; | ||
|
|
||
| export const StacktraceRow = ({ row }: { row: Row<StacktraceFrame> }) => { | ||
| const [lineContextOpen, setLineContextOpen] = React.useState(false); |
There was a problem hiding this comment.
I think it's a bit more intuitive to have the line context open (as it feels like the most helpful part!)
| <Table | ||
| hideHeader | ||
| columns={stacktraceColumns} | ||
| data={firstException.stacktrace?.frames ?? []} |
There was a problem hiding this comment.
I wonder if we should reverse the frames here so that the more specific and likely user-land code is on top?
| </SectionWrapper> | ||
| </CollapsibleSection> | ||
|
|
||
| <CollapsibleSection title="Breadcrumbs" initiallyCollapsed> |
There was a problem hiding this comment.
I also think it'd be nice if this was default open
| header: 'Message', | ||
| size: UNDEFINED_WIDTH, | ||
| cell: ({ row }) => | ||
| row.original.message || <span className="text-slate-500">Empty</span>, |
There was a problem hiding this comment.
I think it'd be nice here if can also make use of the fetch/xhr categories, as they currently show up as empty (maybe a follow up PR?) Here's a few examples if helpful of the data structure I see:
{
"category": "fetch",
"data": {
"method": "GET",
"status_code": 200,
"url": "http://localhost:8080/__nextjs_original-stack-frame"
},
"timestamp": 1700379487.918,
"type": "http"
}
{
"category": "xhr",
"data": {
"method": "POST",
"status_code": 200,
"url": "http://localhost:4318/v1/traces"
},
"timestamp": 1700379491.015,
"type": "http"
}
There was a problem hiding this comment.
I'll do it in a separate pr
| @@ -0,0 +1,333 @@ | |||
| import * as React from 'react'; | |||
There was a problem hiding this comment.
I'm curious how do we differentiate what should belong here vs LogSidePanel? Is this like a generic util elements for the LogSidePanel?
There was a problem hiding this comment.
yep, that's the idea
b4ca772 to
0da725f
Compare
MikeShi42
left a comment
There was a problem hiding this comment.
I'm assuming we'll leave changing displayedTab to trace for exceptions for a follow up PR as well?
| 'breadcrumbs', | ||
| ) | ||
| ], | ||
| ) ?? [] |
There was a problem hiding this comment.
Actually I believe in this case we need to do
JSON.parse(
selectedLogData?.['string.values']?.[
selectedLogData?.['string.names']?.indexOf(
'breadcrumbs',
)
] ?? '[]')
as the undefined value into JSON.parse will throw a parse error
There was a problem hiding this comment.
good point. moved JSON.parse calls into separate memo'd consts with try/catch block just in case
There was a problem hiding this comment.
I'm assuming we'll leave changing displayedTab to trace for exceptions for a follow up PR as well?
yes, will do in a separate PR, this one already becomes a bit unwieldy :D
The dashboard activeTabs URL state goes through parseAsJsonEncoded
(packages/app/src/utils/queryParsers.ts), which intentionally
double-encodes the JSON to survive Microsoft Teams' '+' -> '%2B'
re-encoding. The serializer writes encodeURIComponent(JSON.stringify(...))
and nuqs's URL machinery then encodes the '%XX' sequences a second
time. URLSearchParams.get(...) decodes one level, so the helper has
to decode the second level itself before JSON.parse.
The previous helper read the param, called JSON.parse on a still-
URL-encoded string ('%7B%22..."), threw, and returned {} silently.
Tests #112 and #197 polled getActiveTabsParam()[id] for 5s, got
undefined every time, and timed out on toBeTruthy.
Fix: decodeURIComponent before JSON.parse. Keep a fallback to plain
JSON.parse for compatibility with the old single-encoded format,
mirroring the parser's own backward-compat path.
The implementation (handleTabChange / setUrlActiveTabs in DBDashboardPage)
was correct all along; only the test-side reader needed updating.
The dashboard activeTabs URL state goes through parseAsJsonEncoded
(packages/app/src/utils/queryParsers.ts), which intentionally
double-encodes the JSON to survive Microsoft Teams' '+' -> '%2B'
re-encoding. The serializer writes encodeURIComponent(JSON.stringify(...))
and nuqs's URL machinery then encodes the '%XX' sequences a second
time. URLSearchParams.get(...) decodes one level, so the helper has
to decode the second level itself before JSON.parse.
The previous helper read the param, called JSON.parse on a still-
URL-encoded string ('%7B%22..."), threw, and returned {} silently.
Tests #112 and #197 polled getActiveTabsParam()[id] for 5s, got
undefined every time, and timed out on toBeTruthy.
Fix: decodeURIComponent before JSON.parse. Keep a fallback to plain
JSON.parse for compatibility with the old single-encoded format,
mirroring the parser's own backward-compat path.
The implementation (handleTabChange / setUrlActiveTabs in DBDashboardPage)
was correct all along; only the test-side reader needed updating.
Log Side Panel polish
Screen.Recording.2023-11-18.at.6.12.03.PM.mov