fix: address code review suggestions for PR #108 (#109)

1. cellEditBehavior precedence: Move VS Code env config read after state
   restoration so extension settings override restored webview state.
   If the user changes the setting while a tab is hidden, the new value
   takes effect on re-show.

2. Unit tests: Add tests for getNodeFs() (returns fs module in Node.js,
   returns same reference on repeated calls) and LogEnvelope processing
   (onLog callback invoked with correct args, silent drop without callback).

3. JSDoc cleanup: Remove duplicate processProtocolMessage JSDoc block
   in rpc.ts and consolidate into single comment with onLog parameter.

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

Author: zknpr

core/ui/viewer.html

@@ -67,14 +67,6 @@ async function initializeApp() {
         initSidebarResize();
         initDragAndDrop();
 
-        // Read configuration from environment
-        const vscodeEnv = document.getElementById('vscode-env');
-        if (vscodeEnv) {
-             if (vscodeEnv.dataset.cellEditBehavior) {
-                 state.cellEditBehavior = vscodeEnv.dataset.cellEditBehavior;
-             }
-        }
-
         updateStatus('Connecting to database...');
 
         const result = await backendApi.initialize();
@@ -159,6 +151,16 @@ async function initializeApp() {
             showEmptyState();
         }
 
+        // Apply VS Code extension settings last so they override any restored state.
+        // This ensures that if the user changes a setting while a tab is hidden,
+        // the new setting takes effect when the tab is re-shown.
+        const vscodeEnv = document.getElementById('vscode-env');
+        if (vscodeEnv) {
+            if (vscodeEnv.dataset.cellEditBehavior) {
+                state.cellEditBehavior = vscodeEnv.dataset.cellEditBehavior;
+            }
+        }
+
         // Global shortcuts
         document.addEventListener('keydown', async (event) => {
             // Undo / Redo - Handled natively by VS Code for Custom Editors

core/ui/viewer.js

@@ -200,22 +200,24 @@ type MethodImplementations = Record<string, (...args: any[]) => unknown>;
  */
 type ResponseDispatcher = (response: ResponseEnvelope, transfer?: Transferable[]) => void;
 
+/**
+ * Callback for handling log messages forwarded from a worker.
+ */
+type LogHandler = (level: 'log' | 'warn' | 'error', args: unknown[]) => void;
+
 /**
  * Process an incoming protocol message.
  *
  * For invocation requests: executes local method and sends response.
  * For response messages: resolves pending promise.
+ * For log messages: forwards to the provided log handler.
  *
  * @param envelope - Incoming protocol message
  * @param localMethods - Optional local method implementations
  * @param sendResponse - Optional function to send responses
+ * @param onLog - Optional callback for worker log messages
  * @returns true if message was handled, false otherwise
  */
-/**
- * Callback for handling log messages forwarded from a worker.
- */
-type LogHandler = (level: 'log' | 'warn' | 'error', args: unknown[]) => void;
-
 export function processProtocolMessage(
   envelope: unknown,
   localMethods?: MethodImplementations,

src/core/rpc.ts

@@ -37,6 +37,35 @@ describe('RPC', () => {
       }, 0);
     });
 
+    it('should handle log messages with onLog callback', () => {
+      let capturedLevel: string | null = null;
+      let capturedArgs: unknown[] | null = null;
+      const onLog = (level: string, args: unknown[]) => {
+        capturedLevel = level;
+        capturedArgs = args;
+      };
+
+      const handled = processProtocolMessage(
+        { kind: 'log', level: 'warn', args: ['test warning', 42] },
+        undefined,
+        undefined,
+        onLog
+      );
+
+      assert.strictEqual(handled, true);
+      assert.strictEqual(capturedLevel, 'warn');
+      assert.deepStrictEqual(capturedArgs, ['test warning', 42]);
+    });
+
+    it('should handle log messages without onLog callback (silent drop)', () => {
+      // Log messages should still return true (handled) even without a callback
+      const handled = processProtocolMessage(
+        { kind: 'log', level: 'error', args: ['ignored error'] }
+      );
+
+      assert.strictEqual(handled, true);
+    });
+
     it('should handle unknown methods', () => {
       const methods = {};
       let response: any = null;

tests/unit/rpc.test.ts

@@ -1,7 +1,23 @@
 
 import { describe, it, before, after } from 'node:test';
 import assert from 'node:assert';
-import { createDatabaseEngine } from '../../src/core/sqlite-db';
+import { createDatabaseEngine, getNodeFs } from '../../src/core/sqlite-db';
+
+describe('getNodeFs', () => {
+  it('should return the fs module in Node.js environment', () => {
+    const fs = getNodeFs();
+    assert.ok(fs, 'getNodeFs() should return a module in Node.js');
+    assert.strictEqual(typeof fs.readFileSync, 'function', 'should have readFileSync');
+    assert.strictEqual(typeof fs.writeFileSync, 'function', 'should have writeFileSync');
+    assert.strictEqual(typeof fs.statSync, 'function', 'should have statSync');
+  });
+
+  it('should return the same module on repeated calls', () => {
+    const fs1 = getNodeFs();
+    const fs2 = getNodeFs();
+    assert.strictEqual(fs1, fs2, 'should return the same fs reference');
+  });
+});
 
 describe('WasmDatabaseEngine', () => {
   let engine: any;