Skip to content

Codex/feature polish closeout - #22

Merged
meilingrl merged 12 commits into
masterfrom
codex/feature-polish-closeout
Jun 3, 2026
Merged

meilingrl merged 12 commits into
masterfrom
codex/feature-polish-closeout

Conversation

@meilingrl

Copy link
Copy Markdown
Owner

What changed

Type

  • feat — new feature
  • fix — bug fix
  • refactor — no behavior change
  • docs / task — documentation or task lifecycle only
  • chore — tooling, config, deps

Testing

  • cd backend && mvnw.cmd test — all pass
  • cd frontend && npm test — all pass
  • cd frontend && npm run build — no errors
  • Manual smoke: golden path works, no obvious regressions

Checklist

  • CHANGELOG.md prepended (skip for docs-only)
  • API spec updated if endpoint contract changed (docs/09-api-spec/)
  • .http smoke file updated if endpoint added or changed (docs/06-http/)
  • Task doc archived if this closes an active task (docs/08-tasks/)
  • No unrelated files staged

Notes

meilingrl and others added 11 commits June 3, 2026 17:08
Move superseded planning docs out of current/, archive completed payment and preference tasks, and update AGENTS/README/standards so agents read stage-roadmap plus launch-preparation-roadmap.

Co-authored-by: Cursor <cursoragent@cursor.com>
Add shared id validation, route guards, Axios query sanitization, and backend 400 handling so NaN and other invalid ids no longer trigger Long binding errors.

Co-authored-by: Cursor <cursoragent@cursor.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements a comprehensive feature polish closeout across several modules. Key updates include stabilizing the messages chat layout with internal scrolling, adding fuzzy search and allowlisted sorting to the explore page, introducing cart bulk selection controls, persisting review images, and enabling admin role assignment, dashboard metrics, and CSV exports. It also introduces route guards and query sanitization to block invalid entity IDs. The review feedback highlights critical performance and security issues: N+1 query patterns on the admin dashboard and review image loading, a potential administrator lockout vulnerability during role demotion, inefficient sequential API calls for cart bulk selection, a bug in query parameter sanitization that strips array-valued IDs, and language inconsistencies in the review form UI.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

if (!isSuccessfulSalesOrder(order)) {
continue;
}
List<Map<String, Object>> items = transactionDataStore.findOrderItems(toLong(order.get("id")));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Calling transactionDataStore.findOrderItems(toLong(order.get("id"))) inside a loop over all orders introduces an N+1 query problem. As the number of orders in the system grows, this will cause severe performance degradation on the admin dashboard. Consider batch-loading the order items for all successful orders in a single query, or aggregating the hot products directly in the database using a single SQL query.

Comment on lines +440 to +444
if (FULL_ACCESS_ADMIN_ROLES.contains(currentRole)
&& !FULL_ACCESS_ADMIN_ROLES.contains(nextRole)
&& userMapper.countByRoles(new ArrayList<>(FULL_ACCESS_ADMIN_ROLES)) <= 1) {
throw new BusinessException(ResultCode.BAD_REQUEST, "At least one full-access admin must remain assigned");
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

The check userMapper.countByRoles(...) <= 1 counts all users with full-access admin roles, but it does not check if those users are active. If there are two full-access admins, one active and one disabled, demoting the active one will pass this check (since the count is 2), leaving only a disabled admin. This will permanently lock all administrators out of the system. Consider updating the query or the check to count only active full-access admins (e.g., where status = 'active').

result.put("productId", toLong(row.get("product_id")));
result.put("score", toInt(row.get("score")));
result.put("content", defaultString(row.get("content")));
result.put("images", findReviewImages(PRODUCT_REVIEW_TYPE, toLong(row.get("id"))));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Calling findReviewImages inside normalizeReview (and similarly in normalizeShopReview at line 373) introduces an N+1 query pattern when retrieving lists of reviews (e.g., on the product detail page). If a product has many reviews, this will trigger many separate database queries to load their images, slowing down the page load. Consider batch-loading the review images for all fetched reviews in a single query and mapping them in memory.

Comment on lines +105 to +111
bulkSelectionPending.value = true
try {
let latestCart = null
for (const { item, selected } of updates) {
const response = await updateCartItem(item.id, { selected })
latestCart = response.data || latestCart
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Updating the selection state of cart items by executing sequential updateCartItem API calls in a for loop is highly inefficient. If a user has many items in their cart, clicking "Select all" or "Invert selection" will trigger many sequential HTTP requests, leading to high network overhead and a slow UI response. Consider implementing a batch selection endpoint on the backend (e.g., PUT /api/cart/batch-selection) to handle this in a single request and database transaction.

Comment on lines +76 to +78
if (ID_QUERY_PARAM_PATTERN.test(key) && !isValidEntityId(value)) {
continue
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The sanitizeRequestParams function checks if a parameter key matches ID_QUERY_PARAM_PATTERN and validates it using isValidEntityId(value). If the parameter value is an array (e.g., userIds=[1, 2]), isValidEntityId will stringify it to "1,2", fail the numeric regex check, and return false. As a result, any array-valued query parameters matching the ID pattern will be silently stripped from the request. Consider explicitly handling array values in sanitizeRequestParams to avoid breaking batch query parameters.

    if (ID_QUERY_PARAM_PATTERN.test(key)) {
      if (Array.isArray(value)) {
        if (value.some((val) => !isValidEntityId(val))) continue
      } else if (!isValidEntityId(value)) {
        continue
      }
    }

<div class="review-form">
<div class="review-form__score">
<span class="review-form__label">评分</span>
<span class="review-form__label">Rating</span>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The newly introduced user-facing labels and messages in this file are written in English (e.g., "Rating", "Images", "Add image", "Submit review"), whereas the rest of the application is written in Chinese. This creates an inconsistent and jarring user experience. Please translate these labels and messages to Chinese to maintain consistency with the rest of the application.

      <span class="review-form__label">评分</span>

@meilingrl
meilingrl merged commit 02d1e43 into master Jun 3, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant