Skip to content

Scott/follow up fixes#386

Merged
sagrimson merged 20 commits into
datacommonsorg:mainfrom
madebypxlp:scott/follow-up-fixes
Jul 7, 2026
Merged

Scott/follow up fixes#386
sagrimson merged 20 commits into
datacommonsorg:mainfrom
madebypxlp:scott/follow-up-fixes

Conversation

@sagrimson

@sagrimson sagrimson commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Overview

Improves the follow-up question flow and related queries experience in DataWeaver. Previously, broad/exploratory queries triggered a follow-up disambiguation step instead of returning data directly, follow-up context (the user's original question and selected option) was lost between turns, and multi-place queries could emit multiple conflicting follow-ups. This PR restructures how follow-up context is passed through the pipeline, tightens the data discovery skill to prefer returning data over asking clarifying questions, consolidates per-place follow-ups into a single synthesized message, adds related query suggestions to card footers, strips dead hyperlinks for variables with no data, and introduces a shuffled pool of example prompts.

Changes Made

  • Structured follow-up context: Introduced a FollowUpContext type that chains the original query and each follow-up Q&A pair, passing it from the client store through the API route into both parseQuery and runToolLoop so Gemini has full conversation history when resolving follow-ups.
  • Follow-up promoted to top-level stream event: Added a dedicated follow_up stream event type. Follow-ups are no longer embedded inside per-place QueryResult objects — they're collected across all places in the loop, merged into a single disambiguation message, and emitted once at the end of the stream. This fixes the issue of multiple follow-ups appearing for multi-place queries.
  • Post-loop follow-up synthesis: The API route now accumulates disambiguation signals from each place iteration and synthesizes at most one merged follow-up (combining summaries and deduplicating options). A generic fallback follow-up is emitted if no results and no disambiguation signals were produced.
  • nodeSetFollowUp store action: Added a new store action to write the synthesized follow-up directly onto the HistoryNode, decoupling it from query results. PageHome now reads latestNode.followUp directly instead of scanning through results.
  • Data discovery skill rewrite: Rewrote data_discovery.md to only use followUp as a last-resort fallback (no data found / unintelligible / adversarial query) rather than a disambiguation step for broad queries. Clarified that followUp is an orchestrator signal, not user-facing output. Added rules for data verification, DCID accuracy, place passing, and the new relatedQueries output field.
  • Related queries on cards: Changed relatedQuery (single string) → relatedQueries (string array) across CardChart, CardText, shape props, helpers, and sync_store. Card footers now render multiple related-query buttons in a vertical stack.
  • Card footer layout: Updated footer.module.scss to use column flexbox with gap for stacked related-query buttons.
  • Strip no-data hyperlinks: render_result_html.ts now identifies which variables actually have time-series data and converts #fetch= markdown links for empty variables into plain text — both in the table rows and in the introduction/coverage/insights prose.
  • Safety skill relaxed: Modified the safety classifier to only block genuinely malicious/jailbreak queries; off-topic or nonsensical queries are now allowed through and handled downstream by data discovery.
  • Follow-up dismissal persisted to store: Replaced local dismissedNodeId state in PageHome with a nodeDismissFollowUp store action that removes the follow-up from the node, preventing reappearance on re-renders.
  • Node removal on follow-up dismissal: When a follow-up is dismissed, the associated history node is cleaned up.
  • Example prompts extracted & shuffled: Moved hardcoded example prompts from Intro into a shared configs/example_prompts.ts pool of 20 prompts. The page server component shuffles and slices 4 at render time. The follow-up component also pulls from this pool as a fallback when the API returns no options.
  • Place resolution defaults to Earth: When no explicit place is found, the route now defaults to "Earth" instead of using the raw query string as a place, and strips follow-up options so the model asks the user to specify a location.
  • Parse query improvements: Updated parse_query.md and parse_query.ts to extract places from the full follow-up conversation context and to be more conservative about marking queries as follow-ups when they contain explicit places/topics.
  • Simplified per-place result flow: Removed follow-up handling from the per-place card registration path in query_provider — results now always register cards, and follow-ups arrive separately via the new stream event.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a structured followUpContext to track conversation history (original query and Q&A chain) for follow-up queries, replacing simple query string concatenation. It also defaults the target place to "Earth" when no explicit place is provided, stripping follow-up options in this scenario to prompt the user for a location. A critical issue was identified in route.ts where the resolved places array is not assigned back to parsed.places after fallback logic, which would prevent the default place from propagating to the client and downstream history nodes.

Comment thread dataweaver/apps/web/src/app/api/query/route.ts
@sagrimson sagrimson force-pushed the scott/follow-up-fixes branch from 6397485 to 70d2a1a Compare July 2, 2026 21:53
@sagrimson sagrimson marked this pull request as ready for review July 3, 2026 00:42

@nick-nlb nick-nlb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Scott! Some comments, but once the actionable ones have been addressed we could merge this in. Some of them will require experimentation and discussion (such as how the model now responds to gibberish query).

We should also make sure we keep onto the radar (te tracker) the movement of "feedback" toasts, such as for adversarial queries)into the followUp loop. It doesn't have to give options, but should give text, and let the user type another query. It should do this for adversarial, nonsense or non-data related queries.

*/
const stripNoDataLinks = (md: string, withData: Set<string>): string => {
return md.replace(
/\[([^\]]+)\]\(#fetch=([^&]+)&.*?placeName=[^)]*\)/g,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is highly specific (and mirrors the generated links) but also couples this string detection to that generation. If we ever change the order of the parameters, or add other parameters, this becomes fragile.

Since the only thing we need here is the dcid, we could simplify this and make it less reliant on the strict parameter order.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah true, simplified this one!

- **`options`**: Exactly 3 to 5 concise labels. Each should represent a distinct analytical direction relevant to the place/topic. Phrase them from the user's perspective (e.g. "Economic impact", "Health outcomes for its people", not "GDP variables").
5. **Follow-up (disambiguation signal)**: The `followUp` field is a signal consumed by the orchestrator — it will NOT appear directly in the final per-place result sent to the user. Instead, the orchestrator collects follow-up signals from all places and synthesizes a single follow-up message for the entire conversation round. Include the `followUp` field ONLY as a fallback when you cannot return any meaningful data — for example, the query is gibberish, adversarial, too broad to map to any specific variables, not data-related, or `search_indicators` returned no usable results for the place. In all other cases (including broad exploratory queries like "tell me about Seychelles"), find and return variables directly — do NOT use `followUp` as a disambiguation step.
- **When to include**: The query is unintelligible, nonsensical, or adversarial; the query has no data-related interpretation; no variables were found after searching; the place name is ambiguous and cannot be resolved.
- **When to omit**: Any time you can find and return at least one relevant variable with data. Even for broad queries, select the most representative variables across domains and present them directly.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic pushes the model very hard to avoid asking a "follow-up" - or even in interpreting a nonsense question.

Even pure gibberish such as "asdfasdf" causes it to think it needs to pull something. I had to try very hard to have it ever give any sort of follow-up. This gives the feeling that the agent is disconnected from the questions being asked.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep totally went too far in the 'always provide data' direction :D
I updated this so that:
– if the query fails the safety check, you get a generic follow-up with suggested questions
– if the query fails the parse query step (is gibberish, non-data related, or can't find any places or variables) you get a follow-up that is provided by the llm and is more tailored to the actual question you asked, with suggested questions
– if the data_discovery step doesn't get any good data you get the follow-up plus custom follow-up options that came from the mcp

};

// If multiple places triggered disambiguation, merge summaries.
if (disambiguationEntries.length > 1) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry that this will result in a long list of potentially repetitive or disjointed text, as well as duplicated options. Perhaps this should be run through a follow-up synthesis pass through the LLM to make it a single, coherent summary. However, I would put that aside for now until we have a chance to experiement.

Separately (and more actionable right away), right now we are not dedupping, which would help (but not totally remove the issue, because the two summaries might have similar, but not identical follow-up options).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added deduping, but I think you're right this could warrant another llm pass

@sagrimson sagrimson merged commit 44d93ec into datacommonsorg:main Jul 7, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants