Repository navigation
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c87406c694
ℹ️ 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
left a comment
There was a problem hiding this comment.
Really like the golden master idea, it's a good safety net for the migration. The main thing is where the test drifts from what the app passes in (activePolicyID, the system account IDs, rules/policy tags), since the snapshot can't catch a change in behavior it never recorded. The rest is small. I didn't mark every missing blank line before a comment, could you go through the files for the others?
|
@MelvinBot review |
ReviewThe harness mirrors the production call sites closely. I found three gaps not raised in the earlier review comments. In each one, a snapshot entry looks like it pins a rule but does not. 1. "Hidden thread" does not pin the hidden-thread rule. 2. The blank-query call-site entries record a state the SearchRouter never shows. 3. The focused "Empty thread" blank entry duplicates the unfocused one. I did not run the suite. These findings come from reading the code and the committed snapshot at |
|
All three addressed:
|
Codecov Report✅ All modified and coverable lines are covered by tests. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 359f790f03
ℹ️ 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".
b7a1c18 to
fe6546f
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe6546f568
ℹ️ 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".
fe6546f to
ce46338
Compare
|
@eVoloshchak 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] |
Reviewer Checklist
Screenshots/Videos |
grgia
left a comment
There was a problem hiding this comment.
Ran the parity test on this branch merged into current main and it passes. One non-blocking thought on keeping the harness tied to the screen's config, inline.
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.5.27-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.5.27-2 🚀
Bundle Size Analysis (Sentry): |

Explanation of Change
This is Part 1 of a 3-part stacked PR series introducing a golden master regression harness for
SearchRouteroption search.The harness records, on
mainand before the upcoming in-memorySearchOptionsIndex, what the existingOptionsListUtilspipeline (createFilteredOptionListthroughgetSearchOptions) returns for a deterministic account, so later refactors can prove they change nothing.tests/utils/searchParity/generateSearchFixture.ts: a deterministic account with fixed IDs, unique timestamps and unique contact names. It covers 1:1 DMs, a group chat, a self-DM, own and others' workspace chats, an invoice room,#admins/#announce/custom rooms (one with an error, one whose workspace is gone), threads (with replies, hidden, empty, drafted, pinned), a task, expense reports, an archived DM, a DM with an unknown participant, and 40 filler contacts.Vít Novák,José Ñúñez,İsmail Yılmaz,Groß Straße,Łukasz Wójcik,Οδυσσεύς,Zoë 👩💻(ZWJ),AnaMaria(U+200B),Bob Builder(NBSP), NFD-decomposedRené Dupont,first.last+tag@x.com, an SMS login.JOSÉ,İSMAIL,Vít,René,Groß, an existing contact's phone and the own login, which must not invite).tests/unit/OptionsListUtilsSearchParityTest.ts: runs the production configs and records the results.searchRouterLocal(the SearchAutocompleteList local pass) andsearchRouterDisplayed(its ranking viacombineOrderingOfReportsAndPersonalDetails, theuserToInviteappend and the 20-row cap, recorded as the displayed rows only) for every query.loadAllexpands lists under 20 rows. The small-cap list is only recorded, never searched.Serialization
Each option is one diff-friendly line:
[flags]:[selected],[disabled],[bold],[gbr],[rbr],[archived].[alt: ...]:alternateText(chat previews in search results).[sub: ...]:subtitle, only for rooms and workspace chats, because that is what search matches for them.[accID: ...]: the option'saccountID, printed whenever it is non-zero (1:1 rows and contacts; groups and rooms carry none).[invitedLogin]: only onuserToInvite, when its login differs from its text.Fields the SearchRouter never reads (
participantsList,icons,policyID,ownerAccountID,item) are deliberately not serialized.Notes
getReportByIDis kept stable per reports collection with aWeakMap, asuseCallback(..., [allReports])does inuseFilteredOptions, so the preview cache is not invalidated between queries.hasMoreis recorded fromgetSearchOptions; no SearchRouter consumer reads it (the router'shasMorecomes fromuseFilteredOptions).npm test -- tests/unit/OptionsListUtilsSearchParityTest.ts -u) only against the pre-migration pipeline onmain, so the migration never becomes its own oracle.Fixed Issues
$ #103148
PROPOSAL:
Tests
Run
npm test -- tests/unit/OptionsListUtilsSearchParityTest.ts.Offline tests
N/A
QA Steps
N/A
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)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.