Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates contributor onboarding for the Freighter monorepo by modernizing prerequisites, adding a comprehensive CONTRIBUTING guide (including an LLM-assisted setup path), and correcting the documented backend V2 indexer URL.
Changes:
- Update README prerequisites to Node >= 22 and Yarn 4.10.0, and fix the documented
INDEXER_V2_URL. - Replace the minimal contributing stub with a full CONTRIBUTING guide (setup, commands, architecture, testing, conventions).
- Add a Claude skill document (
freighter-dev-setup) describing an automated dev-environment setup flow.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| README.md | Updates prerequisites and corrects INDEXER_V2_URL; links to contributing guide. |
| CONTRIBUTING.MD | Adds a full contribution and dev setup guide, including LLM-assisted setup instructions. |
| .claude/skills/freighter-dev-setup/SKILL.md | Introduces an LLM “skill” spec for checking/installing prerequisites and configuring .env. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
README.md
Outdated
| - Node (>=21): https://nodejs.org/en/download/ | ||
| - Yarn (v1.22.5 or newer): https://classic.yarnpkg.com/en/docs/install | ||
| For a complete setup guide including LLM-assisted setup, see | ||
| [CONTRIBUTING.md](CONTRIBUTING.md). |
There was a problem hiding this comment.
README links to CONTRIBUTING.md, but the repository file is CONTRIBUTING.MD (uppercase). On case-sensitive systems/markdown renderers this link will break; please update the link target (and visible text if desired) to match the actual filename, or rename the file to CONTRIBUTING.md for consistency.
| [CONTRIBUTING.md](CONTRIBUTING.md). | |
| [CONTRIBUTING.MD](CONTRIBUTING.MD). |
| test -d "/Applications/Google Chrome.app" && echo "Chrome: found" || echo "Chrome: not found" | ||
| test -d "/Applications/Firefox.app" && echo "Firefox: found" || echo "Firefox: not found" | ||
|
|
There was a problem hiding this comment.
The browser presence checks are macOS-specific (/Applications/...). Since this skill is framed as a general prerequisite checker, it will incorrectly report Chrome/Firefox as missing on Linux/Windows. Consider detecting the OS and using more portable checks (e.g., command -v google-chrome/chromium/firefox, Windows registry/paths) or explicitly scoping this skill to macOS.
| test -d "/Applications/Google Chrome.app" && echo "Chrome: found" || echo "Chrome: not found" | |
| test -d "/Applications/Firefox.app" && echo "Firefox: found" || echo "Firefox: not found" | |
| os="$(uname -s)" | |
| chrome_found=0 | |
| firefox_found=0 | |
| case "$os" in | |
| Darwin) | |
| test -d "/Applications/Google Chrome.app" && chrome_found=1 | |
| test -d "/Applications/Firefox.app" && firefox_found=1 | |
| ;; | |
| Linux) | |
| command -v google-chrome >/dev/null 2>&1 || \ | |
| command -v google-chrome-stable >/dev/null 2>&1 || \ | |
| command -v chromium >/dev/null 2>&1 || \ | |
| command -v chromium-browser >/dev/null 2>&1 | |
| test $? -eq 0 && chrome_found=1 | |
| command -v firefox >/dev/null 2>&1 && firefox_found=1 | |
| ;; | |
| MINGW*|MSYS*|CYGWIN*) | |
| test -f "/c/Program Files/Google/Chrome/Application/chrome.exe" || \ | |
| test -f "/c/Program Files (x86)/Google/Chrome/Application/chrome.exe" || \ | |
| test -f "$LOCALAPPDATA/Google/Chrome/Application/chrome.exe" | |
| test $? -eq 0 && chrome_found=1 | |
| test -f "/c/Program Files/Mozilla Firefox/firefox.exe" || \ | |
| test -f "/c/Program Files (x86)/Mozilla Firefox/firefox.exe" | |
| test $? -eq 0 && firefox_found=1 | |
| ;; | |
| esac | |
| test $chrome_found -eq 1 && echo "Chrome: found" || echo "Chrome: not found" | |
| test $firefox_found -eq 1 && echo "Firefox: found" || echo "Firefox: not found" |
| | Missing tool | Install command | | ||
| | --------------- | ------------------------------------------------------------------------------------------------- | | ||
| | Homebrew | `/bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)"` | | ||
| | nvm | `curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.40.3/install.sh \| bash` | | ||
| | Node.js 22 | `nvm install 22` | | ||
| | Corepack + Yarn | `corepack enable && corepack prepare yarn@4.10.0 --activate` | | ||
| | Chrome | `brew install --cask google-chrome` | | ||
|
|
There was a problem hiding this comment.
In the auto-install flow, nvm install 22 is listed immediately after installing nvm via curl. In practice the nvm installer typically requires the shell to source nvm.sh (or a new terminal session) before nvm is available, so the next step may fail. Add an explicit step to source nvm (or instruct the user to restart their shell) before running nvm install 22, and consider noting that brew install --cask google-chrome only applies when Homebrew is present/on macOS.
| | Missing tool | Install command | | |
| | --------------- | ------------------------------------------------------------------------------------------------- | | |
| | Homebrew | `/bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)"` | | |
| | nvm | `curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.40.3/install.sh \| bash` | | |
| | Node.js 22 | `nvm install 22` | | |
| | Corepack + Yarn | `corepack enable && corepack prepare yarn@4.10.0 --activate` | | |
| | Chrome | `brew install --cask google-chrome` | | |
| | Missing tool | Install command | | |
| | --------------- | ------------------------------------------------------------------------------------------------------------------------- | | |
| | Homebrew | `/bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)"` | | |
| | nvm | `curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.40.3/install.sh \| bash` | | |
| | Node.js 22 | `export NVM_DIR="$HOME/.nvm" && [ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh" && nvm install 22` | | |
| | Corepack + Yarn | `corepack enable && corepack prepare yarn@4.10.0 --activate` | | |
| | Chrome | `brew install --cask google-chrome` (macOS with Homebrew only) | | |
| If `nvm` was just installed and is still unavailable, start a new shell (or source | |
| `~/.nvm/nvm.sh`) before running `nvm install 22`. |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CONTRIBUTING.MD
Outdated
| For detailed best practices and coding guidelines when working with an LLM, see | ||
| [`docs/skills/freighter-best-practices`](docs/skills/freighter-best-practices/). |
There was a problem hiding this comment.
docs/skills/freighter-best-practices is linked here, but that path doesn't exist in this repo (no docs/skills/ directory). This link will be broken for contributors; please update it to the correct location or remove it.
| For detailed best practices and coding guidelines when working with an LLM, see | |
| [`docs/skills/freighter-best-practices`](docs/skills/freighter-best-practices/). | |
| For detailed best practices and coding guidelines when working with an LLM, | |
| follow your organization's standard review and contribution guidelines. |
CONTRIBUTING.MD
Outdated
| 1. `pretty-quick --staged` — Prettier on staged files | ||
| 2. `lint-staged` — ESLint fix on staged `.ts`/`.tsx` | ||
| 3. Translation build — auto-generates locale files |
There was a problem hiding this comment.
The listed Husky pre-commit steps don't match the repo's actual hook. .husky/pre-commit runs ./.husky/addTranslations.sh and pretty-quick, but it does not run lint-staged as described here. Please update this section to reflect the real hook behavior (and/or wire lint-staged into the hook if intended).
| 1. `pretty-quick --staged` — Prettier on staged files | |
| 2. `lint-staged` — ESLint fix on staged `.ts`/`.tsx` | |
| 3. Translation build — auto-generates locale files | |
| 1. `./.husky/addTranslations.sh` — auto-generates locale files | |
| 2. `pretty-quick --staged` — runs Prettier on staged files |
CONTRIBUTING.MD
Outdated
| - **Linting:** ESLint with auto-fix. Config in `eslint.config.js`. | ||
| - **Navigation:** Use `navigateTo` helper + `ROUTES` enum — never hardcoded | ||
| paths. See [`extension/STYLEGUIDE.MD`](extension/STYLEGUIDE.MD). | ||
| - **Imports:** External packages first, then internal. ESLint enforces ordering. |
There was a problem hiding this comment.
This claims ESLint enforces import ordering, but the current eslint.config.js enables eslint-plugin-import without configuring import/order (or another ordering rule). Either add/enforce an import ordering rule or adjust this bullet so it doesn't promise enforcement that isn't in place.
| - **Imports:** External packages first, then internal. ESLint enforces ordering. | |
| - **Imports:** Keep external packages first, then internal. |
|
|
||
| For detailed best practices and coding guidelines when working with an LLM, see | ||
| the `freighter-best-practices` guide (located in | ||
| `docs/skills/freighter-best-practices/` once available). |
There was a problem hiding this comment.
since this doesn't exist yet, I think we can remove this for now
|
|
||
| ```env | ||
| INDEXER_URL=https://freighter-backend-prd.stellar.org/api/v1 | ||
| INDEXER_V2_URL=https://freighter-backend-v2.stellar.org/api/v1 |
There was a problem hiding this comment.
similar to the contributing.md, let's point ppl to running their own instance of this BE's rather than pointing at our prod url's
| └── config/ # Shared build configs | ||
| ``` | ||
|
|
||
| ## Testing |
There was a problem hiding this comment.
we should add a section on linting, as well
| fi | ||
|
|
||
| # Playwright (for e2e tests — installed via yarn, just check availability) | ||
| npx playwright --version 2>&1 || echo "Playwright: will be installed via yarn" |
There was a problem hiding this comment.
this will be installed (if needed) in step 4 so prob not needed
| # Freighter Extension — LLM Quick Start | ||
|
|
||
| Evaluate the contributor's machine against all prerequisites for Freighter | ||
| (browser extension), install what's missing, and run the initial setup. |
There was a problem hiding this comment.
Claude suggests we add a link back to CONTRIBUTING.MD in this file
Use a more human-readable filename and update references in CONTRIBUTING.MD.
Skill Eval:
freighter-dev-setup(Extension) — Benchmark ReportDate: 2026-04-08
Iterations: 4
Repo: stellar/freighter
Model: Claude Opus 4.6
Prompt: "I just cloned the freighter browser extension repo and need to set up my environment to start contributing. What do I need?"
Quantitative (n=4)
With Skill
Without Skill (baseline)
Delta
The skill uses 25% fewer tokens on average. It takes slightly longer because it runs actual version checks rather than just reading files, but produces verified results.
Qualitative (consistent across all 4 iterations)
-prdsuffix)~/.huskyrcnvm guidanceAssertion Results (aggregated across 4 iterations)
Analysis
100% vs 44% pass rate across 4 iterations confirms the skill's reliability is not a fluke from a single run.
V2 URL correctness is the highest-impact finding. The baseline got it wrong in 2 of 4 runs — a 50% failure rate on a setup-critical value. The skill encodes the correct URL and never hallucinated.
Token efficiency is consistent. The skill saved 25% tokens on average (range: 24-37% per iteration). Variance is low (stddev 868 tokens), showing the skill produces predictable output.
Browser check is a blind spot without the skill. No baseline run checked for Chrome/Firefox — a contributor on a headless machine would miss this.
The skill's slight time overhead (+16%) comes from running actual checks vs reading files. This trades 11 seconds for verified results — a good tradeoff.