fix: prevent comment overwrite on note save + improve maintenance UX#122
Merged
ryota-murakami merged 2 commits intomainfrom Feb 17, 2026
Merged
fix: prevent comment overwrite on note save + improve maintenance UX#122ryota-murakami merged 2 commits intomainfrom
ryota-murakami merged 2 commits intomainfrom
Conversation
P0: useNoteModal.save() was hardcoding comment: '' in upsertProjectInfo, destroying existing comments whenever a user saved a note. Made comment optional in ProjectInfoData so upsert skips it when undefined. P1: Replace inline DropdownMenu in maintenance grid view with shared OverflowMenu component. Add keyboard shortcuts (., Enter, Escape) to maintenance cards matching the RepoCard pattern. Add onDelete prop to OverflowMenu for maintenance context.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughReplaced per-card DropdownMenu with a new OverflowMenu and per-card openMenuId state; added keyboard shortcuts ('.' to toggle menu, Enter to open note modal, Escape to close); removed Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MaintenanceClient as MaintenanceClient (UI)
participant OverflowMenu as OverflowMenu (menu)
participant NoteModal as NoteModal (modal)
participant API as UpsertProjectInfo (backend)
User->>MaintenanceClient: Focus / press '.' on card
MaintenanceClient->>OverflowMenu: toggle openMenuId -> open
User->>OverflowMenu: Select "Add/Edit Note"
OverflowMenu->>MaintenanceClient: request open note modal (cardId)
MaintenanceClient->>NoteModal: show modal (selectedRepoForNote, note)
User->>NoteModal: Submit note
NoteModal->>API: upsertProjectInfo({ note, links }) -- (no empty comment)
API-->>NoteModal: success
NoteModal-->>MaintenanceClient: close modal / update UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
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 unit tests (beta)
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 |
🤖 Morph Preview Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #122 +/- ##
==========================================
- Coverage 72.21% 72.12% -0.09%
==========================================
Files 143 143
Lines 4214 4233 +19
Branches 1097 1137 +40
==========================================
+ Hits 3043 3053 +10
- Misses 1150 1159 +9
Partials 21 21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 (1)
src/components/Board/OverflowMenu.tsx (1)
209-223: Delete action skips confirmation dialog — intentional delegation to caller.Unlike
onRemovewhich uses the internalAlertDialog,onDeletefires directly. This works becauseMaintenanceClientwraps it withDeleteMaintenanceDialog. Just noting the asymmetry — ifonDeleteis ever reused without an external confirmation dialog, items will be deleted without warning.A brief JSDoc note on the prop (e.g., "caller is responsible for confirmation") would make this contract explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Board/OverflowMenu.tsx` around lines 209 - 223, The Delete menu item calls onDelete(cardId) directly which relies on the caller to present confirmation (unlike onRemove which uses the internal AlertDialog); add a JSDoc comment to the onDelete prop in the OverflowMenu component clearly stating that the caller is responsible for presenting a confirmation dialog (e.g., reference that MaintenanceClient uses DeleteMaintenanceDialog) so future consumers know the contract and won't accidentally delete without confirmation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/maintenance/MaintenanceClient.tsx`:
- Around line 167-183: Remove the dead check e.key === 'Period' inside
handleCardKeyDown and only treat the period shortcut as e.key === '.'; also
prevent that shortcut from toggling openMenuId when the component is in list
view by gating the setOpenMenuId call with the view state (e.g., check
isListView or viewMode before calling setOpenMenuId), or alternatively render
the OverflowMenu in list view for consistent behavior; keep the Enter and Escape
behavior unchanged (openNoteModal and clearing openMenuId) and ensure references
to openMenuId and OverflowMenu are used to locate where to apply the guard.
---
Nitpick comments:
In `@src/components/Board/OverflowMenu.tsx`:
- Around line 209-223: The Delete menu item calls onDelete(cardId) directly
which relies on the caller to present confirmation (unlike onRemove which uses
the internal AlertDialog); add a JSDoc comment to the onDelete prop in the
OverflowMenu component clearly stating that the caller is responsible for
presenting a confirmation dialog (e.g., reference that MaintenanceClient uses
DeleteMaintenanceDialog) so future consumers know the contract and won't
accidentally delete without confirmation.
CodeRabbit review: e.key === 'Period' is dead code (e.key yields '.', not 'Period'). Also gate the dot shortcut to grid view only since list view doesn't render OverflowMenu.
🧪 E2E Coverage Report (Sharded: 12 parallel jobs)
📊 Full report available in workflow artifacts |
🤖 Morph Preview Test⚡ Looks like you hit your rate limits! Please upgrade your limits here, or wait a few minutes and try again. If you need help, reach out to support@morphllm.com. Automated testing by Morph |
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
2 participants
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
useNoteModal.save()was hardcodingcomment: ''inupsertProjectInfo, destroying existing comments whenever a user saved a note. Madecommentoptional inProjectInfoDataso the upsert skips the field whenundefined.DropdownMenuin maintenance grid view with the sharedOverflowMenucomponent. AddedonDeleteprop toOverflowMenufor maintenance context. Added keyboard shortcuts (.menu toggle,Enteropen note,Escapeclose menu) to maintenance cards matching theRepoCardpattern.Changes
shared-project-info.tsProjectInfoData.comment→ optional; upsert skips whenundefineduseNoteModal.tscomment: ''from save (the bug)useMaintenanceNoteModal.tscommentsprop workaroundOverflowMenu.tsxonDeleteprop with destructive styling for maintenanceMaintenanceClient.tsxtabIndex+onKeyDownkeyboard shortcutsuseNoteModal.test.tscommentin save call)MaintenanceClient.test.tsxwindow.openassertion fornoopener,noreferrerTest plan
pnpm typecheck— passpnpm lint— pass (zero warnings)pnpm test— 1282/1282 passpnpm build— passpnpm e2e:parallel— no maintenance/note-modal regressions (pre-existing DB timeout flakes in unrelated tests)Summary by CodeRabbit
New Features
Improvements
Tests