You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(frontend): follow-up fixes for /m view review comments - #8130
Follow-up to #8125 (already in release/v0.128.8). CodeRabbit opened seven threads on #8125 after it merged. Six were real problems and are fixed here, one commit each, each with a test. The seventh is answered on its thread.
Context
Annotations. When no read showed a save yet, closing the annotate sheet dropped what it had saved. Opening it again could create the same annotation a second time.
Test set rows. A new row under a key with a dot, such as {"meta.lang": {source: "x"}}, got {meta: {lang: {source: ""}}}. A field named __proto__ typed in the drawer's JSON view was silently dropped.
Trace drawer address. If the drawer moved from trace t1 to t2 while t2 was loading, the address kept t1's span next to t2. When the drawer wrote the span that Back had just restored, it pushed a new history entry, so Forward no longer worked. When Back went to an address with no span, the old span stayed selected.
Changes
Unconfirmed annotation writes. Writes that no read confirmed are kept per span, outside the sheet. A sheet that opens again starts from them, so it updates them. The app keeps reading after the sheet closes, refreshes the drawer once the writes show, and then forgets them.
__proto__ in test set data. The drawer's split of the row data builds its records with Object.fromEntries. A __proto__ field stays a field, and Object.prototype is not touched.
Dotted keys in nested columns. Column extraction in @agenta/entities now gives each column its path segments (a new optional Column.path). A new row follows that path, so ["meta.lang", "source"] gives {"meta.lang": {source: ""}}. Columns with no path (added in the grid) behave as before. Unsafe path parts are still left out.
Trace drawer address. The address keeps an asked span only when it belongs to the open trace. A write that changes nothing does not push. Back to an address with no span clears the selected span.
Review of this PR. Annotation writes are kept from the moment they land, and an older read cannot drop a newer save. After Back clears the span, the drawer's fallback span write replaces the history entry instead of pushing one. collectColumnPaths keeps the ancestors when it starts below the row.
Not changed: "carry cumulative error totals forward across sparse timestamps". The timed metric rows are per-minute buckets, not running totals (interval is DEFAULT_REFRESH_INTERVAL, one minute, and each scenario is bucketed by its minute). An evaluator with no row at a timestamp had no errors in that minute, so carrying its earlier value forward would count it twice.
/w
Column.path is a new optional field. /w reads columns but does not use it. The /w type check passes.
Tests
Every fix has a new test that fails on the code before it.
pnpm lint-fix in web/ gives 0 errors. Type checks pass for /m, /w (@agenta/oss) and @agenta/entities.
@agenta/entities unit tests: 2978 pass. The touched /m tests pass. Local gitleaks: no leaks.
What to QA
Open a trace, pick a span, pick another span, press Back. The first span shows, and Forward still goes to the second.
In a test set whose rows hold {"meta.lang": {"source": "x"}}, press Add row. The new row has the meta.lang group with an empty source.
Regression: Add row on an ordinary test set still gives one empty field per column.
When no read showed a save yet, closing the sheet dropped what it saved, and
opening it again could create the same annotation a second time. The writes
now stay known for that span until a read shows them, and the drawer
refreshes then.
The drawer split the row data with plain property writes, so a field named
__proto__ set the object's prototype and the update dropped it. The split now
builds its records with Object.fromEntries: the field stays row data and
Object.prototype is not touched.
Column extraction now carries each column's path segments. A new row under a
key with a dot, such as {"meta.lang": {source: "x"}}, gets
{"meta.lang": {source: ""}} instead of {meta: {lang: {source: ""}}}.
Unsafe path parts stay left out.
…Back
The address keeps an asked span only for the trace it belongs to. A span write
that changes nothing (the drawer showing a span Back restored) no longer pushes
a history entry, so Forward still works. Back to an address with no span
clears the selected span.
Annotation saves that take time to appear remain available when the form is reopened, reducing duplicate annotations.
Trace drawers clear stale span selections when the URL changes and avoid unnecessary address updates.
Test set editors preserve dotted keys and nested paths when adding rows and handling data.
Fields named __proto__ remain data and do not affect object behavior.
Column paths retain key segments, including dotted keys, when extracting nested test set columns.
The changes update trace annotation persistence and trace drawer URL synchronization. They also add explicit column paths for testcase rows and change field splitting to preserve special-named fields as data.
Changes: Trace annotation persistence
Layer / File(s)
Summary
Retain and confirm pending annotations web/mobile/src/features/observability/annotate/useTraceAnnotationForm.ts, web/mobile/tests/unit/useTraceAnnotationForm.test.tsx
The form restores unconfirmed writes for a trace and span, then checks stored reads and clears retained entries after confirmation. Tests cover reopening the form and updating an existing annotation instead of creating another.
Trace drawer URL synchronization
Layer / File(s)
Summary
Synchronize trace and span URL state web/mobile/src/features/observability/bindTraceDrawerSeams.ts, web/mobile/src/features/observability/useTraceDrawerFromUrl.ts, web/mobile/tests/unit/traceDrawerFromUrl.render.test.tsx, web/mobile/tests/unit/traceDrawerFromUrlHistory.render.test.tsx, web/mobile/tests/unit/traceDrawerQuery.test.ts
The URL hook clears the active span when the same trace remains open without a span parameter. The query binder retains a span only for the matching trace, skips equivalent query updates, and uses replacement history for the next marked span write. Tests cover clearing, trace changes, fallback, and unchanged addresses.
Testcase row data
Layer / File(s)
Summary
Carry column paths into new rows web/packages/agenta-entities/src/testcase/core/types.ts, web/packages/agenta-entities/src/testcase/core/columnExtraction.ts, web/mobile/src/features/testsets/detail/useTestsetEditor.ts, web/packages/agenta-entities/tests/unit/testcase-column-paths.test.ts, web/mobile/tests/unit/testsetEditorNewRow.test.ts
Extracted columns now include key segments, so dotted keys remain single path segments. New-row creation uses an explicit column path when available.
Field splitting uses Object.fromEntries to preserve __proto__ as a row field. A test checks that editing such a field does not change ordinary object prototypes.
sequenceDiagram
participant useTraceDrawerFromUrl
participant TraceDrawer
participant bindTraceDrawerSeams
participant Router
useTraceDrawerFromUrl->>TraceDrawer: Set span from URL, including null
TraceDrawer->>bindTraceDrawerSeams: Write span query
bindTraceDrawerSeams->>Router: Replace marked span update or push changed query
Loading
Possibly related issues
Agenta-AI/agenta#8093: The issue concerns opening the trace drawer from URL parameters. This change updates URL-to-drawer span handling, but the supplied summaries do not confirm that it fixes the reported page-load behavior.
Possibly related PRs
Agenta-AI/agenta#8125: This PR changes annotation polling, trace drawer URL synchronization, and row creation behavior introduced by that PR.
Agenta-AI/agenta#8123: This PR continues annotation polling and extends the row creation path used by addRow.
Agenta-AI/agenta#8075: This PR changes the trace drawer URL update flow and its handling of cleared trace parameters.
Merge Risk: Moderate · up to 05072
Deleting a special-named field may not persist, and Back navigation can lose Forward history. Resolve these behaviors before merging.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title clearly identifies this as a frontend follow-up that fixes review comments for the /m view. It is concise and related to the main changes.
Description check
The description directly explains the annotation, test-set data, column-path, and trace-drawer fixes, along with tests and validation results.
Docstring Coverage
Docstring coverage is 100.00% which is sufficient. The required threshold is 60.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 60.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 14 files.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
⚠️ Outside diff range comments (1)
Minor · Remove __proto__ when it is deleted from the row. · testcaseDataUpdate.ts:13
web/mobile/src/features/testsets/detail/testcaseDataUpdate.ts:13 Minor · Data Integrity & Integration · Quick win
Remove __proto__ when it is deleted from the row.
When a user removes an own __proto__ field, key in next still returns true because the property is inherited. The update omits the deletion marker, so the merged row keeps the field. Use Object.hasOwn(next, key) and add a removal case to the test. (raw.githubusercontent.com)
37-41: Trivial · Maintainability & Code Quality · Low value
Shorten the new prose comments.
Both comments span multiple lines. Reduce each to one short line. As per coding guidelines, “At most ONE short line per comment. No multi-line blocks narrating why in prose.”
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 2 remain after this review.
Writes are now kept for the span as soon as the API takes them, not after the
first read gives up, so a sheet closed during that wait or after a partly
failed save still knows them. A read that shows a write drops it only when it
shows that same value, so an older read cannot drop a newer save.
With no span the drawer falls back to its first span and writes it to the
address. After Back to an address with no span, that write now replaces the
current entry instead of pushing one, so the Forward entry stays.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 1 remain after this review.
…tring
collectColumnPaths now takes the keys above the object it walks instead of a
dotted parent path, so a caller that starts below the row cannot split a key
like meta.lang into two.
The replace for the drawer's fallback span write is now armed only when
clearing the span changes the span the drawer shows. When the drawer already
showed its first span, nothing is written, and the next pick pushes as usual.
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 0 remain after this review.
Provisioning failed; no manual review time granted. Environment absence verified; retained storage requires separate provider accounting.
Post /preview as a new comment to request a one-hour preview. Repeating it while active does not extend the hour.
Preview data is disposable. Expiry is enforced by the next successful cleanup sweep, not an exact timer.
Environment deletion stops its compute. Provider-retained storage and permanent services may still be billed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Follow-up to #8125 (already in
release/v0.128.8). CodeRabbit opened seven threads on #8125 after it merged. Six were real problems and are fixed here, one commit each, each with a test. The seventh is answered on its thread.Context
{"meta.lang": {source: "x"}}, got{meta: {lang: {source: ""}}}. A field named__proto__typed in the drawer's JSON view was silently dropped.t1tot2whilet2was loading, the address keptt1's span next tot2. When the drawer wrote the span that Back had just restored, it pushed a new history entry, so Forward no longer worked. When Back went to an address with no span, the old span stayed selected.Changes
Unconfirmed annotation writes. Writes that no read confirmed are kept per span, outside the sheet. A sheet that opens again starts from them, so it updates them. The app keeps reading after the sheet closes, refreshes the drawer once the writes show, and then forgets them.
__proto__in test set data. The drawer's split of the row data builds its records withObject.fromEntries. A__proto__field stays a field, andObject.prototypeis not touched.Dotted keys in nested columns. Column extraction in
@agenta/entitiesnow gives each column itspathsegments (a new optionalColumn.path). A new row follows that path, so["meta.lang", "source"]gives{"meta.lang": {source: ""}}. Columns with no path (added in the grid) behave as before. Unsafe path parts are still left out.Trace drawer address. The address keeps an asked span only when it belongs to the open trace. A write that changes nothing does not push. Back to an address with no span clears the selected span.
Review of this PR. Annotation writes are kept from the moment they land, and an older read cannot drop a newer save. After Back clears the span, the drawer's fallback span write replaces the history entry instead of pushing one.
collectColumnPathskeeps the ancestors when it starts below the row.Not changed: "carry cumulative error totals forward across sparse timestamps". The timed metric rows are per-minute buckets, not running totals (
intervalisDEFAULT_REFRESH_INTERVAL, one minute, and each scenario is bucketed by its minute). An evaluator with no row at a timestamp had no errors in that minute, so carrying its earlier value forward would count it twice./w
Column.pathis a new optional field. /w reads columns but does not use it. The /w type check passes.Tests
pnpm lint-fixinweb/gives 0 errors. Type checks pass for /m, /w (@agenta/oss) and@agenta/entities.@agenta/entitiesunit tests: 2978 pass. The touched /m tests pass. Local gitleaks: no leaks.What to QA
{"meta.lang": {"source": "x"}}, press Add row. The new row has themeta.langgroup with an emptysource.