[shaohuzhang1]

feat: error page

[shaohuzhang1]

Code Review

Overall the change is sound. Below are observations, potential issues, and suggestions.

Correctness / Logic

1. watch array source with immediate: true — OK but subtle. When watching an array of getters, both values are provided as a tuple on first run. That's correct here, but note that unavailable is a computed ref; combining it in the array along with a plain getter works, but mixing forms can be confusing. Consider a single getter returning an object/tuple for clarity:

   watch(
     () => [auth.token, unavailable.value] as const,
     ([token, serviceUnavailable]) => { ... },
     { immediate: true },
   )

2. Race / transient state: If route.fullPath changes while a loadCurrentUser() is in flight, the user may still load even though the page just flipped to unavailable. Probably acceptable, but if strictness matters, guard after await or use a cancellation flag.

3. unavailable reactivity relies on auth.unavailablePath being reactive. Ensure unavailablePath lives inside the store's reactive state (ref/reactive), otherwise computed won't recompute. Worth verifying.

4. watch will fire whenever token changes OR unavailable flips. That means navigating to/away from the unavailable route triggers the callback even when the token hasn't changed. The !user.chatUserProfile guard makes this harmless, but it's implicit. A comment explaining the intent would help.

5. Dependency on route inside setup is fine, but note the component must be under a router context (it is, since useRoute is used). Also confirm this view isn't rendered outside <router-view>.

Template / Rendering

6. <NoService v-if="unavailable" /> replaces the entire page including layout. If NoService is expected to render inside the standard chat layout/shell (sidebar + nav), rendering it as a sibling of <main> may break layout. If it's meant to be full-screen, consider wrapping consistently:

   <main class="h-screen bg-white">
     <NoService v-if="unavailable" />
     <ConversationChat v-else-if="auth.token" :key="auth.token" />
   </main>

   This keeps the white background and screen sizing applied uniformly. Otherwise NoService needs its own h-screen.

7. Comment/image drift: The original comment says "页面只加载侧栏所需的当前用户". With the new unavailable branch, the sidebar isn't loaded either. Update the comment.

8. v-if + v-else swap unmounts/remounts ConversationChat internally? No — since the whole <main> is toggled, going unavailable and back remounts ConversationChat, resetting panel state. If preserving state matters (e.g., draft input), prefer keeping <main> mounted and only hiding one child (see 创建模型时，添加 Ollama 供应商报错。 #6).

9. display:none vs v-if: if you want to keep chat state alive across temporary unavailability, use v-show on the panel side.

Minor / Style

10. Naming: unavailable is vague. Prefer isServiceUnavailable or isOnUnavailableRoute.

11. String comparison route.fullPath is exact-match — trailing slash, query parameters, or hash will cause mismatches. If unavailablePath is configured storage value, ensure both sides are normalized (strip trailing /, ignore query/hash) or compare route.path instead of fullPath.

12. watch import ordering: import { computed, watch } from 'vue' — fine, but if lint enforces alphabetical named imports, verify rule.

13. Store destructuring: const { auth, user } = useStore() is fine for Pinia but bypasses some tooling; ensure the store returns settled refs.

14. Type safety: ([token, serviceUnavailable]) types should be inferred; no action needed but adding explicit tuple type would catch regressions in destructuring.

15. Error handling: user.loadCurrentUser() return promise isn't awaited/caught. In a watcher, unhandled rejections can leak. Add .catch(...) or make the callback async with try/catch.

Suggested refined version

const route = useRoute()
const isServiceUnavailable = computed(
  () => auth.unavailablePath === route.path, // or normalized fullPath
)

watch(
  () => [auth.token, isServiceUnavailable.value] as const,
  async ([token, serviceUnavailable]) => {
    if (serviceUnavailable || !token || user.chatUserProfile) return
    try {
      await user.loadCurrentUser()
    } catch (err) {
      console.error('[ChatApplication] failed to load current user', err)
    }
  },
  { immediate: true },
)

Template:

<template>
  <main class="h-screen bg-white">
    <NoService v-if="isServiceUnavailable" />
    <ConversationChat
      v-else-if="auth.token"
      :key="auth.token"
    />
  </main>
</template>

Summary of priorities

• High: verify NoService renders correctly without the <main> wrapper (创建模型时，添加 Ollama 供应商报错。 #6), and that unavailablePath is reactive ([FEATURE]增加首次进入系统强制修改用户密码功能。 #3).
• Medium: normalize path comparison ([FEATURE]Ollama中能否新增llama-chinese:7b，小主机运行不起来13b #11), add error handling in the watcher (perf: 优化镜像大小 #15).
• Low: rename unavailable, update outdated comment, clarify watch source shape.

[shaohuzhang1]

Issues / irregularities:

1. Watcher fires on unavailable change but logic depends on route.fullPath indirectly — if the route changes to a different path while token stays the same and no profile exists, it won't retry loading. Acceptable, but note that navigating away from unavailablePath with token present and no profile won't trigger the reload unless unavailable transitions. Consider whether reload should also occur on route change.

2. Race condition / redundant loads — user.loadCurrentUser() isn't awaited or guarded; rapid token changes (or token + unavailable flips) can trigger concurrent requests. Consider a loading flag in the store.

3. NoService renders outside <main> — markup differs (<main> vs bare component). If NoService doesn't provide a root <main>, semantic/screen-reader structure may differ. Minor.

4. English comment mismatch — comments are in Chinese; fine but ensure consistency if English is required.

Optimization suggestions:

• Use computed for the watcher source; already done — good.
• Prefer watchEffect for conciseness, or keep array watcher (current is fine).
• Add await/error handling: void user.loadCurrentUser().catch(...) to avoid unhandled rejection.
• Consider extracting "should load user" into a computed predicate:

  const shouldLoadUser = computed(
    () => !unavailable.value && !!auth.token && !user.chatUserProfile,
  )
  watch(shouldLoadUser, (v) => { if (v) void user.loadCurrentUser() }, { immediate: true })

• Verify unavailablePath uses fullPath semantics consistently (query/hash included); route.path might be safer if query params shouldn't affect availability.
• The :key="auth.token" remount is fine; just confirm remounting on every token refresh is intended (could be expensive).

Summary: Logic is sound; main risks are unhandled promise rejection and missing reload when only the route path leaves the unavailable state without token/profile changes.

[shaohuzhang1]

Code Review Findings

🔴 Potential Issues

1. Returning true allows navigation to a route that may not exist / shouldn't render
Previously, on auth failure the code redirected to a dedicated 404 route. Now it sets auth.unavailablePath and returns true, letting the original route proceed to render. If downstream components assume an authenticated user or valid app context, they may crash or leak internal error states. You rely entirely on some other component watching unavailablePath to intercept rendering — this coupling is implicit and fragile.

• Suggestion: Verify there is a consumer (e.g., an app-level guard/wrapper) that reads unavailablePath and renders an "unavailable" view instead of the normal component tree. Document this contract.
• Consider returning a dedicated /service-unavailable route as a fallback if no such guard exists.

2. Mutating shared state (auth.unavailablePath) inside a navigation guard
Writing to global reactive state during beforeEach can cause race conditions with concurrent navigations. If two navigations happen in-flight, the second may overwrite unavailablePath set by the first.

• Suggestion: Clear unavailablePath at the start of beforeEach (or when navigation resolves successfully), and consider using route meta / query to carry the reason instead of a shared mutable field.

3. to.fullPath may include hash/query that leaks sensitive data
Storing the full path (including tokens in query, e.g. accessToken) into persistent/reactive state could expose credentials in the UI or logs.

• Suggestion: Sanitize before storing — strip query/hash, or only store to.path.

4. Removed getNotFoundRoute — confirm no other callers
The diff removes the helper but there might be other references elsewhere; ensure it's fully cleaned up (no dangling imports/usages).

---

🟡 Minor / Style

5. Inconsistent replace: true handling
In the earlier block you use { ..., replace: true } for redirects, but when returning true here navigation semantics differ. Confirm whether prior history entries should be replaced to avoid "back button" returning to a broken page.

6. Comment says "在原地址展示服务不可用页面" but code just returns true
The behavior described (render unavailable page at original URL) is not implemented in this diff — it depends on external logic. The comment overstates what the patch does; clarify.

---

🟢 Optimization

7. Duplicate auth-failure handling
The same try/catch → set unavailablePath → return true pattern appears twice. Extract a small helper:

function handleAuthFailure(to: RouteLocationNormalized) {
  auth.unavailablePath = sanitize(to)
  return true
}

8. Early unavailablePath reset
Add auth.unavailablePath = undefined near the top of beforeEach so stale values don't persist across successful navigations.

---

Summary

The main risk is that returning true on auth failure lets unauthenticated/failed-context requests reach application routes without an explicit render-intercept guarantee. Verify the consuming guard exists, sanitize stored paths, and reset shared state per-navigation.