From c89bc30d66fd078b22a4aee12081c0c1fb0dfdb1 Mon Sep 17 00:00:00 2001 From: Jacob Richman Date: Thu, 30 Oct 2025 17:04:52 -0700 Subject: [PATCH] Code review script to catch common package/cli regressions (#12316) Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --- .gemini/commands/review-frontend.toml | 140 ++++++++++++++++++++++++++ CONTRIBUTING.md | 15 +++ 2 files changed, 155 insertions(+) create mode 100644 .gemini/commands/review-frontend.toml diff --git a/.gemini/commands/review-frontend.toml b/.gemini/commands/review-frontend.toml new file mode 100644 index 0000000000..39d723bdc4 --- /dev/null +++ b/.gemini/commands/review-frontend.toml @@ -0,0 +1,140 @@ +description="Reviews a pull request based on issue number." +prompt = """ +Please provide a detailed pull request review on GitHub issue: {{args}}. + +Follow these steps: + +1. Use `gh pr view {{args}}` to pull the information of the PR. +2. Use `gh pr diff {{args}}` to view the diff of the PR. +3. Understand the intent of the PR using the PR description. +4. If PR description is not detailed enough to understand the intent of the PR, + make sure to note it in your review. +5. Make sure the PR title follows Conventional Commits, here are the last five + commits to the repo as examples: !{git log --pretty=format:"%s" -n 5} +6. Search the codebase if required. +7. Write a concise review of the PR, keeping in mind to encourage strong code + quality and best practices. Pay particular attention to the Gemini MD file + in the repo. +8. Consider ways the code may not be consistent with existing code in the repo. + In particular it is critical that the react code uses patterns consistent + with existing code in the repo. +9. Evaluate all tests on the PR and make sure that they are doing the following: + * Using `waitFor` from @{packages/cli/src/test-utils/async.ts} rather than + using `vi.waitFor` for all `waitFor` calls within `packages/cli`. Even if + tests pass, using the wrong `waitFor` could result in flaky tests as `act` + warnings could show up if timing is slightly different. + * Using `act` to wrap all blocks in tests that change component state. + * Using `toMatchSnapshot` to verify that rendering works as expected rather + than matching against the raw content of the output. + * If snapshots were changed as part of the pull request, review the snapshots + changes to ensure they are intentional and comment if any look at all + suspicious. Too many snapshot changes that indicate bugs have been approved + in the past. + * Use `render` or `renderWithProviders` from + @{packages/cli/src/test-utils/render.tsx} rather than using `render` from + `ink-testing-library` directly. This is needed to ensure that we do not get + warnings about spurious `act` calls. If test cases specify providers + directly, consider whether the existing `renderWithProviders` should be + modified to support that use case. + * Ensure the test cases are using parameterized tests where that might reduce + the number of duplicated lines significantly. + * NEVER use fixed waits (e.g. 'await delay(100)'). Always use 'waitFor' with + a predicate to ensure tests are stable and fast. + * Ensure mocks are properly managed: + * Critical dependencies (fs, os, child_process) should only be mocked at + the top of the file. Ideally avoid mocking these dependencies altogether. + * Check to see if there are existing mocks or fakes that can be used rather + than creating new ones for the new tests added. + * Try to avoid mocking the file system whenever possible. If using the real + file system is difficult consider whether the test should be an + integration test rather than a unit test. + * `vi.restoreAllMocks()` should be called in `afterEach` to prevent test + pollution. + * Use `vi.useFakeTimers()` for tests involving time-based logic to avoid + flakiness. + * Avoid using `any` in tests; prefer proper types or `unknown` with + narrowing. + * When creating parameterized tests, give the parameters types to ensure + that the tests are type-safe. +10. Evaluate all react logic carefully keeping in mind that the author of the PR + is not likely an expert on React. Key areas to audit carefully are: + * Whether `setState` calls trigger side effects from within the body of the + `setState` callback. If so, you *must* propose an alternate design using + reducers or other ways the code might be modified to not have to modify + state from within a `setState`. Make sure to comment about absolutely + every case like this as these cases have introduced multiple bugs in the + past. Typically these cases should be resolved using a reducer although + occassionally other techniques such as useRef are appropriate. Consider + suggesting that jacob314@ be tagged on the code review if the solution is + not 100% obvious. + * Whether code might introduce an infinite rendering loop in React. + * Whether keyboard handling is robust. Keyboard handling must go through + `useKeyPress.ts` from the Gemini CLI package rather than using the + standard ink library used by most keyboard handling. Unlike the standard + ink library, the keyboard handling library in Gemini CLI may report + multiple keyboard events one after another in the same React frame. This + is needed to support slow terminals but introduces complexity in all our + code that handles keyboard events. Handling this correctly often means + that reducers must be used or other mechanisms to ensure that multiple + state updates one after another are handled gracefully rather than + overriding values from the first update with the second update. Refer to + text-buffer.ts as a canonical example of using a reducer for this sort of + case. + * Ensure code does not use `console.log`, `console.warn`, or `console.error` + as these indicate debug logging that was accidentally left in the code. + * Avoid synchronous file I/O in React components as it will hang the UI. + * Ensure state initialization is explicit (e.g., use 'undefined' rather than + 'true' as a default if the state is truly unknown initially). + * Carefully manage 'useEffect' dependencies. Prefer to use a reducer + whenever practical to resolve the issues. If that is not practical it is + ok to use 'useRef' to access the latest value of a prop or state inside an + effect without adding it to the dependency array if re-running the effect + is undesirable (common in event listeners). + * NEVER disable 'react-hooks/exhaustive-deps'. Fix the code to correctly + declare dependencies. Disabling this lint rule will almost always lead to + hard to detect bugs. + * Avoid making types nullable unless strictly necessary, as it hurts + readability. + * Do not introduce excessive property drilling. There are multiple providers + that can be leveraged to avoid property drilling. Make sure one of them + cannot be used. Do suggest a provider that might make sense to be extended + to include the new property or propose a new provider to add if the + property drilling is excessive. Only use providers for properties that are + consistent for the entire application. +11. General Gemini CLI design principles: + * Make sure that settings are only used for options that a user might + consider changing. + * Do not add new command line arguments and suggest settings instead. + * New settings must be added to packages/cli/src/config/settingsSchema.ts. + * If a setting has 'showInDialog: true', it MUST be documented in + docs/get-started/configuration.md. + * Ensure 'requiresRestart' is correctly set for new settings. + * Use 'debugLogger' for rethrown errors to avoid duplicate logging. + * All new keyboard shortcuts MUST be documented in + docs/cli/keyboard-shortcuts.md. + * Ensure new keyboard shortcuts are defined in + packages/cli/src/config/keyBindings.ts. + * If new keyboard shortcuts are added, remind the user to test them in + VSCode, iTerm2, Ghostty, and Windows to ensure they work for all + users. + * Be careful of keybindings that require the meta key as only certain + meta key shortcuts are supported on Mac. + * Be skeptical of function keys and keyboard shortcuts that are commonly + bound in VSCode as they may conflict. +12. TypeScript Best Practices: + * Use 'checkExhaustive' in the 'default' clause of 'switch' statements to + ensure all cases are handled. + * Avoid using the non-null assertion operator ('!') unless absolutely + necessary and you are confident the value is not null. +13. If the change might at all impact the prompts sent to Gemini CLI, flagged + that the change could impact Gemini CLI quality and make sure anj-s has been + tagged on the code review. +14. Discuss with me before making any comments on the issue. I will clarify + which possible issues you identified are problems, which ones you need to + investigate further, and which ones I do not care about. +15. If I request you to add comments to the issue, use + `gh pr comment {{args}} --body {{review}}` to post the review to the PR. + +Remember to use the GitHub CLI (`gh`) with the Shell tool for all +GitHub-related tasks. +""" diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5e425c410d..ec7c46bd9f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -53,6 +53,21 @@ All submissions, including submissions by project members, require review. We use [GitHub pull requests](https://docs.github.com/articles/about-pull-requests) for this purpose. +If your pull request involves changes to `packages/cli` (the frontend), we +recommend running our automated frontend review tool. **Note: This tool is +currently experimental.** It helps detect common React anti-patterns, testing +issues, and other frontend-specific best practices that are easy to miss. + +To run the review tool, enter the following command from within Gemini CLI: + +```text +/review-frontend +``` + +Replace `` with your pull request number. Authors are encouraged to +run this on their own PRs for self-review, and reviewers should use it to +augment their manual review process. + ### Self assigning issues If you're looking for an issue to work on, check out our list of issues that are