Conversation
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a new BridgeHistory type and a public getHistory(bridgeIdentifier, walletAddress, limit?, offset?) API across Action and BaseTNClient, plus docs, an example, unit tests, a .gitignore entry, and an increased Vitest hookTimeout. Changes
Sequence DiagramsequenceDiagram
participant Client as Client App
participant BaseTN as BaseTNClient
participant Action as Action
participant Bridge as Bridge Endpoint
Client->>BaseTN: getHistory(bridgeId, wallet, limit, offset)
BaseTN->>Action: load action / getHistory(bridgeId, wallet, limit, offset)
Action->>Action: build params ($wallet_address, $limit, $offset)
Action->>Bridge: call <bridge>_get_history
Bridge-->>Action: BridgeHistory[] or null
Action-->>BaseTN: normalized BridgeHistory[]
BaseTN-->>Client: Promise<BridgeHistory[]>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/contracts-api/action.test.ts (1)
17-50: Test covers the happy path; consider adding edge-case coverage.The delegation test is solid. Two cases worth adding when convenient:
- Null/empty result normalization — verify that when the backend returns
null,getHistoryreturns[](therows || []fallback inaction.ts).- Default parameters — call
getHistory("bridge", "0x123")withoutlimit/offsetto confirm the defaults (20,0) are forwarded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts-api/action.test.ts` around lines 17 - 50, Add two new tests for Action.getHistory: one where the mocked call (spy on Action.call) resolves to Either.right(null) and assert getHistory(...) returns an empty array (exercise the rows || [] branch), and another where you invoke getHistory("bridge","0x123") without limit/offset and assert call was invoked with defaults $limit: 20 and $offset: 0; reuse the existing pattern of spying on action.call, mocking the resolved Either.right value, and asserting both the call arguments and the returned value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/api-reference.md`:
- Around line 632-634: The heading incorrectly attributes getHistory to
TransactionAction; update the docs to reference the correct owner: change
`transactionAction.getHistory(...)` to `client.getHistory(...)` (or
`action.getHistory(...)`) and ensure the text mentions that the method is
provided by Action (via client.loadAction()) and exposed on BaseTNClient as
`client.getHistory(...)`; also verify examples use `client.getHistory(...)`
consistently.
- Around line 656-669: Update the BridgeHistory documentation to match the
source type: change the to_address field to "string | null" (nullable) and
expand the type field to include 'transfer' in addition to 'deposit' and
'withdrawal'; ensure the doc block for the BridgeHistory interface reflects
these exact symbols (BridgeHistory, to_address, type) so it matches
src/types/bridge.ts JSDoc.
In `@src/types/bridge.ts`:
- Around line 79-133: BridgeHistory currently declares to_address as "string |
null" but the API docs treat it as non-nullable; make the type definition the
source of truth by changing the BridgeHistory interface's to_address property to
"string" (non-nullable) and update its JSDoc comment accordingly so the
interface (BridgeHistory and its to_address field) reflects that it always
contains a string.
---
Nitpick comments:
In `@src/contracts-api/action.test.ts`:
- Around line 17-50: Add two new tests for Action.getHistory: one where the
mocked call (spy on Action.call) resolves to Either.right(null) and assert
getHistory(...) returns an empty array (exercise the rows || [] branch), and
another where you invoke getHistory("bridge","0x123") without limit/offset and
assert call was invoked with defaults $limit: 20 and $offset: 0; reuse the
existing pattern of spying on action.call, mocking the resolved Either.right
value, and asserting both the call arguments and the returned value.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/api-reference.md`:
- Around line 145-181: Move the misplaced getHistory docs from the "Stream
Querying" section into the "Bridge Operations" section so it sits alongside
related bridge methods; locate the documentation block for action.getHistory /
client.getHistory (the heading and code block beginning
"action.getHistory(bridgeIdentifier: string...") currently under the "Stream
Querying" heading and cut/paste it into the "Bridge Operations" section near the
docs for getWalletBalance, withdraw, and getWithdrawalProof to maintain
parameter and discoverability consistency. Ensure the heading level and
surrounding anchors/TOC entries remain consistent after relocation.
resolves: https://github.com/truflation/website/issues/3313
Summary by CodeRabbit
New Features
Documentation
Tests
Chores