[MM-69685] Fix /call logs from expanded-view popout (v1 backport) - #1255
[MM-69685] Fix /call logs from expanded-view popout (v1 backport)#1255bgardner8008 wants to merge 4 commits into
Conversation
When /call logs runs in the popout, delegate flush+read to the opener's
realm via window.callsClientFlushAndGetLogs. On web, sessionStorage is
per-window so the popout's local read missed the opener's accumulated
logs; the popout's in-memory buffer also forwards every line to the
opener so it holds almost nothing locally.
Also wires window.addEventListener('error'/'unhandledrejection') into
the calls log buffer so uncaught exceptions appear in /call logs uploads,
and bounds the in-memory buffer with a 50 KB auto-flush threshold.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughClient logging now supports bounded buffering, cross-window delegation from expanded views, consolidated flush-and-read access, and uncaught error logging. The ChangesClient log flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ExpandedViewPopout
participant openerWindow
participant log.ts
participant PersistedLogStorage
ExpandedViewPopout->>openerWindow: callsClientLogAppend(line)
openerWindow->>log.ts: appendLogLine(line)
log.ts->>PersistedLogStorage: flush buffered logs when threshold is exceeded
ExpandedViewPopout->>openerWindow: callsClientFlushAndGetLogs()
openerWindow->>log.ts: flushAndGetLogs()
log.ts-->>ExpandedViewPopout: accumulated log string
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 `@webapp/src/log.ts`:
- Around line 90-94: Update the unhandledrejection listener to format non-Error
rejection reasons with the existing formatArg helper instead of
String(event.reason), while preserving the current Error stack/name/message
handling and passing the resulting diagnostic text to appendClientLog.
- Around line 22-25: Update maybeFlush to wrap flushLogsToAccumulated() in
try/catch so storage quota or security errors do not interrupt logging; when
flushing fails, retain only a bounded tail of clientLogs rather than allowing
unbounded growth.
🪄 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: 902e318e-ddff-412f-83d5-c4049722b1df
📒 Files selected for processing (3)
webapp/src/index.tsxwebapp/src/log.tswebapp/src/slash_commands.tsx
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1255 +/- ##
==========================================
+ Coverage 28.00% 28.09% +0.08%
==========================================
Files 241 241
Lines 13854 13887 +33
Branches 1637 1646 +9
==========================================
+ Hits 3880 3901 +21
- Misses 9543 9551 +8
- Partials 431 435 +4
🚀 New features to boost your workflow:
|
Covers the three features added by this backport: - flushAndGetLogs (including window.callsClientFlushAndGetLogs exposure) - in-memory auto-flush at 50 KB threshold - window error/unhandledrejection listener capture Also ports the popout write-through suite from v2 (opener forwarding in appendClientLog) since v1 now has the same mechanism.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
webapp/src/log.test.ts (1)
330-347: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the lower side of the 50 KB boundary.
These tests prove that a 51 KiB message flushes and that a tiny message does not, but they do not prove that writes just below the configured threshold remain in memory. A regression to a much smaller threshold would still pass. Add a payload whose formatted line is just under 50 KiB.
Suggested test
+ test('does not flush when the formatted line is just below 50 KB', () => { + const message = 'x'.repeat(50 * 1024 - 128); + logDebug(message); + + expect(getClientLogs()).toBe(''); + });🤖 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 `@webapp/src/log.test.ts` around lines 330 - 347, Add a test in the “in-memory auto-flush at 50 KB” suite that logs a payload whose formatted line is just below 50 KiB, then asserts storage remains empty. Account for the timestamp prefix when choosing the payload size, and use the existing logDebug and getClientLogs helpers.
🤖 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.
Nitpick comments:
In `@webapp/src/log.test.ts`:
- Around line 330-347: Add a test in the “in-memory auto-flush at 50 KB” suite
that logs a payload whose formatted line is just below 50 KiB, then asserts
storage remains empty. Account for the timestamp prefix when choosing the
payload size, and use the existing logDebug and getClientLogs helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4a95b31e-0101-4b62-868b-227939c7aeb2
📒 Files selected for processing (1)
webapp/src/log.test.ts
- Wrap flushLogsToAccumulated() in try/catch inside maybeFlush() so storage quota/security errors don't interrupt callers; on failure, retain a bounded tail of the in-memory buffer instead. - Use formatArg() for non-Error unhandledrejection reasons so objects are serialized as JSON instead of reducing to [object Object]. - Add test for the non-Error rejection path.
Move Window interface augmentation (callsClientLogAppend, callsClientFlushAndGetLogs) from index.tsx into log.ts via declare global. The standalone tsconfig doesn't include index.tsx, so the properties were unknown to the compiler when log.ts was built as part of the standalone bundle.
| // Flush the in-memory buffer to storage once it exceeds this size. Keeps | ||
| // memory bounded between calls and during plugin-inactive periods when the | ||
| // window error/unhandledrejection listeners are still writing to the buffer. | ||
| // String .length is O(1) in JS so this check is cheap on every write. |
There was a problem hiding this comment.
nit: this comment about .length feels out of place: we're documenting the const, not the code that later uses it. Maybe move (or remove, if trivial)?
| return String(a); | ||
| } | ||
|
|
||
| // Appends a fully-formatted log line to this realm's in-memory buffer. Exposed |
There was a problem hiding this comment.
nit: we talk about exposing it, but that's "how it's used", not what this does.
| window.callsClientLogAppend = appendLogLine; | ||
| window.callsClientFlushAndGetLogs = flushAndGetLogs; |
There was a problem hiding this comment.
nit: instead of "polluting" window directly, do we have the option of extending an existing calls global (i.e. window.callsClient.*)?
More salient, the asymmetry between LogAppend and appendLogLine makes this harder to understand and grep: can we converge?
| // opener's accumulated logs entirely. | ||
| let allLogs: string; | ||
| try { | ||
| const opener = window.opener as Window | null; |
There was a problem hiding this comment.
It feels backward to have every client of these methods have to know about window.opener. Could we flip the interface to instead have a common method we call, but that itself resolves the window.opener? This would help focus the code in the caller to the relevant parts (flush, get logs) instead of having to sprinkle awareness of the opener distinction everytime we need to use these methods.
3 participants
Summary
Backport of MM-69654 / PR #1249 (merged to
v2) to themain(v1/RTCD) branch. All changes are in the webapp layer with no v2-specific dependencies./call logsfrom the expanded-view popout: exposeswindow.callsClientFlushAndGetLogs()on the main window. The/call logshandler detects when it's running in the popout and delegates flush+read to the opener's realm. Previously the popout's local storage read missed the opener's accumulated logs (on web,sessionStorageis per-window), and the popout's in-memory buffer was mostly empty since each line is forwarded to the opener viawindow.opener.callsClientLogAppend.window.addEventListener('error', ...)andwindow.addEventListener('unhandledrejection', ...)at module load time. Uncaught JS exceptions and unhandled promise rejections now appear in/call logsuploads instead of going only toconsole.error.maybeFlush()call after every write toclientLogs. Prevents unbounded in-memory accumulation between calls and during plugin-inactive periods when the error/unhandledrejection listeners are active.Test plan
/call logsfrom the popout — confirm the upload contains the full call log (not empty/near-empty)/call logsfrom the main window — confirm it still works as beforesetTimeout(() => { throw new Error("test") }, 100)) — confirm it appears in the next/call logsupload