Fix: improve root directory modal behavior and UX#2453
Fix: improve root directory modal behavior and UX#2453HarshMN2345 wants to merge 5 commits intomainfrom
Conversation
WalkthroughReplaces onMount-driven initialization in Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/lib/components/git/selectRootModal.svelte (4)
181-185: Harden path normalization (leading and duplicate slashes)Current logic can produce
.//foofor inputs like/foo. Collapse duplicates and strip leading slashes.- function normalizePath(path: string): string { - if (!path || path === './') return './'; - const trimmed = path.replace(/\/$/, ''); - return trimmed.startsWith('./') ? trimmed : `./${trimmed}`; - } + function normalizePath(path: string): string { + if (!path || path === './') return './'; + let p = path.trim(); + p = p.replace(/^\/+/, ''); // remove leading slashes + p = p.replace(/\/+$/, ''); // remove trailing slashes + p = p.replace(/\/{2,}/g, '/'); // collapse duplicate slashes + if (p === '' || p === '.') return './'; + return p.startsWith('./') ? p : `./${p}`; + }
193-211: Avoid shadowingcurrentPathlocal vs stateLocal
let currentPath = './'shadows the state variable, which is confusing and risks bugs. Use a different local name.- let currentDir = directories[0]; - let currentPath = './'; + let currentDir = directories[0]; + let accPath = './'; for (const segment of segments) { - currentPath = currentPath === './' ? `./${segment}` : `${currentPath}/${segment}`; + accPath = accPath === './' ? `./${segment}` : `${accPath}/${segment}`; // Load the parent directory if not already loaded await loadPath(currentDir.fullPath); // Find the next directory const nextDir = currentDir.children?.find((d) => d.title === segment); if (!nextDir) return; // Path doesn't exist currentDir = nextDir; - expandedStore.update((exp) => [...new Set([...exp, currentPath])]); + expandedStore.update((exp) => [...new Set([...exp, accPath])]); } - currentPath = normalized; + // set selection to final normalized path + currentPath = normalized;
129-131: Nit: fix comment typo"donee" → "done". Improves readability.
- // skip loading if this directory was donee + // skip loading if this directory was done
223-229: Optional: reset expanded state on closeConsider clearing
expandedStorewhen closing so reopening starts from a clean tree (unless intentional to persist expansions).$effect(() => { if (!show && initialized) { initialized = false; + expandedStore.set([]); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
package.json(1 hunks)src/lib/components/git/selectRootModal.svelte(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
🔇 Additional comments (4)
package.json (1)
27-29: Pins for pink packages updated — verify compatibility and lockfile integrityUpgrading to aa2ed6e looks fine. Please:
- Ensure these builds are compatible with Svelte 5 and DirectoryPicker API used here.
- Regenerate pnpm-lock and run e2e/visual checks for icon/component regressions.
src/lib/components/git/selectRootModal.svelte (3)
2-2: Confirm$app/stateimportSvelteKit typically exposes
pagefrom$app/stores. Ensure$app/stateis valid for your Kit version (2.42.1). If not, switch to$app/stores.
21-31:branchis required — ensure it’s always providedNo default is set for
branch, yet it’s used in API calls. Verify parent always passes it to avoid runtime errors.
248-259: Save UX check looks goodDisabling Save when
isLoadingor no changes (!hasChanges) matches the PR goal.Please verify DirectoryPicker’s
expandedexpects a writable store (not an array) and thatopenToaccepts normalized paths as implemented.
| directories[0] = { | ||
| ...directories[0], | ||
| fileCount: content.contents?.length ?? 0, | ||
| children: content.contents | ||
| .filter((e) => e.isDirectory) | ||
| .map((dir) => ({ | ||
| title: dir.name, | ||
| fullPath: `./${dir.name}`, | ||
| fileCount: undefined, | ||
| // set logo for root directories | ||
| thumbnailUrl: dir.name, | ||
| loading: false | ||
| })) | ||
| }; |
There was a problem hiding this comment.
Prevent crash when contents is undefined
You use ?.length but then call .filter/.map on content.contents directly. If contents is absent, this will throw at runtime.
Apply:
- directories[0] = {
+ const contents = content.contents ?? [];
+ directories[0] = {
...directories[0],
- fileCount: content.contents?.length ?? 0,
- children: content.contents
+ fileCount: contents.length,
+ children: contents
.filter((e) => e.isDirectory)
.map((dir) => ({
title: dir.name,
fullPath: `./${dir.name}`,
fileCount: undefined,
// set logo for root directories
thumbnailUrl: dir.name,
loading: false
}))
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| directories[0] = { | |
| ...directories[0], | |
| fileCount: content.contents?.length ?? 0, | |
| children: content.contents | |
| .filter((e) => e.isDirectory) | |
| .map((dir) => ({ | |
| title: dir.name, | |
| fullPath: `./${dir.name}`, | |
| fileCount: undefined, | |
| // set logo for root directories | |
| thumbnailUrl: dir.name, | |
| loading: false | |
| })) | |
| }; | |
| const contents = content.contents ?? []; | |
| directories[0] = { | |
| ...directories[0], | |
| fileCount: contents.length, | |
| children: contents | |
| .filter((e) => e.isDirectory) | |
| .map((dir) => ({ | |
| title: dir.name, | |
| fullPath: `./${dir.name}`, | |
| fileCount: undefined, | |
| // set logo for root directories | |
| thumbnailUrl: dir.name, | |
| loading: false | |
| })) | |
| }; |
🤖 Prompt for AI Agents
In src/lib/components/git/selectRootModal.svelte around lines 88 to 101, the
code uses content.contents?.length but then calls .filter/.map on
content.contents directly which will throw if contents is undefined; change the
children construction to operate on a safe default (e.g. const entries =
content.contents ?? [] and use entries.filter(...).map(...)) so children is
built from an empty array when contents is absent and keep fileCount as
content.contents?.length ?? 0.
…heck-root-drectory-modal-issue
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/lib/components/git/selectRootModal.svelte`:
- Around line 128-129: Fix the typo in the inline comment inside the async
function loadPath: change the comment text from "// skip loading if this
directory was donee" to a correct phrasing such as "// skip loading if this
directory was done" (or similar clear wording) so the comment accurately
describes the intent in the loadPath function.
- Around line 187-211: The function expandToPath declares a local let
currentPath which shadows the component-level reactive currentPath, making the
final assignment (currentPath = normalized) a no-op; rename the local
accumulator (e.g., localCurrentPath or pathAccumulator) inside expandToPath and
update every reference within the function to use that new name, then ensure the
final assignment assigns normalized to the outer/reactive currentPath (or remove
it if not intended) so the component state is actually updated; references:
function expandToPath, local variable currentPath (line ~194), outer reactive
currentPath (component-level).
- Around line 230-236: Replace the loose CustomEvent handler with
DirectoryPicker's typed callback: change the component prop from
on:select={handleSelect} to onSelect={handleSelect} and update the handleSelect
function signature to accept a DirectorySelectDetail (imported or referenced)
instead of CustomEvent; inside handleSelect use detail.fullPath (or fullPath
directly if destructuring), keep the isFetching guard, set currentPath, and
await loadPath(path) — ensure the function is async if you await loadPath;
adjust imports/types to reference DirectorySelectDetail and keep references to
currentPath, isFetching, loadPath, and DirectoryPicker.
| async function loadPath(path: string) { | ||
| // skip loading if this directory was donee |
There was a problem hiding this comment.
Typo in comment: "donee" → "done"
Line 129: // skip loading if this directory was donee
🤖 Prompt for AI Agents
In `@src/lib/components/git/selectRootModal.svelte` around lines 128 - 129, Fix
the typo in the inline comment inside the async function loadPath: change the
comment text from "// skip loading if this directory was donee" to a correct
phrasing such as "// skip loading if this directory was done" (or similar clear
wording) so the comment accurately describes the intent in the loadPath
function.
| async function expandToPath(path: string) { | ||
| const normalized = normalizePath(path); | ||
| const segments = normalized.split('/').filter((s) => s !== '.' && s !== ''); | ||
|
|
||
| expandedStore.update((exp) => [...new Set([...exp, './'])]); | ||
|
|
||
| let currentDir = directories[0]; | ||
| let currentPath = './'; | ||
|
|
||
| for (const segment of segments) { | ||
| currentPath = currentPath === './' ? `./${segment}` : `${currentPath}/${segment}`; | ||
|
|
||
| // Load the parent directory if not already loaded | ||
| await loadPath(currentDir.fullPath); | ||
|
|
||
| // Find the next directory | ||
| const nextDir = currentDir.children?.find((d) => d.title === segment); | ||
| if (!nextDir) return; // Path doesn't exist | ||
|
|
||
| currentDir = nextDir; | ||
| expandedStore.update((exp) => [...new Set([...exp, currentPath])]); | ||
| } | ||
|
|
||
| currentPath = normalized; | ||
| } |
There was a problem hiding this comment.
currentPath on line 194 shadows the outer $state variable
The local let currentPath declared on line 194 shadows the component-level $state currentPath from line 44. This makes the assignment on line 210 (currentPath = normalized) dead code — it writes to the local variable, which is immediately discarded when the function returns. The outer reactive state is unaffected.
While the caller at line 218 sets the outer currentPath before calling expandToPath, this shadowing is confusing and error-prone. Rename the local accumulator.
Proposed fix
async function expandToPath(path: string) {
const normalized = normalizePath(path);
const segments = normalized.split('/').filter((s) => s !== '.' && s !== '');
expandedStore.update((exp) => [...new Set([...exp, './'])]);
let currentDir = directories[0];
- let currentPath = './';
+ let accumulatedPath = './';
for (const segment of segments) {
- currentPath = currentPath === './' ? `./${segment}` : `${currentPath}/${segment}`;
+ accumulatedPath = accumulatedPath === './' ? `./${segment}` : `${accumulatedPath}/${segment}`;
// Load the parent directory if not already loaded
await loadPath(currentDir.fullPath);
// Find the next directory
const nextDir = currentDir.children?.find((d) => d.title === segment);
if (!nextDir) return; // Path doesn't exist
currentDir = nextDir;
- expandedStore.update((exp) => [...new Set([...exp, currentPath])]);
+ expandedStore.update((exp) => [...new Set([...exp, accumulatedPath])]);
}
-
- currentPath = normalized;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function expandToPath(path: string) { | |
| const normalized = normalizePath(path); | |
| const segments = normalized.split('/').filter((s) => s !== '.' && s !== ''); | |
| expandedStore.update((exp) => [...new Set([...exp, './'])]); | |
| let currentDir = directories[0]; | |
| let currentPath = './'; | |
| for (const segment of segments) { | |
| currentPath = currentPath === './' ? `./${segment}` : `${currentPath}/${segment}`; | |
| // Load the parent directory if not already loaded | |
| await loadPath(currentDir.fullPath); | |
| // Find the next directory | |
| const nextDir = currentDir.children?.find((d) => d.title === segment); | |
| if (!nextDir) return; // Path doesn't exist | |
| currentDir = nextDir; | |
| expandedStore.update((exp) => [...new Set([...exp, currentPath])]); | |
| } | |
| currentPath = normalized; | |
| } | |
| async function expandToPath(path: string) { | |
| const normalized = normalizePath(path); | |
| const segments = normalized.split('/').filter((s) => s !== '.' && s !== ''); | |
| expandedStore.update((exp) => [...new Set([...exp, './'])]); | |
| let currentDir = directories[0]; | |
| let accumulatedPath = './'; | |
| for (const segment of segments) { | |
| accumulatedPath = accumulatedPath === './' ? `./${segment}` : `${accumulatedPath}/${segment}`; | |
| // Load the parent directory if not already loaded | |
| await loadPath(currentDir.fullPath); | |
| // Find the next directory | |
| const nextDir = currentDir.children?.find((d) => d.title === segment); | |
| if (!nextDir) return; // Path doesn't exist | |
| currentDir = nextDir; | |
| expandedStore.update((exp) => [...new Set([...exp, accumulatedPath])]); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/lib/components/git/selectRootModal.svelte` around lines 187 - 211, The
function expandToPath declares a local let currentPath which shadows the
component-level reactive currentPath, making the final assignment (currentPath =
normalized) a no-op; rename the local accumulator (e.g., localCurrentPath or
pathAccumulator) inside expandToPath and update every reference within the
function to use that new name, then ensure the final assignment assigns
normalized to the outer/reactive currentPath (or remove it if not intended) so
the component state is actually updated; references: function expandToPath,
local variable currentPath (line ~194), outer reactive currentPath
(component-level).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/components/git/selectRootModal.svelte (1)
264-267:⚠️ Potential issue | 🔴 CriticalFix
rootDirformat to match API expectations
handleSubmitsetsrootDir = currentPath, which uses/-prefixed paths (e.g.,/src). However, thetoProviderPathfunction in this component converts paths to./format (e.g.,./src) before sending to APIs, indicating that's the expected format. Parent components receivingrootDirvia binding pass it directly to API calls without conversion, which will cause a breaking change. Either:
- Have
handleSubmitapplytoProviderPath()before setting rootDir, or- Export
toProviderPathand ensure parent components use it when passingrootDirto APIs.
🤖 Fix all issues with AI agents
In `@src/lib/components/git/selectRootModal.svelte`:
- Around line 234-236: The loop calling loadPath(pathsToExpand) launches
concurrent loads that cause a race where parent loadPath responses replace
targetDir.children and orphan child mutations; change the parallel invocation to
sequential awaits so each path is fully loaded before the next starts (use an
async for...of or explicit await in the loop) and ensure you call
getDirByPath/loadPath in that awaited sequence so the child load sees the
parent’s updated Directory objects instead of mutating stale placeholders
(references: loadPath, getDirByPath, Directory, targetDir.children).
🧹 Nitpick comments (1)
src/lib/components/git/selectRootModal.svelte (1)
274-282:{#keytreeVersion}destroys and recreatesDirectoryPickeron every load
bumpTreeVersion()is called each time a directory finishes loading (lines 127, 188, 232), which destroys the entireDirectoryPickerand recreates it. This will reset scroll position, lose focus state, and may cause visible flicker during incremental navigation.If this is a workaround for
DirectoryPickernot reacting to in-place array mutations, consider restructuring thedirectoriesstate to produce a new array reference on updates (e.g.,directories = [...directories]) instead, and remove the{#key}block. That way the component can diff rather than fully remount.
| for (const pathToLoad of pathsToExpand) { | ||
| loadPath(pathToLoad); | ||
| } |
There was a problem hiding this comment.
Race condition: concurrent loadPath calls corrupt the directory tree
The for loop fires all loadPath calls without await, so they run concurrently. When a parent's loadPath resolves, it replaces targetDir.children with new Directory objects (line 187). Meanwhile, a child's loadPath still holds a reference to the old placeholder object (captured by getDirByPath before the parent's response). The child then mutates the orphaned placeholder — those changes are never reflected in the tree.
Sequential loading is required here since child paths depend on the parent's loaded state.
Proposed fix
- for (const pathToLoad of pathsToExpand) {
- loadPath(pathToLoad);
- }
+ for (const pathToLoad of pathsToExpand) {
+ await loadPath(pathToLoad);
+ }🤖 Prompt for AI Agents
In `@src/lib/components/git/selectRootModal.svelte` around lines 234 - 236, The
loop calling loadPath(pathsToExpand) launches concurrent loads that cause a race
where parent loadPath responses replace targetDir.children and orphan child
mutations; change the parallel invocation to sequential awaits so each path is
fully loaded before the next starts (use an async for...of or explicit await in
the loop) and ensure you call getDirByPath/loadPath in that awaited sequence so
the child load sees the parent’s updated Directory objects instead of mutating
stale placeholders (references: loadPath, getDirByPath, Directory,
targetDir.children).
1 participant
What does this PR do?
Auto-opens root and navigates to typed path, move to svlete5 , disable save when unchanged
update in pink for selection appwrite/pink#368
Test Plan
in discord
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
yes
Summary by CodeRabbit
New Features
Bug Fixes
Refactor