-
-
Notifications
You must be signed in to change notification settings - Fork 5
[WIP]: Create AGENTS.md repository guidelines document with comprehensive development rules and context #1898
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new AGENTS.md documentation file establishing centralized guidelines for the project. The file documents repository structure, service and component organization patterns, development workflows, testing procedures, formatting conventions, and infrastructure setup instructions across backend services and frontend components. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes This is a purely additive documentation file with no code logic or structural changes. Review focus should be:
Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
|
@coderabbitai review Please review the agents.md for correctness/accuracy. |
🧩 Analysis chain🏁 Script executed: Length of output: 5286 🏁 Script executed: Length of output: 3549 🏁 Script executed: Length of output: 321 🏁 Script executed: Length of output: 4110 🏁 Script executed: Length of output: 2761 🏁 Script executed: Length of output: 337 🏁 Script executed: Length of output: 1757 🏁 Script executed: Length of output: 1667 🏁 Script executed: Length of output: 2364 🏁 Script executed: Length of output: 329 🏁 Script executed: Length of output: 5139 🏁 Script executed: Length of output: 2541 🏁 Script executed: Length of output: 1403 🏁 Script executed: Length of output: 434 🏁 Script executed: Length of output: 1304 🏁 Script executed: Length of output: 1054 🏁 Script executed: Length of output: 727 🏁 Script executed: Length of output: 2244 🏁 Script executed: Length of output: 518 🏁 Script executed: Length of output: 1810 🏁 Script executed: Length of output: 1395 🏁 Script executed: Length of output: 2884 🏁 Script executed: Length of output: 892 🏁 Script executed: Length of output: 1814 @myieye I've thoroughly reviewed the AGENTS.md file by validating it against the actual repository structure and configuration files. Here are my findings: ✅ Correct InformationThe following sections are accurate:
❌ Inaccuracies Found1. Test Categories - INCORRECTCurrent text: and Issue: The document mentions categories as "(Unit, Integration, FlakyIntegration)" but actual categories found in the codebase are:
Note: There is NO Suggested correction: - Test Categories: Backend tests use `[Trait("Category", "...")]` attributes to organize test execution
- `Integration` - Tests requiring full service integration
- `FlakyIntegration` - Integration tests with known flakiness
- `RequiresDb` - Tests requiring database infrastructure (run with `task test:unit-with-db`)
- `Slow` - Long-running tests
- No category attribute = Unit tests (run with `task test:unit`)2. Vitest Configuration - MISLEADINGCurrent text: Issue: Suggested correction: - Frontend Testing:
- E2E Testing: Playwright for end-to-end browser tests
- Run: `pnpm run test` or `task ui:playwright-tests`
- Configuration: `playwright.config.ts`
- Test files: `frontend/tests/`
- FwLite Viewer Testing:
- Unit Testing: Vitest for component and utility testing
- Run: `pnpm run test:unit` (from viewer directory)
- Configuration: `frontend/viewer/vitest.config.ts`
- Test files: `frontend/viewer/tests/`3. Missing Important CommandsThe document mentions Suggested addition: - Run unit tests: `task test:unit` (excludes database and integration tests)
- Run unit tests with database: `task test:unit-with-db` (includes RequiresDb category)
- Run integration tests: `task test:integration`4. Frontend Code Formatting - Minor ImprecisionCurrent text: Issue: While the code does use 2-space indentation, this is not explicitly configured in {
"singleQuote": true,
"printWidth": 120
}Suggested correction: - Frontend: Use ESLint and Prettier (single quotes, 120 char line width, default 2-space indentation for TypeScript/Svelte)📝 Recommendations
Overall, the document is well-structured and mostly accurate. The main issues are around test categorization and Vitest configuration location. Would you like me to help generate a corrected version? ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
AGENTS.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-31T15:58:56.761Z
Learnt from: imnasnainaec
Repo: sillsdev/languageforge-lexbox PR: 1867
File: platform.bible-extension/.eslintrc.js:29-29
Timestamp: 2025-07-31T15:58:56.761Z
Learning: In the sillsdev/languageforge-lexbox repository's platform.bible-extension, duplicate ESLint rules in .eslintrc.js files are intentional when part of the template system. The configuration uses a layered approach where rules are first defined in shared regions and then potentially overridden in later sections to allow for template inheritance with customization.
Applied to files:
AGENTS.md
| - Frontend: Use ESLint and Prettier (2-space indentation, single quotes, 120 char line width) | ||
| - Run `pnpm run lint` and `pnpm run format` for frontend code quality |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjust frontend indentation documentation to match actual Prettier configuration.
The documentation claims 2-space indentation is explicitly configured, but the PR's automated review noted that 2-space indentation is actually the Prettier default for TypeScript/Svelte files, not an explicit .prettierrc setting. The .prettierrc only explicitly sets singleQuote and printWidth (120 char).
Apply this diff to correct the indentation claim:
- **Code Formatting**:
- Backend: Follow .NET conventions defined in `.editorconfig` (4-space indentation for C#)
- - Frontend: Use ESLint and Prettier (2-space indentation, single quotes, 120 char line width)
+ - Frontend: Use ESLint and Prettier (single quotes, 120 char line width, default 2-space indentation for TypeScript/Svelte)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In AGENTS.md around lines 13 to 14, the docs incorrectly state that 2-space
indentation is explicitly configured for the frontend; update the text to
reflect that the Prettier config only sets singleQuote and printWidth (120) and
that 2-space indentation is Prettier’s default for TypeScript/Svelte rather than
an explicit .prettierrc setting, and then adjust the sentence to remove the
claim of an explicit indentation setting while keeping the rest of the
lint/format instructions intact.
| - Framework: xUnit with categorized tests (Unit, Integration, FlakyIntegration) | ||
| - Run unit tests: `task test:unit` (excludes database and integration tests) | ||
| - Run integration tests: `task test:integration` | ||
| - Test organization: `backend/Testing/` with subdirectories by service/component | ||
| - Database tests require running infrastructure via `task test:unit-with-db` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test categories list is incomplete and inaccurate.
The PR's automated review identified that the actual test categories in the repository differ from what's documented here. The repository uses Integration, FlakyIntegration, RequiresDb, and Slow category attributes, plus unit tests (no category attribute, run by default). The documentation currently omits RequiresDb and Slow, which are used in the codebase.
Apply this diff to correct the test categories:
- **Backend Testing**:
- Framework: xUnit with categorized tests (Unit, Integration, FlakyIntegration)
+ **Backend Testing**:
+ - Framework: xUnit with categorized tests
+ - No category attribute: Unit tests (run by default with `task test:unit`)
+ - `Integration`: Integration tests (run with `task test:integration`)
+ - `FlakyIntegration`: Flaky integration tests
+ - `RequiresDb`: Tests requiring database (run with `task test:unit-with-db`)
+ - `Slow`: Slow-running testsCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In AGENTS.md around lines 66 to 70, the documented test categories are
incomplete: update the list to match the repository by replacing the current
categories with "Integration", "FlakyIntegration", "RequiresDb", and "Slow", and
clarify that unit tests have no category attribute and run by default; also
ensure the commands note that unit tests exclude database/integration tests and
that database tests require running infrastructure via `task test:unit-with-db`.
| - Database tests require running infrastructure via `task test:unit-with-db` | ||
| - **Frontend Testing**: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify the distinction between unit, unit-with-db, and integration test commands.
The current text mentions task test:unit-with-db but doesn't clearly explain why or when to use each test command. Developers need to understand that test:unit-with-db is specifically for running tests marked with the RequiresDb trait.
Apply this diff to clarify the test command usage:
- **Backend Testing**:
- Framework: xUnit with categorized tests (Unit, Integration, FlakyIntegration)
- - Run unit tests: `task test:unit` (excludes database and integration tests)
+ - Run unit tests: `task test:unit` (excludes database, integration, and slow tests)
+ - Run database-dependent tests: `task test:unit-with-db` (runs tests marked with `RequiresDb`)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In AGENTS.md around lines 70-71, the docs mention `task test:unit-with-db` but
don't explain the difference between test commands; update the text to clarify
that `task test:unit` runs pure unit tests, `task test:unit-with-db` runs unit
tests annotated with the RequiresDb trait against local/test DB infrastructure,
and `task test:integration` runs broader integration tests spanning services;
briefly state when to use each (use unit for fast isolated logic, unit-with-db
when a DB mock is insufficient and tests require real DB access, integration for
end-to-end scenarios) and include the `task test:unit-with-db` invocation note
about running infrastructure via `task test:unit-with-db`.
| - Unit Testing: Vitest for component and utility testing | ||
| - Run: `pnpm run test:unit` | ||
| - Test files located in `frontend/tests/` and `frontend/viewer/tests/` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarify Vitest scope and location in the frontend architecture.
The current documentation implies that Vitest is used for general frontend unit testing, but the automated review found that Vitest exists only in frontend/viewer/ for that specific viewer application. The main frontend uses Playwright for E2E testing. This distinction is critical for developers to understand which testing tool applies to different parts of the codebase.
Apply this diff to clarify the Vitest and testing split:
- **Frontend Testing**:
- E2E Testing: Playwright for end-to-end browser tests
- Run: `pnpm run test` or `task ui:playwright-tests`
- Configuration: `playwright.config.ts`
- - Unit Testing: Vitest for component and utility testing
+ - Unit Testing: Vitest for FwLite viewer component and utility testing
- Run: `pnpm run test:unit`
+ - Note: Vitest is configured in `frontend/viewer/` for the separate viewer application
+ - Main frontend components use Playwright for E2E testing; no dedicated unit test framework for general componentsCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In AGENTS.md around lines 75 to 77, the documentation incorrectly implies Vitest
is used across the frontend; update the text to explicitly state that Vitest is
only used for unit testing in frontend/viewer/ (with test files in
frontend/viewer/tests/ and run via pnpm run test:unit from that package), and
note that the main frontend uses Playwright for end-to-end testing (with its own
test commands and locations), so readers understand the split between Vitest
(viewer unit tests) and Playwright (main frontend E2E).
| <pull_request_formatting> | ||
| </pull_request_formatting> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove or fill the empty <pull_request_formatting> section.
The section is empty and provides no guidance to developers. Either add content describing pull request formatting conventions or remove the section entirely.
Consider either:
- Adding PR formatting guidelines (commit message conventions, PR title/description format, change documentation, etc.), or
- Removing the empty section entirely.
-<pull_request_formatting>
-</pull_request_formatting>or fill with appropriate content about PR conventions.
📝 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.
| <pull_request_formatting> | |
| </pull_request_formatting> |
🤖 Prompt for AI Agents
In AGENTS.md around lines 85-86 the <pull_request_formatting> section is empty;
either remove that empty section tag or replace it with a concise PR formatting
guideline. If removing, delete the tag and any surrounding placeholder text; if
filling, add a short checklist describing expected PR title format, description
template, commit message style, required changelog/labels, reviewers, and
testing/CI requirements. Ensure whichever option you choose keeps the file
consistent with surrounding Markdown structure and references.
WORK IN PROGRESS OPEN SWE PR
Fixes: #1897