Handle special character names on import (#138) + fix wizard-not-showing#249
Conversation
…nd warnings - Added `nameType` classification to `DetectedCharacter` to differentiate between literal, variable, interpolated, tagged, empty, none, and unknown names. - Implemented UI badges and helper text in the `CharacterImportWizard` to provide user guidance based on the `nameType`. - Introduced `stripRenpyTextTags` function to handle Ren'Py text tags, ensuring proper display name extraction. - Created unit tests for character parser service to validate name classification and display name derivation. - Added tests for `CharacterImportWizard` to ensure correct rendering of badges and helper text based on character name types. - Implemented tests for `stripRenpyTextTags` to verify tag stripping functionality and edge cases.
PR #245 (#244) auto-promotes extracted characters into the `characters` table during GitLab/ZIP import, so the wizard's `existingTags` filter (`newCharacters = characters.filter(c => !existingTagsSet.has(c.tag))`) returned an empty list on fresh imports and the wizard never opened. - Show the wizard whenever `detectionResult.characters.length > 0` in all four import flows (ZipImportProject, ZipImportFiles, GitLabImport, GitLabSync). - Pass real `existingTags` to the wizard for the secondary re-sync flows (file-merge, GitLab sync) so the 'Already imported' badge is meaningful on subsequent imports. - Pass `existingTags={[]}` from the two fresh-project dialogs (ZipImportProject, GitLabImport) so the badge doesn't fire on the user's first review of an import. - Add an 'Already imported' badge in the wizard when a detected tag is in `existingTags`. - Add a regression test using real-world RPY data (game/variables.rpy from a live project) covering `define x = Character('[var]')` and string-variable disambiguation.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 11 minutes and 3 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a ChangesCharacterNameType classification and import wizard warnings
Sequence Diagram(s)sequenceDiagram
participant ImportDialog as GitLabImportDialog / ZipImportDialog
participant detectCharacters as charactersApi.detectCharacters
participant CharacterImportWizard
participant getNameTypeBadge
participant importCharacters as charactersApi.importCharacters
ImportDialog->>detectCharacters: detectCharacters(projectId)
detectCharacters-->>ImportDialog: DetectedCharacter[] (with nameType)
note over ImportDialog: opens wizard if characters.length > 0 (no new-only filter)
ImportDialog->>CharacterImportWizard: characters, existingTags
CharacterImportWizard->>getNameTypeBadge: char.nameType
getNameTypeBadge-->>CharacterImportWizard: { label, helperText } | null
note over CharacterImportWizard: renders badge + AlertTriangle when non-literal
CharacterImportWizard->>importCharacters: { projectId, characters: [{ name, displayName, nameType }] }
importCharacters-->>CharacterImportWizard: import result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
React Doctor found 6 issues in 5 files · 6 warnings · score 92 / 100 (Great) · vs 6 warnings
Reviewed by React Doctor for commit |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/character-parser.service.ts`:
- Around line 25-31: The comment for the `name` field in the
character-parser.service.ts file states that export uses the `name` field and
not `displayName`, but this contradicts what the shared `DetectedCharacter`
contract actually specifies. Update the comment for the `name` field to
accurately reflect which field the export functionality actually uses according
to the shared `DetectedCharacter` contract, ensuring the documentation aligns
with the actual implementation so callers have correct guidance.
- Around line 434-437: The regex pattern in the quotedNameMatch variable uses
`([^"]+)` which requires at least one character between quotes, causing empty
quoted strings like "" to not match and fall through to be classified as none.
Fix this by changing the quantifier from `+` to `*` in the regex pattern so it
matches zero or more characters, allowing empty quoted names to be properly
captured in multi-line definitions.
- Around line 343-368: The identifier fallback regex in the variableNameMatch
check is incorrectly capturing `None` as a regular identifier when it should be
treated as a special case. Before the existing identifier matching logic, add a
check to detect if the rest of the line starts with `None` and handle it
separately by setting the appropriate nameForm value to "none" instead of
allowing it to fall through to the identifier case. This ensures that
`Character(None,` on a single line is properly classified as a none value rather
than as an identifier.
- Around line 98-100: In the character-parser.service.ts file, separate the
imports of CharacterNameType and stripRenpyTextTags so that CharacterNameType
uses a type-only import statement while stripRenpyTextTags remains as a regular
import. Since CharacterNameType is only used in type positions (as the interface
property type in the service and the function return type), it should be
imported with the `import type` syntax to comply with TypeScript guidelines.
Remove the redundant type-only export statement for CharacterNameType since it
will be properly declared as a type-only import.
In `@apps/frontend/src/components/__tests__/CharacterImportWizard.test.tsx`:
- Line 14: The import statement for CharacterImportWizard uses a relative path
without a .js extension, which violates the repository's frontend import
conventions. Replace the relative import path with the `@/` alias and add the .js
extension to the import specifier. Change the CharacterImportWizard import to
use `@/components/CharacterImportWizard.js` format instead of the current relative
import.
🪄 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: 0c406366-c237-4f1e-b727-67ce697a6fab
📒 Files selected for processing (13)
apps/backend/src/lib/validation.tsapps/backend/src/services/__tests__/character-parser-real-world.unit.test.tsapps/backend/src/services/__tests__/character-parser.service.unit.test.tsapps/backend/src/services/character-parser.service.tsapps/backend/src/services/gitlab-sync.service.tsapps/frontend/src/components/CharacterImportWizard.tsxapps/frontend/src/components/__tests__/CharacterImportWizard.test.tsxapps/frontend/src/components/ide-shared/GitLabImportDialog.tsxapps/frontend/src/components/ide-shared/ZipImportFilesDialog.tsxapps/frontend/src/components/ide-shared/ZipImportProjectDialog.tsxapps/frontend/src/components/script-mode/GitLabSyncDialog.tsxpackages/shared/src/index.tspackages/shared/src/strip-renpy-tags.unit.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| detectedCharacters, | ||
| conflicts, | ||
| excludedTags, | ||
| existingTags = [], |
There was a problem hiding this comment.
React Doctor · react-doctor/rerender-memo-with-default-value (warning)
This keeps redrawing children that compare props because default prop value [] makes a brand new array every render, so move it to a constant at the top of the file
Fix → Move it to the top of the file: const EMPTY_ITEMS: Item[] = [], then use that as the default value
1 participant
Summary
Closes #138. Adds
CharacterNameTypeclassification to detected characters so the import wizard can warn the user about variable references, Ren'Py interpolation, formatting tags, empty names, and the "???" placeholder. Also fixes a wizard-not-showing regression caused by PR #245 (#244).Issue #138 — Special character names
When importing characters from Ren'Py projects, some character definitions use patterns the parser used to pass through raw to the UI:
Character(boss_name, ...)) — unresolvable at import timeCharacter("[e_name]", ...)) — runtime-dependentCharacter("{color=...}Stranger{/color}", ...)) — display-name noiseNone,"","???") — varying semanticsThe fix adds a 7-state
CharacterNameTypeclassifier plus astripRenpyTextTags()utility inpackages/shared. The parser emits a typed record per character; the wizard renders warning badges and contextual helper text per type, but the user can still override the display name on import. The rawnameis preserved verbatim for round-tripping.Wizard-not-showing fix (follow-up to PR #245)
PR #245 (#244) auto-promotes extracted characters into the
characterstable during import. The pre-existing wizard trigger (newCharacters = characters.filter(c => !existingTagsSet.has(c.tag))) therefore returned an empty list on fresh imports and the wizard never opened.The fix:
detectionResult.characters.length > 0in all four import flows.ZipImportFilesDialog,GitLabSyncDialog), pass realexistingTagsso the new "Already imported" badge is meaningful on subsequent imports.ZipImportProjectDialog,GitLabImportDialog), passexistingTags={[]}so the badge doesn't fire on the user's first review (PR fix(export): place generated branchforge_*.rpy under game/ and dedupe define/default symbols (#244) #245's auto-promote just put them there — the user hasn't reviewed yet).importCharactersendpoint already does upsert, so re-confirming unchanged rows is a safe no-op.What changed
packages/shared/src/index.ts— newCharacterNameTypetype,stripRenpyTextTags()utility,nameTypefield onDetectedCharacter.apps/backend/src/services/character-parser.service.ts— refactoredparseFileto tracknameForm(quoted/bracketed/identifier) and classify viaclassifyName(). Bracketed form preserves brackets innamefor round-trip fidelity. Multi-lineNonefollowed by quoted options no longer overwrites the name.apps/backend/src/lib/validation.ts— addednameTypetodetectedCharacterSchema.apps/backend/src/services/gitlab-sync.service.ts—nameType: "literal"default for the legacy rpy-parser path (documented as a known gap).apps/frontend/src/components/CharacterImportWizard.tsx— warning badge + helper text pernameType, "(unnamed)" placeholder for empty names, "Already imported" badge, "(unnamed)" rendering in the special-character group.apps/frontend/src/components/ide-shared/{ZipImportProject,ZipImportFiles,GitLabImport,GitLabSync}Dialog.tsx— wizard trigger updated,existingTagsthreaded through.Verification
pnpm typecheck— passespnpm lint— passespnpm test:unit— 1659/1659 pass (40 new tests added)Oracle review verdict
PASS (after addressing all must-fix and should-fix findings from the initial review). One should-fix item deferred with justification: the
CharacterConflictshared type does not carrynameType, so the wizard's "Conflicts" group does not show name-type badges. The conflict path is rare (requires manual DB edits conflicting with parser output) and the conflict section already tells the user to "Review in character management after import" — a follow-up PR can addnameTypetoCharacterConflictif needed.Summary by CodeRabbit