Fullscreen viz mode + embedded viz sizing/scroll fixes for MCP App#426
Open
jarhun88 wants to merge 10 commits into
Open
Fullscreen viz mode + embedded viz sizing/scroll fixes for MCP App#426jarhun88 wants to merge 10 commits into
jarhun88 wants to merge 10 commits into
Conversation
Changed overflow from hidden to auto on fullscreen tableau-viz to prevent wide visualizations from being cut off.
- Removed forced height: 100% on tableau-viz in fullscreen - Moved overflow: auto to container level - Viz keeps natural dimensions, container scrolls both axes
- Drop the dead .main.fullscreen layer (duplicate of .viz-container.fullscreen at the same z-index; the container sits on top so .main's fullscreen styling never engaged) - Make .viz-container.fullscreen the single scroll owner; remove the conflicting overflow:auto on the nested tableau-viz (its comment already says the container scrolls) - Remove no-op declarations: height:100% on .viz-container (parent has no fixed height), and max-height/margin/padding resets that only restored defaults - Drop display:block and transition from the fullscreen viz rule (inherited from the base tableau-viz rule)
The base tableau-viz rule set max-height:1200px + overflow-y:auto, turning the viz element into its own scroll container. Combined with the outer scroller (host app frame, or .viz-container.fullscreen), tall vizzes showed two stacked scrollbars that conflicted on first scroll then chained to the outer one. Let the viz grow to its full JS-set natural height and leave a single scroll owner: the host frame in normal mode, .viz-container.fullscreen in fullscreen. Also drop the now-moot max-height:none override on the fullscreen viz.
…rollbar The Tableau Embedding API sizes its internal cross-origin iframe from the <tableau-viz> element's width/height attributes, not from CSS style. Setting only style.height left the iframe at a default size with its own scrollbar, which in fullscreen mode stacked with the .viz-container.fullscreen overflow scroll to produce two conflicting vertical scrollbars. Set the width attribute to '100%' (preserving responsive width, avoiding a horizontal scrollbar in normal mode) and the height attribute to the native sheet height from firstvizsizeknown. Enhance existing embedTableauViz tests to assert the new attributes.
The <tableau-viz> element renders a toolbar (~27-40px) in addition to the worksheet, but we size the element's height to the reported sheet height only (vizSize.sheetSize.maxSize.height), which excludes the toolbar. The extra toolbar height overflowed the element and produced a small internal vertical scrollbar in non-fullscreen mode. Set toolbar="hidden" so the iframe content height matches the reported sheet height exactly. Enhance the createTableauVizElement test to assert the attribute.
jarhun88
force-pushed
the
fullscreen-viz-mode
branch
from
June 29, 2026 03:30
df6efb4 to
cca55c0
Compare
Commit eb26c4d removed max-height:1200px + overflow-y:auto from the base tableau-viz rule (to kill the double scrollbar), which left the element height unbounded: embedTableauViz sizes it to sheetSize.maxSize.height, and some dashboards report a very large natural height, so the app frame grew without limit. Restore the bound as max-height:90vh + overflow:auto on .viz-container:not(.fullscreen). Scoped to non-fullscreen so fullscreen's own 100vh scroll container is untouched. The inner iframe scroll was already eliminated by the attribute-sizing and toolbar=hidden fixes, so the container is now the single scroll owner above the cap (one clean scrollbar).
jarhun88
commented
Jul 8, 2026
| return; | ||
| } | ||
|
|
||
| console.log('height: ', sheetHeight); |
Contributor
Author
There was a problem hiding this comment.
ℹ️ Info · 🧠 Logic
A console.log('height: ', sheetHeight) debug statement is left in production code. This will emit noise in the browser console for every viz load.
Tip
Suggested fix
Remove the console.log statement or replace it with a conditional debug-level log.
Review by CodeNod
| } | ||
| }; | ||
|
|
||
| // Handle host context changes (e.g., when host toggles fullscreen via its own UI) |
Contributor
Author
There was a problem hiding this comment.
The onhostcontextchanged handler duplicates the fullscreen-state logic already encapsulated in applyFullscreenState inside fullscreenToggle.ts. This creates two places to update if the fullscreen class logic changes.
Tip
Suggested fix
Export applyFullscreenState from fullscreenToggle.ts and call it from onhostcontextchanged instead of duplicating the class-toggle logic.
Review by CodeNod
| @@ -1,3 +1,3 @@ | |||
| { | |||
| "mcp-apps": false | |||
| "mcp-apps": true | |||
Contributor
Author
There was a problem hiding this comment.
ℹ️ Info · 🎨 Style
The mcp-apps feature flag is flipped to true in what appears to be a shared config file. If this is intended only for development/testing, shipping it enabled by default may unintentionally expose the feature.
Tip
Suggested fix
Confirm this flag change is intentional for production; if not, revert or gate behind an environment-specific mechanism.
Review by CodeNod
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
1 participant
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.
Summary
Adds a fullscreen display-mode toggle for the embedded Tableau viz in the MCP App, and fixes a series of embedded-viz sizing/scroll issues so the viz renders cleanly with a single scroll owner.
What's included
Fullscreen feature
fullscreenToggle.ts), with host-context-change handling inmcp-app.ts..viz-container.fullscreenis a fixed full-viewport (100vh) overlay that owns its own scroll region.Embedded viz sizing / scroll fixes (this round)
df8a719e— Size the viz via the<tableau-viz>width/height attributes (not just CSSstyle), so the Embedding API sizes its internal cross-origin iframe to the viz's natural dimensions. This removes the inner iframe scrollbar that, in fullscreen, stacked with the container scroll to produce two conflicting vertical scrollbars.cca55c05—toolbar="hidden"on the viz: the toolbar added height beyond the reported sheet height, producing a small residual inner scrollbar.04779e69— Revert of ahide-tabsattempt that broke rendering on a specific dashboard. (Tabs-bar handling intentionally left for follow-up.)df6efb46— Cap non-fullscreen height atmax-height: 90vh(scoped to.viz-container:not(.fullscreen)so it can't affect fullscreen's100vhcontainer). Sizing to full natural height had left non-fullscreen unbounded for very tall dashboards; the container is now the single scroll owner above the cap.Testing
npx vitest run src/web/apps/src/lib/embedTableauViz.test.ts— pass (attribute/sizing assertions included);fullscreenToggle.test.tscovers the toggle.build/web/apps/dist/mcp-app.htmlis regenerated vianpm run build:desktop(the app loads the bundled dist, not source).Manual verification (cross-origin iframe — cannot be unit-tested)
Known follow-up
hide-tabs) was reverted because it broke a specific dashboard's rendering. Worth revisiting from the actual failure mode if tabs-bar height is still a concern.