feat: Add Closing Book feature with sidebar nav and content panels#106
feat: Add Closing Book feature with sidebar nav and content panels#106
Conversation
…nels - Updated sidebar configuration to include a new "Close" link. - Added new components for the Closing Book feature, including CloseContent, StructureSidebar, and various panels (AccountRollupsPanel, PeriodClosePanel, SchedulePanel, StatementPanel). - Implemented data fetching and state management for closing book categories and entries. - Enhanced user interface with loading states and error handling for better user experience. - Introduced view mode toggle functionality for rendered and facts views.
|
@claude please review this PR |
|
Claude finished @jfrench9's task in 2m 24s —— View job PR Review: feat: Add Closing Book feature with sidebar nav and content panels
Overall AssessmentThis is a well-structured, additive feature that integrates cleanly with existing conventions. The component decomposition is thoughtful, test coverage is solid, and dark mode/accessibility basics are handled. A handful of bugs and polish issues are worth addressing before merge. Bugs1. Unused
// line 33
const [hasData, setHasData] = useState(false)
// line 69
setHasData(response.has_data)
// ...but hasData is never referenced in JSX2.
// SchedulePanel.tsx:54
const [creatingEntry, setCreatingEntry] = useState(false)
// line 185 — all buttons disabled when any creation is in flight
disabled={creatingEntry}Design Issues3. Fragile The period preset buttons in const rollupsCategory = response.categories.find(
(c) => c.label === 'Account Rollups' // brittle string match
)A more robust alternative would be for the API to return a dedicated 4. Inconsistent currency formatting for negative values —
Consider extracting a single shared Fix this →%20AccountRollupsPanel.tsx%20and%20StatementTable.tsx%20use%20accounting%20notation%20%28parentheses%20for%20negatives%29%2C%202)%20PeriodClosePanel.tsx%20uses%20Intl.NumberFormat%20with%20a%20minus%20sign.%20Extract%20a%20single%20shared%20%60formatCurrency%60%20function%20into%20a%20local%20utils%20file%20(e.g.%20%60src%2Fapp%2F(app)%2Fledger%2Fclose%2Futils.ts%60)%20that%20uses%20accounting%20notation%20(parentheses%20for%20negatives%2C%20consistent%20with%20financial%20statement%20conventions)%2C%20then%20replace%20all%20three%20inline%20formatters%20with%20the%20shared%20one.&repo=RoboFinSystems%2Froboledger-app) Minor Issues5. Dead The 6. The
If this is a known placeholder for a future panel, a brief 7. { start: `${year}-01-01`, end: `${year}-12-31`, label: `FY ${year}` },This assumes a Jan–Dec fiscal year. Companies with non-standard fiscal years will get incorrect date ranges silently. Low-risk for now, but worth a comment. 8. Sidebar nav label
Positives
| |
- Removed redundant local currency formatting functions from multiple components, centralizing the logic in a utility file. - Updated CloseContent to simplify mapping ID extraction for report regeneration. - Enhanced SchedulePanel to manage entry creation state more effectively. - Improved the integration of currency formatting in StatementTable for consistency.
|
@claude can you review the latest changes? |
|
Claude finished @jfrench9's task in 2m 17s —— View job PR Review: feat: Add Closing Book feature with sidebar nav and content panels
Latest commit: Refactor currency formatting and streamline component logicGood follow-up commit. The latest push (e965b6b) addresses 5 of the 8 issues from the previous review. ✅ Fixed
Still Open1. Dead The 2. { start: `${year}-01-01`, end: `${year}-12-31`, label: `FY ${year}` }Still hardcodes a calendar year-end. Low risk for now but will silently produce wrong date ranges for non-calendar fiscal years. Worth adding an inline comment so future maintainers know this is a known limitation. 3. Sidebar nav label Still reads as a dismiss action rather than "Closing Book." The page header says "Closing Book" — aligning the nav label would avoid confusion. Consider New Observations4. Two currency conventions still visible to users The split into
If this split is intentional (e.g., statement rows use accounting notation, journal/schedule amounts include 5. categories.map((cat) => (
<div key={cat.label} ...>If the API ever returns categories with duplicate labels, this will cause subtle React reconciliation bugs. A stable SummaryThe latest commit is a solid cleanup that addresses all the substantive bugs from the prior review. What remains are three minor open items (dead |
…ar tests - Changed the sidebar navigation label from "Close" to "Closing Book" for clarity. - Removed unused label properties from test cases in StructureSidebar and content tests to streamline the code. - Added comments in StatementPanel to clarify assumptions regarding fiscal year handling.
Summary
Implements the Closing Book feature (
/ledger/close), a new section within the Ledger module that provides a structured interface for managing period-close workflows. The feature introduces a sidebar-driven navigation pattern with multiple specialized content panels for viewing and interacting with closing book data.Changes
New Pages & Components
/ledger/closepage — New Next.js route with a top-levelcontent.tsxorchestrating layout and stateStructureSidebar— Sidebar navigation component for browsing the closing book hierarchy (e.g., statements, schedules, account rollups)StatementPanel/StatementTable— Displays financial statement data in a tabular format with drill-down capabilitySchedulePanel— Renders supporting schedule details tied to statement line itemsAccountRollupsPanel— Shows rolled-up account balances with expandable detail rowsPeriodClosePanel— Manages period-close status, actions, and workflow stateFactsTable— Presents granular fact-level data (journal entries, transactions) for a selected contextViewModeToggle— Toggle control for switching between different display modes (e.g., summary vs. detail)Navigation Integration
sidebar-config.tsxto include the new Closing Book entry under the Ledger sectionOther Modifications
operationHooks.ts— Minor fix/adjustment (2 lines changed) in the task monitoring utilitypackage.json/package-lock.json— Dependency update (1 package version bump)Key UI/UX Improvements
Breaking Changes
None. This is a purely additive feature with no modifications to existing component APIs or routes.
Testing
Automated Tests (6 new test files, ~1,359 lines)
AccountRollupsPanel.test.tsx— Rendering, expansion/collapse behavior, data displayFactsTable.test.tsx— Column rendering, row data, empty statesStatementTable.test.tsx— Table structure, formatting, interactionsStructureSidebar.test.tsx— Navigation item rendering, selection state, hierarchy traversalViewModeToggle.test.tsx— Toggle state management and callback behaviorcontent.test.tsx— Integration-level tests for panel routing, sidebar ↔ content coordinationManual Review Notes
/ledger/closeand verify the sidebar loads with a hierarchical structureViewModeToggleswitches between summary/detail views where applicableStatementPanel→SchedulePanel→AccountRollupsPaneldrill-down flowPeriodClosePaneldisplays status and actions appropriatelyBrowser Compatibility
No novel browser APIs are introduced. The feature uses standard React patterns, CSS layout (likely flexbox/grid per the existing app conventions), and existing UI component library primitives. Should be compatible with all browsers already supported by the application (modern Chrome, Firefox, Safari, Edge).
🤖 Generated with Claude Code
Branch Info:
feature/closing-bookmainCo-Authored-By: Claude noreply@anthropic.com