feat(ZMSKVR-1377): improve ui of opening hours#2592
Open
matthilo96 wants to merge 6 commits into
Open
Conversation
Contributor
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (3)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe availability day page's validation and conflict-checking logic is refactored to use Promise-based flows with immediate state updates on mutations. ChangesAvailabilityDay validation, conflict, and UI flow refactor
Sequence Diagram(s)sequenceDiagram
participant User
participant onPublishAvailability
participant getValidationList
participant SaveHandler as Save Handler
participant refreshData
participant getConflictList
participant API as Backend API
User->>onPublishAvailability: click publish button
onPublishAvailability->>getValidationList: { scrollToErrors: true }
getValidationList-->>onPublishAvailability: errorList (scrolls if blocking)
alt blocking errors exist
onPublishAvailability->>onPublishAvailability: select first invalid availability
onPublishAvailability-->>User: abort, validation errors visible
else no blocking errors
onPublishAvailability->>SaveHandler: proceed to onSaveUpdates
SaveHandler->>API: POST availability updates
API-->>SaveHandler: success response
SaveHandler->>refreshData: await Promise
refreshData->>API: fetch day and conflicts
API-->>refreshData: merged state
refreshData-->>SaveHandler: resolved
SaveHandler->>getConflictList: { scrollToErrors: false }
getConflictList->>API: fetch POST conflict check
API-->>getConflictList: conflictList
getConflictList-->>SaveHandler: resolved
SaveHandler->>SaveHandler: derive first conflicted availability
SaveHandler->>SaveHandler: scrollToSuccessMessage
SaveHandler-->>User: success shown, spinner hidden
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
zmsadmin/js/page/availabilityDay/form/footerButtons.js (1)
6-17: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRemove unused props
hasErrors,hasSlotCountError, anddata.These props are destructured but never used in the component body after the disabled-predicate simplification. The corresponding PropTypes declarations should also be removed to complete the cleanup.
♻️ Proposed fix to remove unused props
const FooterButtons = (props) => { const { hasConflicts, - hasErrors, stateChanged, - data, onNew, onPublish, onAbort, - hasSlotCountError, availabilitylist, selectedDate } = props;FooterButtons.propTypes = { - data: PropTypes.object, hasConflicts: PropTypes.bool, - hasErrors: PropTypes.bool, stateChanged: PropTypes.bool, onNew: PropTypes.func, onPublish: PropTypes.func, onAbort: PropTypes.func, - hasSlotCountError: PropTypes.bool, availabilitylist: PropTypes.array, selectedDate: PropTypes.number.isRequired }Note: After removing these props from FooterButtons, the parent component (
accordion.jsaround line 200-210) should also stop computing and passing them to avoid unnecessary work.Also applies to: 58-69
🤖 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 `@zmsadmin/js/page/availabilityDay/form/footerButtons.js` around lines 6 - 17, Remove the unused props hasErrors, hasSlotCountError, and data from the destructuring of props in the footerButtons component. These props are extracted but never referenced in the component body. Additionally, remove the corresponding PropTypes declarations for these three props from the component's PropTypes definition. After making these changes to FooterButtons, also update the parent component accordion.js (around lines 200-210) to stop computing and passing these unused props to FooterButtons to eliminate unnecessary work.Source: Coding guidelines
🤖 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 `@zmsadmin/js/page/availabilityDay/index.js`:
- Line 123: Replace the console.log call used for error logging in the
refreshData error handler with console.error instead. Since this is in an
error/catch path, console.error is the appropriate standard logging method per
coding guidelines. The same change should be applied wherever console.log is
used for logging errors in failure paths throughout the file.
- Around line 678-719: The fetch promise chain does not validate the HTTP
response status before calling .json(), which allows error responses to silently
resolve as empty conflict lists. After the fetch response is received, check
res.ok before processing the JSON; if the response status indicates an error,
read the response text and throw an error object with status and message
properties so the catch block can handle it properly. This ensures validation
failures are not silently masked. Additionally, replace the console.log call on
line 710 with console.error to properly log the error condition.
- Around line 68-70: The lifecycle method `componentDidUnMount` is not a valid
React class component hook, so React never calls it and the `beforeunload` event
listener registered in `componentDidMount` is never removed. Rename the method
from `componentDidUnMount` to `componentWillUnmount` (the correct React
lifecycle method name) so that the event listener cleanup code properly executes
when the component unmounts.
---
Outside diff comments:
In `@zmsadmin/js/page/availabilityDay/form/footerButtons.js`:
- Around line 6-17: Remove the unused props hasErrors, hasSlotCountError, and
data from the destructuring of props in the footerButtons component. These props
are extracted but never referenced in the component body. Additionally, remove
the corresponding PropTypes declarations for these three props from the
component's PropTypes definition. After making these changes to FooterButtons,
also update the parent component accordion.js (around lines 200-210) to stop
computing and passing these unused props to FooterButtons to eliminate
unnecessary work.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 434728c6-5c14-458c-9580-4998be2741bd
⛔ Files ignored due to path filters (2)
zmsadmin/package-lock.jsonis excluded by!**/package-lock.jsonzmsstatistic/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
zmsadmin/js/page/availabilityDay/form/footerButtons.jszmsadmin/js/page/availabilityDay/form/formButtons.jszmsadmin/js/page/availabilityDay/index.jszmsadmin/js/page/availabilityDay/layouts/accordion.js
💤 Files with no reviewable changes (1)
- zmsadmin/js/page/availabilityDay/form/formButtons.js
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
zmsadmin/js/page/availabilityDay/index.js (1)
68-70:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winThe lifecycle method name is still misspelled—React won't call it.
The method was renamed from
componentDidUnMounttocomponentWillUnMount, but React's lifecycle method iscomponentWillUnmount(lowercase 'm' in 'unmount'). Since JavaScript method names are case-sensitive, React will never invoke this method, leaving thebeforeunloadlistener attached after the component unmounts. This causes memory leaks and stale "unsaved changes" prompts.🔧 Proposed fix
- componentWillUnMount() { + componentWillUnmount() { window.removeEventListener('beforeunload', this.unloadHandler) }🤖 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 `@zmsadmin/js/page/availabilityDay/index.js` around lines 68 - 70, The lifecycle method name is misspelled as componentWillUnMount, but React's correct lifecycle method is componentWillUnmount with a lowercase 'm' in 'unmount'. Since React uses case-sensitive method names and will never invoke the misspelled version, the beforeunload event listener will not be removed when the component unmounts. Rename the method from componentWillUnMount to componentWillUnmount so React properly calls it and the event listener cleanup occurs, preventing memory leaks.
🤖 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.
Duplicate comments:
In `@zmsadmin/js/page/availabilityDay/index.js`:
- Around line 68-70: The lifecycle method name is misspelled as
componentWillUnMount, but React's correct lifecycle method is
componentWillUnmount with a lowercase 'm' in 'unmount'. Since React uses
case-sensitive method names and will never invoke the misspelled version, the
beforeunload event listener will not be removed when the component unmounts.
Rename the method from componentWillUnMount to componentWillUnmount so React
properly calls it and the event listener cleanup occurs, preventing memory
leaks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1c4926bd-d093-4efb-873b-c340ef88e88b
⛔ Files ignored due to path filters (1)
docs/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
zmsadmin/js/page/availabilityDay/index.js
…ser-interface-improvements-for-new-business-hours
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.
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.docshinzugefügt.Summary by CodeRabbit