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>
📝 WalkthroughWalkthroughRefactors password policy test suite to compute policy hashes via RPC call Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 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)
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.
| }) | ||
|
|
||
| describe('password Policy Configuration via SDK', () => { | ||
| describe('Password Policy Configuration via SDK', () => { |
There was a problem hiding this comment.
describe title must begin with a lowercase letter.
ESLint rule test/prefer-lowercase-title flags this. Same issue on line 463.
Proposed fix
-describe('Password Policy Configuration via SDK', () => {
+describe('password Policy Configuration via SDK', () => {Line 463:
-describe('Password Policy Enforcement Integration', () => {
+describe('password Policy Enforcement Integration', () => {📝 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.
| describe('Password Policy Configuration via SDK', () => { | |
| describe('password Policy Configuration via SDK', () => { |
🧰 Tools
🪛 ESLint
[error] 54-54: describes should begin with lowercase
(test/prefer-lowercase-title)
🤖 Prompt for AI Agents
In `@tests/password-policy.test.ts` at 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.
| // 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.
Pipeline type error: Json | undefined is not assignable to Json.
org?.password_policy_config can be undefined when org is null, but the RPC parameter policy_config expects Json. The CI reports TS2322 on this line.
Proposed fix
- const { data: rpcResult } = await getSupabaseClient().rpc('get_password_policy_hash', {
- policy_config: org?.password_policy_config,
- })
+ const policyConfig = org?.password_policy_config
+ if (!policyConfig) {
+ throw new Error('Expected org to have password_policy_config')
+ }
+ const { data: rpcResult } = await getSupabaseClient().rpc('get_password_policy_hash', {
+ policy_config: policyConfig,
+ })📝 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.
| // 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' | |
| // Use the same RPC that production uses to compute the password policy hash | |
| const policyConfig = org?.password_policy_config | |
| if (!policyConfig) { | |
| throw new Error('Expected org to have password_policy_config') | |
| } | |
| const { data: rpcResult } = await getSupabaseClient().rpc('get_password_policy_hash', { | |
| policy_config: policyConfig, | |
| }) | |
| const policyHash = (rpcResult as string | null) ?? 'test_hash' |
🧰 Tools
🪛 GitHub Actions: Run tests
[error] 557-557: vue-tsc --noEmit: TS2322: Type 'Json | undefined' is not assignable to type 'Json'.
🤖 Prompt for AI Agents
In `@tests/password-policy.test.ts` around lines 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.
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).
| // 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 |
|
This PR applies 5/5 suggestions from code quality AI findings.
Summary by CodeRabbit