Skip to content

feat: project-wide bookmark manager with a -/bookmarks tab and home page section - #9863

Open
nishantmonu51 wants to merge 3 commits into
mainfrom
nishantmonu51/bookmarks-manager
Open

feat: project-wide bookmark manager with a -/bookmarks tab and home page section#9863
nishantmonu51 wants to merge 3 commits into
mainfrom
nishantmonu51/bookmarks-manager

Conversation

@nishantmonu51

Copy link
Copy Markdown
Collaborator

Bookmarks were only reachable from a single dashboard's dropdown, so there was no way to find a bookmark across dashboards or manage a long list of them. This adds a bookmark manager that follows the Dashboards listing UX.

  • The project home page gets a Bookmarks section under Dashboards: a 5-row preview, a sort dropdown in the heading (on its own bookmarks_sort URL param so it does not collide with the dashboards sort), and a "See all bookmarks" link. It is hidden for anonymous viewers of public projects.
  • A new -/bookmarks tab and page lists every bookmark the user can see across dashboards, with the shared table toolbar for search and sort.
  • Sort options: last used (tracked per browser, like dashboards), last updated, name and dashboard. Rows show the dashboard with its type badge, Managed / Home / Legacy chips, update and last-used times, and the description.
  • Clicking a row opens the bookmark on its dashboard. Owners and users with the manage bookmarks permission get hover actions to edit the name, description and category (BookmarkMetadataDialog) and to delete with a confirmation.
  • Backend: ListBookmarks now treats resource kind and name as optional filters, requires read access to the project, and returns rows ordered by name; UpdateBookmark bumps updated_on. Same response shape, so the existing dropdown callers are unchanged. Covered by admin/server/bookmarks_test.go.
  • All bookmark mutations invalidate every bookmark query so the dashboard dropdown and the manager stay in sync.
  • UrlParamsState.createStringParam is typed as non-nullable, which also removes pre-existing type errors in the dashboards listing.
  • Playwright coverage in web-admin/tests/bookmarks-manager.spec.ts; unit tests in bookmark-listing-utils.spec.ts.

Tags on bookmarks are a follow-up; the slot next to the sort dropdown is left for the tag filter.

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!

Bookmarks were only reachable from a single dashboard's dropdown. This adds a
Bookmarks section on the project home page and a `-/bookmarks` tab that list
every bookmark the user can see across dashboards, with search, sorting and
open, edit and delete actions.

Backend:
- `ListBookmarks` treats resource kind and name as optional filters, so an
  empty request returns all bookmarks in the project visible to the caller.
- The handler now requires read access to the project and rejects a resource
  name without a kind.
- Rows are returned ordered by name, and `UpdateBookmark` bumps `updated_on`.

Frontend:
- Home page section mirrors the Dashboards section: 5-row preview, sort
  dropdown in the heading on its own `bookmarks_sort` URL param, and a
  "See all bookmarks" link. Hidden for anonymous viewers.
- New `-/bookmarks` page and tab with the shared table toolbar.
- Sort by last used (tracked per browser like dashboards), last updated, name
  or dashboard. Rows show dashboard, category chips, update and usage times.
- Owners and bookmark managers get hover edit (metadata dialog) and delete
  (with confirmation) actions.
- All bookmark mutations invalidate every bookmark query so the dropdown and
  the manager stay in sync.
- `UrlParamsState.createStringParam` is typed as non-nullable, which also
  removes pre-existing type errors in the dashboards listing.

@AdityaHegde AdityaHegde left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Lets align the bookmark icons here to the top. Looks weird being in the middle. Same would apply to bookmarks page.
Image 2. Creating a dashboard doesn't invalidate the project wide query. Needs a refresh to start showing up.

Comment thread proto/rill/admin/v1/api.proto Outdated
string project_id = 1;
// Optional filter on the kind of the resource the bookmark is for (e.g. "rill.runtime.v1.Explore").
// When both resource_kind and resource_name are empty, all bookmarks in the project are returned.
string resource_kind = 2;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to actually add optional to these.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Both fields are optional now; the handler reads them via the getters.

export let pinnedRows: string[] = [];
export let maxRows: number | undefined = undefined;
// Defaults to the resource name, which is only meaningful for runtime resources.
export let getRowId: ((row: unknown, index: number) => string) | undefined =

@AdityaHegde AdityaHegde Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list is specifically meant for resources. So we should make this generic if we intent to use it other places. Perhaps we can just rename this component and move it to a better place?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Moved to web-admin/src/components/list-table/ListTable.svelte (with its toolbar). getRowId is now a required prop, so the component has no V1Resource knowledge; the dashboards and personal canvases lists pass the resource-name id themselves.

const RENAMED_BOOKMARK_NAME = "Manager bookmark renamed";

test.describe.serial("Bookmark manager", () => {
test("Create a bookmark from a dashboard", async ({ adminPage }) => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have a dedicated bookmarks tests this should be a beforeAll instead of a test.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Creation runs in beforeAll using the worker-scoped browser fixture with the admin storage state, since adminPage is test-scoped.

description: values.description,
// Home bookmarks are always shared.
shared: bookmark.default || values.shared === "true",
urlSearch: bookmark.urlSearch ?? "",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since data is not changing this needs to be backwards compatible unlike the form in dashboard. Add data: bookmark.data ?? "" as well.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UpdateBookmarkRequest has no data field, and the SQL update only sets display_name, description, url_search and shared, so a legacy bookmark keeps its data after a metadata edit. urlSearch is passed through unchanged (empty for legacy), so the dropdown still falls back to data. Left as is.

const kind = bookmark.resourceKind ?? "";
const name = bookmark.resourceName ?? "";
const slug = DashboardSlugByKind[kind];
const urlSearch = normalizeUrlSearch(bookmark.urlSearch);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we not supporting legacy bookmarks here? How about just using state=<data> as search?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Legacy explore bookmarks now link with ?state=<data>; legacy canvas data is used as the search directly, matching the canvas dropdown. Dropped the Legacy chip.

- mark `ListBookmarksRequest` filters as optional
- move the web-admin list to `components/list-table/ListTable` with a required `getRowId`
- open legacy explore bookmarks via the `state` param instead of a Legacy chip
- refetch inactive bookmark list queries on invalidation
- top-align the bookmark row icon
- create the e2e fixture bookmark in `beforeAll` and navigate in-app for the dropdown check

Claude-Session: https://claude.ai/code/session_01Ft2MEXK1nWvFWrubUyE82W
@nishantmonu51

Copy link
Copy Markdown
Collaborator Author
  1. Done: icon is top-aligned on the title line (same cell on the home section and the bookmarks page).
  2. Done: root cause is the global query client's refetchOnMount: false, so invalidation only refreshed active queries and the home list stayed stale on remount. invalidateBookmarkQueries now uses refetchType: "all", and the e2e test navigates in-app for the dropdown check to cover it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved moderate findings affect permission-aware editing, accessibility, legacy labeling, and usage tracking.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds a project-wide bookmark manager with a home-page preview, -/bookmarks tab, search, sorting, metadata management, usage tracking, localization, API updates, and tests.

Changes:

  • Adds bookmark navigation, listing, editing, deletion, and usage tracking.
  • Extends bookmark APIs, persistence, invalidation, and permissions.
  • Adds shared list-table behavior, translations, and automated coverage.

Review findings:

  • Moderate (3 votes): manage_bookmarks users cannot access the category selector.
  • Nit (1 vote): the "Required" validation message is not localized.
  • Moderate (3 votes): hover-only actions are inaccessible to keyboard and touch users.
  • Moderate (2 votes): legacy bookmarks do not display the Legacy chip.
  • Moderate (1 vote): unsupported rows incorrectly update last-used tracking.
File summaries
File Description
web-common/src/proto/gen/rill/admin/v1/api_pb.ts Updates generated bookmark filters.
web-common/src/lib/store-utils/url-params-state.svelte.ts Makes string URL parameters non-nullable.
web-common/src/lib/store-utils/url-params-state.svelte.spec.ts Updates URL parameter tests.
web-common/src/lib/i18n/messages/es.json Adds Spanish bookmark translations.
web-common/src/lib/i18n/messages/en.json Adds English bookmark translations.
web-admin/tests/bookmarks-manager.spec.ts Adds end-to-end manager coverage.
web-admin/src/routes/[organization]/[project]/+page.svelte Adds the home-page bookmark preview.
web-admin/src/routes/[organization]/[project]/+layout.svelte Propagates authentication state.
web-admin/src/routes/[organization]/[project]/-/bookmarks/+page.svelte Adds the bookmark manager route.
web-admin/src/features/projects/ProjectTabs.svelte Adds the bookmarks navigation tab.
web-admin/src/features/personal-files/canvas/PersonalCanvasesList.svelte Migrates the list to ListTable.
web-admin/src/features/dashboards/listing/DashboardsTable.svelte Migrates the dashboard list to ListTable.
web-admin/src/features/bookmarks/utils.ts Adds bookmark filter classification helpers.
web-admin/src/features/bookmarks/selectors.ts Adds project queries and invalidation.
web-admin/src/features/bookmarks/recently-used-bookmarks.ts Tracks local bookmark usage.
web-admin/src/features/bookmarks/listing/BookmarksTableCompositeCell.svelte Renders bookmark rows and actions.
web-admin/src/features/bookmarks/listing/BookmarksTable.svelte Implements manager table behavior.
web-admin/src/features/bookmarks/listing/bookmark-listing-utils.ts Builds, filters, and sorts rows.
web-admin/src/features/bookmarks/listing/bookmark-listing-utils.spec.ts Tests listing utilities.
web-admin/src/features/bookmarks/HomeBookmarkButton.svelte Tracks home bookmark usage.
web-admin/src/features/bookmarks/BookmarksFormDialog.svelte Uses shared bookmark invalidation.
web-admin/src/features/bookmarks/Bookmarks.svelte Syncs mutations and usage tracking.
web-admin/src/features/bookmarks/BookmarkMetadataDialog.svelte Adds bookmark metadata editing.
web-admin/src/components/list-table/ListTableToolbar.svelte Adds reusable list search controls.
web-admin/src/components/list-table/ListTable.svelte Generalizes list rows and identities.
web-admin/src/client/gen/index.schemas.ts Updates generated API schemas.
proto/rill/admin/v1/api.proto Makes bookmark filters optional.
proto/gen/rill/admin/v1/openapi.yaml Regenerates OpenAPI documentation.
proto/gen/rill/admin/v1/api.pb.validate.go Regenerates validation code.
proto/gen/rill/admin/v1/admin.swagger.yaml Regenerates Swagger documentation.
admin/server/bookmarks.go Adds access checks and flexible listing.
admin/server/bookmarks_test.go Adds backend bookmark coverage.
admin/database/postgres/postgres.go Updates filtering, ordering, and timestamps.
admin/database/database.go Documents the expanded bookmark query contract.
Review details

Suppressed comments (2)

web-admin/src/features/bookmarks/BookmarkMetadataDialog.svelte:38

  • "Required" is a new user-facing validation message in a component that already uses Paraglide. Use the existing localized m.common_required() message instead; otherwise Spanish users will see English validation copy.
      displayName: string().required("Required"),

web-admin/src/features/bookmarks/listing/BookmarksTableCompositeCell.svelte:54

  • When row.href is undefined for an unsupported resource kind, this anchor still invokes onOpen(row). That records a last-used timestamp even though no dashboard was opened, which can incorrectly reorder the list under Last used. Only record usage for rows with a navigable href.
  <a
    class="flex flex-row items-start gap-x-3 min-w-0 grow h-full py-2.5"
    href={row.href}
    aria-label={m.bookmark_entry_aria_label({ name: displayName })}
    onclick={() => onOpen(row)}
  • Files reviewed: 33/35 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +112 to +116
<ProjectAccessControls {organization} {project}>
<Select
bind:value={$form["shared"]}
id="shared"
label={m.bookmark_category()}
Comment on lines +65 to +69
{#if row.category === "home"}
<Tag color="blue">{m.bookmark_tag_home()}</Tag>
{:else if row.category === "managed"}
<Tag color="gray">{m.bookmark_tag_managed()}</Tag>
{/if}
Comment on lines +117 to +134
{#if hovered}
<Button
square
type="tertiary"
label={m.bookmark_edit()}
onClick={() => onEdit(row)}
>
<Pencil size="16px" />
</Button>
<Button
square
type="tertiary"
label={m.bookmark_delete_bookmark()}
onClick={() => onDelete(row)}
>
<Trash size="16px" />
</Button>
{/if}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants