fix: resolve TypeScript errors in web app source and tests - #240
Conversation
|
@Diyaaa-12 is attempting to deploy a commit to the Collins' projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Hi @collinsezedike just flagging that pnpm test surfaces 15 pre-existing failures in @meridian/web (all storage.setItem is not a function in wallet/vault action tests caused by a localStorage mock issue in the jsdom environment). These failures exist on main before my changes and are unrelated to this PR. Lint and typecheck both pass cleanly. And also vercel need authorization from your end. |
collinsezedike
left a comment
collinsezedike
left a comment
There was a problem hiding this comment.
Good work on narrowing res.json() and fixing the test cast. A few things to address before merge.
| "error" in body && | ||
| typeof (body as Record<string, unknown>).error === "string" | ||
| ? (body as Record<string, unknown>).error as string | ||
| : res.statusText) || `Request failed (${res.status})`; |
There was a problem hiding this comment.
The rewrite narrows body.error only when it is a string, but the original code also handled { error: { message: string } } and { message: string } via the err.error?.message ?? err.message chain. That fallback is now gone. Any error response shaped as { error: { message: "Insufficient balance" } } or { message: "Not found" } will fall through to res.statusText instead of the server's actual message. Worth adding a second narrowing branch for the nested object case to preserve the original behaviour.
| ? (body as Record<string, unknown>).error as string | ||
| : res.statusText) || `Request failed (${res.status})`; | ||
| throw new Error(msg); | ||
| } |
There was a problem hiding this comment.
The body of the if (!res.ok) block is sitting at 2-space indent (the same level as the if keyword itself) and the closing brace is at column 0. The logic is correct since the braces still match, but Prettier will reformat this on the next run and any reader scanning the file will misread throw new Error(msg) as unconditional. Re-indenting the block body to 4 spaces and the closing brace to 2 spaces will make the structure clear.
| const best = selectBestVault( | ||
| [ | ||
| vault({ id: "ondo", protocol: "ondo", apy: 12 }), | ||
| vault({ id: "ondo", protocol: "ondo" as unknown as "blend" | "defindex", apy: 12 }), |
There was a problem hiding this comment.
The as unknown as "blend" | "defindex" pattern gets past the compiler but tells TypeScript to treat "ondo" as a valid union member, which defeats the point of the test. If the protocol union gains a third value later, TypeScript will not flag this line even though the intent is to pass an unsupported protocol. A cleaner fix is to widen protocol in the vault() helper to accept string, so the call site reads vault({ protocol: "ondo" }) with no cast needed.
|
|
||
| it("returns null when nothing is routable", () => { | ||
| expect(selectBestVault([vault({ protocol: "ondo" })], opts)).toBeNull(); | ||
| expect(selectBestVault([vault({ protocol: "ondo" as unknown as "blend" | "defindex" })], opts)).toBeNull(); |
There was a problem hiding this comment.
Same pattern as line 37. Once the vault() helper accepts protocol?: string this cast can be removed here too.
|
The PR description says both |
The test suite passes cleanly on current main with 20 tests. Your branch may be behind. Kindly rebase onto the latest and the failures should clear up. The Vercel authorization is on my end and I will take care of it. |
…on, widen vault protocol type
|
Hi @collinsezedike , thank you for the detailed review! I've addressed all the feedback: api.ts - restored the nested error narrowing branches for { error: { message: string } } and { message: string } shapes, and fixed the indentation of the if (!res.ok) block |
collinsezedike
left a comment
collinsezedike
left a comment
There was a problem hiding this comment.
The narrowing logic and test helper fix are both correct. Pushed a small fixup to extract the repeated cast in the nested error branch.
collinsezedike
left a comment
collinsezedike
left a comment
There was a problem hiding this comment.
Re-approving after the fixup commit.
|
Thank you for the fix, @Diyaaa-12! If you're looking for more to pick up, feel free to browse the open issues. Tackling #7 would honestly be a huge relief for me. Merging now. |
2 participants
Summary
apps/web/src/lib/api.ts— typedres.json()result asunknownand added proper narrowing before accessing properties, handling three error shapes:{ error: string },{ error: { message: string } }, and{ message: string }apps/web/src/components/dashboard/VaultPanel.tsx— caste.targettoHTMLInputElementon both onChange handlers (deposit input and withdraw input)packages/stellar-sdk-helpers/src/routing.test.ts— widenedprotocolin thevault()helper to acceptstring, so test fixtures can pass"ondo"without any cast neededTest plan
pnpm lint && pnpm typecheck && pnpm testpass locallypnpm --filter @meridian/web typecheckexits 0pnpm --filter @meridian/stellar-sdk-helpers typecheckexits 0@ts-ignoreor blanket suppressions added to production codeCloses #223