Skip to content

[No QA] Add CONSISTENCY-19 - prop comment must add information rule - #99906

Merged
roryabraham merged 12 commits into
Expensify:mainfrom
software-mansion-labs:korytko/consistency-19-prop-comment-add-info
Sep 7, 2026
Merged

roryabraham merged 12 commits into
Expensify:mainfrom
software-mansion-labs:korytko/consistency-19-prop-comment-add-info

Conversation

@JakubKorytko

Copy link
Copy Markdown
Member

Explanation of Change

Adds reviewer rule CONSISTENCY-19 "Prop comment must add information", flagging prop comments that only restate the prop's name/type. Amends CONSISTENCY-10 and CONSISTENCY-13's examples (which taught the now-flagged pattern) and the "Code Documentation" section of STYLE.md to require prop comments only where they add a unit, default, constraint, or sibling distinction. Also drops TSPropertySignature from jsdoc/require-jsdoc's contexts for src/types/onyx/**, moving per-property documentation enforcement there from ESLint to CONSISTENCY-13/19, since ESLint can't tell whether a property has anything non-obvious to say.

Out of scope for this PR, left as deliberate follow-ups:

  • Rewording/deleting the ~1697 (or more) existing name-echoing prop comments across the repo. This PR only stops the pattern from growing; existing comments get fixed opportunistically when their prop is touched.
  • src/types/onyx/BillingReceiptDetails.ts:1 and src/types/onyx/OriginalMessage.ts:1739 carry file-wide /* eslint-disable jsdoc/require-jsdoc */ that predates this change. With TSPropertySignature dropped, those disables may now be partially redundant (only the type-level requirement remains), but narrowing/removing them risks surfacing other lint issues those blanket disables were suppressing, so that's left for a separate PR.

Fixed Issues

$ #99453
PROPOSAL: Slack

Tests

N/A

Offline tests

N/A

QA Steps

N/A

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari

@JakubKorytko

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: add6daf045

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread config/eslint/eslint.config.mjs Resolved
@JakubKorytko

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: add6daf045

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@JakubKorytko

Copy link
Copy Markdown
Member Author

@MelvinBot review

@MelvinBot

Copy link
Copy Markdown
Contributor

Review

The direction is right and the ESLint change is safe. One real gap in the rule wording, plus two nits.

1. A name-echoing // comment is caught by neither rule

CONSISTENCY-10's //-comment bullet only fires when "the comment adds a real fact", and CONSISTENCY-19 is scoped to "adds or modifies a /** ... */ comment". So // Whether the modal is visible on a prop matches neither — CONSISTENCY-10 defers to 19, and 19 doesn't accept //.

That's exactly the case the new STYLE.md calls out as // Bad: same issue with // comments.

The trailing sentence on the bullet ("If the // comment adds nothing, the fix is deleting it instead") reads like it intends coverage, but it hangs off a condition that already excluded the case. Under a Flag ONLY when ANY of these is true header, a reviewer applying the list literally will skip it.

Suggested fix — either:

2. Index entry keeps the old CONSISTENCY-13 title

The rule file's title now reads "Document component props and Onyx type properties…", but SKILL.md:54 still says the old one. The index is what a reviewer scans first, so the widened src/types/onyx/** scope is invisible there.

3. Example mismatch (nit)

consistency-10-jsdoc.md:26 and consistency-13-document-props.md:18 put autoDismissDelay in a ButtonProps type and document it as "before the tooltip auto-dismisses". In a rule about comments matching what they describe, that's an unfortunate example. Rename the type to TooltipProps.


What I verified

ESLint change is safe. Dropping TSPropertySignature can only remove errors, and I confirmed it introduces no unused-disable warnings:

  • Only two jsdoc/require-jsdoc disables exist in src/ — BillingReceiptDetails.ts:1 and OriginalMessage.ts:1739, both file-wide. Both are still required by the remaining TSTypeAliasDeclaration context: BillingReceiptDetails.ts declares BillingItem and BillingReceiptDetails with no JSDoc, and OriginalMessage.ts declares OriginalMessage<T> at line 1860 with no JSDoc. So the PR body's caution about leaving them alone holds, and there's no reportUnusedDisableDirectives fallout either way.
  • Ran npx eslint src/types/onyx/BillingReceiptDetails.ts src/types/onyx/OriginalMessage.ts src/types/onyx/Report.ts on the PR head — 0 errors, 1 pre-existing seatbelt-allowed warning in Report.ts unrelated to this change.
  • No other reference to TSPropertySignature in config or docs (the hits in .github/actions/javascript/authorChecklist/index.js are bundled Babel, not this rule).

Coverage tradeoff, noted not blocking. ESLint checked every property in src/types/onyx/** on every run; the AI rules only see changed code. A new property added to an already-documented Onyx type won't trigger CONSISTENCY-13 (it explicitly skips types where at least one member is documented), so it now rests entirely on CONSISTENCY-10's sibling bullet. That's the intended trade — worth being aware it's a judgment call now, not a deterministic one.

No app testing run. This PR touches only reviewer rule markdown, ESLint config, and STYLE.md — no runtime code, and the PR's Tests/QA sections are N/A. There is nothing to exercise in the app.


view run

@JakubKorytko

Copy link
Copy Markdown
Member Author

@MelvinBot review again please, addressed your concerns

@MelvinBot

Copy link
Copy Markdown
Contributor

Re-review — all three addressed 👍

f62341c fixes everything I raised. LGTM.

1. The // comment gap is closed. consistency-10-jsdoc.md:56 now flags any // comment on a prop unconditionally and branches only the fix on content. That's the cleaner of the two options I suggested, since it keeps CONSISTENCY-19 scoped purely to /** */ content.

The four rules now partition the space with no gap and no double-flag:

Situation Owner
// comment on a prop / Onyx property, any content CONSISTENCY-10 → convert or delete
/** */ comment that only restates the name CONSISTENCY-19 → delete
No comment, no member documented, one has a fact CONSISTENCY-13
No comment, siblings documented, has a fact CONSISTENCY-10

2. SKILL.md:54 now carries the widened title.

3. ButtonProps → TooltipProps in both example files, so the type and the comment agree.

One optional nit (non-blocking)

consistency-10-jsdoc.md:70 — the Search Patterns line still reads `//` comments directly above members of a `...Props` type, but the bullet it hints at now also covers src/types/onyx/**. Worth extending so the hint matches the rule's scope. Pre-existing wording, fine to leave.

What I checked
  • f62341c touches only the three rule/index markdown files. config/eslint/eslint.config.mjs and contributingGuides/STYLE.md are byte-identical to add6daf, so my earlier verification still stands: ESLint runs clean on src/types/onyx/**, and both file-wide jsdoc/require-jsdoc disables (BillingReceiptDetails.ts:1, OriginalMessage.ts:1739) are still required by the remaining TSTypeAliasDeclaration context — no unused-directive fallout.
  • Re-read all four rules end to end for gaps and overlaps after the rewrite; the partition in the table above holds.
  • CONSISTENCY-19 stays scoped to /** ... */ at line 48, which is correct now that CONSISTENCY-10 owns // outright.
  • No app testing: this PR still changes only reviewer rule markdown, ESLint config, and STYLE.md. Nothing runtime to exercise, and Tests/QA are N/A.

view run

@JakubKorytko
JakubKorytko marked this pull request as ready for review August 31, 2026 14:00
@JakubKorytko
JakubKorytko requested a review from a team as a code owner August 31, 2026 14:00
@melvin-bot
melvin-bot Bot requested review from brunovjk and removed request for a team August 31, 2026 14:00
@melvin-bot

melvin-bot Bot commented Aug 31, 2026

Copy link
Copy Markdown

@brunovjk Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ee5740dc85

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@JakubKorytko

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 38fcf40262

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread config/eslint/eslint.config.mjs Resolved
@brunovjk

brunovjk commented Aug 31, 2026 •

Copy link
Copy Markdown
Contributor

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
  • I verified tests pass on all platforms & I tested again on:
    • Android: HybridApp
    • Android: mWeb Chrome
    • iOS: HybridApp
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp

N/A

Android: mWeb Chrome

N/A

iOS: HybridApp

N/A

iOS: mWeb Safari

N/A

MacOS: Chrome / Safari

N/A

@JakubKorytko

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 689303413d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@JakubKorytko

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c94c4670e6

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@JakubKorytko

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c94c4670e6

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@JakubKorytko

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 👍

Reviewed commit: 4cc1fad10e

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@JakubKorytko

Copy link
Copy Markdown
Member Author

@MelvinBot review

@MelvinBot

Copy link
Copy Markdown
Contributor

Re-review of the six new commits

The per-nesting-level and diff-scoping work is solid, and the four-rule partition I mapped last time still holds for the cases it covered. But three of the newer commits opened seams. One is a concrete double-flag that this PR widens; the other two are in CONSISTENCY-19.

1. CONSISTENCY-13 double-flags the // case it claims to cede to CONSISTENCY-10

consistency-13-document-props.md:10 says CONSISTENCY-10 "catches the mixed cases (a // comment on a member…)", and line 48 says the goal is to "avoid double-flagging" — but no DO-NOT-flag bullet actually implements that. Walk CONSISTENCY-10's own Incorrect example (consistency-10-jsdoc.md:24) through CONSISTENCY-13:

type TooltipProps = {
    // Milliseconds before the tooltip auto-dismisses, defaults to 3000
    autoDismissDelay?: number;
};
  • L42 ✅ a member of a ...Props type was added
  • L43 ✅ true — none of the members has a /** ... */ block comment; a // isn't one
  • L44 ✅ autoDismissDelay is undocumented, PR-touched, and has a unit + default
  • L48 exception ❌ requires a member "already documented with /** */" — a // doesn't qualify

So CONSISTENCY-13 fires alongside CONSISTENCY-10. And code-inline-reviewer.md requires "Each violation must reference exactly one Rule ID" with no dedup step, and tells the agent to "err on the side of including it" — so this lands as two separate inline comments on the same line, prescribing the same fix.

This existed before in narrow form, but commit c94c467 widened L42 from new types to any pre-existing type, so it now triggers on any PR adding a //-commented prop to an existing uncommented ...Props type.

Fix: add one DO-NOT-flag bullet to CONSISTENCY-13 — "The member carries a // comment — CONSISTENCY-10 owns that case."

2. CONSISTENCY-19's Review Metadata prescribes delete-only, which ping-pongs with CONSISTENCY-13

CONSISTENCY-19's Correct example (line 33) correctly shows both remedies — iconWidth gets the unit added, title/isVisible get deleted. But the Reasoning (line 10) says only "delete the comment instead of padding it," and the Review Metadata gives no branch at all — which is the part the reviewer agent acts on.

Apply the stated remedy to /** Icon width */ iconWidth: number in an otherwise-uncommented type and you land straight in CONSISTENCY-13: L43 ✅ nothing documented, L44 ✅ iconWidth has a unit fact — which CONSISTENCY-19's own Correct example asserts is worth saying. Rule 19 says delete, rule 13 says add it back.

CONSISTENCY-10 already solved this exact problem at line 56 with an explicit branch. Fix: port that sentence into CONSISTENCY-19's Review Metadata — convert if there's a real fact to state, delete if there isn't.

3. The new rename/retype trigger doesn't reach the case it looks aimed at

Line 48 now fires on "changes that property's own name or type while leaving its existing comment untouched" — but the other two ALL-conditions gate it hard:

type IconProps = {
    /** Width in px */   // untouched
-   iconWidth: number;
+   iconHeight: number;
};

Line 50 requires no unit be present, and line 54 excludes any comment naming a unit — so the comment that is now actively wrong is exempt, while a harmlessly redundant one is flagged. CONSISTENCY-10 and 13 don't reach it either (it's neither // nor undocumented), so it falls through all three.

There's a structural reason: line 49 requires the comment to restate the current name. After a rename the comment describes the old name, so by construction it usually can't. The trigger therefore only bites on meaning-preserving renames of already-worthless comments (isVisible → isModalVisible with /** Whether the modal is visible */) — the least valuable case. The retype half works fine.

Fix (pick one): either add "and the comment is still accurate for the property's current name and type" to the line 54 exemption so staleness is genuinely in scope, or drop the rename half of the trigger and keep this rule purely about content — staleness is a different defect than "adds no information," and half-covering it is worse than scoping it out.

Nits

  • Two dead trigger clauses. Line 42 lists "adds, modifies, or removes" a member's /** */ comment as triggers, but line 43 requires that no member has one after the change. If the PR added or modified a /** */, that member has one → line 43 is false. Only removes can ever reach a flag; drop the other two (they also point CONSISTENCY-13 at CONSISTENCY-19's territory).
  • Qualifier asymmetry. 4cc1fad added "at that same nesting level" to line 48 but not to line 43. Line 38's redefinition of "type" makes it technically correct as-is, but qualifying one and not the other invites exactly the misread the commit set out to fix.
  • CONSISTENCY-10 has no nesting statement. Line 38's per-level paragraph lives only in CONSISTENCY-13, yet 13 explicitly hands the documented-sibling case to CONSISTENCY-10's line 57. Whether a member of an anonymous inline literal counts as "a component prop in a Props type" is undefined there — read narrowly, a new member of a nested literal whose siblings ARE documented falls through both. Worth a one-line cross-reference, especially since src/types/onyx/Policy.ts is built largely from nested literals and this PR removed the ESLint backstop.
  • No examples for the new cases. CONSISTENCY-13's Incorrect/Correct still show only a brand-new top-level type — nothing nested, no added-member diff, no deleted-comment diff, and no negative example ("new Props type where every member is self-explanatory → don't flag"), which is the central behavioral change. CONSISTENCY-19's rename trigger has no example either. Examples do most of the disambiguation work for an AI reviewer.
What I checked
  • Diffed f62341c → 4cc1fad (6 commits). Only three files changed: consistency-10-jsdoc.md (+1/-1), consistency-13-document-props.md (+9/-6), consistency-19-prop-comment-add-info.md (+3/-2). config/eslint/eslint.config.mjs and contributingGuides/STYLE.md are byte-identical to add6daf, so my original verification stands: ESLint runs clean on src/types/onyx/**, and both file-wide jsdoc/require-jsdoc disables (BillingReceiptDetails.ts:1, OriginalMessage.ts:1739) are still required by the remaining TSTypeAliasDeclaration context — no unused-directive fallout.
  • Re-read all three rules end to end at 4cc1fad and walked each flag condition literally against constructed examples, rather than reading for intent.
  • Verified the // double-flag using CONSISTENCY-10's own Incorrect example as the input, and confirmed against code-inline-reviewer.md that violations are emitted one-per-ruleId with no dedup pass and an explicit "err on the side of including it" instruction.
  • My nit from last review is resolved: ee5740d extended the CONSISTENCY-10 search-pattern hint to src/types/onyx/**. SKILL.md:54 still carries the widened CONSISTENCY-13 title.
  • Findings 1–3 were independently re-derived by a second pass; a claimed hard contradiction between lines 38 and 43 did not survive scrutiny (line 38 redefines "type"), so it's listed as a nit rather than a bug.
  • No app testing. This PR still touches only reviewer-rule markdown, ESLint config, and STYLE.md — no runtime code, and Tests/QA are N/A. There is nothing to exercise in the app.

view run

@JakubKorytko

Copy link
Copy Markdown
Member Author

@MelvinBot points addressed, please re-review

@MelvinBot

Copy link
Copy Markdown
Contributor

Re-review — all six addressed 👍

301b4cb closes everything I raised. LGTM.

What I raised Fix in 301b4cb
CONSISTENCY-13 double-flagged the // case it cedes to CONSISTENCY-10 New DO-NOT-flag bullet at consistency-13:50 — "The member carries a // comment - CONSISTENCY-10 owns that case". The // mention was also dropped from the sibling bullet, so the two no longer overlap
CONSISTENCY-19's Metadata prescribed delete-only → ping-pong with 13 Convert-vs-delete branch added at consistency-19:52, mirroring CONSISTENCY-10's
Rename trigger didn't reach the case it looked aimed at Removed outright — trigger, exemption, and search pattern all reverted. You took the scope-it-out option, which I think is the right call: staleness is a different defect than "adds no information"
Two unsatisfiable trigger clauses consistency-13:44 now reads "adds or modifies one or more members' declarations, or removes…comments" — every clause can now reach a flag
Nesting qualifier missing on line 43 Line 45 now says "at that same nesting level", matching L40/L51
CONSISTENCY-10 had no nesting statement consistency-10:57 now defines "Sibling" as same-nesting-level, including inside anonymous object literals
No examples for the new cases Three blocks added — member-added-to-existing-type, deleted-comment diff, and the negative ButtonProps case

The negative example is the one I'd have pushed hardest for. It's the central behavioral change of the whole PR and now it's shown explicitly, matching STYLE.md's undocumented isIgnored: boolean.

One optional leftover (non-blocking)

The Search Patterns at consistency-13:57-59 didn't get the nesting qualifier you just added to lines 45 and 51 — line 58 still says "where that type has no /** */-documented member at all". They're grep hints that the Flag conditions filter afterward, and they're only used for files over 5000 lines, so the impact is small. Fine to leave.

What I checked
  • Walked the // case through CONSISTENCY-13 again with the new bullet: a member carrying a // comment is now excluded, CONSISTENCY-10 line 56 handles it with its convert-or-delete branch, and no gap opens — if the // had a fact it converts, if not it's deleted and the member genuinely needs no comment.
  • Confirmed the new trigger wording is satisfiable: "adds/modifies a member's declaration" and "removes a comment" are both compatible with line 45's "none documented after the change", which the old "adds/modifies a comment" clauses were not.
  • Grepped CONSISTENCY-19 for leftover rename/stale references — none. The removal is clean across the trigger, the DO-NOT-flag list, and the search patterns, and the Reasoning never mentioned renames.
  • config/eslint/eslint.config.mjs and contributingGuides/STYLE.md are still byte-identical to add6daf (only the four markdown files have changed since), so my first-round verification stands: ESLint runs clean on src/types/onyx/**, and both file-wide jsdoc/require-jsdoc disables (BillingReceiptDetails.ts:1, OriginalMessage.ts:1739) remain required by the surviving TSTypeAliasDeclaration context — no unused-directive fallout.
  • No app testing. Still only reviewer-rule markdown, ESLint config, and STYLE.md — no runtime code, Tests/QA are N/A, nothing to exercise in the app.

view run

@JakubKorytko

Copy link
Copy Markdown
Member Author

@brunovjk all yours now

@brunovjk

brunovjk commented Sep 1, 2026 •

Copy link
Copy Markdown
Contributor

@JakubKorytko thanks for the work on this PR! One question before approving: with TSPropertySignature dropped from ESLint for src/types/onyx/**, enforcement of CONSISTENCY-13/19 there now depends entirely on the AI reviewer running correctly. Does it already run on synchronize (new commits pushed to the PR), or is that still a gap? Not a blocker if it's already covered, just want to confirm before approving. Thank you.

@JakubKorytko

Copy link
Copy Markdown
Member Author

Does it already run on synchronize (new commits pushed to the PR), or is that still a gap?

that is still a thing for the all consistency- rules, did not want to change it since it would impact the whole AI reviewer. IMO it is out of scope, so posted as a separate proposal in #expert-contributors but I don't think we should block this PR on that - if the proposal is approved, all is good. If it is rejected then well, nothing more to do here either 😄

@brunovjk brunovjk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying! Since the gap is already known and being discussed separately, I’m fine treating it as out of scope for this PR. Approving 👍

@melvin-bot
melvin-bot Bot requested a review from roryabraham September 2, 2026 13:59
@roryabraham
roryabraham merged commit d9bc1b3 into Expensify:main Sep 7, 2026
19 of 20 checks passed
@OSBotify

OSBotify commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here.

@OSBotify

OSBotify commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.74-0 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

@OSBotify

OSBotify commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

🚀 Deployed to production by https://github.com/mountiny in version: 9.4.74-2 🚀

platform result
🕸 web 🕸 success ✅
🤖 android 🤖 success ✅
🍎 iOS 🍎 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants