Report Package implementation revision #1585
Conversation
strongForm wouldn't set it's submission error value, if the error is not directly related to strongForm internal logic
|
Warning Rate limit exceeded@Oksamies has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
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. WalkthroughThis PR removes the modal-based report flow (ReportPackageModal, ReportPackageSubmitted, ReportPackageButton file, and useReportPackage hook) and replaces it with a single inline ReportPackage component and a ReportPackageButton exported from Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Oksamies
changed the title
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
|
||
| const done = ( | ||
| <> | ||
| <Modal.Body className="report-package-body report-package-body--centered"> |
There was a problem hiding this comment.
The CSS class names in this line don't match the defined classes in the CSS file. The classes should be report-package__body report-package__body--centered to align with the BEM naming convention used in ReportPackage.css, rather than report-package-body report-package-body--centered.
| <Modal.Body className="report-package-body report-package-body--centered"> | |
| <Modal.Body className="report-package__body report-package__body--centered"> |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
This(?) breaks the centering in the "submitted" modal in a very obvious way.
Oksamies
force-pushed
the
report-package-asko-revision
branch
from
1c1e3db to
69638e0
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1585 +/- ##
======================================
Coverage 9.89% 9.89%
======================================
Files 308 304 -4
Lines 22497 22497
Branches 404 400 -4
======================================
Hits 2225 2225
Misses 20272 20272 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackage.tsx (1)
211-227: Fix success body class names.
The success view still usesreport-package-body/report-package-body--centered, but the stylesheet definesreport-package__body/report-package__body--centered. As written, the centered layout never kicks in. Please swap to the BEM names.- <Modal.Body className="report-package-body report-package-body--centered"> + <Modal.Body className="report-package__body report-package__body--centered">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackage.css(2 hunks)apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackage.tsx(4 hunks)apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackageButton.tsx(0 hunks)apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackageModal.tsx(0 hunks)apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackageSubmitted.tsx(0 hunks)apps/cyberstorm-remix/app/p/components/ReportPackage/useReportPackage.tsx(0 hunks)apps/cyberstorm-remix/app/p/packageListing.css(1 hunks)apps/cyberstorm-remix/app/p/packageListing.tsx(3 hunks)apps/cyberstorm-remix/cyberstorm/utils/StrongForm/useStrongForm.ts(1 hunks)
💤 Files with no reviewable changes (4)
- apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackageSubmitted.tsx
- apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackageButton.tsx
- apps/cyberstorm-remix/app/p/components/ReportPackage/useReportPackage.tsx
- apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackageModal.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/cyberstorm-remix/app/p/packageListing.tsx (1)
apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackage.tsx (2)
ReportPackageButton(27-42)ReportPackage(64-256)
apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackage.tsx (1)
packages/thunderstore-api/src/index.ts (1)
RequestConfig(1-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Generate visual diffs
| @layer nimbus-layout { | ||
| .report-package--centered { | ||
| .report-package__body { | ||
| min-width: 600px; |
There was a problem hiding this comment.
Figma uses 30rem for form part 35rem for "submitted" part of the modal (I could've sworn the widths were different earlier) 39rem, if we're manually setting the width we should probably use that.
There was a problem hiding this comment.
I'd prefer sticking to pixel widths when there are media width queries being used for the same element. I see no clear benefit of tying the Modal widths to rems as we probably don't want the modal to widen or narrow just when the base font size changes.
There was a problem hiding this comment.
There's a sweet spot range for row width, measured in characters, when aiming for good readability. So assuming the modal's contents are in text and e.g. images, it would make sense to me for the modal to widen or narrow when the base font size changes.
However, based on a discussion with the designer, the main takeaway was that we might want to have the modals to default to 100% width (mobile first) and set max width to limit the width on wide displays. The px/rem discussion was less conclusive, so this is not a hill for me to die on, especially as I think there's a decent change we'll be revisiting the issue in the near future.
| line-height: var(--line-height-md); | ||
| } | ||
|
|
||
| @media (width <= 650px) { |
There was a problem hiding this comment.
Should this be in-built in the modal component? Needing to define it manually in each component seems less than ideal.
There was a problem hiding this comment.
Maybe, but defining component behaviour is out of scope for current mission.
There was a problem hiding this comment.
Regardless, how much time do you wager it would take to do it now? If it's not a big change, it could make sense to do it sooner than later to avoid having this conversation over and over again for each modal. (Note the earlier comment about using 100% width paired with maxWidth affects this one as well.)
| faFlagSwallowtail, | ||
| } from "@fortawesome/pro-solid-svg-icons"; | ||
|
|
||
| export function ReportPackageButton( |
There was a problem hiding this comment.
I very much on purpose extracted the components to separate files. Given that you reviewed those PRs I assume you know this. So why bring them back into one file? If the reason is the one mentioned in the commit message, that this is how things are done in the repo, then I'd like to point out that the reason I split the files is that IMO it shouldn't be done that way. (Or to be accurate, that there is a point where splitting components to more files makes sense.) So IDK what to think of this PR reverting the changes.
There was a problem hiding this comment.
Why create a whole file for one simple component that is meant to be used in the two use cases?
I've felt/assfeelt that creating files for each component regardless of their usage and scope, promotes people over-engineering those components.
This component happens to be used in two places because of (excuse my french) fucking stupid design decisions, so a need for a common component has arised. (to be used at the two places and as fallbacks)
Not sure where the pattern of "create a new file for each component" stems from with React related development, but there absolutely is no need for it. Especially when the LOC is under 300 and the complexity of the said component is extremely low.
There was a problem hiding this comment.
Why create a whole file for one simple component that is meant to be used in the two use cases?
We're not going to run out of files, so what is your actual concern here?
I've felt/assfeelt that creating files for each component regardless of their usage and scope, promotes people over-engineering those components.
I'd rather disregard assfeels; instead I welcome rational, justified arguments. I'd also refrain from pondering what "people" might do and focus on this code base and the files relevant to this PR. Do you think the components I extracted and are now dropped in this PR were overengineered somehow?
a need for a common component has arised. (to be used at the two places and as fallbacks)
What does this have to do with whether the component internally consists of 1 or n files?
Not sure where the pattern of "create a new file for each component" stems from with React related development, but there absolutely is no need for it.
It's not React related, it's universal to all programming. Think of a function that grows too long. You split a logical substep into a helper function and name it something like "doSubStep()". If done correctly this abstraction reduces cognitive load and makes following the code easier. Mind you, there's plenty more arguments for splitting things up, but the aforementioned is the most relevant to my argument here. And yes, too much of good thing can be a bad thing, but IMO the split here was net positive.
Especially when the LOC is under 300 and the complexity of the said component is extremely low.
While there's no fixed limit for the file length, I'd say 300 LOC is well into the orange zone where splitting it up may make sense. And I would argue that anything that currently uses the "strong form" can't be considered "extremely low" complexity. That's why I wanted to have a file/component that contains only that business logic, stripped of all distractions. In this case it meant that the stripped out parts were small by themselves, but combined they're something like 1/3 of the whole. (I've excluded the useReportPackage file from the above calculations as I'm fine with dropping it, as long long as the state management contained in it is not rammed into the form component).
(Also from the naming perspective, you now have a ReportPackageForm component that renders a modal which may or may not display a form, which may be a bit confusing.)
Generally speaking, the splitting of files is not a yes/no issue. In some places it makes sense, in others it's overkill. But I would urge you to consider if a UI component can be considered "extremely low complexity", if it handles internal state management, UI layout including two separate steps, form validation, API request/response, and error handling?
| setError, | ||
| ...requestParams | ||
| } = props; | ||
| const toast = useToast(); |
There was a problem hiding this comment.
As per the commit message from my PR, not showing the error in a toast was also a deliberate choice. Form errors should be shown on the form. Ideally modal related errors should be shown in the modal. Using toasts blinking in the corner of the eye is a bad way to do it.
There was a problem hiding this comment.
There has been no design decision made on the "way of showing errors" so in efforts of keeping the implementations similar, I've switched it to using the Toast in addition to having Alerts in the form.
Using toasts blinking in the corner of the eye is a bad way to do it.
The Toasts are meant to convey that something went wrong with submissions or actions, not that the form has something wrong. Hence the utility for grabbing field errors etc in the strongForm. This form just can't have field errors in practice, so the only errors that should show are the submissions related errors.
Your thinking is not wrong, but you've confused two different things together
There was a problem hiding this comment.
There has been no design decision made on the "way of showing errors" so in efforts of keeping the implementations similar, I've switched it to using the Toast in addition to having Alerts in the form.
Design decision has now been made. Generally speaking the defaults are:
- Errors related to modal should be shown on the modal, not on toast
- Errors related to a form should be shown on the form, not on toast
- Form submission error is an error related to the form
- What to do with "actions" (meaning e.g. like button) remains to be decided
Note that I'm not asking you to change everything everywhere all at once, just to not actively undo the changes already merged with the changes in this PR.
| <div className="report-package__block"> | ||
| <NewAlert csVariant="danger">{error}</NewAlert> | ||
| {strongForm.inputErrors ? ( | ||
| <NewAlert csVariant="danger"> |
There was a problem hiding this comment.
Input specific errors should be shown next to the relevant input if at all possible.
There was a problem hiding this comment.
Not possible currently, as that would need mapping of fields to errors and anchoring to elements on the form. Additionally there has not been a design decision made on how errors should be displayed, for anything, so by that the errors should not be shown there.
There was a problem hiding this comment.
Not possible currently, as that would need mapping of fields to errors and anchoring to elements on the form.
I see this as a major shortcoming in the strong form implementation. Please add a task into Inbox about adding the support for this.
Additionally there has not been a design decision made on how errors should be displayed, for anything, so by that the errors should not be shown there.
You've used your own judgement in plenty of places and situations within the project, so saying that something shouldn't be done at all just because no one has explicitly told you to do it is a lazy argument. Keep in mind that the point is to create a good service, not playing company politics or something like that.
| ) : null} | ||
| {strongForm.submitError ? ( | ||
| <NewAlert csVariant="danger"> | ||
| Error while submitting: {strongForm.submitError.message} |
There was a problem hiding this comment.
This now contains the less user-friendly error message when attempting to report a package while unauthenticated.
There was a problem hiding this comment.
This component is the wrong place to re-define the error message, it should be done in the function that throws the error or at very minimum some a bit more shared function like strongForm.
At one point I tried to create an generic error parser, but put that development on hold because something else came up.
There was a problem hiding this comment.
This component is the wrong place to re-define the error message, it should be done in the function that throws the error or at very minimum some a bit more shared function like strongForm.
User doesn't care where the redefinition gets done as long as it gets done. Consequently I don't care where it gets done as long as it gets done, but now it seems it doesn't get done at all, so the change in this PR makes things worse.
| Cancel | ||
| </NewButton> | ||
| <Modal.Close asChild> | ||
| <NewButton csVariant="secondary">Cancel</NewButton> |
There was a problem hiding this comment.
Cancel no longer resets the form state. At least I'd expect it to do so.
There was a problem hiding this comment.
Hmm, I don't know, I'd expect it to not reset the form state 🤷 This is a UX design decision
There was a problem hiding this comment.
Currently the poll in our Discord has 100% (n=3) in favor of Cancel resetting the form contents.
A LLM also agrees, adding a note that if you're worried about clicking in the modal background accidentally resetting the form for the user, it's behaviour can be different. But that leaves using the X-button or esc-key as a grey area, where closing the modal is unlikely to be accidental. I'd say it's clearer to just reset the form.
|
|
||
| const done = ( | ||
| <> | ||
| <Modal.Body className="report-package-body report-package-body--centered"> |
There was a problem hiding this comment.
This(?) breaks the centering in the "submitted" modal in a very obvious way.
|
|
||
| {ReportPackageButton} | ||
| <div className="package-listing__narrow-other-actions"> | ||
| <Suspense fallback={<p>Loading...</p>}> |
There was a problem hiding this comment.
Side note but this fallback looks very awkward when loading on mobile.
There was a problem hiding this comment.
The mobile designs do not include any indication on how fallbacks or skeleton components should be done. So I've left this as so on purpose, so it can be noticed by the UX testers.
There was a problem hiding this comment.
So I've left this as so on purpose, so it can be noticed by the UX testers.
Yet for the package reporting you're fine with using the button as the fallback. Maybe be consistent and use the same approach here?
| <Suspense fallback={<ReportPackageButton />}> | ||
| <Await resolve={listing}> | ||
| {(resolvedValue) => ( | ||
| <ReportPackage |
There was a problem hiding this comment.
After the changes in this PR, it would probably make sense to move this back into Actions. It was extracted from there just because my implementation wasn't very compatible with it.
There was a problem hiding this comment.
Ah, I've placed the report package under a different Suspense Await, because it only needs the listing.
Or did you mean, that I could combine all the actions into one component and pass it the promises?
There was a problem hiding this comment.
What I tried to say was that before my PRs, PackageReportButton, as well as <div className="package-listing-sidebar__actions"> were contained in the Actions component, which made sense as they're somewhat a logical unit. I only extracted them because my implementation (custom hook) required it. If the custom hook is dropped, I'd say it makes sense to put them back in. Not a blocker but would make things cleaner IMO.
…re done The existing implementation is very well done, but it's not aligned to how other similar components are done in the project. This commit introduces an implementation that is a bit simpler, with the added negative effect of having to have two usage instances of the component in the same page (packageListing route). Performance effects are mostly unknown, but no noticable difference was discovered in local dev env testing.
Oksamies
force-pushed
the
report-package-asko-revision
branch
from
69638e0 to
e3929d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/cyberstorm-remix/app/p/packageListing.tsx (1)
423-450: Consider state synchronization between instances.The ReportPackage component is rendered here and again on lines 613-624. Each instance maintains independent state, so if a user partially fills the form in one location, closes it, and opens it in the other location, they'll see a fresh form. This may be acceptable given the responsive layout, but consider whether shared state would improve UX.
apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackage.tsx (1)
142-145: TODO requires attention.The placeholder behavior is currently broken as documented. Consider creating a tracking issue if this hasn't been addressed yet.
Would you like me to open an issue to track this TODO?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackage.css(2 hunks)apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackage.tsx(4 hunks)apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackageButton.tsx(0 hunks)apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackageModal.tsx(0 hunks)apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackageSubmitted.tsx(0 hunks)apps/cyberstorm-remix/app/p/components/ReportPackage/useReportPackage.tsx(0 hunks)apps/cyberstorm-remix/app/p/packageListing.css(1 hunks)apps/cyberstorm-remix/app/p/packageListing.tsx(3 hunks)
💤 Files with no reviewable changes (4)
- apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackageButton.tsx
- apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackageModal.tsx
- apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackageSubmitted.tsx
- apps/cyberstorm-remix/app/p/components/ReportPackage/useReportPackage.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/cyberstorm-remix/app/p/packageListing.tsx (1)
apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackage.tsx (2)
ReportPackageButton(27-42)ReportPackage(64-256)
apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackage.tsx (1)
packages/thunderstore-api/src/index.ts (1)
RequestConfig(1-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (9)
apps/cyberstorm-remix/app/p/packageListing.css (1)
267-272: LGTM!The new layout class is well-structured and follows BEM naming conventions.
apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackage.css (2)
2-10: LGTM!The BEM naming convention is correct and the centering styles are properly separated into a modifier class.
50-54: LGTM!The responsive override ensures the component displays well on narrow screens.
apps/cyberstorm-remix/app/p/packageListing.tsx (1)
75-78: LGTM!The consolidated imports align with the component restructuring.
apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackage.tsx (5)
27-44: LGTM!The button component is clean and reusable, with proper prop spreading and display name.
57-62: LGTM!The interface is well-defined and matches the component's usage.
64-103: LGTM!The form state management is well-structured using a reducer pattern.
209-234: LGTM!The success view is well-structured with clear messaging and proper BEM class naming.
236-258: LGTM!The modal wrapper correctly manages state and conditionally renders form vs. success views. The form reset logic on successful submission is appropriate.
| onSubmitError: (error) => { | ||
| let message = `Error occurred: ${error.message || "Unknown error"}`; | ||
| if (error.message === "401: Unauthorized") { | ||
| message = "You must be logged in to report a package."; | ||
| } | ||
| setError(message); | ||
| toast.addToast({ | ||
| csVariant: "danger", | ||
| children: message, | ||
| duration: 8000, | ||
| }); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Consider removing duplicate error display.
Submit errors are shown in two places:
- Here as a toast with a user-friendly message (e.g., "You must be logged in to report a package.")
- In the form alert (lines 190-194) with the technical error message (e.g., "Error while submitting: 401: Unauthorized")
This creates redundant and inconsistent error messaging. Consider either:
- Removing
strongForm.submitErrorfrom the form display (lines 190-194) since the toast already communicates the error - Or removing the toast and keeping only the form alert (but applying the user-friendly message transformation there)
Apply this diff to remove the redundant form alert for submit errors:
- {strongForm.inputErrors ||
- strongForm.refineError ||
- strongForm.submitError ? (
+ {strongForm.inputErrors || strongForm.refineError ? (
<div className="report-package__block">
{strongForm.inputErrors ? (
<NewAlert csVariant="danger">
@@ -189,10 +189,6 @@
Error while refining: {strongForm.refineError.message}
</NewAlert>
) : null}
- {strongForm.submitError ? (
- <NewAlert csVariant="danger">
- Error while submitting: {strongForm.submitError.message}
- </NewAlert>
- ) : null}
</div>
) : null}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/cyberstorm-remix/app/p/components/ReportPackage/ReportPackage.tsx around
lines 118-129 and 190-194, the submit error is shown twice (toast in
onSubmitError and a form alert using strongForm.submitError). Remove the
redundant form alert block at lines ~190-194 so only the toast displays
user-friendly errors, or alternatively replace that alert with a conditional
that no longer reads strongForm.submitError; ensure any references to
strongForm.submitError are removed or guarded so the toast remains the sole
error UI for submits.
There was a problem hiding this comment.
The toast will not be reintroduced here
There was a problem hiding this comment.
@anttimaki, understood! I'll keep both error displays as you prefer.
Instead of overwriting the possible first submission error instead check for it before setting the outer error catch as the submission error. Added throws to the other if statements, too so that submitOutput isn't returned
2 participants
Bug fix: Add setting submission error to strongForm
strongForm wouldn't set it's submission error value, if the error is not directly related to strongForm internal logic
Bring ReportPackage Nimbus component closer to how other components are done
The existing implementation is very well done, but it's not aligned to how other similar components are done in the project. This commit introduces an implementation that is a bit simpler, with the added negative effect of having to have two usage instances of the component in the same page (packageListing route).
Performance effects are mostly unknown, but no noticable difference was discovered in local dev env testing.