Surface pending approvals globally#301
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
📝 WalkthroughWalkthroughThe PR adds a global pending-approval banner to the control-center workbench that displays on all pages except mission detail. The banner shows the highest-priority pending approval with optional mission context, includes a "Review approvals" button that navigates to the approvals page, and adapts its layout for mobile screens to prevent overflow. ChangesGlobal Approval Banner Feature
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/control-center-ui-smoke.ts (1)
153-174: ⚡ Quick winConsider adding a mobile test for the approval banner.
The PR description claims to extend "mobile overflow safety" coverage, and the stack context shows a media query for screens under 640px width. However, the smoke test verifies the approval banner only on desktop (1100×760 viewport). The existing mobile test section (lines 788-861) focuses on the mission detail page but doesn't verify the runtime page approval banner.
Following the existing mobile test pattern, consider adding a mobile verification:
- Create a mobile page with operator scope
- Navigate to
#/runtime- Wait for the approval banner
- Assert viewport fit with
assertWithinViewport(mobilePage, ".approval-banner", ...)- Assert no horizontal overflow with
assertNoPageHorizontalOverflow(mobilePage, ...)This would prevent mobile layout regressions and align test coverage with the PR's stated goals.
🤖 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 `@scripts/control-center-ui-smoke.ts` around lines 153 - 174, Add a mobile-specific check for the approval banner similar to the existing desktop flow: create a mobilePage with operator scope (like operatorRuntimePage but using the mobile viewport), navigate it to `#/runtime` (use operatorRuntimePage.goto equivalent for mobilePage), wait for the approval banner locator (approvalBanner or mobilePage.locator(".approval-banner")), then call assertWithinViewport(mobilePage, ".approval-banner", ...) and assertNoPageHorizontalOverflow(mobilePage, ...) to verify fit and no horizontal overflow; mirror the same visibility assertions used for approvalBanner on desktop to ensure the banner content and routing remain correct on mobile.
🤖 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 `@docs/progress/agent-workbench-goal-ledger.md`:
- Around line 3025-3026: The docs claim the pending-approvals banner is excluded
on mission detail, but App.tsx currently renders it whenever
pendingApprovals.length > 0 without checking the route; fix by either updating
the documentation to state that the banner appears on all pages (including
mission detail) to match the implementation, or change the render logic in
App.tsx to only show the banner when pendingApprovals.length > 0 && state.route
!== "mission" (i.e., add the route check where the banner is rendered).
In `@packages/control-center/src/App.tsx`:
- Line 104: The ApprovalBanner is shown whenever pendingApprovals.length > 0 but
must be hidden on mission detail pages; update the render condition for
ApprovalBanner (the line using pendingApprovals and ApprovalBanner) to also
check the route exclusion used on line 103 (e.g., the same isMissionRoute /
isMissionDetail boolean or a check against location.pathname like
!location.pathname.startsWith('/missions/')), so the condition becomes
pendingApprovals.length > 0 && !isMissionRoute (or equivalent) before rendering
<ApprovalBanner approvals={pendingApprovals} />.
---
Nitpick comments:
In `@scripts/control-center-ui-smoke.ts`:
- Around line 153-174: Add a mobile-specific check for the approval banner
similar to the existing desktop flow: create a mobilePage with operator scope
(like operatorRuntimePage but using the mobile viewport), navigate it to
`#/runtime` (use operatorRuntimePage.goto equivalent for mobilePage), wait for
the approval banner locator (approvalBanner or
mobilePage.locator(".approval-banner")), then call
assertWithinViewport(mobilePage, ".approval-banner", ...) and
assertNoPageHorizontalOverflow(mobilePage, ...) to verify fit and no horizontal
overflow; mirror the same visibility assertions used for approvalBanner on
desktop to ensure the banner content and routing remain correct on mobile.
🪄 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: ced2f30f-0338-4730-b6de-b3886977c05b
📒 Files selected for processing (4)
docs/progress/agent-workbench-goal-ledger.mdpackages/control-center/src/App.tsxpackages/control-center/src/styles/app.cssscripts/control-center-ui-smoke.ts
| - Pending approvals are now visible in the main shell with mission/action | ||
| context and a direct Review approvals action. |
There was a problem hiding this comment.
Documentation claims banner excludes mission detail, but code does not.
The checkpoint states that pending approvals are "visible in the main shell" and in the Workbench UX section (line 3025) claims the banner appears on "all pages except mission detail." However, the code in App.tsx line 104 renders the banner whenever pendingApprovals.length > 0 without checking state.route !== "mission".
This is an inconsistency between the ledger documentation and the actual implementation.
The code issue is flagged separately in the App.tsx review. If the mission-detail exclusion is intentional, the code needs the fix proposed there. If the banner should appear on all pages including mission detail, this documentation should be updated to reflect that.
🤖 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 `@docs/progress/agent-workbench-goal-ledger.md` around lines 3025 - 3026, The
docs claim the pending-approvals banner is excluded on mission detail, but
App.tsx currently renders it whenever pendingApprovals.length > 0 without
checking the route; fix by either updating the documentation to state that the
banner appears on all pages (including mission detail) to match the
implementation, or change the render logic in App.tsx to only show the banner
when pendingApprovals.length > 0 && state.route !== "mission" (i.e., add the
route check where the banner is rendered).
| <Sidebar counts={counts} canCreateMission={canCreateMission} onNewMission={openNewMission} /> | ||
| <div className="main"> | ||
| {state.route !== "mission" && <PageToolbar />} | ||
| {pendingApprovals.length > 0 && <ApprovalBanner approvals={pendingApprovals} />} |
There was a problem hiding this comment.
Banner appears on mission detail pages, contradicting documented behavior.
The approval banner is rendered whenever pendingApprovals.length > 0, but according to the PR objectives, AI summary, and ledger checkpoint, it should NOT appear on mission detail pages. Line 103 shows the pattern for excluding the mission route, but line 104 does not apply the same check.
🐛 Proposed fix to exclude mission route
- {pendingApprovals.length > 0 && <ApprovalBanner approvals={pendingApprovals} />}
+ {state.route !== "mission" && pendingApprovals.length > 0 && <ApprovalBanner approvals={pendingApprovals} />}📝 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.
| {pendingApprovals.length > 0 && <ApprovalBanner approvals={pendingApprovals} />} | |
| {state.route !== "mission" && pendingApprovals.length > 0 && <ApprovalBanner approvals={pendingApprovals} />} |
🤖 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 `@packages/control-center/src/App.tsx` at line 104, The ApprovalBanner is shown
whenever pendingApprovals.length > 0 but must be hidden on mission detail pages;
update the render condition for ApprovalBanner (the line using pendingApprovals
and ApprovalBanner) to also check the route exclusion used on line 103 (e.g.,
the same isMissionRoute / isMissionDetail boolean or a check against
location.pathname like !location.pathname.startsWith('/missions/')), so the
condition becomes pendingApprovals.length > 0 && !isMissionRoute (or equivalent)
before rendering <ApprovalBanner approvals={pendingApprovals} />.
1 participant
Summary
Verification
Summary by CodeRabbit
New Features
Tests