refactor(file-browser): Overhaul navigation state, directory caching, async rendering, and parent navigation - #2500
Conversation
Greptile SummaryThis PR overhauds the
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant FB as FileBrowser
participant NS as NavStack
participant RC as renderCurrentDir
participant GD as getDirList
participant DOM as DOM
U->>FB: click folder/navigate
FB->>NS: push(url, name) or popUntil(url)
Note over NS: queueMicrotask → "update" event
FB->>RC: renderCurrentDir()
RC->>RC: abort previous _rndrAbortCtrl
RC->>DOM: "remove old #list, save scroll"
alt cached
RC->>DOM: render from cachedDir immediately
else not cached
RC->>DOM: append placeholder + spinner
RC->>GD: await getDirList(url)
GD->>GD: Promise.race([lsDir(), 15s timeout])
alt success
GD-->>RC: list[]
RC->>DOM: replace placeholder with list
RC->>RC: cachedDir.set(url, dir)
else error / timeout
GD-->>RC: throw Error
RC->>RC: "restore currentDir = _dir"
RC->>DOM: replace placeholder with errMsg
end
alt aborted (new navigate fired)
RC->>RC: return early, abandon DOM update
end
end
Note over NS: microtask fires
NS-->>FB: ""update" event {added, removed}"
FB->>DOM: remove nav elements for removed URLs
FB->>DOM: pushToNavbar for added URLs
FB->>FB: actionStack entries for back navigation
Reviews (35): 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
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
|
@greptile review commit
|
e036de8 to
81ac2ab
Compare
14801eb to
a55b05d
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)
a55b05d to
1e5f381
Compare
| const { promise: p2, reject } = Promise.withResolvers(); | ||
| const timeout = setTimeout(() => { | ||
| reject(new Error("Directory loading timed out.")); | ||
| }, 15000); | ||
| try { | ||
| list = await Promise.race([p1, p2]); | ||
| } finally { |
There was a problem hiding this comment.
Promise.withResolvers() not polyfilled — crashes all non-root directory loads on old WebViews
Promise.withResolvers() was added to core-js in v3.37.0, but .babelrc declares "corejs": "3.22". With "useBuiltIns": "entry", Babel builds the polyfill import list against the declared version, so APIs introduced in v3.23–v3.49 are never included. On any Android WebView below Chrome 119 (the first version to ship native support, October 2023), every call to getDirList for a non-root URL throws TypeError: Promise.withResolvers is not a function, leaving the user stuck at the root storage list with no visible error.
The polyfill mismatch can be fixed either by bumping "corejs" to "3.49" in .babelrc, or by inlining the resolvers manually (a two-line let resolve, reject; const p2 = new Promise((r, rj) => { resolve = r; reject = rj; });).
…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)
ce0de1b to
c95f515
Compare
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))