Skip to content

Add a dialog to clear storage when changing pipeline config#5871

Open
Karakatiza666 wants to merge 3 commits intomainfrom
config-clear-ui
Open

Add a dialog to clear storage when changing pipeline config#5871
Karakatiza666 wants to merge 3 commits intomainfrom
config-clear-ui

Conversation

@Karakatiza666
Copy link
Contributor

Screencast.from.2026-03-19.20-42-50.webm

@Karakatiza666 Karakatiza666 marked this pull request as ready for review March 23, 2026 15:24
Copy link

@mythical-fred mythical-fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dialog system refactor (GenericDialog, useGlobalDialog) and the new clear-storage flow (doClearAndApply, showClearStorageConfirm, phase state machine) are behavioral changes with no tests. Per the hard rule for web-console PRs: please add tests before this can be approved.

@@ -1,7 +1,11 @@
<script lang="ts">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doClearAndApply async flow has three phases (confirmprogresserror) and fallback error paths — this is exactly the kind of business logic that belongs in tests. Please add Vitest + @testing-library/svelte tests covering at least:

  1. Happy path: doClearAndApply succeeds → dialog closes, pipeline.patch called with the pending patch
  2. Error path: postPipelineAction rejects → clearStoragePhase becomes error, message shown
  3. isStorageNotClearedError: unit test the error-detection predicate in isolation

Setup: npm install -D vitest @testing-library/svelte jsdom

@Karakatiza666
Copy link
Contributor Author

Karakatiza666 commented Mar 23, 2026

Testing: after the refactor I manually tested various interactions of all dialogs:

  1. Pipeline configuration dialog
  2. Support bundle download
  3. Samply profile download
  4. API key menu
  5. Pipeline actions confirmations (clear storage, delete, etc.) on Pipeline Edit and Pipeline List pages

onSuccess?: DialogAction
/**
* Called when the user clicks the cancel button, the "X" button, or clicks away from the dialog.
* If `name` is provided, it overrides the default "Cancel" button label.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a field in DialogAction struct

@Karakatiza666 Karakatiza666 force-pushed the config-clear-ui branch 2 times, most recently from 89fbdff to 30de585 Compare March 24, 2026 09:46
@Karakatiza666 Karakatiza666 added this pull request to the merge queue Mar 25, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 25, 2026
@Karakatiza666 Karakatiza666 added this pull request to the merge queue Mar 25, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 25, 2026
@Karakatiza666 Karakatiza666 added this pull request to the merge queue Mar 25, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 25, 2026
@Karakatiza666 Karakatiza666 added this pull request to the merge queue Mar 25, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 25, 2026
Copy link

@mythical-fred mythical-fred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -0,0 +1 @@
# Web Console Library

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this file contains only a heading with no content. Per PR_REVIEW_RULES.md, AGENTS.md/CLAUDE.md files in subdirectories should be driven by a specific observed struggle, not added speculatively. If there is a real need here, add the actual guidance. Otherwise, a README.md would serve humans and agents equally well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added by mistake, removed

Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
Replaced duplicated implementations with GenericDialog instances

Improve API key menu behavior when there are a lot of API keys

Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
@Karakatiza666 Karakatiza666 added this pull request to the merge queue Mar 26, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 26, 2026
@Karakatiza666 Karakatiza666 added this pull request to the merge queue Mar 26, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 26, 2026
@Karakatiza666 Karakatiza666 added this pull request to the merge queue Mar 26, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 26, 2026
…anges

Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
@Karakatiza666 Karakatiza666 added this pull request to the merge queue Mar 26, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants