Details

Reviewers

Commits

rCOMM511682b3a71d: [lib] Introduce MessageSpec.getLastUpdatedTime
rCOMM642c7eb1e0a3: [lib] Introduce MessageSpec.getLastUpdatedTime

Summary

This new method on the MessageSpec will allow us to address ENG-9558. If it returns null or undefined that means that the message should not bump the thread's timestamp. If it returns a Promise that means we don't yet know if it should bump the thread's timestamp.

For now, in this diff we're ignoring any messages whose MessageSpec.getLastUpdatedTime returns a Promise, and assuming it's the same as if it returned null. In later diffs we'll update the logic to be smarter.

Depends on D13912

Test Plan

Tested in combination with the rest of the stack. See video in the last diff

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ashoat created this revision.Nov 12 2024, 12:41 PM

ashoat added a child revision: D13914: [lib] Extract getSidebarItems.

ashoat mentioned this in D13916: [lib][web] Await lastUpdatedTime and update ChatThreadItems if different.Nov 12 2024, 12:43 PM

Lint fixes

Harbormaster failed remote builds in B32624: Diff 45760!Nov 12 2024, 1:07 PM

Harbormaster completed remote builds in B32631: Diff 45767.Nov 12 2024, 2:10 PM

ashoat requested review of this revision.Nov 12 2024, 2:10 PM

Overall, this logic makes sense, but I think we should try to make it easier to understand.

lib/hooks/thread-time.js
41–77 ↗	(On Diff #45767)	I have a hard time trying to understand this logic - I tried to read it a couple of times, but it still confuses me. Are there some ways of making it less confusing? The things that make this logic confusing to me include: Mutability of the variables (`lastUpdatedTime` and `lastUpdatedAtLeastTime`) A couple of variables that are named in a similar way A for loop that is broken based on an unintuitive condition (it is explained in the summary, though) From these, mutability might be the main reason behind my feelings. I think we can consider adding some comments or avoiding mutating variables in a loop. There could be some other ways of making this more readable, though.

This revision is now accepted and ready to land.Nov 13 2024, 4:11 AM

ashoat added inline comments.Nov 13 2024, 10:55 AM

lib/hooks/thread-time.js
41–77 ↗	(On Diff #45767)	Thanks for writing this out. Admittedly this logic is somewhat complex. Regarding the pattern of using a `let` declaration to compose together multiple promises into one, we have this pattern in a couple other places in the codebase, such as here. Given we're iterating through a loop to determine these values (which are later returned), I think it's okay to use `let`. The alternative would be to build a collection and evaluate it at the end, which would probably be a bit worse for performance and I'm not sure would be any more readable. I'll add some comments to clarify the for loop and the mutable variables. Open to changing some variable names too, but to be honest, not sure what would be better.