fix(iec-address): close the paths where address recompaction silently no-ops (DOPE-440) - #1005
fix(iec-address): close the paths where address recompaction silently no-ops (DOPE-440)#1005JulioSergioFS wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 2 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe change adds capability-scoped IEC address allocation, permissive allocation for unresolved targets, centralized Modbus pools, and conditional address recalculation after pin layout changes. Tests cover resolved, unresolved, incapable, and pin-edit scenarios. ChangesIEC address allocation and recalculation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DeviceSlice
participant ProjectSlice
participant IECAddressRegistry
participant AddressPool
DeviceSlice->>DeviceSlice: compare ordered pin-address layout
DeviceSlice->>ProjectSlice: request recalculation when addresses change
ProjectSlice->>IECAddressRegistry: rebuild project-wide registry
IECAddressRegistry->>AddressPool: allocate with target capabilities
AddressPool-->>IECAddressRegistry: return IEC address assignments
IECAddressRegistry-->>ProjectSlice: update registry and allocations
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/store/slices/project/slice.ts (1)
265-272: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate
vendorScreenData['io-mapping']before address allocation.
deviceConfigurationSchemavalidatesvendorScreenDataonly asRecord<string, unknown>. Invalidentriesvalues can makerecalculateIecAddresses()throw or create malformed registry keys. Validate the nested shape with Zod or a type guard, and remove the type assertion at lines 265-272.🤖 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/store/slices/project/slice.ts` around lines 265 - 272, Update readVppEntries to validate vendorScreenData['io-mapping'] before returning entries, using the existing Zod validation approach or a type guard to accept only an object with a VppMappingEntry[] entries property; return an empty array for invalid values. Remove the current type assertion and ensure recalculateIecAddresses receives only validated entries.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/store/__tests__/device-slice.test.ts`:
- Around line 1792-1793: Update modbusAddresses to remove all non-null
assertions and explicitly validate each intermediate fixture value: the first
remote device, its modbusTcpConfig, ioGroups entry, and ioPoints. Throw a
diagnostic error identifying the missing fixture data when any value is absent,
then map the validated IO points to iecLocation.
---
Outside diff comments:
In `@src/frontend/store/slices/project/slice.ts`:
- Around line 265-272: Update readVppEntries to validate
vendorScreenData['io-mapping'] before returning entries, using the existing Zod
validation approach or a type guard to accept only an object with a
VppMappingEntry[] entries property; return an empty array for invalid values.
Remove the current type assertion and ensure recalculateIecAddresses receives
only validated entries.
🪄 Autofix
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 Plus
Run ID: 8378ad7b-5e15-41ea-977b-4f0611120f22
📒 Files selected for processing (9)
docs/iec-address-registry.mdsrc/frontend/store/__tests__/device-slice.test.tssrc/frontend/store/__tests__/project-slice.test.tssrc/frontend/store/slices/device/slice.tssrc/frontend/store/slices/project/slice.tssrc/middleware/shared/utils/iec-address/address-pool.tssrc/middleware/shared/utils/target-capabilities/index.tssrc/middleware/shared/utils/target-capabilities/presets.tssrc/middleware/shared/utils/target-capabilities/types.ts
| const modbusAddresses = (store: ReturnType<typeof makeStore>) => | ||
| store.getState().project.data.remoteDevices![0].modbusTcpConfig!.ioGroups[0].ioPoints!.map((p) => p.iecLocation) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="src/frontend/store/__tests__/device-slice.test.ts"
printf '%s\n' '--- file status ---'
git ls-files -- "$file"
wc -l "$file"
printf '%s\n' '--- relevant source ---'
sed -n '1765,1805p' "$file"
printf '%s\n' '--- helper references ---'
rg -n -C 3 'modbusAddresses|remoteDevices|modbusTcpConfig|ioPoints' "$file"
printf '%s\n' '--- AST outline ---'
ast-grep outline "$file" | sed -n '1,160p'Repository: Autonomy-Logic/openplc-editor
Length of output: 4742
🏁 Script executed:
#!/bin/bash
set -eu
file="src/frontend/store/__tests__/device-slice.test.ts"
printf '%s\n' '--- store factory and fixture setup ---'
sed -n '1,75p' "$file"
sed -n '1720,1871p' "$file"
printf '%s\n' '--- state and action definitions ---'
rg -n -C 4 'remoteDevices|interface .*Project|type .*Project|addIOGroup' src/frontend/store src -g '*.ts' -g '*.tsx' | sed -n '1,260p'
printf '%s\n' '--- read-only structural verifier ---'
python3 - <<'PY'
from pathlib import Path
import re
path = Path("src/frontend/store/__tests__/device-slice.test.ts")
text = path.read_text()
lines = text.splitlines()
helper = next(
(i + 1 for i, line in enumerate(lines)
if "const modbusAddresses" in line),
None,
)
assert helper is not None, "modbusAddresses helper is missing"
helper_lines = "\n".join(lines[helper - 1:helper + 1])
assert helper_lines.count("!") == 3, helper_lines
assert text.count("modbusAddresses(store)") == 4, "unexpected helper call count"
fixture_start = next(
i for i, line in enumerate(lines)
if line.startswith("function seedPinsAndGroup")
)
fixture_end = next(
i for i in range(fixture_start + 1, len(lines))
if lines[i].startswith(" const modbusAddresses")
)
fixture = "\n".join(lines[fixture_start:fixture_end])
for required in ("remoteDevices:", "modbusTcpConfig:", "ioGroups: []", "ioPoints: []"):
assert required in fixture, f"fixture does not contain {required}"
print(f"helper line: {helper}")
print("non-null assertions in helper: 3")
print("modbusAddresses(store) call sites: 4")
print("fixture initializes remoteDevices, modbusTcpConfig, ioGroups, and ioPoints")
PYRepository: Autonomy-Logic/openplc-editor
Length of output: 29306
🏁 Script executed:
#!/bin/bash
set -eu
file="src/frontend/store/__tests__/device-slice.test.ts"
printf '%s\n' '--- store factory and fixture setup ---'
sed -n '1,75p' "$file"
sed -n '1720,1871p' "$file"
printf '%s\n' '--- state and action definitions ---'
rg -n -C 4 'remoteDevices|interface .*Project|type .*Project|addIOGroup' src/frontend/store src -g '*.ts' -g '*.tsx' | sed -n '1,260p'
printf '%s\n' '--- read-only structural verifier ---'
python3 - <<'PY'
from pathlib import Path
path = Path("src/frontend/store/__tests__/device-slice.test.ts")
text = path.read_text()
lines = text.splitlines()
helper = next(
(i + 1 for i, line in enumerate(lines)
if "const modbusAddresses" in line),
None,
)
assert helper is not None, "modbusAddresses helper is missing"
helper_lines = "\n".join(lines[helper - 1:helper + 1])
assert helper_lines.count("!") == 3, helper_lines
assert text.count("modbusAddresses(store)") == 4, "unexpected helper call count"
fixture_start = next(
i for i, line in enumerate(lines)
if line.startswith("function seedPinsAndGroup")
)
fixture_end = next(
i for i in range(fixture_start + 1, len(lines))
if lines[i].startswith(" const modbusAddresses")
)
fixture = "\n".join(lines[fixture_start:fixture_end])
for required in ("remoteDevices:", "modbusTcpConfig:", "ioGroups: []", "ioPoints: []"):
assert required in fixture, f"fixture does not contain {required}"
print(f"helper line: {helper}")
print("non-null assertions in helper: 3")
print("modbusAddresses(store) call sites: 4")
print("fixture initializes remoteDevices, modbusTcpConfig, ioGroups, and ioPoints")
PYRepository: Autonomy-Logic/openplc-editor
Length of output: 32651
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact project and Modbus type declarations ---'
rg -n -C 6 'PLCProjectData|PLCRemoteDevice|ModbusIOGroup|ioPoints\??|remoteDevices\??|modbusTcpConfig\??' \
src/backend/shared src/frontend/store/slices/project src/middleware/shared \
-g '*.ts' | sed -n '1,320p'
printf '%s\n' '--- addIOGroup implementation ---'
rg -n -C 12 'addIOGroup\s*[:=]|function addIOGroup|addIOGroup\(' \
src/frontend/store/slices/project -g '*.ts' | sed -n '1,260p'
printf '%s\n' '--- focused structural verifier ---'
python3 - <<'PY'
from pathlib import Path
path = Path("src/frontend/store/__tests__/device-slice.test.ts")
lines = path.read_text().splitlines()
helper_index = next(i for i, line in enumerate(lines) if "const modbusAddresses" in line)
helper = "\n".join(lines[helper_index:helper_index + 2])
assert helper.count("!") == 3
assert sum("modbusAddresses(store)" in line for line in lines) == 4
seed_index = next(i for i, line in enumerate(lines) if "function seedPinsAndGroup" in line)
helper_index = next(i for i, line in enumerate(lines) if "const modbusAddresses" in line)
fixture = "\n".join(lines[seed_index:helper_index])
required = ("remoteDevices:", "modbusTcpConfig:", "ioGroups: []", "ioPoints: []")
missing = [item for item in required if item not in fixture]
assert not missing, missing
print(f"helper source line: {helper_index + 1}")
print("non-null assertions in helper: 3")
print("helper call sites: 4")
print("fixture initializes all four expected Modbus containers")
PYRepository: Autonomy-Logic/openplc-editor
Length of output: 32877
Remove the non-null assertions from modbusAddresses.
Narrow each intermediate value explicitly, including the first remote device, its Modbus configuration, and its IO points. Throw a diagnostic error when the fixture is incomplete.
🤖 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/store/__tests__/device-slice.test.ts` around lines 1792 - 1793,
Update modbusAddresses to remove all non-null assertions and explicitly validate
each intermediate fixture value: the first remote device, its modbusTcpConfig,
ioGroups entry, and ioPoints. Throw a diagnostic error identifying the missing
fixture data when any value is absent, then map the validated IO points to
iecLocation.
Source: Coding guidelines
Review — good to go after one small test cleanupReviewed together with the web sync PR Autonomy-Logic/openplc-web#664 — the 8 shared files are byte-identical; Read against the ticket (DOPE-440: "IEC addresses not recalculated after a gap … May also affect other plugins", High) rather than just the diff. Verified: 8/8 shared files byte-identical ( The shape of this PR is right"The scenario named in the ticket was already fixed" — and saying so, then going after the paths where the recompaction silently no-ops, is more useful than either re-fixing it or closing as cannot-reproduce. The core is one function and it reads clearly: function allocationCapabilities(live: ProjectSliceRoot): AddressProducerCapabilities {
const boardInfo = resolveBoardInfo(live)
return boardInfo ? resolveTargetCapabilities(boardInfo) : ALL_ADDRESS_PRODUCERS_ACTIVE
}The bug underneath it is worth restating because it's the kind that hides forever: Separating "permissive for allocation" from "empty for feature gating" into two distinct presets, instead of overloading one, is what makes that fix safe to reason about. The ticket's "may also affect other plugins" clauseThat turned out literally true for pin mapping, and finding it required taking the clause seriously rather than treating it as boilerplate: The guard on top of it is the detail I'd single out — all three pin actions compare the pin address signature and recalculate only when it moves, so alias-only and pin-number-only edits stay free. That's what keeps a correctness fix from turning into a performance regression on every keystroke. 🟡 1 — CodeRabbit's open finding: non-null assertions in the test helper
store.getState().project.data.remoteDevices![0].modbusTcpConfig!.ioGroups[0].ioPoints!.map((p) => p.iecLocation)Why it's worth doing: CLAUDE.md is explicit ("No non-null assertions ( Note this file is mirrored, so the fix has to land in both repos byte-identically. 🟡 2 — Document the consequence of "unknown ⇒ permissive", next to the invariant§5.1 records "unknown target ⇒ permissive for allocation, empty for feature gating", which is the right invariant. What it doesn't record is what follows from it: a project authored while the target is unresolved allocates with every producer active, so when the VPP is later installed and the target resolves, capabilities shrink and the addresses recompact — the user sees addresses move on the first open after installing the package. Why it's worth a sentence. For alias-bound variables that's transparent, which is what the registry exists for. For variables bound to a literal CoordinationThis pair and the DOPE-439 pair both touch Review assisted by Claude Code. |
On the third bullet — forcing
|
3 participants
Pull request info
Description of the changes proposed
The scenario named in the ticket was already fixed. The central registry work
(Phases 3–6, July) made
deleteIOGroupcallrecalculateIecAddresses(), and the testrecompacts the surviving groups into the freed slots (bug #4)has passed ever since.The ticket predates that work and was never re-verified. What remained are the paths
where the recompaction silently no-ops, plus the ticket's own "May also affect other
plugins" clause — which turns out to be literally true for pin mapping.
resolveTargetCapabilities(undefined)answers
EMPTY_CAPABILITIES; feeding that to the allocator filters out every consumer,so
assignmentscomes back empty and the write-back leaves the stale addresses in placewhile reporting success. A board id fails to resolve whenever the VPP package isn't
installed, the project was authored elsewhere, or the catalogue hasn't loaded yet. A board
that answered is now honoured exactly as declared (the deliberate kind-deactivation on
target switch is untouched); a board that did not resolve allocates with every producer
active.
so on a Runtime v3 or unresolved target every new group restarted at
%IW0.modbus-tcp-remoteis now always counted in its own pool. This also collapses the threeduplicated ~25-line pool preambles into one helper.
removePinslides the freed slot to the end of the pinblock;
createNewPincan mint an address already held by a VPP or Modbus channel, with noconflict report. All three pin actions now compare the pin address signature and recalculate
only when it moves, so alias-only and pin-number-only edits stay free.
docs/iec-address-registry.mdcapturing the invariant"unknown target ⇒ permissive for allocation, empty for feature gating".
Summary by CodeRabbit
Bug Fixes
Documentation