Review code for improvements and best practices #1

sscarduzio · 2026-01-07T14:23:07Z

No description provided.

…agement - Add useKeyPairs hook to centralize key pair state and localStorage logic - Add useToast hook with ToastProvider for non-blocking notifications - Add unified validation service with validateKeyPair function - Refactor ActivationKeyEditor to use useReducer for predictable state - Remove Monaco Editor key prop to fix unnecessary re-mounts - Replace browser alerts with toast notifications throughout - Extract EXAMPLE_JWT constant and EMPTY_KEY_PAIR constant - Use useCallback for memoized event handlers These changes improve: - DRY: Single source of truth for keyPairs across pages - Maintainability: Reducer pattern makes state changes traceable - Performance: Monaco no longer remounts on JWT changes - UX: Toast notifications instead of blocking alerts

- Add pr-preview.yml workflow for PR preview deployments - Add deploy.yml workflow for main branch deployment - Update vite.config.ts to support dynamic base path via VITE_BASE_PATH PR previews will be available at: https://<org>.github.io/ak-tools/pr-preview/pr-<number>/

cloudflare-workers-and-pages · 2026-01-07T14:32:44Z

Deploying ak-tools with Cloudflare Pages

Latest commit:	`f3b3b4e`
Status:	✅ Deploy successful!
Preview URL:	https://92d09a3c.ak-tools.pages.dev
Branch Preview URL:	https://claude-code-review-improveme.ak-tools.pages.dev

View logs

- Add issuedDate and onIssuedChange props to ActivationKeyMetadataDisplay - Add SET_ISSUED_DATE action to editor reducer - Extract and display issuedAt from JWT metadata - Update payload.iat when signing with custom issued date

- Add useTemplates hook for managing activation key templates - Create Templates page for adding/editing/deleting templates - Add Templates route to navigation (Editor, Templates, Keys) - Replace "Use example" with templates dropdown in editor - Include default Anaphora Free template - Templates stored in localStorage with CRUD operations

- Add "Save as template" button in editor when AK is loaded - Show warning dialog when saving non-expired AK as template - Prompt for template name and optional description - Add safety notice to Templates page explaining expired-only policy - Show "Not Expired" badge on templates that aren't expired - Update template preview to show expiry status Safety feature: Templates should only contain expired activation keys to prevent accidental distribution of valid licenses. Users are warned when attempting to save non-expired keys but can still proceed.

- Add comprehensive activation key format validation with detailed error messages explaining structure issues (header, payload, signature) - Rename interface from JwtTemplate to AkTemplate - Replace 'jwt' field with 'activationKey' throughout templates system - Update all user-facing strings from 'JWT' to 'activation key' - Add inline validation feedback in template creation/edit forms - Display validation errors with explanatory details

- Track source template when loading from templates - Pre-fill template name/description from source template - Show "Update Template" button when editing source template - Add confirmation dialog for overwriting existing templates - Add "Save as expired" checkbox that sets expiry to 24h ago - Automatically re-sign activation key with expired date when saving

- Add ExternalLink button to each template card in Templates page - Store pending template ID in localStorage when clicked - Editor checks for pending template on mount and loads it automatically - Sets source template ID to enable update flow when saving back - Added tooltips to all template card action buttons

- Remove heavy warning banner (safety enforced in save flow instead) - Clean header with integrated "New Template" action button - Compact single-row template cards with inline metadata - Subtle "Active" badge with tooltip for non-expired keys - Better empty state with centered icon and call-to-action - Inline edit mode with delete action visible only when editing - Proper tooltips on all action buttons via TooltipProvider - Responsive grid layout for form fields - Cleaner typography and spacing throughout

- Create useLocalStorageCollection<T> generic hook for localStorage persistence, reducing ~100 lines of duplicated logic - Create useCrudState<T> hook for managing add/edit form state patterns - Refactor use-key-pairs.ts to use generic hook (94 → 31 lines) - Refactor use-templates.ts to use generic hook (123 → 57 lines) - Apply useCrudState to Keys.tsx and Templates.tsx for consistent state management across CRUD forms

The "Open in Editor" feature was broken because the useEffect that loads the pending template was running before templates were loaded from localStorage. Now it waits for templates.length > 0 before attempting to load the template.

UX improvements to the Activation Key Editor: - Add template switcher at top of right panel - Shows current template name (or "No template") - Orange dot indicator when editor has unsaved changes - Dropdown to switch between templates without leaving editor - Confirms before switching if there are unsaved changes - Move signing key selection below metadata - Clear label with key icon: "Signing Key" - Positioned near the "Generate" button for logical flow - Track dirty state to warn users about unsaved changes - Compares current editor value with original loaded value - Visual indicator (orange dot) in template switcher

- Add "Template" label above the template selector for clarity - Move validation checkmark/X inline with the template dropdown - Remove separate ValidationStatus component, use compact indicator - Cleaner layout: validation icon → template dropdown in same row

- Update color palette with vibrant but professional blue accent - Light mode: Clean blue (#3B82F6) primary with subtle tinted background - Dark mode: Brighter blue accent with deeper navy background - Add subtle gradient pattern on page background - Redesign navbar: sticky, frosted glass effect, glowing logo - Add nav-link hover underline animation - Smaller, more refined header (h-16 from h-20) - Add utility classes for interactive cards, status indicators - Keep it enterprise-appropriate: sophisticated, not flashy

Add _redirects file to serve index.html for all routes, fixing client-side navigation on Cloudflare Pages deployment.

Add a professional footer with: - Copyright notice for Beshu Tech - Links to ReadonlyREST, Anaphora, and Beshu websites - Subtle hover effects with external link indicators - Responsive layout (stacks on mobile)

Copilot

Pull request overview

This PR implements significant improvements to the activation key management tool, adding template functionality, improving state management, and enhancing the user experience with a modern toast notification system.

Key Changes:

Introduces a Templates feature for saving and reusing activation key configurations with safety warnings for non-expired keys
Refactors state management with custom hooks (useLocalStorageCollection, useCrudState, useKeyPairs, useTemplates) for better code organization
Replaces browser alerts with a custom toast notification system and confirmation dialogs
Adds comprehensive validation utilities for activation keys and key pairs with detailed error messages

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
vite.config.ts	Adds base path configuration for PR preview deployments
src/utils/validation.ts	New validation utilities for key pairs with format checking and error messages
src/utils/activationKey.ts	Adds detailed activation key validation with structured error responses
src/index.css	Updates theme colors and adds custom CSS utility classes for UI enhancements
src/hooks/use-toast.tsx	New toast notification system with confirmation dialog support
src/hooks/use-templates.ts	Hook for managing activation key templates with localStorage persistence
src/hooks/use-local-storage-collection.ts	Generic hook for CRUD operations on localStorage collections
src/hooks/use-key-pairs.ts	Specialized hook for key pair management using the collection hook
src/hooks/use-crud-state.ts	Reusable hook for managing add/edit UI state patterns
src/components/pages/Templates.tsx	New Templates page for managing reusable activation key configurations
src/components/pages/Keys.tsx	Refactored to use new hooks and toast system, removing localStorage logic
src/components/pages/ActivationKeyEditor.tsx	Major refactor with useReducer, template integration, and improved UX
src/components/activationkey/ActivationKeyMetadata.tsx	Adds issued date modification capability
src/App.tsx	Adds Templates route, wraps app with ToastProvider, adds footer with branding
public/_redirects	SPA routing configuration for deployment platforms
package-lock.json	Adds Node.js engine requirement

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-07T16:35:12Z

src/utils/validation.ts

+  if (trimmed.length < 2) {
+    return 'Name must be at least 2 characters';
+  }
+
+  if (trimmed.length > 100) {
+    return 'Name must be less than 100 characters';


Magic numbers 2 and 100 used for name length validation without being defined as named constants. These constraints should be extracted as constants (e.g., MIN_NAME_LENGTH = 2, MAX_NAME_LENGTH = 100) to improve maintainability and make it easier to adjust these limits in the future.

Copilot · 2026-01-07T16:35:12Z

src/hooks/use-toast.tsx

+  const timeoutsRef = useRef<Map<string, NodeJS.Timeout>>(new Map());
+
+  const dismiss = useCallback((id: string) => {
+    const timeout = timeoutsRef.current.get(id);
+    if (timeout) {
+      clearTimeout(timeout);
+      timeoutsRef.current.delete(id);
+    }
+    setToasts(prev => prev.filter(t => t.id !== id));
+  }, []);
+
+  const toast = useCallback((type: ToastType, message: string, duration = DEFAULT_DURATION) => {
+    const id = crypto.randomUUID();
+    const newToast: Toast = { id, type, message, duration };
+
+    setToasts(prev => [...prev, newToast]);
+
+    if (duration > 0) {
+      const timeout = setTimeout(() => {
+        dismiss(id);
+      }, duration);
+      timeoutsRef.current.set(id, timeout);
+    }
+  }, [dismiss]);


Potential memory leak: The timeoutsRef Map stores timeout IDs but they are only removed when toasts are dismissed. If a component unmounts while toasts are active, these timeouts will continue to run. Consider adding a cleanup effect that clears all timeouts when the component unmounts.

Copilot · 2026-01-07T16:35:12Z

src/utils/activationKey.ts

+      };
+    }
+
+    if (!header.typ && header.typ !== undefined && header.typ !== 'JWT') {


The condition logic is incorrect. This condition will always be false because if header.typ is undefined, the first check fails; if it's defined and equals 'JWT', the second check fails. The intended logic should likely be: if header.typ is defined and is NOT 'JWT', then it's invalid.

Suggested change

if (!header.typ && header.typ !== undefined && header.typ !== 'JWT') {

if (header.typ !== undefined && header.typ !== 'JWT') {

Copilot · 2026-01-07T16:35:13Z

src/components/pages/ActivationKeyEditor.tsx

+  const handleSwitchTemplate = useCallback(async (templateId: string) => {
+    const template = getTemplateById(templateId);
+    if (!template) return;
+
+    // Check for unsaved changes
+    if (isDirty) {
+      const confirmed = await confirm(
+        'You have unsaved changes. Switch template anyway?'
+      );
+      if (!confirmed) return;
+    }
+
+    setTemplateSwitcherOpen(false);
+    setSourceTemplateId(template.id);
+    handleInputChange(template.activationKey);
+  }, [isDirty, confirm, getTemplateById, handleInputChange]);


The handleSwitchTemplate callback includes isDirty in its dependencies, but isDirty is computed from state values rather than being a stable reference. This could cause unnecessary recreations of the callback. Consider using useMemo for isDirty or removing it from the dependency array if the callback can safely close over the current value.

Copilot · 2026-01-07T16:35:13Z

src/utils/activationKey.ts

+  if (!signaturePart || signaturePart.length < 10) {
+    return {
+      isValid: false,
+      errorType: 'structure',
+      message: 'Missing or invalid signature',
+      details: 'The signature section appears to be missing or too short. Make sure you copied the complete activation key.'


Magic number 10 used for signature length validation without explanation. This arbitrary threshold could incorrectly reject valid short signatures or accept invalid longer ones. Consider either removing this check (since the signature format will be validated by the jose library anyway) or documenting why 10 characters is the minimum valid length for a base64url-encoded signature.

Suggested change

if (!signaturePart || signaturePart.length < 10) {

return {

isValid: false,

errorType: 'structure',

message: 'Missing or invalid signature',

details: 'The signature section appears to be missing or too short. Make sure you copied the complete activation key.'

if (!signaturePart) {

return {

isValid: false,

errorType: 'structure',

message: 'Missing or invalid signature',

details: 'The signature section appears to be missing or incomplete. Make sure you copied the complete activation key.'

Copilot · 2026-01-07T16:35:13Z

src/hooks/use-local-storage-collection.ts

+  useEffect(() => {
+    try {
+      const saved = localStorage.getItem(storageKey);
+      if (saved) {
+        const parsed = JSON.parse(saved);
+        if (Array.isArray(parsed) && parsed.length > 0) {
+          setItems(parsed);
+        } else if (defaultItems.length > 0) {
+          setItems(defaultItems);
+          localStorage.setItem(storageKey, JSON.stringify(defaultItems));
+        }
+      } else if (defaultItems.length > 0) {
+        setItems(defaultItems);
+        localStorage.setItem(storageKey, JSON.stringify(defaultItems));
+      }
+    } catch (e) {
+      setError(e instanceof Error ? e : new Error(`Failed to load ${entityName} from storage`));
+      if (defaultItems.length > 0) {
+        setItems(defaultItems);
+      }
+    } finally {
+      setIsLoading(false);
+    }
+  }, [storageKey, entityName, defaultItems]);


The useEffect has defaultItems in its dependency array, but defaultItems is created on every render. This will cause the effect to run on every render, potentially triggering unnecessary localStorage operations and re-initializations. The useMemo wrapping DEFAULT_TEMPLATES in the calling code doesn't prevent this issue because the defaultItems parameter itself is a new reference each time.

Copilot · 2026-01-07T16:35:14Z

src/components/pages/ActivationKeyEditor.tsx

+    const selectedKey = getKeyPairById(selectedKeyId);
    try {
-      // Only validate against the selected key pair
-      const selectedKey = keyPairs.find(k => k.id === selectedKeyId);
-      const validation = await validateJWTSignature(value, selectedKey);
-      setSignatureValidation(validation);
-    } catch (error) {
-      setSignatureValidation({
-        isValid: false,
-        error: "Invalid signature",
+      const validationResult = await validateJWTSignature(value, selectedKey);
+      dispatch({ type: 'SET_VALIDATION', payload: validationResult });
+    } catch {
+      dispatch({
+        type: 'SET_VALIDATION',
+        payload: { isValid: false, error: 'Invalid signature' },
      });
    }
-  };
+  }, [selectedKeyId, getKeyPairById]);


The handleInputChange callback depends on selectedKeyId and getKeyPairById but attempts to use a selected key even when selectedKeyId might be empty. If selectedKeyId is empty or invalid, getKeyPairById will return undefined, which could lead to validation issues. Consider adding a check or initializing the selectedKeyId before attempting validation.

Copilot · 2026-01-07T16:35:14Z

src/components/pages/ActivationKeyEditor.tsx

+  const handleInputChange = useCallback(async (value: string) => {
+    dispatch({ type: 'SET_INPUT', payload: value });

    if (!value) {
-      clearJwt();
+      dispatch({ type: 'CLEAR' });
      return;
    }

-    if (!selectedKeyId && keyPairs.length > 0) {
-      setSelectedKeyId(keyPairs[0].id);
-    }
-
    // Try to parse as JWT
    const metadata = getJwtMetadata(value);
+    let newAlgorithm: Algorithm | undefined;
+    let newExpiryDate: Date | undefined;
+    let newIssuedDate: Date | undefined;
+
    if (metadata) {
      if (metadata.algorithm && SUPPORTED_ALGORITHMS.includes(metadata.algorithm as Algorithm)) {
-        setAlgorithm(metadata.algorithm as Algorithm);
+        newAlgorithm = metadata.algorithm as Algorithm;
      }
      if (metadata.expiresAt) {
-        setExpiryDate(new Date(metadata.expiresAt));
+        newExpiryDate = new Date(metadata.expiresAt);
+      }
+      if (metadata.issuedAt) {
+        newIssuedDate = new Date(metadata.issuedAt);
      }
    }

    const decoded = await decodeJWT(value);
+    let payloadOnly = '{}';
    try {
      const parsedDecoded = JSON.parse(decoded);
-      const payloadOnly = JSON.stringify(parsedDecoded.payload || {}, null, 2);
-      setEditorValue(payloadOnly);
-    } catch (e) {
-      setEditorValue('{}');
+      payloadOnly = JSON.stringify(parsedDecoded.payload || {}, null, 2);
+    } catch {
+      // Keep default empty object
    }
-    setJwt(value);

+    dispatch({
+      type: 'SET_JWT',
+      payload: {
+        jwt: value,
+        editorValue: payloadOnly,
+        algorithm: newAlgorithm,
+        expiryDate: newExpiryDate,
+        issuedDate: newIssuedDate,
+      },
+    });
+
+    // Track original value for dirty detection
+    originalEditorValueRef.current = payloadOnly;
+
+    // Validate signature
+    const selectedKey = getKeyPairById(selectedKeyId);
    try {
-      // Only validate against the selected key pair
-      const selectedKey = keyPairs.find(k => k.id === selectedKeyId);
-      const validation = await validateJWTSignature(value, selectedKey);
-      setSignatureValidation(validation);
-    } catch (error) {
-      setSignatureValidation({
-        isValid: false,
-        error: "Invalid signature",
+      const validationResult = await validateJWTSignature(value, selectedKey);
+      dispatch({ type: 'SET_VALIDATION', payload: validationResult });
+    } catch {
+      dispatch({
+        type: 'SET_VALIDATION',
+        payload: { isValid: false, error: 'Invalid signature' },
      });
    }
-  };
+  }, [selectedKeyId, getKeyPairById]);


The naming is inconsistent between "Activation Key" (used in most of the UI and new code) and "JWT" (used in function names like handleInputChange, getJwtMetadata, etc.). While both terms are technically correct, using a consistent term throughout would improve code clarity. Consider renaming functions to use "activationKey" instead of "jwt" for consistency with the user-facing terminology.

Copilot · 2026-01-07T16:35:14Z

src/components/pages/ActivationKeyEditor.tsx

+    // Set expiry to 24 hours ago
+    const expiredDate = new Date(Date.now() - 24 * 60 * 60 * 1000);
+    payload.exp = Math.floor(expiredDate.getTime() / 1000);


Missing input validation for the expiry date modification. When setting expiry to 24 hours ago, there's no validation to ensure the resulting date is actually in the past or that it doesn't conflict with the issued date (iat). If the system clock is manipulated or there are timezone issues, this could create invalid JWTs where exp < iat.

Copilot · 2026-01-07T16:35:14Z

src/components/pages/ActivationKeyEditor.tsx

  useEffect(() => {
-    if (jwt && selectedKeyId) {
-      const selectedKey = keyPairs.find(k => k.id === selectedKeyId);
-      validateJWTSignature(jwt, selectedKey).then(setSignatureValidation);
+    const pendingTemplateId = localStorage.getItem(PENDING_TEMPLATE_KEY);
+    if (pendingTemplateId && templates.length > 0) {
+      const template = getTemplateById(pendingTemplateId);
+      if (template) {
+        localStorage.removeItem(PENDING_TEMPLATE_KEY);
+        setSourceTemplateId(template.id);
+        handleInputChange(template.activationKey);
+      }
    }
-  }, [selectedKeyId, jwt]);
+  }, [templates, getTemplateById, handleInputChange]);


The dependency array for the useEffect that loads pending templates includes handleInputChange, which is a useCallback that itself has dependencies. This could cause the effect to run more often than necessary. Since this effect should only run when templates change or on mount, consider restructuring to avoid including handleInputChange in the dependencies, or use a ref to call it.

- validation.ts: Extract magic numbers as MIN_NAME_LENGTH, MAX_NAME_LENGTH - use-toast.tsx: Add cleanup effect to clear timeouts on unmount - activationKey.ts: Fix JWT type validation logic (was always false) - activationKey.ts: Remove arbitrary signature length check - ActivationKeyEditor.tsx: Use useMemo for isDirty computation - ActivationKeyEditor.tsx: Add proper selectedKeyId validation - ActivationKeyEditor.tsx: Fix handleInputChange callback dependency - ActivationKeyEditor.tsx: Add exp/iat validation for templates - use-local-storage-collection.ts: Use ref for defaultItems

claude added 3 commits January 7, 2026 14:18

chore: update package-lock.json

fab88f6

claude added 15 commits January 7, 2026 14:39

feat: make Issued date editable via calendar picker

1d3496c

- Add issuedDate and onIssuedChange props to ActivationKeyMetadataDisplay - Add SET_ISSUED_DATE action to editor reducer - Extract and display issuedAt from JWT metadata - Update payload.iat when signing with custom issued date

chore: remove GitHub Pages workflows (using Cloudflare Pages)

e5cae4c

fix: add Cloudflare Pages redirects for SPA routing

1060d9b

Add _redirects file to serve index.html for all routes, fixing client-side navigation on Cloudflare Pages deployment.

feat: add footer with Beshu, ReadonlyREST, and Anaphora links

92741cb

Add a professional footer with: - Copyright notice for Beshu Tech - Links to ReadonlyREST, Anaphora, and Beshu websites - Subtle hover effects with external link indicators - Responsive layout (stacks on mobile)

sscarduzio requested a review from Copilot January 7, 2026 16:29

Copilot started reviewing on behalf of sscarduzio January 7, 2026 16:29 View session

Copilot AI reviewed Jan 7, 2026

View reviewed changes

sscarduzio merged commit f3bd755 into main Jan 7, 2026
1 check passed

sscarduzio deleted the claude/code-review-improvements-M5DkL branch January 7, 2026 17:23

	if (!header.typ && header.typ !== undefined && header.typ !== 'JWT') {
	if (header.typ !== undefined && header.typ !== 'JWT') {

Review code for improvements and best practices #1

Review code for improvements and best practices #1

Uh oh!

Conversation

sscarduzio commented Jan 7, 2026

Uh oh!

cloudflare-workers-and-pages bot commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying ak-tools with Cloudflare Pages

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cloudflare-workers-and-pages bot commented Jan 7, 2026 •

edited

Loading