Fix: improve root directory modal behavior and UX #2453
Conversation
Walkthrough
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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.
| try { | ||
| const content = await sdk | ||
| .forProject(page.params.region, page.params.project) | ||
| .vcs.getRepositoryContents({ | ||
| installationId: $installation.$id, | ||
| providerRepositoryId: $repository.id, | ||
| providerRootDirectory: path, | ||
| providerReference: branch | ||
| }); | ||
| const fileCount = content.contents?.length ?? 0; | ||
| const contentDirectories = content.contents.filter((e) => e.isDirectory); | ||
| if (contentDirectories.length === 0) { | ||
| expandedStore.update((exp) => [...new Set([...exp, path])]); | ||
| return; | ||
| } | ||
| targetDir.fileCount = fileCount; | ||
There was a problem hiding this comment.
Set fileCount before early return and guard .filter usage
Empty directories never set fileCount, so loadPath will keep refetching that path. Also guard against undefined contents.
- const fileCount = content.contents?.length ?? 0;
- const contentDirectories = content.contents.filter((e) => e.isDirectory);
+ const contents = content.contents ?? [];
+ const fileCount = contents.length;
+ const contentDirectories = contents.filter((e) => e.isDirectory);
+
+ // mark as loaded regardless of emptiness
+ targetDir.fileCount = fileCount;
- if (contentDirectories.length === 0) {
+ if (contentDirectories.length === 0) {
expandedStore.update((exp) => [...new Set([...exp, path])]);
return;
}
- targetDir.fileCount = fileCount;
+ // children exist...Also applies to: 147-156
🤖 Prompt for AI Agents
In src/lib/components/git/selectRootModal.svelte around lines 137 to 156, the
code reads content.contents then filters it but returns early for empty
directories without setting targetDir.fileCount and doesn't guard against
undefined contents; compute fileCount first using optional chaining (e.g. const
fileCount = content.contents?.length ?? 0), assign targetDir.fileCount before
any early return, use a safe array for filtering (e.g. const contentDirectories
= (content.contents ?? []).filter(...)), and then update expandedStore and
return if no directories remain so fileCount is always set.
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
Chores