[tasks] implement module#6
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds a new tasks subsystem: DB migration for a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP as "Tasks Handler\n(HTTP)"
participant Service as "tasks.Service"
participant Projects as "projects.Service"
participant Repo as "tasks.Repository"
participant DB as "Database"
Client->>HTTP: POST /tasks (body + auth)
HTTP->>Service: Create(ctx, TaskInput)
Service->>Service: Validate TaskInput
Service->>Projects: Exists(project_slug)
Projects-->>Service: exists / not found
alt project exists
Service->>Repo: Create(ctx, TaskInput)
Repo->>DB: SELECT MAX(number) WHERE project_slug=...
DB-->>Repo: maxNumber
Repo->>DB: INSERT INTO tasks (...)
DB-->>Repo: inserted row
Repo-->>Service: Task
Service-->>HTTP: Task
HTTP-->>Client: 201 Created (body)
else project missing
Service-->>HTTP: ErrProjectNotFound
HTTP-->>Client: 404 Not Found
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
🤖 Pull request artifacts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/db/migrations/20260414000000_create_tasks.sql`:
- Around line 24-25: The migration declares `author_id` and `assignee_id` as
BIGINT but `users.id` is BIGINT UNSIGNED, causing FK creation to fail; update
the column definitions in this CREATE TABLE to use BIGINT UNSIGNED for both
`author_id` and `assignee_id`, and ensure the FK constraint definitions that
reference `users(id)` (the FK lines referencing `author_id`/`assignee_id`)
remain intact and match unsignedness so the foreign keys can be created
successfully.
- Line 44: Remove the stray standalone line containing only `---` from the
migration file and the other listed migration files
(internal/db/migrations/20260204013005_initial.sql,
20260413013944_create_projects.sql, 20260408120000_users.sql, and
internal/db/migrations/20260414000000_create_tasks.sql); locate the `---`
separator between the Up and Down sections in each SQL migration and delete that
line so only valid Goose directives and SQL remain (leave the existing `--
+goose Up` / `-- +goose Down` sections and SQL bodies intact).
In `@internal/tasks/domain.go`:
- Around line 154-186: TaskFilter.IncludeDeleted is ignored by TaskFilter.apply;
update apply (the TaskFilter.apply method) to respect IncludeDeleted by adding a
conditional that excludes deleted rows when IncludeDeleted is false (e.g., add
query.Where("deleted_at IS NULL") or query.Where("is_deleted = FALSE") depending
on your soft-delete column), and skip that filter when IncludeDeleted is true so
callers can opt into deleted rows; ensure you reference
TaskFilter.IncludeDeleted and modify the apply function accordingly.
- Around line 85-101: TaskInput.Validate currently trims local copies
(projectSlug, title) but doesn't update the TaskInput, causing callers (e.g.,
Service.Create) to see untrimmed values; change the method receiver from value
to pointer (func (i *TaskInput) Validate() error), assign trimmed values back to
i.ProjectSlug and i.Title before running length/empty checks (use the same
maxTitleLength constant), and keep returning the same validation errors; this
ensures callers receive normalized, trimmed fields and avoids whitespace-related
lookup/persistence bugs.
In `@internal/tasks/module.go`:
- Around line 14-20: The app startup fx.New() call is missing the tasks module
registration so the tasks repository/service (Module in package tasks) aren't
available for DI; update the BUSINESS MODULES list in the fx.New() call (where
users.Module() and projects.Module() are registered) to include tasks.Module()
so that the tasks.Module() provider functions (NewRepository, NewService) are
wired into the application graph.
In `@internal/tasks/repository.go`:
- Around line 106-107: List and Count are using different filter logic causing
mismatched pagination totals: List calls TaskFilter.apply but Count applies
due/created ranges inline; TaskFilter.apply (in internal/tasks/domain.go)
currently omits due/created filters. Fix by moving the due and created range
filtering into TaskFilter.apply so both List and Count use the same filtering
logic (update TaskFilter.apply to include due_start/due_end and
created_start/created_end handling) and remove duplicated inline range filters
from Count so it delegates to TaskFilter.apply like List.
- Around line 28-56: The current read-max-then-insert in the RunInTx transaction
(tx.NewSelect() reading maxNumber, computing nextNumber, then
tx.NewInsert().Exec) is racy and on unique index violations incorrectly returns
ErrValidationFailed; change this to handle concurrent allocation by either (A)
implementing bounded retry logic around the insert: loop a few attempts,
re-query maxNumber and recompute nextNumber on unique-violation from
tx.NewInsert() and only return ErrValidationFailed for true validation errors
(do not map unique-violation to ErrValidationFailed), or (B) use row locking to
serialize allocation by selecting the project row with FOR UPDATE (or equivalent
in bun) before computing nextNumber so concurrent transactions block instead of
conflict; update the error handling around tx.NewInsert().Exec to detect
db.IsUniqueViolation(insErr) and either trigger a retry attempt or propagate a
retryable error rather than ErrValidationFailed, referencing r.db.RunInTx,
tx.NewSelect(), nextNumber, tx.NewInsert(), and ErrValidationFailed (and the
idx_tasks_project_number unique index) when modifying the logic.
- Around line 118-121: The "-priority" sort branch in
internal/tasks/repository.go currently uses query.OrderExpr("FIELD(priority,
'Blocker', 'Critical', 'Major', 'Minor', 'Trivial') DESC") which yields the same
ordering as the positive "priority" branch; change the comparator to ASC so the
two are opposites. Locate the sort handling (the block checking sort ==
"-priority" and the else branch) and replace the DESC for the '-priority' case
with ASC to invert the order while keeping the other branch unchanged.
In `@internal/tasks/service.go`:
- Around line 98-104: The Update() path currently only validates
Priority/Status; extend its validation to mirror create rules by checking
mutable fields on the incoming update: verify Title (when update.Title != nil)
is trimmed, non-empty and within the allowed length, and verify AssigneeID (when
update.AssigneeID != nil) is positive; when any check fails return
ErrValidationFailed via fmt.Errorf like the existing checks. Locate the update
handling in Update() (references: update variable, Update(),
ErrValidationFailed) and add the Title and AssigneeID checks alongside the
Priority/Status validations so updates cannot write invalid state.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8bc17480-8800-4ea2-8b06-a4dc232c8fbb
📒 Files selected for processing (9)
internal/db/migrations/20260414000000_create_tasks.sqlinternal/tasks/config.gointernal/tasks/doc.gointernal/tasks/domain.gointernal/tasks/errors.gointernal/tasks/models.gointernal/tasks/module.gointernal/tasks/repository.gointernal/tasks/service.go
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
internal/server/tasks/handler.go (1)
255-255: Replace magic number with the defined constant.The hardcoded
100should usetasks.MaxLimitfor consistency with the pagination logic ininternal/tasks/service.go.♻️ Suggested fix
// Fetch tasks from service - taskList, total, err := h.tasksSvc.List(c.Context(), filter, "-created_at", 100, 0) + taskList, total, err := h.tasksSvc.List(c.Context(), filter, "-created_at", tasks.MaxLimit, 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/handler.go` at line 255, The call to h.tasksSvc.List is using a magic number 100 for the page limit; replace that literal with the defined constant tasks.MaxLimit so pagination is consistent. Edit the invocation of h.tasksSvc.List (in handler.go) to pass tasks.MaxLimit instead of 100 and ensure the tasks package is imported/available in this file if not already.internal/tasks/service.go (2)
76-88: Consider parallelizing List and Count queries.The comment at line 76 mentions parallel execution, but the implementation runs queries sequentially. For better performance under load, consider using goroutines with
errgroup:♻️ Optional parallel implementation
+import "golang.org/x/sync/errgroup" + func (s *Service) List(ctx context.Context, filter TaskFilter, sort string, limit, offset int) ([]Task, int64, error) { // ... pagination defaults ... - // Get tasks and total count in parallel (or separate calls) - tasks, err := s.tasks.List(ctx, filter, sort, limit, offset) - if err != nil { - return nil, 0, err - } - - total, err := s.tasks.Count(ctx, filter) - if err != nil { - return nil, 0, err + var tasks []Task + var total int64 + + g, ctx := errgroup.WithContext(ctx) + g.Go(func() error { + var err error + tasks, err = s.tasks.List(ctx, filter, sort, limit, offset) + return err + }) + g.Go(func() error { + var err error + total, err = s.tasks.Count(ctx, filter) + return err + }) + + if err := g.Wait(); err != nil { + return nil, 0, err } return tasks, total, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/service.go` around lines 76 - 88, The List and Count calls are executed sequentially in the method, but the comment suggests running them in parallel; change the implementation to run s.tasks.List(ctx, filter, sort, limit, offset) and s.tasks.Count(ctx, filter) concurrently using an errgroup (or sync.WaitGroup with error propagation), capturing the results into the existing tasks and total variables and ensuring both goroutines use the provided ctx and propagate the first error returned; after group.Wait() return tasks, total, err (nil on success).
17-21: Logger field is injected but never used.The
logger *zap.Loggerfield is stored on the Service struct but not used in any method. Either add logging for important operations (create, update, delete errors) or remove the unused dependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/service.go` around lines 17 - 21, The Service struct currently holds an injected logger (*zap.Logger) that is never used; either remove the logger field from Service or, preferably, add logging calls in the service methods (e.g., Create, Update, Delete or any error-prone functions on Service that interact with Repository and projects.Service) to record important events and errors. Locate the Service struct definition and its methods (references: Service, logger *zap.Logger, tasks *Repository, projects *projects.Service) and update each method to call s.logger.Error or s.logger.Info with contextual messages and the error when operations on s.tasks or s.projects fail; if you opt to remove it, delete the logger field and all constructor/injection code that sets it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/docs/docs.go`:
- Around line 1276-1286: The Swagger enum was split into "In" and "Progress"
because the struct tag for the status field in TaskUpdateRequest contains a
comma-separated list that split the multi-word value; update the status tag in
internal/server/tasks/dto.go (the TaskUpdateRequest struct) to include the full
"In Progress" value (e.g. enum:"New,Open,In Progress,Resolved,Closed,Reopened"
or the equivalent swag tag format), ensure it matches the StatusInProgress = "In
Progress" constant, then re-run swag/swaggo to regenerate
internal/server/docs/docs.go so the generated Swagger uses the single "In
Progress" enum value.
In `@internal/server/tasks/dto.go`:
- Around line 106-128: The functions toTaskResponse and toTaskListResponse are
dead code and their assignee parameter is ignored; either delete both functions
to remove unused code, or refactor the handlers (which currently call
NewTaskResponse/NewTaskListResponse) to call toTaskResponse and
toTaskListResponse instead and ensure toTaskResponse actually uses the assignee
parameter (set Assignee to a populated UserBrief when assignee != nil and
populate DueDate from task.DueDate) so they perform the intended user
enrichment; update any imports/usages accordingly and remove the unused
parameters if you choose deletion.
- Line 65: The struct tag for the Status field in the DTO uses an invalid
escaped space ("In\ Progress") causing a struct-tag parse error; update
validation by removing the spaced literal from the `validate:"oneof=..."` tag
(e.g. use "InProgress" or omit the oneof) and implement mapping or a custom
validator: either map "InProgress" -> "In Progress" in toTaskUpdate() (or vice
versa) before calling TaskUpdate.Validate(), or remove the oneof from the DTO
and rely on TaskUpdate.Validate() (or add a custom validator function) to accept
the spaced value; change the `Status` tag and adjust
toTaskUpdate()/TaskUpdate.Validate() accordingly.
In `@internal/server/tasks/handler.go`:
- Around line 286-316: NewTaskResponse currently returns UserBrief entries
(Author/Assignee) with empty Email/Role/CreatedAt and the handler never calls
usersSvc to enrich them; update the handler to fetch user details and pass them
into NewTaskResponse (or replace NewTaskResponse calls with the existing
toTaskResponse that accepts user params) so Author and Assignee are populated,
or explicitly remove usersSvc/use a documented behavior if you intend to keep
responses un-enriched; specifically, locate NewTaskResponse in handler.go and
either (a) call usersSvc.GetByID for task.AuthorID and task.AssigneeID and
supply those User objects to toTaskResponse, or (b) change the handler to use
toTaskResponse which accepts user parameters, ensuring Email/Role/CreatedAt are
set before returning the TaskResponse.
- Around line 241-252: The current code sets both tasks.TaskFilter.AssigneeID
and .AuthorID, producing an AND condition; change this to produce an OR: perform
two queries using the existing task list function (e.g., call the tasks listing
method twice) — once with a filter that sets AssigneeID to user.ID and once with
a filter that sets AuthorID to user.ID — then merge the results deduplicating by
task ID; alternatively, if you prefer a single-query change, extend TaskFilter
(e.g., add AssigneeOrAuthorID or a boolean flag) and update the underlying query
builder to emit SQL with OR between assignee and author checks.
- Around line 57-59: The generated docs.go documents the /tasks/{id} endpoint as
PUT instead of PATCH; regenerate the API docs from the Swagger annotations so
the documented HTTP method matches the handler's PATCH implementation (ensure
the `@Router` /tasks/{id} [patch] annotation is used), update docs.go to include
the PATCH operation for /tasks/{id} (not PUT), and verify the generated file
reflects the tasks.Patch route and h.patch handler signature.
In `@internal/tasks/domain.go`:
- Around line 144-155: TaskUpdate.Validate trims and validates the Title into a
local variable but never writes the trimmed value back, so the cleaned title is
discarded; update TaskUpdate.Validate to assign the trimmed title back to
*u.Title after validation (same approach used in TaskInput.Validate), ensuring
you reference the Title field and TaskUpdate.Validate function and preserve the
validation checks (empty and MaxTitleLength) before setting the final value.
- Around line 104-107: The DTO-to-domain conversion should set a default
priority when TaskCreateRequest.Priority is nil to avoid failing the domain
validation in i.Priority.IsValid(); modify the toTaskInput() conversion so it
assigns PriorityMinor by default and only casts when req.Priority != nil (e.g.,
create a local variable priority := tasks.PriorityMinor; if req.Priority != nil
{ priority = tasks.Priority(*req.Priority) } and use that value for the domain
input), ensuring TaskCreateRequest.Priority optionality aligns with domain
validation.
In `@internal/tasks/repository.go`:
- Around line 207-213: The RowsAffected() check after executing the update (the
block using result.RowsAffected(), err handling, and ErrNotFound) is unreliable
for MySQL because it treats no-op updates as zero affected rows; change the
update flow in internal/tasks/repository.go to match the approach used in
internal/users/repository.go: either perform a pre-flight EXISTS check for the
task ID before executing the UPDATE (so you can return ErrNotFound based on the
EXISTS result), or remove the RowsAffected() -> ErrNotFound branch and accept
silent no-op updates; do not rely on result.RowsAffected() to decide ErrNotFound
unless you enable clientFoundRows=true in the DSN. Ensure you update or remove
the code referencing result.RowsAffected(), the error variable from
RowsAffected(), and the ErrNotFound return accordingly.
- Around line 228-234: The DELETE/soft-delete path is incorrectly treating
RowsAffected() == 0 as ErrNotFound even when the UPDATE matched a row but made
no change (already soft-deleted); update the soft-delete UPDATE statement in the
repository method that calls result.RowsAffected() (the code using
result.RowsAffected(), ErrNotFound and the deleted_at column) to include "WHERE
deleted_at IS NULL" (or equivalent condition) so the UPDATE only targets
non-deleted rows, preserving the RowsAffected()==0 check as a true not-found
signal. Ensure the SQL parameter list still includes any id/tenant params used
by the function and keep the existing ErrNotFound return logic unchanged.
---
Nitpick comments:
In `@internal/server/tasks/handler.go`:
- Line 255: The call to h.tasksSvc.List is using a magic number 100 for the page
limit; replace that literal with the defined constant tasks.MaxLimit so
pagination is consistent. Edit the invocation of h.tasksSvc.List (in handler.go)
to pass tasks.MaxLimit instead of 100 and ensure the tasks package is
imported/available in this file if not already.
In `@internal/tasks/service.go`:
- Around line 76-88: The List and Count calls are executed sequentially in the
method, but the comment suggests running them in parallel; change the
implementation to run s.tasks.List(ctx, filter, sort, limit, offset) and
s.tasks.Count(ctx, filter) concurrently using an errgroup (or sync.WaitGroup
with error propagation), capturing the results into the existing tasks and total
variables and ensuring both goroutines use the provided ctx and propagate the
first error returned; after group.Wait() return tasks, total, err (nil on
success).
- Around line 17-21: The Service struct currently holds an injected logger
(*zap.Logger) that is never used; either remove the logger field from Service
or, preferably, add logging calls in the service methods (e.g., Create, Update,
Delete or any error-prone functions on Service that interact with Repository and
projects.Service) to record important events and errors. Locate the Service
struct definition and its methods (references: Service, logger *zap.Logger,
tasks *Repository, projects *projects.Service) and update each method to call
s.logger.Error or s.logger.Info with contextual messages and the error when
operations on s.tasks or s.projects fail; if you opt to remove it, delete the
logger field and all constructor/injection code that sets it.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d28edef9-3671-4386-a916-3ad94ce90515
📒 Files selected for processing (14)
internal/app.gointernal/db/migrations/20260414000000_create_tasks.sqlinternal/server/docs/docs.gointernal/server/dto/query.gointernal/server/module.gointernal/server/projects/dto.gointernal/server/projects/handler.gointernal/server/tasks/dto.gointernal/server/tasks/handler.gointernal/tasks/domain.gointernal/tasks/models.gointernal/tasks/repository.gointernal/tasks/service.gorequests.http
💤 Files with no reviewable changes (1)
- internal/server/projects/dto.go
✅ Files skipped from review due to trivial changes (5)
- internal/app.go
- requests.http
- internal/server/dto/query.go
- internal/db/migrations/20260414000000_create_tasks.sql
- internal/tasks/models.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
internal/tasks/repository.go (3)
208-221:⚠️ Potential issue | 🟠 MajorRestrict soft-delete to live rows before checking
RowsAffected().Without
WHERE deleted_at IS NULL, a repeated DELETE can keep updating the same row and still return success. Add the predicate so already-deleted tasks behave like missing tasks and theRowsAffected()==0branch stays meaningful.🐛 Minimal fix
result, err := r.db.NewUpdate().Model((*taskModel)(nil)). Set("deleted_at = NOW()"). Where("id = ?", id). + Where("deleted_at IS NULL"). Exec(ctx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/repository.go` around lines 208 - 221, The soft-delete update currently sets deleted_at unconditionally which allows repeated deletes to succeed; update the query built from r.db.NewUpdate().Model((*taskModel)(nil)) in the delete method to include a predicate restricting to live rows (e.g., add Where("deleted_at IS NULL") alongside Where("id = ?", id)) so that already-deleted tasks produce RowsAffected() == 0 and return ErrNotFound as intended.
190-200:⚠️ Potential issue | 🟠 MajorDon't use
RowsAffected()==0to detect “not found” inUpdate.On MySQL, a matched row with unchanged values also reports
0here, so a no-op PATCH can incorrectly come back asErrNotFound. Use a preflight existence check,clientFoundRows=true, or accept silent no-op updates instead. Based on learnings: In thebit-issues/backendrepository (MySQL),RowsAffected()returns 0 both when no row matches the WHERE clause AND when a matching row's values are unchanged by the UPDATE.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/repository.go` around lines 190 - 200, The Update code currently treats result.RowsAffected()==0 as ErrNotFound which is incorrect for MySQL because an UPDATE that matches but changes no values returns 0; instead perform a preflight existence check before executing the UPDATE: run a simple SELECT (e.g. SELECT 1 FROM tasks WHERE id = ?) or use clientFoundRows=true, and only return ErrNotFound if that SELECT shows no row; remove or stop relying on the result.RowsAffected() == 0 check around query.Exec/result.RowsAffected and keep using ErrNotFound only from the explicit existence check (referencing query.Exec, result.RowsAffected, and ErrNotFound in repository.go).
28-56:⚠️ Potential issue | 🟠 MajorSerialize per-project task-number allocation.
This is still a read-max-then-insert flow. Two concurrent creates for the same project can compute the same
nextNumber, and one request will fail on the unique index even though the input is valid. The transaction does not remove that race by itself.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/repository.go` around lines 28 - 56, The current RunInTx block computes nextNumber by reading the max task number then inserting, which allows two concurrent transactions to compute the same nextNumber; fix by serializing per-project allocation inside the transaction—acquire a FOR UPDATE lock on the project row (e.g. select the project with tx.NewSelect().Model((*projectModel)(nil)).Where("slug = ?", input.ProjectSlug).For("FOR UPDATE").Scan(ctx, &proj)) before running the tx.NewSelect() that reads maxNumber (or alternatively maintain and update a per-project counter row with FOR UPDATE), then compute nextNumber, call newTaskModel and tx.NewInsert as before so concurrent creators serialize on the locked project row and avoid duplicate nextNumber races.internal/tasks/domain.go (2)
144-155:⚠️ Potential issue | 🟡 MinorPersist the trimmed title during updates.
Line 147 trims into a local only. A PATCH with
" title "passes validation but still stores the padded value.🐛 Minimal fix
-func (u TaskUpdate) Validate() error { +func (u *TaskUpdate) Validate() error { if u.Title != nil { - title := strings.TrimSpace(*u.Title) - if title == "" { + trimmed := strings.TrimSpace(*u.Title) + if trimmed == "" { return fmt.Errorf("%w: title is required", ErrValidationFailed) } - if len(title) > MaxTitleLength { + if len(trimmed) > MaxTitleLength { return fmt.Errorf("%w: title too long (max 255 characters)", ErrValidationFailed) } + + u.Title = &trimmed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/domain.go` around lines 144 - 155, In TaskUpdate.Validate, you currently trim the title into a local variable but never persist it back to the struct; update the function so after computing title := strings.TrimSpace(*u.Title) and validating emptiness/length, assign the trimmed string back into the pointer (e.g. *u.Title = title) so the stored value is the trimmed version; keep validations (empty check and MaxTitleLength) against the trimmed title.
87-102:⚠️ Potential issue | 🟠 MajorNormalize the actual
TaskInput, not trimmed locals.This validation trims
ProjectSlugandTitleinto locals only. Callers still keep the original values, so" proj "can fail the project lookup later and whitespace-padded data can still be persisted.🐛 Minimal fix
-func (i TaskInput) Validate() error { +func (i *TaskInput) Validate() error { // Validate project slug - projectSlug := strings.TrimSpace(i.ProjectSlug) - if projectSlug == "" { + i.ProjectSlug = strings.TrimSpace(i.ProjectSlug) + if i.ProjectSlug == "" { return fmt.Errorf("%w: project slug is required", ErrValidationFailed) } // Validate title - title := strings.TrimSpace(i.Title) - if title == "" { + i.Title = strings.TrimSpace(i.Title) + if i.Title == "" { return fmt.Errorf("%w: title is required", ErrValidationFailed) } - if len(title) > MaxTitleLength { + if len(i.Title) > MaxTitleLength { return fmt.Errorf("%w: title too long (max 255 characters)", ErrValidationFailed) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/domain.go` around lines 87 - 102, The validation currently trims into locals but doesn't modify the TaskInput, so callers still have whitespace; change the receiver of Validate from TaskInput to *TaskInput (func (i *TaskInput) Validate() error) and after trimming assign the cleaned values back (e.g., i.ProjectSlug = projectSlug and i.Title = title) before continuing/returning so the normalized fields persist for lookups and persistence.internal/server/tasks/dto.go (1)
63-68:⚠️ Potential issue | 🟠 MajorOptional
prioritycurrently behaves as required.
TaskCreateRequest.Priorityis optional, but Line 215 still converts an omitted field intotasks.Priority("").TaskInput.Validate()will reject that, so create requests withoutpriorityfail instead of using a default.🐛 Minimal fix
func (req TaskCreateRequest) toTaskInput(authorID int64) tasks.TaskInput { - priority := tasks.Priority(req.Priority) + priority := tasks.PriorityMinor + if req.Priority != "" { + priority = tasks.Priority(req.Priority) + } return tasks.TaskInput{ ProjectSlug: req.ProjectSlug, Title: req.Title, Description: req.Description,Also applies to: 214-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto.go` around lines 63 - 68, TaskCreateRequest.Priority is declared as optional but code that builds TaskInput always converts an omitted value into tasks.Priority(""), causing TaskInput.Validate() to fail; update the construction logic (the code that maps TaskCreateRequest -> TaskInput, around where tasks.Priority(...) is used) to only set the TaskInput.Priority when the incoming TaskCreateRequest.Priority is non-empty (or use a nil/optional field) so that omitted priorities are not converted to an empty enum value and the default handling can occur.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/tasks/dto.go`:
- Around line 123-150: The response mapper is dropping stored assignee_id and
due_date values: instead of hardcoding Assignee to nil or DueDate to nil,
populate Assignee when task.AssigneeID != nil (as you're already creating
assignee := &UserBrief{ID: *task.AssigneeID, ...}) and set DueDate based on
task.DueDate (e.g., if task.DueDate != nil then pointer to
task.DueDate.Format(time.RFC3339) else nil). Apply the same fix in the list
mapper (the block around lines 155-177) so both single-item and list mappers
preserve assignee and due_date in TaskResponse.
In `@internal/server/tasks/handler.go`:
- Around line 263-271: The error switch in the task HTTP handler fails to map
tasks.ErrInvalidStatus to a 400, causing status-transition errors to return 500;
add a case for errors.Is(err, tasks.ErrInvalidStatus) that returns
fiber.NewError(fiber.StatusBadRequest, err.Error()) alongside the existing
ErrValidationFailed handling in the switch (the switch handling the error
variable in internal/server/tasks/handler.go).
---
Duplicate comments:
In `@internal/server/tasks/dto.go`:
- Around line 63-68: TaskCreateRequest.Priority is declared as optional but code
that builds TaskInput always converts an omitted value into tasks.Priority(""),
causing TaskInput.Validate() to fail; update the construction logic (the code
that maps TaskCreateRequest -> TaskInput, around where tasks.Priority(...) is
used) to only set the TaskInput.Priority when the incoming
TaskCreateRequest.Priority is non-empty (or use a nil/optional field) so that
omitted priorities are not converted to an empty enum value and the default
handling can occur.
In `@internal/tasks/domain.go`:
- Around line 144-155: In TaskUpdate.Validate, you currently trim the title into
a local variable but never persist it back to the struct; update the function so
after computing title := strings.TrimSpace(*u.Title) and validating
emptiness/length, assign the trimmed string back into the pointer (e.g. *u.Title
= title) so the stored value is the trimmed version; keep validations (empty
check and MaxTitleLength) against the trimmed title.
- Around line 87-102: The validation currently trims into locals but doesn't
modify the TaskInput, so callers still have whitespace; change the receiver of
Validate from TaskInput to *TaskInput (func (i *TaskInput) Validate() error) and
after trimming assign the cleaned values back (e.g., i.ProjectSlug = projectSlug
and i.Title = title) before continuing/returning so the normalized fields
persist for lookups and persistence.
In `@internal/tasks/repository.go`:
- Around line 208-221: The soft-delete update currently sets deleted_at
unconditionally which allows repeated deletes to succeed; update the query built
from r.db.NewUpdate().Model((*taskModel)(nil)) in the delete method to include a
predicate restricting to live rows (e.g., add Where("deleted_at IS NULL")
alongside Where("id = ?", id)) so that already-deleted tasks produce
RowsAffected() == 0 and return ErrNotFound as intended.
- Around line 190-200: The Update code currently treats result.RowsAffected()==0
as ErrNotFound which is incorrect for MySQL because an UPDATE that matches but
changes no values returns 0; instead perform a preflight existence check before
executing the UPDATE: run a simple SELECT (e.g. SELECT 1 FROM tasks WHERE id =
?) or use clientFoundRows=true, and only return ErrNotFound if that SELECT shows
no row; remove or stop relying on the result.RowsAffected() == 0 check around
query.Exec/result.RowsAffected and keep using ErrNotFound only from the explicit
existence check (referencing query.Exec, result.RowsAffected, and ErrNotFound in
repository.go).
- Around line 28-56: The current RunInTx block computes nextNumber by reading
the max task number then inserting, which allows two concurrent transactions to
compute the same nextNumber; fix by serializing per-project allocation inside
the transaction—acquire a FOR UPDATE lock on the project row (e.g. select the
project with tx.NewSelect().Model((*projectModel)(nil)).Where("slug = ?",
input.ProjectSlug).For("FOR UPDATE").Scan(ctx, &proj)) before running the
tx.NewSelect() that reads maxNumber (or alternatively maintain and update a
per-project counter row with FOR UPDATE), then compute nextNumber, call
newTaskModel and tx.NewInsert as before so concurrent creators serialize on the
locked project row and avoid duplicate nextNumber races.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 53207e20-7b9a-4124-8014-a94432c3504b
📒 Files selected for processing (6)
internal/server/docs/docs.gointernal/server/tasks/dto.gointernal/server/tasks/handler.gointernal/tasks/domain.gointernal/tasks/repository.gointernal/tasks/service.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/server/docs/docs.go
- internal/tasks/service.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/server/tasks/dto.go (1)
150-150:⚠️ Potential issue | 🟠 Major
DueDatehardcoded tonil— breaks read-after-write consistency.
toTaskListResponse(line 187) correctly mapst.DueDate, buttoTaskResponsediscards it. This causes GET/POST single-task endpoints to hide the storeddue_date.- DueDate: nil, + DueDate: task.DueDate,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto.go` at line 150, The toTaskResponse function is incorrectly hardcoding DueDate to nil, causing read-after-write inconsistency; update toTaskResponse to map the stored due date (use t.DueDate) the same way toTaskListResponse does so single-task GET/POST responses include the persisted due_date—find the toTaskResponse function and replace the hardcoded DueDate: nil with the equivalent mapping/nullable conversion used in toTaskListResponse.
🧹 Nitpick comments (3)
internal/server/tasks/dto.go (3)
68-68: Missingvalidatetag for Priority field.The
enumstag is for OpenAPI documentation only. UnlikeTaskUpdateRequest.Priority(line 80), this field lacks runtime validation. While domain validation will catch invalid values, validating at the DTO layer provides earlier, clearer error messages.- Priority string `json:"priority,omitempty" enums:"Trivial,Minor,Major,Critical,Blocker"` + Priority string `json:"priority,omitempty" validate:"omitempty,oneof=Trivial Minor Major Critical Blocker" enums:"Trivial,Minor,Major,Critical,Blocker"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto.go` at line 68, Add runtime validation to the Priority DTO field by adding a validate tag that enforces allowed enum values (matching TaskUpdateRequest.Priority). Update the struct field `Priority` to include validate:"oneof=Trivial Minor Major Critical Blocker" alongside the existing json and enums tags so the DTO layer returns early, clear errors for invalid priorities.
24-38: Invalid enum values pass through without validation.
toFilter()directly casts user-supplied strings totasks.Statusandtasks.Prioritywithout checking validity. Invalid values (e.g.,?statuses=Invalid,Bogus) will propagate to the domain layer, potentially causing unexpected query behavior or requiring the repository to handle garbage values.Consider validating each value and filtering out invalid ones:
♻️ Suggested validation approach
func (q *TaskListQuery) toFilter() tasks.TaskFilter { statuses := []tasks.Status{} if q.Statuses != nil { - statuses = lo.Map( - strings.Split(*q.Statuses, ","), - func(s string, _ int) tasks.Status { return tasks.Status(s) }, - ) + for _, s := range strings.Split(*q.Statuses, ",") { + status := tasks.Status(strings.TrimSpace(s)) + if status.IsValid() { + statuses = append(statuses, status) + } + } } priorities := []tasks.Priority{} if q.Priorities != nil { - priorities = lo.Map( - strings.Split(*q.Priorities, ","), - func(s string, _ int) tasks.Priority { return tasks.Priority(s) }, - ) + for _, s := range strings.Split(*q.Priorities, ",") { + priority := tasks.Priority(strings.TrimSpace(s)) + if priority.IsValid() { + priorities = append(priorities, priority) + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto.go` around lines 24 - 38, The toFilter() function currently casts CSV strings directly into tasks.Status and tasks.Priority allowing invalid values through; update toFilter to validate each parsed string before adding it to the statuses/priorities slices (e.g., call or implement small parser/validator functions like parseStatus(string) (tasks.Status, bool) and parsePriority(string) (tasks.Priority, bool) or check against the set of allowed constants), use strings.Split to iterate and only append values that return true from the validator, and drop or ignore invalid entries so only known enum values (tasks.Status, tasks.Priority) are returned by toFilter().
19-19: Inconsistent validation:Assigneeallows 0 whileAuthorrequires min=1.If
min=0is intentional to allow filtering for unassigned tasks, consider documenting this. Otherwise, align withAuthorfor consistency since user IDs are typically positive.- Assignee *int64 `query:"assignee" validate:"omitempty,min=0"` + Assignee *int64 `query:"assignee" validate:"omitempty,min=1"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto.go` at line 19, The Assignee field's validation tag (Assignee *int64 `query:"assignee" validate:"omitempty,min=0"`) is inconsistent with Author (which uses min=1); decide whether zero is a valid sentinel for "unassigned" and then either update the validate tag to `min=1` to match Author or keep `min=0` but add an explanatory comment and documentation stating that 0 is an allowed sentinel for unassigned tasks; modify the Assignee field's validate tag or add the inline comment next to the Assignee declaration accordingly so behavior is explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/server/tasks/dto.go`:
- Line 150: The toTaskResponse function is incorrectly hardcoding DueDate to
nil, causing read-after-write inconsistency; update toTaskResponse to map the
stored due date (use t.DueDate) the same way toTaskListResponse does so
single-task GET/POST responses include the persisted due_date—find the
toTaskResponse function and replace the hardcoded DueDate: nil with the
equivalent mapping/nullable conversion used in toTaskListResponse.
---
Nitpick comments:
In `@internal/server/tasks/dto.go`:
- Line 68: Add runtime validation to the Priority DTO field by adding a validate
tag that enforces allowed enum values (matching TaskUpdateRequest.Priority).
Update the struct field `Priority` to include validate:"oneof=Trivial Minor
Major Critical Blocker" alongside the existing json and enums tags so the DTO
layer returns early, clear errors for invalid priorities.
- Around line 24-38: The toFilter() function currently casts CSV strings
directly into tasks.Status and tasks.Priority allowing invalid values through;
update toFilter to validate each parsed string before adding it to the
statuses/priorities slices (e.g., call or implement small parser/validator
functions like parseStatus(string) (tasks.Status, bool) and
parsePriority(string) (tasks.Priority, bool) or check against the set of allowed
constants), use strings.Split to iterate and only append values that return true
from the validator, and drop or ignore invalid entries so only known enum values
(tasks.Status, tasks.Priority) are returned by toFilter().
- Line 19: The Assignee field's validation tag (Assignee *int64
`query:"assignee" validate:"omitempty,min=0"`) is inconsistent with Author
(which uses min=1); decide whether zero is a valid sentinel for "unassigned" and
then either update the validate tag to `min=1` to match Author or keep `min=0`
but add an explanatory comment and documentation stating that 0 is an allowed
sentinel for unassigned tasks; modify the Assignee field's validate tag or add
the inline comment next to the Assignee declaration accordingly so behavior is
explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed62e69f-3a60-440f-9f39-51c6e8cc9321
📒 Files selected for processing (3)
internal/server/tasks/dto.gointernal/server/tasks/handler.gointernal/tasks/errors.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/server/tasks/handler.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/server/tasks/dto.go (1)
149-151:⚠️ Potential issue | 🟠 MajorSingle-item mapper still drops persisted
due_date.Line 150 hardcodes
DueDatetonil, while list mapping preserves it. This creates inconsistent read behavior across endpoints.💡 Minimal fix
- DueDate: nil, + DueDate: task.DueDate,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto.go` around lines 149 - 151, The single-item mapper currently sets DueDate to nil; change it to use the same logic as the list mapper by reading task.DueDate and formatting it when present. Replace the hardcoded DueDate: nil with code that checks if task.DueDate != nil then sets DueDate to task.DueDate.Format(time.RFC3339) (or a pointer/string equivalent used in the list mapping), otherwise leaves it nil; use the same approach as the list mapping so DueDate, CreatedAt (task.CreatedAt) and other fields are consistent.
🧹 Nitpick comments (1)
internal/server/tasks/dto.go (1)
68-68: Alignpriorityrequiredness between DTO and domain validation.Create DTO marks
priorityas optional, butTaskInput.Validate()requires a valid priority. This mismatch yields inconsistent API contract behavior.💡 Contract-aligned option (require at DTO layer)
- Priority string `json:"priority,omitempty" validate:"omitempty,oneof=Trivial Minor Major Critical Blocker"` + Priority string `json:"priority" validate:"required,oneof=Trivial Minor Major Critical Blocker"`Also applies to: 227-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto.go` at line 68, The DTO currently marks the Priority field optional while TaskInput.Validate() enforces a valid priority; make the contract consistent by requiring priority at the DTO layer: update the Priority struct tag(s) (e.g., the Priority field in the Task DTO and the similar field referenced around lines 227-234) to remove "omitempty" and use validate:"required,oneof=Trivial Minor Major Critical Blocker" (and adjust JSON tag if you want null vs absent behavior), so incoming payloads missing priority fail DTO validation rather than only in TaskInput.Validate().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/tasks/dto.go`:
- Line 82: The AssigneeID field's validator currently rejects 0
(validate:"omitempty,min=1"), preventing PATCH from using assignee_id=0 to clear
the assignee; update the validation tag on AssigneeID in the DTO (AssigneeID
*int64 `json:"assignee_id,omitempty" ...`) to allow zero — e.g. change the tag
to validate:"omitempty,min=0" (or remove the min constraint) so a non-nil 0
value is accepted and can be used to unassign.
- Around line 26-37: The CSV parsing for q.Statuses and q.Priorities currently
splits on commas but does not trim or remove empty tokens, causing invalid enum
values; update the handlers that build `statuses` and `priorities` (where you
call `strings.Split(*q.Statuses, ",")` / `strings.Split(*q.Priorities, ",")` and
then `lo.Map`) to first trim each token (strings.TrimSpace) and drop empty
tokens before converting to `tasks.Status` / `tasks.Priority` (use lo.Filter or
a two-step Map+Filter) so only non-empty, trimmed strings are turned into enums.
---
Duplicate comments:
In `@internal/server/tasks/dto.go`:
- Around line 149-151: The single-item mapper currently sets DueDate to nil;
change it to use the same logic as the list mapper by reading task.DueDate and
formatting it when present. Replace the hardcoded DueDate: nil with code that
checks if task.DueDate != nil then sets DueDate to
task.DueDate.Format(time.RFC3339) (or a pointer/string equivalent used in the
list mapping), otherwise leaves it nil; use the same approach as the list
mapping so DueDate, CreatedAt (task.CreatedAt) and other fields are consistent.
---
Nitpick comments:
In `@internal/server/tasks/dto.go`:
- Line 68: The DTO currently marks the Priority field optional while
TaskInput.Validate() enforces a valid priority; make the contract consistent by
requiring priority at the DTO layer: update the Priority struct tag(s) (e.g.,
the Priority field in the Task DTO and the similar field referenced around lines
227-234) to remove "omitempty" and use validate:"required,oneof=Trivial Minor
Major Critical Blocker" (and adjust JSON tag if you want null vs absent
behavior), so incoming payloads missing priority fail DTO validation rather than
only in TaskInput.Validate().
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 394ccb16-5d84-488f-99cf-460fc761062b
📒 Files selected for processing (1)
internal/server/tasks/dto.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
internal/tasks/repository.go (3)
209-223:⚠️ Potential issue | 🟠 MajorSoft-delete should target only non-deleted rows before
RowsAffectedcheck.Lines 209-223 update by
idonly; repeated delete attempts on already soft-deleted rows can be misclassified through the same MySQLRowsAffectedambiguity. AddWhere("deleted_at IS NULL")to make the operation semantics explicit.🐛 Minimal fix
result, err := r.db.NewUpdate().Model((*taskModel)(nil)). Set("deleted_at = NOW()"). Where("id = ?", id). + Where("deleted_at IS NULL"). Exec(ctx)Based on learnings: In the
bit-issues/backendrepository (MySQL),RowsAffected()can be 0 for matched no-op updates, so relying on it alone causes false not-found outcomes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/repository.go` around lines 209 - 223, The soft-delete update currently updates by id only and then checks result.RowsAffected(), which can return 0 for already-deleted rows; modify the update built on r.db.NewUpdate().Model((*taskModel)(nil)) to include Where("deleted_at IS NULL") (in addition to Where("id = ?", id)) so the UPDATE only targets non-deleted rows before calling RowsAffected(); keep the same error handling (fmt.Errorf and ErrNotFound) after the RowsAffected() check.
161-202:⚠️ Potential issue | 🟠 Major
RowsAffected()==0is not a safe not-found signal for MySQL updates.At Lines 196-202, a no-op update can return
0even when the row exists, which can produce falseErrNotFound. This also affects behavior for unchanged payloads.Use an existence check (
id+deleted_at IS NULL) for not-found semantics, or enableclientFoundRows=trueand adjust logic accordingly.Based on learnings: In the
bit-issues/backendrepository (MySQL),RowsAffected()returns 0 for both missing rows and unchanged updates, makingRowsAffected()==0unreliable for not-found detection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/repository.go` around lines 161 - 202, The current update path uses result.RowsAffected()==0 to return ErrNotFound, but in MySQL an update that changes no columns can also return 0, so change the logic in the update routine that builds the query (starting at r.db.NewUpdate().Model((*taskModel)(nil)).Where("id = ?", id)) to perform an explicit existence check instead of relying on RowsAffected: either query for existence first (SELECT 1 FROM tasks WHERE id = ? AND deleted_at IS NULL) and return ErrNotFound if missing, or run the update and then run a separate existence check using the id and deleted_at IS NULL before returning ErrNotFound; ensure ErrNotFound is only returned when that existence check fails and remove the RowsAffected()==0-based not-found branch.
28-55:⚠️ Potential issue | 🟠 MajorHandle concurrent number allocation without misclassifying conflicts.
Lines 28-55 still use read-max-then-insert, so concurrent creates for the same
project_slugcan collide on(project_slug, number). Mapping that unique conflict toErrValidationFailedtreats a race as client input error.Consider bounded retry on unique conflict (or row-level locking strategy) and return a retryable/conflict-class error instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/repository.go` around lines 28 - 55, The current create flow inside r.db.RunInTx uses a read-max-then-insert (tx.NewSelect(...) then tx.NewInsert()) so concurrent callers can hit the (project_slug, number) unique constraint and you map db.IsUniqueViolation to ErrValidationFailed; change this by adding a bounded retry loop around the allocation+insert: call tx.NewSelect to compute nextNumber, build model via newTaskModel, attempt tx.NewInsert, and if db.IsUniqueViolation(insErr) occurs retry the whole allocation up to N times (or escalate after N attempts) instead of returning ErrValidationFailed; when retries are exhausted return a retryable/conflict-class error (not ErrValidationFailed) so callers can distinguish race vs bad input.internal/server/tasks/dto.go (2)
135-153:⚠️ Potential issue | 🟠 Major
toTaskResponsedrops persisteddue_date.Line 150 hardcodes
DueDate: nil, while list mapping at Line 187 preserves it. This breaks consistency between single-item and list responses for the same task.🐛 Minimal fix
Assignee: assignee, - DueDate: nil, + DueDate: task.DueDate, CreatedAt: task.CreatedAt.Format(time.RFC3339), UpdatedAt: task.UpdatedAt.Format(time.RFC3339),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto.go` around lines 135 - 153, The toTaskResponse mapping currently hardcodes DueDate: nil causing single-item responses to drop persisted due_date; update the toTaskResponse function to populate TaskResponse.DueDate from task.DueDate (matching the list mapping at the other mapping) by converting the task.DueDate to the same nullable/ISO8601 string representation used elsewhere (e.g., RFC3339) or leaving it nil when task.DueDate is nil so single and list responses are consistent.
26-37:⚠️ Potential issue | 🟡 MinorNormalize CSV tokens before enum conversion.
Lines 26-37 still map raw
statuses/prioritiestokens directly. Inputs likestatuses=Open,%20Resolved,produce untrimmed/empty values and unnecessary validation failures.Trim whitespace and drop empty tokens before casting to
tasks.Status/tasks.Priority.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto.go` around lines 26 - 37, The CSV parsing for q.Statuses and q.Priorities currently maps raw tokens directly (using strings.Split and lo.Map) which allows empty or whitespace-only values; update the logic that builds statuses and priorities so you first split the CSV, then trim each token and filter out empty tokens, and only then cast to tasks.Status / tasks.Priority (i.e., adjust the code around q.Statuses, q.Priorities, strings.Split, lo.Map and the mapping to tasks.Status/tasks.Priority to include strings.TrimSpace and a non-empty filter before conversion).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/tasks/repository.go`:
- Around line 142-146: The Count method is forcing deleted_at IS NULL before
applying filters so TaskFilter.IncludeDeleted is ignored; remove the hardcoded
"deleted_at IS NULL" and let filter.apply(query) control deletion filtering (as
List does) so includeDeleted behavior is honored; locate the Count
implementation in repository.go that builds query using
r.db.NewSelect().Model((*taskModel)(nil)) and replace the pre-filter WHERE with
simply applying filter.apply(query) (or ensure filter.apply is called first) so
taskModel soft-delete logic is consistent with List.
In `@requests.http`:
- Around line 196-221: The GET request is using {{taskId}} before it's created
and the two POST requests share the same request name `@name` createTask causing
collisions; move the GET {{baseURL}}/tasks/{{taskId}} so it comes after the
first POST that sets `@taskId` from createTask.response.body.$.id, and rename the
second POST's name token from createTask to a unique identifier like
createTaskMinimal so the response variables (taskId and createTask.response) do
not overwrite each other.
---
Duplicate comments:
In `@internal/server/tasks/dto.go`:
- Around line 135-153: The toTaskResponse mapping currently hardcodes DueDate:
nil causing single-item responses to drop persisted due_date; update the
toTaskResponse function to populate TaskResponse.DueDate from task.DueDate
(matching the list mapping at the other mapping) by converting the task.DueDate
to the same nullable/ISO8601 string representation used elsewhere (e.g.,
RFC3339) or leaving it nil when task.DueDate is nil so single and list responses
are consistent.
- Around line 26-37: The CSV parsing for q.Statuses and q.Priorities currently
maps raw tokens directly (using strings.Split and lo.Map) which allows empty or
whitespace-only values; update the logic that builds statuses and priorities so
you first split the CSV, then trim each token and filter out empty tokens, and
only then cast to tasks.Status / tasks.Priority (i.e., adjust the code around
q.Statuses, q.Priorities, strings.Split, lo.Map and the mapping to
tasks.Status/tasks.Priority to include strings.TrimSpace and a non-empty filter
before conversion).
In `@internal/tasks/repository.go`:
- Around line 209-223: The soft-delete update currently updates by id only and
then checks result.RowsAffected(), which can return 0 for already-deleted rows;
modify the update built on r.db.NewUpdate().Model((*taskModel)(nil)) to include
Where("deleted_at IS NULL") (in addition to Where("id = ?", id)) so the UPDATE
only targets non-deleted rows before calling RowsAffected(); keep the same error
handling (fmt.Errorf and ErrNotFound) after the RowsAffected() check.
- Around line 161-202: The current update path uses result.RowsAffected()==0 to
return ErrNotFound, but in MySQL an update that changes no columns can also
return 0, so change the logic in the update routine that builds the query
(starting at r.db.NewUpdate().Model((*taskModel)(nil)).Where("id = ?", id)) to
perform an explicit existence check instead of relying on RowsAffected: either
query for existence first (SELECT 1 FROM tasks WHERE id = ? AND deleted_at IS
NULL) and return ErrNotFound if missing, or run the update and then run a
separate existence check using the id and deleted_at IS NULL before returning
ErrNotFound; ensure ErrNotFound is only returned when that existence check fails
and remove the RowsAffected()==0-based not-found branch.
- Around line 28-55: The current create flow inside r.db.RunInTx uses a
read-max-then-insert (tx.NewSelect(...) then tx.NewInsert()) so concurrent
callers can hit the (project_slug, number) unique constraint and you map
db.IsUniqueViolation to ErrValidationFailed; change this by adding a bounded
retry loop around the allocation+insert: call tx.NewSelect to compute
nextNumber, build model via newTaskModel, attempt tx.NewInsert, and if
db.IsUniqueViolation(insErr) occurs retry the whole allocation up to N times (or
escalate after N attempts) instead of returning ErrValidationFailed; when
retries are exhausted return a retryable/conflict-class error (not
ErrValidationFailed) so callers can distinguish race vs bad input.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d06e0414-2a0e-4076-8474-376445f1cce6
📒 Files selected for processing (3)
internal/server/tasks/dto.gointernal/tasks/repository.gorequests.http
capcom6
force-pushed
the
tasks/add-module
branch
from
f89db1c to
b179cd3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
internal/server/tasks/dto.go (3)
24-38:⚠️ Potential issue | 🟡 MinorTrim and drop empty CSV tokens before enum conversion.
Inputs like
statuses=Open,%20Resolved,currently produce" Resolved"and"", which then become invalid enum values in the filter.Suggested fix
statuses := []tasks.Status{} if q.Statuses != nil { - statuses = lo.Map( + statuses = lo.FilterMap( strings.Split(*q.Statuses, ","), - func(s string, _ int) tasks.Status { return tasks.Status(s) }, + func(s string, _ int) (tasks.Status, bool) { + s = strings.TrimSpace(s) + if s == "" { + return "", false + } + return tasks.Status(s), true + }, ) } priorities := []tasks.Priority{} if q.Priorities != nil { - priorities = lo.Map( + priorities = lo.FilterMap( strings.Split(*q.Priorities, ","), - func(s string, _ int) tasks.Priority { return tasks.Priority(s) }, + func(s string, _ int) (tasks.Priority, bool) { + s = strings.TrimSpace(s) + if s == "" { + return "", false + } + return tasks.Priority(s), true + }, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto.go` around lines 24 - 38, The toFilter conversion in TaskListQuery (function toFilter) should trim whitespace and drop empty CSV tokens before casting to enums so malformed tokens like " Resolved" or "" are not turned into invalid tasks.Status/tasks.Priority values; update the statuses and priorities parsing to Split the CSV, TrimSpace each token, filter out empty strings, then map the remaining tokens to tasks.Status and tasks.Priority (refer to variables statuses, priorities and the lo.Map usage) so only valid, trimmed enum strings are converted.
226-233:⚠️ Potential issue | 🟡 MinorUse the same default priority as the persistence model.
When the client omits
priority, this path defaults toPriorityMajor, butinternal/tasks/models.gopersistsMinoras the model/schema default. Optional input should not change behavior depending on which layer supplied the default.Suggested fix
- Priority: lo.CoalesceOrEmpty(priority, tasks.PriorityMajor), + Priority: lo.CoalesceOrEmpty(priority, tasks.PriorityMinor),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto.go` around lines 226 - 233, The toTaskInput conversion in TaskCreateRequest currently uses tasks.PriorityMajor as the fallback causing a different default than the persistence model; update the Priority fallback in func (req TaskCreateRequest) toTaskInput(authorID int64) (the tasks.TaskInput Priority field) to use the same default as the persistence layer (tasks.PriorityMinor defined in internal/tasks/models.go) so optional/omitted priorities behave identically across layers.
135-152:⚠️ Potential issue | 🟠 MajorPreserve
due_datein the single-item mapper.
GET /tasks/:id,POST /tasks, andPATCH /tasks/:idall go throughtoTaskResponse(), which still forcesDueDatetonil. The list mapper already returns it, so the API is inconsistent on read-after-write.Suggested fix
Assignee: assignee, - DueDate: nil, + DueDate: task.DueDate, CreatedAt: task.CreatedAt.Format(time.RFC3339), UpdatedAt: task.UpdatedAt.Format(time.RFC3339), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto.go` around lines 135 - 152, toTaskResponse (the single-item mapper that builds TaskResponse) is forcing DueDate to nil causing read-after-write inconsistency; update the mapper to assign TaskResponse.DueDate from task.DueDate the same way the list mapper does (e.g., set a formatted string pointer when task.DueDate is non-zero, otherwise nil) so GET /tasks/:id, POST /tasks and PATCH /tasks/:id return the actual due_date; look for the TaskResponse construction in toTaskResponse and mirror the logic used in the list mapper for setting DueDate.internal/tasks/domain.go (2)
144-155:⚠️ Potential issue | 🟡 MinorWrite the trimmed title back in
TaskUpdate.Validate().This has the same problem as create validation: the cleaned title is discarded, so updates still persist the untrimmed value.
Suggested fix
-func (u TaskUpdate) Validate() error { +func (u *TaskUpdate) Validate() error { if u.Title != nil { - title := strings.TrimSpace(*u.Title) - if title == "" { + title := strings.TrimSpace(*u.Title) + if title == "" { return fmt.Errorf("%w: title is required", ErrValidationFailed) } if len(title) > MaxTitleLength { return fmt.Errorf("%w: title too long (max 255 characters)", ErrValidationFailed) } + + u.Title = &title }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/domain.go` around lines 144 - 155, The Validate method for TaskUpdate currently trims and validates the title but doesn't store the trimmed value back, so updates can persist the original untrimmed string; modify TaskUpdate.Validate (inside the Title != nil branch) to assign the trimmed title string back to *u.Title after validation (use the same trimmed variable), keeping the existing checks against ErrValidationFailed and MaxTitleLength to ensure no behavior changes other than storing the cleaned title.
87-102:⚠️ Potential issue | 🟠 MajorNormalize the trimmed fields on
TaskInputitself.
Validate()trims into locals, butService.Createstill passes the originalProjectSlug/Titledownstream. That means whitespace-padded slugs can miss the project lookup and whitespace-padded titles still get persisted.Suggested fix
-func (i TaskInput) Validate() error { +func (i *TaskInput) Validate() error { // Validate project slug - projectSlug := strings.TrimSpace(i.ProjectSlug) - if projectSlug == "" { + i.ProjectSlug = strings.TrimSpace(i.ProjectSlug) + if i.ProjectSlug == "" { return fmt.Errorf("%w: project slug is required", ErrValidationFailed) } // Validate title - title := strings.TrimSpace(i.Title) - if title == "" { + i.Title = strings.TrimSpace(i.Title) + if i.Title == "" { return fmt.Errorf("%w: title is required", ErrValidationFailed) } - if len(title) > MaxTitleLength { + if len(i.Title) > MaxTitleLength { return fmt.Errorf("%w: title too long (max 255 characters)", ErrValidationFailed) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/domain.go` around lines 87 - 102, TaskInput.Validate currently trims into local vars but doesn't persist them because the method has a value receiver; change the signature to use a pointer receiver (func (i *TaskInput) Validate() error) and assign the trimmed values back to the struct (i.ProjectSlug = strings.TrimSpace(i.ProjectSlug); i.Title = strings.TrimSpace(i.Title)) before running the existing validations (including the MaxTitleLength check) so downstream code like Service.Create receives normalized values.internal/tasks/repository.go (2)
196-202:⚠️ Potential issue | 🟠 Major
RowsAffected()==0is not a reliable not-found check here.On this MySQL setup, a no-op update and a missing row both report
0, so unchanged PATCH requests can incorrectly returnErrNotFound. Prefer a preflight existence check or accept silent no-op updates.Suggested fix
- rows, err := result.RowsAffected() - if err != nil { - return fmt.Errorf("failed to get affected rows: %w", err) - } - if rows == 0 { - return ErrNotFound - } + rows, err := result.RowsAffected() + if err != nil { + return fmt.Errorf("failed to get affected rows: %w", err) + } + if rows == 0 { + exists, existsErr := r.db.NewSelect(). + Model((*taskModel)(nil)). + Where("id = ?", id). + Exists(ctx) + if existsErr != nil { + return fmt.Errorf("failed to verify task existence: %w", existsErr) + } + if !exists { + return ErrNotFound + } + }Based on learnings, in the
bit-issues/backendMySQL setupRowsAffected()returns0for both missing rows and unchanged updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/repository.go` around lines 196 - 202, The current check that treats result.RowsAffected() == 0 as ErrNotFound is unreliable (MySQL returns 0 for both missing rows and unchanged updates); instead, perform an explicit existence preflight before the update or accept silent no-op updates: replace the RowsAffected()==0 ErrNotFound branch in the repository update flow (the code calling result.RowsAffected() and returning ErrNotFound) with either a SELECT EXISTS/SELECT COUNT (using your entity retrieval method or an ExistsByID helper) to confirm the row exists before executing the UPDATE, or remove the ErrNotFound return and treat 0 affected rows as a successful no-op; reference result.RowsAffected(), the local variable rows, and the ErrNotFound symbol when making the change.
28-55:⚠️ Potential issue | 🟠 MajorTask-number allocation is still race-prone.
Two concurrent creates for the same project can read the same
maxNumber, compute the samenextNumber, and one request will fail on the unique index even though the input is valid. The current unique-violation mapping also misclassifies that asErrValidationFailed.Suggested direction
- nextNumber := maxNumber + 1 - model := newTaskModel(input, nextNumber) - - if _, insErr := tx.NewInsert().Model(model).Exec(ctx); insErr != nil { - if db.IsUniqueViolation(insErr) { - return fmt.Errorf( - "%w: task number %d already exists for project %s", - ErrValidationFailed, - nextNumber, - input.ProjectSlug, - ) - } - return fmt.Errorf("failed to insert task: %w", insErr) - } + // Consider either: + // 1) serializing allocation with row locking, or + // 2) retrying bounded times on unique violation before failing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/repository.go` around lines 28 - 55, The task-number allocation can race: two CREATEs can read the same maxNumber and collide; fix by serializing per-project before computing nextNumber and by treating unique-violation as transient (retry) rather than ErrValidationFailed. Inside the RunInTx block (r.db.RunInTx / the transaction closure that does tx.NewSelect and newTaskModel), acquire a project-level lock (e.g., SELECT id FROM projects WHERE slug = ? FOR UPDATE or call pg_advisory_xact_lock using a hash of input.ProjectSlug) before selecting max number, then compute nextNumber and insert; additionally wrap the whole transaction in a small retry loop (N attempts with backoff) and if tx.NewInsert returns db.IsUniqueViolation treat it as a retryable conflict (loop) instead of returning ErrValidationFailed; only after exhausting retries return a clear conflict/error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/tasks/domain.go`:
- Around line 86-119: TaskInput.Validate currently skips checking DueDate,
allowing malformed dates to reach internal/tasks/models.go where they get parsed
to NULL; update TaskInput.Validate to validate the DueDate field (e.g., if
DueDate is non-empty or non-nil attempt to parse it with the same parser/format
used in models.go and return fmt.Errorf("%w: invalid due_date",
ErrValidationFailed) on parse failure) so invalid or unparsable due_date values
are rejected early in TaskInput.Validate.
- Around line 157-169: Add a validation check for DueDate in the same update
validation function that currently checks u.Priority, u.Status and u.AssigneeID:
when u.DueDate != nil attempt to parse the string (e.g. time.Parse with the
project's expected layout such as time.RFC3339) and if parsing fails return
fmt.Errorf("%w: invalid due_date format", ErrValidationFailed); use the same
error pattern and symbols (u.DueDate, ErrValidationFailed and the existing
validation function/method) so bad PATCH payloads produce a validation error
instead of a DB error.
---
Duplicate comments:
In `@internal/server/tasks/dto.go`:
- Around line 24-38: The toFilter conversion in TaskListQuery (function
toFilter) should trim whitespace and drop empty CSV tokens before casting to
enums so malformed tokens like " Resolved" or "" are not turned into invalid
tasks.Status/tasks.Priority values; update the statuses and priorities parsing
to Split the CSV, TrimSpace each token, filter out empty strings, then map the
remaining tokens to tasks.Status and tasks.Priority (refer to variables
statuses, priorities and the lo.Map usage) so only valid, trimmed enum strings
are converted.
- Around line 226-233: The toTaskInput conversion in TaskCreateRequest currently
uses tasks.PriorityMajor as the fallback causing a different default than the
persistence model; update the Priority fallback in func (req TaskCreateRequest)
toTaskInput(authorID int64) (the tasks.TaskInput Priority field) to use the same
default as the persistence layer (tasks.PriorityMinor defined in
internal/tasks/models.go) so optional/omitted priorities behave identically
across layers.
- Around line 135-152: toTaskResponse (the single-item mapper that builds
TaskResponse) is forcing DueDate to nil causing read-after-write inconsistency;
update the mapper to assign TaskResponse.DueDate from task.DueDate the same way
the list mapper does (e.g., set a formatted string pointer when task.DueDate is
non-zero, otherwise nil) so GET /tasks/:id, POST /tasks and PATCH /tasks/:id
return the actual due_date; look for the TaskResponse construction in
toTaskResponse and mirror the logic used in the list mapper for setting DueDate.
In `@internal/tasks/domain.go`:
- Around line 144-155: The Validate method for TaskUpdate currently trims and
validates the title but doesn't store the trimmed value back, so updates can
persist the original untrimmed string; modify TaskUpdate.Validate (inside the
Title != nil branch) to assign the trimmed title string back to *u.Title after
validation (use the same trimmed variable), keeping the existing checks against
ErrValidationFailed and MaxTitleLength to ensure no behavior changes other than
storing the cleaned title.
- Around line 87-102: TaskInput.Validate currently trims into local vars but
doesn't persist them because the method has a value receiver; change the
signature to use a pointer receiver (func (i *TaskInput) Validate() error) and
assign the trimmed values back to the struct (i.ProjectSlug =
strings.TrimSpace(i.ProjectSlug); i.Title = strings.TrimSpace(i.Title)) before
running the existing validations (including the MaxTitleLength check) so
downstream code like Service.Create receives normalized values.
In `@internal/tasks/repository.go`:
- Around line 196-202: The current check that treats result.RowsAffected() == 0
as ErrNotFound is unreliable (MySQL returns 0 for both missing rows and
unchanged updates); instead, perform an explicit existence preflight before the
update or accept silent no-op updates: replace the RowsAffected()==0 ErrNotFound
branch in the repository update flow (the code calling result.RowsAffected() and
returning ErrNotFound) with either a SELECT EXISTS/SELECT COUNT (using your
entity retrieval method or an ExistsByID helper) to confirm the row exists
before executing the UPDATE, or remove the ErrNotFound return and treat 0
affected rows as a successful no-op; reference result.RowsAffected(), the local
variable rows, and the ErrNotFound symbol when making the change.
- Around line 28-55: The task-number allocation can race: two CREATEs can read
the same maxNumber and collide; fix by serializing per-project before computing
nextNumber and by treating unique-violation as transient (retry) rather than
ErrValidationFailed. Inside the RunInTx block (r.db.RunInTx / the transaction
closure that does tx.NewSelect and newTaskModel), acquire a project-level lock
(e.g., SELECT id FROM projects WHERE slug = ? FOR UPDATE or call
pg_advisory_xact_lock using a hash of input.ProjectSlug) before selecting max
number, then compute nextNumber and insert; additionally wrap the whole
transaction in a small retry loop (N attempts with backoff) and if tx.NewInsert
returns db.IsUniqueViolation treat it as a retryable conflict (loop) instead of
returning ErrValidationFailed; only after exhausting retries return a clear
conflict/error.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86101b8e-09ca-4b14-ae31-89d7d68f13f1
📒 Files selected for processing (18)
internal/app.gointernal/db/migrations/20260414000000_create_tasks.sqlinternal/server/docs/docs.gointernal/server/dto/query.gointernal/server/module.gointernal/server/projects/dto.gointernal/server/projects/handler.gointernal/server/tasks/dto.gointernal/server/tasks/handler.gointernal/tasks/config.gointernal/tasks/doc.gointernal/tasks/domain.gointernal/tasks/errors.gointernal/tasks/models.gointernal/tasks/module.gointernal/tasks/repository.gointernal/tasks/service.gorequests.http
💤 Files with no reviewable changes (1)
- internal/server/projects/dto.go
✅ Files skipped from review due to trivial changes (6)
- internal/server/projects/handler.go
- internal/tasks/errors.go
- internal/tasks/doc.go
- internal/db/migrations/20260414000000_create_tasks.sql
- internal/server/docs/docs.go
- internal/tasks/config.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/app.go
- internal/server/module.go
- requests.http
- internal/server/dto/query.go
- internal/tasks/module.go
- internal/tasks/service.go
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/tasks/domain.go (2)
150-161:⚠️ Potential issue | 🟡 MinorPersist trimmed update titles instead of discarding normalization.
TaskUpdate.Validate()validates a trimmed local title but does not write it back tou.Title.Suggested fix
-func (u TaskUpdate) Validate() error { +func (u *TaskUpdate) Validate() error { if u.Title != nil { // Trim and validate title - title := strings.TrimSpace(*u.Title) - if title == "" { + trimmed := strings.TrimSpace(*u.Title) + if trimmed == "" { return fmt.Errorf("%w: title is required", ErrValidationFailed) } - if len(title) > MaxTitleLength { + if len(trimmed) > MaxTitleLength { return fmt.Errorf("%w: title too long (max 255 characters)", ErrValidationFailed) } + u.Title = &trimmed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/domain.go` around lines 150 - 161, TaskUpdate.Validate currently trims and validates the local variable title but doesn't persist the normalized value back to u.Title; update TaskUpdate.Validate to assign the trimmed string back into u.Title (replace the pointer's value or set u.Title = &trimmed) after trimming and before returning so callers receive the normalized title, while keeping the same validation checks (ErrValidationFailed, MaxTitleLength) and error behavior in the same function.
87-102:⚠️ Potential issue | 🟠 MajorNormalize validated create fields on the actual
TaskInput.
TaskInput.Validate()trims into locals, buti.ProjectSlug/i.Titleremain untrimmed for callers after validation.Suggested fix
-func (i TaskInput) Validate() error { +func (i *TaskInput) Validate() error { // Validate project slug - projectSlug := strings.TrimSpace(i.ProjectSlug) - if projectSlug == "" { + i.ProjectSlug = strings.TrimSpace(i.ProjectSlug) + if i.ProjectSlug == "" { return fmt.Errorf("%w: project slug is required", ErrValidationFailed) } // Validate title - title := strings.TrimSpace(i.Title) - if title == "" { + i.Title = strings.TrimSpace(i.Title) + if i.Title == "" { return fmt.Errorf("%w: title is required", ErrValidationFailed) } - if len(title) > MaxTitleLength { + if len(i.Title) > MaxTitleLength { return fmt.Errorf("%w: title too long (max 255 characters)", ErrValidationFailed) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/domain.go` around lines 87 - 102, TaskInput.Validate trims input into local variables but never updates the struct fields, so callers still see untrimmed values; change the method signature from (i TaskInput) Validate() to (i *TaskInput) Validate(), assign the trimmed values back to i.ProjectSlug and i.Title (and any other validated/trimmed locals in this method), and keep returning the same validation errors—this ensures TaskInput is normalized in-place after Validate() is called.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/tasks/domain.go`:
- Around line 150-161: TaskUpdate.Validate currently trims and validates the
local variable title but doesn't persist the normalized value back to u.Title;
update TaskUpdate.Validate to assign the trimmed string back into u.Title
(replace the pointer's value or set u.Title = &trimmed) after trimming and
before returning so callers receive the normalized title, while keeping the same
validation checks (ErrValidationFailed, MaxTitleLength) and error behavior in
the same function.
- Around line 87-102: TaskInput.Validate trims input into local variables but
never updates the struct fields, so callers still see untrimmed values; change
the method signature from (i TaskInput) Validate() to (i *TaskInput) Validate(),
assign the trimmed values back to i.ProjectSlug and i.Title (and any other
validated/trimmed locals in this method), and keep returning the same validation
errors—this ensures TaskInput is normalized in-place after Validate() is called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6cbe1c07-657c-4065-9363-68ae233ccbc3
📒 Files selected for processing (3)
go.modinternal/tasks/domain.gointernal/tasks/models.go
✅ Files skipped from review due to trivial changes (2)
- go.mod
- internal/tasks/models.go
capcom6
force-pushed
the
tasks/add-module
branch
from
d34873c to
e8c03ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/server/tasks/dto.go (1)
148-150:⚠️ Potential issue | 🟠 Major
DueDatedropped in single-task response but preserved in list response.
toTaskResponse(line 150) hardcodesDueDate: nil, whiletoTaskListResponse(line 187) correctly setsDueDate: t.DueDate. This inconsistency breaks read-after-write for the single task endpoint.🐛 Suggested fix
Assignee: assignee, - DueDate: nil, + DueDate: task.DueDate, CreatedAt: task.CreatedAt.Format(time.RFC3339),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto.go` around lines 148 - 150, toTaskResponse currently sets DueDate: nil which causes the single-task response to drop the due date while toTaskListResponse uses t.DueDate; update the toTaskResponse function to populate DueDate from the task (use t.DueDate) instead of hardcoding nil so the single-task endpoint returns the same DueDate as the list endpoint (refer to symbols toTaskResponse, toTaskListResponse, DueDate, and t).
🧹 Nitpick comments (1)
internal/tasks/repository.go (1)
105-110: Priority sorting uses lexicographic order, not semantic severity order.Using
bun.OrderDesc/bun.OrderAscon theprioritycolumn sorts alphabetically (Blocker<Critical<Major<Minor<Trivial), which doesn't match the expected severity order. Consider using MySQL'sFIELD()function for semantic ordering.💡 Example using FIELD() for semantic priority ordering
case "-priority", "priority": if sort == "-priority" { - query = query.OrderBy("priority", bun.OrderDesc) + // High to low: Blocker > Critical > Major > Minor > Trivial + query = query.OrderExpr("FIELD(priority, 'Blocker', 'Critical', 'Major', 'Minor', 'Trivial') ASC") } else { - query = query.OrderBy("priority", bun.OrderAsc) + // Low to high: Trivial < Minor < Major < Critical < Blocker + query = query.OrderExpr("FIELD(priority, 'Trivial', 'Minor', 'Major', 'Critical', 'Blocker') ASC") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/repository.go` around lines 105 - 110, The current priority sorting inside the case "-priority", "priority" uses query.OrderBy("priority", bun.OrderDesc/Asc) which sorts lexicographically; replace that with a semantic ordering using MySQL's FIELD() so priorities are ordered as Blocker, Critical, Major, Minor, Trivial. Update the code in the case handling (where `query` is built) to use an ORDER BY expression (e.g., via query.OrderExpr or equivalent) that calls FIELD(priority, 'Blocker','Critical','Major','Minor','Trivial') and append DESC for "-priority" or ASC for "priority" so the sorting reflects severity rather than alphabetical order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/docs/docs.go`:
- Around line 1260-1262: The Swagger docs currently declare "assignee_id" with
"minimum": 1 which conflicts with the DTO TaskUpdateRequest that uses
validate:"omitempty,min=0" to allow unassignment via assignee_id=0; regenerate
the OpenAPI/Swagger docs (the generator that produces
internal/server/docs/docs.go) so the tasks.TaskUpdateRequest schema sets
assignee_id minimum to 0 to match the DTO validation; ensure the DTO tag
validate:"omitempty,min=0" on TaskUpdateRequest remains the canonical source and
re-run the doc generation step to update docs.go accordingly.
In `@internal/server/tasks/dto.go`:
- Line 83: The struct field DueDate has a typo in its validate tag: replace the
invalid tag value "omitzero" with "omitempty" so the go-playground/validator
will skip validation when DueDate is nil/empty; update the validate tag on the
DueDate field definition in the DTO (the DueDate *string
`json:"due_date,omitempty" validate:"..."` line) to use datetime=2006-01-02 with
omitempty.
- Line 233: The toTaskInput DTO currently sets Priority using
lo.CoalesceOrEmpty(priority, tasks.PriorityMajor) which contradicts the DB
schema default of Minor; update the default to tasks.PriorityMinor so
API-created tasks match the database default (change the fallback in toTaskInput
from PriorityMajor to PriorityMinor) and run tests to ensure no other callers
rely on Major as the implicit default.
---
Duplicate comments:
In `@internal/server/tasks/dto.go`:
- Around line 148-150: toTaskResponse currently sets DueDate: nil which causes
the single-task response to drop the due date while toTaskListResponse uses
t.DueDate; update the toTaskResponse function to populate DueDate from the task
(use t.DueDate) instead of hardcoding nil so the single-task endpoint returns
the same DueDate as the list endpoint (refer to symbols toTaskResponse,
toTaskListResponse, DueDate, and t).
---
Nitpick comments:
In `@internal/tasks/repository.go`:
- Around line 105-110: The current priority sorting inside the case "-priority",
"priority" uses query.OrderBy("priority", bun.OrderDesc/Asc) which sorts
lexicographically; replace that with a semantic ordering using MySQL's FIELD()
so priorities are ordered as Blocker, Critical, Major, Minor, Trivial. Update
the code in the case handling (where `query` is built) to use an ORDER BY
expression (e.g., via query.OrderExpr or equivalent) that calls FIELD(priority,
'Blocker','Critical','Major','Minor','Trivial') and append DESC for "-priority"
or ASC for "priority" so the sorting reflects severity rather than alphabetical
order.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 306797bc-b2c2-41ea-b5e0-392627c02303
📒 Files selected for processing (19)
go.modinternal/app.gointernal/db/migrations/20260414000000_create_tasks.sqlinternal/server/docs/docs.gointernal/server/dto/query.gointernal/server/module.gointernal/server/projects/dto.gointernal/server/projects/handler.gointernal/server/tasks/dto.gointernal/server/tasks/handler.gointernal/tasks/config.gointernal/tasks/doc.gointernal/tasks/domain.gointernal/tasks/errors.gointernal/tasks/models.gointernal/tasks/module.gointernal/tasks/repository.gointernal/tasks/service.gorequests.http
💤 Files with no reviewable changes (1)
- internal/server/projects/dto.go
✅ Files skipped from review due to trivial changes (7)
- go.mod
- internal/tasks/errors.go
- internal/tasks/config.go
- internal/server/projects/handler.go
- internal/db/migrations/20260414000000_create_tasks.sql
- internal/server/dto/query.go
- internal/tasks/doc.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/app.go
- internal/tasks/module.go
- internal/server/module.go
- requests.http
- internal/server/tasks/handler.go
- internal/tasks/service.go
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/server/tasks/dto.go (2)
150-150:⚠️ Potential issue | 🟠 MajorPreserve
DueDatein single-task response mapping.Line 150 hardcodes
DueDate: nil, while list mapping (Line 187) returns the stored due date. This creates inconsistent API behavior between single-item and list responses.🐛 Minimal fix
- DueDate: nil, + DueDate: task.DueDate,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto.go` at line 150, The single-task DTO mapping currently sets DueDate to nil causing inconsistency with the list mapping; update the single-item mapper (the function that builds the task response DTO—look for the single-task mapper/newTaskResponse or ToTaskResponse function in internal/server/tasks/dto.go where DueDate: nil is set) to return the stored due date (e.g., use the source task's DueDate field or the same expression used in the list mapping at line 187) instead of nil so both list and single responses include the same DueDate value.
26-37:⚠️ Potential issue | 🟡 MinorTrim and drop empty CSV tokens before enum conversion.
On Line 28 and Line 35,
strings.Split+ direct cast keeps whitespace/empty tokens (statuses=Open,%20Resolved,), which can inject invalid filter values and unexpectedly return no rows.💡 Suggested fix
statuses := []tasks.Status{} if q.Statuses != nil { - statuses = lo.Map( - strings.Split(*q.Statuses, ","), - func(s string, _ int) tasks.Status { return tasks.Status(s) }, - ) + statuses = lo.FilterMap( + strings.Split(*q.Statuses, ","), + func(s string, _ int) (tasks.Status, bool) { + v := strings.TrimSpace(s) + if v == "" { + return "", false + } + return tasks.Status(v), true + }, + ) } priorities := []tasks.Priority{} if q.Priorities != nil { - priorities = lo.Map( - strings.Split(*q.Priorities, ","), - func(s string, _ int) tasks.Priority { return tasks.Priority(s) }, - ) + priorities = lo.FilterMap( + strings.Split(*q.Priorities, ","), + func(s string, _ int) (tasks.Priority, bool) { + v := strings.TrimSpace(s) + if v == "" { + return "", false + } + return tasks.Priority(v), true + }, + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto.go` around lines 26 - 37, The CSV-to-enum conversion for q.Statuses and q.Priorities currently uses strings.Split and direct casts which preserves whitespace and empty tokens; update the conversion for statuses and priorities (variables statuses and priorities, using q.Statuses/q.Priorities and types tasks.Status/tasks.Priority) to first split, then trim each token and drop any empty tokens before converting to the enum type (e.g., use strings.TrimSpace and skip "" entries or use a FieldsFunc-based split) so that values like "Open, Resolved," or "Open,%20Resolved," produce only valid enum entries.
🧹 Nitpick comments (1)
internal/server/tasks/dto.go (1)
122-123: Clarify the mapper comment to match actual behavior.The comment says this function “requires fetching author and assignee from the users service,” but the function only maps IDs/placeholders. Updating the comment will reduce maintenance confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto.go` around lines 122 - 123, Update the doc comment for the toTaskResponse function to accurately reflect its behavior: instead of saying it “requires fetching author and assignee from the users service,” state that toTaskResponse maps Task fields to the TaskResponse DTO and copies author/assignee IDs or placeholders without performing any user service lookups; reference the toTaskResponse function and TaskResponse DTO so future maintainers know no external calls occur in this mapper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/server/tasks/dto.go`:
- Line 150: The single-task DTO mapping currently sets DueDate to nil causing
inconsistency with the list mapping; update the single-item mapper (the function
that builds the task response DTO—look for the single-task
mapper/newTaskResponse or ToTaskResponse function in
internal/server/tasks/dto.go where DueDate: nil is set) to return the stored due
date (e.g., use the source task's DueDate field or the same expression used in
the list mapping at line 187) instead of nil so both list and single responses
include the same DueDate value.
- Around line 26-37: The CSV-to-enum conversion for q.Statuses and q.Priorities
currently uses strings.Split and direct casts which preserves whitespace and
empty tokens; update the conversion for statuses and priorities (variables
statuses and priorities, using q.Statuses/q.Priorities and types
tasks.Status/tasks.Priority) to first split, then trim each token and drop any
empty tokens before converting to the enum type (e.g., use strings.TrimSpace and
skip "" entries or use a FieldsFunc-based split) so that values like "Open,
Resolved," or "Open,%20Resolved," produce only valid enum entries.
---
Nitpick comments:
In `@internal/server/tasks/dto.go`:
- Around line 122-123: Update the doc comment for the toTaskResponse function to
accurately reflect its behavior: instead of saying it “requires fetching author
and assignee from the users service,” state that toTaskResponse maps Task fields
to the TaskResponse DTO and copies author/assignee IDs or placeholders without
performing any user service lookups; reference the toTaskResponse function and
TaskResponse DTO so future maintainers know no external calls occur in this
mapper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0903ae7e-0637-4c9b-b334-23b49fef0804
📒 Files selected for processing (2)
internal/server/docs/docs.gointernal/server/tasks/dto.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/server/docs/docs.go
capcom6
force-pushed
the
tasks/add-module
branch
from
99ee039 to
8b1c4c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
internal/tasks/domain.go (2)
150-160:⚠️ Potential issue | 🟡 MinorPersist the trimmed title during update validation.
TaskUpdate.Validate()trims into a local variable and discards it, so PATCH can still store whitespace-padded titles after “validation”.Minimal fix
-func (u TaskUpdate) Validate() error { +func (u *TaskUpdate) Validate() error { if u.Title != nil { - title := strings.TrimSpace(*u.Title) - if title == "" { + trimmed := strings.TrimSpace(*u.Title) + if trimmed == "" { return fmt.Errorf("%w: title is required", ErrValidationFailed) } - if len(title) > MaxTitleLength { + if len(trimmed) > MaxTitleLength { return fmt.Errorf("%w: title too long (max 255 characters)", ErrValidationFailed) } + + u.Title = &trimmed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/domain.go` around lines 150 - 160, TaskUpdate.Validate currently trims into a local variable and discards it, allowing whitespace-padded titles to persist; update Validate (the TaskUpdate.Validate method) to trim the incoming u.Title into a local string, run the empty and MaxTitleLength checks against that trimmed string, and then overwrite u.Title with a pointer to the trimmed value so the normalized title is persisted for the subsequent PATCH handling; keep using ErrValidationFailed for error returns and reference MaxTitleLength for the length check.
87-101:⚠️ Potential issue | 🟠 MajorNormalize
ProjectSlugandTitleon the actualTaskInput.
Validate()trims local copies only.Service.Create()still uses the originalinput.ProjectSlug, so whitespace-padded slugs can fail the project lookup and padded titles can still be stored.Minimal fix
-func (i TaskInput) Validate() error { +func (i *TaskInput) Validate() error { // Validate project slug - projectSlug := strings.TrimSpace(i.ProjectSlug) - if projectSlug == "" { + i.ProjectSlug = strings.TrimSpace(i.ProjectSlug) + if i.ProjectSlug == "" { return fmt.Errorf("%w: project slug is required", ErrValidationFailed) } // Validate title - title := strings.TrimSpace(i.Title) - if title == "" { + i.Title = strings.TrimSpace(i.Title) + if i.Title == "" { return fmt.Errorf("%w: title is required", ErrValidationFailed) } - if len(title) > MaxTitleLength { + if len(i.Title) > MaxTitleLength { return fmt.Errorf("%w: title too long (max 255 characters)", ErrValidationFailed) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/domain.go` around lines 87 - 101, The Validate method currently trims only local copies so changes don't persist; change the receiver of TaskInput.Validate to a pointer (func (i *TaskInput) Validate() error), trim and assign the normalized values back to i.ProjectSlug and i.Title inside Validate, and keep the existing validation checks (empty and MaxTitleLength) so Service.Create will use the normalized fields when accessing input.ProjectSlug and input.Title.internal/server/tasks/dto.go (2)
124-153:⚠️ Potential issue | 🟠 MajorSingle-item responses still drop
due_date.
toTaskResponse()always setsDueDate: nil, soGET /tasks/:id,POST /tasks, andPATCH /tasks/:idomit a stored due date even though the list mapper preserves it. That breaks read-after-write behavior.Minimal fix
Author: UserBrief{ ID: task.AuthorID, Email: "", Role: "", CreatedAt: "", }, Assignee: assignee, - DueDate: nil, + DueDate: task.DueDate, CreatedAt: task.CreatedAt.Format(time.RFC3339), UpdatedAt: task.UpdatedAt.Format(time.RFC3339), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto.go` around lines 124 - 153, toTaskResponse currently sets DueDate: nil causing single-item responses to drop stored due_date; update the function (toTaskResponse in internal/server/tasks/dto.go) to mirror the list mapper by checking task.DueDate for nil and, if non-nil, converting it to the expected response type (e.g. formatting with time.RFC3339 and returning a pointer or the same nullable type used by the list mapper) so GET /tasks/:id, POST /tasks and PATCH /tasks/:id return the persisted due date.
24-38:⚠️ Potential issue | 🟡 MinorTrim and drop empty CSV filter tokens before enum conversion.
Inputs like
statuses=Open,%20Resolved,currently become["Open", " Resolved", ""], which makes valid filters stop matching unexpectedly.Minimal fix
statuses := []tasks.Status{} if q.Statuses != nil { - statuses = lo.Map( + statuses = lo.FilterMap( strings.Split(*q.Statuses, ","), - func(s string, _ int) tasks.Status { return tasks.Status(s) }, + func(s string, _ int) (tasks.Status, bool) { + v := strings.TrimSpace(s) + if v == "" { + return "", false + } + return tasks.Status(v), true + }, ) } priorities := []tasks.Priority{} if q.Priorities != nil { - priorities = lo.Map( + priorities = lo.FilterMap( strings.Split(*q.Priorities, ","), - func(s string, _ int) tasks.Priority { return tasks.Priority(s) }, + func(s string, _ int) (tasks.Priority, bool) { + v := strings.TrimSpace(s) + if v == "" { + return "", false + } + return tasks.Priority(v), true + }, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto.go` around lines 24 - 38, The toFilter method on TaskListQuery builds statuses and priorities from CSV but doesn't trim or drop empty tokens, so inputs like "Open, Resolved," become ["Open"," Resolved",""] and produce invalid enums; update (q *TaskListQuery) toFilter to split the CSV, then for each token trim whitespace and skip empty strings before converting to tasks.Status or tasks.Priority (apply the same change for both the statuses and priorities blocks, referencing the existing statuses := []tasks.Status{} and priorities := []tasks.Priority{} logic and conversion functions) so only non-empty, trimmed strings are mapped to enum values.internal/tasks/repository.go (2)
191-202:⚠️ Potential issue | 🟠 Major
RowsAffected()is not a reliable not-found signal for PATCH on MySQL.A no-op update returns
0affected rows too, so sending the same values back can incorrectly becomeErrNotFound. Use a preflight existence check, enableclientFoundRows=true, or stop treatingrows == 0as “missing”.Based on learnings: in this repository’s MySQL setup,
RowsAffected()returns0for both unmatched rows and no-op updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/repository.go` around lines 191 - 202, The code currently treats result.RowsAffected() == 0 as ErrNotFound after calling query.Exec(ctx) which is unreliable for MySQL (no-op updates also return 0); change the update flow in the function that runs query.Exec to perform an existence preflight (e.g. SELECT 1 FROM tasks WHERE id = ? or use your repository's GetByID/exists helper) before executing the PATCH, or alternatively enable clientFoundRows=true at the DB client level and document it; remove the unconditional return of ErrNotFound when result.RowsAffected() == 0 and instead return ErrNotFound only when the preflight SELECT indicates the row does not exist (keep using ErrNotFound symbol and keep query.Exec/result.RowsAffected logic for detecting actual deletions/updates).
28-56:⚠️ Potential issue | 🟠 MajorTask-number allocation is still racy under concurrent creates.
Two creates for the same project can read the same max number, compute the same
nextNumber, and race on insert. The losing request then gets a 400 viaErrValidationFailed, even though the payload was valid. Please serialize allocation with locking or retry on unique violation instead of classifying the collision as user error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/tasks/repository.go` around lines 28 - 56, The code races allocating task numbers because RunInTx reads max number then inserts and treats unique-violation as ErrValidationFailed; change to retry the allocation transaction on unique-violation (or serialize by locking the project row) instead of returning ErrValidationFailed. Concretely, wrap the existing r.db.RunInTx transaction (the block that selects from taskModel, computes nextNumber, calls newTaskModel and does tx.NewInsert) in a small retry loop (e.g., 3 attempts) that re-runs the transaction when db.IsUniqueViolation(insErr) occurs, and only return ErrValidationFailed for true user validation errors; alternatively, acquire a row lock (SELECT ... FOR UPDATE) on the project before computing maxNumber inside the same RunInTx to serialize allocation. Ensure you remove the current branch that maps unique-violation to ErrValidationFailed and instead retry or escalate the DB error after retries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/tasks/handler.go`:
- Around line 257-271: In Handler.errorsHandler, keep the existing errors.Is
checks for tasks.ErrValidationFailed, tasks.ErrNotFound, and
tasks.ErrProjectNotFound, but change the default branch to avoid returning
internal error text: if the error is already a *fiber.Error, return it as-is;
otherwise replace it with a generic
fiber.NewError(fiber.StatusInternalServerError, "internal server error") so
internal service/repo messages are not exposed (locate this logic in the
errorsHandler method to implement the check and return).
---
Duplicate comments:
In `@internal/server/tasks/dto.go`:
- Around line 124-153: toTaskResponse currently sets DueDate: nil causing
single-item responses to drop stored due_date; update the function
(toTaskResponse in internal/server/tasks/dto.go) to mirror the list mapper by
checking task.DueDate for nil and, if non-nil, converting it to the expected
response type (e.g. formatting with time.RFC3339 and returning a pointer or the
same nullable type used by the list mapper) so GET /tasks/:id, POST /tasks and
PATCH /tasks/:id return the persisted due date.
- Around line 24-38: The toFilter method on TaskListQuery builds statuses and
priorities from CSV but doesn't trim or drop empty tokens, so inputs like "Open,
Resolved," become ["Open"," Resolved",""] and produce invalid enums; update (q
*TaskListQuery) toFilter to split the CSV, then for each token trim whitespace
and skip empty strings before converting to tasks.Status or tasks.Priority
(apply the same change for both the statuses and priorities blocks, referencing
the existing statuses := []tasks.Status{} and priorities := []tasks.Priority{}
logic and conversion functions) so only non-empty, trimmed strings are mapped to
enum values.
In `@internal/tasks/domain.go`:
- Around line 150-160: TaskUpdate.Validate currently trims into a local variable
and discards it, allowing whitespace-padded titles to persist; update Validate
(the TaskUpdate.Validate method) to trim the incoming u.Title into a local
string, run the empty and MaxTitleLength checks against that trimmed string, and
then overwrite u.Title with a pointer to the trimmed value so the normalized
title is persisted for the subsequent PATCH handling; keep using
ErrValidationFailed for error returns and reference MaxTitleLength for the
length check.
- Around line 87-101: The Validate method currently trims only local copies so
changes don't persist; change the receiver of TaskInput.Validate to a pointer
(func (i *TaskInput) Validate() error), trim and assign the normalized values
back to i.ProjectSlug and i.Title inside Validate, and keep the existing
validation checks (empty and MaxTitleLength) so Service.Create will use the
normalized fields when accessing input.ProjectSlug and input.Title.
In `@internal/tasks/repository.go`:
- Around line 191-202: The code currently treats result.RowsAffected() == 0 as
ErrNotFound after calling query.Exec(ctx) which is unreliable for MySQL (no-op
updates also return 0); change the update flow in the function that runs
query.Exec to perform an existence preflight (e.g. SELECT 1 FROM tasks WHERE id
= ? or use your repository's GetByID/exists helper) before executing the PATCH,
or alternatively enable clientFoundRows=true at the DB client level and document
it; remove the unconditional return of ErrNotFound when result.RowsAffected() ==
0 and instead return ErrNotFound only when the preflight SELECT indicates the
row does not exist (keep using ErrNotFound symbol and keep
query.Exec/result.RowsAffected logic for detecting actual deletions/updates).
- Around line 28-56: The code races allocating task numbers because RunInTx
reads max number then inserts and treats unique-violation as
ErrValidationFailed; change to retry the allocation transaction on
unique-violation (or serialize by locking the project row) instead of returning
ErrValidationFailed. Concretely, wrap the existing r.db.RunInTx transaction (the
block that selects from taskModel, computes nextNumber, calls newTaskModel and
does tx.NewInsert) in a small retry loop (e.g., 3 attempts) that re-runs the
transaction when db.IsUniqueViolation(insErr) occurs, and only return
ErrValidationFailed for true user validation errors; alternatively, acquire a
row lock (SELECT ... FOR UPDATE) on the project before computing maxNumber
inside the same RunInTx to serialize allocation. Ensure you remove the current
branch that maps unique-violation to ErrValidationFailed and instead retry or
escalate the DB error after retries.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd4c67de-2721-41cf-b3d4-8f34b7a13a9f
📒 Files selected for processing (19)
go.modinternal/app.gointernal/db/migrations/20260414000000_create_tasks.sqlinternal/server/docs/docs.gointernal/server/dto/query.gointernal/server/module.gointernal/server/projects/dto.gointernal/server/projects/handler.gointernal/server/tasks/dto.gointernal/server/tasks/handler.gointernal/tasks/config.gointernal/tasks/doc.gointernal/tasks/domain.gointernal/tasks/errors.gointernal/tasks/models.gointernal/tasks/module.gointernal/tasks/repository.gointernal/tasks/service.gorequests.http
💤 Files with no reviewable changes (1)
- internal/server/projects/dto.go
✅ Files skipped from review due to trivial changes (9)
- internal/tasks/config.go
- internal/tasks/module.go
- internal/server/projects/handler.go
- internal/tasks/doc.go
- internal/tasks/errors.go
- internal/server/dto/query.go
- internal/tasks/models.go
- internal/db/migrations/20260414000000_create_tasks.sql
- internal/server/docs/docs.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/server/module.go
- internal/app.go
- requests.http
- go.mod
This was referenced Apr 17, 2026
This was referenced Apr 27, 2026
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.
Summary by CodeRabbit
New Features
API Behavior
Database
Documentation
Server