Bug 8726-URL-based layout persistence#8729
Open
Seth-Wadsworth wants to merge 2 commits intomakeplane:previewfrom
Open
Bug 8726-URL-based layout persistence#8729Seth-Wadsworth wants to merge 2 commits intomakeplane:previewfrom
Seth-Wadsworth wants to merge 2 commits intomakeplane:previewfrom
Conversation
|
|
Contributor
📝 WalkthroughWalkthroughThis PR introduces URL synchronization for layout selection by adding pure-logic helper functions to parse and update layout query parameters, and integrating them into the layout-selection component to sync the current layout with the URL on mount and after user changes. Changes
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/codemods/tests/layout-selection.logic.test.ts (1)
7-35: Consider adding edge case tests for robustness.The current tests cover the main scenarios well. Consider adding tests for these edge cases:
- Empty search string:
getLayoutFromUrl("", ["list"])- Search without leading
?:getLayoutFromUrl("layout=list", ["list"])- Empty
validLayoutsarray- URL-encoded layout values
📝 Example additional tests
describe("getLayoutFromUrl", () => { + it("handles empty search string", () => { + const result = getLayoutFromUrl("", ["list"]); + expect(result).toBeNull(); + }); + + it("handles search without leading ?", () => { + const result = getLayoutFromUrl("layout=list", ["list"]); + expect(result).toBe("list"); + }); + + it("returns null for empty validLayouts", () => { + const result = getLayoutFromUrl("?layout=list", []); + expect(result).toBeNull(); + }); + it("returns the layout when valid", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/codemods/tests/layout-selection.logic.test.ts` around lines 7 - 35, Add unit tests for edge cases around getLayoutFromUrl and setLayoutInQuery: include tests calling getLayoutFromUrl with an empty search string (""), with a search string that lacks the leading "?" ("layout=list"), with an empty validLayouts array (e.g., []), and with URL-encoded layout values (e.g., "layout=kanban%20view"); also add tests for setLayoutInQuery when the input query is empty ("") and when it lacks a leading "?" to ensure it returns correctly formatted query strings and properly overwrites existing layout entries using the existing getLayoutFromUrl and setLayoutInQuery helpers.apps/web/core/components/issues/issue-layouts/filters/header/layout-selection.tsx (1)
17-18: Import organization: React imports should come first.Per convention, React imports typically precede library and local imports. Consider moving
useEffectimport to the top with other React-related imports.♻️ Suggested import organization
+import { useEffect } from "react"; // plane imports import { ISSUE_LAYOUTS } from "@plane/constants"; import { useTranslation } from "@plane/i18n"; import { Tooltip } from "@plane/propel/tooltip"; import type { EIssueLayoutTypes } from "@plane/types"; import { cn } from "@plane/utils"; // components import { IssueLayoutIcon } from "@/components/issues/issue-layouts/layout-icon"; // hooks import { usePlatformOS } from "@/hooks/use-platform-os"; -import { useEffect } from "react"; import { getLayoutFromUrl, setLayoutInQuery } from "./layout-selection.logic";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/core/components/issues/issue-layouts/filters/header/layout-selection.tsx` around lines 17 - 18, Move the React import so it appears before other imports: place the useEffect import from "react" above the getLayoutFromUrl and setLayoutInQuery imports in layout-selection.tsx (the file that exports/uses useEffect, getLayoutFromUrl, setLayoutInQuery). Ensure the import ordering follows the convention: React imports first, then external libraries, then local modules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/web/core/components/issues/issue-layouts/filters/header/layout-selection.tsx`:
- Around line 31-39: The useEffect in layout-selection.tsx is intended to run
only on mount but currently lists selectedLayout in its dependency array causing
repeated runs; fix by removing selectedLayout from the dependency array so it
becomes useEffect(() => { ... }, [layouts, onChange]) and, if ESLint complains,
either add a // eslint-disable-next-line react-hooks/exhaustive-deps comment
above the effect or implement a simple initialized ref (e.g., const
initializedRef = useRef(false>) and guard the body with if
(initializedRef.current) return; initializedRef.current = true; while keeping
layouts and onChange as needed; reference getLayoutFromUrl, selectedLayout, and
onChange to locate the effect.
- Around line 45-50: This component mutates browser history directly via
window.history.replaceState after calling setLayoutInQuery, which can desync
Next.js routing; replace that logic to use Next.js routing hooks (useRouter
and/or useSearchParams) or remove history handling and delegate URL updates to
the parent HeaderFilters — specifically, stop calling
window.history.replaceState in layout-selection.tsx, instead call
router.replace({ pathname: router.pathname, query: updatedQuery }, undefined, {
shallow: true }) using useRouter (or accept an onChange callback like
MobileLayoutSelection and let HeaderFilters call router.replace). Locate
setLayoutInQuery and the block that does window.history.replaceState and convert
it to useRouter.replace or move the synchronization responsibility to
HeaderFilters so this component only emits onChange.
In `@packages/codemods/tests/layout-selection.logic.test.ts`:
- Around line 2-5: The test imports getLayoutFromUrl and setLayoutInQuery from
another app which violates the monorepo dependency rules; either relocate the
test next to the implementation (move this test file into the same directory as
the implementation so it imports locally) or extract getLayoutFromUrl and
setLayoutInQuery into a shared package (e.g., packages/utils) and add that
package as a dependency for both the app and the test package; update imports to
reference the local relative module or the new package export and ensure
package.json workspace dependencies are updated accordingly.
---
Nitpick comments:
In
`@apps/web/core/components/issues/issue-layouts/filters/header/layout-selection.tsx`:
- Around line 17-18: Move the React import so it appears before other imports:
place the useEffect import from "react" above the getLayoutFromUrl and
setLayoutInQuery imports in layout-selection.tsx (the file that exports/uses
useEffect, getLayoutFromUrl, setLayoutInQuery). Ensure the import ordering
follows the convention: React imports first, then external libraries, then local
modules.
In `@packages/codemods/tests/layout-selection.logic.test.ts`:
- Around line 7-35: Add unit tests for edge cases around getLayoutFromUrl and
setLayoutInQuery: include tests calling getLayoutFromUrl with an empty search
string (""), with a search string that lacks the leading "?" ("layout=list"),
with an empty validLayouts array (e.g., []), and with URL-encoded layout values
(e.g., "layout=kanban%20view"); also add tests for setLayoutInQuery when the
input query is empty ("") and when it lacks a leading "?" to ensure it returns
correctly formatted query strings and properly overwrites existing layout
entries using the existing getLayoutFromUrl and setLayoutInQuery helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a29ace3-1e42-4d1e-ab42-18c4a1f7f647
📒 Files selected for processing (3)
apps/web/core/components/issues/issue-layouts/filters/header/layout-selection.logic.tsapps/web/core/components/issues/issue-layouts/filters/header/layout-selection.tsxpackages/codemods/tests/layout-selection.logic.test.ts
Comment on lines
+31
to
+39
| // Read layout from URL once on mount and apply if valid | ||
| useEffect(() => { | ||
| if (typeof window === "undefined") return; | ||
|
|
||
| const layout = getLayoutFromUrl(window.location.search, layouts); | ||
| if (layout && (layout as EIssueLayoutTypes) !== selectedLayout) { | ||
| onChange(layout as EIssueLayoutTypes); | ||
| } | ||
| }, [layouts, onChange, selectedLayout]); |
Contributor
There was a problem hiding this comment.
useEffect runs on every selectedLayout change, not just on mount.
The comment states "Read layout from URL once on mount," but including selectedLayout in the dependency array causes the effect to run on every layout change. This is unnecessary overhead since the URL is updated synchronously in handleOnChange.
To run truly only on mount, consider removing selectedLayout from dependencies. Alternatively, use a ref to track whether initialization has occurred.
♻️ Option 1: Run only on mount (suppress lint warning)
useEffect(() => {
if (typeof window === "undefined") return;
const layout = getLayoutFromUrl(window.location.search, layouts);
if (layout && (layout as EIssueLayoutTypes) !== selectedLayout) {
onChange(layout as EIssueLayoutTypes);
}
+ // eslint-disable-next-line react-hooks/exhaustive-deps -- Initialize from URL once on mount only
- }, [layouts, onChange, selectedLayout]);
+ }, []);♻️ Option 2: Use a ref to track initialization
+import { useEffect, useRef } from "react";
-import { useEffect } from "react";
export function LayoutSelection(props: Props) {
const { layouts, onChange, selectedLayout } = props;
+ const initializedRef = useRef(false);
useEffect(() => {
+ if (initializedRef.current) return;
if (typeof window === "undefined") return;
const layout = getLayoutFromUrl(window.location.search, layouts);
if (layout && (layout as EIssueLayoutTypes) !== selectedLayout) {
onChange(layout as EIssueLayoutTypes);
}
+ initializedRef.current = true;
}, [layouts, onChange, selectedLayout]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Read layout from URL once on mount and apply if valid | |
| useEffect(() => { | |
| if (typeof window === "undefined") return; | |
| const layout = getLayoutFromUrl(window.location.search, layouts); | |
| if (layout && (layout as EIssueLayoutTypes) !== selectedLayout) { | |
| onChange(layout as EIssueLayoutTypes); | |
| } | |
| }, [layouts, onChange, selectedLayout]); | |
| // Read layout from URL once on mount and apply if valid | |
| useEffect(() => { | |
| if (typeof window === "undefined") return; | |
| const layout = getLayoutFromUrl(window.location.search, layouts); | |
| if (layout && (layout as EIssueLayoutTypes) !== selectedLayout) { | |
| onChange(layout as EIssueLayoutTypes); | |
| } | |
| // eslint-disable-next-line react-hooks/exhaustive-deps -- Initialize from URL once on mount only | |
| }, []); |
Suggested change
| // Read layout from URL once on mount and apply if valid | |
| useEffect(() => { | |
| if (typeof window === "undefined") return; | |
| const layout = getLayoutFromUrl(window.location.search, layouts); | |
| if (layout && (layout as EIssueLayoutTypes) !== selectedLayout) { | |
| onChange(layout as EIssueLayoutTypes); | |
| } | |
| }, [layouts, onChange, selectedLayout]); | |
| import { useEffect, useRef } from "react"; | |
| export function LayoutSelection(props: Props) { | |
| const { layouts, onChange, selectedLayout } = props; | |
| const initializedRef = useRef(false); | |
| useEffect(() => { | |
| if (initializedRef.current) return; | |
| if (typeof window === "undefined") return; | |
| const layout = getLayoutFromUrl(window.location.search, layouts); | |
| if (layout && (layout as EIssueLayoutTypes) !== selectedLayout) { | |
| onChange(layout as EIssueLayoutTypes); | |
| } | |
| initializedRef.current = true; | |
| }, [layouts, onChange, selectedLayout]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/web/core/components/issues/issue-layouts/filters/header/layout-selection.tsx`
around lines 31 - 39, The useEffect in layout-selection.tsx is intended to run
only on mount but currently lists selectedLayout in its dependency array causing
repeated runs; fix by removing selectedLayout from the dependency array so it
becomes useEffect(() => { ... }, [layouts, onChange]) and, if ESLint complains,
either add a // eslint-disable-next-line react-hooks/exhaustive-deps comment
above the effect or implement a simple initialized ref (e.g., const
initializedRef = useRef(false>) and guard the body with if
(initializedRef.current) return; initializedRef.current = true; while keeping
layouts and onChange as needed; reference getLayoutFromUrl, selectedLayout, and
onChange to locate the effect.
Comment on lines
+45
to
+50
| if (typeof window !== "undefined") { | ||
| const newQuery = setLayoutInQuery(window.location.search, layoutKey); | ||
| const newUrl = newQuery ? `${window.location.pathname}?${newQuery}` : window.location.pathname; | ||
|
|
||
| window.history.replaceState({}, "", newUrl); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other components in this codebase handle URL query parameter updates
rg -n "history\.replaceState|history\.pushState" apps/web/ --type ts --type tsx -C 2 | head -50
# Check if there's an established pattern using Next.js router
rg -n "router\.replace|router\.push.*query" apps/web/core/components/ --type ts --type tsx | head -30Repository: makeplane/plane
Length of output: 114
🏁 Script executed:
#!/bin/bash
# Search for history.replaceState and history.pushState without file type specification
rg "history\.(replaceState|pushState)" apps/web/ -C 2
# Also search the specific file to understand its context
cat apps/web/core/components/issues/issue-layouts/filters/header/layout-selection.tsxRepository: makeplane/plane
Length of output: 3428
🏁 Script executed:
#!/bin/bash
# Search for useRouter usage in similar layout/filter components
rg "useRouter\|useSearchParams" apps/web/core/components/issues/ -C 1 | head -50
# Check if layout-selection is used in route components
rg "LayoutSelection" apps/web/ -C 1 | head -30
# Look for similar patterns of URL query updates in the codebase
rg "router\.(push|replace).*query|useSearchParams" apps/web/core/components/ -C 2 | head -50Repository: makeplane/plane
Length of output: 6186
🏁 Script executed:
#!/bin/bash
# Find the useQueryParams utility and its usage pattern
rg "useQueryParams" apps/web/core/components/ -B 2 -A 3 | head -80
# Check how LayoutSelection is integrated in filters.tsx
cat apps/web/core/components/issues/filters.tsx | head -100Repository: makeplane/plane
Length of output: 8562
🏁 Script executed:
#!/bin/bash
# Check mobile-layout-selection.tsx for the same pattern
cat apps/web/core/components/issues/issue-layouts/filters/header/mobile-layout-selection.tsx
# Check the logic file to understand the intent
cat apps/web/core/components/issues/issue-layouts/filters/header/layout-selection.logic.tsRepository: makeplane/plane
Length of output: 2688
Use Next.js routing hooks instead of direct window.history manipulation.
This component directly uses window.history.replaceState, which bypasses Next.js router's internal state and can cause desync if users navigate via Next.js methods (Link, router.push). The codebase has an established pattern using useRouter and useSearchParams (seen in pages/version/root.tsx, navigation-pane/root.tsx, and other routing-aware components).
Either refactor this component to use Next.js routing hooks if it owns URL state, or move URL synchronization responsibility to the parent component (HeaderFilters) which should coordinate both state updates and route updates. Note that MobileLayoutSelection correctly handles this by only exposing the onChange callback without manipulating history.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/web/core/components/issues/issue-layouts/filters/header/layout-selection.tsx`
around lines 45 - 50, This component mutates browser history directly via
window.history.replaceState after calling setLayoutInQuery, which can desync
Next.js routing; replace that logic to use Next.js routing hooks (useRouter
and/or useSearchParams) or remove history handling and delegate URL updates to
the parent HeaderFilters — specifically, stop calling
window.history.replaceState in layout-selection.tsx, instead call
router.replace({ pathname: router.pathname, query: updatedQuery }, undefined, {
shallow: true }) using useRouter (or accept an onChange callback like
MobileLayoutSelection and let HeaderFilters call router.replace). Locate
setLayoutInQuery and the block that does window.history.replaceState and convert
it to useRouter.replace or move the synchronization responsibility to
HeaderFilters so this component only emits onChange.
Comment on lines
+2
to
+5
| import { | ||
| getLayoutFromUrl, | ||
| setLayoutInQuery, | ||
| } from "../../../apps/web/core/components/issues/issue-layouts/filters/header/layout-selection.logic"; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the import path resolves correctly
fd -t f "layout-selection.logic.ts" --exec echo "Found: {}"
# Verify the test file location
fd -t f "layout-selection.logic.test.ts" --exec echo "Test at: {}"
# Check if there are other tests in codemods that import from apps/web
rg -l "from.*\.\./.*apps/web" packages/codemods/Repository: makeplane/plane
Length of output: 275
🏁 Script executed:
# Check tsconfig configurations for both locations
find . -name "tsconfig.json" -type f | head -20Repository: makeplane/plane
Length of output: 843
🏁 Script executed:
# Look at the packages/codemods structure and its tsconfig
cat packages/codemods/tsconfig.json 2>/dev/null || echo "tsconfig.json not found in packages/codemods"Repository: makeplane/plane
Length of output: 352
🏁 Script executed:
# Check if there are other imports from apps/web in the codebase tests
rg "from.*apps/web" --type ts --type tsx | grep -E "(test|spec)" | head -20Repository: makeplane/plane
Length of output: 86
🏁 Script executed:
# Check the root tsconfig and monorepo structure
cat tsconfig.json 2>/dev/null || cat tsconfig.base.json 2>/dev/null || echo "Base tsconfig not found"Repository: makeplane/plane
Length of output: 82
🏁 Script executed:
# Examine the actual import in the test file
cat -n packages/codemods/tests/layout-selection.logic.test.ts | head -10Repository: makeplane/plane
Length of output: 533
🏁 Script executed:
# Check monorepo configuration
cat pnpm-workspace.yaml 2>/dev/null || cat package.json | head -30Repository: makeplane/plane
Length of output: 994
🏁 Script executed:
# Check if there are exports configured in apps/web that would allow external imports
cat apps/web/package.json | grep -A 10 "exports"Repository: makeplane/plane
Length of output: 41
🏁 Script executed:
# Look for any documentation about module imports in the codebase
find . -maxdepth 2 -name "*.md" -type f | xargs grep -l "import\|path\|package" 2>/dev/null | head -10Repository: makeplane/plane
Length of output: 147
🏁 Script executed:
# Check if codemods package has any path or dependency configuration that might allow this import
cat packages/codemods/package.jsonRepository: makeplane/plane
Length of output: 1046
🏁 Script executed:
# Search for similar import patterns (without the test/spec filter)
rg "from.*\.\./.*apps/web" --type ts | head -10Repository: makeplane/plane
Length of output: 216
Cross-package import violates monorepo structure.
The test file imports from apps/web/ without declaring it as a dependency. In a monorepo using pnpm workspaces, cross-package imports should either rely on declared dependencies or be avoided entirely. Additionally, apps/web does not export these functions for external use.
Move the test file alongside the logic file in apps/web/core/components/issues/issue-layouts/filters/header/, or extract the shared logic to a proper package (e.g., packages/utils or similar) that both can depend on.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/codemods/tests/layout-selection.logic.test.ts` around lines 2 - 5,
The test imports getLayoutFromUrl and setLayoutInQuery from another app which
violates the monorepo dependency rules; either relocate the test next to the
implementation (move this test file into the same directory as the
implementation so it imports locally) or extract getLayoutFromUrl and
setLayoutInQuery into a shared package (e.g., packages/utils) and add that
package as a dependency for both the app and the test package; update imports to
reference the local relative module or the new package export and ensure
package.json workspace dependencies are updated accordingly.
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
2 participants
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.
Description
This PR resolves an issue where the selected layout on the Views page would reset whenever the user clicked outside the browser window or navigated between pages. The previous implementation stored layout state only in local component state, which was overwritten during re‑renders triggered by focus changes or navigation. To ensure predictable behavior and persistent layout selection, the layout state has been moved into a URL‑driven model. The selected layout is now encoded in the query parameters, allowing it to persist across navigation, refreshes, and window focus changes.
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
Testing focused on verifying that layout selection now persists reliably across all interaction patterns that previously caused resets. Manual testing confirmed that selecting a layout correctly updates the URL and that refreshing the page restores the chosen layout. Additional regression testing validated that clicking outside the browser window no longer triggers a reset and that navigation between different views preserves the layout parameter. Unit tests were added for the updated layout‑selection logic to ensure that URL parsing and query‑parameter updates behave consistently and do not introduce regressions.
References
#8726
Summary by CodeRabbit