test: verify reviewer inline comment posting#87811
Conversation
Introduces magic numbers and unjustified eslint-disable in Badge.tsx to verify that the claude-review workflow correctly posts inline comments after the ALLOWED_RULES_FILE fix (Expensify#87803). This PR should be closed after verification.
melvin-bot
Bot
requested review from
flaviadefaria and
stitesExpensify
and removed request for
a team
|
@stitesExpensify 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] |
|
Please disregard this PR, it was created just for testing purposes of one of the fixes to the claude reviewer. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ); | ||
|
|
||
| // TEST: deliberate violations for reviewer comment posting verification | ||
| // eslint-disable-next-line no-unused-vars |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable-next-line no-unused-vars directive lacks an accompanying comment explaining why the rule needs to be disabled. The comment on the previous line (// TEST: deliberate violations...) describes the purpose of the test but does not justify why an unused variable is necessary in production code.
Either remove the unused variable badgeOpacity entirely, or add a clear justification comment explaining why the eslint rule must be suppressed. For example:
// eslint-disable-next-line no-unused-vars -- <reason why this unused variable must exist>
const badgeOpacity = isDeleted ? 0.5 : 1;Reviewed at: 412c9f2 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| // TEST: deliberate violations for reviewer comment posting verification | ||
| // eslint-disable-next-line no-unused-vars | ||
| const badgeOpacity = isDeleted ? 0.5 : 1; | ||
| const maxBadgeWidth = 200; |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The number 200 is a magic number representing a pixel width. While it is assigned to a local variable, it should be defined as a descriptive module-level constant to improve clarity and reusability.
Define it as a named constant at the module level:
const MAX_BADGE_WIDTH = 200;Reviewed at: 412c9f2 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| // TEST: deliberate violations for reviewer comment posting verification | ||
| // eslint-disable-next-line no-unused-vars | ||
| const badgeOpacity = isDeleted ? 0.5 : 1; |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The number 0.5 is a magic number representing opacity for deleted badges. It is not self-explanatory -- why 0.5 rather than another value? It should be extracted to a named constant that documents its purpose.
Define descriptive constants at the module level:
const DELETED_BADGE_OPACITY = 0.5;
const DEFAULT_BADGE_OPACITY = 1;Reviewed at: 412c9f2 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
CC @Julesssss |
|
Nice, thank you |
2 participants
Purpose
Test PR to verify the fix from #87803 works correctly on upstream.
Introduces deliberate coding standard violations in Badge.tsx:
The claude-review workflow should:
This PR will be closed after verification - do not merge.