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 4 commits into
Conversation
Greptile SummaryThis PR performs a large-scale architectural overhaul of the file browser: navigation state is extracted into a new
Confidence Score: 2/5The file browser now has a real race-condition fix and correct parallel deletion, but the click handler reads isOneDirUp/isOpenDoc from e.target rather than the closest tile, silently breaking the document-picker on inner-element taps, and several bugs from prior review rounds remain unresolved. The e.target-based flag reading for isOneDirUp and isOpenDoc means tapping any child element of the .. tile or Select-document tile dispatches the wrong action. Combined with open issues from prior rounds (loadStates crashing on malformed stored state, reload with undefined url/name, notSelectable guard depth issue), the file browser has multiple present functional regressions. Files Needing Attention: fileBrowser.js — the click handler isOneDirUp/isOpenDoc flag extraction (lines 922–937) and the loadStates/reload paths flagged in earlier rounds. Important Files Changed
Reviews (21): Last reviewed commit: "feat: Can I add the timeout back?" | 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
96bbb2e to
e640f65
Compare
1cfe31e to
80872d4
Compare
8044c27 to
20749b7
Compare
20749b7 to
16dac10
Compare
|
@AuDevTist1C You should break this pr into small pieces otherwise it will very hard to review and catch stuff, also it's touching a lots of part of file browser. |
| if (!url) { | ||
| const $url = $el.get("data-url"); | ||
| if ($url) { | ||
| url = $url.textContent; | ||
| } | ||
| if ($url) url = $url.textContent; | ||
| } | ||
|
|
||
| if (storageType === "notification") { | ||
| switch (uuid) { | ||
| case "addstorage": | ||
| addStorage(); | ||
| break; | ||
|
|
||
| default: | ||
| break; | ||
| } | ||
| if (uuid === "addstorage") addStorage(); | ||
| return; | ||
| } | ||
|
|
||
| if (!url && action === "open" && isDir && !idOpenDoc && !isContextMenu) { | ||
| if ( | ||
| !url && | ||
| action === "open" && | ||
| isDir && | ||
| !isOpenDoc && | ||
| !isOneDirUp && |
There was a problem hiding this comment.
isOneDirUp/isOpenDoc read from e.target, not the tile — guard bypassed for clicks on child elements
$el is e.target, which is frequently the <span class="icon"> or <div class="text"><span>...</span></div> inside the <li>, not the <li> itself. Those child elements don't carry data-one-dir-up or data-open-doc, so both isOneDirUp and isOpenDoc resolve to false when a sub-element is tapped. Concretely: tapping the icon inside the "Select document" tile makes isOpenDoc = false, causing the case "open" branch to fire file() with a null URL instead of triggering the document-picker flow. The same pattern from the notSelectable guard applies here — using $el.closest(".tile") to read these flags would make them authoritative regardless of click-target depth.
…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. * **Navigation Stack Module (`src/pages/fileBrowser/NavStack.js`):** * Created a dedicated `NavStack` class extending `EventTarget` to manage navigation history stack depth (`#arr`), unique path tracking (`#urlSet`), and traversal boundaries. * Emits typed `push` and `pop` custom DOM events to loosely couple navigation state changes with UI updates and action stack synchronization. * **Modular Item Rendering (`src/pages/fileBrowser/listItem.hbs`):** * Replaced monolithic `list.hbs` template with a modular `listItem.hbs` partial template. * Dynamically constructs DOM list items per record via `createListItemEl()`, avoiding full-list template re-compilation. * **Directory Cache & Async Fetching (`src/pages/fileBrowser/fileBrowser.js`):** * Refactored `cachedDir` state from a plain object to a `Map` instance. * Extracted `getDirList(url)` using `Promise.withResolvers()` and `Promise.race()` with a 10-second timeout on `fsOperation(url).lsDir()` (supporting local storage, content URIs, FTP, and SFTP directory listings). * **Skeleton Placeholder Loading (`src/pages/fileBrowser/fileBrowser.scss` & `fileBrowser.js`):** * Added visual CSS placeholder styles (`.placeholder`) to display animated skeleton loaders while directory listings resolve asynchronously. (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. When navigating quickly between folders or toggling back/forward, concurrent asynchronous `getDirList()` calls can resolve out of order. Slower filesystem reads could resolve after a subsequent directory navigation, overwriting the view with outdated contents. * **Render Cancellation Tracking (`src/pages/fileBrowser/fileBrowser.js`):** * Maintained a scoped `_rndrAbortCtrl` instance within `FileBrowserInclude` to track active render tasks. * Aborts any existing render controller when `renderCurrentDir()` is invoked again or when `$page.onhide` fires. * **Race Condition Guarding:** * Checks `abortSignal.aborted` immediately after `await getDirList(url)` resolves to discard stale directory contents before updating state or mutating the DOM container. (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 Integration (`src/pages/fileBrowser/listItem.hbs`):**
* Added template attribute support for `{{#oneDirUp}}data-one-dir-up{{/oneDirUp}}` to identify parent navigation elements.
* **Dynamic Item Prepending (`src/pages/fileBrowser/fileBrowser.js`):**
* Evaluates navigation stack depth in `renderCurrentDir()`: when `navStack.length >= 2`, automatically prepends a parent folder tile (`..`) to the top of list DOM fragments during both skeleton loading and directory rendering.
* **Navigation & Action Handling:**
* Intercepts tap/click events on items with `data-one-dir-up` to execute upward navigation to `navStack.get(-2)`.
* Excludes parent directory tiles from context menu triggers (`contextMenuHandler`) and storage creation prompts.
(AI generated commit message)
Provide clear visual feedback for empty directories and filesystem errors by programmatically constructing and styling a centered placeholder element. * **Placeholder DOM Construction (`src/pages/fileBrowser/fileBrowser.js`):** * Added `createPlaceholderEl()` helper to generate an `#error-or-empty-dir` placeholder element. * Shifted error handling from inside `getDirList()` into `renderCurrentDir()`, allowing filesystem errors (including local, content URI, and FTP/SFTP `lsDir` failures) to be caught and displayed directly inside the directory view. * Formats virtual path errors and displays either a localized empty directory message or a formatted error string when directory listings return no items. * **Layout & CSS Flexbox Alignment (`src/pages/fileBrowser/fileBrowser.scss`):** * Added CSS flexbox rules for `#error-or-empty-dir` to center placeholder text horizontally and vertically. * Utilized CSS `:has(> [data-one-dir-up])` selector to offset placeholder height (`calc(100% - 45px)`) when the interactive parent directory tile is rendered above it. (AI generated commit message)
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
2 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 5 strategic commits, the codebase transitions from legacy DOM-manipulation patterns and monolithic rendering paradigms to a modern, decoupled, event-driven architecture designed for high scalability, type-safe dataset management, and non-blocking asynchronous execution.Key Objectives Achieved
list.hbstemplate with a granular, item-levellistItem.hbstemplate and introduced an event-drivenNavStackstate container (EventTarget) coupled with CSS skeleton placeholder loading states (.placeholder).AbortControllercancellation signals within directory rendering flows 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).#error-or-empty-dir) to display localized empty folder messages or formatted filesystem errors when directory reads fail or return no entries.Control Flow & Syntax Modernization: Adopted modern ECMAScript features (such as logical nullish assignments||=), refactored nested conditional branches into structuredswitchstatements, simplifiedsanitizeZipPath, and consolidated asynchronous operations across local, content URI, FTP, and SFTP directory listings.High-Level Architecture Comparison
NavStackclass extending standardEventTargetemitting typedpushandpopevents.list.hbstemplate rendering entire directory DOM nodes in a single execution block.listItem.hbstemplate producing single DOM elements viacreateListItemEl()..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).empty-msg) on monolithic list template, lost during refactoring.#error-or-empty-dirplaceholder dynamically injected for empty folders or directory load errors.Control Flow & SyntaxVerbose logical OR / ternary fallbacks and nestedif/elseladders for menu handlers.Logical nullish assignments (||=), strictswitchcontrol flow, and streamlined async operations across storage types (including FTP/SFTP).Subsystem Architectural Breakdown
1. 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.2. 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, the existing controller emits an.abort()signal. The preceding async pipeline catches the abort signal, halts DOM building, and cleans up pending handlers cleanly without unhandled rejections or viewport corruption.3. 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#error-or-empty-direlement inside the list container. Using CSS:has(> [data-one-dir-up]), the container automatically adjusts its height calculation (calc(100% - 45px)) to accommodate parent directory tiles when present.Detailed Commit Breakdown
Commit 1:
e8223e67820fa19dadd16d22ba0c3008b8542897src/pages/fileBrowser/NavStack.js,src/pages/fileBrowser/listItem.hbssrc/pages/fileBrowser/list.hbssrc/pages/fileBrowser/fileBrowser.js,src/pages/fileBrowser/fileBrowser.scssNavStack.js, an event-driven navigation stack extending standardEventTargetto encapsulate navigation depth and path set operations.list.hbsand introduced modularlistItem.hbscompiled element factories (createListItemEl()).Mapinstance (cachedDir).getDirList(url)usingPromise.withResolvers()andPromise.race()with a 10-second timeout (supporting local, content URI, FTP, and SFTP locations)..placeholder) in SCSS to prevent content shifts during async fetches. refactor(file-browser): Overhaul navigation state, skeleton rendering, and async safety #2500 (comment)Commit 2:
26196960a4ec4c052178ba6ee8b571974fe78592src/pages/fileBrowser/fileBrowser.jsAbortControllerinstances within the lifecycle ofrenderCurrentDir().$page.onhide).abortSignal.abortedpost-fetch to discard stale directory entries before modifying DOM elements or updating directory state.Commit 3:
8972a8ae274a4e391bb6e063497d47e60995965bsrc/pages/fileBrowser/fileBrowser.js,src/pages/fileBrowser/listItem.hbs..) folder traversal tiles at the top of file listings.data-one-dir-upinlistItem.hbs...navigation tile whennavStack.length >= 2.data-one-dir-upnodes to invoke upward navigation back tonavStack.get(-2).Commit 4:
38b64425187206d06d63aac8cb42afb6567fdc10src/pages/fileBrowser/fileBrowser.js,src/pages/fileBrowser/fileBrowser.scsscreatePlaceholderEl()helper to construct a<div id="error-or-empty-dir">displaying either localized empty folder text or formatted filesystem error messages.renderCurrentDir()to gracefully log and display directory load errors (including FTP/SFTP or permission failures).#error-or-empty-dirto vertically and horizontally center messages within the list container.:has(> [data-one-dir-up])selector to dynamically adjust height (calc(100% - 45px)) when top-level parent navigation tiles are present.(Will push if the maintainers are OK with this)
Commit 5:Files Changed:src/pages/fileBrowser/fileBrowser.jsRationale: Aligns codebase with modern ES2021+ JavaScript patterns, eliminates redundant branching logic, and streamlines asynchronous handlers.Technical Highlights:Adopted logical nullish assignment operators (||=) for configuration and storage property fallbacks.Replaced nestedif/elseladders in context menu and action bar handlers with strict-equalityswitchstatements.Cleaned up ZIP extraction path sanitization insanitizeZipPath()into a concise path normalization pipeline.Converted verbose promise callbacks into concise single-line arrow functions across storage and file operation flows.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
#error-or-empty-dirrendering without hanging the user interface indefinitely.2. Rendering Memory Overhead & Layout Shift
Replaced full HTML string recompilation and inner HTML injection ($O(M)$ memory footprint where $M$ is the size of the combined directory structure string) with granular single-pass element node creation:
$$\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 Handlebars 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.hbsrenders 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.#error-or-empty-dir..placeholderskeleton items render immediately whilegetDirList()resolves.Migration & Compatibility Considerations
Breaking Changes
list.hbshas been deleted from the repository. Any custom plugins or extensions importinglist.hbsdirectly must switch to usinglistItem.hbsin conjunction withcreateListItemEl().Backwards Compatibility
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))