User Details
- User Since
- Mar 18 2022, 8:34 PM (144 w, 3 d)
- Roles
- Disabled
Sep 15 2022
Addressed @tomek's feedback:
Sep 14 2022
Sep 12 2022
D4343, but looks good otherwise!
Sep 2 2022
Cool, I can see them now! But your revision is still in a Needs Revision state. That means it's been removed from reviewers' queues on the Phabricator home screen (under Ready to Review). If you want reviewers to see this diff, you should submit the Request Review action (same place as Abandon Revision). Every time you update a diff or want reviewers to take a look at it again, submit this action. If you want to remove a diff from reviewers' queues because you plan to make changes, submit the Plan Changes action.
Hey Ginsu, looks like your files weren't attached.
Instead of writing [DISREGARD], you can submit the Abandon Revision action!
Added apostrophe I missed
Sep 1 2022
Going to land this and address @tomek's question in a Linear issue for a (potential) separate diff. For now, I think clearing the queued actions in all cases is a valid way forward.
Aug 31 2022
Had this tab open for 2 hours, didn't realize @atul got a chance to look at this before me. Just submitted my feedback.
Hey @ginsu, your first diff, woohoo!
Read up on tcomb since I had never heard of it before, but still sort of unsure why we need these. Is it because tcomb checks types at runtime and Flow is static? This diff makes sense according to the documentation of tcomb, however, and matches the spec'd design laid out in D4982's diff summary.
Aug 30 2022
Also maybe you should show an example of the case when there are both images and videos in the same message, instead of just one video in the diff summary.
In the future, I'd also move to include a media table (discussed this with @atul offline) but for now this approach should work, and won't involve having to migrate queries, just modifying them to parse the new content column.
Yeah, the main thing that is confusing is uploadResult and uploadThumbnailResult (if video) being falsey after the try / catch block. Like as a reader this feels complicated. To me, I think the upload failing be handled in one section before moving onto the rest of the code where we can assume the uploads were successful, and we no longer have to introduce truthy checks. Obviously we may still need checks later on, but if we broke a lot of things into methods we could let this be handled outside of this huge function.
Patched this and tried it. Wow this should have been done long ago. Makes debugging keyserver so much easier
The current approach clears the changes when an error is detected, so we make it impossible to just retry. Maybe we should revisit it?
Aug 29 2022
So I assume steps 2-8 will be separate diffs in this stack.
Yup, this makes sense!
I'm a bit confused on what it means if uploadResult (or uploadThumbnailResult) fails. Is it just to push a failed 'upload' step to steps? @ashoat sort of mentioned this in D932, although he was referring to uploadThumbnailResult only.
We can just create a variable for it outside where uploadResult is declared, then it's clearer to me what each step means since we explicitly set the inner if statement's result to uploadThumbnailResult
Thanks for the NOTE, that helped me understand why this change can be landed as-is! Looks like the SQL query in upload-deleters will catch all these NULL-assigned containers. Some inline suggestions and questions.
Requesting changes for the inline edits, the footnote discussion can be a Linear issue or separate diff or something. Also curious for @ashoat's thoughts when he gets back, if he decides the footnotes are fine then it's ok. So maybe a Linear issue should be created, he should be subscribed, so when he sees it there he can respond!
Thanks for updating this diff! Accepting, assuming that this code is copied and pasted from dev_environment.md outside of the "Nix" headers.
almost done!
Aug 27 2022
Only the content of nix_dev_env.md changed significantly. The content of the nix_*_{setup,workflows}.md are copy and pasted from the the existing dev_environment.md with exception of initial headers.
Awesome! Can probably land this since all the builds passed.
Looks like the Android CI is failing here. Please don't request review (manually, by submitting an action) if the CI is failing unless you have a good reason. I've restarted the Android CI build to see if it passes again, but Phabricator automatically adds a "Changes Planned" action to your diff if any builds fail. When you request review manually, the Changes Planned tag gets overwritten by the Needs Review tag. But this isn't great, because if the CI is failing, your code can't be landed as-is (unless there's a good reason). So any review is probably unnecessary if the builds fail. Changes Planned removes the diff from reviewers' queues until the CI can pass again.
Aug 26 2022
Hmm...your before and after pics are mine. I didn't test this diff. Additionally, the after pic is just the current state of https://comm.app/, our public facing landing page. This diff broke in dev, not prod.
Remove object values for keys with same name
Use async / await instead of .then(...)
At a high level looks good, but why was updateNavInfoActionType in nav-types to begin with? Is there a difference between a nav type and action type? Or should we keep all "actionable" types together (anything with 'update', 'set', etc.), like in this diff?
Resigning since I don't have much context on adding new dependencies. But nice adding @ashoat here as a first-pass reviewer since he should take a look at all new dependencies!
One inline question.
Aug 25 2022
Destructure changeThreadSettings so editColor doesn't recompute each time props changes. I think the reason I was confused before is because we also import changeThreadSettings from lib/actions/thread-actions (line 9) but it is also passed in as a prop with the same name.
Address @tomek's feedback. Added useCallback to memoize functions and destructured props.