Feat/ereputation adapter #439
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request implements a comprehensive eReputation system architecture by migrating the eReputation platform from a monolithic server to a microservices-based API, adding client-side authentication flows, integrating reputation calculations into eVoting polls, and adding dependent infrastructure updates across multiple platforms. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as eReputation Client
participant Auth as Auth API
participant SSE as SSE Stream
participant CalcAPI as Calculation API
participant OpenAI as OpenAI
participant DB as Database
participant Queue as Job Queue
participant Worker as Reputation Worker
Client->>Auth: GET /api/auth/offer
Auth-->>Client: { offer, sessionId }
Client->>SSE: Subscribe /api/auth/sessions/{sessionId}
Client->>Auth: POST /api/auth/login (ename, sessionId)
Auth->>DB: Find user by ename
Auth->>Auth: Sign JWT token
Auth->>SSE: Emit user + token
SSE-->>Client: { user, token }
Client->>Client: Store token + userId
Client->>CalcAPI: POST /api/reputation/calculate (targetType, targetId, userValues)
CalcAPI->>DB: Create Calculation record (status=processing)
CalcAPI->>DB: Fetch signed references for target
CalcAPI->>OpenAI: POST to GPT-4 with references + userValues
OpenAI-->>CalcAPI: { score, analysis }
CalcAPI->>DB: Update Calculation (status=complete, score, analysis)
CalcAPI-->>Client: { calculationId, score, analysis }
alt eReputation-weighted poll
Queue->>Worker: Poll reputation job
Worker->>DB: Load poll + group members
Worker->>OpenAI: Batch member reputation calculation
Worker->>DB: Save VoteReputationResult
Worker->>Subscriber: Emit results via adapter
Subscriber->>Client: Webhook with reputation data
end
sequenceDiagram
participant DB as PostgreSQL
participant Subscriber as Change Subscriber
participant Adapter as Web3 Adapter
participant Webhook as External Webhook
DB->>Subscriber: afterInsert/Update/Remove event
Subscriber->>Subscriber: Enrich entity (load relations)
Subscriber->>Subscriber: Debounce (3s for groups, junctions)
alt Regular entity
Subscriber->>Adapter: handleChange(entity)
Adapter->>Adapter: fromGlobal() convert
Adapter->>Adapter: Check reentry guard
Adapter->>Adapter: Store mapping
Adapter->>Webhook: Send payload
else Junction table
Subscriber->>Adapter: Resolve parent entity
Subscriber->>Subscriber: Load full parent
Subscriber->>Adapter: handleChange(parent)
Adapter->>Webhook: Send parent webhook
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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
force-pushed
the
feat/ereputation-adapter
branch
from
fac632a to
92da162
Compare
coodos
force-pushed
the
feat/ereputation-adapter
branch
from
09ed918 to
1ee45c3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (12)
platforms/eVoting/src/app/globals.css (2)
283-310: Remove hardcodedcolor-scheme: lightto support dark mode.Line 285 sets
color-scheme: lightunconditionally on datetime-local inputs, which breaks dark mode support defined at lines 159–179. The file already defines a.darktheme; this override prevents proper dark theme application to form inputs.Additionally, line 309 uses a universal selector
* { accent-color: ... !important }to force styling globally. This is a CSS anti-pattern that indicates underlying specificity/cascade issues.Apply this diff:
/* Style all date/time inputs to use red accent color */ input[type="datetime-local"] { accent-color: var(--crimson) !important; - color-scheme: light; }Consider refactoring the button styling (lines 319–351) to use a
button[class*="crimson"]selector instead of the current complex, fragile selectors with excessive!importantflags.
194-206: Consolidate duplicate calendar picker styling—confirmed dead code and anti-pattern usage.The
input[type="datetime-local"]::-webkit-calendar-picker-indicatorselector is defined twice with conflicting properties:
- Lines 194–206:
border: 1px solid,padding: 4px, complex filter- Lines 236–255:
border: 2px solid !important,padding: 4px 6px !important,filter: none !important, plus 17 additional !important declarationsThe second definition completely overrides the first, making lines 194–206 dead code. The extensive use of
!importantis an anti-pattern indicating reactive styling rather than deliberate architecture—consolidate these rules into a single definition that clearly reflects the intended behavior.platforms/group-charter-manager-api/src/controllers/WebhookController.ts (4)
30-35: Add error handling for ANCHR forwarding.The
axios.postcall has no error handling and could throw an exception, disrupting webhook processing. Consider wrapping it in a try-catch block or using.catch()to handle failures gracefully.if (process.env.ANCHR_URL) { - axios.post( - new URL("group-charter-manager", process.env.ANCHR_URL).toString(), - req.body - ); + axios.post( + new URL("group-charter-manager", process.env.ANCHR_URL).toString(), + req.body + ).catch(err => { + console.error("Failed to forward webhook to ANCHR:", err); + }); }
130-131: Improve admin ID parsing robustness.The admin ID parsing logic assumes a specific format
"prefix(id)"and will fail silently or produce incorrect results if the format is different. Add validation and error handling.let admins = local?.data?.admins as string[] ?? [] - admins = admins.map((a) => a.includes("(") ? a.split("(")[1].split(")")[0]: a) + admins = admins.map((a) => { + if (a.includes("(") && a.includes(")")) { + const match = a.match(/\(([^)]+)\)/); + return match ? match[1] : a; + } + return a; + })
113-122: Improve participant ID extraction robustness.The participant ID extraction assumes a specific format
"prefix(userId)"and uses fragile string parsing. If the format is different, this will fail silently. Add validation and consider using regex for more robust parsing.const participantPromises = local.data.participants.map( async (ref: string) => { if (ref && typeof ref === "string") { - const userId = ref.split("(")[1].split(")")[0]; + const match = ref.match(/\(([^)]+)\)/); + if (!match) { + console.warn("Invalid participant reference format:", ref); + return null; + } + const userId = match[1]; console.log("Extracted userId:", userId); return await this.userService.getUserById(userId); } return null; } );
171-174: Ensure locked IDs are released on error.If an error occurs during webhook processing, the
globalIdremains locked permanently, preventing future webhooks for this entity from being processed. Add proper cleanup in afinallyblock.res.status(200).send(); } catch (e) { console.error("Webhook error:", e); res.status(500).send(); + } finally { + // Ensure the lock is released even if an error occurs + if (globalId && this.adapter.lockedIds.includes(globalId)) { + this.adapter.removeFromLockedIds(globalId); + } }platforms/evoting-api/src/web3adapter/watchers/subscriber.ts (1)
389-440: Async errors insetTimeoutcallbacks bypass the try/catch blocks and can cause unhandled rejectionsBoth methods have the same pattern: the
try/catchwraps thesetTimeoutcall itself (which is synchronous), not the async work inside. Errors fromawait this.adapter.mappingDb.getGlobalId()orawait this.adapter.handleChange()will escape the outer catch.The suggested pattern—wrapping async logic in an IIFE with an internal try/catch—is correct and aligns with patterns already used in other platforms in this codebase (e.g., dreamsync-api, eReputation-api use debouncing with internal error handling).
Apply the same fix to both
handleChange(line ~415) andhandleJunctionTableChange(line ~463):setTimeout(() => { (async () => { try { // async work here } catch (error) { console.error(`Error:`, error); } })(); }, 3_000);platforms/eVoting/src/components/navigation.tsx (1)
37-37: MoveusePathname()to component top levelThe code violates React's Rules of Hooks.
usePathname()is called inside the nestedisActivefunction, which will invoke the hook multiple times per render wheneverisActive(path)is called in the component (lines 165, 170-173). Hooks must be called at the component top level, not inside nested functions.Refactor as suggested:
const pathname = usePathname(); const isActive = (path: string) => pathname === path;This preserves behavior while satisfying hook rules and will eliminate any linting or runtime warnings.
platforms/evoting-api/src/services/PollService.ts (1)
178-244: Add runtime validation forvotingWeightand align error messagingThe new votingWeight support is useful and the “no eReputation + rank” guard is a good domain constraint, but there are two points to tighten up:
- Missing validation of
pollData.votingWeightvaluesconst votingWeight = (pollData.votingWeight || "1p1v") as "1p1v" | "ereputation";This cast only satisfies TypeScript and does not prevent clients from sending arbitrary strings. An invalid value will either:
- Break at the database layer if
votingWeightis an enum column, or- Persist inconsistent state if the DB accepts unexpected strings.
It’s better to validate explicitly and fail early with a clear error, e.g.:
const rawVotingWeight = pollData.votingWeight ?? "1p1v"; const allowedVotingWeights = new Set<["1p1v", "ereputation"]>(["1p1v", "ereputation"]); if (!allowedVotingWeights.has(rawVotingWeight as any)) { throw new Error(`Invalid votingWeight "${rawVotingWeight}". Allowed values are "1p1v" and "ereputation".`); } const votingWeight = rawVotingWeight as "1p1v" | "ereputation";Then keep using
votingWeightinpollDataForEntityand the rank‑mode guard as you already do.
- Wording of the RBV error
The error says:
“Please use Simple or PBV mode instead.”
while
modeis typed as"normal" | "point" | "rank". If the public API / UI exposes “Simple” and “PBV” as labels for"normal"/"point", that’s fine; otherwise, it might be clearer to reference the actual enum values or both label and value (e.g.normal ("Simple")).platforms/evoting-api/src/controllers/PollController.ts (1)
55-75: Remove debug console.log statements before merging.Multiple debug console.log statements are present in the production code (lines 55, 59, 75). These should be removed or replaced with proper logging infrastructure before merging to main.
Apply this diff to remove the debug statements:
- console.log('🔍 Full request body:', req.body); const { title, mode, visibility, votingWeight, options, deadline, groupId } = req.body; const creatorId = (req as any).user.id; - console.log('🔍 Extracted data:', { title, mode, visibility, votingWeight, options, deadline, groupId, creatorId }); console.log('🔍 groupId type:', typeof groupId, 'value:', groupId); // groupId is optional - only required for system messages @@ -71,7 +68,6 @@ groupId }); - console.log('🔍 Created poll:', poll); res.status(201).json(poll);platforms/eReputation/client/src/components/modals/other-calculation-modal.tsx (1)
96-106: Stale mutation data can leak between calculations
calculateMutation's result is used in the completion effect, but:
resetForm()clears local state (targetType,searchQuery,selectedTarget,userValues, etc.) but does not reset the mutationhandleStartCalculationstarts a new run withcalculateMutation.mutate()without clearing the previouscalculateMutation.dataOn a second (or later) run,
calculateMutation.datawill still hold the previous result until the new request finishes. If the progress bar reaches 100% before the new response arrives, the effect will treat the stale data as fresh and show the old result for the new target.To fix this, reset the mutation before starting a new calculation or when closing:
const calculateMutation = useMutation({ ... }); const handleStartCalculation = () => { + calculateMutation.reset(); ... setIsCalculating(true); setCurrentStep(0); setProgress(0); calculateMutation.mutate(); }; const resetForm = () => { ... setReputationResult(null); + calculateMutation.reset(); };This keeps the completion logic tied to the current request only.
Also applies to: 122-144, 146-179, 181-206, 222-255
platforms/eVoting/src/app/(app)/create/page.tsx (1)
518-586: Adjust the helper text—the suffix is not applied to poll titlesThe helper text at line 584 is misleading. The codebase does not append "(eReputation Weighted)" to poll titles:
- Frontend (
create/page.tsx:211): sendstitle: data.titleunchanged- API client (
pollApi.ts): postspollDatawithout modification- Backend (
PollService.ts:220): storestitle: pollData.titleas-isThe title flows through unchanged across the entire stack. No code appends the suffix anywhere.
Update the helper text to accurately reflect the current behavior:
{watchedMode === "rank" ? "Not available with Rank Based Voting" : "Votes weighted by each voter's eReputation score"}
platforms/eReputation/client/src/components/modals/view-reputation-modal.tsx
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
sosweetham
left a comment
sosweetham
left a comment
There was a problem hiding this comment.
i dont actually see the removed turbo files in the diff
| minSwipeDistance: 60, | ||
| })} | ||
| onswipe={() => handleSwipe?.(isPaneOpen)} | ||
| use:swipe |
There was a problem hiding this comment.
| // biome-ignore lint/suspicious/noExplicitAny: svelte-gestures type definitions are incomplete | ||
| const swipe = swipeResult.swipe as any; |
| undefined, | ||
| true | ||
| ); | ||
| const swipe = swipeResult.swipe as any; |
xPathin
left a comment
xPathin
left a comment
There was a problem hiding this comment.
Just this little thing, the CodeRabbit Suggestions and what Soham Said but other than that it LGTM.
| export function getAppStoreLink(): string { | ||
| if (typeof window === 'undefined') { | ||
| return "https://play.google.com/store/apps/details?id=foundation.metastate.eid_wallet"; | ||
| } | ||
|
|
||
| const userAgent = navigator.userAgent || navigator.vendor || (window as any).opera; | ||
|
|
||
| if (/android/i.test(userAgent)) { | ||
| return "https://play.google.com/store/apps/details?id=foundation.metastate.eid_wallet"; | ||
| } | ||
|
|
||
| if (/iPad|iPhone|iPod/.test(userAgent) && !(window as any).MSStream) { | ||
| return "https://apps.apple.com/in/app/eid-for-w3ds/id6747748667"; | ||
| } | ||
|
|
||
| return "https://play.google.com/store/apps/details?id=foundation.metastate.eid_wallet"; | ||
| } |
There was a problem hiding this comment.
The else condition if no iPhone is all the same, so this can be massively simplified.
…type-Project/prototype into feat/ereputation-adapter
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
platforms/pictique/src/lib/fragments/Drawer/Drawer.svelte (1)
26-36: Avoid type assertion and verifyuseSwipeAPI usage.Two concerns:
- The type assertion
as anyon line 36 defeats TypeScript's type safety. This was flagged in a previous review.- The empty function
() => {}passed as the first argument touseSwipeappears unusual and should be verified against the library's documented API.Please verify the correct usage pattern for
useSwipefrom svelte-gestures:svelte-gestures useSwipe API documentationplatforms/eReputation-api/src/index.ts (1)
27-35: Critical race condition: server starts before database initialization completes.The issue flagged in the previous review remains unresolved. Lines 54-56 instantiate
JobQueueServiceandPollReputationWorkerbefore the database is initialized, and lines 120-122 start the server outside the initialization promise chain. This causes:
- Early HTTP requests hitting uninitialized database connections
- Workers attempting to use uninitialized repositories
- Runtime errors that are hard to diagnose
Move the server startup and worker initialization inside the
.then()block:+let jobQueueService: JobQueueService; +let pollReputationWorker: PollReputationWorker; + AppDataSource.initialize() .then(async () => { console.log("Database connection established"); console.log("Web3 adapter initialized"); + + // Initialize job queue and worker after DB is ready + jobQueueService = new JobQueueService(); + pollReputationWorker = new PollReputationWorker(jobQueueService); + + app.listen(port, () => { + console.log(`eReputation API server running on port ${port}`); + }); }) .catch((error: unknown) => { console.error("Error during initialization:", error); process.exit(1); }); - -// Initialize job queue and worker -const jobQueueService = new JobQueueService(); -const pollReputationWorker = new PollReputationWorker(jobQueueService); - -// Start server -app.listen(port, () => { - console.log(`eReputation API server running on port ${port}`); -});Also applies to: 54-56, 120-122
platforms/eReputation-api/src/middleware/auth.ts (1)
18-19: Add validation for empty token string.The issue from the previous review remains unresolved. After splitting the Authorization header at line 18, the code should validate that
tokenis not empty before callingverifyToken. A malformed header like"Bearer "would pass an empty string toverifyToken, causing a less informative error.Add validation:
const token = authHeader.split(" ")[1]; + +if (!token || !token.trim()) { + return res.status(401).json({ error: "Invalid token" }); +} + const decoded: AuthTokenPayload = verifyToken(token);platforms/eReputation-api/src/controllers/WebhookController.ts (1)
98-109: User creation path still missing email fields.The issue flagged in the previous review remains unresolved. Lines 101-109 update the newly created user but omit
emailVerifiedfields, while the update path (lines 77-87) includes them. This inconsistency means first-time webhook-created users lose email information.Add the missing fields:
await this.userService.updateUser(user.id, { name: req.body.data.displayName, handle: req.body.data.username, description: req.body.data.bio, avatarUrl: req.body.data.avatarUrl, bannerUrl: req.body.data.bannerUrl, isVerified: req.body.data.isVerified, isPrivate: req.body.data.isPrivate, + email: req.body.data.email, + emailVerified: req.body.data.emailVerified, });
🧹 Nitpick comments (8)
platforms/pictique/src/lib/fragments/Drawer/Drawer.svelte (1)
22-24: Consider reusing the existingdismiss()function.The
downSwipeHandlerduplicates the logic of thedismiss()function (lines 18-20). You can passdismissdirectly toonswipedowninstead of creating a new handler.Apply this diff to eliminate the duplication:
- function downSwipeHandler() { - drawer?.destroy({ animate: true }); - } - const swipeResult = useSwipe( () => {}, () => ({ timeframe: 300, minSwipeDistance: 60 }), { - onswipedown: downSwipeHandler + onswipedown: dismiss } );platforms/eReputation-api/src/services/GroupService.ts (1)
83-85: Useless catch-rethrow: consider removing or adding error handling.The catch block at line 83 rethrows the error without any transformation or logging. This pattern adds no value:
} catch (error) { throw error; }Either remove the try-catch entirely or add meaningful error handling (logging, error transformation, etc.).
platforms/eReputation-api/src/workers/PollReputationWorker.ts (2)
21-26: Consider dependency injection for service instances.The constructor directly instantiates five service classes without dependency injection. This creates tight coupling and makes the worker harder to test. Consider accepting these services as constructor parameters or using a service container pattern.
This also makes the worker vulnerable to the initialization race condition in
index.ts(services attempting to useAppDataSourcebefore it's initialized).
172-172: Dynamic import in hot path: consider static import.Line 172 uses a dynamic
import()inside the job processing flow. Unless there's a specific reason for lazy loading (e.g., circular dependency), a static import at the top of the file would be more performant and clearer:+import { adapter } from "../web3adapter/watchers/subscriber"; + export class PollReputationWorker { // ... private async transmitReputationResults(...) { // ... - const { adapter } = await import("../web3adapter/watchers/subscriber"); await adapter.handleChange({platforms/eReputation-api/src/services/JobQueueService.ts (4)
55-86: Remove useless try-catch block.The try-catch block (lines 55-86) simply rethrows the error without adding context, logging, or handling. This is an anti-pattern that adds no value.
Apply this diff to remove it:
- try { - // Use pollId as the job ID for deduplication (same poll = same job) - const jobId = `poll-reputation:${pollId}`; + // Use pollId as the job ID for deduplication (same poll = same job) + const jobId = `poll-reputation:${pollId}`; - // Check if job already exists or was recently processed - const existingJob = await this.queue.getJob(jobId); - if (existingJob) { - const state = await existingJob.getState(); - if (state === "active" || state === "waiting" || state === "delayed") { - // Job already queued, skip - return; - } + // Check if job already exists or was recently processed + const existingJob = await this.queue.getJob(jobId); + if (existingJob) { + const state = await existingJob.getState(); + if (state === "active" || state === "waiting" || state === "delayed") { + // Job already queued, skip + return; } + } - // Add job with deduplication - await this.queue.add( - "calculate-poll-reputation", - { - pollId, - groupId, - eventId, - }, - { - jobId, // Use pollId as job ID for deduplication - removeOnComplete: true, - removeOnFail: false, - } - ); - } catch (error) { - throw error; - } + // Add job with deduplication + await this.queue.add( + "calculate-poll-reputation", + { + pollId, + groupId, + eventId, + }, + { + jobId, // Use pollId as job ID for deduplication + removeOnComplete: true, + removeOnFail: false, + } + );</review_comment_end>
50-86: Add observability for job enqueue operations.The method lacks logging and observability, making it difficult to debug issues in production. Consider adding structured logging for:
- Successful job enqueues
- Skipped duplicates
- Failures
Example addition:
async enqueuePollReputationJob( pollId: string, groupId: string, eventId: string ): Promise<void> { const jobId = `poll-reputation:${pollId}`; const existingJob = await this.queue.getJob(jobId); if (existingJob) { const state = await existingJob.getState(); if (state === "active" || state === "waiting" || state === "delayed") { console.log(`Job already queued for pollId: ${pollId}, state: ${state}`); return; } } await this.queue.add( "calculate-poll-reputation", { pollId, groupId, eventId }, { jobId, removeOnComplete: true, removeOnFail: false } ); console.log(`Enqueued reputation job for pollId: ${pollId}, eventId: ${eventId}`); }</review_comment_end>
96-98: Consider whether exposing the queue breaks encapsulation.The
getQueue()method exposes the internal BullMQ queue instance, allowing external code to manipulate the queue directly and bypass the service's deduplication logic. If this is needed for worker initialization (as suggested by the PR context mentioningPollReputationWorker), consider documenting this use case and restricting access appropriately.If the queue is only needed for worker setup, consider:
- Making this method
protectedor internal-only- Providing a dedicated
createWorker()factory method instead- Documenting that direct queue manipulation bypasses service guarantees
</review_comment_end>
91-94: Add error handling for graceful shutdown.The
close()method doesn't handle errors during shutdown, which could leave connections hanging or prevent proper cleanup. Consider adding error handling and logging.async close(): Promise<void> { try { await this.queue.close(); await this.redis.quit(); } catch (error) { console.error('Error during JobQueueService shutdown:', error); throw error; } }Note: If you remove the standalone Redis client per the earlier comment, this method becomes simpler with just
await this.queue.close().
</review_comment_end>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
platforms/eReputation-api/package.json(1 hunks)platforms/eReputation-api/src/controllers/WebhookController.ts(1 hunks)platforms/eReputation-api/src/index.ts(1 hunks)platforms/eReputation-api/src/middleware/auth.ts(1 hunks)platforms/eReputation-api/src/services/GroupService.ts(1 hunks)platforms/eReputation-api/src/services/JobQueueService.ts(1 hunks)platforms/eReputation-api/src/services/VotingReputationService.ts(1 hunks)platforms/eReputation-api/src/utils/jwt.ts(1 hunks)platforms/eReputation-api/src/workers/PollReputationWorker.ts(1 hunks)platforms/eReputation-api/tsconfig.json(1 hunks)platforms/eReputation/client/src/lib/utils/mobile-detection.ts(1 hunks)platforms/pictique/src/lib/fragments/Drawer/Drawer.svelte(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- platforms/eReputation/client/src/lib/utils/mobile-detection.ts
- platforms/eReputation-api/package.json
🧰 Additional context used
🧬 Code graph analysis (6)
platforms/eReputation-api/src/index.ts (5)
platforms/eReputation-api/src/database/data-source.ts (1)
AppDataSource(29-29)platforms/eReputation-api/src/services/JobQueueService.ts (1)
JobQueueService(10-99)platforms/eReputation-api/src/workers/PollReputationWorker.ts (1)
PollReputationWorker(13-218)platforms/eReputation-api/src/controllers/WebhookController.ts (1)
WebhookController(14-322)platforms/eReputation-api/src/middleware/auth.ts (2)
authMiddleware(6-34)authGuard(36-41)
platforms/eReputation-api/src/workers/PollReputationWorker.ts (3)
platforms/eReputation-api/src/services/JobQueueService.ts (2)
PollReputationJobData(4-8)JobQueueService(10-99)platforms/eReputation-api/src/services/VotingReputationService.ts (1)
VotingReputationService(8-506)platforms/eReputation-api/src/services/GroupService.ts (1)
GroupService(6-78)
platforms/eReputation-api/src/services/VotingReputationService.ts (3)
platforms/eVoting/src/lib/pollApi.ts (1)
Group(65-75)platforms/evoting-api/src/database/data-source.ts (1)
AppDataSource(34-34)platforms/eReputation-api/src/database/entities/VoteReputationResult.ts (1)
MemberReputation(13-17)
platforms/eReputation-api/src/middleware/auth.ts (2)
platforms/eReputation-api/src/utils/jwt.ts (2)
AuthTokenPayload(10-12)verifyToken(18-29)platforms/eReputation-api/src/database/data-source.ts (1)
AppDataSource(29-29)
platforms/eReputation-api/src/services/GroupService.ts (1)
platforms/eReputation-api/src/database/data-source.ts (1)
AppDataSource(29-29)
platforms/eReputation-api/src/controllers/WebhookController.ts (7)
platforms/eReputation-api/src/services/UserService.ts (1)
UserService(5-66)platforms/eReputation-api/src/services/GroupService.ts (1)
GroupService(6-78)platforms/eReputation-api/src/services/VoteService.ts (1)
VoteService(7-83)platforms/eReputation-api/src/services/PollService.ts (1)
PollService(5-59)platforms/eReputation-api/src/services/JobQueueService.ts (1)
JobQueueService(10-99)platforms/eReputation-api/src/web3adapter/watchers/subscriber.ts (1)
adapter(16-21)platforms/eReputation-api/src/database/data-source.ts (1)
AppDataSource(29-29)
⏰ 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 (8)
platforms/eReputation-api/tsconfig.json (2)
1-24: AI summary inconsistency: baseUrl value mismatch.The AI summary states
"baseUrl": "./", but line 8 of the actual code correctly specifies"baseUrl": "./src". The actual code is correct and aligns with peer API services in the monorepository.
1-24: Configuration properly addresses prior feedback and aligns with established patterns.This TypeScript configuration correctly incorporates
"baseUrl": "./src"(line 8), which was flagged as missing in the previous review and is consistent with other API services in the project (blabsy-w3ds-auth-api, dreamsync-api, evoting-api, pictique-api). The overall configuration is well-suited for a Node.js backend service with proper support for decorators, strict type checking, and full TypeScript emission. No concerns identified.platforms/pictique/src/lib/fragments/Drawer/Drawer.svelte (1)
58-58: LGTM!The element binding correctly integrates
drawerElement,restProps, theswipedirective, and class merging.platforms/eReputation-api/src/services/GroupService.ts (1)
1-1: Past issue resolved: TypeORM 0.3.x compatibility.The code now correctly uses
Inoperator withfindByinstead of the deprecatedfindByIds. The fix aligns with TypeORM 0.3.x breaking changes.Also applies to: 39-39, 45-45
platforms/eReputation-api/src/utils/jwt.ts (1)
4-29: Past issue resolved: fail-fast JWT_SECRET validation and proper typing.The implementation now correctly:
- Fails fast with a clear error message if
JWT_SECRETis missing- Uses
AuthTokenPayloadinterface for type safety- Explicitly validates the decoded token's
userIdfieldThe security gap from the hard-coded fallback has been eliminated.
platforms/eReputation-api/src/controllers/WebhookController.ts (1)
265-275: Past issue resolved: expensive operations now deferred to job queue.The webhook now correctly enqueues reputation calculation jobs via
JobQueueServiceinstead of blocking on expensive operations. This allows the webhook to respond quickly and process heavy work asynchronously with proper retry/backoff policies.The implementation includes:
- Idempotent job enqueueing using
globalIdas event ID- Error handling that logs failures without breaking the webhook
- Deduplication at the queue level
Also applies to: 301-311
platforms/eReputation-api/src/services/VotingReputationService.ts (1)
224-240: Past issue resolved: explicit type validations for AI response parsing.The code now uses explicit type validations instead of truthiness checks:
typeof ename !== 'string' || ename.trim() === ''for string validationtypeof item.score !== 'number' || !Number.isFinite(item.score)for numeric validationtypeof item.justification !== 'string'for justification validationThis approach correctly handles edge cases and won't reject valid falsy values like
score === 0.platforms/eReputation-api/src/services/JobQueueService.ts (1)
28-42: Default job options look reasonable.The retry strategy (3 attempts with exponential backoff) and retention policies are appropriate for background job processing. However, note that lines 79-80 override
removeOnCompleteinconsistently—see separate comment.</review_comment_end>
| export interface PollReputationJobData { | ||
| pollId: string; | ||
| eventId: string; // For idempotency | ||
| groupId: string; | ||
| } |
There was a problem hiding this comment.
Misleading comment: eventId not used for deduplication.
The comment states eventId is "For idempotency," but the actual deduplication logic (line 57) uses pollId as the unique job identifier. The eventId is stored in the job data but doesn't contribute to preventing duplicate jobs. Either update the comment to clarify eventId's actual purpose or implement eventId-based idempotency if that was the intent.
</review_comment_end>
🤖 Prompt for AI Agents
In platforms/eReputation-api/src/services/JobQueueService.ts lines 4-8, the
comment "For idempotency" on eventId is misleading because deduplication uses
pollId as the job id; either update the comment to state that eventId is only
stored for traceability/metadata while pollId is used for deduplication, or
change the enqueue logic to derive the BullMQ jobId from eventId (or a composite
like `${pollId}:${eventId}`) so eventId actually provides idempotency; implement
the chosen approach and ensure tests/usage reflect the new behavior.
| constructor() { | ||
| // Create Redis connection | ||
| this.redis = new Redis({ | ||
| host: process.env.REDIS_HOST || "localhost", | ||
| port: parseInt(process.env.REDIS_PORT || "6379", 10), | ||
| maxRetriesPerRequest: null, | ||
| }); |
There was a problem hiding this comment.
Remove unnecessary standalone Redis client.
The standalone Redis client (this.redis) is created but never used except in the close() method. BullMQ manages its own Redis connection internally (configured in lines 24-27), so this separate client wastes resources by maintaining a duplicate connection.
Apply this diff to remove the redundant connection:
- private redis: Redis;
-
constructor() {
- // Create Redis connection
- this.redis = new Redis({
- host: process.env.REDIS_HOST || "localhost",
- port: parseInt(process.env.REDIS_PORT || "6379", 10),
- maxRetriesPerRequest: null,
- });
-
// Create BullMQ queueAnd update the close method:
async close(): Promise<void> {
await this.queue.close();
- await this.redis.quit();
}</review_comment_end>
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In platforms/eReputation-api/src/services/JobQueueService.ts around lines 14 to
20, remove the redundant standalone Redis client creation (this.redis = new
Redis(...)) since BullMQ manages its own Redis connection; delete the unused
import/field and any associated options (host/port/maxRetriesPerRequest) used
only for that client. Then update the close() method to stop referencing/closing
this.redis and instead only call the appropriate BullMQ queue/worker/connection
close methods (or await queue.close()/worker.close()/connection.disconnect() as
applicable) so no orphan Redis client is left or referenced.
| this.redis = new Redis({ | ||
| host: process.env.REDIS_HOST || "localhost", | ||
| port: parseInt(process.env.REDIS_PORT || "6379", 10), | ||
| maxRetriesPerRequest: null, | ||
| }); |
There was a problem hiding this comment.
Add error handling, validation, and timeout configuration for Redis.
The Redis client initialization has several issues:
- No error handling for connection failures (will crash the service)
parseIntcan returnNaNifREDIS_PORTcontains invalid data- Missing
connectTimeoutandcommandTimeoutcan cause operations to hang indefinitely - No validation that required environment variables are present
Consider adding validation and resilience:
constructor() {
const redisHost = process.env.REDIS_HOST || "localhost";
const redisPort = parseInt(process.env.REDIS_PORT || "6379", 10);
if (isNaN(redisPort) || redisPort < 1 || redisPort > 65535) {
throw new Error(`Invalid REDIS_PORT: ${process.env.REDIS_PORT}`);
}
// Create Redis connection with timeout and retry config
this.redis = new Redis({
host: redisHost,
port: redisPort,
maxRetriesPerRequest: null,
connectTimeout: 10000,
commandTimeout: 5000,
retryStrategy: (times) => {
const delay = Math.min(times * 50, 2000);
return delay;
},
});
this.redis.on('error', (err) => {
console.error('Redis connection error:', err);
});
// ... rest of constructor
}</review_comment_end>
🤖 Prompt for AI Agents
In platforms/eReputation-api/src/services/JobQueueService.ts around lines 16 to
20, the Redis client is created without validation, timeouts, or error handling;
update the constructor to validate REDIS_HOST and REDIS_PORT (ensure REDIS_PORT
parses to an integer within 1–65535 and throw a clear error if invalid or if a
required host is missing), use the parsed safe port and host when instantiating
Redis, configure connectTimeout and commandTimeout to sensible values, add a
retryStrategy to avoid infinite reconnection loops, and attach an error listener
to log and surface connection errors so the service fails fast or degrades
gracefully rather than hanging or crashing silently.
| removeOnComplete: { | ||
| age: 3600, // Keep completed jobs for 1 hour | ||
| count: 1000, |
There was a problem hiding this comment.
Inconsistent job retention configuration.
The default removeOnComplete configuration (lines 34-36) keeps completed jobs for 1 hour for observability, but line 79 overrides this with removeOnComplete: true, causing immediate removal. This defeats the purpose of the retention policy and makes debugging production issues difficult.
Either remove the per-job override to respect the defaults:
{
jobId, // Use pollId as job ID for deduplication
- removeOnComplete: true,
removeOnFail: false,
}Or adjust the defaults if immediate removal is desired:
removeOnComplete: {
- age: 3600, // Keep completed jobs for 1 hour
- count: 1000,
+ count: 10, // Keep only recent jobs
},</review_comment_end>
Also applies to: 79-81
| // Check if job already exists or was recently processed | ||
| const existingJob = await this.queue.getJob(jobId); | ||
| if (existingJob) { | ||
| const state = await existingJob.getState(); | ||
| if (state === "active" || state === "waiting" || state === "delayed") { | ||
| // Job already queued, skip | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Deduplication logic has a race condition.
The deduplication check only prevents adding jobs in "active", "waiting", or "delayed" states. However, since removeOnComplete: true (line 79) immediately removes completed jobs, there's a race window where the same poll could be enqueued multiple times in rapid succession—once a job completes and is removed, nothing prevents immediate re-enqueueing.
Consider either:
- Checking "completed" state as well, or
- Implementing external deduplication tracking (e.g., Redis SET with TTL), or
- Respecting the default
removeOnCompleteretention (see previous comment) to keep jobs during a deduplication window
</review_comment_end>
🤖 Prompt for AI Agents
In platforms/eReputation-api/src/services/JobQueueService.ts around lines 59-67
(and note removeOnComplete at ~line 79 and the try-catch at ~lines 83-85): the
current dedup check only skips jobs in active/waiting/delayed but because
completed jobs are removed immediately there is a race that allows duplicates.
Fix by either (A) including "completed" in the state check and/or changing
removeOnComplete to retain completed jobs for a short deduplication window
(e.g., set removeOnComplete to a retention object or false), or (B) implement
external deduplication using a Redis SET with a TTL keyed by jobId (set before
enqueue and expire after the dedup window) to atomically prevent re-enqueue;
also remove the useless try-catch around lines 83-85 (or replace it with proper
error handling/logging) since it currently only rethrows. Ensure whichever
approach you pick is atomic and covers the short time window after job
completion.
| import { cn } from '$lib/utils'; | ||
| import { swipe } from 'svelte-gestures'; | ||
| import { useSwipe } from 'svelte-gestures'; | ||
| import type { SwipeCustomEvent } from 'svelte-gestures'; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify if SwipeCustomEvent import is still needed.
The import of SwipeCustomEvent appears unused after refactoring to use the onswipedown callback pattern instead of manual event handling.
Run the following script to confirm if this import is referenced elsewhere:
🏁 Script executed:
#!/bin/bash
# Check if SwipeCustomEvent is used in the file
rg -n "SwipeCustomEvent" "platforms/pictique/src/lib/fragments/Drawer/Drawer.svelte"Length of output: 59
Remove unused SwipeCustomEvent import at line 7.
The SwipeCustomEvent type is imported but never used in the file. After the refactor to use the useSwipe callback pattern, this import is no longer needed and should be removed.
- import type { SwipeCustomEvent } from 'svelte-gestures';
🤖 Prompt for AI Agents
In platforms/pictique/src/lib/fragments/Drawer/Drawer.svelte around line 7,
remove the unused import "SwipeCustomEvent" from 'svelte-gestures' because it is
no longer referenced after the refactor; simply delete the import specifier so
the file no longer imports that type.
Description of change
Add eReputation adapter
Issue Number
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.