feat(visual-systems): add per-project visual system configuration#247
Merged
Conversation
Implements GitHub issue #25. Visual system config controls how generated Ren'Py visual filenames are produced (template tokens, group prefixes, padding, shared jump prefix, placeholder base URL). Backend - GET /api/projects/:projectId/visual-system returns the config, auto-creating a default row on first read. - PUT /api/projects/:projectId/visual-system PATCHes a subset of fields. Sentinels ("", {}) clear optional fields back to NULL. - New Zod schema 'visualSystemConfigSchema' with http/https restriction on placeholderBaseUrl and clearable semantics for defaultGroupType, placeholderBaseUrl, and groupPrefixes. - New 'VisualSystemsService' with default-on-first-read and PATCH upsert. Enforces requireProjectOwnership. - Integration tests: 18 cases covering auth, validation, clearing, groupPrefixes JSONB round-trip, default creation. Frontend - New API client, TanStack Query hook (optimistic update with rollback), and VisualSystemDialog with a live preview that feeds the current form into generateVisualName(). - New query keys under visualSystemKeys. Closes #25
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements end-to-end visual system configuration for the BranchForge project: a Zod validation schema with empty-string clearing semantics, a ChangesVisual System Configuration and Settings UI Consolidation
Sequence Diagram(s)sequenceDiagram
participant User
participant ProjectSettingsDialog as ProjectSettingsDialog<br/>(Visual System tab)
participant useVisualSystem
participant visualSystemsApi
participant FastifyRoute as Fastify Route<br/>(PUT /visual-system)
participant VisualSystemsService
participant DB as visualSystems table
User->>ProjectSettingsDialog: opens dialog, Visual System tab
ProjectSettingsDialog->>useVisualSystem: config, isLoading, updateConfig
useVisualSystem->>visualSystemsApi: getVisualSystemConfig(projectId)
visualSystemsApi->>FastifyRoute: GET /projects/:projectId/visual-system
FastifyRoute->>VisualSystemsService: getVisualSystemConfig(projectId, userId)
VisualSystemsService->>DB: insert defaults onConflictDoNothing
VisualSystemsService->>DB: select row
DB-->>VisualSystemsService: row or null
VisualSystemsService-->>FastifyRoute: VisualSystemConfigResult
FastifyRoute-->>visualSystemsApi: 200 VisualSystemConfig
visualSystemsApi-->>useVisualSystem: config
useVisualSystem-->>ProjectSettingsDialog: hydrated config
User->>ProjectSettingsDialog: edits fields and clicks Save
ProjectSettingsDialog->>ProjectSettingsDialog: trim fields, parseGroupPrefixes
ProjectSettingsDialog->>useVisualSystem: updateConfig(patch)
useVisualSystem->>useVisualSystem: optimistic cache merge
useVisualSystem->>visualSystemsApi: updateVisualSystemConfig(projectId, body)
visualSystemsApi->>FastifyRoute: PUT /projects/:projectId/visual-system
FastifyRoute->>VisualSystemsService: updateVisualSystemConfig(projectId, userId, input)
VisualSystemsService->>VisualSystemsService: normalize clearable fields to null
VisualSystemsService->>DB: upsert onConflictDoUpdate
DB-->>VisualSystemsService: updated row
VisualSystemsService-->>FastifyRoute: VisualSystemConfigResult
FastifyRoute-->>visualSystemsApi: 200 VisualSystemConfig
visualSystemsApi-->>useVisualSystem: updated config
useVisualSystem->>useVisualSystem: show success toast, invalidate cache
useVisualSystem-->>ProjectSettingsDialog: updated config
ProjectSettingsDialog->>User: closes dialog
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Contributor
|
React Doctor found 1 issue in 1 file · 1 warning · score 95 / 100 (Great) · vs 1 warning
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/lib/validation.ts`:
- Around line 847-851: The jumpPrefixShared field validation requires a minimum
length of 1 character but the default configuration returns an empty string for
this field, creating a validation conflict that breaks config round-tripping.
Fix the jumpPrefixShared validation schema to allow empty strings by removing
the min(1) constraint, changing it to .optional().default(''), or updating the
default configuration to provide a non-empty value that satisfies the validation
rules.
In `@apps/backend/src/routes/__tests__/visual-systems.routes.integration.test.ts`:
- Around line 314-606: Add a regression test within the "PUT
/projects/:projectId/visual-system" describe block that explicitly verifies the
PUT endpoint accepts an empty string for jumpPrefixShared and properly handles
it. The test should follow the pattern of existing tests like "treats empty
placeholderBaseUrl as null" and "clears defaultGroupType when set to an empty
string" by first seeding a visual system row with a non-empty jumpPrefixShared
value, then sending a PUT request with jumpPrefixShared set to an empty string,
and finally asserting both the response and the database state to confirm the
value is properly stored and round-tripped.
In `@apps/frontend/src/components/VisualSystemDialog.tsx`:
- Around line 126-136: The validation loop iterating through groupPrefixes
entries checks if keys and values are empty strings using length checks, but
doesn't account for whitespace-only strings which pass locally but fail
server-side validation after trimming. In the validation block starting with the
for loop over Object.entries, trim both the key and value before checking their
lengths. Replace the condition checking k.length === 0 with k.trim().length ===
0, and the condition checking v.length === 0 with v.trim().length === 0, to
ensure whitespace-only entries are caught during client-side validation and
match the server-side behavior.
- Around line 164-170: The URL protocol validation in the placeholderBaseUrl
validation block uses startsWith("http") which is too permissive and allows
invalid protocols that would fail backend validation. Update the condition
checking url.protocol to explicitly validate that the protocol is exactly
"http:" or "https:" rather than just checking if it starts with "http", ensuring
client-side validation matches the backend rules exactly.
In `@apps/frontend/src/lib/api/visual-systems.ts`:
- Line 9: The import statement for request in the visual-systems.ts file uses a
relative path `./client` instead of the frontend alias convention. Change the
import statement to use the `@/lib/api/client` path instead of the relative
`./client` path to maintain consistency with the frontend import conventions
established in this codebase.
🪄 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: c652edb8-63ea-4861-a982-99acd9e42d0f
📒 Files selected for processing (9)
apps/backend/src/index.tsapps/backend/src/lib/validation.tsapps/backend/src/routes/__tests__/visual-systems.routes.integration.test.tsapps/backend/src/routes/visual-systems.routes.tsapps/backend/src/services/visual-systems.service.tsapps/frontend/src/components/VisualSystemDialog.tsxapps/frontend/src/hooks/useVisualSystem.tsapps/frontend/src/lib/api/visual-systems.tsapps/frontend/src/lib/query-keys.ts
Four follow-up fixes from inline review of #247: 1. Backend: visualSystemConfigSchema.jumpPrefixShared now accepts the empty string (via .or(z.literal(''))) so the default value of '' in VISUAL_SYSTEM_CONFIG_DEFAULTS can be round-tripped through a PUT without a 400. The DB column is NOT NULL and the shared VisualSystemConfig type declares the field as required, so '' is the natural cleared/default value (not NULL). 2. Integration test: regression test seeds a non-empty jumpPrefixShared, PATCHes with '', and asserts both response and DB row read back ''. 3. Frontend: parseGroupPrefixes now trims keys/values before the empty-length check, matching the server's .trim().min(1) validation. A whitespace-only entry used to pass client-side but fail server-side. 4. Frontend: placeholderBaseUrl protocol check changed from startsWith('http') to exact === 'http:' || === 'https:' to match the server-side check. The startsWith form would let 'httpa://example.com' and similar bogus schemes through.
Replaces the separate Routes / Characters sidebar entries in the left sidebar with a single 'Settings' entry that opens a unified tabbed modal. The tab order is Characters → Routes → Visual System. Tabs are owned by the outer dialog; each tab manages its own persistence. Inner edit dialogs (for Characters and Routes) stack on top as dialog-over-dialog, matching the pattern the user prefers over inline editing. What changed - New 'ProjectSettingsDialog' with a tab strip hosting Characters, Routes, and Visual System. - New 'tabs' primitive (state-based, no Radix dep). - New 'CharacterSettingsContent' and 'RouteSettingsContent' components extracted from the existing dialogs. - New 'visual-system.helpers' shared by the standalone dialog and the tab (parseGroupPrefixes, toVisualSystemFormState, INITIAL_VISUAL_SYSTEM_FORM). - 'VisualSystemFormContent' is now exported and used directly inside the visual tab (no nested dialog) so Cancel/Save work as expected. - 'CharacterList' and 'RouteList' accept a 'columns' prop (default 1; pass 2 in the tab to keep the dialog frame height stable). - 'ProjectSettingsDialog' uses a fixed h-[80vh] min-h-[500px] so the dialog frame is the same size across all three tabs; content scrolls inside. - 'Dialog' primitive now uses top-1/2 left-1/2 transform for reliable centering. The previous m-auto approach didn't reliably center a fixed-height dialog (especially when nested inside another open dialog). - Visual System form: Cancel button removed. Save persists; the dialog's X / outer Close button discards unsaved changes. - LeftSidebar: Routes and Characters buttons replaced with one 'Settings' button (SlidersHorizontal icon) that opens the unified modal. Verified - pnpm typecheck, lint clean. - 815 frontend + 747 backend unit tests pass. - Dialog visually centered in the viewport (was anchored to top before the transform fix).
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/frontend/src/components/CharacterDialog.tsx`:
- Around line 48-54: The close button element in the CharacterDialog component
lacks an accessible name for screen readers since it only contains an icon (X
component). Add an aria-label attribute to the button element with an
appropriate label like "Close" to provide screen readers with a descriptive name
for the control, making the close button accessible to all users.
In `@apps/frontend/src/components/CharacterSettingsContent.tsx`:
- Around line 11-17: The imports for CharacterList and CharacterEditDialog.lazy
in the CharacterSettingsContent.tsx file are using relative import paths
(./CharacterList and ./CharacterEditDialog.lazy) instead of the required `@/`
alias convention. Replace these two relative imports with their `@/` equivalents:
change import { CharacterList } from "./CharacterList" to import { CharacterList
} from "`@/components/CharacterList`" and change import { CharacterEditDialog }
from "./CharacterEditDialog.lazy" to import { CharacterEditDialog } from
"`@/components/CharacterEditDialog.lazy`" to align with the project's import
conventions.
In `@apps/frontend/src/components/ProjectSettingsDialog.tsx`:
- Around line 204-212: The condition `if (isLoading || !config)` in the
ProjectSettingsDialog component shows the loader indefinitely when the config
fetch fails, leaving users without recovery options. Instead of checking only
for `!config`, also check for a query error state (from your data fetching hook)
and display an error message or retry UI when the fetch fails. Replace the
current condition with logic that distinguishes between the loading state (show
spinner) and the error state (show error UI with retry option), ensuring the
spinner only displays while actively fetching and not when the request has
failed.
In `@apps/frontend/src/components/ui/tabs.tsx`:
- Around line 153-163: The tab button component currently lacks keyboard
navigation support for arrow keys, preventing keyboard users from moving between
tabs. Add an onKeyDown event handler to the button element that detects
ArrowLeft and ArrowRight key presses and calls setValue with the appropriate
adjacent tab value. The handler should determine the next or previous tab value
based on the key pressed and update the active tab accordingly, allowing
keyboard users to navigate between tabs using arrow keys while maintaining the
existing onClick handler for mouse interactions.
🪄 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: 24a91e03-10cf-4325-a4cc-0aa6d1e3bb67
📒 Files selected for processing (12)
apps/frontend/src/components/CharacterDialog.tsxapps/frontend/src/components/CharacterList.tsxapps/frontend/src/components/CharacterSettingsContent.tsxapps/frontend/src/components/ProjectSettingsDialog.tsxapps/frontend/src/components/RouteList.tsxapps/frontend/src/components/RouteSettingsContent.tsxapps/frontend/src/components/VisualSystemDialog.tsxapps/frontend/src/components/ide-shared/LeftSidebar.tsxapps/frontend/src/components/ide-shared/RouteSettingsDialog.tsxapps/frontend/src/components/ui/dialog.tsxapps/frontend/src/components/ui/tabs.tsxapps/frontend/src/components/visual-system.helpers.ts
- ProjectSettingsDialog: drop the useEffect that reset activeTab on open/defaultTab change. Use the render-time 'storing information from previous renders' pattern instead, so the user doesn't see a one-frame flash of the previous tab on reopen. Add aria-label to the Dialog. - VisualSystemDialog: same render-time pattern replaces the useEffect that re-hydrated the form when initialConfig changed. Add aria-label to the Dialog and to the group-prefixes textarea (defensive — the wrapping <Label htmlFor> wasn't being detected by react-doctor's static analysis). - dialog.tsx: add optional 'aria-label' and 'aria-labelledby' props on the native <dialog> so callers can give screen readers an accessible name (required by react-doctor/dialog-has-accessible-name). - tabs.tsx: replace useContext with React 19's use() in the tabs context consumer (no-react19-deprecated-apis). - LeftSidebar: extract ModeSwitcher, ProjectSelector, NavButtons, ThemeSwitcher, CollapseButton, and UserActions sub-components to shrink the main function from 477 to 220 lines (no-giant-component). Move each popover's click-outside handler into its sub-component and wrap onClose in useEffectEvent so the listener isn't re-bound on every parent render. - The remaining react-doctor hits on the render-time 'previous value' trackers are false positives (the state IS read in the if-condition, not just in handlers) and the useRef alternative is blocked by the react-hooks/refs eslint rule. Suppressed inline with the react-doctor-disable-next-line pattern already used elsewhere in the codebase. react-doctor: 82 -> 100/100 (no issues on changed files).
Three of the four inline findings were still valid; one was skipped (relative imports — matches the established convention for sibling files in this codebase, same reason it was rejected for visual-systems.ts earlier). 1. CharacterDialog: add aria-label="Close character dialog" to the X close button so screen readers announce the control. Updated the 'Dialog Controls' test to use an exact-name match for the footer Close button (the X button now also matches 'Close' in its accessible name). 2. useVisualSystem: expose isError on the hook return so callers can distinguish a failed fetch from a still-loading one. The visual tab in ProjectSettingsDialog now renders an error UI (icon + message + Retry button) before falling through to the spinner; previously a failed fetch left the spinner spinning forever. 3. tabs: add WAI-ARIA arrow-key navigation. The Tabs root tracks registered tab values via a registerTab callback (in insertion order); each TabsTrigger registers itself on mount and handles ArrowLeft / ArrowRight (wraps), Home, and End. Focus follows the active tab via data-tab-value lookup. Plus a small sidebar cleanup: the Settings button now reads 'Project Settings' (was just 'Settings' — ambiguous next to the user-level Settings gear), and the Flow button now reads 'Flow Graph' (matches its tooltip and icon). Verified - pnpm typecheck, lint clean. - 815 frontend + 747 backend unit tests pass.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/frontend/src/components/ide-shared/LeftSidebar.tsx`:
- Around line 213-225: The useEffect hook that sets up the click-outside handler
has an early return condition that checks for both !isOpen and isCollapsed, but
since the popovers are only rendered when isCollapsed is true, this prevents the
click-outside dismissal handler from being registered in the only mode where
it's needed. Remove the isCollapsed check from the early return condition on
line 214 so that the handler is set up whenever isOpen is true, regardless of
collapsed state. Apply the same fix to the similar code block around lines
388-400.
In `@apps/frontend/src/components/ui/tabs.tsx`:
- Line 200: The tab registration and keyboard navigation logic in this component
has two issues: first, the useEffect that calls registerTab(value) registers all
tabs including disabled ones, which allows arrow key navigation to activate
disabled tabs via the logic in the arrow key handlers; second, the document
query on lines 233-235 is global and not scoped to the current tablist instance,
causing the wrong tab to be focused when multiple tablists exist with the same
value. To fix this, add a check in the useEffect to only register the tab if it
is not disabled, modify the arrow key navigation handlers (around lines 205-213
and 227-230) to skip over disabled tabs when cycling through tab values, and
scope the document query to the current tablist DOM element or container instead
of querying globally on document.
🪄 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: 727a1fe5-9b8b-4cf4-8221-56591b06eb3c
📒 Files selected for processing (8)
apps/frontend/src/components/CharacterDialog.tsxapps/frontend/src/components/ProjectSettingsDialog.tsxapps/frontend/src/components/VisualSystemDialog.tsxapps/frontend/src/components/__tests__/CharacterDialog.test.tsxapps/frontend/src/components/ide-shared/LeftSidebar.tsxapps/frontend/src/components/ui/dialog.tsxapps/frontend/src/components/ui/tabs.tsxapps/frontend/src/hooks/useVisualSystem.ts
✅ Files skipped from review due to trivial changes (1)
- apps/frontend/src/components/tests/CharacterDialog.test.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/frontend/src/components/ProjectSettingsDialog.tsx
- apps/frontend/src/hooks/useVisualSystem.ts
- apps/frontend/src/components/CharacterDialog.tsx
- apps/frontend/src/components/VisualSystemDialog.tsx
…ollow-up The CodeRabbit follow-up commit (9d48051) added WAI-ARIA arrow-key navigation to the tabs primitive, which: - re-introduced useContext (reverting the React 19 use() switch) - added two useRef(new Map/Set) initializers - added a child->parent registerTab effect The useRef new-Map/Set and registerTab-effect patterns trigger react-doctor rules. Both are intentional: - useRef(new Map()) / useRef(new Set()): the rebuild cost is negligible for an empty Map/Set, and the alternative lazy-init pattern forces non-null assertions at every call site inside the closures that follow. Suppressed inline. - useEffect(registerTab): child->parent registration is the standard pattern for tab enumeration; React has no other mechanism for a parent to know about TabsTrigger children statically. Suppressed inline. Also picks up a pre-existing working-tree fix in LeftSidebar: the click-outside effect's early-return used the inverted condition `if (!isOpen || isCollapsed) return;`, which registered the listener only in the mode that has no popover to dismiss. The popover is rendered only in the collapsed branch, so the listener should be (and now is) gated on `isOpen` alone. react-doctor: 90 -> 100/100 on changed files. typecheck, lint, 815 frontend tests all green.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
Closes #25
What
Adds per-project visual system configuration: GET/PUT endpoints,
service layer, and a frontend dialog for editing the config that
controls how generated Ren'Py visual filenames are produced
(template tokens, group prefixes, padding, shared jump prefix,
placeholder base URL).
How
Backend (Fastify + Drizzle)
GET /api/projects/:projectId/visual-systemreturns the config;auto-creates a default row on first read.
PUT /api/projects/:projectId/visual-systemPATCHes a subsetof fields. Sentinels (
"",{}) clear optional fields backto
NULL.visualSystemConfigSchemawith http/https restrictionon
placeholderBaseUrland explicit clearable semantics fordefaultGroupType,placeholderBaseUrl, andgroupPrefixes.VisualSystemsService(default-on-first-read + PATCHupsert) enforces
requireProjectOwnership.clearing,
groupPrefixesJSONB round-trip, default creation,non-http protocol rejection, unknown-field rejection.
Frontend (React + TanStack Query)
useVisualSystemhook (optimistic update withrollback), and
VisualSystemDialogwith a live preview thatfeeds the current form into
generateVisualName().visualSystemKeys.DB mapping
scene_paddingis mapped to the shared API fieldlabelPaddingin the service layer (legacy column kept forbackward compat with the existing schema/migration).
How verified
pnpm typecheck— cleanpnpm lint— cleanpnpm test:unit— 1606 tests pass (no regressions)pnpm test:integration— 410 tests pass (18 new for visualsystems, no regressions)
@oracle verdict
Verdict: ready (after one round of fixes).
Round 1 review surfaced 3 must-fix + 2 should-fix + 1 nit.
All addressed:
un-clearable optional values).
defaultGroupTypeschema accepts""as clear sentinel.placeholderBaseUrlschema restricts to http/https viacustom
.refine().before returning defaults.
non-http protocol rejection.
Round 2 review returned "Verdict: ready".
Summary by CodeRabbit
Release Notes