Conversation
… for line-number resolution - Replace Promise.all with concurrentMap (concurrency capped at 20) to avoid saturating the connection pool on slow/mobile networks - Add AbortSignal.timeout(5 s) per raw file fetch with silent fallback to fragment-relative line numbers on timeout or network error - Add buildLineResolutionProgress() progress bar shown on stderr during the resolution phase; line is erased before TUI starts - Add concurrentMap() helper in api-utils.ts (semaphore, no extra deps) - Add unit tests for buildLineResolutionProgress and concurrentMap Closes #99
|
Coverage after merging fix/line-resolution-hang into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Contributor
There was a problem hiding this comment.
Pull request overview
Addresses a post-pagination “silent hang” in the CLI by bounding the number of concurrent raw-content fetches used to resolve absolute line numbers, and by adding a stderr progress indicator for that phase.
Changes:
- Added
concurrentMap()utility to run async work with a concurrency cap while preserving input order. - Refactored
fetchAllResultsline-number resolution to useconcurrentMap(concurrency 20) plus a per-request timeout and progress updates. - Added unit tests for
concurrentMapand the newbuildLineResolutionProgressprogress-line formatter.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/api.ts |
Uses bounded concurrency for raw fetches and prints/clears a resolution progress line; adds buildLineResolutionProgress for testing. |
src/api.test.ts |
Adds tests for the new line-resolution progress string builder. |
src/api-utils.ts |
Introduces concurrentMap helper (semaphore/worker pattern) for capped parallelism. |
src/api-utils.test.ts |
Adds tests covering ordering, indices, concurrency cap, and error behavior for concurrentMap. |
- Fix concurrentMap docstring: clarify that all items are processed even after an error; first error is re-thrown after all work has completed - Add concurrency input validation: throw RangeError when concurrency is <= 0 or NaN, with a descriptive error message - Add 3 unit tests covering the new validation (0, negative, NaN) - Replace magic-number space padding with ANSI erase-entire-line sequence (\x1b[2K\r) to clear the resolution progress line reliably See: #100
|
Coverage after merging fix/line-resolution-hang into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
- Replace Number.isFinite with Number.isInteger in concurrentMap validation: floats like 1.5 now correctly throw RangeError (isInteger implies finite, covers NaN/Infinity/floats/negatives in a single check) - Add test: concurrency=1.5 throws RangeError - Pass maxRetries=0 to fetchWithRetry for raw file fetches so AbortSignal.timeout acts as a strict hard cap; retry delays in fetchWithRetry are not abort-aware and would otherwise let a timed-out request silently retry for up to 30 s See: #100
|
Coverage after merging fix/line-resolution-hang into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
- Extract buildProgressBar() private helper in api.ts to deduplicate the ANSI colour + block-character logic shared between buildFetchProgress and buildLineResolutionProgress — styling changes now only need one edit - Fix concurrency-cap test: use originalSetTimeout (real async delay) instead of the immediate-call setTimeout shim installed by beforeEach, so worker tasks genuinely interleave and the maxActive assertion is meaningful See: #100
|
Coverage after merging fix/line-resolution-hang into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Coverage after merging fix/line-resolution-hang into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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
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.
Closes #99
Root cause
After the pagination bar completes,
fetchAllResultsfetches the raw content of every unique matched file fromraw.githubusercontent.comto compute absolute line numbers. Two issues caused a silent hang of up to 2 minutes on slow/mobile networks:Promise.allwith no concurrency cap — 200+ HTTP requests fired simultaneously, saturating the connection pool.Changes
src/api-utils.tsconcurrentMap<T, R>()— semaphore-based helper, at most N tasks running in parallel (no new dependencies). Results returned in input order.src/api.tsconcurrentMap.buildLineResolutionProgress(done, total)— pure function, same style asbuildFetchProgress, exported for unit testing.fetchAllResults:Promise.allwithconcurrentMap(..., { concurrency: 20 }).AbortSignal.timeout(5_000)per request; silent fallback to fragment-relative line numbers on timeout or error.Resolving line numbers… ▓▓▓░░░ 12/47progress on stderr, updated after each file.Tests
src/api-utils.test.ts— 6 new tests forconcurrentMap(order, index, concurrency cap, error propagation, empty input).src/api.test.ts— 6 new tests forbuildLineResolutionProgress.Verification
Manual: run a query returning many results on tethering — the
Resolving line numbers…bar should appear immediately after pagination, TUI opens within seconds.