Integrate recommended bookmarks graphql with isl-server

Summary:
This diff uses the new Ent based generalized mapping of recommended bookmarks. This has several benefits including reduced latency at startup, as the server now determines the recommended bookmark names instead of the client (instead of back and forth messages). We also benefit from decouple onboarding of new bookmarks/users from ISL releases.
See [this doc](https://docs.google.com/document/d/17PpbY_fIuuNpRIgdeFVYFZGBtbNFJzuLBqbBUpoMlcU/edit?usp=sharing) for reference.

Still, some improvements can be made by sending over recommended bookmarks with the first smartlog commits fetch. However this does have drawbacks of making the initial fetch a bit slower, and can be considered in a future diff.

Reviewed By: evangrayk

Differential Revision: D86217928

fbshipit-source-id: 258cce7fc08917f112d911e069a9220d19d824e7

Author: babyleafy

addons/isl-server/src/Repository.ts

@@ -171,8 +171,9 @@ export class Repository {
   /**
    * Recommended remote bookmarks to be include in batched `log` fetch.
    * If a bookmark is not in the subscriptions list yet, then it will be pulled explicitly.
+   * Undefined means not yet fetched.
    */
-  public recommendedBookmarks: Array<string> = [];
+  public recommendedBookmarks: Array<string> | undefined;
 
   /**
    * The context used when the repository was created.
@@ -953,6 +954,7 @@ export class Repository {
     const fetchStartTimestamp = Date.now();
     try {
       this.smartlogCommitsBeginFetchingEmitter.emit('start');
+
       const visibleCommitDayRange = this.visibleCommitRanges[this.currentVisibleCommitRangeIndex];
 
       const primaryRevset = '(interestingbookmarks() + heads(draft()))';
@@ -969,7 +971,7 @@ export class Repository {
         '.', // always include wdir parent
         // stable locations hashes may be newer than the repo has, wrap in `present()` to only include if available.
         ...this.stableLocations.map(location => `present(${location.hash})`),
-        ...this.recommendedBookmarks.map(bookmark => `present(${bookmark})`),
+        ...(this.recommendedBookmarks ?? []).map(bookmark => `present(${bookmark})`),
         ...(this.fullRepoBranchModule?.genRevset() ?? []),
       ]
         .filter(notEmpty)
@@ -1025,8 +1027,23 @@ export class Repository {
     }
   });
 
-  async pullRecommendedBookmarks(ctx: RepositoryContext) {
-    if (!this.recommendedBookmarks.length) {
+  public async fetchAndSetRecommendedBookmarks(onFetched?: (bookmarks: Array<string>) => void) {
+    if (!Internal.getRecommendedBookmarks) {
+      return;
+    }
+
+    try {
+      const bookmarks = await Internal.getRecommendedBookmarks(this.initialConnectionContext);
+      onFetched?.((this.recommendedBookmarks = bookmarks.map((b: string) => `remote/${b}`)));
+      void this.pullRecommendedBookmarks(this.initialConnectionContext);
+    } catch (err) {
+      this.initialConnectionContext.logger.error('Error fetching recommended bookmarks:', err);
+      void onFetched?.([]);
+    }
+  }
+
+  async pullRecommendedBookmarks(ctx: RepositoryContext): Promise<void> {
+    if (!this.recommendedBookmarks || !this.recommendedBookmarks.length) {
       return;
     }
 
@@ -1042,7 +1059,7 @@ export class Repository {
       );
 
       if (missingBookmarks.length > 0) {
-        // ISL prefixes bookmarks with remote/, so we need to strip this to pull the remote names
+        // We need to strip to pull the remote names
         const missingRemoteNames = missingBookmarks.map(bookmark =>
           bookmark.replace(/^remote\//, ''),
         );

addons/isl-server/src/ServerToClientAPI.ts

@@ -388,7 +388,11 @@ export default class ServerToClientAPI {
             const disposables: Array<Disposable> = [];
             // send changes as they come from file watcher
             disposables.push(repo.subscribeToSmartlogCommitsChanges(postSmartlogCommits));
-            // trigger a fetch on startup
+
+            // trigger fetches on startup
+            repo.fetchAndSetRecommendedBookmarks(bookmarks => {
+              this.postMessage({type: 'fetchedRecommendedBookmarks', bookmarks});
+            });
             repo.fetchSmartlogCommits();
 
             disposables.push(
@@ -604,6 +608,9 @@ export default class ServerToClientAPI {
       }
       case 'refresh': {
         logger?.log('refresh requested');
+        repo.fetchAndSetRecommendedBookmarks(bookmarks => {
+          this.postMessage({type: 'fetchedRecommendedBookmarks', bookmarks});
+        });
         repo.fetchSmartlogCommits();
         repo.fetchUncommittedChanges();
         repo.fetchSubmoduleMap();

addons/isl/src/BookmarksData.tsx

@@ -10,8 +10,7 @@ import type {StableLocationData} from './types';
 import {atom} from 'jotai';
 import {tracker} from './analytics';
 import serverAPI from './ClientToServerAPI';
-import {Internal} from './Internal';
-import {lazyAtom, localStorageBackedAtom, readAtom, writeAtom} from './jotaiUtils';
+import {localStorageBackedAtom, readAtom, writeAtom} from './jotaiUtils';
 import {latestCommits} from './serverAPIState';
 import {registerDisposable} from './utils';
 
@@ -81,6 +80,22 @@ registerDisposable(
 );
 fetchStableLocations(); // fetch on startup
 
+registerDisposable(
+  serverAPI,
+  serverAPI.onMessageOfType('fetchedRecommendedBookmarks', data => {
+    writeAtom(recommendedBookmarksAtom, new Set(data.bookmarks));
+
+    const bookmarksData = readAtom(bookmarksDataStorage);
+    tracker.track('RecommendedBookmarksStatus', {
+      extras: {
+        enabled: bookmarksData.useRecommendedBookmark ?? false,
+        recommendedBookmarks: data.bookmarks,
+      },
+    });
+  }),
+  import.meta.hot,
+);
+
 export function fetchStableLocations() {
   const data = readAtom(bookmarksDataStorage);
   const additionalStables = data.additionalStables ?? [];
@@ -95,15 +110,6 @@ export const remoteBookmarks = atom(get => {
   return commits.flatMap(commit => commit.remoteBookmarks);
 });
 
-function fetchRecommendedBookmarks(recommendedBookmarks: Array<string>) {
-  if (recommendedBookmarks) {
-    serverAPI.postMessage({
-      type: 'fetchRecommendedBookmarks',
-      recommendedBookmarks,
-    });
-  }
-}
-
 /**
  * For determining if reminders to use recommended bookmarks should be shown
  */
@@ -123,26 +129,7 @@ export const recommendedBookmarksOnboarding = localStorageBackedAtom<boolean>(
   true,
 );
 
-export const recommendedBookmarksAtom = lazyAtom(async _get => {
-  const recommendedBookmarks = await (Internal.getRecommendedBookmarks?.() ??
-    Promise.resolve(new Set<string>()));
-
-  // Fetch the recommended bookmarks from server on startup
-  if (recommendedBookmarks.size > 0) {
-    fetchRecommendedBookmarks(Array.from(recommendedBookmarks));
-
-    // Track recommended bookmarks to approximate adoption
-    const bookmarksData = readAtom(bookmarksDataStorage);
-    tracker.track('RecommendedBookmarksStatus', {
-      extras: {
-        enabled: bookmarksData.useRecommendedBookmark ?? false,
-        recommendedBookmarks: Array.from(recommendedBookmarks),
-      },
-    });
-  }
-
-  return recommendedBookmarks;
-}, new Set<string>());
+export const recommendedBookmarksAtom = atom<Set<string>>(new Set<string>());
 
 /** Checks if recommended bookmarks are available in remoteBookmarks */
 export const recommendedBookmarksAvailableAtom = atom(get => {

addons/isl/src/types.ts

@@ -1124,6 +1124,7 @@ export type ServerToClientMessage =
   | {type: 'confirmedLand'; result: Result<undefined>}
   | {type: 'fetchedCommitCloudState'; state: Result<CommitCloudSyncState>}
   | {type: 'fetchedStables'; stables: StableLocationData}
+  | {type: 'fetchedRecommendedBookmarks'; bookmarks: Array<string>}
   | {type: 'fetchedStableLocationAutocompleteOptions'; result: Result<Array<TypeaheadResult>>}
   | {type: 'renderedMarkup'; html: string; id: number}
   | {type: 'gotSuggestedReviewers'; reviewers: Array<string>; key: string}