Repository navigation
fix(iec-address): close the paths where address recompaction silently no-ops (DOPE-440) - #1005
JulioSergioFS wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe change adds capability-scoped IEC allocation, permissive allocation for unresolved targets, centralized Modbus pools, target-aware remote-device handling, and conditional 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 Mergeability Score: ⚪ Minimal · up to The PR is merge-ready after normal checks and review; no actionable merge-blocking risk remains. 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 resolved or permissive capabilities
AddressPool-->>IECAddressRegistry: return IEC address assignments
IECAddressRegistry-->>ProjectSlice: update registry and allocations
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
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
|
…r into bugfix/DOPE-440-iec-address-recompaction-gaps
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
New Features
Bug Fixes
Documentation