Keep the built-in provider icon fallbacks - #1732
Conversation
|
🚨 SLOP COP 🚨 · I am the SlopCop. I am reviewing this pull request for security, code quality, performance, architecture, and user behavior. |
2a67a10 to
8164992
Compare
8164992 to
84a3a73
Compare
ymichael
changed the title
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
| const pluginOwnedLabel = PLUGIN_OWNED_PROVIDER_ICON_LABELS.get(providerId); | ||
| if (pluginOwnedLabel !== undefined) { | ||
| return { | ||
| icon: PendingPluginProviderIcon, |
There was a problem hiding this comment.
🚨 slopcop/review — The full app test suite now fails after this placeholder replaces the SVG fallback.
SkillsView.test.tsx:659 expects the Codex filter to contain an SVG. SidebarUpdatesBadge.test.tsx:218 expects three provider SVG marks.
Both tests fail locally and in the app CI job. Please register provider icons in these tests or update their placeholder expectations.
| * document where `currentColor` resolves to black — invisible on dark | ||
| * themes — and page CSS cannot reach it. | ||
| * 3. A caller-supplied `logoUrl` (from a server-provided `ProviderInfo`) for | ||
| * 2. An empty, size-preserving placeholder for the first-party providers |
There was a problem hiding this comment.
🚨 slopcop/review — The public API audit guide still promises the removed vendored-map fallback.
docs/api_to_audit.md says a failed reload falls back to the vendored map. This code now returns an empty slot.
Please update the audit guide and its bundle-size note to describe the new fallback order.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain English summary
This pull request removes four provider icon copies from the main app. Each provider plugin now supplies its own icon.
The app shows an empty icon space until the plugin loads.
Findings
- High: The full app test suite fails in two places.
- The new fallback creates spans, but the tests still require SVG provider marks.
- Low:
docs/api_to_audit.mdstill promises the deleted vendored-map fallback.
Security
I found no security issue. The new paths use fixed repository assets. The SVG files contain no active content.
Performance
I found no performance problem. The production build passed the app bundle budget. The change removes 5,342 bytes of duplicate production source.
Architecture
Identical icon paths remain in Automations, Tasks, Web, and the provider plugins. A shared package could reduce duplication but add plugin dependencies.
Three comments still name the deleted app icon files as their source. These comments should point to provider-owned assets.
Validation
- The focused provider icon tests passed: 9 of 9.
- The app typecheck passed.
- The production app build passed.
- The dark-theme Ladle picker loaded Codex, Claude, and Pi icons at the correct size.
- All checked picker images loaded without errors.
- The two affected test files failed locally: 2 failed and 42 passed.
- The app CI job reports the same two failures.
The production behavior looks correct. The two failing tests and the stale API guide need updates.
I posted this as a comment review only.
ymichael
changed the title
1 participant
Closes #1723
Summary
Validation
pnpm exec turbo run test --filter=@bb/app -- --run src/lib/provider-icon.test.tsx(5 passing)