Conversation
Review Status🔍 Review in progress by columbus... Note This comment will be updated with the review results when complete. 🌃 Co-built with Midtown |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27f8e1c167
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/matrix_bridge/client.rs
Outdated
| impl MatrixClient { | ||
| pub fn new(homeserver_url: impl Into<String>, access_token: impl Into<String>) -> Self { | ||
| let homeserver_url = homeserver_url.into().trim_end_matches('/').to_string(); | ||
| let homeserver_domain = infer_homeserver_domain(&homeserver_url); |
There was a problem hiding this comment.
MatrixClient::new infers homeserver_domain from homeserver_url, so with http://127.0.0.1:6167 it becomes 127.0.0.1:6167, but the bridge config sets Conduit server_name to <project>.local and run_as_server uses this inferred domain for sender filtering. In that setup, matrix_user_localpart will reject incoming users from <project>.local, so Matrix→Midtown message ingestion is dropped for normal traffic (and generated user IDs/aliases use the wrong domain).
Useful? React with 👍 / 👎.
src/matrix_bridge/mod.rs
Outdated
| cmd.spawn() | ||
| .map(|_| ()) | ||
| .map_err(|e| format!("Failed to spawn Conduit '{exe}': {e}")) |
There was a problem hiding this comment.
The bridge starts Conduit with cmd.spawn() and immediately discards the Child handle, while midtown matrix stop only kills the PID tracked in bridge.pid (the matrix bridge wrapper process). This leaves Conduit running after stop/restart cycles, so port 6167 and old homeserver state can persist unexpectedly across runs.
Useful? React with 👍 / 👎.
src/bin/midtown/cli/matrix.rs
Outdated
| fn matrix_runtime_dir() -> PathBuf { | ||
| midtown::paths::midtown_base_dir().join("matrix") |
There was a problem hiding this comment.
The bridge PID/state directory is fixed at ~/.midtown/matrix, so all projects share one bridge.pid and one bridge state. In a multi-project workspace this causes cross-project interference: launching midtown view --matrix in project B can reuse project A’s running bridge/state, and stopping from one project tears down the other project’s bridge.
Useful? React with 👍 / 👎.
|
I addressed all remaining P1 review items from the latest review and pushed commit b758a3a:\n\n- Matrix AS domain now comes from bridge config server_name (not inferred from URL) so user aliases align with ".local".\n- Conduit is now started as tracked |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (2.52%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1453 +/- ##
==========================================
- Coverage 67.34% 65.69% -1.65%
==========================================
Files 102 109 +7
Lines 52723 54097 +1374
==========================================
+ Hits 35507 35541 +34
- Misses 17216 18556 +1340
🚀 New features to boost your workflow:
|
1 participant
Summary
Usage:
npm install install all the dependencies in your project
npm install add the dependency to your project
npm test run this project's tests
npm run run the script named
npm -h quick help on
npm -l display usage info for all commands
npm help search for help on
npm help npm more involved overview
All commands:
Specify configs in the ini-formatted file:
/Users/btucker/.npmrc
or on the command line via: npm --key=value
More configuration info: npm help config
Configuration fields: npm help 7 config
npm@11.6.0 /Users/btucker/.nvm/versions/node/v24.1.0/lib/node_modules/npm) and Cinny bootstrapping flow (download latest release tarball, extract, patch local config, install deps, and start client).
Notes
Using the default project-based local server hostname and homeserver configuration generated during bootstrap.