studio: polish styles for file viewer - #95
Merged
Conversation
The title row and the document toolbar now share a height and an inset, with no rule between them, so they read as one band rather than a header with a strip bolted underneath. The toolbar's controls hug the row above and leave their slack as the gap before the document. A hairline closes the band instead: every chrome row carries it, and a row followed by a toolbar drops it, so the stroke lands under whichever row is last whether or not the format has a toolbar. Leading controls that paint their own box or are bare text carry the glyph inset themselves, keeping one optical column down the left edge. The zoom readout also drops its caret, which crowded the percentage at toolbar size.
📝 WalkthroughWalkthroughChangesViewer chrome alignment
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/studio/src/client/styles/globals.css (1)
589-593: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse semantic theme tokens for the stroke colors.
These declarations hard-code black and white. Use an existing semantic foreground or border token with the required alpha. Define a semantic stroke token if no existing token matches.
As per coding guidelines, use shadcn Tailwind semantic colors instead of raw colors.
🤖 Prompt for 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. In `@apps/studio/src/client/styles/globals.css` around lines 589 - 593, Update the --viewer-chrome-stroke-color declarations used by the inset box-shadow to reference existing shadcn semantic foreground or border tokens with the required alpha instead of hard-coded black and white values; define a semantic stroke token only if no suitable token exists, while preserving the light and dark theme behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@apps/studio/src/client/styles/globals.css`:
- Around line 589-593: Update the --viewer-chrome-stroke-color declarations used
by the inset box-shadow to reference existing shadcn semantic foreground or
border tokens with the required alpha instead of hard-coded black and white
values; define a semantic stroke token only if no suitable token exists, while
preserving the light and dark theme behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c3bf56ed-fa68-436e-a2cd-7c50ca181740
📒 Files selected for processing (6)
apps/studio/src/client/components/document-viewers/csv-viewer.tsxapps/studio/src/client/components/document-viewers/viewer-toolbar.tsxapps/studio/src/client/components/document-viewers/xlsx-viewer.tsxapps/studio/src/client/components/file-viewer.tsxapps/studio/src/client/components/zoom-controls.tsxapps/studio/src/client/styles/globals.css
The readout is a menu trigger at three sizes, and the glyph crowded the percentage at every one of them. The trigger's own hover and open states already read as pressable.
Main rewrote the CSV viewer onto the shared DataGrid, which owns the row-count readout this branch had re-inset. The inset moves with it, so every grid-backed viewer keeps the one optical column down the left edge.
Contributor
|
FYI I'm taking this PR over and fixing the double border / flicker issue. |
Which row closes the band comes from the viewer registry rather than a `:has` query on the rendered tree. A viewer mounts its toolbar only once the document has parsed, so the live query drew the hairline under the title row and then moved it a row down when the file landed. The artifact panel's placeholder frame is built from the viewer's own frame and header instead of a second copy, which had drifted to a taller row showing the full path. It renders on every file switch, so that difference read as the name jumping when the viewer took over. The iWork preview banner drops its top border. It sits directly under the hairline that closes the band, so it drew a second line a pixel below the first.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/studio/src/client/components/document-viewers/data-grid.tsx (1)
381-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the Tailwind scale utility for the focus outline.
At
apps/studio/src/client/components/document-viewers/data-grid.tsx:381(Line 381), replacefocus-visible:outline-[2px]withfocus-visible:outline-2. This preserves the 2px outline without an arbitrary value.Proposed change
- focus-visible:outline-[2px] + focus-visible:outline-2🤖 Prompt for 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. In `@apps/studio/src/client/components/document-viewers/data-grid.tsx` at line 381, Update the className on the data-grid container to replace the arbitrary focus-visible outline value with Tailwind’s standard focus-visible:outline-2 utility, preserving the existing 2px focus outline and all other classes.Source: Coding guidelines
🤖 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 `@apps/studio/src/client/components/task/view.tsx`:
- Around line 308-315: The ArtifactPanelShell path currently renders
FileViewerHeader without the resolved MIME type, causing MIME-only toolbar
viewers such as extensionless PDFs to use the wrong chrome layout. Resolve or
propagate the file’s MIME type or equivalent hasToolbar classification before
rendering FileViewerHeader, and pass it through so its divider placement matches
FileViewer’s getFileType result.
---
Nitpick comments:
In `@apps/studio/src/client/components/document-viewers/data-grid.tsx`:
- Line 381: Update the className on the data-grid container to replace the
arbitrary focus-visible outline value with Tailwind’s standard
focus-visible:outline-2 utility, preserving the existing 2px focus outline and
all other classes.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e25ce632-b0c6-466e-95f7-8c16261f9101
📒 Files selected for processing (8)
apps/studio/src/client/components/document-viewers/data-grid.tsxapps/studio/src/client/components/document-viewers/iwork-viewer.tsxapps/studio/src/client/components/document-viewers/viewer-toolbar.tsxapps/studio/src/client/components/document-viewers/xlsx-viewer.tsxapps/studio/src/client/components/file-viewer.tsxapps/studio/src/client/components/task/view.tsxapps/studio/src/client/components/zoom-controls.tsxapps/studio/src/client/styles/globals.css
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/studio/src/client/components/document-viewers/xlsx-viewer.tsx
- apps/studio/src/client/components/document-viewers/viewer-toolbar.tsx
Comment on lines
+308
to
+315
| {/* No mime type: that is part of what the panel is still waiting on. | ||
| Every format whose viewer opens a toolbar is identified by its | ||
| extension anyway, so the chrome band lays out the same either way. */} | ||
| <FileViewerHeader | ||
| filename={filename} | ||
| filePath={filePath} | ||
| onClose={onClose} | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep MIME-only toolbar viewers in the same chrome state.
ArtifactPanelShell omits mimeType at apps/studio/src/client/components/task/view.tsx:308-315 (Lines 308-315). FileViewerHeader therefore classifies an extensionless PDF as unknown and places the divider under the title row. When FileViewer receives application/pdf, getFileType resolves pdf and the PDF entry has hasToolbar: true, so the divider moves below the toolbar. See apps/studio/src/client/components/file-viewer.tsx:742-745 and apps/studio/src/client/lib/get-file-type.ts:291-310.
Carry the resolved MIME type or toolbar classification into ArtifactPanelShell before it renders FileViewerHeader.
🤖 Prompt for 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.
In `@apps/studio/src/client/components/task/view.tsx` around lines 308 - 315, The
ArtifactPanelShell path currently renders FileViewerHeader without the resolved
MIME type, causing MIME-only toolbar viewers such as extensionless PDFs to use
the wrong chrome layout. Resolve or propagate the file’s MIME type or equivalent
hasToolbar classification before rendering FileViewerHeader, and pass it through
so its divider placement matches FileViewer’s getFileType result.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The title row and the document toolbar now share a height and an inset, with no rule between them, so they read as one band rather than a header with a strip bolted underneath. The toolbar's controls hug the row above and leave their slack as the gap before the document.
A hairline closes the band instead: every chrome row carries it, and a row followed by a toolbar drops it, so the stroke lands under whichever row is last whether or not the format has a toolbar. Leading controls that paint their own box or are bare text carry the glyph inset themselves, keeping one optical column down the left edge. The zoom readout also drops its caret, which crowded the percentage at toolbar size.
Summary by CodeRabbit