Conversation
Summary of ChangesHello @PeterDaveHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughAGENTS.md was updated with expanded Prettier formatting specifics, stricter PR validation and "Validation skipped" requirements, tightened localization guidance (no editing existing keys), new cautions about deleting/renaming files or changing build configs, and a reworked runtime vs non-runtime validation schema. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the AGENTS.md documentation by adding clarity to code formatting, localization, and validation procedures. The changes are well-structured and make the contribution guidelines easier to follow. I have one suggestion to further improve clarity around the process for skipping validation to prevent potential misinterpretation.
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR improves the AGENTS.md documentation by adding more specific formatting guidelines, clarifying validation requirements, and providing better guidance for AI agents and developers working on the ChatGPTBox browser extension.
Changes:
- Added specific Prettier configuration details with concrete code examples showing correct vs incorrect formatting
- Clarified PR description requirements for when validation steps are skipped
- Enhanced localization guidelines to explicitly prohibit modifying existing i18n keys
- Added warnings about high-risk changes requiring approval before proceeding
- Restructured validation steps into a clearer tiered approach based on change impact
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
AGENTS.md (1)
247-248: Consider relocating the caution note to "Development Workflow".The note at line 247 is a behavioral/decision guideline for agents, but it lives at the bottom of the "Troubleshooting → Common Development Tasks" recipe list where it may be overlooked. The "Development Workflow → Code Style, Quality, and File Organization" section (lines 139–165) is a more natural and prominent home.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 247 - 248, The caution note currently under "Troubleshooting → Common Development Tasks" should be moved to the "Development Workflow → Code Style, Quality, and File Organization" section to better match its behavioral/decision guidance; update AGENTS.md by cutting the paragraph that begins "**Note:** Ask before deleting/renaming files..." from the Troubleshooting list and pasting it into the Code Style/Quality/File Organization subsection (lines referenced as "Development Workflow → Code Style, Quality, and File Organization") so it appears with other workflow and file-organization guidance and adjust surrounding spacing/ordering to keep section flow consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 172: Update the PR description template line that currently reads
"Validation skipped: <reason>; no runtime files touched." so it does not assert
"no runtime files touched." unconditionally; either restrict this suffix to the
docs-only validation-skip case or make the suffix a replaceable token (e.g.,
"Validation skipped: <reason>[; affected runtime files: <files>]" or "Validation
skipped: <reason> (docs-only)") so agents only assert no runtime changes when
that is true; locate the exact template string "Validation skipped: <reason>; no
runtime files touched." in AGENTS.md and modify it accordingly.
- Line 262: The step text "Behavior-adjacent localization changes
(`src/_locales/**` only): run `npm run build`; run manual browser smoke tests
when keys, placeholders, or UI labels are changed." contains a redundant
qualifier — any change under `src/_locales/**` inherently affects
keys/placeholders/labels — so update the AGENTS.md step to remove the
conditional phrase "when keys, placeholders, or UI labels are changed" and
simply instruct: for `src/_locales/**` changes run `npm run build` and run
manual browser smoke tests; keep the rest of the step intact.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 247-248: The caution note currently under "Troubleshooting →
Common Development Tasks" should be moved to the "Development Workflow → Code
Style, Quality, and File Organization" section to better match its
behavioral/decision guidance; update AGENTS.md by cutting the paragraph that
begins "**Note:** Ask before deleting/renaming files..." from the
Troubleshooting list and pasting it into the Code Style/Quality/File
Organization subsection (lines referenced as "Development Workflow → Code Style,
Quality, and File Organization") so it appears with other workflow and
file-organization guidance and adjust surrounding spacing/ordering to keep
section flow consistent.
PeterDaveHello
force-pushed
the
docs-agents-guidelines
branch
from
164f620 to
36fbf04
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - Commit subject: imperative, capitalize first word; separate subject/body with a blank line; wrap at ~72 characters; explain what and why. | ||
| - PRs: link related issues, summarize scope/behavior changes; include screenshots for UI changes. | ||
| - Note i18n updates in PR description when `src/_locales/` changes. | ||
| - If any validation step is skipped, include `Validation skipped: <reason>; <runtime impact or "no runtime files touched">` in the PR description. |
There was a problem hiding this comment.
There's an inconsistency in how the validation skip format is presented. Line 172 uses angle brackets as placeholders (<reason>, <runtime impact or "no runtime files touched">), while line 263 provides a concrete example without angle brackets (docs-only change; no runtime files touched.). While angle brackets typically indicate placeholders in documentation, it would be clearer to explicitly state this or use a consistent format. Consider adding text like "(replace with actual values)" after the template on line 172, or reformatting the template to match the concrete example style more clearly.
| - If any validation step is skipped, include `Validation skipped: <reason>; <runtime impact or "no runtime files touched">` in the PR description. | |
| - If any validation step is skipped, include `Validation skipped: <reason>; <runtime impact or "no runtime files touched">` in the PR description (replace the `<...>` placeholders with actual values). |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
AGENTS.md (2)
261-264: Clarify step ordering: step 1'ssrc/**glob subsumes step 2'ssrc/_locales/**.An agent scanning top-to-bottom will match
src/_locales/changes against step 1 (src/**) before reaching step 2. Theonlyqualifier in step 2 implies prioritisation, but the override relationship is implicit. Step 4 ("when in doubt, treat as runtime-impacting") reinforces the stricter path, potentially making step 2 a dead letter.Consider making the mutual-exclusion explicit:
📝 Proposed clarification
-1. Runtime-impacting changes (`src/**`, `src/manifest*.json`, `build.mjs`, `package*.json`, `safari/**`): run `npm run build`, verify expected build artifacts, and run manual browser smoke tests. -2. Behavior-adjacent localization changes (`src/_locales/**` only): run `npm run build` and manual browser smoke tests. +1. Runtime-impacting changes (any file under `src/**` except `src/_locales/**`, plus `src/manifest*.json`, `build.mjs`, `package*.json`, `safari/**`): run `npm run build`, verify expected build artifacts, and run manual browser smoke tests. +2. Behavior-adjacent localization changes (`src/_locales/**` only, no other `src/**` files modified): run `npm run build` and manual browser smoke tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 261 - 264, Clarify the precedence between the globs by stating that the more specific pattern wins: update the text around "step 1" and "step 2" to say that changes matching only src/_locales/** are treated as "Behavior-adjacent localization changes" even though they also match src/**, and require only the step 2 workflow; alternatively instruct agents to evaluate specific globs first (src/_locales/**) before the broader src/** check, and keep step 4 as the fallback when a file set spans both categories.
144-147: The.prettierrcsettings are correctly documented.The configuration matches all four rules stated on line 144: 100-char width (
printWidth), no semicolons (semi: false), single quotes (singleQuote: true), and trailing commas (trailingComma: "all"). The good/bad examples in lines 146–147 are accurate for quotes and semicolons, but consider adding a trailing-comma example (e.g.,{a, b,}vs{a, b}) to make the ruleset fully self-contained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 144 - 147, The documentation correctly lists `.prettierrc` rules but misses an explicit example for trailing commas; update the examples section to include a good vs bad pair demonstrating trailing commas (e.g., show an object or array example with a trailing comma as the ✅ good form and without as the ❌ bad form) so the examples fully cover `printWidth`, `semi: false`, `singleQuote: true`, and `trailingComma: "all"`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@AGENTS.md`:
- Around line 261-264: Clarify the precedence between the globs by stating that
the more specific pattern wins: update the text around "step 1" and "step 2" to
say that changes matching only src/_locales/** are treated as "Behavior-adjacent
localization changes" even though they also match src/**, and require only the
step 2 workflow; alternatively instruct agents to evaluate specific globs first
(src/_locales/**) before the broader src/** check, and keep step 4 as the
fallback when a file set spans both categories.
- Around line 144-147: The documentation correctly lists `.prettierrc` rules but
misses an explicit example for trailing commas; update the examples section to
include a good vs bad pair demonstrating trailing commas (e.g., show an object
or array example with a trailing comma as the ✅ good form and without as the ❌
bad form) so the examples fully cover `printWidth`, `semi: false`, `singleQuote:
true`, and `trailingComma: "all"`.
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 1. Runtime-impacting changes (`src/**`, `src/manifest*.json`, `build.mjs`, `package*.json`, `safari/**`): run `npm run build`, verify expected build artifacts, and run manual browser smoke tests. | ||
| 2. Behavior-adjacent localization changes (`src/_locales/**` only): run `npm run build` and manual browser smoke tests. | ||
| 3. Docs-only changes (`*.md`, `screenshots/**`): build/manual browser tests may be skipped, but the PR description must include `Validation skipped: docs-only change; no runtime files touched`. | ||
| 4. When in doubt, treat the change as runtime-impacting and execute the full validation flow. |
There was a problem hiding this comment.
The PR description mentions restructuring validation into three categories, but this section is formatted as four numbered items and item 4 is guidance rather than a validation step. Consider either updating the PR description to match, or reformatting item 4 as an unnumbered note so the numbered list reflects only the actual validation categories/steps.
1 participant
PR Type
Documentation
Description
Enhanced code formatting guidelines with concrete examples
Clarified localization key management and validation procedures
Added guidance on when to skip validation steps for docs-only changes
Refined critical validation steps with runtime-impact categorization
Diagram Walkthrough
File Walkthrough
AGENTS.md
Clarify formatting, validation, and localization guidelinesAGENTS.md
double quotes, semicolons)
modifying existing ones
runtime impact
required PR description notation
modifying build configuration
descriptions
Summary by CodeRabbit