github: add ability to edit commit message from PR in Lando UI (bug 2003888) by Magnolia-Liu · Pull Request #1161 · mozilla-conduit/lando

Magnolia-Liu · 2026-05-14T17:36:45Z

Added a view callable by GitHub to update pull request content with data from Lando
Added editable commit body and title fields in the PR UI
- Land button is disabled while fields are being edited to prevent landing stale content
Added an "Edit" and "Save" commit message button that refreshes the page on submit + cancel changes button
- Text areas are disabled during save to prevent concurrent edits
Added error display for invalid input

Demo of UI:

Screen.Recording.2026-06-01.at.5.21.42.PM.mov

github-actions · 2026-05-14T17:37:12Z

View this pull request in Lando to land it once approved.

zzzeid

Just a few drive-by comments

shtrom

Mostly stylistic comments, but this is looking good, and the overall logic makes sense.

shtrom · 2026-05-21T02:18:30Z

-                <td>{{ pull_request.title }}</td>
+                <td>
+                    <textarea readonly id="commit-title" class="textarea">{{ pull_request.title }}</textarea>
+                    <p class="help is-danger" id="commit-title-error"></p>
+                </td>
            </tr>
            <tr>
                <th>Commit Body</th>
-                <td>{{ pull_request.body }}</td>
+                <td>
+                    <textarea readonly id="commit-body" class="textarea">{{ pull_request.body }}</textarea>
+                    <p class="help is-danger" id="commit-body-error"></p>
+                </td>
            </tr>
        </table>
+        <p>


It may make sense to have title, body and button as a div-like form in a single table row.

shtrom · 2026-05-21T02:22:05Z

+            <button id="post-landing-job"
+                    class="button is-large is-loading post-landing-job has-text-white"


Having an id and a class named the same could be a bit of a footgun for our future selves.

Having an id is good, though. Perhaps we should just make sure that everything rely on it being an id, and remove the class altogether.

This would however force any page to only have a single landing button. I don't think we have it any differently at the moment, but maybe it's not something we want to commit to right here?

If anything, I'd look for ways to avoid needing to make this particular change, if possible.

It may be a good time to reduce clutter here. If .post-landing-job is used elsewhere we could change it to #post-landing-job.

Having said that, not sure if having them have the same name is an issue. I suspect you added this to be able to easily use document.getElementById?.

Having the same name is probably not a technical issue, but from a human differentiation perspective, it's probably best if they are different.

If .post-landing-job is used elsewhere we could change it to #post-landing-job.

@zzzeid I think you meant “if it ISN'T used elsewhere”? (otherwise the ids will collide).

shtrom · 2026-05-21T02:23:59Z

            json={"body": comment},
        )

+    def update_pull_request_content(


Suggested change

def update_pull_request_content(

def update_pull_request_description(

It may also make sense to make the title optional, so we can use it to only change the body.

I think this method is more generic than just updating the description. It can be used to update any attribute on the PR.

zzzeid · 2026-05-22T16:59:49Z

        });
      });
+
+      $("button.save-pr").on("click", function (e) {


I noticed in the UI demo that there is no feedback on the save button when it is clicked (unless I missed it?) This would be good for usability (e.g., disable the button temporarily, or change the text, etc...

zzzeid · 2026-05-22T17:01:06Z

Sign in to view

+              .getElementById("cancel-edit-pr")
+              .classList.add("is-hidden");
+
+            window.location.reload();


It would be good to file a bug under https://bugzilla.mozilla.org/show_bug.cgi?id=2000954 to update this to instead re-trigger the checks without a full refresh.

zzzeid · 2026-05-22T17:04:54Z

+        document.getElementById("commit-title").setAttribute("readonly", true);
+        document.getElementById("commit-body").setAttribute("readonly", true);
+        document.getElementById("commit-title-error").textContent = "";
+        document.getElementById("commit-body-error").textContent = "";
+        document.getElementById("commit-title").classList.remove("is-danger");
+        document.getElementById("commit-body").classList.remove("is-danger");
+        document.getElementById("commit-title").value =
+          document.getElementById("commit-title").defaultValue;
+        document.getElementById("commit-body").value =
+          document.getElementById("commit-body").defaultValue;
+        document.getElementById("save-pr").dataset.mode = "edit";
+        document.getElementById("save-pr").textContent = "Edit Commit Message";
+        document.getElementById("cancel-edit-pr").classList.add("is-hidden");
+        document.getElementById("post-landing-job").disabled = false;


nit: you can save the elements in variables and re-use them, instead of doing document.getElementById multiple times for each element.

zzzeid · 2026-05-22T17:06:47Z

+            <button id="post-landing-job"
+                    class="button is-large is-loading post-landing-job has-text-white"


It may be a good time to reduce clutter here. If .post-landing-job is used elsewhere we could change it to #post-landing-job.

Having said that, not sure if having them have the same name is an issue. I suspect you added this to be able to easily use document.getElementById?.

zzzeid · 2026-05-22T17:10:25Z

+                    <textarea readonly id="commit-title" class="textarea">{{ pull_request.title }}</textarea>
+                    <p class="help is-danger" id="commit-title-error"></p>


This is a bit out of scope for this PR, I filed a bug to remove the use of a table here altogether.

zzzeid · 2026-05-22T17:12:21Z

            json={"body": comment},
        )

+    def update_pull_request_content(


I think this method is more generic than just updating the description. It can be used to update any attribute on the PR.

zzzeid · 2026-05-22T17:13:19Z

This is probably erroneously showing up here because of the merge. I would recommend squashing this entire PR at some point, before attempting to merge it, to clean up the commit history.

shtrom · 2026-06-03T07:41:10Z

+        if not self.target_repo.user_can_push(request.user):
+            return JsonResponse(
+                {"errors": ["You are not allowed to push to this repository."]},
+                status=403,
+            )


I wonder if we should make a little assertion util that would return a consistent error everywhere it's called.

I already have this functionality partially implemented in #1160.

shtrom · 2026-06-03T07:46:16Z

-                    <textarea readonly id="commit-title" class="textarea">{{ pull_request.title }}</textarea>
-                    <p class="help is-danger" id="commit-title-error"></p>
+                    <p id="commit-title">{{ pull_request.title }}</p>
+                    <p id="commit-title-error" class="help is-danger"></p>


I wouldn't set the is-danger class on those, even if empty. It can be set as needed by the JS.

shtrom · 2026-06-03T07:49:02Z

+      $("#save-edit-pr").on("click", function (e) {
+        var save_edit_pr_button = $(this);
+
+        if (save_edit_pr_button.attr("data-mode") === "saved") {


Could we swap the logic of this method, so we do the case for saved by default, and the fetch only when we are sure we have data-mode===edit?

It may be worth splitting those two into separate methods, e.g.,

if (save_edit_pr_button.attr("data-mode") === "edit") { return save_pr_description(); } return edit_pr_description();

Sign in to view

+              .getElementById("cancel-edit-pr")
+              .classList.add("is-hidden");
+
+            window.location.reload();


shtrom · 2026-06-03T07:54:32Z

Sign in to view

+        const pTitle = document.createElement("p");
+        const pBody = document.createElement("p");
+        const textareaTitle = $("#commit-title")[0];
+        const textareaBody = $("#commit-body")[0];


Why do we use array indexing here? This seems odd, and we don't do it before in tho code, e.g., https://github.com/mozilla-conduit/lando/pull/1161/changes/8726f39b11c08695895b0a506e34cad6c652edd4..e6cbe4c36dfa3a2b52f622538df1f06eaa3bbc86#diff-4d791453e88ede62fd62f9e15ecccd2a546f9d0e6fe016091adb198d09a6ca67R272.

If it's not needed (which I'm not sure, tbh), then we should probably be consistent and use the same access type throughout the code.

shtrom · 2026-06-03T07:58:15Z

+            $("#commit-title-error").text("");
+            $("#commit-body-error").text("");
+            $("#commit-title").removeClass("is-danger");
+            $("#commit-body").removeClass("is-danger");
+            $("#commit-title").prop("disabled", true);
+            $("#commit-body").prop("disabled", true);
+            $("#cancel-edit-pr").addClass("is-hidden");


Perhaps we should have utility functions to set/reset the errors, e.g., something like

function set_error_for(selector, error_text, error_class) { $(selector + "-error").text(error_text); $(selector).removeClass("is-hidden"); $(selector).removeClass("is-danger"); $(selector).addClass(error_class); $(selector).prop("disabled", error_text === ""); }

zzzeid

Looking good, just a couple of questions/comments. Is it possible to see a new video with the latest UI implementation?

zzzeid · 2026-06-04T17:36:07Z

+        if not self.target_repo.user_can_push(request.user):
+            return JsonResponse(
+                {"errors": ["You are not allowed to push to this repository."]},
+                status=403,
+            )


I already have this functionality partially implemented in #1160.

zzzeid

Looking good! Just a couple of small changes and I think this will be ready to go.

shtrom

A few nits, and one remaining thing regarding the handling of the status from the fetch to the API (return early on 200, and handle all 4xx).

This looks pretty good overall, though. Nice job!

shtrom

shtrom · 2026-06-22T08:22:56Z


+@pytest.fixture
+def repo_github_api_client(repo_mc):
+    repo_mc(SCMType.GIT, name="git-repo")


nit: it may be safer to name the repo after the fixture it comes from, to avoid risks of collisions

…003888)

Magnolia-Liu requested review from cgsheeh, shtrom and zzzeid May 14, 2026 17:36

zzzeid reviewed May 14, 2026

View reviewed changes

Comment thread src/lando/utils/github.py

Comment thread src/lando/api/views.py

Magnolia-Liu marked this pull request as ready for review May 20, 2026 21:22

Magnolia-Liu requested a review from a team as a code owner May 20, 2026 21:22

shtrom requested changes May 21, 2026

View reviewed changes

shtrom reviewed May 21, 2026

View reviewed changes

Comment thread src/lando/api/tests/test_views.py

zzzeid requested changes May 22, 2026

View reviewed changes

shtrom reviewed May 25, 2026

View reviewed changes

Comment thread src/lando/static_src/legacy/js/components/Stack.js

Magnolia-Liu requested review from shtrom and zzzeid June 2, 2026 15:58

Magnolia-Liu changed the title ~~pull_request: add ability to edit commit message from PR in Lando UI~~ pull_request: add ability to edit commit message from PR in Lando UI (bug 2003888) Jun 2, 2026

shtrom reviewed Jun 3, 2026

View reviewed changes

zzzeid reviewed Jun 4, 2026

View reviewed changes

Magnolia-Liu requested review from shtrom and zzzeid June 12, 2026 18:14

cgsheeh mentioned this pull request Jun 12, 2026

prettier: update configs to more closely match Python formatting (Bug 2047147) #1251

Open

zzzeid requested changes Jun 12, 2026

View reviewed changes

Comment thread src/lando/api/views.py

Comment thread src/lando/api/views.py Outdated

Comment thread src/lando/static_src/legacy/js/components/Stack.js Outdated

Comment thread src/lando/api/tests/test_views.py Outdated

Comment thread src/lando/api/tests/test_views.py Outdated

shtrom requested changes Jun 15, 2026

View reviewed changes

Magnolia-Liu requested review from shtrom and zzzeid June 17, 2026 19:16

zzzeid approved these changes Jun 19, 2026

View reviewed changes

shtrom approved these changes Jun 22, 2026

View reviewed changes

Magnolia-Liu force-pushed the magnolia/bug-2003888-adding-edit-commit-msg branch from 124e4bb to 5d3e39f Compare June 22, 2026 18:54

Magnolia-Liu changed the title ~~pull_request: add ability to edit commit message from PR in Lando UI (bug 2003888)~~ github: add ability to edit commit message from PR in Lando UI (bug 2003888) Jun 22, 2026

Magnolia-Liu force-pushed the magnolia/bug-2003888-adding-edit-commit-msg branch from 5d3e39f to 2781bcf Compare June 22, 2026 19:09

Magnolia-Liu closed this Jun 22, 2026

Magnolia-Liu reopened this Jun 22, 2026

Magnolia-Liu force-pushed the magnolia/bug-2003888-adding-edit-commit-msg branch from 2781bcf to bce8475 Compare June 22, 2026 19:58

Magnolia-Liu closed this Jun 22, 2026

Magnolia-Liu reopened this Jun 22, 2026

Magnolia-Liu force-pushed the magnolia/bug-2003888-adding-edit-commit-msg branch from bce8475 to ee345d8 Compare June 22, 2026 20:28

github: add ability to edit commit message from PR in Lando UI (bug 2…

8af7584

…003888)

Magnolia-Liu force-pushed the magnolia/bug-2003888-adding-edit-commit-msg branch from a4fd00b to 8af7584 Compare June 23, 2026 17:51

		<button id="post-landing-job"
		class="button is-large is-loading post-landing-job has-text-white"

	def update_pull_request_content(
	def update_pull_request_description(

		<textarea readonly id="commit-title" class="textarea">{{ pull_request.title }}</textarea>
		<p class="help is-danger" id="commit-title-error"></p>

Conversation

Magnolia-Liu commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 14, 2026

Uh oh!

zzzeid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shtrom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shtrom Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zzzeid left a comment

Choose a reason for hiding this comment

Magnolia-Liu commented May 14, 2026 •

edited

Loading

shtrom Jun 9, 2026 •

edited

Loading