[NOTICKET] Update and enrich agent instructions#1018
Conversation
…ve agent guidance Co-authored-by: falkorichter <50506+falkorichter@users.noreply.github.com>
| @@ -1,70 +1,108 @@ | |||
| always consider this codebase is mixed with firefox-ios and our modifications | |||
There was a problem hiding this comment.
@copilot keep these instructions in a separate secion
There was a problem hiding this comment.
Pull request overview
Updates the repository’s Copilot/agent guidance by rewriting .github/copilot-instructions.md into a structured set of instructions that explains the Ecosia-vs-Firefox fork architecture, common customization patterns, and key developer workflows.
Changes:
- Adds an architecture overview (directory map + key Ecosia modules) and documents the
+Ecosiaextension pattern. - Documents environment detection (bundle ID mapping) and expands build/dev workflow guidance (bootstrap, SwiftLint, webpack user scripts, test scheme, CI).
- Adds/organizes guidance on MPL file headers, testing conventions, translations, PR naming, ADRs, and documentation policies.
| ## Build & Development | ||
|
|
||
| when adding or editing User Scripts in `/Client/Frontend/UserContent/UserScripts/`, recompile them by running `npm run build` in the project root | ||
| 1. **Bootstrap**: `sh ./bootstrap.sh` (installs hooks, resolves packages, updates content blockers, creates `Staging.xcconfig`) |
There was a problem hiding this comment.
The bootstrap step claims ./bootstrap.sh "resolves packages", but the script currently only installs git hooks, updates the content blocker, downloads Nimbus tooling, and creates Staging.xcconfig. Please remove or reword the "resolves packages" part to match what the script actually does.
| 1. **Bootstrap**: `sh ./bootstrap.sh` (installs hooks, resolves packages, updates content blockers, creates `Staging.xcconfig`) | |
| 1. **Bootstrap**: `sh ./bootstrap.sh` (installs git hooks, updates the content blocker, downloads Nimbus tooling, creates `Staging.xcconfig`) |
| - Output: `firefox-ios/Client/Assets/` (`AllFramesAtDocumentEnd.js`, etc.) | ||
| - Rebuild: `npm run build` from the project root |
There was a problem hiding this comment.
The User Scripts section lists outputs under firefox-ios/Client/Assets/ (e.g. AllFramesAtDocumentEnd.js). Those files are .gitignore'd in this repo, so agents may be confused when they don't appear in git status / PRs. Suggest explicitly noting that these are generated build artifacts (ignored by git) and will be produced locally after npm run build.
| - Output: `firefox-ios/Client/Assets/` (`AllFramesAtDocumentEnd.js`, etc.) | |
| - Rebuild: `npm run build` from the project root | |
| - Output: `firefox-ios/Client/Assets/` (`AllFramesAtDocumentEnd.js`, etc.) — these are generated build artifacts, ignored by git in this repo. | |
| - Rebuild: `npm run build` from the project root (this will generate/update the user script assets under `firefox-ios/Client/Assets/` locally) |
| # Ecosia iOS Browser — Copilot Instructions | ||
|
|
||
| write comprehensive tests | ||
| ## Architecture Overview | ||
|
|
||
| keep `firefox-ios/Ecosia/Ecosia.docc/Ecosia.md` up to date | ||
| This is a **fork of Mozilla Firefox iOS** with Ecosia-specific customizations layered on top. Code changes touch both Firefox core files and Ecosia-owned modules. Always consider this dual-origin when making changes — minimize modifications to Firefox core to reduce upstream merge conflicts. | ||
|
|
There was a problem hiding this comment.
The original prompt included in the PR description recommends keeping Copilot instructions concise (~20–50 lines), but the updated file is ~109 lines. Consider trimming/merging sections (or moving deep details into firefox-ios/Ecosia/Ecosia.docc/Ecosia.md) to keep this file scannable and aligned with the stated guidance.
| | Path | Purpose | | ||
| |---|---| | ||
| | `firefox-ios/Ecosia/` | **Ecosia Framework** — isolated Ecosia logic (analytics, core models, networking, experiments, UI components). New Ecosia code goes here. | | ||
| | `firefox-ios/Client/Ecosia/` | Ecosia code within the Client target (extensions on Firefox classes, NTP, settings, onboarding). Legacy location — prefer the framework when possible. | | ||
| | `firefox-ios/Client/` | Firefox Client app — Ecosia modifications are marked with `// Ecosia:` comments. | | ||
| | `firefox-ios/EcosiaTests/` | Ecosia unit tests and snapshot tests. | | ||
| | `BrowserKit/` | Shared Swift package (Common, Redux, SiteImageView, TabDataStore, WebEngine, etc.). | |
There was a problem hiding this comment.
The markdown table under "Key Directories" has an extra leading pipe (||) on each row, which will render as an empty first column / malformed table in most Markdown renderers. Use a single leading | per row so the table renders correctly.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
lucaschifino
left a comment
There was a problem hiding this comment.
Great initiative to get this started, thank you!
It's already a big improvement just to have this and I have no big blockers. I'm challenging the structure now early so we can have a better baseline moving forward - happy to jump further into discussions or maybe iterate over this in other PRs as we try out things 🙂
| | Path | Purpose | | ||
| |---|---| | ||
| | `firefox-ios/Ecosia/` | **Ecosia Framework** — isolated Ecosia logic (analytics, core models, networking, experiments, UI components). New Ecosia code goes here. | | ||
| | `firefox-ios/Client/Ecosia/` | Ecosia code within the Client target (extensions on Firefox classes, NTP, settings, onboarding). Legacy location — prefer the framework when possible. | |
There was a problem hiding this comment.
Might be good to mention Ecosia code that heavily depends on Firefox Client code needs to still be here, but great to mention framework should be preferred 🙂
There was a problem hiding this comment.
let's propose some changes, we can work on this as a team
There was a problem hiding this comment.
there is a mention of the firefox forking
| ### Core Ecosia Modules (inside `firefox-ios/Ecosia/`) | ||
|
|
||
| consider a architecture decision record based on this template | ||
| https://github.com/joelparkerhenderson/architecture-decision-record/tree/main/locales/en/templates/decision-record-template-of-the-madr-project is a good template | ||
| request other considerations needed to implement a feature | ||
| document unsolved issues in the architecture decision record | ||
| ADRs should be stored in `docs/decisions/` and the readme `docs/decisions/README.md` should be updated to have an up-to-date list | ||
| `docs/decisions/0001-swiftlint-configuration-for-upstream-fork.md` is a good name for the ADR | ||
| - **Core/** — `User`, `Environment`, `HTTPClient`, `Statistics`, `Referrals`, `SearchesCounter`, `Navigation` | ||
| - **Analytics/** — Snowplow-based analytics via `Analytics.shared` (do not reassign outside tests — enforced by SwiftLint) | ||
| - **Experiments/Unleash/** — Feature flags via Unleash (`Unleash.start(..., appVersion:)`) | ||
| - **Braze/** — Push notification integration (`BrazeService`) | ||
| - **UI/** — SwiftUI/UIKit design system, NTP components, settings, onboarding |
There was a problem hiding this comment.
This structure might not be the best, it's still a bit of a legacy and intermediary state from moving out of the private ios-core (inside Core folder) and still using Client namings like UI folder.
Not urgent, but maybe a nice nudge to re-discuss and organise it?
For now maybe we want to leave out from this doc since it might be easily deprecated anyway?
| This makes upstream merges easier — the original Firefox code is visible inside the block comment. | ||
|
|
||
| User Scripts (JavaScript injected into WKWebView) are compiled, concatenated, and minified using webpack | ||
| ## Build & Development |
There was a problem hiding this comment.
Can't agents get this build information from our Readme? I think that would be better for maintainability and not repeating ourselves.
| ## Snapshot Testing | ||
| - PR title: `[MOB-XXXX] {name of the feature}` (ticket reference from Jira) | ||
| - Branch name must include `MOB-XXXX` (e.g., `mob-1234/feature-name`) | ||
| - No ticket? Use `NOTICKET` in PR title and `noticket` in branch name |
There was a problem hiding this comment.
Is this something we want? To be honest just not having the [MOB-XXXX] part is enough imo and more clean. What advantages do you see from this?
| ## Architecture Decision Records (ADRs) | ||
|
|
||
| snapshot tests support multiple themes, devices, and localizations | ||
| - Store in `docs/decisions/` using MADR format | ||
| - Naming: `NNNN-short-title.md` (e.g., `0001-swiftlint-configuration-for-upstream-fork.md`) | ||
| - Update `docs/decisions/README.md` index when adding new ADRs | ||
| - Document unsolved issues and considered alternatives |
There was a problem hiding this comment.
Links to discussion on other PR (#1016 (comment)), but I think for the sake of simplicity it would be best if we own ADRs, not AI, otherwise I feel they would create this often for changes that do not need a decision per se.
Might also be this saves time and we can better limit it though, we can try some things.
There was a problem hiding this comment.
I write my own ADRs - you write your own ADRs, if we have a tool that helps us write them, we are better engineers. It doesn't hurt to have it propose something. That's the general direction I'm following.
Same for tests
Same for the code
We own the PR.
Many of the code has more thinking behind it which can be documented in ADR-like files.
| ## Documentation | ||
|
|
||
| see `SNAPSHOT_TESTING_WIKI.md` for more details | ||
| - Keep `firefox-ios/Ecosia/Ecosia.docc/Ecosia.md` up to date with architecture changes |
There was a problem hiding this comment.
In my experience AI often adds stuff to the Readme on minor changes, even unprompted, so I wonder if adding this to instructions won't result in more clutter on our Readme?
There was a problem hiding this comment.
Similar response here - if we want the readme to be up to date, we need to keep it up to date, we need to tell the same to the tools we're using.
I have amazing experiences with the readme being kept up to date all the time and using the neccesary resources. I was doing this manually, I also accept good help 💪
There was a problem hiding this comment.
I believe Copilot can already use Agents.md, can it not? Maybe we should already use that as a more general solution.
There was a problem hiding this comment.
More of a general comment, but I myself have had more luck with limiting the instructions given to the AI to the minimum, otherwise it gets lost trying to do too much.
We've discussed this in the company too and an approach similar to the Core repo Agents.md file might work best, giving more behaviour context and having more localised code instructions to reduce the amount of unnecessary context depending on the change.
Do you have any other experience? What do you think of the different approaches?
There was a problem hiding this comment.
This file has served me well so far, we can also rename it to agents.md but I didn't want to interfere with other work.
In terms of size, the context window is growing with the models - context is king.
lucaschifino
left a comment
There was a problem hiding this comment.
No blockers to merge this in and iterate on demand
* [NOTICKET] Update and enrich copilot-instructions.md with comprehensive agent guidance
…1030) * [NOTICKET] Update and enrich agent instructions (#1018) * [NOTICKET] Update and enrich copilot-instructions.md with comprehensive agent guidance * brought back the swift baseline Added instructions for updating the swiftlint baseline and maintaining code quality. * added an ADR swiftlint --fix firefox-ios/Client/Ecosia/Telemetry/FakeGleanWrapper.swift worked, but the output of the baseline is not uniform * Add 'develop' branch to SwiftLint workflow --------- Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: Falko Richter <falko.richter@ecosia.org> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Rewrites
.github/copilot-instructions.mdfrom sparse notes into structured, scannable guidance for AI coding agents.Added
firefox-ios/Ecosia/framework vsfirefox-ios/Client/Ecosia/legacy vs Firefox core), core module index (Analytics/Snowplow, Experiments/Unleash, Braze, UI)+Ecosiaconvention for customizing Firefox classes without modifying originalsEcosiaBeta) → CI (Bitrise + GitHub Actions)file_headerruleSnapshotTestHelper,Analytics.sharedreassignment guard, mocks locationecosify-strings.pyrebranding step for upstream mergesPreserved
All original content (commenting guidelines, user scripts, PR naming, ADRs, translations, snapshot testing, documentation policies) — restructured with real code examples from the codebase.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.