feat(web): on-page verbose debug log + surface chunk tags - #72
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds an on-page verbose debug log panel to the web playground: an HTML ChangesDebug Log System
Sequence DiagramsequenceDiagram
participant User
participant loadFile
participant arrange
participant dbg
participant log
participant debugLog as pre#debugLog
User->>loadFile: upload file
loadFile->>dbg: dbg("file", metadata)
dbg->>log: push entry with timestamp
log-->>debugLog: update textContent
loadFile->>dbg: dbg("loaded", summary)
dbg->>log: push entry with timestamp
log-->>debugLog: update textContent
loadFile->>arrange: arrange(true)
arrange->>dbg: dbg("arrange", trace) if logIt true
dbg->>log: push entry with timestamp
log-->>debugLog: update textContent
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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.
Reviewed commit: b817dc1512
ℹ️ 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".
| // Appends one timestamped line to the on-page debug log. `data` (optional) is | ||
| // JSON-stringified, so the exact inputs/outputs of each engine call are visible | ||
| // without opening the browser console — Copy the panel straight into a report. | ||
| function dbg(label, data) { |
There was a problem hiding this comment.
This commit introduces the non-trivial debug-log UI/behavior in a single implementation commit with no preceding failing test commit in the reviewed sequence. /workspace/griff/AGENTS.md requires “Every new module or non-trivial change follows the red-green cycle strictly” and says reviewers must judge the commit sequence, so this needs to be reworked into a red test commit followed by the green implementation commit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Holding on this one — I checked with the maintainer first, who defers to repo precedent here.
Red-green in this repo is enforced through cargo test on the Rust engine, where the public API and the test harness actually live: core/tests/* gate every engine change (e.g. the split seam in #71 landed as a red characterization test → green impl). This PR, by contrast, is browser-only glue — app.js / index.html / style.css, with no Rust, no new public API, and the repo has no JS test harness (web CI is just web/build.sh + a Pages deploy; there's no JS unit lane). The prior frontend-only PRs merged the same way — #67 (capture UI) and #71's pager were not red-green'd in JS.
Standing up a jsdom/vitest lane purely to TDD a diagnostic logging panel would be disproportionate to the change. If we do want JS unit tests as a general capability, that's better as its own infra PR than as a gate on this one — happy to open that separately if the maintainer wants the harness first.
Generated by Claude Code
There was a problem hiding this comment.
Update — reversed course on this: the maintainer opted to stand up the JS test lane, so I've done exactly that and reworked the PR into a red→green sequence.
eb9b5bb(red) — adds the repo's first JS unit-test lane:web/package.json(npm test→node --test, zero deps), a characterization spec for the debug-log ring buffer (web/test/debuglog.test.js), and a CI workflow (.github/workflows/web-test.yml) running it on PRs/main touchingweb/. The module under test doesn't exist yet → fails.5eeec05(green) — implementsweb/static/debuglog.js(pure, DOM-free ring buffer: bounded, timestamped, JSON payloads, ✗-prefixed errors) so the spec passes, then wires it into the panel.
Verified in isolated worktrees: the red commit's node --test exits 1, the green commit exits 0 (7/7). The DOM/wasm-coupled panel wiring stays glue (uncovered by design) — the pure, testable unit is the buffer. Thanks for pushing on this.
Generated by Claude Code
| if (splitChunks.length === 0) { resetSplit(); capMsg('no sounding phrases to split', true); return; } | ||
| if (splitChunks.length === 0) { resetSplit(); capMsg('no sounding phrases to split', true); dbg('split result', { phrases: 0 }); return; } | ||
| let resolvedTags = []; | ||
| try { resolvedTags = JSON.parse(splitChunks[0].chunk).tags || []; } catch (_) { /* leave empty */ } |
There was a problem hiding this comment.
When a split track has notation-derived technique tags only in later phrases, this samples only splitChunks[0], so the copied debug log can still report resolvedTags: [] even though exported phrase chunks contain tags. That undercuts the new tag-diagnosis path for exactly the multi-phrase cases it is meant to explain; aggregate tags across chunks or log tags per phrase instead of reading only the first chunk.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bc067ea — split result now logs tags per phrase (array aligned with ids), so notation-derived technique tags that appear only in later phrases show up instead of being hidden behind splitChunks[0]. The pager already shows the per-phrase tag count; the log now matches it. Good catch.
Generated by Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@web/static/app.js`:
- Around line 95-103: The arrange function calls JSON.parse on the wasmArrange
output without error handling, which will crash the UI if the engine returns
malformed or partial JSON before the status/debug error handling can execute.
Wrap the JSON.parse call that assigns to the current variable in a try/catch
block, and in the catch handler, call dbgErr with the parse error details and
appropriate context (including the mode, seed, offset, variation, track
parameters) similar to how the current error handling works, then return early
from the function to prevent further execution with invalid state.
In `@web/static/style.css`:
- Line 200: Replace the deprecated `word-break: break-word` declaration in the
style.css file with the modern standard property `overflow-wrap: anywhere`.
Optionally, you can also add `word-break: normal` alongside `overflow-wrap:
anywhere` for defensive clarity, but `overflow-wrap: anywhere` alone is
sufficient. This change addresses the Stylelint deprecation warning and aligns
with the CSS Working Group's recommended approach for handling word breaking
behavior.
🪄 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: 8c6e9deb-127e-43e3-95fd-d5e971c809a9
📒 Files selected for processing (3)
web/static/app.jsweb/static/index.htmlweb/static/style.css
Stands up the repo's first JS unit-test lane (node:test, zero deps) and a characterization test for the playground's debug-log buffer: bounded ring, timestamped lines, JSON-encoded payloads, and ✗-prefixed errors. The module under test (static/debuglog.js) does not exist yet — this is the red phase; the next commit implements it green. - web/package.json: `npm test` -> `node --test` - web/test/debuglog.test.js: the (currently failing) spec - .github/workflows/web-test.yml: run it on PRs/main touching web/
Implements static/debuglog.js (pure, DOM-free ring buffer) so the red test from the previous commit passes, then wires it into the playground: - Collapsible Debug log panel (Copy/Clear) tracing every engine call — load, track, detect, capture, split, arrange, and all errors — with inputs and resolved outputs. Copy-paste friendly on a phone; no devtools needed. - Tags made visible: capture/split traces print resolved tags/techniques (per phrase), the pager shows a per-phrase tag count, and the legend notes technique tags are auto-added from notation (ADR-0018) — so an empty `tags` is self-explanatory rather than a surprise. - arrange() guards its JSON parse like the other engine calls; .debuglog uses overflow-wrap: anywhere (not the deprecated word-break: break-word). Static front-end only; no engine/Rust change.
88e3672 to
5eeec05
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/web-test.yml:
- Around line 16-26: The test job executes PR code via npm test while the
actions/checkout step retains credentials by default, creating a
token-exfiltration risk. Add a permissions block at the job level specifying
least-privilege access (likely contents: read only), and modify the
actions/checkout@v4 step to include persist-credentials: false to disable
credential persistence in the workflow environment.
In `@web/static/debuglog.js`:
- Around line 8-27: The `max` parameter in the `createDebugLog` function is not
validated, causing the ring-buffer logic in the `push` function to fail when
`max = 0` (where `lines.slice(-0)` returns an empty array) or for negative
values, breaking the bounded growth contract. Add validation at the start of
`createDebugLog` to ensure `max` is a positive number (greater than 0), either
by throwing an error for invalid values or enforcing a minimum value such as 1.
🪄 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: 2e881efc-4bf2-47d4-a77d-fabac51d4685
📒 Files selected for processing (7)
.github/workflows/web-test.ymlweb/package.jsonweb/static/app.jsweb/static/debuglog.jsweb/static/index.htmlweb/static/style.cssweb/test/debuglog.test.js
✅ Files skipped from review due to trivial changes (1)
- web/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- web/static/index.html
- web/static/style.css
- web/static/app.js
- createDebugLog now rejects a non-positive/non-integer max: slice(-0) is a no-op that would silently disable the bound. Add a test for the guard. - web-test.yml: least-privilege permissions (contents: read) and persist-credentials: false, since npm test runs PR-authored code (CodeRabbit /zizmor: artipacked). Actions stay tag-pinned, consistent with web.yml.
2 participants
Follow-up to #71, from playground feedback that tags looked like they "weren't delivered." Diagnosis: no engine bug — structural tags are a manual curator choice, and only notation-derived technique tags auto-fill (ADR-0018), so an un-ticked track with no techniques yields
tags: [](correct, just opaque). This makes the engine's behaviour visible.What
Debug-log ring buffer + the repo's first JS test lane
web/static/debuglog.js— a pure, DOM-free bounded ring buffer (timestamped lines, JSON-encoded payloads, ✗-prefixed errors), kept separate fromapp.jsprecisely so it's unit-testable.web/test/debuglog.test.jsundernode --test(zero deps), wired into CI via.github/workflows/web-test.yml(runs on PRs/main touchingweb/).eb9b5bbadds the failing spec + lane;5eeec05implements the module green. Verified in isolated worktrees — red commitnode --testexits 1, green exits 0 (7/7).On-page Debug log panel
tags/techniques(per phrase), the pager shows a per-phrase tag count, and the legend notes technique tags are auto-added from notation — so an emptytagsis self-explanatory.arrange()guards its JSON parse like the other engine calls;.debuglogusesoverflow-wrap: anywhere(not the deprecatedword-break: break-word).Scope
Static front-end + test tooling only — no Rust/engine change. The pure buffer is unit-tested; the DOM/wasm-coupled panel wiring stays glue (uncovered by design — there's nothing engine-shaped to pin there).
Validation
npm test(inweb/) → 7/7 green;node --checkonapp.js/debuglog.jsclean.web/distis gitignored — CI'sweb/build.shrebuilds it fromstatic/(and shipsdebuglog.js); deploys to Pages on merge tomain.web/test/andpackage.jsonstay out of the shipped bundle.🤖 Generated with Claude Code
https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
Summary by CodeRabbit