Repository navigation
fix(onboard): persist enabledChannels across resume path - #1703
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a nullable Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant Onboard as "onboard()"
participant Session as "onboardSession"
participant Policies as "setupPoliciesWithSelection()"
participant Creds as "Credential Store / env"
User->>Onboard: start onboarding
Onboard->>User: messaging channels step (select or skip)
User->>Onboard: select channels or press Enter to skip
Onboard->>Session: updateSession({ messagingChannels })
Session-->>Session: normalize & persist messagingChannels
Onboard->>Session: resumeSandbox? load session
Session-->>Onboard: return session (messagingChannels or null)
Onboard->>Policies: setupPoliciesWithSelection(enabledChannels: selectedMessagingChannels)
alt messagingChannels provided
Policies->>Policies: derive presets from messagingChannels
else messagingChannels is null/empty
Policies->>Creds: probe stored credentials / env
Creds-->>Policies: detected channels
end
Policies-->>Onboard: return policy presets
Onboard->>User: display policy presets
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/onboard.ts`:
- Around line 4454-4458: setupMessagingChannels() returns the user's toggles,
not the channels that were actually configured with credentials, so persist only
token-backed channels: after calling setupMessagingChannels() in the onboarding
flow, filter the returned enabledChannels to keep only channels with a
configured provider/credentials (e.g., where a provider token or
provider.isConfigured is present) before calling onboardSession.updateSession
and before passing to setupPoliciesWithSelection/createSandbox; update the code
around setupMessagingChannels(), onboardSession.updateSession, and any
downstream use (setupPoliciesWithSelection/createSandbox) to use this
filteredEnabledChannels list instead of the raw toggled list.
🪄 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: Pro
Run ID: b49201ac-2e97-40de-bc81-3377d3b0d99c
📒 Files selected for processing (2)
src/lib/onboard-session.tssrc/lib/onboard.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard-session.test.ts (1)
233-241: Add explicit empty-array regression (enabledChannels: []).Please add a test for
enabledChannels: []to lock in the “user skipped messaging channels” path and prevent future fallback-to-credentials regressions.Proposed test addition
it("filterSafeUpdates preserves enabledChannels field", () => { session.saveSession(session.createSession()); session.markStepComplete("provider_selection", { enabledChannels: ["slack", "discord"], }); const loaded = session.loadSession(); expect(loaded.enabledChannels).toEqual(["slack", "discord"]); }); + it("preserves explicit empty enabledChannels selection", () => { + session.saveSession(session.createSession()); + session.markStepComplete("provider_selection", { + enabledChannels: [], + }); + + const loaded = session.loadSession(); + expect(loaded.enabledChannels).toEqual([]); + }); + it("createSession with enabledChannels override", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard-session.test.ts` around lines 233 - 241, Add a new unit test alongside the existing "filterSafeUpdates preserves enabledChannels field" test that specifically verifies an explicit empty array is preserved: call session.saveSession(session.createSession()), then session.markStepComplete("provider_selection", { enabledChannels: [] }), then load the session via session.loadSession() and assert loaded.enabledChannels equals [] to lock in the "user skipped messaging channels" path; reference the same helpers used in the file (session.createSession, session.saveSession, session.markStepComplete, session.loadSession) so the regression for fallback-to-credentials is prevented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/onboard-session.test.ts`:
- Around line 233-241: Add a new unit test alongside the existing
"filterSafeUpdates preserves enabledChannels field" test that specifically
verifies an explicit empty array is preserved: call
session.saveSession(session.createSession()), then
session.markStepComplete("provider_selection", { enabledChannels: [] }), then
load the session via session.loadSession() and assert loaded.enabledChannels
equals [] to lock in the "user skipped messaging channels" path; reference the
same helpers used in the file (session.createSession, session.saveSession,
session.markStepComplete, session.loadSession) so the regression for
fallback-to-credentials is prevented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ce8fe9a-7840-468e-b5dd-791750597403
📒 Files selected for processing (1)
src/lib/onboard-session.test.ts
20625da to
776426d
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/onboard.ts (1)
4510-4514:⚠️ Potential issue | 🟡 MinorPersist only token-backed channels, not raw toggles.
Line 4510 currently persists the raw selection from
setupMessagingChannels(). If a user toggles a channel but skips token entry, that channel is still persisted and later suggested in policy presets, even though it won’t be configured in sandbox providers.💡 Suggested fix
- enabledChannels = await setupMessagingChannels(); + const selectedChannels = await setupMessagingChannels(); + enabledChannels = selectedChannels.filter((name) => { + const channel = MESSAGING_CHANNELS.find((entry) => entry.name === name); + return channel + ? Boolean( + getCredential(channel.envKey) || + normalizeCredentialValue(process.env[channel.envKey]), + ) + : false; + }); onboardSession.updateSession((current) => { current.enabledChannels = enabledChannels; return current; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 4510 - 4514, The code currently writes the raw result of setupMessagingChannels() into onboardSession via onboardSession.updateSession, which persists toggles even when the user didn't provide provider tokens; change this to persist only token-backed channels by filtering the enabledChannels returned from setupMessagingChannels() to include only channels with valid provider tokens (e.g., where a token field or a requiresToken check is satisfied) before calling onboardSession.updateSession; update the logic around enabledChannels, setupMessagingChannels, and the onboardSession.updateSession call so only token-backed channel entries are saved and later suggested in policy presets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/onboard.ts`:
- Around line 4510-4514: The code currently writes the raw result of
setupMessagingChannels() into onboardSession via onboardSession.updateSession,
which persists toggles even when the user didn't provide provider tokens; change
this to persist only token-backed channels by filtering the enabledChannels
returned from setupMessagingChannels() to include only channels with valid
provider tokens (e.g., where a token field or a requiresToken check is
satisfied) before calling onboardSession.updateSession; update the logic around
enabledChannels, setupMessagingChannels, and the onboardSession.updateSession
call so only token-backed channel entries are saved and later suggested in
policy presets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 95f5ae98-cf15-4a25-916a-cd6eb0beeb7e
📒 Files selected for processing (3)
src/lib/onboard-session.test.tssrc/lib/onboard-session.tssrc/lib/onboard.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/onboard-session.test.ts
- src/lib/onboard-session.ts
|
Reopening — this was closed in error. This PR is on branch This PR addresses a remaining gap in the enabledChannels fix: the resume path doesn't restore Changes:
Sorry for the confusion on the close. |
776426d to
b0c68fa
Compare
|
Rebased on latest main and updated the approach to align with upstream conventions:
Signed commit (GPG). |
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Code is clean — follows the existing policyPresets pattern, tests cover round-trip, filterSafeUpdates, and resume restore. No regressions. One blocker: DCO check is failing — PR description needs a sign-off line. @kagura-agent can you add Signed-off-by: kagura-agent <kagura.chen28@gmail.com> to the PR body?
2ead797 to
459d0cf
Compare
459d0cf to
6c0f440
Compare
|
Rebased on latest main and added DCO sign-off. Conflict resolved (kept both the new Brave Search resume note and the messagingChannels restore). |
6c0f440 to
6d3bc06
Compare
|
Hi @prekshivyas, thanks for the approval! Just checking — is there anything else needed before this can be merged? |
|
Hi @prekshivyas 👋 Thanks for the approval! Just checking if there's anything else needed before this gets merged, or if it's in the merge queue. Happy to rebase if needed. |
|
Friendly ping — this has been approved. Is there anything else needed for merge? Happy to rebase if needed. |
When resuming an onboard session, selectedMessagingChannels was lost because it was only set inside the non-resume code path. Resumed sessions had no channel selection, causing setupPoliciesWithSelection to fall back to credential-only checks. Changes: - Restore selectedMessagingChannels from session.messagingChannels on the resume path - Add messagingChannels field to Session interface and serialization - Add filterSafeUpdates support for messagingChannels - Add regression tests for messagingChannels round-tripping Signed-off-by: kagura-agent <kagura.chen28@gmail.com>
6d3bc06 to
c2ee378
Compare
|
Rebased on latest main and added DCO sign-off. The conflict was because upstream already incorporated the |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Looks good to me.
- restores
selectedMessagingChannelson the resume path - keeps the functional change narrow to the missing state restoration
- adds regression coverage for
messagingChannelsround-tripping and safe-update preservation
I also ran local targeted validation on the PR branch:
npm run build:clinpx vitest run src/lib/onboard-session.test.tsnpm run typecheck:cli
wscurran
added
area: cli
5 participants
Problem
When resuming an onboard session,
enabledChannelswas lost because it was declared asconstinside theelse(non-resume) block of the sandbox setup step. Resumed sessions had no channel selection, causingsetupPoliciesWithSelectionto fall back to credential-only checks — users who selected messaging channels (Telegram/Slack/Discord) would not see the corresponding policy presets suggested after resume.Root Cause
In
src/lib/onboard.ts, line ~4443:This was scoped inside the else block. The resume path (
if (resumeSandbox)) never setenabledChannels, and it was never persisted to session state.Fix
enabledChannelsto function scope (let enabledChannels: string[] | null = null)session.enabledChannelsonboardSession.updateSession()onboard-session.ts): addenabledChannelsto Session/SessionUpdates interfaces, createSession, normalizeSession, filterSafeUpdates, and summarizeForDebugenabledChannelstosetupPoliciesWithSelection— when provided, use the channel list instead of credential checks (with credential fallback for backward compat)Testing
npm run check(lint + format + tsc): ✅ all passFixes #1610
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests
Signed-off-by: kagura-agent kagura.chen28@gmail.com