Conversation
WalkthroughA dedicated Profit & Loss page was added to the frontend, featuring a new React component to trigger a Xero P&L webhook with an optional date range. Supporting documentation, configuration, and test updates were made, including a new environment variable, a mapping fix, and a Jest configuration adjustment. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ProfitLossPage
participant ProfitLossWebhookTrigger
participant Xero P&L Webhook (n8n)
User->>ProfitLossPage: Visit /reports/profit-loss
ProfitLossPage->>ProfitLossWebhookTrigger: Render component
User->>ProfitLossWebhookTrigger: Select date range & click "Fetch Data"
ProfitLossWebhookTrigger->>Xero P&L Webhook (n8n): Send fetch request with (optional) date params
Xero P&L Webhook (n8n)-->>ProfitLossWebhookTrigger: Respond with success or error
ProfitLossWebhookTrigger-->>User: Show success/error message
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
docs/xero-integration.md (1)
71-77: The frontend usage documentation is clear and informative.The new section effectively documents the added Profit & Loss page functionality, including the date range selection and webhook triggering capabilities. The documentation is concise and maintains the style of the existing documentation.
Consider enhancing this section with:
- Screenshots of the new page
- Specific information about date format requirements
- Examples of validation and request feedback messages
packages/frontend/components/xero/ProfitLossWebhookTrigger.tsx (2)
58-73: Improve UI with loading state and date range reset.Add a loading indicator and reset button for better user experience.
return ( <div className="space-y-4 text-white"> + <p className="text-sm">Select a date range for the Profit & Loss report</p> <div className="flex space-x-4"> <Calendar mode="range" selected={{ from: startDate, to: endDate }} onSelect={(range) => { setStartDate(range?.from); setEndDate(range?.to); }} className="rounded border"/> + {(startDate || endDate) && ( + <Button + variant="outline" + onClick={() => { + setStartDate(undefined); + setEndDate(undefined); + setError(null); + setMessage(null); + }}> + Reset + </Button> + )} </div> {error && <p className="text-red-500">{error}</p>} {message && <p className="text-green-500">{message}</p>} - <Button onClick={trigger}>Fetch Data</Button> + <Button onClick={trigger} disabled={isLoading}> + {isLoading ? 'Loading...' : 'Fetch Data'} + </Button> </div> );
24-35: Improve validation logic.The validation function could be more maintainable with early returns and clearer error messages.
function validateRange(): boolean { + setError(null); + if ((startDate && !endDate) || (!startDate && endDate)) { - setError("Both start and end dates are required"); + setError("Please select both start and end dates"); return false; } + if (startDate && endDate && startDate > endDate) { - setError("Invalid date range"); + setError("End date must be after start date"); return false; } - setError(null); + return true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
docs/xero-integration.md(1 hunks)packages/frontend/__tests__/ProfitLossWebhookTrigger.test.tsx(1 hunks)packages/frontend/app/reports/profit-loss/page.tsx(1 hunks)packages/frontend/components/metrics-explorer.tsx(1 hunks)packages/frontend/components/xero/ProfitLossWebhookTrigger.tsx(1 hunks)packages/frontend/jest.config.js(1 hunks)packages/frontend/wrangler.toml(1 hunks)
🔇 Additional comments (4)
packages/frontend/jest.config.js (1)
15-15: The Jest module path alias update looks good.This change properly aligns the Jest configuration with the project structure, mapping
@/to the root directory instead of a src subdirectory. This is consistent with modern Next.js applications, especially those using the app directory structure.packages/frontend/wrangler.toml (1)
13-13: The new environment variable for the P&L webhook URL is correctly defined.This variable follows the established naming convention and URL pattern, making it consistent with the existing
N8N_BALANCE_SHEET_WEBHOOK_URL. This configuration supports the new profit and loss webhook trigger functionality described in the PR.packages/frontend/components/metrics-explorer.tsx (1)
30-30: Fixed widget ID spelling to match the metric name.This correction ensures consistent spelling between the 'direct labour' metric name and its widget ID, which was previously mismatched ('directlabor'). This alignment prevents potential issues with widget visibility toggling and ensures proper functionality for this metric.
packages/frontend/app/reports/profit-loss/page.tsx (1)
1-10: Page implementation looks good.The profit and loss page is implemented correctly with proper layout, heading, and component integration. The component hierarchy is well-structured and follows React best practices.
| beforeEach(() => { | ||
| global.fetch = jest.fn(() => Promise.resolve({ ok: true })) as any; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use waitFor for asynchronous assertions.
Current test setup doesn't properly handle asynchronous operations. Use waitFor to ensure assertions are evaluated after the asynchronous operation completes.
beforeEach(() => {
- global.fetch = jest.fn(() => Promise.resolve({ ok: true })) as any;
+ global.fetch = jest.fn(() => Promise.resolve({
+ ok: true,
+ json: () => Promise.resolve({})
+ })) as any;
});📝 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.
| beforeEach(() => { | |
| global.fetch = jest.fn(() => Promise.resolve({ ok: true })) as any; | |
| }); | |
| beforeEach(() => { | |
| global.fetch = jest.fn(() => Promise.resolve({ | |
| ok: true, | |
| json: () => Promise.resolve({}) | |
| })) as any; | |
| }); |
🤖 Prompt for AI Agents
In packages/frontend/__tests__/ProfitLossWebhookTrigger.test.tsx around lines 6
to 8, the test setup mocks global.fetch but does not handle asynchronous
operations correctly. Modify the test to wrap assertions that depend on
asynchronous fetch calls inside a `waitFor` block from the testing library to
ensure they run after the async operations complete, preventing false positives
or race conditions.
| /// <reference types="jest" /> | ||
| import { render, screen, fireEvent } from '@testing-library/react'; | ||
| import ProfitLossWebhookTrigger from '../components/xero/ProfitLossWebhookTrigger'; | ||
| import { jest, beforeEach, afterEach, test, expect } from '@jest/globals'; | ||
|
|
||
| beforeEach(() => { | ||
| global.fetch = jest.fn(() => Promise.resolve({ ok: true })) as any; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| (global.fetch as jest.Mock).mockClear(); | ||
| }); | ||
|
|
||
| test('calls webhook with formatted dates', async () => { | ||
| render(<ProfitLossWebhookTrigger baseUrl="https://example.com" />); | ||
| const button = screen.getByText('Fetch Data'); | ||
| fireEvent.click(button); | ||
| expect(fetch).toHaveBeenCalledWith('https://example.com'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test coverage needs improvement.
While the basic functionality test is good, it only tests the simplest case (no date range). Consider adding tests for:
- Selecting date ranges and verifying URL parameters
- Error handling scenarios
- Validation failures
test('calls webhook with formatted dates', async () => {
render(<ProfitLossWebhookTrigger baseUrl="https://example.com" />);
const button = screen.getByText('Fetch Data');
fireEvent.click(button);
expect(fetch).toHaveBeenCalledWith('https://example.com');
});
+
+test('calls webhook with date range parameters', async () => {
+ render(<ProfitLossWebhookTrigger baseUrl="https://example.com" />);
+ // Mock selecting dates in the calendar
+ // This would require more setup with the Calendar component
+ // ...
+ const button = screen.getByText('Fetch Data');
+ fireEvent.click(button);
+ expect(fetch).toHaveBeenCalledWith(
+ expect.stringMatching(/https:\/\/example.com\?start=\d{4}-\d{2}-\d{2}&end=\d{4}-\d{2}-\d{2}/)
+ );
+});
+
+test('displays error message on invalid date range', async () => {
+ render(<ProfitLossWebhookTrigger baseUrl="https://example.com" />);
+ // Setup invalid date range
+ // ...
+ const button = screen.getByText('Fetch Data');
+ fireEvent.click(button);
+ expect(screen.getByText('Invalid date range')).toBeInTheDocument();
+ expect(fetch).not.toHaveBeenCalled();
+});📝 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.
| /// <reference types="jest" /> | |
| import { render, screen, fireEvent } from '@testing-library/react'; | |
| import ProfitLossWebhookTrigger from '../components/xero/ProfitLossWebhookTrigger'; | |
| import { jest, beforeEach, afterEach, test, expect } from '@jest/globals'; | |
| beforeEach(() => { | |
| global.fetch = jest.fn(() => Promise.resolve({ ok: true })) as any; | |
| }); | |
| afterEach(() => { | |
| (global.fetch as jest.Mock).mockClear(); | |
| }); | |
| test('calls webhook with formatted dates', async () => { | |
| render(<ProfitLossWebhookTrigger baseUrl="https://example.com" />); | |
| const button = screen.getByText('Fetch Data'); | |
| fireEvent.click(button); | |
| expect(fetch).toHaveBeenCalledWith('https://example.com'); | |
| }); | |
| /// <reference types="jest" /> | |
| import { render, screen, fireEvent } from '@testing-library/react'; | |
| import ProfitLossWebhookTrigger from '../components/xero/ProfitLossWebhookTrigger'; | |
| import { jest, beforeEach, afterEach, test, expect } from '@jest/globals'; | |
| beforeEach(() => { | |
| global.fetch = jest.fn(() => Promise.resolve({ ok: true })) as any; | |
| }); | |
| afterEach(() => { | |
| (global.fetch as jest.Mock).mockClear(); | |
| }); | |
| test('calls webhook with formatted dates', async () => { | |
| render(<ProfitLossWebhookTrigger baseUrl="https://example.com" />); | |
| const button = screen.getByText('Fetch Data'); | |
| fireEvent.click(button); | |
| expect(fetch).toHaveBeenCalledWith('https://example.com'); | |
| }); | |
| test('calls webhook with date range parameters', async () => { | |
| render(<ProfitLossWebhookTrigger baseUrl="https://example.com" />); | |
| // Mock selecting dates in the calendar | |
| // This would require more setup with the Calendar component | |
| // ... | |
| const button = screen.getByText('Fetch Data'); | |
| fireEvent.click(button); | |
| expect(fetch).toHaveBeenCalledWith( | |
| expect.stringMatching(/https:\/\/example.com\?start=\d{4}-\d{2}-\d{2}&end=\d{4}-\d{2}-\d{2}/) | |
| ); | |
| }); | |
| test('displays error message on invalid date range', async () => { | |
| render(<ProfitLossWebhookTrigger baseUrl="https://example.com" />); | |
| // Setup invalid date range | |
| // ... | |
| const button = screen.getByText('Fetch Data'); | |
| fireEvent.click(button); | |
| expect(screen.getByText('Invalid date range')).toBeInTheDocument(); | |
| expect(fetch).not.toHaveBeenCalled(); | |
| }); |
🤖 Prompt for AI Agents
In packages/frontend/__tests__/ProfitLossWebhookTrigger.test.tsx around lines 1
to 19, the current test only covers the basic fetch call without date
parameters. To improve test coverage, add tests that simulate selecting date
ranges and verify the fetch URL includes the correct query parameters, add tests
that simulate fetch errors and verify error handling behavior, and add tests
that check validation failures such as invalid or missing date inputs to ensure
the component handles these cases properly.
| test('calls webhook with formatted dates', async () => { | ||
| render(<ProfitLossWebhookTrigger baseUrl="https://example.com" />); | ||
| const button = screen.getByText('Fetch Data'); | ||
| fireEvent.click(button); | ||
| expect(fetch).toHaveBeenCalledWith('https://example.com'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make test assert behavior asynchronously.
Since fetch is an asynchronous operation, the test should await for the promise to resolve before making assertions.
-test('calls webhook with formatted dates', async () => {
+test('calls webhook with formatted dates', async () => {
render(<ProfitLossWebhookTrigger baseUrl="https://example.com" />);
const button = screen.getByText('Fetch Data');
fireEvent.click(button);
- expect(fetch).toHaveBeenCalledWith('https://example.com');
+ await waitFor(() => {
+ expect(fetch).toHaveBeenCalledWith('https://example.com');
+ });
});📝 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.
| test('calls webhook with formatted dates', async () => { | |
| render(<ProfitLossWebhookTrigger baseUrl="https://example.com" />); | |
| const button = screen.getByText('Fetch Data'); | |
| fireEvent.click(button); | |
| expect(fetch).toHaveBeenCalledWith('https://example.com'); | |
| }); | |
| test('calls webhook with formatted dates', async () => { | |
| render(<ProfitLossWebhookTrigger baseUrl="https://example.com" />); | |
| const button = screen.getByText('Fetch Data'); | |
| fireEvent.click(button); | |
| await waitFor(() => { | |
| expect(fetch).toHaveBeenCalledWith('https://example.com'); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In packages/frontend/__tests__/ProfitLossWebhookTrigger.test.tsx around lines 14
to 19, the test asserts fetch calls synchronously despite fetch being
asynchronous. Modify the test to await the asynchronous operation by using
async/await or appropriate async utilities like waitFor to ensure the fetch
promise resolves before making assertions.
| const webhookBase = | ||
| baseUrl || | ||
| process.env.NEXT_PUBLIC_N8N_XERO_PNL_WEBHOOK_URL || | ||
| process.env.N8N_XERO_PNL_WEBHOOK_URL || | ||
| ""; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Environment variable fallback requires improvement.
The multiple fallback approach for webhook URL is confusing. Consider using a single environment variable and handling missing configuration more explicitly.
- const webhookBase =
- baseUrl ||
- process.env.NEXT_PUBLIC_N8N_XERO_PNL_WEBHOOK_URL ||
- process.env.N8N_XERO_PNL_WEBHOOK_URL ||
- "";
+ const webhookBase = baseUrl || process.env.NEXT_PUBLIC_N8N_XERO_PNL_WEBHOOK_URL || "";🤖 Prompt for AI Agents
In packages/frontend/components/xero/ProfitLossWebhookTrigger.tsx around lines
18 to 22, the current code uses multiple environment variables as fallbacks for
the webhook URL, which is confusing. Refactor this to use a single, clearly
named environment variable for the webhook URL. Add explicit handling for the
case when this environment variable is missing, such as logging an error or
throwing an exception, to make the configuration requirement clear.
| async function trigger() { | ||
| if (!validateRange()) return; | ||
| if (!webhookBase) { | ||
| setError("Webhook URL not configured"); | ||
| return; | ||
| } | ||
| let url = webhookBase; | ||
| if (startDate && endDate) { | ||
| const start = format(startDate, "yyyy-MM-dd"); | ||
| const end = format(endDate, "yyyy-MM-dd"); | ||
| url += `?start=${start}&end=${end}`; | ||
| } | ||
| try { | ||
| const res = await fetch(url); | ||
| if (!res.ok) throw new Error(`Status ${res.status}`); | ||
| setMessage("Data request initiated"); | ||
| } catch (e) { | ||
| setError("Failed to fetch data"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add loading state and improve error handling.
The component should show a loading state during the fetch operation and provide more detailed error information.
+ const [isLoading, setIsLoading] = useState(false);
async function trigger() {
if (!validateRange()) return;
+ setMessage(null);
+ setError(null);
if (!webhookBase) {
setError("Webhook URL not configured");
return;
}
let url = webhookBase;
if (startDate && endDate) {
const start = format(startDate, "yyyy-MM-dd");
const end = format(endDate, "yyyy-MM-dd");
url += `?start=${start}&end=${end}`;
}
try {
+ setIsLoading(true);
const res = await fetch(url);
- if (!res.ok) throw new Error(`Status ${res.status}`);
+ if (!res.ok) {
+ const errorData = await res.text().catch(() => null);
+ throw new Error(`Status ${res.status}${errorData ? `: ${errorData}` : ''}`);
+ }
setMessage("Data request initiated");
} catch (e) {
- setError("Failed to fetch data");
+ setError(`Failed to fetch data: ${e instanceof Error ? e.message : String(e)}`);
+ } finally {
+ setIsLoading(false);
}
}📝 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.
| async function trigger() { | |
| if (!validateRange()) return; | |
| if (!webhookBase) { | |
| setError("Webhook URL not configured"); | |
| return; | |
| } | |
| let url = webhookBase; | |
| if (startDate && endDate) { | |
| const start = format(startDate, "yyyy-MM-dd"); | |
| const end = format(endDate, "yyyy-MM-dd"); | |
| url += `?start=${start}&end=${end}`; | |
| } | |
| try { | |
| const res = await fetch(url); | |
| if (!res.ok) throw new Error(`Status ${res.status}`); | |
| setMessage("Data request initiated"); | |
| } catch (e) { | |
| setError("Failed to fetch data"); | |
| } | |
| } | |
| const [isLoading, setIsLoading] = useState(false); | |
| async function trigger() { | |
| if (!validateRange()) return; | |
| setMessage(null); | |
| setError(null); | |
| if (!webhookBase) { | |
| setError("Webhook URL not configured"); | |
| return; | |
| } | |
| let url = webhookBase; | |
| if (startDate && endDate) { | |
| const start = format(startDate, "yyyy-MM-dd"); | |
| const end = format(endDate, "yyyy-MM-dd"); | |
| url += `?start=${start}&end=${end}`; | |
| } | |
| try { | |
| setIsLoading(true); | |
| const res = await fetch(url); | |
| if (!res.ok) { | |
| const errorData = await res.text().catch(() => null); | |
| throw new Error( | |
| `Status ${res.status}${errorData ? `: ${errorData}` : ''}` | |
| ); | |
| } | |
| setMessage("Data request initiated"); | |
| } catch (e) { | |
| setError( | |
| `Failed to fetch data: ${e instanceof Error ? e.message : String(e)}` | |
| ); | |
| } finally { | |
| setIsLoading(false); | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/frontend/components/xero/ProfitLossWebhookTrigger.tsx around lines
37 to 56, the trigger function lacks a loading state and detailed error
handling. Add a loading state variable that is set to true before the fetch call
and false after it completes or fails. Update the UI to reflect this loading
state. In the catch block, capture and display the actual error message instead
of a generic one to provide more detailed error information.
1 participant
Summary
/reports/profit-losspage using the componentdirect labourmetric correctlyN8N_XERO_PNL_WEBHOOK_URLin worker configTesting
npm run lint:frontendnpm run typecheck:frontendnpm run test:frontendnpm run build:frontend(fails: connect EHOSTUNREACH 172.25.0.3:8080)Summary by CodeRabbit
New Features
/reports/profit-loss, allowing users to select a custom date range and trigger a Profit & Loss webhook.Bug Fixes
Documentation
Tests
Chores