Support favorite renaming, also updated search to look for both the original name and favorite name#23
Support favorite renaming, also updated search to look for both the original name and favorite name#23Lyinlyon wants to merge 3 commits into
Conversation
the cloadflare favorites schema to be updated with a "favorite_name" column modified: src/backend/routes/favorites.ts 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
modified: src/frontend/search/search.ts modified: src/shared/search.ts
modified: src/frontend/favorites/favorite-menu.tsx
AlexKempen
left a comment
AlexKempen
left a comment
There was a problem hiding this comment.
I think it probably makes sense to fork SearchDocument+doSearch so we have FavoriteSearchDocument and doFavoriteSearch separate from the existing search code. doSearch has a fair amount of cruft that shouldn't be needed for searching favorites.
For the renaming change we talked about on the call, you could probably also look at what we do with the search bar if you want the favorite name to be fully selected when rename is clicked (similar to what Onshape does and what we do with search when you click on the search bar today).
| .set({ defaultConfiguration: body.defaultConfiguration }) | ||
| .where(eq(favorites.id, favoriteId)); | ||
| } | ||
| if (body.favoriteName !== undefined) { |
There was a problem hiding this comment.
Don't need !== undefined
| /** POST /api/default-configuration/:favoriteId */ | ||
| favoriteRoutes.post("/default-configuration/:favoriteId", async (c) => { | ||
| /** POST /api/favorite-configuration/:favoriteId */ | ||
| favoriteRoutes.post("/favorite-config/:favoriteId", async (c) => { |
There was a problem hiding this comment.
This can probably just be /favorite, comment should be consistent
| <CardTitle | ||
| disabled={isAssemblyInPartStudio} | ||
| title={insertable.name} | ||
| title={(favorite.favoriteName ??= insertable.name)} |
There was a problem hiding this comment.
Nullish coalescing assignment (??= vs. ??) is an affront to humanity (I assume you probably wanted just nulish coalescing)
| }); | ||
| }} | ||
| > | ||
| Edit favorite name |
There was a problem hiding this comment.
I would just put "Rename", and probably make this the first option/top-most option under insert
| return { hits, filtered }; | ||
| } | ||
| export function doFavoriteSearch( | ||
| searchDb: MiniSearch<SearchDocument>, |
There was a problem hiding this comment.
I think we should be customizing searchDocument so we don't need as much context passed in to doFavoriteSearch. If we have a FavoriteSearchDocument that uses favorite.id instead of insertable id and includes the neccessary search information (vendors, group name) then doFavoriteSearch probably no longer needs to take showHidden, favorites, and insertables
| if (!hitsById.has(insertable.id)) { | ||
| hitsById.set(insertable.id, { | ||
| id: insertable.id, | ||
| positions: generateHighlightPositionsFromText( |
There was a problem hiding this comment.
We don't currently highlight specific phrases in search results, just the entire word when it matches, so I probably wouldn't worry about implementing that now, just re-use the existing SearchHit implementation we have
| } | ||
|
|
||
| return { | ||
| hits: Array.from(hitsById.values()).slice(0, 50), |
There was a problem hiding this comment.
This shouldn't be neccessary
| hits: Array.from(hitsById.values()).slice(0, 50), | ||
| filtered: { | ||
| byVendor: filtered.byVendor + baseSearchResult.filtered.byVendor, | ||
| byGroup: filtered.byGroup + baseSearchResult.filtered.byGroup |
There was a problem hiding this comment.
Favorites can't be viewed inside a group (e.g., when a group is open) so byGroup logic isn't neccessary
|
|
||
| const tokenizedQuery = query.trim().toLowerCase(); | ||
|
|
||
| const baseSearchResult = doSearch( |
There was a problem hiding this comment.
I don't think I'm a big fan of calling doSearchResult in the favorite search, I feel like we'd be better off forking that and extracting shared code into helpers rather than trying to wrap around it
| "vitest": "^4.1.9", | ||
| "wrangler": "^4.93.0" | ||
| }, | ||
| "allowScripts": { |
There was a problem hiding this comment.
This seems mildly suss to me, is this something your AI needed to work?
2 participants
Package lock and package files were changed, probably can be ignored