[projects] introduce module#3
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds a new "projects" feature: DB migration, domain models and validation, Bun repository and service with slugging/pagination, FX wiring and named logger, Fiber HTTP handler and DTOs with auth/role rules, OpenAPI docs, request examples, and two new go.mod deps. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant Service
participant Repository
participant Database
rect rgba(100,200,100,0.5)
Note over Client,Database: Create Project Flow
Client->>Handler: POST /projects {name, repo_url}
Handler->>Service: Create(ProjectInput)
Service->>Service: Validate + slug.Make(name)
Service->>Repository: Create(ProjectInput, slug)
Repository->>Database: INSERT INTO projects (...)
Database-->>Repository: success / constraint error
Repository-->>Service: *Project or ErrNameAlreadyUsed
Service-->>Handler: *Project or error
Handler-->>Client: 201 Created / error
end
rect rgba(100,150,200,0.5)
Note over Client,Database: List Projects Flow
Client->>Handler: GET /projects?limit&offset
Handler->>Service: List(limit, offset)
Service->>Repository: List(limit, offset)
Repository->>Database: SELECT ... ORDER BY name ASC LIMIT/OFFSET
Database-->>Repository: []projectModel
Repository-->>Service: []Project
Service->>Repository: Count()
Repository->>Database: SELECT COUNT(*)
Database-->>Repository: int64
Repository-->>Service: int64
Service-->>Handler: items + total
Handler-->>Client: 200 OK
end
rect rgba(200,100,100,0.5)
Note over Client,Database: Update Project Flow
Client->>Handler: PATCH /projects/{slug} {name?, repo_url?}
Handler->>Service: Update(slug, ProjectUpdate)
Service->>Service: Validate non-nil fields
Service->>Repository: Update(slug, ProjectUpdate)
Repository->>Database: UPDATE projects SET ...
Database-->>Repository: result / constraint error
Repository-->>Service: nil or ErrNameAlreadyUsed or ErrNotFound
Service->>Repository: GetBySlug(slug)
Repository->>Database: SELECT * FROM projects WHERE id=?
Database-->>Repository: *projectModel
Repository-->>Service: *Project
Service-->>Handler: *Project or error
Handler-->>Client: 200 OK / error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
internal/projects/errors.go (1)
16-18: Align URL error contract with actual validation behavior.Line 17 says “HTTPS or SSH”, but current validation only allows HTTPS. Please either update this comment/error contract or extend validator behavior to match.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/projects/errors.go` around lines 16 - 18, The ErrInvalidURL comment and error text claim “must be HTTPS or SSH”, but the validator only accepts HTTPS; either update the comment/error or extend the validator to accept SSH URLs. Locate the ErrInvalidURL constant and either (A) change the comment and error string to reflect HTTPS-only behavior (e.g., remove “or SSH”), or (B) update the repository URL validation function (e.g., ValidateRepoURL / IsValidRepoURL or whichever function enforces URL formats) to also accept SSH patterns (git@...:repo.git and ssh://...) and add tests for SSH cases; ensure the chosen approach keeps the ErrInvalidURL contract consistent with the validator.internal/server/projects/handler.go (1)
85-107: Consider combining list and count into a single service call.The handler makes two separate service calls (
List+Count), which results in two database queries. For better performance and consistency, consider adding aListWithCountmethod to the service that returns both in a single operation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/projects/handler.go` around lines 85 - 107, The handler currently calls projectsSvc.List and projectsSvc.Count separately causing two DB queries; change Handler.list to call a new service method ListWithCount(ctx, limit, offset) that returns (projects, total, error) and use its results to build NewProjectListResponse. Add ListWithCount to the projects service interface and implement it in the concrete service/repository so it performs a single DB query (or a single transaction) that returns both rows and total count; update any callers/tests to use the new method and remove the separate List/Count usage in Handler.list.internal/projects/service.go (1)
33-40: Consider validating slug generation result.
slug.Make()can produce an empty string or identical slugs for different inputs (e.g., names with only special characters, or "My Project" vs "my-project"). While the database unique constraint will catch collisions, an empty slug would create a project with an empty ID.Consider adding a check for empty or invalid slugs before persisting.
♻️ Proposed validation
func (s *Service) Create(ctx context.Context, input ProjectInput) (*Project, error) { if err := input.Validate(); err != nil { return nil, err } + projectSlug := slug.Make(input.Name) + if projectSlug == "" { + return nil, fmt.Errorf("%w: project name produces invalid slug", ErrValidationFailed) + } + // Create the project - return s.projects.Create(ctx, input, slug.Make(input.Name)) + return s.projects.Create(ctx, input, projectSlug) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/projects/service.go` around lines 33 - 40, In Service.Create, after calling slug.Make(input.Name) (used in the return call to s.projects.Create), validate the generated slug: if slug == "" or fails your slug format rules, return a clear validation error instead of persisting; to avoid collisions also attempt a uniqueness strategy before calling s.projects.Create — e.g., generate base := slug.Make(input.Name), if base == "" return error, then check s.projects.Exists(ctx, base) (or call s.projects.FindBySlug) and if it exists append a deterministic or random suffix (base + "-1" or base + "-" + shortRand) in a loop until a unique slug is found, then pass that final slug into s.projects.Create. Ensure you reference Service.Create, slug.Make(input.Name), and s.projects.Create when implementing the checks.
🤖 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/projects/domain.go`:
- Around line 35-75: The Validate methods currently trim values only for checks
and do not persist the normalized data, so change ProjectInput.Validate and
ProjectUpdate.Validate to have pointer receivers (func (i *ProjectInput)
Validate(...) and func (u *ProjectUpdate) Validate(...)) and assign the trimmed
strings back into the struct (set i.Name and i.RepoURL for ProjectInput; update
*u.Name and *u.RepoURL for ProjectUpdate when non-nil) before running validation
logic so downstream code receives the normalized values; leave
ProjectUpdate.IsEmpty as-is.
- Around line 84-87: The url.Parse error returned from domain.go should be
wrapped with the project's validation sentinel so the errorsHandler maps it to a
4xx; replace the plain fmt.Errorf return in the block that calls
url.Parse(repoURL) with a wrapped error using the validation sentinel (e.g.
return fmt.Errorf("%w: failed to parse repository URL: %v", ErrValidation, err))
so the parse failure is identifiable as a client validation error by the
errorsHandler.
In `@internal/projects/repository.go`:
- Around line 141-153: The Exists method in Repository incorrectly scans the
string "id" column into a bool (in function Exists and model projectModel);
replace the current NewSelect().Model(...).Where("id = ?",
slug).Column("id").Scan(...) logic with Bun's Exists call: call
r.db.NewSelect().Model((*projectModel)(nil)).Where("id = ?", slug).Exists(ctx)
and return that boolean along with any error, removing the Column("id")/Scan
usage so the query performs a proper SQL EXISTS check.
In `@internal/projects/service.go`:
- Around line 78-91: Add slug validation to Service.Update: before calling
update.Validate() or s.projects.Update, check that the slug string is non-empty
(same validation used by GetBySlug/Delete/Exists) and return a clear error if it
is empty. Locate the Update method on type Service and mirror the non-empty slug
check used in GetBySlug/Delete/Exists (or reuse the same helper/constant if one
exists) so Update returns an error immediately for an empty slug instead of
proceeding to s.projects.Update or s.projects.GetBySlug.
In `@internal/server/docs/docs.go`:
- Around line 511-517: The OpenAPI output contains an invalid schema ("type":
"No") for the 204 No Content response because the handler swagger annotation
incorrectly specified a response type; locate the handler annotation that
contains "@Success 204 No Content" (the annotation currently producing the bad
schema) and remove any type/schema specification for the 204 response or change
it to the minimal form that indicates no body (e.g. leave only "@Success 204 No
Content" or remove the `@Success` line entirely), then regenerate the docs so
docs.go no longer includes a schema for the 204 response.
---
Nitpick comments:
In `@internal/projects/errors.go`:
- Around line 16-18: The ErrInvalidURL comment and error text claim “must be
HTTPS or SSH”, but the validator only accepts HTTPS; either update the
comment/error or extend the validator to accept SSH URLs. Locate the
ErrInvalidURL constant and either (A) change the comment and error string to
reflect HTTPS-only behavior (e.g., remove “or SSH”), or (B) update the
repository URL validation function (e.g., ValidateRepoURL / IsValidRepoURL or
whichever function enforces URL formats) to also accept SSH patterns
(git@...:repo.git and ssh://...) and add tests for SSH cases; ensure the chosen
approach keeps the ErrInvalidURL contract consistent with the validator.
In `@internal/projects/service.go`:
- Around line 33-40: In Service.Create, after calling slug.Make(input.Name)
(used in the return call to s.projects.Create), validate the generated slug: if
slug == "" or fails your slug format rules, return a clear validation error
instead of persisting; to avoid collisions also attempt a uniqueness strategy
before calling s.projects.Create — e.g., generate base := slug.Make(input.Name),
if base == "" return error, then check s.projects.Exists(ctx, base) (or call
s.projects.FindBySlug) and if it exists append a deterministic or random suffix
(base + "-1" or base + "-" + shortRand) in a loop until a unique slug is found,
then pass that final slug into s.projects.Create. Ensure you reference
Service.Create, slug.Make(input.Name), and s.projects.Create when implementing
the checks.
In `@internal/server/projects/handler.go`:
- Around line 85-107: The handler currently calls projectsSvc.List and
projectsSvc.Count separately causing two DB queries; change Handler.list to call
a new service method ListWithCount(ctx, limit, offset) that returns (projects,
total, error) and use its results to build NewProjectListResponse. Add
ListWithCount to the projects service interface and implement it in the concrete
service/repository so it performs a single DB query (or a single transaction)
that returns both rows and total count; update any callers/tests to use the new
method and remove the separate List/Count usage in Handler.list.
🪄 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: 6b7935e8-df0b-414d-a2a3-facc79b0c5d2
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
go.modinternal/app.gointernal/db/migrations/20260413013944_create_projects.sqlinternal/projects/config.gointernal/projects/doc.gointernal/projects/domain.gointernal/projects/errors.gointernal/projects/models.gointernal/projects/module.gointernal/projects/repository.gointernal/projects/service.gointernal/server/docs/docs.gointernal/server/module.gointernal/server/projects/dto.gointernal/server/projects/handler.gorequests.http
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/server/projects/handler.go (2)
52-67: Route parameter:idis misleading; consider renaming to:slug.The route parameter is named
:id, but the service layer uses slug-based lookups (GetBySlug,Update(slug),Delete(slug)). This creates confusion for API consumers expecting a numeric or UUID identifier. The Swagger documentation at lines 117, 175, and 210 also describes it as "Project ID" which compounds the inconsistency.♻️ Suggested fix
- projects.Get("/:id", h.get) + projects.Get("/:slug", h.get) // Admin-only routes (require admin role) projects.Post("/", jwtauth.WithRole(users.RoleAdmin), validation.DecorateWithBodyEx(h.Validator, h.post), ) - projects.Patch("/:id", + projects.Patch("/:slug", jwtauth.WithRole(users.RoleAdmin), validation.DecorateWithBodyEx(h.Validator, h.patch), ) - projects.Delete("/:id", + projects.Delete("/:slug", jwtauth.WithRole(users.RoleAdmin), h.delete, )Also update the handler methods to use
c.Params("slug")and fix the Swagger@Paramannotations to describe "Project slug" instead of "Project ID".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/projects/handler.go` around lines 52 - 67, Route parameters are named ":id" but the service uses slugs (GetBySlug, Update(slug), Delete(slug)), causing confusion; rename route params to ":slug" in the route definitions (projects.Get("/:slug", h.get), projects.Patch("/:slug", ...), projects.Delete("/:slug", ...)) and update the handler methods to read c.Params("slug") instead of "id" (e.g., in h.get, h.patch, h.delete) and adjust the Swagger `@Param` annotations to say "Project slug" (replace "Project ID") so docs match implementation.
70-84: Duplicate method description comments.Lines 70-71 contain the doc comment "list retrieves a paginated list of all projects" which is repeated at line 84. The same pattern appears for
get(lines 109/124),post(lines 137/153),patch(lines 167/185), anddelete(lines 201-202/218). Consider keeping only the Swagger annotation block and removing the duplicate plain comments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/projects/handler.go` around lines 70 - 84, Remove the duplicated plain comment lines that repeat the method descriptions for the handler methods list, get, post, patch, and delete so only the Swagger annotation block remains; locate the comment blocks immediately above each handler function (functions named list, get, post, patch, delete) and delete the extra single-line comment (e.g., "list retrieves a paginated list of all projects") that appears after the Swagger annotations, leaving the annotated docs intact.
🤖 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/projects/handler.go`:
- Around line 52-67: Route parameters are named ":id" but the service uses slugs
(GetBySlug, Update(slug), Delete(slug)), causing confusion; rename route params
to ":slug" in the route definitions (projects.Get("/:slug", h.get),
projects.Patch("/:slug", ...), projects.Delete("/:slug", ...)) and update the
handler methods to read c.Params("slug") instead of "id" (e.g., in h.get,
h.patch, h.delete) and adjust the Swagger `@Param` annotations to say "Project
slug" (replace "Project ID") so docs match implementation.
- Around line 70-84: Remove the duplicated plain comment lines that repeat the
method descriptions for the handler methods list, get, post, patch, and delete
so only the Swagger annotation block remains; locate the comment blocks
immediately above each handler function (functions named list, get, post, patch,
delete) and delete the extra single-line comment (e.g., "list retrieves a
paginated list of all projects") that appears after the Swagger annotations,
leaving the annotated docs intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 765ffb94-7d8b-4c0b-b87a-f23bb3f6cd11
📒 Files selected for processing (5)
internal/projects/domain.gointernal/projects/errors.gointernal/projects/repository.gointernal/server/docs/docs.gointernal/server/projects/handler.go
✅ Files skipped from review due to trivial changes (3)
- internal/projects/errors.go
- internal/server/docs/docs.go
- internal/projects/repository.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/projects/domain.go
🤖 Pull request artifacts
|
capcom6
force-pushed
the
projects/introduce-module
branch
from
856d821 to
ce47d9e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/projects/domain.go (1)
35-75:⚠️ Potential issue | 🟠 MajorPersist normalized fields after validation.
On Line 35 and Line 57, both validators trim values only for checks, but the normalized values are not written back. That allows whitespace-padded
Name/RepoURLto pass validation and still be persisted unnormalized.Proposed fix
-func (i ProjectInput) Validate() error { +func (i *ProjectInput) Validate() error { // Trim and validate name name := strings.TrimSpace(i.Name) if name == "" { return fmt.Errorf("%w: project name is required", ErrValidationFailed) } + i.Name = name // Validate repository URL repoURL := strings.TrimSpace(i.RepoURL) if err := validateRepoURL(repoURL); err != nil { return err } + i.RepoURL = repoURL return nil } -func (u ProjectUpdate) Validate() error { +func (u *ProjectUpdate) Validate() error { if u.Name != nil { // Trim and validate name name := strings.TrimSpace(*u.Name) if name == "" { return fmt.Errorf("%w: project name is required", ErrValidationFailed) } + *u.Name = name } if u.RepoURL != nil { // Validate repository URL repoURL := strings.TrimSpace(*u.RepoURL) if err := validateRepoURL(repoURL); err != nil { return err } + *u.RepoURL = repoURL } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/projects/domain.go` around lines 35 - 75, The validators trim inputs for checks but never write the normalized values back, so whitespace-padded Name/RepoURL can pass validation yet be persisted unnormalized; fix ProjectInput.Validate by changing its receiver to a pointer (func (i *ProjectInput) Validate()) and after trimming assign the normalized strings back to i.Name and i.RepoURL before returning, and in ProjectUpdate.Validate (preferably change to func (u *ProjectUpdate) Validate()) after trimming set *u.Name = name and *u.RepoURL = repoURL for the non-nil pointer fields so the cleaned values are persisted.
🧹 Nitpick comments (1)
internal/projects/domain.go (1)
15-24: Align RepoURL contract text with actual validation behavior.Line 15/Line 24 say “SSH or HTTPS”, but Line 90 currently allows only
https. Please make comments and validation rules consistent (either implement SSH support or update wording to HTTPS-only).Also applies to: 77-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/projects/domain.go` around lines 15 - 24, The RepoURL field comment in ProjectInput currently states "SSH or HTTPS" but the actual validation enforces HTTPS-only; update the contract text to reflect HTTPS-only (e.g., "HTTPS repository URL") and also update any other comments at the same block (lines referenced around ProjectInput and the earlier struct) to match, and ensure the validation function that enforces the scheme (the repo URL validator used by ProjectInput validation — e.g., validateRepoURL or ProjectInput.Validate) remains consistent; alternatively, if you prefer SSH support, implement SSH handling in that validator (accept ssh:// or git@... formats) and adjust the ProjectInput.RepoURL comment accordingly.
🤖 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/projects/domain.go`:
- Around line 35-75: The validators trim inputs for checks but never write the
normalized values back, so whitespace-padded Name/RepoURL can pass validation
yet be persisted unnormalized; fix ProjectInput.Validate by changing its
receiver to a pointer (func (i *ProjectInput) Validate()) and after trimming
assign the normalized strings back to i.Name and i.RepoURL before returning, and
in ProjectUpdate.Validate (preferably change to func (u *ProjectUpdate)
Validate()) after trimming set *u.Name = name and *u.RepoURL = repoURL for the
non-nil pointer fields so the cleaned values are persisted.
---
Nitpick comments:
In `@internal/projects/domain.go`:
- Around line 15-24: The RepoURL field comment in ProjectInput currently states
"SSH or HTTPS" but the actual validation enforces HTTPS-only; update the
contract text to reflect HTTPS-only (e.g., "HTTPS repository URL") and also
update any other comments at the same block (lines referenced around
ProjectInput and the earlier struct) to match, and ensure the validation
function that enforces the scheme (the repo URL validator used by ProjectInput
validation — e.g., validateRepoURL or ProjectInput.Validate) remains consistent;
alternatively, if you prefer SSH support, implement SSH handling in that
validator (accept ssh:// or git@... formats) and adjust the ProjectInput.RepoURL
comment accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a1551c5-432c-4c34-a755-86366ebb0d85
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
go.modinternal/app.gointernal/db/migrations/20260413013944_create_projects.sqlinternal/projects/config.gointernal/projects/doc.gointernal/projects/domain.gointernal/projects/errors.gointernal/projects/models.gointernal/projects/module.gointernal/projects/repository.gointernal/projects/service.gointernal/server/docs/docs.gointernal/server/module.gointernal/server/projects/dto.gointernal/server/projects/handler.gorequests.http
✅ Files skipped from review due to trivial changes (7)
- internal/projects/config.go
- go.mod
- internal/app.go
- internal/projects/errors.go
- internal/db/migrations/20260413013944_create_projects.sql
- internal/projects/doc.go
- internal/server/docs/docs.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/projects/module.go
- internal/projects/models.go
- requests.http
- internal/projects/service.go
- internal/projects/repository.go
- internal/server/projects/handler.go
capcom6
force-pushed
the
projects/introduce-module
branch
from
ce47d9e to
143b3cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/projects/repository.go (1)
143-151: Simplify theExistsmethod.The
.Column("id")call is redundant since Bun'sExists()wraps the query in aSELECT EXISTS(...)subquery regardless of selected columns. Additionally, Bun'sExists()returnsfalsefor no matching rows without returningsql.ErrNoRows, making that check unnecessary.♻️ Simplified implementation
func (r *Repository) Exists(ctx context.Context, slug string) (bool, error) { exists, err := r.db.NewSelect().Model((*projectModel)(nil)). Where("id = ?", slug). - Column("id"). Exists(ctx) - if err != nil && !errors.Is(err, sql.ErrNoRows) { + if err != nil { return false, fmt.Errorf("failed to check project existence: %w", err) } return exists, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/projects/repository.go` around lines 143 - 151, The Exists method can be simplified: in Repository.Exists use r.db.NewSelect().Model((*projectModel)(nil)).Where("id = ?", slug).Exists(ctx) without the redundant .Column("id") and remove the unnecessary errors.Is(err, sql.ErrNoRows) branch since Exists returns false for no match; simply check if err != nil and wrap/return the error (e.g., return false, fmt.Errorf(...)) otherwise return exists, nil.
🤖 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/projects/repository.go`:
- Around line 109-115: The current check using result.RowsAffected() to return
ErrNotFound is unreliable for MySQL because RowsAffected can be 0 for no-op
updates; modify the update flow in the function that calls result.RowsAffected()
so that you first perform a pre-flight existence check (e.g., SELECT 1 FROM
projects WHERE id = ?), return ErrNotFound if that SELECT finds no row, then run
the UPDATE and ignore a 0 RowsAffected as a valid no-op; keep the ErrNotFound
return only for the pre-flight SELECT failure and remove the RowsAffected==0 ->
ErrNotFound branch.
In `@internal/server/projects/dto.go`:
- Around line 32-40: NewProjectResponse currently dereferences p without
checking for nil which can panic; update the function NewProjectResponse to
early-check if p == nil and return a safe zero-value ProjectResponse (or
sensible defaults) instead of accessing fields on a nil *projects.Project;
ensure you reference NewProjectResponse and the projects.Project input when
adding the defensive nil check and populate CreatedAt/UpdatedAt safely (e.g.,
empty strings) when p is nil.
---
Nitpick comments:
In `@internal/projects/repository.go`:
- Around line 143-151: The Exists method can be simplified: in Repository.Exists
use r.db.NewSelect().Model((*projectModel)(nil)).Where("id = ?",
slug).Exists(ctx) without the redundant .Column("id") and remove the unnecessary
errors.Is(err, sql.ErrNoRows) branch since Exists returns false for no match;
simply check if err != nil and wrap/return the error (e.g., return false,
fmt.Errorf(...)) otherwise return exists, nil.
🪄 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: 80aee547-f060-4bde-92d4-cbe57a7ea764
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
go.modinternal/app.gointernal/db/migrations/20260413013944_create_projects.sqlinternal/projects/config.gointernal/projects/doc.gointernal/projects/domain.gointernal/projects/errors.gointernal/projects/models.gointernal/projects/module.gointernal/projects/repository.gointernal/projects/service.gointernal/server/docs/docs.gointernal/server/module.gointernal/server/projects/dto.gointernal/server/projects/handler.gorequests.http
✅ Files skipped from review due to trivial changes (9)
- internal/app.go
- go.mod
- internal/projects/config.go
- internal/projects/doc.go
- internal/projects/models.go
- internal/server/module.go
- internal/projects/errors.go
- internal/db/migrations/20260413013944_create_projects.sql
- internal/server/docs/docs.go
🚧 Files skipped from review as they are similar to previous changes (2)
- requests.http
- internal/projects/service.go
capcom6
force-pushed
the
projects/introduce-module
branch
from
143b3cd to
25cdaf0
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
internal/server/projects/dto.go (1)
32-39:⚠️ Potential issue | 🔴 CriticalGuard
NewProjectResponseagainst nil input to prevent panic.
pis dereferenced unconditionally. A nil value will panic and break the request path.🐛 Proposed fix
func NewProjectResponse(p *projects.Project) ProjectResponse { + if p == nil { + return ProjectResponse{} + } return ProjectResponse{ ID: p.ID, Name: p.Name, RepoURL: p.RepoURL, CreatedAt: p.CreatedAt.Format(time.RFC3339), UpdatedAt: p.UpdatedAt.Format(time.RFC3339), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/projects/dto.go` around lines 32 - 39, NewProjectResponse currently dereferences p without a nil check which will panic if called with nil; update NewProjectResponse to guard for nil by returning a zero-value ProjectResponse (or an appropriate empty response with zero/empty fields) when p == nil, otherwise populate ID, Name, RepoURL and formatted CreatedAt/UpdatedAt from the non-nil *projects.Project; ensure you reference NewProjectResponse, ProjectResponse and the input type *projects.Project when making the change.internal/projects/service.go (1)
78-90:⚠️ Potential issue | 🟡 MinorAdd missing slug validation in
Updatefor consistency.
Updateshould reject empty slugs likeGetBySlug,Delete, andExistsdo.🐛 Proposed fix
func (s *Service) Update(ctx context.Context, slug string, update ProjectUpdate) (*Project, error) { + if slug == "" { + return nil, fmt.Errorf("%w: slug is required", ErrValidationFailed) + } + // Validate update data if err := update.Validate(); err != nil { return nil, err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/projects/service.go` around lines 78 - 90, Update the Service.Update method to reject an empty slug like the other methods do: add a pre-check at the top of Service.Update that returns an error when slug == "" (same pattern used by GetBySlug, Delete, and Exists) before calling update.Validate(), then proceed to call s.projects.Update and s.projects.GetBySlug; ensure the returned error type/message matches the project's existing slug-missing error convention used in GetBySlug/Delete/Exists.internal/projects/repository.go (1)
109-115:⚠️ Potential issue | 🟠 Major
RowsAffected()==0is not a reliable not-found signal for MySQL updates.This can misclassify valid no-op updates as
ErrNotFound. Do a preflight existence check (or ensureclientFoundRows=true) and avoidrows==0as the not-found condition.🐛 Proposed fix (preflight exists)
func (r *Repository) Update(ctx context.Context, slug string, update ProjectUpdate) error { if update.IsEmpty() { return nil } + exists, err := r.Exists(ctx, slug) + if err != nil { + return fmt.Errorf("failed to check project existence: %w", err) + } + if !exists { + return ErrNotFound + } + query := r.db.NewUpdate().Model((*projectModel)(nil)).Where("id = ?", slug) if update.Name != nil { query = query.Set("name = ?", *update.Name) } if update.RepoURL != nil { query = query.Set("repo_url = ?", *update.RepoURL) } - result, err := query.Exec(ctx) + _, err = query.Exec(ctx) if err != nil { if db.IsUniqueViolation(err) { return ErrNameAlreadyUsed } return fmt.Errorf("failed to update project: %w", err) } - - rows, err := result.RowsAffected() - if err != nil { - return fmt.Errorf("failed to get affected rows: %w", err) - } - if rows == 0 { - return ErrNotFound - } return nil }#!/bin/bash # Verify DSN/options and locate other RowsAffected-based not-found checks. rg -n --iglob '*.go' 'RowsAffected\(\)|clientFoundRows|sql\.Open|mysql' rg -n --iglob '*.{env,example,yml,yaml,json}' 'clientFoundRows|DATABASE_URL|DSN'Based on learnings: In the
bit-issues/backendrepository (MySQL),RowsAffected()returns 0 both when no row matches and when UPDATE values are unchanged, soRowsAffected()==0can produce falseErrNotFound.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/projects/repository.go` around lines 109 - 115, The current use of result.RowsAffected() to return ErrNotFound is unreliable for MySQL because RowsAffected==0 can mean a no-op update; modify the update path that uses result.RowsAffected() (the code referencing result.RowsAffected(), ErrNotFound in internal/projects/repository.go) to perform a preflight existence check (e.g., SELECT 1 or SELECT id WHERE id=? before attempting UPDATE) and only return ErrNotFound when the preflight indicates the row does not exist, or alternatively ensure the DB client is opened with clientFoundRows=true so RowsAffected distinguishes unchanged rows—update the logic to consult that preflight (or clientFoundRows behavior) instead of directly treating rows==0 as not-found.
🤖 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/projects/repository.go`:
- Around line 109-115: The current use of result.RowsAffected() to return
ErrNotFound is unreliable for MySQL because RowsAffected==0 can mean a no-op
update; modify the update path that uses result.RowsAffected() (the code
referencing result.RowsAffected(), ErrNotFound in
internal/projects/repository.go) to perform a preflight existence check (e.g.,
SELECT 1 or SELECT id WHERE id=? before attempting UPDATE) and only return
ErrNotFound when the preflight indicates the row does not exist, or
alternatively ensure the DB client is opened with clientFoundRows=true so
RowsAffected distinguishes unchanged rows—update the logic to consult that
preflight (or clientFoundRows behavior) instead of directly treating rows==0 as
not-found.
In `@internal/projects/service.go`:
- Around line 78-90: Update the Service.Update method to reject an empty slug
like the other methods do: add a pre-check at the top of Service.Update that
returns an error when slug == "" (same pattern used by GetBySlug, Delete, and
Exists) before calling update.Validate(), then proceed to call s.projects.Update
and s.projects.GetBySlug; ensure the returned error type/message matches the
project's existing slug-missing error convention used in
GetBySlug/Delete/Exists.
In `@internal/server/projects/dto.go`:
- Around line 32-39: NewProjectResponse currently dereferences p without a nil
check which will panic if called with nil; update NewProjectResponse to guard
for nil by returning a zero-value ProjectResponse (or an appropriate empty
response with zero/empty fields) when p == nil, otherwise populate ID, Name,
RepoURL and formatted CreatedAt/UpdatedAt from the non-nil *projects.Project;
ensure you reference NewProjectResponse, ProjectResponse and the input type
*projects.Project when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c1713a25-e632-46d3-80ac-b3f8799cf53b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
go.modinternal/app.gointernal/db/migrations/20260413013944_create_projects.sqlinternal/projects/config.gointernal/projects/doc.gointernal/projects/domain.gointernal/projects/errors.gointernal/projects/models.gointernal/projects/module.gointernal/projects/repository.gointernal/projects/service.gointernal/server/docs/docs.gointernal/server/module.gointernal/server/projects/dto.gointernal/server/projects/handler.gorequests.http
✅ Files skipped from review due to trivial changes (7)
- internal/projects/config.go
- go.mod
- internal/app.go
- internal/db/migrations/20260413013944_create_projects.sql
- internal/projects/doc.go
- internal/projects/errors.go
- requests.http
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/projects/module.go
- internal/server/docs/docs.go
- internal/projects/domain.go
- internal/server/projects/handler.go
This was referenced Apr 16, 2026
Merged
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
Validation & Business Logic
Documentation
Chores