Skip to content

Conversation

@mdatelle
Copy link
Contributor

@mdatelle mdatelle commented Apr 2, 2025

Summary by CodeRabbit

  • New Features

    • Introduced comprehensive testing utilities for Vue components utilizing the composition API.
    • Enhanced testing coverage for the DownloadApiLogs and KeyActions components, ensuring robust functionality and user interaction validation.
    • Added mock implementations for various libraries and components to facilitate isolated unit testing.
    • Improved flexibility in the DummyServerSwitcher component's input handling.
    • Added a new test setup file to configure the testing environment for Vue applications.
    • Added new test files for AuthComponent and KeyActions with comprehensive test cases.
    • Introduced a new mock implementation for UI components to streamline testing.
    • Added a new mock implementation for the useRequest composable to prevent hanging issues during tests.
    • Added a new mock implementation for the server store used by the Auth component.
  • Bug Fixes

    • Improved sanitization process to block inline styles for a safer and more consistent display.
  • Documentation

    • Added README documentation for Vue Component Testing Utilities, detailing usage and examples.
    • Updated ESLint configuration to ignore coverage directory files.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Apr 2, 2025

Walkthrough

The pull request reorganizes component imports, updates configuration files, adds new test files, and introduces multiple mock implementations. Changes include updates in Vue component imports, modifications in package.json scripts and dependencies, improved DOMPurify sanitization, and enhanced test setups with comprehensive mocks for Apollo, Pinia, Vue, and others. Additionally, new utilities for component mocking and detailed example tests for key components (e.g., DownloadApiLogs and KeyActions) are included.

Changes

File(s) Change Summary
web/components/Auth.ce.vue, web/package.json, web/scripts/add-timestamp...js Reorganized imports in the Auth component; updated package.json with testing scripts and new dev dependencies; removed an extra blank line in the manifest script.
web/test/components/*.test.ts Added new test files for AuthComponent, DownloadApiLogs, and KeyActions covering rendering, event handling, and state-based behavior.
web/test/setup.ts, web/vitest.config.mjs, web/nuxt.config.ts Introduced a test setup file, added a new Vitest configuration, and updated Nuxt configuration with server-side rendering settings and module transpilation options.
web/helpers/markdown.ts Updated the DOMPurify.sanitize method with explicit options to forbid <style> tags and style attributes.
web/test/mocks/* (apollo-client, shared-callbacks, pinia, services, stores, ui-, vue-) Added comprehensive mock implementations for Apollo Client, shared callbacks, Pinia, services (including useRequest), various stores (errors, server), UI components, UI libraries, vue-i18n, and Vue composition API.
web/test/mocks/utils/* (README.md, component-mock.ts, examples/...) Introduced utilities for creating mock Vue components and list-component factories, along with documentation and example tests for KeyActions.

Sequence Diagram(s)

sequenceDiagram
    participant T as Test Suite
    participant C as DownloadApiLogs Component
    participant G as Global (CSRF Token)
    
    T->>G: Set CSRF token
    T->>C: Mount Component
    C->>C: Compute download URL with CSRF token
    C->>T: Render button & links with proper attributes
    T->>C: Simulate user interactions
    C->>T: Handle click events & emit actions
Loading
sequenceDiagram
    participant T as Test Suite
    participant K as KeyActions Component
    participant S as Store (KeyActions)
    
    T->>K: Mount component with/without actions prop
    alt Actions provided via props
        K->>T: Render provided actions
    else No actions provided
        K->>S: Retrieve keyActions from store
        S-->>K: Return keyActions
    end
    K->>T: Render action buttons
    T->>K: Simulate button click event
    K->>T: Trigger corresponding click handler
Loading

Possibly related PRs

  • chore(web): add pinia store and select dropdown for dummy server state #1143: The changes in the main PR, specifically the addition of the storeToRefs function from the pinia package in the Auth.ce.vue file, are related to the changes in the retrieved PR, which also involves the use of Pinia for state management in the DummyServerSwitcher.vue component.
  • refactor: unraid ui cleanup and migration #998: The changes in the main PR, specifically the addition of the BrandButton component from @unraid/ui in the Auth.ce.vue file, are related to the modifications in the retrieved PR, which also involves the introduction of the BrandButton component in the context of UI refactoring. Both PRs focus on integrating and utilizing the BrandButton component, indicating a direct connection at the code level.

Poem

A symphony of code unfolds on our screen,
Tests and mocks join in a meticulous scene.
Imports rearranged, settings refined so neat,
Every component and utility now in sync and complete.
Here's to our journey, where each bug meets its defeat!
🎶 Code and tests, together they beat.


🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (3)
web/test/setup.ts (1)

1-17: Well-structured test setup file

Good configuration of Vue Test Utils with Pinia testing integration. The test lifecycle hooks are properly defined for future use.

Consider adding a brief comment explaining what specific setup/cleanup might be needed in the future, or remove the empty hooks if they're just placeholders.

web/test/components/Auth.test.ts (2)

39-54: Consider moving i18n setup to a shared test utility.

The i18n setup could potentially be reused across multiple test files. Consider extracting this to a shared utility function.

// test/utils/i18n.ts
import { createI18n } from 'vue-i18n';

export function createTestI18n(messages = {}) {
  return createI18n({
    legacy: false,
    locale: 'en',
    messages: {
      en: {
        ...messages
      }
    }
  });
}

77-87: Consider adding negative test cases.

The test verifies that the button renders correctly when authAction is present, but doesn't check that the button is not rendered when authAction is undefined.

Add a test like this:

it('does not render the auth button when authAction is not present', () => {
  mockAuthAction.value = undefined;
  
  render(AuthComponent, {
    global: {
      plugins: [i18n],
    },
  });

  expect(screen.queryByText('Test Button')).toBeNull();
});
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7a421ce and 2068cd4.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (5)
  • web/components/Auth.ce.vue (1 hunks)
  • web/package.json (2 hunks)
  • web/test/components/Auth.test.ts (1 hunks)
  • web/test/setup.ts (1 hunks)
  • web/vitest.config.ts (1 hunks)
🧰 Additional context used
🪛 GitHub Check: Build Web App
web/test/components/Auth.test.ts

[failure] 11-11:
'useServerStore' is defined but never used. Allowed unused vars must match /^_/u

web/vitest.config.ts

[failure] 26-26:
Unexpected any. Specify a different type

🪛 GitHub Actions: CI - Main (API)
web/test/components/Auth.test.ts

[error] 11-11: 'useServerStore' is defined but never used. Allowed unused vars must match /^_/u @typescript-eslint/no-unused-vars

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (5)
web/components/Auth.ce.vue (1)

2-3: Import organization looks good!

Clean organization of imports with external libraries at the top followed by components and internal modules.

web/test/components/Auth.test.ts (4)

14-37: Well-structured mock implementation.

Good approach using reactive refs to mock the store state. This allows for easy updates to the mock values during tests.


56-75: Good test setup with proper state reset.

The beforeEach function properly resets the state and mocks before each test, which helps maintain test isolation.


89-111: Good error state testing.

This test effectively verifies the error state rendering. Consider adding a check for the presence of the error icon as well.


113-141: Comprehensive interaction testing.

Good job testing the button click interaction. This ensures the component correctly passes user actions to the store.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (6)
web/nuxt.config.ts (1)

44-45: Small formatting issue

There's an extra empty line after the eslint module that wasn't there before. While this doesn't affect functionality, it's a minor inconsistency with the rest of the file's formatting.

  modules: [
    '@vueuse/nuxt',
    '@pinia/nuxt',
    '@nuxtjs/tailwindcss',
    ...(process.env.NODE_ENV !== 'test' ? ['nuxt-custom-elements'] : []),
    '@nuxt/eslint',
-

  ],
+  ],
web/test/mocks/apollo-client.ts (1)

1-24: Good Apollo Client mock for testing

The implementation provides a solid foundation for testing components that depend on Apollo Client without making actual network requests. The mocked fetch function returning an empty data object is a clean approach.

However, consider making this more flexible for different test scenarios:

-    fetch: (_input: RequestInfo | URL, _init?: RequestInit) => {
-      // Mock the fetch response
-      return Promise.resolve(new Response(JSON.stringify({ data: {} })));
-    },
+    fetch: (input: RequestInfo | URL, _init?: RequestInit) => {
+      // Mock the fetch response with customizable data based on the request
+      const mockResponses: Record<string, any> = {
+        // Add specific responses for different queries/mutations here
+        // Example: 'getUserQuery': { user: { id: 1, name: 'Test User' } }
+      };
+      
+      // Default empty response if no specific mock is defined
+      const defaultResponse = { data: {} };
+      
+      // Try to match the request with a specific mock response
+      const url = input instanceof Request ? input.url : input.toString();
+      const responseData = mockResponses[url] || defaultResponse;
+      
+      return Promise.resolve(new Response(JSON.stringify(responseData)));
+    },
web/test/mocks/shared-callbacks.ts (1)

5-22: Good mock implementation, but consider enhancing with error scenarios.

The mock implementation provides the basic functionality needed for tests, but it doesn't simulate error conditions or edge cases. This might lead to tests that don't fully exercise error handling in the production code.

Consider adding the ability to simulate errors or validation failures:

export const mockSharedCallbacks = {
  encrypt: (data: string, _key: string) => {
+   // Allow tests to trigger encryption errors with specific sentinel values
+   if (data === 'MOCK_ENCRYPTION_ERROR') {
+     throw new Error('Mock encryption error');
+   }
    return data; // Simple mock that returns the input data
  },
  decrypt: (data: string, _key: string) => {
+   // Allow tests to trigger decryption errors with specific sentinel values
+   if (data === 'MOCK_DECRYPTION_ERROR') {
+     throw new Error('Mock decryption error');
+   }
    return data; // Simple mock that returns the input data
  },
  useCallback: ({ encryptionKey: _encryptionKey }: { encryptionKey: string }) => {
    return {
      send: (_payload: SendPayloads) => {
+       // Allow tests to simulate network errors or other failures
+       if (_payload.type === 'MOCK_SEND_ERROR') {
+         return Promise.reject(new Error('Mock send error'));
+       }
        return Promise.resolve();
      },
      watcher: () => {
        return null;
      }
    };
  }
};
web/test/components/Auth.test.ts (1)

33-48: Consider extracting test fixture setup to a beforeEach hook.

The current implementation defines reusable variables inside the renderComponent function. This works, but extracting this to a beforeEach hook can make the test more readable and maintainable.

describe('AuthComponent', () => {
+  let mockAuthAction;
+  let mockStateData;
+
+  beforeEach(() => {
+    mockAuthAction = ref('authenticate');
+    mockStateData = ref({
+      error: false,
+      message: '',
+    });
+  });
+
  const renderComponent = (props = {}) => {
-    const mockAuthAction = ref('authenticate');
-    const mockStateData = ref({
-      error: false,
-      message: '',
-    });

    return mount(AuthComponent, {
      props: {
        authAction: mockAuthAction.value,
        stateData: mockStateData.value,
        ...props,
      },
    });
  };
web/package.json (2)

127-132: Document the need for the crypto-js patch.

The patch for @unraid/shared-callbacks suggests there's an issue with crypto-js imports that needed fixing, but there's no documentation explaining the patch's purpose.

Add a comment explaining the purpose of the patch:

  "pnpm": {
    "patchedDependencies": {
+     // Patch required to fix incorrect imports in shared-callbacks using 'crypto-js/aes.js' instead of 'crypto-js/aes'
      "@unraid/shared-callbacks@1.0.1": "patches/@unraid/shared-callbacks@1.0.1/fix-crypto-js-imports.patch"
    }
  }

34-36: Document the switch from vitest to nuxt test commands.

The test scripts have been changed from using vitest directly to using nuxt test commands. This is a significant change that should be documented.

Add comments explaining the switch:

  "// Testing": "",
+  // Using nuxt test instead of vitest directly to ensure proper integration with Nuxt.js
  "test": "nuxt test",
  "test:watch": "nuxt test --watch",
  "test:ci": "nuxt test run",
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 1c81503 and 398c247.

📒 Files selected for processing (9)
  • web/helpers/markdown.ts (1 hunks)
  • web/nuxt.config.ts (1 hunks)
  • web/package.json (5 hunks)
  • web/test/components/Auth.test.ts (1 hunks)
  • web/test/mocks/apollo-client.ts (1 hunks)
  • web/test/mocks/apollo.ts (1 hunks)
  • web/test/mocks/shared-callbacks.ts (1 hunks)
  • web/test/setup.ts (1 hunks)
  • web/vitest.config.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • web/vitest.config.ts
  • web/test/setup.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
web/nuxt.config.ts (1)

43-43: Smart approach for test configuration

Conditionally including the 'nuxt-custom-elements' module only when not in test mode is a good practice. This prevents potential issues with custom elements during test execution, making tests more reliable.

web/helpers/markdown.ts (1)

8-11: Security enhancement for HTML sanitization

Great improvement by explicitly forbidding style tags and attributes in the sanitized output. This prevents potential CSS-based injection attacks while maintaining the core sanitization functionality.

web/test/mocks/shared-callbacks.ts (1)

25-34:

❓ Verification inconclusive

Ensure mock behavior is consistent with actual implementation.

The current mock implementation for crypto-js/aes seems to assume the original functions return objects with toString methods. It's important to verify this matches the actual behavior of the crypto-js library.


🌐 Web query:

How does crypto-js/aes encrypt and decrypt methods return their results? Do they return objects with toString methods?

💡 Result:

The CryptoJS.AES.encrypt and CryptoJS.AES.decrypt methods return specialized objects that include toString() methods for convenient string conversion. Here's how they work:

Encryption Result (CipherParams Object)

  • Returns: An object of type CipherParams containing encryption details[3][7][10].
  • Key properties:
    • ciphertext: Raw encrypted data (WordArray)
    • key: Derived encryption key
    • iv: Initialization vector
    • salt: Random salt used (if applicable)
  • toString() behavior:
    const ciphertext = CryptoJS.AES.encrypt('message', 'secret').toString();
    // Returns OpenSSL-formatted string (Base64 encoded with salt/IV information)

Decryption Result (WordArray Object)

  • Returns: A WordArray representing raw decrypted bytes[3][7][9]
  • toString() requirement:
    const bytes = CryptoJS.AES.decrypt(ciphertext, 'secret');
    const plaintext = bytes.toString(CryptoJS.enc.Utf8); // Explicit encoding needed

Key Characteristics

Aspect Encryption Decryption
Return Type CipherParams WordArray
Automatic String Conversion Yes (Base64) No
Required for Readable Output .toString() .toString(encoding)
Contains Metadata IV, salt, ciphertext Raw decrypted bytes

The objects provide structured access to cryptographic components while enabling easy string serialization through their toString() methods[3][7][10]. Proper encoding specification during decryption is crucial to avoid misinterpretation of binary data[9].

Citations:


Mock behavior verification for crypto-js/AES

  • The web documentation confirms that both CryptoJS.AES.encrypt and CryptoJS.AES.decrypt return objects that include a toString() method.
  • Your mock for the encrypt method correctly simulates the CipherParams object’s string conversion.
  • For decrypt, note that the actual method returns a WordArray that typically requires an encoding (e.g., CryptoJS.enc.Utf8) when calling toString() to yield a proper plaintext string.
  • Verify that your tests do not rely on this encoding nuance—in which case the current mock is acceptable—or otherwise update the mock to simulate this behavior more accurately.
web/package.json (1)

50-50: Update @pinia/testing to the latest compatible version.

The @pinia/testing package is set to version 0.1.0, but version 1.0.0 is available and compatible with Pinia 3.x.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (6)
web/test/mocks/ui-components.ts (1)

3-10: Consider adding props to the BrandButton mock if needed for tests.

The mock for BrandButton provides a minimal implementation that will work for basic rendering tests. If your tests need to verify prop handling, you may need to expand this mock to handle props correctly.

web/test/mocks/services/request.ts (1)

3-15: Good mock implementation to prevent network requests.

This mock for the request service correctly prevents actual network requests during tests, which helps avoid flaky tests and external dependencies.

Consider making the response configurable for different test scenarios:

-      post: vi.fn(() => Promise.resolve({ data: {} })),
-      get: vi.fn(() => Promise.resolve({ data: {} })),
+      post: vi.fn().mockImplementation(() => Promise.resolve({ data: {} })),
+      get: vi.fn().mockImplementation(() => Promise.resolve({ data: {} })),

This would allow individual tests to override the response with something like:
useRequest().get.mockImplementationOnce(() => Promise.resolve({ data: { customData: true } }))

web/test/mocks/shared-callbacks.ts (1)

13-31: Consider aligning exported mock with the vi.mock implementation.

The exported mockSharedCallbacks object provides a more detailed implementation than the vi.mock above. This could potentially cause confusion or inconsistencies if both are used in different parts of the tests.

Consider either:

  1. Using only the vi.mock approach and removing the exported object
  2. Using the exported object within the vi.mock implementation
  3. Clearly documenting when each should be used
web/test/components/Auth.test.ts (1)

13-17: Consider removing the trivial test.

The first test doesn't provide any value as it only tests that the testing framework works.

describe('AuthComponent', () => {
-  it('runs a basic test', () => {
-    expect(true).toBe(true);
-  });
-
  it('can mount a mock component', () => {
web/test/mocks/ui-libraries.ts (2)

3-22: Simplified but functional clsx mock.

The mock implementation handles the core functionality of clsx (processing strings, arrays, and objects), but may not cover all edge cases of the actual library.

Consider adding a comment explaining the limitations of this mock compared to the real implementation:

// Mock clsx
+// This is a simplified implementation that may not handle all edge cases
vi.mock('clsx', () => {

24-37: Overly simplified tailwind-merge mock.

The current implementation doesn't handle the main purpose of tailwind-merge: resolving conflicts between Tailwind classes. For simple tests this might be sufficient, but it won't accurately represent behavior if tests depend on proper class conflict resolution.

// Mock tailwind-merge
+// NOTE: This is a very simplified mock that doesn't handle class conflicts
+// like the actual tailwind-merge library would
vi.mock('tailwind-merge', () => {
  const twMerge = (...classes: any[]) => {
-    return classes.filter(Boolean).join(' ');
+    // A slightly more realistic implementation would at least
+    // remove duplicate classes
+    return [...new Set(classes.flatMap(c => 
+      typeof c === 'string' ? c.split(' ') : []
+    ))].filter(Boolean).join(' ');
  };
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 398c247 and c51a0e2.

📒 Files selected for processing (17)
  • web/nuxt.config.ts (2 hunks)
  • web/package.json (5 hunks)
  • web/scripts/add-timestamp-webcomponent-manifest.js (0 hunks)
  • web/test/components/Auth.test.ts (1 hunks)
  • web/test/mocks/pinia.ts (1 hunks)
  • web/test/mocks/services/index.ts (1 hunks)
  • web/test/mocks/services/request.ts (1 hunks)
  • web/test/mocks/shared-callbacks.ts (1 hunks)
  • web/test/mocks/stores/errors.ts (1 hunks)
  • web/test/mocks/stores/index.ts (1 hunks)
  • web/test/mocks/stores/server.ts (1 hunks)
  • web/test/mocks/ui-components.ts (1 hunks)
  • web/test/mocks/ui-libraries.ts (1 hunks)
  • web/test/mocks/vue-i18n.ts (1 hunks)
  • web/test/mocks/vue.ts (1 hunks)
  • web/test/setup.ts (1 hunks)
  • web/vitest.config.mjs (1 hunks)
💤 Files with no reviewable changes (1)
  • web/scripts/add-timestamp-webcomponent-manifest.js
✅ Files skipped from review due to trivial changes (2)
  • web/test/mocks/services/index.ts
  • web/test/mocks/stores/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • web/nuxt.config.ts
  • web/test/setup.ts
  • web/package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (14)
web/test/mocks/stores/errors.ts (1)

1-13: Looks good, but verify the errors structure matches usage pattern.

The mock implementation for the errors store follows good mocking practices. However, there's one detail worth checking:

-    errors: { value: [] },
+    errors: [],

If the actual usage in components accesses errors directly (not as errors.value), you should change this to be a direct array. If components do access it as errors.value, then the current implementation is correct.

web/test/mocks/vue-i18n.ts (1)

3-24: Well-structured mock with focused translations.

This mock implementation for vue-i18n provides appropriate translations for the Auth component and handles the fallback case correctly. The structure with both useI18n and createI18n covers the composition API and options API approaches.

web/test/mocks/stores/server.ts (1)

1-16: Well-structured mock for the Auth component's server store.

This mock provides all necessary properties and methods needed by the Auth component for testing, including authentication state, tokens, and methods for authentication flow control.

web/vitest.config.mjs (1)

1-35:

✅ Verification successful

Comprehensive Vitest configuration for Vue testing.

The configuration covers all essential aspects for testing a Vue.js application, including:

  • Vue plugin integration
  • JSDOM test environment
  • Proper path resolution aliases
  • Test file patterns and timeouts
  • Mock reset/cleanup behavior

One note of caution: ensure that the ./test/setup.ts file referenced in setupFiles exists and is properly configured.


🏁 Script executed:

#!/bin/bash
# Check if the setup file exists
if [ -f "./web/test/setup.ts" ]; then
  echo "Setup file exists"
  cat "./web/test/setup.ts"
else
  echo "Setup file does not exist at ./web/test/setup.ts"
fi

Length of output: 1026


Verification Complete: All Configurations and Setup Files are Verified

The Vitest configuration in web/vitest.config.mjs is solid and effectively sets up the Vue testing environment. Key points include:

  • Integration of the Vue plugin and usage of the JSDOM environment.
  • Clearly defined path resolution aliases.
  • Comprehensive test file patterns and appropriate timeouts.
  • Proper mock behaviors and reset/cleanup configurations.
  • The referenced ./test/setup.ts file exists and includes robust global setups with mocks, testing utilities, and configuration updates.

No further changes are needed based on the current configuration.

web/test/mocks/pinia.ts (1)

1-29: Solid Pinia mock implementation.

This mock effectively handles Pinia store creation with proper error handling. The implementation:

  • Preserves the actual Pinia functionality where needed
  • Handles both function and object-based store definitions
  • Adds mock implementations for store methods like $reset and $patch
  • Provides a simplified storeToRefs implementation

One minor consideration: this implementation focuses on setup-style stores but may need expansion if the application uses the options API (state/getters/actions objects).

web/test/mocks/shared-callbacks.ts (2)

5-11: Simple and effective mock for @unraid/shared-callbacks.

The mock implementation provides basic functionality that returns the input data directly, which is appropriate for testing scenarios where actual encryption/decryption isn't needed.


33-43: Appropriate mock for the crypto-js/aes module.

This mock correctly simulates the crypto-js interface by providing objects with toString methods, matching the behavior of the actual implementation while avoiding actual cryptographic operations during tests.

web/test/components/Auth.test.ts (4)

1-11: Good setup with proper imports and a clean mock component.

The mock component is well-structured and correctly mimics the Auth component's interface with proper props and event handling.


18-28: Well-structured component mounting test.

This test properly verifies that the component can be mounted with the expected props and that the critical button element exists.


29-42: Effective conditional rendering test.

The test correctly validates that error messages appear when the appropriate state is provided, including verification of the content.


44-58: Correct event testing approach.

This test properly uses Vue Test Utils' emitted() method to verify event emissions, as suggested in a previous review.

web/test/mocks/vue.ts (3)

3-8: Good approach merging with actual Vue implementation.

Using vi.importActual to get the real Vue implementation and then selectively overriding functions is excellent practice.


8-19: Well-implemented ref and computed mocks with proper error handling.

The implementation properly handles edge cases like non-function computeds and errors within computed functions.


20-25: Limited reactive behavior mocking.

The current implementation has limitations for testing truly reactive behaviors:

  • reactive simply returns the object without making it reactive
  • watch is an empty mock that won't trigger on state changes
  • nextTick returns a resolved promise but doesn't actually wait for DOM updates

This may be sufficient for basic tests but could miss reactivity-related issues.

If you need to test reactive behaviors more thoroughly, consider enhancing these mocks to better simulate Vue's reactivity system.

@elibosley
Copy link
Member

This is getting to a really good place. I'd suggest getting these tests working, merging this, and then continuing down the testing path.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
web/test/mocks/utils/README.md (2)

1-3: Clear introduction to the utilities.

The introduction effectively establishes the purpose of these utilities for testing Vue components that use the composition API.

Consider adding a comma after "composition API" in line 3:

-This directory contains utilities to help test Vue components, particularly those that use the composition API which can be challenging to test directly.
+This directory contains utilities to help test Vue components, particularly those that use the composition API, which can be challenging to test directly.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~3-~3: Possible missing comma found.
Context: ...icularly those that use the composition API which can be challenging to test direct...

(AI_HYDRA_LEO_MISSING_COMMA)


5-12: Well-articulated challenges.

The challenges section clearly identifies the specific issues that make testing composition API components difficult.

Consider rewording line 11 to avoid the split infinitive:

-3. **TypeScript errors** - Attempting to directly test components that use the composition API can lead to TypeScript errors like "computed is not defined".
+3. **TypeScript errors** - Attempting to test components directly that use the composition API can lead to TypeScript errors like "computed is not defined".
🧰 Tools
🪛 LanguageTool

[style] ~11-~11: Style-wise, it’s not ideal to insert an adverb (‘directly’) in the middle of an infinitive construction (‘to test’). Try moving the adverb to avoid split infinitives.
Context: ...re injected. 3. TypeScript errors - Attempting to directly test components that use the composition API can lead to Typ...

(SPLIT_INFINITIVE)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between c51a0e2 and 288a3b7.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (7)
  • web/package.json (5 hunks)
  • web/test/components/DownloadApiLogs.test.ts (1 hunks)
  • web/test/components/KeyActions.test.ts (1 hunks)
  • web/test/mocks/utils/README.md (1 hunks)
  • web/test/mocks/utils/component-mock.ts (1 hunks)
  • web/test/mocks/utils/examples/KeyActions.test.example.ts (1 hunks)
  • web/test/mocks/utils/examples/key-actions-mock.ts (1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
web/test/mocks/utils/examples/KeyActions.test.example.ts (1)
web/test/mocks/utils/examples/key-actions-mock.ts (1)
  • createKeyActionsTest (17-45)
🪛 LanguageTool
web/test/mocks/utils/README.md

[uncategorized] ~3-~3: Possible missing comma found.
Context: ...icularly those that use the composition API which can be challenging to test direct...

(AI_HYDRA_LEO_MISSING_COMMA)


[style] ~11-~11: Style-wise, it’s not ideal to insert an adverb (‘directly’) in the middle of an infinitive construction (‘to test’). Try moving the adverb to avoid split infinitives.
Context: ...re injected. 3. TypeScript errors - Attempting to directly test components that use the composition API can lead to Typ...

(SPLIT_INFINITIVE)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (52)
web/package.json (6)

4-4: Good addition of module type declaration.

The "type": "module" specification correctly configures the project to use ES modules, which aligns with modern JavaScript practices.


34-35: Nice improvement to test scripts.

Separating vitest run (for running tests once) and vitest (for watch mode) enhances developer experience by providing clear commands for different testing scenarios.


47-53: Update @pinia/testing to the Latest Compatible Version

The testing dependencies overall are well-chosen for the Vue/Pinia ecosystem. However, note that @pinia/testing is set to version "^0.1.0", while the latest version compatible with Pinia 3.x is "^1.0.0". Please update this dependency accordingly.

What is the latest version of @pinia/testing compatible with Pinia 3.x?

57-63: Good selection of testing libraries.

The addition of Vue Test Utils, Testing Library, and happy-dom provides a robust foundation for component testing. The coverage reporter will help maintain good test coverage.


78-82: Updated vitest to latest minor version.

Keeping testing dependencies up-to-date is good practice for security and feature improvements.


128-133: Good use of pnpm patched dependencies.

Using patched dependencies to fix imports in @unraid/shared-callbacks is a clean solution without requiring changes to the original package.

web/test/mocks/utils/component-mock.ts (3)

1-9: Well-documented utilities for component mocking.

The documentation clearly explains the purpose and approach of these utilities - creating simplified component mocks that avoid the complexity of the composition API.


15-37: Excellent implementation of createMockComponent.

This function provides a clean way to create testable mock components by transforming props into component data. The implementation is concise and follows best practices.


39-101: Thorough implementation of list component factory.

The createListComponentMockFactory function offers a flexible solution for testing list components with filtering. The code handles edge cases well and provides good defaults.

web/test/mocks/utils/examples/key-actions-mock.ts (3)

1-6: Clear documentation for the example file.

The header comments effectively explain the purpose of this example and how it demonstrates the usage of the component-mock utility.


17-45: Well-structured test factory implementation.

The createKeyActionsTest factory provides a reusable pattern for testing the KeyActions component. The template function and key extractor are appropriate for this use case.


47-103: Comprehensive example usage with various test scenarios.

The example demonstrates different testing scenarios (store actions, custom actions, filtering, and layout options) that would be commonly needed when testing this component.

web/test/components/DownloadApiLogs.test.ts (7)

1-19: Excellent test documentation.

The file header clearly explains what aspects of the component are being tested and why the component is mocked. This documentation helps other developers understand the test's purpose.


26-29: Good use of test setup.

Setting up the CSRF token globally before each test ensures consistent test environments.


31-71: Effective component mocking approach.

The mock component strategy avoids dependency issues while still providing enough functionality to test the real component's behavior.


73-83: Good URL computation test.

This test ensures that the download URL is correctly constructed with the CSRF token.


84-99: Thorough button attribute testing.

The test verifies all important button attributes: href, download, and external flags.


101-122: Comprehensive link testing.

Testing each support link's attributes ensures they're correctly configured for external navigation.


124-147: Good content verification tests.

These tests ensure that the component displays the correct text content, which is important for user guidance.

web/test/mocks/utils/examples/KeyActions.test.example.ts (14)

1-6: Well-structured documentation header.

The header clearly explains the purpose of this example file, which demonstrates how to implement tests for the KeyActions component using the component-mock utility.


8-13: Clean imports organization.

Imports are well-organized with appropriate grouping: external libraries first, followed by types, and then local utilities.


16-42: Comprehensive test data setup.

The sample actions are well-defined with various combinations of properties (href, external, disabled, etc.) which will help test different rendering scenarios.


52-59: Good edge case testing.

Testing the component behavior when no actions are available ensures the component gracefully handles empty states.


61-69: Proper store integration test.

This test correctly verifies that the component will use actions from the store when no actions are provided as props, which is a critical functional requirement.


71-89: Props precedence testing.

Effectively validates that the component prioritizes actions passed via props over those from the store, ensuring proper data flow hierarchy.


91-101: Filter functionality testing.

The test correctly validates the component's ability to filter actions by name when the filterBy prop is provided.


103-113: Filter exclusion testing.

This test properly validates that the filterOut functionality works correctly to exclude specific actions by name.


115-123: UI styling verification.

Good test for conditional class application based on the maxWidth prop, ensuring responsive design behavior works as expected.


125-133: Style exclusion verification.

Complementary test to ensure maxWidth classes are not applied when the prop is false, completing the conditional styling test coverage.


135-158: Thorough attribute verification.

Excellent test that verifies all button attributes are correctly applied, including href, data-external, title, and disabled states.


160-175: Event handling verification.

This test effectively ensures that button click events correctly trigger the associated action's click handler.


177-186: Graceful undefined handling.

Good defensive test to ensure the component gracefully handles undefined filter values.


188-195: Default behavior verification.

Confirms that unfiltered actions are correctly returned when no filtering is specified.

web/test/components/KeyActions.test.ts (14)

1-33: Excellent test documentation.

The header comments provide a comprehensive explanation of test coverage and approach. This level of documentation significantly enhances maintainability and helps other developers understand the testing strategy.


35-41: Appropriate imports.

All necessary testing utilities and component dependencies are correctly imported.


43-69: Well-structured test data.

Sample actions are well-defined with various properties to test different rendering scenarios, consistent with the example file.


79-103: Elegant filter logic implementation.

The helper function correctly emulates the component's filtering logic with proper handling of actions, store actions, and filter options.


105-132: Clean mock component implementation.

The mock component template accurately replicates the original component's structure while simplifying the underlying logic for testing.


134-140: Empty state handling.

Good test for ensuring the component handles the case when no actions are available.


142-149: Store fallback testing.

Verifies that the component correctly uses actions from the store when no explicit actions are provided.


151-168: Props priority verification.

Confirms that explicitly provided actions take precedence over store actions.


170-179: Inclusion filtering test.

Properly verifies that actions are correctly filtered when using the filterBy prop.


181-190: Exclusion filtering test.

Validates the filterOut functionality works as expected.


192-208: Conditional styling tests.

The pair of tests thoroughly verify that maxWidth class is applied or not applied based on the prop value.


210-232: Attribute validation.

Comprehensive verification of all button attributes, ensuring correct rendering of href, data-external, title, and disabled states.


234-248: Click handler testing.

Effective testing of button click events to ensure they trigger the appropriate action handlers.


250-266: Edge case handling.

Good tests for handling undefined filters and verifying default behavior when no filters are specified.

web/test/mocks/utils/README.md (5)

13-21: Clear solution explanation.

The Component Mocking Pattern is well-explained with clear benefits listed, providing a strong rationale for the approach.


22-45: Well-documented utilities.

The function signatures for both utilities are clearly presented, making it easy for developers to understand their usage.


47-53: Helpful usage examples.

Pointing to concrete examples helps developers understand how to apply these utilities in practice.


54-62: Clear guidance on when to use this approach.

The specific scenarios where this approach is most beneficial are well-articulated.


63-69: Balanced implementation considerations.

The documentation acknowledges the trade-offs while explaining why the benefits outweigh them, providing a realistic view of the approach.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
web/test/mocks/apollo-client.ts (3)

11-28: Consider adding error simulation capability.

The createClient function provides a solid foundation for mocking Apollo responses, but it always returns successful responses. Consider adding an option to simulate GraphQL errors or network failures for more comprehensive testing.

interface TestApolloClientOptions {
  uri?: string;
  mockData?: Record<string, unknown>;
+ simulateError?: boolean;
+ errorMessage?: string;
}

function createClient(options: TestApolloClientOptions = {}) {
-  const { uri = 'http://localhost/graphql', mockData = { data: {} } } = options;
+  const { 
+    uri = 'http://localhost/graphql', 
+    mockData = { data: {} },
+    simulateError = false,
+    errorMessage = 'Network error'
+  } = options;

+  const mockFetch = () => {
+    if (simulateError) {
+      return Promise.reject(new Error(errorMessage));
+    }
+    return Promise.resolve(new Response(JSON.stringify(mockData)));
+  };

  return new ApolloClient({
    link: createHttpLink({
      uri,
      credentials: 'include',
-      fetch: () => Promise.resolve(new Response(JSON.stringify(mockData))),
+      fetch: mockFetch,
    }),

22-26: Consider making fetch policy configurable.

The current implementation sets fetchPolicy: 'no-cache' as a default option, which is good for isolated tests. However, for testing cache-related behaviors, it might be useful to make this configurable.

interface TestApolloClientOptions {
  uri?: string;
  mockData?: Record<string, unknown>;
+ fetchPolicy?: 'no-cache' | 'cache-first' | 'network-only' | 'cache-only' | 'cache-and-network';
}

function createClient(options: TestApolloClientOptions = {}) {
-  const { uri = 'http://localhost/graphql', mockData = { data: {} } } = options;
+  const { 
+    uri = 'http://localhost/graphql', 
+    mockData = { data: {} },
+    fetchPolicy = 'no-cache' 
+  } = options;

  // ...

  defaultOptions: {
    watchQuery: {
-      fetchPolicy: 'no-cache',
+      fetchPolicy,
    },
  },

19-19: Consider enhancing mock data flexibility.

The current mock fetch implementation doesn't differentiate between different queries or operations. For more sophisticated tests, consider implementing a mechanism to return different mock data based on the query or operation.

interface TestApolloClientOptions {
  uri?: string;
-  mockData?: Record<string, unknown>;
+  mockData?: Record<string, unknown> | ((operation: Operation) => Record<string, unknown>);
}

function createClient(options: TestApolloClientOptions = {}) {
  const { uri = 'http://localhost/graphql', mockData = { data: {} } } = options;

  return new ApolloClient({
    link: createHttpLink({
      uri,
      credentials: 'include',
-      fetch: () => Promise.resolve(new Response(JSON.stringify(mockData))),
+      fetch: (_, init) => {
+        const body = JSON.parse(init?.body as string || '{}');
+        const response = typeof mockData === 'function' 
+          ? mockData(body) 
+          : mockData;
+        return Promise.resolve(new Response(JSON.stringify(response)));
+      },
    }),
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 288a3b7 and f917a03.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (2)
  • web/package.json (5 hunks)
  • web/test/mocks/apollo-client.ts (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (14)
web/test/mocks/apollo-client.ts (3)

1-9: Good interface definition for test options.

The interface TestApolloClientOptions is well-structured with appropriate optional properties for URI and mock data. This provides good flexibility for tests.


30-38: Good implementation of default mock client.

The default mock client and the provider function are well-implemented. Returning the client from provideMockApolloClient() is helpful for further configuration in tests.


40-47: Well-designed utility for custom Apollo clients.

The createTestApolloClient function provides a clean way to create and configure custom Apollo clients for specific test scenarios. This will make test setup more maintainable.

web/package.json (11)

4-4: Specify ES Module Type

Adding "type": "module" ensures that Node.js interprets the files as ES modules, which is essential for modern JavaScript projects. Please verify that all file imports and configurations align with this change.


34-35: Refine Testing Script Commands

By changing the "test" script to "vitest run" and introducing "test:watch": "vitest", there's a clear separation between single-run testing (ideal for CI) and watch mode for development. Ensure these variations work seamlessly with your CI/CD pipelines.


48-48: Introduce Nuxt Testing Utilities

The addition of @nuxt/test-utils at version ^3.17.2 is a solid choice for enhancing Nuxt component tests. Confirm that this version meets your project’s requirements.


50-50: Update @pinia/testing Dependency

Updating @pinia/testing to ^1.0.0 aligns with the latest compatible version for Pinia 3.x. This improves testing consistency with the current Pinia API.


53-53: Add Vue Testing Library

Including @testing-library/vue at ^8.0.0 provides robust utilities for testing Vue components and aligns well with the overall testing framework.


59-59: Align Vitest Coverage Utilities

The update of @vitest/coverage-v8 to ^3.1.1 ensures that code coverage features are compatible with the updated vitest version, contributing to stable test reports.


61-61: Upgrade Vue Test Utils

The bump to @vue/test-utils version ^2.4.6 is beneficial for leveraging improved vue component testing capabilities.


66-66: Refresh Happy-DOM Version

Updating happy-dom to ^17.4.4 helps in providing a more accurate and performant DOM environment for tests.


79-79: Revise Vitest Dependency

Upgrading vitest to ^3.1.1 promises better performance and fixes over previous versions. This is key to maintaining robust and speedy tests.


128-128: Lock Package Manager Version

Specifying "packageManager": "pnpm@10.6.5" helps enforce consistent dependency handling across various development environments.


129-133: Configure PNPM Patched Dependencies

The added "pnpm" configuration block with "patchedDependencies" ensures that custom patches (such as the one for @unraid/shared-callbacks) are consistently applied. Double-check that the patch file path is correct and relative to the repository root for smooth operation.

@mdatelle mdatelle force-pushed the feat/add-front-end-unit-testing branch from f917a03 to a951a12 Compare April 3, 2025 15:04
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (8)
web/test/mocks/vue.ts (1)

1-26: Well-structured Vue composition API mocks

The mock implementation provides simplified versions of Vue composition API functions that maintain the same interface while being easier to test. The error handling in the computed function is particularly useful.

The error variable e on line 14 is captured but never used. While not critical for test code, consider either using it (e.g., for logging) or using _ to indicate it's intentionally unused:

- } catch (e) {
+ } catch (_) {
🧰 Tools
🪛 GitHub Check: Build Web App

[failure] 14-14:
'e' is defined but never used

web/test/mocks/utils/component-mock.ts (2)

13-13: Remove unused import

The ComponentMountingOptions type is imported but never used in the file.

-import type { ComponentMountingOptions } from '@vue/test-utils';
🧰 Tools
🪛 GitHub Check: Build Web App

[failure] 13-13:
'ComponentMountingOptions' is defined but never used. Allowed unused vars must match /^_/u


21-21: Replace any with more specific types

Using any reduces type safety and nullifies TypeScript's benefits. Consider using more specific types or a generic constraint.

-export function createMockComponent<Props extends Record<string, any>, Data extends Record<string, any>>(
+export function createMockComponent<Props extends Record<string, unknown>, Data extends Record<string, unknown>>(
🧰 Tools
🪛 GitHub Check: Build Web App

[failure] 21-21:
Unexpected any. Specify a different type


[failure] 21-21:
Unexpected any. Specify a different type

web/test/mocks/ui-libraries.ts (2)

5-19: Improve type safety in clsx mock

The function uses any type for arguments, which is flagged by static analysis.

Consider using more specific types to enhance type safety:

-  const clsx = (...args: any[]) => {
+  const clsx = (...args: (string | Record<string, boolean> | (string | false | null | undefined)[])[]): string => {

This better represents the expected types for the clsx function.

🧰 Tools
🪛 GitHub Check: Build Web App

[failure] 5-5:
Unexpected any. Specify a different type


26-36: Improve type safety in tailwind-merge mock

Similar to clsx, this function also uses any type, which is flagged by static analysis.

Consider using more specific types:

-  const twMerge = (...classes: any[]) => {
+  const twMerge = (...classes: (string | undefined | null | boolean)[]) => {

This better represents the expected input types for the twMerge function.

🧰 Tools
🪛 GitHub Check: Build Web App

[failure] 26-26:
Unexpected any. Specify a different type

web/test/mocks/utils/README.md (3)

3-3: Missing comma in sentence

There should be a comma after "composition API" since it introduces a relative clause.

-This directory contains utilities to help test Vue components, particularly those that use the composition API which can be challenging to test directly.
+This directory contains utilities to help test Vue components, particularly those that use the composition API, which can be challenging to test directly.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~3-~3: Possible missing comma found.
Context: ...icularly those that use the composition API which can be challenging to test direct...

(AI_HYDRA_LEO_MISSING_COMMA)


11-11: Split infinitive in documentation

Consider restructuring to avoid the split infinitive "to directly test."

-3. **TypeScript errors** - Attempting to directly test components that use the composition API can lead to TypeScript errors like "computed is not defined".
+3. **TypeScript errors** - Attempting to test components directly that use the composition API can lead to TypeScript errors like "computed is not defined".
🧰 Tools
🪛 LanguageTool

[style] ~11-~11: Style-wise, it’s not ideal to insert an adverb (‘directly’) in the middle of an infinitive construction (‘to test’). Try moving the adverb to avoid split infinitives.
Context: ...re injected. 3. TypeScript errors - Attempting to directly test components that use the composition API can lead to Typ...

(SPLIT_INFINITIVE)


47-53: Practical usage examples referenced

Referencing specific example files helps developers understand how to implement these utilities in their own tests. Consider adding a brief code snippet directly in the README for quick reference.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between f917a03 and a951a12.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (26)
  • web/components/Auth.ce.vue (1 hunks)
  • web/helpers/markdown.ts (1 hunks)
  • web/nuxt.config.ts (2 hunks)
  • web/package.json (3 hunks)
  • web/scripts/add-timestamp-webcomponent-manifest.js (0 hunks)
  • web/test/components/Auth.test.ts (1 hunks)
  • web/test/components/DownloadApiLogs.test.ts (1 hunks)
  • web/test/components/KeyActions.test.ts (1 hunks)
  • web/test/mocks/apollo-client.ts (1 hunks)
  • web/test/mocks/pinia.ts (1 hunks)
  • web/test/mocks/services/index.ts (1 hunks)
  • web/test/mocks/services/request.ts (1 hunks)
  • web/test/mocks/shared-callbacks.ts (1 hunks)
  • web/test/mocks/stores/errors.ts (1 hunks)
  • web/test/mocks/stores/index.ts (1 hunks)
  • web/test/mocks/stores/server.ts (1 hunks)
  • web/test/mocks/ui-components.ts (1 hunks)
  • web/test/mocks/ui-libraries.ts (1 hunks)
  • web/test/mocks/utils/README.md (1 hunks)
  • web/test/mocks/utils/component-mock.ts (1 hunks)
  • web/test/mocks/utils/examples/KeyActions.test.example.ts (1 hunks)
  • web/test/mocks/utils/examples/key-actions-mock.ts (1 hunks)
  • web/test/mocks/vue-i18n.ts (1 hunks)
  • web/test/mocks/vue.ts (1 hunks)
  • web/test/setup.ts (1 hunks)
  • web/vitest.config.mjs (1 hunks)
💤 Files with no reviewable changes (1)
  • web/scripts/add-timestamp-webcomponent-manifest.js
🚧 Files skipped from review as they are similar to previous changes (15)
  • web/test/mocks/services/index.ts
  • web/components/Auth.ce.vue
  • web/test/mocks/stores/index.ts
  • web/nuxt.config.ts
  • web/test/mocks/ui-components.ts
  • web/test/mocks/services/request.ts
  • web/test/setup.ts
  • web/test/mocks/vue-i18n.ts
  • web/test/mocks/stores/errors.ts
  • web/test/mocks/apollo-client.ts
  • web/test/mocks/stores/server.ts
  • web/test/mocks/pinia.ts
  • web/helpers/markdown.ts
  • web/test/components/KeyActions.test.ts
  • web/test/mocks/shared-callbacks.ts
🧰 Additional context used
🧬 Code Definitions (2)
web/test/mocks/utils/examples/KeyActions.test.example.ts (1)
web/test/mocks/utils/examples/key-actions-mock.ts (1)
  • createKeyActionsTest (17-45)
web/test/mocks/utils/examples/key-actions-mock.ts (1)
web/test/mocks/utils/component-mock.ts (1)
  • createListComponentMockFactory (46-101)
🪛 GitHub Check: Build Web App
web/test/mocks/utils/component-mock.ts

[failure] 13-13:
'ComponentMountingOptions' is defined but never used. Allowed unused vars must match /^_/u


[failure] 21-21:
Unexpected any. Specify a different type


[failure] 21-21:
Unexpected any. Specify a different type

web/test/components/DownloadApiLogs.test.ts

[failure] 24-24:
'vi' is defined but never used. Allowed unused vars must match /^_/u

web/test/mocks/ui-libraries.ts

[failure] 5-5:
Unexpected any. Specify a different type


[failure] 26-26:
Unexpected any. Specify a different type

web/test/mocks/utils/examples/key-actions-mock.ts

[failure] 27-27:
'filteredItems' is defined but never used. Allowed unused args must match /^_/u


[failure] 27-27:
'maxWidth' is defined but never used. Allowed unused args must match /^_/u


[failure] 27-27:
't' is defined but never used. Allowed unused args must match /^_/u

web/test/mocks/vue.ts

[failure] 14-14:
'e' is defined but never used

🪛 LanguageTool
web/test/mocks/utils/README.md

[uncategorized] ~3-~3: Possible missing comma found.
Context: ...icularly those that use the composition API which can be challenging to test direct...

(AI_HYDRA_LEO_MISSING_COMMA)


[style] ~11-~11: Style-wise, it’s not ideal to insert an adverb (‘directly’) in the middle of an infinitive construction (‘to test’). Try moving the adverb to avoid split infinitives.
Context: ...re injected. 3. TypeScript errors - Attempting to directly test components that use the composition API can lead to Typ...

(SPLIT_INFINITIVE)

🪛 GitHub Actions: CI - Main (API)
web/test/components/DownloadApiLogs.test.ts

[error] 24-24: 'vi' is defined but never used. Allowed unused vars must match /^_/u @typescript-eslint/no-unused-vars

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (22)
web/vitest.config.mjs (1)

1-35: Solid Vitest configuration with good test separation

This configuration establishes a robust testing environment for Vue.js applications. The setup properly configures:

  • JSDOM environment for DOM simulation
  • Server dependency handling
  • Global test options including mock reset/restore
  • Path aliases for consistent imports
web/package.json (2)

33-34: Good separation of test commands

The updated test commands properly distinguish between:

  • test: single run mode with vitest run
  • test:watch: watch mode with vitest

This gives developers flexibility for both CI environments and local development.


47-49: Complete testing ecosystem with appropriate dependencies

The added testing libraries form a comprehensive ecosystem for Vue application testing:

  • @vue/test-utils: Core Vue component testing
  • @pinia/testing: State management testing
  • @testing-library/vue: User-centric testing
  • @vitest/coverage-v8: Code coverage reporting
  • happy-dom: Alternative to JSDOM for testing

Also applies to: 52-52, 58-58, 60-60, 65-65, 78-78

web/test/mocks/utils/component-mock.ts (1)

39-101: Well-designed factory for list component testing

The createListComponentMockFactory function provides an elegant solution for testing list components with filtering. The implementation:

  • Handles multiple filtering strategies
  • Provides sensible defaults (like translation function)
  • Properly emulates component rendering logic
web/test/mocks/utils/examples/key-actions-mock.ts (1)

50-102: Well-constructed example function

This example function effectively demonstrates the different ways to use the factory, covering key test scenarios:

  • Testing with store actions
  • Testing with custom actions
  • Testing with filtering
  • Testing with the maxWidth option
web/test/components/Auth.test.ts (2)

6-11: Good approach with mock component

Creating a direct mock component is an effective strategy for this test as it isolates the component behavior without dependencies.


44-58: Proper event testing implementation

The event testing approach correctly uses Vue Test Utils' built-in capabilities to verify emitted events, which is the recommended practice.

web/test/components/DownloadApiLogs.test.ts (2)

74-82: Well-structured URL testing

This test effectively validates the URL computation with the CSRF token, which is a critical security feature.


101-122: Comprehensive link attribute testing

The test checks not only the href URLs but also the target and rel attributes, which is important for security when opening external links.

web/test/mocks/utils/examples/KeyActions.test.example.ts (8)

1-6: Clear and comprehensive documentation

The documentation header clearly explains the purpose of this example file and provides context about its relationship with the component mock utility. This is excellent practice for maintainability.


16-42: Well-structured test data with comprehensive scenarios

The sample actions array provides a good variety of test scenarios covering different action types, properties, and configurations. This enables thorough testing of the component's handling of various action structures.


44-45: Simple and effective mock translation function

The translation mock effectively serves its purpose without unnecessary complexity. Consider adding this pattern to your README as a best practice example for mocking i18n functions.


52-59: Good edge case handling

Testing the component's behavior when no actions are available is important for ensuring graceful handling of edge cases.


91-101: Comprehensive filtering tests

The tests for both filterBy and filterOut functionality are thorough and validate the component's filtering capabilities correctly. These tests ensure the component's filtering logic works as expected in different scenarios.

Also applies to: 103-113


135-158: Detailed attribute verification

The test provides detailed verification of button attributes, which is crucial for ensuring proper rendering and functionality. This test case is particularly valuable as it verifies multiple aspects of the component's DOM structure.


160-175: Proper event handling verification

The test correctly validates that click events trigger the appropriate action handlers. This is essential for ensuring the component's interactive functionality works as expected.


177-186: Edge case handling for filters

These tests appropriately verify the component's behavior with undefined filters and when no filters are provided. Handling these edge cases ensures the component is robust and predictable.

Also applies to: 188-195

web/test/mocks/utils/README.md (5)

1-3: Clear introduction to the testing utilities

The introduction effectively communicates the purpose of these utilities and addresses a common challenge in Vue development.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~3-~3: Possible missing comma found.
Context: ...icularly those that use the composition API which can be challenging to test direct...

(AI_HYDRA_LEO_MISSING_COMMA)


13-21: Clear explanation of the solution approach

The explanation of the Component Mocking Pattern is concise and effectively communicates the benefits of this approach. The numbered points make it easy to understand the advantages.


28-30: Helpful function signature documentation

The TypeScript function signature for createMockComponent clearly shows the generic types and parameters, which is valuable for developers using this utility.


36-45: Comprehensive documentation for list component factory

The function signature for createListComponentMockFactory is well-documented with appropriate type parameters and detailed parameter structure.


63-65: Transparent about implementation tradeoffs

Acknowledging the tradeoff of keeping mock implementations in sync with actual components shows transparency and helps developers make informed decisions about using this approach.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
web/test/mocks/utils/component-mock.ts (3)

37-43: Documentation parameter mismatch.

The JSDoc mentions @param itemType, but there's no corresponding parameter in the function signature. Consider updating the documentation to match the actual parameters.

 /**
  * Creates a mock component factory specialized for list-rendering components with filtering
  *
- * @param itemType Type of items being rendered in the list
  * @param templateFn Function that generates the template based on the passed options
+ * @param getItemKey Function that extracts a unique key from each item
  * @returns A factory function that creates test components
  */

67-80: Consider clarifying the filtering logic priority.

The current implementation prioritizes filterOut over filterBy when both are provided. This might be intentional, but it's worth documenting this behavior to avoid confusion.

Also, the return condition return filterOut ? !filterOut.includes(key) : filterBy?.includes(key); could potentially return undefined if filterBy is defined but empty and filterOut is undefined. Consider providing a default value.

- return filterOut ? !filterOut.includes(key) : filterBy?.includes(key);
+ // If filterOut is provided, exclude items with matching keys
+ // Otherwise, if filterBy is provided, include only items with matching keys
+ if (filterOut) {
+   return !filterOut.includes(key);
+ } else if (filterBy) {
+   return filterBy.includes(key);
+ }
+ return true; // Include all items if no filters are provided

69-73: Handle undefined values more explicitly.

The current implementation returns allItems which could be undefined if both items and storeItems are undefined. Consider returning an empty array instead to avoid potential undefined errors.

- const allItems = items || storeItems;
+ const allItems = items || storeItems || [];

  if (!allItems || (!filterBy && !filterOut)) {
-   return allItems;
+   return allItems;  // This now guarantees an array return type
  }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between a951a12 and 1895a49.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (7)
  • web/eslint.config.mjs (1 hunks)
  • web/package.json (3 hunks)
  • web/test/components/DownloadApiLogs.test.ts (1 hunks)
  • web/test/mocks/ui-libraries.ts (1 hunks)
  • web/test/mocks/utils/component-mock.ts (1 hunks)
  • web/test/mocks/utils/examples/key-actions-mock.ts (1 hunks)
  • web/test/mocks/vue.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • web/test/components/DownloadApiLogs.test.ts
  • web/test/mocks/utils/examples/key-actions-mock.ts
  • web/test/mocks/ui-libraries.ts
  • web/test/mocks/vue.ts
  • web/package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
web/eslint.config.mjs (3)

2-4: Consistent import statement formatting.

The addition of semicolons to import statements improves code consistency in the file.


8-8: Good practice to ignore coverage directory.

Adding ignores: ['./coverage/**'] is a good practice as:

  1. Coverage files are auto-generated and shouldn't be linted
  2. It improves linting performance by skipping unnecessary files
  3. It prevents false positives from generated code

This change aligns well with the PR objective of setting up testing infrastructure.


15-16: Improved code formatting.

Removing the line break before the closing parenthesis improves code readability and maintains consistent style.

web/test/mocks/utils/component-mock.ts (3)

1-9: Good documentation header!

The file header clearly explains the purpose and approach of the utility. This is particularly helpful for developers who will use these mocking utilities in their tests.


13-35: Well-implemented utility function with good type safety.

The createMockComponent function is well-designed and properly typed with TypeScript generics. It provides a clean way to create mock components without dealing with the complexities of the composition API.


82-98: LGTM: Component mounting implementation.

The component creation and mounting logic is well-implemented, with appropriate data initialization and template handling.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
web/components/DummyServerSwitcher.vue (1)

9-10: Consider using a more specific type instead of any

The static analysis tool flagged the use of any in the AcceptableValue type definition. While this matches the type from reka-ui, consider using a more specific type for better type safety:

- type AcceptableValue = string | number | Record<string, any> | null;
+ type AcceptableValue = string | number | Record<string, unknown> | null;

This change maintains compatibility while improving type safety.

🧰 Tools
🪛 GitHub Check: Build Web App

[failure] 10-10:
Unexpected any. Specify a different type

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 2fff8f0 and 5da974c.

📒 Files selected for processing (2)
  • web/components/DummyServerSwitcher.vue (1 hunks)
  • web/components/Logs/SingleLogViewer.vue (16 hunks)
🧰 Additional context used
🪛 GitHub Check: Build Web App
web/components/DummyServerSwitcher.vue

[failure] 10-10:
Unexpected any. Specify a different type

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build API
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (14)
web/components/DummyServerSwitcher.vue (2)

15-19: Good type safety improvement

The update to the updateSelector function is well implemented. The function now:

  1. Accepts a broader range of input types via AcceptableValue
  2. Performs a runtime type check to ensure only string values are assigned
  3. Uses proper type assertion when setting the value

This approach improves type safety while maintaining flexibility.


2-7: Good organization of imports

The separation of type imports from value imports is a good TypeScript practice that improves code clarity and readability. The addition of storeToRefs from pinia is also appropriate for maintaining reactivity when destructuring store properties.

web/components/Logs/SingleLogViewer.vue (12)

3-3: Improved import organization.

The reordering of imports from @vue/apollo-composable enhances code readability.


6-7: Clean import organization for UI components.

The reorganization of imports with proper spacing and grouping improves code readability.


9-9: Good switch to isomorphic-dompurify.

Using isomorphic-dompurify is a better approach as it works consistently in both browser and Node.js environments.


13-23: Well-organized language imports with descriptive comment.

The reorganization of language imports with the descriptive comment makes the code more maintainable.


111-111: Type safety improvement for MutationObserver.

The type casting as unknown as Node ensures proper typing for the MutationObserver, avoiding TypeScript errors.


172-172: Consistent use of nextTick with better timeout handling.

The consistent application of nextTick with proper arrow functions and the addition of a timeout for setting canLoadMore improves the component's behavior when loading content.

Also applies to: 175-180


218-242: Enhanced log line highlighting with consistent formatting.

The consistent use of arrow functions with parentheses improves code readability while maintaining the same highlighting functionality.


258-259: Simplified log content computation.

The computed property now uses a more direct approach to combine chunks and highlight them.


363-375: Improved template readability with proper attribute separation.

Breaking out attributes into separate lines improves the readability of complex UI elements.


384-395: Enhanced button structure and accessibility.

The button structure is improved with proper attribute separation and better accessibility attributes.


403-436: Better loading state presentation.

The loading and error states have improved markup structure with proper spacing and attribute organization.


438-442: Improved code presentation structure.

The pre element's attributes are better organized for readability while maintaining the same functionality.

@mdatelle mdatelle marked this pull request as ready for review April 3, 2025 16:50
}
});
observer.observe(scrollViewportRef.value, { childList: true, subtree: true });
observer.observe(scrollViewportRef.value as unknown as Node, { childList: true, subtree: true });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be needed, not sure why this broke for you

@@ -0,0 +1,35 @@
import { fileURLToPath } from 'node:url';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't realize you reset this from the nuxt one, surprised this works


// Properly handle ES modules in testing and build environments
build: {
transpile: [/node_modules\/.*\.mjs$/, 'clsx', 'tailwind-merge'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wild, nice find

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm confused about what this is testing--it basically looks like it's testing whether 1 === 1 here (i.e. i can't figure out when these tests would ever fail). i'm not certain these tests actually improve confidence in the code's robustness.

Copy link
Contributor Author

@mdatelle mdatelle Apr 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are just a trial run to make sure the testing environment was working correctly and to see how Claude handled these tests on auto drive.

I just updated Auth.test.ts with the actual component so we can make sure this setup works with the components. Next PR will have the first batch of tests this one was getting too big.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo, using the actual component vs. a mock would be more useful. it'd be flakier, but then at least the correctness of the tests isn't dependent on the correctness of a mock.

imo mocks are only ever useful when testing higher level logic, where the shape/definition of the mock is arbitrary and variable (i.e where the mock is a stand-in for x). i'm not sure that creating test suites around a specific mock implementation is useful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to clarify, i think where we get into trouble is when mock-specific expectations are encoded into tests (as opposed to feature or logic-specific). cuz then tests are very at-risk for:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above for the Auth.test.ts example, next PR I'll fix these and have more tests. I'll be doing these in batches.

Comment on lines +4 to +5
vi.mock('clsx', () => {
const clsx = (...args: unknown[]) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we have to mock clsx & tailwind-merge? aren't they side-effect free?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to fix so many errors to get all this working this is what worked and was suggested by Claude.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe these can be deleted now after I added the .mjs regex in the config for the inline deps errors.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
web/test/components/Auth.test.ts (1)

7-7: ⚠️ Potential issue

Fix the unused import warning.

The useServerStore import is flagged by static analysis as unused. This is causing pipeline failures even though it's technically referenced in the mock definition.

Fix this by prefixing the import with an underscore to match the allowed pattern for unused vars:

-import { useServerStore } from '~/store/server';
+import { useServerStore as _useServerStore } from '~/store/server';

Then update the mock implementation:

vi.mock('~/store/server', () => ({
-  useServerStore: vi.fn(),
+  useServerStore: vi.fn(),
}));
🧹 Nitpick comments (1)
web/test/components/Auth.test.ts (1)

99-124: Test click handler through UI interaction.

The current test verifies the click handler exists, but doesn't test the actual UI interaction. This makes the test less effective at catching integration issues.

it('provides a click handler in authAction', async () => {
  const mockClick = vi.fn();

  // Mock store values
  const mockAuthAction = ref({
    text: 'Authenticate',
    icon: 'key',
    click: mockClick,
  });
  const mockStateData = ref({ error: false, message: '', heading: '' });

  // Create mock store with required Pinia properties
  const mockStore = createMockStore({
    authAction: mockAuthAction,
    stateData: mockStateData,
  });

  vi.mocked(useServerStore).mockReturnValue(mockStore);

+ const wrapper = mount(Auth, {
+   global: {
+     stubs: {
+       BrandButton: {
+         template: '<button class="brand-button-stub">{{ text }}</button>',
+         props: ['size', 'text', 'icon', 'title'],
+       },
+     },
+   },
+ });
+
+ // Test UI interaction
+ const button = wrapper.find('.brand-button-stub');
+ await button.trigger('click');
+ expect(mockClick).toHaveBeenCalled();

-  expect(mockAuthAction.value.click).toBeDefined();
-  expect(typeof mockAuthAction.value.click).toBe('function');
-
-  mockAuthAction.value.click();
-
-  expect(mockClick).toHaveBeenCalled();
});
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between b7b94de and c2f94c1.

📒 Files selected for processing (2)
  • web/nuxt.config.ts (2 hunks)
  • web/test/components/Auth.test.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • web/nuxt.config.ts
🧰 Additional context used
🪛 GitHub Check: Build Web App
web/test/components/Auth.test.ts

[failure] 22-22:
Unexpected any. Specify a different type


[failure] 33-33:
Unexpected any. Specify a different type

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build API
  • GitHub Check: Cloudflare Pages

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
web/test/components/Auth.test.ts (2)

7-7: ⚠️ Potential issue

Fix potential unused import warning

The useServerStore import is defined but isn't directly used in your code - it's only referenced in the mock. This might trigger linting errors similar to what was already fixed in previous reviews.

Apply this fix:

-import { useServerStore } from '~/store/server';
+import { useServerStore as _useServerStore } from '~/store/server';

Then update the mock implementation:

 vi.mock('~/store/server', () => ({
-  useServerStore: vi.fn(),
+  useServerStore: vi.fn(),
 }));

22-34: ⚠️ Potential issue

Remove any type usage

Using any reduces type safety and has been flagged by static analysis. This issue was previously identified in other files.

Replace with a more specific type:

- function createMockStore(storeProps: Record<string, any>) {
+ function createMockStore<T extends Record<string, unknown>>(storeProps: T) {
  return {
    ...storeProps,
    $id: 'server',
    $state: storeProps,
    $patch: vi.fn(),
    $reset: vi.fn(),
    $dispose: vi.fn(),
    $subscribe: vi.fn(),
    $onAction: vi.fn(),
    $unsubscribe: vi.fn(),
-  };
+  };
}
🧰 Tools
🪛 GitHub Check: Build Web App

[failure] 22-22:
Unexpected any. Specify a different type

🧹 Nitpick comments (2)
web/test/components/Auth.test.ts (2)

101-126: Consider testing the button click event directly

The current test confirms the click handler exists and functions, but it doesn't verify the actual button click interaction, which would better test the component's real-world behavior.

Consider adding a direct click test:

it('calls click handler when button is clicked', async () => {
  const mockClick = vi.fn();
  
  // Mock store values
  const mockAuthAction = ref({
    text: 'Authenticate',
    icon: 'key',
    click: mockClick,
  });
  const mockStateData = ref({ error: false, message: '', heading: '' });
  
  // Create mock store
  const mockStore = createMockStore({
    authAction: mockAuthAction,
    stateData: mockStateData,
  });
  
  vi.mocked(useServerStore).mockReturnValue(mockStore);
  
  const wrapper = mount(Auth, {
    global: {
      stubs: {
        BrandButton: {
          template: '<button class="brand-button-stub">{{ text }}</button>',
          props: ['size', 'text', 'icon', 'title'],
        },
      },
    },
  });
  
  // Find and click the button
  const button = wrapper.find('.brand-button-stub');
  await button.trigger('click');
  
  // Verify click handler was called
  expect(mockClick).toHaveBeenCalled();
});

36-39: Consider adding global test configuration

You're setting up similar stubs and mocks for each test. Consider moving common setup to the beforeEach block for improved maintainability.

 describe('Auth Component', () => {
+  // Common mock values
+  let mockAuthAction;
+  let mockStateData;
+  let mockStore;
+
   beforeEach(() => {
     vi.clearAllMocks();
+    
+    // Initialize common mocks
+    mockAuthAction = ref({
+      text: 'Authenticate',
+      icon: 'key',
+      click: vi.fn(),
+    });
+    mockStateData = ref({ error: false, message: '', heading: '' });
+    
+    // Create mock store with required Pinia properties
+    mockStore = createMockStore({
+      authAction: mockAuthAction,
+      stateData: mockStateData,
+    });
+    
+    vi.mocked(useServerStore).mockReturnValue(mockStore);
   });

Then in each test, you can modify only the specific properties needed for that test.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between c2f94c1 and 14dcd94.

📒 Files selected for processing (1)
  • web/test/components/Auth.test.ts (1 hunks)
🧰 Additional context used
🪛 GitHub Check: Build Web App
web/test/components/Auth.test.ts

[failure] 22-22:
Unexpected any. Specify a different type

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Build API
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
web/test/components/Auth.test.ts (1)

73-99: Improve the skipped test for error message rendering

The test title suggests checking for an error message, but it only verifies the component exists without asserting the presence of error content.

When you're ready to unskip this test, consider adding assertions:

  expect(wrapper.exists()).toBe(true);
+ // Check if error heading and message are displayed
+ expect(wrapper.text()).toContain('Error Occurred');
+ expect(wrapper.text()).toContain('Authentication failed');

Also, you might want to add the same stubbing configuration as in the first test to ensure consistent rendering behavior.

@elibosley elibosley force-pushed the feat/add-front-end-unit-testing branch from 1730921 to 3f89380 Compare April 3, 2025 19:31
@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2025

This plugin has been deployed to Cloudflare R2 and is available for testing.
Download it at this URL:

https://preview.dl.unraid.net/unraid-api/tag/1309/dynamix.unraid.net.plg

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
web/test/mocks/utils/README.md (1)

5-12: Challenge Section Improvement
The "Challenge" section is well-structured and outlines the key issues. However, in line 11, consider rephrasing to avoid the split infinitive. For example, "Attempting to test components that use the composition API directly can lead to TypeScript errors like 'computed is not defined'" would improve clarity.

🧰 Tools
🪛 LanguageTool

[style] ~11-~11: Style-wise, it’s not ideal to insert an adverb (‘directly’) in the middle of an infinitive construction (‘to test’). Try moving the adverb to avoid split infinitives.
Context: ...re injected. 3. TypeScript errors - Attempting to directly test components that use the composition API can lead to Typ...

(SPLIT_INFINITIVE)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 1730921 and 3f89380.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (29)
  • web/components/Auth.ce.vue (1 hunks)
  • web/components/DummyServerSwitcher.vue (1 hunks)
  • web/components/Logs/SingleLogViewer.vue (16 hunks)
  • web/eslint.config.mjs (1 hunks)
  • web/helpers/markdown.ts (1 hunks)
  • web/nuxt.config.ts (2 hunks)
  • web/package.json (3 hunks)
  • web/scripts/add-timestamp-webcomponent-manifest.js (0 hunks)
  • web/test/components/Auth.test.ts (1 hunks)
  • web/test/components/DownloadApiLogs.test.ts (1 hunks)
  • web/test/components/KeyActions.test.ts (1 hunks)
  • web/test/mocks/apollo-client.ts (1 hunks)
  • web/test/mocks/pinia.ts (1 hunks)
  • web/test/mocks/services/index.ts (1 hunks)
  • web/test/mocks/services/request.ts (1 hunks)
  • web/test/mocks/shared-callbacks.ts (1 hunks)
  • web/test/mocks/stores/errors.ts (1 hunks)
  • web/test/mocks/stores/index.ts (1 hunks)
  • web/test/mocks/stores/server.ts (1 hunks)
  • web/test/mocks/ui-components.ts (1 hunks)
  • web/test/mocks/ui-libraries.ts (1 hunks)
  • web/test/mocks/utils/README.md (1 hunks)
  • web/test/mocks/utils/component-mock.ts (1 hunks)
  • web/test/mocks/utils/examples/KeyActions.test.example.ts (1 hunks)
  • web/test/mocks/utils/examples/key-actions-mock.ts (1 hunks)
  • web/test/mocks/vue-i18n.ts (1 hunks)
  • web/test/mocks/vue.ts (1 hunks)
  • web/test/setup.ts (1 hunks)
  • web/vitest.config.mjs (1 hunks)
💤 Files with no reviewable changes (1)
  • web/scripts/add-timestamp-webcomponent-manifest.js
🚧 Files skipped from review as they are similar to previous changes (23)
  • web/test/mocks/services/index.ts
  • web/test/mocks/stores/index.ts
  • web/nuxt.config.ts
  • web/test/mocks/services/request.ts
  • web/vitest.config.mjs
  • web/test/mocks/stores/server.ts
  • web/components/Auth.ce.vue
  • web/helpers/markdown.ts
  • web/test/mocks/stores/errors.ts
  • web/test/mocks/apollo-client.ts
  • web/test/mocks/ui-libraries.ts
  • web/test/mocks/shared-callbacks.ts
  • web/test/mocks/utils/examples/KeyActions.test.example.ts
  • web/test/components/DownloadApiLogs.test.ts
  • web/test/mocks/vue.ts
  • web/components/DummyServerSwitcher.vue
  • web/eslint.config.mjs
  • web/test/setup.ts
  • web/test/mocks/ui-components.ts
  • web/test/mocks/vue-i18n.ts
  • web/components/Logs/SingleLogViewer.vue
  • web/test/mocks/pinia.ts
  • web/package.json
🧰 Additional context used
🧬 Code Definitions (1)
web/test/mocks/utils/examples/key-actions-mock.ts (1)
web/test/mocks/utils/component-mock.ts (1)
  • createListComponentMockFactory (44-99)
🪛 LanguageTool
web/test/mocks/utils/README.md

[style] ~11-~11: Style-wise, it’s not ideal to insert an adverb (‘directly’) in the middle of an infinitive construction (‘to test’). Try moving the adverb to avoid split infinitives.
Context: ...re injected. 3. TypeScript errors - Attempting to directly test components that use the composition API can lead to Typ...

(SPLIT_INFINITIVE)

⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Cloudflare Pages
🔇 Additional comments (23)
web/test/mocks/utils/examples/key-actions-mock.ts (3)

1-6: Well-documented purpose and implementation.

The file starts with a clear explanation of its purpose and how it serves as an example for using the component-mock utility. This documentation provides valuable context for developers.


17-45: Good implementation of the component mock factory.

The factory pattern here creates a well-structured, reusable test component. The use of _options in the template function correctly addresses potential unused parameter warnings that could occur with destructuring.


47-103: Comprehensive example function with clear test scenarios.

The example function demonstrates multiple test cases including:

  • Testing with store actions
  • Testing with custom actions
  • Testing with filtering
  • Testing with layout options

Each scenario is well-commented, making it easy to understand how to use the factory in various test contexts.

web/test/components/Auth.test.ts (4)

11-19: Effective mocking of dependencies.

The mocks for vue-i18n and the server store are properly implemented, focusing on the minimal functionality needed for testing.


25-38: Well-structured mock store creation.

The helper function creates a mock store with all required Pinia properties, using appropriate type assertions with unknown instead of any. This follows TypeScript best practices.


77-103: Good use of test skipping with explanation.

The test for error message rendering is appropriately skipped with a clear explanation about the current testing limitations. This is better than removing the test or leaving it failing without context.


105-130: Clear verification of click handler functionality.

The test validates that the click handler is properly defined, is a function, and can be called. This covers the expected behavior thoroughly.

web/test/components/KeyActions.test.ts (5)

1-33: Excellent test documentation.

The file begins with comprehensive documentation that outlines:

  1. What aspects of the component are being tested
  2. The testing approach
  3. Rationale for the chosen testing strategy

This level of detail significantly improves the maintainability of the tests.


79-132: Well-designed test helper function.

The createComponentWithFilter function effectively emulates the component's behavior without requiring complex mocking of Vue's composition API. This approach makes the tests more robust and easier to maintain.


134-208: Comprehensive rendering and behavior tests.

This section thoroughly tests the component's rendering behavior under different conditions:

  • Empty state handling
  • Data source selection logic (store vs props)
  • Filtering capability
  • Styling application

The tests are focused and specific, with clear assertions.


210-232: Detailed attribute verification.

The test verifies all relevant button attributes including:

  • href values
  • external flags
  • title translations
  • disabled state

This ensures that the component connects its data properly to the DOM elements.


234-266: Thorough event handling and edge case testing.

The tests cover both event handling and edge cases like undefined filters. This ensures the component behaves correctly in all scenarios.

web/test/mocks/utils/component-mock.ts (4)

1-12: Clear utility documentation.

The file begins with concise documentation explaining the purpose of these utilities - to simplify testing Vue components that use the composition API without complex mocking.


19-35: Well-designed generic mock component creator.

The createMockComponent function uses proper TypeScript generics to ensure type safety while providing flexibility. The function signature is clear and the implementation is concise.


44-80: Robust filtering logic implementation.

The filtering implementation in createListComponentMockFactory handles various combinations of filter conditions effectively:

  • It properly prioritizes items vs storeItems
  • It handles both inclusion and exclusion filtering
  • It gracefully manages undefined/missing filter arrays

The logic matches what would be expected in a real component's computed properties.


81-98: Clean component template generation.

The mock component creation code separates template generation from data management, making the code more maintainable. The default options (like the translation function) are sensibly implemented.

web/test/mocks/utils/README.md (7)

1-4: Introduction Clarity
The title and opening paragraph effectively set the context for the Vue Component Testing Utilities. The introduction is clear and engaging.


13-21: Solution Section Verification
The explanation of the Component Mocking Pattern succinctly highlights the benefits of the approach. The bullet points clearly communicate the advantages, making it easy for readers to understand the rationale.


22-31: Utility Documentation: createMockComponent
The utility documentation for createMockComponent is precise and includes the generic signature, which is very helpful. Ensure that the implementation in code stays in sync with this signature.


32-45: Utility Documentation: createListComponentMockFactory
The signature and description for createListComponentMockFactory are well presented. This section clearly explains the parameters and expected functionality, aiding developers in understanding its intended usage.


47-53: Usage Examples Section
The "Usage Examples" section efficiently directs users to practical examples. This guidance enhances comprehension and aids new developers in quickly grasping how to apply these utilities.


54-62: When to Use This Approach
This section effectively enumerates scenarios where the mocking approach is beneficial. The comprehensive list helps developers decide if this pattern suits their specific testing challenges.


63-70: Implementation Notes and Contributing
The final sections provide clear advice on maintaining synchronization between the mock and actual component logic, and they encourage community contributions. This guidance is practical and forward-thinking.

@elibosley elibosley merged commit 0e008aa into main Apr 3, 2025
9 checks passed
@elibosley elibosley deleted the feat/add-front-end-unit-testing branch April 3, 2025 19:50
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.

4 participants