Recreate test#168
Open
SoiBien-AI wants to merge 19 commits into
Open
Conversation
…and restored complete vi.json)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR localizes backend flash messages and frontend admin/operator UI text via an i18n Changesi18n Localization
Backend Business Logic Fixes
E2E Test Suite Overhaul and Infra
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
Contributor
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/resources/js/Pages/admin/product-types/Create.jsx (1)
10-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLocalize the primary CTA too.
The page title and breadcrumbs are translated, but the main submit button is still hard-coded as
Create, so the form remains partially English for non-default locales.♻️ Proposed fix
- submitLabel="Create" + submitLabel={__("Create")}🤖 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 `@backend/resources/js/Pages/admin/product-types/Create.jsx` around lines 10 - 24, The main submit CTA on the Create product type page is still hard-coded in English. Update the `ResourceForm` usage in `Create.jsx` so `submitLabel` uses the translation helper like the title and breadcrumbs, keeping the primary action localized for all locales.backend/resources/js/Pages/admin/product-types/Edit.jsx (1)
10-33: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLocalize the browser title and submit CTA.
The translated breadcrumbs are good, but the document title and primary action are still hard-coded in English, so users will still see locale leaks in the tab and form controls.
♻️ Proposed fix
- <Head title={`Edit ${productType.name}`} /> + <Head title={__("Edit Product Type")} /> ... - submitLabel="Save Changes" + submitLabel={__("Save Changes")}🤖 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 `@backend/resources/js/Pages/admin/product-types/Edit.jsx` around lines 10 - 33, Update the Edit page so the browser title and primary submit label are localized instead of hard-coded English. In the Edit component, replace the literal title passed to Head and the submitLabel passed to ResourceForm with translated strings using the existing __() pattern, matching the translated breadcrumbs so the tab title and form CTA stay in sync with the current locale.backend/resources/js/Pages/admin/product-types/Show.jsx (1)
64-124: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFinish the i18n sweep on this page.
These sections still mix translated labels with hard-coded English copy, so non-English users will continue to see a mostly English product-details page.
Also applies to: 175-237, 277-385
🤖 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 `@backend/resources/js/Pages/admin/product-types/Show.jsx` around lines 64 - 124, The product type details page still mixes translated strings with hard-coded English copy, so complete the i18n pass across this component. Update the remaining labels, button text, section headings, and any English-only status/copy in Show.jsx to use the translation helper consistently, especially around the header/actions area and the other untranslated sections referenced in the review. Use the existing patterns already in this page (such as __("...")) so all user-facing text in Product Type Details is localized.
🧹 Nitpick comments (17)
backend/app/Http/Controllers/Web/Operator/BatchController.php (3)
60-78: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueVerbose per-request
infologging on a hot operator action.
startStepis called on every step start by every operator on the floor; unconditionalinfo-level logging on every invocation (entry, success) adds log volume that scales with production throughput with no apparent toggle. Consider gating this behinddebuglevel (which is typically filtered out in production) or removing it once the underlying issue this was added to diagnose is resolved.🤖 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 `@backend/app/Http/Controllers/Web/Operator/BatchController.php` around lines 60 - 78, The BatchController::startStep method is emitting unconditional info logs on every operator request, which creates unnecessary production log volume. Remove or downgrade the entry/success \Log::info calls in startStep, and keep only error logging in the catch blocks unless you gate the extra logging behind a debug-only condition or similar toggle.
73-76: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDomain/business exceptions logged at
errorlevel.
InsufficientStockExceptionand\DomainExceptionrepresent expected, user-recoverable conditions (e.g., an operator trying to start a step without enough stock), not application errors. Logging these aterrorlevel will pollute error dashboards/alerts with routine business events, drowning out real failures. Considerwarningfor this branch and reserveerrorfor the generic\Exceptioncatch below.🤖 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 `@backend/app/Http/Controllers/Web/Operator/BatchController.php` around lines 73 - 76, In BatchController::startStep, the InsufficientStockException and \DomainException branch is logging expected business/recoverable conditions at error level. Change that \Log::error call in the catch (InsufficientStockException|\DomainException $e) block to warning so routine validation failures don’t pollute error monitoring, and keep the generic catch (\Exception $e) as error for real application failures.
60-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse structured log context instead of string concatenation.
The new log calls build messages via string concatenation (
'startStep called for step ' . $batchStep->id . ...). Laravel's logger accepts a context array as the second argument, which is far more useful for searching/filtering in log aggregators and avoids manual string building:♻️ Proposed refactor using structured context
- \Log::info('startStep called for step ' . $batchStep->id . ' by user ' . $request->user()->id . ' (selected_line_id: ' . $request->session()->get('selected_line_id') . ')'); - - if (! $this->stepBelongsToSelectedLine($request, $batchStep)) { - \Log::error('stepBelongsToSelectedLine failed'); + \Log::info('startStep called', [ + 'step_id' => $batchStep->id, + 'user_id' => $request->user()->id, + 'selected_line_id' => $request->session()->get('selected_line_id'), + ]); + + if (! $this->stepBelongsToSelectedLine($request, $batchStep)) { + \Log::error('stepBelongsToSelectedLine failed', ['step_id' => $batchStep->id, 'user_id' => $request->user()->id]); return back()->with('error', 'This step does not belong to the selected line.'); } try { $picksByMaterial = $this->reshapePicks($request->validated()['picks'] ?? []); $this->batchService->startStep($batchStep, $request->user(), $picksByMaterial); - \Log::info('startStep succeeded'); + \Log::info('startStep succeeded', ['step_id' => $batchStep->id]); return back()->with('success', 'Step started. Materials have been allocated.'); } catch (InsufficientStockException|\DomainException $e) { - \Log::error('startStep domain error: ' . $e->getMessage()); + \Log::warning('startStep domain error', ['step_id' => $batchStep->id, 'message' => $e->getMessage()]); return back()->withErrors(['picks' => $e->getMessage()])->with('error', $e->getMessage()); } catch (\Exception $e) { - \Log::error('startStep exception: ' . $e->getMessage()); + \Log::error('startStep exception', ['step_id' => $batchStep->id, 'message' => $e->getMessage()]); return back()->with('error', $e->getMessage()); }The error-level log at Line 63 also lacks any identifiers (step id, user id), making it harder to correlate with the failure in production.
🤖 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 `@backend/app/Http/Controllers/Web/Operator/BatchController.php` around lines 60 - 78, The BatchController::startStep logging should use structured context instead of concatenated strings. Update the info/error logs around step start to pass identifiers like batchStep->id, request->user()->id, and selected_line_id as the second argument to Log::info/Log::error, and include the same context in the catch blocks for InsufficientStockException|\DomainException and \Exception. Keep the messages descriptive but move the dynamic values into the context array so the logs are easier to search and correlate.backend/app/Http/Controllers/Web/Packaging/PackagingController.php (1)
73-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate packable-state condition across 4 call sites.
The composite filter
status in [DONE, IN_PROGRESS] OR produced_qty > 0is now repeated inscan()(Line 73),buildItemList()(Lines 279-282), and twice inbuildStats()(Lines 318-321, 325-328). Extracting a local scope onWorkOrderwould prevent the logic from drifting out of sync across call sites.♻️ Proposed refactor
// WorkOrder.php public function scopePackable($query) { return $query->where(function ($q) { $q->whereIn('status', [self::STATUS_DONE, self::STATUS_IN_PROGRESS]) ->orWhere('produced_qty', '>', 0); }); }- if (! in_array($workOrder->status, [WorkOrder::STATUS_DONE, WorkOrder::STATUS_IN_PROGRESS]) && $workOrder->produced_qty <= 0) { + if (! WorkOrder::whereKey($workOrder->id)->packable()->exists()) {- return WorkOrder::where(function ($q) { - $q->whereIn('status', [WorkOrder::STATUS_DONE, WorkOrder::STATUS_IN_PROGRESS]) - ->orWhere('produced_qty', '>', 0); - }) + return WorkOrder::packable()Also applies to: 279-282, 318-328
🤖 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 `@backend/app/Http/Controllers/Web/Packaging/PackagingController.php` at line 73, The packable-state filter is duplicated in PackagingController across scan(), buildItemList(), and buildStats(), so extract it into a reusable WorkOrder local scope like scopePackable() and use that scope at each call site instead of repeating the status/produced_qty logic. Update the controller methods to call the new WorkOrder scope so the composite condition stays consistent in one place.tests/e2e/car-production-buildout-vi.spec.ts (5)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead
child_processimport and commented-out reset code.
execSync/child_process(Lines 1-2) is only referenced inside commented-out code (Lines 134-137); the actual DB reset is now handled byreset-test_vi.cmdper the comment on Line 127. Remove the unused import and dead code block for clarity.♻️ Suggested change
import { test, expect, Page } from '`@playwright/test`'; -import { execSync } from 'child_process';- // RESET - //console.log('[Setup] Refreshing database...'); - //execSync(`docker exec openmes-backend php artisan migrate:fresh --seed --force`, { stdio: 'ignore' }); - //execSync(`docker exec openmes-backend php artisan cache:clear`, { stdio: 'ignore' }); - //execSync(`docker exec openmes-backend php artisan tinker --execute="\\App\\Models\\User::create(['name' => 'Administrator', 'username' => config('openmmes.admin_username') ?: 'admin', 'email' => config('openmmes.admin_email') ?: 'admin@example.com', 'password' => \\Illuminate\\Support\\Facades\\Hash::make(config('openmmes.admin_password') ?: 'Admin1234!'), 'force_password_change' => false, 'email_verified_at' => now()])->assignRole('Admin');"`, { stdio: 'ignore' }); -Also applies to: 134-137
🤖 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 `@tests/e2e/car-production-buildout-vi.spec.ts` around lines 1 - 2, Remove the dead child_process usage from the Playwright spec by deleting the unused execSync import and the commented-out DB reset block in car-production-buildout-vi.spec.ts, since the reset is now handled elsewhere. Update the test file around the top-level imports and the cleanup/reset section so only the active reset-test_vi.cmd flow remains, keeping the test logic in the existing spec methods intact.
53-72: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueLanguage-switch flow relies on ordinal selector and fixed sleep.
Line 59 picks
button[aria-haspopup="listbox"]).first()assuming it's the language dropdown, and Line 67 uses a fixed 2swaitForTimeoutto wait for the page reload after switching language. This is consistent with the rest of the file's style and likely works for the current page layout, but is fragile to future layout changes and could be flaky under slow reloads.🤖 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 `@tests/e2e/car-production-buildout-vi.spec.ts` around lines 53 - 72, The language-switch flow in login() is fragile because it relies on an ordinal locator for the dropdown and a fixed sleep after selecting Tiếng Việt. Update the selector to target the language control more specifically instead of page.locator(...).first(), and replace the hardcoded waitForTimeout with a deterministic wait for the locale change or reload using Playwright’s wait-for state/URL behavior. Keep the changes within login() so the language switch remains stable across layout changes and slower reloads.
222-223: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated "pick tomorrow's day" date logic across three steps.
The
new Date(Date.now() + 7*3600*1000).getUTCDate()+ calendar-button-click pattern is repeated at material-lot creation (Lines 222-223), work-order creation (Lines 253-255), and shortage-fill material-lot creation (Lines 299-300). Consider extracting a smallpickTomorrow(page)helper to reduce duplication and centralize the month-boundary edge case (the day-number match can select the wrong month if the picker doesn't scope by month).Also applies to: 253-255, 299-300
🤖 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 `@tests/e2e/car-production-buildout-vi.spec.ts` around lines 222 - 223, The “pick tomorrow’s day” calendar selection logic is duplicated in multiple places in the e2e spec, and the current day-number-only matcher can hit the wrong month. Extract a small helper such as pickTomorrow(page) in car-production-buildout-vi.spec.ts and use it from the material-lot, work-order, and shortage-fill flows to centralize the Date/locator logic and make the month-scoping fix in one place.
353-354: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVietnamese comments violate the English-only coding guideline for
.tsfiles.Lines 353-354 and 405 are code comments written in Vietnamese; while the test's UI-facing strings must be Vietnamese to exercise localization, code comments should remain in English per the coding guideline for this file type.
As per coding guidelines,
**/*.{php,js,jsx,ts,tsx,blade.php}: "All code, Blade/JSX text, validation messages, seeders, and comments must be in English."♻️ Suggested change
- // Đặt số lượng lô (batch) là 1 để Lệnh Sản Xuất không bị hoàn thành 100% - // Nếu hoàn thành 100%, Work Order sẽ chuyển sang trạng thái DONE và không cho phép báo cáo sự cố (Report Issue) nữa. + // Set the batch quantity to 1 so the work order does not reach 100% completion. + // If it reaches 100%, the work order transitions to DONE and no longer allows reporting an issue. const batchDialog = page.locator('.fixed.inset-0.z-50');- // Bấm luôn nút Complete để hoàn thành step, chờ API xử lý xong trước khi chuyển trang + // Click Complete to finish the step, waiting for the API response before navigating away console.log('Clicking Complete button and waiting for response...');Also applies to: 405-405
🤖 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 `@tests/e2e/car-production-buildout-vi.spec.ts` around lines 353 - 354, The comments in car-production-buildout-vi.spec.ts are written in Vietnamese, which violates the English-only guideline for .ts files. Update the affected comments near the test setup and the later matching comment around the same scenario to clear English while keeping the UI-facing Vietnamese strings unchanged; use the surrounding test block and the Report Issue / Work Order logic to find the exact comments.Source: Coding guidelines
338-344: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGeneric
page.locator('select')risks strict-mode ambiguity.If the operator/workstation page ever renders more than one
<select>element, this locator will throw a Playwright strict-mode violation instead of resolving to the intended machine-state dropdown. Worth scoping with a more specific selector (e.g., by label or test id) if the page markup allows it.🤖 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 `@tests/e2e/car-production-buildout-vi.spec.ts` around lines 338 - 344, The machine-state dropdown locator in the E2E test is too generic and can trigger Playwright strict-mode failures if multiple select elements are present. Update the locator in the test around the selectOption flow to target the intended dropdown more specifically, preferably using a label-based or test-id-based selector tied to the operator/workstation page markup. Keep the rest of the interaction logic in place, but make the selection unambiguous by scoping it to the unique machine-state control.playwright.config.ts (1)
11-15: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGlobal
video: 'on'applies to every spec, not just the demo-oriented buildout tests.This config is shared by all Playwright specs, including
tests/e2e/material-hold-release.spec.ts. Forcing video recording'on'for every test run (rather thanretain-on-failure, consistent withtrace/screenshot) will grow CI artifact storage and I/O for tests that don't need a recorded demo. Consider scopingvideo: 'on'to the buildout spec files (e.g., viatest.use()overrides, which the VI/EN specs already do) and keeping the global default lighter.♻️ Suggested change
use: { baseURL: process.env.E2E_BASE_URL || 'http://localhost:8080', trace: 'retain-on-failure', screenshot: 'only-on-failure', ignoreHTTPSErrors: true, - video: 'on', + video: 'retain-on-failure', },🤖 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 `@playwright.config.ts` around lines 11 - 15, The global Playwright config is forcing video recording for every spec, including non-demo tests like material-hold-release. Move the `video: 'on'` setting out of the shared `playwright.config.ts` default and scope it only to the buildout/demo specs using `test.use()` overrides, matching the approach already used in the VI/EN specs. Keep the shared config lighter by leaving video at a failure-based setting for all other suites.tests/e2e/car-production-buildout-en.spec.ts (1)
2-2: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRemove the stale
child_processsetup path.
execSyncis now only supporting commented-out reset code, but it still triggers command-execution static analysis. Remove the import and dead reset block.Also applies to: 119-123
🤖 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 `@tests/e2e/car-production-buildout-en.spec.ts` at line 2, The test still carries a stale child_process execution path via execSync, but it is only used by commented-out reset logic and triggers static analysis unnecessarily. Remove the execSync import and delete the dead reset block in the e2e test so the file no longer references child_process at all; update the test around the reset/setup section to keep only the active buildout flow.Source: Linters/SAST tools
backend/resources/js/Pages/admin/lines/Show.jsx (2)
246-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded "(s)" plural marker in translation key.
':count workstation(s) on this line.'bakes an English pluralization hack straight into the translatable string. The__()helper (backend/resources/js/lib/i18n.js) only does token replacement, not pluralization, so this text will read awkwardly in English (1 workstation(s)) and won't translate naturally into Vietnamese, which doesn't use "(s)"-style plural markers.Consider splitting into singular/plural keys, e.g.:
♻️ Suggested fix
- <p className="text-sm text-om-muted mt-0.5"> - {__(':count workstation(s) on this line.', { count: line.workstations_count })} - </p> + <p className="text-sm text-om-muted mt-0.5"> + {line.workstations_count === 1 + ? __('1 workstation on this line.') + : __(':count workstations on this line.', { count: line.workstations_count })} + </p>🤖 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 `@backend/resources/js/Pages/admin/lines/Show.jsx` around lines 246 - 253, The text in Show.jsx uses a hardcoded “(s)” inside the __() translation key, so replace that workstation count message with proper singular/plural handling instead of a single string. Update the workstation count rendering near the effectiveWorkstations block to choose between distinct singular and plural translation keys (or equivalent plural-aware logic) while keeping the same count from line.workstations_count.
349-370: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent input control: native
<select>vs the sharedDropdowncomponent.Every other selector in this file (
ViewTemplateCard,MaterialFormin Bom.jsx, etc.) uses@openmes/ui'sDropdown, but this one was swapped to a plain HTML<select>. Purely a styling/consistency concern, not a functional bug — thedisabled={form.processing || !form.data.user_id}guard added here is a good improvement.🤖 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 `@backend/resources/js/Pages/admin/lines/Show.jsx` around lines 349 - 370, The operator picker in Show.jsx is using a native select instead of the shared `@openmes/ui` Dropdown, which breaks consistency with the rest of the page. Update the assignment form in the component that renders availableOperators to use the same Dropdown pattern as the other selectors in this file (and related forms like ViewTemplateCard/MaterialForm), while preserving the current form.data.user_id binding, onChange behavior, and disabled guard on the submit button.backend/resources/js/Pages/admin/materials/Edit.jsx (1)
11-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPage
<Head>title left untranslated while the heading was localized.Line 12 wraps the heading in
__("Edit Material"), but line 11'sHead titlekeeps the rawEdit ${material.name}prefix in English, so the browser tab title won't localize even though the on-page heading does.♻️ Suggested fix
- <Head title={`Edit ${material.name}`} /> + <Head title={`${__('Edit')} ${material.name}`} />🤖 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 `@backend/resources/js/Pages/admin/materials/Edit.jsx` around lines 11 - 12, The Edit page title in the Head component is still hardcoded in English while the visible heading uses localization. Update the Head title in Edit.jsx to use the same translation approach as the h1, keeping the Material name variable but localizing the “Edit” prefix so the browser tab title matches the translated UI.backend/resources/js/Pages/admin/process-templates/Bom.jsx (1)
64-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissed translation: "Material" label in the edit branch.
The non-edit branch's equivalent label is now
{__("Material")}(line 73), but this edit-mode label at line 65 is still a raw hardcoded string.♻️ Suggested fix
- <label className="block text-sm font-medium text-om-muted mb-1">Material</label> + <label className="block text-sm font-medium text-om-muted mb-1">{__("Material")}</label>🤖 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 `@backend/resources/js/Pages/admin/process-templates/Bom.jsx` around lines 64 - 69, The edit-mode “Material” label in the Bom page is still hardcoded while the non-edit branch already uses translation. Update the label in the Bom component’s edit branch to use the same __() translation helper as the other branch, keeping the wording consistent and localizable. Locate the label in the Bom JSX markup around the material display section and replace the raw string with the translated form.backend/resources/js/Pages/admin/users/UserForm.jsx (1)
108-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLocalization coverage stops midway through the form.
The account-type/identity/password/role section is fully localized via
__(), but the worker-profile section (labels like "Worker profile", "Worker Code", "Phone", "Crew", "Wage Group", helper text) and the submit/cancel controls ("Saving…", "Save Changes", "Create Account", "Cancel") remain hardcoded English. For consistency with this i18n rollout, wrap the remaining strings too.🤖 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 `@backend/resources/js/Pages/admin/users/UserForm.jsx` around lines 108 - 179, The remaining English strings in UserForm are not localized, while the earlier account fields already use __(). Update the worker-profile section and action controls in UserForm.jsx to wrap all visible labels, helper text, empty states, and button/link text with __(), including the Worker profile block, Worker Code/Phone/Crew/Wage Group, the skills section copy, and the Saving…/Save Changes/Create Account/Cancel actions. Use the existing localization pattern already present in the form so the new strings match the rest of the i18n rollout.backend/resources/js/Pages/admin/work-orders/Index.jsx (1)
65-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNative dialog text left untranslated inside the localized
actionshandler.The
prompt('Produced quantity to complete with:', ...)(line 66) and the twoconfirm(...)messages (lines 77, 84) are the only remaining hardcoded strings in an otherwise fully__()-wrappedactions()function. These are user-facing text just like the surrounding labels.♻️ Proposed fix
- const qty = prompt('Produced quantity to complete with:', r.planned_qty); + const qty = prompt(__('Produced quantity to complete with:'), r.planned_qty);- a.push({ label: __('Cancel'), variant: 'warning', onClick: () => { if (confirm(`Cancel work order ${r.order_no}?`)) post(r.id, 'cancel'); } }); + a.push({ label: __('Cancel'), variant: 'warning', onClick: () => { if (confirm(__('Cancel work order :order?', { order: r.order_no }))) post(r.id, 'cancel'); } });- onClick: () => { if (confirm(`Delete work order ${r.order_no}? (only allowed if it has no batches)`)) router.delete(`/admin/work-orders/${r.id}`, { preserveScroll: true }); }, + onClick: () => { if (confirm(__('Delete work order :order? (only allowed if it has no batches)', { order: r.order_no }))) router.delete(`/admin/work-orders/${r.id}`, { preserveScroll: true }); },Also applies to: 77-77, 84-84
🤖 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 `@backend/resources/js/Pages/admin/work-orders/Index.jsx` around lines 65 - 68, The `actions()` handler in `Index.jsx` still contains hardcoded user-facing dialog text in the `prompt` and `confirm` calls, unlike the rest of the localized labels. Replace the strings in the `onClick` handlers for the complete/cancel actions with `__()`-wrapped translations, using the existing `actions()` localization pattern and the relevant symbols like `post`, `prompt`, and `confirm` to keep all native dialog text consistent and translatable.
🤖 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 `@backend/app/Http/Controllers/Web/Packaging/PackagingController.php`:
- Line 73: The packaging eligibility check in PackagingController is now too
broad because it allows any work order with produced_qty > 0, which lets
non-eligible statuses like CANCELLED or REJECTED pass through scan, list, and
stats paths. Tighten the condition in the affected PackagingController methods
to only allow WorkOrder::STATUS_DONE and WorkOrder::STATUS_IN_PROGRESS, matching
the API controller’s allowlist, or extract that shared predicate into a reusable
method/scope and use it consistently.
In `@backend/app/Http/Requests/Web/Admin/UpdateMaterialRequest.php`:
- Line 46: The UpdateMaterialRequest validation is too strict for admin edits
because material_type_id is currently required in the update rules. Change the
rule in UpdateMaterialRequest to keep material_type_id optional/nullable on
update, while still preserving the exists check when a value is provided, so the
Admin update flow can save materials without a type.
In `@backend/resources/js/app.jsx`:
- Line 3: The app bootstrap in app.jsx is no longer applying the Inertia
timezone prop because loadLocale() does not call setTimezone(), which can make
formatDate*() fall back to the browser timezone. Update the app initialization
to keep passing the Inertia timezone value through the i18n setup, using the
existing loadLocale and timezone handling so the plant timezone remains the
canonical source; if you intend browser-local time instead, adjust the backend
contract and docs accordingly.
In `@backend/resources/js/components/ResourceForm.jsx`:
- Line 222: The date field in ResourceForm’s placeholder handling is skipping
translation for caller-supplied values, unlike the other field types. Update the
DatePicker placeholder logic in ResourceForm.jsx so it passes any explicit
placeholder through __() before falling back to __('Select date'), matching the
translation pattern used by the other branches.
In `@backend/resources/js/Pages/admin/process-templates/Edit.jsx`:
- Line 2: The Edit page localization is incomplete and should match the fully
translated Create page. Update the remaining hardcoded strings in
Edit.jsx—especially the navigation link, field label, placeholder, helper text,
checkbox label, Cancel button, and submit/loading text—by using the existing __
i18n helper and the same translation keys/patterns used in Create.jsx. Use the
Edit component’s Head, h1, and form/button elements as the main locations to
align all visible text with the localized version.
In `@backend/resources/js/Pages/admin/process-templates/Show.jsx`:
- Line 437: The media-type badge in Show.jsx is using a lowercase enum with
__(), but the translation entries in the same component are registered with
capitalized keys, so the lookup never matches. Update the badge rendering and
the related translation registration in the media-type dropdown to use the same
key format consistently, and reference the existing m.media_type usage and the
__('Image')/__('PDF')/__('Video') entries so the lookup resolves to the intended
translated label.
In `@docker-compose.yml`:
- Line 93: Remove the `./backend:/var/www/html` bind mount from the base compose
configuration so the service uses the image built by `backend/Dockerfile` with
its baked dependencies and assets. Keep the live-edit mount behavior only in
`docker-compose.dev.yml`, and ensure the main service definition still starts
cleanly without shadowing `vendor/` or `public/build`.
In `@reset-test_en.cmd`:
- Around line 1-7: The reset script needs to be Windows-safe and stop on setup
failures: in reset-test_en.cmd, make sure the file uses CRLF line endings, quote
the E2E_BASE_URL assignment, and add fail-fast guards after both php artisan
tinker invocations so the Playwright run does not start if either setup step
fails. Keep the existing docker exec, migrate:fresh, cache:clear, and npx
playwright test flow, but ensure the script exits immediately on any nonzero
error from the setup commands.
In `@reset-test_vi.cmd`:
- Around line 1-7: The reset script is not Windows-safe and does not stop on
setup failures, so fix the batch file by saving it with CRLF line endings,
quoting the environment assignment in the `set E2E_BASE_URL` step, and adding
`|| exit /b %errorlevel%` after both `docker exec ... artisan tinker` commands
in `reset-test_vi.cmd` so the Playwright run only starts when setup succeeds.
In `@reset-test.cmd`:
- Around line 1-7: The reset script needs to be Windows-safe and stop on setup
failures. Update the .cmd flow around the docker exec setup commands and the
E2E_BASE_URL assignment so the environment variable is quoted/validated
correctly, and ensure each prerequisite command fails the script immediately
instead of letting the Playwright step run after a bad reset. Also save
reset-test.cmd with CRLF line endings so it executes reliably on Windows.
In `@tests/e2e/car-production-buildout-en.spec.ts`:
- Around line 101-109: The English E2E spec still contains Vietnamese labels and
comments in helper selectors, which should be kept out of the -en test. Update
the shared helpers in car-production-buildout-en.spec to use English-only text
or stable test IDs, and move any Vietnamese fallback coverage into the
Vietnamese spec or shared localized helpers. Also remove any non-English
comments/text from the affected sections so the file stays fully English.
- Around line 390-392: The completion wait in the e2e test is too permissive
because `page.waitForResponse` currently accepts any status at or above 200,
including failures. Update the `Promise.all` around `completeBtn.click({ force:
true })` to only resolve for a successful completion response (for example, a
true 2xx status) so the test cannot proceed after a 4xx/5xx request.
- Around line 7-12: The top-level `.env` read in the test setup can throw before
the fallback password is applied, so guard the `readFileSync`/`match` logic in
`car-production-buildout-en.spec.ts` and make `ADMIN_PASSWORD` optional when the
file is missing. Update the module-level initialization around `envPath`,
`envContent`, and `adminPassMatch` so it safely falls back to the default `PASS`
value if `.env` cannot be read, without crashing at import time.
- Around line 365-384: The lot-pick handling in the e2e test is swallowing
failures by catching errors around the modal flow and only logging them, which
lets the test continue incorrectly. Update the modal handling in the
car-production-buildout spec so that if the Confirm picks flow in the lot pick
modal appears and selection/confirmation fails, the error is rethrown or
otherwise causes the test to fail immediately instead of being logged and
ignored. Keep the existing modal detection logic around page.getByRole('button',
{ name: /(Confirm picks|Xác nhận)/i }) and the selectOption loop, but remove the
silent catch behavior.
In `@tests/e2e/car-production-buildout.spec.ts`:
- Around line 15-18: The default e2e spec contains Vietnamese constants,
selectors, and comments in English-only TypeScript code, which violates the
repository guideline. Update the flow in car-production-buildout.spec.ts to use
English text throughout, or remove the duplicated localized scenario if the
separate Vietnamese spec already covers it. Focus on the localized strings and
comments near the top-level constants and the spec blocks so the default test
remains English-only.
- Around line 370-389: The lot-pick handling in the e2e flow is swallowing
failures in the try/catch around the modal confirmation, which lets the test
continue even when lot picking fails. Update the logic around the confirm button
flow in car-production-buildout.spec.ts so that only the modal-not-present case
is ignored, but any error while selecting lots, waiting for enablement, or
clicking the confirm button is rethrown and fails the test. Keep the existing
modal detection via page.getByRole and confirmBtn, but remove the silent logging
path for actual picker/confirmation errors.
- Around line 395-397: The completion flow in the e2e test is too permissive
because the response check on the page.waitForResponse predicate accepts any
status at or above 200, including failed 4xx/5xx results. Update the predicate
used with completeBtn.click and page.waitForResponse in the
car-production-buildout spec to require a successful completion response only
(for example, a 2xx range), so the test waits for a real success before
proceeding.
- Around line 7-12: The test setup in car-production-buildout.spec.ts reads the
root .env at module import time, which throws before the fallback password can
be used when the file is missing. Move the .env access behind a safe existence
check or lazy load inside the test setup, and keep the ADMIN_PASSWORD parsing
logic in the same place so the fallback in the ADMIN/PASS constants still works
when .env is absent.
---
Outside diff comments:
In `@backend/resources/js/Pages/admin/product-types/Create.jsx`:
- Around line 10-24: The main submit CTA on the Create product type page is
still hard-coded in English. Update the `ResourceForm` usage in `Create.jsx` so
`submitLabel` uses the translation helper like the title and breadcrumbs,
keeping the primary action localized for all locales.
In `@backend/resources/js/Pages/admin/product-types/Edit.jsx`:
- Around line 10-33: Update the Edit page so the browser title and primary
submit label are localized instead of hard-coded English. In the Edit component,
replace the literal title passed to Head and the submitLabel passed to
ResourceForm with translated strings using the existing __() pattern, matching
the translated breadcrumbs so the tab title and form CTA stay in sync with the
current locale.
In `@backend/resources/js/Pages/admin/product-types/Show.jsx`:
- Around line 64-124: The product type details page still mixes translated
strings with hard-coded English copy, so complete the i18n pass across this
component. Update the remaining labels, button text, section headings, and any
English-only status/copy in Show.jsx to use the translation helper consistently,
especially around the header/actions area and the other untranslated sections
referenced in the review. Use the existing patterns already in this page (such
as __("...")) so all user-facing text in Product Type Details is localized.
---
Nitpick comments:
In `@backend/app/Http/Controllers/Web/Operator/BatchController.php`:
- Around line 60-78: The BatchController::startStep method is emitting
unconditional info logs on every operator request, which creates unnecessary
production log volume. Remove or downgrade the entry/success \Log::info calls in
startStep, and keep only error logging in the catch blocks unless you gate the
extra logging behind a debug-only condition or similar toggle.
- Around line 73-76: In BatchController::startStep, the
InsufficientStockException and \DomainException branch is logging expected
business/recoverable conditions at error level. Change that \Log::error call in
the catch (InsufficientStockException|\DomainException $e) block to warning so
routine validation failures don’t pollute error monitoring, and keep the generic
catch (\Exception $e) as error for real application failures.
- Around line 60-78: The BatchController::startStep logging should use
structured context instead of concatenated strings. Update the info/error logs
around step start to pass identifiers like batchStep->id, request->user()->id,
and selected_line_id as the second argument to Log::info/Log::error, and include
the same context in the catch blocks for
InsufficientStockException|\DomainException and \Exception. Keep the messages
descriptive but move the dynamic values into the context array so the logs are
easier to search and correlate.
In `@backend/app/Http/Controllers/Web/Packaging/PackagingController.php`:
- Line 73: The packable-state filter is duplicated in PackagingController across
scan(), buildItemList(), and buildStats(), so extract it into a reusable
WorkOrder local scope like scopePackable() and use that scope at each call site
instead of repeating the status/produced_qty logic. Update the controller
methods to call the new WorkOrder scope so the composite condition stays
consistent in one place.
In `@backend/resources/js/Pages/admin/lines/Show.jsx`:
- Around line 246-253: The text in Show.jsx uses a hardcoded “(s)” inside the
__() translation key, so replace that workstation count message with proper
singular/plural handling instead of a single string. Update the workstation
count rendering near the effectiveWorkstations block to choose between distinct
singular and plural translation keys (or equivalent plural-aware logic) while
keeping the same count from line.workstations_count.
- Around line 349-370: The operator picker in Show.jsx is using a native select
instead of the shared `@openmes/ui` Dropdown, which breaks consistency with the
rest of the page. Update the assignment form in the component that renders
availableOperators to use the same Dropdown pattern as the other selectors in
this file (and related forms like ViewTemplateCard/MaterialForm), while
preserving the current form.data.user_id binding, onChange behavior, and
disabled guard on the submit button.
In `@backend/resources/js/Pages/admin/materials/Edit.jsx`:
- Around line 11-12: The Edit page title in the Head component is still
hardcoded in English while the visible heading uses localization. Update the
Head title in Edit.jsx to use the same translation approach as the h1, keeping
the Material name variable but localizing the “Edit” prefix so the browser tab
title matches the translated UI.
In `@backend/resources/js/Pages/admin/process-templates/Bom.jsx`:
- Around line 64-69: The edit-mode “Material” label in the Bom page is still
hardcoded while the non-edit branch already uses translation. Update the label
in the Bom component’s edit branch to use the same __() translation helper as
the other branch, keeping the wording consistent and localizable. Locate the
label in the Bom JSX markup around the material display section and replace the
raw string with the translated form.
In `@backend/resources/js/Pages/admin/users/UserForm.jsx`:
- Around line 108-179: The remaining English strings in UserForm are not
localized, while the earlier account fields already use __(). Update the
worker-profile section and action controls in UserForm.jsx to wrap all visible
labels, helper text, empty states, and button/link text with __(), including the
Worker profile block, Worker Code/Phone/Crew/Wage Group, the skills section
copy, and the Saving…/Save Changes/Create Account/Cancel actions. Use the
existing localization pattern already present in the form so the new strings
match the rest of the i18n rollout.
In `@backend/resources/js/Pages/admin/work-orders/Index.jsx`:
- Around line 65-68: The `actions()` handler in `Index.jsx` still contains
hardcoded user-facing dialog text in the `prompt` and `confirm` calls, unlike
the rest of the localized labels. Replace the strings in the `onClick` handlers
for the complete/cancel actions with `__()`-wrapped translations, using the
existing `actions()` localization pattern and the relevant symbols like `post`,
`prompt`, and `confirm` to keep all native dialog text consistent and
translatable.
In `@playwright.config.ts`:
- Around line 11-15: The global Playwright config is forcing video recording for
every spec, including non-demo tests like material-hold-release. Move the
`video: 'on'` setting out of the shared `playwright.config.ts` default and scope
it only to the buildout/demo specs using `test.use()` overrides, matching the
approach already used in the VI/EN specs. Keep the shared config lighter by
leaving video at a failure-based setting for all other suites.
In `@tests/e2e/car-production-buildout-en.spec.ts`:
- Line 2: The test still carries a stale child_process execution path via
execSync, but it is only used by commented-out reset logic and triggers static
analysis unnecessarily. Remove the execSync import and delete the dead reset
block in the e2e test so the file no longer references child_process at all;
update the test around the reset/setup section to keep only the active buildout
flow.
In `@tests/e2e/car-production-buildout-vi.spec.ts`:
- Around line 1-2: Remove the dead child_process usage from the Playwright spec
by deleting the unused execSync import and the commented-out DB reset block in
car-production-buildout-vi.spec.ts, since the reset is now handled elsewhere.
Update the test file around the top-level imports and the cleanup/reset section
so only the active reset-test_vi.cmd flow remains, keeping the test logic in the
existing spec methods intact.
- Around line 53-72: The language-switch flow in login() is fragile because it
relies on an ordinal locator for the dropdown and a fixed sleep after selecting
Tiếng Việt. Update the selector to target the language control more specifically
instead of page.locator(...).first(), and replace the hardcoded waitForTimeout
with a deterministic wait for the locale change or reload using Playwright’s
wait-for state/URL behavior. Keep the changes within login() so the language
switch remains stable across layout changes and slower reloads.
- Around line 222-223: The “pick tomorrow’s day” calendar selection logic is
duplicated in multiple places in the e2e spec, and the current day-number-only
matcher can hit the wrong month. Extract a small helper such as
pickTomorrow(page) in car-production-buildout-vi.spec.ts and use it from the
material-lot, work-order, and shortage-fill flows to centralize the Date/locator
logic and make the month-scoping fix in one place.
- Around line 353-354: The comments in car-production-buildout-vi.spec.ts are
written in Vietnamese, which violates the English-only guideline for .ts files.
Update the affected comments near the test setup and the later matching comment
around the same scenario to clear English while keeping the UI-facing Vietnamese
strings unchanged; use the surrounding test block and the Report Issue / Work
Order logic to find the exact comments.
- Around line 338-344: The machine-state dropdown locator in the E2E test is too
generic and can trigger Playwright strict-mode failures if multiple select
elements are present. Update the locator in the test around the selectOption
flow to target the intended dropdown more specifically, preferably using a
label-based or test-id-based selector tied to the operator/workstation page
markup. Keep the rest of the interaction logic in place, but make the selection
unambiguous by scoping it to the unique machine-state control.
🪄 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 Plus
Run ID: f2c3a180-bfe7-4d3d-9f84-4db5c68c63a6
⛔ Files ignored due to path filters (3)
backend/composer.lockis excluded by!**/*.lockbackend/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (55)
backend/.rr.yamlbackend/app/Http/Controllers/Web/Admin/ProcessTemplateManagementController.phpbackend/app/Http/Controllers/Web/IssueManagementController.phpbackend/app/Http/Controllers/Web/Operator/BatchController.phpbackend/app/Http/Controllers/Web/Operator/IssueController.phpbackend/app/Http/Controllers/Web/Packaging/PackagingController.phpbackend/app/Http/Requests/Web/Admin/StoreMaterialRequest.phpbackend/app/Http/Requests/Web/Admin/UpdateMaterialRequest.phpbackend/composer.jsonbackend/composer.pharbackend/database/migrations/2026_06_18_120000_make_material_type_id_nullable_on_materials.phpbackend/lang/vi.jsonbackend/resources/js/Pages/admin/lines/Create.jsxbackend/resources/js/Pages/admin/lines/Edit.jsxbackend/resources/js/Pages/admin/lines/Show.jsxbackend/resources/js/Pages/admin/material-lots/Create.jsxbackend/resources/js/Pages/admin/material-lots/Edit.jsxbackend/resources/js/Pages/admin/material-lots/Index.jsxbackend/resources/js/Pages/admin/materials/Create.jsxbackend/resources/js/Pages/admin/materials/Edit.jsxbackend/resources/js/Pages/admin/materials/fields.jsbackend/resources/js/Pages/admin/pallets/Edit.jsxbackend/resources/js/Pages/admin/pallets/PalletForm.jsxbackend/resources/js/Pages/admin/process-templates/Bom.jsxbackend/resources/js/Pages/admin/process-templates/Create.jsxbackend/resources/js/Pages/admin/process-templates/Edit.jsxbackend/resources/js/Pages/admin/process-templates/Show.jsxbackend/resources/js/Pages/admin/product-types/Create.jsxbackend/resources/js/Pages/admin/product-types/Edit.jsxbackend/resources/js/Pages/admin/product-types/Show.jsxbackend/resources/js/Pages/admin/users/Create.jsxbackend/resources/js/Pages/admin/users/Edit.jsxbackend/resources/js/Pages/admin/users/UserForm.jsxbackend/resources/js/Pages/admin/work-orders/Create.jsxbackend/resources/js/Pages/admin/work-orders/Edit.jsxbackend/resources/js/Pages/admin/work-orders/Index.jsxbackend/resources/js/Pages/admin/workstations/Create.jsxbackend/resources/js/Pages/operator/Queue.jsxbackend/resources/js/Pages/operator/SelectLine.jsxbackend/resources/js/Pages/operator/WorkOrderDetail.jsxbackend/resources/js/Pages/operator/Workstation.jsxbackend/resources/js/app.jsxbackend/resources/js/components/LabelPrintMenu.jsxbackend/resources/js/components/ResourceForm.jsxbackend/resources/js/components/ResourceTable.jsxbackend/resources/js/layouts/AppLayout.jsxdocker-compose.ymlplaywright.config.tsreset-test.cmdreset-test_en.cmdreset-test_vi.cmdtests/e2e/car-production-buildout-en.spec.tstests/e2e/car-production-buildout-vi.spec.tstests/e2e/car-production-buildout.spec.tstests/e2e/material-hold-release.spec.ts
💤 Files with no reviewable changes (1)
- backend/database/migrations/2026_06_18_120000_make_material_type_id_nullable_on_materials.php
| } | ||
|
|
||
| if (! in_array($workOrder->status, [WorkOrder::STATUS_DONE, WorkOrder::STATUS_IN_PROGRESS])) { | ||
| if (! in_array($workOrder->status, [WorkOrder::STATUS_DONE, WorkOrder::STATUS_IN_PROGRESS]) && $workOrder->produced_qty <= 0) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== PackagingController outline ==\n'
ast-grep outline backend/app/Http/Controllers/Web/Packaging/PackagingController.php --view expanded || true
printf '\n== WorkOrder outline ==\n'
ast-grep outline backend/app/Models/WorkOrder.php --view expanded || true
printf '\n== Search status/packaging references ==\n'
rg -n "STATUS_(DONE|IN_PROGRESS|REJECTED|CANCELLED)|TERMINAL_STATUSES|packable|produced_qty" backend/app/Http/Controllers/Web/Packaging/PackagingController.php backend/app/Models/WorkOrder.php backend/app -g '*.php' | sed -n '1,220p'Repository: Mes-Open/OpenMes
Length of output: 28292
Keep packaging eligibility limited to DONE/IN_PROGRESS.
backend/app/Http/Controllers/Web/Packaging/PackagingController.php:73, 279-282, 318-328 now accepts any work order with produced_qty > 0, so CANCELLED/REJECTED orders can slip into scan/list/stats. Match the API controller’s allowlist or extract the predicate into a shared scope.
🤖 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 `@backend/app/Http/Controllers/Web/Packaging/PackagingController.php` at line
73, The packaging eligibility check in PackagingController is now too broad
because it allows any work order with produced_qty > 0, which lets non-eligible
statuses like CANCELLED or REJECTED pass through scan, list, and stats paths.
Tighten the condition in the affected PackagingController methods to only allow
WorkOrder::STATUS_DONE and WorkOrder::STATUS_IN_PROGRESS, matching the API
controller’s allowlist, or extract that shared predicate into a reusable
method/scope and use it consistently.
| 'name' => ['required', 'string', 'max:255'], | ||
| 'description' => ['nullable', 'string'], | ||
| 'material_type_id' => ['nullable', 'exists:material_types,id'], | ||
| 'material_type_id' => ['required', 'exists:material_types,id'], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for existing DB seeders/factories creating Material without material_type_id
rg -n -A5 'Material::(create|factory)' --type=php
# Check for migrations backfilling material_type_id
fd -e php . database/migrations | xargs rg -l 'material_type_id'Repository: Mes-Open/OpenMes
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files matching Material / material_type_id =="
git ls-files | rg '(^|/)(Material|material_type_id)|material_type_id|Material.*Factory|Factory.*Material|Seeder|migration'
echo
echo "== occurrences of material_type_id =="
rg -n 'material_type_id' .
echo
echo "== occurrences of Material::create / factory / new Material =="
rg -n 'Material::(create|factory)|new\s+Material\s*\(' .Repository: Mes-Open/OpenMes
Length of output: 46810
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## material-related files\n'
git ls-files | rg '(^|/)(Material|material_type_id|Seeder|Factory|migration)'
printf '\n## material_type_id references\n'
rg -n 'material_type_id' backend database .
printf '\n## Material create/factory usages\n'
rg -n 'Material::(create|factory)|new\s+Material\s*\(' backend database .Repository: Mes-Open/OpenMes
Length of output: 48312
Keep material_type_id optional on admin updates
This makes the web edit form reject materials that intentionally have no type, even though nullable materials are supported elsewhere. Keep the update rule nullable unless you’re also backfilling existing rows and enforcing the field everywhere.
🤖 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 `@backend/app/Http/Requests/Web/Admin/UpdateMaterialRequest.php` at line 46,
The UpdateMaterialRequest validation is too strict for admin edits because
material_type_id is currently required in the update rules. Change the rule in
UpdateMaterialRequest to keep material_type_id optional/nullable on update,
while still preserving the exists check when a value is provided, so the Admin
update flow can save materials without a type.
| import { createInertiaApp } from '@inertiajs/react'; | ||
| import { createRoot } from 'react-dom/client'; | ||
| import { loadLocale, setTimezone } from './lib/i18n'; | ||
| import { loadLocale } from './lib/i18n'; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## app.jsx\n'
ast-grep outline backend/resources/js/app.jsx --view expanded || true
printf '\n## i18n files\n'
fd -a 'i18n.js' backend/resources/js backend/resources || true
fd -a 'locale' backend/resources/js backend/resources || true
printf '\n## search for timezone usage\n'
rg -n "setTimezone|activeTimezone|timezone|Intl\.DateTimeFormat|formatDate|formatTime|formatDateTime|loadLocale" backend/resources/js -g '!**/node_modules/**' || trueRepository: Mes-Open/OpenMes
Length of output: 13747
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## backend/resources/js/app.jsx\n'
cat -n backend/resources/js/app.jsx | sed -n '1,120p'
printf '\n## backend/resources/js/lib/i18n.js\n'
cat -n backend/resources/js/lib/i18n.js | sed -n '1,180p'
printf '\n## timezone prop references outside frontend\n'
rg -n "timezone" backend -g '!**/node_modules/**' -g '!**/vendor/**' | sed -n '1,220p'Repository: Mes-Open/OpenMes
Length of output: 11679
Keep applying the Inertia timezone prop here.
loadLocale(...) no longer calls setTimezone(...), so formatDate*() falls back to the browser timezone. That conflicts with HandleInertiaRequests and lib/i18n.js, which still treat the plant timezone as the canonical zone, and can display production timestamps differently on shared or remote terminals. If browser-local time is intentional, update the backend contract/docs too.
🤖 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 `@backend/resources/js/app.jsx` at line 3, The app bootstrap in app.jsx is no
longer applying the Inertia timezone prop because loadLocale() does not call
setTimezone(), which can make formatDate*() fall back to the browser timezone.
Update the app initialization to keep passing the Inertia timezone value through
the i18n setup, using the existing loadLocale and timezone handling so the plant
timezone remains the canonical source; if you intend browser-local time instead,
adjust the backend contract and docs accordingly.
| value={value || null} | ||
| onChange={(iso) => set(iso ?? '')} | ||
| placeholder={placeholder} | ||
| placeholder={placeholder || __('Select date')} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
DatePicker placeholder isn't translated when explicitly provided.
Other field types translate a caller-supplied placeholder via __(placeholder) (lines 206, 215, 238), but the date field only translates the default fallback, leaving a custom placeholder untranslated.
🌐 Proposed fix
- placeholder={placeholder || __('Select date')}
+ placeholder={placeholder ? __(placeholder) : __('Select date')}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| placeholder={placeholder || __('Select date')} | |
| placeholder={placeholder ? __(placeholder) : __('Select date')} |
🤖 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 `@backend/resources/js/components/ResourceForm.jsx` at line 222, The date field
in ResourceForm’s placeholder handling is skipping translation for
caller-supplied values, unlike the other field types. Update the DatePicker
placeholder logic in ResourceForm.jsx so it passes any explicit placeholder
through __() before falling back to __('Select date'), matching the translation
pattern used by the other branches.
| @@ -1,4 +1,5 @@ | |||
| import { Head, useForm, usePage } from '@inertiajs/react'; | |||
| import { __ } from '../../../lib/i18n'; | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Localization only partially applied — inconsistent with sibling Create.jsx.
Only the Head title and h1 are translated here. Back to Templates (line 36), Template Name (47), the placeholder (54), the descriptive text (58), the checkbox label (66), Cancel (75), and the submit button text (78, including Saving…) remain hardcoded English, while the near-identical Create.jsx fully localizes all of these same strings. This produces a mixed-language experience on the Edit page for non-English users.
🌐 Proposed fix to finish localizing the remaining strings
</svg>
- Back to Templates
+ {__("Back to Templates")}
</a>
<h1 className="text-3xl font-bold text-om-ink">{__("Edit Process Template")}</h1>
<p className="text-sm text-om-muted mt-1">
{productType.name} — Version {processTemplate.version}
</p>
</div>
<div className="card">
<form onSubmit={submit}>
<div className="mb-6">
- <label htmlFor="name" className="form-label">Template Name</label>
+ <label htmlFor="name" className="form-label">{__("Template Name")}</label>
<input
type="text"
id="name"
value={data.name}
onChange={(e) => setData('name', e.target.value)}
className={`form-input w-full${errors.name ? ' border-om-blocked' : ''}`}
- placeholder="e.g., Standard Assembly Process, Quality Inspection v2"
+ placeholder={__("e.g., Standard Assembly Process, Quality Inspection v2")}
required
autoFocus
/>
- <p className="text-sm text-om-muted mt-1">Descriptive name for this manufacturing process</p>
+ <p className="text-sm text-om-muted mt-1">{__("Descriptive name for this manufacturing process")}</p>
{errors.name && <p className="text-om-blocked text-sm mt-1">{errors.name}</p>}
</div>
<div className="mb-6">
<Checkbox
checked={data.is_active}
onChange={(next) => setData('is_active', next)}
- label="Active (template is ready for use in work orders)"
+ label={__("Active (template is ready for use in work orders)")}
/>
</div>
<div className="flex justify-end gap-3">
<a
href={`/admin/product-types/${productType.id}/process-templates`}
className="btn-touch btn-secondary"
>
- Cancel
+ {__("Cancel")}
</a>
<Button type="submit" variant="primary" loading={processing} disabled={processing}>
- {processing ? 'Saving…' : 'Update Template'}
+ {processing ? __("Saving…") : __("Update Template")}
</Button>
</div>
</form>
</div>Also applies to: 25-25, 38-38
🤖 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 `@backend/resources/js/Pages/admin/process-templates/Edit.jsx` at line 2, The
Edit page localization is incomplete and should match the fully translated
Create page. Update the remaining hardcoded strings in Edit.jsx—especially the
navigation link, field label, placeholder, helper text, checkbox label, Cancel
button, and submit/loading text—by using the existing __ i18n helper and the
same translation keys/patterns used in Create.jsx. Use the Edit component’s
Head, h1, and form/button elements as the main locations to align all visible
text with the localized version.
Comment on lines
+390
to
+392
| await Promise.all([ | ||
| page.waitForResponse(res => res.url().includes('complete') && res.status() >= 200), | ||
| completeBtn.click({ force: true }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require a successful complete response.
res.status() >= 200 accepts 4xx/5xx responses, so the test can advance after a failed completion request.
Proposed fix
- page.waitForResponse(res => res.url().includes('complete') && res.status() >= 200),
+ page.waitForResponse(res => res.url().includes('complete') && res.ok()),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await Promise.all([ | |
| page.waitForResponse(res => res.url().includes('complete') && res.status() >= 200), | |
| completeBtn.click({ force: true }) | |
| await Promise.all([ | |
| page.waitForResponse(res => res.url().includes('complete') && res.ok()), | |
| completeBtn.click({ force: true }) |
🤖 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 `@tests/e2e/car-production-buildout-en.spec.ts` around lines 390 - 392, The
completion wait in the e2e test is too permissive because `page.waitForResponse`
currently accepts any status at or above 200, including failures. Update the
`Promise.all` around `completeBtn.click({ force: true })` to only resolve for a
successful completion response (for example, a true 2xx status) so the test
cannot proceed after a 4xx/5xx request.
Comment on lines
+7
to
+12
| const envPath = resolve(__dirname, '../../.env'); | ||
| const envContent = readFileSync(envPath, 'utf-8'); | ||
| const adminPassMatch = envContent.match(/^ADMIN_PASSWORD=(.*)$/m); | ||
|
|
||
| const ADMIN = 'admin'; | ||
| const PASS = adminPassMatch ? adminPassMatch[1].trim() : 'MFRz9GZBkM9UEYTfsaHPLG1P'; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don’t crash when .env is absent.
readFileSync(envPath) runs at import time, so CI or a fresh checkout without a root .env fails before the fallback password can be used.
Proposed fix
-import { readFileSync } from 'fs';
+import { existsSync, readFileSync } from 'fs';
import { resolve } from 'path';
const envPath = resolve(__dirname, '../../.env');
-const envContent = readFileSync(envPath, 'utf-8');
+const envContent = existsSync(envPath) ? readFileSync(envPath, 'utf-8') : '';
const adminPassMatch = envContent.match(/^ADMIN_PASSWORD=(.*)$/m);
const ADMIN = 'admin';
-const PASS = adminPassMatch ? adminPassMatch[1].trim() : 'MFRz9GZBkM9UEYTfsaHPLG1P';
+const PASS = process.env.ADMIN_PASSWORD?.trim() || (adminPassMatch ? adminPassMatch[1].trim() : 'MFRz9GZBkM9UEYTfsaHPLG1P');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const envPath = resolve(__dirname, '../../.env'); | |
| const envContent = readFileSync(envPath, 'utf-8'); | |
| const adminPassMatch = envContent.match(/^ADMIN_PASSWORD=(.*)$/m); | |
| const ADMIN = 'admin'; | |
| const PASS = adminPassMatch ? adminPassMatch[1].trim() : 'MFRz9GZBkM9UEYTfsaHPLG1P'; | |
| import { existsSync, readFileSync } from 'fs'; | |
| import { resolve } from 'path'; | |
| const envPath = resolve(__dirname, '../../.env'); | |
| const envContent = existsSync(envPath) ? readFileSync(envPath, 'utf-8') : ''; | |
| const adminPassMatch = envContent.match(/^ADMIN_PASSWORD=(.*)$/m); | |
| const ADMIN = 'admin'; | |
| const PASS = process.env.ADMIN_PASSWORD?.trim() || (adminPassMatch ? adminPassMatch[1].trim() : 'MFRz9GZBkM9UEYTfsaHPLG1P'); |
🤖 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 `@tests/e2e/car-production-buildout.spec.ts` around lines 7 - 12, The test
setup in car-production-buildout.spec.ts reads the root .env at module import
time, which throws before the fallback password can be used when the file is
missing. Move the .env access behind a safe existence check or lazy load inside
the test setup, and keep the ADMIN_PASSWORD parsing logic in the same place so
the fallback in the ADMIN/PASS constants still works when .env is absent.
Comment on lines
+15
to
+18
| const LINE = `Dây chuyền lắp ráp ô tô ${TS}`; | ||
| const PRODUCT = `Xe điện Sedan ${TS}`; | ||
| const WO = `WO-CAR-${TS}`; | ||
| const TRIGGER = `Car in-line QC ${TS}`; | ||
| const TRIGGER = `QC Trực tuyến ${TS}`; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Keep TypeScript test code and comments in English.
This default spec embeds Vietnamese data, subtitles, selectors, and comments even though the repository rule requires English in all TS code. Since a dedicated Vietnamese spec exists, make this default spec English-only or remove the duplicate localized flow. As per coding guidelines, "**/*.{php,js,jsx,ts,tsx,blade.php}: All code, Blade/JSX text, validation messages, seeders, and comments must be in English".
Also applies to: 47-48, 123-556
🤖 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 `@tests/e2e/car-production-buildout.spec.ts` around lines 15 - 18, The default
e2e spec contains Vietnamese constants, selectors, and comments in English-only
TypeScript code, which violates the repository guideline. Update the flow in
car-production-buildout.spec.ts to use English text throughout, or remove the
duplicated localized scenario if the separate Vietnamese spec already covers it.
Focus on the localized strings and comments near the top-level constants and the
spec blocks so the default test remains English-only.
Source: Coding guidelines
Comment on lines
+370
to
+389
| // Handle lot pick modal if it appears (due to BOM material allocation) | ||
| try { | ||
| const confirmBtn = page.getByRole('button', { name: /(Confirm picks|Xác nhận)/i }).first(); | ||
| await expect(confirmBtn).toBeVisible({ timeout: 5000 }); | ||
|
|
||
| if (await confirmBtn.isDisabled()) { | ||
| const selects = await page.locator('select').all(); | ||
| for (const select of selects) { | ||
| // Select the first actual lot option (index 1, since index 0 is "+ Add lot...") | ||
| await select.selectOption({ index: 1 }); | ||
| await page.waitForTimeout(200); | ||
| } | ||
| } | ||
|
|
||
| await expect(confirmBtn).toBeEnabled({ timeout: 2000 }); | ||
| await confirmBtn.click(); | ||
| } catch (e) { | ||
| // If modal does not appear or we fail to pick, log it so we know | ||
| console.log('Lot pick modal skipped or failed:', e.message); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t swallow lot-picking failures.
The reset script enables lot tracking, so if the modal appears and picking/confirmation fails, the test should fail at that point instead of continuing with a logged message.
Proposed fix
- try {
- const confirmBtn = page.getByRole('button', { name: /(Confirm picks|Xác nhận)/i }).first();
- await expect(confirmBtn).toBeVisible({ timeout: 5000 });
+ const confirmBtn = page.getByRole('button', { name: /(Confirm picks|Xác nhận)/i }).first();
+ if (await confirmBtn.isVisible({ timeout: 5000 }).catch(() => false)) {
if (await confirmBtn.isDisabled()) {
const selects = await page.locator('select').all();
for (const select of selects) {
// Select the first actual lot option (index 1, since index 0 is "+ Add lot...")
await select.selectOption({ index: 1 });
await page.waitForTimeout(200);
}
}
await expect(confirmBtn).toBeEnabled({ timeout: 2000 });
await confirmBtn.click();
- } catch (e) {
- // If modal does not appear or we fail to pick, log it so we know
- console.log('Lot pick modal skipped or failed:', e.message);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Handle lot pick modal if it appears (due to BOM material allocation) | |
| try { | |
| const confirmBtn = page.getByRole('button', { name: /(Confirm picks|Xác nhận)/i }).first(); | |
| await expect(confirmBtn).toBeVisible({ timeout: 5000 }); | |
| if (await confirmBtn.isDisabled()) { | |
| const selects = await page.locator('select').all(); | |
| for (const select of selects) { | |
| // Select the first actual lot option (index 1, since index 0 is "+ Add lot...") | |
| await select.selectOption({ index: 1 }); | |
| await page.waitForTimeout(200); | |
| } | |
| } | |
| await expect(confirmBtn).toBeEnabled({ timeout: 2000 }); | |
| await confirmBtn.click(); | |
| } catch (e) { | |
| // If modal does not appear or we fail to pick, log it so we know | |
| console.log('Lot pick modal skipped or failed:', e.message); | |
| } | |
| const confirmBtn = page.getByRole('button', { name: /(Confirm picks|Xác nhận)/i }).first(); | |
| if (await confirmBtn.isVisible({ timeout: 5000 }).catch(() => false)) { | |
| if (await confirmBtn.isDisabled()) { | |
| const selects = await page.locator('select').all(); | |
| for (const select of selects) { | |
| // Select the first actual lot option (index 1, since index 0 is "+ Add lot...") | |
| await select.selectOption({ index: 1 }); | |
| await page.waitForTimeout(200); | |
| } | |
| } | |
| await expect(confirmBtn).toBeEnabled({ timeout: 2000 }); | |
| await confirmBtn.click(); | |
| } |
🤖 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 `@tests/e2e/car-production-buildout.spec.ts` around lines 370 - 389, The
lot-pick handling in the e2e flow is swallowing failures in the try/catch around
the modal confirmation, which lets the test continue even when lot picking
fails. Update the logic around the confirm button flow in
car-production-buildout.spec.ts so that only the modal-not-present case is
ignored, but any error while selecting lots, waiting for enablement, or clicking
the confirm button is rethrown and fails the test. Keep the existing modal
detection via page.getByRole and confirmBtn, but remove the silent logging path
for actual picker/confirmation errors.
Comment on lines
+395
to
+397
| await Promise.all([ | ||
| page.waitForResponse(res => res.url().includes('complete') && res.status() >= 200), | ||
| completeBtn.click({ force: true }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require a successful complete response.
res.status() >= 200 accepts 4xx/5xx responses, so the test can advance after a failed completion request.
Proposed fix
- page.waitForResponse(res => res.url().includes('complete') && res.status() >= 200),
+ page.waitForResponse(res => res.url().includes('complete') && res.ok()),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await Promise.all([ | |
| page.waitForResponse(res => res.url().includes('complete') && res.status() >= 200), | |
| completeBtn.click({ force: true }) | |
| await Promise.all([ | |
| page.waitForResponse(res => res.url().includes('complete') && res.ok()), | |
| completeBtn.click({ force: true }) |
🤖 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 `@tests/e2e/car-production-buildout.spec.ts` around lines 395 - 397, The
completion flow in the e2e test is too permissive because the response check on
the page.waitForResponse predicate accepts any status at or above 200, including
failed 4xx/5xx results. Update the predicate used with completeBtn.click and
page.waitForResponse in the car-production-buildout spec to require a successful
completion response only (for example, a 2xx range), so the test waits for a
real success before proceeding.
…ipt fail-fast, and e2e spec safety
…t createBtn helper mismatches
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.
Summary
Type of change
Related issue
Closes #
Testing
php artisan testpassesChecklist
.envsecrets committed$fillableupdated if new model columns addedcomposer auditcleanSummary by CodeRabbit
New Features
Bug Fixes
Chores