Conversation
…ofix Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
…ofix Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
…ofix Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
…ofix Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
…ofix Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
📝 WalkthroughWalkthroughTests updated to use the production RPC Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/password-policy.test.ts (2)
1-1:⚠️ Potential issue | 🔴 CriticalRemove unused
PostgrestErrorimport — pipeline failure.The CI pipeline reports
TS6133: 'PostgrestError' is declared but its value is never read.This will block the build.Proposed fix
-import type { PostgrestError } from '@supabase/supabase-js'
296-303:⚠️ Potential issue | 🟠 Major
throwinsidefinallyoverwrites any exception from thetryblock.If the test assertion in the
tryblock fails and the restore also fails, the original test failure is silently swallowed. Both ESLint (no-unsafe-finally) and Biome flag this.Consider logging or combining errors instead of a bare
throw:Proposed fix
finally { const { error } = await getSupabaseClient() .from('orgs') .update({ password_policy_config: policyConfig }) .eq('id', ORG_ID) - if (error) - throw error + if (error) + console.error('Failed to restore password policy after test:', error) }
🤖 Fix all issues with AI agents
In `@tests/password-policy.test.ts`:
- Around line 555-560: The type error comes from passing
org?.password_policy_config (which can be undefined) to
getSupabaseClient().rpc('get_password_policy_hash', { policy_config }) where
policy_config expects Json; fix by ensuring a Json is passed — e.g. replace
policy_config: org?.password_policy_config with policy_config:
(org?.password_policy_config ?? null) as unknown as Json (or import and use the
Json type and cast appropriately) so the rpc call receives a Json value.
- Line 54: The describe block titles start with an uppercase letter and violate
the test/prefer-lowercase-title rule; update the string arguments passed to the
describe calls (the one currently "Password Policy Configuration via SDK" and
the other at the later describe) so they start with a lowercase letter (e.g.,
"password policy configuration via SDK" and the similar lowercase variant for
the second describe) to satisfy the linter.
There was a problem hiding this comment.
Pull request overview
This PR addresses code-quality findings by tightening and simplifying the password-policy test suite to better match production behavior and improve assertion strictness.
Changes:
- Normalizes
describe(...)titles capitalization for consistency. - Simplifies the “temporarily disable policy / restore policy” logic during
/private/validate_password_compliancetesting. - Makes tests use the production RPC (
get_password_policy_hash) for computing the password policy hash and tightens an invalid-JSON status assertion.
| try { | ||
| const response = await fetch(`${BASE_URL}/private/validate_password_compliance`, { |
There was a problem hiding this comment.
PostgrestError is no longer referenced in this test after removing restoreError, so the type import at the top of the file is now unused and may fail lint/TS checks. Remove the unused import (or reintroduce a typed variable if still needed).
| finally { | ||
| const { error } = await getSupabaseClient() | ||
| .from('orgs') | ||
| .update({ password_policy_config: policyConfig }) | ||
| .eq('id', ORG_ID) | ||
| restoreError = error ?? null | ||
| if (error) | ||
| throw error | ||
| } |
There was a problem hiding this comment.
Throwing inside this finally will override any failure thrown in the try block (e.g., assertion failures), which can mask the real test failure and make debugging harder. Consider capturing/propagating the original error and only failing the test due to restore failure when the main body succeeded (or log restore failure without replacing the original exception).
tests/password-policy.test.ts
Outdated
| // Use the same RPC that production uses to compute the password policy hash | ||
| const { data: rpcResult } = await getSupabaseClient().rpc('get_password_policy_hash', { | ||
| policy_config: org?.password_policy_config, | ||
| }) | ||
|
|
||
| const policyHash = (rpcResult as string | null) ?? 'test_hash' |
There was a problem hiding this comment.
This RPC call ignores the returned error and falls back to 'test_hash' when rpcResult is null, which can make the test pass even if get_password_policy_hash is broken/permissioned incorrectly or org.password_policy_config is unexpectedly null. To ensure the test is actually validating the production hash path, assert error is null and that the RPC returns a non-null string (and consider failing if the org has no policy config rather than using a dummy hash).
|
@copilot test are filing fix them |
* Initial plan * fix: address code review feedback on password policy tests - Remove unused PostgrestError import - Fix finally block to preserve test errors - Add proper error validation for RPC call Co-authored-by: riderx <4084527+riderx@users.noreply.github.com> * fix: lint --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: riderx <4084527+riderx@users.noreply.github.com> Co-authored-by: Martin DONADIEU <martindonadieu@gmail.com>
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)
tests/password-policy.test.ts (1)
614-634:⚠️ Potential issue | 🟡 MinorSilently falling back to empty string masks RPC failures.
Line 630 uses
hashResult || ''— if the RPC fails or returnsnull, the test will upsert an emptypolicy_hashand potentially pass for the wrong reason. Add an assertion (as done in the first test at lines 578–580) to fail fast.🛡️ Proposed fix
const { data: hashResult } = await getSupabaseClient().rpc('get_password_policy_hash', { policy_config: { enabled: true, min_length: 10, require_uppercase: true, require_number: true, require_special: true, }, }) + expect(hashResult).not.toBeNull() + // Insert compliance record with correct hash await getSupabaseClient() .from('user_password_compliance') .upsert({ user_id: USER_ID, org_id: ORG_ID, - policy_hash: hashResult || '', + policy_hash: hashResult as string, validated_at: new Date().toISOString(), }, { onConflict: 'user_id,org_id', })
🧹 Nitpick comments (2)
tests/password-policy.test.ts (2)
296-315: Linter violations:throwinsidefinallyblock.Both ESLint (
no-unsafe-finally) and Biome (noUnsafeFinally) flag lines 308, 309, and 314. While the intent is sound (preserve the test error while also surfacing restore failures), you can restructure to avoidthrowinfinallyentirely by moving the re-throw logic after thefinallyblock completes.♻️ Proposed restructure to eliminate throw-in-finally
- catch (error) { - testError = error as Error - } - finally { - const { error: restoreError } = await getSupabaseClient() - .from('orgs') - .update({ password_policy_config: policyConfig }) - .eq('id', ORG_ID) - - // If restore failed, throw it (but preserve test failure if there was one) - if (restoreError) { - if (testError) - throw new Error(`Test failed AND restore failed: ${testError.message} | Restore error: ${restoreError.message}`) - throw restoreError - } - - // Re-throw original test error if any - if (testError) - throw testError + catch (error) { + testError = error as Error + } + finally { + const { error: restoreError } = await getSupabaseClient() + .from('orgs') + .update({ password_policy_config: policyConfig }) + .eq('id', ORG_ID) + + if (restoreError) { + testError = testError + ? new Error(`Test failed AND restore failed: ${testError.message} | Restore error: ${restoreError.message}`) + : restoreError + } + } + + if (testError) + throw testError
54-54: Tests useit()instead ofit.concurrent().Per the coding guidelines for
tests/**/*.test.ts, tests should useit.concurrent()to run in parallel within the same file. Allit(...)calls in this file use the synchronous variant. This is a pre-existing issue but worth noting since severalitblocks were touched in this PR.As per coding guidelines: "Use
it.concurrent()instead ofit()to run tests in parallel within the same test file".Also applies to: 82-82, 109-109, 151-151, 166-166, 181-181, 196-196, 212-212, 228-228, 245-245, 263-263, 318-318, 334-334, 351-351, 416-416, 427-427, 454-454, 522-522, 538-538, 559-559, 598-598, 646-646, 671-671
|
This PR applies 5/5 suggestions from code quality AI findings.
Summary by CodeRabbit