feat: improve attendee notes access and display - #1031
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesAttendee and notes panel refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EditTicketPage
participant NotesPanel
participant NotesAPI
EditTicketPage->>NotesPanel: pass controlled visibility and identifiers
NotesPanel->>NotesAPI: fetch notes
NotesAPI-->>NotesPanel: return notes
NotesPanel->>EditTicketPage: request panel open
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/notes/notes-panel.js`:
- Around line 58-61: Update the confirmation message construction in the delete
flow around the msg object to remove the extra literal separator before noteId,
relying on the trailing space provided by
T.translate("notes_panel.remove_warning"). Preserve the existing translated
warning text and note ID in the rendered dialog.
- Around line 123-125: Update the showColumns calculation to call the standard
columns.includes method when filtering tableColumns, replacing the invalid
columns.include call while preserving the existing fallback to all tableColumns.
- Around line 41-51: Update the notes-loading useEffect to depend on attendeeId,
ticketId, open, and onToggle, while preventing completions from previous
requests from toggling current accordion state or updating stale data; retain
cleanup via clearNotesParams. Correct the columns method to includes and remove
the extra space in the delete warning text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 473fef30-6a90-4c97-ad66-01cedbc8d9c0
📒 Files selected for processing (4)
src/components/forms/attendee-form/attendee-form.jssrc/components/notes/notes-panel.jssrc/i18n/en.jsonsrc/pages/orders/edit-ticket-page.js
| useEffect(() => { | ||
| getNotes(attendeeId, ticketId, term, 1, perPage, order, orderDir).then( | ||
| () => { | ||
| if (!open) onToggle(); | ||
| } | ||
| ); | ||
|
|
||
| return () => { | ||
| clearNotesParams(); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for key-based remounting of AttendeeForm/EditTicketPage on id changes.
rg -n '<AttendeeForm' -C5
rg -n '<EditTicketPage' -C5 --type=js
rg -n 'ticket_id' -C3 src/pages/orders/edit-ticket-page.jsRepository: fntechgit/summit-admin
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- NotesPanel implementation ---'
sed -n '1,220p' src/components/notes/notes-panel.js
printf '%s\n' '--- AttendeeForm call site ---'
sed -n '390,455p' src/components/forms/attendee-form/attendee-form.js
printf '%s\n' '--- EditTicketPage call site and effects ---'
sed -n '1,100p' src/pages/orders/edit-ticket-page.js
sed -n '425,475p' src/pages/orders/edit-ticket-page.js
printf '%s\n' '--- NotesPanel references and key props ---'
rg -n -C5 'NotesPanel|<AttendeeForm|<EditTicketPage|key=' src --glob '*.js'
printf '%s\n' '--- columns prop usage ---'
rg -n -C3 'columns|include\\(' src/components/notes src --glob '*.js'
printf '%s\n' '--- remove_warning translations ---'
rg -n -C2 'remove_warning' .Repository: fntechgit/summit-admin
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- AttendeeForm state and toggleSection ---'
rg -n -C8 'toggleSection|openSections|useState' src/components/forms/attendee-form/attendee-form.js | head -160
printf '%s\n' '--- AttendeeForm and EditTicketPage route declarations ---'
rg -n -C6 'attendee-form|AttendeeForm|edit-ticket-page|EditTicketPage|ticket/:|ticket_id|attendee_id' src --glob '*.js' --glob '*.jsx' | head -240
printf '%s\n' '--- All NotesPanel call sites only ---'
rg -n -C4 'NotesPanel' src --glob '*.js' --glob '*.jsx'
printf '%s\n' '--- Translation files containing remove_warning ---'
rg -l '"remove_warning"' . | while IFS= read -r file; do
printf '%s\n' "--- $file"
rg -n -C2 '"remove_warning"' "$file"
done
printf '%s\n' '--- Notes state shape and actions ---'
rg -n -C6 'notesState|columns:|getNotes|clearNotesParams' src/actions src/reducers src --glob '*.js' | head -260Repository: fntechgit/summit-admin
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- AttendeeForm consumers and route paths ---'
rg -n -C5 'AttendeeForm|EditTicketPage|edit-ticket|edit_attendee|attendees?/|tickets?/' src/pages src/routes src --glob '*.js' --glob '*.jsx' 2>/dev/null | head -260
printf '%s\n' '--- Notes reducer and action request completion ---'
sed -n '1,230p' src/reducers/notes-reducer.js 2>/dev/null || true
sed -n '1,195p' src/actions/notes-actions.js
printf '%s\n' '--- Targeted static verifier ---'
python3 - <<'PY'
import json
from pathlib import Path
notes = Path("src/components/notes/notes-panel.js").read_text()
translation = json.loads(Path("src/i18n/en.json").read_text())
warning = translation["notes_panel"]["remove_warning"]
print("effect_has_empty_deps:", " }, []);" in notes)
print("uses_columns_include:", "columns.include(c.columnKey)" in notes)
print("remove_warning_repr:", repr(warning))
print("rendered_delete_text_repr:", repr(f"{warning} 123"))
print("double_space_in_rendered_delete_text:", " " in f"{warning} 123")
PYRepository: fntechgit/summit-admin
Length of output: 23264
Refetch notes when attendeeId or ticketId changes. The empty dependency array captures stale identifiers and accordion state. Use current open and onToggle, and ignore completions from previous requests. Also replace columns.include(...) with columns.includes(...) and remove the extra space in the delete warning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/notes/notes-panel.js` around lines 41 - 51, Update the
notes-loading useEffect to depend on attendeeId, ticketId, open, and onToggle,
while preventing completions from previous requests from toggling current
accordion state or updating stale data; retain cleanup via clearNotesParams.
Correct the columns method to includes and remove the extra space in the delete
warning text.
There was a problem hiding this comment.
@santipalenque Confirmed this part applies. Verified that EditSummitAttendeePage.componentDidUpdate and EditTicketPage's useEffect(..., [match.params.ticket_id]) both re-fetch the entity without unmounting the tree when the route id changes — so NotesPanel can stay mounted while showing a different attendee's/ticket's notes.
That said, it's not a regression introduced by this PR: the previous Notes component had the exact same useEffect(() => { getNotes(...); ... }, []) with no attendeeId/ticketId in its deps, so this predates this change. Worth fixing while the file is already being touched, but flagging it as inherited rather than newly introduced.
There was a problem hiding this comment.
@santipalenque Fixed in 7ac5250. <NotesPanel> is now keyed by the attendee/ticket identifiers at both call sites (key={entity.id} in attendee-form.js, key={${entity.owner.id}-${entity.id}} in edit-ticket-page.js), so it remounts — and refetches — instead of staying mounted across navigation with a stale mount-only effect. Went with key rather than widening the effect's deps to avoid the different regression already flagged in this thread (re-running getNotes/clearNotesParams on every panel toggle if open/onToggle were added too). Covered by a new regression test (attendee-form-notes-remount.test.js) that fails without the key and passes with it.
There was a problem hiding this comment.
Pull request overview
This PR improves how admin/attendee notes are accessed and displayed by introducing a dedicated NotesPanel component and wiring it into both ticket editing and attendee editing flows, alongside related i18n key updates.
Changes:
- Replaced the previous notes section usage with a reusable
NotesPanelin ticket and attendee editing screens. - Refactored
AttendeeFormfrom a class component to a function component with hooks and independent section open/close state. - Updated English translations to support the new notes panel labels/messages and moved the “note created” success message key.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/pages/orders/edit-ticket-page.js | Switches ticket admin notes UI to the new NotesPanel and makes section toggle event handling null-safe. |
| src/i18n/en.json | Adjusts translation keys for notes vs. notes panel and removes unused admin notes labels. |
| src/components/notes/notes-panel.js | Introduces/renames the notes UI into a dedicated panel component with table/search/export behaviors. |
| src/components/forms/attendee-form/attendee-form.js | Converts attendee form to hooks, updates section toggling, and embeds NotesPanel for attendee notes. |
Suppressed comments (2)
src/components/notes/notes-panel.js:46
- The initial load effect toggles the panel after the getNotes promise resolves using the initial
openvalue from the mount render. If the user opens/closes the panel while the request is in flight, this callback can flip the panel back to the opposite state unexpectedly.
src/components/notes/notes-panel.js:125 columns.include(...)is not a standard Array API (the standard method isincludes). Ifcolumnsis ever provided as an array (the likely intent), this will throw at runtime.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/forms/attendee-form/__tests__/attendee-form.test.js (1)
27-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the controlled
NotesPanelwiring.This null mock prevents the suite from detecting regressions in the new NotesPanel visibility, identifier, and close-callback wiring. Use a spy component that records the received props and assert the visibility transition owned by
AttendeeForm.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/forms/attendee-form/__tests__/attendee-form.test.js` around lines 27 - 30, Replace the null mock for NotesPanel in the attendee-form tests with a spy component that captures its received props. Add assertions around AttendeeForm to verify the panel’s visibility, attendee identifier, and close callback, including the visibility transition when the form opens and closes the notes panel.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/orders/edit-ticket-page.js`:
- Around line 461-467: Update the NotesPanel usage in EditTicketPage to key the
component by both attendeeId and ticketId, ensuring it remounts when either
identifier changes and resets its fetched notes state for the new ticket.
---
Nitpick comments:
In `@src/components/forms/attendee-form/__tests__/attendee-form.test.js`:
- Around line 27-30: Replace the null mock for NotesPanel in the attendee-form
tests with a spy component that captures its received props. Add assertions
around AttendeeForm to verify the panel’s visibility, attendee identifier, and
close callback, including the visibility transition when the form opens and
closes the notes panel.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef064c7a-44e8-4e87-8cd4-b420e8956f11
📒 Files selected for processing (4)
src/components/forms/attendee-form/__tests__/attendee-form.test.jssrc/components/forms/attendee-form/attendee-form.jssrc/components/notes/notes-panel.jssrc/pages/orders/edit-ticket-page.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/forms/attendee-form/attendee-form.js
- src/components/notes/notes-panel.js
1b79c8b to
12480be
Compare
|
/deploy-preview |
|
🚀 Deployed on https://pr-1031--show-admin-preview.netlify.app |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/notes/__tests__/notes-panel.test.js`:
- Around line 10-18: Update the i18n mock in the notes-panel regression test so
notes_panel.remove_warning resolves from the real English translation resource,
preserving its trailing space instead of hard-coding a trimmed string. Keep the
existing fallback behavior for other translation keys.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3926cb3-d6ad-482c-808c-a0087f7b83af
📒 Files selected for processing (3)
src/components/forms/attendee-form/attendee-form.jssrc/components/notes/__tests__/notes-panel.test.jssrc/i18n/en.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/i18n/en.json
- src/components/forms/attendee-form/attendee-form.js
NotesPanel fetched notes in a mount-only effect and neither AttendeeForm nor EditTicketPage remount when the route's attendee_id or ticket_id changes (both refetch the entity via effects/ componentDidUpdate without unmounting). Navigating between tickets in the same order, or between attendees, left the previous entity's notes displayed while save/delete actions used the freshly-rendered attendeeId/ticketId — a mismatch that could target the wrong attendee's notes. Key NotesPanel by the attendee/ticket identifiers at both call sites so it remounts (and refetches) on change, instead of widening the fetch effect's deps, which would re-run getNotes on every panel toggle. Adds a regression test asserting getNotes is called again with the new attendeeId when AttendeeForm's entity prop switches identity.
columns.include(...) is not a valid method (only .includes exists), so passing a columns prop to NotesPanel would throw TypeError: columns.include is not a function. No current caller passes columns, so this was dormant, but the file was already being touched by the recent notes-panel rename/rewrite. Adds a regression test rendering NotesPanel with a columns prop and asserting it no longer throws.
"dint" -> "didn't". Comment-only, no functional change.
The delete-note confirmation dialog built its text as
`${T.translate("notes_panel.remove_warning")} ${noteId}`, and the
translation already ended with a trailing space, so the rendered
message showed two spaces between "note" and the id.
Adds a regression test that triggers the delete-confirmation flow and
asserts the dialog text has no double space.
54db549 to
78c2a3d
Compare
|
/deploy-preview |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/components/notes/notes-panel.js:96
- Search requests should reset pagination to page 1; otherwise searching while on a later page can return empty/partial results even when matches exist on earlier pages.
src/components/notes/notes-panel.js:167 termis coming fromnotesState, but the notes reducer doesn’t store it (seesrc/reducers/notes/notes-reducer.jsDEFAULT_STATE and RECEIVE_NOTES). This means the search box value will always be blank and pagination/export will drop the active filter term after a search.
src/components/forms/attendee-form/attendee-form.js:142- The required-field validation message is hardcoded; this bypasses the existing i18n validation strings used elsewhere (e.g.,
src/utils/yup.js:51). Using the translation key keeps messaging consistent and localizable.
if (!entity.member) {
required.forEach((fieldId) => {
if (!entity[fieldId]) {
newErrors[fieldId] = "This field is required";
}
src/components/forms/attendee-form/tests/attendee-form.test.js:69
- After switching AttendeeForm to use the i18n validation key, this test should assert the translated key (per the current i18n mock) instead of the previous hardcoded English string.
await waitFor(() =>
expect(screen.getAllByText("This field is required")).toHaveLength(3)
);
https://app.clickup.com/t/9014802374/86b72t1x0
Summary by CodeRabbit
Enhancements
Bug Fixes