Skip to content

refactor(fileBrowser): rewrite navigation history layer with event-driven NavStack and implement parent directory navigation #2500

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
AuDevTist1C wants to merge 1 commit into
base: main
Choose a base branch
from
+156 −90
Open

refactor(fileBrowser): rewrite navigation history layer with event-driven NavStack and implement parent directory navigation #2500

Show file tree
Hide file tree
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
244 changes: 154 additions & 90 deletions src/pages/fileBrowser/fileBrowser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,84 @@ import util from "./util";
* @typedef {{url: String, name: String}} Location
*/

class NavStack extends EventTarget {
/** @type {Set<string>} */
#urlSet = new Set();
/** @type {Array<Location>} */
#arr = [];
/**
* @param {Location}
*/
push({ url, name }) {
url = `${url ?? ""}`;
const urlSet = this.#urlSet;
if (!url || urlSet.has(url)) return;
urlSet.add(url);
this.#arr.push({ url, name });
this.dispatchEvent(
new CustomEvent("push", {
detail: { url, name },
}),
);
}
/**
* @param {string} [url]
*/
#popUntil(url) {
const urlSet = this.#urlSet;
const arr = this.#arr;
for (let i = arr.length - 1; i >= 0; i--) {
const item = arr[i];
const url2 = item.url;
if (url && url === url2) return;
this.#urlSet.delete(url2);
arr.length = i;
this.dispatchEvent(
new CustomEvent("pop", {
detail: { url: url2, name: item.name },
}),
);
if (!url) return;
}
}
popUntil(url) {
if ((url = `${url ?? ""}`)) return this.#popUntil(url);
throw new TypeError('popUntil(url): a non-empty "url" is required.');
}
pop() {
return this.#popUntil();
}
/**
* @param {number} [i]
* @returns {Location}
*/
get(i) {
if ((i = +i) !== i) return;
const arr = this.#arr;
const l = arr.length;
if (i < 0) i += l;
if (i < 0 || i > l - 1) return;
return { ...arr[i] };
}
has(url) {
return this.#urlSet.has(`${url ?? ""}`);
}
/** @returns {number} */
get length() {
return this.#arr.length;
}
/** @returns {Array<Location>} */
toJSON() {
return Array.from(this.#arr);
}
on() {
return this.addEventListener(...arguments);
}
off() {
return this.removeEventListener(...arguments);
}
}

/**
* @typedef Storage
* @property {String} name
Expand All @@ -57,16 +135,23 @@ import util from "./util";
function FileBrowserInclude(mode, info, doesOpenLast = true) {
mode = mode || "file";

const navStack = new NavStack();
const IS_FOLDER_MODE = ["folder", "both"].includes(mode);
const IS_FILE_MODE = ["file", "both"].includes(mode);
const storedState = helpers.parseJSON(localStorage.fileBrowserState) || [];
/**@type {Array<Location>} */
const state = [];
/**@type {Array<Storage>} */
const allStorages = [];
let storageList = helpers.parseJSON(localStorage.storageList);
if (!Array.isArray(storageList)) storageList = [];

if (doesOpenLast) {
const saveFileBrowserState = () => {
localStorage.fileBrowserState = JSON.stringify(navStack);
};
navStack.on("push", saveFileBrowserState);
navStack.on("pop", saveFileBrowserState);
}

let isSelectionMode = false;
let isPasting = false;
let selectedItems = new Set();
Expand Down Expand Up @@ -673,6 +758,22 @@ function FileBrowserInclude(mode, info, doesOpenLast = true) {
document.removeEventListener("resume", reload);
};

navStack.addEventListener("pop", (ev) => {
const { url } = ev.detail;
actionStack.remove(url);
tag.get(`#${getNavId(url)}`)?.remove();
});
navStack.addEventListener("push", (ev) => {
let action;
const prevDir = navStack.get(-2);
if (prevDir) {
const { name, url } = prevDir;
action = () => navigate(url, name);
}
const dir = ev.detail;
pushToNavbar(dir.name, dir.url, action);
});

if (doesOpenLast && storedState.length) {
loadStates(storedState);
return;
Expand Down Expand Up @@ -860,6 +961,7 @@ function FileBrowserInclude(mode, info, doesOpenLast = true) {
$list
.querySelectorAll(".tile:not(.selection-header)")
.forEach((item) => {
if (item.dataset.notSelectable != null) return;
const checkbox = Checkbox("", false);
checkbox.onclick = () => {
const url = item.querySelector("data-url").textContent;
Expand Down Expand Up @@ -912,6 +1014,7 @@ function FileBrowserInclude(mode, info, doesOpenLast = true) {
const $el = e.target;

if (isSelectionMode) {
if ($el.dataset.notSelectable != null) return;
Comment on lines 1016 to +1017

@greptile-apps greptile-apps Bot Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 data-not-selectable guard misses clicks on child elements

$el is e.target, which can be the icon <span> or text <div> inside the <li> rather than the <li> itself. Neither child carries data-not-selectable, so $el.dataset.notSelectable != null evaluates to false and the early-return guard is skipped. The .querySelector(".input-checkbox") fallback happens to save correctness here (notSelectable items never get a checkbox added), but the safety relies on that second check rather than the explicit guard. Replacing with $el.closest(".tile")?.dataset.notSelectable != null would make the check authoritative for any click target depth.

const checkbox = $el.closest(".tile")?.querySelector(".input-checkbox");
if (checkbox && !$el.closest(".selection-header")) {
checkbox.checked = !checkbox.checked;
Expand All @@ -934,7 +1037,8 @@ function FileBrowserInclude(mode, info, doesOpenLast = true) {

let url = $el.dataset.url;
let name = $el.dataset.name || $el.getAttribute("name");
const idOpenDoc = $el.hasAttribute("open-doc");
const isOneDirUp = $el.dataset.oneDirUp != null;
const isOpenDoc = $el.hasAttribute("open-doc");
const uuid = $el.getAttribute("uuid");
const type = $el.getAttribute("type");
const storageType = $el.getAttribute("storageType");
Expand All @@ -960,7 +1064,14 @@ function FileBrowserInclude(mode, info, doesOpenLast = true) {
return;
}

if (!url && action === "open" && isDir && !idOpenDoc && !isContextMenu) {
if (
!url &&
action === "open" &&
isDir &&
!isOpenDoc &&
!isOneDirUp &&
!isContextMenu
) {
loader.hide();
util.addPath(name, uuid).then((res) => {
const storage = allStorages.find((storage) => storage.uuid === uuid);
Expand All @@ -975,7 +1086,8 @@ function FileBrowserInclude(mode, info, doesOpenLast = true) {
}

if (isContextMenu) action = "contextmenu";
else if (idOpenDoc) action = "open-doc";
else if (isOpenDoc) action = "open-doc";
else if (isOneDirUp) action = "oneDirUp";

switch (action) {
case "navigation":
Expand All @@ -991,6 +1103,12 @@ function FileBrowserInclude(mode, info, doesOpenLast = true) {
case "open-doc":
openDoc();
break;
case "oneDirUp": {
const dir = navStack.get(-2);
if (!dir) break;
const { url, name } = dir;
navigate(url, name);
}
Comment on lines +1106 to +1111

@greptile-apps greptile-apps Bot Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 .. resolves to navigation-history parent, not the filesystem parent

navStack.get(-2) returns the previously-visited directory, not the actual URL-parent of the current directory. These are the same in linear navigation, but diverge in edge cases — e.g. if a future feature adds bookmarks or deep-links that push multiple levels to navStack at once (like loadStates already does). In that scenario pressing .. could land on a directory that is not an ancestor of the current one at all. The traditional expected behaviour of .. is Url.dirname(currentDir.url). Consider adding a clarifying comment or computing the real parent as a fallback.

Comment on lines +1106 to +1111

@greptile-apps greptile-apps Bot Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Missing break at end of oneDirUp case

The oneDirUp block has no trailing break. While this is currently safe because it is the last case, future additions to the switch will silently fall through into the new case without any visible indication that the omission is intentional. Adding break makes the intent explicit and future-proof.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

}

async function folder() {
Expand Down Expand Up @@ -1046,7 +1164,7 @@ function FileBrowserInclude(mode, info, doesOpenLast = true) {
if (appSettings.value.vibrateOnTap) {
navigator.vibrate(config.VIBRATION_TIME);
}
if ($el.getAttribute("open-doc") === "true") return;
if (isOneDirUp || isOpenDoc) return;

const deleteText =
currentDir.url === "/" ? strings.remove : strings.delete;
Expand Down Expand Up @@ -1394,12 +1512,14 @@ function FileBrowserInclude(mode, info, doesOpenLast = true) {
util.pushFolder(allStorages, strings["add a storage"], "", {
storageType: "notification",
uuid: "addstorage",
notSelectable: true,
});
}

if (IS_FILE_MODE) {
util.pushFolder(allStorages, "Select document", null, {
"open-doc": true,
notSelectable: true,
});
}

Expand All @@ -1416,6 +1536,7 @@ function FileBrowserInclude(mode, info, doesOpenLast = true) {
const { fileBrowser } = appSettings.value;
let list = [];
let error = false;
let oneDirUp = false;

if (url in cachedDir) {
return cachedDir[url];
Expand All @@ -1440,6 +1561,7 @@ function FileBrowserInclude(mode, info, doesOpenLast = true) {
const fs = fsOperation(url);
try {
list = (await fs.lsDir()) ?? [];
oneDirUp = true;
} catch (err) {
if (progress[id]) {
helpers.error(err, url);
Expand All @@ -1455,11 +1577,19 @@ function FileBrowserInclude(mode, info, doesOpenLast = true) {
loader.destroy();
}
if (error) return null;
list = helpers.sortDir(list, fileBrowser, mode);
if (oneDirUp) {
util.pushFolder(list, "..", null, {
oneDirUp,
notSelectable: true,
});
list.unshift(list.pop());
}
return {
url,
name,
scroll: 0,
list: helpers.sortDir(list, fileBrowser, mode),
list,
};
}
}
Expand All @@ -1469,7 +1599,7 @@ function FileBrowserInclude(mode, info, doesOpenLast = true) {
* @param {String} url
* @param {String} name
*/
async function navigate(url, name, assignBackButton = true) {
async function navigate(url, name) {
if (document.getElementById("search-bar")) {
hideSearchBar();
}
Expand All @@ -1481,48 +1611,19 @@ function FileBrowserInclude(mode, info, doesOpenLast = true) {
throw new Error('navigate(url, name): "name" is required.');
}

if (url === "/") {
if (IS_FOLDER_MODE) $openFolder.disabled = true;
} else {
if (IS_FOLDER_MODE) $openFolder.disabled = false;
}

const $nav = tag.get(`#${getNavId(url)}`);
if (IS_FOLDER_MODE) $openFolder.disabled = url === "/";

//If navigate to previous directories, clear the rest navigation
if ($nav) {
let $topNav;
while (($topNav = $navigation.lastChild) !== $nav) {
const url = $topNav.dataset.url;
actionStack.remove(url);
$topNav.remove();
}

while (1) {
const location = state.slice(-1)[0];
if (!location || location.url === url) break;
state.pop();
}
localStorage.fileBrowserState = JSON.stringify(state);

if (navStack.has(url)) {
navStack.popUntil(url);
const dir = await getDir(url, name);
if (dir) {
render(dir);
}
if (dir) render(dir);
return;
}

const dir = await getDir(url, name);
if (dir) {
const { url: curl, name: cname } = currentDir;
let action;
if (doesOpenLast) pushState({ name, url });
if (curl && cname && assignBackButton) {
action = () => {
navigate(curl, cname, false);
};
}
pushToNavbar(name, url, action);
navStack.push({ name, url });
render(dir);
}
}
Expand Down Expand Up @@ -1667,36 +1768,9 @@ function FileBrowserInclude(mode, info, doesOpenLast = true) {
*/
function loadStates(states) {
if (!Array.isArray(states) || !states.length) return;

const backNavigation = [];
const lastState = states.pop();
if (!lastState || !lastState.url) return;
const { url } = lastState;
const name = lastState.name || Url.basename(url) || url;
let { url: lastUrl, name: lastName } = currentDir;

while (states.length) {
const location = states.splice(0, 1)[0];
if (!location || !location.url) {
continue;
}
const { url, name } = location;
let action;

if (doesOpenLast) pushState({ name, url });
if (lastUrl && lastName) {
backNavigation.push([lastUrl, lastName]);
action = () => {
const [url, name] = backNavigation.pop();
navigate(url, name, false);
};
}
pushToNavbar(name, url, action);
lastUrl = url;
lastName = name;
}

currentDir = { url: lastUrl, name: lastName };
while (states.length) navStack.push(states.shift());
currentDir = navStack.get(-1);
const { url, name } = currentDir;
navigate(url, name);
Comment on lines 1769 to 1774

@greptile-apps greptile-apps Bot Jul 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 loadStates silently drops the last stored directory when name is falsy

The old implementation had a defensive fallback: const name = lastState.name || Url.basename(url) || url. The new code calls navStack.get(-1).name directly and passes it to navigate, which throws synchronously if name is falsy. Because loadStates does not await or .catch() the call, the resulting Promise rejection is unhandled — the navbar is populated (via the push-event loop) but the final directory is never rendered and its content never shown. Any stored state object whose name was somehow saved as null, "", or undefined silently breaks restore-on-open.

}

Expand Down Expand Up @@ -1769,27 +1843,17 @@ function FileBrowserInclude(mode, info, doesOpenLast = true) {
navigate(url, name);
}

function pushState({ url, name }) {
if (!url || !name) return;
if (state.find((l) => l.url === url)) return;
state.push({ url, name });
localStorage.fileBrowserState = JSON.stringify(state);
}

/**
* Adds a new storage and refresh location
*/
function addStorage() {
util
.addPath()
.then((res) => {
storageList.push(res);
localStorage.storageList = JSON.stringify(storageList);
reload();
})
.catch((err) => {
helpers.error(err);
});
async function addStorage() {
try {
storageList.push(await util.addPath());
localStorage.storageList = JSON.stringify(storageList);
reload();
} catch (err) {
helpers.error(err);
}
}
});
}
Expand Down
Loading