[codex] Show ad title when ad URL is missing#546
Conversation
Greptile SummaryThis PR fixes a blank secondary label in the choice ad banner when Carbon ads omit the display URL by falling back to the ad title. The implementation is correct and well-tested. Confidence Score: 5/5Safe to merge — logic is correct, tests cover both branches, and remaining findings are minor style improvements. All findings are P2: a redundant guard in No files require special attention.
|
| Filename | Overview |
|---|---|
| cli/src/components/choice-ad-banner.tsx | Adds getAdDisplayLabel helper and truncateToWidth to fall back to ad title when URL is absent; logic is correct but truncateToWidth has a redundant guard and extractDomain is unnecessarily exported. |
| cli/src/components/tests/choice-ad-banner.test.tsx | New unit tests for getAdDisplayLabel covering the URL-present and URL-absent branches; coverage is appropriate for the introduced logic. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: cli/src/components/choice-ad-banner.tsx
Line: 28-33
Comment:
**Redundant guard and duplicated truncation logic**
The `width === 1` branch on line 31 is dead code — the final `return` already handles it correctly: `text.slice(0, 1 - 1) + '…'` = `'…'`. The branch can be removed. Additionally, `truncateToWidth` is essentially `truncateToLines(text, width, 1)` with a corrected `width ≤ 0` edge case (returning `''` instead of the full text). Consider consolidating into one utility or at least removing the redundant guard:
```suggestion
function truncateToWidth(text: string, width: number): string {
if (width <= 0) return ''
if (text.length <= width) return text
return text.slice(0, width - 1) + '…'
}
```
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/components/choice-ad-banner.tsx
Line: 35
Comment:
**Unnecessary export on `extractDomain`**
`extractDomain` is not imported by the test file (only `getAdDisplayLabel` is), so the `export` added here is unneeded. Keeping it private prevents unintended coupling to callers outside this module.
```suggestion
const extractDomain = (url: string): string => {
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Show ad title without URL" | Re-trigger Greptile
| return text.slice(0, width - 1) + '…' | ||
| } | ||
|
|
||
| export const extractDomain = (url: string): string => { |
There was a problem hiding this comment.
Unnecessary export on
extractDomain
extractDomain is not imported by the test file (only getAdDisplayLabel is), so the export added here is unneeded. Keeping it private prevents unintended coupling to callers outside this module.
| export const extractDomain = (url: string): string => { | |
| const extractDomain = (url: string): string => { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/components/choice-ad-banner.tsx
Line: 35
Comment:
**Unnecessary export on `extractDomain`**
`extractDomain` is not imported by the test file (only `getAdDisplayLabel` is), so the `export` added here is unneeded. Keeping it private prevents unintended coupling to callers outside this module.
```suggestion
const extractDomain = (url: string): string => {
```
How can I resolve this? If you propose a fix, please make it concise.Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Summary
Why
Carbon ads intentionally omit the display URL because their click URL is a tracking redirect. That left the banner's secondary label blank. Falling back to the title preserves useful advertiser context without showing the tracking host.
Validation
bun test cli/src/components/__tests__/choice-ad-banner.test.tsxbun run --cwd cli typecheck