Conversation
main.go: pass default false to config.GetBool so --init flag handling doesn't panic when flag is absent. web: extract monitorsOf() helper + PageRow type in status-page-table.vue for cleaner template access.
- SSL Certificate Monitoring: check_ssl toggle, TLS cert expiry detection in scheduler, frontend badge - Heartbeat/Cron Monitoring: POST /api/heartbeat/:token endpoint, random token generation, missed-ping detection logic - Maintenance Windows: full CRUD module, DB join table for monitor association, incident suppression during active windows - Audit Logs: event-driven module subscribing to entity CRUD events, admin-only paginated view - Status Badge SVG: public /api/status-pages/:slug/badge.svg endpoint with shields-style SVG - Subscribers: email subscribe/verify/unsubscribe for status page updates, public endpoints
|
Warning Review limit reached
More reviews will be available in 31 minutes and 10 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR adds three new backend modules ( ChangesNew Modules and Monitor Enhancements
Sequence Diagram(s)sequenceDiagram
participant Client
participant MonitorHandler
participant MonitorDB
participant IncidentDB
participant EventBus
Client->>MonitorHandler: POST /heartbeat/:token
MonitorHandler->>MonitorDB: WHERE heartbeat_token = token
MonitorDB-->>MonitorHandler: monitor record
MonitorHandler->>MonitorHandler: validate token & active status
MonitorHandler->>MonitorDB: save monitor (UptimeStatus, LastCheckedAt, LastLatency)
MonitorHandler->>MonitorDB: create check record
MonitorHandler->>EventBus: publish monitor.checked
MonitorHandler->>IncidentDB: find active incidents for monitor
loop each active incident
MonitorHandler->>IncidentDB: mark resolved, set ResolvedAt
MonitorHandler->>EventBus: publish incident.resolved
end
MonitorHandler-->>Client: 200 OK
sequenceDiagram
participant Client
participant AuditLogHandler
participant EventBus
participant AuditLogService
participant AuditLogDB
EventBus->>AuditLogHandler: event (e.g. monitor.created)
AuditLogHandler->>AuditLogHandler: marshal payload, splitEventType → entityType/action
AuditLogHandler->>AuditLogHandler: extract entity id from payload map
AuditLogHandler->>AuditLogService: Log(ctx, userID, action, entityType, entityID, details)
AuditLogService->>AuditLogDB: INSERT audit_log row
Client->>AuditLogHandler: GET /audit-logs
AuditLogHandler->>AuditLogService: GetAll(ctx)
AuditLogService->>AuditLogDB: SELECT * ORDER BY created_at DESC LIMIT 200
AuditLogDB-->>AuditLogHandler: []AuditLog
AuditLogHandler-->>Client: 200 JSON
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/views/App.Monitors.vue (1)
573-590:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winURL field should be hidden or disabled for heartbeat monitors.
Heartbeat monitors don't use a URL (the backend clears it in
NewMonitor), but the form still requires a URL input. Users selecting "Heartbeat" type will be confused about what URL to enter.Consider conditionally hiding the URL field or showing a help message when
formType === 'heartbeat'.💡 Suggested approach
<div class="space-y-2" v-if="formType !== 'heartbeat'"> <Label for="url">Target URL</Label> <Input id="url" v-model="formUrl" placeholder="e.g. https://example.com/health" type="url" required /> </div> <div v-else class="space-y-2"> <Label>Heartbeat Token</Label> <p class="text-xs text-muted-foreground"> A unique token will be generated. External services will POST to <code>/api/heartbeat/:token</code>. </p> </div>🤖 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 `@web/src/views/App.Monitors.vue` around lines 573 - 590, The URL input field with v-model="formUrl" is always displayed regardless of the monitor type, but heartbeat monitors don't require a URL input. Add a conditional v-if directive to the div containing the Label "Target URL" and the Input with id="url" to hide this field when formType equals "heartbeat". Replace it with an alternative section (using v-else) that explains heartbeat monitors use a generated token instead, providing users with clear guidance on how heartbeat monitoring works in your system.
🟡 Minor comments (5)
modules/subscribers/handler/subscriber_handler.go-49-51 (1)
49-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused
verifyQuerytype.This type is never referenced and is already flagged by static analysis.
🤖 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 `@modules/subscribers/handler/subscriber_handler.go` around lines 49 - 51, The verifyQuery struct type is never used anywhere in the codebase and should be removed to keep the code clean. Delete the entire verifyQuery type definition (the struct with the Token field tagged with json and query tags) from the subscriber_handler.go file.Source: Linters/SAST tools
modules/subscribers/handler/subscriber_handler.go-74-79 (1)
74-79:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate unsubscribe input before calling the service.
Unsubscribebinds request data but never runsc.Validate(req), so malformed payloads are misclassified as “not found” instead of 400 input errors.Suggested fix
req := new(unsubscribeRequest) if err := c.Bind(req); err != nil { return h.r.BadRequestResponse(c, err.Error()) } + if err := c.Validate(req); err != nil { + return h.r.BadRequestResponse(c, err.Error()) + } if err := h.svc.Unsubscribe(ctx, req.Email, req.StatusPageID); err != nil { return h.r.NotFoundResponse(c, "Subscription not found") }🤖 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 `@modules/subscribers/handler/subscriber_handler.go` around lines 74 - 79, The unsubscribe handler in subscriber_handler.go binds the request data but does not validate it, causing malformed payloads to be mishandled. After the existing c.Bind(req) call that handles binding errors, add a validation step by calling c.Validate(req) and handle any validation errors by returning a BadRequestResponse similar to the binding error handling, ensuring that invalid input is properly rejected with a 400 status code before the h.svc.Unsubscribe service method is invoked.modules/monitors/domain/service/scheduler.go-112-118 (1)
112-118:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMaintenance window query uses
time.Now()twice, creating a potential race window.The two
time.Now()calls in the WHERE clause could return slightly different values if the clock ticks between them. Use a singlenowvariable for consistency.🛠️ Proposed fix
if newStatus == "down" { var count int64 + checkTime := time.Now() database.DB.WithContext(ctx). Table("maintenance_monitors"). Joins("JOIN maintenances ON maintenances.id = maintenance_monitors.maintenance_id"). Where("maintenance_monitors.monitor_id = ?", mon.ID). - Where("maintenances.start_at <= ? AND maintenances.end_at >= ?", time.Now(), time.Now()). + Where("maintenances.start_at <= ? AND maintenances.end_at >= ?", checkTime, checkTime). Count(&count) inMaintenance = count > 0 }🤖 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 `@modules/monitors/domain/service/scheduler.go` around lines 112 - 118, The database query checking maintenance windows calls time.Now() twice in the WHERE clause, which could result in different timestamps if the system clock ticks between calls. Before executing the database query with the Joins and Where methods, capture time.Now() once into a variable named something like now, then use that variable in both places in the WHERE clause where it currently calls time.Now() for the start_at and end_at comparisons.web/src/components/monitor-table.vue-52-60 (1)
52-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix day rounding so recent expirations aren’t shown as “Today.”
Line 52 uses
Math.ceil, which turns small negative diffs into0; that makes recently expired certs display as"Today"instead of"Expired".Suggested fix
- return Math.ceil(diff / (1000 * 60 * 60 * 24)); + return Math.floor(diff / (1000 * 60 * 60 * 24));🤖 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 `@web/src/components/monitor-table.vue` around lines 52 - 60, The daysUntilExpiryNum function uses Math.ceil to round the day difference, which incorrectly converts small negative values (recently expired certs) to 0, causing them to display as "Today" instead of "Expired". Replace Math.ceil with Math.floor in the daysUntilExpiryNum function so that negative differences remain negative and are properly handled by the daysUntilExpiry function's logic checks.web/src/views/App.AuditLogs.vue-16-21 (1)
16-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign action badge mapping with backend action values.
Lines 17-19 map past-tense verbs (
created/updated/deleted), while the backend audit entity contract usescreate/update/delete, so common events fall back to the default color.Suggested fix
const actionColor = (a: string) => { switch (a) { - case 'created': return 'bg-emerald-500/10 text-emerald-500'; - case 'updated': return 'bg-blue-500/10 text-blue-500'; - case 'deleted': return 'bg-red-500/10 text-red-500'; + case 'create': + case 'created': return 'bg-emerald-500/10 text-emerald-500'; + case 'update': + case 'updated': return 'bg-blue-500/10 text-blue-500'; + case 'delete': + case 'deleted': return 'bg-red-500/10 text-red-500'; case 'resolved': return 'bg-emerald-500/10 text-emerald-500'; default: return 'bg-slate-500/10 text-slate-500'; } };🤖 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 `@web/src/views/App.AuditLogs.vue` around lines 16 - 21, The switch statement in the action badge mapping uses past-tense verb cases (created, updated, deleted) but the backend audit entity contract uses present-tense verbs (create, update, delete), causing common events to incorrectly fall back to the default slate color. Update the case statements in the switch block to match the backend action values by changing 'created' to 'create', 'updated' to 'update', and 'deleted' to 'delete' while preserving their corresponding color styles.
🧹 Nitpick comments (3)
modules/monitors/domain/service/scheduler.go (1)
313-315: ⚡ Quick winAdd
MinVersion: tls.VersionTLS12to the TLS config.While
InsecureSkipVerify: trueis intentional here (to retrieve expiry info even from invalid certs), the missingMinVersionallows negotiating deprecated TLS 1.0/1.1. Adding a minimum version improves security posture without breaking the certificate-check functionality.🔒 Proposed fix
conn, err := tls.DialWithDialer(dialer, "tcp", host, &tls.Config{ InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, })🤖 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 `@modules/monitors/domain/service/scheduler.go` around lines 313 - 315, The tls.Config struct passed to the tls.DialWithDialer function call is missing a MinVersion field, which allows negotiation of insecure TLS versions 1.0 and 1.1. Add the field MinVersion: tls.VersionTLS12 to the tls.Config struct alongside the existing InsecureSkipVerify field to enforce TLS 1.2 as the minimum negotiable version, improving security without affecting the certificate expiry retrieval functionality.Source: Linters/SAST tools
web/src/views/App.Monitors.vue (1)
607-614: 💤 Low valueConsider using shadcn-vue
Checkboxcomponent for consistency.The form uses a native
<input type="checkbox">while other form controls use shadcn-vue components (Select,Input). Using@/components/ui/checkboxwould maintain visual consistency with the design system.As per coding guidelines, Vue frontend design system uses shadcn-vue components; avoid direct styling when predefined tokens exist.
🤖 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 `@web/src/views/App.Monitors.vue` around lines 607 - 614, Replace the native HTML checkbox input with id check_ssl with the shadcn-vue Checkbox component from `@/components/ui/checkbox` to maintain consistency with other form controls in the application. The Checkbox component should maintain the same v-model binding to formCheckSsl and preserve the surrounding Label element structure with the Shield icon and spacing. Remove the inline class styling from the native input element as the Checkbox component will handle styling according to the design system.Source: Coding guidelines
modules/maintenances/dto/response/maintenance_response.go (1)
26-31: ⚡ Quick winAvoid server-local timezone conversion in API DTO mapping.
Line 26–31 convert times with
.Local(), which makes API payloads depend on server timezone and DST settings. Prefer returning canonical UTC timestamps and letting clients localize for display.Suggested diff
func FromEntity(m *entity.Maintenance) *MaintenanceResponse { return &MaintenanceResponse{ ID: m.ID, Name: m.Name, Description: m.Description, - StartAt: m.StartAt.Local(), - EndAt: m.EndAt.Local(), + StartAt: m.StartAt.UTC(), + EndAt: m.EndAt.UTC(), Status: m.Status, UserID: m.UserID, - CreatedAt: m.CreatedAt.Local(), - UpdatedAt: m.UpdatedAt.Local(), + CreatedAt: m.CreatedAt.UTC(), + UpdatedAt: m.UpdatedAt.UTC(), } }🤖 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 `@modules/maintenances/dto/response/maintenance_response.go` around lines 26 - 31, The DTO mapping in the maintenance response is converting timestamps to server local timezone using .Local() on the StartAt, EndAt, CreatedAt, and UpdatedAt fields. This creates timezone and DST dependencies in the API payload. Remove the .Local() method calls from all four timestamp fields to return canonical UTC timestamps instead, allowing clients to handle any necessary timezone conversions on their side.
🤖 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 `@modules/audit_logs/domain/entity/audit_log.go`:
- Around line 7-12: The AuditLog entity struct only has an index on the UserID
field, but the repository performs queries filtering by (entity_type, entity_id)
and ordering by created_at, which will result in table scans without proper
indexes. Add GORM index tags to the EntityType and EntityID fields to create a
composite index for the common query pattern, and add an index tag to the
CreatedAt field to support the sorting operations. These indexes should be added
to the struct tags in the AuditLog entity definition.
In `@modules/audit_logs/handler/audit_log_handler.go`:
- Around line 27-34: The getAll method in AuditLogHandler currently fetches all
audit logs without pagination support, causing older records to become
unreachable once repository limits are hit. Modify the getAll method to extract
pagination parameters (page and limit, or cursor-based parameters) from the
echo.Context request, then update the service call to h.svc.GetAll(ctx) to
accept and pass these pagination parameters through to the repository layer.
Ensure the response includes pagination metadata so clients can navigate through
the full audit log history.
- Around line 85-90: The entity ID extraction logic and error handling need
improvement. The current code only checks if payloadMap["id"] is a float64,
which causes entityID to default to 0 when the ID is of a different type (int,
uint, string, json.Number, etc.). Replace the simple type assertion with a type
switch statement that handles all possible ID types and safely coerces them to
uint. Additionally, the svc.Log(ctx, 0, action, entityType, entityID, details)
call currently ignores any returned errors, which means log write failures
silently fail. Capture the error returned by svc.Log() and either log it or
return it appropriately to ensure visibility of write failures.
In `@modules/maintenances/domain/repository/maintenance_repository_impl.go`:
- Around line 16-19: The Delete method in MaintenanceRepositoryImpl performs two
separate database delete operations without a transaction, which can leave the
database in an inconsistent state if the second delete fails. Additionally, the
first delete operation result is not checked for errors. Wrap both delete
operations (the one for MaintenanceMonitor and the one for Maintenance) in a
single database transaction using database.DB.BeginTx, ensure both operations
are executed within the transaction, and return any error that occurs. Apply the
same transaction pattern to the SetMonitorIDs method (around lines 54-59) which
currently silently drops database errors by always returning nil, ensuring that
any errors from database operations are properly captured and returned instead
of being ignored.
In `@modules/maintenances/handler/maintenance_handler.go`:
- Around line 76-95: The GetByID, Update, and Delete methods in
MaintenanceHandler only verify that a user is authenticated via getAuthUser, but
fail to enforce ownership checks before allowing access to maintenance records.
After retrieving the maintenance record with the service GetByID call, add a
check to verify that the current user's ID matches the maintenance record's
UserID field, or that the user has admin role, and return an appropriate error
response if the check fails. Apply this same ownership/admin verification
pattern consistently across all three methods (GetByID, Update, and Delete) to
prevent unauthorized access or modification of other users' maintenance records.
- Around line 113-120: After parsing both startAt and endAt using time.Parse
with RFC3339 format in the maintenance handler, add validation to ensure endAt
is chronologically after startAt. If endAt is not greater than startAt, return
h.r.BadRequestResponse with an error message indicating that end_at must be
after start_at. This validation should be added after both time.Parse calls
succeed and before any further processing. Apply this same validation logic to
both occurrences mentioned (around lines 113-120 and 180-187) to ensure
consistent validation across Create/Update operations.
- Around line 69-70: The code is ignoring errors returned by GetMonitorIDs and
SetMonitorIDs method calls, which causes the handler to return successful
responses even when monitor association operations fail, resulting in missing or
stale data. Replace the blank identifier underscore with a proper error variable
for all four error-ignoring assignments (at line 69 with GetMonitorIDs, line 94,
line 147, and line 208), then add error handling logic to check if the returned
error is not nil and return an appropriate error response instead of continuing
with incomplete monitor association data.
In `@modules/monitors/domain/entity/monitor.go`:
- Around line 46-51: The rand.Read call in the heartbeat token generation block
(within the monitorType == "heartbeat" conditional) does not check its error
return value, which could result in weak tokens if the system entropy source
fails. Check the error returned by rand.Read and handle it appropriately by
either returning an error from the NewMonitor function or using an alternative
error handling strategy, ensuring that weak or unpredictable token generation
does not silently proceed.
- Line 22: Change the HeartbeatToken field in the Monitor entity from a string
type to a nullable pointer type (*string) to allow NULL values which are
excluded from unique index constraints. Then update the NewMonitor function to
assign the heartbeat token as a pointer value for heartbeat monitor types
(setting it to &hexToken or similar) while leaving it nil for non-heartbeat
types like http and ping monitors, ensuring multiple non-heartbeat monitors can
be created without violating the unique constraint.
In `@modules/monitors/domain/service/scheduler.go`:
- Around line 66-79: The heartbeat monitor case returns early when LastCheckedAt
is nil, preventing newly created monitors from getting an initial status update
or check record. Instead of returning when LastCheckedAt is nil, handle this
initial state by setting the success, errMsg, and latency variables to
appropriate values (such as marking it as a grace-period success or explicitly
indicating it is awaiting the first heartbeat). This ensures the monitor status
is updated and a check record is created rather than leaving the monitor in an
indefinite "unknown" state.
- Around line 302-326: The checkSSLCert function extracts the host from the URL
but tls.DialWithDialer requires the host in "host:port" format. When a URL like
"https://example.com" is provided without an explicit port, u.Host returns only
the hostname without the port, causing the connection to fail. After extracting
the host from the URL, check if a port is explicitly specified using u.Port().
If u.Port() returns an empty string, append ":443" (the default HTTPS port) to
the host variable before passing it to tls.DialWithDialer.
In `@modules/monitors/handler/monitor_handler.go`:
- Around line 365-367: The status check at line 365 in the monitor handler
allows any monitor with an active status to be updated via this endpoint, but
this endpoint should only work for heartbeat-type monitors. Add an additional
condition to check that mon.Type equals "heartbeat" alongside the existing
status check, and return a BadRequestResponse if the monitor type is not
heartbeat before allowing any state mutation to occur.
- Around line 374-386: The database operations in the heartbeat check handler
(starting with database.DB.WithContext(ctx).Save(&mon) through the incident
resolution loop) are not wrapped in a transaction and errors are being ignored,
which allows the function to publish events even if persistence fails. Wrap all
the database operations (the monitor save, check record creation, and incident
resolution within the loop) in a database transaction using
database.DB.WithContext(ctx).BeginTx(), check error return values from each
database operation (Save, Create, Find, and the nested Save), and only publish
the events (both the "monitor.checked" and "incident.resolved" events) after the
transaction is successfully committed. If any error occurs during the
transaction, the transaction should be rolled back and the error returned.
In `@modules/subscribers/domain/service/subscriber_service.go`:
- Around line 25-33: The Subscribe method treats any error from
s.repo.FindByEmailAndPage as "not found" and proceeds to create a new
subscriber, which masks actual database failures. You need to differentiate
between a legitimate "not found" error and unexpected repository errors by
checking if the error is specifically a not-found error (likely defined as a
constant or sentinel value in the repository). If the error is not a not-found
error, return it immediately instead of continuing. Apply this same not-found vs
unexpected-error differentiation to the Verify and Unsubscribe methods as well,
where currently all repository errors are being mapped to ErrNotFound. This
ensures that actual database failures are properly surfaced instead of being
hidden behind client-facing errors.
- Around line 35-37: The rand.Read function call used to generate the
verification token does not check for errors. Capture the error return value
from rand.Read(token) and implement proper error handling. If rand.Read fails
(which can happen due to RNG issues), the function should return an error
instead of proceeding with potentially invalid token bytes. This will ensure
verification token generation fails safely rather than creating weak or
unpredictable tokens.
In `@modules/subscribers/handler/subscriber_handler.go`:
- Around line 72-83: The Unsubscribe method in SubscriberHandler is vulnerable
to unauthorized subscriber removal because it allows unsubscribing based only on
email and status_page_id, which are easily guessable. Modify the
unsubscribeRequest struct to accept a secret unsubscribe token instead of email
and status_page_id fields. Update the service call h.svc.Unsubscribe to perform
unsubscribe validation and removal based on the token parameter rather than the
email+page pair. Apply the same token-based approach to the other vulnerable
endpoint mentioned at lines 102-107 to ensure consistent security across all
unsubscribe operations.
In `@web/src/components/maintenance-table.vue`:
- Around line 64-69: The icon-only Button components for edit and delete actions
lack accessible names for screen readers. Add aria-label attributes to both the
edit Button and delete Button to provide descriptive labels that identify their
respective actions to assistive technologies. For example, the edit button
should have an appropriate label like "Edit item" and the delete button should
have a label like "Delete item".
In `@web/src/views/App.Maintenances.vue`:
- Around line 161-201: The maintenance form dialog is using a raw HTML form
element with manual v-model bindings to formName, formDesc, formStart, formEnd,
and formMonitorIDs instead of following the project's standard vee-validate form
structure. Refactor this entire form section to replace the raw `<form>` element
with the `<Form>` component and convert each field (the Input elements for
mname, mdesc, mstart, mend) to use `<FormField>` components. Create or use an
existing Zod validation schema from the `@/validations` directory that defines the
required fields and their validation rules, then integrate it with the Form
component's schema prop. Update the handleSubmit method and form state
management to work with the vee-validate form's values and validation methods
instead of direct v-model binding.
- Around line 48-49: The issue is that slicing RFC3339 timestamp strings in the
formStart.value and formEnd.value assignments removes timezone information and
seconds, causing the timestamps to lose their offset data. When these sliced
values are later re-serialized for submission (lines 65-66), they can result in
shifted or incorrect timestamps being persisted. Instead of slicing the RFC3339
strings to extract only the first 16 characters, preserve the complete RFC3339
format or properly parse and reconstruct the timestamps while maintaining their
timezone offset information. This ensures that when the form data is submitted,
the original timestamp offsets are retained and not corrupted by the round-trip
through the form fields.
---
Outside diff comments:
In `@web/src/views/App.Monitors.vue`:
- Around line 573-590: The URL input field with v-model="formUrl" is always
displayed regardless of the monitor type, but heartbeat monitors don't require a
URL input. Add a conditional v-if directive to the div containing the Label
"Target URL" and the Input with id="url" to hide this field when formType equals
"heartbeat". Replace it with an alternative section (using v-else) that explains
heartbeat monitors use a generated token instead, providing users with clear
guidance on how heartbeat monitoring works in your system.
---
Minor comments:
In `@modules/monitors/domain/service/scheduler.go`:
- Around line 112-118: The database query checking maintenance windows calls
time.Now() twice in the WHERE clause, which could result in different timestamps
if the system clock ticks between calls. Before executing the database query
with the Joins and Where methods, capture time.Now() once into a variable named
something like now, then use that variable in both places in the WHERE clause
where it currently calls time.Now() for the start_at and end_at comparisons.
In `@modules/subscribers/handler/subscriber_handler.go`:
- Around line 49-51: The verifyQuery struct type is never used anywhere in the
codebase and should be removed to keep the code clean. Delete the entire
verifyQuery type definition (the struct with the Token field tagged with json
and query tags) from the subscriber_handler.go file.
- Around line 74-79: The unsubscribe handler in subscriber_handler.go binds the
request data but does not validate it, causing malformed payloads to be
mishandled. After the existing c.Bind(req) call that handles binding errors, add
a validation step by calling c.Validate(req) and handle any validation errors by
returning a BadRequestResponse similar to the binding error handling, ensuring
that invalid input is properly rejected with a 400 status code before the
h.svc.Unsubscribe service method is invoked.
In `@web/src/components/monitor-table.vue`:
- Around line 52-60: The daysUntilExpiryNum function uses Math.ceil to round the
day difference, which incorrectly converts small negative values (recently
expired certs) to 0, causing them to display as "Today" instead of "Expired".
Replace Math.ceil with Math.floor in the daysUntilExpiryNum function so that
negative differences remain negative and are properly handled by the
daysUntilExpiry function's logic checks.
In `@web/src/views/App.AuditLogs.vue`:
- Around line 16-21: The switch statement in the action badge mapping uses
past-tense verb cases (created, updated, deleted) but the backend audit entity
contract uses present-tense verbs (create, update, delete), causing common
events to incorrectly fall back to the default slate color. Update the case
statements in the switch block to match the backend action values by changing
'created' to 'create', 'updated' to 'update', and 'deleted' to 'delete' while
preserving their corresponding color styles.
---
Nitpick comments:
In `@modules/maintenances/dto/response/maintenance_response.go`:
- Around line 26-31: The DTO mapping in the maintenance response is converting
timestamps to server local timezone using .Local() on the StartAt, EndAt,
CreatedAt, and UpdatedAt fields. This creates timezone and DST dependencies in
the API payload. Remove the .Local() method calls from all four timestamp fields
to return canonical UTC timestamps instead, allowing clients to handle any
necessary timezone conversions on their side.
In `@modules/monitors/domain/service/scheduler.go`:
- Around line 313-315: The tls.Config struct passed to the tls.DialWithDialer
function call is missing a MinVersion field, which allows negotiation of
insecure TLS versions 1.0 and 1.1. Add the field MinVersion: tls.VersionTLS12 to
the tls.Config struct alongside the existing InsecureSkipVerify field to enforce
TLS 1.2 as the minimum negotiable version, improving security without affecting
the certificate expiry retrieval functionality.
In `@web/src/views/App.Monitors.vue`:
- Around line 607-614: Replace the native HTML checkbox input with id check_ssl
with the shadcn-vue Checkbox component from `@/components/ui/checkbox` to maintain
consistency with other form controls in the application. The Checkbox component
should maintain the same v-model binding to formCheckSsl and preserve the
surrounding Label element structure with the Shield icon and spacing. Remove the
inline class styling from the native input element as the Checkbox component
will handle styling according to the design system.
🪄 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: d1089548-57d2-4359-92b6-8ccc292d84f6
📒 Files selected for processing (40)
main.gomodules/audit_logs/domain/entity/audit_log.gomodules/audit_logs/domain/repository/audit_log_repository.gomodules/audit_logs/domain/repository/audit_log_repository_impl.gomodules/audit_logs/domain/service/audit_log_service.gomodules/audit_logs/handler/audit_log_handler.gomodules/audit_logs/module.gomodules/maintenances/domain/entity/maintenance.gomodules/maintenances/domain/repository/maintenance_repository.gomodules/maintenances/domain/repository/maintenance_repository_impl.gomodules/maintenances/domain/service/maintenance_service.gomodules/maintenances/dto/request/maintenance_request.gomodules/maintenances/dto/response/maintenance_response.gomodules/maintenances/handler/maintenance_handler.gomodules/maintenances/module.gomodules/monitors/domain/entity/monitor.gomodules/monitors/domain/service/scheduler.gomodules/monitors/dto/request/monitor_request.gomodules/monitors/dto/response/monitor_response.gomodules/monitors/handler/monitor_handler.gomodules/status_pages/handler/status_page_handler.gomodules/subscribers/domain/entity/subscriber.gomodules/subscribers/domain/repository/subscriber_repository.gomodules/subscribers/domain/repository/subscriber_repository_impl.gomodules/subscribers/domain/service/subscriber_service.gomodules/subscribers/handler/subscriber_handler.gomodules/subscribers/module.goweb/src/components/maintenance-table.vueweb/src/components/monitor-table.vueweb/src/components/status-page-table.vueweb/src/composables/useAuditLogs.tsweb/src/composables/useMaintenances.tsweb/src/content/sidebar.tsweb/src/router/index.tsweb/src/stores/auditLogs.tsweb/src/stores/maintenances.tsweb/src/stores/monitors.tsweb/src/views/App.AuditLogs.vueweb/src/views/App.Maintenances.vueweb/src/views/App.Monitors.vue
Comment on lines
+27
to
+34
| func (h *AuditLogHandler) getAll(c echo.Context) error { | ||
| ctx := c.Request().Context() | ||
| items, err := h.svc.GetAll(ctx) | ||
| if err != nil { | ||
| return h.r.InternalServerErrorResponse(c, err.Error()) | ||
| } | ||
| return h.r.SuccessResponse(c, items, "Audit logs retrieved") | ||
| } |
There was a problem hiding this comment.
GET /audit-logs is not paginated, so older records become unreachable after the repository caps are hit.
This handler exposes only “fetch all”, while repository methods apply fixed limits. That prevents a real paginated admin view and silently truncates history as data grows. Please add page/limit (or cursor) parameters and thread them through service/repo.
🤖 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 `@modules/audit_logs/handler/audit_log_handler.go` around lines 27 - 34, The
getAll method in AuditLogHandler currently fetches all audit logs without
pagination support, causing older records to become unreachable once repository
limits are hit. Modify the getAll method to extract pagination parameters (page
and limit, or cursor-based parameters) from the echo.Context request, then
update the service call to h.svc.GetAll(ctx) to accept and pass these pagination
parameters through to the repository layer. Ensure the response includes
pagination metadata so clients can navigate through the full audit log history.
Comment on lines
+85
to
+90
| entityID := uint(0) | ||
| if id, ok := payloadMap["id"].(float64); ok { | ||
| entityID = uint(id) | ||
| } | ||
|
|
||
| svc.Log(ctx, 0, action, entityType, entityID, details) |
There was a problem hiding this comment.
Entity ID extraction is too narrow and log-write errors are silently discarded.
payloadMap["id"] is only handled as float64; when payload maps carry int/uint/string/json.Number, entityID remains 0, breaking entity-level lookup integrity. Also, svc.Log(...) errors are ignored, so failed writes vanish silently. Please add a type switch for ID coercion and handle/log svc.Log errors.
🤖 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 `@modules/audit_logs/handler/audit_log_handler.go` around lines 85 - 90, The
entity ID extraction logic and error handling need improvement. The current code
only checks if payloadMap["id"] is a float64, which causes entityID to default
to 0 when the ID is of a different type (int, uint, string, json.Number, etc.).
Replace the simple type assertion with a type switch statement that handles all
possible ID types and safely coerces them to uint. Additionally, the
svc.Log(ctx, 0, action, entityType, entityID, details) call currently ignores
any returned errors, which means log write failures silently fail. Capture the
error returned by svc.Log() and either log it or return it appropriately to
ensure visibility of write failures.
Comment on lines
+69
to
+70
| monitorIDs, _ := h.svc.GetMonitorIDs(ctx, m.ID) | ||
| resp[i].MonitorIDs = monitorIDs |
There was a problem hiding this comment.
Do not ignore monitor-association service errors.
Lines 69, 94, 147, and 208 drop errors from GetMonitorIDs/SetMonitorIDs. This can return success responses with missing or stale monitor associations.
Suggested diff (example)
- monitorIDs, _ := h.svc.GetMonitorIDs(ctx, m.ID)
+ monitorIDs, err := h.svc.GetMonitorIDs(ctx, m.ID)
+ if err != nil {
+ return h.r.InternalServerErrorResponse(c, err.Error())
+ }
resp[i].MonitorIDs = monitorIDs- if len(req.MonitorIDs) > 0 {
- h.svc.SetMonitorIDs(ctx, m.ID, req.MonitorIDs)
- }
+ if len(req.MonitorIDs) > 0 {
+ if err := h.svc.SetMonitorIDs(ctx, m.ID, req.MonitorIDs); err != nil {
+ return h.r.InternalServerErrorResponse(c, err.Error())
+ }
+ }Also applies to: 94-95, 147-148, 208-208
🤖 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 `@modules/maintenances/handler/maintenance_handler.go` around lines 69 - 70,
The code is ignoring errors returned by GetMonitorIDs and SetMonitorIDs method
calls, which causes the handler to return successful responses even when monitor
association operations fail, resulting in missing or stale data. Replace the
blank identifier underscore with a proper error variable for all four
error-ignoring assignments (at line 69 with GetMonitorIDs, line 94, line 147,
and line 208), then add error handling logic to check if the returned error is
not nil and return an appropriate error response instead of continuing with
incomplete monitor association data.
Comment on lines
+161
to
+201
| <form @submit.prevent="handleSubmit" class="space-y-4 py-4"> | ||
| <div class="space-y-2"> | ||
| <Label for="mname">Name</Label> | ||
| <Input id="mname" v-model="formName" placeholder="e.g. Database migration" required /> | ||
| </div> | ||
| <div class="space-y-2"> | ||
| <Label for="mdesc">Description</Label> | ||
| <Input id="mdesc" v-model="formDesc" placeholder="Brief description" /> | ||
| </div> | ||
| <div class="grid grid-cols-2 gap-4"> | ||
| <div class="space-y-2"> | ||
| <Label for="mstart">Start</Label> | ||
| <Input id="mstart" v-model="formStart" type="datetime-local" required /> | ||
| </div> | ||
| <div class="space-y-2"> | ||
| <Label for="mend">End</Label> | ||
| <Input id="mend" v-model="formEnd" type="datetime-local" required /> | ||
| </div> | ||
| </div> | ||
| <div class="space-y-2"> | ||
| <Label>Affected Monitors</Label> | ||
| <div class="flex flex-wrap gap-2 max-h-32 overflow-y-auto border border-border/40 rounded-lg p-2"> | ||
| <button v-for="m in monitors" :key="m.id" type="button" | ||
| :class="['px-2.5 py-1 text-[11px] font-semibold rounded-full border transition-colors', | ||
| formMonitorIDs.includes(m.id) | ||
| ? 'bg-primary/10 text-primary border-primary/30' | ||
| : 'bg-muted text-muted-foreground border-border/40 hover:border-primary/30' | ||
| ]" | ||
| @click="toggleMonitor(m.id)"> | ||
| {{ m.name }} | ||
| </button> | ||
| </div> | ||
| </div> | ||
| <DialogFooter class="pt-4 border-t border-border/40"> | ||
| <Button type="button" variant="outline" @click="isFormOpen = false">Cancel</Button> | ||
| <Button type="submit" :disabled="formLoading"> | ||
| <Loader2 v-if="formLoading" class="w-4 h-4 mr-1.5 animate-spin" /> | ||
| {{ isEdit ? 'Update' : 'Create' }} | ||
| </Button> | ||
| </DialogFooter> | ||
| </form> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Use the project-standard vee-validate + Zod form structure in this dialog.
This create/edit flow is implemented with a raw <form> and manual field wiring instead of <Form>/<FormField> integrated with a schema from @/validations.
As per coding guidelines: "Vue frontend forms should be built using vee-validate <Form> / <FormField> structure integrated with Zod validation schemas (@/validations)."
🤖 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 `@web/src/views/App.Maintenances.vue` around lines 161 - 201, The maintenance
form dialog is using a raw HTML form element with manual v-model bindings to
formName, formDesc, formStart, formEnd, and formMonitorIDs instead of following
the project's standard vee-validate form structure. Refactor this entire form
section to replace the raw `<form>` element with the `<Form>` component and
convert each field (the Input elements for mname, mdesc, mstart, mend) to use
`<FormField>` components. Create or use an existing Zod validation schema from
the `@/validations` directory that defines the required fields and their
validation rules, then integrate it with the Form component's schema prop.
Update the handleSubmit method and form state management to work with the
vee-validate form's values and validation methods instead of direct v-model
binding.
Source: Coding guidelines
SidebarItem in sidebar.ts
…ndling, a11y - Add composite DB indexes for audit_log queries (entity_type+entity_id, created_at) - Handle audit Log() error instead of silent discard - Wrap maintenance repo Delete/SetMonitorIDs in GORM transactions - Handle svc.GetMonitorIDs/SetMonitorIDs errors in maintenance handler - Handle rand.Read error in subscriber service - Secure unsubscribe by token, move email+pageID behind admin auth - Add aria-labels to icon-only action buttons in maintenance-table - Preserve timezone offset in maintenance datetime-local form fields
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
1 participant
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.
Changes
6 new features:
Backend
Frontend
Summary by CodeRabbit
New Features