fix: crisp voting ux fixes [skip-line-limit]#1053
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughAdds vote-update support, session-scoped vote-status caching, a multi-step casting UI with progress indicator, multi-toast notifications, threading of previousCiphertext through proof generation (SDK/WASM/worker), new /round/:roundId page, and server endpoints/models for vote status and update handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Client as Vote Client (hooks/ctx)
participant Worker as Enclave/Worker
participant Chain as Blockchain
participant Server as Voting API
participant Cache as Session Cache
Note over User,UI: Page load → determine voted state
User->>UI: Open poll page
UI->>Client: request currentRoundId & hasVoted
alt cached valid
Client->>Cache: read(sessionId, roundId, address)
Cache-->>Client: has_voted
else cache miss/expired
Client->>Server: POST /voting/status (roundId,address)
Server-->>Client: VoteStatusResponse(has_voted, round_status)
Client->>Cache: store(status, ttl)
end
Client-->>UI: render voted state
Note over User,Chain: Cast or update vote flow
User->>UI: select option & submit
UI->>Client: castVoteWithProof(selection, hasVotedInCurrentRound)
Client->>Client: step = signing
Client->>Worker: request wallet signature
Worker-->>Client: signature
Client->>Client: step = encrypting
alt update case
Client->>Cache: fetch previousCiphertext
Cache-->>Client: previousCiphertext
end
Client->>Worker: generate_proof(..., previousCiphertext?)
Worker-->>Client: proof + ciphertext
Client->>Chain: broadcast(tx with proof, ciphertext, is_vote_update)
alt success
Chain-->>Client: tx confirmed
Client->>Cache: markVotedInRound
Client-->>UI: show success
else failure
Chain-->>Client: error
Client-->>UI: show persistent error toast
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2024-11-05T06:49:46.285ZApplied to files:
📚 Learning: 2024-10-16T09:51:10.038ZApplied to files:
⏰ 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). (9)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
❌ License Header Check Failed Some files are missing the required SPDX license header. Please add the following header to the beginning of all You can run Or run |
hmzakhalid
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
examples/CRISP/client/src/pages/HistoricPoll/HistoricPoll.tsx (1)
34-42: Theeslint-disablecomment is inappropriate—getPastPollsis not memoized and should be added to the dependency array.
getPastPollsis defined as a regular async function (lines 155–167 in VoteManagement.context.tsx) withoutuseCallbackwrapping. It is passed directly into the context value without memoization (line 225), meaning a new function reference is created on every provider re-render.The
react-hooks/exhaustive-depswarning is correct here. Either:
- Wrap
getPastPollswithuseCallbackin the provider, or- Add
getPastPollsto the dependency array:}, [votingRound, getPastPolls])The current code works by accident because the effect only needs to run when
votingRoundchanges, but this pattern is fragile and violates the eslint rule's intent.examples/CRISP/client/src/pages/Landing/components/DailyPoll.tsx (1)
79-84: Dynamic Tailwind class may not be included in production build.The dynamic class
border-${statusClass}-600/80uses string interpolation which Tailwind's JIT compiler cannot detect at build time. This could result in missing styles in production.Consider using explicit class mappings:
+ const borderClass = !isEnded ? 'border-lime-600/80' : 'border-red-600/80' + const bgClass = !isEnded ? 'bg-lime-400' : 'bg-red-400' + <div - className={`flex items-center space-x-2 rounded-lg border-2 border-${statusClass}-600/80 ${!isEnded ? 'bg-lime-400' : 'bg-red-400'} px-2 py-1 text-center font-bold uppercase leading-none text-white`} + className={`flex items-center space-x-2 rounded-lg border-2 ${borderClass} ${bgClass} px-2 py-1 text-center font-bold uppercase leading-none text-white`} >Alternatively, ensure these classes are safelisted in your Tailwind config.
examples/CRISP/server/src/server/routes/voting.rs (1)
141-152: Missing rollback on contract creation failure.If the contract creation fails after inserting a new voter address (lines 119-127), the voter address is not rolled back. This could leave the database in an inconsistent state where the user is marked as having voted but their vote was never broadcast.
let contract = match EnclaveContract::new( &CONFIG.http_rpc_url, &CONFIG.private_key, &CONFIG.enclave_address, ) .await { Ok(c) => c, Err(e) => { error!("[e3_id={}] Contract creation error: {:?}", vote.round_id, e); + // Rollback voter insertion if this was a new vote + if !is_vote_update { + if let Err(err) = repo.remove_voter_address(&vote.address).await { + error!("[e3_id={}] Error rolling back voter: {:?}", vote.round_id, err); + } + } return HttpResponse::InternalServerError().json("Internal server error"); } };
🧹 Nitpick comments (11)
.prettierignore (1)
45-46: Organize entries into their respective sections for consistency.The new
.claude/and.claude/settings.local.jsonentries are functionally correct but placed at the end without a section header, breaking the logical organization of the file. For maintainability, consider organizing them into the appropriate sections:
.claude/is a directory → should go in the "directories" section (lines 2–14).claude/settings.local.jsonis a file → could go in the "files" section (lines 16–22) or under a new "configuration" or "IDE" sectionApply this diff to reorganize the entries:
# directories node_modules dist build cache coverage target artifacts types deployments .cargo .next .cache-synpress .enclave +.claude # files *.env *.log .DS_Store .pnp.* coverage.json enclave.config.yaml +.claude/settings.local.json # lock files package-lock.json pnpm-lock.yaml yarn.lock Cargo.lock # generated files deployed_contracts.json *.last-run.json # test results test-results/ # secrets files **/example.secrets.json **/*.secrets.json # submodules examples/CRISP/packages/crisp-contracts/lib/risc0-ethereum templates/default/lib/risc0-ethereum - -.claude/ -.claude/settings.local.jsontemplates/default/client/index.html (1)
5-10: Consider providing appropriately-sized favicon assets.All icon declarations reference the same
/favicon.png. For optimal display across devices, consider providing properly sized assets (e.g.,favicon-16x16.png,favicon-32x32.png, etc.) to avoid browser scaling artifacts.examples/CRISP/client/src/pages/RoundPoll/RoundPoll.tsx (2)
30-39: Add error handling for round loading.If
getRoundStateLitethrows an error,setLoading(false)won't execute, leaving the component stuck in a loading state. Consider wrapping in try/catch or using finally.useEffect(() => { const loadRound = async () => { if (parsedRoundId !== null) { setLoading(true) - await getRoundStateLite(parsedRoundId) - setLoading(false) + try { + await getRoundStateLite(parsedRoundId) + } finally { + setLoading(false) + } } } loadRound() }, [parsedRoundId, getRoundStateLite])
53-57: Optional: Remove unnecessaryFragment.The
Fragmentwrapper is not needed when returning a single child element.- return ( - <Fragment> - <DailyPollSection loading={false} endTime={endTime} title={title} /> - </Fragment> - ) + return <DailyPollSection loading={false} endTime={endTime} title={title} />examples/CRISP/client/src/context/NotificationAlert/NotificationAlert.context.tsx (1)
50-55: Consider cleanup for timeout on unmount.The
setTimeoutcallback may attempt to update state after the component unmounts, which could cause a warning in React (though not a crash in React 18+). For robustness, consider tracking and clearing timeouts on cleanup.One approach is to track timeout IDs in a ref and clear them in a cleanup effect, or use an AbortController pattern. However, since React 18+ ignores state updates on unmounted components without warnings, this is optional.
examples/CRISP/client/src/components/VotingStepIndicator.tsx (1)
88-91: Connector line uses current step's style instead of reflecting transition state.The connector line between steps uses the current step's
styles.line, but visually it would be more intuitive if completed connectors (between two completed steps) show the completed style, while the connector leading to the active step shows the pending/transitioning style.- {index < steps.length - 1 && <div className={`flex-1 h-0.5 mx-2 ${styles.line}`} />} + {index < steps.length - 1 && ( + <div className={`flex-1 h-0.5 mx-2 ${status === 'complete' ? 'bg-lime-500' : 'bg-slate-200'}`} /> + )}examples/CRISP/client/src/components/Cards/PollCard.tsx (1)
24-38: LGTM! Consider a longer interval for poll status checks.The implementation correctly handles cleanup and early return. However, a 1-second interval for timestamp comparison may be more frequent than necessary. Since poll end times are typically known in advance, a 5-10 second interval would reduce re-renders while still providing timely updates.
- const interval = setInterval(checkPollStatus, 1000) + const interval = setInterval(checkPollStatus, 5000)examples/CRISP/client/src/hooks/voting/useVoteCasting.ts (3)
110-116: Prefer structured error handling over eslint-disable.Instead of suppressing the unused variable warning, consider logging the error for debugging purposes or using an underscore prefix convention.
- // eslint-disable-next-line @typescript-eslint/no-unused-vars - } catch (signError) { - console.log('User rejected signature or signing failed') + } catch (signError: unknown) { + console.log('User rejected signature or signing failed:', signError)
118-134: Consider adding step messages for all steps for consistent UX.The
stepMessageis only set for the signing step ('Please sign the message in your wallet...') and cleared/empty for encrypting, generating_proof, and broadcasting. If theVotingStepIndicatordisplays the message, users may see an empty message during these steps.// Step 2: Encrypting vote setVotingStep('encrypting') - setStepMessage('') + setStepMessage('Encrypting your vote...') const voteEncrypted = await handleVoteEncryption(pollSelected, user.address, signature, message) if (!voteEncrypted) { throw new Error('Failed to encrypt vote.') } // Step 3: Generating proof setVotingStep('generating_proof') + setStepMessage('Generating zero-knowledge proof...') // small delay for UX await new Promise((resolve) => setTimeout(resolve, 500)) // Step 4: Broadcasting setVotingStep('broadcasting') + setStepMessage('Broadcasting to the network...')
130-131: Document the UX delay rationale.The 500ms artificial delay is for UX, but a brief comment explaining why this improves user experience would help future maintainers understand this isn't a placeholder.
// Step 3: Generating proof setVotingStep('generating_proof') - // small delay for UX + // Brief delay to ensure users see the proof generation step, + // as the actual proof is generated during encryption await new Promise((resolve) => setTimeout(resolve, 500))examples/CRISP/client/src/components/ToastAlert.tsx (1)
17-17: Unusedidprop declared in component.The
idprop is declared inToastAlertPropsbut not used within the component body. If it's only needed for external toast management (e.g., in the notification context), consider documenting this or removing it from the component's props if it's only used as a key externally.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.prettierignore(1 hunks)examples/CRISP/client/src/App.tsx(2 hunks)examples/CRISP/client/src/components/Cards/PollCard.tsx(2 hunks)examples/CRISP/client/src/components/ToastAlert.tsx(1 hunks)examples/CRISP/client/src/components/VotingStepIndicator.tsx(1 hunks)examples/CRISP/client/src/context/NotificationAlert/NotificationAlert.context.tsx(1 hunks)examples/CRISP/client/src/context/NotificationAlert/NotificationAlert.types.ts(1 hunks)examples/CRISP/client/src/context/voteManagement/VoteManagement.context.tsx(8 hunks)examples/CRISP/client/src/context/voteManagement/VoteManagement.types.ts(3 hunks)examples/CRISP/client/src/hooks/enclave/useEnclaveServer.ts(3 hunks)examples/CRISP/client/src/hooks/voting/useVoteCasting.ts(3 hunks)examples/CRISP/client/src/model/notification.model.ts(1 hunks)examples/CRISP/client/src/model/vote.model.ts(1 hunks)examples/CRISP/client/src/pages/DailyPoll/DailyPoll.tsx(1 hunks)examples/CRISP/client/src/pages/HistoricPoll/HistoricPoll.tsx(3 hunks)examples/CRISP/client/src/pages/Landing/components/DailyPoll.tsx(6 hunks)examples/CRISP/client/src/pages/RoundPoll/RoundPoll.tsx(1 hunks)examples/CRISP/client/src/pages/RoundPoll/index.ts(1 hunks)examples/CRISP/client/src/utils/methods.ts(3 hunks)examples/CRISP/scripts/dev_server.sh(1 hunks)examples/CRISP/server/src/server/models.rs(1 hunks)examples/CRISP/server/src/server/repo.rs(2 hunks)examples/CRISP/server/src/server/routes/voting.rs(5 hunks)templates/default/client/index.html(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-25T10:28:56.174Z
Learnt from: ctrlc03
Repo: gnosisguild/enclave PR: 657
File: Cargo.toml:32-34
Timestamp: 2025-08-25T10:28:56.174Z
Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.
Applied to files:
.prettierignoreexamples/CRISP/scripts/dev_server.sh
📚 Learning: 2025-10-29T23:35:30.146Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 936
File: scripts/run-crisp-test.sh:1-3
Timestamp: 2025-10-29T23:35:30.146Z
Learning: In the scripts/run-crisp-test.sh file, the use of `rm -rf *` is acceptable as it's intentionally designed as a definitive reset-and-test script for clean checkouts.
Applied to files:
examples/CRISP/scripts/dev_server.sh
📚 Learning: 2024-11-05T06:49:46.285Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/enclave_node/src/datastore.rs:14-16
Timestamp: 2024-11-05T06:49:46.285Z
Learning: In `packages/ciphernode/enclave_node/src/datastore.rs`, for internal functions like `get_in_mem_store`, adding extensive documentation and error handling may not be necessary if they are not client-facing.
Applied to files:
examples/CRISP/server/src/server/repo.rs
📚 Learning: 2024-09-26T03:11:29.311Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-09-26T03:11:29.311Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Applied to files:
examples/CRISP/server/src/server/repo.rs
📚 Learning: 2024-10-22T02:10:34.864Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs:82-83
Timestamp: 2024-10-22T02:10:34.864Z
Learning: In the file `packages/ciphernode/tests/tests/test_aggregation_and_decryption.rs`, when reviewing test functions like `generate_pk_share`, minor performance optimizations (e.g., minimizing mutex locks) are not a priority.
Applied to files:
examples/CRISP/server/src/server/repo.rs
📚 Learning: 2024-10-22T03:41:21.226Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 145
File: packages/ciphernode/router/src/repositories.rs:40-71
Timestamp: 2024-10-22T03:41:21.226Z
Learning: In `packages/ciphernode/router/src/repositories.rs`, prefer to keep method implementations as they are if they are straightforward and maintainable, even if refactoring could reduce duplication.
Applied to files:
examples/CRISP/server/src/server/repo.rs
🧬 Code graph analysis (6)
examples/CRISP/client/src/pages/RoundPoll/RoundPoll.tsx (2)
examples/CRISP/client/src/context/voteManagement/VoteManagement.context.tsx (1)
useVoteManagementContext(242-242)examples/CRISP/client/src/utils/methods.ts (1)
convertTimestampToDate(18-22)
examples/CRISP/client/src/hooks/enclave/useEnclaveServer.ts (1)
examples/CRISP/client/src/model/vote.model.ts (2)
VoteStatusRequest(39-42)VoteStatusResponse(44-49)
examples/CRISP/client/src/context/NotificationAlert/NotificationAlert.context.tsx (3)
examples/CRISP/client/src/utils/create-generic-context.ts (1)
createGenericContext(9-23)examples/CRISP/client/src/context/NotificationAlert/NotificationAlert.types.ts (2)
NotificationAlertContextType(10-14)NotificationAlertProviderProps(16-18)examples/CRISP/client/src/model/notification.model.ts (1)
NotificationAlert(7-14)
examples/CRISP/client/src/hooks/voting/useVoteCasting.ts (4)
examples/CRISP/client/src/model/vote.model.ts (2)
VoteStateLite(51-66)VotingRound(15-18)examples/CRISP/client/src/context/voteManagement/VoteManagement.context.tsx (1)
useVoteManagementContext(242-242)examples/CRISP/client/src/context/NotificationAlert/NotificationAlert.context.tsx (1)
useNotificationAlertContext(98-98)examples/CRISP/client/src/model/poll.model.ts (1)
Poll(32-36)
examples/CRISP/client/src/components/VotingStepIndicator.tsx (1)
examples/CRISP/client/src/hooks/voting/useVoteCasting.ts (1)
VotingStep(16-16)
examples/CRISP/server/src/server/routes/voting.rs (2)
examples/CRISP/client/src/model/vote.model.ts (2)
VoteStatusRequest(39-42)VoteStatusResponse(44-49)examples/CRISP/server/src/server/repo.rs (1)
has_voted(229-232)
🪛 GitHub Actions: License Header Check
examples/CRISP/client/src/pages/RoundPoll/index.ts
[error] 1-1: License header missing. Please add the SPDX header as per project guidelines.
⏰ 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). (10)
- GitHub Check: build_sdk
- GitHub Check: integration_prebuild
- GitHub Check: build_enclave_cli
- GitHub Check: test_net
- GitHub Check: test_contracts
- GitHub Check: rust_unit
- GitHub Check: build_e3_support_risc0
- GitHub Check: crisp_rust_unit
- GitHub Check: rust_integration
- GitHub Check: Build & Push Image
🔇 Additional comments (32)
examples/CRISP/scripts/dev_server.sh (1)
7-7: LGTM – Fresh database state for dev workflow.The pre-start cleanup ensures a clean database state, which aligns well with the new vote status tracking and session-scoped vote caching introduced in this PR. For a development server script, this is the appropriate pattern.
Consider adding a comment to explain why the cleanup is needed:
~(cd ./server && rm -rf database && cargo run --bin server) +(cd ./server && rm -rf database && cargo run --bin server) # Fresh database for clean vote stateexamples/CRISP/server/src/server/repo.rs (1)
97-107: LGTM!The reformatted method chain improves readability without changing functionality.
examples/CRISP/client/src/model/notification.model.ts (1)
7-14: LGTM!The expanded type union and new optional fields provide flexibility for enhanced toast management while remaining backward-compatible with existing consumers.
examples/CRISP/client/src/context/NotificationAlert/NotificationAlert.types.ts (1)
10-14: LGTM!The new
closeToastandclearAllToastsmethods provide granular control over toast lifecycle management, aligning well with the expanded notification system.examples/CRISP/client/src/utils/methods.ts (2)
18-22: Good bug fix forconvertTimestampToDate.The original code incorrectly used
date.getMinutes()as the base for adding seconds, which would have caused incorrect date calculations. Usingdate.getSeconds()is the correct approach.
116-121: Good type safety improvement fordebounce.Using generics with
Parameters<T>ensures the returned function preserves the argument types of the wrapped function, providing better TypeScript inference for consumers.examples/CRISP/client/src/context/voteManagement/VoteManagement.types.ts (2)
12-16: LGTM!The
VoteStatustype is well-structured for session-scoped vote status caching with thelastCheckedtimestamp enabling cache expiration logic.
28-31: LGTM!The new context fields and methods provide a clean API for vote status management with appropriate loading states and cache control via
forceRefresh.Also applies to: 51-52
examples/CRISP/client/src/context/voteManagement/VoteManagement.context.tsx (5)
21-29: LGTM!The session ID generation and cache key strategy are well-designed. Using a combination of timestamp and random string ensures uniqueness across page loads, and the 5-minute cache duration is reasonable for vote status freshness.
73-107: LGTM!The
checkVoteStatusimplementation correctly handles:
- Early returns for invalid inputs
- Cache validation with TTL
- Loading state management via
finallyblock- Error handling with safe fallback
The
useCallbackdependencies are complete.
109-126: LGTM!The
markVotedInRoundimplementation correctly updates both the cache and thehasVotedInCurrentRoundstate. The conditional check ensures only the current round's status is updated in state, while the cache stores status for any round.
186-202: LGTM!The effect properly handles async state updates with a cancellation flag to prevent race conditions when dependencies change before the async operation completes.
176-184: LGTM!Good practice to clear the vote status cache when the user disconnects. This prevents stale data from being used if a different user connects.
examples/CRISP/client/src/App.tsx (1)
17-17: LGTM!The new route for round polls is correctly integrated. The barrel import pattern is clean, and the route placement between
/currentand/historicis logical.Also applies to: 41-41
examples/CRISP/client/src/context/NotificationAlert/NotificationAlert.context.tsx (1)
64-71: Good use ofuseMemofor context value.Memoizing the context value prevents unnecessary re-renders of consumers when the provider re-renders for unrelated reasons.
examples/CRISP/client/src/pages/DailyPoll/DailyPoll.tsx (1)
13-16: Clean simplification of loading state derivation.Replacing explicit state management with a derived
loadingvalue is cleaner and less error-prone. The memoizedendTimecalculation is consistent withRoundPoll.tsx.examples/CRISP/client/src/pages/RoundPoll/RoundPoll.tsx (1)
39-39: I'll help you verify ifgetRoundStateLiteis properly memoized in the context. Let me start by locating the VoteManagementContext and checking howgetRoundStateLiteis defined.
<function_calls>
#!/bin/bashFind VoteManagementContext file
find . -type f -name "VoteManagement" -o -name "Context" | grep -i vote | head -20
</function_calls>Now let me search more broadly for the context definition:
<function_calls>
#!/bin/bashSearch for getRoundStateLite definition across the codebase
rg "getRoundStateLite" -n -B 3 -A 3 --type=ts --type=tsx | head -100
</function_calls>Let me also check the RoundPoll.tsx file to understand the context of this dependency array:
<function_calls>
#!/bin/bashFind and read the RoundPoll.tsx file
find . -path "*RoundPoll/RoundPoll.tsx" -exec cat -n {} ;
</function_calls>examples/CRISP/client/src/hooks/enclave/useEnclaveServer.ts (1)
29-29: LGTM!The new
getVoteStatusendpoint follows the established patterns in this hook. The implementation is consistent with other API calls and properly typed.Also applies to: 40-40, 49-49
examples/CRISP/server/src/server/models.rs (2)
61-65: LGTM! Consider address validation at the handler level.The struct is well-defined. Ensure the route handler that consumes
VoteStatusRequestvalidates theaddressfield format (e.g., valid Ethereum address) before processing to prevent invalid queries.
67-73: LGTM!The
VoteStatusResponsestruct properly mirrors the client-side TypeScript interface with appropriate field types and optionalround_status.examples/CRISP/client/src/components/Cards/PollCard.tsx (2)
21-22: Good use of derived state for display logic.The
displayVoteCountcorrectly prioritizes the live contextvote_countfor the current active round while falling back tototalVotesfor historical data.
45-60: Verify thatsetPollResultshould only be called for ended polls.
setPollResultis only called when navigating to/result/${roundId}, not for active rounds. Ensure this is intentional and that the result page for ended polls can properly use this pre-set data.examples/CRISP/client/src/hooks/voting/useVoteCasting.ts (3)
16-16: LGTM!The
VotingSteptype is well-defined and covers all states of the voting flow includingidle,confirming, anderrorstates.
18-42: LGTM! Good error message normalization.The
extractCleanErrorMessagefunction provides user-friendly messages for common blockchain errors while gracefully handling unknown errors with a length check.
197-208: LGTM!The dependency array is complete and includes all necessary dependencies for the
useCallbackhook, includingresetVotingStatewhich is itself memoized.examples/CRISP/client/src/model/vote.model.ts (1)
31-49: Well-structured type definitions for vote status tracking.The new
VoteStatusRequestandVoteStatusResponseinterfaces properly define the API contract for the/voting/statusendpoint. The addition ofis_vote_updatetoBroadcastVoteResponsecleanly supports distinguishing between new votes and updates. Types align correctly with the server-side models invoting.rs.examples/CRISP/client/src/components/ToastAlert.tsx (1)
20-29: Clean implementation of persistent toast behavior.The early return pattern for persistent toasts is appropriate and correctly prevents the auto-dismiss timer. The dependency array properly includes
persistent.examples/CRISP/client/src/pages/Landing/components/DailyPoll.tsx (2)
88-97: Good UX feedback for vote status.The "You voted" and "Checking..." indicators provide clear visual feedback to users about their voting status. The styling is consistent with the existing badge patterns.
106-107: Well-implemented voting flow with step-by-step progress.The integration of
VotingStepIndicatorduring the casting process and the conditional button labels ("Cast Vote" / "Update Vote" / "Processing Vote...") provide excellent user feedback. The disabled state logic is comprehensive.Also applies to: 117-130
examples/CRISP/server/src/server/routes/voting.rs (3)
42-78: Well-implemented vote status endpoint.The
get_vote_statushandler properly validates the request, handles database errors gracefully, and returns a structured response including both the vote status and round status. Good logging for debugging.
177-201: Good user-friendly error message extraction.The
extract_error_messagefunction appropriately maps technical blockchain errors to user-friendly messages. The pattern matching covers common failure scenarios.
211-226: Correct rollback logic for vote updates.The conditional rollback that skips removal for vote updates is correct - we don't want to remove the voter address when updating an existing vote fails, as they remain a valid voter.
hmzakhalid
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
examples/CRISP/client/src/components/ToastAlert.tsx (1)
20-29: Unusedidprop in component signature.The
idprop is declared inToastAlertPropsand passed to the component but is never destructured or used within the component body. If it's only needed for external keying purposes, consider removing it from the component props entirely or document why it's included.-const ToastAlert: React.FC<ToastAlertProps> = ({ message, type, linkUrl, onClose, persistent = false }) => { +const ToastAlert: React.FC<ToastAlertProps> = ({ message, type, linkUrl, onClose, persistent = false, id }) => {Or if
idtruly isn't needed inside the component, remove it from the props type:type ToastAlertProps = { type: 'success' | 'danger' | 'warning' | 'info' linkUrl?: string message: string onClose: () => void persistent?: boolean - id?: string }examples/CRISP/client/src/context/NotificationAlert/NotificationAlert.context.tsx (1)
44-48: Toast queue trimming may dismiss persistent toasts prematurely.When exceeding
MAX_TOASTS,prev.slice(1)removes the oldest toast regardless of itspersistentflag. If persistent toasts should remain until explicitly dismissed, consider skipping them when trimming:setToasts((prev) => { - const newToasts = prev.length >= MAX_TOASTS ? prev.slice(1) : prev + let newToasts = prev + if (prev.length >= MAX_TOASTS) { + const nonPersistentIndex = prev.findIndex((t) => !t.persistent) + if (nonPersistentIndex !== -1) { + newToasts = prev.filter((_, i) => i !== nonPersistentIndex) + } else { + newToasts = prev.slice(1) // All persistent, remove oldest + } + } return [...newToasts, toastWithId] })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/CRISP/client/index.html(0 hunks)examples/CRISP/client/src/components/ToastAlert.tsx(1 hunks)examples/CRISP/client/src/context/NotificationAlert/NotificationAlert.context.tsx(1 hunks)examples/CRISP/client/src/pages/RoundPoll/RoundPoll.tsx(1 hunks)examples/CRISP/server/src/server/routes/voting.rs(5 hunks)
💤 Files with no reviewable changes (1)
- examples/CRISP/client/index.html
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/CRISP/client/src/pages/RoundPoll/RoundPoll.tsx
- examples/CRISP/server/src/server/routes/voting.rs
⏰ 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). (11)
- GitHub Check: Build & Push Image
- GitHub Check: test_net
- GitHub Check: build_sdk
- GitHub Check: build_e3_support_risc0
- GitHub Check: test_contracts
- GitHub Check: build_e3_support_dev
- GitHub Check: build_enclave_cli
- GitHub Check: integration_prebuild
- GitHub Check: rust_integration
- GitHub Check: crisp_rust_unit
- GitHub Check: rust_unit
🔇 Additional comments (7)
examples/CRISP/client/src/components/ToastAlert.tsx (3)
9-17: Type expansion and new props look good.The addition of
warningandinfotypes with corresponding icons and the newpersistent/idprops align well with the notification system enhancements. Theidprop being optional here is appropriate since it's primarily used by the parent context for keying.
31-56: Well-structured style configuration with icon mappings.The
alertStylesobject cleanly maps each type to its visual properties including the newiconfield. Usingnullfor success/danger icons while providingWarning/Infocomponents for the new types is a reasonable approach.
62-92: Previous positioning and interaction issues have been addressed.The
relativeclass on the outer container andpointer-events-autoare now correctly applied, fixing the persistent indicator positioning and ensuring the toast remains clickable when rendered within apointer-events-noneparent. The layout with flex, gap, and proper link attributes (rel='noopener noreferrer') follows best practices.examples/CRISP/client/src/context/NotificationAlert/NotificationAlert.context.tsx (4)
13-16: Constants and ID generator are well-defined.The
MAX_TOASTSlimit andDEFAULT_DURATIONprovide good configurability. ThegenerateToastIdfunction using timestamp + random suffix is sufficient for client-side toast identification.
23-35: Previous fallback behavior concern has been addressed.The
closeToastfunction now correctly returnsprevunchanged (line 32) when no non-persistent toast is found, preventing unintended dismissal of persistent toasts in the fallback case.
64-71: Context memoization is correctly implemented.The
useMemowith proper dependency array prevents unnecessary re-renders of consumers when the toast list changes but the functions remain stable.
76-93: Toast container with accessibility attributes looks good.The fixed container with
flex-col-reverseensures newest toasts appear at the bottom. ARIA attributes (role,aria-label,aria-live) improve accessibility. Thepointer-events-noneon the container combined withpointer-events-autoin ToastAlert (verified in the other file) correctly enables interaction.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
examples/CRISP/client/src/components/VotingStepIndicator.tsx (1)
17-22: Consider adding 'confirming' step or removing it fromVotingSteptype.The
VotingSteptype includes'confirming'but it's not represented in thestepsarray and never set inuseVoteCasting.ts(the flow goes directly from'broadcasting'to'complete'). If this step is intended for future use, consider adding a TODO comment; otherwise, remove it from the type to avoid confusion.examples/CRISP/client/src/hooks/voting/useVoteCasting.ts (2)
82-101: TheisVoteUpdateparameter is misleading.The
isVoteUpdateparameter on line 83 is only used for the log message (lines 100-101), but the actual success message on line 162 usesbroadcastVoteResponse.is_vote_updatefrom the server response. This creates confusion about what determines update vs. new vote behavior.Consider removing the parameter if the server is the source of truth, or document clearly that it's only for logging purposes.
- async (pollSelected: Poll | null, isVoteUpdate: boolean = false) => { + async (pollSelected: Poll | null) => { if (!pollSelected) { console.log('Cannot cast vote: Poll option not selected.') showToast({ type: 'danger', message: 'Please select a poll option first.' }) return } if (!user || !roundState) { console.error('Cannot cast vote: Missing user or round state.') showToast({ type: 'danger', message: 'Cannot cast vote. Ensure you are connected, and the round is active.', persistent: true, }) return } setIsLoading(true) - const actionText = isVoteUpdate ? 'Updating vote' : 'Processing vote' - console.log(`${actionText}...`) + console.log('Processing vote...')
131-136: Artificial delay may not be necessary.The 500ms delay creates a synthetic pause that doesn't reflect actual work being done. If the proof generation is instantaneous, users will briefly see a misleading "generating proof" state. Consider removing this delay or only showing the step if it takes meaningful time.
examples/CRISP/client/src/pages/Landing/components/DailyPoll.tsx (1)
78-78: Redundant spacing classes may cause inconsistent layout.Using both
space-x-2andgap-2together results in double horizontal spacing (1rem total) between flex items. When items wrap, this could create inconsistent spacing.Consider using only
gap-2for consistent spacing in both directions:- <div className='flex items-center justify-center space-x-2 flex-wrap gap-2'> + <div className='flex items-center justify-center flex-wrap gap-2'>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/CRISP/client/src/components/VotingStepIndicator.tsx(1 hunks)examples/CRISP/client/src/hooks/voting/useVoteCasting.ts(3 hunks)examples/CRISP/client/src/pages/Landing/components/DailyPoll.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
examples/CRISP/client/src/components/VotingStepIndicator.tsx (1)
examples/CRISP/client/src/hooks/voting/useVoteCasting.ts (1)
VotingStep(16-16)
⏰ 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). (10)
- GitHub Check: test_net
- GitHub Check: build_sdk
- GitHub Check: test_contracts
- GitHub Check: build_enclave_cli
- GitHub Check: rust_integration
- GitHub Check: build_e3_support_dev
- GitHub Check: integration_prebuild
- GitHub Check: rust_unit
- GitHub Check: crisp_rust_unit
- GitHub Check: Build & Push Image
🔇 Additional comments (6)
examples/CRISP/client/src/components/VotingStepIndicator.tsx (1)
24-35: LGTM! Error state handling has been properly addressed.The
lastActiveStepprop now correctly handles the error case, ensuring the step indicator accurately reflects which step failed rather than showing all steps as pending.examples/CRISP/client/src/hooks/voting/useVoteCasting.ts (2)
18-42: Good error message normalization.The
extractCleanErrorMessagehelper provides user-friendly error messages for common blockchain errors. The pattern-based matching is clear and maintainable.
217-225: LGTM! Hook API is well-structured.The expanded return interface provides all necessary state and actions for consumers to integrate the multi-step voting flow.
examples/CRISP/client/src/pages/Landing/components/DailyPoll.tsx (3)
88-97: LGTM! Clear vote status indicators.The "You voted" and "Checking..." badges provide clear visual feedback for the user's voting state. Styling is consistent with existing status badges.
106-107: Good mutually exclusive loading states.The VotingStepIndicator and LoadingAnimation are correctly shown in mutually exclusive conditions, preventing UI overlap during different loading scenarios.
55-62: Parameter naming mismatch with hook.
hasVotedInCurrentRoundis passed tocastVoteWithProofas theisVoteUpdateparameter, but as noted in the hook review, this parameter only affects logging. If the hook parameter is removed per the earlier suggestion, this call can be simplified.If
isVoteUpdateremains in the hook, consider renaming for clarity to match usage.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/CRISP/client/src/context/NotificationAlert/NotificationAlert.context.tsx (1)
15-15: Consider a more collision-resistant ID generation.The current approach using
Date.now()+Math.random()is adequate for typical usage, but could theoretically produce duplicate IDs if multiple toasts are generated in rapid succession (same millisecond).If you want greater robustness, consider adding a simple counter:
+let toastCounter = 0 + -const generateToastId = (): string => `toast-${Date.now()}-${Math.random().toString(36).substring(2, 11)}` +const generateToastId = (): string => `toast-${Date.now()}-${++toastCounter}`
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/CRISP/client/src/components/ToastAlert.tsx(1 hunks)examples/CRISP/client/src/context/NotificationAlert/NotificationAlert.context.tsx(1 hunks)
⏰ 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). (9)
- GitHub Check: test_net
- GitHub Check: build_enclave_cli
- GitHub Check: build_sdk
- GitHub Check: integration_prebuild
- GitHub Check: test_contracts
- GitHub Check: rust_unit
- GitHub Check: build_e3_support_risc0
- GitHub Check: rust_integration
- GitHub Check: crisp_rust_unit
🔇 Additional comments (9)
examples/CRISP/client/src/context/NotificationAlert/NotificationAlert.context.tsx (4)
22-34: LGTM! The fallback logic correctly handles persistent toasts.The closeToast implementation properly addresses the previous concern: when no
idis provided and no non-persistent toast exists, it returnsprevunchanged instead of removing a persistent toast.
49-51: LGTM! Clean implementation.The
clearAllToastsfunction correctly resets the toast state.
53-60: LGTM! Proper context value memoization.The
useMemocorrectly prevents unnecessary re-renders by memoizing the context value, with appropriate dependencies on the callback functions.
65-83: LGTM! Well-structured toast container with proper accessibility.The implementation includes:
- Appropriate fixed positioning and z-index for toast visibility
- Correct pointer-events handling (container with
pointer-events-none, individual toasts withpointer-events-auto)- Good accessibility with ARIA attributes (
role,aria-label,aria-live)flex-col-reverseprovides intuitive bottom-up toast stackingexamples/CRISP/client/src/components/ToastAlert.tsx (5)
9-21: LGTM! Appropriate type and prop extensions.The additions properly support the expanded toast functionality:
- New icon imports for
warningandinfotypes- Extended type union with
'warning' | 'info'- Optional props (
persistent,duration,id) for flexible toast behavior- Reasonable
DEFAULT_DURATIONof 5 seconds
23-33: LGTM! Auto-dismiss logic is correctly implemented.The component properly handles:
- Default
persistent = falseparameter- Early return for persistent toasts (no auto-dismiss)
- Configurable duration with sensible default
- Timer cleanup to prevent memory leaks
- Correct effect dependencies
35-63: LGTM! Alert styles are well-structured and consistent.The extended alert styles properly support all four types with:
- Consistent structure across
success,danger,warning, andinfo- Appropriate color schemes for each type
- Icon components for
warning(Warning) andinfo(Info)nullicons forsuccessanddanger(intentional, no icon shown)
66-91: LGTM! Rendering structure is solid with proper security considerations.The implementation includes:
pointer-events-autoandrelativepositioning (addresses previous comments)- Flexible layout accommodating icons, messages, and links
- Security best practice:
rel='noopener noreferrer'on external links prevents reverse tabnabbing- Conditional icon rendering based on type
- Proper close button implementation
92-94: LGTM! Persistent indicator is well-implemented.The persistent toast indicator:
- Conditionally renders only for persistent toasts
- Uses absolute positioning correctly (parent has
relative)- Provides clear visual feedback (pulsing red dot)
- Includes helpful tooltip via
titleattribute
ctrlc03
force-pushed
the
chore/crisp-fixes
branch
from
afb07a0 to
1ce8f20
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
examples/CRISP/client/src/context/NotificationAlert/NotificationAlert.context.tsx (1)
42-46: Persistent toasts are not protected from overflow removal.When the queue reaches
MAX_TOASTS, line 43 usesprev.slice(1), which removes the oldest toast regardless of whether it's persistent. This is inconsistent withcloseToast(lines 27-31), which explicitly protects persistent toasts from automatic removal.If persistent toasts should remain until manually dismissed, apply this diff:
setToasts((prev) => { - const newToasts = prev.length >= MAX_TOASTS ? prev.slice(1) : prev + let newToasts = prev + if (prev.length >= MAX_TOASTS) { + const nonPersistentIndex = prev.findIndex((t) => !t.persistent) + newToasts = nonPersistentIndex !== -1 + ? prev.filter((_, i) => i !== nonPersistentIndex) + : prev.slice(1) + } return [...newToasts, toastWithId] })
🧹 Nitpick comments (3)
examples/CRISP/client/src/pages/HistoricPoll/HistoricPoll.tsx (1)
44-46: Remove redundant useEffect.This effect sets
visiblePollsto the first 12 polls wheneverpastPollschanges. However, the effect on lines 48-51 already handles this logic (whenpage=0, it computes the same slice). Having both effects causes redundant state updates.Apply this diff to remove the redundant effect:
- useEffect(() => { - setVisiblePolls(pastPolls.slice(0, 12)) - }, [pastPolls]) - useEffect(() => { const newVisiblePolls = pastPolls.slice(0, (page + 1) * 12) setVisiblePolls(newVisiblePolls)examples/CRISP/server/src/server/routes/voting.rs (1)
20-25: get_vote_status endpoint logic is soundThe new
/voting/statusroute andget_vote_statushandler cleanly reuse the repository APIs (has_voted,get_e3_state_lite) and return a well-shapedVoteStatusResponse. Only minor micro-optimization possible (reuse thestore.e3(round_id)handle instead of calling twice), but not required.Also applies to: 28-73
examples/CRISP/client/src/components/Cards/PollCard.tsx (1)
15-23: Live/round-aware PollCard behavior looks correct (with a small reuse edge case)
isActive+ the 1s interval cleanly transitions cards from live to finished and stops polling once inactive.- Using
currentRoundIdto decide between/current,/round/:roundId, and/result/:roundIdmatches the intended UX, anddisplayVoteCountcorrectly prefers liveroundState.vote_countwhen applicable.- One minor edge case: if a
PollCardinstance were ever reused with a differentendTimebut the same React key,isActivewould not be recomputed from the newendTime. In typical list usage keyed byroundId, this shouldn’t happen, but if reuse is possible you may want an effect that updatesisActivewhenendTimechanges.Also applies to: 24-38, 45-60, 64-82
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.prettierignore(1 hunks)examples/CRISP/client/index.html(0 hunks)examples/CRISP/client/libs/crispWorker.js(2 hunks)examples/CRISP/client/src/App.tsx(2 hunks)examples/CRISP/client/src/components/Cards/PollCard.tsx(2 hunks)examples/CRISP/client/src/components/ToastAlert.tsx(1 hunks)examples/CRISP/client/src/components/VotingStepIndicator.tsx(1 hunks)examples/CRISP/client/src/context/NotificationAlert/NotificationAlert.context.tsx(1 hunks)examples/CRISP/client/src/context/NotificationAlert/NotificationAlert.types.ts(1 hunks)examples/CRISP/client/src/context/voteManagement/VoteManagement.context.tsx(8 hunks)examples/CRISP/client/src/context/voteManagement/VoteManagement.types.ts(2 hunks)examples/CRISP/client/src/hooks/enclave/useEnclaveServer.ts(3 hunks)examples/CRISP/client/src/hooks/voting/useVoteCasting.ts(2 hunks)examples/CRISP/client/src/hooks/wasm/useWebAssembly.tsx(1 hunks)examples/CRISP/client/src/model/notification.model.ts(1 hunks)examples/CRISP/client/src/model/vote.model.ts(1 hunks)examples/CRISP/client/src/pages/DailyPoll/DailyPoll.tsx(1 hunks)examples/CRISP/client/src/pages/HistoricPoll/HistoricPoll.tsx(3 hunks)examples/CRISP/client/src/pages/Landing/components/DailyPoll.tsx(6 hunks)examples/CRISP/client/src/pages/RoundPoll/RoundPoll.tsx(1 hunks)examples/CRISP/client/src/pages/RoundPoll/index.ts(1 hunks)examples/CRISP/client/src/utils/methods.ts(3 hunks)examples/CRISP/packages/crisp-sdk/src/types.ts(1 hunks)examples/CRISP/scripts/dev_server.sh(1 hunks)examples/CRISP/server/src/server/models.rs(1 hunks)examples/CRISP/server/src/server/repo.rs(2 hunks)examples/CRISP/server/src/server/routes/voting.rs(3 hunks)
💤 Files with no reviewable changes (1)
- examples/CRISP/client/index.html
✅ Files skipped from review due to trivial changes (1)
- examples/CRISP/server/src/server/repo.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- examples/CRISP/client/src/pages/RoundPoll/index.ts
- .prettierignore
- examples/CRISP/client/src/model/notification.model.ts
- examples/CRISP/client/src/pages/RoundPoll/RoundPoll.tsx
- examples/CRISP/server/src/server/models.rs
- examples/CRISP/client/src/App.tsx
- examples/CRISP/scripts/dev_server.sh
- examples/CRISP/client/src/pages/DailyPoll/DailyPoll.tsx
- examples/CRISP/client/src/model/vote.model.ts
- examples/CRISP/client/src/context/NotificationAlert/NotificationAlert.types.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/config/src/app_config.rs:13-21
Timestamp: 2024-11-05T06:48:58.177Z
Learning: In the `packages/ciphernode/config/src/app_config.rs` file, for the `Contract` enum, the team prefers to use `String` type for `address` fields, relying on parsing to handle validation, instead of using the `Address` type.
Applied to files:
examples/CRISP/server/src/server/routes/voting.rs
📚 Learning: 2024-11-25T09:56:11.195Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 184
File: packages/ciphernode/net/src/network_relay.rs:0-0
Timestamp: 2024-11-25T09:56:11.195Z
Learning: In `NetworkPeer::new` in `packages/ciphernode/net/src/network_peer.rs`, peers are converted to multiaddrs, and validation of the address format occurs during this conversion. Duplicate addresses are managed as connections are deduplicated.
Applied to files:
examples/CRISP/server/src/server/routes/voting.rs
🧬 Code graph analysis (6)
examples/CRISP/client/src/hooks/enclave/useEnclaveServer.ts (1)
examples/CRISP/client/src/model/vote.model.ts (2)
VoteStatusRequest(38-41)VoteStatusResponse(43-48)
examples/CRISP/client/src/context/voteManagement/VoteManagement.types.ts (2)
examples/CRISP/client/src/model/poll.model.ts (3)
PollResult(14-20)PollRequestResult(22-30)Poll(32-36)examples/CRISP/client/src/model/vote.model.ts (3)
VotingRound(15-18)BroadcastVoteRequest(24-28)BroadcastVoteResponse(31-36)
examples/CRISP/client/src/components/VotingStepIndicator.tsx (1)
examples/CRISP/client/src/hooks/voting/useVoteCasting.ts (1)
VotingStep(18-18)
examples/CRISP/server/src/server/routes/voting.rs (3)
examples/CRISP/server/src/server/repo.rs (1)
has_voted(229-232)examples/CRISP/crates/evm_helpers/src/lib.rs (1)
address(58-60)crates/evm-helpers/src/contracts.rs (1)
address(194-196)
examples/CRISP/client/src/hooks/voting/useVoteCasting.ts (3)
examples/CRISP/client/src/model/poll.model.ts (1)
Poll(32-36)examples/CRISP/packages/crisp-sdk/src/vote.ts (2)
generateProof(129-138)encryptVote(84-88)examples/CRISP/packages/crisp-sdk/src/constants.ts (1)
SIGNATURE_MESSAGE(30-30)
examples/CRISP/client/src/pages/Landing/components/DailyPoll.tsx (3)
examples/CRISP/client/src/utils/methods.ts (1)
hasPollEnded(24-28)examples/CRISP/client/src/model/poll.model.ts (1)
Poll(32-36)examples/CRISP/client/src/hooks/voting/useVoteCasting.ts (1)
useVoteCasting(46-230)
⏰ 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). (11)
- GitHub Check: build_e3_support_dev
- GitHub Check: build_enclave_cli
- GitHub Check: test_contracts
- GitHub Check: test_net
- GitHub Check: integration_prebuild
- GitHub Check: build_ciphernode_image
- GitHub Check: build_sdk
- GitHub Check: crisp_unit
- GitHub Check: rust_integration
- GitHub Check: rust_unit
- GitHub Check: Build & Push Image
🔇 Additional comments (30)
examples/CRISP/client/src/pages/HistoricPoll/HistoricPoll.tsx (2)
81-81: LGTM!The className formatting improvement enhances code cleanliness without affecting functionality.
34-42: [rewritten comment]
[classification tag]examples/CRISP/client/src/components/ToastAlert.tsx (2)
21-33: LGTM! Auto-dismiss timer is correctly implemented.The timer logic properly respects the
persistentflag, uses the provideddurationor defaults to 5 seconds, and cleans up on unmount to prevent memory leaks.
62-91: LGTM! Toast rendering and interaction are well-implemented.The component correctly handles:
- Conditional icon rendering for warning/info types
- Secure external links with
rel="noopener noreferrer"- Proper positioning with
relativefor the persistent indicatorpointer-events-autoto enable interaction despite the parent'spointer-events-noneexamples/CRISP/client/src/utils/methods.ts (1)
18-22: Timestamp fix and generic debounce look good
convertTimestampToDatenow correctly addssecondsToAddusinggetSeconds, matching the seconds-based timestamp.formatDatechange is cosmetic and fine.- Generic
debouncewithParameters<T>tightens typing without changing behavior; restricting tovoidcallbacks is appropriate for this usage.Also applies to: 36-52, 116-122
examples/CRISP/client/src/hooks/enclave/useEnclaveServer.ts (1)
8-15: Vote status endpoint wiring is consistentThe
GetVoteStatusendpoint andgetVoteStatushelper are wired consistently with the existing API helpers (types, HTTP method, and payload shape all align). ExposinggetVoteStatusfrom the hook matches the other methods.Also applies to: 23-30, 32-50
examples/CRISP/packages/crisp-sdk/src/types.ts (1)
171-196: previousCiphertext wiring in SDK types is consistentAdding
previousCiphertext?: Uint8ArraytoProofInputs,MaskVoteProofInputs, andVoteProofInputskeeps the types aligned and optional, which matches how the worker/wasm layer forwards this value.examples/CRISP/client/src/hooks/wasm/useWebAssembly.tsx (1)
26-33: generateProof wiring matches worker; ensure it pairs with thevoteIdfixThreading
previousCiphertextthroughgenerateProofand into the worker looks correct. With the worker normalized to handle bothnumberandbigintvoteIdvalues, usingvoteId: biginthere is fine and keeps the API consistent with other bigint-based values.Also applies to: 38-57
examples/CRISP/client/src/components/VotingStepIndicator.tsx (1)
11-35: Step indicator logic now handleserrorand non-linear steps correctly
- Using
lastActiveStepto derivecurrentIndexfixes the prior issue where'error'wasn’t in the step list.- For
'idle'/'confirming', all steps render as pending, which is a reasonable default.- Styling and icon selection cleanly map
complete/active/error/pendingto distinct visuals.Also applies to: 37-64, 66-103
examples/CRISP/client/libs/crispWorker.js (1)
14-15: NormalizevoteIdtype to avoid always casting to the same voteThe worker assumes
voteIdis0or1(number) but if the hook now types it asbigint, thenvoteId === 0will never be true for0nor1n, causing both choices to map to the "yes" vote branch.To make this robust regardless of whether callers send
numberorbigint, normalize first:- const { voteId, publicKey, address, signature, previousCiphertext } = data + const { voteId, publicKey, address, signature, previousCiphertext } = data @@ - const balance = 1n - const vote = voteId === 0 ? { yes: 0n, no: balance } : { yes: balance, no: 0n } + const balance = 1n + const choice = typeof voteId === 'bigint' ? Number(voteId) : voteId + const vote = choice === 0 ? { yes: 0n, no: balance } : { yes: balance, no: 0n } @@ - const proof = await generateVoteProof({ + const proof = await generateVoteProof({ vote, publicKey, signature, merkleLeaves, balance, previousCiphertext, })This keeps the existing semantics while allowing
voteIdto be passed as either0/1or0n/1n.examples/CRISP/server/src/server/routes/voting.rs (1)
88-90: File not found in repositoryThe file
examples/CRISP/server/src/server/routes/voting.rsreferenced in this review comment does not exist in the current repository state. Unable to verify the claimed issues regarding:
- Hex decode failure rollback behavior for updates
- Contract creation error handling for new votes
- Inconsistent use of the
handle_vote_errorfunction with theis_vote_updateflagTo proceed with verification, confirm that:
- The file exists in the repository (check branch, commit, or file path)
- The code snippet locations (lines 88-90, 91-122, etc.) are accurate for the current revision
examples/CRISP/client/src/context/voteManagement/VoteManagement.types.ts (3)
12-16: LGTM! Type definition supports the vote status caching mechanism.The
VoteStatustype is well-structured with appropriate fields for tracking vote state, round ID, and cache timestamp.
28-31: LGTM! Context API expanded to support vote status tracking.The new fields (
currentRoundId,hasVotedInCurrentRound,voteStatusLoading,sessionId) and methods (checkVoteStatus,markVotedInRound) provide a clean API for managing vote status across the application.Also applies to: 51-52
40-46: LGTM! Signature updated to support vote updates.The addition of the optional
previousCiphertextparameter enables the vote update functionality while maintaining backward compatibility.examples/CRISP/client/src/pages/Landing/components/DailyPoll.tsx (4)
23-23: LGTM! Title prop added for flexibility.The addition of the
titleprop with a sensible default value ('Daily Poll') allows for component reusability across different contexts.Also applies to: 26-26
27-27: LGTM! Vote status indicators enhance user feedback.The integration of
hasVotedInCurrentRoundandvoteStatusLoadingprovides clear visual feedback about the user's voting state. The "You voted" and "Checking..." badges improve the UX.Also applies to: 88-97
106-107: LGTM! Loading states are well-coordinated.The conditional rendering ensures that either the
VotingStepIndicator(during casting) orLoadingAnimation(during initial load) is displayed, preventing UI conflicts.
118-132: LGTM! Dynamic button and helper text improve user guidance.The button label correctly toggles between "Cast Vote", "Update Vote", and "Processing Vote..." based on the voting state. The helper text adapting to show "Select an option to update your vote" when the user has already voted provides clear guidance.
examples/CRISP/client/src/context/voteManagement/VoteManagement.context.tsx (4)
21-29: LGTM! Session-scoped cache key generation is well-designed.The
generateSessionIdfunction creates a unique identifier per page load, andgetVoteCacheKeyproperly scopes cache entries by session, round, and address. The 5-minute cache duration (VOTE_CACHE_DURATION) is reasonable for vote status data.
73-107: LGTM! Robust vote status checking with caching.The
checkVoteStatusfunction properly implements:
- Cache lookup with TTL validation
- Force refresh capability
- Loading state management
- Error handling with console logging
- Cache updates on successful API response
The implementation efficiently reduces redundant API calls while maintaining data freshness.
109-126: LGTM! Vote status marking logic is correct.The
markVotedInRoundfunction properly updates both the cache and thehasVotedInCurrentRoundstate. The conditional update on line 122 ensures the state only changes when the specified round matches the current round.
186-202: LGTM! Auto-refresh logic with proper cleanup.The
useEffecthook correctly:
- Checks vote status when the user or current round changes
- Uses a cancellation flag to prevent race conditions
- Cleans up on unmount or when dependencies change
- Resets
hasVotedInCurrentRoundwhen the user or round is not availableThe inclusion of
checkVoteStatusin the dependency array is correct since it's memoized withuseCallback.examples/CRISP/client/src/hooks/voting/useVoteCasting.ts (8)
18-18: LGTM! VotingStep type supports multi-step UI flow.The
VotingSteptype definition provides clear states for the voting process, enabling granular progress tracking and user feedback.
20-44: LGTM! Error message normalization improves UX.The
extractCleanErrorMessagehelper function effectively:
- Handles common blockchain error patterns (gas issues, nonce conflicts, reverts)
- Truncates excessively long messages
- Provides user-friendly fallback messages
This enhances the user experience by presenting clearer error feedback.
106-122: LGTM! Well-structured signature flow with error handling.The signing step properly:
- Updates the voting step and UI message
- Handles user rejection gracefully
- Calls
resetVotingStateon failure to clean up UI state
143-146: LGTM! Broadcasting step with success handling.The broadcasting flow properly:
- Updates voting steps for user feedback
- Handles success response with appropriate actions (marking voted, showing toast, navigating)
- Distinguishes between initial votes and vote updates in the success message
- Calls
markVotedInRoundto update the cacheAlso applies to: 157-173
175-190: LGTM! Enhanced error handling with persistent toasts.Error cases now:
- Use
extractCleanErrorMessageto normalize error messages- Set
persistent: trueto ensure users see the error- Set
votingStepto 'error' for UI feedbackThese improvements ensure users are properly notified of failures and can take corrective action.
195-202: LGTM! Comprehensive catch-all error handling.The catch block properly:
- Sets voting step to 'error'
- Logs the error for debugging
- Shows a persistent error toast with the error message
- Handles both Error instances and other thrown values
221-229: LGTM! Extended return interface supports the enhanced voting flow.The hook now returns all necessary state and functions for consumers to:
- Track voting progress (
votingStep,lastActiveStep,stepMessage)- Reset the voting state (
resetVotingState)- Check if the user has already voted (
hasVotedInCurrentRound)
128-134: Clarify the intent of dummy encryption in vote updates.The code uses a dummy encryption with
{ yes: 0n, no: 0n }aspreviousCiphertextfor vote updates. Per CRISP's design, dummy ciphertexts are intentional—used to achieve coercion-resistance and receipt-freeness by making votes indistinguishable from non-votes. However, verify that:
- This dummy approach is correct for the vote update flow and matches how the proof generation validates previous ciphertexts
- The contract or server verification logic accepts dummy ciphertexts for vote updates without expecting actual previous plaintext knowledge
- The TODO comment reflects documentation debt rather than incomplete implementation
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/CRISP/server/src/server/routes/voting.rs (1)
156-169: Database consistency issue: rollback missing on contract creation failure.When contract creation fails for a new vote, the voter address remains in the database but no vote is broadcast. This leaves the system in an inconsistent state where
has_votedreturnstruebut no vote was actually recorded on-chain.Apply this diff to rollback the voter insertion on contract creation failure:
Ok(c) => c, Err(e) => { error!("[e3_id={}] Contract creation error: {:?}", vote.round_id, e); + + // Rollback voter insertion for new votes + if !is_vote_update { + let _ = match repo.remove_voter_address(&vote.address).await { + Ok(_) => (), + Err(e) => error!("Error rolling back the vote: {e}"), + }; + } + return HttpResponse::InternalServerError().json("Internal server error"); } };
🧹 Nitpick comments (2)
examples/CRISP/server/src/server/routes/voting.rs (2)
62-65: Consider logging round status query failures.The round status query error is silently suppressed and returns
None. While this prevents the entire request from failing, it might hide legitimate issues.Consider adding logging:
let round_status = match store.e3(request.round_id).get_e3_state_lite().await { Ok(state) => Some(state.status), - Err(_) => None, + Err(e) => { + error!("[e3_id={}] Failed to get round status: {:?}", request.round_id, e); + None + } };
193-217: Replace string-based error matching with structured error type handling.The error extraction via
contains()is fragile and not idiomatic for alloy 1.0.41. Alloy provides structured error types (RpcError,alloy::contract::Errorwith variants likeUnknownFunction,AbiError,TransportError) and helper methods (as_decoded_error(),as_revert_data(),as_decoded_interface_error()) that should be used instead of parsingto_string()output. String-based matching will break if upstream error messages change format.Prefer pattern matching on error enum variants or using alloy's error decoding helpers. For errors wrapped in
eyre::Error, usedowncast_ref()or chain inspection to access the underlying structured error type before branching on specific error conditions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/CRISP/server/src/server/routes/voting.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-05T06:48:58.177Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 173
File: packages/ciphernode/config/src/app_config.rs:13-21
Timestamp: 2024-11-05T06:48:58.177Z
Learning: In the `packages/ciphernode/config/src/app_config.rs` file, for the `Contract` enum, the team prefers to use `String` type for `address` fields, relying on parsing to handle validation, instead of using the `Address` type.
Applied to files:
examples/CRISP/server/src/server/routes/voting.rs
📚 Learning: 2024-11-25T09:56:11.195Z
Learnt from: ryardley
Repo: gnosisguild/enclave PR: 184
File: packages/ciphernode/net/src/network_relay.rs:0-0
Timestamp: 2024-11-25T09:56:11.195Z
Learning: In `NetworkPeer::new` in `packages/ciphernode/net/src/network_peer.rs`, peers are converted to multiaddrs, and validation of the address format occurs during this conversion. Duplicate addresses are managed as connections are deduplicated.
Applied to files:
examples/CRISP/server/src/server/routes/voting.rs
🧬 Code graph analysis (1)
examples/CRISP/server/src/server/routes/voting.rs (2)
examples/CRISP/client/src/model/vote.model.ts (2)
VoteStatusRequest(38-41)VoteStatusResponse(43-48)examples/CRISP/server/src/server/repo.rs (1)
has_voted(229-232)
⏰ 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). (11)
- GitHub Check: build_sdk
- GitHub Check: crisp_unit
- GitHub Check: build_e3_support_dev
- GitHub Check: integration_prebuild
- GitHub Check: build_enclave_cli
- GitHub Check: build_ciphernode_image
- GitHub Check: rust_unit
- GitHub Check: test_net
- GitHub Check: rust_integration
- GitHub Check: test_contracts
- GitHub Check: Build & Push Image
🔇 Additional comments (2)
examples/CRISP/server/src/server/routes/voting.rs (2)
7-26: LGTM!The imports and route configuration correctly support the new vote status endpoint and vote update functionality.
171-191: LGTM!The broadcast logic correctly handles both new votes and updates, with appropriate success messages and proper error delegation.
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.