Fix/ereputation signing modal #457
Conversation
WalkthroughThis PR introduces a database migration to create a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Modal as Reference Modal
participant Backend as Backend API
participant SSE as SSE Endpoint
User->>Modal: Submit Reference
Modal->>Backend: POST /api/references
Backend-->>Modal: Success + signingSession
rect rgb(200, 220, 255)
Note over Modal,SSE: Signing Flow Initiated
Modal->>SSE: EventSource (signingSession)
Modal->>Modal: Start 15-min Countdown
Modal->>User: Show QR Code / Deep Link
end
User->>User: Sign on Device
Backend->>SSE: Emit 'signed' status
SSE-->>Modal: Status update
rect rgb(200, 255, 220)
Note over Modal: Signing Complete
Modal->>Modal: Invalidate Queries
Modal->>User: Show Success
Modal->>Modal: Cleanup SSE + Reset
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
platforms/eReputation-api/src/database/migrations/1763710576026-migration.ts (1)
7-7: Consider adding indexes on foreign key columns.The
referenceIdanduserIdcolumns will likely be queried frequently (e.g., fetching all signatures for a reference, or all signatures by a user), but no indexes are defined. Without indexes, these queries will require full table scans as the table grows.Apply this diff to add indexes:
public async up(queryRunner: QueryRunner): Promise<void> { await queryRunner.query(`CREATE TABLE "reference_signatures" ("id" uuid NOT NULL DEFAULT uuid_generate_v4(), "referenceId" uuid NOT NULL, "userId" uuid NOT NULL, "referenceHash" text NOT NULL, "signature" text NOT NULL, "publicKey" text NOT NULL, "message" text NOT NULL, "createdAt" TIMESTAMP NOT NULL DEFAULT now(), "updatedAt" TIMESTAMP NOT NULL DEFAULT now(), CONSTRAINT "PK_e13239bee10fe5f7a990d5fbee4" PRIMARY KEY ("id"))`); + await queryRunner.query(`CREATE INDEX "IDX_reference_signatures_referenceId" ON "reference_signatures" ("referenceId")`); + await queryRunner.query(`CREATE INDEX "IDX_reference_signatures_userId" ON "reference_signatures" ("userId")`); await queryRunner.query(`ALTER TABLE "reference_signatures" ADD CONSTRAINT "FK_63d30e071b1bde272650ddd4c50" FOREIGN KEY ("referenceId") REFERENCES "references"("id") ON DELETE NO ACTION ON UPDATE NO ACTION`); await queryRunner.query(`ALTER TABLE "reference_signatures" ADD CONSTRAINT "FK_273a89a0507f9304a2aa1839b9d" FOREIGN KEY ("userId") REFERENCES "users"("id") ON DELETE NO ACTION ON UPDATE NO ACTION`); }And update the
downmethod:public async down(queryRunner: QueryRunner): Promise<void> { await queryRunner.query(`ALTER TABLE "reference_signatures" DROP CONSTRAINT "FK_273a89a0507f9304a2aa1839b9d"`); await queryRunner.query(`ALTER TABLE "reference_signatures" DROP CONSTRAINT "FK_63d30e071b1bde272650ddd4c50"`); + await queryRunner.query(`DROP INDEX "IDX_reference_signatures_userId"`); + await queryRunner.query(`DROP INDEX "IDX_reference_signatures_referenceId"`); await queryRunner.query(`DROP TABLE "reference_signatures"`); }platforms/eReputation/client/src/components/modals/reference-modal.tsx (3)
67-71: Signing state, timer, and cleanup are sound; consider simplifying the countdown logicThe signing state model and cleanup paths look correct:
eventSourceis consistently closed on modal close, unmount, timer expiry, reset, and “Try Again”, which should prevent leaks.Two small refinements you might consider:
Reduce timer churn (Lines 211-229): because
timeRemainingis in the dependency array, the effect tears down and re‑creates the interval every second. This is safe but a bit noisy. You could instead drive the effect offsigningStatus,signingSession, and maybeeventSource, and rely on the functionalsetTimeRemainingupdater, e.g.:useEffect(() => {
if (signingStatus === "pending" && timeRemaining > 0 && signingSession) {
useEffect(() => {
if (signingStatus === "pending" && signingSession) {
const timer = setInterval(() => {
setTimeRemaining(prev => {
if (prev <= 1) {
setSigningStatus("expired");
if (eventSource) {
eventSource.close();
}
return 0;
}
return prev - 1;
});
}, 1000);return () => clearInterval(timer);
}
- }, [signingStatus, timeRemaining, signingSession, eventSource]);
- }, [signingStatus, signingSession, eventSource]);
- **Handle already‑expired sessions more explicitly** (Lines 109-113, 211-221): if `expiresAt` is in the past when the response arrives, `secondsRemaining` becomes `<= 0`, you set `timeRemaining(0)` but leave `signingStatus` as `"pending"`. You might want to treat that as an immediate expiry instead: ```diff const secondsRemaining = Math.floor((expiresAt.getTime() - now.getTime()) / 1000); - setTimeRemaining(Math.max(0, secondsRemaining)); - startSSEConnection(data.signingSession.sessionId); + const clamped = Math.max(0, secondsRemaining); + setTimeRemaining(clamped); + if (clamped > 0) { + startSSEConnection(data.signingSession.sessionId); + } else { + setSigningStatus("expired"); + }These are refinements only; current behavior is functionally acceptable.
Also applies to: 211-252, 259-271
142-209: Close the SSE connection and surface feedback ononerrorThe happy-path and terminal-status handling in
startSSEConnectionare solid, but theonerrorbranch leaves the EventSource open and silent from the user’s perspective:newEventSource.onerror = (error) => { console.error("SSE connection error:", error); setSigningStatus("error"); };Two small tweaks will make this more robust:
Close the connection on error to avoid repeated reconnect attempts or orphaned sockets:
newEventSource.onerror = (error) => { console.error("SSE connection error:", error);setSigningStatus("error");
- setSigningStatus("error");
- newEventSource.close();
};- **Optionally show a toast** so the user understands why the signing stalled: ```diff newEventSource.onerror = (error) => { console.error("SSE connection error:", error); setSigningStatus("error"); + toast({ + title: "Signing Connection Error", + description: "We lost connection to the signing service. Please try again.", + variant: "destructive", + }); + newEventSource.close(); };This aligns the error path with the “expired” and “security_violation” cases and keeps the EventSource lifecycle clean.
354-445: Clarify “Try Again” semantics to avoid silent duplicate references and improve error UXThe signing UI is well structured, and the conditional footers behave consistently with
signingSessionpresence. One subtle behavioral point:
- When a signing attempt expires, hits a security violation, or hits error, “Try Again” (Lines 429-439) clears
signingSessionand closes the SSE. The user returns to the pre-filled form and can click “Sign & Submit eReference” again.- That second click will re-run
handleSubmit, which (as currently written) posts to/api/referencesagain and will likely create a new reference, leaving the first, unsigned reference around unless the backend handles it.If that’s intentional (each attempt is a new reference), it may be worth making it explicit in copy or docs. If not, you may want to:
- Introduce a separate “recreate signing session” endpoint keyed by the existing reference id, and have “Try Again” call that instead of going back to
handleSubmit, or- Track the created reference id in state and reuse it, only regenerating the signing session.
Additionally, for
signingStatus === "error"you currently show only the “Try Again” button with no message. Consider adding a small inline message similar to the “expired” and “security_violation” blocks—for example, “We couldn’t connect to the signing service. Please try again.”Overall the flow is coherent; these tweaks would just reduce confusion and possible dangling references.
Also applies to: 429-444, 581-613, 615-632
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
platforms/eReputation-api/src/database/migrations/1763710576026-migration.ts(1 hunks)platforms/eReputation/client/src/components/modals/reference-modal.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
platforms/eReputation-api/src/database/migrations/1763710576026-migration.ts (2)
platforms/eReputation-api/src/database/migrations/1761324658339-migration.ts (1)
Migration1761324658339(3-20)platforms/group-charter-manager-api/src/database/migrations/1755598750354-migration.ts (1)
Migration1755598750354(3-18)
platforms/eReputation/client/src/components/modals/reference-modal.tsx (1)
platforms/eReputation/client/src/lib/queryClient.ts (1)
queryClient(44-57)
⏰ 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 (2)
platforms/eReputation-api/src/database/migrations/1763710576026-migration.ts (2)
12-16: Rollback logic is correct.The
downmethod properly drops foreign key constraints before dropping the table, ensuring a clean rollback.Note: If you add the recommended indexes, remember to also drop them in this method (as shown in the previous comment).
8-9: I need to examine the complete ReferenceSignature entity definition and check for any duplicate prevention logic. Let me gather more information:Let me examine the complete ReferenceSignature entity and related code:
Based on the script output and web research, I now have sufficient information to provide a comprehensive rewritten review comment. The evidence confirms both concerns are valid:
- ON DELETE NO ACTION pattern: The codebase consistently uses CASCADE for child entities, making NO ACTION an outlier
- Missing unique constraint: No unique constraint found on the composite key, and no application-level duplicate checks in the service
Verify deletion behavior and add missing unique constraint on signatures.
Two data integrity issues in the foreign key definitions:
ON DELETE NO ACTION inconsistency: When an author is deleted, the book is also deleted with ON DELETE CASCADE. The migration uses ON DELETE NO ACTION, which will reject the operation if related records exist. The codebase pattern shows CASCADE is used throughout (wishlists, votes, followers, group members). Confirm: should signatures be deleted with CASCADE when a reference/user is deleted, or is orphaning intentional for audit purposes?
No unique constraint on (referenceId, userId): The table allows duplicate signatures for the same user-reference pair. TypeORM supports multi-column unique constraints with @unique(["referenceId", "userId"]) in the entity, but the migration contains no such constraint. Add either a database constraint or document if application-level validation prevents duplicates.
platforms/eReputation-api/src/database/migrations/1763710576026-migration.ts
Show resolved
Hide resolved
Description of change
force signing modal to display on eReputation.
Issue Number
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.