fix: address review findings (branch creation, hooks, GitHub resilience)#6
Merged
Conversation
…unset When the target branch did not exist and no default branch was configured, the base branch collapsed to the target branch itself. The GitHub client then tried to read a ref for the not-yet-created branch and failed with "Cannot read base branch ref", so first-time pushes to a new branch always broke unless a default branch was set manually. Resolve the base branch to undefined when no default is configured so the client falls back to the repository's actual default branch, matching the documented automatic branch creation behaviour. Add a pure resolveBaseBranch helper (unit tested) plus the first regression tests for the previously untested GitHub client (branch creation + skip).
useMemo was invoked after three conditional early returns, so the hook count changed between renders depending on export/remote state. This violates the Rules of Hooks and can corrupt component state or crash. Declare the diff memo immediately after useEffect, before any return, so all hooks run unconditionally and in a stable order.
buildDtcgJson hardcoded pluginVersion 0.2.0 while the rest of the app uses the version from src/constants (0.3.1). DTCG exports and the bundle summary therefore reported a stale version, diverging from the Figma-native export. Import PLUGIN_VERSION from constants so all export paths report the same version, and assert the value in the DTCG metadata test.
ghFetch returned the Response for every status, so withRetry (which only retries thrown errors) never retried HTTP error responses. The documented exponential-backoff retry on transient failures and rate limiting was therefore effectively dead. Throw on transient statuses (429 and 5xx) inside the retried function so they are retried with backoff, while still returning 4xx responses (401/403/404/409/422) untouched for callers to inspect.
The commitFiles path (Git Trees API) had no conflict handling: a concurrent update to the branch between reading HEAD and updating the ref threw a bare "Failed to update branch reference" with no recovery, despite the README advertising automatic 409 conflict resolution. The only retry logic lived in upsertFile, which is unused. On a 409/422 ref-update conflict, re-read the branch HEAD and rebuild the commit on top of it once before failing, reusing the content-addressed blobs. updateBranchRef now includes the HTTP status so the conflict is detectable.
upsertFile, UpsertResult and GitHubUpsertOptions were never used: the live commit path is the Trees-API commitFiles. Removing them drops ~120 lines of dead code, including the duplicate (and now redundant) 409 handling. Keep the sandbox-safe base64 codec, exporting toBase64 (previously only consumed by upsertFile) alongside fromBase64, and add round-trip tests covering ASCII, multi-byte and surrogate-pair input plus chunked input.
The COPY_TO_CLIPBOARD message was declared in the protocol and handled as a no-op in the plugin, but never sent: JsonViewer copies via execCommand and a NOTIFY message. Remove the dead type and handler.
scripts/build.mjs was not referenced by any npm script or workflow and was broken: it read a non-existent src/ui/styles.css and emitted dist/ui.html, which the manifest does not use (the real build uses esbuild + Vite). Remove it to avoid confusion.
The auto-export effect only ran when there was no export result, so changing the format, document strategy, filename, or folder after an initial export left a stale bundle in place. The preview and diff showed old data, and committing pushed the previously exported format/paths. Track an export signature of those settings and regenerate the export (with a short debounce to collapse rapid edits) whenever it changes.
Each commit read every file twice: once in the plugin via getRemoteFileHashes (for the skip/diff decision) and again in commitFiles via filterFilesNeedingUpdate. commitFiles now accepts optional knownContentHashes and skips the redundant contents request when a hash is already known. The plugin passes the hashes it fetched only when they came from the commit target branch, so branch-creation paths still verify against the freshly created branch.
lastHash and lastHashes were written to plugin data after every commit but never read for any decision: change detection relies on live remote hashes. Remove the closed-loop persistence, the getLastHashMap helper, the now-unused buildRepoPath import, and the unused settings fields.
The 5xx check used errorMsg.includes('50'), which could match unrelated
numbers. Match whole status codes (5xx and 429) with a word-boundary regex
and add a 5xx retry test.
- Remove the hardcoded 94.51% coverage badge: it was a static, unverifiable value and coverage is not published anywhere (CI status badge remains). - Bump the DTCG and Figma-native example pluginVersion to 0.3.1 to match the actual exported value. - Drop the "preserve last known hash state" dry-run bullet; that state no longer exists. - Fix a code comment that referred to the UI as React (it is Preact).
CI previously mixed Node 24 and 20 across jobs. Pin every job to the current Active LTS line, Node 24 (Krypton, EOL 2028-04-30), and declare it via an engines field. Update the README Node badge and requirements to match so the documented, declared, and tested versions are consistent.
findyourexit
force-pushed
the
fix/code-review-findings
branch
from
June 9, 2026 11:41
de0861d to
829032c
Compare
findyourexit
approved these changes
Jun 9, 2026
tlarcher-gpsw
added
bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses issues found during a deep review of the plugin. Fixes two critical bugs (broken first-time branch creation and a Rules-of-Hooks violation), reconciles the GitHub client's resilience with what the README advertises (retry + conflict handling), and removes dead code / corrects stale docs. Delivered as focused, atomic Conventional Commits.
All changes are verified:
type-check,lint,format:check, 229 tests, andbuildpass. The previously untested GitHub client now has coverage (~83%).Changes
Critical fixes
Cannot read base branch refbecause the base branch collapsed to the (non-existent) target branch. Resolves to the repository default instead. AddsresolveBaseBranch(unit-tested) and the first regression tests for the GitHub client.useMemoran after conditional early returns, changing hook order between renders (Rules of Hooks violation).0.2.0; now uses the shared version (0.3.1).GitHub resilience (matching documented behaviour)
ghFetchreturned the response for every status, sowithRetrynever retried HTTP errors. Transient statuses now trigger backoff; 4xx are still returned for callers to inspect.Correctness / UX
includes('50')heuristic.Cleanup
scripts/build.mjsreferenced a non-existent file and was unreferenced.Docs / CI
enginesfield, and updated the README Node badge/requirements to match.Testing
npm run type-check— passnpm run lint/npm run format:check— passnpm test— 229 tests pass (addedgithubClient,branchPlan, andbase64suites;githubClient.tscoverage 0% → ~83%)npm run build— pass;dist/manifest references verifiedNotes
manifest.jsonidplaceholder was intentionally left unchanged: it is valid for development import and Figma assigns a real id on publish.