feat(downloader): report release download progress via listener#76
Merged
Conversation
Stream the release asset in chunks and notify a DownloadProgressListener with bytes read and total size, so clients can render a real progress bar instead of an indefinite spinner. The existing downloadIfNeeded overload keeps working via a no-op listener. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CXTAaR4opi34idMG7ZZ8BV
|
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:
WalkthroughДобавлены клиент релизов GitHub, интерфейс прогресса скачивания и новый потоковый путь загрузки ассетов с таймаутом. Тесты обновлены под новый API и сценарии прогресса. ChangesПрогресс скачивания BSL Language Server
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 (1)
src/main/java/com/github/_1c_syntax/utils/downloader/BslLanguageServerDownloader.java (1)
278-302: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftДобавить отдельный таймаут на чтение тела ответа
request.timeout(DOWNLOAD_TIMEOUT)в JDK 21 ограничивает только ожидание до получения заголовков. СBodyHandlers.ofInputStream()тело читается уже вcopyToFile, иinput.read(...)может зависнуть без дедлайна при медленном/оборванном соединении. Нужен отдельный read-timeout или асинхронная загрузка с отменой.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/github/_1c_syntax/utils/downloader/BslLanguageServerDownloader.java` around lines 278 - 302, The download(String, Path, DownloadProgressListener) flow only applies DOWNLOAD_TIMEOUT to waiting for response headers, so body reads in copyToFile can still hang indefinitely. Add a separate read timeout for the response body or switch the download path to an async/cancellable approach, and update the download/copyToFile handling in BslLanguageServerDownloader so the body transfer is also bounded by a deadline.
🧹 Nitpick comments (1)
src/test/java/com/github/_1c_syntax/utils/downloader/BslLanguageServerDownloaderTest.java (1)
89-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winТесты не проверяют копирование по нескольким чанкам.
Оба payload'а (20 и 3 байта) меньше размера буфера
copyToFile(16 КБ), поэтому цикл чтения выполняется только один раз, иonProgressвызывается лишь дважды (старт/финиш). Основная цель фичи — потоковое копирование по чанкам с промежуточными уведомлениями о прогрессе — этим тестом не покрыта.Стоит добавить тест с payload больше 16 КБ, чтобы проверить, что
onProgressвызывается несколько раз с монотонно возрастающимbytesRead.♻️ Пример дополнительного теста
`@Test` void copyToFileReportsProgressAcrossMultipleChunks(`@TempDir` Path installDir) throws IOException { var payload = new byte[20 * 1024]; // больше буфера 16 КБ new Random(42).nextBytes(payload); var destination = installDir.resolve("asset.bin"); var progress = new ArrayList<long[]>(); BslLanguageServerDownloader.copyToFile( new ByteArrayInputStream(payload), destination, payload.length, (bytesRead, totalBytes) -> progress.add(new long[]{bytesRead, totalBytes})); assertThat(Files.readAllBytes(destination)).isEqualTo(payload); assertThat(progress.size()).isGreaterThan(2); assertThat(progress.get(progress.size() - 1)).containsExactly(payload.length, payload.length); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/github/_1c_syntax/utils/downloader/BslLanguageServerDownloaderTest.java` around lines 89 - 121, The current tests for BslLanguageServerDownloader.copyToFile only use payloads smaller than the 16 KB buffer, so they never exercise multi-chunk streaming or intermediate progress updates. Add a new test alongside copyToFileWritesContentAndReportsProgress and copyToFilePropagatesUnknownTotalSize that uses a payload larger than the buffer and asserts onProgress is invoked more than twice with monotonically increasing bytesRead, while still verifying the file contents and final progress callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/github/_1c_syntax/utils/downloader/BslLanguageServerDownloader.java`:
- Around line 304-317: In copyToFile, the initial progress callback is invoked
before the try-with-resources block, so a RuntimeException from
DownloadProgressListener.onProgress can leave the InputStream unclosed. Move the
first onProgress(0, totalBytes) call inside the try block (after opening source
as input) or otherwise ensure source is included in resource management before
any callback can throw. Keep the cleanup behavior consistent with the contract
of DownloadProgressListener and the copyToFile method.
---
Outside diff comments:
In
`@src/main/java/com/github/_1c_syntax/utils/downloader/BslLanguageServerDownloader.java`:
- Around line 278-302: The download(String, Path, DownloadProgressListener) flow
only applies DOWNLOAD_TIMEOUT to waiting for response headers, so body reads in
copyToFile can still hang indefinitely. Add a separate read timeout for the
response body or switch the download path to an async/cancellable approach, and
update the download/copyToFile handling in BslLanguageServerDownloader so the
body transfer is also bounded by a deadline.
---
Nitpick comments:
In
`@src/test/java/com/github/_1c_syntax/utils/downloader/BslLanguageServerDownloaderTest.java`:
- Around line 89-121: The current tests for
BslLanguageServerDownloader.copyToFile only use payloads smaller than the 16 KB
buffer, so they never exercise multi-chunk streaming or intermediate progress
updates. Add a new test alongside copyToFileWritesContentAndReportsProgress and
copyToFilePropagatesUnknownTotalSize that uses a payload larger than the buffer
and asserts onProgress is invoked more than twice with monotonically increasing
bytesRead, while still verifying the file contents and final progress callback.
🪄 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: CHILL
Plan: Pro
Run ID: c5ef0bf9-22ed-4366-9a01-0f0c6ced0884
📒 Files selected for processing (3)
src/main/java/com/github/_1c_syntax/utils/downloader/BslLanguageServerDownloader.javasrc/main/java/com/github/_1c_syntax/utils/downloader/DownloadProgressListener.javasrc/test/java/com/github/_1c_syntax/utils/downloader/BslLanguageServerDownloaderTest.java
Address review feedback on the streaming download: - move the initial onProgress call inside the try-with-resources so a throwing listener (e.g. cancellation) cannot leak the response body stream - bound the lazily-read body with DOWNLOAD_TIMEOUT via a watchdog that closes the stream on deadline, restoring the timeout coverage lost when switching from ofFile to ofInputStream - add a test covering multi-chunk copying with monotonically increasing progress Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CXTAaR4opi34idMG7ZZ8BV
Addresses SonarCloud java:S5838 on the multi-chunk progress test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CXTAaR4opi34idMG7ZZ8BV
The server archives are tens to hundreds of MB; the JDK-default 16 KiB buffer means thousands of read/write syscalls and progress callbacks per download. Enlarge the copy buffer to cut syscalls and callback frequency proportionally. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CXTAaR4opi34idMG7ZZ8BV
copyToFile no longer closes the stream it is handed; copyBodyWithDeadline owns it via try-with-resources. The timeout watchdog stays a close-to-interrupt only and is cancelled on the normal path, so the stream is closed exactly once unless the deadline fires (where the second, idempotent close is the interrupt itself). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CXTAaR4opi34idMG7ZZ8BV
Move stream ownership to download(), where response.body() is obtained: it opens the stream in a try-with-resources and hosts the timeout watchdog, so nothing closes a stream received as a parameter. copyToFile only reads. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CXTAaR4opi34idMG7ZZ8BV
…ability The HttpClient and GitHub client were built inside methods, so the downloadIfNeeded flow could not be exercised without hitting the network. Extract release lookup behind a ReleaseCatalog interface (GitHubReleaseCatalog is the default impl) and inject the HttpClient via constructor. Public constructors keep their signatures and wire the GitHub-backed defaults. Add end-to-end tests for the full flow using a fake catalog and a loopback HTTP server serving a real zip: download+extract+record version, progress reporting, interval skip, offline fallback to installed, failure with nothing installed, and channel pass-through. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CXTAaR4opi34idMG7ZZ8BV
Replace the loopback HTTP server in the download tests with a mocked HttpClient (Mockito), so the full downloadIfNeeded flow runs with no real network at all. Collapse the telescoping constructors: keep one public GitHub convenience ctor (installDir, token) and one public dependency-injecting ctor (installDir, checkInterval, ReleaseCatalog, HttpClient); ReleaseCatalog is now public. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CXTAaR4opi34idMG7ZZ8BV
…t ctor Remove the premature ReleaseCatalog/GitHubReleaseCatalog abstraction: release lookup moves back into the downloader as an overridable latestRelease(channel) returning a small Release record, mirroring the existing subclass-override test seam. Collapse to a single constructor that injects the HttpClient (installDir, httpClient, token); the old (installDir, token) ctor is gone. Tests mock the HttpClient and override latestRelease to run the whole downloadIfNeeded flow with no network and no local server. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CXTAaR4opi34idMG7ZZ8BV
…f an override Release lookup moves to a concrete GitHubReleaseClient (no interface) injected via the constructor, so tests mock it with Mockito rather than subclassing the downloader to override a method. The downloader now takes (installDir, GitHubReleaseClient, HttpClient); both collaborators are mocked in the download-flow tests, which run with no network. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CXTAaR4opi34idMG7ZZ8BV
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/test/java/com/github/_1c_syntax/utils/downloader/BslLanguageServerDownloaderTest.java (2)
147-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueИспользуйте импорт вместо полного имени класса.
java.util.concurrent.atomic.AtomicIntegerуказан полным именем вместо импорта, хотя остальной файл использует обычные импорты (см. блок строк 30-44).♻️ Предлагаемое исправление
+import java.util.concurrent.atomic.AtomicInteger; ... - var closed = new java.util.concurrent.atomic.AtomicInteger(); + var closed = new AtomicInteger();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/github/_1c_syntax/utils/downloader/BslLanguageServerDownloaderTest.java` around lines 147 - 161, The test in copyToFileLeavesSourceOpenForCaller uses the fully qualified java.util.concurrent.atomic.AtomicInteger inline instead of the file’s standard import style. Add an import for AtomicInteger alongside the other imports in BslLanguageServerDownloaderTest and update the local variable declaration to use the simple class name, keeping the test logic unchanged.
260-280: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueМок
HttpClient.sendвсегда возвращает один и тот же экземплярByteArrayInputStream.
response.body()заранее настроен возвращать один и тот жеByteArrayInputStream(body)для всех вызововsend(...). Если код загрузчика когда-либо вызоветsend()более одного раза для одного и того же клиента (повторные попытки, редиректы, ретраи после watchdog-таймаута, упомянутого в описании PR), второй вызов получит уже исчерпанный поток, что приведёт к скрытому, трудно диагностируемому провалу теста вместо явной ошибки. В текущих тестахsend()вызывается один раз на клиент, поэтому проблема не проявляется, но стоит держать это в уме при добавлении тестов ретраев/таймаутов.Использование
doReturn(...).when(...)вместоwhen(...).thenReturn(...)— оправданный приём для обхода конфликта дженериков в сигнатуреsend(HttpRequest, BodyHandler<T>), это нормально.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/github/_1c_syntax/utils/downloader/BslLanguageServerDownloaderTest.java` around lines 260 - 280, The HttpClient mock in httpClientReturning() reuses the same ByteArrayInputStream for every send() call, which will break any retry/redirect path that reads the body more than once. Update the response.body() stubbing so each send() gets a fresh ByteArrayInputStream over the same byte[] body, while keeping the existing doReturn(...).when(client).send(...) setup unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@src/test/java/com/github/_1c_syntax/utils/downloader/BslLanguageServerDownloaderTest.java`:
- Around line 147-161: The test in copyToFileLeavesSourceOpenForCaller uses the
fully qualified java.util.concurrent.atomic.AtomicInteger inline instead of the
file’s standard import style. Add an import for AtomicInteger alongside the
other imports in BslLanguageServerDownloaderTest and update the local variable
declaration to use the simple class name, keeping the test logic unchanged.
- Around line 260-280: The HttpClient mock in httpClientReturning() reuses the
same ByteArrayInputStream for every send() call, which will break any
retry/redirect path that reads the body more than once. Update the
response.body() stubbing so each send() gets a fresh ByteArrayInputStream over
the same byte[] body, while keeping the existing
doReturn(...).when(client).send(...) setup unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05054093-3fae-4d06-93a9-9c27ed003633
📒 Files selected for processing (4)
build.gradle.ktssrc/main/java/com/github/_1c_syntax/utils/downloader/BslLanguageServerDownloader.javasrc/main/java/com/github/_1c_syntax/utils/downloader/GitHubReleaseClient.javasrc/test/java/com/github/_1c_syntax/utils/downloader/BslLanguageServerDownloaderTest.java
Address review nitpicks: import AtomicInteger instead of using its FQN, and have the mocked HttpClient hand out a fresh ByteArrayInputStream on each send() so the mock stays correct if the body is ever read more than once. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CXTAaR4opi34idMG7ZZ8BV
…-31-review-g8hz79
…lookup GitHubReleaseClient no longer takes an HttpClient: the no-arg HttpClientGitHubConnector lets github-api build and manage its own client for the API calls. The injected HttpClient stays only in the downloader, solely for the asset download — which github-api does not provide (GHAsset exposes just the browser download URL) and which needs per-byte progress anyway. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CXTAaR4opi34idMG7ZZ8BV
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/utils/downloader/GitHubReleaseClient.java (1)
76-79: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winНе материализуйте
listReleases()перед поиском
PagedIterable.toList()обходит все страницы заранее, хотя здесь нужен только первый не-draft релиз. Это лишние запросы и задержка. Итерируйтесь напрямую и остановитесь на первом совпадении:♻️ Предлагаемый рефакторинг
GHRelease release = null; for (GHRelease it : repository.listReleases()) { if (!it.isDraft()) { release = it; break; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/github/_1c_syntax/utils/downloader/GitHubReleaseClient.java` around lines 76 - 79, The GitHub release lookup in GitHubReleaseClient currently materializes repository.listReleases() via toList() before filtering, which eagerly loads every page; update the release selection logic to iterate directly over repository.listReleases(), stop at the first non-draft GHRelease, and keep the existing release assignment behavior without collecting the full list first.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@src/main/java/com/github/_1c_syntax/utils/downloader/GitHubReleaseClient.java`:
- Around line 76-79: The GitHub release lookup in GitHubReleaseClient currently
materializes repository.listReleases() via toList() before filtering, which
eagerly loads every page; update the release selection logic to iterate directly
over repository.listReleases(), stop at the first non-draft GHRelease, and keep
the existing release assignment behavior without collecting the full list first.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6edf9c5e-0a36-46f9-99c3-e3bb22c15d04
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/utils/downloader/BslLanguageServerDownloader.javasrc/main/java/com/github/_1c_syntax/utils/downloader/GitHubReleaseClient.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/github/_1c_syntax/utils/downloader/BslLanguageServerDownloader.java
Iterate repository.listReleases() lazily and break on the first non-draft prerelease instead of materializing every page via toList(), avoiding needless GitHub API requests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CXTAaR4opi34idMG7ZZ8BV
|
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.
Что и зачем
Часть решения 1c-syntax/intellij-language-1c-bsl#31: при скачивании релиза BSL Language Server клиент (плагин IntelliJ) мог показать только бесконечный «крутящийся» индикатор, потому что загрузчик качал ассет через
HttpResponse.BodyHandlers.ofFile(...)— без какой-либо обратной связи о ходе загрузки.Изменения
DownloadProgressListener— функциональный интерфейс с методомonProgress(bytesRead, totalBytes)(totalBytes == -1, если сервер не прислалContent-Length) и константойNONE.download(...)теперь читает тело ответа потоком (BodyHandlers.ofInputStream) и копирует его в файл по блокам, вызывая слушателя после каждого блока; размер берётся из заголовкаContent-Length.downloadIfNeeded(channel, progressListener). ПрежнийdownloadIfNeeded(channel)сохранён и делегирует новой версии с no-op слушателем — обратная совместимость полная.Проверки
compileJava,test(включая два новых теста наcopyToFile) и проверка лицензионных заголовков (license) проходят локально.🤖 Generated with Claude Code
https://claude.ai/code/session_01CXTAaR4opi34idMG7ZZ8BV
Generated by Claude Code