echo implementation in keyset create flow, update igloo-core to latest version to support#2
Conversation
…t version to support
WalkthroughAdds share-echo functionality: a sendShareEcho utility and export, a useShareEchoListener React hook (with retry/backoff/warnings), UI integrations in KeysetLoad, ShareSaver, and ShareAdd to show echo status, updates igloo-core sendEcho docs/signature, bumps @frostr/igloo-core, and revises AGENTS.md. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant C as Component (KeysetLoad/ShareSaver)
participant H as useShareEchoListener
participant D as igloo-core
participant R as Relays
U->>C: Open load/save view
C->>H: useShareEchoListener(groupCredential, shareCredential)
H->>H: Decode group to infer relays (optional)
H->>D: awaitShareEcho(shareCredential, { relays?, timeout })
D-->>R: Subscribe for echo
Note over H,D: Status = listening (warning timer may fire)
R-->>D: Echo confirmation
D-->>H: Success
H-->>C: status = success, message = null
C-->>U: Show "Echo confirmed"
alt Timeout / transient error
H->>H: Backoff and retry (maxRetries)
end
sequenceDiagram
autonumber
actor U as User
participant S as ShareAdd
participant K as sendShareEcho
participant D as igloo-core
participant R as Relays
U->>S: Save share
S->>S: On success, reset echo state
S->>K: sendShareEcho(groupCredential, shareCredential, opts)
alt No challenge provided
K->>K: Generate 64-hex challenge
end
K->>D: sendEcho(groupCredential, shareCredential, challenge, { relays, timeout })
D-->>R: Publish echo
R-->>D: Ack/confirmation
D-->>S: Promise resolved
S-->>U: Show echo success (or error)
Note over K: Challenge must be even-length hex (64 hex chars typical)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
AGENTS.md(1 hunks)llm/context/igloo-core-readme.md(3 hunks)package.json(1 hunks)src/components/keyset/KeysetLoad.tsx(3 hunks)src/components/keyset/ShareSaver.tsx(6 hunks)src/components/keyset/useShareEchoListener.ts(1 hunks)src/components/share/ShareAdd.tsx(6 hunks)src/keyset/echo.ts(1 hunks)src/keyset/index.ts(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
src/components/keyset/ShareSaver.tsx
[error] 209-209: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (11)
src/components/share/ShareAdd.tsx (3)
1-1: LGTM: Echo state and unmount guard correctly implemented.The imports, state declarations, and
isMountedRefpattern are appropriate for preventing state updates after component unmount. The echo status tracking states (echoStatusandechoError) are well-typed and initialized correctly.Also applies to: 20-20, 84-85, 92-98
301-376: LGTM: Fire-and-forget echo pattern correctly implemented.The echo logic is properly integrated:
- State is reset at the start of save (lines 309-310)
- Echo is sent asynchronously after successful save without blocking the UI transition to 'done' mode
isMountedRefguards prevent state updates after unmount- Error handling captures the message appropriately
The fire-and-forget pattern (void IIFE) is intentional and allows the save flow to complete immediately while echo status updates asynchronously in the background.
530-540: LGTM: Clear and informative echo status UI.The echo status rendering is well-implemented with:
- Clear, concise status messages
- Appropriate color coding (cyan for pending, green for success, yellow for error)
- Conditional error details when available
src/components/keyset/KeysetLoad.tsx (2)
63-68: LGTM: Echo listener hook correctly integrated.The
useShareEchoListenerhook is properly invoked at the top level with the decrypted credentials. The hook correctly handles null inputs whenresultis not yet available.
222-234: LGTM: Echo status UI is clear and informative.The echo status rendering provides helpful feedback:
- Waiting state with optional share index context
- Success confirmation message
- Warning messages displayed when provided by the listener
src/keyset/echo.ts (1)
1-8: LGTM: Clean imports and type definition.The imports and
SendShareEchoOptionstype are well-structured, providing a flexible API with all optional parameters.src/components/keyset/ShareSaver.tsx (5)
18-18: LGTM!The import of
useShareEchoListeneris correctly placed and enables the echo functionality integration.
77-89: LGTM!The credential display blocks improve readability by breaking out the share and group credentials into dedicated, consistently styled sections. This makes the UI clearer across all phases.
214-236: LGTM!The
renderEchoStatushelper cleanly handles the three echo states (idle, listening, success) and provides clear user feedback at each stage.
238-311: Verify whether automated flow should display echo status.The automated flow (when
autoPasswordis provided) saves all shares in a batch but does not integrate or display echo status. This differs from the manual flow, which shows echo feedback.Is this intentional, or should the automated flow also listen for and display echo confirmations? If echo feedback is needed, consider integrating
renderEchoStatus()in the automated flow's UI sections (lines 294-310).
313-322: LGTM!The integration of credential blocks and echo status rendering across all manual flow phases ('saving', 'done', and main password prompt) provides consistent, informative feedback to users about share transmission status.
Also applies to: 324-343, 345-373
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/keyset/echo.ts (1)
10-27: Verify sendEcho API signature matches igloo-core implementation.The code calls
sendEchowithchallengeas a separate third parameter (line 23), but according to previous reviews and available package documentation, the actual signature issendEcho(groupCredential, shareCredential, options?)wherechallengewould be inside the options object, not a separate parameter.This issue was previously flagged. Please verify the actual API signature:
#!/bin/bash # Check the actual sendEcho signature in installed package echo "=== Checking installed @frostr/igloo-core version ===" npm list @frostr/igloo-core echo -e "\n=== Looking for sendEcho signature in type definitions ===" fd -t f -e ts -e d.ts . node_modules/@frostr/igloo-core -x cat {} \; | grep -A10 "sendEcho" echo -e "\n=== Searching for sendEcho in source files ===" fd -t f -e js -e ts . node_modules/@frostr/igloo-core/dist node_modules/@frostr/igloo-core/src 2>/dev/null -x grep -l "sendEcho" {} \; | head -5 | xargs catIf the API has changed in 0.2.1, update the call to match. If not, move
challengeinto the options object.Based on learnings
🧹 Nitpick comments (2)
src/components/keyset/useShareEchoListener.ts (1)
131-196: Echo listener implementation with workaround for upstream bug.The implementation uses a race condition between
awaitShareEchoand a 15-second fallback timer as a workaround for a bug whereawaitShareEchodoesn't resolve. The fallback timer is properly tracked (line 143), nulled immediately (line 144), and cleaned up in multiple paths:
- Success path (lines 158-161)
- Finally block (lines 188-191)
- clearPending (lines 93-96)
The retry logic with exponential backoff (line 181) and max retries check (lines 172-176) is well-implemented.
Do you want me to generate a verification script to check if there are any remaining references to the
awaitShareEchobug or if the upstream issue has been resolved in the new igloo-core version?llm/context/igloo-core-readme.md (1)
322-343: Clarify challenge generation for browser consumers.The example now imports
randomBytesfrom Node’scryptomodule, which breaks in browser builds (the CLI targets React flows too). Let's drop in a short note or alternate snippet usingcrypto.getRandomValuesso web clients aren’t left with an unresolved module error.import { sendEcho } from '@frostr/igloo-core'; -import { randomBytes } from 'crypto'; +// Node.js example: +import { randomBytes } from 'crypto'; + +// Browser example: +// const challengeBytes = crypto.getRandomValues(new Uint8Array(32)); +// const challenge = Buffer.from(challengeBytes).toString('hex'); try { - const challenge = randomBytes(32).toString('hex'); // 32-byte (64 hex char) challenge + const challenge = randomBytes(32).toString('hex'); // 32-byte (64 hex char) challengeAlso consider adding a sentence below noting either approach is fine as long as the final challenge is an even-length hex string.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
AGENTS.md(1 hunks)llm/context/igloo-core-readme.md(3 hunks)package.json(1 hunks)src/components/keyset/KeysetLoad.tsx(3 hunks)src/components/keyset/ShareSaver.tsx(6 hunks)src/components/keyset/useShareEchoListener.ts(1 hunks)src/components/share/ShareAdd.tsx(6 hunks)src/keyset/echo.ts(1 hunks)src/keyset/index.ts(1 hunks)
🔇 Additional comments (23)
src/components/keyset/useShareEchoListener.ts (5)
1-42: LGTM! Clean type definitions and relay extraction.The imports, type definitions, and relay extraction logic are well-structured. The
extractRelayshelper correctly handles multiple relay field naming conventions (relays,relayUrls,relay_urls) with proper validation.
44-78: LGTM! Well-designed hook initialization.The hook signature, default values, and relay extraction logic are well-thought-out. The graceful handling of decode failures for relay extraction (lines 71-76) prevents the hook from breaking when group credentials are malformed.
80-97: LGTM! Proper cleanup implementation.The
clearPendingfunction correctly cleans up all three timeout refs (retry, warning, and fallback) and cancels the controller. This addresses the previous review concern about the fallback timer cleanup.
99-130: LGTM! Solid initialization and warning logic.The initialization sequence is well-structured with proper credential validation, controller setup, and warning timeout configuration. The cancellation check on line 122 correctly prevents state updates after cleanup.
198-222: LGTM! Clean effect and return.The effect properly manages the listener lifecycle with cleanup on unmount. The trigger-based retry mechanism (lines 217-219) is elegant and avoids recreating the
startListeningcallback unnecessarily.src/keyset/echo.ts (1)
1-8: LGTM! Clean imports and type definition.The type
SendShareEchoOptionsis well-structured with sensible optional parameters for relay configuration, challenge, and timeout.AGENTS.md (4)
4-4: LGTM! Clear project structure guidance.The updated structure documentation clearly outlines the organization with specific file locations and the 150-line split guideline. The reminder to keep
llm/prompts updated with UX/protocol changes is valuable for maintaining consistency.
7-7: LGTM! Comprehensive development workflow guidance.The build, test, and development commands section provides clear instructions with the important "release blocker" policy for test failures.
10-10: LGTM! Thorough coding style conventions.The style guide covers essential conventions with clear examples. The emphasis on annotating "non-obvious flows with concise comments, especially around key lifecycle management" (line 10) is particularly valuable for this security-focused codebase.
13-19: LGTM! Essential testing, PR, and security guidelines.The remaining sections provide critical guidance:
- Testing prioritizes type coverage with manual validation documentation
- PR guidelines appropriately flag
llm/and cryptographic changes for thorough review- Security tips emphasize the importance of never committing secrets and removing generated keys
These are all appropriate for a security-focused CLI tool handling cryptographic material.
src/keyset/index.ts (1)
7-7: LGTM! Clean public API extension.The re-export from
./echo.jsfollows the established pattern and properly exposes the new echo functionality (SendShareEchoOptionsandsendShareEcho) to consumers.src/components/keyset/KeysetLoad.tsx (3)
1-68: LGTM! Proper hook integration.The
useShareEchoListenerhook is correctly imported (line 6) and called unconditionally at the top level (lines 65-68), following React's Rules of Hooks. The hook gracefully handlesnullcredentials when no result is available yet.
222-234: LGTM! Clear echo status UI.The echo status rendering provides good user feedback:
- "Listening" state shows a helpful waiting message with optional share index (lines 225-227)
- Warning messages are displayed in yellow (line 228)
- Success state confirms echo reception (lines 231-233)
The conditional rendering ensures the UI only appears when relevant.
70-236: Well-integrated echo feature in load flow.The echo status is appropriately displayed only in the result phase (lines 222-234), after successful share decryption. The component maintains clean phase separation while adding the new echo monitoring capability.
src/components/share/ShareAdd.tsx (4)
1-98: LGTM! Proper state and lifecycle management.The additions are well-structured:
useRefimport (line 1) andsendShareEchoimport (line 20)- Echo state tracking with
echoStatusandechoError(lines 84-85)- Mounted ref pattern (lines 92-98) correctly prevents state updates after unmount
301-370: LGTM! Proper async echo handling with stale state prevention.The echo integration in
handleSaveis well-implemented:
- Echo state reset before save (lines 309-310)
- Pending status set before mode transition (line 354)
- Async IIFE with
voidkeyword (line 356) correctly indicates fire-and-forget- Mounted checks (lines 359-361, 364-366) prevent state updates after unmount
- Proper error capture (lines 367-368)
517-540: LGTM! Comprehensive echo status feedback.The echo UI in the "done" mode provides clear feedback for all states:
- Pending: informative message (lines 530-532)
- Success: positive confirmation (lines 533-535)
- Error: warning with optional detail (lines 536-540)
67-640: Well-integrated echo feature in share add flow.The echo functionality is properly integrated:
- Triggered only after successful save (line 356)
- Doesn't block or interfere with the main save flow
- Status displayed appropriately in the "done" phase
- Clean separation from core save logic
src/components/keyset/ShareSaver.tsx (4)
1-95: LGTM! Hook violation issue resolved.The
useShareEchoListenerhook is now correctly called unconditionally at the top level (lines 77-80), before any early returns. This resolves the previous critical issue about violating React's Rules of Hooks.The credential blocks (lines 82-94) are cleanly defined as reusable JSX elements.
214-236: LGTM! Clear echo status rendering.The
renderEchoStatusfunction provides helpful feedback:
- Guards against missing share (lines 215-217)
- Listening state with optional message (lines 218-226)
- Success state with visual indicator (lines 228-234)
The bold checkmark on success (line 231) is a nice UX touch.
313-372: LGTM! Consistent echo status placement.The echo status is consistently rendered across all phases:
- During saving (line 319)
- After completion (line 331)
- During password entry (line 350)
The consistent placement after credential blocks provides a predictable user experience.
39-374: Echo listener properly integrated for multi-share flow.The component correctly passes
share?.credential(line 79) to the hook, which means the echo listener will restart for each share ascurrentIndexchanges. This is appropriate behavior for tracking echo status per-share in a multi-share saving workflow.package.json (1)
26-26: Dependency bump valid:@frostr/igloo-core@0.2.1exists on npm and is the latest version.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores