UI: Use changelist ID in URL for browsing changelists#53253
Conversation
74ccd94 to
232cf3f
Compare
sashaostrikov
left a comment
There was a problem hiding this comment.
Man, great work! 🎉🎉🎉
Some cleanups are needed indeed, but nothing critical.
Btw, I haven't found the PetriJS dependency, but it obviously works and prints the console logs in PR comments! :mindblown:
There was a problem hiding this comment.
Do you need to support both undefined and null? TBH, I doubt it, I think one of them should be enough, right?
There was a problem hiding this comment.
Nope, the editor complains with only one. My understanding is because its slightly different queries which lead to slightly different data structures that make use of this component.
There was a problem hiding this comment.
Did you try removing null?
I believe having commit?: GitCommitFields is enough, I took a look at usages and we don't pass null anywhere. It's either an object or undefined.
Can you try it?
There was a problem hiding this comment.
Yep. RepositoryCommitPage complains when I remove null and RepositoryChangelistPage complains when I remove undefined.
There was a problem hiding this comment.
and that's valid :) because commit can be null in RepositoryCommitPage.
What you can do, is ensure that your commit constant is only either an object or undefined, but never null:
const commit = useMemo(
() => (data?.node?.__typename === 'Repository' ? data?.node?.commit || undefined : undefined),
[data]
)
There was a problem hiding this comment.
| onDidUpdateExternalLinks: (externalLinks: ExternalLinkFields[] | undefined) => void | |
| onDidUpdateExternalLinks: (externalLinks?: ExternalLinkFields[]) => void |
There was a problem hiding this comment.
onDidUpdateExternalLinks is called with undefined which makes this necessary.
There was a problem hiding this comment.
yeah, I just changed the code so that ?: is used which is the same thing as | undefined, but more concise
There was a problem hiding this comment.
Ah. I missed the ?: in your suggestion. I will test it locally and apply the fix.
There was a problem hiding this comment.
hmm, did you come across the case when Commit was called more than once for a given query. i.e. do we really need once here? 🤔
There was a problem hiding this comment.
Not yet for this API endpoint, but because this is an expensive operation it is a good idea to make sure we don't repeat this. The RepositoryResolver does the same currently.
Sidenote: The backend changes were introduced in #52937 and I didn't do the PR stacking correctly I think which is why it showed up here once that PR was merged. I've fixed it now in this PR by dropping those redundant commits and rebasing off main.
199246f to
f5eb3b4
Compare
f5eb3b4 to
9982b7f
Compare
sashaostrikov
left a comment
There was a problem hiding this comment.
LGTM, thanks for addressing the comments 👍🏻
| const pageTitle = commit | ||
| ? commit.subject | ||
| : repo.sourceType === RepositoryType.PERFORCE_DEPOT | ||
| ? `Changelist ${props.changelistID}` | ||
| : `Commit ${props.revspec}` |
There was a problem hiding this comment.
I am not a fan of nested ternaries like this, would prefer some boomer if-statements, but I also know this is just what React looks like so feel free to ignore.
There was a problem hiding this comment.
Will fix in a follow up.
Co-authored-by: Peter Guy <peter.guy@sourcegraph.com>
🎥 Check out the Loom recording: https://www.loom.com/share/f16f976175614c43ba1d77366b24f756
Blocked by #52937. Part of #40330.
Test plan