fix(export): place generated branchforge_*.rpy under game/ and dedupe define/default symbols (#244)#245
Merged
mikkisguy merged 5 commits intoJun 20, 2026
Conversation
… define/default symbols (#244) The zip export wrote branchforge_variables.rpy, branchforge_stats.rpy and branchforge_definitions.rpy at the archive root, so Ren'Py silently ignored them. Both zip and GitLab exports also produced duplicate `define <tag> = Character(...)` and `default <key> = ...` lines when the source project already declared those symbols, which crashed Ren'Py on launch with `NameError: name 'X' is already defined`. Fix: - New utility `rpy-statements.service.ts` exposes `computeCommonDirectoryPrefix` (moved from gitlab-sync) and a new `extractAndStripRpySymbols` that recognises single- and multi-line `define|default <tag> = Character(...)` plus `default <key> = True/False|<number>` statements. Unknown RHS values (quoted strings, identifier references) are preserved rather than silently dropped, so we never lose user-authored state BranchForge has no place to store. - `export.service.ts` (zip) now places the three generated files under the common top-level directory prefix computed from the project file paths, mirroring the existing GitLab export behaviour. It also defensively strips any `define`/`default` lines that may still be present in `project_files.content` (legacy projects imported before this fix shipped). - `zip-import.service.ts` and `gitlab-sync.service.ts` strip symbols at ingestion, store the pre-strip text in `originalContent`, and promote the extracted characters, variables and stats into their respective tables with `onConflictDoNothing` for idempotent re-imports. The zip symbol-promotion now runs inside the same transaction as the per-file savepoint loop, so file inserts and symbol promotion are atomic. - `characters.service.detectCharacters` reads from `originalContent` (with a fallback to `content`) so the import wizard still surfaces characters whose `define` lines were stripped from the stored file content. Tests: - New unit suite `rpy-statements.service.unit.test.ts` covers prefix computation, single- and multi-line character definitions, `default` keyword parity with `define`, stat vs variable classification, and preservation of unknown defaults. - Extended `export.service.unit.test.ts` asserts that the generated files land under the common prefix and that the defensive export-time strip cleans legacy content. - Extended `zip-import.service.unit.test.ts` asserts the stored file content has the symbols stripped, the DB receives the expected character / variable / stat rows, and re-imports are idempotent. Oracle review verdict: PASS.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR fixes issue ChangesRPY Symbol Stripping and Directory Prefix Fix
Development Process Documentation
Frontend Dependency
Sequence Diagram(s)sequenceDiagram
participant Client
participant Step3 as importZipFile<br/>Step 3
participant extractStrip as extractAndStripRpySymbols
participant Step4 as Step 4<br/>Transaction
participant DB
Client->>Step3: importZipFile(zipBuffer, projectId)
Note over Step3: Preprocess each .rpy entry
loop each .rpy file
Step3->>extractStrip: file.content
extractStrip-->>Step3: cleanedContent, symbols[]
Step3->>Step3: accumulate deduplicated symbols
end
Step3->>Step4: enriched preprocessing results
Note over Step4: Per-file transaction + savepoint
loop each entry
Step4->>DB: upsert project_files(cleanedContent, originalContent)
Step4->>Step4: on success: accumulate symbol dedupes
Step4->>DB: parse and sync STORY labels from originalContent
end
Note over Step4: Step 5 — symbol promotion
Step4->>DB: insert characters onConflictDoNothing
Step4->>DB: insert variables onConflictDoNothing
Step4->>DB: insert stats onConflictDoNothing (minValue=parsed)
DB-->>Client: success
sequenceDiagram
participant Client
participant generateExport
participant extractStrip as extractAndStripRpySymbols
participant computePrefix as computeCommonDirectoryPrefix
participant patchRPY as patchRPYWithVariables
Client->>generateExport: generateExport(projectId)
Note over generateExport: Iterate project files
loop each file
generateExport->>extractStrip: file.content
extractStrip-->>generateExport: strippedContent
generateExport->>generateExport: track safe paths
alt STORY + labels
generateExport->>patchRPY: strippedContent + conditions
patchRPY-->>generateExport: patchedContent
else
generateExport->>generateExport: use strippedContent as-is
end
end
generateExport->>computePrefix: safe project file paths
computePrefix-->>generateExport: fileDirPrefix (e.g. "game/")
generateExport->>generateExport: insert branchforge_*.rpy under fileDirPrefix
generateExport-->>Client: patchedFiles ZIP map
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/src/services/export.service.ts (1)
201-203:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCompute the generated-file prefix from exported safe paths, not raw DB paths.
At Line 202 invalid paths are filtered out, but Line 275 still computes the prefix from all raw
filePaths. A skipped malformed path can alterfileDirPrefixand misplacebranchforge_*.rpy.Use the sanitized paths that actually made it into
patchedFiles(or a collectedsafePathsarray) when callingcomputeCommonDirectoryPrefix.Also applies to: 270-277
🤖 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 `@apps/backend/src/services/export.service.ts` around lines 201 - 203, The computeCommonDirectoryPrefix function is being called with raw filePath values from the database, but invalid paths that fail the sanitizeZipEntryPath check are being filtered out during processing. This causes skipped malformed paths to incorrectly affect the fileDirPrefix calculation and misplace branchforge_*.rpy files. Collect only the sanitized paths that pass the sanitizeZipEntryPath validation check (the safePath values where safePath is truthy) into an array as you iterate through files, then pass this array of valid sanitized paths to computeCommonDirectoryPrefix instead of using the raw file.filePath values.
🤖 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 `@apps/backend/src/services/__tests__/rpy-statements.service.unit.test.ts`:
- Around line 120-161: Add a new test case within the test suite for the
extractAndStripRpySymbols function to verify that character definitions with a
space before the opening parenthesis are handled correctly. Create a test
similar to the existing single-line and multi-line character definition tests,
but use the format "define e = Character (" with a space before the parenthesis,
to ensure the extraction logic properly captures the character tag, name, and
color attributes and removes the definition from the cleaned content regardless
of whitespace placement.
In `@apps/backend/src/services/gitlab-sync.service.ts`:
- Around line 504-524: The file upsert operation using
db.insert(projectFiles).values() is storing stripped content to the database
without being part of a transaction that includes the subsequent symbol
promotion phase. If symbol promotion fails later, the stripped files remain
committed without matching symbol records, creating data inconsistency. Wrap the
db.insert(projectFiles) call and all symbol promotion operations (which occur
around lines 557-638 as noted in the comment) within a single database
transaction boundary so that if symbol promotion fails, the entire transaction
rolls back and neither the stripped files nor incomplete symbols persist to the
database.
In `@apps/backend/src/services/rpy-statements.service.ts`:
- Around line 159-170: The indexOf call searching for "Character(" on line 168
does not account for optional whitespace between "Character" and the opening
parenthesis, even though the regex pattern on line 160 already permits forms
like "Character (". When this spaced syntax is used, indexOf returns -1, causing
an empty body to be passed to parseCharacterBody and losing the character
definition. Update the search logic to handle the optional whitespace that the
regex pattern allows, ensuring consistency between the regex match and the body
extraction in the characterStart block.
In `@apps/backend/src/services/zip-import.service.ts`:
- Around line 263-284: The symbol deduplication logic aggregates characters,
variables, and stats from all preProcessedFiles without verifying that those
files were successfully imported, causing symbols from failed imports to be
promoted to the database. Before populating the charactersByTag, variablesByKey,
and statsByKey maps in the deduplication loop, filter preProcessedFiles to
include only files that completed successfully in Step 4, or alternatively
maintain a set of failed filePaths during the catch phase and exclude them from
the aggregation loop. This ensures only symbols from successfully imported files
are promoted in Step 5.
---
Outside diff comments:
In `@apps/backend/src/services/export.service.ts`:
- Around line 201-203: The computeCommonDirectoryPrefix function is being called
with raw filePath values from the database, but invalid paths that fail the
sanitizeZipEntryPath check are being filtered out during processing. This causes
skipped malformed paths to incorrectly affect the fileDirPrefix calculation and
misplace branchforge_*.rpy files. Collect only the sanitized paths that pass the
sanitizeZipEntryPath validation check (the safePath values where safePath is
truthy) into an array as you iterate through files, then pass this array of
valid sanitized paths to computeCommonDirectoryPrefix instead of using the raw
file.filePath values.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d05833a-c162-4f9f-8a96-7963840fa5c7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
AGENTS.mdapps/backend/src/services/__tests__/export.service.unit.test.tsapps/backend/src/services/__tests__/rpy-statements.service.unit.test.tsapps/backend/src/services/__tests__/zip-import.service.unit.test.tsapps/backend/src/services/characters.service.tsapps/backend/src/services/export.service.tsapps/backend/src/services/gitlab-sync.service.tsapps/backend/src/services/rpy-statements.service.tsapps/backend/src/services/zip-import.service.tsapps/frontend/package.json
Follow-up fixes for issue #244 from code review: - `rpy-statements.service.ts`: the body slice in the character extractor was using `line.indexOf("Character(")` which silently dropped definitions like `define e = Character ("Eileen", ...)` (with a space before the open paren). The regex already accepted the spaced form; the body search now uses `line.search(/Character\s*\(/)` to match. Added a regression test that covers both single- and multi-line spaced forms. - `export.service.ts`: the directory-prefix calculation now uses only the sanitized paths we actually emit. Before, a stray path like `evil/../escape.rpy` (which `sanitizeZipEntryPath` rejects) would have dragged the prefix off `game/` to "" and put `branchforge_*.rpy` at the archive root. New regression test verifies the unsafe path is dropped and the prefix is correct. - `zip-import.service.ts`: cross-file symbol aggregation is now driven by the per-file savepoint loop, not by the raw `preProcessedFiles` list. Files whose savepoint rolls back no longer contribute characters / variables / stats to the database, so a partial-failure import leaves no orphan symbols. Added a small `accumulateSymbols` helper to keep the call sites readable. - `gitlab-sync.service.ts`: the file upsert (Phase 1) and the symbol promotion (Phase 1.5) are now wrapped in a single `db.transaction` so they commit or roll back together. The cross-file aggregation maps are populated inside the transaction, so a mid-import failure also discards any partial symbol accumulation. Tests: `pnpm test:unit` — 747 backend + 815 frontend pass. Oracle verdict (round 1): PASS.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
1 participant
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
Fixes #244.
Two bugs in the Ren'Py project export/import flow:
Bug 1 — zip export places
branchforge_*.rpyat archive rootbranchforge_variables.rpy,branchforge_stats.rpyandbranchforge_definitions.rpywere written with bare keys, so the zipcontained them at the archive root instead of inside
game/. Ren'Pysilently ignored them and the resulting project had no variables, stats
or character definitions.
Bug 2 — exported files duplicate
define/defaultsymbolsBoth zip and GitLab exports could produce files containing
define <tag> = Character(...)anddefault <key> = ...lines thatalready existed in the source project's files. Ren'Py then failed to
launch with
NameError: name 'X' is already defined.Fix
New utility
apps/backend/src/services/rpy-statements.service.ts:computeCommonDirectoryPrefix(moved from gitlab-sync)extractAndStripRpySymbolsrecognises single- and multi-linedefine|default <tag> = Character(...)plusdefault <key> = True|False|<number>statements. Unknown RHSvalues are preserved rather than dropped.
export.service.ts(zip): places the three generated files underthe common top-level directory prefix computed from project file
paths, mirroring the GitLab export. Defensively strips any
define/defaultlines that may still be present in legacyproject_files.contentrows so existing projects are not broken.zip-import.service.tsandgitlab-sync.service.ts: strip symbolsat ingestion, store the pre-strip text in
originalContent, andpromote the extracted characters / variables / stats into their
tables with
onConflictDoNothingfor idempotent re-imports. Thezip symbol-promotion now runs inside the same transaction as the
per-file savepoint loop for atomicity.
characters.service.detectCharacters: reads fromoriginalContent ?? contentso the import wizard still surfacesstripped characters.
Verification
pnpm typecheck— passespnpm lint— passespnpm test:unit— 745 backend + 815 frontend tests pass(30 new test cases across rpy-statements, export, and zip-import)
pnpm format— clean(after addressing all must-fix and should-fix findings)
Summary by CodeRabbit
branchforge_*files within the correct archive subdirectory (not always at the root), and GitLab sync follows the same cleaned-content flow.