Change presets/bg.json => backgrounds.json, migrate, change tab background to tab:background key#3108
Change presets/bg.json => backgrounds.json, migrate, change tab background to tab:background key#3108
Conversation
…d create tab:background key
…clear the new tab:background key...
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughReplaces the preset-based background system with a dedicated backgrounds system: adds BackgroundConfigType and a new "tab:background" settings/meta key, provides MigratePresetsBackgrounds() to move bg@ entries into backgrounds.json, ships a default backgrounds.json and clears old presets.json. Introduces MetaKeyAtomFnType (renamed from BlockMetaKeyAtomFnType), adds WaveEnv APIs getTabMetaKeyAtom and getConfigBackgroundAtom with implementations and mock support, updates frontend hooks/components to resolve tab backgrounds/border colors, extends schemas/types, updates CLI setbg flags/validation, and refreshes docs and editor schemas. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 docstrings
🧪 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 |
Code Review SummaryStatus: 1 Minor Issue | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Other Observations
Files Reviewed (41 files)
Reviewed by minimax-m2.5-20260211 Reviewed by minimax-m2.5-20260211 · 322,645 tokens |
![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 (5)
docs/src/components/versionbadge.css (1)
24-41: LGTM with optional refactor suggestion.The
.deprecated-badgestyling is well-implemented with appropriate gray colors for deprecation semantics and proper dark mode support.The styling properties are largely duplicated from
.version-badge. If more badge variants are added in the future, consider extracting common properties to a shared base class, but this is not necessary for the current scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/src/components/versionbadge.css` around lines 24 - 41, The .deprecated-badge rules duplicate many properties from .version-badge; refactor by extracting shared declarations (display, padding, margin-left, font-size, font-weight, line-height, border-radius, vertical-align, white-space) into a common base class (e.g., .badge or .base-badge) and have .version-badge and .deprecated-badge extend/override only the variant-specific properties like background-color and color (including the [data-theme="dark"] overrides) to reduce duplication and make future badge variants easier to add.cmd/generateschema/main-generateschema.go (1)
188-192: Consider updating the error message for consistency.The schema generation now uses
BackgroundConfigTypeandbackgrounds.json, but the error message still references "bg presets". This is a minor inconsistency.🧹 Suggested fix
bgPresetsTemplate := make(map[string]wconfig.BackgroundConfigType) err = generateSchema(&bgPresetsTemplate, WaveSchemaBackgroundsFileName) if err != nil { - log.Fatalf("bg presets schema error: %v", err) + log.Fatalf("backgrounds schema error: %v", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/generateschema/main-generateschema.go` around lines 188 - 192, The log message "bg presets schema error" is inconsistent with the types and filename; update the error message in the generateSchema call handling to reference BackgroundConfigType/WaveSchemaBackgroundsFileName (e.g., "backgrounds schema error" or "backgrounds.json schema error") so it matches bgPresetsTemplate, BackgroundConfigType and WaveSchemaBackgroundsFileName; locate the block using bgPresetsTemplate and generateSchema and replace the log.Fatalf message accordingly.frontend/app/aipanel/aipanel.tsx (1)
29-29: Minor: Trailing whitespace detected.There appears to be trailing whitespace (tab character) at the end of this import line.
🧹 Proposed fix
-import { BYOKAnnouncement } from "./byokannouncement"; +import { BYOKAnnouncement } from "./byokannouncement";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/aipanel/aipanel.tsx` at line 29, The import line importing BYOKAnnouncement from "./byokannouncement" contains trailing whitespace (a tab) at the end; remove the trailing whitespace character so the line reads exactly: import { BYOKAnnouncement } from "./byokannouncement"; and run the project's formatter or linter to ensure no other trailing whitespace remains.cmd/wsh/cmd/wshcmd-setbg.go (1)
127-132: Consider removing duplicate border color assignment.Border color metadata is set twice: once inside the no-args branch (lines 127-132) and again unconditionally after both branches (lines 198-203). Since lines 198-203 always execute, the assignment at lines 127-132 is redundant.
♻️ Proposed fix to remove duplication
if setBgClear { meta["bg:*"] = true } else if cmd.Flags().Changed("opacity") { meta["bg:opacity"] = setBgOpacity } - if borderColorChanged { - meta["bg:bordercolor"] = setBgBorderColor - } - if activeBorderColorChanged { - meta["bg:activebordercolor"] = setBgActiveBorderColor - } } else if len(args) > 1 {Also applies to: 198-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/wsh/cmd/wshcmd-setbg.go` around lines 127 - 132, In the no-args branch of the set-bg command you currently assign meta["bg:bordercolor"] = setBgBorderColor and meta["bg:activebordercolor"] = setBgActiveBorderColor guarded by borderColorChanged and activeBorderColorChanged, but those same assignments are always applied later unconditionally; remove the guarded assignments in the no-args branch (references: meta map, setBgBorderColor, setBgActiveBorderColor, borderColorChanged, activeBorderColorChanged, function handling the no-args set-bg path) so the metadata is only set once by the later unconditional block.frontend/types/gotypes.d.ts (1)
111-120: Minor documentation/type discrepancy:display:nameoptionality.The documentation states that
display:nameis "required" for background entries, but the TypeScript type defines it as optional ("display:name"?: string). Since this is a generated file from Go types, this may be intentional to allow flexibility during migration or for internal use. Just flagging for awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/types/gotypes.d.ts` around lines 111 - 120, The type for BackgroundConfigType declares "display:name" as optional but the docs state it is required; update the generated Go->TS mapping so BackgroundConfigType's "display:name" property is non-optional (remove the optional marker for "display:name") to match documentation, ensuring the generator that produces this file (or the Go struct tags) is adjusted accordingly; reference the BackgroundConfigType type and the "display:name" property when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/aipanel/aipanel.tsx`:
- Line 268: The caller passes tabModel?.tabId which can be undefined but
useTabBackground currently types tabId as string | null; update useTabBackground
to explicitly handle undefined or accept it in the signature: change the
parameter type to tabId: string | null | undefined OR add an early guard in
useTabBackground that maps undefined -> null (or returns safe defaults) before
calling waveEnv.getTabMetaKeyAtom; reference useTabBackground, useTabModelMaybe
(source of tabModel?.tabId), and waveEnv.getTabMetaKeyAtom to ensure atoms are
always invoked with a value the implementation expects and avoid relying on
implicit conversion via makeORef.
---
Nitpick comments:
In `@cmd/generateschema/main-generateschema.go`:
- Around line 188-192: The log message "bg presets schema error" is inconsistent
with the types and filename; update the error message in the generateSchema call
handling to reference BackgroundConfigType/WaveSchemaBackgroundsFileName (e.g.,
"backgrounds schema error" or "backgrounds.json schema error") so it matches
bgPresetsTemplate, BackgroundConfigType and WaveSchemaBackgroundsFileName;
locate the block using bgPresetsTemplate and generateSchema and replace the
log.Fatalf message accordingly.
In `@cmd/wsh/cmd/wshcmd-setbg.go`:
- Around line 127-132: In the no-args branch of the set-bg command you currently
assign meta["bg:bordercolor"] = setBgBorderColor and
meta["bg:activebordercolor"] = setBgActiveBorderColor guarded by
borderColorChanged and activeBorderColorChanged, but those same assignments are
always applied later unconditionally; remove the guarded assignments in the
no-args branch (references: meta map, setBgBorderColor, setBgActiveBorderColor,
borderColorChanged, activeBorderColorChanged, function handling the no-args
set-bg path) so the metadata is only set once by the later unconditional block.
In `@docs/src/components/versionbadge.css`:
- Around line 24-41: The .deprecated-badge rules duplicate many properties from
.version-badge; refactor by extracting shared declarations (display, padding,
margin-left, font-size, font-weight, line-height, border-radius, vertical-align,
white-space) into a common base class (e.g., .badge or .base-badge) and have
.version-badge and .deprecated-badge extend/override only the variant-specific
properties like background-color and color (including the [data-theme="dark"]
overrides) to reduce duplication and make future badge variants easier to add.
In `@frontend/app/aipanel/aipanel.tsx`:
- Line 29: The import line importing BYOKAnnouncement from "./byokannouncement"
contains trailing whitespace (a tab) at the end; remove the trailing whitespace
character so the line reads exactly: import { BYOKAnnouncement } from
"./byokannouncement"; and run the project's formatter or linter to ensure no
other trailing whitespace remains.
In `@frontend/types/gotypes.d.ts`:
- Around line 111-120: The type for BackgroundConfigType declares "display:name"
as optional but the docs state it is required; update the generated Go->TS
mapping so BackgroundConfigType's "display:name" property is non-optional
(remove the optional marker for "display:name") to match documentation, ensuring
the generator that produces this file (or the Go struct tags) is adjusted
accordingly; reference the BackgroundConfigType type and the "display:name"
property when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 588e5daf-017e-4e49-9bce-2fc87f4c12ef
📒 Files selected for processing (40)
.kilocode/skills/waveenv/SKILL.mdcmd/generateschema/main-generateschema.gocmd/server/main-server.gocmd/wsh/cmd/wshcmd-setbg.godocs/docs/config.mdxdocs/docs/customization.mdxdocs/docs/releasenotes.mdxdocs/docs/tab-backgrounds.mdxdocs/docs/wsh-reference.mdxdocs/src/components/versionbadge.cssdocs/src/components/versionbadge.tsxfrontend/app/aipanel/aipanel.tsxfrontend/app/app-bg.tsxfrontend/app/block/blockenv.tsfrontend/app/block/blockframe.tsxfrontend/app/block/blockutil.tsxfrontend/app/monaco/schemaendpoints.tsfrontend/app/store/global.tsfrontend/app/tab/tabcontextmenu.tsfrontend/app/view/sysinfo/sysinfo.tsxfrontend/app/view/term/fitaddon.tsfrontend/app/view/waveconfig/waveconfig-model.tsfrontend/app/view/waveconfig/waveconfig.tsxfrontend/app/view/waveconfig/waveconfigenv.tsfrontend/app/view/webview/webviewenv.tsfrontend/app/waveenv/waveenv.tsfrontend/app/waveenv/waveenvimpl.tsfrontend/preview/mock/defaultconfig.tsfrontend/preview/mock/mockwaveenv.tsfrontend/types/gotypes.d.tspkg/waveobj/metaconsts.gopkg/waveobj/wtypemeta.gopkg/wconfig/defaultconfig/backgrounds.jsonpkg/wconfig/defaultconfig/presets.jsonpkg/wconfig/metaconsts.gopkg/wconfig/settingsconfig.gopkg/wcore/workspace.goschema/backgrounds.jsonschema/settings.jsontsconfig.json
| const telemetryEnabled = jotai.useAtomValue(getSettingsKeyAtom("telemetry:enabled")) ?? false; | ||
| const isPanelVisible = jotai.useAtomValue(model.getPanelVisibleAtom()); | ||
| const tabModel = useTabModelMaybe(); | ||
| const [tabBorderColor, tabActiveBorderColor] = useTabBackground(waveEnv, tabModel?.tabId); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check useTabBackground implementation for undefined tabId handling
# Find and display the useTabBackground hook implementation
ast-grep --pattern $'function useTabBackground($_, $_) {
$$$
}'
# Also search with export pattern
rg -nA 20 'export function useTabBackground'Repository: wavetermdev/waveterm
Length of output: 1974
🏁 Script executed:
#!/bin/bash
# Check how getTabMetaKeyAtom and getConfigBackgroundAtom handle null/undefined
rg -nA 10 'getTabMetaKeyAtom' frontend/app/block/blockutil.tsxRepository: wavetermdev/waveterm
Length of output: 1412
🏁 Script executed:
#!/bin/bash
# Find the TabBackgroundEnv type definition to understand the atom methods
rg -nB 3 'getTabMetaKeyAtom|getConfigBackgroundAtom' frontend/app/block/blockutil.tsx | head -50Repository: wavetermdev/waveterm
Length of output: 851
🏁 Script executed:
#!/bin/bash
# Check the actual usage at aipanel.tsx line 268
sed -n '265,275p' frontend/app/aipanel/aipanel.tsxRepository: wavetermdev/waveterm
Length of output: 796
🏁 Script executed:
#!/bin/bash
# Search for MetaKeyAtomFnType definition
rg -nB 5 'type MetaKeyAtomFnType' --type typescriptRepository: wavetermdev/waveterm
Length of output: 98
🏁 Script executed:
#!/bin/bash
# Search for WaveEnv type definition and getTabMetaKeyAtom
rg -nA 15 'export type WaveEnv' | head -80Repository: wavetermdev/waveterm
Length of output: 1998
🏁 Script executed:
#!/bin/bash
# Look for getTabMetaKeyAtom implementation
rg -n 'getTabMetaKeyAtom.*=' --type typescript | head -20Repository: wavetermdev/waveterm
Length of output: 98
🏁 Script executed:
#!/bin/bash
# Search for MetaKeyAtomFnType definition
rg -n 'MetaKeyAtomFnType' frontend/Repository: wavetermdev/waveterm
Length of output: 1779
🏁 Script executed:
#!/bin/bash
# Search for getTabMetaKeyAtom implementation
rg -n 'getTabMetaKeyAtom' frontend/app/waveenv/Repository: wavetermdev/waveterm
Length of output: 268
🏁 Script executed:
#!/bin/bash
# Check if useTabModelMaybe can return undefined tabId
rg -nA 10 'useTabModelMaybe'Repository: wavetermdev/waveterm
Length of output: 2318
🏁 Script executed:
#!/bin/bash
# Get the full MetaKeyAtomFnType definition
sed -n '9,15p' frontend/app/waveenv/waveenv.tsRepository: wavetermdev/waveterm
Length of output: 343
🏁 Script executed:
#!/bin/bash
# Check TabModel type definition
rg -nA 20 'export type TabModel' frontend/app/store/tab-model.ts | head -40Repository: wavetermdev/waveterm
Length of output: 866
🏁 Script executed:
#!/bin/bash
# Check the getTabMetaKeyAtom implementation in waveenvimpl.ts
rg -nB 5 -A 20 'getTabMetaKeyAtom' frontend/app/waveenv/waveenvimpl.ts | head -50Repository: wavetermdev/waveterm
Length of output: 1420
🏁 Script executed:
#!/bin/bash
# Find the getTabMetaKeyAtom implementation in global store
rg -n 'export.*getTabMetaKeyAtom' frontend/app/store/Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for getTabMetaKeyAtom definition
rg -nB 3 -A 10 'const getTabMetaKeyAtom' frontend/Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check what happens when null/undefined is passed to getTabMetaKeyAtom
# Look for its implementation in global.ts
rg -nB 5 -A 15 'getTabMetaKeyAtom.*=' frontend/app/store/global.tsRepository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
#!/bin/bash
# List all files in the store directory
ls -la frontend/app/store/Repository: wavetermdev/waveterm
Length of output: 1868
🏁 Script executed:
#!/bin/bash
# Search for getTabMetaKeyAtom more broadly
rg 'getTabMetaKeyAtom' frontend/app/store/ -lRepository: wavetermdev/waveterm
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Try searching in global.ts directly
head -100 frontend/app/store/global.ts | tail -80Repository: wavetermdev/waveterm
Length of output: 2809
🏁 Script executed:
#!/bin/bash
# Search for getTabMetaKeyAtom in global.ts
rg -n 'getTabMetaKeyAtom' frontend/app/store/global.tsRepository: wavetermdev/waveterm
Length of output: 191
🏁 Script executed:
#!/bin/bash
# Get the content around getTabMetaKeyAtom in global.ts
rg -n -B 3 -A 15 'export const getTabMetaKeyAtom' frontend/app/store/global.tsRepository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
#!/bin/bash
# If not found, search in global-atoms.ts
rg -n -B 3 -A 15 'getTabMetaKeyAtom' frontend/app/store/global-atoms.tsRepository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Get the full implementation of getTabMetaKeyAtom
sed -n '135,155p' frontend/app/store/global.tsRepository: wavetermdev/waveterm
Length of output: 849
🏁 Script executed:
#!/bin/bash
# Check how orefAtomCache works - is it null/undefined safe?
rg -n -B 5 -A 10 'orefAtomCache' frontend/app/store/global-atoms.ts | head -50Repository: wavetermdev/waveterm
Length of output: 1131
🏁 Script executed:
#!/bin/bash
# Check WOS.makeORef implementation
rg -n -A 5 'function makeORef\|export.*makeORef' frontend/app/store/wos.tsRepository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check if undefined gets converted to string somewhere
sed -n '1,50p' frontend/app/store/wos.tsRepository: wavetermdev/waveterm
Length of output: 1422
🏁 Script executed:
#!/bin/bash
# Search for makeORef in wos.ts
rg -n 'makeORef' frontend/app/store/wos.tsRepository: wavetermdev/waveterm
Length of output: 252
🏁 Script executed:
#!/bin/bash
# Get more context around the oref building
sed -n '1,150p' frontend/app/store/wos.ts | tail -100Repository: wavetermdev/waveterm
Length of output: 3566
🏁 Script executed:
#!/bin/bash
# Check getSingleOrefAtomCache implementation
rg -n -A 10 'function getSingleOrefAtomCache' frontend/app/store/global.tsRepository: wavetermdev/waveterm
Length of output: 575
🏁 Script executed:
#!/bin/bash
# Check if getWaveObjectAtom handles null oref safely
rg -n -B 3 -A 10 'export function getWaveObjectAtom\|export const getWaveObjectAtom' frontend/app/store/wos.tsRepository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check the actual return values from useTabBackground - what happens when things are undefined
sed -n '169,180p' frontend/app/block/blockutil.tsxRepository: wavetermdev/waveterm
Length of output: 833
🏁 Script executed:
#!/bin/bash
# Check if TabModel.tabId is defined
rg -n -A 5 'class TabModel' frontend/app/store/tab-model.tsRepository: wavetermdev/waveterm
Length of output: 272
🏁 Script executed:
#!/bin/bash
# Check useTabModelMaybe return type
sed -n '80,95p' frontend/app/store/tab-model.tsRepository: wavetermdev/waveterm
Length of output: 382
🏁 Script executed:
#!/bin/bash
# Look for any TypeScript compilation or type checking that might catch this
find . -name 'tsconfig.json' -o -name '.eslintrc*' | head -5Repository: wavetermdev/waveterm
Length of output: 133
🏁 Script executed:
#!/bin/bash
# Check the actual type of useContext return and whether it can be null/undefined
rg -n 'TabModelContext' frontend/app/store/tab-model.tsRepository: wavetermdev/waveterm
Length of output: 241
🏁 Script executed:
#!/bin/bash
# Check what type useContext returns in this case
sed -n '1,20p' frontend/app/store/tab-model.tsRepository: wavetermdev/waveterm
Length of output: 674
🏁 Script executed:
#!/bin/bash
# Search for getWaveObjectAtom implementation in wos.ts
rg -n 'export.*getWaveObjectAtom\|function getWaveObjectAtom' frontend/app/store/wos.tsRepository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Get more context on the waveObjectValueCache and how it handles null orefs
sed -n '100,200p' frontend/app/store/wos.ts | grep -A 20 'getWaveObjectAtom'Repository: wavetermdev/waveterm
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Search for getWaveObjectAtom in all files
rg -n 'getWaveObjectAtom' frontend/app/store/Repository: wavetermdev/waveterm
Length of output: 1642
🏁 Script executed:
#!/bin/bash
# Look for export statements in wos.ts
grep -n 'export' frontend/app/store/wos.ts | head -30Repository: wavetermdev/waveterm
Length of output: 76
🏁 Script executed:
#!/bin/bash
# Get the full implementation of getWaveObjectAtom
sed -n '213,240p' frontend/app/store/wos.tsRepository: wavetermdev/waveterm
Length of output: 1050
🏁 Script executed:
#!/bin/bash
# Check what happens when makeORef returns null and how it's used
sed -n '51,60p' frontend/app/store/wos.tsRepository: wavetermdev/waveterm
Length of output: 361
Add explicit undefined handling or update type signature for useTabBackground.
The function signature declares tabId: string | null, but the caller at line 268 passes tabModel?.tabId which can be undefined (since useTabModelMaybe() returns TabModel | undefined from TabModelContext). While the code happens to work because makeORef converts undefined to null, this relies on an implicit chain rather than explicit handling.
Either:
- Update the type signature to accept
string | null | undefined - Add an explicit null/undefined guard in
useTabBackgroundto return safe defaults early
Current implementation
export function useTabBackground(
waveEnv: TabBackgroundEnv,
tabId: string | null
): [string, string, BackgroundConfigType] {
const tabActiveBorderColorDirect = jotai.useAtomValue(waveEnv.getTabMetaKeyAtom(tabId, "bg:activebordercolor"));
const tabBorderColorDirect = jotai.useAtomValue(waveEnv.getTabMetaKeyAtom(tabId, "bg:bordercolor"));
const tabBg = jotai.useAtomValue(waveEnv.getTabMetaKeyAtom(tabId, "tab:background"));
const configBg = jotai.useAtomValue(waveEnv.getConfigBackgroundAtom(tabBg));
const tabActiveBorderColor = tabActiveBorderColorDirect ?? configBg?.["bg:activebordercolor"];
const tabBorderColor = tabBorderColorDirect ?? configBg?.["bg:bordercolor"];
return [tabBorderColor, tabActiveBorderColor, configBg];
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/aipanel/aipanel.tsx` at line 268, The caller passes
tabModel?.tabId which can be undefined but useTabBackground currently types
tabId as string | null; update useTabBackground to explicitly handle undefined
or accept it in the signature: change the parameter type to tabId: string | null
| undefined OR add an early guard in useTabBackground that maps undefined ->
null (or returns safe defaults) before calling waveEnv.getTabMetaKeyAtom;
reference useTabBackground, useTabModelMaybe (source of tabModel?.tabId), and
waveEnv.getTabMetaKeyAtom to ensure atoms are always invoked with a value the
implementation expects and avoid relying on implicit conversion via makeORef.
Deploying waveterm with
|
| Latest commit: |
3150e03
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://da81fb7b.waveterm.pages.dev |
| Branch Preview URL: | https://sawka-no-config-dirs.waveterm.pages.dev |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/wsh/cmd/wshcmd-setbg.go (1)
133-133: Use defined constants for metadata keys instead of raw strings.The codebase defines constants for these metadata keys in
pkg/waveobj/metaconsts.go:
MetaKey_TabBackground = "tab:background"MetaKey_BgBorderColor = "bg:bordercolor"MetaKey_BgActiveBorderColor = "bg:activebordercolor"Using raw string literals instead of these constants reduces maintainability and risks inconsistency if keys are renamed.
♻️ Suggested refactor to use constants
// Handle background setting meta["bg:*"] = true - meta["tab:background"] = nil + meta[waveobj.MetaKey_TabBackground] = nil if setBgOpacity < 0 || setBgOpacity > 1 {if borderColorChanged { - meta["bg:bordercolor"] = setBgBorderColor + meta[waveobj.MetaKey_BgBorderColor] = setBgBorderColor } if activeBorderColorChanged { - meta["bg:activebordercolor"] = setBgActiveBorderColor + meta[waveobj.MetaKey_BgActiveBorderColor] = setBgActiveBorderColor }You'll need to add an import for the
waveobjpackage.Also applies to: 192-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/wsh/cmd/wshcmd-setbg.go` at line 133, Replace raw metadata key string literals with the defined constants from the waveobj package: use waveobj.MetaKey_TabBackground instead of "tab:background" (the occurrence at meta["tab:background"]) and likewise replace "bg:bordercolor" and "bg:activebordercolor" with waveobj.MetaKey_BgBorderColor and waveobj.MetaKey_BgActiveBorderColor (the occurrences around lines 192–197). Add the waveobj import to the file and update all meta[...] accesses to use these constants to ensure consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/wsh/cmd/wshcmd-setbg.go`:
- Line 133: Replace raw metadata key string literals with the defined constants
from the waveobj package: use waveobj.MetaKey_TabBackground instead of
"tab:background" (the occurrence at meta["tab:background"]) and likewise replace
"bg:bordercolor" and "bg:activebordercolor" with waveobj.MetaKey_BgBorderColor
and waveobj.MetaKey_BgActiveBorderColor (the occurrences around lines 192–197).
Add the waveobj import to the file and update all meta[...] accesses to use
these constants to ensure consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 47ee54fa-5a3e-4814-9358-0374ae83cca6
📒 Files selected for processing (6)
cmd/generateschema/main-generateschema.gocmd/wsh/cmd/wshcmd-setbg.gofrontend/app/aipanel/aipanel.tsxfrontend/types/gotypes.d.tspkg/wconfig/settingsconfig.goschema/backgrounds.json
🚧 Files skipped from review as they are similar to previous changes (3)
- schema/backgrounds.json
- frontend/types/gotypes.d.ts
- cmd/generateschema/main-generateschema.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/wconfig/settingsconfig.go (1)
896-937: Migration logic looks solid with proper error handling.The function correctly:
- Performs idempotent migration (only when
backgrounds.jsondoesn't exist)- Preserves original JSON structure using
json.RawMessage- Uses atomic file writes to prevent corruption
- Handles various error cases with appropriate logging
Minor observation: The migrated file uses 4-space indentation (line 927) while
jsonMarshalConfigInOrderuses 2-space indentation. Consider aligning indentation style across config files for consistency.🔧 Optional: Align indentation style
- outBarr, err := json.MarshalIndent(filtered, "", " ") + outBarr, err := json.MarshalIndent(filtered, "", " ")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/wconfig/settingsconfig.go` around lines 896 - 937, MigratePresetsBackgrounds currently marshals the migrated JSON with 4-space indentation (json.MarshalIndent(filtered, "", " ")), which differs from the 2-space indentation used elsewhere (e.g., jsonMarshalConfigInOrder); update the MarshalIndent call in MigratePresetsBackgrounds to use a 2-space indent string (e.g., " ") so backgrounds.json matches the project's config formatting conventions and then write that output with fileutil.AtomicWriteFile as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/wconfig/settingsconfig.go`:
- Around line 896-937: MigratePresetsBackgrounds currently marshals the migrated
JSON with 4-space indentation (json.MarshalIndent(filtered, "", " ")), which
differs from the 2-space indentation used elsewhere (e.g.,
jsonMarshalConfigInOrder); update the MarshalIndent call in
MigratePresetsBackgrounds to use a 2-space indent string (e.g., " ") so
backgrounds.json matches the project's config formatting conventions and then
write that output with fileutil.AtomicWriteFile as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1eeb2ff1-769e-4977-a657-93567eb038f4
📒 Files selected for processing (3)
frontend/app/app-bg.tsxfrontend/util/waveutil.tspkg/wconfig/settingsconfig.go
✅ Files skipped from review due to trivial changes (1)
- frontend/app/app-bg.tsx
1 participant
also fixes aipanel's border colors