Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Highlight new implementations in summary #852

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

sudo-jarvis
Copy link
Contributor

@sudo-jarvis sudo-jarvis commented Feb 11, 2024

Closes #448.

Implemented a way to fetch older reports as well and check which implementations are new by comparing both the reports and highlight such implementations in the UI in summary table.

Implementation :
Showing a small + sign in front of implementations that are new i.e. not present in the previous build report and on hovering over the + sign, the user sees a Newly Added Implementation Tooltip

Sample :

Screen.Recording.2024-02-14.at.5.28.21.PM.mp4

馃摎 Documentation preview 馃摎: https://bowtie-json-schema--852.org.readthedocs.build/en/852/

@sudo-jarvis
Copy link
Contributor Author

@Julian , does the change in the UI look good ?

@Julian
Copy link
Member

Julian commented Feb 11, 2024

Cool! Let's maybe use some bright color for "new implementation" rather than the same gray as what we use for "active row" -- can you perhaps check to see whether we have some light pink or green maybe in the CSS theming we're using, I suspect so since we're using Bootstrap.

@Julian
Copy link
Member

Julian commented Feb 11, 2024

There's also a small problem I suppose with my suggestion which I'm just thinking of -- essentially this will only show an implementation as new for 24 hours, when ideally we'd show it for some longer period, maybe a week or something.

But I don't have a good idea for how to do that yet, and I think the work you've done is anyways useful for day-over-day information, so maybe the right thing for now is to keep going and we can change/improve this when we have another idea.

Copy link
Member

@Julian Julian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked at this fully yet, so here's just a first pass with one comment that I expect may take a little refactoring.

frontend/src/components/Summary/ImplementationRow.tsx Outdated Show resolved Hide resolved
frontend/src/components/Summary/ImplementationRow.tsx Outdated Show resolved Hide resolved
frontend/src/data/parseReportData.ts Outdated Show resolved Hide resolved
Co-authored-by: Julian Berman <Julian@GrayVines.com>
@sudo-jarvis sudo-jarvis changed the title Dont merge until other PR !! : Highlight new implementations in summary WIP : Highlight new implementations in summary Feb 14, 2024
@sudo-jarvis sudo-jarvis changed the title WIP : Highlight new implementations in summary Highlight new implementations in summary Feb 16, 2024
@sudo-jarvis
Copy link
Contributor Author

@Julian , I have used http streaming now using fetch itself to just fetch the metadata of the report for finding about new implementations. Please have a look

@sudo-jarvis
Copy link
Contributor Author

@Julian ig you missed this

@Julian
Copy link
Member

Julian commented Feb 18, 2024

Yes, thanks for the ping, looking again.

@Julian
Copy link
Member

Julian commented Feb 18, 2024

It also occurs to me that with @adwait-godbole's change to add an implementations.json file, containing the output of Bowtie info, that we have a second way of doing this by instead looking at that file instead of at the report -- but we can also continue down this way and always decide to change later, especially considering that PR isn't merged yet and your previous one which stores yesterday's report already is.

@sudo-jarvis
Copy link
Contributor Author

Oh okay so the implementations.json file has a list of all implementations ? Also @Julian I just checked ig the regenerate test reports workflow hasnt run succesfully after merging the previous report changes. So the previous reports still arent accessible at bowtie.report/previous/some_draft.json

@Julian
Copy link
Member

Julian commented Feb 20, 2024

I just checked ig the regenerate test reports workflow hasnt run succesfully after merging the previous report changes.

I'm having a look to address the disk space issue which has been annoying, I'll get you a working run.

@sudo-jarvis
Copy link
Contributor Author

@Julian, I have taken into consideration the review comments and have made the changes. Please review

@sudo-jarvis
Copy link
Contributor Author

@Julian Please have a look at this as well when you get time

@Julian
Copy link
Member

Julian commented Feb 20, 2024

It looks good from the first glance but I'm playing still with the disk space issue.

frontend/src/data/Dialect.ts Outdated Show resolved Hide resolved
frontend/src/index.tsx Outdated Show resolved Hide resolved
frontend/src/components/Summary/ImplementationRow.tsx Outdated Show resolved Hide resolved
@Julian
Copy link
Member

Julian commented Feb 21, 2024

(There's a working run with previous implementations FWIW now)

@harrel56
Copy link
Collaborator

(There's a working run with previous implementations FWIW now)

Is it always a report from 1 day ago? I'm just thinking if a new implementation appears then it should be marked as new for a longer period of time, even like a month

@sudo-jarvis
Copy link
Contributor Author

@harrel56 Yeah currently it would vanish after 24 hrs when the workflow runs again. Ig to ensure that it stays for some time we would need the date when a particular implementation was added stored somewhere.

@harrel56
Copy link
Collaborator

Hmm, actually having previous report from one month ago in a current setup is a bit hard to achieve. We use https://github.com/dawidd6/action-download-artifact action for downloading artifacts and unfortunately it doesn't have option to somehow filter by creation date. But it uses GH API (https://docs.github.com/en/rest/actions/artifacts?apiVersion=2022-11-28#list-workflow-run-artifacts) and here each artifact has its creation date, so it's totally doable. But i don't know if it's worth to implement on our own

@Julian
Copy link
Member

Julian commented Feb 21, 2024

I think doing fancier things there will be easier if we control more of the site generation from inside Bowtie rather than in the workflow -- specifically getting us to having a bowtie site CLI command (essentially specifically tailored to "generate the data the UI wants") where we can evolve that as needed, including by generating more of these kinds of "materialized views". There's a very first step there now here (on a poorly named badges branch because it started as a way to fix #909) only inasmuch as the sequence of generated data makes slightly more sense, and there's now a site/ directory instead of splatting everything into the repo checkout.

Obviously I agree a day is too short, and a month sounds good there -- trying to also unblock our real current issue (the disk size nonsense) which is more pressing, but I'm certainly open to any kind of idea for this -- whether we delay a bit, merge this as is and improve after, or yeah anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Highlight new implementations in the UI
3 participants