feat: Log Viewer & Logging System Professional Upgrade + Outlook Calendar Fixes #3185
Conversation
…patches source directory for better patch management
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/outlookCalendar/ipc.ts (1)
1008-1017: Incorrect return type annotation.The return type
Promise<Promise<boolean>>on line 1010 appears to be a typo. The function returns a boolean directly, so the correct type should bePromise<boolean>.🐛 Proposed fix
handle( 'outlook-calendar/has-credentials', - async (event): Promise<Promise<boolean>> => { + async (event): Promise<boolean> => { const server = getServerInformationByWebContentsId(event.id);
🤖 Fix all issues with AI agents
In @.sisyphus/boulder.json:
- Line 2: The active_plan value in .sisyphus/boulder.json uses an absolute,
user-specific path; change the "active_plan" entry to a repo-relative path
instead (e.g. point it at the .sisyphus/plans/log-viewer-professional-review.md
path) so the configuration is portable across machines and CI; update the
"active_plan" key in that file to the relative path string.
In `@patches-src/`@ewsjs/xhr/src/ntlmProvider.ts:
- Around line 43-48: The debug logs in ntlmProvider.ts print sensitive auth data
(this.username and other auth fields); update the logging in the NTLM
authentication entry (where console.log is used with options.url, this.domain,
this.username, ntlmOptions.workstation, options.rejectUnauthorized) to omit or
redact sensitive fields—remove the this.username log or replace it with a masked
value (e.g., show only first/last char or fixed placeholder) and ensure any
future sensitive fields use the same redaction or the project's privacy
hook/filter before logging.
In `@patches-src/`@ewsjs/xhr/src/xhrApi.ts:
- Around line 166-169: The xhr method has an extra closing brace that
prematurely ends the method/class and breaks parsing; remove the stray `}` that
follows the console.log('🚫 [INFO] XhrApi - No proxy configuration - direct
connection will be attempted') so that the statement `options =
this.getOptions(options)` remains inside the xhr method of the XhrApi class and
verify the method's opening and closing braces balance (check the xhr function
signature and any surrounding if/else blocks) to restore valid class scope.
In `@patches-src/apply-patches.sh`:
- Line 20: The find invocation in apply-patches.sh incorrectly relies on -o
precedence and only matches the final -name "*.json"; update the command to
group the name tests with escaped parentheses so the -type f applies to all
patterns, e.g. replace find "$src_dir" -type f -name "*.ts" -o -name "*.js" -o
-name "*.json" with a grouped form like find "$src_dir" -type f \( -name "*.ts"
-o -name "*.js" -o -name "*.json" \) | while read -r file; to ensure .ts, .js
and .json files are all processed.
In `@rollup.config.mjs`:
- Around line 118-150: Add a Rollup build entry for the log viewer preload by
copying the pattern used for the videoCallWindow preload config: create a config
object with input 'src/logViewerWindow/preload.ts', same plugins array (json,
replace with NODE_ENV, babel, nodeResolve, commonjs), preserveEntrySignatures
'strict', and output targeting dir 'app/preload' with file name
'log-viewer-preload.js' (format cjs, sourcemap inline, interop 'auto') so the
runtime path app/preload/log-viewer-preload.js referenced in ipc.ts resolves.
In `@src/logging/index.ts`:
- Around line 206-256: The injected consoleOverrideScript references
logLoggingFailure (undefined in renderer) and interpolates serverUrl unsafely;
fix by removing the call to logLoggingFailure in the catch and instead use a
renderer-safe fallback (e.g., console.error) inside the IIFE catch, and safely
embed serverUrl by serializing/escaping it (use JSON.stringify(serverUrl) or a
dedicated escape function) when building consoleOverrideScript; update
references in the diff to the consoleOverrideScript string, the catch block
inside the IIFE, and the serverUrl interpolation so the script uses the
serialized serverUrl and console.error (or another guaranteed renderer API)
instead of logLoggingFailure.
In `@src/logging/privacy.ts`:
- Around line 56-72: The catch block in createPrivacyHook currently re-inserts
message.data (risking sensitive leaks); change the error handling so that when
redaction fails you never write raw data to file logs: for transportName ===
'file' return the message with data replaced by a safe placeholder array (e.g.
['[Privacy redaction failed]']) or fully redacted objects, and ensure you first
check that message.data is an array before mapping—use
redactSensitiveData/redactObject for items when possible and otherwise drop or
replace non-array payloads rather than reinjecting the original message.data.
In `@src/logViewerWindow/ipc.ts`:
- Around line 25-44: The validateLogFilePath function currently allows any
absolute .log/.txt path; tighten this by implementing an allowlist check: accept
only paths that are inside the app's known log directory (e.g., DEFAULT_LOG_DIR)
or match entries in a persisted/temporary user-approved list populated by the
open-file dialog (track selections in the IPC handler that serves 'read-logs');
update validateLogFilePath to resolve and compare normalizedPath against the
allowlist and return an error if not present, and update the 'read-logs' IPC
handler to only call validateLogFilePath for renderer-provided paths and to add
paths to the approved list only via the open dialog flow.
In `@src/outlookCalendar/ipc.ts`:
- Around line 146-150: The code currently disables TLS certificate verification
unconditionally for Rocket.Chat API requests; add an allowSelfSignedCertificates
boolean to the Server type in src/servers/common.ts (default false), retrieve
the server object inside listEventsFromRocketChatServer like the other
functions, and change the HTTPS agent creation in
listEventsFromRocketChatServer, createEventOnRocketChatServer,
updateEventOnRocketChatServer, and deleteEventOnRocketChatServer to set
rejectUnauthorized = !server.allowSelfSignedCertificates so certificate
validation is only relaxed for servers that opt in.
In `@src/preload.ts`:
- Around line 79-81: The start() function can be invoked twice because both
'DOMContentLoaded' and 'load' listeners
(window.addEventListener('DOMContentLoaded', start) and
window.addEventListener('load', start)) plus the retry loop allow overlapping
calls; add a one-time guard to prevent re-entrancy by introducing a
module-scoped boolean (e.g., startStarted or startHasRun) checked at the top of
start() and set true on first entry, or register the listeners with the {once:
true} option and also ensure the retry logic respects the guard so start() exits
immediately if already running; update references to start, the two
addEventListener calls, and the retry loop to consult this guard so
initialization (listeners, store creation, etc.) runs only once.
🟡 Minor comments (13)
workspaces/desktop-release-action/action.yml-53-58 (1)
53-58: Clarify when tokens are required.With these inputs now optional, push/tag releases may fail later without clear errors. Consider updating the descriptions to explicitly state they’re required for release publishing (or add runtime validation in the action).
✍️ Proposed doc tweak
github_token: - description: 'The GitHub token to publish releases' + description: 'The GitHub token to publish releases (required for push/tag runs)' required: false snapcraft_token: - description: 'The Snapcraft token to publish releases' + description: 'The Snapcraft token to publish releases (required for push/tag runs)' required: falsepatches-src/README.md-7-16 (1)
7-16: Add a language to the fenced code block.markdownlint flags MD040 here. Use
text(or another appropriate language) on the fence.✍️ Proposed fix
-``` +```text patches-src/ ├── `@ewsjs/` │ └── xhr/ │ └── src/ │ ├── ntlmProvider.ts # Enhanced NTLM debugging │ └── xhrApi.ts # Enhanced XHR debugging ├── apply-patches.sh # Automation script └── README.md # This file</details> </blockquote></details> <details> <summary>src/logViewerWindow/types.ts-56-68 (1)</summary><blockquote> `56-68`: **Guard log-level parsing against non-string inputs.** If a non-string sneaks in from IPC or user input, `trim()`/`toLowerCase()` will throw. <details> <summary>🛡️ Defensive guard</summary> ```diff -export const isLogLevel = (value: string): value is LogLevel => { - return ['debug', 'info', 'warn', 'error', 'verbose'].includes( - value.toLowerCase() - ); -}; +export const isLogLevel = (value: unknown): value is LogLevel => { + if (typeof value !== 'string') { + return false; + } + return ['debug', 'info', 'warn', 'error', 'verbose'].includes( + value.toLowerCase() + ); +}; -export const parseLogLevel = (value: string): LogLevel => { - const trimmed = value.trim().toLowerCase(); - return isLogLevel(trimmed) ? trimmed : 'info'; -}; +export const parseLogLevel = (value: unknown): LogLevel => { + if (typeof value !== 'string') { + return 'info'; + } + const trimmed = value.trim().toLowerCase(); + return isLogLevel(trimmed) ? trimmed : 'info'; +};src/ui/main/menuBar.ts-617-635 (1)
617-635: Add the new menu label to i18n files (missing in pt-BR).Line 619 uses
menus.openLogViewer, but the pt‑BR locale doesn’t define it, so the UI will fall back to the key.🌍 Suggested pt-BR addition
"menus": { + "openLogViewer": "Abrir Visualizador de Logs", "openDevTools": "Abrir &DevTools", "openDevToolsOnAllWindows": "Abrir &DevTools em todas as janelas",src/i18n/pt-BR.i18n.json-418-520 (1)
418-520: Fix missing PT-BR accents in new log viewer strings.Line 450/424/431/502-503 include “Icone/Video/Calendario” without diacritics, which reads as typos in pt‑BR.
✍️ Suggested fixes
- "videoCall": "Janela de Video Chamada" + "videoCall": "Janela de Vídeo Chamada" ... - "videoCall": "Video Chamada", + "videoCall": "Vídeo Chamada", ... - "logIcon": "Icone do visualizador de logs", + "logIcon": "Ícone do visualizador de logs", ... - "videocall": "Video Chamada", - "outlook": "Calendario Outlook", + "videocall": "Vídeo Chamada", + "outlook": "Calendário Outlook",src/outlookCalendar/preload.ts-86-102 (1)
86-102: Same pattern:invokewithoutawaitand logging sensitive context.Same issues apply here -
invokeisn't awaited, and whileuserIdis logged (which is acceptable), ensure this aligns with the privacy redaction hooks mentioned in the PR objectives.src/outlookCalendar/preload.ts-71-84 (1)
71-84: Same issue:ipcRenderer.invokewithoutawaitin void function.For consistency, this should either use
ipcRenderer.send(fire-and-forget) or await the invoke result. The current pattern can't actually catch synchronous errors from invoke since it returns a Promise.🔧 Suggested fix
export const clearOutlookCredentials = (): void => { console.log('[OutlookCalendar] Preload: Clearing Outlook credentials'); - try { - ipcRenderer.invoke('outlook-calendar/clear-credentials'); - console.log('[OutlookCalendar] Preload: Successfully cleared credentials'); - } catch (error) { - const errorMessage = error instanceof Error ? error.message : String(error); - console.error( - '[OutlookCalendar] Preload: Failed to clear credentials:', - errorMessage - ); - } + ipcRenderer.send('outlook-calendar/clear-credentials'); + console.log('[OutlookCalendar] Preload: Sent clear credentials request'); };src/outlookCalendar/preload.ts-33-50 (1)
33-50: Potential privacy concern: logging Exchange URL.The PR objectives mention "privacy hooks to redact sensitive data" in the logging system. The Exchange URL logged here (line 35) may contain sensitive endpoint information. Consider whether this should be redacted or logged at a more restricted level.
Additionally,
ipcRenderer.invokereturns a Promise but isn't awaited here. For fire-and-forget operations,ipcRenderer.sendis more semantically correct. If the result matters (e.g., for error handling), it should be awaited.🛡️ Suggested fix for consistency
export const setOutlookExchangeUrl = (url: string, userId: string): void => { - console.log('[OutlookCalendar] Preload: Setting Exchange URL:', { - url, + console.log('[OutlookCalendar] Preload: Setting Exchange URL for user:', { userId, }); try { - ipcRenderer.invoke('outlook-calendar/set-exchange-url', url, userId); + ipcRenderer.send('outlook-calendar/set-exchange-url', url, userId); console.log('[OutlookCalendar] Preload: Successfully set Exchange URL'); } catch (error) {src/logging/cleanup.ts-12-19 (1)
12-19: ValidateretentionDaysto prevent unintended mass deletions.If
retentionDaysis non‑finite or ≤ 0, the cutoff can become future/NaN and delete too much or silently skip. Add a guard before computing the cutoff.🔧 Proposed fix
export const cleanupOldLogs = ( retentionDays = DEFAULT_RETENTION_DAYS ): void => { try { + if (!Number.isFinite(retentionDays) || retentionDays <= 0) { + console.warn('[logging] Invalid retentionDays; skipping log cleanup'); + return; + } const logsPath = app.getPath('logs'); const cutoffTime = Date.now() - retentionDays * 24 * 60 * 60 * 1000;src/logViewerWindow/ipc.ts-267-279 (1)
267-279: Handle write-stream errors when creating the ZIP.
createWriteStreamcan emiterrorevents (e.g., permissions, disk full). The current code handlesarchive.on('error')but not errors from the output stream itself, which can cause the promise to hang without rejecting.Add an error handler to the write stream:
Fix
await new Promise<void>((resolve, reject) => { const output = createWriteStream(result.filePath!); + output.on('error', reject); const archive = archiver('zip', { zlib: { level: 9 }, // Maximum compression });src/logging/utils.ts-34-61 (1)
34-61: Log synchronous failures and detect thenables correctly inlogExecutionTime.The function doesn't catch synchronous errors thrown by
fn(), losing both the failure log and execution time. Additionally, usinginstanceof Promisemisses thenable objects (objects with a.thenmethod), which can cause incorrect early completion logging.Wrap the
fn()call in try/catch and check for thenables using a duck-type check for the.thenmethod:Suggested fix
export const logExecutionTime = <T>( functionName: string, fn: () => T | Promise<T> ): T | Promise<T> => { const start = Date.now(); log.debug(`Starting execution of ${functionName}`); - const result = fn(); - - if (result instanceof Promise) { - return result + try { + const result = fn(); + if (result && typeof (result as Promise<T>).then === 'function') { + return (result as Promise<T>) .then((value) => { const duration = Date.now() - start; log.debug(`Completed execution of ${functionName} in ${duration}ms`); return value; }) .catch((error) => { const duration = Date.now() - start; log.error( `Failed execution of ${functionName} in ${duration}ms`, error ); throw error; }); - } - const duration = Date.now() - start; - log.debug(`Completed execution of ${functionName} in ${duration}ms`); - return result; + } + const duration = Date.now() - start; + log.debug(`Completed execution of ${functionName} in ${duration}ms`); + return result; + } catch (error) { + const duration = Date.now() - start; + log.error(`Failed execution of ${functionName} in ${duration}ms`, error); + throw error; + } };patches-src/@ewsjs/xhr/src/ntlmProvider.ts-96-96 (1)
96-96: Misleading error message.The error message says "second request" but this error occurs during the Type 1 request (the first request in the NTLM handshake).
- throw new Error('www-authenticate not found on response of second request'); + throw new Error('www-authenticate not found on response of Type 1 request');src/logViewerWindow/logViewerWindow.tsx-127-142 (1)
127-142: Inconsistent i18n - hardcoded Outlook Calendar string.Line 134 has a hardcoded string
'📅 Outlook Calendar'while all other options use thet()translation function. This breaks i18n consistency.['videocall', t('logViewer.filters.context.videocall')], - ['outlook', '📅 Outlook Calendar'], + ['outlook', t('logViewer.filters.context.outlook')], ['auth', t('logViewer.filters.context.auth')],
🧹 Nitpick comments (14)
workspaces/desktop-release-action/src/github.ts (1)
14-25: Consider a more generic error message.The error message states "github_token is required for release publishing", but
getOctokit()is called from various operations beyond publishing, including reading releases (line 28), reading assets (line 162), and cleanup operations (lines 174, 204). A more generic message would be clearer.💬 Suggested improvement
if (!token) { - throw new Error('github_token is required for release publishing'); + throw new Error('github_token is required for GitHub API operations'); }patches-src/apply-patches.sh (1)
36-41: Remove redundant conditional - both branches are identical.The if/else statement handles scoped and non-scoped packages identically. The comment mentions scoped packages need special handling, but the actual code is the same in both branches.
♻️ Proposed simplification
# Generate patch file echo " 🔄 Generating patch file..." - # Handle scoped package names properly - if [[ "$package_name" == @* ]]; then - # For scoped packages like `@ewsjs/xhr`, we need to pass the full name - yarn patch-package "$package_name" - else - yarn patch-package "$package_name" - fi + yarn patch-package "$package_name" echo " ✅ Patch file updated"src/i18n/en.i18n.json (1)
164-164: Pre-existing typo: "Downlaod Expired"Not introduced by this PR, but noticed a typo in the existing translations.
✏️ Fix typo
- "downloadExpired": "Downlaod Expired", + "downloadExpired": "Download Expired",src/logViewerWindow/log-viewer-window.tsx (1)
14-29: Consider using user's language preference instead of hardcoded fallback.The current implementation always uses the fallback language (
en), whilesrc/i18n/renderer.tsshows the established pattern for requesting the user's language preference via IPC (I18N_LNG_REQUESTED/I18N_LNG_RESPONDED). This means the log viewer won't respect the user's language setting.♻️ Suggested approach to respect user language
+import { request } from '../ipc/renderer'; +import { I18N_LNG_REQUESTED, I18N_LNG_RESPONDED } from '../ipc/channels'; + const setupI18n = async () => { - // For now we'll use the fallback language (en) - const lng = fallbackLng; + const lng = + (await request( + { type: I18N_LNG_REQUESTED }, + I18N_LNG_RESPONDED + )) ?? fallbackLng; await i18next.use(initReactI18next).init({ lng, fallbackLng, resources: { + ...(lng && lng !== fallbackLng && { + [lng]: { + translation: await resources[lng](), + }, + }), [fallbackLng]: { translation: await resources[fallbackLng](), }, }, interpolation, initImmediate: true, }); };src/logging/preload.ts (2)
21-53: Performance concern:getComponentContext()creates a newErroron every console call.Looking at
src/logging/context.ts(lines 103-195),getComponentContext()creates a newErrorobject to capture a stack trace on each invocation. This happens for everyconsole.log/info/warn/error/debugcall, which could impact performance in logging-heavy code paths.Consider either:
- Caching the component context when it's unlikely to change mid-execution
- Making context detection opt-in rather than automatic for every call
- Using a lighter-weight approach for hot paths
♻️ Example: lazy context evaluation
console.log = (...args: any[]) => { - const component = getComponentContext(); - const contextStr = `[${processContext}]${component !== 'general' ? ` [${component}]` : ''}`; - log.debug(contextStr, ...args); + // Only compute component context if debug logging is enabled + const contextStr = `[${processContext}]`; + log.debug(contextStr, ...args); };Alternatively, if component context is important, consider passing it explicitly from callers that need it rather than computing it on every call.
55-60: Stored original methods are not used for fallback.The original console methods are stored on
console.original(line 56), but the catch block (lines 58-60) is empty and doesn't fall back to original methods ifelectron-logfails during a log call. The originals are only useful if external code accessesconsole.original.♻️ Consider using originals as fallback within each override
console.log = (...args: any[]) => { + try { const component = getComponentContext(); const contextStr = `[${processContext}]${component !== 'general' ? ` [${component}]` : ''}`; log.debug(contextStr, ...args); + } catch { + originalConsole.log(...args); + } };src/logViewerWindow/LogEntry.tsx (1)
41-47: Remove non‑essential inline commentary ingetLevelTextColor.The mapping is already clear from code; keeping it self‑documenting aligns with the style guidance.
As per coding guidelines: No unnecessary comments - code must be self-documenting through clear naming and structure; only add comments for complex business logic or non-obvious decisions.🧹 Suggested cleanup
const getLevelTextColor = (level: LogLevel): string => { - // Match text colors to badge BACKGROUND colors: - // error → danger badge background (red) → red text - // warn → warning badge background (orange) → default text (no orange font token) - // info → primary badge background (blue) → blue text - // debug/verbose → secondary badge background (gray) → gray text - switch (level) {patches-src/@ewsjs/xhr/src/ntlmProvider.ts (1)
9-13: Use proper TypeScript null union types for strict mode.When strict mode is enabled, initializing properties with
nullrequires explicit null union types to avoid type errors.♻️ Proposed fix
- private _client: AxiosInstance = null; + private _client: AxiosInstance | null = null; - private username: string = null; - private password: string = null; + private username: string = ''; + private password: string = ''; private domain: string = '';src/outlookCalendar/getOutlookEvents.ts (1)
164-175: Fire-and-forget connectivity test runs on every URL sanitization.The connectivity test runs asynchronously in the background during URL sanitization. While this is documented as "for debugging", it will execute on every call to
sanitizeExchangeUrl, potentially creating unnecessary network traffic. Consider gating this behind a debug flag or environment variable for production builds.+ // Connectivity test for debugging - only in development + if (process.env.NODE_ENV === 'development') { console.log( '[OutlookCalendar] Running connectivity test to check server reachability...' ); // Run connectivity test in background without blocking testExchangeConnectivity(sanitizedUrl).catch((error) => { console.warn('[OutlookCalendar] Background connectivity test failed:', { url: sanitizedUrl, error: error instanceof Error ? error.message : String(error), note: 'This does not affect the URL validation result', }); }); + }src/outlookCalendar/errorClassification.ts (1)
194-207: Unused_contextparameter.The
_contextparameter is declared but not used inclassifyError. Either use it for context-aware classification or remove it.export function classifyError( error: Error | string, - _context: Record<string, any> = {} + context: Record<string, any> = {} ): ErrorClassification { const errorMessage = error instanceof Error ? error.message : String(error); const stack = error instanceof Error ? error.stack : ''; - const fullErrorText = `${errorMessage} ${stack}`.toLowerCase(); + const fullErrorText = `${errorMessage} ${stack} ${JSON.stringify(context)}`.toLowerCase();Alternatively, if context isn't needed:
export function classifyError( error: Error | string, - _context: Record<string, any> = {} ): ErrorClassification {src/logViewerWindow/logViewerWindow.tsx (2)
721-728: Use constant instead of magic number.The timeout value
100should use the existingSCROLL_DELAY_MSconstant for consistency.if (filteredLogs.length > 0 && virtuosoRef.current) { - setTimeout(() => { + setTimeout(() => { virtuosoRef.current?.scrollToIndex({ index: 0, behavior: 'smooth', }); - }, 100); + }, SCROLL_DELAY_MS); }
501-504: Clipboard API may fail silently.
navigator.clipboard.writeTextreturns a Promise that could reject (e.g., due to permissions). Consider handling the error or providing user feedback.const handleCopyLogs = useCallback(() => { const logText = filteredLogs.map((entry) => entry.raw).join('\n'); - navigator.clipboard.writeText(logText); + navigator.clipboard.writeText(logText).catch((error) => { + console.error('Failed to copy logs to clipboard:', error); + }); }, [filteredLogs]);patches-src/@ewsjs/xhr/src/xhrApi.ts (2)
131-258: Excessive debug logging in production code.The
xhrmethod contains extensive debug logging (lines 132-254) that logs request URLs, headers, authentication details, and response data. While useful for debugging, this creates several concerns:
- Performance: Multiple
console.logcalls with JSON.stringify on every request- Privacy: Request headers and data may contain sensitive information
- Log noise: Production logs will be flooded with debug output
Consider:
- Using a configurable debug flag
- Using the structured logging system mentioned in the PR
- Removing or reducing verbose logging before merging to production
324-329: Empty catch block silently swallows errors.The
disconnectmethod has an empty catch block that silently ignores all errors. At minimum, log the error for debugging purposes.disconnect() { if (this.stream) { try { this.stream.destroy(); - } - catch (e) { } + } catch (e) { + console.warn('[XhrApi] Error during stream disconnect:', e); + } } }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/alpha-release-process.md`:
- Line 15: The markdown table row
"|---------|---------------|-------------|-------------|" is missing spaces
around the pipe delimiters; update that row (and the similar row at line 88) to
include a single space before and after each pipe so it becomes "| --------- |
--------------- | ------------- | ------------- |" to satisfy the linter and
keep formatting consistent.
Comment out rejectUnauthorized: false in Outlook Calendar API calls. The code is preserved as comments for future debugging if needed.
- Use relative path in .sisyphus/boulder.json - Redact username in NTLM debug logs - Fix syntax error (extra brace) in xhrApi.ts - Fix find command precedence in apply-patches.sh - Add log viewer preload build to rollup config - Fix undefined logLoggingFailure in renderer context - Don't emit raw data on privacy redaction failure - Add allowlist for log file paths (security) - Prevent double-start in preload.ts - Fix markdown table formatting in docs
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@patches-src/`@ewsjs/xhr/src/ntlmProvider.ts:
- Line 96: Update the misleading throw message in ntlmProvider.ts where the
Error with message 'www-authenticate not found on response of second request' is
raised: change the text to reflect the actual phase (e.g., "www-authenticate not
found on response to Type 1 / first request") so the error accurately identifies
the Type 1 request phase; locate the throw by the existing error string and
adjust the message in the same place where the Error is constructed.
In `@patches-src/`@ewsjs/xhr/src/xhrApi.ts:
- Around line 330-342: Remove the unused private method getProxyString() from
the class in xhrApi.ts: locate the private getProxyString() definition and
delete it (including its body and signature) and ensure no code references it
elsewhere (search for "getProxyString"). If any tests or references exist,
update them to use the existing proxyConfig handling instead; otherwise simply
remove the method to eliminate dead code.
- Around line 305-308: The stream "end" handler currently calls resolve(null)
but the surrounding function/method is typed to return Promise<XMLHttpRequest>,
which is a type mismatch; either change the function's return type to
Promise<XMLHttpRequest | null> or make the "end" handler resolve with a valid
XMLHttpRequest instance (e.g., resolve(this.xhr) or the appropriate request
object used elsewhere). Update the signature and any callers of the method
accordingly, and ensure symbols referenced include this.stream,
progressDelegate, and resolve so you modify the correct handler.
In `@src/logging/index.ts`:
- Around line 220-223: The injected-script string currently only escapes single
quotes for serverUrl, leaving backslashes unescaped which can break the script;
update the construction around the serverUrl and webContents.id injection (the
webContentsId and serverUrl variables) to properly escape backslashes as well
(e.g., replace '\' with '\\\\' before embedding) or, better, serialize the value
using a safe serializer like JSON.stringify(serverUrl) so backslashes, quotes
and control characters are all escaped when creating the injected script string.
In `@src/logViewerWindow/ipc.ts`:
- Around line 288-316: The promise wrapping the ZIP creation lacks an error
handler on the file write stream `output` (created by `createWriteStream`) so
write failures can hang or become unhandled; add `output.on('error', ...)` to
reject the promise on stream errors, keep the existing `archive.on('error',
...)` handler, and ensure the `output.on('close', ...)` (or appropriate
finish/close) still resolves the promise after `archive.finalize()` so both
archive and output errors correctly reject the Promise in the
`archiver`/`archive.finalize` flow.
In `@src/outlookCalendar/ipc.ts`:
- Around line 452-469: The finally block currently sets isSyncInProgress = false
then starts lastSync() without awaiting, allowing a new sync to begin
concurrently; change the flow so the queued sync is awaited while the lock
remains held: when syncQueue.length > 0, pop the lastSync function, clear the
queue, call await lastSync() (with try/catch to log errors) before setting
isSyncInProgress = false so the mutex (isSyncInProgress) prevents concurrent
runs; reference isSyncInProgress, syncQueue, and lastSync in the finally block
and ensure errors from lastSync are caught and logged.
🧹 Nitpick comments (9)
src/logViewerWindow/ipc.ts (1)
221-232: Consider streaming for large log files.Reading the entire file into memory before slicing lines could be inefficient for very large log files. For typical log sizes this is acceptable, but if performance becomes an issue, consider using a streaming approach (e.g., reading chunks from the end of the file).
patches-src/@ewsjs/xhr/src/ntlmProvider.ts (4)
9-13: Consider usingundefinedor union types for nullable properties.Initializing typed properties to
nullcreates a type mismatch in strict TypeScript. Consider using union types to be explicit about nullability:- private _client: AxiosInstance = null; + private _client: AxiosInstance | null = null; - private username: string = null; - private password: string = null; + private username: string = ''; + private password: string = '';Alternatively, since the constructor always sets
usernameandpassword, you can leave them uninitialized and use the definite assignment assertion (!).
24-27: Edge case: domain parsing misses backslash at position 0.Using
indexOf("\\") > 0excludes usernames where the backslash is at position 0 (e.g.,\\username). If this is intentional to require a non-empty domain, consider adding a comment. If not:- if (username.indexOf("\\") > 0) { + if (username.indexOf("\\") >= 0) {However, if the intent is to require a proper domain prefix, the current behavior is correct but could use clarification.
57-57: Remove unnecessary type cast.The
(<any>Object)cast is unnecessary sinceObject.assignis standard ES6+.- let opt: AxiosRequestConfig = (<any>Object).assign({}, options); + let opt: AxiosRequestConfig = Object.assign({}, options);Or use the spread operator for clarity:
let opt: AxiosRequestConfig = { ...options };
110-111: Typo in comments: "wired" → "weird".- delete options.headers['authorization'] // 'fetch' has this wired addition with lower case, with lower case ntlm on server side fails - delete options.headers['connection'] // 'fetch' has this wired addition with lower case, with lower case ntlm on server side fails + delete options.headers['authorization'] // 'fetch' has this weird addition with lower case, with lower case ntlm on server side fails + delete options.headers['connection'] // 'fetch' has this weird addition with lower case, with lower case ntlm on server side failspatches-src/@ewsjs/xhr/src/xhrApi.ts (3)
62-68: Complex boolean logic could be simplified for readability.The double negation and ternary make Line 65 hard to follow:
if (typeof aucoro === 'object') { this.requestOptions = aucoro; - this.allowUntrustedCertificate = !(typeof aucoro.rejectUnauthorized !== 'undefined' ? aucoro.rejectUnauthorized : true); + // allowUntrustedCertificate is the inverse of rejectUnauthorized (default: reject = true → allow = false) + this.allowUntrustedCertificate = aucoro.rejectUnauthorized === false; } else { this.allowUntrustedCertificate = !!aucoro; }This preserves the behavior:
rejectUnauthorized: false→allowUntrustedCertificate: true, whileundefined/true→false.
388-394: Fragile status code parsing from error message.Parsing status codes from error messages using regex is brittle and could break if the message format changes.
let parse: any[] = reason.message.match(/statusCode=(\d*?)$/)This approach should only be a fallback. Consider logging a warning when this path is taken to help identify cases where proper status codes aren't being propagated.
259-279: Consider using async/await for consistency withxhr()method.The
xhrStreammethod uses.then()chains while thexhrmethod usesasync/await. For consistency and improved readability:♻️ Suggested refactor to async/await
- xhrStream(xhroptions: IXHROptions, progressDelegate: (progressData: IXHRProgress) => void): Promise<XMLHttpRequest> { + async xhrStream(xhroptions: IXHROptions, progressDelegate: (progressData: IXHRProgress) => void): Promise<XMLHttpRequest> { let client = axios.create(); // ... options setup ... - return new Promise<XMLHttpRequest>((resolve, reject) => { - let _promise: Promise<AxiosRequestConfig> = Promise.resolve(options); - if (this.authProvider) { - _promise = this.authProvider.preCall({ ... }); - client = this.authProvider.client || client; - } - _promise.then(async opt => { + try { + let opt = options; + if (this.authProvider) { + opt = await this.authProvider.preCall({ ... }); + client = this.authProvider.client || client; + } + const response = await client(opt || options); + // ... stream handling with Promise wrapper for events ...src/outlookCalendar/getOutlookEvents.ts (1)
135-147: Consider extracting duplicated path construction logic.The path segment normalization logic (filter empty segments, lowercase, remove existing ews/exchange.asmx, then append them) is duplicated between the main parsing block and the fallback block.
♻️ Proposed refactor to extract helper function
+const constructEwsPath = (url: URL): void => { + let pathSegments = url.pathname + .split('/') + .filter((segment) => segment.length > 0) + .map((segment) => segment.toLowerCase()); + + pathSegments = pathSegments.filter( + (segment) => segment !== 'ews' && segment !== 'exchange.asmx' + ); + + pathSegments.push('ews', 'exchange.asmx'); + url.pathname = `/${pathSegments.join('/')}`; +}; + export const sanitizeExchangeUrl = (serverUrl: string): string => { // ... validation code ... try { const url = new URL(serverUrl); // ... protocol/hostname validation ... - let pathSegments = url.pathname - .split('/') - .filter((segment) => segment.length > 0) - .map((segment) => segment.toLowerCase()); - - pathSegments = pathSegments.filter( - (segment) => segment !== 'ews' && segment !== 'exchange.asmx' - ); - - pathSegments.push('ews', 'exchange.asmx'); - url.pathname = `/${pathSegments.join('/')}`; + constructEwsPath(url); // ... rest of main logic ... } catch (error) { // ... fallback setup ... try { const url = new URL(fallbackUrl); // ... validation ... - let pathSegments = url.pathname - .split('/') - .filter((segment) => segment.length > 0) - .map((segment) => segment.toLowerCase()); - - pathSegments = pathSegments.filter( - (segment) => segment !== 'ews' && segment !== 'exchange.asmx' - ); - - pathSegments.push('ews', 'exchange.asmx'); - url.pathname = `/${pathSegments.join('/')}`; + constructEwsPath(url); // ... rest of fallback logic ... } } };Also applies to: 209-221
- Revert nodeIntegration to true and contextIsolation to false - Remove preload script reference (not needed with nodeIntegration) - Remove ErrorCatcher wrapper that caused Redux dispatch errors - Matches original working configuration from commit 97e1640
…okens
Log Viewer improvements:
- Fix entry limit to count actual log entries instead of raw lines
- Return totalEntries from backend for accurate 'X of Y entries' display
- Replace SelectLegacy with Select component using proper width prop
- Add aria-label to CheckBox components for accessibility
- Remove emoji from Outlook Calendar context filter
Fuselage color fixes (across codebase):
- Replace invalid font-default/font-danger/font-info with valid tokens
- Replace neutral-600 with hint
- Replace danger-500 with danger
- Replace status-font-on-danger with danger
Files: ipc.ts, logViewerWindow.tsx, LogEntry.tsx, types.ts,
DownloadItem.tsx, FileIcon.tsx, ServerInfoContent.tsx,
SettingsView.tsx, i18n files
- Add system theme detection using matchMedia for dark/light mode - Wrap LogViewerWindow with PaletteStyleTag for Fuselage theming - Fix checkbox labels to use color='default' for theme compatibility - Increase entry limit Select width to prevent text wrapping
The patch was conflicting with Yarn's built-in patch protocol which is already applied via package.json. Yarn's patch properly handles error re-throwing while patch-package was trying to apply to already-modified code, causing CI failures on all platforms.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/i18n/pt-BR.i18n.json`:
- Around line 449-505: The PT-BR strings are missing a few diacritics; update
the values for aria.logIcon ("Icone do visualizador de logs" -> "Ícone do
visualizador de logs"), filters.context.videocall ("Video Chamada" -> "Vídeo
Chamada") and filters.context.outlook ("Calendario Outlook" -> "Calendário
Outlook") to include the proper accents so the translations read correctly.
Ensure you edit the keys aria.logIcon, filters.context.videocall and
filters.context.outlook in the JSON snippet.
In `@src/logViewerWindow/ipc.ts`:
- Around line 120-124: Update the BrowserWindow webPreferences in ipc.ts to
enable contextIsolation and load the existing preload script instead of enabling
nodeIntegration: set contextIsolation: true, remove or set nodeIntegration and
nodeIntegrationInSubFrames to false, and add the preload path pointing to the
compiled preload bundle (the preload script defined in
src/logViewerWindow/preload.ts that exposes logViewerAPI). Ensure the
BrowserWindow creation uses this webPreferences object so the renderer consumes
the typed logViewerAPI via the preload.
In `@src/logViewerWindow/logViewerWindow.tsx`:
- Around line 698-785: The SearchInput currently relies on a placeholder which
screen readers don’t treat as a label; update the SearchInput component usage
(the instance using props placeholder={t('logViewer.placeholders.searchLogs')}
and value={searchFilter} with onChange={handleSearchFilterChange}) to include an
accessible label by adding either aria-label or aria-labelledby (e.g.,
aria-label={t('logViewer.labels.searchLogs')} or point aria-labelledby to a
visually-hidden label element) so screen readers announce the field; ensure the
translation key exists (or add t('logViewer.labels.searchLogs')) and keep the
placeholder as-is for visual UX.
- Around line 821-835: The overlay showing the count uses a hard-coded English
word "entries" — replace the literal in the div that renders
"{filteredLogs.length} entries" with a localized label using the project's i18n
helper (e.g., call t('entries') or i18n.t('entries') via useTranslation) and
import/use the translation hook at the top of logViewerWindow.tsx; ensure the
translation key (e.g., "entries" or a namespaced key like "logViewer.entries")
is used so the displayed text becomes "{filteredLogs.length}
{t('logViewer.entries')}" (or equivalent) and add the new key to locale files.
🧹 Nitpick comments (3)
src/public/log-viewer-window.html (1)
4-8: Consider tightening the CSP beyond scripts.Only
script-srcis defined, so other resource types remain unrestricted. Addingdefault-src 'self'and an explicitstyle-src(or moving inline styles to CSS) would harden this window.🔐 Suggested CSP update
- <meta http-equiv="Content-Security-Policy" content="script-src 'self'" /> + <meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline'" />src/logViewerWindow/logViewerWindow.tsx (2)
1-1: Consider renaming this component file to PascalCase.
LogViewerWindowis a component export, so the filename should beLogViewerWindow.tsxto match the convention. As per coding guidelines: Use camelCase for file names and PascalCase for component file names.
498-501: Resolve the streaming TODO before release.The toggle is user-facing but real-time streaming isn’t implemented. Either wire it up or hide the control until ready. Do you want me to draft the IPC + renderer changes for streaming?
| webPreferences: { | ||
| nodeIntegration: true, | ||
| nodeIntegrationInSubFrames: true, | ||
| contextIsolation: false, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/logViewerWindow/ipc.ts | head -150Repository: RocketChat/Rocket.Chat.Electron
Length of output: 5264
🏁 Script executed:
cat -n src/logViewerWindow/ipc.ts | tail -100Repository: RocketChat/Rocket.Chat.Electron
Length of output: 3827
🏁 Script executed:
find . -name "*log-viewer*" -o -name "*preload*" | grep -E "\.(ts|js|html)$" | head -20Repository: RocketChat/Rocket.Chat.Electron
Length of output: 333
🏁 Script executed:
cat -n src/logViewerWindow/preload.tsRepository: RocketChat/Rocket.Chat.Electron
Length of output: 2023
🏁 Script executed:
cat -n src/logViewerWindow/ipc.ts | grep -A 15 -B 5 "webPreferences"Repository: RocketChat/Rocket.Chat.Electron
Length of output: 691
🏁 Script executed:
# Check another window configuration to compare pattern
rg -A 10 "webPreferences" src/jitsi/ipc.ts src/notifications/ipc.ts | head -30Repository: RocketChat/Rocket.Chat.Electron
Length of output: 139
🏁 Script executed:
# Find window configurations with preload script
rg -l "webPreferences" src/ --type ts | head -10Repository: RocketChat/Rocket.Chat.Electron
Length of output: 186
🏁 Script executed:
cat -n src/videoCallWindow/ipc.ts | grep -A 15 -B 5 "webPreferences"Repository: RocketChat/Rocket.Chat.Electron
Length of output: 854
🏁 Script executed:
cat -n src/ui/main/rootWindow.ts | grep -A 15 -B 5 "webPreferences"Repository: RocketChat/Rocket.Chat.Electron
Length of output: 2182
🏁 Script executed:
cat -n src/logViewerWindow/ipc.ts | grep -A 5 "select-log-file"Repository: RocketChat/Rocket.Chat.Electron
Length of output: 331
🏁 Script executed:
# Check the directory structure to understand preload path
ls -la src/logViewerWindow/Repository: RocketChat/Rocket.Chat.Electron
Length of output: 642
🏁 Script executed:
# Check the entire openLogViewerWindow function to see full BrowserWindow config
sed -n '82,151p' src/logViewerWindow/ipc.tsRepository: RocketChat/Rocket.Chat.Electron
Length of output: 1835
🏁 Script executed:
# Check how other preload scripts are referenced
grep -r "preload:" src/ --type ts | head -5Repository: RocketChat/Rocket.Chat.Electron
Length of output: 192
🏁 Script executed:
rg "preload:" src/ -t ts | head -10Repository: RocketChat/Rocket.Chat.Electron
Length of output: 174
🏁 Script executed:
cat -n src/ui/main/serverView/index.ts | grep -B 10 -A 5 "preload:"Repository: RocketChat/Rocket.Chat.Electron
Length of output: 650
🏁 Script executed:
# Check how preload scripts are normally resolved
rg "resolvePreloadPath" src/ -t ts -B 3 -A 3 | head -20Repository: RocketChat/Rocket.Chat.Electron
Length of output: 1377
Add the preload script to the log viewer window's webPreferences to enable context isolation.
The preload script at src/logViewerWindow/preload.ts already exists with a minimal, typed API (logViewerAPI), but it's not configured in the BrowserWindow. Enable contextIsolation: true and add the preload path to webPreferences instead of relying on nodeIntegration: true.
🤖 Prompt for AI Agents
In `@src/logViewerWindow/ipc.ts` around lines 120 - 124, Update the BrowserWindow
webPreferences in ipc.ts to enable contextIsolation and load the existing
preload script instead of enabling nodeIntegration: set contextIsolation: true,
remove or set nodeIntegration and nodeIntegrationInSubFrames to false, and add
the preload path pointing to the compiled preload bundle (the preload script
defined in src/logViewerWindow/preload.ts that exposes logViewerAPI). Ensure the
BrowserWindow creation uses this webPreferences object so the renderer consumes
the typed logViewerAPI via the preload.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/i18n/pt-BR.i18n.json`:
- Around line 449-505: Update the Portuguese strings to use proper accents:
change aria.logIcon from "Icone do visualizador de logs" to "Ícone do
visualizador de logs", change filters.context.videocall from "Video Chamada" to
"Vídeo Chamada" (or "Chamada de vídeo" if preferred), and change
filters.context.outlook from "Calendario Outlook" to "Calendário Outlook" so the
new log viewer strings match existing accented PT-BR copy.
In `@src/logViewerWindow/logViewerWindow.tsx`:
- Around line 358-377: The current checkForUpdates uses
ipcRenderer.invoke('log-viewer-window/read-logs') which causes full file reads
every poll; change it to call a lightweight IPC like
'log-viewer-window/stat-log' (or similar) that only returns metadata
(lastModifiedTime and size) and update main process to implement this new
handler (stat only, no content); in checkForUpdates (and lastModifiedTimeRef
logic) invoke the new stat IPC, compare response.lastModifiedTime/size to
lastModifiedTimeRef.current, and only call loadLogs() or
ipcRenderer.invoke('log-viewer-window/read-logs', ...) when the metadata
indicates a change. Ensure you update any callers that assumed read-logs for
stats to use the new stat IPC and keep
ipcRenderer.invoke('log-viewer-window/read-logs') for actual content loading
only.
- Around line 821-835: Replace the hard-coded "entries" overlay with a localized
string using the project's i18n API: import useTranslation (or the project's
equivalent) into logViewerWindow.tsx, call const { t } = useTranslation() in the
component, and render the counter as t('logs.entries', { count:
filteredLogs.length }) (or the project's pluralization key) instead of the
literal "{filteredLogs.length} entries"; update the localization keys
accordingly and keep the same div styling and use of filteredLogs to supply the
count.
🧹 Nitpick comments (2)
src/logViewerWindow/logViewerWindow.tsx (2)
1-2: Rename this component file to PascalCase.Consider renaming to
LogViewerWindow.tsxto align with component file naming.As per coding guidelines, Use camelCase for file names and PascalCase for component file names.
498-501: Resolve the TODO in the streaming toggle.Either implement real-time streaming or remove the TODO/feature to avoid shipping incomplete behavior.
Want me to draft the streaming implementation or open a tracking issue?
- Fix incomplete string escaping in logging/index.ts (add backslash escape) - Fix race condition in outlookCalendar/ipc.ts (process queued sync inside lock) - Add missing error handler for ZIP output stream in logViewerWindow/ipc.ts - Fix misleading error message in @ewsjs/xhr ntlmProvider.ts - Fix type mismatch in @ewsjs/xhr xhrApi.ts resolve call - Remove unused getProxyString() method from @ewsjs/xhr xhrApi.ts
…rence) Comment out rejectUnauthorized: false code for Exchange 2016 self-signed certificate support. Marked with TODO for future re-enablement if needed.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/logging/index.ts`:
- Around line 205-258: The Promise returned by
webContents.executeJavaScript(consoleOverrideScript) isn't handled; append a
.catch(...) to handle rejections and log the failure—e.g., call
webContents.executeJavaScript(consoleOverrideScript).catch(err => /* log using
processLogger or logger */) so injection failures are recorded; update the call
site (webContents.executeJavaScript) to attach the .catch and include the error
and context (webContents.id and serverUrl) in the log message.
- Around line 123-144: The error JSON hook is writing raw message.data to
errors.json; import redactSensitiveData from ./privacy.ts and apply it to
message.data before serializing so sensitive fields are redacted (in the same
hook that constructs jsonEntry). Update the hook that pushes to log.hooks (the
block that defines errorJsonPath and builds jsonEntry) to call
redactSensitiveData(message.data) (or a shallow copy) and use the redacted
result for the text field, leaving createPrivacyHook() in place for the file
transport.
In `@src/outlookCalendar/ipc.ts`:
- Around line 189-195: Replace direct logging of event.subject in the Outlook
calendar IPC logging with a redacted value (e.g., "[REDACTED]" or subject
length) so sensitive calendar content isn't written to disk; specifically update
the console.log call that prints url, eventId (event.id), subject
(event.subject), and userId in the block using
urls.server(...).calendarEvents.import to omit the raw subject and instead log
only an ID, subject length, or a fixed redaction string. Apply the same change
to the other logging sites referenced (lines around the blocks that reference
event.subject / event.id) so all occurrences consistently avoid persisting
calendar subjects.
- Around line 87-121: The decryptedCredentials function currently throws on any
decryption error which breaks sync for legacy/plaintext creds; update it to
detect/handle unencrypted values by attempting to decrypt only when
safeStorage.isEncryptionAvailable() is true AND the stored string looks like
base64 (e.g., /^[A-Za-z0-9+/=]+$/ plus reasonable length), otherwise treat
login/password as plaintext and return credentials; if decryptString throws,
catch that specific error, log a warning via loggers.outlook including userId
and that legacy/plaintext fallback is used, and return the original credentials
instead of rethrowing (optionally set/return a flag like needsReauth on the
returned object if you want to prompt re-entry later).
🧹 Nitpick comments (6)
patches-src/@ewsjs/xhr/src/ntlmProvider.ts (3)
11-34: The_clientproperty is never assigned, getter always returnsnull.The
_clientproperty is initialized tonull(line 12) but is never assigned a value anywhere in this class. Theclientgetter will always returnnull, causingXhrApi.xhr()to always fall back to its default axios instance at line 239 of xhrApi.ts (client = this.authProvider.client || client).If this is intentional (the provider doesn't need its own client), consider removing the unused
_clientproperty and returningnullexplicitly or documenting this behavior. If a custom client is intended, it should be initialized.♻️ Suggested fix if a custom client is not needed
export class NtlmProvider implements IProvider { - private _client: AxiosInstance = null; - private username: string = null; private password: string = null; private domain: string = ''; ... get client(): AxiosInstance { - return this._client; + return null; // Uses XhrApi's default client }
163-170: Consider redacting sensitive details from Type 2 message logging.The Type 2 NTLM message contains server security information including the server challenge, target domain, and negotiation flags. While useful for debugging, this could expose security-relevant details in log files.
Consider logging only non-sensitive fields or a success indicator:
🔒 Suggested fix
let type2msg = decodeType2Message(response.headers['www-authenticate']); console.log( '[DEBUG] NTLM Provider - Type 2 message decoded successfully' ); - console.log( - '[DEBUG] NTLM Provider - Type 2 message details:', - JSON.stringify(type2msg, null, 2) - ); + console.log( + '[DEBUG] NTLM Provider - Type 2 target domain:', + type2msg.targetInfo?.domainName || '[not provided]' + );
78-81: Request headers may contain the Authorization token.Logging the full request headers at this point includes the
Authorizationheader containing the NTLM Type 1 message. While Type 1 doesn't contain credentials directly, it does reveal domain/workstation info. Consider excluding the Authorization header from logged output for consistency with the redaction approach used elsewhere.patches-src/@ewsjs/xhr/src/xhrApi.ts (3)
260-261: Unnecessary fallback in axios call.The expression
opt || (options as any)has an unnecessary fallback. Since_promiseis always resolved (either fromPromise.resolve(options)or fromauthProvider.preCall()),optwill never be nullish unlesspreCallexplicitly returnsnull/undefined, which would indicate a bug in the provider.Consider removing the fallback or adding explicit null checking with a proper error:
♻️ Suggested fix
- const response = await client(opt || (options as any)); + if (!opt) { + throw new Error('Auth provider returned invalid options'); + } + const response = await client(opt);
441-447: Empty catch block silently swallows errors.The
disconnect()method catches exceptions but doesn't log or handle them. While stream destruction errors may be ignorable, silent swallowing makes debugging difficult.♻️ Suggested fix
disconnect() { if (this.stream) { try { this.stream.destroy(); - } catch (e) {} + } catch (e) { + console.log('[DEBUG] XhrApi - Error during stream disconnect:', e.message); + } } }
154-157: Debug logging may expose sensitive headers.The request and response headers are logged in full (lines 154-157, 267-269), which could include
Authorization,Cookie, or other sensitive headers. Consider filtering sensitive header names before logging, consistent with the privacy filtering approach mentioned in the PR objectives.🔒 Suggested approach
const sensitiveHeaders = ['authorization', 'cookie', 'set-cookie']; const safeHeaders = Object.fromEntries( Object.entries(headers).filter(([k]) => !sensitiveHeaders.includes(k.toLowerCase())) ); console.log('[DEBUG] XhrApi - Request headers:', JSON.stringify(safeHeaders, null, 2));Also applies to: 267-269
- Add .catch() to executeJavaScript in logging to handle promise rejection - Fix PT-BR accents: Ícone, Vídeo Chamada - Keep 'Outlook Calendar' untranslated (brand name) - Add aria-label to SearchInput for accessibility - Use i18n for entries count in log viewer - Fix type bug: has-credentials channel should return boolean, not Promise<boolean>
jeanfbrito
force-pushed
the
debug-outlook
branch
from
bbb644c to
d271dc5
Compare
Windows installer download |
macOS installer download |
- Fix decryptedCredentials to handle plaintext/legacy credentials gracefully instead of throwing (checks base64 format before decryption attempt) - Apply redactSensitiveData to error JSON hook to prevent sensitive data leaks - Fix logExecutionTime to catch sync errors and properly detect thenables - Validate retentionDays in cleanupOldLogs to prevent invalid deletions - Add error handling to clipboard copy operation - Improve type guards to accept unknown input safely
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@patches-src/`@ewsjs/xhr/src/xhrApi.ts:
- Around line 151-345: The debug logs in XhrApi (around the request flow using
xhroptions, options, client and authProvider in xhrApi.ts) currently print raw
headers, request/response bodies and error payloads which may leak
Authorization/Cookie tokens and user data; update the logging to sanitize
headers (mask Authorization, Cookie, Set-Cookie and any header containing
"token" or "auth") and log only header keys and masked values or sizes, avoid
printing raw body/error.response.data — log content length and a safe preview
indicator instead, and apply the same redaction when logging options,
response.headers, error.response.headers and error.response.data before calling
setupXhrResponse; ensure use of safe property access (e.g., error?.message) when
logging error fields.
- Around line 454-477: getProxyOption currently uses proxyParts.host (which
includes the port) and always defaults to port 80; update getProxyOption to pass
proxyParts.hostname to the Axios proxy.host field and choose a protocol-aware
default port: if proxyParts.protocol startsWith "https" use 443, otherwise 80.
Keep existing auth handling (this.proxyConfig.userName/password) and the return
type AxiosProxyConfig; ensure proxyParts.port is used when present, otherwise
use the protocol-aware default.
🧹 Nitpick comments (5)
src/logging/context.ts (1)
86-96: Consider logging a warning when server ID pool is exhausted.When all 100 server IDs are allocated,
availableServerIds.shift()returnsundefinedand the fallback assignsMAX_SERVER_IDto all subsequent webviews. This could cause log ambiguity if multiple webContents share the same anonymous ID.While 100+ concurrent webviews is unlikely, a warning would help diagnose this edge case if it occurs.
💡 Optional: Add exhaustion warning
// For webviews, assign anonymous server ID from pool if (webContents.getType() === 'webview') { - const serverId = availableServerIds.shift() || MAX_SERVER_ID; + const serverId = availableServerIds.shift(); + if (serverId === undefined) { + console.warn('Server ID pool exhausted, using fallback ID'); + } + const finalServerId = serverId ?? MAX_SERVER_ID; usedServerIds.set(webContentsId, serverId); - const serverName = `server-${serverId}`; + const serverName = `server-${finalServerId}`; serverContextMap.set(webContentsId, { serverUrl: 'unknown', serverName, }); return serverName; }src/outlookCalendar/ipc.ts (1)
31-37: Unsafe type cast returns empty object asServer.The function returns
{} as Serverwhen no server is found. Callers that access properties likeserver.urlorserver.outlookCredentialswill getundefined, which may cause subtle bugs. Consider returningundefinedinstead and updating callers to handle it explicitly.♻️ Proposed refactor
-const getServerInformationByWebContentsId = (webContentsId: number): Server => { +const getServerInformationByWebContentsId = (webContentsId: number): Server | undefined => { const { servers } = select(selectPersistableValues); const server = servers.find( (server) => server.webContentsId === webContentsId ); - return server || ({} as Server); + return server; };src/logViewerWindow/logViewerWindow.tsx (3)
251-253: Consider usingTextEncoderfor size calculation.Creating a
Blobto measure string byte size is slightly inefficient.TextEncoderis more direct and avoids object allocation overhead.♻️ Suggested refactor
- const sizeInBytes = new Blob([logText]).size; + const sizeInBytes = new TextEncoder().encode(logText).length;
498-501: TODO comment indicates incomplete feature.The streaming toggle sets state but the TODO suggests full real-time streaming isn't implemented yet. Consider tracking this in an issue if it's intended for future work.
Would you like me to open an issue to track the real-time log streaming implementation?
727-741: Use constant for scroll delay consistency.The hardcoded
100delay should useSCROLL_DELAY_MSfrom constants for consistency with other scroll delays in the component.♻️ Suggested fix
onChange={() => { const newAutoScroll = !autoScroll; setAutoScroll(newAutoScroll); if (newAutoScroll) { setUserHasScrolled(false); if (filteredLogs.length > 0 && virtuosoRef.current) { setTimeout(() => { virtuosoRef.current?.scrollToIndex({ index: 0, behavior: 'smooth', }); - }, 100); + }, SCROLL_DELAY_MS); } } }}
| console.log('[DEBUG] XhrApi - Starting XHR request'); | ||
| console.log('[DEBUG] XhrApi - Request URL:', xhroptions.url); | ||
| console.log('[DEBUG] XhrApi - Request method:', xhroptions.type); | ||
| console.log( | ||
| '[DEBUG] XhrApi - Request headers:', | ||
| JSON.stringify(xhroptions.headers, null, 2) | ||
| ); | ||
| console.log( | ||
| '[DEBUG] XhrApi - Request data:', | ||
| xhroptions.data | ||
| ? `${xhroptions.data.toString().substring(0, 200)}...` | ||
| : 'No data' | ||
| ); | ||
| console.log('[DEBUG] XhrApi - Allow redirect:', xhroptions.allowRedirect); | ||
| console.log( | ||
| '[DEBUG] XhrApi - Auth provider:', | ||
| this.authProvider ? this.authProvider.providerName : 'None' | ||
| ); | ||
| console.log( | ||
| '[DEBUG] XhrApi - Allow untrusted certificate:', | ||
| this.allowUntrustedCertificate | ||
| ); | ||
|
|
||
| let client = axios.create(); | ||
| //setup xhr for github.com/andris9/fetch options | ||
| let options: AxiosRequestConfig = { | ||
| url: xhroptions.url, | ||
| data: xhroptions.data, | ||
| headers: xhroptions.headers, | ||
| method: <any>xhroptions.type, | ||
| maxRedirects: !xhroptions.allowRedirect ? 0 : 5, | ||
| //resolveWithFullResponse: true | ||
| }; | ||
|
|
||
| // TODO: Re-enable for Exchange 2016 self-signed certificate support | ||
| // if (this.allowUntrustedCertificate) { | ||
| // options.httpsAgent = new https.Agent({ rejectUnauthorized: false }); | ||
| // console.log( | ||
| // '🔧 [SUCCESS FACTOR] XhrApi - Using custom HTTPS agent with rejectUnauthorized: false' | ||
| // ); | ||
| // console.log( | ||
| // '📋 [SUCCESS FACTOR] This setting allows self-signed/invalid certificates - CRITICAL for many Exchange servers!' | ||
| // ); | ||
| // } else { | ||
| // console.log( | ||
| // '🔒 [POTENTIAL ISSUE] XhrApi - Using default certificate validation - may fail with self-signed certs' | ||
| // ); | ||
| // } | ||
|
|
||
| let proxyConfig = this.getProxyOption(); | ||
| if (proxyConfig) { | ||
| options['proxy'] = proxyConfig; | ||
| console.log( | ||
| '🌐 [SUCCESS FACTOR] XhrApi - Using proxy configuration:', | ||
| JSON.stringify(proxyConfig, null, 2) | ||
| ); | ||
| console.log( | ||
| '📋 [SUCCESS FACTOR] Proxy is configured - CRITICAL for corporate environments behind firewalls!' | ||
| ); | ||
| } else { | ||
| console.log( | ||
| '🚫 [INFO] XhrApi - No proxy configuration - direct connection will be attempted' | ||
| ); | ||
| } | ||
| options = this.getOptions(options); | ||
|
|
||
| console.log( | ||
| '[DEBUG] XhrApi - Final options before auth:', | ||
| JSON.stringify( | ||
| { | ||
| url: options.url, | ||
| method: options.method, | ||
| maxRedirects: options.maxRedirects, | ||
| headers: options.headers, | ||
| }, | ||
| null, | ||
| 2 | ||
| ) | ||
| ); | ||
|
|
||
| let _promise: Promise<AxiosRequestConfig> = Promise.resolve(options); | ||
|
|
||
| try { | ||
| if (this.authProvider) { | ||
| console.log('[DEBUG] XhrApi - Calling auth provider preCall method'); | ||
| _promise = this.authProvider.preCall({ | ||
| ...options, | ||
| // TODO: Re-enable for Exchange 2016 self-signed certificate support | ||
| // rejectUnauthorized: !this.allowUntrustedCertificate, | ||
| }); | ||
| client = this.authProvider.client || client; | ||
| console.log( | ||
| '[DEBUG] XhrApi - Auth provider client:', | ||
| client ? 'Custom client' : 'Default client' | ||
| ); | ||
| } | ||
|
|
||
| const opt = await _promise; | ||
| console.log( | ||
| '[DEBUG] XhrApi - Options after auth preCall:', | ||
| JSON.stringify( | ||
| { | ||
| url: opt.url, | ||
| method: opt.method, | ||
| headers: opt.headers, | ||
| }, | ||
| null, | ||
| 2 | ||
| ) | ||
| ); | ||
|
|
||
| console.log('[DEBUG] XhrApi - Executing final HTTP request'); | ||
| const response = await client(opt || (options as any)); | ||
|
|
||
| console.log('[DEBUG] XhrApi - HTTP request completed successfully'); | ||
| console.log('[DEBUG] XhrApi - Response status:', response.status); | ||
| console.log('[DEBUG] XhrApi - Response statusText:', response.statusText); | ||
| console.log( | ||
| '[DEBUG] XhrApi - Response headers:', | ||
| JSON.stringify(response.headers, null, 2) | ||
| ); | ||
| console.log( | ||
| '[DEBUG] XhrApi - Response data length:', | ||
| response.data ? response.data.toString().length : 0 | ||
| ); | ||
| console.log( | ||
| '[DEBUG] XhrApi - Response data preview:', | ||
| response.data | ||
| ? response.data.toString().substring(0, 300) + '...' | ||
| : 'No data' | ||
| ); | ||
|
|
||
| let xhrResponse: XMLHttpRequest = <any>{ | ||
| response: response.data ? response.data.toString() : '', | ||
| status: response.status, | ||
| //redirectCount: meta.redirectCount, | ||
| headers: response.headers, | ||
| finalUrl: response.headers.location || response.request.res.responseUrl, | ||
| responseType: '', | ||
| statusText: response.statusText, | ||
| }; | ||
| if (xhrResponse.status === 200) { | ||
| console.log('🎉 [COMPLETE SUCCESS] XhrApi - HTTP 200 OK received!'); | ||
| console.log( | ||
| '📋 [COMPLETE SUCCESS] NTLM authentication + Exchange communication FULLY WORKING!' | ||
| ); | ||
| console.log( | ||
| '🏆 [SOLUTION SUMMARY] The following factors enabled success:' | ||
| ); | ||
| // TODO: Re-enable for Exchange 2016 self-signed certificate support | ||
| // if (this.allowUntrustedCertificate) { | ||
| // console.log( | ||
| // ' ✅ SSL Certificate validation disabled (allowUntrustedCertificate: true)' | ||
| // ); | ||
| // } | ||
| if (this.getProxyOption()) { | ||
| console.log(' ✅ Proxy configuration was used'); | ||
| } | ||
| console.log(' ✅ NTLM 3-step handshake completed successfully'); | ||
| console.log(' ✅ Exchange server accepted authentication'); | ||
| return setupXhrResponse(xhrResponse); | ||
| } else { | ||
| console.log( | ||
| '❌ [FINAL FAILURE] XhrApi - Request failed with status:', | ||
| xhrResponse.status | ||
| ); | ||
| console.log( | ||
| '📋 [FINAL FAILURE] Even after successful NTLM auth, Exchange rejected the request' | ||
| ); | ||
| throw setupXhrResponse(xhrResponse); | ||
| } | ||
| } catch (error) { | ||
| console.log('[DEBUG] XhrApi - Request failed with exception:'); | ||
| console.log('[DEBUG] XhrApi - Error type:', typeof error); | ||
| console.log('[DEBUG] XhrApi - Error message:', error.message); | ||
| console.log('[DEBUG] XhrApi - Error code:', error.code); | ||
| console.log('[DEBUG] XhrApi - Error stack:', error.stack); | ||
|
|
||
| if (error.response) { | ||
| console.log( | ||
| '[DEBUG] XhrApi - Error response status:', | ||
| error.response.status | ||
| ); | ||
| console.log( | ||
| '[DEBUG] XhrApi - Error response statusText:', | ||
| error.response.statusText | ||
| ); | ||
| console.log( | ||
| '[DEBUG] XhrApi - Error response headers:', | ||
| JSON.stringify(error.response.headers, null, 2) | ||
| ); | ||
| console.log( | ||
| '[DEBUG] XhrApi - Error response data:', | ||
| error.response.data | ||
| ); |
There was a problem hiding this comment.
Redact sensitive data in debug logs.
Logging raw headers/body/error data risks leaking Authorization, cookies, tokens, and user content into logs. Please redact or log sizes only.
🔒 Proposed fix (redact headers + avoid raw body dumps)
- console.log(
- '[DEBUG] XhrApi - Request headers:',
- JSON.stringify(xhroptions.headers, null, 2)
- );
+ const safeRequestHeaders = redactHeaders(xhroptions.headers);
+ console.log(
+ '[DEBUG] XhrApi - Request headers:',
+ JSON.stringify(safeRequestHeaders, null, 2)
+ );
console.log(
'[DEBUG] XhrApi - Request data:',
- xhroptions.data
- ? `${xhroptions.data.toString().substring(0, 200)}...`
- : 'No data'
+ xhroptions.data
+ ? `[${xhroptions.data.toString().length} bytes]`
+ : 'No data'
);
@@
- console.log(
- '[DEBUG] XhrApi - Response headers:',
- JSON.stringify(response.headers, null, 2)
- );
+ const safeResponseHeaders = redactHeaders(response.headers);
+ console.log(
+ '[DEBUG] XhrApi - Response headers:',
+ JSON.stringify(safeResponseHeaders, null, 2)
+ );
@@
- console.log(
- '[DEBUG] XhrApi - Response data preview:',
- response.data
- ? response.data.toString().substring(0, 300) + '...'
- : 'No data'
- );
+ console.log(
+ '[DEBUG] XhrApi - Response data preview:',
+ response.data
+ ? `[${response.data.toString().length} bytes]`
+ : 'No data'
+ );
@@
- console.log(
- '[DEBUG] XhrApi - Error response headers:',
- JSON.stringify(error.response.headers, null, 2)
- );
+ const safeErrorHeaders = redactHeaders(error.response.headers);
+ console.log(
+ '[DEBUG] XhrApi - Error response headers:',
+ JSON.stringify(safeErrorHeaders, null, 2)
+ );
console.log(
'[DEBUG] XhrApi - Error response data:',
- error.response.data
+ error.response.data
+ ? `[${error.response.data.toString().length} bytes]`
+ : 'No data'
);
@@
+function redactHeaders(headers: Record<string, any>) {
+ if (!headers) return headers;
+ const redacted = { ...headers };
+ for (const key of Object.keys(redacted)) {
+ if (/authorization|cookie|token|password/i.test(key)) {
+ redacted[key] = '[REDACTED]';
+ }
+ }
+ return redacted;
+}🤖 Prompt for AI Agents
In `@patches-src/`@ewsjs/xhr/src/xhrApi.ts around lines 151 - 345, The debug logs
in XhrApi (around the request flow using xhroptions, options, client and
authProvider in xhrApi.ts) currently print raw headers, request/response bodies
and error payloads which may leak Authorization/Cookie tokens and user data;
update the logging to sanitize headers (mask Authorization, Cookie, Set-Cookie
and any header containing "token" or "auth") and log only header keys and masked
values or sizes, avoid printing raw body/error.response.data — log content
length and a safe preview indicator instead, and apply the same redaction when
logging options, response.headers, error.response.headers and
error.response.data before calling setupXhrResponse; ensure use of safe property
access (e.g., error?.message) when logging error fields.
| private getProxyOption(): AxiosProxyConfig { | ||
| if (this.proxyConfig.enabled) { | ||
| let url: string = this.proxyConfig.url; | ||
| let proxyParts = new URL(url); | ||
| if (this.proxyConfig.userName && this.proxyConfig.password) { | ||
| return { | ||
| protocol: proxyParts.protocol, | ||
| host: proxyParts.host, | ||
| port: proxyParts.port ? Number(proxyParts.port) : 80, | ||
| auth: { | ||
| username: this.proxyConfig.userName, | ||
| password: this.proxyConfig.password, | ||
| }, | ||
| }; | ||
| } else { | ||
| return { | ||
| protocol: proxyParts.protocol, | ||
| host: proxyParts.host, | ||
| port: proxyParts.port ? Number(proxyParts.port) : 80, | ||
| }; | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
URL.host vs URL.hostname difference JavaScript
💡 Result:
In JavaScript (both new URL(...) and window.location):
-
url.hostname= just the domain (or IP), no port. Example:
https://example.com:8080/path→hostname === "example.com"[2] -
url.host= hostname plus port (if a non-default port is present). Example:
https://example.com:8080/path→host === "example.com:8080"[1]
For default ports like:443onhttps, the port is typically omitted inhost(so you’ll often just get"example.com"). [1]
So: host = hostname + optional :port. [1] [2]
🌐 Web query:
Axios proxy configuration host port format requirements
💡 Result:
In Axios (Node.js), the proxy config is an object (not a host:port string) with these format requirements:
proxy.host: string hostname or IP (e.g."127.0.0.1"or"proxy.example.com"). (npmjs.com)proxy.port: number (e.g.8080, not"8080"). (npmjs.com)proxy.protocol(optional): string like"http"or"https"; required if the proxy itself is HTTPS (set to"https"). (npmjs.com)proxy.auth(optional):{ username, password }for Basic auth; Axios setsProxy-Authorization. (npmjs.com)- If both are provided,
proxy.hostnametakes precedence overproxy.host. (npmjs.com) - Use
proxy: falseto disable proxying (including ignoringHTTP_PROXY/HTTPS_PROXYenv vars). (npmjs.com)
Example:
axios.get('https://example.com', {
proxy: {
protocol: 'http',
host: 'proxy.example.com',
port: 8080,
auth: { username: 'user', password: 'pass' }
}
});
``` ([npmjs.com](https://www.npmjs.com/package/axios/v/1.2.2?utm_source=openai))
Citations:
- 1: https://www.npmjs.com/package/axios/v/1.2.2?utm_source=openai
- 2: https://www.npmjs.com/package/axios/v/1.2.2?utm_source=openai
- 3: https://www.npmjs.com/package/axios/v/1.2.2?utm_source=openai
- 4: https://www.npmjs.com/package/axios/v/1.2.2?utm_source=openai
- 5: https://www.npmjs.com/package/axios/v/1.2.2?utm_source=openai
- 6: https://www.npmjs.com/package/axios/v/1.2.2?utm_source=openai
- 7: https://www.npmjs.com/package/axios/v/1.2.2?utm_source=openai
---
🏁 Script executed:
```shell
# Check if the file exists and get context
fd "xhrApi.ts" -type fRepository: RocketChat/Rocket.Chat.Electron
Length of output: 245
🏁 Script executed:
# Find the xhrApi.ts file
find . -name "xhrApi.ts" -type f 2>/dev/nullRepository: RocketChat/Rocket.Chat.Electron
Length of output: 113
🏁 Script executed:
# Read the file around lines 454-477
sed -n '454,477p' ./patches-src/@ewsjs/xhr/src/xhrApi.tsRepository: RocketChat/Rocket.Chat.Electron
Length of output: 819
Use hostname instead of host, and set protocol-aware default port.
The code currently passes proxyParts.host to Axios, but URL.host includes the port (e.g., "proxy.example.com:8080"), while Axios's proxy.host field expects only the hostname. This will break Axios proxy resolution. Additionally, all proxies default to port 80, but HTTPS proxies should default to 443.
🔧 Proposed fix
let proxyParts = new URL(url);
+ const defaultPort = proxyParts.port
+ ? Number(proxyParts.port)
+ : proxyParts.protocol === 'https:'
+ ? 443
+ : 80;
if (this.proxyConfig.userName && this.proxyConfig.password) {
return {
protocol: proxyParts.protocol,
- host: proxyParts.host,
- port: proxyParts.port ? Number(proxyParts.port) : 80,
+ host: proxyParts.hostname,
+ port: defaultPort,
auth: {
username: this.proxyConfig.userName,
password: this.proxyConfig.password,
},
};
} else {
return {
protocol: proxyParts.protocol,
- host: proxyParts.host,
- port: proxyParts.port ? Number(proxyParts.port) : 80,
+ host: proxyParts.hostname,
+ port: defaultPort,
};
}📝 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.
| private getProxyOption(): AxiosProxyConfig { | |
| if (this.proxyConfig.enabled) { | |
| let url: string = this.proxyConfig.url; | |
| let proxyParts = new URL(url); | |
| if (this.proxyConfig.userName && this.proxyConfig.password) { | |
| return { | |
| protocol: proxyParts.protocol, | |
| host: proxyParts.host, | |
| port: proxyParts.port ? Number(proxyParts.port) : 80, | |
| auth: { | |
| username: this.proxyConfig.userName, | |
| password: this.proxyConfig.password, | |
| }, | |
| }; | |
| } else { | |
| return { | |
| protocol: proxyParts.protocol, | |
| host: proxyParts.host, | |
| port: proxyParts.port ? Number(proxyParts.port) : 80, | |
| }; | |
| } | |
| } | |
| return null; | |
| } | |
| private getProxyOption(): AxiosProxyConfig { | |
| if (this.proxyConfig.enabled) { | |
| let url: string = this.proxyConfig.url; | |
| let proxyParts = new URL(url); | |
| const defaultPort = proxyParts.port | |
| ? Number(proxyParts.port) | |
| : proxyParts.protocol === 'https:' | |
| ? 443 | |
| : 80; | |
| if (this.proxyConfig.userName && this.proxyConfig.password) { | |
| return { | |
| protocol: proxyParts.protocol, | |
| host: proxyParts.hostname, | |
| port: defaultPort, | |
| auth: { | |
| username: this.proxyConfig.userName, | |
| password: this.proxyConfig.password, | |
| }, | |
| }; | |
| } else { | |
| return { | |
| protocol: proxyParts.protocol, | |
| host: proxyParts.hostname, | |
| port: defaultPort, | |
| }; | |
| } | |
| } | |
| return null; | |
| } |
🤖 Prompt for AI Agents
In `@patches-src/`@ewsjs/xhr/src/xhrApi.ts around lines 454 - 477, getProxyOption
currently uses proxyParts.host (which includes the port) and always defaults to
port 80; update getProxyOption to pass proxyParts.hostname to the Axios
proxy.host field and choose a protocol-aware default port: if
proxyParts.protocol startsWith "https" use 443, otherwise 80. Keep existing auth
handling (this.proxyConfig.userName/password) and the return type
AxiosProxyConfig; ensure proxyParts.port is used when present, otherwise use the
protocol-aware default.
Instead of calling read-logs with limit:1 to check for file changes, use a dedicated stat-log IPC that only reads file metadata (mtime, size). This reduces I/O during auto-refresh polling intervals.
Summary
This PR brings together multiple improvements:
Changes
Outlook Calendar Fixes
@ewsjs/xhr Patch (PR #3184)
Logging System Upgrade
loggers.outlook,loggers.auth, etc. for organized loggingLog Viewer Feature
Testing
yarn lintpasses (0 errors)Screenshots
The Log Viewer can be accessed via View menu → Open Log Viewer
Summary by CodeRabbit
New Features
New Features
Improvements
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.