fix: three simple bugs — locale duration, dispatcher reset, impact test count#529
Merged
anandgupta42 merged 2 commits intomainfrom Mar 28, 2026
Merged
fix: three simple bugs — locale duration, dispatcher reset, impact test count#529anandgupta42 merged 2 commits intomainfrom
anandgupta42 merged 2 commits intomainfrom
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 11 minutes and 51 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughParses dbt test nodes into the manifest result and exposes Changes
Sequence DiagramsequenceDiagram
participant Manifest as Manifest Data
participant Parser as Manifest Parser
participant Analysis as Impact Analysis
participant Reporter as Report Generator
Manifest->>Parser: provide manifest.nodes
activate Parser
loop for each node
alt node.resource_type == "test"
Parser->>Parser: extract unique_id, name, depends_on
else
Parser->>Parser: handle models/sources
end
end
Parser-->>Analysis: DbtManifestResult (models, sources, tests)
deactivate Parser
Analysis->>Analysis: build target + downstream model ID set
Analysis->>Analysis: filter manifest.tests by depends_on ∩ targetIDs
Analysis-->>Reporter: affectedTests count
Reporter->>Reporter: format "Affected tests: N"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 unit tests (beta)
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. Comment |
![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 the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/altimate/native/dispatcher.ts`:
- Around line 23-31: The reset() function clears nativeHandlers but nukes the
lazy registration path by setting _ensureRegistered = null, leaving subsequent
call()s unable to trigger registration; update reset() to restore the module's
default lazy registration function instead of null (reinitialize
_ensureRegistered to the same default used at module load time), or
alternatively split behavior by adding a new test-only clearRegistrationHook()
that sets _ensureRegistered = null while leaving reset() to re-establish the
default hook; adjust references to _ensureRegistered, reset(),
setRegistrationHook(), and call() accordingly so tests can isolate handlers
without disabling registration for the process.
In `@packages/opencode/src/altimate/tools/impact-analysis.ts`:
- Around line 266-268: The sentence building the dbt test guidance uses
data.affectedTestCount and always uses "these" and "pass"; update the template
in the lines.push(...) that builds the message so it uses "this" and "passes"
when data.affectedTestCount === 1, and keeps "these" and "pass" otherwise (i.e.,
conditionalize the determiner and verb around data.affectedTestCount). Reference
the existing expression that currently interpolates data.affectedTestCount to
modify the wording accordingly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 98ad254f-b282-4918-8195-c494fa6d0cdc
📒 Files selected for processing (5)
packages/opencode/src/altimate/native/dbt/manifest.tspackages/opencode/src/altimate/native/dispatcher.tspackages/opencode/src/altimate/native/types.tspackages/opencode/src/altimate/tools/impact-analysis.tspackages/opencode/src/util/locale.ts
kulvirgit
force-pushed
the
fix/three-simple-bugs
branch
from
6612863 to
35ea4bf
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/opencode/src/altimate/tools/impact-analysis.ts (1)
73-84: Use existingmodelsByNameMap instead of repeatedfind()calls.The
modelsByNameMap is already built on lines 63-66 but isn't used here. Each.find()call iterates the entire models array, making this O(n × m) when O(n) is achievable.♻️ Proposed optimization
// Step 4: Count only tests that reference the target model or its downstream models const affectedModelIds = new Set([ targetModel.unique_id, ...downstream.map((d) => { - const m = manifest.models.find((m: { name: string }) => m.name === d.name) + const m = modelsByName.get(d.name) return m?.unique_id }).filter(Boolean), ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/impact-analysis.ts` around lines 73 - 84, Replace the repeated manifest.models.find(...) calls with the existing modelsByName Map to avoid O(n×m) scans: when building affectedModelIds (currently using targetModel.unique_id and downstream.map(...).filter(Boolean)), look up each downstream model by name via modelsByName.get(d.name) and pull its unique_id if present; then use that Set for filtering tests (the affectedTests and affectedTestCount logic can remain unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/opencode/src/altimate/tools/impact-analysis.ts`:
- Around line 73-84: Replace the repeated manifest.models.find(...) calls with
the existing modelsByName Map to avoid O(n×m) scans: when building
affectedModelIds (currently using targetModel.unique_id and
downstream.map(...).filter(Boolean)), look up each downstream model by name via
modelsByName.get(d.name) and pull its unique_id if present; then use that Set
for filtering tests (the affectedTests and affectedTestCount logic can remain
unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 423cbdfe-99b8-4939-bbc1-ae5f03d05632
📒 Files selected for processing (5)
packages/opencode/src/altimate/native/dbt/manifest.tspackages/opencode/src/altimate/native/dispatcher.tspackages/opencode/src/altimate/native/types.tspackages/opencode/src/altimate/tools/impact-analysis.tspackages/opencode/src/util/locale.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/opencode/src/altimate/native/dispatcher.ts
- packages/opencode/src/altimate/native/dbt/manifest.ts
- packages/opencode/src/util/locale.ts
kulvirgit
force-pushed
the
fix/three-simple-bugs
branch
from
35ea4bf to
8292dcd
Compare
…st count 1. Fix Locale.duration for >=24h (#368): days and hours were swapped. `Math.floor(input / 3600000)` gave total hours, not days. Now correctly: days = input/86400000, hours = remainder/3600000. 2. Fix dispatcher.reset() not clearing lazy registration hook (#468): reset() only cleared handlers but left _ensureRegistered alive. Next call() would re-register all handlers via the stale hook, causing flaky test failures when test files share module state. 3. Fix impact_analysis showing project-wide test_count (#371): Was using manifest.test_count (all tests in project). Now collects individual test nodes with their depends_on refs and counts only tests that reference the target model or its downstream dependents. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
kulvirgit
force-pushed
the
fix/three-simple-bugs
branch
from
8292dcd to
098563d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/opencode/src/altimate/tools/impact-analysis.ts`:
- Around line 73-81: The current impact calculation adds the target model to
affectedModelIds causing affectedTestCount to be >0 even when downstream.length
=== 0, which leads to contradictory "safe" reports; update the logic so
affectedModelIds only includes downstream model IDs (exclude
targetModel.unique_id) when computing affectedTests/affectedTestCount, and then
adjust formatting in formatImpactReport to gate the "Change is safe to make"
branch on data.affectedTestCount === 0 (and emit a distinct message when only
direct tests on the target model are affected) so report severity and messages
align with the actual tests impacted.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f56abb81-f50c-4637-a575-90e73b34177a
📒 Files selected for processing (6)
packages/opencode/src/altimate/native/dbt/manifest.tspackages/opencode/src/altimate/native/dispatcher.tspackages/opencode/src/altimate/native/types.tspackages/opencode/src/altimate/tools/impact-analysis.tspackages/opencode/src/util/locale.tspackages/opencode/test/util/locale.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/opencode/src/altimate/native/dispatcher.ts
- packages/opencode/src/util/locale.ts
- packages/opencode/src/altimate/native/dbt/manifest.ts
anandgupta42
added a commit
that referenced
this pull request
Mar 29, 2026
Pre-existing marker violation from #529 — wraps the days/hours swap fix with proper markers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix 3 long-standing bugs that each require minimal code changes:
1. Locale.duration wrong for >=24h (#368)
Days and hours were swapped in the
>=86400000msbranch:Bug was found by PR #369, tests existed, but the fix was never merged.
2. dispatcher.reset() doesn't clear lazy registration hook (#468)
reset()cleared handlers but left_ensureRegisteredalive. When test files share module state, the stale hook re-registers all handlers on the nextcall(), causing flaky CI failures.Fix: Add
_ensureRegistered = nulltoreset()and move the variable declaration before the function (required becauseletis not hoisted).3. impact_analysis shows project-wide test_count (#371)
Was using
manifest.test_count(all tests in project) instead of counting only tests that reference the affected model. A change tostg_orderswould report "Tests in project: 47" implying all 47 are at risk.Fix:
DbtTestInfotype andtests[]array to manifest parser (collects test nodes with theirdepends_onreferences)depends_onincludes the target model or its downstream dependentsTest plan
bun test test/util/locale.test.ts— 12 pass, 0 failbun test test/altimate/dispatcher.test.ts— 8 pass, 0 failbun test test/altimate/impact-analysis.test.ts— 12 pass, 0 failCloses #368, #468, #371
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests