fix[mobile]: task bug#2631
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughUpdated split layout reconciliation logic to preserve excluded splits by deriving comparisons from Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
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)
js/app/packages/app/component/split-layout/layoutManager.ts (1)
942-966:⚠️ Potential issue | 🟡 MinorEdge case to confirm: URL returning to content matching excluded split.
If
newSplitscontains content matching the currently-excluded split (e.g., browser back to a URL whose segment corresponds to the background split on mobile),reconcileSplitswill create a fresh split for that content while preserving the excluded split, resulting in two splits instate.splitswith the same(type, id)— one excluded, one not. This breaks assumptions ingetSplitByContent(which filters to non-excluded only), duplicate-detection, and potentially the swipe-back flow.This isn't a regression introduced by the diff, and it may be intentionally out of scope, but the
navigationInterceptoronly guards programmatic navigation — not browser back or manual URL entry. Worth confirming whether URL-based navigation to excluded content is expected/acceptable behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/app/component/split-layout/layoutManager.ts` around lines 942 - 966, reconcileSplits can create a duplicate of an already-excluded split when URL/navigation returns to that content; update reconcileSplits to detect and reuse an existing split even if it's currently excluded instead of building a fresh one: when iterating newSplits, if sameContent matches a split in state.splits that is excluded, reuse that split (clear or update its excluded state so it becomes visible), reuse its id and add to usedIds, and push it into resultSplits; adjust the reuse logic around buildSplit and the check that currently only inspects visibleSplits so it queries state.splits (or calls getSplitByContent with an includeExcluded option) to avoid producing duplicate (type,id) entries and to keep swipe/back flows consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@js/app/packages/app/component/split-layout/layoutManager.ts`:
- Around line 942-966: reconcileSplits can create a duplicate of an
already-excluded split when URL/navigation returns to that content; update
reconcileSplits to detect and reuse an existing split even if it's currently
excluded instead of building a fresh one: when iterating newSplits, if
sameContent matches a split in state.splits that is excluded, reuse that split
(clear or update its excluded state so it becomes visible), reuse its id and add
to usedIds, and push it into resultSplits; adjust the reuse logic around
buildSplit and the check that currently only inspects visibleSplits so it
queries state.splits (or calls getSplitByContent with an includeExcluded option)
to avoid producing duplicate (type,id) entries and to keep swipe/back flows
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a50c47f-7fd9-4745-b957-c8ca8716a575
📒 Files selected for processing (2)
js/app/packages/app/component/split-layout/layoutManager.tsjs/app/packages/app/component/split-layout/mobile/createMobileSwipeLayout.ts
we were not filtering the background mobile splits out when we attempted to reconcile the splits against url state