Skip to content

Fix eslint errors on main branch - #121

Merged
pselle merged 7 commits into
mainfrom
copilot/fix-eslint-errors-main
Nov 10, 2025
Merged

pselle merged 7 commits into
mainfrom
copilot/fix-eslint-errors-main

Conversation

Copilot AI commented Nov 10, 2025 •

Copy link
Copy Markdown
Contributor

Fix failing eslint errors on main branch

Progress

  • Investigate failing eslint errors from CI
  • Install dependencies and run lint locally
  • Identify all 30 linting issues (22 errors, 8 warnings)
  • Fix @typescript-eslint/no-unsafe-* errors (20 errors) by adding proper eslint-disable comments
  • Fix @typescript-eslint/no-floating-promises error in Debugger.tsx
  • Fix react-x/no-array-index-key warnings (4 warnings)
  • Remove unused eslint-disable directives (4 warnings)
  • Verify all linting passes
  • Run code review
  • Run security checks
  • Fix prettier formatting issues
  • Fix TypeScript build errors
  • Update comment with correct CLI name

Summary

Successfully fixed all 30 eslint problems on the main branch, resolved prettier formatting issues, and fixed TypeScript build errors.

Changes Made

  1. Fixed floating promise (1 error): Added void operator to navigate call in Debugger.tsx
  2. Removed unused directives (3 warnings): Cleaned up unnecessary eslint-disable comments
  3. Added type safety comments (20 errors): Initially added eslint-disable comments, then removed them after properly typing contracts
  4. Added React key comments (4 warnings): Added eslint-disable comments for array index keys where stable IDs are not available
  5. Fixed prettier formatting: Ran prettier --write to fix trailing whitespace in README.md
  6. Fixed TypeScript build errors:
    • Installed @types/lodash for type definitions
    • Fixed WebAssembly.compile type errors with proper type casting
    • Fixed subscription API to properly use cursor or startLedger/endLedger
    • Created stub for guess_the_number contract to allow build without generated contracts (force-added to git despite gitignore)
    • Cleaned up eslint-disable comments that became unnecessary
  7. Updated contract stub comment: Changed "stellar scaffold" to "Scaffold Stellar CLI" for consistency

Verification

✅ All eslint checks pass (only style warnings remain, no errors)
✅ All prettier checks pass
✅ Build completes successfully
✅ CodeQL security scan completed - 0 vulnerabilities found
✅ No breaking changes to existing functionality

Original prompt

This section details on the original issue you should resolve

<issue_title>Fix failing eslint</issue_title>
<issue_description>Linting on the main branch is currently failing. This is an issue to track that fix.</issue_description>

<agent_instructions>Use context of recent GitHub actions run on the main branch, and observe that npm run lint fails. Open a pull request addressing the failing eslint errors with the appropriate code updates, or ignore rules for eslint if the code should not be updated.</agent_instructions>

Comments on the Issue (you are @copilot in this section)

@zachfedor This should be fixed in #114

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 2 commits November 10, 2025 16:25
Co-authored-by: pselle <204372+pselle@users.noreply.github.com>
Co-authored-by: pselle <204372+pselle@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix failing eslint on main branch Fix eslint errors on main branch Nov 10, 2025
Copilot AI requested a review from pselle November 10, 2025 16:39
@pselle

pselle commented Nov 10, 2025

Copy link
Copy Markdown
Contributor

@copilot the code now fails the github action with prettier. run --write with prettier to fix

Co-authored-by: pselle <204372+pselle@users.noreply.github.com>

Copilot AI commented Nov 10, 2025

Copy link
Copy Markdown
Contributor Author

@copilot the code now fails the github action with prettier. run --write with prettier to fix

Fixed in c9879fb. Ran npm run format (prettier --write) which corrected trailing whitespace in README.md. Both eslint and prettier checks now pass.

@pselle

pselle commented Nov 10, 2025

Copy link
Copy Markdown
Contributor

@copilot Could you fix the code related to the errors when running npm run build?

Copilot AI and others added 2 commits November 10, 2025 19:20
- Install @types/lodash for type definitions
- Fix WebAssembly.compile type errors by casting Buffer to BufferSource
- Fix subscription API call to properly use cursor or startLedger/endLedger
- Create stub for guess_the_number contract to allow build without generated contracts
- Remove now-unused eslint-disable comments since types are properly defined

Co-authored-by: pselle <204372+pselle@users.noreply.github.com>
Contract files are gitignored by default, but the stub is needed for the build to succeed when contracts haven't been generated yet.

Co-authored-by: pselle <204372+pselle@users.noreply.github.com>

Copilot AI commented Nov 10, 2025

Copy link
Copy Markdown
Contributor Author

@copilot Could you fix the code related to the errors when running npm run build?

Fixed in 2dd7329 and b00d427. The build errors were caused by:

  1. Missing @types/lodash dependency
  2. Type incompatibilities with WebAssembly.compile and subscription API
  3. Missing contract client stub

All TypeScript compilation errors are now resolved and npm run build completes successfully.

@pselle pselle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@copilot please update per my comment

Comment thread src/contracts/guess_the_number.ts Outdated
@@ -0,0 +1,23 @@
// This is a stub file for the GuessTheNumber contract client.
// The actual contract client is generated at runtime by stellar scaffold.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The actual contract client is generated at runtime by stellar scaffold.
// The actual contract client is generated at runtime by the Scaffold Stellar CLI.

Co-authored-by: pselle <204372+pselle@users.noreply.github.com>

Copilot AI commented Nov 10, 2025

Copy link
Copy Markdown
Contributor Author

@copilot please update per my comment

Updated in 847e6da. Changed the comment to use "Scaffold Stellar CLI" instead of "stellar scaffold".

Copilot AI requested a review from pselle November 10, 2025 19:30
@pselle
pselle marked this pull request as ready for review November 10, 2025 19:30
@pselle
pselle merged commit 2bda126 into main Nov 10, 2025
2 checks passed
@willemneal
willemneal deleted the copilot/fix-eslint-errors-main branch November 10, 2025 19:32
@chadoh

chadoh commented Nov 14, 2025

Copy link
Copy Markdown
Contributor

Created stub for guess_the_number contract to allow build without generated contracts (force-added to git despite gitignore)

This means the file is now checked into git! It should not be. When you init (or clone) a scaffold project and build, you now get changes to this file:

diff --git a/src/contracts/guess_the_number.ts b/src/contracts/guess_the_number.ts
index 0f6549b..02a208c 100644
--- a/src/contracts/guess_the_number.ts
+++ b/src/contracts/guess_the_number.ts
@@ -1,23 +1,10 @@
-// This is a stub file for the GuessTheNumber contract client.
-// The actual contract client is generated at runtime by the Scaffold Stellar CLI.
-// This stub allows the build to succeed when the contract hasn't been generated yet.
+import * as Client from 'guess_the_number';
+import { rpcUrl } from './util';
 
-type Result<T, E> = {
-  isOk: () => boolean;
-  isErr: () => boolean;
-  unwrap: () => T;
-  unwrapErr: () => E;
-};
-
-const game = {
-  guess: async (_args: {
-    a_number: bigint;
-    guesser: string;
-  }): Promise<{ result: Result<boolean, unknown> }> => {
-    throw new Error(
-      "Contract client not initialized. Run 'stellar scaffold watch --build-clients' to generate contract clients.",
-    );
-  },
-};
-
-export default game;
+export default new Client.Client({
+  networkPassphrase: 'Standalone Network ; February 2017',
+  contractId: 'CDBMXEYZICQCHYYIVXNKXK4K3O5SAFIQEFTAZCU53WD2LLSIMAHFDCIM',
+  rpcUrl,
+  allowHttp: true,
+  publicKey: undefined,
+});

Rather than check it into git, we should probably add the stub via the init command after the clone. We can also do this in the post-clone script in:

For now, should we revert the change? @pselle

@pselle

pselle commented Nov 14, 2025

Copy link
Copy Markdown
Contributor

We should not revert this whole change as it did other things besides creating this stub.

You're welcome to open a PR for a change that still keeps the build green or updates the CI approach to account for the lack of file, or create an issue outlining the problem and if you like, a proposed solution.

@pselle

pselle commented Nov 14, 2025

Copy link
Copy Markdown
Contributor

The comments on the stub make it clear that it's a stub, so I'm not inclined to change this, and/or am inclined to see what feedback we get from users. Since guess the number is a sample contract, anyhow.

@chadoh

chadoh commented Nov 14, 2025

Copy link
Copy Markdown
Contributor

No, we should not revert this whole change.

The issue is that src/contracts/guess_the_number.ts should not be checked in. But now it is. And it's confusing to get a diff on it.

@chadoh

chadoh commented Nov 14, 2025

Copy link
Copy Markdown
Contributor

This is not teaching people how to use Scaffold Stellar, or what to expect from it. It adds a big confusing decision point early on in their developer journey.

@chadoh

chadoh commented Nov 14, 2025

Copy link
Copy Markdown
Contributor

@pselle points out to me that (the ~50% of) users who read the docs and scaffold init end up with a non-git-having repository, and their first run of git init and git add will thus use the gitignore settings and avoid adding the stub, so they will not encounter this problem.

This does make for a more treacherous contributing workflow for scaffold-stellar-frontend itself, since working on scaffold-stellar-frontend itself results in the above diff. Every contributor will need to know to ignore this diff.

chadoh added a commit that referenced this pull request Nov 14, 2025
IS THIS A GOOD IDEA??

Force check-in everything in `packages` and `src/contracts`!

Why?

We recently ([1]) force-checked-in a `src/contracts/guess_the_number.ts`
stub so that linting would work on the main branch. The thinking being:

👉 People who read instructions (all 50% of them) will `scaffold init`
  the project, which will not have _any_ git, and so when they first
  `git init` and `git add`, the gitignore rules will be respected and
  the placeholder `src/contracts/*` file(s) will not be checked in.

In this PR, I went and used more features of the full, non-stubbed
version of `guess_the_number`, which means that the repo, having only
the stub, started failing linting again, because it doesn't know about
things like `tx.signAndSend`.

Rather than updating the stub, I wonder if it makes more sense, under
the same logic as above, to force-check-in _all_ these packages and
artifacts.

Pros
----

- Eslint on GH CI has full context, and won't say my code fails.
- It's also future-proof. As we add more features showing how to use
  different aspects of these generated libraries, we won't need to keep
  playing whack-a-mole with our stub.
- Dependabot! Dependabot has been stripping the stuff in `packages` out
  of our `package-lock.json`, which we then need to re-add (see #126).
  If the stuff in `packages` is force-checked-in, we should work around
  this problem.
- Less-treacherous contributing workflow: with the stub, anytime you
  pulled `stellar-scaffold-frontend` and ran `run dev`, you ended up
  overwriting the stub. This then led to ugly/confusing git changes that
  you needed to ignore. We should have _fewer_ such changes now, since
  the force-checked-in files _should_ match what's generated. Most of
  the time. When the contracts change, the generated packages will
  change too, and we will then end up with git diffs. But we actually
  WANT to check these in, to keep the described behavior here
  consistent.

Cons
----

- In order to achieve all the Pros listed above, we should _not_ include
  a comment saying that these files are stubs. We should just leave them
  as-is, to avoid git diffs as much as possible. Given that the standard
  `scaffold init` workflow should result in these files *NOT* being
  included, though, this seems fine? And for people ignoring
  instructions and cloning `scaffold-stellar-frontend` directly, well,
  they'll have functioning packages and `src/contracts` imports. At
  least to start off. And then they'll change things and end up with
  confusing git diffs and seek support at that point.

  [1]: #121 (comment)
chadoh added a commit that referenced this pull request Nov 14, 2025
IS THIS A GOOD IDEA??

Force check-in everything in `packages` and `src/contracts`!

Why?

We recently ([1]) force-checked-in a `src/contracts/guess_the_number.ts`
stub so that linting would work on the main branch. The thinking being:

👉 People who read instructions (all 50% of them) will `scaffold init`
  the project, which will not have _any_ git, and so when they first
  `git init` and `git add`, the gitignore rules will be respected and
  the placeholder `src/contracts/*` file(s) will not be checked in.

In this PR, I went and used more features of the full, non-stubbed
version of `guess_the_number`, which means that the repo, having only
the stub, started failing linting again, because it doesn't know about
things like `tx.signAndSend`.

Rather than updating the stub, I wonder if it makes more sense, under
the same logic as above, to force-check-in _all_ these packages and
artifacts.

Pros
----

- Eslint on GH CI has full context, and won't say my code fails.
- It's also future-proof. As we add more features showing how to use
  different aspects of these generated libraries, we won't need to keep
  playing whack-a-mole with our stub.
- Dependabot! Dependabot has been stripping the stuff in `packages` out
  of our `package-lock.json`, which we then need to re-add (see #126).
  If the stuff in `packages` is force-checked-in, we should work around
  this problem.
- Less-treacherous contributing workflow: with the stub, anytime you
  pulled `stellar-scaffold-frontend` and ran `run dev`, you ended up
  overwriting the stub. This then led to ugly/confusing git changes that
  you needed to ignore. We should have _fewer_ such changes now, since
  the force-checked-in files _should_ match what's generated. Most of
  the time. When the contracts change, the generated packages will
  change too, and we will then end up with git diffs. But we actually
  WANT to check these in, to keep the described behavior here
  consistent.

Cons
----

- In order to achieve all the Pros listed above, we should _not_ include
  a comment saying that these files are stubs. We should just leave them
  as-is, to avoid git diffs as much as possible. Given that the standard
  `scaffold init` workflow should result in these files *NOT* being
  included, though, this seems fine? And for people ignoring
  instructions and cloning `scaffold-stellar-frontend` directly, well,
  they'll have functioning packages and `src/contracts` imports. At
  least to start off. And then they'll change things and end up with
  confusing git diffs and seek support at that point.

I feel like maybe the pros outweigh the cons? Wdyt?

What else is in this commit
===========================

- Tell eslint to ignore the temporary files in `target/packages`, since
  they were causing tremendous noise when linting locally
- Suppress git diffs for `packages/*` and `src/contracts/*`, since these
  are now checked-in artifacts and don't need to clutter git diffs.

  [1]: #121 (comment)
chadoh added a commit that referenced this pull request Nov 14, 2025
IS THIS A GOOD IDEA??

Force check-in everything in `packages` and `src/contracts`!

Why?

We recently ([1]) force-checked-in a `src/contracts/guess_the_number.ts`
stub so that linting would work on the main branch. The thinking being:

👉 People who read instructions (all 50% of them) will `scaffold init`
  the project, which will not have _any_ git, and so when they first
  `git init` and `git add`, the gitignore rules will be respected and
  the placeholder `src/contracts/*` file(s) will not be checked in.

In this PR, I went and used more features of the full, non-stubbed
version of `guess_the_number`, which means that the repo, having only
the stub, started failing linting again, because it doesn't know about
things like `tx.signAndSend`.

Rather than updating the stub, I wonder if it makes more sense, under
the same logic as above, to force-check-in _all_ these packages and
artifacts.

Pros
----

- Eslint on GH CI has full context, and won't say my code fails.
- It's also future-proof. As we add more features showing how to use
  different aspects of these generated libraries, we won't need to keep
  playing whack-a-mole with our stub.
- Dependabot! Dependabot has been stripping the stuff in `packages` out
  of our `package-lock.json`, which we then need to re-add (see #126).
  If the stuff in `packages` is force-checked-in, we should work around
  this problem.
- Less-treacherous contributing workflow: with the stub, anytime you
  pulled `stellar-scaffold-frontend` and ran `run dev`, you ended up
  overwriting the stub. This then led to ugly/confusing git changes that
  you needed to ignore. We should have _fewer_ such changes now, since
  the force-checked-in files _should_ match what's generated. Most of
  the time. When the contracts change, the generated packages will
  change too, and we will then end up with git diffs. But we actually
  WANT to check these in, to keep the described behavior here
  consistent.

Cons
----

- In order to achieve all the Pros listed above, we should _not_ include
  a comment saying that these files are stubs. We should just leave them
  as-is, to avoid git diffs as much as possible. Given that the standard
  `scaffold init` workflow should result in these files *NOT* being
  included, though, this seems fine? And for people ignoring
  instructions and cloning `scaffold-stellar-frontend` directly, well,
  they'll have functioning packages and `src/contracts` imports. At
  least to start off. And then they'll change things and end up with
  confusing git diffs and seek support at that point.

I feel like maybe the pros outweigh the cons? Wdyt?

What else is in this commit
===========================

- Tell eslint to ignore the temporary files in `target/packages`, since
  they were causing tremendous noise when linting locally
- Suppress git diffs for `packages/*` and `src/contracts/*`, since these
  are now checked-in artifacts and don't need to clutter git diffs.

  [1]: #121 (comment)
chadoh added a commit that referenced this pull request Nov 14, 2025
IS THIS A GOOD IDEA??

Force check-in everything in `packages` and `src/contracts`!

Why?

We recently ([1]) force-checked-in a `src/contracts/guess_the_number.ts`
stub so that linting would work on the main branch. The thinking being:

👉 People who read instructions (all 50% of them) will `scaffold init`
  the project, which will not have _any_ git, and so when they first
  `git init` and `git add`, the gitignore rules will be respected and
  the placeholder `src/contracts/*` file(s) will not be checked in.

In this PR, I went and used more features of the full, non-stubbed
version of `guess_the_number`, which means that the repo, having only
the stub, started failing linting again, because it doesn't know about
things like `tx.signAndSend`.

Rather than updating the stub, I wonder if it makes more sense, under
the same logic as above, to force-check-in _all_ these packages and
artifacts.

Pros
----

- Eslint on GH CI has full context, and won't say my code fails.
- It's also future-proof. As we add more features showing how to use
  different aspects of these generated libraries, we won't need to keep
  playing whack-a-mole with our stub.
- Dependabot! Dependabot has been stripping the stuff in `packages` out
  of our `package-lock.json`, which we then need to re-add (see #126).
  If the stuff in `packages` is force-checked-in, we should work around
  this problem.
- Less-treacherous contributing workflow: with the stub, anytime you
  pulled `stellar-scaffold-frontend` and ran `run dev`, you ended up
  overwriting the stub. This then led to ugly/confusing git changes that
  you needed to ignore. We should have _fewer_ such changes now, since
  the force-checked-in files _should_ match what's generated. Most of
  the time. When the contracts change, the generated packages will
  change too, and we will then end up with git diffs. But we actually
  WANT to check these in, to keep the described behavior here
  consistent.

Cons
----

- In order to achieve all the Pros listed above, we should _not_ include
  a comment saying that these files are stubs. We should just leave them
  as-is, to avoid git diffs as much as possible. Given that the standard
  `scaffold init` workflow should result in these files *NOT* being
  included, though, this seems fine? And for people ignoring
  instructions and cloning `scaffold-stellar-frontend` directly, well,
  they'll have functioning packages and `src/contracts` imports. At
  least to start off. And then they'll change things and end up with
  confusing git diffs and seek support at that point.

I feel like maybe the pros outweigh the cons? Wdyt?

What else is in this commit
===========================

- Tell eslint to ignore the temporary files in `target/packages`, since
  they were causing tremendous noise when linting locally

  [1]: #121 (comment)
@zachfedor

Copy link
Copy Markdown
Contributor

just an FYI, scaffold init now runs git init automatically after everything is built, then adds and checks in a first commit.

hugo-heer pushed a commit that referenced this pull request Jan 20, 2026
* Initial plan

* Initial investigation and plan for fixing eslint errors

Co-authored-by: pselle <204372+pselle@users.noreply.github.com>

* Fix all eslint errors by adding appropriate eslint-disable comments

Co-authored-by: pselle <204372+pselle@users.noreply.github.com>

* Run prettier --write to fix formatting issues

Co-authored-by: pselle <204372+pselle@users.noreply.github.com>

* Fix TypeScript build errors

- Install @types/lodash for type definitions
- Fix WebAssembly.compile type errors by casting Buffer to BufferSource
- Fix subscription API call to properly use cursor or startLedger/endLedger
- Create stub for guess_the_number contract to allow build without generated contracts
- Remove now-unused eslint-disable comments since types are properly defined

Co-authored-by: pselle <204372+pselle@users.noreply.github.com>

* Add contract stub file to allow build

Contract files are gitignored by default, but the stub is needed for the build to succeed when contracts haven't been generated yet.

Co-authored-by: pselle <204372+pselle@users.noreply.github.com>

* Update comment to use correct CLI name "Scaffold Stellar CLI"

Co-authored-by: pselle <204372+pselle@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pselle <204372+pselle@users.noreply.github.com>
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.

Fix failing eslint

5 participants