Conversation
Greptile SummaryAdds a Confidence Score: 5/5Safe to merge — fallback logic is correct, dependency array is properly updated, and all remaining comments are P2 style suggestions. The core retry loop correctly uses No files require special attention.
|
| Filename | Overview |
|---|---|
| cli/src/hooks/use-gravity-ad.ts | Adds fallbackProvider param; wraps single fetch in a for-loop over [provider, fallbackProvider]; adds both to the useEffect dependency array. Logic is correct — continue on HTTP error and implicit fall-through on no-fill both advance to the fallback provider. |
| cli/src/chat.tsx | Adds provider: 'gravity' and fallbackProvider: 'carbon' to the existing useGravityAd call; no other logic changes. |
| cli/src/components/waiting-room-screen.tsx | Switches waiting-room from Carbon-only (provider: 'carbon') to Gravity-first with Carbon fallback; comment updated to match. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: cli/src/hooks/use-gravity-ad.ts
Line: 284-287
Comment:
**Simplify `providersToTry` construction**
The dedup check (`fallbackProvider !== provider`) can be collapsed using `Set`, which also handles the `undefined` case more naturally without the extra boolean guard:
```suggestion
const providersToTry = [...new Set([provider, fallbackProvider].filter((p): p is AdProvider => p != null))]
```
This removes the nested ternary and makes the intent ("unique, defined providers in priority order") immediately clear.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: cli/src/hooks/use-gravity-ad.ts
Line: 309-319
Comment:
**Silent no-fill fallback makes debugging harder**
When the primary provider returns a 200 with no ad content (neither `data.ad` nor a valid `data.ads` array), the loop falls through to the fallback silently — no log line is emitted. Adding a brief `logger.debug` here would make it easy to spot "primary returned no fill, trying fallback" in logs without changing behaviour:
```suggestion
if (data.ad) {
return { variant: 'banner', ad: data.ad as AdResponse }
}
// No fill from this provider; try the next one if available
logger.debug({ provider: providerToTry }, '[ads] No fill, trying next provider')
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Add Carbon fallback for ads" | Re-trigger Greptile
| const providersToTry = | ||
| fallbackProvider && fallbackProvider !== provider | ||
| ? [provider, fallbackProvider] | ||
| : [provider] |
There was a problem hiding this comment.
Simplify
providersToTry construction
The dedup check (fallbackProvider !== provider) can be collapsed using Set, which also handles the undefined case more naturally without the extra boolean guard:
| const providersToTry = | |
| fallbackProvider && fallbackProvider !== provider | |
| ? [provider, fallbackProvider] | |
| : [provider] | |
| const providersToTry = [...new Set([provider, fallbackProvider].filter((p): p is AdProvider => p != null))] |
This removes the nested ternary and makes the intent ("unique, defined providers in priority order") immediately clear.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/hooks/use-gravity-ad.ts
Line: 284-287
Comment:
**Simplify `providersToTry` construction**
The dedup check (`fallbackProvider !== provider`) can be collapsed using `Set`, which also handles the `undefined` case more naturally without the extra boolean guard:
```suggestion
const providersToTry = [...new Set([provider, fallbackProvider].filter((p): p is AdProvider => p != null))]
```
This removes the nested ternary and makes the intent ("unique, defined providers in priority order") immediately clear.
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!
| if (!response.ok) { | ||
| logger.warn( | ||
| { | ||
| provider: providerToTry, | ||
| status: response.status, | ||
| response: await response.json(), | ||
| }, | ||
| '[ads] Web API returned error', | ||
| ) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Silent no-fill fallback makes debugging harder
When the primary provider returns a 200 with no ad content (neither data.ad nor a valid data.ads array), the loop falls through to the fallback silently — no log line is emitted. Adding a brief logger.debug here would make it easy to spot "primary returned no fill, trying fallback" in logs without changing behaviour:
| if (!response.ok) { | |
| logger.warn( | |
| { | |
| provider: providerToTry, | |
| status: response.status, | |
| response: await response.json(), | |
| }, | |
| '[ads] Web API returned error', | |
| ) | |
| continue | |
| } | |
| if (data.ad) { | |
| return { variant: 'banner', ad: data.ad as AdResponse } | |
| } | |
| // No fill from this provider; try the next one if available | |
| logger.debug({ provider: providerToTry }, '[ads] No fill, trying next provider') |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/hooks/use-gravity-ad.ts
Line: 309-319
Comment:
**Silent no-fill fallback makes debugging harder**
When the primary provider returns a 200 with no ad content (neither `data.ad` nor a valid `data.ads` array), the loop falls through to the fallback silently — no log line is emitted. Adding a brief `logger.debug` here would make it easy to spot "primary returned no fill, trying fallback" in logs without changing behaviour:
```suggestion
if (data.ad) {
return { variant: 'banner', ad: data.ad as AdResponse }
}
// No fill from this provider; try the next one if available
logger.debug({ provider: providerToTry }, '[ads] No fill, trying next provider')
```
How can I resolve this? If you propose a fix, please make it concise.
Summary
fallbackProvidersupport touseGravityAdso the CLI retries a secondary ad network when the primary returns no fill or errorsValidation
bun x tsc --noEmit -p cli/tsconfig.json