Skip to content

fix: address post-merge review feedback from PR #219 - #227

Merged
wrenj merged 6 commits into
mainfrom
followup
Sep 24, 2026
Merged

wrenj merged 6 commits into
mainfrom
followup

Conversation

@wrenj

@wrenj wrenj commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Addresses the post-merge review comments on #219:

  1. Fold ng-basic-with-mcp-catalog into ng-basic-catalog & clean up README.md

    • Moved BasicWithMcpCatalog and its MCP function entries (listMcpApps, displayMcpApp) into samples/ng-basic-catalog (src/basic-catalog.ts and catalog.json) and deleted the duplicate samples/ng-basic-with-mcp-catalog workspace.
    • Removed the misplaced ### Angular Basic with MCP Renderer header from README.md so the ### Starting the A2UI Composer steps remain contiguous.
  2. Move doesCatalogSupportMcp to McpClientManagerService

    • Relocated doesCatalogSupportMcp out of catalog-storage.model.ts into McpClientManagerService (shell/src/app/mcp/mcp-client-manager.service.ts) and updated ChatService and unit tests accordingly.
  3. Pin zod version in bridge/package.json

    • Pinned "zod": "4.6.5" (removing the ^ range prefix) in bridge/package.json and updated yarn.lock.
  4. Settings MCP Servers UI improvements

    • Replaced the oversized Add Server flat button with a compact mat-icon-button using <mat-icon aria-hidden="true">add_circle</mat-icon> alongside the MCP Servers header.
    • Added inline editing (edit, check, close actions) for existing MCP server URLs backed by McpClientManagerService.updateServerUrl.
    • Added a Test button (mat-stroked-button) on each server entry to test connectivity on demand via McpClientManagerService.testServer.

@wrenj
wrenj requested a review from jgindin September 22, 2026 19:57

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request consolidates the MCP catalog sample by removing the standalone ng-basic-with-mcp-catalog workspace and integrating its MCP capabilities directly into the standard ng-basic-catalog sample. It also enhances the shell's settings view to support editing and testing MCP server connections, moving the MCP catalog support check into McpClientManagerService. Feedback on these changes highlights a few key improvements in McpClientManagerService: first, update the servers signal before disconnecting in updateServerUrl to avoid UI flickering; second, add a guard clause to prevent unnecessary reconnections when the URL is unchanged; and third, wrap the client closing logic in a finally block within testServer to prevent potential connection leaks.

wrenj added a commit that referenced this pull request Sep 24, 2026
- Return early in updateServerUrl when the trimmed URL matches the
  existing server URL.
- Update the servers signal and persist changes before awaiting
  disconnectServer in updateServerUrl to prevent UI flicker.
- Ensure client.close() is invoked inside a finally block in
  testServer to prevent leaked connections if listTools throws.
wrenj added a commit that referenced this pull request Sep 24, 2026
- Return early in updateServerUrl when the trimmed URL matches the
  existing server URL.
- Update the servers signal and persist changes before awaiting
  disconnectServer in updateServerUrl to prevent UI flicker.
- Ensure client.close() is invoked inside a finally block in
  testServer to prevent leaked connections if listTools throws.
@github-actions

github-actions Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

⚡ A2UI Composer PR Preview

Your automated preview is successfully live (commit 7162770):
👉 Launch PR Preview

Note: This environment will be wiped automatically when the PR is merged or closed.

- Fold BasicWithMcpCatalog and MCP function schemas into
  samples/ng-basic-catalog and remove the duplicate
  samples/ng-basic-with-mcp-catalog workspace.
- Restore README.md section structure under Starting the A2UI Composer.
- Move doesCatalogSupportMcp from catalog-storage.model.ts into
  McpClientManagerService.
- Pin zod dependency version to 4.6.5 without a caret prefix in
  bridge/package.json.
- Replace the oversized Add Server button in Settings with a compact
  add_circle icon button, add inline URL editing, and add a Test
  connection button.
- Return early in updateServerUrl when the trimmed URL matches the
  existing server URL.
- Update the servers signal and persist changes before awaiting
  disconnectServer in updateServerUrl to prevent UI flicker.
- Ensure client.close() is invoked inside a finally block in
  testServer to prevent leaked connections if listTools throws.
Use the gemini-flash-latest model alias instead of a pinned version in
Standalone3pLlmClient and update the corresponding unit test assertion.
- Add McpServerDialogComponent matching the Renderer and API Key dialogs
- Support optional custom display names in addServer and updateServerUrl
- Replace inline URL inputs in Settings with a header add button and
  clean empty state
- Update SettingsHarness and unit tests to cover dialog workflows
…-detect

- Remove Name input field from McpServerDialogComponent
- Always resolve MCP server display name from serverInfo.name on
  connection with URL fallback
- Update unit tests to match the URL-only dialog form
@wrenj
wrenj merged commit 4fe3349 into main Sep 24, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants