(fix) O3-4898: Implementer tools: Prevent crash when hovering over tree-level descriptions #1485

RajPrakash681 · 2025-10-21T10:41:11Z

Requirements

This PR has a title that briefly describes the work done including the ticket number. Ensure your PR title includes a conventional commit label (such as feat, fix, or chore, among others). See existing PR titles for inspiration.

If applicable

My work is based on designs, which are linked or shown either in the Jira ticket or the description below.
My work includes tests or is validated by existing tests.
I have updated the esm-framework mock to reflect any API changes I have made.

Summary

This PR fixes a crash in the Implementer Tools that occurred when hovering over configuration nodes that have _description at the tree level (for parent/container nodes) instead of only at the leaf level.

Problem

The OpenMRS config system supports descriptions at both tree and leaf levels:

Tree level: foo: { _description: "Controls the big Foo thing", bar: {...} }
Leaf level: foo: { _description: "A setting", _value: "val", _source: "default" }

However, the ConfigSubtree component was iterating over ALL keys in config objects, including metadata keys like _description, _validators, _type, etc., and trying to render them as config tree nodes. When it encountered a tree-level _description, it would:

Try to render _description as a config property node
Attempt to recursively process the string value as if it were a config object
Call Object.entries("string value") which causes unexpected behavior and crashes

Issue

https://openmrs.atlassian.net/browse/O3-4898

RajPrakash681 · 2025-10-21T10:44:10Z

@denniskigen this one also

ibacher

Can we undo the changes to the en.json files, please?

.../esm-implementer-tools-app/src/configuration/interactive-editor/config-subtree.component.tsx

packages/apps/esm-implementer-tools-app/translations/en.json

RajPrakash681 · 2025-10-22T09:08:46Z

@ibacher i have fixed the changes you asked to do

denniskigen · 2025-10-23T10:40:57Z

.../esm-implementer-tools-app/src/configuration/interactive-editor/config-subtree.component.tsx

+        .filter(([key]) => !key.startsWith('_'))
+        .map(([key, value], i) => {
+          const thisPath = path.concat([key]);
+          const isLeaf = Object.hasOwn(value, '_value');


Suggested change

const isLeaf = Object.hasOwn(value, '_value');

const isLeaf = value && typeof value === 'object' && Object.hasOwn(value, '_value');

Can we add a type check here before calling Object.hasOwn()? It will throw a TypeError if called on null or undefined. Without a guard, hovering over or rendering a config node with a null/undefined value will crash. With this check, value && short-circuits when value is null or undefined, so we never reach the typeof or Object.hasOwn() calls. This prevents the TypeError.

denniskigen · 2025-10-23T10:45:17Z

Tangential, but can we fix the improper use of lodash.unset in editable-value.component.tsx?

lodash.unset() mutates the object in place and returns a boolean (true if property existed), not the modified object. The current code passes this boolean to setState():

temporaryConfigStore.setState(unset(temporaryConfigStore.getState(), ['config', ...path]) as any);

This corrupts the store state. Instead, we should clone first, mutate the clone, then pass the clone to setState():

const state = cloneDeep(temporaryConfigStore.getState());
unset(state, ['config', ...path]);
temporaryConfigStore.setState(state);

This follows immutable state update patterns and prevents store corruption when resetting config values to defaults.

RajPrakash681 · 2025-10-23T12:25:30Z

@denniskigen i have applied the changes
also can you please look at openmrs/openmrs-esm-patient-management#2058 and openmrs/openmrs-esm-form-engine-lib#653

RajPrakash681 · 2025-10-25T10:42:06Z

@denniskigen sir any updates on this

RajPrakash681 · 2025-10-27T20:16:26Z

@denniskigen sorry to tag you again wanted to hop you up about this what else i need to do?

…descriptions

- Add type checks before Object.hasOwn() to prevent TypeError on null/undefined values - Fix improper use of lodash.unset by cloning state before mutation - Follows immutable state update patterns to prevent store corruption

- Add tests to verify type guards prevent TypeError on null/undefined values - Add tests to verify immutable state update pattern with lodash.unset - Ensures changes follow best practices and prevent store corruption

… component Adds type guards for _source and _description properties to prevent runtime crashes when accessing metadata on non-leaf config nodes. Also removes unused parameters and adds explicit TypeScript type annotations for better type safety.

Converts CRLF (Windows) line endings to LF (Unix) to match repository convention.

Previous tests were not canonical and did not test the component-level behavior. Rather, they test implementation details. They also used non-canonical imports instead of Jest globals and are not colocated properly to the code they were testing.

denniskigen · 2025-10-27T22:01:05Z

@RajPrakash681 could you change your IDE's line endings from CRLF (Carriage Return + Line Field) to LF (Line Field) to align with the repo's conventions? That explains the spurious en.json file changes.

denniskigen

Thanks, @RajPrakash681!

RajPrakash681 · 2025-10-28T08:11:30Z

@RajPrakash681 could you change your IDE's line endings from CRLF (Carriage Return + Line Field) to LF (Line Field) to align with the repo's conventions? That explains the spurious en.json file
Sure!

brandones · 2025-11-04T18:44:47Z

I don't know why this was allowed to be merged without a video or screenshot—the crash no longer happens, but the higher-level description elements do not get displayed.

denniskigen · 2025-11-04T18:48:49Z

I don't know why this was allowed to be merged without a video or screenshot—the crash no longer happens, but the higher-level description elements do not get displayed.

🤦🏼 should we revert it, @brandones?

brandones · 2025-11-04T18:52:31Z

I didn't read the code, dunno what it does; up to you. I updated the ticket description to clarify that expectation.

RajPrakash681 · 2025-11-04T18:57:29Z

@denniskigen @brandones
Hello,

Sorry for any inconvenience caused by my message. I see that the crash issue has been fixed, but now the tree-level descriptions are not displaying as expected,

I am happy to look into fixing this display issue with the higher-level descriptions in the Implementer Tools. Please confirm if you would like me to take this up, and if you can provide any additional details or test cases that I should consider when working on this fix.

RajPrakash681 · 2025-11-04T19:00:26Z

also,Also, I didn’t expect this issue to occur because all the tests were passing earlier in the same manner. Sorry for the unexpected problem and any confusion it may have caused.

brandones · 2025-11-04T19:21:18Z

@RajPrakash681 please see the ticket description and my screenshot above. If it is clear to you what is being asked, then yes please go ahead and fix it. If not, ask a clarifying question.

Also, I didn’t expect this issue to occur because all the tests were passing earlier in the same manner.

The tests were also passing when the bug was causing the UI to crash completely. I see you wrote a test that has nothing to do with the problem (no mention of _description). I imagine that it too would pass without your change. That said, tests are not a sufficient guide to whether things work or not; you need to actually think about it. Think, "What is this supposed to do? What should it look like when it's working?" If you can write a test that fails, and which should pass if the bug is fixed, TDD style, then that's great.

This is a large, complex application, and you can't expect to just throw AI at it and get correct results. You have to try to understand it a little bit, at least the little piece you're working on.

denniskigen · 2025-11-04T19:29:49Z

I just ran the allergies app locally and I can see the descriptions:

window.spaVersion returns 8.0.1-pre.3473-20251029.

RajPrakash681 · 2025-11-04T19:37:39Z

also I promise to be more thorough in understanding the requirements and testing my changes properly before submitting. Issues like this won't happen again from my side - I'll make sure to always verify the feature works end-to-end in the UI, not just that the code compiles.

I'll update here once I have a proper fix ready for review.

RajPrakash681 · 2025-11-04T20:17:56Z

@denniskigen i can see the description

brandones · 2025-11-04T23:09:35Z

I just ran the allergies app locally and I can see the descriptions:

It's the descriptions of upper-level elements (so in that example, concepts) rather than leaf nodes that are problematic. The leaf node descriptions have always worked fine.

denniskigen requested a review from ibacher October 21, 2025 21:52

ibacher reviewed Oct 21, 2025

View reviewed changes

RajPrakash681 force-pushed the fix/implementer-tools-tree-description-crash branch from 8123176 to e477c92 Compare October 22, 2025 09:06

denniskigen reviewed Oct 23, 2025

View reviewed changes

denniskigen changed the title ~~(fix) O3-4898 : Implementer tools: Prevent crash when hovering over tree-level~~ (fix) O3-4898: Implementer tools: Prevent crash when hovering over tree-level descriptions Oct 23, 2025

RajPrakash681 and others added 7 commits October 27, 2025 23:53

(fix) Implementer tools: Prevent crash when hovering over tree-level …

2081be2

…descriptions

fix: use Object.hasOwn() and revert en.json changes

9148da5

test: add unit tests for type guards and lodash.unset fix

715dd51

- Add tests to verify type guards prevent TypeError on null/undefined values - Add tests to verify immutable state update pattern with lodash.unset - Ensures changes follow best practices and prevent store corruption

Normalize line endings in translation files to LF

014df6b

Converts CRLF (Windows) line endings to LF (Unix) to match repository convention.

denniskigen force-pushed the fix/implementer-tools-tree-description-crash branch from 062ebca to d903639 Compare October 27, 2025 21:50

denniskigen approved these changes Oct 27, 2025

View reviewed changes

denniskigen merged commit 19a7954 into openmrs:main Oct 27, 2025
12 of 14 checks passed

	const isLeaf = Object.hasOwn(value, '_value');
	const isLeaf = value && typeof value === 'object' && Object.hasOwn(value, '_value');

(fix) O3-4898: Implementer tools: Prevent crash when hovering over tree-level descriptions #1485

(fix) O3-4898: Implementer tools: Prevent crash when hovering over tree-level descriptions #1485

Uh oh!

Conversation

RajPrakash681 commented Oct 21, 2025 • edited by brandones Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Requirements

If applicable

Summary

Problem

Issue

Uh oh!

RajPrakash681 commented Oct 21, 2025

Uh oh!

ibacher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RajPrakash681 commented Oct 22, 2025

Uh oh!

denniskigen Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

denniskigen commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RajPrakash681 commented Oct 23, 2025

Uh oh!

RajPrakash681 commented Oct 25, 2025

Uh oh!

RajPrakash681 commented Oct 27, 2025

Uh oh!

denniskigen commented Oct 27, 2025

Uh oh!

denniskigen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RajPrakash681 commented Oct 28, 2025

Uh oh!

brandones commented Nov 4, 2025

Uh oh!

denniskigen commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandones commented Nov 4, 2025

Uh oh!

RajPrakash681 commented Nov 4, 2025

Uh oh!

RajPrakash681 commented Nov 4, 2025

Uh oh!

brandones commented Nov 4, 2025

Uh oh!

denniskigen commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RajPrakash681 commented Nov 4, 2025

Uh oh!

RajPrakash681 commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandones commented Nov 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RajPrakash681 commented Oct 21, 2025 •

edited by brandones

Loading

denniskigen Oct 23, 2025 •

edited

Loading

denniskigen commented Oct 23, 2025 •

edited

Loading

denniskigen commented Nov 4, 2025 •

edited

Loading

denniskigen commented Nov 4, 2025 •

edited

Loading

RajPrakash681 commented Nov 4, 2025 •

edited

Loading