pr-review-considerations

# PR Review Considerations Use this skill when reviewing pull requests, preparing review guidance, or deciding what GitHub Copilot should pay attention to in this repository. > GitHub Copilot PR review on GitHub.com does **not** load repository skills directly. To influence GitHub review comments, keep the concise version of this guidance in `.github/instructions/pr-review.instructions.md`. GitHub's current repository instruction model also means that file can influence other Copilot interactions for matching files, not only PR review. ## Review stance These considerations **supplement** — not replace — standard code review practices. Continue checking for correctness, security, performance, and readability as you normally would. The guidance below adds iTwin.js-specific domain concerns on top of that baseline. - In addition to standard review checks, prioritize issues likely to cause runtime regressions, consumer confusion, integration churn, or repeated reviewer back-and-forth. - Skip pure style-only nits unless they hide a correctness problem or conflict with an established repo pattern. - Prefer comments that explain *why* the issue matters to consumers, downstream apps, or maintainers. ## What reviewers repeatedly look for ### 1. Consumer impact and compatibility - For `package.json`, peer dependency, or version bumps, ask whether each bump is necessary and whether the peer range can stay broader. - Watch for unrelated dependency churn getting folded into a feature PR. - If a change is declared safe because of fallback behavior, verify the fallback exists and ask for the exact helper or test when the proof is missing. - For parser, persistence, schema, or serialization changes, check old-data compatibility and roundtrip behavior. - When integration risk exists, ask whether the change was validated in downstream apps, learning snippets, or consumer patch testing. ### 2. Stronger invariants and less ambiguity - Push `undefined` and loading handling upward instead of spreading optional checks through the tree. - Remove redundant or duplicated state when a single source of truth is available. - Encode behavioral requirements in types when possible, especially prop combinations tied to modes like `editable`. - Prefer extracting shared helpers over duplicating similar conditionals, heuristics, or parsing logic. - Call out ad hoc string-matching heuristics or repeated condition fragments that should be centralized. ### 3. UI and UX expectations - UI workflow changes should include a GIF or screenshot in the PR description. - Check CTA placement, layout shift, copy clarity, localization wording, accessibility, and whether UX feedback is warranted. - Confirm that app-specific glue is not being accidentally presented as reusable package behavior. ### 4. Docs, examples, and change notes - README content, comments, screenshots, and example snippets should match what actually ships. - Prefer compiled or extracted snippets over handwritten README code that can drift. - For public or beta API changes, verify release tags, `rush extract-api`, and required `rush change` files. - Only ask for `docs/changehistory/NextVersion.md` and migration guidance when the change is breaking or is a significant new user-visible feature. - In changelog text, describe concrete user-visible behavior instead of labeling every bug fix as "breaking." ## High-signal review prompts - "If this is safe because of a fallback, can you point to the helper or test that proves it?" - "Can this invariant be expressed in the type instead of repeated runtime checks?" - "Does this peer dependency bump need to happen in this PR, or can the range stay broader?" - "Will this behavior survive save/load or serialize/deserialize roundtrips?" - "Please add a GIF or screenshot so reviewers can validate the UI workflow quickly."