Skip to content

feat(#792): add upload-ui-extensions action#798

Open
Vedd-Patel wants to merge 5 commits intomedic:mainfrom
Vedd-Patel:upload-ui-extensions
Open

feat(#792): add upload-ui-extensions action#798
Vedd-Patel wants to merge 5 commits intomedic:mainfrom
Vedd-Patel:upload-ui-extensions

Conversation

@Vedd-Patel
Copy link
Copy Markdown

@Vedd-Patel Vedd-Patel commented Mar 28, 2026

Description

Fixes #792
Added a new upload-ui-extensions action to the CLI. UI extensions require both a .js and .properties.json file.

This action reads the ui-extensions directory, validates the .properties.json via Joi (ensuring type is either app_main_tab or app_drawer_tab, and checking types for title, icon, roles, and config), and pushes them to the medic database under the ui-extension:${fileName} ID. The .js file is stored as an attachment. It also supports targeted uploads (e.g., cht upload-ui-extensions -- delivery) utilizing environment.extraArgs.

The core logic has been extracted into src/lib/upload-ui-extensions.js for better code management, and unit tests have been included to verify directory and file parsing behavior.

Note: I am currently familiarizing myself with the codebase conventions and commenting styles. Please let me know if there are any suggestions or improvements I can make regarding my code style!

medic/cht-conf #792

Code review items

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or integration tests where appropriate
  • Backwards compatible: Works with existing data and configuration. Any breaking changes documented in the release notes.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@Vedd-Patel
Copy link
Copy Markdown
Author

@dianabarsan @jkuester
i'm new to SonarCloud, so it took me a little extra time to familiarize myself with the code quality checks, but everything is passing now!
could you please review this PR when you have a chance?

@ChinHairSaintClair
Copy link
Copy Markdown
Contributor

ChinHairSaintClair commented Mar 30, 2026

Hi @Vedd-Patel, thank you for taking the time to create this feature. And so quickly too! I'm not an official reviewer, just looking through this out of interest (while working on some other CHT conf tasks), so feel free to ignore anything off-base. Mostly trying to understand some of the decisions made here.

  1. I noticed optional chaining used in one place (line 11 in fn/upload-ui-extensions.js), but || instead of ?? in another (line 91 in lib/upload-ui-extensions). Was that intentional? I think the ES version that comes with node >= 20 should support that.
  2. Is there a reason for reimplementing directory reading here instead of using attachmentsFromDir (as in upload-extension-libs line 16)? It seems like that would give you path construction, existence checks, and recursion out of the box.
  3. I didn't see a test case for missing .js or .properties files. Was this intentional? Noticed that the scripts throws if the files are missing.
  4. Reading/parsing files can throw (permissions, IO, invalid JSON). Should this be wrapped in a try/catch for clearer error messaging, or is fail-fast the intended behaviour?
  5. Since validation is being introduced, would it make sense to add a test for invalid .properties entries?
  6. Would it be useful to include test cases similar to those in extension-libs (e.g. 'does nothing if doc matches remote', 'should update doc when attachments found'), since those outcomes seem supported here as well?
  7. If uploaded items can be constrained to a select few, should that functionality also be tested to yield the expected results?
  8. Lastly, do you have an example of what a correct js and props file would look like?

@jkuester jkuester self-requested a review March 30, 2026 17:15
@Vedd-Patel
Copy link
Copy Markdown
Author

@ChinHairSaintClair
Thanks so much for taking the time to look through this! official reviewer or not, i really appreciate the detailed feedback and fresh eyes on the code!!

here are my thoughts on your points:

  • ?? vs ||: good catch over here! i'll swap the || to ?? to make sure it only falls back on null or undefined. definetely an oversight on my part, and it's much better to align with modern Node standards here.

  • reimplementing directory reading vs attachmentsFromDir: i actually went with a custom implementation here intentionally. attachmentsFromDir is awesome for scooping up arbitrary files, but UI extensions have much stricter requirements. We specifically need to pair the .js and .properties.json files together, read the JSON, and run it through Joi validation before anything gets pushed. Since attachmentsFromDir doesn't support that kind of pre-flight parsing and pairing out of the box, I had to write custom logic for it.

  • missing files & fail-fast behavior: the script throwing an error on missing files or invalid JSON is actually by design. for a CLI deployment tool, we want it to fail-fast and halt the process so we don't accidentally push incomplete or broken extensions to couchDB. that being said, you are completely right about the error messaging,a raw stack trace isn't great UX. i'll wrap the file I/O in a try/catch block so it outputs a clear, user-friendly error message instead.

  • test coverage: these are great suggestions. i'll expand the test suite to cover missing files, invalid Joi validation entries, the targeted upload functionality, and the diffing behavior (ensuring it doesn't update if the doc matches the remote).

  • example files: for sure! hre’s a quick look at what a valid setup looks like for a 'delivery' extension.

delivery.properties.json

{
  "type": "app_main_tab",
  "title": "Delivery Tracker",
  "icon": "icon-mother-child",
  "roles": ["chw", "nurse"],
  "config": {
    "url": "[https://custom-app.example.org](https://custom-app.example.org)"
  }
}

delivery.js*

(function() {
  window.addEventListener('cht-extension-load', (event) => {
    if (event.detail.extensionId === 'delivery') {
      const container = document.getElementById(event.detail.containerId);
      container.innerHTML = `<h2>Delivery Tracker Loaded</h2>`;
    }
  });
})();

@ChinHairSaintClair
Copy link
Copy Markdown
Contributor

ChinHairSaintClair commented Mar 31, 2026

@Vedd-Patel awesome, glad we're able to chat through the approach 🙂 really appreciate you being open to discussion.

Re: attachmentsFromDir yeah, I thought I might have missed something. Makes sense, thank you for clarifying.

In the spirit of extensions having strict requirements and failing early where necessary, perhaps a folder-based structure could help? For example:

ui-extensions/
  extension1/
    code.js
    props.properties.json
  extension2/
    code.js
    props.properties.json

This structure naturally groups related files and makes filtering by extension name a little easier. It would also support statically named "entry points", and leaves room for additional assets like images/audio/etc., which I vaguely remember discussing in one of the squad calls. Taking this approach now could save us from having to implement two capture modes later.

Following this, the attachmentsFromDir could then work (but isn't necessary), we might just need to enhance it with lazy loading to avoid eagerly reading large files. Conceptually:

module.exports = (dir, lazy = false) => {
...
attachments[attachmentPath] = lazy ? () => attachmentFromFile(file) : attachmentFromFile(file);

From there, it seems feasible to flatten getNamesToUpload / getExtensionDoc into a single pass, grouping by folder name, validating that each extension includes the required files, and halting early if validation fails.

For example, processing could be done like so:

const extensions = {};
const entries = Object.entries(attachmentFromFile(uiExtensionsDir));
const subset = specificExtensions?.length 
  ? names.filter(name => specificExtensions.includes(name)) 
  : [];

for (const [key, value] of entries) {
  const extensionName = key.substring(0, key.indexOf('/'));
  if (subset.length > 0 && !subset.includes(extensionName)){
    continue;
  }

  if (!(extensionName in extensions)) {
    extensions[extensionName] = {};
  }

  const file = key.substring(key.indexOf(extensionName) + extensionName.length + 1);

  switch (file) {
    case 'code.js':
      extensions[extensionName]['code'] = value;
      break;
    case 'props.properties.json':
      const propsContent = JSON.parse(value().data);
      const validation = schema.validate(propsContent);
      if (validation.error) {
        throw new Error(`Validation error for UI extension "${extensionName}": ${validation.error.message}`);
      }
      extensions[extensionName]['props'] = propsContent;
      break;
    default:
      extensions[extensionName][file] = value;
  }
}

A conceptual getExtensionDoc could then enforce the presence of code and props:

const getExtensionDoc = (key, obj) => {
  if (!('code' in obj) || !('props' in obj)) {
    throw new Error(`UI Extension "${key}" is missing either its .js or .properties file.`);
  }

  let code;
  try {
    code = obj.code();
  } catch(e) {
    throw new Error(`Unable to load JS content for UI extension "${key}"`);
  }

  return {
    _id: `ui-extension:${key}`,
    type: 'ui-extension',
    ...obj.props,
    _attachments: {
      'extension.js': {
        content_type: code.mime ?? 'application/javascript',
        data: code.data
      },
      // attach other media assets here ?
    }
  };
};

I haven’t tested this locally yet. Would love your thoughts on whether this approach seems reasonable.

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.

Add cht-conf support for upload-ui-extensions action

3 participants