TurboSnap should treat uncommitted builds like missing commits

[ghengeveld]

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?

1. Make some changes, don't commit
2. Run a local build (A*)
3. Accept the changes
4. Undo the changes, make changes to different CSF files (or no changes at all).
5. Run a second local build (with TS) (A**)
6. 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.

[MichaelArestad]

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