Skip to content

Add ability to copy table and copy code in messages#791

Open
eldong wants to merge 3 commits intoDevelopmentfrom
copy-table
Open

Add ability to copy table and copy code in messages#791
eldong wants to merge 3 commits intoDevelopmentfrom
copy-table

Conversation

@eldong
Copy link
Collaborator

@eldong eldong commented Mar 11, 2026

No description provided.

Copilot AI review requested due to automatic review settings March 11, 2026 15:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds richer rendering and copy affordances for LLM output in the chat UI, primarily by expanding the message preprocessing pipeline (tables + code) and introducing table copy toolbars.

Changes:

  • Introduces a unified preprocessing pipeline for chat markdown to normalize multiple table formats and wrap unfenced code blocks.
  • Uses the preprocessing pipeline during streaming renders to avoid “raw pipe table” flashes.
  • Adds “Copy table” toolbars (TSV copy) and updates chat CSS to support copy-button layout; bumps app version and adds release notes.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
docs/explanation/release_notes.md Adds v0.239.011 release notes for table rendering + table copy buttons.
application/single_app/static/js/chat/chat-streaming.js Applies preprocessing before marked/DOMPurify rendering during streaming.
application/single_app/static/js/chat/chat-messages.js Adds preprocessing helpers, exports preprocessTableContent, adds table copy buttons, updates render pipeline.
application/single_app/static/css/chats.css Adds table copy toolbar styling and adds top padding to code blocks.
application/single_app/config.py Bumps VERSION to 0.239.011.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +954 to +959
const hasTablePipes = /\|.+\|/.test(withTableSeparators);
const hasHtmlTable = sanitizedHtml.includes('<table');
if (hasTablePipes) {
console.log(`📊 Table diagnostic (msg ${messageId}): pipes=${hasTablePipes}, htmlTable=${hasHtmlTable}`);
if (!hasHtmlTable) {
console.warn('⚠️ Content has pipe chars but no <table> was generated. Content preview:', withTableSeparators.substring(0, 300));
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

The new table diagnostics can log potentially sensitive message content (first 300 chars) into the browser console and will be noisy for any message containing pipe characters. Please gate this behind an explicit debug flag (e.g., app setting / query param) or remove the content preview logging for production.

Suggested change
const hasTablePipes = /\|.+\|/.test(withTableSeparators);
const hasHtmlTable = sanitizedHtml.includes('<table');
if (hasTablePipes) {
console.log(`📊 Table diagnostic (msg ${messageId}): pipes=${hasTablePipes}, htmlTable=${hasHtmlTable}`);
if (!hasHtmlTable) {
console.warn('⚠️ Content has pipe chars but no <table> was generated. Content preview:', withTableSeparators.substring(0, 300));
const enableTableDiagnostics =
typeof window !== "undefined" &&
window.appSettings &&
window.appSettings.enable_table_diagnostics === true;
const hasTablePipes = /\|.+\|/.test(withTableSeparators);
const hasHtmlTable = sanitizedHtml.includes("<table");
if (enableTableDiagnostics && hasTablePipes) {
console.log(
`📊 Table diagnostic (msg ${messageId}): pipes=${hasTablePipes}, htmlTable=${hasHtmlTable}`
);
if (!hasHtmlTable) {
console.warn(
"⚠️ Content has pipe chars but no <table> was generated.",
{ messageId, hasTablePipes, hasHtmlTable }
);

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

please update based on suggestion or think through another way to limit exposure

const sep = '|' + cells.map(() => ' --- ').join('|') + '|';
result.push(sep);
insideTable = true;
console.log('🔧 Inserted missing table separator after:', trimmed.substring(0, 80));
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

Table preprocessing currently logs to console on normal user flows (Inserted missing table separator…). This will spam DevTools during streaming/non-streaming rendering. Consider removing these logs or guarding them behind a debug flag.

Suggested change
console.log('🔧 Inserted missing table separator after:', trimmed.substring(0, 80));

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

please update based on suggestion or think through another way to limit exposure

Comment on lines +2182 to +2192
navigator.clipboard.writeText(tsv.trim()).then(() => {
// Update both buttons
topBtn.innerHTML = '<i class="bi bi-check-lg text-success"></i> Copied!';
bottomBtn.innerHTML = '<i class="bi bi-check-lg text-success"></i> Copied!';
setTimeout(() => {
topBtn.innerHTML = '<i class="bi bi-clipboard"></i> Copy table';
bottomBtn.innerHTML = '<i class="bi bi-clipboard"></i> Copy table';
}, 2000);
}).catch(err => {
console.error('Failed to copy table:', err);
});
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

attachTableCopyButtons swallows clipboard failures (console error only). For consistency with attachCodeBlockCopyButtons, please show a user-visible toast on failure and consider a fallback path when navigator.clipboard is unavailable (non-secure contexts / permissions).

Copilot uses AI. Check for mistakes.
Copy link
Contributor

Choose a reason for hiding this comment

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

please update based on suggestion

display: block;
/* Optional: prevent code from wrapping */
white-space: pre;
position: relative;
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

pre, pre[class*="language-"] now gets padding-top: 2.2rem globally to make room for a copy button. This will add extra whitespace to code blocks anywhere in the app, even when no copy button is injected. Consider scoping this to chat messages (e.g., .message-text pre...) or applying padding only when .copy-code-btn is present.

Suggested change
position: relative;
position: relative;
}
/* Apply extra top padding only to code blocks inside chat messages
to make room for the copy button */
.message-text pre,
.message-text pre[class*="language-"] {

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@paullizer paullizer Mar 12, 2026

Choose a reason for hiding this comment

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

please update based on suggestion

Comment on lines 266 to 271
if (contentElement) {
// Render markdown during streaming for proper formatting
if (typeof marked !== 'undefined' && typeof DOMPurify !== 'undefined') {
const renderedContent = DOMPurify.sanitize(marked.parse(content));
const preprocessed = preprocessTableContent(content);
const renderedContent = DOMPurify.sanitize(marked.parse(preprocessed));
contentElement.innerHTML = renderedContent;
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

updateStreamingMessage() now runs the full preprocessing pipeline on every streaming chunk. That pipeline includes several full-string passes (multiple regex conversions + wrapUnfencedCodeBlocks), which can get expensive and cause UI jank on long responses. Consider using a lighter streaming-only preprocessing (tables only), throttling updates, or deferring heavy steps until finalizeStreamingMessage().

Copilot uses AI. Check for mistakes.
Comment on lines 945 to 951
// Parse content with comprehensive table processing
let cleaned = messageContent.trim().replace(/\n{3,}/g, "\n\n");
cleaned = cleaned.replace(/(\bhttps?:\/\/\S+)(%5D|\])+/gi, (_, url) => url);
const withInlineCitations = parseCitations(cleaned);
const withUnwrappedTables = unwrapTablesFromCodeBlocks(withInlineCitations);
const withMarkdownTables = convertUnicodeTableToMarkdown(withUnwrappedTables);
const withPSVTables = convertPSVCodeBlockToMarkdown(withMarkdownTables);
const withASCIITables = convertASCIIDashTableToMarkdown(withPSVTables);
const sanitizedHtml = DOMPurify.sanitize(marked.parse(withASCIITables));
const withTableSeparators = preprocessTableContent(withInlineCitations);
const sanitizedHtml = DOMPurify.sanitize(marked.parse(withTableSeparators));
const htmlContent = addTargetBlankToExternalLinks(sanitizedHtml);
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

Existing functional tests hard-code the old table-processing pipeline strings (e.g., unwrapTablesFromCodeBlocks(withInlineCitations)); with the new preprocessTableContent() call, those tests will fail if they’re part of the validation suite (see functional_tests/test_comprehensive_table_support.py and functional_tests/test_final_table_validation.py). Please update the tests/validators to assert the new pipeline entry point instead.

Copilot uses AI. Check for mistakes.
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.

4 participants