Repository navigation
Fix Connections regressions from #102265 - #104402
Krishna2323 wants to merge 8 commits into
Conversation
…oncierge, disconnect flash, and Back after refresh on settings pages Signed-off-by: krishna2323 <[email protected]>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@JmillsExpensify @trjExpensify @lakchote Two questions before I open it for review:
Which do you prefer?
|
…an integration turns Connections on Signed-off-by: krishna2323 <[email protected]>
I'm fine with this, so long as we go through staging regression testing first. What we want to avoid is deploying it so customers see the consolidated page and then reverting it.
Yeah, let's do this. We could have the link open on the |
Signed-off-by: krishna2323 <[email protected]>
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx bun ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
|
@MelvinBot review |
ReviewThe PR looks solid overall. I found one real regression to fix before merge, and a few small issues. The four fixes in the description all check out in the code. Should fix
Minor
What I checked
|
…liation Back on the stack, and only skip the connections fetch for a workspace still being created Signed-off-by: krishna2323 <[email protected]>
…anel where an integration is connected, the Accounting tab otherwise Signed-off-by: krishna2323 <[email protected]>
|
@MelvinBot 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. |
Follow-up reviewI reviewed the two new commits. They fix four of my earlier points and introduce no new bugs. Two points are still open. Fixed
Still open
Optional: four callers now pass four I didn't run typecheck or test the app. |
|
My quick comments:
|
|
@Krishna2323 agree with @trjExpensify and @JmillsExpensify, let's turn it on for everyone once it's been through staging regression testing. One ask if it's not too much work: could the beta removal and the page deletions go in their own PR on top of the fixes? Then if QA finds something on staging we revert only the launch and the fixes still ship. The "Click Connections" change you already have is fine to keep. Tasks that were already posted will keep saying "Click Accounting", but their link goes to @trjExpensify @JmillsExpensify one more thing that comes with the launch: in NewDot, MCP can only be turned off from More features today, and this PR removes that toggle. So admins won't have a way to block AI assistants on a workspace. MelvinBot flagged it here: #104402 (comment). Are we fine with that, or do we want an MCP on/off switch in Connections before launch? |
|
I don't think that matters. The MCP capability is more of an account-specific thing. We're including it in the workspace for customer visibility more than anything else. |
Yeah, does it actually do that? |
|
@trjExpensify No, it doesn't. I double-checked after MelvinBot's flag and I was wrong about that part. The PR does remove the off switch, but turning it off only saved |
|
Cool yeah, to Jason's point, my recollection is that this is mostly an advertorial thing. |
…kspace, so connecting an integration doesn't show a loader, and drop the leftovers of the deleted pages Signed-off-by: krishna2323 <[email protected]>
Added as test step 10. |
…verts the launch and these fixes still ship Signed-off-by: krishna2323 <[email protected]>
Done. This PR now has only the fixes. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari104013.mov104029.mov104032.mov104319.mov104320-not.fixed.mov104321.mov |
|
Since the launch was moved to separate PR, let's update the description and Fixed Issues accordingly |
|
@MelvinBot review |
Review of
|
Regression check of
|
|
@situchan Updated the description and Fixed Issues. |
|
Please merge main |
|
#104320 is still reproducible. after clicking on accounting, app still opens More features Screen.Recording.2026-10-22.at.9.57.38.PM.mov |
|
Looks good except #104402 (comment). |
Explanation of Change
Fixes the regressions reported after #102265. The launch (beta removal, page deletions, link updates and the onboarding text) is in #, on top of this one, so it can be reverted without these fixes.
/connectionsinstead of/accounting.Fixed Issues
$ #104013
$ #104032
$ #104319
$ #104320
$ #104029
$ #104321
PROPOSAL: N/A, follow-up to #102265
Tests
Precondition: enable the
unifiedConnectionsbeta./connections.Offline tests
Precondition: enable the
unifiedConnectionsbeta.QA Steps
Same as tests, with the
unifiedConnectionsbeta enabled.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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
test_1.mp4