Court features selection #2117
Open
Court features selection #2117
Conversation
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
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds dispute-kit-driven feature selection: new feature enums/helpers, UI components (feature groups, radios, gated ERC-20/ERC-1155, classic vote), context wiring for selected features, a new Court page with FeatureSelection and skeleton, and extends court details GraphQL query to include hiddenVotes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant Court as Court Page
participant Ctx as NewDisputeContext
participant CT as useCourtTree
participant FS as FeatureSelection
participant SDK as useSupportedDisputeKits
participant DK as disputeFeature helpers
U->>Court: Open Resolver › Court
Court->>CT: Fetch court tree
CT-->>Court: Court items
U->>Court: Select court
Court->>Ctx: setDisputeData({ courtId, disputeKitId: undefined })
Court->>FS: Render FeatureSelection
FS->>SDK: Fetch supported kit ids for court
SDK-->>FS: Supported kit ids
FS->>DK: getVisibleFeaturesForCourt(ids)
DK-->>FS: Visible features/groups
U->>FS: Toggle feature(s)
FS->>DK: ensureValidSmart / toggleFeature
DK-->>FS: Validated selection
FS->>DK: findMatchingKits(selection)
DK-->>FS: Matching kits
alt exactly one match
FS->>Ctx: setDisputeData({ disputeKitId, disputeKitData })
else zero matches
FS->>Ctx: setDisputeData({ disputeKitId: undefined })
end
sequenceDiagram
autonumber
actor U as User
participant UI as GatedErc20/GatedErc1155
participant Val as Validation Hooks
participant Ctx as NewDisputeContext
U->>UI: Enter token address / tokenId
UI->>Val: Validate address (ERC-20/ERC-721/ERC-1155)
Val-->>UI: { isValid, error, loading }
UI->>Ctx: Update disputeKitData.{ tokenGate, tokenId, isTokenGateValid }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Pre-merge checks (3 passed)✅ Passed checks (3 passed)
✨ Finishing Touches
🧪 Generate unit tests
Comment |
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx (1)
34-48: Consider memoization optimizationThe voting and eligibility feature flags are recomputed on every render. While the computation is light, these could be combined into a single
useMemofor better organization.- // if supported dispute kits have Classic and Shutter - const showVotingFeatures = useMemo(() => { - return ( - disputeKitOptions.some((dk) => dk.text === DisputeKits.Classic) && - disputeKitOptions.some((dk) => dk.text === DisputeKits.Shutter) - ); - }, [disputeKitOptions]); - - // if supported dispute kits have Classic, TokenGated, TokenGatedShutter - const showEligibilityFeatures = useMemo(() => { - return ( - disputeKitOptions.some((dk) => dk.text === DisputeKits.Classic) && - disputeKitOptions.some((dk) => dk.text === DisputeKits.GatedShutter) && - disputeKitOptions.some((dk) => dk.text === DisputeKits.Gated) - ); - }, [disputeKitOptions]); + const { showVotingFeatures, showEligibilityFeatures } = useMemo(() => { + const hasClassic = disputeKitOptions.some((dk) => dk.text === DisputeKits.Classic); + const hasShutter = disputeKitOptions.some((dk) => dk.text === DisputeKits.Shutter); + const hasGated = disputeKitOptions.some((dk) => dk.text === DisputeKits.Gated); + const hasGatedShutter = disputeKitOptions.some((dk) => dk.text === DisputeKits.GatedShutter); + + return { + showVotingFeatures: hasClassic && hasShutter, + showEligibilityFeatures: hasClassic && hasGatedShutter && hasGated, + }; + }, [disputeKitOptions]);web/src/pages/Resolver/Parameters/Court/index.tsx (1)
59-63: Redundant court ID comparisonThe check for
disputeData.courtId !== courtIdis unnecessary since React will handle the state update optimization.Simplify the handler:
const handleCourtChange = (courtId: string) => { - if (disputeData.courtId !== courtId) { - setDisputeData({ ...disputeData, courtId, disputeKitId: undefined }); - } + setDisputeData({ ...disputeData, courtId, disputeKitId: undefined }); };web/src/pages/Resolver/Parameters/Court/FeatureSelection/JurorEligibility.tsx (1)
179-179: Typo in subtitle textThere's an unnecessary period at the end of the question.
- <SubTitle>Who can be selected as a juror?.</SubTitle> + <SubTitle>Who can be selected as a juror?</SubTitle>
📜 Review details
Configuration used: CodeRabbit UI
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 sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
web/src/context/NewDisputeContext.tsx(6 hunks)web/src/pages/Resolver/Parameters/Court.tsx(0 hunks)web/src/pages/Resolver/Parameters/Court/FeatureSelection/JurorEligibility.tsx(1 hunks)web/src/pages/Resolver/Parameters/Court/FeatureSelection/ShieldedVoting.tsx(1 hunks)web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx(1 hunks)web/src/pages/Resolver/Parameters/Court/index.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- web/src/pages/Resolver/Parameters/Court.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-10-28T05:55:12.728Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1716
File: web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx:29-42
Timestamp: 2024-10-28T05:55:12.728Z
Learning: In the `CustomContextInputs` component located at `web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx`, the `DisputeRequestParams` array is used to exclude certain variables from the custom input since they are already provided in a preceding component. Therefore, converting it to a type is unnecessary.
Applied to files:
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsxweb/src/context/NewDisputeContext.tsx
📚 Learning: 2024-10-14T13:58:25.708Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: web/src/hooks/queries/usePopulatedDisputeData.ts:58-61
Timestamp: 2024-10-14T13:58:25.708Z
Learning: In `web/src/hooks/queries/usePopulatedDisputeData.ts`, the query and subsequent logic only execute when `disputeData.dispute?.arbitrableChainId` and `disputeData.dispute?.externalDisputeId` are defined, so `initialContext` properties based on these values are safe to use without additional null checks.
Applied to files:
web/src/context/NewDisputeContext.tsx
📚 Learning: 2025-05-15T06:50:40.859Z
Learnt from: tractorss
PR: kleros/kleros-v2#1982
File: web/src/pages/Resolver/Landing/index.tsx:62-62
Timestamp: 2025-05-15T06:50:40.859Z
Learning: In the Landing component, it's safe to pass `dispute?.dispute?.arbitrated.id as 0x${string}` to `usePopulatedDisputeData` without additional null checks because the hook internally handles undefined parameters through its `isEnabled` flag and won't execute the query unless all required data is available.
Applied to files:
web/src/context/NewDisputeContext.tsx
📚 Learning: 2024-06-27T10:11:54.861Z
Learnt from: nikhilverma360
PR: kleros/kleros-v2#1632
File: web/src/components/DisputeView/DisputeInfo/DisputeInfoList.tsx:37-42
Timestamp: 2024-06-27T10:11:54.861Z
Learning: `useMemo` is used in `DisputeInfoList` to optimize the rendering of `FieldItems` based on changes in `fieldItems`, ensuring that the mapping and truncation operation are only performed when necessary.
Applied to files:
web/src/context/NewDisputeContext.tsx
🧬 Code graph analysis (5)
web/src/pages/Resolver/Parameters/Court/index.tsx (5)
web-devtools/src/styles/responsiveSize.ts (1)
responsiveSize(9-12)web/src/context/NewDisputeContext.tsx (1)
useNewDisputeContext(118-124)web/src/hooks/queries/useCourtTree.ts (2)
useCourtTree(39-47)rootCourtToItems(55-62)web/src/components/StyledSkeleton.tsx (1)
StyledSkeleton(8-10)web/src/pages/Resolver/Parameters/Court.tsx (1)
useNewDisputeContext(170-354)
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx (1)
web/src/context/NewDisputeContext.tsx (1)
useNewDisputeContext(118-124)
web/src/pages/Resolver/Parameters/Court/FeatureSelection/JurorEligibility.tsx (2)
web/src/context/NewDisputeContext.tsx (2)
useNewDisputeContext(118-124)IGatedDisputeData(69-75)web/src/hooks/useTokenAddressValidation.ts (2)
useERC20ERC721Validation(75-87)useERC1155Validation(95-104)
web/src/context/NewDisputeContext.tsx (2)
web/src/hooks/queries/useSupportedDisputeKits.ts (1)
useSupportedDisputeKits(19-32)web/src/hooks/useDisputeKitAddresses.ts (1)
useDisputeKitAddressesAll(105-163)
web/src/pages/Resolver/Parameters/Court/FeatureSelection/ShieldedVoting.tsx (2)
web-devtools/src/styles/Theme.tsx (1)
theme(3-10)web/src/context/NewDisputeContext.tsx (1)
useNewDisputeContext(118-124)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: Mend Security Check
🔇 Additional comments (3)
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx (1)
50-59: Missing dependencies in useEffect and potential race conditionThe
useEffectis missingdisputeDataandsetDisputeDatafrom its dependency array, which could lead to stale closures. Additionally, there's a potential race condition where multiple rapid changes todisputeKitOptionscould cause state inconsistencies.Apply this fix to include missing dependencies:
useEffect(() => { - // there's only one, NOTE: what happens when here only TokenGated is support? we need the value + // When there's only one dispute kit option available, auto-select it if (disputeKitOptions.length === 1) { const disputeKit = disputeKitOptions[0]; setDisputeData({ ...disputeData, disputeKitId: disputeKit.value }); setShowFeatures(false); } else { setShowFeatures(true); } - }, [disputeKitOptions]); + }, [disputeKitOptions, disputeData, setDisputeData]);⛔ Skipped due to learnings
Learnt from: tractorss PR: kleros/kleros-v2#1982 File: web/src/pages/Resolver/Parameters/NotablePersons/PersonFields.tsx:64-0 Timestamp: 2025-05-09T13:39:15.086Z Learning: In PersonFields.tsx, the useEffect hook for address validation intentionally uses an empty dependency array to run only once on component mount. This is specifically designed for the dispute duplication flow when aliasesArray is already populated with addresses that need initial validation.Learnt from: jaybuidl PR: kleros/kleros-v2#1582 File: web-devtools/src/app/(main)/ruler/SelectArbitrable.tsx:88-90 Timestamp: 2024-10-09T10:22:41.474Z Learning: Next.js recommends using the `useEffect` hook to set `isClient` and using `suppressHydrationWarning` as a workaround for handling hydration inconsistencies, especially when dealing with data like `knownArbitrables` that may differ between server-side and client-side rendering. This approach is acceptable in TypeScript/React applications, such as in `web-devtools/src/app/(main)/ruler/SelectArbitrable.tsx`.Learnt from: tractorss PR: kleros/kleros-v2#1982 File: web/src/pages/Resolver/Landing/index.tsx:0-0 Timestamp: 2025-05-15T06:50:45.650Z Learning: In the Kleros V2 codebase, it's acceptable to use ESLint disable comments for dependency arrays in useEffect hooks when including certain dependencies (like state that is being updated within the effect) would cause infinite loops.Learnt from: tractorss PR: kleros/kleros-v2#1982 File: web/src/pages/Resolver/Landing/index.tsx:62-62 Timestamp: 2025-05-15T06:50:40.859Z Learning: In the Landing component, it's safe to pass `dispute?.dispute?.arbitrated.id as 0x${string}` to `usePopulatedDisputeData` without additional null checks because the hook internally handles undefined parameters through its `isEnabled` flag and won't execute the query unless all required data is available.Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1703 File: web/src/hooks/queries/usePopulatedDisputeData.ts:58-61 Timestamp: 2024-10-14T13:58:25.708Z Learning: In `web/src/hooks/queries/usePopulatedDisputeData.ts`, the query and subsequent logic only execute when `disputeData.dispute?.arbitrableChainId` and `disputeData.dispute?.externalDisputeId` are defined, so `initialContext` properties based on these values are safe to use without additional null checks.Learnt from: kemuru PR: kleros/kleros-v2#1774 File: web/src/components/CasesDisplay/index.tsx:61-61 Timestamp: 2024-12-06T13:04:50.495Z Learning: In `web/src/components/CasesDisplay/index.tsx`, the variables `numberDisputes` and `numberClosedDisputes` can sometimes be `NaN`, and should default to `0` using logical OR (`||`) to prevent display issues in the `StatsAndFilters` component.web/src/pages/Resolver/Parameters/Court/index.tsx (1)
71-71: Non-null assertion could throw runtime errorUsing the non-null assertion operator (
!) onpath.split("/").pop()is risky and could throw if the path format is unexpected.Add proper null checking:
- onSelect={(path: string | number) => typeof path === "string" && handleCourtChange(path.split("/").pop()!)} + onSelect={(path: string | number) => { + if (typeof path === "string") { + const courtId = path.split("/").pop(); + if (courtId) { + handleCourtChange(courtId); + } + } + }}⛔ Skipped due to learnings
Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1729 File: web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx:125-127 Timestamp: 2024-10-29T10:14:52.512Z Learning: In `web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx`, when `isEmailUpdateable` is false, `user?.emailUpdateableAt` is always defined. Therefore, using the non-null assertion `!` with `user?.emailUpdateableAt!` is acceptable because TypeScript may not infer its definiteness.web/src/pages/Resolver/Parameters/Court/FeatureSelection/JurorEligibility.tsx (1)
134-173: Missing dependencies and complex logic flowThe
useEffecthas missing dependencies and contains complex branching logic that could be simplified.Refactor for clarity and add missing dependencies:
useEffect(() => { + if (eligibilityType === undefined) return; + if (eligibilityType === EligibilityType.Classic) { const disputeKit = disputeKitOptions.find((dk) => dk.text === DisputeKits.Classic); - - setDisputeData({ ...disputeData, disputeKitId: disputeKit?.value, disputeKitData: undefined }); + if (disputeKit) { + setDisputeData({ ...disputeData, disputeKitId: disputeKit.value, disputeKitData: undefined }); + } } else if (eligibilityType === EligibilityType.GatedERC20 || eligibilityType === EligibilityType.GatedERC1155) { const disputeKitGated = disputeKitOptions.find((dk) => dk.text === DisputeKits.Gated); const disputeKitGatedShutter = disputeKitOptions.find((dk) => dk.text === DisputeKits.GatedShutter); - const currentDisputeKit = disputeKitOptions.find((dk) => dk.value === disputeData.disputeKitId); const disputeKitData: IGatedDisputeData = { ...(disputeData.disputeKitData as IGatedDisputeData), type: "gated", isERC1155: eligibilityType === EligibilityType.GatedERC1155, + tokenGate: (disputeData.disputeKitData as IGatedDisputeData)?.tokenGate || "", + tokenId: (disputeData.disputeKitData as IGatedDisputeData)?.tokenId || "0", }; + + let selectedKitId: number | undefined; // classic is selected, so here we change it to TokenGated if (currentDisputeKit?.text === DisputeKits.Classic) { - setDisputeData({ - ...disputeData, - disputeKitId: disputeKitGated?.value, - disputeKitData, - }); + selectedKitId = disputeKitGated?.value; } // shutter is selected, so here we change it to TokenGatedShutter else if (currentDisputeKit?.text === DisputeKits.Shutter) { - setDisputeData({ - ...disputeData, - disputeKitId: disputeKitGatedShutter?.value, - disputeKitData, - }); + selectedKitId = disputeKitGatedShutter?.value; } else { + selectedKitId = disputeKitGated?.value; + } + + if (selectedKitId !== undefined) { setDisputeData({ ...disputeData, - disputeKitId: disputeKitGated?.value, + disputeKitId: selectedKitId, disputeKitData, }); } } - }, [eligibilityType, disputeKitOptions]); + }, [eligibilityType, disputeKitOptions, disputeData, setDisputeData]);⛔ Skipped due to learnings
Learnt from: tractorss PR: kleros/kleros-v2#1982 File: web/src/pages/Resolver/Parameters/NotablePersons/PersonFields.tsx:64-0 Timestamp: 2025-05-09T13:39:15.086Z Learning: In PersonFields.tsx, the useEffect hook for address validation intentionally uses an empty dependency array to run only once on component mount. This is specifically designed for the dispute duplication flow when aliasesArray is already populated with addresses that need initial validation.Learnt from: jaybuidl PR: kleros/kleros-v2#1582 File: web-devtools/src/app/(main)/ruler/SelectArbitrable.tsx:88-90 Timestamp: 2024-10-09T10:22:41.474Z Learning: Next.js recommends using the `useEffect` hook to set `isClient` and using `suppressHydrationWarning` as a workaround for handling hydration inconsistencies, especially when dealing with data like `knownArbitrables` that may differ between server-side and client-side rendering. This approach is acceptable in TypeScript/React applications, such as in `web-devtools/src/app/(main)/ruler/SelectArbitrable.tsx`.Learnt from: tractorss PR: kleros/kleros-v2#1982 File: web/src/pages/Resolver/Landing/index.tsx:0-0 Timestamp: 2025-05-15T06:50:45.650Z Learning: In the Kleros V2 codebase, it's acceptable to use ESLint disable comments for dependency arrays in useEffect hooks when including certain dependencies (like state that is being updated within the effect) would cause infinite loops.Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1703 File: web/src/hooks/queries/usePopulatedDisputeData.ts:58-61 Timestamp: 2024-10-14T13:58:25.708Z Learning: In `web/src/hooks/queries/usePopulatedDisputeData.ts`, the query and subsequent logic only execute when `disputeData.dispute?.arbitrableChainId` and `disputeData.dispute?.externalDisputeId` are defined, so `initialContext` properties based on these values are safe to use without additional null checks.Learnt from: tractorss PR: kleros/kleros-v2#1982 File: web/src/pages/Resolver/Landing/index.tsx:62-62 Timestamp: 2025-05-15T06:50:40.859Z Learning: In the Landing component, it's safe to pass `dispute?.dispute?.arbitrated.id as 0x${string}` to `usePopulatedDisputeData` without additional null checks because the hook internally handles undefined parameters through its `isEnabled` flag and won't execute the query unless all required data is available.Learnt from: nikhilverma360 PR: kleros/kleros-v2#1632 File: web/src/components/DisputeView/DisputeInfo/DisputeInfoList.tsx:37-42 Timestamp: 2024-06-27T10:11:54.861Z Learning: `useMemo` is used in `DisputeInfoList` to optimize the rendering of `FieldItems` based on changes in `fieldItems`, ensuring that the mapping and truncation operation are only performed when necessary.
web/src/pages/Resolver/Parameters/Court/FeatureSelection/JurorEligibility.tsx
Outdated
Show resolved
Hide resolved
web/src/pages/Resolver/Parameters/Court/FeatureSelection/JurorEligibility.tsx
Outdated
Show resolved
Hide resolved
web/src/pages/Resolver/Parameters/Court/FeatureSelection/ShieldedVoting.tsx
Outdated
Show resolved
Hide resolved
web/src/pages/Resolver/Parameters/Court/FeatureSelection/ShieldedVoting.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (16)
web/src/hooks/queries/useCourtDetails.ts (2)
39-41: Prefer array queryKey for stability.Array keys reduce accidental collisions and improve devtools readability.
- queryKey: [`courtDetails${id}`], + queryKey: ["courtDetails", id],
35-36: Enable flag should guard empty strings.
id !== undefinedtreats""as enabled; use a boolean cast.- const isEnabled = id !== undefined; + const isEnabled = Boolean(id);web/src/consts/disputeFeature.ts (5)
16-18: Tighten typing of FeatureGroups to enum keys.Use
Record<Group, Features[]>to prevent stray string keys and improve exhaustiveness.-export type FeatureGroups = Record<string, Features[]>; +export type FeatureGroups = Record<Group, Features[]>;
39-47: Docs nit: fix typos and clarify single-selection behavior.Minor spelling and clarity improvements.
-// withing a group only one feature can be selected, we deselect the other one when a new one is selected +// within a group only one feature can be selected; we deselect the others when a new one is selected. -// DEV: the order of features in array , determine the order the radios appear on UI +// DEV: the order of features in the array determines the order radios appear in the UI.
144-172: ensureValidSmart: unify prefix checks to a single helper.
isValidOrPrefixandcanStillLeadToMatchimplement similar “prefix-of” logic. Extract one helper to avoid drift.- function isValidOrPrefix(candidate: Features[]): boolean { + function isValidOrPrefix(candidate: Features[]): boolean { return ( findMatchingKits(candidate, kits).length > 0 || - kits.some((kit) => - kit.featureSets.some( - (set) => candidate.every((f) => set.includes(f)) // prefix check - ) - ) + canStillLeadToMatch(candidate, kits) ); }
188-210: Avoid disabling the currently selected option.Current logic can mark the checked option as disabled (because toggling it would create an invalid state). This yields “checked + disabled,” which is confusing UX for radios. Keep the selected option enabled; only disable alternatives that cannot be selected.
- const candidate = toggleFeature(selected, feature, groups); + const candidate = toggleFeature(selected, feature, groups); + const isCurrentlySelected = selected.includes(feature); ... - if (!valid) { + if (!valid && !isCurrentlySelected) { disabled.add(feature); }
44-47: Enforce “feature appears in only one group” at dev-time.Add a dev-time assertion to catch duplicates early.
// Add near featureGroups definition (DEV only) if (process.env.NODE_ENV !== "production") { const seen = new Map<Features, string>(); for (const [groupName, feats] of Object.entries(featureGroups)) { for (const f of feats) { const prev = seen.get(f); if (prev && prev !== groupName) { throw new Error(`Feature ${f} appears in multiple groups: ${prev} and ${groupName}`); } seen.set(f, groupName); } } }web/src/components/DisputeFeatures/Features/ClassicVote.tsx (2)
14-27: Prevent label flicker while loading court details.Until the query resolves, the label defaults to the non-commit variant, causing a brief mislabel. Gate on
isLoadingto render a neutral label or keep the last known value.- const { data: courtData } = useCourtDetails(disputeData.courtId); - const isCommitEnabled = Boolean(courtData?.court?.hiddenVotes); + const { data: courtData, isLoading } = useCourtDetails(disputeData.courtId); + const isCommitEnabled = !isLoading && Boolean(courtData?.court?.hiddenVotes); ... - <StyledRadio label={isCommitEnabled ? "Two-steps commit-reveal" : "Classic one step voting"} small {...props} /> + <StyledRadio + label={ + isLoading + ? "Voting mode" + : isCommitEnabled + ? "Two-step commit-reveal" + : "Classic one-step voting" + } + small + {...props} + />
16-24: Copy nits: tighten tooltip grammar.Minor clarity and consistency improvements.
- ? `The jurors' votes are hidden. - Nobody can see them before the voting period completes. - (It takes place in two steps commit-reveal)` - : `The jurors' votes are not hidden. - Everybody can see the justification and voted choice before the voting period completes.` + ? `Jurors' votes are hidden. Nobody can see them until the voting period ends. (Two-step commit–reveal)` + : `Jurors' votes are not hidden. Everyone can see the justification and vote choice before the voting period ends.`web/src/pages/Resolver/Parameters/Court/FeatureSelection/FeatureSkeleton.tsx (1)
1-6: Use theme colors for Skeleton for visual consistency.
Hook into the app theme and wrap with SkeletonTheme.-import styled from "styled-components"; +import styled, { useTheme } from "styled-components"; -import Skeleton from "react-loading-skeleton"; +import Skeleton, { SkeletonTheme } from "react-loading-skeleton"; @@ - return ( - <Container> + const theme = useTheme(); + return ( + <SkeletonTheme + baseColor={theme.klerosUIComponentsSkeletonBackground} + highlightColor={theme.klerosUIComponentsSkeletonHighlight} + > + <Container> @@ - </Container> + </Container> + </SkeletonTheme> );Also applies to: 36-44
web/src/components/DisputeFeatures/Features/GatedErc20.tsx (1)
67-79: Normalize input to reduce accidental whitespace issues.
Trim address on change.- const handleTokenAddressChange = (event: React.ChangeEvent<HTMLInputElement>) => { + const handleTokenAddressChange = (event: React.ChangeEvent<HTMLInputElement>) => { + const next = event.target.value.trim(); const currentData = disputeData.disputeKitData as IGatedDisputeData; @@ - tokenGate: event.target.value, + tokenGate: next, isTokenGateValid: null, // Reset validation state when address changesweb/src/context/NewDisputeContext.tsx (1)
137-143: Reset selectedFeatures along with dispute data to avoid stale UI state.
Currently resetDisputeData and the route-leave cleanup don’t clear selectedFeatures.const resetDisputeData = useCallback(() => { const freshData = getInitialDisputeData(); setDisputeData(freshData); setBatchSize(MIN_DISPUTE_BATCH_SIZE); + setSelectedFeatures([]); // eslint-disable-next-line react-hooks/exhaustive-deps }, []); @@ useEffect(() => { // Cleanup function to clear local storage when user leaves the route if (location.pathname.includes("/resolver") || location.pathname.includes("/attachment")) return; resetDisputeData(); + setSelectedFeatures([]); // eslint-disable-next-line react-hooks/exhaustive-deps }, [location.pathname]);Also applies to: 144-151
web/src/components/DisputeFeatures/GroupsUI.tsx (2)
6-13: Use flex-start for broader browser support.
CSS align-items: start has patchy support; flex-start is safer.- align-items: start; + align-items: flex-start;
47-49: Fix punctuation in subtitle.- <SubTitle>Who can be selected as a juror?.</SubTitle> + <SubTitle>Who can be selected as a juror?</SubTitle>web/src/components/DisputeFeatures/Features/GatedErc1155.tsx (1)
109-114: Constrain tokenId input to numbers.
Prevents accidental non-numeric entry.- <StyledField + <StyledField dir="auto" onChange={handleTokenIdChange} value={(disputeData.disputeKitData as IGatedDisputeData)?.tokenId ?? "0"} placeholder="Eg. 1" + type="number" + min="0" />web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx (1)
55-73: useEffect without deps will drift if disputeKitId changes.
Either add deps or explicitly disable the lint rule with justification.Option A — add deps:
- useEffect(() => { + useEffect(() => { if (!isUndefined(disputeData?.disputeKitId)) { @@ - }, []); + }, [disputeData?.disputeKitId, disputeData?.disputeKitData, setSelected]);Option B — keep once-on-mount but document intent:
- useEffect(() => { + // eslint-disable-next-line react-hooks/exhaustive-deps + useEffect(() => { if (!isUndefined(disputeData?.disputeKitId)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
web/src/components/DisputeFeatures/Features/ClassicVote.tsx(1 hunks)web/src/components/DisputeFeatures/Features/GatedErc1155.tsx(1 hunks)web/src/components/DisputeFeatures/Features/GatedErc20.tsx(1 hunks)web/src/components/DisputeFeatures/Features/index.tsx(1 hunks)web/src/components/DisputeFeatures/GroupsUI.tsx(1 hunks)web/src/consts/disputeFeature.ts(1 hunks)web/src/context/NewDisputeContext.tsx(6 hunks)web/src/hooks/queries/useCourtDetails.ts(1 hunks)web/src/pages/Resolver/Parameters/Court/FeatureSelection/FeatureSkeleton.tsx(1 hunks)web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx(1 hunks)web/src/pages/Resolver/Parameters/Court/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/pages/Resolver/Parameters/Court/index.tsx
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2024-10-28T05:55:12.728Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1716
File: web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx:29-42
Timestamp: 2024-10-28T05:55:12.728Z
Learning: In the `CustomContextInputs` component located at `web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx`, the `DisputeRequestParams` array is used to exclude certain variables from the custom input since they are already provided in a preceding component. Therefore, converting it to a type is unnecessary.
Applied to files:
web/src/consts/disputeFeature.tsweb/src/components/DisputeFeatures/Features/index.tsxweb/src/context/NewDisputeContext.tsx
📚 Learning: 2024-10-14T13:58:25.708Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: web/src/hooks/queries/usePopulatedDisputeData.ts:58-61
Timestamp: 2024-10-14T13:58:25.708Z
Learning: In `web/src/hooks/queries/usePopulatedDisputeData.ts`, the query and subsequent logic only execute when `disputeData.dispute?.arbitrableChainId` and `disputeData.dispute?.externalDisputeId` are defined, so `initialContext` properties based on these values are safe to use without additional null checks.
Applied to files:
web/src/context/NewDisputeContext.tsx
📚 Learning: 2025-05-15T06:50:40.859Z
Learnt from: tractorss
PR: kleros/kleros-v2#1982
File: web/src/pages/Resolver/Landing/index.tsx:62-62
Timestamp: 2025-05-15T06:50:40.859Z
Learning: In the Landing component, it's safe to pass `dispute?.dispute?.arbitrated.id as 0x${string}` to `usePopulatedDisputeData` without additional null checks because the hook internally handles undefined parameters through its `isEnabled` flag and won't execute the query unless all required data is available.
Applied to files:
web/src/context/NewDisputeContext.tsx
📚 Learning: 2024-12-06T13:04:50.495Z
Learnt from: kemuru
PR: kleros/kleros-v2#1774
File: web/src/components/CasesDisplay/index.tsx:61-61
Timestamp: 2024-12-06T13:04:50.495Z
Learning: In `web/src/components/CasesDisplay/index.tsx`, the variables `numberDisputes` and `numberClosedDisputes` can sometimes be `NaN`, and should default to `0` using logical OR (`||`) to prevent display issues in the `StatsAndFilters` component.
Applied to files:
web/src/context/NewDisputeContext.tsx
📚 Learning: 2024-06-27T10:11:54.861Z
Learnt from: nikhilverma360
PR: kleros/kleros-v2#1632
File: web/src/components/DisputeView/DisputeInfo/DisputeInfoList.tsx:37-42
Timestamp: 2024-06-27T10:11:54.861Z
Learning: `useMemo` is used in `DisputeInfoList` to optimize the rendering of `FieldItems` based on changes in `fieldItems`, ensuring that the mapping and truncation operation are only performed when necessary.
Applied to files:
web/src/context/NewDisputeContext.tsx
📚 Learning: 2025-05-09T13:39:15.086Z
Learnt from: tractorss
PR: kleros/kleros-v2#1982
File: web/src/pages/Resolver/Parameters/NotablePersons/PersonFields.tsx:64-0
Timestamp: 2025-05-09T13:39:15.086Z
Learning: In PersonFields.tsx, the useEffect hook for address validation intentionally uses an empty dependency array to run only once on component mount. This is specifically designed for the dispute duplication flow when aliasesArray is already populated with addresses that need initial validation.
Applied to files:
web/src/context/NewDisputeContext.tsx
🧬 Code graph analysis (7)
web/src/components/DisputeFeatures/Features/GatedErc20.tsx (3)
web/src/components/DisputeFeatures/Features/index.tsx (2)
RadioInput(14-20)StyledRadio(24-28)web/src/context/NewDisputeContext.tsx (2)
useNewDisputeContext(118-124)IGatedDisputeData(68-74)web/src/hooks/useTokenAddressValidation.ts (1)
useERC20ERC721Validation(75-87)
web/src/components/DisputeFeatures/GroupsUI.tsx (1)
web-devtools/src/styles/Theme.tsx (1)
theme(3-10)
web/src/components/DisputeFeatures/Features/index.tsx (1)
web-devtools/src/styles/Theme.tsx (1)
theme(3-10)
web/src/components/DisputeFeatures/Features/GatedErc1155.tsx (3)
web/src/components/DisputeFeatures/Features/index.tsx (2)
RadioInput(14-20)StyledRadio(24-28)web/src/context/NewDisputeContext.tsx (2)
useNewDisputeContext(118-124)IGatedDisputeData(68-74)web/src/hooks/useTokenAddressValidation.ts (1)
useERC1155Validation(95-104)
web/src/components/DisputeFeatures/Features/ClassicVote.tsx (3)
web/src/components/DisputeFeatures/Features/index.tsx (2)
RadioInput(14-20)StyledRadio(24-28)web/src/context/NewDisputeContext.tsx (1)
useNewDisputeContext(118-124)web/src/hooks/queries/useCourtDetails.ts (1)
useCourtDetails(34-46)
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx (5)
web/src/context/NewDisputeContext.tsx (2)
useNewDisputeContext(118-124)IGatedDisputeData(68-74)web/src/hooks/queries/useSupportedDisputeKits.ts (1)
useSupportedDisputeKits(19-32)web/src/consts/disputeFeature.ts (6)
disputeKits(51-77)featureGroups(44-47)getDisabledOptions(188-210)findMatchingKits(119-128)toggleFeature(99-114)ensureValidSmart(144-172)web/src/components/DisputeFeatures/GroupsUI.tsx (1)
GroupsUI(34-53)web/src/components/DisputeFeatures/Features/index.tsx (1)
FeatureUIs(30-50)
web/src/context/NewDisputeContext.tsx (1)
web/src/consts/disputeFeature.ts (1)
DisputeKits(37-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
- GitHub Check: Mend Security Check
🔇 Additional comments (4)
web/src/hooks/queries/useCourtDetails.ts (1)
29-29: EnsurehiddenVotesis in schema and regenerate types
hiddenVotesis declared onCourtin all subgraph schemas (core, core-neo, core-university). Runyarn workspace @kleros/kleros-v2-web generateto regenerate GraphQL types so
CourtDetailsQueryincludeshiddenVotes.web/src/pages/Resolver/Parameters/Court/FeatureSelection/FeatureSkeleton.tsx (1)
35-45: LGTM — simple, readable skeleton.web/src/context/NewDisputeContext.tsx (2)
12-13: No changes needed—DisputeKitsenum is correctly imported fromsrc/consts/index.ts.
12-13: Type mix-up: DisputeKits here refers to a list type elsewhere; use the enum for labels.
This will otherwise cause TS errors and confusion with the DisputeKits alias in consts/disputeFeature.ts.-import { DisputeKits } from "src/consts"; +// Rename import to avoid collision with disputeFeature's DisputeKits (array alias) +import { DisputeKits as DisputeKitLabel } from "consts"; @@ -export type DisputeKitOption = { - text: DisputeKits; +export type DisputeKitOption = { + text: DisputeKitLabel; value: number; gated: boolean; };Also applies to: 30-35
⛔ Skipped due to learnings
Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1716 File: web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx:29-42 Timestamp: 2024-10-28T05:55:12.728Z Learning: In the `CustomContextInputs` component located at `web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx`, the `DisputeRequestParams` array is used to exclude certain variables from the custom input since they are already provided in a preceding component. Therefore, converting it to a type is unnecessary.
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx (3)
6-15: ImportGrouptype for type-safe map indexing.Add the
Grouptype to your imports to enable type-safe indexing intoGroupsUI.import { disputeKits, ensureValidSmart, featureGroups, Features, + Group, findMatchingKits, getDisabledOptions, getVisibleFeaturesForCourt, toggleFeature, } from "consts/disputeFeature";
121-131: Use functional updater to avoid stale closure issues.The current implementation reads
disputeDatafrom the outer scope which can lead to stale data issues if the state changes between renders.Since
setDisputeDataonly accepts direct state values (as noted in learnings), you'll need to ensure you're working with the latest state. Consider using auseRefto track the latest disputeData or restructure the logic to avoid potential race conditions.
139-159: Add React keys and type-safe indexing for map iterations.The current implementation is missing React keys for list items and uses unsafe string indexing into
GroupsUIandFeatureUIs.{Object.entries(courtGroups).length > 0 ? ( - Object.entries(courtGroups).map(([groupName, features], index) => ( - <> + Object.entries(courtGroups).map(([groupName, features], index) => { + const group = groupName as Group; + const GroupWrapper = GroupsUI[group]; + return ( + <Fragment key={group}> - {GroupsUI[groupName]({ + {GroupWrapper({ children: ( - <Fragment key={groupName}> + <Fragment> {features.map((feature) => - FeatureUIs[feature]({ + FeatureUIs[feature]({ + key: feature, name: groupName, checked: selected.includes(feature), disabled: disabled.has(feature), onClick: () => handleToggle(feature), value: feature, }) )} </Fragment> ), })} {index !== Object.entries(courtGroups).length - 1 ? <Separator /> : null} - </> - )) + </Fragment> + ); + }) ) : (
🧹 Nitpick comments (3)
web/src/consts/disputeFeature.ts (3)
35-36: Fix typo in comment.// groups -// withing a group only one feature can be selected, we deselect the other one when a new one is selected +// within a group only one feature can be selected, we deselect the other one when a new one is selected
86-101: Consider renaming the underscore variable for clarity.While valid JavaScript, using
_for the group name reduces readability.export function toggleFeature(selected: Features[], feature: Features, groups: FeatureGroups): Features[] { - const group = Object.entries(groups).find(([_, feats]) => feats.includes(feature)); + const group = Object.entries(groups).find(([groupName, feats]) => feats.includes(feature)); if (!group) return selected; // not found in any group - const [_, features] = group; // <= this is the group we are making selection in currently + const [groupName, features] = group; // <= this is the group we are making selection in currently
147-147: Consider renaming underscore variables in loops.Multiple functions use
_for unused destructured values, which reduces code clarity.// Line 147 - for (const [_, features] of Object.entries(groups)) { + for (const [groupName, features] of Object.entries(groups)) { // Line 180 - for (const [_, features] of Object.entries(groups)) { + for (const [groupName, features] of Object.entries(groups)) {Also applies to: 180-180
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
web/src/components/DisputeFeatures/Features/ClassicVote.tsx(1 hunks)web/src/components/DisputeFeatures/Features/GatedErc1155.tsx(1 hunks)web/src/components/DisputeFeatures/Features/GatedErc20.tsx(1 hunks)web/src/components/DisputeFeatures/Features/index.tsx(1 hunks)web/src/consts/disputeFeature.ts(1 hunks)web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- web/src/components/DisputeFeatures/Features/GatedErc20.tsx
- web/src/components/DisputeFeatures/Features/ClassicVote.tsx
- web/src/components/DisputeFeatures/Features/GatedErc1155.tsx
- web/src/components/DisputeFeatures/Features/index.tsx
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-10-28T05:55:12.728Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1716
File: web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx:29-42
Timestamp: 2024-10-28T05:55:12.728Z
Learning: In the `CustomContextInputs` component located at `web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx`, the `DisputeRequestParams` array is used to exclude certain variables from the custom input since they are already provided in a preceding component. Therefore, converting it to a type is unnecessary.
Applied to files:
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsxweb/src/consts/disputeFeature.ts
📚 Learning: 2025-09-09T13:33:46.871Z
Learnt from: tractorss
PR: kleros/kleros-v2#2117
File: web/src/components/DisputeFeatures/Features/GatedErc1155.tsx:51-66
Timestamp: 2025-09-09T13:33:46.871Z
Learning: The `setDisputeData` function in `NewDisputeContext` at `web/src/context/NewDisputeContext.tsx` has signature `(disputeData: IDisputeData) => void` and only accepts direct state values, not functional updates like standard React state setters. It cannot be used with the pattern `setDisputeData((prev) => ...)`.
Applied to files:
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx
📚 Learning: 2024-10-14T13:58:25.708Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: web/src/hooks/queries/usePopulatedDisputeData.ts:58-61
Timestamp: 2024-10-14T13:58:25.708Z
Learning: In `web/src/hooks/queries/usePopulatedDisputeData.ts`, the query and subsequent logic only execute when `disputeData.dispute?.arbitrableChainId` and `disputeData.dispute?.externalDisputeId` are defined, so `initialContext` properties based on these values are safe to use without additional null checks.
Applied to files:
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx
📚 Learning: 2025-05-15T06:50:40.859Z
Learnt from: tractorss
PR: kleros/kleros-v2#1982
File: web/src/pages/Resolver/Landing/index.tsx:62-62
Timestamp: 2025-05-15T06:50:40.859Z
Learning: In the Landing component, it's safe to pass `dispute?.dispute?.arbitrated.id as 0x${string}` to `usePopulatedDisputeData` without additional null checks because the hook internally handles undefined parameters through its `isEnabled` flag and won't execute the query unless all required data is available.
Applied to files:
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx
📚 Learning: 2024-12-06T13:04:50.495Z
Learnt from: kemuru
PR: kleros/kleros-v2#1774
File: web/src/components/CasesDisplay/index.tsx:61-61
Timestamp: 2024-12-06T13:04:50.495Z
Learning: In `web/src/components/CasesDisplay/index.tsx`, the variables `numberDisputes` and `numberClosedDisputes` can sometimes be `NaN`, and should default to `0` using logical OR (`||`) to prevent display issues in the `StatsAndFilters` component.
Applied to files:
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx
🧬 Code graph analysis (1)
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx (5)
web/src/context/NewDisputeContext.tsx (2)
useNewDisputeContext(118-124)IGatedDisputeData(68-74)web/src/hooks/queries/useSupportedDisputeKits.ts (1)
useSupportedDisputeKits(19-32)web/src/consts/disputeFeature.ts (7)
disputeKits(46-70)getVisibleFeaturesForCourt(200-226)featureGroups(39-42)getDisabledOptions(172-194)findMatchingKits(106-112)toggleFeature(86-101)ensureValidSmart(128-156)web/src/components/DisputeFeatures/GroupsUI.tsx (1)
GroupsUI(34-53)web/src/components/DisputeFeatures/Features/index.tsx (1)
FeatureUIs(30-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (4)
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx (1)
104-112: Ensure auto-selection logic is isolated and covered by tests
Consolidate or clearly separate the two useEffect blocks (initial selection on lines 56–73 vs. auto-selection on lines 107–112) to prevent overlapping updates, and add unit tests inFeatureSelectioncovering:
- single
disputeKit+featureSet(auto-select)- multiple options (no auto-select)
web/src/consts/disputeFeature.ts (3)
1-14: LGTM! Well-structured enums for feature categorization.The enums provide clear categorization of features with good documentation explaining the atomic nature of features.
158-166: Good implementation of prefix matching logic.The
canStillLeadToMatchhelper correctly implements the subset check to determine if a partial selection could lead to a valid complete selection.
200-226: Verify supportedDisputeKits configuration for all courts
We couldn’t locate where courts are seeded with supportedDisputeKits arrays—confirm each court is configured with a valid (non-empty) list of dispute kit IDs so getVisibleFeaturesForCourt handles all edge cases.
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx (3)
6-15: Import Group for type-safe indexing into GroupsUI/FeatureGroups.
Prepares typed access later in render.import { disputeKits, ensureValidSmart, featureGroups, Features, + Group, findMatchingKits, getDisabledOptions, getVisibleFeaturesForCourt, toggleFeature, } from "consts/disputeFeature";
139-159: Add React keys and type-safe lookups; avoid recomputing entries.
Fixes list-key warnings and TS index errors; reduces redundant Object.entries calls.- {Object.entries(courtGroups).length > 0 ? ( - Object.entries(courtGroups).map(([groupName, features], index) => ( - <> - {GroupsUI[groupName]({ - children: ( - <Fragment key={groupName}> - {features.map((feature) => - FeatureUIs[feature]({ - name: groupName, - checked: selected.includes(feature), - disabled: disabled.has(feature), - onClick: () => handleToggle(feature), - value: feature, - }) - )} - </Fragment> - ), - })} - {index !== Object.entries(courtGroups).length - 1 ? <Separator /> : null} - </> - )) - ) : ( + {Object.entries(courtGroups as Record<Group, Features[]>).length > 0 ? ( + Object.entries(courtGroups as Record<Group, Features[]>).map(([groupName, features], index) => { + const group = groupName as Group; + const GroupWrapper = GroupsUI[group]; + return ( + <Fragment key={group}> + {GroupWrapper({ + children: ( + <Fragment> + {features.map((feature) => { + const UI = FeatureUIs[feature]; + return ( + <UI + key={feature} + name={group} + checked={selected.includes(feature)} + disabled={disabled.has(feature)} + onClick={() => handleToggle(feature)} + value={feature} + /> + ); + })} + </Fragment> + ), + })} + {index !== Object.entries(courtGroups).length - 1 ? <Separator /> : null} + </Fragment> + ); + }) + ) : ( <FeatureSkeleton /> )}
114-133: Prevent stale overwrites in setDisputeData; derive isERC1155; add deps.
Current effect captures a stale disputeData and can clobber newer fields. Per learning, setDisputeData is not a functional setter; fix by making updates idempotent and adding deps.-useEffect(() => { - // work of feature selection ends here by giving us the disputeKitId, - // any further checks we do separately, like for NextButton - // right now we don't have kits that can have same features, so we assume it will be 1 length array - if (matchingKits.length === 1) { - const selectedKit = matchingKits[0]; - - setDisputeData({ - ...disputeData, - disputeKitId: selectedKit.id, - disputeKitData: - selectedKit.type === "general" - ? undefined - : ({ ...disputeData.disputeKitData, type: selectedKit.type } as IGatedDisputeData), - }); - } else if (matchingKits.length === 0) { - setDisputeData({ ...disputeData, disputeKitId: undefined }); - } - // eslint-disable-next-line react-hooks/exhaustive-deps -}, [matchingKits]); +useEffect(() => { + // work of feature selection ends here by giving us the disputeKitId, + // any further checks we do separately, like for NextButton + if (matchingKits.length === 1) { + const selectedKit = matchingKits[0]; + const isGated = selectedKit.type === "gated"; + const isERC1155 = selected.includes(Features.GatedErc1155); + + const next = { + ...disputeData, + disputeKitId: selectedKit.id, + disputeKitData: isGated + ? ({ ...(disputeData.disputeKitData ?? {}), type: "gated", isERC1155 } as IGatedDisputeData) + : undefined, + }; + + const changed = + disputeData.disputeKitId !== next.disputeKitId || + (isGated + ? (disputeData.disputeKitData as IGatedDisputeData | undefined)?.isERC1155 !== isERC1155 || + (disputeData.disputeKitData as IGatedDisputeData | undefined)?.type !== "gated" + : disputeData.disputeKitData !== undefined); + + if (changed) setDisputeData(next); + } else if (matchingKits.length === 0) { + if (disputeData.disputeKitId !== undefined) { + setDisputeData({ ...disputeData, disputeKitId: undefined }); + } + } +}, [matchingKits, selected, disputeData, setDisputeData]);
🧹 Nitpick comments (3)
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx (3)
55-73: Make initial-selection effect react to kit changes and avoid overriding user choice.
Run when disputeKitId changes and only if nothing is selected.-useEffect(() => { - if (!isUndefined(disputeData?.disputeKitId)) { - const defaultKit = disputeKits.find((dk) => dk.id === disputeData.disputeKitId); - if (!defaultKit) return; - - // some kits like gated can have two feature sets, one for gatedERC20 and other for ERC1155 - if (defaultKit?.featureSets.length > 1) { - if ((disputeData?.disputeKitData as IGatedDisputeData)?.isERC1155) { - // defaultKit.featureSets[0][0] - is either Classic or Shutter - setSelected([defaultKit.featureSets[0][0], Features.GatedErc1155]); - } else { - setSelected([defaultKit.featureSets[0][0], Features.GatedErc20]); - } - } else if (defaultKit.featureSets.length === 1) { - setSelected(defaultKit.featureSets[0]); - } - } -}, []); +useEffect(() => { + if (isUndefined(disputeData?.disputeKitId) || selected.length > 0) return; + const defaultKit = disputeKits.find((dk) => dk.id === disputeData.disputeKitId); + if (!defaultKit) return; + // some kits like gated can have two feature sets, one for gatedERC20 and other for ERC1155 + if (defaultKit.featureSets.length > 1) { + const erc1155 = (disputeData?.disputeKitData as IGatedDisputeData | undefined)?.isERC1155; + setSelected([defaultKit.featureSets[0][0], erc1155 ? Features.GatedErc1155 : Features.GatedErc20]); + } else if (defaultKit.featureSets.length === 1) { + setSelected(defaultKit.featureSets[0]); + } +}, [disputeData?.disputeKitId, disputeData?.disputeKitData, selected.length, setSelected]);
104-113: Autoselect only when nothing is already selected.
Prevents late query results from clobbering user picks.-useEffect(() => { - // if only one disputeKit is found, and that dk has only one featureSEt to pick, then select by default - if (allowedDisputeKits.length === 1 && allowedDisputeKits[0].featureSets.length === 1) { - setSelected(allowedDisputeKits[0].featureSets[0]); - } -}, [allowedDisputeKits, setSelected]); +useEffect(() => { + // if only one disputeKit is found, and that dk has only one featureSet to pick, then select by default + if (selected.length === 0 && allowedDisputeKits.length === 1 && allowedDisputeKits[0].featureSets.length === 1) { + setSelected(allowedDisputeKits[0].featureSets[0]); + } +}, [allowedDisputeKits, selected, setSelected]);
129-131: Optionally clear disputeKitData when no kit matches.
Prevents stale gated state from leaking forward.- setDisputeData({ ...disputeData, disputeKitId: undefined }); + setDisputeData({ ...disputeData, disputeKitId: undefined, disputeKitData: undefined });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
web/src/components/DisputeFeatures/Features/ClassicVote.tsx(1 hunks)web/src/components/DisputeFeatures/Features/GatedErc1155.tsx(1 hunks)web/src/components/DisputeFeatures/Features/GatedErc20.tsx(1 hunks)web/src/components/DisputeFeatures/Features/index.tsx(1 hunks)web/src/components/DisputeFeatures/GroupsUI.tsx(1 hunks)web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- web/src/components/DisputeFeatures/Features/GatedErc1155.tsx
- web/src/components/DisputeFeatures/Features/index.tsx
- web/src/components/DisputeFeatures/Features/ClassicVote.tsx
- web/src/components/DisputeFeatures/GroupsUI.tsx
- web/src/components/DisputeFeatures/Features/GatedErc20.tsx
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-10-28T05:55:12.728Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1716
File: web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx:29-42
Timestamp: 2024-10-28T05:55:12.728Z
Learning: In the `CustomContextInputs` component located at `web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx`, the `DisputeRequestParams` array is used to exclude certain variables from the custom input since they are already provided in a preceding component. Therefore, converting it to a type is unnecessary.
Applied to files:
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx
📚 Learning: 2025-09-09T13:33:46.896Z
Learnt from: tractorss
PR: kleros/kleros-v2#2117
File: web/src/components/DisputeFeatures/Features/GatedErc1155.tsx:51-66
Timestamp: 2025-09-09T13:33:46.896Z
Learning: The `setDisputeData` function in `NewDisputeContext` at `web/src/context/NewDisputeContext.tsx` has signature `(disputeData: IDisputeData) => void` and only accepts direct state values, not functional updates like standard React state setters. It cannot be used with the pattern `setDisputeData((prev) => ...)`.
Applied to files:
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx
📚 Learning: 2024-10-14T13:58:25.708Z
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: web/src/hooks/queries/usePopulatedDisputeData.ts:58-61
Timestamp: 2024-10-14T13:58:25.708Z
Learning: In `web/src/hooks/queries/usePopulatedDisputeData.ts`, the query and subsequent logic only execute when `disputeData.dispute?.arbitrableChainId` and `disputeData.dispute?.externalDisputeId` are defined, so `initialContext` properties based on these values are safe to use without additional null checks.
Applied to files:
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx
📚 Learning: 2025-05-15T06:50:40.859Z
Learnt from: tractorss
PR: kleros/kleros-v2#1982
File: web/src/pages/Resolver/Landing/index.tsx:62-62
Timestamp: 2025-05-15T06:50:40.859Z
Learning: In the Landing component, it's safe to pass `dispute?.dispute?.arbitrated.id as 0x${string}` to `usePopulatedDisputeData` without additional null checks because the hook internally handles undefined parameters through its `isEnabled` flag and won't execute the query unless all required data is available.
Applied to files:
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx
📚 Learning: 2024-12-06T13:04:50.495Z
Learnt from: kemuru
PR: kleros/kleros-v2#1774
File: web/src/components/CasesDisplay/index.tsx:61-61
Timestamp: 2024-12-06T13:04:50.495Z
Learning: In `web/src/components/CasesDisplay/index.tsx`, the variables `numberDisputes` and `numberClosedDisputes` can sometimes be `NaN`, and should default to `0` using logical OR (`||`) to prevent display issues in the `StatsAndFilters` component.
Applied to files:
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx
🧬 Code graph analysis (1)
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx (5)
web/src/context/NewDisputeContext.tsx (2)
useNewDisputeContext(118-124)IGatedDisputeData(68-74)web/src/hooks/queries/useSupportedDisputeKits.ts (1)
useSupportedDisputeKits(19-32)web/src/consts/disputeFeature.ts (7)
disputeKits(46-70)getVisibleFeaturesForCourt(200-226)featureGroups(39-42)getDisabledOptions(172-194)findMatchingKits(106-112)toggleFeature(86-101)ensureValidSmart(128-156)web/src/components/DisputeFeatures/GroupsUI.tsx (1)
GroupsUI(34-53)web/src/components/DisputeFeatures/Features/index.tsx (1)
FeatureUIs(30-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (1)
web/src/pages/Resolver/Parameters/Court/FeatureSelection/index.tsx (1)
61-71: LGTM: corrected gated-kits condition.
Using featureSets.length > 1 fixes the ERC20/1155 dual-set case.
jaybuidl
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
WIP, initial implementation
PR-Codex overview
This PR introduces a new feature selection interface for dispute kits in a court arbitration system, enhancing the user experience by allowing jurors to select features related to voting and eligibility.
Detailed summary
FeatureSkeletonfor loading states.ClassicVoteandGroupsUIfor dispute features.FeatureSelectioncomponent to manage feature toggling.NewDisputeContextto include selected features.disputeFeatureconstants with new feature definitions.Summary by CodeRabbit
New Features
Refactor
Data