fix(pos): item selector section ui/ux #51521
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR enhances the Point of Sale system with UI improvements and backend logic updates. The changes include: CSS animations for item loading states and an "items not found" message display; refactoring the parent item group retrieval in the POS backend to accept a pos_profile parameter instead of ignoring permissions; converting item group initialization in the JavaScript item selector to an async operation that awaits profile configuration before loading items; updating filter state management and loading animations in the item selector; adjusting error message translation in the company defaults utility; removing a deprecated desktop icon configuration; and adding a validation guard in the transaction base utility to skip processing items without an item code. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
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: 1
Fix all issues with AI Agents 🤖
In @erpnext/selling/page/point_of_sale/pos_item_selector.js:
- Around line 13-14: Await the async parent-item-group fetch before starting
component init to avoid the race: sequence the calls so get_parent_item_group()
completes and sets this.item_group before inti_component() runs. Since
constructors can't be async, add or use an async initializer (e.g., an
initialize or existing async method) that does "await
this.get_parent_item_group(); await this.inti_component();" and call that
initializer from the constructor (or replace the direct calls) so
inti_component() / load_items_data() / get_items() always see a populated
this.item_group.
🧹 Nitpick comments (1)
erpnext/selling/page/point_of_sale/pos_item_selector.js (1)
17-23: Consider removing async keyword if not needed.The
inti_component()method is markedasyncbut doesn't contain anyawaitstatements. While this might be for future-proofing or to support the fix for the race condition mentioned above, if it's not intentionally async, the keyword can be removed to avoid confusion.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
erpnext/public/scss/point-of-sale.scsserpnext/selling/page/point_of_sale/point_of_sale.pyerpnext/selling/page/point_of_sale/pos_item_selector.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-16T05:33:58.723Z
Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.
Applied to files:
erpnext/selling/page/point_of_sale/point_of_sale.py
🧬 Code graph analysis (1)
erpnext/selling/page/point_of_sale/point_of_sale.py (1)
erpnext/accounts/doctype/pos_profile/pos_profile.py (1)
get_item_groups(228-247)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (6)
erpnext/public/scss/point-of-sale.scss (1)
120-125: LGTM! Clean CSS for empty state.The flexbox centering approach is appropriate for displaying the "Items not found" message. The styling properly supports the not-found UI state introduced in the JavaScript changes.
erpnext/selling/page/point_of_sale/point_of_sale.py (1)
125-131: LGTM! Permission-aware item group retrieval.The function now properly retrieves item groups from the POS Profile with permissions respected via
get_item_groups(), and gracefully falls back to root item groups when none are configured. The return of the first group (or None) aligns with the client-side expectations.erpnext/selling/page/point_of_sale/pos_item_selector.js (4)
76-103: Excellent empty state handling!The not-found state implementation is clean and user-friendly:
- Early return when no items (lines 79-82) avoids unnecessary rendering
- Proper class toggling between not-found and display modes (lines 84-87)
- Optional chaining (
items?.length,items?.forEach) prevents errors with undefined/null- Dedicated
set_items_not_found_banner()method is well-structured
202-230: Good UX improvements to item group filter.The changes improve the item group selector:
only_select: true(line 208) prevents users from adding new item groups or using advanced search, which is appropriate for POS filtering- Async
onchangehandler (line 209) properly handles the filtering flow- Using
me.pos_profile(line 219) directly is cleaner than deriving from the form doc- Unsetting awesomplete min-width (line 230) fixes the dropdown width issue mentioned in the PR
235-239: Nice addition for dynamic filter labels.The
set_item_selector_filter_label()method cleanly updates the UI label to reflect the selected item group, providing clear visual feedback to users. The fallback to "All Items" when no value is provided maintains consistency with the initial state.
43-51: This concern is already addressed by existing fallback logic.The server-side
get_items()function (line 148-149 of point_of_sale.py) explicitly handles the case whenitem_groupis undefined or doesn't exist. It contains a fallback that callsget_root_of("Item Group")to assign a valid default item group before using it. Therefore, undefineditem_groupdoes not cause downstream issues—it is gracefully handled by the server-side code.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #51521 +/- ##
===========================================
- Coverage 79.11% 79.11% -0.01%
===========================================
Files 1179 1179
Lines 121576 121579 +3
===========================================
+ Hits 96185 96186 +1
- Misses 25391 25393 +2
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @erpnext/selling/page/point_of_sale/pos_item_selector.js:
- Around line 43-53: The get_parent_item_group() method currently uses
frappe.call with a callback and does not return anything; change it to return
the promise from frappe.call (omit the callback and use .then or async/await) so
callers can await it; inside the then/await set this.item_group and
this.parent_item_group from the response (use r.message) and ensure the method
returns the frappe.call promise so the constructor/initializer can await
get_parent_item_group().
🧹 Nitpick comments (1)
erpnext/selling/page/point_of_sale/pos_item_selector.js (1)
211-216: Unnecessaryasynckeyword on the handler.The
onchangehandler is markedasyncbut doesn't useawait. Sincefilter_items()(line 214) is called without awaiting, the async keyword serves no purpose and could be removed for clarity.🔎 Simplify by removing async
- onchange: async function () { + onchange: function () { me.item_group = this.value; !me.item_group && (me.item_group = me.parent_item_group); me.filter_items(); me.set_item_selector_filter_label(this.value); },
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
erpnext/selling/page/point_of_sale/pos_item_selector.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (6)
erpnext/selling/page/point_of_sale/pos_item_selector.js (6)
10-10: Good refactor to make the display class dynamic.Deriving
item_display_classfromhide_imagesand applying it inprepare_dom()makes the code clearer and easier to maintain.Also applies to: 40-41
81-89: Excellent defensive coding and state management.The empty-state handling (lines 81-84) with optional chaining and early return is robust. The class restoration logic (lines 86-89) properly handles transitions from the "not found" state back to showing items. Using optional chaining on line 95 prevents potential runtime errors.
Also applies to: 95-95
101-105: LGTM - clear implementation of the empty state.The banner method correctly manages CSS classes and displays the "Items not found." message as described in the PR objectives.
210-210: UI/UX improvements align with PR objectives.Line 210's
only_select: trueprevents adding new Item Groups as intended. The filter label update (line 215) and awesomplete width fix (line 232) both match the PR description. Usingpos_profilein the query filter (line 221) is correct.Also applies to: 215-215, 221-221, 232-232
237-241: Clean implementation of the dynamic filter label.The method correctly updates the label based on the selected Item Group or defaults to "All Items", matching the PR objective of updating the label when the Item Group field changes.
13-14: The race condition flagged in the previous review is still present.The constructor calls
this.get_parent_item_group()(line 13) without awaiting, then immediately callsthis.inti_component()(line 14). Sinceget_parent_item_group()usesfrappe.call()with a callback (line 49) that setsthis.item_groupasynchronously (line 50), andinti_component()→load_items_data()(line 20) →get_items()(line 61) usesthis.item_group(line 69), there's a race whereget_items()may execute beforethis.item_groupis initialized, leading to incorrect or incomplete filtering.The previous review provided a detailed fix. Please address this critical issue.
Likely an incorrect or invalid review comment.
diptanilsaha
force-pushed
the
pos_item_selector_ui_ux
branch
from
be27335 to
114fb2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @erpnext/selling/page/point_of_sale/pos_item_selector.js:
- Around line 43-51: The get_parent_item_group method may leave this.item_group
and this.parent_item_group undefined when r.message is falsy; update it to
assign a safe fallback when no parent is returned (e.g., set both
this.item_group and this.parent_item_group to a defined default such as null or
an empty string or a sentinel like "All"), and optionally log a debug/info
message; make the change inside get_parent_item_group so any downstream code
that reads this.item_group or this.parent_item_group (used in filtering
elsewhere) always gets a defined value.
🧹 Nitpick comments (3)
erpnext/selling/page/point_of_sale/pos_item_selector.js (3)
17-17: Optional: Fix typo in method name.The method name
inti_componentappears to be a typo forinit_component. Consider renaming for clarity.📝 Suggested fix
- async inti_component() { + async init_component() {Also update the call site on line 14:
- this.inti_component(); + this.init_component();
210-216: Review the async onchange handler.The
onchangehandler is declared async (line 211), but it doesn't await any operations within its body. While this won't cause errors, it's unnecessary and could potentially cause issues if Frappe's form control doesn't properly handle async callbacks.Consider removing the
asynckeyword unless there's a specific reason for it.♻️ Suggested fix
- onchange: async function () { + onchange: function () { me.item_group = this.value; !me.item_group && (me.item_group = me.parent_item_group); me.filter_items(); me.set_item_selector_filter_label(this.value); },
232-234: Consider alternative to direct CSS manipulation.While the direct CSS manipulation on line 232 works, consider if this could be handled via a CSS class for better maintainability.
💡 Alternative approach
Add a CSS rule in the stylesheet:
.items-selector .item-group-field .awesomplete > ul { min-width: unset; }Then remove line 232 from the JavaScript.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
erpnext/public/scss/point-of-sale.scsserpnext/selling/page/point_of_sale/point_of_sale.pyerpnext/selling/page/point_of_sale/pos_item_selector.js
🚧 Files skipped from review as they are similar to previous changes (2)
- erpnext/public/scss/point-of-sale.scss
- erpnext/selling/page/point_of_sale/point_of_sale.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Patch Test
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Summary
🔇 Additional comments (6)
erpnext/selling/page/point_of_sale/pos_item_selector.js (6)
53-64: LGTM: Proper async coordination.The pattern of storing the promise in
item_ready_group(line 13) and awaiting it here ensures that the parent item group is fetched before loading items. This is a good approach for coordinating async operations.
78-99: LGTM: Proper empty state handling.The logic correctly handles the "items not found" state and properly restores the item display when items are available again. The optional chaining on line 95 adds defensive safety.
101-105: LGTM: Clean implementation.The not-found banner implementation is clean and properly uses translation.
238-246: LGTM: Clean helper methods.Both helper methods are well-implemented and support the PR objectives of improving the item group filter UX.
257-274: Verify the item group clear button behavior.The clear button uses
.val("").trigger("input")to reset the Link field (line 272). For Frappe Link fields, this might not properly trigger theonchangehandler or reset internal field state.Consider testing whether clearing the field this way:
- Properly triggers the
onchangehandler to reset filters- Clears the field's internal awesomplete state
- Updates the filter label correctly
If issues are found, consider using
this.item_group_field.set_value("")instead:this.$clear_item_group_btn.on("click", () => { - $(this.item_group_field.$input[0]).val("").trigger("input"); + this.item_group_field.set_value(""); this.item_group_field.set_focus(); });
10-10: LGTM: Good refactoring.Computing
item_display_classonce in the constructor and reusing it throughout is a good practice that improves maintainability.Also applies to: 40-40
diptanilsaha
force-pushed
the
pos_item_selector_ui_ux
branch
from
baf614c to
0934c9c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
erpnext/selling/page/point_of_sale/pos_item_selector.js (1)
411-459: Critical: CSS class name mismatch prevents loading animation.Lines 454 and 458 use the class
is-loading, but the SCSS file (line 120) defines the selector as.item-loading. This mismatch means the skeleton loading animation will never be applied to the items container.🐛 Proposed fix
start_item_loading_animation() { - this.$items_container.addClass("is-loading"); + this.$items_container.addClass("item-loading"); } stop_item_loading_animation() { - this.$items_container.removeClass("is-loading"); + this.$items_container.removeClass("item-loading"); }
🤖 Fix all issues with AI agents
In @erpnext/public/scss/point-of-sale.scss:
- Around line 120-147: The animation name used on .item-loading::after
("skeletonMove") does not match the defined keyframes ("shimmer"); update one to
match the other so the animation runs (either rename the animation property on
.item-loading::after to "shimmer" or rename the @keyframes declaration to
"skeletonMove"), ensuring references to animation: skeletonMove and @keyframes
shimmer are kept consistent.
- Around line 149-154: The .items-not-found rule sets height: 100vh which causes
overflow of the parent .items-container (which uses max-height: calc(100vh -
5rem)); change the .items-not-found selector to use height: 100% so it respects
the parent container bounds and fills the available space; keep the existing
display, align-items and justify-content rules intact to preserve centering
behavior.
🧹 Nitpick comments (1)
erpnext/selling/page/point_of_sale/pos_item_selector.js (1)
17-17: Optional: Consider fixing method name typo.The method
inti_componentappears to be a typo forinit_component. While the async conversion is correct, consider renaming for clarity.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
erpnext/public/scss/point-of-sale.scsserpnext/selling/page/point_of_sale/pos_item_selector.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Python Unit Tests (4)
- GitHub Check: Python Unit Tests (3)
- GitHub Check: Python Unit Tests (1)
- GitHub Check: Python Unit Tests (2)
- GitHub Check: Patch Test
- GitHub Check: Summary
🔇 Additional comments (6)
erpnext/selling/page/point_of_sale/pos_item_selector.js (6)
10-10: Good: Centralized display class logic.Storing
item_display_classin the constructor makes the display mode easily accessible throughout the component.Also applies to: 13-14
40-51: LGTM: Async parent item group fetching.The async
get_parent_item_groupmethod correctly fetches the parent group based on the POS profile, andprepare_domproperly applies the display class.
53-70: Good async flow with proper cleanup.The method correctly awaits the parent item group, starts the loading animation before the async operation, and uses
.always()to ensure the animation stops regardless of success or failure.
84-111: Excellent: Proper handling of empty item state.The not-found banner logic correctly handles the empty state, uses optional chaining for safety, and properly transitions between display states by swapping CSS classes.
216-248: LGTM: Item group field improvements align with PR objectives.The changes successfully implement the requirements:
only_select: trueprevents creating new item groups- Label updates dynamically with
set_item_selector_filter_label- Awesomplete dropdown width fix and open link button removal improve UX
Note: The
asynckeyword on line 217'sonchangeis unnecessary sincefilter_items()doesn't return a promise, but it's harmless.
263-280: Good: Clear button for item group filter.The clear button implementation correctly resets the item group field and refocuses the input.
diptanilsaha
force-pushed
the
pos_item_selector_ui_ux
branch
2 times, most recently
from
c238092 to
88f8456
Compare
diptanilsaha
force-pushed
the
pos_item_selector_ui_ux
branch
from
88f8456 to
277a8dc
Compare
4 participants
Changes include:
parent_item_groupis selected now based upon theitem_groupsselected on the POS Profile.Item Group select field:
Fixed the awesomplete dropdown width.
Before:
After:
Item Group Field is now only select, no provision to add new Item Group or Advanced Search.
Before:
After:
Onchange of Item Group Field,
labelon Item Selector section changes.Screen recording:
Screen.Recording.2026-01-06.at.12.48.58.AM.mov
If there are no items,
Items not foundmessage will be displayed.Animate on loading items on Item Selector.
Better UX to remove Item Group Field selection and removed the
Open Linkbutton from the Item Group Field.