Draft
Conversation
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
❌ Deploy Preview for kleros-v2-neo failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPre-commit hook now fails fast and conditionally runs kleros-app type checks on staged files. A dedicated Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (git)
participant Husky as Husky pre-commit
participant Staged as Staged Files
participant Yarn as Yarn workspace
participant TSC as TypeScript (tsc)
participant Lint as lint-staged -> ESLint/Prettier
Dev->>Husky: git commit (triggers pre-commit)
Husky->>Staged: list staged paths
Staged-->>Husky: paths
alt paths include kleros-app/
Husky->>Yarn: run yarn workspace `@kleros/kleros-app` check-types
Yarn->>TSC: tsc -p kleros-app/tsconfig.check.json
TSC-->>Yarn: success/failure
Yarn-->>Husky: exit code
opt failure
Husky-->>Dev: abort commit (fail-fast)
end
end
Husky->>Lint: run lint-staged (eslint --fix, prettier)
Lint-->>Husky: exit code
Husky-->>Dev: allow/abort commit
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
kyrers
force-pushed
the
feat/pre-commit-type-check
branch
from
7df22dc to
c0302df
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
kleros-sdk/test/dataMappings.test.ts (1)
283-285: Avoid repeatedas unknown astest casts.At Line 283, Line 309, Line 335, and Line 358, double assertions bypass type checks entirely. Prefer a small runtime narrowing helper so test failures surface shape mismatches earlier.
♻️ Suggested pattern
+const expectRecord = (value: unknown): Record<string, unknown> => { + expect(value).to.be.an("object"); + expect(value).to.not.equal(null); + return value as Record<string, unknown>; +}; -const result = (await subgraphAction(mapping)) as unknown as SubgraphActionResult; +const raw = expectRecord(await subgraphAction(mapping)); +expect(raw).to.have.property("escrowsData"); +const result = raw as SubgraphActionResult;Also applies to: 309-309, 335-335, 358-360
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kleros-sdk/test/dataMappings.test.ts` around lines 283 - 285, The test uses unsafe double-casts like "(await subgraphAction(mapping)) as unknown as SubgraphActionResult" which hides shape mismatches; replace these with a small runtime narrowing helper (e.g., assertIsSubgraphActionResult(value) or a validateSubgraphActionResult function) that takes the raw await subgraphAction(mapping) output, performs runtime type checks for the expected fields, and returns the value typed as SubgraphActionResult or throws a descriptive error; update the occurrences around the result variable in the subgraphAction(...) calls so tests fail with clear messages instead of silently bypassing type checking.kleros-sdk/src/dataMappings/utils/disputeDetailsSchema.ts (1)
9-12: RefactorisMultiaddrinto smaller validators.Line 10 packs multiple URI formats/protocol alternatives into one regex, which is hard to maintain and already triggers complexity checks. Splitting the checks into separate validators will keep behavior clearer and easier to evolve.
♻️ Proposed refactor
+const MULTIADDR_PATH_REGEX = + /^\/(?:ip4|ip6|dns4|dns6|dnsaddr|tcp|udp|utp|tls|ws|wss|p2p-circuit|p2p-webrtc-star|p2p-webrtc-direct|p2p-websocket-star|onion|ipfs)(\/[^\s/]+)+$/; +const IPFS_URI_REGEX = + /^ipfs:\/\/[a-zA-Z0-9]+\/[a-zA-Z0-9]+(\.[a-zA-Z0-9]+)?$/; + export const isMultiaddr = (str: string): boolean => - /^\/(?:ip4|ip6|dns4|dns6|dnsaddr|tcp|udp|utp|tls|ws|wss|p2p-circuit|p2p-webrtc-star|p2p-webrtc-direct|p2p-websocket-star|onion|ipfs)(\/[^\s/]+)+$|^ipfs:\/\/[a-zA-Z0-9]+\/[a-zA-Z0-9]+(\.[a-zA-Z0-9]+)?$/.test( - str, - ); + MULTIADDR_PATH_REGEX.test(str) || IPFS_URI_REGEX.test(str);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kleros-sdk/src/dataMappings/utils/disputeDetailsSchema.ts` around lines 9 - 12, The isMultiaddr regexp is doing too much and should be split into smaller, composable validators: create separate helper functions (e.g., isMultiaddrPath, isIpfsUri, isOnionAddr or similar) that each validate one URI/scheme pattern, export them if needed, then rewrite isMultiaddr to return the OR of those helpers (e.g., return isMultiaddrPath(str) || isIpfsUri(str) || isOnionAddr(str)); update any callers/tests to use the new helpers or keep behavior identical and add unit tests for each helper to preserve coverage and clarity; refer to the existing isMultiaddr function name to locate where to change the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@kleros-sdk/src/dataMappings/retrieveRealityData.ts`:
- Around line 87-90: populatedTemplate returned from
rc_question.populatedJSONForTemplate is typed as any and its .type is accessed
later (at uses around where .type is read); add a runtime type-narrowing guard
before those accesses by checking the value is an object and has a string 'type'
property (e.g. if (populatedTemplate && typeof populatedTemplate === 'object' &&
'type' in populatedTemplate && typeof populatedTemplate.type === 'string') { ...
} ), or define and use a local interface (e.g. PopulatedTemplate { type: string;
[key: string]: any }) then narrow/cast after validating the shape so subsequent
reads at the locations that reference populatedTemplate.type (calls near
rc_question.populatedJSONForTemplate) are safe.
In `@kleros-sdk/src/dataMappings/utils/createResultObject.ts`:
- Around line 10-18: The traversal guards in createResultObject.ts use truthy
checks that drop valid falsy values (e.g., 0, false, ""), so change those checks
to nullish checks: replace "if (!acc)" with "if (acc == null)" and replace the
"innerData ? ... : undefined" condition with a nullish check like "if (innerData
== null) return undefined" before accessing (innerData as Record<string,
unknown>)[index]; ensure you reference the variables acc, accRecord, part, and
innerData in the updated checks so valid falsy values are preserved during
traversal.
---
Nitpick comments:
In `@kleros-sdk/src/dataMappings/utils/disputeDetailsSchema.ts`:
- Around line 9-12: The isMultiaddr regexp is doing too much and should be split
into smaller, composable validators: create separate helper functions (e.g.,
isMultiaddrPath, isIpfsUri, isOnionAddr or similar) that each validate one
URI/scheme pattern, export them if needed, then rewrite isMultiaddr to return
the OR of those helpers (e.g., return isMultiaddrPath(str) || isIpfsUri(str) ||
isOnionAddr(str)); update any callers/tests to use the new helpers or keep
behavior identical and add unit tests for each helper to preserve coverage and
clarity; refer to the existing isMultiaddr function name to locate where to
change the logic.
In `@kleros-sdk/test/dataMappings.test.ts`:
- Around line 283-285: The test uses unsafe double-casts like "(await
subgraphAction(mapping)) as unknown as SubgraphActionResult" which hides shape
mismatches; replace these with a small runtime narrowing helper (e.g.,
assertIsSubgraphActionResult(value) or a validateSubgraphActionResult function)
that takes the raw await subgraphAction(mapping) output, performs runtime type
checks for the expected fields, and returns the value typed as
SubgraphActionResult or throws a descriptive error; update the occurrences
around the result variable in the subgraphAction(...) calls so tests fail with
clear messages instead of silently bypassing type checking.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c7c7f72-4c06-4591-8653-9ac9a051a51d
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (31)
.husky/pre-commit.lintstagedrc.jsoneslint-config/.eslintrc.jseslint-config/flat.mjseslint-config/package.jsoneslint.config.mjskleros-app/.lintstagedrc.jsonkleros-app/tsconfig.check.jsonkleros-sdk/.lintstagedrc.jsonkleros-sdk/eslint.config.mjskleros-sdk/package.jsonkleros-sdk/src/dataMappings/actions/fetchIpfsJsonAction.tskleros-sdk/src/dataMappings/actions/subgraphAction.tskleros-sdk/src/dataMappings/executeActions.tskleros-sdk/src/dataMappings/retrieveRealityData.tskleros-sdk/src/dataMappings/utils/actionTypeValidators.tskleros-sdk/src/dataMappings/utils/actionTypes.tskleros-sdk/src/dataMappings/utils/createResultObject.tskleros-sdk/src/dataMappings/utils/disputeDetailsSchema.tskleros-sdk/src/dataMappings/utils/disputeDetailsTypes.tskleros-sdk/src/dataMappings/utils/index.tskleros-sdk/src/dataMappings/utils/populateTemplate.tskleros-sdk/src/dataMappings/utils/replacePlaceholdersWithValues.tskleros-sdk/src/errors/index.tskleros-sdk/src/requests/fetchDisputeDetails.tskleros-sdk/src/requests/fetchDisputeTemplateFromId.tskleros-sdk/src/types/index.tskleros-sdk/src/types/reality-eth-lib.d.tskleros-sdk/src/utils/getDispute.tskleros-sdk/test/dataMappings.test.tspackage.json
✅ Files skipped from review due to trivial changes (19)
- kleros-sdk/.lintstagedrc.json
- kleros-sdk/src/types/reality-eth-lib.d.ts
- kleros-sdk/src/dataMappings/utils/disputeDetailsTypes.ts
- kleros-sdk/src/dataMappings/actions/subgraphAction.ts
- eslint.config.mjs
- kleros-sdk/src/dataMappings/utils/index.ts
- kleros-sdk/src/dataMappings/actions/fetchIpfsJsonAction.ts
- kleros-sdk/src/errors/index.ts
- kleros-app/tsconfig.check.json
- kleros-sdk/src/types/index.ts
- kleros-sdk/eslint.config.mjs
- kleros-sdk/src/dataMappings/utils/actionTypeValidators.ts
- kleros-sdk/src/dataMappings/utils/actionTypes.ts
- kleros-sdk/src/requests/fetchDisputeDetails.ts
- kleros-sdk/src/dataMappings/utils/populateTemplate.ts
- package.json
- kleros-sdk/src/dataMappings/utils/replacePlaceholdersWithValues.ts
- kleros-sdk/src/requests/fetchDisputeTemplateFromId.ts
- kleros-sdk/src/dataMappings/executeActions.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .lintstagedrc.json
- .husky/pre-commit
tractorss
reviewed
Apr 29, 2026
tractorss
reviewed
Apr 29, 2026
| } catch (err: any) { | ||
| throw err; | ||
| } | ||
| data = await executeActions( |
Contributor
There was a problem hiding this comment.
Any reason to unwrap the try-catch 👀
Contributor
Author
There was a problem hiding this comment.
Because it was flagged by no-useless-catch as the behavior is the same.
The try-catch only threw the error, which is the same behavior as now.
Contributor
There was a problem hiding this comment.
Yup, makes sense.
In that case we can just do
const data = await executeActions
wdyt, there is no reason to initialize to an empty object
Contributor
Author
There was a problem hiding this comment.
Addressed in dd3f18f.
It can't be exactly like that because it is inside an if, and I don't think populateTemplate can handle undefined, but you're right that it can be simplified.
tractorss
reviewed
Apr 29, 2026
…NT flat config format
…issing-import rule
…es in type checking
…ommit hook. - Also fix some eslint.config.mjs errors and warnings that were already being flagged by eslint.
kyrers
force-pushed
the
feat/pre-commit-type-check
branch
from
dd3f18f to
c5b4ce8
Compare
|
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
2 participants
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.
Resolves #2273.
PR-Codex overview
This PR primarily focuses on improving code quality and consistency by updating TypeScript types, enhancing error handling, and refining ESLint configurations across various modules.
Detailed summary
additionalContexttype fromanytounknowninGetDisputeParametersOptions.fetchDisputeDetailsandfetchDisputeTemplateFromId.fetchIpfsJsonActionand other action mappings..lintstagedrc.jsonfiles.Summary by CodeRabbit