[Gel] Add PeriodicReport queries | refactor service/repo layers
#3247
Conversation
bryanjnelson
force-pushed
the
0933-periodic-report-queries
branch
2 times, most recently
from
1d3efe9 to
07d8f1b
Compare
src/components/periodic-report/periodic-report.edgedb.repository.ts
Outdated
Show resolved
Hide resolved
bryanjnelson
force-pushed
the
0933-periodic-report-queries
branch
3 times, most recently
from
5b844a6 to
ec52210
Compare
src/components/periodic-report/periodic-report.edgedb.repository.ts
Outdated
Show resolved
Hide resolved
bryanjnelson
changed the title
PeriodicReport queries | refactor service/repo layers appropriatelyPeriodicReport queries | refactor service/repo layers
bryanjnelson
force-pushed
the
0933-periodic-report-queries
branch
6 times, most recently
from
22a97e2 to
8962f0d
Compare
bryanjnelson
force-pushed
the
0933-periodic-report-queries
branch
from
8962f0d to
3e3db8d
Compare
📝 WalkthroughWalkthroughIntroduce a GEL-specific PeriodicReport repository, switch sync handlers to use repositories, add AnyReport DTO and ReportConcretes mapping, refactor repository/service merge/update/delete flows (add mergeFinalReport/readOne, logging, error guards), and register splitDb provider for repository resolution. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/components/periodic-report/handlers/sync-progress-report-to-engagement.handler.ts (1)
47-49: Consistent gel database bypass pattern.This handler implements the same early return logic for gel databases as the project sync handler. The consistency is good, and this aligns with the verification script from the previous handler review.
🧹 Nitpick comments (4)
src/components/periodic-report/dto/periodic-report.dto.ts (1)
54-54: Track the TODO comment for future implementation.The TODO suggests changing the type to
Secured<LinkTo<'File'> | null>. This would make the property nullable and potentially change how file references are handled.Would you like me to create an issue to track this TODO for future implementation?
src/components/periodic-report/periodic-report.gel.repository.ts (2)
44-44: Address the TODO comments for ProgressReport hydration.Multiple methods have the same TODO comment indicating that ProgressReport hydration needs work. This suggests incomplete implementation that should be addressed.
Would you like me to help implement the proper hydration logic for ProgressReport or create an issue to track this work?
Also applies to: 93-93, 112-112, 132-132, 152-152
55-55: Consider using an injected date provider for better testability.Using
Date.now()directly in queries makes unit testing difficult and could cause timezone-related issues. Consider injecting a date provider or accepting the current date as a parameter.-e.op(report.end, '<', Date.now()), +e.op(report.end, '<', this.dateProvider.now()),Also applies to: 124-124
src/components/periodic-report/periodic-report.repository.ts (1)
71-196: Consider breaking down the complex merge method.The merge method is quite long and handles multiple responsibilities. Consider extracting helper methods for better readability and maintainability.
Consider extracting:
- Interval preparation logic (lines 81-88)
- Query construction logic
- Result processing and logging
This would make the method easier to test and maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/components/periodic-report/dto/periodic-report.dto.ts(4 hunks)src/components/periodic-report/handlers/abstract-periodic-report-sync.ts(3 hunks)src/components/periodic-report/handlers/sync-periodic-report-to-project.handler.ts(2 hunks)src/components/periodic-report/handlers/sync-progress-report-to-engagement.handler.ts(3 hunks)src/components/periodic-report/periodic-report.gel.repository.ts(1 hunks)src/components/periodic-report/periodic-report.module.ts(3 hunks)src/components/periodic-report/periodic-report.repository.ts(5 hunks)src/components/periodic-report/periodic-report.service.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/periodic-report/dto/periodic-report.dto.ts (1)
src/components/file/dto/file.dto.ts (1)
DefinedFile(165-165)
🔇 Additional comments (11)
src/components/periodic-report/periodic-report.module.ts (1)
2-2: LGTM: Clean dual repository pattern implementation.The introduction of
splitDbto provide bothPeriodicReportRepositoryandPeriodicReportGelRepositoryimplements a clean dual repository pattern. This allows conditional database access based on configuration while maintaining the same interface.Also applies to: 11-11, 30-30
src/components/periodic-report/handlers/sync-periodic-report-to-project.handler.ts (2)
3-9: LGTM: Clean service-to-repository refactoring.The refactoring correctly replaces
PeriodicReportServicewithPeriodicReportRepositoryand addsConfigServicedependency injection. The constructor and super call are properly updated.Also applies to: 13-13, 27-31
35-37:Details
✅ Verification successful
Verify the gel database bypass logic.
The early return when
databaseEngine === 'gel'completely bypasses periodic report syncing for gel databases. This seems intentional given the newPeriodicReportGelRepository, but it's worth confirming that this handler should indeed be completely skipped for gel databases rather than using the gel-specific repository.
🏁 Script executed:
#!/bin/bash # Description: Check if other handlers have similar gel database bypass logic and verify gel repository usage # Search for similar gel database bypasses in other handlers echo "=== Searching for gel database bypass patterns ===" rg -A 2 -B 2 "databaseEngine.*gel" # Search for gel repository usage patterns echo -e "\n=== Searching for PeriodicReportGelRepository usage ===" rg -A 5 -B 2 "PeriodicReportGelRepository" # Check for any gel-specific handling in the repository echo -e "\n=== Checking for gel-specific methods or logic ===" fd "periodic-report.*gel" --type fLength of output: 7614
Bypass logic for GEL database is consistent and intentional
I’ve verified that all other sync and setup handlers early-return on
databaseEngine === 'gel', and thatPeriodicReportGelRepositoryis correctly wired in viasplitDbinperiodic-report.module.ts. No changes needed here.src/components/periodic-report/handlers/abstract-periodic-report-sync.ts (1)
4-4: LGTM: Consistent service-to-repository refactoring.The abstract base class correctly refactors from
PeriodicReportServicetoPeriodicReportRepository. All method calls are consistently updated to use the newperiodicReportsRepoproperty, maintaining the same logic while using the repository pattern.Also applies to: 12-14, 28-28, 30-30, 39-39
src/components/periodic-report/handlers/sync-progress-report-to-engagement.handler.ts (1)
2-8: LGTM: Consistent refactoring pattern across handlers.This handler follows the same clean refactoring pattern as
sync-periodic-report-to-project.handler.ts, replacingPeriodicReportServicewithPeriodicReportRepositoryand addingConfigServicedependency. The constructor and super call are correctly updated.Also applies to: 17-17, 38-43
src/components/periodic-report/dto/periodic-report.dto.ts (2)
22-25: LGTM! Well-designed mutually exclusive union type.The use of
MergeExclusiveensures type safety by preventing an instance from being multiple report types simultaneously. This is a good pattern for discriminated unions.
93-97: Good addition for type-safe report class references.The
ReportConcretesmapping provides a clean way to dynamically reference report classes, which is useful for factory patterns and type resolution.src/components/periodic-report/periodic-report.service.ts (2)
50-57: LGTM! Clean refactoring of the update method.The update method now properly constructs a single object with all necessary properties for the repository update. This aligns well with the repository's simplified interface.
61-61: Good security practice!Using the secured version of the report file ensures proper authorization checks are applied.
src/components/periodic-report/periodic-report.repository.ts (2)
198-218: Well-implemented final report merge logic.The method cleanly handles both updating existing final reports and creating new ones, with an appropriate early return for no-change scenarios.
433-433: Good addition of deletion logging.Adding logging for delete operations provides a valuable audit trail and helps with debugging.
bryanjnelson
force-pushed
the
0933-periodic-report-queries
branch
from
3e3db8d to
1125d52
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/components/periodic-report/periodic-report.gel.repository.ts (2)
2-2: Avoid importing from internal paths.This is the same issue flagged in previous reviews - importing from internal type-fest paths is unstable and could break in future versions.
66-91: Remove or implement thegetCurrentDuemethod.This method is unimplemented and has been flagged in previous reviews. The complex union type signature also suggests potential design issues.
🧹 Nitpick comments (2)
src/components/periodic-report/periodic-report.gel.repository.ts (2)
44-44: Address the repeated TODO comments about ProgressReport hydration.Multiple methods have identical TODO comments indicating that ProgressReport hydration needs work. This suggests a systemic issue that should be addressed rather than leaving multiple TODO markers.
Would you like me to help analyze what specific issues exist with ProgressReport hydration and propose a solution, or should this be tracked as a separate issue?
Also applies to: 93-93, 112-112, 132-132, 152-152
45-170: Consider refactoring to reduce code duplication.The query methods share significant boilerplate code for setting up the enhanced resource, casting the resource, and building the base query structure. This duplication makes the code harder to maintain.
Consider extracting a common query builder method:
+ private buildBaseQuery(parentId: ID, reportType: ReportType) { + const enhancedResource = EnhancedResource.of( + resolveReportType({ type: reportType }), + ); + const resource = e.cast(enhancedResource.db, e.uuid(parentId)); + return { enhancedResource, resource }; + } async matchCurrentDue(parentId: ID, reportType: ReportType) { - const enhancedResource = EnhancedResource.of( - resolveReportType({ type: reportType }), - ); - const resource = e.cast(enhancedResource.db, e.uuid(parentId)); + const { resource } = this.buildBaseQuery(parentId, reportType); const report = e.select(resource, (report) => ({ ...this.hydrate(report), filter: e.all( e.set( e.op(resource.id, '=', report.container.id), e.op(report.end, '<', Date.now()), ), ), order_by: [ { expression: report.end, direction: e.DESC }, { expression: report.start, direction: e.ASC }, ], })); return await this.db.run(report); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/components/periodic-report/dto/periodic-report.dto.ts(4 hunks)src/components/periodic-report/handlers/abstract-periodic-report-sync.ts(3 hunks)src/components/periodic-report/handlers/sync-periodic-report-to-project.handler.ts(2 hunks)src/components/periodic-report/handlers/sync-progress-report-to-engagement.handler.ts(3 hunks)src/components/periodic-report/periodic-report.gel.repository.ts(1 hunks)src/components/periodic-report/periodic-report.module.ts(3 hunks)src/components/periodic-report/periodic-report.repository.ts(5 hunks)src/components/periodic-report/periodic-report.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/components/periodic-report/periodic-report.module.ts
- src/components/periodic-report/handlers/sync-periodic-report-to-project.handler.ts
- src/components/periodic-report/handlers/abstract-periodic-report-sync.ts
- src/components/periodic-report/dto/periodic-report.dto.ts
- src/components/periodic-report/handlers/sync-progress-report-to-engagement.handler.ts
- src/components/periodic-report/periodic-report.service.ts
- src/components/periodic-report/periodic-report.repository.ts
src/components/periodic-report/periodic-report.gel.repository.ts
Outdated
Show resolved
Hide resolved
bryanjnelson
force-pushed
the
0933-periodic-report-queries
branch
from
1125d52 to
9420c44
Compare
bryanjnelson
force-pushed
the
0933-periodic-report-queries
branch
2 times, most recently
from
89c5696 to
75af83c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/periodic-report/periodic-report.repository.ts (1)
192-195: FixCreationFailedconstruction to include the original exception properly.
CreationFailedexpects an options object withcause, not the error directly. This will currently drop type information and may not compile depending on strictness.Apply:
- } catch (exception) { + } catch (exception) { const Report = resolveReportType({ type: input.type }); - throw new CreationFailed(Report, exception); + throw new CreationFailed(Report, { cause: exception as Error }); }
🧹 Nitpick comments (6)
src/components/periodic-report/dto/periodic-report.dto.ts (3)
54-54: ExposereportFilevia GraphQL or document as intentionally internal.
reportFilelacks a@Field()decorator while other properties are exposed. If clients need to read it, add the decorator. Per the team learning, GraphQL type can be inferred when using the direct class reference, so no need for an explicit type function.Apply if you intend to expose it:
@Field() readonly skippedReason: SecuredStringNullable; - readonly reportFile: DefinedFile; //TODO? - Secured<LinkTo<'File'> | null> + @Field() + readonly reportFile: DefinedFile; // TODO? - Secured<LinkTo<'File'> | null>
93-98: Type theReportConcretesmap to guard against drift when adding new report types.Adding a
satisfiesconstraint ensures keys stay in sync withReportTypeand values are properPeriodicReportconstructors.Apply:
-export const ReportConcretes = { +export const ReportConcretes = { Financial: FinancialReport, Narrative: NarrativeReport, Progress: ProgressReport, -}; +} as const satisfies Record<ReportType, typeof PeriodicReport>;If your TS version doesn't support
satisfies, fall back to a simple cast:} as Record<ReportType, typeof PeriodicReport>;
22-25:AnyReportis an unused export; consider removal or inlining
TheAnyReportalias insrc/components/periodic-report/dto/periodic-report.dto.ts(lines 22–25) isn’t referenced anywhere else in the repo. To keep the API surface minimal, either remove this type or inline its definition at the eventual call sites.src/components/periodic-report/periodic-report.repository.ts (3)
66-69: Injecting a logger improves observability.Scope label matches the service; consider a repository-specific scope later if logs get noisy, but fine for now.
205-213: Avoid numeric coercion forCalendarDateequality.
+report.start === +atrelies on implicit coercion; prefer a semantic equality. IfCalendarDatelacks anequals, compare ISO strings.Apply one of:
- if (+report.start === +at) { + if (report.start.toISO() === at.toISO()) { // no change return; }Or, if available:
if (report.start.equals(at)) { ... }Please confirm whether
CalendarDateexposes anequalsor comparable method; if so, prefer it over string comparison.
433-434: Deletion logging is useful; consider including interval filters for traceability.If logs become ambiguous, consider logging the filtered intervals list too.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/components/periodic-report/dto/periodic-report.dto.ts(4 hunks)src/components/periodic-report/handlers/abstract-periodic-report-sync.ts(3 hunks)src/components/periodic-report/handlers/sync-periodic-report-to-project.handler.ts(2 hunks)src/components/periodic-report/handlers/sync-progress-report-to-engagement.handler.ts(3 hunks)src/components/periodic-report/periodic-report.gel.repository.ts(1 hunks)src/components/periodic-report/periodic-report.module.ts(3 hunks)src/components/periodic-report/periodic-report.repository.ts(5 hunks)src/components/periodic-report/periodic-report.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/periodic-report/handlers/abstract-periodic-report-sync.ts
- src/components/periodic-report/handlers/sync-progress-report-to-engagement.handler.ts
- src/components/periodic-report/periodic-report.gel.repository.ts
- src/components/periodic-report/handlers/sync-periodic-report-to-project.handler.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-15T20:49:50.809Z
Learnt from: CarsonF
PR: SeedCompany/cord-api-v3#3544
File: src/components/tools/tool-usage/dto/tool-usage.dto.ts:26-27
Timestamp: 2025-08-15T20:49:50.809Z
Learning: In NestJS GraphQL within this codebase, when a field uses a direct class reference (like `SecuredDateNullable`), the Field() decorator can automatically infer the GraphQL type without needing an explicit type function like Field(() => SecuredDateNullable).
Applied to files:
src/components/periodic-report/dto/periodic-report.dto.ts
🧬 Code Graph Analysis (3)
src/components/periodic-report/periodic-report.repository.ts (6)
src/components/periodic-report/dto/merge-periodic-reports.dto.ts (1)
resolveReportType(22-28)src/core/database/database.service.ts (1)
query(129-139)src/core/database/query/create-node.ts (1)
createNode(53-90)src/core/database/query/create-relationships.ts (1)
createRelationships(88-187)src/common/temporal/date-interval.ts (1)
DateInterval(28-185)src/common/exceptions/creation-failed.exception.ts (1)
CreationFailed(5-15)
src/components/periodic-report/dto/periodic-report.dto.ts (1)
src/components/file/dto/file.dto.ts (1)
DefinedFile(165-165)
src/components/periodic-report/periodic-report.service.ts (1)
src/components/periodic-report/dto/merge-periodic-reports.dto.ts (1)
resolveReportType(22-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: lint
- GitHub Check: Generate (head)
- GitHub Check: Generate (base)
- GitHub Check: Clean
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: Unit
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: E2E Tests (neo4j 1/6)
🔇 Additional comments (13)
src/components/periodic-report/dto/periodic-report.dto.ts (2)
2-2: Good call usingMergeExclusivefor DTO exclusivity.This clearly encodes the mutual exclusivity intent at the type level without runtime overhead.
19-19: ImportingProgressReporthere improves locality for the type map.Keeps consumers from reaching into the progress-report module indirectly.
src/components/periodic-report/periodic-report.module.ts (3)
2-2: WiringsplitDbfrom core is the right direction for dual engines.This keeps the module clean while allowing engine-specific repos.
11-11: Import of GEL repo looks correct and localized.No side-effects and used only for provider resolution.
30-30: Manual verification required forPeriodicReportRepositoryexportsI ran a project-wide search for any direct constructor or
@Injectusage ofPeriodicReportRepositoryoutside ofsrc/components/periodic-report/**and found no occurrences. However, absence of matches isn’t definitive proof that no other modules rely on this provider.• Please manually confirm that no other modules inject
PeriodicReportRepositorydirectly.
• If any direct injections do exist, addPeriodicReportRepositoryto theexportsarray inperiodic-report.module.tsalongside the service.src/components/periodic-report/periodic-report.service.ts (4)
40-45: Privilege checks now correctly use the repo’s current snapshot.Reading via
repo.readOnebefore diffing is safer for authorization and diff accuracy.
48-55: Update flow looks good; avoids passingreportFileto the repo.Preserving
start/endfrom the current entity while lettingsimpleChangesoverride them if present is sound.
59-59: Good: securing before passingDefinedFileinto FileService.This ensures access checks are honored prior to mutation.
41-41: getActualChanges is inherited via DtoRepository; no changes neededThe PeriodicReportRepository extends DtoRepository(…) which, in src/core/database/dto.repository.ts, defines:
getActualChanges = getChanges(resource);(see line 46 of DtoRepositoryClass) – so this.repo.getActualChanges(…) is available and behaves as intended.
src/components/periodic-report/periodic-report.repository.ts (4)
72-75: No-op early return on empty intervals is correct.Prevents unnecessary query plumbing.
220-229: Returning the hydrated entity after update is helpful.Keeps service code simple and avoids stale reads.
231-239: Nice guard onreadOnewithNotFoundException.Clear message and field path aid clients and logs.
375-379: Early exit on empty/invalid intervals avoids a destructive broad delete.Good defensive check.
| .apply( | ||
| await createNode(File, { | ||
| initialProps: { | ||
| name: variable('apoc.temporal.format(interval.end, "date")'), | ||
| }, | ||
| baseNodeProps: { | ||
| id: variable('interval.tempFileId'), | ||
| createdAt: variable('now'), | ||
| }, | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t pass a function call via variable(...); pre-compute fileName instead.
variable('apoc.temporal.format(interval.end, "date")') is not a valid variable import and may break createNode()’s subquery scoping. Compute the file name via WITH and pass a real variable to createNode(File).
Apply:
- // rename node to report, so we can call create node again for the file
- .with('now, interval, node as report')
+ // rename node to report, so we can call create node again for the file
+ .with('now, interval, node as report')
+ // Precompute fileName to avoid injecting a function call as a variable
+ .raw('WITH now, interval, report, apoc.temporal.format(interval.end, "date") as fileName')
.apply(
await createNode(File, {
initialProps: {
- name: variable('apoc.temporal.format(interval.end, "date")'),
+ name: variable('fileName'),
},
baseNodeProps: {
id: variable('interval.tempFileId'),
createdAt: variable('now'),
},
}),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .apply( | |
| await createNode(File, { | |
| initialProps: { | |
| name: variable('apoc.temporal.format(interval.end, "date")'), | |
| }, | |
| baseNodeProps: { | |
| id: variable('interval.tempFileId'), | |
| createdAt: variable('now'), | |
| }, | |
| }), | |
| // rename node to report, so we can call create node again for the file | |
| .with('now, interval, node as report') | |
| // Precompute fileName to avoid injecting a function call as a variable | |
| .raw('WITH now, interval, report, apoc.temporal.format(interval.end, "date") as fileName') | |
| .apply( | |
| await createNode(File, { | |
| initialProps: { | |
| name: variable('fileName'), | |
| }, | |
| baseNodeProps: { | |
| id: variable('interval.tempFileId'), | |
| createdAt: variable('now'), | |
| }, | |
| }), | |
| ) |
🤖 Prompt for AI Agents
In src/components/periodic-report/periodic-report.repository.ts around lines 161
to 170, you are passing a function call string into variable(...) which is
invalid for subquery scoping; instead compute the file name with a WITH that
evaluates apoc.temporal.format(interval.end, "date") AS fileName and then call
createNode(File) using variable('fileName') in initialProps.name (and ensure the
WITH is placed so fileName is in scope for the createNode call); update the
query flow so the expression is evaluated before createNode and only a real
variable is passed into createNode.
bryanjnelson
force-pushed
the
0933-periodic-report-queries
branch
from
75af83c to
7d8e6c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/periodic-report/periodic-report.repository.ts (2)
192-195: Good: original exception is preserved in CreationFailed.This improves diagnosability compared to a generic error.
160-170: Don’t pass function calls viavariable(...); computefileNamefirst and reference it.
variable('apoc.temporal.format(interval.end, "date")')is not a valid scoped variable and can breakcreateNode(File)’s subquery. Compute the value in-scope and pass a real variable.- // rename node to report, so we can call create node again for the file - .with('now, interval, node as report') + // rename node to report, so we can call create node again for the file + .with('now, interval, node as report') + // Precompute fileName in-scope for createNode(File) + .raw('WITH now, interval, report, apoc.temporal.format(interval.end, "date") as fileName') .apply( await createNode(File, { initialProps: { - name: variable('apoc.temporal.format(interval.end, "date")'), + name: variable('fileName'), }, baseNodeProps: { id: variable('interval.tempFileId'), createdAt: variable('now'), }, }), )#!/bin/bash # Find any other instances where a function call is passed to variable(...) rg -nP "variable\\('\\w+\\(.*\\)'" -C2 --type=ts
🧹 Nitpick comments (3)
src/components/periodic-report/periodic-report.repository.ts (1)
66-67: Use a distinct logger context for the repository.Using 'periodic:report:service' here conflates repo vs. service logs. Prefer a repo-specific context to improve observability.
- @Logger('periodic:report:service') private readonly logger: ILogger, + @Logger('periodic:report:repo') private readonly logger: ILogger,src/components/periodic-report/handlers/abstract-periodic-report-sync.ts (1)
25-29: Null-guard ondiff.Probably unnecessary given the method’s callers, but harmless as a defensive check.
src/components/periodic-report/periodic-report.service.ts (1)
40-46: Using repo.readOne for current state and repo.getActualChanges is clean.Minor: the
as UpdatePeriodicReportInput & Record<any, never>cast is a smell. Consider refininggetActualChanges’s generics to avoid the assertion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/components/periodic-report/dto/periodic-report.dto.ts(4 hunks)src/components/periodic-report/handlers/abstract-periodic-report-sync.ts(3 hunks)src/components/periodic-report/handlers/sync-periodic-report-to-project.handler.ts(2 hunks)src/components/periodic-report/handlers/sync-progress-report-to-engagement.handler.ts(3 hunks)src/components/periodic-report/periodic-report.gel.repository.ts(1 hunks)src/components/periodic-report/periodic-report.module.ts(3 hunks)src/components/periodic-report/periodic-report.repository.ts(5 hunks)src/components/periodic-report/periodic-report.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/periodic-report/periodic-report.module.ts
- src/components/periodic-report/handlers/sync-periodic-report-to-project.handler.ts
- src/components/periodic-report/dto/periodic-report.dto.ts
- src/components/periodic-report/periodic-report.gel.repository.ts
- src/components/periodic-report/handlers/sync-progress-report-to-engagement.handler.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: E2E Tests (neo4j 1/6)
- GitHub Check: E2E Tests (neo4j 6/6)
- GitHub Check: E2E Tests (neo4j 3/6)
- GitHub Check: E2E Tests (neo4j 5/6)
- GitHub Check: E2E Tests (neo4j 2/6)
- GitHub Check: E2E Tests (neo4j 4/6)
- GitHub Check: Clean
- GitHub Check: Unit
- GitHub Check: Generate (base)
- GitHub Check: Generate (head)
- GitHub Check: lint
🔇 Additional comments (12)
src/components/periodic-report/periodic-report.repository.ts (7)
72-75: Early return on empty intervals is correct and avoids a no-op query.Good guard to short-circuit work when there’s nothing to merge.
82-88: Pre-generating IDs for report and file nodes is solid.This keeps subqueries deterministic and ties file/report creation together cleanly.
90-94: Conditional extras for Progress reports look good.Keeping
getCreateOptionsscoped to progress reports avoids leaking progress-only props to other report types.
101-133: Idempotent creation pattern is sound.The subquery inverts 0 matches to 1 row to gate creation per interval. This keeps merges idempotent without extra client-side checks.
184-191: Helpful merge summary logging.The existing/new counts and ISO intervals make troubleshooting easy. Nice.
221-229: Update flow looks good.Using
updatePropertiesthen re-reading ensures the service receives the canonical hydrated DTO.
375-379: Early-return on no-op deletions is good.Filtering out empty intervals and short-circuiting avoids hitting the DB unnecessarily.
src/components/periodic-report/handlers/abstract-periodic-report-sync.ts (3)
12-14: Switch to repository dependency is aligned with the new architecture.The abstract sync now depends on the repo abstraction, which is consistent with the PR’s GEL split-db approach.
28-35: Delete before merge is the right order.Prevents duplicates when ranges overlap across cycles.
39-40: DelegatingmergeFinalReportto repo keeps sync thin and focused.Good separation of concerns.
src/components/periodic-report/periodic-report.service.ts (2)
52-60: Pass-through to repo.update and re-securing is correct.Spreading
simpleChangesafter currentstart/endensures newstart/end(if present) override the current values.
63-73: Securely updating the file and publishing the upload event looks good.Using
this.secure(current).reportFileenforces access control on the file node.
| if (report) { | ||
| if (+report.start === +at) { | ||
| // no change | ||
| return; | ||
| } | ||
| await this.update({ id: report.id, start: at, end: at }); | ||
| } else { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Bug: comparing CalendarDate values with unary plus will always be falsey.
+report.start === +at will evaluate to NaN === NaN if CalendarDate is a string-like type, causing unnecessary updates every time a final report already matches the date.
- if (+report.start === +at) {
+ if (report.start === at) {
// no change
return;
}🏁 Script executed:
#!/bin/bash
# Verify CalendarDate is string-like and search for any other unary-plus comparisons
rg -nP "(export\\s+type|type)\\s+CalendarDate\\b" --type=ts
rg -nP "\\+\\s*(?:report\\.)?(?:start|end)\\b|\\+\\s*at\\b" src --type=tsLength of output: 2769
🏁 Script executed:
#!/bin/bash
# Find the exact definition of CalendarDate
rg -nP "export\s+type\s+CalendarDate\s*=" --type=ts
# Locate CalendarDate in common/temporal files for any inline definitions
rg -n "CalendarDate" -g "src/common/temporal*" -nLength of output: 252
🏁 Script executed:
#!/bin/bash
# List all files under src/common/temporal to locate where CalendarDate is defined
ls -R src/common/temporal
# Search within those files for the CalendarDate type alias
rg -n "type CalendarDate" src/common/temporalLength of output: 186
Critical: Fix CalendarDate comparison to avoid NaN
The CalendarDate type is a string alias, so using the unary plus operator (+) on a string produces NaN. As a result, the check
if (+report.start === +at)always evaluates to false, causing the repository to issue unnecessary updates even when the dates are identical.
Please update the comparison to use direct string equality:
Affected location:
src/components/periodic-report/periodic-report.repository.ts, around lines 205–211
Suggested change:
- if (+report.start === +at) {
+ if (report.start === at) {
// no change
return;
}No other unary-plus usages on CalendarDate were found in the codebase; this one change will prevent the spurious updates.
🤖 Prompt for AI Agents
In src/components/periodic-report/periodic-report.repository.ts around lines 205
to 211, the comparison uses unary plus on CalendarDate strings (if
(+report.start === +at)) which yields NaN and always fails; replace the numeric
coercion with a direct string equality check (if (report.start === at)) so
identical dates don't trigger unnecessary updates.
Closes #3474.
The first two commits are ready for review. The third one needs some help from @CarsonF in tweaking the hydrate method; hopefully I can then finish the remaining part of that commit (the actual Gel repo methods).