Refactor management UI to list view and edit dialog pattern#180
Merged
mikkisguy merged 4 commits intoMay 31, 2026
Merged
Conversation
|
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 (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR refactors routes, variables, and stats from inline management UIs into list views with dedicated edit dialogs, locks route/variable keys by removing them from update schemas and service inputs, updates frontend types/hooks, adds list/edit components, rewires parent dialogs, adjusts tests, and adds a design spec. ChangesRoutes, Variables, and Stats: List + Edit Dialog Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 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
changed the title
![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/__tests__/variables.service.integration.test.ts`:
- Around line 422-425: Add an explicit test in this integration suite that
verifies key immutability by calling updateVariable with a payload containing a
new key (e.g., updateVariable(id, userId, { key: "new_key" })) and asserting the
contract: either the call rejects with the appropriate error or the variable's
key remains unchanged after the call; place the test near the existing "should
throw NotFoundError when variable does not exist" case and reference
updateVariable and the variables service so CI enforces that key updates are
rejected/ignored.
In `@apps/frontend/src/components/script-mode/VariablesDialog.tsx`:
- Around line 100-111: The VariableEditDialog's visibility is currently driven
only by editingVariableId and ignores the parent dialog's open state, causing
the edit dialog to remain or reopen when the parent closes; update the logic so
the edit dialog only opens when both the parent `open` prop/state and
`editingVariableId` indicate edit mode (e.g., pass open={open &&
editingVariableId !== null} to VariableEditDialog) or add an effect that clears
`editingVariableId` when the parent `open` becomes false (useEffect(() => { if
(!open) setEditingVariableId(null); }, [open])); reference variables:
VariableEditDialog, editingVariableId, MODE_NEW, and the parent `open`
prop/state.
In `@apps/frontend/src/components/StatEditDialog.tsx`:
- Around line 7-20: The TSX file's local imports must include .js extensions for
ESM compliance: update the imports that reference local modules (Dialog
components from "`@/components/ui/dialog`", Button from "`@/components/ui/button`",
Input from "`@/components/ui/input`", Label from "`@/components/ui/label`", and
useStats from "`@/hooks/useStats`") to include the .js extension (e.g.
"`@/components/ui/dialog.js`", "`@/components/ui/button.js`", etc.), leaving
external package imports (like "lucide-react" and "`@branchforge/shared`")
unchanged.
- Around line 140-145: The numeric conversion in handleChange (which updates
StatFormState via setForm for fields minValue/maxValue) can produce NaN; change
the conversion to parse the value, check isNaN, and if NaN either retain the
previous numeric value or set a safe fallback (e.g., null/undefined) instead of
storing NaN, and add an explicit guard in the save handler
(handleSave/handleSubmit or whatever submits create/update payloads) to validate
that minValue and maxValue are real numbers (not NaN) and fail validation if
they are not before sending the payload.
In `@apps/frontend/src/components/StatList.tsx`:
- Around line 56-70: The row currently uses a non-semantic div with
role="button" and contains focusable edit/delete Button controls which violates
ARIA; change the markup so the selectable action is a real interactive element
and not a parent of other buttons: remove role="button" from the outer container
in StatList.tsx and either (a) replace the row click handler with a dedicated
<button> element that covers the selection area and keeps onClick={() =>
onSelect(stat.key)} / onKeyDown logic attached to that button (leaving the
edit/delete Buttons as siblings inside the row), or (b) keep the outer div
purely presentational (no role or tabIndex) and add a child <button> (or
focusable element) that invokes onSelect(stat.key); ensure you update references
around onSelect, stat.key, stat.id, and isSelected so the edit/delete Buttons
(the Buttons previously at ~lines 91-117) are no longer descendants of a
role="button" element and stopPropagation handling is not required.
🪄 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: 37cc612f-c4b9-4e55-9490-693cf3b290c3
📒 Files selected for processing (25)
apps/backend/src/lib/validation.tsapps/backend/src/services/__tests__/variables.service.integration.test.tsapps/backend/src/services/route-configs.service.tsapps/backend/src/services/variables.service.tsapps/frontend/src/components/CharacterDialog.tsxapps/frontend/src/components/RouteEditDialog.tsxapps/frontend/src/components/RouteList.tsxapps/frontend/src/components/RouteSettingsContent.tsxapps/frontend/src/components/StatEditDialog.tsxapps/frontend/src/components/StatList.tsxapps/frontend/src/components/StatManagementContent.tsxapps/frontend/src/components/StatsContent.tsxapps/frontend/src/components/VariableEditDialog.tsxapps/frontend/src/components/VariablesContent.tsxapps/frontend/src/components/VariablesList.tsxapps/frontend/src/components/VariablesManagementContent.tsxapps/frontend/src/components/ide-shared/RouteSettingsDialog.tsxapps/frontend/src/components/script-mode/StatManagementDialog.tsxapps/frontend/src/components/script-mode/VariablesDialog.tsxapps/frontend/src/components/write-mode/ProseEditor.tsxapps/frontend/src/hooks/useRouteConfigs.tsapps/frontend/src/hooks/useVariables.tsapps/frontend/src/lib/api/route-configs.tsapps/frontend/src/lib/api/variables.tsdocs/superpowers/specs/2026-05-30-list-edit-dialogs-refactor-design.md
💤 Files with no reviewable changes (13)
- apps/frontend/src/components/VariablesContent.tsx
- apps/frontend/src/components/StatManagementContent.tsx
- apps/frontend/src/hooks/useRouteConfigs.ts
- apps/frontend/src/components/StatsContent.tsx
- apps/frontend/src/lib/api/variables.ts
- apps/frontend/src/lib/api/route-configs.ts
- apps/frontend/src/components/RouteSettingsContent.tsx
- apps/backend/src/services/route-configs.service.ts
- apps/backend/src/services/variables.service.ts
- apps/frontend/src/components/CharacterDialog.tsx
- apps/frontend/src/components/VariablesManagementContent.tsx
- apps/frontend/src/hooks/useVariables.ts
- apps/backend/src/lib/validation.ts
mikkisguy
deleted the
restructuring-character-management-and-handling-in-sidebars
branch
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 by CodeRabbit
New Features
Changes
Documentation