fix: ereputation signing session logic and cosmetic changes #458
Conversation
WalkthroughThe PR extends eReputation API response schemas to include Changes
Sequence DiagramsequenceDiagram
participant Client
participant ReferenceSigningSessionService
participant sessionStorage as Module-Level Storage
Client->>ReferenceSigningSessionService: createSession(userId, payload)
ReferenceSigningSessionService->>sessionStorage: sessions.set(sessionId, session)
Client->>ReferenceSigningSessionService: processSignedPayload(sessionId, signature, publicKey)
ReferenceSigningSessionService->>sessionStorage: sessions.get(sessionId)
rect rgb(255, 240, 245)
Note over ReferenceSigningSessionService: Signature & User-Key Validation
alt Validation Success
ReferenceSigningSessionService->>sessionStorage: update session.status = "completed"
ReferenceSigningSessionService->>Client: ✓ Completion confirmed
else Security Violation
ReferenceSigningSessionService->>sessionStorage: emit security_violation event
ReferenceSigningSessionService->>Client: ✗ Security violation detected
Note over Client: Log diagnostic info (storage size, keys)
end
end
Client->>ReferenceSigningSessionService: getSession(sessionId)
ReferenceSigningSessionService->>sessionStorage: sessions.get(sessionId)
ReferenceSigningSessionService->>Client: return session
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
coodos
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: 1
🧹 Nitpick comments (4)
platforms/eReputation-api/src/services/CalculationService.ts (1)
158-158: Consider improving prompt clarity and grammar.The prompt instruction is lengthy and has minor grammar issues. The phrase "which mentions" should be "that mention" (plural agreement), and the compound sentence could be broken up for better clarity.
Consider this refactor:
-You are analyzing the reputation of "${targetName}" based on the following user values and references. Remember that the user may also supply a wishlist, only focus on the part of the wishlist and values which mentions what the user values in a person, group, or platform, depending on what type of profile it is that you are evaluating, do not care about anything else in the wishlist. +You are analyzing the reputation of "${targetName}" based on the following user values and references. + +If a wishlist is provided, only focus on the parts that describe what the user values in the profile type being evaluated (person, group, or platform). Ignore other wishlist content.platforms/eReputation/client/src/components/modals/reference-modal.tsx (1)
519-530: Consider extracting duplicated target rendering logic.The target result rendering logic (lines 519-530) is nearly identical to the same logic in
platforms/eReputation/client/src/components/modals/other-calculation-modal.tsx(lines 477-486). This duplication creates maintenance overhead.Consider extracting this into a shared component like
<TargetResultItem>that both modals can use:// components/ui/target-result-item.tsx interface TargetResultItemProps { result: any; targetType: string; onClick: () => void; } export function TargetResultItem({ result, targetType, onClick }: TargetResultItemProps) { return ( <button onClick={onClick} className="w-full text-left px-4 py-3 hover:bg-fig-10 transition-colors border-b border-fig/10 last:border-b-0 first:rounded-t-2xl last:rounded-b-2xl"> <div className="flex items-center gap-3"> <div className="w-8 h-8 bg-fig/10 rounded-lg flex items-center justify-center"> {/* icon logic */} </div> <div className="flex-1 min-w-0"> <div className="font-bold text-fig truncate"> {result.name || result.ename || result.handle || 'Unknown'} </div> <div className="text-xs text-fig/70"> {targetType === 'user' ? ( result.ename ? (result.ename.startsWith('@') ? result.ename : `@${result.ename}`) : 'no ename' ) : ( <span className="capitalize">{result.type || result.category || targetType}</span> )} </div> </div> </div> </button> ); }platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts (2)
41-42: Module-level singleton storage fixes session lookup but has architectural limitations.This change likely addresses the core bug mentioned in the PR title. By using a module-level singleton instead of per-instance storage, sessions are now shared across all
ReferenceSigningSessionServiceinstances, preventing "session not found" errors when different instances handle creation vs. lookup.However, this approach has limitations:
- Sessions are lost on server restart
- Not suitable for multi-process/distributed deployments
- Sessions exist only in memory of a single Node.js process
For production deployments:
- Consider using Redis or another distributed cache for session storage
- Add session persistence to handle server restarts gracefully
- Implement session cleanup to prevent memory leaks from expired sessions
109-110: Diagnostic logging helps debugging but consider log levels.The added logging at lines 109-110 and 115 helps diagnose the "session not found" issue. However, logging all session IDs (line 110) could be verbose in production with many active sessions.
Consider using debug-level logging or limiting the output:
- console.log(`Current session storage size: ${sessionStorage.size}`); - console.log(`Session IDs in storage:`, Array.from(sessionStorage.keys())); + console.log(`Current session storage size: ${sessionStorage.size}`); + if (sessionStorage.size < 20) { + console.log(`Session IDs in storage:`, Array.from(sessionStorage.keys())); + }Also applies to: 115-115
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
platforms/eReputation-api/src/controllers/GroupController.ts(1 hunks)platforms/eReputation-api/src/services/CalculationService.ts(1 hunks)platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts(6 hunks)platforms/eReputation/client/src/components/modals/other-calculation-modal.tsx(1 hunks)platforms/eReputation/client/src/components/modals/reference-modal.tsx(2 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). (1)
- GitHub Check: build
🔇 Additional comments (5)
platforms/eReputation/client/src/components/modals/other-calculation-modal.tsx (1)
477-486: LGTM! Improved layout with proper truncation and ename handling.The updated rendering logic properly handles long names with truncation and conditionally displays ename for user targets with appropriate fallbacks. The layout improvements using
flex-1 min-w-0ensure text truncation works correctly in flexbox containers.platforms/eReputation/client/src/components/modals/reference-modal.tsx (1)
547-553: LGTM! Proper truncation handling for selected target display.The layout updates correctly prevent the icon from shrinking (
flex-shrink-0) while allowing the text to truncate properly withflex-1 min-w-0and thetruncateclass. This ensures long target names don't break the layout.platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts (2)
76-77: LGTM! Consistent usage of module-level sessionStorage.All references to session storage have been consistently updated to use the module-level
sessionStorageMap instead of instance-level storage. The logging at line 77 helps with debugging session state.Also applies to: 81-81, 84-84, 92-92, 101-101, 153-153, 192-192
142-164: LGTM! Proper security violation handling with graceful error response.The security check correctly validates that the signing public key matches the user's ename who created the session. The implementation:
- Strips @ prefixes for comparison (lines 139-140)
- Updates session status to
security_violation(lines 152-153)- Returns an error result instead of throwing, enabling graceful client-side handling
- Logs detailed diagnostic information (lines 143-149)
The client-side SSE handler in
reference-modal.tsx(lines 187-194) properly handles this security_violation status, showing an appropriate user message.platforms/eReputation-api/src/controllers/GroupController.ts (1)
32-32: ename field is properly defined and nullable in Group entity.The verification confirms that the Group entity in
platforms/eReputation-api/src/database/entities/Group.ts(line 61-62) includes theenamefield with@Column({ nullable: true}), allowing it to be null in the database. The response mapping correctly includes this field, and since it's properly marked as nullable, passing it to the client as-is is appropriate.
platforms/eReputation-api/src/services/ReferenceSigningSessionService.ts
Show resolved
Hide resolved
Description of change
fix the bug with eReputation signing modal having issues
Issue Number
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
New Features
enamefield in group search results and target selection modals.Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.