Favorite renaming#30
Conversation
new file: drizzle/0003_favorites_add_name.sql modified: package-lock.json modified: package.json modified: src/__test_utils__/seed.ts modified: src/backend/routes/favorites.test.ts modified: src/backend/routes/favorites.ts modified: src/frontend/cards/card-components.tsx modified: src/frontend/favorites/favorite-button.tsx modified: src/frontend/favorites/favorite-card.tsx modified: src/frontend/favorites/favorite-menu.tsx modified: src/shared/api-models.ts modified: src/shared/schema.ts
new file: src/frontend/favorites/favorite-search.test.ts new file: src/frontend/favorites/favorite-search.ts modified: src/frontend/favorites/favorites-list.tsx modified: src/shared/search.ts
modified: src/frontend/favorites/favorite-menu.tsx
modified: src/backend/routes/favorites.test.ts modified: src/backend/routes/favorites.ts
There was a problem hiding this comment.
I think for the create endpoint we can simplify the sortOrder logic to:
const existingFavorites = await db.$count(
favorites,
and(eq(favorites.userId, userId), eq(favorites.libraryId, libraryId))
);
await db
.insert(favorites)
.values({
id: favoriteId,
userId,
libraryId,
insertableId,
sortOrder: existingFavorites + 1
})
.onConflictDoNothing();
Which just sets the sortOrder to the number of existing favorites+1 more directly
AlexKempen
left a comment
There was a problem hiding this comment.
It's looking good, you can see my comments above.
I also realized our search logic was pretty scuffed and/or scattered, so I added a typed SearchResultDocument helper and renamed/re-organized our existing search implementation so the organization is a bit clearer, so make sure you run git pull the next time you work on it so you get my changes/don't run into conflicts
There was a problem hiding this comment.
Here's an existing pattern we use for updating a row
const body = await c.req.json<{ theme?: Theme; libraryId?: LibraryId }>();
const db = getDb(c.env.DB);
await db.insert(users).values({ id: userId }).onConflictDoNothing();
if (Object.keys(body).length > 0) {
await db.update(users).set(body).where(eq(users.id, userId));
}
| }); | ||
| }; | ||
|
|
||
| const isSearchActive = Boolean(uiState.searchQuery && searchHit); |
There was a problem hiding this comment.
&& is already a boolean, so Boolean() is redundant
|
|
||
| const isSearchActive = Boolean(uiState.searchQuery && searchHit); | ||
| const titleComponent = | ||
| isSearchActive && searchHit ? ( |
There was a problem hiding this comment.
Double checking searchHit (already checked in isSearchActive)
| isSearchActive && searchHit ? ( | ||
| <SearchHitTitle title={favoriteName} searchHit={searchHit} /> | ||
| ) : ( | ||
| <TextInput |
There was a problem hiding this comment.
This TextInput (and related hooks and whatnot) feels like a separate EditTitle component that takes an onEditComplete callback
| }); | ||
| } | ||
|
|
||
| function useUpdateFavoriteNameMutation() { |
There was a problem hiding this comment.
I really like how this is a seperate custom hook to encapsulate the mutation logic!
| const disabled = props.disabled ?? false; | ||
| const isHidden = props.showHiddenTag ?? false; | ||
|
|
||
| let cardTitle: ReactNode; |
There was a problem hiding this comment.
It would probably be best to rename this component CardTitleGroup and have it wrap a different CardTitle component that composes a standard title, SearchHitTitle, and EditTitle components. See https://react.dev/learn/passing-props-to-a-component#passing-jsx-as-children for info on how you can type/use the component, the end pattern you're looking for is:
<CardTitleGroup thumbnailUrls={thumbnailUrls} buildStatusBadge={badge}>
<CardTitle title={title} searchHit={searchHit} isEditing={isEditing} />
<CardTitleGroup />
The EditTitle component can come from your editable title logic below.
| interface FavoriteSearchDocument { | ||
| id: string; | ||
| name: string; | ||
| favoriteId: string; |
There was a problem hiding this comment.
I think it would be good to also include vendors: Vendors[] in here so we can easily display the number of favorites filtered out (the pattern should match the existing pattern). I also added a filter block to the search function you can use to implement
| defaultConfiguration: text("default_configuration", { | ||
| mode: "json" | ||
| }).$type<Configuration | null>(), | ||
| name: text("name").notNull().default(""), |
There was a problem hiding this comment.
Favorite name should be nullable and null by default, meaning "use the name of the insertable since the user hasn't set one", since otherwise admin renames won't update people's favorites by default
| } | ||
|
|
||
| const FAVORITE_SEARCH_OPTIONS: Options<FavoriteSearchDocument> = { | ||
| fields: ["name", "favoriteId"], |
There was a problem hiding this comment.
Shouldn't need to index fields other than name (and vendors if you add it)
| searchDb.addAll( | ||
| favorites.map((favorite) => ({ | ||
| id: favorite.id, | ||
| name: favorite.name ?? "", |
There was a problem hiding this comment.
We do need the insertable name in the searchDb when favorite name is undefined, which probably means passing in insertables. You could also probably write a helper like getInsertableName(favorite, insertable) which returns "" if the insertable doesn't exist
Favorites can now be renamed inline. The favorite search now searches by the favorite id and name rather than the insertable. The changes to packages and the addition of the coverage folder were done automatically and I have no idea what they do.