web: navigate to file@commit instead of file@branch#58381
Conversation
stefanhengl
force-pushed
the
sh/navigate-to-commit-instead-of-head
branch
from
a66d7b4 to
0269b6f
Compare
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 206a997...5c76ab1.
|
keegancsmith
left a comment
keegancsmith
left a comment
There was a problem hiding this comment.
This is def changelog worthy.
client/shared/src/search/stream.ts
Outdated
There was a problem hiding this comment.
I ain't know TS programmer but I think this can be shorted with the elvis operator:
| if (branches && branches.length > 0) { | |
| if (branches?.length > 0) { |
There was a problem hiding this comment.
not a TS programmer either, but I believe typescript doesn't let us compare "undefined" to a number literal, which would happen if branches is undefined.
client/shared/src/search/stream.ts
Outdated
There was a problem hiding this comment.
you should document why you don't use getRevision and maybe add a docstring to getRevision saying it prefers branches to version.
Also elvis comes to the party again (I think)
const revision = fileMatch.commit ?? fileMatch.branches?.[0]
Clicking on a search result might jump to the wrong line in the file if HEAD on gitserver is ahead of the commit we indexed with Zoekt. With this change we always jump to the commit of the search result we display. As a consequence - file URLs will contain the commit hash - we will always display the commit hash (instead of the branch) at the top of the blob view, even if commit = HEAD. While touching the code, I also refactored getRevision. I think the code is more readable now. Test plan: - manual testing
stefanhengl
force-pushed
the
sh/navigate-to-commit-instead-of-head
branch
from
0269b6f to
a063f1e
Compare
|
rebased to pull in latest CHANGELOG changes. |
|
@stefanhengl I noticed some confusing behavior on S2 after this was merged. The search indexes are fully up-to-date, but clicking on a search result brings me to a specific commit SHA. I think this SHA actually represents HEAD (a commit only 2 hours ago), but our blob view claims this commit is from 3 days ago 🤔 |
|
That's expected, but admittedly confusing. With the change, we always show the SHA, even if it represents HEAD. The blob view shows the date (3 days ago) of the last commit that changed this file, not the date of the index. The commit message is shown next to it. |
|
Suggestion: We add a little prefix "committed" or "authored" to the date, depending on whether we show the |
|
I see, thanks for the explanation! If I was confused, it's likely we'll have some confused users too! Could we avoid showing the hash if it's up-to-date? It feels best to maintain the previous "happy path" behavior that was clean and unsurprising. |
With this change we always jump to blob@commit instead of blob@head if a user clicks on a search result. This avoids inconsistencies in line numbers if indexing lags behind repo syncing. We keep the old logic, for displaying commit SHAs or branches on the search results page. As a consequence - file URLs will contain the commit hash (see updated tests) - we will always display the commit hash (instead of the branch) at the top of the blob view, even if commit = HEAD. While touching the code, I also refactored `getRevision`. I think the code is more readable now. Test plan: - manual testing - for a couple of searches, I checked the commit returned by the Stream API and verified that it matches the commit ID of the blob we display. - updated test
This reverts commit e45c2ed.
4 participants
With this change we always jump to blob@commit instead of blob@head if a user clicks on a search result. This avoids inconsistencies in line numbers if indexing lags behind repo syncing.
We keep the old logic, for displaying commit SHAs or branches on the search results page.
As a consequence
While touching the code, I also refactored
getRevision. I think the code is more readable now.Test plan: