feat: add site icons to sidebar with fallback colors#2651
Draft
shaunandrews wants to merge 5 commits intotrunkfrom
Draft
feat: add site icons to sidebar with fallback colors#2651shaunandrews wants to merge 5 commits intotrunkfrom
shaunandrews wants to merge 5 commits intotrunkfrom
Conversation
Fetch WordPress site icons via WP-CLI (site_icon_url option) with favicon.ico fallback. Cache as 64x64 PNGs alongside thumbnails. Wire up IPC events and handlers for loading, copying, and cleanup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add SiteIcon component that renders the cached icon or a letter avatar. Extend ThemeDetailsContext with siteIcons state and update SiteItem to display the icon before each site name. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ning sites The WP-CLI command `wp option get site_icon_url` doesn't work because `site_icon_url` isn't a real WordPress option (the actual option is `site_icon`, an attachment ID). Replace with `wp eval` calling `get_site_icon_url(512)` which resolves the URL properly. Also remove the broken `/favicon.ico` fallback (WordPress doesn't serve a physical favicon.ico), add error logging for icon fetch failures, refresh all running sites on window focus (not just selected), and refresh on site selection change. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Increase site icon from 16px to 28px, widen sidebar from 208px to 248px, and clean up sidebar spacing (remove left margin on site list, move icon inside the button for better click targeting). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sites without a custom icon now get a distinguishable muted color from an 8-color palette instead of a generic grey placeholder. Color is assigned at creation time and persisted to appdata. Existing sites get a deterministic color derived from their ID. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function fetchImageBuffer( url: string ): Promise< Buffer > { | ||
| return new Promise( ( resolve, reject ) => { | ||
| const protocol = url.startsWith( 'https' ) ? https : http; | ||
| const request = protocol.get( url, { timeout: 5000, rejectUnauthorized: false }, ( res ) => { |
Check failure
Code scanning / CodeQL
Disabling certificate validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 days ago
In general, the problem is caused by explicitly setting rejectUnauthorized: false on an HTTPS request, which tells Node.js to skip verification of the server’s TLS certificate. The fix is to stop disabling this check: either remove the option entirely (so the default secure behavior applies) or explicitly set rejectUnauthorized: true. If there are legitimate issues with self‑signed or custom certificates, those should be handled via a proper CA configuration (e.g., ca option), not by turning off verification.
For this specific code, the best fix with minimal behavior change is:
- Only pass TLS‑specific options when using HTTPS.
- For HTTPS, either omit
rejectUnauthorizedor set it totrue. Since the code doesn’t show any custom CA handling, omitting it and relying on the default (which istrue) is simplest and standard. - For HTTP, keep the current behavior (no TLS options).
Concretely:
- In
apps/studio/src/site-server.ts, infetchImageBuffer, replace the singleprotocol.getcall that passes{ timeout: 5000, rejectUnauthorized: false }with logic that:- Calls
http.get(url, { timeout: 5000 }, ...)for HTTP URLs. - Calls
https.get(url, { timeout: 5000 }, ...)for HTTPS URLs, withoutrejectUnauthorized: false.
- Calls
No new imports or helper functions are strictly necessary.
Suggested changeset
1
apps/studio/src/site-server.ts
| @@ -82,26 +82,30 @@ | ||
|
|
||
| function fetchImageBuffer( url: string ): Promise< Buffer > { | ||
| return new Promise( ( resolve, reject ) => { | ||
| const protocol = url.startsWith( 'https' ) ? https : http; | ||
| const request = protocol.get( url, { timeout: 5000, rejectUnauthorized: false }, ( res ) => { | ||
| if ( | ||
| res.statusCode && | ||
| res.statusCode >= 300 && | ||
| res.statusCode < 400 && | ||
| res.headers.location | ||
| ) { | ||
| fetchImageBuffer( res.headers.location ).then( resolve, reject ); | ||
| return; | ||
| const isHttps = url.startsWith( 'https' ); | ||
| const request = ( isHttps ? https : http ).get( | ||
| url, | ||
| { timeout: 5000 }, | ||
| ( res ) => { | ||
| if ( | ||
| res.statusCode && | ||
| res.statusCode >= 300 && | ||
| res.statusCode < 400 && | ||
| res.headers.location | ||
| ) { | ||
| fetchImageBuffer( res.headers.location ).then( resolve, reject ); | ||
| return; | ||
| } | ||
| if ( ! res.statusCode || res.statusCode >= 400 ) { | ||
| reject( new Error( `HTTP ${ res.statusCode } fetching ${ url }` ) ); | ||
| return; | ||
| } | ||
| const chunks: Buffer[] = []; | ||
| res.on( 'data', ( chunk: Buffer ) => chunks.push( chunk ) ); | ||
| res.on( 'end', () => resolve( Buffer.concat( chunks ) ) ); | ||
| res.on( 'error', reject ); | ||
| } | ||
| if ( ! res.statusCode || res.statusCode >= 400 ) { | ||
| reject( new Error( `HTTP ${ res.statusCode } fetching ${ url }` ) ); | ||
| return; | ||
| } | ||
| const chunks: Buffer[] = []; | ||
| res.on( 'data', ( chunk: Buffer ) => chunks.push( chunk ) ); | ||
| res.on( 'end', () => resolve( Buffer.concat( chunks ) ) ); | ||
| res.on( 'error', reject ); | ||
| } ); | ||
| ); | ||
| request.on( 'timeout', () => { | ||
| request.destroy(); | ||
| reject( new Error( `Timeout fetching ${ url }` ) ); |
Copilot is powered by AI and may make mistakes. Always verify output.
Collaborator
📊 Performance Test ResultsComparing c62ecdf vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
get_site_icon_url()appdata-v1.jsoniconColorIndexget a deterministic color derived from hashing their site ID (no migration needed)Test plan