feat: add icalendar file download support#2909
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaces the Google Calendar link flow with client-side ICS export: adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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.
Actionable comments posted: 5
🧹 Nitpick comments (4)
frontend/jest.setup.ts (1)
125-129: Make theicsmock ESM-safe for named imports.
To reduce flakiness with Jest’s ESM resolution, return__esModule: trueas well.jest.mock('ics', () => { return { + __esModule: true, createEvent: jest.fn(), } })frontend/__tests__/unit/components/CalendarButton.test.tsx (1)
79-108: Add cleanup assertions once production code revokes the blob URL / removes the temp link.
Right now the test only verifies creation + click; after implementing cleanup infrontend/src/components/CalendarButton.tsx, please also assertremoveChild(orlink.remove()) andURL.revokeObjectURL(mockUrl)were called.frontend/src/components/CalendarButton.tsx (1)
39-52: Settype="button"to avoid accidental form submits.<button + type="button" onClick={handleDownload} onMouseEnter={() => setIsHovered(true)} onMouseLeave={() => setIsHovered(false)} disabled={isDownloading} aria-label={ariaLabel} className={className} >frontend/src/utils/getIcsFileUrl.ts (1)
27-50: Optional refactoring: Conditionally set optional EventAttributes fields for clarity.The
icslibrary gracefully handlesundefinedvalues in EventAttributes—omitting them from the output rather than causing errors. While the current code works correctly as written, you may consider using a cleaner pattern if the codebase prefers explicit field presence:const eventAttributes: EventAttributes = { start: startArray, end: finalEndArray, title: event.title, status: 'CONFIRMED', busyStatus: 'BUSY', ...(event.description && { description: event.description }), ...(event.location && { location: event.location }), ...(event.url && { url: event.url }), }This pattern is purely stylistic—no functional change is required.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
cspell/custom-dict.txt(1 hunks)frontend/__tests__/e2e/pages/CalendarButton.spec.ts(1 hunks)frontend/__tests__/unit/components/CalendarButton.test.tsx(9 hunks)frontend/__tests__/unit/utils/getIcsFileUrl.test.ts(1 hunks)frontend/jest.setup.ts(1 hunks)frontend/package.json(1 hunks)frontend/src/components/CalendarButton.tsx(1 hunks)frontend/src/utils/getIcsFileUrl.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/__tests__/unit/components/CalendarButton.test.tsx (1)
frontend/src/components/CalendarButton.tsx (1)
CalendarButton(9-54)
frontend/src/components/CalendarButton.tsx (1)
frontend/src/types/calendar.ts (1)
CalendarButtonProps(12-19)
frontend/__tests__/unit/utils/getIcsFileUrl.test.ts (1)
frontend/src/utils/getIcsFileUrl.ts (1)
getIcsFileUrl(4-52)
🔇 Additional comments (2)
cspell/custom-dict.txt (1)
68-73: LGTM (dictionary update).frontend/package.json (1)
49-52: No compatibility issues found withics@^3.8.1. The package supports both ESM (import * as ics from 'ics') and CommonJS formats, and bothstatusandbusyStatusfields are available in the EventAttributes. This dependency is compatible with Jest 30 ESM setup and Next.js bundling.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Please address coderabbit's comments first
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/__tests__/unit/components/CalendarButton.test.tsx (3)
28-31: Consider moving URL mocks tobeforeEachfor test isolation.Global mocks set in
beforeAllpersist across all tests without reset. While this works here since the mocks aren't directly exercised (generateIcsFileUrl is fully mocked), moving them tobeforeEachwould ensure cleaner test isolation and align with the pattern used for other mocks.- beforeAll(() => { - global.URL.createObjectURL = jest.fn(() => 'mock-url'); - global.URL.revokeObjectURL = jest.fn(); - }); - beforeEach(() => { + global.URL.createObjectURL = jest.fn(() => 'mock-url'); + global.URL.revokeObjectURL = jest.fn(); ;(generateIcsFileUrl as jest.Mock).mockResolvedValue(mockUrl)
85-113: Consider adding assertions for cleanup behavior.The test validates the download flow but doesn't verify that cleanup occurs (URL revocation and anchor removal). Since the cleanup logic was specifically added to fix a memory leak, testing it would add confidence.
await waitFor(() => { expect(button).not.toBeDisabled() }) + + // Verify cleanup + expect(global.URL.revokeObjectURL).toHaveBeenCalledWith(mockUrl) + expect(createdLink.parentNode).toBeNull() // anchor removed from DOM })
99-106: The link-finding logic is fragile; consider a simpler approach.Searching through
createSpy.mock.resultsby href works but is complex. A cleaner approach would be to capture the anchor directly when it's appended.- const createdLink = createSpy.mock.results.find( - (call) => call.value instanceof HTMLAnchorElement && call.value.href === mockUrl - )?.value - - expect(createdLink).toBeDefined() - expect(createdLink.download).toBe('invite.ics') - - expect(appendSpy).toHaveBeenCalledWith(createdLink) + // Get the anchor that was appended + const appendedElement = appendSpy.mock.calls[0]?.[0] as HTMLAnchorElement + expect(appendedElement).toBeInstanceOf(HTMLAnchorElement) + expect(appendedElement.href).toBe(mockUrl) + expect(appendedElement.download).toBe('invite.ics')
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/__tests__/e2e/pages/CalendarButton.spec.ts(1 hunks)frontend/__tests__/unit/components/CalendarButton.test.tsx(9 hunks)frontend/__tests__/unit/utils/getIcsFileUrl.test.ts(1 hunks)frontend/jest.setup.ts(1 hunks)frontend/src/components/CalendarButton.tsx(1 hunks)frontend/src/utils/getIcsFileUrl.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/tests/e2e/pages/CalendarButton.spec.ts
- frontend/src/utils/getIcsFileUrl.ts
- frontend/jest.setup.ts
- frontend/tests/unit/utils/getIcsFileUrl.test.ts
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/__tests__/unit/components/CalendarButton.test.tsx (1)
frontend/src/components/CalendarButton.tsx (1)
CalendarButton(9-59)
frontend/src/components/CalendarButton.tsx (1)
frontend/src/types/calendar.ts (1)
CalendarButtonProps(12-19)
🔇 Additional comments (5)
frontend/src/components/CalendarButton.tsx (2)
23-41: Good implementation of cleanup logic.The previous concern about memory leaks has been properly addressed. The anchor element is now removed and the blob URL is revoked in the
finallyblock, ensuring cleanup happens regardless of success or failure.
44-57: LGTM!The button implementation is well-structured with proper accessibility (
aria-label), loading state management (disabled={isDownloading}), and hover interactions for the icon toggle.frontend/__tests__/unit/components/CalendarButton.test.tsx (3)
48-82: LGTM!The rendering tests comprehensively cover the component's various rendering scenarios including default state, custom icons, and custom JSX elements.
115-129: Good error handling test coverage.The test properly verifies that errors are handled gracefully with an alert and that the button returns to an enabled state after failure.
132-144: LGTM!Accessibility tests and label prop tests properly verify the updated labels ("Add to Calendar" instead of "Add to Google Calendar") and the aria-label behavior.
Also applies to: 206-234
Utkarsh-0304
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/src/utils/getIcsFileUrl.ts (1)
6-9: Redundantwindowcheck after successful entry guard.The check at line 6 already guards against server-side execution by rejecting early. The callback at lines 49-54 will only execute if we passed line 6, making the second check unreachable in practice.
createEvent(eventAttributes, (error, value) => { if (error) { reject(error) return } - if (typeof globalThis.window !== 'undefined') { - const blob = new Blob([value], { type: 'text/calendar;charset=utf-8' }) - resolve(globalThis.URL.createObjectURL(blob)) - } else { - reject(new Error('Window not defined (server-side generation not supported)')) - } + const blob = new Blob([value], { type: 'text/calendar;charset=utf-8' }) + resolve(globalThis.URL.createObjectURL(blob)) })Also applies to: 49-54
frontend/__tests__/unit/components/CalendarButton.test.tsx (2)
8-8: Consider consistent naming with the source module.The import alias
generateIcsFileUrldiffers from the source function namegetIcsFileUrl. While valid, using the original name improves traceability.-import generateIcsFileUrl from 'utils/getIcsFileUrl' +import getIcsFileUrl from 'utils/getIcsFileUrl'Then update usages throughout the file (e.g., lines 11, 31, 91, 110).
28-43: Consider usingjest.spyOnfor URL methods to ensure proper cleanup.Lines 29-30 directly assign to
globalThis.URLmethods, butjest.restoreAllMocks()only restores spies. SincebeforeEachreassigns them for each test, this works within this file, but using spies would be more consistent with the pattern used forappendChild,createElement, etc.beforeEach(() => { - globalThis.URL.createObjectURL = jest.fn(() => 'mock-url'); - globalThis.URL.revokeObjectURL = jest.fn(); + jest.spyOn(globalThis.URL, 'createObjectURL').mockReturnValue('mock-url') + jest.spyOn(globalThis.URL, 'revokeObjectURL').mockImplementation(() => {}) ;(generateIcsFileUrl as jest.Mock).mockResolvedValue(mockUrl)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/__tests__/e2e/pages/CalendarButton.spec.ts(1 hunks)frontend/__tests__/unit/components/CalendarButton.test.tsx(9 hunks)frontend/__tests__/unit/utils/getIcsFileUrl.test.ts(1 hunks)frontend/src/utils/getIcsFileUrl.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/e2e/pages/CalendarButton.spec.ts
🔇 Additional comments (6)
frontend/__tests__/unit/utils/getIcsFileUrl.test.ts (2)
4-26: LGTM!The test setup correctly captures and restores
globalThis.URL.createObjectURL. The mock event provides comprehensive coverage of the expected input shape, and theafterEachcleanup ensures no mock state leaks between tests.
28-128: Solid test coverage for the ICS utility.The tests comprehensively cover:
- Successful blob URL generation
- String and Date object formatting (including 0-indexed month correction)
- Same-day event end date adjustment
- Error propagation from
createEvent- All event attribute passthrough
frontend/src/utils/getIcsFileUrl.ts (2)
10-26: LGTM on date handling logic.
formatDatecorrectly normalizes both string andDateinputs, handling the 0-indexed month fromDate.getMonth(). ThegetEndDatefunction properly usesDatearithmetic to handle month/year boundary rollover when incrementing the day for same-day events.
32-55: Clean implementation of ICS generation.The conditional spreading for optional fields (
description,location,url) keeps the attributes object minimal. The blob MIME typetext/calendar;charset=utf-8is correct for ICS files.frontend/__tests__/unit/components/CalendarButton.test.tsx (2)
81-122: Good coverage of the async download flow.The tests properly verify:
- Button disables during download
getIcsFileUrlis called with event data- Anchor element is created, appended, and clicked with correct attributes
- Button re-enables after success or failure
- Error path shows user-friendly alert
125-353: Comprehensive prop and scenario coverage.The tests thoroughly validate accessibility attributes, styling props, label behavior, icon extensibility, and real-world usage patterns. The transition from anchor to button element is consistently applied throughout.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/utils/getIcsFileUrl.ts (1)
10-17: Consider renaming localformatDateto avoid confusion with shared utilThere is already a
formatDatehelper infrontend/src/utils/dateFormatter.tswith different behavior. Using the same name here for a very ICS‑specific transformer can be confusing.Consider renaming this to something like
toIcsDateArray(and possibly moving it to a shared ICS/date helper later) to make intent clearer and avoid mental clashes with the generic formatter.frontend/__tests__/unit/components/CalendarButton.test.tsx (1)
28-31: Usejest.spyOnforURLmethods instead of direct reassignment
beforeEachreplacesglobalThis.URL.createObjectURLandglobalThis.URL.revokeObjectURLwithjest.fn, butafterEachonly callsjest.restoreAllMocks(), which won’t restore these direct assignments. This can leak mocked URL behavior into other tests.Consider switching to spies so
restoreAllMocksfully cleans up:- beforeEach(() => { - globalThis.URL.createObjectURL = jest.fn(() => 'mock-url') - globalThis.URL.revokeObjectURL = jest.fn() + beforeEach(() => { + jest.spyOn(globalThis.URL, 'createObjectURL').mockReturnValue('mock-url') + jest.spyOn(globalThis.URL, 'revokeObjectURL').mockImplementation(() => {}) ;(getIcsFileUrl as jest.Mock).mockResolvedValue(mockUrl) @@ - afterEach(() => { - jest.restoreAllMocks() - }) + afterEach(() => { + jest.restoreAllMocks() + })This keeps the behavior the same for this suite while avoiding cross‑test pollution.
Also applies to: 41-43
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/CalendarButton.test.tsx(9 hunks)frontend/src/utils/getIcsFileUrl.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/utils/getIcsFileUrl.ts (1)
frontend/src/utils/dateFormatter.ts (1)
formatDate(1-20)
🔇 Additional comments (2)
frontend/src/utils/getIcsFileUrl.ts (1)
19-30: Verify inclusive/exclusive semantics for multi‑day all‑day eventsICS all‑day events typically treat
endas exclusive. Here,getEndDateonly adds a day whenstart === end, but multi‑day events whereendDateis stored as an inclusive date (e.g., 2–3 Dec) would be passed through unchanged and may appear one day shorter in calendar clients.Please double‑check how
CalendarEvent.endDateis defined upstream (inclusive vs exclusive) and adjustgetEndDateor add tests accordingly if your domain model is inclusive for multi‑day all‑day events.frontend/__tests__/unit/components/CalendarButton.test.tsx (1)
81-127: ICS download and UI behavior tests look comprehensiveThe tests around the download flow, error handling, ARIA labels, className/icon behavior, and various layout scenarios are thorough and closely aligned with the new button‑based ICS UX. Assertions on disabled state,
getIcsFileUrlcalls, link creation, and accessibility attributes give good confidence in the component behavior.Also applies to: 129-357
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@Utkarsh-0304 please resolve conflicts 👍🏼
Also, there's one new issue introduced with this PR found by Sonar - please address that too
Utkarsh-0304
force-pushed
the
feat/ics_download_support
branch
from
c9e7e22 to
3eaf0d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/__tests__/e2e/pages/CalendarButton.spec.ts (1)
36-38: Consider using non-null assertion for type safety.After the truthy assertion, TypeScript may still consider
pathas potentiallynull. Usingpath!explicitly communicates the intent.const path = await download.path() expect(path, 'Expected Playwright to provide a download path').toBeTruthy() - const content = fs.readFileSync(path, 'utf-8') + const content = fs.readFileSync(path!, 'utf-8')
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
cspell/custom-dict.txt(1 hunks)frontend/__tests__/e2e/pages/CalendarButton.spec.ts(1 hunks)frontend/__tests__/e2e/pages/Home.spec.ts(1 hunks)frontend/__tests__/unit/components/CalendarButton.test.tsx(9 hunks)frontend/__tests__/unit/utils/getIcsFileUrl.test.ts(1 hunks)frontend/jest.setup.ts(1 hunks)frontend/package.json(1 hunks)frontend/src/components/CalendarButton.tsx(1 hunks)frontend/src/utils/getIcsFileUrl.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/jest.setup.ts
- frontend/package.json
- cspell/custom-dict.txt
- frontend/src/utils/getIcsFileUrl.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/e2e/pages/Home.spec.tsfrontend/__tests__/e2e/pages/CalendarButton.spec.ts
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/e2e/pages/Home.spec.ts
🧬 Code graph analysis (2)
frontend/src/components/CalendarButton.tsx (2)
frontend/src/types/calendar.ts (1)
CalendarButtonProps(12-19)frontend/src/utils/getIcsFileUrl.ts (1)
getIcsFileUrl(4-53)
frontend/__tests__/unit/components/CalendarButton.test.tsx (2)
frontend/src/utils/getIcsFileUrl.ts (1)
getIcsFileUrl(4-53)frontend/src/components/CalendarButton.tsx (1)
CalendarButton(9-60)
🪛 Biome (2.1.2)
frontend/__tests__/unit/components/CalendarButton.test.tsx
[error] 33-44: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (7)
frontend/src/components/CalendarButton.tsx (1)
1-59: Clean implementation of ICS download flow.The component correctly:
- Uses
'use client'directive for client-side functionality- Manages download state to prevent duplicate clicks
- Cleans up the temporary anchor and revokes the blob URL in the
finallyblock (addressing previous feedback)- Provides accessible button with appropriate aria-label
frontend/__tests__/e2e/pages/Home.spec.ts (1)
103-105: Test selectors correctly updated for button-based calendar control.The change from link to button selector aligns with the
CalendarButtoncomponent refactor. Usingexact: trueensures precise element matching.frontend/__tests__/e2e/pages/CalendarButton.spec.ts (1)
24-46: Solid e2e test for ICS download validation.The test correctly:
- Sets up download listener before triggering the click
- Validates the suggested filename
- Asserts download path availability before reading (addressing previous feedback)
- Validates essential ICS structure elements
frontend/__tests__/unit/utils/getIcsFileUrl.test.ts (2)
4-26: Proper restoration ofURL.createObjectURL(previous feedback addressed).The setup now correctly captures and restores
originalCreateObjectURLinstead of the entireURLobject, ensuring the mock is properly cleaned up.
28-128: Comprehensive test coverage forgetIcsFileUrl.The tests thoroughly validate:
- Successful blob URL generation
- String and Date object parsing with correct month handling
- Single-day event end date adjustment
- Error propagation
- Complete attribute forwarding to
createEventfrontend/__tests__/unit/components/CalendarButton.test.tsx (2)
86-131: Well-structured download functionality tests.The tests correctly validate:
- Button disabled state during download
getIcsFileUrlcalled with event data- Temporary anchor creation with correct
downloadattribute- Click triggered on the anchor
- Button re-enabled after completion
- Error handling with alert fallback
134-361: Thorough coverage of component props and edge cases.Tests comprehensively cover accessibility attributes, className/iconClassName application, label behavior, icon extensibility, reusability scenarios, and long title handling. All selectors correctly use button role matching the component refactor.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/CalendarButton.tsx (1)
30-30: Consider including event title in the download filename.The hardcoded
'invite.ics'works, but a more descriptive filename would improve UX when users have multiple calendar files in their downloads folder.- link.setAttribute('download', 'invite.ics') + const sanitizedTitle = safeTitle.replace(/[^a-zA-Z0-9-_]/g, '-').substring(0, 50) + link.setAttribute('download', `${sanitizedTitle}.ics`)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/CalendarButton.test.tsx(9 hunks)frontend/src/components/CalendarButton.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/CalendarButton.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/CalendarButton.test.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/CalendarButton.test.tsx
🧬 Code graph analysis (2)
frontend/src/components/CalendarButton.tsx (2)
frontend/src/types/calendar.ts (1)
CalendarButtonProps(12-19)frontend/src/utils/getIcsFileUrl.ts (1)
getIcsFileUrl(4-53)
frontend/__tests__/unit/components/CalendarButton.test.tsx (2)
frontend/src/utils/getIcsFileUrl.ts (1)
getIcsFileUrl(4-53)frontend/src/components/CalendarButton.tsx (1)
CalendarButton(8-62)
🔇 Additional comments (7)
frontend/__tests__/unit/components/CalendarButton.test.tsx (4)
1-42: Test setup looks good with proper mocking and cleanup.The test setup correctly mocks the ICS generation flow with:
getIcsFileUrlmock returning a blob URL- DOM manipulation spies for
appendChild,createElement, andclickalertmock for error handling tests- Proper cleanup via
jest.restoreAllMocks()inafterEach
77-122: Comprehensive download flow and error handling tests.The tests properly validate:
- Button disables during download (
expect(button).toBeDisabled())getIcsFileUrlis called with the event- Temporary anchor is created with correct
downloadattribute- Anchor is appended and clicked
- Button re-enables after completion
- Error path shows alert and keeps button enabled
125-137: Accessibility tests correctly updated for new labeling.The aria-label format
"Add {title} to Calendar"aligns with the component implementation. Tests verify both normal titles and fallback behavior.
199-227: Label tests correctly reflect the new default label.Tests properly verify:
- Default label "Add to Calendar" is hidden unless
showLabelis true- Custom labels override the default
- Labels are not rendered without
showLabelpropfrontend/src/components/CalendarButton.tsx (3)
1-6: Client directive and imports are appropriate.The
'use client'directive is correctly placed for this component that uses React hooks (useState) and browser APIs (document.createElement,URL.revokeObjectURL).
22-40: Download handler properly manages resources and state.The implementation correctly:
- Tracks download state to disable the button
- Creates a temporary anchor for programmatic download
- Cleans up the anchor element via
link.remove()- Revokes the blob URL to prevent memory leaks
- Resets state in
finallyblock ensuring cleanup on both success and failure
43-60: Button implementation is well-structured with proper accessibility.The button element includes:
type="button"to prevent accidental form submissiondisabled={isDownloading}to prevent duplicate downloads- Proper
aria-labelandtitlefor screen readers and tooltips- Preserved hover state icon swap behavior
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@Utkarsh-0304 thanks for working on this!
Works great 👍🏼
Left some requests for UI\UX.
There was a problem hiding this comment.
Can we change this to be a toast? I feel like that is a better UX.
There was a problem hiding this comment.
I have replaced 'alert' with 'heroui-toast' to enhance the user experience.
There was a problem hiding this comment.
Can we use event name here instead of hardcoded invite.ics?
There was a problem hiding this comment.
I have updated the code to use the slugify utility function to parse the event title and display that in place of a hardcoded value.
There was a problem hiding this comment.
Could you update the UI? After switching to a button the whole line is just jumbled there.
Here's current view on this branch:
And here's current view in prod:
There was a problem hiding this comment.
I adjusted the className attribute in the button tag to address this UI issue.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/__tests__/unit/components/CalendarButton.test.tsx (1)
84-112: Add explicit verification of resource cleanup.While the test verifies the button re-enables (indirectly confirming the finally block runs), it doesn't explicitly assert that
URL.revokeObjectURLandlink.remove()are called. These cleanup operations are critical for preventing memory leaks (as addressed in past reviews).🔎 Add assertions to verify cleanup
expect(clickSpy).toHaveBeenCalled() await waitFor(() => { expect(button).not.toBeDisabled() }) + + expect(globalThis.URL.revokeObjectURL).toHaveBeenCalledWith(mockUrl) + expect(createdLink.remove).toHaveBeenCalled()Note: You'll need to spy on the
removemethod when creating the link:const createdLink = createSpy.mock.results.find( (call) => call.value instanceof HTMLAnchorElement && call.value.href === mockUrl )?.value expect(createdLink).toBeDefined() + const removeSpy = jest.spyOn(createdLink, 'remove') expect(createdLink.download).toBe(`${slugify(mockEvent.title)}.ics`)frontend/src/components/CalendarButton.tsx (1)
36-43: Consider standard sentence capitalization for the toast description.The description starts with a lowercase letter:
"couldn't export your calendar...". While this may be intentional for visual hierarchy, standard sentence case would be"Couldn't export your calendar..."🔎 Apply standard capitalization
addToast({ - description: "couldn't export your calendar. Please try again.", + description: "Couldn't export your calendar. Please try again.", title: 'Download Failed',
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/e2e/pages/CalendarButton.spec.ts(1 hunks)frontend/__tests__/unit/components/CalendarButton.test.tsx(9 hunks)frontend/src/components/CalendarButton.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/e2e/pages/CalendarButton.spec.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/CalendarButton.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/CalendarButton.test.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/CalendarButton.test.tsx
🧬 Code graph analysis (2)
frontend/__tests__/unit/components/CalendarButton.test.tsx (2)
frontend/src/utils/getIcsFileUrl.ts (1)
getIcsFileUrl(4-53)frontend/src/components/CalendarButton.tsx (1)
CalendarButton(10-71)
frontend/src/components/CalendarButton.tsx (2)
frontend/src/types/calendar.ts (1)
CalendarButtonProps(12-19)frontend/src/utils/getIcsFileUrl.ts (1)
getIcsFileUrl(4-53)
🔇 Additional comments (6)
frontend/__tests__/unit/components/CalendarButton.test.tsx (3)
1-48: LGTM! Test setup is comprehensive and correct.The JSDOM environment directive, mocks for
getIcsFileUrlandaddToast, URL API mocks, and spies are all properly configured. The duplicatebeforeEachissue mentioned in past reviews has been resolved.
114-135: LGTM! Error handling test is comprehensive.The test properly verifies the error path: toast notification with correct content, and button re-enabling. Good coverage of the failure scenario.
50-363: LGTM! Test suite is comprehensive and well-structured.All tests have been properly updated to reflect the button-based implementation:
- Rendering tests verify button element and icons
- Accessibility tests ensure proper ARIA attributes
- Styling tests confirm className application
- Edge case tests cover long titles and flex container scenarios
The test structure follows project patterns and provides excellent coverage.
frontend/src/components/CalendarButton.tsx (3)
1-9: LGTM! Client directive and imports are appropriate.The
'use client'directive is necessary for this component's client-side state and DOM manipulation. All imports support the new ICS download functionality.
10-23: LGTM! State and label updates are appropriate.The
isDownloadingstate provides good UX by preventing multiple simultaneous downloads. Label changes correctly reflect the generic calendar support (ICS format works with any calendar app, not just Google Calendar).
51-71: LGTM! Button implementation is correct and accessible.The button properly:
- Prevents form submission with
type="button"- Disables during download for good UX
- Provides accessibility with
aria-labelandtitle- Combines className props correctly
- Maintains the hover-driven icon swap behavior
There was a problem hiding this comment.
@Utkarsh-0304 for this particular case you can add a logger and a comment for eslint to ignore this. I believe we have the same setup somewhere in the code already 👌🏼
| } catch { | |
| addToast({ | |
| } catch { | |
| // eslint-disable-next-line no-console | |
| console.error('Failed to download ICS file:', error) | |
| addToast({ |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/CalendarButton.tsx (1)
32-32: Consider usingsafeTitlefor download filename consistency.The download attribute uses
slugify(event.title)directly, while line 21 definessafeTitleas a fallback for empty titles. For consistency with the accessibility labels (line 22) and better handling of edge cases, consider usingsafeTitlehere:- link.setAttribute('download', `${slugify(event.title)}.ics`) + link.setAttribute('download', `${slugify(safeTitle)}.ics`)This ensures that if
event.titleis empty or undefined, the filename defaults toevent.icsinstead of just.ics.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/CalendarButton.test.tsxfrontend/src/components/CalendarButton.tsx
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/components/CalendarButton.tsx
📚 Learning: 2025-06-18T20:00:23.899Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1634
File: frontend/src/app/api/auth/[...nextauth]/route.ts:30-55
Timestamp: 2025-06-18T20:00:23.899Z
Learning: The OWASP Nest application has logging disabled, so avoid suggesting console.log, console.error, or any other logging statements in code review suggestions.
Applied to files:
frontend/src/components/CalendarButton.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/CalendarButton.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/CalendarButton.test.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/CalendarButton.test.tsx
🧬 Code graph analysis (1)
frontend/src/components/CalendarButton.tsx (2)
frontend/src/types/calendar.ts (1)
CalendarButtonProps(12-19)frontend/src/utils/getIcsFileUrl.ts (1)
getIcsFileUrl(4-53)
🔇 Additional comments (5)
frontend/__tests__/unit/components/CalendarButton.test.tsx (3)
1-48: LGTM! Test setup is comprehensive.The test setup properly mocks all dependencies (
getIcsFileUrl,@heroui/toast, URL APIs) and creates spies for DOM interactions. ThebeforeEachandafterEachhooks correctly initialize and restore mocks for each test.
84-141: Excellent coverage of download flow and error handling.The tests thoroughly verify:
- The complete async download workflow with proper state management (disabled during download)
- DOM manipulation (anchor creation, attributes, append, click)
- Error handling with toast notification and console logging
- Cleanup and state restoration in both success and failure paths
The use of
waitForappropriately handles the asynchronous nature of the download flow.
50-369: Well-structured tests with thorough coverage.The test suite comprehensively covers:
- Rendering variations (default icon, custom icons, JSX elements)
- All prop combinations (className, iconClassName, showLabel, label)
- Accessibility attributes and ARIA labels with calendar-agnostic wording
- Reusability scenarios for different contexts (homepage, poster page)
- Edge cases (long titles, flex container interactions)
All tests properly updated from anchor-based to button-based assertions with appropriate role queries and element checks.
frontend/src/components/CalendarButton.tsx (2)
24-51: Download handler is well-implemented with proper cleanup.The
handleDownloadfunction correctly:
- Manages download state with
isDownloading- Creates and triggers a temporary anchor for download
- Handles errors with toast notification and logging (with appropriate eslint disable)
- Cleans up DOM elements and revokes object URLs in the
finallyblock- Re-enables the button after completion or error
The resource management and error handling are solid.
53-72: Button implementation is accessible and properly configured.The button element correctly:
- Uses semantic
type="button"to prevent form submission- Binds the
handleDownloadhandler toonClick- Disables during download with the
isDownloadingstate- Provides accessible labels with
aria-labelandtitleattributes- Maintains hover interaction for icon animation
- Combines user-provided
classNamewith layout classesThe transition from anchor to button preserves all intended functionality while providing better semantics for a download action.
| } finally { | ||
| if (link) link.remove() | ||
| if (url) URL.revokeObjectURL(url) | ||
| setIsDownloading(false) |
There was a problem hiding this comment.
@Utkarsh-0304 this works great! One more suggestion - can we show a success toast on successful download of the file? It happens so quickly that user can easily miss anything was downloaded.
| } finally { | |
| if (link) link.remove() | |
| if (url) URL.revokeObjectURL(url) | |
| setIsDownloading(false) | |
| } finally { | |
| if (link) link.remove() | |
| if (url) URL.revokeObjectURL(url) | |
| setIsDownloading(false) | |
| addToast({ | |
| description: 'Successfully downloaded ICS file', | |
| title: `${event.title}`, | |
| timeout: 3000, | |
| shouldShowTimeoutProgress: true, | |
| color: 'success', | |
| variant: 'solid', | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/unit/components/CalendarButton.test.tsxfrontend/src/components/CalendarButton.tsxfrontend/tsconfig.json
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/components/CalendarButton.tsx
📚 Learning: 2025-06-18T20:00:23.899Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1634
File: frontend/src/app/api/auth/[...nextauth]/route.ts:30-55
Timestamp: 2025-06-18T20:00:23.899Z
Learning: The OWASP Nest application has logging disabled, so avoid suggesting console.log, console.error, or any other logging statements in code review suggestions.
Applied to files:
frontend/src/components/CalendarButton.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/CalendarButton.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/CalendarButton.test.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/CalendarButton.test.tsx
🧬 Code graph analysis (1)
frontend/__tests__/unit/components/CalendarButton.test.tsx (2)
frontend/src/utils/getIcsFileUrl.ts (1)
getIcsFileUrl(4-53)frontend/src/components/CalendarButton.tsx (1)
CalendarButton(10-81)
🔇 Additional comments (5)
frontend/__tests__/unit/components/CalendarButton.test.tsx (1)
1-49: LGTM! Test setup is well-structured.The test environment configuration, mocks, and lifecycle hooks are properly configured. The duplicate
beforeEachissue from the previous review has been addressed.frontend/tsconfig.json (1)
14-14: LGTM! JSX configuration aligns with React 19.The change to
"react-jsx"enables the automatic JSX runtime, which is the recommended approach for React 19 and eliminates the need to import React in every component file.frontend/src/components/CalendarButton.tsx (3)
1-23: LGTM! Component setup and state management.The imports, state declarations, and accessibility labels are well-structured and align with the ICS download functionality.
35-45: LGTM! Error handling with user feedback.The error handling correctly logs the error (as requested in past reviews) and displays an appropriate error toast to the user.
62-80: LGTM! Button implementation with accessibility.The button is properly implemented with:
- Correct type and click handling
- Disabled state during download
- Accessible labels and titles
- Hover state interaction
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/__tests__/unit/components/CalendarButton.test.tsx (1)
122-149: Test gap: Still missing assertion that success toast is NOT called on error.This test verifies the error toast is shown but doesn't assert that the success toast is NOT called. This is the same gap flagged in the previous review (lines 122-149) and remains unaddressed. Without this assertion, a regression where the success toast appears in the finally block would pass this test.
🔎 Add assertion to verify only error toast called
After line 145, add:
expect(consoleSpy).toHaveBeenCalledWith( expect.stringContaining('Failed to download ICS file'), errorMock ) + // Verify only the error toast was called, not the success toast + expect(addToast).toHaveBeenCalledTimes(1) })Alternatively, explicitly verify the success toast was not called:
expect(button).not.toBeDisabled() + + // Verify success toast was NOT called + expect(addToast).not.toHaveBeenCalledWith( + expect.objectContaining({ + color: 'success', + title: mockEvent.title, + }) + ) consoleSpy.mockRestore()
🧹 Nitpick comments (1)
frontend/src/components/CalendarButton.tsx (1)
46-53: Consider using event title in error toast for consistency.The success toast uses the event title (line 37:
title: ${event.title}), while the error toast uses a generic "Download Failed" title. For consistency and better user context, consider using the event title in the error toast as well.🔎 Suggested improvement
addToast({ description: 'Failed to download ICS file', - title: 'Download Failed', + title: event.title, timeout: 3000, shouldShowTimeoutProgress: true, color: 'danger', variant: 'solid', })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/CalendarButton.test.tsxfrontend/src/components/CalendarButton.tsx
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/CalendarButton.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/CalendarButton.test.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/CalendarButton.test.tsx
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/components/CalendarButton.tsx
📚 Learning: 2025-06-18T20:00:23.899Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1634
File: frontend/src/app/api/auth/[...nextauth]/route.ts:30-55
Timestamp: 2025-06-18T20:00:23.899Z
Learning: The OWASP Nest application has logging disabled, so avoid suggesting console.log, console.error, or any other logging statements in code review suggestions.
Applied to files:
frontend/src/components/CalendarButton.tsx
🧬 Code graph analysis (2)
frontend/__tests__/unit/components/CalendarButton.test.tsx (2)
frontend/src/utils/getIcsFileUrl.ts (1)
getIcsFileUrl(4-53)frontend/src/components/CalendarButton.tsx (1)
CalendarButton(10-81)
frontend/src/components/CalendarButton.tsx (2)
frontend/src/types/calendar.ts (1)
CalendarButtonProps(12-19)frontend/src/utils/getIcsFileUrl.ts (1)
getIcsFileUrl(4-53)
🔇 Additional comments (1)
frontend/src/components/CalendarButton.tsx (1)
35-42: Success toast correctly placed in try block.The success toast is now properly positioned within the try block, ensuring it only fires after a successful download trigger. This correctly addresses the critical issue from the previous review where the toast was in the finally block and would show even on errors.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/__tests__/unit/components/CalendarButton.test.tsx (3)
43-43: Optional: Remove unused alert mock.The
globalThis.alertmock is defined but never asserted or used in any test. Consider removing it to keep the test setup clean.🔎 Proposed cleanup
clickSpy = jest.spyOn(HTMLAnchorElement.prototype, 'click').mockImplementation(() => {}) - - jest.spyOn(globalThis, 'alert').mockImplementation(() => {}) })
84-120: Verify URL cleanup in download flow.The test comprehensively covers the download flow but doesn't assert that
URL.revokeObjectURLis called. The component should revoke the object URL after download to prevent memory leaks.🔎 Add assertion for URL cleanup
After line 104 (after verifying the click), add:
expect(clickSpy).toHaveBeenCalled() + expect(globalThis.URL.revokeObjectURL).toHaveBeenCalledWith(mockUrl) expect(addToast).toHaveBeenCalledWith({
154-154: Optional: Remove redundant mockRestore.The explicit
consoleSpy.mockRestore()is redundant sincejest.restoreAllMocks()is already called in theafterEachblock (line 47), which will restore all mocks including this spy.🔎 Proposed cleanup
) - consoleSpy.mockRestore() })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/components/CalendarButton.test.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/CalendarButton.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/CalendarButton.test.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Applied to files:
frontend/__tests__/unit/components/CalendarButton.test.tsx
🧬 Code graph analysis (1)
frontend/__tests__/unit/components/CalendarButton.test.tsx (2)
frontend/src/utils/getIcsFileUrl.ts (1)
getIcsFileUrl(4-53)frontend/src/components/CalendarButton.tsx (1)
CalendarButton(10-81)
🔇 Additional comments (1)
frontend/__tests__/unit/components/CalendarButton.test.tsx (1)
148-153: Excellent addition: Success toast assertion in error path.This properly addresses the past review comment. The test now verifies that the success toast is NOT called when an error occurs, catching any bugs where the success toast might be shown in the
finallyblock.
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@Utkarsh-0304 Thanks for working on this 👍🏼
* fix:resolve conflicts & failing e2e tests * fix: coderabbit suggestions * fix:sonarqube_fix * fix:coderabbit_nitpicks * fix:formatting * fix:make check issues * fix:coderabbit suggestions and merge conflicts * fix:ui changes and download file name updation * fix:coderabbit suggestions * feat: display toast for successful download * fix: coderabbit suggestions * Update tests * Fix tests --------- Co-authored-by: Kate <kate@kgthreads.com>
3 participants
Proposed change
Resolves #2841
This PR adds ICS file download support (using ics package). This rules out existing Google Calendar link functionality and unifies it for any type of calendar service. Additionally, unit and e2e tests are added.
Checklist
make check-testlocally and all tests passed