From fb0f33f310ba6b3ea6d2dac384d90c266183592e Mon Sep 17 00:00:00 2001 From: Evan Krause Date: Mon, 23 Sep 2024 10:55:14 -0700 Subject: [PATCH] Lookup head commit to push in modal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: "Commit & Push..." didn't work well, because it would not know the new commit hash to push. It would just use the head commit at modal creation time, which was wrong. I was back and forth about how to implement this. The trouble is that the commit we can look up the moment you click "commit & push..." can at best be the optimistic commit from the commit operation. But this will become a real commit shortly after opening the modal. In order for this not to disappear, we need to be able to use a reference that is maintained while the modal is open. I settled on passing a CommitInfo for the top of the stack, and looking up the stack in the Modal. This way, the stack can update if the underlying graph updates. Typically, this shouldn't change while the modal is open. But if it does, we should update the UI to show what clicking "push" would actually do—which is to act on the head commit. Reviewed By: quark-zju Differential Revision: D62669564 fbshipit-source-id: ef4ed11e9ba6bedd1ec8c160e02fcc4af6941191 --- .../isl/src/CommitInfoView/CommitInfoView.tsx | 45 ++++++++++++------- .../codeReview/github/BranchingPrModal.tsx | 14 +++--- .../github/BranchingPrModalContent.tsx | 16 +++++-- 3 files changed, 46 insertions(+), 29 deletions(-) diff --git a/addons/isl/src/CommitInfoView/CommitInfoView.tsx b/addons/isl/src/CommitInfoView/CommitInfoView.tsx index f62bdd592d884..76dbaf586b735 100644 --- a/addons/isl/src/CommitInfoView/CommitInfoView.tsx +++ b/addons/isl/src/CommitInfoView/CommitInfoView.tsx @@ -5,7 +5,6 @@ * LICENSE file in the root directory of this source tree. */ -import type {UICodeReviewProvider} from '../codeReview/UICodeReviewProvider'; import type {Operation} from '../operations/Operation'; import type {CommitInfo, DiffId} from '../types'; import type {CommitInfoMode, EditedMessage} from './CommitInfoState'; @@ -55,7 +54,7 @@ import {SetConfigOperation} from '../operations/SetConfigOperation'; import {useRunOperation} from '../operationsState'; import {useUncommittedSelection} from '../partialSelection'; import platform from '../platform'; -import {CommitPreview, uncommittedChangesWithPreviews} from '../previews'; +import {CommitPreview, dagWithPreviews, uncommittedChangesWithPreviews} from '../previews'; import {repoRelativeCwd, useIsIrrelevantToCwd} from '../repositoryData'; import {selectedCommits} from '../selection'; import {commitByHash, latestHeadCommit, repositoryInfo} from '../serverAPIState'; @@ -93,6 +92,7 @@ import {Badge} from 'isl-components/Badge'; import {Banner, BannerKind, BannerTooltip} from 'isl-components/Banner'; import {Button} from 'isl-components/Button'; import {Divider} from 'isl-components/Divider'; +import {ErrorNotice} from 'isl-components/ErrorNotice'; import {Column} from 'isl-components/Flex'; import {Icon} from 'isl-components/Icon'; import {RadioGroup} from 'isl-components/Radio'; @@ -1025,21 +1025,32 @@ function SubmitButton({ primary disabled={disabledReason != null} onClick={async () => { - const operations = await getApplicableOperations(); - if (operations == null || operations.length === 0) { - return; - } - - for (const operation of operations) { - runOperation(operation); - } - - const pushOps = await showBranchingPrModal(commit); - if (pushOps == null) { - return; - } - for (const pushOp of pushOps) { - runOperation(pushOp); + try { + const operations = await getApplicableOperations(); + if (operations == null || operations.length === 0) { + return; + } + + for (const operation of operations) { + runOperation(operation); + } + const dag = readAtom(dagWithPreviews); + const topOfStack = commit.isDot && isCommitMode ? dag.resolve('.') : commit; + if (topOfStack == null) { + throw new Error('could not find commit to push'); + } + const pushOps = await showBranchingPrModal(topOfStack); + if (pushOps == null) { + return; + } + for (const pushOp of pushOps) { + runOperation(pushOp); + } + } catch (err) { + const error = err as Error; + showToast(Failed to push commits} />, { + durationMs: 10000, + }); } }}> {commit.isDot && anythingToCommit ? ( diff --git a/addons/isl/src/codeReview/github/BranchingPrModal.tsx b/addons/isl/src/codeReview/github/BranchingPrModal.tsx index 85bf96f4a6be0..280b6243b1fda 100644 --- a/addons/isl/src/codeReview/github/BranchingPrModal.tsx +++ b/addons/isl/src/codeReview/github/BranchingPrModal.tsx @@ -10,8 +10,6 @@ import type {CommitInfo} from '../../types'; import type {GithubUICodeReviewProvider} from './github'; import {T} from '../../i18n'; -import {readAtom} from '../../jotaiUtils'; -import {dagWithPreviews} from '../../previews'; import {showModal} from '../../useModal'; import {codeReviewProvider} from '../CodeReviewInfo'; import {ErrorNotice} from 'isl-components/ErrorNotice'; @@ -22,10 +20,8 @@ import {lazy, Suspense} from 'react'; const BranchingPrModalContent = lazy(() => import('./BranchingPrModalContent')); export function showBranchingPrModal( - topOfStackToPush: CommitInfo, + topOfStack: CommitInfo, ): Promise | undefined> { - const dag = readAtom(dagWithPreviews); - const stack = dag.getBatch(dag.ancestors(topOfStackToPush.hash, {within: dag.draft()}).toArray()); return showModal | undefined>({ maxWidth: 'calc(min(90vw, 800px)', maxHeight: 'calc(min(90vw, 600px)', @@ -34,16 +30,16 @@ export function showBranchingPrModal( type: 'custom', dataTestId: 'create-pr-modal', component: ({returnResultAndDismiss}) => ( - + ), }); } export function CreatePrModal({ - stack, + topOfStack, returnResultAndDismiss, }: { - stack: Array; + topOfStack: CommitInfo; returnResultAndDismiss: (operations: Array | undefined) => unknown; }) { const provider = useAtomValue(codeReviewProvider); @@ -63,7 +59,7 @@ export function CreatePrModal({ diff --git a/addons/isl/src/codeReview/github/BranchingPrModalContent.tsx b/addons/isl/src/codeReview/github/BranchingPrModalContent.tsx index 03310f07bda87..34b9ab0ceb840 100644 --- a/addons/isl/src/codeReview/github/BranchingPrModalContent.tsx +++ b/addons/isl/src/codeReview/github/BranchingPrModalContent.tsx @@ -14,7 +14,7 @@ import {Commit} from '../../Commit'; import {Column, FlexSpacer, Row} from '../../ComponentUtils'; import {T} from '../../i18n'; import {PushOperation} from '../../operations/PushOperation'; -import {CommitPreview} from '../../previews'; +import {CommitPreview, dagWithPreviews} from '../../previews'; import {latestSuccessorUnlessExplicitlyObsolete} from '../../successionUtils'; import * as stylex from '@stylexjs/stylex'; import {Badge} from 'isl-components/Badge'; @@ -22,6 +22,7 @@ import {Button} from 'isl-components/Button'; import {Checkbox} from 'isl-components/Checkbox'; import {Dropdown} from 'isl-components/Dropdown'; import {HorizontallyGrowingTextField} from 'isl-components/HorizontallyGrowingTextField'; +import {useAtomValue} from 'jotai'; import {useState} from 'react'; const styles = stylex.create({ @@ -51,16 +52,25 @@ function recommendNewBranchName(stack: Array) { } export default function BranchingPrModalContent({ - stack, + topOfStack, provider, returnResultAndDismiss, }: { - stack: Array; + topOfStack: CommitInfo; provider: GithubUICodeReviewProvider; returnResultAndDismiss: (result: Array | undefined) => unknown; }) { const [createPr, setCreatePr] = useState(false); + const dag = useAtomValue(dagWithPreviews); + // If passed the optimistic isDot commit, we may need to resolve it to a real commit + // once the optimistic commit is no longer in the dag. + const top = + topOfStack.isDot && topOfStack.optimisticRevset != null + ? dag.resolve('.') ?? topOfStack + : topOfStack; + const stack = dag.getBatch(dag.ancestors(top.hash, {within: dag.draft()}).toArray()); + const pushChoices = getPushChoices(provider); const [pushChoice, setPushChoice] = useState(pushChoices[0]);