Conversation
dividedmind
force-pushed
the
feat/review-ui
branch
from
51c9b6a to
524c32c
Compare
|
@jansorg this is a draft because we're still finalizing the upstream UI changes, but do you mind giving this a look? Code here is pretty much done except some possible styling changes. Thanks! |
|
@dividedmind Is this superseding #901? |
jansorg
left a comment
jansorg
left a comment
There was a problem hiding this comment.
I like that all the webviews are in a single directory now. That was long overdue.
I can't comment on the packaging with this many files, but I assume it's okay 😄
Overall, the changes to the plugin itself seem fine to me and I didn't notice anything out of place, only a few minor issues.
I'll do another pass when this PR is ready for review.
At least on my setup, the review editor uses a different font for headings than the others. Probably just a missing .css or font resource.
dustinbyrne
force-pushed
the
feat/review-ui
branch
from
2a54f49 to
36229ba
Compare
It's meant to extend #901 and since we still haven't released that one I think we can merge and release both together. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds experimental review UI functionality by consolidating multiple appland modules into a unified structure. The changes remove several standalone modules (appland-signin, appland-navie, appland-install-guide, appland-findings) and update the appland-webview module to handle the review UI functionality directly.
Key changes:
- Consolidates Vue plugin initialization and webview mounting into appland-webview/appmap.js
- Updates HTML to use ES modules and direct script loading
- Removes multiple duplicate module directories and their build configurations
Reviewed Changes
Copilot reviewed 19 out of 179 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| appland-webview/appmap.js | Consolidates Vue plugin setup and webview mounting, changes export pattern |
| appland-webview/appmap.html | Updates to use ES modules and simplified script loading |
| appland-signin/* | Complete removal of signin module and build configuration |
| appland-navie/main.js | Removal of navie module entry point |
| appland-install-guide/* | Complete removal of install guide module and build configuration |
| appland-findings/* | Complete removal of findings module and build configuration |
jansorg
left a comment
jansorg
left a comment
There was a problem hiding this comment.
I added a few comments, mostly minor.
I'm not sure if the new, experimental review editor should be ready in this PR. If it shouldn't, please ignore my comment.
Previously, I added a few notes to the earlier PR at #901. I didn't repeat them here.
|
|
||
| import org.jetbrains.annotations.Nls; | ||
|
|
||
| public class ReviewEditor extends WebviewEditor<Void> { |
There was a problem hiding this comment.
I don't think the new, experimental review editor is ready yet.
The rendering of fonts and colors doesn't match the other AppMap editors or the IDE, for example.
There was a problem hiding this comment.
Styling is a bit of a work in progress, but I think the consensus was to get this merged and continue iterating on the main branch — it is behind an undocumented feature flag https://github.com/getappmap/appmap-intellij-plugin/pull/902/files#diff-1591b1656feb9ac287713ce03bd0887575a64fdd21d27ecccf9c34990c08f935R147 , so the user should beware that it's pre-release quality. @dustinbyrne @kgilpin is that fair?
dividedmind
force-pushed
the
feat/review-ui
branch
from
36229ba to
0968c01
Compare
|
Can this be closed? |
Uh, thanks for the ping. Actually, let me get this into shape and let's merge it. |
dividedmind
force-pushed
the
feat/review-ui
branch
from
0968c01 to
f869514
Compare
|
@jansorg I rebased and did some more cleanup (I noticed a js update gh job that was seemingly never used – removed that now), can you give it a quick glance and merge? The UI is still behind a feature flag and not active by default, but I think it makes it easier to work with and test if it's in main. Plus, the merge of webviews was long overdue... |
|
This looks good to me, but I'll merge after I had a bit more time to make sure the packaging is fine. But I'm pretty sure it is... (I used the GitHub JS update job, but apparently nobody else. It's fine to remove it.) |
Thank you!
Oh, GH logs don't show any executions, but I suppose it must have pruned old log entries. Anyway, I think in some ways it's better if the update is more manual, as it gives the opportunity to do a sanity check beyond the automated tests. |
This is both for ease of maintainance and to reduce the size of the plugin. Yarn is migrated to 4.1.3 and added to the repository; webview update CI workflow is also removed (it appears to never have been used).
Note this is currently behind appmap.experimental.review.ui registry feature flag. Thus it is not normally exposed to the users; some more work, not least UI polish, is needed.
I pushed a change to exclude *.map from the plugin zip package. This reduces the total size from 28mb to about 21mb. At least I'm pretty sure that the mapping files are not used in production. |
Without the .maps any js backtraces will be nearly useless. OTOH they're kinda hard to get to anyway, I don't think we've ever used them. |
Wouldn't they be stored in git under a tag? We don't have to push them to every user to use them for stacktraces, I believe. |
That is true, although I'm not even sure how to recover locations manually from a mapless stacktrace - if the maps are in place the runtime automatically uses them when generating the stacktrace. But you're right, not useless :) |
Thanks, I didn't know that the runtime already applies them when available. I haven't seen a JS stacktrace yet. I'd say we can add *.map back into the package after we receive the first JS stacktrace. |
This adds the experimental review UI. Note currently the components code isn't released for this yet; it requires https://github.com/getappmap/appmap-js/tree/feat/review-homepage branch of appmap-js.