#12 Issue 3 — No test suite and no test step in CI#13
Conversation
|
@smith961 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughAdds Vitest as the test runner with jsdom environment, React plugin, and path alias configuration. Introduces ChangesTest Suite, Validation, and CI Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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 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 Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test-stubs/acta-sdk/index.ts (1)
1-6: 📐 Maintainability & Code Quality | 🟡 MinorRemove the duplicate
src/test-stubs/acta-sdk.tsfile.The file is an identical copy of
src/test-stubs/acta-sdk/index.ts. Sincepackage.jsonresolves@acta-team/acta-sdkto the directoryfile:src/test-stubs/acta-sdk, Node's module resolution will always loadindex.ts, leavingacta-sdk.tsunreferenced and redundant. Removing it eliminates unnecessary duplication and confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test-stubs/acta-sdk/index.ts` around lines 1 - 6, The file acta-sdk.ts in src/test-stubs is a duplicate of acta-sdk/index.ts, and since package.json resolves the module to the acta-sdk directory, Node's module resolution will always load index.ts making acta-sdk.ts redundant. Delete the duplicate acta-sdk.ts file from src/test-stubs to remove this unnecessary duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr-checks.yml:
- Around line 52-60: Replace the floating action tags with pinned commit SHAs
for both the checkout and setup-node actions to ensure immutability. For the
actions/checkout@v4 step, add a persist-credentials field and set it to false to
prevent credential persistence. This addresses the security concerns by pinning
exact action versions and disabling automatic credential caching in the checkout
step.
In `@package.json`:
- Line 17: The `@acta-team/acta-sdk` dependency in package.json is incorrectly
pointing to the test stub file path in the dependencies section, causing
production code in ActaConfigProvider, use-vault-setup, use-vault-read,
use-issuer-authorize, and use-credential-form to resolve the stub instead of the
real SDK. Restore the real SDK package version to the dependencies section of
package.json, then configure the test stub as a Vitest alias in the Vitest
configuration file so the stub is only used during test resolution and does not
affect runtime code.
In `@src/components/modules/credential/hooks/use-credential-form.test.ts`:
- Around line 36-71: Add a test case to the useCredentialForm describe block
that verifies the DID validation guard behavior. Create a new test that renders
the hook, sets the subjectDid state to an invalid value, calls handleCreate
within an act block, and then assert that a toast error is displayed and
issueMock is not called. This will ensure the validation predicate within the
hook prevents credential issuance when the DID is invalid.
In `@src/providers/wallet.provider.tsx`:
- Around line 68-75: The eslint-disable-next-line directive on line 68
references the deprecated rule `@typescript-eslint/no-var-requires`, but the
project's ESLint configuration uses the modern
`@typescript-eslint/no-require-imports` rule instead. Update the eslint-disable
comment that precedes the require statement for stellar-wallets-kit imports to
replace `@typescript-eslint/no-var-requires` with
`@typescript-eslint/no-require-imports` to properly suppress the correct rule.
---
Outside diff comments:
In `@src/test-stubs/acta-sdk/index.ts`:
- Around line 1-6: The file acta-sdk.ts in src/test-stubs is a duplicate of
acta-sdk/index.ts, and since package.json resolves the module to the acta-sdk
directory, Node's module resolution will always load index.ts making acta-sdk.ts
redundant. Delete the duplicate acta-sdk.ts file from src/test-stubs to remove
this unnecessary duplication.
🪄 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: 2b8e50ef-fef8-4f8f-a40a-ac8186d9c0cd
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
.github/workflows/pr-checks.yml.husky/pre-pushpackage.jsonsrc/components/modules/credential/hooks/use-credential-form.test.tssrc/components/modules/credential/hooks/use-credential-form.tssrc/lib/validation.test.tssrc/lib/validation.tssrc/providers/did.provider.test.tssrc/providers/did.provider.tsxsrc/providers/wallet.provider.tsxsrc/test-stubs/acta-sdk.tssrc/test-stubs/acta-sdk/index.tssrc/test-stubs/acta-sdk/package.jsonvitest.config.tsvitest.setup.ts
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '20' | ||
| cache: 'npm' | ||
| cache-dependency-path: package-lock.json |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin action SHAs and disable checkout credential persistence in the new job.
On Lines 53 and 56, floating tags (@v4) are not immutable, and checkout defaults to persisted credentials. Pin both actions to commit SHAs and set persist-credentials: false for the checkout step.
Suggested hardening patch
test:
name: Test
runs-on: ubuntu-latest
steps:
- name: Checkout code
- uses: actions/checkout@v4
+ uses: actions/checkout@<PINNED_SHA>
+ with:
+ persist-credentials: false
- name: Setup Node.js
- uses: actions/setup-node@v4
+ uses: actions/setup-node@<PINNED_SHA>
with:
node-version: '20'
cache: 'npm'
cache-dependency-path: package-lock.json🧰 Tools
🪛 zizmor (1.26.1)
[warning] 52-53: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 53-53: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 56-56: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/pr-checks.yml around lines 52 - 60, Replace the floating
action tags with pinned commit SHAs for both the checkout and setup-node actions
to ensure immutability. For the actions/checkout@v4 step, add a
persist-credentials field and set it to false to prevent credential persistence.
This addresses the security concerns by pinning exact action versions and
disabling automatic credential caching in the checkout step.
Source: Linters/SAST tools
| }, | ||
| "dependencies": { | ||
| "@acta-team/acta-sdk": "^1.0.7", | ||
| "@acta-team/acta-sdk": "file:src/test-stubs/acta-sdk", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether `@acta-team/acta-sdk` is used by runtime (non-test) code.
fd -t f -e ts -e tsx src | xargs rg -n --no-heading "`@acta-team/acta-sdk`" \
| rg -v "\.test\.(ts|tsx)$"Repository: ACTA-Team/demo-ACTA
Length of output: 1565
Restore the real SDK to dependencies and scope the test stub to test resolution only.
The @acta-team/acta-sdk dependency at line 17 currently points to file:src/test-stubs/acta-sdk in dependencies, meaning runtime code will resolve the stub. This affects multiple production components:
src/components/config/ActaConfigProvider.tsxsrc/components/modules/vault/hooks/use-vault-setup.tssrc/components/modules/vault/hooks/use-vault-read.tssrc/components/modules/issuer/hooks/use-issuer-authorize.tssrc/components/modules/credential/hooks/use-credential-form.ts
Move the real SDK back to dependencies and configure the test stub as a Vitest alias or mock instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 17, The `@acta-team/acta-sdk` dependency in
package.json is incorrectly pointing to the test stub file path in the
dependencies section, causing production code in ActaConfigProvider,
use-vault-setup, use-vault-read, use-issuer-authorize, and use-credential-form
to resolve the stub instead of the real SDK. Restore the real SDK package
version to the dependencies section of package.json, then configure the test
stub as a Vitest alias in the Vitest configuration file so the stub is only used
during test resolution and does not affect runtime code.
| describe('useCredentialForm', () => { | ||
| it('short-circuits when required fields are empty', async () => { | ||
| const { result } = renderHook(() => useCredentialForm()); | ||
|
|
||
| await act(async () => { | ||
| await result.current.handleCreate(); | ||
| }); | ||
|
|
||
| expect(result.current.state.txId).toBeNull(); | ||
| expect(issueMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('issues a credential when the form is valid', async () => { | ||
| const { result } = renderHook(() => useCredentialForm()); | ||
|
|
||
| await act(async () => { | ||
| result.current.updateField('issuerName', 'Example University'); | ||
| result.current.updateField('degreeType', 'ExampleBachelorDegree'); | ||
| result.current.updateField('degreeName', 'Bachelor of Science and Arts'); | ||
| }); | ||
|
|
||
| await act(async () => { | ||
| await result.current.handleCreate(); | ||
| }); | ||
|
|
||
| expect(issueMock).toHaveBeenCalledOnce(); | ||
| const issuedPayload = issueMock.mock.calls[0][0]; | ||
| expect(issuedPayload).toMatchObject({ | ||
| owner: validStellarAddress, | ||
| issuer: validStellarAddress, | ||
| issuerDid: validSubjectDid, | ||
| signTransaction: signTransactionMock, | ||
| }); | ||
| expect(result.current.state.txId).toBe('mockTxId'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚖️ Poor tradeoff
Add a test case for the DID validation guard (lines 50–53 of the hook).
The test suite covers the required-field short-circuit and the valid-submission happy path, but does not exercise the DID validation gate. If state.subjectDid is set to an invalid value, the hook should show a toast error and not call issueMock.
While the validation predicate itself is tested in validation.test.ts, this integration test should verify the guard's behavior within the credential creation flow.
✅ Proposed test case for invalid DID rejection
it('issues a credential when the form is valid', async () => {
const { result } = renderHook(() => useCredentialForm());
await act(async () => {
result.current.updateField('issuerName', 'Example University');
result.current.updateField('degreeType', 'ExampleBachelorDegree');
result.current.updateField('degreeName', 'Bachelor of Science and Arts');
});
await act(async () => {
await result.current.handleCreate();
});
expect(issueMock).toHaveBeenCalledOnce();
const issuedPayload = issueMock.mock.calls[0][0];
expect(issuedPayload).toMatchObject({
owner: validStellarAddress,
issuer: validStellarAddress,
issuerDid: validSubjectDid,
signTransaction: signTransactionMock,
});
expect(result.current.state.txId).toBe('mockTxId');
});
+
+ it('rejects credential creation with an invalid subject DID', async () => {
+ const { result } = renderHook(() => useCredentialForm());
+
+ await act(async () => {
+ result.current.updateField('issuerName', 'Example University');
+ result.current.updateField('subjectDid', 'invalid-did-format');
+ result.current.updateField('degreeType', 'ExampleBachelorDegree');
+ result.current.updateField('degreeName', 'Bachelor of Science and Arts');
+ });
+
+ await act(async () => {
+ await result.current.handleCreate();
+ });
+
+ expect(issueMock).not.toHaveBeenCalled();
+ expect(result.current.state.txId).toBeNull();
+ });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/modules/credential/hooks/use-credential-form.test.ts` around
lines 36 - 71, Add a test case to the useCredentialForm describe block that
verifies the DID validation guard behavior. Create a new test that renders the
hook, sets the subjectDid state to an invalid value, calls handleCreate within
an act block, and then assert that a toast error is displayed and issueMock is
not called. This will ensure the validation predicate within the hook prevents
credential issuance when the DID is invalid.
| // eslint-disable-next-line @typescript-eslint/no-var-requires, global-require | ||
| const { | ||
| StellarWalletsKit, | ||
| WalletNetwork, | ||
| FreighterModule, | ||
| AlbedoModule, | ||
| xBullModule, | ||
| } = require('@creit.tech/stellar-wallets-kit'); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which require-related ESLint rules are configured and where.
fd -HI --glob 'eslint.config.*' --glob '.eslintrc*' . | xargs rg -n --no-heading "no-require-imports|no-var-requires"
rg -n --no-heading "eslint-disable-next-line.*no-var-requires|require\('`@creit`\.tech/stellar-wallets-kit'\)" src/providers/wallet.provider.tsxRepository: ACTA-Team/demo-ACTA
Length of output: 359
🏁 Script executed:
#!/bin/bash
# Search for ESLint configuration files with corrected approach
find . -maxdepth 3 -name '.eslintrc*' -o -name 'eslint.config.*' 2>/dev/null | head -20
# Also check package.json for eslintConfig
if [ -f "package.json" ]; then
echo "=== Checking package.json for eslintConfig ==="
jq '.eslintConfig' package.json 2>/dev/null || echo "No eslintConfig in package.json"
fi
# Check the exact context around lines 68-75
echo "=== Context around lines 68-75 in wallet.provider.tsx ==="
sed -n '60,80p' src/providers/wallet.provider.tsxRepository: ACTA-Team/demo-ACTA
Length of output: 1938
🌐 Web query:
TypeScript ESLint @typescript-eslint/no-var-requiresvs@typescript-eslint/no-require-imports rule difference
💡 Result:
The rule @typescript-eslint/no-var-requires has been deprecated in favor of @typescript-eslint/no-require-imports [1][2][3]. The primary differences and relationship between the two are as follows: 1. Deprecation Status: @typescript-eslint/no-var-requires is deprecated and users are advised to switch to @typescript-eslint/no-require-imports [1][3]. 2. Scope and Functionality: @typescript-eslint/no-var-requires was historically more limited in scope, focusing specifically on variable declarations using require [4][1]. In contrast, @typescript-eslint/no-require-imports is the comprehensive rule designed to cover all require expressions, including both bare require calls and TypeScript import = require declarations [4][5]. 3. Configuration and Parity: The functionality of the deprecated rule is fully encompassed by the newer rule [4]. By enabling the allowAsImport: true option in @typescript-eslint/no-require-imports, you can replicate the behavior where TypeScript's import = require syntax is permitted while still flagging standard require calls [4][5]. In summary, @typescript-eslint/no-require-imports is the modern, superseding rule that provides a unified way to enforce ES Module syntax over CommonJS require statements [5].
Citations:
- 1: https://typescript-eslint.io/rules/no-var-requires/
- 2: chore(eslint-plugin): deprecate no-var-requires in favor of no-require-imports typescript-eslint/typescript-eslint#8334
- 3: https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/src/rules/no-var-requires.ts
- 4: Repo: deprecate no-var-requires in favor of no-require-imports typescript-eslint/typescript-eslint#8092
- 5: https://typescript-eslint.io/rules/no-require-imports/
🏁 Script executed:
#!/bin/bash
# Check for root-level ESLint config files more thoroughly
ls -la | grep -E "eslint|\.rc"
# Check if there's a config in other common locations
if [ -f ".eslintrc.json" ]; then
echo "=== .eslintrc.json ===" && cat .eslintrc.json
fi
if [ -f ".eslintrc.js" ]; then
echo "=== .eslintrc.js ===" && cat .eslintrc.js
fi
if [ -f ".eslintrc.cjs" ]; then
echo "=== .eslintrc.cjs ===" && cat .eslintrc.cjs
fi
if [ -f ".eslintrc.yml" ]; then
echo "=== .eslintrc.yml ===" && cat .eslintrc.yml
fi
if [ -f "eslint.config.js" ]; then
echo "=== eslint.config.js ===" && cat eslint.config.js
fi
# Also check the project structure to understand the setup
echo "=== Root level files ==="
ls -la *.json *.js *.cjs 2>/dev/null | head -20Repository: ACTA-Team/demo-ACTA
Length of output: 518
🏁 Script executed:
#!/bin/bash
# Read the ESLint config
echo "=== eslint.config.mjs ===" && cat eslint.config.mjsRepository: ACTA-Team/demo-ACTA
Length of output: 1100
Update ESLint suppress directive to target the correct rule.
Line 68 disables @typescript-eslint/no-var-requires, which is deprecated. The project's ESLint config extends next/typescript, which enforces the modern @typescript-eslint/no-require-imports rule instead. Update the directive to suppress the correct rule:
- // eslint-disable-next-line `@typescript-eslint/no-var-requires`, global-require
+ // eslint-disable-next-line `@typescript-eslint/no-require-imports`, global-require📝 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.
| // eslint-disable-next-line @typescript-eslint/no-var-requires, global-require | |
| const { | |
| StellarWalletsKit, | |
| WalletNetwork, | |
| FreighterModule, | |
| AlbedoModule, | |
| xBullModule, | |
| } = require('@creit.tech/stellar-wallets-kit'); | |
| // eslint-disable-next-line `@typescript-eslint/no-require-imports`, global-require | |
| const { | |
| StellarWalletsKit, | |
| WalletNetwork, | |
| FreighterModule, | |
| AlbedoModule, | |
| xBullModule, | |
| } = require('`@creit.tech/stellar-wallets-kit`'); |
🧰 Tools
🪛 ESLint
[error] 75-75: A require() style import is forbidden.
(@typescript-eslint/no-require-imports)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/providers/wallet.provider.tsx` around lines 68 - 75, The
eslint-disable-next-line directive on line 68 references the deprecated rule
`@typescript-eslint/no-var-requires`, but the project's ESLint configuration uses
the modern `@typescript-eslint/no-require-imports` rule instead. Update the
eslint-disable comment that precedes the require statement for
stellar-wallets-kit imports to replace `@typescript-eslint/no-var-requires` with
`@typescript-eslint/no-require-imports` to properly suppress the correct rule.
Source: Linters/SAST tools
2 participants
🚀 ACTA Pull Request
Mark with an
xall the checkboxes that apply (like[x])Closes Issue 3 — No test suite and no test step in CI #12
Added tests (if necessary)
Run tests
Run formatting
Evidence attached
Commented the code
📌 Type of Change
📝 Changes description
📸 Evidence (A Loom/Cap video is required as evidence, we WON'T merge if there's no proof)
⏰ Time spent breakdown
🌌 Comments
Thank you for contributing to ACTA! We hope you can continue contributing to this project.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores