Skip to content

feat: add jest tests#443

Open
shyim wants to merge 1 commit into
mainfrom
feat/jest-tests
Open

feat: add jest tests#443
shyim wants to merge 1 commit into
mainfrom
feat/jest-tests

Conversation

@shyim

@shyim shyim commented Jul 19, 2026

Copy link
Copy Markdown
Member

No description provided.

@greptile-apps

greptile-apps Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR introduces a ft-command-palette component (opened with Ctrl/⌘+K or the sidebar search button) that lets users jump to any Tools tab and trigger common actions (clear caches, compile themes, reset queue, register tasks). Alongside the new feature it ships a proper Jest test infrastructure — a shared mountFrosh helper that mounts components with the real ft-* design-system tree and real translations instead of per-spec stubs — and migrates all existing specs to use it.

  • New ft-command-palette: keyboard-navigable command palette with filtering/ranking, confirmation flow for destructive actions, and localStorage-backed "recent" list.
  • New test infrastructure (test/mount.js, test/resolve-admin-path.js, test/run.js): piggybacks on the Shopware installation's own Jest binary and harness; mountFrosh eliminates stub-translated snippet keys from assertions.
  • Existing spec upgrades: all component tests now mount with real child components and real en-GB translations, replacing stub-based $t: (key) => key mocks and enabling DOM-level assertions on rendered text and CSS classes.

Confidence Score: 4/5

Safe to merge — the new command palette and test infrastructure are well-structured and the existing tests have been meaningfully upgraded.

The only concrete inaccuracy in the changed code is the clear-all-caches action reporting list.length pools cleared while silently skipping unnamed pools. The test for clearCacheIfPresent also never exercises its error branch. Neither issue is blocking.

commands.js (success count logic) and commands.spec.js (missing error-path coverage)

Important Files Changed

Filename Overview
.github/workflows/admin-jest.yml New CI workflow running jest against Shopware 6.6.x and trunk; delegates to the shared shopware/github-actions reusable workflow with coverage upload.
src/Resources/app/administration/test/mount.js Shared integration mount helper; builds real ft-* child components and wires real en-GB/de-DE snippets through vue-i18n, eliminating per-spec boilerplate stubs and stub-translated keys.
src/Resources/app/administration/src/module/frosh-tools/component/ft-command-palette/commands.js Defines all palette commands and the filterCommands ranking function. One minor correctness issue: clearAllCaches reports list.length pools cleared but skips unnamed pools during iteration.
src/Resources/app/administration/src/module/frosh-tools/component/ft-command-palette/index.js Full command palette component: keyboard navigation, confirmation flow, recent-command memory in localStorage, and proper scroll-to-active. Two instance properties (previousOverflow, previousActiveElement) not declared in data().
src/Resources/app/administration/src/module/frosh-tools/component/ft-command-palette/commands.spec.js Tests filtering/ranking and command execution; the "reports missing ones" path of clearCacheIfPresent is never actually exercised despite the test title implying otherwise.
src/Resources/app/administration/src/module/frosh-tools/component/ft-command-palette/index.spec.js Integration tests for the full palette: rendering, filtering, navigation, confirmation flow, Escape handling, and feature-flag gating.
src/Resources/app/administration/src/module/frosh-tools/page/index/index.js Adds commandPaletteOpen state and wires up Ctrl/Cmd+K via a capture-phase document listener; correctly cleaned up in unmounted().

Comments Outside Diff (3)

  1. src/Resources/app/administration/src/module/frosh-tools/component/ft-command-palette/commands.js, line 773-786 (link)

    P2 Success count may overstate pools actually cleared

    The notification reports list.length (all pools returned by getCacheInfo) as the cleared count, but only pools whose pool.name is truthy are passed to clearCache. If any pool object arrives without a name, those are skipped silently and the user is told more caches were cleared than actually were — e.g., "Cleared 5 cache pool(s)" when only 4 were targeted. A local cleared counter incremented inside the guard would keep the message accurate.

    Fix in Claude Code

  2. src/Resources/app/administration/src/module/frosh-tools/component/ft-command-palette/commands.spec.js, line 141-165 (link)

    P2 "reports missing ones" branch is never exercised

    The test is titled "clears matching cache pools and reports missing ones" but only tests the success path (a matching pool is found). There is no assertion that notifyError is called and clearCache is not called when getCacheInfo returns pools whose names do not match any of the candidates. Adding a second it block where getCacheInfo returns [{ name: 'object' }] for the HTTP clear command (no http_cache present) would verify notifyError fires and clearCache is never called.

    Fix in Claude Code

  3. src/Resources/app/administration/src/module/frosh-tools/component/ft-command-palette/index.js, line 1368-1382 (link)

    P2 Non-reactive instance properties not declared in data()

    this.previousOverflow and this.previousActiveElement are assigned directly on the component instance inside mounted() but are not declared in data(). Vue 3 uses a Proxy for the component instance, and properties set outside the Options API declarations are stored on the raw object — they work correctly here because they do not need reactivity and are consumed synchronously in unmounted(). Declaring them in data() (as null) would make the intent explicit and eliminate any ambiguity for readers or future refactors.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

    Fix in Claude Code

Fix All in Claude Code

Reviews (1): Last reviewed commit: "feat: add jest tests" | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant