Skip to content

fix: chat user login #7806

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
shaohuzhang1 merged 1 commit into from
Nov 10, 2026
Merged

fix: chat user login #7806

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
2 changes: 1 addition & 1 deletion apps/chat/serializers/chat_user_serializer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def _get_login_policy(cls, access_token):
auth_setting.get("lock_time", 10),
)
portal = Portal.objects.first()
if portal is None or not portal.enable_auth:
if portal is None:
return ("portal", 1, 5, 10)
auth_config = portal.auth_config or {}
return (

@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 the analysis of the provided code snippet:

1. Irregularities & Potential Issues

  • Logic Change (Intentional?): The core change removes the not portal.enable_auth check. This implies the login policy limits are now applied even if authentication is disabled. If this is a security requirement, it's fine. If not, you might be enforcing lockouts on a system that isn't even using auth.
  • Hardcoded Defaults: The return value ("portal", 1, 5, 10) contains magic numbers. These should ideally match the defaults from auth_setting for consistency.
  • None Check Redundancy: Portal.objects.first() returns an object or None. The is None check is correct, but note that this queries the database every time _get_login_policy is called.

2. Optimization Suggestions

  • Caching: Calling .first() in a frequently accessed method (like login) causes unnecessary DB hits. Consider caching Portal.auth_config using functools.lru_cache, django's cache framework, or storing it in settings/environment variables if it rarely changes.
  • Magic Number Consistency: Define constants for default values to keep them synchronized with auth_setting.

3. Improved Code Suggestion

@@ -42,7 +42,19 @@ def _get_login_policy(cls, access_token):
                 auth_setting.get("lock_time", 10),
             )
         portal = Portal.objects.first()
+        # Optional: Cache this query or data if performance is critical
         if portal is None:
             # Return consistent defaults; consider defining these as class constants
             return ("portal", 1, 5, 10) 
         auth_config = portal.auth_config or {}
         # Ensure .get() returns the same default types as above to avoid type mismatches downstream
         return (
             auth_config.get("type", "portal"),
             auth_config.get("max_attempts", 1),
             auth_config.get("warning_threshold", 5),
             auth_config.get("lock_time", 10),
         )

Key Takeaway

Ensure removing enable_auth from the condition is intended. If enable_auth=False means "no security," checking portal is None only might inadvertently apply lockout rules to non-auth environments.

Expand Down
6 changes: 3 additions & 3 deletions ui/src/conversation-panel/left-sidebar/account-menu/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ const displayName = computed(() => account.value?.nickName || account.value?.use
// 配额尚未接入时保留空值展示，不将缺失数据表示为零用量。
const tokenPercentage = computed(() => {
const quota = account.value?.token_quota
if (!quota) return 0
return Math.min(100, Math.max(0, (quota.used_tokens / quota.total_tokens) * 100))
if (!quota || quota.quota_type === QUOTA_TYPE.UNLIMITED || !quota.token_limit) return 0
return Math.min(100, Math.max(0, (quota.used_tokens / quota.token_limit) * 100))
})

const userPwdDialogRef = useTemplateRef<InstanceType<typeof UserPwdDialog>>('userPwdDialogRef')
Expand Down Expand Up @@ -71,7 +71,7 @@ const goLogin = () => {
<span class="mr-3">Tokens</span>
<span class="text-primary">{{ formatTokenNumber(account?.token_quota?.used_tokens) }}</span>
<span class="text-N500">
/{{ account?.token_quota?.quota_type === QUOTA_TYPE.UNLIMITED ? '不限' : formatTokenNumber(account?.token_quota?.total_tokens) }}
/{{ account?.token_quota?.quota_type === QUOTA_TYPE.UNLIMITED ? '不限' : formatTokenNumber(account?.token_quota?.token_limit) }}
</span>
</div>

@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.

Review

1. Correctness / edge cases

!quota.token_limit conflates several states. It returns 0 for null, undefined, 0, NaN, and ''. For "missing limit" that's fine, but if the backend legitimately returns 0 (quota exhausted / no tokens allowed) the bar silently shows 0% instead of 100%. Prefer an explicit numeric guard:

if (!quota || quota.quota_type === QUOTA_TYPE.UNLIMITED) return 0
if (!quota.token_limit || quota.token_limit <= 0) return 0

used_tokens may be undefined. undefined / number → NaN, and Math.min(100, Math.max(0, NaN)) is NaN (not clamped). Must coalesce:

const used = quota.used_tokens ?? 0

Unlimited quota mapped to 0% is semantically odd. An "unlimited" plan rendered as an empty progress bar can read as "no usage" or "nothing available". Either hide the bar for unlimited (v-if="!isUnlimited") or treat it as full/indeterminate. Note the template already special-cases /不限, so the bar and text currently disagree in intent.

0 is overloaded. The computed returns 0 for "no data", "unlimited", and "0% used". If the UI needs to distinguish "unknown" from "zero", return null and let the template decide (a comment in the file explicitly says missing data should not be shown as zero usage — the computed now violates that spirit for the percentage case).

2. Consistency / DRY

  • The quota_type === QUOTA_TYPE.UNLIMITED check is now duplicated in the computed and in the template. Extract it: 
    const isUnlimited = computed(() => account.value?.token_quota?.quota_type === QUOTA_TYPE.UNLIMITED)
const tokenLimit = computed(() => account.value?.token_quota?.token_limit)
    Then both the bar and the label use the same source of truth — currently the template still references account?.token_quota?.quota_type directly, so a future change to the rule must be made twice.
  • The Math.min(100, Math.max(0, x)) clamp is repeated across the file (likely); consider a small clamp(x, 0, 100) util.

3. API / rename risk (total_tokens → token_limit)

  • Verify the backend actually ships token_limit in all environments before merging; the diff silently drops total_tokens. Grep the repo for other total_tokens usages (other components, mocks, fixtures, tests, OpenAPI/client types) — any leftover will break or show undefined.
  • If this is a staged backend rollout, a temporary fallback (quota.token_limit ?? quota.total_tokens) avoids a regression window.
  • Confirm the field is number (not a string) in the generated types.

4. Template issue not covered by the computed guard

/{{ isUnlimitedDisplay ? '不限' : formatTokenNumber(account?.token_quota?.token_limit) }}

If quota_type !== UNLIMITED but token_limit is missing, this still renders formatTokenNumber(undefined). Check that formatTokenNumber handles undefined/null (returns '-' or '', not "NaN" or "0"). The computed guards this case, the label does not — inconsistent.

5. Minor / style

  • if (!quota || ...) reads fine, but for boolean-ish checks on numbers prefer explicit == null comparisons to avoid the falsy-0 trap.
  • Consider const pct = (used / tokenLimit) * 100 before clamping for readability.
  • The Chinese comment above the computed says missing data should not be shown as zero usage; returning 0 here contradicts it. Either update the comment or return null.

Suggested refactor

const quota = computed(() => account.value?.token_quota)
const isUnlimited = computed(() => quota.value?.quota_type === QUOTA_TYPE.UNLIMITED)

const tokenPercentage = computed(() => {
  const q = quota.value
  // No quota data or unlimited plan → no meaningful ratio (not "0% used")
  if (!q || isUnlimited.value) return null
  const limit = q.token_limit
  if (!limit || limit <= 0) return null
  const used = q.used_tokens ?? 0
  return Math.min(100, Math.max(0, (used / limit) * 100))
})
<span class="text-N500">
  /{{ isUnlimited ? '不限' : formatTokenNumber(quota?.token_limit) }}
</span>
<!-- progress bar -->
<ProgressBar v-if="tokenPercentage !== null" :value="tokenPercentage" />

Bottom line: the direction of the fix is right (it avoids dividing by an undefined total_tokens and no longer treats unlimited as a ratio), but it still has a NaN path via used_tokens, a falsy-0 ambiguity on token_limit, a duplicated unlimited check, and an unguarded template label.

Expand Down
Loading