Clear 'Report Issue' Form Fields on Successful Submission #877
Conversation
The unified report form's state is now cleared from `localStorage` on a successful submission. - The `localStorage` entry `report_form_state` is removed in the form's `action` handler, ensuring the cleanup occurs before the server redirects. - If a user navigates to the report page with a specific machine in the URL (`defaultMachineId`), any existing draft in `localStorage` is cleared, preventing stale data from populating a new report. Co-authored-by: timothyfroehlich <5819722+timothyfroehlich@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR implements form clearing functionality for the "Report Issue" form to prevent pre-populating fields with previously submitted data. The solution clears localStorage when a user submits the form and when they navigate to the page with a machine parameter in the URL.
Changes:
- Clear localStorage draft when navigating with a machine URL parameter (indicating intent to report for specific machine)
- Clear localStorage immediately before form submission via inline action wrapper
- Add early-return logic in restoration useEffect when defaultMachineId is present
| <form | ||
| action={(formData) => { | ||
| window.localStorage.removeItem("report_form_state"); | ||
| formAction(formData); | ||
| }} | ||
| className="space-y-4" |
There was a problem hiding this comment.
The new localStorage clearing behavior introduced in this PR lacks test coverage. While other report form functionality is tested in files like e2e/smoke/public-reporting.spec.ts, there are no tests verifying that:
- localStorage is cleared after successful submission
- localStorage is cleared when navigating to the page with a machine parameter
- Form fields don't persist after successful submission
Consider adding E2E tests to verify this critical user-facing behavior, especially since the implementation touches client-side state management that can only be properly validated through browser tests.
| <form | ||
| action={(formData) => { | ||
| window.localStorage.removeItem("report_form_state"); | ||
| formAction(formData); | ||
| }} | ||
| className="space-y-4" | ||
| > |
There was a problem hiding this comment.
This inline function breaks progressive enhancement. According to the repository guidelines, "Forms: Use <form action={serverAction}> pattern. Avoid JS-only submission flows" and "Progressive Enhancement: Forms must submit without JavaScript."
With this implementation, if JavaScript is disabled or fails to load, the form will not submit at all because the action is an inline function that requires JavaScript to execute.
The original approach of passing the server action directly (action={formAction}) would allow the form to submit without JavaScript. Consider implementing the localStorage clearing logic in a different way that doesn't compromise progressive enhancement, such as:
- Clearing localStorage in a useEffect that watches for successful state.success
- Using the Next.js router to detect successful navigation away from the page
- Implementing the clearing logic in the server action itself (though this is less ideal for client-side state management)
| <form action={formAction} className="space-y-4"> | ||
| <form | ||
| action={(formData) => { | ||
| window.localStorage.removeItem("report_form_state"); |
There was a problem hiding this comment.
This approach clears the localStorage immediately before the form is submitted, which means the user's draft is lost even if the submission fails (e.g., due to rate limiting or validation errors).
If the server action returns an error state (lines 183-189 show error handling), the user will see the error but their form data will be gone from localStorage, and they'll have to re-enter everything.
A better approach would be to only clear localStorage upon successful submission. Since the server action redirects on success, you could clear localStorage in a useEffect that detects when state.success is true, or right before the redirect occurs. Alternatively, consider clearing localStorage only after confirming the submission succeeded, not before attempting it.
| window.localStorage.removeItem("report_form_state"); |
The "Report Issue" form fields are now cleared after a successful submission. This is achieved by removing the
report_form_statefromlocalStoragebefore the server action is invoked. Additionally, the form will now clear any saved draft if the user navigates to the page with a specific machine in the URL, indicating a new report.Fixes #851
PR created automatically by Jules for task 5688178641210715879 started by @timothyfroehlich