refactor: add lint and tscheck to git commit hook #660
Conversation
WalkthroughThis set of changes updates module import paths to include explicit Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant Git
participant verify-commit.js
participant simple-git-hooks
Developer->>Git: Commit changes
Git->>simple-git-hooks: Trigger commit-msg hook
simple-git-hooks->>verify-commit.js: Run script
verify-commit.js->>Git: Read commit message
verify-commit.js-->>simple-git-hooks: Pass/fail based on regex
simple-git-hooks-->>Developer: Allow or block commit
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
src/main/presenter/mcpPresenter/inMemoryServers/powerpackServer.ts (1)
55-72: Regex blacklist is porous – arbitrary code execution still possibleFiltering user-supplied code with a small set of
RegExppatterns is not a reliable sandbox.
Examples that currently bypass the filter:process['ex' + 'it']() // split string bypass import('fs').then(r => r.readFileSync('/etc/passwd')) require?.('fs') // optional-chainingConsider:
- Running code inside
vm2/isolated-vmor an external sandbox only.- Dropping local execution entirely when E2B is disabled.
- If you must keep a blacklist, complement it with an allow-list AST walk (e.g.
acorn) instead of regex.Failing to address this leaves the host process fully compromise-able.
src/main/presenter/mcpPresenter/inMemoryServers/deepResearchServer.ts (1)
330-343: Extensive console output in production pathLines 330-343 log raw errors to stdout. Replace with structured logger and include severity level per guideline (
ERROR,WARN…). Avoid leaking sensitive data fromaxioserrors.
♻️ Duplicate comments (4)
src/main/presenter/mcpPresenter/inMemoryServers/filesystem.ts (1)
10-10: Same deep import concern as abovePlease confirm that the
.jsfile truly exists inside the package; otherwise TypeScript path mapping may compile but the app will 404 at runtime.src/main/presenter/mcpPresenter/inMemoryServers/difyKnowledgeServer.ts (1)
5-5: Ensure Transport sub-path with.jsis exportedSame reasoning: validate the path and prefer the package’s public export if available.
src/main/presenter/mcpPresenter/inMemoryServers/conversationSearchServer.ts (1)
6-6: Deep path import – verify availabilityRe-validate that
@modelcontextprotocol/sdk/shared/transport.jsexists in production builds.src/main/presenter/mcpPresenter/inMemoryServers/meetingServer.ts (1)
7-7: Deep path import – verify availabilitySame note as previous files.
🧹 Nitpick comments (11)
src/main/presenter/llmProviderPresenter/providers/openAICompatibleProvider.ts (1)
34-45: Duplicate entry and hard-coded list breaks DRY
'o1-pro'appears twice and the exact same constant is now copied into two files.
- The duplicate is noise – it does nothing functionally but makes the list harder to scan.
- Maintaining two diverging copies will cause the lists to drift.
const OPENAI_REASONING_MODELS = [ 'o4-mini', 'o1-pro', 'o3', 'o3-pro', 'o3-mini', 'o3-preview', 'o1-mini', - 'o1-pro', + // 'o1-pro', // duplicate removed 'o1-preview', 'o1' ]Follow-up: move this array to a shared
constants.tsand import it in both providers to stay DRY.src/main/presenter/llmProviderPresenter/providers/openAIResponsesProvider.ts (1)
24-35: Same duplication here – extract to shared constantIdentical issues as the other provider:
'o1-pro'is duplicated.- The list is copy-pasted; any future edit risks inconsistency.
Keep a single source of truth and remove the duplicate element:
const OPENAI_REASONING_MODELS = [ 'o4-mini', 'o1-pro', 'o3', 'o3-pro', 'o3-mini', 'o3-preview', 'o1-mini', - 'o1-pro', 'o1-preview', 'o1' ]Suggest exporting the list from a shared module (e.g.
src/main/constants/openai.ts).src/main/presenter/mcpPresenter/inMemoryServers/artifactsServer.ts (1)
7-34: Non-English comments violate project guidelineGuideline
**/*.{ts,tsx,js,jsx,vue}→ “Use English for logs and comments”.
Lines 7-34 (and many others below) are Chinese. Convert to English to keep consistency and help non-Chinese contributors.src/main/presenter/mcpPresenter/inMemoryServers/powerpackServer.ts (1)
153-162: Log messages are not in EnglishGuideline: “Use English for logs and comments”.
console.warn/infohere are still in Chinese which breaks consistency.- console.warn('运行时未找到，无法执行代码') + console.warn('No runtime found (Bun / Node.js / E2B), code execution disabled')Please sweep the file for other occurrences.
src/main/presenter/mcpPresenter/inMemoryServers/builtinKnowledgeServer.ts (1)
24-31: Chinese error strings – switch to English for consistencyExample:
throw new Error('需要提供Builtin知识库配置')Update all thrown / logged strings in this file to English per project guideline.
src/main/presenter/mcpPresenter/inMemoryServers/imageServer.ts (1)
258-261: Potentially large base64 payload returned as single text chunkReturning multi-MB base64 strings in one MCP text content may hit websocket/frame limits and blows up memory.
Consider:
- Splitting into
tool_call_chunkevents, or- Returning an object
{ path, size, base64: <truncated>, downloadUrl }.This will scale better for large images.
src/main/presenter/mcpPresenter/inMemoryServers/autoPromptingServer.ts (1)
132-140: Non-English log / error message
'Unable to retrieve the list of template names.'is English, but many other messages around this block remain Chinese. Please unify to English.src/main/presenter/mcpPresenter/inMemoryServers/deepResearchServer.ts (1)
55-85: Hard-coded multi-line prompt inside source fileStoring the 30-line default prompt in code hurts maintainability and i18n.
Move it to:
- a
.mdasset file, orconfigPresenter.Load at runtime to keep the TS source lean.
electron.vite.config.ts (1)
63-63: Consider using @ts-expect-error with explanationUsing
@ts-expect-errorinstead of@ts-ignoreis preferred as it ensures there's actually a type error to suppress. Also consider adding a brief explanation of why the suppression is needed.- // @ts-ignore + // @ts-expect-error - PostCSS plugin types may not align perfectly with Vite's expectationsscripts/verify-commit.js (1)
1-29: LGTM: Well-implemented commit message validationThis script effectively enforces conventional commit standards with:
- Comprehensive regex pattern covering all standard commit types
- Clear, helpful error messages with examples
- Proper integration with git hooks
- Good use of colors for better user experience
Consider adding error handling for the file read operation:
-const msg = readFileSync(msgPath, 'utf-8').trim() +let msg +try { + msg = readFileSync(msgPath, 'utf-8').trim() +} catch (error) { + console.error(`Error reading commit message: ${error.message}`) + process.exit(1) +}package.json (1)
167-170: Pre-commit hook executes full type-check on every commit – consider scoping or caching
Runningpnpm typecheck(~30-60 s in this repo) for every commit hampers velocity. Options:
• Use--incremental/--cacheand only check changed packages.
• Gate the heavy check behind an env flag (SKIP_TYPECHECK).
Summary by CodeRabbit
New Features
Chores
Bug Fixes
Refactor
Style
Revert