feat: add Metabase MCP server package#20
Merged
Conversation
CRUD for cards, dashboards, collections. Run SQL queries, list databases/tables, get collection items.
📝 WalkthroughWalkthroughAdds a new package Changes
Sequence DiagramssequenceDiagram
participant App as Application
participant Server as MetabaseMCPServer
participant MCP as MCP SDK
participant Client as MetabaseClient
participant API as Metabase API
App->>Server: fromEnvironment()
Server->>Client: new MetabaseClient(config)
Server->>MCP: init McpServer & register tools
App->>Server: start()
Server->>Client: testConnection()
Client->>API: GET /api/user/current
API-->>Client: 200 OK
Client-->>Server: connection OK
Server->>MCP: create StdioServerTransport & connect()
MCP-->>Server: connected
Server-->>App: started
sequenceDiagram
participant MCPClient as MCP Client
participant Server as MetabaseMCPServer
participant Tools as MetabaseMCPTools
participant Client as MetabaseClient
participant API as Metabase API
MCPClient->>Server: invoke tool(run_card_query)
Server->>Tools: runCardQuery(params)
Tools->>Client: runCardQuery(cardId, parameters)
Client->>API: POST /api/card/{id}/query
API-->>Client: result set
Client-->>Tools: result
Tools->>Server: McpToolResult (JSON)
Server-->>MCPClient: tool response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/metabase/src/index.ts`:
- Around line 5-12: The file currently starts the server on import by calling
MetabaseMCPServer.fromEnvironment(), server.setupGracefulShutdown(), and await
server.start(); wrap that startup sequence in a CLI guard so it only runs when
invoked directly (use if (require.main === module) { ... } or equivalent async
IIFE inside that guard). Move the try/catch inside the guard and avoid calling
process.exit(1) from module scope—handle errors by logging and rethrowing or
returning a rejected promise only within the CLI path, leaving the exported
MetabaseMCPServer symbol usable without side effects.
In `@packages/metabase/src/metabase-client.ts`:
- Around line 120-155: addCardToDashboard currently rebuilds every dashcard from
a six-field projection and PUTs that array, which can drop fields and clobber
concurrent changes; instead preserve the full server dashcard objects and
perform an append: fetch dashboard via getDashboard (dashboard), read
existingCards = dashboard.dashcards and push a newDashcard object that includes
the minimal required fields (match server shape — e.g., use card_id or card
reference consistently) without reconstructing other properties, then call
this.request("PUT", `/dashboard/${dashboardId}`, { dashcards:
dashboard.dashcards }) so only the new card is added and existing dashcard
attributes are retained.
- Around line 15-53: The request method can hang because fetch has no timeout;
update the private async request<T> method to create an AbortController, pass
its signal into fetch, and set a timer that calls controller.abort() after a
configurable deadline (e.g., a default timeout constant or a new optional
parameter); when aborting, catch the abort error and throw a MetabaseMCPError
with a distinct code like "REQUEST_TIMEOUT" (preserving the original error as
the cause), and ensure the timer is cleared on success or other failures so
there are no leaks; keep existing handling for MetabaseMCPError and non-timeout
errors.
In `@packages/metabase/src/server.ts`:
- Around line 134-141: The tool registration for "list_dashboards" exposes no
input but your types export ListDashboardsParamsSchema.filter and
MetabaseClient.listDashboards() ignores filters; either thread the filter
through or remove the public type. Fix option A: update the server.registerTool
call for "list_dashboards" to accept the ListDashboardsParamsSchema (or its
.filter) as inputSchema and pass that filter into
MetabaseClient.listDashboards(filter) so the client uses the provided params.
Fix option B: if filtering isn't supported, remove or stop exporting
ListDashboardsParamsSchema.filter from packages/metabase/src/types.ts and update
any callers to avoid relying on that contract; ensure the unique symbols to
change are the server.registerTool invocation for "list_dashboards",
ListDashboardsParamsSchema.filter, and MetabaseClient.listDashboards().
- Around line 51-273: Add a minimal Vitest smoke test suite that (1) verifies
setupTools() registers all expected tools by spying/mocking server.registerTool
and asserting it was called 17 times with the known tool names ("list_cards",
"get_card", ... "list_tables") and that each registration includes the
corresponding inputSchema shapes referenced in the diff, and (2) adds a couple
of MetabaseClient.request() tests that mock the HTTP layer to assert a
successful JSON response is returned and an error path throws/returns the
expected error; target the setupTools method and MetabaseClient.request by
importing those symbols, mock the server object (with a registerTool spy) and
the HTTP client used inside MetabaseClient.request, and assert both registration
behavior and request success/error cases so CI vitest --coverage passes.
In `@packages/metabase/src/tools.ts`:
- Around line 157-159: The code assigns list from rawItems.data or items and
then calls list.map, which can throw if list is not an array; update the logic
around rawItems, list and the summary mapping so you first verify the fallback
is an Array (e.g., Array.isArray(rawItems.data) ? rawItems.data :
Array.isArray(items) ? items : []) before mapping, ensuring list is always an
array and handling the empty-array case rather than calling list.map on a
non-array.
In `@packages/metabase/src/types.ts`:
- Around line 54-60: Update UpdateCardParamsSchema so its display field uses the
same constrained enum as CreateCardParamsSchema instead of allowing any string:
replace the display: z.string().optional() in UpdateCardParamsSchema with a
z.enum([...]).optional() using the exact values from CreateCardParamsSchema
("table","bar","line","pie","scalar","row","area","combo","funnel","scatter","waterfall","progress","gauge","map")
so updates are validated against the supported visualization types before
calling Metabase.
- Around line 25-50: CreateCardParamsSchema's dataset_query allows invalid
combos (e.g., type: "native" without native.query) because it's a plain object;
change dataset_query to a discriminated union on the "type" field so each
variant enforces its required shape: one branch for type "native" that requires
native with query: string and database:number, and another branch for type
"query" that requires query (with "source-table" optional) and database:number;
update the CreateCardParamsSchema to use this discriminated union for
dataset_query so native.query is required for native and query object is
required for query.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8bb3f93-7fc9-40f8-bd67-ae30d1eb2e4a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
packages/metabase/.gitignorepackages/metabase/README.mdpackages/metabase/eslint.config.jspackages/metabase/package.jsonpackages/metabase/src/index.tspackages/metabase/src/metabase-client.tspackages/metabase/src/server.tspackages/metabase/src/tools.tspackages/metabase/src/types.tspackages/metabase/tsconfig.jsonpackages/metabase/vitest.config.ts
Joao208
approved these changes
Mar 18, 2026
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/metabase/src/metabase-client.test.ts`:
- Around line 37-45: The test calls the async method
MetabaseClient.testConnection() without awaiting it, which can mask failures;
make the test function async and await the call to c.testConnection() so any
thrown/rejected errors surface, keeping the existing mockResponse(...) and
expect(mockFetch) assertions unchanged; locate this in the test that constructs
new MetabaseClient and calls testConnection to apply the change.
- Around line 378-383: The test "should throw MetabaseMCPError on HTTP error"
calls client.listCards() twice while mockErrorResponse(...) uses
mockResolvedValueOnce(), so remove the redundant second expect and follow the
adjacent-test pattern: call mockErrorResponse(401, "Unauthorized"), await
client.listCards() inside a try/catch, catch the error and assert it is an
instance of MetabaseMCPError (and optionally assert message or status),
referencing client.listCards, mockErrorResponse, and MetabaseMCPError to locate
the test and update the assertions accordingly.
- Around line 385-404: The tests use try/catch assertions which can pass if the
promise doesn't reject; replace them with Vitest's async rejection assertions:
use await expect(client.getCard(999)).rejects.toMatchObject({ constructor:
MetabaseMCPError, code: "HTTP_404" }) (or
expect(...).rejects.toBeInstanceOf(MetabaseMCPError) and
.rejects.toMatchObject({ code: "HTTP_404" })) and similarly replace the
listCards test with await expect(client.listCards()).rejects.toMatchObject({
code: "REQUEST_FAILED" }) (ensuring mockErrorResponse(404, "Not found") and
mockFetch.mockRejectedValueOnce(new Error("Network error")) remain as the
arranged conditions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e82e255d-7d76-4772-ad84-de2b3b15f14a
📒 Files selected for processing (2)
packages/metabase/src/metabase-client.test.tspackages/metabase/src/tools.test.ts
Comment on lines
+37
to
+45
| it("should strip trailing slash from URL", () => { | ||
| const c = new MetabaseClient({ url: "https://metabase.example.com/", apiKey: "key" }); | ||
| mockResponse({ id: 1 }); | ||
| c.testConnection(); | ||
| expect(mockFetch).toHaveBeenCalledWith( | ||
| "https://metabase.example.com/api/user/current", | ||
| expect.any(Object) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/metabase/src/metabase-client.test.ts | head -60Repository: arvoreeducacao/arvore-mcp-servers
Length of output: 2289
🏁 Script executed:
cat -n packages/metabase/src/metabase-client.ts | grep -A 10 "testConnection"Repository: arvoreeducacao/arvore-mcp-servers
Length of output: 450
Await the async call to avoid masking potential errors.
testConnection() is an async method that returns a Promise. Line 40 calls it without await, which means the test can finish before any async failures propagate. Make the test function async and await the call to ensure errors are properly caught.
Suggested fix
- it("should strip trailing slash from URL", () => {
+ it("should strip trailing slash from URL", async () => {
const c = new MetabaseClient({ url: "https://metabase.example.com/", apiKey: "key" });
mockResponse({ id: 1 });
- c.testConnection();
+ await c.testConnection();
expect(mockFetch).toHaveBeenCalledWith(
"https://metabase.example.com/api/user/current",
expect.any(Object)
);
});📝 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
| it("should strip trailing slash from URL", () => { | |
| const c = new MetabaseClient({ url: "https://metabase.example.com/", apiKey: "key" }); | |
| mockResponse({ id: 1 }); | |
| c.testConnection(); | |
| expect(mockFetch).toHaveBeenCalledWith( | |
| "https://metabase.example.com/api/user/current", | |
| expect.any(Object) | |
| ); | |
| }); | |
| it("should strip trailing slash from URL", async () => { | |
| const c = new MetabaseClient({ url: "https://metabase.example.com/", apiKey: "key" }); | |
| mockResponse({ id: 1 }); | |
| await c.testConnection(); | |
| expect(mockFetch).toHaveBeenCalledWith( | |
| "https://metabase.example.com/api/user/current", | |
| expect.any(Object) | |
| ); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/metabase/src/metabase-client.test.ts` around lines 37 - 45, The test
calls the async method MetabaseClient.testConnection() without awaiting it,
which can mask failures; make the test function async and await the call to
c.testConnection() so any thrown/rejected errors surface, keeping the existing
mockResponse(...) and expect(mockFetch) assertions unchanged; locate this in the
test that constructs new MetabaseClient and calls testConnection to apply the
change.
Comment on lines
+378
to
+383
| it("should throw MetabaseMCPError on HTTP error", async () => { | ||
| mockErrorResponse(401, "Unauthorized"); | ||
|
|
||
| await expect(client.listCards()).rejects.toThrow(MetabaseMCPError); | ||
| await expect(client.listCards()).rejects.toThrow(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd packages/metabase && wc -l src/metabase-client.test.tsRepository: arvoreeducacao/arvore-mcp-servers
Length of output: 108
🏁 Script executed:
cd packages/metabase && sed -n '375,390p' src/metabase-client.test.tsRepository: arvoreeducacao/arvore-mcp-servers
Length of output: 578
🏁 Script executed:
cd packages/metabase && rg -A 20 "mockErrorResponse" src/metabase-client.test.ts | head -50Repository: arvoreeducacao/arvore-mcp-servers
Length of output: 1584
🏁 Script executed:
cd packages/metabase && sed -n '385,405p' src/metabase-client.test.tsRepository: arvoreeducacao/arvore-mcp-servers
Length of output: 773
🏁 Script executed:
cd packages/metabase && rg -B 2 -A 8 "rejects.toThrow" src/metabase-client.test.tsRepository: arvoreeducacao/arvore-mcp-servers
Length of output: 466
Fix redundant call in test or use proper mock setup.
The test calls client.listCards() twice but mockErrorResponse() uses mockResolvedValueOnce(), which only provides a mock response for the first call. The second assertion won't test the same error path.
Additionally, the suggested fix in the original comment is flawed—running multiple rejects assertions on the same promise won't work reliably and doesn't follow the project's testing patterns. Instead, use the try/catch approach consistent with the adjacent test (line 386):
Suggested fix
it("should throw MetabaseMCPError on HTTP error", async () => {
mockErrorResponse(401, "Unauthorized");
- await expect(client.listCards()).rejects.toThrow(MetabaseMCPError);
- await expect(client.listCards()).rejects.toThrow();
+ try {
+ await client.listCards();
+ expect.fail("Should have thrown");
+ } catch (error) {
+ expect(error).toBeInstanceOf(MetabaseMCPError);
+ expect((error as MetabaseMCPError).code).toBe("HTTP_401");
+ }
});📝 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
| it("should throw MetabaseMCPError on HTTP error", async () => { | |
| mockErrorResponse(401, "Unauthorized"); | |
| await expect(client.listCards()).rejects.toThrow(MetabaseMCPError); | |
| await expect(client.listCards()).rejects.toThrow(); | |
| }); | |
| it("should throw MetabaseMCPError on HTTP error", async () => { | |
| mockErrorResponse(401, "Unauthorized"); | |
| try { | |
| await client.listCards(); | |
| expect.fail("Should have thrown"); | |
| } catch (error) { | |
| expect(error).toBeInstanceOf(MetabaseMCPError); | |
| expect((error as MetabaseMCPError).code).toBe("HTTP_401"); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/metabase/src/metabase-client.test.ts` around lines 378 - 383, The
test "should throw MetabaseMCPError on HTTP error" calls client.listCards()
twice while mockErrorResponse(...) uses mockResolvedValueOnce(), so remove the
redundant second expect and follow the adjacent-test pattern: call
mockErrorResponse(401, "Unauthorized"), await client.listCards() inside a
try/catch, catch the error and assert it is an instance of MetabaseMCPError (and
optionally assert message or status), referencing client.listCards,
mockErrorResponse, and MetabaseMCPError to locate the test and update the
assertions accordingly.
Comment on lines
+385
to
+404
| it("should throw MetabaseMCPError with correct code on HTTP error", async () => { | ||
| mockErrorResponse(404, "Not found"); | ||
|
|
||
| try { | ||
| await client.getCard(999); | ||
| } catch (error) { | ||
| expect(error).toBeInstanceOf(MetabaseMCPError); | ||
| expect((error as MetabaseMCPError).code).toBe("HTTP_404"); | ||
| } | ||
| }); | ||
|
|
||
| it("should throw MetabaseMCPError on network failure", async () => { | ||
| mockFetch.mockRejectedValueOnce(new Error("Network error")); | ||
|
|
||
| try { | ||
| await client.listCards(); | ||
| } catch (error) { | ||
| expect(error).toBeInstanceOf(MetabaseMCPError); | ||
| expect((error as MetabaseMCPError).code).toBe("REQUEST_FAILED"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd metabase-client.test.tsRepository: arvoreeducacao/arvore-mcp-servers
Length of output: 122
🏁 Script executed:
wc -l packages/metabase/src/metabase-client.test.tsRepository: arvoreeducacao/arvore-mcp-servers
Length of output: 126
🏁 Script executed:
sed -n '385,404p' packages/metabase/src/metabase-client.test.tsRepository: arvoreeducacao/arvore-mcp-servers
Length of output: 765
🏁 Script executed:
head -50 packages/metabase/src/metabase-client.test.tsRepository: arvoreeducacao/arvore-mcp-servers
Length of output: 1534
🏁 Script executed:
rg -A 10 "class MetabaseMCPError|interface MetabaseMCPError|export.*MetabaseMCPError" packages/metabase/src/types.tsRepository: arvoreeducacao/arvore-mcp-servers
Length of output: 276
🏁 Script executed:
cat -n packages/metabase/src/types.ts | head -30Repository: arvoreeducacao/arvore-mcp-servers
Length of output: 1190
🏁 Script executed:
sed -n '350,420p' packages/metabase/src/metabase-client.test.tsRepository: arvoreeducacao/arvore-mcp-servers
Length of output: 2111
Replace try/catch assertions with expect().rejects.toMatchObject() to ensure errors are actually thrown.
Tests that only assert inside catch blocks will pass even if the promise resolves without throwing. Both test cases need the proper Vitest rejection assertion pattern.
Suggested fix
it("should throw MetabaseMCPError with correct code on HTTP error", async () => {
mockErrorResponse(404, "Not found");
-
- try {
- await client.getCard(999);
- } catch (error) {
- expect(error).toBeInstanceOf(MetabaseMCPError);
- expect((error as MetabaseMCPError).code).toBe("HTTP_404");
- }
+ await expect(client.getCard(999)).rejects.toMatchObject({
+ name: "MetabaseMCPError",
+ code: "HTTP_404",
+ });
});
it("should throw MetabaseMCPError on network failure", async () => {
mockFetch.mockRejectedValueOnce(new Error("Network error"));
-
- try {
- await client.listCards();
- } catch (error) {
- expect(error).toBeInstanceOf(MetabaseMCPError);
- expect((error as MetabaseMCPError).code).toBe("REQUEST_FAILED");
- }
+ await expect(client.listCards()).rejects.toMatchObject({
+ name: "MetabaseMCPError",
+ code: "REQUEST_FAILED",
+ });
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/metabase/src/metabase-client.test.ts` around lines 385 - 404, The
tests use try/catch assertions which can pass if the promise doesn't reject;
replace them with Vitest's async rejection assertions: use await
expect(client.getCard(999)).rejects.toMatchObject({ constructor:
MetabaseMCPError, code: "HTTP_404" }) (or
expect(...).rejects.toBeInstanceOf(MetabaseMCPError) and
.rejects.toMatchObject({ code: "HTTP_404" })) and similarly replace the
listCards test with await expect(client.listCards()).rejects.toMatchObject({
code: "REQUEST_FAILED" }) (ensuring mockErrorResponse(404, "Not found") and
mockFetch.mockRejectedValueOnce(new Error("Network error")) remain as the
arranged conditions).
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.
Descricao
Adiciona novo pacote
@arvoretech/metabase-mcpao monorepo de MCP servers. O servidor expoe 18 ferramentas para interagir com a API do Metabase: CRUD de cards, dashboards e collections, listagem de databases/tables, execucao de queries SQL, execucao de queries de cards, adicao de cards a dashboards e listagem de itens de collections.Etiquetas (Labels)
Historia Relacionada
N/A
Motivacao e Contexto
Permitir que agentes AI interajam com o Metabase para criar dashboards, executar queries e gerenciar recursos de BI diretamente via MCP. Segue o mesmo padrao dos outros pacotes do monorepo (datadog, mysql, etc).
Como Isso Foi Testado?
Analise de Risco e Impacto
Capturas de Tela ou Auxilios Visuais (se apropriado)
N/A
Summary by CodeRabbit
New Features
Documentation
Tests
Chores