Merged
Conversation
ghost
self-assigned this
Contributor
📝 WalkthroughWalkthroughFetchSingle now accepts variadic request options. POR app adds per-entry proxy flags, loads proxies from the DB, rotates them via a mutex-protected index, and passes proxy-aware request options into FetchSingle. Types updated to carry proxy fields on app, entry, and URL definitions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant POR as POR App
participant DB as DB
participant Rot as Proxy Rotator
participant Fetcher as Fetcher.FetchSingle
participant HTTP as request.Request
participant Red as reducer.Reduce
POR->>DB: Query proxies at startup
POR->>POR: Construct entries (useProxy flags)
POR->>POR: buildRequestOpts(entry)
alt entry.useProxy == true and proxies available
POR->>Rot: getNextProxy()
Rot-->>POR: proxy URL
POR->>POR: append WithProxy(proxy) to opts
else
POR->>POR: base opts only
end
POR->>Fetcher: FetchSingle(ctx, definition, opts...)
Fetcher->>HTTP: request.Request(endpoint, headers, timeout, ...opts)
HTTP-->>Fetcher: raw response
Fetcher->>Red: reducer.Reduce(raw)
Red-->>Fetcher: value
Fetcher-->>POR: value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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). (1)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
node/pkg/por/types.go (1)
42-46: Mutex embed is fine; consider minimizing lock contention and copy hazards.
- Ensure
appmethods remain on pointer receivers (they do) to avoid copying a struct with an embeddedsync.Mutex.- Optional: switch
proxyIdxto an atomic counter or keep it bounded (see app.go note) to reduce contention from many jobs.node/pkg/fetcher/utils.go (1)
17-24: Nit: preallocate requestOptions capacity.
Avoid a tiny reallocation when appendingreqOpts.- requestOptions := []request.RequestOption{ + requestOptions := make([]request.RequestOption, 0, 3+len(reqOpts)) + requestOptions = append(requestOptions, request.WithEndpoint(*definition.Url), request.WithHeaders(definition.Headers), request.WithTimeout(10 * time.Second), - } + )node/pkg/por/app.go (3)
36-82: Use keyed struct literals to decouple from field order.
Safer if urlEntry fields change; improves readability.-var urls = map[string]urlEntry{ - "peg-por": { - "/{CHAIN}/peg-{CHAIN}.por.json", - "/{CHAIN}/peg.por.json", - false, - }, - "gp": { - "/{CHAIN}/gp-{CHAIN}.json", - "/{CHAIN}/gp.json", - false, - }, - "aapl": { - "/{CHAIN}/aapl-{CHAIN}.json", - "/{CHAIN}/aapl.json", - true, - }, - "amzn": { - "/{CHAIN}/amzn-{CHAIN}.json", - "/{CHAIN}/amzn.json", - true, - }, - "googl": { - "/{CHAIN}/googl-{CHAIN}.json", - "/{CHAIN}/googl.json", - true, - }, - "meta": { - "/{CHAIN}/meta-{CHAIN}.json", - "/{CHAIN}/meta.json", - true, - }, - "msft": { - "/{CHAIN}/msft-{CHAIN}.json", - "/{CHAIN}/msft.json", - true, - }, - "nvda": { - "/{CHAIN}/nvda-{CHAIN}.json", - "/{CHAIN}/nvda.json", - true, - }, - "tsla": { - "/{CHAIN}/tsla-{CHAIN}.json", - "/{CHAIN}/tsla.json", - true, - }, -} +var urls = map[string]urlEntry{ + "peg-por": {adapterEndpoint: "/{CHAIN}/peg-{CHAIN}.por.json", aggregatorEndpoint: "/{CHAIN}/peg.por.json", useProxy: false}, + "gp": {adapterEndpoint: "/{CHAIN}/gp-{CHAIN}.json", aggregatorEndpoint: "/{CHAIN}/gp.json", useProxy: false}, + "aapl": {adapterEndpoint: "/{CHAIN}/aapl-{CHAIN}.json", aggregatorEndpoint: "/{CHAIN}/aapl.json", useProxy: true}, + "amzn": {adapterEndpoint: "/{CHAIN}/amzn-{CHAIN}.json", aggregatorEndpoint: "/{CHAIN}/amzn.json", useProxy: true}, + "googl": {adapterEndpoint: "/{CHAIN}/googl-{CHAIN}.json", aggregatorEndpoint: "/{CHAIN}/googl.json", useProxy: true}, + "meta": {adapterEndpoint: "/{CHAIN}/meta-{CHAIN}.json", aggregatorEndpoint: "/{CHAIN}/meta.json", useProxy: true}, + "msft": {adapterEndpoint: "/{CHAIN}/msft-{CHAIN}.json", aggregatorEndpoint: "/{CHAIN}/msft.json", useProxy: true}, + "nvda": {adapterEndpoint: "/{CHAIN}/nvda-{CHAIN}.json", aggregatorEndpoint: "/{CHAIN}/nvda.json", useProxy: true}, + "tsla": {adapterEndpoint: "/{CHAIN}/tsla-{CHAIN}.json", aggregatorEndpoint: "/{CHAIN}/tsla.json", useProxy: true}, +}
370-381: KeepproxyIdxbounded to avoid theoretical overflow issues.
Prevents negative modulo on overflow in 32-bit builds; preserves round-robin order.-func (a *app) getNextProxy() *string { +func (a *app) getNextProxy() *string { a.Lock() defer a.Unlock() if len(a.proxies) == 0 { return nil } - proxy := a.proxies[a.proxyIdx%len(a.proxies)].GetProxyUrl() - a.proxyIdx++ - return &proxy + idx := a.proxyIdx % len(a.proxies) + proxy := a.proxies[idx].GetProxyUrl() + a.proxyIdx = (a.proxyIdx + 1) % len(a.proxies) + return &proxy }Optional follow-up: return
(string, bool)instead of*stringto avoid pointer-to-local and make intent explicit.
383-391: LGTM: proxy option applied conditionally.
Consider logging once ifuseProxyis true but no proxies are loaded to aid observability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
node/pkg/fetcher/utils.go(1 hunks)node/pkg/por/app.go(6 hunks)node/pkg/por/types.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
node/pkg/fetcher/utils.go (2)
node/pkg/fetcher/types.go (1)
Definition(96-110)node/pkg/utils/request/request.go (5)
RequestOption(26-26)WithEndpoint(34-38)WithHeaders(46-50)WithTimeout(28-32)Request(64-102)
node/pkg/por/app.go (3)
node/pkg/db/pgsql.go (1)
QueryRows(92-100)node/pkg/fetcher/utils.go (1)
FetchSingle(16-31)node/pkg/utils/request/request.go (2)
RequestOption(26-26)WithProxy(52-56)
⏰ 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). (1)
- GitHub Check: core-build
🔇 Additional comments (8)
node/pkg/por/types.go (3)
6-6: LGTM on new imports.
Necessary for mutex and Proxy type usage.Also applies to: 10-10
52-52: Per-entry proxy flag looks good.
Keeps proxy behavior explicit at the entry level.
83-84: useProxy on urlEntry aligns with entry; good wiring point.node/pkg/fetcher/utils.go (2)
16-26: Good API: variadic request options with caller overrides.
Order ensures caller-provided opts can override defaults.
16-16: All FetchSingle call sites use the new signature — no stale usages found.
rg output shows calls in node/pkg/por/app.go (passes a.buildRequestOpts(e)...), node/pkg/por/por_test.go, and node/pkg/fetcher/utils_test.go; all calls use either variadic opts or none.node/pkg/por/app.go (3)
15-16: LGTM on imports.
Adds logging, types, and DB for proxy support.Also applies to: 19-20
127-128: LGTM: wiringuseProxyfrom config into entry.
Keeps runtime decision local to the entry.
232-234: LGTM: per-request opts plumbed into fetch.
Enables proxy-aware calls without touching fetcher internals.
Comment on lines
18
to
19
| request.WithEndpoint(*definition.Url), | ||
| request.WithHeaders(definition.Headers), |
Contributor
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Guard against nil URL to prevent panic.
*definition.Url will panic if Url is nil (e.g., malformed config). Confirm upstream guarantees or add a defensive check returning an error.
Run to spot any definitions missing url:
🏁 Script executed:
#!/bin/bash
rg -nP -C2 '"url"\s*:\s*(null|""|\{\s*\})' -g '!**/node_modules/**' -SLength of output: 148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf "PWD: %s\n\n" "$(pwd)"
printf "Top-level files:\n"
ls -la | sed -n '1,120p' || true
printf "\nCheck exact path: node/pkg/fetcher/utils.go\n"
if [ -f node/pkg/fetcher/utils.go ]; then
printf "FOUND: node/pkg/fetcher/utils.go\n\n"
nl -ba node/pkg/fetcher/utils.go | sed -n '1,200p'
else
printf "Not found at node/pkg/fetcher/utils.go\n\n"
printf "Listing all utils.go files (maxdepth 6):\n"
find . -maxdepth 6 -type f -name 'utils.go' -print || true
printf "\nSearching for relevant patterns across the repo (grep fallback; excluding node_modules and .git):\n"
grep -RIn --exclude-dir=node_modules --exclude-dir=.git -E 'definition\.Url|definition\.url|\*\s*definition\.Url|WithEndpoint\(|WithHeaders\(' . || true
printf "\nSearching for Go struct fields named 'Url' (Url string or Url *string) in .go files:\n"
grep -RIn --exclude-dir=node_modules --exclude-dir=.git --include='*.go' -E '\bUrl\b\s+(\*?string)' . || true
printf "\nSearching for types named *Definition (type .*Definition) to locate 'definition' struct declarations:\n"
grep -RIn --exclude-dir=node_modules --exclude-dir=.git --include='*.go' -E 'type\s+[A-Za-z0-9_]*Definition\b' . || true
fiLength of output: 1528
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
if [ -f node/pkg/fetcher/utils.go ]; then
echo "----- node/pkg/fetcher/utils.go (first 200 lines) -----"
awk '{printf("%4d: %s\n", NR, $0)}' node/pkg/fetcher/utils.go | sed -n '1,200p'
else
echo "node/pkg/fetcher/utils.go not found"
fi
echo
echo "=== Repo searches (rg fallback to grep) ==="
rg_cmd() {
if command -v rg >/dev/null 2>&1; then
rg -n --hidden --no-ignore -S "$@" --glob '!**/node_modules/**' || true
else
grep -RIn --exclude-dir=node_modules --exclude-dir=.git -E "$@" . || true
fi
}
echo
echo "-- occurrences of 'definition.Url' --"
rg_cmd 'definition\.Url'
echo
echo "-- occurrences of '*definition.Url' (explicit deref) --"
rg_cmd '\*definition\.Url'
echo
echo "-- usages of WithEndpoint( --"
rg_cmd 'WithEndpoint\('
echo
echo "-- Go declarations that include 'Url' (to find field type) --"
rg_cmd '\bUrl\b'
echo
echo "-- type names ending with Definition --"
rg_cmd 'type\s+[A-Za-z0-9_]*Definition\b'Length of output: 3793
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "1) List Go files under node/pkg (top 200):"
find node/pkg -type f -name '*.go' -not -path '*/node_modules/*' -not -path '*/.git/*' -print | sed -n '1,200p' || true
echo
echo "2) Find 'type ...Definition struct' declarations:"
find . -type f -name '*.go' -not -path '*/node_modules/*' -not -path '*/.git/*' -print0 | xargs -0 grep -nE --line-number 'type\s+[A-Za-z0-9_]*Definition\s+struct' || true
echo
echo "3) Find any 'Definition' symbol occurrences (to locate the type):"
find . -type f -name '*.go' -not -path '*/node_modules/*' -not -path '*/.git/*' -print0 | xargs -0 grep -nE --line-number '\bDefinition\b' || true
echo
echo "4) Search for 'Url' field declarations and occurrences in Go files:"
find . -type f -name '*.go' -not -path '*/node_modules/*' -not -path '*/.git/*' -print0 | xargs -0 grep -nE --line-number '\bUrl\b' || true
echo
echo "5) Show files in node/pkg/utils/request (if present) and search for WithEndpoint signature:"
if [ -d node/pkg/utils/request ]; then
echo "Files in node/pkg/utils/request:"
find node/pkg/utils/request -maxdepth 2 -type f -name '*.go' -print || true
echo
echo "Searching for 'WithEndpoint' definition/usage in repo:"
find . -type f -name '*.go' -not -path '*/node_modules/*' -not -path '*/.git/*' -print0 | xargs -0 grep -nE --line-number 'WithEndpoint' || true
echo
echo "If found, print the request package files (first 200 lines each):"
for f in $(find node/pkg/utils/request -type f -name '*.go' -print); do
echo "---- $f ----"
sed -n '1,200p' "$f" || true
echo
done
else
echo "node/pkg/utils/request not found"
fiLength of output: 31334
Guard against nil definition.Url (avoid panic).
Definition.Url is a *string (node/pkg/fetcher/types.go:97) but is dereferenced without a nil-check. Fix by validating/enforcing url on input or adding defensive checks that return an error instead of panicking. Relevant call sites to update: node/pkg/fetcher/utils.go:18, node/pkg/fetcher/fetcher.go:170,174, node/script/test_single_fetch/main.go:28.
🤖 Prompt for AI Agents
In node/pkg/fetcher/utils.go around lines 18-19 the code dereferences
definition.Url (a *string) without checking for nil which can cause a panic; add
a defensive nil-check and return an error if Url is nil (or validate earlier and
propagate a clear error) rather than dereferencing. Update the other call sites
mentioned (node/pkg/fetcher/fetcher.go lines ~170/174 and
node/script/test_single_fetch/main.go:28) to handle the new error/validation
(either validate definition.Url before calling or handle the returned error) so
no code ever blindly dereferences the pointer.
Comment on lines
+148
to
+151
| proxies, err := db.QueryRows[types.Proxy](ctx, "SELECT * FROM proxies", nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t hard-fail on proxy DB errors; degrade gracefully.
If the proxies table is missing or DB is transiently down, POR can still run without proxies. Recommend warning + proceed with no proxies.
- proxies, err := db.QueryRows[types.Proxy](ctx, "SELECT * FROM proxies", nil)
- if err != nil {
- return nil, err
- }
+ proxies, err := db.QueryRows[types.Proxy](ctx, "SELECT * FROM proxies", nil)
+ if err != nil {
+ log.Warn().Err(err).Msg("failed to load proxies; continuing without proxies")
+ proxies = nil
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| proxies, err := db.QueryRows[types.Proxy](ctx, "SELECT * FROM proxies", nil) | |
| if err != nil { | |
| return nil, err | |
| } | |
| proxies, err := db.QueryRows[types.Proxy](ctx, "SELECT * FROM proxies", nil) | |
| if err != nil { | |
| log.Warn().Err(err).Msg("failed to load proxies; continuing without proxies") | |
| proxies = nil | |
| } |
🤖 Prompt for AI Agents
In node/pkg/por/app.go around lines 148 to 151, the current code returns on any
DB error when querying proxies; change this to degrade gracefully by catching
the error, logging a warning that includes the error details and context (e.g.,
"unable to load proxies, proceeding without proxies"), and then set proxies to
an empty slice (or nil) so the rest of POR can continue; only return an error
for truly fatal conditions, not for missing/temporary proxy DB failures.
ghost
force-pushed
the
feat/minor-updates-for-m7
branch
from
47a0333 to
a556a6b
Compare
This pull request was closed.
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.
Description
Please include a summary of the changes and the related issue.
Please also include relevant motivation and context.
List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
Checklist before requesting a review
Deployment