refactor(mcp): migrate mcp_service/index.js from JavaScript to TypeScript by shayanshafii · Pull Request #22 · COG-GTM/apache-superset

shayanshafii · 2026-06-30T22:00:29Z

SUMMARY

Migrates superset/mcp_service/index.js to TypeScript (index.ts).

Changes:

index.js → index.ts with typed interfaces:
- SupersetMCPServerOptions (input, all fields optional)
- ResolvedOptions (internal, all fields required with defaults applied)
- ChildProcess | null for the process field
tsconfig.json added targeting ES2020/CommonJS with strict mode, outputting to dist/
package.json updated: main → dist/index.js, added types, build/prebuild scripts, typescript + @types/node devDeps
.gitignore added for node_modules/ and dist/
require('./bin/superset-mcp.js') → require('../bin/superset-mcp.js') to resolve correctly from dist/
Uses export = SupersetMCPServer for clean CJS semantics (matching original module.exports)

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — no UI changes.

TESTING INSTRUCTIONS

cd superset/mcp_service
npm install
npm run build        # should compile cleanly
node -e "const S = require('./dist/index.js'); const s = new S({port: 9000}); console.log(typeof s.stop)"
# should print: function

ADDITIONAL INFORMATION

Link to Devin session: https://app.devin.ai/sessions/ef0439e5c59a4eaca3a30a2997df2113
Requested by: @shayanshafii

Devin Review

Status	Commit
⚪ Not started	—

Run Devin Review

…ript - Convert index.js to index.ts with proper interfaces and types - Add SupersetMCPServerOptions and ResolvedOptions interfaces - Add tsconfig.json for TypeScript compilation - Update package.json: main points to dist/index.js, add types field, add build script, add typescript and @types/node devDependencies - Add .gitignore for node_modules/ and dist/ Co-Authored-By: shayan <shayan@cognition.ai>

devin-ai-integration · 2026-06-30T22:00:32Z

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

Disable automatic comment, CI, and merge conflict monitoring

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

devin-ai-integration

Devin Review found 4 potential issues.

devin-ai-integration · 2026-06-30T22:02:28Z

+  start(): void {
+    require('./bin/superset-mcp.js');
+  }


🔴 Server stop method can never work because the start method never captures the spawned process

The server's child process reference is never assigned during startup (require('../bin/superset-mcp.js') at superset/mcp_service/index.ts:66), so the stop method's kill call is always skipped.

Impact: Callers who use this class to manage the MCP server lifecycle cannot programmatically stop it — the spawned Python process keeps running.

start() never populates this.process, making stop() dead code

The start() method at superset/mcp_service/index.ts:65-67 calls require('../bin/superset-mcp.js') which internally runs main() in superset/mcp_service/bin/superset-mcp.js:201-278. That main() function calls spawn() at line 250, creating a child process, but the process handle is local to main() and never returned or exported.

Meanwhile, this.process is initialized to null at superset/mcp_service/index.ts:62 and is never reassigned. The stop() method at lines 69-73 checks if (this.process) which is always null, so this.process.kill() is never reached.

The old JS code at index.js:43-46 had the same bug. The TypeScript conversion faithfully preserved the broken lifecycle management.

Was this helpful? React with 👍 or 👎 to provide feedback.

This is a pre-existing issue in the original JS code — start() never captured the process handle there either. Intentionally not fixing it in this migration PR to keep the scope to a JS→TS conversion, but it would be a good follow-up.

devin-ai-integration · 2026-06-30T22:02:30Z

+    "module": "commonjs",
+    "lib": ["ES2020"],
+    "outDir": "./dist",
+    "rootDir": ".",


📝 Info: tsconfig rootDir includes package.json, which may produce unexpected dist/ structure

tsconfig.json:7 sets rootDir: "." but include at line 18 only lists index.ts. While this means only index.ts is compiled, resolveJsonModule: true (line 15) combined with rootDir: "." means if any future TS file imports package.json, TypeScript would try to preserve the directory structure relative to . in dist/. For now this is benign since only index.ts is compiled and it doesn't import JSON, but it's a fragile setup if the project grows.

Was this helpful? React with 👍 or 👎 to provide feedback.

Acknowledged — benign for the current single-file setup. If more .ts files are added that import JSON, rootDir can be narrowed to "./src" at that point.

…ompat - require('./bin/superset-mcp.js') -> require('../bin/superset-mcp.js') since compiled output lives in dist/ - Replace dual export default + module.exports with export= for clean CJS semantics Co-Authored-By: shayan <shayan@cognition.ai>

devin-ai-integration

Devin Review found 2 new potential issues.

devin-ai-integration · 2026-06-30T22:06:00Z

+  }
+
+  start(): void {
+    require('../bin/superset-mcp.js');


📝 Info: Require path adjusted correctly for dist/ output directory

The old index.js used require('./bin/superset-mcp.js') which resolved from superset/mcp_service/ to superset/mcp_service/bin/superset-mcp.js. The new index.ts uses require('../bin/superset-mcp.js') (superset/mcp_service/index.ts:66). Since tsconfig.json sets outDir: "./dist", the compiled output lands at superset/mcp_service/dist/index.js, and from there ../bin/superset-mcp.js correctly resolves back to superset/mcp_service/bin/superset-mcp.js. The path change is mechanically correct.

Was this helpful? React with 👍 or 👎 to provide feedback.

devin-ai-integration · 2026-06-30T22:06:01Z

+  constructor(options: SupersetMCPServerOptions = {}) {
+    this.options = {
+      transport: options.transport ?? 'http',
+      host: options.host ?? '127.0.0.1',
+      port: options.port ?? 5008,
+      debug: options.debug ?? false,
+      pythonPath: options.pythonPath ?? null,
+      supersetRoot: options.supersetRoot ?? null,
+      configPath: options.configPath ?? null,
+    };
+    this.process = null;
+  }


🚩 Constructor options are stored but never consumed

The options property (transport, host, port, debug, etc.) is carefully resolved and stored at superset/mcp_service/index.ts:53-61, but the start() method at line 65-67 simply calls require('../bin/superset-mcp.js') which parses its own CLI args and environment variables independently (superset/mcp_service/bin/superset-mcp.js:67-74). None of the stored options are forwarded. This means constructing new SupersetMCPServer({ port: 9000 }) has no effect on the actual server behavior. This is a pre-existing design gap (the old JS code had the same issue), but worth noting since the TypeScript conversion would have been a natural time to address it.

Was this helpful? React with 👍 or 👎 to provide feedback.

Copilot AI review requested due to automatic review settings June 30, 2026 22:00

shayanshafii self-assigned this Jun 30, 2026

Copilot AI reviewed Jun 30, 2026

devin-ai-integration Bot reviewed Jun 30, 2026

View reviewed changes

Navigation Menu

Uh oh!

refactor(mcp): migrate mcp_service/index.js from JavaScript to TypeScript - #22

refactor(mcp): migrate mcp_service/index.js from JavaScript to TypeScript#22
shayanshafii wants to merge 2 commits into
masterfrom
devin/1782856722-migrate-index-js-to-ts

shayanshafii commented Jun 30, 2026 •

edited by devin-ai-integration Bot

Loading

Uh oh!

devin-ai-integration Bot commented Jun 30, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

devin-ai-integration Bot left a comment

Uh oh!

Uh oh!

devin-ai-integration Bot Jun 30, 2026 •

edited

Loading

Uh oh!

devin-ai-integration Bot Jun 30, 2026

Uh oh!

Uh oh!

devin-ai-integration Bot Jun 30, 2026

Uh oh!

devin-ai-integration Bot Jun 30, 2026

Uh oh!

devin-ai-integration Bot left a comment

Uh oh!

devin-ai-integration Bot Jun 30, 2026

Uh oh!

devin-ai-integration Bot Jun 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

shayanshafii commented Jun 30, 2026 • edited by devin-ai-integration Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Devin Review

Uh oh!

devin-ai-integration Bot commented Jun 30, 2026

🤖 Devin AI Engineer

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devin-ai-integration Bot Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devin-ai-integration Bot Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shayanshafii commented Jun 30, 2026 •

edited by devin-ai-integration Bot

Loading

devin-ai-integration Bot Jun 30, 2026 •

edited

Loading