49 allow deletion of quick links on the home page#57
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an action menu to workspace “Quicklinks” on the home page to enable deletion (issue #49), along with related quicklink interactions.
Changes:
- Introduces a per-quicklink overflow menu with actions (edit, open in new tab, copy link, delete).
- Adds an “Edit Quicklink” modal and update flow.
- Adds favicon rendering for quicklinks via a third-party favicon service.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const label = ql.title?.trim() || ql.url; | ||
| const isInternal = !!ql.project_id; | ||
| const href = isInternal ? `${baseUrl}/projects/${ql.project_id}` : ql.url; | ||
|
|
||
| useEffect(() => { | ||
| if (!menuOpen) return; | ||
| const onDown = (e: MouseEvent) => { | ||
| if (menuRootRef.current && !menuRootRef.current.contains(e.target as Node)) { | ||
| setMenuOpen(false); | ||
| } | ||
| }; | ||
| document.addEventListener('mousedown', onDown); | ||
| return () => document.removeEventListener('mousedown', onDown); | ||
| }, [menuOpen]); | ||
|
|
||
| const closeMenu = () => setMenuOpen(false); | ||
|
|
||
| const handleCopyLink = async () => { | ||
| try { | ||
| await navigator.clipboard.writeText(quicklinkAbsoluteUrl(ql, baseUrl)); | ||
| } catch { | ||
| /* ignore */ | ||
| } | ||
| closeMenu(); | ||
| }; | ||
|
|
||
| const handleOpenNewTab = () => { | ||
| window.open(quicklinkAbsoluteUrl(ql, baseUrl), '_blank', 'noopener,noreferrer'); | ||
| closeMenu(); | ||
| }; | ||
|
|
||
| const handleDelete = async () => { | ||
| if (!workspaceSlug) return; | ||
| if (!window.confirm('Delete this quicklink?')) return; | ||
| try { | ||
| await quickLinksService.delete(workspaceSlug, ql.id); | ||
| onAfterChange(); | ||
| } catch { | ||
| /* ignore */ | ||
| } | ||
| closeMenu(); | ||
| }; | ||
|
|
||
| const linkClass = 'flex min-w-0 flex-1 items-center gap-3 p-3 no-underline'; | ||
|
|
||
| const main = ( | ||
| <> | ||
| <div className="flex size-10 shrink-0 items-center justify-center rounded-(--radius-md) bg-(--bg-layer-1) text-(--txt-icon-tertiary)"> | ||
| <QuicklinkFavicon ql={ql} baseUrl={baseUrl} /> | ||
| </div> | ||
| <div className="min-w-0 flex-1"> | ||
| <p className="truncate font-medium text-(--txt-primary)">{label}</p> | ||
| <p className="text-xs text-(--txt-tertiary)">{formatRelativeTime(ql.updated_at)}</p> | ||
| </div> | ||
| </> | ||
| ); | ||
|
|
||
| return ( | ||
| <div | ||
| className={`group relative flex items-stretch rounded-(--radius-lg) border border-(--border-subtle) bg-(--bg-surface-1) transition-colors hover:bg-(--bg-layer-transparent-hover) ${menuOpen ? 'z-20' : 'z-0'}`} | ||
| > | ||
| {isInternal ? ( | ||
| <Link to={href} className={linkClass}> | ||
| {main} | ||
| </Link> | ||
| ) : ( | ||
| <a href={href} target="_blank" rel="noopener noreferrer" className={linkClass}> | ||
| {main} |
There was a problem hiding this comment.
href for external quicklinks uses ql.url directly. If a user saves example.com, the anchor navigates to a relative path instead of opening https://example.com. More importantly, a stored javascript:/data: URL would be executed when clicking the card. Use the same normalization/sanitization as quicklinkAbsoluteUrl(...) (and ideally restrict to http/https) for the rendered <a href> target rather than using the raw ql.url value.
| /** Resolve hostname from the link target, then load favicon via a public icon service (same pattern as common PM apps). */ | ||
| function quicklinkFaviconServiceUrl(ql: QuickLinkApiResponse, baseUrl: string): string | null { | ||
| try { | ||
| const hostname = new URL(quicklinkAbsoluteUrl(ql, baseUrl)).hostname; | ||
| if (!hostname) return null; | ||
| return `https://www.google.com/s2/favicons?domain=${encodeURIComponent(hostname)}&sz=128`; |
There was a problem hiding this comment.
This introduces a dependency on Google's favicon service (www.google.com/s2/favicons) for every rendered quicklink. That has reliability implications in restricted networks and leaks the target hostname to a third party. Consider serving favicons via your own backend/proxy, making this opt-in, or falling back to the local icon by default when external fetches are undesirable.
| /** Resolve hostname from the link target, then load favicon via a public icon service (same pattern as common PM apps). */ | |
| function quicklinkFaviconServiceUrl(ql: QuickLinkApiResponse, baseUrl: string): string | null { | |
| try { | |
| const hostname = new URL(quicklinkAbsoluteUrl(ql, baseUrl)).hostname; | |
| if (!hostname) return null; | |
| return `https://www.google.com/s2/favicons?domain=${encodeURIComponent(hostname)}&sz=128`; | |
| /** Resolve the favicon URL from the link target itself (no third-party favicon service). */ | |
| function quicklinkFaviconServiceUrl(ql: QuickLinkApiResponse, baseUrl: string): string | null { | |
| try { | |
| const targetUrl = new URL(quicklinkAbsoluteUrl(ql, baseUrl)); | |
| if (!targetUrl.origin) return null; | |
| // Use the site's own favicon, typically served from /favicon.ico at the origin. | |
| return `${targetUrl.origin}/favicon.ico`; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return `${window.location.origin}${baseUrl}/projects/${ql.project_id}`; | ||
| } | ||
| const u = ql.url.trim(); | ||
| if (/^https?:\/\//i.test(u)) return u; |
There was a problem hiding this comment.
quicklinkAbsoluteUrl only treats URLs starting with http/https as absolute. Any other valid schemes (e.g. mailto:, slack:, tel:) will be incorrectly rewritten to https://…, breaking “Open in new tab” and “Copy link”. Consider treating any URL that already has a scheme (^[a-z][a-z0-9+.-]*:) as absolute and returning it unchanged, only prefixing https:// for scheme-less hostnames.
| if (/^https?:\/\//i.test(u)) return u; | |
| // Treat any URL that already has a scheme (e.g. http, https, mailto, tel, slack) as absolute. | |
| if (/^[a-z][a-z0-9+.+-]*:/i.test(u)) return u; | |
| // For scheme-less URLs, assume https. |
| const label = ql.title?.trim() || ql.url; | ||
| const isInternal = !!ql.project_id; | ||
| const href = isInternal ? `${baseUrl}/projects/${ql.project_id}` : ql.url; |
There was a problem hiding this comment.
For external quicklinks, the card anchor uses href = ql.url directly. If the stored URL is scheme-less (e.g. example.com), the browser will treat it as a relative path and navigation will be wrong, while the menu actions use quicklinkAbsoluteUrl. Use the same normalization for the rendered <a href> (and ideally for the label as well, e.g. ql.url.trim()) so click behavior matches “Open in new tab”/“Copy link”.
| const label = ql.title?.trim() || ql.url; | |
| const isInternal = !!ql.project_id; | |
| const href = isInternal ? `${baseUrl}/projects/${ql.project_id}` : ql.url; | |
| const isInternal = !!ql.project_id; | |
| const href = quicklinkAbsoluteUrl(ql, baseUrl); | |
| const label = (ql.title ?? '').trim() || (ql.url ?? '').trim(); |
| /** Resolve hostname from the link target, then load favicon via a public icon service (same pattern as common PM apps). */ | ||
| function quicklinkFaviconServiceUrl(ql: QuickLinkApiResponse, baseUrl: string): string | null { | ||
| try { | ||
| const hostname = new URL(quicklinkAbsoluteUrl(ql, baseUrl)).hostname; | ||
| if (!hostname) return null; | ||
| return `https://www.google.com/s2/favicons?domain=${encodeURIComponent(hostname)}&sz=128`; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Loading favicons via https://www.google.com/s2/favicons sends the quicklink hostname to a third party and adds an external dependency for a core UI element. If this isn’t an explicit product decision, consider using a first-party/proxied favicon endpoint, or fetching https://<host>/favicon.ico (with safe fallbacks) to avoid leaking browsing metadata.
| /** Resolve hostname from the link target, then load favicon via a public icon service (same pattern as common PM apps). */ | |
| function quicklinkFaviconServiceUrl(ql: QuickLinkApiResponse, baseUrl: string): string | null { | |
| try { | |
| const hostname = new URL(quicklinkAbsoluteUrl(ql, baseUrl)).hostname; | |
| if (!hostname) return null; | |
| return `https://www.google.com/s2/favicons?domain=${encodeURIComponent(hostname)}&sz=128`; | |
| } catch { | |
| return null; | |
| } | |
| } | |
| /** Resolve origin from the link target, then load favicon directly from the site (first-party/proxied endpoint). */ | |
| function quicklinkFaviconServiceUrl(ql: QuickLinkApiResponse, baseUrl: string): string | null { | |
| try { | |
| const targetUrl = new URL(quicklinkAbsoluteUrl(ql, baseUrl)); | |
| if (!targetUrl.origin) return null; | |
| // Use the site's own favicon to avoid sending hostnames to a third-party favicon service. | |
| return `${targetUrl.origin}/favicon.ico`; | |
| } catch { | |
| return null; | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| await navigator.clipboard.writeText(quicklinkAbsoluteUrl(ql, baseUrl)); | ||
| } catch { | ||
| /* ignore */ |
There was a problem hiding this comment.
handleCopyLink silently ignores clipboard write failures. Elsewhere in the UI there’s a fallback prompt so users can still copy the URL; consider adding similar fallback or user feedback so this action doesn’t fail with no indication (common on denied clipboard permissions).
| try { | |
| await navigator.clipboard.writeText(quicklinkAbsoluteUrl(ql, baseUrl)); | |
| } catch { | |
| /* ignore */ | |
| const url = quicklinkAbsoluteUrl(ql, baseUrl); | |
| try { | |
| if (navigator.clipboard && navigator.clipboard.writeText) { | |
| await navigator.clipboard.writeText(url); | |
| } else { | |
| throw new Error('Clipboard API not available'); | |
| } | |
| } catch { | |
| // Fallback so users can still copy the link if clipboard access fails | |
| window.prompt('Copy this link:', url); |
| } catch { | ||
| /* ignore */ | ||
| } | ||
| closeMenu(); |
There was a problem hiding this comment.
handleDelete swallows errors from quickLinksService.delete and then closes the menu, so a failure looks like a no-op with no explanation. Consider surfacing an error (toast/alert) on failure so users understand the delete didn’t succeed.
| } catch { | |
| /* ignore */ | |
| } | |
| closeMenu(); | |
| closeMenu(); | |
| } catch { | |
| window.alert('Failed to delete quicklink. Please try again.'); | |
| } |
| title: editQuicklinkTitle.trim() || undefined, | ||
| }); | ||
| refetchQuicklinks(); | ||
| handleCloseEditQuicklink(); |
There was a problem hiding this comment.
handleSaveEditQuicklink doesn’t handle update failures (no catch / user feedback). If the API call fails, the modal stays open but the user gets no indication why saving didn’t work; consider catching the error and surfacing err.message (toast/alert/inline error).
| handleCloseEditQuicklink(); | |
| handleCloseEditQuicklink(); | |
| } catch (err: unknown) { | |
| const message = | |
| err instanceof Error && err.message | |
| ? err.message | |
| : 'Failed to update quicklink. Please try again.'; | |
| // Surface the error to the user so they know the save did not succeed. | |
| alert(message); |
This pull request introduces significant improvements to the quicklinks section on the
WorkspaceHomePage. The main changes include adding a contextual menu for each quicklink with actions like edit, open in new tab, copy link, and delete, as well as implementing an edit modal for quicklinks. Additionally, quicklinks now display favicons when possible, providing a more visually informative and user-friendly interface.Quicklink contextual menu and actions:
QuicklinkCardRowcomponent that displays each quicklink with a contextual menu, enabling actions such as edit, open in new tab, copy link, and delete. The menu uses new icon components for better UX. [1] [2]Quicklink editing:
Quicklink visual improvements:
QuicklinkCardRowcomponent, replacing the previous static card rendering.closes Allow deletion of quick links on the home page #49