fix: ensure transcript summaries flush after silence#715
Open
Shreya-nipunge wants to merge 2 commits into
Open
fix: ensure transcript summaries flush after silence#715Shreya-nipunge wants to merge 2 commits into
Shreya-nipunge wants to merge 2 commits into
Conversation
Contributor
🚀 Thank You for Contributing to Late-MeetPlease ensure that:
Thank you for contributing 💙 |
github-actions
Bot
added
gssoc
Contributor
|
👋 Thank you @Shreya-nipunge for your contribution to Late-Meet! ✅ Verified: You are assigned to the linked issue #714. 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! |
Contributor
📝 WalkthroughWalkthroughThis PR fixes a bug where transcript summarization was never retriggered after the interval guard early-returned during silence. The changes implement an alarm-based retry mechanism in ChangesSummarization Retry and Shutdown Flush
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
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/background.ts (1)
2303-2312:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPipeline failure: Test mock missing
chrome.runtime.onSuspend.All CI test runs fail because the test harness in
src/background.summary.test.tsdoes not mockchrome.runtime.onSuspend. When this module loads, line 2303 throwsTypeError: Cannot read properties of undefined (reading 'addListener').Add the missing mock in the test file's Chrome runtime mock object:
// In src/background.summary.test.ts, inside the chrome.runtime mock: onSuspend: { addListener: () => {} },🤖 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 2303 - 2312, The test harness is missing a mock for chrome.runtime.onSuspend which causes chrome.runtime.onSuspend.addListener to be undefined when src/background.ts loads; update the chrome.runtime mock in src/background.summary.test.ts to include an onSuspend object with a no-op addListener function so that calls to chrome.runtime.onSuspend.addListener succeed during tests (target the chrome.runtime mock where other runtime properties are defined).Source: Pipeline failures
🧹 Nitpick comments (1)
src/background.summary.test.ts (1)
123-127: ⚡ Quick win
sendMessage()does not await async listener work, making timing assertions flaky.Line 123-Line 127 dispatches listeners synchronously and ignores returned promises, so assertions depend on arbitrary sleeps (Line 149, Line 159, Line 174). Await listener completion to reduce nondeterminism in CI.
Suggested refactor
async function sendMessage(msg: any) { - for (const listener of onMessageListeners) { - listener(msg, {}, () => {}); - } + for (const listener of onMessageListeners) { + await Promise.resolve(listener(msg, {}, () => {})); + } }Also applies to: 149-159, 174-175
🤖 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.summary.test.ts` around lines 123 - 127, The sendMessage helper dispatches onMessageListeners synchronously and ignores returned promises, causing flaky timing; modify sendMessage to collect each listener's return value (call listener(msg, {}, () => {})) and await Promise.all on those results (use Promise.resolve for non-promise returns or Promise.allSettled if you want to continue despite individual failures) so tests relying on asynchronous listener work no longer need arbitrary sleeps; update any callers/tests that assumed synchronous behavior if necessary.
🤖 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 `@package.json`:
- Line 68: Run Prettier to reformat package.json and commit the result so CI
passes the npm run format:check step; e.g., run the project's formatting script
(npm run format -- package.json) or npx prettier --write package.json, then
stage and commit the updated package.json so the formatting drift is resolved.
In `@src/background.summary.test.ts`:
- Around line 110-112: The test's storage mock methods get, set, and remove
declare unused parameters causing TS6133; update the parameter names in the mock
for get, set, and remove inside src/background.summary.test.ts to use
underscore-prefixed names (e.g., _key, _items, _key) or otherwise consume them
so they are no longer reported as unused—locate the mock object with the get:
async (key: string) => ..., set: async (items: any) => ..., remove: async (key:
string) => ... and rename the parameters accordingly.
- Around line 53-115: The Chrome mock in installChromeMock is missing the
runtime.onSuspend listener which causes importing background.ts (which calls
chrome.runtime.onSuspend.addListener) to crash; update the mock object inside
installChromeMock (the chrome.runtime section) to include onSuspend: {
addListener: () => {} } so chrome.runtime.onSuspend.addListener(...) is defined
when background.ts is imported.
---
Outside diff comments:
In `@src/background.ts`:
- Around line 2303-2312: The test harness is missing a mock for
chrome.runtime.onSuspend which causes chrome.runtime.onSuspend.addListener to be
undefined when src/background.ts loads; update the chrome.runtime mock in
src/background.summary.test.ts to include an onSuspend object with a no-op
addListener function so that calls to chrome.runtime.onSuspend.addListener
succeed during tests (target the chrome.runtime mock where other runtime
properties are defined).
---
Nitpick comments:
In `@src/background.summary.test.ts`:
- Around line 123-127: The sendMessage helper dispatches onMessageListeners
synchronously and ignores returned promises, causing flaky timing; modify
sendMessage to collect each listener's return value (call listener(msg, {}, ()
=> {})) and await Promise.all on those results (use Promise.resolve for
non-promise returns or Promise.allSettled if you want to continue despite
individual failures) so tests relying on asynchronous listener work no longer
need arbitrary sleeps; update any callers/tests that assumed synchronous
behavior if necessary.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e9d7d08-8725-4b7c-9af2-2de5c0a6ad8c
📒 Files selected for processing (3)
package.jsonsrc/background.summary.test.tssrc/background.ts
| "rollup": "^2.80.0 || ^4.59.0" | ||
| } | ||
| } | ||
| } No newline at end of file |
Contributor
There was a problem hiding this comment.
Fix package.json formatting to unblock CI.
npm run format:check is currently failing due to formatting drift in this file (reported by the pipeline). Please run Prettier on package.json and commit the formatted result.
Suggested command
npm run format -- package.json
# or
npx prettier --write package.json🤖 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 `@package.json` at line 68, Run Prettier to reformat package.json and commit
the result so CI passes the npm run format:check step; e.g., run the project's
formatting script (npm run format -- package.json) or npx prettier --write
package.json, then stage and commit the updated package.json so the formatting
drift is resolved.
Source: Pipeline failures
Owner
|
try to resolve the coderabbit usggestions and also some chcksre not running try to fix them @Shreya-nipunge |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Shreya-nipunge
force-pushed
the
fix/714-summary-retrigger-silence
branch
from
2350fbc to
6955be0
Compare
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/background.summary.test.ts (1)
117-117: 💤 Low valueOptional: type assertion may be unnecessary.
SonarCloud flags this type assertion as unnecessary because TypeScript's control flow analysis should infer that
keysis astringafter theArray.isArray(keys)guard on line 107. The assertion improves readability but isn't strictly required.♻️ Optional simplification
- const key = keys as string; + const key = keys;🤖 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.summary.test.ts` at line 117, The type assertion `as string` on the `keys` variable is unnecessary because TypeScript's control flow analysis already infers that `keys` is a string after the Array.isArray guard check. Remove the type assertion and change `const key = keys as string;` to `const key = keys;` to simplify the code while maintaining the same functionality and type safety.
🤖 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.summary.test.ts`:
- Line 8: Add a new test case within or after the interval-guard test (lines
152-186) that verifies the alarm listener retry behavior. The test should
trigger the interval guard to create the alarm, then manually invoke the alarm
listener by calling alarmListeners[0]({ name: "summarize-retry" }) to simulate
the alarm firing, and finally verify that summarizeTranscriptIfNeeded() is
re-invoked and a summary fetch is dispatched. This ensures that the alarm
mechanism actually triggers the retry logic when the scheduled alarm fires, not
just that the alarm was created.
In `@src/background.ts`:
- Around line 1817-1825: The polling loop waits for the summaryInFlight boolean
to become false, but if it remains true after 10 seconds, the immediate call to
summarizeTranscriptIfNeeded(true) returns immediately due to a guard condition
that prevents execution when a summary is already in flight. Instead of relying
on polling and calling the function, capture and directly await the actual
in-flight summary promise or operation, or alternatively, if the summary is
still in flight after the timeout period, fail the session save path (around
line 1829) rather than persisting the session with an incomplete summary.
- Around line 1011-1016: The SUMMARY_RETRY_ALARM listener in the
chrome.alarms.onAlarm callback is missing the state hydration and persistence
steps that the normal chunk path performs. Before calling
summarizeTranscriptIfNeeded(), add a call to hydrateState() to load the current
state from storage so the function sees the correct state rather than defaults.
After summarizeTranscriptIfNeeded() completes, add a call to
broadcastStateUpdate() to persist any changes to activeMeetingState and notify
the UI of the updated summary. This ensures the deferred summary is properly
saved and not lost on the next worker suspend.
---
Nitpick comments:
In `@src/background.summary.test.ts`:
- Line 117: The type assertion `as string` on the `keys` variable is unnecessary
because TypeScript's control flow analysis already infers that `keys` is a
string after the Array.isArray guard check. Remove the type assertion and change
`const key = keys as string;` to `const key = keys;` to simplify the code while
maintaining the same functionality and type safety.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6775264b-dcb6-4698-880e-554c76796e43
📒 Files selected for processing (3)
package.jsonsrc/background.summary.test.tssrc/background.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #714
This change ensures transcript summaries are not silently skipped when the summarization interval guard is hit shortly before a meeting becomes inactive.
Changes
stopAudioCapture()before saving the session.Problem
Previously, if a transcript update arrived during the summarization cooldown window,
summarizeTranscriptIfNeeded()returned early without scheduling a retry. If the meeting then became silent or ended before another transcript chunk arrived, the latest discussion could be missing from summaries and action items.This change guarantees that deferred summaries are retried and that remaining transcript content is summarized before session persistence.
Type of Change
Checklist
Summary by CodeRabbit