Add CNC machine export templates and extend coverage#528
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe pull request adds machine file generation to the CNC export pipeline. A new Changes
Sequence DiagramsequenceDiagram
participant Client
participant CNCService as CNC Service
participant Templates as Template Module
participant Jinja as Jinja2
Client->>CNCService: export_cnc(panels)
activate CNCService
CNCService->>CNCService: _build_machine_files(panels)
activate CNCService
rect rgb(230, 245, 255)
note over CNCService,Jinja2: Generate three formats
CNCService->>Templates: render_cix_program(panels)
Templates->>Jinja: render(program.cix.jinja, {panels})
Jinja-->>Templates: CIX program string
Templates-->>CNCService: rendered CIX
CNCService->>Templates: render_xxl_program(panels)
Templates->>Jinja: render(program.xxl.jinja, {panels})
Jinja-->>Templates: XXL program string
Templates-->>CNCService: rendered XXL
CNCService->>Templates: render_mpr_program(panels)
Templates->>Jinja: render(program.mpr.jinja, {panels})
Jinja-->>Templates: MPR program string
Templates-->>CNCService: rendered MPR
end
rect rgb(230, 255, 230)
note over CNCService: Create artifacts
CNCService->>CNCService: encode to UTF-8 streams
CNCService->>CNCService: create MachineFileArtifact objects
CNCService-->>CNCService: {cix: Artifact, xxl: Artifact, mpr: Artifact}
end
deactivate CNCService
CNCService-->>Client: CNCExportArtifacts with machines field
deactivate CNCService
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes introduce cohesive new functionality across multiple file types (service, module, templates, tests, frontend types). Review complexity stems from understanding template rendering setup, validating format specifications across three CNC program types, and verifying integration with the export pipeline. However, individual changes are relatively straightforward (template syntax, dataclass definition, type additions) with consistent patterns repeated across the three formats. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| from pathlib import Path | ||
| from typing import Sequence | ||
|
|
||
| from jinja2 import Environment, FileSystemLoader | ||
|
|
There was a problem hiding this comment.
The new CNC template helpers import jinja2 to render machine programs, but the backend’s pyproject.toml still does not list Jinja2 in the main dependency set (it only appears in the optional docs group). When the backend is installed in a production environment that only installs [project] dependencies, this import will raise ModuleNotFoundError and the CNC export path will fail at startup. Please add Jinja2 to the runtime dependencies or otherwise ensure it is installed wherever the service runs.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
backend/api/exports.py(2 hunks)backend/api/schemas.py(2 hunks)backend/services/cnc_service.py(4 hunks)backend/services/cnc_templates/__init__.py(1 hunks)backend/services/cnc_templates/templates/program.cix.jinja(1 hunks)backend/services/cnc_templates/templates/program.mpr.jinja(1 hunks)backend/services/cnc_templates/templates/program.xxl.jinja(1 hunks)backend/tests/test_cnc_service.py(1 hunks)backend/tests/test_cnc_templates.py(1 hunks)backend/tests/test_routes_quote_cnc.py(1 hunks)frontend/src/api/client.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
frontend/src/api/client.ts (1)
backend/api/schemas.py (1)
StoredArtifact(100-104)
backend/api/schemas.py (1)
frontend/src/api/client.ts (1)
StoredArtifact(5-10)
backend/services/cnc_templates/__init__.py (1)
backend/api/schemas.py (1)
CNCPanel(90-97)
backend/api/exports.py (2)
backend/api/schemas.py (1)
StoredArtifact(100-104)frontend/src/api/client.ts (1)
StoredArtifact(5-10)
backend/tests/test_cnc_templates.py (2)
backend/api/schemas.py (1)
CNCPanel(90-97)backend/services/cnc_templates/__init__.py (3)
render_cix_program(27-30)render_mpr_program(39-42)render_xxl_program(33-36)
backend/services/cnc_service.py (3)
backend/api/schemas.py (1)
CNCPanel(90-97)backend/services/cnc_templates/__init__.py (3)
render_cix_program(27-30)render_mpr_program(39-42)render_xxl_program(33-36)services/cad-service/nesting/engine.py (1)
NestingResult(32-38)
🔇 Additional comments (17)
backend/services/cnc_templates/templates/program.xxl.jinja (2)
1-4: LGTM: Header section follows XXL format conventions.The header correctly defines VERSION, UNITS, and derives PANELCOUNT from the panels collection.
6-9: Verify edge_band field handling in XXL format.The CNCPanel schema includes an optional edge_band field, but it's not rendered in this template. Please confirm whether:
- XXL format specification doesn't support edge banding information, or
- Edge band should be included in the output format.
If edge_band is required, consider appending it to the PART line.
backend/services/cnc_templates/templates/program.cix.jinja (2)
1-5: Verify hardcoded MATERIAL=DEFAULT in global header.The MAINDATA section sets MATERIAL=DEFAULT while individual panels specify their own materials in PARAM,MATERIAL. Please confirm whether:
- The CIX format requires a global default material declaration, or
- This should be removed or set to a meaningful aggregate value.
If DEFAULT is a required placeholder per the CIX specification, consider adding a comment explaining this.
7-15: LGTM: MACRO block structure follows CIX conventions.The RECT macro blocks correctly emit panel dimensions and material. Note that edge_band is not included, consistent with the XXL template.
backend/api/schemas.py (1)
10-10: LGTM: Schema correctly extended with machines field.The addition of Dict import and the machines field to CNCExportResponse properly supports the new machine-specific artifacts feature. Type annotation aligns with the frontend interface.
Also applies to: 131-131
backend/services/cnc_templates/templates/program.mpr.jinja (1)
1-8: LGTM: MPR template structure is correct and consistent.The template properly defines the MPR header and emits PART records with formatted dimensions. Edge band handling is consistent with the other machine format templates.
frontend/src/api/client.ts (1)
41-41: LGTM: Frontend type correctly mirrors backend schema.The machines field type Record<string, StoredArtifact> properly corresponds to the backend's Dict[str, StoredArtifact].
backend/tests/test_cnc_templates.py (1)
25-41: LGTM: Tests provide adequate coverage of core template functionality.The tests correctly validate that each renderer produces output containing the expected format markers and panel identifiers. While more detailed assertions (e.g., checking dimensions, material values) could be added, the current coverage is sufficient for initial validation.
backend/api/exports.py (2)
101-110: LGTM! Machine file upload follows established patterns.The implementation correctly mirrors the existing CSV and SVG upload logic, with proper stream positioning and conversion to StoredArtifact.
132-132: LGTM! Response properly includes machine artifacts.The machines field is correctly populated with the uploaded machine artifacts and included in the response.
backend/services/cnc_templates/__init__.py (3)
14-19: LGTM! Jinja2 environment appropriately configured for CNC program generation.The use of
autoescape=Falseis correct for generating non-HTML machine programs, andtrim_blocks/lstrip_blockswill produce cleaner output. Since panel data originates from validated Pydantic models rather than untrusted user input, the disabled autoescaping does not introduce security concerns.
22-24: LGTM! Clean abstraction for template rendering.The helper function eliminates duplication across the three render functions. Template loading errors will propagate naturally from Jinja2 with clear error messages.
27-42: LGTM! Render functions follow consistent pattern.All three render functions are implemented identically with clear naming conventions for templates. The implementation is straightforward and maintainable.
backend/services/cnc_service.py (4)
13-13: LGTM! Imports properly support new functionality.The typing imports and template renderer imports are correctly added to support machine file generation.
Also applies to: 16-16
141-147: LGTM! Well-designed dataclass for machine file artifacts.The
MachineFileArtifactdataclass is appropriately structured with immutability (frozen=True) and clear field definitions for representing machine-specific program files.
155-155: LGTM! Machine files field properly typed and integrated.The
machine_filesfield is correctly typed as a dictionary mapping format names toMachineFileArtifactinstances, consistent with the dataclass structure.
241-241: LGTM! Machine files properly integrated into export workflow.The machine files are generated at the appropriate point in the export process and correctly included in the returned artifacts.
Also applies to: 248-248
| def _sample_panels() -> list[CNCPanel]: | ||
| return [ | ||
| CNCPanel( | ||
| panel_id="MOD1-P001", | ||
| module_id="MOD1", | ||
| module_name="Base", | ||
| width=600.0, | ||
| height=720.0, | ||
| material="Walnut", | ||
| edge_band="ABS-1mm", | ||
| ), | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider verifying edge_band handling in tests.
The sample panel includes edge_band="ABS-1mm" but none of the tests verify whether edge banding appears in the rendered output. If edge banding should be included in the machine formats, add assertions to validate it; otherwise, consider removing it from the sample data to avoid confusion.
🤖 Prompt for AI Agents
In backend/tests/test_cnc_templates.py around lines 11 to 22, the sample panel
includes edge_band="ABS-1mm" but tests do not assert whether edge banding is
rendered; update the tests to either (a) add assertions that the rendered
machine output includes the expected edge band string/attribute for this sample
panel (match exact token or xml/format field used by the renderer), or (b) if
edge banding is irrelevant for machine formats, remove the edge_band field from
the _sample_panels() fixture to avoid confusion; ensure whichever change you
choose keeps other tests' expectations consistent and update any fixtures or
expected outputs accordingly.
| assert set(cnc["machines"]) == {"cix", "xxl", "mpr"} | ||
| for fmt, ext in ("cix", "cix"), ("xxl", "xxl"), ("mpr", "mpr"): | ||
| machine = cnc["machines"][fmt] | ||
| assert machine["bucket"] | ||
| assert machine["key"].endswith(f".{ext}") | ||
| assert machine["url"].startswith("memory://") | ||
| assert machine["content_type"] == "text/plain" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Simplify the machine format iteration loop.
The loop on line 149 uses fmt and ext variables that are always identical. This can be simplified for better readability.
Apply this diff to simplify:
- for fmt, ext in ("cix", "cix"), ("xxl", "xxl"), ("mpr", "mpr"):
+ for fmt in ("cix", "xxl", "mpr"):
machine = cnc["machines"][fmt]
assert machine["bucket"]
- assert machine["key"].endswith(f".{ext}")
+ assert machine["key"].endswith(f".{fmt}")
assert machine["url"].startswith("memory://")
assert machine["content_type"] == "text/plain"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert set(cnc["machines"]) == {"cix", "xxl", "mpr"} | |
| for fmt, ext in ("cix", "cix"), ("xxl", "xxl"), ("mpr", "mpr"): | |
| machine = cnc["machines"][fmt] | |
| assert machine["bucket"] | |
| assert machine["key"].endswith(f".{ext}") | |
| assert machine["url"].startswith("memory://") | |
| assert machine["content_type"] == "text/plain" | |
| assert set(cnc["machines"]) == {"cix", "xxl", "mpr"} | |
| for fmt in ("cix", "xxl", "mpr"): | |
| machine = cnc["machines"][fmt] | |
| assert machine["bucket"] | |
| assert machine["key"].endswith(f".{fmt}") | |
| assert machine["url"].startswith("memory://") | |
| assert machine["content_type"] == "text/plain" |
🤖 Prompt for AI Agents
In backend/tests/test_routes_quote_cnc.py around lines 148 to 154, the for-loop
currently unpacks identical pairs (fmt, ext) where ext is always the same as
fmt; simplify by iterating a single variable over the machine names (e.g., for
fmt in ("cix", "xxl", "mpr")) and use fmt for both the map lookup and file
extension check, leaving the rest of the assertions unchanged.
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68f7fdec473483309b4f042de515fac4
Summary by CodeRabbit