| *** ralonsoh_ is now known as ralonsoh | 11:36 | |
| opendevreview | Thales Elero Cervi proposed openstack/project-config master: Add repo app-prometheus to StarlingX https://review.opendev.org/c/openstack/project-config/+/982495 | 13:11 |
|---|---|---|
| opendevreview | Thales Elero Cervi proposed openstack/project-config master: Add repo app-prometheus to StarlingX https://review.opendev.org/c/openstack/project-config/+/982495 | 13:22 |
| *** ykarel_ is now known as ykarel | 14:02 | |
| *** ykarel is now known as ykarel_ | 14:14 | |
| *** ykarel_ is now known as ykarel | 14:15 | |
| opendevreview | Merged openstack/project-config master: Add repo app-prometheus to StarlingX https://review.opendev.org/c/openstack/project-config/+/982495 | 19:12 |
| sean-k-mooney | clarkb: by the way remember the issue i raised with gerrit not displaying all comment when navagating between reviews. that is still happening today | 21:28 |
| sean-k-mooney | do we think gerrit 3.12 or the cache backend changes will fix it | 21:29 |
| sean-k-mooney | as far as i coudl tell it was an issue in the gerrit react applcation itself | 21:29 |
| clarkb | ok I remember spending a lot of time trying to reproduce it and I thought we decided the problem had to do with gerrit autohiding things across patchsets | 21:29 |
| sean-k-mooney | yes | 21:29 |
| sean-k-mooney | but i was also seeing it on the latest revision of a patch today | 21:29 |
| clarkb | like if you're looking at the wrong patchset version for teh comment. SO I think it is "intentional" (whether correct is another story) so I doubt that the cache change would help | 21:30 |
| clarkb | it is possible that they have fixed it as part of the UI updates though | 21:30 |
| sean-k-mooney | in this case i was looking a the latest revsion of a patch where i had draft comment pending submission | 21:30 |
| sean-k-mooney | and my draft comments were not showing | 21:30 |
| clarkb | was the draft comment for an older patchset? | 21:30 |
| sean-k-mooney | until i hit F5 | 21:30 |
| sean-k-mooney | no | 21:31 |
| sean-k-mooney | i may have got do an older revsion at some point and gon back to the latest but when i did next and prevs it didnt fix it | 21:31 |
| sean-k-mooney | as in the app | 21:31 |
| clarkb | ok then ya I doubt the cache backend update will affect that. I suspect it has more to do with how the web ui is deciding what data it needs/wants and whether or not it can proceed without that | 21:31 |
| clarkb | so it is possible newer Gerrit will help just not due to the cache change | 21:32 |
| sean-k-mooney | so the reason i noticed it is i had the same review open in two diffent browser windows since i was review diffent fiels in both (test and production code i think) | 21:32 |
| sean-k-mooney | and the comen i saw in oen browser were not lining up to the other | 21:32 |
| clarkb | unfortunately, I don't think I've ever observed the issue on my end. With all of the weirdness I saw being explained by looking at the "wrong" patchset | 21:34 |
| clarkb | if you see it again maybe check the network log for the page or the js console to see if there are any errors notes? | 21:34 |
| sean-k-mooney | last time i looke there were none | 21:34 |
| sean-k-mooney | so i think the issue is a related to how they redwa the screen when you change patches using next/previous or the keyboard shortcurt | 21:35 |
| sean-k-mooney | i dont think its doing a full page refresh | 21:35 |
| sean-k-mooney | i think they are doing ajax queries for the data and re rendering | 21:35 |
| sean-k-mooney | in that flow it does not get all the comments | 21:36 |
| sean-k-mooney | in a full page refrsh it does | 21:36 |
| clarkb | ya that could explain it. Could also explain why I don't see the problem as I don't tend to change patchsets at the top level just compare within the diffs | 21:36 |
| sean-k-mooney | i think i looked at that in the network tab in the past | 21:42 |
| sean-k-mooney | so ya if you change files it just calls the diff endpoint | 21:42 |
| sean-k-mooney | https://review.opendev.org/changes/openstack%2Fwatcher-dashboard~978258/revisions/5/files/releasenotes%2Fsource%2Fconf.py/diff?intraline&whitespace=IGNORE_NONE | 21:42 |
| sean-k-mooney | which is just the json for that file | 21:42 |
| sean-k-mooney | based on somehting (presumabel the content of the diff responce it sometall does a secod or subsequet network requet for content and fix:preview | 21:45 |
| clarkb | I think that change details with the comments option is how you fetch the comments | 21:53 |
| clarkb | but I'm not positive that that is how the web ui does it | 21:53 |
| sean-k-mooney | well looking at the raw web responce in the borswer i actully cant see the network request for them at all | 21:54 |
| sean-k-mooney | even when it dispalying | 21:54 |
| sean-k-mooney | so the are doing somethign tricky in the background | 21:54 |
| sean-k-mooney | i can see the XHR ajax request for each page but its on tincldued in the respocnes | 21:55 |
| clarkb | it is the changes detail request I found it | 21:57 |
| clarkb | give me a second to extract it out of firefox and I can share | 21:57 |
| clarkb | https://review.opendev.org/changes/openstack%2Fwatcher-dashboard~978258/detail?O=somevalue | 21:58 |
| clarkb | I don't know if the somevalue is user specific and should be kept secret so I edited it | 21:58 |
| sean-k-mooney | ack | 21:58 |
| sean-k-mooney | so im not seeing it call that for what its worth when i navaget between paages | 21:59 |
| clarkb | so maybe it is caching the value in the browser with data at time A. Then you work in browser #2 and make new changes at time B where B>A. If the first tab isn't refreshed to reload that content then you don't see the new state | 22:00 |
| sean-k-mooney | its loading it on the first refesh | 22:01 |
| sean-k-mooney | and then its not querying it again | 22:01 |
| sean-k-mooney | if you change version of navigate to a new file | 22:02 |
| sean-k-mooney | it does not qurey that again | 22:02 |
| clarkb | right its using all the existing state and not requerying | 22:02 |
| sean-k-mooney | yep which is invild | 22:02 |
| clarkb | I wonder how much of that is intentional vs oversight on their part. I knwo in the past they had the little yellow pop up thing telling you there is new conent and you can manually refresh | 22:02 |
| sean-k-mooney | its not how it used to work in the past | 22:02 |
| sean-k-mooney | as in when it was still a gwt applciations | 22:03 |
| clarkb | right but its been years since then. I wonder if the yellow helper box was intented to mitigate this and now that isn't working as expected for some reason either? | 22:03 |
| sean-k-mooney | they have done some major rewrite but even when you click up to go back to the main view | 22:03 |
| sean-k-mooney | it does not reload it | 22:03 |
| sean-k-mooney | so it very possible to leave review comemtn wihtout seeing feedback form others | 22:04 |
| sean-k-mooney | what yellow box are you refering too by the way? | 22:04 |
| sean-k-mooney | i dont think i have ever seen that | 22:04 |
| clarkb | it is like a pop up thing that would say "change has been updated click here to refresh" | 22:04 |
| clarkb | it used to do that and now I wonder if this behavior is why | 22:04 |
| clarkb | I also wonder if that stopped working | 22:05 |
| sean-k-mooney | i have seen that on the on the main page but never on any of the diff views | 22:05 |
| clarkb | yes on th top level change page not the diffs | 22:05 |
| sean-k-mooney | but if they can send that notificaion they can requry the content so i would still conidre it a bug | 22:05 |
| clarkb | but I haven't seen that do it in a while | 22:05 |
| sean-k-mooney | the only popup i have seen in gerrit recently is the one they do when you need to login again | 22:06 |
| clarkb | and yes if they know the data is stale then they could query it. I know that Google in particular is very sensitive to UI responsiveness and wonder if this all goes back to them optimizing those metrics by not fetching data too often | 22:06 |
| sean-k-mooney | well for me this is a ui resposivness fail | 22:06 |
| clarkb | in 3.13 or 3.14 they stopped making updates to the UI synchronous with backend actions so I'm sure we'll find even more weird cases | 22:06 |
| sean-k-mooney | becuas it requrie a full applcation reset and often getting an email notficaton and going back to review comemtn that were there but you could not see | 22:07 |
| clarkb | like when you post comments it will immediately report success while the backend is workign on indexing them so no one will actually see them yet. Currently one reason taht posting comments or abandoning changes etc can be slow is it waits for the backend to report success before indicating success to you | 22:07 |
| sean-k-mooney | right that sound like a terible idea | 22:08 |
| clarkb | application reset in this case is a refresh of the page | 22:08 |
| sean-k-mooney | there have been cases in teh past where one person has +w and anohter has -1'd | 22:08 |
| sean-k-mooney | so any non tansactionality is a problem | 22:08 |
| clarkb | I mean that can happen anyway | 22:08 |
| clarkb | I agree that this behavior is less than ideal. But Gerrit can't solve disagreements between reviewers generally | 22:09 |
| sean-k-mooney | it can but ligitbally if i cant see your coment when im navagting between file or on the main view when im clickign reply | 22:09 |
| sean-k-mooney | that a problem | 22:09 |
| sean-k-mooney | rgiht but if its activly hiding the comemnt that a problem | 22:09 |
| sean-k-mooney | as in if i only use the naviationin the gerrit applciation | 22:10 |
| sean-k-mooney | whewn i click reploy i cant be sure i have seen feeback for other that was there before i naviaged back to the main page | 22:10 |
| sean-k-mooney | in the past it was alwasy visabel and you could confirm you didnt miss anything before leaving your review | 22:10 |
| clarkb | doing some testing I'm able to reproduce what you are saying (which si good because we should be able to take that to gerrit if so) but only if I'm navigating within the change. As soon as I navigate back up to the project listing then back to the change it loads the data | 22:11 |
| clarkb | still doesn't help if two people are reviewing the same change concurrently and are in that change view the entire time | 22:11 |
| sean-k-mooney | rgiht so i would expect most pepoel review by navigating the chage diff then going back up to the main review without ever refressing the page | 22:12 |
| clarkb | yup or even just viewing the diffs on the main page | 22:13 |
| clarkb | loading the change in a new tag also seems to update things | 22:13 |
| clarkb | so its not a shared state across all tabs but seems to be tab specific? | 22:14 |
| sean-k-mooney | yep so gerrit is expected to be a synconous transactional view of the db. any eventual consticy is stictly a bug in my view | 22:14 |
| sean-k-mooney | but at the minim i think the navaction up to the main view should do a full refersh for other comments | 22:14 |
| sean-k-mooney | clarkb: each tab has its own instance of the applcation running client side | 22:15 |
| clarkb | when you say main view you mean the top level chagne page right? | 22:15 |
| sean-k-mooney | yes | 22:15 |
| sean-k-mooney | i.e. https://review.opendev.org/c/openstack/watcher-dashboard/+/978258 | 22:15 |
| clarkb | got it. Just wanted to clarify because going up to the top level project page then back to the change does reload the info | 22:16 |
| sean-k-mooney | how did you go up did you click on the rep/branch linke | 22:16 |
| sean-k-mooney | or one of the nave bar drop downs | 22:16 |
| clarkb | yes I clicked on the opendev/sandbox link in the top right of the change page next to Repo | Bnrach | 22:17 |
| clarkb | then I clicked on the change from that link to go abck to the change and that updated with the new comments | 22:17 |
| sean-k-mooney | ya so that is hitting the detail enpoint | 22:18 |
| sean-k-mooney | my guess is they have a react compont fo rthe review/diff view | 22:18 |
| sean-k-mooney | and when we are naviagting within a view we are navagatin gin that compoent | 22:18 |
| sean-k-mooney | but when we go up to the change view and back in its reloading it | 22:19 |
| sean-k-mooney | there is a <gr-app id="pg-app"></gr-app> custom element | 22:19 |
| sean-k-mooney | hum ok no its mroe then that | 22:20 |
| clarkb | it isn't react its something else, but yes probably operates similarly | 22:21 |
| sean-k-mooney | it used to be gwt | 22:21 |
| clarkb | yes now it is "polygerrit" | 22:21 |
| clarkb | which is some custom fork of some other tool | 22:21 |
| sean-k-mooney | the google web toolkit . it might be dart? | 22:21 |
| sean-k-mooney | but ya | 22:21 |
| sean-k-mooney | oh its using polymer | 22:23 |
| clarkb | I've left a tab open hoping that the stale tab will eventually notice it is stale but it hasn't | 22:23 |
| sean-k-mooney | https://polymer-library.polymer-project.org/3.0/docs/devguide/feature-overview | 22:23 |
| clarkb | so yes I think that at a minimum here the bug is that gerrit isn't noticing and asking you to refresh. Ideally it would just load the new data when it notices it is missing in the first place | 22:23 |
| clarkb | 3.14 is the version that makes web ui actions less synchronous with the backend (not 3.13) | 22:24 |
| clarkb | and 3.14 hasn't released yet but will soon | 22:24 |
| sean-k-mooney | when i was workign at my university i ported a buch of freefrom javscript enrichmetn to a php sigte to polermer.js/dart | 22:24 |
| sean-k-mooney | polymer was the main way to make webcomente just work across all browsers back in 2012 | 22:25 |
| sean-k-mooney | im sort of happy it still exists | 22:25 |
| sean-k-mooney | less so that it needs to exist as it really only exisit to make things work on older borwser that dont supprot modern web standards but im glad that the project is still maintained | 22:26 |
| fungi | it's relatively new in terms of gerrit webui implementations. this is, like, the third webui they've had now (second complete rewrite)? | 22:26 |
| fungi | i suppose when they started working on the polygerrit interface, polymer wasn't all that old yet | 22:27 |
| clarkb | yes gerrit 3.0 was the first version with it iirc | 22:27 |
| clarkb | so its about 7 years old? | 22:28 |
| sean-k-mooney | well polemer is actuly prtty nice as a way to od web-compoent in genreal | 22:28 |
| clarkb | I need to pop out now to take advantage of a dry afternoon, but I think this is something we can put together into a bug for upstream | 22:28 |
| sean-k-mooney | the browse api sfor this exsit but are not the nicest thing to work with normally | 22:28 |
| clarkb | basically with gerrit 3.11.9 if you update a change in browser A and browser B already has the change loaded then nothing loads the new data in browser B or indicates that the data viewed is stale until you navigate away from the change and back to it again | 22:29 |
| sean-k-mooney | yep ^ | 22:29 |
| clarkb | I can file that. I'm also happy if someone else does it. I'll get to it tomorrow | 22:30 |
| sean-k-mooney | you might want to also clarify exactly what you mean by naviate away as it requrie mroe the next/up/prev and requires clicking oen of the other links in the application | 22:31 |
| sean-k-mooney | or like manually havatating a bookmark or something | 22:31 |
| sean-k-mooney | by the way im pretty sure the pollygerrit ui is using https://developer.mozilla.org/en-US/docs/Web/API/History_API | 22:33 |
| sean-k-mooney | internally to fake the history as if you had navigated | 22:33 |
| sean-k-mooney | that why the url and history look like your naviagte but your actully not | 22:34 |
| sean-k-mooney | its one of my favorte feature in htmx https://htmx.org/attributes/hx-push-url/ but it make the concept of "navigating" kind fo fuzzy | 22:34 |
| sean-k-mooney | specificly i woudl guess they are using https://developer.mozilla.org/en-US/docs/Web/API/History/pushState to enulate full page navagation without actully doing it | 22:36 |
| clarkb | yes, in this case the url needs to change from prefix/number to something else | 22:38 |
| sean-k-mooney | "this.prevPageContext=i,this.currentPath=i.path,this.dispatch(i,n),t&&!i.preventPush&&i.pushState()" | 22:40 |
| sean-k-mooney | so inlien jsvascript is not very readable but ya they are using that in the even handlers | 22:41 |
| sean-k-mooney | ah you can pretty pitn the source https://paste.opendev.org/show/b2FxgvkZY7gj68MLgDPZ/ | 22:42 |
| sean-k-mooney | before "pretty" printing the file is ~25000 lines of js and embded html templsate after its ~113,000 lines of admittly much more readable code | 22:44 |
| sean-k-mooney | but still not "pretty" | 22:44 |
| sean-k-mooney | ah | 22:47 |
| sean-k-mooney | pushState() { | 22:47 |
| sean-k-mooney | window.history.pushState(this.state, this.title, this.canonicalPath) | 22:47 |
| sean-k-mooney | } | 22:47 |
| sean-k-mooney | that exactly what i was expecting to see | 22:47 |
| sean-k-mooney | ok ill stop distracting you. night all o/ | 22:48 |
Generated by irclog2html.py 4.1.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!