fix(onboard): skip unreachable Telegram during non-interactive setup#4307
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtracts Telegram reachability into a new module, wires it into onboarding (imports and deps), invokes the probe in non-interactive and interactive flows to skip/disable Telegram when appropriate, and adds Vitest tests for probe outcomes and prompts. ChangesTelegram Reachability Probe
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/onboard.ts (1)
5830-5861: Run targeted onboarding/messaging E2E for this behavior change.Given this touches core onboarding and Telegram channel gating logic, please run at least
cloud-e2e,sandbox-operations-e2e,channels-stop-start-e2e, andmessaging-compatible-endpoint-e2eon this branch before merge.As per coding guidelines, "
src/lib/onboard.ts: This file contains core onboarding logic. Changes here affect the full sandbox creation and configuration flow." and the listed E2E test recommendations apply.Also applies to: 5980-5986
🤖 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/lib/onboard.ts` around lines 5830 - 5861, Run the targeted E2E suites covering the onboarding/messaging changes introduced in setupMessagingChannels: execute cloud-e2e, sandbox-operations-e2e, channels-stop-start-e2e, and messaging-compatible-endpoint-e2e against this branch to validate the non-interactive messaging gating and Telegram reachability behavior (checkTelegramReachability, resolveMessagingChannelSeed, getValidatedMessagingToken/getValidatedMessagingTokenByEnvKey paths), and also run the same tests for the related changes around lines ~5980-5986 to ensure no regressions in sandbox creation and messaging channel startup/shutdown flows.
🤖 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/lib/onboard/telegram-reachability.test.ts`:
- Around line 98-110: The test sets a process.exit spy but may leak it if an
assertion throws; wrap the spy lifecycle in a try/finally (or move to afterEach)
so restore always runs: create the exitSpy via vi.spyOn(process, "exit") before
awaiting checkTelegramReachability("123:abc", deps) and put the assertions
inside a try block, then call exitSpy.mockRestore() in finally; reference
runCurlProbe, checkTelegramReachability, makeDeps and the exitSpy to locate the
change.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 5830-5861: Run the targeted E2E suites covering the
onboarding/messaging changes introduced in setupMessagingChannels: execute
cloud-e2e, sandbox-operations-e2e, channels-stop-start-e2e, and
messaging-compatible-endpoint-e2e against this branch to validate the
non-interactive messaging gating and Telegram reachability behavior
(checkTelegramReachability, resolveMessagingChannelSeed,
getValidatedMessagingToken/getValidatedMessagingTokenByEnvKey paths), and also
run the same tests for the related changes around lines ~5980-5986 to ensure no
regressions in sandbox creation and messaging channel startup/shutdown flows.
🪄 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: CHILL
Plan: Enterprise
Run ID: 1414f0d5-0f08-49c7-b08f-dbc84560767f
📒 Files selected for processing (3)
src/lib/onboard.tssrc/lib/onboard/telegram-reachability.test.tssrc/lib/onboard/telegram-reachability.ts
cjagwani
left a comment
There was a problem hiding this comment.
lgtm. closes #4238 with the right pattern:
- mirrors the Brave optional-integration warn-and-skip path (
web-search-flow.ts) — consistent with existing codebase convention - proper DI on
isNonInteractive/note/promptYesNoOrDefaultmakes the module testable without globals - net-negative on
onboard.ts(10+/61-), satisfies entrypoint budget - 401/404 token-rejected case also takes the skip path with rationale comment — correct: a rejected token can't function downstream either
- interactive default Y favors soft-fail (matches optional-integration contract)
- recovery hint in skip warning is brand-agnostic via
cliName() - test coverage hits OK, every curl code in TELEGRAM_NETWORK_CURL_CODES, 401, 404, env override, interactive prompt path
dismissed — posted without maintainer authorization
Summary
Fixes non-interactive onboarding so optional Telegram setup no longer aborts when Telegram validation fails. If
api.telegram.orgis unreachable or the bot token is rejected, onboarding now warns, disables Telegram for the run, and continues, matching the optional-component behavior.Related Issue
Fixes #4238
Changes
src/lib/onboard/telegram-reachability.ts.401/404token rejection into warn-and-skip results.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: rluo8 ruluo@nvidia.com
Summary by CodeRabbit
New Features
Tests
Refactor