Fix starship rendering regressions with clean diff and PTY debug runbook#225
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds Live PTY debugging documentation and runbooks; emits frame-audit events from table rendering; extends native frame/perf audit codes and worker parsing/emit paths; updates pagination vnode structure and tests; applies layout height/flex changes across several Starship screens; refactors theme color helpers and updates tests; guards native vendor gitlink reading and adds related tests. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Renderer / UI
participant Node as Node (frameAudit)
participant Worker as Engine Worker
participant Native as Native Audit Source
UI->>Node: emitFrameAudit(table layout & metrics)
Node->>Worker: enqueue/read native audit queue (FRAME/PERF mask)
Worker->>Native: request per-record payloads (frame / perf records)
Native-->>Worker: payload bytes (frame summary / perf timing / diffPath)
Worker->>Node: emit native.frame.summary / native.perf.timing / native.perf.diffPath
Node-->>UI: frame-audit events surfaced for diagnostics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0292100eb
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/node/src/worker/engineWorker.ts (1)
435-442: Scope native debug capture to the intended categories.You query using
NATIVE_FRAME_AUDIT_CATEGORY_MASK, but enable currently captures all categories. Using the same mask at enable-time will reduce ring pressure and dropped relevant records.♻️ Suggested change
- categoryMask: 0xffff_ffff, + categoryMask: NATIVE_FRAME_AUDIT_CATEGORY_MASK,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/node/src/worker/engineWorker.ts` around lines 435 - 442, The engineDebugEnable call is enabling capture for all native categories (categoryMask: 0xffff_ffff) which increases ring pressure; change it to use the intended NATIVE_FRAME_AUDIT_CATEGORY_MASK constant so only desired categories are captured. Update the categoryMask parameter in the native.engineDebugEnable call (the block initializing debug via engineDebugEnable(engineId, {...})) to NATIVE_FRAME_AUDIT_CATEGORY_MASK while leaving other params (ringCapacity/FRAME_AUDIT_NATIVE_RING_BYTES, captureDrawlistBytes, etc.) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/dev/live-pty-debugging.md`:
- Around line 23-26: Replace the hard-coded path "/home/k3nig/Rezi" used before
the "npx tsc -b packages/core packages/node packages/create-rezi" command in
docs/dev/live-pty-debugging.md with a generic repo-root placeholder (e.g.
"<repo-root>" or "$REPO_ROOT"); update the other occurrence of the same pattern
later in the file so all "cd /home/k3nig/Rezi" run commands are portable and
reference the repository root generically.
In `@packages/core/src/renderer/renderToDrawlist/widgets/collections.ts`:
- Line 31: The import ordering in
packages/core/src/renderer/renderToDrawlist/widgets/collections.ts is incorrect
and causing organizeImports to fail; run the repository's import
organizer/formatter (or your editor's "Organize Imports" action) on this file
and commit the reordered imports so the import list (including symbols like
emitFrameAudit and FRAME_AUDIT_ENABLED) follows the project's configured
ordering and lint rules.
In `@packages/node/src/worker/engineWorker.ts`:
- Around line 12-25: The import/formatting in this file is out of sync; run the
project's formatter and lint auto-fixers (e.g., npm/yarn script for format and
eslint --fix) and commit the resulting changes so imports and spacing match the
repo style; focus on normalizing the import block that includes symbols like
FRAME_AUDIT_NATIVE_ENABLED, FRAME_AUDIT_NATIVE_RING_BYTES, ZR_DEBUG_CAT_FRAME,
ZR_DEBUG_CAT_DRAWLIST, ZR_DEBUG_CAT_PERF and the ZR_DEBUG_CODE_* constants (also
check the similar region around the 604-651 change) to ensure sorted/organized
imports and consistent formatting before pushing.
---
Nitpick comments:
In `@packages/node/src/worker/engineWorker.ts`:
- Around line 435-442: The engineDebugEnable call is enabling capture for all
native categories (categoryMask: 0xffff_ffff) which increases ring pressure;
change it to use the intended NATIVE_FRAME_AUDIT_CATEGORY_MASK constant so only
desired categories are captured. Update the categoryMask parameter in the
native.engineDebugEnable call (the block initializing debug via
engineDebugEnable(engineId, {...})) to NATIVE_FRAME_AUDIT_CATEGORY_MASK while
leaving other params (ringCapacity/FRAME_AUDIT_NATIVE_RING_BYTES,
captureDrawlistBytes, etc.) unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
AGENTS.mdCLAUDE.mddocs/dev/live-pty-debugging.mddocs/dev/testing.mdmkdocs.ymlpackages/core/src/renderer/renderToDrawlist/widgets/collections.tspackages/core/src/widgets/__tests__/pagination.test.tspackages/core/src/widgets/pagination.tspackages/create-rezi/templates/starship/src/screens/bridge.tspackages/create-rezi/templates/starship/src/screens/cargo.tspackages/create-rezi/templates/starship/src/screens/comms.tspackages/create-rezi/templates/starship/src/screens/crew.tspackages/create-rezi/templates/starship/src/screens/engineering.tspackages/create-rezi/templates/starship/src/screens/settings.tspackages/create-rezi/templates/starship/src/theme.tspackages/node/src/frameAudit.tspackages/node/src/worker/engineWorker.ts
packages/core/src/renderer/renderToDrawlist/widgets/collections.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/dev/live-pty-debugging.md (1)
38-39: Optional: Add comment to clarify file truncation idiom.The
: >syntax is valid bash for creating/truncating files, but a brief comment would help developers less familiar with this idiom.📝 Optional clarity improvement
+# Truncate/create empty log files before run : > /tmp/rezi-frame-audit.ndjson : > /tmp/starship.log🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/dev/live-pty-debugging.md` around lines 38 - 39, Add a short explanatory comment above the two truncation lines that use the bash idiom ": > /tmp/rezi-frame-audit.ndjson" and ": > /tmp/starship.log" to clarify they create or truncate the files; update the docs/dev/live-pty-debugging.md snippet by inserting one brief sentence like "Use ': > <file>' to create or truncate a file" immediately before those two lines so readers unfamiliar with the ": >" idiom understand its purpose.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/node/src/worker/engineWorker.ts`:
- Around line 606-631: Create a shared helper (e.g., tryReadDebugPayload) used
by engineDebugGetPayload calls to centralize payload reads and error handling:
have it attempt the existing read, catch exceptions and emit the same
native.debug.payload_error via frameAudit.emit (or the appropriate audit
emitter) and additionally detect short reads (0 < wrote < expected) and emit
native.debug.payload_error with context (reason, recordId.toString(), expected
bytes, wrote bytes) instead of silently dropping; then replace the five inline
payload-read branches (the calls around engineDebugGetPayload) to call this
helper and, on success, proceed to the existing summary emit (using
DEBUG_FRAME_RECORD_BYTES, frameAudit.emit, nativeFrameCodeName, u64FromView,
etc.).
---
Nitpick comments:
In `@docs/dev/live-pty-debugging.md`:
- Around line 38-39: Add a short explanatory comment above the two truncation
lines that use the bash idiom ": > /tmp/rezi-frame-audit.ndjson" and ": >
/tmp/starship.log" to clarify they create or truncate the files; update the
docs/dev/live-pty-debugging.md snippet by inserting one brief sentence like "Use
': > <file>' to create or truncate a file" immediately before those two lines so
readers unfamiliar with the ": >" idiom understand its purpose.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/dev/live-pty-debugging.mdpackages/core/src/renderer/renderToDrawlist/widgets/collections.tspackages/node/src/worker/engineWorker.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/renderer/renderToDrawlist/widgets/collections.ts
Summary
Rgb24values so app startup/tests are valid on currentmaintable.layout) to accelerate regression triageAGENTS.md,CLAUDE.md, and developer testing docsValidation
npx tsc -b packages/core packages/create-rezi packages/nodenpx tsx --test packages/core/src/widgets/__tests__/pagination.test.tsnpx tsx --test packages/create-rezi/templates/starship/src/__tests__/render.test.ts packages/create-rezi/templates/starship/src/__tests__/reducer.test.ts packages/create-rezi/templates/starship/src/__tests__/keybindings.test.tsdocs/dev/live-pty-debugging.md)Notes
main.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests / Chores