Fix known issues/gaps and UI consistency pass - #58
Merged
Conversation
setAdGroupDefaultBid (and setTargetBid, which had the same pattern) validated only that a bid was finite/non-negative, then silently rewrote anything below Amazon's real $0.02 minimum to $0.02 — a caller requesting $0.00 got $0.02 back with a history entry claiming that's what they asked for, contradicting the engine's documented fail-fast convention. Added assertValidBid (finite, non-negative, and >= the real minimum) for these "set an explicit bid on an existing entity" actions, and removed the clamp. Left addTarget/normalizeCampaign's clamping alone — those are creation/normalization paths filling in defaults for otherwise-incomplete data, a different and still-valid use case for clamping. Tightened the two UI call sites' pre-submit guards to match the new floor so they no longer throw uncaught. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
…formatters Several visual/consistency issues flagged by review: - Dashboard, OverviewTab, and PortfolioOverview wrapped their dense data tables in Card, violating the "Table renders edge-to-edge, never Card-wrapped" convention followed everywhere else (Manager*Tab, BudgetRulesTab). Split each into a Card for the header/summary widget portion and a bare Table sibling below it. - PortfolioOverview's campaign-name link used ad-hoc inline styles instead of the shared .row-link class, so it had no hover affordance unlike the identical link on Dashboard and Campaign Manager. - ManagerCampaignsTab always showed "No campaigns yet — Create your first campaign" even when the real cause was an active filter/search matching nothing, pointing the user at campaign creation instead of clearing filters. Added a distinct "no matches" empty state with a Clear filters action, using a new hasAnyCampaigns prop to tell the two cases apart. - Deduplicated three hand-rolled metrics-summing reduce blocks (PortfolioOverview x2, CampaignManager) to the canonical totalMetrics() engine helper, and Dashboard's local fmtMoney/fmtWhole/fmtPercent (which had drifted to 2-decimal percentages) to the canonical formatMoney/formatWhole/formatPercent. - Extracted the "which adFormat means video for this campaign type" check (previously an inline compound boolean in OverviewTab) into a shared isVideoFormat(type, adFormat) engine helper. Updated the Astryx Card-contract test that had pinned the old (incorrect) 3-Card count for OverviewTab down to 2, since "Top targets" is intentionally no longer Card-wrapped. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
…alues - BulkOpsPage's CSV preview table and ReportsPage's selected-report table were Card-wrapped; split each into a header + bare Table, matching the edge-to-edge convention applied elsewhere. - .pill.purple (the SD campaign-type badge) used raw hex (#f3e8ff / #7c3aed) while every sibling pill variant (.active, .orange, .green, .bad) resolves through --info/--accent/--success/--danger tokens. Added --purple/--purple-soft tokens with the same values so a future theme/rebrand updates this badge along with the rest. - TargetsTab's "add keyword" inline form used raw pixel gap/margin literals (8, 10) instead of the --space-* tokens used for the same purpose everywhere else in the app. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
… truth
launchCampaign built campaign ids from Date.now() alone with no
counter, unlike every other entity id in the engine — two campaigns
of the same type launched within the same millisecond (e.g. a
double-clicked "Launch" button) collided, corrupting lookups that
assume unique ids. Now uses generateId('C-' + type), matching the
identical pattern duplicateCampaign already uses.
Also replaced four near-identical hand-rolled uid()/counter/Date.now
helpers in the reports, profiles, integrity, and trainer feature
engines — each was its own independent module-scoped counter that
resets on hot reload — with the shared generateId(prefix), so ID
generation has exactly one implementation instead of five.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
The 6-item training-view check (drills/missions/reports/bulk/trainer/ integrity) was repeated as an identical view === 'x' || ... chain in three functions (getLeftRail, activeTopbarSection, sidebarSectionForView) — exactly the class of duplication that already caused audit H-03 (one of the copies falling out of sync). Extracted a single TRAINING_VIEWS set all three now check against. getKpiTiles also hand-rolled its own ctr/acos/roas formulas instead of calling the canonical calc(), duplicating logic that a future formula fix (e.g. guarding against negative sales) would otherwise need to be applied in two places. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
13 useState hooks (keyword/target inputs, SB creative fields) and two store-action hooks (selectProduct/removeProduct) were initialized from the draft but never read or updated anywhere in the component — the actual form state lives in each step's own store subscription (draft/updateDraft), not these local copies. A future maintainer editing this file could reasonably assume these hooks were live and wire new logic to them, silently doing nothing since the real data flow bypasses them entirely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
- simulation.ts checked each newly generated search term against both the campaign's existing terms and the terms generated so far via two nested linear scans per candidate. Since searchTerms only grows across simulation runs (never pruned), each subsequent 7-day sim click re-scanned an ever-larger array. Replaced with a single Set for O(1) membership checks. - Dashboard subscribed to the whole store `state` and recomputed totalMetrics/calc/getKpiTiles directly in the render body, so any state change anywhere in the app (not just campaign metrics) redid the full-campaign-list aggregate. Memoized on state.campaigns, which keeps the same array reference for state changes that don't touch campaigns. - ManagerSearchTermsTab did the same flatMap+filter scan (O(search terms x negatives)) directly in the render body with no memoization; wrapped in useMemo keyed on the campaigns prop. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
The landing page's feature-grid and CTA icons used Tailwind sizing classes (w-6 h-6, w-4 h-4), but this repo has no Tailwind/PostCSS compiler wired up (confirmed: no tailwind.config.*, no postcss.config.*, not in package.json). Without a compiler these classes are dead — the SVGs had no explicit width/height, so browsers fall back to the default inline-SVG size (300x150) instead of the intended 24px/16px icons. Replaced with explicit width/height attributes, matching how EmptyState.tsx already sizes its icon SVGs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Jpq3N94psa27k8xASDy8B
src/app/page.tsx had literal garbled byte sequences (—, →) committed in place of an em-dash and a right-arrow, likely from an encoding mismatch at some earlier edit. Replaced with the correct UTF-8 characters.
Two Next.js <Image fill> elements on the landing page use className="object-cover" but no CSS rule ever defined that class (no Tailwind compiler is wired up in this repo), so the images had no object-fit applied and could render stretched.
.split used grid-template-columns: 2fr 1fr, which per the CSS grid sizing algorithm floors each track at its content's min-content width rather than actually letting it shrink to its fractional share. With the app sidebar now taking up real width (see next commit), the 1fr column (Operator alerts / Training coverage) no longer fit and was clipped past the right edge of the viewport instead of wrapping. minmax(0, ...) lets both tracks shrink to fit, so content wraps inside its column instead of overflowing.
…ner/Integrity were unreachable The Sidebar component (and the getLeftRail/TRAINING_RAIL model behind it, with its own full unit-test coverage in consoleNav.test.ts) was fully built but never mounted anywhere in the component tree — the .app-body/.app-sidebar CSS classes it depends on existed in globals.css but had no consumer either. Since the top nav's "Training" button only lands on Drills, there was no way for a desktop user to reach Missions, Reports, Bulk ops, Trainer, or Integrity, and no way to jump directly to a campaign's Ad groups/Targeting/etc. tabs or Portfolio's Budget rules from the nav. Mount <Sidebar /> in AdConsole.tsx inside a new .app-body wrapper alongside the existing <main>, and hide it below the tablet breakpoint (same threshold useBreakpoint uses for isMobileOrTablet) so it doesn't double up with the mobile hamburger drawer. MobileNav had the same underlying gap from a different angle: its `section` was hardcoded to only ever resolve to 'portfolio' or 'campaigns', so training views never got a training rail, and its active-item logic only matched the 'campaigns' group. Switched both to the shared sidebarSectionForView/isSidebarItemActive/resolveSidebarClick helpers that Sidebar itself uses, so mobile and desktop navigation stay consistent. Added a contract test pinning that the sidebar renders and actually drives navigation to the previously-unreachable views.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR centralizes bid validation and ID generation, updates Ad Console navigation and campaign metrics, adds filtered empty-state handling, expands campaign launch review data, adjusts card-based layouts, and corrects landing-page styling, copy, and release documentation. ChangesAd Console updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AdConsole
participant MobileNav
participant consoleNav
participant Store
AdConsole->>MobileNav: Render navigation
MobileNav->>consoleNav: Resolve section and click action
consoleNav->>Store: Set tab or view
Store-->>MobileNav: Provide selected tab and view
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engine/ad-console/core/engine/campaign.ts (1)
295-318: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd tests for the changed campaign engine behavior.
Cover changed and unchanged placement history in
savePlacements. CoverSB,SD, and fallback cases inisVideoFormat. The supplied engine test change covers only rejected target bids.As per coding guidelines: changes under
src/engine/must include tests.#!/usr/bin/env bash set -euo pipefail matches="$(rg -n --glob '*.test.ts' --glob '*.test.tsx' \ '\b(savePlacements|isVideoFormat)\b' src/engine || true)" if [[ -z "$matches" ]]; then echo "No engine tests found for savePlacements or isVideoFormat." >&2 exit 1 fi printf '%s\n' "$matches"🤖 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/engine/ad-console/core/engine/campaign.ts` around lines 295 - 318, Add engine tests for savePlacements covering both changed placement values and unchanged values with their exact history entries. Add isVideoFormat tests covering the SB and SD video strings plus fallback cases for unsupported campaign types and nonmatching or missing formats, using the existing engine test conventions.
🧹 Nitpick comments (5)
src/app/page.tsx (1)
11-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace hardcoded SVG dimensions with Astryx sizing.
The changed icons use hardcoded
width="24",height="24",width="16", andheight="16". Use the applicable Astryx icon component or a token-based size definition instead.As per coding guidelines, “Do not use raw hex colors, pixel values, StyleX, Tailwind utility classes, or
xstyle.”Also applies to: 144-144, 178-178, 301-301
🤖 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/app/page.tsx` around lines 11 - 58, Replace the hardcoded width and height attributes on the SVG icons in the page feature sections, including the additional occurrences, with the applicable Astryx icon component sizing or shared token-based size definition. Preserve each icon’s existing appearance and avoid introducing raw pixel values or other prohibited styling approaches.Source: Coding guidelines
src/components/AdConsole/features/reports/ReportsPage.tsx (1)
96-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse an Astryx empty-state component instead of Card.
This empty state is not a dashboard widget, gallery, or settings group. Replace the
Cardwith the appropriate Astryx empty-state component.As per coding guidelines, “Use
Cardonly for dashboard widgets, galleries, and settings groups.”🤖 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/AdConsole/features/reports/ReportsPage.tsx` around lines 96 - 100, Replace the Card wrapper in the empty report branch of ReportsPage with the appropriate Astryx empty-state component, preserving the existing “This report type has no data yet.” message and styling semantics.Source: Coding guidelines
src/components/AdConsole/CampaignManager.tsx (1)
93-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Astryx layout primitives for the changed containers.
The changed containers use raw
<div>elements for layout.
src/components/AdConsole/CampaignManager.tsx#L93-L107: replace the KPI grid wrapper with an Astryx layout component.src/components/AdConsole/Dashboard.tsx#L76-L82: replace the campaign-section wrapper with an Astryx layout component.src/components/AdConsole/features/bulk/BulkOpsPage.tsx#L66-L87: replace the preview wrapper with an Astryx layout component.src/components/AdConsole/features/reports/ReportsPage.tsx#L70-L75,L101-L101: replace the selected-report wrapper with an Astryx layout component.As per coding guidelines, “Use Astryx components for layout; do not use raw
<div>elements.”#!/bin/bash set -euo pipefail npx astryx search "layout grid stack table section" npx astryx component Stack npx astryx component Grid🤖 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/AdConsole/CampaignManager.tsx` around lines 93 - 107, Replace the raw layout divs with appropriate Astryx layout primitives, preserving existing classes, content, and behavior: use an Astryx grid component for the KPI wrapper in src/components/AdConsole/CampaignManager.tsx lines 93-107, an Astryx layout component for the campaign-section wrapper in src/components/AdConsole/Dashboard.tsx lines 76-82, an Astryx layout component for the preview wrapper in src/components/AdConsole/features/bulk/BulkOpsPage.tsx lines 66-87, and an Astryx layout component for both selected-report wrapper locations in src/components/AdConsole/features/reports/ReportsPage.tsx lines 70-75 and 101-101. Use the available Astryx Grid or Stack primitives as appropriate.src/app/globals.css (1)
63-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfigure the SD colors through Astryx theme.
The new values use raw hex colors in
:root. Define these colors throughastryx themeand consume the generated Astryx color tokens.As per coding guidelines, “Do not use raw hex colors” and “configure brand/accent colors through
astryx theme.”🤖 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/app/globals.css` around lines 63 - 64, Replace the raw SD color values in the :root variables --purple and --purple-soft with the corresponding generated Astryx theme color tokens. Configure the SD campaign badge colors through the Astryx theme rather than defining hex values directly, while preserving the existing variable names and usage.Source: Coding guidelines
src/components/AdConsole/PortfolioOverview.tsx (1)
98-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace raw layout elements and hardcoded styling with Astryx primitives.
The portfolio section adds raw
<div>and<span>layout elements. It also usesgap: 8and inline visual styles. Use Astryx layout and text components. Use Astryx tokens or component props for spacing and text styling. Keep the campaignTableoutside theCard.As per coding guidelines, “Use Astryx components for layout; do not use raw
<div>elements” and “Do not use raw hex colors, pixel values, StyleX, Tailwind utility classes, orxstyle.”Also applies to: 143-144
🤖 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/AdConsole/PortfolioOverview.tsx` around lines 98 - 129, Replace the raw layout and text elements in the portfolio card rendering around the manage-mode header and metrics grid with the appropriate Astryx layout and text primitives, including component props or design tokens for spacing and typography instead of inline gap, margin, and font-weight values. Update both the portfolio header and the additionally referenced lines 143–144, while keeping the campaign Table outside the Card.Source: Coding guidelines
🤖 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/AdConsole/AdConsole.tsx`:
- Around line 54-61: Update AdConsole.tsx to use Astryx’s AppShell as the page
shell and place the Sidebar navigation inside SideNav while preserving the main
content and ErrorBoundary flow. In
src/components/AdConsole/details/OverviewTab.tsx lines 105-129, retain the
edge-to-edge Table but replace its raw div wrapper with the available Astryx
layout component.
In `@src/components/AdConsole/details/AdGroupsTab.tsx`:
- Around line 66-69: Add visible validation feedback to both bid handlers:
src/components/AdConsole/details/AdGroupsTab.tsx lines 66-69 should report when
the bid is below MIN_BID instead of silently skipping setAdGroupDefaultBid, and
src/components/AdConsole/details/TargetsTab.tsx lines 111-114 should do the same
when setTargetBid is skipped. Use a shared validation result or field-level
error state consistently across both paths while preserving valid-bid updates.
In `@src/components/AdConsole/mobile/MobileNav.tsx`:
- Around line 6-12: Update the MobileNav component’s layout markup to use
Astryx’s SideNav and supported drawer, group, label, and icon-button components
instead of the raw div and span elements identified in the navigation and lines
85-90. Preserve the existing shared navigation action logic and navigation
behavior while replacing only the layout primitives.
In `@src/engine/ad-console/core/simulation.ts`:
- Around line 77-98: Add regression tests for the simulation flow around
seenTerms and repeated simulateDays calls, covering overlapping Keyword targets
and multiple simulation runs. Assert that searchTerms contains exactly one
record per generated term, with no duplicates across targets or runs, and place
the tests according to the repository’s existing src/engine testing conventions.
In `@src/engine/ad-console/features/integrity/engine.ts`:
- Around line 10-13: Add regression tests for each shared ID migration using a
fixed timestamp and verify repeated creations produce distinct IDs: test the
public issue-creation path in
src/engine/ad-console/features/integrity/engine.ts:10-13, repeated createProfile
calls in src/engine/ad-console/features/profiles/engine.ts:6-10, repeated
createReportRequest calls in
src/engine/ad-console/features/reports/engine.ts:6-13, repeated generateReport
calls in src/engine/ad-console/features/reports/engine.ts:47-47, and repeated
addNote calls in src/engine/ad-console/features/trainer/engine.ts:6-11.
---
Outside diff comments:
In `@src/engine/ad-console/core/engine/campaign.ts`:
- Around line 295-318: Add engine tests for savePlacements covering both changed
placement values and unchanged values with their exact history entries. Add
isVideoFormat tests covering the SB and SD video strings plus fallback cases for
unsupported campaign types and nonmatching or missing formats, using the
existing engine test conventions.
---
Nitpick comments:
In `@src/app/globals.css`:
- Around line 63-64: Replace the raw SD color values in the :root variables
--purple and --purple-soft with the corresponding generated Astryx theme color
tokens. Configure the SD campaign badge colors through the Astryx theme rather
than defining hex values directly, while preserving the existing variable names
and usage.
In `@src/app/page.tsx`:
- Around line 11-58: Replace the hardcoded width and height attributes on the
SVG icons in the page feature sections, including the additional occurrences,
with the applicable Astryx icon component sizing or shared token-based size
definition. Preserve each icon’s existing appearance and avoid introducing raw
pixel values or other prohibited styling approaches.
In `@src/components/AdConsole/CampaignManager.tsx`:
- Around line 93-107: Replace the raw layout divs with appropriate Astryx layout
primitives, preserving existing classes, content, and behavior: use an Astryx
grid component for the KPI wrapper in
src/components/AdConsole/CampaignManager.tsx lines 93-107, an Astryx layout
component for the campaign-section wrapper in
src/components/AdConsole/Dashboard.tsx lines 76-82, an Astryx layout component
for the preview wrapper in
src/components/AdConsole/features/bulk/BulkOpsPage.tsx lines 66-87, and an
Astryx layout component for both selected-report wrapper locations in
src/components/AdConsole/features/reports/ReportsPage.tsx lines 70-75 and
101-101. Use the available Astryx Grid or Stack primitives as appropriate.
In `@src/components/AdConsole/features/reports/ReportsPage.tsx`:
- Around line 96-100: Replace the Card wrapper in the empty report branch of
ReportsPage with the appropriate Astryx empty-state component, preserving the
existing “This report type has no data yet.” message and styling semantics.
In `@src/components/AdConsole/PortfolioOverview.tsx`:
- Around line 98-129: Replace the raw layout and text elements in the portfolio
card rendering around the manage-mode header and metrics grid with the
appropriate Astryx layout and text primitives, including component props or
design tokens for spacing and typography instead of inline gap, margin, and
font-weight values. Update both the portfolio header and the additionally
referenced lines 143–144, while keeping the campaign Table outside the Card.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 19f7a1ca-0341-4ed3-9254-23d14e326ea3
📒 Files selected for processing (33)
src/app/globals.csssrc/app/page.tsxsrc/components/AdConsole/AdConsole.tsxsrc/components/AdConsole/CampaignManager.tsxsrc/components/AdConsole/Dashboard.tsxsrc/components/AdConsole/PortfolioOverview.tsxsrc/components/AdConsole/__tests__/ManagerCampaignsTab.test.tsxsrc/components/AdConsole/__tests__/contracts/cards-astryx.test.tsxsrc/components/AdConsole/__tests__/contracts/sidebar.test.tsxsrc/components/AdConsole/details/AdGroupsTab.tsxsrc/components/AdConsole/details/ManagerCampaignsTab.tsxsrc/components/AdConsole/details/ManagerSearchTermsTab.tsxsrc/components/AdConsole/details/OverviewTab.tsxsrc/components/AdConsole/details/TargetsTab.tsxsrc/components/AdConsole/features/bulk/BulkOpsPage.tsxsrc/components/AdConsole/features/reports/ReportsPage.tsxsrc/components/AdConsole/mobile/MobileNav.tsxsrc/components/AdConsole/nav/consoleNav.tssrc/components/AdConsole/wizard/CreateCampaignWizard.tsxsrc/engine/ad-console/__tests__/store.test.tssrc/engine/ad-console/core/__tests__/adgroup.test.tssrc/engine/ad-console/core/__tests__/engine.test.tssrc/engine/ad-console/core/engine/adgroup.tssrc/engine/ad-console/core/engine/campaign.tssrc/engine/ad-console/core/engine/index.tssrc/engine/ad-console/core/engine/target.tssrc/engine/ad-console/core/simulation.tssrc/engine/ad-console/core/slices/core.tssrc/engine/ad-console/features/integrity/engine.tssrc/engine/ad-console/features/profiles/engine.tssrc/engine/ad-console/features/reports/engine.tssrc/engine/ad-console/features/trainer/engine.tssrc/lib/validation.ts
💤 Files with no reviewable changes (1)
- src/components/AdConsole/wizard/CreateCampaignWizard.tsx
There was a problem hiding this comment.
Pull request overview
This PR tightens core engine correctness (bid validation + ID generation) and follows through on a UI consistency/navigation pass across the AdConsole surface, including wiring previously-unreachable Training views into the main layout.
Changes:
- Enforce a shared minimum-bid contract (
MIN_BID) and centralize explicit-bid validation for “set bid” actions; add regression tests. - Deduplicate ID generation and other drift-prone logic (metrics/KPIs/nav), plus a simulation performance improvement for search-term duplicate detection.
- UI/UX consistency + navigation fixes: mount the Sidebar in the main layout, align MobileNav with the shared nav helpers, and remove Card-wrapping from dense tables per the project’s table conventions.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/lib/validation.ts | Adds MIN_BID + assertValidBid for fail-fast bid validation. |
| src/engine/ad-console/features/trainer/engine.ts | Switches trainer note IDs to shared generateId. |
| src/engine/ad-console/features/reports/engine.ts | Switches report/request IDs to shared generateId. |
| src/engine/ad-console/features/profiles/engine.ts | Switches trainee profile IDs to shared generateId. |
| src/engine/ad-console/features/integrity/engine.ts | Switches integrity issue IDs to shared generateId. |
| src/engine/ad-console/core/slices/core.ts | Fixes campaign ID collision risk in launchCampaign via generateId. |
| src/engine/ad-console/core/simulation.ts | Improves search-term duplicate detection from O(n·m) to O(1) lookups via Set. |
| src/engine/ad-console/core/engine/target.ts | Makes setTargetBid fail fast below the platform minimum bid. |
| src/engine/ad-console/core/engine/index.ts | Re-exports isVideoFormat from campaign engine. |
| src/engine/ad-console/core/engine/campaign.ts | Adds isVideoFormat helper as single source of truth for “video” adFormat strings. |
| src/engine/ad-console/core/engine/adgroup.ts | Makes setAdGroupDefaultBid fail fast below the minimum bid. |
| src/engine/ad-console/core/tests/engine.test.ts | Adds coverage for below-min bid behavior on setTargetBid. |
| src/engine/ad-console/core/tests/adgroup.test.ts | Updates/extends coverage for setAdGroupDefaultBid bid-floor behavior. |
| src/engine/ad-console/tests/store.test.ts | Adds regression test for campaign ID uniqueness within the same millisecond. |
| src/components/AdConsole/wizard/CreateCampaignWizard.tsx | Removes dead local state/hooks and unused imports. |
| src/components/AdConsole/PortfolioOverview.tsx | Uses shared metrics aggregation + removes Card-wrapping from dense tables. |
| src/components/AdConsole/nav/consoleNav.ts | Deduplicates training-view detection + uses shared calc for KPIs. |
| src/components/AdConsole/mobile/MobileNav.tsx | Aligns mobile drawer behavior with shared sidebar click/active helpers. |
| src/components/AdConsole/features/reports/ReportsPage.tsx | Removes Card-wrapping from the main dense table; keeps Card for empty state. |
| src/components/AdConsole/features/bulk/BulkOpsPage.tsx | Removes Card-wrapping around dense preview table. |
| src/components/AdConsole/details/TargetsTab.tsx | Guards “Set bid” UI against below-min bids using MIN_BID. |
| src/components/AdConsole/details/OverviewTab.tsx | Uses isVideoFormat + removes Card-wrapping from dense “Top targets” table. |
| src/components/AdConsole/details/ManagerSearchTermsTab.tsx | Memoizes derived rows to avoid expensive recomputation on re-renders. |
| src/components/AdConsole/details/ManagerCampaignsTab.tsx | Distinguishes “no campaigns yet” vs “no matches” empty states. |
| src/components/AdConsole/details/AdGroupsTab.tsx | Guards “Save default bid” UI against below-min bids using MIN_BID. |
| src/components/AdConsole/Dashboard.tsx | Memoizes KPI computations + removes Card-wrapping from the dense campaigns table. |
| src/components/AdConsole/CampaignManager.tsx | Uses shared totalMetrics + wires “Clear filters” into empty-state UX. |
| src/components/AdConsole/AdConsole.tsx | Mounts Sidebar into the main desktop layout (app-body). |
| src/components/AdConsole/tests/ManagerCampaignsTab.test.tsx | Updates tests for new empty-state behavior/props. |
| src/components/AdConsole/tests/contracts/sidebar.test.tsx | New contract tests ensuring Sidebar is mounted and drives navigation. |
| src/components/AdConsole/tests/contracts/cards-astryx.test.tsx | Updates card-count expectations to match the “dense tables not Card-wrapped” convention. |
| src/app/page.tsx | Fixes landing-page mojibake and replaces Tailwind-size classes with explicit SVG sizing. |
| src/app/globals.css | Adds purple tokens, fixes .split overflow via minmax(0, …), and adds .object-cover. |
Suppressed comments (1)
src/engine/ad-console/core/engine/target.ts:152
- Because
setTargetBidnow throws for bids below the platform floor,adjustTargetBid(which callssetTargetBid(c, ..., t.bid * multiplier)) will also throw when a decrement pushes the bid below the minimum. This can happen via the UI "-10%" button when a target is already at $0.02, and will surface as an unexpected runtime error. Consider clamping insideadjustTargetBidfor decrement-style actions so they respect the floor without throwing.
? `Bid for "${t.value}" (${t.type}) changed from $${t.bid.toFixed(2)} to $${newBid.toFixed(2)}`
: `Bid updated for target ${targetId}`;
})(),
],
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot flagged that setTargetBid's new fail-fast MIN_BID check (previous commit) has a real regression: adjustTargetBid — the "-10%"/"+10%" bid-adjustment buttons — calls setTargetBid with a multiplied value it doesn't control, so decrementing a target already near the floor now throws an uncaught ValidationError instead of the old clamping behavior. Unlike an explicit "Set bid" action, adjustTargetBid is a relative nudge, so it should floor at MIN_BID rather than fail fast.
CodeRabbit flagged several changed engine functions without direct regression tests, per CLAUDE.md's "changes under src/engine/ must include tests" convention: - isVideoFormat: SB/SD video-string matches and the SP/undefined fallback. - Shared generateId migration (profiles/reports/trainer/integrity engines): each creation path now has a same-millisecond uniqueness regression test, mirroring the one already added for launchCampaign. - simulateDays' Set-based search-term dedup: added a test covering duplicates across repeated simulateDays calls, complementing the existing "does not add duplicate search terms" test which already covered duplicates within a single run from overlapping targets.
- reports/engine.ts imported assertNonEmpty but never used it (pre-existing, unrelated to this PR's changes; Copilot flagged it). - ReportsPage's "no data yet" fallback used a bare Card instead of the EmptyState component already used the same way elsewhere (ManagerCampaignsTab, ManagerSearchTermsTab, HistoryTab).
Owner
Author
|
Thanks both — went through everything from CodeRabbit and Copilot. Pushed three follow-up commits: Fixed:
Not doing, with reasons:
All 659 tests pass, Generated by Claude Code |
Root cause: Astryx ships its component styles (including the padding
generated from a Card's `padding` prop) inside `@layer astryx-base`/
`@layer astryx-theme`. Per the CSS cascade-layers spec, an unlayered
declaration always wins over a layered one regardless of specificity.
This repo's global reset (`*, *::before, *::after { ...; padding: 0; }`)
is unlayered, so it was unconditionally beating every Astryx `padding`
prop across the entire app — every Card rendered with 0 padding no
matter what value was passed, which is why content (e.g. the "Remove"
button on wizard product cards, headings on dashboard cards) sat
flush against card edges instead of respecting their intended spacing.
Confirmed via computed-style inspection: a Card with `padding={6}`
resolved to `padding: 0px` before this fix and `padding: 23px`
(24px token minus the 1px border inset) after.
Scoped the padding reset down to the two native elements that actually
relied on it (`ul`, `ol`) instead of zeroing it on every element.
makeDraft() defaults audienceLookback to '30' regardless of campaign
type, so Step6ReviewLaunch's `{d.audienceLookback && ...}` check was
truthy from the moment the wizard opens — every campaign's review
screen showed "Lookback: 30 days", including plain Sponsored Products
campaigns where the concept doesn't exist. Gated it on
`targetingMode.includes('Audiences')` instead, matching the same
condition the SB/SD targeting steps already use to show the lookback
selector in the first place.
Also filled in review-step gaps found during the same audit: ASIN
targets, category targets, and audience targets entered in step 4 had
no summary row at all, and SB/SD creative fields (headline, brand,
destination) set in step 3 weren't shown anywhere before launch.
Owner
Author
|
Did a deep pass on the Campaign Creation wizard specifically (all 6 steps × SP/SB/SD, logic/flow/fidelity to the real Amazon console). Two more commits pushed: Root-cause CSS bug, sitewide impact: while checking the product-selection step's spacing, found that every Astryx Wizard-specific bug: the Review & Launch step showed a "Lookback: 30 days" row on every single campaign type, including plain Sponsored Products — Rest of the wizard (all 3 ad types' targeting/bidding/placement options, product selection, launch → target creation) checked out as functionally correct and reasonably faithful to the real console. 665 tests passing, Generated by Claude Code |
Reflects this release's changes: sidebar navigation wiring, the sitewide Astryx Card padding fix, wizard review-step fixes, and the bid-validation/ID-generation/UI-consistency work — see CHANGELOG.md. Synced package-lock.json's duplicate version fields and the unused coreState.version display field to match.
- Add CHANGELOG.md (new — this repo had no changelog convention before), covering every fix/change in this release. - CLAUDE.md: document MIN_BID/assertValidBid, the isVideoFormat helper, the EmptyState component convention, and — most importantly — the CSS cascade-layers rule that caused the sitewide Card padding bug, so it doesn't get silently reintroduced by a future "helpful" reset rule. - docs/FEATURES.md: the mobile/responsive breakpoint table described a "sidebar collapses to 200px" tablet behavior that was never actually implemented (and now that the sidebar is wired up, would be misleading) — corrected to match what the code actually does: hidden below 1100px, hamburger drawer takes over for both mobile and tablet.
Owner
Author
|
Pushed two more commits documenting this release:
665 tests passing, Generated by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/AdConsole/wizard/__tests__/Step6ReviewLaunch.test.tsx`:
- Around line 57-70: Update the test around the “shows the SB/SD creative
headline, brand, and destination when set” case to explicitly set the creative
type to both SB and SD, preferably by parameterizing the test or adding separate
cases. Ensure each case updates the draft with the intended type before
rendering Step6ReviewLaunch and retains the existing headline, brand, and
destination assertions.
In `@src/components/AdConsole/wizard/Step6ReviewLaunch.tsx`:
- Around line 41-47: Update the campaign summary section in Step6ReviewLaunch to
use Astryx layout components instead of review-box/review-row divs and raw
span/strong elements. Apply one consistent Astryx row pattern to all displayed
fields, including targets, lookback, and creative values, while preserving the
existing conditional rendering and counts.
In `@src/engine/ad-console/features/integrity/__tests__/engine.test.ts`:
- Around line 118-136: The integrity test at
src/engine/ad-console/features/integrity/__tests__/engine.test.ts lines 118-136
must assert that runIntegrityCheck produces multiple issues before checking ID
uniqueness. In src/engine/ad-console/core/__tests__/simulation.test.ts lines
174-187, assert the first run produces terms and verify every first-run term
exists in the second run; update both tests to prevent passing with empty or
missing records.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f8851b2-c1e9-4603-a0a1-7970594e4157
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
CHANGELOG.mdCLAUDE.mddocs/FEATURES.mdpackage.jsonsrc/app/globals.csssrc/components/AdConsole/features/reports/ReportsPage.tsxsrc/components/AdConsole/wizard/Step6ReviewLaunch.tsxsrc/components/AdConsole/wizard/__tests__/Step6ReviewLaunch.test.tsxsrc/engine/ad-console/core/__tests__/engine.test.tssrc/engine/ad-console/core/__tests__/simulation.test.tssrc/engine/ad-console/core/engine/target.tssrc/engine/ad-console/core/slices/core.tssrc/engine/ad-console/features/integrity/__tests__/engine.test.tssrc/engine/ad-console/features/profiles/__tests__/engine.test.tssrc/engine/ad-console/features/reports/__tests__/engine.test.tssrc/engine/ad-console/features/reports/engine.tssrc/engine/ad-console/features/trainer/__tests__/engine.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/AdConsole/features/reports/ReportsPage.tsx
- src/engine/ad-console/core/slices/core.ts
- src/engine/ad-console/core/engine/target.ts
- src/engine/ad-console/features/reports/engine.ts
…uous tests
CodeRabbit flagged that AdGroupsTab/TargetsTab silently do nothing when
a typed bid is below MIN_BID, with no visible error. Investigated both:
- AdGroupsTab uses Astryx's NumberInput with min={0.02} — it already
shows real-time invalid styling (aria-invalid, red border, "Invalid
number" live-region text) via the component's own built-in min
validation, with no app code needed. Added a regression test proving
this rather than adding redundant/dead validation logic.
- TargetsTab uses a raw native <input type="number">, which doesn't
get that behavior for free — genuinely had no visible feedback.
Added a red border, aria-invalid, and a title tooltip when the
edited value is below MIN_BID.
Also fixed two review-flagged test gaps that could pass vacuously:
- The integrity same-millisecond-uniqueness test didn't assert the
fixture actually produced more than one issue before checking
uniqueness (an empty or single-item array trivially passes).
- The repeated-simulateDays dedup test didn't assert the first run
produced any search terms, nor that the second run actually carried
them forward — both now asserted explicitly.
Declined (with reasoning, per the PR's review thread): rewriting
Step6ReviewLaunch's review-row layout onto different Astryx layout
primitives — `review-row`/`review-box` is the same pattern already
used in OverviewTab.tsx; changing just one of the two would make them
inconsistent with each other, not more consistent.
Owner
Author
|
Addressed the latest round of review feedback: Fixed:
Not doing, with reason: rewriting 668 tests passing, Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two parts, per the request to "fix all known issues, gaps and then focus on UI, visual look for consistency, missing padding, borders, overlaps, colors, styles":
Known issues/gaps (from earlier code-review findings):
setTargetBid/setAdGroupDefaultBidnow fail fast on a below-minimum bid instead of silently clamping it (creation paths likeaddTargetstill clamp, since that's for incomplete-data defaults, not explicit user intent).PortfolioOverview,Dashboard,CampaignManager, severalfeatures/*/engine.tsfiles,launchCampaign), fixing a real ID-collision risk inlaunchCampaignalong the way.PortfolioOverview,Dashboard,OverviewTab,BulkOpsPage,ReportsPage— CLAUDE.md says dense tables render edge-to-edge, not Card-wrapped).CreateCampaignWizard's 13 unuseduseStatehooks and unused imports).Visual QA pass (live browser walkthrough via Playwright):
ΓÇö,ΓåÆ) in the landing page copy — an em-dash and an arrow got corrupted at some earlier edit..object-coverCSS rule — two landing-page<Image fill>elements referenced a Tailwind-style class that this repo has no compiler for, so they had noobject-fitapplied.Sidebarcomponent (and thegetLeftRail/training-rail model behind it, with its own full unit-test coverage) was fully built but never mounted into the app layout —.app-sidebar/.app-bodyexisted in CSS with no consumer. This meant Missions, Reports, Bulk ops, Trainer, and Integrity were completely unreachable from the desktop UI, andMobileNavhad the same gap from a different angle (its section resolution was hardcoded to onlyportfolio/campaigns). Wired the sidebar intoAdConsole.tsx, hid it below the tablet breakpoint so it doesn't double up with the mobile drawer, and fixedMobileNavto use the same shared nav-resolution helpers..split's2fr 1frgrid didn't shrink correctly once the sidebar took up real width, causing the "Operator alerts"/"Training coverage" cards to be clipped past the viewport edge instead of wrapping.Confirmed some suspected issues were false alarms and left alone: Framer Motion scroll-in animations rendering blank in a screenshot taken without scrolling, and wide tables scrolling internally via Astryx's own
.astryx-table-scroll-wrapper(both are correct, intentional behavior per CLAUDE.md's own documented conventions).Test plan
npx tsc --noEmitnpx vitest run(648 tests passing, including a new contract test suite pinning the sidebar fix)npm run buildGenerated by Claude Code
Summary by CodeRabbit