feat: post-redesign cleanup (isDefaultDir, PlanList icon, strip PropTypes) - #27
Merged
Conversation
Adds IsDefaultSettingsDir() to ConfigurationService, returns it as isDefaultDir in the GET /api/config response, and threads it through Routes -> Profiles as a prop. Profiles drops its local useState; the Reset-to-default button now hides correctly on first load when the current directory matches the OS default Tranquility location.
PropTypes runtime validation was deprecated in React 19 — declarations
are no longer checked. Removed all 'import PropTypes from prop-types'
lines and all <Component>.propTypes = { … } blocks across 46 files via
scripts/strip-proptypes.mjs (kept for reference / re-runs).
The 'prop-types' npm dep is removed in the next commit.
PropTypes were stripped from source in the previous commit; this removes the now-unused npm dependency.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds a backend signal IsDefaultSettingsDir → exposes isDefaultDir on GET /api/config and passes it to Profiles; swaps PlanList’s inline plan icon rendering to use EveTypeIcon; and mechanically strips PropTypes imports/declarations from 40+ renderer files and removes the ChangesBackend isDefaultDir Signal & Frontend Integration
PlanList Icon Enhancement
PropTypes Removal Across Frontend
Sequence DiagramsequenceDiagram
participant Backend as ConfigurationService
participant Handler as Config Handler
participant API as GET /api/config
participant Routes as Routes.jsx
participant Profiles as Profiles Component
Routes->>API: request config
API->>Handler: run handler
Handler->>Backend: IsDefaultSettingsDir()
Backend-->>Handler: returns bool / error
Handler->>API: include isDefaultDir in payload
API-->>Routes: config with isDefaultDir
Routes->>Routes: extract isDefaultDir (camel/Pascal fallback)
Routes->>Profiles: pass isDefaultDir prop
Profiles->>Profiles: render UI (reset button visibility)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
renderer/src/pages/Profiles.jsx (1)
156-166:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
refreshData()call after successful reset to update UI.The
handleResetToDefaultoperation callsresetToDefaultDirectory()but therun()function doesn't trigger a data refresh. After the API succeeds, theisDefaultDirprop won't update, leaving the Reset button visible until the user manually refreshes the page. CallrefreshData()(fromuseAppData) on success to reflect the backend state change.🤖 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 `@renderer/src/pages/Profiles.jsx` around lines 156 - 166, handleResetToDefault currently calls resetToDefaultDirectory via run but never refreshes application state; after the run completes successfully, call refreshData() (from useAppData) to update isDefaultDir and the UI — i.e., in handleResetToDefault, after the await run(...) resolves and indicates success, invoke refreshData() so the profile page reflects the backend change (referencing handleResetToDefault, run, resetToDefaultDirectory, and refreshData).
🤖 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 `@docs/superpowers/plans/2026-05-05-post-redesign-cleanup.md`:
- Around line 614-615: The document line "Expected: same error count as on
baseline (the codebase has 230 pre-existing errors, mostly `no-console`).
Crucially: no **new** errors." is out of date; update that text to reflect the
current pre-existing ESLint error count (588) or replace the hard-coded number
with a number-agnostic phrase like "must match baseline" / "no new errors beyond
baseline" so CI/triage isn't misled; update the same sentence in the "Expected:"
block and any nearby references to the baseline number to ensure consistency.
- Line 7: There are inconsistent expected Vitest totals: update every occurrence
of the phrases "117 tests" and "119" in this document so they all match the
actual current Vitest run count; run the test suite locally to determine the
correct total and replace all instances (search for the exact text "117 tests"
and the other "119" mentions) so the runbook uses one consistent number
throughout.
---
Outside diff comments:
In `@renderer/src/pages/Profiles.jsx`:
- Around line 156-166: handleResetToDefault currently calls
resetToDefaultDirectory via run but never refreshes application state; after the
run completes successfully, call refreshData() (from useAppData) to update
isDefaultDir and the UI — i.e., in handleResetToDefault, after the await
run(...) resolves and indicates success, invoke refreshData() so the profile
page reflects the backend change (referencing handleResetToDefault, run,
resetToDefaultDirectory, and refreshData).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b4d633eb-1ccb-4b8e-a773-d295370d43c6
⛔ Files ignored due to path filters (1)
renderer/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (56)
docs/superpowers/plans/2026-05-05-post-redesign-cleanup.mddocs/superpowers/specs/2026-05-05-post-redesign-cleanup-design.mdinternal/handlers/config.gointernal/services/config/configuration_service.gointernal/services/interfaces/configuration.gointernal/testutil/mock_interfaces.gorenderer/package.jsonrenderer/src/Routes.jsxrenderer/src/components/character-overview/CharacterOverviewFilter.jsxrenderer/src/components/character-overview/CharacterOverviewToolbar.jsxrenderer/src/components/character-overview/CharacterRow.jsxrenderer/src/components/character-overview/ExpandedRow.jsxrenderer/src/components/character-overview/GroupBlock.jsxrenderer/src/components/character-overview/RowMenu.jsxrenderer/src/components/common/AccountPromptModal.jsxrenderer/src/components/common/CharacterDetailModal.jsxrenderer/src/components/common/CustomConfirmDialog.jsxrenderer/src/components/common/Header.jsxrenderer/src/components/common/HeaderNav.jsxrenderer/src/components/common/HeaderToolbarActions.jsxrenderer/src/components/common/HeaderUserMenu.jsxrenderer/src/components/common/SubPageHeader.jsxrenderer/src/components/landing/LoginButton.jsxrenderer/src/components/profiles/MapAccountCard.jsxrenderer/src/components/profiles/MapCharacterCard.jsxrenderer/src/components/profiles/MappingView.jsxrenderer/src/components/profiles/MtimeSwatch.jsxrenderer/src/components/profiles/SwatchBridge.jsxrenderer/src/components/profiles/SyncProfileRow.jsxrenderer/src/components/profiles/SyncView.jsxrenderer/src/components/setup/FirstRunDialog.jsxrenderer/src/components/skillplan/AddSkillPlanModal.jsxrenderer/src/components/skillplan/MatrixShell.jsxrenderer/src/components/skillplan/MissingSkillsPopover.jsxrenderer/src/components/skillplan/PlanList.jsxrenderer/src/components/skillplan/PlanMatrix.jsxrenderer/src/components/skillplan/PlanTimeline.jsxrenderer/src/components/skills/SkillProgress.jsxrenderer/src/components/ui/Connector.jsxrenderer/src/components/ui/EveTypeIcon.jsxrenderer/src/components/ui/FilterBar.jsxrenderer/src/components/ui/Kbd.jsxrenderer/src/components/ui/LoadingScreen.jsxrenderer/src/components/ui/PageShell.jsxrenderer/src/components/ui/PairSelect.jsxrenderer/src/components/ui/ProgressRail.jsxrenderer/src/components/ui/SegmentedControl.jsxrenderer/src/components/ui/SkeletonLoader.jsxrenderer/src/components/ui/StatusDot.jsxrenderer/src/components/ui/Subheader.jsxrenderer/src/components/ui/Surface.jsxrenderer/src/pages/CharacterOverview.jsxrenderer/src/pages/Profiles.jsxrenderer/src/pages/Profiles.test.jsxrenderer/src/pages/SkillPlans.jsxscripts/strip-proptypes.mjs
💤 Files with no reviewable changes (45)
- renderer/src/components/profiles/SyncProfileRow.jsx
- renderer/src/components/skillplan/PlanMatrix.jsx
- renderer/src/components/ui/Surface.jsx
- renderer/src/components/ui/Kbd.jsx
- renderer/src/components/common/AccountPromptModal.jsx
- renderer/src/components/skillplan/MatrixShell.jsx
- renderer/src/components/landing/LoginButton.jsx
- renderer/src/components/profiles/MapAccountCard.jsx
- renderer/src/components/ui/FilterBar.jsx
- renderer/src/components/ui/Subheader.jsx
- renderer/src/components/profiles/MtimeSwatch.jsx
- renderer/src/components/skillplan/PlanTimeline.jsx
- renderer/src/components/common/HeaderUserMenu.jsx
- renderer/src/components/character-overview/ExpandedRow.jsx
- renderer/src/components/ui/SegmentedControl.jsx
- renderer/src/components/profiles/MappingView.jsx
- renderer/src/components/ui/ProgressRail.jsx
- renderer/src/components/character-overview/CharacterOverviewFilter.jsx
- renderer/src/components/skillplan/AddSkillPlanModal.jsx
- renderer/src/components/skillplan/MissingSkillsPopover.jsx
- renderer/src/components/common/CustomConfirmDialog.jsx
- renderer/package.json
- renderer/src/components/profiles/SwatchBridge.jsx
- renderer/src/components/ui/Connector.jsx
- renderer/src/components/profiles/SyncView.jsx
- renderer/src/components/common/CharacterDetailModal.jsx
- renderer/src/components/ui/PairSelect.jsx
- renderer/src/components/skills/SkillProgress.jsx
- renderer/src/components/character-overview/RowMenu.jsx
- renderer/src/components/ui/EveTypeIcon.jsx
- renderer/src/pages/SkillPlans.jsx
- renderer/src/pages/CharacterOverview.jsx
- renderer/src/components/ui/SkeletonLoader.jsx
- renderer/src/components/character-overview/CharacterOverviewToolbar.jsx
- renderer/src/components/ui/StatusDot.jsx
- renderer/src/components/common/HeaderNav.jsx
- renderer/src/components/common/HeaderToolbarActions.jsx
- renderer/src/components/common/SubPageHeader.jsx
- renderer/src/components/character-overview/GroupBlock.jsx
- renderer/src/components/common/Header.jsx
- renderer/src/components/setup/FirstRunDialog.jsx
- renderer/src/components/profiles/MapCharacterCard.jsx
- renderer/src/components/ui/PageShell.jsx
- renderer/src/components/character-overview/CharacterRow.jsx
- renderer/src/components/ui/LoadingScreen.jsx
|
|
||
| **Goal:** Tie up three loose ends from PR #26: drive `Profiles.isDefaultDir` from a new backend signal; replace the inline icon in `PlanList` with `EveTypeIcon`; strip the now-inert PropTypes declarations and the `prop-types` dependency. | ||
|
|
||
| **Architecture:** A → B → C in one PR with three logical commits (Section A is one commit because the frontend depends on the new backend field). Section C is mechanical — a Node script walks `renderer/src/`, deletes the `prop-types` import line and the `<Component>.propTypes = { … };` block from each file. The PR ships when lint, full vitest suite (117 tests), and `npm run build` are all green. |
There was a problem hiding this comment.
Unify the expected Vitest test count in this runbook.
Line 7 says 117 tests, while Line 407 and Line 630 say 119. Keep one consistent expected total to prevent false alarms during execution handoff.
Also applies to: 407-407, 630-630
🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: Ensure spelling is correct
Context: ...each file. The PR ships when lint, full vitest suite (117 tests), and npm run build ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@docs/superpowers/plans/2026-05-05-post-redesign-cleanup.md` at line 7, There
are inconsistent expected Vitest totals: update every occurrence of the phrases
"117 tests" and "119" in this document so they all match the actual current
Vitest run count; run the test suite locally to determine the correct total and
replace all instances (search for the exact text "117 tests" and the other "119"
mentions) so the runbook uses one consistent number throughout.
Comment on lines
+614
to
+615
| Expected: same error count as on baseline (the codebase has 230 pre-existing errors, mostly `no-console`). Crucially: no **new** errors. If new errors appear, they're caused by malformed deletions — revert and investigate. | ||
|
|
There was a problem hiding this comment.
Update the ESLint baseline count to match current project reality.
This section says baseline is 230 errors, but the PR context for this change indicates 588 pre-existing errors. Use either the current number or a number-agnostic check (“must match baseline”) to avoid mis-triage.
🤖 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 `@docs/superpowers/plans/2026-05-05-post-redesign-cleanup.md` around lines 614
- 615, The document line "Expected: same error count as on baseline (the
codebase has 230 pre-existing errors, mostly `no-console`). Crucially: no
**new** errors." is out of date; update that text to reflect the current
pre-existing ESLint error count (588) or replace the hard-coded number with a
number-agnostic phrase like "must match baseline" / "no new errors beyond
baseline" so CI/triage isn't misled; update the same sentence in the "Expected:"
block and any nearby references to the baseline number to ensure consistency.
handleResetToDefault and handleChooseSettingsDir mutate state on the backend but never invalidated the frontend's cached config, so the isDefaultDir-driven Reset button stayed visible after a successful reset until the 10-min HTTP cache expired. Both handlers now call refreshData() on success so the UI reflects the backend change. Same pattern as the original Sync.jsx pre-redesign; we lost it in the A.4 cleanup that removed the local setIsDefaultDir state and assumed the backend would drive the prop on its own.
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
Three loose ends from PR #26:
Profiles.isDefaultDirnow derives from a backend signal. AddsIsDefaultSettingsDir()toConfigurationService, exposes it asisDefaultDirinGET /api/config, threads it through Routes → Profiles. The Reset-to-default button hides correctly on first load when the user is already on the default Tranquility directory.PlanListuses theEveTypeIconprimitive (introduced in feat(ui): SkillPlans Timeline + merged Profiles page; six shared primitives #26) instead of duplicating the inline<img>.prop-typesnpm dep is removed.Spec / plan
docs/superpowers/specs/2026-05-05-post-redesign-cleanup-design.mddocs/superpowers/plans/2026-05-05-post-redesign-cleanup.mdCommits (in order)
feat(profiles): drive isDefaultDir from backend config— Section A end-to-endrefactor(skill-plans): use EveTypeIcon in PlanList rows— Section Bchore: strip PropTypes (inert in React 19) from renderer source— Section C.1chore: remove prop-types from renderer dependencies— Section C.255 files changed (+113/-499). The strip is net negative as expected.
Tooling
scripts/strip-proptypes.mjsis checked in as a one-shot tool used to do the strip. Idempotent — running it again is a no-op. Two files needed manual cleanup the regex couldn't handle (StatusDot's adjacent-helper case and Connector/PairSelect's standalone PropType-shape consts); those are also fixed.Test plan
go test ./...passescd renderer && npm test— 30/30 files, 119/119 tests passingcd renderer && npm run buildsucceeds/profiles?view=syncon first launch with default Tranquility dir → Reset button is hidden/skill-plansBy-plan view still renders plan icons correctlyNotes
no-consoleand React 19prop-typesrule violations that are now resolved by the strip — net lint count actually drops). No new errors introduced.prop-typesnpm package itself is still present transitively (pulled in by@mui/utils). That's expected — we only removed the direct dependency.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests