fix(frontend): show clear preview unavailable message for office documents#958
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review
🟡 Acceptable — Clean implementation with one minor testing gap against the acceptance criteria.
What's Good
DRY improvement: Replacing three identical inline fallback <div> blocks with a single UnpreviewableFallback component is a worthwhile cleanup independent of the feature itself. That component handles the full lifecycle: it reads the path's extension, maps it to a label via OFFICE_DOCUMENT_LABELS, and emits the correct data-testid so tests can distinguish Office docs from generic binaries.
Extension handling: Using lastIndexOf(".") with .toLowerCase() correctly handles demo.PPTX and paths like archive.tar.pptx. The OFFICE_DOCUMENT_LABELS map serving dual duty as both allow-list and label source is commented clearly — adding a new format is a one-line change.
Translation coverage: All 15 existing locales are updated with the new FILES$UNSUPPORTED_DOCUMENT key. The {type} placeholder correctly injects the format-specific name (PowerPoint, Word, Excel) rather than the raw extension.
Test approach: Mocking the service layer rather than the hook itself is the right call — it lets the real classification pipeline (classifyKind → isLikelyBinary) run, so the test actually validates the end-to-end path rather than just asserting a mock was called.
Testing Gap
The acceptance criteria states: "both Rich and Plain modes" should show the clear message. The new test file only exercises viewMode="rich". The code path for plain mode (line 121 in file-content-viewer.tsx) also routes through UnpreviewableFallback, so it should work correctly — but an explicit assertion would make the coverage match the acceptance criteria.
Additionally, there is no regression test asserting that a non-Office binary (e.g., app.bin) still renders the generic file-content-viewer-binary-fallback element. The data-testid was changed from a static attribute to a conditional expression — a regression test would prevent someone from breaking this distinction later.
Risk Assessment
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Frontend-only change. No API surface touched. No breaking changes to existing behavior (generic binary fallback preserved). Thedata-testidvalues are preserved for existing consumers. The 25 pre-existing tests remain green per the PR author.
VERDICT:
✅ Worth merging — Core logic is sound. The test coverage gap is minor given that the existing test validates the primary new code path end-to-end; the missing plain-mode and binary-regression cases can be addressed in a follow-up if the acceptance criteria is a hard requirement.
KEY INSIGHT: The real value of this PR isn't just the Office-doc message — it's the UnpreviewableFallback refactor that eliminates the three-way duplication and makes the binary fallback policy a single, easily-audited location.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better by adding a
.agents/skills/custom-codereview-guide.mdfile to your branch.Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review
The previous review from all-hands-bot covered this PR comprehensively. This review adds a few fresh observations on points not yet discussed, and confirms the implementation is ready.
✅ What Works Well
it.each covers both view modes: The test uses it.each(["rich", "plain"] as const), so both plain and rich modes are exercised in a single parametrized spec. This satisfies the acceptance criteria without extra boilerplate.
Consistent fallback policy: All three UnpreviewableFallback call sites — plain-mode binary (line 118), rich-mode binary (line 162), and the final "unknown payload" fallback (line 227) — now route through the same helper. This means even an empty or corrupt .pptx payload shows a format-specific message rather than a blank pane. The behavior change is subtle but correct and aligns with the stated acceptance criterion: "The preview pane is never blank for these files."
Extension map is self-documenting: OFFICE_DOCUMENT_LABELS doubles as the allow-list and the label source. Adding a new format is a one-liner and the intent is clear from the comment.
🔍 Minor Observations
downloadFileMock is set up but appears unused in the current test: The mock is defined and reset in beforeEach, but the test exercises the window.fetch path only — file content flows through fetch, not downloadFile. The setup is harmless and could serve as scaffolding for future tests in this file, but it adds a small amount of noise.
OpenDocument and macro-enabled formats not covered: .odt, .odp, .ods (LibreOffice) and macro-enabled variants (.xlsm, .pptm, .docm) fall through to the generic binary message. This is consistent with the stated scope ("common Office/document extensions"), but the gap is worth a follow-up issue if usage of these formats is expected.
Risk Assessment
🟢 LOW — Frontend-only change. No API surface, data layer, or breaking behavioral changes. Existing data-testid="file-content-viewer-binary-fallback" consumers are unaffected for non-Office binaries; Office documents get a dedicated file-content-viewer-unsupported-document testid.
VERDICT: ✅ LGTM — Clean, well-tested implementation that addresses a real UX gap without over-engineering. Ready to merge.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review
The previous reviews from
all-hands-bothave already covered this PR comprehensively. This review confirms the current state of the branch is clean and adds a few targeted observations on aspects not yet discussed.
✅ Current State Is Clean
The downloadFileMock handle noted in the inline comment thread was already removed in the current HEAD (a47b8ec). The stub for agent-server-runtime-service is correctly retained as an anonymous vi.fn() — the hook statically imports the cloud runtime service at module load time, so removing the stub entirely would pull real cloud/proxy machinery into a local-path unit test. This is the right balance between isolation and realism.
🔍 Fresh Observations
getActiveBackendMock is load-bearing, not dead weight: Unlike the removed downloadFileMock handle, getActiveBackendMock actively controls which code path useWorkspaceFileContent exercises. By returning { kind: "local" }, the mock forces the fetch branch (not the cloud downloadFile branch), which is why fetchMock is the thing you assert against. This separation is intentional and correct.
The "unknown / empty payload" fallback is intentionally inclusive: The third UnpreviewableFallback call site (the final catch-all in FileContentViewer) means a .pptx that fails to load — whether due to a network error or an empty body — still shows "Preview isn't available for PowerPoint files." rather than a blank pane. This could be read as slightly misleading if the cause is transient (a retry might succeed), but it correctly satisfies the acceptance criterion: "The preview pane is never blank for these files." If a future iteration adds error-state UI with a retry affordance, this fallback is the right place to differentiate.
Upper-case extensions are handled silently: getExtension calls .toLowerCase(), so Demo.PPTX resolves correctly to "pptx" and matches the label map without any special-casing.
Risk Assessment
🟢 LOW — Frontend-only. No API or data-layer changes. Existing data-testid="file-content-viewer-binary-fallback" consumers are unaffected for non-Office binaries. The 25 pre-existing tests confirm no regressions.
VERDICT: ✅ LGTM — The implementation is correct, well-tested, and the branch is in a clean state. Ready to merge.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review
Previous reviews from
all-hands-botandopenhands-ai[bot]have covered this PR thoroughly. This review confirms the current state at HEAD (a47b8ec) and adds a few targeted observations on aspects not previously discussed.
✅ Implementation Is Sound
The fix is minimal, focused, and correct. Three identical fallback <div> blocks are replaced by a single UnpreviewableFallback component that routes on file extension — a strict improvement in DRY-ness and user-facing clarity.
The classification pipeline (classifyKind → isLikelyBinary) is intentionally untouched. Keeping the Office-specific extension check inside UnpreviewableFallback rather than adding an "office" kind to classifyKind is pragmatic: it avoids threading a new classification variant through the entire rendering pipeline just to change a message. This is the right tradeoff for the stated scope.
🔍 Fresh Observations
Hidden-file edge case: getExtension(".pptx") returns "pptx" because lastIndexOf(".") returns 0, not -1. A file literally named .pptx would be labeled as "PowerPoint". This is an unlikely pattern in practice and the behavior is defensible — no action required.
Accessibility: UnpreviewableFallback renders a plain <div> with no ARIA role or aria-label. Screen readers will read the text content, which is meaningful in both message variants. No accessibility regression is introduced.
OpenDocument formats omitted by design: .odp, .odt, and .ods (LibreOffice equivalents) are not in OFFICE_DOCUMENT_LABELS and fall through to the generic binary fallback. The PR description explicitly scopes to MS Office formats, so this is correct. Worth tracking as a follow-up if OpenDocument files appear in user-reported issues.
Translation completeness: All 15 locales present in FILES$BINARY_FALLBACK are also present in FILES$UNSUPPORTED_DOCUMENT. The {{type}} placeholder is consistent with existing i18n patterns in the file.
Risk Assessment
Low risk. The change is purely additive on a new code branch: files whose extensions appear in OFFICE_DOCUMENT_LABELS receive a new, clearer message; every other code path is unchanged. The it.each(["rich", "plain"]) parametrized test covers both view modes that exercise the new component.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
@all-hands-bot If the PR looks good, please go ahead and approve it. |
|
🔍 Review in progress… We are performing the review through OpenHands Cloud Automation. You can log in and view the conversation here. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review
🟢 Good Taste — Clean, minimal fix that is correct and ready to merge.
Context
Previous reviews by all-hands-bot and openhands-ai[bot] have covered this PR thoroughly. This review confirms the current HEAD (a47b8ec) is clean and adds two targeted observations not yet covered.
✅ What's Solid at HEAD
-
Both prior concerns resolved: The first review noted (1) only
viewMode="rich"was tested and (2) thedownloadFileMockhandle was dead weight. Both are fixed ina47b8ec:it.each(["rich", "plain"] as const)exercises both code paths, and the nameddownloadFileMockhandle is gone — replaced by an inlinevi.fn()inside thevi.mock()factory with an explanatory comment. -
UnpreviewableFallbackdesign is correct: Routing all three previously-duplicated fallback blocks through a single helper that branches onOFFICE_DOCUMENT_LABELS[ext]is strictly better — a new format is a one-line addition, and thedata-testiddistinction allows test assertions on both the Office and generic paths. -
Real classification pipeline exercised: Mocking the service layer (not the hook) and returning real ZIP header bytes (
[0x50, 0x4b, 0x03, 0x04, 0x00]) letsisLikelyBinary()run end-to-end and flip the kind to"binary". This test would catch a regression in classification, not just a UI change.
🔍 Fresh Observations
Testing gap — non-Office binary regression: The new file-content-viewer.test.tsx asserts that .pptx shows data-testid="file-content-viewer-unsupported-document" but has no test asserting that a non-Office binary (e.g., app.bin) still renders data-testid="file-content-viewer-binary-fallback". The data-testid attribute changed from a static value to a conditional expression — a conditional that has two outputs. Only one output is exercised in the new test file. If the existing suite in files-tab.test.tsx already covers this case, the gap is covered; otherwise a one-liner using it.each on a non-Office path would make coverage airtight.
Only .pptx covered — acceptable by design: The parametrised spec runs .pptx only. Since all Office extensions share the same code path through OFFICE_DOCUMENT_LABELS, the omission is low risk, but a quick it.each over ["demo.docx", "data.xlsx"] would directly satisfy the acceptance criteria's wording (".pptx / .docx / .xlsx"). Worth a follow-up if the test suite is ever expanded.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW — Frontend-only change. No API surface touched, no data-layer change (classifyKindis unchanged), no behavioral change for non-Office binaries or images/PDFs. Three identical<div>blocks collapsed into one helper that adds a conditional. The change is trivially reversible.
VERDICT:
✅ Worth merging — Implementation is correct, tests cover both view modes, translations are complete across all 15 locales. The non-Office-binary test gap is low severity and can be tracked as a follow-up if desired.
KEY INSIGHT: The dual-use of OFFICE_DOCUMENT_LABELS as both allow-list and label source is the cleanest possible API for this feature — adding a format requires zero coordination between the UI text and the detection logic.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
Was this review helpful? React with 👍 or 👎 to give feedback.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Already covered:
Left as-is: all Office extensions go through the same data-driven |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Code Review
🟢 Ready to merge — Minimal, correct, and well-tested fix.
Prior reviews by all-hands-bot covered the main concerns: three duplicated fallback <div> blocks replaced by UnpreviewableFallback, both view modes exercised by it.each, and the dead downloadFileMock handle removed. Current HEAD (a47b8ec) is clean.
What's Solid
OFFICE_DOCUMENT_LABELS dual purpose: The map is elegant — it serves simultaneously as the allow-list (a present entry means "this is an Office doc") and the label source (the value feeds the i18n placeholder). Adding a new format requires only one entry rather than separate extension check + label constant.
UnpreviewableFallback handles all three call sites consistently: Plain-mode binary branch, rich-mode kind === "binary" branch, and the final unknown-kind fallback all route through the same helper. The final fallback case is noteworthy: a .pptx file that arrives with an empty payload (no NUL bytes to trigger isLikelyBinary) will still show the format-specific message rather than the generic string, which is the correct user-facing behavior.
Backward-compatible data-testid: Non-Office binaries still emit "file-content-viewer-binary-fallback", keeping the existing files-tab.test.tsx assertions green without changes.
i18n coverage: All 15 existing locales have translations for the new FILES$UNSUPPORTED_DOCUMENT key and the {{type}} interpolation follows the react-i18next convention.
One Minor Observation (not a blocker)
The new test asserts the data-testid is present but does not verify the rendered text (e.g. "Preview isn't available for PowerPoint files."). Since the i18n key is TypeScript-typed via the I18nKey enum the compiler would catch a wrong key name, and react-i18next falls back to showing the key string on a lookup miss, so no silent failure is possible. An additional findByText(/Preview isn't available for PowerPoint/i) assertion would give stricter coverage of the {{type}} interpolation, but the current approach is acceptable.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
✅ Mock-LLM Docker E2E Test Results12/12 passed Commit:
Posted by the Mock-LLM E2E workflow · results are deterministic (scripted LLM responses) |
📸 Snapshot Test Report✅ All snapshots match the main branch baselines.
✅ Unchanged snapshots (73)
Generated by the Snapshot Tests workflow. This comment was created by an AI agent (OpenHands) on behalf of the repo maintainers. |
3 participants
Why
Problem
When a user opens a
.pptxfile in a conversation's Files tab, the preview pane shows only the generic "Binary file – preview not available" message (and in some flows reads as an empty pane). Nothing indicates that the file is a PowerPoint document or why it can't be shown.Steps to reproduce
npm run dev).demo.pptx.demo.pptx.Current behavior
The preview pane shows the generic "Binary file – preview not available" string for
.pptx(and other Office documents) — no useful information.Expected behavior
Either render the document or show a clear message that the format isn't supported for preview, e.g. "Preview isn't available for PowerPoint files."
Root cause
The preview pipeline only recognizes images and PDFs.
.pptxfalls throughclassifyKind()to"text"; after the bytes are fetched,isLikelyBinary()finds NUL bytes in the ZIP header and flips it tokind: "binary"; andFileContentViewerrenders a single generic message for every binary kind. There is no Office-document recognition or format-specific messaging.Proposed solution
Recognize common Office/document extensions (
.pptx, .ppt, .docx, .doc, .xlsx, .xls) inFileContentViewerand render a clear, format-named message via a newFILES$UNSUPPORTED_DOCUMENTi18n key. Keep the existing generic message for all other binaries. Rendering the actual slide/document contents (which would need a heavy client-side dependency) is out of scope.Acceptance criteria
.pptx/.docx/.xlsxshows a clear "Preview isn't available for {PowerPoint|Word|Excel} files." message in both Rich and Plain modes.Summary
Office documents (
.pptx,.docx,.xlsx, …) opened in the Files tab previously showed only the generic "Binary file – preview not available" message (and could read as a blank pane). They now show a clear, format-aware message — e.g. "Preview isn't available for PowerPoint files." — so the limitation is explicit and the pane is never blank.Root cause
The preview pipeline only classifies images and PDFs specially:
classifyKind()(use-workspace-file-content.ts) sends.pptxto"text".isLikelyBinary()finds NUL bytes in the ZIP header →kind: "binary".FileContentViewerrenders a single generic message for every"binary"file.So Office documents are treated as anonymous binaries with no format-specific messaging.
What changed
file-content-viewer.tsx— added anOFFICE_DOCUMENT_LABELSmap (which doubles as the allow-list) and a smallUnpreviewableFallbackhelper that shows a clear, format-named message for Office docs and the existing generic message for any other binary. The three previously-duplicated fallback blocks (plain mode, rich-mode binary branch, and final fallback) now route through this helper, so the message is consistent across Rich/Plain and the pane is never blank.i18n/translation.json— newFILES$UNSUPPORTED_DOCUMENTkey (all 15 locales,{{type}}placeholder).declaration.tsis regenerated from this bymake-i18n(gitignored).__tests__/components/features/files-tab/file-content-viewer.test.tsxdrives the realuseWorkspaceFileContenthook with the underlyingfetchservice mocked to return.pptxbytes, and asserts the clear message renders.The data layer (
classifyKind,WorkspaceFileKind) is unchanged — real Office docs already resolve to"binary".Behavior
demo.pptxnotes.docxdata.xlsxapp.bin)Testing
npm run typecheck— cleanfiles-tab.test.tsxanduse-workspace-file-content.test.tsxremain greenOut of scope
staticUrlis available if we want to add one later).Issue Number
Resolves #953
How to Test
agent-server-guirepository and start the development server:From the home page, create a new conversation using a workspace that already contains a file named
demo.pptx.Open the conversation, navigate to the Files tab, and select
demo.pptxto preview it.Video/Screenshots
Type
🐳 Docker images for this PR
• GHCR package: https://github.com/OpenHands/agent-canvas/pkgs/container/agent-canvas
ghcr.io/openhands/agent-canvasghcr.io/openhands/agent-server:1.24.0-pythonopenhands-automation==1.0.0a5d91e9a5559cbf845c63ed3fe7f9c69aef0d6c413Pull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-d91e9a5Run
All tags pushed for this build
About Multi-Architecture Support
sha-d91e9a5) is a multi-arch manifest supporting both amd64 and arm64sha-d91e9a5-amd64) are also available if needed