-
Notifications
You must be signed in to change notification settings - Fork 260
Clean up unused projects when migration setup is cancelled #3201
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
base: main
Are you sure you want to change the base?
Changes from all commits
ed6e8d5
a2f05e3
0f4fa4a
3ac1341
b6d10cc
c35280d
6297c1f
a005ed3
0ca760b
0137b47
ce2396d
2700a17
d63306c
58d1154
52f7ed4
c5d5c2e
66569f4
21e5a2c
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 |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest'; | ||
| import { get } from 'svelte/store'; | ||
| import { wizard } from './wizard'; | ||
|
|
||
| vi.mock('$lib/actions/analytics', () => ({ trackEvent: vi.fn() })); | ||
|
|
||
| describe('wizard exit handler ownership', () => { | ||
| beforeEach(() => wizard.hide()); | ||
|
|
||
| it('unregisters the handler owned by the closing component', () => { | ||
| const handler = vi.fn(); | ||
| 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(); | ||
|
|
||
|
Comment on lines
+12
to
+47
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests directly inspect the private 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 AIThis 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! |
||
| expect(get(wizard).show).toBe(false); | ||
| expect(get(wizard).exitHandler).toBeNull(); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| <p>Choose migration resources</p> |
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.
When the user presses browser Back, this code cancels the navigation and passes
nullto 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