fix(assets): constrain candidates preview panel height to viewport#2356
fix(assets): constrain candidates preview panel height to viewport#2356SajalChaplot wants to merge 4 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why. then check back every 2 minutes on a loop and see if any new feedback posted, until at least 20 minutes go by without any new feedback posted we want to address, including making sure CI passes too and no merge conflicts (make sure code is mergeable) |
Visual recap — screenshot failedA recap was published, but the PR-comment screenshot could not be captured or uploaded. Open the interactive recap directly: Open the full interactive recap Diagnostic: light: page.screenshot: Timeout 30000ms exceeded. Call log: - taking page screenshot - waiting for fonts to load... |
|
Checked latest PR feedback: there are no inline review comments or review threads posted yet (only this trigger comment). CI check found a real issue: Lint & format was failing due to an oxfmt formatting violation (a stray blank line) in No merge conflicts: Will keep checking every ~2 minutes for new feedback/CI results until 20 minutes pass with nothing new to address. |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Update: The automated Review Agent (Builder bot) reviewed the changes and reported looks good ✅ / Risk: Low — no confirmed bugs, no action items. Nothing to fix there. Separately, the Fast tests required check failed on the latest push, but it's unrelated to this PR's diff: I don't have permission to re-run the workflow on this fork PR ( |
|
I encountered an error while processing your request. Please try again. |
|
Final status after 20+ min of monitoring with no new feedback:
Stopping the monitoring loop here since no new feedback has appeared in the last 20+ minutes. |
|
@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why. then check back every 2 minutes on a loop and see if any new feedback posted, until at least 20 minutes go by without any new feedback posted we want to address, including making sure CI passes too and no merge conflicts (make sure code is mergeable) also rebase off latest main - that might fix the e2e issue |
c278fcc to
d1ee630
Compare
|
Checked latest PR feedback: still no inline review comments or human review threads posted (only trigger comments). Rebased onto latest Will keep monitoring every ~2 minutes for new feedback and CI results until 20 minutes pass with nothing new. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Great news — the rebase fixed the CI issues. All required checks now pass, including:
The automated Review Agent re-reviewed the rebased code and again reported looks good ✅ / Risk: Low, no new issues. PR is |
|
There was a problem with your request, please try again later. Error id: |
|
Final status after 20+ min of monitoring with no new feedback:
Stopping the monitoring loop here. |
|
@builderio-bot look at latest PR feedback and fix anything you agree with. Be skeptical. Reply to every comment (directly on the comment thread of each comment) if you fixed it or not and why. then check back every 2 minutes on a loop and see if any new feedback posted, until at least 20 minutes go by without any new feedback posted we want to address, including making sure CI passes too and no merge conflicts (make sure code is mergeable) |
Preview container now uses a viewport-relative height (60vh capped at 30rem) instead of growing unconstrained with image content.
Surfaces the asset detail link in the candidates header alongside Save/Add to References/Dismiss actions. Also removes the now-unused CandidateStatusPill component.
…tes header Reconciles with the AssetPreviewDialog introduced in #2296 so the header's Open details action opens the inline dialog when available, matching the click-through behavior already used on the image and elsewhere in the file.
d1ee630 to
cab4f79
Compare
|
Checked latest PR feedback: no new inline review comments or human review threads (only the trigger comment). Found and fixed a real merge conflict: since the last rebase, PR #2296 ("Unify asset preview dialog with details panel and navigation") merged into Rebased onto latest
Pushed as cab4f79. PR is now |
There was a problem hiding this comment.
Builder reviewed your changes — looks good ✅
Review Details
Code Review Summary
The latest PR state adds the onOpen/href preview behavior back into the refactored candidates panel and places LiveCandidatesActions alongside the per-candidate header actions. The full-width preview remains bounded by min(60vh, 30rem), thumbnails remain below it with horizontal overflow, and the old desktop two-column grid/sidebar has been removed consistently. The current call sites provide the reference-action handler, and the preview open behavior preserves dialog priority when onOpen is available with link fallback otherwise.
Parallel incremental reviews found no confirmed new, actionable regressions. One agent again noted the optional callback type, but there is no current missing-handler call site, so it is not a reportable runtime issue for this PR.
Risk: Low — UI layout and interaction presentation only; no API, persistence, or schema changes.
✅ Good patterns: preserved dialog-vs-navigation behavior, responsive header wrapping, bounded preview sizing, and retained thumbnail switching semantics.
🧪 Browser testing: Attempted but blocked by infrastructure — browser executors lacked navigation/click/screenshot tools, and the dev server's asset action routes returned NitroVite 500 errors during setup. No app failure was inferred from those environment issues.
|
CI is fully green after the conflict resolution. All required checks pass, and the automated Review Agent re-reviewed the merge resolution and reported looks good ✅ / Risk: Low — confirmed the PR is |
|
There was a problem with your request, please try again later. Error id: |
|
Final status after 20+ min of monitoring with no new feedback:
Stopping the monitoring loop here. |
3 participants
Summary
Previously the preview in drafts tab was smaller that user must open it in a detailed view to see the asset properly. This PR improves UX by increasing the height of the preview.
Test plan
object-contain