feat(server): add support for drag-drop image upload

[thseiler]

One area where StrictDoc is a bit unintuitive is the use of images:

• filenames cannot have spaces, or the reST parsing might break
• the folder needs to be named exactly "_assets", not "images", "figures", "diagrams" or "pictures"

This PR introduces an image upload mechanism to make things a little easier for new users.

When editing a requirement, it is now possible to:

• drag-and-drop an image from the local filesystem into the statement text edit, or
• paste an image from the clipboard there
• and a reST image directive is inserted automatically at the current cursor position, while the image upload is being handled in the background. Spaces in filenames are automatically replaced with underscores.
• To support the use of the wildcard enhanced image directive, it is possible to drag both the raster (i.e. .png) and the vector (.svg) file in one operation, and the resulting reSt directive will have the wildcard.

While working on this, I realised that the current approach to placing images in the relative _assets folder is not ideal, especially when requirements are moved around. I will try to address this in a subsequent PR...

[mettta]

@thseiler, thanks for picking up this feature. I’ve wanted this for a long time.

I tested the new image upload flow and noticed three problematic cases.

(1) Pasting from the clipboard can create two image blocks instead of one when the pasted image has a default name such as image.png or image.jpg. One block keeps that name, the other uses a generated name like pasted_<uuid>.png.

This reproduces when pasting image data from the clipboard or when pasting an image file already named image.png / image.jpg.
Pasting an image file with a normal filename produces only one image block.

Looking at the code, it seems that handleImagePaste() iterates over the same pasted_items collection twice and appends image files to imageFiles in both loops. In the second loop, files named image.png or image.jpg are renamed to pasted_<uuid>..., which results in two image directives for a single paste action.

(2) When inserting an image into a newly created node before the node is saved for the first time, the editor shows .. image:: Uploading ..., but after saving the image is broken because the file was never uploaded.

It looks like the client sends the upload request immediately, while upload_asset() expects requirement_mid to already exist in the traceability index. The new node is not yet present there, so the upload fails. The image directive stays in the node text, but the file was never stored on the server.

(3) If a node already contains an older .. image:: Uploading ... placeholder and I insert another image later in the same field, the older placeholder gets updated instead of the new one.

It looks like the code replaces text in the whole field (content.replace(text, rst)), so the replacement is not tied to a specific placeholder.

[mettta]

Small formatting note: the JS file has some trailing spaces and whitespace-only lines, and at least one place where the brace style differs, e.g.

  async function handleAssetUpload(editable, files)
  {
    const uniqueStems = ...

This might just be an editor setting, but it would be great to clean those up too.

[mettta]

I also tested the upload failure path by forcing upload_asset() to return HTTPException(status_code=500, detail="test").

In that case the UI does not replace .. image:: Uploading ... with \n**[Image upload failed]**\n.

It looks like the catch block calls content.replace(placeholder, ...), but placeholder there is the placeholder object rather than the placeholder text, so the replacement never happens.

[mettta]

Not for this PR necessarily, but maybe something to keep in mind for later (@stanislaw).

Unless I’m missing something, this looks like it can leave orphaned files in _assets.

upload_asset() writes the file to _assets/<requirement_mid>/... before the node is saved. If the form is later cancelled or fails validation, the uploaded file does not seem to be cleaned up.

[mettta]

The new E2E test covers the happy path well, but it might be worth extending coverage to the cases mentioned above.

A test for the upload failure path would also be useful. That may require some extra test support to simulate a server error (? @stanislaw), so it could also be tracked separately.

[thseiler]

@mettta Thanks a lot for the review & the detailed feedback!

(1) This was a copy-paste error; there should have been only one loop there! Sorry, my mistake...
(2) I adapted upload_asset() to accept assets even if the supplied requirement_mid does not yet exist in the traceability index. This allows uploading images when creating a new requirement, but can leave orphaned files in _assets if the requirement is not created. To help the user keep their repos clean, I have added a CLI command to list/clean up the unused assets...
(3) I have switched to using createTextNode() for adding the rst directives; this should avoid the string replacing issues, and work better for error feedback, too.

I also fixed the formatting of the stimulus controller and added some more test cases to cover:

• upload when creating a new requirement
• copy&paste
• the error case (here I use an empty file to trigger an error)

[mettta]

@thseiler Looks good, thanks for the changes.

I noticed the help text in ManageAssetsCommand() says "Manages project assets (images, attachments)", but the implementation only looks for .. image:: references and image files.

Should the description be updated to mention images only, or is support for attachments still expected here?

[mettta]

@thseiler By the way, not a blocker for the merge - just something I noticed.

I’m not seeing this take effect in any browser:

/* Change cursor to 'not-allowed' during a drag */
body:has(.is-dragging) {
  cursor: no-drop;
}

In my tests, the cursor just stays default during a file drag ((it only changes to 'copy' over editable).

Did you manage to get this working on your side?

As mentioned, not a blocker - the current behavior is fine as is. We can revisit this later in a separate PR.

[thseiler]

@mettta The CSS was an attempt to prevent flicker; without it, I would get flicker between the copy icon and the slash icon when dragging over a single-line editable. I found that the flicker was happening due to a missing "dragenter" handler in the controller. I added it and removed the redundant CSS.

I updated the help text for the CLI command to indicate that this is images-only.

I also noticed that when editing two nodes simultaneously, all uploaded images would be erroneously be attributed to the first node, due to const requirement_mid = document.getElementById('requirement_mid').value returning the first hit. I fixed that as well.

[thseiler]

I noticed that I also need to handle the "@asset" macro for rst & json export. I hope to address this in the coming days...

[stanislaw]

@thseiler this is great work! I only left a few small comments and would like to ask you:

I have started moving all low-level StrictDoc implementation requirements to the L3/LLR document. Would it be too much if I asked you to create a few L3/LLRs to support the work that you did here?

You could create a section about this drag-and-drop paste feature in L3 and add a small! number of essential requirements that govern your implementation. You could use the existing Rust or Markdown sections for inspiration.

If your spare time budget is too short for this work, I could do this myself after we merge your MR. What you did is already a great contribution, and I am happy to merge it without you also having to do the LLRs.

[thseiler]

Thanks! I've written some L3 draft requirements for the image upload. I couldn't find, however, good parent requirements in L1/L2, so I left these unlinked. While writing them, I also realised that my implementation was incomplete in many edge cases. Therefore:

• I moved the @asset macro expansion to markup_renderer.py, so that it applies to Markdown as well.
• I added support for the document markup type to editable_field_controller, so that it inserts the correct directive.
• It now also checks if the image is already present and won't insert it a second time, this is to support image replacement.
• I fixed the strictdoc manage assets CLI to identify assets in HTML and Markdown documents as well.
• I added some more tests and also tried to add @relation markers to the code where appropriate.

The implementation is now more complete, but you may need to re-review some of these areas.

Please feel free to adapt or rewrite the L3 requirements to better align with the project’s guidelines.

[stanislaw]

Thanks @thseiler for the extra effort. I will review your work very soon, and I did not even expect that you would uncover some incompleteness along the way 👍

The truth is that this recent L3 introduction is really based on common sense and the understanding that we needed a clear layer closest to the implementation. All precise linking and best practices for good L2-L3 traces are still a topic of exploration for me. This part:

I couldn't find, however, good parent requirements in L1/L2, so I left these unlinked.

I will handle myself and figure out what we actually want there 👍

[stanislaw]

I would also like to mention this:

I recently looked into how the Confluence API works, and it turns out that Confluence does not really manage outdated assets when they are removed from pages where they were previously uploaded. If you download a page via the Confluence API, especially an old 'veteran' page with a lot of content, experience shows that all attachments for that page include both current and outdated assets that are no longer in use.

It looks like the way you handle this with the manage ... command is a straightforward, StrictDoc-style way of dealing with this at the moment, and I cannot think of any better way of doing this.

[stanislaw]

@thseiler

I have pushed to a separate branch two integration tests:

https://github.com/strictdoc-project/strictdoc/compare/feature/drag-and-drop-images__itest_with_docs?expand=1

The test _30_ passes.

The test _31_ reproduces the case of StrictDoc's documentation:

I am testing the image uploading with StrictDoc's own documentation and it is creating a folder _assets at the root of StrictDoc's repo which is not how it was designed to work until now (StrictDoc's documents are in docs/ but the server export command uses . to read the docs and source files).

Do you have an idea how to make your branch to support this case? Should we introduce a config option dir_for_user_assets? And I would set it to docs/? 🤔

[thseiler]

@stanislaw

Many thanks for these test cases and for fixing the CI issues.

I think I have found a possible solution: We can associate a top-level _assets folder to every "document root" (i.e. one per entry in include_docs_path). I added an implementation and also updated the L3 requirements accordingly. This should work with existing projects, keep the _assets in their respective document trees, and also support StrictDocs' own documentation. I kind of like this approach, as it also makes it easier to split such projects later, should the need arise.

I realised that we don't need to support image uploads for nodes parsed from source code, since these are inherently read-only (autogen). I've now added an error message for when a user tries to upload an image to an autogenerated node, along with a test case to cover it.

To support image upload even in projects that don't use MIDs, I also implemented using the UID as a fallback.

A small sidenote: While working on this PR, I noticed that the UI allows users to modify autogen nodes, and the edits appear to work successfully (the in-memory model is updated), but the changes aren't persisted. (Ideally, the UI should indicate that edit functions are unavailable for such nodes. I hope to provide a separate PR in the coming days...)

[stanislaw]

@thseiler I had a short vacation and was away from coding for some time. Please give me some time to review your new changes and re-evaluate the entire PR once more. I will also check your html2pdf4doc change soon.

[stanislaw]

@thseiler I have integrated your smaller changes (the html2pdf4doc_python and Windows defender issues), feel free to rebase and remove those changes from your PR. Please also squash it to one commit on the way — it could help me review the next iterations locally.

Is there a chance you could join the office hours this week? I would like to discuss:

1. If there is a way to avoid introducing a new @assets macro. Also, if we want to customize the behavior of handling paths differently if starting from / (root) or without (relative).
2. If there are any implications of the overall algorithm when we develop StrictDoc to support multiple projects with multiple strictdoc_config.py files at the same time.

cc @haxtibal

[stanislaw]

Is there a reason for using getattr here instead of field.multiline?

[stanislaw]

Maybe it was me who introduced this typo but we probably should be doing .. image:: because that's how the directives usually look like.

[stanislaw]

Is there a reason for using getattr here instead of field.multiline?

[stanislaw]

Is there a reason for using getattr here instead of requirement_mode.autogen?

[stanislaw]

Is there a reason for using getattr here instead of requirement_mode.mid_permanent?

[stanislaw]

Is there a reason for using getattr here?

[stanislaw]

and here: why getattr?

[stanislaw]

I keep thinking about this new @assets and am wondering if we could avoid introducing it as a new custom construct and use your path resolution algorithm from paths based on _assets without @.

I am slightly concerned about the portability of this solution. Yes, StrictDoc has custom things like LINK or ANCHOR, but maybe we could avoid creating this new macro @assets in this case.