-
-
Notifications
You must be signed in to change notification settings - Fork 133
feat(Bookmarks): Custom bookmark ordering #2493
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
base: master
Are you sure you want to change the base?
Conversation
af21dad
to
45b255d
Compare
dbdbe65
to
af658c8
Compare
api/resolvers/item.js
Outdated
for (let i = 0; i < itemIds.length; i++) { | ||
const itemId = Number(itemIds[i]) | ||
await models.bookmark.updateMany({ | ||
where: { | ||
userId: me.id, | ||
itemId | ||
}, | ||
data: { | ||
customOrder: i + 1 | ||
} | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will run itemIds.length
update queries one by one, in sequential order. It could be a potential bottleneck with a large number of bookmarks. For a small number, it may be ok.
If you want to use the updateMany
, but run the queries in parallel, one option could be
for (let i = 0; i < itemIds.length; i++) { | |
const itemId = Number(itemIds[i]) | |
await models.bookmark.updateMany({ | |
where: { | |
userId: me.id, | |
itemId | |
}, | |
data: { | |
customOrder: i + 1 | |
} | |
}) | |
} | |
await Promise.all( | |
itemIds.map((itemId, i) => | |
models.bookmark.updateMany({ | |
where: { userId: me.id, itemId: Number(itemId) }, | |
data: { customOrder: i + 1 }, | |
}) | |
) | |
) |
However, this still will fire itemIds.length
queries (but in parallel instead of sequentially)
A better option could be dynamically building a SQL query with something like this
for (let i = 0; i < itemIds.length; i++) { | |
const itemId = Number(itemIds[i]) | |
await models.bookmark.updateMany({ | |
where: { | |
userId: me.id, | |
itemId | |
}, | |
data: { | |
customOrder: i + 1 | |
} | |
}) | |
} | |
const caseStatements = itemIds | |
.map((id, i) => `WHEN ${id} THEN ${i + 1}`) | |
.join(' '); | |
await prisma.$executeRawUnsafe(` | |
UPDATE "Bookmark" | |
SET "customOrder" = (CASE "itemId" | |
${caseStatements} | |
END) | |
WHERE "userId" = ${me.id} | |
AND "itemId" IN (${itemIds.join(',')}) | |
`); |
Something like this should update all bookmarks in one sql statement.
I hope it helps!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting solution, hadn't considered this a bottleneck due to the practicality of re-ordering large lists of bookmarks frequently. However, for the custom SQL solution, I think @huumn mentioned avoiding using prisma.$executeRawUnsafe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we definitely do not want to use $executeRawUnsafe
.
You should be able to use Prisma client's in
operator.
Oh, I see, you are not updating all items with the same value so it's more complicated than just using the in
operator.
Still, there should be a way to do this in one query without executing unsafe SQL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ekzyis Sorry, I suggested using executeRawUnsafe
because I'm not that familiar with Prisma and I saw queryRawUnsafe
was being used already, but I Totally get the point of not using it 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
components/bookmark.js
Outdated
|
||
const { items, cursor } = useMemo(() => { | ||
if (!dat) return { items: [], cursor: null } | ||
return dat?.items || { items: [], cursor: null } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor comment, but dat
is always defined as it's checked above, so this ternary operator can be removed
return dat?.items || { items: [], cursor: null } | |
return dat.items || { items: [], cursor: null } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option could just be constructing the useMemo
like this:
const { items, cursor } = useMemo(() => dat?.items ?? { items: [], cursor: null }, [dat])
removing the redundant check and keeping the optional chaining
af658c8
to
78d2114
Compare
Description
fix #2480
Adding ability for users to manually order their bookmarks. Order stored in DB, using DND provider from wallets.
Screen.Recording.2025-09-08.at.20.41.09.mov
Screenshots
Additional Context
Was anything unclear during your work on this PR? Anything we should definitely take a closer look at?
Checklist
Are your changes backward compatible? Please answer below:
Y
For example, a change is not backward compatible if you removed a GraphQL field or dropped a database column.
On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
9
For frontend changes: Tested on mobile, light and dark mode? Please answer below:
Y
Did you introduce any new environment variables? If so, call them out explicitly here:
N