nixos/mysql: remove fixed timeout and use notify service type

Description of changes
Removes the hard coded 30 second startup timeout in postStart since systemd's TimeoutStartSec is a suitable and more flexible alternative. The 30 second timeout also not long enough for large databases where innodb initialization can often take >1min.
The systemd service type itself is changed to "notify" for MySQL >= 8.0 [1]. This means none of the MySQL/MariaDB versions in nixpkgs use the "simple" service type and therefore don't even need the manual loop/polling for the socket. However, there may be other MySQL implementations added to nixpkgs in the future (MyRocks, etc.) that may be based on MySQL < 8.0 and require the "simple" service type.
[1] https://dev.mysql.com/doc/refman/8.0/en/using-systemd.html
Things done
The mysql test for mysql80 and mariadb both pass. I've confirmed that systemd's TimeoutStartSec setting applies to both the ExecStart and ExecStartPost scripts.

Built on platform(s)

[x] x86_64-linux
[ ] aarch64-linux
[ ] x86_64-darwin
[ ] aarch64-darwin

For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)

[ ] sandbox = relaxed
[ ] sandbox = true

[x] Tested, as applicable:

NixOS test(s) (look inside nixos/tests)
and/or package tests
or, for functions and "core" functionality, tests in lib/tests or pkgs/test
made sure NixOS tests are linked to the relevant packages

[ ] Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
[ ] Tested basic functionality of all binary files (usually in ./result/bin/)
24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)

[ ] (Package updates) Added a release notes entry if the change is major or breaking
[ ] (Module updates) Added a release notes entry if the change is significant
[ ] (Module addition) Added a release notes entry if adding a new NixOS module

[ ] Fits CONTRIBUTING.md.

does anyone here have a database that takes longer than a second to start up that can test this?
this looks fine but it would be really nice if we could practically test this
if not... i guess we merge and rely on logic

@aanderse My synthetic testing method was to add a sleep 30 to the beginning of the ExecStart script to test that the ExecStartPost script is also postponed an additional 30 seconds. Technically, this doesn't test that MySQL itself waits to send the systemd READY=1 status until after the socket is available - just going by upstream docs and observed behavior.
Here's the log of the mysql test with the 30 second delay but without sub-second resolution it's hard to confirm:
Feb 15 08:19:23 mariadb80 mysql-pre-start[861]: 2024-02-15T08:19:23.122910Z 0 [System] [MY-013169] [Server] /nix/store/vwcld5bqs1dg7h2ffcgz8am4vdrxw176-mysql-8.0.36/bin/mysqld (mysqld 8.0.36) initializing of server in progress as process 861
Feb 15 08:19:23 mariadb80 mysql-pre-start[861]: 2024-02-15T08:19:23.155992Z 1 [System] [MY-013576] [InnoDB] InnoDB initialization has started.
Feb 15 08:19:23 mariadb80 mysql-pre-start[861]: 2024-02-15T08:19:23.756727Z 1 [System] [MY-013577] [InnoDB] InnoDB initialization has ended.
Feb 15 08:19:24 mariadb80 mysql-pre-start[861]: 2024-02-15T08:19:24.149674Z 0 [Warning] [MY-013501] [Server] Ignoring --plugin-load[_add] list as the server is running with --initialize(-insecure).
Feb 15 08:19:25 mariadb80 mysql-pre-start[861]: 2024-02-15T08:19:25.123350Z 6 [Warning] [MY-010453] [Server] root@localhost is created with an empty password ! Please consider switching off the --initialize-insecure option.
Feb 15 08:19:27 mariadb80 mysql-start[988]: MySQL delay start...
Feb 15 08:19:57 mariadb80 mysql-start[988]: MySQL delay end...
Feb 15 08:19:57 mariadb80 mysql-start[988]: 2024-02-15T08:19:57.716130Z 0 [System] [MY-010116] [Server] /nix/store/vwcld5bqs1dg7h2ffcgz8am4vdrxw176-mysql-8.0.36/bin/mysqld (mysqld 8.0.36) starting as process 988
Feb 15 08:19:57 mariadb80 mysql-start[988]: 2024-02-15T08:19:57.732427Z 1 [System] [MY-013576] [InnoDB] InnoDB initialization has started.
Feb 15 08:19:58 mariadb80 mysql-start[988]: 2024-02-15T08:19:58.121585Z 1 [System] [MY-013577] [InnoDB] InnoDB initialization has ended.
Feb 15 08:19:58 mariadb80 mysql-start[988]: 2024-02-15T08:19:58.453086Z 0 [Warning] [MY-010068] [Server] CA certificate ca.pem is self signed.
Feb 15 08:19:58 mariadb80 mysql-start[988]: 2024-02-15T08:19:58.454453Z 0 [System] [MY-013602] [Server] Channel mysql_main configured to support TLS. Encrypted connections are now supported for this channel.
Feb 15 08:19:58 mariadb80 mysql-start[988]: 2024-02-15T08:19:58.478320Z 0 [System] [MY-011323] [Server] X Plugin ready for connections. Bind-address: '::' port: 33060, socket: /run/mysqld/mysqlx.sock
Feb 15 08:19:58 mariadb80 mysql-start[988]: 2024-02-15T08:19:58.479943Z 0 [System] [MY-010931] [Server] /nix/store/vwcld5bqs1dg7h2ffcgz8am4vdrxw176-mysql-8.0.36/bin/mysqld: ready for connections. Version: '8.0.36'  socket: '/run/mysqld/mysqld.sock'  port: 3306  Source distribution.
Feb 15 08:19:58 mariadb80 mysql-post-start[1162]: Creating initial database: testdb3

This worked for me on a large database that was not starting before the post start script caused the service to restart.


Conflict if Microsoft.MsBuild.Sql templates are installed

It is a choice, not really a conflict...


Menu instability on macOS

Issue Type: Bug

Quite frequently there is no "Open Recent..." submenu after opening a new VS Code window. Trying it again makes it appear.
selecting an item from the "Open Recent..." submenu sometimes has no effect on first try.
sometimes the menubar menus are missing too on first try.

VS Code version: Code - Insiders 1.26.0-insider (21ddb4843b2f40e3cd82500794dc9a1b9404717d, 2018-08-06T08:51:57.522Z)
OS version: Darwin x64 17.7.0

System Info

Item
Value

CPUs
Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz (12 x 2900)

GPU Status
2d_canvas: enabledchecker_imaging: disabled_offflash_3d: enabledflash_stage3d: enabledflash_stage3d_baseline: enabledgpu_compositing: enabledmultiple_raster_threads: enabled_onnative_gpu_memory_buffers: enabledrasterization: enabledvideo_decode: enabledvideo_encode: enabledwebgl: enabledwebgl2: enabled

Load (avg)
2, 2, 2

Memory (System)
32.00GB (0.38GB free)

Process Argv
/Applications/Visual Studio Code - Insiders.app/Contents/MacOS/Electron -psn_0_7907210

Screen Reader
no

VM
0%

Extensions: none

Yup, see it too

Open VS Code
Quickly click the File menu as soon as it opens
Move mouse to Open Recent..., nothing comes up on that submenu]
Move mouse to Edit top level menu, nothing comes up

@joaomoreno are you seeing this on macOS?

Yeah.

revert to old menu https://github.com/Microsoft/vscode/pull/55913


Add missing features

Node.js target (not to mention any others) is still missing some features:

[ ] Inheritance between C++ classes and automatic upcasting.
[x] Const and non-const pointers as argument and return types.
[x] Const and non-const references as argument and return types.
[x] Smart pointers as argument and return types.
[x] Conversion between JavaScript arrays and C++ vectors.
[x] Plain functions (like static methods but outside any class).
[x] Destroy method, freeing the C++ instance and invalidating the wrapper object's internal pointer.
[x] Support passing int64_t and uint64_t losslessly to a bignum object with hi, lo parts and a toString method.

Multiple inheritance is almost done. It makes overload resolution much more annoying, so overload support has been turned into a project and postponed until remaining issues are resolved.

Now these are finally done!


Plant Gatherer destroys my house

Industrial Foregoing Version: 1.15.2-2.2.1-f27c515
TeslaCoreLib Version:
Crashlog (If Any):
I placed Plant Gatherer with +8 upgrade and half of my house gone. It dig up this blocks: wood, planks, wooden fence, chest, door, barrels (from 'metabarrels' mod)
It's very strange behavior. Seems like duplicate of #661

I recorded it
https://youtu.be/RLEbraMVCH8

Demolished some villager's house

I don't understand why this types of blocks are harvestable

seems like its limited to a wider selection of wood blocks instead of something like minecraft:logs wich every log should have :D

Anything wood base it will destroy; Chests, Barrels, Jukebox, e.t.c. I found out the hard way having a max tiered range upgrade installed.

Fixed in that PR ^


All pins are not accessible

Hello,
Surely because you use wiringPi, but all pins are not accessible
Using default pin config, it works.
But in my project, I need to use GPIO 21 (pin 40) of my Raspi 4 and I wasn't able to find the right wiringPi pin
Any idea?

Pin 40 is WiringPi 29

@kencormack Thank you :-)


Remove images and allow per project configuration of possible cards

default card set configurable in plugin settings
concrete card set configurable in board configuration
card display with css instead of using images

Idea for styling https://jsfiddle.net/xd6hbe8a/6/

cards are now rendered by css with commit d1379094f1b8030da25c8ea77d942016aea3e525

cards can only be numbers when using the Estimation field shipping with Jira Software


NetService conformation with protocol Hashable

I use references to NetService services as keys in a dictionary, so if it's possible to make it hashable that would be wonderful!  Thank you!

You could add this through an extension as well I think?

Great idea.  Thanks!

This appears to have been resolved by upgrading Swift to 4.1.3.


Upgrade lightning-kmp to latest development version (1.3)

This PR upgrades lightning-kmp to 1.3.0-SNAPSHOT which contains several breaking changes:

there are new Features that need to be properly signaled
with the certificate pinning changes, the Connection object has been modified
the payments database needs to implement new methods to purge expired payments

@sstone @t-bast The node params Features changes are in this commit: https://github.com/ACINQ/phoenix/pull/269/commits/4291e47348285f717b405687d5fee3b0952063d1


chore(deps): update plugin org.ajoberstar.stutter to v0.7.2

Closes https://github.com/gradle-nexus/publish-plugin/pull/195
Inspired by https://github.com/gradle-nexus/publish-plugin/issues/226
Still to do:

[x] rewrite java-version.yml because lockfile format changed
[x] rewrite gradle-latest-versions.yml because lockfile format changed
[x] consider removing workarounds for fixed issues: https://github.com/ajoberstar/gradle-stutter/issues/22#issuecomment-1031680877

@szpak or anyone else: I need help with this, because I'm not entirely sure what those GHA jobs do.

@szpak I think this is ready for review and merge. The GHA jobs all run the equivalent tasks. Can you please kick off https://github.com/gradle-nexus/publish-plugin/actions/workflows/gradle-latest-versions.yml as well, although it haven't been passing for a while...

Thanks @TWiStErRob for working on that PR. Just one question about foojay-resolver-convention and I think it is ready to be merged.
Btw, I triggered the gradle-latest-version workflow (with enabled build scan) for the version from this branch.

So that scheduled workflow is also behaving as before as far as I see. I would merge this and then fix the ivy issue separately (+include a lockfile update from milestone to RC in that PR).

Thanks @TWiStErRob


Questions about model parameter initialization

Hello.
This work is based on the ControlNet method. The structure diagram of the paper shows that the parameters of UNet in the model are frozen. I would like to know how the parameters of UNet are initialized. Can you send me parameters or links if it is a pre-trained model?
Thanks for your reply!

We freeze parameter settings consistent with Controlnet; you can refer to https://github.com/lllyasviel/ControlNet.


Error in log when opening prefs

ERROR TypeError: Cannot set properties of undefined (setting 'disabled')
at i.enableSaveButton (main.670fb76c19c20a76.js:1:1320767)
at main.670fb76c19c20a76.js:1:1322229
at rv (main.670fb76c19c20a76.js:1:2085971)
at Object._ [as next] (main.670fb76c19c20a76.js:1:2086180)
at _e.next (main.670fb76c19c20a76.js:1:2006532)
at ze._next (main.670fb76c19c20a76.js:1:2006199)
at ze.next (main.670fb76c19c20a76.js:1:2005885)
at main.670fb76c19c20a76.js:1:2003756
at b (main.670fb76c19c20a76.js:1:2019718)
at Hk.next (main.670fb76c19c20a76.js:1:2003593)

Note: when opening settings this is logged twice.

I see it too when opening logbook settings. This is the unredacted stacktrace:
ERROR TypeError: Cannot set properties of undefined (setting 'disabled')
    at LogbookSettingsComponent.enableSaveButton (logbook-settings.component.ts:49:29)
    at LogbookSettingsComponent_Template_kel_station_detail_stationChange_7_listener (logbook-settings.component.html:8:26)
    at executeListenerWithErrorHandling (core.mjs:14935:16)
    at Object.wrapListenerIn_markDirtyAndPreventDefault [as next] (core.mjs:14976:22)
    at ConsumerObserver.next (Subscriber.js:91:33)
    at SafeSubscriber._next (Subscriber.js:60:26)
    at SafeSubscriber.next (Subscriber.js:31:18)
    at Subject.js:34:30
    at errorContext (errorContext.js:19:9)
    at EventEmitter_.next (Subject.js:27:21)


Support for Points is missing

Rather similar to Polygons; need support for xtgeo Points objects

Solved in #122


WIP: Review project README + change for F4PGA

Reviewed the README and while I was at it, also switched to the new name, so this will have to wait until the rename is completed + will require changes in the code (as symbiflow is used in make env etc)

Make sure the symbiflow command names are kept for backward compatibility...

Superseeded by #454.


Median flux option

adding a median flux command

I'm closing this PR in favor of #932, which contains the same changes but without the extra/unrelated commits. :+1:

I'm closing this PR in favor of #932, which contains the same changes but without the extra/unrelated commits. :+1:

I have added a check to test_targetpixelfile.py which passes.

I have added a check to test_targetpixelfile.py which passes.

Made alteration to test_targetpixelfile.py

Made alteration to test_targetpixelfile.py


Discord Logo will not dissaper

I replaced img src of discord to  tag, kindly check.

🛠 Fixes Issue

👨💻 Changes proposed

✔ Check List (Check all the applicable boxes)

[ ] My code follows the code style of this project.
[ ] This PR does not contain plagiarized content.
[ ] The title of my pull request is a short description of the requested changes.

📄 Note to reviewers

📷 Screenshots

hey its not corrected check the hachtobesrfest page in that join page its still reflecting.
Please open it once again Ill do it


:bug: no response if receiving unrecognized command in group/room

TO CHECK:

post_back from another bot will effect or not?

Codecov Report

:exclamation: No coverage uploaded for pull request base (support_line_room_and_group@1e3ac6f). Click here to learn what that means.
The diff coverage is n/a.

@@                      Coverage Diff                       @@
##             support_line_room_and_group     #157   +/-   ##
==============================================================
  Coverage                               ?   51.85%
==============================================================
  Files                                  ?       19
  Lines                                  ?      513
  Branches                               ?        0
==============================================================
  Hits                                   ?      266
  Misses                                 ?      218
  Partials                               ?       29

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e3ac6f...fa52977. Read the comment docs.


docs: add v0.2.3 release details to the CHANGELOG

Description
This adds the v0.2.3 release details to the changelog.
References

Review Checklist

[x] I have clicked on "allow edits by maintainers".
[x] I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
[x] The correct base branch is being used, if not main
[x] I have added tests to validate that the change in functionality is working as expected

Codecov Report
Base: 80.72% // Head: 80.64% // Decreases project coverage by -0.08% :warning:

Coverage data is based on head (5f9e4ce) compared to base (6269abc).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #267      +/-   ##
==========================================
- Coverage   80.72%   80.64%   -0.09%
==========================================
  Files          85       85
  Lines       11612    11612
==========================================
- Hits         9374     9364      -10
- Misses       1827     1834       +7
- Partials      411      414       +3

Impacted Files
Coverage Δ

server/commands/check_utils.go
93.27% <0.00%> (-1.27%)
:arrow_down:

server/errors/errors.go
78.75% <0.00%> (-1.25%)
:arrow_down:

storage/postgres/utils.go
88.65% <0.00%> (-0.71%)
:arrow_down:

storage/postgres/postgres.go
65.81% <0.00%> (-0.59%)
:arrow_down:

server/commands/check.go
86.42% <0.00%> (-0.56%)
:arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.


High memory  usage

Hi, all
cargo run --bin winit

winit.exe uses more than 200MB memory. Is that expected? Any way to reduce memory usage? Thanks.

It's expected for now, and we will get more precise about memory usage, but it's not a high priority at the moment.


Fix LHN toggle in responsive view!

Details

Fixed Issues

$ https://github.com/Expensify/App/issues/23810
PROPOSAL: https://github.com/Expensify/App/issues/23810#issuecomment-1656068129
Tests
Test Case 1

Go to help docs on mobile device.
Open LHN and click Split Bills.
Click on Paying Friends and Workspaces links.
The LHN should close and the page should be scrolled to the appropriate section.

[ ] Verify that no errors appear in the JS console

Offline tests
Same as above
QA Steps
Same as above

[ ] Verify that no errors appear in the JS console

PR Author Checklist

[x] I linked the correct issue in the ### Fixed Issues section above
[x] I wrote clear testing steps that cover the changes made in this PR

[x] I added steps for local testing in the Tests section
[x] I added steps for the expected offline behavior in the Offline steps section
[x] I added steps for Staging and/or Production testing in the QA steps section
[x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
[x] 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)
[x] 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).

[x] I included screenshots or videos for tests on all platforms
[x] I ran the tests on all platforms & verified they passed on:

[x] Android / native
[x] Android / Chrome
[x] iOS / native
[x] iOS / Safari
[x] MacOS / Chrome / Safari
[x] MacOS / Desktop

[x] 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)
[x] I followed proper code patterns (see Reviewing the code)

[x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
[x] I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
[x] I verified that comments were added to code that is not self explanatory
[x] 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.
[x] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method

[x] If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:

[x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
[x] 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 approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
[x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
[x] I verified the JSDocs style guidelines (in STYLE.md) were followed

[x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
[x] I followed the guidelines as stated in the Review Guidelines
[x] 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)
[x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
[x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
[x] I verified that if a function's arguments changed that all usages have also been updated correctly
[x] If a new component is created I verified that:

[x] A similar component doesn't exist in the codebase
[x] All props are defined accurately and each prop has a /** comment above it */
[x] The file is named correctly
[x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
[x] The only data being stored in the state is data necessary for rendering and nothing else
[x] If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
[x] For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
[x] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
[x] All JSX used for rendering exists in the render method
[x] 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

[x] If any new file was added I verified that:

[x] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory

[x] If a new CSS style is added I verified that:

[x] A similar style doesn't already exist
[x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))

[x] 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.
[x] 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)
[x] 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.
[x] 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.
[x] If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
[x] 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.
[x] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
Not applicable since the issue only exists in responsive mode.

Mobile Web - Chrome
https://github.com/Expensify/App/assets/36226639/3330db36-4259-4463-9f00-a83520950bb6

Mobile Web - Safari
https://github.com/Expensify/App/assets/36226639/889c0e00-a7c2-4bf5-a72d-d4ffb5780b8f

Desktop
Not applicable since the help docs are only available on web.

iOS
Not applicable since the help docs are only available on web.

Android
Not applicable since the help docs are only available on web.

Hi @abdulrahuman5196! Bump for PR review please.

Reviewing now

Reviewing now

@abdulrahuman5196 Any update on the PR review?

@Talha345 Can you remove the test case 2 from author's checklist? It is not relevant anymore

Reviewer Checklist

[x] I have verified the author checklist is complete (all boxes are checked off).
[x] I verified the correct issue is linked in the ### Fixed Issues section above
[x] I verified testing steps are clear and they cover the changes made in this PR

[x] I verified the steps for local testing are in the Tests section
[x] I verified the steps for Staging and/or Production testing are in the QA steps section
[x] 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)
[x] 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)

[x] I checked that screenshots or videos are included for tests on all platforms
[ ] I included screenshots or videos for tests on all platforms
[x] I verified tests pass on all platforms & I tested again on:

[x] Android / native
[x] Android / Chrome
[x] iOS / native
[x] iOS / Safari
[x] MacOS / Chrome / Safari
[x] MacOS / Desktop

[ ] 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
[x] I verified proper code patterns were followed (see Reviewing the code)

[x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
[x] I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
[x] I verified that comments were added to code that is not self explanatory
[x] 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.
[x] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
[x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
[x] 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 approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
[x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
[x] I verified the JSDocs style guidelines (in STYLE.md) were followed

[x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
[x] I verified that this PR follows the guidelines as stated in the Review Guidelines
[x] 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)
[x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
[x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
[x] If a new component is created I verified that:

[x] A similar component doesn't exist in the codebase
[x] All props are defined accurately and each prop has a /** comment above it */
[x] The file is named correctly
[x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
[x] The only data being stored in the state is data necessary for rendering and nothing else
[x] For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
[x] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
[x] All JSX used for rendering exists in the render method
[x] 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

[x] If any new file was added I verified that:

[x] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory

[x] If a new CSS style is added I verified that:

[x] A similar style doesn't already exist
[x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)

[x] 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.
[x] 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)
[x] 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.
[x] If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
[x] 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

Web

https://github.com/Expensify/App/assets/46707890/41a99b06-e090-41df-aa5d-190065256b91

Mobile Web - Chrome

https://github.com/Expensify/App/assets/46707890/bc004e7b-2fd1-48c3-a266-dd31e5b54a93

Mobile Web - Safari

https://github.com/Expensify/App/assets/46707890/ffa20d1e-8123-4772-b84e-0ec4a667b967

Desktop

iOS

Android

NAB: For future cases kindly add video screenshots for web as well even though the issue is in mobile size screens as well. For this case I have tested now.

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

🚀 Deployed to staging by https://github.com/tgolen in version: 1.3.71-0 🚀

platform
result

🤖 android 🤖
success ✅

🖥 desktop 🖥
success ✅

🍎 iOS 🍎
failure ❌

🕸 web 🕸
success ✅


Update App to React Native 0.72 version

Details
This PR aims to update Expensify's App to use our RN 0.72 fork. Some changes were made in order to follow upgrading from previous RN version to current one, following this steps.
SLACK DISCUSSION THREAD: https://expensify.slack.com/archives/C01GTK53T8Q/p1683321604215369
Patches created and corresponding upstream PRs

@onfido/react-native-sdk -> https://github.com/onfido/react-native-sdk/pull/130
lottie-react-native -> https://github.com/lottie-react-native/lottie-react-native/issues/1022
@oguzhnatly/react-native-image-manipulator -> We are going to replace with Expo Image Manipulator in the future

Fixed Issues
$ https://github.com/Expensify/App/issues/18444
$ https://github.com/Expensify/App/issues/21308
PROPOSAL: -
Tests
Full regression test

[x] Verify that no errors appear in the JS console

Offline tests
Same as above.
QA Steps
Same as above.

[x] Verify that no errors appear in the JS console

PR Author Checklist

[x] I linked the correct issue in the ### Fixed Issues section above
[x] I wrote clear testing steps that cover the changes made in this PR

[x] I added steps for local testing in the Tests section
[x] I added steps for the expected offline behavior in the Offline steps section
[x] I added steps for Staging and/or Production testing in the QA steps section
[x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
[x] 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)
[x] 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).

[x] I included screenshots or videos for tests on all platforms
[x] I ran the tests on all platforms & verified they passed on:

[x] Android / native
[x] Android / Chrome
[x] iOS / native
[x] iOS / Safari
[x] MacOS / Chrome / Safari
[x] MacOS / Desktop

[x] 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)
[x] I followed proper code patterns (see Reviewing the code)

[x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
[x] I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
[x] I verified that comments were added to code that is not self explanatory
[x] 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.
[x] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method

[x] If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:

[x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
[x] 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 approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
[x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
[x] I verified the JSDocs style guidelines (in STYLE.md) were followed

[x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
[x] I followed the guidelines as stated in the Review Guidelines
[x] 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)
[x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
[x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
[x] I verified that if a function's arguments changed that all usages have also been updated correctly
[x] If a new component is created I verified that:

[x] A similar component doesn't exist in the codebase
[x] All props are defined accurately and each prop has a /** comment above it */
[x] The file is named correctly
[x] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
[x] The only data being stored in the state is data necessary for rendering and nothing else
[x] For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
[x] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
[x] All JSX used for rendering exists in the render method
[x] 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

[x] If any new file was added I verified that:

[x] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory

[x] If a new CSS style is added I verified that:

[x] A similar style doesn't already exist
[x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))

[x] 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.
[x] 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)
[x] 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.
[x] If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
[x] 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.
[x] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

Update
✅ Web and Desktop builds are working at a first glance.
❌ Android and iOS builds are totally broken.
Trying to run npm run android or npm run ios will lead to error: unknown command 'run-android' and error: unknown command 'run-ios' errors respectively. Trying to run pod install will lead to errors too, complaining about missing files (Error: Cannot find module 'react-native/scripts/react_native_pods.rb').
After some investigation, it seems that v0.72.0-rc.1-alpha.0 release is broken. First evidence is that it is incredibly small (only 27 MBs) compared to previous one. Second evidence is that its source files are wrong, missing files and scripts necessary to build the project (hence the errors above).
It looks like the sources must be built from packages/react-native/ files, according to publish-npm.js file. It was a change introduced in this commit, so it's something new.
We have our own publish-npm-expensify.js file to build the sources for our fork, but this change isn't there and is likely the reason for the broken release.
So, I changed every path in the file to point to packages/react-native/, built a local release, used it on the project and the errors above stopped (other errors appeared but are build errors and needs further investigations). The release also had a more realistic size, 350 MBs.
@roryabraham I will raise a PR to Expensify's RN fork with these changes for you to review it, which branch do I have to target?
We are probably going to need another release to continue with the RN update.

Normally I would say you should target the latest release v0.71.2-rc.1-alpha.0 but if its borked, maybe we should wait for a new one. cc @roryabraham

@roryabraham I created a branch from the latest tag, here is the changes.
Which branch should I target for the PR?

@fabioh8010 with the reanmiated changes feel free to post in #open-source channel and tag Tomek Zawadski and we might get help from them. I will try to ask Rory for help in terms of the branch

I can help with resolving iOS build errors related to custom properties positionY/X  since I was involved in implementing this functionality

@fabioh8010  I noticed that the selectionOrigin variable was not defined. This may have been an oversight on my part. To resolve this issue, you can define selectionOrigin in the textInputDidChangeSelection method in Libraries/Text/TextInput/RCTBaseTextInputView.m as follows:
- (void)textInputDidChangeSelection
{
  if (!_onSelectionChange) {
    return;
  }

  RCTTextSelection *selection = self.selection;
  UITextRange *selectedTextRange = self.backedTextInputView.selectedTextRange;
  CGPoint selectionOrigin = [self.backedTextInputView caretRectForPosition:selectedTextRange.start].origin;

  _onSelectionChange(@{
    @"selection" : @{
      @"start" : @(selection.start),
      @"end" : @(selection.end),
      @"positionY": @(selectionOrigin.y),
      @"positionX": @(selectionOrigin.x),
    },
  });
}

Update
✅ All platforms are building now.

Updated react-native-reanimated library to 3.1.0 in order to fix the remaining Android build error.
Created a temporary patch to fix iOS build following @perunt's suggestions.

@mountiny I guess that at this point now we can start involving QA to help test the application and find problems.

Update

Created a issue and a PR about the patches I made in the libraries.
Merged branch with main
Currently investigating why unit tests are failing:

314 of 534 tests are failing, the two most common reasons are ReferenceError: setImmediate is not defined and thrown: "Exceeded timeout of 5000 ms for a hook. Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

@fabioh8010 Sorry was at a conference. Would we be good to add the Ready to Build label and try the build for QA?

@mountiny Yes, I think we are good for testing now. All platforms are building and running, I'm getting some crashes in iOS when navigating from Chat to Sidebar but besides it we could already build it for QA play around and find issues.

triggered build

Use the links below to test this build in android and iOS. Happy testing!

@mountiny Looks like the Android and iOS build failed, where can I take a look at the build logs?

Update
Unit tests failing
I didn't find a solution for this yet. The two majority errors are:
ReferenceError: setImmediate is not defined
Example of error:
 FAIL  tests/unit/SidebarTest.js (12.844 s)
  ● Sidebar › archived chats › renders the archive reason as the preview message of the chat

    ReferenceError: setImmediate is not defined

      6 |  * @returns {Promise}
      7 |  */
    > 8 | export default () => new Promise(setImmediate);
        |                                  ^
      9 |

      at setImmediate (tests/utils/waitForPromisesToResolve.js:8:34)
      at Object.<anonymous> (tests/unit/SidebarTest.js:55:41)

Neither setImmediate and clearImmediate are available during tests, I tried to define a mock version by myself e.g.:
global.setImmediate = global.setImmediate || function(fn) { return setTimeout(fn, 0); };
global.clearImmediate = global.clearImmediate || global.clearTimeout;

It solves the problem but I wouldn't say that's the path to go without understanding why this errors started to show up.
Exceeded timeout of 5000 ms for a hook. Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test.
Example of error:
 FAIL  tests/unit/SidebarOrderTest.js (98.761 s)
  ● Sidebar › in default mode › is not rendered when there are no props passed to it

    thrown: "Exceeded timeout of 5000 ms for a hook.
    Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

      1 | import Onyx from 'react-native-onyx';
    > 2 | import {cleanup, screen} from '@testing-library/react-native';
        | ^
      3 | import lodashGet from 'lodash/get';
      4 | import waitForPromisesToResolve from '../utils/waitForPromisesToResolve';
      5 | import * as LHNTestUtils from '../utils/LHNTestUtils';

      at Object.afterEach (node_modules/@testing-library/react-native/src/index.ts:13:5)
      at Object.<anonymous> (tests/unit/SidebarOrderTest.js:2:1)
      at asyncGeneratorStep (node_modules/@babel/runtime/helpers/asyncToGenerator.js:3:24)
      at asyncGeneratorStep (node_modules/@babel/runtime/helpers/asyncToGenerator.js:25:9)

It seems that all related test errors point to the same import, import {cleanup, screen} from '@testing-library/react-native';. Maybe it's a compatibility issue with new RN version, will investigate more.
I also noticed that we have this comment on jest.config.js file.
module.exports = {
    // TODO: change this back to preset: 'react-native' once we are able fix the tests on React Native >= 0.71.2
    preset: '@testing-library/react-native',
    ...
};

It was introduced in this commit. @priyeshshah11 Do you have some context about this change?
iOS crashes
I'm getting this crash sometimes when navigating between the chat and the sidebar:
2023-05-15 18:09:38.512311+0100 New Expensify[54437:738696] *** Assertion failure in -[REANodesManager uiManager:performMountingWithBlock:](), /Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/node_modules/react-native-reanimated/ios/REANodesManager.mm:246
2023-05-15 18:09:38.524994+0100 New Expensify[54437:738696] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Mouting block is expected to not be set'
*** First throw call stack:
(
	0   CoreFoundation                      0x000000011b4328ab __exceptionPreprocess + 242
	1   libobjc.A.dylib                     0x000000010f5a0ba3 objc_exception_throw + 48
	2   Foundation                          0x000000011573d271 -[NSMutableDictionary(NSMutableDictionary) classForCoder] + 0
	3   New Expensify                       0x0000000101a81241 -[REANodesManager uiManager:performMountingWithBlock:] + 289
	4   New Expensify                       0x0000000101c69051 -[RCTUIManagerObserverCoordinator uiManager:performMountingWithBlock:] + 529
	5   New Expensify                       0x0000000101c62f02 -[RCTUIManager flushUIBlocksWithCompletion:] + 626
	6   New Expensify                       0x0000000101c62c10 -[RCTUIManager _layoutAndMount] + 832
	7   New Expensify                       0x0000000101c628c1 -[RCTUIManager batchDidComplete] + 33
	8   New Expensify                       0x0000000101a805bf -[RCTUIManager(SyncUpdates) runSyncUIUpdatesWithObserver:] + 335
	9   New Expensify                       0x0000000101a81987 __36-[REANodesManager performOperations]_block_invoke + 695
	10  New Expensify                       0x0000000101c699dd __RCTExecuteOnUIManagerQueue_block_invoke + 29
	11  libdispatch.dylib                   0x000000011b0f4d18 _dispatch_call_block_and_release + 12
	12  libdispatch.dylib                   0x000000011b0f5f5b _dispatch_client_callout + 8
	13  libdispatch.dylib                   0x000000011b0fdd41 _dispatch_lane_serial_drain + 1064
	14  libdispatch.dylib                   0x000000011b0feb1b _dispatch_lane_invoke + 441
	15  libdispatch.dylib                   0x000000011b10cf13 _dispatch_workloop_worker_thread + 959
	16  libsystem_pthread.dylib             0x000000011ed93ce3 _pthread_wqthread + 326
	17  libsystem_pthread.dylib             0x000000011ed92c67 start_wqthread + 15
)
libc++abi: terminating with uncaught exception of type NSException
Message from debugger: Terminated due to signal 6

https://github.com/Expensify/App/assets/20051562/a3616eb4-ea83-4629-9bd6-ffab9287f41e
Since we are already using the latest react-native-reanimated version on this PR, I'll tag Tomek Zawadski on Slack to see if he can help.

@fabioh8010 here is the build run https://github.com/Expensify/App/actions/runs/4983345153

Update

Fixed iOS crashes with Tomek Zawadski's help, now we are going to use a nightly build of react-native-reanimated until we have a release with the latest fixes.
Updated lottie-react-native to 5.1.6 which fixes the Android builds, so I removed my patch as well.
Made some adjustments which I believe will solve the build problems in GitHub.

@mountiny Could you trigger another build? Thanks.

New build triggered here

:test_tube::test_tube: Use the links below to test this build in android and iOS. Happy testing! :test_tube::test_tube:

android :robot:
iOS :apple:

❌ FAILED ❌
❌ FAILED ❌

The QR code can't be generated, because the android build failed
The QR code can't be generated, because the iOS build failed

desktop :computer:
web :spider_web:

https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/18507/NewExpensify.dmg
https://18507.pr-testing.expensify.com

Update

Merged branch with main.
All unit tests are passing now ✅
Investigating the build errors reported by CI. I think the iOS build failed because it wasn't updated with main branch, but Android seems to be another cause.

Running the build again to see if merging main helped :)

I'm following the builds, iOS seems promising now but Android already failed.
I'm able to reproduce the Android build error if I run the same Gradle task Fastlane is running, assembleInternalRelease.
> Task :app:createBundleInternalReleaseJsAndAssets FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':app:createBundleInternalReleaseJsAndAssets'.
> Couldn't determine Hermesc location. Please set `react.hermesCommand` to the path of the hermesc binary file. node_modules/react-native/sdks/hermesc/%OS-BIN%/hermesc

Since it's happening locally I can investigate more easily.

:test_tube::test_tube: Use the links below to test this build in android and iOS. Happy testing! :test_tube::test_tube:

android :robot:
iOS :apple:

❌ FAILED ❌
https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/18507/index.html

The QR code can't be generated, because the android build failed

desktop :computer:
web :spider_web:

https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/18507/NewExpensify.dmg
https://18507.pr-testing.expensify.com

Great, thanks!

:test_tube::test_tube: Use the links below to test this build in android and iOS. Happy testing! :test_tube::test_tube:

android :robot:
iOS :apple:

https://ad-hoc-expensify-cash.s3.amazonaws.com/android/18507/index.html
❌ FAILED ❌

The QR code can't be generated, because the iOS build failed

desktop :computer:
web :spider_web:

https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/18507/NewExpensify.dmg
https://18507.pr-testing.expensify.com

Triggered build here

Hi @roryabraham , iOS build failed with this error:

It's not the first time I saw this one, is this something that happens randomly?

@fabioh8010 can you merge main to resolve conflicts before we try again?

:test_tube::test_tube: Use the links below to test this build in android and iOS. Happy testing! :test_tube::test_tube:

android :robot:
iOS :apple:

https://ad-hoc-expensify-cash.s3.amazonaws.com/android/18507/index.html
❌ FAILED ❌

The QR code can't be generated, because the iOS build failed

desktop :computer:
web :spider_web:

https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/18507/NewExpensify.dmg
https://18507.pr-testing.expensify.com

@roryabraham Done!

https://github.com/Expensify/App/actions/runs/5147752902

@roryabraham I keep getting this error during cocoapods step

I'm not having this problem locally, which makes me thinking if it's a cache issue. Is there a way to trigger a build and clear the cache?

:test_tube::test_tube: Use the links below to test this build in android and iOS. Happy testing! :test_tube::test_tube:

android :robot:
iOS :apple:

https://ad-hoc-expensify-cash.s3.amazonaws.com/android/18507/index.html
❌ FAILED ❌

The QR code can't be generated, because the iOS build failed

desktop :computer:
web :spider_web:

https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/18507/NewExpensify.dmg
https://18507.pr-testing.expensify.com

:test_tube::test_tube: Use the links below to test this build in android and iOS. Happy testing! :test_tube::test_tube:

android :robot:
iOS :apple:

https://ad-hoc-expensify-cash.s3.amazonaws.com/android/18507/index.html
https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/18507/index.html

desktop :computer:
web :spider_web:

https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/18507/NewExpensify.dmg
https://18507.pr-testing.expensify.com

@mountiny We have working builds now 🎉 We could start assigning a tester to test the RN update, wdyt?

I'm not sure if we should really test this fully yet because we need to be using our fork instead of @fabioh8010's in production.

@roryabraham My fork is based on Expensify's one with just some minor fixes to make the release work properly, but I'm bringing the discussion to our thread again so we can get this resolved more quickly!

@roryabraham in case we would like to drop our RN fork, should we close this Pr and create a new fresh PR so there is not that much clutter here?

I think we have consensus to drop our RN fork, which means that we can just upgrade to an upstream release - either https://github.com/facebook/react-native/releases/tag/v0.72.0-rc.5 or a newer nightly build

@fabioh8010 https://github.com/facebook/react-native/releases/tag/v0.72.0-rc.6 has landed, let's pick this PR up and try to get it over the finish line with the upstream dependency instead of our fork. Thanks 🚀

Update

Updated app to use RN 0.72.0-rc.6
Having build issues with Android, iOS and Desktop. No solutions for now, will investigate the root cause of the problems below.

Android
FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':tasks'.
> Could not create task ':oguzhnatly_react-native-image-manipulator:compileDebugJavaWithJavac'.
   > In order to compile Java 9+ source, please set compileSdkVersion to 30 or above

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 4m 28s

iOS
error: Error: Unable to resolve module ./LogBoxImages/close.png from /Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/node_modules/react-native/Libraries/LogBox/UI/LogBoxNotification.js:

None of these files exist:
  * node_modules/react-native/Libraries/LogBox/UI/LogBoxImages/close.png(.native|.ios.jsx|.native.jsx|.jsx|.ios.js|.native.js|.js|.ios.ts|.native.ts|.ts|.ios.tsx|.native.tsx|.tsx|.ios.json|.native.json|.json|.ios.svg|.native.svg|.svg)
  * node_modules/react-native/Libraries/LogBox/UI/LogBoxImages/close.png/index(.native|.ios.jsx|.native.jsx|.jsx|.ios.js|.native.js|.js|.ios.ts|.native.ts|.ts|.ios.tsx|.native.tsx|.tsx|.ios.json|.native.json|.json|.ios.svg|.native.svg|.svg)
  105 |         style={dismissStyles.press}>
  106 |         <Image
> 107 |           source={require('./LogBoxImages/close.png')}
      |                            ^
  108 |           style={dismissStyles.image}
  109 |         />
  110 |       </LogBoxButton>
    at ModuleResolver.resolveDependency (/Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/node_modules/@react-native-community/cli-plugin-metro/node_modules/metro/src/node-haste/DependencyGraph/ModuleResolution.js:114:15)
    at DependencyGraph.resolveDependency (/Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/node_modules/@react-native-community/cli-plugin-metro/node_modules/metro/src/node-haste/DependencyGraph.js:277:43)
    at Object.resolve (/Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/node_modules/@react-native-community/cli-plugin-metro/node_modules/metro/src/lib/transformHelpers.js:169:21)
    at Graph._resolveDependencies (/Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/node_modules/@react-native-community/cli-plugin-metro/node_modules/metro/src/DeltaBundler/Graph.js:472:35)
    at Graph._processModule (/Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/node_modules/@react-native-community/cli-plugin-metro/node_modules/metro/src/DeltaBundler/Graph.js:260:38)
    at async Graph._addDependency (/Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/node_modules/@react-native-community/cli-plugin-metro/node_modules/metro/src/DeltaBundler/Graph.js:371:20)
    at async Promise.all (index 5)
    at async Graph._processModule (/Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/node_modules/@react-native-community/cli-plugin-metro/node_modules/metro/src/DeltaBundler/Graph.js:321:5)
    at async Graph._addDependency (/Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/node_modules/@react-native-community/cli-plugin-metro/node_modules/metro/src/DeltaBundler/Graph.js:371:20)
    at async Promise.all (index 14)

Desktop
[Electron] App threw an error during load
[Electron] Error: Cannot find module 'electron-serve'
[Electron] Require stack:
[Electron] - /Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/desktop/dist/main.js
[Electron] - /Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/desktop/dev.js
[Electron] - /Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/node_modules/electron/dist/Electron.app/Contents/Resources/default_app.asar/main.js
[Electron] -
[Electron]     at Module._resolveFilename (node:internal/modules/cjs/loader:963:15)
[Electron]     at n._resolveFilename (node:electron/js2c/browser_init:2:109751)
[Electron]     at Module._load (/Users/fabiohenriques/.npm/_npx/a9286e5dc762839a/node_modules/runtime-required/runtime-required.js:28:44)
[Electron]     at Module.require (node:internal/modules/cjs/loader:1035:19)
[Electron]     at require (node:internal/modules/cjs/helpers:102:18)
[Electron]     at electron-serve (/Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/desktop/dist/main.js:4167:18)
[Electron]     at __webpack_require__ (/Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/desktop/dist/main.js:8292:42)
[Electron]     at eval (webpack://new.expensify/./desktop/main.js?:3:15)
[Electron]     at ./desktop/main.js (/Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/desktop/dist/main.js:38:1)
[Electron]     at __webpack_require__ (/Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/desktop/dist/main.js:8292:42)

RN 0.72.0 is out 🎉 I will update the PR to target it!

@fabioh8010 Could we also bump these other packages https://github.com/Expensify/App/issues/21308

Agree with @parasharrajat here, let's focus on upgrading just RN here and handle Reanimated, GH, and Screens in separate PR(s).

@roryabraham @parasharrajat Just FYI that in this PR I'm already bumping Reanimated to 3.3.0, so I don't think it will be troublesome to bumping the others too but it's up to you.
I'm still investigating the problems above (coming to a solution about iOS problem).

@fabioh8010 I didn't realize the new package versions introduce support for RN 0.72, so I take it back and think it's fine to include here.

Update

Android, iOS and Web builds are working ✅
Desktop still failing ❌
Created a patch file for @oguzhnatly/react-native-image-manipulator. Looks like we are using our own fork here.

Desktop error:
[Electron] App threw an error during load
[Electron] Error: Cannot find module 'electron-serve'
[Electron] Require stack:
[Electron] - /Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/desktop/dist/main.js
[Electron] - /Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/desktop/dev.js
[Electron] - /Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/node_modules/electron/dist/Electron.app/Contents/Resources/default_app.asar/main.js
[Electron] -
[Electron]     at Module._resolveFilename (node:internal/modules/cjs/loader:963:15)
[Electron]     at n._resolveFilename (node:electron/js2c/browser_init:2:109751)
[Electron]     at Module._load (/Users/fabiohenriques/.npm/_npx/a9286e5dc762839a/node_modules/runtime-required/runtime-required.js:28:44)
[Electron]     at Module.require (node:internal/modules/cjs/loader:1035:19)
[Electron]     at require (node:internal/modules/cjs/helpers:102:18)
[Electron]     at electron-serve (/Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/desktop/dist/main.js:4167:18)
[Electron]     at __webpack_require__ (/Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/desktop/dist/main.js:8292:42)
[Electron]     at eval (webpack://new.expensify/./desktop/main.js?:3:15)
[Electron]     at ./desktop/main.js (/Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/desktop/dist/main.js:38:1)
[Electron]     at __webpack_require__ (/Users/fabiohenriques/Documents/Work/Callstack/Projects/Expensify/expensify-app/desktop/dist/main.js:8292:42)

@fabioh8010 Electron build worked fine on this branch for me.  Strangely, node_modules was not present in that folder and I had to install it manually like you said. Is it something that we have to do manually or should it install together when running npm install on the root folder?

Update

All builds are stable ✅
Documented the patches and PRs in the issue description.
Addressed some comments here.

@mountiny @roryabraham I think we are good to go with this PR now. Please let me know if you need more testing from my side, or a specific one.

Is it something that we have to do manually or should it install together when running npm install on the root folder? I'm asking this because I don't remember having to do it before.

It should happen in a postInstall script

@fabioh8010 there's an unrelated test failure that was present on main. Can you merge main again to resolve that please?

Hi @parasharrajat Yes, I saw this warning as well and I was able to reproduce it on main on Friday. I will test it again.

Makes sense @ntdiary , this is probably happening on main as well because this patch is also there, @parasharrajat could you please confirm this?

Question: Do you see opening or closing animation for the FAB menu on iOS and Android?

Testing..

@parasharrajat I tested the FAB button on both main and this branch and there is no difference in my machine.

"main" branch - iOS
https://github.com/Expensify/App/assets/20051562/9df56ac7-e923-4fa3-95fd-183852fec3dc

"main" branch - iOS

@parasharrajat I tested the FAB button on both main and this branch and there is no difference in my machine.

"main" branch - Android
https://github.com/Expensify/App/assets/20051562/9df56ac7-e923-4fa3-95fd-183852fec3dc

"main" branch - iOS
https://github.com/Expensify/App/assets/20051562/a86cfdfa-fb85-4d2c-9c8b-ac44314e55e6

this branch - Android
https://github.com/Expensify/App/assets/20051562/d8b3cf5e-969b-4765-81d4-b51b9f2912c9

this branch - iOS
https://github.com/Expensify/App/assets/20051562/7543a0cc-be5a-44e6-8ad1-05fbdfc47a99

Besides that, I've updated this branch against main again, please feel free to continue testing.

@parasharrajat Please let me know if you need more help from my side.

Good call, I thought we have already managed to get all our fork changes to the upstream @chrispader would you be able to have a look at this?

Good call, I thought we have already managed to get all our fork changes to the upstream @chrispader would you be able to have a look at this?

@mountiny @aimane-chnaif I recalled about a spreadsheet with the status of each PR merged in Expensify's fork that was shared in the channel -> https://docs.google.com/spreadsheets/d/1Fvc2x0ythHoqE1NnsP5JTOLC0eXbWV5IrA-37hbJDGc/edit#gid=1594942597
Tomorrow I can take a look in each PR and see if's actually available on the last release.

If this would require a patch while we will wait for the fix to hit upstream, I think that would be better than holding this update.

It's worth mentioning that it won't be possible to use patches to fix native issues in Android, as the Android sources are now shipped in pre-built artifacts. cc @chrispader

Looking through the spreadsheet and changelog:
🟢 Merged and available on Upstream RN.
🟡 Merged but not available on latest RN release yet.
🟠 Not sure about status on Upstream RN.
🔴 No Upstream PR or not merged on Upstream RN yet.

🟡 https://github.com/Expensify/react-native/pull/53: Merged on our Fork. Merged on Upstream but not available in the latest release yet.
🔴 https://github.com/Expensify/react-native/pull/49: Merged on our Fork. Still open on Upstream.
🟡 https://github.com/Expensify/react-native/pull/46: Merged on our Fork. I don't know if this one is already on Upstream, but this one is available since 0.72.0. Could you confirm @janicduplessis?
🔴 https://github.com/Expensify/react-native/pull/45: Merged on our Fork. Still open on Upstream.
🟡 https://github.com/Expensify/react-native/pull/40: Merged on our Fork. Merged on Upstream but not available in the latest release yet.
🔴 https://github.com/Expensify/react-native/pull/37: Merged on our Fork. Still open on Upstream.
🟢 https://github.com/Expensify/react-native/pull/36: Merged on our Fork. Merged on Upstream and available since 0.72.0.
🟠 https://github.com/Expensify/react-native/pull/33: Merged on our Fork. It seems that there is no Upstream PR. Could you confirm @hannojg?
🟠 https://github.com/Expensify/react-native/pull/32: Merged on our Fork. It seems that there is no Upstream PR for Fabric support. Could you confirm @janicduplessis?
🟢 https://github.com/Expensify/react-native/pull/24: Merged on our Fork. Merged on Upstream and available since 0.70.1.
🟢 https://github.com/Expensify/react-native/pull/21: Merged on our Fork. It's just examples for RNTester.
🟡 https://github.com/Expensify/react-native/pull/18: Merged on our Fork. I think this is included in https://github.com/Expensify/react-native/pull/36. Could you confirm @janicduplessis?
🟡 https://github.com/Expensify/react-native/pull/17: Merged on our Fork. I think this is included in https://github.com/Expensify/react-native/pull/36. Could you confirm @janicduplessis?
🟠 https://github.com/Expensify/react-native/pull/16: Merged on our Fork. It seems that there is no Upstream PR. Could you confirm @janicduplessis?
🟠 https://github.com/Expensify/react-native/pull/11: Merged on our Fork. It seems that there is no Upstream PR. Could you confirm @lbaldy?
🟡 https://github.com/Expensify/react-native/pull/8: Merged on our Fork. I think this is included in https://github.com/Expensify/react-native/pull/36. Could you confirm @janicduplessis?

cc @mountiny

Thanks @parasharrajat , updated the list.

https://github.com/Expensify/react-native/pull/33: Merged on our Fork. It seems that there is no Upstream PR. Could you confirm @hannojg?

This PR is for a fix on android to circumvent a ANR, we are working to get these changes into react native core.
The tracking PRs can be found here:

https://github.com/facebook/react-native/pull/38071

https://github.com/facebook/react-native/pull/38073

Until they are merged I recommend making the verticalScrollbarPosition part of a patch for react native (its a relatively small and none invasive change I'd say)

@fabioh8010 All fixes related to maintainVisibleContentPosition have been upstreamed and released in 0.72 except for https://github.com/Expensify/react-native/pull/16.
I will work on getting https://github.com/Expensify/react-native/pull/16 upstreamed but I don't think this is a blocker as we don't use this functionality currently.

Ah I swear I remember someone mentioning that most of our stuff should be merged and release already so probably they did not have this spreadsheet. Thanks!
Which one of these do we need to block on if any. Can we use the patch for what @hannojg mentioned?

@fabioh8010 All fixes related to maintainVisibleContentPosition have been upstreamed and released in 0.72 except for https://github.com/Expensify/react-native/pull/16.
I will work on getting https://github.com/Expensify/react-native/pull/16 upstreamed but I don't think this is a blocker as we don't use this functionality currently.

Thanks for the info @janicduplessis! Regarding to https://github.com/Expensify/react-native/pull/46, could you please confirm if this commit got merged on Upstream or is part of some fix? I was unable to find it in the source code and changelog.

Ah I swear I remember someone mentioning that most of our stuff should be merged and release already so probably they did not have this spreadsheet. Thanks!

@mountiny I got the spreadsheet from this message on Slack, I think we just didn't have enough visibility of the status of the PRs without doing a deeper investigation.

Which one of these do we need to block on if any. Can we use the patch for what @hannojg mentioned?

@hannojg From what I understood we would have to patch Android source files to apply the fix, right? If it's the case, it's not possible because the android sources are shipped pre-built during build process, so making a patch won't make difference.
@mountiny I updated the list with ❌ (possible blocker) and 🔨 (can be patched) labels. Currently, it seems to me that we have two possible blockers, https://github.com/Expensify/react-native/pull/37 and https://github.com/Expensify/react-native/pull/33. @aimane-chnaif @parasharrajat could you confirm if I'm missing something?

As we're using RN directly, android bugs from our previously forked RN are gone:
https://github.com/Expensify/App/issues/17368 (fixed already but was workaround, all numberOfLines={100} occurrences can be safely removed)
[HOLD][$1000] Web- Preferences - The recent mode description is truncated in Spanish preference  #17509 (to fix, revert alternateTextMaxLines back to 0 from undefined)
https://github.com/Expensify/App/issues/17441 (fixed)

@aimane-chnaif Just to clarify, I have to apply those 2 changes you suggested in this branch, right? all numberOfLines={100} occurrences can be safely removed and to fix, revert alternateTextMaxLines back to 0 from undefined.

@fabioh8010 Yes, these are native changes, however its a change on the java files, not on the CPP files. The pre-shipped binaries are only for the native libs afaik.
So that shouldn't be an issue 😊

Hmm I think it also applies to Java files 🤔 , will test @hannojg and let you know, thanks.

It's worth mentioning that it won't be possible to use patches to fix native issues in Android, as the Android sources are now shipped in pre-built artifacts. cc @chrispader

Yes, @fabioh8010 is right, we won't be able to include the maxNumberOfLines and numberOfLines feature @Szymon20000 implemented in the fork with just a patch. @mountiny
I'll check with @Szymon20000 how the progress in the upstream PR is going...

Yes @chrispader @hannojg the prebuilts also include Java files, so any fixes to native Android side can't be patched.
It would be good if it was possible to come up with workarounds on JS side for now, until the pending PRs are merged and available in a future RN release, to avoid blocking this one.

@aimane-chnaif I removed the occurrences of numberOfLines={100} and it's working as expected, but I wasn't able to reproduce the bug related to https://github.com/Expensify/App/issues/17509, could you please check?

@aimane-chnaif this branch.

Regarding to https://github.com/Expensify/react-native/pull/46, could you please confirm if this commit got merged on Upstream or is part of some fix? I was unable to find it in the source code and changelog.

@fabioh8010 It was merged as part of https://github.com/facebook/react-native/pull/35993

@aimane-chnaif It turns out that PriorityModePage is using another component now, SelectionListRadio. This component list items don't use this alternateTextMaxLines prop, so we can safely remove alternateTextMaxLines from PriorityModePage.

@fabioh8010 Kindly disagreeing here, the react-native core files are shipped as source with the rpm react-native package (and it will stay this way).
You can verify yourself by checking e.g. node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/

FYI - Excited for this one so I kicked off another AdHoc build

:test_tube::test_tube: Use the links below to test this build in android and iOS. Happy testing! :test_tube::test_tube:

android :robot:
iOS :apple:

https://ad-hoc-expensify-cash.s3.amazonaws.com/android/18507/index.html
https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/18507/index.html

desktop :computer:
web :spider_web:

https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/18507/NewExpensify.dmg
https://18507.pr-testing.expensify.com

🔴 / 🔨 https://github.com/Expensify/react-native/pull/16: Merged on our Fork. There is no Upstream PR yet, @janicduplessis will open one but it seems that is a functionality that is not used yet in the project.

Upstream PR opened https://github.com/facebook/react-native/pull/38245

@fabioh8010 Kindly disagreeing here, the react-native core files are shipped as source with the npm react-native package (and it will stay this way).
You can verify yourself by checking e.g. node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/

@hannojg Yes, there are indeed Java files in that folder but modifying them won't make any difference when building the app (I just tested here as well), as stated in this guide we will be consuming precompiled artifacts when building for Android.
However, I also discovered that we can configure the project to build from source and by doing that it will be possible to apply changes to those files. I was able to apply your fix by configuring the project to build from source (the scroll indicator is on the correct side now):
https://github.com/Expensify/App/assets/20051562/b62cec7b-ba0c-43dc-b59b-1aad2db1a3a0
@roryabraham @mountiny  WDYT about this? In this way we can patch the two fixes https://github.com/Expensify/react-native/pull/37 and https://github.com/Expensify/react-native/pull/33. The first disadvantage that I see is that the patch file will be gigantic, but it would allows us to move forward with this PR.

@fabioh8010 building from source sure is a bummer because of increased build times, but it's an acceptable cost since we seem to be using those upstream features.

Actually, I just remembered that there is a canary release available for multiple patches published from this PR branch: https://github.com/ds300/patch-package/pull/474
So I'd suggest we start using it right away rather than creating a monolithic patch. Any feedback would be very helpful.
You can do npm install patch-package@canary to try out the --append flag. I think the workflow will be like so:

Make the changes from https://github.com/Expensify/react-native/pull/33, then run npx patch-package --append 'VerticalScrollBarPosition'
Make the changes from https://github.com/Expensify/react-native/pull/37, then run npx patch-package --append 'NumberOfLines'

Actually, just re-reviewing these PRs more carefully:

https://github.com/facebook/react-native/pull/35703 – this adds numberOfLines and maximumNumberOfLines support for TextInput, TextArea, EditText components. However, I looked through all usages of numberOfLines in our repo, and they are all on standard Text components. So I think this may currently be unused.
https://github.com/facebook/react-native/pull/38073 – I don't think we need this either, since we already have another workaround in place: https://github.com/Expensify/App/blob/b8aeec0bcf2cac06d51129f287f932ba0421ed4e/src/components/InvertedFlatList/index.android.js#L21

So it looks like neither of these are true blockers and we don't need to use the patches after all. Sorry, I just got excited about our new tool there... but I'm even more excited about not having to build Android from source.

@roryabraham Just to clarify better the issues:

https://github.com/facebook/react-native/pull/35703
We have some usages of maximumNumberOfLines in the Composer component for native platforms. Without this PR the composer will grow infinitely as reported by @aimane-chnaif.

https://github.com/Expensify/App/assets/20051562/d431b075-f5b6-47c5-bc4b-5cac9d4bfca3

https://github.com/facebook/react-native/pull/38073
Without this PR the vertical scroll indicator will be on the wrong side on Android.

https://github.com/Expensify/App/assets/20051562/32f24266-2758-4b08-901e-fe27c362fa42
If we are OK with having this issues while they are not available in Upstream RN yet, we can safely merge this PR 👍

[TextInput] Add numberOfLines and maximumNumberOfLines props on iOS and fix Android implementation  facebook/react-native#35703
We have some usages of maximumNumberOfLines in the Composer component for native platforms. Without this PR the composer will grow infinitely as reported by @aimane-chnaif.

If we are OK with having this issues while they are not available in Upstream RN yet, we can safely merge this PR 👍

Yes exactly, we're using maximumNumberOfLines in the Composer implementation in both android and ios

Ah, sorry I missed those things. I guess let's go back to the patch-package plan then.

@fabioh8010 Should i prepare the patch file for the maxNumberOfLines changes for you?
This means we will need to build android from source though, if that's okay with you guys?

@chrispader Yes, we agreed with proceeding with building from source to be able to include those patches temporally. No need to prepare, I'm already handling that as we are going to use experimental feature of patch-package, so it's best to do from my side, thanks!

@parasharrajat In my machine:

Normal Android build -> 4-5 min
Building from source, first time -> 12-15 min
Building from source, subsequent times -> 4-5 min

The first build will take more time as it needs to compile sources etc, but the other ones looked the same for me. Probably it will have more impact on CI.

Tests are failing... will investigate.

Tests were failing because of new code added in @Szymon20000's PR: https://github.com/facebook/react-native/pull/35703
It was only failing in test environment, so I added this mock to prevent it. @Szymon20000 not sure if it's something specific for Expensify, but maybe it's worth taking a look at this.

@parasharrajat Updated the list with the two PRs that were patched.

That pr is a bit outdated as the main focus it now on iOS.
The pr was too big for reviewers and they wanted me to split it.
We would need to cherry-pick some latest commits from this pr https://github.com/facebook/react-native/pull/38021/commits to fix the tests.

I probably can do it tomorrow.

@Szymon20000 Could you confirm which commits do I have to consider to apply in our patch? I think they are these ones but I want to confirm with you.

@fabioh8010 Exactly those. Would be great if you can cherry-pick it. I'm super busy today and going on vacation tomorrow :). If you also don't have time I will do my best to do it later today.

@Szymon20000 Sure! I will do that!

I've updated the patch to include this changes and removed the Jest mock workaround.
cc @roryabraham

Kicked off a new test build

:test_tube::test_tube: Use the links below to test this build in android and iOS. Happy testing! :test_tube::test_tube:

android :robot:
iOS :apple:

https://ad-hoc-expensify-cash.s3.amazonaws.com/android/18507/index.html
https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/18507/index.html

desktop :computer:
web :spider_web:

https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/18507/NewExpensify.dmg
https://18507.pr-testing.expensify.com

@parasharrajat @aimane-chnaif can you review this one again please?

Asking @robertKozik and @ArekChr for reviews and testing too

@aimane-chnaif @parasharrajat I've updated the branch against main.
I was able to reproduce the first issue on main as well.

Not sure if it's related to this PR, but there is no header when opening replies. Shows [Deleted message]

Production:

The same error happens on all platforms.

The PR is still missing this commit from main, if I am not mistaken which should address the issue mentioned by @ArekChr
https://github.com/Expensify/App/commit/b602c3d3b372a59fc4e251b3dd26dddd818a97d9

On the left latest main/staging, on the right this PR. I'm not sure which one is intended, but the left align on the multiline name seems odd

First issue is workspaces list, not LHN

@aimane-chnaif It's also happening on workspace list on main.

The PR is still missing this commit from main, if I am not mistaken which should address the issue mentioned by @ArekChr

Yes I've merged with main and it's fixed now in this PR.

On the left latest main/staging, on the right this PR. I'm not sure which one is intended, but the left align on the multiline name seems odd

I'm able to reproduce this problem in several places of the app as well, in iOS. It's probably related to the number of lines PR I have patched here. I will re-check the patch file to see if everything is correct.

@robertKozik @ArekChr Any chance you could give this PR another test to see if there are any other issues we should address? It would be great to get this out.

@robertKozik @ArekChr Any chance you could give this PR another test to see if there are any other issues we should address? It would be great to get this out.

Yes, working on that

@ArekChr Yeah it's broken, I'm checking that 👍

The multiline header does not fit in the reply screen.

trigerring the build

:test_tube::test_tube: Use the links below to test this build in android and iOS. Happy testing! :test_tube::test_tube:

android :robot:
iOS :apple:

https://ad-hoc-expensify-cash.s3.amazonaws.com/android/18507/index.html
https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/18507/index.html

desktop :computer:
web :spider_web:

https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/18507/NewExpensify.dmg
https://18507.pr-testing.expensify.com

On IOS overall looks good to me except for truncating and multiline issues.
I started working on Android. Tomorrow I will post an update if I find something.

I've updated the PR with main again and after re-checking multiple times the NumberOfLines PR, I noticed that there is a specific code that is causing issues in all Text components.
@Szymon20000 This block is causing all Text components to lose its ability to restrict the number of lines, so I changed my patch file to comment this part and always return { maximumNumberOfLines: numberOfLinesValue }. It seems something related to CONTAINS_MAX_NUMBER_OF_LINES_RENAME flag, but I'm struggling to understand what's its purpose there.

On IOS overall looks good to me except for truncating and multiline issues.

This was fixed yesterday. Just this branch doesn't have that commit.

@ArekChr @aimane-chnaif Both issues are fixed now.

Hey @fabioh8010 !
Fb reviewers wanted me to add it as they first update js code then native code. This flag is checking if the code contains native changes.

@Szymon20000 So, can I keep with the workaround I made?

@aimane-chnaif Could you check if you have the NDK version 23.1.7779620 installed in you Android Studio?

@fabioh8010 yes that should work fine.

I've updated the PR with main again to resolve the merge conflict.
I noticed that in both this branch and main, I'm not being able to log in on Android, the loading keeps spinning forever.
cc @roryabraham

Running a new build here https://github.com/Expensify/App/actions/runs/5668615955

:test_tube::test_tube: Use the links below to test this build in android and iOS. Happy testing! :test_tube::test_tube:

android :robot:
iOS :apple:

https://ad-hoc-expensify-cash.s3.amazonaws.com/android/18507/index.html
https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/18507/index.html

desktop :computer:
web :spider_web:

https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/18507/NewExpensify.dmg
https://18507.pr-testing.expensify.com

Yeah, that fixes this issue.
Otherwise didn't find any different issues except one regression which happened on the main as well.
Also didn't find any truncating issues on Android, LGTM 🙌

Hey @marcaaron :wave: I was updating this branch against main to resolve the merge conflicts and I noticed that react-native-reanimated was reverted to 3.1.0 and a patch file was created as well. Could you give me some context why was this necessary?
Asking this because for RN 0.72 we should use at least 3.3.0 according to this issue.

@aimane-chnaif Thanks for the info! So, it seems that proposal is approved will be done by the contributor, right? Once the component is migrated we can continue.

Assigned the contributor and bumped them

Waiting for https://github.com/Expensify/App/pull/23854 to be merged so we can update this branch against main again.

Since https://github.com/Expensify/App/pull/23854 was merged I've updated this PR against main again.
Could we trigger a build?
cc @mountiny @roryabraham @parasharrajat @aimane-chnaif

triggered the build

:test_tube::test_tube: Use the links below to test this build in android and iOS. Happy testing! :test_tube::test_tube:

android :robot:
iOS :apple:

https://ad-hoc-expensify-cash.s3.amazonaws.com/android/18507/index.html
https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/18507/index.html

desktop :computer:
web :spider_web:

https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/18507/NewExpensify.dmg
https://18507.pr-testing.expensify.com

Update: There are merge conflicts again, but let's wait until https://github.com/Expensify/App/pull/24036 is merged because we are having a crash in the app.
Context -> https://github.com/Expensify/App/pull/20179#issuecomment-1661550145 and https://expensify.slack.com/archives/C049HHMV9SM/p1690954042805659
cc @mountiny @roryabraham

@parasharrajat I've updated against main! cc @mountiny

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

🚀 Deployed to staging by https://github.com/luacmartins in version: 1.3.50-0 🚀

platform
result

🤖 android 🤖
success ✅

🖥 desktop 🖥
success ✅

🍎 iOS 🍎
success ✅

🕸 web 🕸
success ✅

🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.50-3 🚀

platform
result

🤖 android 🤖
success ✅

🖥 desktop 🖥
success ✅

🍎 iOS 🍎
success ✅

🕸 web 🕸
success ✅

🚀 Deployed to staging by https://github.com/luacmartins in version: 1.3.51-0 🚀

platform
result

🤖 android 🤖
success ✅

🖥 desktop 🖥
success ✅

🍎 iOS 🍎
success ✅

🕸 web 🕸
success ✅


CLI github success message should be consistent with twitter CLI

Even this issue has been taken care of, we can close it


Unable to removeChild successfully.

Hello, I've been running into key issue when using your library (using L4). When I run removeChild(0) method, it fails to delete the values in my many-to-many table as well as my main closure table. A foreign key constraint is causing this to happen, but I'm using the schema generated from the migration packaged with your software. I even tried removing the foreign keys, and the relationships still weren't removed. Can you shed some light on this issue? Should I be handling the detach on my own, or am I overlooking something?
Everything else seems to be working as intended.

I just tried the same thing and it worked without an issue for me (I'm using the L5 version, but nothing has changed with that between the 2 versions).
It deleted the child and removed the rows from the closure table.
Would it be possible to get a small test case created? Just an empty Laravel site with some seeding that shows the issue?

Just as I suspected, it was something on my end. The model I was using needed to use the SoftDelete trait. Thanks for timely response.


update comments, readme

Coverage decreased (-0.08%) to 95.349% when pulling 955ef43bdeb4a986804077882582725c62ff3c2a on ryan-williams:docs into<PHONE_NUMBER>aa80aca53e74125eb7a6ada2b8b506 on hammerlab:master.


INSTALL.md: clarify macOS prerequisites

I didn't realize I could just go ahead and run bazel build :urbit without first adapting one of the bazel run //toolchain... prerequisite commands first.

Nit: '%s/MacOS/macOS/g'

@mcevoypeter PTAL


emoji colors

If you follow an emoji up with a skin tone code it does not change the color of the emoji but instead displays both as individual emojis.

Fixed in 3cdaeb071aadde9389d7e416b1f8e89c4511c434


Assert: "Unknown hash type"

With newly synced/built PerfView, and https://github.com/dotnet/coreclr, I'm doing a PerfView collect->Run using:
c:\gh\coreclr\bin\tests\Windows_NT.x86.Release\Tests\Core_Root\superpmi.exe c:\spmi\x86\alltests_win_minopts.mch c:\gh\coreclr\bin\tests\Windows_NT.x86.Release\Tests\Core_Root\clrjit.dll

(This requires a alltests_win_minopts.mch that I have built)
In the CPU stacks view, I choose the top Exc% line and hit ALT-D (go to source), and then get this assert:
        unsafe internal SourceFile(SymbolModule module, IDiaSourceFile sourceFile)
        {
            m_symbolModule = module;
            BuildTimeFilePath = sourceFile.fileName;

            // 0 No checksum present.
            // 1 CALG_MD5 checksum generated with the MD5 hashing algorithm.
            // 2 CALG_SHA1 checksum generated with the SHA1 hashing algorithm.
            m_hashType = sourceFile.checksumType;
            if (m_hashType != 2 && m_hashType != 1 && m_hashType != 0)
            {
                Debug.Assert(false, "Unknown hash type");
                m_hashType = 0;
            }

in this case, m_hashType is 3.
Has something changed? Namely, is there a new hash type that needs support?
Is there a workaround for using PerfView without this, or forcing a different hash type?

cc @sharwell @vancem

I found the issue (talked to a VC++ dev) and have PR'ed a fix.


Retrieving Data

On my pull request, I added a JSON feature, but I saw in the object syntax, there's the data attribute. However, that's only for sending. Is there something that the user has to set to specify the data from the XML/JSON? Like if I wanted to get/parse the data from this .json file.
{
  "txt": "This is an example of using JSON!"
}
It may be easier to answer if there's only one string or data type, but when there's more the request will have to be more detailed.

minAjax accepts the JSON as the default object  , however you can parse XML and convert it to JSON before invoking minAjax. Idea is to keep library as minimal as possible. I hope this answers your question.

Okay, I see. I could go ahead and make that clear on my (or the) README.
Before I close the issue and do that, though, did that require anything to make it accept JSON by default? Because as far as I know, the parse() is what actually makes it usable in a script.


Improve doc based on feedback

Looks like Procrastinate was featured on the orange website.
Let's browse the answers and see if there is interesting feedback to gather:

Things we code do better
Things we could explain better in the docs

Ping @noodlesuk: if you're the OP from HN, thanks :)

I believe we could at least take a look at the alternative libs mentionned and add them to our list in the discussion page.

Link to discussion: https://news.ycombinator.com/item?id=30126152


Beacon's Measured Power as an argument

Took Measured Power as an argument from the user instead of generating automatically. Also checked whether the value is as per BLE specifications.
Modified btshell app to use the new argument while setting eddystone beacon.

BTW after the commit - f7efcd3, the checks are failing because of an issue in travis.


Upgrade deprecated CDK property used by API Gateway patterns

I was using aws-cloudfront-apigateway-lambda pattern, and I found the default authorization type for API Gateway is IAM. I needed to change that one to NONE, but I saw that the pattern is using deprecated CDK property options.
<EMAIL_ADDRESS>Use Case

options property is still available, but since it's deprecated property, it would be better to replace options to other live properties.
Proposed Solution

Since every options properties are available in LambdaRestApiProps, it would be easy to use LambdaRestApiProps itself instead of options.
<EMAIL_ADDRESS>Other

These are source code where options are used.
https://github.com/awslabs/aws-solutions-constructs/blob/ccf819c99a907634cc78de26cca07d77eb748656/source/patterns/%40aws-solutions-constructs/core/lib/apigateway-defaults.ts#L50-L56
https://github.com/awslabs/aws-solutions-constructs/blob/ccf819c99a907634cc78de26cca07d77eb748656/source/patterns/%40aws-solutions-constructs/core/lib/apigateway-defaults.ts#L64-L70

[ ] :wave: I may be able to implement this feature request
[x] :warning: This feature might incur a breaking change

This is a :rocket: Feature Request

This has been addressed in the latest release v1.54.0


Print flattens arrays

evaluate (e.g. in the REPL):
duk> [[1,2],[3,4,[5,6]]];
= 1,2,3,4,5,6
Why does duktape flatten arrays on output? This seems confusing.

It doesn't actually flatten arrays, but string coerces them. String coercion of an array (in any engine) is comma joined list of string coerced elements:
duk> a = [1,2,3,[4,5]]; String(a)
= 1,2,3,4,5

By the way, there's a "console" provider in the Duktape master (extras/console) which JX-coerces e.g. console.log() arguments rather than just string coercing them:
$ ./duk.O2
((o) Duktape [linenoise] 1.99.99 (v1.5.0-438-ga35d09e-dirty)
duk> console.log([1,2,3,[4,5]])
[1,2,3,[4,5]]
duk> console.log({foo:[1,2,3,[4,5]]})
{foo:[1,2,3,[4,5]]}

While print() just prints strings and string coercion is (at least arguably) appropriate for it, console.log() and related calls are intended for debug logging so that using serialization makes maybe more sense for them.

To illustrate what @svaarala is saying, output from Node.js when explicitly calling .toString() on the array:
C:\Users\fatce>node
> [1,2,3,[4,5]].toString()
'1,2,3,4,5'
>

Of course Node will show the real structure of the array if you omit the .toString() bit as it uses a custom JSON-like provider for that.  Maybe duk could do something similar, e.g. JX encoding the result of expressions?

@fatcerberus At least for me a print() binding would naturally string encode its arguments. It's also what the print() browser bindings do, e.g. on Firefox:
print([1,2,3,[4,5]])
"1,2,3,4,5"

The console.log() and related bindings are intended to dump object structure, and the extras/console implementation does that using JX, falling back to string coercion if JX encoding fails (e.g. due to a loop).

I agree, string coercion makes the most sense for print().  I was referring specifically to duk's output - it might make more sense for the command-line tool to use a more verbose format for the result of expressions, like Node does.

Ah, I see what you mean now. I agree with that, maybe duk could essentially use the same approach as extras/console, i.e. attempt JX and fall back to String(x) if that fails.

The extras/console approach seems like it would be generally more useful than coercing to string.
For comparison, it looks like the Node console descends objects and arrays to a depth of three, then prints "[Object]" for any objects at level four. That's true for both expressions evaluated in the repl and console.log().

One simple change would be for duk to try console.log(result) and if that fails, do string coercion as before. This works if the extras/console support is enabled when compiling duk (which is the default in the dist Makefile.cmdline).

Maybe have it keep a heap pointer around for console.log in case the user does delete console.log or something at the command line. :)

True. Well, it'd also be easy to just copy paste the JX formatting from extras/console, it's just:
	duk_eval_string(ctx,
		"(function format(v){"
		    "try{"
		        "return Duktape.enc('jx',v);"
		    "}catch(e){"
		        "return ''+v;"
		    "}"
		"})");

That's actually not perfect either because it depends on Duktape.enc existing. Another closure wrapper or stash would be needed to secure that.

If Duktape.enc doesn't exist it will throw and hit the fallback in the catch.  The real problem is if Duktape.enc gets replaced, i.e. with another function.

Agree, this applies to the console extra too at present. I don't see that as a really big issue unless one is working with actively hostile code though :)

Ok, #906 has a fix for the console-extra Duktape.dec() runtime lookup. The revised code is:
	duk_eval_string(ctx,
		"(function () {"
		    "var E=Duktape.enc;"
		    "return function format(v){"
		        "try{"
		            "return E('jx',v);"
		        "}catch(e){"
		            "return ''+v;"
		        "}"
		    "};"
		"})()");

That was a quick fix. :)


Publish package to PyPI

Automate publishing to PyPI using develop and master pattern.

As a quick comment, you might be interested in the release script I use for CleverCSV and other packages to semi-automate the release process.

@GjjvdBurg This is super-useful, thanks!


HiRes fix sample

Hey @s9roll7 any chance you can share the json for the hires fix example in the readme (girl with hat)?

https://github.com/s9roll7/animatediff-cli-prompt-travel/issues/188