feat: use forceColorScheme for ?theme= param#2645
Conversation
… tests Replace ThemeSync (setColorScheme → localStorage) with MantineProvider's forceColorScheme prop so ?theme= overrides appearance without persisting. Extract search params schema to dedicated module. Add E2E tests covering ?theme=, ?dynamic=, and ?relationships= observable browser behaviour.
🦋 Changeset detectedLatest commit: 5f6e01b The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds URL-driven search-parameter parsing and theme forcing: new Zod-based Changes
Sequence DiagramsequenceDiagram
participant User
participant Browser
participant SearchParams as SearchParams Module
participant Root as __root Route
participant Mantine as MantineProvider
participant ColorScheme as ColorSchemeToggle
User->>Browser: Visit URL (e.g. ?theme=dark) or click toggle
Browser->>SearchParams: Parse & validate URL query
SearchParams->>Root: Return parsed SearchParams + resolved forceColorScheme
Root->>Mantine: Provide forceColorScheme prop and render Outlet
Mantine->>Browser: Render UI with enforced scheme
alt Toggle while theme is forced
ColorScheme->>ColorScheme: store pendingScheme (buffer)
ColorScheme->>Browser: navigate to remove ?theme
Browser->>Root: URL updated (no theme)
Root->>Mantine: forceColorScheme = undefined
ColorScheme->>ColorScheme: detect cleared force -> apply pendingScheme
ColorScheme->>Mantine: set scheme directly
else Toggle while not forced
ColorScheme->>Mantine: apply scheme directly
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
e2e/tests/search-params.spec.ts (1)
66-86: Consider adding a test for absent?dynamic=on a dynamic view.The tests cover
sequence,diagram, and an invalid value (timeline), but there's no test for what happens when?dynamic=is entirely absent on a dynamic view. Per the schema, it defaults to'diagram', so behavior should match thediagramcase — but an explicit test would lock that contract down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/search-params.spec.ts` around lines 66 - 86, Add a test to cover the case where the query param ?dynamic= is absent: inside the existing test.describe('?dynamic= search parameter'...) add a new test (similar to the other cases) that calls gotoAndWaitForCanvas(page, exportUrl(DYNAMIC_VIEW)) with no dynamic option, then assert that page.locator(SEQ_ACTOR_SELECTOR) has count 0 (or is not visible) using TIMEOUT_CANVAS; use the same helpers (gotoAndWaitForCanvas, exportUrl) and constants (DYNAMIC_VIEW, SEQ_ACTOR_SELECTOR, TIMEOUT_CANVAS) so the absent-param behavior is verified to match the 'diagram' default.packages/likec4/app/src/components/ColorSchemeToggle.tsx (1)
30-46: Consider the case where navigation resolves butcomputedColorSchemehas already changed.If the user has
?theme=dark(socomputedColorSchemeis'dark'), clicking toggle storesnext = 'light'inpendingSchemeand navigates. After navigation clears the force, Mantine falls back to localStorage/system — which could already resolve to'light'. In that case the effect still callssetColorScheme('light'), which is harmless but redundant. Not a bug, just a subtlety worth being aware of.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/likec4/app/src/components/ColorSchemeToggle.tsx` around lines 30 - 46, The toggle flow can leave pendingScheme set when navigation succeeds, causing the subsequent effect to call setColorScheme even if computedColorScheme already matches the pending value; update the post-navigation logic so that when navigation resolves you compare computedColorScheme to pendingScheme.current and only call setColorScheme(pending) if they differ, then clear pendingScheme.current (and still clear it in the navigate.catch branch). Refer to pendingScheme, navigate({...}).catch(...), computedColorScheme, and setColorScheme to locate and implement the check and clearing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/likec4/app/src/searchParams.ts`:
- Line 5: Replace the invalid Zod usage where theme is defined as
z.literal(['light', 'dark', 'auto']) by using a union of literal schemas (or
z.enum) so Zod v3 accepts it; locate the theme definition in searchParams.ts
(the theme symbol) and change it to z.union([z.literal('light'),
z.literal('dark'), z.literal('auto')]) or equivalent
z.enum(['light','dark','auto']) to validate the three allowed string values.
---
Nitpick comments:
In `@e2e/tests/search-params.spec.ts`:
- Around line 66-86: Add a test to cover the case where the query param
?dynamic= is absent: inside the existing test.describe('?dynamic= search
parameter'...) add a new test (similar to the other cases) that calls
gotoAndWaitForCanvas(page, exportUrl(DYNAMIC_VIEW)) with no dynamic option, then
assert that page.locator(SEQ_ACTOR_SELECTOR) has count 0 (or is not visible)
using TIMEOUT_CANVAS; use the same helpers (gotoAndWaitForCanvas, exportUrl) and
constants (DYNAMIC_VIEW, SEQ_ACTOR_SELECTOR, TIMEOUT_CANVAS) so the absent-param
behavior is verified to match the 'diagram' default.
In `@packages/likec4/app/src/components/ColorSchemeToggle.tsx`:
- Around line 30-46: The toggle flow can leave pendingScheme set when navigation
succeeds, causing the subsequent effect to call setColorScheme even if
computedColorScheme already matches the pending value; update the
post-navigation logic so that when navigation resolves you compare
computedColorScheme to pendingScheme.current and only call
setColorScheme(pending) if they differ, then clear pendingScheme.current (and
still clear it in the navigate.catch branch). Refer to pendingScheme,
navigate({...}).catch(...), computedColorScheme, and setColorScheme to locate
and implement the check and clearing.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
e2e/tests/search-params.spec.ts (3)
84-84: Absence assertions lack explicit timeout — inconsistent with the rest of the file.Lines 84 and 103 use
toHaveCount(0)without a timeout while every positive assertion in the file passes{ timeout: TIMEOUT_CANVAS }. If nodes or overlays are rendered asynchronously after the canvas becomes visible, an absence check without a matching wait window could pass prematurely before the element appears.♻️ Suggested fix for consistency
- await expect(page.locator(SEQ_ACTOR_SELECTOR)).toHaveCount(0) + await expect(page.locator(SEQ_ACTOR_SELECTOR)).toHaveCount(0, { timeout: TIMEOUT_CANVAS })- await expect(page.locator(RELATIONSHIPS_BROWSER)).toHaveCount(0) + await expect(page.locator(RELATIONSHIPS_BROWSER)).toHaveCount(0, { timeout: TIMEOUT_CANVAS })Also applies to: 103-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/search-params.spec.ts` at line 84, The absence assertions using page.locator(SEQ_ACTOR_SELECTOR).toHaveCount(0) are missing an explicit timeout and should mirror the other checks; update both occurrences (the locator calls at the lines referencing SEQ_ACTOR_SELECTOR) to pass { timeout: TIMEOUT_CANVAS } into toHaveCount so the test waits the same window before asserting absence.
68-68: Hardcoded CSS class selectors couple tests to implementation details.
.react-flow__node-seq-actorand.relationships-browserare internal class names. If either is renamed or replaced with a CSS-module-scoped name, these tests will break without a compile-time signal. Where possible, preferdata-testidattributes or other stable semantic selectors for E2E assertions.Also applies to: 94-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/search-params.spec.ts` at line 68, The test uses hardcoded implementation-specific selectors (SEQ_ACTOR_SELECTOR = '.react-flow__node-seq-actor' and another use of '.relationships-browser') which makes E2E fragile; change the tests to use stable semantic selectors (e.g., data-testid attributes) by replacing SEQ_ACTOR_SELECTOR and the other selector with constants that reference data-testid values (e.g., data-testid="seq-actor" and data-testid="relationships-browser"), and add those data-testid attributes to the corresponding components/DOM elements in the app code if they don't exist; update any test queries that reference SEQ_ACTOR_SELECTOR or '.relationships-browser' to query by the new data-testid selectors instead.
47-59: Misleading test name forabsentlabel in loop-generated tests.The template
`?theme=${label}`withlabel = 'absent'produces the name?theme=absent does not force — falls back to light, which reads as though the URL contained?theme=absent. The actual scenario is that the parameter is entirely absent. The same issue occurs in the?dynamic=block with['absent', {}]. Consider making the label self-describing in context, e.g.:♻️ Suggested label adjustments
- ['auto', { theme: 'auto' }], - ['absent', {}], - ['invalid (sepia)', { theme: 'sepia' }], + ['?theme=auto', { theme: 'auto' }], + ['(absent)', {}], + ['?theme=sepia (invalid)', { theme: 'sepia' }],- ['diagram', { dynamic: 'diagram' }], - ['absent', {}], - ['invalid (timeline)', { dynamic: 'timeline' }], + ['?dynamic=diagram', { dynamic: 'diagram' }], + ['(absent)', {}], + ['?dynamic=timeline (invalid)', { dynamic: 'timeline' }],Then update the test description to drop the redundant prefix:
- test(`?theme=${label} does not force — falls back to light`, ... + test(`${label} does not force — falls back to light`, ...Also applies to: 75-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/search-params.spec.ts` around lines 47 - 59, The test title generation is misleading for the 'absent' case because the template `?theme=${label}` yields `?theme=absent` even though the parameter is actually omitted; update the loop that iterates over `const [label, extra]` (and the analogous `?dynamic=` block) to use a descriptive display label (e.g., compute displayLabel = label === 'absent' ? 'parameter absent' : `?theme=${label}`) and use displayLabel in the test name, or simply change the label string from 'absent' to a self-describing value like 'parameter absent'/'missing' and adjust the template accordingly; keep the assertion/logic using gotoAndWaitForCanvas, exportUrl(STATIC_VIEW, extra) and COLOR_SCHEME_ATTR unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/tests/search-params.spec.ts`:
- Line 84: The absence assertions using
page.locator(SEQ_ACTOR_SELECTOR).toHaveCount(0) are missing an explicit timeout
and should mirror the other checks; update both occurrences (the locator calls
at the lines referencing SEQ_ACTOR_SELECTOR) to pass { timeout: TIMEOUT_CANVAS }
into toHaveCount so the test waits the same window before asserting absence.
- Line 68: The test uses hardcoded implementation-specific selectors
(SEQ_ACTOR_SELECTOR = '.react-flow__node-seq-actor' and another use of
'.relationships-browser') which makes E2E fragile; change the tests to use
stable semantic selectors (e.g., data-testid attributes) by replacing
SEQ_ACTOR_SELECTOR and the other selector with constants that reference
data-testid values (e.g., data-testid="seq-actor" and
data-testid="relationships-browser"), and add those data-testid attributes to
the corresponding components/DOM elements in the app code if they don't exist;
update any test queries that reference SEQ_ACTOR_SELECTOR or
'.relationships-browser' to query by the new data-testid selectors instead.
- Around line 47-59: The test title generation is misleading for the 'absent'
case because the template `?theme=${label}` yields `?theme=absent` even though
the parameter is actually omitted; update the loop that iterates over `const
[label, extra]` (and the analogous `?dynamic=` block) to use a descriptive
display label (e.g., compute displayLabel = label === 'absent' ? 'parameter
absent' : `?theme=${label}`) and use displayLabel in the test name, or simply
change the label string from 'absent' to a self-describing value like 'parameter
absent'/'missing' and adjust the template accordingly; keep the assertion/logic
using gotoAndWaitForCanvas, exportUrl(STATIC_VIEW, extra) and COLOR_SCHEME_ATTR
unchanged.
m9810223
changed the title
…ests Export route does not wire up dynamic variant switching or the relationship browser. Switch those tests to the interactive view route where the features are actually available.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
e2e/tests/search-params.spec.ts (1)
79-79: Inline CSS selectors belong in the sharedselectorshelper.
.react-flow__node-seq-actor(line 79) and.relationships-browser(line 105) are hardcoded strings with no central registry. The project already hase2e/helpers/selectors.tsfor this purpose. Moving them there keeps selector maintenance in one place and makes future refactors less error-prone.Also applies to: 105-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/search-params.spec.ts` at line 79, The test hardcodes CSS selectors (the constant SEQ_ACTOR_SELECTOR = '.react-flow__node-seq-actor' and the literal '.relationships-browser') instead of using the shared selectors registry; move these strings into the shared selectors helper (selectors.ts) as exported constants (e.g., export const SEQ_ACTOR_SELECTOR = '.react-flow__node-seq-actor' and export const RELATIONSHIPS_BROWSER_SELECTOR = '.relationships-browser'), then update e2e/tests/search-params.spec.ts to import and use those exported symbols instead of local literals, and scan other tests to replace any duplicate hardcoded occurrences with the new shared exports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/tests/search-params.spec.ts`:
- Line 23: The URLSearchParams initialization in the test (const params = new
URLSearchParams({ padding: '20', ...extra })) uses padding '20' which diverges
from the project's standard '22'; update the default padding value to '22' so
the test aligns with e2e/bootstrap.mjs and avoids inconsistent canvas
state—locate the URLSearchParams usage in search-params.spec.ts and change the
padding entry to '22'.
- Around line 58-70: The parameterised tests currently use the label 'absent'
which produces misleading titles like `?theme=absent` / `?dynamic=absent` even
though exportUrl(STATIC_VIEW, {}) omits the param; update the data tuples in
both the theme and dynamic loops so the absent case uses a clearer label (e.g.
'(no param)') or pull the absent case out into its own test with an explicit
title, and keep test bodies using gotoAndWaitForCanvas(page,
exportUrl(STATIC_VIEW, extra)) and assertions on COLOR_SCHEME_ATTR unchanged;
ensure you update the test(...) titles generated by the template literal so they
accurately reflect no parameter being passed.
---
Nitpick comments:
In `@e2e/tests/search-params.spec.ts`:
- Line 79: The test hardcodes CSS selectors (the constant SEQ_ACTOR_SELECTOR =
'.react-flow__node-seq-actor' and the literal '.relationships-browser') instead
of using the shared selectors registry; move these strings into the shared
selectors helper (selectors.ts) as exported constants (e.g., export const
SEQ_ACTOR_SELECTOR = '.react-flow__node-seq-actor' and export const
RELATIONSHIPS_BROWSER_SELECTOR = '.relationships-browser'), then update
e2e/tests/search-params.spec.ts to import and use those exported symbols instead
of local literals, and scan other tests to replace any duplicate hardcoded
occurrences with the new shared exports.
e2e/tests/search-params.spec.ts
Outdated
| const DYNAMIC_VIEW = 'dynamic-view-1' | ||
|
|
||
| function exportUrl(viewId: string, extra?: Record<string, string>): string { | ||
| const params = new URLSearchParams({ padding: '20', ...extra }) |
There was a problem hiding this comment.
padding: '20' diverges from the project's standard padding=22.
e2e/bootstrap.mjs constructs export URLs with ?padding=22. Using '20' here creates an inconsistency that could produce a different canvas state (e.g. clipping) from what the rest of the test suite uses. Align the default to '22' unless there's a deliberate reason for the difference.
🔧 Suggested fix
- const params = new URLSearchParams({ padding: '20', ...extra })
+ const params = new URLSearchParams({ padding: '22', ...extra })📝 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.
| const params = new URLSearchParams({ padding: '20', ...extra }) | |
| const params = new URLSearchParams({ padding: '22', ...extra }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/tests/search-params.spec.ts` at line 23, The URLSearchParams
initialization in the test (const params = new URLSearchParams({ padding: '20',
...extra })) uses padding '20' which diverges from the project's standard '22';
update the default padding value to '22' so the test aligns with
e2e/bootstrap.mjs and avoids inconsistent canvas state—locate the
URLSearchParams usage in search-params.spec.ts and change the padding entry to
'22'.
|
@davydkov Could you please review this? |
- Change exportUrl padding from 20 to 22 to match e2e/bootstrap.mjs - Rename 'absent' labels to 'no ?theme= param' / 'no ?dynamic= param' so generated test titles don't misleadingly read as ?theme=absent
|
Hey @m9810223, do you mind to add changeset? |
|
@davydkov changeset added! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/force-color-scheme-theme.md:
- Around line 1-5: The changeset currently labels 'likec4' as patch but
describes a behavioral change (URL ?theme now uses MantineProvider's
forceColorScheme and no longer persists to localStorage); update the changeset
to use at least a `minor` bump instead of `patch` and expand the description to
explicitly state that theme preferences specified via the URL will no longer be
persisted to localStorage (mentioning the move to forceColorScheme) so consumers
are aware of the breaking/behavioral change.
| --- | ||
| 'likec4': patch | ||
| --- | ||
|
|
||
| Use MantineProvider's forceColorScheme for ?theme= URL parameter instead of persisting to localStorage |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Explore the changesets directory to understand versioning patterns
echo "=== Recent changesets ==="
ls -lah .changeset/ | head -20
echo -e "\n=== Changeset file contents (last few) ==="
find .changeset -maxdepth 1 -name "*.md" -type f ! -name "README.md" | sort -r | head -5 | xargs -I {} sh -c 'echo "File: {}"; head -10 {}'
echo -e "\n=== Look for documentation on changesets ==="
cat .changeset/README.md 2>/dev/null || echo "No README found"
echo -e "\n=== Look for contributing guidelines ==="
find . -maxdepth 2 -iname "*contrib*" -o -iname "*develop*" | grep -E '\.(md|txt)$'Repository: likec4/likec4
Length of output: 1225
🏁 Script executed:
#!/bin/bash
# Check CONTRIBUTING.md for version bump guidance
echo "=== CONTRIBUTING.md content ==="
cat CONTRIBUTING.md
echo -e "\n=== package.json version and dependencies ==="
cat package.json | grep -A 5 '"version"'Repository: likec4/likec4
Length of output: 6821
🏁 Script executed:
#!/bin/bash
# Search for theme-related code and the changes mentioned in the review
echo "=== Search for theme-related code ==="
rg -i 'theme.*url|forceColorScheme|localStorage.*theme' --type ts --type js -B 2 -A 2 | head -50
echo -e "\n=== Search for the files modified in this changeset ==="
rg -i 'forceColorScheme|localStorage' --type ts --type jsx --max-count 20
echo -e "\n=== Check if there's a CHANGELOG ==="
find . -maxdepth 2 -iname 'CHANGELOG*' -o -iname 'HISTORY*'Repository: likec4/likec4
Length of output: 4048
🏁 Script executed:
#!/bin/bash
# Check CHANGELOG for version bump patterns
echo "=== CHANGELOG entries (last 30 lines) ==="
tail -30 CHANGELOG.md
echo -e "\n=== searchParams.ts implementation ==="
cat -n packages/likec4/app/src/searchParams.tsRepository: likec4/likec4
Length of output: 3203
Reconsider version bump type from patch to minor.
The changeset description—"Use MantineProvider's forceColorScheme for ?theme= URL parameter instead of persisting to localStorage"—indicates a behavior change, not a bug fix. The code confirms:
- Theme parameters previously persisted to localStorage
- Now using
forceColorSchemeinstead, preventing localStorage persistence - URL-specified theme overrides localStorage while preserving manual user preferences
Patch versions are reserved for backwards-compatible bug fixes. Since this modifies how the theme feature behaves (no longer persisting to localStorage), it qualifies as at least a minor version bump (new/modified behavior). If existing users relied on localStorage persistence, this could warrant a major version bump.
The description could also explicitly mention that theme preferences no longer persist to localStorage when specified via URL parameter, helping users understand the behavioral change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.changeset/force-color-scheme-theme.md around lines 1 - 5, The changeset
currently labels 'likec4' as patch but describes a behavioral change (URL ?theme
now uses MantineProvider's forceColorScheme and no longer persists to
localStorage); update the changeset to use at least a `minor` bump instead of
`patch` and expand the description to explicitly state that theme preferences
specified via the URL will no longer be persisted to localStorage (mentioning
the move to forceColorScheme) so consumers are aware of the breaking/behavioral
change.
…ption The ?theme= URL parameter change is a behavioral/semantic change (no longer persists to localStorage, search param default changed from 'auto' to undefined), warranting a minor bump rather than patch.
2 participants
Summary
ThemeSync(setColorScheme→ localStorage) withMantineProvider'sforceColorSchemeprop so?theme=overrides appearance without persisting to localStoragesearchParams.tsmodule?theme=,?dynamic=, and?relationships=observable browser behaviourMotivation
The
?theme=search param defaulted to'auto'andThemeSynccalledsetColorScheme(theme)on every mount, unconditionally writing to localStorage. This destroyed any preference the user had set via the color scheme toggle. The new approach only forces a color scheme when the URL explicitly requests one, and never touches localStorage.Changes
searchParams.ts__root.tsx;themeis now.optional().catch(undefined)instead of.default('auto').catch('auto')routes/__root.tsxThemeSync; movedMantineProviderhere frommain.tsx; usesforceColorSchemeconditionallymain.tsxMantineProviderwrapper (now lives in root route)ColorSchemeToggle.tsxforceColorSchemeis active, navigates to remove?theme=first, then applies the scheme viauseEffect+pendingSchemeref; includes double-click guarde2e/tests/search-params.spec.ts?theme=,?dynamic=,?relationships=e2e/.gitignoresearch-params.spec.tsandstatic-navigation.spec.tsHow it works now
?theme=forceColorSchemeautolight/dark?theme=, then applies via effect; otherwise toggles directlySemantic changes to note
?theme=autono longer means "force follow system" — it now falls back to Mantine's normal priority (localStorage > system). If the user previously toggled to dark,?theme=autowill show dark.ThemeSyncalways wrote a valid value; now garbage persists until the user clicks the toggle.Checklist
mainbefore creating this PR.Summary by CodeRabbit
New Features
Improvements
Tests
Chores