[DBA-94] Fix plot propagation to previous messages#68
[DBA-94] Fix plot propagation to previous messages#68SimonKaran13 wants to merge 4 commits intomainfrom
Conversation
Restrict the shared thread._visualization_result fallback to the latest message only, preventing older messages from rendering a newer query's plot. Add allow_thread_fallback guard to render_visualization_section as defense-in-depth, fix persistence round-trip when _has_spec_df marker is present but parquet file is missing, and add diagnostic logging. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
eee1c94 to
429d320
Compare
429d320 to
bb68723
Compare
There was a problem hiding this comment.
Pull request overview
Fixes an issue where visualization results from newer plots could incorrectly render in older messages by preventing non-latest messages from falling back to the shared thread._visualization_result, and hardening visualization persistence loading.
Changes:
- Restricts thread-level visualization fallback rendering to the latest message via an explicit
allow_thread_fallbackguard. - Improves
load_chatvisualization persistence round-trip when_has_spec_dfis set but the parquet file is missing (ensuresspec_dfis present andNone). - Adds regression tests covering fallback behavior and persistence edge cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/databao_cli/ui/components/results.py |
Adds guarded thread fallback and adjusts rendering logic to avoid plot “propagation” to older messages. |
src/databao_cli/ui/services/chat_persistence.py |
Ensures spec_df is explicitly set to None (and logs) when _has_spec_df expects parquet but it’s missing or unreadable. |
tests/test_plot_propagation.py |
Adds regression tests for fallback gating and persistence loading behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Only mark has_visualization=True when _extract_visualization_data actually returns data, preventing collapsed sections with no content. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| elif visualization_data is not None: | ||
| render_visualization_section(thread, visualization_data) | ||
| elif is_latest and (has_visualization or thread._visualization_result is not None): | ||
| render_visualization_section(thread, None, allow_thread_fallback=True) |
There was a problem hiding this comment.
In render_visualization_and_actions, the elif visualization_data is not None: branch will run even when the dict can’t actually be rendered (e.g., missing spec/spec_df), which prevents the latest-message thread fallback branch from ever running. This can lead to the latest message showing no visualization despite thread._visualization_result being available. Consider gating this branch on persisted data being renderable (e.g., both spec and spec_df present), or allowing render_visualization_section to fall back to the thread result when allow_thread_fallback=True and persisted data is incomplete.
There was a problem hiding this comment.
_extract_visualization_data is all-or-nothing: it either returns a complete dict with both spec and spec_df, or None. There is no code path that produces an incomplete dict, so in practice this branch cannot run with unrenderable data. The early return at L231 in render_visualization_section is already a safety net for that theoretical case. Adding an extra renderability gate here would be defensive code for a scenario that cannot currently occur.
Problem
When a user sends multiple queries that generate plots, new plots overwrite previously rendered messages. The root cause is that
thread._visualization_resultis a shared mutable singleton on the Thread object — everythread.plot()call overwrites it, so non-latest messages withhas_visualization=Truebut no per-messagevisualization_datafall back to this shared state and render the wrong plot.Solution
Restrict the shared thread fallback to the latest message only. Non-latest messages now render exclusively from their own persisted
visualization_data. Anallow_thread_fallbackguard onrender_visualization_sectionprovides defense-in-depth, and a persistence fix ensures missing parquet files don't leavespec_dfabsent from the dict (which caused silent render failures).Summary
thread._visualization_resultfallback to the latest message onlyallow_thread_fallbackkeyword-only parameter torender_visualization_sectionas defense-in-depth guardload_chatwhen_has_spec_dfis True but parquet file is missingvisualization_datais None after a successfulthread.plot()callTest plan
make checkpasses (ruff, mypy, agent guidance)make testpasses (pre-existingtest_indexfailure only)tests/test_plot_propagation.py🤖 Generated with Claude Code