[shaohuzhang1]

fix: chat user login

[shaohuzhang1]

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.

[shaohuzhang1]

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.