[refactor] Annotation and trace helpers move into @agenta/entities - #5954
[refactor] Annotation and trace helpers move into @agenta/entities#5954ardaerzin wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
size:XXL
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesObservability extraction foundation
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: 🟡 Moderate · up to This refactor moves annotation and trace behavior into shared packages, but the current implementation can mishandle missing annotations, query traces through the wrong integration contract, or select the wrong trace node when metadata contains a matching ID. These issues should be resolved before merging. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
web/packages/agenta-shared/src/utils/attachments.ts (1)
1-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce these comments to one short line.
web/packages/agenta-shared/src/utils/attachments.ts#L1-L6: Replace the five-line block with one short summary line.web/packages/agenta-shared/src/utils/typeNarrowing.ts#L30-L33: Replace the four-line block with one short summary line.As per coding guidelines, “Keep in-code comments to at most one short line; use longer comments only for genuinely surprising constraints such as bugs, races, or ordering requirements.”
Source: Coding guidelines
web/packages/agenta-entities/tests/unit/annotation-dto-helpers.test.ts (1)
3-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse package entry points in this test.
These direct
srcimports bypass the package export contract. Import annotation DTO symbols from@agenta/entities/annotation/dto. ImportgetNodeByIdfrom@agenta/entities/trace. If an export is missing, add it to the corresponding public entry point.As per coding guidelines, use exported subpath imports for tree-shaking.
Sources: Coding guidelines, Learnings
web/packages/agenta-entities/src/annotation/dto/helpers.ts (1)
179-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn recursively annotated child traces.
Define a recursive annotated trace type, constrain
TtoAnnotatableTrace, and return that type. This exposes annotation fields on child traces and removes the cast inannotation-dto-helpers.test.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 54b180a5-c719-4fee-ace2-bbac6eb4737d
📒 Files selected for processing (24)
docs/design/observability-packages/KICKOFF.mddocs/design/observability-packages/plan.mdweb/oss/src/lib/helpers/utils.tsweb/oss/src/lib/hooks/useAnnotations/assets/transformer.tsweb/oss/src/lib/hooks/useAnnotations/index.tsweb/oss/src/lib/hooks/useAnnotations/types/index.tsweb/oss/src/lib/traces/observability_helpers.tsweb/oss/src/services/annotations/api/index.tsweb/packages/agenta-entities/package.jsonweb/packages/agenta-entities/src/annotation/dto/api.tsweb/packages/agenta-entities/src/annotation/dto/helpers.tsweb/packages/agenta-entities/src/annotation/dto/index.tsweb/packages/agenta-entities/src/annotation/dto/transformer.tsweb/packages/agenta-entities/src/annotation/dto/types.tsweb/packages/agenta-entities/src/organization/index.tsweb/packages/agenta-entities/src/organization/state.tsweb/packages/agenta-entities/src/trace/index.tsweb/packages/agenta-entities/src/trace/utils/index.tsweb/packages/agenta-entities/src/trace/utils/nodeTree.tsweb/packages/agenta-entities/tests/unit/annotation-dto-helpers.test.tsweb/packages/agenta-shared/src/utils/attachments.tsweb/packages/agenta-shared/src/utils/download.tsweb/packages/agenta-shared/src/utils/index.tsweb/packages/agenta-shared/src/utils/typeNarrowing.ts
💤 Files with no reviewable changes (4)
- web/oss/src/lib/hooks/useAnnotations/assets/transformer.ts
- web/oss/src/lib/helpers/utils.ts
- web/oss/src/lib/hooks/useAnnotations/types/index.ts
- web/oss/src/lib/traces/observability_helpers.ts
| > **Not yet decided:** the observability session row (WP5) and the `useEvaluatorReference` | ||
| > subtree (WP3) — both flagged inline, both need an answer before their WP starts. | ||
| > **To execute this plan, start from `KICKOFF.md` in this directory.** | ||
| > **WP0–WP3 are done** (empty states deferred within WP3 — see below) (§9 re-verified on `079bc20be4`; all numbers reproduced). Corrections it | ||
| > found are folded in below and listed in the table at the end of §9. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Publish one authoritative D1 decision.
The plan header and kickoff gate say D1 is open, while the WP3 audit says D1 is resolved by injecting the evaluator label.
docs/design/observability-packages/plan.md#L6-L10: update the status summary.docs/design/observability-packages/plan.md#L333-L335: retain the resolveddisplayNamecontract if it is final.docs/design/observability-packages/KICKOFF.md#L46-L60: remove the stale D1 blocker and option list.
📍 Affects 2 files
docs/design/observability-packages/plan.md#L6-L10(this comment)docs/design/observability-packages/plan.md#L333-L335docs/design/observability-packages/KICKOFF.md#L46-L60
| > **Corrected — there are no re-export shims.** `web/eslint.config.mjs` bans | ||
| > `export … from "@agenta/*"` in `oss/src/**` and `ee/src/**` (`no-restricted-syntax`, for | ||
| > tree-shaking). Every WP in this stack must therefore **rewrite the call sites** to import | ||
| > from the package and delete the old OSS module, rather than leaving a shim behind. WP0 | ||
| > rewrote 30 import sites across 25 files. Budget for this in every later WP — the plan's | ||
| > "nothing else in OSS moves in this PR" framing does not hold. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the extraction and compatibility rules across both documents.
The documents currently describe incompatible WP0/WP1 migrations and shim policies.
docs/design/observability-packages/plan.md#L158-L163: keep the direct-import and deletion rule.docs/design/observability-packages/plan.md#L199-L203: remove the value re-export-shim instruction.docs/design/observability-packages/KICKOFF.md#L68-L75: use the corrected DTO/API andgetNodeByIdscope.docs/design/observability-packages/KICKOFF.md#L142-L147: remove the requirement for old-path value shims.
📍 Affects 2 files
docs/design/observability-packages/plan.md#L158-L163(this comment)docs/design/observability-packages/plan.md#L199-L203docs/design/observability-packages/KICKOFF.md#L68-L75docs/design/observability-packages/KICKOFF.md#L142-L147
| > **Split `assets/constants.ts` (770 LOC) — it holds two unrelated exports.** `FILTER_COLUMNS` | ||
| > (lines 37–705) is filter field metadata and belongs to this WP; `spanTypeStyles` (line 706+) | ||
| > is the span-category icon/colour map consumed by `AvatarTreeContent` and `NodeNameCell` and | ||
| > belongs to **WP3**. Moving the file whole into either WP creates a backwards dependency | ||
| > between them. Split it at the extraction, don't carry it. (Confirmed exactly: line 37 and | ||
| > line 706.) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use one WP owner for spanTypeStyles.
Lines 269-274 assign spanTypeStyles to WP3. Line 330 calls it a WP2 split. Change the dependency audit to say WP3. Otherwise the sequence can move this symbol before its intended package boundary.
Also applies to: 330-330
| cd web/oss/src/components/pages/observability | ||
| for f in components/*.tsx components/SessionsTable/components/Cells/*.tsx; do | ||
| d=$(grep -h '@/oss/' "$f" | sed 's/.*"\(@\/oss[^"]*\)".*/\1/' | tr '\n' ' ') | ||
| [ -n "$d" ] && echo "$f → $d" | ||
| done | ||
|
|
||
| # §8 the table | ||
| D=web/packages/agenta-ui/src/InfiniteVirtualTable | ||
| find $D -type f | xargs wc -l | tail -1 # 13295 total | ||
| grep -rl 'from "antd"' $D | wc -l # 12 runtime importers | ||
| grep -rl 'ColumnsType\|ColumnType\|antd/es/table' $D | wc -l # 22 type-coupled | ||
| grep -rln '@agenta/ui/table' web/oss/src web/ee/src web/packages/*/src | wc -l # 82 consumers | ||
| grep -rln '\.ant-table' web/oss/src web/ee/src web/packages/*/src | wc -l # 18 DOM-coupled |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the validation commands use consistent working directories.
The plan changes directory and does not restore it. The kickoff block runs from web/ but uses a repository-root path.
docs/design/observability-packages/plan.md#L642-L654: use a subshell or restore the repository root before settingD.docs/design/observability-packages/KICKOFF.md#L83-L92: usepackages/agenta-observability-ui/srcfromweb/, or run the block from the repository root.
📍 Affects 2 files
docs/design/observability-packages/plan.md#L642-L654(this comment)docs/design/observability-packages/KICKOFF.md#L83-L92
| const response = await axios.post( | ||
| `${getAgentaApiUrl()}/simple/traces/query`, | ||
| mapAnnotationQueryToTrace(queries), | ||
| {params: projectId ? {project_id: projectId} : undefined}, | ||
| ) | ||
| return mapTraceResponseToAnnotation(response.data ?? {}) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline web/packages/agenta-sdk/src/resources.ts --items all
rg -n -C 3 'simple/traces|queryAllAnnotations|safeParseWithLogging' web/packages web/ossRepository: Agenta-AI/agenta
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- annotation DTO API ---'
cat -n web/packages/agenta-entities/src/annotation/dto/api.ts
printf '%s\n' '--- annotation DTO schemas and mappers ---'
rg -n -C 5 'annotationSchema|AnnotationDto|mapAnnotationQueryToTrace|mapTraceResponseToAnnotation|trace' \
web/packages/agenta-entities/src/annotation
printf '%s\n' '--- Fern resource accessor ---'
cat -n web/packages/agenta-sdk/src/resources.ts
printf '%s\n' '--- generated query method ---'
sed -n '1340,1425p' web/packages/agenta-api-client/src/generated/api/resources/traces/client/Client.ts
rg -n -C 8 'query.*trac|SimpleTrace.*Query|Query.*SimpleTrace|simple_traces_query|SimpleTracesQuery' \
web/packages/agenta-api-client/src/generated/api/resources/traces web/packages/agenta-api-client/src/generated/apiRepository: Agenta-AI/agenta
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact DTO API imports and callers ---'
sed -n '1,120p' web/packages/agenta-entities/src/annotation/dto/api.ts
sed -n '1,100p' web/oss/src/services/annotations/api/index.ts
rg -n -C 4 'queryAllAnnotations\(' web --glob '*.{ts,tsx}'
printf '%s\n' '--- generated request/response type definitions ---'
rg -n -C 12 'QueryTraces|queryTraces|QuerySimple|SimpleTrace' \
web/packages/agenta-api-client/src/generated/api/resources/traces \
web/packages/agenta-api-client/src/generated/api
printf '%s\n' '--- validation helper contract and annotation schemas ---'
rg -n -C 10 'function safeParseWithLogging|const safeParseWithLogging|export .*safeParseWithLogging' \
web/packages/agenta-entities/src web/packages/agenta-entities
rg -n -C 8 'annotationSchema|annotationsResponseSchema|AnnotationsResponse|Trace' \
web/packages/agenta-entities/src/annotation web/packages/agenta-entities/src --glob '*schema*'Repository: Agenta-AI/agenta
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- generated traces exports and method signatures ---'
rg -n 'export|query|Query|Trace' web/packages/agenta-api-client/src/generated/api/resources/traces \
--glob '*.ts' | head -160
printf '%s\n' '--- all SDK resource accessors and imports ---'
cat -n web/packages/agenta-sdk/src/resources.ts
printf '%s\n' '--- DTO API and nearby schemas ---'
cat -n web/packages/agenta-entities/src/annotation/dto/api.ts
find web/packages/agenta-entities/src/annotation -maxdepth 3 -type f -printRepository: Agenta-AI/agenta
Length of output: 32757
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- simple trace query method ---'
sed -n '1360,1425p' web/packages/agenta-api-client/src/generated/api/resources/traces/client/Client.ts
printf '%s\n' '--- generated type names and definitions ---'
rg -n 'Simple.*Query|Query.*Simple|SimpleTrace.*Response|TracesResponse|type .*Request|interface .*Request' \
web/packages/agenta-api-client/src/generated/api \
--glob '*.ts' | grep -E 'Simple|TracesResponse|Query.*Request|Request.*Query' | head -200
printf '%s\n' '--- generated traces resource entrypoint ---'
cat -n web/packages/agenta-api-client/src/generated/api/resources/traces/client/index.ts
cat -n web/packages/agenta-api-client/src/generated/api/resources/traces/index.tsRepository: Agenta-AI/agenta
Length of output: 35534
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- method around /simple/traces/query ---'
awk 'NR>=1365 && NR<=1422 {printf "%6d\t%s\n", NR, $0}' \
web/packages/agenta-api-client/src/generated/api/resources/traces/client/Client.ts
printf '%s\n' '--- generated declarations containing simple query types ---'
rg -n -C 6 'simple/traces/query|SimpleTrace.*Query|Query.*SimpleTrace|SimpleTraceResponse|TracesResponse' \
web/packages/agenta-api-client/src/generated --glob '*.ts'
printf '%s\n' '--- DTO types ---'
cat -n web/packages/agenta-entities/src/annotation/dto/types.tsRepository: Agenta-AI/agenta
Length of output: 49655
Use the existing Fern traces accessor and validate the response envelope.
Call getTracesClient().querySimpleTraces() with {queryParams: {project_id: projectId}}. Preserve raw trace items, but validate the {count, traces} envelope with safeParseWithLogging before mapping it. Do not add a new accessor; getTracesClient() already exists.
Source: Coding guidelines
| export interface AnnotationsResponseDto { | ||
| count: number | ||
| annotation: AnnotationDto | ||
| annotations: AnnotationDto[] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make AnnotationsResponseDto.annotation nullable.
Both response mappers return null when the API omits trace. The DTO declares annotation as AnnotationDto, and mapTraceResponseToAnnotation hides the mismatch with a cast. Callers can then dereference a missing annotation.
web/packages/agenta-entities/src/annotation/dto/types.ts#L96-L99: changeannotationtoAnnotationDto | null.web/packages/agenta-entities/src/annotation/dto/api.ts#L24-L32: remove the unsafeas AnnotationDtocast and preserve the nullable value.web/oss/src/services/annotations/api/index.ts#L28-L32: retain the nullable value under the corrected DTO contract and handle the union in consumers.
📍 Affects 3 files
web/packages/agenta-entities/src/annotation/dto/types.ts#L96-L99(this comment)web/packages/agenta-entities/src/annotation/dto/api.ts#L24-L32web/oss/src/services/annotations/api/index.ts#L28-L32
| if (nodes) { | ||
| for (const value of Object.values(nodes) as (T | T[])[]) { | ||
| if (Array.isArray(value)) { | ||
| for (const node of value) { | ||
| if (node.span_id === id) { | ||
| return node | ||
| } | ||
|
|
||
| if (node.children) { | ||
| const foundNode = getNodeById(node.children as T[], id) | ||
| if (foundNode) return foundNode | ||
| } | ||
| } | ||
| } else { | ||
| if (value?.span_id === id) { | ||
| return value | ||
| } | ||
|
|
||
| if (value?.children) { | ||
| const foundNode = getNodeById(value.children as T[], id) | ||
| if (foundNode) return foundNode | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Traverse only children nodes.
Object.values(nodes) scans all node properties. If metadata such as a span link contains the requested span_id, this function can return that metadata object instead of the matching tree node.
Normalize the root input to an array. Then recurse only through node.children. Add a regression test with a non-child nested object that has the same span_id as a real child.
Proposed fix
- if (nodes) {
- for (const value of Object.values(nodes) as (T | T[])[]) {
- if (Array.isArray(value)) {
- for (const node of value) {
- if (node.span_id === id) {
- return node
- }
-
- if (node.children) {
- const foundNode = getNodeById(node.children as T[], id)
- if (foundNode) return foundNode
- }
- }
- } else {
- if (value?.span_id === id) {
- return value
- }
-
- if (value?.children) {
- const foundNode = getNodeById(value.children as T[], id)
- if (foundNode) return foundNode
- }
- }
- }
+ const roots = !nodes ? [] : Array.isArray(nodes) ? nodes : [nodes]
+ for (const node of roots) {
+ if (node.span_id === id) return node
+
+ if (Array.isArray(node.children)) {
+ const foundNode = getNodeById(node.children as T[], id)
+ if (foundNode) return foundNode
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (nodes) { | |
| for (const value of Object.values(nodes) as (T | T[])[]) { | |
| if (Array.isArray(value)) { | |
| for (const node of value) { | |
| if (node.span_id === id) { | |
| return node | |
| } | |
| if (node.children) { | |
| const foundNode = getNodeById(node.children as T[], id) | |
| if (foundNode) return foundNode | |
| } | |
| } | |
| } else { | |
| if (value?.span_id === id) { | |
| return value | |
| } | |
| if (value?.children) { | |
| const foundNode = getNodeById(value.children as T[], id) | |
| if (foundNode) return foundNode | |
| } | |
| } | |
| const roots = !nodes ? [] : Array.isArray(nodes) ? nodes : [nodes] | |
| for (const node of roots) { | |
| if (node.span_id === id) return node | |
| if (Array.isArray(node.children)) { | |
| const foundNode = getNodeById(node.children as T[], id) | |
| if (foundNode) return foundNode | |
| } | |
| } |
| export function getStringOrJson(value: unknown): string { | ||
| return typeof value === "string" ? value : JSON.stringify(value, null, 2) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -HI -t f 'tsconfig*.json' . -x sh -c '
echo "=== $1 ==="
rg -n "\"strict\"|\"strictNullChecks\"" "$1" || true
' sh {}Repository: Agenta-AI/agenta
Length of output: 2108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="web/packages/agenta-shared/src/utils/typeNarrowing.ts"
echo "=== file ==="
cat -n "$file"
echo "=== package and workspace TypeScript configuration ==="
for f in web/tsconfig.json web/packages/agenta-shared/tsconfig.json; do
if [ -f "$f" ]; then
echo "--- $f"
cat -n "$f"
fi
done
echo "=== declarations and call sites ==="
rg -n -C 3 "getStringOrJson" web --glob '*.{ts,tsx}' || true
echo "=== related formatter usage ==="
rg -n -C 3 "trace text|traceText|formatter|format.*trace|StringOrJson" web/packages web/oss web/ee --glob '*.{ts,tsx}' || true
echo "=== JSON.stringify compiler declarations ==="
rg -n "JSON\.stringify|declare.*stringify" web/packages/agenta-shared web --glob '*.{ts,tsx,d.ts}' | head -80 || trueRepository: Agenta-AI/agenta
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== agenta-shared compiler configuration ==="
cat -n web/packages/tsconfig.base.json 2>/dev/null || true
cat -n web/packages/agenta-shared/package.json 2>/dev/null || true
echo "=== exact references to the shared helper ==="
rg -n --glob '*.{ts,tsx}' '\bgetStringOrJson\b' web/packages/agenta-shared web/oss web/ee \
| head -100 || true
echo "=== JSON.stringify behavior for relevant inputs ==="
node - <<'JS'
const circular = {}
circular.self = circular
for (const [name, value] of [
["undefined", undefined],
["function", function f() {}],
["symbol", Symbol("x")],
["bigint", 1n],
["circular", circular],
]) {
try {
const result = JSON.stringify(value, null, 2)
console.log(`${name}: ${result === undefined ? "undefined" : JSON.stringify(result)}`)
} catch (error) {
console.log(`${name}: throws ${error.constructor.name}`)
}
}
JS
echo "=== TypeScript compiler availability ==="
if command -v tsc >/dev/null 2>&1; then
tsc --version
else
echo "tsc unavailable"
fiRepository: Agenta-AI/agenta
Length of output: 7934
Return a string on every path.
With strictNullChecks, JSON.stringify is string | undefined. It returns undefined for functions, symbols, and undefined, and throws for BigInt and circular values. Handle both cases before returning. safeStringify has the same defect.
Six issues raised on #5954-#5958, each verified against the code first. `getNodeById` walked `Object.values(node)`, which visits every property rather than just `children`. Span metadata carries `span_id` too — an annotation span's `invocationIds` points at a different span — so a single-node lookup could return that bag instead of the span. The regression test covers exactly that shape; it passes on an array input either way, which is why the first version of it caught nothing. Both annotation queries scoped their request by `projectId` but not their cache key, so a project switch with the same links reused the previous project's annotations. The drawer store had inherited the same mistake when it stopped calling the oss wrapper that resolved the project internally. Session token and cost totals used `||` down their fallback chains, so a real incremental total of 0 fell through to the cumulative one and double-counted a span that reported no new tokens. `getOperator` can return undefined for an operator in the union but missing from OPERATORS; dereferencing `hidesValue` threw during validation and took the dialog's render with it. Plus two small ones: the docs link opened without `noopener`, and a secondary-only empty state rendered an orphaned "Or" separator.
1 participant
Context
Observability is being extracted from
web/ossinto packages so thatweb/mobilecan render the same data without a second implementation. Before any of that can move, the annotation and trace helpers it depends on have to stop living inoss/src/lib/hooks.This is the first of five stacked PRs. It moves only the shared leaf helpers, so nothing above it has to reach back into the app layer.
Changes
useAnnotations' transformer, types and helpers move to@agenta/entities/annotation/dto. The hook keeps its place in the app and now imports the moved pieces.observability_helpers.tsand the annotations API follow their imports.The design that governs the whole stack lands here too, in
docs/design/observability-packages/: the plan, and a kickoff describing how the lanes were split.Tests / notes
@agenta/entitiesbuilds, lints and passes its unit tests.obs/wp1orobs/wp2in isolation is expected, not a regression.