Workspace theme independent from document themes#59
Workspace theme independent from document themes#59sushilbang wants to merge 2 commits intosohzm:masterfrom
Conversation
|
@sushilbang is attempting to deploy a commit to the sz47's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR implements workspace-level theme persistence by introducing methods to store and retrieve workspace-specific themes in localStorage, updating the editor to use workspace theme as a fallback when document theme is not explicitly set, and extending the sidebar to sync theme changes across the workspace. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Sidebar as Home Sidebar
participant Theme as Theme Manager
participant Storage as localStorage
participant Editor as Editor
User->>Sidebar: Change theme selector
Sidebar->>Theme: setWorkspaceTheme(themeName)
Theme->>Storage: Read/update workspaces + persist
Theme->>Sidebar: Emit wisk-theme-changed event
Sidebar->>Sidebar: Update selector
rect rgb(230, 245, 250)
Note over Editor,Storage: Editor Initialization
Editor->>Theme: getWorkspaceTheme()
Theme->>Storage: Read current workspace theme
Theme-->>Editor: Return workspace theme
Editor->>Editor: Compute themeToApply<br/>(doc theme or workspace fallback)
Editor->>Theme: setTheme(themeToApply)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
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: 2
🤖 Fix all issues with AI agents
In @js/theme/theme.js:
- Around line 121-133: The function wisk.theme.setWorkspaceTheme should guard
against corrupted localStorage and bad inputs: wrap
JSON.parse(localStorage.getItem('workspaces')) in a try/catch and handle parse
errors (log/return) instead of throwing; validate that parsed &&
Array.isArray(parsed.workspaces) before accessing .findIndex; validate themeName
(e.g., ensure typeof themeName === 'string' and not empty, or check against an
allowed themes list if available) and return/log on invalid input; also handle
missing currentWorkspace or workspaceIndex === -1 by logging/returning rather
than attempting to set parsed.workspaces[workspaceIndex].theme; finally only
call localStorage.setItem when all checks pass.
- Around line 110-119: The getWorkspaceTheme function lacks JSON parse error
handling and structure validation; wrap JSON.parse of
localStorage.getItem('workspaces') in a try/catch, return null on parse errors
(and optionally log the error), then ensure the parsed value has a workspaces
array before calling parsed.workspaces.find; also validate currentWorkspaceId
exists before searching and safely handle the case where find returns undefined
so the function returns null rather than throwing (refer to
wisk.theme.getWorkspaceTheme and variables workspacesData, parsed,
currentWorkspaceId, workspace).
🧹 Nitpick comments (1)
js/editor.js (1)
547-551: Consider simplifying the theme fallback logic for clarity.The current nested ternary works correctly but is somewhat difficult to parse. The logic chain is: document theme → workspace theme → 'default'. Consider refactoring for readability.
♻️ Proposed refactor for clarity
- // Use workspace theme as fallback if document theme is 'default' or empty - const docTheme = doc.data.config.theme; - const workspaceTheme = wisk.theme.getWorkspaceTheme(); - const themeToApply = (!docTheme || docTheme === 'default') ? (workspaceTheme || docTheme) : docTheme; - wisk.theme.setTheme(themeToApply); + // Apply theme with fallback: document theme → workspace theme → 'default' + const docTheme = doc.data.config.theme; + let themeToApply = docTheme; + + // If document has no explicit theme, fall back to workspace theme + if (!docTheme || docTheme === 'default') { + const workspaceTheme = wisk.theme.getWorkspaceTheme(); + themeToApply = workspaceTheme || docTheme || 'default'; + } + + wisk.theme.setTheme(themeToApply);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
js/editor.jsjs/scripts/home-sidebar.jsjs/theme/theme.js
🧰 Additional context used
🧬 Code graph analysis (2)
js/scripts/home-sidebar.js (3)
js/wisk.js (1)
wisk(3-3)js/theme/theme.js (1)
workspace(116-116)js/storage/db.js (1)
workspace(247-247)
js/editor.js (1)
js/wisk.js (1)
wisk(3-3)
🔇 Additional comments (4)
js/scripts/home-sidebar.js (4)
79-80: LGTM - Workspace theme persistence.The call to
setWorkspaceThemecorrectly persists the theme change to the workspace after applying it. The order is appropriate: first apply the theme withsetTheme, then persist it withsetWorkspaceTheme.
82-85: LGTM - Theme change synchronization.The event listener keeps the theme selector UI in sync when the theme changes from other sources. This ensures the dropdown always reflects the current theme.
310-310: LGTM - New workspace inherits current theme.New workspaces correctly inherit the current theme with a sensible 'Light' fallback. This ensures a consistent user experience.
123-126: No conflict exists between home sidebar and editor theme application—they operate in separate execution contexts.The home sidebar theme applies only on the home page view (when no document ID is present), while the editor theme applies when loading a specific document. These are distinct code paths that never execute together.
Likely an incorrect or invalid review comment.
| wisk.theme.getWorkspaceTheme = function () { | ||
| const workspacesData = localStorage.getItem('workspaces'); | ||
| if (!workspacesData) return null; | ||
|
|
||
| const parsed = JSON.parse(workspacesData); | ||
| const currentWorkspaceId = localStorage.getItem('currentWorkspace'); | ||
| const workspace = parsed.workspaces.find(w => w.id === currentWorkspaceId); | ||
|
|
||
| return workspace?.theme || null; | ||
| }; |
There was a problem hiding this comment.
Add error handling for JSON parsing and data structure validation.
The function lacks error handling for JSON.parse() and doesn't validate that parsed.workspaces exists before accessing it. If localStorage data is corrupted or has an unexpected structure, this will throw a runtime error.
🛡️ Proposed fix with error handling
wisk.theme.getWorkspaceTheme = function () {
const workspacesData = localStorage.getItem('workspaces');
if (!workspacesData) return null;
+ try {
const parsed = JSON.parse(workspacesData);
+ if (!parsed.workspaces || !Array.isArray(parsed.workspaces)) return null;
+
const currentWorkspaceId = localStorage.getItem('currentWorkspace');
const workspace = parsed.workspaces.find(w => w.id === currentWorkspaceId);
return workspace?.theme || null;
+ } catch (error) {
+ console.error('Failed to parse workspace data:', error);
+ return null;
+ }
};🤖 Prompt for AI Agents
In @js/theme/theme.js around lines 110 - 119, The getWorkspaceTheme function
lacks JSON parse error handling and structure validation; wrap JSON.parse of
localStorage.getItem('workspaces') in a try/catch, return null on parse errors
(and optionally log the error), then ensure the parsed value has a workspaces
array before calling parsed.workspaces.find; also validate currentWorkspaceId
exists before searching and safely handle the case where find returns undefined
so the function returns null rather than throwing (refer to
wisk.theme.getWorkspaceTheme and variables workspacesData, parsed,
currentWorkspaceId, workspace).
| wisk.theme.setWorkspaceTheme = function (themeName) { | ||
| const workspacesData = localStorage.getItem('workspaces'); | ||
| if (!workspacesData) return; | ||
|
|
||
| const parsed = JSON.parse(workspacesData); | ||
| const currentWorkspaceId = localStorage.getItem('currentWorkspace'); | ||
| const workspaceIndex = parsed.workspaces.findIndex(w => w.id === currentWorkspaceId); | ||
|
|
||
| if (workspaceIndex !== -1) { | ||
| parsed.workspaces[workspaceIndex].theme = themeName; | ||
| localStorage.setItem('workspaces', JSON.stringify(parsed)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add error handling and input validation.
The function needs:
- Error handling for
JSON.parse()to prevent runtime errors from corrupted data - Validation that
parsed.workspacesexists and is an array - Optional: validation of the
themeNameparameter to ensure it's a valid theme
🛡️ Proposed fix with error handling and validation
wisk.theme.setWorkspaceTheme = function (themeName) {
+ if (!themeName || typeof themeName !== 'string') {
+ console.error('Invalid theme name provided');
+ return;
+ }
+
const workspacesData = localStorage.getItem('workspaces');
if (!workspacesData) return;
+ try {
const parsed = JSON.parse(workspacesData);
+ if (!parsed.workspaces || !Array.isArray(parsed.workspaces)) {
+ console.error('Invalid workspaces data structure');
+ return;
+ }
+
const currentWorkspaceId = localStorage.getItem('currentWorkspace');
const workspaceIndex = parsed.workspaces.findIndex(w => w.id === currentWorkspaceId);
if (workspaceIndex !== -1) {
parsed.workspaces[workspaceIndex].theme = themeName;
localStorage.setItem('workspaces', JSON.stringify(parsed));
}
+ } catch (error) {
+ console.error('Failed to update workspace theme:', error);
+ }
};📝 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.
| wisk.theme.setWorkspaceTheme = function (themeName) { | |
| const workspacesData = localStorage.getItem('workspaces'); | |
| if (!workspacesData) return; | |
| const parsed = JSON.parse(workspacesData); | |
| const currentWorkspaceId = localStorage.getItem('currentWorkspace'); | |
| const workspaceIndex = parsed.workspaces.findIndex(w => w.id === currentWorkspaceId); | |
| if (workspaceIndex !== -1) { | |
| parsed.workspaces[workspaceIndex].theme = themeName; | |
| localStorage.setItem('workspaces', JSON.stringify(parsed)); | |
| } | |
| }; | |
| wisk.theme.setWorkspaceTheme = function (themeName) { | |
| if (!themeName || typeof themeName !== 'string') { | |
| console.error('Invalid theme name provided'); | |
| return; | |
| } | |
| const workspacesData = localStorage.getItem('workspaces'); | |
| if (!workspacesData) return; | |
| try { | |
| const parsed = JSON.parse(workspacesData); | |
| if (!parsed.workspaces || !Array.isArray(parsed.workspaces)) { | |
| console.error('Invalid workspaces data structure'); | |
| return; | |
| } | |
| const currentWorkspaceId = localStorage.getItem('currentWorkspace'); | |
| const workspaceIndex = parsed.workspaces.findIndex(w => w.id === currentWorkspaceId); | |
| if (workspaceIndex !== -1) { | |
| parsed.workspaces[workspaceIndex].theme = themeName; | |
| localStorage.setItem('workspaces', JSON.stringify(parsed)); | |
| } | |
| } catch (error) { | |
| console.error('Failed to update workspace theme:', error); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In @js/theme/theme.js around lines 121 - 133, The function
wisk.theme.setWorkspaceTheme should guard against corrupted localStorage and bad
inputs: wrap JSON.parse(localStorage.getItem('workspaces')) in a try/catch and
handle parse errors (log/return) instead of throwing; validate that parsed &&
Array.isArray(parsed.workspaces) before accessing .findIndex; validate themeName
(e.g., ensure typeof themeName === 'string' and not empty, or check against an
allowed themes list if available) and return/log on invalid input; also handle
missing currentWorkspace or workspaceIndex === -1 by logging/returning rather
than attempting to set parsed.workspaces[workspaceIndex].theme; finally only
call localStorage.setItem when all checks pass.
1 participant
Summary
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.