Skip to content

Explicit null for the system card means no card set #3724

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.

Sign up for GitHub

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

Open
IanCal wants to merge 2 commits into
base: main
Choose a base branch
from
+11 −3
Open

Explicit null for the system card means no card set #3724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cde0045
Explicit null for the system card means no card set
IanCal Dec 16, 2025
357b817
Support nulls in the type
IanCal Dec 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions packages/host/app/services/matrix-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1947,9 +1947,16 @@ export default class MatrixService extends Service {
return this._systemCard;
}

private async setSystemCard(systemCardId: string | undefined) {
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;
}
Comment on lines +1950 to +1959
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
if (!systemCardId) {
systemCardId = ENV.defaultSystemCardId;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/host/tests/helpers/mock-matrix.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export interface Config {
autostart?: boolean;
now?: () => number;
directRooms?: string[];
systemCardAccountData?: { id?: string };
systemCardAccountData?: { id?: string | null };
}

export function setupMockMatrix(
Expand Down
1 change: 1 addition & 0 deletions packages/host/tests/integration/commands/open-workspace-test.gts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ module('Integration | commands | open-workspace', function (hooks) {
loggedInAs: '@testuser:localhost',
activeRealms: [testRealmURL],
autostart: true,
systemCardAccountData: { id: null },
});
Comment on lines 21 to 25
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Dec 16, 2025

Choose a reason for hiding this comment

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

P1 Badge Null systemCardAccountData breaks type checks

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 👍 / 👎.


hooks.beforeEach(async function () {
Expand Down
Loading