-
Notifications
You must be signed in to change notification settings - Fork 490
fix: workspace icon flash and credits showing 0 while workspace is in… #8323
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -12,12 +12,18 @@ | |||||
| :class=" | ||||||
| cn( | ||||||
| 'flex items-center gap-1 rounded-full hover:bg-interface-button-hover-surface justify-center', | ||||||
| compact && 'size-full aspect-square' | ||||||
| compact && 'size-full ' | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Nit: Trailing space in class string.
🧹 Remove trailing space- compact && 'size-full '
+ compact && 'size-full'📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| ) | ||||||
| " | ||||||
| > | ||||||
| <Skeleton | ||||||
| v-if="showWorkspaceSkeleton" | ||||||
| shape="circle" | ||||||
| width="32px" | ||||||
| height="32px" | ||||||
| /> | ||||||
| <WorkspaceProfilePic | ||||||
| v-if="showWorkspaceIcon" | ||||||
| v-else-if="showWorkspaceIcon" | ||||||
| :workspace-name="workspaceName" | ||||||
| :class="compact && 'size-full'" | ||||||
| /> | ||||||
|
|
@@ -40,20 +46,24 @@ | |||||
| } | ||||||
| }" | ||||||
| > | ||||||
| <!-- Workspace mode: workspace-aware popover --> | ||||||
| <!-- Workspace mode: workspace-aware popover (only when ready) --> | ||||||
| <CurrentUserPopoverWorkspace | ||||||
| v-if="teamWorkspacesEnabled" | ||||||
| v-if="teamWorkspacesEnabled && initState === 'ready'" | ||||||
| @close="closePopover" | ||||||
| /> | ||||||
| <!-- Legacy mode: original popover --> | ||||||
| <CurrentUserPopover v-else @close="closePopover" /> | ||||||
| <CurrentUserPopover | ||||||
| v-else-if="!teamWorkspacesEnabled" | ||||||
| @close="closePopover" | ||||||
| /> | ||||||
|
Comment on lines
+49
to
+58
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Empty popover state during workspace loading. When Consider either:
🔧 Option 1: Disable popover toggle during loading <Button
v-if="isLoggedIn"
class="p-1 hover:bg-transparent"
variant="muted-textonly"
:aria-label="$t('g.currentUser')"
- `@click`="popover?.toggle($event)"
+ `@click`="!showWorkspaceSkeleton && popover?.toggle($event)"
>🤖 Prompt for AI Agents |
||||||
| </Popover> | ||||||
| </div> | ||||||
| </template> | ||||||
|
|
||||||
| <script setup lang="ts"> | ||||||
| import { storeToRefs } from 'pinia' | ||||||
| import Popover from 'primevue/popover' | ||||||
| import Skeleton from 'primevue/skeleton' | ||||||
| import { computed, defineAsyncComponent, ref } from 'vue' | ||||||
|
|
||||||
| import UserAvatar from '@/components/common/UserAvatar.vue' | ||||||
|
|
@@ -85,12 +95,20 @@ const photoURL = computed<string | undefined>( | |||||
| () => userPhotoUrl.value ?? undefined | ||||||
| ) | ||||||
|
|
||||||
| const showWorkspaceIcon = computed(() => isCloud && teamWorkspacesEnabled.value) | ||||||
| const { workspaceName: teamWorkspaceName, initState } = storeToRefs( | ||||||
| useTeamWorkspaceStore() | ||||||
| ) | ||||||
|
|
||||||
| const showWorkspaceSkeleton = computed( | ||||||
| () => isCloud && teamWorkspacesEnabled.value && initState.value === 'loading' | ||||||
| ) | ||||||
| const showWorkspaceIcon = computed( | ||||||
| () => isCloud && teamWorkspacesEnabled.value && initState.value === 'ready' | ||||||
| ) | ||||||
|
|
||||||
| const workspaceName = computed(() => { | ||||||
| if (!showWorkspaceIcon.value) return '' | ||||||
| const { workspaceName } = storeToRefs(useTeamWorkspaceStore()) | ||||||
| return workspaceName.value | ||||||
| return teamWorkspaceName.value | ||||||
| }) | ||||||
|
|
||||||
| const popover = ref<InstanceType<typeof Popover> | null>(null) | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Consider using
vi.hoisted()to enable per-test mock state manipulation.The current mocks are static, making it impossible to test the component's behavior with different states (e.g.,
initState: 'ready'vs'idle', orteamWorkspacesEnabled: truevsfalse). Per the coding guidelines, usevi.hoisted()to allow per-test Arrange phase manipulation of mock state.Additionally, the PR objectives mention fixing "flash of wrong workspace icon (replaced with loader)" and "credits showing 0" behavior tied to
initState, but no tests verify this new loading/state-driven behavior.♻️ Suggested refactor to enable per-test state control
Then add tests for the new behavior:
Based on learnings,
vi.hoisted()should be used when necessary to allow per-test Arrange phase manipulation of deeper mock state.🤖 Prompt for AI Agents