Skip to content

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

Open
AuDevTist1C wants to merge 1 commit into
Acode-Foundation:mainfrom
AuDevTist1C:refactor/file-browser
Open

refactor(fileBrowser): rewrite navigation history layer with event-driven NavStack and implement parent directory navigation#2500
AuDevTist1C wants to merge 1 commit into
Acode-Foundation:mainfrom
AuDevTist1C:refactor/file-browser

Conversation

@AuDevTist1C

@AuDevTist1C AuDevTist1C commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Context & Motivation

In the legacy file browser implementation, navigation state tracking was heavily coupled with side-effects, manual DOM manipulation, and visual layout updates. Directory historical states were stored in a basic array (state) inside local storage and local scopes, and was synchronized through complex manual array operations (such as .splice() loops and state array filtering). The logic to handle back navigation and forward rendering on index shifts depended on keeping temporary tracks of prior directories (lastUrl, lastName) and executing manual nested loops to traverse the DOM tree elements to clear obsolete visual buttons.

This tight coupling introduced several fragility factors:

  1. Race Conditions & Desynchronization: Slicing or mutating historical arrays manually made it easy for the visual breadcrumbs (navbar buttons) to drift out of sync with the true active folder state.
  2. Brittle Back-Navigation: Managing navigation events required passing arbitrary callback actions or manual back-navigation tracker arrays (backNavigation), which became excessively complicated to maintain when dealing with deeply nested virtual, remote (FTP/SFTP), and local directory structures.
  3. Hard-to-Maintain Boilerplate: Visual state cleanup on backward traversal required nested manual loops that explicitly inspected the last child element of the navigation bar, mixed with side effects writing directly to localStorage.
  4. Absence of O(1) History Validation: Verifying whether a path was already present in the active navigation history required scanning arrays manually, resulting in poor performance on deep hierarchies.

To resolve these architectural limitations, this refactor introduces a completely encapsulated, event-driven history management class called NavStack, which decouples the file browser's state mutations from its representation in the DOM. By treating the history stack as an independent state machine that emits custom events, visual components can independently bind to these events to update themselves automatically.

Additionally, this change introduces robust parent directory navigation support ("one-dir-up" via the classic .. item) and significantly hardens selection mode operations to prevent non-file actions from being selected.


Architectural Design and Structural Modifications

1. Introduction of the NavStack Class

The core architectural change is the implementation of a modern NavStack class that inherits from EventTarget. Moving to a native class pattern allows all navigation history properties and mutations to remain fully isolated, exposing a predictable API to the file browser:

  • Set-backed Array Tracking: The class maintains both a private array (#arr) of Location objects to preserve order and index-based retrieval, and a private Set (#urlSet) containing all active path URIs. This hybrid data structure provides immediate, O(1) checks for path membership (has(url)) while maintaining sequential index integrity.
  • Self-Sanitizing Stack Push: The .push() method ensures state duplicate avoidance. It normalizes incoming URIs, checks the internal Set for membership, and skips execution if the path is already registered. If the path is unique, it is added to both the array and the set, and dispatches a "push" CustomEvent.
  • Stateful Sequential Pop: The #popUntil() and .pop() procedures sequentially peel items from the top of the stack, cleanly removing them from the set-backing index and dispatching localized "pop" events containing the removed directory metadata. This allows listeners to act specifically on deleted paths as they are removed.
  • Relative Index Retrieval: The .get(i) method provides clean element lookups. It converts indexes using numeric evaluation, supports negative indexing (allowing quick retrieval of relative nodes, e.g., .get(-1) for active, and .get(-2) for parent nodes) and returns deep copies to prevent external mutations.
  • Event Delegation: By extending EventTarget, other parts of the system can easily register to state updates using shorthand methods .on() and .off().

2. Event-Driven UI Synchronization & Loose Coupling

With the introduction of NavStack, the file browser controller shifts from manual visual synchronization to an observer-pattern model. Inside the page inclusion logic, a global navStack instance is instantiated.

  • Push Synchronization Listener: Instead of manually building breadcrumb elements and assembling navigate callbacks during standard folder navigation, a centralized "push" listener handles the visual injection on the navigation bar. It identifies the immediate previous directory using .get(-2) and automatically sets up the recursive directory navigation action.
  • Pop Synchronization Listener: When a directory path is popped from the stack (whether during back button clicks or navigation index clicks), the "pop" listener automatically catches the event details, deletes the bound action stack reference, and removes the associated visual navbar element from the DOM using its unique ID hash (getNavId(url)).
  • Persistence Handlers: The storage of browser history in local storage is now cleanly bound directly to the stack's change events. If doesOpenLast is configured, saving to localStorage.fileBrowserState is executed automatically on "push" or "pop" occurrences, ensuring immediate durability.

3. Support for Parent Directory Navigation ("One Dir Up")

Screenshot_20260717-174212_Acode

To align with standard terminal and desktop file management paradigms, this change integrates a dedicated parent folder navigation item ".." as the first item in directory listings.

  • Prefixing List Entries: When executing directory reads inside getDir(), if the environment is a valid storage provider that supports listing files, a oneDirUp flag is toggled. Upon list load, util.pushFolder() creates a virtual item named ".." with custom flags (oneDirUp: true, notSelectable: true).
  • Array Reordering: The list modifications move the parent directory tile from the end of the loaded list to the absolute top of the index (list.unshift(list.pop())), ensuring the .. folder tile is always presented at the top of the folder list.
  • Navigation Routing: Clicking on a .. tile triggers a dedicated "oneDirUp" action case, which retrieves the second-to-last item on the stack via navStack.get(-2) and navigates to it, facilitating seamless upward navigation across both local and remote filesystems.

4. Hardening Selection Mode and Bypassing Interactive Tiles

In selection mode, users can perform batch operations on files and folders. However, helper tiles and navigation buttons are not target files. This commit introduces explicit safeguards to keep these tiles outside selection flows:

  • Not-Selectable Flagging: Tiles like the parent navigation directory .., the "Add a Storage" notification banner, and the "Select document" file browser action are injected with a notSelectable attribute.
  • Multi-Select Interception: In selection mode, during checkbox initialization or item selection, elements with data-not-selectable are explicitly skipped. This avoids rendering checkbox overlays on top of the parent directory .. or action lists, preventing invalid selections.
  • Click Prevention: When selection mode is active, the click handler immediately intercepts events targeted at non-selectable tiles, discarding the action and preventing accidental multi-select inclusions.
  • Long-Press Deletion Safety: Intercepts delete operations to ensure that attempting to delete or uninstall an item is ignored if the item represents parent navigation (isOneDirUp) or file explorer document action overlays.

5. Promise to Async/Await Modernization

To align with modern clean-code standards and reduce code complexity, structural processes like addStorage have been converted from traditional nested Promise chains (.then().catch()) to contemporary async/await block structures wrapped in robust try/catch clauses.


File-by-File Changes Description

1. src/pages/fileBrowser/fileBrowser.js

  • NavStack Definition: Added a complete class definition for NavStack at the top of the module, extending EventTarget and utilizing private fields (#urlSet, #arr) to achieve O(1) validation capabilities.
  • navStack Initialization: Removed the unstructured local variable state array and replaced it with an active navStack class instance.
  • Sync Logic Refactoring: Incorporated centralized listeners for the "push" and "pop" events emitted by the stack instance to cleanly drive DOM mutations on the navigation bar, handling actionStack actions dynamically.
  • Selection Interception: Added protective statements inside querySelectorAll loop filters to immediately return if the target tile possesses data-not-selectable.
  • Interception of Actions: Modified the global click listener on $content tiles to extract isOneDirUp from dataset attributes, route parent navigation actions into the "oneDirUp" execution block, and guard deletion checks against parent items.
  • Folder Listing Updates: Updated getDir() to track and append a virtual ".." directory tile, sorting list contents, and using .unshift() to enforce placing the directory at index 0.
  • navigate Method Simplification: Extensively simplified the core navigation routine, deleting nested DOM-removal loops, manual local storage updates, and manual arrays, replacing them with a single execution of navStack.popUntil() and navStack.push().
  • loadStates Structural Cleanup: Eliminated elaborate while-loops, back-navigation variables, and redundant state push functions in favor of sequential stack hydration via array shifts.
  • addStorage Rewrite: Migrated the storage addition handler to asynchronous try/catch scopes.

2. src/pages/fileBrowser/list.hbs

  • Template Directives: Added semantic Mustache tags representing {{#notSelectable}}data-not-selectable{{/notSelectable}} and {{#oneDirUp}}data-one-dir-up{{/oneDirUp}} properties inside the standard <li> file-list tiles. This binds the custom dataset attributes needed for Javascript click routing and selection exclusion checks.

Engineering Impact and System Stability

  1. Strict State Isolation: By consolidating navigation history into its own class, the file browser's memory footprint is more manageable, and tracking state shifts is straightforward.
  2. Visual and Data Decoupling: Removing coupled UI updates from standard navigation functions makes state transitions safe and consistent.
  3. Correctness in Multi-Selection: Preventing action tiles from participating in multi-select pipelines resolves bugs where system action tiles could be selected during batch operations.
  4. Improved Usability: Adding parent directory tiles (..) simplifies keyboard, mouse, and touch traversal, eliminating the need to rely exclusively on small back-navigation header icons.
  5. Performance Optimization: O(1) set tracking for active directories reduces lookups to a single hash map index evaluation.

(PR name and description are AI generated)

…iven `NavStack` and implement parent directory navigation

In the legacy file browser implementation, navigation state tracking was heavily coupled with side-effects, manual DOM manipulation, and visual layout updates. Directory historical states were stored in a basic array (`state`) inside local storage and local scopes, and was synchronized through complex manual array operations (such as `.splice()` loops and state array filtering). The logic to handle back navigation and forward rendering on index shifts depended on keeping temporary tracks of prior directories (`lastUrl`, `lastName`) and executing manual nested loops to traverse the DOM tree elements to clear obsolete visual buttons.

This tight coupling introduced several fragility factors:
1. **Race Conditions & Desynchronization**: Slicing or mutating historical arrays manually made it easy for the visual breadcrumbs (navbar buttons) to drift out of sync with the true active folder state.
2. **Brittle Back-Navigation**: Managing navigation events required passing arbitrary callback actions or manual back-navigation tracker arrays (`backNavigation`), which became excessively complicated to maintain when dealing with deeply nested virtual, remote (FTP/SFTP), and local directory structures.
3. **Hard-to-Maintain Boilerplate**: Visual state cleanup on backward traversal required nested manual loops that explicitly inspected the last child element of the navigation bar, mixed with side effects writing directly to `localStorage`.
4. **Absence of O(1) History Validation**: Verifying whether a path was already present in the active navigation history required scanning arrays manually, resulting in poor performance on deep hierarchies.

To resolve these architectural limitations, this refactor introduces a completely encapsulated, event-driven history management class called `NavStack`, which decouples the file browser's state mutations from its representation in the DOM. By treating the history stack as an independent state machine that emits custom events, visual components can independently bind to these events to update themselves automatically.

Additionally, this change introduces robust parent directory navigation support ("one-dir-up" via the classic `..` item) and significantly hardens selection mode operations to prevent non-file actions from being selected.

---

The core architectural change is the implementation of a modern `NavStack` class that inherits from `EventTarget`. Moving to a native class pattern allows all navigation history properties and mutations to remain fully isolated, exposing a predictable API to the file browser:
* **Set-backed Array Tracking**: The class maintains both a private array (`#arr`) of Location objects to preserve order and index-based retrieval, and a private Set (`#urlSet`) containing all active path URIs. This hybrid data structure provides immediate, O(1) checks for path membership (`has(url)`) while maintaining sequential index integrity.
* **Self-Sanitizing Stack Push**: The `.push()` method ensures state duplicate avoidance. It normalizes incoming URIs, checks the internal `Set` for membership, and skips execution if the path is already registered. If the path is unique, it is added to both the array and the set, and dispatches a `"push"` CustomEvent.
* **Stateful Sequential Pop**: The `#popUntil()` and `.pop()` procedures sequentially peel items from the top of the stack, cleanly removing them from the set-backing index and dispatching localized `"pop"` events containing the removed directory metadata. This allows listeners to act specifically on deleted paths as they are removed.
* **Relative Index Retrieval**: The `.get(i)` method provides clean element lookups. It converts indexes using numeric evaluation, supports negative indexing (allowing quick retrieval of relative nodes, e.g., `.get(-1)` for active, and `.get(-2)` for parent nodes) and returns deep copies to prevent external mutations.
* **Event Delegation**: By extending `EventTarget`, other parts of the system can easily register to state updates using shorthand methods `.on()` and `.off()`.

With the introduction of `NavStack`, the file browser controller shifts from manual visual synchronization to an observer-pattern model. Inside the page inclusion logic, a global `navStack` instance is instantiated.
* **Push Synchronization Listener**: Instead of manually building breadcrumb elements and assembling navigate callbacks during standard folder navigation, a centralized `"push"` listener handles the visual injection on the navigation bar. It identifies the immediate previous directory using `.get(-2)` and automatically sets up the recursive directory navigation action.
* **Pop Synchronization Listener**: When a directory path is popped from the stack (whether during back button clicks or navigation index clicks), the `"pop"` listener automatically catches the event details, deletes the bound action stack reference, and removes the associated visual navbar element from the DOM using its unique ID hash (`getNavId(url)`).
* **Persistence Handlers**: The storage of browser history in local storage is now cleanly bound directly to the stack's change events. If `doesOpenLast` is configured, saving to `localStorage.fileBrowserState` is executed automatically on `"push"` or `"pop"` occurrences, ensuring immediate durability.

To align with standard terminal and desktop file management paradigms, this change integrates a dedicated parent folder navigation item `".."` as the first item in directory listings.
* **Prefixing List Entries**: When executing directory reads inside `getDir()`, if the environment is a valid storage provider that supports listing files, a `oneDirUp` flag is toggled. Upon list load, `util.pushFolder()` creates a virtual item named `".."` with custom flags (`oneDirUp: true`, `notSelectable: true`).
* **Array Reordering**: The list modifications move the parent directory tile from the end of the loaded list to the absolute top of the index (`list.unshift(list.pop())`), ensuring the `..` folder tile is always presented at the top of the folder list.
* **Navigation Routing**: Clicking on a `..` tile triggers a dedicated `"oneDirUp"` action case, which retrieves the second-to-last item on the stack via `navStack.get(-2)` and navigates to it, facilitating seamless upward navigation across both local and remote filesystems.

In selection mode, users can perform batch operations on files and folders. However, helper tiles and navigation buttons are not target files. This commit introduces explicit safeguards to keep these tiles outside selection flows:
* **Not-Selectable Flagging**: Tiles like the parent navigation directory `..`, the "Add a Storage" notification banner, and the "Select document" file browser action are injected with a `notSelectable` attribute.
* **Multi-Select Interception**: In selection mode, during checkbox initialization or item selection, elements with `data-not-selectable` are explicitly skipped. This avoids rendering checkbox overlays on top of the parent directory `..` or action lists, preventing invalid selections.
* **Click Prevention**: When selection mode is active, the click handler immediately intercepts events targeted at non-selectable tiles, discarding the action and preventing accidental multi-select inclusions.
* **Long-Press Deletion Safety**: Intercepts delete operations to ensure that attempting to delete or uninstall an item is ignored if the item represents parent navigation (`isOneDirUp`) or file explorer document action overlays.

To align with modern clean-code standards and reduce code complexity, structural processes like `addStorage` have been converted from traditional nested Promise chains (`.then().catch()`) to contemporary `async`/`await` block structures wrapped in robust `try`/`catch` clauses.

---

* **`NavStack` Definition**: Added a complete class definition for `NavStack` at the top of the module, extending `EventTarget` and utilizing private fields (`#urlSet`, `#arr`) to achieve O(1) validation capabilities.
* **`navStack` Initialization**: Removed the unstructured local variable `state` array and replaced it with an active `navStack` class instance.
* **Sync Logic Refactoring**: Incorporated centralized listeners for the `"push"` and `"pop"` events emitted by the stack instance to cleanly drive DOM mutations on the navigation bar, handling `actionStack` actions dynamically.
* **Selection Interception**: Added protective statements inside `querySelectorAll` loop filters to immediately return if the target tile possesses `data-not-selectable`.
* **Interception of Actions**: Modified the global click listener on `$content` tiles to extract `isOneDirUp` from dataset attributes, route parent navigation actions into the `"oneDirUp"` execution block, and guard deletion checks against parent items.
* **Folder Listing Updates**: Updated `getDir()` to track and append a virtual `".."` directory tile, sorting list contents, and using `.unshift()` to enforce placing the directory at index `0`.
* **`navigate` Method Simplification**: Extensively simplified the core navigation routine, deleting nested DOM-removal loops, manual local storage updates, and manual arrays, replacing them with a single execution of `navStack.popUntil()` and `navStack.push()`.
* **`loadStates` Structural Cleanup**: Eliminated elaborate while-loops, back-navigation variables, and redundant state push functions in favor of sequential stack hydration via array shifts.
* **`addStorage` Rewrite**: Migrated the storage addition handler to asynchronous `try/catch` scopes.

* **Template Directives**: Added semantic Mustache tags representing `{{#notSelectable}}data-not-selectable{{/notSelectable}}` and `{{#oneDirUp}}data-one-dir-up{{/oneDirUp}}` properties inside the standard `<li>` file-list tiles. This binds the custom dataset attributes needed for Javascript click routing and selection exclusion checks.

---

1.  **Strict State Isolation**: By consolidating navigation history into its own class, the file browser's memory footprint is more manageable, and tracking state shifts is straightforward.
2.  **Visual and Data Decoupling**: Removing coupled UI updates from standard navigation functions makes state transitions safe and consistent.
3.  **Correctness in Multi-Selection**: Preventing action tiles from participating in multi-select pipelines resolves bugs where system action tiles could be selected during batch operations.
4.  **Improved Usability**: Adding parent directory tiles (`..`) simplifies keyboard, mouse, and touch traversal, eliminating the need to rely exclusively on small back-navigation header icons.
5.  **Performance Optimization**: O(1) set tracking for active directories reduces lookups to a single hash map index evaluation.

(AI generated commit message)
@greptile-apps

greptile-apps Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR replaces the file browser's manual navigation-state array with an event-driven NavStack class (extending EventTarget) and adds a .. parent-directory entry to every non-root directory listing. The navigate / loadStates functions are rewritten to use NavStack push/pop events to drive both the breadcrumb navbar and the Android back-button actionStack.

  • NavStack encapsulates navigation history with deduplication, popUntil, and toJSON for localStorage persistence; push/pop CustomEvents allow decoupled listeners for the UI layer.
  • getDir now appends a .. tile (with data-one-dir-up and data-not-selectable) to the sorted file list for every non-root directory; clicking it reads navStack.get(-2) and calls navigate to step back one history level.
  • loadStates is significantly simplified by pushing all stored states into NavStack in a loop, relying on the event handlers to rebuild the navbar; however, the old defensive name-fallback (lastState.name || Url.basename(url) || url) is no longer present.

Confidence Score: 3/5

The core navigation rewrite is mostly sound, but loadStates loses a defensive fallback that could silently fail to restore the last browsed directory on open.

The navigate function throws when name is falsy, but loadStates now passes the raw stored name directly without a fallback. Any stored state with a missing name produces an unhandled Promise rejection, leaving the file browser with a populated navbar but a blank content pane. The old code used Url.basename(url) as a safety net in exactly this situation.

src/pages/fileBrowser/fileBrowser.js — particularly the loadStates function and the oneDirUp click-handler path.

Important Files Changed

Filename Overview
src/pages/fileBrowser/fileBrowser.js Core rewrite of navigation history: replaces the manual state array with a new event-driven NavStack class, adds a ".." entry to non-root directory listings, and refactors loadStates/navigate. Contains a regression (missing name fallback in loadStates) and several style/logic concerns.
src/pages/fileBrowser/list.hbs Adds two new optional boolean data-attributes (data-not-selectable, data-one-dir-up) to the tile template; straightforward and correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A([User opens FileBrowser]) --> B{doesOpenLast && storedState?}
    B -- Yes --> C[loadStates: push all saved states to navStack]
    C --> C1[navStack.push fires push event for each state]
    C1 --> D[navigate to last state URL]
    B -- No --> E[navigate to root]
    D --> F{navStack.has url?}
    E --> F
    F -- Yes --> G[navStack.popUntil url]
    G --> H[getDir + render]
    F -- No --> I[getDir url, name]
    I --> J{url === root?}
    J -- Yes --> K[listAllStorages]
    J -- No --> L[fs.lsDir + prepend .. entry]
    K --> N[return dir object]
    L --> N
    N --> O[navStack.push]
    O --> P[pushToNavbar + actionStack back action]
    P --> H
    H --> Q[render directory]
    Q --> R{User action}
    R -- Clicks .. --> S[oneDirUp: navStack.get minus 2]
    R -- Hardware back --> T[actionStack triggers prev dir]
    R -- Navbar breadcrumb --> U[navigate to breadcrumb URL]
    S --> F
    T --> F
    U --> F
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A([User opens FileBrowser]) --> B{doesOpenLast && storedState?}
    B -- Yes --> C[loadStates: push all saved states to navStack]
    C --> C1[navStack.push fires push event for each state]
    C1 --> D[navigate to last state URL]
    B -- No --> E[navigate to root]
    D --> F{navStack.has url?}
    E --> F
    F -- Yes --> G[navStack.popUntil url]
    G --> H[getDir + render]
    F -- No --> I[getDir url, name]
    I --> J{url === root?}
    J -- Yes --> K[listAllStorages]
    J -- No --> L[fs.lsDir + prepend .. entry]
    K --> N[return dir object]
    L --> N
    N --> O[navStack.push]
    O --> P[pushToNavbar + actionStack back action]
    P --> H
    H --> Q[render directory]
    Q --> R{User action}
    R -- Clicks .. --> S[oneDirUp: navStack.get minus 2]
    R -- Hardware back --> T[actionStack triggers prev dir]
    R -- Navbar breadcrumb --> U[navigate to breadcrumb URL]
    S --> F
    T --> F
    U --> F
Loading

Reviews (1): Last reviewed commit: "refactor(fileBrowser): rewrite navigatio..." | Re-trigger Greptile

Comment on lines 1769 to 1774
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);

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.

Comment on lines +1106 to +1111
case "oneDirUp": {
const dir = navStack.get(-2);
if (!dir) break;
const { url, name } = dir;
navigate(url, name);
}

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 1016 to +1017
if (isSelectionMode) {
if ($el.dataset.notSelectable != null) return;

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.

Comment on lines +1106 to +1111
case "oneDirUp": {
const dir = navStack.get(-2);
if (!dir) break;
const { url, name } = dir;
navigate(url, name);
}

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

1 participant