fix: standalone bugfix batch (6 issues)#278
Conversation
- #223: sparkline crash on all-NA latencies (is.finite guard) - #220: NA guard on provider$api_key in query builder - #224: extract hard-coded cost_usd to OA_CONTENT_DOWNLOAD_COST_USD constant - #222: nchar multi-element vector crash in extract_message_content (all() wrap) - Also closed #128 and #221 as already resolved
There was a problem hiding this comment.
Pull request overview
This PR bundles several small bugfixes across the OpenRouter integration, cost/latency UI, and OpenAlex usage logging, with accompanying test coverage to prevent regressions.
Changes:
- Hardened OpenRouter message parsing and API-key validation edge cases.
- Fixed latency sparkline handling for all-NA / non-finite max latency scenarios, and added tests.
- Replaced a hardcoded OpenAlex content-download cost with a named config constant.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-extract-message-content.R | Adds unit tests covering extract_message_content() shapes and edge cases. |
| tests/testthat/test-cost-tracking.R | Adds sparkline edge-case tests and imports shiny for tag class assertions. |
| R/mod_query_builder.R | Adds NA/empty guarding for provider$api_key before calling OpenRouter. |
| R/mod_cost_tracker.R | Prevents sparkline crash when max() is non-finite or non-positive. |
| R/import_paper.R | Uses OA_CONTENT_DOWNLOAD_COST_USD constant for OpenAlex usage logging. |
| R/config.R | Introduces OA_CONTENT_DOWNLOAD_COST_USD constant. |
| R/api_openrouter.R | Fixes if() crash on multi-element content by wrapping emptiness check in all(). |
Comments suppressed due to low confidence (2)
R/mod_cost_tracker.R:329
bar_heightscan beNAwhentrend$avg_latency_mscontainsNA, which then makesmax(as.integer(bar_heights[i]), 2)returnNA(sincemax()propagatesNAby default). This produces invalid CSS likeheight: NApxfor those bars. Consider treating non-finite/NA heights as 0 before applying the minimum height (e.g., useif (!is.finite(h)) h <- 0ormax(..., na.rm = TRUE)).
bar_heights <- (trend$avg_latency_ms / max_ms) * 40
tags$div(
class = "d-flex align-items-end gap-1",
style = "height: 48px;",
lapply(seq_len(nrow(trend)), function(i) {
tags$div(
style = sprintf(
"width: 6px; height: %dpx; background: var(--bs-secondary); border-radius: 2px 2px 0 0; opacity: 0.7;",
max(as.integer(bar_heights[i]), 2)
),
R/mod_query_builder.R:86
- The PR description references issue #220 for the API key
NAguard, but the linked Issue #220 text is about vision describe error-path tests and an OpenRouter response bounds check inR/api_openrouter.R:60-67. If this PR is intended to close the linked Issue #220, those items don’t appear to be addressed here; consider updating the PR description/issue references to avoid confusion.
if ((is.null(provider$api_key) || is.na(provider$api_key) || !nzchar(provider$api_key)) && !is_local_provider(provider)) {
showNotification(
"OpenRouter API key not configured. Please go to Settings.",
type = "warning",
duration = 5
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tryCatch( | ||
| log_oa_usage(con, "content_download", | ||
| paste0("content/works/", abs$paper_id, ".pdf"), | ||
| dl$usage, cost_usd = 0.01), | ||
| dl$usage, cost_usd = OA_CONTENT_DOWNLOAD_COST_USD), | ||
| error = function(e) message("[import] Failed to log OA usage: ", e$message) |
There was a problem hiding this comment.
OA_CONTENT_DOWNLOAD_COST_USD is defined in config.R, but import_single_paper() is sourced/used in at least one worker context that does not source config.R (e.g., batch_import_task in mod_search_notebook.R sources import_paper.R without config.R). In that path, evaluating cost_usd = OA_CONTENT_DOWNLOAD_COST_USD will fail and silently skip OA usage logging (caught by tryCatch). Consider either ensuring all entrypoints source config.R before import_paper.R, or adding a local fallback (e.g., defaulting to 0.01 when the constant is not defined) so usage logging works consistently.
R/api_openrouter.R
Outdated
| # Reasoning models (e.g. gpt-5-nano) put output in $reasoning, $content is NULL | ||
|
|
||
| if (is.null(content) || (is.character(content) && nchar(content) == 0)) { | ||
| if (is.null(content) || (is.character(content) && all(nchar(content) == 0))) { |
There was a problem hiding this comment.
The all(nchar(content) == 0) condition can evaluate to NA if content contains NA values (since nchar(NA_character_) is NA), which would make the if (...) throw “missing value where TRUE/FALSE needed”. Consider making the emptiness check NA-safe (e.g., wrap with isTRUE(...) or treat NA as empty via all(is.na(content) | nchar(content) == 0)).
| if (is.null(content) || (is.character(content) && all(nchar(content) == 0))) { | |
| if (is.null(content) || (is.character(content) && all(is.na(content) | nchar(content) == 0))) { |
Summary
max(-Inf)guard withis.finite()checkis.na()check beforenchar()callcost_usd = 0.01to named constantOA_CONTENT_DOWNLOAD_COST_USDin config.Rnchar(content) == 0inall()to preventif()error on vector inputTest plan
build_latency_sparklineedge cases (all-NA, all-zero, mixed, normal, single-row)extract_message_content(normal, NULL+reasoning, NULL-no-reasoning, empty string, multi-element vector, list-of-parts, mixed parts)