Conversation
|
Preview (prod) → https://195-prod.rucq-ui-preview.trapti.tech/ |
There was a problem hiding this comment.
Pull request overview
This PR migrates user icon caching from TanStack Query to Service Worker (SW) cache, significantly simplifying the UserIcon.vue component while improving caching strategy. The change moves icon caching closer to the network layer, allowing the browser's native caching mechanisms to handle user icons more efficiently.
Changes:
- Added Service Worker runtime caching configuration for user icon API endpoints with CacheFirst strategy
- Simplified
UserIcon.vueby removing TanStack Query dependency and implementing native image load state management - Removed unused
iconKeysfrom query keys file
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| vite.config.ts | Adds Service Worker runtime caching configuration for user icons with 1-week expiration and 200 entry limit |
| src/components/generic/UserIcon.vue | Refactors component to use native image loading with SW cache instead of TanStack Query; simplifies template structure |
| src/api/queries/keys.ts | Removes iconKeys object that is no longer needed after migration to SW cache |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
vite.config.ts
Outdated
| maxAgeSeconds: 7 * 24 * 60 * 60, // 1 週間 | ||
| }, | ||
| cacheableResponse: { | ||
| statuses: [0, 200], |
There was a problem hiding this comment.
The cacheableResponse includes status 0, which is typically for opaque responses from cross-origin requests. However, the URL pattern matches https://q.trap.jp/api/v3/public/icon/*, which appears to be a same-origin or CORS-enabled API endpoint. If the API endpoint properly returns CORS headers and status 200 responses, including status 0 may be unnecessary and could cache failed opaque responses.
Verify whether status 0 is actually needed for this endpoint, or if only status 200 should be cached.
| statuses: [0, 200], | |
| statuses: [200], |
src/components/generic/UserIcon.vue
Outdated
| const isLoading = ref(true) | ||
| const hasError = ref(false) |
There was a problem hiding this comment.
The loading and error states are not reset when the userId prop changes. When the id prop changes (e.g., when rendering a different user's icon), the component will continue to show the skeleton loader or error state from the previous user until the new image loads or fails. This can cause visual inconsistencies where an old skeleton or error state is displayed while the new image is loading.
Add a watch effect to reset these states when userId changes.
| <v-tooltip | ||
| :activator="iconRef" | ||
| :open-on-hover="idTooltip" | ||
| :open-on-click="idTooltip" | ||
| :open-delay="1000" | ||
| location="top" | ||
| > | ||
| <span class="text-white font-weight-medium">{{ tooltipText }}</span> | ||
| <span class="text-white font-weight-medium">{{ `@${userId}` }}</span> | ||
| </v-tooltip> |
There was a problem hiding this comment.
The v-tooltip is positioned outside the v-avatar but references it via activator. This creates a DOM structure where the tooltip's visual container (div.rounded-circle) wraps both the avatar and the tooltip element. However, with the current structure, the tooltip will always be rendered in the DOM even when idTooltip is false, which could have performance implications in lists with many UserIcon components.
Consider moving the v-tooltip inside the v-avatar or conditionally rendering it only when idTooltip is true.
src/components/generic/UserIcon.vue
Outdated
| v-show="!showSkeleton" | ||
| class="w-100 h-100" | ||
| tabindex="0" | ||
| :src="iconUrl" |
There was a problem hiding this comment.
The img element no longer has the loading="lazy" attribute that was present in the original implementation. Without this attribute, all user icons will be loaded immediately when the page renders, which could impact page load performance when many user icons are displayed (e.g., in the room info view or answer lists). The Service Worker cache will help with repeated loads, but the initial page load could be slower.
Consider adding loading="lazy" back to the img element to preserve lazy loading behavior.
| :src="iconUrl" | |
| :src="iconUrl" | |
| loading="lazy" |
src/components/generic/UserIcon.vue
Outdated
| <div class="rounded-circle overflow-hidden"> | ||
| <v-avatar ref="iconRef" :size="size" class="d-block" :style="cursorStyle"> | ||
| <v-skeleton-loader v-if="showSkeleton" type="image" class="w-100 h-100" /> | ||
| <img |
There was a problem hiding this comment.
The removal of v-bind="attrs" means that any attributes passed to the UserIcon component (like class, style, or data attributes) will no longer be forwarded to the appropriate element. While the current usages in the codebase mainly pass standard props, this change could break existing or future usages that rely on attribute inheritance.
If attribute forwarding is not needed, consider adding inheritAttrs: false to the script setup to make this explicit. Otherwise, consider applying v-bind="$attrs" to an appropriate element.
|
image-proxy.trap.jp にはちょっとした不具合がありそうなので traQ の API に戻します |
📝 WalkthroughWalkthroughユーザーアイコンの取得をVue Query依存からクライアント側のロード状態管理へ移行し、関連するクエリキー（ Changes
Sequence Diagram(s)sequenceDiagram
participant Client as UserIcon Component
participant Browser as Browser (img)
participant SW as ServiceWorker (Workbox)
participant API as Backend API
Client->>Browser: set <img src=iconUrl>
Browser->>SW: fetch iconUrl (request)
alt cached
SW->>SW: read from 'user-icons' cache
SW-->>Browser: return cached image
else not cached
SW->>API: network fetch icon
API-->>SW: image response
SW->>SW: put response into 'user-icons' cache
SW-->>Browser: return network image
end
Browser-->>Client: emit load / error
alt load
Client->>Client: isLoading=false, showSkeleton=false, hasError=false
else error
Client->>Client: isLoading=false, hasError=true, showSkeleton=true
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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: 1
🤖 Fix all issues with AI agents
In `@src/components/generic/UserIcon.vue`:
- Around line 2-29: The component currently never clears hasError and never
resets loading state when iconUrl changes; update handleLoad to also set
hasError.value = false so a successful load clears the error, and add a watcher
on iconUrl (the computed used to build the image URL) to reset isLoading.value =
true and hasError.value = false whenever iconUrl changes (so switching IDs or
retries will re-run loading). Reference: hasError, isLoading, iconUrl,
handleLoad, handleError.
🧹 Nitpick comments (2)
vite.config.ts (1)
83-98: CacheFirst だとアイコン更新が最大1週間反映されませんCacheFirst はキャッシュ命中時にネットワークへ行かないため、ユーザーがアイコンを変更しても
maxAgeSecondsの間は古い画像が残ります。更新の即時反映を重視するなら StaleWhileRevalidate などで裏側更新を行う方が安全です。♻️ 変更案
- handler: 'CacheFirst', + handler: 'StaleWhileRevalidate',src/components/generic/UserIcon.vue (1)
37-43:imgがフォーカス可能なのに説明がなく、アクセシビリティ上不利です非インタラクティブなら
tabindexは外し、意味のある画像であればaltを付けてください。♿ 修正案
<img v-show="!showSkeleton" class="w-100 h-100" - tabindex="0" :src="iconUrl" + :alt="userId ? `@${userId}` : ''" `@load`="handleLoad" `@error`="handleError" />
src/components/generic/UserIcon.vue
Outdated
| import { computed, ref } from 'vue' | ||
| import { useUserStore } from '@/store' | ||
|
|
||
| const props = defineProps<{ id?: string; size: number; idTooltip?: boolean }>() | ||
| // idTooltip ... クリック時に Tooltip で ID を表示するかどうか | ||
| const userId = props.id ?? useUserStore().user.id // id が指定されていない場合は自分のアイコンを表示 | ||
| const props = defineProps<{ | ||
| id?: string | ||
| size: number | ||
| /** クリック時に Tooltip で ID を表示 */ | ||
| idTooltip?: boolean | ||
| }>() | ||
|
|
||
| const attrs = useAttrs() | ||
| const iconRef = ref<HTMLElement | undefined>() | ||
|
|
||
| const imageStyle = { | ||
| width: `${props.size}px`, | ||
| height: `${props.size}px`, | ||
| objectFit: 'contain' as const, | ||
| borderRadius: '50%', | ||
| display: 'block', | ||
| cursor: props.idTooltip ? 'pointer' : 'default', | ||
| } | ||
|
|
||
| const directUrl = `https://q.trap.jp/api/v3/public/icon/${userId}` | ||
| const userId = computed(() => props.id ?? useUserStore().user.id) | ||
| const iconUrl = computed(() => `https://q.trap.jp/api/v3/public/icon/${userId.value}`) | ||
| const cursorStyle = computed(() => ({ cursor: props.idTooltip ? 'pointer' : 'default' })) | ||
|
|
||
| const { | ||
| data: cachedIconUrl, | ||
| isLoading, | ||
| isFetching, | ||
| isError, | ||
| } = useQuery<string, Error>({ | ||
| queryKey: qk.icons.user(userId), | ||
| staleTime: 24 * 60 * 60_000, // 24h | ||
| gcTime: 24 * 60 * 60_000, // 24h | ||
| retry: 0, | ||
| // データURLで保存する | ||
| queryFn: async () => { | ||
| const res = await fetch(directUrl) | ||
| if (!res.ok) throw new Error(`アイコンを取得できませんでした: ${res.status}`) | ||
| // 画像のロード状態を管理 | ||
| const isLoading = ref(true) | ||
| const hasError = ref(false) | ||
| const showSkeleton = computed(() => isLoading.value || hasError.value) | ||
|
|
||
| const blob = await res.blob() | ||
| const dataUrl = await new Promise<string>((resolve, reject) => { | ||
| const reader = new FileReader() | ||
| reader.onloadend = () => resolve(reader.result as string) | ||
| reader.onerror = () => reject(new Error('アイコンの変換に失敗しました')) | ||
| reader.readAsDataURL(blob) | ||
| }) | ||
| return dataUrl | ||
| }, | ||
| }) | ||
|
|
||
| const showSkeleton = computed( | ||
| () => !cachedIconUrl.value || isLoading.value || isFetching.value || isError.value, | ||
| ) | ||
| const handleLoad = () => { | ||
| isLoading.value = false | ||
| } | ||
|
|
||
| const tooltipText = `@${userId}` | ||
| const handleError = () => { | ||
| isLoading.value = false | ||
| hasError.value = true |
There was a problem hiding this comment.
エラー後/ID変更後にスケルトンが固定される可能性があります
hasError を解除する経路がなく、iconUrl が変わっても isLoading/hasError がリセットされないため、一時的な失敗やID切替後にアイコンが表示されなくなります。iconUrl 変更時に状態をリセットし、成功時に hasError をクリアしてください。
🛠️ 修正案
-import { computed, ref } from 'vue'
+import { computed, ref, watch } from 'vue'
@@
-const iconUrl = computed(() => `https://q.trap.jp/api/v3/public/icon/${userId.value}`)
+const iconUrl = computed(() =>
+ userId.value ? `https://q.trap.jp/api/v3/public/icon/${userId.value}` : ''
+)
@@
const showSkeleton = computed(() => isLoading.value || hasError.value)
+
+watch(iconUrl, () => {
+ isLoading.value = true
+ hasError.value = false
+})
@@
const handleLoad = () => {
isLoading.value = false
+ hasError.value = false
}🤖 Prompt for AI Agents
In `@src/components/generic/UserIcon.vue` around lines 2 - 29, The component
currently never clears hasError and never resets loading state when iconUrl
changes; update handleLoad to also set hasError.value = false so a successful
load clears the error, and add a watcher on iconUrl (the computed used to build
the image URL) to reset isLoading.value = true and hasError.value = false
whenever iconUrl changes (so switching IDs or retries will re-run loading).
Reference: hasError, isLoading, iconUrl, handleLoad, handleError.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/generic/UserIcon.vue`:
- Around line 43-50: The <img> in UserIcon.vue is missing an alt attribute;
update the component to accept or compute an accessible alt text and bind it to
the image (e.g., add an alt prop or computed like altText) so the <img> using
iconUrl and v-show="!showSkeleton" includes :alt="altText" (or a sensible
default such as 'User avatar'); ensure decorative images can be marked with an
empty alt ("") and keep existing handlers (handleLoad, handleError) unchanged.
mumumu6
left a comment
mumumu6
left a comment
There was a problem hiding this comment.
タブの遷移(infoやeventなど)から戻るとcache storageに非同期でswが取りに行っているようでローディングが挟まるのが気になります。
tanstackqueryだとメモリに展開されているのでこれがないです。
また、v-imgの最適化で画面に表示されたときにfetchするようになっていますが、スクロールするたびにloadingが見えてこれも少し気になりませんか？
ユーザーアイコンのキャッシュ管理を TanStack Query → SW に移動
UserIcon.vue のバグ修正とリファクタリングを伴う
Summary by CodeRabbit