[comments] introduce module and API#7
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds a comments feature (domain, repo, service, handlers, docs, migration), introduces a generic DB pagination utility, updates projects/tasks/users to use shared pagination and return totals from List, updates server DTOs/docs/handlers to include comments and adjusted totals, registers Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as "HTTP Handler"
participant CommentsSvc as "Comments Service"
participant UsersSvc as "Users Service"
participant Repo as "Comments Repository"
participant DB as "Database"
Client->>Handler: POST /tasks/{task_id}/comments
Handler->>CommentsSvc: Create(ctx, CommentInput)
CommentsSvc->>CommentsSvc: Validate input
CommentsSvc->>Repo: Create(ctx, input)
Repo->>DB: INSERT comment
DB-->>Repo: row
Repo-->>CommentsSvc: *Comment
CommentsSvc-->>Handler: *Comment
Handler-->>Client: 201 Created (Comment)
Client->>Handler: PUT /tasks/{task_id}/comments/{id}
Handler->>CommentsSvc: Update(ctx, userID, taskID, id, update)
CommentsSvc->>Repo: GetByID(ctx, id)
Repo->>DB: SELECT comment
DB-->>Repo: row
Repo-->>CommentsSvc: *Comment
alt requester is author
CommentsSvc->>Repo: Update(ctx, id, content)
else
CommentsSvc->>UsersSvc: IsAdmin(ctx, userID)
UsersSvc->>DB: SELECT role WHERE id=?
DB-->>UsersSvc: role
UsersSvc-->>CommentsSvc: isAdmin
alt isAdmin
CommentsSvc->>Repo: Update(ctx, id, content)
else
CommentsSvc-->>Handler: ErrUnauthorized
Handler-->>Client: 403
end
end
Client->>Handler: DELETE /tasks/{task_id}/comments/{id}
Handler->>CommentsSvc: Delete(ctx, userID, taskID, id)
CommentsSvc->>Repo: GetByID(ctx, id)
Repo->>DB: SELECT comment
DB-->>Repo: row
Repo-->>CommentsSvc: *Comment
CommentsSvc->>UsersSvc: IsAdmin(ctx, userID) (if needed)
UsersSvc->>DB: SELECT role
DB-->>UsersSvc: role
UsersSvc-->>CommentsSvc: isAdmin
alt authorized
CommentsSvc->>Repo: Delete(ctx, id)
Repo->>DB: UPDATE comments SET deleted_at=NOW()
DB-->>Repo: result
Repo-->>CommentsSvc: nil
CommentsSvc-->>Handler: nil
Handler-->>Client: 204 No Content
else
CommentsSvc-->>Handler: ErrUnauthorized
Handler-->>Client: 403
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 4
🧹 Nitpick comments (1)
internal/db/pagination.go (1)
17-38: Avoid making defaults depend on typed-nil receivers.
NewPaginationleavesdefzero-valued, soLimit()works only because all currentDefaultPaginationimplementations use pointer receivers—allowing methods to be called on nil. This creates an implicit contract that future implementations must follow or the code breaks silently. Initialize a real default provider in the constructor, or store resolved default/max limit values directly inPaginationto make the contract explicit and enforce it at the type level.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/db/pagination.go` around lines 17 - 38, NewPagination currently leaves the def field zero-valued so Limit() relies on nil-receiver behavior of DefaultPagination implementations; change NewPagination to initialize a concrete default provider or resolve and store the default and max limits directly on the Pagination[T] struct so Limit() no longer calls methods on a potentially typed-nil def. Specifically, update NewPagination to populate Pagination.def with a real DefaultPagination instance (or set pagination.defaultLimit and pagination.maxLimit fields) and adjust Limit() to use those stored values instead of calling p.def.DefaultLimit()/p.def.MaxLimit(), ensuring Pagination, NewPagination, and Limit no longer depend on nil receiver method semantics.
🤖 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/comments/doc.go`:
- Around line 13-33: Update the doc examples to match the current
comments.Service API: when calling ListByTask include the pagination argument
(use svc.ListByTask(ctx, taskID, pagination)); for Update swap the ID order so
userID comes before commentID (use svc.Update(ctx, userID, commentID, update));
for Delete likewise pass userID before commentID (use svc.Delete(ctx, userID,
commentID)); ensure the example uses the existing types CommentInput and
CommentUpdate and the service variable name svc to match comments.NewService.
In `@internal/comments/domain.go`:
- Around line 57-59: The validation currently uses len(content) (byte count) but
the error refers to characters; change the checks in domain.go that compare
content to MaxContentLength (and the similar check at the block around lines
72-74) to count Unicode code points instead (use utf8.RuneCountInString or
equivalent) so non-ASCII input is measured in characters, and keep the error
return formatting using ErrValidationFailed and MaxContentLength unchanged.
In `@internal/db/migrations/20260417000000_create_comments.sql`:
- Line 19: Remove the extraneous separator line containing only '---' between
the migration sections: locate the migration file's markers `-- +goose Up` and
`-- +goose Down` and delete the standalone `---` line so only the `-- +goose
Down` directive separates the Up and Down sections.
In `@internal/users/service.go`:
- Around line 74-80: IsAdmin currently calls s.repo.GetByID which returns
UserWithPasswordHash and unnecessarily loads password_hash; add a narrower repo
method (e.g. GetRoleByID(ctx, id int64) (Role, error) or IsAdminByID(ctx, id
int64) (bool, error)) in the repository interface and implement it to only
SELECT role or use an EXISTS admin query, then change Service.IsAdmin to call
that new method instead of repo.GetByID to avoid bringing credential material
into memory.
---
Nitpick comments:
In `@internal/db/pagination.go`:
- Around line 17-38: NewPagination currently leaves the def field zero-valued so
Limit() relies on nil-receiver behavior of DefaultPagination implementations;
change NewPagination to initialize a concrete default provider or resolve and
store the default and max limits directly on the Pagination[T] struct so Limit()
no longer calls methods on a potentially typed-nil def. Specifically, update
NewPagination to populate Pagination.def with a real DefaultPagination instance
(or set pagination.defaultLimit and pagination.maxLimit fields) and adjust
Limit() to use those stored values instead of calling
p.def.DefaultLimit()/p.def.MaxLimit(), ensuring Pagination, NewPagination, and
Limit no longer depend on nil receiver method semantics.
🪄 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: 56fc5f18-628d-4132-829e-1d9c03915907
📒 Files selected for processing (24)
go.modinternal/app.gointernal/comments/doc.gointernal/comments/domain.gointernal/comments/errors.gointernal/comments/models.gointernal/comments/module.gointernal/comments/repository.gointernal/comments/service.gointernal/db/migrations/20260417000000_create_comments.sqlinternal/db/pagination.gointernal/projects/domain.gointernal/projects/repository.gointernal/projects/service.gointernal/server/projects/dto.gointernal/server/projects/handler.gointernal/server/tasks/dto.gointernal/server/users/handler.gointernal/tasks/domain.gointernal/tasks/repository.gointernal/tasks/service.gointernal/users/domain.gointernal/users/repository.gointernal/users/service.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/comments/domain.go (1)
52-60: Consider consolidating duplicated content validation logic.Line 53-60 and Line 68-75 implement the same trim/empty/length checks. Extracting a shared helper will prevent rule drift between create/update validation paths.
♻️ Proposed refactor
+func validateContent(content string) error { + content = strings.TrimSpace(content) + if content == "" { + return fmt.Errorf("%w: content is required", ErrValidationFailed) + } + if utf8.RuneCountInString(content) > MaxContentLength { + return fmt.Errorf("%w: content too long (max %d characters)", ErrValidationFailed, MaxContentLength) + } + return nil +} + // Validate checks that the input data is valid for comment creation. func (i CommentInput) Validate() error { // Validate task ID if i.TaskID <= 0 { return fmt.Errorf("%w: task_id must be positive", ErrValidationFailed) } @@ - content := strings.TrimSpace(i.Content) - if content == "" { - return fmt.Errorf("%w: content is required", ErrValidationFailed) - } - - if utf8.RuneCountInString(content) > MaxContentLength { - return fmt.Errorf("%w: content too long (max %d characters)", ErrValidationFailed, MaxContentLength) - } + if err := validateContent(i.Content); err != nil { + return err + } return nil } @@ func (u CommentUpdate) Validate() error { - // Validate content - content := strings.TrimSpace(u.Content) - if content == "" { - return fmt.Errorf("%w: content is required", ErrValidationFailed) - } - - if utf8.RuneCountInString(content) > MaxContentLength { - return fmt.Errorf("%w: content too long (max %d characters)", ErrValidationFailed, MaxContentLength) + if err := validateContent(u.Content); err != nil { + return err } return nil }Also applies to: 67-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/comments/domain.go` around lines 52 - 60, The content trim/empty/length checks (using strings.TrimSpace(i.Content), ErrValidationFailed and MaxContentLength with utf8.RuneCountInString) are duplicated; extract a single helper like validateContent(content string) error (or validateCommentContent) that performs TrimSpace, empty check and max-length check and returns the same fmt.Errorf(...) errors, then replace both inline blocks with calls to that helper from the create/update validation paths so they share one implementation and avoid rule drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/comments/domain.go`:
- Around line 52-60: The content trim/empty/length checks (using
strings.TrimSpace(i.Content), ErrValidationFailed and MaxContentLength with
utf8.RuneCountInString) are duplicated; extract a single helper like
validateContent(content string) error (or validateCommentContent) that performs
TrimSpace, empty check and max-length check and returns the same fmt.Errorf(...)
errors, then replace both inline blocks with calls to that helper from the
create/update validation paths so they share one implementation and avoid rule
drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8ee38f3-0132-49cc-b6bb-7cdb49f53aab
📒 Files selected for processing (4)
internal/comments/doc.gointernal/comments/domain.gointernal/users/repository.gointernal/users/service.go
✅ Files skipped from review due to trivial changes (1)
- internal/comments/doc.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/users/repository.go
capcom6
force-pushed
the
comments/add-module
branch
from
1577ad2 to
de5cb64
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/server/tasks/handler.go (1)
150-152:⚠️ Potential issue | 🟡 MinorSwagger annotation is out of sync with the created-task payload.
Line 174 returns
TaskDetailsResponse, but Lines 150-152 still documentTaskResponse. The generated spec therefore omits thecommentsfield forPOST /tasks.Suggested fix
-// `@Success` 201 {object} TaskResponse +// `@Success` 201 {object} TaskDetailsResponseAlso applies to: 174-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/handler.go` around lines 150 - 152, The Swagger annotations for the POST /tasks handler are stale: update the `@Success` 201 response type from TaskResponse to TaskDetailsResponse (the handler returns TaskDetailsResponse which includes comments) and ensure any other occurrences of TaskResponse for this endpoint (e.g., the earlier `@Param/`@Success block and the later single-line annotation referenced at line 174) are changed to TaskDetailsResponse so the generated spec includes the comments field; locate annotations near the handler for TaskCreateRequest/TaskResponse and replace the response type symbol with TaskDetailsResponse.internal/server/tasks/dto.go (1)
118-135:⚠️ Potential issue | 🟠 MajorPreserve
due_datein single-task responses.Line 133 hardcodes
DueDate: nil, soGET /tasks/{id},POST /tasks, andPATCH /tasks/{id}now drop an existing due date while the list mapper still includes it on Line 191.Suggested fix
return TaskResponse{ ID: task.ID, ProjectSlug: task.ProjectSlug, Number: task.Number, Title: task.Title, Description: task.Description, Priority: string(task.Priority), Status: string(task.Status), Author: dto.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 118 - 135, The single-task mapper is dropping the due date by hardcoding DueDate: nil in the TaskResponse construction; update the mapping in the function that builds TaskResponse (the block returning TaskResponse with ID, ProjectSlug, Number, etc.) to mirror the list mapper's behavior and set DueDate based on task.DueDate (i.e., if task.DueDate is non-nil, format it as RFC3339 and assign that pointer/string, otherwise leave nil) so GET /tasks/{id}, POST /tasks and PATCH /tasks/{id} preserve existing due dates.
🤖 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_comments.go`:
- Around line 36-48: toCommentResponse currently fills dto.UserBrief fields
(Email, Role, CreatedAt) with empty strings causing clients to receive a
misleading author object; either resolve and populate the actual author data or
shrink the contract to only expose AuthorID. Fix by updating
toCommentResponse(comment *comments.Comment) to fetch the author (e.g., via the
user service or repository available to handlers) and set
dto.UserBrief.{ID,Email,Role,CreatedAt} from the real user before returning
CommentResponse, or change CommentResponse to remove the full dto.UserBrief and
only include AuthorID and adjust callers (create/update handlers and task detail
responses) to expect the slimmer shape. Ensure you update any DTO type
definitions and usages of CommentResponse accordingly (referencing
CommentResponse, dto.UserBrief, comments.Comment).
In `@requests.http`:
- Around line 307-309: The explanatory note is pointing to a non-existent
request name createComment2; update the comment above the DELETE request (DELETE
{{baseURL}}/tasks/{{taskId}}/comments/{{commentId}}) to reference the actual
request that sets `@commentId` (createComment) or remove the bogus reference
entirely so it correctly instructs the reader to use the `@commentId` returned by
createComment.
---
Outside diff comments:
In `@internal/server/tasks/dto.go`:
- Around line 118-135: The single-task mapper is dropping the due date by
hardcoding DueDate: nil in the TaskResponse construction; update the mapping in
the function that builds TaskResponse (the block returning TaskResponse with ID,
ProjectSlug, Number, etc.) to mirror the list mapper's behavior and set DueDate
based on task.DueDate (i.e., if task.DueDate is non-nil, format it as RFC3339
and assign that pointer/string, otherwise leave nil) so GET /tasks/{id}, POST
/tasks and PATCH /tasks/{id} preserve existing due dates.
In `@internal/server/tasks/handler.go`:
- Around line 150-152: The Swagger annotations for the POST /tasks handler are
stale: update the `@Success` 201 response type from TaskResponse to
TaskDetailsResponse (the handler returns TaskDetailsResponse which includes
comments) and ensure any other occurrences of TaskResponse for this endpoint
(e.g., the earlier `@Param/`@Success block and the later single-line annotation
referenced at line 174) are changed to TaskDetailsResponse so the generated spec
includes the comments field; locate annotations near the handler for
TaskCreateRequest/TaskResponse and replace the response type symbol with
TaskDetailsResponse.
🪄 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: 896c02ca-6147-43b2-bcd2-51f7ed45e405
📒 Files selected for processing (9)
internal/comments/domain.gointernal/comments/repository.gointernal/comments/service.gointernal/server/docs/docs.gointernal/server/dto/response.gointernal/server/tasks/dto.gointernal/server/tasks/dto_comments.gointernal/server/tasks/handler.gorequests.http
✅ Files skipped from review due to trivial changes (1)
- internal/server/dto/response.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/comments/service.go
- internal/comments/repository.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/server/tasks/dto.go (1)
155-199: Consider reusingnewTaskResponseto reduce duplication.The inline mapping logic (lines 166-194) duplicates
newTaskResponse. Refactoring to reuse the existing function improves maintainability.♻️ Proposed refactor
func toTaskListResponse(items []tasks.Task, total int) TaskListResponse { return TaskListResponse{ Items: lo.Map( items, func(t tasks.Task, _ int) TaskResponse { - var assignee *dto.UserBrief - if t.AssigneeID != nil { - assignee = &dto.UserBrief{ - ID: *t.AssigneeID, - Email: "", // Will be populated by service - Role: "", - CreatedAt: "", - } - } - - return TaskResponse{ - ID: t.ID, - ProjectSlug: t.ProjectSlug, - Number: t.Number, - Title: t.Title, - Description: t.Description, - Priority: string(t.Priority), - Status: string(t.Status), - Author: dto.UserBrief{ - ID: t.AuthorID, - Email: "", // Will be populated by service - Role: "", - CreatedAt: "", - }, - Assignee: assignee, - DueDate: t.DueDate, - CreatedAt: t.CreatedAt.Format(time.RFC3339), - UpdatedAt: t.UpdatedAt.Format(time.RFC3339), - } + return newTaskResponse(&t) }, ), Total: total, } }🤖 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 155 - 199, The mapping in toTaskListResponse duplicates the construction logic already implemented in newTaskResponse; replace the inline lambda in lo.Map with a call to newTaskResponse (or a small adapter that calls newTaskResponse) to build each TaskResponse from tasks.Task, preserving special handling for nil AssigneeID and ensuring CreatedAt/UpdatedAt are formatted (or that newTaskResponse handles formatting). Update toTaskListResponse to pass the domain task into newTaskResponse (or convert any missing fields before calling it) and remove the duplicated field-by-field construction so all task->TaskResponse logic lives in newTaskResponse.
🤖 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_comments.go`:
- Around line 100-138: The Swagger docs for deleteComment are missing a 400
response for invalid ID parsing; update the comment block above the
deleteComment function to include a `@Failure` 400 {object} fiberfx.ErrorResponse
(or similar) entry describing "invalid task ID or comment ID" so the OpenAPI
spec documents the bad-request cases returned when strconv.ParseInt fails for
task_id or id.
---
Nitpick comments:
In `@internal/server/tasks/dto.go`:
- Around line 155-199: The mapping in toTaskListResponse duplicates the
construction logic already implemented in newTaskResponse; replace the inline
lambda in lo.Map with a call to newTaskResponse (or a small adapter that calls
newTaskResponse) to build each TaskResponse from tasks.Task, preserving special
handling for nil AssigneeID and ensuring CreatedAt/UpdatedAt are formatted (or
that newTaskResponse handles formatting). Update toTaskListResponse to pass the
domain task into newTaskResponse (or convert any missing fields before calling
it) and remove the duplicated field-by-field construction so all
task->TaskResponse logic lives in newTaskResponse.
🪄 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: 3923ee5a-70d1-411e-9bd9-8217fce32657
📒 Files selected for processing (4)
internal/server/tasks/dto.gointernal/server/tasks/handler.gointernal/server/tasks/handler_comments.gorequests.http
🚧 Files skipped from review as they are similar to previous changes (1)
- requests.http
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/server/tasks/dto.go (2)
155-158: Minor type inconsistency forTotalfield.
TaskListResponse.TotalisintwhileCommentListResponse.Total(indto_comments.go) isint64. For consistency and to avoid potential truncation on 32-bit platforms, consider usingint64for all pagination totals.🤖 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 155 - 158, Change the Total field on TaskListResponse from int to int64 to match CommentListResponse and avoid platform truncation; update the struct TaskListResponse (type TaskListResponse { Items []TaskResponse `json:"items"`; Total int -> int64 }) and any code that constructs or reads TaskListResponse.Total to use int64 to keep types consistent with CommentListResponse in dto_comments.go.
145-150: Consider avoiding intermediate allocation.
toCommentListResponse(comments, 0).Itemsallocates aCommentListResponsestruct only to immediately discard everything exceptItems. Consider extracting the slice-mapping logic into a shared helper or uselo.Mapdirectly.♻️ Suggested refactor
func newTaskDetailsResponse(task *tasks.Task, comments []comments.Comment) *TaskDetailsResponse { + commentResponses := lo.Map(comments, func(c comments.Comment, _ int) CommentResponse { + return *toCommentResponse(&c) + }) return &TaskDetailsResponse{ TaskResponse: newTaskResponse(task), - Comments: toCommentListResponse(comments, 0).Items, + Comments: commentResponses, } }🤖 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 145 - 150, newTaskDetailsResponse currently calls toCommentListResponse(comments, 0).Items which creates a CommentListResponse just to take its Items; change this to map the comments directly to []CommentResponse to avoid the intermediate allocation—either add a small helper like mapCommentsToResponses(comments []comments.Comment) []CommentResponse (or toCommentResponses) that performs the slice mapping using the same logic as toCommentListResponse, or use lo.Map inline to produce the []CommentResponse; then replace the Comments field assignment in newTaskDetailsResponse to call that helper (or lo.Map) instead of toCommentListResponse(...).Items.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/server/tasks/dto.go`:
- Around line 155-158: Change the Total field on TaskListResponse from int to
int64 to match CommentListResponse and avoid platform truncation; update the
struct TaskListResponse (type TaskListResponse { Items []TaskResponse
`json:"items"`; Total int -> int64 }) and any code that constructs or reads
TaskListResponse.Total to use int64 to keep types consistent with
CommentListResponse in dto_comments.go.
- Around line 145-150: newTaskDetailsResponse currently calls
toCommentListResponse(comments, 0).Items which creates a CommentListResponse
just to take its Items; change this to map the comments directly to
[]CommentResponse to avoid the intermediate allocation—either add a small helper
like mapCommentsToResponses(comments []comments.Comment) []CommentResponse (or
toCommentResponses) that performs the slice mapping using the same logic as
toCommentListResponse, or use lo.Map inline to produce the []CommentResponse;
then replace the Comments field assignment in newTaskDetailsResponse to call
that helper (or lo.Map) instead of toCommentListResponse(...).Items.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4dcbbd07-1a60-44b9-a629-ad73e67bdfd3
📒 Files selected for processing (3)
internal/server/docs/docs.gointernal/server/tasks/dto.gointernal/server/tasks/handler_comments.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/server/tasks/handler_comments.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/server/tasks/dto_comments.go (1)
29-44:⚠️ Potential issue | 🟠 MajorDon’t emit a full
authorobject with placeholder empty fields.Line 40–44 constructs
authorwith emptyrole, andcreated_at, which makes the response shape misleading. Either hydrate author details before mapping, or exposeauthor_idinstead ofdto.UserBrief.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto_comments.go` around lines 29 - 44, The toCommentResponse function is emitting a dto.UserBrief with empty fields which is misleading; update the mapping in toCommentResponse to either (a) populate dto.UserBrief by fetching/hydrating the author details (e.g., call your user service/Repo to get user by comment.AuthorID and set ID/Email/Role/CreatedAt accordingly) or (b) change the CommentResponse struct and this mapper to expose AuthorID: comment.AuthorID instead of Author: dto.UserBrief so you don't return placeholder empty fields (modify CommentResponse and any callers that expect Author to use AuthorID if you choose option b).
🧹 Nitpick comments (1)
internal/server/tasks/dto_comments.go (1)
36-56: Return comment DTOs by value instead of pointer.
toCommentResponsereturns*CommentResponse, but callers immediately dereference it. Returning by value removes unnecessary pointer churn.♻️ Proposed simplification
-func toCommentResponse(comment *comments.Comment) *CommentResponse { - return &CommentResponse{ +func toCommentResponse(comment *comments.Comment) CommentResponse { + return CommentResponse{ ID: comment.ID, Author: dto.UserBrief{ ID: comment.AuthorID, Email: "", Role: "", CreatedAt: "", }, Content: comment.Content, CreatedAt: comment.CreatedAt.UTC().Format(time.RFC3339), UpdatedAt: comment.UpdatedAt.UTC().Format(time.RFC3339), } } @@ comments := make([]CommentResponse, 0, len(items)) for _, item := range items { - comments = append(comments, *toCommentResponse(&item)) + comments = append(comments, toCommentResponse(&item)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/dto_comments.go` around lines 36 - 56, Change toCommentResponse to return a CommentResponse by value instead of *CommentResponse to avoid unnecessary pointers; update its signature (toCommentResponse) and body to construct and return a value, then update callers such as toCommentsList to append the returned value directly (remove the dereference of *toCommentResponse(&item)), and scan for and fix any other call sites that currently expect a pointer to use the new value return type.
🤖 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_comments.go`:
- Around line 29-44: The toCommentResponse function is emitting a dto.UserBrief
with empty fields which is misleading; update the mapping in toCommentResponse
to either (a) populate dto.UserBrief by fetching/hydrating the author details
(e.g., call your user service/Repo to get user by comment.AuthorID and set
ID/Email/Role/CreatedAt accordingly) or (b) change the CommentResponse struct
and this mapper to expose AuthorID: comment.AuthorID instead of Author:
dto.UserBrief so you don't return placeholder empty fields (modify
CommentResponse and any callers that expect Author to use AuthorID if you choose
option b).
---
Nitpick comments:
In `@internal/server/tasks/dto_comments.go`:
- Around line 36-56: Change toCommentResponse to return a CommentResponse by
value instead of *CommentResponse to avoid unnecessary pointers; update its
signature (toCommentResponse) and body to construct and return a value, then
update callers such as toCommentsList to append the returned value directly
(remove the dereference of *toCommentResponse(&item)), and scan for and fix any
other call sites that currently expect a pointer to use the new value return
type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6da556b-b162-4690-b26e-ea89822c6078
📒 Files selected for processing (2)
internal/server/tasks/dto.gointernal/server/tasks/dto_comments.go
capcom6
force-pushed
the
comments/add-module
branch
from
be55b0a to
5617b1a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/comments/domain.go (1)
50-58: Validation trims but doesn't persist the trimmed value.The validation creates a local
content := strings.TrimSpace(i.Content)for checking, but the originali.Content(potentially with whitespace) is what gets stored. Consider normalizing the content before validation or documenting this behavior.♻️ Optional fix to normalize content
func (i CommentInput) Validate() error { + i.Content = strings.TrimSpace(i.Content) + // Validate task ID if i.TaskID <= 0 { return fmt.Errorf("%w: task_id must be positive", ErrValidationFailed) } // Validate author ID if i.AuthorID <= 0 { return fmt.Errorf("%w: author_id must be positive", ErrValidationFailed) } // Validate content - content := strings.TrimSpace(i.Content) - if content == "" { + if i.Content == "" { return fmt.Errorf("%w: content is required", ErrValidationFailed) } - if utf8.RuneCountInString(content) > MaxContentLength { + if utf8.RuneCountInString(i.Content) > MaxContentLength { return fmt.Errorf("%w: content too long (max %d characters)", ErrValidationFailed, MaxContentLength) } return nil }Note: This requires changing the receiver to a pointer
(i *CommentInput)to mutate the struct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/comments/domain.go` around lines 50 - 58, The validation trims into a local variable but doesn't persist the trimmed string, so update the validator to normalize input by assigning the trimmed value back to the struct (set i.Content = strings.TrimSpace(i.Content)) before running the length checks; to allow mutation change the receiver to a pointer (e.g., func (i *CommentInput) Validate(...)) and then use utf8.RuneCountInString(i.Content) with MaxContentLength and ErrValidationFailed as currently done to return the same errors when validation fails.internal/comments/repository.go (1)
45-61: Consider adding pagination for task comments.The
Listmethod returns all comments for a task without pagination. For tasks with many comments, this could lead to performance issues and large response payloads. Consider adding optional pagination parameters consistent with the generic DB pagination utility mentioned in the PR summary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/comments/repository.go` around lines 45 - 61, Update Repository.List to support pagination by adding optional pagination parameters (e.g., limit and offset or a pagination options struct) to the method signature and applying them to the query before calling OrderExpr/Scan; specifically, modify List(ctx context.Context, taskID int64) to accept pagination inputs, apply those to r.db.NewSelect() (using the project’s generic DB pagination utility if available), and ensure the returned slice of commentModel -> Comment conversion via model.toDomain() remains unchanged so callers get a paginated []Comment result.internal/server/tasks/dto.go (1)
107-137: Author/Assignee UserBrief fields are only partially populated.The
newTaskResponsefunction initializesdto.UserBriefwith onlyIDpopulated, leavingRole, andCreatedAtas empty strings. This is consistent with the pattern indto_comments.go(which has a similar existing review comment). Clients will receive aUserBriefobject with missing data.If resolving user details is deferred intentionally (e.g., for performance or to allow client-side enrichment), consider documenting this contract or slimming the DTO to only include
author_idandassignee_id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/comments/domain.go`:
- Around line 50-58: The validation trims into a local variable but doesn't
persist the trimmed string, so update the validator to normalize input by
assigning the trimmed value back to the struct (set i.Content =
strings.TrimSpace(i.Content)) before running the length checks; to allow
mutation change the receiver to a pointer (e.g., func (i *CommentInput)
Validate(...)) and then use utf8.RuneCountInString(i.Content) with
MaxContentLength and ErrValidationFailed as currently done to return the same
errors when validation fails.
In `@internal/comments/repository.go`:
- Around line 45-61: Update Repository.List to support pagination by adding
optional pagination parameters (e.g., limit and offset or a pagination options
struct) to the method signature and applying them to the query before calling
OrderExpr/Scan; specifically, modify List(ctx context.Context, taskID int64) to
accept pagination inputs, apply those to r.db.NewSelect() (using the project’s
generic DB pagination utility if available), and ensure the returned slice of
commentModel -> Comment conversion via model.toDomain() remains unchanged so
callers get a paginated []Comment result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b09c9c37-08da-4681-8af1-be9f5fb8dcf6
📒 Files selected for processing (17)
go.modinternal/app.gointernal/comments/doc.gointernal/comments/domain.gointernal/comments/errors.gointernal/comments/models.gointernal/comments/module.gointernal/comments/repository.gointernal/comments/service.gointernal/db/migrations/20260417000000_create_comments.sqlinternal/server/docs/docs.gointernal/server/dto/response.gointernal/server/tasks/dto.gointernal/server/tasks/dto_comments.gointernal/server/tasks/handler.gointernal/server/tasks/handler_comments.gorequests.http
✅ Files skipped from review due to trivial changes (9)
- internal/app.go
- go.mod
- internal/comments/module.go
- internal/comments/errors.go
- internal/server/dto/response.go
- requests.http
- internal/db/migrations/20260417000000_create_comments.sql
- internal/comments/doc.go
- internal/comments/models.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/server/tasks/handler.go
- internal/comments/service.go
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
Improvements
Documentation