Feat/ads sponsored tips#1674
Conversation
- Gitlawb earning tips now appear on a per-turn cadence (default every 2nd tip slot, OPENCLAUDE_ADS_TIP_EVERY-tunable) for opt-in users, bypassing the per-startup sponsored gate that only ever showed one ad per session. - Each shown earning tip fetches a real impression and confirms after the dwell, crediting opengateway credits; fails silent so ads never break the CLI. - Mark /ads isSensitive so the earn code is redacted from history.
/ads on always opens a masked TextInput dialog (mask="*") — the code is never typed inline, because the terminal echoes inline args as you type (isSensitive only redacts history after submission, not the live echo). An inline /ads on <code> now also opens the dialog and warns that the typed code is exposed and should be rotated. Converts /ads to a local-jsx command.
📝 WalkthroughWalkthroughAdds a sponsored earning-tips system. ChangesSponsored Ads Earning System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 6
🤖 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/commands/ads.test.ts`:
- Around line 7-10: Test files are mutating process environment variables
without restoring them, causing state leakage between test suites. In
src/commands/ads.test.ts at lines 7-10, add an afterEach hook that restores the
original value of process.env.ADS_BASE_URL (save it before the beforeEach
modifies it). Similarly, in src/services/tips/gitlawbEarn.test.ts at lines
14-20, add an afterEach hook that restores both process.env.ADS_BASE_URL and
process.env.OPENCLAUDE_ADS_TIP_EVERY to their original values (capture them
before any test setup modifies them). This ensures each test suite cleans up
after itself and prevents cross-suite contamination.
- Around line 30-43: The current tests for the "on" and "on <code>" commands in
ads.test.ts only verify that a dialog is rendered but do not test the core user
interaction paths. Add two new focused test cases: one that simulates the user
submitting the dialog (onSubmit) and verifies that ads become enabled and the
code is persisted in the global config, and another that simulates the user
canceling the dialog (onCancel) and verifies that a cancellation message is
displayed. These tests should follow the same pattern as the existing test
function (using the run helper) but should test the completion behavior rather
than just the dialog rendering.
In `@src/commands/ads.tsx`:
- Around line 118-121: When disabling ads with the condition where sub ===
'off', the saveGlobalConfig call only sets ads.enabled to false but leaves the
earnCode credential persisted in the ads object. Modify the saveGlobalConfig
invocation to also clear the earnCode property from the ads object when
disabling ads, ensuring that no credentials remain stored after the user opts
out of the feature. This should be done alongside the existing enabled: false
assignment in the ads object spread operation.
In `@src/services/ads.ts`:
- Around line 54-58: The fetchWithProxyRetry calls at lines 54-58 and 85-93 lack
timeout/deadline parameters, which can cause stalled connections to hang
indefinitely. Since fetchNextTip is awaited in the spinner-tip path, this
violates the requirement that ads must never block. Add a timeout parameter to
both fetchWithProxyRetry invocations to enforce a hard deadline and prevent
promises from hanging. Ensure the timeout value is reasonable for network
operations while preventing prolonged blocking.
In `@src/services/tips/gitlawbEarn.ts`:
- Around line 90-92: The setTimeout call that schedules the confirmTip timer can
keep short-lived CLI processes alive while waiting for the delay to complete. To
fix this, store the return value of setTimeout in a variable and call unref() on
it to detach the timer from the process lifetime, allowing the process to exit
without waiting for this best-effort confirmation timer to fire. This change
should be made to the setTimeout call around line 90 that calls confirmTip(code,
tip.token).
In `@src/services/tips/tipScheduler.ts`:
- Around line 58-63: Add an integration test in tipScheduler.test.ts that
verifies the new earning-tip early-return branch in getTipToShowOnSpinner
executes correctly. The test should mock shouldShowEarningTip to return true and
verify that getTipToShowOnSpinner calls buildEarningTip and returns an earning
tip, ensuring the new precedence path is properly covered by the scheduler's
integration tests rather than relying only on unit tests in gitlawbEarn.test.ts.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 40c86d22-9a71-4a26-a957-175c87de2c91
📒 Files selected for processing (8)
src/commands.tssrc/commands/ads.test.tssrc/commands/ads.tsxsrc/services/ads.tssrc/services/tips/gitlawbEarn.test.tssrc/services/tips/gitlawbEarn.tssrc/services/tips/tipScheduler.tssrc/utils/config.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise in code
Files:
src/services/tips/tipScheduler.tssrc/commands/ads.tsxsrc/utils/config.tssrc/commands/ads.test.tssrc/services/ads.tssrc/services/tips/gitlawbEarn.test.tssrc/services/tips/gitlawbEarn.tssrc/commands.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/services/tips/tipScheduler.tssrc/commands/ads.tsxsrc/utils/config.tssrc/commands/ads.test.tssrc/services/ads.tssrc/services/tips/gitlawbEarn.test.tssrc/services/tips/gitlawbEarn.tssrc/commands.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/services/tips/tipScheduler.tssrc/commands/ads.tsxsrc/utils/config.tssrc/commands/ads.test.tssrc/services/ads.tssrc/services/tips/gitlawbEarn.test.tssrc/services/tips/gitlawbEarn.tssrc/commands.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/commands/ads.test.tssrc/services/tips/gitlawbEarn.test.ts
🔇 Additional comments (1)
src/utils/config.ts (1)
422-427: LGTM!
| beforeEach(() => { | ||
| process.env.ADS_BASE_URL = 'http://127.0.0.1:0' | ||
| saveGlobalConfig(c => ({ ...c, ads: undefined })) | ||
| }) |
There was a problem hiding this comment.
Shared root cause: test files mutate process env without restoring it, which can leak state into unrelated suites.
src/commands/ads.test.ts#L7-L10: snapshot and restoreprocess.env.ADS_BASE_URLinafterEach/afterAll.src/services/tips/gitlawbEarn.test.ts#L14-L20: snapshot and restore bothADS_BASE_URLandOPENCLAUDE_ADS_TIP_EVERYafter each test (or suite) to prevent cross-suite contamination.
As per coding guidelines, “Review tests for … isolation of global/env/config state.”
📍 Affects 2 files
src/commands/ads.test.ts#L7-L10(this comment)src/services/tips/gitlawbEarn.test.ts#L14-L20
🤖 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/commands/ads.test.ts` around lines 7 - 10, Test files are mutating
process environment variables without restoring them, causing state leakage
between test suites. In src/commands/ads.test.ts at lines 7-10, add an afterEach
hook that restores the original value of process.env.ADS_BASE_URL (save it
before the beforeEach modifies it). Similarly, in
src/services/tips/gitlawbEarn.test.ts at lines 14-20, add an afterEach hook that
restores both process.env.ADS_BASE_URL and process.env.OPENCLAUDE_ADS_TIP_EVERY
to their original values (capture them before any test setup modifies them).
This ensures each test suite cleans up after itself and prevents cross-suite
contamination.
Source: Coding guidelines
| test('"on" returns the masked dialog and does not enable yet', async () => { | ||
| const { node, text } = await run('on') | ||
| expect(node).toBeTruthy() // renders AdsCodeDialog | ||
| expect(text).toBeUndefined() // resolves only after the user submits | ||
| expect(getGlobalConfig().ads?.enabled).toBeFalsy() | ||
| }) | ||
|
|
||
| test('"on <code>" never enables inline — it also opens the masked dialog', async () => { | ||
| const { node, text } = await run('on earn_typed_inline') | ||
| expect(node).toBeTruthy() | ||
| expect(text).toBeUndefined() | ||
| // The inline code is ignored; nothing is persisted from the command line. | ||
| expect(getGlobalConfig().ads?.enabled).toBeFalsy() | ||
| }) |
There was a problem hiding this comment.
Add focused coverage for completing /ads on (submit/cancel).
These cases only assert that a dialog node exists. They don’t verify the user-visible completion behavior (onSubmit enabling + persisted code, onCancel cancellation message), which is the core changed path.
As per coding guidelines, “Review tests for meaningful coverage of the changed behavior … Block when risky runtime changes lack focused regression coverage.”
🤖 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/commands/ads.test.ts` around lines 30 - 43, The current tests for the
"on" and "on <code>" commands in ads.test.ts only verify that a dialog is
rendered but do not test the core user interaction paths. Add two new focused
test cases: one that simulates the user submitting the dialog (onSubmit) and
verifies that ads become enabled and the code is persisted in the global config,
and another that simulates the user canceling the dialog (onCancel) and verifies
that a cancellation message is displayed. These tests should follow the same
pattern as the existing test function (using the run helper) but should test the
completion behavior rather than just the dialog rendering.
Source: Coding guidelines
| if (sub === 'off') { | ||
| const wasOn = getGlobalConfig().ads?.enabled | ||
| saveGlobalConfig(c => ({ ...c, ads: { ...(c.ads ?? {}), enabled: false } })) | ||
| onDone(wasOn ? 'Sponsored tips disabled.' : 'Sponsored tips are already off.', { |
There was a problem hiding this comment.
Clear persisted earnCode on /ads off.
/ads off disables the feature but keeps ads.earnCode stored at rest. That leaves a credential persisted after opt-out with no user-facing benefit.
Suggested fix
if (sub === 'off') {
const wasOn = getGlobalConfig().ads?.enabled
- saveGlobalConfig(c => ({ ...c, ads: { ...(c.ads ?? {}), enabled: false } }))
+ saveGlobalConfig(c => {
+ const { earnCode: _removed, ...restAds } = c.ads ?? {}
+ return { ...c, ads: { ...restAds, enabled: false } }
+ })
onDone(wasOn ? 'Sponsored tips disabled.' : 'Sponsored tips are already off.', {
display: 'system',
})🤖 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/commands/ads.tsx` around lines 118 - 121, When disabling ads with the
condition where sub === 'off', the saveGlobalConfig call only sets ads.enabled
to false but leaves the earnCode credential persisted in the ads object. Modify
the saveGlobalConfig invocation to also clear the earnCode property from the ads
object when disabling ads, ensuring that no credentials remain stored after the
user opts out of the feature. This should be done alongside the existing
enabled: false assignment in the ads object spread operation.
| const resp = await fetchWithProxyRetry( | ||
| url, | ||
| { method: 'GET', headers: COMMON_HEADERS(earnCode) }, | ||
| { maxAttempts: 2 }, | ||
| ) |
There was a problem hiding this comment.
Add hard timeouts to ads network calls.
Line 54 and Line 85 issue outbound requests with retries but no deadline. A stalled connection can hang these promises; fetchNextTip is awaited in the spinner-tip path, so this can violate the “ads must never block” behavior.
Suggested fix
+const ADS_REQUEST_TIMEOUT_MS = 5_000
+
+function makeTimeoutSignal(ms: number): { signal: AbortSignal; cancel: () => void } {
+ const c = new AbortController()
+ const t = setTimeout(() => c.abort(), ms)
+ return { signal: c.signal, cancel: () => clearTimeout(t) }
+}
+
export async function fetchNextTip(
earnCode: string,
surface = 'openclaude',
): Promise<SponsoredTip | null> {
try {
const url = `${adsBaseUrl()}/api/ads/next?surface=${encodeURIComponent(surface)}`
+ const { signal, cancel } = makeTimeoutSignal(ADS_REQUEST_TIMEOUT_MS)
+ let resp
+ try {
+ resp = await fetchWithProxyRetry(
+ url,
+ { method: 'GET', headers: COMMON_HEADERS(earnCode), signal },
+ { maxAttempts: 2 },
+ )
+ } finally {
+ cancel()
+ }
- const resp = await fetchWithProxyRetry(
- url,
- { method: 'GET', headers: COMMON_HEADERS(earnCode) },
- { maxAttempts: 2 },
- )
if (!resp.ok) return null
const data = (await resp.json()) as Record<string, unknown>
@@
export async function confirmTip(
earnCode: string,
token: string,
): Promise<ConfirmResult> {
- const resp = await fetchWithProxyRetry(
- `${adsBaseUrl()}/api/ads/confirm`,
- {
- method: 'POST',
- headers: COMMON_HEADERS(earnCode),
- body: JSON.stringify({ token }),
- },
- { maxAttempts: 2 },
- )
+ const { signal, cancel } = makeTimeoutSignal(ADS_REQUEST_TIMEOUT_MS)
+ let resp
+ try {
+ resp = await fetchWithProxyRetry(
+ `${adsBaseUrl()}/api/ads/confirm`,
+ {
+ method: 'POST',
+ headers: COMMON_HEADERS(earnCode),
+ body: JSON.stringify({ token }),
+ signal,
+ },
+ { maxAttempts: 2 },
+ )
+ } finally {
+ cancel()
+ }
const data = (await resp.json().catch(() => ({}))) as Record<string, unknown>Also applies to: 85-93
🤖 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/services/ads.ts` around lines 54 - 58, The fetchWithProxyRetry calls at
lines 54-58 and 85-93 lack timeout/deadline parameters, which can cause stalled
connections to hang indefinitely. Since fetchNextTip is awaited in the
spinner-tip path, this violates the requirement that ads must never block. Add a
timeout parameter to both fetchWithProxyRetry invocations to enforce a hard
deadline and prevent promises from hanging. Ensure the timeout value is
reasonable for network operations while preventing prolonged blocking.
| setTimeout(() => { | ||
| void confirmTip(code, tip.token).catch(() => {}) | ||
| }, delay) |
There was a problem hiding this comment.
Detach confirm timer from process lifetime.
Line 90 schedules a best-effort confirm timer that can keep short-lived CLI runs alive for up to 30s. This should not hold process exit.
Suggested fix
- setTimeout(() => {
+ const timer = setTimeout(() => {
void confirmTip(code, tip.token).catch(() => {})
}, delay)
+ timer.unref?.()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setTimeout(() => { | |
| void confirmTip(code, tip.token).catch(() => {}) | |
| }, delay) | |
| const timer = setTimeout(() => { | |
| void confirmTip(code, tip.token).catch(() => {}) | |
| }, delay) | |
| timer.unref?.() |
🤖 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/services/tips/gitlawbEarn.ts` around lines 90 - 92, The setTimeout call
that schedules the confirmTip timer can keep short-lived CLI processes alive
while waiting for the delay to complete. To fix this, store the return value of
setTimeout in a variable and call unref() on it to detach the timer from the
process lifetime, allowing the process to exit without waiting for this
best-effort confirmation timer to fire. This change should be made to the
setTimeout call around line 90 that calls confirmTip(code, tip.token).
| // Opt-in earning users (`/ads on <code>`) see Gitlawb sponsored tips on a | ||
| // per-turn cadence, bypassing the per-startup sponsored gate — they opted in | ||
| // to earn, so we surface (and credit) ads frequently rather than once a session. | ||
| if (shouldShowEarningTip()) { | ||
| return buildEarningTip() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether tip scheduler tests explicitly cover the earning branch behavior.
rg -n --type=ts -C2 'getTipToShowOnSpinner|shouldShowEarningTip|buildEarningTip|gitlawb-earn|spinnerTipsEnabled' src/services/tipsRepository: Gitlawb/openclaude
Length of output: 9596
🏁 Script executed:
cat -n src/services/tips/tipScheduler.test.tsRepository: Gitlawb/openclaude
Length of output: 6619
Add integration test for earning-tip branch precedence in getTipToShowOnSpinner.
The new early-return path (lines 58–63) changes tip selection precedence but has no focused integration test in tipScheduler.test.ts. Unit tests for earning functions exist in gitlawbEarn.test.ts, but the scheduler's integration with shouldShowEarningTip() and buildEarningTip() is not covered. Add a test that verifies the earning branch executes and returns an earning tip when enabled.
🤖 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/services/tips/tipScheduler.ts` around lines 58 - 63, Add an integration
test in tipScheduler.test.ts that verifies the new earning-tip early-return
branch in getTipToShowOnSpinner executes correctly. The test should mock
shouldShowEarningTip to return true and verify that getTipToShowOnSpinner calls
buildEarningTip and returns an earning tip, ensuring the new precedence path is
properly covered by the scheduler's integration tests rather than relying only
on unit tests in gitlawbEarn.test.ts.
Source: Coding guidelines
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Do not confirm an impression that was never rendered
src/commands/ads.tsx:33
Enabling/adsimmediately callswarmOneEarn, which fetches a sponsored tip, waits up to eight seconds, and then callsconfirmTipwithout ever rendering that tip in the spinner path. That credits an impression just for opting in, even though the PR text and the ads client contract both say credits happen when a tip is shown after dwell. Please remove this warm-confirm path, or make the warm path actually display the impression before confirming it. -
[P2] Complete CodeRabbit's request to bound ad fetch latency
src/services/ads.ts:54
CodeRabbit requested a hard deadline on bothfetchWithProxyRetrycalls, and the current patch still awaitsfetchNextTipfrombuildEarningTip().content()before the spinner can render.fetchWithProxyRetrydoes not add a timeout itself, so a stalled ads endpoint or proxy can hang a loading tip despite the feature promising that ads never block the CLI. Please add a bounded abort/timeout around both thenextandconfirmrequests. -
[P2] Complete CodeRabbit's request to remove the earn code on opt-out
src/commands/ads.tsx:120
/ads offonly flipsenabledto false and leavesads.earnCodepersisted in the global config. Since the command treats the earn code as a credential, opting out should stop retaining it locally unless the user explicitly re-enters it later. Please clear the stored code when disabling sponsored tips. -
[P2] Complete CodeRabbit's request to detach the confirm timer
src/services/tips/gitlawbEarn.ts:90
The dwell confirmation timer is best-effort background work, but the returned timer is still ref'ed. Every shown earning tip can therefore keep an otherwise-finished CLI process alive for up to 30 seconds solely to settle an ad confirmation. Pleaseunref()this timer after scheduling it so sponsored tips do not extend process lifetime. -
[P3] Complete CodeRabbit's test-isolation and coverage requests
src/commands/ads.test.ts:7
The new tests mutateprocess.env.ADS_BASE_URLandOPENCLAUDE_ADS_TIP_EVERYwithout restoring their previous values, and the/ads ontests only assert that a dialog was returned rather than exercising submit/cancel or the scheduler early-return path. Please complete those CodeRabbit requests so these tests do not leak state into neighboring suites and so the credential-submit behavior is covered directly.
2 participants
Summary
per-turn earning that fetches a sponsored tip and credits opengateway credits when one is shown during loading; a small ads service client (fetchNextTip/confirmTip).
via a masked field and never accepted inline.
Impact
The earn code is entered through a masked dialog (shown as ********); inline /ads on
is refused (opens the dialog + warns to rotate, since terminals echo inline args). Ads always fail silent — they neverbreak or block the CLI.
behavior is unchanged). /ads converted from local → local-jsx. GlobalConfig gains an optional ads field. New env knobs: OPENCLAUDE_ADS_TIP_EVERY, ADS_BASE_URL (defaults to https://ads.gitlawb.com).
Testing
Notes
dwell → confirm → credit landed in the shared ledger, idempotent on retry).
attention-tied approach is the server-side Tier-2 model (gateway grants on real inference), planned separately.
Summary by CodeRabbit
New Features
/adscommand to enable/disable sponsored tips functionalityTests