Implement conflict banner link to send feedback #17829
Conversation
aaazalea
force-pushed
the
jakob/KBFS-4150-pre-filled-log-send-action
branch
from
0d8e77f
to
b15101a
Compare
aaazalea
force-pushed
the
jakob/KBFS-4149-CR-banner-setup
branch
from
4a28073
to
ad2e089
Compare
|
@songgao I've addressed all of your feedback. I'm going to wait to rebase until the other PR is merged and you've looked at this again. |
shared/reducers/fs.tsx
Outdated
| (newState === 'in-conflict-stuck' || newState === 'in-conflict-not-stuck') | ||
| ) { | ||
| // If the conflict is being manually resolved, ignore new | ||
| // conflicts that come in. |
There was a problem hiding this comment.
If I understand correctly, the only state that InManualResolution can transit into is Finishing. After that, this TLF is deleted since all the needed data has been supposedly copied into the main one where the conflict state is cleared.
There was a problem hiding this comment.
but if we're in the middle of resolving a conflict, and then another one appears on the same TLF, what should we do?
There was a problem hiding this comment.
You mean after the TLF already branched into two? Then conflict can indeed happen again, but that'd be only on the server-view one, and it'd just be able to branch into a third TLF. The KBFS daemon would give it a different suffix in this case, so all three TLFs show up in favorites fine.
There was a problem hiding this comment.
Let's say that you have a TLF, jakob223,songgao. You get a conflict in it:
- jakob223,songgao [in-conflict-stuck]
and you press "Start conflict resolution". Now you have 2 TLFs: - jakob223,songgao [in-manual-resolution]
- jakob223,songgao (conflict High-level comments on major files #1) [in-manual-resolution]
and now you get a second conflict. I claim thatjakob223,songgaoshould not switch fromin-manual-resolutiontoin-conflict-stuckbecause that would hinder the ability of the user to finish resolving the first conflict. This amounts to forcing them to resolve the first conflict before another appears.
WDYT?
There was a problem hiding this comment.
it'd be like this:
jakob223,songgao
gets stuck
jakob223,songgao [conflicted]
hit start manual resolve
jakob223,songgao
jakob223,songgao (conflict #1)
first one (server view) get conflicted again
jakob223,songgao [conflicted]
jakob223,songgao (conflict#1)
hit manual resolve in the first one
jakob223,songgao
jakob223,songgao (conflict#1)
jakob223,songgao (conflict#2)
Note that branched out TLFs (with the conflict suffixes) can never get conflicted again -- they're a local branch and becomes readonly.
There was a problem hiding this comment.
But since we are only showing the "finish conflict resolution" button on the original / main-line TLF, we don't expose any way for the user to mark the current CR as finished in the
jakob223,songgao [conflicted]
jakob223,songgao (conflict#1)
state. Additionally, it's not clear which conflict should be marked as finished when the user clicks "finish" in jakob223,songgao in the case below:
jakob223,songgao
jakob223,songgao (conflict#1)
jakob223,songgao (conflict#2)
There was a problem hiding this comment.
You are right. Not sure what the best answer is here. Perhaps "finish cr" should be in the local view(s).
There was a problem hiding this comment.
Maybe we should rename "finish" to "delete this conflict view" or something.
There was a problem hiding this comment.
Yeah, that sounds good to me too
There was a problem hiding this comment.
Cool, made this change. Thanks for all the discussion 👍
pre-fill feedback form and take them to it. Issue: Y2K-42 Issue: KBFS-4150
aaazalea
force-pushed
the
jakob/KBFS-4150-pre-filled-log-send-action
branch
from
2eef245
to
cd383bd
Compare
|
I'm realizing some of my feedback here are actually already implemented in my plumbing PR. sigh |
Issue: Y2K-42