Elsa/260515 email#121
Open
eltouma wants to merge 55 commits into
Open
Conversation
* move admin repository code out of category_repository * add GetNotifiableAdmins for email notification recipients
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/sql/014_add_email_queue.up.sql (1)
19-19: ⚡ Quick winAdd a queue-polling index matching the worker query pattern.
Line 19 creates
idx_email_queue_status, but the poller filters by status and orders bycreated_at; that can still force sorting under load. Add a composite index like(status, created_at)to reduce polling latency as the table grows.♻️ Suggested migration tweak
-CREATE INDEX idx_email_queue_status ON email_queue(status); +CREATE INDEX idx_email_queue_status_created_at ON email_queue(status, created_at);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sql/014_add_email_queue.up.sql` at line 19, The current index idx_email_queue_status only covers filtering by status, but the email queue poller also orders results by created_at, which forces a sorting operation under load. Replace the single-column index on status with a composite index on (status, created_at) to allow the database to satisfy both the filter and sort operations efficiently in a single index scan, improving polling performance as the table grows.src/moderation_handler.go (1)
196-199: ⚡ Quick winConsider skipping notification when owner lookup fails.
If
GetOwnerOrcidfails,uploaderOrcidbecomes empty and the notification is enqueued with no recipient. The worker will then fail to resolve the email. While the failure is graceful (logged and retried until marked failed), consider logging the lookup error and skipping notification creation when the owner can't be determined.uploaderOrcid, err := h.recordRepo.GetOwnerOrcid(ctx, id) if err != nil { - uploaderOrcid = "" + errorLogger.Printf("moderation handler: failed to get owner orcid for record %s: %v", id, err) }Then guard the notification calls:
+if uploaderOrcid != "" { if err := h.notificationService.CreateForRecordModeration(ctx, id, uploaderOrcid, StatusApproved); err != nil { errorLogger.Printf("moderation handler: case approved: %v", err) } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/moderation_handler.go` around lines 196 - 199, The issue is that when GetOwnerOrcid fails, the error is silently ignored and uploaderOrcid is set to an empty string, causing a notification to be enqueued with no recipient that will fail downstream. To fix this, log the actual error returned by GetOwnerOrcid when it fails, and then add a guard condition to skip notification creation entirely when the owner cannot be determined. Instead of setting uploaderOrcid to an empty string on error, check the error explicitly and return early or skip the notification-related code blocks when the lookup fails.src/orcid_service.go (1)
38-38: 💤 Low valueRemove commented-out dead code.
This leftover debug line adds noise. Delete it to keep the codebase clean.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/orcid_service.go` at line 38, Remove the commented-out dead code line containing the http.DefaultClient.Do(req) call at line 38. Delete the entire commented line that starts with //res, err := http.DefaultClient.Do(req) to eliminate code noise and keep the codebase clean.src/build.sh (1)
13-14: ⚡ Quick winAvoid double dependency installation in the container build path.
Line 13 re-runs dependency resolution even though
Dockerfilealready installs packages before calling this script, which adds avoidable build time and network churn. Keep one deterministic install step only.Proposed simplification
-# create dist/index.js -yarn install --immutable -yarn exec esbuild \ +# create dist/index.js +yarn exec esbuild \And make Docker deterministic at install point:
-RUN yarn install +RUN yarn install --immutable🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/build.sh` around lines 13 - 14, Remove the redundant yarn install --immutable command from the build script since package installation is already performed in the Dockerfile before this script is invoked. Keep the yarn exec esbuild command and all other build steps, but eliminate the duplicate dependency resolution step to reduce unnecessary build time and network requests. Ensure the Dockerfile remains the single deterministic point where yarn install runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/admin_repository.go`:
- Around line 39-40: The context parameter is not being propagated to SQL
queries, preventing proper cancellation and deadline handling. In
src/admin_repository.go at lines 39-40, replace the r.db.Query(...) call in the
GetAllAdmins method with r.db.QueryContext(ctx, ...). Similarly, in
src/comment.go at lines 289-290, replace the r.db.Query(...) call with
r.db.QueryContext(ctx, ...). Both changes involve passing the ctx parameter as
the first argument to ensure the context is properly used for query execution.
In `@src/email_sender.go`:
- Line 64: The smtp.SendMail operation on line 64 lacks network timeouts, which
can cause indefinite blocking on slow or broken networks. Replace the direct
smtp.SendMail call with a context-based timeout approach by creating a context
with a reasonable deadline (following the pattern used elsewhere in the codebase
like ror_name_cache.go) and use smtp.SendMailWithContext instead, or wrap the
operation with explicit dial/read/write timeouts using a custom dialer to
prevent hung connections from blocking worker threads.
In `@src/notification_service.go`:
- Around line 86-101: The enqueueEmail function accepts an empty recipientOrcid
parameter without validation, which allows unsendable queue items to be
persisted and cause unnecessary retry churn. Add an early validation check in
the enqueueEmail function (after the existing emailQueueRepo nil check) to
reject empty or whitespace-only recipientOrcid values, returning an appropriate
error message similar to the existing nil check pattern.
In `@src/record_repository.go`:
- Around line 281-283: The comment on the line before the moderationStatus
assignment in the record_repository.go file is misleading because it mentions
"based on configuration" when the code actually hardcodes StatusPendingReview
without any configuration lookup. Replace the comment to accurately describe the
actual behavior by clarifying that all new records require explicit approval
before publication, as shown in the review suggestion.
In `@src/templates/browse.html`:
- Line 5: Fix the grammar errors in the privacy notice text on line 5 of the
browse.html template. Change "informations" to "information" (information is an
uncountable noun and should not be pluralized) and change "we does not store" to
"we do not store" (correct subject-verb agreement for the plural subject "we").
These corrections should be made in the welcome banner that starts with "Welcome
to the template sharing site".
---
Nitpick comments:
In `@src/build.sh`:
- Around line 13-14: Remove the redundant yarn install --immutable command from
the build script since package installation is already performed in the
Dockerfile before this script is invoked. Keep the yarn exec esbuild command and
all other build steps, but eliminate the duplicate dependency resolution step to
reduce unnecessary build time and network requests. Ensure the Dockerfile
remains the single deterministic point where yarn install runs.
In `@src/moderation_handler.go`:
- Around line 196-199: The issue is that when GetOwnerOrcid fails, the error is
silently ignored and uploaderOrcid is set to an empty string, causing a
notification to be enqueued with no recipient that will fail downstream. To fix
this, log the actual error returned by GetOwnerOrcid when it fails, and then add
a guard condition to skip notification creation entirely when the owner cannot
be determined. Instead of setting uploaderOrcid to an empty string on error,
check the error explicitly and return early or skip the notification-related
code blocks when the lookup fails.
In `@src/orcid_service.go`:
- Line 38: Remove the commented-out dead code line containing the
http.DefaultClient.Do(req) call at line 38. Delete the entire commented line
that starts with //res, err := http.DefaultClient.Do(req) to eliminate code
noise and keep the codebase clean.
In `@src/sql/014_add_email_queue.up.sql`:
- Line 19: The current index idx_email_queue_status only covers filtering by
status, but the email queue poller also orders results by created_at, which
forces a sorting operation under load. Replace the single-column index on status
with a composite index on (status, created_at) to allow the database to satisfy
both the filter and sort operations efficiently in a single index scan,
improving polling performance as the table grows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 32379678-39a2-478f-b573-799aaf9e6787
📒 Files selected for processing (23)
Dockerfilesrc/admin_repository.gosrc/build.shsrc/category_repository.gosrc/comment.gosrc/comment_handler.gosrc/email.gosrc/email_model.gosrc/email_queue_repository.gosrc/email_sender.gosrc/email_worker.gosrc/main.gosrc/model.gosrc/moderation.gosrc/moderation_handler.gosrc/notification_service.gosrc/oidc.gosrc/orcid_service.gosrc/record_handler.gosrc/record_repository.gosrc/sql/014_add_email_queue.up.sqlsrc/sql/015_add_email_queue.down.sqlsrc/templates/browse.html
💤 Files with no reviewable changes (2)
- src/email.go
- src/moderation.go
* use context in GetAllAdmins() and GetAllOrcids() * fix typo
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/moderation_handler.go (1)
196-219:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
GetOwnerOrciderror is not checked before usinguploaderOrcid.Line 196 calls
h.recordRepo.GetOwnerOrcid(ctx, id)and stores the error, but it's never checked. If the lookup fails,uploaderOrcidwill be empty, and the subsequentCreateForRecordModerationcalls (lines 207, 217) will fail with "recipient ORCID is empty". While this won't cause a panic (the validation inenqueueEmailcatches it), it creates unnecessary noise and the notification silently fails.🐛 Proposed fix
var newStatus ModerationStatus uploaderOrcid, err := h.recordRepo.GetOwnerOrcid(ctx, id) + if err != nil { + errorLogger.Printf("moderation handler: failed to get owner ORCID for record %s: %v", id, err) + // Continue with moderation but notifications will be skipped + } switch req.Action { case "approve": newStatus = StatusApproved // Check if there's a pending version to approve if err := h.moderationRepo.ApprovePendingVersion(ctx, id); err != nil { http.Error(w, "Error approving record/version", http.StatusInternalServerError) return } - if err := h.notificationService.CreateForRecordModeration(ctx, id, uploaderOrcid, StatusApproved); err != nil { - errorLogger.Printf("moderation handler: case approved: %v", err) + if uploaderOrcid != "" { + if err := h.notificationService.CreateForRecordModeration(ctx, id, uploaderOrcid, StatusApproved); err != nil { + errorLogger.Printf("moderation handler: case approved: %v", err) + } } case "reject": newStatus = StatusRejected // Check if there's a pending version to reject if err := h.moderationRepo.RejectPendingVersion(ctx, id); err != nil { http.Error(w, "Error rejecting record/version", http.StatusInternalServerError) return } - if err := h.notificationService.CreateForRecordModeration(ctx, id, uploaderOrcid, StatusRejected); err != nil { - errorLogger.Printf("moderation handler: case rejected: %v", err) + if uploaderOrcid != "" { + if err := h.notificationService.CreateForRecordModeration(ctx, id, uploaderOrcid, StatusRejected); err != nil { + errorLogger.Printf("moderation handler: case rejected: %v", err) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/moderation_handler.go` around lines 196 - 219, The error returned by h.recordRepo.GetOwnerOrcid(ctx, id) on line 196 is not checked before using uploaderOrcid, which means if the lookup fails, uploaderOrcid will be empty and cause the subsequent h.notificationService.CreateForRecordModeration calls in the approve and reject cases to fail silently. Add an error check immediately after the GetOwnerOrcid call, and if err is not nil, respond with an appropriate HTTP error and return early to prevent empty uploaderOrcid from being passed to the notification service.src/email_sender.go (1)
24-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on missing SMTP configuration at startup.
Line 24–Line 31 accepts empty SMTP settings, which defers failures to delivery time and can create noisy retry cycles.
Proposed fix
func NewEmailSender() *EmailSender { - return &EmailSender{ + s := &EmailSender{ smtpHost: os.Getenv("SMTP_HOST"), smtpPort: os.Getenv("SMTP_PORT"), smtpFromAddress: os.Getenv("SMTP_FROM_ADDRESS"), smtpUsername: os.Getenv("SMTP_USERNAME"), smtpPassword: os.Getenv("SMTP_PASSWORD"), } + if s.smtpHost == "" || s.smtpPort == "" || s.smtpFromAddress == "" || s.smtpUsername == "" || s.smtpPassword == "" { + return nil + } + return s }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/email_sender.go` around lines 24 - 31, The NewEmailSender function initializes the EmailSender struct with environment variables without validating that they are not empty, causing configuration errors to surface only during email delivery. Add validation in the NewEmailSender function to check that all required SMTP configuration variables (smtpHost, smtpPort, smtpFromAddress, smtpUsername, and smtpPassword) are not empty strings. If any required configuration is missing, log an appropriate error message and either panic or call os.Exit to fail fast at application startup before returning the EmailSender instance.
🧹 Nitpick comments (4)
src/notification_service.go (1)
11-15: 💤 Low value
commentRepofield is stored but never used.The
NotificationServicestorescommentRepobut no method in this file references it. If this is for future use, consider adding a comment. Otherwise, remove it to reduce unnecessary dependencies.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/notification_service.go` around lines 11 - 15, The commentRepo field in the NotificationService struct is stored but never used by any method in the file. Either remove the commentRepo field from the NotificationService struct to eliminate the unnecessary dependency, or if it is intended for future use, add a comment above the field explaining its intended purpose.src/comment.go (1)
280-287: 💤 Low value
GetCommentatorOrciddoesn't distinguish "comment not found" from other errors.Unlike
GetOwnerOrcidwhich mapssql.ErrNoRowstoErrRecordNotFound, this method returns rawsql.ErrNoRowswhen the comment doesn't exist. For consistency and clearer error handling by callers, consider mapping to a domain-specific error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/comment.go` around lines 280 - 287, In the GetCommentatorOrcid method, modify the error handling to check if the returned error equals sql.ErrNoRows and map it to a domain-specific error like ErrRecordNotFound for consistency with how GetOwnerOrcid handles the same scenario. If the error is sql.ErrNoRows, return the domain-specific error; otherwise, return the original error. This ensures callers can reliably distinguish between "comment not found" and other database errors.src/admin_repository.go (1)
9-14: 💤 Low value
GetAllAdmins.The
Adminstruct declares anGetAllAdminsquery only selectsorcid, created_at, modified_at. TheOrcidService.GetEmail), this field appears intentionally unused. Consider removing it to avoid confusion, or add a clarifying comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/admin_repository.go` around lines 9 - 14, The Email field in the Admin struct is declared but never populated by the GetAllAdmins query, which only selects orcid, created_at, and modified_at. Since email resolution happens dynamically via OrcidService.GetEmail at notification send time, the Email field in the Admin struct is unused and creates confusion. Either remove the Email field entirely from the Admin struct to keep it clean, or add a clarifying comment above the field explaining why it exists but is intentionally not populated by the query.src/email_worker.go (1)
9-20: ⚡ Quick winDepend on
OrcidResolverinstead of concrete*OrcidServicein the worker.Line 12 and Line 15 unnecessarily couple
EmailWorkerto one implementation even though an interface already exists.Proposed refactor
type EmailWorker struct { emailQueueRepo EmailQueueRepository emailSender *EmailSender - orcidService *OrcidService + orcidService OrcidResolver } -func NewEmailWorker(emailQueueRepo EmailQueueRepository, emailSender *EmailSender, orcidService *OrcidService) *EmailWorker { +func NewEmailWorker(emailQueueRepo EmailQueueRepository, emailSender *EmailSender, orcidService OrcidResolver) *EmailWorker { return &EmailWorker{ emailQueueRepo: emailQueueRepo, emailSender: emailSender, orcidService: orcidService, } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/email_worker.go` around lines 9 - 20, The EmailWorker struct and its NewEmailWorker constructor are tightly coupled to the concrete OrcidService type when an OrcidResolver interface already exists for this purpose. Replace the orcidService field in the EmailWorker struct (line 12) from the concrete type *OrcidService to the OrcidResolver interface type, and update the corresponding parameter in the NewEmailWorker constructor function (line 15) to also use the OrcidResolver interface instead of *OrcidService. This follows the Dependency Inversion Principle and allows the worker to depend on abstractions rather than concrete implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/email_sender.go`:
- Around line 126-132: The error handling for client.Quit() treats SMTP session
termination failures as delivery failures, but once writer.Close() succeeds, the
message is already accepted by the server. Modify the code so that after
writer.Close() succeeds at line 126, any error from client.Quit() at line 130 is
logged as a warning but does not cause the function to return an error. The
successful completion should be signaled to the caller only if the data writer
closes successfully, regardless of whether the subsequent Quit operation fails.
In `@src/main.go`:
- Around line 75-76: The constant EMAIL_NOTIF_INTERVAL_SEC is defined as 20
seconds, but there is a comment at line 496 that incorrectly states the interval
is "every minute". Update the comment near line 496 to accurately reflect that
the notification interval is 20 seconds instead of one minute, ensuring the
documentation matches the actual constant value defined for
EMAIL_NOTIF_INTERVAL_SEC.
In `@src/sql/015_add_email_queue.down.sql`:
- Around line 1-2: The down migration file is incorrectly numbered as 015 while
the corresponding up migration is numbered 014, which will cause golang-migrate
to fail recognizing them as a paired migration. Rename the file from
015_add_email_queue.down.sql to 014_add_email_queue.down.sql to match the
numeric prefix of the up migration (014_add_email_queue.up.sql) so that the
migration runner can properly pair them for rollback operations.
---
Duplicate comments:
In `@src/email_sender.go`:
- Around line 24-31: The NewEmailSender function initializes the EmailSender
struct with environment variables without validating that they are not empty,
causing configuration errors to surface only during email delivery. Add
validation in the NewEmailSender function to check that all required SMTP
configuration variables (smtpHost, smtpPort, smtpFromAddress, smtpUsername, and
smtpPassword) are not empty strings. If any required configuration is missing,
log an appropriate error message and either panic or call os.Exit to fail fast
at application startup before returning the EmailSender instance.
In `@src/moderation_handler.go`:
- Around line 196-219: The error returned by h.recordRepo.GetOwnerOrcid(ctx, id)
on line 196 is not checked before using uploaderOrcid, which means if the lookup
fails, uploaderOrcid will be empty and cause the subsequent
h.notificationService.CreateForRecordModeration calls in the approve and reject
cases to fail silently. Add an error check immediately after the GetOwnerOrcid
call, and if err is not nil, respond with an appropriate HTTP error and return
early to prevent empty uploaderOrcid from being passed to the notification
service.
---
Nitpick comments:
In `@src/admin_repository.go`:
- Around line 9-14: The Email field in the Admin struct is declared but never
populated by the GetAllAdmins query, which only selects orcid, created_at, and
modified_at. Since email resolution happens dynamically via
OrcidService.GetEmail at notification send time, the Email field in the Admin
struct is unused and creates confusion. Either remove the Email field entirely
from the Admin struct to keep it clean, or add a clarifying comment above the
field explaining why it exists but is intentionally not populated by the query.
In `@src/comment.go`:
- Around line 280-287: In the GetCommentatorOrcid method, modify the error
handling to check if the returned error equals sql.ErrNoRows and map it to a
domain-specific error like ErrRecordNotFound for consistency with how
GetOwnerOrcid handles the same scenario. If the error is sql.ErrNoRows, return
the domain-specific error; otherwise, return the original error. This ensures
callers can reliably distinguish between "comment not found" and other database
errors.
In `@src/email_worker.go`:
- Around line 9-20: The EmailWorker struct and its NewEmailWorker constructor
are tightly coupled to the concrete OrcidService type when an OrcidResolver
interface already exists for this purpose. Replace the orcidService field in the
EmailWorker struct (line 12) from the concrete type *OrcidService to the
OrcidResolver interface type, and update the corresponding parameter in the
NewEmailWorker constructor function (line 15) to also use the OrcidResolver
interface instead of *OrcidService. This follows the Dependency Inversion
Principle and allows the worker to depend on abstractions rather than concrete
implementations.
In `@src/notification_service.go`:
- Around line 11-15: The commentRepo field in the NotificationService struct is
stored but never used by any method in the file. Either remove the commentRepo
field from the NotificationService struct to eliminate the unnecessary
dependency, or if it is intended for future use, add a comment above the field
explaining its intended purpose.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a682837-befb-4a30-8fa9-15c445162308
📒 Files selected for processing (23)
Dockerfilesrc/admin_repository.gosrc/build.shsrc/category_repository.gosrc/comment.gosrc/comment_handler.gosrc/email.gosrc/email_model.gosrc/email_queue_repository.gosrc/email_sender.gosrc/email_worker.gosrc/main.gosrc/model.gosrc/moderation.gosrc/moderation_handler.gosrc/notification_service.gosrc/oidc.gosrc/orcid_service.gosrc/record_handler.gosrc/record_repository.gosrc/sql/014_add_email_queue.up.sqlsrc/sql/015_add_email_queue.down.sqlsrc/templates/browse.html
💤 Files with no reviewable changes (2)
- src/email.go
- src/moderation.go
* check smtp status code * remove useless code
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/comment.go (1)
280-287: ⚡ Quick win
GetCommentatorOrciddoesn't mapsql.ErrNoRowsto a domain error.Unlike
GetOwnerOrcidwhich mapssql.ErrNoRowstoErrRecordNotFound, this method returns the raw SQL error when the comment doesn't exist. Consider adding consistent error handling.Proposed fix
func (r *PostgresCommentRepository) GetCommentatorOrcid(ctx context.Context, id int64) (string, error) { var commenterOrcid string err := r.db.QueryRowContext(ctx, `SELECT commenter_orcid FROM comments WHERE id = $1`, id).Scan(&commenterOrcid) + if err == sql.ErrNoRows { + return "", fmt.Errorf("comment not found: %d", id) + } if err != nil { return "", err } return commenterOrcid, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/comment.go` around lines 280 - 287, The GetCommentatorOrcid method in PostgresCommentRepository is returning raw SQL errors instead of mapping them to domain errors for consistency. Add error handling after the QueryRowContext Scan operation to check if the error is sql.ErrNoRows and return the domain error ErrRecordNotFound in that case, similar to how GetOwnerOrcid handles this scenario. Only return the raw error for other types of database errors.src/notification_service.go (1)
111-124: ⚖️ Poor tradeoff
enqueueForAdminsfails fast on first admin error, leaving partial notification state.If enqueuing to one admin fails (line 120), the loop returns immediately. Subsequent admins won't be notified, but there's no rollback of already-enqueued items. Consider logging failures and continuing to notify remaining admins.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/notification_service.go` around lines 111 - 124, The `enqueueForAdmins` method in NotificationService returns immediately when enqueuing fails for a single admin, leaving subsequent admins unnotified and creating partial notification state. Instead of returning on the first error inside the for loop that iterates over notifiableAdmins, log the error for that specific admin and continue processing the remaining admins in the loop. Either collect all errors encountered and return them at the end, or log them individually and complete the notification attempt for all admins before returning.src/email_worker.go (1)
85-89: ⚡ Quick winAdd identifying information to the success log.
Line 89 logs a generic success message without any identifying information (queue ID, recipient, or subject). Including at least the queue ID would help correlate logs with the queue table for debugging and monitoring.
📊 Proposed improvement
markErr := w.emailQueueRepo.MarkAsSent(ctx, pending.Id) if markErr != nil { return fmt.Errorf("%s: failed to mark email as sent (queue_id %d): %w", worker, pending.Id, markErr) } - log.Printf("%s: email sent successfully", worker) + log.Printf("%s: email sent successfully (queue_id %d, recipient %s)", worker, pending.Id, pending.RecipientOrcid)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/email_worker.go` around lines 85 - 89, The success log message using log.Printf is missing identifying information like the queue ID. Modify the log.Printf call that logs "email sent successfully" to include at least the pending.Id (queue ID) in the log output, similar to how the error log above it includes pending.Id in the error message. This will provide better correlation between logs and the queue table.src/email_sender.go (1)
41-83: 🏗️ Heavy liftPropagate caller context into
Sendinstead of starting fromcontext.Background().Line 76 breaks cancellation propagation. This weakens graceful shutdown because SMTP I/O won’t stop until timeout even if the worker context is canceled.
♻️ Suggested refactor
-func (e *EmailSender) Send(to string, subject string, bodyText string, bodyHTML string) error { +func (e *EmailSender) Send(ctx context.Context, to string, subject string, bodyText string, bodyHTML string) error { @@ - ctx, cancel := context.WithTimeout(context.Background(), smtpTimeout) + ctx, cancel := context.WithTimeout(ctx, smtpTimeout) defer cancel()Call-site update needed (outside this file), e.g. pass worker context into
Send.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/email_sender.go` around lines 41 - 83, The Send method in EmailSender creates a new context from context.Background() instead of accepting a caller-provided context, which breaks cancellation propagation during graceful shutdown. Add a context parameter as the first argument to the Send method signature, then use context.WithTimeout on the passed context instead of on context.Background() to ensure SMTP operations respect caller cancellation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/email_queue_repository.go`:
- Around line 76-113: The MarkAsSent, MarkAsFailed, and MarkForRetry methods
update rows by id only, creating a race condition where concurrent workers can
overwrite each other's status transitions since stale rows can be reclaimed for
reprocessing. Add expected-state guards to the WHERE clause of each UPDATE
statement by including a status check along with the id check, so that only rows
currently in the "processing" status can be transitioned to sent, failed, or
retry states. This prevents late-arriving updates from overwriting transitions
already made by other workers, and the existing row count validation will catch
any races by detecting when zero rows are updated.
In `@src/notification_service.go`:
- Around line 74-86: The buildModerationBody function appends a generic "You can
view it here" message to the email body for both approved and rejected statuses,
but this is misleading since rejected content is not published. Modify the
function to conditionally construct the fullBody string based on the
ModerationStatus value, so that only approved items include the "You can view it
here" link, while rejected items use a different closing message that does not
reference viewing the item on the platform.
In `@src/orcid_service.go`:
- Around line 41-47: Add input validation at the beginning of the GetEmail
function to ensure the orcid parameter is non-empty. If orcid is an empty
string, immediately return an error without proceeding to the URL construction
on line 42. This prevents the creation of malformed URLs like
https://pub.orcid.org/v3.0//email and provides clear, defensive error handling
for callers passing invalid input to this public API function.
---
Nitpick comments:
In `@src/comment.go`:
- Around line 280-287: The GetCommentatorOrcid method in
PostgresCommentRepository is returning raw SQL errors instead of mapping them to
domain errors for consistency. Add error handling after the QueryRowContext Scan
operation to check if the error is sql.ErrNoRows and return the domain error
ErrRecordNotFound in that case, similar to how GetOwnerOrcid handles this
scenario. Only return the raw error for other types of database errors.
In `@src/email_sender.go`:
- Around line 41-83: The Send method in EmailSender creates a new context from
context.Background() instead of accepting a caller-provided context, which
breaks cancellation propagation during graceful shutdown. Add a context
parameter as the first argument to the Send method signature, then use
context.WithTimeout on the passed context instead of on context.Background() to
ensure SMTP operations respect caller cancellation.
In `@src/email_worker.go`:
- Around line 85-89: The success log message using log.Printf is missing
identifying information like the queue ID. Modify the log.Printf call that logs
"email sent successfully" to include at least the pending.Id (queue ID) in the
log output, similar to how the error log above it includes pending.Id in the
error message. This will provide better correlation between logs and the queue
table.
In `@src/notification_service.go`:
- Around line 111-124: The `enqueueForAdmins` method in NotificationService
returns immediately when enqueuing fails for a single admin, leaving subsequent
admins unnotified and creating partial notification state. Instead of returning
on the first error inside the for loop that iterates over notifiableAdmins, log
the error for that specific admin and continue processing the remaining admins
in the loop. Either collect all errors encountered and return them at the end,
or log them individually and complete the notification attempt for all admins
before returning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 738dc1f1-582f-4a7d-a9c8-09ba104e97ca
📒 Files selected for processing (24)
.gitignoreDockerfilesrc/admin_repository.gosrc/build.shsrc/category_repository.gosrc/comment.gosrc/comment_handler.gosrc/email.gosrc/email_model.gosrc/email_queue_repository.gosrc/email_sender.gosrc/email_worker.gosrc/main.gosrc/model.gosrc/moderation.gosrc/moderation_handler.gosrc/notification_service.gosrc/oidc.gosrc/orcid_service.gosrc/record_handler.gosrc/record_repository.gosrc/sql/014_add_email_queue.down.sqlsrc/sql/014_add_email_queue.up.sqlsrc/templates/browse.html
💤 Files with no reviewable changes (2)
- src/email.go
- src/moderation.go
* remove confusion for user if record/comment is rejected * prevent overwrite
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
* add test for successful ProcessPending flow
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/category_repository.go (1)
268-274:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
txbefore executing the delete.Line 269 dereferences
txunconditionally; a nil transaction will panic and take down the request path. Return a normal error instead.Proposed fix
func (r *PostgresCategoryRepository) ClearRecordCategories(ctx context.Context, tx *sql.Tx, recordID string) error { + if tx == nil { + return fmt.Errorf("category repository: transaction is nil") + } _, err := tx.ExecContext(ctx, `DELETE FROM records_categories WHERE record_id = $1`, recordID, ) return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/category_repository.go` around lines 268 - 274, The ClearRecordCategories method unconditionally dereferences the tx parameter when calling tx.ExecContext, which will cause a panic if tx is nil and crash the request. Add a nil guard at the start of the ClearRecordCategories method to check if tx is nil before attempting to use it, and return an appropriate error instead of allowing the panic to occur.
♻️ Duplicate comments (2)
src/templates/browse.html (1)
5-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix remaining grammar issues in the privacy notice.
The privacy notice content is good, but three grammar issues remain:
- "informations" → "information" (information is uncountable)
- "Beta mode" → "beta mode" (lowercase in running text)
- "Thank you for helping us to test so that we can open the site" → "Thank you for helping us test the site" (clearer, concise)
These corrections improve readability and polish for a public-facing notice.
✏️ Suggested wording
- Welcome to the template sharing site, this is still in <b>Beta mode</b>. For more informations, please see the <a href="/about">About</a> page.<br>Please note that we do not store your email address. To receive notifications, make sure your email address is set to <b>public</b> on orcid.org<br><br>Thank you for helping us to test so that we can open the site. + Welcome to the template sharing site. This is still in <b>beta mode</b>. For more information, please see the <a href="/about">About</a> page.<br>Please note that we do not store your email address. To receive notifications, make sure your email address is set to <b>public</b> on orcid.org.<br><br>Thank you for helping us test the site before general release.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/templates/browse.html` at line 5, Fix three grammar issues in the privacy notice welcome message. Change "informations" to "information" since information is an uncountable noun. Change "Beta mode" to "beta mode" to use lowercase in running text. Replace the phrase "Thank you for helping us to test so that we can open the site" with "Thank you for helping us test the site" to make it clearer and more concise.src/email_worker.go (1)
78-98:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftPrevent duplicate delivery by claiming before the external send.
On Line 87, SMTP send happens before a durable ownership/processing transition; if the process crashes or Line 95 fails, the same pending row can be delivered again on a later run. Move to an atomic claim/in-progress step before send and finalize only from claimed state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/email_worker.go` around lines 78 - 98, The email sending loop has a race condition where emails can be delivered twice if a crash occurs after the Send call but before MarkAsSent is persisted. To fix this, add an atomic claim/in-progress step before sending the email in the emailSender.Send call (around line 87). Call a method to mark the pending email record as claimed or in-progress before attempting to send via the emailSender.Send method, and only proceed with the send if the claim succeeds. This ensures that even if the process crashes after sending, the email row will be marked as claimed and won't be reprocessed on the next worker run. Then finalize by marking it as sent with MarkAsSent after a successful send.
🧹 Nitpick comments (3)
src/comment_handler.go (1)
183-201: ⚡ Quick winUse the already-loaded comment owner ORCID.
Line 184 already loads
comment, so the extraGetCommentatorOrcidquery at Line 198 adds a second DB failure point before any fan-out. Usecomment.CommenterOrciddirectly and keep notification failures best-effort.♻️ Proposed simplification
recordOwner, err := h.recordRepo.GetOwnerOrcid(ctx, comment.RecordID) if err != nil { return fmt.Errorf("%s: failed to get owner orcid: %v", handler, err) } - commentOwner, err := h.commentRepo.GetCommentatorOrcid(ctx, commentID) - if err != nil { - return fmt.Errorf("%s: failed to get commentator orcid: %v", handler, err) - } + commentOwner := strings.TrimSpace(comment.CommenterOrcid) if commentOwner != recordOwner {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/comment_handler.go` around lines 183 - 201, In the createApprovedNotification function, remove the redundant database query to GetCommentatorOrcid (which queries the database using commentID) since the comment object is already loaded at the beginning of the function via h.commentRepo.GetByID. Instead of making this extra query, directly use the commentator ORCID from the already-loaded comment object by accessing comment.CommenterOrcid field. This eliminates an unnecessary database call and reduces failure points before the notification is sent.src/sql/014_add_email_queue.up.sql (1)
19-19: ⚡ Quick winOptimize queue polling indexes for the worker hot path.
Line 19 adds only
status, butGetPendingalso filters onmodified_atand orders bycreated_at; asemail_queuegrows, this can degrade into repeated sorts/scans every poll cycle.💡 Suggested migration adjustment
-CREATE INDEX idx_email_queue_status ON email_queue(status); +CREATE INDEX idx_email_queue_pending_created_at + ON email_queue(status, created_at); + +CREATE INDEX idx_email_queue_processing_modified_at + ON email_queue(status, modified_at) + WHERE status = 1;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sql/014_add_email_queue.up.sql` at line 19, The CREATE INDEX statement for idx_email_queue_status on the email_queue table only includes the status column, but the GetPending query filters on both status and modified_at while also ordering by created_at. To optimize this hot path and prevent expensive sorts and scans as the table grows, modify the index definition to include all columns used in the GetPending filtering and ordering logic (status, modified_at, and created_at) to create a composite index that matches the query access pattern.src/email_worker_test.go (1)
106-256: ⚡ Quick winAdd retry-policy tests for ORCID HTTP status and attempt boundary.
Current tests miss the new/expected branch where ORCID non-retryable HTTP errors should fail immediately, and they don’t lock the
Attempts+1 < maxAttemptsboundary behavior.Suggested additional tests
+func TestRetryOrFailHTTPStatus4xxFailsImmediately(t *testing.T) { + ctx := context.Background() + mockRepo := &MockEmailQueueRepository{} + worker := &EmailWorker{emailQueueRepo: mockRepo} + pending := EmailQueue{Id: 7, Attempts: 0} + + err := worker.retryOrFail(ctx, pending, "recipient email resolution", &HTTPStatusError{ + StatusCode: 404, + Message: "fetch email", + }) + if err != nil { + t.Fatalf("expected retryOrFail to succeed, got error: %v", err) + } + if !mockRepo.markAsFailedCalled || mockRepo.markForRetryCalled { + t.Fatal("expected MarkAsFailed only") + } +} + +func TestRetryOrFailFailsAtMaxAttemptBoundary(t *testing.T) { + ctx := context.Background() + mockRepo := &MockEmailQueueRepository{} + worker := &EmailWorker{emailQueueRepo: mockRepo} + pending := EmailQueue{Id: 9, Attempts: maxAttempts - 1} + + err := worker.retryOrFail(ctx, pending, "send", fmt.Errorf("temporary network issue")) + if err != nil { + t.Fatalf("expected retryOrFail to succeed, got error: %v", err) + } + if !mockRepo.markAsFailedCalled || mockRepo.markForRetryCalled { + t.Fatal("expected MarkAsFailed only at max attempt boundary") + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/email_worker_test.go` around lines 106 - 256, The test suite is missing coverage for two critical scenarios: Add a test case that verifies when the ORCID service returns a non-retryable HTTP error (e.g., 4xx status codes), the worker should call MarkAsFailed instead of MarkForRetry in the ProcessPending method. Additionally, add test cases that cover the attempt boundary behavior, specifically testing the condition where Attempts+1 reaches or exceeds maxAttempts to ensure emails are marked as failed when the retry limit is exhausted, and verify they are still marked for retry when attempts remain below the limit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/email_sender.go`:
- Line 41: The Send method in EmailSender does not accept a context parameter,
which prevents graceful shutdown signals from being propagated to the SMTP
operation. Modify the Send method signature to accept a context.Context as the
first parameter, then replace the context.Background() call on line 76 with the
passed context parameter. Finally, update the call site in ProcessPending where
Send is invoked (line 87) to pass its available context to the Send method.
In `@src/email_worker.go`:
- Around line 60-67: The retryOrFail function currently does not treat HTTP 4xx
status errors as permanent failures, causing unnecessary retries for cases like
404 invalid ORCID responses. Extend the condition that checks for permanent
errors by adding a check for HTTPStatusError with 4xx status codes (along with
the existing checks for EmailUnavailable and isSMTPPermanentErr). This will
ensure that HTTP 4xx errors are caught by the first branch and handled as
terminal failures via the failed method, eliminating avoidable queue churn from
retry attempts.
In `@src/notification_service.go`:
- Around line 74-88: The buildModerationBody function only handles
StatusApproved and StatusRejected cases in its switch statement, meaning any
other ModerationStatus value will result in an empty or incomplete email body
being sent to users. Add explicit status validation in the moderation
entrypoints (the callers of buildModerationBody, referenced at lines 150-169) to
ensure only valid and expected ModerationStatus values are passed to this
function, either by validating the status before calling buildModerationBody or
by adding a default case in the switch statement that returns an error for
unexpected statuses.
- Around line 113-117: In the enqueueForAdmins method, add a nil check for the
adminRepo field before calling GetAllAdmins. The check should verify if
s.adminRepo is nil, and if it is, return a controlled error (using fmt.Errorf
with appropriate context) instead of allowing the code to proceed and
potentially panic. Place this nil check immediately after the function
signature, before the call to s.adminRepo.GetAllAdmins(ctx).
- Around line 128-184: Add nil pointer validation checks at the beginning of
each exported method that accepts pointer arguments. Specifically, in
CreateForRecord check if the record parameter is nil before dereferencing its
fields (UploaderName, Id), in CreateForComment check if the comment parameter is
nil before accessing its fields (CommenterName, Content, RecordID, ID), in
CreateForCommentModeration validate the comment parameter before dereferencing,
and in CreateForApprovedComment validate the comment parameter before
dereferencing. For each method, return an explicit validation error using a
helper function (similar to notificationErr) if any required pointer argument is
nil, ensuring these methods fail gracefully with a clear error message rather
than panicking.
---
Outside diff comments:
In `@src/category_repository.go`:
- Around line 268-274: The ClearRecordCategories method unconditionally
dereferences the tx parameter when calling tx.ExecContext, which will cause a
panic if tx is nil and crash the request. Add a nil guard at the start of the
ClearRecordCategories method to check if tx is nil before attempting to use it,
and return an appropriate error instead of allowing the panic to occur.
---
Duplicate comments:
In `@src/email_worker.go`:
- Around line 78-98: The email sending loop has a race condition where emails
can be delivered twice if a crash occurs after the Send call but before
MarkAsSent is persisted. To fix this, add an atomic claim/in-progress step
before sending the email in the emailSender.Send call (around line 87). Call a
method to mark the pending email record as claimed or in-progress before
attempting to send via the emailSender.Send method, and only proceed with the
send if the claim succeeds. This ensures that even if the process crashes after
sending, the email row will be marked as claimed and won't be reprocessed on the
next worker run. Then finalize by marking it as sent with MarkAsSent after a
successful send.
In `@src/templates/browse.html`:
- Line 5: Fix three grammar issues in the privacy notice welcome message. Change
"informations" to "information" since information is an uncountable noun. Change
"Beta mode" to "beta mode" to use lowercase in running text. Replace the phrase
"Thank you for helping us to test so that we can open the site" with "Thank you
for helping us test the site" to make it clearer and more concise.
---
Nitpick comments:
In `@src/comment_handler.go`:
- Around line 183-201: In the createApprovedNotification function, remove the
redundant database query to GetCommentatorOrcid (which queries the database
using commentID) since the comment object is already loaded at the beginning of
the function via h.commentRepo.GetByID. Instead of making this extra query,
directly use the commentator ORCID from the already-loaded comment object by
accessing comment.CommenterOrcid field. This eliminates an unnecessary database
call and reduces failure points before the notification is sent.
In `@src/email_worker_test.go`:
- Around line 106-256: The test suite is missing coverage for two critical
scenarios: Add a test case that verifies when the ORCID service returns a
non-retryable HTTP error (e.g., 4xx status codes), the worker should call
MarkAsFailed instead of MarkForRetry in the ProcessPending method. Additionally,
add test cases that cover the attempt boundary behavior, specifically testing
the condition where Attempts+1 reaches or exceeds maxAttempts to ensure emails
are marked as failed when the retry limit is exhausted, and verify they are
still marked for retry when attempts remain below the limit.
In `@src/sql/014_add_email_queue.up.sql`:
- Line 19: The CREATE INDEX statement for idx_email_queue_status on the
email_queue table only includes the status column, but the GetPending query
filters on both status and modified_at while also ordering by created_at. To
optimize this hot path and prevent expensive sorts and scans as the table grows,
modify the index definition to include all columns used in the GetPending
filtering and ordering logic (status, modified_at, and created_at) to create a
composite index that matches the query access pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3eecf246-91ed-40bc-b7cb-56d200723d14
📒 Files selected for processing (26)
.gitignoreDockerfilesrc/admin_repository.gosrc/build.shsrc/category_handler_test.gosrc/category_repository.gosrc/comment.gosrc/comment_handler.gosrc/email.gosrc/email_model.gosrc/email_queue_repository.gosrc/email_sender.gosrc/email_worker.gosrc/email_worker_test.gosrc/main.gosrc/model.gosrc/moderation.gosrc/moderation_handler.gosrc/notification_service.gosrc/oidc.gosrc/orcid_service.gosrc/record_handler.gosrc/record_repository.gosrc/sql/014_add_email_queue.down.sqlsrc/sql/014_add_email_queue.up.sqlsrc/templates/browse.html
💤 Files with no reviewable changes (2)
- src/email.go
- src/moderation.go
* rabbit review
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/email_sender.go (1)
24-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when required SMTP config is missing.
NewEmailSenderaccepts empty env values and defers failure to delivery time. This should fail at startup so queue processing doesn’t churn on predictable misconfiguration.💡 Proposed fix
func NewEmailSender() *EmailSender { - return &EmailSender{ + s := &EmailSender{ smtpHost: os.Getenv("SMTP_HOST"), smtpPort: os.Getenv("SMTP_PORT"), smtpFromAddress: os.Getenv("SMTP_FROM_ADDRESS"), smtpUsername: os.Getenv("SMTP_USERNAME"), smtpPassword: os.Getenv("SMTP_PASSWORD"), } + if s.smtpHost == "" || s.smtpPort == "" || s.smtpFromAddress == "" || s.smtpUsername == "" || s.smtpPassword == "" { + return nil, fmt.Errorf("email sender: missing SMTP configuration") + } + return s, nil }// src/main.go call site emailSender, err := NewEmailSender() if err != nil { log.Fatalf("failed to init email sender: %v", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/email_sender.go` around lines 24 - 32, The NewEmailSender function currently accepts environment variables without validating they contain required values, deferring failures to email delivery time. Modify the NewEmailSender function to validate that all required SMTP configuration values (SMTP_HOST, SMTP_PORT, SMTP_FROM_ADDRESS, SMTP_USERNAME, SMTP_PASSWORD) are present and non-empty, and return an error if any are missing. Update the function signature to return both a pointer to EmailSender and an error. Then update the call site in main.go to handle the returned error and fail fast with a descriptive log message if initialization fails.src/main.go (1)
75-77:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign interval comments/constants with actual worker cadence.
Current comments say “every minute” while the configured ticker is 20 seconds. Keep these in sync to avoid operator confusion during incident/debug work.
💡 Proposed fix
-// TODO: set to 60 const EMAIL_NOTIF_INTERVAL_SEC = 20 @@ -// process notification event every minute +// Process notification emails every EMAIL_NOTIF_INTERVAL_SEC seconds.Also applies to: 497-498
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.go` around lines 75 - 77, The EMAIL_NOTIF_INTERVAL_SEC constant is set to 20 seconds but has a TODO comment indicating it should be set to 60, which aligns with the "every minute" documentation mentioned in the review. Update the EMAIL_NOTIF_INTERVAL_SEC constant value from 20 to 60 to align with the intended cadence and remove the TODO comment once updated. Additionally, locate the similar interval configuration issue at lines 497-498 and apply the same fix to ensure consistency across the codebase.src/moderation_handler.go (1)
196-197:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle owner ORCID lookup failure before entering the action switch.
Line 196 fetches
uploaderOrcidbut ignoreserr; moderation continues even when owner resolution fails, and notification behavior degrades to a later logged failure instead of a controlled response.🐛 Proposed fix
- uploaderOrcid, err := h.recordRepo.GetOwnerOrcid(ctx, id) + uploaderOrcid, err := h.recordRepo.GetOwnerOrcid(ctx, id) + if err != nil { + errorLogger.Printf("moderation handler: failed to resolve uploader ORCID for record %s: %v", id, err) + http.Error(w, "Error resolving record owner", http.StatusInternalServerError) + return + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/moderation_handler.go` around lines 196 - 197, The GetOwnerOrcid method call on h.recordRepo is ignoring the returned error, allowing moderation to continue even when owner resolution fails. Add proper error handling immediately after the GetOwnerOrcid call to check if err is not nil, and return an appropriate error response or log message before proceeding further into the action switch statement or any subsequent logic. This ensures that owner resolution failures are handled in a controlled manner rather than causing degraded notification behavior later.
🧹 Nitpick comments (1)
src/email_worker_test.go (1)
107-179: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd explicit tests for HTTPStatusError retry classification.
retryOrFailnow has HTTP 4xx/429-specific branching, but this file only validates SMTP branches. Add direct tests for404 -> MarkAsFailedand429 -> MarkForRetryto lock policy behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/email_worker_test.go` around lines 107 - 179, The test file currently only validates SMTP error handling branches in the retryOrFail function with TestRetryOrFailSMTPPermanentError and TestRetryOrFailSMTPTemporaryError, but does not test HTTP status error handling. Add two new test functions following the same pattern: one test that verifies HTTP 404 errors result in MarkAsFailed being called, and another test that verifies HTTP 429 errors result in MarkForRetry being called. Both tests should create an HTTPStatusError, pass it to retryOrFail, and assert the expected repository method was invoked with the correct queue ID.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/email_sender.go`:
- Around line 24-32: The NewEmailSender function currently accepts environment
variables without validating they contain required values, deferring failures to
email delivery time. Modify the NewEmailSender function to validate that all
required SMTP configuration values (SMTP_HOST, SMTP_PORT, SMTP_FROM_ADDRESS,
SMTP_USERNAME, SMTP_PASSWORD) are present and non-empty, and return an error if
any are missing. Update the function signature to return both a pointer to
EmailSender and an error. Then update the call site in main.go to handle the
returned error and fail fast with a descriptive log message if initialization
fails.
In `@src/main.go`:
- Around line 75-77: The EMAIL_NOTIF_INTERVAL_SEC constant is set to 20 seconds
but has a TODO comment indicating it should be set to 60, which aligns with the
"every minute" documentation mentioned in the review. Update the
EMAIL_NOTIF_INTERVAL_SEC constant value from 20 to 60 to align with the intended
cadence and remove the TODO comment once updated. Additionally, locate the
similar interval configuration issue at lines 497-498 and apply the same fix to
ensure consistency across the codebase.
In `@src/moderation_handler.go`:
- Around line 196-197: The GetOwnerOrcid method call on h.recordRepo is ignoring
the returned error, allowing moderation to continue even when owner resolution
fails. Add proper error handling immediately after the GetOwnerOrcid call to
check if err is not nil, and return an appropriate error response or log message
before proceeding further into the action switch statement or any subsequent
logic. This ensures that owner resolution failures are handled in a controlled
manner rather than causing degraded notification behavior later.
---
Nitpick comments:
In `@src/email_worker_test.go`:
- Around line 107-179: The test file currently only validates SMTP error
handling branches in the retryOrFail function with
TestRetryOrFailSMTPPermanentError and TestRetryOrFailSMTPTemporaryError, but
does not test HTTP status error handling. Add two new test functions following
the same pattern: one test that verifies HTTP 404 errors result in MarkAsFailed
being called, and another test that verifies HTTP 429 errors result in
MarkForRetry being called. Both tests should create an HTTPStatusError, pass it
to retryOrFail, and assert the expected repository method was invoked with the
correct queue ID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55383769-9f00-4df3-ac29-7cf5883957a2
📒 Files selected for processing (27)
.gitignoreDockerfilesrc/admin_repository.gosrc/build.shsrc/category_handler_test.gosrc/category_repository.gosrc/comment.gosrc/comment_handler.gosrc/email.gosrc/email_model.gosrc/email_queue_repository.gosrc/email_sender.gosrc/email_worker.gosrc/email_worker_test.gosrc/main.gosrc/model.gosrc/moderation.gosrc/moderation_handler.gosrc/notification_service.gosrc/oidc.gosrc/orcid_service.gosrc/orcid_service_test.gosrc/record_handler.gosrc/record_repository.gosrc/sql/014_add_email_queue.down.sqlsrc/sql/014_add_email_queue.up.sqlsrc/templates/browse.html
💤 Files with no reviewable changes (2)
- src/email.go
- src/moderation.go
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Tests
Chores