-
-
Notifications
You must be signed in to change notification settings - Fork 13
Replace zxing QR reader implementation with react-qr-scanner #2474
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2474 +/- ##
==========================================
+ Coverage 77.99% 78.18% +0.18%
==========================================
Files 281 281
Lines 20098 20039 -59
Branches 2029 2027 -2
==========================================
- Hits 15676 15668 -8
+ Misses 4376 4325 -51
Partials 46 46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pylipp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, I was struggling with finding the printscreen button there 😂 Looking forward to the review :) |
Signed-off-by: Philipp Metzner <10617122+pylipp@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code review passed
great, clean work @MomoRazor !
functional review
1.) The new QrReader behaves differently regarding "Re-scanning". The delayBetweenScanAttempts is different than before. Any idea why @MomoRazor ? I attached a video for this. Without a known reason it would be a blocker for me.
Screencast.2025-12-15.17.02.03.mp4
2.) Due to former bugs with IPhone, I would like an iPhone user to test before release on staging.
|
The multiple throwing of the same message would be a blocker for me atm. Screencast.2025-12-16.12.10.17.mp4PS. I hope you do not get to frustrated by this and I hoped I prepared you enough for expecting it with the QR-Reader |
|
@MomoRazor sorry for finding more problems Screencast.2025-12-16.12.22.29.mp4 |
|
I see, weirdly i was getting the delay on error as well, as intended, but maybe i missed a corner case or a different throw? I'll check it the moment I'm back on my PC :) Don't worry about frustration! It comes with the territory of React FE, and I've more than had my fair share professionally 😅. Will keep you posted once I push changes :) |
@HaGuesto the issue (switch multi-solo-multi, then scan and being directed to BoxDetails) you showed also happened to me on staging, I found out. So it might not be an issue with the new package. The final (data?) error I could not reproduce |
|
An update from my side (finally! Apologies for the delay, life's a bit hectic right now) I've managed to fix the issue with the delay not working when in error mode. However, that said, the rest of the issues are somewhat complex:
Let me know if all of this tracks, and I will post another update when I've managed to work on the missing error messages |
|
After having re-tested the error message issue this morning, I found that I'm unable to replicate it anymore, so I'm presuming that adding the dynamic key to make sure that the scanner gets re-rendered when needed might have fixed it. Let me know if anyone replicates it again. |
|
Sorry about that! Fixed it in the last commit. Also going to try and keep pushing on the related package PR so that hopefully I get it merged soon. |
|
Good news on this! Seems the PR on the related package got approved! Here's to hoping the next update of the package gets published soon! :) |
|
Glad to report that the package was updated with our PR, and this branch has been updated to point to the latest version of the package! |
Functional reviewDid a testing round with valid QR codes, those for unauthorized boxes, and non-boxtribute ones. I could not find any bugs. Especially the scanDelay works nicely. thanks @MomoRazor 🙏 Code reviewThere's some commented code that supposedly could be removed. Could you update the corresponding ADR (status: implemented, link to this PR; maybe a short sentence about the adoption of the react-qr-scanner package and the required upstream fix)? |
| const subButton = await waitFor( | ||
| () => screen.getByText(clicks[1]), | ||
| {}, | ||
| { timeout: 10000 }, | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use this part of the diff (probably overriden by merge), I had introduced it a couple of days ago in b355d73
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully I understood this properly! I just replaced all the setTimeouts in tests used to arbitrarely wait for UI changes. Tests seem to have worked :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it'd be better to use
const subButton = await waitFor(
() => screen.getByText(clicks[1]),
{},
{ timeout: 10000 },
);
than const subButton = await screen.findByText(clicks[1]); because of the explicit waitFor.
I had the impression it made the tests less flaky in CI (even if they might pass locally)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use this part of the diff (probably overriden by merge), I had introduced it a couple of days ago in b355d73
Sorry now I understood what you mean here. Not sure if you get this on your side too, however when I use this pattern, the Eslint starts to complain. Let me know if it's just me, and I'll look at my own setup in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have eslint set up locally but it was not reported in CI, otherwise the build-front job would have failed.
I know it can be really annoying to figure out the issue in one's local setup, so I don' mind if we use the findByText version 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've kept the findByText version, but added some extra expected to a test in BoxCreateView on the successfully creates a box and navigates to box details test, which seemed to be the error I was getting in CI. Let me know if the flakyness returns
@MomoRazor do you have access to an IPhone for testing? |
|
Unfortunately I don't have access to an IPhone </3 However I'll still look into ngrok to test on my android phone. |
|
|
No problem with the test merge! As for the pnpm lock issue, I presume it is a prettier issue. I'll revert that so that we don't get a bunch of changes for no reason |
|
I should have done the merges and fixed the npm-lock. Still not eniterly sure what was the problem, as double quotes in npm lock can only mean I was using an older version of pnpm somehow... anyway let me know if it looks good on your side too! |


The first implemention of the QR Reader Scanner using the primary option (@yudiel/react-qr-scanner)
https://trello.com/c/Zv00dw2j