Explicit null for the system card means no card set#3724
Explicit null for the system card means no card set#3724
Conversation
![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.
ℹ️ 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".
| loggedInAs: '@testuser:localhost', | ||
| activeRealms: [testRealmURL], | ||
| autostart: true, | ||
| systemCardAccountData: { id: null }, | ||
| }); |
There was a problem hiding this comment.
The new test passes systemCardAccountData: { id: null } into setupMockMatrix, but Config.systemCardAccountData is still typed { id?: string } in tests/helpers/mock-matrix.ts (line 26). With strictNullChecks enabled in packages/host/tsconfig.json, this value is not assignable and pnpm lint:glint/TypeScript compilation of the host tests will fail before runtime. Please allow null in the config type (or avoid passing it) so the test suite continues to type-check.
Useful? React with 👍 / 👎.
Preview deployments |
Host Test Results 1 files ± 0 1 suites ±0 1h 31m 0s ⏱️ + 2m 26s For more details on these errors, see this check. Results for commit 357b817. ± Comparison against base commit f30d04d. |
There was a problem hiding this comment.
Pull request overview
This PR enables explicit null values for the system card configuration to completely disable system cards, primarily to quiet test output. The change distinguishes between undefined (use default) and null (no system card).
Key Changes:
- Updated the
setSystemCardmethod to handle null as an explicit signal to remove the system card - Modified type signatures to allow
id?: string | nullin the system card account data configuration - Added test configuration to use
{ id: null }to disable system cards in tests
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/host/app/services/matrix-service.ts |
Implements null handling in setSystemCard to explicitly disable system cards when null is provided |
packages/host/tests/helpers/mock-matrix.ts |
Updates the Config type to allow null values for the system card id |
packages/host/tests/integration/commands/open-workspace-test.gts |
Configures the test to use systemCardAccountData: { id: null } to disable system cards |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private async setSystemCard(systemCardId: string | undefined | null) { | ||
| // Set the system card to use | ||
| // If there is none, we fall back to the default | ||
| // If it is null, we remove any current system card | ||
| // If it is undefined, we fall back to the default | ||
| if (systemCardId === null) { | ||
| // explicit null means no system card | ||
| this.store.dropReference(this._systemCard?.id); | ||
| this._systemCard = undefined; | ||
| return; | ||
| } |
There was a problem hiding this comment.
The setUserSystemCard function (line 1979) should also accept null as a parameter to allow users to explicitly disable the system card. Currently, it only accepts string | undefined, which is inconsistent with the private setSystemCard function that now accepts null. This creates an API inconsistency where the internal implementation supports null but the public-facing method doesn't expose this capability.
1 participant
This lets us turn off system cards entirely, and allows a quick way of quieting the tests.