chore: show error in tooltip for failed plans#1055
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 41 minutes and 55 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTwo files updated to enhance plan error visibility: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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/web/app/routes/ws/deployments/page.$deploymentId.plans.$planId.tsx (1)
111-111: Prefer explicit props over spreading the entireresult.
<PlanStatusBadge {...result} />forwards every field onresult(e.g.,environment,resource,agent,resultId,diffStats,hasChanges, ...) as props. TypeScript's excess-property check doesn't apply to spreads, so these silently flow through and are ignored by the component. Being explicit makes the contract between caller and component clearer and avoids surprises ifPlanStatusBadgelater adds a prop that happens to collide with aResultfield name.♻️ Proposed change
- <PlanStatusBadge {...result} /> + <PlanStatusBadge status={result.status} message={result.message} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/app/routes/ws/deployments/page`.$deploymentId.plans.$planId.tsx at line 111, The call currently spreads the entire result object into PlanStatusBadge which lets unrelated Result fields leak into the component; replace the spread with explicit prop assignments by reading PlanStatusBadge's prop signature and passing only those fields from result (for example map result.status, result.hasChanges, result.diffStats, result.environment, result.resource, etc. to the corresponding PlanStatusBadge props), removing {...result} and explicitly mapping each required prop so TypeScript can catch excess/missing props and avoid accidental prop collisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web/app/routes/ws/deployments/page`.$deploymentId.plans.$planId.tsx:
- Line 111: The call currently spreads the entire result object into
PlanStatusBadge which lets unrelated Result fields leak into the component;
replace the spread with explicit prop assignments by reading PlanStatusBadge's
prop signature and passing only those fields from result (for example map
result.status, result.hasChanges, result.diffStats, result.environment,
result.resource, etc. to the corresponding PlanStatusBadge props), removing
{...result} and explicitly mapping each required prop so TypeScript can catch
excess/missing props and avoid accidental prop collisions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 847edc03-2407-4119-b0ce-71aa07f3c162
📒 Files selected for processing (2)
apps/web/app/routes/ws/deployments/_components/plans/PlanStatusBadge.tsxapps/web/app/routes/ws/deployments/page.$deploymentId.plans.$planId.tsx
There was a problem hiding this comment.
Pull request overview
Adds inline visibility into deployment plan failures by surfacing plan result error messages via a tooltip on the status badge (fixes #1052).
Changes:
- Update
PlanStatusBadgeto optionally render an error/info tooltip when amessageis present. - Pass plan result
messageinto the status badge from the plan detail results table. - Remove the previous
title-attribute error display from the “Changes” cell for errored results.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/web/app/routes/ws/deployments/page.$deploymentId.plans.$planId.tsx | Passes full result (including message) into PlanStatusBadge and removes the old title tooltip from the “Changes” column. |
| apps/web/app/routes/ws/deployments/_components/plans/PlanStatusBadge.tsx | Adds tooltip UI around the status badge when a message exists and indicates presence via an icon. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <TooltipTrigger> | ||
| <PlanStatusBadgeInner status={status} hasMessage /> |
There was a problem hiding this comment.
TooltipTrigger is used without asChild, so Radix renders a <button> wrapper. In this table, rows can be clickable (onClick on TableRow), so clicking/focusing the status badge can unintentionally trigger the row click (and the badge is announced as a button even though it’s just a status). Consider using asChild with an explicit element and stopping event propagation (e.g., onClick/onPointerDown) so the tooltip trigger doesn’t activate the row handler.
| <TooltipTrigger> | |
| <PlanStatusBadgeInner status={status} hasMessage /> | |
| <TooltipTrigger asChild> | |
| <span | |
| onClick={(event) => event.stopPropagation()} | |
| onPointerDown={(event) => event.stopPropagation()} | |
| > | |
| <PlanStatusBadgeInner status={status} hasMessage /> | |
| </span> |
| <TooltipContent className="wrap-break-word flex max-w-sm items-center gap-1.5"> | ||
| {message} | ||
| </TooltipContent> |
There was a problem hiding this comment.
message is rendered as plain text inside the tooltip; any newline characters in stored plan error messages will be collapsed by HTML, making multi-line errors/stack traces hard to read. Consider applying a whitespace-preserving style (e.g., whitespace-pre-wrap) and word-breaking to keep long/multi-line messages readable within the max width.
fixes #1052
Summary by CodeRabbit
New Features
Refactor