Conversation
Greptile SummaryThis PR successfully simplifies the ad provider contract by replacing the Confidence Score: 5/5Safe to merge; all remaining findings are minor style suggestions. No logic bugs or data-integrity issues found. The simplification is correct end-to-end: client, server, and provider types all agree on the new array shape. The two P2 comments are optional follow-ups. No files require special attention.
|
| Filename | Overview |
|---|---|
| web/src/lib/ad-providers/types.ts | Removed AdVariant type and simplified FetchAdResult from a banner/choice union to a plain { ads: NormalizedAd[] } |
| web/src/lib/ad-providers/gravity.ts | Removed A/B variant logic (getGravityVariant, BANNER_PLACEMENT_ID) and now always returns { ads: [...] }; straightforward simplification. |
| web/src/lib/ad-providers/carbon.ts | Trivially updated return value from { variant: 'choice', ads } to { ads }; no logic change. |
| web/src/app/api/v1/ads/_post.ts | Added noAdsResponse helper and unified all response paths to { ads, variant: 'choice', provider }; variant field is vestigial but harmless. |
| cli/src/hooks/use-gravity-ad.ts | Removed AdData/AdVariant types, banner cache, and showAd(); now uses a single ads state and choice cache path; showChoiceAds is now a trivial wrapper around setAds. |
| cli/src/chat.tsx | Switched from adData with variant-conditional array construction to ads directly; render logic is cleaner. |
| cli/src/components/waiting-room-screen.tsx | Same adData → ads simplification as chat.tsx; no issues. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: cli/src/hooks/use-gravity-ad.ts
Line: 183-186
Comment:
**`showChoiceAds` is now a trivial wrapper**
After removing `showAd`, `showChoiceAds` is a single-line function wrapping `setAds`. It could be inlined at its two call sites to further reduce indirection.
Then replace the two `showChoiceAds(result.ads)` / `showChoiceAds(cachedSet)` calls with `setAds(result.ads)` / `setAds(cachedSet)` directly.
**Context Used:** Find ways to simplify the implementation ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: web/src/app/api/v1/ads/_post.ts
Line: 56-61
Comment:
**Vestigial `variant` field in responses**
Now that both the CLI hook and the server-side type contract have dropped the banner/choice distinction, `variant: 'choice'` in `noAdsResponse` (and in the success/error responses in `_post.ts`) is never read by any consumer. Dropping it would complete the simplification and keep the wire format consistent with the new `FetchAdResult` shape.
```suggestion
function noAdsResponse(provider: AdProviderId) {
return NextResponse.json(
{ ads: [], provider },
{ status: 200 },
)
}
```
**Context Used:** Find ways to simplify the implementation ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Simplify ad response shape" | Re-trigger Greptile
| // Show a choice ad set (impressions are fired by the component for visible ads only) | ||
| const showChoiceAds = (ads: AdResponse[]): void => { | ||
| setAd(ads[0] ?? null) // Keep backwards compat for ad field | ||
| setAdData({ variant: 'choice', ads }) | ||
| setAds(ads) | ||
| } |
There was a problem hiding this comment.
showChoiceAds is now a trivial wrapper
After removing showAd, showChoiceAds is a single-line function wrapping setAds. It could be inlined at its two call sites to further reduce indirection.
Then replace the two showChoiceAds(result.ads) / showChoiceAds(cachedSet) calls with setAds(result.ads) / setAds(cachedSet) directly.
Context Used: Find ways to simplify the implementation (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/hooks/use-gravity-ad.ts
Line: 183-186
Comment:
**`showChoiceAds` is now a trivial wrapper**
After removing `showAd`, `showChoiceAds` is a single-line function wrapping `setAds`. It could be inlined at its two call sites to further reduce indirection.
Then replace the two `showChoiceAds(result.ads)` / `showChoiceAds(cachedSet)` calls with `setAds(result.ads)` / `setAds(cachedSet)` directly.
**Context Used:** Find ways to simplify the implementation ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| function noAdsResponse(provider: AdProviderId) { | ||
| return NextResponse.json( | ||
| { ads: [], variant: 'choice', provider }, | ||
| { status: 200 }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Vestigial
variant field in responses
Now that both the CLI hook and the server-side type contract have dropped the banner/choice distinction, variant: 'choice' in noAdsResponse (and in the success/error responses in _post.ts) is never read by any consumer. Dropping it would complete the simplification and keep the wire format consistent with the new FetchAdResult shape.
| function noAdsResponse(provider: AdProviderId) { | |
| return NextResponse.json( | |
| { ads: [], variant: 'choice', provider }, | |
| { status: 200 }, | |
| ) | |
| } | |
| function noAdsResponse(provider: AdProviderId) { | |
| return NextResponse.json( | |
| { ads: [], provider }, | |
| { status: 200 }, | |
| ) | |
| } |
Context Used: Find ways to simplify the implementation (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/app/api/v1/ads/_post.ts
Line: 56-61
Comment:
**Vestigial `variant` field in responses**
Now that both the CLI hook and the server-side type contract have dropped the banner/choice distinction, `variant: 'choice'` in `noAdsResponse` (and in the success/error responses in `_post.ts`) is never read by any consumer. Dropping it would complete the simplification and keep the wire format consistent with the new `FetchAdResult` shape.
```suggestion
function noAdsResponse(provider: AdProviderId) {
return NextResponse.json(
{ ads: [], provider },
{ status: 200 },
)
}
```
**Context Used:** Find ways to simplify the implementation ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.
Summary
Simplifies the ad provider contract so Gravity and Carbon always return an ads array instead of a banner-or-choice union.
Updates the CLI ad hook, chat, and waiting room render paths to consume that array directly while preserving the choice banner behavior.
Normalizes no-fill and error responses from the ads API to the same array-based shape.
Validation