ERA-10594: Leverage `read` filter param in messages API calls from das-web-react #1259

gaboDev · 2025-03-21T17:28:46Z

What does this PR do?

Adds getUnreadMessagesCount duck, used to calculate unread messages batch and separate that responsibility from fetching the messages.

Relevant link(s)

Tracking tickets: ERA-10594
Hosted demo environments

gaboDev · 2025-03-21T17:32:57Z

src/Nav/MessageMenu.js

      .catch((error) => console.warn('error fetching messages', { error }));
  }, [dispatch]);

  useEffect(() => {
-    fetchMenuMessages();
-  }, [dispatch, fetchMenuMessages]);
+    if (subjects.length > 0){


This is needed because internally fetchMessagesSuccess action is calling messageIsValidForDisplay function which grabs data from subjectStore, so most of the times the store was empty/not-ready and the comparison threw a false negative

Hmm I don't think I understand the logic of this. I feel like this needs to be re-done, since we shouldn't care if the subjects are loaded or not. We just want to fetch the unread messages to show a badge, independently of the subjects that sent the message, why do we need to wait? I guess its a constraint of old logic, but we shouldn't rely on that, if we need to re-work old selectors, reducers, etc... Or simply add new ones, we should!

gaboDev · 2025-03-21T17:34:33Z

src/Nav/MessageMenu.js

-      .then((results = []) => dispatch(fetchMessagesSuccess({ results })))
+  const fetchMenuMessages = useCallback((getUnreadMessagesOnly = false) => {
+    const page_size = 100;
+    const params = !getUnreadMessagesOnly ? { page_size } : { page_size, read: false };


Since I believe We only want to use read:false to calculate the badge count I'm building the params conditionally since fetchMenuMessages is being used in several places of the component tree of this file

I think the condition could be simplified to:

fetchAllMessages({ page_size: MENU_MESSAGES_PAGE_SIZE, read: !getUnreadMessagesOnly })

Now, for the case where all we care is to know if we need to render the badge, do we need a page size of 100? Feels like all that data is unused, all we care is to know if there's one or more messages to show the badge but we don't care about the data. We could even send a page_size of 0 and only care about the count in the response right?.

gaboDev · 2025-03-21T17:39:20Z

I wanted to get ride of this filtering which is checking if there are unread messages

const unreads = state.results.filter((message) => !message.read);

But the MessageContext is also being affected in <MessageSummaryList/> in the useEffect in charge of loading recent messages so once recent messages are loaded, the selector in MessageMenu will provoke a re-render causing side-effects in the badge calculation.

gaboDev · 2025-03-21T17:42:45Z

Honestly, I got a bit lost trying to figure out the business logic for this implementation, is not very clear to me whats the purpose of this ticket beyond optimize that first call of fetchAllMessages to calculate the badge display, at this moment there is no clear description of whats expected for this ticket so I would like to leave this as draft PR and get some feedback and maybe some directions on whats is missing here to call this one read for review

luixlive

Some quesitons, not totally sure if I understand the approach we are following here. The code doesn't speak by itself. I see the read param being added but it still feels like this old code could be improved a lot. The conditions to call the endpoint are a bit confusing for me 🤔

luixlive · 2025-03-21T19:18:26Z

src/Nav/MessageMenu.js

-    fetchAllMessages({ page_size: 100 })
-      .then((results = []) => dispatch(fetchMessagesSuccess({ results })))
+  const fetchMenuMessages = useCallback((getUnreadMessagesOnly = false) => {
+    const page_size = 100;


This feels like a top of the file constant MENU_MESSAGES_PAGE_SIZE 😄

luixlive · 2025-03-21T19:21:32Z

src/Nav/MessageMenu.js

-      .then((results = []) => dispatch(fetchMessagesSuccess({ results })))
+  const fetchMenuMessages = useCallback((getUnreadMessagesOnly = false) => {
+    const page_size = 100;
+    const params = !getUnreadMessagesOnly ? { page_size } : { page_size, read: false };


I think the condition could be simplified to:

fetchAllMessages({ page_size: MENU_MESSAGES_PAGE_SIZE, read: !getUnreadMessagesOnly })

Now, for the case where all we care is to know if we need to render the badge, do we need a page size of 100? Feels like all that data is unused, all we care is to know if there's one or more messages to show the badge but we don't care about the data. We could even send a page_size of 0 and only care about the count in the response right?.

luixlive · 2025-03-21T19:22:38Z

src/Nav/MessageMenu.js

+    const params = !getUnreadMessagesOnly ? { page_size } : { page_size, read: false };
+    fetchAllMessages(params)
+      .then((results = []) => {
+        dispatch(fetchMessagesSuccess({ results }));


I know this is old code, but given the ticket and the refactor, shouldn't we take this logic to its own duck? Maybe a messages duck so the app has a single store for them?

luixlive · 2025-03-21T19:24:15Z

src/Nav/MessageMenu.js

      .catch((error) => console.warn('error fetching messages', { error }));
  }, [dispatch]);

  useEffect(() => {
-    fetchMenuMessages();
-  }, [dispatch, fetchMenuMessages]);
+    if (subjects.length > 0){


Hmm I don't think I understand the logic of this. I feel like this needs to be re-done, since we shouldn't care if the subjects are loaded or not. We just want to fetch the unread messages to show a badge, independently of the subjects that sent the message, why do we need to wait? I guess its a constraint of old logic, but we shouldn't rely on that, if we need to re-work old selectors, reducers, etc... Or simply add new ones, we should!

gaboDev · 2025-03-24T19:10:38Z

I have addressed the feedback received from @luixlive also I changed the approach a little bit after the discussion with @JoshuaVulcan about the expected output of this ticket which is to separate the calculation of unread messages count from the fetching of the messages only when the user clicks on the message icon to display the message feed.

luixlive

I think the main point of this review is that we shouldn't be handling the messages store with a React reducer. Messages is ER data, fetched from an endpoint, updated with the socket... There's no sense in attaching it to a React component, this data should be properly fetched, processed, stored, and globally retrievable with nice selectors.

luixlive · 2025-03-24T22:57:02Z

src/MessageList/ParamFedMessageList.js

@@ -84,9 +84,10 @@ const ParamFedMessageList = ({ isReverse, params, ...restProps }) => {

  useEffect(() => {
    if (!!unreads.length) {
-      bulkReadMessages(unreads.map(({ id }) => id));
+      bulkReadMessages(unreads.map(({ id }) => id))
+        .then(() => onMessageRead?.());


I feel like this callback could have a more explicit name: onUnreadMessagesRead maybe?

luixlive · 2025-03-25T15:36:14Z

src/ducks/messaging.js

@@ -35,6 +41,18 @@ export const removeMessageById = id => ({

 const { get, post } = axios;

+export const getUnreadMessagesCount = () => {
+  const paramString = objectToParamString(


I have two comments about this method:
1- nit: feels a bit like an overkill to use a helper method to build a string with three constant values, we could simply:

export const getUnreadMessagesCount = () => get(`${MESSAGING_API_URL}?include_additional_data=false&page_size=0&read=false`);

2- We are not following a good Redux duck pattern here. This is a duck file, and as such, its responsibility is to handle reducers and their actions. However this method is neither an action, action creator nor a reducer. And because of that, the component that is calling this method needs to do some manual handling of dispatches and error, which should be done here:

export const fetchUnreadMessagesCount = () => async (dispatch) => { try { const response = await axios.get(`${MESSAGING_API_URL}?include_additional_data=false&page_size=0&read=false`); const unreadMessagesCount = response.data.data.count; dispatch({ payload: unreadMessagesCount, type: UPDATE_UNREAD_MESSAGES_COUNT }) } catch (error) { console.warn('error checking for unread messages', { error }); } };

Following this approach, we no longer need updateUnreadMessagesCount and we don't need to do duck stuff in our component, so checkForUnreadMessages gets simplified to:

const checkForUnreadMessages = useCallback(() => dispatch(fetchUnreadMessagesCount()), [dispatch]);

The function was simplified so much that I don't think we need it anymore 😄 We could remove it and simply put dispatch(getUnreadMessagesCount()); wherever we need to fetch the count.

luixlive · 2025-03-25T15:42:10Z

src/ducks/messaging.js

@@ -67,6 +85,7 @@ export const INITIAL_MESSAGE_LIST_STATE = {
  next: null,
  previous: null,
  count: 0,


It's out of scope, but it would be nice to refactor this reducer a bit. It's just storing an Axios response object, instead of a more ER usable data structure of our data. That's why we end up needing complicated selectors that are triggered several times. If we processed the data in our actions and reducers to store them in a more usable way we would need less and simpler selectors. Even the new property you added stands out because it doesn't fit the response object properties haha. If it's not that complicated to do so, it would be a great addition to this PR to rework this a bit!

luixlive · 2025-03-25T15:45:35Z

src/Nav/MessageMenu.js

@@ -29,23 +36,36 @@ const MessageMenu = () => {

  const [selectedSubject, setSelectedSubject] = useState(null);

-  const unreads = state.results.filter((message) => !message.read);
-  const badgeCount = unreads.length > 9 ? '9+' : unreads.length;
+  const { unreadMessagesCount } = state;


Ok so seems like we are not using a Redux reducer but a React reducer. Could we migrate to Redux instead? A React reducer is attached to the lifecycle of the component that holds it (renders), while having to pass down its state and dispatch method (like we are doing with the context). We don't need to reinvent the wheel, and also we could take advantage of having the messages reducer everywhere in the app and even the usage of Redux selectors if we move it to Redux.

React reducers are still useful, it doesn't mean that we should have every single reducer in Redux. The main difference is that Redux is designed to be a global store, accessible anywhere in the app, with a defined flow of action -> thunk processing -> reducer -> selectors. Messages data, which comes from DAS and is part of the ER data, should be stored there. React reducers should be used to store component states, just like useState. I know you didn't do it, but I think this ticket is the perfect place to improve old code!

luixlive · 2025-03-25T15:58:23Z

src/Nav/MessageMenu.js


  const canShowMessageMenu = useMemo(
    () => !!state.results.length || !!subjects.filter((subject) => !!subject?.messaging?.length).length,
    [state.results.length, subjects]
  );

+  const onMessagesRead = useCallback(() => dispatch(updateUnreadMessagesCount(0)), [dispatch]);


Hmm, I see we are calling bulkReadMessages to mark all our messages as read in the backend, and then dispatching updateUnreadMessagesCount(0). Logically it makes sense, but we could stay with our backend data as the source of truth right? Instead of forcing a 0 we could call getUnreadMessagesCount again to make sure that the backend operation was successful and that it returns a count of 0 from DAS.

gaboDev · 2025-03-26T20:06:55Z

After some analysis, We decided to keep the approach as close as possible to the approach followed on this components.
A deep refactor is need here, I have created this ticket: ERA-11339 to handle this refactor separately so I the value of this improvement can be delivered a soon as possible.

luixlive

Nice work Gabo, and thanks for trying to improve this by moving those 3 component-level React reducers to a global store Redux reducer.

I understand it's too complicated and out of the scope for this ticket, but it is a huge area of opportunity we have since we are wasting memory and processing. A simple Redux reducer with all the messages with a couple memoized selectors would do the same work with less effort and memory, and our backend data wouldn't be tied to the lifecycle of a React component which is a terrible practice.

The PR looks good 👍

Adding read:false to get message duck

985be85

gaboDev added the deploy label Mar 21, 2025

gaboDev requested review from AlanCalvillo, luixlive and JoshuaVulcan March 21, 2025 17:28

gaboDev temporarily deployed to ERA-10594 March 21, 2025 17:28 — with GitHub Actions Inactive

gaboDev temporarily deployed to padas-app March 21, 2025 17:29 — with GitHub Actions Inactive

gaboDev commented Mar 21, 2025

View reviewed changes

luixlive requested changes Mar 21, 2025

View reviewed changes

Adding new duck to handle fetch of unread message count

1537b5f

gaboDev temporarily deployed to ERA-10594 March 24, 2025 19:07 — with GitHub Actions Inactive

gaboDev temporarily deployed to padas-app March 24, 2025 19:08 — with GitHub Actions Inactive

gaboDev marked this pull request as ready for review March 24, 2025 19:10

gaboDev requested a review from luixlive March 24, 2025 19:14

luixlive reviewed Mar 25, 2025

View reviewed changes

Adding feedback PR

26066ef

gaboDev temporarily deployed to ERA-10594 March 26, 2025 19:10 — with GitHub Actions Inactive

gaboDev temporarily deployed to padas-app March 26, 2025 19:10 — with GitHub Actions Inactive

Reset files

18408f8

gaboDev temporarily deployed to ERA-10594 March 26, 2025 19:11 — with GitHub Actions Inactive

gaboDev temporarily deployed to padas-app March 26, 2025 19:12 — with GitHub Actions Inactive

gaboDev requested a review from luixlive March 26, 2025 20:06

luixlive approved these changes Mar 26, 2025

View reviewed changes

gaboDev merged commit 319bb30 into develop Mar 26, 2025
3 checks passed

gaboDev deleted the ERA-10594 branch March 26, 2025 20:40

ERA-10594: Leverage read filter param in messages API calls from das-web-react #1259

ERA-10594: Leverage read filter param in messages API calls from das-web-react #1259

Uh oh!

Conversation

gaboDev commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Relevant link(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaboDev commented Mar 21, 2025

Uh oh!

gaboDev commented Mar 21, 2025

Uh oh!

luixlive left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaboDev commented Mar 24, 2025

Uh oh!

luixlive left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luixlive Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaboDev commented Mar 26, 2025 • edited by jira bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luixlive left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ERA-10594: Leverage `read` filter param in messages API calls from das-web-react #1259

ERA-10594: Leverage `read` filter param in messages API calls from das-web-react #1259

gaboDev commented Mar 21, 2025 •

edited

Loading

luixlive Mar 25, 2025 •

edited

Loading

gaboDev commented Mar 26, 2025 •

edited by jira bot

Loading