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

Checkout Pull Request #1563

Merged
merged 69 commits into from Jul 16, 2018

Conversation

Projects
None yet
8 participants
@smashwilson
Copy link
Member

smashwilson commented Jul 5, 2018

Build on the foundation established in #1556 to add pull request checkout functionality to the IssueishPaneItem.

Screenshots
Before After
screen shot 2018-07-05 at 9 19 13 am screen shot 2018-07-06 at 2 50 10 pm

And a .gif for good measure:

username-autocomplete

Features
  • Navigate to a pull request pane from the pull request list and click the "checkout" button to fetch the pull request's head ref into a local branch.
  • The local branch is configured to track the head ref, so git fetch, git pull, and the fetch and pull buttons on Atom's status bar all work as expected to pull newer work pushed to the pull request.
  • If an existing remote is pointing to the pull request's head repository, it is used instead of a new one.
  • If an existing branch is already tracking the pull request, it is checked out and any new changes are pulled.
  • When the current branch already corresponds to the pull request, the "checkout" button is labelled "checked out" and is disabled.
  • The "checkout" button is also disabled if the active repository is in the middle of a merge or a rebase.
  • If you open an issue by pasting its URL into the dialog shown by the GitHub: Open Issue Or Pull Request command from the command palette, the button is hidden entirely on the pane item.
    • This is currently the only way to show an issue in the "issueish pane," but ultimately you'll be able to get there from a customized accordion list search.
  • If you open a pull request with the GitHub: Open Issue Or Pull Request command with a base repository corresponding to a remote in exactly one project in your workspace, the pull request will be checked out in that project's git repository. (If the pull request corresponds to remotes in none or more than one project, the button is disabled.)
    • As an example: open Atom, add project folders for directories containing clones of atom/atom and atom/etch, then run the GitHub: Open Issue Or Pull Request command and paste the URL https://github.com/atom/etch/pull/67. The pull request will be checked out in the atom/etch repository.

Remaining Work
  • Associate a working directory with each IssueishPaneItem by its URI. Use a WorkdirContextPool to identify the corresponding Repository.
  • Observe the Repository model in the IssueishDetailContainer and extract repository data for the IssueishDetailController.
  • Pass a WorkdirContextPool to each IssueishPaneItem from the RootController that creates it.
  • Introduce a MaybeOperation model to capture the common pattern of an action handler that (a) may be either enabled or disabled for various reasons and (b) toggles component state to true while performing some asynchronous work.
  • Return an indexed RemoteSet from repository.getRemotes(), like we do with the BranchSet returned by getBranches().
    • Update other callers of getRemotes() to use the RemoteSet.
  • Create a MaybeOperation for the checkout action. Populate its enablement based on the relationship of the current issueish and the state of the repository.
  • Write the handler method for the checkout action.
    • "Happy path:" fetch and checkout the PR's branch to a new branch in the local repository.
    • Handle the case where the PR already corresponds to a branch in the local repository.
  • Render a button in the IssueishDetailView that triggers the checkout action. Disable it and modify its text based on the operation state.
  • Track changes to the current issueish by switching the active repository.
  • Get tests green
  • Checkout button text changes
  • Write an integration test
  • Polish up any missing test coverage
From feedback
  • Add the head repository owner name to the generated PR branch
  • Report checkout button click metrics
  • Unbreak the GitHub: Open Issue Or Pull Request command
  • Improve failure notifications
    • ... when the remote already exists
    • ... when the branch already exists
  • Exception when hovering over @-mention links on the Issueish pane
Reviews

Before we merge, I'd like:

  • A review from @simurai once he's happy with the design and UX flow.
  • A review from @sguthals for Q&A

smashwilson added some commits Jul 5, 2018

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 5, 2018

Coverage Status

Coverage increased (+1.7%) to 79.515% when pulling 6fe9069 on aw/actually-checkout-pr into c9056b5 on master.

smashwilson added some commits Jul 5, 2018

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jul 10, 2018

Metrics are in - I'm incrementing a checkout-pr counter each time that a PR is successfully checked out.

smashwilson added some commits Jul 11, 2018

Match empty-valued query parameters
Also 🚀 test coverage 🚀 for uri-pattern
@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jul 11, 2018

I've added somewhat nicer error messages for anticipated git operation failures.

I wanted to make the errors a bit more specialized for the "Checkout PR" action, but at the moment our git operation error handling is centralized in a pipeline that's shared from everywhere that a Repository method is called. If I mentioned PR checkout specifically in the errors I added, we'd put ourselves in an awkward place if we call those git methods from anywhere else in the codebase.

Here's the verbiage I've settled on:

⭐️ If you already have a remote named after the head repository owner that didn't point to the head repository on GitHub:

screen shot 2018-07-11 at 10 43 28 am

⭐️ If your existing local branch has diverged from the fetched head ref

screen shot 2018-07-11 at 10 43 59 am

@sguthals

This comment has been minimized.

Copy link
Contributor

sguthals commented Jul 12, 2018

👋 Ok some thoughts.

First of all - really impressive work @simurai and @smashwilson!!! I'm really excited about this feature!

Happy to jump on a call to discuss any and all of these.

--

screen shot 2018-07-12 at 2 25 23 pm

When we have multiple authors on a commit, this is how the UI looks. What if there are 10 contributors? Also, everything is right aligned, but then at the bottom in the separated section that avatar and text isn't also scooted over. I am OK with this design, but I want to make sure it was deliberate and not coincidental. Maybe more of a question for @simurai

--

I clicked Checkout on this PR: atom/atom#17140 and received this error:
screen shot 2018-07-12 at 2 43 23 pm
I'm confused by the error message, as in, what action should I take to properly checkout this PR then? If it already exists, shouldn't I just switch to it then? Why wouldn't our functionality switch to it if it already existed?

Actually, this might be a bug? Because now I'm seeing that when I switch to branch pr-17681 the UI automatically updates with that being my current PR and "Checked Out", but when I switch to branch pr-17140 that doesn't happen. Why would that be? What would cause me to get into that state?

This is the state in the GitHub tab when I'm on that same branch:
screen shot 2018-07-12 at 3 00 31 pm

Ah - I think I see, I think it's when I open a PR that is from a forked repo, because the same thing happens on this PR:
atom/atom#17628

The messaging issue might be the same for the one you put above:
screen shot 2018-07-12 at 2 43 23 pm

I think we might need to suggest actions for folks to follow to get to the state they want to get to, like you did here:
screen shot 2018-07-12 at 2 43 23 pm

--

Do we want reactions to look like this:
screen shot 2018-07-12 at 2 43 02 pm
I can see how that matches the rest of the aesthetic, but for some reason looks really flat to me? It's also a little strange because I can't click on them, yet they look kind of like buttons? I like the idea of having them, but would it better to wait until we have support for them too?

--

Everything here at the top:
screen shot 2018-07-12 at 2 48 37 pm
Is clickable except for the green open (which makes sense) and the title. The title, for some reason, threw me. I expected to be able to click on it and have it take me to the PR on dotcom. I know that the file path there at the top takes me there, but maybe because I'm used to clicking on titles and I'm used to things in white being links? Similar to how the thing that is a link here is in white and everything else is in grey?
screen shot 2018-07-12 at 2 50 10 pm

--

Should we add a line here between the last PR and more:
screen shot 2018-07-12 at 2 50 41 pm
Otherwise it looks like the more might be for that single PR?

--

I tried opening this issue: atom/atom#17683 with the `GitHub: Open Issue or Pull Request" and I got a blank file?
screen shot 2018-07-12 at 2 57 22 pm

--

I tried opening this PR: atom/atom#17674 with the `GitHub: Open Issue or Pull Request and I got blank file also:
screen shot 2018-07-12 at 2 58 02 pm

--

When I looked at this PR:
atom/atom#17141
And I hovered over the @Rezayovan I got this error:

/Users/sarahguthals/Documents/GitHub/github/lib/views/issueish-timeline-view.js:132 unrecognized timeline event type: RenamedTitleEvent
_react2.default.createElement.groupedEdges.map @ /Users/sarahguthals/Documents/GitHub/github/lib/views/issueish-timeline-view.js:132
/Users/sarahguthals/Documents/GitHub/github/lib/views/issueish-timeline-view.js:132 unrecognized timeline event type: LabeledEvent
_react2.default.createElement.groupedEdges.map @ /Users/sarahguthals/Documents/GitHub/github/lib/views/issueish-timeline-view.js:132
/Users/sarahguthals/Documents/GitHub/github/node_modules/react-relay/lib/ReactRelayQueryRenderer.js:217 Uncaught TypeError: Cannot read property 'unstable_internal' of undefined
    at fetchQueryAndComputeStateFromProps (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-relay/lib/ReactRelayQueryRenderer.js:217:51)
    at new ReactRelayQueryRenderer (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-relay/lib/ReactRelayQueryRenderer.js:77:8)
    at constructClassInstance (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:11447:18)
    at updateClassComponent (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:13144:7)
    at beginWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:13824:14)
    at performUnitOfWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:15863:12)
    at workLoop (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:15902:24)
    at HTMLUnknownElement.callCallback (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:100:14)
    at Object.invokeGuardedCallbackDev (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:138:16)
    at invokeGuardedCallback (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:187:29)
    at replayUnitOfWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:15310:5)
    at renderRoot (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:15962:11)
    at performWorkOnRoot (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16560:22)
    at performWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16482:7)
    at performSyncWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16454:3)
    at requestWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16354:5)
    at scheduleWork$1 (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16218:11)
    at scheduleRootUpdate (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16785:3)
    at updateContainerAtExpirationTime (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16812:10)
    at updateContainer (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16839:10)
    at ReactRoot.render (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:17122:3)
    at /Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:17262:14
    at unbatchedUpdates (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16679:10)
    at legacyRenderSubtreeIntoContainer (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:17258:5)
    at Object.render (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:17317:12)
    at UserMentionTooltipItem.get element [as element] (/Users/sarahguthals/Documents/GitHub/github/lib/items/user-mention-tooltip-item.js:48:34)
    at UserMentionTooltipItem.getElement (/Users/sarahguthals/Documents/GitHub/github/lib/items/user-mention-tooltip-item.js:14:12)
    at ViewRegistry.module.exports.ViewRegistry.createView (/private/var/folders/tk/mrlhdh_9507_4_nz58bz3mjr0000gn/T/AppTranslocation/8…A022B9C27DC/d/Atom.app/Contents/Resources/app.asar/src/view-registry.js:87:26)
    at ViewRegistry.module.exports.ViewRegistry.getView (/private/var/folders/tk/mrlhdh_9507_4_nz58bz3mjr0000gn/T/AppTranslocation/8…A022B9C27DC/d/Atom.app/Contents/Resources/app.asar/src/view-registry.js:75:21)
    at Tooltip.setContent (/private/var/folders/tk/mrlhdh_9507_4_nz58bz3mjr0000gn/T/AppTranslocation/8…309-BA022B9C27DC/d/Atom.app/Contents/Resources/app.asar/src/tooltip.js:342:41)
/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:14226 The above error occurred in the <ReactRelayQueryRenderer> component:
    in ReactRelayQueryRenderer

Consider adding an error boundary to your tree to customize error handling behavior.
Visit https://fb.me/react-error-boundaries to learn more about error boundaries.
logCapturedError @ /Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:14226
/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16543 Uncaught TypeError: Cannot read property 'unstable_internal' of undefined
    at fetchQueryAndComputeStateFromProps (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-relay/lib/ReactRelayQueryRenderer.js:217:51)
    at new ReactRelayQueryRenderer (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-relay/lib/ReactRelayQueryRenderer.js:77:8)
    at constructClassInstance (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:11447:18)
    at updateClassComponent (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:13144:7)
    at beginWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:13824:14)
    at performUnitOfWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:15863:12)
    at workLoop (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:15902:24)
    at renderRoot (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:15942:7)
    at performWorkOnRoot (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16560:22)
    at performWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16482:7)
    at performSyncWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16454:3)
    at requestWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16354:5)
    at scheduleWork$1 (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16218:11)
    at scheduleRootUpdate (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16785:3)
    at updateContainerAtExpirationTime (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16812:10)
    at updateContainer (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16839:10)
    at ReactRoot.render (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:17122:3)
    at /Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:17262:14
    at unbatchedUpdates (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16679:10)
    at legacyRenderSubtreeIntoContainer (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:17258:5)
    at Object.render (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:17317:12)
    at UserMentionTooltipItem.get element [as element] (/Users/sarahguthals/Documents/GitHub/github/lib/items/user-mention-tooltip-item.js:48:34)
    at UserMentionTooltipItem.getElement (/Users/sarahguthals/Documents/GitHub/github/lib/items/user-mention-tooltip-item.js:14:12)
    at ViewRegistry.module.exports.ViewRegistry.createView (/private/var/folders/tk/mrlhdh_9507_4_nz58bz3mjr0000gn/T/AppTranslocation/8…A022B9C27DC/d/Atom.app/Contents/Resources/app.asar/src/view-registry.js:87:26)
    at ViewRegistry.module.exports.ViewRegistry.getView (/private/var/folders/tk/mrlhdh_9507_4_nz58bz3mjr0000gn/T/AppTranslocation/8…A022B9C27DC/d/Atom.app/Contents/Resources/app.asar/src/view-registry.js:75:21)
    at Tooltip.setContent (/private/var/folders/tk/mrlhdh_9507_4_nz58bz3mjr0000gn/T/AppTranslocation/8…309-BA022B9C27DC/d/Atom.app/Contents/Resources/app.asar/src/tooltip.js:342:41)
    at Tooltip.show (/private/var/folders/tk/mrlhdh_9507_4_nz58bz3mjr0000gn/T/AppTranslocation/8…309-BA022B9C27DC/d/Atom.app/Contents/Resources/app.asar/src/tooltip.js:227:10)
    at Tooltip.enter (/private/var/folders/tk/mrlhdh_9507_4_nz58bz3mjr0000gn/T/AppTranslocation/8…309-BA022B9C27DC/d/Atom.app/Contents/Resources/app.asar/src/tooltip.js:178:17)
    at HTMLAnchorElement.innerHandler (/private/var/folders/tk/mrlhdh_9507_4_nz58bz3mjr0000gn/T/AppTranslocation/8…9C27DC/d/Atom.app/Contents/Resources/app.asar/src/delegated-listener.js:24:7)

Ok - looks like I'm getting that error every time I hover over a link that is in the description or comments of the PR. The links still work, however.

--

Would knowing what branch the PR is targeting be something we want to include here?

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jul 12, 2018

@sguthals: wow, thanks for the thoroughness 😁

This PR only touched the Issueish pane to add the checkout button, but like @simurai said, he's been contemplating a comprehensive redesign of the rest of the pane soon. Hopefully he can take your feedback into account as he's tackling that 👍

I'm confused by the error message, as in, what action should I take to properly checkout this PR then? If it already exists, shouldn't I just switch to it then? Why wouldn't our functionality switch to it if it already existed?

Actually, this might be a bug? Because now I'm seeing that when I switch to branch pr-17681 the UI automatically updates with that being my current PR and "Checked Out", but when I switch to branch pr-17140 that doesn't happen. Why would that be? What would cause me to get into that state?

I'm confused, too - it should have detected that pr-17681 corresponded to the PR by its configured upstream. Can you run the following command in that repository and share its output:

git for-each-ref --format '%(upstream) - %(upstream:remotename) - %(upstream:remoteref) | %(push) - %(push:remotename) - %(push:remoteref)' refs/heads/pr-17140/Rezayovan/master

This could be something like a mismatch in the way the remote is set up or incorrectly cached data. Does it detect properly if you reload the Atom window?

I think we might need to suggest actions for folks to follow to get to the state they want to get to, like you did here:

The problem here is that I can't currently customize the error messaging based on the originator of the git checkout call. So If I changed it to be more specific about the checkout PR action, it would show that for any time you tried to check out a branch and got that error.

I have an idea about ways to improve our error handling on a per-callsite basis, but it's a bit of a sprawling change and would benefit other places too, so I was hoping to defer that to a separate PR.

I tried opening this issue: atom/atom#17683 with the `GitHub: Open Issue or Pull Request" and I got a blank file?

Are you sure you're on the latest version of this branch? I thought I fixed that earlier this week. Can you verify the commit of atom/github you're running?

Should we add a line here between the last PR and more: [...]

That sounds good to me, but it'd be out of scope for this PR. @simurai, any thoughts?

Ok - looks like I'm getting that error every time I hover over a link that is in the description or comments of the PR. The links still work, however.

Oh weird. What kind of links - to another issue/pull request, an @-mention, or any hyperlink at all? I'll add a note to investigate this one.

@sguthals

This comment has been minimized.

Copy link
Contributor

sguthals commented Jul 12, 2018

@smashwilson -
Ran the command:

Sarahs-iMac:atom sarahguthals$ git for-each-ref --format '%(upstream) - %(upstream:remotename) - %(upstream:remoteref) | %(push) - %(push:remotename) - %(push:remoteref)' refs/heads/pr-17140/Rezayovan/master
 -  -  |  - https://github.com/Rezayovan/atom.git - 
Sarahs-iMac:atom sarahguthals$ 

Reloading did not help.

--

The problem here is that I can't currently customize the error messaging based on the originator of the git checkout call. So If I changed it to be more specific about the checkout PR action, it would show that for any time you tried to check out a branch and got that error.
I have an idea about ways to improve our error handling on a per-callsite basis, but it's a bit of a sprawling change and would benefit other places too, so I was hoping to defer that to a separate PR.

Got it! No problem 👍

--

Are you sure you're on the latest version of this branch? I thought I fixed that earlier this week. Can you verify the commit of atom/github you're running?

Ughhh I'm really sorry - you're right, I hadn't fetched. That is in fact working.

--

Oh weird. What kind of links - to another issue/pull request, an @-mention, or any hyperlink at all? I'll add a note to investigate this one.

So it only pops up the console when I hover on an @ mention, but then when the console is open, I do see the error on any link

--

Out of Scope for this PR, but to keep it all in one place for right this second, I also noticed that there is no messaging around the fact that I have un-staged/ un-committed changes on a branch in the side panel:
screen shot 2018-07-12 at 4 50 51 pm

@sguthals

This comment has been minimized.

Copy link
Contributor

sguthals commented Jul 12, 2018

From your comment in Slack, you're right, after I pulled I now get this error still:
screen shot 2018-07-12 at 4 53 22 pm

However, below the error I see:
screen shot 2018-07-12 at 4 53 27 pm

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jul 13, 2018

From your comment in Slack, you're right, after I pulled I now get this error still: [...]

Ah, right, you'd have to delete the branch and check out the PR again to get it configured properly. (Or you could try it with another PR from a fork?)

@sguthals
Copy link
Contributor

sguthals left a comment

When I looked at this PR:
atom/atom#17141
And I hovered over the @Rezayovan I got this error:

/Users/sarahguthals/Documents/GitHub/github/lib/views/issueish-timeline-view.js:132 unrecognized timeline event type: RenamedTitleEvent
_react2.default.createElement.groupedEdges.map @ /Users/sarahguthals/Documents/GitHub/github/lib/views/issueish-timeline-view.js:132
/Users/sarahguthals/Documents/GitHub/github/lib/views/issueish-timeline-view.js:132 unrecognized timeline event type: LabeledEvent
_react2.default.createElement.groupedEdges.map @ /Users/sarahguthals/Documents/GitHub/github/lib/views/issueish-timeline-view.js:132
/Users/sarahguthals/Documents/GitHub/github/node_modules/react-relay/lib/ReactRelayQueryRenderer.js:217 Uncaught TypeError: Cannot read property 'unstable_internal' of undefined
    at fetchQueryAndComputeStateFromProps (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-relay/lib/ReactRelayQueryRenderer.js:217:51)
    at new ReactRelayQueryRenderer (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-relay/lib/ReactRelayQueryRenderer.js:77:8)
    at constructClassInstance (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:11447:18)
    at updateClassComponent (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:13144:7)
    at beginWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:13824:14)
    at performUnitOfWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:15863:12)
    at workLoop (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:15902:24)
    at HTMLUnknownElement.callCallback (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:100:14)
    at Object.invokeGuardedCallbackDev (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:138:16)
    at invokeGuardedCallback (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:187:29)
    at replayUnitOfWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:15310:5)
    at renderRoot (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:15962:11)
    at performWorkOnRoot (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16560:22)
    at performWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16482:7)
    at performSyncWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16454:3)
    at requestWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16354:5)
    at scheduleWork$1 (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16218:11)
    at scheduleRootUpdate (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16785:3)
    at updateContainerAtExpirationTime (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16812:10)
    at updateContainer (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16839:10)
    at ReactRoot.render (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:17122:3)
    at /Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:17262:14
    at unbatchedUpdates (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16679:10)
    at legacyRenderSubtreeIntoContainer (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:17258:5)
    at Object.render (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:17317:12)
    at UserMentionTooltipItem.get element [as element] (/Users/sarahguthals/Documents/GitHub/github/lib/items/user-mention-tooltip-item.js:48:34)
    at UserMentionTooltipItem.getElement (/Users/sarahguthals/Documents/GitHub/github/lib/items/user-mention-tooltip-item.js:14:12)
    at ViewRegistry.module.exports.ViewRegistry.createView (/private/var/folders/tk/mrlhdh_9507_4_nz58bz3mjr0000gn/T/AppTranslocation/8…A022B9C27DC/d/Atom.app/Contents/Resources/app.asar/src/view-registry.js:87:26)
    at ViewRegistry.module.exports.ViewRegistry.getView (/private/var/folders/tk/mrlhdh_9507_4_nz58bz3mjr0000gn/T/AppTranslocation/8…A022B9C27DC/d/Atom.app/Contents/Resources/app.asar/src/view-registry.js:75:21)
    at Tooltip.setContent (/private/var/folders/tk/mrlhdh_9507_4_nz58bz3mjr0000gn/T/AppTranslocation/8…309-BA022B9C27DC/d/Atom.app/Contents/Resources/app.asar/src/tooltip.js:342:41)
/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:14226 The above error occurred in the <ReactRelayQueryRenderer> component:
    in ReactRelayQueryRenderer

Consider adding an error boundary to your tree to customize error handling behavior.
Visit https://fb.me/react-error-boundaries to learn more about error boundaries.
logCapturedError @ /Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:14226
/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16543 Uncaught TypeError: Cannot read property 'unstable_internal' of undefined
    at fetchQueryAndComputeStateFromProps (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-relay/lib/ReactRelayQueryRenderer.js:217:51)
    at new ReactRelayQueryRenderer (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-relay/lib/ReactRelayQueryRenderer.js:77:8)
    at constructClassInstance (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:11447:18)
    at updateClassComponent (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:13144:7)
    at beginWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:13824:14)
    at performUnitOfWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:15863:12)
    at workLoop (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:15902:24)
    at renderRoot (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:15942:7)
    at performWorkOnRoot (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16560:22)
    at performWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16482:7)
    at performSyncWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16454:3)
    at requestWork (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16354:5)
    at scheduleWork$1 (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16218:11)
    at scheduleRootUpdate (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16785:3)
    at updateContainerAtExpirationTime (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16812:10)
    at updateContainer (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16839:10)
    at ReactRoot.render (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:17122:3)
    at /Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:17262:14
    at unbatchedUpdates (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:16679:10)
    at legacyRenderSubtreeIntoContainer (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:17258:5)
    at Object.render (/Users/sarahguthals/Documents/GitHub/github/node_modules/react-dom/cjs/react-dom.development.js:17317:12)
    at UserMentionTooltipItem.get element [as element] (/Users/sarahguthals/Documents/GitHub/github/lib/items/user-mention-tooltip-item.js:48:34)
    at UserMentionTooltipItem.getElement (/Users/sarahguthals/Documents/GitHub/github/lib/items/user-mention-tooltip-item.js:14:12)
    at ViewRegistry.module.exports.ViewRegistry.createView (/private/var/folders/tk/mrlhdh_9507_4_nz58bz3mjr0000gn/T/AppTranslocation/8…A022B9C27DC/d/Atom.app/Contents/Resources/app.asar/src/view-registry.js:87:26)
    at ViewRegistry.module.exports.ViewRegistry.getView (/private/var/folders/tk/mrlhdh_9507_4_nz58bz3mjr0000gn/T/AppTranslocation/8…A022B9C27DC/d/Atom.app/Contents/Resources/app.asar/src/view-registry.js:75:21)
    at Tooltip.setContent (/private/var/folders/tk/mrlhdh_9507_4_nz58bz3mjr0000gn/T/AppTranslocation/8…309-BA022B9C27DC/d/Atom.app/Contents/Resources/app.asar/src/tooltip.js:342:41)
    at Tooltip.show (/private/var/folders/tk/mrlhdh_9507_4_nz58bz3mjr0000gn/T/AppTranslocation/8…309-BA022B9C27DC/d/Atom.app/Contents/Resources/app.asar/src/tooltip.js:227:10)
    at Tooltip.enter (/private/var/folders/tk/mrlhdh_9507_4_nz58bz3mjr0000gn/T/AppTranslocation/8…309-BA022B9C27DC/d/Atom.app/Contents/Resources/app.asar/src/tooltip.js:178:17)
    at HTMLAnchorElement.innerHandler (/private/var/folders/tk/mrlhdh_9507_4_nz58bz3mjr0000gn/T/AppTranslocation/8…9C27DC/d/Atom.app/Contents/Resources/app.asar/src/delegated-listener.js:24:7)

Ok - looks like I'm getting that error every time I hover over a link that is in the description or comments of the PR. The links still work, however.

@simurai

This comment has been minimized.

Copy link
Member

simurai commented Jul 13, 2018

@sguthals Should we add a line here between the last PR and more: [...]
@smashwilson That sounds good to me, but it'd be out of scope for this PR. @simurai, any thoughts?

Chatted with @sguthals earlier today and yeah, the plan is to make a new PR for all the visual improvements. 👍

@simurai simurai referenced this pull request Jul 13, 2018

Closed

More PR improvements #1582

4 of 5 tasks complete

smashwilson added some commits Jul 16, 2018

@smashwilson

This comment has been minimized.

Copy link
Member Author

smashwilson commented Jul 16, 2018

Hovers and clicks in rendered GitHub markdown work without a stack trace:

screen shot 2018-07-16 at 8 58 18 am

@sguthals: Ready for another pass 🔎

@sguthals
Copy link
Contributor

sguthals left a comment

Looks great! Thank you @smashwilson and @simurai for all of your work on this!

@smashwilson smashwilson merged commit 5167d0c into master Jul 16, 2018

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+1.7%) to 79.515%
Details

@smashwilson smashwilson deleted the aw/actually-checkout-pr branch Jul 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.