feat(web): add fullscreen table preview and export actions - #1693
feat(web): add fullscreen table preview and export actions#1693techotaku39 wants to merge 21 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
- [Major] Neutralize formula-leading CSV cells - quoting alone does not stop spreadsheet applications from evaluating cells beginning with
=,+,-, or@. Since table text can come from agent/repository output, opening the downloaded CSV can execute attacker-controlled formulas. Evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:135.
Suggested fix:function escapeCsvCell(value: string): string { const safeValue = /^[=+\-@]/.test(value) ? `'${value}` : value return `"${safeValue.replace(/"/g, '""')}"` }
- [Major] Bound PNG rasterization size - the new export passes the full table dimensions at up to 2x scale without the pixel cap already used by
ShareTurnDialog. A large generated table can request a canvas hundreds of megapixels wide/high, exhausting memory or exceeding browser limits; mobile also starts this render eagerly when the viewer opens. Evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:243.
Suggested fix:const MAX_TABLE_EXPORT_PIXELS = 24_000_000 const maxScale = Math.sqrt(MAX_TABLE_EXPORT_PIXELS / (tableWidth * tableHeight)) const scale = Math.min(window.devicePixelRatio || 1, 2, maxScale)
- [Minor] Recognize phones already in landscape - mobile behavior is gated by viewport width, so common phone landscape widths such as 844-932px are classified as desktop and never request fullscreen/orientation lock. The E2E test only rotates after opening and does not cover opening while already landscape. Evidence
web/src/components/assistant-ui/MarkdownTable.tsx:126.
Suggested fix:const shortSide = Math.min(window.innerWidth, window.innerHeight) return shortSide <= 767 && isCoarsePointerDevice()
Summary
Review mode: initial
Three issues found in the new export/fullscreen path: CSV formula injection, unbounded image rasterization, and initial-landscape phone detection.
Testing
Not run (automation; PR code was not executed under the review security policy). GitHub integration is currently failing in unchanged CLI integration tests; the main test check was pending at review time. Add coverage for formula-leading CSV cells, oversized-table scale reduction, and opening the mobile viewer from an initial landscape viewport.
HAPI Bot
| return text.replace(/\s+/g, ' ').trim() | ||
| } | ||
|
|
||
| function escapeCsvCell(value: string): string { |
There was a problem hiding this comment.
[MAJOR] Quoting alone does not neutralize spreadsheet formulas. A table cell beginning with =, +, -, or @ is still evaluated by Excel/Sheets after the user opens the export, and this content can originate in untrusted agent/repository output.
Suggested fix:
function escapeCsvCell(value: string): string {
const safeValue = /^[=+\-@]/.test(value) ? `'${value}` : value
return `"${safeValue.replace(/"/g, '""')}"`
}Please add a regression test with formula-leading cells.
| backgroundColor: backgroundColor || null, | ||
| foreignObjectRendering: false, | ||
| logging: false, | ||
| scale: Math.min(window.devicePixelRatio || 1, 2), |
There was a problem hiding this comment.
[MAJOR] This uses the entire table dimensions at up to 2× scale with no pixel budget. Large generated tables can exceed browser canvas limits or allocate hundreds of MB/GB; on mobile, renderTableAsImage is also invoked eagerly on viewer open. The existing share-image implementation caps output at 24M pixels.
Suggested fix:
const MAX_TABLE_EXPORT_PIXELS = 24_000_000
const maxScale = Math.sqrt(MAX_TABLE_EXPORT_PIXELS / (tableWidth * tableHeight))
const scale = Math.min(window.devicePixelRatio || 1, 2, maxScale)Pass scale to html2canvas and test that oversized tables reduce it.
| /** Exported for responsive behavior tests and future table viewers. */ | ||
| export function isMobileTableViewerViewport(): boolean { | ||
| if (typeof window === 'undefined') return false | ||
| return window.matchMedia('(max-width: 767px)').matches && isCoarsePointerDevice() |
There was a problem hiding this comment.
[MINOR] Width-only detection misses phones that are already in landscape: common CSS viewport widths (844–932px) fail this check, so opening the table then uses desktop mode and skips fullscreen/orientation handling. The new E2E test starts portrait and rotates only after opening.
Suggested fix:
const shortSide = Math.min(window.innerWidth, window.innerHeight)
return shortSide <= 767 && isCoarsePointerDevice()Add an initial-landscape viewport case before clicking the open button.
There was a problem hiding this comment.
Findings
- [Major] Defer table PNG rendering until the user requests it — opening any table on a mobile-classified device immediately calls
renderTableAsImage, even when the user only wants to read, copy Markdown, or download CSV. The configured 24M-pixel ceiling still permits an RGBA canvas of roughly 96 MB before clone/encoder overhead, so a large table can stall or terminate a low-memory mobile tab merely by opening the viewer. Evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:409.
Suggested fix:useEffect(() => { if (!props.open) setPreparedImage(null) }, [props.open]) const blob = preparedImage ?? await renderTableAsImage(table) downloadBlob(blob, filename) setPreparedImage(blob)
- [Minor] Preserve column alignment when copying Markdown — the renderer honors GFM alignment such as
---:, but serialization always emits---, so copying the fixture changes the Stars column from right-aligned to left-aligned when pasted back. Evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:167.
Suggested fix:const headerCells = Array.from(table.tHead?.rows[0]?.cells ?? []) const separator = Array.from({ length: width }, (_, index) => { const align = headerCells[index]?.getAttribute('align') return align === 'center' ? ':---:' : align === 'right' ? '---:' : align === 'left' ? ':---' : '---' })
- [Minor] Do not classify every touch-enabled desktop as mobile —
maxTouchPoints > 0makes hybrid laptops pass the coarse-pointer check; after the short-side change, a normal 1366×768 touch laptop browser viewport can now request fullscreen and landscape orientation. Evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:116.
Suggested fix:const ipadDesktopUa = /Macintosh/i.test(navigator.userAgent) && navigator.maxTouchPoints > 1 return coarsePointer || mobileUserAgent || ipadDesktopUa
- [Minor] Wire the new browser tests into CI — these specs are under
web/e2e, while.github/workflows/test.yml:16runs the root Playwright config and explicitly selects only two root specs.bun run testis Vitest-only, so the added fullscreen/export coverage is not exercised on PRs. Evidenceweb/e2e/markdown-table.spec.ts:1.
Suggested fix:- run: cd web && bunx playwright install chromium - run: cd web && bunx playwright test e2e/markdown-table.spec.ts e2e/markdown-table-mobile.spec.ts
Summary
Review mode: follow-up after new commits
Four issues remain in the current full diff: one mobile performance risk, two behavior regressions, and one CI coverage gap.
Testing
Not run (automation; PR code was not executed under the review security policy). Static diff/context inspection only.
HAPI Bot
| if (!table) return undefined | ||
|
|
||
| let cancelled = false | ||
| void renderTableAsImage(table).then((blob) => { |
There was a problem hiding this comment.
[MAJOR] Opening the viewer on every mobile-classified device eagerly rasterizes the table, even if the user only reads it or exports CSV/Markdown. With the 24M-pixel cap, the canvas alone can still require roughly 96 MB of RGBA storage before clone and PNG-encoding overhead, so a large table can stall or kill a mobile tab just by opening the viewer.
Suggested fix:
useEffect(() => {
if (!props.open) setPreparedImage(null)
}, [props.open])
const blob = preparedImage ?? await renderTableAsImage(table)
downloadBlob(blob, filename)
setPreparedImage(blob)Remove the eager renderTableAsImage call from the open effect, render on Save, and add a test that opening the mobile viewer does not call html2canvas.
|
|
||
| const width = Math.max(...rows.map((row) => row.length), 1) | ||
| const header = rows[0] ?? [] | ||
| const separator = Array.from({ length: width }, () => '---') |
There was a problem hiding this comment.
[MINOR] The rendered table preserves GFM alignment, but copy serialization always emits ---. For the included ---: Stars column, copying and pasting the table changes it from right-aligned to left-aligned.
Suggested fix:
const headerCells = Array.from(table.tHead?.rows[0]?.cells ?? [])
const separator = Array.from({ length: width }, (_, index) => {
const align = headerCells[index]?.getAttribute('align')
return align === 'center' ? ':---:' : align === 'right' ? '---:' : align === 'left' ? ':---' : '---'
})Update the serialization test to retain the fixture's right alignment.
| if (typeof window === 'undefined') return false | ||
|
|
||
| const coarsePointer = window.matchMedia('(pointer: coarse)').matches | ||
| const touchPoints = typeof navigator !== 'undefined' ? navigator.maxTouchPoints : 0 |
There was a problem hiding this comment.
[MINOR] maxTouchPoints > 0 does not mean the primary pointer is coarse; it also matches Windows hybrid laptops. Combined with the new short-side check, a normal 1366×768 touch laptop browser viewport can be classified as mobile and unexpectedly request fullscreen/orientation lock.
Suggested fix:
const ipadDesktopUa = /Macintosh/i.test(navigator.userAgent) && navigator.maxTouchPoints > 1
return coarsePointer || mobileUserAgent || ipadDesktopUaThis keeps desktop-UA iPads covered without treating every touch-enabled desktop as a phone.
| @@ -0,0 +1,101 @@ | |||
| import { expect, test } from '@playwright/test' | |||
There was a problem hiding this comment.
[MINOR] These new specs are not run by PR automation. They live under web/e2e, but .github/workflows/test.yml:16 invokes the root Playwright config and explicitly selects only terminal-wrap-fidelity.spec.ts and composer-copy.spec.ts; bun run test only runs Vitest.
Suggested fix:
- run: cd web && bunx playwright install chromium
- run: cd web && bunx playwright test e2e/markdown-table.spec.ts e2e/markdown-table-mobile.spec.tsThat makes the fullscreen/export regression coverage effective on future PRs.
There was a problem hiding this comment.
Findings
- [Minor] Exclude the fullscreen affordance from share-turn snapshots —
handleShareTurncaptures messageouterHTML, and the share preview only strips controls matching its exclusion conventions. The new.aui-md-table-actionswrapper matches none of them, so the expand button survives as a dead control in the share preview and remains visible in touch-layout PNG exports; the unconditional header padding also survives after removal. Evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:642(related contextweb/src/components/AssistantChat/ShareTurnDialog.tsx:37,web/src/components/AssistantChat/ShareTurnDialog.tsx:540).
Suggested fix:<div data-hapi-share-exclude="true" className="aui-md-table-actions flex items-center" >
.aui-md-table-frame:has(> .aui-md-table-actions) .aui-md-table > thead > tr > :last-child { padding-right: 5rem; }
Summary
Review mode: follow-up after new commits
The four prior findings are resolved in the latest head. One minor share-export regression remains.
Testing
Not run by reviewer (automation/security policy). Static full-diff and surrounding-context inspection; the current CI table Playwright step passed. Add a share-turn snapshot/export assertion that .aui-md-table-actions is absent.
HAPI Bot
| {children} | ||
| </table> | ||
| </div> | ||
| <div className="aui-md-table-actions flex items-center"> |
There was a problem hiding this comment.
[MINOR] handleShareTurn snapshots the message via outerHTML, but this new action wrapper matches none of ShareTurnDialog’s control-exclusion selectors. The expand button therefore survives as a dead control in the share preview and is visible in touch-layout PNG exports; simply removing it would also leave the unconditional 5rem header padding behind.
Suggested fix:
<div
data-hapi-share-exclude="true"
className="aui-md-table-actions flex items-center"
>.aui-md-table-frame:has(> .aui-md-table-actions) .aui-md-table > thead > tr > :last-child {
padding-right: 5rem;
}Please add a share-turn regression assertion that the captured preview/export contains neither .aui-md-table-actions nor the extra action padding.
There was a problem hiding this comment.
Findings
- None.
Summary
Review mode: follow-up after new commits
No issues found in the latest full diff. The prior share-turn control/padding finding is resolved at the current head.
Testing
Not run (automation/security policy). GitHub test passed, including the table Playwright coverage. The separate integration job failed in unchanged CLI runner tests and is not attributable to this web-only diff. Residual gap: web/src/components/assistant-ui/MarkdownTable.test.tsx:93 verifies the exclusion marker, but there is still no share-turn preview/export assertion confirming that the action and its header padding are absent.
HAPI Bot
There was a problem hiding this comment.
Findings
- [Major] The newly mandatory table Playwright step is failing on the current head (
.github/workflows/test.yml:17). - [Major] The prepared PNG cache can retain and reuse a stale table render (
web/src/components/assistant-ui/MarkdownTable.tsx:818). - [Minor] The table action is 11 px off the header center in the PR's own browser check (
web/src/index.css:494).
Summary
Review mode: follow-up after new commits
Three issues found in the latest full diff. The prior bot review was against 830e0fa; the current head adds new table-viewer/export behavior and a mandatory browser-test step that is red.
Testing
Not run locally (automation/security policy). Current GitHub run 33064071048: typecheck and the earlier root E2E step passed; the added table Playwright step had 5 passed / 2 failed, so bun run test was not reached. The separate integration job also failed in unchanged CLI runner tests and is not attributable to this web-only diff.
HAPI Bot
| - run: bun typecheck | ||
| - run: bunx playwright install --with-deps chromium | ||
| - run: bun run test:e2e -- terminal-wrap-fidelity.spec.ts composer-copy.spec.ts | ||
| - run: cd web && bunx playwright test --config playwright.config.ts e2e/markdown-table.spec.ts e2e/markdown-table-mobile.spec.ts |
There was a problem hiding this comment.
[MAJOR] Prebundle the remaining Workbox client before making this suite mandatory.
The clean current-head run fails here. Its log reports new dependencies optimized: workbox-window followed by optimized dependencies changed. reloading; that reload tears down the mobile table dialog, and the close-button click times out on a detached element. web/vite.config.ts:183 already prebundles Workbox modules specifically to prevent this E2E reload, but omits workbox-window.
Suggested fix:
optimizeDeps: {
include: [
'workbox-precaching',
'workbox-routing',
'workbox-strategies',
'workbox-expiration',
'workbox-window',
],
},| }, [copy, props.tableRef]) | ||
|
|
||
| const getPreparedImage = useCallback((table: HTMLTableElement): Promise<Blob> => { | ||
| if (preparedImage) return Promise.resolve(preparedImage) |
There was a problem hiding this comment.
[MAJOR] Do not reuse a blob unless it still matches the current table.
preparedImage is cleared by effects when the viewer closes or wrapping changes, but an in-flight renderTableAsImage() can resolve afterward and repopulate it. Streaming table children also do not invalidate it. Reopening or exporting again can therefore copy/download old rows or the previous wrap layout.
The minimal safe fix is to remove this cache; if caching is required, key it by the current DOM plus wrap state and invalidate in-flight generations.
Suggested fix:
const getPreparedImage = useCallback(
(table: HTMLTableElement) => renderTableAsImage(table),
[],
)|
|
||
| .aui-md-table-actions { | ||
| position: absolute; | ||
| top: 0.1875rem; |
There was a problem hiding this comment.
[MINOR] Center the action against the header row instead of using a fixed top offset.
The PR's added browser assertion at web/e2e/markdown-table.spec.ts:223 currently receives an 11 px center delta (limit: 2 px), so the current test check is red and the control is visibly high relative to the header. Size the overlay from the same line-height and vertical padding as .aui-md-th.
Suggested fix:
.aui-md-table-actions {
top: 0;
min-height: calc(1lh + 1rem); /* header line box + py-2 */
align-items: center;
}
There was a problem hiding this comment.
Findings
- [Major] The follow-up header-action alignment fix still fails the newly required Playwright check (web/src/index.css:496).
- [Major] Copy Image performs the clipboard write only after asynchronous rasterization, outside the original click activation (web/src/components/assistant-ui/MarkdownTable.tsx:831).
- [Minor] Automatic wrapping can latch the portrait measurement before mobile landscape rotation completes (web/src/components/assistant-ui/MarkdownTable.tsx:768).
Questions
- None.
Summary
- Review mode: follow-up after new commits
- The current head still has a required browser-test failure and two browser-lifecycle regressions.
Testing
- Not run locally (automation; PR code was not executed). Current GitHub Actions: test fails because the added geometry assertion receives 10 px (expected <=2 px). integration also fails in unchanged CLI tests and is not attributed to this web-only diff.
HAPI Bot
| position: absolute; | ||
| top: 0; | ||
| right: 0.1875rem; | ||
| min-height: calc(1.25rem + 1rem); |
There was a problem hiding this comment.
[MAJOR] The alignment follow-up still fails the required browser test.
The current test job fails at web/e2e/markdown-table.spec.ts:223: actionHeaderCenterDelta is 10 px, with a limit of 2 px. The hard-coded 1.25rem does not match the inherited table line height, so this follow-up does not resolve the prior finding and leaves the PR's newly added required check red.
Suggested fix:
.aui-md-table-actions {
min-height: calc(1lh + 1rem); /* inherited line box + py-2 */
}| setImageAction('copy') | ||
| void getPreparedImage(table) | ||
| .then(async (blob) => { | ||
| await copyTableImageToClipboard(blob) |
There was a problem hiding this comment.
[MAJOR] Start the clipboard write inside the menu-item click.
renderTableAsImage() dynamically imports and rasterizes before navigator.clipboard.write() runs. Browsers that require transient user activation for image clipboard writes reject the later call; the catch then silently clears the spinner, so the new Copy Image action appears to do nothing. Pass the render promise into ClipboardItem and invoke write() synchronously from the click handler.
Suggested fix:
const imagePromise = renderTableAsImage(table)
void navigator.clipboard.write([
new ClipboardItem({ 'image/png': imagePromise }),
])
.then(markCopied)
.catch(() => {/* surface the failure */})
.finally(() => setImageAction(null))| const table = props.tableRef.current | ||
| if (!viewer || !table) return | ||
| const shouldWrap = shouldWrapTableByDefault(table, viewer) | ||
| if (shouldWrap) automaticWrapRef.current = false |
There was a problem hiding this comment.
[MINOR] Re-evaluate the automatic default after landscape rotation.
On first mobile open, the viewer can measure the portrait viewport before orientation.lock() finishes. If that measurement overflows, this line permanently disables automatic measurement; the later resize/ResizeObserver callbacks immediately return, so the landscape viewer remains wrapped based on portrait geometry. The mobile E2E changes the viewport after opening but accepts either wrap value, so it does not cover this regression.
Suggested fix:
const hasExplicitWrapRef = useRef(false)
// when opening
hasExplicitWrapRef.current = storedPreference !== null
const measureOverflow = () => {
if (hasExplicitWrapRef.current) return
const viewer = viewerRef.current
const table = props.tableRef.current
if (!viewer || !table) return
setWrapEnabled(shouldWrapTableByDefault(table, viewer))
}
// in handleWrapToggle
hasExplicitWrapRef.current = true
There was a problem hiding this comment.
Findings
- [Major] The header-action alignment fix still leaves the required browser check red —
line-height: 1.25remmakes1lhequal the same hard-coded 1.25rem, so the computed minimum height is still 36px. Current-head CI still reports a 10px center delta (expected ≤2px), evidenceweb/src/index.css:496.
Suggested fix:.aui-md-table-actions { /* inherit the markdown surface line height */ min-height: calc(1lh + 1rem); }
- [Major] Vertical tiling does not bound the final output canvas — the scale only caps total pixel area, while
outputCanvas.height = ceil(tableHeight * scale)remains unbounded. A sufficiently narrow, tall table can exceed the browser's canvas-dimension limit and fail attoBlob, despite entering the tiled path, evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:504andweb/src/components/assistant-ui/MarkdownTable.tsx:573.
Suggested fix:const MAX_TABLE_EXPORT_DIMENSION = 16_384 const dimensionScale = MAX_TABLE_EXPORT_DIMENSION / Math.max(tableWidth, tableHeight) return Math.min( devicePixelRatio || 1, 2, Math.sqrt(MAX_TABLE_EXPORT_PIXELS / area), dimensionScale, )
- [Minor] Automatic wrap remeasurement reads already-wrapped geometry — once wrapping is enabled, the new CSS forces the table to 100% width, so
table.scrollWidthno longer exposes intrinsic overflow. A resize/orientation callback can therefore disable wrapping even when the new viewport is still too narrow; the added unit test mocksscrollWidthindependently of this CSS and misses the case, evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:778.
Suggested fix:const wrapped = table.dataset.hapiTableWrap table.removeAttribute('data-hapi-table-wrap') const intrinsicWidth = table.scrollWidth if (wrapped) table.dataset.hapiTableWrap = wrapped setWrapEnabled(intrinsicWidth > viewer.clientWidth + 1)
Questions
- None.
Summary
Review mode: follow-up after new commits
The current head still has one required browser-test failure plus long-table export and resize-lifecycle regressions.
Testing
- Not run locally (automation; PR code was not executed).
- Current-head CI: the added table Playwright suite passed 6/7 and failed the geometry assertion;
bun run testwas not reached. CLI integration failures are outside this web-only diff and are not attributed here. - Add coverage for narrow-to-narrow resize while auto-wrapped and for maximum final canvas dimensions on a very tall table.
HAPI Bot
| position: absolute; | ||
| top: 0; | ||
| right: 0.1875rem; | ||
| line-height: 1.25rem; |
There was a problem hiding this comment.
[MAJOR] The alignment fix still computes the previous height.
Setting line-height: 1.25rem makes the following 1lh exactly 1.25rem, so min-height remains 36px—the same geometry as the previous revision. Current-head CI still fails web/e2e/markdown-table.spec.ts:223 with a 10px center delta (expected ≤2px).
Suggested fix:
.aui-md-table-actions {
/* inherit the markdown surface line height */
min-height: calc(1lh + 1rem);
}| return Math.min( | ||
| devicePixelRatio || 1, | ||
| 2, | ||
| Math.sqrt(MAX_TABLE_EXPORT_PIXELS / area), |
There was a problem hiding this comment.
[MAJOR] Bound canvas dimensions as well as pixel area.
This scale limits area only. The tiled branch later allocates one final canvas with height = ceil(tableHeight * scale), so a narrow/tall table can still exceed the browser's finite canvas-dimension limit and make toBlob fail. Tiling the inputs does not bound that final allocation.
Suggested fix:
const MAX_TABLE_EXPORT_DIMENSION = 16_384
const dimensionScale =
MAX_TABLE_EXPORT_DIMENSION / Math.max(tableWidth, tableHeight)
return Math.min(
devicePixelRatio || 1,
2,
Math.sqrt(MAX_TABLE_EXPORT_PIXELS / area),
dimensionScale,
)| const viewer = viewerRef.current | ||
| const table = props.tableRef.current | ||
| if (!viewer || !table) return | ||
| const shouldWrap = shouldWrapTableByDefault(table, viewer) |
There was a problem hiding this comment.
[MINOR] Measure intrinsic width, not the wrapped table.
When wrapEnabled is true, [data-hapi-table-wrap] forces the table to 100% width and breaks cell content. A resize/orientation callback therefore sees scrollWidth <= clientWidth and turns wrapping off even when the new viewport still cannot fit the unwrapped table. The new test mocks scrollWidth independently of the applied CSS, so it does not exercise this path.
Suggested fix:
const wrapped = table.dataset.hapiTableWrap
table.removeAttribute('data-hapi-table-wrap')
const intrinsicWidth = table.scrollWidth
if (wrapped) table.dataset.hapiTableWrap = wrapped
setWrapEnabled(intrinsicWidth > viewer.clientWidth + 1)
There was a problem hiding this comment.
Findings
- [Major] Header actions remain vertically misaligned when a header row wraps — the action box still derives a fixed height from one line, while the header row can grow to multiple lines. Current-head CI fails the required assertion with an 11px center delta, evidence
web/src/index.css:496andweb/e2e/markdown-table.spec.ts:223.
Suggested fix:useLayoutEffect(() => { const row = inlineTableRef.current?.tHead?.rows[0] const actions = inlineActionsRef.current if (!row || !actions) return const sync = () => { actions.style.height = `${row.getBoundingClientRect().height}px` } sync() const observer = new ResizeObserver(sync) observer.observe(row) return () => observer.disconnect() }, [children])
- [Minor] Manual browser-fullscreen exit skips orientation cleanup — the
fullscreenchangepath closes the viewer and clears the mobile refs without calling the same unlock path used by the close button. Implementations that retain the explicit lock can leave the app in landscape, evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:1077.
Suggested fix:if (!document.fullscreenElement) { leaveMobileTableViewer(false) // already exited; release orientation only enteredFullscreenRef.current = false mobileViewerRef.current = false openRef.current = false setViewerOpen(false) }
- [Minor] Fractional export scales can overlap or skip tile-boundary pixels — each tile canvas rounds its height up, but its destination offset is rounded independently. For example, a 1666px tile at scale 1.2 produces 2000px while the next tile starts at 1999px, duplicating a row in the PNG, evidence
web/src/components/assistant-ui/MarkdownTable.tsx:599.
Suggested fix:const destinationTop = Math.round(tileTop * scale) const destinationBottom = Math.round((tileTop + currentTileHeight) * scale) outputContext.drawImage( tileCanvas, 0, 0, tileCanvas.width, tileCanvas.height, 0, destinationTop, outputCanvas.width, destinationBottom - destinationTop, )
Questions
- None.
Summary
Review mode: follow-up after new commits
Current head remains non-green: the added table Playwright suite passes 6/7 and still fails its header-action geometry assertion. Two additional mobile-lifecycle and large-export edge cases remain.
Testing
- Not run locally (automation; PR code was not executed).
- Current-head CI: table Playwright 6/7;
bun run testwas not reached. CLI integration failures are outside this web-only diff and are not attributed here. - Add regression coverage for manual
fullscreenchangecleanup and contiguous tile destinations at a fractional scale.
HAPI Bot
| position: absolute; | ||
| top: 0; | ||
| right: 0.1875rem; | ||
| line-height: normal; |
There was a problem hiding this comment.
[MAJOR] Size the action overlay from the actual header row.
line-height: normal still gives this overlay a one-line fixed height, but the header row grows when any heading wraps. Current-head CI measures an 11px center delta at web/e2e/markdown-table.spec.ts:223 (expected ≤2px), so this required browser check remains red.
Suggested fix:
useLayoutEffect(() => {
const row = inlineTableRef.current?.tHead?.rows[0]
const actions = inlineActionsRef.current
if (!row || !actions) return
const sync = () => { actions.style.height = `${row.getBoundingClientRect().height}px` }
sync()
const observer = new ResizeObserver(sync)
observer.observe(row)
return () => observer.disconnect()
}, [children])Attach inlineActionsRef to .aui-md-table-actions and let that measured height replace the one-line min-height assumption.
| if (!openRef.current || !mobileViewerRef.current || !enteredFullscreenRef.current) return | ||
| if (document.fullscreenElement) return | ||
|
|
||
| enteredFullscreenRef.current = false |
There was a problem hiding this comment.
[MINOR] Unlock orientation on manual fullscreen exit.
This path closes the viewer and clears the mobile refs without calling the cleanup used by the close button. If the user exits browser fullscreen directly, the explicit landscape lock is therefore never released by HAPI.
Suggested fix:
if (!document.fullscreenElement) {
leaveMobileTableViewer(false) // fullscreen already ended; unlock only
enteredFullscreenRef.current = false
mobileViewerRef.current = false
openRef.current = false
setViewerOpen(false)
}Add a test that dispatches fullscreenchange with no fullscreen element and asserts orientation.unlock() is called.
| height: currentTileHeight, | ||
| windowHeight: Math.max(document.documentElement.clientHeight, currentTileHeight), | ||
| }) | ||
| outputContext.drawImage(tileCanvas, 0, Math.round(tileTop * scale)) |
There was a problem hiding this comment.
[MINOR] Stitch fractional-scale tiles to shared rounded boundaries.
The tile canvas height is effectively ceil(currentTileHeight * scale), while the destination Y uses round(tileTop * scale). At fractional scales these disagree: a 1666px tile at 1.2x is 2000px tall, but the next tile begins at 1999px, duplicating a boundary row (other values can leave gaps).
Suggested fix:
const destinationTop = Math.round(tileTop * scale)
const destinationBottom = Math.round((tileTop + currentTileHeight) * scale)
outputContext.drawImage(
tileCanvas,
0, 0, tileCanvas.width, tileCanvas.height,
0, destinationTop, outputCanvas.width, destinationBottom - destinationTop,
)Cover this with a fractional-scale test that verifies every destination interval starts where the previous one ends.
There was a problem hiding this comment.
Findings
- [Major] Current head does not typecheck — the new wrapped-header test calls
tHeadon anHTMLElement, and current-head CI stops at this error before the added browser and unit suites run, evidenceweb/src/components/assistant-ui/MarkdownTable.test.tsx:124.
Suggested fix:const table = screen.getByRole<HTMLTableElement>('table')
- [Minor] Shared-preview tables retain the inline
w-fullsizing — the captured table class is passed after the viewer'sw-max, sotwMergekeepsw-full. Wide tables in the shared-turn viewer can therefore wrap/shrink even while the wrap toggle reports disabled, evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:1021(related inline class atweb/src/components/assistant-ui/MarkdownTable.tsx:1138).
Suggested fix:className: cn(props.table.getAttribute('class'), 'w-max min-w-full'),
- [Minor] Standalone table viewers bypass localization — when no chat context exists, the hard-coded
Tabletitle preventstable.viewerTitlefrom being used, so Simplified Chinese file-preview viewers still show an English title and filename component, evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:1049.
Suggested fix:const imageTitle = chatContext?.sessionTitle?.trim() || t('table.viewerTitle')
Questions
- None.
Summary
Review mode: follow-up after new commits
Current head remains non-green because of the added test's TypeScript error. The previous bot findings are addressed in the latest source, but the shared-preview sizing path and standalone localized fallback still need correction.
Testing
- Not run locally (automation; PR code was not executed).
- Current-head CI:
bun typecheckfails atMarkdownTable.test.tsx:124; table Playwright and unit suites were not reached. - CLI integration failures are outside this web-only diff and are not attributed here.
- Add coverage using a shared-preview source table with
w-full, plus a Simplified Chinese standalone viewer assertion.
HAPI Bot
| try { | ||
| renderTable() | ||
| const table = screen.getByRole('table') | ||
| const row = table.tHead?.rows[0] |
There was a problem hiding this comment.
[MAJOR] Keep the new test type-safe.
screen.getByRole() defaults to HTMLElement, so accessing table.tHead fails strict TypeScript. This is the current-head CI failure, and the workflow stops before the new Playwright and Vitest coverage runs.
Suggested fix:
const table = screen.getByRole<HTMLTableElement>('table')| }) { | ||
| const tableRef = useRef<HTMLTableElement | null>(null) | ||
| const tableProps: TableProps = { | ||
| className: props.table.getAttribute('class') ?? undefined, |
There was a problem hiding this comment.
[MINOR] Preserve max-content sizing in the shared-preview viewer.
The source element is the inline table rendered at line 1138, so its captured class list contains w-full. Passing that list here after TableViewer's w-max makes twMerge keep the later w-full; wide shared-preview tables can then shrink/wrap while the toolbar says wrapping is disabled, and the automatic overflow measurement sees the wrong layout.
Suggested fix:
className: cn(props.table.getAttribute('class'), 'w-max min-w-full'),Cover this with a shared-preview table whose captured class includes w-full, asserting the viewer table retains w-max and can overflow before wrapping is enabled.
| const openRef = useRef(false) | ||
| const mobileViewerRef = useRef(false) | ||
| const enteredFullscreenRef = useRef(false) | ||
| const imageTitle = chatContext?.sessionTitle ?? 'Table' |
There was a problem hiding this comment.
[MINOR] Use the localized fallback title outside chat sessions.
This hard-coded fallback is always non-empty, so TableViewer never reaches t('table.viewerTitle'). Standalone/file-preview tables in Simplified Chinese therefore display Table and use it in exported filenames despite the added 表格 locale entry.
Suggested fix:
const imageTitle = chatContext?.sessionTitle?.trim() || t('table.viewerTitle')
There was a problem hiding this comment.
Findings
- [Minor] Image actions silently discard failures — both PNG download and Copy image clear the progress state after a rejected render/clipboard operation without showing an error, so permission denial, unsupported clipboard behavior, or PNG encoding failure appears as a no-op, evidence
web/src/components/assistant-ui/MarkdownTable.tsx:855(same pattern at:867).
Suggested fix:const [imageError, setImageError] = useState<string | null>(null) // Clear before starting either action. setImageError(null) void getPreparedImage(table) .then(/* existing action */) .catch(() => setImageError(t('table.imageActionFailed'))) {imageError ? <div role="alert">{imageError}</div> : null}
Questions
- None.
Summary
Review mode: follow-up after new commits
The latest commits resolve the prior typecheck, shared-preview sizing, and standalone localization findings. One remaining failure-path issue affects the new image actions.
Testing
- Not run locally (automation; PR code was not executed).
- Current-head
testjob passed: typecheck, table Playwright suites, and repository unit tests. - Current-head CLI integration job failed in runner session tests; no CLI files are changed by this PR, so it is not attributed here.
- Add rejected
html2canvasand rejected clipboard-write tests asserting visible error feedback.
HAPI Bot
| const filename = getShareImageFileName(props.imageTitle, 'table') | ||
| void getPreparedImage(table) | ||
| .then((blob) => downloadBlob(blob, filename)) | ||
| .catch(() => undefined) |
There was a problem hiding this comment.
[MINOR] Surface image-action failures.
This catch discards every rendering or download error; the Copy image path does the same below. A denied clipboard write or failed PNG encode therefore only removes the spinner and leaves the user with no result or explanation.
Suggested fix:
const [imageError, setImageError] = useState<string | null>(null)
setImageError(null)
void getPreparedImage(table)
.then((blob) => downloadBlob(blob, filename))
.catch(() => setImageError(t('table.imageActionFailed')))Render imageError with role="alert", and use the same handler for Copy image.
There was a problem hiding this comment.
Findings
- [Minor] Awaiting the orientation lock delays fullscreen bookkeeping — after
requestFullscreen()succeeds, the helper waits fororientation.lock()before callers set theirenteredFullscreenrefs. If the user exits browser fullscreen while that lock promise is pending, bothfullscreenchangehandlers ignore the exit because the refs are still false; when the promise later settles, the refs become true after the only exit event has passed, leaving the table viewer open and its cleanup path unrun. Evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:671(callers at:1091andweb/src/components/AssistantChat/ShareTurnDialog.tsx:548).
Suggested fix:if (orientation && typeof orientation.lock === 'function') { try { void orientation.lock('landscape').catch(() => undefined) } catch { // Unsupported browser; keep the viewer usable. } } return enteredFullscreen
Summary
Review mode: follow-up after new commits
The latest commit resolves the prior image-failure feedback finding. One mobile fullscreen lifecycle race remains.
Testing
- Not run locally (automation; PR code was not executed).
- Current-head
testjob passed, including typecheck, table Playwright suites, and repository unit tests. - Current-head
integrationjob failed only in unchangedcli/src/runner/runner.integration.test.ts; not attributed to this web-only diff. - Add a delayed
orientation.lock()test that dispatches externalfullscreenchangebefore the lock settles and asserts the viewer closes and orientation unlocks.
HAPI Bot
| : undefined | ||
| if (orientation && typeof orientation.lock === 'function') { | ||
| try { | ||
| await orientation.lock('landscape') |
There was a problem hiding this comment.
[MINOR] Record fullscreen before waiting on the orientation lock.
After requestFullscreen() succeeds, this await prevents both callers from setting their enteredFullscreen refs until rotation finishes. If the user exits fullscreen while the lock promise is pending, the fullscreenchange handlers see a false ref and ignore the exit; the promise then settles and records fullscreen after the only exit event has passed, leaving the viewer open and cleanup unrun.
Suggested fix:
if (orientation && typeof orientation.lock === 'function') {
try {
void orientation.lock('landscape').catch(() => undefined)
} catch {
// Unsupported browser; keep the viewer usable.
}
}
return enteredFullscreenAdd a regression test with a deferred lock promise and an external fullscreen exit before it resolves.
There was a problem hiding this comment.
Findings
- [Minor] Copy Markdown drops inline links and formatting — every cell is flattened through rendered plain text, so a cell such as
[Docs](https://example.com)becomes onlyDocs; inline code and emphasis are also lost. Evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:243
Suggested fix:function serializeInlineMarkdown(node: Node): string { if (node.nodeType === Node.TEXT_NODE) return node.textContent ?? String() if (node instanceof HTMLAnchorElement) { const label = Array.from(node.childNodes, serializeInlineMarkdown).join(String()) return `[${label}](${node.getAttribute("href") ?? String()})` } if (node instanceof HTMLElement && node.tagName === "CODE") { return `\`${node.textContent ?? String()}\`` } return Array.from(node.childNodes, serializeInlineMarkdown).join(String()) } const rows = Array.from(table.rows).map((row) => Array.from(row.cells).map((cell) => Array.from(cell.childNodes, serializeInlineMarkdown).join(String()).trim(), ), )
Summary
Review mode: follow-up after new commits
The new commit removes the pending orientation-lock race identified in the prior review. One minor Markdown-export correctness issue remains. Residual browser risk remains around Clipboard/fullscreen implementations outside Chromium.
Testing
- Not run locally (automation; PR code was not executed).
- Current
testjob passed, including typecheck, the added table Playwright suites, and repository tests. - Current
integrationjob failed in unchangedcli/src/runner/runner.integration.test.tsrunner/session cases; not attributed to this frontend-only diff. - Add coverage for table cells containing links, inline code, and emphasis.
HAPI Bot
| } | ||
|
|
||
| export function serializeTableToMarkdown(table: HTMLTableElement): string { | ||
| const rows = Array.from(table.rows).map((row) => |
There was a problem hiding this comment.
[MINOR] Preserve inline Markdown semantics in Copy Markdown.
This maps every cell through getTableCellText, so rich GFM cells are flattened before export. For example, [Docs](https://example.com) becomes Docs, losing the URL; inline code and emphasis lose their delimiters as well.
Suggested fix:
function serializeInlineMarkdown(node: Node): string {
if (node.nodeType === Node.TEXT_NODE) return node.textContent ?? String()
if (node instanceof HTMLAnchorElement) {
const label = Array.from(node.childNodes, serializeInlineMarkdown).join(String())
return `[${label}](${node.getAttribute("href") ?? String()})`
}
if (node instanceof HTMLElement && node.tagName === "CODE") {
return `\`${node.textContent ?? String()}\``
}
return Array.from(node.childNodes, serializeInlineMarkdown).join(String())
}
const rows = Array.from(table.rows).map((row) =>
Array.from(row.cells).map((cell) =>
Array.from(cell.childNodes, serializeInlineMarkdown).join(String()).trim(),
),
)Please add a regression test with a linked cell and inline code.
There was a problem hiding this comment.
Findings
-
[Minor] Synchronous image-clipboard failures leave the action permanently busy —
ClipboardItemconstruction andnavigator.clipboard.write(...)can throw before this helper returns a promise. In that case, the caller's.catch(...).finally(...)chain is never installed, soimageActionremains"copy", the spinner stays visible, and image actions remain disabled even after closing/reopening this table. Evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:680Suggested fix:
function writeTableImageToClipboard( mimeType: string, image: Blob | PromiseLike<Blob>, ): Promise<void> { try { const ClipboardItemCtor = window.ClipboardItem if (!navigator.clipboard?.write || !ClipboardItemCtor) { throw new Error('Image clipboard is not supported in this browser') } return Promise.resolve(navigator.clipboard.write([ new ClipboardItemCtor({ [mimeType]: image }), ])) } catch (error) { return Promise.reject(error) } }
Summary
Review mode: follow-up after new commits
The latest commit resolves the prior Copy Markdown formatting finding. One cross-browser clipboard failure path remains. Residual browser risk remains around Clipboard/fullscreen implementations outside Chromium.
Testing
- Not run locally (automation; PR code was not executed).
- Current
testjob passed, including the added table Playwright suites and repository tests. - Current
integrationjob failed in unchangedcli/src/runner/runner.integration.test.tssession/runner cases; not attributed to this frontend-only diff. - Add a unit test where the
ClipboardItemconstructor (and optionallyclipboard.write) throws synchronously; assert the localized alert appears and the busy status clears.
HAPI Bot
| return Promise.reject(new Error('Image clipboard is not supported in this browser')) | ||
| } | ||
| return navigator.clipboard.write([ | ||
| new ClipboardItemCtor({ [mimeType]: image }), |
There was a problem hiding this comment.
[MINOR] Handle synchronous clipboard API exceptions.
ClipboardItem construction or navigator.clipboard.write(...) can throw synchronously. When that happens here, copyTableImagePromiseToClipboard(...) never returns a promise, so the caller cannot run its .catch(...).finally(...); imageAction stays "copy" and the UI remains stuck busy/disabled.
Suggested fix:
function writeTableImageToClipboard(
mimeType: string,
image: Blob | PromiseLike<Blob>,
): Promise<void> {
try {
const ClipboardItemCtor = window.ClipboardItem
if (!navigator.clipboard?.write || !ClipboardItemCtor) {
throw new Error('Image clipboard is not supported in this browser')
}
return Promise.resolve(navigator.clipboard.write([
new ClipboardItemCtor({ [mimeType]: image }),
]))
} catch (error) {
return Promise.reject(error)
}
}Please add a regression test with a ClipboardItem stub whose constructor throws and verify the alert appears and the busy status clears.
There was a problem hiding this comment.
Findings
-
[Minor] Copy Markdown can lose original link destinations - the serializer reads the rendered DOM
href. HAPI intentionally replaces unapproved custom-scheme links with#, and share-preview cleanup removeshrefentirely, so otherwise valid table links are copied as[label](#)or plain text. Evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:234(related contextweb/src/components/assistant-ui/markdown-text.tsx:692andweb/src/components/AssistantChat/ShareTurnDialog.tsx:67).Suggested fix:
// Preserve the source destination on rendered anchors. <a {...rest} data-hapi-markdown-href={href} href={domHref} /> // Before share-preview cleanup removes href: const href = anchor.getAttribute('href') if (href && !anchor.dataset.hapiMarkdownHref) { anchor.dataset.hapiMarkdownHref = href } // Serializer: const href = element.dataset.hapiMarkdownHref ?? element.getAttribute('href')
-
[Minor] Inline-code serialization is not round-trip safe - every
<code>is wrapped in a single backtick and the completed cell is then whitespace-collapsed. A valid source such as a double-fenced code span containing a backtick becomes invalid Markdown, and significant repeated spaces inside code are changed. Evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:239andweb/src/components/assistant-ui/MarkdownTable.tsx:264.Suggested fix:
function serializeCodeSpan(value: string): string { const longestRun = Math.max( 0, ...Array.from(value.matchAll(/`+/g), (match) => match[0].length), ) const fence = '`'.repeat(longestRun + 1) const needsPadding = value.startsWith('`') || value.endsWith('`') || (value.startsWith(' ') && value.endsWith(' ') && value.trim().length > 0) return `${fence}${needsPadding ? ' ' : ''}${value}${needsPadding ? ' ' : ''}${fence}` } // Normalize prose text nodes, not the fully serialized cell. if (node.nodeType === Node.TEXT_NODE) { return (node.textContent ?? '').replace(/\s+/g, ' ') } case 'code': return serializeCodeSpan(element.textContent ?? '') return Array.from(cell.childNodes, serializeInlineMarkdown).join('').trim()
Questions
- None.
Summary
Review mode: follow-up after new commits
The latest commit addresses the prior synchronous clipboard-exception finding. Two Markdown-copy data-fidelity gaps remain.
Testing
- Not run locally (automation/security policy; PR code was not executed).
- Current
testworkflow passed, including typecheck, table Playwright suites, and repository tests. - Current
integrationworkflow failed in unchanged CLI runner tests; no CLI files are changed, so it is not attributed to this PR diff. - Add regression tests for custom/sanitized links in both normal and shared-preview viewers, plus code spans containing backticks and repeated spaces.
HAPI Bot
| const children = () => Array.from(element.childNodes, serializeInlineMarkdown).join('') | ||
| switch (element.tagName.toLowerCase()) { | ||
| case 'a': { | ||
| const href = element.getAttribute('href') |
There was a problem hiding this comment.
[MINOR] Preserve original link destinations when copying Markdown.
This reads the rendered DOM href, but HAPI deliberately changes some destinations before rendering: unapproved custom schemes become #, and share-preview cleanup removes href entirely. The new Copy Markdown action therefore emits [label](#) or only the label instead of the original valid target.
Suggested fix:
// On rendered anchors:
data-hapi-markdown-href={href}
// Before share-preview cleanup removes href:
const href = anchor.getAttribute('href')
if (href && !anchor.dataset.hapiMarkdownHref) {
anchor.dataset.hapiMarkdownHref = href
}
// Here:
const href = element.dataset.hapiMarkdownHref ?? element.getAttribute('href')Add a shared-preview regression test with an HTTPS link and a normal-viewer test for a custom-scheme link.
| return href ? `[${label}](${href})` : label | ||
| } | ||
| case 'code': | ||
| return `\`${element.textContent ?? ''}\`` |
There was a problem hiding this comment.
[MINOR] Serialize code spans with a safe backtick fence.
A single hard-coded backtick cannot represent code text that itself contains a backtick. The later cell-wide .replace(/\s+/g, ' ') also changes significant repeated spaces inside code. For example, a valid double-fenced source containing a\b` is copied as invalid Markdown.
Suggested fix:
function serializeCodeSpan(value: string): string {
const longestRun = Math.max(
0,
...Array.from(value.matchAll(/`+/g), (match) => match[0].length),
)
const fence = '`'.repeat(longestRun + 1)
const needsPadding = value.startsWith('`')
|| value.endsWith('`')
|| (value.startsWith(' ') && value.endsWith(' ') && value.trim().length > 0)
return `${fence}${needsPadding ? ' ' : ''}${value}${needsPadding ? ' ' : ''}${fence}`
}
// Normalize prose at the text-node boundary instead of after serialization.
if (node.nodeType === Node.TEXT_NODE) {
return (node.textContent ?? '').replace(/\s+/g, ' ')
}
case 'code':
return serializeCodeSpan(element.textContent ?? '')Then remove the cell-wide whitespace collapse and add cases for embedded backticks and repeated spaces.
There was a problem hiding this comment.
Findings
-
[Minor] Chat file links copy an internal HAPI URL —
remarkFilePathLinksrewrites explicit relative file links before rendering, while the new source attribute records the rewrittenhapi-file:destination. Copy Markdown therefore emits an app-internal URL and loses any original fragment, query, or line suffix. Evidenceweb/src/components/assistant-ui/markdown-text.tsx:534(related contextweb/src/lib/remark-file-path-links.ts:214).
Suggested fix:// Preserve the source URL before node.url is rewritten. node.data = { ...node.data, hProperties: { ...node.data?.hProperties, 'data-hapi-markdown-href': url, }, } node.url = createFileHref(target) // FilePathAnchor: prefer the preserved source URL. data-hapi-markdown-href={sourceHref ?? filePath}
-
[Minor] Standalone/file-preview links serialize as plain text — path-like links render through
InertMarkdownHrefas spans, but the new serializer only recognizes anchors. File previews use the standalone renderer, so Copy Markdown loses the destination and outputs only the label. Evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:233(related contextweb/src/components/assistant-ui/markdown-text.tsx:592,web/src/routes/sessions/file.tsx:483).
Suggested fix:<span data-hapi-markdown-href={props.href} {...rest}> {props.children} </span> case 'span': { const href = element.dataset.hapiMarkdownHref return href ? `[${children()}](${href})` : children() }
Questions
- None.
Summary
Review mode: follow-up after new commits
The follow-up addresses the prior custom-scheme/share-preview destination and code-fence findings. Two file-link rendering paths still do not round-trip through Copy Markdown.
Testing
- Not run locally (automation/security policy; PR code was not executed).
- Current GitHub
testworkflow passed, including typecheck, table Playwright suites, and repository tests. - Current
integrationworkflow failed in unchangedcli/src/runner/runner.integration.test.tscoverage (4 failed, 9 passed, 1 skipped); no CLI/shared source is changed, so it is not attributed to this diff. - Add regression cases for a chat table link such as
[Guide](docs/guide.md#setup)and the same link in the standalone file-preview renderer.
HAPI Bot
| <a | ||
| {...anchorProps} | ||
| href={href} | ||
| data-hapi-markdown-href={anchorProps.href} |
There was a problem hiding this comment.
[MINOR] Preserve the pre-rewrite file-link destination.
remarkFilePathLinks changes an explicit relative link such as docs/foo.md#setup into hapi-file:docs%2Ffoo.md before this component runs, and it strips the fragment/query/line suffix for navigation. Storing anchorProps.href here therefore makes Copy Markdown emit the internal HAPI target rather than the authored destination.
Suggested fix:
// In rewriteFileLinkNode, preserve the original URL as an hProperty
// before assigning node.url = createFileHref(target).
node.data = {
...node.data,
hProperties: {
...node.data?.hProperties,
'data-hapi-markdown-href': url,
},
}
// Here, prefer that preserved prop; fall back to the decoded file path.
data-hapi-markdown-href={sourceHref ?? filePath}Add a regression case for [Guide](docs/guide.md#setup) and assert the copied Markdown keeps the original URL.
| const element = node as HTMLElement | ||
| const children = () => Array.from(element.childNodes, serializeInlineMarkdown).join('') | ||
| switch (element.tagName.toLowerCase()) { | ||
| case 'a': { |
There was a problem hiding this comment.
[MINOR] Serialize links that the renderer intentionally makes inert.
The serializer only recognizes <a>. HAPI renders path-like links through InertMarkdownHref as <span> when navigation is unavailable; the file-preview route uses this standalone renderer. Copy Markdown from those tables therefore returns only the label and drops the destination.
Suggested fix:
// In InertMarkdownHref:
<span data-hapi-markdown-href={props.href} ...>
{props.children}
</span>
// In serializeInlineMarkdown:
case 'span': {
const href = element.dataset.hapiMarkdownHref
return href ? `[${children()}](${href})` : children()
}Add a standalone MarkdownRenderer regression test with [Guide](docs/guide.md).
There was a problem hiding this comment.
Findings
-
[Minor] Copy Markdown doubles backslashes inside inline code - the serializer constructs a code span and then applies a cell-wide backslash escape. For a rendered
<code>C:\tmp</code>, the copied source becomes`C:\\tmp`, and code-span backslashes are literal, so the copied table renders different content. Evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:221.
Suggested fix:function serializeMarkdownText(value: string): string { return value.replace(/\s+/g, ' ').replace(/\\/g, '\\\\') } if (node.nodeType === Node.TEXT_NODE) { return serializeMarkdownText(node.textContent ?? '') } function escapeMarkdownTableCell(value: string): string { return value.replace(/\|/g, '\\|').replace(/\r?\n/g, ' ') }
-
[Minor] Pending mobile fullscreen work survives component unmount - cleanup releases only the currently recorded state but leaves
openReftrue. IfrequestFullscreen()resolves after the message/session unmounts, the continuation starts the orientation lock and treats the removed viewer as still open, leaving no later cleanup to exit fullscreen. Evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:1182.
Suggested fix:useEffect(() => () => { const wasMobile = mobileViewerRef.current const enteredFullscreen = enteredFullscreenRef.current openRef.current = false mobileViewerRef.current = false enteredFullscreenRef.current = false if (wasMobile) leaveMobileTableViewer(enteredFullscreen) }, [])
Questions
- None.
Summary
Review mode: follow-up after new commits
The new commits address the prior file-link round-trip findings. The full current diff still has two Minor correctness issues: code-span content can change during Markdown copy, and a deferred mobile fullscreen request can outlive its table component.
Testing
- Not run locally (automation/security policy; PR code was not executed).
- Current GitHub
testworkflow passed, including typecheck, table Playwright suites, and repository tests. - Current
integrationworkflow failed in unchangedcli/src/runner/runner.integration.test.tscoverage (4 failed, 9 passed, 1 skipped); no CLI/shared source is changed, so it is not attributed to this diff. - Add regressions for a table code span containing
C:\tmpand for unmounting whilerequestFullscreen()is deferred.
HAPI Bot
| } | ||
|
|
||
| function escapeMarkdownTableCell(value: string): string { | ||
| return value.replace(/\\/g, '\\\\').replace(/\|/g, '\\|').replace(/\r?\n/g, ' ') |
There was a problem hiding this comment.
[MINOR] Do not escape code-span backslashes as ordinary cell text.
serializeInlineMarkdown constructs the code span first, then this final pass doubles every backslash. A rendered <code>C:\tmp</code> becomes `C:\\tmp`; Markdown code spans treat backslashes literally, so Copy Markdown changes the cell content.
Suggested fix:
function serializeMarkdownText(value: string): string {
return value.replace(/\s+/g, ' ').replace(/\\/g, '\\\\')
}
if (node.nodeType === Node.TEXT_NODE) {
return serializeMarkdownText(node.textContent ?? '')
}
function escapeMarkdownTableCell(value: string): string {
return value.replace(/\|/g, '\\|').replace(/\r?\n/g, ' ')
}Add a regression with an inline code cell containing C:\tmp.
| return () => document.removeEventListener('fullscreenchange', handleFullscreenChange) | ||
| }, []) | ||
|
|
||
| useEffect(() => () => { |
There was a problem hiding this comment.
[MINOR] Invalidate pending fullscreen work during unmount.
If this component unmounts while requestFullscreen() is pending, cleanup unlocks only the current state but leaves openRef true. After the await, enterMobileTableViewer starts the orientation lock and the continuation treats the removed viewer as still open, so no later cleanup exits fullscreen.
Suggested fix:
useEffect(() => () => {
const wasMobile = mobileViewerRef.current
const enteredFullscreen = enteredFullscreenRef.current
openRef.current = false
mobileViewerRef.current = false
enteredFullscreenRef.current = false
if (wasMobile) leaveMobileTableViewer(enteredFullscreen)
}, [])Add a test with deferred requestFullscreen(): unmount, resolve it, then assert exit/unlock cleanup.
There was a problem hiding this comment.
Findings
- [Minor] Copy Markdown does not escape literal Markdown metacharacters in plain text —
serializeMarkdownTextescapes backslashes only. For example, an authored\*literal\*renders as the DOM text*literal*, then Copy Markdown emits*literal*, which reparses as emphasis instead of literal text. The same round-trip corruption affects literal brackets, backticks, tildes, and angle brackets. Evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:225.
Suggested fix:function serializeMarkdownText(value: string): string { return value .replace(/\s+/g, ' ') .replace(/\\/g, '\\\\') .replace(/([`*_\[\]<>~])/g, '\\$1') }
Questions
- None.
Summary
Review mode: follow-up after new commits
The latest commits address both prior HAPI Bot findings. The full current diff still has one Minor Markdown round-trip issue on the new Copy Markdown path.
Testing
- Not run locally (automation/security policy; untrusted PR code was not executed).
- GitHub
testworkflow passed, including typecheck, existing browser coverage, the new table Playwright suites, and repository tests. - GitHub
integrationworkflow failed in unchangedcli/src/runner/runner.integration.test.tscoverage (4 failed, 9 passed, 1 skipped); no CLI/shared source changed, so it is not attributed to this diff. - Add a render → copy → render regression covering literal
*,_,[ ], backticks,~~, and<...>in plain table text.
HAPI Bot
| } | ||
|
|
||
| function serializeMarkdownText(value: string): string { | ||
| return value.replace(/\s+/g, ' ').replace(/\\/g, '\\\\') |
There was a problem hiding this comment.
[MINOR] Escape literal Markdown syntax in plain text nodes.
This helper escapes backslashes only. An authored \*literal\* renders as DOM text *literal*; Copy Markdown emits that unchanged, so the copied table reparses it as emphasis. Literal brackets, backticks, tildes, and angle brackets have the same semantic round-trip problem.
Suggested fix:
function serializeMarkdownText(value: string): string {
return value
.replace(/\s+/g, ' ')
.replace(/\\/g, '\\\\')
.replace(/([`*_\[\]<>~])/g, '\\$1')
}Add a render → copy → render regression for escaped literal Markdown punctuation in a table cell.
There was a problem hiding this comment.
Findings
- [Minor] Auto-linked file paths copy HAPI-internal destinations — the serializer prefers
data-hapi-markdown-href, but bare path autolinks receive an internalhapi-file:/hapi-file-candidate:fallback fromFilePathAnchor. Copy Markdown therefore emits a non-portable link such as[web/src/router.tsx](hapi-file:web%2Fsrc%2Frouter.tsx)instead of the original bare path. Evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:241(related contextweb/src/components/assistant-ui/markdown-text.tsx:508).
Suggested fix:const label = children() if (href?.startsWith('hapi-file:') || href?.startsWith('hapi-file-candidate:')) { return label } return href ? `[${label}](${href})` : label
- [Minor] Image alt text is not escaped before rebuilding Markdown — an authored alt such as
a\]brenders asa]b, then copies as![a]b](...), which no longer reparses as the same image. Evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:260.
Suggested fix:const alt = serializeMarkdownText(element.getAttribute('alt') ?? '') return src ? `` : alt
Questions
- None.
Summary
Review mode: follow-up after new commits
The latest commit fixes the prior plain-text metacharacter finding. The full current diff still has two Minor Copy Markdown round-trip defects.
Testing
- Not run locally (automation/security policy; untrusted PR code was not executed).
- GitHub
testcheck is still in progress at review time. - GitHub
integrationfailed inbun run test:cli:integration; this PR changes web/workflow files only, so the failure is not attributed to the diff. - Add render → copy → render coverage for bare/inline-code file-path autolinks and image alt text containing
],\, and|.
HAPI Bot
| const children = () => Array.from(element.childNodes, serializeInlineMarkdown).join('') | ||
| switch (element.tagName.toLowerCase()) { | ||
| case 'a': { | ||
| const href = element.dataset.hapiMarkdownHref ?? element.getAttribute('href') |
There was a problem hiding this comment.
[MINOR] Keep internal file-link schemes out of copied Markdown.
Bare file paths are auto-linked through hapi-file: / hapi-file-candidate:. FilePathAnchor stores that internal fallback in data-hapi-markdown-href, so this branch emits a non-portable destination such as [web/src/router.tsx](hapi-file:web%2Fsrc%2Frouter.tsx) rather than restoring the original bare path. Explicit authored file links already carry their original destination and should remain links.
Suggested fix:
const label = children()
if (href?.startsWith('hapi-file:') || href?.startsWith('hapi-file-candidate:')) {
return label
}
return href ? `[${label}](${href})` : labelAdd a chat-context regression for both a plain file-path autolink and an inline-code file-path autolink.
| return ' ' | ||
| case 'img': { | ||
| const src = element.getAttribute('src') | ||
| const alt = element.getAttribute('alt') ?? '' |
There was a problem hiding this comment.
[MINOR] Escape image alt text before rebuilding Markdown.
alt comes from the rendered DOM and is interpolated raw. For example, authored a\]b becomes DOM alt text a]b, then Copy Markdown emits ![a]b](...), which reparses with a broken image label.
Suggested fix:
const alt = serializeMarkdownText(element.getAttribute('alt') ?? '')
return src ? `` : altAdd a render → copy → render regression with ], \, and | in image alt text.
There was a problem hiding this comment.
Findings
- [Minor] Per-cell header colors are overwritten in PNG exports —
copyTableImageStylesfirst preserves each header cell’s computed background, butapplyTableImageHeaderBackgroundthen assigns the samethead/theme color to every cloned header cell. Tables with independently styled headers therefore export differently from the live table. Evidenceweb/src/components/assistant-ui/MarkdownTable.tsx:514.
Suggested fix:cloneHead.querySelectorAll<HTMLElement>(':is(th, td)').forEach((cell) => { if (!isTransparentColor(cell.style.getPropertyValue('background-color'))) return cell.style.setProperty('background-color', background, 'important') })
Questions
- None.
Summary
Review mode: follow-up after new commits
The two prior Copy Markdown findings are resolved. The full current diff has one remaining Minor PNG fidelity defect.
Testing
- Not run locally (automation/security policy; untrusted PR code was not executed).
- GitHub
testpassed. - GitHub
integrationfailed; this PR changes web/workflow files only, so the failure is not attributed to the diff. - Add a PNG-clone regression with two header cells using different background colors.
HAPI Bot
| cloneHead.style.setProperty('background-color', background, 'important') | ||
| cloneHead.querySelectorAll(':is(th, td)').forEach((cell) => { | ||
| if (!(cell instanceof HTMLElement)) return | ||
| cell.style.setProperty('background-color', background, 'important') |
There was a problem hiding this comment.
[MINOR] Preserve per-cell header colors in PNG exports.
copyTableImageStyles has already copied each header cell’s computed background, but this loop overwrites every cloned cell with one thead/theme color. A table with differently colored header cells therefore exports with a uniform header.
Suggested fix:
cloneHead.querySelectorAll<HTMLElement>(':is(th, td)').forEach((cell) => {
if (!isTransparentColor(cell.style.getPropertyValue('background-color'))) return
cell.style.setProperty('background-color', background, 'important')
})Add a regression with two header cells using distinct background colors.
There was a problem hiding this comment.
Findings
- None.
Summary
Review mode: follow-up after new commits
No new high-confidence issues found in the full current diff. The prior per-cell PNG header-color finding is resolved by preserving non-transparent cloned cell backgrounds and adding a two-color regression test.
Residual risk: image export remains browser-dependent (canvas, clipboard, fullscreen/orientation APIs), but the PR includes focused Vitest and Chromium coverage for those paths.
Testing
- Not run locally (automation/security policy; untrusted PR code was not executed).
- GitHub
testpassed, including the new required table Playwright suites. - GitHub
integrationfailed in CLI runner integration tests; this PR changes web/workflow files only, so the failure is not attributed to the diff.
HAPI Bot
1 participant
Problem / Motivation
Markdown tables need a compact way to inspect wide or long content without disrupting the table header. The initial viewer also needed reliable image export fidelity across desktop, mobile, and shared previews.
Summary
Validation
bun typecheck— passed.pwsh -NoProfile -File .\scripts\Invoke-HapiTaskPlaywright.ps1 -Name table-actions-fullscreen -Suite Root terminal-wrap-fidelity.spec.ts— 2/2 passed.bun run test -- src/components/assistant-ui/MarkdownTable.test.tsx src/components/AssistantChat/ShareTurnDialog.test.tsx) — 47/47 passed.bun run test -- src/lib/remark-file-path-links.test.ts src/components/assistant-ui/markdown-a.test.tsx) — 119/119 passed.pwsh -NoProfile -File .\scripts\Invoke-HapiTaskPlaywright.ps1 -Name table-actions-fullscreen -Suite Web) — 22 passed, 16 skipped by the existing live-session configuration.bun run build— passed.Related Issues
None
AI Disclosure
Implemented and validated with OpenAI Codex (GPT-5.6) assistance.