LF- 5213: Farm notes offline support#4108
Conversation
* fix broken translation by converting object to function * add missing enqueueErrorSnackbar
| onSuccess: () => void; | ||
| onCancel: () => void; |
There was a problem hiding this comment.
Renamed onSuccess and onCancel to a single onClose callback to make it clearer why it's called after a network error.
There was a problem hiding this comment.
Extracted snackbar messages and the function into a separate file. Let me know if I broke your original intent!
There was a problem hiding this comment.
Thank you for making it so much cleaner + more extensible ❤️ It had become truly behemoth since that original file!
kathyavini
left a comment
There was a problem hiding this comment.
Thank you so much for taking on this development task, and for figuring out all the nuances of working with RTK Query for this!! Just a few hours of pnpm build + pnpm preview testing reminded me OFFLINE IS SO HARD! Thank you for establishing the patterns we can use going forward 🙏
The code looked excellent, and the only bug I could spot was in the isNetworkError function, which was giving false positive to all API errors.
| "ONLINE": "Note updated. Will save when you're back online", | ||
| "SUCCESS": "Note updated offline has been saved successfully" | ||
| }, | ||
| "FAILED": "Task changes failed to save", |
There was a problem hiding this comment.
"Note changes failed to save?"
There was a problem hiding this comment.
Thank you for making it so much cleaner + more extensible ❤️ It had become truly behemoth since that original file!
| 'farm_notes.create': { | ||
| successMessage: i18n.t('message:FARM_NOTE.SYNC.ADD.SUCCESS'), | ||
| errors: {}, | ||
| retryMessage: i18n.t('message:FARM_NOTE.SYNC.ADD.NETWORK_ERROR'), |
There was a problem hiding this comment.
Were you at all bothered by the task pattern being <FEATURE>.<ACTION>.SYNC and this one being <FEATURE>.SYNC.<ACTION>? 😅
I guess it's probably easier to group the SYNC messages together top-level so that should be our pattern going forward, right?
There was a problem hiding this comment.
I think so; it made more sense to me.
The task pattern uses <FEATURE> as the top-level key, but in farm note, it's already nested under SUCCESS/ERROR, so adding <FEATURE> felt redundant (here's how farm note looks with the task pattern).
If I'm missing something, please let me know!
| try { | ||
| await queryFulfilled; | ||
| } catch (error: any) { | ||
| if (isNetworkError(error)) { |
There was a problem hiding this comment.
Edit: how did this comment lose it's formatting??! 😅 Lost its link too
In tasks, we only show the to-be-synced snackbar when offline, not in the case of the API being down (which is also a network error). Here the to-be-synced when online would be shown in both cases. I think the original concern from tasks (two snackbars) doesn't exist anymore now that the queue is always replayed manually, though? 🤔
Should we remove the if (isOffline) check in the task sagas now so these two features behave equivalently?
In an ideal world, I think in both places we would use the NETWORK_ERROR string if the user is online, and the ONLINE string if the user is offline. Do you think that's worth the refactor in tasks? Or is it too much of edge case? I don't love where tasks ended up... when the API is down you get:
- optimistic update
- no snackbar explaining it
- and then (on replay of the queue) the series of "task ____ when offline" snackbars! 😬
Here you get the wrong (offline) snackbars, but it's still less confusing!
There was a problem hiding this comment.
Thank you for bringing this up, I didn't remember the discussion 🙏
In an ideal world, I think in both places we would use the NETWORK_ERROR string if the user is online, and the ONLINE string if the user is offline.
Let me quickly update it!
There was a problem hiding this comment.
Ah, the context aware-snackbars ARE so pleasing!! I love them.
| if (isNetworkError(error)) { | ||
| dispatch(enqueueSuccessSnackbar(i18n.t('message:FARM_NOTE.SYNC.ADD.ONLINE'))); | ||
| } else { | ||
| // Server error: rollback the optimistic updateconsole.error(error); |
There was a problem hiding this comment.
Is the console.error(error) meant to be active / on its own line?
There was a problem hiding this comment.
🫨 The component logs it, so it shouldn't be here. Thank you for catching 🙏🙏🙏
|
|
||
| export type QueryResult<T> = { data: T } | { error: FetchBaseQueryError }; | ||
|
|
||
| // In RTK Query, network errors result in undefined status or specific error structure |
There was a problem hiding this comment.
In which case were you able to create an error with undefined status? I saw both the offline case and the API down case matching the type of FetchBaseQueryError (the one imported in this file) which always had a status of 'FETCH_ERROR`!
Additionally, although this works in the component context (with .unwrap), when it's called from onQueryStarted, it is giving a false positive to every API error, because status is nested within error like this for a regular HTTP error:
So regular API errors like 404 were also giving the sync snackbar (from onQueryStarted) and then the regular error snackbar from the component:
Screen.Recording.2026-04-09.at.12.39.16.PM.mov
I think the logic can be updated like this:
export const isNetworkError = (error: any) => {
const status = error?.error?.status ?? error?.status;
return status === 'FETCH_ERROR';
};but again I wasn't able to identify an undefined status state (which this would not work for)...!
There was a problem hiding this comment.
I'm so sorry, that was an AI hallucination I didn’t catch 😭
Thank you so much!!!
SayakaOno
left a comment
There was a problem hiding this comment.
Thank you so much for reviewing!
I addressed your comments and added storeActivity('/', 'open_farm_notes') to record the event in offline_event_log table.
I'd appreciate another review 🙏
| 'farm_notes.create': { | ||
| successMessage: i18n.t('message:FARM_NOTE.SYNC.ADD.SUCCESS'), | ||
| errors: {}, | ||
| retryMessage: i18n.t('message:FARM_NOTE.SYNC.ADD.NETWORK_ERROR'), |
There was a problem hiding this comment.
I think so; it made more sense to me.
The task pattern uses <FEATURE> as the top-level key, but in farm note, it's already nested under SUCCESS/ERROR, so adding <FEATURE> felt redundant (here's how farm note looks with the task pattern).
If I'm missing something, please let me know!
| if (isNetworkError(error)) { | ||
| dispatch(enqueueSuccessSnackbar(i18n.t('message:FARM_NOTE.SYNC.ADD.ONLINE'))); | ||
| } else { | ||
| // Server error: rollback the optimistic updateconsole.error(error); |
There was a problem hiding this comment.
🫨 The component logs it, so it shouldn't be here. Thank you for catching 🙏🙏🙏
| try { | ||
| await queryFulfilled; | ||
| } catch (error: any) { | ||
| if (isNetworkError(error)) { |
There was a problem hiding this comment.
Thank you for bringing this up, I didn't remember the discussion 🙏
In an ideal world, I think in both places we would use the NETWORK_ERROR string if the user is online, and the ONLINE string if the user is offline.
Let me quickly update it!
|
|
||
| export type QueryResult<T> = { data: T } | { error: FetchBaseQueryError }; | ||
|
|
||
| // In RTK Query, network errors result in undefined status or specific error structure |
There was a problem hiding this comment.
I'm so sorry, that was an AI hallucination I didn’t catch 😭
Thank you so much!!!
kathyavini
left a comment
There was a problem hiding this comment.
Looks fantastic!! Thank you!
Description
getFarmNotesendpointqueryFnreturns the cached data to keep the query in "fulfilled" status; otherwise optimistic updates won't appear on the UI once the query moves to "rejected".useServiceWorkerListenerto handle farm note routes, consolidating snackbar messages in one place.RETRY_ROUTESinsw.js.Jira link: https://lite-farm.atlassian.net/browse/LF-5213
Type of change
How Has This Been Tested?
Checklist:
pnpm i18nto help with this)