Fix K2 for new GitHub UI #226

mjasikowski · 2025-01-14T09:06:53Z

The new javascript selectors are looking nasty, unfortunately the new GitHub UI has a lot of random, unreliable class names that we can't really attach to. This the best I could do in a short timespan.

I hope they won't shift the UI around once again.

Testing

Build the extension for your browser of choice with npm run build:chrome or npm run build:firefox
Package it to a dist.zip file with npm run package
Load it up (for Firefox, you need to go to about:debugging)
Check if the K2 dashboard work correctly at https://github.com/Expensify/Expensify#k2
Go to an issue (you can use this one for testing, but still be wary what you press). Check if the issue owner change buttons (stars) and buttons on the right hand side load correctly and if they work.
Got a PR that has [HOLD] in the title (you can use this one for testing) and see if the merge pull request button is disabled with an appropriate message above it

mjasikowski · 2025-01-14T10:50:02Z

Made this version 1.4.0 (minor version bump), as it's quite a big change.

src/js/lib/pages/github/issue.js

tgolen · 2025-01-14T14:57:34Z

src/js/lib/pages/github/pr.js

+        $('div[data-testid="mergebox-partial"] > div > div > section:last-of-type h3')
+            .text('You cannot merge your own PR');
+        $('div[data-testid="mergebox-partial"] > div > div > section:last-of-type p')
+            .html('It\'s like giving yourself a high-five in public.');


tgolen

I'm gonna install this and test it out today!

tgolen · 2025-01-14T15:30:54Z

It looks like the issue owner feature isn't working quite right.

Bug 1

Steps taken:

Click the star next to my name
Verify that the Issue Owner data is added to the GH description

Expected outcome:

The star next to my name should now be yellow to indicate that I am the issue owner

Actual outcome:

All the stars remain uncolored

tgolen · 2025-01-14T15:32:37Z

Bug 2

Steps Taken:

With one person assigned as the issue owner (as verified by the Issue Owner markdown in the GH description)
Click the star next to a different assignee to change them to be the current owner

Expected Outcome

The old owner should be removed and the new owner should be added.

Actual Outcome

The markdown in the GH description contains duplicate Issue Owner data for both people.

neil-marcellini

Thanks, looks good. Please add test steps and make sure everything changed is working. I see Tim found some bugs.

mjasikowski · 2025-01-14T15:59:02Z

Hm, that's weird, I tested the stars earlier today and they were working fine. I'll take another look and see if I can fix that! Thanks for reporting.

MonilBhavsar

Thanks for fixing this!

MonilBhavsar · 2025-01-14T16:12:33Z

src/js/lib/pages/github/_base.js

@@ -47,11 +45,11 @@ export default function () {
    Page.setup = function () {};

    Page.getRepoOwner = function () {
-        return $('.author a span').text();
+        return document.querySelectorAll('.AppHeader-context-item-label.Truncate-text')[0].textContent.trim();


NAB, I wonder if we could keep the repeatedly used selectors as constant with meaningful names of what we are selecting - For example - const selectorForRepoLabel = '.AppHeader-context-item-label.Truncate-text'; , so next time github changes UI, we could simply change the constant instead of going all over the code, and hopefully it should be simple to transition and works 🙏

For sure, I agree with that. Once we get this PR of the gate I'll spend some time to reorganize the selectors a little bit.

That would be great. Thank you!!

NAB ➕ I would also like to see this. Is it easy enough to do in this PR?

The repeating selectors are now removed, but I've added a bunch of comments for existing selectors to clarify what exactly they are selecting.

I chose comments over variables to not have to introduce additional null checks to satisfy ESLint for selectors that turn out empty.

mjasikowski · 2025-01-15T11:27:02Z

A few new changes:

Fixed bugs reported by @tgolen
I've modified the owner setting behavior to work based on events with async/await, rather than issue body parsing (for some reason this wasn't working too well)
Removed the previously-borked-but-fixed behavior that prevented PR self-merging per this discussion
Added some simple spinner animations for changing the issue owner and clicking checklist copy buttons
Optimized/simplified the code for setting the owner and rendering checklist copy buttons
Removed createpr.js, which was unused and contained references to functions that don't exist
Removed some obsolete rules from .eslintrc.js:

{
        '@lwc/lwc/no-async-await': 'off',
        'es/no-nullish-coalescing-operators' : 'off'
}

mjasikowski · 2025-01-15T15:04:37Z

Small bug fix, but ready for review now.

src/js/lib/pages/github/_base.js

src/js/lib/pages/github/issue.js

Co-authored-by: Tim Golen <[email protected]>

src/js/lib/pages/github/issue.js

tgolen · 2025-01-15T22:51:02Z

It looks like the K2 label buttons aren't working quite right.

Here, I've changed the issue from Daily to Monthly (but clicking on the "M" button) and it's left both labels on the issue.

Here is a case where the "M" button isn't highlighted (like after refreshing the page).

…ew-ui-fix

mjasikowski · 2025-01-16T10:32:30Z

@tgolen reverted the previous assignee setting functions and fixed the K2 label selectors, would you mind giving a spin again?

neil-marcellini

One small fix please. Other than that it looks good assuming testing passes. Can you please write test steps even if it's just "load the K2 extension from this branch and test all features"?

neil-marcellini · 2025-01-16T18:21:05Z

src/js/lib/pages/github/_base.js

+                console.error(`Failed to load contents of ${checklistUrl}: ${response.statusText}`);
+                return;
+            }
+
+            const fileContents = await response.text();
+
+            if (!fileContents) {
+                console.error(`Could not load contents of ${checklistUrl} for some reason`);
+                return;


In these error cases we should revert the loader. So instead of logging and returning we could throw an error and have the catch and finally block handle things.

This was the original code moved to a different function, but I changed it to:

try { // Fetch the checklist contents const response = await fetch(checklistUrl); const fileContents = await response.text(); // Call the API to add the comment await API.addComment(fileContents); } catch (error) { console.error('Error fetching the checklist:', error); } finally { // Restore the original button content button.innerHTML = originalContent; }

this way the loader will always be reverted, and:

If fetch() returns something other than 200, it will throw anyway

If fetched text is empty, then API.addComment() will throw due to incorrect parameters

So all error cases are covered now.

neil-marcellini · 2025-01-16T18:22:09Z

src/js/lib/pages/github/_base.js

@@ -47,11 +45,11 @@ export default function () {
    Page.setup = function () {};

    Page.getRepoOwner = function () {
-        return $('.author a span').text();
+        return document.querySelectorAll('.AppHeader-context-item-label.Truncate-text')[0].textContent.trim();


NAB ➕ I would also like to see this. Is it easy enough to do in this PR?

mjasikowski · 2025-01-17T10:12:16Z

@neil-marcellini basic test steps added and all change requests were addressed

neil-marcellini

Good to go! Thanks for your hard work.

mjasikowski added 3 commits January 13, 2025 14:27

Fixed checklist copy buttons for new UI

3665c56

Fixed k2 for new github UI

8a4130a

Fixed k2 for new github UI

e803157

mjasikowski changed the title ~~Fix K2 for new GitHub UI~~ [Hold] Fix K2 for new GitHub UI Jan 14, 2025

mjasikowski marked this pull request as ready for review January 14, 2025 09:24

mjasikowski added 2 commits January 14, 2025 11:43

Fixed k2 for new github UI

0f09fff

Removed leftover webpack config

d2969a9

mjasikowski requested review from a team and tgolen January 14, 2025 10:46

melvin-bot bot requested review from MonilBhavsar and removed request for a team January 14, 2025 10:46

mjasikowski added 2 commits January 14, 2025 11:48

Updated version numbers

eb83283

Updated CHANGELOG.md

fd96fd6

mjasikowski self-assigned this Jan 14, 2025

mjasikowski changed the title ~~[Hold] Fix K2 for new GitHub UI~~ Fix K2 for new GitHub UI Jan 14, 2025

tgolen requested changes Jan 14, 2025

View reviewed changes

Fixed selectors

fbf2d89

tgolen changed the title ~~Fix K2 for new GitHub UI~~ [HOLD] Fix K2 for new GitHub UI Jan 14, 2025

tgolen previously approved these changes Jan 14, 2025

View reviewed changes

tgolen changed the title ~~[HOLD] Fix K2 for new GitHub UI~~ Fix K2 for new GitHub UI Jan 14, 2025

neil-marcellini requested changes Jan 14, 2025

View reviewed changes

MonilBhavsar previously approved these changes Jan 14, 2025

View reviewed changes

Fixed owner behavior

8e464d5

mjasikowski dismissed stale reviews from MonilBhavsar and tgolen via 8e464d5 January 15, 2025 09:57

Fixed owner behavior, cleanup + eye candy

59ccfab

mjasikowski changed the title ~~Fix K2 for new GitHub UI~~ [HOLD] Fix K2 for new GitHub UI Jan 15, 2025

mjasikowski changed the title ~~[HOLD] Fix K2 for new GitHub UI~~ Fix K2 for new GitHub UI Jan 15, 2025

mjasikowski added 2 commits January 15, 2025 16:02

Fixed checklist copy for issues

6eee628

Fixed checklist copy for issues

6deae98

tgolen requested changes Jan 15, 2025

View reviewed changes

src/js/lib/pages/github/_base.js Outdated Show resolved Hide resolved

src/js/lib/pages/github/issue.js Outdated Show resolved Hide resolved

Update src/js/lib/pages/github/_base.js

ab4410b

Co-authored-by: Tim Golen <[email protected]>

tgolen requested changes Jan 15, 2025

View reviewed changes

src/js/lib/pages/github/issue.js Outdated Show resolved Hide resolved

mjasikowski added 2 commits January 16, 2025 11:31

Fixed panel buttons, reverted back to previous assignee behavior

1cdb513

Merge remote-tracking branch 'origin/github-new-ui-fix' into github-n…

90f1e3c

…ew-ui-fix

neil-marcellini requested changes Jan 16, 2025

View reviewed changes

tgolen previously approved these changes Jan 16, 2025

View reviewed changes

Added selector comments

dbf1cbb

mjasikowski dismissed tgolen’s stale review via dbf1cbb January 17, 2025 09:54

Fixed the copy checklist error handling

91f5531

tgolen approved these changes Jan 17, 2025

View reviewed changes

neil-marcellini approved these changes Jan 17, 2025

View reviewed changes

neil-marcellini merged commit d433312 into main Jan 17, 2025
3 checks passed

neil-marcellini deleted the github-new-ui-fix branch January 17, 2025 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix K2 for new GitHub UI #226

Fix K2 for new GitHub UI #226

mjasikowski commented Jan 14, 2025 •

edited

Loading

mjasikowski commented Jan 14, 2025

tgolen Jan 14, 2025

tgolen left a comment

tgolen commented Jan 14, 2025

tgolen commented Jan 14, 2025

neil-marcellini left a comment

mjasikowski commented Jan 14, 2025

MonilBhavsar left a comment

MonilBhavsar Jan 14, 2025

mjasikowski Jan 14, 2025

MonilBhavsar Jan 14, 2025

neil-marcellini Jan 16, 2025

mjasikowski Jan 17, 2025

mjasikowski commented Jan 15, 2025

mjasikowski commented Jan 15, 2025

tgolen commented Jan 15, 2025

mjasikowski commented Jan 16, 2025

neil-marcellini left a comment

neil-marcellini Jan 16, 2025

mjasikowski Jan 17, 2025

neil-marcellini Jan 16, 2025

mjasikowski commented Jan 17, 2025

neil-marcellini left a comment

Fix K2 for new GitHub UI #226

Fix K2 for new GitHub UI #226

Conversation

mjasikowski commented Jan 14, 2025 • edited Loading

Testing

mjasikowski commented Jan 14, 2025

Choose a reason for hiding this comment

tgolen left a comment

Choose a reason for hiding this comment

tgolen commented Jan 14, 2025

Bug 1

Expected outcome:

Actual outcome:

tgolen commented Jan 14, 2025

Bug 2

Expected Outcome

Actual Outcome

neil-marcellini left a comment

Choose a reason for hiding this comment

mjasikowski commented Jan 14, 2025

MonilBhavsar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjasikowski commented Jan 15, 2025

mjasikowski commented Jan 15, 2025

tgolen commented Jan 15, 2025

mjasikowski commented Jan 16, 2025

neil-marcellini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjasikowski commented Jan 17, 2025

neil-marcellini left a comment

Choose a reason for hiding this comment

mjasikowski commented Jan 14, 2025 •

edited

Loading