feat: improve error handling and lifecycle management for tabCapture#872
Conversation
🚀 Thank You for Contributing to Late-MeetPlease ensure that:
Thank you for contributing 💙 |
github-actions
Bot
added
gssoc
|
👋 Thank you @vansh2604-star for your contribution to Late-Meet! ✅ Verified: You are assigned to the linked issue #871. 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! |
github-actions
Bot
added
type:code
📝 WalkthroughWalkthroughAudio capture now retries startup failures, persists capture errors, stops on meeting navigation changes, handles inactive offscreen sessions, and displays retryable error banners in the popup and dashboard. Tests cover retry exhaustion and navigation-triggered stopping. ChangesAudio capture reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant User
participant Popup
participant Background
participant TabCapture
participant Offscreen
User->>Popup: Start recording
Popup->>Background: MANUAL_START_AUDIO
Background->>TabCapture: getMediaStreamId
TabCapture-->>Background: Stream ID or failure
Background->>Offscreen: OFFSCREEN_START_CAPTURE
Offscreen-->>Background: Capture result
Background-->>Popup: Updated audio state and captureError
User->>Popup: Retry or close error
Popup->>Background: Retry capture or clear error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
package.jsonOops! Something went wrong! :( ESLint: 10.5.0 TypeError: Error while loading rule 'no-irregular-whitespace': sourceCode.getAllComments is not a function 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/popup.html (1)
1-1: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winError banner close button lacks
aria-labeland banner lacksrole="alert"in both popup and dashboard. Screen readers cannot properly announce the error or interpret the×close button without accessible names.
src/popup.html#L109-115: Addrole="alert"to#error-bannerandaria-label="Dismiss"to#error-close-btn.src/dashboard.html#L153-159: Addrole="alert"to#dash-error-bannerandaria-label="Dismiss"to#dash-error-close-btn.♿ Proposed fix for popup.html
- <div id="error-banner" class="error-banner" style="display: none;"> + <div id="error-banner" class="error-banner" role="alert" style="display: none;"> <span id="error-message" class="error-message">Audio capture failed — please retry.</span> <button id="error-retry-btn" class="error-retry-btn">Retry</button> - <button id="error-close-btn" class="error-close-btn">×</button> + <button id="error-close-btn" class="error-close-btn" aria-label="Dismiss">×</button>♿ Proposed fix for dashboard.html
- <div id="dash-error-banner" class="dash-error-banner" style="display: none;"> + <div id="dash-error-banner" class="dash-error-banner" role="alert" style="display: none;"> <span id="dash-error-message" class="dash-error-message">Audio capture failed — please retry.</span> <button id="dash-error-retry-btn" class="dash-error-retry-btn">Retry</button> - <button id="dash-error-close-btn" class="dash-error-close-btn">×</button> + <button id="dash-error-close-btn" class="dash-error-close-btn" aria-label="Dismiss">×</button>🤖 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.html` at line 1, Update the error banner and close button accessibility attributes in both popup.html and dashboard.html: add role="alert" to `#error-banner` and `#dash-error-banner`, and aria-label="Dismiss" to `#error-close-btn` and `#dash-error-close-btn`. Preserve the existing banner structure and behavior.
🧹 Nitpick comments (3)
src/audioCaptureRetry.test.ts (2)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
Functionwith specific callback types.ESLint flags
Functionas unsafe (@typescript-eslint/no-unsafe-function-type). Define explicit signatures for the tab-updated listener and setTimeout override.♻️ Proposed fix
-type MessageListener = ( +type ChromeMessageListener = ( message: AnyRecord, sender: AnyRecord, sendResponse: (response?: unknown) => void, ) => boolean | undefined; -let tabUpdatedListener: Function | null = null; +type TabUpdatedListener = ( + tabId: number, + changeInfo: { url?: string; status?: string }, + tab: { id?: number; url?: string }, +) => void; + +let tabUpdatedListener: TabUpdatedListener | null = null;And for the setTimeout override:
-const originalSetTimeout = globalThis.setTimeout; -(globalThis as any).setTimeout = (cb: Function, ms?: number) => { - return originalSetTimeout(cb, 0); -}; +const originalSetTimeout = globalThis.setTimeout; +(globalThis as any).setTimeout = (cb: (...args: unknown[]) => void, ms?: number) => { + return originalSetTimeout(cb, 0); +};Also applies to: 20-20, 64-64
🤖 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/audioCaptureRetry.test.ts` at line 16, Replace the broad Function types in the tab-updated listener and setTimeout override declarations with explicit callback signatures matching how each callback is invoked. Update tabUpdatedListener and the related declarations around the affected lines, preserving their existing nullable or override behavior while satisfying `@typescript-eslint/no-unsafe-function-type`.Source: Linters/SAST tools
115-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests share mutable global state without isolation resets.
Test 2 begins by asserting
audioActive === false, which depends on test 1's failure path having calledresetState(). If test 2 is run in isolation (e.g., via--test-name-pattern),state.isActivemay still befalsefrom initial load butcaptureErrorand other fields from test 1 persist. More critically, if test ordering changes or test 1 is modified to succeed, test 2's precondition breaks silently.Consider adding a
beforeEachor a shared reset helper that clearssentMessages, resets mock counters, and sends a state-reset message.♻️ Proposed fix
+function resetTestState() { + getMediaStreamIdCalls = 0; + getMediaStreamIdCallbackValue = null; + lastErrorMock = null; + sentMessages = []; +} + test("audio capture retries 3 times and reports error on failure", async () => { + resetTestState(); getMediaStreamIdCalls = 0; getMediaStreamIdCallbackValue = null; lastErrorMock = { message: "Mocked capture stream permission denied" }; sentMessages = [];test("audio capture stops when active tab navigates away from Google Meet", async () => { + resetTestState(); // Reset states const finalState1 = await sendMessage({ type: "GET_STATE" }); assert.equal(finalState1.audioActive, false);🤖 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/audioCaptureRetry.test.ts` around lines 115 - 178, Isolate the audio capture tests from shared mutable state by adding a beforeEach hook or shared reset helper for the test suite. Reset sentMessages, getMediaStreamIdCalls, getMediaStreamIdCallbackValue, lastErrorMock, and application state via the existing state-reset message before each test, so audio capture stops when active tab navigates away from Google Meet does not depend on the preceding test.src/background.ts (1)
1659-1757: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
isStartingAudiowith try/finally to prevent permanent lock.
isStartingAudiois set totrueat line 1659 but only reset tofalseon explicit success (line 1757) or failure (line 1753) paths. If thecreatedSessionblock (lines 1664–1665) throws — e.g.,chrome.storage.local.remove()rejects —isStartingAudiostaystruepermanently, blocking all future audio starts until the service worker restarts.stopAudioCapturealready uses atry/finallyforisStoppingAudio; the same pattern should be applied here.♻️ Proposed try/finally wrapper
isStartingAudio = true; + try { const createdSession = !state.isActive || !state.meetingId; // ... existing retry loop and failure handling ... // Remove the two manual `isStartingAudio = false;` lines at 1753 and 1757 + } finally { + isStartingAudio = false; + }🤖 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/background.ts` around lines 1659 - 1757, Wrap the entire audio-start flow following the isStartingAudio assignment, including the createdSession initialization and retry logic, in a try/finally block. Move the isStartingAudio = false cleanup into finally so it executes when initialization, capture attempts, success, or failure throws, while preserving the existing success and failure behavior in the try block.
🤖 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/background.ts`:
- Around line 1877-1880: Call hydrateState() at the start of the
navigation-detection handler, before checking tabId against state.targetTabId.
Then retain the existing target-tab and newUrl conditions so navigation
detection uses hydrated state after service-worker restarts.
- Around line 1883-1892: Update the URL-change handling around
getMeetingIdFromUrl and state.meetingId so a current state.meetingId of
"unknown" is replaced with the newly resolved meeting ID without calling
stopAudioCapture. Preserve the existing stop reasons and behavior for genuine
meeting-ID changes, leaving Google Meet, or missing meeting IDs.
- Around line 1684-1700: Update the retry flow surrounding streamId and
getMediaStreamId so each retry obtains a fresh token after the initial attempt.
Do not reuse providedStreamId once it has been consumed; clear or otherwise
bypass it after first use while preserving its use for the initial attempt.
In `@src/dashboard.html`:
- Around line 153-159: The dash-error-banner in dashboard.html needs the same
accessibility attributes as the shared error banner: add role="alert" to the
banner and an aria-label describing the action to the dash-error-close-btn
button. Leave the existing message, retry control, and styling unchanged.
In `@src/dashboard.ts`:
- Around line 368-383: Update the errorCloseBtn click handler to use the Promise
result from chrome.storage.local.get directly instead of passing a callback.
Within the same awaited try flow, clear captureError on the retrieved
activeMeetingState, await chrome.storage.local.set, and preserve the lastState
update and existing error logging.
In `@src/popup.html`:
- Around line 109-115: Add accessibility semantics to the error banner markup:
update the element identified by error-banner with role="alert", and add a
descriptive aria-label to error-close-btn indicating that it closes the error
message. Preserve the existing visual text, styling, and behavior.
In `@src/popup.ts`:
- Around line 403-418: Update the errorCloseBtn click handler to use the
Promise-based chrome.storage.local.get result instead of its callback form,
awaiting the retrieved activeMeetingState and its subsequent
chrome.storage.local.set within the existing try/catch. Keep
lastState.captureError clearing after the storage write completes so storage
failures are handled by the error handler.
---
Outside diff comments:
In `@src/popup.html`:
- Line 1: Update the error banner and close button accessibility attributes in
both popup.html and dashboard.html: add role="alert" to `#error-banner` and
`#dash-error-banner`, and aria-label="Dismiss" to `#error-close-btn` and
`#dash-error-close-btn`. Preserve the existing banner structure and behavior.
---
Nitpick comments:
In `@src/audioCaptureRetry.test.ts`:
- Line 16: Replace the broad Function types in the tab-updated listener and
setTimeout override declarations with explicit callback signatures matching how
each callback is invoked. Update tabUpdatedListener and the related declarations
around the affected lines, preserving their existing nullable or override
behavior while satisfying `@typescript-eslint/no-unsafe-function-type`.
- Around line 115-178: Isolate the audio capture tests from shared mutable state
by adding a beforeEach hook or shared reset helper for the test suite. Reset
sentMessages, getMediaStreamIdCalls, getMediaStreamIdCallbackValue,
lastErrorMock, and application state via the existing state-reset message before
each test, so audio capture stops when active tab navigates away from Google
Meet does not depend on the preceding test.
In `@src/background.ts`:
- Around line 1659-1757: Wrap the entire audio-start flow following the
isStartingAudio assignment, including the createdSession initialization and
retry logic, in a try/finally block. Move the isStartingAudio = false cleanup
into finally so it executes when initialization, capture attempts, success, or
failure throws, while preserving the existing success and failure behavior in
the try block.
🪄 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
Run ID: a0366f64-315f-4217-8b96-b96947558208
📒 Files selected for processing (11)
package.jsonsrc/audioCaptureRetry.test.tssrc/background.tssrc/dashboard.csssrc/dashboard.htmlsrc/dashboard.tssrc/offscreen.tssrc/popup.csssrc/popup.htmlsrc/popup.tssrc/types.ts
| let streamId = providedStreamId; | ||
|
|
||
| if (!streamId) { | ||
| streamId = await new Promise<string | null>((resolve) => { | ||
| chrome.tabCapture.getMediaStreamId({ targetTabId: tabId }, (id) => { | ||
| if (chrome.runtime.lastError) { | ||
| console.error( | ||
| `[LateMeet] getMediaStreamId error (attempt ${attempts + 1}):`, | ||
| chrome.runtime.lastError.message || chrome.runtime.lastError, | ||
| ); | ||
| resolve(null); | ||
| } else { | ||
| resolve(id); | ||
| } | ||
| }); | ||
| }); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding retry logic in the reviewed file.
if [ -f src/background.ts ]; then
sed -n '1650,1725p' src/background.ts
else
echo "src/background.ts not found"
fi
# Find all uses of getMediaStreamId and providedStreamId in the repo.
rg -n "getMediaStreamId|providedStreamId|tabCapture" src . || trueRepository: shouri123/Late-Meet
Length of output: 2026
🌐 Web query:
chrome.tabCapture.getMediaStreamId stream ID expiration retry backoff documentation
💡 Result:
The chrome.tabCapture.getMediaStreamId method generates an opaque string (stream ID) that acts as a short-lived, single-use token for accessing a MediaStream [1][2]. Official documentation specifies that the created stream ID can be used only once and expires after a few seconds if it is not utilized [1][2]. There is no formal documentation for a retry backoff mechanism because the API is designed for immediate consumption [1][2]. If the stream ID has expired or is invalid, attempting to use it with getUserMedia will fail [2]. Developers should ensure the ID is passed to the consumer context (e.g., an offscreen document) and used immediately upon receipt to avoid expiration [3][2]. Key constraints for using the returned stream ID include: - Single-use and short-lived: It must be used promptly after generation [2]. - Security context: If consumerTabId is specified, the ID can be used by a getUserMedia call in any frame in the target tab that shares the same security origin [2][4]. - Same-process usage: If consumerTabId is not specified (beginning in Chrome 116), the ID can be used in any frame with the same security origin in the same render process as the caller, which facilitates passing the ID from a service worker to an offscreen document [2][4].
Citations:
- 1: https://chromium.googlesource.com/chromium/src/+/main/chrome/common/extensions/api/tab_capture.idl
- 2: https://developer.chrome.com/docs/extensions/mv2/reference/tabCapture?hl=id
- 3: https://developer.chrome.com/docs/extensions/how-to/web-platform/screen-capture
- 4: https://developer.chrome.com/docs/extensions/reference/api/tabCapture?hl=id
Avoid reusing the same tabCapture stream ID across retries. chrome.tabCapture.getMediaStreamId() returns a short-lived, single-use token, so once the first attempt fails, later retries can keep hitting an invalid ID. Fetch a fresh ID on each retry (or clear providedStreamId after the first use).
🤖 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/background.ts` around lines 1684 - 1700, Update the retry flow
surrounding streamId and getMediaStreamId so each retry obtains a fresh token
after the initial attempt. Do not reuse providedStreamId once it has been
consumed; clear or otherwise bypass it after first use while preserving its use
for the initial attempt.
| // If the target tab being captured updates its URL or completes loading: | ||
| if (tabId === state.targetTabId) { | ||
| const newUrl = changeInfo.url || (changeInfo.status === "complete" ? tab.url : undefined); | ||
| if (newUrl) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Call hydrateState() before the state.targetTabId check.
hydrateState() is called at line 1881, inside the if (newUrl) block — but only after the tabId === state.targetTabId guard at line 1878. If the service worker recently restarted and state.targetTabId is still null (not yet hydrated), the guard fails and the entire navigation-detection block is skipped. This means meeting-exit navigation could go undetected after a service worker restart until some other code path triggers hydration.
🐛 Proposed fix
- // If the target tab being captured updates its URL or completes loading:
- if (tabId === state.targetTabId) {
- const newUrl = changeInfo.url || (changeInfo.status === "complete" ? tab.url : undefined);
- if (newUrl) {
- await hydrateState();
+ // If the target tab being captured updates its URL or completes loading:
+ await hydrateState();
+ if (tabId === state.targetTabId) {
+ const newUrl = changeInfo.url || (changeInfo.status === "complete" ? tab.url : undefined);
+ if (newUrl) {📝 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.
| // If the target tab being captured updates its URL or completes loading: | |
| if (tabId === state.targetTabId) { | |
| const newUrl = changeInfo.url || (changeInfo.status === "complete" ? tab.url : undefined); | |
| if (newUrl) { | |
| // If the target tab being captured updates its URL or completes loading: | |
| await hydrateState(); | |
| if (tabId === state.targetTabId) { | |
| const newUrl = changeInfo.url || (changeInfo.status === "complete" ? tab.url : undefined); | |
| if (newUrl) { |
🤖 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/background.ts` around lines 1877 - 1880, Call hydrateState() at the start
of the navigation-detection handler, before checking tabId against
state.targetTabId. Then retain the existing target-tab and newUrl conditions so
navigation detection uses hydrated state after service-worker restarts.
| const newMeetingId = getMeetingIdFromUrl(newUrl); | ||
| if (!newMeetingId || newMeetingId !== state.meetingId) { | ||
| let reason = "Left meeting"; | ||
| if (newMeetingId && newMeetingId !== state.meetingId) { | ||
| reason = "Meeting code changed"; | ||
| } else if (!newUrl.includes("meet.google.com")) { | ||
| reason = "Navigated away from Google Meet"; | ||
| } | ||
| await stopAudioCapture(reason); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
"unknown" meeting ID triggers false "Meeting code changed" stop.
When startAudioCapture is called with a null meetingId, state.meetingId is set to "unknown" (line 1668). Later, when the tab URL resolves to a real meeting ID, getMeetingIdFromUrl returns a valid ID that differs from "unknown", so the condition newMeetingId !== state.meetingId at line 1884 is true and capture is stopped with reason "Meeting code changed." The meeting didn't change — the ID was just resolved. Consider treating "unknown" as a special case by updating state.meetingId instead of stopping capture.
🐛 Proposed fix
if (state.isActive) {
const newMeetingId = getMeetingIdFromUrl(newUrl);
- if (!newMeetingId || newMeetingId !== state.meetingId) {
+ if (!newMeetingId) {
let reason = "Left meeting";
- if (newMeetingId && newMeetingId !== state.meetingId) {
- reason = "Meeting code changed";
- } else if (!newUrl.includes("meet.google.com")) {
+ if (!newUrl.includes("meet.google.com")) {
reason = "Navigated away from Google Meet";
}
await stopAudioCapture(reason);
+ } else if (state.meetingId === "unknown") {
+ // Meeting ID was unknown; now resolved — update it
+ state.meetingId = newMeetingId;
+ addTimeline(`Meeting ID resolved (${newMeetingId})`);
+ await broadcastStateUpdate(true);
+ } else if (newMeetingId !== state.meetingId) {
+ await stopAudioCapture("Meeting code changed");
}
}📝 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.
| const newMeetingId = getMeetingIdFromUrl(newUrl); | |
| if (!newMeetingId || newMeetingId !== state.meetingId) { | |
| let reason = "Left meeting"; | |
| if (newMeetingId && newMeetingId !== state.meetingId) { | |
| reason = "Meeting code changed"; | |
| } else if (!newUrl.includes("meet.google.com")) { | |
| reason = "Navigated away from Google Meet"; | |
| } | |
| await stopAudioCapture(reason); | |
| } | |
| const newMeetingId = getMeetingIdFromUrl(newUrl); | |
| if (!newMeetingId) { | |
| let reason = "Left meeting"; | |
| if (!newUrl.includes("meet.google.com")) { | |
| reason = "Navigated away from Google Meet"; | |
| } | |
| await stopAudioCapture(reason); | |
| } else if (state.meetingId === "unknown") { | |
| // Meeting ID was unknown; now resolved — update it | |
| state.meetingId = newMeetingId; | |
| addTimeline(`Meeting ID resolved (${newMeetingId})`); | |
| await broadcastStateUpdate(true); | |
| } else if (newMeetingId !== state.meetingId) { | |
| await stopAudioCapture("Meeting code changed"); | |
| } |
🤖 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/background.ts` around lines 1883 - 1892, Update the URL-change handling
around getMeetingIdFromUrl and state.meetingId so a current state.meetingId of
"unknown" is replaced with the newly resolved meeting ID without calling
stopAudioCapture. Preserve the existing stop reasons and behavior for genuine
meeting-ID changes, leaving Google Meet, or missing meeting IDs.
| <!-- Error Banner --> | ||
| <div id="dash-error-banner" class="dash-error-banner" style="display: none;"> | ||
| <span id="dash-error-message" class="dash-error-message">Audio capture failed — please retry.</span> | ||
| <button id="dash-error-retry-btn" class="dash-error-retry-btn">Retry</button> | ||
| <button id="dash-error-close-btn" class="dash-error-close-btn">×</button> | ||
| </div> | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Same accessibility gaps as popup.html error banner.
Close button lacks aria-label and the banner lacks role="alert". See the consolidated comment for the shared root cause and fix.
🤖 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/dashboard.html` around lines 153 - 159, The dash-error-banner in
dashboard.html needs the same accessibility attributes as the shared error
banner: add role="alert" to the banner and an aria-label describing the action
to the dash-error-close-btn button. Leave the existing message, retry control,
and styling unchanged.
| errorCloseBtn?.addEventListener("click", async () => { | ||
| if (errorBanner) errorBanner.style.display = "none"; | ||
| try { | ||
| await chrome.storage.local.get("activeMeetingState", async (data) => { | ||
| if (data && data.activeMeetingState) { | ||
| data.activeMeetingState.captureError = null; | ||
| await chrome.storage.local.set({ activeMeetingState: data.activeMeetingState }); | ||
| } | ||
| }); | ||
| if (lastState) { | ||
| lastState.captureError = null; | ||
| } | ||
| } catch (err) { | ||
| console.error("[LateMeet] Failed to clear captureError:", err); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '340,410p' src/dashboard.ts && printf '\n--- popup.ts ---\n' && sed -n '1,220p' src/popup.ts && printf '\n--- chrome.storage usages ---\n' && rg -n "chrome\.storage\.local\.get|chrome\.storage\.local\.set|storage\.local\.get" src -nRepository: shouri123/Late-Meet
Length of output: 2026
🌐 Web query:
Chrome extension storage.local.get callback Promise return value await undefined documentation
💡 Result:
In modern Chrome Extensions (Manifest V3), the chrome.storage.local.get method natively returns a Promise that resolves to the requested storage object [1]. Consequently, you do not need to wrap it in a new Promise unless you are working in a specific legacy environment [1][2]. If your code using await chrome.storage.local.get(...) is returning undefined, it is likely due to one of the following two reasons: 1. Providing a Callback Argument: In the Chrome API, if you provide a callback function as an argument to get, the method performs the operation asynchronously via the callback and returns undefined [1][3]. To use await, you must omit the callback argument entirely [2][3]. Incorrect (returns undefined): const result = await chrome.storage.local.get('myKey', (data) => { console.log(data); }); Correct (returns the result object): const result = await chrome.storage.local.get('myKey'); console.log(result.myKey); 2. Returning from Inside the Callback: If you are still using the callback pattern and trying to return a value from within that callback function, that return statement only exits the callback function itself, not the outer function where you are calling it [4][5][6]. If you choose to use the modern Promise-based syntax, simply await the call without a callback, and destructure the returned object to access your specific keys [2][3]. If the key does not exist in storage, the API returns an empty object or the default value provided in the input, not undefined [1][7].
Citations:
- 1: https://developer.chrome.com/docs/extensions/reference/api/storage/StorageArea
- 2: https://stackoverflow.com/questions/77715628/practical-way-to-load-from-chrome-storage-local-get
- 3: https://groups.google.com/a/chromium.org/g/chromium-extensions/c/yTRpjnCu95g
- 4: https://stackoverflow.com/questions/67014546/chrome-storage-returning-undefined-async-await
- 5: https://stackoverflow.com/questions/48861028/callback-returns-undefined-with-chrome-storage-local-get
- 6: https://stackoverflow.com/questions/54645390/chrome-storage-local-get-returns-undefined
- 7: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/StorageArea/get
🌐 Web query:
MDN chrome.storage.local.get callback returns void await
💡 Result:
The chrome.storage.local.get method is asynchronous and supports two distinct usage patterns: the traditional callback pattern and the modern promise-based pattern [1][2][3]. If you pass a callback function as an argument, chrome.storage.local.get does not return a value (it returns void) [3][4]. Instead, the results are passed as an argument to your provided callback function [5][3]. Attempting to assign the result of the function call directly to a variable will result in undefined [3]. In modern Chrome extensions (Manifest V3), the method is natively promisified [2][6]. If you omit the callback argument, chrome.storage.local.get returns a Promise that resolves with the storage results [1][2]. This allows you to use async/await syntax to retrieve data directly [7][6]. Example using async/await: let result = await chrome.storage.local.get(['key']); console.log(result.key); Example using the callback pattern: chrome.storage.local.get(['key'], (result) => { console.log(result.key); }); You cannot mix these two patterns; providing a callback will prevent the method from returning a promise [4].
Citations:
- 1: https://developer.chrome.com/docs/extensions/reference/api/storage
- 2: https://developer.chrome.com/docs/extensions/reference/api/storage/StorageArea
- 3: https://stackoverflow.com/questions/48861028/callback-returns-undefined-with-chrome-storage-local-get
- 4: https://stackoverflow.com/questions/37700051/chrome-extension-is-there-any-way-to-make-chrome-storage-local-get-return-so
- 5: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/StorageArea/get
- 6: https://stackoverflow.com/questions/77715628/practical-way-to-load-from-chrome-storage-local-get
- 7: https://developer.chrome.com/docs/extensions/mv2/reference/storage
Use await with chrome.storage.local.get directly
This mixes the callback and Promise forms of chrome.storage.local.get, so the outer try/catch never sees failures from the storage work in the callback. Await the get call directly, then clear captureError and set the updated state in the same async flow.
🤖 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/dashboard.ts` around lines 368 - 383, Update the errorCloseBtn click
handler to use the Promise result from chrome.storage.local.get directly instead
of passing a callback. Within the same awaited try flow, clear captureError on
the retrieved activeMeetingState, await chrome.storage.local.set, and preserve
the lastState update and existing error logging.
| <!-- Error Banner --> | ||
| <div id="error-banner" class="error-banner" style="display: none;"> | ||
| <span id="error-message" class="error-message">Audio capture failed — please retry.</span> | ||
| <button id="error-retry-btn" class="error-retry-btn">Retry</button> | ||
| <button id="error-close-btn" class="error-close-btn">×</button> | ||
| </div> | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add aria-label to close button and role="alert" to error banner.
The close button uses × without an aria-label, so screen readers may announce it as "multiplication sign" or nothing useful. The error banner also lacks role="alert", so screen readers won't proactively announce errors when the banner appears.
🤖 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.html` around lines 109 - 115, Add accessibility semantics to the
error banner markup: update the element identified by error-banner with
role="alert", and add a descriptive aria-label to error-close-btn indicating
that it closes the error message. Preserve the existing visual text, styling,
and behavior.
| errorCloseBtn?.addEventListener("click", async () => { | ||
| if (errorBanner) errorBanner.style.display = "none"; | ||
| try { | ||
| await chrome.storage.local.get("activeMeetingState", async (data) => { | ||
| if (data && data.activeMeetingState) { | ||
| data.activeMeetingState.captureError = null; | ||
| await chrome.storage.local.set({ activeMeetingState: data.activeMeetingState }); | ||
| } | ||
| }); | ||
| if (lastState) { | ||
| lastState.captureError = null; | ||
| } | ||
| } catch (err) { | ||
| console.error("[LateMeet] Failed to clear captureError:", err); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '360,440p' src/popup.tsRepository: shouri123/Late-Meet
Length of output: 2026
🌐 Web query:
Chrome extensions storage API Promise callback get returns undefined when callback provided documentation
💡 Result:
The chrome.storage API methods, including get, are asynchronous [1][2]. The reason you receive undefined when attempting to return a value from a callback or directly from the get call itself is due to the non-blocking, asynchronous nature of JavaScript [1]. When you call chrome.storage.local.get, the function initiates the request and immediately returns undefined (or a Promise, if using modern syntax), while the actual data retrieval happens in the background [1]. The callback function you provide is only executed once the operation completes [1][2]. To correctly access the data, you must handle it within the asynchronous execution flow [1]. You have two primary approaches: 1. Using Promises (Modern Approach): Since Chrome 95, chrome.storage methods return a Promise that resolves with the storage data [3]. You can use async/await to write cleaner, synchronous-looking code [4]: async function getStorageData(key) { const result = await chrome.storage.local.get([key]); return result[key]; } 2. Using Callbacks (Legacy Approach): If you prefer or must use callbacks, you cannot return the value to the outer function [5][1]. Instead, you must pass the data to another callback function or handle the logic directly inside the callback [1][2]: function getStorageData(key, callback) { chrome.storage.local.get([key], (result) => { callback(result[key]); }); } // Usage getStorageData('myKey', (value) => { console.log(value); }); The documentation does not state that the get method returns undefined when a callback is provided; rather, it correctly indicates that the function is asynchronous [3][6]. The confusion often arises because the method's return value is not the requested data, but rather the result of initiating the asynchronous process [1].
Citations:
- 1: https://stackoverflow.com/questions/18699075/callback-returns-undefined-with-chrome-storage-sync-get
- 2: https://stackoverflow.com/questions/64834513/chrome-extension-chrome-storage-why-does-chrome-storage-sync-get-returns-und
- 3: https://developer.chrome.com/docs/extensions/reference/api/storage/StorageArea
- 4: https://groups.google.com/a/chromium.org/g/chromium-extensions/c/yTRpjnCu95g
- 5: https://stackoverflow.com/questions/54645390/chrome-storage-local-get-returns-undefined
- 6: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/storage/StorageArea/get
🌐 Web query:
Chrome storage local get Promise returned when callback provided undefined docs callback omitted
💡 Result:
In Chrome extensions, the chrome.storage.local.get method supports both a callback-based pattern and a Promise-based pattern, depending on how it is called [1][2]. If you provide a callback function as an argument, the method will execute that callback with the requested data upon completion [2]. In this case, the method itself returns undefined, as the result is handled within the callback [3]. If you omit the callback function, the method returns a Promise that resolves with the requested data or rejects on failure [1][4]. Key behavioral points: - Callback Pattern: chrome.storage.local.get(keys, callback). The return value is undefined. You must access the data inside the callback function [3][2]. - Promise Pattern: chrome.storage.local.get(keys). The return value is a Promise that resolves to the result object [1][2]. - Support: Promise support was introduced in Chrome 95 [1][2]. Before this, or if you still use a callback, you must handle the asynchronous nature of the API within the callback or by wrapping the call in a Promise yourself [3][2]. If you are experiencing unexpected results (e.g., getting undefined when you expected a Promise), ensure you are not passing a callback if you intend to use the Promise, or conversely, ensure you are not expecting a return value from the function call itself when a callback is present [3].
Citations:
- 1: https://developer.chrome.com/docs/extensions/reference/api/storage/StorageArea
- 2: https://developer.chrome.google.cn/docs/extensions/mv2/reference/storage/StorageArea
- 3: https://stackoverflow.com/questions/48861028/callback-returns-undefined-with-chrome-storage-local-get
- 4: https://chromium.googlesource.com/chromium/src/+/master/extensions/common/api/storage.json
Use the Promise form here so the storage update stays inside the try/catch. chrome.storage.local.get(..., callback) returns immediately, so the callback runs outside this block and any rejection from chrome.storage.local.set will skip the error handler. lastState.captureError = null also updates before the storage write completes.
🤖 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 403 - 418, Update the errorCloseBtn click handler
to use the Promise-based chrome.storage.local.get result instead of its callback
form, awaiting the retrieved activeMeetingState and its subsequent
chrome.storage.local.set within the existing try/catch. Keep
lastState.captureError clearing after the storage write completes so storage
failures are handled by the error handler.
|
This PR has been marked as stale due to inactivity. |
1 participant
Description
Introduces robust error handling, transient retry logic with exponential backoff, and strict lifecycle management for
tabCaptureaudio recording in Late-Meet.captureErrorto the sessionStateinterface. Captures initialization/connection failures and broadcasts them to active UI components.meet.google.comor if the meeting code changes."Stop Audio"to"Stop Recording".Fixes #871
Type of Change
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests