Repository navigation
feat(UX): improve document associations, previews and deletion navigation - #1552
Conversation
| <Box sx={{ display: 'flex', alignItems: 'stretch' }}> | ||
| <CardActionArea | ||
| onClick={handleClick} | ||
| {...(longPressHandlers ?? {})} |
There was a problem hiding this comment.
The long-press handlers are spread onto the whole CardActionArea, which also contains the selection Checkbox (line 116). That checkbox only guards onClick with stopPropagation — it has no touch-event guard, and onTouchStart/onTouchEnd still bubble up to the action area.
Scenario: in the document-association list, the user presses and holds on a row checkbox for 500 ms or more. At 500 ms useLongPress fires, longPressFiredRef is set and onRowClick(doc) opens the full document preview dialog. On release, the checkbox's own onChange still fires, so the single gesture both opens the preview and toggles the row's selection — one deliberate action produces two unrelated state changes.
Stopping touch propagation on the checkbox (or attaching the long-press handlers to the CardContent rather than the action area) would keep the two gestures separate.
There was a problem hiding this comment.
✅ Fixed — the checkbox now stops touch start/move/end/cancel propagation, so holding it cannot start the card's preview gesture. Added unit and Cypress coverage for a 600 ms checkbox press followed by selection.
Commit: 09ceb9eda.
| } | ||
| : null; | ||
|
|
||
| const handleClick = () => { |
There was a problem hiding this comment.
longPressFiredRef is only cleared in two places: the wrapped onTouchStart and this handleClick early return. Neither runs if the long-press is not followed by a click on this card.
Scenario: a long press fires and the preview dialog opens. The synthetic click that follows is retargeted to the dialog/backdrop, so handleClick never runs and the ref stays true. The dialog is closed, and the user then activates the same card without a new touch sequence — Enter/Space on the CardActionArea (it is a ButtonBase, so it is keyboard-focusable) or a mouse click on a hybrid touch/trackpad device. handleClick sees the stale true, swallows that first activation and only resets the flag, so the selection toggle silently does nothing and the user has to click twice.
Clearing the ref on onTouchEnd/onTouchCancel (after the click has had a chance to fire) or keying it off onMouseDown/onKeyDown as well would avoid the stale state.
There was a problem hiding this comment.
✅ Fixed — a fresh mouse/pen pointer press or keyboard activation resets the long-press suppression flag; touch cancellation also clears it. The compatibility click from the original touch remains suppressed. Regression tests cover the intercepted-click scenario followed by keyboard and mouse activation.
Commit: 09ceb9eda.
| }); | ||
| }; | ||
|
|
||
| export const useLinkDocumentToEntrances = () => { |
There was a problem hiding this comment.
useLinkDocumentToEntrances re-implements, line for line, the linkDocuments helper already defined at the top of this same file (lines 19-39): Promise.allSettled over the pairs, filter the rejections, attach error.rejections, rethrow the first reason. Only the URL builder and the loop variable differ.
The risk is divergence rather than a crash: a future fix to the partial-failure contract (e.g. aggregating messages, or switching to AggregateError) will be applied to one copy and silently missed in the other, so the two association flows will start reporting different things for the same kind of partial failure.
Worth extracting a single associateAll(pairs, urlFor) helper in this module and having both linkDocuments and this new hook call it.
There was a problem hiding this comment.
✅ Fixed — both batch association mutations now call one associateAll helper. It waits for every request, attaches all rejection reasons to the first error, and preserves the existing cache invalidation behavior. The association tests cover success and partial failure in both directions.
Commit: 09ceb9eda.
| onSuccess: () => { | ||
| if (isPermanent) { | ||
| navigate( | ||
| getPostDeletionUrl( |
There was a problem hiding this comment.
This onDeletePress body is now duplicated verbatim in five places — components/appli/Entry/index.jsx:117, components/appli/Massif/Massif.jsx:100, components/appli/Network/index.jsx:99, components/appli/Organization/index.jsx:101 and pages/DocumentDetails/index.jsx:215 — with only the entity id, the DELETED_ENTITIES key and the redirectTo accessor changing. Person.jsx is a sixth, slightly drifted copy (always permanent, no onError rollback).
That drift is the concrete risk: Person.jsx already lacks the onError handler that resets wantedDeletedState, so a failed person deletion leaves the card stuck in its optimistic "deleted" state while the other five recover. Any further change to the post-deletion contract has to be made six times and will drift again.
Since getPostDeletionUrl was extracted anyway, consider going one step further and extracting the whole thing — e.g. usePostDeletion(entityType, id, entity) returning onDeletePress plus the wantedDeletedState pair — so there is a single implementation to keep correct.
There was a problem hiding this comment.
✅ Fixed — extracted usePostDeletion and reused it in all six entity pages. It owns the desired deletion state, error rollback, and navigation after successful permanent deletion; each page still supplies its own mutation hook. Person deletion defaults to permanent. Regression tests cover replacement/existing-redirect/homepage navigation, failures, soft deletion, and state synchronization.
One factual clarification: the current Person component had no optimistic wantedDeletedState, so the specific stuck-deleted-card scenario was not present there. It now shares the same deletion callback contract as the other pages.
Commit: 09ceb9eda.
| startIcon={<DeleteForeverIcon />} | ||
| disabled={isSearchMandatory && !selectedEntity} | ||
| disabled={ | ||
| isCurrentEntity(selectedEntity, entityId) || |
There was a problem hiding this comment.
Two things about this guard.
First, isCurrentEntity(selectedEntity, entityId) is unreachable: handleSelection (line 239) refuses to store a self-selection, and the suggestion list is already filtered at line 231, so selectedEntity can never be the current entity by the time it reaches this disabled expression. It reads as a safety net but it is dead code, which makes the real invariant harder to see.
Second — and this is the user-visible part — because handleSelection silently drops a self-selection, picking your own entity in the autocomplete clears the input and leaves the form in exactly the state it was in, with no message. A moderator who types the current entity's name (an easy mistake when merging near-duplicates) sees the field blank itself and no explanation of why nothing happened.
Keeping the guard in one place and surfacing it — reject in handleSelection and show a short "cannot redirect to itself" helper text — would be both simpler and clearer.
There was a problem hiding this comment.
✅ Fixed — removed the redundant self-selection check from the confirmation button and kept rejection at the selection boundary, alongside suggestion filtering. A helper explains that an entity cannot redirect to itself; a stale self-selection highlights the field/helper and no longer clears the input silently. The message is translated in all 15 language files, and the regression test verifies rejection feedback and valid replacement selection.
Commit: 09ceb9eda.
There was a problem hiding this comment.
Fixed in ae9afa87b. I chose the permanent constraint hint: removed the unreachable hasSelfSelectionError state, its error colouring, and aria-live from that hint. The current entity is still filtered out, and the selection boundary keeps a defensive guard without inventing a user-visible error state.
Search failures now display a separate localized error message with role="alert", which disappears when the query recovers. The new message is translated in all 15 language files.
Removed the artificial stale-result button from the test mock. The tests now use the real AutoCompleteSearch, covering filtering with numeric/string IDs, selecting an available replacement, and search failure/recovery. All 3 tests pass; full repository lint passes with no errors or warnings.
| /> | ||
|
|
||
| {linkedEntities.length > 0 && ( | ||
| {(linkedEntities.length > 0 || |
There was a problem hiding this comment.
permissions.isAuth && !documentData.isDeleted && !hideActions is evaluated twice (here for section visibility, again at line 810 for the icon), and linkedEntities.length > 0 three times. A canAssociateEntrances const would remove the chance of the two copies drifting.
More concretely, the icon prop: when the condition is false this expression evaluates to false, not undefined. ScrollableContent renders the icon slot as {!isNil(icon) && <div role="presentation" onClick={...}>{icon}</div>}, and isNil(false) is false — so an empty presentational <div> with a click handler is injected into the section header for every anonymous visitor and every hideActions render (i.e. every time this page is shown inside the search preview dialog). Harmless visually, but it is a clickable empty element in the header. icon={cond ? <SectionCreateButton ... /> : undefined} avoids it.
There was a problem hiding this comment.
✅ Fixed — introduced canAssociateEntrances and hasLinkedEntities, and the unavailable action now passes undefined to the icon slot. Applied the same icon-slot correction to Entry/Documents. The entrance-section regression tests verify that the action slot disappears when editing/authentication is no longer allowed.
Commit: 09ceb9eda.
| } | ||
| content={ | ||
| <> | ||
| {isEntranceSearchVisible && ( |
There was a problem hiding this comment.
isEntranceSearchVisible is only ever reset by the toggle button and by onSuccess. Nothing resets it when the document stops being associable.
Scenario: a moderator opens the Associate panel, then soft-deletes the document from the action bar on the same page. documentData.isDeleted flips to true, so the icon condition at line 810 goes false and the toggle button disappears — but isEntranceSearchVisible is still true, so the SearchEntranceForm stays mounted with no way to dismiss it, and submitting it fires useLinkDocumentToEntrances against a now-deleted document.
Resetting the flag when the entity becomes deleted (or gating the content on the same condition as the icon) closes both the stuck-UI and the submit-after-delete paths. Note Entry/Documents has the same shape, so this may be a pre-existing pattern worth fixing in one place.
There was a problem hiding this comment.
✅ Fixed — the entrance search form is rendered only while the document is associable, using the same condition as its toggle. Pending deletion also closes the form. Entry/Documents now gates its search the same way when editing/authentication is lost. Cypress verifies closing an open association panel after document soft deletion; unit tests cover the entrance panel's permission changes.
Commit: 09ceb9eda.
Re-review of
|
|
Addressed the re-review in ae9afa87b.
Validation:
Also replied in the original inline thread. |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1552.westeurope.azurestaticapps.net |
Re-review of
|
|
@urien cette PR est prete pour etre testée à toutes fins utiles. Il y a plusieurs fonctionnalités d'amélioration UI/UX pour les contributeurs et admins/moderateurs |
|
Associate entrances from a document page. Depuis le formulaire d'édition d'un document je n'ai pas vu de champ avec les entrées associées Test fait avec https://orange-rock-0d4f87503-1552.westeurope.azurestaticapps.net/ui/documents/234372/edit De même quand je créé un nouveau document il n'y a pas de champ pour lier une entrée Pour tester les différents rôles j'ai utilisé l'outil disponible pour les admins |
|
Preview documents before associating them |
|
Improve document validation on narrow screens |
|
Navigate to the replacement after successful deletion |
|
Make network descriptions easier to discover Je suis allé sur https://orange-rock-0d4f87503-1552.westeurope.azurestaticapps.net/ui/entrances/1 qui est lié au réseau https://orange-rock-0d4f87503-1552.westeurope.azurestaticapps.net/ui/caves/75262 |
ClemRz
left a comment
There was a problem hiding this comment.
Re-review of 5eb6e0eb and 179775f4
Thanks, both changes are correct.
Verified
- The blocking bug from the last round is fixed. Caves are now
canUnlink: false(index.jsx#L366), andonUnlinkdispatches onentity.type(L852-L864). A massif can no longer be sent to the entrance endpoint. - Massif unlinking is wired up correctly.
useUnlinkDocumentToMassifgets{ massifId, documentId }, which matches its signature, and invalidates bothmassifKeys.detailanddocumentKeys.detail. - Gating both types behind
isModeratoris correct. In the API,entrance/unlink-document.jsandmassif/unlink-document.jsboth requirehasGroup(MODERATOR). yarn test: 180 files, 1234 tests pass.yarn lintis clean.
Blocking
No test covers the page-level fix. The new test in LinkedEntityCards.test.jsx#L70 passes hand-written entities with canUnlink already set. It tests that the component respects the flag, not that DocumentDetails sets it correctly. The bug was in the page's config and callback.
To check, I put back the pre-fix DocumentDetails/index.jsx from fd28e7bd and ran the full suite. All 1234 tests still passed. So nothing in the suite would catch this bug if it came back.
DocumentDetails/index.jsx has no test file. Please add one, even a narrow one, covering the behaviour this PR changes. As a moderator viewing a document with a massif, a cave and an entrance linked:
- the cave has no unlink button;
- unlinking the massif calls the massif mutation with
{ massifId, documentId }; - unlinking the entrance calls the entrance mutation with
{ entranceId, documentId }; - a non-moderator sees no unlink buttons.
Mocking the two mutation hooks and the document query is enough. No real API calls are needed.
Nits (non-blocking)
- The comment above
canUnlinkLinkedEntities(L198) still mentions onlyentrance/unlink-document.js. It now coversmassif/unlink-document.jstoo. - The
return nullfor other types (L864) can't be reached today, because caves are opted out. If it ever is reached,LinkedEntityCardsawaits it and closes the dialog as if the unlink worked. Throwing there would make that case fail loudly.
179775f to
651d5f6
Compare
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1552.westeurope.azurestaticapps.net |
|
Addressed the latest review in 510321e78.
Validation:
|
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1552.westeurope.azurestaticapps.net |
ClemRz
left a comment
There was a problem hiding this comment.
Re-review of 510321e7
The last blocker is resolved. Approving.
Verified
- Rebase is clean.
git range-diffshows all 16 earlier PR commits are identical after the rebase ontodevelop. The only new work is510321e7. - The new page test catches the original bug. I put back the pre-fix
DocumentDetails/index.jsx(fromd97adff5) and ranindex.test.jsx. Two of its five tests fail: "offers unlink actions for massifs and entrances, but not caves" and "unlinks Vercors through its own mutation". Last round, the whole suite stayed green against that same file. - The test is well built. Using id
7for the massif, the cave and the entrance means that calling the wrong endpoint fails the test instead of passing by accident. CheckingotherMutationbefore and after confirming also pins down which endpoint gets called. - Both nits are addressed. The comment now cites both API controllers. The unsupported-type branch now throws.
LinkedEntityCards.handleUnlinkcatches the error and keeps the dialog open, so the unlink no longer looks like it succeeded. yarn test: 183 files, 1265 tests pass.yarn lintis clean.
Nit (non-blocking)
- The test names its fixture
document(L64), which hides jsdom's globaldocumentin this file. Nothing breaks today, because RTL uses its own reference. A name likedocumentDatawould avoid surprises if someone later adds a direct DOM call here.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Closes #1156 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The document page already lets users associate entrances but had no way to remove one once added. Wire the existing `useUnlinkDocumentToEntrance` mutation through `LinkedEntityCards` (which already renders the unlink button and confirmation dialog) and expose it on entrance entries only. The backend restricts the unlink endpoint to moderators in `entrance/unlink-document.js` (controller-level `hasGroup(MODERATOR)` check on top of the `tokenAuth` policy), so the gating here is narrower than the Associate side — the two must not be aligned on `isAuth`. Add an opt-out `canUnlink` field on `LinkedEntityCards` entities so massif and cave entries (whose unlink hooks are not wired yet) don't pick up the button; existing callers default to the previous behaviour.
On a deleted Description / Comment / History row, the three action buttons used icon + colour choices that no other deletion UI in the app shares with it: - "Permanently delete" rendered the `RemoveCircleRounded` glyph (red barred circle), while the entity-level `DeletedCard` and the shared `DeleteConfirmationDialog` use `DeleteForeverRounded`. Converge on the majority spelling. - "Restore" rendered in primary, giving it the same visual weight as the History button next to it and leaving the destructive "Permanently delete" without a hierarchy to stand against. `DeletedCard` sets its restore button to secondary for the same reason; mirror that. Icon-only density is preserved; only the shared tokens (icon + colour) move.
The opt-out canUnlink default meant massif and cave cards picked up the unlink button and reached the entrance mutation with a foreign id, usually a silent no-op and occasionally unlinking an unrelated entrance whose id happened to collide. Set canUnlink: false on massif and cave entries and guard the page callback on entity.type === 'entrance'.
The dispatch now routes on entity.type: entrance entries hit useUnlinkDocumentToEntrance, massif entries hit useUnlinkDocumentToMassif. Cave stays opt-out until its hook exists.
510321e to
1c3e9d6
Compare
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1552.westeurope.azurestaticapps.net |






feat(UX): improve document associations, previews and deletion navigation
🤔 What
Related to #1156.
major.minor.patchformat to its matching GitHub release. Development and preview version labels remain plain text.The new association labels, empty-state message, and selection guidance are translated across all 15 supported language files.
🤷♂️ Why
Contributors need to inspect documents before linking them and manage entrance associations directly while viewing a document, supporting the document verification workflow described in #1156. These changes also make document review easier on small screens, keep replacement navigation consistent after deletion, and improve the visibility of descriptions and release notes.
🔍 How
denseoption toStandardDialogand use MUI'ssmviewport breakpoint for fullscreen document dialogs. Mobile selection cards expose a separate preview button and reuse the long-press hook.deletedEntityRedirect.js, reuse that utility in deleted entity cards, and navigate from successful deletion callbacks.🔗 Related API PR
None required. The association flow uses an existing API endpoint.
🧪 Testing
yarn lintcompleted with no errors or warnings.node scripts/translations/sync-with-en.js --check.node scripts/translations/sort.js --check.git diff --check origin/develop...HEAD.associate-document-preview.cy.jspassed in headless Electron, checking desktop row preview, mobile card preview, and independent selection.📸 Previews
No screenshots attached.