Skip to content

Refactor coding standards baseline - #3

Merged
nonnil merged 2 commits into
mainfrom
symphony/tra-3-coding-standards-refactor
May 9, 2026
Merged

Refactor coding standards baseline#3
nonnil merged 2 commits into
mainfrom
symphony/tra-3-coding-standards-refactor

Conversation

@nonnil

@nonnil nonnil commented May 9, 2026

Copy link
Copy Markdown
Member

Summary

  • Tightened foundation code against the new coding standards without adding feature scope.
  • Removed non-null assertions and broad local SQLite adapter typing.
  • Replaced local frontmatter parsing assertions with explicit guarded reads.
  • Removed console.log audit hits from spawned DB test snippets.
  • Removed the untracked local package-lock.json artifact from the workspace; Bun remains the only package-manager lockfile.

Audit Summary

  • rg "any|as any|@ts-ignore|@ts-expect-error|eslint-disable" src tests now matches only Drizzle relation helper identifiers named many; there is no literal any, as any, ts-ignore, ts-expect-error, or eslint-disable usage.
  • rg "unknown|Record<string, unknown>|object|Function" src tests remains limited to trust-boundary parsing/error narrowing in config, store, and tests. The broad unknown-typed SQLite adapter was replaced with Bun SQLite types.
  • rg "createEffect|createMemo|splitProps|onCleanup|on\\(" src tests matches createMemoryContentFixture names and Drizzle .on(...) schema calls only; no Solid reactive API misuse found.
  • rg "sql\\.raw|db\\.run|db\\.exec|query\\(|console\\.log" src tests has no remaining console.log matches. Remaining query( is Drizzle repository API usage.
  • rg "TODO|FIXME|HACK|temporary|legacy|workaround" src tests docs found temporary-file/test workflow language and workflow documentation text only; no TODO/FIXME/HACK implementation debt.

Files Refactored

  • src/entry-client.tsx
  • src/server/config/load.ts
  • src/server/db/connection.ts
  • src/server/store/memory-content.ts
  • tests/server/db/schema.test.ts

Verification

  • MISE_TRUSTED_CONFIG_PATHS="$PWD/mise.toml" mise exec -C "$PWD" -- bun run typecheck passed.
  • MISE_TRUSTED_CONFIG_PATHS="$PWD/mise.toml" mise exec -C "$PWD" -- bun run test passed: 4 files, 23 tests.
  • MISE_TRUSTED_CONFIG_PATHS="$PWD/mise.toml" mise exec -C "$PWD" -- bun run build passed.
  • git --git-dir=.tmp/gitdir --work-tree="$PWD" diff --check passed.
  • Pre-push hook passed typecheck, test, and build again.
  • MISE_TRUSTED_CONFIG_PATHS="$PWD/mise.toml" mise exec -C "$PWD" -- bun run test:e2e did not reach app assertions because Chromium failed to launch in this macOS sandbox: MachPortRendezvousServer ... Permission denied (1100).
  • Fallback route smoke: built server started with bun run start -- --port 4173; curl -fsS http://127.0.0.1:4173/memories | rg "Trauma|Memories|No memories yet" passed.

Scope Confirmation

No importer behavior, reader rendering, highlight behavior, backup behavior, auth, new UI flow, or runtime dependency was added.

Summary by CodeRabbit

  • Bug Fixes

    • Improved error handling with explicit validation for missing application elements and configuration fields
    • Enhanced frontmatter parsing with stricter key validation and error reporting
  • Refactor

    • Optimized configuration validation logic for better clarity
    • Updated internal database connection typing for improved type safety

Review Change Stack

Rework Update

  • Addressed Copilot thread on src/server/store/memory-content.ts: isFrontmatterKey now uses FRONTMATTER_KEY_SET derived from canonical FRONTMATTER_KEYS.
  • Addressed Copilot thread on src/server/db/connection.ts: BunDatabaseConstructor now uses typeof import("bun:sqlite").Database.
  • Rework commit: fabf1c5.
  • Inline review threads were replied to, reacted to, and resolved.
  • Rework validation: typecheck passed; test passed (4 files, 23 tests); build passed; git --git-dir=.tmp/gitdir --work-tree="$PWD" diff --check passed. The Gemini verification wrapper was attempted first and timed out after 900s, so the same commands were run locally.

@coderabbitai

coderabbitai Bot commented May 9, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

This PR refactors the codebase to eliminate non-null assertions and add explicit runtime validation. Database connection typing now uses Bun's native types; config validation extracts and validates individual fields with early failure guards; frontmatter parsing builds results via explicit per-field reads; the client entry point validates the DOM element before mounting; and test output is switched to stdout for compatibility.

Changes

Runtime Validation Without Non-Null Assertions

Layer / File(s) Summary
Database Connection Types
src/server/db/connection.ts
Introduces BunDatabaseConstructor type and updates TraumaDatabaseConnection.sqlite to use Bun's BunDatabase instead of local SQLiteDatabase interface.
Config Validation Refactoring
src/server/config/load.ts
validateTraumaConfig splits git config validation into individual fields (gitEnabled, gitRemote, gitBranch, gitPush, gitCommitMessageTemplate); adds early failure guard when required values are undefined; resolves paths without non-null assertions.
Database Connection Helpers
src/server/db/connection.ts
loadDatabaseConstructor uses BunDatabaseConstructor type; createDrizzleDatabase accepts BunDatabase directly without casts.
Frontmatter Parsing
src/server/store/memory-content.ts
Introduces FRONTMATTER_KEY_SET for O(1) key validation; refactors parseSerializedFrontmatter to read each required field explicitly via new readSerializedFrontmatterValue helper; adds explicit unknown type annotation in JSON parse.
Client Entry Point Validation
src/entry-client.tsx
Adds explicit lookup and runtime guard for #app DOM element; throws error if missing instead of using non-null assertion; passes validated element to mount call.
Test Output Compatibility
tests/server/db/schema.test.ts
Updates inline Bun scripts to write JSON results via process.stdout.write(JSON.stringify(...)) instead of console.log(...) for improved test output reliability.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • hauntedfail/Trauma#1: Directly related edits to src/server/config/load.ts and src/server/db/connection.ts that refine the config validation and DB connection code from the base PR.

Poem

🐰 No more exclamation marks and casts,
Each field now checked before it lasts,
The bunny hops through types so clear,
With validation guards near and dear,
Safe paths emerge from assertion's past! 🌳✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'Refactor coding standards baseline' is vague and generic, using non-descriptive terms that don't clearly convey the specific technical changes in the changeset. Consider a more specific title that highlights the main technical changes, such as 'Remove non-null assertions and refactor SQLite typing' or 'Replace assertions with explicit validation in core modules'.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch symphony/tra-3-coding-standards-refactor

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@nonnil

nonnil commented May 9, 2026

Copy link
Copy Markdown
Member Author

@claude review once

@nonnil nonnil left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

LGTM

@nonnil
nonnil requested a review from Copilot May 9, 2026 09:09
@nonnil

nonnil commented May 9, 2026

Copy link
Copy Markdown
Member Author

review @codex

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 👍

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Refactors several foundational modules to comply with tightened TypeScript/coding standards (removing non-null assertions, narrowing types, and making parsing/validation more explicit) without changing feature scope.

Changes:

  • Replaced non-null assertions with explicit guards (client entry) and explicit validated reads (frontmatter parsing).
  • Narrowed the SQLite handle typing to Bun’s bun:sqlite Database type and removed broad local adapter typing.
  • Removed console.log usage from DB foundation test scripts by writing directly to stdout.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/entry-client.tsx Replaces ! with an explicit DOM root guard before mounting the SolidStart client.
src/server/config/load.ts Removes config type assertions by requiring validated values before building the resolved config object.
src/server/db/connection.ts Replaces a locally-defined SQLite interface with Bun’s Database typing and cleans up drizzle initialization typing.
src/server/store/memory-content.ts Replaces Object.fromEntries casting with explicit key reads and strengthens frontmatter key/type guards.
tests/server/db/schema.test.ts Removes console.log in spawned Bun scripts by using process.stdout.write for structured JSON output.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/server/store/memory-content.ts Outdated
Comment thread src/server/db/connection.ts Outdated

@nonnil nonnil left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

LGTM

@nonnil
nonnil merged commit 82aa416 into main May 9, 2026
2 checks passed
@nonnil
nonnil deleted the symphony/tra-3-coding-standards-refactor branch May 9, 2026 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants