Conversation
Greptile SummaryThis PR routes the Freebuff waiting room ad requests through a new Confidence Score: 5/5Safe to merge; only one minor style suggestion remains. All findings are P2. The single comment is a simplification of an intermediate variable in gravity.ts that doesn't affect correctness or runtime behavior. web/src/lib/ad-providers/gravity.ts — minor simplification opportunity in the placement/variant derivation block.
|
| Filename | Overview |
|---|---|
| web/src/lib/ad-providers/gravity.ts | Adds WAITING_ROOM_PLACEMENT_IDS and routes waiting_room surface requests to those placements; variant/placement derivation logic can be simplified by removing the intermediate requestedPlacementIds variable. |
| cli/src/hooks/use-gravity-ad.ts | Adds AdSurface type and surface option; correctly threads it into the fetch body and adds it to the useEffect dependency array. |
| web/src/app/api/v1/ads/_post.ts | Adds surfaceSchema validation and passes surface through to the ad provider; straightforward and correct. |
| web/src/lib/ad-providers/types.ts | Adds AdSurface type and optional surface field to FetchAdInput; clean addition. |
| cli/src/components/waiting-room-screen.tsx | Passes surface: 'waiting_room' to useGravityAd; minimal, correct one-liner change. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: web/src/lib/ad-providers/gravity.ts
Line: 114-128
Comment:
**Simplify placement/variant derivation**
The intermediate `requestedPlacementIds` variable is checked twice with `?.length`, and the `length === 1 ? 'banner' : 'choice'` branch will never be `'banner'` because `WAITING_ROOM_PLACEMENT_IDS` always has 4 entries. Checking `input.surface` directly removes the extra variable and the duplicate guards.
```suggestion
const variant =
input.surface === 'waiting_room' ? 'choice' : getGravityVariant(userId)
const filteredMessages = prepareGravityMessages(messages)
const placementIds =
input.surface === 'waiting_room'
? WAITING_ROOM_PLACEMENT_IDS
: variant === 'choice'
? CHOICE_PLACEMENT_IDS
: [BANNER_PLACEMENT_ID]
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Use waiting room Gravity placements" | Re-trigger Greptile
| const requestedPlacementIds = | ||
| input.surface === 'waiting_room' ? WAITING_ROOM_PLACEMENT_IDS : undefined | ||
| const variant = requestedPlacementIds?.length | ||
| ? requestedPlacementIds.length === 1 | ||
| ? 'banner' | ||
| : 'choice' | ||
| : getGravityVariant(userId) | ||
| const filteredMessages = prepareGravityMessages(messages) | ||
|
|
||
| const placements = | ||
| variant === 'choice' | ||
| ? CHOICE_PLACEMENT_IDS.map((id) => ({ | ||
| placement: 'below_response', | ||
| placement_id: id, | ||
| })) | ||
| : [{ placement: 'below_response', placement_id: BANNER_PLACEMENT_ID }] | ||
| const placementIds = requestedPlacementIds?.length | ||
| ? requestedPlacementIds | ||
| : variant === 'choice' | ||
| ? CHOICE_PLACEMENT_IDS | ||
| : [BANNER_PLACEMENT_ID] | ||
|
|
There was a problem hiding this comment.
Simplify placement/variant derivation
The intermediate requestedPlacementIds variable is checked twice with ?.length, and the length === 1 ? 'banner' : 'choice' branch will never be 'banner' because WAITING_ROOM_PLACEMENT_IDS always has 4 entries. Checking input.surface directly removes the extra variable and the duplicate guards.
| const requestedPlacementIds = | |
| input.surface === 'waiting_room' ? WAITING_ROOM_PLACEMENT_IDS : undefined | |
| const variant = requestedPlacementIds?.length | |
| ? requestedPlacementIds.length === 1 | |
| ? 'banner' | |
| : 'choice' | |
| : getGravityVariant(userId) | |
| const filteredMessages = prepareGravityMessages(messages) | |
| const placements = | |
| variant === 'choice' | |
| ? CHOICE_PLACEMENT_IDS.map((id) => ({ | |
| placement: 'below_response', | |
| placement_id: id, | |
| })) | |
| : [{ placement: 'below_response', placement_id: BANNER_PLACEMENT_ID }] | |
| const placementIds = requestedPlacementIds?.length | |
| ? requestedPlacementIds | |
| : variant === 'choice' | |
| ? CHOICE_PLACEMENT_IDS | |
| : [BANNER_PLACEMENT_ID] | |
| const variant = | |
| input.surface === 'waiting_room' ? 'choice' : getGravityVariant(userId) | |
| const filteredMessages = prepareGravityMessages(messages) | |
| const placementIds = | |
| input.surface === 'waiting_room' | |
| ? WAITING_ROOM_PLACEMENT_IDS | |
| : variant === 'choice' | |
| ? CHOICE_PLACEMENT_IDS | |
| : [BANNER_PLACEMENT_ID] |
Prompt To Fix With AI
This is a comment left during a code review.
Path: web/src/lib/ad-providers/gravity.ts
Line: 114-128
Comment:
**Simplify placement/variant derivation**
The intermediate `requestedPlacementIds` variable is checked twice with `?.length`, and the `length === 1 ? 'banner' : 'choice'` branch will never be `'banner'` because `WAITING_ROOM_PLACEMENT_IDS` always has 4 entries. Checking `input.surface` directly removes the extra variable and the duplicate guards.
```suggestion
const variant =
input.surface === 'waiting_room' ? 'choice' : getGravityVariant(userId)
const filteredMessages = prepareGravityMessages(messages)
const placementIds =
input.surface === 'waiting_room'
? WAITING_ROOM_PLACEMENT_IDS
: variant === 'choice'
? CHOICE_PLACEMENT_IDS
: [BANNER_PLACEMENT_ID]
```
How can I resolve this? If you propose a fix, please make it concise.
Summary
Route Freebuff waiting room ad requests through a new
waiting_roomads surface.Gravity maps that surface to
waiting-room-1throughwaiting-room-4, while existing chat ad placements keep their current banner/choice behavior.Validation
bun run --cwd cli typecheckbun run --cwd web typecheck