Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a visual interface for Firebase deployment within the Model Context Protocol (MCP) framework, featuring a new web-based UI and status polling mechanism. It also includes a safety check for Firestore index deployment and updates utility functions to support structured content. The review feedback highlights several critical improvements for the UI logic, specifically regarding the inefficient and error-prone re-rendering of logs during polling. Additionally, multiple comments point out violations of the repository's style guide concerning the use of "any" types and the preference for centralized logging over "console.error".
| statusList.innerHTML = ""; | ||
| job.logs.forEach((log: string) => addLog(log)); |
There was a problem hiding this comment.
Clearing and re-rendering the entire log list on every poll interval has several issues:
- Performance: DOM manipulation becomes increasingly expensive as the log history grows.
- Correctness: The
addLogfunction generates a timestamp usingnew Date(). Re-rendering all logs causes every entry to display the time of the latest poll rather than when the log was actually received. - Data Loss: Local logs added before polling starts (e.g., the 'Starting deployment' message at line 86) are lost when
innerHTMLis cleared.
Consider tracking the number of logs already displayed and only appending new ones.
src/mcp/apps/deploy/mcp-app.ts
Outdated
| } catch (err: any) { | ||
| console.error("Failed to connect app:", err); |
There was a problem hiding this comment.
Using console.error for user-facing error reporting is discouraged by the repository style guide (line 29). In this UI context, consider using the addLog function to display the error to the user.
| } catch (err: any) { | |
| console.error("Failed to connect app:", err); | |
| } catch (err) { | |
| addLog("Failed to connect app: " + (err instanceof Error ? err.message : String(err)), "error"); |
References
- Use the central logger; never use console.log() for user-facing output. (link)
5c96996 to
ff684e7
Compare
8ebae16 to
a7fd37b
Compare
34837da to
df9b936
Compare
Adds the Deploy MCP App UI and resources. Also includes a small fix for firestore deploy to avoid errors on empty indexes. - Verified build and file changes.
Adds the Deploy MCP App UI. (Chained off #10262)