Skip to content

Address PR #251 review feedback: fix imports, memory leaks, accessibility, and add tests#252

Merged
kocheck merged 4 commits intoclaude/refactor-modular-architecture-GUfVCfrom
copilot/sub-pr-251
Feb 10, 2026
Merged

Address PR #251 review feedback: fix imports, memory leaks, accessibility, and add tests#252
kocheck merged 4 commits intoclaude/refactor-modular-architecture-GUfVCfrom
copilot/sub-pr-251

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Feb 10, 2026

Addresses 6 review comments from PR #251 focusing on correctness (import paths, memory management), performance (asset pipeline), and accessibility (keyboard navigation).

Import Path Fix

  • DoorBlocking.test.ts: Corrected relative imports ../types/*../../types/* to resolve to src/types/

Memory Leak Prevention

  • useCanvasDrop: Revoke existing object URL before creating new one when multiple files are dropped while crop is pending
  • useCanvasDrop: Revoke object URL on both confirm and cancel paths

Asset Pipeline Integration

Replaced base64 conversion with asset processing pipeline for cropped images:

// Before: base64 string (33% overhead, in-memory)
const reader = new FileReader();
reader.readAsDataURL(blob);
reader.onloadend = () => {
  addToken({ src: reader.result as string, ... });
};

// After: WebP + temp storage
const file = new File([blob], `token-${Date.now()}.png`, { type: blob.type });
const handle = processImage(file, 'TOKEN');
const processedSrc = await handle.promise;
addToken({ src: processedSrc.replace('file:', 'media:'), ... });

Reduces memory usage, store size, and IPC sync payload.

Accessibility Improvements

  • CommandPalette: Added tabIndex={0} + aria-activedescendant pattern for keyboard navigation
  • CanvasAccessibility: Blocked arrow key token movement in World View to prevent unsynced edits

Test Coverage

Added useCanvasDrop.test.ts with 12 tests covering:

  • Drop behavior (library tokens, generic tokens, file drops)
  • Object URL lifecycle (creation, revocation, memory leak scenarios)
  • Asset pipeline integration and error handling
  • Cancel/cleanup paths

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 3 commits February 10, 2026 20:22
…essibility

- Fix DoorBlocking.test.ts import paths (../types → ../../types)
- Revoke existing object URL before creating new one in useCanvasDrop
- Use asset pipeline (processImage) instead of base64 for cropped images
- Improve CommandPalette keyboard accessibility (tabIndex={0} + aria-activedescendant)
- Gate arrow key movement in World View in CanvasAccessibility

Addresses feedback from PR #251 review thread.

Co-authored-by: kocheck <7952000+kocheck@users.noreply.github.com>
- Test drag over behavior (World View blocking)
- Test library token drop
- Test generic token drop
- Test file drop and object URL creation
- Test object URL revocation on multiple drops (memory leak prevention)
- Test crop confirm with asset pipeline integration
- Test crop confirm error handling
- Test crop cancel cleanup

12 tests covering all main branches and edge cases.
Addresses Issue #6 from PR review thread.

Co-authored-by: kocheck <7952000+kocheck@users.noreply.github.com>
Wrap handleCropConfirm with void operator to satisfy TypeScript linter
when passing async function to prop expecting void return.

Co-authored-by: kocheck <7952000+kocheck@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Feb 10, 2026

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • collector-prod.1am6wm210aow.us-south.codeengine.appdomain.cloud
    • Triggering command: /home/REDACTED/work/_temp/ghcca-node/node/bin/node node /home/REDACTED/work/Graphium/Graphium/node_modules/@ibm/telemetry-js/dist/REDACTED-process.js --log=/tmp/ibmtelemetry-20260210T202049341Z-332188.log --config=telemetry.yml (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI changed the title [WIP] Add modular architecture refactor game plan Address PR #251 review feedback: fix imports, memory leaks, accessibility, and add tests Feb 10, 2026
Copilot AI requested a review from kocheck February 10, 2026 20:29
@kocheck kocheck marked this pull request as ready for review February 10, 2026 21:35
@kocheck kocheck merged commit f7a78f3 into claude/refactor-modular-architecture-GUfVC Feb 10, 2026
@kocheck kocheck deleted the copilot/sub-pr-251 branch February 10, 2026 21:35
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.

2 participants