Skip to content

frontend: settings: ask for confirmation when removing logs#3825

Open
nicoschmdt wants to merge 1 commit intobluerobotics:masterfrom
nicoschmdt:remove-logs-ask-confirmation
Open

frontend: settings: ask for confirmation when removing logs#3825
nicoschmdt wants to merge 1 commit intobluerobotics:masterfrom
nicoschmdt:remove-logs-ask-confirmation

Conversation

@nicoschmdt
Copy link
Contributor

@nicoschmdt nicoschmdt commented Mar 12, 2026

fix: #1486

Summary by Sourcery

Add a confirmation step before clearing system and MAVLink logs from the settings view to prevent accidental deletion.

New Features:

  • Introduce a reusable warning dialog for confirming log clearing actions in the settings view.

Enhancements:

  • Track pending log clear type to show context-specific confirmation text for system vs MAVLink logs and reset this state after operations complete.

@sourcery-ai
Copy link

sourcery-ai bot commented Mar 12, 2026

Reviewer's Guide

Adds a confirmation dialog before clearing service or MAVLink logs in the Settings view, wiring the existing log removal actions through a shared warning-dialog with type-specific titles and messages and ensuring state is reset after operations complete.

Sequence diagram for new log clear confirmation flow

sequenceDiagram
  actor User
  participant SettingsView
  participant WarningDialog
  participant ServiceLogRemoval
  participant MavlinkLogRemoval

  User->>SettingsView: Click Clear service logs button
  SettingsView->>SettingsView: pending_log_clear_type = service
  SettingsView->>SettingsView: show_log_clear_confirm = true
  SettingsView->>WarningDialog: Bind v_model(show_log_clear_confirm), title, message
  User->>WarningDialog: Click Clear logs (confirm)
  WarningDialog-->>SettingsView: confirm event
  SettingsView->>SettingsView: onConfirmClearLogs()
  alt pending_log_clear_type is service
    SettingsView->>ServiceLogRemoval: remove_service_log_files()
    ServiceLogRemoval-->>SettingsView: deletion_complete
  else pending_log_clear_type is mavlink
    SettingsView->>MavlinkLogRemoval: remove_mavlink_log_files()
    MavlinkLogRemoval-->>SettingsView: deletion_complete
  end
  SettingsView->>SettingsView: show_log_clear_confirm = false
  SettingsView->>SettingsView: pending_log_clear_type = null
Loading

Class diagram for updated SettingsView log clearing logic

classDiagram
  class SettingsView {
    <<VueComponent>>
    // data
    bool show_log_clear_confirm
    string|null pending_log_clear_type
    string current_deletion_path
    number current_deletion_size
    string current_deletion_status
    bool operation_in_progress

    // computed
    string log_clear_confirm_title()
    string log_clear_confirm_message()

    // methods
    void onConfirmClearLogs()
    Promise reset_settings()
    Promise remove_service_log_files()
    Promise remove_mavlink_log_files()
  }

  class WarningDialog {
    <<VueComponent>>
    // props
    bool value
    string title
    string message
    string confirmLabel

    // events
    void confirm()
    void input(bool newValue)
  }

  SettingsView "1" o-- "1" WarningDialog : uses_v_model_and_confirm

  class ServiceLogRemoval {
    void remove_service_log_files()
  }

  class MavlinkLogRemoval {
    void remove_mavlink_log_files()
  }

  SettingsView ..> ServiceLogRemoval : delegates_service_log_clear
  SettingsView ..> MavlinkLogRemoval : delegates_mavlink_log_clear

  class LogClearState {
    bool show_log_clear_confirm
    string|null pending_log_clear_type
    void reset_state()
  }

  SettingsView *-- LogClearState : manages_confirmation_state
Loading

File-Level Changes

Change Details Files
Route service and MAVLink log deletion through a shared confirmation dialog instead of calling deletion methods directly from buttons.
  • Change service log clear button click handler to open confirmation dialog and mark pending type as service.
  • Change MAVLink log clear button click handler to open confirmation dialog and mark pending type as mavlink.
  • Add a warning-dialog instance bound to new state, with a shared confirm handler for both log types.
core/frontend/src/views/SettingsView.vue
Introduce state and computed helpers to drive the new confirmation dialog content based on the pending log type.
  • Add show_log_clear_confirm and pending_log_clear_type to component data with appropriate union typing.
  • Add computed title and message getters that vary text depending on whether service or MAVLink logs are pending deletion.
core/frontend/src/views/SettingsView.vue
Ensure confirmation dialog and pending type state are cleared after log deletion completes or aborts.
  • Add onConfirmClearLogs method that calls the appropriate log removal method based on pending_log_clear_type.
  • Reset show_log_clear_confirm and pending_log_clear_type in log removal completion handlers to avoid stale state.
core/frontend/src/views/SettingsView.vue

Assessment against linked issues

Issue Objective Addressed Explanation
#1486 Require a user confirmation step before removing system/service log files in the settings UI.
#1486 Require a user confirmation step before removing MAVLink log files in the settings UI.

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 1 issue, and left some high level feedback:

  • The click handlers for the log delete buttons inline multiple state mutations (show_log_clear_confirm and pending_log_clear_type); consider moving this into a small method (e.g. openLogClearConfirm('service' | 'mavlink')) to keep the template simpler and reduce duplication.
  • The confirmation dialog state is reset inconsistently: for service logs it is only cleared when current_deletion_status === 'complete', whereas MAVLink clears it unconditionally after the operation; aligning these to always clear show_log_clear_confirm and pending_log_clear_type regardless of success/failure would avoid the dialog getting stuck in an inconsistent state.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The click handlers for the log delete buttons inline multiple state mutations (`show_log_clear_confirm` and `pending_log_clear_type`); consider moving this into a small method (e.g. `openLogClearConfirm('service' | 'mavlink')`) to keep the template simpler and reduce duplication.
- The confirmation dialog state is reset inconsistently: for service logs it is only cleared when `current_deletion_status === 'complete'`, whereas MAVLink clears it unconditionally after the operation; aligning these to always clear `show_log_clear_confirm` and `pending_log_clear_type` regardless of success/failure would avoid the dialog getting stuck in an inconsistent state.

## Individual Comments

### Comment 1
<location path="core/frontend/src/views/SettingsView.vue" line_range="715-717" />
<code_context>
         this.current_deletion_path = ''
         this.current_deletion_size = 0
         this.current_deletion_status = ''
+        this.show_log_clear_confirm = false
+        this.pending_log_clear_type = null
       }

</code_context>
<issue_to_address>
**issue (bug_risk):** Resetting the log clear dialog only on successful service deletion is inconsistent with MAVLink and may leave the dialog open after failures.

For service logs, `show_log_clear_confirm` and `pending_log_clear_type` are only reset when `current_deletion_status === 'finished'`, unlike the MAVLink flow where they’re reset unconditionally. If deletion fails or is interrupted, the dialog and pending type can remain set, confusing users. Please move these resets to a path that always runs (e.g., with `this.operation_in_progress = false` or in a `finally`), matching the MAVLink behavior.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines 715 to +717
this.current_deletion_status = ''
this.show_log_clear_confirm = false
this.pending_log_clear_type = null
Copy link

Choose a reason for hiding this comment

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

issue (bug_risk): Resetting the log clear dialog only on successful service deletion is inconsistent with MAVLink and may leave the dialog open after failures.

For service logs, show_log_clear_confirm and pending_log_clear_type are only reset when current_deletion_status === 'finished', unlike the MAVLink flow where they’re reset unconditionally. If deletion fails or is interrupted, the dialog and pending type can remain set, confusing users. Please move these resets to a path that always runs (e.g., with this.operation_in_progress = false or in a finally), matching the MAVLink behavior.

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.

Remove logs tool should ask for confirmation

1 participant