Skip to content

Fix ESM conversion in search and logging tabs#2434

Merged
sensei-hacker merged 3 commits intoiNavFlight:masterfrom
sensei-hacker:esm_modules_strays
Nov 28, 2025
Merged

Fix ESM conversion in search and logging tabs#2434
sensei-hacker merged 3 commits intoiNavFlight:masterfrom
sensei-hacker:esm_modules_strays

Conversation

@sensei-hacker
Copy link
Member

@sensei-hacker sensei-hacker commented Nov 26, 2025

User description

Summary

  • Convert remaining require() statements to ESM imports in search tab
  • Fix missing store import in logging tab
  • Add appendFile IPC handler for logging tab file operations

PR Type

Bug fix, Enhancement


Description

  • Convert search tab from CommonJS to ESM imports

  • Add missing store import to logging tab

  • Implement appendFile IPC handler for file operations

  • Replace synchronous fs.writeFileSync with async appendFile API


Diagram Walkthrough

flowchart LR
  A["Search Tab<br/>CommonJS require"] -->|Convert to ESM| B["Search Tab<br/>ESM imports"]
  C["Logging Tab<br/>Missing store"] -->|Add import| D["Logging Tab<br/>Complete imports"]
  E["fs.writeFileSync<br/>Sync operation"] -->|Replace with| F["appendFile IPC<br/>Async operation"]
  B -->|Dynamic import| G["search.html loader"]
  F -->|Handler added| H["Main process IPC"]
  H -->|Exposed via| I["Preload API"]
Loading

File Walkthrough

Relevant files
Enhancement
configurator_main.js
Convert search tab to ESM dynamic import                                 

js/configurator_main.js

  • Convert search tab loading from synchronous require() to dynamic ESM
    import()
  • Maintain consistent async pattern with other tab initializations
+1/-2     
main.js
Add appendFile IPC handler for logging                                     

js/main/main.js

  • Add appendFile to fs/promises imports
  • Implement new appendFile IPC handler for async file append operations
  • Handler follows same error handling pattern as existing writeFile
    handler
+13/-2   
preload.js
Expose appendFile API to renderer                                               

js/main/preload.js

  • Expose new appendFile method to renderer process via electronAPI
  • Method invokes main process appendFile IPC handler
+1/-0     
search.js
Convert search tab to ESM modules                                               

tabs/search.js

  • Convert CommonJS require() statements to ESM import statements
  • Replace path.join(__dirname, ...) with dynamic import of search.html
  • Update GUI.load callback to work with imported HTML content
+4/-5     
Bug fix
logging.js
Fix imports and async file operations                                       

tabs/logging.js

  • Add missing store import from localization module
  • Replace synchronous fs.writeFileSync() with async
    window.electronAPI.appendFile()
  • Remove unnecessary blank lines for code cleanup
+2/-6     

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Nov 26, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Action Logging: The PR adds an async append operation for log writing, but the new code does not include
user identity or action outcome in log entries, which may be required for comprehensive
audit trails.

Referred Code
    if (log_buffer.length && readyToWrite) { // only execute when there is actual data to write
        window.electronAPI.appendFile(loggingFileName, log_buffer.join('\n') + '\n');
        $('.samples').text(samples += log_buffer.length);
        log_buffer = [];
    }
}, 1000);

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Missing Await/Error: The new appendFile call is not awaited and lacks error handling, risking silent failures
and data loss if the append operation fails.

Referred Code
if (log_buffer.length && readyToWrite) { // only execute when there is actual data to write
    window.electronAPI.appendFile(loggingFileName, log_buffer.join('\n') + '\n');
    $('.samples').text(samples += log_buffer.length);

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Log Content Risk: The code appends buffered data to a log file without demonstrating filtering or redaction,
so it is unclear whether sensitive data could be written to logs.

Referred Code
    if (log_buffer.length && readyToWrite) { // only execute when there is actual data to write
        window.electronAPI.appendFile(loggingFileName, log_buffer.join('\n') + '\n');
        $('.samples').text(samples += log_buffer.length);
        log_buffer = [];
    }
}, 1000);

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
IPC Validation: Newly exposed appendFile IPC lacks visible validation or sanitization of filename and data
in the renderer-to-main pathway, which could pose security risks.

Referred Code
writeFile: (filename, data) => ipcRenderer.invoke('writeFile', filename, data),
appendFile: (filename, data) => ipcRenderer.invoke('appendFile', filename, data),
readFile: (filename, encoding = 'utf8') => ipcRenderer.invoke('readFile', filename, encoding),

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Nov 26, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix silent error handling bug
Suggestion Impact:The commit replaced the Promise resolve pattern with a try/catch that returns false on success and throws on error, matching the suggested fix to enable correct error handling.

code diff:

   ipcMain.handle('appendFile', (_event, filename, data) => {
-    return new Promise(async resolve => {
-      try {
-        await appendFile(filename, data);
-        resolve(false)
-      } catch (err) {
-        resolve(err);
-      }
-    });
+    try {
+      await appendFile(filename, data);
+      return false;
+    } catch (err) {
+      // Re-throwing the error will cause the promise on the renderer side to be rejected.
+      throw err;
+    }
   });

Refactor the appendFile IPC handler to be a simple async function that properly
rejects on error, instead of resolving with an error object, to enable correct
error handling in the renderer process.

js/main/main.js [320-327]

-return new Promise(async resolve => {
-  try {
-    await appendFile(filename, data);
-    resolve(false)
-  } catch (err) {
-    resolve(err);
-  }
-});
+try {
+  await appendFile(filename, data);
+  return false;
+} catch (err) {
+  // Re-throwing the error will cause the promise on the renderer side to be rejected.
+  throw err;
+}

[Suggestion processed]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a promise anti-pattern in the new appendFile handler that would lead to silent failures and provides the correct fix.

Medium
Handle potential file writing errors

Add promise error handling to the window.electronAPI.appendFile call to catch
and manage potential file writing failures, preventing silent data loss.

tabs/logging.js [85]

-window.electronAPI.appendFile(loggingFileName, log_buffer.join('\n') + '\n');
+window.electronAPI.appendFile(loggingFileName, log_buffer.join('\n') + '\n')
+    .catch(error => {
+        console.error('Failed to write to log file:', error);
+        // Optionally, you could stop the logging intervals here and notify the user.
+    });
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly points out that the newly introduced asynchronous appendFile call lacks error handling, which could lead to silent data loss if file writing fails.

Medium
General
Add error handling for dynamic import

Add a .catch() block to the dynamic import() of search.html to handle potential
loading errors and prevent the tab from failing silently.

tabs/search.js [187-192]

 import('./search.html?raw').then(({default: html}) => GUI.load(html, function () {
     i18n.localize();
     document.getElementById('search-label').addEventListener('click', searchKeyword, false);
     document.getElementById('search-keyword').addEventListener('keyup', searchKeywordTyping, false);
     GUI.content_ready(callback);
-}));
+})).catch(error => {
+    console.error('Failed to load search.html:', error);
+    // Optionally, notify the user that the tab could not be loaded.
+});
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the new dynamic import() lacks error handling, which could cause the search tab to fail to load silently.

Medium
Learned
best practice
Normalize ESM import paths

Drop the .js extension to match existing ESM import conventions used elsewhere
in the codebase, ensuring module resolution remains consistent after bundling.

tabs/search.js [1-2]

-import { GUI, TABS } from './../js/gui.js';
-import i18n from './../js/localization.js';
+import { GUI, TABS } from './../js/gui';
+import i18n from './../js/localization';

[Suggestion processed]

Suggestion importance[1-10]: 5

__

Why:
Relevant best practice - Ensure imports reference the correct module identifiers/paths when converting to ESM to avoid broken bindings at runtime.

Low
  • Update

@sensei-hacker sensei-hacker changed the title Fix ESM conversion regressions in search and logging tabs Fix ESM conversion in search and logging tabs Nov 26, 2025
Comment on lines +1 to +2
import { GUI, TABS } from './../js/gui.js';
import i18n from './../js/localization.js';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Normalize ESM import paths

Suggested change
import { GUI, TABS } from './../js/gui.js';
import i18n from './../js/localization.js';
import { GUI, TABS } from './../js/gui';
import i18n from './../js/localization';

Co-authored-by: qodo-merge-pro[bot] <151058649+qodo-merge-pro[bot]@users.noreply.github.com>
The previous commit added error handling to the appendFile IPC handler
but forgot to add the 'async' keyword to the arrow function. This
caused the build to fail with "await can only be used inside an async
function" error.
@sensei-hacker sensei-hacker merged commit ac4d93c into iNavFlight:master Nov 28, 2025
6 checks passed
@sensei-hacker sensei-hacker added this to the 9.0 milestone Nov 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant