Skip to content
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

[Bug] Items created in the same second may not be shown in the bookmark grid due to pagination #140

Closed
MohamedBassem opened this issue May 5, 2024 · 5 comments
Labels
bug Something isn't working

Comments

@MohamedBassem
Copy link
Collaborator

When we're paginating using createdAt only, items created in the same second can result in duplicates or missing items.

@kamtschatka
Copy link
Contributor

kamtschatka commented May 17, 2024

I ran into this issue also just now. Of course this happens a lot if you import a lot of bookmarks using the cli (or in my case the PR to allow importing bookmarks via the UI)

I noticed that it is an easy fix in schema.ts by changing timestamp to timestamp_ms in the createdAtField.
I tried running the database migrations (https://docs.hoarder.app/Development/database), but it only adds
CREATE INDEX `bookmarkLinks_url_idx` ON `bookmarkLinks` (`url`);

I was hoping that would also create a migration of the existing timestamps, but seems like it doesn't.

@MohamedBassem
Copy link
Collaborator Author

@kamtschatka Changing the mode of the column is a software layer thing. You'll have to create an empty migration with pnpm drizzle-kit generate:sqlite --custom and write some SQL to multiple all the createdAt columns by 1000. However, using milliseconds makes the chances of the problem happening lower but it doesn't really solve the problem. The proper fix is to paginate both on createdAt and the id. But we'll need to be careful if we're to do that in a backward compatible way.

As for what you got when you generated a migration, that's actually my fault. I apparently forgot to create a migration when I implemented the URL dedups :) Will get that fixed.

@MohamedBassem
Copy link
Collaborator Author

Pushed c9dc23f to fix the dangling migration problem.

@kamtschatka
Copy link
Contributor

it would at least alleviate the problem, since that would then affect maybe 1 or 2 bookmarks instead of all of them, if you use the CLI.
I am not sure how much work it is though to improve the pagination using createdAt and id, maybe you can fix that really quick, then switching to timestamp_ms makes no sense.

@MohamedBassem
Copy link
Collaborator Author

I think fixing the pagination shouldn't be hard. I'll prioritize it specially in the context of your PR to support importing from the UI.

kamtschatka pushed a commit to kamtschatka/hoarder-app that referenced this issue May 24, 2024
MohamedBassem pushed a commit that referenced this issue May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants