fix: show full pre-built image build errors#503
Conversation
📝 WalkthroughWalkthroughReplaces inline truncated error text for failed repository images with a tooltip-based display, adding tooltip components and wrapping the component in a tooltip provider while preserving visual truncation in the layout. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
![open-inspect[bot]](https://avatars.githubusercontent.com/u/2492022?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR updates the failed pre-built image status UI to preserve a compact row layout while exposing the full build error through a tooltip and native title. I reviewed the single-file diff and did not find any blocking correctness, security, performance, or maintainability issues.
- PR Title and number: fix: show full pre-built image build errors (#503)
- Author: @ColeMurray
- Files changed count, additions/deletions: 1 file, +16/-3
Critical Issues
None.
Suggestions
- [Testing]
packages/web/src/components/settings/images-settings.tsx- No automated UI coverage was added for the failed-state tooltip/title behavior. This is reasonable for a small UI tweak, but a component-level test would reduce regression risk if this area changes again.
Nitpicks
None.
Positive Feedback
- The change stays narrowly scoped to the presentation layer and preserves the existing API contract.
- Keeping the truncated inline preview while exposing the full message on demand is a good balance between density and debuggability.
- Adding the native
titlealongside the tooltip gives a useful fallback path.
Questions
None.
Verdict
Approve
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/web/src/components/settings/images-settings.tsx (1)
242-256: HoistTooltipProviderto the component root.
TooltipProvideris instantiated per failed image inside therepos.map()loop (via theImageStatuscomponent). Radix UI recommends a single provider at the app/component root so itsdelayDurationandskipDelayDurationare shared across all tooltips, enabling smooth transitions without reapplying delays when moving between triggers. Wrap the outerreturn (<div>...)with oneTooltipProviderinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/components/settings/images-settings.tsx` around lines 242 - 256, Move the TooltipProvider out of the per-item rendering so it is instantiated once at the component root: remove TooltipProvider from inside the ImageStatus/repo.map loop and wrap the top-level return of the ImagesSettings (or parent component that renders repos.map) with a single TooltipProvider so all TooltipTrigger/TooltipContent instances share the same provider and delay settings; update files referencing TooltipProvider, ImageStatus, and the repos.map rendering to reflect the provider now lives at the component root.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/components/settings/images-settings.tsx`:
- Around line 242-256: The span inside the TooltipTrigger in images-settings.tsx
currently has a native title attribute which causes the browser's native tooltip
to appear in addition to Radix's TooltipContent; remove the title prop from the
span (the element wrapped by TooltipTrigger) so only Radix's Tooltip handles the
hover tooltip, and if you need a no-JS/accessibility fallback move the title
text to a non-interactive wrapper outside TooltipTrigger or provide an
appropriate aria-label/aria-describedby instead.
---
Nitpick comments:
In `@packages/web/src/components/settings/images-settings.tsx`:
- Around line 242-256: Move the TooltipProvider out of the per-item rendering so
it is instantiated once at the component root: remove TooltipProvider from
inside the ImageStatus/repo.map loop and wrap the top-level return of the
ImagesSettings (or parent component that renders repos.map) with a single
TooltipProvider so all TooltipTrigger/TooltipContent instances share the same
provider and delay settings; update files referencing TooltipProvider,
ImageStatus, and the repos.map rendering to reflect the provider now lives at
the component root.
🪄 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
Run ID: bcd64d74-8460-41f1-853c-2107940f69d6
📒 Files selected for processing (1)
packages/web/src/components/settings/images-settings.tsx
Terraform Validation Results
Pushed by: @open-inspect[bot], Action: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/components/settings/images-settings.tsx`:
- Around line 244-253: Replace the non-focusable span used as the TooltipTrigger
with a focusable element (e.g., a button) so keyboard users can open the
tooltip: update the TooltipTrigger usage around the element rendering
image.error_message (the code using Tooltip, TooltipTrigger asChild and the
span). Also constrain the TooltipContent to prevent viewport overflow by adding
a max-height and enabling scrolling (use overflow-auto and a sensible max-height
such as max-h-[60vh] or similar) and ensure the trigger has an accessible name
(aria-label or visible text) so screen readers can interact with the button.
🪄 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
Run ID: 8a6e0901-e116-447a-893f-cd9b4950185a
📒 Files selected for processing (1)
packages/web/src/components/settings/images-settings.tsx
1 participant
Summary
error_messagein a tooltip and native title instead of only a hard-truncated previewTesting
Created with Open-Inspect
Summary by CodeRabbit
New Features
Bug Fixes