fix(onboard): respect messaging channel toggle in sandbox creation#1508
Merged
fix(onboard): respect messaging channel toggle in sandbox creation#1508
Conversation
setupMessagingChannels() now returns the selected channel names so createSandbox() can filter messagingTokenDefs to only include channels the user actually enabled. Previously, any channel with a saved token was configured regardless of the toggle picker state. Closes #1505 Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughcreateSandbox now accepts an optional enabledChannels parameter; setupMessagingChannels returns the user-selected channel names and is exported. The onboarding flow awaits channel selection and passes enabledChannels into createSandbox, which filters messaging token definitions so only selected channels are configured. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OnboardScript as Onboard
participant CredStore as CredentialStore
participant SandboxManager as Sandbox/Providers
User->>Onboard: run onboarding
Onboard->>Onboard: setupMessagingChannels() (interactive or non-interactive)
Onboard-->>User: present channel toggles / detect env tokens
User-->>Onboard: selectedChannels (array)
Onboard->>Onboard: build enabledTokenEnvKeys from selectedChannels
Onboard->>CredStore: query credential store for messaging tokens (filtered by enabledTokenEnvKeys)
CredStore-->>Onboard: available tokens (only enabled channels)
Onboard->>SandboxManager: createSandbox(..., enabledChannels) with filtered providers
SandboxManager-->>Onboard: provider creation results
Onboard-->>User: onboarding completes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/onboard.test.js (1)
1264-1273: Add a behavior-level regression test for channel deselection.This regex check confirms parameter wiring, but it won’t catch regressions where
createSandboxignoresenabledChannels. Please add one integration assertion that a stored token for a deselected channel (e.g., Slack) does not produce aprovider create ... slack-bridgecommand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.js` around lines 1264 - 1273, The test currently checks parameter wiring with a regex but doesn't assert behavior when channels are deselected; add an integration assertion that verifies createSandbox actually respects enabledChannels by storing a token for a deselected channel (e.g., Slack) and asserting that the resulting command output or recorded provider actions does NOT include a "provider create ... slack-bridge" invocation. Update the test around the createSandbox/startRecordedStep checks (look for createSandbox and enabledChannels usage) to simulate a deselected channel token and assert absence of any provider create slack-bridge call in the produced commands or logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/onboard.test.js`:
- Around line 1264-1273: The test currently checks parameter wiring with a regex
but doesn't assert behavior when channels are deselected; add an integration
assertion that verifies createSandbox actually respects enabledChannels by
storing a token for a deselected channel (e.g., Slack) and asserting that the
resulting command output or recorded provider actions does NOT include a
"provider create ... slack-bridge" invocation. Update the test around the
createSandbox/startRecordedStep checks (look for createSandbox and
enabledChannels usage) to simulate a deselected channel token and assert absence
of any provider create slack-bridge call in the produced commands or logs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cd911baa-674a-4375-a6f8-d906fbe37146
📒 Files selected for processing (2)
bin/lib/onboard.jstest/onboard.test.js
…eractive return Verify that createSandbox respects the enabledChannels parameter: - only selected channels get providers created - empty enabledChannels creates no messaging providers - setupMessagingChannels returns correct channel list in non-interactive mode - setupMessagingChannels returns empty array when no tokens are set Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/lib/onboard.js (1)
2531-2556:⚠️ Potential issue | 🟠 MajorApply
enabledChannelsto all downstream messaging state, not just provider tokens.This filter only trims
messagingTokenDefs.messagingAllowedIdsandsetupPoliciesWithSelection()still infer channels from saved env/credentials, and a ready sandbox can be reused without knowing it was built with a broader channel set. A user who deselects Slack/Telegram can therefore still carry that channel’s config forward. Please persist the selected set and use it for downstream config and recreate decisions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 2531 - 2556, The current code only applies enabledChannels via enabledEnvKeys to messagingTokenDefs, but messagingAllowedIds and setupPoliciesWithSelection still derive channels from existing env/credentials; update the flow so the selected channel set is persisted and used everywhere: compute enabledEnvKeys from enabledChannels (as already done) and then apply that same Set to filter messagingAllowedIds and to short-circuit or parameterize setupPoliciesWithSelection (use the enabledEnvKeys or enabledChannels as an explicit argument) so recreate decisions and downstream config only consider the persisted selection; also store the selected channels on the sandbox metadata/state so future runs read that persisted selection instead of inferring from saved credentials (references: enabledChannels, enabledEnvKeys, MESSAGING_CHANNELS, messagingTokenDefs, messagingAllowedIds, setupPoliciesWithSelection).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@bin/lib/onboard.js`:
- Around line 2531-2556: The current code only applies enabledChannels via
enabledEnvKeys to messagingTokenDefs, but messagingAllowedIds and
setupPoliciesWithSelection still derive channels from existing env/credentials;
update the flow so the selected channel set is persisted and used everywhere:
compute enabledEnvKeys from enabledChannels (as already done) and then apply
that same Set to filter messagingAllowedIds and to short-circuit or parameterize
setupPoliciesWithSelection (use the enabledEnvKeys or enabledChannels as an
explicit argument) so recreate decisions and downstream config only consider the
persisted selection; also store the selected channels on the sandbox
metadata/state so future runs read that persisted selection instead of inferring
from saved credentials (references: enabledChannels, enabledEnvKeys,
MESSAGING_CHANNELS, messagingTokenDefs, messagingAllowedIds,
setupPoliciesWithSelection).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4a931019-3c48-4ce4-b11e-ef87bc7ee521
📒 Files selected for processing (2)
bin/lib/onboard.jstest/onboard.test.js
Gate messagingAllowedIds on the filtered messagingTokenDefs so user IDs for deselected channels are not written into the sandbox Dockerfile. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
cv
approved these changes
Apr 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
setupMessagingChannels()now returns the selected channel namescreateSandbox()acceptsenabledChannelsparam and filtersmessagingTokenDefsto only include user-selected channelsenabledChannelsis null (backward compat / direct calls), all channels with tokens are includedCloses #1505
Test plan
slack-bridgeprovider is created during sandbox setupSummary by CodeRabbit
New Features
Improvements
Tests