Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR adds native Juplend lending account support via the ChangesJuplend lending account native support
Trellis infrastructure and tooling updates
🎯 4 (Complex) | ⏱️ ~50 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/adapters/juplend/private-adapter.ts (1)
262-269:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd request timeouts to Juplend HTTP fetches.
Line 263 uses
fetchwithout a timeout; a hanging upstream call can block bootstrap/poll completion and keep account state stale.⏱️ Suggested hard timeout wrapper
+const DEFAULT_HTTP_TIMEOUT_MS = 10_000; + async function readJson<T>(url: string, init?: RequestInit): Promise<T> { - const response = await fetch(url, init); + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), DEFAULT_HTTP_TIMEOUT_MS); + const response = await fetch(url, { + ...init, + signal: init?.signal ?? controller.signal, + }).finally(() => clearTimeout(timer)); if (!response.ok) { throw new Error(`Juplend HTTP ${response.status}: ${response.statusText}`); }🤖 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/adapters/juplend/private-adapter.ts` around lines 262 - 269, The fetch in readJson<T> can hang; wrap it with an AbortController and a hard timeout: create a controller, start a timeout (e.g. configurable default like 10s) that calls controller.abort(), merge the controller.signal into the fetch by adding it to the passed init (preserving other init options), and if init.signal exists, forward that signal by listening to it and calling controller.abort() so either signal cancels the request; clear the timeout on successful response, and when abort occurs throw a clear timeout-specific error (e.g., "Juplend fetch timeout") instead of waiting forever.tests/integration/account.test.ts (1)
673-679:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix failing Jup API call assertions by enabling enrichment in these tests.
Line 746 and Line 1162 assert two
api.jup.agcalls, but these test clients are created without a Juplend API key, so the requests stay at 0 (matching CI failures). ConfigurejupApiKeyin these tests (or relax the assertions).Suggested patch
@@ const client = createClient({ account: { juplend: { pollIntervalMs: 60_000, + jupApiKey: "test-key", }, }, }); @@ - const client = createClient(); + const client = createClient({ + account: { + juplend: { + jupApiKey: "test-key", + }, + }, + });Also applies to: 744-746, 1133-1133, 1160-1162
🤖 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 `@tests/integration/account.test.ts` around lines 673 - 679, The tests create test clients via createClient({ account: { juplend: { pollIntervalMs: 60_000 }}}) but do not set a jupApiKey, so Juplend enrichment is disabled and expected api.jup.ag calls remain zero; update the test client config used in these failing specs (tests referencing createClient in account.test.ts / the tests asserting api.jup.ag calls around the failing assertions) to include a jupApiKey (a valid or mocked key) so enrichment runs and the two api.jup.ag requests occur, or alternatively relax the assertions to not require two calls—prefer adding jupApiKey to the createClient invocation to fix the failures.
🧹 Nitpick comments (1)
.trellis/scripts/common/session_context.py (1)
89-109: 💤 Low valueRename ambiguous loop variable
ltoline.Static analysis flagged the single-letter variable name. Using
lineimproves readability.✏️ Suggested fix
- changes = len([l for l in status_out.splitlines() if l.strip()]) + changes = len([line for line in status_out.splitlines() if line.strip()])🤖 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 @.trellis/scripts/common/session_context.py around lines 89 - 109, In _collect_git_repo_info rename the ambiguous loop variable `l` used in the list comprehension that counts changes to a clearer name like `line`; update the expression that builds the list from status_out.splitlines() if l.strip() to use `line.strip()` instead so the semantics remain the same and improve readability.
🤖 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 @.agents/skills/trellis-spec-bootstarp/SKILL.md:
- Line 2: Update the skill name/title to correct the typo "bootstarp" →
"bootstrap": change the frontmatter name value `trellis-spec-bootstarp` to
`trellis-spec-bootstrap` and update the H1/title text in SKILL.md to use
"bootstrap" as well; if the identifier must remain stable, at minimum correct
the visible H1/title while leaving the frontmatter name unchanged and add a
brief note or comment explaining the decision.
In @.claude/skills/trellis-spec-bootstarp/SKILL.md:
- Line 2: The skill name contains a typo "trellis-spec-bootstarp"; update the
name value to "trellis-spec-bootstrap" and also correct any other occurrences of
"bootstarp" (notably the other occurrence referenced at line 6) so the `.claude`
and `.agents` skill names remain consistent and searchable.
In @.trellis/spec/backend/venue-lending.md:
- Around line 30-34: The Juplend options example incorrectly marks all fields
optional; update the signature block for the `options` example to reflect the
real union contract by replacing the current `{ walletAddress?: string;
vaultId?: string; positionId?: string }` with a discriminated union like `{
walletAddress: string } | { vaultId: string; positionId: string }` (or
equivalent wording stating "either walletAddress OR both vaultId and positionId
are required") and make the same change to the other Juplend signature
occurrence mentioned so the docs match `src/types/shared.ts`.
In `@docs/api.md`:
- Line 1473: The docs table entry for ACCOUNT_BOOTSTRAP_FAILED is incomplete:
update the row for ACCOUNT_BOOTSTRAP_FAILED to mention both bootstrap failure
due to missing options.walletAddress and the alternative valid path using
vaultId + positionId (i.e., direct mode). Modify the description so it clearly
states that bootstrap can fail when options.walletAddress is missing OR when
using direct mode the caller must supply valid vaultId and positionId, and
ensure the text references the exact keys options.walletAddress, vaultId, and
positionId so readers aren’t misled.
In `@docs/architecture.md`:
- Around line 316-317: Update the options shape so it enforces the Juplend
contract constraint that either walletAddress is provided OR both vaultId and
positionId are provided (not all three optional); change the documented/typed
signature for options (the object containing walletAddress, vaultId, positionId)
into a discriminated union or add explicit runtime validation in the code paths
that consume options to require { walletAddress: string } OR { vaultId: string,
positionId: string } and reject/throw/log when the combination is invalid.
In `@src/adapters/juplend/private-adapter.ts`:
- Around line 448-450: The current freshness gate only returns cached.vaults
when cacheFresh && (cached.enriched || !apiKey), which forces reloads if
enrichment fails while an API key is present; change the early-return to return
cached.vaults whenever cacheFresh is true (i.e., remove the enriched||!apiKey
requirement) so fallback data is honored, and in the enrichment catch block (the
error handler around the enrichment logic that currently retries/reloads) mark
the cached entry as a fallback (e.g., set cached.enriched = false or
cached.fallback = true) and preserve its TTL so subsequent polls use the cached
vaults until the TTL expires.
---
Outside diff comments:
In `@src/adapters/juplend/private-adapter.ts`:
- Around line 262-269: The fetch in readJson<T> can hang; wrap it with an
AbortController and a hard timeout: create a controller, start a timeout (e.g.
configurable default like 10s) that calls controller.abort(), merge the
controller.signal into the fetch by adding it to the passed init (preserving
other init options), and if init.signal exists, forward that signal by listening
to it and calling controller.abort() so either signal cancels the request; clear
the timeout on successful response, and when abort occurs throw a clear
timeout-specific error (e.g., "Juplend fetch timeout") instead of waiting
forever.
In `@tests/integration/account.test.ts`:
- Around line 673-679: The tests create test clients via createClient({ account:
{ juplend: { pollIntervalMs: 60_000 }}}) but do not set a jupApiKey, so Juplend
enrichment is disabled and expected api.jup.ag calls remain zero; update the
test client config used in these failing specs (tests referencing createClient
in account.test.ts / the tests asserting api.jup.ag calls around the failing
assertions) to include a jupApiKey (a valid or mocked key) so enrichment runs
and the two api.jup.ag requests occur, or alternatively relax the assertions to
not require two calls—prefer adding jupApiKey to the createClient invocation to
fix the failures.
---
Nitpick comments:
In @.trellis/scripts/common/session_context.py:
- Around line 89-109: In _collect_git_repo_info rename the ambiguous loop
variable `l` used in the list comprehension that counts changes to a clearer
name like `line`; update the expression that builds the list from
status_out.splitlines() if l.strip() to use `line.strip()` instead so the
semantics remain the same and improve readability.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4983e953-d9ec-403c-b25b-508b3cec5285
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (40)
.agents/skills/trellis-spec-bootstarp/SKILL.md.agents/skills/trellis-spec-bootstarp/references/mcp-setup.md.agents/skills/trellis-spec-bootstarp/references/repository-analysis.md.agents/skills/trellis-spec-bootstarp/references/spec-task-planning.md.agents/skills/trellis-spec-bootstarp/references/spec-writing.md.claude/hooks/inject-workflow-state.py.claude/hooks/session-start.py.claude/settings.json.claude/skills/trellis-spec-bootstarp/SKILL.md.claude/skills/trellis-spec-bootstarp/references/mcp-setup.md.claude/skills/trellis-spec-bootstarp/references/repository-analysis.md.claude/skills/trellis-spec-bootstarp/references/spec-task-planning.md.claude/skills/trellis-spec-bootstarp/references/spec-writing.md.codex/config.toml.codex/hooks.json.codex/hooks/inject-workflow-state.py.codex/hooks/session-start.py.trellis/.template-hashes.json.trellis/.version.trellis/scripts/common/safe_commit.py.trellis/scripts/common/session_context.py.trellis/scripts/common/task_store.py.trellis/spec/backend/venue-lending.md.trellis/workflow.mdREADME.mddocs/api.mddocs/architecture.mdpackage.jsonscripts/live-juplend-account-smoke.tssrc/adapters/juplend/lend-read.tssrc/adapters/juplend/private-adapter.tssrc/client/context.tssrc/client/private-subscription-coordinator.tssrc/client/runtime.tssrc/managers/order-manager.tssrc/types/shared.tstests/integration/account.test.tstests/integration/order.test.tstests/support/exchanges/juplend.tstests/type/register-account-input.ts
Comment on lines
+448
to
+450
| if (cacheFresh && (cached.enriched || !apiKey)) { | ||
| return cached.vaults; | ||
| } |
There was a problem hiding this comment.
Cache fallback vault data when enrichment fails with API key enabled.
With the current freshness gate on Line 448 and the catch path on Line 476–478, repeated enrichment failures can trigger full reloads every poll instead of honoring TTL for fallback data.
🧩 Suggested fallback-cache behavior
- if (cacheFresh && (cached.enriched || !apiKey)) {
+ if (cacheFresh) {
return cached.vaults;
}
@@
try {
const enrichedVaults = await enrichVaultsWithJupApi({
apiKey,
baseVaults,
});
enrichmentCache.set(cacheKey, {
loadedAt: now,
vaults: enrichedVaults,
enriched: true,
});
return enrichedVaults;
} catch {
+ enrichmentCache.set(cacheKey, {
+ loadedAt: now,
+ vaults: baseVaults,
+ enriched: false,
+ });
return baseVaults;
}Also applies to: 465-478
🤖 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/adapters/juplend/private-adapter.ts` around lines 448 - 450, The current
freshness gate only returns cached.vaults when cacheFresh && (cached.enriched ||
!apiKey), which forces reloads if enrichment fails while an API key is present;
change the early-return to return cached.vaults whenever cacheFresh is true
(i.e., remove the enriched||!apiKey requirement) so fallback data is honored,
and in the enrichment catch block (the error handler around the enrichment logic
that currently retries/reloads) mark the cached entry as a fallback (e.g., set
cached.enriched = false or cached.fallback = true) and preserve its TTL so
subsequent polls use the cached vaults until the TTL expires.
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.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes / Behavior
Chores