[feature] Add multicolor progress bar for mass upgrade operations #349#399
Open
nishantharkut wants to merge 1 commit into
Open
[feature] Add multicolor progress bar for mass upgrade operations #349#399nishantharkut wants to merge 1 commit into
nishantharkut wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds multisegment (multicolor) progress reporting for batch upgrades. Backend WebSocket publisher now includes per-outcome counters ( Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend as Frontend (JS)
participant WS as WebSocket Consumer
participant Backend as Batch Publisher
participant DB as Batch Instance
User->>Frontend: Open batch upgrade page
Frontend->>WS: Connect & request current state
WS->>Backend: handle current state request
Backend->>DB: Query batch operations / stats
DB-->>Backend: Return operations/stats
Backend->>Backend: Compute counts (total, completed, successful, failed, aborted, cancelled)
Backend->>WS: Send batch_status payload with counts
WS-->>Frontend: batch_status payload
Frontend->>Frontend: updateBatchProgress() — build multisegment bar + legend (or fallback)
Frontend-->>User: Render progress bar and legend
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openwisp_firmware_upgrader/tests/test_websockets.py (1)
165-200: 🧹 Nitpick | 🔵 TrivialAdd a snapshot-path assertion for the new counters.
This only exercises forwarded
batch_statusevents. The page requestsrequest_current_stateon connect, so a regression inBatchUpgradeProgressConsumer._handle_current_state_request()could still ship without failing CI. Please add a test that sends{"type": "request_current_state"}and asserts the returnedbatch_state.batch_statusincludessuccessful,failed,aborted, andcancelled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/tests/test_websockets.py` around lines 165 - 200, In test_batch_upgrade_progress_consumer_connection, after establishing communicator (from _get_batch_upgrade_progress_communicator) send a JSON message with {"type": "request_current_state"} to exercise BatchUpgradeProgressConsumer._handle_current_state_request(), await the response via communicator.receive_json_from(), then assert the response contains batch_state and that batch_state["batch_status"] includes the keys "successful", "failed", "aborted", and "cancelled" (and verify their expected values, e.g. 0) to ensure the current-state request forwards the new counters.openwisp_firmware_upgrader/websockets.py (1)
251-271: 🧹 Nitpick | 🔵 TrivialUse the model’s aggregate helper for snapshot payloads.
_handle_current_state_request()now recomputes the four counters locally but still sendsbatch_operation.statusfrom the row. That gives the reconnect snapshot a different source of truth than the live publisher path, which already usesBatchUpgradeOperation.calculate_and_update_status()inopenwisp_firmware_upgrader/base/models.py:719-791. Please derive bothstatusand the counters from one shared helper here as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/websockets.py` around lines 251 - 271, In _handle_current_state_request(), don't recompute counters locally or use batch_operation.status from the DB row; instead call the model helper BatchUpgradeOperation.calculate_and_update_status() (the same aggregate used by the live publisher) to obtain the derived status and counts for operations_list, then use that returned status and counters in the payload (replace use of batch_operation.status and the local successful/failed/aborted/cancelled/completed calculations with the values returned by the helper).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js`:
- Around line 151-220: The segment widths are each rounded independently causing
totals to be off; after building the segments array in the block that creates
segments from successful/failed/aborted/cancelled, compute widths so all but the
last segment use Math.round((count/total)*100) and set the last segment's width
to 100 minus the sum of the previous rounded widths (clamp to 0..100 to avoid
negatives/overflow); update the code that renders width
(escapeHtml(String(seg.width))) to use these computed widths so the rendered
fills always sum to 100% and avoid gaps/overflow.
---
Outside diff comments:
In `@openwisp_firmware_upgrader/tests/test_websockets.py`:
- Around line 165-200: In test_batch_upgrade_progress_consumer_connection, after
establishing communicator (from _get_batch_upgrade_progress_communicator) send a
JSON message with {"type": "request_current_state"} to exercise
BatchUpgradeProgressConsumer._handle_current_state_request(), await the response
via communicator.receive_json_from(), then assert the response contains
batch_state and that batch_state["batch_status"] includes the keys "successful",
"failed", "aborted", and "cancelled" (and verify their expected values, e.g. 0)
to ensure the current-state request forwards the new counters.
In `@openwisp_firmware_upgrader/websockets.py`:
- Around line 251-271: In _handle_current_state_request(), don't recompute
counters locally or use batch_operation.status from the DB row; instead call the
model helper BatchUpgradeOperation.calculate_and_update_status() (the same
aggregate used by the live publisher) to obtain the derived status and counts
for operations_list, then use that returned status and counters in the payload
(replace use of batch_operation.status and the local
successful/failed/aborted/cancelled/completed calculations with the values
returned by the helper).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 75fd549f-dda9-4bf7-a79c-ac051d84d960
📒 Files selected for processing (4)
openwisp_firmware_upgrader/static/firmware-upgrader/css/batch-upgrade-operation.cssopenwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.jsopenwisp_firmware_upgrader/tests/test_websockets.pyopenwisp_firmware_upgrader/websockets.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2026-02-25T11:20:13.903Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Applied to files:
openwisp_firmware_upgrader/tests/test_websockets.py
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_websockets.py
📚 Learning: 2026-02-24T00:04:04.187Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-options.css:11-13
Timestamp: 2026-02-24T00:04:04.187Z
Learning: In openwisp-firmware-upgrader, prefer hiding empty submit rows via CSS (e.g., `#upgradeoperation_form` .submit-row { display: none; }) rather than removing elements in templates. This keeps template logic simple and can improve rendering performance. Apply this pattern to CSS files in openwisp_firmware_upgrader/static/firmware-upgrader/css/*.css for consistent behavior across upgrade-option related styles.
Applied to files:
openwisp_firmware_upgrader/static/firmware-upgrader/css/batch-upgrade-operation.css
📚 Learning: 2026-02-26T17:22:26.110Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: :0-0
Timestamp: 2026-02-26T17:22:26.110Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, avoid making suggestions about UI/frontend code (JavaScript DOM traversal, CSS selectors, jQuery operations) unless there is a clear, objective bug (e.g., syntax error, undefined variable). UI code structure and DOM traversal paths should be verified through manual testing by the maintainers, as suggestions without access to the actual rendered DOM can break functionality.
Applied to files:
openwisp_firmware_upgrader/static/firmware-upgrader/css/batch-upgrade-operation.css
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
Applied to files:
openwisp_firmware_upgrader/websockets.py
📚 Learning: 2026-03-07T01:16:24.447Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
Applied to files:
openwisp_firmware_upgrader/websockets.py
nishantharkut
force-pushed
the
issue/349-multicolor-progressBar
branch
2 times, most recently
from
b6a8cde to
aa742c2
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_firmware_upgrader/tests/test_websockets.py (1)
165-200: 🧹 Nitpick | 🔵 TrivialCover the initial
batch_stateresponse too.This only asserts a synthetic
batch_statuspush. The page requestsrequest_current_state()immediately after connect, and the new counters are also assembled inBatchUpgradeProgressConsumer._handle_current_state_request(), so the first-render path can regress without failing here.🧪 Suggested coverage
+ await communicator.send_json_to({"type": "request_current_state"}) + response = await communicator.receive_json_from() + self.assertEqual(response["type"], "batch_state") + self.assertEqual(response["batch_status"]["successful"], 0) + self.assertEqual(response["batch_status"]["failed"], 0) + self.assertEqual(response["batch_status"]["aborted"], 0) + self.assertEqual(response["batch_status"]["cancelled"], 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/tests/test_websockets.py` around lines 165 - 200, The test test_batch_upgrade_progress_consumer_connection only asserts the synthetic group_send batch_status message and misses asserting the initial batch_state response sent when the consumer handles request_current_state() on connect; update the test to also receive and assert the initial message produced by BatchUpgradeProgressConsumer._handle_current_state_request() (verify keys like "type" == "batch_state" or the initial counters: completed, total, successful, failed, aborted, cancelled) before sending the synthetic group message so the first-render path is covered.
♻️ Duplicate comments (1)
openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (1)
190-206:⚠️ Potential issue | 🟠 MajorThe segment remainder can still go negative.
Line 195 subtracts individually rounded widths from a once-rounded
progressPercentage. Withsuccessful=failed=aborted=cancelled=1andtotal=200, the first three segments already consume3%whileprogressPercentageis2%, so the last segment becomes-1%and the bar misrenders.🛠️ Safer width calculation
- let usedWidth = 0; + let usedWidth = 0; + let cumulativeCount = 0; let segmentsHtml = segments - .map(function (seg, index) { - let segmentWidth = - index === segments.length - 1 - ? progressPercentage - usedWidth - : Math.round((seg.count / total) * 100); - usedWidth += segmentWidth; + .map(function (seg) { + cumulativeCount += seg.count; + let cumulativeWidth = Math.round((cumulativeCount / total) * 100); + let segmentWidth = Math.max(cumulativeWidth - usedWidth, 0); + usedWidth = cumulativeWidth; return ( '<div class="upgrade-progress-fill ' + escapeHtml(seg.cssClass) + '" style="width: ' + escapeHtml(String(segmentWidth)) +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js` around lines 190 - 206, The segment width calculation can produce a negative value for the last segment because it subtracts rounded per-segment widths from a once-rounded progressPercentage; update the mapping logic in the segmentsHtml generation (the map callback that computes usedWidth and segmentWidth) to ensure the last segment width is computed as the non-negative remainder (e.g., Math.max(0, progressPercentage - usedWidth)) or by calculating widths from unrounded fractions and only rounding at the end; keep using the same variables (usedWidth, segmentWidth, progressPercentage, total, segments, escapeHtml) and ensure segmentWidth is never negative before building the HTML.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@openwisp_firmware_upgrader/tests/test_websockets.py`:
- Around line 165-200: The test test_batch_upgrade_progress_consumer_connection
only asserts the synthetic group_send batch_status message and misses asserting
the initial batch_state response sent when the consumer handles
request_current_state() on connect; update the test to also receive and assert
the initial message produced by
BatchUpgradeProgressConsumer._handle_current_state_request() (verify keys like
"type" == "batch_state" or the initial counters: completed, total, successful,
failed, aborted, cancelled) before sending the synthetic group message so the
first-render path is covered.
---
Duplicate comments:
In
`@openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js`:
- Around line 190-206: The segment width calculation can produce a negative
value for the last segment because it subtracts rounded per-segment widths from
a once-rounded progressPercentage; update the mapping logic in the segmentsHtml
generation (the map callback that computes usedWidth and segmentWidth) to ensure
the last segment width is computed as the non-negative remainder (e.g.,
Math.max(0, progressPercentage - usedWidth)) or by calculating widths from
unrounded fractions and only rounding at the end; keep using the same variables
(usedWidth, segmentWidth, progressPercentage, total, segments, escapeHtml) and
ensure segmentWidth is never negative before building the HTML.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a38ae84-3a06-4811-bfb2-333b95817ab0
📒 Files selected for processing (4)
openwisp_firmware_upgrader/static/firmware-upgrader/css/batch-upgrade-operation.cssopenwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.jsopenwisp_firmware_upgrader/tests/test_websockets.pyopenwisp_firmware_upgrader/websockets.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
Applied to files:
openwisp_firmware_upgrader/websockets.py
📚 Learning: 2026-03-07T01:16:24.447Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
Applied to files:
openwisp_firmware_upgrader/websockets.py
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_websockets.py
📚 Learning: 2026-02-24T00:04:04.187Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-options.css:11-13
Timestamp: 2026-02-24T00:04:04.187Z
Learning: In openwisp-firmware-upgrader, prefer hiding empty submit rows via CSS (e.g., `#upgradeoperation_form` .submit-row { display: none; }) rather than removing elements in templates. This keeps template logic simple and can improve rendering performance. Apply this pattern to CSS files in openwisp_firmware_upgrader/static/firmware-upgrader/css/*.css for consistent behavior across upgrade-option related styles.
Applied to files:
openwisp_firmware_upgrader/static/firmware-upgrader/css/batch-upgrade-operation.css
📚 Learning: 2026-02-26T17:22:26.110Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: :0-0
Timestamp: 2026-02-26T17:22:26.110Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, avoid making suggestions about UI/frontend code (JavaScript DOM traversal, CSS selectors, jQuery operations) unless there is a clear, objective bug (e.g., syntax error, undefined variable). UI code structure and DOM traversal paths should be verified through manual testing by the maintainers, as suggestions without access to the actual rendered DOM can break functionality.
Applied to files:
openwisp_firmware_upgrader/static/firmware-upgrader/css/batch-upgrade-operation.cssopenwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js
📚 Learning: 2026-02-25T00:17:23.479Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
Applied to files:
openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js
nishantharkut
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js`:
- Line 210: Fix the typo in the inline comment for the legend building function:
change the comment text "legend builing function" to "legend building function"
in batch-upgrade-progress.js (the comment above the legend building function in
batch-upgrade-progress.js).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d12949ac-9d87-4cde-8231-30bc145aaeb7
📒 Files selected for processing (3)
openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.jsopenwisp_firmware_upgrader/tests/test_websockets.pyopenwisp_firmware_upgrader/websockets.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
Applied to files:
openwisp_firmware_upgrader/websockets.py
📚 Learning: 2026-03-07T01:16:24.447Z
Learnt from: atif09
Repo: openwisp/openwisp-firmware-upgrader PR: 387
File: openwisp_firmware_upgrader/admin.py:420-423
Timestamp: 2026-03-07T01:16:24.447Z
Learning: In openwisp-firmware-upgrader, UpgradeOperation and BatchUpgradeOperation always share the same organization (BatchUpgradeOperation.build.category.organization). A user who can view an UpgradeOperation can always view its related BatchUpgradeOperation. Model-level has_view_permission(request) is sufficient for batch breadcrumb visibility in UpgradeOperationAdmin.change_view; object-level permission checks are not needed.
Applied to files:
openwisp_firmware_upgrader/websockets.py
📚 Learning: 2026-02-25T11:20:13.903Z
Learnt from: pandafy
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: runtests.py:27-38
Timestamp: 2026-02-25T11:20:13.903Z
Learning: In openwisp-firmware-upgrader, runtests.py now uses `execute_from_command_line()` to run Django tests in-process rather than via subprocess, so the `patch = subprocess` coverage directive is not needed in pyproject.toml.
Applied to files:
openwisp_firmware_upgrader/tests/test_websockets.py
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_websockets.py
📚 Learning: 2026-02-25T00:17:23.479Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
Applied to files:
openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js
📚 Learning: 2026-02-26T17:22:26.110Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: :0-0
Timestamp: 2026-02-26T17:22:26.110Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, avoid making suggestions about UI/frontend code (JavaScript DOM traversal, CSS selectors, jQuery operations) unless there is a clear, objective bug (e.g., syntax error, undefined variable). UI code structure and DOM traversal paths should be verified through manual testing by the maintainers, as suggestions without access to the actual rendered DOM can break functionality.
Applied to files:
openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js
🔇 Additional comments (7)
openwisp_firmware_upgrader/tests/test_websockets.py (2)
185-208: LGTM! Good test coverage for the new per-status counters.The test correctly verifies:
- The batch_status message includes the new
successful,failed,aborted, andcancelledfields- The
request_current_stateWebSocket path returns these counters in thebatch_stateresponseThis ensures the frontend can rely on these fields being present.
420-437: LGTM! Publisher test correctly validates new parameters.The test verifies that
publish_batch_statusproperly includes the new keyword arguments in the emitted WebSocket data, with correct values (successful=4, failed=1, aborted=0, cancelled=0).openwisp_firmware_upgrader/websockets.py (2)
251-265: LGTM! Clean integration withcalculate_and_update_status.The code correctly:
- Uses
sync_to_asyncto call the synchronous model method from async context- Unpacks the returned
(status, stats)tuple- Accesses all required stats keys which are guaranteed to exist from the model's aggregate query
This centralizes status calculation logic in the model while keeping the WebSocket layer focused on publishing.
501-536: LGTM! Backward-compatible signature extension.The new keyword arguments with default values of
0ensure backward compatibility if any code callspublish_batch_statuswithout the new parameters. Theupdate_batch_statusmethod correctly forwards all stats to the publisher.openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (3)
190-208: LGTM! Cumulative width calculation correctly prevents rounding gaps.The algorithm accumulates counts and computes cumulative widths, ensuring segments always sum to exactly
progressPercentage. UsingMath.max(..., 0)provides a defensive guard against any edge cases. This properly addresses the previous review concern about independent rounding causing gaps or overflow.
155-189: LGTM! Robust guard conditions for multicolor rendering.The condition
data.successful !== undefined && total > 0 && statusCountSum > 0correctly ensures:
- Backend sent per-status data (not falling back to old payload format)
- No division by zero
- At least one status has counts to render
The segments array construction with defensive
|| 0defaults handles any missing fields gracefully.
234-256: LGTM! Clean fallback path for backward compatibility.When per-status counts are unavailable (e.g., older cached data or intermediate states), the code falls back to single-color rendering based on
data.status. This ensures graceful degradation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js`:
- Around line 190-208: The segment width calculation in the segments.map block
uses total when computing cumulativeWidth, causing segment percentages to be
scaled incorrectly during in-progress batches; change the divisor from total to
statusCountSum in the cumulativeWidth computation (i.e., compute cumulativeWidth
= Math.round((cumulativeCount / statusCountSum) * progressPercentage)) and add a
guard so if statusCountSum is 0 you treat cumulativeWidth as 0 (to avoid
division-by-zero). Update references inside the segments map (usedWidth,
cumulativeCount, segmentWidth, progressPercentage, escapeHtml) accordingly so
the segmentsHtml sums correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 88352358-08b3-4558-8ab8-22bb102321c6
📒 Files selected for processing (1)
openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-02-25T00:17:23.479Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
Applied to files:
openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js
📚 Learning: 2026-02-26T17:22:26.110Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: :0-0
Timestamp: 2026-02-26T17:22:26.110Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, avoid making suggestions about UI/frontend code (JavaScript DOM traversal, CSS selectors, jQuery operations) unless there is a clear, objective bug (e.g., syntax error, undefined variable). UI code structure and DOM traversal paths should be verified through manual testing by the maintainers, as suggestions without access to the actual rendered DOM can break functionality.
Applied to files:
openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js
🔇 Additional comments (3)
openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (3)
143-155: LGTM!The setup properly guards against division by zero and the condition for entering multicolor mode is appropriately defensive.
210-227: LGTM!Legend building correctly escapes all dynamic content and the structure matches the expected output.
234-263: LGTM!The fallback single-color path correctly handles terminal states and the final HTML construction properly concatenates the legend when present.
nishantharkut
force-pushed
the
issue/349-multicolor-progressBar
branch
2 times, most recently
from
cdb014e to
dcadd47
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_firmware_upgrader/tests/test_websockets.py (1)
185-208: 🧹 Nitpick | 🔵 TrivialUse distinct non-zero counters in this consumer test.
This still only exercises the empty-batch case, so a key-mapping regression in
BatchUpgradeProgressConsumer._handle_current_state_request()would pass because every new field is0. Seed at least one non-zero outcome here, or stubcalculate_and_update_status()with distinct values, so thebatch_stateassertion proves each field is wired correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/tests/test_websockets.py` around lines 185 - 208, The test currently only asserts zeros so it doesn't validate per-field wiring; update the test to seed at least one non-zero counter (e.g., set successful=1 and failed=1 or similar) before sending the current-state request, or alternatively stub BatchUpgradeProgressConsumer.calculate_and_update_status() to return a dict with distinct non-zero values, then assert those specific values in the received batch_state to ensure _handle_current_state_request() maps each field correctly.
♻️ Duplicate comments (1)
openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js (1)
190-199:⚠️ Potential issue | 🟠 MajorScale segment widths by completed outcomes, not
total.At Line 196,
progressPercentagealready representscompleted / total. Dividing the cumulative outcome counts bytotalagain makes in-progress batches underfill the bar badly; for example, 5 completed out of 10 renders only 25% of the bar instead of 50%. UsestatusCountSumas the divisor when converting the completed-status counts into widths.🐛 Proposed fix
- let cumulativeWidth = Math.round( - (cumulativeCount / total) * progressPercentage, - ); + let cumulativeWidth = Math.round( + (cumulativeCount / statusCountSum) * progressPercentage, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js` around lines 190 - 199, The segment width calculation in the segments.map block uses total as the divisor when computing cumulativeWidth, which double-counts total because progressPercentage already equals completed/total; change the divisor to statusCountSum so cumulativeWidth = Math.round((cumulativeCount / statusCountSum) * progressPercentage) (update the calculation inside the segments.map where cumulativeCount, usedWidth and cumulativeWidth are computed) to scale segment widths by completed outcomes rather than by total.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@openwisp_firmware_upgrader/tests/test_websockets.py`:
- Around line 185-208: The test currently only asserts zeros so it doesn't
validate per-field wiring; update the test to seed at least one non-zero counter
(e.g., set successful=1 and failed=1 or similar) before sending the
current-state request, or alternatively stub
BatchUpgradeProgressConsumer.calculate_and_update_status() to return a dict with
distinct non-zero values, then assert those specific values in the received
batch_state to ensure _handle_current_state_request() maps each field correctly.
---
Duplicate comments:
In
`@openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js`:
- Around line 190-199: The segment width calculation in the segments.map block
uses total as the divisor when computing cumulativeWidth, which double-counts
total because progressPercentage already equals completed/total; change the
divisor to statusCountSum so cumulativeWidth = Math.round((cumulativeCount /
statusCountSum) * progressPercentage) (update the calculation inside the
segments.map where cumulativeCount, usedWidth and cumulativeWidth are computed)
to scale segment widths by completed outcomes rather than by total.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5aefe10e-55a1-4037-8f3a-2487e369c93c
📒 Files selected for processing (4)
openwisp_firmware_upgrader/static/firmware-upgrader/css/batch-upgrade-operation.cssopenwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.jsopenwisp_firmware_upgrader/tests/test_websockets.pyopenwisp_firmware_upgrader/websockets.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2026-02-24T00:04:04.187Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/css/upgrade-options.css:11-13
Timestamp: 2026-02-24T00:04:04.187Z
Learning: In openwisp-firmware-upgrader, prefer hiding empty submit rows via CSS (e.g., `#upgradeoperation_form` .submit-row { display: none; }) rather than removing elements in templates. This keeps template logic simple and can improve rendering performance. Apply this pattern to CSS files in openwisp_firmware_upgrader/static/firmware-upgrader/css/*.css for consistent behavior across upgrade-option related styles.
Applied to files:
openwisp_firmware_upgrader/static/firmware-upgrader/css/batch-upgrade-operation.css
📚 Learning: 2026-02-26T17:22:26.110Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 0
File: :0-0
Timestamp: 2026-02-26T17:22:26.110Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, avoid making suggestions about UI/frontend code (JavaScript DOM traversal, CSS selectors, jQuery operations) unless there is a clear, objective bug (e.g., syntax error, undefined variable). UI code structure and DOM traversal paths should be verified through manual testing by the maintainers, as suggestions without access to the actual rendered DOM can break functionality.
Applied to files:
openwisp_firmware_upgrader/static/firmware-upgrader/css/batch-upgrade-operation.cssopenwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js
📚 Learning: 2026-02-27T19:08:56.218Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 383
File: openwisp_firmware_upgrader/tests/test_admin.py:50-50
Timestamp: 2026-02-27T19:08:56.218Z
Learning: In tests for openwisp-firmware-upgrader, derive app_label values from model meta instead of hard-coding. Specifically, let BaseTestAdmin compute app_label from Build._meta.app_label and config_app_label from Device._meta.app_label to ensure tests remain correct if models are swapped. This pattern should apply to all test files under openwisp_firmware_upgrader/tests.
Applied to files:
openwisp_firmware_upgrader/tests/test_websockets.py
📚 Learning: 2026-02-23T21:44:41.470Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/websockets.py:233-246
Timestamp: 2026-02-23T21:44:41.470Z
Learning: In openwisp-firmware-upgrader, the `upgrade_operations` attribute on `BatchUpgradeOperation` is a cached property that returns a queryset, so it should be accessed as `batch_operation.upgrade_operations.all()` rather than being called as a method.
Applied to files:
openwisp_firmware_upgrader/websockets.py
📚 Learning: 2026-02-25T00:17:23.479Z
Learnt from: nemesifier
Repo: openwisp/openwisp-firmware-upgrader PR: 377
File: openwisp_firmware_upgrader/static/firmware-upgrader/js/upgrade-progress.js:458-463
Timestamp: 2026-02-25T00:17:23.479Z
Learning: In the openwisp/openwisp-firmware-upgrader repository, focus review comments on real, actionable issues rather than theoretical edge cases, trivialities, or potential future problems. Avoid flagging issues that are functionally correct but could theoretically become problems only if the code were significantly refactored or used in different contexts.
Applied to files:
openwisp_firmware_upgrader/static/firmware-upgrader/js/batch-upgrade-progress.js
🔇 Additional comments (3)
openwisp_firmware_upgrader/websockets.py (1)
251-265: Good end-to-end wiring of the new batch counters.The current-state response and live
batch_statusupdates now forward the same per-outcome fields from the same status-calculation path, which keeps the websocket payload shape consistent for the frontend.Also applies to: 501-535
openwisp_firmware_upgrader/static/firmware-upgrader/css/batch-upgrade-operation.css (1)
203-235: The new selectors line up with the generated multisegment markup.The wrapped layout, segment end-cap rules, and
.legend-dot.{success|failed|aborted|cancelled}styles match the classes emitted bybatch-upgrade-progress.js, so the stacked bar has the CSS hooks it needs on both desktop and mobile.Also applies to: 272-307
openwisp_firmware_upgrader/tests/test_websockets.py (1)
420-437: Good publisher-level coverage for the expanded payload.This locks down the new
successful/failed/aborted/cancelledfields at the wire-format boundary, which is the right place to catch accidental payload regressions.
nishantharkut
force-pushed
the
issue/349-multicolor-progressBar
branch
from
dcadd47 to
0a5051b
Compare
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
1 participant
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.
Checklist
Reference to Existing Issue
Closes #349 .
Description of Changes
my design thinking :
Screenshot
Before :
After :