feat: PDF export and breakdown table improvements#134
Conversation
- Add print styles for dark theme PDF export (A4, proper spacing) - Add "With Financials" / "Without Financials" export dropdown - Show "Hours per Week" label in print mode (non-interactive) - Add Total Price column to breakdown table (Hours × Unit Price) - Make Job Planning Lines and Job Ledger Entry clickable BC links - Separate showCosts (internal) from showPrices (customer-facing) - Add print footer with GetThyme.ai and KnowAll AI links - Hide interactive elements (toggles, navigation) in print - Auto-expand task breakdown rows in print mode Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove Unit Price and Total Price columns from the breakdown table. These were confusing because: - Unit Price display was unclear (task-level vs resource-level rates) - Fixed Price projects showed irrelevant hourly rates - Didn't distinguish between Budget vs Actual The financial columns will be properly reimplemented later with: - Budget vs Actual distinction - Proper handling of T&M vs Fixed Price projects - Rate consistency detection Closes #132 Related issues for future work: - #133 - Three-level hierarchy (Task/Role/User) - thyme-bc-extension#29 - Unit of measure conversion Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Support for DAY and other time units in job planning lines: - Add unitOfMeasureCode field to BCJobPlanningLine type - Add BCResourceUnitOfMeasure type for conversion factors - Add getResourceUnitsOfMeasure() method to bcClient - Update projectDetailsService to convert quantities to hours - Update usePlanStore to convert quantities to hours - Update docs/bc-terminology.adoc with UOM documentation Requires Thyme BC Extension v1.7.0+ which exposes: - unitOfMeasureCode on /jobPlanningLines - /resourceUnitsOfMeasure endpoint with qtyPerUnitOfMeasure Closes thyme-bc-extension#29 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix DAY to HOURS conversion: divide by HOUR factor instead of multiplying (1.5 DAY / 0.133 = 11.25 HOURS, not 1.5 × 1 = 1.5) - Fix hoursPerDay calculation: find HOUR unit with factor < 1 (fractional) to correctly determine hours per day (7.5 instead of 1) - Rename "Hours Planned" to "Hours Budgeted" for BC terminology alignment - Add troubleshooting entry for hours/days display configuration Fixes #135 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…136) - Add side-by-side bars in Hours per Week chart: grey (planned) vs colored (actual) - Aggregate planned hours by week from Job Planning Lines (using planningDate) - Rename "Hours X" to "Time X" in all KPI cards (Fixes #37) - Reorder KPIs: Budgeted, Spent, Unposted, Posted - Tooltip shows "Planned" for budgeted hours, "Approved"/"Pending" for actual Fixes #136, Fixes #37 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Shows Planned (grey), Approved (green), and Pending (orange) indicators Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix unit conversion: BC uses resource's base unit (DAY), so convert hours to days before saving (e.g., 4 hours ÷ 7.5 = 0.533 days) - Don't trust API's unitOfMeasureCode when reading - always convert based on resource's HOUR factor - Add yellow dotted outline on hover for editable task rows - Fix wrong week bug: find allocation from clicked week, not first - Fix chart legend fitting inside container Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds PDF export options with/without financials, removes confusing financial columns from the breakdown table, implements unit conversion for DAY-based resources, and improves the Plan screen UX with hover states and week selection fixes.
Changes:
- Added PDF export dropdown with "With Financials" and "Without Financials" options
- Removed Unit Price and Total Price columns from breakdown table (kept Hours, Approved, Pending)
- Implemented unit of measure conversion between HOUR and DAY for resources, fetching conversion factors from BC API
- Added yellow dotted outline hover effect for editable task rows and fixed wrong week selection bug in Plan screen
- Enhanced Hours per Week chart with planned hours visualization and legend
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/bcUrls.ts | Added URL helpers for Job Planning Lines and Job Ledger Entries pages |
| src/types/index.ts | Added unitOfMeasureCode field and BCResourceUnitOfMeasure type |
| src/services/bc/bcClient.ts | Added getResourceUnitsOfMeasure API endpoint and unitOfMeasureCode parameter |
| src/services/bc/projectDetailsService.ts | Implemented unit conversion logic for DAY/HOUR and added planned hours by week |
| src/hooks/usePlanStore.ts | Added unit conversion when loading allocations |
| src/hooks/useProjectDetailsStore.ts | Added showPrices toggle for customer-facing prices |
| src/components/projects/ProjectTasksTable.tsx | Removed Unit Price columns and added expand/collapse all functionality |
| src/components/projects/ProjectKPICards.tsx | Added hours/days display format and BC links to financial metrics |
| src/components/projects/ProjectHeader.tsx | Implemented PDF export dropdown with financials options |
| src/components/projects/ProjectCharts.tsx | Added planned hours bars and legend to weekly chart |
| src/components/plan/PlanPanel.tsx | Added hover outline and fixed wrong week bug with findAllocationForWeek |
| src/components/plan/PlanEntryModal.tsx | Implemented unit conversion for DAY-based resources |
| src/components/plan/PlanEditModal.tsx | Implemented unit conversion for DAY-based resources |
| src/components/layout/Layout.tsx | Added print-specific footer text |
| src/components/layout/Header.tsx | Hidden navigation in print mode |
| src/app/globals.css | Added comprehensive print styles for PDF export |
| docs/bc-terminology.adoc | Documented unit of measure conversion |
| docs/TROUBLESHOOTING.adoc | Added troubleshooting entry for hours/days conversion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@BenGWeeks do you want me to fix co-pilots comments or you prefer to do that? |
- Use afterprint event instead of setTimeout for print dialog state restore - Fix CSS comment inconsistency (gap-4 = 16px) - Make update unit conversion consistent with create (check isResourceDayBased) - Remove unused hoursPerDay variable in usePlanStore Note: Unit conversion logic (hourFactor > 1) is correct - BC shows HOUR=7.5 meaning 1 DAY = 7.5 HOURS, not the inverse suggested by CoPilot. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Handle different BC unit configurations: - HOUR > 1 (e.g., 7.5): qtyPerUnitOfMeasure is hours-per-day - HOUR < 1 (e.g., 0.125): qtyPerUnitOfMeasure is day-per-hour (1/8 = 8 hours/day) - HOUR = 1: Resource is hour-based, no conversion needed This makes the code flexible for different BC setups. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Consolidate duplicated HOUR/DAY conversion logic from 4 files into src/utils/unitConversion.ts. Also fix PDF export setTimeout with requestAnimationFrame for more reliable render flush. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add division-by-zero guards for 0/negative qtyPerUnitOfMeasure values in getHoursPerDay, convertToHours, convertFromHours, isResourceDayBased - Fix UTC vs local date comparison in findAllocationForWeek by using string comparison instead of Date objects - Update showPrices comment to reflect actual usage (revenue KPIs/PDF) - Align hoursPerDay default to 8 across interface, service, and utility - Fix bc-terminology.adoc: clarify that code ignores unitOfMeasureCode and update UOM conversion example to match implementation - Add 22 unit tests for unitConversion.ts (buildUOMConversionMap, getHoursPerDay, convertToHours, convertFromHours, isResourceDayBased) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Merged both UOM conversion utilities (from this branch) and ResourceWorkload component (from main) in PlanEditModal and PlanEntryModal imports. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
BenGWeeks
left a comment
There was a problem hiding this comment.
Summary
Large, well-structured PR that adds PDF export with financials toggle, unit of measure (DAY/HOUR) conversion for BC resources, plan screen UX improvements, chart legend with planned hours visualization, KPI rebranding (Hours → Time), BC deep links, expand/collapse all in breakdown table, and comprehensive unit tests.
CI Status: All checks passing (Build, Lint/Format/TypeCheck, Unit Tests).
Secrets scan: Clean — no hardcoded secrets found.
Blockers
None. The critical issues from the earlier Copilot review (division-by-zero, UTC timezone mismatch, conversion logic) have been addressed.
Suggestions
-
PlanEntryModal.tsxline 80 — unnecessaryresourceNumberin useEffect dependency: ThegetResourceUnitsOfMeasure()API fetches all resource UOMs (not per-resource), so includingresourceNumberin the dependency array triggers redundant API calls whenever the resource changes. Consider removing it — the UOM data only needs to be fetched once when the modal opens. -
PlanEditModal.tsx—getResourceUnitsOfMeasure()called on every modal open: This fetches the full UOM table every time the edit modal opens. For users who open/close edit modals frequently (e.g., adjusting multiple weeks), this creates repeated API calls. Consider caching the UOM map at a higher level (e.g., in the plan store or a shared hook) so it's fetched once per session. -
ProjectHeader.tsx—requestAnimationFrame+setTimeout(0)for print timing: The double-frame delay (requestAnimationFrame→setTimeout(0)) to wait for React re-render beforewindow.print()works in practice but is fragile. If React batches state updates differently in a future version or the component tree is deep, the DOM might not be updated in time. A more robust approach would be to useflushSyncfromreact-domto force synchronous rendering before callingwindow.print(). -
getHoursPerDayglobal fallback (line 50-55 ofunitConversion.ts): The global fallback finds any resource's HOUR factor when the specific resource isn't found. In a multi-resource environment where resources have different hours-per-day configurations (e.g., some at 7.5h, some at 8h), this could return an incorrect value depending on the order of the array. Consider whether the fallback should be removed or documented more explicitly.
Questions
-
showPricesstate — is it intentionally screen-persistent? TheshowPricestoggle inuseProjectDetailsStoredefaults totrueand is used to control financial row visibility in print. But unlikeshowCosts(which has a toggle button in the header), there's no UI toggle forshowPriceson screen — it's only manipulated programmatically during "Without Financials" export. Is this by design, or should there be a visible toggle? -
Print behavior — always task view? The breakdown table forces task view during print (
groupBy === 'task' || isPrinting). If a user specifically switches to team view before printing, they might expect the team view in the PDF. Was this an intentional design choice (task view is always better for customers)?
Nits
-
unitConversion.tsJSDoc line 26: Says "Default: 7.5" but the actual default return value on line 58 is8. The docstring should say "Default: 8". -
PlanEntryModal.tsxline 80: TheresourceNumberdependency is included but unused by the effect body (the API call doesn't use it). ESLint may not catch this since the variable is in scope. Consider adding a comment if it's intentional (e.g., to refetch when resource changes for cache invalidation).
…ttern Fixes #142 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ection - Remove unnecessary resourceNumber from PlanEntryModal useEffect deps - Cache UOM conversion map in plan store to avoid redundant API calls - Both PlanEntryModal and PlanEditModal now use cached UOM data - Replace fragile rAF+setTimeout with flushSync for print timing - Remove getHoursPerDay global fallback (was unreliable with mixed resources) - Fix JSDoc: default is 8 hours/day, not 7.5 - Print user's chosen view (task or team) instead of always forcing task view - Add printExpandAll support to TeamBreakdownTable Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR Review Feedback Addressed (08c2d9c)All suggestions, questions, and nits from the review have been implemented: Suggestions
Questions
Nits
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…sPerDay - Parse planningDate as local date to avoid UTC timezone shift with YYYY-MM-DD - Compare existing vs new hours in same unit (hours) in PlanEditModal - Derive hoursPerDay from actual project resources instead of always defaulting to 8 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
flushSync in beforeprint handler ensures isPrinting state is committed to DOM before browser captures for print preview (React 19 batching would otherwise defer it). Added requestAnimationFrame before window.print() for Without Financials to allow a paint frame after cost/price toggle changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Changes
PDF Export (#130)
Consistent Report Naming (#142)
Breakdown Table
Unit Conversion Fix
unitOfMeasureCodeparameterPlan Screen UX
Related Issues
Test Plan
🤖 Generated with Claude Code