Fix composable cleanup leaks in useReviewProposals, useGlobalSearch, usePaperReviewSelectors#1104
Conversation
Ensures the 60s expiry-detection clock is cleared when the composable's effect scope is disposed, preventing leaked intervals.
Clears pending debounce timer and aborts in-flight search request when the composable's effect scope is disposed.
Aborts in-flight deep review API requests when the composable's effect scope is disposed, preventing orphaned network calls.
Verify useReviewProposals registers stopClock via onScopeDispose and that calling the disposal callback clears the clock interval. Add effectScope test for useGlobalSearch confirming debounce timer is cleared on scope disposal.
Review record (carried from #1096)This PR is the rebased continuation of #1096, which was auto-closed when its stacked base branch was deleted. Both independent adversarial reviews live there:
All findings addressed (verified post-rebase):
Verification: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces lifecycle cleanup using onScopeDispose across several composables (useGlobalSearch, usePaperReviewSelectors, and useReviewProposals) to clear timers, abort in-flight requests, and prevent post-disposal state mutations, accompanied by comprehensive unit tests. The review feedback highlights that while these cleanups are beneficial, several asynchronous operations and their corresponding try/catch blocks in useReviewProposals and useGlobalSearch remain unguarded against post-disposal state writes and potential router navigation, and recommends adding isDisposed checks to fully secure these operations.
| } | ||
| } | ||
|
|
||
| onScopeDispose(stopClock) |
There was a problem hiding this comment.
While cleaning up the clock interval is a great improvement, useReviewProposals still lacks guards for its asynchronous operations (loadProposals, openProposalFromHash, and loadBoardOptions).\n\nIf the composable's owning scope is disposed while these requests are in-flight, they will still resolve and mutate reactive state (proposals.value, proposalsLoading.value, etc.). More critically, openProposalFromHash can trigger router navigation (safeReplace), which could unexpectedly redirect the user even after they have navigated away and the component has been unmounted.\n\nWe should introduce an isDisposed flag and use it to guard all post-await state writes and router actions.
let isDisposed = false\n onScopeDispose(() => {\n isDisposed = true\n stopClock()\n })| hasMoreCards.value = false | ||
| } finally { | ||
| loading.value = false | ||
| if (!isDisposed) loading.value = false |
There was a problem hiding this comment.
While guarding the finally block prevents loading.value from being updated after disposal, the reactive state writes inside the try and catch blocks are still unguarded.\n\nIf the network request resolves or rejects (with an error other than AbortError) after the scope is disposed, the callbacks will still execute and mutate boards.value, cards.value, error.value, etc.\n\nTo fully prevent post-disposal mutations, we should check isDisposed immediately after the await call and at the start of the catch block.
| error.value = 'Failed to load more results.' | ||
| } finally { | ||
| loadingMore.value = false | ||
| if (!isDisposed) loadingMore.value = false |
There was a problem hiding this comment.
Similar to executeSearch, the try and catch blocks in loadMore are not guarded against post-disposal mutations. If the request resolves or rejects after the scope is disposed, reactive state like cards.value and error.value will still be mutated.\n\nPlease add if (isDisposed) return guards right after the await call and at the start of the catch block.
There was a problem hiding this comment.
Pull request overview
This PR addresses composable lifecycle cleanup leaks (timers, debounce timeouts, and AbortControllers) by registering disposal handlers via onScopeDispose, reducing the risk of memory/resource leaks when a component/effect scope is torn down.
Changes:
- Add
onScopeDispose(stopClock)and an idempotent guard to prevent leaked intervals inuseReviewProposals. - Add
onScopeDisposecleanup for debounce timers and in-flight requests inuseGlobalSearch, plus new disposal-focused tests. - Add
onScopeDisposeabort + fetch-generation invalidation to prevent post-disposal reactive writes inusePaperReviewSelectors, plus a new disposal test.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/taskdeck-web/src/composables/useReviewProposals.ts | Stops the 60s clock interval automatically on scope disposal and prevents double-start interval leaks. |
| frontend/taskdeck-web/src/composables/useGlobalSearch.ts | Clears debounce timers and aborts in-flight searches on disposal; introduces disposal-guarding logic (needs adjustment for axios cancellation + state writes). |
| frontend/taskdeck-web/src/composables/usePaperReviewSelectors.ts | Aborts in-flight deep review requests and invalidates fetch generation to prevent stale writes after disposal. |
| frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts | Adds tests ensuring onScopeDispose registration and interval idempotency. |
| frontend/taskdeck-web/src/tests/composables/useGlobalSearch.spec.ts | Adds effect-scope disposal tests for debounce cleanup and mid-search disposal behavior (needs to mirror axios cancellation). |
| frontend/taskdeck-web/src/tests/composables/usePaperReviewSelectors.spec.ts | Adds effect-scope disposal test ensuring aborted in-flight results don’t write back. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,4 +1,4 @@ | |||
| import { ref, watch } from 'vue' | |||
| import { onScopeDispose, ref, watch } from 'vue' | |||
| cards.value = [] | ||
| totalCardCount.value = 0 | ||
| hasMoreCards.value = false | ||
| } finally { | ||
| loading.value = false | ||
| if (!isDisposed) loading.value = false |
| error.value = 'Failed to load more results.' | ||
| } finally { | ||
| loadingMore.value = false | ||
| if (!isDisposed) loadingMore.value = false | ||
| } | ||
| } |
| import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' | ||
| import { nextTick } from 'vue' | ||
| import { effectScope, nextTick } from 'vue' |
| // Mirror the real API: reject with AbortError once the signal aborts. | ||
| mockSearch.mockImplementation( | ||
| (_q: string, signal?: AbortSignal) => | ||
| new Promise((_resolve, reject) => { | ||
| signal?.addEventListener('abort', () => | ||
| reject(new DOMException('Aborted', 'AbortError')), | ||
| ) | ||
| }) as ReturnType<typeof searchApi.search>, | ||
| ) |
Resolves #1094. (Re-opened from #1096, which GitHub auto-closed when its stacked base branch
fix/1093was deleted on merge of #1095. Rebased onto main; carries the same commits + both adversarial reviews.)What
Adds
onScopeDisposecleanup to three composables so timers, intervals, and in-flight AbortControllers are released when the owning scope tears down:useReviewProposals— 60s clock interval (with idempotentstartClockguard)useGlobalSearch— debounce timer + AbortController, withfinallywrites guarded against post-disposal mutationusePaperReviewSelectors— AbortController, with fetch-generation invalidation on dispose so abortedPromise.allSettledcontinuations don't write reactive stateReviews
Two independent adversarial reviews were completed on #1096; all findings (3 HIGH, 3 MEDIUM, 1 LOW) addressed. See the consolidated review + fix-evidence comment below.
Verification
vue-tsc -bclean · eslint clean · 63/63 tests across the 3 specs (3 new disposal tests added).