feat(ethercat): surface dedup-after-retry ESI uploads in the UI#849
feat(ethercat): surface dedup-after-retry ESI uploads in the UI#849marconetsf wants to merge 4 commits into
Conversation
Shared-surface sync with openplc-web (feat/esi-upload-retry): mirror the EsiPort `dedupAfterRetry` contract field and the ESIUpload consumer that re-lists the repository when an upload is recovered after a transient-failure retry but comes back without an item — so the file surfaces instead of silently vanishing. Adds the byte-identical esi-upload.test.tsx covering the four branches (normal add, dedup-after-retry refetch, refetch-failure fallback, real duplicate). Retry/adapter internals stay web-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WalkthroughThis PR extends the ESI file upload flow to handle a ChangesESI Upload Deduplication After Retry
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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: 2
🤖 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
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/__tests__/esi-upload.test.tsx:
- Line 1: The test file
src/frontend/components/_features/[workspace]/editor/device/ethercat/components/__tests__/esi-upload.test.tsx
is present in this repo but missing from the openplc-web repository, causing the
surface-sync CI check to fail; either add the identical file to the web repo on
the same target branch or coordinate with the web repo maintainers to merge the
corresponding PR (e.g., the one that should contain esi-upload.test.tsx) before
merging this PR so the byte-identical test is present in both repos and the
compare-surfaces.py check passes.
In
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/esi-upload.tsx:
- Around line 116-127: The UI branch relying on result.dedupAfterRetry is dead
because dedupAfterRetry isn't propagated end-to-end; either implement
propagation or remove the UI branch. To implement propagation: update backend
function parseAndSaveFile in esi-service.ts to set and return
dedupAfterRetry:boolean when appropriate, update the IPC handler
esiParseAndSaveFile in src/main/modules/ipc/renderer.ts to include
dedupAfterRetry in its response, and update the middleware adapter
editor/esi-adapter.ts mapping to pass dedupAfterRetry through when calling
EsiPort.parseAndSaveFile. Alternatively, remove the
needsRepositoryRefresh/dedupAfterRetry handling from the UI component
(esi-upload.tsx) to match current backend behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 007b5214-9cca-4570-b6b1-da66d7e84b70
📒 Files selected for processing (3)
src/frontend/components/_features/[workspace]/editor/device/ethercat/components/__tests__/esi-upload.test.tsxsrc/frontend/components/_features/[workspace]/editor/device/ethercat/components/esi-upload.tsxsrc/middleware/shared/ports/esi-port.ts
| @@ -0,0 +1,96 @@ | |||
| import type { ESIRepositoryItemLight } from '@root/middleware/shared/ports/esi-types' | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if openplc-web#494 is merged and verify file existence
# Check if we can query the web repo (requires GitHub CLI with appropriate access)
echo "=== Checking openplc-web PR `#494` status ==="
gh pr view 494 --repo Autonomy-Logic/openplc-web --json state,title,merged,mergedAt 2>/dev/null || echo "Cannot access openplc-web repo or PR does not exist"
echo -e "\n=== Checking if test file exists in openplc-web ==="
gh api repos/Autonomy-Logic/openplc-web/contents/src/frontend/components/_features/workspace/editor/device/ethercat/components/__tests__/esi-upload.test.tsx 2>/dev/null && echo "File exists in web repo" || echo "File does NOT exist in web repo"Repository: Autonomy-Logic/openplc-editor
Length of output: 381
🏁 Script executed:
# Find CI configuration files
find . -type f \( -name "*.yml" -o -name "*.yaml" \) | grep -E "(workflow|action|ci)" | head -20Repository: Autonomy-Logic/openplc-editor
Length of output: 551
🏁 Script executed:
# Check for documentation about repository sync or relationships
fd -t f -E node_modules -E dist \( -name "README*" -o -name "CONTRIBUTING*" -o -name ".github" \)Repository: Autonomy-Logic/openplc-editor
Length of output: 304
🏁 Script executed:
# Look for any sync-related scripts or configurations
rg "surface.*sync|sync.*surface" --type ts --type tsx --type js --type jsx --type yml --type yaml -l 2>/dev/null | head -20Repository: Autonomy-Logic/openplc-editor
Length of output: 55
🏁 Script executed:
cat -n ./.github/workflows/ci-sync.ymlRepository: Autonomy-Logic/openplc-editor
Length of output: 19516
🏁 Script executed:
# Check if there's documentation about the sync strategy
rg -i "surface.*sync|byte.*identical|mirror" --type md --type txt -A 3 -B 1Repository: Autonomy-Logic/openplc-editor
Length of output: 1819
🏁 Script executed:
# Look for any environment variables or configuration related to sync
rg "SYNC|surface" .github/workflows/ci-sync.ymlRepository: Autonomy-Logic/openplc-editor
Length of output: 989
🏁 Script executed:
cat -n scripts/compare-surfaces.py | head -100Repository: Autonomy-Logic/openplc-editor
Length of output: 3414
🏁 Script executed:
# Find the SURFACES definition that's referenced in ci-sync.yml
find . -type f -name "*.py" -o -name "*.json" -o -name "*.ts" -o -name "*.tsx" | xargs grep -l "SURFACES" 2>/dev/null | grep -v node_modules | head -10Repository: Autonomy-Logic/openplc-editor
Length of output: 144
🏁 Script executed:
# Look for configuration about what constitutes a "surface"
fd -t f "surface" --type f | grep -v node_modules | head -20Repository: Autonomy-Logic/openplc-editor
Length of output: 140
🏁 Script executed:
cat -n scripts/compare-surfaces.py | tail -50Repository: Autonomy-Logic/openplc-editor
Length of output: 2028
🏁 Script executed:
# Check if there are any filters or exclusions for test files in the comparison
grep -n "test\|spec\|__tests__\|skip\|exclude\|ignore" scripts/compare-surfaces.pyRepository: Autonomy-Logic/openplc-editor
Length of output: 55
🏁 Script executed:
# Verify the exact test file path structure in the editor repo
find src/frontend -path "*esi-upload.test.tsx" -o -path "*esi-upload*" 2>/dev/nullRepository: Autonomy-Logic/openplc-editor
Length of output: 278
Test file must be synced to openplc-web repository.
Test files are included in the byte-identical surface comparison (no exclusions exist in compare-surfaces.py). This file exists in the editor repo but is missing from the web repo, which will cause the CI surface-sync check to fail.
The sync check will report an error and block the merge unless:
- A corresponding web PR (such as
#494) with this file is merged first, OR - The file is added to the web repo on the same target branch.
Coordinate with the web repository to ensure this test file is synced before merging.
🧰 Tools
🪛 GitHub Actions: CI / 3_sync _ Shared Surface Sync.txt
[error] 1-1: compare-surfaces.py reported a mismatch: file exists only in editor repo (not in web/src).
🪛 GitHub Actions: CI / sync _ Shared Surface Sync
[error] 1-1: [frontend] File exists only in editor repo
🤖 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
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/__tests__/esi-upload.test.tsx
at line 1, The test file
src/frontend/components/_features/[workspace]/editor/device/ethercat/components/__tests__/esi-upload.test.tsx
is present in this repo but missing from the openplc-web repository, causing the
surface-sync CI check to fail; either add the identical file to the web repo on
the same target branch or coordinate with the web repo maintainers to merge the
corresponding PR (e.g., the one that should contain esi-upload.test.tsx) before
merging this PR so the byte-identical test is present in both repos and the
compare-surfaces.py check passes.
| // A recovered-but-unlisted upload (dedupAfterRetry) is on the backend yet | ||
| // missing from `newItems`. Re-list the repository so it shows up; fall | ||
| // back to the locally accumulated list if the refresh itself fails. | ||
| if (needsRepositoryRefresh) { | ||
| const refreshed = await esi!.loadRepositoryLight() | ||
| if (refreshed.success && refreshed.items) { | ||
| onFilesLoaded(refreshed.items, errors.length > 0 ? errors : undefined) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| onFilesLoaded([...repository, ...newItems], errors.length > 0 ? errors : undefined) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check IPC bridge type for esiParseAndSaveFile
# Search for the bridge method signature
rg -n -A5 'esiParseAndSaveFile.*Promise' --type=tsRepository: Autonomy-Logic/openplc-editor
Length of output: 55
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find all occurrences of dedupAfterRetry
rg -n --hidden --no-ignore-vcs "dedupAfterRetry" . || true
# 2) Find ESI port / loadRepositoryLight in the repo
rg -n --hidden --no-ignore-vcs "loadRepositoryLight" src || true
rg -n --hidden --no-ignore-vcs "EsiPort" src || true
# 3) Inspect main/renderer IPC bridge files for any ESI-related methods
rg -n --hidden --no-ignore-vcs "(esi|Esi).*Parse|esiParse|loadRepository" src/main/modules/ipc || true
rg -n --hidden --no-ignore-vcs "window\\.bridge\\." src/middleware/adapters/editor || true
# 4) Directly search for IPC method declarations mentioning ESI in renderer bridge
rg -n --hidden --no-ignore-vcs "ipc.*esi|esi.*ipc|loadRepositoryLight|RepositoryLight" src/main/modules/ipc/renderer.ts || trueRepository: Autonomy-Logic/openplc-editor
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If dedupAfterRetry exists, print surrounding type declarations in main IPC renderer bridge
# (Use a small window to avoid huge output)
for pat in "dedupAfterRetry" "loadRepositoryLight" "RepositoryLight" "needsRepositoryRefresh" ; do
echo "---- pattern: $pat ----"
rg -n -A12 -B4 "$pat" src/main/modules/ipc/renderer.ts src/main/modules/ipc/main.ts || true
doneRepository: Autonomy-Logic/openplc-editor
Length of output: 6062
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print EsiPort contract for parseAndSaveFile (includes dedupAfterRetry)
sed -n '1,120p' src/middleware/shared/ports/esi-port.ts
# Print exact IPC renderer bridge signature for esiParseAndSaveFile
# (small window around the line indicated in the earlier grep)
sed -n '540,640p' src/main/modules/ipc/renderer.ts
# Print editor ESI adapter mapping for parseAndSaveFile
sed -n '1,140p' src/middleware/adapters/editor/esi-adapter.tsRepository: Autonomy-Logic/openplc-editor
Length of output: 11447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/frontend/components/_features/[workspace]/editor/device/ethercat/components/esi-upload.tsxRepository: Autonomy-Logic/openplc-editor
Length of output: 7707
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the main-process handler implementation
rg -n "handleESIParseAndSaveFile" src/main/modules/ipc/main.ts
# Print the handler body
rg -n -A60 "handleESIParseAndSaveFile" src/main/modules/ipc/main.ts
# Locate backend ESI service parse method
rg -n "parseAndSaveFile" src/backend -S
# Print likely method signature / return type around dedupAfterRetry
rg -n -A40 -B5 "dedupAfterRetry" src/backend/editor src/backend/shared src/backend -S || trueRepository: Autonomy-Logic/openplc-editor
Length of output: 6076
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the parseAndSaveFile implementation area
rg -n -A140 -B20 "async parseAndSaveFile" src/backend/editor/ethercat/esi-service.ts
# Also look for dedupAfterRetry usage in backend
rg -n "dedupAfterRetry" src/backend/editor/ethercat/esi-service.ts src/backend/shared -S || trueRepository: Autonomy-Logic/openplc-editor
Length of output: 5383
Propagate dedupAfterRetry consistently through backend → IPC bridge → EsiPort adapter (or remove the UI dead branch)
EsiPort.parseAndSaveFilealready modelsdedupAfterRetry?: booleaninsrc/middleware/shared/ports/esi-port.ts.src/backend/editor/ethercat/esi-service.tsparseAndSaveFilecurrently never returnsdedupAfterRetry(onlyitem?,duplicate?,error?).src/main/modules/ipc/renderer.tsesiParseAndSaveFilereturn type also omitsdedupAfterRetry.src/middleware/adapters/editor/esi-adapter.tsdrops any such field (it maps onlyitemandduplicate).
This makes the ESIUpload path that sets needsRepositoryRefresh on result.dedupAfterRetry effectively unreachable in the real flow; it only works when mocked in tests. Fix by either implementing dedupAfterRetry end-to-end (service → IPC bridge → adapter) or removing dedupAfterRetry from the port/UI until the backend supports 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
`@src/frontend/components/_features/`[workspace]/editor/device/ethercat/components/esi-upload.tsx
around lines 116 - 127, The UI branch relying on result.dedupAfterRetry is dead
because dedupAfterRetry isn't propagated end-to-end; either implement
propagation or remove the UI branch. To implement propagation: update backend
function parseAndSaveFile in esi-service.ts to set and return
dedupAfterRetry:boolean when appropriate, update the IPC handler
esiParseAndSaveFile in src/main/modules/ipc/renderer.ts to include
dedupAfterRetry in its response, and update the middleware adapter
editor/esi-adapter.ts mapping to pass dedupAfterRetry through when calling
EsiPort.parseAndSaveFile. Alternatively, remove the
needsRepositoryRefresh/dedupAfterRetry handling from the UI component
(esi-upload.tsx) to match current backend behavior.
1 participant
What
Shared-surface sync with openplc-web PR Autonomy-Logic/openplc-web#494.
Mirrors, byte-identical, the three shared-surface files from that PR:
middleware/shared/ports/esi-port.ts— thededupAfterRetrycontract field onparseAndSaveFile.frontend/.../ethercat/components/esi-upload.tsx— theESIUploadconsumer that re-lists the repository when an upload is recovered after a transient-failure retry but comes back without an item, so the file surfaces instead of silently vanishing.frontend/.../components/__tests__/esi-upload.test.tsx— the four-branch test (normal add, dedup-after-retry refetch, refetch-failure fallback, real duplicate). Runner-agnostic mock, runs under Jest here and Vitest on web.Why
Keeps the shared surface in lockstep with openplc-web (enforced by the cross-repo surface-sync CI). The retry/adapter internals are web-only and intentionally not ported — the editor's main-process ESI adapter doesn't retry, and the new
dedupAfterRetryfield is optional, so this is backward compatible.Tests
esi-upload.test.tsxpasses under the editor's Jest (4/4).🤖 Generated with Claude Code
Summary by CodeRabbit