feat: add group visibility and group selector for memos#6068
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds group support across the API, store, memo visibility, and web UI. It introduces group persistence and membership tables, memo group scoping, server-side access checks, SSE/stat filtering updates, and frontend pages, routes, queries, and translations for managing groups. ChangesGroups Feature
Locale Display Name Lookup
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
server/router/api/v1/group_service.go-25-30 (1)
25-30: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
DisplayNameisn't validated as required.The proto marks
display_nameasREQUIRED, butCreateGroupdoesn't reject an empty value before persisting it.🔧 Suggested fix
+ if request.Group.DisplayName == "" { + return nil, status.Errorf(codes.InvalidArgument, "display_name is required") + } create := &store.Group{🤖 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 `@server/router/api/v1/group_service.go` around lines 25 - 30, CreateGroup currently persists request.Group.DisplayName without enforcing the proto’s required display_name field. Update the CreateGroup path in group_service.go to validate request.Group.DisplayName before constructing the store.Group, and return a clear invalid-argument error when it is empty. Use the existing CreateGroup/request.Group handling so the check happens before the store.Group literal is created.store/db/mysql/group.go-176-182 (1)
176-182: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
UpdateGroupMemberdoesn't verify a row was actually updated.If
group_id/user_iddoesn't match any row,ExecContextsucceeds silently (0 rows affected) and the function still returnsupdateas if it succeeded, misleading callers.🔧 Suggested fix
func (d *DB) UpdateGroupMember(ctx context.Context, update *store.GroupMember) (*store.GroupMember, error) { query := "UPDATE group_members SET role = ? WHERE group_id = ? AND user_id = ?" - if _, err := d.db.ExecContext(ctx, query, update.Role, update.GroupID, update.UserID); err != nil { - return nil, err + result, err := d.db.ExecContext(ctx, query, update.Role, update.GroupID, update.UserID) + if err != nil { + return nil, errors.Wrap(err, "failed to update group member") + } + if rows, _ := result.RowsAffected(); rows == 0 { + return nil, errors.New("group member not found") } return update, 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 `@store/db/mysql/group.go` around lines 176 - 182, UpdateGroupMember in DB should not treat a successful ExecContext call as success unless it actually changed a row. After running the UPDATE on group_members, check the result’s rows affected and return an error when it is zero so callers know the group_id/user_id pair did not match any record; keep the existing UpdateGroupMember signature and use the existing db.ExecContext flow to locate the fix.server/router/api/v1/group_service.go-79-83 (1)
79-83: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse exact group-name parsing in both update and delete paths.
fmt.Sscanfaccepts trailing garbage (groups/123garbageparses as123), so malformed resource names can slip through. Reuse the shared resource-name parsing pattern here, or add a small group helper for consistency.🤖 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 `@server/router/api/v1/group_service.go` around lines 79 - 83, The group name parsing in group_service.go is too permissive because fmt.Sscanf on request.Group.Name can accept trailing garbage, so align this path with the stricter parsing used elsewhere. Update the group ID extraction in the update/delete flow to use the same exact resource-name parsing approach as the shared helper or introduce a small group-name helper and reuse it consistently in the relevant service methods, including the function handling request.Group.Name.store/db/mysql/group.go-108-116 (1)
108-116: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winWrap
DeleteGroupin a transaction
The two deletes can leave the store half-updated if the second one fails. Use a transaction so group membership and the group row are removed atomically.🤖 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 `@store/db/mysql/group.go` around lines 108 - 116, DeleteGroup currently runs two separate deletes with d.db.ExecContext, which can leave data half-updated if the second delete fails. Update the DeleteGroup method on DB to wrap both DELETE statements in a single transaction: begin the transaction, execute the group_members and groups deletes using the transaction handle, and commit only if both succeed, otherwise roll back on any error.server/router/api/v1/memo_service.go-106-111 (1)
106-111: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMalformed
Memo.Groupis silently ignored.If
Sscanffails to parse*request.Memo.Group, the error is swallowed andcreate.GroupIDstays nil. When visibility is GROUP this is later caught (Line 114), but for non-GROUP memos a malformed group reference is silently dropped rather than reported. Consider returningInvalidArgumenton parse failure.🔧 Proposed change
if request.Memo.Group != nil { var groupID int32 - if _, err := fmt.Sscanf(*request.Memo.Group, "groups/%d", &groupID); err == nil { - create.GroupID = &groupID - } + if _, err := fmt.Sscanf(*request.Memo.Group, "groups/%d", &groupID); err != nil { + return nil, status.Errorf(codes.InvalidArgument, "invalid group name: %s", *request.Memo.Group) + } + create.GroupID = &groupID }🤖 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 `@server/router/api/v1/memo_service.go` around lines 106 - 111, In memo_service.go, the Memo.Group parsing in the create path silently ignores malformed group references when fmt.Sscanf fails, which can drop invalid input without surfacing an error. Update the logic around request.Memo.Group and create.GroupID so that parse failures return an InvalidArgument error instead of leaving GroupID nil, while still allowing valid "groups/%d" values to populate groupID as before.server/router/api/v1/memo_service.go-258-261 (1)
258-261: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winMove GROUP access checks before pagination.
InListMemos, inaccessibleGROUPmemos are dropped afterlimit/offsetandNextPageTokenare computed, so a page can come back short or empty while still advertising more results. Scoping the query to the caller’s group IDs would keep pagination consistent and avoid loading rows the caller can’t see.🤖 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 `@server/router/api/v1/memo_service.go` around lines 258 - 261, ListMemos is applying GROUP visibility filtering too late, after pagination and NextPageToken are already derived, which can make pages short or empty while still implying more data. Move the GROUP access restriction into the query-building path before limit/offset are applied, using the current caller’s group IDs in the existing memoFind.Filters/memoFind.VisibilityList logic so inaccessible rows are never counted or paginated.web/src/components/CreateGroupDialog.tsx-54-76 (1)
54-76: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSuccess toast fires even when nothing changed.
When
updateMaskis empty (no fields modified),updateGroup.mutateAsyncis skipped but the success toast at Line 75 still fires unconditionally, misleading the user into thinking an update was persisted.🩹 Proposed fix
if (updateMask.length > 0) { await updateGroup.mutateAsync({ group: groupToUpdate, updateMask, }); + toast.success(t("group.update-successfully")); } - toast.success(t("group.update-successfully"));🤖 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 `@web/src/components/CreateGroupDialog.tsx` around lines 54 - 76, The success toast is shown unconditionally in CreateGroupDialog even when updateMask is empty and no update was sent. Update the else branch in CreateGroupDialog so toast.success(t("group.update-successfully")) only runs after a real mutation via updateGroup.mutateAsync, and skip or replace the toast when no fields changed. Use the existing create, updateMask, and updateGroup logic to gate the success message correctly.web/src/pages/GroupDetail.tsx-71-105 (1)
71-105: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMissing empty state when no groups exist.
When
groupsis an empty array, the grid renders nothing. The locale keygroup.no-groups-foundis already defined but never used here.🩹 Proposed fix
{isLoading ? ( <p className="text-gray-500">{t("group.loading")}</p> + ) : groups?.length === 0 ? ( + <p className="text-gray-500">{t("group.no-groups-found")}</p> ) : ( groups?.map((group) => (🤖 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 `@web/src/pages/GroupDetail.tsx` around lines 71 - 105, The GroupDetail grid currently handles loading but not the case where `groups` is an empty array, so nothing is shown. Update the `GroupDetail` render path around the `groups?.map(...)` block to explicitly detect the empty state and display the existing `group.no-groups-found` translation key. Keep the current loading branch intact, and ensure the fallback only appears when loading is false and `groups.length === 0`.web/src/components/MemoEditor/services/memoService.ts-51-54 (1)
51-54: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClear
group_idwhen the editor unsets a group. The"group"update path currently drops empty strings, and the store layer only writesgroup_idwhenupdate.GroupID != nil, so unsetting a group leaves the old association in place. Use a nullable field or explicit clear flag so this can writeNULL.🤖 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 `@web/src/components/MemoEditor/services/memoService.ts` around lines 51 - 54, Update the memo update flow in memoService so clearing the editor’s group actually removes the association instead of leaving the old one. In the logic around state.metadata.groupNames and the patch for "group", stop treating an empty string as a no-op and propagate an explicit null/clear value through the update path. Make the corresponding store-side update handler that consumes update.GroupID able to distinguish “clear group” from “unchanged” so it writes NULL when the group is unset.web/src/hooks/useMemoFilters.ts-112-112 (1)
112-112: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMissing
includeGroupsin theuseMemodependency array.
includeGroupsis read at line 58 but absent from the dependency list, so the memoized filter could go stale if this value ever changes without another dependency changing.🐛 Proposed fix
- }, [creatorName, includeShortcuts, includePinned, visibilities, selectedShortcut, filters, groupName]); + }, [creatorName, includeShortcuts, includePinned, visibilities, selectedShortcut, filters, groupName, includeGroups]);🤖 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 `@web/src/hooks/useMemoFilters.ts` at line 112, The useMemo dependency array in useMemoFilters is missing includeGroups, which can leave the memoized filter stale when that value changes. Update the dependency list for the useMemo block to include includeGroups alongside the other referenced inputs, and verify the filter logic in useMemoFilters stays in sync with all values it reads.web/src/components/MemoEditor/components/GroupSelector.tsx-18-27 (1)
18-27: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDerive
displayTextinstead of syncing viauseEffect.
displayTextis fully derivable fromselectedGroupNames/groups/t; syncing it throughuseState+useEffectcauses an extra render and a brief flash of the stale/default text (t("group.select-groups")) before the effect fires — e.g. when editing an existing group memo, the label first shows "select groups" before flipping to the correct name.♻️ Proposed refactor to compute displayText directly
- const [displayText, setDisplayText] = useState(t("group.select-groups")); - - useEffect(() => { - if (!selectedGroupNames || selectedGroupNames.length === 0) { - setDisplayText(t("group.select-groups")); - } else if (selectedGroupNames.length === 1) { - const group = groups.find((g) => g.name === selectedGroupNames[0]); - setDisplayText(group?.displayName || t("group.selected")); - } else { - setDisplayText(t("group.multiple-groups", { count: selectedGroupNames.length })); - } - }, [selectedGroupNames, groups, t]); + const displayText = useMemo(() => { + if (!selectedGroupNames || selectedGroupNames.length === 0) { + return t("group.select-groups"); + } + if (selectedGroupNames.length === 1) { + const group = groups.find((g) => g.name === selectedGroupNames[0]); + return group?.displayName || t("group.selected"); + } + return t("group.multiple-groups", { count: selectedGroupNames.length }); + }, [selectedGroupNames, groups, t]);🤖 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 `@web/src/components/MemoEditor/components/GroupSelector.tsx` around lines 18 - 27, `GroupSelector` is syncing `displayText` through `useState`/`useEffect`, but it can be derived directly from `selectedGroupNames`, `groups`, and `t`. Refactor the component to compute `displayText` inline (or via a memoized derived value) instead of updating it in the effect, and remove the `useEffect`/state path that causes the stale default label flash.store/migration/mysql/0.29/00__group.sql-15-20 (1)
15-20: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winNo CHECK/enum constraint on
group_members.role.Unlike
memo.visibility(constrained via CHECK in the sqlite fix migration),roleaccepts any text value, which could let bad data ("member" vs "MEMBER", typos) slip in and break authorization checks that likely compare against exact string values.🤖 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 `@store/migration/mysql/0.29/00__group.sql` around lines 15 - 20, The group_members.role column currently accepts arbitrary text, so add a constraint in the group table migration to restrict it to the valid role values used by the app, matching the style used for memo.visibility in the sqlite fix migration. Update the CREATE TABLE definition in group_members so role is enforced via a CHECK or enum-like constraint, and keep the constraint tied to the group_members schema so authorization code relying on exact role strings like MEMBER cannot receive invalid values.store/migration/mysql/LATEST.sql-127-144 (1)
127-144: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
descriptioncolumn lacks a default, unlike Postgres/SQLite equivalents.Here
descriptionisTEXT NOT NULLwith no default, while the Postgres and SQLite migrations for the same table useTEXT NOT NULL DEFAULT ''. Under MySQL strict mode, any insert path that omitsdescriptionwill fail with a "doesn't have a default value" error, whereas the same insert would silently succeed on the other two drivers. Also consider adding an index ongroup_members.user_idto support "groups a user belongs to" lookups, since the composite PK(group_id, user_id)only optimizes group-id-first lookups.💡 Proposed fix
`creator_id` INT NOT NULL, - `description` TEXT NOT NULL, + `description` TEXT NOT NULL DEFAULT '', `visibility` VARCHAR(256) NOT NULL DEFAULT 'PRIVATE',🤖 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 `@store/migration/mysql/LATEST.sql` around lines 127 - 144, The MySQL migration for the groups table is missing the same default on description that the Postgres and SQLite migrations use, so update the groups table definition in LATEST.sql to make description match the other drivers’ TEXT NOT NULL DEFAULT '' behavior. Also add an index in the group_members schema on user_id (separate from the existing composite primary key on group_id, user_id) so lookups for memberships by user are efficient; make the change in the migration section that defines groups and group_members.
🧹 Nitpick comments (17)
server/router/api/v1/group_service.go (2)
3-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport grouping doesn't follow stdlib → third-party →
github.com/usememos/memosordering.
google.golang.org/grpc/...andgoogle.golang.org/protobuf/...third-party imports aren't separated from stdlib (context,fmt,time) or from thegithub.com/usememos/memos/...imports by blank lines.As per coding guidelines, "Keep Go imports grouped as stdlib, third-party, then
github.com/usememos/memos; goimports is run by golangci-lint."♻️ Fix
import ( "context" "fmt" "time" + "google.golang.org/grpc/codes" "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/emptypb" "google.golang.org/protobuf/types/known/timestamppb" + v1pb "github.com/usememos/memos/proto/gen/api/v1" "github.com/usememos/memos/store" )🤖 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 `@server/router/api/v1/group_service.go` around lines 3 - 14, The import block in group_service.go is not grouped in the required stdlib → third-party → github.com/usememos/memos order. Reformat the imports around the existing context, fmt, time, google.golang.org/grpc, google.golang.org/protobuf, v1pb, and store entries by inserting blank lines between each group and keeping the github.com/usememos/memos imports last, matching goimports/golangci-lint expectations.Source: Coding guidelines
107-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated owner/admin permission-check logic.
The
isOwnerOrAdmincomputation (role loop + creator check + admin check) is identical betweenUpdateGroupandDeleteGroup.♻️ Suggested extraction
func (s *APIV1Service) isGroupOwnerOrAdmin(ctx context.Context, groupID int32, group *store.Group, user *store.User) (bool, error) { if user.Role == store.RoleAdmin || group.CreatorID == user.ID { return true, nil } members, err := s.Store.ListGroupMembers(ctx, &store.FindGroupMember{GroupID: &groupID, UserID: &user.ID}) if err != nil { return false, err } for _, member := range members { if member.Role == "OWNER" || member.Role == "ADMIN" { return true, nil } } return false, nil }Also applies to: 182-194
🤖 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 `@server/router/api/v1/group_service.go` around lines 107 - 119, The owner/admin permission check is duplicated in both UpdateGroup and DeleteGroup, so extract it into a shared helper on APIV1Service such as isGroupOwnerOrAdmin and reuse it from both flows. Move the existing role loop, creator ID check, and admin check into that helper, keep the same member lookup behavior via Store.ListGroupMembers, and have both methods call it instead of maintaining separate isOwnerOrAdmin logic.server/router/api/v1/memo_service.go (1)
73-79: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMembership-check DB errors are masked as
PermissionDenied.
if err != nil || !isMembertreats aCheckUserInGroupfailure identically to a non-member, then the creator fallback similarly foldsGetGrouperrors into the denial. A transient store error will surface to the caller as "not a group member" rather than an internal error. This same pattern is duplicated inCreateMemo(Lines 117-124) andUpdateMemo(Lines 572-596); consider handling the error path distinctly (e.g., returncodes.Internalonerr != 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 `@server/router/api/v1/memo_service.go` around lines 73 - 79, The membership check in memo authorization is treating store failures as a normal permission denial, which masks real DB errors. Update the logic around CheckUserInGroup and the GetGroup creator fallback to handle err separately from the not-member case, returning an Internal status on store errors and PermissionDenied only when the user is actually not allowed. Apply the same fix in CreateMemo and UpdateMemo where this pattern is duplicated.server/router/api/v1/memo_service_converter.go (1)
362-363: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
v1pb.Visibility_GROUPfor group visibility
The generated protobuf enum already exposesVisibility_GROUP = 4, so the rawv1pb.Visibility(4)cast is unnecessary and makes the mapping brittle. Replace both conversion branches with the named enum constant.🤖 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 `@server/router/api/v1/memo_service_converter.go` around lines 362 - 363, The visibility mapping in the converter uses a raw numeric cast for group visibility, which is brittle and should use the generated protobuf enum constant instead. Update the group visibility branch in the memo service conversion logic (the switch in the visibility conversion function) to return v1pb.Visibility_GROUP, and replace any similar raw cast mappings with the named enum constant for clarity and stability.web/src/components/CreateGroupDialog.tsx (1)
92-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse existing
group.create-group/group.update-groupstrings instead of concatenating generic parts.
group.create-groupandgroup.update-groupkeys already exist and are purpose-built for this exact dialog title; concatenatingcommon.create/common.edit+common.groupsduplicates that intent and risks incorrect singular/plural phrasing in some locales.♻️ Proposed fix
- <DialogTitle>{`${isCreating ? t("common.create") : t("common.edit")} ${t("common.groups")}`}</DialogTitle> + <DialogTitle>{isCreating ? t("group.create-group") : t("group.update-group")}</DialogTitle>🤖 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 `@web/src/components/CreateGroupDialog.tsx` at line 92, The dialog title in CreateGroupDialog should use the existing purpose-built translation keys instead of building the text from common.create/common.edit and common.groups. Update the DialogTitle logic to switch between group.create-group and group.update-group so the title stays locale-safe and avoids generic concatenation.web/src/pages/GroupTimeline.tsx (1)
23-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale "fallback/demonstration" comments should be cleaned up or verified.
These comments claim group filtering isn't fully backed by the CEL compiler yet, but the stack indicates
group_idfiltering has already been added tointernal/filter/schema.go. If that's now implemented, please update/remove these comments so they don't mislead future readers about the feature's completeness.Also applies to: 57-58
🤖 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 `@web/src/pages/GroupTimeline.tsx` around lines 23 - 24, The fallback/demonstration comments in GroupTimeline are now misleading if group_id filtering is already supported by the backend filter schema. Review the comments near the filtering logic in GroupTimeline and either remove them or rewrite them to reflect the current implementation accurately, using the existing group filtering code path and any related filter helper symbols to verify the behavior.web/src/components/MemoEditor/services/memoService.ts (1)
117-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfirm the
as anycast is still required forcreate(MemoSchema, ...).
groupis now a first-class field onMemo/MemoSchema, so passing it inside an object cast toanybypasses type checking that could otherwise catch shape mismatches for this and other fields in the same object.🤖 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 `@web/src/components/MemoEditor/services/memoService.ts` around lines 117 - 119, The object passed to create(MemoSchema, ...) in memoService should no longer rely on an as any cast now that group is a first-class Memo/MemoSchema field. Update the create payload in the memo service to use the real schema-typed object shape, remove the cast if the types already match, and verify the fields around state.metadata.groupNames[0] are still correctly accepted by the MemoSchema types.web/src/pages/Explore.tsx (1)
17-17: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueUse
Visibility.GROUPhere instead of4 as any.🤖 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 `@web/src/pages/Explore.tsx` at line 17, Replace the hardcoded `4 as any` in `Explore`’s `visibilities` initialization with the proper `Visibility.GROUP` enum value. Update the `Explore.tsx` logic where `currentUser` determines the visibility list so it uses the named symbol from the `Visibility` enum instead of an unsafe numeric cast.web/src/hooks/useGroupQueries.ts (1)
26-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
anyfor the mutation input type.
mutationFn: async (group: any) => ...bypasses theGroupSchemacontract entirely. Callers likeCreateGroupDialogonly ever pass{ displayName, description }, so this can be typed precisely instead ofany, catching mismatched payloads at compile time.♻️ Suggested typing
- mutationFn: async (group: any) => { + mutationFn: async (group: Pick<Group, "displayName" | "description">) => { const createdGroup = await groupServiceClient.createGroup({ group: create(GroupSchema, group), });🤖 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 `@web/src/hooks/useGroupQueries.ts` around lines 26 - 40, The mutation input in useCreateGroup is typed as any, which bypasses the GroupSchema contract. Update the mutationFn parameter to use a precise object type matching the payload callers pass from CreateGroupDialog (the displayName and description fields), and keep the create(GroupSchema, group) call so the compiler can catch mismatched inputs. Use useCreateGroup and groupServiceClient.createGroup as the key locations to adjust the typing.web/src/components/MemoExplorer/GroupsSection.tsx (1)
11-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded English string bypasses i18n.
"No groups joined yet."is a raw literal while every other label in this component (and the newgroup.*locale keys added in this same PR) goes throught(...). This string won't be translated for non-English users.💚 Suggested fix
- <p className="px-1 text-xs text-gray-400">No groups joined yet.</p> + <p className="px-1 text-xs text-gray-400">{t("group.no-groups-joined")}</p>(add a corresponding
group.no-groups-joinedkey to the locale files)🤖 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 `@web/src/components/MemoExplorer/GroupsSection.tsx` around lines 11 - 26, The empty-state message in GroupsSection is hardcoded English and bypasses i18n. Replace the raw “No groups joined yet.” text in the GroupsSection component with a translation lookup via t(...), and add or use a matching locale key such as group.no-groups-joined alongside the existing group.* keys so the empty state is translated like the rest of this component.store/migration/sqlite/LATEST.sql (2)
41-42: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider indexing the new
group_idcolumn.
memo.group_idis now used inWHERE memo.group_id = ?filters (perListMemosin the MySQL/Postgres/SQLite drivers) and via the new CELgroup_idfield. Without an index, group-scoped timeline queries will require a full table scan as memo volume grows.🤖 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 `@store/migration/sqlite/LATEST.sql` around lines 41 - 42, Add an index for the new memo.group_id field in the SQLite schema so group-scoped lookups do not fall back to full table scans. Update the migration in LATEST.sql alongside the memo table definition to index group_id, and keep it consistent with how ListMemos and the new CEL group_id filter query this column across the SQLite driver.
128-145: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMissing CHECK constraints on
groups.visibilityandgroup_members.role.
memo.visibilityenforces aCHECK (visibility IN (...))constraint, but the newgroups.visibilityandgroup_members.rolecolumns accept any string with no equivalent validation. This is inconsistent with the established defense-in-depth pattern in this schema and could allow invalid values to be persisted from application bugs (especially sinceGroupMember.Rolein Go is an untyped string).🛡️ Suggested constraints
CREATE TABLE groups ( id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT NOT NULL, description TEXT NOT NULL DEFAULT '', creator_id INTEGER NOT NULL, - visibility TEXT NOT NULL DEFAULT 'PRIVATE', + visibility TEXT NOT NULL CHECK (visibility IN ('PUBLIC', 'PROTECTED', 'PRIVATE')) DEFAULT 'PRIVATE', created_ts BIGINT NOT NULL DEFAULT (strftime('%s', 'now')) ); -- group_members CREATE TABLE group_members ( group_id INTEGER NOT NULL, user_id INTEGER NOT NULL, - role TEXT NOT NULL DEFAULT 'MEMBER', + role TEXT NOT NULL CHECK (role IN ('MEMBER', 'ADMIN', 'OWNER')) DEFAULT 'MEMBER', PRIMARY KEY (group_id, user_id) );Also consider adding a secondary index on
group_members(user_id)— the composite PK(group_id, user_id)doesn't efficiently serve lookups byuser_idalone (e.g.ListGroupMembersfiltering only byUserID).🤖 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 `@store/migration/sqlite/LATEST.sql` around lines 128 - 145, Add the same validation pattern used by other schema tables to the new `groups` and `group_members` definitions in `LATEST.sql`: update the `CREATE TABLE groups` and `CREATE TABLE group_members` statements to include CHECK constraints on `visibility` and `role` so only the expected enum-like values can be stored. Also add an index on `group_members(user_id)` to support lookups used by member-listing paths such as `ListGroupMembers`, since the `(group_id, user_id)` primary key does not optimize filtering by `user_id` alone.store/group.go (2)
7-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReusing
Memo'sVisibilitytype forGroup.Visibilityconflates two distinct domains.Since
Visibilitynow includesGroupVisibility("GROUP"), aGroupcould nonsensically be assigned visibility "GROUP" (a group whose visibility is "group"). Also,Group.Nameis populated with the proto'sdisplay_name(per theCreateGrouphandler), not the resource identifiername— worth renaming to avoid confusion with the proto'sname/display_namedistinction.Consider a dedicated
GroupVisibilitytype (e.g.,Public/Private) scoped to groups, and renamingGroup.NametoGroup.DisplayNamefor clarity.🤖 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 `@store/group.go` around lines 7 - 14, The `Group` model is reusing the shared `Visibility` enum and has a misleading `Name` field, which mixes group semantics with memo semantics. Update `Group.Visibility` to use a dedicated group-scoped type such as `GroupVisibility` with only group-appropriate values, and rename `Group.Name` to `Group.DisplayName` so it matches how `CreateGroup` populates the model from the proto. Adjust any references to `Group`, `Visibility`, and related constructors/mappers accordingly.
16-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
GroupMember.Roleuses an untyped string instead of typed constants.The valid values are only documented in a comment ("MEMBER", "ADMIN", "OWNER"), unlike
Visibilitywhich uses typed constants (Public,Protected,GroupVisibility) instore/memo.go. This risks typos/inconsistent casing across callers with no compile-time safety, especially since the proto layer definesRoleas a proper enum.♻️ Suggested constants
+const ( + GroupMemberRoleMember GroupMemberRole = "MEMBER" + GroupMemberRoleAdmin GroupMemberRole = "ADMIN" + GroupMemberRoleOwner GroupMemberRole = "OWNER" +) + +type GroupMemberRole string + type GroupMember struct { GroupID int32 UserID int32 - Role string // "MEMBER", "ADMIN", "OWNER" + Role GroupMemberRole }🤖 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 `@store/group.go` around lines 16 - 20, GroupMember.Role is an untyped string and should be replaced with typed role constants to match the enum-backed proto and the pattern used by Visibility in store/memo.go. Introduce a dedicated role type plus constants for the valid values (MEMBER, ADMIN, OWNER), update GroupMember and any related constructors/mappers to use those symbols, and ensure callers reference the constants instead of raw string literals to prevent typos and casing mismatches.proto/api/v1/group_service.proto (1)
103-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding an explicit
userreference field toGroupMember.
Groupexposes a dedicatedcreatorfield withresource_reference, butGroupMemberonly encodes the user id inside the resourcenamestring. Clients need to parsenameto determine the associated user rather than getting a structured reference likestring user = 3 [(google.api.resource_reference) = {type: "memos.api.v1/User"}];.🤖 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 `@proto/api/v1/group_service.proto` around lines 103 - 124, Add an explicit structured user reference to GroupMember instead of relying only on the user ID embedded in name. Update the GroupMember message in group_service.proto to include a user field with google.api.resource_reference targeting memos.api.v1/User, while keeping the existing name resource identifier and role field intact. Ensure any generated/consuming code paths that construct or read GroupMember use the new user field as the canonical user link.store/driver.go (1)
84-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGroup/GroupMember delete & update signatures diverge from established interface conventions.
Every other model in this interface uses a dedicated request struct for deletes (
DeleteMemo(ctx, *DeleteMemo),DeleteUser(ctx, *DeleteUser),DeleteAttachment(ctx, *DeleteAttachment)), butDeleteGrouptakes a bareid int32, andDeleteGroupMember/UpdateGroupMemberreuse the full*GroupMemberdomain struct (carrying an unusedRolefor deletes). Aligning these with the rest of the interface now avoids inconsistent call sites and awkward evolution later.♻️ Suggested signature alignment
- DeleteGroup(ctx context.Context, id int32) error + DeleteGroup(ctx context.Context, delete *DeleteGroup) error // GroupMember model related methods. CreateGroupMember(ctx context.Context, create *GroupMember) (*GroupMember, error) ListGroupMembers(ctx context.Context, find *FindGroupMember) ([]*GroupMember, error) - UpdateGroupMember(ctx context.Context, update *GroupMember) (*GroupMember, error) - DeleteGroupMember(ctx context.Context, delete *GroupMember) error + UpdateGroupMember(ctx context.Context, update *UpdateGroupMember) (*GroupMember, error) + DeleteGroupMember(ctx context.Context, delete *DeleteGroupMember) error🤖 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 `@store/driver.go` around lines 84 - 95, The Group and GroupMember delete/update methods in the Driver interface are inconsistent with the rest of the model APIs. Update DeleteGroup, UpdateGroupMember, and DeleteGroupMember to use dedicated request structs like the other delete methods (for example, matching the DeleteMemo/DeleteUser pattern), and avoid passing the full GroupMember domain object when only delete criteria are needed. Keep the interface signatures aligned in store/driver.go and adjust the corresponding implementations/call sites for CreateGroup, UpdateGroup, DeleteGroup, CreateGroupMember, UpdateGroupMember, and DeleteGroupMember as needed.store/migration/mysql/LATEST.sql (1)
41-41: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMissing index on
memo.group_id.
group_idis registered as a filterable CEL field (internal/filter/schema.gomapsgroup_idtomemo.group_idwithCompareEq/CompareNeq), and GROUP-visibility memo listings (e.g. group timelines) will filter by it. Without an index, these queries force full table scans as the memo table grows.💡 Proposed fix
`group_id` INT DEFAULT NULL, `pinned` BOOLEAN NOT NULL DEFAULT FALSE, `payload` JSON NOT NULL ); + +CREATE INDEX `idx_memo_group_id` ON `memo` (`group_id`);🤖 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 `@store/migration/mysql/LATEST.sql` at line 41, Add an index for memo.group_id in the MySQL migration so GROUP-based memo filters do not scan the whole table; update the LATEST.sql schema near the group_id column to create the appropriate index, and keep it consistent with the filterable field mapping in internal/filter/schema.go and any memo query paths that use group visibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 985ae968-3ff2-4323-b8a3-f910aba83fa1
⛔ Files ignored due to path filters (8)
proto/gen/api/v1/apiv1connect/group_service.connect.gois excluded by!**/gen/**proto/gen/api/v1/group_service.pb.gois excluded by!**/*.pb.go,!**/gen/**proto/gen/api/v1/group_service.pb.gw.gois excluded by!**/*.pb.gw.go,!**/gen/**proto/gen/api/v1/group_service_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**proto/gen/api/v1/memo_service.pb.gois excluded by!**/*.pb.go,!**/gen/**proto/gen/openapi.yamlis excluded by!**/gen/**proto/gen/store/instance_setting.pb.gois excluded by!**/*.pb.go,!**/gen/**server/router/frontend/dist/index.htmlis excluded by!**/dist/**
📒 Files selected for processing (64)
internal/filter/schema.goproto/api/v1/group_service.protoproto/api/v1/memo_service.protoserver/router/api/v1/connect_handler.goserver/router/api/v1/connect_services.goserver/router/api/v1/group_service.goserver/router/api/v1/memo_service.goserver/router/api/v1/memo_service_converter.goserver/router/api/v1/sse_hub.goserver/router/api/v1/user_service_stats.goserver/router/api/v1/v1.gostore/db/mysql/group.gostore/db/mysql/memo.gostore/db/postgres/group.gostore/db/postgres/memo.gostore/db/sqlite/group.gostore/db/sqlite/memo.gostore/driver.gostore/group.gostore/memo.gostore/migration/mysql/0.29/00__group.sqlstore/migration/mysql/LATEST.sqlstore/migration/postgres/0.29/00__group.sqlstore/migration/postgres/LATEST.sqlstore/migration/sqlite/0.29/00__group.sqlstore/migration/sqlite/0.29/01__fix_memo_visibility.sqlstore/migration/sqlite/LATEST.sqlweb/src/components/CreateGroupDialog.tsxweb/src/components/MemoActionMenu/MemoActionMenu.tsxweb/src/components/MemoEditor/Toolbar/VisibilitySelector.tsxweb/src/components/MemoEditor/components/EditorMetadata.tsxweb/src/components/MemoEditor/components/GroupSelector.tsxweb/src/components/MemoEditor/components/index.tsweb/src/components/MemoEditor/services/memoService.tsweb/src/components/MemoEditor/state/types.tsweb/src/components/MemoExplorer/GroupsSection.tsxweb/src/components/MemoExplorer/MemoExplorer.tsxweb/src/components/Navigation.tsxweb/src/components/VisibilityIcon.tsxweb/src/connect.tsweb/src/hooks/useFilteredMemoStats.tsweb/src/hooks/useGroupQueries.tsweb/src/hooks/useMemoFilters.tsweb/src/locales/ar.jsonweb/src/locales/en.jsonweb/src/locales/fa.jsonweb/src/locales/hi.jsonweb/src/locales/id.jsonweb/src/locales/ko.jsonweb/src/locales/mr.jsonweb/src/locales/pt-BR.jsonweb/src/locales/th.jsonweb/src/locales/vi.jsonweb/src/locales/zh-Hans.jsonweb/src/locales/zh-Hant.jsonweb/src/pages/Explore.tsxweb/src/pages/GroupDetail.tsxweb/src/pages/GroupTimeline.tsxweb/src/pages/Home.tsxweb/src/router/index.tsxweb/src/router/routes.tsweb/src/types/proto/api/v1/group_service_pb.tsweb/src/types/proto/api/v1/memo_service_pb.tsweb/src/utils/i18n.ts
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/router/api/v1/group_service.go (1)
60-73: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
request.Groupandrequest.UpdateMaskagainst nil to avoid a panic.
UpdateGroupdereferencesrequest.Group.Nameat Line 70 andrequest.UpdateMask.Pathsat Line 116 without nil checks. A client omitting either field triggers a nil-pointer panic on the request thread. NoteUpdateMemoalready guardsUpdateMask(seememo_service.goLine 512); mirror that here.🛡️ Proposed guard
if user == nil { return nil, status.Errorf(codes.Unauthenticated, "user not authenticated") } + if request.Group == nil { + return nil, status.Errorf(codes.InvalidArgument, "group is required") + } + if request.UpdateMask == nil || len(request.UpdateMask.Paths) == 0 { + return nil, status.Errorf(codes.InvalidArgument, "update mask is required") + } var groupID int32 _, err = fmt.Sscanf(request.Group.Name, "groups/%d", &groupID)🤖 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 `@server/router/api/v1/group_service.go` around lines 60 - 73, UpdateGroup currently dereferences request.Group.Name and later request.UpdateMask.Paths without checking for nil, which can panic when clients omit those fields. Add explicit nil guards at the start of APIV1Service.UpdateGroup for request.Group and request.UpdateMask, returning an InvalidArgument error when either is missing, and keep the existing parsing/update logic unchanged once both are validated; mirror the nil-check pattern used by UpdateMemo.store/db/postgres/group.go (1)
44-80: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMissing
rows.Err()check after iteration.Same gap as noted for the mysql driver: a mid-scan connection error would silently return a truncated list.
🛡️ Proposed fix
list = append(list, &group) } + if err := rows.Err(); err != nil { + return nil, errors.Wrap(err, "failed to iterate groups") + } return list, 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 `@store/db/postgres/group.go` around lines 44 - 80, The ListGroups method in DB should check for iteration errors after the rows loop, since a mid-scan failure can otherwise return a partial result. Update the store/db/postgres/group.go implementation of DB.ListGroups to inspect rows.Err() after the for rows.Next() loop and return a wrapped error if it is non-nil, keeping the existing scan/query error handling unchanged.store/db/mysql/group.go (1)
70-106: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMissing
rows.Err()check after iteration.If the connection drops mid-scan,
rows.Next()just returnsfalseand the loop exits silently, returning a truncated list with no error.🛡️ Proposed fix
list = append(list, &group) } + if err := rows.Err(); err != nil { + return nil, errors.Wrap(err, "failed to iterate groups") + } return list, 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 `@store/db/mysql/group.go` around lines 70 - 106, The ListGroups method in DB is missing a rows.Err() check after iterating over rows.Next(), so scan/connection errors can be swallowed and return a partial result. Update the loop in ListGroups to check rows.Err() after the iteration completes, and return a wrapped error if it is non-nil, alongside the existing query and scan error handling.
♻️ Duplicate comments (5)
store/db/postgres/group.go (1)
141-171: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSame missing
rows.Err()check here.See comment on
ListGroups(lines 44-80) — same fix applies toListGroupMembers.🤖 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 `@store/db/postgres/group.go` around lines 141 - 171, ListGroupMembers has the same missing post-iteration error handling as ListGroups. After the rows.Next() loop in DB.ListGroupMembers, check rows.Err() and return a wrapped error if iteration failed; keep the existing query/scan flow unchanged and use the ListGroupMembers method plus rows.Next/rows.Err as the key spots to update.store/db/sqlite/group.go (3)
127-138: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
DeleteGroupisn't transactional.Same atomicity gap flagged in the mysql and postgres drivers: the unset/delete/delete sequence runs outside a transaction, unlike
CreateGroupabove in this same file. A crash or failure mid-sequence leaves the DB in a partially-cleaned state.🔒️ Proposed fix
func (d *DB) DeleteGroup(ctx context.Context, id int32) error { - if _, err := d.db.ExecContext(ctx, "UPDATE memo SET group_id = NULL WHERE group_id = ?", id); err != nil { - return errors.Wrap(err, "failed to unset group_id in memo") - } - if _, err := d.db.ExecContext(ctx, "DELETE FROM group_members WHERE group_id = ?", id); err != nil { - return errors.Wrap(err, "failed to delete group members") - } - if _, err := d.db.ExecContext(ctx, "DELETE FROM groups WHERE id = ?", id); err != nil { - return errors.Wrap(err, "failed to delete group") - } - return nil + tx, err := d.db.BeginTx(ctx, nil) + if err != nil { + return errors.Wrap(err, "failed to begin transaction") + } + defer func() { _ = tx.Rollback() }() + + if _, err := tx.ExecContext(ctx, "UPDATE memo SET group_id = NULL WHERE group_id = ?", id); err != nil { + return errors.Wrap(err, "failed to unset group_id in memo") + } + if _, err := tx.ExecContext(ctx, "DELETE FROM group_members WHERE group_id = ?", id); err != nil { + return errors.Wrap(err, "failed to delete group members") + } + if _, err := tx.ExecContext(ctx, "DELETE FROM groups WHERE id = ?", id); err != nil { + return errors.Wrap(err, "failed to delete group") + } + if err := tx.Commit(); err != nil { + return errors.Wrap(err, "failed to commit transaction") + } + 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 `@store/db/sqlite/group.go` around lines 127 - 138, DeleteGroup in DB should run the unset/delete/delete sequence atomically instead of issuing separate ExecContext calls on d.db. Wrap the logic in a transaction using the same DB handle pattern as CreateGroup, execute the memo, group_members, and groups updates/deletes through that transaction, and make sure any error rolls back and the transaction is committed only after all steps succeed.
64-100: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMissing
rows.Err()check after iteration.Same gap as flagged in the mysql/postgres drivers — a mid-scan error silently truncates the returned list.
🛡️ Proposed fix
list = append(list, &group) } + if err := rows.Err(); err != nil { + return nil, errors.Wrap(err, "failed to iterate groups") + } return list, 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 `@store/db/sqlite/group.go` around lines 64 - 100, ListGroups is missing a final rows.Err() check after the rows.Next() loop, so a mid-iteration scan failure can be hidden and return a truncated result. Update the ListGroups method in the sqlite DB group query path to check rows.Err() after the loop and return a wrapped error if it is non-nil, alongside the existing query and scan error handling.
166-196: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSame missing
rows.Err()check here.See comment on
ListGroups(lines 64-100) — same fix applies toListGroupMembers.🤖 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 `@store/db/sqlite/group.go` around lines 166 - 196, ListGroupMembers has the same result-iteration bug as ListGroups: it returns after the rows loop without checking rows.Err(), so iteration errors can be missed. Update the ListGroupMembers method in group.go to inspect rows.Err() after the for rows.Next() loop and return a wrapped error if it is non-nil, using the existing error style alongside the rows.Scan and QueryContext handling.store/db/mysql/group.go (1)
172-202: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSame missing
rows.Err()check here.See comment on
ListGroups(lines 70-106) — same fix applies toListGroupMembers.🤖 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 `@store/db/mysql/group.go` around lines 172 - 202, ListGroupMembers is missing the post-iteration rows.Err() check, so scan/query iteration errors can be dropped. Update the ListGroupMembers method in DB to mirror the fix used in ListGroups: after the rows.Next() loop and before returning the list, check rows.Err() and wrap/return any error. Use the existing ListGroupMembers symbol and keep the existing error handling style consistent with the query and scan paths.
🧹 Nitpick comments (3)
server/router/api/v1/memo_service.go (1)
266-284: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the GROUP visibility filter builder to remove triplicated logic.
This exact block (fetch memberships → build
group_id == %ddisjunction →visibility == "GROUP"filter) is repeated inListAllUserStatsandGetUserStats(user_service_stats.go). A shared helper such asbuildGroupVisibilityFilter(ctx, currentUser)would keep the access-control expression consistent across all three read paths, which is important since divergence here is a data-exposure risk.🤖 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 `@server/router/api/v1/memo_service.go` around lines 266 - 284, The GROUP visibility filter construction is duplicated across memo and user stats read paths, so extract it into a shared helper to keep access control consistent. Move the membership lookup and the “group_id == %d” disjunction plus “visibility == "GROUP"” wrapping into a reusable function such as buildGroupVisibilityFilter(ctx, currentUser) and use it from memo_service.go and the corresponding logic in ListAllUserStats and GetUserStats in user_service_stats.go.server/router/api/v1/group_service.go (1)
83-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated owner/admin permission block in
UpdateGroupandDeleteGroup.The membership fetch and
isOwnerOrAdmincomputation (Lines 83-110 and 155-182) are identical. Consider extracting a helper likes.canManageGroup(ctx, group, user)to keep the two authorization paths in sync as roles evolve. Also prefer typed constants over the literal"OWNER"/"ADMIN"role strings.Also applies to: 155-182
🤖 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 `@server/router/api/v1/group_service.go` around lines 83 - 110, The owner/admin authorization logic is duplicated in UpdateGroup and DeleteGroup, so extract the shared membership lookup and permission check into a helper on the service, such as s.canManageGroup(ctx, group, user), and call it from both methods to keep behavior in sync. Move the ListGroupMembers and isOwnerOrAdmin computation out of the two handlers, and replace the literal "OWNER"/"ADMIN" comparisons with the existing typed role constants from store so role changes stay centralized.store/migration/postgres/0.29/00__group.sql (1)
13-13: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdding the FK in the same
ALTER TABLEtakes aSHARE ROW EXCLUSIVElock onmemoandgroups.On a large, actively-written
memotable this blocks concurrent writes during the constraint validation scan. Since new rows defaultgroup_idtoNULL, the scan is cheap, but the lock is still acquired. To minimize blocking, split the column add from the constraint: add the constraintNOT VALIDfirst, thenVALIDATE CONSTRAINTin a separate transaction.♻️ Suggested split to reduce lock contention
-- Add group_id column to memo table ALTER TABLE memo ADD COLUMN group_id INTEGER DEFAULT NULL; -- Add FK without an immediate full-table validation scan ALTER TABLE memo ADD CONSTRAINT memo_group_id_fkey FOREIGN KEY (group_id) REFERENCES groups(id) ON DELETE SET NULL NOT VALID; -- Validate in a separate transaction (takes a weaker lock) ALTER TABLE memo VALIDATE CONSTRAINT memo_group_id_fkey;Source: Linters/SAST tools
🤖 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 `@server/router/api/v1/memo_service.go`:
- Around line 571-580: The UpdateMemo path handling for request.Memo.Group
silently ignores malformed non-nil values because the fmt.Sscanf parse failure
in the group branch is not surfaced. Update the group parsing logic in
UpdateMemo so that when request.Memo.Group is present but does not match
groups/<id>, it returns an InvalidArgument error instead of leaving
update.GroupID and update.ClearGroupID unset; keep the existing nil case
behavior that clears the group association.
- Around line 259-264: The ListGroupMembers call is being ignored when it fails,
which leaves myGroupIDs empty and causes the memo filter to exclude the user’s
GROUP memos. Update the logic in memo service code that builds the CEL filter to
handle the returned err from s.Store.ListGroupMembers by propagating it instead
of continuing with an empty list. Apply the same error-handling pattern wherever
ListGroupMembers is used in sse_handler and user_service_stats so transient
store failures do not silently hide data.
In `@store/db/mysql/group.go`:
- Around line 133-144: DeleteGroup currently performs three separate writes
without transactional protection, so partial cleanup can leave the database
inconsistent. Update DB.DeleteGroup to follow the same transaction pattern used
by CreateGroup: start a transaction with BeginTx, run the memo.group_id update,
group_members delete, and groups delete through that tx, and commit only after
all succeed. Make sure any failure rolls back the transaction and returns the
wrapped error.
In `@store/db/postgres/group.go`:
- Around line 107-118: DeleteGroup in DB is executing the UPDATE memo, DELETE
group_members, and DELETE groups statements outside a transaction, which can
leave partial cleanup if one step fails. Update DeleteGroup to follow the same
transactional pattern used by CreateGroup: begin a transaction from d.db, run
all three ExecContext calls on that transaction, and commit only after all
succeed, with rollback on any error. Keep the existing error wrapping in
DeleteGroup and use the transaction object consistently so the operation is
atomic.
---
Outside diff comments:
In `@server/router/api/v1/group_service.go`:
- Around line 60-73: UpdateGroup currently dereferences request.Group.Name and
later request.UpdateMask.Paths without checking for nil, which can panic when
clients omit those fields. Add explicit nil guards at the start of
APIV1Service.UpdateGroup for request.Group and request.UpdateMask, returning an
InvalidArgument error when either is missing, and keep the existing
parsing/update logic unchanged once both are validated; mirror the nil-check
pattern used by UpdateMemo.
In `@store/db/mysql/group.go`:
- Around line 70-106: The ListGroups method in DB is missing a rows.Err() check
after iterating over rows.Next(), so scan/connection errors can be swallowed and
return a partial result. Update the loop in ListGroups to check rows.Err() after
the iteration completes, and return a wrapped error if it is non-nil, alongside
the existing query and scan error handling.
In `@store/db/postgres/group.go`:
- Around line 44-80: The ListGroups method in DB should check for iteration
errors after the rows loop, since a mid-scan failure can otherwise return a
partial result. Update the store/db/postgres/group.go implementation of
DB.ListGroups to inspect rows.Err() after the for rows.Next() loop and return a
wrapped error if it is non-nil, keeping the existing scan/query error handling
unchanged.
---
Duplicate comments:
In `@store/db/mysql/group.go`:
- Around line 172-202: ListGroupMembers is missing the post-iteration rows.Err()
check, so scan/query iteration errors can be dropped. Update the
ListGroupMembers method in DB to mirror the fix used in ListGroups: after the
rows.Next() loop and before returning the list, check rows.Err() and wrap/return
any error. Use the existing ListGroupMembers symbol and keep the existing error
handling style consistent with the query and scan paths.
In `@store/db/postgres/group.go`:
- Around line 141-171: ListGroupMembers has the same missing post-iteration
error handling as ListGroups. After the rows.Next() loop in DB.ListGroupMembers,
check rows.Err() and return a wrapped error if iteration failed; keep the
existing query/scan flow unchanged and use the ListGroupMembers method plus
rows.Next/rows.Err as the key spots to update.
In `@store/db/sqlite/group.go`:
- Around line 127-138: DeleteGroup in DB should run the unset/delete/delete
sequence atomically instead of issuing separate ExecContext calls on d.db. Wrap
the logic in a transaction using the same DB handle pattern as CreateGroup,
execute the memo, group_members, and groups updates/deletes through that
transaction, and make sure any error rolls back and the transaction is committed
only after all steps succeed.
- Around line 64-100: ListGroups is missing a final rows.Err() check after the
rows.Next() loop, so a mid-iteration scan failure can be hidden and return a
truncated result. Update the ListGroups method in the sqlite DB group query path
to check rows.Err() after the loop and return a wrapped error if it is non-nil,
alongside the existing query and scan error handling.
- Around line 166-196: ListGroupMembers has the same result-iteration bug as
ListGroups: it returns after the rows loop without checking rows.Err(), so
iteration errors can be missed. Update the ListGroupMembers method in group.go
to inspect rows.Err() after the for rows.Next() loop and return a wrapped error
if it is non-nil, using the existing error style alongside the rows.Scan and
QueryContext handling.
---
Nitpick comments:
In `@server/router/api/v1/group_service.go`:
- Around line 83-110: The owner/admin authorization logic is duplicated in
UpdateGroup and DeleteGroup, so extract the shared membership lookup and
permission check into a helper on the service, such as s.canManageGroup(ctx,
group, user), and call it from both methods to keep behavior in sync. Move the
ListGroupMembers and isOwnerOrAdmin computation out of the two handlers, and
replace the literal "OWNER"/"ADMIN" comparisons with the existing typed role
constants from store so role changes stay centralized.
In `@server/router/api/v1/memo_service.go`:
- Around line 266-284: The GROUP visibility filter construction is duplicated
across memo and user stats read paths, so extract it into a shared helper to
keep access control consistent. Move the membership lookup and the “group_id ==
%d” disjunction plus “visibility == "GROUP"” wrapping into a reusable function
such as buildGroupVisibilityFilter(ctx, currentUser) and use it from
memo_service.go and the corresponding logic in ListAllUserStats and GetUserStats
in user_service_stats.go.
🪄 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: f82fff62-2ff9-43aa-a8b0-765b3c592a13
📒 Files selected for processing (33)
server/router/api/v1/group_service.goserver/router/api/v1/memo_service.goserver/router/api/v1/sse_handler.goserver/router/api/v1/sse_hub.goserver/router/api/v1/sse_hub_test.goserver/router/api/v1/sse_service_test.goserver/router/api/v1/user_service_stats.gostore/db/mysql/group.gostore/db/mysql/memo.gostore/db/postgres/group.gostore/db/postgres/memo.gostore/db/sqlite/group.gostore/db/sqlite/memo.gostore/group.gostore/memo.gostore/migration/mysql/0.29/00__group.sqlstore/migration/mysql/LATEST.sqlstore/migration/postgres/0.29/00__group.sqlstore/migration/postgres/LATEST.sqlstore/migration/sqlite/0.29/00__group.sqlstore/migration/sqlite/LATEST.sqlstore/test/migrator_test.goweb/src/components/MemoEditor/Toolbar/VisibilitySelector.tsxweb/src/components/MemoEditor/components/EditorMetadata.tsxweb/src/components/MemoEditor/components/GroupSelector.tsxweb/src/components/MemoEditor/services/memoService.tsweb/src/components/VisibilityIcon.tsxweb/src/hooks/useGroupQueries.tsweb/src/hooks/useMemoFilters.tsweb/src/pages/Explore.tsxweb/src/pages/GroupTimeline.tsxweb/src/utils/i18n.tsweb/vite.config.mts
🚧 Files skipped from review as they are similar to previous changes (19)
- web/src/pages/Explore.tsx
- web/src/components/MemoEditor/Toolbar/VisibilitySelector.tsx
- store/migration/sqlite/0.29/00__group.sql
- web/src/utils/i18n.ts
- store/db/mysql/memo.go
- web/src/components/VisibilityIcon.tsx
- web/src/components/MemoEditor/components/EditorMetadata.tsx
- store/migration/mysql/LATEST.sql
- store/migration/mysql/0.29/00__group.sql
- server/router/api/v1/sse_hub.go
- store/migration/postgres/LATEST.sql
- store/db/postgres/memo.go
- store/migration/sqlite/LATEST.sql
- store/memo.go
- web/src/hooks/useMemoFilters.ts
- web/src/components/MemoEditor/services/memoService.ts
- web/src/hooks/useGroupQueries.ts
- store/group.go
- web/src/pages/GroupTimeline.tsx
| members, err := s.Store.ListGroupMembers(ctx, &store.FindGroupMember{UserID: ¤tUser.ID}) | ||
| if err == nil { | ||
| for _, member := range members { | ||
| myGroupIDs = append(myGroupIDs, member.GroupID) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Silently swallowed ListGroupMembers error hides a user's own GROUP memos.
if err == nil drops the error and leaves myGroupIDs empty on a transient store failure, so the built CEL filter degrades to group_id == -1 and the authenticated member silently sees none of their GROUP-visible memos. Propagate the error instead of masking it. The same pattern appears in sse_handler.go (Lines 61-66) and user_service_stats.go (Lines 90-95, 253-258).
🛠️ Suggested handling
- members, err := s.Store.ListGroupMembers(ctx, &store.FindGroupMember{UserID: ¤tUser.ID})
- if err == nil {
- for _, member := range members {
- myGroupIDs = append(myGroupIDs, member.GroupID)
- }
- }
+ members, err := s.Store.ListGroupMembers(ctx, &store.FindGroupMember{UserID: ¤tUser.ID})
+ if err != nil {
+ return nil, status.Errorf(codes.Internal, "failed to list group members")
+ }
+ for _, member := range members {
+ myGroupIDs = append(myGroupIDs, member.GroupID)
+ }📝 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.
| members, err := s.Store.ListGroupMembers(ctx, &store.FindGroupMember{UserID: ¤tUser.ID}) | |
| if err == nil { | |
| for _, member := range members { | |
| myGroupIDs = append(myGroupIDs, member.GroupID) | |
| } | |
| } | |
| members, err := s.Store.ListGroupMembers(ctx, &store.FindGroupMember{UserID: ¤tUser.ID}) | |
| if err != nil { | |
| return nil, status.Errorf(codes.Internal, "failed to list group members") | |
| } | |
| for _, member := range members { | |
| myGroupIDs = append(myGroupIDs, member.GroupID) | |
| } |
🤖 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 `@server/router/api/v1/memo_service.go` around lines 259 - 264, The
ListGroupMembers call is being ignored when it fails, which leaves myGroupIDs
empty and causes the memo filter to exclude the user’s GROUP memos. Update the
logic in memo service code that builds the CEL filter to handle the returned err
from s.Store.ListGroupMembers by propagating it instead of continuing with an
empty list. Apply the same error-handling pattern wherever ListGroupMembers is
used in sse_handler and user_service_stats so transient store failures do not
silently hide data.
| } else if path == "group" { | ||
| if request.Memo.Group != nil { | ||
| var groupID int32 | ||
| if _, err := fmt.Sscanf(*request.Memo.Group, "groups/%d", &groupID); err == nil { | ||
| update.GroupID = &groupID | ||
| } | ||
| } else { | ||
| // If group is null, set to null in DB | ||
| update.ClearGroupID = true | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Malformed group value is silently dropped in UpdateMemo.
When request.Memo.Group is non-nil but not groups/<id>, Sscanf returns an error, the if ... == nil branch is skipped, and neither GroupID nor ClearGroupID is set. The update then silently no-ops the group association instead of reporting the bad input. Return InvalidArgument on parse failure.
🛠️ Suggested handling
} else if path == "group" {
if request.Memo.Group != nil {
var groupID int32
- if _, err := fmt.Sscanf(*request.Memo.Group, "groups/%d", &groupID); err == nil {
- update.GroupID = &groupID
- }
+ if _, err := fmt.Sscanf(*request.Memo.Group, "groups/%d", &groupID); err != nil {
+ return nil, status.Errorf(codes.InvalidArgument, "invalid group name: %s", *request.Memo.Group)
+ }
+ update.GroupID = &groupID
} else {
update.ClearGroupID = true
}
}📝 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.
| } else if path == "group" { | |
| if request.Memo.Group != nil { | |
| var groupID int32 | |
| if _, err := fmt.Sscanf(*request.Memo.Group, "groups/%d", &groupID); err == nil { | |
| update.GroupID = &groupID | |
| } | |
| } else { | |
| // If group is null, set to null in DB | |
| update.ClearGroupID = true | |
| } | |
| } else if path == "group" { | |
| if request.Memo.Group != nil { | |
| var groupID int32 | |
| if _, err := fmt.Sscanf(*request.Memo.Group, "groups/%d", &groupID); err != nil { | |
| return nil, status.Errorf(codes.InvalidArgument, "invalid group name: %s", *request.Memo.Group) | |
| } | |
| update.GroupID = &groupID | |
| } else { | |
| // If group is null, set to null in DB | |
| update.ClearGroupID = true | |
| } |
🤖 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 `@server/router/api/v1/memo_service.go` around lines 571 - 580, The UpdateMemo
path handling for request.Memo.Group silently ignores malformed non-nil values
because the fmt.Sscanf parse failure in the group branch is not surfaced. Update
the group parsing logic in UpdateMemo so that when request.Memo.Group is present
but does not match groups/<id>, it returns an InvalidArgument error instead of
leaving update.GroupID and update.ClearGroupID unset; keep the existing nil case
behavior that clears the group association.
| func (d *DB) DeleteGroup(ctx context.Context, id int32) error { | ||
| if _, err := d.db.ExecContext(ctx, "UPDATE memo SET group_id = NULL WHERE group_id = ?", id); err != nil { | ||
| return errors.Wrap(err, "failed to unset group_id in memo") | ||
| } | ||
| if _, err := d.db.ExecContext(ctx, "DELETE FROM group_members WHERE group_id = ?", id); err != nil { | ||
| return errors.Wrap(err, "failed to delete group members") | ||
| } | ||
| if _, err := d.db.ExecContext(ctx, "DELETE FROM groups WHERE id = ?", id); err != nil { | ||
| return errors.Wrap(err, "failed to delete group") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
DeleteGroup isn't transactional.
Unlike CreateGroup (which correctly wraps its writes in BeginTx/Commit), DeleteGroup runs 3 separate statements (unset memo.group_id, delete group_members, delete groups) without a transaction. If the process crashes or a later step fails, the DB is left in a partially-cleaned state (e.g., group deleted but stale group_members rows remain, or vice versa).
🔒️ Proposed fix
func (d *DB) DeleteGroup(ctx context.Context, id int32) error {
- if _, err := d.db.ExecContext(ctx, "UPDATE memo SET group_id = NULL WHERE group_id = ?", id); err != nil {
- return errors.Wrap(err, "failed to unset group_id in memo")
- }
- if _, err := d.db.ExecContext(ctx, "DELETE FROM group_members WHERE group_id = ?", id); err != nil {
- return errors.Wrap(err, "failed to delete group members")
- }
- if _, err := d.db.ExecContext(ctx, "DELETE FROM groups WHERE id = ?", id); err != nil {
- return errors.Wrap(err, "failed to delete group")
- }
- return nil
+ tx, err := d.db.BeginTx(ctx, nil)
+ if err != nil {
+ return errors.Wrap(err, "failed to begin transaction")
+ }
+ defer func() { _ = tx.Rollback() }()
+
+ if _, err := tx.ExecContext(ctx, "UPDATE memo SET group_id = NULL WHERE group_id = ?", id); err != nil {
+ return errors.Wrap(err, "failed to unset group_id in memo")
+ }
+ if _, err := tx.ExecContext(ctx, "DELETE FROM group_members WHERE group_id = ?", id); err != nil {
+ return errors.Wrap(err, "failed to delete group members")
+ }
+ if _, err := tx.ExecContext(ctx, "DELETE FROM groups WHERE id = ?", id); err != nil {
+ return errors.Wrap(err, "failed to delete group")
+ }
+ if err := tx.Commit(); err != nil {
+ return errors.Wrap(err, "failed to commit transaction")
+ }
+ return 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.
| func (d *DB) DeleteGroup(ctx context.Context, id int32) error { | |
| if _, err := d.db.ExecContext(ctx, "UPDATE memo SET group_id = NULL WHERE group_id = ?", id); err != nil { | |
| return errors.Wrap(err, "failed to unset group_id in memo") | |
| } | |
| if _, err := d.db.ExecContext(ctx, "DELETE FROM group_members WHERE group_id = ?", id); err != nil { | |
| return errors.Wrap(err, "failed to delete group members") | |
| } | |
| if _, err := d.db.ExecContext(ctx, "DELETE FROM groups WHERE id = ?", id); err != nil { | |
| return errors.Wrap(err, "failed to delete group") | |
| } | |
| return nil | |
| } | |
| func (d *DB) DeleteGroup(ctx context.Context, id int32) error { | |
| tx, err := d.db.BeginTx(ctx, nil) | |
| if err != nil { | |
| return errors.Wrap(err, "failed to begin transaction") | |
| } | |
| defer func() { _ = tx.Rollback() }() | |
| if _, err := tx.ExecContext(ctx, "UPDATE memo SET group_id = NULL WHERE group_id = ?", id); err != nil { | |
| return errors.Wrap(err, "failed to unset group_id in memo") | |
| } | |
| if _, err := tx.ExecContext(ctx, "DELETE FROM group_members WHERE group_id = ?", id); err != nil { | |
| return errors.Wrap(err, "failed to delete group members") | |
| } | |
| if _, err := tx.ExecContext(ctx, "DELETE FROM groups WHERE id = ?", id); err != nil { | |
| return errors.Wrap(err, "failed to delete group") | |
| } | |
| if err := tx.Commit(); err != nil { | |
| return errors.Wrap(err, "failed to commit transaction") | |
| } | |
| 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 `@store/db/mysql/group.go` around lines 133 - 144, DeleteGroup currently
performs three separate writes without transactional protection, so partial
cleanup can leave the database inconsistent. Update DB.DeleteGroup to follow the
same transaction pattern used by CreateGroup: start a transaction with BeginTx,
run the memo.group_id update, group_members delete, and groups delete through
that tx, and commit only after all succeed. Make sure any failure rolls back the
transaction and returns the wrapped error.
| func (d *DB) DeleteGroup(ctx context.Context, id int32) error { | ||
| if _, err := d.db.ExecContext(ctx, "UPDATE memo SET group_id = NULL WHERE group_id = "+placeholder(1), id); err != nil { | ||
| return errors.Wrap(err, "failed to unset group_id in memo") | ||
| } | ||
| if _, err := d.db.ExecContext(ctx, "DELETE FROM group_members WHERE group_id = "+placeholder(1), id); err != nil { | ||
| return errors.Wrap(err, "failed to delete group members") | ||
| } | ||
| if _, err := d.db.ExecContext(ctx, "DELETE FROM groups WHERE id = "+placeholder(1), id); err != nil { | ||
| return errors.Wrap(err, "failed to delete group") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
DeleteGroup isn't transactional.
Same atomicity gap as flagged for store/db/mysql/group.go DeleteGroup: 3 sequential statements (UPDATE memo, DELETE group_members, DELETE groups) run outside a transaction, unlike CreateGroup in this same file. A failure partway leaves the DB inconsistently cleaned up. (Note: the static-analysis "SQL injection" flags on these lines are false positives — placeholder(1) just emits $1; id is passed as a bound arg.)
🔒️ Proposed fix
func (d *DB) DeleteGroup(ctx context.Context, id int32) error {
- if _, err := d.db.ExecContext(ctx, "UPDATE memo SET group_id = NULL WHERE group_id = "+placeholder(1), id); err != nil {
- return errors.Wrap(err, "failed to unset group_id in memo")
- }
- if _, err := d.db.ExecContext(ctx, "DELETE FROM group_members WHERE group_id = "+placeholder(1), id); err != nil {
- return errors.Wrap(err, "failed to delete group members")
- }
- if _, err := d.db.ExecContext(ctx, "DELETE FROM groups WHERE id = "+placeholder(1), id); err != nil {
- return errors.Wrap(err, "failed to delete group")
- }
- return nil
+ tx, err := d.db.BeginTx(ctx, nil)
+ if err != nil {
+ return errors.Wrap(err, "failed to begin transaction")
+ }
+ defer func() { _ = tx.Rollback() }()
+
+ if _, err := tx.ExecContext(ctx, "UPDATE memo SET group_id = NULL WHERE group_id = "+placeholder(1), id); err != nil {
+ return errors.Wrap(err, "failed to unset group_id in memo")
+ }
+ if _, err := tx.ExecContext(ctx, "DELETE FROM group_members WHERE group_id = "+placeholder(1), id); err != nil {
+ return errors.Wrap(err, "failed to delete group members")
+ }
+ if _, err := tx.ExecContext(ctx, "DELETE FROM groups WHERE id = "+placeholder(1), id); err != nil {
+ return errors.Wrap(err, "failed to delete group")
+ }
+ if err := tx.Commit(); err != nil {
+ return errors.Wrap(err, "failed to commit transaction")
+ }
+ return 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.
| func (d *DB) DeleteGroup(ctx context.Context, id int32) error { | |
| if _, err := d.db.ExecContext(ctx, "UPDATE memo SET group_id = NULL WHERE group_id = "+placeholder(1), id); err != nil { | |
| return errors.Wrap(err, "failed to unset group_id in memo") | |
| } | |
| if _, err := d.db.ExecContext(ctx, "DELETE FROM group_members WHERE group_id = "+placeholder(1), id); err != nil { | |
| return errors.Wrap(err, "failed to delete group members") | |
| } | |
| if _, err := d.db.ExecContext(ctx, "DELETE FROM groups WHERE id = "+placeholder(1), id); err != nil { | |
| return errors.Wrap(err, "failed to delete group") | |
| } | |
| return nil | |
| } | |
| func (d *DB) DeleteGroup(ctx context.Context, id int32) error { | |
| tx, err := d.db.BeginTx(ctx, nil) | |
| if err != nil { | |
| return errors.Wrap(err, "failed to begin transaction") | |
| } | |
| defer func() { _ = tx.Rollback() }() | |
| if _, err := tx.ExecContext(ctx, "UPDATE memo SET group_id = NULL WHERE group_id = "+placeholder(1), id); err != nil { | |
| return errors.Wrap(err, "failed to unset group_id in memo") | |
| } | |
| if _, err := tx.ExecContext(ctx, "DELETE FROM group_members WHERE group_id = "+placeholder(1), id); err != nil { | |
| return errors.Wrap(err, "failed to delete group members") | |
| } | |
| if _, err := tx.ExecContext(ctx, "DELETE FROM groups WHERE id = "+placeholder(1), id); err != nil { | |
| return errors.Wrap(err, "failed to delete group") | |
| } | |
| if err := tx.Commit(); err != nil { | |
| return errors.Wrap(err, "failed to commit transaction") | |
| } | |
| return nil | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 107-107: SQL query is built by concatenating a string literal with a variable and passed to a database/sql call (Query, Exec, QueryRow, Prepare, or their Context variants). String concatenation lets attacker-controlled input alter the query structure, enabling SQL injection. Use parameterized queries with placeholders ('?' or '') and pass the values as separate arguments instead of concatenating them into the query string.
Context: d.db.ExecContext(ctx, "UPDATE memo SET group_id = NULL WHERE group_id = "+placeholder(1), id)
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-query-string-concat-go)
[error] 110-110: SQL query is built by concatenating a string literal with a variable and passed to a database/sql call (Query, Exec, QueryRow, Prepare, or their Context variants). String concatenation lets attacker-controlled input alter the query structure, enabling SQL injection. Use parameterized queries with placeholders ('?' or '') and pass the values as separate arguments instead of concatenating them into the query string.
Context: d.db.ExecContext(ctx, "DELETE FROM group_members WHERE group_id = "+placeholder(1), id)
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-query-string-concat-go)
[error] 113-113: SQL query is built by concatenating a string literal with a variable and passed to a database/sql call (Query, Exec, QueryRow, Prepare, or their Context variants). String concatenation lets attacker-controlled input alter the query structure, enabling SQL injection. Use parameterized queries with placeholders ('?' or '') and pass the values as separate arguments instead of concatenating them into the query string.
Context: d.db.ExecContext(ctx, "DELETE FROM groups WHERE id = "+placeholder(1), id)
Note: [CWE-89] Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection').
(sql-injection-query-string-concat-go)
🤖 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 `@store/db/postgres/group.go` around lines 107 - 118, DeleteGroup in DB is
executing the UPDATE memo, DELETE group_members, and DELETE groups statements
outside a transaction, which can leave partial cleanup if one step fails. Update
DeleteGroup to follow the same transactional pattern used by CreateGroup: begin
a transaction from d.db, run all three ExecContext calls on that transaction,
and commit only after all succeed, with rollback on any error. Keep the existing
error wrapping in DeleteGroup and use the transaction object consistently so the
operation is atomic.
|
Hi all, I have addressed all the feedback and review comments:
Please let me know if there is anything else that needs adjustment. Thank you! |
2 participants
What does this PR do? This PR introduces a new "Group" visibility feature for memos. It allows users to restrict memo visibility to specific groups, expanding beyond the existing Private/Protected/Public options.
Key Changes:
Added GROUP visibility logic to the backend filter schema (internal/filter/schema.go).
Implemented a GroupSelector component in the frontend (MemoEditor) to allow users to select target groups when the visibility is set to Group (value 4).
Integrated the new selector with the updated CodeMirror editor architecture using useEditorSelector.
Why is this needed? To support community and team-based workflows where memos need to be shared only with specific predefined groups of users, rather than all registered users (Protected) or the public.