-
Notifications
You must be signed in to change notification settings - Fork 159
feat: dashboard chat follow ups #8445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Di7design
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check UXQA here.
Review: Architecture Feedback on Rich Text RenderingHi! While reviewing the changes, I noticed an architectural issue with how we're rendering user messages that we should discuss. The IssueWe're using TipTap (ProseMirror) for rich text input but then discarding its document structure for display, falling back to manual string parsing and Current flow:
Key problems:
Suggested SolutionSince we're already using TipTap for input, we should use it for display too: // Store the rich document structure
const messageContent = editor.getJSON(); // Instead of getText()
// Display with read-only TipTap
const displayEditor = new Editor({
element,
editable: false,
content: messageContent,
extensions: [...] // Same as input
});This would:
What do you think of this approach? I believe addressing this now while we're already working on the chat UI would prevent this technical debt from growing, but I'm interested in your perspective on whether there are constraints or considerations I might be missing. Developed in collaboration with Claude Code |
Observation: MeasureSelection Class ArchitectureI noticed the Current pattern: // In measure-selection.ts
class MeasureSelection {
public readonly start = writable<Date | null>(null); // Domain data
public readonly end = writable<Date | null>(null); // Domain data
public readonly x = writable<number | null>(null); // Screen position
public readonly y = writable<number | null>(null); // Screen position
calculatePoint(start, end, scaler, config) { /* updates x/y */ }
}Potential concerns:
A few alternative patterns to consider:
I'm curious about your thoughts on whether separating data from presentation might make this more maintainable, or if there are specific reasons for the current approach that I might be missing (like performance considerations or cross-component position sharing). Developed in collaboration with Claude Code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See two observations in the Claude Code comments above
|
@ericpgreen2 The explain button is rendered outside the |
6da2609 to
15fc656
Compare
Di7design
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve for the must fixed UXQA issues.
ericpgreen2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good! Two small things below.
* feat: dashboard chat follow ups * Fix anomaly detection edge cases * Add floating ui * UXQA pass 1 * fix clicks being ignored * fix context dropdown sizing * Use tiptap for readonly mode * Cleanups

A few dashboard chat follow ups,
More items: https://www.notion.so/rilldata/UXQA-Rill-Cloud-AI-Features-2bdba33c8f578079a3a6fb883d4c73dc
Checklist: