feat: add configurable sidebar width in settings#1975
feat: add configurable sidebar width in settings#1975soumik183 wants to merge 1 commit intoAcode-Foundation:mainfrom
Conversation
github-actions
bot
added
enhancement
Greptile SummaryThis PR adds a user-configurable sidebar width setting, persisting the value in Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant SettingsUI as Settings UI (appSettings.js)
participant AppSettings as appSettings (lib/settings.js)
participant Sidebar as Sidebar (index.js)
User->>SettingsUI: Enter new sidebar width value
SettingsUI->>SettingsUI: validate (200 ≤ value ≤ 1500)
SettingsUI->>AppSettings: update({ sidebarWidth: value })
AppSettings->>AppSettings: persist to settings.json
AppSettings-->>Sidebar: emit "update:sidebarWidth"
Sidebar->>Sidebar: width = Math.min(sidebarWidth, MAX_WIDTH())
Sidebar->>Sidebar: setWidth(width)
User->>Sidebar: Drag resize bar
Sidebar->>Sidebar: resize(deltaX) → setWidth(newWidth)
Sidebar->>AppSettings: value.sidebarWidth = width (direct mutation)
Sidebar->>AppSettings: update(false)
AppSettings->>AppSettings: #getChangedKeys detects mutation
AppSettings-->>Sidebar: emit "update:sidebarWidth" (redundant)
Sidebar->>Sidebar: setWidth(width) called again (no-op)
Reviews (1): Last reviewed commit: "feat: add configurable sidebar width in ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| appSettings.on("update:sidebarWidth", () => { | ||
| width = Math.min(appSettings.value.sidebarWidth || MIN_WIDTH, MAX_WIDTH()); | ||
| setWidth(width); | ||
| }); |
There was a problem hiding this comment.
appSettings.on("update:sidebarWidth", ...) is registered inside create() but there is no corresponding appSettings.off(...) call when the sidebar element is destroyed (e.g. after hideMaster removes it from the DOM, or if create is ever called more than once in the singleton pattern).
Each call to create() accumulates another live listener on the global appSettings object. Over time those stale callbacks will call setWidth on detached elements and keep closed-over DOM nodes alive. Store the callback reference and deregister it in a cleanup path (the existing hide/hideMaster flow would be a good place):
const onSidebarWidthUpdate = () => {
width = Math.min(appSettings.value.sidebarWidth || MIN_WIDTH, MAX_WIDTH());
setWidth(width);
};
appSettings.on("update:sidebarWidth", onSidebarWidthUpdate);
// … later, when the sidebar is destroyed:
// appSettings.off("update:sidebarWidth", onSidebarWidthUpdate);| appSettings.value.sidebarWidth = width; | ||
| appSettings.update(false); |
There was a problem hiding this comment.
setWidth call
Setting appSettings.value.sidebarWidth = width directly mutates the settings object, and then appSettings.update(false) detects it as a change through #getChangedKeys(). This causes the update:sidebarWidth listener (registered at line 97) to fire, which calls setWidth(width) a second time with the exact value that was just applied.
Use the idiomatic update API instead to avoid the extra round-trip and make the intent clear:
| appSettings.value.sidebarWidth = width; | |
| appSettings.update(false); | |
| appSettings.update({ sidebarWidth: width }, false); |
| promptOptions: { | ||
| test(value) { | ||
| value = Number.parseInt(value); | ||
| return value >= 200 && value <= 1500; | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The promptOptions.test allows values up to 1500 px, but at runtime the sidebar is clamped to innerWidth - 40. On a typical mobile device (e.g. 400 px wide) the ceiling is ~360 px — far below 1500. A user who enters, say, 800 px will pass validation, the value 800 will be persisted to settings, but the sidebar will silently render at 360 px. The mismatch between the stored value and the rendered width will be confusing.
Consider deriving the max from screen dimensions at validation time, or at least document the runtime clamping in the info string so users understand why the displayed width may differ from what they entered:
| promptOptions: { | |
| test(value) { | |
| value = Number.parseInt(value); | |
| return value >= 200 && value <= 1500; | |
| }, | |
| }, | |
| test(value) { | |
| value = Number.parseInt(value); | |
| const runtimeMax = window.innerWidth - 40; | |
| return value >= 200 && value <= runtimeMax; | |
| }, |
| $el.getWidth = function () { | ||
| const width = innerWidth * 0.7; | ||
| return mode === "phone" ? (width >= 350 ? 350 : width) : MIN_WIDTH; | ||
| const configuredWidth = appSettings.value.sidebarWidth || MIN_WIDTH; | ||
| return mode === "phone" ? Math.min(configuredWidth, MAX_WIDTH()) : width; | ||
| }; |
There was a problem hiding this comment.
getWidth() inconsistency in tab mode
For mode === "phone", getWidth() correctly re-evaluates Math.min(configuredWidth, MAX_WIDTH()). However for tab mode it returns the closed-over width local variable. After a window resize event onWindowResize updates the local innerWidth and hides the sidebar, but it does not re-clamp width. On the next show() call setWidth(width) may use a stale value larger than the new MAX_WIDTH().
For consistency (and to avoid the stale-value issue), tab mode should also be re-clamped:
| $el.getWidth = function () { | |
| const width = innerWidth * 0.7; | |
| return mode === "phone" ? (width >= 350 ? 350 : width) : MIN_WIDTH; | |
| const configuredWidth = appSettings.value.sidebarWidth || MIN_WIDTH; | |
| return mode === "phone" ? Math.min(configuredWidth, MAX_WIDTH()) : width; | |
| }; | |
| $el.getWidth = function () { | |
| const configuredWidth = appSettings.value.sidebarWidth || MIN_WIDTH; | |
| return Math.min(configuredWidth, MAX_WIDTH()); | |
| }; |
| return value >= 200 && value <= 1500; | ||
| }, | ||
| }, | ||
| info: "Adjust the width of the sidebar (min 200px).", |
There was a problem hiding this comment.
info string
Every other settings item in this file uses a localized strings[...] lookup for its info field. This one uses a bare string literal "Adjust the width of the sidebar (min 200px).", which will not be translated when the user changes language. Add a corresponding key to en-us.json and look it up via strings:
| info: "Adjust the width of the sidebar (min 200px).", | |
| info: strings["info-sidebarWidth"] || "Adjust the width of the sidebar (min 200px).", |
1 participant
This PR introduces a configurable sidebar width, allowing users to customize the size of the sidebar via the app settings. It includes dynamic safety clamping to prevent the sidebar from taking up the entire screen on mobile devices.