Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a store for cloneable repositories #6278

Merged
merged 50 commits into from
Dec 10, 2018
Merged
Show file tree
Hide file tree
Changes from 48 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
acfba79
Add an initial outline for a per-account repository store
niik Nov 12, 2018
2cdbea9
:art: Cleanup
niik Nov 12, 2018
d6c280d
Add the API repo store as an app store dependency
niik Nov 12, 2018
5ef2ef8
Avoid loading repositories when another load is in progress
niik Nov 13, 2018
f53ee49
:art: cleanup
niik Nov 13, 2018
9b86e5e
:art: cleanup
niik Nov 13, 2018
c17df5b
First hacky attempt at wiring it all up
niik Nov 13, 2018
a4eca38
Merge branch 'pr/5931' into api-accounts-store
niik Nov 13, 2018
ace1452
Signals are cool tho
niik Nov 13, 2018
0a0307a
This is a temporary hack
niik Nov 13, 2018
4e1e32e
Merge branch 'master' into api-accounts-store
niik Nov 15, 2018
a416f27
Don't need this anymore
niik Nov 15, 2018
d370f79
Typescript 2.9 supports generic components in JSX
niik Nov 15, 2018
f6b5ba4
Need to initialize the first state
niik Nov 22, 2018
50dfdd7
Fix refresh of repositories
niik Nov 22, 2018
af09780
Maintain separate state for each tab
niik Nov 22, 2018
c835e34
Persist filter text in parent component
niik Nov 22, 2018
32718fd
Parent component already takes care of updating the path
niik Nov 22, 2018
c6958eb
Store selected repository in parent component
niik Nov 22, 2018
6ed2052
Make list scroll to selected item on mount
niik Nov 22, 2018
4225f85
This can be pure now, yeay!
niik Nov 22, 2018
2d9990d
:book:
niik Nov 22, 2018
fb7f91a
Merge branch 'master' into api-accounts-store
niik Nov 22, 2018
bbbbb71
Don't regroup repositories when selected item changes
niik Nov 23, 2018
d3eff1a
Document and clean up apiRepositoriesStore
niik Nov 23, 2018
db39f8f
Support refreshing the repositories list
niik Nov 23, 2018
4d61275
Update the url when selection changes
niik Nov 23, 2018
f5ae2e1
Typo
niik Nov 23, 2018
f6d4205
Trigger a refresh on update, if necessary
niik Nov 23, 2018
7b86695
:fire: obsolete state
niik Nov 23, 2018
b24ad49
Whooops!
niik Nov 23, 2018
ba73a22
:art: cleanup
niik Nov 23, 2018
95ab2db
Merge branch 'master' into api-accounts-store
niik Nov 29, 2018
32a9025
Bind 'this' in constructor
niik Nov 29, 2018
2cd5426
Fix clumsy naming of store
niik Nov 29, 2018
5002118
:book:
niik Nov 29, 2018
2c0ea64
:book:
niik Nov 29, 2018
d095b41
Merge branch 'master' into api-accounts-store
niik Nov 29, 2018
e506aac
:book: props
niik Nov 29, 2018
f38148a
:book: state
niik Nov 29, 2018
3849855
:art: a little breathing room
niik Nov 29, 2018
6bf46f4
:book: state update methods
niik Nov 29, 2018
8d90e4b
:book:
niik Nov 29, 2018
9919fd0
Merge branch 'master' into api-accounts-store
niik Nov 30, 2018
2a30914
Lint!
niik Nov 30, 2018
a56d939
Fix issue where account is updated during repository refresh
niik Dec 3, 2018
e6cfe16
Expand documentation
niik Dec 3, 2018
e737b39
Even moar :book:
niik Dec 3, 2018
05ad0b9
Merge branch 'master' into api-accounts-store
niik Dec 4, 2018
456075d
Merge branch 'master' into api-accounts-store
niik Dec 10, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"fs-extra": "^6.0.0",
"fuzzaldrin-plus": "^0.6.0",
"keytar": "^4.0.4",
"memoize-one": "^4.0.3",
"moment": "^2.17.1",
"mri": "^1.1.0",
"primer-support": "^4.0.0",
Expand Down
17 changes: 17 additions & 0 deletions app/src/lib/app-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import { Shell } from './shells'
import { ComparisonCache } from './comparison-cache'

import { ApplicationTheme } from '../ui/lib/application-theme'
import { IAccountRepositories } from './stores/api-repositories-store'

export enum SelectionType {
Repository,
Expand Down Expand Up @@ -191,6 +192,22 @@ export interface IAppState {

/** The currently selected appearance (aka theme) */
readonly selectedTheme: ApplicationTheme

/**
* A map keyed on a user account (GitHub.com or GitHub Enterprise)
* containing an object with repositories that the authenticated
* user has explicit permission (:read, :write, or :admin) to access
* as well as information about whether the list of repositories
* is currently being loaded or not.
*
* If a currently signed in account is missing from the map that
* means that the list of accessible repositories has not yet been
* loaded. An entry for an account with an empty list of repositories
* means that no accessible repositories was found for the account.
*
* See the ApiRepositoriesStore for more details on loading repositories
*/
readonly apiRepositories: ReadonlyMap<Account, IAccountRepositories>
}

export enum FoldoutType {
Expand Down
9 changes: 9 additions & 0 deletions app/src/lib/dispatcher/dispatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1147,6 +1147,15 @@ export class Dispatcher {
return this.appStore._changeCloneRepositoriesTab(tab)
}

/**
* Request a refresh of the list of repositories that
* the provided account has explicit permissions to access.
* See ApiRepositoriesStore for more details.
*/
public refreshApiRepositories(account: Account) {
return this.appStore._refreshApiRepositories(account)
}

/** Open the merge tool for the given file. */
public openMergeTool(repository: Repository, path: string): Promise<void> {
return this.appStore._openMergeTool(repository, path)
Expand Down
196 changes: 196 additions & 0 deletions app/src/lib/stores/api-repositories-store.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
import { BaseStore } from './base-store'
import { AccountsStore } from './accounts-store'
import { IAPIRepository, API } from '../api'
import { Account } from '../../models/account'
import { merge } from '../merge'

/**
* Returns a value indicating whether two account instances
* can be considered equal. Equality is determined by comparing
* the two instances' endpoints and user id. This allows
* us to keep receiving updated Account details from the API
* while still maintaining the association between repositories
* and a particular account.
*/
function accountEquals(x: Account, y: Account) {
return x.endpoint === y.endpoint && x.id === y.id
}

/**
* Attempt to look up an existing account in the account state
* map based on endpoint and user id equality (see accountEquals).
*
* The purpose of this method is to ensure that we're using the
* most recent Account instance during our asynchronous refresh
* operations. While we're refreshing the list of repositories
* that a user has explicit permissions to access it's possible
* that the accounts store will emit updated account instances
* (for example updating the user real name, or the list of
* email addresses associated with an account) and in order to
* guarantee reference equality with the accounts emitted by
* the accounts store we need to ensure we're in sync.
*
* If no match is found the provided account is returned.
*/
function resolveAccount(
account: Account,
accountState: ReadonlyMap<Account, IAccountRepositories>
) {
// The set uses reference equality so if we find our
// account instance in the set there's no need to look
// any further.
if (accountState.has(account)) {
return account
}

// If we can't find our account instance in the set one
// of two things have happened. Either the account has
// been removed (by the user explicitly signing out) or
// the accounts store has refreshed the account details
// from the API and as such the reference equality no
// longer holds. In the latter case we attempt to
// find the updated account instance by comparing its
// user id and endpoint to the provided account.
for (const existingAccount of accountState.keys()) {
if (accountEquals(existingAccount, account)) {
iAmWillShepherd marked this conversation as resolved.
Show resolved Hide resolved
return existingAccount
}
}

// If we can't find a matching account it's likely
// that it's the first time we're loading the list
// of repositories for this account so we return
// whatever was provided to us such that it may be
// inserted into the set as a new entry.
return account
}

/**
* An interface describing the current state of
* repositories that a particular account has explicit
* permissions to access and whether or not the list of
* repositores is being loaded or refreshed.
*
* This main purpose of this interface is to describe
* the state necessary to render a list of cloneable
* repositories.
*/
export interface IAccountRepositories {
/**
* The list of repositories that a particular account
* has explicit permissions to access.
*/
readonly repositories: ReadonlyArray<IAPIRepository>

/**
* Whether or not the list of repositories is currently
* being loaded for the first time or refreshed.
*/
readonly loading: boolean
}

/**
* A store responsible for providing lists of repositories
* that the currently signed in user(s) have explicit access
* to. It's primary purpose is to serve state required for
* the application to present a list of cloneable repositories
* for a particular user.
*/
export class ApiRepositoriesStore extends BaseStore {
/**
* The main internal state of the store. Note that
* all state in this store should be treated as immutable such
* that consumers can use reference equality to determine whether
* state has actually changed or not.
*/
private accountState: ReadonlyMap<Account, IAccountRepositories> = new Map<
Account,
IAccountRepositories
>()

public constructor(private readonly accountsStore: AccountsStore) {
super()
accountsStore.onDidUpdate(() => this.onAccountsChanged())
}

/**
* Called whenever the accounts store emits an update which
* usually means that a new account was added or an account
* was removed due to sign out but it could also mean that
* the account data has been updated. It's crucial that
* the ApiRepositories store match (through reference
* equality) the accounts in the accounts store and this
* method therefore attempts to merge its internal state
* with the new accounts.
*/
private async onAccountsChanged() {
const accounts = await this.accountsStore.getAll()
const newState = new Map<Account, IAccountRepositories>()

for (const account of accounts) {
for (const [key, value] of this.accountState.entries()) {
// Check to see whether the accounts store only emitted an
// updated Account for the same login and endpoint meaning
// that we don't need to discard our cached data.
if (accountEquals(key, account)) {
newState.set(account, value)
break
}
}
}

this.accountState = newState
this.emitUpdate()
}

private updateAccount<T, K extends keyof IAccountRepositories>(
account: Account,
repositories: Pick<IAccountRepositories, K>
) {
const newState = new Map<Account, IAccountRepositories>(this.accountState)

// The account instance might have changed between the refresh and
// the update so we'll need to look it up by endpoint and user id.
// If we can't find it we're likely being asked to insert info for
// an account for the first time.
const newOrExistingAccount = resolveAccount(account, newState)
const existingRepositories = newState.get(newOrExistingAccount)

const newRepositories =
existingRepositories === undefined
? merge({ loading: false, repositories: [] }, repositories)
: merge(existingRepositories, repositories)

newState.set(newOrExistingAccount, newRepositories)

this.accountState = newState
this.emitUpdate()
}

/**
* Request that the store loads the list of repositories that
* the provided account has explicit permissions to access.
*/
public async loadRepositories(account: Account) {
const existingRepositories = this.accountState.get(account)

if (existingRepositories !== undefined && existingRepositories.loading) {
return
}

this.updateAccount(account, { loading: true })

const api = API.fromAccount(account)
const repositories = await api.fetchRepositories()

if (repositories === null) {
this.updateAccount(account, { loading: false })
} else {
this.updateAccount(account, { loading: false, repositories })
}
}

public getState(): ReadonlyMap<Account, IAccountRepositories> {
return this.accountState
}
}
17 changes: 16 additions & 1 deletion app/src/lib/stores/app-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ import { GitStoreCache } from './git-store-cache'
import { MergeConflictsErrorContext } from '../git-error-context'
import { setNumber, setBoolean, getBoolean, getNumber } from '../local-storage'
import { ExternalEditorError } from '../editors/shared'
import { ApiRepositoriesStore } from './api-repositories-store'
import {
updateChangedFiles,
updateConflictState,
Expand Down Expand Up @@ -302,7 +303,8 @@ export class AppStore extends TypedBaseStore<IAppState> {
private readonly accountsStore: AccountsStore,
private readonly repositoriesStore: RepositoriesStore,
private readonly pullRequestStore: PullRequestStore,
private readonly repositoryStateCache: RepositoryStateCache
private readonly repositoryStateCache: RepositoryStateCache,
private readonly apiRepositoriesStore: ApiRepositoriesStore
) {
super()

Expand Down Expand Up @@ -386,6 +388,9 @@ export class AppStore extends TypedBaseStore<IAppState> {
this.pullRequestStore.onDidUpdate(gitHubRepository =>
this.onPullRequestStoreUpdated(gitHubRepository)
)

this.apiRepositoriesStore.onDidUpdate(() => this.emitUpdate())
this.apiRepositoriesStore.onDidError(error => this.emitError(error))
}

/** Load the emoji from disk. */
Expand Down Expand Up @@ -508,6 +513,7 @@ export class AppStore extends TypedBaseStore<IAppState> {
selectedCloneRepositoryTab: this.selectedCloneRepositoryTab,
selectedBranchesTab: this.selectedBranchesTab,
selectedTheme: this.selectedTheme,
apiRepositories: this.apiRepositoriesStore.getState(),
}
}

Expand Down Expand Up @@ -3607,6 +3613,15 @@ export class AppStore extends TypedBaseStore<IAppState> {
return Promise.resolve()
}

/**
* Request a refresh of the list of repositories that
* the provided account has explicit permissions to access.
* See ApiRepositoriesStore for more details.
*/
public _refreshApiRepositories(account: Account) {
return this.apiRepositoriesStore.loadRepositories(account)
}

public _openMergeTool(repository: Repository, path: string): Promise<void> {
const gitStore = this.gitStoreCache.get(repository)
return gitStore.openMergeTool(path)
Expand Down
6 changes: 6 additions & 0 deletions app/src/ui/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1164,6 +1164,8 @@ export class App extends React.Component<IAppProps, IAppState> {
dispatcher={this.props.dispatcher}
selectedTab={this.state.selectedCloneRepositoryTab}
onTabSelected={this.onCloneRepositoriesTabSelected}
apiRepositories={this.state.apiRepositories}
onRefreshRepositories={this.onRefreshRepositories}
/>
)
case PopupType.CreateBranch: {
Expand Down Expand Up @@ -1452,6 +1454,10 @@ export class App extends React.Component<IAppProps, IAppState> {
this.props.dispatcher.changeCloneRepositoriesTab(tab)
}

private onRefreshRepositories = (account: Account) => {
this.props.dispatcher.refreshApiRepositories(account)
}

private onShowAdvancedPreferences = () => {
this.props.dispatcher.showPopup({
type: PopupType.Preferences,
Expand Down
10 changes: 1 addition & 9 deletions app/src/ui/clone-repository/clone-generic-repository.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,20 +54,12 @@ export class CloneGenericRepository extends React.Component<
placeholder="repository path"
onValueChanged={this.props.onPathChanged}
/>
<Button onClick={this.onChooseDirectory}>Choose…</Button>
<Button onClick={this.props.onChooseDirectory}>Choose…</Button>
</Row>
</DialogContent>
)
}

private onChooseDirectory = async () => {
const path = await this.props.onChooseDirectory()

if (path) {
this.props.onPathChanged(path)
}
}

private onUrlChanged = (url: string) => {
this.props.onUrlChanged(url)
}
Expand Down
Loading