Conversation
Co-authored-by: ghostleek <44336310+ghostleek@users.noreply.github.com>
…randing.ts Co-authored-by: ghostleek <44336310+ghostleek@users.noreply.github.com>
Fix premature pin on failed profile add
Extract OPAL branding helpers into shared module
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This pull request centralizes Opal branding normalization into a shared utility and adjusts profile UI logic to only pin apps after they’ve been successfully added to a profile.
Changes:
- Introduces
src/lib/branding.tsas the single source of truth forOPAL_CANONICAL_LOGOandnormalizeOpalLogo. - Updates edge API routes to import branding utilities instead of maintaining local copies.
- Fixes
PersonalProfilepinning flow to run only after a successful add.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/branding.ts | Adds centralized Opal logo constant + normalization helper. |
| api/apps.ts | Switches to shared normalizeOpalLogo import for app list responses. |
| api/profile/manage.ts | Switches to shared normalizeOpalLogo import for pinned app details. |
| api/profile/[slug].ts | Switches to shared OPAL_CANONICAL_LOGO import for pinned app logo override. |
| src/components/PersonalProfile.tsx | Pins an app only after addAppToProfile succeeds. |
| package-lock.json | Updates lockfile dependency metadata, including concurrently placement/flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
src/lib/branding.ts
Outdated
| export function normalizeOpalLogo<T extends { slug: string | null; logoUrl: string | null }>(app: T): T { | ||
| if (app.slug === 'opal') { | ||
| return { | ||
| ...app, | ||
| logoUrl: OPAL_CANONICAL_LOGO, | ||
| }; |
There was a problem hiding this comment.
normalizeOpalLogo is declared to return T, but it can change logoUrl to a string literal. If a caller ever uses a narrower T (e.g. logoUrl: null), the type signature becomes unsound and can mislead downstream code. Consider changing the return type to widen logoUrl (e.g. Omit<T, 'logoUrl'> & { logoUrl: string | null }) so the type accurately reflects the mutation.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| import { OPAL_CANONICAL_LOGO } from '../../src/lib/branding'; | ||
|
|
||
| export const config = { |
There was a problem hiding this comment.
This file still hard-codes Opal logo normalization inline (item.appSlug === 'opal' ? OPAL_CANONICAL_LOGO : ...). Since normalizeOpalLogo now exists as the centralized normalizer, consider applying it to the pinned-app objects here as well to avoid duplicating the normalization rule in multiple places.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@ghostleek I've opened a new pull request, #46, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@ghostleek I've opened a new pull request, #47, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: ghostleek <44336310+ghostleek@users.noreply.github.com>
Fix unsound return type on `normalizeOpalLogo`
This pull request refactors how the canonical Opal logo and normalization logic are managed across the codebase. The main improvement is centralizing the
OPAL_CANONICAL_LOGOconstant and thenormalizeOpalLogofunction in a new branding utility, reducing duplication and improving maintainability. Additionally, there is a minor fix in the app pinning logic in thePersonalProfilecomponent.Branding logic centralization:
OPAL_CANONICAL_LOGOconstant and thenormalizeOpalLogofunction to a new file,src/lib/branding.ts, to serve as a single source of truth for branding-related utilities.api/apps.tsandapi/profile/manage.tsto usenormalizeOpalLogofrom the new branding utility, and removed their local definitions. [1] [2]api/profile/[slug].tsto useOPAL_CANONICAL_LOGOfrom the branding utility, removing the local constant. (api/profile/[slug].tsR5-L11)Bug fix in component logic:
handleAddExistingAppfunction inPersonalProfile.tsxso that pinning logic only runs after a successful app addition.