Skip to content

Preserve tree scroll position after file review actions #1196

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Open
Providence-o wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Preserve tree scroll position after file review actions #1196

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion airlock/static/assets/file_browser/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
// Only disable browser scroll restoration when there is a position to restore
if (sessionStorage.getItem("treeScrollTop") && 'scrollRestoration' in history) {
history.scrollRestoration = 'manual';
}
Copy link
Copy Markdown
Contributor

@rebkwok rebkwok May 20, 2026

Choose a reason for hiding this comment

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

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>");
  }
}

// keep the selected class up to date in the tree on the client side
function setTreeSelection(tree, event) {
// target here is the hx-get link that has been clicked on
Expand Down Expand Up @@ -155,4 +159,32 @@ if (document.readyState !== "loading") {

// Every time a datatable is rendered we need to update the checkboxes
// so they match the saved state
document.body.addEventListener("clusterize-table-updated", renderCheckboxStatus);
document.body.addEventListener("clusterize-table-updated", renderCheckboxStatus);

// Save scroll position before approve/request_changes form submits
document.body.addEventListener("submit", (event) => {
const form = event.target.closest("form");
if (!form) return;
const action = form.getAttribute("action") || "";
if (action.includes("/approve/") || action.includes("/request_changes/") || action.includes("/reset_review/")) {
Copy link
Copy Markdown
Contributor

@rebkwok rebkwok May 20, 2026

Choose a reason for hiding this comment

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

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.

sessionStorage.setItem("treeScrollTop", document.getElementById("tree-container").scrollTop);
}
});

// Restore scroll position on page load
document.addEventListener("DOMContentLoaded", () => {
const saved = sessionStorage.getItem("treeScrollTop");
const treeContainer = document.getElementById("tree-container");

if (saved) {
sessionStorage.removeItem("treeScrollTop");
treeContainer.style.visibility = "hidden";
Copy link
Copy Markdown
Contributor

@rebkwok rebkwok May 20, 2026

Choose a reason for hiding this comment

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

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.


// 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);
treeContainer.style.visibility = "visible";
}, 125);
}
Copy link
Copy Markdown
Contributor

@rebkwok rebkwok May 20, 2026

Choose a reason for hiding this comment

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

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);
  }

});