Skip to content

Unify printer discovery device keys#279

Open
kilbot wants to merge 2 commits into
nextfrom
fix/unify-printer-device-key
Open

Unify printer discovery device keys#279
kilbot wants to merge 2 commits into
nextfrom
fix/unify-printer-device-key

Conversation

@kilbot

@kilbot kilbot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Summary

  • import the shared printer device-key codec from @wcpos/printer
  • return full discovered-printer shapes from USB and serial discovery IPC handlers
  • route serial/winspool raw-print handlers through parsed device-key kinds instead of local prefix parsing

Companion PRs

Verification

  • cd packages/printer && ../../node_modules/.bin/tsc --noEmit
  • cd apps/electron && pnpm ts:check
  • cd packages/printer && ../../node_modules/.bin/vitest run src/hooks/use-printer-discovery.electron.test.tsx src/transport/__tests__/device-adapter.electron.test.ts src/transport/__tests__/device-key.test.ts
  • cd apps/electron && pnpm run test:serial-printer && pnpm run test:winspool-printer && pnpm run test:printer-discovery
  • cd /Users/kilbot/Projects/monorepo-v2/.worktrees/unify-printer-device-key && pnpm lint
  • cd apps/electron && pnpm lint

Summary by CodeRabbit

  • New Features

    • Improved printer device identification and discovery for serial, USB, and spooler printers with enhanced connection type detection.
  • Chores

    • Added @wcpos/printer dependency.
    • Updated printer package to version 1.9.1.

@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: wcpos/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: be2c0c70-c402-4470-81bb-f34dd96a6991

📥 Commits

Reviewing files that changed from the base of the PR and between 5851f72 and 5e1c5f1.

📒 Files selected for processing (9)
  • package.json
  • packages/printer/package.json
  • packages/printer/src/transport/device-key.ts
  • pnpm-workspace.yaml
  • src/main/serial-printer.test.ts
  • src/main/serial-printer.ts
  • src/main/usb-printer.ts
  • src/main/winspool-printer.ts
  • tsconfig.json

📝 Walkthrough

Walkthrough

A new @wcpos/printer/transport/device-key module is created inside the packages/printer workspace, exposing prefix constants, ParsedTarget union types, key-builder functions, and connection-type resolution helpers. The serial-printer.ts, usb-printer.ts, and winspool-printer.ts IPC handlers are refactored to use these shared utilities, replacing inline string concatenation and regex matching with typed parse/build calls.

Changes

Shared device-key module and printer handler refactor

Layer / File(s) Summary
Workspace and package wiring
pnpm-workspace.yaml, package.json, packages/printer/package.json, tsconfig.json
Adds pnpm-workspace.yaml defining the monorepo layout, registers @wcpos/printer as a workspace:* dependency, bumps @wcpos/printer to 1.9.1, and adds a ts-node CJS module-type override for device-key.ts.
device-key.ts: types, parse, build, and connection helpers
packages/printer/src/transport/device-key.ts
Creates the new shared module with prefix constants, ParsedTarget/TargetConnectionType union types, TARGET_KIND_CONNECTION_TYPE mapping, parseTarget, key-builder functions (buildSerialKey, buildUsbKey, buildWinspoolKey, buildCloudTarget), and connectionTypeForParsedTarget/connectionTypeForTarget helpers.
Serial printer: DiscoveredSerialPrinter and parseTarget validation
src/main/serial-printer.ts, src/main/serial-printer.test.ts
Re-exports SERIAL_PREFIX from device-key, uses buildSerialKey for port IDs, adds DiscoveredSerialPrinter interface and toDiscoveredSerialPrinter helper (enforcing bluetooth), updates serial-discovery to return DiscoveredSerialPrinter[], and replaces manual prefix slicing in print-raw-serial with parseTarget + target.kind === 'serial' guard. Test updated to assert full shape.
USB printer: unified discoveredPrinter and parseTarget routing
src/main/usb-printer.ts
Removes vendorId/productId from UsbPrinterInfo, adds discoveredPrinter helper using buildUsbKey/connectionTypeForTarget, maps Windows spooler queues into the unified shape, and replaces regex-based print-raw-usb routing with parseTarget-based winspool/USB dispatch.
Winspool printer: delegate prefix and ID building to device-key
src/main/winspool-printer.ts
Imports buildWinspoolKey and WINSPOOL_PREFIX from device-key, re-exports the prefix constant, and switches filterSpoolerPrinters to compute IDs via buildWinspoolKey(p.name).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • wcpos/electron#239: Adds the original usb-discovery and print-raw-usb IPC handlers in src/main/usb-printer.ts that this PR refactors to use the shared parseTarget/buildUsbKey utilities.

Poem

🖨️ No more magic strings to parse,
A device-key to chart the course.
Serial, USB, winspool too—
One parseTarget to rule them through.
Keys now built with typed delight,
The printer stack is clean and right! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main objective of the changeset, which consolidates printer device key handling across serial, USB, and winspool discovery/printing code.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/unify-printer-device-key

Comment @coderabbitai help to get the list of available commands and usage tips.

@wcpos-bot

wcpos-bot Bot commented Jun 17, 2026

Copy link
Copy Markdown

Fix round triage before changes:

# Source File Class Category Decision Outcome
1 CI test package.json Mechanical Install failure Fix pnpm install fails with ERR_PNPM_WORKSPACE_PKG_NOT_FOUND because @wcpos/printer@workspace:* is declared but this standalone repo has no @wcpos/printer workspace package.

Unresolved inline review threads from fresh GraphQL inventory: 0.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f8ece73a5a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread package.json
@wcpos-bot

wcpos-bot Bot commented Jun 17, 2026

Copy link
Copy Markdown

Fix pushed: 5e1c5f1 adds a minimal local pnpm workspace package for @wcpos/printer, so the standalone Electron checkout can resolve @wcpos/printer@workspace:* during pnpm install. This addresses the failing test check install blocker.

Fresh unresolved inline review-thread inventory after push: 0 remaining.

Thread File Issue Decision Commit
discussion package.json @wcpos/printer@workspace:* was not resolvable in this standalone repo checkout, blocking install before tests. Fixed and resolved 5e1c5f1

Excluded threads: none.

@kilbot kilbot changed the base branch from main to next June 17, 2026 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant