Retain today's runtime logs after restart#176
Closed
Eldon27232 wants to merge 7 commits intoMistEO:mainfrom
Closed
Retain today's runtime logs after restart#176Eldon27232 wants to merge 7 commits intoMistEO:mainfrom
Eldon27232 wants to merge 7 commits intoMistEO:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Hey - 我发现了两个问题,并留下了一些总体反馈:
- 在每次追加日志、清空日志以及更改 maxLogsPerInstance 时都持久化到 localStorage,可能会对日志特别频繁的实例造成不必要的 I/O 和 UI 卡顿;建议对 persistRuntimeLogs 做节流/防抖,或对更新进行批处理(例如基于时间间隔,或使用 requestIdleCallback)。
- runtimeLogPersistence.ts 引入了自己的 DEFAULT_MAX_LOGS_PER_INSTANCE,并且在清除单个实例日志时也使用了该默认值;为避免配置偏移以及意外增加保留日志数量,建议复用现有的共享常量,和/或将当前的 maxLogsPerInstance 传递到 clearPersistedRuntimeLogs,而不是将 2000 写死。
供 AI Agents 使用的提示(Prompt)
Please address the comments from this code review:
## Overall Comments
- Persisting to localStorage on every log append, log clear, and maxLogsPerInstance change may cause unnecessary I/O and UI jank for chatty instances; consider throttling/debouncing persistRuntimeLogs or batching updates (e.g., on an interval or using requestIdleCallback).
- runtimeLogPersistence.ts introduces its own DEFAULT_MAX_LOGS_PER_INSTANCE and also uses that default when clearing a single instance’s logs; to avoid config drift and unexpected expansion of retained logs, consider reusing the existing shared constant and/or threading the current maxLogsPerInstance into clearPersistedRuntimeLogs instead of hardcoding 2000.
## Individual Comments
### Comment 1
<location path="src/utils/runtimeLogPersistence.ts" line_range="133" />
<code_context>
+ ),
+ };
+
+ window.localStorage.setItem(RUNTIME_LOG_STORAGE_KEY, JSON.stringify(payload));
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Wrap localStorage.setItem in a try/catch to avoid runtime failures (e.g. quota issues).
Unlike loadPersistedRuntimeLogs, this write path can still throw if localStorage is full, disabled, or otherwise restricted (QuotaExceededError, SecurityError, etc.). Please wrap setItem in a try/catch and either no-op or clear the key to prevent hard failures on each log append when retention is enabled.
</issue_to_address>
### Comment 2
<location path="src/utils/runtimeLogPersistence.ts" line_range="144-153" />
<code_context>
+ return;
+ }
+
+ const existing = loadPersistedRuntimeLogs(DEFAULT_MAX_LOGS_PER_INSTANCE);
+ if (!(instanceId in existing)) return;
+
+ const { [instanceId]: _, ...rest } = existing;
+ if (Object.keys(rest).length === 0) {
+ window.localStorage.removeItem(RUNTIME_LOG_STORAGE_KEY);
+ return;
+ }
+
+ persistRuntimeLogs(rest, DEFAULT_MAX_LOGS_PER_INSTANCE);
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using a hardcoded DEFAULT_MAX_LOGS_PER_INSTANCE when clearing a single instance can diverge from the user-configured limit.
This path reloads and re-persists logs using DEFAULT_MAX_LOGS_PER_INSTANCE rather than the current maxLogsPerInstance. For users with a custom limit, the persisted logs may no longer match in-memory behavior (either truncated or over-retained). Consider passing the active limit into this function (or storing it with the payload) so persistence respects the configured limit.
Suggested implementation:
```typescript
export function clearPersistedRuntimeLogs(
instanceId?: string,
maxLogsPerInstance: number = DEFAULT_MAX_LOGS_PER_INSTANCE,
): void {
if (!canUseLocalStorage()) return;
if (!instanceId) {
window.localStorage.removeItem(RUNTIME_LOG_STORAGE_KEY);
return;
}
const existing = loadPersistedRuntimeLogs(maxLogsPerInstance);
if (!(instanceId in existing)) return;
const { [instanceId]: _, ...rest } = existing;
if (Object.keys(rest).length === 0) {
window.localStorage.removeItem(RUNTIME_LOG_STORAGE_KEY);
return;
}
persistRuntimeLogs(rest, maxLogsPerInstance);
}
const RUNTIME_LOG_STORAGE_KEY = 'mxu.runtime-logs.v1';
```
To fully respect the user-configured limit:
1. Update all call sites of `clearPersistedRuntimeLogs` to pass the active `maxLogsPerInstance` value used when logging (e.g., `clearPersistedRuntimeLogs(instanceId, maxLogsPerInstance)`).
2. If there are contexts where the configured limit is not available, decide whether using the default is acceptable there or if the limit should be threaded through those call paths as well.帮我变得更有用!请对每条评论点 👍 或 👎,我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- Persisting to localStorage on every log append, log clear, and maxLogsPerInstance change may cause unnecessary I/O and UI jank for chatty instances; consider throttling/debouncing persistRuntimeLogs or batching updates (e.g., on an interval or using requestIdleCallback).
- runtimeLogPersistence.ts introduces its own DEFAULT_MAX_LOGS_PER_INSTANCE and also uses that default when clearing a single instance’s logs; to avoid config drift and unexpected expansion of retained logs, consider reusing the existing shared constant and/or threading the current maxLogsPerInstance into clearPersistedRuntimeLogs instead of hardcoding 2000.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Persisting to localStorage on every log append, log clear, and maxLogsPerInstance change may cause unnecessary I/O and UI jank for chatty instances; consider throttling/debouncing persistRuntimeLogs or batching updates (e.g., on an interval or using requestIdleCallback).
- runtimeLogPersistence.ts introduces its own DEFAULT_MAX_LOGS_PER_INSTANCE and also uses that default when clearing a single instance’s logs; to avoid config drift and unexpected expansion of retained logs, consider reusing the existing shared constant and/or threading the current maxLogsPerInstance into clearPersistedRuntimeLogs instead of hardcoding 2000.
## Individual Comments
### Comment 1
<location path="src/utils/runtimeLogPersistence.ts" line_range="133" />
<code_context>
+ ),
+ };
+
+ window.localStorage.setItem(RUNTIME_LOG_STORAGE_KEY, JSON.stringify(payload));
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Wrap localStorage.setItem in a try/catch to avoid runtime failures (e.g. quota issues).
Unlike loadPersistedRuntimeLogs, this write path can still throw if localStorage is full, disabled, or otherwise restricted (QuotaExceededError, SecurityError, etc.). Please wrap setItem in a try/catch and either no-op or clear the key to prevent hard failures on each log append when retention is enabled.
</issue_to_address>
### Comment 2
<location path="src/utils/runtimeLogPersistence.ts" line_range="144-153" />
<code_context>
+ return;
+ }
+
+ const existing = loadPersistedRuntimeLogs(DEFAULT_MAX_LOGS_PER_INSTANCE);
+ if (!(instanceId in existing)) return;
+
+ const { [instanceId]: _, ...rest } = existing;
+ if (Object.keys(rest).length === 0) {
+ window.localStorage.removeItem(RUNTIME_LOG_STORAGE_KEY);
+ return;
+ }
+
+ persistRuntimeLogs(rest, DEFAULT_MAX_LOGS_PER_INSTANCE);
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using a hardcoded DEFAULT_MAX_LOGS_PER_INSTANCE when clearing a single instance can diverge from the user-configured limit.
This path reloads and re-persists logs using DEFAULT_MAX_LOGS_PER_INSTANCE rather than the current maxLogsPerInstance. For users with a custom limit, the persisted logs may no longer match in-memory behavior (either truncated or over-retained). Consider passing the active limit into this function (or storing it with the payload) so persistence respects the configured limit.
Suggested implementation:
```typescript
export function clearPersistedRuntimeLogs(
instanceId?: string,
maxLogsPerInstance: number = DEFAULT_MAX_LOGS_PER_INSTANCE,
): void {
if (!canUseLocalStorage()) return;
if (!instanceId) {
window.localStorage.removeItem(RUNTIME_LOG_STORAGE_KEY);
return;
}
const existing = loadPersistedRuntimeLogs(maxLogsPerInstance);
if (!(instanceId in existing)) return;
const { [instanceId]: _, ...rest } = existing;
if (Object.keys(rest).length === 0) {
window.localStorage.removeItem(RUNTIME_LOG_STORAGE_KEY);
return;
}
persistRuntimeLogs(rest, maxLogsPerInstance);
}
const RUNTIME_LOG_STORAGE_KEY = 'mxu.runtime-logs.v1';
```
To fully respect the user-configured limit:
1. Update all call sites of `clearPersistedRuntimeLogs` to pass the active `maxLogsPerInstance` value used when logging (e.g., `clearPersistedRuntimeLogs(instanceId, maxLogsPerInstance)`).
2. If there are contexts where the configured limit is not available, decide whether using the default is acceptable there or if the limit should be threaded through those call paths as well.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
|
这个功能不好,我更倾向于日志全部持久化(不分天数),然后右上角那个“垃圾桶”的清理按钮,旁边加一个“每次启动自动清理”,默认自动清理 |
Author
|
已由 #179 替代 |
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
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
Why
This change is for MaaEnd issue: 希望允许保留一天日志 #2138.
MXU already keeps file logs on disk, but the runtime log panel itself lost the current-day context after restart.
Impact
Users can opt in from Settings -> Debug to keep today's runtime logs visible after restarting MXU, while older logs are still dropped automatically.
Validation
Summary by Sourcery
添加一个可选设置,用于在应用重启后持久化并恢复当日的运行时日志,以便在重新打开应用时日志面板仍能保留上下文。
新功能:
增强:
文档:
Original summary in English
Summary by Sourcery
Add an optional setting to persist and restore same-day runtime logs across app restarts so the logs panel retains context after reopening the app.
New Features:
Enhancements:
Documentation: