feat: add Update Environment MCP App#10260
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new visual interface for selecting the active Firebase project within a workspace, including an HTML template, a TypeScript application for logic, and a resource definition to serve the UI. Several critical issues were identified: the HTML template incorrectly references a .ts file instead of the compiled .js file, and the firebase_get_environment tool is expected to return structured data but currently returns a string, which will break the 'Current Context' display. Additionally, there are multiple violations of the repository style guide, specifically the use of any types and deeply nested logic, along with a UI bug where safe area insets could overwrite necessary default padding.
| <div id="status-box"></div> | ||
| </div> | ||
| </div> | ||
| <script type="module" src="mcp-app.ts"></script> |
There was a problem hiding this comment.
Browsers cannot execute TypeScript files directly. This script tag should reference the compiled JavaScript file (e.g., mcp-app.js) to ensure the application functions correctly in the host environment.
| <script type="module" src="mcp-app.ts"></script> | |
| <script type="module" src="mcp-app.js"></script> |
| // Fetch current environment | ||
| try { | ||
| const envResult = await app.callServerTool({ name: "firebase_get_environment", arguments: {} }); | ||
| const envData = envResult.structuredContent as any; |
There was a problem hiding this comment.
The firebase_get_environment tool (defined in src/mcp/tools/core/get_environment.ts) returns a formatted string via toContent, not a structured object. Consequently, envResult.structuredContent will be undefined, and the 'Current Context' section of the UI will not be populated. You should update the tool to return structured data or adjust the UI to retrieve this information from a different source.
| let projects: any[] = []; | ||
| let filteredProjects: any[] = []; | ||
| let selectedProjectId: string | null = null; |
There was a problem hiding this comment.
The repository style guide (line 38) explicitly forbids the use of any or unknown as an escape hatch. Please define a proper interface for the project data to ensure type safety and maintainability.
| let projects: any[] = []; | |
| let filteredProjects: any[] = []; | |
| let selectedProjectId: string | null = null; | |
| interface FirebaseProject { | |
| projectId: string; | |
| displayName?: string; | |
| } | |
| let projects: FirebaseProject[] = []; | |
| let filteredProjects: FirebaseProject[] = []; | |
| let selectedProjectId: string | null = null; |
References
- Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)
| if (ctx.styles?.css?.fonts) applyHostFonts(ctx.styles.css.fonts); | ||
| if (ctx.safeAreaInsets) { | ||
| const { top, right, bottom, left } = ctx.safeAreaInsets; | ||
| document.body.style.padding = `${top}px ${right}px ${bottom}px ${left}px`; |
There was a problem hiding this comment.
Setting document.body.style.padding directly from safeAreaInsets will overwrite the default 24px padding defined in the CSS (line 27). If the safe area insets are zero, the UI will lose its intended margins. It is better to ensure a minimum padding that respects both the safe area and the design's spacing.
| document.body.style.padding = `${top}px ${right}px ${bottom}px ${left}px`; | |
| document.body.style.padding = `${Math.max(top, 24)}px ${Math.max(right, 24)}px ${Math.max(bottom, 24)}px ${Math.max(left, 24)}px`; |
| (async () => { | ||
| try { | ||
| await app.connect(); | ||
| showStatus("Connecting to server...", "info"); | ||
|
|
||
| // Fetch current environment | ||
| try { | ||
| const envResult = await app.callServerTool({ name: "firebase_get_environment", arguments: {} }); | ||
| const envData = envResult.structuredContent as any; | ||
| if (envData) { | ||
| envProjectIdEl.textContent = envData.projectId || "<NONE>"; | ||
| envUserEl.textContent = envData.authenticatedUser || "<NONE>"; | ||
| } | ||
| } catch (err: any) { | ||
| console.error("Failed to fetch environment:", err); | ||
| showStatus(`Failed to fetch environment: ${err.message}`, "error"); | ||
| } | ||
|
|
||
| // Fetch projects on load | ||
| const result = await app.callServerTool({ name: "firebase_list_projects", arguments: {} }); | ||
| const data = result.structuredContent as any; | ||
|
|
||
| if (data && data.projects) { | ||
| projects = data.projects; | ||
| filteredProjects = projects; | ||
| renderProjects(); | ||
| showStatus("Projects loaded successfully.", "success"); | ||
| setTimeout(() => { if (statusBox.className === "status success") statusBox.style.display = "none"; }, 3000); | ||
| } else { | ||
| showStatus("No projects returned from server.", "error"); | ||
| } | ||
| } catch (err: any) { | ||
| showStatus(`Failed to load projects: ${err.message}`, "error"); | ||
| } | ||
| })(); |
There was a problem hiding this comment.
This initialization block contains multiple levels of nested try-catch and async operations, which can be difficult to follow. Following the style guide (line 33), consider flattening this logic or extracting the environment and project fetching into separate helper functions to improve readability.
References
- Reduce nesting as much as possible. Code should avoid unnecessarily deep nesting or long periods of nesting. Handle edge cases early and exit or fold them into the general case. (link)
|
|
||
| // Fetch projects on load | ||
| const result = await app.callServerTool({ name: "firebase_list_projects", arguments: {} }); | ||
| const data = result.structuredContent as any; |
There was a problem hiding this comment.
Avoid using any for the tool result data. Use the FirebaseProject interface to maintain type safety as per the style guide (line 38).
| const data = result.structuredContent as any; | |
| const data = result.structuredContent as { projects: FirebaseProject[] } | undefined; |
References
- Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)
92d2b57 to
69b87cf
Compare
### Description - Adds mcpapps experiment flag to src/experiments.ts. - Adds applyAppMeta helper function to src/mcp/util.ts to conditionally add UI metadata. - Adds unit tests for applyAppMeta in src/mcp/util.spec.ts. ### Scenarios Tested - Unit tests passed. - Build succeeds.
### Description - Fixes applyAppMeta to preserve existing metadata. - Moves mcpapps flag to be grouped with other MCP experiments. - Removes as any in util.spec.ts by importing CallToolResult. ### Scenarios Tested - Build succeeds. - Lint passes for modified files (ignoring pre-existing warnings). - Unit tests for applyAppMeta pass.
Adds support for returning structured content from tools, which is used by MCP Apps to pass complex data to the host. Also updates the resource index. - Verified build and file changes.
### Description Adds the Update Environment MCP App, allowing users to switch projects and directories from the UI. ### Scenarios Tested - Verified build and file changes.
69b87cf to
a5b2a79
Compare
Adds the Update Environment MCP App. (Chained off #10259)