Notifications #155
Notifications #155
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
I've checked the DB query and the rows read scales with the total number of notifications for a given user (including both read and unread). Not bad but ideally it would factor in the limit and/or read state. I tried adding indexes on created_at and read_at, but that didn't help. Maybe I need to tweak the query a little, or maybe new indexes don't take into consideration old data???? |
|
We agreed to mirror the bluesky UX as v1 which is to have no "mark as read" buttons and implicitly mark notifications as read when you open the page. This means we can simply revalidate the notifications on navigation because the read state is much more ephemeral. |
tom-sherman
force-pushed
the
un-77-notifications
branch
from
73a1d51 to
03247f4
Compare
|
@WillCorrigan this is a feature |
|
@timothyis here are the initial cards, not much to look at and a bit same-y all unread, dark mode: all unread, light mode: one marked as read (with the tick), dark mode: one marked as read (with the tick), light mode: |
| `id` integer PRIMARY KEY NOT NULL, | ||
| `did` text NOT NULL, | ||
| `created_at` text DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) NOT NULL, | ||
| `read_at` text, |
There was a problem hiding this comment.
should this also be strftime?
There was a problem hiding this comment.
It's nullable so doesn't have a default.
| : undefined, | ||
| ), | ||
| ) | ||
| .leftJoin( |
There was a problem hiding this comment.
if the comment is deleted from db we prob don't want the notification to still be shown, so probably make it an inner join
| schema.Comment, | ||
| eq(schema.Comment.id, schema.Notification.commentId), | ||
| ) | ||
| .leftJoin(schema.Post, eq(schema.Post.id, schema.Comment.postId)) |
There was a problem hiding this comment.
same with this, inner join to avoid null post situation
| id: integer("id").primaryKey(), | ||
| did: did("did").notNull(), | ||
| createdAt: dateIsoText("created_at").notNull().default(nowAsIsoString), | ||
| readAt: dateIsoText("read_at"), |
There was a problem hiding this comment.
you can default this to strftime and leave it nullable maybe?
There was a problem hiding this comment.
Can't default it because it needs to be null to indicate not read.
tom-sherman
force-pushed
the
un-77-notifications
branch
from
1222147 to
9a8e5cd
Compare
Todo: