[tags] introduce module with project support - #23
Closed
capcom6 wants to merge 2 commits into
Closed
Conversation
📝 WalkthroughWalkthroughThis PR implements a complete project tagging feature. A new tags module is created with domain models, database persistence, and business logic for tag validation and normalization. Projects are extended with tag support in the domain layer, repository (including filtering), and service. The HTTP API layer adds DTO support and handler filtering. Frontend pages display tags in project tables and add a tag filter UI. ChangesProject Tagging Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
🤖 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/projects/repository.go (1)
28-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake project creation atomic with the initial tag write.
If
SetProjectTagsfails after the project insert succeeds,Createreturns an error but still leaves the project row behind without its requested tags. Wrap the project insert and initial tag association write in one transaction so a failed create cannot half-succeed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/projects/repository.go` around lines 28 - 39, The Create flow currently inserts the project via r.db.NewInsert().Model(model).Exec(ctx) and then calls r.SetProjectTags(ctx, slug, tags) separately, which can leave a project without tags on failure; wrap both the insert and the initial tag write in a single DB transaction (use the repository's DB transaction helper or r.db.Begin/RunInTx with ctx) so that if SetProjectTags returns an error the transaction is rolled back and no project row is persisted. Ensure you still detect unique-violation errors from the insert and return ErrNameAlreadyUsed, propagate other errors with the same fmt.Errorf messages, and commit only after SetProjectTags succeeds.
🧹 Nitpick comments (3)
internal/server/projects/dto.go (2)
14-14: 💤 Low valueConsider adding a maximum array size validation.
The current validation validates individual tag length (max=255) but not the total number of tags allowed. Consider adding
max=Nat the array level to prevent clients from sending excessive numbers of tags.🔧 Example validation with array limit
- Tags []string `json:"tags,omitempty" validate:"omitempty,dive,max=255"` + Tags []string `json:"tags,omitempty" validate:"omitempty,max=50,dive,max=255"`(Adjust the array max value based on your requirements)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/server/projects/dto.go` at line 14, The Tags field in the Projects DTO currently validates individual tag length but not the number of tags; update the struct tag on Tags (field name: Tags in internal/server/projects/dto.go) to add an array-level max (e.g., max=50) in the validate tag so the rule becomes something like omitempty,max=<N>,dive,max=255, ensuring the validator enforces both a maximum count and per-tag length; choose N per requirements and run unit/validation tests after the change.
22-22: 💤 Low valueConsider adding a maximum array size validation.
Same as
ProjectRequest.Tags- consider validating the maximum number of tags to prevent unbounded arrays.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/server/projects/dto.go` at line 22, The Tags field on the DTO currently allows an unbounded slice; update its validation tag to enforce a maximum number of tags (matching ProjectRequest.Tags) and still validate each element's max length—e.g., on the Tags `*[]string` field add a `max=<N>` before `dive` (so the tag reads like `validate:"omitempty,max=<N>,dive,max=255"`) to cap the array size while preserving per-tag length checks.internal/tags/repository.go (1)
25-43: ⚡ Quick winPrefer batch insert for better performance.
The current implementation inserts tags one-by-one in a loop, resulting in N database roundtrips. Bun supports batch inserts by passing a slice of models to
Model(), which would reduce this to a single roundtrip.♻️ Refactor to use batch insert
func (r *Repository) EnsureExists(ctx context.Context, names []string) error { + if len(names) == 0 { + return nil + } + now := time.Now() - for _, name := range names { - if _, err := r.db.NewInsert(). - Model(&tagModel{ - BaseModel: schema.BaseModel{}, - TimedModel: bunfx.TimedModel{ - CreatedAt: now, - UpdatedAt: now, - }, - Name: name, - }). - Ignore(). - Exec(ctx); err != nil { - return fmt.Errorf("failed to ensure tag %q: %w", name, err) - } + models := make([]tagModel, len(names)) + for i, name := range names { + models[i] = tagModel{ + BaseModel: schema.BaseModel{}, + TimedModel: bunfx.TimedModel{ + CreatedAt: now, + UpdatedAt: now, + }, + Name: name, + } } - return nil + + if _, err := r.db.NewInsert(). + Model(&models). + Ignore(). + Exec(ctx); err != nil { + return fmt.Errorf("failed to ensure tags: %w", err) + } + + return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tags/repository.go` around lines 25 - 43, The EnsureExists method currently performs N individual inserts in a loop causing multiple DB roundtrips; instead, build a slice of tagModel (populate BaseModel/TimedModel.Name and set CreatedAt/UpdatedAt to now) and pass that slice to r.db.NewInsert().Model(&tagsSlice).Ignore().Exec(ctx) so Bun performs a single batch insert; update the Repository.EnsureExists function to assemble the []tagModel and replace the per-name NewInsert() loop with one batch NewInsert() call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/db/migrations/20260601001838_create_tags.sql`:
- Line 20: Remove the stray separator line containing only `---` in the
migration between the markers `-- +goose StatementEnd` and `-- +goose Down` in
the migration file so the `Up` section ends cleanly and the parser moves
directly to `-- +goose Down`; simply delete that standalone `---` line.
In `@internal/projects/repository.go`:
- Around line 68-76: Normalize filter.Tags by removing empty strings and
deduplicating before building the query: create a normalized slice (e.g.,
normalizedTags) from filter.Tags that trims empties and removes duplicates, then
use normalizedTags for bun.List(...) in the WHERE on projectTagModel and use
len(normalizedTags) in the HAVING("COUNT(DISTINCT pt.tag_name) = ?") instead of
len(filter.Tags); ensure you skip the whole tags branch if normalizedTags is
empty so the HAVING clause never gets a mismatched count.
- Around line 143-165: SetProjectTags currently deletes project_tag rows then
inserts new ones separately; if the insert fails the tags are lost. Wrap the
delete and insert calls (the
r.db.NewDelete().Model((*projectTagModel)(nil)).Where("project_id = ?",
projectID) and r.db.NewInsert().Model(&junctions)) in a single database
transaction (use r.db's transaction API), perform the delete then insert on the
same tx, rollback on any error and return that error, and commit only if both
succeed so the replacement is atomic.
In `@internal/projects/service.go`:
- Around line 81-100: Currently the code calls s.projects.Update(...) before
provisioning tags which can leave partial updates or surface FK errors; first
call s.tags.EnsureExists(ctx, tags) and provision/validate tags (when
update.Tags != nil) before touching the project record, then replace the
two-step Update + SetProjectTags with one atomic repository operation on
s.projects (e.g., add/use a method like UpdateWithTags(ctx, projectSlug, update,
tags) or extend Update to accept tags) so the field update and tag replacement
occur in a single transaction and the repository implementation returns
ErrNotFound for missing slugs instead of foreign-key errors from SetProjectTags.
In `@internal/server/projects/handler.go`:
- Around line 94-101: The tag parsing in the handler (tagsParam, tagList and
projects.ProjectFilter) must remove empty or whitespace-only entries after
splitting and trimming to avoid passing empty tags to the repository; update the
code that builds tagList to filter out elements where strings.TrimSpace(tag) ==
"" (e.g., build a new slice of non-empty tags), and only set filter =
&projects.ProjectFilter{Tags: filteredTags} if filteredTags has length > 0
(otherwise leave filter nil).
In `@internal/tags/doc.go`:
- Around line 19-24: Update the example call to match Service.EnsureExists(ctx
context.Context, names []string) ([]string, error): pass a slice (e.g.,
[]string{"bug","feature"}) to svc.EnsureExists and handle the returned
normalized names and error (assign the two return values, check/handle err, and
use the normalized names); modify the example in internal/tags/doc.go where
svc.EnsureExists is shown so it compiles and demonstrates proper error handling.
---
Outside diff comments:
In `@internal/projects/repository.go`:
- Around line 28-39: The Create flow currently inserts the project via
r.db.NewInsert().Model(model).Exec(ctx) and then calls r.SetProjectTags(ctx,
slug, tags) separately, which can leave a project without tags on failure; wrap
both the insert and the initial tag write in a single DB transaction (use the
repository's DB transaction helper or r.db.Begin/RunInTx with ctx) so that if
SetProjectTags returns an error the transaction is rolled back and no project
row is persisted. Ensure you still detect unique-violation errors from the
insert and return ErrNameAlreadyUsed, propagate other errors with the same
fmt.Errorf messages, and commit only after SetProjectTags succeeds.
---
Nitpick comments:
In `@internal/server/projects/dto.go`:
- Line 14: The Tags field in the Projects DTO currently validates individual tag
length but not the number of tags; update the struct tag on Tags (field name:
Tags in internal/server/projects/dto.go) to add an array-level max (e.g.,
max=50) in the validate tag so the rule becomes something like
omitempty,max=<N>,dive,max=255, ensuring the validator enforces both a maximum
count and per-tag length; choose N per requirements and run unit/validation
tests after the change.
- Line 22: The Tags field on the DTO currently allows an unbounded slice; update
its validation tag to enforce a maximum number of tags (matching
ProjectRequest.Tags) and still validate each element's max length—e.g., on the
Tags `*[]string` field add a `max=<N>` before `dive` (so the tag reads like
`validate:"omitempty,max=<N>,dive,max=255"`) to cap the array size while
preserving per-tag length checks.
In `@internal/tags/repository.go`:
- Around line 25-43: The EnsureExists method currently performs N individual
inserts in a loop causing multiple DB roundtrips; instead, build a slice of
tagModel (populate BaseModel/TimedModel.Name and set CreatedAt/UpdatedAt to now)
and pass that slice to r.db.NewInsert().Model(&tagsSlice).Ignore().Exec(ctx) so
Bun performs a single batch insert; update the Repository.EnsureExists function
to assemble the []tagModel and replace the per-name NewInsert() loop with one
batch NewInsert() call.
🪄 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: 6bba696d-2b9c-4a81-87ba-d973f4b74ad4
📒 Files selected for processing (22)
internal/commands/serve/serve.gointernal/db/migrations/20260601001838_create_tags.sqlinternal/projects/domain.gointernal/projects/models.gointernal/projects/module.gointernal/projects/repository.gointernal/projects/service.gointernal/server/docs/docs.gointernal/server/projects/dto.gointernal/server/projects/handler.gointernal/tags/doc.gointernal/tags/domain.gointernal/tags/errors.gointernal/tags/models.gointernal/tags/module.gointernal/tags/repository.gointernal/tags/service.goweb/static/css/tailwind.cssweb/static/js/app.jsweb/static/pages/admin_projects.htmlweb/static/pages/project_tasks.htmlweb/static/pages/projects.html
💤 Files with no reviewable changes (1)
- internal/projects/module.go
Comment on lines
+68
to
+76
| if filter != nil && len(filter.Tags) > 0 { | ||
| query = query.Where("p.id IN (?)", | ||
| r.db.NewSelect(). | ||
| Column("pt.project_id"). | ||
| Model((*projectTagModel)(nil)). | ||
| Where("pt.tag_name IN (?)", bun.List(filter.Tags)). | ||
| Group("pt.project_id"). | ||
| Having("COUNT(DISTINCT pt.tag_name) = ?", len(filter.Tags)), | ||
| ) |
There was a problem hiding this comment.
Deduplicate and drop empty filter tags before HAVING COUNT(...).
The handler only splits and trims the tags query string, so inputs like bug,bug or bug, reach this code as duplicates/empties. Using len(filter.Tags) in the HAVING clause then makes valid matches disappear. Normalize the slice first and use the normalized count.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/projects/repository.go` around lines 68 - 76, Normalize filter.Tags
by removing empty strings and deduplicating before building the query: create a
normalized slice (e.g., normalizedTags) from filter.Tags that trims empties and
removes duplicates, then use normalizedTags for bun.List(...) in the WHERE on
projectTagModel and use len(normalizedTags) in the HAVING("COUNT(DISTINCT
pt.tag_name) = ?") instead of len(filter.Tags); ensure you skip the whole tags
branch if normalizedTags is empty so the HAVING clause never gets a mismatched
count.
Comment on lines
+143
to
+165
| if _, err := r.db.NewDelete(). | ||
| Model((*projectTagModel)(nil)). | ||
| Where("project_id = ?", projectID). | ||
| Exec(ctx); err != nil { | ||
| return fmt.Errorf("failed to delete existing project tags: %w", err) | ||
| } | ||
|
|
||
| if len(tagNames) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| junctions := make([]projectTagModel, len(tagNames)) | ||
| for i, name := range tagNames { | ||
| junctions[i] = projectTagModel{ | ||
| BaseModel: schema.BaseModel{}, | ||
| ProjectID: projectID, | ||
| TagName: name, | ||
| } | ||
| } | ||
|
|
||
| if _, err := r.db.NewInsert().Model(&junctions).Exec(ctx); err != nil { | ||
| return fmt.Errorf("failed to insert project tags: %w", err) | ||
| } |
There was a problem hiding this comment.
Replace tag associations atomically.
SetProjectTags deletes the current rows before inserting the new ones. If the insert fails, the project loses all existing tags even though the method returns an error. Put the delete+insert in a single transaction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/projects/repository.go` around lines 143 - 165, SetProjectTags
currently deletes project_tag rows then inserts new ones separately; if the
insert fails the tags are lost. Wrap the delete and insert calls (the
r.db.NewDelete().Model((*projectTagModel)(nil)).Where("project_id = ?",
projectID) and r.db.NewInsert().Model(&junctions)) in a single database
transaction (use r.db's transaction API), perform the delete then insert on the
same tx, rollback on any error and return that error, and commit only if both
succeed so the replacement is atomic.
Comment on lines
+81
to
+100
| // Update project fields | ||
| if err := s.projects.Update(ctx, projectSlug, update); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Return updated project | ||
| return s.projects.GetBySlug(ctx, slug) | ||
| // Update tags if provided | ||
| if update.Tags != nil { | ||
| tags := *update.Tags | ||
| if len(tags) > 0 { | ||
| var err error | ||
| if tags, err = s.tags.EnsureExists(ctx, tags); err != nil { | ||
| return nil, fmt.Errorf("failed to ensure tags: %w", err) | ||
| } | ||
| } | ||
| if err := s.projects.SetProjectTags(ctx, projectSlug, tags); err != nil { | ||
| return nil, fmt.Errorf("failed to set project tags: %w", err) | ||
| } | ||
| } | ||
|
|
||
| return s.projects.GetBySlug(ctx, projectSlug) |
There was a problem hiding this comment.
Don't persist project fields before tag provisioning succeeds.
Lines 82-97 update the project first and only then call EnsureExists/SetProjectTags. A tag failure here returns an error after name or repo_url may already be committed, and a tag-only update against a missing slug can surface a foreign-key error instead of ErrNotFound because Repository.Update no-ops when only tags are set. Move tag provisioning/existence checks ahead of the write and make the field update plus tag replacement one atomic repository operation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/projects/service.go` around lines 81 - 100, Currently the code calls
s.projects.Update(...) before provisioning tags which can leave partial updates
or surface FK errors; first call s.tags.EnsureExists(ctx, tags) and
provision/validate tags (when update.Tags != nil) before touching the project
record, then replace the two-step Update + SetProjectTags with one atomic
repository operation on s.projects (e.g., add/use a method like
UpdateWithTags(ctx, projectSlug, update, tags) or extend Update to accept tags)
so the field update and tag replacement occur in a single transaction and the
repository implementation returns ErrNotFound for missing slugs instead of
foreign-key errors from SetProjectTags.
Comment on lines
+94
to
+101
| var filter *projects.ProjectFilter | ||
| if tagsParam := c.Query("tags"); tagsParam != "" { | ||
| tagList := strings.Split(tagsParam, ",") | ||
| for i := range tagList { | ||
| tagList[i] = strings.TrimSpace(tagList[i]) | ||
| } | ||
| filter = &projects.ProjectFilter{Tags: tagList} | ||
| } |
There was a problem hiding this comment.
Filter out empty tag strings after splitting and trimming.
The current implementation doesn't filter empty strings that can arise from:
- Consecutive commas:
?tags=tag1,,tag3 - Trailing commas:
?tags=tag1,tag2, - Whitespace-only elements:
?tags=tag1, ,tag3 - Pure whitespace parameter:
?tags=
Empty strings in the tag filter will be passed to the repository query and could cause incorrect filtering results.
🔧 Proposed fix to filter empty tags
var filter *projects.ProjectFilter
if tagsParam := c.Query("tags"); tagsParam != "" {
tagList := strings.Split(tagsParam, ",")
+ filteredTags := make([]string, 0, len(tagList))
for i := range tagList {
tagList[i] = strings.TrimSpace(tagList[i])
+ if tagList[i] != "" {
+ filteredTags = append(filteredTags, tagList[i])
+ }
}
- filter = &projects.ProjectFilter{Tags: tagList}
+ if len(filteredTags) > 0 {
+ filter = &projects.ProjectFilter{Tags: filteredTags}
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var filter *projects.ProjectFilter | |
| if tagsParam := c.Query("tags"); tagsParam != "" { | |
| tagList := strings.Split(tagsParam, ",") | |
| for i := range tagList { | |
| tagList[i] = strings.TrimSpace(tagList[i]) | |
| } | |
| filter = &projects.ProjectFilter{Tags: tagList} | |
| } | |
| var filter *projects.ProjectFilter | |
| if tagsParam := c.Query("tags"); tagsParam != "" { | |
| tagList := strings.Split(tagsParam, ",") | |
| filteredTags := make([]string, 0, len(tagList)) | |
| for i := range tagList { | |
| tagList[i] = strings.TrimSpace(tagList[i]) | |
| if tagList[i] != "" { | |
| filteredTags = append(filteredTags, tagList[i]) | |
| } | |
| } | |
| if len(filteredTags) > 0 { | |
| filter = &projects.ProjectFilter{Tags: filteredTags} | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/server/projects/handler.go` around lines 94 - 101, The tag parsing
in the handler (tagsParam, tagList and projects.ProjectFilter) must remove empty
or whitespace-only entries after splitting and trimming to avoid passing empty
tags to the repository; update the code that builds tagList to filter out
elements where strings.TrimSpace(tag) == "" (e.g., build a new slice of
non-empty tags), and only set filter = &projects.ProjectFilter{Tags:
filteredTags} if filteredTags has length > 0 (otherwise leave filter nil).
Comment on lines
+19
to
+24
| // app := fx.New( | ||
| // tags.Module(), | ||
| // fx.Invoke(func(svc *tags.Service) { | ||
| // svc.EnsureExists(ctx, "bug", "feature") | ||
| // }), | ||
| // ) |
There was a problem hiding this comment.
Incorrect example code syntax.
The example shows svc.EnsureExists(ctx, "bug", "feature"), but the actual signature is:
func (s *Service) EnsureExists(ctx context.Context, names []string) ([]string, error)The example should pass a slice of strings and handle the returned normalized names and error.
📝 Corrected example
// app := fx.New(
// tags.Module(),
// fx.Invoke(func(svc *tags.Service) {
-// svc.EnsureExists(ctx, "bug", "feature")
+// normalized, err := svc.EnsureExists(ctx, []string{"bug", "feature"})
+// if err != nil {
+// log.Fatal(err)
+// }
+// // normalized contains ["bug", "feature"]
// }),
// )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/tags/doc.go` around lines 19 - 24, Update the example call to match
Service.EnsureExists(ctx context.Context, names []string) ([]string, error):
pass a slice (e.g., []string{"bug","feature"}) to svc.EnsureExists and handle
the returned normalized names and error (assign the two return values,
check/handle err, and use the normalized names); modify the example in
internal/tags/doc.go where svc.EnsureExists is shown so it compiles and
demonstrates proper error handling.
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
Release Notes