Repository navigation
[No QA] Remove comments that do not add context - #100576
roryabraham merged 5 commits into
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
ReviewThe mechanical part of this PR is clean: I verified that every added and removed line in the diff is a comment or blank line — no code, no lint/compiler directives, no JSDoc tags. Three things to fix before merge, one of them a real defect. 1. Orphaned
|
|
@MelvinBot valid, addressed all 3, re-review please |
Re-review — all three resolved, LGTMRe-verified
Nice side effect: Re-verificationFull sweep re-run on
Still worth landing quickly once approved — 878 files will conflict with a lot of what's open against |
| /** | ||
| * The errors of all card feeds. | ||
| */ | ||
| all: CardFeedErrorState; | ||
|
|
||
| /** | ||
| * The errors of company cards. | ||
| */ | ||
| companyCards: CardFeedErrorState; | ||
|
|
||
| /** | ||
| * The errors of expensify card. | ||
| */ | ||
| expensifyCard: CardFeedErrorState; | ||
|
|
||
| /** | ||
| * The errors of personal card. | ||
| */ | ||
| personalCard: CardFeedErrorState; |
There was a problem hiding this comment.
all: CardFeedErrorState lost the only text distinguishing it from its three identically-typed siblings companyCards/expensifyCard/personalCard; the bare name doesn't say what it aggregates.
There was a problem hiding this comment.
"The errors of all card feeds" restates all and the type name CardFeedErrorState. Which subset each sibling aggregates is in its name (companyCards, expensifyCard, personalCard), same as all, so I disagree with this one
| /** | ||
| * Whether to show the RBR for each workspace account ID. | ||
| */ | ||
| shouldShowRbrForWorkspaceAccountID: Record<number, boolean>; | ||
|
|
||
| /** | ||
| * Whether to show the RBR for each feed name with domain ID. | ||
| */ | ||
| shouldShowRbrForFeedNameWithDomainID: Record<string, boolean>; |
There was a problem hiding this comment.
shouldShowRbrForWorkspaceAccountID, shouldShowRbrForFeedNameWithDomainID lost the comments documenting the Record key.
The latter's key is a composite feed-name+domain-ID string, so a wrong lookup key returns undefined rather than failing to compile.
There was a problem hiding this comment.
the key is in the member name in both cases: ForWorkspaceAccountID and ForFeedNameWithDomainID. The removed comments said the same words as the identifier. Nothing to distinguish or add, so leaving these out
| contentContainerStyle: StyleProp<ViewStyle>; | ||
|
|
||
| /** Outer container style for the list wrapper. */ | ||
| containerStyle: StyleProp<ViewStyle>; |
There was a problem hiding this comment.
contentContainerStyle and containerStyle are both StyleProp<ViewStyle> and adjacent.
The removed comments were the only thing saying which one is the outer wrapper.
There was a problem hiding this comment.
contentContainerStyle vs containerStyle is standard RN naming (FlatList/ScrollView use the same pair, content is the inner one). The comments restated that convention, but if you find this super hard to read I guess I can bring the comment back, leaving out for now
| /** Fires when the list scrolls near its end (router's fetchMoreResults). */ | ||
| onEndReached: () => void; | ||
|
|
||
| /** Fires on the list's first layout and on layout changes. */ | ||
| onLayout: () => void; |
There was a problem hiding this comment.
onLayout lost its firing condition ("first layout and on layout changes") while the adjacent onEndReached kept its equivalent.
There was a problem hiding this comment.
onLayout firing on first layout and on changes is what every RN onLayout does. The comment restated the platform contract. onEndReached kept its comment because it names the caller (router's fetchMoreResults), which is not derivable
| @@ -424,13 +417,11 @@ type TransactionMemberGroupListItemType = TransactionGroupListItemType & {groupe | |||
| }; | |||
|
|
|||
| type TransactionMonthGroupListItemType = TransactionGroupListItemType & {groupedBy: typeof CONST.SEARCH.GROUP_BY.MONTH} & SearchMonthGroup & { | |||
| /** Final and formatted "month" value used for displaying */ | |||
| formattedMonth: string; | |||
There was a problem hiding this comment.
formattedMonth/formattedWeek/formattedYear/formattedQuarter lost "Final and formatted … used for displaying" while their shortFormatted* siblings kept "Compact … where space is tight", leaving the pairs asymmetric.
There was a problem hiding this comment.
the shortFormatted* comments survive because they state where the value is used (chart axis labels) and why (space is tight). "Final and formatted month value used for displaying" restates formattedMonth. Symmetry alone is not one of the keep categories, so leaving these out
| @@ -1100,13 +1006,11 @@ type OriginalMessageConciergeAutoMatchVendor = { | |||
|
|
|||
| /** Policy rules modified fields */ | |||
| type PolicyRulesModifiedFields = { | |||
| /** The value that the merchant was changed to */ | |||
| merchant?: string; | |||
|
|
|||
| /** The value that the amount was changed to */ | |||
| category?: string; | |||
There was a problem hiding this comment.
category is not the amount
There was a problem hiding this comment.
wrong on main too, but yeah, fixed, now handled by the type-level comment
| @@ -1100,13 +1006,11 @@ type OriginalMessageConciergeAutoMatchVendor = { | |||
|
|
|||
| /** Policy rules modified fields */ | |||
| type PolicyRulesModifiedFields = { | |||
| /** The value that the merchant was changed to */ | |||
| merchant?: string; | |||
There was a problem hiding this comment.
PolicyRulesModifiedFields / PersonalRulesModifiedFields: half-stripped, and the surviving comment is the wrong one. (line 1008-1061)
Every member of both types carried the parallel phrase "The value that X was changed to".
That phrase is the type's entire semantics - these hold the new values a rule wrote, not current values.
The PR deleted it from merchant, tag, billable, tax (and reportName in the personal variant) while keeping it on category, comment, description, reimbursable.
A bare merchant?: string in a type named ...ModifiedFields now reads as "the merchant", which is the opposite of what it is.
Either restore the phrase on all members or delete it from all and document it once on the type.
There was a problem hiding this comment.
agreed the half-stripped state was bad. Went with your second option: dropped the phrase from every member (including the wrong category one and reimbursable) and documented it once on both types: "Each member holds the new value the rule wrote, not the current one". Kept comment/description since they carry the backend key mapping
| @@ -3245,7 +3113,6 @@ type Policy = OnyxCommon.OnyxValueWithOfflineFeedback< | |||
| /** Max amount for an expense with no itemized receipt violation */ | |||
| maxExpenseAmountNoItemizedReceipt?: number; | |||
|
|
|||
| /** Whether GL codes are enabled */ | |||
| glCodes?: boolean; | |||
There was a problem hiding this comment.
glCodes?: boolean lost the only thing making it a flag.
The name is a bare plural noun and the type is boolean - nothing left says this is an enable switch rather than a collection.
It's also one of three GL-code booleans in the same Policy type (showTagGLCodes :2937, showCategoryGLCodes : 2945), both also stripped.
Those two are self-explanatory from their verb prefixes.
glCodes is the master flag and is not. This is the rule's own "distinction from a sibling" keep category.
There was a problem hiding this comment.
I'm not sure I follow, glCodes?: boolean cannot be a collection, the type already says flag? "Whether GL codes are enabled" restates name plus type, which is exactly what this PR removes. Leaving it out.
There was a problem hiding this comment.
I meant that glCodes?: boolean itself doesn't self-explain "Whether GL codes are enabled or not (like shouldEnableGLCodes or enableGLCodes does).
I am fine leaving it out though.
| conciergeChat: OnyxEntry<Report>; | ||
| /** The admins chat report, looked up by ONYXKEYS.ONBOARDING_ADMINS_CHAT_REPORT_ID. */ | ||
| adminsChatReport?: OnyxEntry<Report>; | ||
| /** The self-DM report, looked up by ONYXKEYS.SELF_DM_REPORT_ID. */ | ||
| selfDMReport?: OnyxEntry<Report>; |
There was a problem hiding this comment.
selfDMReport dropped its Onyx-key pointer while both siblings (conciergeChat, adminsChatReport) kept theirs.
Three parallel params, one loses the "which Onyx key feeds this" pointer. That's ownership - an explicit keep category - and callers of this action genuinely need it to know what to pass.
There was a problem hiding this comment.
yup, restored, the Onyx lookup pointer is useful for callers and matches the two siblings
| /* The policy that the card will be issued under */ | ||
| /** The policy that the card will be issued under */ | ||
| policyID: string; |
There was a problem hiding this comment.
This should be policy ID, not policy
There was a problem hiding this comment.
yeah, same on main but fixed
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.mov |
updated PR desc |
There was a problem hiding this comment.
Wasn't expecting this PR, can't reasonably review the whole thing line-by-line. So I scrolled through and checked for any non-comment changes, didn't see any. Did not read every removed comment.
NAB: remove empty lines between props/types that have no comments to make the code a bit more compact and reduce the total number of lines in the affected files.
ah, it was mentioned in the previous PR description, sorry for not giving a heads up though
✅ |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
"Validate Mobile-Expensify submodule" failure is unrelated. Ignoring and merging to avoid further conflicts |
|
🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
@roryabraham looks like this was merged without a test passing. These checks had not passed on bbf8f72 when it was merged:
Please add a note explaining why this was done and remove the |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.75-0 🚀
|
|
🤖 No help site changes are required. I did not create a docs PR. Why: I pulled all 877 changed files and scanned every patch hunk. Every added or removed line is a code comment ( Verification detailsScanned each file's patch from All 877 files returned a patch, so nothing was skipped for size. Specific checks against things the help site does track:
The help site under Because no docs PR was needed, I also skipped the UI label verification pass — there is no new help site copy to check feature names, tab names, or button capitalization against. If this PR is later found to have changed behavior, re-run me and I'll draft the docs update. @JakubKorytko, no help site PR was created, so there is nothing to review or mark |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.75-3 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.75-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Removes ~3700 member comments that only restate the member name or type ("Icon width" on
iconWidth, "List of reports" ondata: Report[]), per the amended CONSISTENCY-10 rule from #99906. Most are component props andsrc/types/onyxtypes; the same pass also covers action param types (src/libs/actions/*), navigation route param types (src/libs/Navigation/*) and key comments insrc/ONYXKEYS.tsandsrc/CONST.Comments that state a unit, default, condition, key semantics or sibling distinction are kept.
Some
//and/* */member comments that do add information were converted to/** */.Fixed Issues
$ #99453
PROPOSAL: N/A
Tests
Not testable in the app. The diff contains only comment deletions and
//to/** */conversions above type members. No runtime code, copy, or styles changed.Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari