feat(data-types): make .dt persistence a deploy setting and survivable to roll back (DOPE-542) - #999
feat(data-types): make .dt persistence a deploy setting and survivable to roll back (DOPE-542)#999JoaoGSP wants to merge 3 commits into
Conversation
…e to roll back Resolve the flag from a build-time injected global instead of a hard-coded constant. The original plan — true on `development`, reverted to false on the promotion branch — inverts the safety: every promotion needs someone to remember the revert, and nothing catches a miss, since the mirror gate compares web against editor rather than development against main. Injecting it keeps the source identical on both branches and makes "on in staging, off in production" a deploy setting. Same mechanism the repos already use for APP_NAME / BUILD_DATE. Ungate the read. The flag gated reading `.dt` files as well as writing them, so a flag-off build hydrated from the emptied `data.dataTypes` and showed no data types at all — a rollback was a data-loss event. Reading unconditionally makes a flag-off build open a migrated project and write the legacy form back on the next save, so the flag can be turned off and on again. Delete the `.dt` files on a flag-off save. Hydration prefers those files whenever they exist, and desktop leaves them on disk, so a project edited with the flag off would silently revert to the pre-rollback copy the next time the flag was on. Web already dropped them by omission; this makes the two platforms agree. Write `project.json` last. Every file went out in one unordered `Promise.all`, so a rejected `.dt` write after the index had landed — already carrying `dataTypes: []` — lost that type from both places. The first save of a migrating project is exactly when that bites. Split on the project-json category rather than on `.dt`: the index also declares `pous: []`, so POUs carry the identical exposure. The flag ships off, and merging this changes nothing on its own — the staging build has to set DATATYPES_DT_FILES. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvGrhB1LyJz9MBwYhjoBDY
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change adds build-time control for datatype-file persistence, keeps datatype files loadable, removes disabled files during saves, and writes content files before ChangesDatatype persistence
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant executeSaveProject
participant isDataTypeFilesEnabled
participant ProjectService.writeProjectFiles
participant Filesystem
executeSaveProject->>isDataTypeFilesEnabled: read DATATYPES_DT_FILES
isDataTypeFilesEnabled-->>executeSaveProject: return enabled or disabled
executeSaveProject->>ProjectService.writeProjectFiles: submit datatype files and deletions
ProjectService.writeProjectFiles->>Filesystem: write content files in parallel
Filesystem-->>ProjectService.writeProjectFiles: complete or reject content writes
ProjectService.writeProjectFiles->>Filesystem: write project.json after successful content writes
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 3
🤖 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/backend/editor/services/project-service/index.ts`:
- Line 500: Update writeProjectFiles to use Promise.allSettled for content
writes so every write completes before returning failure; exclude project.json
from this batch, detect any rejected result, and preserve the { success: false }
outcome. Add a regression test covering one rejected content write alongside one
delayed write.
In `@src/frontend/utils/__tests__/feature-flags.test.ts`:
- Line 11: Remove the prohibited type assertions across the listed test sites:
in src/frontend/utils/__tests__/feature-flags.test.ts:11-11, replace the
globalThis assertion with Reflect.set and Reflect.deleteProperty; in
src/middleware/adapters/editor/__tests__/project-adapter.test.ts:355-355 and
:369-369, use typed mock references or jest.mocked/vi.mocked for the configured
mock; and in src/frontend/services/__tests__/save-actions.test.ts:125-125,
retain saveProject as a typed test fixture instead of casting
ProjectPort.saveProject. Allow only as const assertions.
In `@src/middleware/adapters/editor/project-adapter.ts`:
- Around line 205-210: The dataTypeFiles handling in both project-open paths
only validates the array container, allowing malformed entries into
parseProjectFiles. Add a shared Zod schema or type guard for individual IPC file
entries, filter or reject invalid payloads before parsing, and preserve the
empty-array fallback only when the field is absent; use this helper at both
visible dataTypeFiles call sites.
🪄 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: d84a5f6d-52ce-4919-99d1-87fb65f6019b
📒 Files selected for processing (10)
configs/webpack/webpack.app-info.tssrc/backend/editor/services/project-service/__tests__/write-project-files.test.tssrc/backend/editor/services/project-service/index.tssrc/frontend/services/__tests__/save-actions.test.tssrc/frontend/services/save-actions.tssrc/frontend/utils/__tests__/feature-flags.test.tssrc/frontend/utils/feature-flags.tssrc/globals.d.tssrc/middleware/adapters/editor/__tests__/project-adapter.test.tssrc/middleware/adapters/editor/project-adapter.ts
… failure `Promise.all` rejects on the first failure but leaves the other writes running, so the save returned while the disk was still being touched — a straggler from the failed attempt could land after, and overwrite, a write from the user's retry. `allSettled` holds until the batch drains, then rethrows, so `project.json` is still skipped on failure. Also drops the type assertions from the new test cases: `Reflect` for the injected global, `vi.mocked` for the save-port spy. Reported by review on #999. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvGrhB1LyJz9MBwYhjoBDY
JulioSergioFS
left a comment
JulioSergioFS
left a comment
There was a problem hiding this comment.
PR Review — openplc-editor #999
Verdict: Approve with comments — one item worth fixing before this is turned on in staging
Three genuinely good decisions here: injecting the flag instead of hard-coding it, ungating the read so a rollback is a round trip rather than a data-loss event, and ordering project.json after the content files. The write-order fix in particular is the kind of latent bug that only shows up on the first save of a migrating project, i.e. exactly the population this release targets.
The one thing I would address before the flag goes on: the rollback guarantee rests on a deletion loop that swallows failures and still reports success (finding 1), and the load path has a related sharp edge that the ungated read newly exposes (finding 2).
What I verified against the code
- Every
isDataTypeFilesEnabled()consumer is renderer-side —frontend/services/save-actions.ts,frontend/services/st-lsp/*,frontend/components/.../data-type/index.tsx— and both renderer webpack configs spreadgetAppInfoDefines()(webpack.config.renderer.dev.ts:179,webpack.config.renderer.prod.ts:168). No main-process consumer that would read an un-injectedundefined. Removing the import frommiddleware/adapters/editor/project-adapter.tsis consistent with that. - The main-process read was already unconditional (
project-service/index.ts:293readsdatatypes/**.dtregardless of any flag), so ungating the adapter makes the two halves agree rather than opening a new path. - Rename and delete already queue the old
.dtintopendingDeletions(store/slices/project/slice.ts:1116and:1139), solegacyDataTypeCleanupcomposes with them instead of duplicating; thenew Set(...)dedupe and its test pin that. - A filename/declared-name mismatch cannot escape the cleanup.
parse-project-files.tspassesgetBaseNameFromPath(relativePath)asexpectedNametoparseDataTypeFromText, so a.dtwhose declared type name differs from its filename is routed tounparsedDataTypeFiles— which the cleanup list also covers. The "derive the path fromdt.name" shortcut is safe for that reason, not by luck. - The
category === 'project-json'split matches the generator.iterate-write-project-files.tsyields exactly oneproject-jsonentry withrelativePath: 'project.json'; splitting on the category rather than on.dtalso covers thepous: []exposure, as the description says.
Strengths
- The injected-global argument is the right one, and it is stated correctly. "A branch that has to be edited during promotion is a branch someone forgets to edit" — and the mirror gate compares web↔editor, not development↔main, so nothing would have caught the missed revert. Reusing the
APP_NAME/BUILD_DATEmechanism keeps this boring. - The
typeofguard plus module-scope capture is correct across all three environments — webpackDefinePlugin, Vitedefine, and un-injected Jest/Vitest — and the tests exercise all three byjest.resetModules()+ re-import rather than asserting on one. - Ungating the read is the single most important change here. A flag gating both sides made rollback destructive in the plainest possible way: a flag-off build hydrated from an emptied
data.dataTypesand showed nothing. Making the write the only gated side is the correct asymmetry. - The write-order bug was found by walking the round trip, not by a test failing, and the fix generalises past the symptom.
allSettled-then-rethrow instead ofallis also right for a non-obvious reason that the comment states: it stops a straggler from a failed save landing on top of a write from the user's retry. - The manual validation is the right validation.
PROGRAM_MD5identical before and after migration is a much stronger claim than "the tests pass", and the note thatprogram.stisn't persisted (comparebuild/<board>/src/defines.h) will save the next person an hour. - The description correctly refuses to let merge be mistaken for release. Naming that the flag is still dark, and asking that the card not be closed on merge, is exactly right for a PR whose whole point is that turning it on is a separate act.
Findings
1. (Major) The rollback guarantee rests on deletions that are swallowed and still reported as success
ProjectService.writeProjectFiles processes deletions after project.json lands, catching per file:
} catch (deleteError) {
console.error(`Error deleting file ${filePath}:`, deleteError)
}and then returns { success: true, message: 'Your project was saved successfully' }.
So on Windows, with datatypes/Motor.dt held open by an indexer, AV scanner or another editor, a flag-off save writes the legacy project.json, fails to unlink the .dt, and tells the user the project saved. On the next open, hydration prefers the .dt (parse-project-files.ts: dataTypeFiles.length > 0 ? dataTypesFromFiles : data.dataTypes) and the stale pre-rollback copy silently outranks the edit — which is the exact bug section 3 of this PR exists to fix, reached through a different door.
Because the deletions run after project.json has already landed carrying the full legacy data, nothing is lost by failing loudly here. Suggested: collect the failed relative paths and either return success: false with them named, or surface a specific warning that names them. Silence is the one option that reintroduces the bug.
2. (Medium) A single unparseable .dt now empties legacy dataTypes on load — and the next save persists that
parse-project-files.ts decides the source of truth on dataTypeFiles.length > 0, which counts files that failed to parse:
dataTypes: dataTypeFiles.length > 0 ? dataTypesFromFiles : ((data.dataTypes as PLCDataType[]) ?? []),Before this PR, a flag-off build passed [] into the parser, so project.json always won and this was unreachable off the flag. Now the read is unconditional, so one unreadable datatypes/Broken.dt sitting next to a legacy project.json yields dataTypes: [] in the store — and a flag-off save then writes that emptiness back (save-actions.ts:80: dataTypes: isDataTypeFilesEnabled() ? [] : project.data.dataTypes) while legacyDataTypeCleanup deletes Broken.dt. The raw file is preserved in unparsedDataTypeFiles, but the legacy types are gone from both places.
This is directly connected to the write-order fix's own motivating scenario: a .dt write that rejects part-way leaves a truncated file on disk. The fix correctly protects project.json in that moment — but on reopen, that truncated file is enough to hide every legacy type in the UI even though project.json still holds them.
Suggested: fall back to data.dataTypes when dataTypesFromFiles is empty and every .dt failed to parse — i.e. key on "we successfully read at least one type from files", not on "at least one file exists".
3. (Medium) Nothing in this repo turns the flag on, and a build with it off is indistinguishable from one with it on
The safety argument is "the flag is a deploy setting" — but no workflow, .env sample, or build doc in this PR sets DATATYPES_DT_FILES, so "the staging build has to set it" stays a manual step in whatever produces the staging desktop build, with nothing enforcing or recording it. The failure mode is quiet in both directions: a staging build with the var unset behaves exactly like production, and the only way a tester can tell which build they have is to open a migrated project and look for a datatypes/ folder.
Two small things would close it inside this PR:
- add the variable to the staging build workflow (or, at minimum, document it next to
BUILD_DATEinwebpack.app-info.ts's header and in the release notes); - log the effective flag value once at startup, alongside
APP_NAME/BUILD_DATE, so a tester or a bug report can state which build they were on.
4. (Low) Promise.allSettled keeps only the first rejection
const rejected = settled.find((result) => result.status === 'rejected')
if (rejected?.status === 'rejected') throw rejected.reasonIf the disk fills mid-save, one ENOSPC propagates and the other six failures are dropped without ever being logged. Logging every rejected reason before rethrowing the first costs two lines and turns "save failed" into something diagnosable.
5. (Low) project.json is still written non-atomically
The ordering fix removes "the index describes files that were never written", which is the right first fix. It does not remove "the index is half-written" — a writeFile that fails part-way still truncates the one file everything else depends on, and now it is also the last thing written, so it is the write most likely to be interrupted by the user closing the app. Worth a line in the new comment so the ordering rationale isn't read as "project.json is now safe", and a follow-up ticket for write-temp-then-rename.
6. (Nit) One test in the new group depends on the module system rather than the injected global
it('leaves the .dt files alone when the write side is on') uses vi.spyOn(flags, 'isDataTypeFilesEnabled'), while its three siblings set globalThis.DATATYPES_DT_FILES and re-import. Spying on a module namespace object is the one thing in this group whose behaviour depends on the transpilation mode, and this is a mirrored file that has to pass under both Jest and Vitest. Making it consistent with the other three removes a runner-specific failure mode for free.
7. (Nit) The mirror gate compares the source, not the two build configs
feature-flags.ts is byte-mirrored with openplc-web and now reads a bare global that each repo's bundler has to define — but the sync check only compares the shared surface, not configs/webpack/* against web's Vite configs. A missing define in one of web's build entries reads as "off" with no error anywhere. Worth naming, in both this PR and #658, exactly which config file was changed on each side, so the reviewer of either can check the other half.
Test assessment
The three writeProjectFiles tests are the best part of this PR. Each is stated as verified-to-fail-without-the-fix, and 'waits for the slow writes before reporting a failure' covers a property that normally goes untested entirely — that a failed save doesn't hand control back while writes are still in flight. The flag tests correctly cover all three injection states rather than just the two interesting ones, and the project-adapter pair pins both halves of the ungated read (hydrate from .dt; degrade to legacy when the IPC payload carries no array).
Gaps worth closing:
- The deletion loop has no test at all.
makeFiles()usesdeletions: [], so nothing covers the path where finding 1 lives — not the happy path, not a failing unlink, not the ordering relative toproject.json. Given that this PR's third section is about deletions, that is the notable hole. - No test that deletions run after
project.json— the ordering that makes "nothing is ever only-on-disk in a format nothing reads" true. - No test for finding 2's shape: a legacy
project.jsonplus an unparseable.dt. That combination is the one where the load path silently disagrees with the disk.
2 participants
Closes DOPE-542 — the final delivery PR of the DOPE-385 series. Turns on
datatypes/<Name>.dtpersistence for staging.Merging this changes nothing on its own. The flag is now a deploy setting: the staging build has to set
VITE_DATATYPES_DT_FILES=true(web) /DATATYPES_DT_FILES=true(editor). Please don't close the card on merge while the feature is still dark.What's here
1. The flag resolves from a build-time injected global, not a hard-coded constant.
The original plan —
trueondevelopment, reverted tofalseon the promotion branch — inverts the safety. Everydevelopment→mainpromotion would need someone to remember the revert commit, and nothing catches a miss: the mirror gate compares web↔editor, not development↔main, and a constant changing value is not a merge conflict. The default becomes ships to production unless someone remembers.Injecting it keeps the source byte-identical on both branches. Same mechanism the repos already use for
APP_NAME/BUILD_DATE/BUILD_ID, and the AI feature already gates onVITE_AI_ENABLED. Un-injected builds (unit tests) read as off.2. The read is ungated; only the write is still gated.
The flag gated reading
.dtfiles as well as writing them, which made rollback destructive — a flag-off build hydrated from the emptieddata.dataTypesand showed no data types. Now a flag-off build opens a migrated project fine and writes the legacy form back on the next save, so the flag can be turned off and on again.3. A flag-off save deletes the
.dtfiles.Found while walking the desktop round trip. Hydration prefers
.dtfiles whenever they exist, and desktop leaves them on disk (writeProjectFilesonly unlinkspendingDeletions; a plain edit queues nothing). So: migrate → roll back → edit a type → save → flip on again, and the stale.dtsilently wins, reverting the edit. Web never had this since its save replaces the whole file set. The two platforms now agree.Deliberate consequence: rollback is single-use per direction. Data reaches
project.jsonbefore the files are removed, so nothing is ever only-on-disk in a format nothing reads.4.
project.jsonis written last (editor only).writeProjectFilessent every entry in one unorderedPromise.all. A.dtwrite rejecting after the index had landed — already carryingdataTypes: []— lost that type from both places, and the first save of a migrating project is exactly when that bites. Split on theproject-jsoncategory rather than on.dt: the index also declarespous: [], so POUs carry the identical exposure. Three tests, each verified to fail without the fix.Validation
Manual, on desktop, with a purpose-built legacy fixture (enum with an initial value; struct with an
ARRAY [1..10] OF INTfield, a documented field and a user-type field; a 2-D array type):PROGRAM_MD5identical before and after migration, so the generated ST is byte-for-byte the same.datatypes/empty) → flip on (edit survives).Note for anyone repeating this:
program.stis not persisted in the build output — it's dropped as a transient intermediate. ComparePROGRAM_MD5inbuild/<board>/src/defines.h.Automated: editor jest 6107 pass / 0 fail; web vitest 6106 pass with 78 pre-existing failures (DOPE-549, baselined against a clean tree);
tsc, eslint and prettier clean in both; mirror gate 1016 files / 0 diffs.Found while fixturing — pre-existing, not blocking
Matrixcollides with theoscat-basicFB) produces an uncompilable project with a truncated C++ error.[i][j]for multi-dimensional arrays whileArray2Donly exposes.at(i, j).Mirror of https://github.com/Autonomy-Logic/openplc-web/pull/658
Summary by CodeRabbit
New Features
Bug Fixes