fix: persist ask_user_question, SQL results, and timing across conversation switches

[cyyeh]

Summary

• Persist ask_user_question tool call/result blocks in thinking section so they survive conversation switches
• Persist SQL query result data (columns/rows) in backend segments so tables render on reload
• Track and persist answer timing (answerDurationMs) on user question blocks
• Track and persist overall response duration (durationMs) on assistant messages
• Reconstruct all persisted data in buildChatMessages on frontend reload

Test plan

• Send a message that triggers ask_user_question, answer it, verify the tool block and question UI render
• Switch to another conversation and switch back — verify the ask_user_question tool block still appears in the thinking section
• Verify SQL query result tables show data after switching conversations
• Verify answer timing shows on the question block after reload
• Verify overall response duration shows on the answer block after reload

🤖 Generated with Claude Code

[claude]

Claude finished @cyyeh's task in 2m 42s —— View job

---

Code Review

• Gather context and understand the PR
• Review backend changes (agent.py)
• Review frontend type definitions (types.ts)
• Review frontend utility (buildChatMessages.ts)
• Review context changes (AgentContext.tsx)
• Review UI component changes
• Compile and post final review

---

Summary

This PR adds conversation state persistence so that ask_user_question interactions, SQL result tables, and timing metadata survive conversation switches and page reloads. The overall approach is sound: persist structured segments into the assistant message's metadata JSON on the backend, then reconstruct them in buildChatMessages on the frontend. Good feature overall with clear, targeted changes.

---

Issues Found

🐛 Bug: answerDurationMs attached to wrong question when multiple questions exist

frontend/src/contexts/AgentContext.tsx lines 257–263 and 581–587

When an ask_user_question tool result arrives with answerDurationMs, the code finds the first user_question segment rather than the one matching the toolCallId:

// In both sendMessage and editMessage:
const qIdx = state.segments.findIndex(
  s => s.type === 'user_question' && s.questionData
);

If an agent asks multiple clarifying questions sequentially in a single response (e.g. first "Which chart?", then "Which columns?"), the timing for every question would be pinned to the first question segment instead of the one that was actually answered. It should match by toolCallId since result.toolCallId is available:

const qIdx = state.segments.findIndex(
  s => s.type === 'user_question' && s.questionData &&
       s.questionData.questionId === result.toolCallId
);

Or more precisely, match via the toolCallId stored on the segment when it was appended:

// The user_question segment stores toolCallId at the top level (set in AgentContext)
// but it's inside questionData.questionId on the frontend side

Note this appears in both sendMessage (line ~257) and editMessage (line ~581) — both need the same fix. Fix this →

---

⚠️ Minor Bug: Falsy check silently drops answerDurationMs = 0

frontend/src/utils/buildChatMessages.ts line 61

if (seg.answerDurationMs) s.answerDurationMs = seg.answerDurationMs as number;

A duration of 0ms is practically impossible but a value of 1ms is plausible. More importantly, using a truthy check here is inconsistent with how durationMs is checked on the outer message (line 26: if (typeof meta?.durationMs === 'number')). Prefer:

if (seg.answerDurationMs != null) s.answerDurationMs = seg.answerDurationMs as number;

---

⚠️ Null pollution in persisted tool segments

backend/app/agent.py lines 714–716

seg["rowCount"] = result_data.get("rowCount")   # may be None
seg["error"] = result_data.get("error")         # may be None

When rowCount or error are absent from result_data, None is explicitly written into the segment dict and serialised to JSON as null. This is harmless now (the frontend coerces null to 0 / undefined), but it inflates the stored JSON unnecessarily. Prefer conditional assignment:

if "rowCount" in result_data:
    seg["rowCount"] = result_data["rowCount"]
if "error" in result_data:
    seg["error"] = result_data["error"]

---

ℹ️ durationMs always persisted, even without segments

backend/app/agent.py lines 779–785

meta = assistant_metadata.copy() if (...) else {}
if persisted_segments:
    meta["segments"] = persisted_segments
meta["durationMs"] = int((time.time() - stream_start_time) * 1000)  # always set
memory_store.add_message(..., metadata=meta if meta else None)

Because durationMs is set unconditionally, meta is always truthy, so metadata=meta if meta else None effectively always passes meta. This means even plain text responses with no tools/segments will store a {"durationMs": N} metadata object. This is arguably desirable (the UI shows "Answered in Xs" even for simple responses), but it's worth being deliberate about: if the intent is to only store metadata when there's meaningful segment data, durationMs should be inside the if persisted_segments: block.

---

Code Quality Observations

Significant duplication: sendMessage vs editMessage in AgentContext.tsx

Both callbacks share ~150 lines of near-identical streaming event handling logic (tool results, thinking, subagent, user questions, done, error). The PR adds the same answerDurationMs attachment code in both places. This is a pre-existing structural issue, but each PR that adds new streaming logic doubles the maintenance surface. A shared createStreamHandlers(state, convId, assistantId, ...) factory would make future changes much safer. Not a blocker, but worth tracking.

buildChatMessages.ts doesn't restore command field

The ToolCallResult type has an optional command field, but buildChatMessages doesn't restore it when reconstructing tool segments from persisted metadata. Since command isn't being persisted on the backend either (the persisted_segments dict stores toolName, sql, but not command), this is consistently missing on both sides. If command needs to survive reloads in the future, both backend and buildChatMessages would need updates.

---

What Works Well

• Approach is correct: Persisting structured segments into message metadata is the right pattern — it decouples rendering logic from raw text content.
• finally block persistence (agent.py) is a good safety net that catches stream data even when the client disconnects mid-response.
• Row truncation at 100 in the backend prevents metadata bloat for large query results.
• Graceful fallback in buildChatMessages (line 72–74): when metadata is missing or malformed, falls back to rendering the plain message content as an answer segment.
• i18n coverage: New strings (questionAnswered, answeredIn) are added to both en.json and zh-TW.json.

---

Testing Gap

No automated tests cover the new persistence/reconstruction path. The cases most worth testing:

• Backend: persisted_segments built correctly for ask_user_question tool calls (with userAnswer, userFreeText, answerDurationMs)
• Backend: persisted_segments for SQL results (columns/rows populated)
• Frontend: buildChatMessages correctly reconstructs user_question segments with timing and answers