docs(prd): Asset-Index PDF Export (v0.1) — seeking CodeRabbit + CTO review - #2562
docs(prd): Asset-Index PDF Export (v0.1) — seeking CodeRabbit + CTO review#2562carlosvirreira wants to merge 9 commits into
Conversation
Read-only render-side feature: export the current asset-index view as a workspace-branded PDF via react-to-print, mirroring the booking-overview-pdf and audit-receipt-pdf precedents. Reuses canExportAssets tier gate, AssetIndexSettings columns/showAssetImage, and the existing select-all pattern. No schema change. No new dependency. No PR-0 (real-DB harness) dependency — Suite B is empty by design. Filed as a draft PR to invite CodeRabbit review on the spec itself before implementation work begins. CTO sign-off needed on §14 open questions (MAX_PDF_ROWS cap, page orientation, columns-verbatim reaffirmation, client-rendered architecture).
![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.
Reviewed commit: f8cea51de4
ℹ️ 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".
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughUpdates the Asset Index PDF Export PRD to v0.4 §14 sealed: defines a read-only, render-side printable export (react-to-print) mirroring filters/sort/visible columns and thumbnail toggle, enforces Suite A verifiable assertions including IDOR hardening (A12), and removes the previously proposed PDF row-cap. ChangesAsset Index PDF Export PRD
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
🧹 Nitpick comments (3)
PRD-asset-index-pdf-export.md (3)
275-275: ⚡ Quick winAdd language specifier to fenced code block for event names.
The code block containing event name definitions should specify a language for proper syntax highlighting and documentation clarity. Since these are event name definitions that resemble JavaScript/JSON, consider using
javascriptorjsonas the language specifier.📝 Proposed fix
-``` +```javascript asset_pdf_export_button_clicked { workspace_id, plan, selected_count, include_images }As per coding guidelines, Markdown files should maintain proper documentation format.
🤖 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 `@PRD-asset-index-pdf-export.md` at line 275, The fenced code block that defines the event name asset_pdf_export_button_clicked lacks a language specifier; update the opening fence from ``` to ```javascript (or ```json) so the block reads as a JavaScript/JSON code block and ensures proper syntax highlighting for the event definition (asset_pdf_export_button_clicked { workspace_id, plan, selected_count, include_images }).
186-186: ⚡ Quick winClarify the print-CSS implementation for repeating headers.
Test A7 mentions
thead.repeat-on-printbut this doesn't appear to be a standard CSS class or property. The standard approach for repeating table headers in print is usingdisplay: table-header-groupon<thead>elements (which is the default for thead).Consider clarifying whether:
repeat-on-printis a custom class to be created that applies the necessary styles- The implementation should use the standard browser behavior for
<thead>- This is placeholder text and "or equivalent" covers the implementation details
The "or equivalent" phrase provides flexibility, but more specific guidance would help ensure the test assertion matches the implementation.
🤖 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 `@PRD-asset-index-pdf-export.md` at line 186, Test A7's mention of "thead.repeat-on-print" is ambiguous; update the spec and implementation to explicitly state whether "repeat-on-print" is a custom CSS class or if we rely on standard thead behavior: add a concrete decision and implement/apply it in the printable component (e.g., create a CSS class .repeat-on-print that sets display: table-header-group and any print-specific rules or remove the custom class and rely on the native <thead> rendering), then update the A7 assertion text to match the chosen approach (reference the symbol thead.repeat-on-print or plain <thead> as appropriate) and ensure the test checklist and Playwright/manual PR-2 item reflect that exact implementation.
88-88: ⚡ Quick winAdd language specifier to fenced code block.
For better syntax highlighting and documentation clarity, specify a language for this code block. Since this is a specification/goal condition rather than executable code, consider using
textor leaving the language empty but being explicit about it.📝 Proposed fix
-``` +```text /goal The branch satisfies ALL of the following, with the literal commandAs per coding guidelines, Markdown files should maintain proper documentation format.
🤖 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 `@PRD-asset-index-pdf-export.md` at line 88, Update the fenced code block that begins with the literal line "/goal The branch satisfies ALL of the following, with the literal command" by adding a language specifier (e.g., use ```text) to the opening fence so the block is explicitly marked as non-executable text; locate the fenced block in PRD-asset-index-pdf-export.md and change the opening backticks to include the chosen language specifier.
🤖 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.
Nitpick comments:
In `@PRD-asset-index-pdf-export.md`:
- Line 275: The fenced code block that defines the event name
asset_pdf_export_button_clicked lacks a language specifier; update the opening
fence from ``` to ```javascript (or ```json) so the block reads as a
JavaScript/JSON code block and ensures proper syntax highlighting for the event
definition (asset_pdf_export_button_clicked { workspace_id, plan,
selected_count, include_images }).
- Line 186: Test A7's mention of "thead.repeat-on-print" is ambiguous; update
the spec and implementation to explicitly state whether "repeat-on-print" is a
custom CSS class or if we rely on standard thead behavior: add a concrete
decision and implement/apply it in the printable component (e.g., create a CSS
class .repeat-on-print that sets display: table-header-group and any
print-specific rules or remove the custom class and rely on the native <thead>
rendering), then update the A7 assertion text to match the chosen approach
(reference the symbol thead.repeat-on-print or plain <thead> as appropriate) and
ensure the test checklist and Playwright/manual PR-2 item reflect that exact
implementation.
- Line 88: Update the fenced code block that begins with the literal line "/goal
The branch satisfies ALL of the following, with the literal command" by adding a
language specifier (e.g., use ```text) to the opening fence so the block is
explicitly marked as non-executable text; locate the fenced block in
PRD-asset-index-pdf-export.md and change the opening backticks to include the
chosen language specifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e2be92b-c734-4906-a76a-d378411fd928
📒 Files selected for processing (1)
PRD-asset-index-pdf-export.md
Five findings, all accepted. v0.1 -> v0.2. Codex P2: - Fix turn-cap inconsistency (§4.3 said 80, §5 caps at 60). §4.3 now says 60 and points at §5 as the source. - Resolve A1 vs §6.0 contradiction by introducing selectVisibleColumns() helper as the loader-layer filter+sort owner. A1 tests the helper; new A1b tests the component renders input order verbatim. Clean SoC. CodeRabbit nitpicks (all applied): - A7 thead.repeat-on-print disambiguated -> use native <thead>; assert DOM structure in happy-dom; visual multi-page repeat moved to PR-2 manual checklist (jsdom cannot simulate print pagination). - §5 /goal block + §13 events block: add `text` language fence. Bonus: A2 gains an explicit XSS-regression sub-assertion (was implicit in risk #6, now owned by a test). Changelog in §15.4. Test count: ~22 -> ~24 cases / 12 -> 13 groups.
Review-pass-1 addressed in e6854f5Five findings total, all accepted. v0.1 → v0.2. CodeRabbit (3 nitpicks)
Codex (2 P2 contradictions) — replied inline + resolved
Bonus
Net deltas
Re-review welcome. Once §14 open questions get CTO sign-off and the §4.2 adequacy gate passes, we proceed to the test-suite commit (commit 1, all red) before any implementation. |
|
@coderabbitai full review |
|
@codex review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6854f58c6
ℹ️ 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".
…v0.3
Front-loads the predictable CR/Codex security finding. Per
.claude/rules/org-scope-user-supplied-ids.md ("the original bug: edit
validated, create did not"), any user-supplied entity ID consumed by a
query must be proven org-scoped. The export route takes selection /
search-params from request input, so a cross-org IDOR test is mandatory.
Already enforced by reuse of getAssetsWhereInput({organizationId,
currentSearchParams}); A12 makes the guarantee a test, not an
implementation memory.
- New test A12 (Suite A): cross-org asset ID silently filtered out
- New risk register row #7: cross-org IDOR as named ship-blocker
- Test count: ~24 -> ~25 cases / 13 -> 14 groups
- Changelog: new §15.5
Route tests live in apps/webapp/test/routes-tests/ per established repo convention (see e.g. apps/webapp/test/routes-tests/api.kits.bulk-actions .test.ts). Co-locating *.test.ts inside apps/webapp/app/routes/_layout+/ would conflict with remix-flat-routes path mapping. - §5 goal condition path corrected - §6.1 file path header corrected - Route file path uses the existing [.pdf] bracketing precedent - Surfaced by writing commit 1 of the implementation PR — exactly the kind of small spec-vs-reality drift TDD-backwards is meant to catch before /goal runs.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PRD-asset-index-pdf-export.md (1)
180-182:⚠️ Potential issue | 🟠 Major | ⚡ Quick winChange loader return type to
Promise<Response>to match documented HTML behavior.Lines 180-182 declare
Promise<{ asset: never }>but the loader returns rendered HTML (as documented in test A0 at line 194). The correct Remix type for a loader returning HTML isPromise<Response>. Update the signature to:export async function loader(args: LoaderFunctionArgs): Promise<Response>The type must match the actual return contract so tests and implementation target consistent behavior.
🤖 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 `@PRD-asset-index-pdf-export.md` around lines 180 - 182, The loader function signature currently declares Promise<{ asset: never }>, but the implementation returns rendered HTML; update the declaration of export async function loader(args: LoaderFunctionArgs) to return Promise<Response> so the declared type matches the actual Response return (ensure any related imports/types used with LoaderFunctionArgs remain valid and adjust callers/tests expecting the old shape if needed).
🤖 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 `@PRD-asset-index-pdf-export.md`:
- Line 219: Replace the inconsistent PDF route filename notation so the PRD uses
a single canonical form: choose either `assets.export.$fileName[.pdf].tsx` or
`assets.export.$fileName.pdf.tsx` and update all occurrences (e.g., the entries
currently at lines referencing `assets.export.$fileName[.pdf].tsx` and
`assets.export.$fileName.pdf.tsx`) to that chosen convention to prevent
spec-to-test drift; ensure the README/PRD mentions the canonical pattern once
and update any related examples that reference `assets.export...` accordingly.
---
Outside diff comments:
In `@PRD-asset-index-pdf-export.md`:
- Around line 180-182: The loader function signature currently declares
Promise<{ asset: never }>, but the implementation returns rendered HTML; update
the declaration of export async function loader(args: LoaderFunctionArgs) to
return Promise<Response> so the declared type matches the actual Response return
(ensure any related imports/types used with LoaderFunctionArgs remain valid and
adjust callers/tests expecting the old shape if needed).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55be0061-0f69-4524-972f-cc8458a653a7
📒 Files selected for processing (1)
PRD-asset-index-pdf-export.md
Yes for now I would make it consistent. If they want to print less columns they can be temp disabled in the column picker.
I am not sure why we need this limitation tbh. If we generate the PDF in the same manner we do in the other places, i don't see a reason to do this.
Agreed.
I think for now this is tried and tested so I would use it. If one day we decide to do something else, we can migrate all our pdf generation functionality. |
Nikolay's §14 answers landed; Codex P1 + CR review-pass-2 addressed.
§14 sealed:
- Q1 columns verbatim: confirmed
- Q2 row cap: REMOVED. Nikolay's pushback ("if we generate the PDF in
the same manner we do in the other places, I don't see a reason to do
this") is correct — grep confirms booking-overview-pdf.tsx and
audit-receipt-pdf.tsx have zero MAX/limit/truncate. Dropping A8, the
MAX_PDF_ROWS constant from §6.0, the truncated event, and reframing
risk #2 to match existing PDF precedent.
- Q3 orientation: landscape confirmed
- Q4 react-to-print: confirmed
Codex P1 + CR inline (line 218/219) — filename unification:
- All references now use the canonical [.pdf] bracketed form matching
the existing [.csv] precedent for remix-flat-routes literal extensions.
CR outside-diff (lines 180-182) — loader signature:
- Promise<{asset:never}> -> Promise<Response> (matches the actual HTML
Response contract documented in A0).
Test count: ~25 -> ~22 cases / 14 -> 13 groups. PRD sealed pending CTO
ratification. Impl PR #2564's stub gets MAX_PDF_ROWS + A8 removed next.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PRD-asset-index-pdf-export.md (1)
158-159:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove stale truncation language from the interface contract.
totalRowCountstill mentions a “truncation note”, but v0.4 explicitly removed truncation/cap behavior. This can mislead implementation and tests.Suggested doc fix
- totalRowCount: number; // for the footer count + truncation note + totalRowCount: number; // for the footer count🤖 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 `@PRD-asset-index-pdf-export.md` around lines 158 - 159, The interface comment for totalRowCount is stale—remove the phrase "for the footer count + truncation note" and update the comment to only describe its current purpose (e.g., total number of rows for footer/display) so it no longer refers to truncation behavior; locate the totalRowCount field in the interface declaration and edit its trailing comment to reflect v0.4 semantics (omit any mention of truncation/cap).
🤖 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.
Outside diff comments:
In `@PRD-asset-index-pdf-export.md`:
- Around line 158-159: The interface comment for totalRowCount is stale—remove
the phrase "for the footer count + truncation note" and update the comment to
only describe its current purpose (e.g., total number of rows for
footer/display) so it no longer refers to truncation behavior; locate the
totalRowCount field in the interface declaration and edit its trailing comment
to reflect v0.4 semantics (omit any mention of truncation/cap).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e5cd7132-c06e-47e5-ae40-81b95471b135
📒 Files selected for processing (1)
PRD-asset-index-pdf-export.md
§14 sealed in v0.4 (24d0a17) — all four answers folded inThanks for the calls. Locked in:
Cascaded to impl PR #2564 in commits 9118804 + b40fe9d: §14 now closed (full quotes in PRD §14, change log in §15.6). The §4.2 adequacy gate is the only remaining human checkpoint before |
CR review-pass-2 outside-diff finding addressed in v0.4 (24d0a17)
Accepted. §6.0 loader signature is now |
…omment CR review-pass-3 outside-diff (lines 158-159): the AssetIndexPdfProps.totalRowCount comment still said "for the footer count + truncation note" — stale after v0.4 removed the cap. Updated to "for the footer count (no truncation in v0.4 — see §14 Q2)". Trivial one-line fix; the spec was internally inconsistent without it. Exactly the kind of stale-prose-after-feature-removal that CR catches that humans skim past.
CR review-pass-3 outside-diff finding addressed in v0.4.1 (just pushed)
Accepted. The comment on Per Nikolay: §14 is CTO-ratified as of this round. v0.4.1 is the sealed PRD; only stale-prose corrections from this point. The §4.2 adequacy gate (test suite review in PR #2564) is the only remaining human checkpoint before |
|
@coderabbitai full review |
|
@codex review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d27618488
ℹ️ 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".
Self-§4.2 gate review on PR #2564 commit b40fe9d surfaced 6 findings; applied Option C+ (4 fixed, 1 deferred, 1 skipped, plus process fix). PRD changes: - §4.2 criteria sharpened: security-class tests MUST be behavioral (not structural-proxy); "fails for the right reason" criterion explicitly excludes fixture/accessible-name/cwd-path failures. - §4.2 process: ADDED reviewer-≠-author rule. Self-review structurally compromised; gate requires an independent agent or human pass. - §6.0: pinned "Include thumbnails" as the exact accessible name on the ExportAssetsPdfButton checkbox (closes A3 fuzzy-regex wrong- reason fail). - §15.7: full changelog of the 6 findings + resolutions. Companion test-suite fixes live in PR #2564 (commit cc13401). This is the Musk-algorithm step-1 outcome ("make the requirement less dumb"): the self-review caught an A12 security test that was structural- proxy (would have passed against an impl that doesn't actually exclude foreign-org rows), wrong-reason fails that would have burned /goal turns, and a missing edge case (null thumbnail) common in real high-volume workspaces. Worth the 30-45 minutes. NEXT: an independent agent or human runs §4.2 v2 against PR #2564 commit cc13401, then /goal can be set.
§4.2 adversarial-pass corrections in v0.5 (commit 1662abc) + impl PR #2564 cc13401I ran a §4.2 gate review on my own work (acknowledging the structural conflict — see process correction below). Six findings; Option C+ applied: 4 fixed, 1 deferred + documented, 1 skipped, plus 1 process fix. Fixed (the four that matter at scale)
Deferred (documented in §15.7)
Skipped
Process correction in §4.2Reviewer ≠ author. I wrote both the PRD §6.1 and the test suite, so my self-review was structurally compromised. §4.2 now requires that the gate be run by an agent or human OTHER than the author. The author may run an adversarial first pass (which I just did) but the gate is only "passed" after an independent reviewer signs off. Commits
State now
The next reviewer's job is small: take 10-15 min, read |
Clause (5) used a three-dot `git diff --stat main...HEAD`, which renders the net-new test files as all-additions and so cannot detect assertions weakened after commit-1. Add a two-dot `git diff --numstat <commit-1>..HEAD` check on the two frozen files asserting zero deletions.
2 participants
What this PR is
A draft PRD only (no code) for an "Export PDF" peer to the existing CSV export on the asset index. Filed as a draft PR specifically to invite CodeRabbit and CTO review on the spec before any implementation work begins. Companion to #2538 (Asset Image History PRD).
TL;DR
Export the current asset-index view as a workspace-branded PDF that mirrors the user's chosen columns, order, filters, and selection — using the same select-all-across-pages semantics as CSV export. Visually consistent with the existing
booking-overview-pdf.tsx. Tier-gated identically to CSV (canExportAssets). Asset thumbnails per row by default (toggleable at export), defaulting from the user's existingAssetIndexSettings.showAssetImagepreference.Important asymmetry vs the Image History PRD (#2538)
This is a read-only render-side feature. Suite B (DB-semantic, real-DB-harness-gated) is empty by design. PR-0 (the CTO-owned real-DB test harness) is not a dependency for this feature. The feature
/goalPR is honestly pristine end-to-end. Reviewers: please don't bundle infra into this PRD by reflex.Architecture facts (verified, not hallucinated)
booking-overview-pdf.tsxandaudit-receipt-pdf.tsxusereact-to-print— client-side, browser print → Save-as-PDF. No server-side PDF library exists in the repo, and we do not add one (test A11 statically asserts this).AssetIndexSettings.columnsis a JSON[{name, visible, position}]array (fixed entries + dynamic custom fields). Used verbatim — no inline column picker.canExportAssetsalready exists onTierLimit/CustomTierLimit; no new flag.requirePermission({entity:asset, action:export})at the loader (server-side, never UI-only).CLAUDE.md"Bulk Operations & Select All" — lifted verbatim fromexport-assets-button.tsx.All cited against
~/ShelfDev/shelf-architecture.json(the self-verifying whole-app architecture artifact developed alongside #2538).What's locked
canExportAssets(same as CSV)AssetIndexSettingsverbatim, no pickerAssetIndexSettings.showAssetImage, toggleable at exportreact-to-printonly; no server PDF depprisma validateHALTs the/goalloop on anypackages/database/prisma/diff)Open questions for CTO sign-off (§14)
AssetIndexSettingsverbatim, no inline picker.MAX_PDF_ROWScap value — suggested 500 (browser print sweet spot with thumbnails); CSV remains the answer above that.Key risk flagged
Safari print rendering (per Shelf's standing pattern that print/render jank tends to live in WebKit). Test A7 asserts the print CSS is applied (jsdom/happy-dom); PR-2 carries a manual Safari/WebKit checklist (or Playwright WebKit smoke test) to verify it actually renders right before general release.
What reviewers should focus on
/goalcondition (specifically the schema-must-not-change halt and the no-server-PDF-lib static assertion).What this PR is not
--draftuntil §14 is resolved and CodeRabbit feedback is addressed.Related: #2538 (Asset Image History PRD, the property-nature precedent this PRD inherits).
Summary by CodeRabbit
New Features
Documentation