feat(editor): expose exportFloorplanPdf and add structure+utility export scope - #636
feat(editor): expose exportFloorplanPdf and add structure+utility export scope#636SomSamantray wants to merge 6 commits into
Conversation
Aymericr
left a comment
Aymericr
left a comment
There was a problem hiding this comment.
Thanks for picking up #619 — the direction is right and the refactor is clean. I checked both call sites and the predicate is genuinely behavior-preserving for 'full' and 'structure', and the package-entry re-export has direct precedent in exportSceneToGlb. I also confirmed your apps/editor assertion is meaningful rather than decorative: exportFloorplanPdf really is undefined on the current entry, so it fails today and passes with your change. Three things before I can merge.
1. Drop docs/residual-review-findings/feat-floorplan-export-routing-scope.md. This repo has no top-level docs/ tree — durable knowledge lives in wiki/. The file is review-run bookkeeping that cites /tmp/compound-engineering-501/... and a docs/plans/2026-08-11-001-...md that doesn't exist here, so it can't ship publicly. Please keep that context in the PR description instead.
2. 'routing' needs a trigger, or should be deferred. Right now no user in this repo can select it, which is your own #632. Either add the button in settings-panel/index.tsx next to the existing two, or split this into just the re-export — that alone closes half of #619 and I'd merge it quickly. On naming: we now also have PrintContentScope = 'structure' | 'everything' from #701, so 'routing' would be a third scope vocabulary. I'd lean 'services' or 'mep', and I'd rather settle that before it's a published type.
3. CI hasn't actually run. As a first-time contributor the Actions suite was sitting at action_required — only Cursor Bugbot has reported. I've approved the run, but the test/type-check/build results in your description are currently unverified.
Smaller things: please rebase (base is ~2 weeks behind main); rename the predicate's first parameter from node to definition, since it receives a NodeDefinition and the node = (category) => ({ category }) test helper reinforces the wrong model; and the new test's comment calls itself compile-time when it's a runtime import that pulls the whole editor barrel into what was a server-only bun test lib suite — it works, just describe it accurately.
One more, on process: #632, #633, #634 and #635 should have been comments here, not upstream issues. #633 and #634 don't survive verification (drawingType is pinned to its only inhabitable value, so nothing is mis-delivered; and the padding is derived from the larger dimension and applied symmetrically, so thin geometry gets more short-axis padding, while the viewport is already measured post-filter). #631 was the one real pre-existing bug and I've kept it, reframed around the shipped 'structure' scope. Filing bot review residuals against the upstream tracker costs maintainer time — please keep them in the PR.
Add a third FloorplanExportScope value that includes structure and utility nodes (ducts, pipes, HVAC) without furniture, shared through a pure isFloorplanNodeInExportScope predicate used at both collection filter sites. Existing 'full' and 'structure' behavior is unchanged. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
Re-export exportFloorplanPdf and the FloorplanExportScope type from @pascal-app/editor so hosts with their own export UI can trigger a floorplan PDF export without reaching into the settings panel. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
…t comment Per code review, add a consumer-side compile-time assertion in apps/editor that imports exportFloorplanPdf and FloorplanExportScope from the @pascal-app/editor package entry, so a broken re-export fails check-types instead of passing silently. Also drop the WHAT-narration doc comment on collectFloorplanGeometry (behavior-preserving). Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
Per review on pascalorg#636: the 'routing' scope has no UI trigger anywhere (pascalorg#632) and its name would collide with PrintContentScope's vocabulary from pascalorg#701. Narrows FloorplanExportScope back to 'full' | 'structure' and simplifies the predicate accordingly. 'full' and 'structure' behavior is unchanged. Also renames the predicate's first parameter from `node` to `definition` (it receives a NodeDefinition, not a node instance) and corrects the apps/editor smoke test's comment, which mis-described a runtime barrel import as a compile-time check.
This repo keeps durable docs in wiki/, not a top-level docs/ tree, and the file cited paths that don't exist here (a /tmp/... run path and a docs/plans/... plan file). The two facts worth keeping — the tracker issues filed from the original review round, and that pascalorg#631 is the one confirmed-real finding — move into the PR description instead.
40a1543 to
f730e4f
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f730e4f. Configure here.
| ): boolean { | ||
| if (scope === 'full') return true | ||
| return definition?.category === 'structure' | ||
| } |
There was a problem hiding this comment.
Routing export scope never added
Medium Severity
FloorplanExportScope and isFloorplanNodeInExportScope still only distinguish full and structure. Hosts cannot request the advertised routing sheet that keeps structure and utility nodes (ducts, pipes, HVAC) while dropping furniture.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit f730e4f. Configure here.
There was a problem hiding this comment.
This is expected, not a regression: the 'routing' scope was intentionally dropped from this PR per @Aymericr's review (comment above) — it had no UI trigger anywhere in the tree (#632) and its name would have collided with PrintContentScope from #701. It's deferred to a follow-up PR with a UI trigger and a non-colliding name ('services'/'mep' suggested). See the updated PR description for the full context.
2 participants
What does this PR do?
Fixes the gap from #619 that blocked host apps from shipping their own floorplan export UI:
exportFloorplanPdfis now exported from@pascal-app/editor, so hosts can trigger a PDF export directly instead of only through the built-in Settings panel.Update: this PR originally also added a
'routing'export scope (structure + utility nodes, for electrical/engineering sheets). Per review feedback, that scope has been dropped from this PR — it had no UI trigger anywhere in the tree (tracked separately as #632), and its name would have collided with the unrelatedPrintContentScopevocabulary introduced by #701. It can come back in a follow-up PR once it has a UI trigger and a name that doesn't collide ('services'/'mep'were suggested).'full'and'structure'scope behavior is unchanged throughout.The duplicated inline
scope === 'full' || category === 'structure'check at both floorplan-geometry collection sites is now a single shared predicate,isFloorplanNodeInExportScope.How to test
bun test packages/editor/src/lib/floorplan— 53 pass, includingisFloorplanNodeInExportScopecases for both scopes across categories.bun run --cwd apps/editor check-typesandbun run --cwd packages/editor check-types— pass, including a consumer-side import assertion guarding theexportFloorplanPdfre-export.Screenshots / screen recording
Not applicable — library API change with no visual difference.
Checklist
bun devbun checkto verify)mainbranchRelated: #619, #632
Review round context
The original review round on this PR filed tracker issues #631–#635 and flagged a stray
docs/residual-review-findings/...file that referenced paths not present in this repo (that file has been removed). Of those:'structure'scope.drawingTypeis pinned to its only inhabitable value (nothing is mis-delivered), and the padding is derived from the larger dimension and applied symmetrically (thin geometry gets more short-axis padding as intended).'routing'scope was deferred out of this PR, as above.isFloorplanNodeInExportScopetest suite in this PR.Note
Low Risk
Barrel re-export plus a behavior-preserving refactor of export node filtering, covered by new unit and consumer import tests.
Overview
Public API:
exportFloorplanPdfandFloorplanExportScope('full' | 'structure') are now re-exported from@pascal-app/editor, so host apps can trigger floorplan PDF export without reaching into internal modules. Anapps/editorsmoke test imports the barrel so a dropped re-export fails at type-check/test time.Export scope logic: Inline
(scope === 'full' || category === 'structure')checks in floorplan geometry collection (level tree and linked-level nodes) are replaced by a shared pure helperisFloorplanNodeInExportScope.'full'still includes every node;'structure'still onlystructure-category nodes (utility, furnish, etc. stay out). New unit tests lock that matrix down.Note: This diff does not add a
'routing'scope or changeFloorplanExportScopebeyond what was already'full' | 'structure'.Reviewed by Cursor Bugbot for commit f730e4f. Bugbot is set up for automated code reviews on this repo. Configure here.