Open
Conversation
FieldGroup, CustomLayout, and Disease.
- Removed PCR_TEST_SPECIFICATION loc from HTML_LAYOUT - Removed pcrTestSpecification instance variable - Removed addField(PCR_TEST_SPECIFICATION) from addFields() - Removed pcrTestSpecification.setValue(...) from setValue() - Removed pcrTestSpecification.setVisible(false) and FieldHelper.setVisibleWhen(PCR_TEST_SPECIFICATION) from addFields() CoronavirusDiseaseSectionLayout - Added HTML layout with fluidRowLocs(PCR_TEST_SPECIFICATION, "") - bindFields() now buildAndBinds the field, adds it to the panel, and wires visibility - unbindFields() now unbinds and removes the field from the panel - Updated class comment
…B and clean up the PathogenTestForm
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors PathogenTest UI into a composition model: PathogenTestForm delegates disease-specific fields to new DiseaseSectionLayout implementations, adds PathogenTestFormConfig and TubeFieldHandler, restructures field wiring/visibility into per-disease classes, and includes minor controller Javadoc/whitespace tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Form as PathogenTestForm
participant Factory as DiseaseSectionLayout.forDisease()
participant Section as DiseaseSectionLayout
participant Group as FieldGroup
participant Panel as CustomLayout
User->>Form: setDisease(TUBERCULOSIS)
Form->>Factory: forDisease(TUBERCULOSIS)
Factory-->>Form: TuberculosisDiseaseSectionLayout
Form->>Section: swapDiseaseSection(TUBERCULOSIS, config)
Form->>Section: bindFields(Group, Panel, TUBERCULOSIS, config)
Section->>Group: create/bind TB-specific fields (tubes, rifampicin, etc.)
Section->>Panel: add HTML layout + components
Section->>Group: register visibility listeners (returns Registration)
User->>Group: change TEST_TYPE -> GENOTYPING
Group->>Section: listener triggers -> onTestTypeChanged(...)
Section->>Group: updateItems, adjust visibility
User->>Form: setDisease(MEASLES)
Form->>Section: unbindFields(Group, Panel)
Section->>Group: remove listeners via Registration.remove()
Form->>Factory: forDisease(MEASLES)
Factory-->>Form: MeaslesDiseaseSectionLayout
Form->>Section: bindFields(Group, Panel, MEASLES, config)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sormas-ui/src/main/java/de/symeda/sormas/ui/utils/FieldHelper.java (1)
347-368:⚠️ Potential issue | 🟠 MajorThe null guard hides stale visibility listeners instead of cleaning them up.
setVisibleWhen(..., Map, ...)attaches anonymous listeners to shared source fields every time a disease section is bound. With the new long-lived form, repeated section swaps will keep stacking those listeners; this null check only turns the old failures into silent no-ops, and once the target field is rebound all accumulated listeners will fire again against the new instance. Please make these registrations removable or move them to a one-time form-level setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sormas-ui/src/main/java/de/symeda/sormas/ui/utils/FieldHelper.java` around lines 347 - 368, The onValueChangedSetVisible registration currently attaches anonymous listeners to source fields each time setVisibleWhen is called (via onValueChangedSetVisible), causing listener accumulation across disease section swaps; instead make these listener registrations removable by storing the ListenerRegistration (or equivalent) when you add the listener (e.g., in onValueChangedSetVisible/setVisibleWhen) and remove it when the target field is unbound or before re-binding, or move the registration to a one-time form-level setup so listeners are only added once for the FieldGroup; specifically, change the code paths around onValueChangedSetVisible, setVisibleWhen, and where targetField is unbound to keep a map of registrations per targetPropertyId (or per Field) and call remove/deregister on those registrations instead of relying on the null guard.
🧹 Nitpick comments (2)
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/TubeFieldHandler.java (1)
97-112: Potential infinite listener cascade when clearing numeric field on parse error.When
Float.parseFloat(val)throwsNumberFormatExceptionat line 108-111, the code clears the numeric field and then callsgt10.select(false). Clearing the numeric field triggers this same listener again withval = null, which then callsgt10.select(false)again at line 103. This could cause redundant calls, though it shouldn't cause an infinite loop since the null check exits early.Consider guarding against redundant GT10 updates:
♻️ Suggested improvement
private void handleNumericChange(String val, String numericId, String gt10Id) { NullableOptionGroup gt10 = (NullableOptionGroup) fieldGroup.getField(gt10Id); if (gt10 == null) { return; } if (val == null) { - gt10.select(false); + if (!Boolean.FALSE.equals(getCurrentGt10Value(gt10))) { + gt10.select(false); + } return; } try { - gt10.select(Float.parseFloat(val) > 10); + boolean shouldBeChecked = Float.parseFloat(val) > 10; + if (!Boolean.valueOf(shouldBeChecked).equals(getCurrentGt10Value(gt10))) { + gt10.select(shouldBeChecked); + } } catch (NumberFormatException e) { fieldGroup.getField(numericId).clear(); - gt10.select(false); + // Clearing the field will trigger this listener again with null value } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/TubeFieldHandler.java` around lines 97 - 112, In handleNumericChange, avoid redundant calls to gt10.select(false) when clearing the numeric field on parse error by first checking the current selection state on the NullableOptionGroup (obtained via fieldGroup.getField(gt10Id)) and only calling gt10.select(false) if it is currently selected; similarly, in the null-val branch (val == null) check the same current state before calling gt10.select(false) so the listener won't perform redundant updates (use numericId/gt10Id to locate the fields and query the NullableOptionGroup's selection state before setting it).sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.java (1)
673-686: Class-based equality check may prevent legitimate section re-binding.The comparison
newSection.getClass() == activeSection.getClass()at line 676 prevents swapping when the same section type is used. However, if a section needs to rebind with different state (e.g., after a DTO reset), this check would prevent it. Consider using instance equality or adding aneedsRebind()method if this becomes an issue.The current approach is acceptable for the refactor phase since disease sections are meant to be stateless templates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.java` around lines 673 - 686, The current swapDiseaseSection method short-circuits when newSection.getClass() == activeSection.getClass(), which blocks rebinding the same section instance when state changes; update swapDiseaseSection to use instance equality or a rebind predicate instead — e.g., replace the class comparison with (!newSection.equals(activeSection)) or introduce a boolean needsRebind(DiseaseSectionLayout other) on DiseaseSectionLayout and change the check to if (!newSection.needsRebind(activeSection)) return; so that when rebind is required you still call activeSection.unbindFields(...), newSection.bindFields(...), and rebuildSectionFieldAllowances(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@sormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/ImiDiseaseSectionLayout.java`:
- Around line 92-99: In ImiDiseaseSectionLayout.unbindFields(FieldGroup
fieldGroup, CustomLayout panel) you only unbind and remove
drugSusceptibilityField; extend this to also handle the SERO_GROUP_SPECIFICATION
and SERO_GROUP_SPECIFICATION_TEXT fields (the fields referenced by FIELD_IDS) by
calling fieldGroup.unbind(...) and panel.removeComponent(...) for each, and then
nulling their instance variables (e.g., seroGroupSpecificationField,
seroGroupSpecificationTextField) so all section fields are properly unbound and
cleaned up per the interface contract.
- Around line 85-90: drugSusceptibilityField (instance of
DrugSusceptibilityForm) is bound to the fieldGroup but never added to the UI, so
removal in unbindFields() is ineffective; fix by adding the component to the
container/layout after creating it (e.g., call the panel/layout's
addComponent/add method for drugSusceptibilityField right after
fieldGroup.bind(...)) so the form is actually part of the view and can be
removed by unbindFields(), keeping the existing drugSusceptibilityField,
DrugSusceptibilityForm, fieldGroup.bind and unbindFields() usage intact.
In
`@sormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/IpiDiseaseSectionLayout.java`:
- Around line 99-104: The drugSusceptibilityField is bound to fieldGroup but
never added to the panel, causing UI inconsistency and making unbindFields()
fail to remove it; after calling fieldGroup.bind(drugSusceptibilityField,
PathogenTestDto.DRUG_SUSCEPTIBILITY) add the component to the panel via
panel.addComponent(drugSusceptibilityField, PathogenTestDto.DRUG_SUSCEPTIBILITY)
so the form is displayed and unbindFields() can remove it correctly; verify
unbindFields() references drugSusceptibilityField by the same
PathogenTestDto.DRUG_SUSCEPTIBILITY key.
In
`@sormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/MeaslesDiseaseSectionLayout.java`:
- Around line 68-93: The GENOTYPE_RESULT_TEXT visibility rule currently only
depends on GENOTYPE_RESULT==OTHER and its listener is not scoped to the
disease/test-type, causing it to leak into other sections; update the visibility
registration to include the same disease/test-type predicate as the first rule
(use genoTypingDependencies plus the TEST_TYPE dependency) so the text field is
only shown when the disease matches AND TEST_TYPE/GENOTYPE_RESULT conditions
hold (i.e. change the FieldHelper.setVisibleWhen call for
PathogenTestDto.GENOTYPE_RESULT_TEXT to include genoTypingDependencies and
PathogenTestDto.TEST_TYPE along with PathogenTestDto.GENOTYPE_RESULT ==
GenoTypeResult.OTHER), or alternatively ensure any listener created for that
visibility is removed in unbindFields (remove the corresponding value change
listener like testTypeListener or null it) so the rule cannot fire after
unbinding.
In `@sormas-ui/src/main/java/de/symeda/sormas/ui/utils/AbstractEditForm.java`:
- Around line 571-576: The removeFromVisibleAllowedFields and
removeFromEditableAllowedFields methods currently call
visibleAllowedFields.remove(field) and editableAllowedFields.remove(field),
which only removes the first matching entry; update these methods to remove all
occurrences of the given Field so duplicates from
PathogenTestForm.rebuildSectionFieldAllowances cannot leave stale allow-list
entries that still make isVisibleAllowed/isEditableAllowed return true.
Implement the fix by replacing the single-remove with a full removal of all
matches (e.g., removeAll(Collections.singleton(field)) or a loop that removes
until no more matches) to ensure the lists contain no leftover duplicates.
In `@sormas-ui/src/main/java/de/symeda/sormas/ui/utils/AbstractForm.java`:
- Around line 562-577: The discard() override in AbstractForm currently iterates
a snapshot of getFields() but swallows all exceptions from f.discard(), hiding
failures; change it to keep the snapshot iteration but record the first thrown
Exception (from Field.discard()) into a local variable (e.g., firstException)
and continue discarding remaining fields, and after the loop if firstException
!= null rethrow it (or wrap/propagate it) so callers of AbstractForm.discard()
see the failure; ensure you reference the discard() method, getFields() snapshot
creation, and Field.discard() when making the change.
---
Outside diff comments:
In `@sormas-ui/src/main/java/de/symeda/sormas/ui/utils/FieldHelper.java`:
- Around line 347-368: The onValueChangedSetVisible registration currently
attaches anonymous listeners to source fields each time setVisibleWhen is called
(via onValueChangedSetVisible), causing listener accumulation across disease
section swaps; instead make these listener registrations removable by storing
the ListenerRegistration (or equivalent) when you add the listener (e.g., in
onValueChangedSetVisible/setVisibleWhen) and remove it when the target field is
unbound or before re-binding, or move the registration to a one-time form-level
setup so listeners are only added once for the FieldGroup; specifically, change
the code paths around onValueChangedSetVisible, setVisibleWhen, and where
targetField is unbound to keep a map of registrations per targetPropertyId (or
per Field) and call remove/deregister on those registrations instead of relying
on the null guard.
---
Nitpick comments:
In
`@sormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/TubeFieldHandler.java`:
- Around line 97-112: In handleNumericChange, avoid redundant calls to
gt10.select(false) when clearing the numeric field on parse error by first
checking the current selection state on the NullableOptionGroup (obtained via
fieldGroup.getField(gt10Id)) and only calling gt10.select(false) if it is
currently selected; similarly, in the null-val branch (val == null) check the
same current state before calling gt10.select(false) so the listener won't
perform redundant updates (use numericId/gt10Id to locate the fields and query
the NullableOptionGroup's selection state before setting it).
In `@sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.java`:
- Around line 673-686: The current swapDiseaseSection method short-circuits when
newSection.getClass() == activeSection.getClass(), which blocks rebinding the
same section instance when state changes; update swapDiseaseSection to use
instance equality or a rebind predicate instead — e.g., replace the class
comparison with (!newSection.equals(activeSection)) or introduce a boolean
needsRebind(DiseaseSectionLayout other) on DiseaseSectionLayout and change the
check to if (!newSection.needsRebind(activeSection)) return; so that when rebind
is required you still call activeSection.unbindFields(...),
newSection.bindFields(...), and rebuildSectionFieldAllowances(...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3349be1-34d7-4c20-8b25-6f0b505ec102
📒 Files selected for processing (16)
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestController.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/CoronavirusDiseaseSectionLayout.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/CryptosporidiosisDiseaseSectionLayout.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/CsmDiseaseSectionLayout.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/DefaultDiseaseSectionLayout.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/DiseaseSectionLayout.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/ImiDiseaseSectionLayout.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/IpiDiseaseSectionLayout.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/MeaslesDiseaseSectionLayout.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/PathogenTestFormConfig.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/TubeFieldHandler.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/TuberculosisDiseaseSectionLayout.javasormas-ui/src/main/java/de/symeda/sormas/ui/utils/AbstractEditForm.javasormas-ui/src/main/java/de/symeda/sormas/ui/utils/AbstractForm.javasormas-ui/src/main/java/de/symeda/sormas/ui/utils/FieldHelper.java
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/ImiDiseaseSectionLayout.java
Show resolved
Hide resolved
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/ImiDiseaseSectionLayout.java
Show resolved
Hide resolved
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/IpiDiseaseSectionLayout.java
Show resolved
Hide resolved
...ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/MeaslesDiseaseSectionLayout.java
Outdated
Show resolved
Hide resolved
sormas-ui/src/main/java/de/symeda/sormas/ui/utils/AbstractEditForm.java
Outdated
Show resolved
Hide resolved
Resolved conflict in PathogenTestForm.java by preserving the refactored DiseaseSectionLayout architecture. TB fields, tube fields, and serotyping fields remain encapsulated in their respective section layout classes rather than inlined. Removed stale call to FieldHelper.addSoftRequiredStyle() which was deleted in development. Removed incorrectly-staged PathogenTestFormConfig.java from samples package (correct location is diseasesection sub-package).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.java (2)
321-325: Minor inefficiency: template set twice during initialization.The
diseaseSectionPaneltemplate is set here usingactiveSection.getHtmlLayout()(which isDefaultDiseaseSectionLayout), then immediately overwritten inaddDiseaseAndResultFields()at lines 436-437 with the actual disease-specific template. Consider deferring the initial template setup toaddDiseaseAndResultFields()where the actual disease is known.♻️ Suggested refactor to avoid double template setting
private void addHeaderAndLayoutFields() { formConfig = PathogenTestFormConfig.fromCurrentConfig(); pathogenTestHeadingLabel = new Label(); pathogenTestHeadingLabel.addStyleName(H3); getContent().addComponent(pathogenTestHeadingLabel, PATHOGEN_TEST_HEADING_LOC); - // Install the disease section panel — a nested CustomLayout whose template is swapped on disease change diseaseSectionPanel = new CustomLayout(); - diseaseSectionPanel.setTemplateContents(activeSection.getHtmlLayout()); diseaseSectionPanel.setWidth(100, Unit.PERCENTAGE); getContent().addComponent(diseaseSectionPanel, DISEASE_SECTION_LOC);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.java` around lines 321 - 325, Currently diseaseSectionPanel has its template set to activeSection.getHtmlLayout() during initialization and then overwritten in addDiseaseAndResultFields(); remove the initial setTemplateContents call and defer template initialization to addDiseaseAndResultFields() so the template is only set once with the actual disease-specific layout. Specifically, delete or comment out diseaseSectionPanel.setTemplateContents(activeSection.getHtmlLayout()) in the constructor/initializer and ensure addDiseaseAndResultFields() (the method that calls diseaseSectionPanel.setTemplateContents(...) with the real template) is always invoked after disease selection so the panel receives its correct template once.
621-626: Consider documenting the double-cast pattern.The
(com.vaadin.v7.ui.AbstractField<PathogenTestResultType>) (Object) testResultFieldcast works but obscures intent. This pattern is necessary due to Vaadin 7's generic type limitations (ComboBoxextendsAbstractField<Object>). A brief comment explaining why would help future maintainers.📝 Suggested documentation
+ // Double-cast needed: ComboBox extends AbstractField<Object>, but onTestTypeChanged + // expects AbstractField<PathogenTestResultType> for type-safe result handling activeSection.onTestTypeChanged( testType, (Disease) diseaseField.getValue(), (com.vaadin.v7.ui.AbstractField<PathogenTestResultType>) (Object) testResultField, formConfig);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.java` around lines 621 - 626, The double-cast (com.vaadin.v7.ui.AbstractField<PathogenTestResultType>) (Object) testResultField obscures intent due to Vaadin 7 generics (ComboBox extends AbstractField<Object>), so add a brief inline comment immediately above the cast in the call to activeSection.onTestTypeChanged explaining that the Object->AbstractField<PathogenTestResultType> double-cast is required to satisfy the legacy Vaadin 7 API and to prevent unsafe generic warnings; reference the testResultField variable and the activeSection.onTestTypeChanged invocation in the comment so future maintainers understand why the cast is necessary and should not be simplified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.java`:
- Around line 687-704: rebuildSectionFieldAllowances currently only calls
addToVisibleAllowedFields so newly added disease section fields become visible
but not editable; update rebuildSectionFieldAllowances to also call
addToEditableAllowedFields(f) for each non-null field (similar to
removeFromAllowedLists which touches both visible and editable lists) so that
when fieldAccessesInitialized is true the fields are modifiable; locate the
method rebuildSectionFieldAllowances and add the addToEditableAllowedFields call
alongside addToVisibleAllowedFields for each field.
---
Nitpick comments:
In `@sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.java`:
- Around line 321-325: Currently diseaseSectionPanel has its template set to
activeSection.getHtmlLayout() during initialization and then overwritten in
addDiseaseAndResultFields(); remove the initial setTemplateContents call and
defer template initialization to addDiseaseAndResultFields() so the template is
only set once with the actual disease-specific layout. Specifically, delete or
comment out
diseaseSectionPanel.setTemplateContents(activeSection.getHtmlLayout()) in the
constructor/initializer and ensure addDiseaseAndResultFields() (the method that
calls diseaseSectionPanel.setTemplateContents(...) with the real template) is
always invoked after disease selection so the panel receives its correct
template once.
- Around line 621-626: The double-cast
(com.vaadin.v7.ui.AbstractField<PathogenTestResultType>) (Object)
testResultField obscures intent due to Vaadin 7 generics (ComboBox extends
AbstractField<Object>), so add a brief inline comment immediately above the cast
in the call to activeSection.onTestTypeChanged explaining that the
Object->AbstractField<PathogenTestResultType> double-cast is required to satisfy
the legacy Vaadin 7 API and to prevent unsafe generic warnings; reference the
testResultField variable and the activeSection.onTestTypeChanged invocation in
the comment so future maintainers understand why the cast is necessary and
should not be simplified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90c1db08-1226-4cd3-b088-c69400ed19ad
📒 Files selected for processing (3)
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.javasormas-ui/src/main/java/de/symeda/sormas/ui/utils/AbstractEditForm.javasormas-ui/src/main/java/de/symeda/sormas/ui/utils/FieldHelper.java
🚧 Files skipped from review as they are similar to previous changes (2)
- sormas-ui/src/main/java/de/symeda/sormas/ui/utils/AbstractEditForm.java
- sormas-ui/src/main/java/de/symeda/sormas/ui/utils/FieldHelper.java
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.java
Show resolved
Hide resolved
Contributor
|
SonarCloud analysis: https://sonarcloud.io/dashboard?id=SORMAS-Project&pullRequest=13878 |
raulbob
requested changes
Mar 12, 2026
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/TubeFieldHandler.java
Outdated
Show resolved
Hide resolved
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/TubeFieldHandler.java
Show resolved
Hide resolved
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.java
Show resolved
Hide resolved
sormas-ui/src/main/java/de/symeda/sormas/ui/utils/FieldHelper.java
Outdated
Show resolved
Hide resolved
...c/main/java/de/symeda/sormas/ui/samples/diseasesection/TuberculosisDiseaseSectionLayout.java
Outdated
Show resolved
Hide resolved
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/IpiDiseaseSectionLayout.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
sormas-ui/src/main/java/de/symeda/sormas/ui/utils/AbstractForm.java (1)
581-583:⚠️ Potential issue | 🟡 MinorUnsafe cast may throw ClassCastException for checked exceptions.
If
firstFailureis a checked exception (e.g.,SourceExceptionfromField.discard()), the direct cast toRuntimeExceptionwill fail. Wrap non-RuntimeExceptions to preserve the original exception while avoiding the unsafe cast.🛡️ Proposed fix
if (firstFailure != null) { - throw (RuntimeException) firstFailure; + if (firstFailure instanceof RuntimeException) { + throw (RuntimeException) firstFailure; + } + throw new IllegalStateException("Discard failed", firstFailure); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sormas-ui/src/main/java/de/symeda/sormas/ui/utils/AbstractForm.java` around lines 581 - 583, The code in AbstractForm that does "if (firstFailure != null) { throw (RuntimeException) firstFailure; }" unsafely casts checked exceptions; change it to check if firstFailure is a RuntimeException and rethrow it, otherwise wrap it in a new RuntimeException (or another appropriate unchecked wrapper) using the original exception as the cause so checked exceptions from Field.discard() (or similar) are preserved and no ClassCastException occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@sormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/TubeFieldHandler.java`:
- Around line 85-103: The ValueChangeListeners added in addTubePair (for
numericField -> handleNumericChange and gt10Field -> handleGt10Change) are
anonymous and never removed in detachTubePair, causing listener buildup; fix by
storing the listener references when attaching (e.g. keep a Map<String,
ValueChangeListener<?>> keyed by the pair IDs or field IDs) and on
detachTubePair look up and remove the stored listeners from the corresponding
Field instances before unbinding/removing them, then clear the entries from the
Map—follow the same listener storage/removal pattern used in
TuberculosisDiseaseSectionLayout.unbindFields() and add the necessary Map field
and imports.
---
Duplicate comments:
In `@sormas-ui/src/main/java/de/symeda/sormas/ui/utils/AbstractForm.java`:
- Around line 581-583: The code in AbstractForm that does "if (firstFailure !=
null) { throw (RuntimeException) firstFailure; }" unsafely casts checked
exceptions; change it to check if firstFailure is a RuntimeException and rethrow
it, otherwise wrap it in a new RuntimeException (or another appropriate
unchecked wrapper) using the original exception as the cause so checked
exceptions from Field.discard() (or similar) are preserved and no
ClassCastException occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60a0e067-3636-4292-b63e-8bd527db9239
📒 Files selected for processing (14)
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/PathogenTestForm.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/CoronavirusDiseaseSectionLayout.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/CryptosporidiosisDiseaseSectionLayout.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/CsmDiseaseSectionLayout.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/DiseaseSectionLayout.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/ImiDiseaseSectionLayout.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/IpiDiseaseSectionLayout.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/MeaslesDiseaseSectionLayout.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/TubeFieldHandler.javasormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/TuberculosisDiseaseSectionLayout.javasormas-ui/src/main/java/de/symeda/sormas/ui/utils/AbstractEditForm.javasormas-ui/src/main/java/de/symeda/sormas/ui/utils/AbstractForm.javasormas-ui/src/main/java/de/symeda/sormas/ui/utils/FieldHelper.javasormas-ui/src/main/java/de/symeda/sormas/ui/utils/Registration.java
🚧 Files skipped from review as they are similar to previous changes (1)
- sormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/IpiDiseaseSectionLayout.java
sormas-ui/src/main/java/de/symeda/sormas/ui/samples/diseasesection/TubeFieldHandler.java
Outdated
Show resolved
Hide resolved
Contributor
|
SonarCloud analysis: https://sonarcloud.io/dashboard?id=SORMAS-Project&pullRequest=13878 |
1 similar comment
Contributor
|
SonarCloud analysis: https://sonarcloud.io/dashboard?id=SORMAS-Project&pullRequest=13878 |
…ate defensive null check
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sormas-ui/src/main/java/de/symeda/sormas/ui/utils/FieldHelper.java (1)
365-371: Inconsistent return type: this overload doesn't returnRegistration.Unlike other
setVisibleWhenoverloads updated in this PR, this method still returnsvoid. The listener added at line 370 cannot be removed by the caller, which could lead to listener accumulation if this method is called multiple times on the same target field.For API consistency, consider returning
Registrationhere as well:♻️ Proposed fix
- public static void setVisibleWhen(final Field targetField, Map<Field, ? extends List<?>> sourceFieldsAndValues, final boolean clearOnHidden) { + public static Registration setVisibleWhen(final Field targetField, Map<Field, ? extends List<?>> sourceFieldsAndValues, final boolean clearOnHidden) { onValueChangedSetVisible(targetField, sourceFieldsAndValues, clearOnHidden); + Map<Field, Property.ValueChangeListener> listenerMap = new HashMap<>(); sourceFieldsAndValues.forEach( - (sourcePropertyId, sourceValues) -> targetField - .addValueChangeListener(event -> onValueChangedSetVisible(targetField, sourceFieldsAndValues, clearOnHidden))); + (sourceField, sourceValues) -> { + Property.ValueChangeListener listener = event -> onValueChangedSetVisible(targetField, sourceFieldsAndValues, clearOnHidden); + sourceField.addValueChangeListener(listener); + listenerMap.put(sourceField, listener); + }); + return () -> listenerMap.forEach((sourceField, listener) -> sourceField.removeValueChangeListener(listener)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sormas-ui/src/main/java/de/symeda/sormas/ui/utils/FieldHelper.java` around lines 365 - 371, Change the method signature of setVisibleWhen(Field, Map<Field,? extends List<?>>, boolean) to return a Registration instead of void; call onValueChangedSetVisible(...) immediately as now, then when adding listeners (using targetField.addValueChangeListener(...) for each entry) collect each returned Registration into a list, and return a single Registration whose remove() iterates and removes all collected registrations (or use an existing CompositeRegistration helper if available). Also update imports and any call sites to handle the returned Registration. Ensure you reference the existing onValueChangedSetVisible and the targetField addValueChangeListener usages when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@sormas-ui/src/main/java/de/symeda/sormas/ui/utils/FieldHelper.java`:
- Around line 365-371: Change the method signature of setVisibleWhen(Field,
Map<Field,? extends List<?>>, boolean) to return a Registration instead of void;
call onValueChangedSetVisible(...) immediately as now, then when adding
listeners (using targetField.addValueChangeListener(...) for each entry) collect
each returned Registration into a list, and return a single Registration whose
remove() iterates and removes all collected registrations (or use an existing
CompositeRegistration helper if available). Also update imports and any call
sites to handle the returned Registration. Ensure you reference the existing
onValueChangedSetVisible and the targetField addValueChangeListener usages when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8673f0b-be08-4699-9cba-ffbd54dff4ee
📒 Files selected for processing (1)
sormas-ui/src/main/java/de/symeda/sormas/ui/utils/FieldHelper.java
Contributor
|
SonarCloud analysis: https://sonarcloud.io/dashboard?id=SORMAS-Project&pullRequest=13878 |
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
3 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.
Fixes #13852
Summary by CodeRabbit
New Features
Improvements