Clean up unused projects when migration setup is cancelled - #3201
HarshMN2345 wants to merge 18 commits into
Conversation
Console (appwrite/console)Project ID: Tip Git integration provides automatic deployments with optional PR comments |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if ($wizard.exitHandler) { | ||
| navigation.cancel(); | ||
| } | ||
| if (navigation.type !== 'leave') { | ||
| wizard.hide(); | ||
| $wizard.exitHandler( | ||
| navigation.type === 'popstate' ? null : (navigation.to?.url.href ?? null) | ||
| ); |
There was a problem hiding this comment.
When the user presses browser Back, this code cancels the navigation and passes null to the exit handler. After cleanup succeeds, the wizard closes, but no destination exists to resume the canceled history traversal. The user therefore remains on the same route and must press Back again.
Knowledge Base Used: Console application shell
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/layout/shell.svelte
Line: 114-118
Comment:
**Back Navigation Is Lost**
When the user presses browser Back, this code cancels the navigation and passes `null` to the exit handler. After cleanup succeeds, the wizard closes, but no destination exists to resume the canceled history traversal. The user therefore remains on the same route and must press Back again.
**Knowledge Base Used:** [Console application shell](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/console/-/docs/console-application-shell.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| const unregister = wizard.setExitHandler(handler); | ||
|
|
||
| expect(get(wizard).exitHandler).toBe(handler); | ||
| unregister(); | ||
|
|
||
| expect(get(wizard).exitHandler).toBeNull(); | ||
| }); | ||
|
|
||
| it('does not unregister a replacement component handler', () => { | ||
| const unregister = wizard.setExitHandler(vi.fn()); | ||
| const replacement = vi.fn(); | ||
| wizard.setExitHandler(replacement); | ||
|
|
||
| unregister(); | ||
|
|
||
| expect(get(wizard).exitHandler).toBe(replacement); | ||
| }); | ||
|
|
||
| it('clears the previous handler when another wizard starts', () => { | ||
| const unregister = wizard.setExitHandler(vi.fn()); | ||
|
|
||
| wizard.start(() => ({})); | ||
| expect(get(wizard).exitHandler).toBeNull(); | ||
|
|
||
| const replacement = vi.fn(); | ||
| wizard.setExitHandler(replacement); | ||
| unregister(); | ||
| expect(get(wizard).exitHandler).toBe(replacement); | ||
| }); | ||
|
|
||
| it('clears navigation interception when the wizard is hidden', () => { | ||
| wizard.start(() => ({})); | ||
| wizard.setExitHandler(vi.fn()); | ||
|
|
||
| wizard.hide(); | ||
|
|
There was a problem hiding this comment.
These tests directly inspect the private exitHandler store field and mirror its registration and replacement logic instead of testing observable wizard behavior. The migration suite repeats this problem by invoking get(wizard).exitHandler(...) and asserting private store state. This violates the repository directive to test observable behavior rather than source implementation, so the tests must be rewritten before merging to exercise navigation through a mounted wizard.
Context Used: Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/stores/wizard.test.ts
Line: 12-47
Comment:
**Tests Mirror Private State**
These tests directly inspect the private `exitHandler` store field and mirror its registration and replacement logic instead of testing observable wizard behavior. The migration suite repeats this problem by invoking `get(wizard).exitHandler(...)` and asserting private store state. This violates the repository directive to test observable behavior rather than source implementation, so the tests must be rewritten before merging to exercise navigation through a mounted wizard.
**Context Used:** Call out and harshly judge implementation-coupled tests. We don't mirror source code, configuration, or version pins in assertions. We test observable behavior; use linters for syntax and schema checks. ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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!
1 participant
Cancelling migration setup after creating a destination project left the project behind. Going back to edit the setup and continuing also created another project each time.
Track the project created by the wizard, reuse it while its settings match, and delete an unused destination before confirming exit or replacing it. Wait for in-flight creation and deletion, use the returned project region and ID, and keep the wizard open when cleanup fails so the user can retry. Once an import request is attempted, retain the destination even if the response fails because data may already be importing.
Route browser Back and internal links through the same exit confirmation and cleanup. Resume an internal link after successful cleanup; successful migration submission closes the wizard before navigating to its destination. Only wizards with an exit cleanup callback register the navigation handler.
Closes appwrite/appwrite#7685.
Validation:
Cleanup covers confirmed wizard exits and internal navigation. Tab close, reload and external page unload cannot await asynchronous cleanup.