Conversation
Add ItemCategory model with JSONB-based i18n names/descriptions, CategoryService (CRUD + EnsureCategory auto-creation), REST API endpoints, and wire EnsureCategory into all item write paths (CreateItem, UpdateItem, sync, archive upload). Existing items' category strings remain unchanged; a Goose migration seeds the new table from existing distinct category values. Signed-off-by: 林凯90331 <90331@sangfor.com> Co-authored-by: CoStrict <zgsm@sangfor.com.cn>
The ListAllItems endpoint was missing per-user favorited status, causing the UI to show unfavorited state even after a user favorited an item. Batch-query ItemFavorite table for the current user and populate the favorited boolean in the response, consistent with GetItem behavior. Signed-off-by: 林凯90331 <90331@sangfor.com> Co-authored-by: CoStrict <zgsm@sangfor.com.cn>
📝 WalkthroughWalkthroughThis PR introduces a comprehensive item categorization system comprising a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 11
🧹 Nitpick comments (1)
server/internal/handlers/capability_item_test.go (1)
41-41: Add at least one test route with a non-nilcategorySvc.Using
nilhere bypasses the newly added category auto-provisioning path; consider dedicated create/update tests that assertEnsureCategorybehavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/handlers/capability_item_test.go` at line 41, The test currently constructs ItemHandler with a nil categorySvc which skips the new category auto-provisioning path; update the test to create and pass a non-nil mock or real CategoryService into NewItemHandler (replace the nil argument) and add at least one route/test case exercising create/update that triggers EnsureCategory on the handler (assert CategoryService.EnsureCategory was called and that categories are created/updated as expected). Use NewItemHandler, the ItemHandler create/update route under test, and the EnsureCategory method on your CategoryService mock to locate and wire the behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/proposals/ITEM_CATEGORY_DESIGN.md`:
- Around line 47-56: The fenced code blocks in ITEM_CATEGORY_DESIGN.md
(including the ASCII diagram block showing CapabilityItem → ItemCategory and the
other blocks at ranges 113-115, 138-140, 153-163, 171-180, 188-190) need
explicit language tags to satisfy markdownlint MD040; update each
triple-backtick fence to include an appropriate language identifier (e.g.,
```text for ASCII diagrams, ```json for JSON examples, ```sql for SQL snippets,
```http for HTTP examples, or ```go where applicable) so the fences render
correctly and the linter stops flagging them.
- Around line 28-33: The backfill should normalize existing
CapabilityItem.Category values into canonical slugs rather than inserting raw
category text into the dictionary.slug, so update the backfill script to
slugify/lowercase/trim each DISTINCT category before inserting into the
dictionary and store the original raw string only inside dictionary.names (or
names metadata) for display; ensure the dictionary unique index is applied to
the normalized slug value (not the raw category), and update any references in
the migration/backfill routine that currently write to slug (and the unique
constraint) to use the normalized slug generation function so legacy variants
like "Development" and "development" collapse to one entry.
- Around line 58-60: Deletion of ItemCategory currently allows hard deletes
which can orphan CapabilityItem.Category slugs; update the API and data-layer to
protect deletes by checking references to ItemCategory.Slug: when handling
DELETE for a category, query CapabilityItem (filtering CapabilityItem.Category
== ItemCategory.Slug) and if any exist return 409 Conflict with a clear error;
alternatively support an atomic "reassign" flow that updates all matching
CapabilityItem.Category slugs to a new slug inside a transaction before deleting
the ItemCategory; ensure EnsureCategory semantics and any delete handlers
enforce this guard so deletes cannot leave dangling slugs.
In `@server/cmd/worker/main.go`:
- Around line 83-87: The worker enables CategorySvc in main.go but the migration
set used during worker bootstrap omits the item_categories table; update the
worker's schema/bootstrap migration sequence (the function that runs migrations
for the worker—e.g., the runMigrations/bootstrapMigrations/RegisterMigrations
routine used during worker startup) to include the item_categories table
creation migration so environments that self-bootstrap will create
item_categories before CategorySvc is used; reference CategorySvc in the worker
init to ensure ordering so the migration runs prior to any category
provisioning.
In `@server/internal/handlers/capability_item.go`:
- Around line 371-373: Normalize the category string before assigning it to
item.Category (apply the same trimming/canonicalization that EnsureCategory
uses), then call CategorySvc.EnsureCategory synchronously inside the same write
transaction and return/abort if it returns an error instead of
fire-and-for-getting it; update all occurrences where CategorySvc.EnsureCategory
is currently invoked (the code paths using EnsureCategory around the symbols
item.Category and CategorySvc, including the other sites noted) to
validate/assign the canonical value first and propagate any EnsureCategory error
to prevent saving mismatched or orphaned rows.
In `@server/internal/services/category_service.go`:
- Around line 41-43: Normalize slugs by trimming and lowercasing before any
lookup or creation: in EnsureCategory, replace slug = strings.TrimSpace(slug)
with slug = strings.ToLower(strings.TrimSpace(slug)) and use that normalized
slug for the lookup and any returned/persisted value; do the same in Create (or
CreateCategory) so the stored Category.Slug is the normalized value and any
queries use the normalized slug (this also applies to the other occurrence
around line 97).
- Around line 63-65: In the duplicate-key recovery branch in category_service.go
(where you check strings.Contains(err.Error(), "duplicate key") ...), ensure the
re-fetch via s.DB.Where("slug = ?", slug).First(&cat) checks and propagates its
error: capture the result of First into an error variable, and if that error is
non-nil return nil and that error (or wrap it) instead of unconditionally
returning &cat, nil; this prevents returning a zero-value category when the
fallback query fails.
- Around line 143-151: The Delete method in CategoryService currently
hard-deletes models.ItemCategory and can leave dangling references in
capability_items.category; before calling s.DB.Delete(&models.ItemCategory{},
"id = ?", id) query the capability items for references (e.g.,
s.DB.Model(&models.CapabilityItem{}).Where("category = ?",
id).Limit(1).Count(&count) or similar) and if any exist return a new sentinel
error (e.g., ErrCategoryInUse) instead of deleting; keep the existing
ErrCategoryNotFound behavior for zero rows affected and only proceed to delete
when no referencing capability items are found.
- Around line 63-64: Replace substring error matching with typed error checks:
in EnsureCategory (where current code checks err.Error() for "duplicate
key"/"UNIQUE constraint") and in Create (the similar block), use errors.Is(err,
gorm.ErrDuplicatedKey) to detect unique constraint violations; update the
conditional to errors.Is(err, gorm.ErrDuplicatedKey) and keep the existing
duplicate-handling logic (e.g., querying s.DB.Where("slug = ?",
slug).First(&cat)) so GORM's translated sentinel error is used instead of
fragile string matching.
In `@server/internal/services/sync_service.go`:
- Around line 346-348: The calls to
s.CategorySvc.EnsureCategory(parsed.Category, triggerUser) currently ignore
returned errors; capture the returned error at both call sites (the one using
parsed.Category/triggerUser around EnsureCategory and the other at the later
site) and handle it instead of dropping it — e.g., assign err :=
s.CategorySvc.EnsureCategory(...); if err != nil { log the error via the service
logger (or wrap and return it from the enclosing function) with context like
"ensuring category <parsed.Category> for user <triggerUser>" so category
provisioning failures are surfaced and propagated appropriately.
In `@server/migrations/20260407000000_seed_item_categories.sql`:
- Around line 13-16: The seed query currently selects DISTINCT category from
capability_items and filters with category <> '' but does not trim or exclude
whitespace-only values; update the subquery to SELECT DISTINCT TRIM(category) AS
category FROM capability_items WHERE category IS NOT NULL AND TRIM(category) <>
'' so the cats alias contains trimmed, non-blank category values (and consider
using TRIM(...) in both the SELECT and WHERE to prevent creating separate slugs
for values with leading/trailing spaces).
---
Nitpick comments:
In `@server/internal/handlers/capability_item_test.go`:
- Line 41: The test currently constructs ItemHandler with a nil categorySvc
which skips the new category auto-provisioning path; update the test to create
and pass a non-nil mock or real CategoryService into NewItemHandler (replace the
nil argument) and add at least one route/test case exercising create/update that
triggers EnsureCategory on the handler (assert CategoryService.EnsureCategory
was called and that categories are created/updated as expected). Use
NewItemHandler, the ItemHandler create/update route under test, and the
EnsureCategory method on your CategoryService mock to locate and wire the
behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80a9b322-62b5-401b-8674-156ceb29cc64
📒 Files selected for processing (12)
docs/proposals/ITEM_CATEGORY_DESIGN.mdserver/cmd/api/main.goserver/cmd/migrate/main.goserver/cmd/worker/main.goserver/internal/handlers/capability_item.goserver/internal/handlers/capability_item_test.goserver/internal/handlers/category.goserver/internal/handlers/sync.goserver/internal/models/models.goserver/internal/services/category_service.goserver/internal/services/sync_service.goserver/migrations/20260407000000_seed_item_categories.sql
| `CapabilityItem.Category` 原为自由文本字符串字段，存在以下问题： | ||
|
|
||
| 1. **无分类字典**：category 值无统一管理，同一分类可能出现不同拼写（如 `dev`、`development`、`Development`） | ||
| 2. **不支持国际化**：前端无法获取分类的多语言名称和描述 | ||
| 3. **无审计记录**：不知道分类是谁、何时、通过什么途径创建的 | ||
|
|
There was a problem hiding this comment.
Seed the dictionary from canonical slugs, not raw category text.
This backfill inserts DISTINCT category directly into slug, so legacy variants like Development vs development survive as separate categories. The dictionary will start polluted and the unique index will only freeze that state. Normalize/slugify during the backfill and keep the original label only in names.
Also applies to: 234-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/proposals/ITEM_CATEGORY_DESIGN.md` around lines 28 - 33, The backfill
should normalize existing CapabilityItem.Category values into canonical slugs
rather than inserting raw category text into the dictionary.slug, so update the
backfill script to slugify/lowercase/trim each DISTINCT category before
inserting into the dictionary and store the original raw string only inside
dictionary.names (or names metadata) for display; ensure the dictionary unique
index is applied to the normalized slug value (not the raw category), and update
any references in the migration/backfill routine that currently write to slug
(and the unique constraint) to use the normalized slug generation function so
legacy variants like "Development" and "development" collapse to one entry.
| ``` | ||
| ┌──────────────────┐ ┌──────────────────┐ | ||
| │ CapabilityItem │ │ ItemCategory │ | ||
| │ │ slug │ │ | ||
| │ category: str ──┼─ ─ ─ ─ ─▷ slug: str (UK) │ | ||
| │ │ │ names: jsonb │ | ||
| │ │ │ descriptions: │ | ||
| │ │ │ jsonb │ | ||
| └──────────────────┘ └──────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Add languages to the fenced code blocks.
markdownlint is already flagging these fences. Tagging them as text, http, go, json, and sql will clear MD040 and improve rendering.
Also applies to: 113-115, 138-140, 153-163, 171-180, 188-190
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 47-47: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/proposals/ITEM_CATEGORY_DESIGN.md` around lines 47 - 56, The fenced code
blocks in ITEM_CATEGORY_DESIGN.md (including the ASCII diagram block showing
CapabilityItem → ItemCategory and the other blocks at ranges 113-115, 138-140,
153-163, 171-180, 188-190) need explicit language tags to satisfy markdownlint
MD040; update each triple-backtick fence to include an appropriate language
identifier (e.g., ```text for ASCII diagrams, ```json for JSON examples, ```sql
for SQL snippets, ```http for HTTP examples, or ```go where applicable) so the
fences render correctly and the linter stops flagging them.
| - **不使用外键约束**：`CapabilityItem.Category` 存储 slug 字符串，`ItemCategory.Slug` 为唯一索引 | ||
| - **自动创建**：写入 item 时通过 `EnsureCategory` 自动创建缺失的分类记录 | ||
| - **兼容现有数据**：现有 item 的 category 字段无需变更 |
There was a problem hiding this comment.
Deletion needs an explicit “in use” guard.
With no FK and items storing only the slug, a hard delete can orphan existing items immediately. The API should define a conflict response for referenced categories, or rewrite those item slugs atomically during delete, instead of only documenting 204/404.
Also applies to: 186-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/proposals/ITEM_CATEGORY_DESIGN.md` around lines 58 - 60, Deletion of
ItemCategory currently allows hard deletes which can orphan
CapabilityItem.Category slugs; update the API and data-layer to protect deletes
by checking references to ItemCategory.Slug: when handling DELETE for a
category, query CapabilityItem (filtering CapabilityItem.Category ==
ItemCategory.Slug) and if any exist return 409 Conflict with a clear error;
alternatively support an atomic "reassign" flow that updates all matching
CapabilityItem.Category slugs to a new slug inside a transaction before deleting
the ItemCategory; ensure EnsureCategory semantics and any delete handlers
enforce this guard so deletes cannot leave dangling slugs.
| DB: db, | ||
| Git: &services.GitService{TempBaseDir: tmpDir}, | ||
| Parser: &services.ParserService{}, | ||
| CategorySvc: &services.CategoryService{DB: db}, | ||
| } |
There was a problem hiding this comment.
CategorySvc is wired, but worker migrations still omit item_categories.
Line 86 enables category provisioning during sync, but this process can fail if the table does not exist in environments where worker bootstraps schema itself.
Proposed fix
err = db.AutoMigrate(
&models.SyncLog{},
&models.SyncJob{},
&models.CapabilityRegistry{},
&models.CapabilityItem{},
&models.CapabilityVersion{},
&models.CapabilityAsset{},
&models.SecurityScan{},
&models.ScanJob{},
+ &models.ItemCategory{},
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/cmd/worker/main.go` around lines 83 - 87, The worker enables
CategorySvc in main.go but the migration set used during worker bootstrap omits
the item_categories table; update the worker's schema/bootstrap migration
sequence (the function that runs migrations for the worker—e.g., the
runMigrations/bootstrapMigrations/RegisterMigrations routine used during worker
startup) to include the item_categories table creation migration so environments
that self-bootstrap will create item_categories before CategorySvc is used;
reference CategorySvc in the worker init to ensure ordering so the migration
runs prior to any category provisioning.
| if CategorySvc != nil && req.Category != "" { | ||
| CategorySvc.EnsureCategory(req.Category, req.CreatedBy) | ||
| } |
There was a problem hiding this comment.
Provision categories with the same canonical value you persist, and don’t fire-and-forget it.
EnsureCategory trims the slug internally and returns errors, but these paths save the raw category string and ignore the call result. " dev " can therefore leave the item storing " dev " while the dictionary row becomes dev, and the pre-save/archive paths can also create category rows even when the later item write fails. Normalize once before assigning item.Category, then call EnsureCategory inside the same write transaction and abort on failure.
Also applies to: 476-478, 631-633, 1144-1146, 1265-1267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/internal/handlers/capability_item.go` around lines 371 - 373,
Normalize the category string before assigning it to item.Category (apply the
same trimming/canonicalization that EnsureCategory uses), then call
CategorySvc.EnsureCategory synchronously inside the same write transaction and
return/abort if it returns an error instead of fire-and-for-getting it; update
all occurrences where CategorySvc.EnsureCategory is currently invoked (the code
paths using EnsureCategory around the symbols item.Category and CategorySvc,
including the other sites noted) to validate/assign the canonical value first
and propagate any EnsureCategory error to prevent saving mismatched or orphaned
rows.
| if strings.Contains(err.Error(), "duplicate key") || strings.Contains(err.Error(), "UNIQUE constraint") { | ||
| s.DB.Where("slug = ?", slug).First(&cat) | ||
| return &cat, nil |
There was a problem hiding this comment.
Check and propagate re-fetch errors in the duplicate-key recovery path.
At Line 64, the fallback query result is ignored. If that query fails, Line 65 still returns success with an invalid/zero-value category.
Proposed fix
- if strings.Contains(err.Error(), "duplicate key") || strings.Contains(err.Error(), "UNIQUE constraint") {
- s.DB.Where("slug = ?", slug).First(&cat)
- return &cat, nil
+ if strings.Contains(err.Error(), "duplicate key") || strings.Contains(err.Error(), "UNIQUE constraint") {
+ if getErr := s.DB.Where("slug = ?", slug).First(&cat).Error; getErr != nil {
+ return nil, getErr
+ }
+ return &cat, nil
}📝 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.
| if strings.Contains(err.Error(), "duplicate key") || strings.Contains(err.Error(), "UNIQUE constraint") { | |
| s.DB.Where("slug = ?", slug).First(&cat) | |
| return &cat, nil | |
| if strings.Contains(err.Error(), "duplicate key") || strings.Contains(err.Error(), "UNIQUE constraint") { | |
| if getErr := s.DB.Where("slug = ?", slug).First(&cat).Error; getErr != nil { | |
| return nil, getErr | |
| } | |
| return &cat, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/internal/services/category_service.go` around lines 63 - 65, In the
duplicate-key recovery branch in category_service.go (where you check
strings.Contains(err.Error(), "duplicate key") ...), ensure the re-fetch via
s.DB.Where("slug = ?", slug).First(&cat) checks and propagates its error:
capture the result of First into an error variable, and if that error is non-nil
return nil and that error (or wrap it) instead of unconditionally returning
&cat, nil; this prevents returning a zero-value category when the fallback query
fails.
| if strings.Contains(err.Error(), "duplicate key") || strings.Contains(err.Error(), "UNIQUE constraint") { | ||
| s.DB.Where("slug = ?", slug).First(&cat) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For GORM v1.30.0, what is the recommended way to detect unique constraint violations on PostgreSQL and SQLite? Is gorm.ErrDuplicatedKey available, and does it require TranslateError?
💡 Result:
-
Yes,
gorm.ErrDuplicatedKeyexists in GORM v1.30.0 (it’s a core GORM sentinel error). It represents “unique key constraint violation”. [1] -
Recommended approach (PostgreSQL + SQLite): enable error translation and use
errors.Is:db, _ := gorm.Open(postgres.Open(dsn), &gorm.Config{TranslateError: true}) // or: gorm.Open(sqlite.Open("file.db"), &gorm.Config{TranslateError: true}) if errors.Is(result.Error, gorm.ErrDuplicatedKey) { // unique constraint violation }
This is the approach documented by GORM under “Dialect Translated Errors”. [2]
-
Does it require
TranslateError? Yes. WithoutTranslateError: true, GORM generally returns the raw driver error (e.g., SQLSTATE23505on Postgres, SQLite constraint codes), soerrors.Is(err, gorm.ErrDuplicatedKey)is not reliably true. GORM explicitly documents that these generalized dialect errors are returned whenTranslateErroris enabled. [2] -
What gets translated (drivers):
- PostgreSQL driver maps SQLSTATE
23505→gorm.ErrDuplicatedKey. [3] - SQLite driver maps ExtendedCode
1555and2067→gorm.ErrDuplicatedKey. [4]
- PostgreSQL driver maps SQLSTATE
Sources:
[1] GORM core error definitions (ErrDuplicatedKey) https://github.com/go-gorm/gorm/blob/master/errors.go
[2] GORM docs: Error Handling → Dialect Translated Errors (uses TranslateError: true + errors.Is(..., gorm.ErrDuplicatedKey)) https://gorm.io/docs/error_handling.html
[3] Postgres driver error translator (23505 → ErrDuplicatedKey) https://github.com/go-gorm/postgres/blob/master/error_translator.go
[4] SQLite driver error translator (1555/2067 → ErrDuplicatedKey) https://github.com/go-gorm/sqlite/blob/master/error_translator.go
🏁 Script executed:
# Find and check GORM version
find . -name "go.mod" -o -name "go.sum" | head -5Repository: XDfield/costrict-web
Length of output: 129
🏁 Script executed:
# Locate the file under review
find . -name "category_service.go" -type fRepository: XDfield/costrict-web
Length of output: 110
🏁 Script executed:
# Search for database configuration/initialization
rg -i "TranslateError|NewDB|gorm.Open" --max-count=20Repository: XDfield/costrict-web
Length of output: 424
🏁 Script executed:
# Read the category_service.go file to see the code at lines 63-64 and 105-106
cat -n server/internal/services/category_service.go | head -120Repository: XDfield/costrict-web
Length of output: 4336
🏁 Script executed:
# Check GORM version in server/go.mod
grep -A 1 "^require\|gorm" server/go.mod | head -30Repository: XDfield/costrict-web
Length of output: 317
🏁 Script executed:
# Get more details on database.go initialization
cat -n server/internal/database/database.go | head -50Repository: XDfield/costrict-web
Length of output: 1870
Replace string matching with typed error handling for duplicate key detection.
Use errors.Is(err, gorm.ErrDuplicatedKey) instead of substring matching. Since TranslateError: true is already enabled in the database configuration, GORM will properly translate unique constraint violations to this sentinel error on both PostgreSQL and SQLite.
if errors.Is(err, gorm.ErrDuplicatedKey) {
// handle duplicate key
}This applies to lines 63–64 (EnsureCategory) and 105–106 (Create).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/internal/services/category_service.go` around lines 63 - 64, Replace
substring error matching with typed error checks: in EnsureCategory (where
current code checks err.Error() for "duplicate key"/"UNIQUE constraint") and in
Create (the similar block), use errors.Is(err, gorm.ErrDuplicatedKey) to detect
unique constraint violations; update the conditional to errors.Is(err,
gorm.ErrDuplicatedKey) and keep the existing duplicate-handling logic (e.g.,
querying s.DB.Where("slug = ?", slug).First(&cat)) so GORM's translated sentinel
error is used instead of fragile string matching.
| func (s *CategoryService) Delete(id string) error { | ||
| result := s.DB.Delete(&models.ItemCategory{}, "id = ?", id) | ||
| if result.Error != nil { | ||
| return result.Error | ||
| } | ||
| if result.RowsAffected == 0 { | ||
| return ErrCategoryNotFound | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Guard category deletion when referenced by capability items.
This hard delete can leave dangling category references in capability_items.category because there is no FK enforcing integrity.
Proposed fix
func (s *CategoryService) Delete(id string) error {
+ var cat models.ItemCategory
+ if err := s.DB.First(&cat, "id = ?", id).Error; err != nil {
+ if errors.Is(err, gorm.ErrRecordNotFound) {
+ return ErrCategoryNotFound
+ }
+ return err
+ }
+
+ var refs int64
+ if err := s.DB.Model(&models.CapabilityItem{}).Where("category = ?", cat.Slug).Count(&refs).Error; err != nil {
+ return err
+ }
+ if refs > 0 {
+ return errors.New("category is still referenced by capability items")
+ }
+
result := s.DB.Delete(&models.ItemCategory{}, "id = ?", id)
if result.Error != nil {
return result.Error
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/internal/services/category_service.go` around lines 143 - 151, The
Delete method in CategoryService currently hard-deletes models.ItemCategory and
can leave dangling references in capability_items.category; before calling
s.DB.Delete(&models.ItemCategory{}, "id = ?", id) query the capability items for
references (e.g., s.DB.Model(&models.CapabilityItem{}).Where("category = ?",
id).Limit(1).Count(&count) or similar) and if any exist return a new sentinel
error (e.g., ErrCategoryInUse) instead of deleting; keep the existing
ErrCategoryNotFound behavior for zero rows affected and only proceed to delete
when no referencing capability items are found.
| if s.CategorySvc != nil && parsed.Category != "" { | ||
| s.CategorySvc.EnsureCategory(parsed.Category, triggerUser) | ||
| } |
There was a problem hiding this comment.
Handle EnsureCategory errors instead of ignoring them.
Both call sites drop the returned error, so sync may succeed while category provisioning fails silently.
Proposed fix
- if s.CategorySvc != nil && parsed.Category != "" {
- s.CategorySvc.EnsureCategory(parsed.Category, triggerUser)
- }
+ if s.CategorySvc != nil && parsed.Category != "" {
+ if _, err := s.CategorySvc.EnsureCategory(parsed.Category, triggerUser); err != nil {
+ result.Failed++
+ result.Errors = append(result.Errors, fmt.Sprintf("ensure category %q: %v", parsed.Category, err))
+ continue
+ }
+ }
@@
- if s.CategorySvc != nil && parsed.Category != "" {
- s.CategorySvc.EnsureCategory(parsed.Category, triggerUser)
- }
+ if s.CategorySvc != nil && parsed.Category != "" {
+ if _, err := s.CategorySvc.EnsureCategory(parsed.Category, triggerUser); err != nil {
+ result.Failed++
+ result.Errors = append(result.Errors, fmt.Sprintf("ensure category %q: %v", parsed.Category, err))
+ continue
+ }
+ }Also applies to: 416-418
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/internal/services/sync_service.go` around lines 346 - 348, The calls
to s.CategorySvc.EnsureCategory(parsed.Category, triggerUser) currently ignore
returned errors; capture the returned error at both call sites (the one using
parsed.Category/triggerUser around EnsureCategory and the other at the later
site) and handle it instead of dropping it — e.g., assign err :=
s.CategorySvc.EnsureCategory(...); if err != nil { log the error via the service
logger (or wrap and return it from the enclosing function) with context like
"ensuring category <parsed.Category> for user <triggerUser>" so category
provisioning failures are surfaced and propagated appropriately.
| SELECT DISTINCT category | ||
| FROM capability_items | ||
| WHERE category IS NOT NULL AND category <> '' | ||
| ) AS cats |
There was a problem hiding this comment.
Trim/filter category values during seed to avoid invalid slugs.
Current filter (category <> '') still inserts whitespace-only categories and preserves accidental leading/trailing spaces as separate slugs.
Proposed fix
FROM (
- SELECT DISTINCT category
+ SELECT DISTINCT btrim(category) AS category
FROM capability_items
- WHERE category IS NOT NULL AND category <> ''
+ WHERE NULLIF(btrim(category), '') IS NOT NULL
) AS cats📝 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.
| SELECT DISTINCT category | |
| FROM capability_items | |
| WHERE category IS NOT NULL AND category <> '' | |
| ) AS cats | |
| SELECT DISTINCT btrim(category) AS category | |
| FROM capability_items | |
| WHERE NULLIF(btrim(category), '') IS NOT NULL | |
| ) AS cats |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/migrations/20260407000000_seed_item_categories.sql` around lines 13 -
16, The seed query currently selects DISTINCT category from capability_items and
filters with category <> '' but does not trim or exclude whitespace-only values;
update the subquery to SELECT DISTINCT TRIM(category) AS category FROM
capability_items WHERE category IS NOT NULL AND TRIM(category) <> '' so the cats
alias contains trimmed, non-blank category values (and consider using TRIM(...)
in both the SELECT and WHERE to prevent creating separate slugs for values with
leading/trailing spaces).
1 participant
Summary by CodeRabbit
Release Notes