Fix/4060 download button#4145
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
Changesfile-viewer duplicate download and button positioning fixes
Estimated code review effort: 2 (Simple) | ~8 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4145/ |
|
yeah, missed the number , should be 4070 instead... |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/file-viewer/file-viewer.tsx (1)
271-279: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the text/email fallback path.
This fixes a UI regression-prone branch, but the adjacent coverage shown here only locks in generic
div.buttonsbehavior. Please add a spec/e2e that forces the text/email fallback and asserts there is exactly one download action whenallowDownloadis enabled.Also applies to: 294-297, 337-360
🤖 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 `@src/components/file-viewer/file-viewer.tsx` around lines 271 - 279, Add a regression spec/e2e around file-viewer.tsx that exercises the renderText/renderEmail fallback path, not just the generic renderButtons behavior. Force the text/email <object> fallback to render, enable allowDownload, and assert there is exactly one download action exposed in that state. Use the file-viewer component methods renderText, renderEmail, and renderButtons to locate the branch and cover the fallback path explicitly.
🤖 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.
Outside diff comments:
In `@src/components/file-viewer/file-viewer.tsx`:
- Around line 271-279: Add a regression spec/e2e around file-viewer.tsx that
exercises the renderText/renderEmail fallback path, not just the generic
renderButtons behavior. Force the text/email <object> fallback to render, enable
allowDownload, and assert there is exactly one download action exposed in that
state. Use the file-viewer component methods renderText, renderEmail, and
renderButtons to locate the branch and cover the fallback path explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bdba5fff-9802-4c4c-bc45-df26495c9ec8
📒 Files selected for processing (1)
src/components/file-viewer/file-viewer.tsx
hatchakka3
left a comment
hatchakka3
left a comment
There was a problem hiding this comment.
Changes look good and have been verified by @ha2818an ✅
ha2818an
force-pushed
the
fix/4060-download-button
branch
from
5a94f4e to
2774b25
Compare
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/file-viewer/partial-styles/ui-controls.scss`:
- Around line 4-8: The `.buttons` positioning in the file viewer controls is
still hardcoded, which leaves consumers unable to avoid collisions with their
own UI. Update the `ui-controls.scss` styles for `.buttons` to expose a consumer
override point instead of just changing the inset, such as a CSS custom property
or a `::part(buttons)` hook, and wire it through the file viewer component so
the toolbar position can be customized externally without relying on internal
DOM structure.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1c3cb9a8-f79e-4fc7-9ef4-54f30175e7ac
📒 Files selected for processing (2)
src/components/file-viewer/file-viewer.tsxsrc/components/file-viewer/partial-styles/ui-controls.scss
| .buttons { | ||
| position: absolute; | ||
| z-index: 1; | ||
| top: 0.25rem; | ||
| bottom: 0.25rem; | ||
| right: 0.25rem; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Repositioning doesn't address the root cause from the linked issue.
Issue #4070 notes there's no exposed ::part or CSS variable for consumers to reposition .buttons, so any fixed position can collide with consumer UI. Hardcoding bottom: 0.25rem instead of top just relocates the collision risk (e.g., to bottom-anchored consumer controls) rather than giving consumers a way to override it.
Consider exposing a CSS custom property (e.g. --limel-file-viewer-buttons-inset) or a ::part(buttons) so consumers can reposition the toolbar without depending on internal DOM.
💡 Example: expose a CSS variable for positioning
.buttons {
position: absolute;
z-index: 1;
- bottom: 0.25rem;
- right: 0.25rem;
+ bottom: var(--limel-file-viewer-buttons-bottom, 0.25rem);
+ right: var(--limel-file-viewer-buttons-right, 0.25rem);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .buttons { | |
| position: absolute; | |
| z-index: 1; | |
| top: 0.25rem; | |
| bottom: 0.25rem; | |
| right: 0.25rem; | |
| .buttons { | |
| position: absolute; | |
| z-index: 1; | |
| bottom: var(--limel-file-viewer-buttons-bottom, 0.25rem); | |
| right: var(--limel-file-viewer-buttons-right, 0.25rem); |
🤖 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 `@src/components/file-viewer/partial-styles/ui-controls.scss` around lines 4 -
8, The `.buttons` positioning in the file viewer controls is still hardcoded,
which leaves consumers unable to avoid collisions with their own UI. Update the
`ui-controls.scss` styles for `.buttons` to expose a consumer override point
instead of just changing the inset, such as a CSS custom property or a
`::part(buttons)` hook, and wire it through the file viewer component so the
toolbar position can be customized externally without relying on internal DOM
structure.
When `allowDownload` is enabled and a text or email file cannot be rendered, the fallback "unsupported file type" message rendered its own download button on top of the one already shown in the controls bar, resulting in two download buttons.
ha2818an
force-pushed
the
fix/4060-download-button
branch
from
2774b25 to
aab9503
Compare
|
🎉 This PR is included in version 39.39.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
4 participants
fix: #4070
Summary by CodeRabbit
Bug Fixes
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: