Add：redacted diagnostic logs#173
Closed
hych0317 wants to merge 1 commit into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR adds a complete diagnostic logging system to capture service lifecycle events across the application, store them locally in IndexedDB with privacy-aware sanitization, and expose them through a new "Logs" tab in the settings panel with filtering and export capabilities. ChangesDiagnostic Logging Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/components/settings/LogsPanel.tsx (1)
48-59: ⚡ Quick winDebounce the reload on log events.
handleChangetriggers a fullgetLogs()(reads up tomaxEntriesfrom IndexedDB) plus re-render on everygsm:diagnostic-log-added. While the panel is open during a chatty flow (e.g., AI analysis issuing many GitHub requests), this can cause a burst of redundant reads and UI jank. A small debounce coalesces the events.♻️ Proposed debounce
useEffect(() => { loadLogs(); - const handleChange = () => { - void loadLogs(); - }; + let timer: ReturnType<typeof setTimeout> | null = null; + const handleChange = () => { + if (timer) clearTimeout(timer); + timer = setTimeout(() => { void loadLogs(); }, 300); + }; window.addEventListener('gsm:diagnostic-log-added', handleChange); window.addEventListener('gsm:diagnostic-logs-cleared', handleChange); return () => { + if (timer) clearTimeout(timer); window.removeEventListener('gsm:diagnostic-log-added', handleChange); window.removeEventListener('gsm:diagnostic-logs-cleared', handleChange); }; }, [loadLogs]);🤖 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/LogsPanel.tsx` around lines 48 - 59, The event handler in LogsPanel (inside the useEffect that calls loadLogs) fires loadLogs on every gsm:diagnostic-log-added and gsm:diagnostic-logs-cleared causing redundant reads; wrap the reload in a short debounce: replace the inline handleChange with a debounced version (e.g., useRef timer or a useDebouncedCallback) that batches rapid events (100–300ms) and only calls loadLogs once per burst, ensure the cleanup clears the timer and removes the same debounced handler from window events so loadLogs and the event listeners in the useEffect still reference the debounced function.
🤖 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/LogsPanel.tsx`:
- Around line 200-204: The CheckCircle/AlertCircle icons in LogsPanel.tsx convey
status only visually; update the JSX that renders CheckCircle and AlertCircle
(the block using entry.success) to add role="img" and a meaningful aria-label
(e.g., aria-label={`Log entry ${entry.success ? 'succeeded' : 'failed'}`} or
similar) so screen readers announce status; keep the existing classNames and
conditional logic but ensure the aria-label reflects the success boolean.
- Around line 182-185: The current JSX always shows "No logs yet" when
filteredLogs.length === 0; change it to differentiate the case where there are
logs but none match the current filters/search. In the LogsPanel component,
replace the single conditional rendering that checks filteredLogs.length with a
nested check: if logs.length === 0 render the existing "No logs yet" message,
otherwise render a distinct message like "No matching logs" (or "No logs match
your filters") so users know filters are excluding results; update the branch
that references filteredLogs and logs accordingly in the component's
render/return.
In `@src/services/aiService.ts`:
- Around line 331-340: The log call in aiService.ts is persisting raw upstream
error bodies via appLogger.serializeError(error); update the error logging in
the AI request failure path (the appLogger.error call inside
requestText()/request handling) to remove or redact provider response bodies
before serialization: extract error metadata you need (status/code/message) but
strip any response.body, response.data, prompt, or text fields added to the
error object by the provider/backend, then pass the sanitized error to
appLogger.serializeError (or log a constructed sanitizedError object) so no raw
provider response content is persisted.
In `@src/services/appLogger.ts`:
- Around line 209-214: clearLogs() currently deletes IndexedDB immediately which
races with pending writes; change it to run via the existing writeQueue so the
delete executes only after prior queued writes and prevents new writes from
running concurrently. Concretely, wrap the idbDelete(LOG_STORAGE_KEY) +
window.dispatchEvent(...) logic inside the writeQueue task runner (e.g.
writeQueue.run / writeQueue.enqueue / writeQueue.add depending on the queue API
used) and keep the canUseIndexedDB and withTimeout checks inside that queued
task so the deletion is serialized with other operations.
---
Nitpick comments:
In `@src/components/settings/LogsPanel.tsx`:
- Around line 48-59: The event handler in LogsPanel (inside the useEffect that
calls loadLogs) fires loadLogs on every gsm:diagnostic-log-added and
gsm:diagnostic-logs-cleared causing redundant reads; wrap the reload in a short
debounce: replace the inline handleChange with a debounced version (e.g., useRef
timer or a useDebouncedCallback) that batches rapid events (100–300ms) and only
calls loadLogs once per burst, ensure the cleanup clears the timer and removes
the same debounced handler from window events so loadLogs and the event
listeners in the useEffect still reference the debounced function.
🪄 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: 6766fe8c-abf7-482a-be40-5c7f5ee13482
📒 Files selected for processing (9)
src/components/SettingsPanel.tsxsrc/components/settings/LogsPanel.tsxsrc/components/settings/index.tssrc/services/aiAnalysisHelper.tssrc/services/aiService.tssrc/services/appLogger.tssrc/services/backendAdapter.tssrc/services/githubApi.tssrc/types/index.ts
Comment on lines
+182
to
+185
| {filteredLogs.length === 0 ? ( | ||
| <div className="p-8 text-center text-sm text-gray-500 dark:text-text-tertiary"> | ||
| {t('暂无日志', 'No logs yet')} | ||
| </div> |
Contributor
There was a problem hiding this comment.
Distinguish "no logs" from "no matching logs".
When logs.length > 0 but filters/search exclude everything, the panel still shows "No logs yet", implying all logs are gone. Branch the message on logs.length.
💡 Proposed fix
{filteredLogs.length === 0 ? (
<div className="p-8 text-center text-sm text-gray-500 dark:text-text-tertiary">
- {t('暂无日志', 'No logs yet')}
+ {logs.length === 0
+ ? t('暂无日志', 'No logs yet')
+ : t('没有符合条件的日志', 'No logs match the current filters')}
</div>🤖 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/LogsPanel.tsx` around lines 182 - 185, The current
JSX always shows "No logs yet" when filteredLogs.length === 0; change it to
differentiate the case where there are logs but none match the current
filters/search. In the LogsPanel component, replace the single conditional
rendering that checks filteredLogs.length with a nested check: if logs.length
=== 0 render the existing "No logs yet" message, otherwise render a distinct
message like "No matching logs" (or "No logs match your filters") so users know
filters are excluding results; update the branch that references filteredLogs
and logs accordingly in the component's render/return.
Comment on lines
+200
to
+204
| {entry.success !== undefined && ( | ||
| entry.success | ||
| ? <CheckCircle className="w-4 h-4 text-green-600 dark:text-green-400" /> | ||
| : <AlertCircle className="w-4 h-4 text-red-600 dark:text-red-400" /> | ||
| )} |
Contributor
There was a problem hiding this comment.
Add accessible names to status icons.
Success/failure is conveyed only by the CheckCircle/AlertCircle icon shape and color, so screen-reader users get no status. Add role="img" + aria-label.
♿ Proposed fix
{entry.success !== undefined && (
entry.success
- ? <CheckCircle className="w-4 h-4 text-green-600 dark:text-green-400" />
- : <AlertCircle className="w-4 h-4 text-red-600 dark:text-red-400" />
+ ? <CheckCircle role="img" aria-label={t('成功', 'Success')} className="w-4 h-4 text-green-600 dark:text-green-400" />
+ : <AlertCircle role="img" aria-label={t('失败', 'Failed')} className="w-4 h-4 text-red-600 dark:text-red-400" />
)}📝 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
| {entry.success !== undefined && ( | |
| entry.success | |
| ? <CheckCircle className="w-4 h-4 text-green-600 dark:text-green-400" /> | |
| : <AlertCircle className="w-4 h-4 text-red-600 dark:text-red-400" /> | |
| )} | |
| {entry.success !== undefined && ( | |
| entry.success | |
| ? <CheckCircle role="img" aria-label={t('成功', 'Success')} className="w-4 h-4 text-green-600 dark:text-green-400" /> | |
| : <AlertCircle role="img" aria-label={t('失败', 'Failed')} className="w-4 h-4 text-red-600 dark:text-red-400" /> | |
| )} |
🤖 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/LogsPanel.tsx` around lines 200 - 204, The
CheckCircle/AlertCircle icons in LogsPanel.tsx convey status only visually;
update the JSX that renders CheckCircle and AlertCircle (the block using
entry.success) to add role="img" and a meaningful aria-label (e.g.,
aria-label={`Log entry ${entry.success ? 'succeeded' : 'failed'}`} or similar)
so screen readers announce status; keep the existing classNames and conditional
logic but ensure the aria-label reflects the success boolean.
Comment on lines
+331
to
+340
| appLogger.error('ai', 'request', 'AI request failed', { | ||
| success: false, | ||
| durationMs: Date.now() - startedAt, | ||
| details: { | ||
| apiType, | ||
| model: this.config.model, | ||
| provider: backend.isAvailable ? 'backend-proxy' : 'direct', | ||
| requestChars, | ||
| error: appLogger.serializeError(error), | ||
| }, |
Contributor
There was a problem hiding this comment.
Don't persist raw upstream AI error bodies.
This log stores serializeError(error) verbatim, but the errors thrown earlier in requestText() already append the provider/backend response body. A validation failure can therefore end up writing prompt fragments or other provider-returned content into IndexedDB.
Suggested fix
} catch (error) {
+ const rawError = appLogger.serializeError(error);
+ const safeError = rawError.includes(' - ') ? rawError.split(' - ')[0] : rawError;
+
appLogger.error('ai', 'request', 'AI request failed', {
success: false,
durationMs: Date.now() - startedAt,
details: {
apiType,
model: this.config.model,
provider: backend.isAvailable ? 'backend-proxy' : 'direct',
requestChars,
- error: appLogger.serializeError(error),
+ error: safeError,
},
});
throw error;
}📝 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
| appLogger.error('ai', 'request', 'AI request failed', { | |
| success: false, | |
| durationMs: Date.now() - startedAt, | |
| details: { | |
| apiType, | |
| model: this.config.model, | |
| provider: backend.isAvailable ? 'backend-proxy' : 'direct', | |
| requestChars, | |
| error: appLogger.serializeError(error), | |
| }, | |
| } catch (error) { | |
| const rawError = appLogger.serializeError(error); | |
| const safeError = rawError.includes(' - ') ? rawError.split(' - ')[0] : rawError; | |
| appLogger.error('ai', 'request', 'AI request failed', { | |
| success: false, | |
| durationMs: Date.now() - startedAt, | |
| details: { | |
| apiType, | |
| model: this.config.model, | |
| provider: backend.isAvailable ? 'backend-proxy' : 'direct', | |
| requestChars, | |
| error: safeError, | |
| }, | |
| }); | |
| throw 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 `@src/services/aiService.ts` around lines 331 - 340, The log call in
aiService.ts is persisting raw upstream error bodies via
appLogger.serializeError(error); update the error logging in the AI request
failure path (the appLogger.error call inside requestText()/request handling) to
remove or redact provider response bodies before serialization: extract error
metadata you need (status/code/message) but strip any response.body,
response.data, prompt, or text fields added to the error object by the
provider/backend, then pass the sanitized error to appLogger.serializeError (or
log a constructed sanitizedError object) so no raw provider response content is
persisted.
Comment on lines
+209
to
+214
| async clearLogs(): Promise<void> { | ||
| if (typeof window === 'undefined') return; | ||
| if (canUseIndexedDB()) { | ||
| await withTimeout(idbDelete(LOG_STORAGE_KEY)); | ||
| } | ||
| window.dispatchEvent(new CustomEvent('gsm:diagnostic-logs-cleared')); |
Contributor
There was a problem hiding this comment.
Serialize clearLogs() through writeQueue.
clearLogs() deletes IndexedDB immediately, but any write already queued in writeQueue can still run afterward and repopulate the store. That makes “Clear” nondeterministic and can leave supposedly deleted entries visible again.
Suggested fix
async clearLogs(): Promise<void> {
if (typeof window === 'undefined') return;
- if (canUseIndexedDB()) {
- await withTimeout(idbDelete(LOG_STORAGE_KEY));
- }
- window.dispatchEvent(new CustomEvent('gsm:diagnostic-logs-cleared'));
+ writeQueue = writeQueue.then(async () => {
+ if (canUseIndexedDB()) {
+ await withTimeout(idbDelete(LOG_STORAGE_KEY));
+ }
+ window.dispatchEvent(new CustomEvent('gsm:diagnostic-logs-cleared'));
+ });
+
+ await writeQueue;
},🤖 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/appLogger.ts` around lines 209 - 214, clearLogs() currently
deletes IndexedDB immediately which races with pending writes; change it to run
via the existing writeQueue so the delete executes only after prior queued
writes and prevents new writes from running concurrently. Concretely, wrap the
idbDelete(LOG_STORAGE_KEY) + window.dispatchEvent(...) logic inside the
writeQueue task runner (e.g. writeQueue.run / writeQueue.enqueue /
writeQueue.add depending on the queue API used) and keep the canUseIndexedDB and
withTimeout checks inside that queued task so the deletion is serialized with
other operations.
Owner
|
重复了兄弟，我也在弄 |
Contributor
Author
那很不巧了LOL |
Owner
你那个UI回头我可以抄下，我现在只做了导出。主要是方便定位用户遇到的BUG |
Contributor
Author
|
你直接改就行，我勾选了你能直接编辑的权限 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概述
隐私与安全
验证
npm run build通过，本次改动不涉及 Electron 主进程或打包配置。git diff --check通过。Summary by CodeRabbit