fix(webapp): correct button disabling state broken by Remix v2 useNav…#4235
fix(webapp): correct button disabling state broken by Remix v2 useNav…#4235Naresh-official wants to merge 1 commit into
Conversation
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughUpdated loading-state conditions across alert, dashboard, and environment-variable routes to compare ✨ 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 |
|
Hi @Naresh-official, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
| navigation.formMethod === "POST" && | ||
| navigation.formData?.get("action") === "delete"; |
There was a problem hiding this comment.
🟡 Disable-alert button never shows its loading indicator
The loading-state check looks for the action value "delete" (navigation.formData?.get("action") === "delete" at apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts/route.tsx:426) but the button actually submits "disable" (line 444), so the condition is never true and the button text stays as "Disable" instead of changing to "Disabling" during submission.
Impact: Users get no visual feedback that the disable action is in progress.
Action value mismatch inherited from copy-paste of DeleteAlertChannelButton
The DisableAlertChannelButton component's isLoading expression was copy-pasted from DeleteAlertChannelButton at apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts/route.tsx:384-387, which correctly checks for "delete". The value was never updated to "disable" to match the button at line 444:
<Button name="action" value="disable" ...>
{isLoading ? "Disabling" : "Disable"}
</Button>Before this PR, the bug was masked because formMethod === "post" (lowercase) never matched Remix v2's uppercase "POST", making isLoading always false for a different reason. Now that the PR correctly uses "POST", this action-value mismatch becomes the sole reason isLoading is broken.
| navigation.formMethod === "POST" && | |
| navigation.formData?.get("action") === "delete"; | |
| navigation.formMethod === "POST" && | |
| navigation.formData?.get("action") === "disable"; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| navigation.formMethod === "POST" && | ||
| navigation.formData?.get("action") === "delete"; |
There was a problem hiding this comment.
🟡 Enable-alert button never shows its loading indicator
The loading-state check looks for the action value "delete" (navigation.formData?.get("action") === "delete" at apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts/route.tsx:466) but the button actually submits "enable" (line 484), so the condition is never true and the button text stays as "Enable" instead of changing to "Enabling" during submission.
Impact: Users get no visual feedback that the enable action is in progress.
Action value mismatch inherited from copy-paste of DeleteAlertChannelButton
The EnableAlertChannelButton component's isLoading expression was copy-pasted from DeleteAlertChannelButton at apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.alerts/route.tsx:384-387, which correctly checks for "delete". The value was never updated to "enable" to match the button at line 484:
<Button name="action" value="enable" ...>
{isLoading ? "Enabling" : "Enable"}
</Button>Before this PR, the bug was masked because formMethod === "post" (lowercase) never matched Remix v2's uppercase "POST", making isLoading always false for a different reason. Now that the PR correctly uses "POST", this action-value mismatch becomes the sole reason isLoading is broken.
| navigation.formMethod === "POST" && | |
| navigation.formData?.get("action") === "delete"; | |
| navigation.formMethod === "POST" && | |
| navigation.formData?.get("action") === "enable"; |
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
This PR fixes an issue where loading states were no longer activated for several form submissions after the Remix v2 upgrade.
navigation.formMethodnow returns uppercase HTTP methods (e.g."POST"), while the existing code was still comparing against lowercase ("post"). As a result, the loading state was never triggered, leaving action buttons enabled during submissions.On the Alerts page, this allowed users to click Save multiple times before the first request completed, resulting in duplicate alert channels being created.
Fixes #3455
Problem
Several routes determine whether a form is submitting using logic similar to:
With Remix v2,
navigation.formMethodis now uppercase ("POST"). Since the comparison never matched,isLoadingalways remainedfalse.This caused:
Solution
Updated the affected routes to compare against the correct uppercase HTTP method:
This restores the expected loading state behavior across the affected forms, ensuring that action buttons are disabled while a submission is in progress.
The change applies to:
Testing