aminghadersohi commented on PR #44797: URL: https://github.com/apache/superset/pull/44797#issuecomment-5902051017
Review pass at 892fc24aad, covering correctness, access scoping, and tests. **Findings and dispositions** - Bito run #0086c7, "Dead selected_ids initializer": valid. Fixed in 6bff49560c and replied on the thread, which is now resolved. Every path that reads `selected_ids` first binds it through `_subtree_ids`: an explicit `tab` does that, and the unscoped, `untabbed_only` and `tabs_only`-without-`tab` paths all return before reaching it. - Bito additional suggestion, "Unbounded DFS on tab tree": does not apply. `_extract_layout_from_position` walks from `ROOT_ID` with a visited set. `parent_tab_id` comes from the traversal ancestry, not from `position_json`. So the parsed tabs form an acyclic tree rooted at the top-level tabs, and `_tab_summaries` cannot loop or miss a depth. - Test coverage gap (from this review): the shared-permalink test already ran with `tabs_only` and with `tab` set to a tab other than the permalink's active tab. It only checked that the permalink state survived, never that the scope was applied. Fixed in 892fc24aad: the test now also asserts `scope`, `tab_tree`, `tabs` and chart placements. I checked it by disabling `_scope_layout`, which makes the test fail. **No issues found** - Access: scoping only filters the already-authorized parsed layout, and the dashboard is still fetched through `DashboardDAO` under the `Dashboard` permission. The suggestions on the oversized-response error reuse that caller's own blocked payload, with tab IDs capped at 5 and truncated to 64 characters. The `list_charts` hint goes through that tool's normal access filtering. - Tab resolution (ID before title, ambiguous titles, blank selector), distinct `untabbed_chart_count`, and `untabbed_only` exclusivity all behave as described and have tests. **Validation:** `tests/unit_tests/mcp_service/dashboard`, `utils` and the middleware tests: 1357 passed. The layout tool file alone: 55 passed. Pre-commit on the changed file passed (mypy, ruff, ruff-format). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
