SCENARIO: Delete a picture from a document (see#187). #283

Sidqui-Youssef · 2025-04-29T18:22:35Z

We, @Sidqui-Youssef @lucas-rubagotti @Amine-jabote @FredWantou @IlanPin @nasschml , hereby grant to Hyperglosae maintainers the right to publish our contribution under the terms of any licenses the Free Software Foundation classifies as Free Software Licenses.

benel · 2025-04-30T14:10:34Z

Thank you @Sidqui-Youssef and @Amine-jabote for your scenario.

Please note that your scenario should be in the frontend/scenario folder instead of the feature folder (the latter is deprecated, it was to be used with Capybara instead of Cypress test framework).

Co-authored-by: Amine-jabote <[email protected]>

… (see #199). Co-authored-by: lucas-rubagotti <[email protected]>

benel · 2025-05-26T14:52:34Z

Thank you for your contribution @lucas-rubagotti @IlanPin @nasschml.
Any idea why the tests are failing?
Do you need help?

Sidqui-Youssef · 2025-05-27T14:05:37Z

On remercie @lunatiique et @floporsch911 qui nous ont aidé pour la gestion de l'historique des commits .

benel

Thank you for your contribution @IlanPin and @nasschml (and the others for scenarios and tests).

However the implementation cannot be integrated for now as it still does not comply with the quality expectations of the project.

The related skill to be acquired in this course is named "Développer une fonctionnalité en minimisant l'impact des changements sur le code existant".

For now the last commit impacts 319 lines. This is HUGE for a single feature. If you look at others you will see it ranges from 20 to 80 lines. Please amend the code to comply with my first review and I will start a seconde one.

Regards.

benel · 2025-05-29T07:50:46Z

frontend/src/components/EditableText.jsx

@@ -1,103 +1,309 @@
+import { useState, useEffect, useCallback, useMemo, useRef } from 'react';


Please don't move the line from line 3 to line 1. This will make the diff more understandable (and hence easier to rebase and to understand blame view)

benel · 2025-05-29T07:52:16Z

frontend/src/components/EditableText.jsx

 import DiscreeteDropdown from './DiscreeteDropdown';
 import PictureUploadAction from '../menu-items/PictureUploadAction';
-import {v4 as uuid} from 'uuid';
+import { v4 as uuid } from 'uuid';


Please don't include changes unrelated with your contribution. This "pollutes" blame view and more lines means more conflicts when integrating your work.

benel · 2025-05-29T07:53:10Z

frontend/src/components/EditableText.jsx

+  setSelectedText,
+  backend,
+  setLastUpdate
+}) {


Please don't include changes unrelated with your contribution. This "pollutes" blame view and more lines means more conflicts when integrating your work.

Moreover, in this case, it makes it very difficult to locate if you added a parameter or not.

benel · 2025-05-29T07:54:02Z

frontend/src/components/EditableText.jsx

-  const [editedDocument, setEditedDocument] = useState();
-  const [editedText, setEditedText] = useState();
-  const PASSAGE = new RegExp(`\\{${rubric}} ?([^{]*)`);
+  const [editedDocument, setEditedDocument] = useState(null);


Please don't include changes unrelated with your contribution. This "pollutes" blame view and more lines means more conflicts when integrating your work.

In this cas, this means exactly the same (no parameter means a null parameter).

benel · 2025-05-29T07:55:03Z

frontend/src/components/EditableText.jsx

-  const [editedText, setEditedText] = useState();
-  const PASSAGE = new RegExp(`\\{${rubric}} ?([^{]*)`);
+  const [editedDocument, setEditedDocument] = useState(null);
+  const [editedText, setEditedText] = useState('');


Please don't include changes unrelated with your contribution. This "pollutes" blame view and more lines means more conflicts when integrating your work.
Moreover null is different from "". This could have unexpected effects.

benel · 2025-05-29T13:21:31Z

frontend/src/hyperglosae.js

      });

-  this.deleteDocument = ({_id, _rev}) =>
+  this.deleteDocument = ({ _id, _rev }) =>


Unrelated with the contribution.

benel · 2025-05-29T13:21:59Z

frontend/src/hyperglosae.js

      .then(x => x.userCtx?.name);

-  this.postSession = ({name, password}) =>
+  this.postSession = ({ name, password }) =>


Unrelated with the contribution.

benel · 2025-05-29T13:22:32Z

frontend/src/hyperglosae.js


  this.refreshMetadata = (id, callback) => {
-    this.getView({view: 'metadata', id, options: ['include_docs']})
+    this.getView({ view: 'metadata', id, options: ['include_docs'] })


Unrelated with the contribution.

Why haven't you fixed this?

benel · 2025-05-29T13:22:59Z

frontend/src/hyperglosae.js


  this.refreshContent = (id, callback) => {
-    this.getView({view: 'content', id, options: ['include_docs']})
+    this.getView({ view: 'content', id, options: ['include_docs'] })


Unrelated with the contribution.

Why haven't you fixed this?

benel · 2025-05-29T13:23:28Z

frontend/src/hyperglosae.js


  this.getAllDocuments = (user) =>
-    this.getView({view: 'all_documents', id: user || 'PUBLIC', options: ['include_docs']})
+    this.getView({ view: 'all_documents', id: user || 'PUBLIC', options: ['include_docs'] })


Unrelated with the contribution.

benel

@IlanPin @nasschml
Please refactor the code as requested if you want it to be integrated and your badges to be validated.

benel · 2025-06-16T06:58:34Z

frontend/src/components/EditableText.jsx

-    ? rawText.match(PASSAGE)[1]
-    : rawText;
+  // Memoized regex to extract our passage
+  const PASSAGE = useMemo(() => new RegExp(`\\{${rubric}} ?([^\\{]*)`), [rubric]);


In other words, you added useMemo although it had nothing to do with your contribution.

benel · 2025-06-16T06:59:14Z

frontend/src/components/EditableText.jsx

+          <svg viewBox="0 0 16 16">
+            <path d="M5.5 5.5A.5.5 0 0 1 6 6v6a.5.5 0 0 1-1 0V6a.5.5 0 0 1 .5-.5m2.5 0a.5.5 0 0 1 .5.5v6a.5.5 0 0 1-1 0V6a.5.5 0 0 1 .5-.5m3 .5a.5.5 0 0 0-1 0v6a.5.5 0 0 0 1 0z"/>
+            <path d="M14.5 3a1 1 0 0 1-1 1H13v9a2 2 0 0 1-2 2H5a2 2 0 0 1-2-2V4h-.5a1 1 0 0 1-1-1V2a1 1 0 0 1 1-1H6a1 1 0 0 1 1-1h2a1 1 0 0 1 1 1h3.5a1 1 0 0 1 1 1z"/>
+          </svg>`;


Why haven't you fixed this?

benel · 2025-06-16T06:59:22Z

frontend/src/components/EditableText.jsx

+        fig.appendChild(trash);
+        fig.addEventListener('mouseenter', () => (trash.style.opacity = '1'));
+        fig.addEventListener('mouseleave', () => (trash.style.opacity = '0'));
+        trash.addEventListener('click', () => {


Why haven't you fixed this?

benel · 2025-06-16T06:59:34Z

frontend/src/components/EditableText.jsx

+      });
+    };
+
+    const obs = new MutationObserver(attachTrash);


Why haven't you fixed this?

benel · 2025-06-16T06:59:58Z

frontend/src/hyperglosae.js


  this.refreshMetadata = (id, callback) => {
-    this.getView({view: 'metadata', id, options: ['include_docs']})
+    this.getView({ view: 'metadata', id, options: ['include_docs'] })


Why haven't you fixed this?

benel · 2025-06-16T07:00:23Z

frontend/src/hyperglosae.js


  this.refreshContent = (id, callback) => {
-    this.getView({view: 'content', id, options: ['include_docs']})
+    this.getView({ view: 'content', id, options: ['include_docs'] })


Why haven't you fixed this?

nasschml · 2025-06-16T10:30:10Z

We have decided to keep the use of MutationObserver in the EditableText component. After further review, we realized that removing it would have required major refactoring of related logic. We have now made sure that everything not related to our contribution remains untouched.

To integrate the React Bootstrap Icons trash component cleanly into the DOM, we imported both react-dom (for createRoot) and react-bootstrap-icons. We refactored the relevant useEffect to use the component instead of injecting raw SVG code. The associated CSS was updated to style the new button as required (bottom-right, with a dark background).

In addition, we updated the event test (event.js). The workaround with manual click propagation is no longer needed, since our click handling is now fully compatible with Cypress.

All changes are now limited strictly to our contribution, and the commit history is clean.

benel · 2025-06-16T15:46:35Z

@IlanPin @nasschml Here is a prompt you can type into ChatGPT:

Pourquoi le responsable du code d'un projet en React refuse d'intégrer mon code parce qu'il contiendrait des appels aux méthodes querySelectorAll, querySelector, document.createElement, appendChild, observe, closest, getAttribute ?

It will both explain you why these are bad practices and give you the rules you can apply to correct your code.

Co-authored-by: lucas-rubagotti <[email protected]>

nasschml · 2025-06-16T18:14:58Z

@benel

Thank you for your feedback regarding the use of DOM methods like querySelectorAll, querySelector, document.createElement, etc.

We now fully understand the importance of maintainability and React best practices:
Direct DOM access can break the React rendering flow, It makes the code less predictable and harder to maintain, Pure React logic ensures better state management and component reusability.

With the latest commit, all logic for displaying and deleting images is now handled 100% in a React-friendly way:
Images are extracted from the markdown and rendered using .map() in the JSX,Trash icons are attached to each image purely through React,No more direct DOM manipulation everything relies on React state and callbacks.

Everything works as expected on my local setup (display, image deletion, etc.), but some tests still fail in the CI pipeline. I will continue investigating this on my side. In the meantime, I am open to any feedback or suggestions you may have to further improve the React implementation.

benel

Thank you for your edits @nasschml and @IlanPin.

Please:

remove unrelated line edits from your commit,
don't use a name with an uppercase for a variable,
refactor your contribution into one (or two) distinct components.

benel · 2025-06-16T18:04:30Z

frontend/src/components/EditableText.jsx

 import {v4 as uuid} from 'uuid';

-function EditableText({id, text, rubric, isPartOf, links, fragment, setFragment, setHighlightedText, setSelectedText, backend, setLastUpdate}) {
+function EditableText({ id, text, rubric, isPartOf, links, fragment, setFragment, setHighlightedText, setSelectedText, backend, setLastUpdate }) {


This edited line is not related with your contribution.

benel · 2025-06-16T18:05:52Z

frontend/src/components/EditableText.jsx

      </div>
      <DiscreeteDropdown>
-        <PictureUploadAction {... {id, backend, handleImageUrl}}/>
+        <PictureUploadAction {...{id, backend, handleImageUrl}} />


This edited line is not related with your contribution.

benel · 2025-06-16T18:08:07Z

frontend/src/components/EditableText.jsx

+      )}
    </div>
  );
+


This edited line is not related with your contribution.

benel · 2025-06-16T18:09:10Z

frontend/src/components/EditableText.jsx


+  const imageRegex = /!\[([^\]]*)\]\(([^)]+)\)/g;
+  let images = [];
+  let Text = text || '';


Upper case names are usually used for classes and not for variables.

benel · 2025-06-16T18:09:40Z

frontend/src/components/EditableText.jsx

+  while ((match = imageRegex.exec(Text)) !== null) {
+    images.push({ alt: match[1], src: match[2] });
+  }
+  Text = Text.replace(imageRegex, '');


Upper case names are usually used for classes and not for variables.

benel · 2025-06-16T18:11:24Z

frontend/src/components/EditableText.jsx

+    const clean = t => (t || '').replace(mdRx, '').replace(/\n{2,}/g, '\n\n').trim();
+    if (internal) {
+      backend.deleteAttachment(id, name, res => {
+        if (!res.ok) return alert('Error deleting attachment.');


Don't show notifications to users if you don't provide solutions for them to fix the problem.

benel · 2025-06-16T18:12:01Z

frontend/src/components/EditableText.jsx

      <div className="formatted-text" onClick={handleClick}>
-        <FormattedText {...{setHighlightedText, setSelectedText}}>
-          {text || '&nbsp;'}
+        <FormattedText {...{ setHighlightedText, setSelectedText }}>


This line is not related with your contribution.

nasschml · 2025-06-16T19:58:57Z

@benel

About the conditional FormattedText renderingWe had to adjust the prop passed to to avoid image duplication, and this change is not unrelated to our contribution.

Here’s why:The markdown string we receive in text may contain image tags (e.g. ).If we simply map and display the images below, but keep passing the original text (with images inside) to , those images are rendered twice: once by the markdown parser inside , and once as our mapped images (with the trash icon for deletion).This is a direct UX regression (duplication of images), and also prevents correct linking of the trash/delete functionality to each image, since the markdown renderer doesn't expose the React events we need.

To maintain a clean UI and the expected behavior (edit/delete for each image), we must strip images from the string we send to whenever we also display them in our mapped list.
If we don’t conditionally clean the string, we get either missing images or duplicates, both are blockers for the feature and for Cypress/Capybara tests.So, this line is part of the contribution and necessary for a maintainable and predictable UI.

It is not possible to implement the feature properly without this small conditional, unless we modify the markdown renderer itself (which would be a much heavier change).

Also, I have committed this feature several times, and it works correctly when I run the tests on my local machine. However, the tests keep failing on the Git pipeline. I am still checking what could be causing this difference between environments, but wanted to highlight that everything works locally for me.

Co-authored-by: Nasschml <[email protected]>

benel mentioned this pull request May 2, 2025

187 - Delete a picture from a document #210

Closed

Sidqui-Youssef force-pushed the feat-187 branch 2 times, most recently from 2d4fa8b to 593d936 Compare May 6, 2025 11:13

IlanPin force-pushed the feat-187 branch from 593d936 to aeaddae Compare May 6, 2025 13:36

lucas-rubagotti force-pushed the feat-187 branch from e742d17 to 593d936 Compare May 6, 2025 13:43

IlanPin force-pushed the feat-187 branch from 593d936 to aeaddae Compare May 6, 2025 13:50

FredWantou force-pushed the feat-187 branch from 03092f0 to e99d85c Compare May 13, 2025 13:14

IlanPin force-pushed the feat-187 branch 2 times, most recently from 7a9c4be to 52a5fcb Compare May 13, 2025 15:14

Sidqui-Youssef and others added 2 commits May 13, 2025 17:15

SCENARIO: Delete a picture from a document (see #187).

2d736b6

Co-authored-by: Amine-jabote <[email protected]>

PROCESS: Upload image in document may be tested with cypress-cucumber…

ec780eb

… (see #199). Co-authored-by: lucas-rubagotti <[email protected]>

IlanPin force-pushed the feat-187 branch from 52a5fcb to 2f93440 Compare May 13, 2025 15:16

benel marked this pull request as ready for review May 26, 2025 14:50

benel self-requested a review as a code owner May 26, 2025 14:50

FredWantou force-pushed the feat-187 branch 4 times, most recently from 34cc072 to 274098c Compare May 27, 2025 13:57

benel requested changes May 29, 2025

View reviewed changes

nasschml force-pushed the feat-187 branch 2 times, most recently from bc0fdaf to ba21230 Compare June 15, 2025 11:53

benel requested changes Jun 16, 2025

View reviewed changes

nasschml force-pushed the feat-187 branch from ba21230 to 2f61b7a Compare June 16, 2025 10:23

nasschml force-pushed the feat-187 branch from 2f61b7a to ff54fb7 Compare June 16, 2025 17:56

TEST: Delete a picture from a document (see #187).

2eea2cf

Co-authored-by: lucas-rubagotti <[email protected]>

nasschml force-pushed the feat-187 branch 2 times, most recently from 5688511 to 2c55c78 Compare June 16, 2025 18:04

benel requested changes Jun 16, 2025

View reviewed changes

nasschml force-pushed the feat-187 branch 7 times, most recently from caddc1a to 201e3d2 Compare June 16, 2025 19:57

FEATURE: Delete a picture from a document (closes #187).

00ed1b0

Co-authored-by: Nasschml <[email protected]>

nasschml force-pushed the feat-187 branch from 201e3d2 to 00ed1b0 Compare June 17, 2025 15:59

		@@ -1,103 +1,309 @@
		import { useState, useEffect, useCallback, useMemo, useRef } from 'react';

SCENARIO: Delete a picture from a document (see#187). #283

Are you sure you want to change the base?

SCENARIO: Delete a picture from a document (see#187). #283

Uh oh!

Conversation

Sidqui-Youssef commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benel commented Apr 30, 2025

Uh oh!

benel commented May 26, 2025

Uh oh!

Sidqui-Youssef commented May 27, 2025

Uh oh!

benel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nasschml commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benel commented Jun 16, 2025

Uh oh!

nasschml commented Jun 16, 2025

Uh oh!

benel left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Sidqui-Youssef commented Apr 29, 2025 •

edited

Loading

nasschml commented Jun 16, 2025 •

edited

Loading

benel left a comment •

edited

Loading