Svelte: add debug view for code intel occurrences#63475
Conversation
There was a problem hiding this comment.
Updating to match the designs. Notably, this also changes the display of the theme selector to be a more traditional looking radio menu
There was a problem hiding this comment.
Is there anything here I need to worry about accessibility-wise? I'm not exactly sure how adding an <input type="radio"> interacts with use:radioItem, which also adds an aria role to the outer div.
There was a problem hiding this comment.
Yeah, that doesn't feel right. Looking at the accessibility inspector I can see that we now have a radio element inside a menuitemradio, and the former doesn't have a label.
I don't know what a good solution would be for this situation. I wish lucide had icons that look like radio buttons. The closest I could find would be circle and maybe circle-check-big. https://lucide.dev/icons/?search=circle
There was a problem hiding this comment.
Okay, I used aria-hidden on the input itself because it is just a visual aid for what's already encoded in the menuitemradio fields. I also adjusted the text so that the menuitemradio can determine its label from its contents, and attached the "Code intelligence preview" as a label on the group.
There was a problem hiding this comment.
I could not figure out how to make codemirror position the element correctly. Using a block element broke up the line, which is not what I wanted. Instead, we just add the element at the end of the line and offset it with spaces.
f1af668 to
d879d6a
Compare
There was a problem hiding this comment.
Should we add something like MenuText that adds the dropdown menu styling? Looks like wildcard has somehting similar
d879d6a to
f9a9e93
Compare
There was a problem hiding this comment.
Needed to ensure that the offset provided by the spaces is not messed up by padding on cm-line. Ideally, we'd be able to use codemirror to position to a char, but I spent more time than I'd like to admit trying to get that to work.
f9a9e93 to
ca64b83
Compare
ca64b83 to
2b2d2be
Compare
There was a problem hiding this comment.
I haven't looked at the React implementation, but is it functionally the same? You said the React version makes a separate request to fetch the data... is that per index/commit or for the current revision of the file?
My understanding of how it currently works: The code graph data contains information from multiple commits. I.e. occurrences from a later commit basically overwrite earlier occurrences data. The menu lets you debug/view only the data of a specific commit, i.e. it will be a subset of the currently available data.
Is it the same in the React version or does it actually fetch all the occurrences data for the selected index/commit?
And if there is a difference, is that difference relevant?
There was a problem hiding this comment.
It's been a while since I wrote a CodeMirror extension, but I don't think you have to use a field here. You should be able to compute the decorations directly from the facet. Something like:
enables: facet => EditorView.decorations.compute(
[facet],
state => Decoration.set(state.facet(facet).map(...))
),There was a problem hiding this comment.
Yeah, so, weird thing. There is a restriction with Code Mirror that block widgets can only be added via state fields. I don't really understand why, but it's something to do with being able to calculate the height of the elements in order to figure out what's in view. It throws an error when I try to do this without a state field.
There was a problem hiding this comment.
I think it's fine to have that live in web-sveltekit btw, but not important.
There was a problem hiding this comment.
Yeah, that doesn't feel right. Looking at the accessibility inspector I can see that we now have a radio element inside a menuitemradio, and the former doesn't have a label.
I don't know what a good solution would be for this situation. I wish lucide had icons that look like radio buttons. The closest I could find would be circle and maybe circle-check-big. https://lucide.dev/icons/?search=circle
I think this is directionally correct, in that we want the client to get structured data and format it appropriately instead of just getting strings and jamming those into the DOM
Yeah, exactly, that's why we want to standardize on SCIP, so that reusing the same machinery in different places. However, this will still be somewhat of a regression for debugging, as relationship information is currently not available through the occurrences API. Relationships affect code navigation behavior in the presence of sub-classing etc. I've filed a follow-up issue for fixing that here. https://linear.app/sourcegraph/issue/GRAPH-713/make-relationship-information-available-in-occurrences-api -- We can work on updating the UI once that is available. |
varungandhi-src
left a comment
There was a problem hiding this comment.
Don't have specific input on the frontend code; thanks for adding this! ❤️
804666c to
cfb2298
Compare
2b2d2be to
eb1ce42
Compare
It is not quite the same because, as Varun pointed out, the data returned by the new occurrences API does not quite have the same richness as the data returned by the snapshot API. However, the data returned by the occurrences API will exactly match what we now use as our hoverable tokens because it uses the exact same source of data: the list of occurrences.
I don't think that's quite true. My understanding is that the list of commits is the set of commits with precise code intel that are ancestors of the current commit and have not been made obsolete by a new commit with precise code intel. There might be multiple because commit histories are nonlinear, and there is no obvious way to choose between them. The current implementation arbitrarily chooses one (the first one) and ignores any others because we have no heuristics for when one is more relevant than another right now.
It doesn't actually fetch anything new. It uses the same data we fetch on page load for the precise occurrences for hovers, which includes all occurrences for the possibly-relevant commits. |
This implements the debug view for code intel ranges. Since we're doing work here, it's very useful to be able scan the info while exploratory testing and debugging.
Stacked on https://github.com/sourcegraph/sourcegraph/pull/63473
Note that this does not use the snapshot API. After #63473, everything uses occurrences, so rather than rely on another API request, the only argument to the debug extension is a set of occurrences. This is particularly nice because it would be very easy to do things like:
Fixes [an open issue that I cannot find]
Test plan
screenshot-2024-06-25_16-03-47.mp4