[US-18.2 / OBT-257] Console: Phases page is admin-only; managers only move phase status - #39
Open
levigtri wants to merge 4 commits into
Conversation
The Phases page exposes global phase CRUD and the dependency graph editor, but its route was not wrapped in AdminRoute (unlike users/apps) and its nav entry sat in contentNavItems, so any manager could reach it and create, rename or delete a phase and rewire dependencies platform-wide. Gate /app/phases behind AdminRoute and move the sidebar entry to adminNavItems.
The status union was declared locally in ProjectPhasesTab and duplicated by MapPage, while the shared ProjectPhaseResponse typed status as a plain string and forced an unchecked cast. Move PhaseStatus and PHASE_STATUSES into src/types/phase.ts and consume them from both pages and from projectsAPI.updatePhaseStatus.
Every phase defined by a platform admin belongs to every project, so a per-project selection no longer exists and a manager must not be able to remove a phase from their own project. Remove the Attach Phase dialog, the per-node detach button and its confirm dialog from ProjectPhasesTab, along with the attachPhase/detachPhase API methods whose endpoints are gone. The tab keeps the read-only graph and the status popover — the manager's only phase write.
📝 WalkthroughWalkthroughThe update introduces shared typed phase statuses, removes phase attachment and detachment controls, restricts the phases route to platform admins, moves Map into content navigation, and changes map phase-color lookup behavior. ChangesPhase management
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/pages/MapPage.tsx (1)
45-50: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore the fallback for unexpected phase statuses
ProjectPhaseResponse.statusis only trusted at compile time; if the API returns a value outsidePhaseStatus,PHASE_STATUS_COLORS[p.status]becomesundefinedandcolors.bgthrows while rendering the popup. Restore?? PHASE_STATUS_COLORS.not_startedhere.Suggested fix
- const colors = PHASE_STATUS_COLORS[p.status] + const colors = PHASE_STATUS_COLORS[p.status] ?? PHASE_STATUS_COLORS.not_started🤖 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 `@src/components/pages/MapPage.tsx` around lines 45 - 50, Restore the fallback in the popup color lookup using PHASE_STATUS_COLORS: when resolving colors from p.status, apply PHASE_STATUS_COLORS.not_started whenever the status key is missing or unexpected, while preserving mapped colors for valid PhaseStatus 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.
Outside diff comments:
In `@src/components/pages/MapPage.tsx`:
- Around line 45-50: Restore the fallback in the popup color lookup using
PHASE_STATUS_COLORS: when resolving colors from p.status, apply
PHASE_STATUS_COLORS.not_started whenever the status key is missing or
unexpected, while preserving mapped colors for valid PhaseStatus values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: c25b3e71-1ad8-4da2-9d57-edac5c43d870
📒 Files selected for processing (7)
src/App.tsxsrc/components/layout/Sidebar.tsxsrc/components/pages/MapPage.tsxsrc/components/pages/ProjectPhasesTab.tsxsrc/services/api.tssrc/types/index.tssrc/types/phase.ts
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
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
Phases are a platform-owned catalog: only a PlatformAdmin defines them and organizes their dependencies, and every phase automatically belongs to every project. A manager's only phase action is moving the status of a phase on a project they manage.
The console did not reflect that.
/app/phases— the full phase CRUD plus the dependency graph editor — was the one route under/appthat was not wrapped inAdminRoute(unlikeusersandapps), and its sidebar entry sat incontentNavItems, so any manager could reach it and create, rename or delete a phase and rewire the dependency graph platform-wide. The project's Phases tab also exposed Attach/Detach, letting a manager remove a phase from their own project.This gates the Phases page behind
AdminRoute, removes the attach/detach UI (the endpoints are gone in US-18.1), and lifts the phase status union into the shared types soProjectPhasesTabandMapPagestop duplicating it. Pairs with US-18.1 / OBT-256 on the backend.Changes
src/App.tsx— thephasesroute is now<AdminRoute><PhasesPage /></AdminRoute>, so a manager hitting the URL directly getsAccessDeniedPageinstead of the editor.src/components/layout/Sidebar.tsx—/app/phasesmoves fromcontentNavItemstoadminNavItems, hiding it from non-admins in both the desktop and mobile navs.src/types/phase.ts,src/types/index.ts—PhaseStatus(not_started | in_progress | completed | blocked) andPHASE_STATUSESnow live in the shared types and are re-exported by name from the index.ProjectPhaseResponse.statusis typedPhaseStatusinstead ofstring.src/services/api.ts—projectsAPI.attachPhaseandprojectsAPI.detachPhaseare removed (their endpoints no longer exist);updatePhaseStatusnow takesPhaseStatusrather than a freestring.src/components/pages/ProjectPhasesTab.tsx— removes theAttachPhaseDialog, the "Attach Phase" button, the per-node detach (X) button and itsConfirmDialog. Keeps the read-only dependency graph and the status popover, now driven by the sharedPHASE_STATUSES, and drops the uncheckedas PhaseStatuscast. The empty state explains that phases are defined by a platform admin and apply to every project.src/components/pages/MapPage.tsx—PHASE_STATUS_COLORSis keyed byPhaseStatusinstead of a plain string, so the four status values are no longer duplicated and the?? not_startedfallback becomes unnecessary.Type of Change
Testing
npm run typecheck— passesnpm run lint— 0 errors (the pre-existingProjectsPage.tsxexhaustive-deps warning remains)npm run build— passesManual, against the US-18.1 branch of the API:
/app/phasesdirectly renders Access Denied.Summary by CodeRabbit
Access Changes
Improvements