Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a client-side dashboard PDF export: a modal to pick a session date range, a client reporting service that aggregates marks into a session report, and an in-browser multi-page branded PDF renderer offered for download. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Dashboard as DashboardPage
participant Modal as SessionReportModal
participant Service as sessionReport.ts
participant PDFDoc as SessionReportDocument
participant Browser as Browser
User->>Dashboard: Click "Generate Master PDF"
Dashboard->>Modal: open modal (isOpen=true)
Modal->>Service: getSectionDateRange(boys)
Service-->>Modal: {startDate, endDate} or null
User->>Modal: Select date range & confirm
Modal->>Service: buildSessionReportData({boys, section, range})
Service-->>Modal: SessionReportData
Modal->>PDFDoc: Render SessionReportDocument(report)
PDFDoc-->>Browser: Generate multi-page PDF (client-side)
Browser-->>User: Download branded session PDF
User->>Modal: Close
Modal->>Dashboard: onClose (isOpen=false)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
services/reporting/sessionReport.test.ts (1)
45-50: Add one edge-case test for empty marks input.Please add an assertion that
getSectionDateRange([])(or boys with emptymarks) returnsnull, to lock in no-data behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/reporting/sessionReport.test.ts` around lines 45 - 50, Add an edge-case test in sessionReport.test.ts to assert that getSectionDateRange returns null for no-data input: call getSectionDateRange([]) (and/or pass a boys array where each member has empty marks) and expect the result toEqual(null); place this new it(...) alongside the existing test that uses companyBoys and reference the getSectionDateRange function so the no-marks behavior is locked in.docs/user-guide.md (1)
33-38: Optional readability tweak in the “Getting Started” list.Several consecutive steps start with “Use …”. Consider varying sentence openings to improve scanability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user-guide.md` around lines 33 - 38, The "Getting Started" numbered list repeats the phrase "Use ..." which hurts scanability; update the list entries that start with "Use `Home`", "Use `Weekly Marks`", and "Use `Dashboard`" to vary their openings (for example: "Manage the member roster with `Home`", "Record weekly attendance and scores in `Weekly Marks`", "View section performance or download the session PDF from `Dashboard`") while keeping the same meanings and existing backticks for UI labels.components/reports/SessionReportDocument.tsx (1)
13-16: External image URLs create a runtime dependency on third-party hosting.These hardcoded URLs rely on
postimg.ccavailability. If this service becomes unreachable or the images are removed, PDF generation will fail or produce broken images. Consider bundling these assets locally or using a CDN under your control for production reliability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/reports/SessionReportDocument.tsx` around lines 13 - 16, The hardcoded external image URLs (BB_LOGO_URL, BB_BACKGROUND_URL, COMPANY_LOGO_URL, JUNIOR_LOGO_URL) create a runtime dependency on postimg.cc; replace them with locally bundled assets (or a controlled CDN) and update the constants to reference the imported/local paths instead. Add the image files to the repo's assets/static/images directory, import them in SessionReportDocument (or require them where the constants are defined), and update any PDF generation/rendering code to use these local imports; also add a simple fallback (placeholder image or conditional check) when an import fails to avoid broken images at runtime. Ensure references to BB_LOGO_URL, BB_BACKGROUND_URL, COMPANY_LOGO_URL, and JUNIOR_LOGO_URL are updated across the file so PDF generation uses the new local/controlled URLs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/SessionReportModal.tsx`:
- Around line 83-95: The inputs and the download button in SessionReportModal
use hard-coded focus/active classes like focus:ring-company-blue, causing
inconsistent styling when activeSection is junior; update the className for the
date inputs (the inputs bound to startDate/endDate via setStartDate/setEndDate
and sectionRange) and the download button (the element rendering the "Download"
action) to compute the focus/outline classes based on activeSection (e.g.,
choose company vs junior brand class), replacing the static
focus:ring-company-blue and focus:border-company-blue with a conditional class
expression that references activeSection so the UI uses junior branding when
activeSection indicates junior (also apply the same change to the other
occurrence mentioned around lines 148-149).
---
Nitpick comments:
In `@components/reports/SessionReportDocument.tsx`:
- Around line 13-16: The hardcoded external image URLs (BB_LOGO_URL,
BB_BACKGROUND_URL, COMPANY_LOGO_URL, JUNIOR_LOGO_URL) create a runtime
dependency on postimg.cc; replace them with locally bundled assets (or a
controlled CDN) and update the constants to reference the imported/local paths
instead. Add the image files to the repo's assets/static/images directory,
import them in SessionReportDocument (or require them where the constants are
defined), and update any PDF generation/rendering code to use these local
imports; also add a simple fallback (placeholder image or conditional check)
when an import fails to avoid broken images at runtime. Ensure references to
BB_LOGO_URL, BB_BACKGROUND_URL, COMPANY_LOGO_URL, and JUNIOR_LOGO_URL are
updated across the file so PDF generation uses the new local/controlled URLs.
In `@docs/user-guide.md`:
- Around line 33-38: The "Getting Started" numbered list repeats the phrase "Use
..." which hurts scanability; update the list entries that start with "Use
`Home`", "Use `Weekly Marks`", and "Use `Dashboard`" to vary their openings (for
example: "Manage the member roster with `Home`", "Record weekly attendance and
scores in `Weekly Marks`", "View section performance or download the session PDF
from `Dashboard`") while keeping the same meanings and existing backticks for UI
labels.
In `@services/reporting/sessionReport.test.ts`:
- Around line 45-50: Add an edge-case test in sessionReport.test.ts to assert
that getSectionDateRange returns null for no-data input: call
getSectionDateRange([]) (and/or pass a boys array where each member has empty
marks) and expect the result toEqual(null); place this new it(...) alongside the
existing test that uses companyBoys and reference the getSectionDateRange
function so the no-marks behavior is locked in.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 339829e1-3aa7-4109-9624-2d37f725643d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
ARCHITECTURE.mdcomponents/DashboardPage.tsxcomponents/SessionReportModal.tsxcomponents/reports/SessionReportDocument.tsxdocs/06-data-and-services.mddocs/user-guide.mdpackage.jsonservices/reporting/sessionReport.test.tsservices/reporting/sessionReport.tstypes/reporting.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/reports/SessionReportDocument.tsx (1)
13-16: Consider bundling or self-hosting image assets.These external URLs depend on
postimg.ccavailability. If the service is slow or unreachable, PDF generation could fail or hang. Consider bundling these assets locally or hosting them on infrastructure you control.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/reports/SessionReportDocument.tsx` around lines 13 - 16, The four hard-coded external image constants (BB_LOGO_URL, BB_BACKGROUND_URL, COMPANY_LOGO_URL, JUNIOR_LOGO_URL) in SessionReportDocument.tsx create a runtime dependency on postimg.cc; change them to point to locally bundled assets (import or require the image files into the module) or to your controlled CDN URLs, update any code that uses these constants to use the new imported variables, and remove reliance on external fetching during PDF generation (optionally add a fallback local asset or error handling where these constants are consumed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/reports/SessionReportDocument.tsx`:
- Around line 458-493: The column widths for the junior branch in
renderMemberTable sum to 110%, causing layout overflow; update both the header
Text widths and the corresponding row Text widths when section === 'junior' so
the junior widths sum to 100% (use: Member 24%, Squad 8%, Year 8%, Attend 8%,
Absent 8%, Rate 8%, Total 12%, Uniform 12%, Behav. 12%) ensuring you change the
width values in the table head Text nodes and the members.map row Text nodes
that currently use section === 'junior' or hardcoded percentages.
---
Nitpick comments:
In `@components/reports/SessionReportDocument.tsx`:
- Around line 13-16: The four hard-coded external image constants (BB_LOGO_URL,
BB_BACKGROUND_URL, COMPANY_LOGO_URL, JUNIOR_LOGO_URL) in
SessionReportDocument.tsx create a runtime dependency on postimg.cc; change them
to point to locally bundled assets (import or require the image files into the
module) or to your controlled CDN URLs, update any code that uses these
constants to use the new imported variables, and remove reliance on external
fetching during PDF generation (optionally add a fallback local asset or error
handling where these constants are consumed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed060c12-1efd-424a-82d9-55c5ec315efd
📒 Files selected for processing (1)
components/reports/SessionReportDocument.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/reports/SessionReportDocument.tsx`:
- Around line 554-555: topMonthMark is incorrectly computed from
report.meetings; update the calculation for topMonthMark to use report.months
(i.e., Math.max(...report.months.map(month => month.totalMarks), 0)) so the
"Monthly Session Pattern" chart uses the correct maxValue; keep the same
fallback of 0 and leave topMeetingMark as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 947afaac-74f4-44d0-9852-e11ccb68815a
⛔ Files ignored due to path filters (4)
assets/branding/bb-background.jpgis excluded by!**/*.jpgassets/branding/bb-logo.pngis excluded by!**/*.pngassets/branding/company-logo.pngis excluded by!**/*.pngassets/branding/junior-logo.pngis excluded by!**/*.png
📒 Files selected for processing (4)
components/SessionReportModal.tsxcomponents/reports/SessionReportDocument.tsxdocs/user-guide.mdservices/reporting/sessionReport.test.ts
✅ Files skipped from review due to trivial changes (1)
- components/SessionReportModal.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/user-guide.md
- services/reporting/sessionReport.test.ts
Comment on lines
+554
to
+555
| const topMeetingMark = Math.max(...report.meetings.map((meeting) => meeting.totalMarks), 0); | ||
| const topMonthMark = Math.max(...report.months.map((month) => month.totalMarks), 0); |
There was a problem hiding this comment.
Bug: topMonthMark is computed from report.meetings instead of report.months.
Line 555 iterates over report.meetings but the variable is named topMonthMark and the callback parameter is named month. This causes the "Monthly Session Pattern" chart (lines 721-732) to use incorrect maxValue for bar width calculations.
🐛 Proposed fix
const topMeetingMark = Math.max(...report.meetings.map((meeting) => meeting.totalMarks), 0);
-const topMonthMark = Math.max(...report.meetings.map((month) => month.totalMarks), 0);
+const topMonthMark = Math.max(...report.months.map((month) => month.totalMarks), 0);📝 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.
Suggested change
| const topMeetingMark = Math.max(...report.meetings.map((meeting) => meeting.totalMarks), 0); | |
| const topMonthMark = Math.max(...report.months.map((month) => month.totalMarks), 0); | |
| const topMeetingMark = Math.max(...report.meetings.map((meeting) => meeting.totalMarks), 0); | |
| const topMonthMark = Math.max(...report.months.map((month) => month.totalMarks), 0); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/reports/SessionReportDocument.tsx` around lines 554 - 555,
topMonthMark is incorrectly computed from report.meetings; update the
calculation for topMonthMark to use report.months (i.e.,
Math.max(...report.months.map(month => month.totalMarks), 0)) so the "Monthly
Session Pattern" chart uses the correct maxValue; keep the same fallback of 0
and leave topMeetingMark as-is.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Summary by CodeRabbit
New Features
Documentation