Skip to content

fix(mcp-server): fix DuckDB connection info and Dockerfile caching#1461

Merged
goldmedal merged 2 commits intoCanner:mainfrom
goldmedal:fix/mcp-ui-duckdb
Mar 18, 2026
Merged

fix(mcp-server): fix DuckDB connection info and Dockerfile caching#1461
goldmedal merged 2 commits intoCanner:mainfrom
goldmedal:fix/mcp-ui-duckdb

Conversation

@goldmedal
Copy link
Copy Markdown
Contributor

@goldmedal goldmedal commented Mar 18, 2026

Summary

  • Add missing format: "duckdb" hidden field to DuckDB connection info in the Web UI — this field is required but was not being sent
  • Rename DuckDB url label to "Directory Path" with a hint clarifying it points to a directory, not a .duckdb file
  • Add template support for hidden fields and per-field hints
  • Add BuildKit cache mounts for poetry in ibis-server Dockerfile to speed up rebuilds

Test plan

  • Start MCP server, open Web UI, select DuckDB, verify the format field is included in the submitted connection info
  • Verify the hint text appears below the Directory Path field
  • Build ibis-server Docker image and confirm poetry cache mounts work

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • New Features

    • Added support for hint text in form fields, providing helpful guidance on input fields.
  • Changes

    • Updated DuckDB datasource configuration to accept directory paths instead of individual file paths.
  • Chores

    • Optimized Docker build performance with improved caching strategy.

goldmedal and others added 2 commits March 18, 2026 10:23
…connection

DuckDB connection info requires `format: "duckdb"` but the Web UI was not
sending it. Add it as a hidden field with a fixed value. Also rename the
label to "Directory Path" with a hint clarifying it is not a .duckdb file path.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nv comments

Use BuildKit cache mounts for poetry cache/artifacts to speed up rebuilds.
Remove redundant inline comments for env vars.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions bot added ibis python Pull requests that update Python code labels Mar 18, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 18, 2026

📝 Walkthrough

Walkthrough

This PR introduces hidden form field support to the MCP server template system, updates the DuckDB datasource configuration to use directory paths instead of file paths, and optimizes Docker builds with Poetry cache mounts.

Changes

Cohort / File(s) Summary
Build Optimization
ibis-server/Dockerfile
Removed PIP_NO_CACHE_DIR environment variable and replaced plain poetry install with BuildKit-enabled cache mounts for Poetry's cache and artifacts directories to optimize build performance across rebuilds.
Template Enhancement
mcp-server/app/templates/_fields.html
Added conditional rendering for hidden form fields (no label, input only) and introduced optional hint text display for non-hidden fields; hidden fields are now excluded from visible form output.
DuckDB Configuration
mcp-server/app/web.py
Replaced single "path" field with "url" field expecting directory paths plus hidden "format" field; changes DuckDB datasource to collect directory containing .duckdb files rather than a specific .duckdb file path.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested labels

ibis, python

Suggested reviewers

  • douenergy
  • wwwy3y3

Poem

A hidden field tucked out of sight, 🐰
Templates now render hints just right,
DuckDB seeks folders, not files alone,
Docker caches poetry—faster builds are shown! 📦✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main changes: DuckDB connection info fixes and Dockerfile caching improvements.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

CodeRabbit can enforce grammar and style rules using `languagetool`.

Configure the reviews.tools.languagetool setting to enable/disable rules and categories. Refer to the LanguageTool Community to learn more.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
mcp-server/app/templates/_fields.html (1)

5-5: Preserve existing hidden values when re-rendering forms.

Line 5 currently ignores persisted connection_info and always uses the field default, which can silently overwrite hidden values on update flows.

Suggested patch
-    <input type="hidden" name="{{ field.name }}" value="{{ field.get('value', '') }}">
+    <input type="hidden" name="{{ field.name }}" value="{{ connection_info.get(field.name, field.get('value', '')) }}">
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mcp-server/app/templates/_fields.html` at line 5, The hidden input always
uses field.get('value', '') which overwrites persisted connection_info on
re-render; change the template to prefer any submitted/form value first (e.g.
use request.form.get(field.name, field.get('value', '')) or the framework's
equivalent) so existing hidden values are preserved when the form is
re-rendered; update the value expression for the input whose name is {{
field.name }} to read from form/request data with a fallback to
field.get('value', '').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@mcp-server/app/web.py`:
- Line 41: The dictionary literal for the DUCKDB field (the dict with keys
"name": "url", "label": "Directory Path", "type": "text", "placeholder":
"/data", "hint": "Path to a directory containing .duckdb files, not the .duckdb
file itself.") exceeds the 88-char line limit; reformat that dict in web.py so
each key/value pair is on its own line (or otherwise wrapped with implicit line
continuation inside the braces) to keep lines ≤88 chars and preserve the same
keys/values and order.
- Around line 41-42: When saving/updating connections in post_connection, detect
legacy payloads that use "path" and normalize them to "url" so existing saved
DuckDB locations are not dropped on re-save: if the incoming connection dict has
no "url" but has "path", copy/rename that value into "url" before
validation/storage; additionally enforce the hidden format server-side by
setting connection["format"] = "duckdb" unconditionally for DuckDB-type
connections (do this in post_connection before any client-driven validation or
persistence) so clients cannot omit or override the format.

---

Nitpick comments:
In `@mcp-server/app/templates/_fields.html`:
- Line 5: The hidden input always uses field.get('value', '') which overwrites
persisted connection_info on re-render; change the template to prefer any
submitted/form value first (e.g. use request.form.get(field.name,
field.get('value', '')) or the framework's equivalent) so existing hidden values
are preserved when the form is re-rendered; update the value expression for the
input whose name is {{ field.name }} to read from form/request data with a
fallback to field.get('value', '').

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 7b3c7409-b2d8-4ec1-a97a-c9b1298b9863

📥 Commits

Reviewing files that changed from the base of the PR and between 926cedc and 98ea281.

📒 Files selected for processing (3)
  • ibis-server/Dockerfile
  • mcp-server/app/templates/_fields.html
  • mcp-server/app/web.py

@goldmedal goldmedal requested review from chilijung and wwwy3y3 March 18, 2026 02:54
@goldmedal goldmedal merged commit a754ded into Canner:main Mar 18, 2026
7 checks passed
nhaluc1005 pushed a commit to nhaluc1005/text2sql-practice that referenced this pull request Apr 3, 2026
…anner#1461)

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ibis python Pull requests that update Python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants