From 4552bcbe1102fe61dcb65a967908257a8949ee3d Mon Sep 17 00:00:00 2001 From: Anthony Stirling <77850077+Frooodle@users.noreply.github.com> Date: Thu, 20 Aug 2026 11:16:39 +0100 Subject: [PATCH] Move usage-ranked tool suggestions to the post-operation panel --- .../api/ToolRecommendationController.java | 40 ---- .../model/ToolRecommendationDismissal.java | 52 ---- .../model/ToolRecommendationDismissalId.java | 19 -- ...ToolRecommendationDismissalRepository.java | 27 --- .../security/service/UserService.java | 5 +- .../service/ToolRecommendationService.java | 54 +---- .../api/ToolRecommendationControllerTest.java | 60 ----- .../ToolRecommendationRepositoriesTest.java | 48 ---- .../ToolUsageErasureDeletesUserTest.java | 7 - .../security/service/UserServiceTest.java | 5 +- .../ToolRecommendationServiceTest.java | 118 +-------- .../ToolUsagePostgresConcurrencyTest.java | 24 -- .../public/locales/en-GB/translation.toml | 6 - .../public/locales/en-US/translation.toml | 6 - .../src/core/api/toolRecommendations.test.ts | 48 +--- .../src/core/api/toolRecommendations.ts | 28 --- .../src/core/components/tools/ToolPicker.tsx | 60 +---- .../tools/shared/SuggestedToolsSection.tsx | 38 ++- .../tools/toolPicker/ToolButton.test.tsx | 125 ---------- .../tools/toolPicker/ToolButton.tsx | 42 ---- .../tools/toolPicker/ToolPicker.css | 17 -- .../src/core/hooks/useSuggestedTools.test.ts | 116 +++++++++ .../src/core/hooks/useSuggestedTools.ts | 101 ++++---- .../hooks/useToolRecommendations.test.tsx | 104 +------- .../src/core/hooks/useToolRecommendations.ts | 64 +---- .../src/core/hooks/useToolSections.test.tsx | 223 ------------------ .../editor/src/core/hooks/useToolSections.ts | 115 ++++----- 27 files changed, 241 insertions(+), 1311 deletions(-) delete mode 100644 app/proprietary/src/main/java/stirling/software/proprietary/model/ToolRecommendationDismissal.java delete mode 100644 app/proprietary/src/main/java/stirling/software/proprietary/model/ToolRecommendationDismissalId.java delete mode 100644 app/proprietary/src/main/java/stirling/software/proprietary/repository/ToolRecommendationDismissalRepository.java delete mode 100644 frontend/editor/src/core/components/tools/toolPicker/ToolButton.test.tsx create mode 100644 frontend/editor/src/core/hooks/useSuggestedTools.test.ts delete mode 100644 frontend/editor/src/core/hooks/useToolSections.test.tsx diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/controller/api/ToolRecommendationController.java b/app/proprietary/src/main/java/stirling/software/proprietary/controller/api/ToolRecommendationController.java index 32695e3ca3..adab4b539d 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/controller/api/ToolRecommendationController.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/controller/api/ToolRecommendationController.java @@ -5,7 +5,6 @@ import java.util.Optional; import java.util.regex.Pattern; import org.springframework.http.ResponseEntity; -import org.springframework.web.bind.annotation.DeleteMapping; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.RequestBody; @@ -19,7 +18,6 @@ import lombok.extern.slf4j.Slf4j; import stirling.software.common.annotations.api.ProprietaryUiDataApi; import stirling.software.common.service.UserServiceInterface; -import stirling.software.proprietary.model.ToolRecommendationDismissal; import stirling.software.proprietary.service.ToolRecommendationService; import stirling.software.proprietary.service.ToolRecommendationService.ToolRecommendation; import stirling.software.proprietary.service.ToolRecommendationService.ToolWorkflow; @@ -50,8 +48,6 @@ public class ToolRecommendationController { */ public record UsageRequest(String toolKey, List> priorChains) {} - public record DismissalRequest(String contextTool, String dismissedTool) {} - @GetMapping("/tool-recommendations") @Operation( summary = "Get recommended tools", @@ -117,42 +113,6 @@ public class ToolRecommendationController { return ResponseEntity.noContent().build(); } - @PostMapping("/tool-recommendations/dismissals") - @Operation( - summary = "Dismiss a recommended tool", - description = - "Never recommend dismissedTool again while on contextTool. Use context '*' to" - + " suppress it everywhere.") - public ResponseEntity dismiss( - @RequestBody DismissalRequest request, - @RequestHeader(value = "X-Browser-Id", required = false) String browserId) { - if (request == null || !isValidDismissal(request.contextTool(), request.dismissedTool())) { - return ResponseEntity.badRequest().build(); - } - recommendationService.dismiss( - resolvePrincipal(browserId), request.contextTool(), request.dismissedTool()); - return ResponseEntity.noContent().build(); - } - - @DeleteMapping("/tool-recommendations/dismissals") - @Operation(summary = "Undo a recommendation dismissal") - public ResponseEntity undoDismiss( - @RequestParam("contextTool") String contextTool, - @RequestParam("dismissedTool") String dismissedTool, - @RequestHeader(value = "X-Browser-Id", required = false) String browserId) { - if (!isValidDismissal(contextTool, dismissedTool)) { - return ResponseEntity.badRequest().build(); - } - recommendationService.undoDismiss(resolvePrincipal(browserId), contextTool, dismissedTool); - return ResponseEntity.noContent().build(); - } - - private static boolean isValidDismissal(String contextTool, String dismissedTool) { - return ToolUsageTrackingService.isValidToolKey(dismissedTool) - && (ToolRecommendationDismissal.ANY_CONTEXT.equals(contextTool) - || ToolUsageTrackingService.isValidToolKey(contextTool)); - } - /** Logged-in username, else a per-browser pseudo-identity, else a shared anonymous bucket. */ private String resolvePrincipal(String browserId) { String username = diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/model/ToolRecommendationDismissal.java b/app/proprietary/src/main/java/stirling/software/proprietary/model/ToolRecommendationDismissal.java deleted file mode 100644 index 13617cecce..0000000000 --- a/app/proprietary/src/main/java/stirling/software/proprietary/model/ToolRecommendationDismissal.java +++ /dev/null @@ -1,52 +0,0 @@ -package stirling.software.proprietary.model; - -import java.io.Serializable; - -import jakarta.persistence.Column; -import jakarta.persistence.Entity; -import jakarta.persistence.Id; -import jakarta.persistence.IdClass; -import jakarta.persistence.Index; -import jakarta.persistence.Table; - -import lombok.Getter; -import lombok.NoArgsConstructor; -import lombok.Setter; - -/** - * A principal's opt-out: never recommend {@code dismissedTool} while they are on {@code - * contextTool}. A context of {@code *} suppresses the tool in every context. The whole row is its - * own key, so saving one twice is a no-op rather than a duplicate. - */ -@Entity -@Table( - name = "tool_recommendation_dismissals", - indexes = @Index(name = "idx_tool_rec_dismissal_principal", columnList = "principal")) -@IdClass(ToolRecommendationDismissalId.class) -@Getter -@Setter -@NoArgsConstructor -public class ToolRecommendationDismissal implements Serializable { - - private static final long serialVersionUID = 1L; - - public static final String ANY_CONTEXT = "*"; - - @Id - @Column(name = "principal", length = 255) - private String principal; - - @Id - @Column(name = "context_tool", length = 64) - private String contextTool; - - @Id - @Column(name = "dismissed_tool", length = 64) - private String dismissedTool; - - public ToolRecommendationDismissal(String principal, String contextTool, String dismissedTool) { - this.principal = principal; - this.contextTool = contextTool; - this.dismissedTool = dismissedTool; - } -} diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/model/ToolRecommendationDismissalId.java b/app/proprietary/src/main/java/stirling/software/proprietary/model/ToolRecommendationDismissalId.java deleted file mode 100644 index ff1cfa2cdc..0000000000 --- a/app/proprietary/src/main/java/stirling/software/proprietary/model/ToolRecommendationDismissalId.java +++ /dev/null @@ -1,19 +0,0 @@ -package stirling.software.proprietary.model; - -import java.io.Serializable; - -import lombok.AllArgsConstructor; -import lombok.Data; -import lombok.NoArgsConstructor; - -@Data -@NoArgsConstructor -@AllArgsConstructor -public class ToolRecommendationDismissalId implements Serializable { - - private static final long serialVersionUID = 1L; - - private String principal; - private String contextTool; - private String dismissedTool; -} diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/repository/ToolRecommendationDismissalRepository.java b/app/proprietary/src/main/java/stirling/software/proprietary/repository/ToolRecommendationDismissalRepository.java deleted file mode 100644 index 3af42fbfeb..0000000000 --- a/app/proprietary/src/main/java/stirling/software/proprietary/repository/ToolRecommendationDismissalRepository.java +++ /dev/null @@ -1,27 +0,0 @@ -package stirling.software.proprietary.repository; - -import java.util.List; - -import org.springframework.data.jpa.repository.JpaRepository; -import org.springframework.data.jpa.repository.Modifying; -import org.springframework.data.jpa.repository.Query; -import org.springframework.data.repository.query.Param; -import org.springframework.stereotype.Repository; -import org.springframework.transaction.annotation.Transactional; - -import stirling.software.proprietary.model.ToolRecommendationDismissal; -import stirling.software.proprietary.model.ToolRecommendationDismissalId; - -@Repository -public interface ToolRecommendationDismissalRepository - extends JpaRepository { - - List findByPrincipal(String principal); - - // Erasure: rows key on the raw username, so a recreated name would inherit the opt-outs. - // No clearAutomatically: a clear would detach the User deleteUser deletes right after this. - @Modifying - @Transactional - @Query("DELETE FROM ToolRecommendationDismissal d WHERE d.principal = :principal") - int deleteByPrincipal(@Param("principal") String principal); -} diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/security/service/UserService.java b/app/proprietary/src/main/java/stirling/software/proprietary/security/service/UserService.java index faf94ea96c..24b7d8be93 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/security/service/UserService.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/security/service/UserService.java @@ -46,7 +46,6 @@ import stirling.software.proprietary.integration.model.IntegrationConfig; import stirling.software.proprietary.integration.repository.IntegrationConfigRepository; import stirling.software.proprietary.model.Team; import stirling.software.proprietary.repository.ToolChainStatRepository; -import stirling.software.proprietary.repository.ToolRecommendationDismissalRepository; import stirling.software.proprietary.repository.ToolUsageStatRepository; import stirling.software.proprietary.security.database.repository.AuthorityRepository; import stirling.software.proprietary.security.database.repository.PersistentLoginRepository; @@ -101,7 +100,6 @@ public class UserService implements UserServiceInterface { private final ApiKeyAuthenticationService apiKeyAuthenticationService; private final ToolUsageStatRepository toolUsageStatRepository; private final ToolChainStatRepository toolChainStatRepository; - private final ToolRecommendationDismissalRepository toolRecommendationDismissalRepository; @Transactional public void processSSOPostLogin( @@ -269,10 +267,9 @@ public class UserService implements UserServiceInterface { private void deleteUserRelatedData(User user) { log.info("Deleting all associated data for user: {}", user.getUsername()); - // Tool usage and dismissals key on the username, so a recreated name would inherit them + // Tool usage keys on the username, so a recreated name would inherit it toolUsageStatRepository.deleteByPrincipal(user.getUsername()); toolChainStatRepository.deleteByPrincipal(user.getUsername()); - toolRecommendationDismissalRepository.deleteByPrincipal(user.getUsername()); // Drop ACL grants held by this user and detach grants they issued resourceGrantRepository.deleteByPrincipalTypeAndPrincipalId( diff --git a/app/proprietary/src/main/java/stirling/software/proprietary/service/ToolRecommendationService.java b/app/proprietary/src/main/java/stirling/software/proprietary/service/ToolRecommendationService.java index a61086f4c0..20076dad33 100644 --- a/app/proprietary/src/main/java/stirling/software/proprietary/service/ToolRecommendationService.java +++ b/app/proprietary/src/main/java/stirling/software/proprietary/service/ToolRecommendationService.java @@ -7,27 +7,20 @@ import java.util.List; import java.util.Map; import java.util.Set; -import org.springframework.dao.DataIntegrityViolationException; import org.springframework.stereotype.Service; import org.springframework.transaction.annotation.Transactional; import lombok.RequiredArgsConstructor; -import lombok.extern.slf4j.Slf4j; import stirling.software.common.model.ApplicationProperties; -import stirling.software.proprietary.model.ToolRecommendationDismissal; -import stirling.software.proprietary.model.ToolRecommendationDismissalId; -import stirling.software.proprietary.repository.ToolRecommendationDismissalRepository; import stirling.software.proprietary.service.ToolUsageSignalService.TeamScope; import stirling.software.proprietary.service.ToolUsageSignalService.ToolChainSummary; /** * Scores "what tool next". Transitions out of the current tool dominate, then the caller's own - * usage, their team's, and the whole install's. Scoring is deliberately uncached - the costly - * aggregates are cached inside {@link ToolUsageSignalService} and shared by everyone, so a - * dismissal takes effect immediately without invalidating anyone else's data. + * usage, their team's, and the whole install's. Scoring itself is cheap and uncached; the costly + * aggregates are cached inside {@link ToolUsageSignalService} and shared by everyone. */ -@Slf4j @Service @RequiredArgsConstructor public class ToolRecommendationService { @@ -47,7 +40,6 @@ public class ToolRecommendationService { static final int MIN_WORKFLOW_TOOLS = 2; private final ToolUsageSignalService signals; - private final ToolRecommendationDismissalRepository dismissalRepository; private final ApplicationProperties applicationProperties; public record ToolRecommendation(String toolKey, double score) {} @@ -162,12 +154,9 @@ public class ToolRecommendationService { } merge(scores, signals.globalFrequency(cutoff, recent), WEIGHT_FREQUENCY_GLOBAL); - Set excluded = dismissedTools(principal, currentTool); - if (currentTool != null) { - excluded.add(currentTool); - } + // Never answer "what next" with the tool the user is already in. return scores.entrySet().stream() - .filter(e -> !excluded.contains(e.getKey())) + .filter(e -> !e.getKey().equals(currentTool)) .sorted( Map.Entry.comparingByValue() .reversed() @@ -189,41 +178,6 @@ public class ToolRecommendationService { signal.forEach((tool, value) -> scores.merge(tool, weight * (value / max), Double::sum)); } - private Set dismissedTools(String principal, String currentTool) { - Set excluded = new HashSet<>(); - for (ToolRecommendationDismissal dismissal : - dismissalRepository.findByPrincipal(principal)) { - String context = dismissal.getContextTool(); - if (ToolRecommendationDismissal.ANY_CONTEXT.equals(context) - || context.equals(currentTool)) { - excluded.add(dismissal.getDismissedTool()); - } - } - return excluded; - } - - /** - * Idempotent: the row is its own primary key. Two simultaneous dismissals (double click, or two - * nodes) can still race to insert it, and losing that race already means the desired row - * exists. - */ - @Transactional - public void dismiss(String principal, String contextTool, String dismissedTool) { - try { - dismissalRepository.save( - new ToolRecommendationDismissal(principal, contextTool, dismissedTool)); - } catch (DataIntegrityViolationException e) { - log.debug("Dismissal {}/{} already stored", contextTool, dismissedTool); - } - } - - @Transactional - public void undoDismiss(String principal, String contextTool, String dismissedTool) { - dismissalRepository - .findById(new ToolRecommendationDismissalId(principal, contextTool, dismissedTool)) - .ifPresent(dismissalRepository::delete); - } - private static double round(double value) { return Math.round(value * 1000.0) / 1000.0; } diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/controller/api/ToolRecommendationControllerTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/controller/api/ToolRecommendationControllerTest.java index 804da706f4..d2c86c84b0 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/controller/api/ToolRecommendationControllerTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/controller/api/ToolRecommendationControllerTest.java @@ -22,7 +22,6 @@ import org.mockito.junit.jupiter.MockitoExtension; import org.springframework.http.ResponseEntity; import stirling.software.common.service.UserServiceInterface; -import stirling.software.proprietary.controller.api.ToolRecommendationController.DismissalRequest; import stirling.software.proprietary.controller.api.ToolRecommendationController.RecommendationsResponse; import stirling.software.proprietary.controller.api.ToolRecommendationController.UsageRequest; import stirling.software.proprietary.controller.api.ToolRecommendationController.WorkflowsResponse; @@ -205,63 +204,4 @@ class ToolRecommendationControllerTest { verify(trackingService).recordUsage("anon:" + BROWSER_ID, "ocr", null); } } - - @Nested - @DisplayName("Dismissals") - class Dismissals { - - @Test - @DisplayName("a context-scoped dismissal is stored") - void dismissStored() { - when(userService.getCurrentUsername()).thenReturn("alice"); - - ResponseEntity response = - controller.dismiss(new DismissalRequest("compare", "ocr"), null); - - assertThat(response.getStatusCode().value()).isEqualTo(204); - verify(recommendationService).dismiss("alice", "compare", "ocr"); - } - - @Test - @DisplayName("the any-context wildcard is accepted") - void wildcardContextAccepted() { - when(userService.getCurrentUsername()).thenReturn("alice"); - - ResponseEntity response = - controller.dismiss(new DismissalRequest("*", "ocr"), null); - - assertThat(response.getStatusCode().value()).isEqualTo(204); - verify(recommendationService).dismiss("alice", "*", "ocr"); - } - - @Test - @DisplayName("junk context or tool is rejected with 400") - void junkRejected() { - assertThat( - controller - .dismiss(new DismissalRequest("bad context!", "ocr"), null) - .getStatusCode() - .value()) - .isEqualTo(400); - assertThat( - controller - .dismiss(new DismissalRequest("compare", "bad tool!"), null) - .getStatusCode() - .value()) - .isEqualTo(400); - assertThat(controller.dismiss(null, null).getStatusCode().value()).isEqualTo(400); - verifyNoInteractions(recommendationService); - } - - @Test - @DisplayName("undo removes the stored dismissal") - void undoRemoves() { - when(userService.getCurrentUsername()).thenReturn("alice"); - - ResponseEntity response = controller.undoDismiss("compare", "ocr", null); - - assertThat(response.getStatusCode().value()).isEqualTo(204); - verify(recommendationService).undoDismiss("alice", "compare", "ocr"); - } - } } diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/repository/ToolRecommendationRepositoriesTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/repository/ToolRecommendationRepositoriesTest.java index 8e0ac86431..9b9b9d5f02 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/repository/ToolRecommendationRepositoriesTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/repository/ToolRecommendationRepositoriesTest.java @@ -18,8 +18,6 @@ import org.springframework.data.domain.PageRequest; import org.springframework.data.jpa.repository.config.EnableJpaRepositories; import stirling.software.proprietary.model.ToolChainStat; -import stirling.software.proprietary.model.ToolRecommendationDismissal; -import stirling.software.proprietary.model.ToolRecommendationDismissalId; import stirling.software.proprietary.model.ToolUsageStat; /** Exercises the windowed CASE aggregation and increment queries against H2. */ @@ -31,7 +29,6 @@ class ToolRecommendationRepositoriesTest { @Autowired private ToolUsageStatRepository usageRepository; @Autowired private ToolChainStatRepository chainRepository; - @Autowired private ToolRecommendationDismissalRepository dismissalRepository; private static Map byTool(List rows) { return rows.stream() @@ -275,35 +272,6 @@ class ToolRecommendationRepositoriesTest { assertThat(chainRepository.findAll().get(0).getChainKey()).hasSize(key.length()); } - @Test - @DisplayName("saving the same dismissal twice leaves one row") - void dismissalsAreIdempotent() { - dismissalRepository.saveAndFlush( - new ToolRecommendationDismissal("alice", "compare", "ocr")); - dismissalRepository.saveAndFlush( - new ToolRecommendationDismissal("alice", "compare", "ocr")); - - assertThat(dismissalRepository.findByPrincipal("alice")).hasSize(1); - } - - @Test - @DisplayName("dismissals are addressable by their full composite key") - void dismissalsAddressableByKey() { - dismissalRepository.saveAndFlush( - new ToolRecommendationDismissal("alice", "compare", "ocr")); - dismissalRepository.saveAndFlush(new ToolRecommendationDismissal("alice", "merge", "ocr")); - - assertThat( - dismissalRepository.findById( - new ToolRecommendationDismissalId("alice", "compare", "ocr"))) - .isPresent(); - assertThat( - dismissalRepository.findById( - new ToolRecommendationDismissalId("alice", "split", "ocr"))) - .isEmpty(); - assertThat(dismissalRepository.findByPrincipal("alice")).hasSize(2); - } - @Test @DisplayName("erasure removes a principal's usage rows and leaves everyone else's") void deleteByPrincipalErasesUsage() { @@ -319,22 +287,6 @@ class ToolRecommendationRepositoriesTest { assertThat(usageRepository.sumByPrincipal("alice", DAY - 30, DAY - 7)).isEmpty(); } - @Test - @DisplayName("erasure removes a principal's dismissals and leaves everyone else's") - void deleteByPrincipalErasesDismissals() { - dismissalRepository.saveAndFlush( - new ToolRecommendationDismissal("alice", "compare", "ocr")); - dismissalRepository.saveAndFlush( - new ToolRecommendationDismissal( - "alice", ToolRecommendationDismissal.ANY_CONTEXT, "merge")); - dismissalRepository.saveAndFlush(new ToolRecommendationDismissal("bob", "compare", "ocr")); - - assertThat(dismissalRepository.deleteByPrincipal("alice")).isEqualTo(2); - - assertThat(dismissalRepository.findByPrincipal("alice")).isEmpty(); - assertThat(dismissalRepository.findByPrincipal("bob")).hasSize(1); - } - @SpringBootConfiguration @EntityScan( basePackages = { diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/repository/ToolUsageErasureDeletesUserTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/repository/ToolUsageErasureDeletesUserTest.java index bef7258caa..0bd6a82a4e 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/repository/ToolUsageErasureDeletesUserTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/repository/ToolUsageErasureDeletesUserTest.java @@ -15,7 +15,6 @@ import org.springframework.data.jpa.repository.config.EnableJpaRepositories; import jakarta.persistence.EntityManager; import stirling.software.proprietary.model.ToolChainStat; -import stirling.software.proprietary.model.ToolRecommendationDismissal; import stirling.software.proprietary.model.ToolUsageStat; import stirling.software.proprietary.security.database.repository.UserRepository; import stirling.software.proprietary.security.model.Authority; @@ -35,7 +34,6 @@ class ToolUsageErasureDeletesUserTest { @Autowired private UserRepository userRepository; @Autowired private ToolUsageStatRepository usageRepository; @Autowired private ToolChainStatRepository chainRepository; - @Autowired private ToolRecommendationDismissalRepository dismissalRepository; @Autowired private EntityManager entityManager; private Long seedUser(String username) { @@ -49,7 +47,6 @@ class ToolUsageErasureDeletesUserTest { usageRepository.save(new ToolUsageStat(username, NONE, "ocr", DAY, 1)); usageRepository.save(new ToolUsageStat(username, "compare", "merge", DAY - 5, 2)); chainRepository.save(new ToolChainStat(username, "compare>merge", DAY, 2, 2)); - dismissalRepository.save(new ToolRecommendationDismissal(username, "compare", "ocr")); entityManager.flush(); entityManager.clear(); return user.getId(); @@ -75,7 +72,6 @@ class ToolUsageErasureDeletesUserTest { usageRepository.deleteByPrincipal("tracked"); chainRepository.deleteByPrincipal("tracked"); - dismissalRepository.deleteByPrincipal("tracked"); // The erasures must leave the user managed, or delete() merges (and cascades) instead assertThat(entityManager.contains(user)).isTrue(); @@ -100,11 +96,9 @@ class ToolUsageErasureDeletesUserTest { assertThat(chainRepository.findAll()) .extracting(ToolChainStat::getPrincipal) .containsOnly("bystander"); - assertThat(dismissalRepository.findByPrincipal("tracked")).isEmpty(); // The bystander is untouched by any of it assertThat(userRepository.findByUsernameIgnoreCase("bystander")).isPresent(); - assertThat(dismissalRepository.findByPrincipal("bystander")).hasSize(1); assertThat(countSettings(keptId)).isEqualTo(1); } @@ -118,7 +112,6 @@ class ToolUsageErasureDeletesUserTest { usageRepository.deleteByPrincipal("tracked"); chainRepository.deleteByPrincipal("tracked"); - dismissalRepository.deleteByPrincipal("tracked"); assertThat(entityManager.contains(user)).isTrue(); assertThat(before).isNotEmpty(); diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/security/service/UserServiceTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/security/service/UserServiceTest.java index 2321b7e756..8eeac6459a 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/security/service/UserServiceTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/security/service/UserServiceTest.java @@ -27,7 +27,6 @@ import stirling.software.common.model.exception.UnsupportedProviderException; import stirling.software.proprietary.access.repository.ResourceGrantRepository; import stirling.software.proprietary.model.Team; import stirling.software.proprietary.repository.ToolChainStatRepository; -import stirling.software.proprietary.repository.ToolRecommendationDismissalRepository; import stirling.software.proprietary.repository.ToolUsageStatRepository; import stirling.software.proprietary.security.database.repository.AuthorityRepository; import stirling.software.proprietary.security.database.repository.PersistentLoginRepository; @@ -77,7 +76,6 @@ class UserServiceTest { @Mock private ApiKeyAuthenticationService apiKeyAuthenticationService; @Mock private ToolUsageStatRepository toolUsageStatRepository; @Mock private ToolChainStatRepository toolChainStatRepository; - @Mock private ToolRecommendationDismissalRepository toolRecommendationDismissalRepository; @Spy @InjectMocks private UserService userService; @@ -292,7 +290,7 @@ class UserServiceTest { } @Test - void deleteUser_erasesToolUsageAndDismissals() { + void deleteUser_erasesToolUsage() { User user = new User(); user.setId(4L); user.setUsername("tracked"); @@ -307,7 +305,6 @@ class UserServiceTest { // Every table keys on the username, so a recreated name would inherit the old profile verify(toolUsageStatRepository).deleteByPrincipal("tracked"); verify(toolChainStatRepository).deleteByPrincipal("tracked"); - verify(toolRecommendationDismissalRepository).deleteByPrincipal("tracked"); // The erasures must not displace the user row itself verify(userRepository).delete(user); } diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/service/ToolRecommendationServiceTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/service/ToolRecommendationServiceTest.java index 6c2eb2c1fb..8cbaecbba1 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/service/ToolRecommendationServiceTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/service/ToolRecommendationServiceTest.java @@ -14,21 +14,16 @@ import static org.mockito.Mockito.when; import java.util.List; import java.util.Map; -import java.util.Optional; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import stirling.software.common.model.ApplicationProperties; -import stirling.software.proprietary.model.ToolRecommendationDismissal; -import stirling.software.proprietary.model.ToolRecommendationDismissalId; -import stirling.software.proprietary.repository.ToolRecommendationDismissalRepository; import stirling.software.proprietary.service.ToolRecommendationService.ToolRecommendation; import stirling.software.proprietary.service.ToolRecommendationService.ToolWorkflow; import stirling.software.proprietary.service.ToolRecommendationService.WorkflowScope; @@ -42,7 +37,6 @@ class ToolRecommendationServiceTest { private static final TeamScope TEAM = new TeamScope(7L, List.of("bob", "carol")); @Mock private ToolUsageSignalService signalService; - @Mock private ToolRecommendationDismissalRepository dismissalRepository; private ApplicationProperties properties; private ToolRecommendationService service; @@ -51,8 +45,7 @@ class ToolRecommendationServiceTest { void setUp() { properties = new ApplicationProperties(); properties.getSystem().setEnableAnalytics(true); - service = new ToolRecommendationService(signalService, dismissalRepository, properties); - lenient().when(dismissalRepository.findByPrincipal(anyString())).thenReturn(List.of()); + service = new ToolRecommendationService(signalService, properties); lenient().when(signalService.resolveTeamScope(anyString())).thenReturn(TeamScope.none()); lenient() .when(signalService.userFrequency(anyString(), anyLong(), anyLong())) @@ -304,7 +297,7 @@ class ToolRecommendationServiceTest { properties.getToolRecommendations().setEnabled(false); assertThat(service.getRecommendations(PRINCIPAL, "compare", 6)).isEmpty(); - verifyNoInteractions(signalService, dismissalRepository); + verifyNoInteractions(signalService); } @Test @@ -313,7 +306,7 @@ class ToolRecommendationServiceTest { properties.getSystem().setEnableAnalytics(null); assertThat(service.getRecommendations(PRINCIPAL, "compare", 6)).isEmpty(); - verifyNoInteractions(signalService, dismissalRepository); + verifyNoInteractions(signalService); } @Test @@ -326,109 +319,4 @@ class ToolRecommendationServiceTest { verify(signalService, never()).globalTransitions(anyString(), anyLong(), anyLong()); } } - - @Nested - @DisplayName("Dismissals") - class Dismissals { - - @Test - @DisplayName("a dismissal for the current context hides the tool") - void contextDismissalFilters() { - when(signalService.userFrequency(eq(PRINCIPAL), anyLong(), anyLong())) - .thenReturn(Map.of("ocr", 6.0, "merge", 5.0)); - when(dismissalRepository.findByPrincipal(PRINCIPAL)) - .thenReturn( - List.of(new ToolRecommendationDismissal(PRINCIPAL, "compare", "ocr"))); - - List result = service.getRecommendations(PRINCIPAL, "compare", 6); - - assertThat(toolKeys(result)).containsExactly("merge"); - } - - @Test - @DisplayName("a dismissal for another context does not hide the tool") - void unrelatedContextDismissalKept() { - when(signalService.userFrequency(eq(PRINCIPAL), anyLong(), anyLong())) - .thenReturn(Map.of("ocr", 6.0)); - when(dismissalRepository.findByPrincipal(PRINCIPAL)) - .thenReturn( - List.of(new ToolRecommendationDismissal(PRINCIPAL, "merge", "ocr"))); - - List result = service.getRecommendations(PRINCIPAL, "compare", 6); - - assertThat(toolKeys(result)).containsExactly("ocr"); - } - - @Test - @DisplayName("an any-context dismissal hides the tool everywhere") - void anyContextDismissalFilters() { - when(signalService.userFrequency(eq(PRINCIPAL), anyLong(), anyLong())) - .thenReturn(Map.of("ocr", 6.0)); - when(dismissalRepository.findByPrincipal(PRINCIPAL)) - .thenReturn( - List.of( - new ToolRecommendationDismissal( - PRINCIPAL, - ToolRecommendationDismissal.ANY_CONTEXT, - "ocr"))); - - assertThat(service.getRecommendations(PRINCIPAL, null, 6)).isEmpty(); - assertThat(service.getRecommendations(PRINCIPAL, "compare", 6)).isEmpty(); - } - - @Test - @DisplayName("a dismissal takes effect on the very next read (nothing cached)") - void dismissalAppliesImmediately() { - when(signalService.userFrequency(eq(PRINCIPAL), anyLong(), anyLong())) - .thenReturn(Map.of("ocr", 6.0, "merge", 5.0)); - when(dismissalRepository.findByPrincipal(PRINCIPAL)) - .thenReturn(List.of()) - .thenReturn( - List.of(new ToolRecommendationDismissal(PRINCIPAL, "compare", "ocr"))); - - assertThat(toolKeys(service.getRecommendations(PRINCIPAL, "compare", 6))) - .containsExactly("ocr", "merge"); - assertThat(toolKeys(service.getRecommendations(PRINCIPAL, "compare", 6))) - .containsExactly("merge"); - } - - @Test - @DisplayName("dismiss saves the row keyed by principal, context and tool") - void dismissSavesRow() { - service.dismiss(PRINCIPAL, "compare", "ocr"); - - ArgumentCaptor captor = - ArgumentCaptor.forClass(ToolRecommendationDismissal.class); - verify(dismissalRepository).save(captor.capture()); - assertThat(captor.getValue().getPrincipal()).isEqualTo(PRINCIPAL); - assertThat(captor.getValue().getContextTool()).isEqualTo("compare"); - assertThat(captor.getValue().getDismissedTool()).isEqualTo("ocr"); - } - - @Test - @DisplayName("undoDismiss deletes the stored dismissal") - void undoDismissDeletes() { - ToolRecommendationDismissal stored = - new ToolRecommendationDismissal(PRINCIPAL, "compare", "ocr"); - when(dismissalRepository.findById( - new ToolRecommendationDismissalId(PRINCIPAL, "compare", "ocr"))) - .thenReturn(Optional.of(stored)); - - service.undoDismiss(PRINCIPAL, "compare", "ocr"); - - verify(dismissalRepository).delete(stored); - } - - @Test - @DisplayName("undoing a dismissal that was never made is a no-op") - void undoUnknownDismissalIsNoOp() { - when(dismissalRepository.findById( - new ToolRecommendationDismissalId(PRINCIPAL, "compare", "ocr"))) - .thenReturn(Optional.empty()); - - service.undoDismiss(PRINCIPAL, "compare", "ocr"); - - verify(dismissalRepository, never()).delete(any(ToolRecommendationDismissal.class)); - } - } } diff --git a/app/proprietary/src/test/java/stirling/software/proprietary/service/ToolUsagePostgresConcurrencyTest.java b/app/proprietary/src/test/java/stirling/software/proprietary/service/ToolUsagePostgresConcurrencyTest.java index 71dfc9a2d6..0cb7f371d8 100644 --- a/app/proprietary/src/test/java/stirling/software/proprietary/service/ToolUsagePostgresConcurrencyTest.java +++ b/app/proprietary/src/test/java/stirling/software/proprietary/service/ToolUsagePostgresConcurrencyTest.java @@ -28,10 +28,8 @@ import org.testcontainers.junit.jupiter.Testcontainers; import stirling.software.common.model.ApplicationProperties; import stirling.software.proprietary.model.ToolChainStat; -import stirling.software.proprietary.model.ToolRecommendationDismissal; import stirling.software.proprietary.model.ToolUsageStat; import stirling.software.proprietary.repository.ToolChainStatRepository; -import stirling.software.proprietary.repository.ToolRecommendationDismissalRepository; import stirling.software.proprietary.repository.ToolUsageStatRepository; /** @@ -69,10 +67,8 @@ class ToolUsagePostgresConcurrencyTest { @Autowired private ToolUsageStatRepository usageRepository; @Autowired private ToolChainStatRepository chainRepository; - @Autowired private ToolRecommendationDismissalRepository dismissalRepository; private ToolUsageTrackingService trackingService; - private ToolRecommendationService recommendationService; @BeforeEach void setUp() { @@ -80,12 +76,6 @@ class ToolUsagePostgresConcurrencyTest { properties.getSystem().setEnableAnalytics(true); trackingService = new ToolUsageTrackingService(usageRepository, chainRepository, properties); - recommendationService = - new ToolRecommendationService( - new ToolUsageSignalService( - usageRepository, chainRepository, java.util.Optional.empty()), - dismissalRepository, - properties); } /** Runs {@code task} on {@code NODES} threads at once; returns how many threw. */ @@ -189,20 +179,6 @@ class ToolUsagePostgresConcurrencyTest { assertThat(rows.get(0).getChainLength()).isEqualTo(2); } - @Test - @Transactional(propagation = Propagation.NOT_SUPPORTED) - @DisplayName("the same dismissal from several nodes at once stores one row and never throws") - void concurrentDismissalsAreIdempotent() throws InterruptedException { - dismissalRepository.deleteAll(); - - int failures = race(NODES, () -> recommendationService.dismiss("alice", "compare", "ocr")); - - assertThat(failures).isZero(); - assertThat(dismissalRepository.findByPrincipal("alice")) - .extracting(ToolRecommendationDismissal::getDismissedTool) - .containsExactly("ocr"); - } - @Test @Transactional(propagation = Propagation.NOT_SUPPORTED) @DisplayName("the retention sweep is idempotent when every node runs it together") diff --git a/frontend/editor/public/locales/en-GB/translation.toml b/frontend/editor/public/locales/en-GB/translation.toml index b89fe0d46f..a91d52cca0 100644 --- a/frontend/editor/public/locales/en-GB/translation.toml +++ b/frontend/editor/public/locales/en-GB/translation.toml @@ -10828,12 +10828,6 @@ noToolsFound = "No tools found" quickAccess = "QUICK ACCESS" searchPlaceholder = "Search tools..." -[toolPicker.recommendations] -dismiss = "Don't recommend this tool here" -dismissed = "{{tool}} won't be recommended here again" -dismissFailed = "Could not save that preference. Please try again." -undo = "Undo" - [toolPicker.subcategories] advancedFormatting = "Advanced Formatting" automation = "Automation" diff --git a/frontend/editor/public/locales/en-US/translation.toml b/frontend/editor/public/locales/en-US/translation.toml index 3768118203..69211b6360 100644 --- a/frontend/editor/public/locales/en-US/translation.toml +++ b/frontend/editor/public/locales/en-US/translation.toml @@ -10953,12 +10953,6 @@ noToolsFound = "No tools found" quickAccess = "QUICK ACCESS" searchPlaceholder = "Search tools..." -[toolPicker.recommendations] -dismiss = "Don't recommend this tool here" -dismissed = "{{tool}} won't be recommended here again" -dismissFailed = "Could not save that preference. Please try again." -undo = "Undo" - [toolPicker.subcategories] advancedFormatting = "Advanced Formatting" automation = "Automation" diff --git a/frontend/editor/src/core/api/toolRecommendations.test.ts b/frontend/editor/src/core/api/toolRecommendations.test.ts index 02c8bf20e1..df3f97c921 100644 --- a/frontend/editor/src/core/api/toolRecommendations.test.ts +++ b/frontend/editor/src/core/api/toolRecommendations.test.ts @@ -5,18 +5,15 @@ import { fetchToolRecommendations, fetchToolWorkflows, recordToolUsage, - dismissToolRecommendation, - undoDismissToolRecommendation, resetToolRecommendationsAvailabilityForTests, } from "@app/api/toolRecommendations"; vi.mock("@app/services/apiClient", () => ({ - default: { get: vi.fn(), post: vi.fn(), delete: vi.fn() }, + default: { get: vi.fn(), post: vi.fn() }, })); const mockGet = vi.mocked(apiClient.get); const mockPost = vi.mocked(apiClient.post); -const mockDelete = vi.mocked(apiClient.delete); const http404 = Object.assign(new Error("not found"), { response: { status: 404 }, @@ -140,47 +137,4 @@ describe("toolRecommendations api", () => { expect(mockPost).toHaveBeenCalledTimes(1); }); }); - - describe("dismissals", () => { - it("posts a context-scoped dismissal", async () => { - mockPost.mockResolvedValue({}); - - await dismissToolRecommendation("compare", "ocr"); - - expect(mockPost).toHaveBeenCalledWith( - expect.stringContaining("/dismissals"), - { contextTool: "compare", dismissedTool: "ocr" }, - expect.objectContaining({ suppressErrorToast: true }), - ); - }); - - it("maps a null context to the any-context wildcard", async () => { - mockPost.mockResolvedValue({}); - - await dismissToolRecommendation(null, "ocr"); - - expect(mockPost.mock.calls[0][1]).toEqual({ - contextTool: "*", - dismissedTool: "ocr", - }); - }); - - it("undo issues a delete with the same coordinates", async () => { - mockDelete.mockResolvedValue({}); - - await undoDismissToolRecommendation("compare", "ocr"); - - const url = mockDelete.mock.calls[0][0] as string; - expect(url).toContain("contextTool=compare"); - expect(url).toContain("dismissedTool=ocr"); - }); - - it("propagates dismissal failures so the UI can warn the user", async () => { - mockPost.mockRejectedValue(new Error("boom")); - - await expect(dismissToolRecommendation("compare", "ocr")).rejects.toThrow( - "boom", - ); - }); - }); }); diff --git a/frontend/editor/src/core/api/toolRecommendations.ts b/frontend/editor/src/core/api/toolRecommendations.ts index d7ce324b88..f8587a0649 100644 --- a/frontend/editor/src/core/api/toolRecommendations.ts +++ b/frontend/editor/src/core/api/toolRecommendations.ts @@ -5,9 +5,6 @@ export interface ToolRecommendationDto { score: number; } -/** Dismiss in every context (used when no tool is active). */ -export const ANY_CONTEXT = "*"; - const BASE_PATH = "/api/v1/proprietary/ui-data/tool-recommendations"; // Core-only backends have no recommendations API; remember the 404 so we stop asking. @@ -106,28 +103,3 @@ export async function fetchToolWorkflows( return null; } } - -export async function dismissToolRecommendation( - contextTool: string | null, - dismissedTool: string, -): Promise { - await apiClient.post( - `${BASE_PATH}/dismissals`, - { contextTool: contextTool ?? ANY_CONTEXT, dismissedTool }, - { suppressErrorToast: true, skipAuthRedirect: true }, - ); -} - -export async function undoDismissToolRecommendation( - contextTool: string | null, - dismissedTool: string, -): Promise { - const params = new URLSearchParams({ - contextTool: contextTool ?? ANY_CONTEXT, - dismissedTool, - }); - await apiClient.delete(`${BASE_PATH}/dismissals?${params}`, { - suppressErrorToast: true, - skipAuthRedirect: true, - }); -} diff --git a/frontend/editor/src/core/components/tools/ToolPicker.tsx b/frontend/editor/src/core/components/tools/ToolPicker.tsx index 448666aaa0..5c5ce0e261 100644 --- a/frontend/editor/src/core/components/tools/ToolPicker.tsx +++ b/frontend/editor/src/core/components/tools/ToolPicker.tsx @@ -1,16 +1,11 @@ -import React, { useCallback, useMemo, useRef } from "react"; +import React, { useMemo, useRef } from "react"; import { Box, Stack } from "@mantine/core"; import { useTranslation } from "react-i18next"; -import { alert } from "@app/components/toast"; import { Button } from "@app/ui/Button"; import { ToolRegistryEntry } from "@app/data/toolsTaxonomy"; import "@app/components/tools/toolPicker/ToolPicker.css"; import { useToolSections } from "@app/hooks/useToolSections"; import type { SubcategoryGroup } from "@app/hooks/useToolSections"; -import { - useDismissToolRecommendation, - useRecommendationContextTool, -} from "@app/hooks/useToolRecommendations"; import { useFavoriteToolItems } from "@app/hooks/tools/useFavoriteToolItems"; import NoToolsFound from "@app/components/tools/shared/NoToolsFound"; import { renderToolButtons } from "@app/components/tools/shared/renderToolButtons"; @@ -75,57 +70,8 @@ const ToolPicker = ({ const scrollableRef = useRef(null); - const { sections: visibleSections, rankedRecommendationIds } = - useToolSections(filteredTools); + const { sections: visibleSections } = useToolSections(filteredTools); const { favoriteTools, toolRegistry } = useToolWorkflowData(); - const recommendationContext = useRecommendationContextTool(); - const dismissRecommendation = useDismissToolRecommendation(); - - // Dismiss only applies to usage-derived recommendations; the static list is not persisted. - const handleDismissRecommendation = useCallback( - (toolId: ToolId, toolName: string) => { - const reportFailure = () => - alert({ - alertType: "error", - title: t( - "toolPicker.recommendations.dismissFailed", - "Could not save that preference. Please try again.", - ), - }); - - void (async () => { - try { - const undo = await dismissRecommendation( - recommendationContext, - toolId, - ); - alert({ - alertType: "neutral", - title: t("toolPicker.recommendations.dismissed", { - defaultValue: "{{tool}} won't be recommended here again", - tool: toolName, - }), - buttonText: t("toolPicker.recommendations.undo", "Undo"), - buttonCallback: () => void undo().catch(reportFailure), - durationMs: 6000, - }); - } catch { - reportFailure(); - } - })(); - }, - [dismissRecommendation, recommendationContext, t], - ); - - // Only usage-ranked entries can be dismissed; on the curated top-up (and on - // Shared Signing, pinned by its badge) a dismissal could never take effect. - const dismissHandlerFor = useCallback( - (id: string, tool: ToolRegistryEntry) => - rankedRecommendationIds.has(id as ToolId) && id !== "sharedSign" - ? () => handleDismissRecommendation(id as ToolId, tool.name) - : undefined, - [rankedRecommendationIds, handleDismissRecommendation], - ); const favoriteToolItems = useFavoriteToolItems(favoriteTools, toolRegistry); @@ -231,7 +177,6 @@ const ToolPicker = ({ badgeCount={ id === "sharedSign" ? signingBadgeCount : undefined } - onDismiss={dismissHandlerFor(id, tool)} /> ))} @@ -289,7 +234,6 @@ const ToolPicker = ({ badgeCount={ id === "sharedSign" ? signingBadgeCount : undefined } - onDismiss={dismissHandlerFor(id, tool)} /> ))} diff --git a/frontend/editor/src/core/components/tools/shared/SuggestedToolsSection.tsx b/frontend/editor/src/core/components/tools/shared/SuggestedToolsSection.tsx index 79be895590..96d33d0dfd 100644 --- a/frontend/editor/src/core/components/tools/shared/SuggestedToolsSection.tsx +++ b/frontend/editor/src/core/components/tools/shared/SuggestedToolsSection.tsx @@ -17,26 +17,24 @@ export function SuggestedToolsSection(): React.ReactElement { - {suggestedTools.map((tool) => { - const IconComponent = tool.icon; - return ( - - - - } /> - - {tool.title} - - - - - ); - })} + {suggestedTools.map((tool) => ( + + + + + + {tool.title} + + + + + ))} ); diff --git a/frontend/editor/src/core/components/tools/toolPicker/ToolButton.test.tsx b/frontend/editor/src/core/components/tools/toolPicker/ToolButton.test.tsx deleted file mode 100644 index 1f60b70e53..0000000000 --- a/frontend/editor/src/core/components/tools/toolPicker/ToolButton.test.tsx +++ /dev/null @@ -1,125 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; -import { render, screen, fireEvent } from "@testing-library/react"; -import { MantineProvider } from "@mantine/core"; - -import ToolButton from "@app/components/tools/toolPicker/ToolButton"; -import { - SubcategoryId, - ToolCategoryId, - ToolRegistryEntry, -} from "@app/data/toolsTaxonomy"; - -vi.mock("@app/contexts/ToolWorkflowContext", () => ({ - useToolWorkflowData: () => ({ - isFavorite: () => false, - toolAvailability: {}, - }), - useToolWorkflowActions: () => ({ toggleFavorite: vi.fn() }), -})); -vi.mock("@app/contexts/HotkeyContext", () => ({ - useHotkeys: () => ({ hotkeys: {} }), -})); -vi.mock("@app/hooks/useToolNavigation", () => ({ - useToolNavigation: () => ({ getToolNavigation: () => null }), -})); -vi.mock("@app/contexts/AppConfigContext", () => ({ - useAppConfig: () => ({ config: {} }), -})); -vi.mock("@app/hooks/useWillUseCloud", () => ({ - useWillUseCloud: () => false, -})); -// Tooltip pulls in preferences/logo providers irrelevant to this test. -vi.mock("@app/components/shared/Tooltip", () => ({ - Tooltip: ({ children }: { children: React.ReactNode }) => <>{children}, -})); - -const tool: ToolRegistryEntry = { - icon: null, - name: "OCR", - component: (() => null) as never, - description: "Recognise text", - categoryId: ToolCategoryId.RECOMMENDED_TOOLS, - subcategoryId: SubcategoryId.EXTRACTION, - automationSettings: null, -} as ToolRegistryEntry; - -const renderButton = (props: Partial[0]> = {}) => - render( - - - , - ); - -describe("ToolButton dismiss control", () => { - beforeEach(() => { - vi.clearAllMocks(); - }); - - it("renders no dismiss control by default", () => { - renderButton(); - - expect( - screen.queryByLabelText("toolPicker.recommendations.dismiss"), - ).not.toBeInTheDocument(); - }); - - it("renders the dismiss X when onDismiss is provided", () => { - renderButton({ onDismiss: vi.fn() }); - - expect( - screen.getByLabelText("toolPicker.recommendations.dismiss"), - ).toBeInTheDocument(); - }); - - it("clicking the X fires onDismiss without selecting the tool", () => { - const onDismiss = vi.fn(); - const onSelect = vi.fn(); - renderButton({ onDismiss, onSelect }); - - fireEvent.click( - screen.getByLabelText("toolPicker.recommendations.dismiss"), - ); - - expect(onDismiss).toHaveBeenCalledTimes(1); - expect(onSelect).not.toHaveBeenCalled(); - }); - - it("clicking the tool itself still selects it", () => { - const onSelect = vi.fn(); - renderButton({ onDismiss: vi.fn(), onSelect }); - - fireEvent.click(screen.getByText("OCR")); - - expect(onSelect).toHaveBeenCalledWith("ocr"); - }); - - it("exposes the X as a keyboard-reachable button to assistive tech", () => { - renderButton({ onDismiss: vi.fn() }); - - // aria-label is ARIA-prohibited on a bare span, so role is what makes it nameable. - const dismissButton = screen.getByRole("button", { - name: "toolPicker.recommendations.dismiss", - }); - expect(dismissButton).toHaveAttribute("tabindex", "0"); - }); - - it.each(["Enter", " "])("pressing %s on the X fires onDismiss", (key) => { - const onDismiss = vi.fn(); - const onSelect = vi.fn(); - renderButton({ onDismiss, onSelect }); - - fireEvent.keyDown( - screen.getByLabelText("toolPicker.recommendations.dismiss"), - { key }, - ); - - expect(onDismiss).toHaveBeenCalledTimes(1); - expect(onSelect).not.toHaveBeenCalled(); - }); -}); diff --git a/frontend/editor/src/core/components/tools/toolPicker/ToolButton.tsx b/frontend/editor/src/core/components/tools/toolPicker/ToolButton.tsx index 67856a3923..4792fd721a 100644 --- a/frontend/editor/src/core/components/tools/toolPicker/ToolButton.tsx +++ b/frontend/editor/src/core/components/tools/toolPicker/ToolButton.tsx @@ -1,7 +1,5 @@ import React, { memo } from "react"; import { Badge } from "@mantine/core"; -import CloseRoundedIcon from "@mui/icons-material/CloseRounded"; -import { ActionIcon } from "@app/ui/ActionIcon"; import { Button } from "@app/ui/Button"; import { useTranslation } from "react-i18next"; import { Tooltip } from "@app/components/shared/Tooltip"; @@ -39,8 +37,6 @@ interface ToolButtonProps { /** Called when an unavailable tool is clicked; if provided, overrides the default no-op */ onUnavailableClick?: () => void; badgeCount?: number; - /** Shows a hover-only X that dismisses this tool from the recommended list. */ - onDismiss?: () => void; } const ToolButton: React.FC = ({ @@ -54,7 +50,6 @@ const ToolButton: React.FC = ({ showDescription = false, onUnavailableClick, badgeCount, - onDismiss, }) => { const { t } = useTranslation(); const { config } = useAppConfig(); @@ -319,46 +314,9 @@ const ToolButton: React.FC = ({ /> ) : null; - const handleDismiss = (e: React.SyntheticEvent) => { - e.stopPropagation(); - e.preventDefault(); - onDismiss?.(); - }; - - const dismiss = onDismiss ? ( - { - e.stopPropagation(); - if (e.key === "Enter" || e.key === " ") handleDismiss(e); - }} - onMouseDown={(e: React.MouseEvent) => e.stopPropagation()} - className="tool-button-dismiss" - aria-label={t( - "toolPicker.recommendations.dismiss", - "Don't recommend this tool here", - )} - title={t( - "toolPicker.recommendations.dismiss", - "Don't recommend this tool here", - )} - > - - - ) : null; - return (
{star} - {dismiss} ({ + useToolRecommendations: vi.fn(), +})); +vi.mock("@app/contexts/NavigationContext", () => ({ + useNavigationState: vi.fn(), +})); +vi.mock("@app/contexts/ToolWorkflowContext", () => ({ + useToolWorkflow: vi.fn(), +})); +vi.mock("@app/hooks/useToolNavigation", () => ({ + useToolNavigation: () => ({ + getToolNavigation: (toolId: string) => ({ + href: `/${toolId}`, + onClick: () => {}, + }), + }), +})); + +const mockRecommendations = vi.mocked(useToolRecommendations); +const mockNavigation = vi.mocked(useNavigationState); +const mockWorkflow = vi.mocked(useToolWorkflow); + +/** A tool that can actually open, so it survives the availability filter. */ +function entry(name: string): ToolRegistryEntry { + return { + name, + icon: null, + component: (() => null) as unknown as ToolRegistryEntry["component"], + } as ToolRegistryEntry; +} + +const REGISTRY: Partial> = { + compress: entry("Compress"), + convert: entry("Convert"), + sanitize: entry("Sanitize"), + split: entry("Split"), + ocr: entry("OCR"), + addPassword: entry("Add Password"), + merge: entry("Merge"), + // No component and no link - nothing to open. + automate: { + name: "Automate", + icon: null, + component: null, + } as ToolRegistryEntry, +}; + +function setup( + recommendedToolIds: ToolId[] | null, + selectedTool: ToolId | null = null, +) { + mockRecommendations.mockReturnValue({ + recommendedToolIds, + contextTool: selectedTool, + }); + mockNavigation.mockReturnValue({ selectedTool } as ReturnType< + typeof useNavigationState + >); + mockWorkflow.mockReturnValue({ + getSelectedTool: (id: ToolId | null) => + id ? (REGISTRY[id] ?? null) : null, + } as unknown as ReturnType); + + return renderHook(() => useSuggestedTools()).result.current.map((t) => t.id); +} + +describe("useSuggestedTools", () => { + beforeEach(() => vi.clearAllMocks()); + + it("shows the curated list when the backend has no usage data", () => { + expect(setup(null)).toEqual(["compress", "convert", "sanitize", "split"]); + }); + + it("leads with the usage ranking, then tops up from the curated list", () => { + expect(setup(["addPassword", "merge"])).toEqual([ + "addPassword", + "merge", + "compress", + "convert", + ]); + }); + + it("never suggests the tool the user is currently in", () => { + expect(setup(["compress", "addPassword"], "compress")).toEqual([ + "addPassword", + "convert", + "sanitize", + "split", + ]); + }); + + it("skips tools that are unknown or cannot open", () => { + expect(setup(["automate", "nonsense" as ToolId, "merge"])).toEqual([ + "merge", + "compress", + "convert", + "sanitize", + ]); + }); + + it("does not repeat a ranked tool that is also in the curated list", () => { + const ids = setup(["split", "compress"]); + expect(ids).toEqual(["split", "compress", "convert", "sanitize"]); + expect(new Set(ids).size).toBe(ids.length); + }); +}); diff --git a/frontend/editor/src/core/hooks/useSuggestedTools.ts b/frontend/editor/src/core/hooks/useSuggestedTools.ts index 5deb5d3a0d..029b243a8b 100644 --- a/frontend/editor/src/core/hooks/useSuggestedTools.ts +++ b/frontend/editor/src/core/hooks/useSuggestedTools.ts @@ -2,81 +2,66 @@ import { useMemo } from "react"; import { useNavigationState } from "@app/contexts/NavigationContext"; import { useToolNavigation } from "@app/hooks/useToolNavigation"; import { useToolWorkflow } from "@app/contexts/ToolWorkflowContext"; +import { useToolRecommendations } from "@app/hooks/useToolRecommendations"; import { ToolId } from "@app/types/toolId"; -// Material UI Icons -import CompressIcon from "@mui/icons-material/Compress"; -import SwapHorizIcon from "@mui/icons-material/SwapHoriz"; -import CleaningServicesIcon from "@mui/icons-material/CleaningServices"; -import CropIcon from "@mui/icons-material/Crop"; -import TextFieldsIcon from "@mui/icons-material/TextFields"; - export interface SuggestedTool { id: ToolId; title: string; - icon: React.ComponentType; + icon: React.ReactNode; href: string; onClick: (e: React.MouseEvent) => void; } -const ALL_SUGGESTED_TOOLS: Omit[] = [ - { - id: "compress", - title: "Compress", - icon: CompressIcon, - }, - { - id: "convert", - title: "Convert", - icon: SwapHorizIcon, - }, - { - id: "sanitize", - title: "Sanitize", - icon: CleaningServicesIcon, - }, - { - id: "split", - title: "Split", - icon: CropIcon, - }, - { - id: "ocr", - title: "OCR", - icon: TextFieldsIcon, - }, +/** Shown when usage tracking is off or has nothing to say yet. */ +const FALLBACK_TOOL_IDS: ToolId[] = [ + "compress", + "convert", + "sanitize", + "split", + "ocr", ]; +const SUGGESTION_COUNT = 4; + +// A couple spare, so tools that cannot open still leave a full list. +const FETCH_LIMIT = SUGGESTION_COUNT + 2; + +/** + * What to do next with the file that just came out of a tool. + * + * Ranked by how this user, their team and the install actually use tools after + * the current one, and topped up from the curated list so the section never + * shrinks. Falls back to the curated list entirely when the backend has no + * usage data - a fresh install, or analytics turned off. + */ export function useSuggestedTools(): SuggestedTool[] { const { selectedTool } = useNavigationState(); const { getToolNavigation } = useToolNavigation(); const { getSelectedTool } = useToolWorkflow(); + const { recommendedToolIds } = useToolRecommendations(FETCH_LIMIT); return useMemo(() => { - // Filter out the current tool - const filteredTools = ALL_SUGGESTED_TOOLS.filter( - (tool) => tool.id !== selectedTool, - ); + const ordered = [...(recommendedToolIds ?? []), ...FALLBACK_TOOL_IDS]; + const suggestions: SuggestedTool[] = []; + const seen = new Set(); - // Add navigation props to each tool - return filteredTools.map((tool) => { - const toolRegistryEntry = getSelectedTool(tool.id); - if (!toolRegistryEntry) { - // Fallback for tools not in registry - return { - ...tool, - href: `/${tool.id}`, - onClick: (e: React.MouseEvent) => { - e.preventDefault(); - }, - }; - } + for (const id of ordered) { + if (id === selectedTool || seen.has(id)) continue; + const tool = getSelectedTool(id); + // A card that cannot open anything is worse than a shorter list. + if (!tool || (tool.component === null && !tool.link)) continue; - const navProps = getToolNavigation(tool.id, toolRegistryEntry); - return { - ...tool, - ...navProps, - }; - }); - }, [selectedTool, getToolNavigation, getSelectedTool]); + seen.add(id); + suggestions.push({ + id, + title: tool.name, + icon: tool.icon, + ...getToolNavigation(id, tool), + }); + if (suggestions.length === SUGGESTION_COUNT) break; + } + + return suggestions; + }, [recommendedToolIds, selectedTool, getToolNavigation, getSelectedTool]); } diff --git a/frontend/editor/src/core/hooks/useToolRecommendations.test.tsx b/frontend/editor/src/core/hooks/useToolRecommendations.test.tsx index f04df3ca1b..9f1f2de774 100644 --- a/frontend/editor/src/core/hooks/useToolRecommendations.test.tsx +++ b/frontend/editor/src/core/hooks/useToolRecommendations.test.tsx @@ -1,34 +1,20 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { renderHook, waitFor, act } from "@testing-library/react"; -import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; -import { qk } from "@app/query/keys"; import { TestQueryProvider } from "@app/tests/utils/TestQueryProvider"; -import { - fetchToolRecommendations, - dismissToolRecommendation, - undoDismissToolRecommendation, -} from "@app/api/toolRecommendations"; -import { - useDismissToolRecommendation, - useToolRecommendations, -} from "@app/hooks/useToolRecommendations"; +import { fetchToolRecommendations } from "@app/api/toolRecommendations"; +import { useToolRecommendations } from "@app/hooks/useToolRecommendations"; import { notifyToolCompleted, resetToolUsageTrackerForTests, } from "@app/services/toolUsageTracker"; vi.mock("@app/api/toolRecommendations", () => ({ - ANY_CONTEXT: "*", fetchToolRecommendations: vi.fn(), recordToolUsage: vi.fn().mockResolvedValue(undefined), - dismissToolRecommendation: vi.fn(), - undoDismissToolRecommendation: vi.fn(), })); const mockFetch = vi.mocked(fetchToolRecommendations); -const mockDismiss = vi.mocked(dismissToolRecommendation); -const mockUndo = vi.mocked(undoDismissToolRecommendation); describe("useToolRecommendations", () => { beforeEach(() => { @@ -94,89 +80,3 @@ describe("useToolRecommendations", () => { expect(result.current.contextTool).toBe("compare"); }); }); - -describe("useDismissToolRecommendation", () => { - beforeEach(() => { - vi.clearAllMocks(); - resetToolUsageTrackerForTests(); - }); - - it("persists the dismissal and returns a working undo", async () => { - mockDismiss.mockResolvedValue(undefined); - mockUndo.mockResolvedValue(undefined); - - const { result } = renderHook(() => useDismissToolRecommendation(), { - wrapper: TestQueryProvider, - }); - - const undo = await result.current("compare", "ocr"); - expect(mockDismiss).toHaveBeenCalledWith("compare", "ocr"); - - await undo(); - expect(mockUndo).toHaveBeenCalledWith("compare", "ocr"); - }); - - it("propagates failures so callers can surface an error toast", async () => { - mockDismiss.mockRejectedValue(new Error("boom")); - - const { result } = renderHook(() => useDismissToolRecommendation(), { - wrapper: TestQueryProvider, - }); - - await expect(result.current(null, "ocr")).rejects.toThrow("boom"); - }); - - it("a failing undo rejects so the caller can report it", async () => { - mockDismiss.mockResolvedValue(undefined); - mockUndo.mockRejectedValue(new Error("offline")); - - const { result } = renderHook(() => useDismissToolRecommendation(), { - wrapper: TestQueryProvider, - }); - - const undo = await result.current("compare", "ocr"); - await expect(undo()).rejects.toThrow("offline"); - }); - - it("optimistically hides the tool only in the context it was dismissed from", async () => { - mockFetch.mockImplementation(async (context) => - context === "compare" - ? [ - { toolKey: "ocr", score: 5 }, - { toolKey: "merge", score: 3 }, - ] - : [{ toolKey: "ocr", score: 4 }], - ); - mockDismiss.mockResolvedValue(undefined); - - const client = new QueryClient({ - defaultOptions: { queries: { retry: false } }, - }); - const wrapper = ({ children }: { children: React.ReactNode }) => ( - {children} - ); - - // Populate both a "compare" context list and the no-context list. - await client.fetchQuery({ - queryKey: qk.toolRecommendations("compare", 8), - queryFn: () => fetchToolRecommendations("compare", 8), - }); - await client.fetchQuery({ - queryKey: qk.toolRecommendations("*", 8), - queryFn: () => fetchToolRecommendations(null, 8), - }); - - const { result } = renderHook(() => useDismissToolRecommendation(), { - wrapper, - }); - await result.current("compare", "ocr"); - - expect(client.getQueryData(qk.toolRecommendations("compare", 8))).toEqual([ - { toolKey: "merge", score: 3 }, - ]); - // The other context keeps its entry: this dismissal does not apply there. - expect(client.getQueryData(qk.toolRecommendations("*", 8))).toEqual([ - { toolKey: "ocr", score: 4 }, - ]); - }); -}); diff --git a/frontend/editor/src/core/hooks/useToolRecommendations.ts b/frontend/editor/src/core/hooks/useToolRecommendations.ts index 08a12fa38a..b33c26825e 100644 --- a/frontend/editor/src/core/hooks/useToolRecommendations.ts +++ b/frontend/editor/src/core/hooks/useToolRecommendations.ts @@ -1,13 +1,8 @@ -import { useCallback, useMemo, useSyncExternalStore } from "react"; +import { useMemo, useSyncExternalStore } from "react"; -import { useQuery, useQueryClient } from "@tanstack/react-query"; +import { useQuery } from "@tanstack/react-query"; -import { - ANY_CONTEXT, - dismissToolRecommendation, - fetchToolRecommendations, - undoDismissToolRecommendation, -} from "@app/api/toolRecommendations"; +import { fetchToolRecommendations } from "@app/api/toolRecommendations"; import { qk } from "@app/query/keys"; import { getLastCompletedTool, @@ -16,6 +11,9 @@ import { import { isValidToolId, ToolId } from "@app/types/toolId"; const RECOMMENDATIONS_STALE_TIME = 2 * 60 * 1000; + +/** Query-key stand-in for "no tool has finished yet". */ +const NO_CONTEXT = "*"; export const DEFAULT_RECOMMENDATION_LIMIT = 8; /** The tool the user most recently completed; recommendations answer "what next after it". */ @@ -24,8 +22,9 @@ export function useRecommendationContextTool(): ToolId | null { } /** - * Usage-ranked tool ids for the recommended section, or null when the backend - * has no data (or no recommendations API) and the static list should be shown. + * Usage-ranked tool ids for the "what next" suggestions shown after a tool + * finishes, or null when the backend has no data (or no recommendations API) + * and the curated list should be shown instead. */ export function useToolRecommendations( limit: number = DEFAULT_RECOMMENDATION_LIMIT, @@ -36,7 +35,7 @@ export function useToolRecommendations( const contextTool = useRecommendationContextTool(); const { data } = useQuery({ - queryKey: qk.toolRecommendations(contextTool ?? ANY_CONTEXT, limit), + queryKey: qk.toolRecommendations(contextTool ?? NO_CONTEXT, limit), queryFn: () => fetchToolRecommendations(contextTool, limit), staleTime: RECOMMENDATIONS_STALE_TIME, retry: false, @@ -50,46 +49,3 @@ export function useToolRecommendations( return { recommendedToolIds, contextTool }; } - -/** - * Dismisses a recommendation for the given context (or everywhere when the - * context is null), with an optimistic cache update; returns an undo callback. - */ -export function useDismissToolRecommendation(): ( - contextTool: ToolId | null, - dismissedTool: ToolId, -) => Promise<() => Promise> { - const queryClient = useQueryClient(); - - return useCallback( - async (contextTool: ToolId | null, dismissedTool: ToolId) => { - // Dismissals are context-scoped, so only that context's cached lists are - // touched; the key prefix stops short of the limit to cover every variant. - const contextKey = [ - "editor", - "toolRecommendations", - contextTool ?? ANY_CONTEXT, - ]; - const invalidate = () => - void queryClient.invalidateQueries({ queryKey: contextKey }); - - queryClient.setQueriesData<{ toolKey: string; score: number }[] | null>( - { queryKey: contextKey }, - (existing) => - existing - ? existing.filter((r) => r.toolKey !== dismissedTool) - : existing, - ); - try { - await dismissToolRecommendation(contextTool, dismissedTool); - } finally { - invalidate(); - } - return async () => { - await undoDismissToolRecommendation(contextTool, dismissedTool); - invalidate(); - }; - }, - [queryClient], - ); -} diff --git a/frontend/editor/src/core/hooks/useToolSections.test.tsx b/frontend/editor/src/core/hooks/useToolSections.test.tsx deleted file mode 100644 index 100bd55f52..0000000000 --- a/frontend/editor/src/core/hooks/useToolSections.test.tsx +++ /dev/null @@ -1,223 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; -import { renderHook } from "@testing-library/react"; - -import { - SubcategoryId, - ToolCategoryId, - ToolRegistryEntry, -} from "@app/data/toolsTaxonomy"; -import { useToolSections } from "@app/hooks/useToolSections"; -import { useToolRecommendations } from "@app/hooks/useToolRecommendations"; -import { ToolId } from "@app/types/toolId"; - -// useToolSections imports the limit from here too, so the mock must supply it. -const LIMIT = 8; -vi.mock("@app/hooks/useToolRecommendations", () => ({ - DEFAULT_RECOMMENDATION_LIMIT: 8, - useToolRecommendations: vi.fn(), -})); - -const mockUseToolRecommendations = vi.mocked(useToolRecommendations); - -function makeTool( - overrides: Partial = {}, -): ToolRegistryEntry { - return { - icon: null, - name: "Tool", - component: (() => null) as never, - description: "", - categoryId: ToolCategoryId.STANDARD_TOOLS, - subcategoryId: SubcategoryId.GENERAL, - automationSettings: null, - ...overrides, - } as ToolRegistryEntry; -} - -function entry(id: string, tool: ToolRegistryEntry) { - return { item: [id as ToolId, tool] as [ToolId, ToolRegistryEntry] }; -} - -const registryFixture = [ - entry( - "merge", - makeTool({ - name: "Merge", - categoryId: ToolCategoryId.RECOMMENDED_TOOLS, - subcategoryId: SubcategoryId.GENERAL, - }), - ), - entry( - "compare", - makeTool({ - name: "Compare", - categoryId: ToolCategoryId.RECOMMENDED_TOOLS, - subcategoryId: SubcategoryId.DOCUMENT_REVIEW, - }), - ), - entry( - "ocr", - makeTool({ name: "OCR", subcategoryId: SubcategoryId.EXTRACTION }), - ), - entry( - "split", - makeTool({ name: "Split", subcategoryId: SubcategoryId.PAGE_FORMATTING }), - ), - entry( - "removePassword", - makeTool({ - name: "Remove password", - subcategoryId: SubcategoryId.DOCUMENT_SECURITY, - // Not ready: no component and no link, so never shown in Quick Access. - component: null, - }), - ), -]; - -type SectionsResult = { - sections: { key: string; subcategories: { tools: { id: ToolId }[] }[] }[]; - rankedRecommendationIds: Set; -}; - -function sectionIds(result: SectionsResult, key: string): ToolId[] { - const section = result.sections.find((s) => s.key === key); - return section - ? section.subcategories.flatMap((sc) => sc.tools.map((t) => t.id)) - : []; -} - -const quickIds = (result: SectionsResult) => sectionIds(result, "quick"); -const allIds = (result: SectionsResult) => sectionIds(result, "all"); -const rankedIds = (result: SectionsResult) => [ - ...result.rankedRecommendationIds, -]; - -describe("useToolSections recommendations", () => { - beforeEach(() => { - vi.clearAllMocks(); - }); - - it("keeps the static recommended list when no usage data exists", () => { - mockUseToolRecommendations.mockReturnValue({ - recommendedToolIds: null, - contextTool: null, - }); - - const { result } = renderHook(() => useToolSections(registryFixture)); - - expect(quickIds(result.current)).toEqual(["compare", "merge"]); - expect(rankedIds(result.current)).toEqual([]); - }); - - it("leads with the usage ranking in score order, then tops up from the static list", () => { - mockUseToolRecommendations.mockReturnValue({ - recommendedToolIds: ["split", "ocr", "merge"] as ToolId[], - contextTool: null, - }); - - const { result } = renderHook(() => useToolSections(registryFixture)); - - // 'compare' is the only curated entry the ranking did not already cover. - expect(quickIds(result.current)).toEqual([ - "split", - "ocr", - "merge", - "compare", - ]); - expect(rankedIds(result.current)).toEqual(["split", "ocr", "merge"]); - }); - - it("drops recommended ids that are unknown or not ready", () => { - mockUseToolRecommendations.mockReturnValue({ - recommendedToolIds: ["removePassword", "ocr", "automate"] as ToolId[], - contextTool: null, - }); - - const { result } = renderHook(() => useToolSections(registryFixture)); - - expect(quickIds(result.current)).toEqual(["ocr", "compare", "merge"]); - expect(rankedIds(result.current)).toEqual(["ocr"]); - }); - - it("falls back to the static list when no recommended id survives filtering", () => { - mockUseToolRecommendations.mockReturnValue({ - recommendedToolIds: ["automate"] as ToolId[], - contextTool: null, - }); - - const { result } = renderHook(() => useToolSections(registryFixture)); - - expect(quickIds(result.current)).toEqual(["compare", "merge"]); - expect(rankedIds(result.current)).toEqual([]); - }); - - it("never lists a tool in both Quick Access and All Tools", () => { - mockUseToolRecommendations.mockReturnValue({ - recommendedToolIds: ["ocr", "merge"] as ToolId[], - contextTool: null, - }); - - const { result } = renderHook(() => useToolSections(registryFixture)); - - const quick = quickIds(result.current); - const all = allIds(result.current); - expect(quick).toEqual(["ocr", "merge", "compare"]); - expect(all.filter((id) => quick.includes(id))).toEqual([]); - }); - - it("keeps the statically recommended tools in Quick Access when the ranking omits them", () => { - // The regression this guards: a couple of tool runs used to replace the whole - // curated list, collapsing Quick Access to one or two entries on a fresh install. - mockUseToolRecommendations.mockReturnValue({ - recommendedToolIds: ["ocr"] as ToolId[], - contextTool: null, - }); - - const { result } = renderHook(() => useToolSections(registryFixture)); - - expect(quickIds(result.current)).toEqual(["ocr", "compare", "merge"]); - expect(allIds(result.current)).not.toContain("merge"); - }); - - it("tops the quick list up to the limit and no further", () => { - const curated = Array.from({ length: LIMIT }, (_, i) => - entry( - `static${i}`, - makeTool({ - name: `Static ${i}`, - categoryId: ToolCategoryId.RECOMMENDED_TOOLS, - subcategoryId: SubcategoryId.GENERAL, - }), - ), - ); - mockUseToolRecommendations.mockReturnValue({ - recommendedToolIds: ["ocr"] as ToolId[], - contextTool: null, - }); - - const { result } = renderHook(() => - useToolSections([...curated, entry("ocr", makeTool({ name: "OCR" }))]), - ); - - // One ranked tool leads; the curated entries fill the remaining slots. - const quick = quickIds(result.current); - expect(quick).toHaveLength(LIMIT); - expect(quick[0]).toBe("ocr"); - expect(quick).not.toContain(`static${LIMIT - 1}`); - }); - - it("still hides the static recommended tools from All Tools when no ranking exists", () => { - mockUseToolRecommendations.mockReturnValue({ - recommendedToolIds: null, - contextTool: null, - }); - - const { result } = renderHook(() => useToolSections(registryFixture)); - - expect(quickIds(result.current)).toEqual(["compare", "merge"]); - expect(allIds(result.current)).not.toContain("merge"); - expect(allIds(result.current)).toEqual( - expect.arrayContaining(["ocr", "split", "removePassword"]), - ); - }); -}); diff --git a/frontend/editor/src/core/hooks/useToolSections.ts b/frontend/editor/src/core/hooks/useToolSections.ts index e4908a2b80..84ccc22c83 100644 --- a/frontend/editor/src/core/hooks/useToolSections.ts +++ b/frontend/editor/src/core/hooks/useToolSections.ts @@ -7,16 +7,8 @@ import { ToolRegistryEntry, } from "@app/data/toolsTaxonomy"; import { useTranslation } from "react-i18next"; -import { - DEFAULT_RECOMMENDATION_LIMIT, - useToolRecommendations, -} from "@app/hooks/useToolRecommendations"; import { ToolId } from "@app/types/toolId"; -/** Tools that can actually open: have a component, an external link, or are navigational. */ -const isReadyTool = ({ tool, id }: { tool: ToolRegistryEntry; id: ToolId }) => - tool.component !== null || !!tool.link || id === "read" || id === "multiTool"; - type SubcategoryIdMap = { [subcategoryId in SubcategoryId]: Array<{ id: ToolId; @@ -52,7 +44,6 @@ export function useToolSections( searchQuery?: string, ) { const { t } = useTranslation(); - const { recommendedToolIds } = useToolRecommendations(); const groupedTools = useMemo(() => { if (!filteredTools || !Array.isArray(filteredTools)) { @@ -71,12 +62,46 @@ export function useToolSections( return grouped; }, [filteredTools]); - const { sections, rankedRecommendationIds } = useMemo(() => { + const sections: ToolSection[] = useMemo(() => { const getOrderIndex = (id: SubcategoryId) => { const idx = SUBCATEGORY_ORDER.indexOf(id); return idx === -1 ? Number.MAX_SAFE_INTEGER : idx; }; + const quick = {} as SubcategoryIdMap; + const all = {} as SubcategoryIdMap; + + Object.entries(groupedTools).forEach(([c, subs]) => { + const categoryId = c as ToolCategoryId; + + Object.entries(subs).forEach(([s, tools]) => { + const subcategoryId = s as SubcategoryId; + // Build the 'all' collection without duplicating recommended tools + // Recommended tools are shown in the Quick section only + if (categoryId !== ToolCategoryId.RECOMMENDED_TOOLS) { + if (!all[subcategoryId]) all[subcategoryId] = []; + all[subcategoryId].push(...tools); + } + }); + + if (categoryId === ToolCategoryId.RECOMMENDED_TOOLS) { + Object.entries(subs).forEach(([s, tools]) => { + const subcategoryId = s as SubcategoryId; + if (!quick[subcategoryId]) quick[subcategoryId] = []; + // Only include ready tools (have a component or external link) in Quick Access + // Special case: read and multiTool are navigational tools that don't need components + const readyTools = tools.filter( + ({ tool, id }) => + tool.component !== null || + !!tool.link || + id === "read" || + id === "multiTool", + ); + quick[subcategoryId].push(...readyTools); + }); + } + }); + const sortSubs = (obj: SubcategoryIdMap) => Object.entries(obj) .sort(([a], [b]) => { @@ -92,63 +117,6 @@ export function useToolSections( ({ subcategoryId, tools }) as SubcategoryGroup, ); - // Every tool starts in 'all'; whatever Quick Access ends up showing is removed - // from it below, so a tool is never listed twice nor lost when the quick list changes. - let quick = {} as SubcategoryIdMap; - const all = {} as SubcategoryIdMap; - - Object.entries(groupedTools).forEach(([c, subs]) => { - const categoryId = c as ToolCategoryId; - - Object.entries(subs).forEach(([s, tools]) => { - const subcategoryId = s as SubcategoryId; - if (!all[subcategoryId]) all[subcategoryId] = []; - all[subcategoryId].push(...tools); - - if (categoryId === ToolCategoryId.RECOMMENDED_TOOLS) { - if (!quick[subcategoryId]) quick[subcategoryId] = []; - // Only include ready tools (have a component or external link) in Quick Access - // Special case: read and multiTool are navigational tools that don't need components - quick[subcategoryId].push(...tools.filter(isReadyTool)); - } - }); - }); - - // Ranked tools lead, curated ones top the list back up - a couple of runs may - // reorder Quick Access but must never shrink it. One bucket keeps score order. - const ranked = new Set(); - if (recommendedToolIds) { - const byId = new Map(); - filteredTools.forEach(({ item: [id, tool] }) => byId.set(id, tool)); - const dynamicTools = recommendedToolIds - .filter((id) => byId.has(id)) - .map((id) => ({ id, tool: byId.get(id)! })) - .filter(isReadyTool); - if (dynamicTools.length > 0) { - dynamicTools.forEach(({ id }) => ranked.add(id)); - const topUp = sortSubs(quick) - .flatMap(({ tools }) => tools) - .filter(({ id }) => !ranked.has(id)); - quick = { - [SubcategoryId.GENERAL]: [...dynamicTools, ...topUp].slice( - 0, - Math.max(dynamicTools.length, DEFAULT_RECOMMENDATION_LIMIT), - ), - } as SubcategoryIdMap; - } - } - - const quickIds = new Set( - Object.values(quick).flatMap((tools) => tools.map(({ id }) => id)), - ); - Object.keys(all).forEach((key) => { - const subcategoryId = key as SubcategoryId; - all[subcategoryId] = all[subcategoryId].filter( - ({ id }) => !quickIds.has(id), - ); - if (all[subcategoryId].length === 0) delete all[subcategoryId]; - }); - const built: ToolSection[] = [ { key: "quick", @@ -162,13 +130,10 @@ export function useToolSections( }, ]; - return { - sections: built.filter((section) => - section.subcategories.some((sc) => sc.tools.length > 0), - ), - rankedRecommendationIds: ranked, - }; - }, [groupedTools, recommendedToolIds, filteredTools, t]); + return built.filter((section) => + section.subcategories.some((sc) => sc.tools.length > 0), + ); + }, [groupedTools]); const searchGroups: SubcategoryGroup[] = useMemo(() => { if (!filteredTools || !Array.isArray(filteredTools)) { @@ -217,5 +182,5 @@ export function useToolSections( ); }, [filteredTools, searchQuery]); - return { sections, searchGroups, rankedRecommendationIds }; + return { sections, searchGroups }; }