Update state detection and error handling logic#486
Merged
Conversation
…ms remain disabled during both the "submitting" and "loading" (revalidation) phases by checking for `state !== "idle"
Contributor
📝 WalkthroughWalkthroughReplaces many exact "submitting" checks with broader "non-idle" checks (e.g., Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
coderabbitai
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
fdm-app/app/components/blocks/soil/list.tsx (1)
97-99: Scope the loading label to the row being deleted.At Line 111, every row shows
"Verwijderen..."when one delete is in progress because the condition is global (fetcher.state !== "idle"). Keep global disabling if desired, but tie spinner/label to the activea_idto avoid confusing feedback.Also applies to: 111-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/soil/list.tsx` around lines 97 - 99, The delete label is shown for every row because the loading check uses the global fetcher.state; change the condition so the spinner/“Verwijderen...” label is true only when the current row's id matches the active deletion request. Modify the expression that currently uses fetcher.state !== "idle" (and analysis.a_source === "nl-other-nmi") to also verify the active a_id from the fetcher submission/formData (e.g. fetcher.submission?.formData?.get("a_id") === analysis.a_id) or use a local activeDeletingId state set when dispatching the delete; apply this check where the label/spinner is rendered (reference: fetcher, fetcher.state, fetcher.submission/formData and analysis.a_id).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/famous-hotels-travel.md:
- Line 5: The release note contains an unclosed inline code span around the
expression `state !== "idle"` which breaks markdown; edit
.changeset/famous-hotels-travel.md and close the inline code by adding the
missing backtick so the sentence reads with a properly fenced inline code span
(e.g., `state !== "idle"`), ensuring the markdown renders correctly.
In `@fdm-app/app/components/blocks/cultivation/card-details.tsx`:
- Around line 106-107: The checkbox is using a different busy-state check
(fetcher.state === "submitting") than the fieldset and button (fetcher.state !==
"idle"); update the checkbox's busy/disabled condition to use fetcher.state !==
"idle" so it aligns with the existing pattern (locate the checkbox input in the
CardDetails component and replace the fetching condition), and verify other
related controls use the same fetcher.state !== "idle" check for consistency.
In `@fdm-app/app/components/blocks/fertilizer-applications/list.tsx`:
- Around line 93-95: FertilizerApplicationsList is checking a local useFetcher
(fetcher.state !== "idle") but that fetcher never runs mutations so the busy
checks are ineffective; update the component to accept a boolean prop isBusy
(passed from parent FertilizerApplicationCard which actually runs
handleDelete/handleEdit) and replace all uses of fetcher.state !== "idle" in
FertilizerApplicationsList with the new isBusy prop, update the parent to
compute and pass isBusy (true while its fetcher is loading/submitting) and
remove or ignore the unused local useFetcher in FertilizerApplicationsList.
In `@fdm-app/app/components/blocks/harvest/form.tsx`:
- Around line 620-621: The submit controls and form wrappers in the harvest form
currently only check form.formState.isSubmitting and thus allow submits while a
delete fetcher is in flight; update every submit button and surrounding form
container in this component (where you currently reference
form.formState.isSubmitting) to also consider fetcher.state by disabling when
(form.formState.isSubmitting || fetcher.state !== "idle"), and add a disabled
attribute to the inline submit button so it obeys the same gating; reference the
existing fetcher variable and form.formState.isSubmitting in Harvest form JSX
and apply the combined check to the submit buttons and form containers.
In `@fdm-app/app/lib/error.ts`:
- Around line 248-251: handleLoaderError currently checks for permission issues
using exact equality which can miss wrapped errors; update handleLoaderError to
use the same fuzzy check as handleActionError by calling
containsErrorMessage(error, "Principal does not have permission to perform this
action") (or equivalent helper) so permission-related errors caught in loaders
map to 403 instead of falling through to 500; reference containsErrorMessage,
handleLoaderError, handleActionError and the permission message when making the
change.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.field.fertilizer._index.tsx:
- Line 327: The overlay shows fertilizer-specific submit messages because
isSubmitting is set from navigation.state !== "idle", which also flips true on
loader revalidations (e.g., handleSelectionChange updating search params);
change isSubmitting to only true for real form submissions (e.g.,
navigation.state === "submitting" && Boolean(navigation.formData)) or inspect a
known form field in navigation.formData (such as the fertilizer form's submit
name) so loader-triggered navigations don't show the fertilizer messages; update
the conditional used to render the overlay (the isSubmitting variable and the
JSX at the fertilizer overlay render around the current overlay text) to rely on
the new check so only genuine fertilizer form submissions display "Bemesting
wordt gewijzigd..." / "Bemesting wordt toegevoegd...".
In
`@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation_.fertilizer._index.tsx:
- Line 308: Change the isSubmitting check to only treat real form (mutation)
submissions as submitting: replace the current navigation.state !== "idle" logic
with a guard that also requires navigation.formData to be present (e.g.,
isSubmitting = navigation.state !== "idle" && Boolean(navigation.formData)).
This will prevent search-param navigations triggered by setSearchParams() from
showing the fertilizer overlay ("Bemesting wordt toegevoegd...") during filter
changes; update any uses of isSubmitting (the overlay render) accordingly so it
only shows when navigation.formData is populated.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.rotation_.harvest._index.tsx:
- Line 379: isSubmitting currently treats any navigation (including query-param
updates from setSearchParams) as a submission because it uses navigation.state
!== "idle"; change the check to only flag real form submissions by requiring
navigation.state === "submitting" and that navigation.formData exists (e.g.,
const isSubmitting = navigation.state === "submitting" &&
Boolean(navigation.formData)), so the "Oogst wordt toegevoegd..." overlay only
appears for actual harvest form submissions (references: isSubmitting,
navigation.state, navigation.formData, setSearchParams).
---
Nitpick comments:
In `@fdm-app/app/components/blocks/soil/list.tsx`:
- Around line 97-99: The delete label is shown for every row because the loading
check uses the global fetcher.state; change the condition so the
spinner/“Verwijderen...” label is true only when the current row's id matches
the active deletion request. Modify the expression that currently uses
fetcher.state !== "idle" (and analysis.a_source === "nl-other-nmi") to also
verify the active a_id from the fetcher submission/formData (e.g.
fetcher.submission?.formData?.get("a_id") === analysis.a_id) or use a local
activeDeletingId state set when dispatching the delete; apply this check where
the label/spinner is rendered (reference: fetcher, fetcher.state,
fetcher.submission/formData and analysis.a_id).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.changeset/famous-hotels-travel.md.changeset/lucky-tigers-report.mdfdm-app/app/components/blocks/atlas/atlas-panels.tsxfdm-app/app/components/blocks/cultivation/card-details.tsxfdm-app/app/components/blocks/fertilizer-applications/card.tsxfdm-app/app/components/blocks/fertilizer-applications/columns.tsxfdm-app/app/components/blocks/fertilizer-applications/form.tsxfdm-app/app/components/blocks/fertilizer-applications/list.tsxfdm-app/app/components/blocks/field/delete.tsxfdm-app/app/components/blocks/harvest/form.tsxfdm-app/app/components/blocks/mijnpercelen/form-upload.tsxfdm-app/app/components/blocks/soil/form-upload.tsxfdm-app/app/components/blocks/soil/list.tsxfdm-app/app/lib/error.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.field.$b_id.fertilizer._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.field.fertilizer._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rotation_.fertilizer._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rotation_.harvest._index.tsxfdm-app/app/routes/farm.$b_id_farm.settings.delete.tsxfdm-app/app/routes/farm.$b_id_farm.settings.organic-certification.tsxfdm-app/app/routes/signin.check-your-email.tsxfdm-app/app/routes/signin.verify.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
fdm-app/app/components/blocks/cultivation/card-details.tsx (1)
84-90: Consider extracting a sharedisBusyflag to avoid condition drift.The same busy expression is repeated in multiple places; centralizing it improves readability and reduces future inconsistencies.
♻️ Suggested refactor
+ const isBusy = form.formState.isSubmitting || fetcher.state !== "idle" ... disabled={ - form.formState.isSubmitting || - fetcher.state !== "idle" + isBusy } > - {form.formState.isSubmitting || - fetcher.state !== "idle" ? ( + {isBusy ? ( ... disabled={ !editable || - form.formState.isSubmitting || - fetcher.state !== "idle" + isBusy } ... disabled={ !editable || - form.formState - .isSubmitting || - fetcher.state !== "idle" + isBusy } ... disabled={ - form.formState.isSubmitting || - fetcher.state !== "idle" + isBusy } > - {form.formState.isSubmitting || - fetcher.state !== "idle" ? ( + {isBusy ? (Also applies to: 103-107, 140-145, 214-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/cultivation/card-details.tsx` around lines 84 - 90, Extract the repeated busy check (form.formState.isSubmitting || fetcher.state !== "idle") into a single boolean like isBusy at the top of the CardDetails component (or the relevant render function) and replace all occurrences (currently using form.formState.isSubmitting and fetcher.state checks) with that isBusy variable; ensure you update the JSX props and conditional render blocks that reference the expression (the spots around the existing usages of form.formState.isSubmitting and fetcher.state) so they all read from isBusy to prevent condition drift.fdm-app/app/components/blocks/fertilizer-applications/list.tsx (1)
149-153: Consider row-scoped pending visuals instead of global spinner state.
isBusycurrently renders spinner icons on every delete button during a single in-flight delete. Tracking a pendingp_app_idwould give clearer feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/fertilizer-applications/list.tsx` around lines 149 - 153, Replace the global isBusy boolean with a row-scoped pending id: introduce state (e.g., pendingDeleteId) and in the delete handler set pendingDeleteId to the p_app_id being deleted, then clear it on success/error; in the row UI replace the isBusy check with (pendingDeleteId === p_app.id) so only the deleting row shows <Spinner /> while others render <Trash />, and also disable the delete button when pendingDeleteId === p_app.id to prevent duplicate deletes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@fdm-app/app/components/blocks/cultivation/card-details.tsx`:
- Around line 84-90: Extract the repeated busy check
(form.formState.isSubmitting || fetcher.state !== "idle") into a single boolean
like isBusy at the top of the CardDetails component (or the relevant render
function) and replace all occurrences (currently using
form.formState.isSubmitting and fetcher.state checks) with that isBusy variable;
ensure you update the JSX props and conditional render blocks that reference the
expression (the spots around the existing usages of form.formState.isSubmitting
and fetcher.state) so they all read from isBusy to prevent condition drift.
In `@fdm-app/app/components/blocks/fertilizer-applications/list.tsx`:
- Around line 149-153: Replace the global isBusy boolean with a row-scoped
pending id: introduce state (e.g., pendingDeleteId) and in the delete handler
set pendingDeleteId to the p_app_id being deleted, then clear it on
success/error; in the row UI replace the isBusy check with (pendingDeleteId ===
p_app.id) so only the deleting row shows <Spinner /> while others render <Trash
/>, and also disable the delete button when pendingDeleteId === p_app.id to
prevent duplicate deletes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.changeset/famous-hotels-travel.mdfdm-app/app/components/blocks/cultivation/card-details.tsxfdm-app/app/components/blocks/fertilizer-applications/card.tsxfdm-app/app/components/blocks/fertilizer-applications/list.tsxfdm-app/app/components/blocks/harvest/form.tsxfdm-app/app/components/blocks/soil/list.tsxfdm-app/app/lib/error.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.field.fertilizer._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rotation_.fertilizer._index.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.rotation_.harvest._index.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- fdm-app/app/components/blocks/soil/list.tsx
- .changeset/famous-hotels-travel.md
BoraIneviNMI
approved these changes
Mar 2, 2026
Collaborator
BoraIneviNMI
left a comment
BoraIneviNMI
left a comment
There was a problem hiding this comment.
All changes look correct.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit