Preserve tree scroll position after file review actions

[Providence-o]

Closes: #906

When reviewing files in a large request, clicking approve/request changes (and undoing these actions) would redirect back to the same file but reset the file tree scroll position to the top.

This fix saves the tree scroll bar position before the form submits and restores it after the page reloads.

There is no test added for this change because the scroll bar position restoration is dependent on a hard-coded delay, making it prone to flakiness.

[rebkwok]

I apologise for getting Claude to review this (but it is JS, in my defence). I think the approach is good, some suggestions that I think make the UX slightly better and the code less brittle.

I think this should be testable with a playwright test (I've had some success with getting Claude to write those)

[rebkwok]

There's currently a slightly disconcerting jump in the tree when you click an approve/reject etc button - the tree appears scrolled to the top and then jumps to the remembered position. If we modify this slightly we can hide the tree here, and then unhide it in the restore code below. That means that there's a brief period where the tree isn't visible at all as the page loads, but I think it's a better UX than the jumping.

if (sessionStorage.getItem("treeScrollTop")) {
  if ('scrollRestoration' in history) {
    history.scrollRestoration = 'manual';
    document.head.insertAdjacentHTML("beforeend", "<style id='scroll-restore-style'>#tree-container{visibility:hidden}</style>");
  }
}

[rebkwok]

To unhide the tree now, we'd change this to:

if (saved) {
    sessionStorage.removeItem("treeScrollTop");
    // Wait for browser scroll restoration to complete before applying saved position.
    // The browser may reset scrollTop after DOMContentLoaded, so we delay slightly.
    setTimeout(() => {
      treeContainer.scrollTop = parseInt(saved, 10);
      document.getElementById("scroll-restore-style")?.remove();
    }, 125);
  }

[rebkwok]

I think this is intended to do what I've suggested, hiding the tree altogether first and then unhiding it after the scroll restoration. But when I test it locally, I see the tree at the top position flash briefly.
Claude says this is because DOMContentLoaded fires too late.

Here's the timing:

When the browser encounters <script src="index.js">, it pauses HTML parsing and
fetches the file over the network. During that fetch, the browser may paint
whatever it has already parsed — including #tree-container, visible at the wrong
scroll position. That's the first paint, and it's done before the script even runs.

By the time the script executes, fires its DOMContentLoaded listener, and sets
visibility: hidden, the user has already seen that first frame.

The style injection at the top of index.js works because it runs synchronously
during script execution — before the browser gets its next paint opportunity. The
browser batches paints; it won't paint again until the current JS task finishes. So
the tree is hidden before the next frame, and stays hidden through the scroll
restoration until the timeout reveals it at the right position.

[rebkwok]

Do we need to check for the action? Could we cjust check that there's a tree present and assume we want to do this? We've already confirmed that there's a form that could be submitted. i.e.

const tree = document.getElementById("tree-container");                    
  if (tree) sessionStorage.setItem("treeScrollTop", tree.scrollTop); 

That has the added bonus of maintaining the scroll position if you're in a workspace with lots of groups, and you submit the context/controls or comments forms, and if we ever change/add actions for that form, it'll continue to work.

[rebkwok]

Since I've got Claude looking at this anyway, here's a playwright test it wrote for me:

@pytest.mark.parametrize(
    "button_id",
    ["#file-approve-button", "#file-request-changes-button"],
)
def test_tree_scroll_preserved_after_file_review_action(
    live_server, context, page, button_id
):
    login_as_user(
        live_server,
        context,
        user_dict=factories.create_api_user(
            username="checker", output_checker=True
        ),
    )
    release_request = factories.create_request_at_status(
        "workspace",
        status=RequestStatus.SUBMITTED,
        files=[
            factories.request_file(group="group", path=f"file{i:02d}.txt")
            for i in range(30)
        ],
    )

    page.goto(live_server.url + release_request.get_url("group/file00.txt"))

    assert page.evaluate("document.getElementById('tree-container').scrollTop") == 0
    page.evaluate("document.getElementById('tree-container').scrollTop = 200")
    scroll_before = page.evaluate("document.getElementById('tree-container').scrollTop")
    assert scroll_before > 0

    page.locator(button_id).click()
    page.wait_for_load_state("load")
    # Tree is hidden during scroll restoration; wait for it to become visible
    expect(page.locator("#tree-container")).to_be_visible()

    scroll_after = page.evaluate("document.getElementById('tree-container').scrollTop")
    assert scroll_after == scroll_before