Add unit tests for auto-switch guard logic in create-org.vue#133
Merged
Conversation
- Implement tests to verify the behavior of the auto-switch feature. - Ensure redirection occurs only when the user has exactly 1 organization and no active organization. - Include tests for various scenarios including loading state, existing active organization, and different view modes. - Regression test for issue #131.
|
🚅 Deployed to the reqcore-pr-133 environment in applirank
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughRenamed live-demo runtime config key and updated sign-in autofill; onboarding auto-switch now respects an existing active org; added AI connection test endpoint and UI, plus related encryption/schema tests and parse-retry UI for document analysis; E2E fixture password is now env-configurable. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "Settings UI"
participant Browser as "Browser (with cookies)"
participant Server as "Server /api/ai-config/test-connection"
participant DB as "Database (aiConfig)"
participant AI as "AI Provider"
UI->>Browser: Click "Test connection"
Browser->>Server: POST /api/ai-config/test-connection (cookie headers)
Server->>Server: Rate limiter & permission checks
Server->>DB: query.aiConfig.findFirst(activeOrganizationId)
DB-->>Server: aiConfig (provider, model, apiKeyEncrypted, baseUrl)
Server->>AI: generateStructuredOutput(provider, model, apiKeyDecrypted, prompt)
AI-->>Server: { ok: boolean } or error
Server-->>Browser: { success: true } or HTTP 422 with message
Browser-->>UI: Render success/failure banner
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🧹 Nitpick comments (2)
e2e/fixtures.ts (1)
23-23: Unify E2E password source to avoid cross-spec auth failuresLine 23 introduces
E2E_TEST_PASSWORD, bute2e/critical-flows/resume-upload.spec.tsstill hardcodes'TestPassword123!'(see Lines 54/64 and usage at Lines 69/72 in the provided snippet). IfE2E_TEST_PASSWORDis set, those flows can fail login while fixture-based tests pass.Consider extracting a shared password resolver (or reusing
generateTestAccount) and updating that spec to consume it.Suggested direction
+// e2e/fixtures.ts +export const getE2eTestPassword = (): string => + process.env.E2E_TEST_PASSWORD || 'TestPassword123!' function generateTestAccount(workerId: number): TestAccount { const id = `${Date.now()}-${workerId}` return { name: `E2E Tester ${id}`, email: `e2e-${id}@test.local`, - password: process.env.E2E_TEST_PASSWORD || 'TestPassword123!', + password: getE2eTestPassword(), orgName: `E2E Org ${id}`, orgSlug: `e2e-org-${id}`, } }-// e2e/critical-flows/resume-upload.spec.ts -password: 'TestPassword123!', +password: getE2eTestPassword(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/fixtures.ts` at line 23, Create and export a single password resolver (e.g., getE2EPassword or reuse generateTestAccount's password logic) that returns process.env.E2E_TEST_PASSWORD || 'TestPassword123!', then replace the hardcoded 'TestPassword123!' occurrences in resume-upload.spec.ts with an import call to that resolver (or use generateTestAccount's returned password) so both fixture-based tests and the critical-flows spec use the same source of truth (update imports and references to call the new function instead of using the literal string).tests/unit/create-org-auto-switch.test.ts (1)
17-33: Avoid logic duplication between test helper and production guardLines 17–33 duplicate the guard logic instead of exercising a shared source function, so future drift can make tests pass while behavior regresses.
Refactor direction (shared guard)
+// app/pages/onboarding/create-org-auto-switch.guard.ts +export interface AutoSwitchContext { + loading: boolean + autoSwitched: boolean + viewMode: 'picker' | 'create' | 'join' + orgCount: number + hasActiveOrg: boolean +} + +export function shouldAutoSwitch(ctx: AutoSwitchContext): boolean { + if (ctx.loading || ctx.autoSwitched || ctx.viewMode !== 'picker') return false + return ctx.orgCount === 1 && !ctx.hasActiveOrg +}- function shouldAutoSwitch(ctx: AutoSwitchContext): boolean { - if (ctx.loading || ctx.autoSwitched || ctx.viewMode !== 'picker') return false - if (ctx.orgCount === 1 && !ctx.activeOrg) return true - return false - } + import { shouldAutoSwitch } from '~/app/pages/onboarding/create-org-auto-switch.guard'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/create-org-auto-switch.test.ts` around lines 17 - 33, The test duplicates the production guard logic in the helper shouldAutoSwitch, risking drift; refactor so the test imports and uses the single shared guard from the production code (the watcher guard in create-org.vue) instead of reimplementing it: export the guard function from the production module (preserve the same signature/shape as AutoSwitchContext), remove the duplicate shouldAutoSwitch implementation from tests, update the test to import that exported guard and call it with the existing test context, and ensure the test's AutoSwitchContext type aligns with the exported function's expected type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/pages/onboarding/create-org.vue`:
- Around line 39-41: The watcher on orgs/isOrgsLoading can fire before activeOrg
is settled causing premature auto-switch; update the guard in the watch([orgs,
isOrgsLoading], ...) callback to also check the active-org resolution flag (or
the composable's "activeOrg loading/resolved" state) before deciding to run
switchOrg — i.e., include the activeOrg-loading/resolved symbol (or a provided
isActiveOrgResolved/isHydrated flag) alongside orgs, isOrgsLoading,
autoSwitched, and viewMode in the conditional so switchOrg only runs when
activeOrg is fully settled.
---
Nitpick comments:
In `@e2e/fixtures.ts`:
- Line 23: Create and export a single password resolver (e.g., getE2EPassword or
reuse generateTestAccount's password logic) that returns
process.env.E2E_TEST_PASSWORD || 'TestPassword123!', then replace the hardcoded
'TestPassword123!' occurrences in resume-upload.spec.ts with an import call to
that resolver (or use generateTestAccount's returned password) so both
fixture-based tests and the critical-flows spec use the same source of truth
(update imports and references to call the new function instead of using the
literal string).
In `@tests/unit/create-org-auto-switch.test.ts`:
- Around line 17-33: The test duplicates the production guard logic in the
helper shouldAutoSwitch, risking drift; refactor so the test imports and uses
the single shared guard from the production code (the watcher guard in
create-org.vue) instead of reimplementing it: export the guard function from the
production module (preserve the same signature/shape as AutoSwitchContext),
remove the duplicate shouldAutoSwitch implementation from tests, update the test
to import that exported guard and call it with the existing test context, and
ensure the test's AutoSwitchContext type aligns with the exported function's
expected type.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74ce201b-9f54-4e94-bb19-18e00ab447d2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
app/pages/auth/sign-in.vueapp/pages/onboarding/create-org.vuee2e/fixtures.tsnuxt.config.tstests/unit/create-org-auto-switch.test.ts
Comment on lines
39
to
+41
| watch([orgs, isOrgsLoading], async ([orgList, loading]) => { | ||
| if (loading || autoSwitched.value || viewMode.value !== 'picker') return | ||
| if (orgList.length === 1) { | ||
| if (orgList.length === 1 && !activeOrg.value) { |
There was a problem hiding this comment.
Auto-switch can run before active-org state is fully settled
On Line 39, the watcher only reacts to orgs and isOrgsLoading. If activeOrg is still unset during the immediate run, Line 41 can still pass and trigger switchOrg, even for users who already have an active org once auth state finishes hydrating.
Suggested hardening
- watch([orgs, isOrgsLoading], async ([orgList, loading]) => {
- if (loading || autoSwitched.value || viewMode.value !== 'picker') return
- if (orgList.length === 1 && !activeOrg.value) {
+ watch([orgs, isOrgsLoading, activeOrg], async ([orgList, loading, currentActiveOrg]) => {
+ if (loading || autoSwitched.value || viewMode.value !== 'picker') return
+ if (orgList.length === 1 && !currentActiveOrg) {
const firstOrg = orgList[0]
if (!firstOrg) return
autoSwitched.value = true
isLoading.value = true
try {
await switchOrg(firstOrg.id)
}
catch {
isLoading.value = false
autoSwitched.value = false
}
}
}, { immediate: true })If your composable exposes an explicit “active org loading/resolved” flag, include it in this guard as well.
📝 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.
Suggested change
| watch([orgs, isOrgsLoading], async ([orgList, loading]) => { | |
| if (loading || autoSwitched.value || viewMode.value !== 'picker') return | |
| if (orgList.length === 1) { | |
| if (orgList.length === 1 && !activeOrg.value) { | |
| watch([orgs, isOrgsLoading, activeOrg], async ([orgList, loading, currentActiveOrg]) => { | |
| if (loading || autoSwitched.value || viewMode.value !== 'picker') return | |
| if (orgList.length === 1 && !currentActiveOrg) { | |
| const firstOrg = orgList[0] | |
| if (!firstOrg) return | |
| autoSwitched.value = true | |
| isLoading.value = true | |
| try { | |
| await switchOrg(firstOrg.id) | |
| } | |
| catch { | |
| isLoading.value = false | |
| autoSwitched.value = false | |
| } | |
| } | |
| }, { immediate: true }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/pages/onboarding/create-org.vue` around lines 39 - 41, The watcher on
orgs/isOrgsLoading can fire before activeOrg is settled causing premature
auto-switch; update the guard in the watch([orgs, isOrgsLoading], ...) callback
to also check the active-org resolution flag (or the composable's "activeOrg
loading/resolved" state) before deciding to run switchOrg — i.e., include the
activeOrg-loading/resolved symbol (or a provided isActiveOrgResolved/isHydrated
flag) alongside orgs, isOrgsLoading, autoSwitched, and viewMode in the
conditional so switchOrg only runs when activeOrg is fully settled.
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 `@server/api/ai-config/test-connection.post.ts`:
- Line 21: The permission scope for the cost-incurring test endpoint is too
permissive: change the requirePermission call in
server/api/ai-config/test-connection.post.ts to require the narrower
scoring:create scope instead of scoring: ['read']; specifically update the call
to requirePermission(event, { scoring: ['create'] }) so only users allowed to
create/run provider tests can invoke the billed provider call (adjust any
related checks or error messages in the same function if present).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b813fc7-4fdd-44d6-9792-abbe553f3f2f
📒 Files selected for processing (5)
app/pages/dashboard/jobs/new.vueapp/pages/dashboard/settings/ai.vueserver/api/ai-config/test-connection.post.tstests/unit/ai-config-encryption.test.tstests/unit/ai-config-schema.test.ts
| */ | ||
| export default defineEventHandler(async (event) => { | ||
| await limiter(event) | ||
| const session = await requirePermission(event, { scoring: ['read'] }) |
There was a problem hiding this comment.
Tighten permission scope for this cost-incurring endpoint.
Line 21 currently allows scoring: ['read'], but this route triggers a real provider call. That is broader than the settings UI guard (scoring: ['create'] in app/pages/dashboard/settings/ai.vue Line 14) and allows unauthorized users to invoke billable tests.
🔧 Proposed fix
- const session = await requirePermission(event, { scoring: ['read'] })
+ const session = await requirePermission(event, { scoring: ['create'] })📝 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.
Suggested change
| const session = await requirePermission(event, { scoring: ['read'] }) | |
| const session = await requirePermission(event, { scoring: ['create'] }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/api/ai-config/test-connection.post.ts` at line 21, The permission
scope for the cost-incurring test endpoint is too permissive: change the
requirePermission call in server/api/ai-config/test-connection.post.ts to
require the narrower scoring:create scope instead of scoring: ['read'];
specifically update the call to requirePermission(event, { scoring: ['create']
}) so only users allowed to create/run provider tests can invoke the billed
provider call (adjust any related checks or error messages in the same function
if present).
…g in candidate analysis
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
6 tasks
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
1 participant
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
Type of change
Validation
DCO
Signed-off-by) viagit commit -sSummary by CodeRabbit
New Features
Bug Fixes
Chores / Tests