-
Notifications
You must be signed in to change notification settings - Fork 103
[MM-69685] Fix /call logs from expanded-view popout (v1 backport) #1255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
8b45b9f
f168314
4465745
3d9b1ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,10 +9,34 @@ import {getPersistentStorage} from 'src/utils'; | |
|
|
||
| import {pluginId} from './manifest'; | ||
|
|
||
| declare global { | ||
| interface Window { | ||
| callsClientLogAppend?: (line: string) => void; | ||
| callsClientFlushAndGetLogs?: () => string; | ||
| } | ||
| } | ||
|
|
||
| let clientLogs = ''; | ||
|
|
||
| const maxArgLength = 256; | ||
|
|
||
| // 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. | ||
| const maxInMemoryLogSize = 50 * 1024; | ||
|
|
||
| function maybeFlush() { | ||
| if (clientLogs.length > maxInMemoryLogSize) { | ||
| try { | ||
| flushLogsToAccumulated(); | ||
| } catch { | ||
| // Storage quota or security error — keep only the most recent portion in memory. | ||
| clientLogs = clientLogs.slice(-maxInMemoryLogSize); | ||
| } | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
|
|
||
| function formatArg(a: unknown): string { | ||
| if (a instanceof Error) { | ||
| return a.message; | ||
|
|
@@ -28,8 +52,59 @@ function formatArg(a: unknown): string { | |
| return String(a); | ||
| } | ||
|
|
||
| // Appends a fully-formatted log line to this realm's in-memory buffer. Exposed | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: we talk about exposing it, but that's "how it's used", not what this does. |
||
| // on `window` so the expanded-view popout can write through to its opener's | ||
| // buffer rather than persisting separately. | ||
| function appendLogLine(line: string) { | ||
| clientLogs += line; | ||
| maybeFlush(); | ||
| } | ||
|
|
||
| function appendClientLog(level: string, ...args: unknown[]) { | ||
| clientLogs += `${level} [${new Date().toISOString()}] ${args.map(formatArg).join(' ')}\n`; | ||
| // Serialize in the originating realm: Error/object args belong to this | ||
| // window's realm and would fail instanceof checks if passed to the opener. | ||
| const line = `${level} [${new Date().toISOString()}] ${args.map(formatArg).join(' ')}\n`; | ||
|
|
||
| // In the expanded-view popout, route the line to the opener's buffer so | ||
| // popout-realm logs ride the main window's existing flush machinery (single | ||
| // source of truth, no cross-window storage read-modify-write race). | ||
| try { | ||
| const opener = window.opener as Window | null; | ||
| if (opener && opener !== window && typeof opener.callsClientLogAppend === 'function') { | ||
| opener.callsClientLogAppend(line); | ||
| return; | ||
| } | ||
| } catch { | ||
| // Cross-origin opener: fall through to this realm's local buffer. | ||
| } | ||
|
|
||
| clientLogs += line; | ||
| maybeFlush(); | ||
| } | ||
|
|
||
| // Expose this realm's appender and flush+getter so an expanded-view popout can | ||
| // write logs through to (and read them back from) the opener's realm. | ||
| if (typeof window !== 'undefined') { | ||
| window.callsClientLogAppend = appendLogLine; | ||
| window.callsClientFlushAndGetLogs = flushAndGetLogs; | ||
|
Comment on lines
+88
to
+89
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: instead of "polluting" More salient, the asymmetry between |
||
|
|
||
| // Wire uncaught JS errors and unhandled promise rejections into the client- | ||
| // log buffer. Without this, exceptions that crash a handler go only to | ||
| // console.error and never appear in /call logs uploads. | ||
| window.addEventListener('error', (event: ErrorEvent) => { | ||
| const {message, filename, lineno, colno, error} = event; | ||
| const errStr = error instanceof Error ? | ||
| (error.stack || `${error.name}: ${error.message}`) : | ||
| String(error || message); | ||
| appendClientLog('error', `[uncaught] ${errStr} (${filename}:${lineno}:${colno})`); | ||
| }); | ||
|
|
||
| window.addEventListener('unhandledrejection', (event: PromiseRejectionEvent) => { | ||
| const reason = event.reason instanceof Error ? | ||
| (event.reason.stack || `${event.reason.name}: ${event.reason.message}`) : | ||
| formatArg(event.reason); | ||
| appendClientLog('error', `[unhandledrejection] ${reason}`); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| }); | ||
| } | ||
|
|
||
| export function flushLogsToAccumulated(stats?: CallsClientStats | null) { | ||
|
|
@@ -74,6 +149,14 @@ export function getClientLogs() { | |
| return getPersistentStorage().getItem(STORAGE_CALLS_CLIENT_LOGS_KEY) || ''; | ||
| } | ||
|
|
||
| // Flushes this realm's in-memory buffer to storage and returns the full | ||
| // accumulated log string. Exposed on `window` so a popout can delegate the | ||
| // entire flush+read to its opener's realm in one call. | ||
| export function flushAndGetLogs(): string { | ||
| flushLogsToAccumulated(); | ||
| return getClientLogs(); | ||
| } | ||
|
|
||
| export function logErr(...args: unknown[]) { | ||
| console.error(`${pluginId}:`, ...args); | ||
| try { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,7 @@ import { | |
| import RestClient from 'src/rest_client'; | ||
| import {modals} from 'src/webapp_globals'; | ||
|
|
||
| import {flushLogsToAccumulated, getClientLogs, logDebug} from './log'; | ||
| import {flushAndGetLogs, logDebug} from './log'; | ||
| import { | ||
| areGroupCallsAllowed, | ||
| channelHasCall, | ||
|
|
@@ -177,8 +177,24 @@ export default async function slashCommandsHandler(store: Store, joinCall: joinC | |
| return {message: `/call stats ${btoa(data)}`, args}; | ||
| } | ||
| case 'logs': { | ||
| flushLogsToAccumulated(); | ||
| const allLogs = getClientLogs(); | ||
| // When running in the expanded-view popout, delegate flush+read to the | ||
| // opener's realm. The popout's own in-memory buffer is ~empty (every | ||
| // appended line is forwarded to opener.callsClientLogAppend), and on | ||
| // web sessionStorage is per-window so a local read would miss the | ||
| // opener's accumulated logs entirely. | ||
| let allLogs: string; | ||
| try { | ||
| const opener = window.opener as Window | null; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels backward to have every client of these methods have to know about |
||
| if (opener && opener !== window && typeof opener.callsClientFlushAndGetLogs === 'function') { | ||
| allLogs = opener.callsClientFlushAndGetLogs(); | ||
| } else { | ||
| allLogs = flushAndGetLogs(); | ||
| } | ||
| } catch { | ||
| // Cross-origin opener (SecurityError) or missing function — fall | ||
| // back to the local realm. | ||
| allLogs = flushAndGetLogs(); | ||
| } | ||
|
|
||
| if (!allLogs || allLogs.trim().length === 0) { | ||
| return {error: {message: 'No call logs available'}}; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this comment about
.lengthfeels out of place: we're documenting the const, not the code that later uses it. Maybe move (or remove, if trivial)?