Skip to content

refactor(rollups): evaluate own-property iteration without key arrays - #602

Draft
seonghobae wants to merge 9 commits into
developmentalfrom
jules-3197975644067938090-fd9daaa2
Draft

refactor(rollups): evaluate own-property iteration without key arrays#602
seonghobae wants to merge 9 commits into
developmentalfrom
jules-3197975644067938090-fd9daaa2

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 8, 2026

Copy link
Copy Markdown

Current boundary

  • protected base: developmental@2fa92012bcf80acc1f921a4bafea76b3b1424b46
  • exact head: f348e6d1f932a2318d7e9303d00e3f15f56c2f75
  • lifecycle: Draft / structural refactor present / representative performance evidence missing

The production candidate replaces selected Object.keys(obj) loops in daily/weekly rollup aggregation with for...in guarded by Object.hasOwn(obj, key). That structurally avoids materializing a keys array at those call sites. It does not by itself establish faster buyer-facing report generation, lower p95, lower CPU, or material GC/heap improvement on the supported Node/V8 runtime.

f348e6d1f932a2318d7e9303d00e3f15f56c2f75 restores .jules/bolt.md byte-for-byte to protected blob 57daf4712f8a58203eab836b9b8e209b6bad1ab9. An unmeasured refactor must not create repository-wide doctrine that for...in is generally preferable to Object.keys().

Scope debt

The current comparison still contains unrelated root dependency updates (package.json, pnpm-lock.yaml) for Browserslist/DeepmergeTS. Those are not performance evidence for this refactor and must not be silently discarded: dependency lane #525 already owns the transitive security/compatibility problem but currently carries its own mixed-scope debt. Reconcile the dependency generations through that owner path first; after the valid dependency delta reaches protected ancestry, restack this branch by ordinary descendant so the rollup PR contains only its own semantic/test/evidence delta.

Acceptance before Ready/merge

  1. Preserve exact aggregation results for skill counts, agent counts, model tokens, total calls and weekly distinct-skill semantics, including objects with inherited enumerable properties; prove caller inputs are not mutated.
  2. If performance remains the merge rationale, compare protected and candidate implementations on representative/right-cleared daily and weekly rollup shapes under the same supported Node/V8/runtime/CPU. Record cardinality, warm-up policy, repeated median/p95, CPU profile, allocations/heap and GC.
  3. Measure the buyer-facing daily/weekly report path where applicable. A source-level allocation observation is not evidence that the fleet p95≤20ms target improved.
  4. If the controlled comparison is neutral or worse, prefer the simpler protected implementation rather than retaining extra for...in/hasOwn complexity as an assumed optimization.
  5. Reconcile the dependency delta through fix(security): patch transitive deepmerge-ts stack exhaustion #525, then require all applicable CI/Security/SAST/CodeQL checks and qualifying independent review on one unchanged exact descendant.

No force push, destructive rebase, self-approval, scanner suppression, source-neutral retrigger, predecessor-GREEN transfer, or gate weakening is acceptance evidence.

@google-labs-jules

Copy link
Copy Markdown

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 3444c2fc-4497-4660-863a-9ac049cc07e4

📥 Commits

Reviewing files that changed from the base of the PR and between cceb5f1 and 73451d9.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (1)
  • package.json

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

일일 및 주간 집계 루프를 for...inObject.hasOwn() 방식으로 변경했습니다. browserslistdeepmerge-ts 버전을 고정했습니다. 기존 집계 결과는 유지됩니다.

Changes

집계 순회 최적화

Layer / File(s) Summary
집계 핫 패스 순회 변경
packages/web/src/lib/server/daily-rollup.ts, packages/web/src/lib/server/weekly-report.ts, .jules/bolt.md
skillCounts, agentCounts, modelTokens 순회를 Object.keys() 기반 방식에서 for...inObject.hasOwn() 검증 방식으로 변경했습니다. 기존 합산 결과는 유지됩니다. 변경 원칙을 학습 기록에 추가했습니다.

패키지 버전 고정

Layer / File(s) Summary
패키지 오버라이드 추가
package.json
pnpm overridesbrowserslist 4.28.9와 deepmerge-ts 8.0.2를 추가했습니다.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 8a6ab

Daily and weekly rollup aggregation avoids temporary key-array allocations while preserving own-property counting behavior, and the pinned dependency configuration is reflected in the lockfile. No merge-blocking risk is evident.

Suggested reviewers: claude

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 제목은 rollups 리팩터링과 Object.keys() 배열 할당을 제거하기 위한 own-property 순회 검토를 명확하게 설명합니다. 변경 내용과 직접 관련되며 간결합니다.
Full details: Docstring Coverage

Explanation

Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jules-3197975644067938090-fd9daaa2

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae
seonghobae marked this pull request as draft September 9, 2026 07:44
@seonghobae seonghobae changed the title ⚡ Bolt: [performance improvement] 데이터 집계 핫 패스에서 배열 할당 제거 refactor(rollups): evaluate own-property iteration without key arrays Sep 9, 2026
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.

1 participant