fix(mcp): add dataset_id to update_chart to support rebinding chart datasource#40853
fix(mcp): add dataset_id to update_chart to support rebinding chart datasource#40853aminghadersohi wants to merge 6 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40853 +/- ##
==========================================
- Coverage 64.18% 64.17% -0.01%
==========================================
Files 2655 2655
Lines 143925 143981 +56
Branches 33181 33197 +16
==========================================
+ Hits 92382 92405 +23
- Misses 49921 49952 +31
- Partials 1622 1624 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
From my agent reviewer: Overall: solid fix, security model intact. One real bug to fix before merge. 🔴 High — Rename silently dropped 🟡 Medium — Dataset-only rebind skips validation 🟢 Low — datasource_type hardcoded to "table" 🟢 Low — DRY 🟢 Low — Test gaps |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #77fe3f
Actionable Suggestions - 2
-
superset/mcp_service/chart/tool/update_chart.py - 2
- Missing validation for dataset-only rebind · Line 139-144
- Missing validation in preview path · Line 505-514
Review Details
-
Files reviewed - 3 · Commit Range:
5f83dec..5f83dec- superset/mcp_service/chart/schemas.py
- superset/mcp_service/chart/tool/update_chart.py
- tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| # Dataset-only update: rebind chart to a different dataset without changing viz | ||
| if request.dataset_id is not None: | ||
| return { | ||
| "datasource_id": request.dataset_id, | ||
| "datasource_type": "table", | ||
| } |
There was a problem hiding this comment.
When dataset_id is provided alone (without config), the code skips validation entirely. The new dataset might not exist, might be inaccessible, or might have incompatible schema. Add a validation call using _validate_update_against_dataset with the new dataset_id before returning the payload at line 140.
Code suggestion
Check the AI-generated fix before applying
--- superset/mcp_service/chart/tool/update_chart.py (lines 139-144) ---
139: # Dataset-only update: rebind chart to a different dataset without changing viz
140: if request.dataset_id is not None:
141: + # Validate the new dataset exists and is accessible before rebinding
142: + validation_error = _validate_update_against_dataset(
143: + None, # parsed_config
144: + {}, # form_data (empty for dataset-only update)
145: + chart,
146: + dataset_id=request.dataset_id,
147: + )
148: + if validation_error is not None:
149: + return validation_error
150: return {
151: "datasource_id": request.dataset_id,
152: "datasource_type": "table",
153: }
Code Review Run #77fe3f
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Fixed in faef804. Added validation for the dataset-only rebind path in both persist and preview flows. The target dataset is now checked for existence via _validate_update_against_dataset before writing or caching. The compile check is intentionally skipped (run_compile_check=False) since there is no new chart config to execute — the existing chart form_data is preserved as-is, only the datasource binding changes.
There was a problem hiding this comment.
The implementation correctly addresses the suggestion by adding the required validation call using _validate_update_against_dataset before the dataset rebind occurs. By passing None for the config and an empty dictionary for the form data, the code ensures that the dataset existence and accessibility are verified without triggering unnecessary compilation checks, which is appropriate for a dataset-only update.
superset/mcp_service/chart/tool/update_chart.py
if request.dataset_id is not None:
# Validate the new dataset exists and is accessible before rebinding
validation_error = _validate_update_against_dataset(
None, # parsed_config
{}, # form_data (empty for dataset-only update)
chart,
dataset_id=request.dataset_id,
)
if validation_error is not None:
return validation_error
| if parsed_config is not None: | ||
| with event_logger.log_context(action="mcp.update_chart.validation"): | ||
| validation_error = _validate_update_against_dataset( | ||
| parsed_config, preview_or_error, chart | ||
| parsed_config, | ||
| preview_or_error, | ||
| chart, | ||
| dataset_id=request.dataset_id, | ||
| ) | ||
| if validation_error is not None: | ||
| return validation_error |
There was a problem hiding this comment.
The preview path (generate_preview=True) has the same validation gap as the persist path. When only dataset_id is provided, validation is skipped. Add a check: if request.dataset_id is not None and parsed_config is None: validation_error = _validate_update_against_dataset(None, {}, chart, dataset_id=request.dataset_id)
Code suggestion
Check the AI-generated fix before applying
--- superset/mcp_service/chart/tool/update_chart.py (lines 505-514) ---
505: # Validate before caching the form_data — same rationale as above.
506: + # Also validate dataset-only updates to catch invalid/inaccessible datasets
507: + if parsed_config is not None or request.dataset_id is not None:
508: if parsed_config is not None:
509: with event_logger.log_context(action="mcp.update_chart.validation"):
510: validation_error = _validate_update_against_dataset(
511: parsed_config,
512: preview_or_error,
513: chart,
514: dataset_id=request.dataset_id,
515: )
516: + else:
517: + # Dataset-only update: validate the new dataset exists
518: + with event_logger.log_context(action="mcp.update_chart.validation"):
519: + validation_error = _validate_update_against_dataset(
520: + None,
521: preview_or_error,
522: chart,
523: dataset_id=request.dataset_id,
524: )
Code Review Run #77fe3f
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
Fixed in faef804. Same dataset existence validation is now applied in the preview path when dataset_id is provided without config. The compile check is skipped (run_compile_check=False) for the same reason — no chart config to execute against the new dataset.
There was a problem hiding this comment.
The suggestion to add validation in the preview path for dataset-only updates is appropriate. It ensures that dataset existence and accessibility are verified even when no chart configuration is provided, maintaining consistency with the persist path.
superset/mcp_service/chart/tool/update_chart.py
507: + if parsed_config is not None or request.dataset_id is not None:
508: if parsed_config is not None:
509: with event_logger.log_context(action="mcp.update_chart.validation"):
510: validation_error = _validate_update_against_dataset(
511: parsed_config,
512: preview_or_error,
513: chart,
514: dataset_id=request.dataset_id,
515: )
516: + else:
517: + # Dataset-only update: validate the new dataset exists
518: + with event_logger.log_context(action="mcp.update_chart.validation"):
519: + validation_error = _validate_update_against_dataset(
520: + None,
521: preview_or_error,
522: chart,
523: dataset_id=request.dataset_id,
524: )
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| # Dataset-only update: rebind chart to a different dataset without changing viz | ||
| if request.dataset_id is not None: | ||
| return { | ||
| "datasource_id": request.dataset_id, | ||
| "datasource_type": "table", | ||
| } |
There was a problem hiding this comment.
Suggestion: In the non-config path, when both dataset_id and chart_name are provided, the function returns early with only datasource fields and silently drops the rename. This creates inconsistent behavior (preview can show the renamed title while save-without-preview won't persist it). Include slice_name in the dataset-only payload when chart_name is provided instead of discarding it. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Save path silently ignores requested chart title rename.
- ⚠️ Preview and saved chart titles become inconsistent for users.
- ⚠️ Dataset rebind plus rename workflow behaves unpredictably.Steps of Reproduction ✅
1. Call the MCP `update_chart` tool in
`superset/mcp_service/chart/tool/update_chart.py:339-400` with `generate_preview=True`,
omitting `config` so `parsed_config` is `None`, but providing both `dataset_id` (to
rebind) and `chart_name` (to rename), matching the documented dataset+name update use case
in the PR description.
2. In this preview path, `_build_preview_form_data` at
`superset/mcp_service/chart/tool/update_chart.py:152-200` loads `existing_form_data`,
computes `effective_dataset_id`, and because `parsed_config is None` but
`request.dataset_id` is not `None`, it skips `_missing_config_or_name_error` at lines
187-188, copies `existing_form_data` into `merged`, then sets `merged["slice_name"] =
request.chart_name` at lines 191-192 and `merged["datasource"] =
f"{effective_dataset_id}__table"` at lines 196-198, so the preview URL reflects both the
new dataset and the new title.
3. Next, call `update_chart` again with the same `identifier`, `dataset_id`, and
`chart_name` but with `generate_preview=False` (the "persist immediately" mode described
in the docstring around lines 374-381 of `update_chart.py`), which routes execution into
the non-preview save path at lines 72-119.
4. In this save path, `_build_update_payload` at
`superset/mcp_service/chart/tool/update_chart.py:98-149` sees `parsed_config is None` and
skips the config branch, then hits the "Dataset-only update" block at lines 139-144: `if
request.dataset_id is not None: return {"datasource_id": request.dataset_id,
"datasource_type": "table"}`. This early return ignores `request.chart_name`, so
`UpdateChartCommand` (invoked at lines 112-114) updates only
`datasource_id`/`datasource_type` and leaves `slice_name` unchanged, causing the rename to
be silently dropped even though the earlier preview showed the new title.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/update_chart.py
**Line:** 139:144
**Comment:**
*Incomplete Implementation: In the non-config path, when both `dataset_id` and `chart_name` are provided, the function returns early with only datasource fields and silently drops the rename. This creates inconsistent behavior (preview can show the renamed title while save-without-preview won't persist it). Include `slice_name` in the dataset-only payload when `chart_name` is provided instead of discarding it.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Fixed in c80a1b5. When both dataset_id and chart_name are provided without config, _build_update_payload now includes slice_name in the payload so the rename is persisted alongside the datasource rebind.
| if dataset_id is not None: | ||
| dataset = DatasetDAO.find_by_id(dataset_id) |
There was a problem hiding this comment.
Suggestion: The new dataset lookup only checks existence and skips dataset-level authorization, so a user can pass a dataset_id they cannot access and still run validation against it. This can leak whether the dataset exists and potentially schema-derived validation details before the write path permission checks run. Add an explicit access check (same pattern used in other chart MCP tools) immediately after resolving dataset_id and treat unauthorized datasets as inaccessible. [security]
Severity Level: Critical 🚨
- ❌ update_chart tool can probe unauthorized dataset existence.
- ⚠️ Validation errors may leak schema or compile details.
- ⚠️ Live compile runs against datasets before access checks.Steps of Reproduction ✅
1. Invoke the MCP `update_chart` tool defined in
`superset/mcp_service/chart/tool/update_chart.py:339-400` with a request containing an
accessible `identifier` (existing chart) plus a `dataset_id` pointing to a different
dataset the caller cannot access, and a valid `config` payload so `parsed_config` is
non-null.
2. Inside `update_chart`, the chart is loaded and the existing datasource is checked via
`check_chart_data_access(chart)` at
`superset/mcp_service/chart/tool/update_chart.py:39-60`, which validates only the chart's
current dataset, not the requested `dataset_id`, so the request continues even if the
target dataset is unauthorized.
3. In the non-preview path (`generate_preview=False`), `update_chart` builds
`payload_or_error` and then calls `_validate_update_against_dataset(parsed_config,
new_form_data, chart, dataset_id=request.dataset_id)` at
`superset/mcp_service/chart/tool/update_chart.py:87-94`, passing the untrusted
`dataset_id` straight into validation.
4. `_validate_update_against_dataset` in
`superset/mcp_service/chart/tool/update_chart.py:203-249` executes `dataset =
DatasetDAO.find_by_id(dataset_id)` at lines 221-222 with no permission check, then calls
`validate_and_compile(parsed_config, form_data, dataset,
run_compile_check=run_compile_check)` at lines 247-249 and returns either success or
detailed validation errors in a `GenerateChartResponse`. Because the write path's access
control (in `superset/commands/chart/update.py:144-151` using `get_datasource_by_id` and
`security_manager.raise_for_access`) runs later, callers can probe which `dataset_id`
values exist and observe schema-derived validation behavior on datasets they are not
authorized to access.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/chart/tool/update_chart.py
**Line:** 221:222
**Comment:**
*Security: The new dataset lookup only checks existence and skips dataset-level authorization, so a user can pass a `dataset_id` they cannot access and still run validation against it. This can leak whether the dataset exists and potentially schema-derived validation details before the write path permission checks run. Add an explicit access check (same pattern used in other chart MCP tools) immediately after resolving `dataset_id` and treat unauthorized datasets as inaccessible.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Acknowledged. The intent here was to keep the pre-validation lightweight — the actual write path enforces access via UpdateChartCommand (which calls security_manager.raise_for_access). For run_compile_check=False (dataset-only rebind) the only information that leaks before the write check is whether the dataset ID exists, which is a minor exposure. Adding a full has_dataset_access check here introduces complexity and test surface, and the write path already gates the mutation. Happy to add it if the reviewer considers it necessary.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #b54a9dActionable Suggestions - 0Additional Suggestions - 2
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…atasource UpdateChartRequest lacked a dataset_id field, so Pydantic silently dropped the parameter whenever the LLM passed it. _build_update_payload always fell back to chart.datasource_id and never included datasource_id/datasource_type in the UpdateChartCommand payload, causing the tool to return success: true while the chart stayed bound to its original dataset. Added dataset_id: int | None to UpdateChartRequest and plumbed it through _build_update_payload (includes datasource_id/datasource_type in payload), _build_preview_form_data (updates datasource field), _validate_update_against_dataset (validates against the new dataset when provided), and _create_preview_url (uses new datasource for form_data caching). Also allows dataset-only updates without requiring config or chart_name.
When dataset_id is provided without config, the target dataset is now checked for existence before persisting or caching. The compile check is intentionally skipped (run_compile_check=False) since there is no new chart config to execute against the new dataset. Also fixes the DatasetNotAccessible error message to report the correct dataset ID when rebinding to a new dataset.
…e drop in dataset rebind - Wrap two E501 docstrings in test_update_chart.py (lines 1553, 1601) to stay within 88-char limit - Add DatasetDAO.find_by_id mock to test_dataset_id_passed_to_update_command so the dataset-only rebind validation path no longer fails with an unpatched DAO call - Fix _build_update_payload to include slice_name in the dataset-only rebind payload when chart_name is also provided (silent drop bug) - Update test_dataset_and_name_update assertion to expect slice_name in the payload
aminghadersohi
force-pushed
the
aminghadersohi/fix-update-chart-dataset-id
branch
from
5d3a1f3 to
c80a1b5
Compare
Code Review Agent Run #ca44c6Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…r in tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review Agent Run #28b5deActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| class TestBuildUpdatePayloadDatasetId: | ||
| """Tests for dataset_id support in _build_update_payload.""" | ||
|
|
||
| def test_dataset_only_update_returns_datasource_fields(self): |
There was a problem hiding this comment.
Suggestion: Add explicit type hints to this new test method signature, including a -> None return annotation, to satisfy the fully-typed new code rule. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly added test method and it has no return type annotation. Under the rule, new Python methods should be fully typed, so the omission is a real violation.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
**Line:** 1353:1353
**Comment:**
*Custom Rule: Add explicit type hints to this new test method signature, including a `-> None` return annotation, to satisfy the fully-typed new code rule.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Fixed in 2cc5fbd. Added -> None return annotation and explicit Any/Mock parameter type hints to the flagged test methods.
| assert isinstance(result, dict) | ||
| assert result == {"datasource_id": 42, "datasource_type": "table"} | ||
|
|
||
| def test_dataset_and_name_update(self): |
There was a problem hiding this comment.
Suggestion: Add type annotations to this new method signature and declare a -> None return type so the added test code is fully typed. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This newly introduced test method lacks a return annotation and any parameter typing beyond self. That matches the custom rule requiring newly added Python methods to be fully typed.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
**Line:** 1364:1364
**Comment:**
*Custom Rule: Add type annotations to this new method signature and declare a `-> None` return type so the added test code is fully typed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Fixed in 2cc5fbd. Added -> None return annotation and explicit Any/Mock parameter type hints to the flagged test methods.
| async def test_dataset_id_passed_to_update_command( | ||
| self, | ||
| mock_db_session, | ||
| mock_find_by_id, | ||
| mock_validate, | ||
| mock_update_cmd_cls, | ||
| mock_check_access, | ||
| mcp_server, |
There was a problem hiding this comment.
Suggestion: Annotate all parameters in this newly added async test method (including fixture/mocked arguments) and add an explicit -> None return type. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This added async test method has untyped fixture/mocked parameters and no -> None return annotation. The rule explicitly flags new Python methods that omit type hints.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
**Line:** 1489:1496
**Comment:**
*Custom Rule: Annotate all parameters in this newly added async test method (including fixture/mocked arguments) and add an explicit `-> None` return type.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Fixed in 2cc5fbd. Added -> None return annotation and explicit Any/Mock parameter type hints to the flagged test methods.
| async def test_dataset_only_rebind_invalid_dataset_returns_error( | ||
| self, | ||
| mock_db_session, | ||
| mock_chart_find, | ||
| mock_dataset_find, | ||
| mock_check_access, | ||
| mcp_server, |
There was a problem hiding this comment.
Suggestion: Add full type hints for this new async method's parameters and include a -> None return annotation to comply with typing requirements for added code. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This new async test method omits type annotations for its parameters and does not declare a return type. That is a genuine violation of the fully-typed new code rule.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
**Line:** 1545:1551
**Comment:**
*Custom Rule: Add full type hints for this new async method's parameters and include a `-> None` return annotation to comply with typing requirements for added code.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Fixed in 2cc5fbd. Added -> None return annotation and explicit Any/Mock parameter type hints to the flagged test methods.
| async def test_dataset_only_rebind_invalid_dataset_preview_returns_error( | ||
| self, | ||
| mock_db_session, | ||
| mock_chart_find, | ||
| mock_dataset_find, | ||
| mock_check_access, | ||
| mcp_server, |
There was a problem hiding this comment.
Suggestion: Add explicit annotations for all arguments in this newly introduced async test and declare -> None as the return type. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This added async method also lacks parameter type hints and a -> None return annotation. The custom rule applies directly here, so the suggestion is verified.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_update_chart.py
**Line:** 1595:1601
**Comment:**
*Custom Rule: Add explicit annotations for all arguments in this newly introduced async test and declare `-> None` as the return type.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Fixed in 2cc5fbd. Added -> None return annotation and explicit Any/Mock parameter type hints to the flagged test methods.
Code Review Agent Run #e85895Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review Agent Run #d96fd7Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
2 participants
Summary
UpdateChartRequestlacked adataset_idfield, so Pydantic silently dropped the parameter whenever the LLM passed it_build_update_payloadalways fell back tochart.datasource_idand never includeddatasource_id/datasource_typein theUpdateChartCommandpayload — the tool returnedsuccess: truewhile the chart stayed bound to its original datasetdataset_id: int | NonetoUpdateChartRequestand plumbed it through_build_update_payload,_build_preview_form_data,_validate_update_against_dataset, and_create_preview_urlTest plan
TestBuildUpdatePayloadDatasetId— 4 unit tests covering dataset-only, dataset+name, dataset+config, and config-without-dataset payloadsTestBuildPreviewFormDataDatasetId— 3 unit tests verifying thedatasourcefield is set correctly in preview form_dataTestUpdateChartDatasetIdIntegration— async integration test verifyingUpdateChartCommandreceivesdatasource_idanddatasource_typein payload