-
-
Notifications
You must be signed in to change notification settings - Fork 12
feat(browser): PR 9 follow-up — BookmarksBar + SitePermissionsPanel #313
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
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| import { describe, it, expect, beforeEach, vi, afterEach } from "vitest"; | ||
| import { render, screen, fireEvent, waitFor, act } from "@testing-library/react"; | ||
| import { BookmarksBar } from "./BookmarksBar"; | ||
| import * as bookmarksApi from "@/lib/browser-bookmarks-api"; | ||
| import { useBrowserStore } from "@/stores/browser-store"; | ||
|
|
||
| vi.mock("@/lib/browser-bookmarks-api"); | ||
|
|
||
| const WINDOW_ID = "win-bbar"; | ||
| const PROFILE_ID = "personal"; | ||
|
|
||
| function makeBookmark(overrides: Partial<bookmarksApi.Bookmark> = {}): bookmarksApi.Bookmark { | ||
| return { | ||
| bookmark_id: "bm-1", | ||
| url: "https://example.com", | ||
| title: "Example Site", | ||
| created_at: Date.now(), | ||
| ...overrides, | ||
| }; | ||
| } | ||
|
|
||
| beforeEach(() => { | ||
| vi.mocked(bookmarksApi.listBookmarks).mockResolvedValue([]); | ||
| vi.mocked(bookmarksApi.removeBookmark).mockResolvedValue(true); | ||
| useBrowserStore.setState({ windows: {} }); | ||
| useBrowserStore.getState().createWindow(WINDOW_ID, PROFILE_ID); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| describe("BookmarksBar", () => { | ||
| it("renders null (nothing) when bookmark list is empty", async () => { | ||
| vi.mocked(bookmarksApi.listBookmarks).mockResolvedValue([]); | ||
| const { container } = render( | ||
| <BookmarksBar windowId={WINDOW_ID} profileId={PROFILE_ID} />, | ||
| ); | ||
| // Wait for the async load to settle | ||
| await act(async () => {}); | ||
| expect(container.firstChild).toBeNull(); | ||
| }); | ||
|
|
||
| it("renders one chip per bookmark", async () => { | ||
| vi.mocked(bookmarksApi.listBookmarks).mockResolvedValue([ | ||
| makeBookmark({ bookmark_id: "bm-1", title: "Alpha", url: "https://alpha.com" }), | ||
| makeBookmark({ bookmark_id: "bm-2", title: "Beta", url: "https://beta.com" }), | ||
| ]); | ||
|
|
||
| render(<BookmarksBar windowId={WINDOW_ID} profileId={PROFILE_ID} />); | ||
| await waitFor(() => screen.getByText("Alpha")); | ||
| expect(screen.getByText("Beta")).toBeTruthy(); | ||
| }); | ||
|
|
||
| it("click on chip calls navigateTab with bookmark URL", async () => { | ||
| vi.mocked(bookmarksApi.listBookmarks).mockResolvedValue([ | ||
| makeBookmark({ bookmark_id: "bm-1", title: "Docs", url: "https://docs.example.com" }), | ||
| ]); | ||
| const navigateSpy = vi.spyOn(useBrowserStore.getState(), "navigateTab"); | ||
|
|
||
| render(<BookmarksBar windowId={WINDOW_ID} profileId={PROFILE_ID} />); | ||
| await waitFor(() => screen.getByText("Docs")); | ||
|
|
||
| fireEvent.click(screen.getByRole("button", { name: /Go to Docs/i })); | ||
| expect(navigateSpy).toHaveBeenCalledWith( | ||
| WINDOW_ID, | ||
| expect.any(String), | ||
| "https://docs.example.com", | ||
| ); | ||
| }); | ||
|
|
||
| it("right-click opens context menu, Remove calls removeBookmark with bookmark id", async () => { | ||
| vi.mocked(bookmarksApi.listBookmarks).mockResolvedValue([ | ||
| makeBookmark({ bookmark_id: "bm-42", title: "My Page", url: "https://my.example.com" }), | ||
| ]); | ||
|
|
||
| render(<BookmarksBar windowId={WINDOW_ID} profileId={PROFILE_ID} />); | ||
| await waitFor(() => screen.getByText("My Page")); | ||
|
|
||
| const chip = screen.getByRole("button", { name: /Go to My Page/i }); | ||
| fireEvent.contextMenu(chip); | ||
|
|
||
| const removeBtn = await waitFor(() => screen.getByRole("menuitem", { name: /Remove bookmark/i })); | ||
| await act(async () => { | ||
| fireEvent.click(removeBtn); | ||
| }); | ||
|
|
||
| expect(bookmarksApi.removeBookmark).toHaveBeenCalledWith(PROFILE_ID, "bm-42"); | ||
| }); | ||
|
|
||
| it("after successful remove the chip disappears", async () => { | ||
| vi.mocked(bookmarksApi.listBookmarks).mockResolvedValue([ | ||
| makeBookmark({ bookmark_id: "bm-1", title: "ToRemove", url: "https://remove.me" }), | ||
| ]); | ||
| vi.mocked(bookmarksApi.removeBookmark).mockResolvedValue(true); | ||
|
|
||
| render(<BookmarksBar windowId={WINDOW_ID} profileId={PROFILE_ID} />); | ||
| await waitFor(() => screen.getByText("ToRemove")); | ||
|
|
||
| const chip = screen.getByRole("button", { name: /Go to ToRemove/i }); | ||
| fireEvent.contextMenu(chip); | ||
| const removeBtn = await waitFor(() => screen.getByRole("menuitem", { name: /Remove bookmark/i })); | ||
| await act(async () => { | ||
| fireEvent.click(removeBtn); | ||
| }); | ||
|
|
||
| await waitFor(() => expect(screen.queryByText("ToRemove")).toBeNull()); | ||
| }); | ||
|
|
||
| it("truncates long titles to ~20 chars", async () => { | ||
| const longTitle = "A Very Long Title That Should Be Truncated"; | ||
| vi.mocked(bookmarksApi.listBookmarks).mockResolvedValue([ | ||
| makeBookmark({ bookmark_id: "bm-1", title: longTitle, url: "https://long.example.com" }), | ||
| ]); | ||
|
|
||
| render(<BookmarksBar windowId={WINDOW_ID} profileId={PROFILE_ID} />); | ||
| await waitFor(() => screen.getByText("A Very Long Title Th…")); | ||
| }); | ||
| }); | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,176 @@ | ||
| /** | ||
| * BrowserApp v2 — BookmarksBar. | ||
| * | ||
| * Horizontal row of bookmark chips shown below the tab strip when the | ||
| * active profile has at least one bookmark. Auto-hides when empty. | ||
| * | ||
| * Each chip: | ||
| * - Favicon (Google S2 service, 32px) | ||
| * - Title truncated to ~20 characters | ||
| * - Click → navigates active tab | ||
| * - Right-click → context menu with "Remove bookmark" | ||
| */ | ||
| import { useEffect, useRef, useState } from "react"; | ||
| import { listBookmarks, removeBookmark, type Bookmark } from "@/lib/browser-bookmarks-api"; | ||
| import { useBrowserStore } from "@/stores/browser-store"; | ||
|
|
||
| interface BookmarksBarProps { | ||
| windowId: string; | ||
| profileId: string; | ||
| } | ||
|
|
||
| function faviconUrl(url: string): string { | ||
| try { | ||
| const { hostname } = new URL(url); | ||
| return `https://www.google.com/s2/favicons?domain=${encodeURIComponent(hostname)}&sz=32`; | ||
| } catch { | ||
| return ""; | ||
| } | ||
| } | ||
|
|
||
| function truncate(text: string, max = 20): string { | ||
| if (text.length <= max) return text; | ||
| return text.slice(0, max) + "…"; | ||
| } | ||
|
|
||
| interface ContextMenuState { | ||
| x: number; | ||
| y: number; | ||
| bookmarkId: string; | ||
| } | ||
|
|
||
| export function BookmarksBar({ windowId, profileId }: BookmarksBarProps) { | ||
| const navigateTab = useBrowserStore((s) => s.navigateTab); | ||
| const win = useBrowserStore((s) => s.windows[windowId]); | ||
|
|
||
| const [bookmarks, setBookmarks] = useState<Bookmark[]>([]); | ||
| const [contextMenu, setContextMenu] = useState<ContextMenuState | null>(null); | ||
| const menuRef = useRef<HTMLDivElement | null>(null); | ||
| const loadSeqRef = useRef(0); | ||
|
|
||
| async function load() { | ||
| const seq = ++loadSeqRef.current; | ||
| const list = await listBookmarks(profileId); | ||
| if (seq !== loadSeqRef.current) return; | ||
| setBookmarks(list); | ||
| } | ||
|
|
||
| useEffect(() => { | ||
| load(); | ||
| }, [profileId]); | ||
|
|
||
| // React to bookmark-changed events fired by other components (AddressBar star, | ||
| // etc.) so the bar stays in sync. Skip events we dispatched ourselves. | ||
| useEffect(() => { | ||
| const handler = (e: Event) => { | ||
| const ce = e as CustomEvent<{ profileId: string; url: string; bookmarkId: string | null; source?: string }>; | ||
| if (ce.detail.profileId !== profileId) return; | ||
| if (ce.detail.source === "bookmarks-bar") return; | ||
| load(); | ||
| }; | ||
| window.addEventListener("taos-browser:bookmark-changed", handler); | ||
| return () => window.removeEventListener("taos-browser:bookmark-changed", handler); | ||
| }, [profileId]); | ||
|
|
||
| // Dismiss context menu on outside click | ||
| useEffect(() => { | ||
| if (!contextMenu) return; | ||
| const handler = (e: MouseEvent) => { | ||
| if (!menuRef.current?.contains(e.target as Node)) { | ||
| setContextMenu(null); | ||
| } | ||
| }; | ||
| window.addEventListener("mousedown", handler); | ||
| return () => window.removeEventListener("mousedown", handler); | ||
| }, [contextMenu]); | ||
|
|
||
| if (bookmarks.length === 0) return null; | ||
|
|
||
| const activeTabId = win?.activeTabId; | ||
|
|
||
| function handleChipClick(url: string) { | ||
| if (!activeTabId) return; | ||
| navigateTab(windowId, activeTabId, url); | ||
| } | ||
|
|
||
| function handleChipContextMenu(e: React.MouseEvent, bookmarkId: string) { | ||
| e.preventDefault(); | ||
| setContextMenu({ x: e.clientX, y: e.clientY, bookmarkId }); | ||
| } | ||
|
|
||
| async function handleRemove(bookmarkId: string) { | ||
| setContextMenu(null); | ||
| const removed = bookmarks.find((b) => b.bookmark_id === bookmarkId); | ||
| const ok = await removeBookmark(profileId, bookmarkId); | ||
| if (ok) { | ||
| // Update local state immediately — don't wait for a re-fetch | ||
| setBookmarks((prev) => prev.filter((b) => b.bookmark_id !== bookmarkId)); | ||
| // Notify other components (AddressBar star icon, etc.) but mark the | ||
| // event as originating here so our own listener doesn't trigger a reload | ||
| window.dispatchEvent( | ||
| new CustomEvent("taos-browser:bookmark-changed", { | ||
| detail: { | ||
| profileId, | ||
| url: removed?.url ?? "", | ||
| bookmarkId: null, | ||
| source: "bookmarks-bar", | ||
| }, | ||
| }), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return ( | ||
| <> | ||
| <div | ||
| role="toolbar" | ||
| aria-label="Bookmarks bar" | ||
| className="flex items-center gap-1 px-2 h-8 bg-shell-surface border-b border-shell-border-subtle overflow-x-auto" | ||
| style={{ scrollbarWidth: "none" }} | ||
| > | ||
| {bookmarks.map((bm) => ( | ||
| <button | ||
| key={bm.bookmark_id} | ||
| type="button" | ||
| aria-label={`Go to ${bm.title}`} | ||
| onClick={() => handleChipClick(bm.url)} | ||
| onContextMenu={(e) => handleChipContextMenu(e, bm.bookmark_id)} | ||
| className="flex items-center gap-1 px-2 py-0.5 rounded bg-shell-bg-deep border border-shell-border-subtle text-xs hover:bg-shell-hover whitespace-nowrap flex-shrink-0" | ||
| > | ||
| {faviconUrl(bm.url) && ( | ||
| <img | ||
| src={faviconUrl(bm.url)} | ||
| alt="" | ||
| aria-hidden="true" | ||
| width={14} | ||
| height={14} | ||
| className="w-3.5 h-3.5 object-contain" | ||
| /> | ||
| )} | ||
| <span>{truncate(bm.title)}</span> | ||
| </button> | ||
| ))} | ||
| </div> | ||
|
|
||
| {contextMenu && ( | ||
| <div | ||
| ref={menuRef} | ||
| role="menu" | ||
| aria-label="Bookmark actions" | ||
| className="fixed z-[80] rounded border border-shell-border-subtle bg-shell-surface shadow-lg py-1 text-xs" | ||
| style={{ top: contextMenu.y, left: contextMenu.x }} | ||
| > | ||
| <button | ||
| type="button" | ||
| role="menuitem" | ||
| aria-label="Remove bookmark" | ||
| onClick={() => handleRemove(contextMenu.bookmarkId)} | ||
| className="w-full text-left px-3 py-1.5 hover:bg-shell-hover text-shell-text" | ||
| > | ||
| Remove bookmark | ||
| </button> | ||
| </div> | ||
| )} | ||
| </> | ||
| ); | ||
| } |
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
CRITICAL: Test expectation mismatch - the truncate function slices to 20 characters and appends "…", resulting in "A Very Long Title Tha…", but the test expects "A Very Long Title Th…" (missing 'a'). This will cause the test to fail.