Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
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:
📝 WalkthroughWalkthroughCentralizes agent runtime utilities into lib, adds an import-guard lint step, migrates many type imports to core type modules, introduces new agent runtime helpers (question tool, session paths, system-env prompt builder), refactors renderer display message types, and adds multi-batch agent-cleanup documentation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CI
Participant Package as "package.json (lint)"
Participant Guard as "scripts/agent-cleanup-guard.mjs"
Participant FS as "Repository Files"
Participant Reporter as "Console/CI"
CI->>Package: run lint
Package->>Guard: pnpm run lint:agent-cleanup
Guard->>FS: scan protected directories (collect imports)
FS-->>Guard: file list + import specifiers
Guard->>Guard: classify violations vs ALLOWED_BASELINE
alt unexpected violations found
Guard->>Reporter: print failures / suggestions
Reporter-->>CI: exit non-zero
else only baseline or no violations
Guard->>Reporter: print success summary
Reporter-->>CI: exit zero
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 3
🧹 Nitpick comments (6)
docs/specs/agent-cleanup/spec.md (1)
49-49: Minor grammar nit: consider hyphenating compound adjective.Per static analysis, "primary-flow" should be hyphenated when used as a compound adjective (e.g., "primary-flow type imports").
📝 Proposed fix
-- Narrow new primary-flow type imports. +- Narrow new primary-flow-type imports.Or restructure to: "Narrow type imports in new primary flow."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/agent-cleanup/spec.md` at line 49, Change the phrase "Narrow new primary-flow type imports." to use a hyphenated compound adjective or rephrase; update the sentence to either "Narrow new primary-flow type imports." (ensure "primary-flow" is hyphenated) or reword as "Narrow type imports in new primary flow." in the spec.md content.test/renderer/components/NewThreadPage.test.ts (1)
138-140: Consider removing redundant stubs.The
TooltipProvideris mocked viavi.doMockon line 138-140, but it's also stubbed astruein the global stubs (line 146). While this works, it's redundant. The same applies toChatInputToolbarandChatStatusBar(lines 155-156).♻️ Proposed cleanup
global: { stubs: { - TooltipProvider: true, Button: true, DropdownMenu: true, DropdownMenuTrigger: true, DropdownMenuContent: true, DropdownMenuItem: true, DropdownMenuLabel: true, DropdownMenuSeparator: true, - Icon: true, - ChatInputToolbar: true, - ChatStatusBar: true + Icon: true } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/components/NewThreadPage.test.ts` around lines 138 - 140, Remove redundant component stubs by keeping only one mocking mechanism for TooltipProvider, ChatInputToolbar, and ChatStatusBar: either keep the explicit vi.doMock(...) calls (e.g., vi.doMock('@shadcn/components/ui/tooltip' returning TooltipProvider) and remove their entries from the global stubs object, or delete the vi.doMock calls and leave the boolean/string stubs in the global stubs; ensure references to TooltipProvider, ChatInputToolbar, and ChatStatusBar appear only once (via vi.doMock or via the stubs map) to avoid duplicate mocks.scripts/agent-cleanup-guard.mjs (1)
82-98: Regex patterns may miss some edge cases.The import extraction patterns are generally solid, but note:
- Multiline imports with type modifiers may not be fully captured if they span multiple lines in complex ways.
- The patterns don't handle
export * from '...'syntax (though this may be intentional).If these edge cases matter for your codebase, consider extending the patterns or using a proper parser. For a guardrail script, the current approach is likely sufficient.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/agent-cleanup-guard.mjs` around lines 82 - 98, The extractModuleSpecifiers function's regexes miss some forms like bare side-effect imports (e.g., import 'x'), export-star-from (export * from 'x'), and some multiline/complex type/import combinations; update the patterns array in extractModuleSpecifiers to include a regex for side-effect imports and for export * from '...' and broaden/multiline-enable the existing patterns (or switch to a parser) so module specifiers like "import 'mod'", "export * from 'mod'", and multiline import/export statements with type modifiers are captured; reference the function name extractModuleSpecifiers and the patterns variable when making the change.src/main/lib/agentRuntime/questionTool.ts (1)
42-56: Consider extracting the JSON parsing logic.The try-catch-try-catch pattern for JSON parsing with jsonrepair fallback could be extracted to a reusable helper if this pattern is needed elsewhere in the agent runtime code.
♻️ Optional extraction
function safeParseJson(rawArgs: string): { success: true; data: unknown } | { success: false; error: string } { if (!rawArgs?.trim()) { return { success: true, data: {} } } try { return { success: true, data: JSON.parse(rawArgs) } } catch { try { return { success: true, data: JSON.parse(jsonrepair(rawArgs)) } } catch { return { success: false, error: 'Invalid JSON for question tool arguments.' } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/lib/agentRuntime/questionTool.ts` around lines 42 - 56, The JSON parsing with a jsonrepair fallback in parseQuestionToolArgs is duplicated and should be extracted into a helper to improve reuse and readability; create a helper function (e.g., safeParseJson or tryParseWithRepair) that accepts rawArgs and returns a standardized { success: true; data: unknown } | { success: false; error: string } result, use it inside parseQuestionToolArgs to replace the nested try/catch blocks, and ensure the helper handles empty/whitespace input by returning an empty object result and preserves the same error string ('Invalid JSON for question tool arguments.') on failure so existing callers (parseQuestionToolArgs) keep the same behavior.src/main/lib/agentRuntime/systemEnvPromptBuilder.ts (1)
29-32: Use shared logger instead ofconsole.warnfor consistent observability.You already depend on
@shared/loggerin this module, so switching this catch path tologger.warnkeeps logs structured and centralized.Suggested patch
- } catch (error) { - console.warn( - `[SystemEnvPromptBuilder] Failed to resolve model display name for ${providerId}/${modelId}:`, - error - ) - } + } catch (error) { + logger.warn('[SystemEnvPromptBuilder] Failed to resolve model display name', { + providerId, + modelId, + error + }) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/lib/agentRuntime/systemEnvPromptBuilder.ts` around lines 29 - 32, Replace the console.warn call in the SystemEnvPromptBuilder catch path with the shared logger: import logger from '@shared/logger' if it's not already imported, then call logger.warn with the same message and error so logs remain structured (preserve the interpolated providerId/modelId and the error object); update the catch block where providerId and modelId are referenced to use logger.warn instead of console.warn to maintain consistent observability.src/main/lib/agentRuntime/sessionPaths.ts (1)
26-34: Consider more comprehensive sanitization fortoolCallId.The slash replacement prevents path traversal within the session directory. For additional robustness, consider sanitizing other potentially problematic characters like null bytes (which can truncate paths on some systems) and colons (Windows drive separators).
♻️ Proposed enhancement for broader sanitization
- const safeToolCallId = toolCallId.replace(/[\\/]/g, '_') + const safeToolCallId = toolCallId.replace(/[\\/:\0]/g, '_')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/lib/agentRuntime/sessionPaths.ts` around lines 26 - 34, The current resolveToolOffloadPath uses a simple slash replacement leaving other unsafe characters; update resolveToolOffloadPath to robustly sanitize toolCallId before creating safeToolCallId by (1) stripping or replacing null bytes, colons, backslashes, forward slashes and other control/OS-reserved chars, (2) restricting to a safe whitelist (e.g., alphanumerics, dot, dash, underscore) and collapsing runs of invalid chars, and (3) for inputs that become empty or still risky, fallback to a deterministic hash of toolCallId (or encode it safely) so the filename is always safe; adjust references to safeToolCallId used in resolveToolOffloadPath accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/specs/agent-cleanup/plan.md`:
- Around line 80-91: Remove the duplicated "Batch 1" scope block and the second
"Rollback" statement in the plan to avoid ambiguity: locate the repeated section
that restates Batch 1 (and overlaps with Batch 1A/1B/1C) and delete that
duplicate block so only the original Batch 1A/1B/1C definitions remain, and
remove the extra Rollback line so there's a single clear rollback entry
associated with the original Batch 1 definitions.
In `@docs/specs/agent-cleanup/tasks.md`:
- Around line 21-60: The checklist in docs/specs/agent-cleanup/tasks.md is
stale: entries referencing deepchatAgentPresenter/dispatch.ts (questionTool,
presenter barrel type imports like MCPToolDefinition and SearchResult) were
already migrated in this PR; update the checklist to reflect the migrations by
checking those items or adding a clear "historical pre-migration snapshot" note
next to the listed lines (specifically the lines mentioning
deepchatAgentPresenter/dispatch.ts, questionTool, systemEnvPromptBuilder, and
the MCPToolDefinition/SearchResult imports) so task tracking reflects the
current state and avoids future confusion.
In `@src/main/lib/agentRuntime/sessionPaths.ts`:
- Around line 8-24: The resolveSessionDir function currently treats
conversationId "." as valid because resolvedSessionDir equals sessionsRoot;
update resolveSessionDir to explicitly reject dot-root inputs by returning null
when conversationId.trim() === "." (and optionally also reject ".."), so that a
request for "." does not return the sessions root; refer to symbols
conversationId, sessionsRoot, resolvedSessionDir and add the check early (after
the existing trim/empty check) to return null for ".".
---
Nitpick comments:
In `@docs/specs/agent-cleanup/spec.md`:
- Line 49: Change the phrase "Narrow new primary-flow type imports." to use a
hyphenated compound adjective or rephrase; update the sentence to either "Narrow
new primary-flow type imports." (ensure "primary-flow" is hyphenated) or reword
as "Narrow type imports in new primary flow." in the spec.md content.
In `@scripts/agent-cleanup-guard.mjs`:
- Around line 82-98: The extractModuleSpecifiers function's regexes miss some
forms like bare side-effect imports (e.g., import 'x'), export-star-from (export
* from 'x'), and some multiline/complex type/import combinations; update the
patterns array in extractModuleSpecifiers to include a regex for side-effect
imports and for export * from '...' and broaden/multiline-enable the existing
patterns (or switch to a parser) so module specifiers like "import 'mod'",
"export * from 'mod'", and multiline import/export statements with type
modifiers are captured; reference the function name extractModuleSpecifiers and
the patterns variable when making the change.
In `@src/main/lib/agentRuntime/questionTool.ts`:
- Around line 42-56: The JSON parsing with a jsonrepair fallback in
parseQuestionToolArgs is duplicated and should be extracted into a helper to
improve reuse and readability; create a helper function (e.g., safeParseJson or
tryParseWithRepair) that accepts rawArgs and returns a standardized { success:
true; data: unknown } | { success: false; error: string } result, use it inside
parseQuestionToolArgs to replace the nested try/catch blocks, and ensure the
helper handles empty/whitespace input by returning an empty object result and
preserves the same error string ('Invalid JSON for question tool arguments.') on
failure so existing callers (parseQuestionToolArgs) keep the same behavior.
In `@src/main/lib/agentRuntime/sessionPaths.ts`:
- Around line 26-34: The current resolveToolOffloadPath uses a simple slash
replacement leaving other unsafe characters; update resolveToolOffloadPath to
robustly sanitize toolCallId before creating safeToolCallId by (1) stripping or
replacing null bytes, colons, backslashes, forward slashes and other
control/OS-reserved chars, (2) restricting to a safe whitelist (e.g.,
alphanumerics, dot, dash, underscore) and collapsing runs of invalid chars, and
(3) for inputs that become empty or still risky, fallback to a deterministic
hash of toolCallId (or encode it safely) so the filename is always safe; adjust
references to safeToolCallId used in resolveToolOffloadPath accordingly.
In `@src/main/lib/agentRuntime/systemEnvPromptBuilder.ts`:
- Around line 29-32: Replace the console.warn call in the SystemEnvPromptBuilder
catch path with the shared logger: import logger from '@shared/logger' if it's
not already imported, then call logger.warn with the same message and error so
logs remain structured (preserve the interpolated providerId/modelId and the
error object); update the catch block where providerId and modelId are
referenced to use logger.warn instead of console.warn to maintain consistent
observability.
In `@test/renderer/components/NewThreadPage.test.ts`:
- Around line 138-140: Remove redundant component stubs by keeping only one
mocking mechanism for TooltipProvider, ChatInputToolbar, and ChatStatusBar:
either keep the explicit vi.doMock(...) calls (e.g.,
vi.doMock('@shadcn/components/ui/tooltip' returning TooltipProvider) and remove
their entries from the global stubs object, or delete the vi.doMock calls and
leave the boolean/string stubs in the global stubs; ensure references to
TooltipProvider, ChatInputToolbar, and ChatStatusBar appear only once (via
vi.doMock or via the stubs map) to avoid duplicate mocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84b12514-9d6b-40ef-9778-9c2237be2829
📒 Files selected for processing (34)
docs/specs/agent-cleanup/plan.mddocs/specs/agent-cleanup/spec.mddocs/specs/agent-cleanup/tasks.mdpackage.jsonscripts/agent-cleanup-guard.mjssrc/main/lib/agentRuntime/questionTool.tssrc/main/lib/agentRuntime/sessionPaths.tssrc/main/lib/agentRuntime/systemEnvPromptBuilder.tssrc/main/presenter/agentPresenter/message/systemEnvPromptBuilder.tssrc/main/presenter/agentPresenter/tools/questionTool.tssrc/main/presenter/deepchatAgentPresenter/dispatch.tssrc/main/presenter/deepchatAgentPresenter/index.tssrc/main/presenter/deepchatAgentPresenter/messageStore.tssrc/main/presenter/deepchatAgentPresenter/toolOutputGuard.tssrc/main/presenter/deepchatAgentPresenter/types.tssrc/main/presenter/newAgentPresenter/index.tssrc/main/presenter/newAgentPresenter/legacyImportService.tssrc/main/presenter/sessionPresenter/sessionPaths.tssrc/renderer/src/components/chat-input/composables/useContextLength.tssrc/renderer/src/components/chat/ChatAttachmentItem.vuesrc/renderer/src/components/chat/ChatInputBox.vuesrc/renderer/src/components/chat/composables/useChatInputFiles.tssrc/renderer/src/components/message/MessageBlockContent.vuesrc/renderer/src/components/message/ReferencePreview.vuesrc/renderer/src/pages/ChatPage.vuesrc/renderer/src/pages/NewThreadPage.vuesrc/renderer/src/stores/reference.tssrc/renderer/src/stores/ui/message.tssrc/renderer/src/stores/ui/session.tssrc/shared/types/core/search.tssrc/shared/types/presenters/thread.presenter.d.tstest/main/presenter/deepchatAgentPresenter/deepchatAgentPresenter.test.tstest/renderer/components/ChatPage.test.tstest/renderer/components/NewThreadPage.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/src/pages/ChatPage.vue (1)
199-205:⚠️ Potential issue | 🟡 MinorLocalize the fallback speaker labels.
The fallback
'Assistant'and the new'You'/'Assistant'display names are part of the renderer-facing message model, so hardcoding English here bypasses localization in translated builds. Please source these labels fromvue-i18nkeys instead. As per coding guidelines, "Use vue-i18n keys fromsrc/renderer/src/i18nfor all user-facing strings; never hardcode UI text".Also applies to: 226-243, 266-276
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/pages/ChatPage.vue` around lines 199 - 205, Replace hardcoded English speaker labels with vue-i18n translations: in resolveAssistantModelName use the i18n translation for the assistant fallback (call the i18n translator instead of returning 'Assistant'); similarly update the other spots referenced (the blocks around lines 226-243 and 266-276 that set display names for 'You' / 'Assistant') to use the appropriate i18n keys from the renderer i18n module (use useI18n().t(...) or the shared i18n instance) and return t('...') values instead of hardcoded strings so all user-facing speaker labels use localization keys.
🧹 Nitpick comments (1)
src/renderer/src/pages/ChatPage.vue (1)
154-188: Avoid reparsing the full history on streaming updates.These helpers now sit on the hot path for
toDisplayMessage(), and this file later rebuildsdisplayMessageswhile streaming state changes. In long chats that turns live updates into repeated full-listJSON.parsework. Consider memoizing parsed records byid/updatedAt, or splitting persisted-message decoding from the streaming overlay so static messages are only parsed whenmessageStore.messageschanges.Also applies to: 221-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/pages/ChatPage.vue` around lines 154 - 188, The parseUserMessageContent and parseAssistantMessageContent functions are re-parsing full JSON on every streaming update; add a simple memoization cache (e.g., a module-scoped Map keyed by `${record.id}:${record.updatedAt}`) and have each function first check the cache and return the cached parsed object if present, otherwise JSON.parse, normalize the result into the same shape, store it in the cache, and return it; apply the same caching approach to the other persisted-message decoding code paths referenced in this file (the other parse helpers used by toDisplayMessage) so static messages are only parsed when their id/updatedAt change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/src/components/chat/messageListItems.ts`:
- Line 1: The exported MessageFile type incorrectly uses Date for
metadata.fileCreated/fileModified but the runtime
DisplayUserMessageContent.files payload (hydrated via JSON.parse in
ChatPage.vue) contains JSON-safe strings; update by introducing a
serialized/display file type (e.g., DisplayMessageFile or SerializedMessageFile)
and change DisplayUserMessageContent.files to use that type, or alternatively
normalize the parsed payload in the parsing path to convert
metadata.fileCreated/fileModified strings into Date objects before assigning to
DisplayUserMessageContent.files; update references to MessageFile in
messageListItems.ts and any consumers to use the new serialized/display type or
the normalized shape so type definitions match actual runtime data.
---
Outside diff comments:
In `@src/renderer/src/pages/ChatPage.vue`:
- Around line 199-205: Replace hardcoded English speaker labels with vue-i18n
translations: in resolveAssistantModelName use the i18n translation for the
assistant fallback (call the i18n translator instead of returning 'Assistant');
similarly update the other spots referenced (the blocks around lines 226-243 and
266-276 that set display names for 'You' / 'Assistant') to use the appropriate
i18n keys from the renderer i18n module (use useI18n().t(...) or the shared i18n
instance) and return t('...') values instead of hardcoded strings so all
user-facing speaker labels use localization keys.
---
Nitpick comments:
In `@src/renderer/src/pages/ChatPage.vue`:
- Around line 154-188: The parseUserMessageContent and
parseAssistantMessageContent functions are re-parsing full JSON on every
streaming update; add a simple memoization cache (e.g., a module-scoped Map
keyed by `${record.id}:${record.updatedAt}`) and have each function first check
the cache and return the cached parsed object if present, otherwise JSON.parse,
normalize the result into the same shape, store it in the cache, and return it;
apply the same caching approach to the other persisted-message decoding code
paths referenced in this file (the other parse helpers used by toDisplayMessage)
so static messages are only parsed when their id/updatedAt change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 803bedd2-d51c-4c5a-aeaa-a8dc47d5276a
📒 Files selected for processing (4)
scripts/agent-cleanup-guard.mjssrc/renderer/src/components/chat/MessageList.vuesrc/renderer/src/components/chat/messageListItems.tssrc/renderer/src/pages/ChatPage.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/agent-cleanup-guard.mjs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/agent-cleanup-guard.mjs (1)
146-152: Consider reusing the constant arrays instead of hardcoding paths.The paths here duplicate
MAIN_PROTECTED_DIRSandRENDERER_PROTECTED_DIRS. Using the constants directly would reduce maintenance burden if the protected paths change.♻️ Suggested refactor
async function findViolations() { - const files = [ - ...(await collectFiles(path.join(ROOT, 'src/main/presenter/newAgentPresenter'))), - ...(await collectFiles(path.join(ROOT, 'src/main/presenter/deepchatAgentPresenter'))), - ...(await collectFiles(path.join(ROOT, 'src/renderer/src/pages/ChatPage.vue'))), - ...(await collectFiles(path.join(ROOT, 'src/renderer/src/pages/NewThreadPage.vue'))), - ...(await collectFiles(path.join(ROOT, 'src/renderer/src/stores/ui'))), - ...(await collectFiles(path.join(ROOT, 'src/renderer/src/components/chat'))), - ...(await collectFiles(path.join(ROOT, 'src/renderer/src/components/message'))) - ] + const protectedPaths = [...MAIN_PROTECTED_DIRS, ...RENDERER_PROTECTED_DIRS] + const fileArrays = await Promise.all(protectedPaths.map(collectFiles)) + const files = fileArrays.flat()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/agent-cleanup-guard.mjs` around lines 146 - 152, The duplicated hardcoded protected paths should be replaced by reusing the existing MAIN_PROTECTED_DIRS and RENDERER_PROTECTED_DIRS constants: update the code that calls collectFiles(...) (the spread entries shown) to iterate over MAIN_PROTECTED_DIRS and RENDERER_PROTECTED_DIRS and call collectFiles(path.join(ROOT, dir)) for each dir, then spread the resulting arrays; this keeps the same use of collectFiles and ROOT while removing the duplicated literal paths so changes to MAIN_PROTECTED_DIRS/RENDERER_PROTECTED_DIRS automatically propagate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/agent-cleanup-guard.mjs`:
- Around line 146-152: The duplicated hardcoded protected paths should be
replaced by reusing the existing MAIN_PROTECTED_DIRS and RENDERER_PROTECTED_DIRS
constants: update the code that calls collectFiles(...) (the spread entries
shown) to iterate over MAIN_PROTECTED_DIRS and RENDERER_PROTECTED_DIRS and call
collectFiles(path.join(ROOT, dir)) for each dir, then spread the resulting
arrays; this keeps the same use of collectFiles and ROOT while removing the
duplicated literal paths so changes to
MAIN_PROTECTED_DIRS/RENDERER_PROTECTED_DIRS automatically propagate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d8bee88-6478-4148-928a-90c77b091a31
📒 Files selected for processing (5)
scripts/agent-cleanup-guard.mjssrc/renderer/src/components/message/MessageContent.vuesrc/renderer/src/components/message/MessageItemAssistant.vuesrc/renderer/src/components/message/MessageItemUser.vuetest/renderer/components/message/MessageBlockToolCall.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/renderer/src/components/sidepanel/WorkspacePanel.vue (1)
144-144: Use a storage-facing parse shape (with a runtime guard) instead of a direct display-type cast.
ChatMessageRecord.contentis persisted payload, but Lines 180-182 cast it directly to a display-only type subset. That couples persistence parsing to renderer display typing and can hide payload drift (legacy/unknownstatus, non-array shape). Prefer guarding parsed data and then mapping to the display subset.Suggested hardening
+type ParsedAssistantBlock = { + content?: unknown + status?: unknown +} + +const assistantStatuses = new Set(['success', 'loading', 'cancel', 'error'] as const) + const parseAssistantBlocks = (record: ChatMessageRecord) => { try { - return JSON.parse(record.content) as Array< - Pick<DisplayAssistantMessageBlock, 'content' | 'status'> - > + const raw = JSON.parse(record.content) + if (!Array.isArray(raw)) { + return [] + } + + return raw.flatMap((block) => { + const candidate = block as ParsedAssistantBlock + if ( + typeof candidate?.content !== 'string' || + typeof candidate?.status !== 'string' || + !assistantStatuses.has(candidate.status as (typeof assistantStatuses extends Set<infer T> ? T : never)) + ) { + return [] + } + + return [ + { + content: candidate.content, + status: candidate.status as Pick<DisplayAssistantMessageBlock, 'status'>['status'] + } + ] + }) } catch { return [] } }Also applies to: 178-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/sidepanel/WorkspacePanel.vue` at line 144, The code is casting persisted ChatMessageRecord.content directly to the display-only type DisplayAssistantMessageBlock; instead define a storage-facing parse shape (e.g., ChatMessageContentStorage or a runtime schema) and add a runtime guard/validation function to check that ChatMessageRecord.content is the expected structure (array shape, allowed status values, etc.), then map/transform the validated storage shape to DisplayAssistantMessageBlock before using it in WorkspacePanel.vue (referencing the DisplayAssistantMessageBlock type and the ChatMessageRecord.content usage). Ensure the guard returns a safe default or skips rendering on failure and log or handle unexpected shapes to avoid silent payload drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/renderer/components/message/MessageBlockMedia.test.ts`:
- Around line 211-219: The test reads stale DOM because Vue updates
asynchronously: after calling element.onUIAction (which triggers handleUIAction
in MessageBlockMcpUi.vue and sets errorMessage.value synchronously before
throwing) add an await nextTick() (import nextTick from 'vue' if not already
imported) immediately before asserting wrapper.text() to wait for Vue to
re-render and ensure the error message is present.
---
Nitpick comments:
In `@src/renderer/src/components/sidepanel/WorkspacePanel.vue`:
- Line 144: The code is casting persisted ChatMessageRecord.content directly to
the display-only type DisplayAssistantMessageBlock; instead define a
storage-facing parse shape (e.g., ChatMessageContentStorage or a runtime schema)
and add a runtime guard/validation function to check that
ChatMessageRecord.content is the expected structure (array shape, allowed status
values, etc.), then map/transform the validated storage shape to
DisplayAssistantMessageBlock before using it in WorkspacePanel.vue (referencing
the DisplayAssistantMessageBlock type and the ChatMessageRecord.content usage).
Ensure the guard returns a safe default or skips rendering on failure and log or
handle unexpected shapes to avoid silent payload drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4e994b6-f118-46c6-96d3-0ebf3e5ca1a0
📒 Files selected for processing (18)
scripts/agent-cleanup-guard.mjssrc/renderer/src/components/message/MessageBlockAction.vuesrc/renderer/src/components/message/MessageBlockAudio.vuesrc/renderer/src/components/message/MessageBlockContent.vuesrc/renderer/src/components/message/MessageBlockError.vuesrc/renderer/src/components/message/MessageBlockImage.vuesrc/renderer/src/components/message/MessageBlockMcpUi.vuesrc/renderer/src/components/message/MessageBlockPlan.vuesrc/renderer/src/components/message/MessageBlockQuestionRequest.vuesrc/renderer/src/components/message/MessageBlockThink.vuesrc/renderer/src/components/message/MessageBlockToolCall.vuesrc/renderer/src/components/sidepanel/WorkspacePanel.vuesrc/renderer/src/composables/useArtifacts.tstest/renderer/components/WorkspacePanel.test.tstest/renderer/components/message/MessageBlockBasics.test.tstest/renderer/components/message/MessageBlockContent.test.tstest/renderer/components/message/MessageBlockMedia.test.tstest/renderer/components/message/MessageBlockToolCall.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/agent-cleanup-guard.mjs
- src/renderer/src/components/message/MessageBlockContent.vue
Comment on lines
+211
to
+219
| await expect( | ||
| element.onUIAction?.({ | ||
| type: 'tool', | ||
| payload: {} | ||
| }) | ||
| ).rejects.toThrow('Tool name missing in MCP UI action') | ||
|
|
||
| expect(wrapper.text()).toContain('common.error.requestFailed') | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the test file to understand the context
wc -l test/renderer/components/message/MessageBlockMedia.test.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 127
🏁 Script executed:
# Read the test file around the mentioned lines with broader context
head -n 240 test/renderer/components/message/MessageBlockMedia.test.ts | tail -n +190Repository: ThinkInAIXYZ/deepchat
Length of output: 865
🏁 Script executed:
# Check imports at the top of the file to see if nextTick is available
head -n 30 test/renderer/components/message/MessageBlockMedia.test.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 969
🏁 Script executed:
# Search for other uses of nextTick in this test file
rg "nextTick" test/renderer/components/message/MessageBlockMedia.test.ts -A 2 -B 2Repository: ThinkInAIXYZ/deepchat
Length of output: 422
🏁 Script executed:
# Find the MessageBlockMcpUi component
fd MessageBlockMcpUi.vueRepository: ThinkInAIXYZ/deepchat
Length of output: 122
🏁 Script executed:
# Check the component file for error handling
cat -n src/components/message/MessageBlockMcpUi.vue | head -n 100Repository: ThinkInAIXYZ/deepchat
Length of output: 141
🏁 Script executed:
# Search for how errors are handled in this component and if they require state updates
rg "error" src/components/message/MessageBlockMcpUi.vue -A 3 -B 3Repository: ThinkInAIXYZ/deepchat
Length of output: 149
🏁 Script executed:
# Check for similar test patterns in other message block tests
rg "rejects.toThrow" test/renderer/components/message/ -A 3 -B 3Repository: ThinkInAIXYZ/deepchat
Length of output: 661
🏁 Script executed:
# Read the component file with the correct path
cat -n src/renderer/src/components/message/MessageBlockMcpUi.vue | head -n 150Repository: ThinkInAIXYZ/deepchat
Length of output: 5835
🏁 Script executed:
# Search for error handling in the component
rg "error|requestFailed" src/renderer/src/components/message/MessageBlockMcpUi.vue -A 3 -B 3Repository: ThinkInAIXYZ/deepchat
Length of output: 2119
🏁 Script executed:
# Look at the onUIAction handler logic
rg "onUIAction" src/renderer/src/components/message/MessageBlockMcpUi.vue -A 5 -B 2Repository: ThinkInAIXYZ/deepchat
Length of output: 928
Add await nextTick() before DOM assertion to wait for Vue re-render
When handleUIAction rejects with a missing tool name, it synchronously sets errorMessage.value (line 104 in MessageBlockMcpUi.vue) before throwing. However, Vue's DOM update happens asynchronously. Without flushing a tick, wrapper.text() reads stale DOM that hasn't been re-rendered to reflect the error message yet, causing potential timing flakes.
await expect(
element.onUIAction?.({
type: 'tool',
payload: {}
})
).rejects.toThrow('Tool name missing in MCP UI action')
+ await nextTick()
expect(wrapper.text()).toContain('common.error.requestFailed')📝 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
| await expect( | |
| element.onUIAction?.({ | |
| type: 'tool', | |
| payload: {} | |
| }) | |
| ).rejects.toThrow('Tool name missing in MCP UI action') | |
| expect(wrapper.text()).toContain('common.error.requestFailed') | |
| }) | |
| await expect( | |
| element.onUIAction?.({ | |
| type: 'tool', | |
| payload: {} | |
| }) | |
| ).rejects.toThrow('Tool name missing in MCP UI action') | |
| await nextTick() | |
| expect(wrapper.text()).toContain('common.error.requestFailed') | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/renderer/components/message/MessageBlockMedia.test.ts` around lines 211
- 219, The test reads stale DOM because Vue updates asynchronously: after
calling element.onUIAction (which triggers handleUIAction in
MessageBlockMcpUi.vue and sets errorMessage.value synchronously before throwing)
add an await nextTick() (import nextTick from 'vue' if not already imported)
immediately before asserting wrapper.text() to wait for Vue to re-render and
ensure the error message is present.
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 (1)
docs/FLOWS.md (1)
141-151:⚠️ Potential issue | 🟡 Minor标题与流程已不一致（Minimap 已移除）
这一段流程已经改为
Store -> MessageList直更messageItems，不再包含 Minimap 路径；但本节标题仍是“含 Minimap”。建议同步改标题，避免读者误解。✏️ 建议文档修正
-## 2. 渲染与流式更新流程（含 Minimap） +## 2. 渲染与流式更新流程🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/FLOWS.md` around lines 141 - 151, The section title is inconsistent with the sequence diagram: the flow now goes Store->>MessageList (List) and no longer passes through Minimap, so update the heading that currently mentions "含 Minimap" to reflect the new path (e.g., remove "Minimap" or rename to "Store -> MessageList (no Minimap)"); search for the section containing the sequence diagram and the symbols "List as MessageList (Renderer)", "Store->>List: update messageItems" and change the title text to match the current flow so readers aren’t misled.
♻️ Duplicate comments (1)
docs/specs/agent-cleanup/plan.md (1)
80-90:⚠️ Potential issue | 🟡 MinorRemove the leftover pre-split Batch 1 block.
The block from Line 80 through Line 90 restates Batch 1 immediately after 1A/1B/1C and adds a second rollback path, so the plan is still ambiguous.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/agent-cleanup/plan.md` around lines 80 - 90, Remove the duplicated "Extract shared helpers..." / "Rollback: - Point new-flow imports back to legacy helper modules." block that restates Batch 1 after 1A/1B/1C; edit the plan.md section so only the original Batch 1 content and a single, clear rollback path remain (delete the redundant Block that begins with "Extract shared helpers to neutral modules:" and the subsequent "Rollback:" lines).
🧹 Nitpick comments (2)
test/main/presenter/sqlitePresenter.test.ts (1)
68-108: Cover an existing-row migration case.This currently proves the schema changes, not that version-14
new_sessionsrows survive the migration withactive_skillsbackfilled to a usable default. Seeding one legacy row before constructingSQLitePresenterwould catch that regression.🧪 Suggested expansion
bootstrapDb.exec(` CREATE TABLE IF NOT EXISTS schema_versions ( version INTEGER PRIMARY KEY, applied_at INTEGER NOT NULL ); INSERT INTO schema_versions (version, applied_at) VALUES (14, ${Date.now()}); CREATE TABLE IF NOT EXISTS new_sessions ( id TEXT PRIMARY KEY, agent_id TEXT NOT NULL, title TEXT NOT NULL, project_dir TEXT, is_pinned INTEGER DEFAULT 0, is_draft INTEGER NOT NULL DEFAULT 0, created_at INTEGER NOT NULL, updated_at INTEGER NOT NULL ); + INSERT INTO new_sessions ( + id, agent_id, title, project_dir, is_pinned, is_draft, created_at, updated_at + ) VALUES ('legacy-session', 'deepchat', 'Legacy', NULL, 0, 0, 1, 1); `) @@ const columnNames = new Set(newSessionColumns.map((column) => column.name)) expect(columnNames.has('active_skills')).toBe(true) + + const migratedRow = checkDb + .prepare("SELECT active_skills FROM new_sessions WHERE id = 'legacy-session'") + .get() as { active_skills: string } + expect(migratedRow.active_skills).toBe('[]')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/sqlitePresenter.test.ts` around lines 68 - 108, Test only checks that the schema and schema_versions reached 15, but not that existing version-14 rows in new_sessions are preserved and have active_skills backfilled; modify the test to insert a legacy new_sessions row (without active_skills) into the bootstrap DB before instantiating SQLitePresenter, then after presenter initialization query the row from new_sessions and assert the row still exists and its active_skills column is present and set to the expected default/usable value; reference the test constructs new_sessions, schema_versions, and the SQLitePresenter initialization to locate where to insert the legacy row and where to validate the migrated row.scripts/agent-cleanup-guard.mjs (1)
144-157: Make the protected path lists the scan source of truth.This block duplicates the roots already declared above. If a future PR updates
MAIN_PROTECTED_DIRSorRENDERER_PROTECTED_DIRSbut misses this list, the path is marked protected yet never scanned, so the guard silently loses coverage.♻️ Suggested cleanup
async function findViolations() { - const files = [ - ...(await collectFiles(path.join(ROOT, 'src/main/presenter/newAgentPresenter'))), - ...(await collectFiles(path.join(ROOT, 'src/main/presenter/deepchatAgentPresenter'))), - ...(await collectFiles(path.join(ROOT, 'src/main/presenter/skillPresenter'))), - ...(await collectFiles(path.join(ROOT, 'src/main/presenter/mcpPresenter/toolManager.ts'))), - ...(await collectFiles(path.join(ROOT, 'src/renderer/src/pages/ChatPage.vue'))), - ...(await collectFiles(path.join(ROOT, 'src/renderer/src/pages/NewThreadPage.vue'))), - ...(await collectFiles(path.join(ROOT, 'src/renderer/src/stores/ui'))), - ...(await collectFiles(path.join(ROOT, 'src/renderer/src/components/chat'))), - ...(await collectFiles(path.join(ROOT, 'src/renderer/src/components/message'))), - ...(await collectFiles(path.join(ROOT, 'src/renderer/src/composables/useArtifacts.ts'))), - ...(await collectFiles(path.join(ROOT, 'src/renderer/src/components/sidepanel/WorkspacePanel.vue'))) - ] + const scanRoots = [ + ...MAIN_PROTECTED_DIRS, + ...MAIN_COMPAT_PROTECTED_DIRS, + ...RENDERER_PROTECTED_DIRS + ] + const files = [ + ...new Set((await Promise.all(scanRoots.map((entryPath) => collectFiles(entryPath)))).flat()) + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/agent-cleanup-guard.mjs` around lines 144 - 157, The hard-coded files array in findViolations duplicates the roots defined in MAIN_PROTECTED_DIRS and RENDERER_PROTECTED_DIRS and should be replaced so the protected path lists are the single source of truth; update findViolations to iterate MAIN_PROTECTED_DIRS and RENDERER_PROTECTED_DIRS (and any specific protected file entries like 'mcpPresenter/toolManager.ts' or 'sidepanel/WorkspacePanel.vue' if they are individual protected paths) to call collectFiles(path.join(ROOT, dir)) and flatten the results, ensuring no duplicate manual lists remain and that all protected dirs are scanned consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/agent-cleanup-guard.mjs`:
- Around line 7-17: The extractor currently only matches "import ... from",
"export ... from", and dynamic import() in extractModuleSpecifiers(), but
SOURCE_EXTENSIONS includes .cjs/.cts so you must also detect side-effect imports
(import 'module') and CommonJS require() calls to avoid misses; update
extractModuleSpecifiers() to add regex/AST paths that capture import statements
without specifiers (e.g., ImportDeclaration with no specifiers) and
CallExpressions of require(...) (and require.resolve if desired), and ensure the
same extension filtering logic using SOURCE_EXTENSIONS is applied; reference the
SOURCE_EXTENSIONS set and the extractModuleSpecifiers() function when making the
changes.
In `@src/main/presenter/sqlitePresenter/tables/newSessions.ts`:
- Around line 41-48: The base CREATE TABLE returned by getCreateTableSQL() for
the new_sessions table must be updated to include the new column active_skills
(TEXT NOT NULL DEFAULT '[]') so that when getLatestVersion() returns 15 and the
table is created from scratch it contains that column; modify
getCreateTableSQL() (and the CREATE TABLE SQL string it returns for the latest
version) to add the active_skills column, and keep the existing ALTER TABLE
branch for version === 15 for upgrades so both fresh creates and migrations are
consistent.
---
Outside diff comments:
In `@docs/FLOWS.md`:
- Around line 141-151: The section title is inconsistent with the sequence
diagram: the flow now goes Store->>MessageList (List) and no longer passes
through Minimap, so update the heading that currently mentions "含 Minimap" to
reflect the new path (e.g., remove "Minimap" or rename to "Store -> MessageList
(no Minimap)"); search for the section containing the sequence diagram and the
symbols "List as MessageList (Renderer)", "Store->>List: update messageItems"
and change the title text to match the current flow so readers aren’t misled.
---
Duplicate comments:
In `@docs/specs/agent-cleanup/plan.md`:
- Around line 80-90: Remove the duplicated "Extract shared helpers..." /
"Rollback: - Point new-flow imports back to legacy helper modules." block that
restates Batch 1 after 1A/1B/1C; edit the plan.md section so only the original
Batch 1 content and a single, clear rollback path remain (delete the redundant
Block that begins with "Extract shared helpers to neutral modules:" and the
subsequent "Rollback:" lines).
---
Nitpick comments:
In `@scripts/agent-cleanup-guard.mjs`:
- Around line 144-157: The hard-coded files array in findViolations duplicates
the roots defined in MAIN_PROTECTED_DIRS and RENDERER_PROTECTED_DIRS and should
be replaced so the protected path lists are the single source of truth; update
findViolations to iterate MAIN_PROTECTED_DIRS and RENDERER_PROTECTED_DIRS (and
any specific protected file entries like 'mcpPresenter/toolManager.ts' or
'sidepanel/WorkspacePanel.vue' if they are individual protected paths) to call
collectFiles(path.join(ROOT, dir)) and flatten the results, ensuring no
duplicate manual lists remain and that all protected dirs are scanned
consistently.
In `@test/main/presenter/sqlitePresenter.test.ts`:
- Around line 68-108: Test only checks that the schema and schema_versions
reached 15, but not that existing version-14 rows in new_sessions are preserved
and have active_skills backfilled; modify the test to insert a legacy
new_sessions row (without active_skills) into the bootstrap DB before
instantiating SQLitePresenter, then after presenter initialization query the row
from new_sessions and assert the row still exists and its active_skills column
is present and set to the expected default/usable value; reference the test
constructs new_sessions, schema_versions, and the SQLitePresenter initialization
to locate where to insert the legacy row and where to validate the migrated row.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54be66c6-5060-4680-a52c-ffafbcfd375b
📒 Files selected for processing (27)
archives/code/dead-renderer-batch-1/README.mdarchives/code/dead-renderer-batch-1/src/renderer/src/components/MessageNavigationSidebar.vuearchives/code/dead-renderer-batch-1/src/renderer/src/components/message/MessageMinimap.vuearchives/code/dead-renderer-batch-1/src/renderer/src/composables/message/useMessageMinimap.tsarchives/code/dead-renderer-batch-1/src/renderer/src/lib/messageRuntimeCache.tsdocs/FLOWS.mddocs/specs/agent-cleanup/plan.mddocs/specs/agent-cleanup/spec.mddocs/specs/agent-cleanup/tasks.mdscripts/agent-cleanup-guard.mjssrc/main/presenter/skillPresenter/index.tssrc/main/presenter/sqlitePresenter/tables/newSessions.tssrc/renderer/src/i18n/da-DK/chat.jsonsrc/renderer/src/i18n/en-US/chat.jsonsrc/renderer/src/i18n/fa-IR/chat.jsonsrc/renderer/src/i18n/fr-FR/chat.jsonsrc/renderer/src/i18n/he-IL/chat.jsonsrc/renderer/src/i18n/ja-JP/chat.jsonsrc/renderer/src/i18n/ko-KR/chat.jsonsrc/renderer/src/i18n/pt-BR/chat.jsonsrc/renderer/src/i18n/ru-RU/chat.jsonsrc/renderer/src/i18n/zh-CN/chat.jsonsrc/renderer/src/i18n/zh-HK/chat.jsonsrc/renderer/src/i18n/zh-TW/chat.jsontest/main/presenter/newAgentPresenter/integration.test.tstest/main/presenter/skillPresenter/skillPresenter.test.tstest/main/presenter/sqlitePresenter.test.ts
💤 Files with no reviewable changes (12)
- src/renderer/src/i18n/ko-KR/chat.json
- src/renderer/src/i18n/he-IL/chat.json
- src/renderer/src/i18n/zh-TW/chat.json
- src/renderer/src/i18n/fr-FR/chat.json
- src/renderer/src/i18n/pt-BR/chat.json
- src/renderer/src/i18n/fa-IR/chat.json
- src/renderer/src/i18n/ru-RU/chat.json
- src/renderer/src/i18n/ja-JP/chat.json
- src/renderer/src/i18n/en-US/chat.json
- src/renderer/src/i18n/zh-HK/chat.json
- src/renderer/src/i18n/zh-CN/chat.json
- src/renderer/src/i18n/da-DK/chat.json
✅ Files skipped from review due to trivial changes (1)
- archives/code/dead-renderer-batch-1/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/specs/agent-cleanup/tasks.md
Comment on lines
+7
to
+17
| const SOURCE_EXTENSIONS = new Set([ | ||
| '.js', | ||
| '.jsx', | ||
| '.mjs', | ||
| '.cjs', | ||
| '.ts', | ||
| '.tsx', | ||
| '.mts', | ||
| '.cts', | ||
| '.vue' | ||
| ]) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="scripts/agent-cleanup-guard.mjs"
echo "SOURCE_EXTENSIONS:"
sed -n '7,17p' "$FILE"
echo
echo "extractModuleSpecifiers patterns:"
sed -n '76,82p' "$FILE"
echo
echo "Protected files with side-effect imports or require():"
rg -nP "^\s*import\s*['\"][^'\"]+['\"]\s*;?|require\s*\(\s*['\"][^'\"]+['\"]\s*\)" \
src/main/presenter src/renderer/src || trueRepository: ThinkInAIXYZ/deepchat
Length of output: 941
🏁 Script executed:
# Check protected directories and violation logic
grep -n "PROTECTED_DIRS\|VIOLATION\|ban" scripts/agent-cleanup-guard.mjs | head -30Repository: ThinkInAIXYZ/deepchat
Length of output: 332
🏁 Script executed:
# Get the full context of the protected directories and violation rules
sed -n '1,70p' scripts/agent-cleanup-guard.mjsRepository: ThinkInAIXYZ/deepchat
Length of output: 2187
🏁 Script executed:
sed -n '85,165p' scripts/agent-cleanup-guard.mjsRepository: ThinkInAIXYZ/deepchat
Length of output: 2923
🏁 Script executed:
# Check for side-effect imports and require() in actual protected directories
echo "=== Side-effect imports in protected main dirs ==="
rg -nP "^\s*import\s+['\"][^'\"]+['\"]\s*;?|require\s*\(\s*['\"][^'\"]+['\"]\s*\)" \
src/main/presenter/newAgentPresenter \
src/main/presenter/deepchatAgentPresenter \
src/main/presenter/skillPresenter \
src/main/presenter/mcpPresenter/toolManager.ts \
|| echo "(none found)"
echo ""
echo "=== Side-effect imports in protected renderer dirs ==="
rg -nP "^\s*import\s+['\"][^'\"]+['\"]\s*;?|require\s*\(\s*['\"][^'\"]+['\"]\s*\)" \
src/renderer/src/pages/ChatPage.vue \
src/renderer/src/pages/NewThreadPage.vue \
src/renderer/src/stores/ui \
src/renderer/src/components/chat \
src/renderer/src/components/message \
src/renderer/src/composables/useArtifacts.ts \
src/renderer/src/components/sidepanel/WorkspacePanel.vue \
|| echo "(none found)"
echo ""
echo "=== CommonJS require() anywhere in protected dirs (checking all) ==="
rg -n "require\s*\(" \
src/main/presenter/newAgentPresenter \
src/main/presenter/deepchatAgentPresenter \
src/main/presenter/skillPresenter \
src/main/presenter/mcpPresenter/toolManager.ts \
src/renderer/src/pages/ChatPage.vue \
src/renderer/src/pages/NewThreadPage.vue \
src/renderer/src/stores/ui \
src/renderer/src/components/chat \
src/renderer/src/components/message \
src/renderer/src/composables/useArtifacts.ts \
src/renderer/src/components/sidepanel/WorkspacePanel.vue \
|| echo "(none found)"Repository: ThinkInAIXYZ/deepchat
Length of output: 280
Add side-effect imports and require() to the module specifier extractor.
SOURCE_EXTENSIONS includes .cjs and .cts, but extractModuleSpecifiers() only matches import ... from, export ... from, and dynamic import(). While no side-effect imports or require() calls currently exist in protected directories, the guard should handle these patterns to prevent future violations.
Suggested fix
function extractModuleSpecifiers(source) {
const specifiers = new Set()
const patterns = [
/\bimport\s+(?:type\s+)?[\s\S]*?\bfrom\s*['"]([^'"]+)['"]/g,
+ /\bimport\s*['"]([^'"]+)['"]/g,
/\bexport\s+[\s\S]*?\bfrom\s*['"]([^'"]+)['"]/g,
- /\bimport\s*\(\s*['"]([^'"]+)['"]\s*\)/g
+ /\bimport\s*\(\s*['"]([^'"]+)['"]\s*\)/g,
+ /\brequire\s*\(\s*['"]([^'"]+)['"]\s*\)/g
]Also applies to: 76-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/agent-cleanup-guard.mjs` around lines 7 - 17, The extractor currently
only matches "import ... from", "export ... from", and dynamic import() in
extractModuleSpecifiers(), but SOURCE_EXTENSIONS includes .cjs/.cts so you must
also detect side-effect imports (import 'module') and CommonJS require() calls
to avoid misses; update extractModuleSpecifiers() to add regex/AST paths that
capture import statements without specifiers (e.g., ImportDeclaration with no
specifiers) and CallExpressions of require(...) (and require.resolve if
desired), and ensure the same extension filtering logic using SOURCE_EXTENSIONS
is applied; reference the SOURCE_EXTENSIONS set and the
extractModuleSpecifiers() function when making the changes.
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 by CodeRabbit
Documentation
Build & Tooling
New Features
Improvements
Tests