Skip to content

fix: chat user group add member #7816

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wxg0103 merged 1 commit into from
Nov 10, 2026
Merged

fix: chat user group add member #7816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions apps/system_manage/serializers/chat_user.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,8 +452,11 @@ def get_user_list(self, with_valid=True):
return ChatUserInstanceSerializer(users, many=True).data

@classmethod
def list(cls):
users = ChatUser.objects.all().order_by("-create_time")
def list(cls, nick_name=None):
query_set = ChatUser.objects.all().order_by("-create_time")
if nick_name:
query_set = query_set.filter(nick_name__contains=nick_name)
users = query_set[:200]
return ChatUserInstanceSerializer(users, many=True).data


@shaohuzhang1 shaohuzhang1 Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is functionally reasonable, but there are several issues and improvements to consider:

Potential issues

  1. Silent truncation to 200 records

    • query_set[:200] silently limits results.
    • This changes existing list() behavior and can break callers expecting all users.
    • Prefer explicit pagination (limit/offset or page/page_size) and return metadata if needed.

  2. Case-sensitive / inconsistent nickname search

    • nick_name__contains is case-sensitive on PostgreSQL, but may behave differently on MySQL/SQLite depending on collation.
    • For user-facing search, use nick_name__icontains for consistent case-insensitive matching.

  3. No input normalization or validation

    • if nick_name: skips empty string but not whitespace-only values.
    • A very long nick_name can trigger an expensive search.
    • Suggested: 
      if nick_name:
    nick_name = nick_name.strip()
    if nick_name:
        query_set = query_set.filter(nick_name__icontains=nick_name)
    • Add a max length check if this is exposed through an API.

  4. Performance risk with contains

    • nick_name__contains usually generates LIKE '%value%', which cannot use a normal B-tree index efficiently.
    • For large tables, consider:
      • istartswith if prefix search is acceptable,
      • trigram/GIN index for PostgreSQL,
      • full-text search for more advanced search.
    • Also ensure create_time is indexed because of order_by("-create_time").

  5. Non-deterministic ordering when create_time values tie

    • Add a tie-breaker for stable ordering/pagination: 
      order_by("-create_time", "-id")

  6. Possible N+1 queries

    • If ChatUserInstanceSerializer includes related fields, add appropriate select_related() / prefetch_related().

Minor style points

  • query_set is usually named queryset.
  • Consider type hints and explicit pagination parameters: 
    @classmethod
def list(cls, nick_name: str | None = None, limit: int = 200, offset: int = 0):
  • Validate/clamp limit to prevent abuse.

Suggested revised version

@classmethod
def list(cls, nick_name=None, limit=200, offset=0):
    queryset = ChatUser.objects.all()

    if nick_name:
        nick_name = nick_name.strip()
        if nick_name:
            queryset = queryset.filter(nick_name__icontains=nick_name)

    queryset = queryset.order_by("-create_time", "-id")
    users = queryset[offset:offset + limit]

    return ChatUserInstanceSerializer(users, many=True).data

Overall: the change works, but the hard-coded 200 limit and case-sensitive search are the main concerns. Add pagination, use icontains, normalize input, and verify indexing/performance for nickname searches.

Expand Down
2 changes: 1 addition & 1 deletion apps/system_manage/views/system_chat_user.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ class List(APIView):
RoleConstants.WORKSPACE_MANAGE,
)
def get(self, request: Request):
return result.success(ChatUserSerializer.list())
return result.success(ChatUserSerializer.list(request.query_params.get("nick_name", None)))

class Operate(APIView):
authentication_classes = [TokenAuth]
Expand Down
4 changes: 2 additions & 2 deletions ui/src/api/admin/system/chat-management/chat-user.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ import type {
const prefix = '/system/chat_user'

/** 获取对话用户。 */
const getChatUser = () => {
return get<ChatUserBase[]>(`${prefix}/list`)
const getChatUser = (query?: Dict<unknown>) => {
return get<ChatUserBase[]>(`${prefix}/list`, query)
}

/** 获取对话用户分页列表。 */

@shaohuzhang1 shaohuzhang1 Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No syntax irregularity in this small diff. The change is backward-compatible for normal callers, but there are a few points to verify:

  1. Confirm what the second argument of get means

    • If get<T>(url, params) expects query parameters directly, the change is correct.
    • If get<T>(url, config) is axios-like or expects a request config, passing query directly is a bug. You would need: 
      get<ChatUserBase[]>(`${prefix}/list`, { params: query })

  2. Dict<unknown> is too loose

    • It allows non-serializable values such as nested objects, arrays, Date, or functions, whose serialization may be inconsistent.
    • Prefer an explicit query DTO: 
      interface ChatUserListQuery {
  keyword?: string
  page?: number
  pageSize?: number
}

const getChatUser = (query?: ChatUserListQuery) => {
  return get<ChatUserBase[]>(`${prefix}/list`, query)
}
    • If the query is user-controlled, consider allowlisting supported parameters.

  3. Optional argument can be accidentally passed an event object

    • If getChatUser is used directly as an event handler, e.g. onClick={getChatUser}, the click event may be passed as query and forwarded to the API.
    • Use a wrapper in such cases: 
      onClick={() => getChatUser()}

  4. Ensure undefined/empty query values are handled

    • Many HTTP clients skip undefined query values, but verify your get wrapper does too.
    • If query is {}, it may produce the same request, but if your client appends ?, normalize it.

  5. Caching implications

    • If results are cached by URL or a query key, make sure the new query parameters are included in the cache key.
    • If different query values return different lists, existing cache invalidation logic may need updating.

  6. Server compatibility

    • Verify /system/chat_user/list accepts the new query parameters. Unknown parameters may be ignored by some servers but can cause 400 errors on others.

Minor naming suggestion: getChatUser returns a list, so getChatUserList would be clearer, but this is not a bug.

Recommended implementation, assuming get accepts query params directly:

interface ChatUserListQuery {
  keyword?: string
  page?: number
  pageSize?: number
}

const getChatUser = (query?: ChatUserListQuery) => {
  return get<ChatUserBase[]>(`${prefix}/list`, query)
}

Expand Down
26 changes: 21 additions & 5 deletions ui/src/views/system/chat-management/user-groups/dialog/CreateGroupMemberDialog.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,24 @@ const userOptions = ref<ChatUserBase[]>([])
const memberForm = reactive<{ userIds: string[] }>({ userIds: [] })
const formRules: FormRules = { userIds: [{ required: true, message: '请选择对话用户', trigger: 'change' }] }

function open() {
visible.value = true
function loadUserOptions(keyword = '') {
optionsLoading.value = true
return ChatUserApi.getChatUser()
return ChatUserApi.getChatUser(keyword ? { nick_name: keyword } : undefined)
.then((users) => {
userOptions.value = users
const selectedUserIds = new Set(memberForm.userIds)
const selectedUserOptions = userOptions.value.filter(({ id }) => selectedUserIds.has(id))
userOptions.value = [...new Map([...selectedUserOptions, ...users].map((user) => [user.id, user])).values()]
})
.finally(() => {
optionsLoading.value = false
})
}

function open() {
visible.value = true
return loadUserOptions()
}

function submit() {
const groupId = props.currentGroup?.id
if (!groupId) return
Expand Down Expand Up @@ -64,7 +70,17 @@ defineExpose({ open })
<MkDialog v-model="visible" title="添加成员" @closed="resetData">
<el-form ref="formRef" :model="memberForm" :rules="formRules" label-position="top" require-asterisk-position="right" @submit.prevent>
<el-form-item label="对话用户" prop="userIds">
<el-select v-model="memberForm.userIds" class="w-full" filterable multiple :loading="optionsLoading" placeholder="请选择用户名或姓名">
<el-select
v-model="memberForm.userIds"
class="w-full"
filterable
multiple
:loading="optionsLoading"
remote
:remote-method="loadUserOptions"
:reserve-keyword="false"
placeholder="请选择用户名或姓名"
>
<el-option v-for="user in userOptions" :key="user.id" :label="user.nick_name" :value="user.id" />
</el-select>
</el-form-item>

@shaohuzhang1 shaohuzhang1 Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a review of the provided code changes, focusing on logic, potential bugs, and optimization.

1. Race Condition in loadUserOptions

Issue: The asynchronous call to ChatUserApi.getChatUser does not handle stale responses. If loadUserOptions is called multiple times quickly (e.g., via remote search), a later request finishing before an earlier one will result in the earlier (stale) response overwriting the newer data.
Suggestion: Implement a cleanup function or use an AbortController to check if the current request is still active before updating state.

2. Logic Flaw: Missing Selected Options on Refresh

Issue: When loadUserOptions fetches new users based on a keyword, it merges them with existing selected options using [...new Map([...selectedUserOptions, ...users]...)]. However, if the initial load (open()) hasn't finished or if memberForm.userIds contains IDs not present in userOptions.value initially, those options might be lost or fail to render correctly until they are fetched again. More critically, if a user is selected but then filtered out by a search (server-side), the UI might rely entirely on this local merge logic. Ensure that El-Select behaves correctly when the option is not in the current options list but is in the v-model. Element Plus usually requires the option to exist in the slot or dynamically generated list to show the label properly.
Suggestion: Consider caching full user objects for all selected IDs locally so their labels can always be rendered regardless of the current userOptions list content from the API.

3. Potential Memory/Performance Issue with Large Lists

Issue: [...new Map(...).values()] creates a new array every time options are loaded. For very large datasets, this is inefficient.
Suggestion: This is generally acceptable for typical chat user lists, but ensure Map keys are stable. The current approach is O(N) which is fine unless N is very large (>10k items).

4. Missing Initial Load State Handling

Issue: open() sets visible.value = true and then calls loadUserOptions(). It's good that loading is handled, but ensure the dialog shows an appropriate empty/loading state if userOptions is initially empty before the API responds.
Suggestion: Verify that the UI gracefully handles the initial empty userOptions.value = [] state.

5. Form Reset Logic

Issue: Not visible in the diff, but resetData is called on @closed. Ensure it resets userOptions back to a default list (or clears it) and memberForm.userIds to an empty array to prevent leakage between dialog open/close cycles.

✅ Suggested Fix for Race Condition & Label Rendering

// Use a counter to ignore stale responses
let requestCounter = 0;
const selectedUsersCache = reactive(new Map<string, ChatUserBase>()); // Cache selected users' details locally

function loadUserOptions(keyword = '') {
  const currentRequest = ++requestCounter;
  optionsLoading.value = true
  
  return ChatUserApi.getChatUser(keyword ? { nick_name: keyword } : undefined)
    .then((users) => {
      // Ignore response if a newer request has started
      if (currentRequest !== requestCounter) return;

      users.forEach(user => selectedUsersCache.set(user.id, user));
      
      // Combine cached selected users + newly fetched users for display
      // You may need to filter 'users' to avoid duplicates if you don't want to re-cache everything, 
      // but here we just update cache.
      
      // To render options properly, El-Select needs the option elements.
      // If you are only rendering 'userOptions', ensure it includes ALL currently selected users 
      // whose details are known.
      const selectedSet = new Set(memberForm.userIds);
      const currentOptions = new Map<string, ChatUserBase>();

      // Add currently selected users from cache
      selectedSet.forEach(id => {
        const cachedUser = selectedUsersCache.get(id);
        if (cachedUser) currentOptions.set(id, cachedUser);
      });

      // Add fetched users
      users.forEach(user => currentOptions.set(user.id, user));

      userOptions.value = Array.from(currentOptions.values());
    })
    .catch(() => {
      // Handle error
    })
    .finally(() => {
      if (currentRequest === requestCounter) {
        optionsLoading.value = false;
      }
    });
}

Summary of Recommendations:

  1. Add race condition protection (using a counter or signal).
  2. Verify label rendering for selected items when they are not in the current filterable result set. Caching selected user objects separately is a robust pattern.
  3. Ensure resetData clears both memberForm.userIds and any local caches like userOptions or selectedUsersCache.

Expand Down
Loading