LF-5231: Fix unread indicator missing for notes fetched after seen at is set#4110
Conversation
SayakaOno
force-pushed
the
LF-5231/Fix_unread_indicator_missing_for_notes_fetched_after_seen_at_is_set
branch
from
3830399 to
a003280
Compare
SayakaOno
changed the base branch from
integration
to
LF-5213/Farm_notes_offline_support
SayakaOno
changed the title
kathyavini
left a comment
kathyavini
left a comment
There was a problem hiding this comment.
Fantastic! The new logic makes total sense, and not calling the farm_notes_read endpoint every time the drawer is opened is 🔥
| */ | ||
| export const up = async function (knex) { | ||
| return knex.schema.alterTable('farm_notes_read', (table) => { | ||
| table.renameColumn('last_read_at', 'read_through'); |
There was a problem hiding this comment.
read_through is a bit ambiguous -- here it means "read (the notes) through to this particularly timestamp", but the meaning that actually came to mind for me first is "has read through the entire list". Maybe that's just me?
I think I would prefer read_up_to or last_seen_note_time? But if read_through makes more sense to you, then it's also okay.
There was a problem hiding this comment.
Thank you for the suggestions! I wasn't sure about this although I went with one of Claude's suggestions. I'll update it to read_up_to!
| method: 'POST', | ||
| }, | ||
| { | ||
| // This matcher will include farm_note/{uuid} paths |
There was a problem hiding this comment.
Re: the comment, should this now be /farm_notes/{uuid} ?
| const handleOpenDrawer = () => { | ||
| setIsDrawerOpen(true); | ||
| markFarmNotesRead(); | ||
| if (hasUnread) { |
| { | ||
| // This matcher will include farm_note/{uuid} paths | ||
| matcher: ({ url }) => url.pathname.includes('/farm_note'), | ||
| matcher: ({ url }) => url.pathname.includes('/farm_notes'), |
There was a problem hiding this comment.
Oh, this was equally true for the original code, but I didn't catch it!
This will already match /farm_notes_read so the lower/dedicated matcher is never used. Perhaps narrow this one with the trailing slash (/farm_notes/) or expand the comment to be explicit?
There was a problem hiding this comment.
Oh, thank you! I'll update the matcher!
…for_notes_fetched_after_seen_at_is_set
SayakaOno
left a comment
SayakaOno
left a comment
There was a problem hiding this comment.
Thank you so much for paying attention to the details! 🙏❤️❤️
I'd appreciate it if you could check the updates!
| { | ||
| // This matcher will include farm_note/{uuid} paths | ||
| matcher: ({ url }) => url.pathname.includes('/farm_note'), | ||
| matcher: ({ url }) => url.pathname.includes('/farm_notes'), |
There was a problem hiding this comment.
Oh, thank you! I'll update the matcher!
| */ | ||
| export const up = async function (knex) { | ||
| return knex.schema.alterTable('farm_notes_read', (table) => { | ||
| table.renameColumn('last_read_at', 'read_through'); |
There was a problem hiding this comment.
Thank you for the suggestions! I wasn't sure about this although I went with one of Claude's suggestions. I'll update it to read_up_to!
2 participants
Description
/farm_noteto/farm_notes.last_seen_atinfarm_notes_readtable toread_through.updated_atasread_through.markFarmNotesReadrequest when notes have been read.Jira link: https://lite-farm.atlassian.net/browse/LF-5231
Type of change
How Has This Been Tested?
Checklist:
pnpm i18nto help with this)