Skip to content

fix(assistant): clearer proposal connect button + host-supplied integration icons#180

Merged
Tjemmmic merged 3 commits into
mainfrom
fix/proposal-connect-affordance-and-icons
Jul 9, 2026
Merged

fix(assistant): clearer proposal connect button + host-supplied integration icons#180
Tjemmmic merged 3 commits into
mainfrom
fix/proposal-connect-affordance-and-icons

Conversation

@Tjemmmic

@Tjemmmic Tjemmmic commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Problem

Two UX bugs in the workflow-proposal integration requirement rows (tangle-network/agent-dev-container#3351, items 1 & 2):

  • The connect affordance rendered as plain primary-colored text with an arrow, reading as a link rather than a button.
  • Every integration provider (github, slack, gmail, notion, …) showed a letter-monogram instead of its brand icon, because the card used ProviderLogo, whose marks are LLM model providers.

Changes

  • Restyle the connect affordance as an outlined button (border, padding, hover), and drop the link-style arrow.
  • Add an optional renderProviderIcon(provider) prop, threaded AssistantDock → AssistantPanel → ProposalCard the same way renderGraph is, so a host can render integration marks from its own integrations catalog (the single source of truth) while the assistant stays host-agnostic. When the prop is absent, or returns a nullish node for a provider the host doesn't recognize, the row falls back to the built-in ProviderLogo — no behaviour change for hosts that don't wire it.

Host follow-up

platform-web will pass renderProviderIcon (reusing its integrations-catalog icon logic) after this releases; its ^0.43.x range already allows the new version, so no dependency bump is needed there beyond pnpm update.

Testing

  • 250 assistant tests pass, including two new cases for the injected-icon path and the ProviderLogo fallback.
  • Existing proposal-card connect-button tests unchanged and green.

Tjemmmic added 2 commits July 9, 2026 13:34
…l requirements

The proposal card rendered every integration requirement with `ProviderLogo`,
whose marks are LLM model providers (anthropic, openai, …) — so integration
providers (github, slack, gmail, notion, …) all fell back to a letter monogram
instead of their brand icon.

Add an optional `renderProviderIcon(provider)` prop, threaded
AssistantDock → AssistantPanel → ProposalCard the same way `renderGraph` is, so
a host can render integration marks from its own integrations catalog (the
single source of truth for provider icons) while the assistant stays
host-agnostic. When the prop is absent, or returns a nullish node for a provider
the host doesn't recognize, the row falls back to the built-in `ProviderLogo`
mark — no behaviour change for hosts that don't wire it.
@Tjemmmic

This comment has been minimized.

…sh the card

`renderProviderIcon` is an untrusted host callback invoked during render. If a
host throws for a provider it doesn't recognize (e.g. indexing a catalog without
guarding an unknown id), the error would unwind the whole proposal card. Compute
the icon under try/catch and degrade a throw to the built-in `ProviderLogo` mark
— the same containment `onConnect` already gets — so one bad provider mark can't
take down the row.
@Tjemmmic

Tjemmmic commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

🤖 AI Code Review (ensemble)

Resolution (2933f6c): 1 acknowledged.

Summary

This PR threads an optional host-supplied renderProviderIcon prop through the assistant component hierarchy into ProposalCard, allowing the host app to override the built-in provider mark, while also restyling the connect button with a clearer pill affordance and removing the arrow character. The implementation is clean: the untrusted host callback is guarded with a try/catch and nullish fallback to ProviderLogo, and the new behavior is covered by three focused tests. The only notable finding is a P2 about silent error swallowing in the catch block.

Issues Found

1 total — 0 P1 (blocking) · 1 P2 (should fix) · 0 P3 (nice to have)

⚠️ P2 — Provider icon error is silently swallowed with no logging ℹ️ Acknowledged

Note: Left as-is by design. The catch runs in the render path, so a console.error there would fire on every render when a host callback throws deterministically — a side-effect-in-render antipattern and console noise. The sibling untrusted host callback onConnect is contained silently the same way; the benign fallback to the built-in mark is the intended behavior. Consistent logging for untrusted host callbacks would be a separate, deliberate change across both call sites.

  • File: src/assistant/ProposalCard.tsx:289-291
  • Problem: The catch block silently discards errors from the host callback with an empty catch {}. While falling back to ProviderLogo is correct for resilience, leaving zero trace makes host integration debugging painful — a host whose renderProviderIcon throws for certain providers will see the fallback icon with no indication anything went wrong. This runs on every render for every requirement row, so a systematic host bug would be silently masked across all rows.
  • Fix: Add a console.error breadcrumb so host developers can diagnose issues during integration:
try {
  providerIcon = renderProviderIcon?.(req.provider) ?? null;
} catch (err) {
  console.error("renderProviderIcon threw for provider", req.provider, err);
  providerIcon = null;
}

This mirrors the standard defensive-library pattern: degrade gracefully but leave a trace.


✅ APPROVE

The implementation is solid across the board: the prop threading is clean, the try/catch guard around the untrusted host callback is well-reasoned, the nullish-coalesce fallback to ProviderLogo is correct, the button restyle improves affordance clarity without altering the connection contract, and the three new tests cover happy path, absent/null fallback, and throw fallback. The single P2 (silent error swallowing) is a reasonable follow-up that doesn't block merge — with fewer than 3 P2s and no P1s, this is an approve.

Quick Reference

  • P2: Provider icon error is silently swallowed with no logging

Synthesized by Sokuza AI from multiple independent reviewers

@Tjemmmic Tjemmmic marked this pull request as ready for review July 9, 2026 22:39
@Tjemmmic Tjemmmic merged commit 9046af0 into main Jul 9, 2026
1 check 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.

1 participant