Skip to content

feat: DH-23321: Persist Browser Logs to IndexedDB - #2770

Open
dgodinez-dh wants to merge 14 commits into
deephaven:mainfrom
dgodinez-dh:dag_IndexedDbLogs
Open

dgodinez-dh wants to merge 14 commits into
deephaven:mainfrom
dgodinez-dh:dag_IndexedDbLogs

Conversation

@dgodinez-dh

Copy link
Copy Markdown
Contributor
  • writes log statements to IndexedDB to persist beyond browser refresh
  • uses log proxy like existing LogHistory
  • prunes old log statements to avoid going over storage quota
  • unit tests
  • hand tested in Core and Enterprise

@dgodinez-dh
dgodinez-dh requested a balanced review from Copilot October 7, 2026 14:56
@semanticdiff-com

semanticdiff-com Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Review changes with  SemanticDiff

Changed Files
File Status
  packages/redux/src/middleware/index.ts  47% smaller
  packages/log/src/LogInit.ts  23% smaller
  packages/log/src/LogExport.ts  11% smaller
  package-lock.json  9% smaller
  jest.setup.ts  0% smaller
  package.json  0% smaller
  packages/app-utils/src/components/AppBootstrap.test.tsx  0% smaller
  packages/app-utils/src/components/AppBootstrap.tsx  0% smaller
  packages/log/package.json  0% smaller
  packages/log/src/LogIndexedDbHistory.test.ts  0% smaller
  packages/log/src/LogIndexedDbHistory.ts  0% smaller
  packages/log/src/LogInit.test.ts  0% smaller
  packages/log/src/index.ts  0% smaller
  packages/redux/src/middleware/logUser.test.ts  0% smaller
  packages/redux/src/middleware/logUser.ts  0% smaller

@codecov

codecov Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.98496% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.60%. Comparing base (af8215a) to head (f2bb154).

Files with missing lines Patch % Lines
packages/log/src/LogIndexedDbHistory.ts 96.59% 8 Missing ⚠️
packages/log/src/LogExport.ts 37.50% 5 Missing ⚠️
packages/log/src/LogInit.ts 72.72% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2770      +/-   ##
==========================================
+ Coverage   53.33%   53.60%   +0.27%     
==========================================
  Files         816      830      +14     
  Lines       47498    47939     +441     
  Branches    12465    12370      -95     
==========================================
+ Hits        25331    25698     +367     
- Misses      22147    22221      +74     
  Partials       20       20              
Flag Coverage Δ
unit 53.60% <93.98%> (+0.27%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Flush races, cross-user data retention, and runtime compatibility issues must be addressed.

6 open findings
What changed in this PR

Adds cross-refresh browser log persistence through IndexedDB and includes persisted logs in support archives.

Changes:

  • Adds buffered persistence, retention pruning, formatting, and clearing.
  • Integrates persisted history with initialization and log exports.
  • Adds IndexedDB tests, mocks, and dependencies.
File Description
packages/​log/​src/​LogInit.ts Initializes persistent log history.
packages/​log/​src/​LogIndexedDbHistory.ts Implements IndexedDB log storage.
packages/​log/​src/​LogIndexedDbHistory.test.ts Tests persistence and pruning.
packages/​log/​src/​LogExport.ts Adds persisted logs to exports.
packages/​log/​src/​index.ts Exports the new API.
packages/​log/​package.json Adds runtime dependencies.
package.json Adds the IndexedDB test dependency.
package-lock.json Locks added dependencies.
jest.setup.ts Adds structured-clone support for tests.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/log/package.json
Comment thread packages/log/src/LogInit.ts Outdated
Comment thread packages/log/src/LogIndexedDbHistory.ts Outdated
Comment thread packages/log/src/LogIndexedDbHistory.ts Outdated
Comment thread packages/log/src/LogIndexedDbHistory.ts
Comment thread packages/log/src/LogExport.ts
@dgodinez-dh
dgodinez-dh marked this pull request as ready for review October 7, 2026 18:09
@dgodinez-dh
dgodinez-dh requested review from a team and jnumainville and a balanced review from Copilot and removed request for a team October 7, 2026 18:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment thread packages/log/src/LogIndexedDbHistory.ts Outdated
Comment thread packages/redux/src/middleware/logUser.ts Outdated
Copilot AI balanced review requested due to automatic review settings October 7, 2026 18:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Logout retains the previous user's log identity, and repeated initialization creates duplicate persistence listeners.

2 open findings
2 resolved since last review

🧠 Review effort: Balanced

Comment thread packages/log/src/LogIndexedDbHistory.ts Outdated
Comment thread packages/log/src/LogInit.ts Outdated
Copilot AI balanced review requested due to automatic review settings October 7, 2026 19:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

User attribution begins too late, stack call sites are inaccurate, and retention can substantially exceed its configured limit.

1 open finding
2 resolved since last review
Previously missed (2)

In code that hasn't changed since last review

Medium severity Prune log history based on projected batch entry count

packages/​log/​src/​LogIndexedDbHistory.ts:518

Retention is checked only once per 30 write transactions, while each transaction can contain up to 5,000 buffered entries. Starting at the 20,000-entry default, the store can reach roughly 170,000 entries before this prune runs; a quota failure during those writes disables persistence before pruning can recover it. Enforce the count bound on each batch (while keeping the more expensive quota estimate throttled), or trigger pruning from the projected entry count.

Medium severity Fix stack first-line detection to preserve the actual caller

packages/​log/​src/​LogIndexedDbHistory.ts:632

This does not reliably report the actual call site. LogHistory.formatStack checks stack[0] === 'Error' (LogHistory.ts:64), but stack[0] is one character, so an ordinary Error\n... stack always uses the wrong splice count and retains an internal console-proxy frame. Correct the helper's first-line detection and assert that persisted output contains the caller rather than the wrapper.

🧠 Review effort: Balanced

Comment thread packages/redux/src/middleware/logUser.ts
Copilot AI balanced review requested due to automatic review settings October 7, 2026 19:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Persistence status can be reported incorrectly while its request is pending, and the support-export integration lacks targeted coverage.

0 open findings

1 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Await persistence before reporting eviction protection status

packages/​log/​src/​LogIndexedDbHistory.ts:477

The persistence request is fire-and-forget, but getFormattedHistory() immediately interprets the default false as “not granted.” If persist() is still pending (for example while the browser waits on permission), an export can incorrectly report that eviction protection was denied even though it later resolves true. Track this request and await it before producing the header, or represent the pending state explicitly.

🧠 Review effort: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants