Merge pull request #15549 from desktop/shift-down-arrow

Fix Shift + Down Arrow not selecting properly in the Commit List

app/src/lib/stores/app-store.ts

@@ -6590,6 +6590,25 @@ export class AppStore extends TypedBaseStore<IAppState> {
     }
   }
 
+  /**
+   * Multi selection on the commit list can give an order of 1, 5, 3 if that is
+   * how the user selected them. However, we want to main chronological ordering
+   * of the commits to reduce the chance of conflicts during interact rebasing.
+   * Thus, assuming 1 is the first commit made by the user and 5 is the last. We
+   * want the order to be, 1, 3, 5.
+   */
+  private orderCommitsByHistory(
+    repository: Repository,
+    commits: ReadonlyArray<CommitOneLine>
+  ) {
+    const { compareState } = this.repositoryStateCache.get(repository)
+    const { commitSHAs } = compareState
+
+    return [...commits].sort(
+      (a, b) => commitSHAs.indexOf(b.sha) - commitSHAs.indexOf(a.sha)
+    )
+  }
+
   /** This shouldn't be called directly. See `Dispatcher`. */
   public async _cherryPick(
     repository: Repository,
@@ -6600,13 +6619,15 @@ export class AppStore extends TypedBaseStore<IAppState> {
       return CherryPickResult.UnableToStart
     }
 
+    const orderedCommits = this.orderCommitsByHistory(repository, commits)
+
     await this._refreshRepository(repository)
 
     const progressCallback =
       this.getMultiCommitOperationProgressCallBack(repository)
     const gitStore = this.gitStoreCache.get(repository)
     const result = await gitStore.performFailableOperation(() =>
-      cherryPick(repository, commits, progressCallback)
+      cherryPick(repository, orderedCommits, progressCallback)
     )
 
     return result || CherryPickResult.Error

app/src/ui/history/commit-list.tsx

@@ -275,15 +275,9 @@ export class CommitList extends React.Component<ICommitListProps, {}> {
   }
 
   private onSelectionChanged = (rows: ReadonlyArray<number>) => {
-    // Multi select can give something like 1, 5, 3 depending on order that user
-    // selects. We want to ensure they are in chronological order for best
-    // cherry-picking results. If user wants to use cherry-picking for
-    // reordering, they will need to do multiple cherry-picks.
-    // Goal: first commit in history -> first on array
-    const sorted = [...rows].sort((a, b) => b - a)
-    const selectedShas = sorted.map(r => this.props.commitSHAs[r])
+    const selectedShas = rows.map(r => this.props.commitSHAs[r])
     const selectedCommits = this.lookupCommits(selectedShas)
-    this.props.onCommitsSelected?.(selectedCommits, this.isContiguous(sorted))
+    this.props.onCommitsSelected?.(selectedCommits, this.isContiguous(rows))
   }
 
   /**
@@ -297,13 +291,15 @@ export class CommitList extends React.Component<ICommitListProps, {}> {
       return true
     }
 
-    for (let i = 0; i < indexes.length; i++) {
-      const current = indexes[i]
-      if (i + 1 === indexes.length) {
+    const sorted = [...indexes].sort((a, b) => b - a)
+
+    for (let i = 0; i < sorted.length; i++) {
+      const current = sorted[i]
+      if (i + 1 === sorted.length) {
         continue
       }
 
-      if (current - 1 !== indexes[i + 1]) {
+      if (current - 1 !== sorted[i + 1]) {
         return false
       }
     }

app/src/ui/lib/list/list.tsx

@@ -74,6 +74,14 @@ interface IListProps {
    * The currently selected rows indexes. Used to attach a special
    * selection class on those row's containers as well as being used
    * for keyboard selection.
+   *
+   * It is expected that the use case for this is setting of the initially
+   * selected rows or clearing a list selection.
+   *
+   * N.B. Since it is used for keyboard selection, changing the ordering of
+   * elements in this array in a parent component may result in unexpected
+   * behaviors when a user modifies their selection via key commands.
+   * See #15536 lessons learned.
    */
   readonly selectedRows: ReadonlyArray<number>