-
Notifications
You must be signed in to change notification settings - Fork 253
Fix: improve root directory modal behavior and UX #2453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
c3fb402
45e9a40
5c31ce2
16f9f46
0181088
7f31579
aecdfd5
c849eed
9f13711
523f228
affe37f
0079071
5d4a44a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,141 +7,260 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { installation, repository } from '$lib/stores/vcs'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { VCSDetectionType, type Models } from '@appwrite.io/console'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { DirectoryPicker } from '@appwrite.io/pink-svelte'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { onMount } from 'svelte'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { writable } from 'svelte/store'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type Directory = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| title: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fullPath: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fileCount: number; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| thumbnailUrl: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fileCount?: number; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| thumbnailUrl?: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| children?: Directory[]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| loading?: boolean; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export let show = false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export let rootDir: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export let product: 'sites' | 'functions' = 'functions'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export let branch: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| show = $bindable(false), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| rootDir = $bindable(''), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| product = 'functions' as 'sites' | 'functions', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| branch | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| show?: boolean; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| rootDir?: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| product?: 'sites' | 'functions'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| branch: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } = $props(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let isLoading = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let directories: Directory[] = [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let isLoading = $state(true); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let directories = $state<Directory[]>([ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| title: 'Root', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fullPath: './', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fullPath: '/', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fileCount: 0, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| thumbnailUrl: 'root', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| thumbnailUrl: $iconPath('empty', 'grayscale'), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| children: [], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| loading: false | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let currentPath: string = './'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let currentDir: Directory; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export let expanded = writable(['lib-0', 'tree-0']); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let currentPath = $state('/'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let expandedStore = writable<string[]>([]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let initialized = $state(false); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let treeVersion = $state(0); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let initialPath = $state('/'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const inFlightPaths = new Set<string>(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| $effect(() => { | |
| // Clear caches when the modal is closed or when the repository, | |
| // installation, or branch context changes to avoid stale data and | |
| // excessive memory usage. | |
| // Using `show`, `$repository`, `$installation`, and `branch` here | |
| // makes them dependencies of this effect. | |
| if (!show || !$repository || !$installation) { | |
| inFlightPaths.clear(); | |
| contentsCache.clear(); | |
| iconCache.clear(); | |
| } | |
| }); |
Copilot
AI
Feb 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The effect on lines 230-262 launches an async IIFE that may continue executing even after the component unmounts or the effect re-runs. This can lead to race conditions, stale state updates, or errors. Consider using an AbortController or a cleanup function that sets a flag to prevent stale updates, especially since it modifies state like directories, isLoading, and expandedPaths.
Copilot
AI
Feb 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error on line 311 is logged to the console but doesn't provide any user feedback or update the UI to reflect the error state. Users won't know that loading a specific directory failed. Consider adding error state management and displaying an error indicator in the UI, or disabling further interactions with that directory until a retry succeeds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
Copilot
AI
Feb 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The effect on lines 362-370 calls an async function expandToPath without awaiting it or handling potential errors. If the effect re-runs or the component unmounts while expandToPath is still executing, it could lead to race conditions or stale state updates. Consider using a cleanup mechanism (e.g., AbortController) or ensuring that state updates only occur if the effect is still active.
Copilot
AI
Feb 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The effect on lines 373-377 only resets the initialized flag when the modal closes but doesn't reset other state variables like directories, currentPath, expandedPaths, isLoading, or clear the caches. This means if the user opens the modal again, stale state from the previous session may be displayed. Consider adding a more comprehensive reset that includes clearing all state and caches to ensure a fresh start each time the modal opens.
| initialized = false; | |
| initialized = false; | |
| directories = []; | |
| currentPath = ''; | |
| initialPath = ''; | |
| expandedPaths = []; | |
| isLoading = false; |
Copilot
AI
Feb 19, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handleSelect function on lines 379-383 calls loadPath asynchronously but doesn't await the result or handle potential errors. If loadPath fails or takes a long time, the user won't receive any feedback. Consider awaiting the loadPath call and adding error handling, or at least showing loading state while the path is being loaded.
| loadPath(path); | |
| try { | |
| isLoading = true; | |
| await loadPath(path); | |
| } catch (error) { | |
| console.error('Failed to load path in handleSelect:', error); | |
| } finally { | |
| isLoading = false; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
loadPath is not awaited in handleSelect, silently swallowing errors.
handleSelect is declared async but calls loadPath(path) without await. Any rejection from loadPath (network error, etc.) is lost. The fix also removes the now-redundant as string cast (see proposed change for the double-callback comment above).
🐛 Proposed fix
async function handleSelect(detail: { fullPath: string }) {
- const path = detail.fullPath as string;
- currentPath = path;
- loadPath(path);
+ await loadPath(detail.fullPath);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/components/git/selectRootModal.svelte` around lines 379 - 383, The
handler handleSelect currently calls loadPath(path) without awaiting so any
rejection is swallowed; change it to set currentPath = detail.fullPath (remove
the redundant "as string" cast) and await loadPath(currentPath) so errors
propagate; optionally wrap the await in a try/catch inside handleSelect if you
need to log or handle failures instead of bubbling them up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Directory type defined in selectRootModal.svelte (lines 12-20) is similar to but not identical to the DirectoryEntry type in types.ts. Directory is missing
thumbnailIcon,thumbnailHtml, andshowThumbnailfields that are present in DirectoryEntry. This type inconsistency could lead to issues if these components expect different shapes. Consider using the DirectoryEntry type from types.ts throughout to ensure consistency.