feat(dev-server): add log buffer and logs tab to DevTools#1455
feat(dev-server): add log buffer and logs tab to DevTools#1455KanchanWaldia wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR introduces a Logs tab to DevTools with buffered log rendering and scroll control, plus adds ASCII-mode tests for the Gauge widget. DevTools now captures stdout/stderr output, maintains a 100-entry buffer, and displays logs in the panel with viewport scrolling. Gauge tests extend coverage for NO_UNICODE environment configuration. ChangesDevTools Logs Tab Feature
Gauge ASCII Mode Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
area:widgets
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/dev-server/src/devtools.test.ts (1)
53-53: 💤 Low valueRemove leftover development comment.
The
// ← ADD THIScomment appears to be a development artifact that should be cleaned up.Proposed fix
- dt.setTab('logs'); // ← ADD THIS + dt.setTab('logs');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dev-server/src/devtools.test.ts` at line 53, Remove the leftover development inline comment after the dt.setTab('logs') call: delete the " // ← ADD THIS" text so the line reads simply dt.setTab('logs'); (no other behavior changes required).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/dev-server/src/devtools.test.ts`:
- Around line 40-48: The test in DevTools (methods appendLog, setTab, scrollLog,
getPanel) calls scrollLog(-5) which clamps at 0 and doesn't prove offset
behavior; change the test to move the offset away from 0 (e.g., call
dt.scrollLog(5) or another positive value after appending 20 lines) and then
call dt.getPanel(...) and assert the panel reflects that offset (for example
expect it to contain 'line 5' and/or not contain 'line 0') so the test actually
verifies that scroll offset affects visible content.
In `@packages/widgets/src/data/Gauge.test.ts`:
- Around line 37-43: Replace the direct mutation of process.env and the
redundant getValue assertion with a rendering test that spies on the capability
flag and uses a real Screen: use vi.spyOn(capabilitiesModule,
'unicode').mockReturnValue(false) (or vi.spyOn(caps, 'unicode') depending on the
import) before creating the Gauge('CPU'), create a Screen, call g.setValue(0.5)
and render the widget to the Screen, then assert the rendered output contains
ASCII characters like '#' or '-' and does not contain full-block chars like '█';
use beforeEach/afterEach to create/teardown the Screen and restore the spy
(spy.mockRestore()) to avoid side-effects and remove any process.env.NO_UNICODE
manipulation and the duplicate getValue() assertion.
- Around line 31-35: The test currently mutates process.env and only asserts
that mutation instead of verifying observable behavior; replace it by spying and
rendering: use vi.spyOn to mock the caps value exported from the env-caps module
(spy on the module's caps or the function that computes it so caps.unicode
returns false), render the Gauge with a real Screen instance, call updateRect()
and render(), and assert the Screen output contains ASCII-only characters (or
that the Gauge output matches an expected ASCII snapshot). Use beforeEach to set
up the spy and create the Screen/Gauge, and afterEach to restore spies and
destroy the Screen (vi.restoreAllMocks()/spy.mockRestore()) to avoid leaking
state; reference caps.unicode, vi.spyOn, Screen, updateRect(), render(),
beforeEach, and afterEach in your changes.
---
Nitpick comments:
In `@packages/dev-server/src/devtools.test.ts`:
- Line 53: Remove the leftover development inline comment after the
dt.setTab('logs') call: delete the " // ← ADD THIS" text so the line reads
simply dt.setTab('logs'); (no other behavior changes required).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 30823af8-be99-4f4d-9037-1c498fba0865
📒 Files selected for processing (3)
packages/dev-server/src/devtools.test.tspackages/dev-server/src/devtools.tspackages/widgets/src/data/Gauge.test.ts
| it('getPanel respects scroll offset', () => { | ||
| const dt = new DevTools(); | ||
| for (let i = 0; i < 20; i++) dt.appendLog(`line ${i}`); | ||
| dt.setTab('logs'); | ||
| dt.scrollLog(-5); | ||
| const panel = dt.getPanel(40, 10); | ||
| const joined = panel.join('\n'); | ||
| expect(joined).toContain('line 0'); | ||
| }); |
There was a problem hiding this comment.
Test does not verify scroll offset behavior.
Calling scrollLog(-5) when the offset is already 0 clamps to 0, so the panel shows line 0 regardless of the scroll call. This test passes trivially without proving that scroll offset affects panel content.
To properly test that scroll offset changes visible output:
Proposed fix
it('getPanel respects scroll offset', () => {
const dt = new DevTools();
for (let i = 0; i < 20; i++) dt.appendLog(`line ${i}`);
dt.setTab('logs');
- dt.scrollLog(-5);
+ dt.scrollLog(10);
const panel = dt.getPanel(40, 10);
const joined = panel.join('\n');
- expect(joined).toContain('line 0');
+ expect(joined).not.toContain('line 0');
+ expect(joined).toContain('line 10');
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('getPanel respects scroll offset', () => { | |
| const dt = new DevTools(); | |
| for (let i = 0; i < 20; i++) dt.appendLog(`line ${i}`); | |
| dt.setTab('logs'); | |
| dt.scrollLog(-5); | |
| const panel = dt.getPanel(40, 10); | |
| const joined = panel.join('\n'); | |
| expect(joined).toContain('line 0'); | |
| }); | |
| it('getPanel respects scroll offset', () => { | |
| const dt = new DevTools(); | |
| for (let i = 0; i < 20; i++) dt.appendLog(`line ${i}`); | |
| dt.setTab('logs'); | |
| dt.scrollLog(10); | |
| const panel = dt.getPanel(40, 10); | |
| const joined = panel.join('\n'); | |
| expect(joined).not.toContain('line 0'); | |
| expect(joined).toContain('line 10'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/dev-server/src/devtools.test.ts` around lines 40 - 48, The test in
DevTools (methods appendLog, setTab, scrollLog, getPanel) calls scrollLog(-5)
which clamps at 0 and doesn't prove offset behavior; change the test to move the
offset away from 0 (e.g., call dt.scrollLog(5) or another positive value after
appending 20 lines) and then call dt.getPanel(...) and assert the panel reflects
that offset (for example expect it to contain 'line 5' and/or not contain 'line
0') so the test actually verifies that scroll offset affects visible content.
| it('caps.unicode is false when NO_UNICODE=1', () => { | ||
| process.env.NO_UNICODE = '1'; | ||
| expect(process.env.NO_UNICODE).toBe('1'); | ||
| delete process.env.NO_UNICODE; | ||
| }); |
There was a problem hiding this comment.
Test violates guidelines and doesn't verify observable behavior.
This test has several critical issues:
-
Violates coding guideline: The guideline explicitly states "To test
caps.unicodeorcaps.motionbehavior, usevi.spyOn, never direct mutation." Settingprocess.env.NO_UNICODEdirectly is forbidden. -
Doesn't test observable behavior: The test only asserts
expect(process.env.NO_UNICODE).toBe('1'), which is a placeholder test. It doesn't verify thatcaps.unicodeis actually false or that the Gauge renders ASCII characters. -
Missing Screen rendering: The guideline requires "Tests must use the real
Screen... Render widgets in tests usingScreen,updateRect(), andrender()." To verify ASCII mode, you must render the widget and check the output. -
Incorrect approach:
caps.unicodeis determined at module load time (seepackages/core/src/terminal/env-caps.ts). Settingprocess.env.NO_UNICODEafter the module is imported won't changecaps.unicode. -
Missing cleanup structure:
beforeEach/afterEachwere imported but not used. Inline cleanup withdeletecan leak state if the test throws.
✅ Correct approach using vi.spyOn and Screen
- it('caps.unicode is false when NO_UNICODE=1', () => {
- process.env.NO_UNICODE = '1';
- expect(process.env.NO_UNICODE).toBe('1');
- delete process.env.NO_UNICODE;
- });
+ it('renders ASCII characters when caps.unicode is false', () => {
+ vi.spyOn(caps, 'unicode', 'get').mockReturnValue(false);
+
+ const screen = new Screen();
+ screen.updateRect({ x: 0, y: 0, width: 30, height: 5 });
+
+ const g = new Gauge('CPU');
+ g.setValue(0.5);
+ g.updateRect({ x: 0, y: 0, width: 30, height: 1 });
+ g.render(screen);
+
+ // Verify ASCII chars '#' and '-' are used instead of '█' and '░'
+ const output = screen.toString();
+ expect(output).toContain('#');
+ expect(output).toContain('-');
+ expect(output).not.toContain('█');
+ expect(output).not.toContain('░');
+
+ vi.restoreAllMocks();
+ });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/widgets/src/data/Gauge.test.ts` around lines 31 - 35, The test
currently mutates process.env and only asserts that mutation instead of
verifying observable behavior; replace it by spying and rendering: use vi.spyOn
to mock the caps value exported from the env-caps module (spy on the module's
caps or the function that computes it so caps.unicode returns false), render the
Gauge with a real Screen instance, call updateRect() and render(), and assert
the Screen output contains ASCII-only characters (or that the Gauge output
matches an expected ASCII snapshot). Use beforeEach to set up the spy and create
the Screen/Gauge, and afterEach to restore spies and destroy the Screen
(vi.restoreAllMocks()/spy.mockRestore()) to avoid leaking state; reference
caps.unicode, vi.spyOn, Screen, updateRect(), render(), beforeEach, and
afterEach in your changes.
Source: Coding guidelines
| it('getValue returns 0.5 after setValue(0.5) in ASCII mode', () => { | ||
| process.env.NO_UNICODE = '1'; | ||
| const g = new Gauge('CPU'); | ||
| g.setValue(0.5); | ||
| expect(g.getValue()).toBe(0.5); | ||
| delete process.env.NO_UNICODE; | ||
| }); |
There was a problem hiding this comment.
Test doesn't verify ASCII mode rendering and violates guidelines.
This test has critical issues:
-
Violates coding guideline: "To test
caps.unicodeorcaps.motionbehavior, usevi.spyOn, never direct mutation." Direct mutation ofprocess.env.NO_UNICODEis forbidden. -
Doesn't test ASCII mode: The test only checks
getValue()returns0.5, which is unaffected by unicode mode.getValue()/setValue()behavior is already tested in lines 15-23. ASCII mode affects rendering (whether the gauge uses#/-or█/░), not the internal value. -
Missing Screen usage: To verify ASCII mode, you must render the widget with a real
Screenand check that ASCII characters appear in the output. -
Incorrect approach: Setting
process.env.NO_UNICODEafter module import won't changecaps.unicode(it's determined at load time). -
Missing cleanup structure:
beforeEach/afterEachwere imported but not used.
✅ Correct approach to test ASCII mode rendering
- it('getValue returns 0.5 after setValue(0.5) in ASCII mode', () => {
- process.env.NO_UNICODE = '1';
- const g = new Gauge('CPU');
- g.setValue(0.5);
- expect(g.getValue()).toBe(0.5);
- delete process.env.NO_UNICODE;
- });
+ it('renders filled ASCII bars with # when unicode is disabled', () => {
+ vi.spyOn(caps, 'unicode', 'get').mockReturnValue(false);
+
+ const screen = new Screen();
+ screen.updateRect({ x: 0, y: 0, width: 30, height: 5 });
+
+ const g = new Gauge('CPU', {}, {});
+ g.setValue(0.75); // 75% filled
+ g.updateRect({ x: 0, y: 0, width: 30, height: 1 });
+ g.render(screen);
+
+ const output = screen.toString();
+ // Verify filled portion uses '#' and empty portion uses '-'
+ expect(output).toMatch(/##+.*-+/);
+
+ vi.restoreAllMocks();
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('getValue returns 0.5 after setValue(0.5) in ASCII mode', () => { | |
| process.env.NO_UNICODE = '1'; | |
| const g = new Gauge('CPU'); | |
| g.setValue(0.5); | |
| expect(g.getValue()).toBe(0.5); | |
| delete process.env.NO_UNICODE; | |
| }); | |
| it('renders filled ASCII bars with # when unicode is disabled', () => { | |
| vi.spyOn(caps, 'unicode', 'get').mockReturnValue(false); | |
| const screen = new Screen(); | |
| screen.updateRect({ x: 0, y: 0, width: 30, height: 5 }); | |
| const g = new Gauge('CPU', {}, {}); | |
| g.setValue(0.75); // 75% filled | |
| g.updateRect({ x: 0, y: 0, width: 30, height: 1 }); | |
| g.render(screen); | |
| const output = screen.toString(); | |
| // Verify filled portion uses '#' and empty portion uses '-' | |
| expect(output).toMatch(/##+.*-+/); | |
| vi.restoreAllMocks(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/widgets/src/data/Gauge.test.ts` around lines 37 - 43, Replace the
direct mutation of process.env and the redundant getValue assertion with a
rendering test that spies on the capability flag and uses a real Screen: use
vi.spyOn(capabilitiesModule, 'unicode').mockReturnValue(false) (or
vi.spyOn(caps, 'unicode') depending on the import) before creating the
Gauge('CPU'), create a Screen, call g.setValue(0.5) and render the widget to the
Screen, then assert the rendered output contains ASCII characters like '#' or
'-' and does not contain full-block chars like '█'; use beforeEach/afterEach to
create/teardown the Screen and restore the spy (spy.mockRestore()) to avoid
side-effects and remove any process.env.NO_UNICODE manipulation and the
duplicate getValue() assertion.
Source: Coding guidelines
Karanjot786
added
gssoc:approved
KanchanWaldia
force-pushed
the
feat/devtools-log-panel
branch
from
e502762 to
07f0dff
Compare
|
Hi @Karanjot786, I accidentally closed the PR. Could you help reopen it? Sorry for the inconvenience. |
|
Heads-up: this PR currently has an empty diff.
Looks like the branch was reset/force-updated and lost its commits. Please push the actual feature commits, then re-request review. Thanks! |
2 participants
Description
Added appendLog, scrollLog, logs getter, and a logs tab to DevTools, following the existing _eventLog buffer pattern.
Related Issue
Closes #508
Which package(s)?
@termuijs/dev-server
Type of Change
type:bug)type:feature)type:docs)type:testing)type:refactor)type:design)type:accessibility)type:performance)type:devops)type:security)Checklist
needs-starcheck blocks your merge otherwise.bun vitest runbun run buildbun run typecheckCONTRIBUTING.md.type: short description.markDirty()(if your change affects rendering).anytypes without an inline comment explaining why.GSSoC 2026 Participation
https://gssoc.girlscript.org/profile/9d129ba7-b2c3-46e7-aa8a-9557f15eef5f