Skip to content

Conversation

@bordalix
Copy link
Collaborator

@bordalix bordalix commented Nov 15, 2025

Summary by CodeRabbit

  • Chores

    • Updated SDK dependency to latest version
  • Tests

    • Enhanced wallet send validation test coverage

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 15, 2025

Walkthrough

Dependency version bump of @arkade-os/sdk from 0.3.6 to 0.3.7 in package.json. Test file send.test.tsx modified with type casting relaxations and test assertion updates for satoshi validation.

Changes

Cohort / File(s) Summary
Dependency Update
package.json
Updated @arkade-os/sdk from 0.3.6 to 0.3.7
Test Modifications
src/test/screens/wallet/send.test.tsx
Added type casting (as any) for FlowContext.Provider value in two test locations; changed test assertion from expecting "Continue" button to expecting "Amount below 1 satoshi" text

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

  • Verify compatibility of @arkade-os/sdk 0.3.7 patch update
  • Confirm test assertion change aligns with updated satoshi validation logic

Possibly related PRs

  • Send tests #218: Originally added the send.test.tsx test file that is now being modified in this PR.

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'update dependencies' is generic and vague. While a dependency version bump is present in package.json, the PR also includes test changes that alter test expectations and type casting, making the title incomplete and partially misleading about the full scope of changes. Consider a more specific title that reflects all changes, such as 'Update @arkade-os/sdk dependency and fix send wallet tests' to better communicate the full intent of the PR.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch revert_kukks

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cloudflare-workers-and-pages
Copy link

Deploying wallet-signet with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4aea271
Status: ✅  Deploy successful!
Preview URL: https://2bdfa693.wallet-23u.pages.dev
Branch Preview URL: https://revert-kukks.wallet-23u.pages.dev

View logs

@cloudflare-workers-and-pages
Copy link

Deploying wallet-mutinynet with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4aea271
Status: ✅  Deploy successful!
Preview URL: https://1df4a3bf.arkade-wallet.pages.dev
Branch Preview URL: https://revert-kukks.arkade-wallet.pages.dev

View logs

@cloudflare-workers-and-pages
Copy link

Deploying wallet-bitcoin with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4aea271
Status: ✅  Deploy successful!
Preview URL: https://0e6c3901.wallet-bitcoin.pages.dev
Branch Preview URL: https://revert-kukks.wallet-bitcoin.pages.dev

View logs

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 806b0bf and 4aea271.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (2)
  • package.json (1 hunks)
  • src/test/screens/wallet/send.test.tsx (3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.

Applied to files:

  • src/test/screens/wallet/send.test.tsx
🧬 Code graph analysis (1)
src/test/screens/wallet/send.test.tsx (2)
src/providers/flow.tsx (1)
  • FlowContext (89-104)
src/test/screens/mocks.ts (1)
  • mockFlowContextValue (79-92)
⏰ 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: test
🔇 Additional comments (1)
package.json (1)

8-8: Verify the SDK upgrade manually—public release notes unavailable.

No security advisories were found for @arkade-os/sdk. However, public release notes for v0.3.7 could not be located, and the package repository information is not available through standard sources. Manually verify the upgrade by checking the package source directly (e.g., npm package page, GitHub releases, or package documentation) to confirm any behavioral changes and compatibility with your codebase, particularly given the test modifications in this PR.

<LightningContext.Provider value={mockLightningContextValue as any}>
<OptionsContext.Provider value={mockOptionsContextValue as any}>
<FlowContext.Provider value={mockFlowContextValue}>
<FlowContext.Provider value={mockFlowContextValue as any}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Fix the type error instead of suppressing it with as any.

The type cast hides a legitimate type mismatch. Looking at the relevant code snippets, mockFlowContextValue is missing the deepLinkInfo property that FlowContextProps requires.

In src/test/screens/mocks.ts, add the missing property to the mock:

export const mockFlowContextValue = {
  txInfo: mockTxInfo,
  swapInfo: undefined,
  initInfo: emptyInitInfo,
  noteInfo: emptyNoteInfo,
  deepLinkInfo: undefined,  // Add this line
  recvInfo: emptyRecvInfo,
  sendInfo: emptySendInfo,
  setInitInfo: () => {},
  setNoteInfo: () => {},
  setDeepLinkInfo: () => {},  // Add this if missing
  setRecvInfo: () => {},
  setSendInfo: () => {},
  setSwapInfo: () => {},
  setTxInfo: () => {},
}

Then remove the as any casts from lines 35 and 60:

-                  <FlowContext.Provider value={mockFlowContextValue as any}>
+                  <FlowContext.Provider value={mockFlowContextValue}>

Also applies to: 60-60

🤖 Prompt for AI Agents
In src/test/screens/wallet/send.test.tsx around lines 35 and 60, the test is
using "mockFlowContextValue as any" to silence a type mismatch; instead add the
missing deepLinkInfo property (and its setter) to the mockFlowContextValue in
src/test/screens/mocks.ts so the object conforms to FlowContextProps, then
remove the "as any" casts on lines 35 and 60; ensure deepLinkInfo is present
(can be undefined) and the corresponding setDeepLinkInfo is a noop function so
the mock satisfies the required type.

Comment on lines 82 to +83
// button continue should be present
expect(screen.getByText('Continue')).toBeInTheDocument()
expect(screen.getByText('Amount below 1 satoshi')).toBeInTheDocument()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

Update the outdated comment and verify the behavioral change.

The comment on line 82 states "button continue should be present", but the assertion now checks for validation text "Amount below 1 satoshi" instead. This suggests the UI behavior has changed, possibly due to the SDK upgrade introducing stricter amount validation.

Update the comment to reflect the current behavior:

-    // button continue should be present
+    // validation message should appear for amounts below 1 satoshi
     expect(screen.getByText('Amount below 1 satoshi')).toBeInTheDocument()

Additionally, verify this behavioral change is intentional by checking the SendForm component to confirm the validation logic was updated:


🏁 Script executed:

#!/bin/bash
# Find the validation logic for minimum satoshi amount
ast-grep --pattern 'below 1 satoshi'

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Search for satoshi validation logic in the codebase
rg -i "satoshi" --type=ts --type=tsx -B 3 -A 3

Length of output: 27


🏁 Script executed:

#!/bin/bash
# Search for SendForm component to verify validation implementation
fd "SendForm" --type=ts --type=tsx --exec grep -l "satoshi\|Amount below" {} \;

Length of output: 169


🏁 Script executed:

#!/bin/bash
# Search more broadly for the validation message
rg "Amount below" --type=ts --type=tsx -B 2 -A 2

Length of output: 27


🏁 Script executed:

#!/bin/bash
# Search for satoshi validation without type restrictions
rg "satoshi" -B 3 -A 3

Length of output: 37205


🏁 Script executed:

#!/bin/bash
# Search for the validation message text
rg "Amount below" -B 2 -A 2

Length of output: 2437


🏁 Script executed:

#!/bin/bash
# Find SendForm component
fd "SendForm" -e ts -e tsx

Length of output: 0


Update the outdated comment to reflect the current validation behavior.

The comment on line 82 is outdated. The test now verifies that a validation message appears when the amount is below 1 satoshi, not that a "Continue" button is present. This behavioral change is intentional—the SendForm component in src/screens/Wallet/Send/Form.tsx includes validation logic that sets "Amount below 1 satoshi" as the button label when satoshis < 1.

Update the comment:

-    // button continue should be present
+    // validation message should appear for amounts below 1 satoshi
     expect(screen.getByText('Amount below 1 satoshi')).toBeInTheDocument()
📝 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
// button continue should be present
expect(screen.getByText('Continue')).toBeInTheDocument()
expect(screen.getByText('Amount below 1 satoshi')).toBeInTheDocument()
// validation message should appear for amounts below 1 satoshi
expect(screen.getByText('Amount below 1 satoshi')).toBeInTheDocument()
🤖 Prompt for AI Agents
In src/test/screens/wallet/send.test.tsx around lines 82-83, the inline comment
"button continue should be present" is outdated; update it to accurately reflect
that the test asserts the validation message/label for amounts below 1 satoshi.
Replace the comment with a short note such as "button shows 'Amount below 1
satoshi' for satoshis < 1" (or similar) so the comment matches the
expect(screen.getByText('Amount below 1 satoshi')).toBeInTheDocument()
assertion.

@bordalix bordalix merged commit 09e6cb2 into master Nov 15, 2025
6 checks passed
@bordalix bordalix deleted the revert_kukks branch November 26, 2025 14:29
This was referenced Jan 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants