Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Polish and improvements #14

Merged
merged 17 commits into from
Jan 1, 2025
Merged

feat: Polish and improvements #14

merged 17 commits into from
Jan 1, 2025

Conversation

t1mmen
Copy link
Owner

@t1mmen t1mmen commented Jan 1, 2025

PR Type

Enhancement, Tests, Bug fix


Description

  • Major improvements to watch mode stability and performance

  • Enhanced error handling and cleanup across components

  • Added fullscreen CLI mode and better UI layout

  • Comprehensive test coverage for watch functionality


Changes walkthrough 📝

Relevant files
Error handling
2 files
vitest.setup.ts
Improve test setup with better error handling and logging
+46/-27 
databaseConnection.ts
Add proper signal handling for database cleanup                   
+12/-1   
Enhancement
4 files
_app.tsx
Add fullscreen mode and improve UI layout                               
+25/-15 
watch.tsx
Enhance watch mode UI and error handling                                 
+21/-7   
useTemplateManager.ts
Simplify template manager hook and improve types                 
+98/-121
templateManager.ts
Major refactor of template manager with better error handling
+167/-53
Documentation
4 files
cuddly-elephants-shave.md
Add changeset for watcher cleanup                                               
+5/-0     
sharp-owls-boil.md
Add changeset for watch mode improvements                               
+5/-0     
tricky-lemons-type.md
Add changeset for fullscreen CLI                                                 
+5/-0     
wise-fishes-kick.md
Add changeset for template loading improvements                   
+5/-0     
Configuration changes
1 files
package.json
Update start script to use tsx                                                     
+1/-1     
Additional files
1 files
templateManager.test.ts +528/-10

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

Copy link

changeset-bot bot commented Jan 1, 2025

🦋 Changeset detected

Latest commit: 26b9346

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@t1mmen/srtd Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

qodo-merge-pro-for-open-source bot commented Jan 1, 2025

CI Failure Feedback 🧐

(Checks updated until commit c17cbb0)

Action: test (20.x)

Failed stage: Test [❌]

Failed test name: src/lib/templateManager.test.ts > TemplateManager > should handle sequential template operations

Failure summary:

The action failed due to two failing tests in the TemplateManager test suite:

  • Test "should handle sequential template operations" failed because an empty array was returned when
    it expected an array of length 1
  • Test "should cleanup resources when disposed" failed with the same assertion error
  • Both failures appear to be related to array length validation in the TemplateManager component,
    suggesting potential issues with template processing or state management

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    264:  �[2mCoverage enabled with �[22m�[33mv8�[39m
    265:  �[32m✓�[39m src/utils/config.test.ts�[2m > �[22mconfig�[2m > �[22mshould call getConfig with the correct path
    266:  �[32m✓�[39m src/utils/config.test.ts�[2m > �[22mconfig�[2m > �[22mshould return mocked test config values
    267:  �[32m✓�[39m src/utils/config.test.ts�[2m > �[22mconfig�[2m > �[22mshould save and merge with default config
    268:  �[32m✓�[39m src/utils/config.test.ts�[2m > �[22mconfig�[2m > �[22mshould handle empty config object
    269:  �[32m✓�[39m src/utils/config.test.ts�[2m > �[22mconfig�[2m > �[22mshould preserve unknown fields
    270:  �[32m✓�[39m src/utils/config.test.ts�[2m > �[22mconfig�[2m > �[22mshould handle nested paths correctly
    271:  �[32m✓�[39m src/utils/applyMigrations.test.ts�[2m > �[22mapplyMigration�[2m > �[22mshould successfully apply valid SQL
    272:  �[32m✓�[39m src/utils/applyMigrations.test.ts�[2m > �[22mapplyMigration�[2m > �[22mshould fail and rollback on invalid SQL
    ...
    
    277:  �[32m✓�[39m src/utils/applyMigrations.test.ts�[2m > �[22mapplyMigration�[2m > �[22mshould handle concurrent migrations with advisory locks
    278:  �[32m✓�[39m src/utils/applyMigrations.test.ts�[2m > �[22mapplyMigration�[2m > �[22mshould handle transactions properly
    279:  �[32m✓�[39m src/utils/loadBuildLog.test.ts�[2m > �[22mloadBuildLog�[2m > �[22mshould load existing build log
    280:  �[32m✓�[39m src/utils/loadBuildLog.test.ts�[2m > �[22mloadBuildLog�[2m > �[22mshould return empty build log when file does not exist
    281:  �[32m✓�[39m src/utils/loadBuildLog.test.ts�[2m > �[22mloadBuildLog�[2m > �[22mshould handle invalid JSON
    282:  �[32m✓�[39m src/utils/loadBuildLog.test.ts�[2m > �[22mloadBuildLog�[2m > �[22mshould handle missing fields
    283:  �[32m✓�[39m src/utils/loadBuildLog.test.ts�[2m > �[22mloadBuildLog�[2m > �[22mshould load correct file based on type parameter
    284:  �[32m✓�[39m src/utils/applyMigrations.test.ts�[2m > �[22mapplyMigration�[2m > �[22mshould handle large SQL statements
    285:  �[32m✓�[39m src/utils/applyMigrations.test.ts�[2m > �[22mapplyMigration�[2m > �[22mshould handle errors in stored procedures
    ...
    
    292:  (Use `node --trace-warnings ...` to show where the warning was created)
    293:  �[32m✓�[39m src/lib/templateManager.test.ts�[2m > �[22mTemplateManager�[2m > �[22mshould handle rapid template changes�[33m 889�[2mms�[22m�[39m
    294:  �[32m✓�[39m src/lib/templateManager.test.ts�[2m > �[22mTemplateManager�[2m > �[22mshould apply WIP templates directly to database
    295:  �[32m✓�[39m src/__tests__/watch.test.tsx�[2m > �[22mWatch Command�[2m > �[22mrenders initial state with no templates
    296:  �[31m�[39m src/lib/templateManager.test.ts�[2m > �[22mTemplateManager�[2m > �[22mshould handle sequential template operations
    297:  �[31m   → expected [] to have a length of 1 but got +0�[39m
    298:  �[32m✓�[39m src/lib/templateManager.test.ts�[2m > �[22mTemplateManager�[2m > �[22mshould generate unique timestamps for multiple templates
    299:  �[32m✓�[39m src/lib/templateManager.test.ts�[2m > �[22mTemplateManager�[2m > �[22mshould handle mix of working and broken templates
    300:  �[32m✓�[39m src/lib/templateManager.test.ts�[2m > �[22mTemplateManager�[2m > �[22mshould handle database errors gracefully
    301:  �[32m✓�[39m src/lib/templateManager.test.ts�[2m > �[22mTemplateManager�[2m > �[22mshould handle file system errors
    ...
    
    311:  �[31m   → expected [] to have a length of 1 but got +0�[39m
    312:  �[32m✓�[39m src/lib/templateManager.test.ts�[2m > �[22mTemplateManager�[2m > �[22mshould auto-cleanup with using statement
    313:  �[32m✓�[39m src/lib/templateManager.test.ts�[2m > �[22mTemplateManager�[2m > �[22mshould not process unchanged templates
    314:  �[32m✓�[39m src/lib/templateManager.test.ts�[2m > �[22mTemplateManager�[2m > �[22mshould only process modified templates in batch
    315:  �[32m✓�[39m src/lib/templateManager.test.ts�[2m > �[22mTemplateManager�[2m > �[22mshould correctly update local buildlog on apply
    316:  �[32m✓�[39m src/lib/templateManager.test.ts�[2m > �[22mTemplateManager�[2m > �[22mshould skip apply if template hash matches local buildlog
    317:  �[32m✓�[39m src/lib/templateManager.test.ts�[2m > �[22mTemplateManager�[2m > �[22mshould not reapply unchanged templates in watch mode
    318:  �[32m✓�[39m src/lib/templateManager.test.ts�[2m > �[22mTemplateManager�[2m > �[22mshould process unapplied templates on startup
    319:  �[32m✓�[39m src/lib/templateManager.test.ts�[2m > �[22mTemplateManager�[2m > �[22mshould handle error state transitions correctly
    320:  �[32m✓�[39m src/lib/templateManager.test.ts�[2m > �[22mTemplateManager�[2m > �[22mshould maintain correct state through manager restarts
    321:  �[32m✓�[39m src/lib/templateManager.test.ts�[2m > �[22mTemplateManager�[2m > �[22mshould properly format and propagate error messages
    322:  �[31m⎯⎯⎯⎯⎯⎯⎯�[1m�[7m Failed Tests 2 �[27m�[22m⎯⎯⎯⎯⎯⎯⎯�[39m
    323:  �[31m�[1m�[7m FAIL �[27m�[22m�[39m src/lib/templateManager.test.ts�[2m > �[22mTemplateManager�[2m > �[22mshould handle sequential template operations
    324:  �[31m�[1mAssertionError�[22m: expected [] to have a length of 1 but got +0�[39m
    ...
    
    329:  �[36m �[2m❯�[22m src/lib/templateManager.test.ts:�[2m223:26�[22m�[39m
    330:  �[90m221| �[39m          �[32m`�[39m�[36m${�[39mtestContext�[33m.�[39mtestFunctionName�[36m}�[39m�[32m_sequence_test_�[39m�[36m${�[39mi�[36m}�[39m�[32m`�[39m�[33m,�[39m
    331:  �[90m222| �[39m        ])�[33m;�[39m
    332:  �[90m223| �[39m        �[34mexpect�[39m(res�[33m.�[39mrows)�[33m.�[39m�[34mtoHaveLength�[39m(�[34m1�[39m)�[33m;�[39m
    333:  �[90m   | �[39m                         �[31m^�[39m
    334:  �[90m224| �[39m      }
    335:  �[90m225| �[39m    } �[35mfinally�[39m {
    336:  �[31m�[2m⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/2]⎯�[22m�[39m
    337:  �[31m�[1m�[7m FAIL �[27m�[22m�[39m src/lib/templateManager.test.ts�[2m > �[22mTemplateManager�[2m > �[22mshould cleanup resources when disposed
    338:  �[31m�[1mAssertionError�[22m: expected [] to have a length of 1 but got +0�[39m
    ...
    
    343:  �[36m �[2m❯�[22m src/lib/templateManager.test.ts:�[2m584:21�[22m�[39m
    344:  �[90m582| �[39m    �[35mawait�[39m �[35mnew�[39m �[33mPromise�[39m(resolve �[33m=>�[39m �[34msetTimeout�[39m(resolve�[33m,�[39m �[34m100�[39m))�[33m;�[39m
    345:  �[90m583| �[39m
    346:  �[90m584| �[39m    �[34mexpect�[39m(changes)�[33m.�[39m�[34mtoHaveLength�[39m(�[34m1�[39m)�[33m;�[39m
    347:  �[90m   | �[39m                    �[31m^�[39m
    348:  �[90m585| �[39m    �[34mexpect�[39m(changes[�[34m0�[39m])�[33m.�[39m�[34mtoBe�[39m(�[32m`test-before-dispose-�[39m�[36m${�[39mtestContext�[33m.�[39mtimesta…
    349:  �[90m586| �[39m  })�[33m;�[39m
    350:  �[31m�[2m⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[2/2]⎯�[22m�[39m
    351:  �[2m Test Files �[22m �[1m�[31m1 failed�[39m�[22m�[2m | �[22m�[1m�[32m5 passed�[39m�[22m�[90m (6)�[39m
    352:  �[2m      Tests �[22m �[1m�[31m2 failed�[39m�[22m�[2m | �[22m�[1m�[32m51 passed�[39m�[22m�[90m (53)�[39m
    353:  �[2m   Start at �[22m 14:23:41
    354:  �[2m   Duration �[22m 5.55s�[2m (transform 279ms, setup 488ms, collect 640ms, tests 5.45s, environment 3ms, prepare 536ms)�[22m
    355:  JUNIT report written to /home/runner/work/srtd/srtd/test-report.junit.xml
    356:  ##[error]Process completed with exit code 1.
    ...
    
    369:  gpg: Signature made Sat Dec  7 16:07:53 2024 UTC
    370:  gpg:                using RSA key 27034E7FDB850E0BBC2C62FF806BB28AED779869
    371:  gpg: Good signature from "Codecov Uploader (Codecov Uploader Verification Key) <security@codecov.io>" [unknown]
    372:  gpg: WARNING: This key is not certified with a trusted signature!
    373:  gpg:          There is no indication that the signature belongs to the owner.
    374:  Primary key fingerprint: 2703 4E7F DB85 0E0B BC2C  62FF 806B B28A ED77 9869
    375:  ==> Uploader SHASUM verified (bb3c8dcaf649c47ce0321ce153ebc781b88421c97c584b1956fb62c3399db755  codecov)
    376:  ==> Running version latest
    377:  Could not pull latest version information: SyntaxError: Unexpected token '<', "<!DOCTYPE "... is not valid JSON
    ...
    
    421:  2025-01-01 14:23:16.206 UTC [1] LOG:  starting PostgreSQL 15.10 on x86_64-pc-linux-musl, compiled by gcc (Alpine 14.2.0) 14.2.0, 64-bit
    422:  2025-01-01 14:23:16.206 UTC [1] LOG:  listening on IPv4 address "0.0.0.0", port 5432
    423:  2025-01-01 14:23:16.206 UTC [1] LOG:  listening on IPv6 address "::", port 5432
    424:  2025-01-01 14:23:16.208 UTC [1] LOG:  listening on Unix socket "/var/run/postgresql/.s.PGSQL.5432"
    425:  2025-01-01 14:23:16.212 UTC [55] LOG:  database system was shut down at 2025-01-01 14:23:16 UTC
    426:  2025-01-01 14:23:16.217 UTC [1] LOG:  database system is ready to accept connections
    427:  2025-01-01 14:23:24.685 UTC [65] FATAL:  role "root" does not exist
    428:  2025-01-01 14:23:34.747 UTC [72] FATAL:  role "root" does not exist
    429:  2025-01-01 14:23:42.107 UTC [75] ERROR:  syntax error at or near "INVALID" at character 1
    430:  2025-01-01 14:23:42.107 UTC [75] STATEMENT:  INVALID SQL SYNTAX
    431:  2025-01-01 14:23:42.340 UTC [75] ERROR:  division by zero
    432:  2025-01-01 14:23:42.340 UTC [75] STATEMENT:  
    433:  CREATE TABLE IF NOT EXISTS test_table_1735741422041 (id serial);
    434:  SELECT 1/0; -- Force error
    435:  2025-01-01 14:23:42.384 UTC [75] ERROR:  Custom error message
    436:  2025-01-01 14:23:42.384 UTC [75] CONTEXT:  PL/pgSQL function srtd_scoped_test_func_1735741422041() line 3 at RAISE
    437:  2025-01-01 14:23:42.384 UTC [75] STATEMENT:  
    438:  CREATE OR REPLACE FUNCTION srtd_scoped_test_func_1735741422041()
    439:  RETURNS void AS $$
    440:  BEGIN
    441:  RAISE EXCEPTION 'Custom error message';
    442:  END;
    443:  $$ LANGUAGE plpgsql;
    444:  SELECT srtd_scoped_test_func_1735741422041();
    445:  2025-01-01 14:23:43.236 UTC [96] ERROR:  syntax error at or near "INVALID" at character 1
    446:  2025-01-01 14:23:43.236 UTC [96] STATEMENT:  INVALID SQL SYNTAX;
    447:  2025-01-01 14:23:43.251 UTC [97] ERROR:  division by zero
    448:  2025-01-01 14:23:43.251 UTC [97] STATEMENT:  SELECT 1/0;
    449:  2025-01-01 14:23:44.801 UTC [112] FATAL:  role "root" does not exist
    450:  2025-01-01 14:23:46.726 UTC [124] ERROR:  syntax error at or near "INVALID" at character 1
    451:  2025-01-01 14:23:46.726 UTC [124] STATEMENT:  INVALID SQL;
    452:  2025-01-01 14:23:46.860 UTC [126] ERROR:  relation "nonexistent_table" does not exist at character 15
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Race Condition

    The template processing queue implementation may have race conditions when multiple templates are processed simultaneously. The processNextTemplate method uses setImmediate which could lead to overlapping processing.

    private async processNextTemplate(): Promise<void> {
      if (this.processQueue.size === 0) {
        this.processing = false;
        return;
      }
    
      const templatePath = this.processQueue.values().next().value;
      if (!templatePath) {
        this.processing = false;
        return;
      }
    
      this.processQueue.delete(templatePath);
      this.processingTemplate = templatePath;
    
      try {
        await this.processTemplate(templatePath);
      } finally {
        this.processingTemplate = null;
        setImmediate(() => void this.processNextTemplate());
      }
    }
    Resource Cleanup

    The cleanup function uses void to ignore the disconnect promise, which could lead to incomplete cleanup if the process exits before disconnect completes.

    function cleanup() {
      // Sync disconnect since exit handlers must be synchronous
      try {
        void disconnect();
      } catch {
        // Ignore errors during shutdown
      }
      process.exit(0);
    }
    Memory Leak

    The updates array grows without bounds, only limited by slice(0, MAX_CHANGES). Consider implementing a circular buffer or cleaning up old updates periodically.

    const sortedUpdates = useMemo(() => {
      return [...updates]
        .sort((a, b) => new Date(a.timestamp).getTime() - new Date(b.timestamp).getTime())
        .slice(0, MAX_CHANGES);
    }, [updates]);

    Copy link
    Contributor

    qodo-merge-pro-for-open-source bot commented Jan 1, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    ✅ Add timeout handling for database connection tests to prevent indefinite hanging
    Suggestion Impact:Added a 5-second timeout mechanism for database connection testing using Promise.race

    code diff:

    -      const isConnected = await testConnection();
    +      const connectionTimeout = new Promise((_, reject) =>
    +        setTimeout(() => reject(new Error('Database connection timeout')), 5000)
    +      );
    +      const isConnected = await Promise.race([testConnection(), connectionTimeout]);

    Add a timeout to the database connection test to prevent hanging if the database is
    unresponsive.

    src/lib/templateManager.ts [356-361]

     if (options.apply) {
    -  const isConnected = await testConnection();
    +  const connectionTimeout = new Promise((_, reject) => 
    +    setTimeout(() => reject(new Error('Database connection timeout')), 5000)
    +  );
    +  const isConnected = await Promise.race([testConnection(), connectionTimeout]);
       if (!isConnected) {
         this.log('Failed to connect to database, cannot proceed. Is Supabase running?', 'error');
         return result;
       }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This is a crucial improvement for application reliability, preventing the system from hanging indefinitely when database connection issues occur, which could severely impact user experience.

    9
    Validate template content to prevent processing of empty SQL files

    Add a check to ensure the template content is not empty before processing it, as
    empty SQL files could cause issues with the database.

    src/lib/templateManager.ts [249-253]

     async applyTemplate(templatePath: string): Promise<ProcessedTemplateResult> {
       const template = await this.getTemplateStatus(templatePath);
       const content = await fs.readFile(templatePath, 'utf-8');
    +  if (!content.trim()) {
    +    throw new Error(`Template file is empty: ${templatePath}`);
    +  }
       const relPath = path.relative(this.baseDir, templatePath);
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This is a critical validation that prevents potential database issues by catching empty SQL files early, which could otherwise lead to database errors or unexpected behavior.

    8
    ✅ Fail fast when test setup encounters critical errors to prevent unreliable test execution
    Suggestion Impact:The commit implements error throwing when test directory creation fails, though it removes the retry attempt and throws immediately

    code diff:

    @@ -35,12 +35,7 @@
         await fs.mkdir(TEST_ROOT, { recursive: true });
       } catch (error) {
         console.error('Error creating test root:', error, ', retrying once.');
    -
    -    try {
    -      await fs.mkdir(TEST_ROOT, { recursive: true });
    -    } catch (error) {
    -      console.error('Error creating test root:', error, ', aborting.');
    -    }
    +    throw error;
       }

    The error handling for creating test directory should throw after the second attempt
    fails, as silently continuing with a failed test setup could lead to unreliable
    tests.

    src/tests/vitest.setup.ts [41-44]

     try {
       await fs.mkdir(TEST_ROOT, { recursive: true });
     } catch (error) {
       console.error('Error creating test root:', error, ', aborting.');
    +  throw error;
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Critical improvement for test reliability - failing fast when test directory creation fails prevents running tests in an invalid state, which could lead to false positives.

    8
    Add error handling for file watcher initialization to prevent silent failures

    Add error handling for the case when the watcher fails to initialize. Currently, if
    chokidar.watch() fails, the error is not caught or handled properly.

    src/lib/templateManager.ts [215-223]

    -const watcher = chokidar.watch(templatePath, {
    -  ignoreInitial: false,
    -  ignored: ['**/!(*.sql)'],
    -  persistent: true,
    -  awaitWriteFinish: {
    -    stabilityThreshold: 50,
    -    pollInterval: 50,
    -  },
    -});
    +try {
    +  const watcher = chokidar.watch(templatePath, {
    +    ignoreInitial: false,
    +    ignored: ['**/!(*.sql)'],
    +    persistent: true,
    +    awaitWriteFinish: {
    +      stabilityThreshold: 50,
    +      pollInterval: 50,
    +    },
    +  });
    +} catch (error) {
    +  this.log(`Failed to initialize watcher: ${error}`, 'error');
    +  throw new Error(`Failed to initialize file watcher: ${error}`);
    +}
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion addresses a potential reliability issue by adding error handling for watcher initialization, which could prevent silent failures and improve error reporting in production.

    7
    General
    ✅ Properly handle process termination signals with appropriate exit codes for error states
    Suggestion Impact:The commit implements error handling during database disconnection with proper error logging and non-zero exit code on failure, similar to the suggestion's intent

    code diff:

    -  } catch {
    -    // Ignore errors during shutdown
    +  } catch (e) {
    +    logger.error(`Error disconnecting from database: ${String(e)}`);
    +    process.exit(1);

    The cleanup function should handle the process exit code from the signal, and
    propagate error states by using a non-zero exit code when disconnection fails.

    src/utils/databaseConnection.ts [68-76]

    -function cleanup() {
    +function cleanup(signal: NodeJS.Signals) {
       // Sync disconnect since exit handlers must be synchronous
       try {
         void disconnect();
    -  } catch {
    -    // Ignore errors during shutdown
    +    process.exit(0);
    +  } catch (error) {
    +    console.error(`Failed to disconnect during ${signal}:`, error);
    +    process.exit(1);
       }
    -  process.exit(0);
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Important enhancement to system reliability by properly handling cleanup errors and providing accurate exit codes, which is crucial for automated environments and error tracking.

    7
    Handle empty state in UI components to prevent potential rendering issues and improve user experience

    Consider adding error handling for the case when sortedUpdates is empty to avoid
    potential rendering issues. Add a conditional check or display a "no updates"
    message.

    src/commands/watch.tsx [108-111]

     return (
       <Box flexDirection="column" marginTop={1}>
         <Text bold>Changelog:</Text>
    -    {sortedUpdates.map(update => (
    +    {sortedUpdates.length > 0 ? (
    +      sortedUpdates.map(update => (
    +    )) : (
    +      <Text dimColor>No updates yet</Text>
    +    )}
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion improves user experience by adding a fallback UI state when no updates are available, though it's not critical for functionality since an empty list would render fine.

    5

    Copy link

    codecov bot commented Jan 1, 2025

    Codecov Report

    Attention: Patch coverage is 41.77898% with 216 lines in your changes missing coverage. Please review.

    Project coverage is 46.01%. Comparing base (d6a89ce) to head (04c1037).
    Report is 1 commits behind head on main.

    ✅ All tests successful. No failed tests found.

    Files with missing lines Patch % Lines
    src/hooks/useTemplateManager.ts 32.53% 56 Missing ⚠️
    src/commands/clear.tsx 0.00% 42 Missing ⚠️
    src/utils/config.ts 6.89% 27 Missing ⚠️
    src/commands/watch.tsx 16.12% 26 Missing ⚠️
    src/lib/templateManager.ts 82.06% 26 Missing ⚠️
    src/commands/_app.tsx 0.00% 21 Missing ⚠️
    src/utils/databaseConnection.ts 18.18% 9 Missing ⚠️
    src/commands/index.tsx 0.00% 8 Missing ⚠️
    src/commands/apply.tsx 0.00% 1 Missing ⚠️
    Additional details and impacted files
    @@            Coverage Diff             @@
    ##             main      #14      +/-   ##
    ==========================================
    - Coverage   49.79%   46.01%   -3.78%     
    ==========================================
      Files          31       32       +1     
      Lines        1223     1406     +183     
      Branches      137      151      +14     
    ==========================================
    + Hits          609      647      +38     
    - Misses        614      759     +145     

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    @t1mmen t1mmen changed the title Cleanup misc feat: Polish and improvements Jan 1, 2025
    @github-actions github-actions bot added the feat label Jan 1, 2025
    @t1mmen t1mmen merged commit e96e857 into main Jan 1, 2025
    3 of 5 checks passed
    @t1mmen t1mmen deleted the cleanup-misc branch January 1, 2025 14:31
    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