Trace server improvements

[nforro]

• Use enum values of supported OTEL types directly
  This fixes span rendering in trace-server.
• Make spans linkable
  With these changes clicking on a span updates the URL and when accessing an URL with a span ID the corresponding span is scrolled to and focused. Sidebar navigation has been updated to use span IDs as well,
  for consistency.

[qodo-for-packit]

PR Summary by Qodo

Trace server: fix OTEL enum rendering and add deep-linkable spans

🐞 Bug fix ✨ Enhancement 🕐 20-40 Minutes

AI Description

• Convert Enum-valued OTEL attributes to raw values to prevent span rendering issues.
• Add span deep-linking: clicking a span updates the URL and supports load-and-scroll by span ID.
• Harden async view rendering with route guards to avoid stale updates during navigation.

Diagram

graph TD
  U([User]) --> R["Router (#hash)"] --> TD["Trace detail view"] --> API["Trace API"] --> SP[(Spans data)]
  TD --> UI["Span list + sidebar"] --> H["URL w/ spanId"]
  UI --> CSS["Highlight styles"]

  subgraph Legend
    direction LR
    _user([User]) ~~~ _ui["UI module"] ~~~ _api["API call"] ~~~ _data[(Data)]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use query params for span selection

• ➕ Avoids brittle path parsing/pop logic for optional spanId
• ➕ Easier to extend later (e.g., tab, time range, filters)
• ➖ Would require changing the existing hash route format and router logic
• ➖ Potentially breaking bookmarked URLs unless backward compatibility is added

2. Use history.pushState instead of replaceState

• ➕ Allows browser Back/Forward to traverse span selections naturally
• ➖ Could create excessive history entries when exploring many spans
• ➖ May be annoying unless throttled/debounced or only used for deliberate actions

3. Centralize URL update + highlight into a single helper

• ➕ Reduces duplicated hash parsing and highlight clearing across span row/sidebar/jump button
• ➕ Lowers risk of subtle inconsistencies in future updates
• ➖ Primarily a maintainability refactor; current changes are already functional
• ➖ May slightly increase up-front complexity for a small codebase

Recommendation: Current approach (optional spanId in the hash path + replaceState) is pragmatic and keeps routing consistent with existing URL structure. If this pattern grows, consider either (a) migrating span selection to a query parameter for robustness, or (b) extracting a small helper to update the trace URL and manage highlight state to avoid further duplication.

Files changed (3) +120 / -9

Enhancement (2) +110 / -4

app.jsAdd span deep-linking, focus/highlight behavior, and async route guards +100/-4

Add span deep-linking, focus/highlight behavior, and async route guards

• Extends trace routing to accept an optional spanId and passes it into trace rendering so the target span can be scrolled into view and highlighted on load. Clicking span rows/sidebar items updates the URL to include the selected span ID and briefly highlights the selected element. Adds view/route guards after async API calls to avoid updating the DOM when the user has navigated away.

trace_server/static/app.js

style.cssIntroduce theme highlight color and hover affordance for span headers +10/-0

Introduce theme highlight color and hover affordance for span headers

• Adds a '--highlight-bg' theme variable for both light/dark modes. Styles span row headers with hover background and minor spacing/transition to reinforce clickability and highlight behavior.

trace_server/static/style.css

Bug fix (1) +10 / -5

openinference-streaming.patchNormalize Enum-valued attributes before OTEL type checks +10/-5

Normalize Enum-valued attributes before OTEL type checks

• Adds Enum handling so span attribute values that are Enums are converted to their underlying '.value'. This prevents non-OTEL attribute types from leaking into span attributes and fixes downstream span rendering expectations.

openinference-streaming.patch

[qodo-for-packit]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Global enum prefix stripping ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New

Description

getVal() now strips the "OpenInferenceSpanKindValues." prefix from any OTLP stringValue, so
unrelated string attributes can be silently modified in the UI when they happen to start with that
prefix. This change was intended for span-kind rendering but applies to all extracted string
attributes used throughout the viewer.

Code

trace_server/static/app.js[R17-20]

function getVal(value) {
  if (!value || typeof value !== 'object') return null;
-  for (const k of ['stringValue', 'intValue', 'boolValue', 'doubleValue']) {
+  if ('stringValue' in value) return stripEnumPrefix(value.stringValue);
+  for (const k of ['intValue', 'boolValue', 'doubleValue']) {

Relevance

●●● Strong

Team often accepts scoping/compat changes to avoid false positives and legacy-prefix side effects.

PR-#404
PR-#505
PR-#540

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The diff applies stripEnumPrefix() unconditionally for any stringValue extracted by getVal().
The same getVal() helper is used broadly across unrelated attributes (token counts, model name,
etc.), so this normalization affects far more than span kind.

trace_server/static/app.js[7-26]
trace_server/static/app.js[144-166]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`stripEnumPrefix()` is applied globally via `getVal()` for *all* OTLP `stringValue`s, not just the span-kind attribute. This can silently alter unrelated string attributes.

### Issue Context
This was added to make older spans (where enum values were stringified as `OpenInferenceSpanKindValues.MEMBER`) render consistently, but `getVal()` is a generic helper used across many different attributes.

### Fix Focus Areas
- trace_server/static/app.js[7-26]
- trace_server/static/app.js[144-166]

Suggested implementation direction (choose one):
- Make `stripEnumPrefix()` only strip when the entire string matches an enum-like value (e.g. `^OpenInferenceSpanKindValues\.[A-Z0-9_]+$`), not merely `startsWith`.
- Or, remove stripping from `getVal()` and apply it only at the call sites that read `openinference.span.kind`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. ID case mismatch ✓ Resolved 🐞 Bug ≡ Correctness

Description

parseTraceHash() lowercases traceId/spanId, but trace-server persists OTLP JSON IDs without
normalizing and filters with case-sensitive equality, so traces/spans with uppercase hex IDs will
fail to load or deep-link highlight.

Code

trace_server/static/app.js[R108-110]

+      spanId = parts.pop().toLowerCase();
+      traceId = parts.pop().toLowerCase();
+      issue = decodeURIComponent(parts.join('/'));

Relevance

●●● Strong

Uppercase IDs would break trace/span lookups; repo has precedent accepting case-insensitive
normalization fixes.

PR-#655
PR-#699

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The UI now forces lowercase IDs, while the backend stores OTLP JSON IDs unchanged and uses
case-sensitive equality to filter by trace_id; this creates a mismatch whenever ingested IDs contain
uppercase characters, causing empty query results and/or failing `getElementById('span-' +
targetSpanId)` lookups.

trace_server/static/app.js[88-115]
trace_server/static/app.js[859-862]
trace_server/server.py[343-383]
trace_server/server.py[483-540]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`parseTraceHash()` lowercases `traceId`/`spanId`, but the backend stores and filters IDs case-sensitively. If any OTLP JSON payload includes uppercase hex IDs, the UI will request a different `trace_id` than what is stored and span deep-links won’t match rendered row IDs.

## Issue Context
- UI now normalizes IDs to lowercase when parsing the URL.
- `trace_server/server.py` stores `traceId`/`spanId` from OTLP JSON as-is and filters with `si.trace_id = ?`.

## Fix Focus Areas
- trace_server/static/app.js[105-115]
- trace_server/server.py[343-383]
- trace_server/server.py[483-540]

## Suggested fix
1) **Backend canonicalization (preferred):** when extracting spans from OTLP JSON, normalize `traceId`, `spanId`, `parentSpanId` to lowercase *only when they match expected hex lengths/patterns* (32 for trace, 16 for span). This ensures DB/storage matches UI and avoids corrupting non-hex IDs.
2) Optionally, also normalize incoming `trace_id` query params to lowercase before filtering.
3) (Optional defense-in-depth) Ensure DOM row ids are generated from a canonical (lowercase) `span_id` too, so URL parsing and DOM lookup always agree.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Stale jumpBtn blocks recreation ✓ Resolved 🐞 Bug ≡ Correctness

Description

onScroll() creates the jump-to-bottom button only when jumpBtn is null, but route() removes the
.jump-bottom element from the DOM without clearing the global jumpBtn reference. If the user
navigates away from a trace while the button exists, returning to a trace can leave autoScroll
disabled with no visible jump button to recover.

Code

trace_server/static/app.js[R1279-1282]

+    if (!jumpBtn) {
+      jumpBtn = el('button', {
+        className: 'jump-bottom',
+        onClick: () => {

Relevance

●●● Strong

Deterministic state bug: removing DOM node without nulling jumpBtn prevents recreation; likely to be
fixed.

PR-#699
PR-#718

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

route() removes the jump button via a DOM query, but leaves the jumpBtn variable untouched;
later, both onScroll() and maybeAutoScroll() gate button creation on !jumpBtn, so a stale
reference prevents rendering the control.

trace_server/static/app.js[299-308]
trace_server/static/app.js[1268-1301]
trace_server/static/app.js[1304-1317]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`route()` removes the `.jump-bottom` element from the DOM, but does not reset the global `jumpBtn` variable. Since `onScroll()` (and `maybeAutoScroll()`) only create a new button when `!jumpBtn`, a detached-but-non-null `jumpBtn` prevents the button from ever being re-added.

### Issue Context
This is triggered when leaving a trace view while auto-scroll is disabled and the jump button exists, then returning to a trace view and scrolling (or relying on `maybeAutoScroll()`).

### Fix Focus Areas
- trace_server/static/app.js[299-308]
- trace_server/static/app.js[1268-1301]
- trace_server/static/app.js[1304-1317]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View review recommended (8)

4. Enum arrays stringify attributes ✓ Resolved 🐞 Bug ≡ Correctness

Description

In BeeAIInstrumentor’s OTEL attribute normalization, only top-level Enum values are unwrapped;
lists/tuples containing Enum elements fail the OTEL-type check and are coerced to a single string.
This changes attribute shape (array -> string) and can break downstream rendering/querying that
expects an array of primitives.

Code

openinference-streaming.patch[R85-88]

++            # Extract enum value if it's an enum
++            if isinstance(value, Enum):
++                value = value.value
++            # Convert to string if not an OTEL type

Relevance

●●● Strong

Likely real bug: Enum elements in arrays get coerced to string; team accepts shape-preserving
correctness fixes.

PR-#115

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new logic unwraps Enum only for scalar values. For list/tuple attributes, it validates
elements against _OTEL_TYPES without unwrapping element Enums, so any Enum element causes the
whole collection to be stringified via str(value).

openinference-streaming.patch[82-93]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`openinference-streaming.patch` adds Enum handling during attribute normalization, but it only unwraps when the attribute value itself is an `Enum`. If the value is a list/tuple containing Enums (e.g. `[MyEnum.A, MyEnum.B]`), the code will fall through to `str(value)`, converting an array attribute into a string.

### Issue Context
OpenTelemetry attributes support primitive scalars and arrays of primitives. The new Enum logic should also unwrap Enum elements inside arrays/tuples before validating/coercing.

### Fix Focus Areas
- openinference-streaming.patch[82-93]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Uppercase spanId won't scroll ✓ Resolved 🐞 Bug ≡ Correctness

Description

parseTraceHash() accepts uppercase hex span/trace IDs (case-insensitive regex) but returns them
without normalization, and the deep-link scroll uses `document.getElementById('span-' +
targetSpanId)`. If rendered span row IDs use a different casing (commonly lowercase from API/span
data), URLs containing uppercase IDs won’t match and the scroll/highlight will fail.

Code

trace_server/static/app.js[R95-97]

+  const isTraceId = s => /^[0-9a-f]{32}$/i.test(s);
+  const isSpanId = s => /^[0-9a-f]{16}$/i.test(s);
+

Relevance

●●● Strong

Small deterministic fix (normalize hex IDs) to prevent deep-link lookup mismatch; similar
case-insensitive handling accepted before.

PR-#655

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The parser explicitly accepts mixed-case IDs (/i) and returns them as-is. The deep-linking logic
then uses targetSpanId directly in getElementById('span-' + targetSpanId), while span row
elements are identified by 'span-' + span.span_id (coming from span data), so a casing mismatch
causes lookup failure.

trace_server/static/app.js[88-125]
trace_server/static/app.js[586-602]
trace_server/static/app.js[856-860]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`parseTraceHash()` validates span/trace IDs using case-insensitive regexes but does not canonicalize the returned IDs. Deep-link lookup uses the raw `targetSpanId` as part of a DOM element ID, which is typically constructed from `span.span_id` values.

### Issue Context
To make deep links robust, normalize `traceId` and `spanId` to a consistent casing (e.g. lowercase) when parsing (and/or before DOM lookup / API calls).

### Fix Focus Areas
- trace_server/static/app.js[88-125]
- trace_server/static/app.js[586-602]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Guard ignores issue ✓ Resolved 🐞 Bug ≡ Correctness

Description

The new stale-response guards in renderTraceDetail()/pollNewSpans() only compare
state.currentTraceId, but the underlying request is also scoped by issue, so a response for a
different issue can still be applied if it shares the same traceId. This can overwrite state.spans
and render the wrong spans after fast navigation between issues that reference the same trace_id.

Code

trace_server/static/app.js[R527-528]

    const data = await api.spans(issue, {traceId: traceId});
+    if (state.view !== 'trace' || state.currentTraceId !== traceId) return;

Relevance

●●● Strong

Correctness race: team commonly accepts tightening guards to prevent cross-context async/concurrency
state corruption.

PR-#700
PR-#657

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code added a stale-response guard but it only checks currentTraceId; however, the request is
made for a specific issue, and the backend data model explicitly allows a single trace_id to be
linked to multiple issues, so traceId alone is insufficient to disambiguate navigation races.

trace_server/static/app.js[312-318]
trace_server/static/app.js[523-531]
trace_server/static/app.js[617-621]
trace_server/server.py[12-14]
trace_server/server.py[146-151]
trace_server/server.py[527-540]
trace_server/server.py[681-689]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The trace view’s new async “stale response” guards only check `state.view` and `state.currentTraceId`, but the fetch is performed against `/traces/<issue>?trace_id=...`, so `issue` must also match to safely apply results.

### Issue Context
A single `trace_id` can be associated with multiple Jira issues (see `span_issues` and `query_recent_traces()` returning multiple issues per trace). If a user navigates quickly between two issues that both reference the same trace, an older response can still pass the current guard and overwrite the UI.

### Fix Focus Areas
- trace_server/static/app.js[523-531]
- trace_server/static/app.js[611-614]
- trace_server/static/app.js[617-621]
- trace_server/static/app.js[588-600]

### Suggested fix
1. In `renderTraceDetail()`, change the post-fetch guard to also require `state.currentIssue === issue` (capture `expectedIssue = issue` similarly to `expectedTraceId` if you prefer).
2. Apply the same issue+trace guard in the `catch` block and in `pollNewSpans()`.
3. In the `requestAnimationFrame` callback for `targetSpanId`, also ensure `state.currentIssue` matches before scrolling/highlighting.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. Jump links wrong span ✓ Resolved 🐞 Bug ≡ Correctness

Description

The jump-to-bottom button writes a spanId to the URL using state.spans[state.spans.length - 1],
but state.spans is assigned from the API response without sorting on initial load, so the last
array element is not guaranteed to be the bottom-most rendered span. This can produce a valid URL
that, on reload/share, scrolls/highlights a different span than the one at the bottom of the page.

Code

trace_server/static/app.js[R1285-1288]

+          if (parsed && state.spans.length > 0) {
+            const lastSpanId = state.spans[state.spans.length - 1].span_id;
+            const newHash = '#/trace/' + encodeURIComponent(parsed.issue) + '/' + parsed.traceId + '/' + lastSpanId;
+            history.replaceState(null, '', newHash);

Relevance

●●● Strong

Correctness bug in new linkable-span feature; team likely fixes deep-link target deterministically.

PR-#699
PR-#718

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new jump button logic selects the last element of state.spans, but on initial load
state.spans is taken directly from the API without sorting; meanwhile the UI order is driven by
buildSpanTree, which explicitly sorts nodes by start_time. Therefore, the array's last element
can differ from the last rendered row, leading to incorrect deep links.

trace_server/static/app.js[1278-1293]
trace_server/static/app.js[523-531]
trace_server/static/app.js[700-784]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The jump-to-bottom handler updates the URL using `state.spans[state.spans.length - 1].span_id`, but `state.spans` is not guaranteed to be ordered the same as the rendered span list. This can generate deep links that scroll to the wrong span on page load.

### Issue Context
- On initial trace load, `state.spans` is assigned directly from the API response (no sort).
- Rendering uses `buildSpanTree(...)` which sorts roots/children by `start_time`, so visual order can differ from the raw API array.
- The new feature specifically aims to make spans linkable; writing the wrong spanId undermines this.

### Fix Focus Areas
- trace_server/static/app.js[1282-1290]

### Suggested fix
In the jump button `onClick`, derive the target spanId from the rendered DOM (or from the same ordering used for rendering), e.g.:
- Find the last rendered `.span-row` (e.g., `document.querySelector('.span-list .span-row:last-child')`), parse its id (`span-<id>`), and write that id into the URL.
- Keep the existing guards (`parsed` and `state.spans.length > 0`) and add a guard if the DOM query returns null.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

8. decodeURIComponent can crash routing ✓ Resolved 🐞 Bug ☼ Reliability

Description

parseTraceHash() calls decodeURIComponent() on the hash-derived issue segment without guarding
against URIError. A malformed percent-encoded hash can throw and break routing (and span/sidebar
click handlers that call parseTraceHash(location.hash)).

Code

trace_server/static/app.js[R107-110]

+    spanId = parts.pop();
+    traceId = parts.pop();
+    issue = decodeURIComponent(parts.join('/'));
+  } else if (isTraceId(lastPart)) {

Relevance

●●● Strong

Team often accepts defensive parsing/error-guards to prevent crashes from malformed inputs.

PR-#450
PR-#488

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new helper decodes URL components directly and is called from route(); without a try/catch,
malformed hashes can throw and abort route().

trace_server/static/app.js[88-119]
trace_server/static/app.js[293-313]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`decodeURIComponent()` can throw `URIError` on malformed percent-encoding, and `parseTraceHash()` currently does not catch it.

### Issue Context
`parseTraceHash(location.hash)` is invoked from routing and multiple click handlers; a thrown exception can prevent navigation/highlighting and leave the app in a broken state until reload.

### Fix Focus Areas
- trace_server/static/app.js[88-119]
- trace_server/static/app.js[306-313]

### Suggested fix
- Wrap the `decodeURIComponent(...)` calls in a `try/catch` inside `parseTraceHash()`.
- On `URIError`, return `null` (or a structured error) so callers can fall back gracefully.
- Optionally, log to console for debugging (without throwing).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

9. Blank screen on invalid trace ✓ Resolved 🐞 Bug ≡ Correctness

Description

route() clears the app container, but when the hash starts with "#/trace/" and parseTraceHash()
returns null, it renders nothing and leaves state.view/currentTraceId potentially stale. This can
strand users on an empty page for malformed/stale trace links.

Code

trace_server/static/app.js[R307-310]

+    const parsed = parseTraceHash(hash);
+    if (parsed) {
+      state.view = 'trace';
+      state.currentIssue = parsed.issue;

Relevance

●●● Strong

Graceful fallback on invalid/partial inputs aligns with prior accepted robustness fixes in
routing/workflow paths.

PR-#555
PR-#450

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

route() clears the UI (app.innerHTML = '') before the trace branch, but only sets view/renders when
parseTraceHash succeeds; otherwise it does nothing, leaving an empty app and potentially stale
state.

trace_server/static/app.js[293-325]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
When `location.hash` begins with `#/trace/` but `parseTraceHash()` returns `null`, the router has already cleared `#app` and stopped polling, but does not render any view or error.

### Issue Context
This is a regression introduced by the new conditional `if (parsed) { ... }` block.

### Fix Focus Areas
- trace_server/static/app.js[293-325]

### Suggested fix
- Add an `else` branch for the `#/trace/` route to either:
 - render an error banner (e.g., “Invalid trace URL”), and/or
 - fall back to the default `recent` view (`state.view='recent'` + `renderRecent(app)`), and clear `state.currentIssue/currentTraceId`.
- Ensure `updateNav()` reflects the chosen fallback view.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

10. Ambiguous spanId parsing ✓ Resolved 🐞 Bug ≡ Correctness

Description

route() (and the span/sidebar/jump handlers) infer spanId presence purely from parts.length >= 3,
which mis-parses hashes where the issue contains an unencoded '/' and there is no spanId (e.g.
#/trace/org/repo/<traceId> becomes issue=org, traceId=repo, spanId=<traceId>). This is a
regression versus the prior behavior and can break manual/legacy deep links and also cause click
handlers to rewrite the URL to an incorrect trace.

Code

trace_server/static/app.js[R274-277]

    const parts = hash.slice(8).split('/');
+    const spanId = parts.length >= 3 ? parts.pop() : null;
    const traceId = parts.pop();
    const issue = decodeURIComponent(parts.join('/'));

Relevance

●●● Strong

Correctness regression in URL parsing can break legacy/manual deep links; likely they’ll tighten
spanId detection.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new logic pops a spanId whenever there are 3+ segments, which is correct only if the issue is
encoded into a single path segment. The codebase itself indicates issues may contain '/', since it
encodes issues when generating trace URLs; unencoded slash-containing issues are therefore a
realistic input for manual/legacy URLs and become ambiguous under the new heuristic.

trace_server/static/app.js[273-281]
trace_server/static/app.js[432-440]
trace_server/static/app.js[816-830]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The trace router and several click handlers decide whether a spanId is present by checking `parts.length >= 3` and popping the last segment as `spanId`. This breaks hashes where the issue portion contains one or more unencoded `/` characters and the URL has no spanId (3+ segments).

### Issue Context
The app generally generates trace links with `encodeURIComponent(issue)`, but users may still have old/manual bookmarks or external links with unencoded slashes in the issue. With the new optional `/<spanId>` suffix, segment-count parsing becomes ambiguous and can mis-route.

### Fix Focus Areas
- trace_server/static/app.js[273-281]
- trace_server/static/app.js[824-833]
- trace_server/static/app.js[1157-1164]
- trace_server/static/app.js[1245-1253]

### Implementation notes
Use an unambiguous rule to detect presence of spanId/traceId, e.g.:
- Treat the last segment as `spanId` only if it matches OTEL span-id format (typically 16 lowercase hex) *and* the previous segment matches trace-id format (typically 32 lowercase hex).
- Otherwise, treat the last segment as `traceId` and `spanId = null`.
Apply the same parsing helper in route() and in the onClick handlers to avoid divergence.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

11. Unguarded span scroll callback ✓ Resolved 🐞 Bug ☼ Reliability

Description

renderTraceDetail() schedules a requestAnimationFrame scroll/highlight for targetSpanId
without re-checking state.view/state.currentTraceId inside the callback. If the user navigates
to another trace/view before the callback runs, it can scroll/highlight the wrong element (or leave
global autoScroll disabled) in the new view.

Code

trace_server/static/app.js[R541-544]

+    if (targetSpanId) {
+      autoScroll = false;
+      requestAnimationFrame(() => {
+        const target = document.getElementById('span-' + targetSpanId);

Relevance

●●● Strong

Team often accepts async/race-condition hardening; adding state/view guard in deferred callback is
low-risk reliability fix.

PR-#675
PR-#540

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The function guards the async fetch result and error path with view/trace checks, but the new RAF
block that scrolls/highlights lacks those checks and will run even if the route changes after
scheduling.

trace_server/static/app.js[482-484]
trace_server/static/app.js[541-555]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The new `requestAnimationFrame` callback in `renderTraceDetail()` performs DOM operations and updates global state (`autoScroll`, `currentHighlightedSpan`) without confirming the app is still on the same trace.

### Issue Context
Async fetch paths already guard against stale updates (`if (state.view !== 'trace' || state.currentTraceId !== traceId) return;`), but that guard does not apply to the later RAF callback.

### Fix Focus Areas
- trace_server/static/app.js[541-555]

### Suggested fix
- Capture `const expectedTraceId = traceId;` (and optionally `expectedView = 'trace'`) and add a guard inside the RAF callback:
 - `if (state.view !== 'trace' || state.currentTraceId !== expectedTraceId) return;`
- Optionally defer `autoScroll = false` until after the guard passes (inside the callback) to avoid leaking stale state across navigation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

12. Highlight ref not cleared ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New

Description

The new global currentHighlightedSpan reference is not reset during navigation in route(), so
after route() clears #app it can temporarily retain a detached DOM subtree until the 2s highlight
timeout runs. This is avoidable memory retention introduced by the new highlight feature.

Code

trace_server/static/app.js[R318-320]

  document.querySelector('.jump-bottom')?.remove();
+  jumpBtn = null;

Relevance

●●● Strong

Small deterministic cleanup to avoid detached DOM retention; repo commonly accepts resource/cleanup
hardening.

PR-#450
PR-#675
PR-#590

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

route() clears the view but only resets jumpBtn, not the newly added currentHighlightedSpan.
The click handler assigns currentHighlightedSpan = row, so without a navigation reset the detached
element can remain referenced until the timeout clears it.

trace_server/static/app.js[310-320]
trace_server/static/app.js[870-893]
trace_server/static/app.js[1273-1276]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`route()` clears the app container on navigation but doesn’t reset the new `currentHighlightedSpan` global, which can keep a detached span row alive briefly after navigation.

### Issue Context
Click handlers assign DOM nodes to `currentHighlightedSpan` and clear them later via `setTimeout(..., 2000)`. If the user navigates away during that window, the detached subtree remains referenced.

### Fix Focus Areas
- trace_server/static/app.js[310-320]
- trace_server/static/app.js[876-894]
- trace_server/static/app.js[1273-1276]

Suggested fix:
- In `route()`, after clearing the view (`app.innerHTML = ''`), set `currentHighlightedSpan = null` (and optionally clear any highlight styles if needed).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

13. Invalid URL banner cleared ✓ Resolved 🐞 Bug ◔ Observability

Description

In the invalid-trace branch, route() appends an "Invalid trace URL" error banner and then calls
renderRecent(app), but renderRecent() later clears container.innerHTML on both success and error.
This makes the invalid-link banner transient (it disappears after the recent-traces request
completes).

Code

trace_server/static/app.js[R324-325]

+      app.appendChild(el('div', {className: 'error-banner'}, 'Invalid trace URL'));
+      renderRecent(app);

Relevance

●●● Strong

UI/UX bug: error banner gets wiped by renderRecent() innerHTML clear; easy to preserve banner.

PR-#699
PR-#718

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The invalid-trace branch appends an .error-banner then immediately calls renderRecent(app).
renderRecent() clears container.innerHTML after the fetch (and also in its catch path), which
removes previously appended children including the banner.

trace_server/static/app.js[312-326]
trace_server/static/app.js[398-447]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The invalid-trace banner is appended before calling `renderRecent(app)`, but `renderRecent()` clears the container after its async fetch resolves/rejects, removing the banner.

### Issue Context
Users following malformed trace links briefly see an error but then it disappears, reducing clarity about why they were redirected.

### Fix Focus Areas
- trace_server/static/app.js[312-326]
- trace_server/static/app.js[398-447]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

14. Invalid trace URL unstyled ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

When parseTraceHash() fails, route() renders an "Invalid trace URL" message with `className:
'error', but the stylesheet defines .error-banner and no .error` rule. The new invalid-trace
message is therefore likely to appear unstyled/inconsistent with other errors.

Code

trace_server/static/app.js[324]

+      app.appendChild(el('div', {className: 'error'}, 'Invalid trace URL'));

Relevance

●●● Strong

Trivial UX/style consistency fix: use existing error styling class; usually accepted for user-facing
polish.

PR-#651

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The invalid-trace route branch uses className: 'error'. The CSS file defines styling for
.error-banner but does not define .error, so this message won’t get the intended error styling.

trace_server/static/app.js[312-326]
trace_server/static/style.css[492-499]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The invalid-trace URL path renders an error message with a CSS class (`error`) that is not defined in `style.css`, unlike other error messages which use `error-banner`.

### Issue Context
This is an error path added to avoid a blank screen; using the consistent class keeps error presentation uniform.

### Fix Focus Areas
- trace_server/static/app.js[319-326]
- trace_server/static/style.css[492-499]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View optional (1)

15. Auto-scroll disabled without target ✓ Resolved 🐞 Bug ≡ Correctness

Description

When a spanId is provided in the hash but no element matches it, renderTraceDetail() still sets
autoScroll=false before checking whether the target exists. This can unexpectedly disable live
auto-follow behavior for deep links with stale/invalid span IDs.

Code

trace_server/static/app.js[R576-579]

+        if (state.view !== 'trace' || state.currentTraceId !== expectedTraceId) return;
+        autoScroll = false;
+        const target = document.getElementById('span-' + targetSpanId);
+        if (target) {

Relevance

●●● Strong

Small, local UX correctness fix; avoids disabling behavior on invalid target, low risk.

PR-#584
PR-#555

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

autoScroll is set to false before the code checks whether the target element exists, so an invalid
spanId still turns off auto-follow.

trace_server/static/app.js[571-589]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The deep-link highlight code disables `autoScroll` unconditionally, even if the target span element isn’t found.

### Issue Context
This affects `#/trace/<issue>/<traceId>/<spanId>` links where the spanId is stale/incorrect.

### Fix Focus Areas
- trace_server/static/app.js[571-589]

### Suggested fix
- Move `autoScroll = false` inside the `if (target) { ... }` block, or
- If `target` is missing, keep `autoScroll` unchanged (or explicitly restore it) and consider clearing the spanId from the URL or showing a small “span not found” notice.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context

✅ Compliance rules (platform): 7 rules

Tip of the day

💡 Did you know, you can group findings by type and pick your Finding display, from Minimal to Full

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit 34623dd

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit 67653ce

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit 20b2c10

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit 3629362

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit b207808

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit 8924301

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit 9d947bd

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit 193b428

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit 9a7896c

[nforro]

/agentic_review

[qodo-for-packit]

Code review by qodo was updated up to the latest commit c4cd9a3

[lbarcziova]

LGTM, don't see any issues, thanks!