Start new v0.11.0 development#32
Open
Fl0p wants to merge 4 commits into
Open
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Regenerate openspec skills for claude/codex/cursor harnesses - Add opsx command files for claude and cursor - Ignore macOS, editor, log, and env/secret noise Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…df subpath Add a node-only `ksef-client-ts/pdf` subpath that renders FA(2)/FA(3) invoices and UPO(4.2)/(4.3) receipts to PDF from a template-driven block DSL, with pl/en/pl+en labels and automatic KSeF Code I QR whose hash is taken over the original input bytes. pdfmake is an optional peer (^0.2.20), lazily loaded, so the core install stays clean and `./pdf` imports never pull it in. Also adds the `ksef invoice pdf` CLI command, built-in templates for each supported version, i18n bundles, a verification harness (attw / publint / pdf-types / cold-subpath across the Node 18/20/22 matrix), a how-to guide, and the OpenSpec spec for the new invoice-pdf-render capability. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EaMDwzgFf8zzWn7mM1Tjq5
The invoice PDF renderer dropped bank account data entirely and emitted a QR verification URL with a NaN date: the payment due date and issue date were bound to the wrong XML paths, the payment-form code was left raw, and bank details were never modeled at all. - Add a bank-account repeater to the payment block (account number, SWIFT, bank name), rendered as localized label:value lines; skip empty optional payment fields. - Bind the payment due date to the payment-term element and decode the payment-form code to its name. - Read the QR issue date from Fa/P_1 (not the document root) and guard the verification-link service against an unparseable date instead of emitting a NaN segment. - Rebuild the FA(2)/FA(3) fixtures from a realistic payment structure so the strict self-consistency test exercises the payment/bank paths, and add tests for the QR date, the payment-form formatter, and bank-account rendering. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EaMDwzgFf8zzWn7mM1Tjq5
📝 WalkthroughWalkthroughAdds a new ChangesPDF Export Feature
Estimated code review effort: 4 (Complex) | ~75 minutes OpsX Workflow Command Documentation
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ksef-client-ts/src/pdf/template/builtin/upo-4_2.json (1)
1-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIdentical to
upo-4_3.jsonaside fromschema.Same DRY observation as the FA2/FA3 templates — both UPO templates are byte-for-byte identical except the
schemavalue. Consider factoring shared UPO block layout into one source of truth to avoid the two copies drifting apart when a field label or column changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ksef-client-ts/src/pdf/template/builtin/upo-4_2.json` around lines 1 - 44, The UPO 4.2 template is duplicated with UPO 4.3 except for the schema value, so the shared block layout should be centralized. Refactor the reusable structure in the UPO template definitions (the blocks/stack/columns layout in the builtin UPO JSON templates) into a single source of truth and have both schema variants reference it, keeping only the schema-specific value separate.
♻️ Duplicate comments (1)
packages/ksef-client-ts/src/pdf/template/builtin/fa3-default.json (1)
61-67: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame single-VAT-rate totals limitation as
fa2-default.json.See the corresponding comment on
fa2-default.json— the totals block here has the identical gap forFa.P_13_1/Fa.P_14_1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ksef-client-ts/src/pdf/template/builtin/fa3-default.json` around lines 61 - 67, The totals block in the fa3 default template has the same single-VAT-rate gap as the fa2 template, so it only maps the overall totals and misses the per-rate fields for Fa.P_13_1 and Fa.P_14_1. Update the totals configuration in the built-in JSON template to include the same VAT-rate-specific handling used elsewhere in the PDF templates, keeping the change localized to the totals rows definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/commands/opsx/apply.md:
- Around line 88-99: The transcript-style examples in this section are using
bare fences for non-code content; update the affected examples to use text
fences instead. Make the change consistently across the referenced blocks in the
apply.md instructions, keeping the same content but changing the fence language
so the markdown follows the repo rule for non-code snippets.
In @.claude/commands/opsx/archive.md:
- Around line 92-101: The archive output examples in the `archive` command docs
use bare fences for non-code transcripts, which violates the markdown rule.
Update the success/error transcript examples in this section to use `text`
fences consistently, including the examples around the `Archive Complete` block
and the related transcript sections referenced by the comment. Keep the content
unchanged and only adjust the fence language for the relevant example blocks.
In @.claude/commands/opsx/bulk-archive.md:
- Around line 51-54: The markdown examples in this doc use bare fenced blocks
for non-code content; update the capability map, status tables, and
transcript/output examples to use text fences consistently. Locate the affected
examples in the bulk-archive command doc and replace each non-code fence with a
text-tagged fence so the repo markdown rule is followed throughout.
In @.claude/commands/opsx/continue.md:
- Around line 41-45: The Output section in continue.md is currently
unconditional, but the `isComplete: true` branch already handles the final
handoff with `/opsx:apply` or `/opsx:archive`. Update the prompt flow around the
`isComplete` handling so the closing instruction is only shown when artifacts
are incomplete, and keep the completion branch in the `isComplete: true` path as
the only final stop message. Use the existing `isComplete` check and the final
output/suggestion text to locate the conditional prompt text.
In @.claude/commands/opsx/explore.md:
- Around line 57-72: The ASCII diagram block in explore.md is using a bare
fenced block, which violates the Markdown docs rule for non-code content. Update
the diagram section to use a text fence with the existing diagram content
preserved, and verify the surrounding markdown still renders correctly in the
opsx explore docs.
In @.claude/commands/opsx/verify.md:
- Around line 109-118: The verification report template in verify.md uses a bare
fenced block for the scorecard, which should be marked as non-code text. Update
the Markdown snippet under the Verification Report section to use a text fence
instead of an untyped fence, keeping the content in the scorecard block
unchanged; look for the template block containing “Verification Report”,
“Summary”, and the Completeness/Correctness/Coherence table.
In @.cursor/commands/opsx-apply.md:
- Around line 88-99: The transcript-style example blocks in opsx-apply.md are
using bare fences, which violates the markdown rule for non-code content. Update
the affected examples to use text fences instead of plain triple-backtick fences
throughout this section, and make the same fence change in the other referenced
transcript blocks so they are consistently tagged as non-code.
In @.cursor/commands/opsx-archive.md:
- Around line 92-101: The archive transcript examples in opsx-archive.md use
bare fences for non-code content, which violates the markdown rule. Update the
archive output examples in this section to use text fences everywhere they
appear, including the blocks around the success and error transcripts, and keep
the wording intact while changing only the fence language.
In @.cursor/commands/opsx-bulk-archive.md:
- Around line 51-54: The doc uses bare fenced blocks for non-code content in the
capability map, status tables, and transcript/output examples, which violates
the markdown rule. Update the affected fenced sections in opsx-bulk-archive.md
to use text fences instead of plain triple backticks, including the examples
around the capability map and the later status/output blocks, and keep the
content unchanged aside from the fence language.
In @.cursor/commands/opsx-continue.md:
- Around line 41-45: The Output section is unconditionally ending with the
`/opsx:continue` prompt even when `isComplete: true`, which conflicts with the
completion branch in the same document. Update the logic in the opsx continue
prompt so the final call-to-action is conditional: when using the completion
path in the branch around the `isComplete: true` guidance, stop with the
apply/archive suggestion, and only use `/opsx:continue` for incomplete
artifacts. Keep the behavior aligned with the existing `isComplete` and Output
section wording.
In @.cursor/commands/opsx-explore.md:
- Around line 57-72: The diagram block in the opsx-explore markdown content is
using a bare fence instead of a text fence. Update the fenced block under the
ASCII diagram section to use a text code fence so the markdown docs rule is
satisfied, and keep the existing diagram content unchanged; the relevant block
is the one containing the “Use ASCII diagrams liberally” section.
In `@packages/ksef-client-ts/docs/pdf-export.md`:
- Around line 175-181: The `format` documentation in `pdf-export.md` is missing
the supported `paymentForm` formatter, while the block renderer and its tests
already rely on it. Update the `Bindings, labels, conditions, and formats`
section to include `paymentForm` alongside `money`, `date`, `number`, and `nip`,
using the same concise description style as the other formatters so template
authors can discover it.
In `@packages/ksef-client-ts/src/cli/commands/invoice.ts`:
- Around line 621-633: The invoice PDF selection logic in the command handler is
letting the forced UPO path win before explicit template options, so
`--template` and `--template-file` are ignored when `--upo` is present. Update
the branching in `invoice.ts` so explicit template selection takes priority over
the `isUpo`/`renderUpoPdf` path, keeping the built-in UPO render only as a
fallback when no template flag is provided. Use the `isUpo` check,
`renderUpoPdf`, `renderInvoicePdfFromFile`, and `renderInvoicePdf` flow to
locate the change, and add/adjust coverage in the invoice PDF CLI tests for the
`--upo` plus template combination.
- Around line 605-611: The `--env` option in `invoice.ts` is only being
type-cast in the render options and can still pass invalid values through,
unlike `--locale` which is validated first. Add explicit validation for
`args.env` before building `renderOpts` in the invoice command, using the same
fail-fast pattern as the `locale` handling, and only forward a value when it
matches the allowed env set (`prod`, `test`, `demo`); keep the existing
`renderOpts` construction in the `invoice` command but make sure invalid `--env`
inputs are rejected early.
In `@packages/ksef-client-ts/src/pdf/fonts.ts`:
- Around line 102-110: `createPdfBuffer` can hang forever if
`pdfMake.createPdf(...).getBuffer` never invokes its callback, so add a timeout
guard around the Promise in this function. Use a timer inside `createPdfBuffer`
to reject with a `KSeFPdfError` if `getBuffer` does not resolve in time, and
make sure to clear the timer on both resolve and reject so callers like
`renderWithTemplate` do not await indefinitely.
- Around line 86-93: The dynamic imports in the `fonts` module currently swallow
every failure and always rethrow `missingError()`, which hides the real cause.
Update the `try`/`catch` around the `pdfmakeMod` and `vfsMod` imports to capture
the thrown error and preserve it when reporting the failure, while keeping the
existing fallback message for true missing-module cases. Use the
`missingError()` path as the base, but attach or wrap the original import error
so debugging broken installs, permissions, or resolution issues remains
possible.
In `@packages/ksef-client-ts/src/pdf/parse.ts`:
- Around line 59-66: detectUpoVersion is matching KSeF/v4-3 and KSeF/v4-2
against the entire XML string, which can produce false positives even when the
root Potwierdzenie element is different. Tighten the version check by inspecting
only the root element’s opening tag/namespace in detectUpoVersion, or switch to
a parse path that preserves the root xmlns so the version can be read directly.
Keep the existing Potwierdzenie guard, and update the logic around
parseXmlForPdf and the regex checks to avoid scanning nested content.
In `@packages/ksef-client-ts/src/pdf/qr.ts`:
- Around line 67-81: The QR URL builder in deriveInvoiceQrUrl handles a missing
issue date but not a missing NIP, so a blank Podmiot1.DaneIdentyfikacyjne.NIP
can still produce a broken invoice verification link. Add an explicit
empty-string check for the nip value in deriveInvoiceQrUrl, mirroring the
existing issueDate guard, and throw KSeFPdfError with a clear message when NIP
is missing before calling VerificationLinkService.buildInvoiceVerificationUrl;
also add a qr.test.ts case covering the non-strict missing-NIP path.
In `@packages/ksef-client-ts/src/pdf/template/builtin/fa2-default.json`:
- Around line 1-90: The FA(2) template definition duplicates the same `blocks`
and `styles` used in `fa3-default.json`, leaving only `schema` different.
Refactor this JSON setup so the shared layout lives in one base object and
`fa2-default.json` only overrides `schema` while reusing the common template
structure, using the existing template config symbols like `blocks`, `styles`,
and `schema` to keep both FA templates in sync.
- Around line 61-67: The totals block in fa2-default.json is incomplete because
it only includes the first FA(2) VAT-rate group fields, so expand the
totals.rows mapping to cover every VAT-rate breakdown present in the invoice
data while keeping Fa.P_15 unchanged. Update the totals template entries in the
builtin PDF template so the rows reference all relevant Fa.P_13_* and Fa.P_14_*
symbols used for multi-rate invoices, preserving the existing totals structure
and labels.
In `@packages/ksef-client-ts/src/pdf/template/interpret.ts`:
- Around line 104-110: The divider rendering in interpret.ts currently hardcodes
the line length in divider() to a fixed width, which ignores page size and
margins. Update RenderContext to carry an effective content width derived from
PageConfig, then use that value in the divider block’s canvas line so the line
spans the actual printable area for any template. Keep the change localized
around interpret.ts, RenderContext, and the DividerBlock handling in dsl.ts.
In `@packages/ksef-client-ts/tests/unit/pdf/i18n.test.ts`:
- Around line 4-46: The resolveLabel test suite is missing coverage for the
Polish-fallback path in resolveOne, where a key exists in the pl bundle but is
absent from en. Add a regression test in resolveLabel that simulates an en-side
gap (via stubbing/overrides or a mocked bundle entry) and asserts the function
returns the Polish label instead of the raw key; use the existing resolveLabel
helper and its pl/en bundle lookup behavior to locate the branch.
In `@packages/ksef-client-ts/tsup.config.ts`:
- Around line 14-15: The tsup externalization for pdfmake is too narrow, so the
lazy PDF loader can still get bundled through subpath imports. Update the
tsup.config.ts external setting near the pdfmake entry to also match pdfmake
subpaths used by the runtime loader, referencing the existing pdfmake external
configuration, so imports like the build/pdfmake.js and build/vfs_fonts.js
variants remain external as well.
---
Outside diff comments:
In `@packages/ksef-client-ts/src/pdf/template/builtin/upo-4_2.json`:
- Around line 1-44: The UPO 4.2 template is duplicated with UPO 4.3 except for
the schema value, so the shared block layout should be centralized. Refactor the
reusable structure in the UPO template definitions (the blocks/stack/columns
layout in the builtin UPO JSON templates) into a single source of truth and have
both schema variants reference it, keeping only the schema-specific value
separate.
---
Duplicate comments:
In `@packages/ksef-client-ts/src/pdf/template/builtin/fa3-default.json`:
- Around line 61-67: The totals block in the fa3 default template has the same
single-VAT-rate gap as the fa2 template, so it only maps the overall totals and
misses the per-rate fields for Fa.P_13_1 and Fa.P_14_1. Update the totals
configuration in the built-in JSON template to include the same
VAT-rate-specific handling used elsewhere in the PDF templates, keeping the
change localized to the totals rows definition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d483902-bcdd-40df-a1e6-93b2d0c12ee7
⛔ Files ignored due to path filters (3)
openspec/specs/cli-invoice/spec.mdis excluded by!openspec/**openspec/specs/invoice-pdf-render/spec.mdis excluded by!openspec/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (109)
.claude/commands/opsx/apply.md.claude/commands/opsx/archive.md.claude/commands/opsx/bulk-archive.md.claude/commands/opsx/continue.md.claude/commands/opsx/explore.md.claude/commands/opsx/ff.md.claude/commands/opsx/new.md.claude/commands/opsx/sync.md.claude/commands/opsx/verify.md.claude/skills/openspec-apply-change/SKILL.md.claude/skills/openspec-archive-change/SKILL.md.claude/skills/openspec-bulk-archive-change/SKILL.md.claude/skills/openspec-continue-change/SKILL.md.claude/skills/openspec-explore/SKILL.md.claude/skills/openspec-ff-change/SKILL.md.claude/skills/openspec-new-change/SKILL.md.claude/skills/openspec-sync-specs/SKILL.md.claude/skills/openspec-verify-change/SKILL.md.codex/skills/openspec-apply-change/SKILL.md.codex/skills/openspec-archive-change/SKILL.md.codex/skills/openspec-bulk-archive-change/SKILL.md.codex/skills/openspec-continue-change/SKILL.md.codex/skills/openspec-explore/SKILL.md.codex/skills/openspec-ff-change/SKILL.md.codex/skills/openspec-new-change/SKILL.md.codex/skills/openspec-sync-specs/SKILL.md.codex/skills/openspec-verify-change/SKILL.md.cursor/commands/opsx-apply.md.cursor/commands/opsx-archive.md.cursor/commands/opsx-bulk-archive.md.cursor/commands/opsx-continue.md.cursor/commands/opsx-explore.md.cursor/commands/opsx-ff.md.cursor/commands/opsx-new.md.cursor/commands/opsx-sync.md.cursor/commands/opsx-verify.md.cursor/skills/openspec-apply-change/SKILL.md.cursor/skills/openspec-archive-change/SKILL.md.cursor/skills/openspec-bulk-archive-change/SKILL.md.cursor/skills/openspec-continue-change/SKILL.md.cursor/skills/openspec-explore/SKILL.md.cursor/skills/openspec-ff-change/SKILL.md.cursor/skills/openspec-new-change/SKILL.md.cursor/skills/openspec-sync-specs/SKILL.md.cursor/skills/openspec-verify-change/SKILL.md.github/workflows/ci.yml.gitignoreREADME.mdpackages/ksef-client-ts/CHANGELOG.mdpackages/ksef-client-ts/docs/.vitepress/config.tspackages/ksef-client-ts/docs/index.mdpackages/ksef-client-ts/docs/pdf-export.mdpackages/ksef-client-ts/package.jsonpackages/ksef-client-ts/scripts/check-pdf-cold.mjspackages/ksef-client-ts/src/cli/commands/invoice.tspackages/ksef-client-ts/src/pdf/accessor.tspackages/ksef-client-ts/src/pdf/errors.tspackages/ksef-client-ts/src/pdf/fonts.tspackages/ksef-client-ts/src/pdf/format.tspackages/ksef-client-ts/src/pdf/i18n/en.tspackages/ksef-client-ts/src/pdf/i18n/index.tspackages/ksef-client-ts/src/pdf/i18n/pl.tspackages/ksef-client-ts/src/pdf/i18n/types.tspackages/ksef-client-ts/src/pdf/index.tspackages/ksef-client-ts/src/pdf/parse.tspackages/ksef-client-ts/src/pdf/pdfmake-modules.d.tspackages/ksef-client-ts/src/pdf/qr.tspackages/ksef-client-ts/src/pdf/template/blocks/annotations.tspackages/ksef-client-ts/src/pdf/template/blocks/footer.tspackages/ksef-client-ts/src/pdf/template/blocks/header.tspackages/ksef-client-ts/src/pdf/template/blocks/image.tspackages/ksef-client-ts/src/pdf/template/blocks/index.tspackages/ksef-client-ts/src/pdf/template/blocks/lines.tspackages/ksef-client-ts/src/pdf/template/blocks/parties.tspackages/ksef-client-ts/src/pdf/template/blocks/payment.tspackages/ksef-client-ts/src/pdf/template/blocks/qr.tspackages/ksef-client-ts/src/pdf/template/blocks/table.tspackages/ksef-client-ts/src/pdf/template/blocks/totals.tspackages/ksef-client-ts/src/pdf/template/builtin/fa2-default.jsonpackages/ksef-client-ts/src/pdf/template/builtin/fa3-default.jsonpackages/ksef-client-ts/src/pdf/template/builtin/index.tspackages/ksef-client-ts/src/pdf/template/builtin/upo-4_2.jsonpackages/ksef-client-ts/src/pdf/template/builtin/upo-4_3.jsonpackages/ksef-client-ts/src/pdf/template/dsl.tspackages/ksef-client-ts/src/pdf/template/interpret.tspackages/ksef-client-ts/src/qr/verification-link-service.tspackages/ksef-client-ts/tests/fixtures/pdf-types-check.tspackages/ksef-client-ts/tests/fixtures/pdf/fa2.xmlpackages/ksef-client-ts/tests/fixtures/pdf/fa3.xmlpackages/ksef-client-ts/tests/fixtures/pdf/upo-4_2.xmlpackages/ksef-client-ts/tests/fixtures/pdf/upo-4_3.xmlpackages/ksef-client-ts/tests/unit/cli/commands/invoice-pdf.test.tspackages/ksef-client-ts/tests/unit/pdf/accessor.test.tspackages/ksef-client-ts/tests/unit/pdf/blocks-primitive.test.tspackages/ksef-client-ts/tests/unit/pdf/blocks-semantic.test.tspackages/ksef-client-ts/tests/unit/pdf/dsl.test.tspackages/ksef-client-ts/tests/unit/pdf/errors.test.tspackages/ksef-client-ts/tests/unit/pdf/fonts-loader.test.tspackages/ksef-client-ts/tests/unit/pdf/fonts.test.tspackages/ksef-client-ts/tests/unit/pdf/format.test.tspackages/ksef-client-ts/tests/unit/pdf/i18n.test.tspackages/ksef-client-ts/tests/unit/pdf/interpret.test.tspackages/ksef-client-ts/tests/unit/pdf/parse.test.tspackages/ksef-client-ts/tests/unit/pdf/qr.test.tspackages/ksef-client-ts/tests/unit/pdf/render-builtins.test.tspackages/ksef-client-ts/tests/unit/pdf/render-smoke.test.tspackages/ksef-client-ts/tests/unit/qr/verification-link-service.test.tspackages/ksef-client-ts/tsconfig.pdf-check.jsonpackages/ksef-client-ts/tsup.config.ts
Comment on lines
+88
to
+99
| ``` | ||
| ## Implementing: <change-name> (schema: <schema-name>) | ||
| Working on task 3/7: <task description> | ||
| [...implementation happening...] | ||
| ✓ Task complete | ||
| Working on task 4/7: <task description> | ||
| [...implementation happening...] | ||
| ✓ Task complete | ||
| ``` | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use text fences for the output examples.
These transcript-style blocks are non-code content, so bare fences violate the repo markdown rule. Please convert them to text fences throughout this section. As per coding guidelines, docs must tag non-code fences with text.
Also applies to: 102-115, 119-135
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/commands/opsx/apply.md around lines 88 - 99, The transcript-style
examples in this section are using bare fences for non-code content; update the
affected examples to use text fences instead. Make the change consistently
across the referenced blocks in the apply.md instructions, keeping the same
content but changing the fence language so the markdown follows the repo rule
for non-code snippets.
Source: Coding guidelines
Comment on lines
+92
to
+101
| ``` | ||
| ## Archive Complete | ||
|
|
||
| **Change:** <change-name> | ||
| **Schema:** <schema-name> | ||
| **Archived to:** openspec/changes/archive/YYYY-MM-DD-<name>/ | ||
| **Specs:** ✓ Synced to main specs | ||
|
|
||
| All artifacts complete. All tasks complete. | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use text fences for the archive output examples.
These success/error transcripts are non-code content, so bare fences violate the repo markdown rule. Please convert them to text fences throughout this section. As per coding guidelines, docs must tag non-code fences with text.
Also applies to: 105-114, 118-132, 136-148
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/commands/opsx/archive.md around lines 92 - 101, The archive output
examples in the `archive` command docs use bare fences for non-code transcripts,
which violates the markdown rule. Update the success/error transcript examples
in this section to use `text` fences consistently, including the examples around
the `Archive Complete` block and the related transcript sections referenced by
the comment. Keep the content unchanged and only adjust the fence language for
the relevant example blocks.
Source: Coding guidelines
Comment on lines
+51
to
+54
| ``` | ||
| auth -> [change-a, change-b] <- CONFLICT (2+ changes) | ||
| api -> [change-c] <- OK (only 1 change) | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use text fences for the tables and transcript examples.
The capability map, status tables, and output examples are non-code content, so bare fences violate the repo markdown rule. Please convert these sections to text fences throughout this doc. As per coding guidelines, docs must tag non-code fences with text.
Also applies to: 82-95, 98-101, 139-159, 164-192, 196-229
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/commands/opsx/bulk-archive.md around lines 51 - 54, The markdown
examples in this doc use bare fenced blocks for non-code content; update the
capability map, status tables, and transcript/output examples to use text fences
consistently. Locate the affected examples in the bulk-archive command doc and
replace each non-code fence with a text-tagged fence so the repo markdown rule
is followed throughout.
Source: Coding guidelines
Comment on lines
+41
to
+45
| **If all artifacts are complete (`isComplete: true`)**: | ||
| - Congratulate the user | ||
| - Show final status including the schema used | ||
| - Suggest: "All artifacts created! You can now implement this change with `/opsx:apply` or archive it with `/opsx:archive`." | ||
| - STOP |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the final prompt conditional on completion.
The isComplete: true branch already says to stop and suggest /opsx:apply or /opsx:archive, so the Output section should not always end with /opsx:continue.
Suggested fix
- - Prompt: "Run `/opsx:continue` to create the next artifact"
+ - Prompt: If `isComplete: true`, show `/opsx:apply` or `/opsx:archive`; otherwise, "Run `/opsx:continue` to create the next artifact"Also applies to: 83-88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/commands/opsx/continue.md around lines 41 - 45, The Output section
in continue.md is currently unconditional, but the `isComplete: true` branch
already handles the final handoff with `/opsx:apply` or `/opsx:archive`. Update
the prompt flow around the `isComplete` handling so the closing instruction is
only shown when artifacts are incomplete, and keep the completion branch in the
`isComplete: true` path as the only final stop message. Use the existing
`isComplete` check and the final output/suggestion text to locate the
conditional prompt text.
Comment on lines
+57
to
+72
| ``` | ||
| ┌─────────────────────────────────────────┐ | ||
| │ Use ASCII diagrams liberally │ | ||
| ├─────────────────────────────────────────┤ | ||
| │ │ | ||
| │ ┌────────┐ ┌────────┐ │ | ||
| │ │ State │────────▶│ State │ │ | ||
| │ │ A │ │ B │ │ | ||
| │ └────────┘ └────────┘ │ | ||
| │ │ | ||
| │ System diagrams, state machines, │ | ||
| │ data flows, architecture sketches, │ | ||
| │ dependency graphs, comparison tables │ | ||
| │ │ | ||
| └─────────────────────────────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a text fence for the diagram block.
This is non-code content, so the Markdown docs rule requires ```text here instead of a bare fence.
Suggested fix
-```
+```text
┌─────────────────────────────────────────┐
│ Use ASCII diagrams liberally │
├─────────────────────────────────────────┤
@@
└─────────────────────────────────────────┘
-```
+```🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/commands/opsx/explore.md around lines 57 - 72, The ASCII diagram
block in explore.md is using a bare fenced block, which violates the Markdown
docs rule for non-code content. Update the diagram section to use a text fence
with the existing diagram content preserved, and verify the surrounding markdown
still renders correctly in the opsx explore docs.
Source: Coding guidelines
Comment on lines
+1
to
+90
| { | ||
| "schema": "FA(2)", | ||
| "page": { "size": "A4", "margins": [40, 40, 40, 50] }, | ||
| "styles": { | ||
| "title": { "fontSize": 20, "bold": true }, | ||
| "h2": { "fontSize": 10, "bold": true, "margin": [0, 10, 0, 3], "color": "#333333" }, | ||
| "muted": { "color": "#666666", "fontSize": 8 }, | ||
| "offline": { "color": "#b00020", "bold": true }, | ||
| "footerNote": { "fontSize": 7, "color": "#999999" } | ||
| }, | ||
| "blocks": [ | ||
| { "type": "header", "logo": "opts.logo", "title": { "label": "invoice" }, "number": "Fa.P_2", "date": "Fa.P_1" }, | ||
| { | ||
| "type": "columns", | ||
| "when": "hasKsefNumber", | ||
| "columns": [ | ||
| { "type": "text", "label": "ksefNumber", "style": "muted" }, | ||
| { "type": "text", "path": "opts.ksefNumber", "style": "muted" } | ||
| ] | ||
| }, | ||
| { "type": "text", "label": "offline", "when": "offline", "style": "offline" }, | ||
| { "type": "divider" }, | ||
| { "type": "spacer", "height": 6 }, | ||
| { | ||
| "type": "parties", | ||
| "left": { | ||
| "label": "seller", | ||
| "fields": [ | ||
| "Podmiot1.DaneIdentyfikacyjne.Nazwa", | ||
| "Podmiot1.DaneIdentyfikacyjne.NIP", | ||
| "Podmiot1.Adres.AdresL1", | ||
| "Podmiot1.Adres.AdresL2" | ||
| ] | ||
| }, | ||
| "right": { | ||
| "label": "buyer", | ||
| "fields": [ | ||
| "Podmiot2.DaneIdentyfikacyjne.Nazwa", | ||
| "Podmiot2.DaneIdentyfikacyjne.NIP", | ||
| "Podmiot2.Adres.AdresL1", | ||
| "Podmiot2.Adres.AdresL2" | ||
| ] | ||
| } | ||
| }, | ||
| { "type": "spacer", "height": 12 }, | ||
| { | ||
| "type": "lines", | ||
| "from": "Fa.FaWiersz", | ||
| "columns": [ | ||
| { "label": "lp", "path": "NrWierszaFa" }, | ||
| { "label": "name", "path": "P_7" }, | ||
| { "label": "unit", "path": "P_8A" }, | ||
| { "label": "qty", "path": "P_8B", "format": "number" }, | ||
| { "label": "unitPrice", "path": "P_9A", "format": "money" }, | ||
| { "label": "vatRate", "path": "P_12" }, | ||
| { "label": "net", "path": "P_11", "format": "money" } | ||
| ] | ||
| }, | ||
| { "type": "spacer", "height": 10 }, | ||
| { | ||
| "type": "totals", | ||
| "rows": [ | ||
| { "label": "totalNet", "path": "Fa.P_13_1", "format": "money" }, | ||
| { "label": "totalVat", "path": "Fa.P_14_1", "format": "money" }, | ||
| { "label": "totalDue", "path": "Fa.P_15", "format": "money" } | ||
| ] | ||
| }, | ||
| { "type": "spacer", "height": 10 }, | ||
| { | ||
| "type": "payment", | ||
| "when": "Fa.Platnosc", | ||
| "rows": [ | ||
| { "label": "paid", "path": "Fa.Platnosc.Zaplacono" }, | ||
| { "label": "paymentDate", "path": "Fa.Platnosc.TerminPlatnosci.Termin", "format": "date" }, | ||
| { "label": "paymentMethod", "path": "Fa.Platnosc.FormaPlatnosci", "format": "paymentForm" } | ||
| ], | ||
| "accounts": { | ||
| "from": "Fa.Platnosc.RachunekBankowy", | ||
| "heading": "bankAccounts", | ||
| "fields": [ | ||
| { "label": "bankAccount", "path": "NrRB" }, | ||
| { "label": "swift", "path": "SWIFT" }, | ||
| { "label": "bankName", "path": "NazwaBanku" } | ||
| ] | ||
| } | ||
| }, | ||
| { "type": "qr", "when": "qr", "fit": 90 }, | ||
| { "type": "footer", "text": "ksef-client-ts", "style": "footerNote" } | ||
| ] | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider deriving shared block layout to avoid duplicating with fa3-default.json.
This template's blocks/styles are byte-identical to fa3-default.json except for the schema field. A shared base object (spread with a schema override) would remove this drift risk going forward.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ksef-client-ts/src/pdf/template/builtin/fa2-default.json` around
lines 1 - 90, The FA(2) template definition duplicates the same `blocks` and
`styles` used in `fa3-default.json`, leaving only `schema` different. Refactor
this JSON setup so the shared layout lives in one base object and
`fa2-default.json` only overrides `schema` while reusing the common template
structure, using the existing template config symbols like `blocks`, `styles`,
and `schema` to keep both FA templates in sync.
Comment on lines
+61
to
+67
| "type": "totals", | ||
| "rows": [ | ||
| { "label": "totalNet", "path": "Fa.P_13_1", "format": "money" }, | ||
| { "label": "totalVat", "path": "Fa.P_14_1", "format": "money" }, | ||
| { "label": "totalDue", "path": "Fa.P_15", "format": "money" } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the FA(2) XSD to confirm the full set of P_13_x/P_14_x total fields
fd -e xsd . packages/ksef-client-ts/docs/schemas/FA | xargs rg -n 'P_13_|P_14_' -A1 2>/dev/null | head -80Repository: Flopsstuff/ksef-client-ts
Length of output: 6639
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the FA2 default PDF template and the totals renderer
ast-grep outline packages/ksef-client-ts/src/pdf/template/builtin/fa2-default.json || true
rg -n '"type": "totals"|totals\.rows|P_13_1|P_14_1|P_13_2|P_14_2' packages/ksef-client-ts/src/packages/ksef-client-ts/src || trueRepository: Flopsstuff/ksef-client-ts
Length of output: 339
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the FA2 default PDF template and the totals renderer
wc -l packages/ksef-client-ts/src/pdf/template/builtin/fa2-default.json
cat -n packages/ksef-client-ts/src/pdf/template/builtin/fa2-default.json | sed -n '1,140p'
# Find where totals blocks are interpreted
rg -n '"type": "totals"|totals\.rows|P_13_1|P_14_1|P_13_2|P_14_2' packages/ksef-client-ts/srcRepository: Flopsstuff/ksef-client-ts
Length of output: 7303
Include every FA(2) VAT-rate group in the totals block
totals.rows only maps Fa.P_13_1 / Fa.P_14_1, so multi-rate invoices will omit the remaining VAT breakdowns from the PDF. Fa.P_15 is fine, but the totals section is incomplete for invoices with more than one rate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ksef-client-ts/src/pdf/template/builtin/fa2-default.json` around
lines 61 - 67, The totals block in fa2-default.json is incomplete because it
only includes the first FA(2) VAT-rate group fields, so expand the totals.rows
mapping to cover every VAT-rate breakdown present in the invoice data while
keeping Fa.P_15 unchanged. Update the totals template entries in the builtin PDF
template so the rows reference all relevant Fa.P_13_* and Fa.P_14_* symbols used
for multi-rate invoices, preserving the existing totals structure and labels.
Comment on lines
+104
to
+110
| divider: (block) => { | ||
| const b = block as import('./dsl.js').DividerBlock; | ||
| return withStyle( | ||
| { canvas: [{ type: 'line', x1: 0, y1: 0, x2: 515, y2: 0, lineWidth: 0.5, lineColor: '#cccccc' }] }, | ||
| b.style, | ||
| ); | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
Divider width is hardcoded, ignoring configurable page margins.
x2: 515 assumes A4 with 40pt margins on both sides (595 − 40 − 40 = 515). RenderContext has no page-width field, so if a template sets page.margins/page.size differently, the divider line will no longer span the actual content width. Since PageConfig is already a first-class DSL concept (dsl.ts Lines 19-23), threading an effective content width into RenderContext would make this correct for arbitrary templates.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ksef-client-ts/src/pdf/template/interpret.ts` around lines 104 -
110, The divider rendering in interpret.ts currently hardcodes the line length
in divider() to a fixed width, which ignores page size and margins. Update
RenderContext to carry an effective content width derived from PageConfig, then
use that value in the divider block’s canvas line so the line spans the actual
printable area for any template. Keep the change localized around interpret.ts,
RenderContext, and the DividerBlock handling in dsl.ts.
Comment on lines
+4
to
+46
| describe('resolveLabel', () => { | ||
| it('resolves a Polish label from the pl bundle', () => { | ||
| expect(resolveLabel('seller', 'pl')).toBe('Sprzedawca'); | ||
| }); | ||
|
|
||
| it('resolves an English label from the en bundle', () => { | ||
| expect(resolveLabel('seller', 'en')).toBe('Seller'); | ||
| }); | ||
|
|
||
| it('joins pl and en with the default " / " separator for pl+en', () => { | ||
| expect(resolveLabel('seller', 'pl+en')).toBe('Sprzedawca / Seller'); | ||
| }); | ||
|
|
||
| it('honours a custom bilingual separator', () => { | ||
| expect(resolveLabel('seller', 'pl+en', { bilingualSeparator: '\n' })).toBe( | ||
| 'Sprzedawca\nSeller', | ||
| ); | ||
| }); | ||
|
|
||
| it('lets an override win over the bundle (pl)', () => { | ||
| expect(resolveLabel('seller', 'pl', { overrides: { seller: 'Wystawca' } })).toBe('Wystawca'); | ||
| }); | ||
|
|
||
| it('lets an override win over the bundle (en)', () => { | ||
| expect(resolveLabel('seller', 'en', { overrides: { seller: 'Vendor' } })).toBe('Vendor'); | ||
| }); | ||
|
|
||
| it('applies overrides to both halves of a bilingual label', () => { | ||
| expect( | ||
| resolveLabel('seller', 'pl+en', { overrides: { seller: 'X' } }), | ||
| ).toBe('X / X'); | ||
| }); | ||
|
|
||
| it('falls back to the raw key when the key is unknown in every bundle', () => { | ||
| expect(resolveLabel('totally-unknown-key', 'en')).toBe('totally-unknown-key'); | ||
| }); | ||
|
|
||
| it('falls back to the raw key for a bilingual unknown key on both sides', () => { | ||
| expect(resolveLabel('totally-unknown-key', 'pl+en')).toBe( | ||
| 'totally-unknown-key / totally-unknown-key', | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Missing regression test for the Polish-fallback branch.
The suite covers override precedence, bilingual join, and the fully-unknown-key fallback (pl[key] ?? key where both sides are undefined), but never exercises the middle case in resolveOne — a key present in the pl bundle but absent from en — which is the actual "falls back to Polish" behavior the module docstring promises. Since pl/en are currently kept in perfect sync, this branch is effectively untested and would only surface as a bug once the bundles drift (e.g., a new label added to pl but forgotten in en).
Consider adding a test that stubs/overrides an en-side gap, or at minimum documents that this branch relies on bundle parity being manually maintained.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ksef-client-ts/tests/unit/pdf/i18n.test.ts` around lines 4 - 46, The
resolveLabel test suite is missing coverage for the Polish-fallback path in
resolveOne, where a key exists in the pl bundle but is absent from en. Add a
regression test in resolveLabel that simulates an en-side gap (via
stubbing/overrides or a mocked bundle entry) and asserts the function returns
the Polish label instead of the raw key; use the existing resolveLabel helper
and its pl/en bundle lookup behavior to locate the branch.
Comment on lines
+14
to
+15
| // pdfmake is an optional peer — never bundle it; it is loaded lazily at runtime. | ||
| external: ["pdfmake"], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "pdfmake" packages/ksef-client-ts/src/pdf packages/ksef-client-ts/src/cliRepository: Flopsstuff/ksef-client-ts
Length of output: 4603
🌐 Web query:
esbuild external exact package name subpath behavior pdfmake external "pdfmake/build/pdfmake.js"
💡 Result:
In esbuild, the external option supports two types of matching: exact string matching and pattern matching [1][2]. 1. Exact Matching: If you provide an exact string (e.g., "pdfmake/build/pdfmake.js"), esbuild will only mark that specific import path as external [1][2]. Imports to other paths, such as "pdfmake" or "pdfmake/some-other-file.js", will not be affected by this rule. 2. Pattern Matching: If you want to mark a package and all of its subpaths as external, you must use a pattern containing an asterisk (e.g., "pdfmake/"). Any import path that starts with the prefix and ends with the suffix of the pattern will be marked as external [1][2]. Regarding your specific case for pdfmake: - If you use --external:pdfmake/build/pdfmake.js, only that exact subpath import will be excluded from the bundle [1][2]. - If you intend for all imports from the pdfmake package to be external, you should use --external:pdfmake/ instead [1][2]. Esbuild determines whether an import is external by checking the import path against these configured rules after path resolution (like alias or tsconfig paths) is complete [3][4]. If you require more complex logic—such as marking everything in a package as external except for a specific subpath—the standard external configuration is not sufficient [5]. In such cases, you must implement a custom plugin using the onResolve hook to manually handle the filtering logic [6][4][7].
Citations:
- 1: https://github.com/evanw/esbuild/blob/d6427c91/internal/resolver/resolver.go
- 2: https://github.com/evanw/esbuild/blob/0a8d2f9cf0ebd32c2ec93e5bd1538cbb2ce148de/internal/resolver/resolver.go
- 3: https://esbuild.github.io/api/
- 4: --packages=external does not work in conjunction with typescript path aliases evanw/esbuild#2792
- 5: exclude paths from external[] evanw/esbuild#3442
- 6: Example of using
externalwith a filepath? evanw/esbuild#1649 - 7: external doesn't work with typescript's paths nor package's import evanw/esbuild#2803
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## tsup.config.ts\n'
cat -n packages/ksef-client-ts/tsup.config.ts
printf '\n## pdf/fonts.ts (relevant lines)\n'
sed -n '1,160p' packages/ksef-client-ts/src/pdf/fonts.ts
printf '\n## package.json dependency hints\n'
sed -n '1,220p' packages/ksef-client-ts/package.jsonRepository: Flopsstuff/ksef-client-ts
Length of output: 9409
🌐 Web query:
esbuild external package name subpaths official docs react/jsx-runtime external exact string
💡 Result:
In esbuild, the external option is used to mark modules as external, meaning they are excluded from the bundle and left as imports to be evaluated at runtime [1][2]. Regarding your query about subpaths like react/jsx-runtime: 1. Exact String Matching: The external option requires exact string matching for the package or path provided [1][3]. If you mark react as external, it does not automatically externalize its subpaths (e.g., react/jsx-runtime) [4]. You must explicitly list both the main package and any specific subpaths you wish to exclude if they are imported separately [4]. 2. Handling Subpaths: If you want to exclude react/jsx-runtime, you should include it in your external configuration, for example: external: ['react', 'react/jsx-runtime'] [4]. 3. Advanced Control via Plugins: If you need more complex behavior, such as dynamically determining whether a subpath should be externalized, the built-in external option is not sufficient [4]. In such cases, you should use the esbuild Plugin API [4][5]. By using an onResolve hook, you can intercept specific imports and mark them as external programmatically [5]. For command-line usage, you can provide multiple external flags by repeating the option: --external:react --external:react/jsx-runtime [6].
Citations:
- 1: Example of using
externalwith a filepath? evanw/esbuild#1649 - 2: When using esbuild with external react I get Dynamic require of “react” is not supported evanw/esbuild#1454
- 3: external doesn't work with typescript's paths nor package's import evanw/esbuild#2803
- 4: Bundle react/jsx-runtime even if react is listed as external evanw/esbuild#2947
- 5: The right way to modify bundle output evanw/esbuild#2791
- 6: How to specific multiple external module by command arguments? evanw/esbuild#2438
Keep the pdfmake subpaths external too. external: ["pdfmake"] does not cover pdfmake/build/pdfmake.js or pdfmake/build/vfs_fonts.js, so tsup can still bundle the lazy PDF loader’s imports. Add those subpaths, or use pdfmake/*, to preserve the optional-peer contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ksef-client-ts/tsup.config.ts` around lines 14 - 15, The tsup
externalization for pdfmake is too narrow, so the lazy PDF loader can still get
bundled through subpath imports. Update the tsup.config.ts external setting near
the pdfmake entry to also match pdfmake subpaths used by the runtime loader,
referencing the existing pdfmake external configuration, so imports like the
build/pdfmake.js and build/vfs_fonts.js variants remain external as well.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation