fix: resolved signal re-use on retry and storage null-set leak - #909
fix: resolved signal re-use on retry and storage null-set leak#909saurabhhhcodes wants to merge 1 commit into
Conversation
…e instead of set(null)
🚀 Thank You for Contributing to Late-MeetPlease ensure that:
Thank you for contributing 💙 |
github-actions
Bot
added
type:code
|
👋 Thank you @saurabhhhcodes for your contribution to Late-Meet!
Please review any automated suggestions or code review comments that may appear below! We will review your PR as soon as possible! Please consider starring the repository ⭐ to show your support! |
📝 WalkthroughWalkthroughThe changes harden popup DOM access, delete pending meeting sessions from Chrome storage, and exclude request abort signals from recursive fetch retries. ChangesPopup defensive handling
Pending session cleanup
Retry signal handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/popup.ts`:
- Around line 144-146: Update maybeStartPopupTour() to return immediately when
mainView is null before accessing mainView.style.display or starting the tour.
Keep the existing guarded setupView/mainView updates and only invoke the tour
logic when mainView exists.
In `@src/utils/api.ts`:
- Around line 49-50: Update the retry logic in fetchWithRetry so an active
options.signal is preserved for retries triggered by 429/5xx or transient
network errors, while an already-aborted signal is stripped. For the
aborted-signal retry path, create and pass a bounded fresh timeout signal so
validateOpenAIKey and validateElevenLabsKey remain time-limited.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 400f56dd-bc92-459f-b0db-34ac4dbf937b
📒 Files selected for processing (3)
src/popup.tssrc/sessionStorage.tssrc/utils/api.ts
| if (setupView) setupView.style.display = "none"; | ||
| if (mainView) mainView.style.display = "block"; | ||
| void maybeStartPopupTour(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard mainView inside maybeStartPopupTour() as well.
These guards still lead to maybeStartPopupTour(), which unconditionally evaluates mainView.style.display at Line 170. If mainView is missing, the save path still throws after the guarded updates. Return early when mainView is null before starting the tour.
Proposed fix
- if (stored[POPUP_ONBOARDING_TOUR_KEY] || mainView.style.display === "none") return;
+ if (
+ stored[POPUP_ONBOARDING_TOUR_KEY] ||
+ !mainView ||
+ mainView.style.display === "none"
+ ) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/popup.ts` around lines 144 - 146, Update maybeStartPopupTour() to return
immediately when mainView is null before accessing mainView.style.display or
starting the tour. Keep the existing guarded setupView/mainView updates and only
invoke the tour logic when mainView exists.
| const { signal: _signal, ...restOptions } = options; | ||
| return fetchWithRetry(url, restOptions, retries - 1, backoff * 2); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Preserve cancellation and timeouts for non-aborted retries.
This unconditionally removes options.signal, so retries triggered by 429/5xx or transient network errors no longer honor the caller’s active abort signal. In validateOpenAIKey and validateElevenLabsKey, the 5-second timeout therefore only bounds the first attempt; later retries can continue beyond it and may hang without any timeout. Strip the signal only when it is already aborted, and provide a bounded fresh timeout for that retry path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/api.ts` around lines 49 - 50, Update the retry logic in
fetchWithRetry so an active options.signal is preserved for retries triggered by
429/5xx or transient network errors, while an already-aborted signal is
stripped. For the aborted-signal retry path, create and pass a bounded fresh
timeout signal so validateOpenAIKey and validateElevenLabsKey remain
time-limited.
1 participant
Bug Fixes
1. fetchWithRetry reuses aborted AbortController signal on retries (
src/utils/api.ts)When a request is aborted (e.g. timeout via AbortController),
fetchWithRetrypasses the sameoptionsobject — including the already-abortedsignal— to each retry attempt. This causes all subsequent fetch calls to immediately reject with anAbortError, rendering the retry mechanism completely ineffective.Fix: Destructure and omit
signalfrom options on retry so each attempt gets a fresh signal-less request.2.
discardPendingMeetingSessionandpersistPendingMeetingSessionleak storage (src/sessionStorage.ts)Both functions use
storage.set({ [PENDING_SESSION_KEY]: null })which stores anullvalue in Chrome storage instead of removing the key. This leaves phantom null entries that waste storage quota and can interfere with state checks.Fix: Use
storage.remove(PENDING_SESSION_KEY)to properly clean up.Summary by CodeRabbit