feat(components-native): Expose Isolated Aspects of FormatFile

[ZakaryH]

Motivations

we have a case in product where we would like to use FormatFile for a handful of the features like handling files vs images and all the thumbnail display properties. there are a few issues though. we want to make a dynamic grid, and we plan to have N amount of thumbnails.

there are a few problems:

it knows about layout and implements it in an odd way with styleInGrid but it doesn't give you control over the dimensions. it's a hard coded value, that comes with margin. this is waaay too prescriptive for the component. it should know nothing about a layout at all. there are plenty of ways to make a grid that don't involve a margin right that takes nothing into account.

on top of all that, it has no way to set the dimensions of the container. there's a whole bunch of funky stuff we're doing between the FileView and MediaView with aspect ratios and hard coded values, but then parent components with overflow: hidden.

each FormatFile gets a bottomsheet. I don't understand this decision, because we have things like ThumbnailList that map over files and create a FormatFile for each. that means we have N bottom sheets. this is really not an efficient pattern, there's no need for 25 bottom sheets just because you have 25 images. we should never do this and it is another big problem with the component that we should address later.

so, we need to be able to give dimensions to the Thumbnail-ish part, without built in styles, and without these bottom sheets and pressable functionality built in

Changes

Added

created and exported a new FormatFileThumbnail component. this isolates what is arguably the core responsibility of this component. it opts out of layouts, bottomsheets and allows to configure the dimensions. IF this was meant to be long lived, I would consider some presets for dimensions like we have on web's FormatFile, but I would much rather refactor this component in the near future.

with that in mind, this small export is a wrapper around most of the other parts. it does carry on the anti-pattern of useCreateThumbnail which violates the rules of hooks, but this is existing and again a reason to do a proper refactor. we can allow consumers to use this for now and avoid many of these issues, and later do the refactor and ideally deprecate and replace just this piece which should be not too bad because it's doing 1-1.5 things rather than 3+.

Changed

split up the component into some sharable pieces so the new component can leverage what is already there and we can avoid altering existing usages.

Deprecated

Removed

Fixed

fixed a problem with the faulty regex that was causing CI failures

Security

Testing

since this is RN, you'll need to install this on a real device. storybook will not be able to do much. also, it's worth mentioning that I'm not even sure this component belongs in Atlantis....it's so heavily tied to uploads. anyways, that's for another day.

existing FormatFiles should remain the exact same handling loading states, and bottom sheets. thumbnailLists too since they use this with the styleInGrid prop

for new usages, try using the FormatFileThumbnail on its own. it should work and handle everything except layouts, and presses/bottomsheet interactions. you should be able to determine the dimensions.

Changes can be
tested via Pre-release

---

In Atlantis we use Github's built in pull request reviews.

[cloudflare-workers-and-pages]

Deploying atlantis with    Cloudflare Pages

Latest commit:	6529768
Status:	✅  Deploy successful!
Preview URL:	https://519eb933.atlantis.pages.dev
Branch Preview URL:	https://job-151214-mobile-formatfile.atlantis.pages.dev

View logs

[ZakaryH]

was correctly getting a CI warning/failure about Polynomial regular expression used on uncontrolled data

there were two problems

In regex, * doesn't mean "anything" like it does in a file search. It means "repeat the thing before me zero or more times." So:
~* means "zero or more tilde characters" — i.e. match ~, ~~, ~~~, or nothing at all
.* means "zero or more of any character" — this is probably what the author wanted
The author likely confused shell-style wildcards (where * means "anything") with regex syntax.

The | in regex means "or", but it splits the entire pattern, not just the part near it.
/~*.doc$|docx$/ reads as: match ~*.doc$ OR match docx$. The second half has no connection to ~* at all. It's two completely separate patterns glued together.

Because ~* matches tildes and . also matches tildes, the regex engine gets confused on input with lots of tildes. If you feed it ~~~~~~~x, it tries: "what if ~* eats all 7 tildes and . gets x? No match. What if ~* eats 6 and . gets the 7th tilde? No match..." and so on. It has to try every possible split. With a long enough string of tildes, this gets slow — that's the "polynomial" backtracking the alert is about.

[ZakaryH]

reading the docs we have for FormatFile, I don't understand.

When contributing to, or consuming the FormatFile component, consider the following:

FormatFile components should take up the full width of the parent container (1 or 2 files per row)
The delete button will only be displayed if the callback function is passed in

why should it only be one or two per row? a parent container is of arbitrary width. we cannot know what the appropriate amount would be.

[github-actions]

Could not publish Pre-release for 36ad2d3. View Logs
The problem is likely in the NPM Publish or NPM CI step in the Trigger Pre-release Build Job.

Previous build information:

Published Pre-release for 36ad2d3 with versions:

  - @jobber/components-native@0.95.5-JOB-151214-36ad2d3.17+36ad2d3b9

To install the new version(s) for Mobile run:

npm install @jobber/components-native@0.95.5-JOB-151214-36ad2d3.17+36ad2d3b9
```.

For more troubleshooting steps, see the Pre-release Troubleshooting Guide

[Aiden-Brine]

I left some comments but nothing too major. I did some manual testing as well and things are looking good 👍

[Aiden-Brine]

I think this should be wrapped in a useMemo as it is getting called on every render. createUseCreateThumbnail is not very expensive but I think the effect is that the value of useCreateThumbnail will change on every render. Since that is passed to the provider I think that will cause other unnecessary re-renders.

[ZakaryH]

to be honest, I am not really sure what will happen with this code.

it's named like a hook, but it's a function that is NOT a hook nor a component calling hooks, which is not allowed, using an effect inside a useCallback which is also not allowed.

I can see if a useMemo helps, but it needs a lot more than that 😄

[ZakaryH]

actually wait, I don't think that will help. if we make a useMemo, createThumbnail will be a dependency since that's the only input.

on the line above though, we can see that when we don't have a crateThumbnail function, we make a brand new, anonymous arrow function each time. so that's not stable, and would have the exact same result with the additional overhead of a useMemo

so we have to fix that too....

oh, and then we are passing an object literal as the value to the context, meaning that even if all this was somehow memoize properly, we're creating a new object every render so everything above does nothing.

finally, even in the non arrow function case, if a consumer passes a non stable function we're right back to square one.

if this code was sane, I would 100% agree with you. however in the state it is in, I don't think a useMemo or anything short of a refactor can help us.

[jdeichert]

Just jumping in for a quick moment, note that createUseCreateThumbnail is in a really bad state... we have a ticket JOB-137525

[Aiden-Brine]

I think you should do one of three things here:

• Make this required
• Add a default in this component
• If leaving optional with no default, adding to this docstring what the experience will be if not provided.

[Aiden-Brine]

I know this is indirectly tested via FormatFileThumbnail.test.tsx . What do you think about adding dedicated unit tests for it to make sure all of the edge cases are covered? I don't feel too strongly on this so if you think that would be overkill I am okay to leave those out.

[ZakaryH]

that's a good call out. I think some units tests would provide more coverage with relative ease. I'll add some.

[Aiden-Brine]

I think you need to add in a useRealTimers to clean things up

[Aiden-Brine]

I think this is redundant?

[ZakaryH]

good catch, it definitely is.

[Aiden-Brine]

Worth adding some docs to the docs site for this or do you want the component to evolve a bit more before that?

[ZakaryH]

I'd rather not advertise this one. this is more or less a workaround to get something unblocked, with minimal changes that don't make us significantly more committed to this approach.

if we had the time I would say the component needs a moderate to total overhaul, and for us to ask ourselves if it even belongs in the design system anymore.

[edison-cy-yang]

This logic was already here so we don't have to address this, but videoExtensions in constants.ts has a few more video file types mov, 3gpp, and hevc. This means files without video MIME type will fall through to generic file icon.

[edison-cy-yang]

parseFile is inexpensive computation but gets called on every render, should we perhaps useMemo this?

[edison-cy-yang]

This is an educated guess, I have not verified.
I know this is intended to be used with virtualized list, so the component can be recycled to be used with different files. The way this useState is setup, it's only going to get evaluated on initial render, and showOverlay will remain true until onUploadComplete. I wonder if showOverlay state will remain false for the next file that recycles this component?

[ZakaryH]

I know this is intended to be used with virtualized list, so the component can be recycled to be used with different files.

not exactly. the way this component gets used is that you have one for each file, so much like ThumbnailList.tsx does array.map(() => <FormatFile />

we'll have N FormatFileThumbnails so each one would only be used for a single file, never re-used or recycled for a different image.