[server] fill user info in references#13
Merged
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughBatch user lookup was added and threaded through task/comment/attachment DTO mappers; handlers centralize response preparation and call the new users bulk-lookup service; comment/attachment handlers enrich responses with resolved user briefs; Swagger PATCH /tasks/{id} success schema changed to TaskDetailsResponse. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as "Client"
participant Handler as "Tasks Handler"
participant UsersSvc as "Users Service"
participant Repo as "Users Repo/DB"
participant DTO as "DTO mappers"
Client->>Handler: API request (list/get/post/patch/attach/comment)
Handler->>Handler: collect user IDs from tasks/comments/attachments
Handler->>UsersSvc: LookupByIDs(ctx, ids)
UsersSvc->>Repo: GetByIDs(ctx, ids)
Repo-->>UsersSvc: map[id]User
UsersSvc-->>Handler: usersMap
Handler->>DTO: toTaskList/DetailsResponse(..., usersMap)
DTO-->>Handler: Response payload (Author/Assignee/UploadedBy resolved)
Handler-->>Client: HTTP response (TaskList/TaskDetails)
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. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
internal/server/dto/response.go (1)
29-49: Optional: reduce duplication and simplify the stub literal.The "found" branch duplicates
ToUserBrief, and the not-found branch sets fields to their zero values explicitly. Both can be tightened without changing behavior:♻️ Proposed refactor
func ResolveUserBrief(id *int64, lookup map[int64]users.User) *UserBrief { if id == nil { return nil } if u, ok := lookup[*id]; ok { - return &UserBrief{ - ID: u.ID, - Name: u.Name, - Role: string(u.Role), - CreatedAt: u.CreatedAt.Format(time.RFC3339), - } + brief := ToUserBrief(&u) + return &brief } - return &UserBrief{ - ID: *id, - Name: "", - Role: "", - CreatedAt: "", - } + return &UserBrief{ID: *id} }Worth flagging separately: when a user is missing,
CreatedAtis serialized as an empty string"", which is not a valid RFC3339 timestamp. Confirm this is acceptable for clients that parse the field as a date (no regression vs. prior behavior, just easy to overlook).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/dto/response.go` around lines 29 - 49, ResolveUserBrief duplicates the full UserBrief literal; change the "found" branch to reuse the existing conversion helper (e.g., call ToUserBrief(u) or a single construction helper) and simplify the "not found" branch to return &UserBrief{ID: *id} so zero-values (Name, Role, CreatedAt) are implicit. Update ResolveUserBrief to reference the helper (ToUserBrief or a new user->UserBrief converter) and ensure CreatedAt remains the empty string behavior only if that matches prior expectations for clients that parse RFC3339.internal/server/tasks/handler_comments.go (1)
156-180: Minor: parameter name shadows thecommentspackage and the error message is duplicated by the caller.
comments []comments.Commentshadows the importedcommentspackage within this function. It compiles today because the body never references the package, but it's a foot-gun if anyone later tries to usecomments.X. Renaming toitems(matchesdto_attachments.go/dto_comments.go) avoids the trap.fetchUsersForCommentsalready wraps the error as"failed to fetch users: %w", and both call sites increateComment/updateCommentwrap it again with the same message. Either drop the inner wrap or the outer wrap.♻️ Proposed tweak
// fetchUsersForComments collects unique user IDs from comments and fetches them in a single batch call. func (h *Handler) fetchUsersForComments( ctx context.Context, - comments []comments.Comment, + items []comments.Comment, ) (map[int64]users.User, error) { idSet := make(map[int64]struct{}) - for _, c := range comments { + for _, c := range items { idSet[c.AuthorID] = struct{}{} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/handler_comments.go` around lines 156 - 180, The function fetchUsersForComments currently shadows the imported comments package by using the parameter name "comments" and also double-wraps the error message; rename the parameter to "items" (or "dtos") in fetchUsersForComments and update its local uses (the function signature and the for-range) to avoid shadowing, and remove the inner fmt.Errorf wrapping in fetchUsersForComments by returning the underlying err directly (or alternatively remove the outer wraps at the createComment/updateComment call sites) so the "failed to fetch users" message is only applied once.internal/server/tasks/handler.go (2)
353-356: Localcommentsvariable shadows thecommentspackage.
comments, err := h.commentsSvc.ListByTask(...)(line 353) shadows thegithub.com/bit-issues/backend/internal/commentsimport that this file relies on (e.g.,comments.ErrNotFoundinerrorsHandleris in a different scope so it still works, but having the same identifier mean two different things in the same file is a footgun). Renaming tocommentList(matchingattachmentListnext to it) keeps things consistent.- comments, err := h.commentsSvc.ListByTask(ctx, task.ID) + commentList, err := h.commentsSvc.ListByTask(ctx, task.ID) if err != nil { return nil, fmt.Errorf("failed to get comments: %w", err) } ... - return newTaskDetailsResponse(task, usersMap, comments, attachmentList), nil + return newTaskDetailsResponse(task, usersMap, commentList, attachmentList), nil🤖 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 353 - 356, The local variable named comments returned from h.commentsSvc.ListByTask(...) shadows the imported comments package; rename that local variable to commentList (to match attachmentList) and update all subsequent references in this function/handler to use commentList instead of comments so no identifier conflicts remain with the comments package (e.g., change the variable from comments to commentList around the call to h.commentsSvc.ListByTask and where it is consumed later in the same scope).
305-347: Avoid shadowing thetaskspackage import with parameter names.Both
fetchUsersForTasks(line 306) andprepareListResponse(line 337) declare a parameter namedtasksof type[]tasks.Task. The type annotation resolves first so this compiles, but inside the functiontasksnow refers to the slice and the package is no longer reachable — fragile if you later need atasks.SomeOtherreference, and confusing to readers. A short, distinct name keeps the package accessible.♻️ Suggested rename
-func (h *Handler) fetchUsersForTasks(ctx context.Context, tasks []tasks.Task) (map[int64]users.User, error) { +func (h *Handler) fetchUsersForTasks(ctx context.Context, taskList []tasks.Task) (map[int64]users.User, error) { // Collect unique user IDs idSet := make(map[int64]struct{}) - for _, t := range tasks { + for _, t := range taskList { idSet[t.AuthorID] = struct{}{} if t.AssigneeID != nil { idSet[*t.AssigneeID] = struct{}{} } } ... } func (h *Handler) prepareListResponse( ctx context.Context, - tasks []tasks.Task, + taskList []tasks.Task, total int, ) (*TaskListResponse, error) { - usersMap, err := h.fetchUsersForTasks(ctx, tasks) + usersMap, err := h.fetchUsersForTasks(ctx, taskList) ... - return toTaskListResponse(tasks, usersMap, total), nil + return toTaskListResponse(taskList, usersMap, total), nil }🤖 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 305 - 347, The parameter name "tasks" in fetchUsersForTasks and prepareListResponse shadows the imported tasks package; rename the slice parameters (e.g., to taskList, ts, or taskSlice) in both functions so references like []tasks.Task remain unambiguous and the tasks package stays reachable; update all uses of the old parameter name inside fetchUsersForTasks and prepareListResponse (including the call site prepareListResponse(ctx, tasks, total)) to the new parameter name to avoid compilation errors.internal/server/tasks/handler_attachments.go (1)
69-93: Parameterattachmentsshadows theattachmentspackage import.Same pattern as in
handler.go:attachments []attachments.Attachmentresolves at parse time but inside the function the package is unreachable through that identifier. Rename for clarity:func (h *Handler) fetchUsersForAttachments( ctx context.Context, - attachments []attachments.Attachment, + attachmentList []attachments.Attachment, ) (map[int64]users.User, error) { idSet := make(map[int64]struct{}) - for _, a := range attachments { + for _, a := range attachmentList { idSet[a.UploadedBy] = struct{}{} } ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/handler_attachments.go` around lines 69 - 93, The parameter name attachments in fetchUsersForAttachments shadows the attachments package import, preventing use of the package identifier; rename the parameter (e.g., to atts or attList) in the function signature and update all references inside fetchUsersForAttachments (including the for loop and any usages like a.UploadedBy) so the package attachments remains accessible by name, keeping the function name fetchUsersForAttachments and behavior unchanged.
🤖 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 326-344: fetchUsersForTasks currently wraps the LookupByIDs error
with "failed to fetch users: %w" and then prepareListResponse /
prepareDetailsResponse wrap that same message again, causing duplicate prefixes;
to fix, remove the redundant wrap inside fetchUsersForTasks (leave the
LookupByIDs error unwrapped) so that prepareListResponse and
prepareDetailsResponse can perform a single contextual wrap. Update
fetchUsersForTasks to propagate the raw err returned from usersSvc.LookupByIDs
(refer to function fetchUsersForTasks and method LookupByIDs) instead of
wrapping it, keeping the outer fmt.Errorf("failed to prepare list response: %w",
err) / prepareDetailsResponse wrapping as the single user-facing context.
- Around line 349-369: prepareDetailsResponse builds usersMap only from
fetchUsersForTasks, so comment authors and attachment uploaders are missing;
update prepareDetailsResponse to collect unique user IDs from the task
(task.AuthorID, task.AssigneeID), from comments (use fetchUsersForComments or
extract author IDs from comments), and from attachments (use
fetchUsersForAttachments or extract uploader IDs), then perform a single batch
lookup via h.usersSvc.LookupByIDs (or combine results from
fetchUsersForComments/fetchUsersForAttachments) to produce a merged usersMap and
pass that merged map to newTaskDetailsResponse so
toCommentsList/toAttachmentsList have full user data available.
---
Nitpick comments:
In `@internal/server/dto/response.go`:
- Around line 29-49: ResolveUserBrief duplicates the full UserBrief literal;
change the "found" branch to reuse the existing conversion helper (e.g., call
ToUserBrief(u) or a single construction helper) and simplify the "not found"
branch to return &UserBrief{ID: *id} so zero-values (Name, Role, CreatedAt) are
implicit. Update ResolveUserBrief to reference the helper (ToUserBrief or a new
user->UserBrief converter) and ensure CreatedAt remains the empty string
behavior only if that matches prior expectations for clients that parse RFC3339.
In `@internal/server/tasks/handler_attachments.go`:
- Around line 69-93: The parameter name attachments in fetchUsersForAttachments
shadows the attachments package import, preventing use of the package
identifier; rename the parameter (e.g., to atts or attList) in the function
signature and update all references inside fetchUsersForAttachments (including
the for loop and any usages like a.UploadedBy) so the package attachments
remains accessible by name, keeping the function name fetchUsersForAttachments
and behavior unchanged.
In `@internal/server/tasks/handler_comments.go`:
- Around line 156-180: The function fetchUsersForComments currently shadows the
imported comments package by using the parameter name "comments" and also
double-wraps the error message; rename the parameter to "items" (or "dtos") in
fetchUsersForComments and update its local uses (the function signature and the
for-range) to avoid shadowing, and remove the inner fmt.Errorf wrapping in
fetchUsersForComments by returning the underlying err directly (or alternatively
remove the outer wraps at the createComment/updateComment call sites) so the
"failed to fetch users" message is only applied once.
In `@internal/server/tasks/handler.go`:
- Around line 353-356: The local variable named comments returned from
h.commentsSvc.ListByTask(...) shadows the imported comments package; rename that
local variable to commentList (to match attachmentList) and update all
subsequent references in this function/handler to use commentList instead of
comments so no identifier conflicts remain with the comments package (e.g.,
change the variable from comments to commentList around the call to
h.commentsSvc.ListByTask and where it is consumed later in the same scope).
- Around line 305-347: The parameter name "tasks" in fetchUsersForTasks and
prepareListResponse shadows the imported tasks package; rename the slice
parameters (e.g., to taskList, ts, or taskSlice) in both functions so references
like []tasks.Task remain unambiguous and the tasks package stays reachable;
update all uses of the old parameter name inside fetchUsersForTasks and
prepareListResponse (including the call site prepareListResponse(ctx, tasks,
total)) to the new parameter name to avoid compilation errors.
🪄 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: 6bb35366-f7fd-4574-be07-4bf8323f51c1
📒 Files selected for processing (12)
internal/server/docs/docs.gointernal/server/dto/response.gointernal/server/tasks/dto.gointernal/server/tasks/dto_attachments.gointernal/server/tasks/dto_comments.gointernal/server/tasks/handler.gointernal/server/tasks/handler_attachments.gointernal/server/tasks/handler_comments.gointernal/users/models.gointernal/users/repository.gointernal/users/service.gorequests.http
🤖 Pull request artifacts
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/server/tasks/handler.go (1)
349-369:⚠️ Potential issue | 🔴 CriticalCritical: comment authors and attachment uploaders are missing from
usersMap.
prepareDetailsResponsebuildsusersMapfromfetchUsersForTasks(ctx, []tasks.Task{*task}), which only collectstask.AuthorID/task.AssigneeID. The same map is then forwarded tonewTaskDetailsResponse → toCommentsList / toAttachmentsList. For any comment author or attachment uploader that isn't also the task's author/assignee,ResolveUserBriefwill fall back to a stubUserBriefwith emptyName,Role, andCreatedAt— exactly the data this PR aims to fill in.Merge IDs from
commentsandattachmentListinto a single batch before constructing the response (or perform 2–3 lookups and merge the resulting maps).🛠️ Suggested fix
- // Fetch users for author and assignee enrichment - usersMap, err := h.fetchUsersForTasks(ctx, []tasks.Task{*task}) - if err != nil { - return nil, err - } + // Collect user IDs from task, comments, and attachments for a single batch lookup. + idSet := map[int64]struct{}{task.AuthorID: {}} + if task.AssigneeID != nil { + idSet[*task.AssigneeID] = struct{}{} + } + for _, c := range comments { + idSet[c.AuthorID] = struct{}{} + } + for _, a := range attachmentList { + idSet[a.UploadedBy] = struct{}{} + } + ids := make([]int64, 0, len(idSet)) + for id := range idSet { + ids = append(ids, id) + } + usersMap, err := h.usersSvc.LookupByIDs(ctx, ids) + if err != nil { + return nil, fmt.Errorf("failed to fetch users: %w", err) + }(Adjust field names —
c.AuthorID/a.UploadedBy— to match the actual domain types; confirm againstinternal/commentsandinternal/attachments.)#!/bin/bash # Confirm the user-ID fields on Comment and Attachment so the merge uses correct names. ast-grep --pattern 'type Comment struct { $$$ }' ast-grep --pattern 'type Attachment struct { $$$ }' rg -nP '\bAuthorID\b|\bUploadedBy\b' internal/comments internal/attachments🤖 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 349 - 369, prepareDetailsResponse only fetches users for the task (via fetchUsersForTasks) so comment authors and attachment uploaders are missing; collect all unique user IDs (task.AuthorID, task.AssigneeID, every comment.AuthorID, and every attachment.UploadedBy), call the appropriate user lookup (either extend fetchUsersForTasks to accept extra IDs or call a user lookup like fetchUsersByIDs/ListByIDs on the users service), merge the returned users into usersMap, and then pass that enriched usersMap into newTaskDetailsResponse so toCommentsList/toAttachmentsList can resolve full UserBriefs for commenters and uploaders.
🧹 Nitpick comments (1)
internal/server/tasks/handler.go (1)
305-347: Avoid shadowing the importedtaskspackage with a parameter name.In both
fetchUsersForTasks(line 306) andprepareListResponse(line 337), the parameter is namedtasks []tasks.Task, which locally shadows the importedtaskspackage inside those function bodies. It compiles fine today because the package isn't referenced inside, but it's a foot-gun for future edits and hurts readability. Renaming the parameter (e.g.itemsorlist) keeps the package accessible and matches the convention already used infetchUsersForAttachments(items []attachments.Attachment).♻️ Proposed rename
-func (h *Handler) fetchUsersForTasks(ctx context.Context, tasks []tasks.Task) (map[int64]users.User, error) { +func (h *Handler) fetchUsersForTasks(ctx context.Context, items []tasks.Task) (map[int64]users.User, error) { // Collect unique user IDs idSet := make(map[int64]struct{}) - for _, t := range tasks { + for _, t := range items { idSet[t.AuthorID] = struct{}{} if t.AssigneeID != nil { idSet[*t.AssigneeID] = struct{}{} } } @@ func (h *Handler) prepareListResponse( ctx context.Context, - tasks []tasks.Task, + items []tasks.Task, total int, ) (*TaskListResponse, error) { - usersMap, err := h.fetchUsersForTasks(ctx, tasks) + usersMap, err := h.fetchUsersForTasks(ctx, items) if err != nil { return nil, err } - return toTaskListResponse(tasks, usersMap, total), nil + return toTaskListResponse(items, usersMap, total), nil }🤖 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 305 - 347, The parameter name "tasks" in fetchUsersForTasks and prepareListResponse shadows the imported tasks package; rename the parameter (e.g., to "items" or "list") in both functions to avoid package shadowing and match the convention used in fetchUsersForAttachments (items []attachments.Attachment), then update all references inside fetchUsersForTasks and the call site in prepareListResponse (and toTaskListResponse invocation) to use the new parameter name.
🤖 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_attachments.go`:
- Around line 59-65: The helper fetchUsersForAttachments currently wraps
LookupByIDs with the same prefix as its caller, producing duplicate "failed to
fetch users" text; change fetchUsersForAttachments to return the raw error from
LookupByIDs (remove the fmt.Errorf("failed to fetch users: %w") wrap) so caller
(the code that calls fetchUsersForAttachments in handler_attachments.go) can
provide the single contextual wrap using fmt.Errorf("failed to fetch users: %w")
when appropriate; locate and update the error return in fetchUsersForAttachments
and leave the caller's wrapping logic as-is.
---
Duplicate comments:
In `@internal/server/tasks/handler.go`:
- Around line 349-369: prepareDetailsResponse only fetches users for the task
(via fetchUsersForTasks) so comment authors and attachment uploaders are
missing; collect all unique user IDs (task.AuthorID, task.AssigneeID, every
comment.AuthorID, and every attachment.UploadedBy), call the appropriate user
lookup (either extend fetchUsersForTasks to accept extra IDs or call a user
lookup like fetchUsersByIDs/ListByIDs on the users service), merge the returned
users into usersMap, and then pass that enriched usersMap into
newTaskDetailsResponse so toCommentsList/toAttachmentsList can resolve full
UserBriefs for commenters and uploaders.
---
Nitpick comments:
In `@internal/server/tasks/handler.go`:
- Around line 305-347: The parameter name "tasks" in fetchUsersForTasks and
prepareListResponse shadows the imported tasks package; rename the parameter
(e.g., to "items" or "list") in both functions to avoid package shadowing and
match the convention used in fetchUsersForAttachments (items
[]attachments.Attachment), then update all references inside fetchUsersForTasks
and the call site in prepareListResponse (and toTaskListResponse invocation) to
use the new parameter name.
🪄 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: b23edb11-9383-47c0-b0c3-15ba7280a320
📒 Files selected for processing (4)
internal/server/dto/response.gointernal/server/tasks/handler.gointernal/server/tasks/handler_attachments.gointernal/server/tasks/handler_comments.go
✅ 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 (1)
- internal/server/tasks/handler_comments.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/server/tasks/handler_attachments.go (1)
60-63: Avoid failing the request after the upload is already confirmed.At Line 49 the state-changing confirm already succeeds, but Line 60–63 can still return an error due to enrichment lookup. That can surface a 5xx for a successful mutation and trigger confusing client retries.
💡 Suggested change (best-effort enrichment)
- // Fetch user for uploaded_by enrichment - usersMap, err := h.fetchUsersForAttachments(c.Context(), []attachments.Attachment{*attachment}) - if err != nil { - return err - } + // Fetch user for uploaded_by enrichment (best effort; confirm already succeeded) + usersMap := make(map[int64]users.User) + if fetchedUsers, err := h.fetchUsersForAttachments(c.Context(), []attachments.Attachment{*attachment}); err == nil { + usersMap = fetchedUsers + } return c.JSON(toConfirmResponse(attachment, downloadURL, usersMap))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/handler_attachments.go` around lines 60 - 63, The enrichment lookup after the state-changing confirm (the call to h.fetchUsersForAttachments with []attachments.Attachment{*attachment}) must be best-effort: do not return the error from fetchUsersForAttachments (which would surface a 5xx for an already-successful mutation); instead catch any error, log it with context (including the attachment ID and error), and continue processing with a nil/empty usersMap so the request completes successfully. Ensure you reference the call to h.fetchUsersForAttachments, the usersMap variable, and the attachment value so the change is made at the correct spot.
🤖 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/handler_attachments.go`:
- Around line 60-63: The enrichment lookup after the state-changing confirm (the
call to h.fetchUsersForAttachments with []attachments.Attachment{*attachment})
must be best-effort: do not return the error from fetchUsersForAttachments
(which would surface a 5xx for an already-successful mutation); instead catch
any error, log it with context (including the attachment ID and error), and
continue processing with a nil/empty usersMap so the request completes
successfully. Ensure you reference the call to h.fetchUsersForAttachments, the
usersMap variable, and the attachment value so the change is made at the correct
spot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5043860f-ab0a-4800-95e1-ffeda801c5a7
📒 Files selected for processing (1)
internal/server/tasks/handler_attachments.go
capcom6
force-pushed
the
server/fill-user-info
branch
from
a377dd1 to
c5ead8c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/server/tasks/handler.go (1)
363-369:⚠️ Potential issue | 🟠 MajorDetails user map is incomplete for comments and attachments.
prepareDetailsResponseonly resolves users fromtask.AuthorID/task.AssigneeID, then passes that map into comment/attachment DTO mapping. Authors/uploaders outside those two IDs will degrade to stub user briefs.🛠️ Suggested fix
- // Fetch users for author and assignee enrichment - usersMap, err := h.fetchUsersForTasks(ctx, []tasks.Task{*task}) + // Collect all related user IDs (task + comments + attachments) + idSet := map[int64]struct{}{ + task.AuthorID: {}, + } + if task.AssigneeID != nil { + idSet[*task.AssigneeID] = struct{}{} + } + for _, c := range comments { + idSet[c.AuthorID] = struct{}{} + } + for _, a := range attachmentList { + idSet[a.UploadedBy] = struct{}{} + } + + ids := make([]int64, 0, len(idSet)) + for id := range idSet { + ids = append(ids, id) + } + + usersMap, err := h.usersSvc.LookupByIDs(ctx, ids) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to fetch users: %w", err) }🤖 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 363 - 369, Current user lookup only includes task.AuthorID and task.AssigneeID, so comment authors and attachment uploaders fall back to stubs; update the flow that builds the users map (where fetchUsersForTasks is called before newTaskDetailsResponse / prepareDetailsResponse) to also collect all comment AuthorID values and attachment UploaderID values, deduplicate those IDs, and include them when calling fetchUsersForTasks (or call a new helper like fetchUsersByIDs) so the returned usersMap contains full user briefs for comments and attachments as well as the task author/assignee.
🤖 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 52-56: The enrichment lookup currently returns the error from
h.fetchUsersForComments (usersMap, err := h.fetchUsersForComments(...)) which
causes the entire Create/Update mutation to fail even though the DB operation
succeeded; change this to a best-effort pattern: if fetchUsersForComments
returns an error, log the error and continue using an ID-only author stub for
the comment(s) instead of returning the error, and apply the same change in the
updateComment path (the other call around lines 105-109). Ensure you still use
usersMap when available to enrich the author, but never abort the request on
lookup failure.
---
Duplicate comments:
In `@internal/server/tasks/handler.go`:
- Around line 363-369: Current user lookup only includes task.AuthorID and
task.AssigneeID, so comment authors and attachment uploaders fall back to stubs;
update the flow that builds the users map (where fetchUsersForTasks is called
before newTaskDetailsResponse / prepareDetailsResponse) to also collect all
comment AuthorID values and attachment UploaderID values, deduplicate those IDs,
and include them when calling fetchUsersForTasks (or call a new helper like
fetchUsersByIDs) so the returned usersMap contains full user briefs for comments
and attachments as well as the task author/assignee.
🪄 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: 063e6472-a76a-4040-849b-ce915817b9e8
📒 Files selected for processing (12)
internal/server/docs/docs.gointernal/server/dto/response.gointernal/server/tasks/dto.gointernal/server/tasks/dto_attachments.gointernal/server/tasks/dto_comments.gointernal/server/tasks/handler.gointernal/server/tasks/handler_attachments.gointernal/server/tasks/handler_comments.gointernal/users/models.gointernal/users/repository.gointernal/users/service.gorequests.http
✅ Files skipped from review due to trivial changes (2)
- requests.http
- internal/server/docs/docs.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/server/dto/response.go
capcom6
force-pushed
the
server/fill-user-info
branch
from
88b5332 to
11ab06a
Compare
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