Details

Reviewers

atul
ginsu
kamil
tomek
ashoat

Commits

rCOMMb813c7baa64b: [keyserver/lib] Set up the endpoints and responder for retrieving media

Summary

This diff handles setting up the endpoints and responder for actually fetching the media for a given thread. The changes made are:

Set up the endpoint (keyserver/src/endpoints.js) and the type (lib/types/endpoints.js)
Use callServerEndpoint in lib/actions/thread-actions.js to hit the newly created endpoint, and route to the appropriate responder.
Handle the validation of the request and call fetchMediaForThread from the responder in keyserver/src/responders/thread-responders.js

Linear: https://linear.app/comm/issue/ENG-2874/set-up-the-endpoints-and-responder-for-retrieving-media

Depends on D6485

Test Plan

Ran flow for all of the new type introductions, and this will be tested in the next diff where we actually use useServerCall to retrieve the media.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Feb 2 2023, 9:10 AM

Herald added a subscriber: ashoat. · View Herald TranscriptFeb 2 2023, 9:10 AM

rohan edited the summary of this revision. (Show Details)Feb 2 2023, 9:11 AM

Harbormaster completed remote builds in B16020: Diff 21866.Feb 2 2023, 9:21 AM

rohan requested review of this revision.Feb 2 2023, 9:21 AM

ashoat requested changes to this revision.Feb 2 2023, 11:00 AM

ashoat added inline comments.

keyserver/src/fetchers/upload-fetchers.js
120 ↗	(On Diff #21866)	What is the point of this change?
lib/types/thread-types.js
442 ↗	(On Diff #21866)	Don't think we need a separate type for this

This revision now requires changes to proceed.Feb 2 2023, 11:00 AM

rohan added inline comments.Feb 2 2023, 12:31 PM

keyserver/src/fetchers/upload-fetchers.js
120 ↗	(On Diff #21866)	You're right, I don't need `viewer` here so I can remove it from `fetchMediaForThread`. I'll do that in the revision
lib/types/thread-types.js
442 ↗	(On Diff #21866)	That's fair, I changed this just before putting up this revision to follow the general convention. I can remove both `ThreadFetchMediaResult` and `FetchThreadMediaPayload` types and just replace them directly with `$ReadOnlyArray<Media>` in the method headers.

Remove ThreadFetchMediaResult and FetchThreadMediaPayload and replace them with $ReadOnlyArray<Media> directly. Also remove the unused viewer parameter

Looks like some commits got combined, will fix this

Harbormaster completed remote builds in B16048: Diff 21921.Feb 2 2023, 3:18 PM

Hopefully fix the git issue?

Harbormaster completed remote builds in B16049: Diff 21922.Feb 2 2023, 3:33 PM

You should keep ThreadFetchMediaResult – otherwise it will be ready hard to update the result to include something new in the future, since it would break older clients

Looks like some commits got combined, will fix this

Encourage you to think critically about your workflow. This sort of thing generally shouldn't happen unless you're being "messy" with git

This revision now requires changes to proceed.Feb 3 2023, 10:50 AM

Bring back ThreadFetchMediaResult, I realize that it definitely makes sense to make future changes (if needed) much simpler

Harbormaster completed remote builds in B16084: Diff 21971.Feb 3 2023, 4:19 PM

ashoat requested changes to this revision.Feb 3 2023, 8:15 PM

ashoat added inline comments.

lib/types/thread-types.js
438 ↗	(On Diff #21971)	Otherwise it's hard to extend this later to eg. return something else as well

This revision now requires changes to proceed.Feb 3 2023, 8:15 PM

rohan added a child revision: D6587: [keyserver] Use the internal-ip package in the dev environment to ensure physical device / emulators can access uploads.Feb 4 2023, 3:28 PM

Update types and add support for optional param limit

rohan added inline comments.Feb 4 2023, 8:35 PM

keyserver/src/fetchers/upload-fetchers.js
123	Changing return type here now that `ThreadFetchMediaResult` to make it easier to change what's returned if the type is modified

Harbormaster completed remote builds in B16140: Diff 22032.Feb 4 2023, 8:40 PM

ashoat accepted this revision.Feb 6 2023, 7:09 AM

ashoat added inline comments.

lib/actions/thread-actions.js
173	I would just return `response` here and have the return type be `ThreadFetchMediaResult`. It's not as important as the return type from the server (since the server will need to support old clients in perpetuity), but it would make it more convenient for your `_SUCCESS` action's payload to be an object, which will make it easier to extend that payload in the future without having to update reducer code. It also lets you pass the same object through the whole workflow, which is arguably more convenient
lib/types/thread-types.js
443	I think `limit` won't be enough for paginating... you'll probably need a `cursor` of some sort as well. Not sure if pagination is intended to be addressed later – bringing this up here since you've presumably started work on it given the `limit` parameter

This revision is now accepted and ready to land.Feb 6 2023, 7:09 AM

rohan added inline comments.Feb 6 2023, 10:33 AM

lib/actions/thread-actions.js
173	That's definitely fair, I can just return `response` here and in the gallery component extract the `media` from the response
lib/types/thread-types.js
443	I was planning on using something like `OFFSET` to prevent loading every single media all at once. This way I could fetch more media on scroll with a specific offset (say 30 images each time) onEndReached

Address feedback, make limit required and introduce offset. Also to prevent passing in more and more parameters to fetchMediaForThread, I changed it to just accept request: ThreadFetchMediaRequest

Harbormaster completed remote builds in B16196: Diff 22160.Feb 6 2023, 2:18 PM

ashoat added inline comments.Feb 7 2023, 4:53 AM

keyserver/src/fetchers/upload-fetchers.js
126–127 ↗	(On Diff #22160)	Nit: I would just inline these in `query` rather than using `.append`

Inline offset and limit statements

Harbormaster completed remote builds in B16225: Diff 22190.Feb 7 2023, 8:41 AM

Rebase

Harbormaster failed remote builds in B16382: Diff 22419!Feb 13 2023, 6:48 AM

Check CI failures log and try to fix them

Try to resolve CI failures

Harbormaster completed remote builds in B16389: Diff 22426.Feb 13 2023, 7:12 AM

Harbormaster completed remote builds in B16390: Diff 22427.Feb 13 2023, 7:16 AM

Harbormaster completed remote builds in B16391: Diff 22428.Feb 13 2023, 7:20 AM

rohan mentioned this in D6485: [keyserver] Create a function to query for all uploads from a provided threadID.Feb 17 2023, 3:44 PM

Resolve any type now that ThreadFetchMediaResult and ThreadFetchMediaRequest are introduced, allow ThreadFetchMediaResult to
return an adjustedOffset (see previous diff for context), and allow ThreadFetchMediaRequest to accept an array of media IDs (string).