feat: Implement plugin settings surface isolation for Feishu integration#1613
Merged
Conversation
- Added functionality to show plugin settings action for disabled plugins with settings contributions. - Updated tests to verify that plugin-owned MCP servers are hidden from the global settings list. - Created a new MCP server for Feishu with appropriate configuration and error handling. - Introduced a skill contribution for the Feishu plugin to guide tool usage. - Developed settings UI for Feishu plugin, including credential management and MCP preset selection. - Added regression tests to ensure proper behavior of plugin settings and MCP server status.
Contributor
📝 WalkthroughWalkthroughAdds a Feishu/Lark plugin: manifest, MCP bootstrap (credential-based or warning fallback), settings UI and preload types, presenter changes for plugin-local config and discovery/sync, settings contribution resolution with filesystem validation, renderer filtering of plugin-owned MCP servers, related tests, and planning/spec docs. ChangesFeishu Plugin Settings and Skill Guidance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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: 7
🤖 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 `@plugins/feishu/mcp/serve.mjs`:
- Around line 155-168: The npx invocation uses an unversioned package which is
non-deterministic: update the args array where '@larksuiteoapi/lark-mcp' is
added to pin a specific version (e.g. '@larksuiteoapi/lark-mcp@0.5.1') or read
the package/token from a lockfile/config instead; also remove or make the
hardcoded registry fallback configurable by changing where npm_config_registry
is set in the spawn env (currently spreading process.env and defaulting to
'https://registry.npmmirror.com')—either delete that default, read a
REGISTRY_OVERRIDE env var, or document/centralize the fallback so the spawn(...)
call and args behave reproducibly and without an implicit hardcoded registry.
In `@plugins/feishu/plugin.json`:
- Line 25: The plugin configuration currently uses a blanket auto-approval
policy ("autoApprove": ["all"]) in plugin.json; change this to a safer default
by removing or emptying autoApprove or replacing ["all"] with a narrowly-scoped
allowlist of specific principals or scopes (e.g., [] or
["<specific-team-or-scope>"]) so writes require explicit approval, and ensure
any consumer of the autoApprove key (plugin loader/validation code) handles the
new value correctly.
In `@plugins/feishu/settings/assets/index.js`:
- Around line 39-56: In refreshStatus(), when handling the various mcp states in
the getStatus response (function refreshStatus, local variable mcp, UI nodes
mcpStateNode and message via setMessage), ensure any stale error message is
cleared whenever the MCP is healthy or there is no lastError: specifically, call
the message-clearing action (e.g., setMessage('') or the equivalent clear
function) in the branches where mcp is not found, where mcp.running is true,
where mcp.enabled is true (Stopped), and where mcp has no lastError; only leave
the error message set in the branch that handles mcp.lastError.
- Around line 58-81: The save click handler (the async callback attached to
document.getElementById('save')?.addEventListener('click', ...) that calls
getPluginApi().invokeAction('config.set', ...)) can throw and should be wrapped
in try/catch; modify the handler to wrap the await
getPluginApi().invokeAction(...) call in a try block, set an error message via
setMessage(...) in the catch (including the caught error message), and ensure
any in-progress UI state (e.g., 'Saving...') is cleared or updated in a finally
block so the page never remains stuck on an unresolved state.
In `@src/main/presenter/pluginPresenter/index.ts`:
- Around line 230-235: The handler for the 'config.set' case is persisting any
payload shape (including arrays, null, primitives) to plugin.root/config.json;
add a runtime guard in the 'config.set' branch (around the payload variable used
before JSON.stringify) to ensure payload is a plain non-null object (reject
Array.isArray(payload), typeof payload !== 'object' or payload === null) and
return a clear error result (or throw) rather than writing invalid JSON shapes;
keep using getInstalledOrOfficialPluginOrThrow to locate plugin and only call
fs.writeFileSync when the payload passes this plain-object check.
- Around line 754-763: When discovery rejects a manifest in the
isPluginPlatformSupported or assertTrustedOfficialPlugin checks, also remove or
disable the plugin's persisted installation state so initialize() and
repairMissingPluginResources() don't resurrect MCP servers, settings, or
policies; after the console.warn/continue path for untrusted or the
console.info/continue for unsupported, call the component that manages stored
installations (e.g. invoke a storage removal or mark-disabled API for the plugin
id — implement a helper like removePersistedInstallation(plugin.manifest.id) if
none exists) to delete MCP server entries, settings resources and tool policy
records tied to that manifest id.
🪄 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: 33496493-8130-47d9-bd9b-7d537b4744ac
📒 Files selected for processing (18)
docs/issues/plugin-settings-surface-isolation/plan.mddocs/issues/plugin-settings-surface-isolation/spec.mddocs/issues/plugin-settings-surface-isolation/tasks.mddocs/issues/plugin-skill-tool-guidance/plan.mddocs/issues/plugin-skill-tool-guidance/spec.mddocs/issues/plugin-skill-tool-guidance/tasks.mdplugins/feishu/mcp/serve.mjsplugins/feishu/plugin.jsonplugins/feishu/settings/assets/index.cssplugins/feishu/settings/assets/index.jsplugins/feishu/settings/index.htmlplugins/feishu/skills/feishu-tools/SKILL.mdplugins/feishu/types/settings-preload.d.tssrc/main/presenter/pluginPresenter/index.tssrc/renderer/settings/components/PluginsSettings.vuetest/main/presenter/pluginPresenter.test.tstest/renderer/components/McpServers.test.tstest/renderer/components/PluginsSettings.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 (2)
test/renderer/components/McpServers.test.ts (2)
36-84:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winIntegrate the new
serverListandconfigvariables intomcpStoreinitialization.The
serverListandconfigvariables created on lines 30-35 are never used. ThemcpStoreinitialization still uses the oldoptions.withServersternary logic (lines 39-67), which means the new test cases that pass customserverListandconfigvalues won't work as intended.🔧 Proposed fix to use the new variables
const mcpStore = reactive({ mcpInstallCache: '', clearMcpInstallCache: vi.fn(), - serverList: options.withServers - ? [ - { - name: 'running-server', - icons: '', - descriptions: '', - command: '', - args: [], - enabled: true, - isRunning: true - }, - { - name: 'stopped-server', - icons: '', - descriptions: '', - command: '', - args: [], - enabled: false, - isRunning: false - } - ] - : [], - config: { - mcpServers: options.withServers - ? { - 'running-server': { type: 'stdio' }, - 'stopped-server': { type: 'stdio' } - } - : {} - }, + serverList: serverList.length > 0 + ? serverList + : options.withServers + ? [ + { + name: 'running-server', + icons: '', + descriptions: '', + command: '', + args: [], + enabled: true, + isRunning: true + }, + { + name: 'stopped-server', + icons: '', + descriptions: '', + command: '', + args: [], + enabled: false, + isRunning: false + } + ] + : [], + config: config,🤖 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 `@test/renderer/components/McpServers.test.ts` around lines 36 - 84, The test creates local variables serverList and config but mcpStore still builds serverList and config from options.withServers; update mcpStore initialization to use the new serverList and config variables instead of the options.withServers ternaries (replace the serverList: options.withServers ? [...] : [] and config.mcpServers: options.withServers ? {...} : {} logic with the serverList and config variables), keeping other mocked methods (addServer, updateServer, etc.) unchanged.
17-17:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUpdate function signature to include missing parameters.
The function signature is missing the
serverListandconfigparameters that are passed by the new test cases (lines 167-186, 196-209) and referenced in the function body (lines 30-35). This mismatch will cause the tests to fail.🐛 Proposed fix to add missing parameters
-const setup = async (options: { withServers?: boolean; showFooterAddButton?: boolean } = {}) => { +const setup = async (options: { + withServers?: boolean + showFooterAddButton?: boolean + serverList?: Array<{ name: string; [key: string]: any }> + config?: { mcpServers?: Record<string, any> } +} = {}) => {🤖 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 `@test/renderer/components/McpServers.test.ts` at line 17, The setup helper function signature must accept the additional parameters used by the tests: add serverList and config to the setup parameters so its declaration becomes setup(options: { withServers?: boolean; showFooterAddButton?: boolean; serverList?: ServerType; config?: ConfigType } = {}) (use the actual types used in the tests or widen to any if types are unclear), and update any internal references that read serverList and config inside setup to use options.serverList and options.config so the calls in the tests (lines referencing serverList/config) map correctly to the function arguments.
🧹 Nitpick comments (2)
test/renderer/components/McpServers.test.ts (2)
166-193: ⚡ Quick winEnsure
serverListobjects include all required fields.The
serverListobjects passed to the test only include thenameproperty (line 168), but the existing test setup shows that server objects should include additional fields likeicons,descriptions,command,args,enabled, andisRunning(lines 41-49). While the test may still work if the component doesn't require these fields, including them would make the test more robust and consistent with the rest of the test suite.♻️ Proposed enhancement for test robustness
const { wrapper } = await setup({ - serverList: [{ name: 'feishu-tools' }, { name: 'user-server' }], + serverList: [ + { + name: 'feishu-tools', + icons: '', + descriptions: '', + command: 'node', + args: [], + enabled: true, + isRunning: true + }, + { + name: 'user-server', + icons: '', + descriptions: '', + command: 'node', + args: [], + enabled: true, + isRunning: true + } + ], config: {🤖 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 `@test/renderer/components/McpServers.test.ts` around lines 166 - 193, The test "hides plugin-owned MCP servers from the global settings list" passes serverList items with only name; update the serverList entries in the setup(...) call so each server object includes the full required fields (icons, descriptions, command, args, enabled, isRunning) matching the shape used elsewhere in tests, e.g. provide icons/descriptions strings, command and args arrays, enabled boolean and isRunning boolean for both 'feishu-tools' and 'user-server' so the setup(...) and component logic consume complete server objects.
195-214: ⚡ Quick winEnsure
serverListobjects include all required fields.Similar to the previous test, the
serverListobject only includes thenameproperty (line 197). For consistency and test robustness, it should include all the fields that server objects typically have.♻️ Proposed enhancement for test robustness
const { wrapper } = await setup({ - serverList: [{ name: 'feishu-tools' }], + serverList: [ + { + name: 'feishu-tools', + icons: '', + descriptions: '', + command: 'node', + args: [], + enabled: true, + isRunning: true + } + ], config: {🤖 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 `@test/renderer/components/McpServers.test.ts` around lines 195 - 214, The test "shows the empty state when only plugin-owned MCP servers exist" passes a serverList with only the name; update the serverList entry passed to setup to include the full set of required server fields (e.g., id, type, command, args, enabled, source, ownerPluginId or whatever the app's server shape expects) so the component receives a complete server object; modify the serverList in the setup call in McpServers.test.ts to mirror the same keys used in other tests and in the config.mcpServers entry.
🤖 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 `@test/renderer/components/McpServers.test.ts`:
- Around line 30-35: The test assigns serverList and builds config using a
non-existent variable overrides, causing a ReferenceError; update the references
so serverList = options.serverList ?? [] and use options.config (e.g.,
...(options.config?.mcpServers ?? {})) when building config.mcpServers, ensuring
all occurrences of overrides in this block are replaced with options and
preserving the existing nullish-coalescing behavior for mcpServers and
serverList.
---
Outside diff comments:
In `@test/renderer/components/McpServers.test.ts`:
- Around line 36-84: The test creates local variables serverList and config but
mcpStore still builds serverList and config from options.withServers; update
mcpStore initialization to use the new serverList and config variables instead
of the options.withServers ternaries (replace the serverList:
options.withServers ? [...] : [] and config.mcpServers: options.withServers ?
{...} : {} logic with the serverList and config variables), keeping other mocked
methods (addServer, updateServer, etc.) unchanged.
- Line 17: The setup helper function signature must accept the additional
parameters used by the tests: add serverList and config to the setup parameters
so its declaration becomes setup(options: { withServers?: boolean;
showFooterAddButton?: boolean; serverList?: ServerType; config?: ConfigType } =
{}) (use the actual types used in the tests or widen to any if types are
unclear), and update any internal references that read serverList and config
inside setup to use options.serverList and options.config so the calls in the
tests (lines referencing serverList/config) map correctly to the function
arguments.
---
Nitpick comments:
In `@test/renderer/components/McpServers.test.ts`:
- Around line 166-193: The test "hides plugin-owned MCP servers from the global
settings list" passes serverList items with only name; update the serverList
entries in the setup(...) call so each server object includes the full required
fields (icons, descriptions, command, args, enabled, isRunning) matching the
shape used elsewhere in tests, e.g. provide icons/descriptions strings, command
and args arrays, enabled boolean and isRunning boolean for both 'feishu-tools'
and 'user-server' so the setup(...) and component logic consume complete server
objects.
- Around line 195-214: The test "shows the empty state when only plugin-owned
MCP servers exist" passes a serverList with only the name; update the serverList
entry passed to setup to include the full set of required server fields (e.g.,
id, type, command, args, enabled, source, ownerPluginId or whatever the app's
server shape expects) so the component receives a complete server object; modify
the serverList in the setup call in McpServers.test.ts to mirror the same keys
used in other tests and in the config.mcpServers entry.
🪄 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: cc1bca72-eb7f-468c-8448-a419052c5805
📒 Files selected for processing (2)
src/renderer/settings/components/PluginsSettings.vuetest/renderer/components/McpServers.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/renderer/settings/components/PluginsSettings.vue
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 `@test/renderer/plugins/feishuSettings.test.ts`:
- Around line 29-31: The test uses window.eval to execute the bundled script
(readFile/scriptPath -> window.eval(...)) which triggers Biome's noGlobalEval
rule; update the test to keep window.eval but add a Biome suppression comment
immediately above the window.eval call that explains this is an intentional
test-harness evaluation (e.g., "/* biome-disable-next-line noGlobalEval --
intentional test harness execution of bundled script */") so linting is
satisfied while preserving the runtime behavior.
🪄 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: 438d226c-8e2f-4d94-ad3a-5f4df28d0662
📒 Files selected for processing (9)
docs/issues/plugin-settings-surface-isolation/plan.mddocs/issues/plugin-settings-surface-isolation/spec.mddocs/issues/plugin-settings-surface-isolation/tasks.mdplugins/feishu/mcp/serve.mjsplugins/feishu/plugin.jsonplugins/feishu/settings/assets/index.jssrc/main/presenter/pluginPresenter/index.tstest/main/presenter/pluginPresenter.test.tstest/renderer/plugins/feishuSettings.test.ts
✅ Files skipped from review due to trivial changes (2)
- plugins/feishu/plugin.json
- docs/issues/plugin-settings-surface-isolation/tasks.md
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/feishu/settings/assets/index.js
- test/main/presenter/pluginPresenter.test.ts
- plugins/feishu/mcp/serve.mjs
- src/main/presenter/pluginPresenter/index.ts
Comment on lines
+29
to
+31
| const script = await readFile(scriptPath, 'utf8') | ||
| window.eval(`(() => {\n${script}\n})()`) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the test file
fd -type f -name "feishuSettings.test.ts"Repository: ThinkInAIXYZ/deepchat
Length of output: 236
🏁 Script executed:
# Check if Biome configuration exists
fd -type f -name "biome.json" -o -name ".biome.json" -o -name "biome.jsonc"Repository: ThinkInAIXYZ/deepchat
Length of output: 236
🏁 Script executed:
# Search for window.eval usages in the codebase
rg "window\.eval" --type ts --type tsx -nRepository: ThinkInAIXYZ/deepchat
Length of output: 93
🏁 Script executed:
# Find the test file
find . -name "feishuSettings.test.ts" 2>/dev/nullRepository: ThinkInAIXYZ/deepchat
Length of output: 112
🏁 Script executed:
# Find Biome configuration files
find . -name "biome.json*" -o -name ".biome.json*" 2>/dev/null | head -20Repository: ThinkInAIXYZ/deepchat
Length of output: 78
🏁 Script executed:
# Search for window.eval usages
rg "window\.eval" -nRepository: ThinkInAIXYZ/deepchat
Length of output: 244
🏁 Script executed:
# Read the feishuSettings.test.ts file, focusing on lines 25-35
sed -n '25,35p' test/renderer/plugins/feishuSettings.test.ts | cat -nRepository: ThinkInAIXYZ/deepchat
Length of output: 451
🏁 Script executed:
# Read the Biome configuration
cat biome.jsonRepository: ThinkInAIXYZ/deepchat
Length of output: 1324
🏁 Script executed:
# Check if cuaSettings.test.ts has any suppression comments
sed -n '30,35p' test/renderer/plugins/cuaSettings.test.ts | cat -nRepository: ThinkInAIXYZ/deepchat
Length of output: 261
window.eval violates Biome's noGlobalEval rule and will block linting
Line 30 uses window.eval(...), which is configured as an error in biome.json ("noGlobalEval": "error" in security rules). This will fail CI. Add a suppression comment explaining the test-harness intent:
Fix
const runSettingsScript = async (): Promise<void> => {
const script = await readFile(scriptPath, 'utf8')
+ // biome-ignore lint/security/noGlobalEval: test harness executes plugin browser bundle in jsdom
window.eval(`(() => {\n${script}\n})()`)
}📝 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
| const script = await readFile(scriptPath, 'utf8') | |
| window.eval(`(() => {\n${script}\n})()`) | |
| } | |
| const runSettingsScript = async (): Promise<void> => { | |
| const script = await readFile(scriptPath, 'utf8') | |
| // biome-ignore lint/security/noGlobalEval: test harness executes plugin browser bundle in jsdom | |
| window.eval(`(() => {\n${script}\n})()`) | |
| } |
🧰 Tools
🪛 Biome (2.4.15)
[error] 30-30: eval() exposes to security risks and performance issues.
(lint/security/noGlobalEval)
🤖 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 `@test/renderer/plugins/feishuSettings.test.ts` around lines 29 - 31, The test
uses window.eval to execute the bundled script (readFile/scriptPath ->
window.eval(...)) which triggers Biome's noGlobalEval rule; update the test to
keep window.eval but add a Biome suppression comment immediately above the
window.eval call that explains this is an intentional test-harness evaluation
(e.g., "/* biome-disable-next-line noGlobalEval -- intentional test harness
execution of bundled script */") so linting is satisfied while preserving the
runtime behavior.
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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests