Conversation
WalkthroughAdds an account refresh feature: UI layout and styles for a refresh button, a preload bridge method, an IPC handler that calls oauth-jagex.writeAccountsToFile, renderer persistence/restore and sorting logic, a fullscreen window toggle, tests, and a package version bump. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Sidebar UI
participant Renderer
participant Preload as Preload Bridge
participant Main as IPC Handler
participant OAuth as oauth-jagex
participant FS as File System
User->>UI: Click refresh button
UI->>Renderer: click handler
Renderer->>Preload: electron.refreshAccounts()
Preload->>Main: invoke 'refresh-accounts'
Main->>FS: check & read accounts.json
FS-->>Main: return accounts array
Main->>OAuth: writeAccountsToFile(sessionId)
OAuth->>FS: fetch/write updated accounts.json
FS-->>Main: return updated accounts
Main-->>Preload: return { success, accounts }
Preload-->>Renderer: resolved accounts
Renderer->>Renderer: sort, update UI, persist/restore selection
Renderer-->>UI: render refreshed accounts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (3)tests/**/*.test.js📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.js📄 CodeRabbit inference engine (AGENTS.md)
Files:
{main.js,preload.js,renderer.js}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (2)tests/refresh-accounts-ipc.test.js (1)
renderer.js (1)
🔇 Additional comments (11)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
35-35: Upgrade axios from 1.7.2 to address CVE-2024-39338 SSRF vulnerability.Axios 1.7.2 contains CVE-2024-39338, a high-severity SSRF vulnerability where path-relative URLs can be treated as protocol-relative URLs, allowing server-side request forgery. Fixes are available in later 1.7.x and 1.8.x releases. Update package.json to a patched version.
🧹 Nitpick comments (3)
libs/ipc-handlers.js (2)
82-93: Consider using async filesystem operations.The handler uses synchronous filesystem operations (fs.existsSync, fs.readFileSync) which can block the main process. Since this is already an async handler, consider using the async equivalents (fs.promises.access, fs.promises.readFile) for better performance.
-const accountsPath = path.join(microbotDir, 'accounts.json'); -if (!fs.existsSync(accountsPath)) { - return { error: 'accounts.json does not exist' }; -} - -let accountsData = []; -try { - const raw = fs.readFileSync(accountsPath, 'utf8'); - accountsData = JSON.parse(raw); -} catch (err) { - log.error('Failed to read accounts.json for refresh:', err.message); - return { error: 'Failed to read accounts.json' }; -} +const accountsPath = path.join(microbotDir, 'accounts.json'); +let accountsData = []; +try { + const raw = await fs.promises.readFile(accountsPath, 'utf8'); + accountsData = JSON.parse(raw); +} catch (err) { + if (err.code === 'ENOENT') { + return { error: 'accounts.json does not exist' }; + } + log.error('Failed to read accounts.json for refresh:', err.message); + return { error: 'Failed to read accounts.json' }; +}
99-104: Document the sessionId sharing assumption.The comment states "assuming all accounts share the same session scope." This is a critical assumption for the refresh logic. Consider adding validation or error handling if accounts have different sessionIds, or document this constraint more explicitly in the function documentation.
tests/refresh-accounts-ipc.test.js (1)
75-93: Consider adding more test coverage.The current tests cover the happy path and missing file case, but could benefit from additional scenarios:
- Empty accounts array (should return error)
- Accounts without sessionId (should return error)
- Verify the correct sessionId is passed to writeAccountsToFile
- Multiple accounts with different sessionIds
Example test for missing sessionId:
test('refresh-accounts errors when sessionId is missing', async () => { const accountsFile = path.join(tempDir, 'accounts.json'); fs.writeFileSync(accountsFile, JSON.stringify([ { accountId: 'acc1', displayName: 'No Session' } ])); const handler = registeredHandlers['refresh-accounts']; const result = await handler(); expect(result.error).toMatch(/No sessionId found/); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
css/styles.css(2 hunks)index.html(1 hunks)libs/ipc-handlers.js(1 hunks)libs/oauth-jagex.js(3 hunks)main.js(1 hunks)package.json(1 hunks)preload.js(1 hunks)renderer.js(8 hunks)tests/refresh-accounts-ipc.test.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
{main.js,preload.js,renderer.js}
📄 CodeRabbit inference engine (AGENTS.md)
Keep Electron entry points at the repo root: main process in main.js, with preload.js and renderer.js bridging UI logic
Files:
main.jspreload.jsrenderer.js
**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.js: Use 4-space indentation, trailing semicolons, and single quotes in JavaScript files
Prefer const/let over var in JavaScript
Prefer async/await over raw Promises in JavaScript
Files:
main.jspreload.jslibs/ipc-handlers.jsrenderer.jslibs/oauth-jagex.jstests/refresh-accounts-ipc.test.js
libs/**
📄 CodeRabbit inference engine (AGENTS.md)
libs/**: Place shared functionality (IPC handlers, utilities) under libs/
Keep Electron IPC handlers isolated within libs/
Files:
libs/ipc-handlers.jslibs/oauth-jagex.js
libs/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Name shared modules in camelCase (e.g., libs/memoryUtils.js)
Files:
libs/ipc-handlers.jslibs/oauth-jagex.js
package.json
📄 CodeRabbit inference engine (AGENTS.md)
package.json: Versioning follows package.json; update it when releasing
When changing auto-update endpoints or build metadata, update build.publish.url in package.json
Files:
package.json
index.html
📄 CodeRabbit inference engine (AGENTS.md)
Keep the main HTML entry at index.html in the project root
Files:
index.html
tests/**/*.test.js
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.test.js: Put tests under tests/ and name them with the .test.js suffix (e.g., feature-name.test.js)
Name test files using kebab or consistent style mirroring feature-name.test.js
Files:
tests/refresh-accounts-ipc.test.js
css/**
📄 CodeRabbit inference engine (AGENTS.md)
Store renderer CSS assets under css/
Files:
css/styles.css
css/**/*.css
📄 CodeRabbit inference engine (AGENTS.md)
Name renderer CSS files in kebab-case (e.g., css/app-shell.css)
Files:
css/styles.css
🧬 Code graph analysis (3)
libs/ipc-handlers.js (3)
libs/oauth-jagex.js (5)
require(1-1)require(7-7)path(5-5)fs(4-4)log(6-6)main.js (8)
require(1-1)require(6-6)require(8-8)require(11-11)require(12-16)path(2-2)fs(3-3)log(7-7)libs/dir-module.js (5)
path(2-2)microbotDir(15-15)fs(84-84)fs(111-111)fs(192-192)
renderer.js (1)
libs/accounts-loader.js (3)
accounts(10-10)accounts(66-66)accounts(180-180)
tests/refresh-accounts-ipc.test.js (3)
libs/oauth-jagex.js (5)
fs(4-4)require(1-1)require(7-7)path(5-5)result(127-127)main.js (9)
fs(3-3)require(1-1)require(6-6)require(8-8)require(11-11)require(12-16)path(2-2)deps(37-52)handler(36-36)libs/ipc-handlers.js (11)
require(18-22)require(23-27)require(75-79)require(364-368)require(379-383)accountsPath(81-81)sessionId(101-101)raw(88-88)deps(2-13)result(292-296)result(345-345)
🔇 Additional comments (12)
package.json (1)
3-3: LGTM!Version bump to 3.2.6 is appropriate for the new account refresh feature.
preload.js (1)
76-76: LGTM!The refreshAccounts API method follows the established pattern and correctly bridges the IPC call to the main process.
index.html (1)
120-127: LGTM!The UI changes are well-structured with proper accessibility attributes (aria-label, aria-live, title). The horizontal layout with the refresh button is intuitive.
libs/oauth-jagex.js (1)
419-419: LGTM!Exporting writeAccountsToFile enables the account refresh functionality while maintaining the existing implementation. The function is well-tested through the new IPC handler tests.
css/styles.css (1)
220-265: LGTM!The styling for the refresh button is well-implemented with:
- Proper accessibility states (focus-visible, disabled, hover)
- Consistent color scheme matching the existing design
- Flexible layout that integrates smoothly with the accounts dropdown
renderer.js (7)
2-2: LGTM! Flag prevents infinite loops.The
restoringSelectedAccountflag correctly prevents the character change handler from persisting during restoration, avoiding circular updates.
248-249: Verify restoration logic doesn't override intentional selection changes.After detecting file changes and updating the selection (lines 228-246), the code immediately attempts to restore the persisted account. This could override the selection that was just updated based on account additions/deletions.
For example:
- If a new account is added and line 231 selects it (
updateCharacterSelection(latestAccount.accountId))- Line 249 immediately calls
restoreSelectedAccountIfAny()which might restore a different accountVerify this is the intended behavior and that restoration should take precedence over automatic selection of new accounts.
357-359: LGTM! Correctly prevents circular updates.The conditional check ensures that persistence only occurs during user-initiated changes, not during programmatic restoration, which prevents infinite loops between the character change handler and the restoration logic.
1203-1220: LGTM! Efficient persistence logic with proper error handling.The function correctly:
- Clears the persisted selection when 'none' is selected
- Only writes to properties when the value changes (avoiding unnecessary I/O)
- Handles errors gracefully with logging
1222-1240: LGTM! Robust restoration logic with stale data cleanup.The function correctly:
- Validates that the saved account still exists before restoring
- Cleans up stale references when accounts are deleted
- Uses the restoration flag to prevent circular updates
- Handles errors gracefully
1259-1259: LGTM! Correct placement of restoration during initialization.Calling
restoreSelectedAccountIfAny()after accounts are loaded and the sidebar is set up ensures the persisted selection can be properly restored on application startup.
1276-1277: LGTM! Refresh button setup correctly placed during initialization.Calling
setupRefreshAccountsButton()at the end of UI initialization ensures the handler is registered after the DOM is ready.
| try { | ||
| const updatedRaw = fs.readFileSync(accountsPath, 'utf8'); | ||
| const updatedAccounts = JSON.parse(updatedRaw); | ||
| return { success: true, accounts: updatedAccounts }; | ||
| } catch (err) { | ||
| log.error('Failed to read updated accounts.json after refresh:', err.message); | ||
| return { success: true }; | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling after refresh.
If re-reading the updated accounts.json fails after a successful refresh, the handler returns { success: true } without the updated accounts list. This could cause the UI to not reflect the refreshed accounts. Consider either:
- Returning the error and requiring a full page refresh
- Keeping the accounts in memory before refresh as a fallback
-try {
- const updatedRaw = fs.readFileSync(accountsPath, 'utf8');
- const updatedAccounts = JSON.parse(updatedRaw);
- return { success: true, accounts: updatedAccounts };
-} catch (err) {
- log.error('Failed to read updated accounts.json after refresh:', err.message);
- return { success: true };
-}
+try {
+ const updatedRaw = fs.readFileSync(accountsPath, 'utf8');
+ const updatedAccounts = JSON.parse(updatedRaw);
+ return { success: true, accounts: updatedAccounts };
+} catch (err) {
+ log.error('Failed to read updated accounts.json after refresh:', err.message);
+ return { error: 'Refresh completed but failed to read updated accounts. Please reload.' };
+}📝 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.
| try { | |
| const updatedRaw = fs.readFileSync(accountsPath, 'utf8'); | |
| const updatedAccounts = JSON.parse(updatedRaw); | |
| return { success: true, accounts: updatedAccounts }; | |
| } catch (err) { | |
| log.error('Failed to read updated accounts.json after refresh:', err.message); | |
| return { success: true }; | |
| } | |
| try { | |
| const updatedRaw = fs.readFileSync(accountsPath, 'utf8'); | |
| const updatedAccounts = JSON.parse(updatedRaw); | |
| return { success: true, accounts: updatedAccounts }; | |
| } catch (err) { | |
| log.error('Failed to read updated accounts.json after refresh:', err.message); | |
| return { error: 'Refresh completed but failed to read updated accounts. Please reload.' }; | |
| } |
🤖 Prompt for AI Agents
In libs/ipc-handlers.js around lines 109-116, the catch block after re-reading
accounts.json returns { success: true } without accounts, causing the UI to lose
refreshed data; update the handler to either return a failure with the error
(e.g. { success: false, error: err.message }) so callers can trigger a full
refresh, or keep a previously-read accounts snapshot before the refresh and
return it as a fallback (e.g. { success: true, accounts: previousAccounts });
ensure you capture previousAccounts before performing the refresh and log the
full error message when returning failure.
| // Sort alphabetically by displayName (case-insensitive); fallback to accountId | ||
| try { | ||
| result.sort((a, b) => { | ||
| const nameA = (a?.displayName || a?.accountId || '').toString().trim().toLowerCase(); | ||
| const nameB = (b?.displayName || b?.accountId || '').toString().trim().toLowerCase(); | ||
| if (nameA < nameB) return -1; | ||
| if (nameA > nameB) return 1; | ||
| return 0; | ||
| }); | ||
| } catch (_) { | ||
| // Ignore sort errors | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicate sorting logic and avoid silent error swallowing.
This sorting logic is duplicated in setupRefreshAccountsButton() (lines 899-905). Additionally, the catch block silently ignores all errors, which could hide bugs.
Extract the sorting logic into a helper function:
+function sortAccountsAlphabetically(accountsList) {
+ if (!Array.isArray(accountsList)) return accountsList;
+ try {
+ accountsList.sort((a, b) => {
+ const nameA = (a?.displayName || a?.accountId || '').toString().trim().toLowerCase();
+ const nameB = (b?.displayName || b?.accountId || '').toString().trim().toLowerCase();
+ if (nameA < nameB) return -1;
+ if (nameA > nameB) return 1;
+ return 0;
+ });
+ } catch (err) {
+ window.electron?.logError?.(`Failed to sort accounts: ${err?.message || err}`);
+ }
+ return accountsList;
+}
+
async function safeReadAccounts() {
const result = await window.electron.readAccounts();
if (result?.error) {
reportAccountsError(`Failed to load accounts: ${result.error}`);
return null;
}
if (!Array.isArray(result)) {
reportAccountsError('Accounts data is in an unexpected format.');
return null;
}
lastAccountsReadError = null;
- // Sort alphabetically by displayName (case-insensitive); fallback to accountId
- try {
- result.sort((a, b) => {
- const nameA = (a?.displayName || a?.accountId || '').toString().trim().toLowerCase();
- const nameB = (b?.displayName || b?.accountId || '').toString().trim().toLowerCase();
- if (nameA < nameB) return -1;
- if (nameA > nameB) return 1;
- return 0;
- });
- } catch (_) {
- // Ignore sort errors
- }
- return result;
+ return sortAccountsAlphabetically(result);
}Then update setupRefreshAccountsButton() to use the same helper (lines 898-905):
if (Array.isArray(updated)) {
- // Sort updated accounts alphabetically
- accounts = [...updated].sort((a, b) => {
- const nameA = (a?.displayName || a?.accountId || '').toString().trim().toLowerCase();
- const nameB = (b?.displayName || b?.accountId || '').toString().trim().toLowerCase();
- if (nameA < nameB) return -1;
- if (nameA > nameB) return 1;
- return 0;
- });
+ accounts = sortAccountsAlphabetically([...updated]);📝 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.
| // Sort alphabetically by displayName (case-insensitive); fallback to accountId | |
| try { | |
| result.sort((a, b) => { | |
| const nameA = (a?.displayName || a?.accountId || '').toString().trim().toLowerCase(); | |
| const nameB = (b?.displayName || b?.accountId || '').toString().trim().toLowerCase(); | |
| if (nameA < nameB) return -1; | |
| if (nameA > nameB) return 1; | |
| return 0; | |
| }); | |
| } catch (_) { | |
| // Ignore sort errors | |
| } | |
| function sortAccountsAlphabetically(accountsList) { | |
| if (!Array.isArray(accountsList)) return accountsList; | |
| try { | |
| accountsList.sort((a, b) => { | |
| const nameA = (a?.displayName || a?.accountId || '').toString().trim().toLowerCase(); | |
| const nameB = (b?.displayName || b?.accountId || '').toString().trim().toLowerCase(); | |
| if (nameA < nameB) return -1; | |
| if (nameA > nameB) return 1; | |
| return 0; | |
| }); | |
| } catch (err) { | |
| console.error('Failed to sort accounts:', err); | |
| } | |
| return accountsList; | |
| } | |
| async function safeReadAccounts() { | |
| const result = await window.electron.readAccounts(); | |
| if (result?.error) { | |
| reportAccountsError(`Failed to load accounts: ${result.error}`); | |
| return null; | |
| } | |
| if (!Array.isArray(result)) { | |
| reportAccountsError('Accounts data is in an unexpected format.'); | |
| return null; | |
| } | |
| lastAccountsReadError = null; | |
| return sortAccountsAlphabetically(result); | |
| } |
🤖 Prompt for AI Agents
In renderer.js around lines 114-125 (and duplicate at ~898-905), the
alphabetical sort logic is duplicated and its try/catch silently swallows
errors; extract this into a single helper function (e.g.,
sortAccounts(accounts)) that encapsulates the case-insensitive
displayName/accountId comparison and returns the sorted array (document whether
it sorts in-place or returns a new array), then replace both existing blocks to
call that helper; remove the empty catch and instead let the helper either
propagate errors or catch and log them (use console.error or a logger) and
return the original array as a safe fallback.
1 participant
test
Summary by CodeRabbit
New Features
Improvements
Tests
Chores