feat: surface conversation tags in the conversation panel#1926
Conversation
Agent-server conversations carry server-side key-value tags (settable at creation and via PATCH), but the UI only ever consumed the reserved acpserver routing tag — tags stamped by automations or API clients for attribution (e.g. origin=slack, owner=alice) were invisible. - expose the normalized tags map on AppConversation (local adapter; null for Cloud conversations) - render non-reserved tags as sorted key: value chips in the conversation card footer, reusing the metadata indent - gate the chips behind a new "Tags" toggle in the panel's Metadata filter-menu section (persisted like the existing metadata toggles) - filter reserved keys through getDisplayConversationTags so acpserver never double-renders next to the agent chip Closes OpenHands#1925 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F5QQqqrqmVBXv3kdSFKpAy
|
Someone is attempting to deploy a commit to the openhands Team on Vercel. A member of the Team first needs to authorize it. |
|
Just a couple of questions: where are tags saved? Because i think the correct thing to do would be to save tags in the trajectories i.e. in the agent server. And then you visualize just that with the UI |
neubig
left a comment
There was a problem hiding this comment.
🟡 Acceptable — The data plumbing is straightforward, and keeping Tags as a visible option in the Metadata menu is the right product direction. The unbounded card rendering and incomplete #1925 scope need rework before merge.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This renders API-controlled metadata in every conversation card. The main risk is sidebar layout/performance degradation from unbounded input; there are no security-sensitive or dependency changes.
VERDICT:
❌ Needs rework: Bound chip rendering, preserve access to overflow tags, and either implement the issue's tag-filter acceptance criterion or stop closing #1925.
KEY INSIGHT:
Server-owned metadata needs an explicit UI display budget and an end-to-end assertion at the adapter boundary.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
|
I will rather open a Pr in agent-server to add tags and then this one will just display them |
…emantics Review follow-ups from OpenHands#1926: - cap the card's tag-chip row at MAX_VISIBLE_TAG_CHIPS (3) sorted keys and fold the remainder into a "+N" chip whose tooltip lists the hidden tags, so an API-controlled tag map can't grow a card unboundedly; the full map stays on AppConversation.tags for non-display consumers - extend the conversation-service tests to assert the wire → AppConversation.tags boundary end-to-end: well-formed payloads arrive intact, non-string values are dropped, an absent wire field surfaces as null (the malformed-tags test previously only asserted acp_server) - document that agent-server PATCH replaces the complete tags map rather than merging keys, so writers must read-merge-write Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012t7NV3kLhJv3BWCp8vimsm
|
@VascoSch92 Tags are already stored server-side — no agent-server change is needed. |
|
Ah ok perfect. |
✅ Mock-LLM E2E Tests60/60 passed Commit: Details
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
neubig
left a comment
There was a problem hiding this comment.
Re-reviewed the final head after the requested fixes and merge from main. All four review concerns are addressed, and the required Linux/Windows CI, full unit suite, Mock-LLM E2E, builds, and package verification pass.
HUMAN:
A small quality of life improvement which i thought might help community. But pelase feel free to reject the changes if you dont see fit
AGENT:
This PR was implemented by an AI agent (Claude Code) working with @harish-chandramowli; the HUMAN section above is left for him to fill in after testing on our self-hosted deployment.
Why
Agent-server conversations carry server-side key-value
tags(settable at creation and viaPATCH /api/conversations/{id}), and the frontend already fetches and normalizes them — but the UI only ever consumes the reservedacpserverrouting tag. Tags stamped by automations or API clients for attribution (e.g.origin=slack,owner=alice) are invisible, so in a panel with Slack-automation-born, API-born, and browser-born conversations there is no way to tell which is which.Discussed in #1925.
Summary
tagsmap onAppConversation(local adapter;nullfor Cloud conversations) and addgetDisplayConversationTags()which filters reserved keys (acpserver) and returns sorted entrieskey: valuechips in the conversation card footer with a fixed display budget: at most 3 chips (sorted by key) plus a+Noverflow chip whose tooltip lists the hidden tags — an API-controlled tag map can't grow a card unboundedly. The full map stays onAppConversation.tagsfor non-display consumersIssue Number
Part of #1925 — this PR delivers the chip-display half. The issue's second item ("match tags in the text filter") turned out to rest on a wrong premise of mine: there is no free-text conversation filter in the panel today (
Filter conversationsis only the aria-label of the filter-menu button), so tag filtering means building a text-filter feature first and is left as follow-up scope on the issue.How to Test
npm install && npm run devagainst a local agent-server backendcurl -X POST localhost:8000/api/conversations -H 'Content-Type: application/json' -d '{ ..., "tags": {"origin": "slack", "owner": "alice"} }'To tag an existing conversation, note that
PATCHreplaces the completetagsmap (it does not merge keys) — read the current map first, merge, and send the full result, e.g.GET /api/conversations/{id}→ merge →PATCHwith{"tags": {"acpserver": "claude-code", "origin": "slack", "owner": "alice"}}. Sending only the new key would silently drop every existing tag, including the reservedacpserverrouting tagorigin: slack/owner: alicechips; the reservedacpservertag never renders as a chip; with more than 3 non-reserved tags the card shows the first 3 (sorted by key) plus a+Nchip whose tooltip lists the restAutomated evidence from this branch (macOS, Node 22):
npm run lint— typecheck + eslint + prettier all passnpm test— 3941 passed / 12 failed; all 12 failures (recommended-automations.test.tsx,conversation-runtime-info.test.ts,use-websocket.test.ts) reproduce identically on a cleanmainworktree in the same environment, i.e. pre-existing/env-specific and untouched by this diff. New coverage: 6 tag-chip tests inconversation-card.test.tsx(including the display-budget cap and+Noverflow tooltip), 3 wire-boundary tests inagent-server-conversation-service.test.tsasserting a wire payload's valid/malformed/absenttagsreachAppConversation.tagsnormalized, plus store-default/persist and filter-menu prop updatesnpm run build— succeedscheck-translation-completeness) passed on commitLive end-to-end verification (after the initial push): ran
npm run dev:frontendon this branch against our real self-hosted agent-server (openhands-agent-server 1.36.1) over an SSH tunnel,PATCHed{"tags": {"origin": "review", "owner": "harish"}}onto an existing conversation, and drove the UI with Playwright: the card renders both chips, the reservedacpservertag never renders, and the filter-menu "Tags" toggle hides/re-shows the row (chip count 2 → 0 → 2). Screenshots/video from that session are attached below.Video/Screenshots
Agent-recorded Playwright walkthrough against our live self-hosted backend (agent-server 1.36.1, real conversations): video (mp4) (webm) — chips visible → Tags toggle off → back on.
tags-demo.mp4
Type
Notes
tagsisnullthere).ConversationInfo.tags: Record[str, str]and Canvas already fetched/normalized them — this PR only makes them visible.🤖 Generated with Claude Code
https://claude.ai/code/session_01F5QQqqrqmVBXv3kdSFKpAy