feat: search on all pub page #1290

tefkah · 2025-06-17T15:54:43Z

Issue(s) Resolved

Resolves #1197

High-level Explanation of PR

Adds search capabilities to getPubsWithRelatedValues and adds a searching filter to /pubs.

Not done:

Made global Cmd+K search use PubCards or this component. This is somewhat tricky to do as PubCard is still a server component. I'd need to rework it more, which i may do in feat: make pubcard more useable on mobile #1293
No results screen/message (maybe i should still add that)

Video overview (kinda rambly sorry)

pubsearch-overview.mp4

Test Plan

go to legacy/pubs
search some stuff
page through the results if possible

Screenshots (if applicable)

When searching

Changed the pagination a bit when searching, as i didn't want to find out how many results there are in total (that's kinda costly), so I just get N = perPage + 1 items, check if N > perPage, if so i allow the user to paginate to the next page.

There's an argument to make to just remove pagination for searching entirely, and only show the first ~25 results, and the user just needs to provide a better query if they aint happy.

Notes

tefkah · 2025-06-23T12:27:31Z

core/app/c/[communitySlug]/pubs/page.tsx

+		<ContentLayout
+			title={
+				<>
+					<BookOpen size={24} strokeWidth={1} className="mr-2 text-gray-500" /> Pubs
+				</>
+			}


added the contentlayout for the pubs page

tefkah · 2025-06-23T12:29:25Z

core/app/c/[communitySlug]/pubs/PubSearchInput.tsx

+	// deferred query to keep track of server updates
+	// without this, we can't only check if inputValue !== query.query,
+	// which only tells us that the debounce has happened
+	const deferredQuery = useDeferredValue(query.query);


this is kind of mysterious to me, but this allows me to keep the body of the search at half-opacity until the content has fully refreshed. this could also be done by using useTransition.

tefkah · 2025-06-23T12:30:55Z

packages/ui/src/hooks/useKeyBoardShortcut.tsx

+export type KeyCombination =
+	| `${Modifiers}+${Keys}`
+	| `${Modifiers}+${Modifiers}+${Keys}`
+	| `${Modifiers}+${Modifiers}+${Modifiers}+${Keys}`
+	| `${Modifiers}+${Modifiers}+${Modifiers}+${Modifiers}+${Keys}`;


niceish type that allows for fully typed Mod+k like strings. doesn't really cover every key (only english-y ones), but that's fine bc it's us defining the shortcuts, and we (mostly) use English keyboards

tefkah · 2025-06-23T12:33:15Z

packages/ui/src/hooks/useKeyBoardShortcut.tsx

this is a smallish shortcut handler that allows us to define shortcuts kinda easily, and executes them in order of priority.

i wanted this bc on the pubs page we were using CMD+k to focus on the input, while on any other page it should pull up the search dialogue.

sadly this doesn't yet incorporate the editor, as the keyboard shortcut logic there is handled by prosemirror itself.

the way i was thinking of doing that was something like:

expose some hooks to enable/disable certain keys from here

use this hook in the body of context editor

pass enable/disable functions to keymap plugin

have keymap disable those also used by the editor (cmd+k)

ideally it would disable/enable the shortcuts on blur/focus as well, but that might be too fragile. if you have a better idea please let me know!

tefkah · 2025-06-23T12:41:44Z

core/lib/authentication/lucia.ts

+/**
+ * Separately cache this call because validateRequest can be called with different options
+ * leading to cache misses
+ */
+const validateLuciaSession = cache(async (sessionId: string) => {
+	const result = await lucia.validateSession(sessionId);
+	return result;
+});
+


this is just a small thing i found when looking at logs:

we sometimes call getPageLoginData and sometimes getLoginData on the same request. i would expect the call to the db to be cached between these two calls, bc getLoginData is called by getPageLoginData and getLoginData is React.cached.

platform/core/lib/authentication/loginData.ts

Lines 17 to 51 in f633a09

/**

* Get the users login data based on the session cookie

*/

export const getLoginData = cache(async (opts?: ExtraSessionValidationOptions) => {

return validateRequest(opts);

});

const defaultPageOpts: ExtraSessionValidationOptions & LoginRedirectOpts = {

allowedSessions: [AuthTokenType.generic, AuthTokenType.verifyEmail],

};

/**

* Get the login data for the current page, and redirect to the login page if the user is not logged in.

*/

export const getPageLoginData = cache(

async (opts?: ExtraSessionValidationOptions & LoginRedirectOpts) => {

const options = opts ?? defaultPageOpts;

const loginData = await getLoginData(options);

if (!loginData.user) {

redirectToLogin(options);

}

if (loginData.session && loginData.session.type === AuthTokenType.verifyEmail) {

const pathname = getPathname();

redirectToVerify({

redirectTo: expect(pathname, "pathname is missing for redirectToVerify").toString(),

});

}

return loginData;

}

);

export type LoginData = Awaited<ReturnType<typeof getLoginData>>;

but that doesn't happen, bc we pass in some specific arguments to getLoginData (allowedSessions) when called by getPageLoginData.

this leads to getLoginData to be called with diff arguments most of the time (we usually don't pass anything to getLoginData), leading to cache misses.

So it made more sense to me cache the actual call to the db as well, as that's the main bottleneck and always called with the same arguments (for the same request): the sessionid.
this takes the number of calls to the db for this purpose from 2 to 1 in many cases.

tefkah · 2025-06-23T12:42:38Z

core/lib/server/pub.ts

+			.$if(Boolean(tsQuery), (qb) =>
+				qb.select((eb) => [
+					sql<string>`ts_headline(
+								'${sql.raw(language)}',
+								pt.title, 
+								${tsQuery}, 
+								'${sql.raw(headlineConfig)}'
+							)`.as("title"),
+				])
+			)


not sure if this should override the normal title, i just did that here bc it's easy, but we could also have it be returned separately

tefkah · 2025-06-23T12:52:35Z

core/lib/server/pub.ts

+			.$if(Boolean(includeSearchValues), (qb) =>
+				qb.select((eb) =>
+					jsonArrayFrom(
+						eb
+							.selectFrom("pub_values")
+							.innerJoin("pub_fields", "pub_fields.id", "pub_values.fieldId")
+							.innerJoin("_PubFieldToPubType", (join) =>
+								join
+									.onRef("A", "=", "pub_fields.id")
+									.onRef("B", "=", "pt.pubTypeId")
+							)
+							.select([
+								"pub_values.value",
+								"pub_fields.name",
+								"pub_fields.slug",
+								"pub_fields.schemaName",
+								"_PubFieldToPubType.isTitle",
+								sql<string>`ts_headline(
+							'${sql.raw(language)}',
+							pub_values.value#>>'{}',
+							${tsQuery},
+							'${sql.raw(headlineConfig)}'
+						)`.as("highlights"),
+							])
+							.$narrowType<{
+								value: JsonValue;
+								// still typed as null in db
+								schemaName: CoreSchemaType;
+							}>()
+							.whereRef("pub_values.pubId", "=", "pt.pubId")
+							.where(
+								(eb) => sql`to_tsvector(${language}, value#>>'{}') @@ ${tsQuery}`
+							)
+					).as("matchingValues")
+				)
+			)


this is kinda expensive i think, especially if theres a lot of long RichText and a lot of values, but i don't have a better solution.

very bad measurements, but searching for comp on legacy was roughly ~80ms each time with this, and ~40ms each time without. im pretty sure most of the extra delay comes from having to both fetch and highlight the really long body of the "Test article" called "The Complexity of Joint Regeneration: How an Advanced Implant Could Fail by Its In Vivo Proven Bone Component".

i worry about how the performance will scale with a ton of these.

but i think we should just leave this for now and leave the performance tuning for after doing some initial legacy migrations. if we find performance really lacking there we could maybe think about including a dedicated fts solution, maybe use redis for that as well, or a meilisearch instance or something.

tefkah · 2025-06-23T12:54:45Z

core/lib/server/stages.ts

+			.$if(withActionInstances === "count", (qb) =>
+				qb.select((eb) =>
+					eb
+						.selectFrom("action_instances")
+						.whereRef("action_instances.stageId", "=", "stages.id")
+						.select((eb) =>
+							eb.fn.count<number>("action_instances.id").as("actionInstancesCount")
+						)
+						.as("actionInstancesCount")
+				)
+			)
+			.$if(withActionInstances === "full", (qb) =>
+				qb.select((eb) =>
+					jsonArrayFrom(
+						eb
+							.selectFrom("action_instances")
+							.whereRef("action_instances.stageId", "=", "stages.id")
+							.selectAll("action_instances")
+					).as("actionInstances")
+				)
+			)


i kind of changes how the data is fetched for the pubs page, which wasn't working properly (see comment there).

tefkah · 2025-06-23T12:55:09Z

packages/contracts/src/resources/site.ts

+	/**
+	 * Whether to include matched and highlighted values
+	 * @default true if `search` is defined
+	 */
+	withSearchValues?: boolean;


might be redundant to offer this a setting in addition to search, but eh

3mcd

I love how snappy the search feels, and combined with the newer/condensed pub cards and pagination, the all pubs page is starting to look reaaaally good!

feat: search on all pub page

f633a09

tefkah added the preview Auto-deploys a preview application label Jun 17, 2025

3mcd had a problem deploying to gh-654103159-pr-1290 June 17, 2025 15:59 Error

tefkah marked this pull request as draft June 17, 2025 16:00

tefkah added 6 commits June 18, 2025 14:03

fix: some layout + accessibility adjustments to list

c25c21f

feat: properly track server update status for highlighting

060833f

fix: spacing in pubcard

9384d35

feat: add keyboard shortcut handler

1687a1d

fix: make pagination work properly when querying

37c3406

fix: fix lint and type errors

19b03bc

3mcd temporarily deployed to gh-654103159-pr-1290 June 23, 2025 11:43 Inactive

tefkah commented Jun 23, 2025

View reviewed changes

tefkah added 5 commits June 23, 2025 15:00

feat: add clear search input button

c3061a9

fix: properly suspend a bunch of queries on the pub page

fe90cd1

fix: fix layout

648e5ac

fix: don't flash Ctrl -> Cmd on mac

40432dd

fix: remove keyboard shortcut for sidebar

7d041da

3mcd temporarily deployed to gh-654103159-pr-1290 June 23, 2025 13:48 Inactive

tefkah marked this pull request as ready for review June 23, 2025 13:59

Merge branch 'main' into tfk/all-pubs-search

a173804

tefkah requested a review from 3mcd June 23, 2025 13:59

3mcd temporarily deployed to gh-654103159-pr-1290 June 23, 2025 14:04 Inactive

3mcd approved these changes Jun 24, 2025

View reviewed changes

Merge branch 'main' into tfk/all-pubs-search

575ba9b

3mcd had a problem deploying to gh-654103159-pr-1290 June 24, 2025 19:44 Error

3mcd merged commit 7363bad into main Jun 24, 2025
17 checks passed

3mcd deleted the tfk/all-pubs-search branch June 24, 2025 20:34

	/**
	* Get the users login data based on the session cookie
	*/
	export const getLoginData = cache(async (opts?: ExtraSessionValidationOptions) => {
	return validateRequest(opts);
	});

	const defaultPageOpts: ExtraSessionValidationOptions & LoginRedirectOpts = {
	allowedSessions: [AuthTokenType.generic, AuthTokenType.verifyEmail],
	};

	/**
	* Get the login data for the current page, and redirect to the login page if the user is not logged in.
	*/
	export const getPageLoginData = cache(
	async (opts?: ExtraSessionValidationOptions & LoginRedirectOpts) => {
	const options = opts ?? defaultPageOpts;
	const loginData = await getLoginData(options);

	if (!loginData.user) {
	redirectToLogin(options);
	}

	if (loginData.session && loginData.session.type === AuthTokenType.verifyEmail) {
	const pathname = getPathname();
	redirectToVerify({
	redirectTo: expect(pathname, "pathname is missing for redirectToVerify").toString(),
	});
	}

	return loginData;
	}
	);

	export type LoginData = Awaited<ReturnType<typeof getLoginData>>;

feat: search on all pub page #1290

feat: search on all pub page #1290

Uh oh!

Conversation

tefkah commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue(s) Resolved

High-level Explanation of PR

Test Plan

Screenshots (if applicable)

Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

3mcd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tefkah commented Jun 17, 2025 •

edited

Loading