feat: add complete logging system for bug troubleshooting#172
Merged
Conversation
Add a structured logging system that covers both frontend and backend, with automatic sanitization of all sensitive data (tokens, API keys, passwords, emails) at write time. Users can export logs from Settings > Data Management > Log Export to send to AI for debugging. Key components: - Frontend Logger (ring buffer, write-time sanitization, console forwarding) - Backend Logger (ring buffer, Morgan stream integration) - Log sanitizer utilities (mask secrets, emails, URL params) - GET /api/logs endpoint with X-Log-Count header - Log Export UI in DataManagementPanel with scope/level filtering - Export JSON includes environment info (platform, electron version, screen resolution, repo count, backend availability, etc.) Replaces ~100 console.* calls across 17 files with logger.* calls, adding structured module prefixes and context data for AI-friendly debugging. Covers: AI analysis, sync, GitHub API, WebDAV, backend proxy, error handling, store hydration, Electron, and more. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCentralizes logging: adds sanitizers, backend/frontend in-memory loggers, replaces console.* with structured logger usage across server and client, exposes GET /api/logs, and adds a frontend log-export UI. ChangesLogging Infrastructure and Log Export
Sequence DiagramsequenceDiagram
participant Client
participant UI as DataManagementPanel
participant FrontendLogger as Frontend Logger
participant API as Backend API
participant BackendLogger as Backend Logger
Client->>UI: open Log Export
UI->>FrontendLogger: getCounts()
FrontendLogger-->>UI: {total,debug,info,warn,error}
UI->>API: GET /api/logs?level=warn&limit=100
API->>BackendLogger: getEntries(filters)
BackendLogger-->>API: entries + counts
API-->>UI: entries + X-Log-Count
Client->>UI: Click export
UI->>FrontendLogger: getEntries(filters)
FrontendLogger-->>UI: frontend entries
UI->>API: GET /api/logs (filtered)
API-->>UI: backend entries
UI->>Client: Download logs.json
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/services/proxyService.ts (1)
141-162:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLog the handled Axios failure branches before returning.
logger.errorFromError(...)on Line 161 only runs for the final fallthrough case. The early returns for timeout,ECONNREFUSED,ETIMEDOUT, anderror.responseall skip logging, so the most common proxy failures never make it into the ring buffer or/api/logsexport.Proposed fix
} catch (error) { if (axios.isAxiosError(error)) { if (error.code === 'ECONNABORTED' || error.message.includes('timeout')) { + logger.errorFromError('proxy.error', 'Proxy request timed out', error, { + method, + url: redactUrl(url), + }); return { status: 504, headers: {}, data: { error: 'Gateway Timeout', code: 'GATEWAY_TIMEOUT' } }; } if (error.code === 'ECONNREFUSED') { + logger.errorFromError('proxy.error', 'Proxy connection refused', error, { + method, + url: redactUrl(url), + }); return { status: 502, headers: {}, data: { error: 'Proxy connection refused', code: 'PROXY_CONNECTION_REFUSED', details: error.message } }; } if (error.code === 'ETIMEDOUT') { + logger.errorFromError('proxy.error', 'Proxy connection timeout', error, { + method, + url: redactUrl(url), + }); return { status: 504, headers: {}, data: { error: 'Proxy connection timeout', code: 'PROXY_TIMEOUT', details: error.message } }; } if (error.response) { + logger.errorFromError('proxy.error', 'Upstream returned an error response', error, { + method, + url: redactUrl(url), + status: error.response.status, + }); return { status: error.response.status, headers: {}, data: error.response.data || { error: 'Upstream error' } }; } }🤖 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 `@server/src/services/proxyService.ts` around lines 141 - 162, The catch block in proxyService.ts currently returns early for several Axios error branches (timeout, ECONNABORTED, ECNREFUSED, ETIMEDOUT, and error.response) without logging, so add logging before each early return: invoke logger.errorFromError('proxy.error', <descriptive message>, error) (or a small helper like logProxyError) in the catch block inside the function that handles proxy requests so the timeout/ECONNREFUSED/ETIMEDOUT and error.response branches are recorded to the ring buffer and /api/logs, then return the same response objects as before; ensure the message includes the branch context (e.g., "Proxy timeout", "Proxy connection refused", "Proxy upstream response") and include the original error object.
🧹 Nitpick comments (4)
src/main.tsx (1)
35-35: ⚡ Quick winCapture bootstrap exceptions with
errorFromError.Use
logger.errorFromError('app', 'Failed to render React app', error)so startup failures consistently retain sanitized error details.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.tsx` at line 35, Replace the direct logger.error call with the error-retaining helper: in main.tsx where logger.error('app', 'Failed to render React app', error) is invoked, call logger.errorFromError('app', 'Failed to render React app', error) instead so startup exceptions use the sanitized error-preserving path (referencing logger.errorFromError and the existing error variable).src/services/updateService.ts (1)
57-57: ⚡ Quick winUse
errorFromErrorin update-check catch path.Switching this to
logger.errorFromError('update', '检查更新失败', error)keeps error serialization consistent with other exception logging paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/updateService.ts` at line 57, Replace the plain logger.error call in the update-check catch path with logger.errorFromError to keep error serialization consistent; locate the occurrence of logger.error('update', '检查更新失败', error) in updateService.ts (the update-check catch block) and change it to logger.errorFromError('update', '检查更新失败', error) so the error is serialized the same way as other exception logging paths.src/services/githubApi.ts (1)
195-195: ⚡ Quick winPrefer
logger.errorFromError/ normalized error payload for caught exceptions.Several catch blocks log
errordirectly vialogger.warn/error. For consistent stack/message capture in exported logs, uselogger.errorFromError(...)(or stringify explicitly) at these sites.Suggested pattern
- logger.warn('githubApi', `Failed to fetch README for ${owner}/${repo}`, error); + logger.errorFromError('githubApi', `Failed to fetch README for ${owner}/${repo}`, error);Also applies to: 224-224, 406-406, 554-554, 563-563, 774-774, 791-791, 978-978, 1038-1038, 1048-1048, 1061-1061
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/githubApi.ts` at line 195, Caught exceptions in src/services/githubApi.ts are being logged directly via logger.warn/error which loses normalized stack/message formatting; replace those calls with the normalized error helper logger.errorFromError (or build a normalized payload) so the stack and message are exported consistently. Locate each occurrence that currently calls logger.warn or logger.error with the raw error (examples shown at the README fetch block and the other spots listed: the calls at lines ~195, 224, 406, 554, 563, 774, 791, 978, 1038, 1048, 1061) and change them to use logger.errorFromError(<context-string>, error) or logger.errorFromError(<context-string>, new Error(String(error))) as appropriate, preserving the original log message text (e.g., "Failed to fetch README for ${owner}/${repo}") as the context argument so logs remain informative and normalized across the file.src/services/webdavService.ts (1)
17-17: ⚡ Quick winUse
errorFromErrorfor exception objects in WebDAV failure paths.Some warning/error calls pass raw exceptions. Switching those to
logger.errorFromError(...)will keep error details consistent in sanitized logs.Also applies to: 67-67, 87-87, 193-193, 196-196, 204-204, 291-291, 296-296, 301-301, 373-373, 533-533
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/webdavService.ts` at line 17, Replace instances where raw exception objects are passed to logger.warn/logger.error in src/services/webdavService.ts with logger.errorFromError so sanitized logs include the exception details; specifically, locate calls like logger.warn('webdav', 'JSON压缩失败，使用原始内容', e) and the other occurrences noted in the review and change them to use logger.errorFromError while preserving the original tag ('webdav') and message text and passing the exception object as the error parameter; update every listed occurrence (including the ones around JSON compression and the other line locations) to follow this pattern so the message, tag, and exception are all captured via errorFromError.
🤖 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 `@server/src/routes/configs.ts`:
- Around line 30-34: The log message interpolates user-provided
configName/configId directly into the free-text message
(logger.warn('configs.decrypt', ...)), which bypasses sanitization; change the
message to a generic constant (e.g., "Failed to decrypt config" or include only
kind) and move any sensitive fields (configId, configName, c.id, c.name) into
the structured data argument passed to logger (the third param) so they are
sanitized at write-time; update the logger.warn calls in configs.ts that
currently use template strings with configName/configId (including the
occurrences with c.id/c.name) to follow this pattern.
In `@server/src/routes/logs.ts`:
- Around line 9-15: Validate the incoming query params before calling
logger.getEntries: check that level (the variable `level`) is one of the allowed
LogLevel values (or a key in `LEVEL_ORDER`) and reject with 400 when it's not,
and check that since (the variable `since`) is either undefined or a valid
ISO/timestamp parseable by Date (or use Date.parse and verify !isNaN) and reject
with 400 when invalid; only call `logger.getEntries({ level, since })` when both
validations pass and include clear error responses for invalid inputs.
In `@server/src/services/logger.ts`:
- Around line 32-45: The LogEntry currently persists message raw (in log method)
allowing unsanitized content to be stored and returned; update the logger.log
implementation to sanitize the message using sanitizeForLog() (or a string-safe
variant) before assigning it to the LogEntry and before passing it to
morganLoggerStream.write so any interpolated secrets or request targets are
scrubbed; apply the same change to the other logging block referenced (the
similar code around the second LogEntry creation at the 131-136 section) and
ensure you keep using sanitizedData for data and sanitizedMessage for message
when constructing LogEntry and writing to morganLoggerStream.
In `@server/src/services/logSanitizer.ts`:
- Around line 77-83: sanitizeString currently misses plain-string API keys
because it never calls looksLikeSecret; update sanitizeString to first check
looksLikeSecret(value) and return maskSecret(value) for suspected secrets before
the other checks (while preserving existing handling for GitHub tokens, emails,
URLs, Bearer/Basic prefixes). Reference: sanitizeString, looksLikeSecret,
maskSecret, redactUrl, EMAIL_RE.
- Around line 43-49: The redactUrl function currently uses URLSearchParams.has()
which is case-sensitive; update redactUrl to perform case-insensitive matching
by building a lower-cased set of SENSITIVE_URL_PARAMS (e.g., lowerSensitive =
new Set(SENSITIVE_URL_PARAMS.map(k => k.toLowerCase()))), then iterate over
parsed.searchParams (for...of parsed.searchParams) and for each paramName check
if lowerSensitive.has(paramName.toLowerCase()); if so call
parsed.searchParams.set(paramName, '***') to replace the original-cased key's
value and return parsed.toString() as before.
In `@src/components/settings/DataManagementPanel.tsx`:
- Around line 230-241: The export currently allows no scopes selected; add a
guard after computing scopeCheckboxes/selectedScopes (the same pattern used for
selectedLevels) to check if selectedScopes.length === 0 and if so call
showError(t('请至少选择一个日志范围', 'Please select at least one log scope')), call
setIsExportingLogs(false), and return to prevent export; locate the block where
scopeCheckboxes and selectedScopes are defined in DataManagementPanel.tsx and
mirror the existing level-validation logic using those symbols.
- Around line 242-260: The current logic reduces selectedLevels into a minimum
threshold (minLevelName) which allows non-selected severities through; instead,
after fetching entries from logger.getEntries and from the backend fetch
(variables: frontendLogs, backendLogs, logger.getEntries, fetch(`/api/logs?...`)
and selectedLevels/levelOrder), filter the returned arrays to include only
entries whose level is explicitly in selectedLevels. Update the frontend path to
call logger.getEntries(...) as you do now but then run frontendLogs =
frontendLogs.filter(e => selectedLevels.includes(e.level)), and for backendLogs,
after parsing res.json() run backendLogs = backendLogs.filter(e =>
selectedLevels.includes(e.level)); keep using levelOrder only if you need to map
names to numbers elsewhere but do not rely on minLevel/minLevelName for final
filtering.
In `@src/services/logger.ts`:
- Around line 31-45: The log method currently only sanitizes the structured data
field but leaves the message verbatim; update the log(level, module, message,
data?) implementation to sanitize the message as well before building the
LogEntry (e.g., create sanitizedMessage = sanitizeForLog(message.toString()) or
use a string-specific sanitizer), then use sanitizedMessage in the LogEntry
instead of raw message; keep sanitizing data via sanitizeForLog(data) as before
and ensure logger.getEntries() will return the sanitized message field.
In `@src/store/useAppStore.ts`:
- Around line 781-783: The logger in the setUser function is currently emitting
a PII field (user?.login); update the logger.info call inside setUser to remove
the login field and only include non-identifying info (e.g., hasUser) — locate
the setUser function and the logger.info invocation (within useAppStore) and
change the logged payload so it no longer contains user.login before calling
set({ user, isAuthenticated: !!user }).
In `@src/utils/logSanitizer.ts`:
- Around line 43-49: redactUrl and maskUrlDomain only match lowercase param
names so sensitive params like "apiKey" or "ClientSecret" bypass redaction;
update both functions to perform case-insensitive matching by normalizing param
names before comparison: for each param in parsed.searchParams, compare its
lowercased name against SENSITIVE_URL_PARAMS entries (also lowercased) and
replace the value with '***' when matched, or alternatively map
SENSITIVE_URL_PARAMS to a Set of lowercase keys and use that for lookups; apply
the same lowercasing logic to the code path in maskUrlDomain that handles URL
query param masking (the block referenced around lines 66-68) so both helpers
redact regardless of case.
- Around line 110-133: sanitizeString currently skips masking standalone API
keys/JWTs because looksLikeSecret is never used; update sanitizeString (and any
helpers it calls like maskSecret) to call looksLikeSecret(value) for non-empty
strings before returning the raw value and, when it returns true, replace the
entire sensitive value with maskSecret(value) (or an appropriately
truncated/masked form) so standalone tokens (provider API keys, JWTs) are
redacted even when they don't match EMAIL_RE/URL/Authorization/Basic patterns;
ensure this check runs early (e.g., immediately after GitHub token check or
before URL/Authorization checks) to catch raw secrets.
---
Outside diff comments:
In `@server/src/services/proxyService.ts`:
- Around line 141-162: The catch block in proxyService.ts currently returns
early for several Axios error branches (timeout, ECONNABORTED, ECNREFUSED,
ETIMEDOUT, and error.response) without logging, so add logging before each early
return: invoke logger.errorFromError('proxy.error', <descriptive message>,
error) (or a small helper like logProxyError) in the catch block inside the
function that handles proxy requests so the timeout/ECONNREFUSED/ETIMEDOUT and
error.response branches are recorded to the ring buffer and /api/logs, then
return the same response objects as before; ensure the message includes the
branch context (e.g., "Proxy timeout", "Proxy connection refused", "Proxy
upstream response") and include the original error object.
---
Nitpick comments:
In `@src/main.tsx`:
- Line 35: Replace the direct logger.error call with the error-retaining helper:
in main.tsx where logger.error('app', 'Failed to render React app', error) is
invoked, call logger.errorFromError('app', 'Failed to render React app', error)
instead so startup exceptions use the sanitized error-preserving path
(referencing logger.errorFromError and the existing error variable).
In `@src/services/githubApi.ts`:
- Line 195: Caught exceptions in src/services/githubApi.ts are being logged
directly via logger.warn/error which loses normalized stack/message formatting;
replace those calls with the normalized error helper logger.errorFromError (or
build a normalized payload) so the stack and message are exported consistently.
Locate each occurrence that currently calls logger.warn or logger.error with the
raw error (examples shown at the README fetch block and the other spots listed:
the calls at lines ~195, 224, 406, 554, 563, 774, 791, 978, 1038, 1048, 1061)
and change them to use logger.errorFromError(<context-string>, error) or
logger.errorFromError(<context-string>, new Error(String(error))) as
appropriate, preserving the original log message text (e.g., "Failed to fetch
README for ${owner}/${repo}") as the context argument so logs remain informative
and normalized across the file.
In `@src/services/updateService.ts`:
- Line 57: Replace the plain logger.error call in the update-check catch path
with logger.errorFromError to keep error serialization consistent; locate the
occurrence of logger.error('update', '检查更新失败', error) in updateService.ts (the
update-check catch block) and change it to logger.errorFromError('update',
'检查更新失败', error) so the error is serialized the same way as other exception
logging paths.
In `@src/services/webdavService.ts`:
- Line 17: Replace instances where raw exception objects are passed to
logger.warn/logger.error in src/services/webdavService.ts with
logger.errorFromError so sanitized logs include the exception details;
specifically, locate calls like logger.warn('webdav', 'JSON压缩失败，使用原始内容', e) and
the other occurrences noted in the review and change them to use
logger.errorFromError while preserving the original tag ('webdav') and message
text and passing the exception object as the error parameter; update every
listed occurrence (including the ones around JSON compression and the other line
locations) to follow this pattern so the message, tag, and exception are all
captured via errorFromError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2431695-7dbf-4d12-9c04-c1c1998b17eb
📒 Files selected for processing (22)
server/src/db/connection.tsserver/src/db/migrations.tsserver/src/index.tsserver/src/middleware/errorHandler.tsserver/src/routes/configs.tsserver/src/routes/logs.tsserver/src/routes/proxy.tsserver/src/services/logSanitizer.tsserver/src/services/logger.tsserver/src/services/proxyService.tssrc/components/ErrorBoundary.tsxsrc/components/settings/DataManagementPanel.tsxsrc/main.tsxsrc/services/aiService.tssrc/services/autoSync.tssrc/services/backendAdapter.tssrc/services/githubApi.tssrc/services/logger.tssrc/services/updateService.tssrc/services/webdavService.tssrc/store/useAppStore.tssrc/utils/logSanitizer.ts
- Sanitize the message field in both loggers (Critical fix) - Move config name/id from message string into structured data param (Major) - Make URL param redaction case-insensitive (Major) - Validate level and since params on /api/logs endpoint (Minor)
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/src/services/logSanitizer.ts (1)
113-124: ⚡ Quick winAdd masking for
Cookie/Set-Cookie/Proxy-AuthorizationinsanitizeHeaders()(defense in depth)
sanitizeHeaders()currently masks onlyauthorizationandx-api-key;cookie,set-cookie, andproxy-authorizationaren’t handled, so they could leak if a headers object is ever logged.Current log paths don’t appear to include header objects: access logging uses
morgan('combined')(which doesn’t includeCookie), and the existinglogger.*calls log strings/URL/status rather thanreq.headers/res.headers.🔒 Proposed fix to mask credential-bearing headers
function sanitizeHeaders(headers: Record<string, unknown>): Record<string, unknown> { const result: Record<string, unknown> = {}; for (const [key, value] of Object.entries(headers)) { const lowerKey = key.toLowerCase(); - if (lowerKey === 'authorization' || lowerKey === 'x-api-key') { + if (lowerKey === 'authorization' || lowerKey === 'x-api-key' || lowerKey === 'proxy-authorization') { result[key] = typeof value === 'string' ? sanitizeString(value) : '****'; + } else if (lowerKey === 'cookie' || lowerKey === 'set-cookie') { + result[key] = '****'; } else { result[key] = sanitizeForLog(value); } } return result; }🤖 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 `@server/src/services/logSanitizer.ts` around lines 113 - 124, The sanitizeHeaders function currently only masks 'authorization' and 'x-api-key'; update sanitizeHeaders to also detect and mask credential-bearing headers 'cookie', 'set-cookie', and 'proxy-authorization' (case-insensitively) the same way as 'authorization' (i.e., use sanitizeString when value is a string, otherwise replace with '****'), leaving non-credential headers to sanitizeForLog; reference the sanitizeHeaders function and existing helpers sanitizeString and sanitizeForLog when making the change.
🤖 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 `@server/src/services/logSanitizer.ts`:
- Around line 113-124: The sanitizeHeaders function currently only masks
'authorization' and 'x-api-key'; update sanitizeHeaders to also detect and mask
credential-bearing headers 'cookie', 'set-cookie', and 'proxy-authorization'
(case-insensitively) the same way as 'authorization' (i.e., use sanitizeString
when value is a string, otherwise replace with '****'), leaving non-credential
headers to sanitizeForLog; reference the sanitizeHeaders function and existing
helpers sanitizeString and sanitizeForLog when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d7ae9e0-cf85-44cd-81db-3e891d73cdfc
📒 Files selected for processing (6)
server/src/routes/configs.tsserver/src/routes/logs.tsserver/src/services/logSanitizer.tsserver/src/services/logger.tssrc/services/logger.tssrc/utils/logSanitizer.ts
✅ Files skipped from review due to trivial changes (1)
- server/src/routes/configs.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- server/src/routes/logs.ts
- src/services/logger.ts
- server/src/services/logger.ts
- src/utils/logSanitizer.ts
Addresses CodeRabbit finding: Standalone API keys are not sanitized. Plain-string secrets (e.g., sk-..., long base64-like strings) now pass through the looksLikeSecret check before being logged.
- Remove PII (user login) from setUser log (useAppStore.ts) - Fix case-insensitive URL param redaction in maskUrlDomain (logSanitizer.ts) - Add explicit level filtering to honor exact level selection (DataManagementPanel.tsx) - Add scope validation guard (DataManagementPanel.tsx)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/settings/DataManagementPanel.tsx`:
- Around line 248-251: ESLint flags the unused "_" in the Object.entries
destructure; update the find callback in the minLevelName calculation to ignore
the first entry without creating an unused identifier—for example change the
callback from ([_, v]) => v === minLevel to ([, v]) => v === minLevel (or rename
to (_key, v) if you prefer) so that levelOrder, minLevel, minLevelName and
selectedLevels remain used correctly and the warning is suppressed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0e2e2f4-29a4-4412-ae74-977fe5610fce
📒 Files selected for processing (4)
server/src/services/logSanitizer.tssrc/components/settings/DataManagementPanel.tsxsrc/store/useAppStore.tssrc/utils/logSanitizer.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- server/src/services/logSanitizer.ts
- src/store/useAppStore.ts
Comment on lines
+248
to
+251
| // Determine min level for filtering | ||
| const levelOrder: Record<LogLevel, number> = { debug: 0, info: 1, warn: 2, error: 3 }; | ||
| const minLevel = selectedLevels.reduce((min, lvl) => Math.min(min, levelOrder[lvl]), 3); | ||
| const minLevelName = (Object.entries(levelOrder).find(([_, v]) => v === minLevel)?.[0] as LogLevel) || 'debug'; |
Contributor
There was a problem hiding this comment.
Fix unused variable from static analysis.
ESLint flags _ as unused in the destructuring pattern. Use a void expression or rename to suppress the warning.
🔧 Proposed fix
- const minLevelName = (Object.entries(levelOrder).find(([_, v]) => v === minLevel)?.[0] as LogLevel) || 'debug';
+ const minLevelName = (Object.entries(levelOrder).find(([, v]) => v === minLevel)?.[0] as LogLevel) || 'debug';📝 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.
Suggested change
| // Determine min level for filtering | |
| const levelOrder: Record<LogLevel, number> = { debug: 0, info: 1, warn: 2, error: 3 }; | |
| const minLevel = selectedLevels.reduce((min, lvl) => Math.min(min, levelOrder[lvl]), 3); | |
| const minLevelName = (Object.entries(levelOrder).find(([_, v]) => v === minLevel)?.[0] as LogLevel) || 'debug'; | |
| // Determine min level for filtering | |
| const levelOrder: Record<LogLevel, number> = { debug: 0, info: 1, warn: 2, error: 3 }; | |
| const minLevel = selectedLevels.reduce((min, lvl) => Math.min(min, levelOrder[lvl]), 3); | |
| const minLevelName = (Object.entries(levelOrder).find(([, v]) => v === minLevel)?.[0] as LogLevel) || 'debug'; |
🧰 Tools
🪛 ESLint
[error] 251-251: '_' is defined but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/settings/DataManagementPanel.tsx` around lines 248 - 251,
ESLint flags the unused "_" in the Object.entries destructure; update the find
callback in the minLevelName calculation to ignore the first entry without
creating an unused identifier—for example change the callback from ([_, v]) => v
=== minLevel to ([, v]) => v === minLevel (or rename to (_key, v) if you prefer)
so that levelOrder, minLevel, minLevelName and selectedLevels remain used
correctly and the warning is suppressed.
Owner
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/services/aiService.ts (1)
677-710: ⚡ Quick winLog the caught error for better diagnostics.
Line 701 catches
errorbut never uses it. The warning message on line 702 should include the error for troubleshooting.📊 Proposed fix
} catch (error) { - logger.warn('ai', 'AI semantic search failed, falling back to enhanced basic search', { apiType: this.getApiType(), model: this.config.model, configId: this.config.id }); + logger.errorFromError('ai', 'AI semantic search failed, falling back to enhanced basic search', error, { apiType: this.getApiType(), model: this.config.model, configId: this.config.id }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/aiService.ts` around lines 677 - 710, In searchRepositoriesWithReranking, the caught error in the catch block is not included in the warning log—update the logger.warn call in the catch to include the error details (e.g., error.message and/or error.stack or a stringified error) as part of the metadata (alongside apiType, model, configId) so diagnostics capture the actual exception thrown during requestText/AI semantic search.server/src/services/logger.ts (2)
76-93: 💤 Low valueConsider renaming
dataStror stringifying it for clarity.Line 78 assigns
entry.data(which may be an object) directly todataStr, but the name suggests a string. While console methods handle objects correctly, the naming is misleading. Consider renaming todataValueor usingJSON.stringifyif you want actual string output.🤖 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 `@server/src/services/logger.ts` around lines 76 - 93, The variable dataStr in forwardToConsole is misleading because it can hold non-string values; rename it (e.g., dataValue) and ensure you output a readable string when required by stringifying objects — implement: const dataValue = entry.data !== undefined ? entry.data : ''; then before logging, if (typeof dataValue === 'object' && dataValue !== null) { try { const dataJson = JSON.stringify(dataValue); use dataJson in the console call } catch { /* fallback to dataValue to avoid throwing */ } } — update all console.* calls in forwardToConsole to use the new symbol (dataValue or dataJson) and keep the LogEntry.type handling unchanged.
72-74: 💤 Low valueValidate the type of
extrabefore spreading.If
extrais not an object (e.g., a string or number), the type assertion on line 73 is unsafe and could cause runtime errors when spreading. Consider adding a runtime check or wrapping non-objects in a property.🛡️ Proposed defensive check
errorFromError(module: string, message: string, err: unknown, extra?: unknown): void { - this.log('error', module, message, { ...sanitizeError(err), ...(extra !== undefined ? sanitizeForLog(extra) as Record<string, unknown> : {}) }); + const sanitizedExtra = extra !== undefined ? sanitizeForLog(extra) : undefined; + const extraObj = sanitizedExtra !== undefined && typeof sanitizedExtra === 'object' && !Array.isArray(sanitizedExtra) + ? sanitizedExtra as Record<string, unknown> + : sanitizedExtra !== undefined ? { extra: sanitizedExtra } : {}; + this.log('error', module, message, { ...sanitizeError(err), ...extraObj }); }🤖 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 `@server/src/services/logger.ts` around lines 72 - 74, The errorFromError method spreads sanitizeForLog(extra) without ensuring extra is an object, which can throw at runtime for primitives; update errorFromError to validate extra at runtime (e.g., check extra !== null && typeof extra === 'object' && !Array.isArray(extra')) before spreading sanitizeForLog(extra) into the log payload, and for non-objects wrap the value under a safe key (e.g., { extra }) or include it as { extraValue: extra } so sanitizeForLog is only called/spread for real objects; references: errorFromError, sanitizeForLog, sanitizeError, and the log(...) call.
🤖 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 `@server/src/index.ts`:
- Line 25: The CORS middleware is not exposing the custom response header
X-Log-Count, so browsers can’t read res.headers.get('X-Log-Count'); update the
cors configuration in server startup by replacing app.use(cors()) with
app.use(cors({ exposedHeaders: ['X-Log-Count'] })) (or equivalent CORS options)
so the X-Log-Count header set in routes/logs.ts is accessible to the frontend;
ensure the Express app variable (app) uses this new cors(...) call.
In `@server/src/services/logSanitizer.ts`:
- Around line 70-112: sanitizeForLog and its helpers can recurse into cyclic
objects causing stack overflows; add a WeakSet "seen" that sanitizeForLog
accepts (default new WeakSet()) and check seen.has(obj) at the top to return a
safe marker (e.g., '[Circular]') for already-seen objects; when processing an
object, call seen.add(obj) before recursing and pass the same seen into
sanitizeObject, sanitizeHeaders and any recursive sanitizeForLog calls so cycles
are short-circuited. Ensure sanitizeObject and sanitizeHeaders signatures accept
and forward the seen WeakSet and only use the marker for object reference cycles
(primitives unchanged).
In `@src/services/logger.ts`:
- Around line 76-78: The errorFromError helper currently spreads
sanitizeForLog(extra) regardless of runtime type, which throws for
primitives/arrays/strings; update errorFromError to detect whether extra is a
plain object (e.g. typeof extra === 'object' && extra !== null &&
!Array.isArray(extra')) and only spread sanitizeForLog(extra) in that case,
otherwise attach the sanitized value under a single key (e.g. { extra:
sanitizeForLog(extra) }); reference the errorFromError method and
sanitizeForLog/sanitizeError helpers when making the change.
In `@src/utils/logSanitizer.ts`:
- Around line 104-112: sanitizeForLog and sanitizeObject currently recurse
without tracking visited objects, causing stack overflows on circular
references; fix by threading a WeakSet (e.g., visited) through sanitizeForLog
and sanitizeObject, add an early check that if an object is already in visited
return a canonical placeholder like "[Circular]" (or null), add newly-seen
objects to visited before recursing into their properties/array items and remove
if necessary, and ensure calls from top-level entry create the WeakSet and pass
it into recursive calls so both sanitizeForLog and sanitizeObject use the same
visited tracking (also apply the same pattern to the other recursive sanitizer
code block mentioned).
---
Nitpick comments:
In `@server/src/services/logger.ts`:
- Around line 76-93: The variable dataStr in forwardToConsole is misleading
because it can hold non-string values; rename it (e.g., dataValue) and ensure
you output a readable string when required by stringifying objects — implement:
const dataValue = entry.data !== undefined ? entry.data : ''; then before
logging, if (typeof dataValue === 'object' && dataValue !== null) { try { const
dataJson = JSON.stringify(dataValue); use dataJson in the console call } catch {
/* fallback to dataValue to avoid throwing */ } } — update all console.* calls
in forwardToConsole to use the new symbol (dataValue or dataJson) and keep the
LogEntry.type handling unchanged.
- Around line 72-74: The errorFromError method spreads sanitizeForLog(extra)
without ensuring extra is an object, which can throw at runtime for primitives;
update errorFromError to validate extra at runtime (e.g., check extra !== null
&& typeof extra === 'object' && !Array.isArray(extra')) before spreading
sanitizeForLog(extra) into the log payload, and for non-objects wrap the value
under a safe key (e.g., { extra }) or include it as { extraValue: extra } so
sanitizeForLog is only called/spread for real objects; references:
errorFromError, sanitizeForLog, sanitizeError, and the log(...) call.
In `@src/services/aiService.ts`:
- Around line 677-710: In searchRepositoriesWithReranking, the caught error in
the catch block is not included in the warning log—update the logger.warn call
in the catch to include the error details (e.g., error.message and/or
error.stack or a stringified error) as part of the metadata (alongside apiType,
model, configId) so diagnostics capture the actual exception thrown during
requestText/AI semantic search.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cceeaad4-c872-41b4-8018-24a829580f96
📒 Files selected for processing (22)
server/src/db/connection.tsserver/src/db/migrations.tsserver/src/index.tsserver/src/middleware/errorHandler.tsserver/src/routes/configs.tsserver/src/routes/logs.tsserver/src/routes/proxy.tsserver/src/services/logSanitizer.tsserver/src/services/logger.tsserver/src/services/proxyService.tssrc/components/ErrorBoundary.tsxsrc/components/settings/DataManagementPanel.tsxsrc/main.tsxsrc/services/aiService.tssrc/services/autoSync.tssrc/services/backendAdapter.tssrc/services/githubApi.tssrc/services/logger.tssrc/services/updateService.tssrc/services/webdavService.tssrc/store/useAppStore.tssrc/utils/logSanitizer.ts
- Expose X-Log-Count via CORS (Critical) - Guard recursive sanitization against circular references in both logSanitizers (Major) - Fix errorFromError crash when spreading primitive extra values (Critical)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/src/services/logSanitizer.ts (1)
17-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStandalone JWTs bypass detection here too.
GENERIC_SECRET_REexcludes., so JWTs slip throughlooksLikeSecret(). On the backend this is higher-stakes: Morgan/request logging can surface bearer JWTs as raw values. Add a JWT pattern as suggested for the frontend copy.🔧 Proposed fix
const GITHUB_TOKEN_RE = /^ghp_[a-zA-Z0-9]{36}$/; const GENERIC_SECRET_RE = /^[a-zA-Z0-9+/=_-]{20,}$/; +const JWT_RE = /^[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}$/;function sanitizeString(value: string): string { if (isGitHubToken(value)) return maskSecret(value); if (looksLikeSecret(value)) return maskSecret(value); + if (JWT_RE.test(value)) return maskSecret(value); if (EMAIL_RE.test(value)) return maskEmail(value);🤖 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 `@server/src/services/logSanitizer.ts` around lines 17 - 18, Update logSanitizer to detect standalone JWTs: add a JWT regex (e.g., matching three base64url segments separated by dots) alongside GITHUB_TOKEN_RE and GENERIC_SECRET_RE and ensure looksLikeSecret() checks this new JWT pattern too so bearer JWTs are caught by backend logging sanitization; reference the existing constants GITHUB_TOKEN_RE, GENERIC_SECRET_RE and the looksLikeSecret function when making the change.src/utils/logSanitizer.ts (1)
17-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStandalone JWTs bypass secret detection.
GENERIC_SECRET_REomits., solooksLikeSecret()never matches a JWT (eyJ...........). A JWT logged as a raw value — or under a key likejwt(not inSENSITIVE_FIELD_NAMES, notoken/secret/apikeysubstring) — is persisted unmasked and ends up in exported logs. Add a dedicated JWT pattern rather than wideningGENERIC_SECRET_RE(which would over-match dotted identifiers/paths).🔧 Proposed fix
const GITHUB_TOKEN_RE = /^ghp_[a-zA-Z0-9]{36}$/; const GENERIC_SECRET_RE = /^[a-zA-Z0-9+/=_-]{20,}$/; // long base64-ish strings +const JWT_RE = /^[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}$/;function sanitizeString(value: string): string { // GitHub token pattern if (isGitHubToken(value)) return maskSecret(value); // Plain-string secrets (e.g., sk-..., long base64-like strings) if (looksLikeSecret(value)) return maskSecret(value); + + // JWT (header.payload.signature) + if (JWT_RE.test(value)) return maskSecret(value);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/logSanitizer.ts` around lines 17 - 18, looksLikeSecret() currently misses JWTs because GENERIC_SECRET_RE excludes dots; add a new JWT regex constant (e.g., JWT_RE or JWT_PATTERN) that matches typical three-dot base64url segments (header.payload.signature) and include it in the set of checks used by looksLikeSecret() alongside GITHUB_TOKEN_RE and GENERIC_SECRET_RE so raw JWTs are detected and masked without widening GENERIC_SECRET_RE to over-match dotted identifiers.
🤖 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 `@server/src/services/logSanitizer.ts`:
- Around line 70-81: sanitizeForLog currently treats any shared reference as a
cycle because the WeakSet `seen` keeps objects forever; to fix it, only mark an
object as "in-progress" while traversing its subtree: for arrays and for object
handling in sanitizeForLog (and when delegating to sanitizeObject), call
seen.add(input) before recursing into members/properties and call
seen.delete(input) after finishing that subtree so non-cyclic shared references
(e.g., diamonds or repeated references like [a, a]) are preserved instead of
being collapsed to '[Circular]'; update the logic around Array.isArray(input)
and the object branch that calls sanitizeObject (and adjust sanitizeObject if it
also uses `seen`) to use the add-then-delete pattern while still returning
'[Circular]' when seen.has(input) at the start.
In `@src/utils/logSanitizer.ts`:
- Around line 104-116: sanitizeForLog currently keeps every visited object in
the WeakSet for the whole traversal which mislabels shared but non-circular
references as "[Circular]"; change the visitation to path-based: when you add an
object to seen (in sanitizeForLog) do so on entry and ensure you remove it
(seen.delete(input as object)) after processing its subtree so only ancestor
path membership is checked; apply this for both Array.isArray handling (map) and
the object branch that calls sanitizeObject (ensure sanitizeObject either
follows the same add/delete pattern or let sanitizeForLog add/delete around that
call) so genuine cycles still short-circuit but legitimate shared references are
preserved.
---
Outside diff comments:
In `@server/src/services/logSanitizer.ts`:
- Around line 17-18: Update logSanitizer to detect standalone JWTs: add a JWT
regex (e.g., matching three base64url segments separated by dots) alongside
GITHUB_TOKEN_RE and GENERIC_SECRET_RE and ensure looksLikeSecret() checks this
new JWT pattern too so bearer JWTs are caught by backend logging sanitization;
reference the existing constants GITHUB_TOKEN_RE, GENERIC_SECRET_RE and the
looksLikeSecret function when making the change.
In `@src/utils/logSanitizer.ts`:
- Around line 17-18: looksLikeSecret() currently misses JWTs because
GENERIC_SECRET_RE excludes dots; add a new JWT regex constant (e.g., JWT_RE or
JWT_PATTERN) that matches typical three-dot base64url segments
(header.payload.signature) and include it in the set of checks used by
looksLikeSecret() alongside GITHUB_TOKEN_RE and GENERIC_SECRET_RE so raw JWTs
are detected and masked without widening GENERIC_SECRET_RE to over-match dotted
identifiers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c04ce96f-1b4f-4a38-88b2-2415d238dcc7
📒 Files selected for processing (5)
server/src/index.tsserver/src/services/logSanitizer.tsserver/src/services/logger.tssrc/services/logger.tssrc/utils/logSanitizer.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- server/src/index.ts
- server/src/services/logger.ts
- src/services/logger.ts
Non-cyclic shared references (diamonds, [a, a]) were incorrectly collapsed to [Circular] because the WeakSet accumulated all visited objects. Changed to add-on-entry/delete-after-subtree pattern so only genuine cycles are short-circuited.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
console.*calls across 17 files with structuredlogger.*callsDetails
New files (5)
src/utils/logSanitizer.ts— Frontend sanitization (mask tokens, API keys, passwords, emails, URL params)server/src/services/logSanitizer.ts— Backend sanitization (same logic)src/services/logger.ts— Frontend Logger singleton (ring buffer 2000 entries, write-time sanitization)server/src/services/logger.ts— Backend Logger singleton + Morgan stream integrationserver/src/routes/logs.ts—GET /api/logsendpoint withX-Log-CountheaderModified files (17)
index.ts,errorHandler.ts,proxyService.ts,configs.ts,proxy.ts,migrations.ts,connection.tsautoSync.ts,backendAdapter.ts,aiService.ts,webdavService.ts,githubApi.ts,updateService.ts,useAppStore.ts,ErrorBoundary.tsx,main.tsx,DataManagementPanel.tsxLog Export UI
***<last4>format)Sanitization
ghp_*):***<last4>***<last4>****u***@domain.com***apiKey,token,password,secret, etc.Coverage (based on 50 historical issues)
maskApiKeyandmaskPassword#64)Test plan
dev:all, perform typical operations (sync, AI analysis, search)frontendLogsandbackendLogs🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Other