refactor(file-browser): Overhaul navigation state, directory caching, async rendering, and parent navigation - #2500
Conversation
Greptile SummaryThis PR overhauls the file browser by replacing the inline
Confidence Score: 1/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant handleClick
participant navigate
participant NavStack
participant renderCurrentDir
participant getDirList
participant AbortController
User->>handleClick: click tile / prevDir
handleClick->>navigate: navigate(url, name)
navigate->>NavStack: has(url) ?
alt URL already in stack
NavStack-->>navigate: true
navigate->>NavStack: popUntil(url)
else new URL
NavStack-->>navigate: false
navigate->>NavStack: push(url, name)
end
NavStack-->>navigate: fires "update" microtask
navigate->>renderCurrentDir: renderCurrentDir()
renderCurrentDir->>AbortController: abort previous _rndrAbortCtrl
renderCurrentDir->>AbortController: new AbortController
alt list not cached
renderCurrentDir->>renderCurrentDir: append spinner placeholder
renderCurrentDir->>getDirList: getDirList(url)
getDirList->>getDirList: Promise.race([lsDir, 15s timeout])
getDirList-->>renderCurrentDir: list[] or throws
renderCurrentDir->>AbortController: check abortSignal.aborted
alt aborted
renderCurrentDir-->>User: (no DOM update)
else not aborted
renderCurrentDir->>renderCurrentDir: replace placeholder with $list
end
else list cached
renderCurrentDir->>renderCurrentDir: render immediately from cache
end
renderCurrentDir->>renderCurrentDir: cachedDir.set(url, dir)
Reviews (31): Last reviewed commit: "feat(file-browser): Add parent directory..." | 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
This comment has been minimized.
This comment has been minimized.
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
7717fb2 to
e3be768
Compare
|
Thanks for the heads up! I'll consider that concern resolved via #2725 then. No further action needed on this PR for that issue. |
|
Why there is need of skeleton loading on file browser ? |
e3be768 to
7972ceb
Compare
AuDevTist1C
changed the title
7972ceb to
ee00d08
Compare
|
@greptile there's already a |
|
You're right — the |
@greptile somewhere in #2500 (comment) |
|
Now I have the full picture. The original concern from the summary was about the null-destructure crash in The refactored while (states.length) {
try {
navStack.push(states.shift());
} catch (err) {
console.error(err);
}
}The |
|
You're right, and I can confirm it directly from the file. The function loadStates(states) {
if (!Array.isArray(states) || !states.length) return;
while (states.length) {
try {
navStack.push(states.shift());
} catch (err) {
console.error(err);
}
}
const dir = navStack.get(-1);
if (dir) navigate(dir);
}The |
|
You're right — the The catch only does |
f127b23 to
6e88a94
Compare
|
@greptile review this, because #2776 has been merged. Also be sure to review from the base branch all the way to every last commit and diff in this PR so you don't have to miss any new changes, like you've missed with |
3ca79e9 to
6e02451
Compare
6e02451 to
e036de8
Compare
… management Abstract navigation tracking, history state management, and navbar UI syncing into a dedicated `EventTarget` class. Create `NavStack` class (`src/pages/fileBrowser/NavStack.js`): - Implement `NavStack` extending `EventTarget` with a custom `Symbol.toStringTag` property - Add `push`, `pop`, `popUntil`, `get` (supporting negative indexing), `has`, `on`, `off`, and `toJSON` methods with parameter validation - Maintain an internal `#urlSet` to prevent duplicate stack entries - Queue microtasks for `update` event dispatching, providing read-only `added` and `removed` location diffs in event details Integrate `NavStack` into file browser (`src/pages/fileBrowser/fileBrowser.js`): - Replace manual `state` array and direct `localStorage` persistence with a `NavStack` instance - Listen to `update` events on `NavStack` to persist state to `localStorage`, clean up removed navbar elements and `actionStack` entries, and register new back-navigation actions - Cache navbar DOM elements using a `navBarEls` Map with `getOrInsertComputed` - Refactor `navigate` to accept location objects or strings and manage stack state using `navStack.has`, `navStack.popUntil`, and `navStack.push` - Refactor `loadStates` to push history entries into `navStack` and navigate directly to the top item (`navStack.get(-1)`) - Update folder selection button state (`$openFolder.disabled`) in `render()` and remove obsolete `pushState()` helper function (AI generated commit message)
Replace the plain object container used for cached directories with an ES6 `Map` to improve key lookup operations and key management semantics. Update cached directory data structure (`src/pages/fileBrowser/fileBrowser.js`): - Re-initialize `cachedDir` variable as a `Map` - Replace object property lookups with `Map.prototype.has()` and `Map.prototype.get()` - Update cache writes to use `Map.prototype.set()` - Update directory deletion calls to use `Map.prototype.delete()` (AI generated commit message)
…nline spinner Transition directory loading from blocking modal dialogs to inline loading state indicators with explicit timeout handling. Add async directory list fetching and rendering (`src/pages/fileBrowser/fileBrowser.js`): - Extract list retrieval into `getDirList` using `Promise.withResolvers` and `Promise.race` with a 15-second timeout - Replace render function with `renderCurrentDir` to handle asynchronous state changes - Render inline SVG spinner placeholder in place of missing list content - Maintain scroll position across directory re-renders Style inline spinner (`src/pages/fileBrowser/fileBrowser.scss`): - Add flexbox alignment styles for `#spinner` inside directory list container (AI generated commit message)
…ange or page hide Introduce `AbortController` tracking to cancel pending directory listing render tasks when navigating quickly or hiding the page. Add render abort controller logic (`src/pages/fileBrowser/fileBrowser.js`): - Instantiate `AbortController` instance inside `renderCurrentDir` - Abort existing controller prior to running new render sequence - Validate `abortSignal` before committing directory list DOM updates - Call `abort` on active render controller when `$page.onhide` fires (AI generated commit message)
Add a dedicated '..' list tile at the top of directory listings to provide explicit one-level-up navigation.
Update list template (`src/pages/fileBrowser/list.hbs`):
- Render parent directory tile ('..') when `prevDir` flag is active
- Adjust handlebars conditionals for message displays and item list loop
Handle `prevDir` action in file browser (`src/pages/fileBrowser/fileBrowser.js`):
- Add `prevDir` action handler navigating to `navStack.get(-2)`
- Pass `prevDir` condition check (`navStack.length >= 2`) into list render template
- Ignore context menu actions triggered on the `prevDir` item
Adjust layout styling (`src/pages/fileBrowser/fileBrowser.scss`):
- Adjust height calculation for message and spinner containers when `prevDir` item is visible
(AI generated commit message)
e036de8 to
81ac2ab
Compare
| /** @type {() => HTMLSpanElement} */ | ||
| let el = navBarEls.get(url, f); | ||
| if (!el) { | ||
| el = ( | ||
| <span | ||
| id={getNavId(url)} | ||
| className="nav" | ||
| data-url={url} | ||
| data-name={name} | ||
| data-action="navigation" | ||
| attr-text={name} | ||
| tabIndex={-1} | ||
| ></span> | ||
| ); | ||
| navBarEls.set(url, el); | ||
| } | ||
| const el = navBarEls.getOrInsertComputed(url, f); | ||
| $navigation.append(el); | ||
| $navigation.scrollLeft = $navigation.scrollWidth; | ||
| } |
There was a problem hiding this comment.
el declaration crashes entire module
pushToNavbar contains both let el (line 1629) and const el (line 1644) in the same block scope. JavaScript throws a parse-time SyntaxError: Identifier 'el' has already been declared before any code in the file runs, so the file browser never loads at all. This is a merge artifact: the manual if (!el) guard approach and the getOrInsertComputed one-liner were both left in. Additionally, f is referenced as a second argument to both calls but is never declared anywhere in scope — once the duplicate-declaration error is fixed, every call to pushToNavbar would immediately throw a ReferenceError: f is not defined.
The fix is to remove the manual let el / if (!el) { ... } navBarEls.set(url, el) block and keep only the getOrInsertComputed line (with a proper inline factory instead of the undeclared f), or conversely keep the manual block and remove the getOrInsertComputed call entirely.
3 participants
Executive Summary
This Pull Request delivers an architectural overhaul, performance refactoring, state management modernization, and user experience enhancement for the application's central File Browser module (
src/pages/fileBrowser/). Building upon the decoupledNavStackhistory foundation introduced in PR #2793, this PR spans 4 strategic commits to transition the directory cache to an ES6Map, implement non-blocking asynchronous list rendering with inline SVG spinners, introduce race-safeAbortControllertask cancellation, and add explicit parent directory traversal controls.Key Objectives Achieved
cachedDir = {}) to an ES6Mapinstance (cachedDir = new Map()), leveraginghas(),get(),set(), anddelete()methods for cleaner key management semantics and improved lookup performance.renderCurrentDir(), utilizingPromise.withResolvers()andPromise.race()with a 15-second timeout guard to display an inline SVG spinner placeholder while keeping the interface responsive.AbortController: IntroducedAbortControllertracking (_rndrAbortCtrl) within directory rendering sequences to cancel obsolete in-flight directory reads upon rapid navigation path changes or when$page.onhidetriggers...parent directory tile (data-action="prevDir") at the top of directory listings whenever the navigation stack depth supports upward navigation (navStack.length >= 2).High-Level Architecture Comparison
state = []) and directlocalStorage/actionStackcalls scattered across file browser methods.NavStackclass extending standardEventTargetemitting asynchronous"update"microtask events.cachedDir = {}) utilizinginlookups anddeleteoperations.Mapinstance (cachedDir = new Map()) using nativehas(),get(),set(), anddelete()methods.loader.create()) that froze user interaction during long filesystem reads.renderCurrentDir) displaying an SVGtailSpinspinner inside#spinnerwith a 15s timeout.AbortControllerper render task; obsolete tasks are immediately cancelled via.abort()on navigation or page hide...parent directory tile (data-action="prevDir") prepended at the top of listings whennavStack.length >= 2.Subsystem Architectural Breakdown
1. Event-Driven Navigation Stack (
NavStack) — PR #2793 DependencyThe file browser's path history and state tracking rely on
src/pages/fileBrowser/NavStack.js(from PR #2793):EventTargetand setsSymbol.toStringTagto"NavStack".#urlSet(Set<string>) for#arr(Array<Location>) for ordered stack depth management.push(),pop(), andpopUntil()collect path changes in a private#updatedURLsstructure and schedule a singleCustomEvent("update")usingqueueMicrotask().fileBrowser.jssubscribes to the"update"event to automatically updatelocalStorage.fileBrowserState, syncactionStackpush/remove commands, and push breadcrumb items to$navigation.2. ES6 Map Directory Cache
Refactored directory list caching from plain objects to an ES6
Mapcontainer (cachedDir):if (url in cachedDir)checks withcachedDir.has(url).cachedDir.get(url)andcachedDir.set(url, dir).delete cachedDir[url]statements withcachedDir.delete(url)during cache invalidation and directory reloads.3. Non-Blocking Async Rendering & Inline Spinner Lifecycle
Replaced blocking modal loader dialogs with non-blocking asynchronous directory rendering:
getDirList(url)Pipeline: Wraps filesystem calls (lsDir()) withPromise.withResolvers()andPromise.race()to enforce a strict 15-second (15000ms) loading timeout.renderCurrentDir()appends a temporary.placeholderelement containing<span id="spinner">${createTailSpinSvg()}</span>into$content.scrollTopon$oldListbefore removal and restoresscrollToponce directory DOM elements are appended.4. Concurrency Control & Render Cancellation (
AbortController)To prevent race conditions during rapid directory switching:
renderCurrentDir()instantiates a freshAbortController(rndrAbortCtrl) and aborts any active prior controller_rndrAbortCtrl?.abort().abortSignal.aborted.$page.onhideexecutes (e.g., navigating away or closing the file browser),_rndrAbortCtrl?.abort()is invoked immediately to cancel pending async directory operations.5. Parent Directory Traversal Tile (
list.hbs)Restored explicit parent directory traversal in the main item list:
list.hbsconditionally renders a<li class="tile" data-action="prevDir" data-not-selectable>tile with standard..text whenprevDirevaluates totrue.renderCurrentDir()checksnavStack.length >= 2to passprevDir: true...tile triggersnavStack.get(-2)and navigates to the parent directory. Context menu events onprevDiritems are explicitly ignored.:has(> [data-action="prevDir"])to automatically adjust empty folder messages and inline spinner container heights (height: calc(100% - 45px)) when the parent tile is visible.Detailed Commit Breakdown
Commit 1:
1a6bf82821012da039439e8e749228fa91997f36src/pages/fileBrowser/fileBrowser.jsMapinstance to improve cache operation semantics and lookup performance.cachedDir = new Map().url in cachedDir) tocachedDir.has(url)andcachedDir.get(url).cachedDir.set(url, dir).delete cachedDir[url]) withcachedDir.delete(url)calls across reload and deletion handlers.Commit 2:
ec6b7f087f40ca43a1409e89cea87d5d75431288src/pages/fileBrowser/fileBrowser.js,src/pages/fileBrowser/fileBrowser.scssgetDirList(url)usingPromise.withResolvers()andPromise.race()with a 15-second (15000ms) timeout guard.renderCurrentDir(force)to replace legacy synchronousrenderfunction.<span id="spinner">${createTailSpinSvg()}</span>during active directory fetches.scrollTop) across directory re-renders.#spinnerinfileBrowser.scss.Commit 3:
6b2fb3cb5cfab3155c79cc1c29295c73781a2341src/pages/fileBrowser/fileBrowser.js_rndrAbortCtrl(AbortController) insiderenderCurrentDir._rndrAbortCtrl?.abort()before beginning a new rendering operation.abortSignal.abortedstatus prior to committing DOM updates._rndrAbortCtrl?.abort()to the$page.onhideevent handler to cancel pending fetches when hiding the file browser.Commit 4:
81ac2ab6c846fda4a9976232dddfc13867d1aa52src/pages/fileBrowser/fileBrowser.js,src/pages/fileBrowser/fileBrowser.scss,src/pages/fileBrowser/list.hbs..) directly inside the list view.list.hbstemplate to render parent directory tile (data-action="prevDir") whenprevDircondition is active.navStack.length >= 2to passprevDirflag into list template rendering.prevDiraction handler to navigate directly tonavStack.get(-2).prevDirtiles.:has(> [data-action="prevDir"])to adjust container height calculations for empty messages and spinners.Mathematical Performance & Complexity Analysis
1. Asynchronous Directory Fetching Guard
Let$T_{\text{lsDir}}$ denote the total asynchronous I/O execution latency for reading a directory listing across local storage, SAF Content URIs, FTP, or SFTP protocols, and let $T_{\text{guard}} = 15,000\text{ms}$ .
The race condition pipeline bounds latency according to:
$$T_{\text{fetch}} = \min(T_{\text{lsDir}}, T_{\text{guard}})$$
In network-constrained or unresponsive server conditions, execution is guaranteed to reject and exit within$T_{\text{guard}}$ ($15\text{s}$ ), preventing UI hangs or unresolved modal loaders.
2. Time & Space Complexity Comparisons
NavStackURL Lookup (PR #2793)has(url)Set.prototype.has)NavStackMutation (PR #2793)push(url)Set+Arraypush)has(url)/get(url)Map.prototype.get)delete obj[key])Map.prototype.delete)AbortController.abort()* Note: Plain JavaScript object lookups incur prototype chain resolution overhead and key stringification costs that are eliminated by using standard ES6
Mapkeys.Testing Plan & Quality Assurance Matrix
1. Unit & Structural Verification
NavStackClass (PR feat(file-browser): Modularize navigation history management #2793): Verifiedpush(),pop(),popUntil(),get(), andhas()behavior, ensuring parameter type checking throws explicitTypeErrorinstances on invalid inputs.MapCache Store: Verified directory entries correctly set, hit, and delete fromcachedDirwithout retaining stale references.2. Integration & Edge Case Scenarios
AbortControllercancels pending fetches; active view renders correct final directory without state leakage...tile at the top of a nested folder listing.navStack.get(-2))...tile.$page.onhidetriggers_rndrAbortCtrl.abort(), canceling pending renders cleanly.tailSpinSVG spinner renders inside list view without blocking UI dialogs.Migration & Compatibility Considerations
Backwards Compatibility & Dependencies
fileBrowser.jsremain fully compatible with existing router mounts.Conclusion
This pull request significantly modernizes the
fileBrowsersubsystem by introducing event-driven navigation history tracking (via PR #2793), race-safe rendering pipelines withAbortController, ES6Mapcaching, and explicit parent directory traversal controls.(PR name and description are AI generated (Gemini 3.6 Flash))