[backport] [1.4] update delete log to use stream#3806
Open
nicoschmdt wants to merge 3 commits intobluerobotics:1.4from
Open
[backport] [1.4] update delete log to use stream#3806nicoschmdt wants to merge 3 commits intobluerobotics:1.4from
nicoschmdt wants to merge 3 commits intobluerobotics:1.4from
Conversation
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on March 15. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
Reviewer's GuideImplements a streaming-based system log deletion API in the commander service and wires it to the Settings menu, adding a live progress UI, cancellation when the dialog closes, and backend support for per-file deletion reporting. Sequence diagram for streaming system log deletionsequenceDiagram
actor User
participant SettingsMenu
participant Axios as Axios_back_axios
participant Commander as CommanderService
participant Utils as delete_everything_stream
User->>SettingsMenu: Click delete system logs
SettingsMenu->>SettingsMenu: Set deletion_in_progress = true
SettingsMenu->>Axios: POST /commander/v1.0/services/remove_log_stream
Axios->>Commander: HTTP POST remove_log_services_stream
Commander->>Utils: delete_everything_stream(LOG_FOLDER_PATH)
loop For each file or directory
Utils-->>Commander: DeletionInfo dict (JSON-serializable)
Commander-->>Axios: JSON fragment in streaming response
Axios-->>SettingsMenu: onDownloadProgress(progressEvent)
SettingsMenu->>SettingsMenu: parseStreamingResponse
SettingsMenu->>SettingsMenu: Update current_deletion_path
SettingsMenu->>SettingsMenu: Update current_deletion_size
SettingsMenu->>SettingsMenu: Update current_deletion_total_size
SettingsMenu->>SettingsMenu: Update current_deletion_status
end
Commander-->>Axios: Stream complete
Axios-->>SettingsMenu: Request resolved
SettingsMenu->>SettingsMenu: deletion_in_progress = false
SettingsMenu->>SettingsMenu: Reset deletion state
SettingsMenu->>Axios: GET /commander/v1.0/services/log_folder_size
Axios-->>SettingsMenu: Updated folder size
SettingsMenu-->>User: Updated free space and final status
Sequence diagram for cancelling streaming log deletion on dialog closesequenceDiagram
actor User
participant SettingsMenu
participant Axios as Axios_back_axios
User->>SettingsMenu: Close Settings dialog
SettingsMenu->>SettingsMenu: show_dialog = false
SettingsMenu->>SettingsMenu: Watcher on show_dialog triggered
alt dialog closed and deletion in progress
SettingsMenu->>Axios: deletion_log_abort_controller.cancel()
Axios-->>SettingsMenu: Request cancelled (CancelToken)
SettingsMenu->>SettingsMenu: deletion_in_progress = false
SettingsMenu->>SettingsMenu: Reset deletion state
else dialog not closed or no deletion
SettingsMenu->>SettingsMenu: No action
end
Class diagram for deletion streaming and settings menu updatesclassDiagram
class DeletionInfo {
+str path
+int size
+str type
+bool success
+dict to_dict()
}
class GeneralUtils {
+async delete_everything_stream(path)
+delete_everything(path)
+file_is_open(path)
}
class CommanderService {
+async remove_log_services(i_know_what_i_am_doing)
+async remove_log_services_stream(i_know_what_i_am_doing)
+async remove_mavlink_log_services(i_know_what_i_am_doing)
}
class SettingsMenuComponent {
<<VueComponent>>
-bool show_dialog
-bool operation_in_progress
-str operation_description
-str operation_error
-bool deletion_in_progress
-deletion_log_abort_controller
-str current_deletion_path
-int current_deletion_size
-int current_deletion_total_size
-str current_deletion_status
-bool disable_remove
+bool has_error
+mounted()
+prepare_operation(description)
+formatSize(bytes)
+download_service_log_files()
+get_log_folder_size()
+get_mavlink_log_folder_size()
+remove_service_log_files()
+remove_mavlink_log_files()
}
GeneralUtils <|.. DeletionInfo : uses
CommanderService ..> GeneralUtils : calls delete_everything_stream
SettingsMenuComponent ..> CommanderService : calls remove_log_services_stream via HTTP
SettingsMenuComponent ..> DeletionInfo : consumes JSON fields in stream
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
remove_service_log_files, usingresult.last()andreduce(...)without an explicit initial value can break in environments withoutArray.prototype.lastand will throw whenresultis empty; consider replacing withresult[result.length - 1]andreduce((acc, fragment) => ..., 0). - The new
delete_everything_streamimplementation walks directories and deletes files but never removes the directories themselves, which changes the behavior fromdelete_everything(and may leave empty folder trees behind); if you intend to fully clear the log folder, you may want tormdirdirectories once their contents are processed. - The
remove_log_services_streamendpoint advertisesapplication/x-ndjsonbut the generator only yields JSON strings without explicit newline delimiters; verify thatstreameris actually inserting\nbetween items or add them in the generator to conform to NDJSON expectations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `remove_service_log_files`, using `result.last()` and `reduce(...)` without an explicit initial value can break in environments without `Array.prototype.last` and will throw when `result` is empty; consider replacing with `result[result.length - 1]` and `reduce((acc, fragment) => ..., 0)`.
- The new `delete_everything_stream` implementation walks directories and deletes files but never removes the directories themselves, which changes the behavior from `delete_everything` (and may leave empty folder trees behind); if you intend to fully clear the log folder, you may want to `rmdir` directories once their contents are processed.
- The `remove_log_services_stream` endpoint advertises `application/x-ndjson` but the generator only yields JSON strings without explicit newline delimiters; verify that `streamer` is actually inserting `\n` between items or add them in the generator to conform to NDJSON expectations.
## Individual Comments
### Comment 1
<location path="core/frontend/src/components/app/SettingsMenu.vue" line_range="329" />
<code_context>
+ onDownloadProgress: (progressEvent) => {
+ let result = parseStreamingResponse(progressEvent.currentTarget.response)
+ result = result.filter((fragment) => fragment.fragment !== -1)
+ const last_fragment = result.last()
+ const total_deleted = result
+ .reduce((acc, fragment) => acc + (JSON.parse(fragment?.data ?? '{}')?.size ?? 0), 0)
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `Array.prototype.last()` may not be supported in all target browsers/environments.
Depending on your runtime targets and TS lib settings, `Array.prototype.last()` may be missing or incorrectly typed. To avoid compatibility issues, use an index-based access instead, for example:
```ts
const last_fragment = result.length > 0 ? result[result.length - 1] : undefined
```
</issue_to_address>
### Comment 2
<location path="core/frontend/src/components/app/SettingsMenu.vue" line_range="311-316" />
<code_context>
},
async remove_service_log_files(): Promise<void> {
- this.prepare_operation('Removing system log files...')
+ this.deletion_log_abort_controller = axios.CancelToken.source()
+ this.deletion_in_progress = true
+ this.current_deletion_path = '...'
+ this.current_deletion_size = 0
+ this.current_deletion_total_size = 0
+ this.current_deletion_status = 'Starting deletion...'
- await back_axios({
</code_context>
<issue_to_address>
**question (bug_risk):** `operation_in_progress` is no longer set when starting log deletion but is still cleared in `finally`.
`remove_service_log_files` used to set `operation_in_progress = true` via `prepare_operation`, but now only the `finally` block sets it back to `false`. This means the flag may never be set to `true` for this operation, which can break any UI that relies on it. Either restore `prepare_operation` (or set `operation_in_progress` directly) or remove this flag handling here to keep the state consistent.
</issue_to_address>
### Comment 3
<location path="core/libs/commonwealth/commonwealth/utils/general.py" line_range="141-143" />
<code_context>
+ success=True
+ ).to_dict()
+ # fmt: on
+ if item.is_dir() and not item.is_symlink():
+ # Delete folder contents
+ async for info in delete_everything_stream(item):
+ yield info
+ except Exception as exception:
</code_context>
<issue_to_address>
**issue (bug_risk):** Directories are traversed but not actually removed, which may leave empty directory structures behind.
In `delete_everything_stream`, directories are only recursed into (`delete_everything_stream(item)`) and never removed once emptied, unlike `delete_everything`. If the goal is a full cleanup, also delete the directory after processing its contents and optionally yield a `DeletionInfo` for it.
</issue_to_address>
### Comment 4
<location path="core/services/commander/main.py" line_range="190-191" />
<code_context>
+
+ async def generate() -> AsyncGenerator[str, None]:
+ try:
+ async for info in delete_everything_stream(Path(LOG_FOLDER_PATH)):
+ yield json.dumps(info)
+ except Exception as error:
+ logger.error(f"Error during log deletion stream: {error}")
</code_context>
<issue_to_address>
**issue (bug_risk):** The NDJSON streaming output lacks explicit newline delimiters between records.
Since the endpoint returns `application/x-ndjson`, clients will expect one JSON object per line, delimited by `\n`. Here you’re yielding `json.dumps(info)` without a newline, so consumers may see an undelimited JSON stream. Please ensure each record is newline-terminated, e.g. `yield json.dumps(info) + "\n"` (or equivalent in `streamer`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
backporting #3250 to BlueOS 1.4
Summary by Sourcery
Stream log deletion progress from the backend to the Settings UI and expose it as a new streaming delete endpoint.
New Features:
Enhancements: