refactor(file-browser): Overhaul navigation state, skeleton rendering, and async safety - #2500
refactor(file-browser): Overhaul navigation state, skeleton rendering, and async safety #2500AuDevTist1C wants to merge 7 commits into
Conversation
Greptile SummaryThis PR overhauled the
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
actor User
participant navigate
participant NavStack
participant renderCurrentDir
participant getDirList
participant DOM
User->>navigate: tap directory tile
navigate->>NavStack: has(url)?
alt url already in stack
navigate->>NavStack: popUntil(url)
NavStack-->>DOM: pop events → remove navbar items
else new url
navigate->>NavStack: push(url, name)
NavStack-->>DOM: push event → add navbar item + actionStack entry
end
navigate->>renderCurrentDir: (no args)
renderCurrentDir->>renderCurrentDir: abort previous _rndrAbortCtrl
renderCurrentDir->>renderCurrentDir: create new AbortController
renderCurrentDir->>DOM: remove old $list, append empty $list + skeleton items
renderCurrentDir->>getDirList: await getDirList(url)
alt "url === "/""
getDirList->>getDirList: await listAllStorages()
else FTP/SFTP
getDirList->>getDirList: await lsDir() [NO TIMEOUT]
else other protocol
getDirList->>getDirList: Promise.race([lsDir(), 10s timeout])
end
getDirList-->>renderCurrentDir: list[] or throws
alt abortSignal.aborted
renderCurrentDir-->>DOM: return (stale render discarded)
else list is truthy
renderCurrentDir->>DOM: replaceChildren(oneDirUp + items)
renderCurrentDir->>renderCurrentDir: cachedDir.set(url, dir)
else list undefined (error)
renderCurrentDir->>DOM: replaceChildren(oneDirUp + error placeholder)
else list empty []
renderCurrentDir->>DOM: replaceChildren(oneDirUp + empty-folder placeholder)
renderCurrentDir->>renderCurrentDir: cachedDir.set(url, dir)
end
Reviews (24): Last reviewed commit: "feat: Render explicit empty directory an..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| case "oneDirUp": { | ||
| const dir = navStack.get(-2); | ||
| if (!dir) break; | ||
| const { url, name } = dir; | ||
| navigate(url, name); | ||
| } |
There was a problem hiding this comment.
.. 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.
| case "oneDirUp": { | ||
| const dir = navStack.get(-2); | ||
| if (!dir) break; | ||
| const { url, name } = dir; | ||
| navigate(url, name); | ||
| } |
There was a problem hiding this comment.
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!
682762f to
b7687ee
Compare
This comment was marked as outdated.
This comment was marked as outdated.
08ccd4c to
105dc80
Compare
003df66 to
b513169
Compare
This comment was marked as outdated.
This comment was marked as outdated.
b513169 to
8bb4dc0
Compare
|
(Edit: pushed) Recording.at.July21-064140pm.2.mp4 |
AuDevTist1C
changed the title
NavStack and implement parent directory navigation
8bb4dc0 to
d8e4881
Compare
| if (checkbox) { | ||
| checkbox.checked = checked; | ||
| const url = item.querySelector("data-url").textContent; | ||
| if (checked) { | ||
| selectedItems.add(url); | ||
| } else { | ||
| selectedItems.delete(url); | ||
| } | ||
| selectedItems[checked ? "add" : "delete"](url); | ||
| } | ||
| }); | ||
| updateSelectionCount($count); |
There was a problem hiding this comment.
When checked = false (deselect all), selectedItems.delete(url) is called only for items that have a rendered checkbox. Any URL already in selectedItems before "Select All" was clicked is not removed, leaving phantom entries that pollute the next delete/copy/compress batch operation. A selectedItems.clear() before the deselect loop would make the deselect path symmetric with the select path.
96bbb2e to
e640f65
Compare
1cfe31e to
80872d4
Compare
16dac10 to
38b6442
Compare
|
@greptile some changes were removed from this PR, and I've opened separate PRs for some of them. |
|
Thanks for the heads up! That makes sense — keeping the syntax/control flow modernization (Commit 5) in a separate PR makes the diff easier to review in isolation. If you'd like me to take a look at those separate PRs when they're ready, feel free to tag me. |
|
Thanks for the links! Here's a quick overview of the four spin-off PRs:
Good call splitting these out — they're much easier to review independently. A few notes per PR: #2543 — Clean and self-contained. The #2545 — The #2546 — Solid. The #2548 — The |
AuDevTist1C
changed the title
38b6442 to
29255a5
Compare
|
Good to know — thanks for the heads up. That said, there's a reconciliation issue worth noting between the two PRs: PR #2545 modifies Additionally, #2545's selection guards check for
Either way, these two PRs will need to be coordinated before merging. |
|
@greptile there's already |
|
You're right that That's exactly the gap: the JS side sets the flag, but the template doesn't output it, so any selection-mode guard that reads The fix is a one-liner in With that added, |
5a913dd to
b561ec5
Compare
788df88 to
7717fb2
Compare
Isolate the file system rename operation into its own dedicated commit to preserve continuous history tracking within Git. Renaming `list.hbs` to `listItem.hbs` without altering any content ensures Git records this change as a 100% file rename. Decoupling this step prevents Git from misinterpreting subsequent structural template refactors as a destructive file deletion followed by the addition of an entirely new file. * **Template File Rename (`src/pages/fileBrowser/`):** * Renamed `src/pages/fileBrowser/list.hbs` to `src/pages/fileBrowser/listItem.hbs` with zero line changes (100% similarity score), guaranteeing clean `git blame` and file history continuity across structural revisions. (AI generated commit message)
…eholder
Overhaul the directory rendering logic in the file browser by replacing monolithic template compilation with programmatic DOM element construction and individual item parsing.
Previously, directory list rendering relied on a single template (`list.hbs`) that wrapped the outer `<ul>` element, handled list iteration (`{{#list}}`), and relied on the `mustache` package's built-in capability to render placeholder text if the element had no children when the `empty-msg` HTML attribute was provided.
This commit refactors the template down to a granular single-item scale and adds the empty state placeholder back explicitly via programmatic rendering. By splitting template rendering into granular helper functions (`createListEl`, `createListItemEl`, and `createPlaceholderEl`), directory rendering now builds list elements individually and explicitly appends a styled placeholder node whenever a directory contains no files or folders.
* **Template Scope Reduction (`src/pages/fileBrowser/listItem.hbs`):**
* Removed the enclosing `<ul class="list" id="list">` container tag and the surrounding `{{#list}}...{{/list}}` iteration block from the Handlebars template.
* Converted the file into a standalone item partial that takes an entry object and produces a single `<li>` element representing a file or directory row.
* **DOM Element Construction Helpers (`src/pages/fileBrowser/fileBrowser.js`):**
* Added `createListEl()` to dynamically generate the parent `<ul className="list" id="list">` element.
* Added `createListItemEl(obj)` to parse individual item objects through `mustache.render(_listItem, obj)` into single `HTMLLIElement` nodes.
* Added `createPlaceholderEl(msg)` to create dedicated empty-state DOM elements (`<div id="placeholder">{msg}</div>`).
* **Render Loop & Empty State Logic (`src/pages/fileBrowser/fileBrowser.js`):**
* Updated `render(dir)` to construct list containers programmatically and append rendered child elements via standard DOM iteration (`$list.appendChild(el)`).
* Re-implemented empty directory handling: if `list.length` is zero, a placeholder element containing the localized empty folder string is appended to the list, restoring the empty message functionality previously supplied via Mustache's `empty-msg` attribute.
* **Placeholder Layout Styling (`src/pages/fileBrowser/fileBrowser.scss`):**
* Defined CSS rules for `#placeholder` utilizing Flexbox (`display: flex`, `align-items: center`, `justify-content: center`) to ensure empty folder messages are centered within the file browser container.
(AI generated commit message)
…t item rendering with skeleton loading Overhaul navigation architecture and list view rendering performance by decoupling history tracking into an event-driven `NavStack` class and replacing full-list re-renders with granular element construction and visual skeleton states. * **Event-Driven Navigation Stack (`src/pages/fileBrowser/NavStack.js`):** * Created `NavStack` extending `EventTarget` to encapsulate navigation array history (`#arr`) and URL uniqueness tracking (`#urlSet`). * Emits custom `push` and `pop` DOM events to keep UI updates, state serialization, and action stack clean-ups decoupled. * Implemented bounds-checked array indexing (`get(i)`), conditional popping (`popUntil(url)`), and state inspection (`has(url)`, `toJSON()`). * **Directory Caching & Asynchronous Fetching (`src/pages/fileBrowser/fileBrowser.js`):** * Transitioned `cachedDir` storage from a plain object to a standard JavaScript `Map` instance. * Replaced `getDir` with `getDirList(url)`, utilizing `Promise.withResolvers()` and `Promise.race()` to enforce a 10-second timeout on directory listings across storage mechanisms. * **Skeleton Loading & Incremental Rendering (`src/pages/fileBrowser/fileBrowser.js` & `.scss`):** * Added skeleton placeholder generation during directory fetching using a `DocumentFragment` populated with random-width bar elements. * Introduced `.placeholder` CSS styling in `fileBrowser.scss` and updated `listItem.hbs` to handle non-interactive skeleton rows gracefully. (AI generated commit message)
…g AbortController Introduce cancellation checking via `AbortController` in `renderCurrentDir()` to prevent stale asynchronous directory reads from overwriting active UI state during rapid user navigation. * **Render Task Tracking (`src/pages/fileBrowser/fileBrowser.js`):** * Declared a module-scoped `_rndrAbortCtrl` reference to monitor in-flight directory rendering tasks. * Automatically aborts any existing controller upon initiating a new `renderCurrentDir()` invocation or when triggering the page hide handler (`$page.onhide`). * **Asynchronous Race Condition Guarding (`src/pages/fileBrowser/fileBrowser.js`):** * Evaluates `abortSignal.aborted` immediately after `await getDirList(url)` resolves. * Discards outdated directory payload promises before mutating the directory cache or appending child elements to the DOM. (AI generated commit message)
…ation
Automatically prepend a dedicated parent directory (`..`) navigation tile at the top of directory listings when navigating deep within a directory hierarchy.
* **Template Support (`src/pages/fileBrowser/listItem.hbs`):**
* Added template attribute binding `{{#oneDirUp}}data-one-dir-up{{/oneDirUp}}` to flag parent navigation element targets.
* **Parent Node Generation (`src/pages/fileBrowser/fileBrowser.js`):**
* Evaluates navigation stack depth (`navStack.length >= 2`) during `renderCurrentDir()`.
* Programmatically builds a `..` parent item tile and prepends it to the DOM fragment across both skeleton loading states and final directory renders.
* **Interaction & Event Handling (`src/pages/fileBrowser/fileBrowser.js`):**
* Intercepts `open` actions on `data-one-dir-up` elements to execute upward directory traversal using `navStack.get(-2)`.
* Explicitly bypasses context menu triggers (`contextMenuHandler`) and new path prompts for parent tiles.
* **Container Layout (`src/pages/fileBrowser/fileBrowser.scss`):**
* Added `:has(> [data-one-dir-up])` layout rules to offset placeholder heights when parent tiles are rendered above empty/loading states.
(AI generated commit message)
Provide clear visual feedback for empty directories and filesystem errors by programmatically constructing and styling a centered placeholder element. * **Error Handling Relocation (`src/pages/fileBrowser/fileBrowser.js`):** * Removed internal `try/catch` block from `getDirList()` and moved exception handling directly into `renderCurrentDir()`. * Captures filesystem inspection errors (including local, content URI, and remote FTP/SFTP protocol failures), formatting virtual paths and error strings for UI display. * **Placeholder Rendering Logic (`src/pages/fileBrowser/fileBrowser.js`):** * Refined `createPlaceholderEl()` helper to validate text inputs and output `<div id="placeholder">` nodes. * Injects formatted filesystem error messages into the list placeholder node, falling back to localized empty folder messages (`strings["empty folder message"]`) when listings return no items. (AI generated commit message)
7717fb2 to
e3be768
Compare
|
@greptile review this, and the updated PR description, especially the dependence on yet another PR. |
|
Thanks for the heads up! I'll consider that concern resolved via #2725 then. No further action needed on this PR for that issue. |
3 participants
Executive Summary
This Pull Request delivers a comprehensive architectural overhaul, performance refactoring, and user experience enhancement for the application's central File Browser module (
src/pages/fileBrowser/). Over the course of 4 strategic commits, the codebase transitions from legacy state-mutation patterns and un-cancelled async renders to a modern, decoupled, event-driven architecture designed for high scalability, type-safe navigation state management, and non-blocking asynchronous execution.External Dependencies
Key Objectives Achieved
NavStackstate container (EventTarget) coupled with CSS skeleton placeholder loading states (.placeholder).AbortControllercancellation signals within directory rendering flows (renderCurrentDir) to eliminate UI state corruption caused by rapid directory switching...) at the top of directory views when navigation stack depth allows upward traversal (navStack.length >= 2).#placeholder) to display localized empty folder messages or formatted filesystem errors when directory reads fail or return no entries.High-Level Architecture Comparison
NavStackclass extending standardEventTargetemitting typedpushandpopevents.createListItemEl(), leveraging PR #2723..placeholder) maintaining layout stability while fetching directory listings.AbortControlleractively cancel obsolete in-flight directory rendering tasks...) prepended at the top of nested listings (navStack.length >= 2).#placeholderelement dynamically injected for empty folders or formatted filesystem load errors.Subsystem Architectural Breakdown
1. Dependency Integration (PR #2723)
This PR relies directly on PR #2723, which introduces granular item rendering and programmatic DOM list construction. Utilizing this baseline allows list updates to perform targeted element building rather than incurring full DOM list teardowns.
2. Event-Driven Navigation Stack (
NavStack)The core file browser navigation has been fully decoupled from view-rendering logic through the creation of a standalone
NavStackclass insrc/pages/fileBrowser/NavStack.js.By inheriting from standard browser
EventTarget,NavStackencapsulates full control over path stack depth (#arr), unique path tracking (#urlSet), and boundary constraints while notifying listeners through native event dispatch mechanisms (push,pop).3. Async Safety & Concurrency Optimization
A critical vulnerability in asynchronous file managers occurs when network (FTP/SFTP) or local disk read latency varies. If a user navigates from
Directory AtoDirectory BtoDirectory Cin rapid succession:Directory Afetch initiates (500ms delay).Directory Bfetch initiates (100ms delay).Directory Bresolves and renders to the DOM.Directory Aresolves late and overwrites the active viewport with stale directory data.To permanently eradicate this race condition,
renderCurrentDir()now instantiates anAbortController. When a new navigation event occurs or the page is hidden ($page.onhide), the existing controller emits an.abort()signal. The preceding async pipeline checksabortSignal.aborted, halts DOM building, and discards stale directory entries cleanly.4. Error Handling and Empty State Placeholders
When opening an empty directory or when directory fetching fails (e.g., connection timeout or permission error across local, content URI, or FTP/SFTP sources),
renderCurrentDir()programmatically appends a centered#placeholderelement inside the list container. Using CSS:has(> [data-one-dir-up]) > #placeholder, the container automatically adjusts its height calculation (calc(100% - 45px)) to accommodate parent directory tiles when present.Detailed Commit Breakdown
Commit 1:
9bad1e03916c5054a775c979790829ac997b5761src/pages/fileBrowser/NavStack.jssrc/pages/fileBrowser/fileBrowser.js,src/pages/fileBrowser/fileBrowser.scss,src/pages/fileBrowser/listItem.hbsMap, and adding CSS skeleton states during async operations.NavStack.js, an event-driven navigation stack extending standardEventTargetto encapsulate navigation depth (#arr) and URL set operations (#urlSet).Mapinstance (cachedDir).getDirintogetDirList(url)usingPromise.withResolvers()andPromise.race()with a 10-second timeout guard..placeholder) in SCSS to prevent content shifts during async fetches.Commit 2:
9c1c84999ba96b734940eea367ca8d1451b820c1src/pages/fileBrowser/fileBrowser.jsAbortController(_rndrAbortCtrl) within the lifecycle ofrenderCurrentDir().$page.onhide).abortSignal.abortedpost-fetch to discard stale directory entries before modifying DOM elements or updating directory state.Commit 3:
3bc412289dbd6c7a798a77c88a2e662232bc52e3src/pages/fileBrowser/fileBrowser.js,src/pages/fileBrowser/fileBrowser.scss,src/pages/fileBrowser/listItem.hbs..) folder traversal tiles at the top of file listings.data-one-dir-upinlistItem.hbs...navigation tile whennavStack.length >= 2.oneDirUpaction ondata-one-dir-upnodes to invoke upward navigation back tonavStack.get(-2).data-one-dir-uptiles from context menu handlers.Commit 4:
e3be76825fb62afdaffb0c2eeece7e2714c84dadsrc/pages/fileBrowser/fileBrowser.jsrenderCurrentDir()to catch filesystem errors (local, content URIs, FTP/SFTP) and format them into readable error messages.createPlaceholderEl()helper to construct a<div id="placeholder">displaying either localized empty folder text or formatted filesystem error messages.:has(> [data-one-dir-up]) > #placeholderselector to dynamically adjust placeholder height (calc(100% - 45px)) when parent navigation tiles are present.Mathematical Performance & Complexity Analysis
1. Async Directory Fetching & Timeout Guard
Let$T_{\text{lsDir}}$ represent the I/O latency of listing a directory across local storage, content URIs, or network protocols (FTP/SFTP), and let $T_{\text{timeout}} = 10,000\text{ms}$ .
The async race pipeline evaluates:
$$T_{\text{fetch}} = \min(T_{\text{lsDir}}, T_{\text{timeout}})$$
In network-bound locations (e.g. unresponsive FTP/SFTP servers), execution is bounded by$T_{\text{timeout}}$ , after which the promise rejects and triggers
#placeholderrendering without hanging the user interface indefinitely.2. Rendering Memory Overhead & Layout Shift
Replaced full DOM list re-compilation with single-pass element node creation (leveraging PR #2723):
$$\text{Memory Allocation}{\text{legacy}} \propto \text{String Size}(M) + \text{DOM Nodes}(N)$$
$$\text{Memory Allocation}{\text{refactored}} \propto \text{DOM Nodes}(N)$$
By eliminating duplicate intermediate string allocations during template parsing, total garbage collection frequency during heavy directory scrolling is reduced significantly.
Testing Plan & Quality Assurance Matrix
1. Unit & Structural Integrity Verification
NavStackpush, pop, clear, and boundary conditions:.pop()at root level does not throw or reduce stack depth below 1..lengthand dispatches standardpushevents.listItem.hbsand programmatic list item construction (PR refactor(file-browser): Granular item rendering and programmatic DOM list construction #2723) render cleanly for files, folders, symlinks, user-added storage items, and the..parent directory tile.2. Integration & Edge Case Scenarios
AbortControllercancels obsolete pending fetches; active view renders correct final directory without state leakage...tile at top of nested directory.navStack.get(-2))...tile is present.#placeholder..placeholderskeleton items render immediately whilegetDirList()resolves.Migration & Compatibility Considerations
Backwards Compatibility & Dependencies
fileBrowser.jsremain fully backwards-compatible with existing host application view router mounts.Conclusion
This pull request significantly stabilizes the
fileBrowsersubsystem, drastically reduces rendering memory overhead, eliminates async race conditions, and delivers a modern visual experience with race-safe navigation controls.(PR name and description are AI generated (Gemini 3.6 Flash))