Compact document-viewer redesign: dedup chrome, honest cards, tighter PDF surface#509
Conversation
… PDF surface Addresses a 20-point design review of the document viewer: - Nav chrome: drop the redundant Answer|Documents toggle and the ‹Documents breadcrumb; header is now back-arrow + document <h1> title + scope + actions. Mobile back uses ArrowLeft (was a Menu hamburger). - Overview cards: remove misleading chevrons; replace the fake wireframe page thumbnails with real "Page N" jump chips; de-dup the overview into a single header teaser; derive the type eyebrow instead of hardcoding "Clinical guideline"; fix uneven card heights. - Actions: rebalance to a clean grid-cols-2 sm:grid-cols-4 row; relabel "Scope" to "Add to scope". - PDF surface: collapse the toolbar to one row; stop the page indicator stretching full-width; replace the always-on helper paragraph and canvas/browser jargon with a "Sharper zoom"/"Standard view" toggle + tooltip; reduce reserved empty min-height. - Evidence/tables/layout: de-emphasise the empty pinned-evidence state; let the extracted table lead with the raw image behind a "Show source image" disclosure; let the summary rail flow with the page (drop the nested sticky/overflow scroll), which also clears the composer overlap. Preserves test invariants (h1 title, pinned-source-evidence/pdf-toolbar testids, viewer-nav anchors, fullscreen/Answer labels, static mobile toolbar). tsc/eslint/prettier clean; ui-smoke document-viewer suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughDocumentViewer updates image/table presentation, document overview content and actions, PDF controls, header labels, and responsive layout behavior. ChangesDocument Viewer
Estimated code review effort: 4 (Complex) | ~45 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/DocumentViewer.tsx`:
- Around line 519-555: The structured-table rendering must pass imageBlock as
lowConfidenceFallback to AccessibleTable so low-confidence tables keep the
source image visible. Also update the hasStructuredTable condition to require
usable table data—rows or markdown, not columns alone—or otherwise route to the
existing image branch, preventing an empty caption and collapsed source image.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 00b598ef-5ead-439c-b174-06d97766f0c9
📒 Files selected for processing (1)
src/components/DocumentViewer.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52e4707c50
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Addresses CodeRabbit/Codex review on #509: the table-leads branch collapsed the source image behind a disclosure whenever any table metadata was present, but AccessibleTable renders nothing for columns-only input or unparseable markdown — leaving an empty caption above a hidden image. - Add exported hasRenderableAccessibleTable() mirroring AccessibleTable's own parse+normalize so the layout decision can't diverge from what it renders; columns-only / unparseable-markdown tables now route to the image-first branch. - Pass the source image as lowConfidenceFallback so a table that normalizes but can't be confidently reconstructed shows the image inline instead of a misleading grid. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Finding addressed in a82f7c3 (hasRenderableAccessibleTable() drives the table-leads branch; source image kept via lowConfidenceFallback). Both review threads resolved and CI is green.
Summary
Implements a 20-point design review of the document viewer (
src/components/DocumentViewer.tsx). All changes are isolated to that one file.Navigation chrome
back-arrow + document title (h1) + scope + actions.ArrowLefton both breakpoints (was aMenuhamburger that sat under the shell's real hamburger).Overview cards
?page=N#pdf-preview-section).documentTypeEyebrow()instead of hardcoding "Clinical guideline".lg:items-stretch→lg:items-start).Actions
grid-cols-2 sm:grid-cols-4(no orphaned button).PDF surface
Evidence, tables, layout
sticky/max-h/overflow-y-auto), which also clears the composer overlap.Verification
tsc --noEmit✅,eslint✅,prettier --check✅tests/ui-smoke.spec.tsdocument-viewer tests pass (incl.@critical).pinned-source-evidence/pdf-toolbartestids, viewer-nav anchors, fullscreen/"Answer from this" labels, static-on-mobile toolbar.🤖 Generated with Claude Code