fix: add watch cleanup and promise reset in Pinia stores#1415
Conversation
- Store unwatch functions in organization.ts and dialogv2.ts to prevent watcher accumulation - Clean up auth listener subscription on sign-out to prevent memory leaks - Reset _initialLoadPromise after rejection to allow subsequent awaits to succeed - Add proper types with WatchStopHandle and Subscription for cleanup functions Fixes watcher accumulation bugs that caused memory leaks during store lifecycle. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThese changes introduce explicit watcher and subscription cleanup mechanisms across three store files, replacing implicit cleanup with stored handles that are properly disposed during state resets and lifecycle transitions to prevent memory leaks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5b70b486f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Clean up watchers to prevent callbacks on stale state | ||
| _unwatchCurrentOrganization?.() | ||
| _unwatchOrganizations?.() |
There was a problem hiding this comment.
Unsubscribing _unwatchCurrentOrganization and _unwatchOrganizations on SIGNED_OUT stops the only watchers that keep currentOrganization/_organizations in sync, but they are never re-registered afterward (the watch setup runs only once when the store is created). In a SPA session where a user signs out and then signs back in without a full reload, those watchers remain detached, so currentRole, the app/org mapping, and _initialLoadPromise resolution never update. This leaves subsequent sessions with stale org state and hanging awaitInitialLoad() calls. Consider re-establishing the watchers on re-init or avoid stopping them during sign-out.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/stores/dialogv2.ts (1)
1-6: Missingwatchandrefimports from Vue.The file uses
watch(line 77) andref(lines 23-26) but neither is imported. Add them to the import statement.🐛 Proposed fix
-import type { WatchStopHandle } from 'vue' +import type { WatchStopHandle } from 'vue' +import { ref, watch } from 'vue' import { acceptHMRUpdate, defineStore } from 'pinia'
🤖 Fix all issues with AI agents
In @src/stores/main.ts:
- Line 45: The code uses Promise.withResolvers() when initializing
_initialLoadPromise, which is an ES2024 API not available under the current
tsconfig target "es2020"; either update the TS target to "es2024" or replace the
call to Promise.withResolvers() in the _initialLoadPromise initialization with
an equivalent manual resolver pattern (create a new Promise and capture its
resolve/reject into an object) so the code matches the build target;
alternatively ensure a polyfill (e.g., core-js) is configured to provide
Promise.withResolvers before using it.
In @src/stores/organization.ts:
- Around line 40-43: The sign-out handlers call `_unwatchCurrentOrganization`
and `_unwatchOrganizations` then null them, but the watchers are only created
once, so they are not recreated on subsequent sign-ins; fix by extracting the
watcher creation logic into a helper (e.g., `setupWatchers`) that first invokes
and clears any existing `_unwatchCurrentOrganization`/`_unwatchOrganizations`,
then reattaches the `watch` on `currentOrganization` and `_organizations` (copy
the existing watcher bodies), call `setupWatchers()` at store creation and again
inside `fetchOrganizations` when `_initialized` transitions back to true (or
when `!_initialized.value` becomes false) so watchers are re-established after
sign-in; ensure cleanup code only nullifies after invoking the stop handles.
- Around line 1-2: The import order violates perfectionist/sort-imports: move
the Subscription import from "@supabase/supabase-js" so it appears before the
Vue imports (ComputedRef, Ref, WatchStopHandle) — update the top-level import
block in src/stores/organization.ts to list the "@supabase/supabase-js" import
before the "vue" import so ESLint no longer flags the ordering error.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/stores/dialogv2.tssrc/stores/main.tssrc/stores/organization.ts
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.{ts,tsx,vue,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
~/alias for imports fromsrc/directory in frontend TypeScript and Vue components
Files:
src/stores/dialogv2.tssrc/stores/main.tssrc/stores/organization.ts
src/**/*.{vue,ts,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Frontend ESLint must pass before commit; run
bun lint:fixto auto-fix issues in frontend files
Files:
src/stores/dialogv2.tssrc/stores/main.tssrc/stores/organization.ts
**/*.{vue,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Run
bun lintto lint Vue, TypeScript, and JavaScript files; usebun lint:fixto auto-fix issues
Files:
src/stores/dialogv2.tssrc/stores/main.tssrc/stores/organization.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use single quotes and no semicolons per @antfu/eslint-config rules
Files:
src/stores/dialogv2.tssrc/stores/main.tssrc/stores/organization.ts
src/**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
Use path alias
~/to map tosrc/for imports in TypeScript and JavaScript files
Files:
src/stores/dialogv2.tssrc/stores/main.tssrc/stores/organization.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (AGENTS.md)
Run
bun lintor lint/format command before validating any backend or frontend task to ensure consistent formatting
Files:
src/stores/dialogv2.tssrc/stores/main.tssrc/stores/organization.ts
🧠 Learnings (1)
📚 Learning: 2026-01-10T04:55:25.248Z
Learnt from: CR
Repo: Cap-go/capgo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T04:55:25.248Z
Learning: Applies to src/**/*.vue : Use Pinia stores for state management in Vue components
Applied to files:
src/stores/dialogv2.tssrc/stores/organization.ts
🪛 GitHub Actions: Run tests
src/stores/organization.ts
[error] 2-2: ESLint: perfectionist/sort-imports violated. Expected "@supabase/supabase-js" to come before "vue". (bun lint: eslint) - lint script exited with code 1.
🔇 Additional comments (4)
src/stores/main.ts (1)
116-121: Good fix for promise reset on rejection.Resetting
_initialLoadPromiseafter rejection correctly allows subsequentawaitInitialLoad()calls to succeed on retry. The error is still re-thrown for upstream handling.src/stores/dialogv2.ts (1)
65-85: Well-structured dismiss watcher cleanup.The implementation correctly:
- Cleans up any existing watcher before creating a new one (line 68)
- Handles the already-closed case with an early return (lines 71-75)
- Nullifies the handle after cleanup to prevent stale references (line 80)
This prevents watcher accumulation when
onDialogDismissis called multiple times.src/stores/organization.ts (2)
246-266: Auth subscription cleanup is correctly managed.The sign-out handler properly:
- Unsubscribes from auth state changes (lines 252-253)
- Resets all state including
_initialLoadPromise(line 260)- Resets
_initializedto allow re-initialization (line 263)The existing subscription is also cleaned before creating a new one (line 248).
37-37: Consistent use ofPromise.withResolvers<boolean>()across stores.This aligns with the same pattern in
main.ts, ensuring both stores handle promise-based initialization consistently.
| const totalStorage = ref<number>(0) | ||
| const dashboardFetched = ref<boolean>(false) | ||
| const _initialLoadPromise = ref(Promise.withResolvers()) | ||
| const _initialLoadPromise = ref(Promise.withResolvers<boolean>()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the actual file and its context
cat -n src/stores/main.ts | head -60Repository: Cap-go/capgo
Length of output: 2447
🏁 Script executed:
# Check for tsconfig or build configuration
fd -e json | grep -E '(tsconfig|package\.json|vite|webpack|esbuild|tsup)' | head -20Repository: Cap-go/capgo
Length of output: 104
🏁 Script executed:
# Check if Promise.withResolvers is used elsewhere in the codebase
rg "Promise\.withResolvers" --type ts --type js --type vue -A 2Repository: Cap-go/capgo
Length of output: 83
🌐 Web query:
Promise.withResolvers browser support Node.js compatibility ES2024
💡 Result:
Summary — what Promise.withResolvers is, ES status, and runtime support:
- What it does: static Promise.withResolvers() returns { promise, resolve, reject } (convenience for creating externally-resolvable promises). [1]
- ECMAScript / spec: added to the language feature set (listed in ECMAScript spec; treated as a new baseline feature from 2024 onward). [1][2]
- Browser support: available in modern browsers (Chrome 119+, Edge 119+, Firefox 121+, Safari 17.4+). [2]
- Node.js support: available natively starting in Node.js v22 (older Node versions do not include it). Use a polyfill or upgrade Node if you need it on earlier runtimes. [2][3]
Sources: MDN documentation on Promise.withResolvers [1]; Web Platform / feature status (browser matrix and baseline date) [2]; Node/compatibility reports and community notes showing Node v22+ requirement and common polyfill/upgrade remedies [3].
🏁 Script executed:
# Check tsconfig.json for target
cat tsconfig.jsonRepository: Cap-go/capgo
Length of output: 1257
🏁 Script executed:
# Check package.json for Node version requirement
jq '.engines, .devDependencies, .dependencies' package.json | head -50Repository: Cap-go/capgo
Length of output: 1590
🏁 Script executed:
# Search for polyfills or Promise.withResolvers elsewhere
rg "Promise\.withResolvers|polyfill|Promise" --type ts --type js -l | head -20Repository: Cap-go/capgo
Length of output: 674
🏁 Script executed:
# Check if there's any build configuration that might transpile/polyfill
rg "withResolvers" --type ts --type js -A 3 -B 3Repository: Cap-go/capgo
Length of output: 1824
🏁 Script executed:
# Check vite config
cat vite.config.mts 2>/dev/null | head -100Repository: Cap-go/capgo
Length of output: 3025
🏁 Script executed:
# Search for polyfill setup, core-js, or Promise shims
rg "polyfill|core-js|Promise" package.jsonRepository: Cap-go/capgo
Length of output: 69
🏁 Script executed:
# Check for browserlist or runtime config
fd -e browserslistrc -e "\.browserslistrc"Repository: Cap-go/capgo
Length of output: 38
🏁 Script executed:
# Check for any Promise.withResolvers polyfill/shim imports
rg "Promise\.withResolvers|withResolvers.*polyfill" --type ts --type jsRepository: Cap-go/capgo
Length of output: 421
Update TypeScript target or replace Promise.withResolvers to match es2020 target.
Promise.withResolvers() is an ES2024 feature, but your tsconfig.json targets es2020. This creates a mismatch—modern browsers (Chrome 119+, Firefox 121+, Safari 17.4+) support it natively, but the code violates your build target constraint. Either upgrade target to "es2024" in tsconfig.json, or replace this with the equivalent pattern: Promise.resolve().then(resolve => ({ promise: ..., resolve, reject: ... })), or ensure core-js is configured to polyfill this API.
🤖 Prompt for AI Agents
In @src/stores/main.ts at line 45, The code uses Promise.withResolvers() when
initializing _initialLoadPromise, which is an ES2024 API not available under the
current tsconfig target "es2020"; either update the TS target to "es2024" or
replace the call to Promise.withResolvers() in the _initialLoadPromise
initialization with an equivalent manual resolver pattern (create a new Promise
and capture its resolve/reject into an object) so the code matches the build
target; alternatively ensure a polyfill (e.g., core-js) is configured to provide
Promise.withResolvers before using it.
| import type { ComputedRef, Ref, WatchStopHandle } from 'vue' | ||
| import type { Subscription } from '@supabase/supabase-js' |
There was a problem hiding this comment.
Fix import order to resolve lint failure.
ESLint: perfectionist/sort-imports violated. Expected "@supabase/supabase-js" to come before "vue".
🔧 Proposed fix
-import type { ComputedRef, Ref, WatchStopHandle } from 'vue'
-import type { Subscription } from '@supabase/supabase-js'
+import type { Subscription } from '@supabase/supabase-js'
+import type { ComputedRef, Ref, WatchStopHandle } from 'vue'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import type { ComputedRef, Ref, WatchStopHandle } from 'vue' | |
| import type { Subscription } from '@supabase/supabase-js' | |
| import type { Subscription } from '@supabase/supabase-js' | |
| import type { ComputedRef, Ref, WatchStopHandle } from 'vue' |
🧰 Tools
🪛 GitHub Actions: Run tests
[error] 2-2: ESLint: perfectionist/sort-imports violated. Expected "@supabase/supabase-js" to come before "vue". (bun lint: eslint) - lint script exited with code 1.
🤖 Prompt for AI Agents
In @src/stores/organization.ts around lines 1 - 2, The import order violates
perfectionist/sort-imports: move the Subscription import from
"@supabase/supabase-js" so it appears before the Vue imports (ComputedRef, Ref,
WatchStopHandle) — update the top-level import block in
src/stores/organization.ts to list the "@supabase/supabase-js" import before the
"vue" import so ESLint no longer flags the ordering error.
| // Store cleanup functions | ||
| let _unwatchCurrentOrganization: WatchStopHandle | null = null | ||
| let _unwatchOrganizations: WatchStopHandle | null = null | ||
| let _authSubscription: Subscription | null = null |
There was a problem hiding this comment.
Watchers are stopped on sign-out but never recreated on subsequent sign-in.
When SIGNED_OUT fires, _unwatchCurrentOrganization and _unwatchOrganizations are invoked (lines 255-256), stopping the watchers. However, the watchers are only created once at store definition time (lines 84, 133). After a user signs out and signs back in, the watchers won't exist, breaking organization change detection and app-to-org mapping.
Consider recreating the watchers in fetchOrganizations when _initialized becomes true again, or guard the cleanup to only nullify after invoking without permanently losing the watch.
💡 Suggested approach
Extract watcher setup into a helper function and call it when re-initializing:
const setupWatchers = () => {
// Clean up existing watchers first
_unwatchCurrentOrganization?.()
_unwatchOrganizations?.()
_unwatchCurrentOrganization = watch(currentOrganization, async (currentOrganizationRaw, oldOrganization) => {
// ... existing watcher body
})
_unwatchOrganizations = watch(_organizations, async (organizationsMap) => {
// ... existing watcher body
})
}Then call setupWatchers() at store creation and again in fetchOrganizations when !_initialized.value.
Also applies to: 83-84, 132-133, 254-256
🤖 Prompt for AI Agents
In @src/stores/organization.ts around lines 40 - 43, The sign-out handlers call
`_unwatchCurrentOrganization` and `_unwatchOrganizations` then null them, but
the watchers are only created once, so they are not recreated on subsequent
sign-ins; fix by extracting the watcher creation logic into a helper (e.g.,
`setupWatchers`) that first invokes and clears any existing
`_unwatchCurrentOrganization`/`_unwatchOrganizations`, then reattaches the
`watch` on `currentOrganization` and `_organizations` (copy the existing watcher
bodies), call `setupWatchers()` at store creation and again inside
`fetchOrganizations` when `_initialized` transitions back to true (or when
`!_initialized.value` becomes false) so watchers are re-established after
sign-in; ensure cleanup code only nullifies after invoking the stop handles.
|
1 participant
Summary (AI generated)
Motivation (AI generated)
Pinia stores had multiple watch cleanup issues causing memory leaks and watcher accumulation. Watch functions without cleanup accumulate over time, especially on re-initialization. Auth listener subscriptions weren't being stored and cleaned up properly. Promise resolvers that reject become permanently failed, preventing retry logic from working.
Business Impact (AI generated)
Fixes memory leaks in the application that could cause performance degradation and increased memory usage over time. Ensures proper cleanup on authentication state changes and store re-initialization. Enables robust retry logic for failed dashboard and organization data loads.
Test Plan (AI generated)
bun typecheckto verify TypeScript typesbun lintto verify code styleGenerated with AI
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.