Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds tag support across the stack: CLI flags for create/list/update/show, domain command/event/tag handling, projector persistence, server-side tag filtering, and UI/client normalization and payload handling for tags. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI
participant API_Server as Server
participant Domain
participant EventStore
participant Projector
Note over User,CLI: Create rune with --tag flags
User->>CLI: run create --tag Backend --tag " urgent "
CLI->>API_Server: POST /create-rune { ..., tags:["backend","urgent"] }
API_Server->>Domain: HandleCreateRune(cmd)
Domain->>EventStore: persist RuneCreated{Tags: normalizeTags(cmd.Tags)}
EventStore->>Projector: emit RuneCreated event
Projector->>Projector: normalize/apply tags -> persist projection with Tags
Projector-->>API_Server: projection persisted
API_Server-->>CLI: 201 Created
CLI-->>User: display success
sequenceDiagram
autonumber
participant User
participant CLI
participant API_Server as Server
participant ProjectionDB as Projections
Note over User,CLI: List runes filtered by tag
User->>CLI: run list --tag backend
CLI->>API_Server: GET /runes?tag=backend
API_Server->>ProjectionDB: query summaries, filter by tags includes "backend"
ProjectionDB-->>API_Server: filtered rune list
API_Server-->>CLI: 200 OK [rune summaries with normalized tags]
CLI-->>User: render list
Possibly related PRs
🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
cli/list_test.go (1)
292-298: Consider adding a test for multiple tags.The helper only supports a single tag. A test verifying multiple
--tagflags are combined correctly (e.g.,--tag Backend --tag urgent→tags=backend,urgent) would strengthen coverage.Example test case and helper extension
func (tc *listTestContext) execute_list_with_tags(tags ...string) { tc.t.Helper() cmd := NewListCmd(func() *Client { return tc.client }, tc.buf) args := []string{} for _, tag := range tags { args = append(args, "--tag", tag) } cmd.Command.SetArgs(args) cmd.Command.SetErr(tc.buf) tc.err = cmd.Command.Execute() }t.Run("passes multiple tag filters as comma-separated tags query parameter", func(t *testing.T) { tc := newListTestContext(t) // Given tc.server_that_captures_request_and_returns_runes() tc.client_configured() // When tc.execute_list_with_tags("Backend", "Urgent") // Then tc.command_has_no_error() tc.request_query_param_was("tags", "backend,urgent") })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/list_test.go` around lines 292 - 298, The current test helper execute_list_with_tag in listTestContext only supports a single --tag flag; add a variadic helper (e.g., execute_list_with_tags(tags ...string)) that builds args by appending "--tag", tag for each provided tag and sets those on NewListCmd (use the same NewListCmd(func() *Client { return tc.client }, tc.buf) flow), then add a test that uses tc.execute_list_with_tags("Backend", "Urgent") and asserts via the existing request capture helpers (e.g., request_query_param_was) that the server receives tags=backend,urgent (lowercased, comma-separated).cli/update.go (2)
49-70: Consider extracting shared tag normalization logic.The same normalization pattern (trim, lowercase, filter empty) is repeated in
create.go,list.go, and here. A shared helper would reduce duplication and ensure consistency.Example helper function
// In a shared location like cli/tags.go func normalizeTags(tags []string) []string { normalized := make([]string, 0, len(tags)) for _, tag := range tags { tag = strings.ToLower(strings.TrimSpace(tag)) if tag != "" { normalized = append(normalized, tag) } } return normalized }Usage in update.go:
if cmd.Flags().Changed("add-tag") { tags, _ := cmd.Flags().GetStringSlice("add-tag") - normalized := make([]string, 0, len(tags)) - for _, tag := range tags { - tag = strings.ToLower(strings.TrimSpace(tag)) - if tag != "" { - normalized = append(normalized, tag) - } - } + normalized := normalizeTags(tags) body["add_tags"] = normalized }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/update.go` around lines 49 - 70, The tag normalization code repeated in update.go (the blocks handling cmd.Flags().Changed("add-tag") and "remove-tag" that produce body["add_tags"] and body["remove_tags"]) should be extracted into a shared helper (e.g., normalizeTags(tags []string)) placed in a common CLI file (like cli/tags.go) and used from create.go, list.go and update.go; replace the inline loops by calling normalizeTags on the slice returned by cmd.Flags().GetStringSlice and assign the result to body["add_tags"] / body["remove_tags"] respectively to remove duplication and ensure consistent behavior.
58-58: Minor: Empty arrays sent when all tags normalize to empty.When
--add-tagis provided but all values normalize to empty strings (e.g.,--add-tag " "), an emptyadd_tagsarray is sent in the request body. This is benign since the server handles empty arrays gracefully, but it differs from how other flags work (they're only added to body when they have meaningful values).Optional fix to skip empty arrays
if cmd.Flags().Changed("add-tag") { tags, _ := cmd.Flags().GetStringSlice("add-tag") normalized := make([]string, 0, len(tags)) for _, tag := range tags { tag = strings.ToLower(strings.TrimSpace(tag)) if tag != "" { normalized = append(normalized, tag) } } - body["add_tags"] = normalized + if len(normalized) > 0 { + body["add_tags"] = normalized + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/update.go` at line 58, The code unconditionally sets body["add_tags"] = normalized even when normalized is an empty slice; change the logic in the update command (where variable normalized is computed and body is a map) to only assign body["add_tags"] when len(normalized) > 0 so empty arrays are not sent—locate the block that currently does body["add_tags"] = normalized and wrap it with a conditional check on normalized's length.ui/src/lib/api.ts (1)
145-150: Share one tag-normalization helper.This trim/lowercase/filter pipeline is now copy-pasted across detail normalization, list normalization, and update payload construction. Pulling it into one private helper will keep those paths from drifting the next time tag rules change.
Refactor sketch
+ private normalizeTags(raw: unknown): string[] { + return Array.isArray(raw) + ? raw + .filter((tag): tag is string => typeof tag === "string") + .map((tag) => tag.trim().toLowerCase()) + .filter((tag) => tag.length > 0) + : []; + } + private normalizeRuneDetail(raw: RuneDetail | (Partial<RuneDetail> & { id: string })): RuneDetail { @@ - tags: Array.isArray(raw.tags) - ? raw.tags - .filter((tag): tag is string => typeof tag === "string") - .map((tag) => tag.trim().toLowerCase()) - .filter((tag) => tag.length > 0) - : [], + tags: this.normalizeTags(raw.tags), }; } private normalizeRuneListItem(raw: RuneListItem): RuneListItem { return { ...raw, - tags: Array.isArray(raw.tags) - ? raw.tags - .filter((tag): tag is string => typeof tag === "string") - .map((tag) => tag.trim().toLowerCase()) - .filter((tag) => tag.length > 0) - : [], + tags: this.normalizeTags(raw.tags), }; } @@ if (Array.isArray(updates.tags)) { - command.tags = updates.tags - .filter((tag): tag is string => typeof tag === "string") - .map((tag) => tag.trim().toLowerCase()) - .filter((tag) => tag.length > 0); + command.tags = this.normalizeTags(updates.tags); }Also applies to: 154-162, 342-347
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/lib/api.ts` around lines 145 - 150, Extract the repeated tag-cleaning pipeline into a single private helper called normalizeTags(value: unknown): string[] that returns an array of trimmed, lowercased, non-empty strings; then replace each inline pipeline that begins with Array.isArray(raw.tags) (and equivalent pipelines used in detail normalization, list normalization, and update payload construction) with a call to normalizeTags(raw.tags) so all tag normalization is centralized and consistent.domain/projectors/tag_utils.go (1)
8-53: Centralize the tag normalization/mutation rules.These helpers are already duplicated in
domain/handlers.go, andserver/handlers.gonow has a third normalization path. If one copy changes independently, aggregate rebuilds, projections, and list filtering can drift on the same event stream.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@domain/projectors/tag_utils.go` around lines 8 - 53, The normalization and mutation logic in normalizeTags and applyTagMutations is duplicated across domain/projectors.go, domain/handlers.go, and server/handlers.go; extract these functions into a single shared package (e.g., domain/tags or internal/tags) and replace the local copies with imports and calls to the shared normalizeTags and applyTagMutations to ensure one canonical implementation; update all callers (functions that currently call normalizeTags or applyTagMutations in domain/projectors, domain/handlers.go, and server/handlers.go) to import the new package and remove the duplicated definitions so normalization/mutation rules stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/create_test.go`:
- Around line 390-401: The helper request_body_has_array_field currently ignores
non-string elements and thus treats arrays like ["backend", 123] as valid;
change it to fail if any element is not a string: after asserting
tc.receivedBody[key] is a []any, iterate the slice in
request_body_has_array_field and if any item is not a string call tc.t.Fatalf or
require.Failf with a clear message including the offending index and value, then
collect the strings and assert equality with expected as before (use the
function name request_body_has_array_field and the receivedBody[key] reference
to locate the code).
In `@cli/update_test.go`:
- Around line 250-261: The helper request_body_has_array_field drops non-string
elements when building actual, allowing arrays with mixed types to pass; change
it to fail if any element is not a string by iterating over
tc.receivedBody[key].([]any) and asserting each item is a string (use
require.True/require.Fail with a clear message referencing
request_body_has_array_field and the offending item) so that mixed-type arrays
like ["backend", 123] cause the test to fail before comparing to expected.
In `@domain/events_test.go`:
- Line 382: The test currently only verifies marshal/unmarshal round-trips which
misses typos in JSON struct tags; update the test cases that populate tc.jsonMap
to explicitly assert the presence and exact keys "tags", "add_tags", and
"remove_tags" (and in the empty RuneCreated omission case ensure "tags" is
asserted as present/omitted correctly), replacing or augmenting the loose
type-based checks so the test fails if the JSON wire keys are incorrect; apply
the same explicit-key assertions for the other occurrences mentioned (the blocks
around the other test cases analogous to lines 407-409 and 685-688) and
reference the tc.jsonMap and RuneCreated test cases when making the changes.
In `@domain/handlers.go`:
- Around line 692-698: normalizeTagPointer currently collapses an explicit
empty-slice intent into nil because normalizeTags returns nil for empty input;
update normalizeTagPointer so that if tags != nil and len(*tags) == 0 it returns
a pointer to an explicit empty slice (e.g. &[]string{}), otherwise call
normalizeTags(*tags) and return a pointer to that result — this preserves the
difference between an explicit "clear all tags" (non-nil pointer to an empty
slice) and "omit field" (nil pointer). Reference: normalizeTagPointer and
normalizeTags.
---
Nitpick comments:
In `@cli/list_test.go`:
- Around line 292-298: The current test helper execute_list_with_tag in
listTestContext only supports a single --tag flag; add a variadic helper (e.g.,
execute_list_with_tags(tags ...string)) that builds args by appending "--tag",
tag for each provided tag and sets those on NewListCmd (use the same
NewListCmd(func() *Client { return tc.client }, tc.buf) flow), then add a test
that uses tc.execute_list_with_tags("Backend", "Urgent") and asserts via the
existing request capture helpers (e.g., request_query_param_was) that the server
receives tags=backend,urgent (lowercased, comma-separated).
In `@cli/update.go`:
- Around line 49-70: The tag normalization code repeated in update.go (the
blocks handling cmd.Flags().Changed("add-tag") and "remove-tag" that produce
body["add_tags"] and body["remove_tags"]) should be extracted into a shared
helper (e.g., normalizeTags(tags []string)) placed in a common CLI file (like
cli/tags.go) and used from create.go, list.go and update.go; replace the inline
loops by calling normalizeTags on the slice returned by
cmd.Flags().GetStringSlice and assign the result to body["add_tags"] /
body["remove_tags"] respectively to remove duplication and ensure consistent
behavior.
- Line 58: The code unconditionally sets body["add_tags"] = normalized even when
normalized is an empty slice; change the logic in the update command (where
variable normalized is computed and body is a map) to only assign
body["add_tags"] when len(normalized) > 0 so empty arrays are not sent—locate
the block that currently does body["add_tags"] = normalized and wrap it with a
conditional check on normalized's length.
In `@domain/projectors/tag_utils.go`:
- Around line 8-53: The normalization and mutation logic in normalizeTags and
applyTagMutations is duplicated across domain/projectors.go, domain/handlers.go,
and server/handlers.go; extract these functions into a single shared package
(e.g., domain/tags or internal/tags) and replace the local copies with imports
and calls to the shared normalizeTags and applyTagMutations to ensure one
canonical implementation; update all callers (functions that currently call
normalizeTags or applyTagMutations in domain/projectors, domain/handlers.go, and
server/handlers.go) to import the new package and remove the duplicated
definitions so normalization/mutation rules stay consistent.
In `@ui/src/lib/api.ts`:
- Around line 145-150: Extract the repeated tag-cleaning pipeline into a single
private helper called normalizeTags(value: unknown): string[] that returns an
array of trimmed, lowercased, non-empty strings; then replace each inline
pipeline that begins with Array.isArray(raw.tags) (and equivalent pipelines used
in detail normalization, list normalization, and update payload construction)
with a call to normalizeTags(raw.tags) so all tag normalization is centralized
and consistent.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d858deb4-1cfa-44c8-b7a8-e43e886d0a0f
📒 Files selected for processing (24)
cli/create.gocli/create_test.gocli/list.gocli/list_test.gocli/show.gocli/show_test.gocli/update.gocli/update_test.godomain/commands.godomain/commands_test.godomain/events.godomain/events_test.godomain/handlers.godomain/handlers_test.godomain/projectors/rune_detail.godomain/projectors/rune_detail_test.godomain/projectors/rune_summary_projector.godomain/projectors/rune_summary_projector_test.godomain/projectors/tag_utils.goserver/handlers.goserver/handlers_test.goui/src/lib/api.spec.tsui/src/lib/api.tsui/src/types/rune.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 4 file(s) based on 4 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 4 file(s) based on 4 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
4 tasks
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.
No description provided.