Conversation
There was a problem hiding this comment.
Pull request overview
Adds temporal (time-range) selection to web downloads—primarily for DigitalRF capture downloads—by introducing temporal filtering helpers, exposing timing metadata on capture API responses, and wiring a noUiSlider-based UI into the existing web download modal flow.
Changes:
- Add temporal filtering backend plumbing (helper + task/view parameters) to select a subset of capture RF data files by time bounds.
- Extend capture serializers to provide duration/cadence/bounds metadata needed to drive the slider UI.
- Integrate noUiSlider and update templates/JS to show a time-range slider and dynamic file/size labels for capture downloads.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| gateway/sds_gateway/users/views.py | Accepts start_time/end_time POST params and forwards them to the Celery download task. |
| gateway/sds_gateway/api_methods/tasks.py | Threads start_time/end_time into file collection and adds DigitalRF-only temporal filtering for capture downloads. |
| gateway/sds_gateway/api_methods/helpers/temporal_filtering.py | New helper utilities for DigitalRF filename parsing, OpenSearch bounds lookup, cadence estimation, and time-bounded file selection. |
| gateway/sds_gateway/api_methods/serializers/capture_serializers.py | Adds new timing/size/count fields used by the capture download slider UI. |
| gateway/sds_gateway/templates/base.html | Loads noUiSlider CSS/JS from CDN. |
| gateway/sds_gateway/templates/users/partials/web_download_modal.html | Adds capture-only slider UI + client-side formatting and passes time bounds in the download request. |
| gateway/sds_gateway/static/js/actions/DownloadActionManager.js | Initializes the slider from capture button data attributes and opens the web download modal for captures. |
| gateway/sds_gateway/static/js/file-list.js | Adds capture timing/size/count metadata into capture-row download button attributes (dynamic table). |
| gateway/sds_gateway/templates/users/partials/captures_page_table.html | Adds capture timing/size/count metadata into capture-row download button attributes (server-rendered table). |
| gateway/sds_gateway/templates/users/file_list.html | Includes the web download modal partial for capture downloads. |
| gateway/sds_gateway/templates/users/dataset_list.html | Includes the web download modal partial with item_type="dataset". |
| gateway/sds_gateway/templates/users/published_datasets_list.html | Includes the web download modal partial with item_type="dataset". |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gateway/sds_gateway/templates/users/partials/web_download_modal.html
Outdated
Show resolved
Hide resolved
gateway/sds_gateway/templates/users/partials/web_download_modal.html
Outdated
Show resolved
Hide resolved
| // Initialize temporal slider from button data attributes (clears or builds slider) | ||
| const durationMs = parseInt(button.getAttribute("data-length-of-capture-ms"), 10); | ||
| const fileCadenceMs = parseInt(button.getAttribute("data-file-cadence-ms"), 10); | ||
| const perDataFileSize = parseFloat(button.getAttribute("data-per-data-file-size"), 10); |
There was a problem hiding this comment.
parseFloat does not take a radix argument; the second parameter is ignored. This is misleading and suggests base conversion that won't happen. Use parseFloat(value) (or Number(...)) consistently here.
| const perDataFileSize = parseFloat(button.getAttribute("data-per-data-file-size"), 10); | |
| const perDataFileSize = parseFloat(button.getAttribute("data-per-data-file-size")); |
| """ | ||
|
|
||
| uuid = Faker("uuid4") | ||
| directory = f"/files/{self.owner.email}/{self.capture.top_level_dir}/" |
There was a problem hiding this comment.
self is not defined here, this needs to be in a method, or wrapped in a LazyAttribute
| channel = Faker("word") | ||
| capture_type = "drf" | ||
| top_level_dir = Faker("file_path", depth=2).replace("/", "_") | ||
| owner = Faker("subfactory", factory=UserFactory) | ||
| name = Faker("slug") |
There was a problem hiding this comment.
check if these attributes work
$ uv run python -c 'from factory import Faker; print(Faker("file_path", depth=2).replace("/", "_"))'
Traceback (most recent call last):
File "<string>", line 1, in <module>
from factory import Faker; print(Faker("file_path", depth=2).replace("/", "_"))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'Faker' object has no attribute 'replace'| @extend_schema_field(serializers.IntegerField) | ||
| def get_files_count(self, obj: dict[str, Any]) -> int: | ||
| def get_data_files_count(self, obj: dict[str, Any]) -> int | None: | ||
| """Get the total count of files across all channels.""" | ||
| if obj["capture_type"] != CaptureType.DigitalRF: | ||
| return None | ||
|
|
||
| total_count = 0 | ||
| for channel_data in obj["channels"]: | ||
| capture_uuid = channel_data["uuid"] | ||
| capture = Capture.objects.get(uuid=capture_uuid) | ||
| total_count += get_capture_files(capture, include_deleted=False).count() | ||
| total_count += get_data_files(capture.capture_type, capture).count() | ||
| return total_count | ||
|
|
||
| @extend_schema_field(serializers.IntegerField) | ||
| def get_data_files_total_size(self, obj: dict[str, Any]) -> int | None: | ||
| """Exact sum of data file sizes across all channels.""" | ||
| if obj["capture_type"] != CaptureType.DigitalRF: | ||
| return None | ||
| total = 0 | ||
| for channel_data in obj["channels"]: | ||
| capture_uuid = channel_data["uuid"] | ||
| capture = Capture.objects.get(uuid=capture_uuid) | ||
| data_files = get_data_files(capture.capture_type, capture) | ||
| result = data_files.aggregate(total_size=Sum("size")) | ||
| total += result.get("total_size") or 0 | ||
| return total | ||
|
|
There was a problem hiding this comment.
we're doing some rework here, maybe there's a way to return both count and size in one pass for both serialized fields
|
|
||
| def get_data_files(capture_type: CaptureType, capture: Capture) -> QuerySet[File]: | ||
| """Get the data files in the capture.""" | ||
| _catch_capture_type_error(capture_type) | ||
|
|
||
| return get_capture_files(capture).filter(name__regex=DRF_RF_FILENAME_REGEX_STR) |
There was a problem hiding this comment.
This is a hot path involving a DB call: it's called once per channel, then for each capture being serialized, and from time range computations, file cadence, and capture bounds.
See if we can refactor this call stack first, then cache results (lru_cache if a function, or django.utils.functional.cached_property if you make it a method of Capture).
The refactoring suggestion is because this and other functions in this file might make more sense as properties or methods of a Capture instance.
| data_files = get_data_files(capture.capture_type, capture) | ||
|
|
||
| if data_files.count() == 0: | ||
| return None | ||
|
|
||
| data_file_sizes = data_files.aggregate(total_size=Sum("size")) | ||
| total_size = data_file_sizes.get("total_size") | ||
|
|
||
| if not total_size: | ||
| return None | ||
|
|
||
| return float(total_size) / data_files.count() | ||
|
|
There was a problem hiding this comment.
calling 3 queries (2x count + 1x agg); see if a single query works here
stats = data_files.aggregate(
total_size=Sum("size"),
count=Count("id")
)
if stats["count"] == 0:
return None
return float(stats["total_size"]) / stats["count"]| length_of_capture_ms = serializers.SerializerMethodField() | ||
| file_cadence_ms = serializers.SerializerMethodField() | ||
| capture_start_epoch_sec = serializers.SerializerMethodField() | ||
| data_files_count = serializers.SerializerMethodField() | ||
| data_files_total_size = serializers.SerializerMethodField() | ||
| per_data_file_size = serializers.SerializerMethodField() |
There was a problem hiding this comment.
change gateway/sds_gateway/static/js/components.js after this files_count removal
| try: | ||
| start_time, end_time = get_capture_bounds(capture_type, capture_uuid) | ||
| except ValueError as e: | ||
| log.error(e) | ||
| raise e |
There was a problem hiding this comment.
try-except not needed, just let it raise, or handle it
There was a problem hiding this comment.
Ask the agent to replace all vars with consts (preferred, easier to reason about) and lets when they need to be mutable.
Also, see if it can break down these long functions here and add tests.
| function formatBytes(bytes) { | ||
| const n = Number(bytes); | ||
| if (!Number.isFinite(n) || n < 0) return "0 bytes"; | ||
| if (n === 0) return "0 bytes"; | ||
| const units = ["bytes", "KB", "MB", "GB"]; | ||
| let i = 0; | ||
| let v = n; | ||
| while (v >= 1024 && i < units.length - 1) { | ||
| v /= 1024; | ||
| i++; | ||
| } | ||
| return (i === 0 ? v : v.toFixed(2)) + " " + units[i]; | ||
| } | ||
|
|
There was a problem hiding this comment.
we have 4 definitions of this in multiple js files, let's start reusing it
| * Use web download modal (with temporal slider) when DownloadActionManager is available. | ||
| */ | ||
| handleDownloadCapture(button) { | ||
| if (window.currentDownloadManager && document.getElementById("webDownloadModal")) { |
There was a problem hiding this comment.
This silently fails if the element is not there.
We can reverse logic, raise a pretty "danger" alert to user (or at least a console.err) if element is not present and return.
Select times for web download: