Page MenuHomePhabricator

[native] Introduce `constructThreadTraversalNodes(...)`
ClosedPublic

Authored by atul on Apr 25 2023, 1:12 AM.
Tags
None
Referenced Files
F3188361: D7600.id25698.diff
Fri, Nov 8, 6:35 PM
F3188342: D7600.id25645.diff
Fri, Nov 8, 6:29 PM
F3186010: D7600.diff
Fri, Nov 8, 2:02 PM
Unknown Object (File)
Thu, Oct 31, 3:34 PM
Unknown Object (File)
Thu, Oct 31, 3:34 PM
Unknown Object (File)
Thu, Oct 31, 3:34 PM
Unknown Object (File)
Thu, Oct 31, 3:34 PM
Unknown Object (File)
Thu, Oct 31, 3:31 PM
Subscribers

Details

Summary

This function constructs an array of root ThreadTraversalNodes that will be consumed in updateRolesAndPermissions.

Note that these nodes JUST contain threadIDs and children and will be used for traversing the thread hierarchy. We will use these IDs to index into ThreadStoreThreadInfos (which is a map of threadID => rawThreadInfo). Considered different approaches, but this seemed the cleanest.

I introduced a bare bones updateRolesAndPermissions so this function isn't "dangling" and consume the bare bones updateRolesAndPermissions function in migration 38.


Depends on D7590

Test Plan

Produces tree of ThreadTraversalNodes:

c80b0d.png (1×610 px, 95 KB)

Unit tests added later in the stack

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul edited the test plan for this revision. (Show Details)
atul requested review of this revision.Apr 25 2023, 1:30 AM

Can you explain a little bit why you opted against using createRecursiveDrawerItemsData? Assume this is what you meant when you said "Considered different approaches, but this seemed the cleanest", but would like a bit more context

This revision is now accepted and ready to land.Apr 25 2023, 3:57 AM

Can you explain a little bit why you opted against using createRecursiveDrawerItemsData? Assume this is what you meant when you said "Considered different approaches, but this seemed the cleanest", but would like a bit more context

Here's some high level context, happy to go into more specifics IRL if necessary.

WRT using createRecursiveDrawerItemsData there were three points I considered:

  1. Using createRecursiveDrawerItemsData as-is and just ignoring the peripheral fields. One problem here is that the recursiveDrawerItems algorithm takes a maxDepth argument to determine how deep it traverses. From current use cases, maxDepth seems to be a UI consideration and doesn't reflect true depth of thread hierarchy. We don't have access to depth on client, but we could maybe crank up maxDepth to 999 or something crazy, but A. that's hacky B. there's some overhead... at a minimum we'll instantiate 999 empty arrays:

099e02.png (976×1 px, 245 KB)

We don't have to worry about depth with the recursive approach in this diff. It will recursively follow "pointers" (threadIDs) until the parentThreadID is null and then stop traversing.

  1. Updating createRecursiveDrawerItemsData to handle passing parentPermissions state to children. With the recursive approach in this diff we can easily pass parentPermissions to child nodes as an argument to the function call involving the relevant child nodes. However, with this iterative queue approach we would need to add additional data structures to hold onto this state AND we'd have to add all sorts of logic and multi-level indexing into these objects to get the right data for the right threadID/memberID/etc. There's also no easy way to index eg into the updated parent thread's roles field from the child node because child nodes have no visibility into updated parent node (no pointers pointing backwards). This wouldn't be an issue in the final implementation, but it WAS for earlier implementations when I was experimenting and matching keyserver functionality which I later realized was unnecessary:

3ae682.png (260×1 px, 69 KB)

At the time I made the decision, I thought I'd need to be able to access updated parent threadNodes in this way.

  1. Modifying createRecursiveDrawerItemsData to traverse thread hierarchy recursively instead of of iteratively with queue. This would solve the maxDepth issue. However, the "output" of this modified createRecursiveDrawerItemsData would be a constructed tree of nodes that is intended to be consumed by the UI. In our situation our intended output isn't a tree of threads, we're starting with ThreadStoreThreadInfos and we want to end up with ThreadStoreThreadInfos. There's now the overhead of having to transform the tree of updated nodes back into reconstructed ThreadStoreThreadInfos. If we were to keep with out design of "three passes," we would need to go from ThreadStoreThreadInfos -> CommunityDrawerItemData -> ThreadStoreThreadInfos -> CommunityDrawerItemData -> -> ThreadStoreThreadInfos -> CommunityDrawerItemData -> ThreadStoreThreadInfos. Maybe not the end of the world, we often go from one immutable object to another even though it's not as "efficient" as mutating... but my ThreadStoreThreadInfos is like 3.5MB so there's probably some actual overhead there.

Well why not produce the updated ThreadInfos in one pass?

I drafted a "single traversal" approach early on and found that things blew up in complexity pretty fast. There were ways to "reduce code," eg collapsing some of the members and currentUser permissions code... but it hurt legibility a lot. I think handling things in three passes makes things much easier to reason about. It also made things MUCH easier to reason about when debugging because I could isolate one of the "passes" at a time when troubleshooting inconsistencies. And since we're indexing directly into ThreadStoreThreadInfos there's little overhead in having multiple passes.

(Also, subjectively think it's easier to think through (and read through) traversing nodes recursively, rather than iteratively where you have to maintain a queue and think through levels and whatnot... but that wasn't a consideration here)


At a high level, the purpose of constructing any sort of tree was to A. give us ordering in which to update threadInfos B. give us a mechanism through which we can "pass" data to children. This approach achieves both in IMO a lightweight way that would be difficult to achieve with createRecursiveDrawerItemsData. Another aspect was speed of getting this out. It was much easier to create a new constructThreadTraversalNodes that handles just what I needed than to modify the existing createRecursiveDrawerItemsData and then also making sure it didn't break functionality when used on web, etc. Basically going with this approach was easier/faster and let me focus on recursivelyUpdatePermissions(ThreadTraversalNode)/recursivelyUpdateCurrentMemberPermissions(...) which I was much more worried about getting done/right. Figured we could always revisit traversal and whatnot later if necessary.

This revision was landed with ongoing or failed builds.Apr 25 2023, 12:28 PM
This revision was automatically updated to reflect the committed changes.