Add copilot instructions for GH code reviews

[RayBB]

Feature Request

Problem / Opportunity

We use Copilot a lot to give initial feedback on code and I personally think it works pretty great.

However, we're possibly missing an opportunity to have it more customized to suit OL and our styles with custom instructions.

First step is to determine what we actually want copilot to give better feedback on.
Will post about this in Slack

Proposal

Breakdown

Related files

Refer to this map of common Endpoints:
*

Requirements Checklist

Checklist of requirements that need to be satisfied in order for this issue to be closed:

• [ ]

Stakeholders

---

Instructions for Contributors

• Before creating a new branch or pushing up changes to a PR, please first run these commands to ensure your repository is up to date, as the pre-commit bot may add commits to your PRs upstream.

[RayBB]

@mekarpeles I wrote a tiny script to gather all the PR comments and from 2024 and asked Gemini 2.5 Pro to review for patterns. Here's what it came up with. I don't think we should use all of them but quite a few are good.

Script used to gather comments is here

General Principles for AI Feedback

1. Be Actionable and Specific: Don't just point out a problem; explain why it's a problem and offer a clear solution.

   • Guideline: Always provide a concrete code snippet as a fix using GitHub's suggestion block syntax.
   • Source Example (PR Use IdentifiersInput Vue component for work identifier UI #10032): user: cdrini provides a direct suggestion to fix a duplicate HTML ID.

2. Explain the "Why": Briefly explain the reasoning behind a suggestion (e.g., "for accessibility," "to improve performance," "for consistency with the codebase").

   • Guideline: Each major suggestion should be accompanied by a short rationale.
   • Source Example (PR Add Trending Field to Solr  #10057): cdrini explains that frequent commit=true calls in Solr can cause performance issues.

3. Promote Code Cleanup: Identify and suggest the removal of dead or commented-out code, unused variables, and unnecessary comments.

   • Guideline: Flag code that is commented out or appears to be unused. If a comment is just restating what the code does, suggest removing it.
   • Source Example (PR changed the innerHTML with textContent for security reasons #10105): jimchamp requests the removal of commented-out code lines.

---

Code Style & Readability

1. Enforce Naming Conventions: Variable, function, and class names should be descriptive, clear, and consistent with the project's style (e.g., snake_case for Python variables, UpperCamelCase for Vue components).

   • Guideline: Check for vague names (like data, p, item) or inconsistent capitalization. Suggest more descriptive alternatives.
   • Source Example (PR Add Trending Field to Solr  #10057): cdrini suggests renaming fetch_works to the more specific fetch_work_trending_scores.

2. Encourage Idiomatic Code: Suggest more idiomatic and concise ways to write code for the given language (e.g., using comprehensions in Python, modern JavaScript features).

   • Guideline: Identify verbose code blocks that can be simplified. For example, replace if len(items) > 0: with if items:.
   • Source Example (PR Improve author name resolution #9003): A multi-line loop to create a set is simplified into a set comprehension.

3. Don't Repeat Yourself (DRY): Identify repeated code blocks that could be abstracted into a single reusable function or component.

   • Guideline: If two or more code blocks are nearly identical, suggest creating a shared helper function.
   • Source Example (PR Use IdentifiersInput Vue component for work identifier UI #10032): cdrini suggests creating a shared helper method for get_identifiers since it's the same for both Works and Editions.

---

Front-End Development (HTML, CSS, JavaScript)

1. Separate Concerns: Enforce the separation of structure (HTML), presentation (CSS), and behavior (JS).

   • Guideline: Flag inline style attributes and onclick event handlers in HTML. Suggest moving them to dedicated .less/.css and .js files.
   • Source Example (PR Copy/Paste Option for adding Book Covers #10101): jimchamp and cdrini both comment on removing inline styles and onclick attributes.

2. Follow CSS Best Practices:

   • Guideline: Enforce a consistent naming convention like BEM (.block__element--modifier). Use variables (e.g., @primary-blue) instead of hardcoded hex values. Avoid !important unless absolutely necessary.
   • Source Example (PR Redesign/DRY search box on search pages #9146): cdrini suggests using BEM notation for new CSS.

3. Write Modern and Safe JavaScript:

   • Guideline: When constructing URLs, use URLSearchParams to handle query parameters correctly instead of manual string concatenation.
   • Source Example (PR Asynchronously load search facets sidebar #9397, Use correct URL for untriaged issues #9421): Multiple reviewers suggest using URLSearchParams for building query strings safely and readably.

---

Back-End Development (Python)

1. Preserve Type Safety: Identify patterns that discard type information, making the code harder to debug and maintain.

   • Guideline: When a function uses **kwargs generically, question if a more specific interface or data class could be used to preserve type hints and enable static analysis.
   • Source Example (PR Use proxy for external requests #10008): cdrini points out that proxied_request with **kwargs loses type info, making it prone to typos.

2. Promote Safe Defaults: Identify functions with parameters that have potentially dangerous or performance-impacting default values.

   • Guideline: Suggest changing the default to a safer option and requiring the caller to explicitly pass the "dangerous" value when needed.
   • Source Example (PR Add Trending Field to Solr  #10057): The commit=true parameter in a Solr function is flagged, with a suggestion to default it to False.

3. Fail Fast: Discourage defensively masking errors that should be surfaced.

   • Guideline: When code uses .get(key, default_value) for a key that should always be present, suggest using direct dict[key] access to let a KeyError be raised if the data is malformed.
   • Source Example (PR Aggregate want to read counts by author #9545): cdrini advises against using .get() with a default for a mandatory field, as it hides potential data errors.

---

Performance

1. Batch Operations: Detect loops that perform individual network or database requests.

   • Guideline: Suggest batching these operations into a single, more efficient call (e.g., using get_many instead of multiple get calls).
   • Source Example (PR feat: use author identifiers in import API #10110, Show editions in reading log #8821): cdrini identifies multiple places where get_many can replace a loop of get calls, improving database performance.

2. Analyze Caching Strategies: Evaluate the effectiveness of caching for different endpoints.

   • Guideline: Question the value of caching for highly dynamic or low-hit-rate endpoints. Recommend caching for static assets and high-traffic, computationally expensive pages.
   • Source Example (PR Cache language autocomplete inputs/endpoints #9112): cdrini argues against caching a language autocomplete endpoint because the hit rate would be extremely low.

---

Accessibility (a11y)

1. Ensure All Images Have Alternatives: All <img> tags must have a descriptive alt attribute.

   • Guideline: Flag any <img> tag that is missing an alt attribute or has an empty one, unless it is purely decorative. The alt text should describe the image's purpose, not its appearance.
   • Source Example (PR Copy/Paste Option for adding Book Covers #10101, Add loading and success indicators for backend checks #9593): accesslint[bot] and cdrini both point out missing or unhelpful alt text.

2. Make Interactive Elements Accessible: Buttons, links, and inputs must be usable by everyone.

   • Guideline: Ensure all interactive elements (<button>, <input>, <a>) have an accessible name (from their content, aria-label, or title). Warn against removing outline styles, as they are crucial for keyboard navigation.
   • Source Example (PR Make MergeUI edition covers link to enlarged covers #8845, Redesign/DRY search box on search pages #9146): Copilot and cdrini provide feedback on making interactive elements accessible.

---

Internationalization (i18n)

1. Wrap All User-Facing Strings: All text displayed to the user in the UI must be wrapped in a translation function.

   • Guideline: Scan HTML templates for raw text in tags or attributes like title and placeholder. Flag any string that is not wrapped in $_(...) or part of a $:_('...') block.
   • Source Example (PR Indicate start date for Trending Books data collection #9085, 8829/fix/redesign sort UI #9489): Multiple reviewers catch untranslated strings and suggest wrapping them.

2. Use Correct i18n Syntax: Ensure correct syntax for variable interpolation and HTML content.

   • Guideline: For strings with variables, verify the use of named placeholders (e.g., _('Hello, %(name)s')). For strings containing HTML, ensure the $:_('...') syntax is used to prevent double-escaping.
   • Source Example (PR i18n syntax additions in HTML and Python files #8981): rebecca-shoptaw corrects multiple instances of incorrect i18n syntax.

3. Identify Non-Translatable Strings: Some strings, like brand names or analytics keys, should not be translated.

   • Guideline: If a string matches a known pattern of non-translatable content, suggest marking it for exclusion from the i18n system.
   • Source Example (PR Update i18n exclude list and add remaining exceptions where needed #9508): The discussion around whether "WorldCat" should be translated highlights this need.

By following these guidelines, an AI reviewer could automate a significant portion of the feedback process, catching common issues and allowing human reviewers to focus on more complex architectural and logical problems.

[RayBB]

A quick dump of comments before I delete locally
pr-comments.zip