Skip to content

fix: [DHIS2-21580] Sequential errors are not shown in the FeedbackBar#4591

Open
henrikmv wants to merge 14 commits into
masterfrom
hv/fix/DHIS2-21580_sequential-errors-are-not-shown-FeedbackBar
Open

fix: [DHIS2-21580] Sequential errors are not shown in the FeedbackBar#4591
henrikmv wants to merge 14 commits into
masterfrom
hv/fix/DHIS2-21580_sequential-errors-are-not-shown-FeedbackBar

Conversation

@henrikmv
Copy link
Copy Markdown
Contributor

@henrikmv henrikmv commented May 28, 2026

DHIS2-21580

This PR fixes feedback alert behavior by:

  1. Adding a unique id (via uuid()) to each feedback item, used as the key on AlertBar so React re-mounts the component when a new feedback arrives (resetting the auto-hide timer).
  2. Passing onHidden to AlertBar so it properly closes after the duration.
  3. Replacing state accumulation ([...state, newFeedback]) with a single-item array ([newFeedback]), meaning only the latest feedback is ever shown.

The reducer helpers are refactored so addErrorFeedback no longer takes state and always returns a fresh single-element array.

@henrikmv henrikmv requested a review from a team as a code owner May 28, 2026 12:38
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

View 1 additional finding in Devin Review.

Open in Devin Review

variant,
};
}
const addErrorFeedback = (input: ErrorFeedbackInput) => [getErrorFeedback(input)];
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot May 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Info: Behavioral change: feedback queue replaced with single-item replacement

The old addErrorFeedback accumulated items via [...state, newItem], forming a FIFO queue where CLOSE_FEEDBACK would shift items to reveal the next. The new version (feedback.reducerDescriptionGetter.ts:57) always returns [singleItem], discarding any existing feedback. This is actually a sensible fix because the old queue was never properly drained — AlertBar-type feedbacks had no mechanism to dispatch CLOSE_FEEDBACK (no onHidden was wired up), so the queue grew indefinitely while only feedbacks[0] was ever displayed. The new approach pairs correctly with the onHidden={onClose} addition.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

@henrikmv henrikmv May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The replacement behaviour is intentional.

The 5000ms auto-dismiss only applies to non-critical AlertBars- DHIS2 UI's AlertBar requires the user to manually close critical alerts. With the old queue, when multiple errors fired, users had to dismiss each stale message one by one.

Replacing instead of appending means the user sees the most recent, most actionable error - not a backlog of errors.

devin-ai-integration[bot]

This comment was marked as resolved.

@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

View 4 additional findings in Devin Review.

Open in Devin Review

<AlertStack>
{isAlertBarOpen && (
<AlertBar {...alertVariant} duration={5000}>
<AlertBar key={id} {...alertVariant} duration={5000} onHidden={onClose}>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Potential race condition between onHidden and new feedback arriving

The new onHidden={onClose} on the AlertBar (FeedbackBar.component.tsx:36) dispatches CLOSE_FEEDBACK when the alert finishes hiding. If a new error arrives while an old AlertBar is mid-hide-animation (timer expired but onHidden not yet called), the key change causes React to unmount the old AlertBar and mount a new one. If the old AlertBar's onHidden fires during or after unmount, it would dispatch CLOSE_FEEDBACK and shift the NEW error off the state at feedback.reducerDescriptionGetter.ts:61-64, causing the new error to disappear immediately. This depends on whether dhis2/ui's AlertBar properly cancels its hide callback on unmount. Standard React cleanup patterns should prevent this, but it's worth verifying with the AlertBar implementation.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified against @dhis2-ui/alert's AlertBar: its useEffect returns clearAllTimeouts as cleanup, so a key change unmounts the old bar and synchronously clears the pending hideTimeout before remove() (which calls onHidden) can fire. The race isn't reachable — remove() is purely synchronous and JS is single-threaded, so onHidden cannot fire after unmount.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant