chore: update coderabbit guidance + review guidelines markdown - #434
Conversation
|
@johanpel If the new generated rust instructions are nonsensical i'm happy to keep this scoped to UI only |
📝 WalkthroughWalkthroughAdds ChangesReview guidance
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8).coderabbit.yamlTraceback (most recent call last): Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/REVIEW.md (1)
40-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAllow atoms in dedicated state modules.
Keep the ban on genericutils, but update the rule to permit dedicated state modules like@quent/hooks/src/atoms/*andui/src/atoms/*; that matches the existing package structure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/REVIEW.md` around lines 40 - 42, Update the Jotai guidance in the documentation to allow atoms in dedicated state modules, including `@quent/hooks/src/atoms/`* and ui/src/atoms/*. Keep the restriction against defining atoms in generic utils, while preserving the existing separation of server data in Query and UI interaction state in atoms.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ui/REVIEW.md`:
- Around line 140-146: Update the generated-binding guidance to use the
repository contract: bindings are written to ts/src/generated/ and regenerated
with cargo test -p quent-schema --features ts --test export_bindings. In
ui/REVIEW.md lines 140-146 and 164-169, replace the simulator path and cargo
build references; apply the same ts/src/generated/ path correction in
.coderabbit.yaml lines 93-94.
---
Nitpick comments:
In `@ui/REVIEW.md`:
- Around line 40-42: Update the Jotai guidance in the documentation to allow
atoms in dedicated state modules, including `@quent/hooks/src/atoms/`* and
ui/src/atoms/*. Keep the restriction against defining atoms in generic utils,
while preserving the existing separation of server data in Query and UI
interaction state in atoms.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 249c4c4e-8745-431b-b0fc-68b6c717313d
📒 Files selected for processing (2)
.coderabbit.yamlui/REVIEW.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.coderabbit.yaml (2)
64-64: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winScope the SPDX requirement to source files.
Under
crates/**/Cargo.toml, “SPDX headers” is ambiguous and may lead reviewers to expect headers inCargo.toml. Move this requirement tocrates/**/*.rsor explicitly qualify it as applying to new crate source files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.coderabbit.yaml at line 64, Update the “New crates” requirement in the repository review configuration to scope SPDX headers to new crate source files under crates/**/*.rs, rather than leaving the requirement ambiguous for crates/**/Cargo.toml; preserve the existing edition 2024 and publish = false requirements.
99-99: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAllow
.test.tsas well as.test.tsx.The test matcher covers both extensions, but Line 99 mandates
*.test.tsxand Testing Library/MSW for every test. Use*.test.{ts,tsx}and scope those tools to rendering or network tests that need them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.coderabbit.yaml at line 99, Update the testing convention in the relevant configuration entry to allow both .test.ts and .test.tsx via *.test.{ts,tsx}. Scope Vitest, Testing Library, and MSW guidance to tests that render components or exercise network behavior rather than requiring them for every test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.coderabbit.yaml:
- Line 64: Update the “New crates” requirement in the repository review
configuration to scope SPDX headers to new crate source files under
crates/**/*.rs, rather than leaving the requirement ambiguous for
crates/**/Cargo.toml; preserve the existing edition 2024 and publish = false
requirements.
- Line 99: Update the testing convention in the relevant configuration entry to
allow both .test.ts and .test.tsx via *.test.{ts,tsx}. Scope Vitest, Testing
Library, and MSW guidance to tests that render components or exercise network
behavior rather than requiring them for every test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 73c1ac50-93a6-4ec1-aa8a-9ced6c901c98
📒 Files selected for processing (2)
.coderabbit.yamlui/REVIEW.md
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ui/REVIEW.md (2)
100-105: 🎯 Functional Correctness | 🟠 MajorLimit
queryOptionsrequirements to read endpoints.Both files require Query options for every route/endpoint, which is incorrect for mutations, downloads, and streams.
ui/REVIEW.md#L100-L105: distinguish read/query routes from mutation or one-shot operations..coderabbit.yaml#L117-L123: requirequeryOptionsonly for read endpoints and document the appropriate client primitive for other operations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/REVIEW.md` around lines 100 - 105, Update the route-review guidance to require queryOptions only for read/query endpoints, while distinguishing mutations, downloads, and streams and documenting the appropriate client primitive for those operations. Apply this clarification in ui/REVIEW.md at lines 100-105 and .coderabbit.yaml at lines 117-123; ensure both files use consistent guidance.
71-75: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClarify the package dependency direction in both guidance sources.
The arrows are ambiguous or reversed if interpreted as import/dependency edges; label them as layering or reverse them consistently.
ui/REVIEW.md#L71-L75: clarify the diagram’s arrow meaning or reverse it to consumer → dependency..coderabbit.yaml#L97-L99: apply the same corrected convention to the CodeRabbit rule.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/REVIEW.md` around lines 71 - 75, Clarify the dependency diagram convention in ui/REVIEW.md lines 71-75 by labeling the arrows as layering direction or reversing them to consistently show consumer-to-dependency direction. Apply the identical convention to the corresponding CodeRabbit rule in .coderabbit.yaml lines 97-99, keeping both guidance sources aligned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.coderabbit.yaml:
- Around line 65-69: Scope the analyzer-specific instructions currently attached
to the broad domains/**/server/**/*.rs matcher to analyzer server paths only, or
explicitly qualify them for analyzer capability handlers. Preserve the existing
UI route and enum/filter guidance where applicable, but ensure
AnalyzerError::Unsupported and HTTP 501 requirements do not apply to unrelated
domain servers.
---
Outside diff comments:
In `@ui/REVIEW.md`:
- Around line 100-105: Update the route-review guidance to require queryOptions
only for read/query endpoints, while distinguishing mutations, downloads, and
streams and documenting the appropriate client primitive for those operations.
Apply this clarification in ui/REVIEW.md at lines 100-105 and .coderabbit.yaml
at lines 117-123; ensure both files use consistent guidance.
- Around line 71-75: Clarify the dependency diagram convention in ui/REVIEW.md
lines 71-75 by labeling the arrows as layering direction or reversing them to
consistently show consumer-to-dependency direction. Apply the identical
convention to the corresponding CodeRabbit rule in .coderabbit.yaml lines 97-99,
keeping both guidance sources aligned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: f3abf53b-98af-4883-9b68-c5be639803d6
📒 Files selected for processing (2)
.coderabbit.yamlui/REVIEW.md
| - path: "domains/**/server/**/*.rs" | ||
| instructions: | | ||
| - UI HTTP routes (especially under `/api/engines`) must stay in sync with `@quent/client` fetchers. | ||
| - Reject unknown requested enum/filter values rather than silently accepting them. Use typed unsupported errors for unsupported capabilities. | ||
| - Unsupported analyzer capabilities should use typed `AnalyzerError::Unsupported` (or equivalent) and map to HTTP 501, not ad-hoc response variants. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Scope analyzer-specific errors to analyzer server paths.
domains/**/server/**/*.rs covers every domain server, but these rules require AnalyzerError::Unsupported and HTTP 501 semantics. Narrow the matcher to the analyzer server or qualify the rule for analyzer capability handlers only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.coderabbit.yaml around lines 65 - 69, Scope the analyzer-specific
instructions currently attached to the broad domains/**/server/**/*.rs matcher
to analyzer server paths only, or explicitly qualify them for analyzer
capability handlers. Preserve the existing UI route and enum/filter guidance
where applicable, but ensure AnalyzerError::Unsupported and HTTP 501
requirements do not apply to unrelated domain servers.
|
/merge |
2 participants
Description
This scrapes old pull request comments to compile guidelines for coderabbit reviews going forward, and details the UI sections with hand written guidelines as well.
Related Issues
Fixes #344
Testing
N/A
Screenshots
N/A