chore: address TypeScript ESLint v8 upgrade feedback and improve type safety #895
Conversation
transphorm
had a problem deploying
to
development
GitHub Actions
Failure
WalkthroughRepository-wide linting and TypeScript config updates, dependency/version adjustments, and minor code refactors. Adds stricter ESLint rules with targeted overrides, removes Node type inclusions from multiple tsconfigs, swaps ts-node for tsx in contracts scripts, introduces lazy TextDecoder init in NFC processing, and small no-op catch variable renames. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as parseNFCResponse
participant Util as getDecoder()
participant Node as node:util (optional)
Note over Caller: Needs a TextDecoder to decode NFC bytes
Caller->>Util: getDecoder()
alt DECODER not initialized
Util->>Node: require('node:util') if available
Util-->>Caller: new TextDecoder instance
else DECODER initialized
Util-->>Caller: cached TextDecoder
end
Caller->>Caller: decode(value)
Caller-->>Caller: return parsed response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (3)
contracts/hardhat.config.ts (1)
45-64: Guard against undefined PRIVATE_KEY — make accounts optional or fail fastVerified: the provided pre-check printed "PRIVATE_KEY missing; ensure deployments do not target: mainnet sepolia celo alfajores" and exited 1 — PRIVATE_KEY can be unset, so casting it as a string risks passing undefined to Hardhat.
Files/locations to fix:
- contracts/hardhat.config.ts — networks: mainnet, sepolia, celo, alfajores (around lines 45–64)
Apply this safe, minimal change (normalize once and make accounts undefined when missing):
+// normalize once (place near the top of the file, before networks) +const PRIVATE_KEY = process.env.PRIVATE_KEY?.trim(); mainnet: { chainId: 1, url: process.env.MAINNET_RPC_URL || "https://eth.llamarpc.com", - accounts: [process.env.PRIVATE_KEY as string], + accounts: PRIVATE_KEY ? [PRIVATE_KEY] : undefined, }, sepolia: { chainId: 11155111, url: process.env.SEPOLIA_RPC_URL || "https://eth-sepolia.public.blastapi.io", - accounts: [process.env.PRIVATE_KEY as string], + accounts: PRIVATE_KEY ? [PRIVATE_KEY] : undefined, }, celo: { chainId: 42220, url: process.env.CELO_RPC_URL || "https://forno.celo.org", - accounts: [process.env.PRIVATE_KEY as string], + accounts: PRIVATE_KEY ? [PRIVATE_KEY] : undefined, }, alfajores: { chainId: 44787, url: process.env.CELO_ALFAJORES_RPC_URL || "https://alfajores-forno.celo-testnet.org", - accounts: [process.env.PRIVATE_KEY as string], + accounts: PRIVATE_KEY ? [PRIVATE_KEY] : undefined, },Optional alternative (fail-fast when targeting public networks): add a short runtime check in the config or CI that aborts deployments if PRIVATE_KEY is missing for public networks (your bash check already demonstrates this).
Fix required: the accounts lines should be hardened as above to avoid misconfigured deployments.
circuits/tsconfig.json (1)
1-25: Re-add Node types — circuits uses Node globals in tests & build scriptsFound concrete uses of Node APIs/globals in the circuits tree (process, Buffer, path/fs, fileURLToPath/__dirname, crypto). Dropping
"types": ["node"]will degrade or break TypeScript checking for these files. Please restore Node types in the package tsconfigs.Files that need Node type coverage (representative):
- circuits/tests/**/*.test.ts — many import path + fileURLToPath and use __dirname (e.g. circuits/tests/utils/rsapss.test.ts uses process.env).
- circuits/tests/utils/generateMockInputsInCircuits.ts — Buffer, crypto usage.
- circuits/tsup.config.ts — path, fileURLToPath, __filename/__dirname.
- circuits/scripts/postBuild.mjs, circuits/scripts/organize-types.mjs — node:fs, node:path, readFileSync/writeFileSync.
- circuits/scripts/utils/*.sh — shell scripts reference file paths (not type-checked, but relevant to build behavior).
Suggested minimal fix — add Node types to compilerOptions in circuits/tsconfig.json (and mirror in tsconfig.spec.json if you run tests through that config):
Patch example:
@@ circuits/tsconfig.json "compilerOptions": { + "types": ["node"], "resolveJsonModule": true, "esModuleInterop": true, ... }Repeat in circuits/tsconfig.spec.json if you want test-specific type coverage.
Reason: Node type defs provide proper typings for Buffer, process, built-in modules (fs/path/crypto) and the "node:" import specifiers — without them you will see missing-module/type errors or loose
anytypes in tests and build scripts.packages/mobile-sdk-alpha/src/processing/nfc.ts (1)
20-32: Metro/webpack may still try to statically resolvenode:util; add a safe fallback and harden the guard.You’ve done the right thing by gating on a Node environment and aliasing
require, but some bundlers can still attempt static resolution of string-literal module specifiers. Two improvements:
- Add a fallback to
require('util')for older Node versions that don’t support thenode:scheme.- Keep the indirection, but catch per-attempt to avoid a single failure swallowing the fallback.
Apply this diff inside the Node branch:
- try { - // eslint-disable-next-line @typescript-eslint/no-var-requires - const req = typeof require === 'function' ? require : undefined; - const util = req ? req('node:util') : undefined; - if (util?.TextDecoder) { - return new util.TextDecoder('utf-8', { fatal: true }); - } - } catch { - // Fall through to error - } + try { + // eslint-disable-next-line @typescript-eslint/no-var-requires + const req = typeof require === 'function' ? require : undefined; + let utilMod: any | undefined; + if (req) { + try { + utilMod = req('node:util'); + } catch { + // older Node may not support the node: scheme + } + if (!utilMod) { + try { + utilMod = req('util'); + } catch { + // ignore; fall through to error + } + } + } + const TD = utilMod?.TextDecoder; + if (TD) { + return new TD('utf-8', { fatal: true }); + } + } catch { + // Fall through to error + }Note: If you see Metro attempting to resolve even through an alias, we can switch to a more defensive pattern (e.g.,
(0, eval)('require')) but I'd avoid introducingevalunless necessary.
🧹 Nitpick comments (6)
common/tsconfig.json (1)
14-17: Node types removed: ensure required libs are explicit for cross-platform usagecommon is consumed in multiple environments (web/RN/Node). With Node types removed, verify that any usage of web APIs (TextEncoder/Decoder, crypto.subtle) has proper typings via lib settings, and any Node APIs are imported from modules with their own types (e.g., buffer).
Optionally make libs explicit to prevent accidental environment coupling:
"compilerOptions": { "target": "ES2020", "module": "NodeNext", + "lib": ["ES2020", "DOM", "DOM.Iterable", "ESNext.AsyncIterable"], ... }contracts/hardhat.config.ts (1)
7-11: Duplicate side-effect import of hardhat-contract-sizer
hardhat-contract-sizeris imported twice. Keep a single import to avoid redundant plugin registration.Apply this diff:
import "hardhat-contract-sizer"; import "@nomicfoundation/hardhat-ignition-ethers"; import "solidity-coverage"; import "hardhat-gas-reporter"; -import "hardhat-contract-sizer";common/src/utils/shaPad.ts (2)
56-74: Reduce quadratic copying during paddingThe repeated
mergeUInt8Arraysinside loops leads to O(n^2) copying. Precompute the final length and allocate once to write bytes in place for better performance on large inputs.High-level approach:
- Compute target lengths (pre-1 bit, zero pad, length field, then maxShaBytes).
- Allocate a single
Uint8Array(finalLen)and write segments via offsets.
I can provide a concrete refactor if desired.Also applies to: 91-99
9-12: Fix comment typos for clarityMinor typos in comments: “litteEndian” and incorrect byte size for Int8.
Apply this diff:
- const arr = new ArrayBuffer(8); // an Int32 takes 4 bytes + const arr = new ArrayBuffer(8); // two Int32 = 8 bytes total const view = new DataView(arr); - view.setInt32(4, num, false); // byteOffset = 0; litteEndian = false + view.setInt32(4, num, false); // byteOffset = 4; littleEndian = false - const arr = new ArrayBuffer(1); // an Int8 takes 4 bytes + const arr = new ArrayBuffer(1); // an Int8 takes 1 byte const view = new DataView(arr); - view.setUint8(0, num); // byteOffset = 0; litteEndian = false + view.setUint8(0, num); // byteOffset = 0; littleEndian = falseAlso applies to: 30-33
app/.eslintrc.cjs (1)
156-158: Scope the rule exemptions to where they’re neededGlobally disabling
@typescript-eslint/no-require-importsandno-empty-object-typecan mask issues across the app. Prefer targeted overrides (e.g., for legacy CJS scripts, RN shims, or specific modules) to keep signal high.Example override scoping:
rules: { - '@typescript-eslint/no-require-imports': 'off', - '@typescript-eslint/no-empty-object-type': 'off', + // keep default behavior; enable scoped overrides below + // '@typescript-eslint/no-require-imports': 'off', + // '@typescript-eslint/no-empty-object-type': 'off', }, overrides: [ { + files: ['scripts/**/*.cjs', 'scripts/*.cjs'], + rules: { + '@typescript-eslint/no-require-imports': 'off', + }, + }, + { + files: [ + // list modules that intentionally use '{}' or object shapes without members + 'src/utils/**/types.ts', + ], + rules: { + '@typescript-eslint/no-empty-object-type': 'off', + }, + }, + { files: ['docs/examples/**/*.ts'], rules: { '@typescript-eslint/no-unused-vars': 'off', 'no-unused-vars': 'off', 'import/no-unresolved': 'off', }, },packages/mobile-sdk-alpha/src/processing/nfc.ts (1)
34-37: Consider a more actionable error message with remediation hints.Point to an installation/compat note or polyfill guidance to help integrators resolve environment issues faster.
Suggested tweak:
- throw new Error( - 'TextDecoder not available in this environment. ' + - 'This SDK requires TextDecoder support which is available in modern browsers, Node.js, and React Native.', - ); + throw new Error( + 'TextDecoder is not available in this environment. ' + + 'This SDK requires TextDecoder support (available in modern browsers, Node.js ≥14+, and React Native). ' + + 'If you are in Node, ensure a recent Node version or provide a WHATWG TextDecoder polyfill.' + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
app/.eslintrc.cjs(1 hunks)app/package.json(2 hunks)circuits/package.json(0 hunks)circuits/tsconfig.json(1 hunks)common/package.json(1 hunks)common/src/utils/shaPad.ts(2 hunks)common/tsconfig.json(1 hunks)contracts/hardhat.config.ts(1 hunks)contracts/package.json(1 hunks)contracts/tsconfig.json(1 hunks)package.json(0 hunks)packages/mobile-sdk-alpha/.eslintrc.cjs(1 hunks)packages/mobile-sdk-alpha/package.json(1 hunks)packages/mobile-sdk-alpha/src/processing/nfc.ts(3 hunks)sdk/core/package.json(0 hunks)sdk/core/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (3)
- circuits/package.json
- package.json
- sdk/core/package.json
🧰 Additional context used
🪛 GitHub Actions: Contracts CI
circuits/tsconfig.json
[error] 1-1: Step 'yarn prettier:check' failed with exit code 1. Prettier formatting check failed for tsconfig.json. Run 'yarn prettier --write' to fix formatting.
sdk/core/tsconfig.json
[error] 1-1: Step 'yarn prettier:check' failed with exit code 1. Prettier formatting check failed for tsconfig.json. Run 'yarn prettier --write' to fix formatting.
common/tsconfig.json
[error] 1-1: Step 'yarn prettier:check' failed with exit code 1. Prettier formatting check failed for tsconfig.json. Run 'yarn prettier --write' to fix formatting.
contracts/tsconfig.json
[error] 1-1: Step 'yarn prettier:check' failed with exit code 1. Prettier formatting check failed for tsconfig.json. Run 'yarn prettier --write' to fix formatting.
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: analyze-android
- GitHub Check: e2e-ios
- GitHub Check: run_circuit_tests
- GitHub Check: build
🔇 Additional comments (9)
contracts/hardhat.config.ts (1)
3-4: ESM dotenv import/config looks good — Hardhat/ts-node ESM config couldn't be verified hereI ran the suggested checks in the sandbox; ripgrep did not find a "NodeNext" module setting and the node import failed with ERR_MODULE_NOT_FOUND for "dotenv", so I couldn't confirm Hardhat/TS ESM loading.
Files / checks to verify locally:
- contracts/hardhat.config.ts — ensure dotenv is imported before any code that reads process.env:
import dotenv from "dotenv"; dotenv.config({
- tsconfig.json (root) and contracts/tsconfig.json — confirm:
- "module": "NodeNext"
- "moduleResolution": "NodeNext"
- package.json — confirm "type": "module" if applicable and that "dotenv" is listed in dependencies/devDependencies.
- Hardhat startup — ensure Hardhat runs the TS config as ESM (ts-node/esm or appropriate loader) so dotenv.config executes before other modules read env.
Quick local verification commands:
- rg -n $'module\W*:\W*"NodeNext"' tsconfig.json contracts/tsconfig.json -A3
- Check dotenv presence: jq -r '.dependencies + .devDependencies | keys[]' package.json | rg dotenv || npm ls dotenv
- Test dotenv import (after installing dotenv locally):
node -e 'import("dotenv").then(d=>{d.config({path:".env.test"}); console.log("dotenv ok")}).catch(e=>{console.error(e); process.exit(1)})'Security note: ensure .env is gitignored and dotenv.config is called before any code that consumes secrets.
packages/mobile-sdk-alpha/package.json (1)
57-58: Confirmed — @typescript-eslint v8 is consistent across the monorepoAll occurrences of @typescript-eslint/eslint-plugin and @typescript-eslint/parser reference major 8 (^) — no mismatches found.
Files checked (plugin=parser):
- app/package.json: ^8.0.0 / ^8.0.0
- common/package.json: ^8.0.0 / ^8.0.0
- packages/mobile-sdk-alpha/package.json: ^8.0.0 / ^8.0.0
- sdk/qrcode/package.json: ^8.0.0 / ^8.0.0
No changes required; approving the bump.
common/src/utils/shaPad.ts (1)
48-49: Type tightening to Uint8Array is a winConverting the working buffer to
Uint8Arrayremoves implicit any, improves type safety, and aligns with downstream operations.Also applies to: 87-88
packages/mobile-sdk-alpha/src/processing/nfc.ts (4)
40-46: Lazy initialization is a good call; avoids import-time failures.Caching the decoder and initializing on first use keeps RN and browser bundles safe from Node-only code paths at import time. This also avoids repeated instantiation costs.
102-105: Switching togetDecoder()here is correct and keeps runtime behavior unchanged.The decode path remains functionally identical, with improved resilience due to lazy construction.
5-18: Minor: RN branch likely redundant ifglobalThis.TextDecoderis set; verify necessity.Most modern RN runtimes expose
TextDecoderonglobalThis, making the RN-specific branch unreachable. If you keep it for legacy RN, fine; otherwise, consider simplifying by relying solely on the first check for maintainability.If you want to verify, run this at runtime in RN dev to see where
TextDecoderis coming from:
- console.log('has globalThis.TextDecoder', 'TextDecoder' in globalThis);
- console.log('has global.TextDecoder', !!(globalThis as any)?.global?.TextDecoder);
40-46: No action required —DOMlib is present in packages/mobile-sdk-alpha/tsconfig.jsonVerified: packages/mobile-sdk-alpha/tsconfig.json sets compilerOptions.lib = ["ES2020","DOM"], so TextDecoder types are available and the lazy initializer in packages/mobile-sdk-alpha/src/processing/nfc.ts is fine as-is.
app/package.json (2)
163-163: Good alignment:@types/react-domnow matches React 18.This avoids React 19 type APIs while the runtime is on React 18.3.x.
189-189: Safe patch bump on ts-node.No breaking changes expected; this is a straightforward maintenance update.
|
@coderabbitai title |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
contracts/tsconfig.json (2)
2-11: Pin "lib" to a Node-friendly ES set and fix moduleResolution casingRan the suggested scan — the contracts package clearly uses Node globals (process, fs, path, Buffer, etc.), e.g.:
- contracts/hardhat.config.ts (process)
- contracts/scripts/setVerifiers.ts (fs, path, ethers provider)
- contracts/test/utils/example/merkle-tree.ts (Buffer)
Action: update contracts/tsconfig.json to avoid DOM type bleed and normalize moduleResolution.
Proposed diff:
"compilerOptions": { "target": "es2020", "module": "node16", - "moduleResolution": "Node16", + "moduleResolution": "node16", "esModuleInterop": true, "forceConsistentCasingInFileNames": true, "strict": true, "skipLibCheck": true, "resolveJsonModule": true, + "lib": ["es2020"] }Optional: if you rely on ambient Node typings (process/Buffer without importing), ensure @types/node is installed or add "types": ["node"] — but the lib change is the minimum safe fix.
5-5: Normalize moduleResolution values to lowercase (fix required)TypeScript config values should use lowercase identifiers (e.g. node, node16, nodenext, bundler). Capitalized variants can cause tsc/IDE inconsistencies — normalize them across the repo.
Files to fix (current → suggested):
- contracts/tsconfig.json: "Node16" → "node16" (line 5)
- sdk/qrcode/tsconfig.json: "NodeNext" → "nodenext" (line 14)
- sdk/core/tsconfig.json: "NodeNext" → "nodenext" (line 13)
- circuits/tsconfig.json: "NodeNext" → "nodenext" (line 7)
- common/tsconfig.json: "NodeNext" → "nodenext" (line 13)
- circuits/tsconfig.spec.json: "NodeNext" → "nodenext" (line 7)
- packages/mobile-sdk-alpha/tsconfig.cjs.json: "Node" → "node" (line 5)
- common/tsconfig.cjs.json: "Node" → "node" (line 5)
- circuits/tsconfig.cjs.json: "Node" → "node" (line 5)
- packages/mobile-sdk-alpha/tsconfig.json: "Bundler" → "bundler" (line 5)
Apply (example for contracts/tsconfig.json):
- "moduleResolution": "Node16", + "moduleResolution": "node16",Run a quick repo-wide replace or update these files manually, then verify by running a TypeScript build/IDE check.
♻️ Duplicate comments (5)
contracts/tsconfig.json (1)
10-11: Dangling comma fixed — JSON now validThe trailing comma after resolveJsonModule has been removed. The file parses correctly now; Prettier/tsc should stop failing on this.
common/package.json (1)
367-369: @typescript-eslint v8 bump: audit configs for removed/renamed rulesThe version bump is aligned with Node 22 and TS 5.9. Validate local ESLint configs for v8-breaking changes (e.g., ban-types removal, no-var-requires deprecation).
Run this quick scan to spot deprecated/renamed rules across the repo:
#!/bin/bash set -euo pipefail echo "Scanning ESLint configs for removed/deprecated @typescript-eslint rules..." rg -n --no-heading -S '@typescript-eslint/(ban-types|no-var-requires|prefer-ts-expect-error)' --glob '**/.eslintrc.*' -A 1 -B 1 || true echo echo "Recommended replacements (ensure your configs reflect these):" echo "- ban-types -> no-restricted-types, no-empty-object-type, no-unsafe-function-type, no-wrapper-object-types" echo "- no-var-requires -> no-require-imports"app/package.json (2)
169-170: Upgrade to @typescript-eslint v8 is appropriate.This aligns with the stricter lint rules added in the app’s ESLint config.
173-173: ESLint bumped to ^8.57.0: resolves prior peer-dep mismatch.This satisfies typescript-eslint v8’s peer requirements.
packages/mobile-sdk-alpha/.eslintrc.cjs (1)
54-55: Enableno-require-importsandno-empty-object-type: great hardening.This prevents accidental CommonJS creep in ESM/React Native paths and discourages loose
{}typing.
🧹 Nitpick comments (14)
app/src/utils/proving/provingMachine.ts (1)
415-430: Use optional catch binding to avoid unused variable and keep consistencyThe underscore-prefixed catch var works, but optional catch binding is cleaner and avoids any tool that still flags it as unused.
- } catch (_error) { + } catch { if (navigationRef.isReady()) { navigationRef.navigate('Launch'); } }app/src/screens/settings/ManageDocumentsScreen.tsx (1)
128-147: Nit: drop unused catch variable; tighten types fordocumentCategory
- Prefer optional catch binding to silence unused-var warnings.
- The function accepts
documentCategory: string; you already haveDocumentCategoryin this module’s types. Tightening the type improves safety and autocomplete.- const extractCountryFromData = ( - data: string, - documentCategory: string, - ): string | null => { + const extractCountryFromData = ( + data: string, + documentCategory: DocumentCategory, + ): string | null => { if (!data) return null; try { if (documentCategory === 'passport' || documentCategory === 'id_card') { if (data.length >= 5) { const countryCode = data.substring(2, 5); // Extract positions 2-4 from MRZ return countryCode; } } else if (documentCategory === 'aadhaar') { return 'IND'; } return null; - } catch (_error) { + } catch { return null; } };Also add the missing type import (outside this hunk):
// at the existing import from '@/providers/passportDataProvider' import { type DocumentCatalog, type DocumentMetadata, type DocumentCategory, // add this usePassport, } from '@/providers/passportDataProvider';app/scripts/mobile-deploy-confirm.cjs (2)
62-69: Silence unused catch variable warnings by using optional catch bindingStatic analysis flagged the unused
_errorhere. Optional catch binding is supported in modern Node and keeps things tidy.function safeReadFile(filePath, description) { try { return fs.readFileSync(filePath, 'utf8'); - } catch (_error) { + } catch { console.warn(`Warning: Could not read ${description} at ${filePath}`); return null; } }
77-98: Two improvements: remove unused catch var and prefer argumentized process execution
- Swap to optional catch binding to address the unused
_errorwarning.- Longer-term: consider using execFileSync/spawn with args instead of stringly-typed execSync to eliminate shell interpolation risks entirely (you already whitelist commands; this would make it airtight).
function safeExecSync(command, description) { // Whitelist of allowed commands to prevent command injection const allowedCommands = [ 'git branch --show-current', 'git status --porcelain', ]; // Validate that the command is in the whitelist if (!allowedCommands.includes(command)) { console.warn( `Warning: Command '${command}' is not allowed for security reasons`, ); return null; } try { return execSync(command, { encoding: 'utf8' }).trim(); - } catch (_error) { + } catch { console.warn(`Warning: Could not ${description}`); return null; } }Optional harderening (illustrative, not a drop-in within this hunk):
// Alternative approach using execFileSync to avoid shell: // Represent allowed commands as tuples [cmd, ...args] and match against them. const { execFileSync } = require('child_process'); const allowedCommands = [ ['git', 'branch', '--show-current'], ['git', 'status', '--porcelain'], ]; function safeExecFile(allowed, description) { try { return execFileSync(allowed[0], allowed.slice(1), { encoding: 'utf8' }).trim(); } catch { console.warn(`Warning: Could not ${description}`); return null; } }app/src/screens/prove/ProofRequestStatusScreen.tsx (1)
86-101: Use optional catch binding in try/catch around deep link URL parsingThe caught variable is unused; drop it for cleanliness and to satisfy stricter linters.
- } catch (_error) { + } catch { console.warn( 'Invalid deep link URL provided:', selfApp.deeplinkCallback, ); }app/src/utils/testingUtils.ts (1)
25-32: Use optional catch binding in testing utility cleanupsBoth catches ignore the error; optional catch binding communicates intent and avoids unused-var warnings.
for (const doc of catalog.documents) { try { await Keychain.resetGenericPassword({ service: `document-${doc.id}` }); console.log(`Cleared document: ${doc.id}`); - } catch (_error) { + } catch { console.log(`Document ${doc.id} not found or already cleared`); } } // Clear the catalog itself try { await Keychain.resetGenericPassword({ service: 'documentCatalog' }); console.log('Cleared document catalog'); - } catch (_error) { + } catch { console.log('Document catalog not found or already cleared'); }Also applies to: 35-40
app/docs/examples/tree-shaking/level2-optimal-example.ts (1)
20-27: Console logs in docs example: OK with targeted lint suppression; avoid leaking real data in productionGiven this is a docs example, the logs are reasonable. If this example is ever included in production builds, consider gating logs behind a debug flag to avoid noisy logs and accidental exposure of environment endpoints or derived identifiers.
app/docs/examples/tree-shaking/level3-optimal-example.ts (1)
30-33: Console logs look intentional for documentation; consider a guard to prevent accidental prod loggingThe inline
no-consoledisables are appropriate for a docs example. If these examples can be bundled into any production app surfaces, wrap these logs with an environment or DEBUG guard to reduce noise and avoid leaking operational details.Also applies to: 37-38, 42-43
app/src/providers/passportDataProvider.tsx (1)
281-283: Underscore-prefixed catch parameters: Good alignment with no-unused-vars policyRenaming
errorto_errorin non-essential cleanup paths is a good fit to satisfy stricter ESLint without changing behavior. Optional: for consistency, consider standardizing error handling across the file (either always include error objects when useful for debugging, or keep the minimal messages for expected "not found" cases).Also applies to: 290-292, 308-310, 348-350
app/src/components/BackupDocumentationLink.tsx (1)
13-13: Empty props interface removal is good; consider avoiding React.FC to prevent implicitchildren.Using React.FC implicitly adds a
childrenprop that this component doesn’t use. A simple function type with an explicit return keeps the surface tight and avoids confusion.Apply this diff to the signature:
-const BackupDocumentationLink: React.FC = () => { +const BackupDocumentationLink = (): JSX.Element => {contracts/package.json (1)
35-35: Switch to tsx for script execution: good move.Replacing ts-node with tsx simplifies ESM handling and cold starts. Consider pinning tsx in devDependencies for reproducible CI installs (rather than always relying on npx’s network fetch).
Add to devDependencies (outside this hunk):
{ "devDependencies": { "tsx": "^4.19.2" } }app/package.json (1)
189-189: ts-node bump to ^10.9.2.No objections. If ts-node isn’t actually used in app scripts, you can consider removing it later to reduce dev surface area.
app/.eslintrc.cjs (2)
156-160: Top-level rule adjustments look sensible for RN app ergonomics.
- Enforcing no-require-imports prevents bundler/runtime surprises.
- Warnings for unused vars/expressions balance signal vs. friction.
Consider planning a future pass to elevate
no-unused-varsback to error to keep the codebase crisp.
223-237: Tighten the*.jsoverride scope to avoid masking ESM issues.With
"type": "module"in package.json, blanket-disablingno-require-importsfor all.jsfiles may allow accidentalrequire()in ESM code. Restrict to script/config locations where CJS is expected.Apply this diff to narrow the override:
- { - files: ['*.cjs', '*.js'], + { + files: ['*.cjs', 'scripts/**/*.cjs', 'scripts/*.js', 'scripts/**/*.js', 'metro.config.js', 'babel.config.js'], env: { node: true, commonjs: true, es6: true, }, parserOptions: { ecmaVersion: 2020, sourceType: 'script', }, rules: { '@typescript-eslint/no-var-requires': 'off', - '@typescript-eslint/no-require-imports': 'off', + '@typescript-eslint/no-require-imports': 'off', 'no-undef': 'off', }, },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (25)
app/.eslintrc.cjs(4 hunks)app/docs/examples/tree-shaking/level2-optimal-example.ts(1 hunks)app/docs/examples/tree-shaking/level3-optimal-example.ts(1 hunks)app/package.json(2 hunks)app/scripts/mobile-deploy-confirm.cjs(2 hunks)app/scripts/tests/tree-shaking.test.cjs(1 hunks)app/src/components/BackupDocumentationLink.tsx(1 hunks)app/src/hooks/useRecoveryPrompts.ts(1 hunks)app/src/providers/passportDataProvider.tsx(4 hunks)app/src/screens/prove/ProofRequestStatusScreen.tsx(1 hunks)app/src/screens/settings/ManageDocumentsScreen.tsx(1 hunks)app/src/stores/proofHistoryStore.ts(1 hunks)app/src/utils/cloudBackup/helpers.ts(1 hunks)app/src/utils/logger/nativeLoggerBridge.ts(1 hunks)app/src/utils/proving/provingMachine.ts(1 hunks)app/src/utils/testingUtils.ts(2 hunks)app/tests/src/providers/passportDataProvider.test.tsx(1 hunks)circuits/package.json(1 hunks)circuits/tsconfig.json(0 hunks)common/.eslintrc.cjs(0 hunks)common/package.json(2 hunks)contracts/package.json(3 hunks)contracts/tsconfig.json(1 hunks)packages/mobile-sdk-alpha/.eslintrc.cjs(2 hunks)packages/mobile-sdk-alpha/package.json(2 hunks)
💤 Files with no reviewable changes (2)
- circuits/tsconfig.json
- common/.eslintrc.cjs
✅ Files skipped from review due to trivial changes (6)
- app/scripts/tests/tree-shaking.test.cjs
- app/src/stores/proofHistoryStore.ts
- app/src/hooks/useRecoveryPrompts.ts
- app/src/utils/cloudBackup/helpers.ts
- app/tests/src/providers/passportDataProvider.test.tsx
- app/src/utils/logger/nativeLoggerBridge.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/mobile-sdk-alpha/package.json
🧰 Additional context used
📓 Path-based instructions (1)
app/src/**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit Configuration File
app/src/**/*.{ts,tsx,js,jsx}: Review React Native TypeScript code for:
- Component architecture and reusability
- State management patterns
- Performance optimizations
- TypeScript type safety
- React hooks usage and dependencies
- Navigation patterns
Files:
app/src/components/BackupDocumentationLink.tsxapp/src/utils/proving/provingMachine.tsapp/src/screens/settings/ManageDocumentsScreen.tsxapp/src/screens/prove/ProofRequestStatusScreen.tsxapp/src/utils/testingUtils.tsapp/src/providers/passportDataProvider.tsx
🧬 Code Graph Analysis (2)
app/docs/examples/tree-shaking/level2-optimal-example.ts (4)
common/index.ts (3)
API_URL(20-20)PASSPORT_ATTESTATION_ID(36-36)hash(91-91)common/src/constants/index.ts (2)
API_URL(4-4)PASSPORT_ATTESTATION_ID(22-22)common/src/utils/hash/sha.ts (1)
hash(1-1)common/src/utils/index.ts (1)
hash(27-27)
app/docs/examples/tree-shaking/level3-optimal-example.ts (5)
common/index.ts (4)
API_URL(20-20)PASSPORT_ATTESTATION_ID(36-36)hash(91-91)flexiblePoseidon(89-89)common/src/constants/index.ts (2)
API_URL(4-4)PASSPORT_ATTESTATION_ID(22-22)common/src/utils/hash/sha.ts (1)
hash(1-1)common/src/utils/index.ts (2)
hash(27-27)flexiblePoseidon(24-24)common/src/utils/hash/poseidon.ts (1)
flexiblePoseidon(1-1)
🪛 GitHub Check: lint
app/scripts/mobile-deploy-confirm.cjs
[warning] 65-65:
'_error' is defined but never used
[warning] 94-94:
'_error' is defined but never used
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run_circuit_tests
- GitHub Check: analyze-android
- GitHub Check: e2e-ios
🔇 Additional comments (9)
circuits/package.json (1)
110-110: Pinning Yarn 4.6.0 is fine; ensure Corepack is enabled in CI/dev environmentsWith Node 22 engines, Yarn 4.6.0 is compatible. To avoid developer/CI drift, make sure Corepack is enabled and that your CI uses the pinned Yarn version.
- Ensure CI and local setup docs include:
corepack enable(Node >=16.10).- Verify the repo has a single authoritative Yarn version across workspaces (root package.json or toolchain config) to avoid mixed Yarn versions during workspace scripts.
common/package.json (1)
382-382: Pinning Yarn 4.6.0 at package level: ensure consistency across the monorepoTo avoid Yarn version skew, ensure the root and all packages either share the same
packageManagerfield or inherit a single root policy. Confirm CI uses that exact Yarn version via Corepack.contracts/package.json (2)
39-45: Consistent migration to tsx + dotenv usage looks solid.The environment variable defaults and use of dotenv-cli are consistent and safe. Scripts should run cleanly under Node 22 with TS 5.9.
123-123: Standardizing package manager to Yarn 4.6.0: good for workspace consistency.Make sure CI’s corepack is kept in sync to avoid resolution differences.
app/package.json (2)
163-163: Correctly aligned React DOM types with React 18.Matching
@types/react-domto 18.x with React 18 is the right call.
193-193: Pinned Yarn 4.6.0 at the app package level is consistent.Ensure CI caches are updated to reflect Yarn 4’s cache location changes.
packages/mobile-sdk-alpha/.eslintrc.cjs (1)
84-90: Targeted override for NFC shim is scoped and justified.Allowing require only in
src/processing/nfc.tsis the right exception for conditional polyfills and environment-safe imports.app/.eslintrc.cjs (2)
203-207: Test overrides are practical.Allowing
require()andanyin tests makes mocking easier without polluting app code.
217-222: Scoped exception for proving machine is appropriate.This localizes the CJS interop/dynamic import needs without weakening the rest of the app.
| "types": "tsc -noEmit", | ||
| "update:cscaroot": "npx dotenv-cli -- bash -c 'yarn hardhat ignition deploy ignition/modules/scripts/updateRegistryCscaRoot.ts --network ${NETWORK:-localhost} ${VERIFY:+--verify}'", | ||
| "update:hub": "npx dotenv-cli -- bash -c 'NETWORK=${NETWORK:-staging} npx ts-node scripts/setRegistry.ts'", | ||
| "update:hub": "npx dotenv-cli -- bash -c 'NETWORK=${NETWORK:-staging} npx tsx scripts/setRegistry.ts'", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Likely copy/paste error: update:hub points to setRegistry.ts.
This looks incorrect given the script name. It probably should target a hub-related script (e.g., setHubV2.ts or updateHub.ts). Please correct the target to avoid accidental registry updates.
Run this to verify the intended script exists:
Apply this diff if scripts/setHubV2.ts is the intended target:
-"update:hub": "npx dotenv-cli -- bash -c 'NETWORK=${NETWORK:-staging} npx tsx scripts/setRegistry.ts'",
+"update:hub": "npx dotenv-cli -- bash -c 'NETWORK=${NETWORK:-staging} npx tsx scripts/setHubV2.ts'",🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Showing contracts scripts:"
fd -t f . contracts/scripts | sort
echo
echo "Does setHubV2.ts exist?"
if fd -t f setHubV2.ts contracts/scripts >/dev/null; then echo "OK: scripts/setHubV2.ts exists"; else echo "Missing: scripts/setHubV2.ts"; fi
echo
echo "Does setRegistry.ts exist?"
if fd -t f setRegistry.ts contracts/scripts >/dev/null; then echo "OK: scripts/setRegistry.ts exists"; else echo "Missing: scripts/setRegistry.ts"; fi
echo
echo "Does updateHub.ts exist?"
if fd -t f updateHub.ts contracts/scripts >/dev/null; then echo "OK: scripts/updateHub.ts exists"; else echo "Missing: scripts/updateHub.ts"; fiLength of output: 1296
I'll wait for the results and then produce the final rewritten review comment.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "contracts/package.json -> scripts section:"
python - <<'PY'
import json,sys
p='contracts/package.json'
try:
with open(p) as f:
data=json.load(f)
except Exception as e:
print("ERROR reading",p,e)
sys.exit(1)
scripts=data.get("scripts",{})
import pprint
pprint.pprint(scripts)
PY
echo
echo "---- head of contracts/scripts/updateHub.ts ----"
sed -n '1,160p' contracts/scripts/updateHub.ts || true
echo
echo "---- head of contracts/scripts/setHubV2.ts ----"
sed -n '1,160p' contracts/scripts/setHubV2.ts || true
echo
echo "---- head of contracts/scripts/setRegistry.ts ----"
sed -n '1,160p' contracts/scripts/setRegistry.ts || trueLength of output: 15733
Fix: contracts/package.json "update:hub" points to scripts/setRegistry.ts (copy/paste error)
Verified: contracts/scripts/setHubV2.ts and contracts/scripts/setRegistry.ts both exist; there is no updateHub.ts. setHubV2.ts updates hub registries, while setRegistry.ts updates registry address/CSCA root — so "update:hub" should run the hub script.
Files to fix:
- contracts/package.json — scripts.update:hub
- contracts/scripts/setHubV2.ts — (exists; intended target)
- contracts/scripts/setRegistry.ts — (exists; current, incorrect target)
Apply this diff:
-"update:hub": "npx dotenv-cli -- bash -c 'NETWORK=${NETWORK:-staging} npx tsx scripts/setRegistry.ts'",
+"update:hub": "npx dotenv-cli -- bash -c 'NETWORK=${NETWORK:-staging} npx tsx scripts/setHubV2.ts'",Please update to avoid accidentally running registry updates when you intended to update the hub.
📝 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.
| "update:hub": "npx dotenv-cli -- bash -c 'NETWORK=${NETWORK:-staging} npx tsx scripts/setRegistry.ts'", | |
| "update:hub": "npx dotenv-cli -- bash -c 'NETWORK=${NETWORK:-staging} npx tsx scripts/setHubV2.ts'", |
🤖 Prompt for AI Agents
In contracts/package.json around line 67, the "update:hub" npm script
incorrectly invokes scripts/setRegistry.ts (copy/paste error) instead of the hub
update script; update the command to run contracts/scripts/setHubV2.ts (e.g.,
replace reference to setRegistry.ts with setHubV2.ts) so the script triggers the
hub registry update rather than registry/CSCA updates.
There was a problem hiding this comment.
this is pre-existing and i only changed ts-node to tsx so will leave the command as is.
fyi @kevinsslin @Nesopie
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
Tip For best results, initiate chat on the files or code changes. @transphorm Looking at the changes in this PR, I'd suggest a more descriptive title: "chore: address TypeScript ESLint v8 upgrade feedback and improve type safety" This better reflects what was actually accomplished:
The current title "Chore: typescript upgrade cr feedback" is a bit generic and doesn't convey the specific improvements made. |
transphorm
changed the title
|
@seshanthS @Nesopie this pull request unifies package versions and adds typing and type safety checks. no logic has been changed |
Summary by CodeRabbit
Chores
Refactor
Documentation
Tests