Skip to content

fix(project): only commit files that were actually modified - #251

Merged
robingenz merged 3 commits into
mainfrom
fix/vfs-modified-tracking
Aug 15, 2026
Merged

fix(project): only commit files that were actually modified#251
robingenz merged 3 commits into
mainfrom
fix/vfs-modified-tracking

Conversation

@robingenz

Copy link
Copy Markdown
Collaborator

Problem

AndroidManifest.xml and project.pbxproj are opened into the VFS whenever the
corresponding platform directory exists, regardless of what the config asks for.
VFS.commitAll() then wrote back every open file with no modification check —
VFSRef.modified / isModified() existed but were never read anywhere in the repo.

Running a project.json-only config against a project with ios/ and android/
therefore:

  • reformatted AndroidManifest.xml (one-time normalization churn: attributes collapsed
    onto one line, indentation rewritten),
  • rewrote project.pbxproj byte-identically but bumped its mtime on every run,
  • and listed both as updated in the CLI output.

Fix

Filtering commitAll() on isModified() alone would have been a much worse bug: only
XmlFile (and a few IosProject plist writes) called vfs.set(). PlistFile,
JsonFile, PropertiesFile, StringsFile, XCConfigFile, GradleFile and the pbxproj
all mutate their in-memory document in place and never flagged the ref — filtering first
would have silently stopped committing gradle/properties/strings/xcconfig edits.

So, in order:

  1. Every file wrapper now flags its VFS ref on mutation. Added VFSRef.markModified()
    / VFS.markModified(filename) and called it from every mutating path:
    • PlistFile.setDocument/setFromXml/set/merge (update goes through setDocument)
    • JsonFile.set/merge
    • PropertiesFile.updateProperties
    • StringsFile.set
    • XCConfigFile.set
    • GradleFile — all three this.source = … sinks now go through a private setSource()
    • XmlFile.mergeFragment — the one XmlFile mutator that was missing the flag
    • IosProject — a private markPbxModified() on every method that writes build
      settings or pbxproj state (setBundleId, setProductName, setBuild,
      incrementBuild, setVersion, setBuildProperty, addFramework, addFile)
    • AndroidProject.setAppName poked the manifest DOM directly, bypassing XmlFile;
      it now goes through XmlFile.setAttrs
  2. Then VFS.commitAll(), VFS.diffAll() and checkModifiedFiles() in the CLI filter
    on the new VFS.modifiedFiles(), so untouched files are neither listed as updated
    nor rewritten.

Verified locally

  • packages/project: 126 tests / 20 files passed (Java 21 present, gradle tests real)
  • packages/configure: 71 tests / 23 files passed
  • New tests:
    • run.test.ts — the Project config files touched/updated even if not in yaml #110 repro: a project-only config against the ios-and-android
      fixture leaves AndroidManifest.xml and project.pbxproj byte-identical and
      un-touched (mtime), and vfs.modifiedFiles() contains only the two project files.
      Confirmed it fails without the fix, with exactly the reported reformatting diff.
    • A "writes to disk on commit" guard for each file type — json, plist, strings,
      xcconfig, gradle, pbxproj build settings (properties and xml already had one).
      Confirmed all seven fail when markModified() is stubbed out, i.e. they really do
      guard against the trap above.
    • vfs.test.tscommitAll() commits only modified refs.
    • project.android.test.tssetAppName writes the manifest when there is no
      android:label (the direct-DOM path). Confirmed it fails if that path is reverted.

Closes #110

🤖 Generated with Claude Code

Copilot AI lite review requested due to automatic review settings August 15, 2026 06:41
@vercel

vercel Bot commented Aug 15, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
trapeze Ready Ready Preview Aug 15, 2026 8:31am

Request Review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates Trapeze’s VFS and file wrappers so that only files actually mutated during a run are considered “modified” and therefore are the only ones diffed, listed as updated, and written back to disk. This directly addresses the churn/touching of AndroidManifest.xml and project.pbxproj reported in #110 when a config doesn’t target those platforms.

Changes:

  • Add explicit “modified” tracking in the VFS (markModified(), modifiedFiles()) and filter commitAll() / diffAll() to only act on modified refs.
  • Ensure every mutating code path in file wrappers and platform projects flags its VFS entry as modified (including in-place mutations like pbxproj and Gradle source updates).
  • Add/extend tests to prevent regressions (commit/diff filtering, pbxproj/manifest writes, and “untouched files remain untouched” integration coverage in run).

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/project/src/vfs.ts Adds markModified()/modifiedFiles() and filters commit/diff operations to modified refs only.
packages/project/src/xml.ts Ensures mergeFragment() marks the XML file as modified so it’s committed when changed.
packages/project/src/plist.ts Flags plist VFS refs on in-place document mutations so commits aren’t skipped.
packages/project/src/json.ts Flags JSON VFS refs on in-place document mutations so commits aren’t skipped.
packages/project/src/properties.ts Flags properties VFS refs when updating merged properties.
packages/project/src/strings.ts Flags strings VFS refs when values are set in-place.
packages/project/src/xcconfig.ts Flags xcconfig VFS refs when content is updated.
packages/project/src/android/gradle-file.ts Funnels Gradle source mutations through setSource() to consistently mark modified.
packages/project/src/android/project.ts Routes manifest label updates through XmlFile.setAttrs() to ensure modifications are tracked/committed.
packages/project/src/ios/project.ts Tracks the pbxproj VFS ref and marks it modified on in-place pbx project mutations.
packages/project/test/vfs.test.ts Adds unit test asserting commitAll() only commits modified files.
packages/project/test/project.android.test.ts Adds test ensuring manifest label is written when android:label was missing.
packages/configure/src/tasks/run.ts Switches CLI “updated” listing to only show modified files.
packages/configure/test/tasks/run.test.ts Adds integration repro test to ensure untouched platform files remain byte-identical and not re-touched.
packages/configure/test/ops/android.json.test.ts Adds guard test ensuring JSON changes are actually written on commit.
packages/configure/test/ops/android.gradle.test.ts Adds guard test ensuring Gradle changes are actually written on commit.
packages/configure/test/ops/ios.plist.test.ts Adds guard test ensuring plist changes are actually written on commit.
packages/configure/test/ops/ios.strings.test.ts Adds guard test ensuring strings changes are actually written on commit.
packages/configure/test/ops/ios.xcconfig.test.ts Adds guard test ensuring xcconfig changes are actually written on commit.
packages/configure/test/ops/ios.buildVersion.test.ts Adds guard test ensuring pbxproj build setting changes are actually written on commit.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/configure/src/tasks/run.ts
robingenz and others added 3 commits August 15, 2026 10:25
AndroidManifest.xml and project.pbxproj are opened into the VFS whenever
the platform directory exists, and commitAll() wrote back every open file
without checking whether anything had changed - the modified flag existed
but was never read anywhere. A project.json-only config therefore
reformatted the manifest and re-touched the pbxproj on every run.

Every file wrapper now flags its VFS ref on mutation (plist, json,
properties, strings, xcconfig, gradle, XmlFile.mergeFragment and the
pbxproj writes in IosProject), so commitAll(), diffAll() and the CLI's
"updated" list can filter on isModified() without silently dropping edits.

Refs #110

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
checkModifiedFiles() now lists vfs.modifiedFiles(), so the list can be
empty when every operation was skipped or was a no-op. The interactive
"Apply changes?" confirmation was still shown in that case, with no
preceding "updated" lines to explain what it would apply.

Log an informational line instead and return early, leaving -y, --dry-run
and --diff behavior unchanged for runs that did modify something.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An earlier test pushes --no-commit onto process.argv without cleanup; with
the new commander semantics that made this test's commit silently skip.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@robingenz
robingenz force-pushed the fix/vfs-modified-tracking branch from 0a16b15 to 16c010c Compare August 15, 2026 08:31
@robingenz
robingenz merged commit 2d091d4 into main Aug 15, 2026
3 checks passed
@github-actions github-actions Bot mentioned this pull request Aug 15, 2026
robingenz added a commit that referenced this pull request Aug 15, 2026
… hygiene

Test files were never type-checked: both packages' tsconfig only include
`src`, and vitest strips types with esbuild without checking them. Add a
`tsconfig.test.json` and a `typecheck` script per package, wire them into
turbo and CI, and fix everything the check surfaced — a stale `@types/fs-extra`
`rm()` overload in 17 places and a bare URL at project.ios.test.ts:526 that
JavaScript parsed as a label on the `describe` below it.

Harden test isolation: `clearMocks`/`restoreMocks` and a shared `test/setup.ts`
that restores `process.argv` and `process.env` after every test. That kills the
`--no-commit` leak from run.test.ts, which had made the regression test for
"don't prompt when nothing was modified" pass for the wrong reason — it asserted
on `ctx.args.noCommit`, a key `src` never reads, so the leaked `commit === false`
returned before the prompt could ever be reached.

Replace the three ad-hoc copy-to-tempy idioms with a shared `useFixture()`
helper that registers its own cleanup, so a throwing assertion can no longer
leak a temp directory, and delete the orphaned `ios-only`, `web-only` and
`splash.png` fixtures.

Make `packages/common/test/fixtures/**` a turbo global dependency: it appeared
in no task's input set, so a local `npm test` could serve a cached PASS from
before a fixture edit.

Add two CI jobs: the Gradle parser tests on Windows (the only platform-specific
code in the repo builds a Java classpath by hand and takes a `win32` branch that
has never been executed in CI), and a pack -> install -> run smoke test of the
published tarballs, which is the only thing that covers an incomplete `files`
field and the operation modules loading from `dist` instead of `src`.

Docs: `vfs.all()` no longer previews what will be committed after #251, the
`ios.frameworks` and `android.properties` operations were undocumented, and
CLAUDE.md still described the changesets release flow.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
robingenz added a commit that referenced this pull request Aug 15, 2026
… hygiene (#257)

* fix(ci): typecheck tests, harden CI, add tarball smoke test, fix test hygiene

Test files were never type-checked: both packages' tsconfig only include
`src`, and vitest strips types with esbuild without checking them. Add a
`tsconfig.test.json` and a `typecheck` script per package, wire them into
turbo and CI, and fix everything the check surfaced — a stale `@types/fs-extra`
`rm()` overload in 17 places and a bare URL at project.ios.test.ts:526 that
JavaScript parsed as a label on the `describe` below it.

Harden test isolation: `clearMocks`/`restoreMocks` and a shared `test/setup.ts`
that restores `process.argv` and `process.env` after every test. That kills the
`--no-commit` leak from run.test.ts, which had made the regression test for
"don't prompt when nothing was modified" pass for the wrong reason — it asserted
on `ctx.args.noCommit`, a key `src` never reads, so the leaked `commit === false`
returned before the prompt could ever be reached.

Replace the three ad-hoc copy-to-tempy idioms with a shared `useFixture()`
helper that registers its own cleanup, so a throwing assertion can no longer
leak a temp directory, and delete the orphaned `ios-only`, `web-only` and
`splash.png` fixtures.

Make `packages/common/test/fixtures/**` a turbo global dependency: it appeared
in no task's input set, so a local `npm test` could serve a cached PASS from
before a fixture edit.

Add two CI jobs: the Gradle parser tests on Windows (the only platform-specific
code in the repo builds a Java classpath by hand and takes a `win32` branch that
has never been executed in CI), and a pack -> install -> run smoke test of the
published tarballs, which is the only thing that covers an incomplete `files`
field and the operation modules loading from `dist` instead of `src`.

Docs: `vfs.all()` no longer previews what will be committed after #251, the
`ios.frameworks` and `android.properties` operations were undocumented, and
CLAUDE.md still described the changesets release flow.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* test(configure): correct copy-pasted describe label in ios.xcconfig tests

Addresses review feedback on #257.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(ci): drop npm cache from workflow jobs

Speed-only addition that wasn't part of the agreed scope.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* test: fix rebase fallout in the typecheck and fixture layer

The new typecheck gate flagged the rm() overload in test files added by
later-merged PRs — rm now comes from fs/promises in those files. The
ios-only fixture is restored: it was orphaned when this branch deleted
it, but the platform-matrix test merged since then uses it. The two op
test files added on main adopt the useFixture convention.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Project config files touched/updated even if not in yaml

2 participants