TurboSnap should treat uncommitted builds like missing commits

From AP-3778

How is the user affected? And what is the expected behavior?

Consider the following sequence of builds:

A - A* - A**

A* is an uncommitted build on commit A with a first set of changes
A** is a second uncommitted build on commit A with a different set of changes.

Currently TS would only take the changes in A** into account (ie the difference between A and A**) and could miss stories that changed (and were accepted) in A* but not in A**.

How many and/or what class of users does this impact?

Folks using TS & the VTA. Likely to be most of our existing customers.

Is there a workaround?

Avoid TS.

What are the steps for reproducing the issue?

Make some changes, don't commit
Run a local build (A*)
Accept the changes
Undo the changes, make changes to different CSF files (or no changes at all).
Run a second local build (with TS) (A**)
Notice that you don't get prompted to accept the "undoing".

Other information

A solution that extends our previous approaches would be to treat A* like it was missing from the repository, similar to the solution to AP-543. So we'd still diff A** and A, but we'd also take into account whichever stories changed between A and A*. IIRC this wouldn't be a big change as we already have this logic for the case where the middle commit entirely does not exist. Basically in here we would treat !!build.uncommittedHash in a similar way to an error fetching build.commit:

https://github.com/chromaui/chromatic-cli/blob/5366e6b8a73f606cdf48bec69176c090bdaf58c4/node-src/git/getChangedFilesWithReplacement.ts#L24

Probably the way to do that would be to tweak this logic to treat a build with build.uncommittedHash in the same way that we treat builds where getChangedFiles errors due to missing commits.

A couple of things to check with replacement builds:

Ensure that a build the same commit can be a replacement (e.g. we can use A as a replacement of A*)
Ensure we never use builds with uncommittedHash as replacement build.

An alternative solution would be to implement hash based TS (this is not planned any time soon).

See the proposal in the mentioned ticket.

Comments from the original ticket:

@tom How much of an effort is this? How soon should this be tackled? Gert and Matt have a quite a bit on their plate to finish before the end of the supercycle. via @thafryer

How much of an effort is this?

I think it wouldn't be super difficult as it's just getting an existing code path to apply to a new scenario. However I'm not completely sure, there's always the potential for complications.

How soon should this be tackled?

Hard to say. My feeling is that it would be a bad bug if it happens, but I think it's still theoretical at this point. I don't think it's a blocker to using the addon, simply something that might reduce confidence in the addon if encountered.

via @tmeasday

chromaui / addon-visual-tests