Repository navigation
feat(pages): default to org visibility, cached thumbnails, owner + delete in the grid #6932
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
12 commits
Select commit
Hold shift + click to select a range
90aa685
feat(pages): default new pages to org, add cached thumbnails, owner, …
harshithmullapudi 65fbe12
fix(pages): address review on thumbnail caching and share popover
harshithmullapudi ca8ed61
fix(pages): guard blank captures and recover from evicted thumbnails
harshithmullapudi 58dc2b2
fix(pages): stop the thumbnail spinner when page.pull fails
harshithmullapudi 964b005
test(pages): cover the thumbnail cache boundary
harshithmullapudi 949a905
Merge origin/main into fix/pages-v2
harshithmullapudi eaf9ccc
test(pages): drop wall-clock polling from the recency test
harshithmullapudi 2c246f3
refactor(pages): fold page content and thumbnail into one page router
harshithmullapudi 6869529
Merge origin/main into fix/pages-v2
harshithmullapudi ac293ac
feat(pages): restore scroll position when reopening a page
harshithmullapudi 956c7d1
Merge remote-tracking branch 'origin/main' into pr-6932
saddlepaddle 78b0fe8
i18n(pages): translate the delete action and toast (ja, zh-CN, ko, fr)
saddlepaddle 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
This file was deleted.
Oops, something went wrong.
File renamed without changes.
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 @@ | ||
| export { createPageRouter } from "./page"; |
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,10 @@ | ||
| import { router } from "../.."; | ||
| import { createPageContentRouter } from "./content"; | ||
| import { createPageThumbnailRouter } from "./thumbnail"; | ||
|
|
||
| export const createPageRouter = () => { | ||
| return router({ | ||
| content: createPageContentRouter(), | ||
| thumbnail: createPageThumbnailRouter(), | ||
| }); | ||
| }; |
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,21 @@ | ||
| import { ensureThumbnail, peekThumbnail } from "main/lib/pageThumbnails"; | ||
| import { z } from "zod"; | ||
| import { publicProcedure, router } from "../.."; | ||
|
|
||
| const thumbnailKey = z.object({ | ||
| accountId: z.string(), | ||
| pageId: z.string(), | ||
| version: z.string(), | ||
| }); | ||
|
|
||
| export const createPageThumbnailRouter = () => { | ||
| return router({ | ||
| peek: publicProcedure | ||
| .input(thumbnailKey) | ||
| .query(({ input }) => peekThumbnail(input)), | ||
|
|
||
| ensure: publicProcedure | ||
| .input(thumbnailKey.extend({ html: z.string() })) | ||
| .mutation(({ input }) => ensureThumbnail(input)), | ||
| }); | ||
| }; | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| export { | ||
| ensureThumbnail, | ||
| peekThumbnail, | ||
| THUMBNAIL_SCHEME, | ||
| type ThumbnailKey, | ||
| thumbnailProtocolHandler, | ||
| thumbnailUrl, | ||
| } from "./pageThumbnails"; |
254 changes: 254 additions & 0 deletions
254
apps/desktop/src/main/lib/pageThumbnails/pageThumbnails.test.ts
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,254 @@ | ||
| import { beforeEach, describe, expect, mock, test } from "bun:test"; | ||
| import { mkdir, rm, stat, utimes, writeFile } from "node:fs/promises"; | ||
| import { tmpdir } from "node:os"; | ||
| import { join } from "node:path"; | ||
|
|
||
| const USER_DATA = join(tmpdir(), "superset-test"); | ||
|
|
||
| mock.module("electron", () => ({ | ||
| app: { | ||
| getPath: () => USER_DATA, | ||
| getName: () => "test-app", | ||
| getVersion: () => "1.0.0", | ||
| getAppPath: () => USER_DATA, | ||
| isPackaged: false, | ||
| }, | ||
| BrowserWindow: mock(() => ({ | ||
| webContents: { send: mock(), setAudioMuted: mock() }, | ||
| loadURL: mock(), | ||
| isDestroyed: () => false, | ||
| destroy: mock(), | ||
| on: mock(), | ||
| })), | ||
| session: { | ||
| fromPartition: () => ({ | ||
| protocol: { handle: mock(), isProtocolHandled: () => false }, | ||
| }), | ||
| }, | ||
| protocol: { | ||
| handle: mock(), | ||
| isProtocolHandled: () => false, | ||
| registerSchemesAsPrivileged: mock(), | ||
| }, | ||
| dialog: { | ||
| showMessageBox: mock(), | ||
| showOpenDialog: mock(), | ||
| showSaveDialog: mock(), | ||
| }, | ||
| ipcMain: { handle: mock(), on: mock() }, | ||
| webContents: { fromId: () => null }, | ||
| shell: { openExternal: mock(), openPath: mock() }, | ||
| clipboard: { writeText: mock(), readText: () => "", writeImage: mock() }, | ||
| Menu: { | ||
| buildFromTemplate: mock(() => ({ popup: mock() })), | ||
| setApplicationMenu: mock(), | ||
| }, | ||
| Notification: mock(() => ({ show: mock(), on: mock() })), | ||
| })); | ||
|
|
||
| const { | ||
| ensureThumbnail, | ||
| peekThumbnail, | ||
| THUMBNAIL_SCHEME, | ||
| thumbnailProtocolHandler, | ||
| thumbnailUrl, | ||
| } = await import("./pageThumbnails"); | ||
|
|
||
| const CACHE_DIR = join(USER_DATA, "page-thumbnails"); | ||
|
|
||
| const ACCOUNT_A = "11111111-1111-1111-1111-111111111111"; | ||
| const ACCOUNT_B = "22222222-2222-2222-2222-222222222222"; | ||
| const PAGE = "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"; | ||
|
|
||
| const JPEG_BYTES = Buffer.from([0xff, 0xd8, 0xff, 0xdb, 0x00, 0x01]); | ||
|
|
||
| function pathFor(accountId: string, pageId: string, version: string): string { | ||
| return join(CACHE_DIR, accountId, `${pageId}-${version}.jpg`); | ||
| } | ||
|
|
||
| async function seed( | ||
| accountId: string, | ||
| pageId: string, | ||
| version: string, | ||
| ): Promise<string> { | ||
| const path = pathFor(accountId, pageId, version); | ||
| await mkdir(join(CACHE_DIR, accountId), { recursive: true }); | ||
| await writeFile(path, JPEG_BYTES); | ||
| return path; | ||
| } | ||
|
|
||
| function serve(url: string): Promise<Response> { | ||
| return thumbnailProtocolHandler(new Request(url)); | ||
| } | ||
|
|
||
| beforeEach(async () => { | ||
| await rm(CACHE_DIR, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| describe("thumbnailUrl", () => { | ||
| test("scopes the url to the account", () => { | ||
| expect( | ||
| thumbnailUrl({ accountId: ACCOUNT_A, pageId: PAGE, version: "3" }), | ||
| ).toBe(`${THUMBNAIL_SCHEME}://${ACCOUNT_A}/${PAGE}/3`); | ||
| }); | ||
|
|
||
| test("distinguishes versions of the same page", () => { | ||
| const v3 = thumbnailUrl({ | ||
| accountId: ACCOUNT_A, | ||
| pageId: PAGE, | ||
| version: "3", | ||
| }); | ||
| const v4 = thumbnailUrl({ | ||
| accountId: ACCOUNT_A, | ||
| pageId: PAGE, | ||
| version: "4", | ||
| }); | ||
| expect(v3).not.toBe(v4); | ||
| }); | ||
| }); | ||
|
|
||
| describe("peekThumbnail", () => { | ||
| test("returns null when nothing is cached", async () => { | ||
| await expect( | ||
| peekThumbnail({ accountId: ACCOUNT_A, pageId: PAGE, version: "1" }), | ||
| ).resolves.toBeNull(); | ||
| }); | ||
|
|
||
| test("returns the url once a thumbnail exists", async () => { | ||
| await seed(ACCOUNT_A, PAGE, "1"); | ||
| await expect( | ||
| peekThumbnail({ accountId: ACCOUNT_A, pageId: PAGE, version: "1" }), | ||
| ).resolves.toBe( | ||
| thumbnailUrl({ accountId: ACCOUNT_A, pageId: PAGE, version: "1" }), | ||
| ); | ||
| }); | ||
|
|
||
| test("does not surface another account's thumbnail", async () => { | ||
| await seed(ACCOUNT_A, PAGE, "1"); | ||
| await expect( | ||
| peekThumbnail({ accountId: ACCOUNT_B, pageId: PAGE, version: "1" }), | ||
| ).resolves.toBeNull(); | ||
| }); | ||
|
|
||
| test("misses a version that has not been captured", async () => { | ||
| await seed(ACCOUNT_A, PAGE, "3"); | ||
| await expect( | ||
| peekThumbnail({ accountId: ACCOUNT_A, pageId: PAGE, version: "4" }), | ||
| ).resolves.toBeNull(); | ||
| }); | ||
|
|
||
| test("cannot escape the account directory via the page id", async () => { | ||
| await seed(ACCOUNT_B, PAGE, "1"); | ||
| await expect( | ||
| peekThumbnail({ | ||
| accountId: ACCOUNT_A, | ||
| pageId: `../${ACCOUNT_B}/${PAGE}`, | ||
| version: "1", | ||
| }), | ||
| ).resolves.toBeNull(); | ||
| }); | ||
|
|
||
| test("cannot escape the account directory via the account id", async () => { | ||
| await seed(ACCOUNT_B, PAGE, "1"); | ||
| await expect( | ||
| peekThumbnail({ | ||
| accountId: `../page-thumbnails/${ACCOUNT_B}`, | ||
| pageId: PAGE, | ||
| version: "1", | ||
| }), | ||
| ).resolves.toBeNull(); | ||
| }); | ||
|
|
||
| test("rejects a version that is not a number", async () => { | ||
| await seed(ACCOUNT_A, PAGE, "1"); | ||
| await expect( | ||
| peekThumbnail({ accountId: ACCOUNT_A, pageId: PAGE, version: "1.jpg" }), | ||
| ).resolves.toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("thumbnailProtocolHandler", () => { | ||
| test("serves a cached thumbnail as an immutable jpeg", async () => { | ||
| await seed(ACCOUNT_A, PAGE, "1"); | ||
| const response = await serve( | ||
| `${THUMBNAIL_SCHEME}://${ACCOUNT_A}/${PAGE}/1`, | ||
| ); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(response.headers.get("Content-Type")).toBe("image/jpeg"); | ||
| expect(response.headers.get("Cache-Control")).toContain("immutable"); | ||
| expect(new Uint8Array(await response.arrayBuffer())).toEqual( | ||
| new Uint8Array(JPEG_BYTES), | ||
| ); | ||
| }); | ||
|
|
||
| test("404s when the thumbnail is absent", async () => { | ||
| const response = await serve( | ||
| `${THUMBNAIL_SCHEME}://${ACCOUNT_A}/${PAGE}/1`, | ||
| ); | ||
| expect(response.status).toBe(404); | ||
| }); | ||
|
|
||
| test("will not serve one account's thumbnail to another", async () => { | ||
| await seed(ACCOUNT_A, PAGE, "1"); | ||
| const response = await serve( | ||
| `${THUMBNAIL_SCHEME}://${ACCOUNT_B}/${PAGE}/1`, | ||
| ); | ||
| expect(response.status).toBe(404); | ||
| }); | ||
|
|
||
| test("refuses segments that are not plain identifiers", async () => { | ||
| await seed(ACCOUNT_A, PAGE, "1"); | ||
| const dotted = await serve( | ||
| `${THUMBNAIL_SCHEME}://${ACCOUNT_A}/..secrets/1`, | ||
| ); | ||
| const encoded = await serve( | ||
| `${THUMBNAIL_SCHEME}://${ACCOUNT_A}/%2e%2e%2fsecrets/1`, | ||
| ); | ||
| expect(dotted.status).toBe(404); | ||
| expect(encoded.status).toBe(404); | ||
| }); | ||
|
|
||
| test("records access so eviction can order by recency", async () => { | ||
| const path = await seed(ACCOUNT_A, PAGE, "1"); | ||
| const stale = new Date(Date.now() - 60_000); | ||
| await utimes(path, stale, stale); | ||
| const before = (await stat(path)).mtimeMs; | ||
|
|
||
| const response = await serve( | ||
| `${THUMBNAIL_SCHEME}://${ACCOUNT_A}/${PAGE}/1`, | ||
| ); | ||
| expect(response.status).toBe(200); | ||
|
|
||
| expect((await stat(path)).mtimeMs).toBeGreaterThan(before); | ||
| }); | ||
| }); | ||
|
|
||
| describe("ensureThumbnail", () => { | ||
| test("returns the cached url without attempting a capture", async () => { | ||
| await seed(ACCOUNT_A, PAGE, "7"); | ||
|
|
||
| await expect( | ||
| ensureThumbnail({ | ||
| accountId: ACCOUNT_A, | ||
| pageId: PAGE, | ||
| version: "7", | ||
| html: "<p>never rendered</p>", | ||
| }), | ||
| ).resolves.toBe( | ||
| thumbnailUrl({ accountId: ACCOUNT_A, pageId: PAGE, version: "7" }), | ||
| ); | ||
| }); | ||
|
|
||
| test("refuses a key that would read outside the account directory", async () => { | ||
| await seed(ACCOUNT_B, PAGE, "1"); | ||
| await expect( | ||
| ensureThumbnail({ | ||
| accountId: ACCOUNT_A, | ||
| pageId: `../${ACCOUNT_B}/${PAGE}`, | ||
| version: "1", | ||
| html: "<p>hi</p>", | ||
| }), | ||
| ).rejects.toThrow("Invalid thumbnail key"); | ||
| }); | ||
| }); |
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.
Uh oh!
There was an error while loading. Please reload this page.