Enable package reporting #1582
Enable package reporting #1582
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1582 +/- ##
=========================================
- Coverage 9.93% 9.91% -0.02%
=========================================
Files 303 304 +1
Lines 22378 22435 +57
Branches 399 400 +1
=========================================
+ Hits 2223 2225 +2
- Misses 20155 20210 +55 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackage.tsx
Outdated
Show resolved
Hide resolved
anttimaki
force-pushed
the
packageListing-imports
branch
from
7da8d23 to
d3c3726
Compare
anttimaki
force-pushed
the
enable-package-reporting
branch
from
cf3841c to
58bcf93
Compare
The parts regarding opening the modal are discarded as they're outdated and wouldn't work anymore.
|
|
||
| async function awaitAndSetProps() { | ||
| if (!props) { | ||
| setProps(await formProps); |
There was a problem hiding this comment.
Since the formProps here is a Promise and awaitAndSetProps is called from useEffect. Should potential Promise rejections be handled in useEffect?
There was a problem hiding this comment.
I commented on this in the commit message. The page where the reporting feat resides currently renders a 500 error page if the promise rejects, so I chose to ignore it here too. Do you find that reasonable?
anttimaki
force-pushed
the
packageListing-imports
branch
from
d3c3726 to
3b7b553
Compare
anttimaki
force-pushed
the
enable-package-reporting
branch
from
58bcf93 to
34828c4
Compare
apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackage.tsx
Outdated
Show resolved
Hide resolved
apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackage.tsx
Outdated
Show resolved
Hide resolved
The idea here is to further separate the sub component from the main view component. The view component had made some optimizations that assumed everything is located in the same file. To circumvent this, small changes were required to its markup structure. Some corners were cut short on the hook's side when it comes to handling the props-as-promise. E.g. it's assumed the listing always eventually resolves to correct values, but if that doesn't happen, the whole page is broken anyway. Also no fallback is currently provided while the promise is resolving (null is returned instead, but this can be changed). useEffect is used to resolve the props rather than Suspense and Await components, as the latters caused ~1s delay for the modal to open, since when moved to hook, they started processing only when the user clicked the report button.
I'm not entirely sure how modal widths are supposed to be adjusted, or if they're supposed to just take the space required by the content. Currently the "Additional information (optional)" label sets the width for the modal, which means the textarea on the form is very narrow on desktop, making annoying to write anything longer than a couple of words. On the other hand setting a min width to the textarea makes it not that responsive on mobile layouts. I think this should be addressed in some generic fashion, perhaps in the Modal component, and is therefore outside the scope of these changes.
anttimaki
force-pushed
the
enable-package-reporting
branch
from
34828c4 to
668b921
Compare
|
|
||
| export const packageListingReportRequestDataSchema = z.object({ | ||
| version: z.number().optional(), | ||
| version: z.number().optional(), // TODO: use SemVer string |
There was a problem hiding this comment.
A good TODO, but the SemVer string requires proper SemVer from the backend too. Currently the backend doesn't fully dishout proper SemVer.
Also this should probably be a string instead of number? (bad me)
3 participants
No description provided.