Fix terminal special character rendering#2152
Conversation
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/code/src/main/services/shell/service.test.ts:297-307
**Redundant test — encoding already asserted elsewhere**
The existing "creates a new shell session" test (line 189) was also updated in this PR to include `encoding: "utf8"` in its `objectContaining` assertion, making this standalone test a duplicate. Both tests call `service.create("session-1")` and assert the same `encoding` property on the same spawn call, violating OnceAndOnlyOnce. The new test can be removed; the encoding expectation is already covered.
Reviews (1): Last reviewed commit: "Fix terminal special character rendering" | Re-trigger Greptile |
| it("decodes pty output as UTF-8 text", async () => { | ||
| await service.create("session-1"); | ||
|
|
||
| expect(mockPty.spawn).toHaveBeenCalledWith( | ||
| expect.any(String), | ||
| expect.any(Array), | ||
| expect.objectContaining({ | ||
| encoding: "utf8", | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Redundant test — encoding already asserted elsewhere
The existing "creates a new shell session" test (line 189) was also updated in this PR to include encoding: "utf8" in its objectContaining assertion, making this standalone test a duplicate. Both tests call service.create("session-1") and assert the same encoding property on the same spawn call, violating OnceAndOnlyOnce. The new test can be removed; the encoding expectation is already covered.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/services/shell/service.test.ts
Line: 297-307
Comment:
**Redundant test — encoding already asserted elsewhere**
The existing "creates a new shell session" test (line 189) was also updated in this PR to include `encoding: "utf8"` in its `objectContaining` assertion, making this standalone test a duplicate. Both tests call `service.create("session-1")` and assert the same `encoding` property on the same spawn call, violating OnceAndOnlyOnce. The new test can be removed; the encoding expectation is already covered.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Good catch, thanks. I removed the redundant test and kept the encoding assertion in the existing session creation test. Checks are passing.
ca27a98 to
a4d94cc
Compare
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/code/src/main/services/shell/service.test.ts:188-203
**`createCommandSession` encoding untested; parameterised test preferred**
The PR adds `encoding: PTY_ENCODING` to both `create` (interactive shell) and `createCommandSession` (line 219 of `service.ts`), but the test assertion only covers the `create` path — `createCommandSession` has no test coverage at all. Per the team's "Expresses every idea" rule and preference for parameterised tests, a single `it.each` over both spawn call sites would verify the encoding fix uniformly without duplicating setup.
Reviews (2): Last reviewed commit: "Fix terminal special character rendering" | Re-trigger Greptile |
a4d94cc to
98a827a
Compare
|
Updated the test coverage to use a parameterized test for both create and createCommandSession, so the PTY UTF-8 encoding option is asserted on both spawn paths without duplicating the create-session test. Focused shell service test and Biome check are passing locally. |
|
Updated to cover both PTY spawn paths with a single parameterized test, including createCommandSession. The focused shell service test and Biome check pass locally. |
|
Reviews (3): Last reviewed commit: "Fix terminal special character rendering" | Re-trigger Greptile |
Summary
Fixes #1789.
Tests