Skip to content

Conversation

@tigloo
Copy link
Contributor

@tigloo tigloo commented Jun 27, 2025

This feature adds bulk import of resources to Fire Scribe. So far this is poorly tested but want to merge the feature and fix in subsequent PRs in case any bugs exist.

Note that even though the screen is named "ImportExportPage", I removed the export feature because it is very complicated and proprietary with Microsoft. I plan to re-add it later on, so for now I just changed the strings to avoid showing "export" anywhere.

image

This PR includes basic chores to update packages, so has unrelated code changes because of breaking changes in packages.

Most notable change is go_router which requires mixins to be generated, requiring relocation of the route declaration into the central routes.dart file.

@tigloo tigloo requested review from bartekwk2 and sh1l0n June 27, 2025 12:15

Choose a reason for hiding this comment

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

issue (non-blocking): updating dependencies can introduce bugs into the code base, especially when incrementing major versions (looks like we had to make some changes in our pagination). If this does end up introducing a bug into the codebase, there is no way to roll back without also undoing your import/export changes.
suggestion: update dependencies in a separate PR

// Import button
FilledButton.icon(
onPressed:
_urlController.text.trim().isNotEmpty ? handleImport : null,

Choose a reason for hiding this comment

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

issue (blocking): Import button doesn't work properly

The urlController doesn't trigger a rebuild of the widget when it is updated, so this button doesn't get 'enabled' correctly.

suggestion: use the textfield onChanged to set a local stateful variable, or add a listener to the urlController to trigger a rebuild.

adding an empty setState listener to the urlController (as below) would trigger the rebuild, and it's less invasive than refactoring the whole widget to get rid of the urlController and replace it with a stateful variable. One could argue that this is a bit hacky, as we are then causing the entire page to rebuild instead of just that button - but for a page like this I don't see that being problematic.

  @override
  void initState() {
    super.initState();
    _urlController.addListener(_onUrlChanged);
  }

  void _onUrlChanged() {
    setState(() {});
  }

  @override
  void dispose() {
    _urlController.removeListener(_onUrlChanged);
    _urlController.dispose();
    _urlFocusNode.dispose();
    super.dispose();
  }


@override
Widget build(BuildContext context) {
return Scaffold(

Choose a reason for hiding this comment

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

nit: scaffold isn't needed here afaik

"type": "text",
"placeholders": {}
},
"ndjsonFileToImport": "NDJSON file to import",

Choose a reason for hiding this comment

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

suggestion (non-blocking): update this text and description to reference URL rather than file

This makes it clearer that we are performing an import from another hosted fhir server, rather than a NDJSON file that the user might have locally saved.

}

// Make the import request using the underlying Dio client
final response = await fhirClient.dio.post(
Copy link

@kstrathdee-evoleen kstrathdee-evoleen Jun 27, 2025

Choose a reason for hiding this comment

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

question: it looks like we are only authenticating against the server we are importing into, but not against the server that we are importing from. How involved would it be to add some type of auth here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since in this case we submit a URL to the server, the server needs to be able to access the file stored at the specified URL. Microsoft FHIR requires a public URL for this (no joke).

I'm using signed URLs that time out after 1h or so, that way the exposure is very limited. But it's definitely not fun if you have a large patient database that you need to import.

);

if (fhirClient == null) {
// TODO: Show error message

Choose a reason for hiding this comment

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

todo: did you mean to leave this todo here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean to implement a dialog here, ended up deciding against it because it's a highly unlikely internal state problem and then forgot to remove the comment. Will fix.


class _ResourcePaginatedListState extends State<ResourcePaginatedList> {
final pagingController = PagingController<int, Resource>(firstPageKey: 0);
// final pagingController = PagingController<int, Resource>(firstPageKey: 0);

Choose a reason for hiding this comment

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

todo: remove commented out code

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.

3 participants