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

@@ -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.
+"""

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 <PR_NUMBER>
+```
+
+Replace `<PR_NUMBER>` 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