[1227E] - #8858
Conversation
WalkthroughThe path navigator now carries request paths through asynchronous actions and validates them in reducers. Stale successes, failures, and parent-item responses are ignored. Refresh and parent-item 404 recovery only resets to the root when the requested path remains active. ChangesPath-aware navigation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PathRequest
participant pathNavigatorEpics
participant pathNavigatorReducer
PathRequest->>pathNavigatorEpics: request navigation or refresh
pathNavigatorEpics->>pathNavigatorReducer: start fetch with requested path
pathNavigatorEpics->>pathNavigatorReducer: complete or fail with path
pathNavigatorReducer->>pathNavigatorReducer: compare requested path with current path
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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)
studio-ui/ui/app/src/state/reducers/pathNavigator.ts (1)
152-172: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
pathNavigatorConditionallySetPathis the one flow left out of the new stale-result protection. Every other completion/failure action in this PR now discards results when the navigator has moved on;pathNavigatorConditionallySetPath's round-trip does not, even though it is the action used to recover from a 404 during navigation.
studio-ui/ui/app/src/state/reducers/pathNavigator.ts#L152-L172: addpathtopathNavigatorConditionallySetPathFailed's payload, movecurrentPathimmediately in thepathNavigatorConditionallySetPathcase (mirroringpathNavigatorFetchPath), and guardpathNavigatorConditionallySetPathComplete/FailedwithisStaleResult, as shown in the per-file proposed fix.studio-ui/ui/app/src/state/epics/pathNavigator.ts#L138-L148: once the reducer guard exists, forward the request path throughpathNavigatorConditionallySetPath's completion/failure so this fallback benefits from it.studio-ui/ui/app/src/state/epics/pathNavigator.ts#L384-L387: same change for the secondpathNavigatorConditionallySetPathdispatch site.🤖 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 `@studio-ui/ui/app/src/state/reducers/pathNavigator.ts` around lines 152 - 172, Update studio-ui/ui/app/src/state/reducers/pathNavigator.ts lines 152-172: include path in pathNavigatorConditionallySetPathFailed, set currentPath immediately in pathNavigatorConditionallySetPath, and guard pathNavigatorConditionallySetPathComplete and pathNavigatorConditionallySetPathFailed with isStaleResult. Update studio-ui/ui/app/src/state/epics/pathNavigator.ts lines 138-148 and 384-387 to forward the request path through both pathNavigatorConditionallySetPath completion and failure dispatches.
🤖 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 `@studio-ui/ui/app/src/state/reducers/pathNavigator.ts`:
- Around line 152-172: Update
studio-ui/ui/app/src/state/reducers/pathNavigator.ts lines 152-172: include path
in pathNavigatorConditionallySetPathFailed, set currentPath immediately in
pathNavigatorConditionallySetPath, and guard
pathNavigatorConditionallySetPathComplete and
pathNavigatorConditionallySetPathFailed with isStaleResult. Update
studio-ui/ui/app/src/state/epics/pathNavigator.ts lines 138-148 and 384-387 to
forward the request path through both pathNavigatorConditionallySetPath
completion and failure dispatches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13a7e10b-be56-4cd7-9307-5647eb3e9b3f
📒 Files selected for processing (3)
studio-ui/ui/app/src/state/actions/pathNavigator.tsstudio-ui/ui/app/src/state/epics/pathNavigator.tsstudio-ui/ui/app/src/state/reducers/pathNavigator.ts
|
@coderabbitai about the outside diff range comment, pathNavigatorFetchPath can move currentPath immediately because navigation is committed. ConditionallySetPath is different: it probes a candidate and only adopts it on success when parent.childrenCount > 0. |
|
Tip For best results, initiate chat on the files or code changes.
I withdraw the outside-diff-range comment. 🐇 ✏️ Learnings added
You are interacting with an AI system. |
|
@coderabbitai how does this work look to be merged? Any issues you can point out? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR metadata and patch ==='
gh pr view 8858 --repo craftersoftware/craftercms --json number,title,state,isDraft,baseRefName,headRefName,mergeable,reviewDecision,statusCheckRollup
gh pr diff 8858 --repo craftersoftware/craftercms --patch
echo '=== Source outlines ==='
for f in \
studio-ui/ui/app/src/state/actions/pathNavigator.ts \
studio-ui/ui/app/src/state/epics/pathNavigator.ts \
studio-ui/ui/app/src/state/reducers/pathNavigator.ts
do
echo "--- $f ---"
ast-grep outline "$f" --items all || true
done
echo '=== All changed-action references with context ==='
rg -n -C 4 \
'pathNavigator(FetchPathComplete|BulkFetchPathComplete|FetchParentItemsComplete|FetchPathFailed|BulkFetchPathFailed)\s*\(' \
studio-ui/ui/app/srcLength of output: 29392 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Epic: complete request-flow context ==='
nl -ba studio-ui/ui/app/src/state/epics/pathNavigator.ts | sed -n '90,420p'
echo '=== Reducer: complete action-handling context ==='
nl -ba studio-ui/ui/app/src/state/reducers/pathNavigator.ts | sed -n '1,270p'
echo '=== Action declarations ==='
nl -ba studio-ui/ui/app/src/state/actions/pathNavigator.ts | sed -n '1,150p'
echo '=== Path navigator tests and relevant test cases ==='
fd -t f -i 'pathNavigator' studio-ui/ui/app | sort
rg -n -C 5 \
'(PATH_NAV_|pathNavigator(FetchPath|Refresh|BulkRefresh|FetchParentItems|ConditionallySetPath)|stale|currentPath)' \
studio-ui/ui/app \
-g '*test*' -g '*spec*' || true
echo '=== All action creators and raw action-type producers in the repository ==='
rg -n -C 3 \
'(pathNavigatorFetchPathComplete|pathNavigatorBulkFetchPathComplete|pathNavigatorFetchParentItemsComplete|pathNavigatorFetchPathFailed|pathNavigatorBulkFetchPathFailed)\b|PATH_NAV_(FETCH_PATH_COMPLETE|BULK_FETCH_PATH_COMPLETE|FETCH_PARENT_ITEMS_COMPLETE|FETCH_PATH_FAILED|BULK_FETCH_PATH_FAILED)' \
.Length of output: 1497 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Epic: request-flow context ==='
sed -n '90,420p' studio-ui/ui/app/src/state/epics/pathNavigator.ts
echo '=== Reducer: action-handling context ==='
sed -n '1,270p' studio-ui/ui/app/src/state/reducers/pathNavigator.ts
echo '=== Action declarations ==='
sed -n '1,150p' studio-ui/ui/app/src/state/actions/pathNavigator.ts
echo '=== Path navigator test files ==='
find studio-ui/ui/app -type f \( -iname '*pathNavigator*test*' -o -iname '*pathNavigator*spec*' \) -print | sort
echo '=== Relevant test cases ==='
rg -n -C 5 \
'(PATH_NAV_|pathNavigator(FetchPath|Refresh|BulkRefresh|FetchParentItems|ConditionallySetPath)|stale|currentPath)' \
studio-ui/ui/app \
-g '*test*' -g '*spec*' || true
echo '=== All altered-action producers ==='
rg -n -C 3 \
'(pathNavigatorFetchPathComplete|pathNavigatorBulkFetchPathComplete|pathNavigatorFetchParentItemsComplete|pathNavigatorFetchPathFailed|pathNavigatorBulkFetchPathFailed)\b|PATH_NAV_(FETCH_PATH_COMPLETE|BULK_FETCH_PATH_COMPLETE|FETCH_PARENT_ITEMS_COMPLETE|FETCH_PATH_FAILED|BULK_FETCH_PATH_FAILED)' \
.Length of output: 46822 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Completion side-effect epic ==='
sed -n '407,500p' studio-ui/ui/app/src/state/epics/pathNavigator.ts
echo '=== Content reducer completion handlers ==='
sed -n '130,215p' studio-ui/ui/app/src/state/reducers/content.ts
echo '=== Definitions used by the content reducer ==='
rg -n -C 5 \
'function (updateItemByPath|updateItemsByPaths)|const (updateItemByPath|updateItemsByPaths)|updateItemByPath\s*=|updateItemsByPaths\s*=' \
studio-ui/ui/app/src/state/reducers/content.ts
echo '=== Test directories and configuration ==='
find . -type d \( -name '__tests__' -o -name 'test' -o -name 'tests' \) -print | sort | head -100
find . -maxdepth 4 -type f \( -name 'package.json' -o -name 'jest.config.*' -o -name 'vitest.config.*' \) -print | sort | head -100
echo '=== Existing navigator test references across the repository ==='
rg -n -C 3 \
'(pathNavigator|PATH_NAV_)' \
. \
-g '*test.*' -g '*spec.*' || trueLength of output: 10001
I found no merge-blocking issue. The implementation now captures the request path for refresh, bulk refresh, fetch, pagination, keyword, and parent-item flows. The reducer rejects a completion or failure when that path differs from The bulk refresh change is important. It preserves each navigator path before the asynchronous request starts. Travis CI passed. The PR is mergeable. GitHub still reports One non-blocking recommendation: add reducer or epic tests for these cases:
You are interacting with an AI system. |
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="craftersoftware/craftercms"
pr="8858"
echo '=== Pull request status ==='
gh pr view "$pr" --repo "$repo" \
--json number,title,state,isDraft,mergeable,reviewDecision,statusCheckRollup,headRefOid,baseRefOid
echo '=== Current pull request diff ==='
gh pr diff "$pr" --repo "$repo" --patch
echo '=== Changed file outlines ==='
for file in \
studio-ui/ui/app/src/state/actions/pathNavigator.ts \
studio-ui/ui/app/src/state/epics/pathNavigator.ts \
studio-ui/ui/app/src/state/reducers/pathNavigator.ts
do
echo "--- ${file} ---"
ast-grep outline "$file" --items all || true
done
echo '=== Action declarations ==='
sed -n '1,180p' studio-ui/ui/app/src/state/actions/pathNavigator.ts
echo '=== Epic request and completion flows ==='
sed -n '1,540p' studio-ui/ui/app/src/state/epics/pathNavigator.ts
echo '=== Reducer handlers ==='
sed -n '1,340p' studio-ui/ui/app/src/state/reducers/pathNavigator.ts
echo '=== All changed action creator call sites ==='
rg -n -C 4 --type ts \
'\bpathNavigator(FetchPathComplete|BulkFetchPathComplete|FetchParentItemsComplete|FetchPathFailed|BulkFetchPathFailed)\s*\(' \
studio-ui/ui/app/src
echo '=== Relevant test coverage ==='
fd -t f . studio-ui/ui/app | rg '(pathNavigator|PathNavigator).*\.(test|spec)\.(ts|tsx)$' || true
rg -n -C 4 --type ts \
'\b(pathNavigator(FetchPath|Refresh|BulkRefresh|FetchParentItems|ConditionallySetPath)|PATH_NAV_)' \
studio-ui/ui/app -g '*test*' -g '*spec*' || trueLength of output: 64280
The request path is captured before each asynchronous operation completes. The reducer compares that path with
GitHub reports that the PR is mergeable. CodeRabbit and Travis CI checks passed. The PR remains a draft and requires human review. The repository search found no existing path navigator tests. I recommend follow-up test coverage for stale success and failure responses, but this does not block handoff for human review.
|
rart
left a comment
rart
left a comment
There was a problem hiding this comment.
Can we make this backward compatible so that it continues to work as before if the path arg is not sent as before?
@rart the thing is that the issue was happening because of a lack of a path to identify the fetch.
By adding the path, the tree reload is performed against the new path being set, even if it wasn't fully fetched/set yet. |
2 participants
https://github.com/craftersoftware/craftercms-e/issues/1227
Summary by CodeRabbit