fix: preserve raw bytes when downloading topic messages#4130
Conversation
…F-8 text Topic message download reused the UTF-8-decoded preview string for the Download button. For non-UTF-8 binary payloads this silently corrupted the data (invalid byte sequences replaced with U+FFFD), so the downloaded file no longer matched the original message bytes. Download now uses the raw decoded Uint8Array directly; the preview string is still used for JSON/text rendering and clipboard copy. Fixes ydb-platform#4129
|
@greptile review |
astandrik
left a comment
There was a problem hiding this comment.
The implementation looks correct, but the binary download path needs focused regression coverage before merge.
Adds a component test asserting the Download button receives the raw Uint8Array bytes for non-UTF-8 payloads (and unchanged bytes for plain UTF-8 text), so reverting to the lossy decodedMessage string would fail the suite. Also gives the Download button an explicit aria-label (matching how ClipboardButton already labels itself) so the test - and screen readers - can target it without relying on the hover/focus-only tooltip, and polyfills the Encoding API in jsdom test setup, which TopicMessage needs at module load time. Addresses review feedback on ydb-platform#4130.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 486cda798f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| e.stopPropagation(); | ||
| createAndDownloadFile( | ||
| decodedMessage, | ||
| rawBytes ?? decodedMessage, |
There was a problem hiding this comment.
Narrow raw byte type before passing it as BlobPart
With the repo's TypeScript 5.8 DOM typings, the rawBytes variable is annotated as plain Uint8Array, which widens to Uint8Array<ArrayBufferLike>, but createAndDownloadFile now requires BlobPart and DOM BlobPart only accepts an ArrayBufferView<ArrayBuffer>. This makes npm run typecheck reject the new download call (Uint8Array<ArrayBufferLike> is not assignable to BlobPart) even though the runtime path is valid; keep the byte array typed as Uint8Array<ArrayBuffer> or otherwise narrow/convert it before passing it here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a false positive — the code type-checks clean. npx tsc --noEmit over the whole project reports 0 errors on TS 5.8.3 with lib: ["dom","dom.iterable","esnext"] (the exact setup flagged here). rawBytes ?? decodedMessage is Uint8Array<ArrayBufferLike> | string, and in this DOM lib ArrayBufferView defaults to ArrayBufferView<ArrayBufferLike>, so a Uint8Array<ArrayBufferLike> already satisfies BufferSource ⊂ BlobPart. No narrowing needed.
Summary
Uint8Arraydirectly; preview/clipboard still use the best-effort UTF-8 stringFixes #4129
Test plan
tsc --noEmitpasseseslinton changed files — no new warnings/errorsGreptile Summary
This PR fixes silent data corruption when downloading binary (non-UTF-8) topic messages. Previously, the download reused the lossy UTF-8-decoded preview string, replacing invalid byte sequences with U+FFFD; now the raw
Uint8Arrayfromatobis passed directly toBlob, preserving every original byte.TopicMessage.tsx:rawBytes(aUint8Array) is captured in theuseMemoalongsidedecodedMessage; the download button usesrawBytes ?? decodedMessage, while preview and clipboard continue to use the best-effort UTF-8 string. All fallback paths (schematized messages, non-string values, failedatob) safely fall back todecodedMessage.downloadFile.ts: Thedataparameter ofcreateAndDownloadFileis widened fromstringtoBlobPart, the minimal typing change needed to acceptUint8Array.Confidence Score: 5/5
Safe to merge — the change is narrowly scoped to the download path and preserves all existing preview/clipboard behaviour unchanged.
The fix is correct and complete: rawBytes is populated from the same atob/Uint8Array decode that already feeds the UTF-8 preview, every fallback path (schematized messages, non-string payloads, failed atob) continues to work via the ?? decodedMessage guard, and the only typing change in downloadFile.ts is a minimal widening of BlobPart that does not alter any other callers.
No files require special attention.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant UI as TopicMessage Component participant Memo as useMemo (decode logic) participant DL as createAndDownloadFile participant Blob as Browser Blob API UI->>Memo: message (base64 string), isSchematized alt non-schematized base64 string Memo->>Memo: atob(message) → binary string Memo->>Memo: Uint8Array from binary (rawBytes) Memo->>Memo: utf8Decoder.decode(bytes) → decodedMessage (lossy) else schematized / non-string Memo->>Memo: "rawBytes = undefined" Memo->>Memo: "decodedMessage = string/JSON" end Memo-->>UI: "{preparedMessage, decodedMessage, isJson, rawBytes}" note over UI: Preview & Clipboard use decodedMessage (UTF-8 string) note over UI: Download uses rawBytes ?? decodedMessage UI->>DL: rawBytes (Uint8Array) OR decodedMessage (string) DL->>Blob: new Blob([data]) Blob-->>DL: blob URL DL->>DL: trigger anchor click → download DL->>Blob: revokeObjectURL%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant UI as TopicMessage Component participant Memo as useMemo (decode logic) participant DL as createAndDownloadFile participant Blob as Browser Blob API UI->>Memo: message (base64 string), isSchematized alt non-schematized base64 string Memo->>Memo: atob(message) → binary string Memo->>Memo: Uint8Array from binary (rawBytes) Memo->>Memo: utf8Decoder.decode(bytes) → decodedMessage (lossy) else schematized / non-string Memo->>Memo: "rawBytes = undefined" Memo->>Memo: "decodedMessage = string/JSON" end Memo-->>UI: "{preparedMessage, decodedMessage, isJson, rawBytes}" note over UI: Preview & Clipboard use decodedMessage (UTF-8 string) note over UI: Download uses rawBytes ?? decodedMessage UI->>DL: rawBytes (Uint8Array) OR decodedMessage (string) DL->>Blob: new Blob([data]) Blob-->>DL: blob URL DL->>DL: trigger anchor click → download DL->>Blob: revokeObjectURLReviews (2): Last reviewed commit: "Merge branch 'main' into fix/topic-messa..." | Re-trigger Greptile