-
Notifications
You must be signed in to change notification settings - Fork 0
Add app icon display to Preprod Build Details sidebar #92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: ryan/add_api_to_serve_preprod_app_icons
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| from sentry.objectstore.service import ClientBuilder | ||
|
|
||
| attachments = ClientBuilder("attachments") | ||
| app_icons = ClientBuilder("app-icons") |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,140 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import logging | ||
|
|
||
| from django.http import HttpResponse | ||
| from rest_framework.request import Request | ||
|
|
||
| from sentry.api.api_owners import ApiOwner | ||
| from sentry.api.api_publish_status import ApiPublishStatus | ||
| from sentry.api.base import region_silo_endpoint | ||
| from sentry.api.bases.project import ProjectEndpoint | ||
| from sentry.models.project import Project | ||
| from sentry.objectstore import app_icons | ||
| from sentry.objectstore.service import ClientError | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def detect_image_content_type(image_data: bytes) -> str: | ||
| """ | ||
| Detect the content type of an image from its magic bytes. | ||
| Returns the appropriate MIME type or a default if unknown. | ||
| """ | ||
| if not image_data: | ||
| return "application/octet-stream" | ||
|
|
||
| # Check magic bytes for common image formats | ||
| if image_data[:8] == b"\x89PNG\r\n\x1a\n": | ||
| return "image/png" | ||
| elif image_data[:3] == b"\xff\xd8\xff": | ||
| return "image/jpeg" | ||
| elif image_data[:4] == b"RIFF" and image_data[8:12] == b"WEBP": | ||
| return "image/webp" | ||
| elif image_data[:2] in (b"BM", b"BA", b"CI", b"CP", b"IC", b"PT"): | ||
| return "image/bmp" | ||
| elif image_data[:6] in (b"GIF87a", b"GIF89a"): | ||
| return "image/gif" | ||
| elif image_data[:4] == b"\x00\x00\x01\x00": | ||
| return "image/x-icon" | ||
| elif len(image_data) >= 12 and image_data[4:12] in (b"ftypavif", b"ftypavis"): | ||
| return "image/avif" | ||
| elif len(image_data) >= 12 and image_data[4:12] in ( | ||
| b"ftypheic", | ||
| b"ftypheix", | ||
| b"ftyphevc", | ||
| b"ftyphevx", | ||
| ): | ||
| return "image/heic" | ||
|
|
||
| # Default to generic binary if we can't detect the type | ||
| logger.warning( | ||
| "Could not detect image content type from magic bytes", | ||
| extra={"first_bytes": image_data[:16].hex() if len(image_data) >= 16 else image_data.hex()}, | ||
| ) | ||
| return "application/octet-stream" | ||
|
|
||
|
|
||
| @region_silo_endpoint | ||
| class ProjectPreprodArtifactIconEndpoint(ProjectEndpoint): | ||
| owner = ApiOwner.EMERGE_TOOLS | ||
| publish_status = { | ||
| "GET": ApiPublishStatus.EXPERIMENTAL, | ||
| } | ||
|
|
||
| def get( | ||
| self, | ||
| request: Request, | ||
| project: Project, | ||
| app_icon_id: str, | ||
| ) -> HttpResponse: | ||
| organization_id = project.organization_id | ||
| project_id = project.id | ||
|
|
||
| # object_key = f"{organization_id}/{project_id}/{app_icon_id}" | ||
| logger.info( | ||
| "Retrieving app icon from objectstore", | ||
| extra={ | ||
| "organization_id": organization_id, | ||
| "project_id": project_id, | ||
| "app_icon_id": app_icon_id, | ||
| }, | ||
| ) | ||
| client = app_icons.for_project(organization_id, project_id) | ||
|
|
||
| try: | ||
| result = client.get(app_icon_id) | ||
| # Read the entire stream at once | ||
| image_data = result.payload.read() | ||
|
|
||
| # Detect content type from the image data | ||
| content_type = detect_image_content_type(image_data) | ||
|
|
||
| logger.info( | ||
| "Retrieved app icon from objectstore", | ||
| extra={ | ||
| "organization_id": organization_id, | ||
| "project_id": project_id, | ||
| "app_icon_id": app_icon_id, | ||
| "size_bytes": len(image_data), | ||
| "content_type": content_type, | ||
| }, | ||
| ) | ||
| return HttpResponse(image_data, content_type=content_type) | ||
|
|
||
| except ClientError as e: | ||
| if e.status == 404: | ||
| logger.warning( | ||
| "App icon not found in objectstore", | ||
| extra={ | ||
| "organization_id": organization_id, | ||
| "project_id": project_id, | ||
| "app_icon_id": app_icon_id, | ||
| }, | ||
| ) | ||
|
|
||
| # Upload failed, return appropriate error | ||
| return HttpResponse({"error": "Not found"}, status=404) | ||
|
|
||
| logger.warning( | ||
| "Failed to retrieve app icon from objectstore", | ||
| extra={ | ||
| "organization_id": organization_id, | ||
| "project_id": project_id, | ||
| "app_icon_id": app_icon_id, | ||
| "error": str(e), | ||
| "status": e.status, | ||
| }, | ||
| ) | ||
| return HttpResponse({"error": "Failed to retrieve app icon"}, status=500) | ||
|
|
||
| except Exception: | ||
| logger.exception( | ||
| "Unexpected error retrieving app icon", | ||
| extra={ | ||
| "organization_id": organization_id, | ||
| "project_id": project_id, | ||
| "app_icon_id": app_icon_id, | ||
| }, | ||
| ) | ||
| return HttpResponse({"error": "Internal server error"}, status=500) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ import {t} from 'sentry/locale'; | |
| import {formatBytesBase10} from 'sentry/utils/bytes/formatBytesBase10'; | ||
| import {getFormattedDate} from 'sentry/utils/dates'; | ||
| import {unreachable} from 'sentry/utils/unreachable'; | ||
| import useOrganization from 'sentry/utils/useOrganization'; | ||
| import {openInstallModal} from 'sentry/views/preprod/components/installModal'; | ||
| import { | ||
| BuildDetailsSizeAnalysisState, | ||
|
|
@@ -70,14 +71,21 @@ interface BuildDetailsSidebarAppInfoProps { | |
| } | ||
|
|
||
| export function BuildDetailsSidebarAppInfo(props: BuildDetailsSidebarAppInfoProps) { | ||
| const organization = useOrganization(); | ||
| const labels = getLabels(props.appInfo.platform ?? undefined); | ||
|
|
||
| let iconUrl = null; | ||
| if (props.appInfo.app_icon_id) { | ||
| iconUrl = `/api/0/projects/${organization.slug}/${props.projectId}/files/app-icons/${props.appInfo.app_icon_id}/`; | ||
| } | ||
|
|
||
| return ( | ||
| <Flex direction="column" gap="xl"> | ||
| <Flex align="center" gap="sm"> | ||
| <AppIcon> | ||
| {iconUrl && <img src={iconUrl} alt="App Icon" width={24} height={24} />} | ||
| {!iconUrl && ( | ||
| <AppIconPlaceholder>{props.appInfo.name?.charAt(0) || ''}</AppIconPlaceholder> | ||
| </AppIcon> | ||
| )} | ||
|
Comment on lines
+85
to
+88
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [BestPractice] Missing error handling for image loading: If the constructed const [imageError, setImageError] = useState(false);
// In the JSX:
{iconUrl && !imageError && (
<img
src={iconUrl}
alt="App Icon"
width={24}
height={24}
onError={() => setImageError(true)}
/>
)}
{(!iconUrl || imageError) && (
<AppIconPlaceholder>{props.appInfo.name?.charAt(0) || ''}</AppIconPlaceholder>
)}Note: You'll need to add Context for Agents |
||
| {props.appInfo.name && <Heading as="h3">{props.appInfo.name}</Heading>} | ||
| </Flex> | ||
|
|
||
|
|
@@ -187,19 +195,16 @@ export function BuildDetailsSidebarAppInfo(props: BuildDetailsSidebarAppInfoProp | |
| ); | ||
| } | ||
|
|
||
| const AppIcon = styled('div')` | ||
| const AppIconPlaceholder = styled('div')` | ||
| width: 24px; | ||
| height: 24px; | ||
| border-radius: 4px; | ||
| background: #ff6600; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| flex-shrink: 0; | ||
| `; | ||
|
|
||
| const AppIconPlaceholder = styled('div')` | ||
| color: white; | ||
| background: ${p => p.theme.purple400}; | ||
| color: ${p => p.theme.white}; | ||
| font-weight: ${p => p.theme.fontWeight.bold}; | ||
| font-size: ${p => p.theme.fontSize.sm}; | ||
| `; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[BestPractice]
Missing accessibility attributes: The
<img>element should include proper accessibility attributes for better screen reader support:⚡ Committable suggestion
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Context for Agents