Overview of cleanup - suggested indexes and export tool - #1975
Draft
RagnarFatland wants to merge 25 commits into
Draft
Overview of cleanup - suggested indexes and export tool#1975RagnarFatland wants to merge 25 commits into
RagnarFatland wants to merge 25 commits into
Conversation
…ry for cleanup of two issues. The perform this in a reasonable time frame I suggest adding two indexes. See documentation for justification. This commit is not intended to be merged, it is only created for conveience and review.
RagnarFatland
marked this pull request as draft
May 28, 2026 07:00
Contributor
📝 WalkthroughWalkthroughThis PR introduces a new DialogActivityExporter console application to export dialog activity records from PostgreSQL to CSV in batches. The solution includes the complete tool with CLI argument parsing, Azure AD authentication, batched CSV export via cursor pagination, comprehensive database query documentation, index optimization strategy with deployment scripts, and user-facing guides for operation and troubleshooting. ChangesDialog Activity Exporter Tool & Database Optimization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (1)
docs/database/Index_Creation_Scripts.sql (1)
212-452: ⚡ Quick winTrim the post-script impact assessment from this executable SQL file.
After the first
END OF SCRIPT, the file switches into a long mixed-scope narrative (including additional indexes and runtime claims). Keeping this in the deployment SQL increases operator error risk and drifts from the dedicated markdown docs.🤖 Prompt for 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. In `@docs/database/Index_Creation_Scripts.sql` around lines 212 - 452, The SQL file contains a long post-script narrative after the "-- ============================================================================ -- END OF SCRIPT" marker (including sections like "RUNTIME WRITE PERFORMANCE IMPACT", "DISK SPACE REQUIREMENTS", "MIGRATION STRATEGY RECOMMENDATION" and the IdempotencyKeys/ExternalReferences/A2Parties index narratives) that should not live in an executable SQL file; remove or relocate everything after the first "-- END OF SCRIPT" marker so the SQL file ends immediately at that marker and move the entire impact assessment and narrative text to the project's documentation (e.g., markdown in docs), ensuring references to the specific index names from the script (IX_IdempotencyKeys_Lookup_Composite, IX_ExternalReferences_CorrespondenceId_ReferenceType, IX_A2Parties_PartyUuid_Covering, IX_CorrespondenceStatuses_*) are preserved in the docs for operator guidance.
🤖 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 `@docs/database/Index_Creation_Scripts.sql`:
- Around line 120-123: The monitoring section wrongly contains an index-creation
DDL for IX_A2Parties_PartyUuid_Covering on correspondence."A2Parties"; remove
this CREATE INDEX CONCURRENTLY statement (or move it to the dedicated A2Parties
optimization script) so the monitoring section only contains non-mutating
monitoring queries; ensure any notes or references to A2Parties optimization
remain consistent with the line-16 comment that it is handled separately.
- Around line 43-50: Update the pg_stat_user_indexes queries to use the correct
column names relname and indexrelname instead of tablename/indexname (e.g., in
the SELECTs that currently reference tablename/indexname and filter by
indexrelname), and fix pg_relation_size calls that pass mixed-case, quoted
identifiers as plain strings by using a properly quoted regclass cast like
'schema."IndexName"'::regclass (or build with quote_ident/format) so
mixed-case/quoted index names resolve correctly.
In `@docs/database/Query_Documentation.md`:
- Line 107: The docs entry for the cutoff example is inconsistent: the table row
shows `stats."StatusChanged" < '2026-05-19'` while the actual query uses a
timestamp cutoff `'2026-05-19 11:35:59'`; update the documentation to match the
query by changing the cutoff to the full timestamp (`'2026-05-19 11:35:59'`)
wherever `stats."StatusChanged"` is documented, or alternatively update the
query example to use a date-only cutoff, ensuring `stats."StatusChanged"` is
documented consistently across the file.
- Line 115: Several fenced code blocks in Query_Documentation.md are missing
language identifiers (triggering markdownlint MD040); update each
triple-backtick block (notably the blocks around the current entries near line
115 and 151) to include the appropriate language tag (e.g., ```sql, ```js,
```json, or ```bash) so the code is syntax-highlighted and MD040 is resolved;
scan the file for other ``` blocks and add the correct language identifier based
on the snippet content.
In `@docs/database/Quick_Reference.md`:
- Line 103: Update the answer text that currently reads "A: Index will be marked
INVALID. Drop it with `DROP INDEX` and recreate." to use `DROP INDEX
CONCURRENTLY` instead (e.g., "A: Index will be marked INVALID. Drop it with
`DROP INDEX CONCURRENTLY` and recreate.") so it matches the rollback section and
uses the safer concurrent drop in production.
- Line 55: The "Disk Space" row currently shows "After: -13.5 GB" which implies
a reduction; update that table cell to clearly indicate this is additional index
footprint and a positive increase (for example change "-13.5 GB" to "+13.5 GB"
or "13.5 GB increase" / "Additional index footprint: 13.5 GB") so readers aren't
confused; locate the table row labeled "Disk Space" (the cell containing "-13.5
GB") and replace the value and/or add clarifying text to indicate it is an
increase.
- Line 82: The WHERE clause currently uses double-escaped quotes (WHERE
indexname LIKE ''IX_CorrespondenceStatuses%'') which is invalid SQL; replace the
escaped literal with a normal SQL string literal such as WHERE indexname LIKE
'IX_CorrespondenceStatuses%' so the LIKE pattern is quoted correctly (ensure
only single quotes around IX_CorrespondenceStatuses%).
In `@docs/database/README.md`:
- Line 61: The fenced code block currently uses a plain ``` fence which triggers
markdownlint MD040; add an explicit language tag after the opening backticks
(for example ```bash, ```sh, ```text, or the appropriate language for the
snippet) so the block is labeled and MD040 is satisfied—update the opening ```
to include the correct language token matching the block contents.
In `@docs/database/Technical_Documentation.md`:
- Line 40: The unlabelled fenced code block (the standalone "```" fence) in
Technical_Documentation.md should include a language tag to satisfy markdownlint
MD040; locate the triple-backtick fence and replace the opening fence with a
labeled one (e.g., ```text, ```bash, ```json, or the appropriate language for
the block) so the fence is no longer unlabeled.
- Around line 205-206: Update the "Table Locks" row for CREATE INDEX
CONCURRENTLY to avoid saying "None" or "prevents locks" and instead state that
it "doesn't block writes/DML but acquires a lightweight lock (e.g., SHARE UPDATE
EXCLUSIVE) that can conflict with other schema/DDL operations"; specifically
edit the table cell referencing CREATE INDEX CONCURRENTLY and the accompanying
description so it clearly mentions non-blocking DML but possible conflicts with
other schema-changing operations.
In `@docs/DialogActivityExport_CombinedMode.md`:
- Around line 58-60: Add a language tag to the fenced code block that contains
the progress example string "[████████████░░░░░░░░] 47.23% |
70,845,000/157,000,000 | 12,450 rows/sec | ETA: 01:45:32" by changing the
opening fence from ``` to ```text (or another appropriate language) so the block
satisfies markdownlint MD040.
- Around line 88-90: The current manual merge pipeline treats both files as a
single stream and uses Select-Object -Skip 1 which only removes the very first
header, leaving the second file's header as data; fix by reading the first CSV
intact and then appending the remaining CSVs with their first line removed
(i.e., use Get-Content on the first file, then for each subsequent file use
Get-Content piped to Select-Object -Skip 1 before combining and Out-File to
produce combined.csv) so only the initial header remains.
In `@docs/DialogActivityExport_Summary.md`:
- Around line 128-145: The fenced code block showing the directory tree in
docs/DialogActivityExport_Summary.md is missing a language tag and triggers
markdownlint MD040; update the opening fence to include the language `text`
(i.e., change ``` to ```text) for the tree block so the snippet is properly
annotated without altering its contents.
In
`@tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs`:
- Around line 330-337: The CSV output only calls EscapeCSV for ActorName; update
the call-site that builds the CSV row in FormatCSVLine usage to escape all
string fields (DialogId, DialogActivityId string, Timestamp string, ActorId,
ActorName, ActivityType) before passing them to FormatCSVLine so any embedded
quotes/commas are handled; reuse the existing EscapeCSV method for each field
(convert nullable Guid via ToString() then EscapeCSV) so every element is safely
quoted and CSV-safe.
- Around line 274-310: The query in DialogActivityExportService.cs uses
single-column paging on stats."CorrespondenceId" (and the code advances the
cursor via lastProcessedId = reader.GetGuid(2)) which can skip rows with the
same CorrespondenceId across the UNION ALL; change to a stable composite cursor
(e.g., (CorrespondenceId, Timestamp, Status) or (CorrespondenceId, Timestamp,
ActivityType)) and update the SQL WHERE and ORDER BY to use the composite
comparison and deterministic ordering (include the same columns in ORDER BY and
SELECT), and update the cursor handling in the reader/lastProcessedId logic (and
any variables like `@lastId`, reader.GetGuid(2), timestampColumn, batchSize, and
the code that stores/parses the last cursor) so the service compares the full
composite cursor when fetching the next page.
- Around line 246-310: The SQL in the batchQuery string has two syntax issues:
fix the mis-quoted join column by changing ap."" PartyUuid"" to ap.""PartyUuid""
in both UNION branches, and remove the per-branch ORDER BY
stats.""CorrespondenceId"" clauses (the two ORDER BY lines inside each SELECT)
so only the final ORDER BY ""CorrespondenceId"" LIMIT `@batchSize` remains; update
the string construction in DialogActivityExportService.cs where batchQuery is
built (references: batchQuery, timestampColumn, syncFilter, createdFilter,
idcFetch/idcConfirm).
In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs`:
- Around line 81-96: The progress rendering uses console cursor APIs that throw
when stdout is redirected; wrap the interactive cursor/width operations in a
guard checking Console.IsOutputRedirected (or Console.IsOutputRedirected ==
false) before calling Console.SetCursorPosition or accessing
Console.WindowWidth, and fall back to a non-interactive plain-line progress
output when redirected; update the Progress<ExportProgress> handler (variable
progress) and any use of CreateProgressBar(percent, 40) to avoid relying on
Console.WindowWidth in the redirected case (e.g., use a fixed width or skip the
bar), ensuring no Console.SetCursorPosition/Console.WindowWidth calls execute
when Console.IsOutputRedirected is true.
In `@tools/Altinn.Correspondence.DialogActivityExporter/README.md`:
- Around line 170-173: Update the README links that currently point to
docs/database/DBA_Index_Creation_Scripts.sql and
docs/database/DBA_Index_Request_Executive_Summary.md to the repo's current
document names (e.g., docs/database/Index_Creation_Scripts.sql and
docs/database/Quick_Reference.md) so they resolve correctly; locate and edit the
three-line block in tools/Altinn.Correspondence.DialogActivityExporter/README.md
that lists the index docs and replace the old DBA_* filenames with the new
filenames used in this PR.
- Line 127: The README's `--oldest` flag docs state the format as `yyyy-MM-dd
HH:mm:ss` but examples use date-only values like `2019-03-23`; update the
documentation for the `--oldest` flag to match the examples (or update the
examples to include time) so there is no ambiguity: change the documented format
string for `--oldest` to accept either `yyyy-MM-dd` or `yyyy-MM-dd HH:mm:ss` and
update all occurrences (including the examples showing `2019-03-23`) to
demonstrate the accepted formats and a parsing note; look for the `--oldest`
references in the README and make the format consistent across the listed lines.
- Around line 208-210: The fenced code block showing the progress bar example is
missing a language tag which triggers markdownlint MD040; update the
triple-backtick fence that contains "[████████████░░░░░░░░] 47.23% |
70,845,000/150,000,000 | 12,450 rows/sec | ETA: 01:45:32" to use a language tag
(e.g., add "text" so it reads ```text) so the block is properly annotated for
markdownlint.
---
Nitpick comments:
In `@docs/database/Index_Creation_Scripts.sql`:
- Around line 212-452: The SQL file contains a long post-script narrative after
the "--
============================================================================
-- END OF SCRIPT" marker (including sections like "RUNTIME WRITE PERFORMANCE
IMPACT", "DISK SPACE REQUIREMENTS", "MIGRATION STRATEGY RECOMMENDATION" and the
IdempotencyKeys/ExternalReferences/A2Parties index narratives) that should not
live in an executable SQL file; remove or relocate everything after the first
"-- END OF SCRIPT" marker so the SQL file ends immediately at that marker and
move the entire impact assessment and narrative text to the project's
documentation (e.g., markdown in docs), ensuring references to the specific
index names from the script (IX_IdempotencyKeys_Lookup_Composite,
IX_ExternalReferences_CorrespondenceId_ReferenceType,
IX_A2Parties_PartyUuid_Covering, IX_CorrespondenceStatuses_*) are preserved in
the docs for operator guidance.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fb609bf1-c667-412b-9fdb-87bb41b6e329
📒 Files selected for processing (13)
Altinn.Correspondence.slndocs/DialogActivityExport_CombinedMode.mddocs/DialogActivityExport_Summary.mddocs/database/Index_Creation_Scripts.sqldocs/database/Query_Documentation.mddocs/database/Quick_Reference.mddocs/database/README.mddocs/database/Technical_Documentation.mdtools/Altinn.Correspondence.DialogActivityExporter/Altinn.Correspondence.DialogActivityExporter.csprojtools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cstools/Altinn.Correspondence.DialogActivityExporter/Program.cstools/Altinn.Correspondence.DialogActivityExporter/README.mdtools/Altinn.Correspondence.DialogActivityExporter/appsettings.json
…uid urn in recipient
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/database/Fix_A2Parties_Recipient_Filter.sql`:
- Around line 21-24: The header in Fix_A2Parties_Recipient_Filter.sql
incorrectly claims "no indexes, fast operations" and "Estimated time: < 1
minute" while the script creates a covering index using CREATE INDEX
CONCURRENTLY; update the deployment header to state that the migration creates a
covering index (via CREATE INDEX CONCURRENTLY), note that index creation on
large tables can take from minutes to hours depending on table size, clarify
that CONCURRENTLY avoids exclusive locks but does not guarantee a short runtime,
and adjust the Estimated time and downtime statement accordingly.
- Around line 172-175: The script incorrectly uses PL/pgSQL-only RAISE NOTICE at
top level; update the file so the ALTER TABLE on correspondence."A2Parties"
altering column "RecipientUrn" remains as-is and move the RAISE NOTICE into a
PL/pgSQL block (e.g., wrap the message in a DO $$ BEGIN RAISE NOTICE 'Added NOT
NULL constraint to RecipientUrn'; END $$;), or alternatively replace the RAISE
NOTICE with a top-level statement such as COMMENT ON COLUMN
correspondence."A2Parties"."RecipientUrn" IS 'Added NOT NULL constraint' or a
simple SELECT '...' to emit a message; ensure the identifier
correspondence."A2Parties" and column "RecipientUrn" are referenced exactly as
in the diff.
In `@docs/database/Index_Creation_Scripts.sql`:
- Around line 3-16: The file contains duplicated CREATE TABLE statements for
correspondence."A2Parties" (same columns and CONSTRAINT "PK_A2Parties"), which
will cause "relation already exists" errors; remove the repeated block so only
one CREATE TABLE corresponds to correspondence."A2Parties" remains, or replace
the second duplicate with an existence guard (e.g., CREATE TABLE IF NOT EXISTS
logic) if the intention is to make the script idempotent. Ensure the remaining
statement preserves the "PartyUuid" primary key constraint "PK_A2Parties".
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45dd4c70-8867-40ce-847b-a5efe2c592b9
📒 Files selected for processing (6)
docs/database/Fix_A2Parties_Recipient_Filter.sqldocs/database/Index_Creation_Scripts.sqldocs/database/Query_Documentation.mddocs/database/Quick_Reference.mddocs/database/README.mdtools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs
✅ Files skipped from review due to trivial changes (2)
- docs/database/README.md
- docs/database/Quick_Reference.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/database/Query_Documentation.md
- tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs
Contributor
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 `@docs/database/Fix_A2Parties_Recipient_Filter.sql`:
- Around line 190-192: The CREATE INDEX CONCURRENTLY statement for
"IX_A2Parties_PartyUuid_Covering" on correspondence."A2Parties" must run outside
any transaction; move this statement into a separate migration script that your
runner executes without an explicit BEGIN/COMMIT (or add a clear top-of-file
guardrail note that instructs the runner to skip transaction wrapping), and
ensure the new script contains only this non-transactional index creation so
earlier DDL in the original migration cannot cause the CONCURRENTLY call to
fail.
- Around line 21-27: The deployment header overstates zero-impact: clarify that
ALTER TABLE ... RENAME COLUMN on A2Parties, adding a column, and ALTER COLUMN
"RecipientUrn" SET NOT NULL can acquire locks and may validate/scan rows
(non-zero impact), so remove “no downtime/production unaffected” and note
potential locking/scan behavior; additionally, explicitly state that CREATE
INDEX CONCURRENTLY must be executed outside any BEGIN/COMMIT transaction (and
thus migration runners that wrap migrations in a transaction will fail), and
recommend running the CONCURRENTLY index step separately or using a
non-transactional migration execution for that statement.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24611b1b-3b3c-4c5a-87b8-12ccaf7c6a56
📒 Files selected for processing (8)
docs/database/Fix_A2Parties_Recipient_Filter.sqldocs/database/Index_Creation_Scripts.sqldocs/database/Query_Documentation.mddocs/database/Quick_Reference.mddocs/database/README.mdtools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cstools/Altinn.Correspondence.DialogActivityExporter/Program.cstools/Altinn.Correspondence.DialogActivityExporter/README.md
✅ Files skipped from review due to trivial changes (4)
- docs/database/README.md
- docs/database/Quick_Reference.md
- tools/Altinn.Correspondence.DialogActivityExporter/README.md
- docs/database/Query_Documentation.md
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/database/Index_Creation_Scripts.sql
- tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs
- tools/Altinn.Correspondence.DialogActivityExporter/Program.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tools/Altinn.Correspondence.DialogActivityExporter/Program.cs (1)
86-88:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDivision by zero when elapsed time is zero.
When
p.ElapsedTime.TotalSecondsis 0 (e.g., first progress report), Line 87 divides by zero, resulting inNaNorInfinityin the output. The interactive branch (Line 101) has the same issue.Proposed fix
if (Console.IsOutputRedirected) { // Non-interactive output for redirected/piped scenarios var percent = p.PercentComplete; - var rate = p.TotalProcessed / p.ElapsedTime.TotalSeconds; + var rate = p.ElapsedTime.TotalSeconds > 0 + ? p.TotalProcessed / p.ElapsedTime.TotalSeconds + : 0; var eta = p.EstimatedTimeRemaining;Apply the same fix to Line 101 in the interactive branch.
🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs` around lines 86 - 88, The calculation of rate uses p.ElapsedTime.TotalSeconds without guarding against zero, causing division by zero; update the logic where rate is computed (the block using p.PercentComplete, p.TotalProcessed, p.ElapsedTime.TotalSeconds, p.EstimatedTimeRemaining) to first check if p.ElapsedTime.TotalSeconds > 0 and only then compute rate = p.TotalProcessed / p.ElapsedTime.TotalSeconds, otherwise set rate to 0 (or a safe default); make the identical guard and fallback in the interactive branch where the same computation occurs so both code paths avoid NaN/Infinity.
🧹 Nitpick comments (1)
tools/Altinn.Correspondence.DialogActivityExporter/Program.cs (1)
363-368: ⚡ Quick winHardcoded production server and email domain.
The connection string builder embeds
altinn-corr-prod-dbserver.postgres.database.azure.comand@ai-dev.no. This couples the tool to a specific environment and may fail for users in different domains.Consider reading server hostname and domain from configuration or environment variables (e.g.,
CORRESPONDENCE_DB_HOST,AZURE_AD_DOMAIN), or at minimum document these assumptions in the help text and README.🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs` around lines 363 - 368, The connection string in Program.cs currently hardcodes the production host and email domain causing environment coupling; change the code that builds connectionString to read the DB host and AD domain from environment/config (e.g., CORRESPONDENCE_DB_HOST and AZURE_AD_DOMAIN) and fall back to the current values if those variables are absent, using the existing username and token variables when interpolating; also update any help text or README to document these env vars so users can override host and domain without editing code.
🤖 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 `@docs/database/Fix_A2Parties_Recipient_Filter_Index.sql`:
- Around line 51-58: The DO block uses current_setting('transaction_isolation',
true) to detect a transaction, which always returns a value and thus always
emits warnings; replace that check with pg_current_xact_id_if_assigned() (which
returns NULL when not inside an explicit transaction) so the warnings about
CREATE INDEX CONCURRENTLY only trigger when pg_current_xact_id_if_assigned() IS
NOT NULL; update the conditional inside the DO $$ ... $$ block accordingly and
keep the existing warning messages and context about CREATE INDEX CONCURRENTLY.
---
Duplicate comments:
In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs`:
- Around line 86-88: The calculation of rate uses p.ElapsedTime.TotalSeconds
without guarding against zero, causing division by zero; update the logic where
rate is computed (the block using p.PercentComplete, p.TotalProcessed,
p.ElapsedTime.TotalSeconds, p.EstimatedTimeRemaining) to first check if
p.ElapsedTime.TotalSeconds > 0 and only then compute rate = p.TotalProcessed /
p.ElapsedTime.TotalSeconds, otherwise set rate to 0 (or a safe default); make
the identical guard and fallback in the interactive branch where the same
computation occurs so both code paths avoid NaN/Infinity.
---
Nitpick comments:
In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs`:
- Around line 363-368: The connection string in Program.cs currently hardcodes
the production host and email domain causing environment coupling; change the
code that builds connectionString to read the DB host and AD domain from
environment/config (e.g., CORRESPONDENCE_DB_HOST and AZURE_AD_DOMAIN) and fall
back to the current values if those variables are absent, using the existing
username and token variables when interpolating; also update any help text or
README to document these env vars so users can override host and domain without
editing code.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14fd499e-7a3c-49bb-af68-f4211479797a
📒 Files selected for processing (12)
docs/DialogActivityExport_CombinedMode.mddocs/DialogActivityExport_Summary.mddocs/database/Fix_A2Parties_Recipient_Filter_Index.sqldocs/database/Fix_A2Parties_Recipient_Filter_Schema.sqldocs/database/Index_Creation_Scripts.sqldocs/database/Query_Documentation.mddocs/database/Quick_Reference.mddocs/database/README.mddocs/database/Technical_Documentation.mdtools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cstools/Altinn.Correspondence.DialogActivityExporter/Program.cstools/Altinn.Correspondence.DialogActivityExporter/README.md
✅ Files skipped from review due to trivial changes (7)
- docs/DialogActivityExport_Summary.md
- docs/database/README.md
- docs/DialogActivityExport_CombinedMode.md
- docs/database/Technical_Documentation.md
- tools/Altinn.Correspondence.DialogActivityExporter/README.md
- docs/database/Query_Documentation.md
- docs/database/Quick_Reference.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs
- docs/database/Index_Creation_Scripts.sql
…st before export.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/database/Fix_A2Parties_Recipient_Filter_Index.sql`:
- Around line 75-120: The index presence check is insufficient because CREATE
INDEX CONCURRENTLY can leave an invalid index entry; update the verification
logic for IX_A2Parties_PartyUuid_Covering to join pg_class/pg_index (or query
pg_index.indisvalid) and ensure indisvalid = true before reporting "Index
Creation - SUCCESS" (use the index name IX_A2Parties_PartyUuid_Covering to
locate the row), and change the size lookup to use
to_regclass('correspondence.IX_A2Parties_PartyUuid_Covering') (NULL-safe)
instead of the ::regclass cast so the diagnostic SELECT won’t error when the
index is missing or invalid.
In `@docs/database/Fix_A2Parties_Recipient_Filter_Schema.sql`:
- Around line 150-175: The migration currently selects RecipientUrn into
v_selfidentified_sample, v_person_sample, and v_org_sample then emits them with
RAISE NOTICE, which can leak identifiers; change the RAISE NOTICE calls in the
sample block (the SELECT "RecipientUrn" INTO ... statements and the RAISE NOTICE
'Sample RecipientUrn formats:' section) to log only redacted versions (e.g.,
mask all but a fixed prefix or replace with a deterministic hash or just
indicate presence/type) instead of printing the raw v_selfidentified_sample,
v_person_sample, v_org_sample values so CI/ops logs never contain full URNs.
In `@docs/database/Test_Export_Query.sql`:
- Around line 62-97: The UNION ALL branches are invalid because each SELECT
contains its own ORDER BY (remove the ORDER BY lines inside the individual
branches and place a single ORDER BY stats."CorrespondenceId", stats."Status"
after the whole UNION ALL), and the cursor tuple logic is wrong: stop hardcoding
the second element per branch (the commented cursor lines that use ..., 4 or
..., 6); instead use the actual last-returned tuple (lastId, lastStatus) for the
tuple comparison or rewrite the pagination predicate as
(stats."CorrespondenceId" > lastId) OR (stats."CorrespondenceId" = lastId AND
stats."Status" > lastStatus) so the branches using stats."CorrespondenceId" and
stats."Status" (and the commented cursor lines near WHERE) correctly page
without skipping rows.
- Around line 61-63: The keyset pagination predicate in Test_Export_Query.sql is
inconsistent across the UNION ALL branches and must use the same last-row cursor
tuple everywhere. Update the branch filters around the existing
stats."CorrespondenceId"/stats."Status" comparisons so they all use the single
cursor values (lastCorrespondenceId, lastStatus) rather than hardcoded
branch-specific statuses like 4 or 6, and keep the global ORDER BY on
stats."CorrespondenceId", stats."Status" aligned with that cursor logic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d02de6d-ab2c-4981-95b5-0514984d91e9
📒 Files selected for processing (13)
docs/DialogActivityExport_CombinedMode.mddocs/DialogActivityExport_Summary.mddocs/database/Fix_A2Parties_Recipient_Filter_Index.sqldocs/database/Fix_A2Parties_Recipient_Filter_Schema.sqldocs/database/Index_Creation_Scripts.sqldocs/database/Query_Documentation.mddocs/database/Quick_Reference.mddocs/database/README.mddocs/database/Technical_Documentation.mddocs/database/Test_Export_Query.sqltools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cstools/Altinn.Correspondence.DialogActivityExporter/Program.cstools/Altinn.Correspondence.DialogActivityExporter/README.md
✅ Files skipped from review due to trivial changes (6)
- docs/database/README.md
- docs/DialogActivityExport_Summary.md
- docs/DialogActivityExport_CombinedMode.md
- docs/database/Technical_Documentation.md
- docs/database/Quick_Reference.md
- tools/Altinn.Correspondence.DialogActivityExporter/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/database/Index_Creation_Scripts.sql
- tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs
- tools/Altinn.Correspondence.DialogActivityExporter/Program.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
docs/database/Fix_A2Parties_Recipient_Filter_Schema.sql (1)
150-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact sample
RecipientUrnvalues in migration notices.This block still emits raw identifiers to logs. Replace raw values with masked/presence-only output to avoid PII leakage in CI/CD and ops logs.
🔐 Minimal redaction patch
- IF v_selfidentified_sample IS NOT NULL THEN - RAISE NOTICE ' Self-identified: %', v_selfidentified_sample; - END IF; - IF v_person_sample IS NOT NULL THEN - RAISE NOTICE ' Person: %', v_person_sample; - END IF; - IF v_org_sample IS NOT NULL THEN - RAISE NOTICE ' Organization: %', v_org_sample; - END IF; + IF v_selfidentified_sample IS NOT NULL THEN + RAISE NOTICE ' Self-identified: [present]'; + END IF; + IF v_person_sample IS NOT NULL THEN + RAISE NOTICE ' Person: [present]'; + END IF; + IF v_org_sample IS NOT NULL THEN + RAISE NOTICE ' Organization: [present]'; + END IF;🤖 Prompt for 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. In `@docs/database/Fix_A2Parties_Recipient_Filter_Schema.sql` around lines 150 - 175, The migration emits raw RecipientUrn values (v_selfidentified_sample, v_person_sample, v_org_sample) into logs; change the RAISE NOTICE calls to avoid PII by replacing the raw v_* variables with masked/presence-only strings (e.g. a fixed "<redacted>" or a deterministic masked form like showing only a safe prefix) when printing sample RecipientUrn values from correspondence."A2Parties"; update the IF blocks that call RAISE NOTICE so they output the redacted text instead of the unmodified variable.
🤖 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.
Duplicate comments:
In `@docs/database/Fix_A2Parties_Recipient_Filter_Schema.sql`:
- Around line 150-175: The migration emits raw RecipientUrn values
(v_selfidentified_sample, v_person_sample, v_org_sample) into logs; change the
RAISE NOTICE calls to avoid PII by replacing the raw v_* variables with
masked/presence-only strings (e.g. a fixed "<redacted>" or a deterministic
masked form like showing only a safe prefix) when printing sample RecipientUrn
values from correspondence."A2Parties"; update the IF blocks that call RAISE
NOTICE so they output the redacted text instead of the unmodified variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 023a9f96-c8f3-434e-ac3b-06c5e9db68af
📒 Files selected for processing (14)
docs/DialogActivityExport_CombinedMode.mddocs/DialogActivityExport_Summary.mddocs/database/Fix_A2Parties_Recipient_Filter_Index.sqldocs/database/Fix_A2Parties_Recipient_Filter_Schema.sqldocs/database/Index_Creation_Scripts.sqldocs/database/Query_Documentation.mddocs/database/Quick_Reference.mddocs/database/README.mddocs/database/Technical_Documentation.mddocs/database/Test_Export_Query.sqldocs/database/Test_Export_Query_Verification.mdtools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cstools/Altinn.Correspondence.DialogActivityExporter/Program.cstools/Altinn.Correspondence.DialogActivityExporter/README.md
✅ Files skipped from review due to trivial changes (7)
- docs/database/Test_Export_Query_Verification.md
- docs/DialogActivityExport_CombinedMode.md
- docs/DialogActivityExport_Summary.md
- docs/database/Query_Documentation.md
- docs/database/Technical_Documentation.md
- tools/Altinn.Correspondence.DialogActivityExporter/README.md
- docs/database/Quick_Reference.md
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/database/README.md
- tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs
- docs/database/Index_Creation_Scripts.sql
- tools/Altinn.Correspondence.DialogActivityExporter/Program.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/Altinn.Correspondence.DialogActivityExporter/Program.cs (1)
417-427:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix Entra PostgreSQL connection-string Username derivation—don’t use
Environment.UserName
tools/Altinn.Correspondence.DialogActivityExporter/Program.cs(lines ~417-427) setsUsername={Environment.UserName}@ai-dev.no`` while using an Entra access token as the password. For Entra authentication on Azure Database for PostgreSQL Flexible Server, the connection-stringUsernamemust match the exact Microsoft Entra principal granted access (UPN for user accounts; application/client id or object id for service principals/managed identities). `Environment.UserName` won’t reliably match that identity, so DB login can fail even if token acquisition succeeds. Require the Entra principal identifier (Username) via explicit CLI/config and use `DefaultAzureCredential` only to obtain the token.🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs` around lines 417 - 427, The code sets Username using Environment.UserName which is incorrect for Entra-authenticated Postgres; change the logic so the Entra principal identifier is provided explicitly (via CLI flag/config value) and used as the Username when building connectionString, while DefaultAzureCredential (or the existing token variable) is only used to obtain the access token; specifically replace usage of the username variable derived from Environment.UserName with a configured principal (e.g., upn/service-principal-id) and ensure connectionString uses that configuredPrincipal and token as the Password in the Program.cs connectionString construction.tools/Altinn.Correspondence.DialogActivityExporter/README.md (1)
16-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate README to match the tool’s Azure.Identity
DefaultAzureCredentialauth flow and fix broken DBA_ doc links*
tools/Altinn.Correspondence.DialogActivityExporter/README.mdcurrently states Azure CLI is required and the token comes fromaz account get-access-token, buttools/Altinn.Correspondence.DialogActivityExporter/Program.csusesDefaultAzureCredential.GetTokenAsync(Azure CLI/Visual Studio/VS Code/managed identity, etc.), so the README should remove/soften the “Azure CLI required” wording and describe the actual flow.tools/Altinn.Correspondence.DialogActivityExporter/README.md“Database Requirements” links todocs/database/DBA_Index_Creation_Scripts.sqlanddocs/database/DBA_Index_Request_Executive_Summary.md, which don’t exist; update them to the correct existing filenames (e.g.,docs/database/Index_Creation_Scripts.sql/docs/database/Index_Creation_Production_Summary.md).🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/README.md` around lines 16 - 17, Update the README to reflect that authentication uses Azure.Identity's DefaultAzureCredential flow (as invoked by DefaultAzureCredential.GetTokenAsync in Program.cs) rather than requiring the Azure CLI; remove or soften the "Azure CLI required" wording and list supported credential sources (Azure CLI, Visual Studio, VS Code, managed identity) and how to select them. Also fix the broken Database Requirements links by replacing references to docs/database/DBA_Index_Creation_Scripts.sql and docs/database/DBA_Index_Request_Executive_Summary.md with the correct filenames (e.g., docs/database/Index_Creation_Scripts.sql and docs/database/Index_Creation_Production_Summary.md) so the links resolve.
♻️ Duplicate comments (3)
tools/Altinn.Correspondence.DialogActivityExporter/README.md (1)
208-210:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the database doc links again.
These paths still point to
DBA_*files, but the current docs set in this PR uses renamed files such asIndex_Creation_Scripts.sql. The README links will 404 as written.🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/README.md` around lines 208 - 210, Update the two database doc links in the README so they point to the renamed files used in this PR: replace references to `docs/database/DBA_Index_Creation_Scripts.sql` and `docs/database/DBA_Index_Request_Executive_Summary.md` with the current filenames `docs/database/Index_Creation_Scripts.sql` and `docs/database/Index_Request_Executive_Summary.md` respectively; modify the lines in the README that list these links to avoid 404s.docs/database/Fix_A2Parties_Recipient_Filter_Schema.sql (1)
149-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact the sampled
RecipientUrnvalues before logging.This verification block still prints raw person/org/self-identified URNs into migration logs. For an ops-facing script, that unnecessarily exposes identifiers; log only presence/type or a masked value instead.
🤖 Prompt for 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. In `@docs/database/Fix_A2Parties_Recipient_Filter_Schema.sql` around lines 149 - 175, The migration currently logs raw URNs from v_selfidentified_sample, v_person_sample, and v_org_sample; update the verification block so it does not print raw identifiers—either log only the type detected (e.g., "Self-identified present") or replace the URN with a masked string (e.g., keep scheme and mask the rest) before passing into RAISE NOTICE. Locate the RAISE NOTICE lines that reference v_selfidentified_sample, v_person_sample, and v_org_sample and change them to output a redacted value or a presence message (use a simple masking transformation such as regexp_replace or substring to replace identifying segments) while preserving the existing conditional checks. Ensure no raw URN variables are interpolated into logs after the change.docs/database/Test_Export_Query.sql (1)
60-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the actual last
(CorrespondenceId, Status)tuple in every cursor example.The commented examples still hardcode
(last-uuid, 4)/(last-uuid, 6). Because the merged export order is(CorrespondenceId, Status), that can skip the boundary row when one batch ends on status4and the next valid row for the same correspondence is status6.Also applies to: 91-92, 136-137, 167-168
🤖 Prompt for 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. In `@docs/database/Test_Export_Query.sql` around lines 60 - 61, The cursor examples hardcode ('last-uuid', 4)/(6) which can skip the boundary row because the sort/merge is on (CorrespondenceId, Status); update each commented pagination example (the AND (stats."CorrespondenceId", stats."Status") > (...)) to show the actual last tuple from the prior batch—use a realistic UUID and the exact last Status value (e.g., ('<last-CorrespondenceId>'::uuid, <last_Status>)) so the tuple matches the last returned row; apply this change to all occurrences referenced (the examples around the cursor pagination blocks).
🧹 Nitpick comments (3)
tools/Altinn.Correspondence.DialogActivityExporter/test-export.ps1 (2)
98-101: ⚡ Quick winCross-platform compatibility: hardcoded Windows path.
The default output path uses
C:\temp, which is Windows-specific. The script's shebang (#!/usr/bin/env pwsh) suggests cross-platform support, but this path won't work on Linux/macOS.🔧 Proposed fix: Use cross-platform temp directory
# Generate default output path if not provided if ([string]::IsNullOrEmpty($OutputPath)) { $timestamp = Get-Date -Format "yyyyMMdd_HHmmss" - $OutputPath = "C:\temp\test_export_$($Issue)_$($timestamp).csv" + $tempDir = if ($IsWindows -or $env:OS -match "Windows") { "C:\temp" } else { "/tmp" } + $OutputPath = Join-Path $tempDir "test_export_$($Issue)_$($timestamp).csv" }Alternatively, use PowerShell's built-in
[System.IO.Path]::GetTempPath():if ([string]::IsNullOrEmpty($OutputPath)) { $timestamp = Get-Date -Format "yyyyMMdd_HHmmss" - $OutputPath = "C:\temp\test_export_$($Issue)_$($timestamp).csv" + $tempDir = [System.IO.Path]::GetTempPath() + $OutputPath = Join-Path $tempDir "test_export_$($Issue)_$($timestamp).csv" }🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/test-export.ps1` around lines 98 - 101, The script sets a Windows-only default path using "C:\temp" when $OutputPath is empty; replace this with a cross-platform temp directory by using the platform-agnostic temp path provider (e.g. [System.IO.Path]::GetTempPath()) and combine it with the generated filename (which uses $Issue and the timestamp) via Join-Path or equivalent to build $OutputPath; ensure the filename is sanitized for invalid characters before joining so variables like $OutputPath, $Issue and the timestamp continue to be used but without a hardcoded Windows path.
169-169: 💤 Low valueLine counting loads entire file into memory.
Get-Content $OutputPath | Measure-Object -Linereads the entire CSV file into memory to count lines. For large test exports (e.g., MaxBatches=100), this could be slow or cause memory pressure.However, given the test mode default (MaxBatches=2, BatchSize=1000 = ~2000 rows), this is unlikely to be a practical issue. The current implementation is simple and works well for typical test scenarios.
If you want to optimize for larger test runs:
♻️ Optional optimization: Stream-based line count or skip for test mode
- Write-Host "Rows (approx): ~$((Get-Content $OutputPath | Measure-Object -Line).Lines - 1)" -ForegroundColor White + $expectedRows = $BatchSize * $MaxBatches + Write-Host "Rows (approx): ~$expectedRows (based on $MaxBatches batches)" -ForegroundColor WhiteThis avoids reading the file entirely and uses the known test parameters instead.
🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/test-export.ps1` at line 169, The current Write-Host uses Get-Content $OutputPath | Measure-Object -Line which loads the whole file into memory; change it to use a streaming line count or skip counting in test mode: either replace Get-Content with a streaming API like [System.IO.File]::ReadLines($OutputPath) piped to Measure-Object to avoid full-file load, or, when MaxBatches/BatchSize indicate test mode, compute the approximate row count from those variables (e.g., MaxBatches * BatchSize) and display that instead; update the Write-Host call that references $OutputPath so it uses the streamed count or the computed test-mode estimate.docs/database/Testing_Guide.md (1)
373-377: 💤 Low valueAdd language specifiers to fenced code blocks.
Markdown linting reports missing language specifiers for code blocks that appear to contain instructional text (lines 373-377, 380-384).
📝 Proposed fix
-``` +```text 1. In Visual Studio: Tools → Options → Azure Service Authentication 2. Sign in with your Azure account 3. Run test: .\test-export.ps1-
+text
- Install Azure Account extension in VS Code
- Sign in to Azure (Ctrl+Shift+P → "Azure: Sign In")
- Run test: .\test-export.ps1
As per coding guidelines, markdownlint flagged: "Fenced code blocks should have a language specified."
Also applies to: 380-384
🤖 Prompt for 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. In `@docs/database/Testing_Guide.md` around lines 373 - 377, The fenced code blocks in Testing_Guide.md (the block starting with "1. In Visual Studio: Tools → Options → Azure Service Authentication" and the subsequent block "1. Install Azure Account extension in VS Code...") are missing language specifiers causing markdownlint failures; update each triple-backtick fence to include a language specifier (e.g., ```text) and apply the proposed content change for the second block (replace the VS Code instructions as shown in the proposed fix) so both blocks are fenced as ```text ... ``` to satisfy the markdownlint "Fenced code blocks should have a language specified" rule.
🤖 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 `@docs/database/Azure_Identity_Migration_Summary.md`:
- Around line 104-107: Update the fenced code blocks in
docs/database/Azure_Identity_Migration_Summary.md so each has a language
identifier to satisfy markdownlint MD040: add ```text (or ```powershell/```bash
where appropriate) to the block containing "Azure authentication failed. Make
sure you're logged in via Azure CLI (az login), Visual Studio, or VS Code." and
to the blocks with the numbered lists starting "1. Tools → Options → Azure
Service Authentication → Sign in", "1. Install Azure Account extension", "1. Set
environment variables for service principal", and "1. Enable Managed Identity on
Azure resource"; ensure each opening backtick fence includes the chosen language
token and leave the block contents unchanged.
In `@docs/database/Check_Disk_Space_And_Table_Stats.sql`:
- Around line 14-39: The SIZE queries use string concatenation with
schemaname||'.'||tablename which can fail for mixed-case or quoted relations;
update the calls to pg_total_relation_size and pg_relation_size to use the OID
columns instead: use pg_total_relation_size(pg_stat_user_tables.relid) and
pg_relation_size(pg_stat_user_tables.relid) in the first SELECT (referencing
pg_stat_user_tables.relid) and use
pg_relation_size(pg_stat_user_indexes.indexrelid) in the index SELECT
(referencing pg_stat_user_indexes.indexrelid), leaving other selected columns
and WHERE filters (schemaname = 'correspondence' AND tablename =
'CorrespondenceStatuses') unchanged so sizes resolve correctly for
quoted/mixed-case relations.
In `@docs/database/Configure_PostgreSQL_For_Index_Creation.sql`:
- Around line 17-37: The pg_settings.unit conversion CASE in the SELECT (used to
compute pretty_value) doesn't handle units like '8kB' and thus miscomputes
shared_buffers and effective_cache_size; update the logic that computes
pretty_value to detect numeric multipliers in unit strings (e.g., '8kB') and use
current_setting('block_size')::int to convert block-count units into bytes when
unit contains 'kB' or a numeric prefix, ensuring pg_size_pretty((setting::bigint
* <computed_multiplier>)::bigint) is calculated correctly for rows from
pg_settings (refer to the SELECT, pg_settings.unit, and the pretty_value
expression).
In `@docs/database/Index_Creation_Production_Summary.md`:
- Around line 92-107: The SQL snippet for checking dead tuples lacks an opening
fenced code block with language tag; add a triple-backtick fence with "sql"
before the SELECT (the block containing the SELECT ... FROM pg_stat_user_tables
and VACUUM ANALYZE correspondence."CorrespondenceStatuses";) so the section is
fenced as sql, ensuring the markdown renders correctly and satisfies
markdownlint.
- Around line 248-258: The example query uses incorrect column names and a
fragile size expression: update the query on pg_stat_user_indexes to use relname
(alias to tablename) and indexrelname (alias to indexname) instead of
tablename/indexname, and compute size with pg_relation_size(indexrelid) instead
of pg_relation_size(schemaname||'.'||indexname) to avoid identifier/casing
issues; modify the WHERE and ORDER BY to reference relname and indexrelname
accordingly so the query runs in psql and handles quoted names.
In `@docs/database/Index_Creation_Scripts.sql`:
- Around line 25-26: Update the stale comment references that point to
Fix_A2Parties_Indexes.sql and Fix_A2Parties_Recipient_Filter.sql so operators
are directed to the correct companion scripts; replace those mentions with
Fix_A2Parties_Recipient_Filter_Schema.sql and
Fix_A2Parties_Recipient_Filter_Index.sql in the same comment blocks (also apply
the same replacement to the other occurrences around the block referenced as
384-386). Ensure the comments that mention the A2Parties index/recipient filter
explicitly reference the new filenames to match the companion files in this PR.
In `@docs/database/Max_Batches_Feature_Summary.md`:
- Around line 14-23: Update the documented API and CLI examples to match the
current exporter signature by removing references to oldestCorrespondenceDate,
skipTotalCount, and the --oldest flag and replacing runtime COUNT behavior with
the new preCalculatedCount parameter; specifically, update the ExportToCSVAsync
signature example (remove oldestCorrespondenceDate and skipTotalCount,
keep/rename maxBatches if still present) and show passing preCalculatedCount
instead of doing a runtime COUNT, and update any CLI examples to use the new
flag name that supplies preCalculatedCount so the max-batches docs align with
the actual service and CLI signatures.
In `@docs/database/Performance_Optimization_Summary.sql`:
- Around line 59-123: The SQL in this doc diverges from
DialogActivityExportService by using a UNION ALL and LEFT JOIN to
correspondence."IdempotencyKeys", which changes plan and result set; update the
documented query to match the exporter: split into the same two separate SELECTs
used by DialogActivityExportService (one for Status=4 and one for Status=6), use
INNER JOIN to correspondence."IdempotencyKeys" (or the exact join pattern the
service uses) and preserve the same INNER JOINs to
correspondence."Correspondences", correspondence."A2Parties" and
correspondence."ExternalReferences" as in the service, and keep the same
pagination approach (cursor/WHERE tuple) instead of the shown UNION ALL + LIMIT
to ensure EXPLAIN/timings are representative.
In `@docs/database/Query_Logging_Feature_Summary.md`:
- Around line 30-47: The sample SQL in the test-mode logging block (_isTestMode
/ logQuery / _logger.LogInformation) is outdated: remove the
oldestCorrespondenceDate/@oldestDate substitution and any related conditional,
change the documented JOIN from LEFT JOIN IdempotencyKeys to INNER JOIN
IdempotencyKeys, and replace the older cutoff predicate with the Issue-1951
style BETWEEN '2019-03-23 00:00:00' AND `@cutoffTimestamp`; update the sample SQL
strings printed by logQuery (and the same examples referenced at lines noted:
58-88, 111-116) so the logged Query matches the current implementation exactly.
In `@docs/database/Test_Export_Query.sql`:
- Around line 5-20: The SQL branches that apply a keyset predicate on
(CorrespondenceId, Status) and use LIMIT (the Status 4 and Status 6 query blocks
in Test_Export_Query.sql) must include an explicit ORDER BY on the same keys
before LIMIT so keyset batching is deterministic; update those query blocks (and
the corresponding query shape used by DialogActivityExportService.cs where the
cursor predicate + LIMIT `@fetchLimit` is used) to ORDER BY CorrespondenceId,
Status (and any tie-breakers used by the in-memory merge) so each branch returns
a consistent, non-overlapping subset for cursor pagination.
In `@docs/database/Testing_Guide.md`:
- Line 86: Remove or correct the inaccurate claim "offer to open CSV file
automatically" from the documentation where it appears (the exact text string
appears at two places) because the script test-export.ps1 does not implement
that behavior; update both occurrences to either delete the sentence or replace
it with the actual behavior (e.g., "displays file size and row count after
completion") and mention test-export.ps1 by name so readers know the source of
truth.
- Line 93: Update the example and tests to stop implying --oldest is effective:
remove the deprecated --oldest flag from the sample CLI invocation in
Testing_Guide.md (the line containing dotnet run ... --oldest ...) or explicitly
append a note that "--oldest" is deprecated and ignored; also update the test
script test-export.ps1 to either drop passing --oldest or to suppress/expect the
deprecation warning so docs and tests match the exporter behavior. Ensure
references to --oldest in the docs and tests mention the exact deprecation
warning text ("WARNING: --oldest parameter is deprecated and no longer used
(performance optimization)") if you choose to keep it for clarity.
In `@tools/Altinn.Correspondence.DialogActivityExporter/calculate-counts.sql`:
- Around line 14-20: Update the USAGE note in calculate-counts.sql to remove the
outdated guidance about setting counts to 0 to force runtime calculation and
clarify current behavior: state that setting a count to 0 is interpreted by the
exporter as “no total count available” and will cause the UI to show
processed-only progress (not trigger a COUNT query), and instruct operators to
provide explicit non-zero totals or leave the pre-calculated entries out to
allow the exporter’s intended behavior; edit the paragraph under the "-- USAGE:"
header (the usage note lines) to reflect this new semantic and to instruct how
to force runtime calculation if there is a different current mechanism.
In
`@tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs`:
- Around line 147-155: The code treats totalCount as known even when one
per-issue count is zero; change the logic to only consider the combined total as
known when both count1716 and count1951 are > 0: replace the current if
(totalCount > 0) guard around the _logger.LogInformation call with a check
(count1716 > 0 && count1951 > 0) and log the "Total count not available" message
otherwise; also update any downstream percent/ETA calculations that use
totalCount (e.g., PercentComplete or EstimatedTimeRemaining computations in
DialogActivityExportService methods) to skip or fall back to "unknown" timing
when either count1716 or count1951 is zero so percent never exceeds 100% and ETA
never goes negative.
- Around line 346-376: The SQL in the query string used for cursor pagination
lacks an ORDER BY which makes LIMIT non-deterministic; update the query in
DialogActivityExportService where the variable query is built to append an
explicit ORDER BY that matches the cursor tuple used in the WHERE clause (i.e.
ORDER BY stats."CorrespondenceId", stats."Status") so the pagination using
`@lastId` and `@lastStatus` is stable and deterministic before applying LIMIT
`@fetchLimit`.
In
`@tools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sql`:
- Line 48: The performance test only exercises stats."Status" = 4 so add
coverage for Status 6 (Confirmed) to match the export behavior: update each
query block (Query 1A, Query 1B, Query 1, Query 2, Query 3, Query 4) to either
duplicate the block with a Status = 6 variant or modify the WHERE clause to
stats."Status" IN (4, 6) so both status paths and NULL join behavior are
exercised; ensure the duplicated queries use unique labels/names in the test
script and keep the same join/filter structure as the original to preserve
comparability.
In `@tools/Altinn.Correspondence.DialogActivityExporter/test-export.ps1`:
- Around line 111-126: The script assigns to the automatic PowerShell variable
$args which can conflict with undeclared script parameters; rename that array to
a non-reserved name (e.g., $commandArgs) and update all places that build the
argument list (the block that currently references $args, the conditional that
appends connection/azure flags using $ConnectionString and $UseAzureAd) as well
as the later invocation that passes the arguments so it uses $commandArgs
instead; ensure no other references to the automatic $args remain.
---
Outside diff comments:
In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs`:
- Around line 417-427: The code sets Username using Environment.UserName which
is incorrect for Entra-authenticated Postgres; change the logic so the Entra
principal identifier is provided explicitly (via CLI flag/config value) and used
as the Username when building connectionString, while DefaultAzureCredential (or
the existing token variable) is only used to obtain the access token;
specifically replace usage of the username variable derived from
Environment.UserName with a configured principal (e.g.,
upn/service-principal-id) and ensure connectionString uses that
configuredPrincipal and token as the Password in the Program.cs connectionString
construction.
In `@tools/Altinn.Correspondence.DialogActivityExporter/README.md`:
- Around line 16-17: Update the README to reflect that authentication uses
Azure.Identity's DefaultAzureCredential flow (as invoked by
DefaultAzureCredential.GetTokenAsync in Program.cs) rather than requiring the
Azure CLI; remove or soften the "Azure CLI required" wording and list supported
credential sources (Azure CLI, Visual Studio, VS Code, managed identity) and how
to select them. Also fix the broken Database Requirements links by replacing
references to docs/database/DBA_Index_Creation_Scripts.sql and
docs/database/DBA_Index_Request_Executive_Summary.md with the correct filenames
(e.g., docs/database/Index_Creation_Scripts.sql and
docs/database/Index_Creation_Production_Summary.md) so the links resolve.
---
Duplicate comments:
In `@docs/database/Fix_A2Parties_Recipient_Filter_Schema.sql`:
- Around line 149-175: The migration currently logs raw URNs from
v_selfidentified_sample, v_person_sample, and v_org_sample; update the
verification block so it does not print raw identifiers—either log only the type
detected (e.g., "Self-identified present") or replace the URN with a masked
string (e.g., keep scheme and mask the rest) before passing into RAISE NOTICE.
Locate the RAISE NOTICE lines that reference v_selfidentified_sample,
v_person_sample, and v_org_sample and change them to output a redacted value or
a presence message (use a simple masking transformation such as regexp_replace
or substring to replace identifying segments) while preserving the existing
conditional checks. Ensure no raw URN variables are interpolated into logs after
the change.
In `@docs/database/Test_Export_Query.sql`:
- Around line 60-61: The cursor examples hardcode ('last-uuid', 4)/(6) which can
skip the boundary row because the sort/merge is on (CorrespondenceId, Status);
update each commented pagination example (the AND (stats."CorrespondenceId",
stats."Status") > (...)) to show the actual last tuple from the prior batch—use
a realistic UUID and the exact last Status value (e.g.,
('<last-CorrespondenceId>'::uuid, <last_Status>)) so the tuple matches the last
returned row; apply this change to all occurrences referenced (the examples
around the cursor pagination blocks).
In `@tools/Altinn.Correspondence.DialogActivityExporter/README.md`:
- Around line 208-210: Update the two database doc links in the README so they
point to the renamed files used in this PR: replace references to
`docs/database/DBA_Index_Creation_Scripts.sql` and
`docs/database/DBA_Index_Request_Executive_Summary.md` with the current
filenames `docs/database/Index_Creation_Scripts.sql` and
`docs/database/Index_Request_Executive_Summary.md` respectively; modify the
lines in the README that list these links to avoid 404s.
---
Nitpick comments:
In `@docs/database/Testing_Guide.md`:
- Around line 373-377: The fenced code blocks in Testing_Guide.md (the block
starting with "1. In Visual Studio: Tools → Options → Azure Service
Authentication" and the subsequent block "1. Install Azure Account extension in
VS Code...") are missing language specifiers causing markdownlint failures;
update each triple-backtick fence to include a language specifier (e.g.,
```text) and apply the proposed content change for the second block (replace the
VS Code instructions as shown in the proposed fix) so both blocks are fenced as
```text ... ``` to satisfy the markdownlint "Fenced code blocks should have a
language specified" rule.
In `@tools/Altinn.Correspondence.DialogActivityExporter/test-export.ps1`:
- Around line 98-101: The script sets a Windows-only default path using
"C:\temp" when $OutputPath is empty; replace this with a cross-platform temp
directory by using the platform-agnostic temp path provider (e.g.
[System.IO.Path]::GetTempPath()) and combine it with the generated filename
(which uses $Issue and the timestamp) via Join-Path or equivalent to build
$OutputPath; ensure the filename is sanitized for invalid characters before
joining so variables like $OutputPath, $Issue and the timestamp continue to be
used but without a hardcoded Windows path.
- Line 169: The current Write-Host uses Get-Content $OutputPath | Measure-Object
-Line which loads the whole file into memory; change it to use a streaming line
count or skip counting in test mode: either replace Get-Content with a streaming
API like [System.IO.File]::ReadLines($OutputPath) piped to Measure-Object to
avoid full-file load, or, when MaxBatches/BatchSize indicate test mode, compute
the approximate row count from those variables (e.g., MaxBatches * BatchSize)
and display that instead; update the Write-Host call that references $OutputPath
so it uses the streamed count or the computed test-mode estimate.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f089f403-3c86-4d3a-b05a-081c3c96096f
📒 Files selected for processing (29)
docs/DialogActivityExport_CombinedMode.mddocs/DialogActivityExport_Summary.mddocs/database/Azure_Identity_Migration_Summary.mddocs/database/Check_Disk_Space_And_Table_Stats.sqldocs/database/Configure_PostgreSQL_For_Index_Creation.sqldocs/database/Fix_A2Parties_Recipient_Filter_Index.sqldocs/database/Fix_A2Parties_Recipient_Filter_Schema.sqldocs/database/Index_Creation_Production_Summary.mddocs/database/Index_Creation_Scripts.sqldocs/database/Max_Batches_Feature_Summary.mddocs/database/Performance_Optimization_Summary.sqldocs/database/Query_Logging_Feature_Summary.mddocs/database/Quick_Test_Reference.mddocs/database/README.mddocs/database/Test_Export_Query.sqldocs/database/Testing_Documentation_Summary.mddocs/database/Testing_Guide.mdtools/Altinn.Correspondence.DialogActivityExporter/Altinn.Correspondence.DialogActivityExporter.csprojtools/Altinn.Correspondence.DialogActivityExporter/CHANGELOG.mdtools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cstools/Altinn.Correspondence.DialogActivityExporter/Pre_Calculated_Counts_Feature.mdtools/Altinn.Correspondence.DialogActivityExporter/Program.cstools/Altinn.Correspondence.DialogActivityExporter/README.mdtools/Altinn.Correspondence.DialogActivityExporter/Simplified_Progress_Tracking.mdtools/Altinn.Correspondence.DialogActivityExporter/appsettings.jsontools/Altinn.Correspondence.DialogActivityExporter/appsettings.local.example.jsontools/Altinn.Correspondence.DialogActivityExporter/calculate-counts.sqltools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sqltools/Altinn.Correspondence.DialogActivityExporter/test-export.ps1
✅ Files skipped from review due to trivial changes (7)
- tools/Altinn.Correspondence.DialogActivityExporter/appsettings.local.example.json
- tools/Altinn.Correspondence.DialogActivityExporter/Pre_Calculated_Counts_Feature.md
- docs/database/Quick_Test_Reference.md
- tools/Altinn.Correspondence.DialogActivityExporter/CHANGELOG.md
- docs/database/Testing_Documentation_Summary.md
- docs/DialogActivityExport_Summary.md
- docs/DialogActivityExport_CombinedMode.md
🚧 Files skipped from review as they are similar to previous changes (3)
- tools/Altinn.Correspondence.DialogActivityExporter/appsettings.json
- tools/Altinn.Correspondence.DialogActivityExporter/Altinn.Correspondence.DialogActivityExporter.csproj
- docs/database/README.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs (1)
463-482:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSyntax error: Stray closing brace breaks class structure.
Line 469 contains an extra
}that would close the class prematurely, leavingDialogActivityRecordandExportProgressorphaned. This will cause compilation errors.🐛 Proposed fix
private static string EscapeCSV(string value) { if (string.IsNullOrEmpty(value)) return string.Empty; return value.Replace("\"", "\"\""); } - } // Record to hold query results before CSV export internal record DialogActivityRecord🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs` around lines 463 - 482, The file has a stray closing brace after the EscapeCSV method that prematurely closes the class and leaves DialogActivityRecord (and ExportProgress) orphaned; remove the extra `}` that follows EscapeCSV so the class remains open and DialogActivityRecord and ExportProgress are defined inside the intended class/namespace, ensuring methods like EscapeCSV and the internal record DialogActivityRecord compile together.tools/Altinn.Correspondence.DialogActivityExporter/Program.cs (1)
68-81:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
Console.ReadKey()against redirected stdin.
Console.ReadKey()throwsInvalidOperationExceptionwhen stdin is redirected (e.g., CI pipelines, scripted execution). The tool already has--yesto skip confirmation, but users who forget it will get an unexpected crash.🛡️ Proposed fix
// Confirm if (!options.SkipConfirmation) { + if (Console.IsInputRedirected) + { + logger.LogError("Cannot prompt for confirmation: stdin is redirected. Use --yes to skip confirmation."); + return 1; + } Console.ForegroundColor = ConsoleColor.Yellow; Console.Write("Continue? (y/n): "); Console.ResetColor(); var key = Console.ReadKey();🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs` around lines 68 - 81, Guard the interactive confirmation block around options.SkipConfirmation by checking Console.IsInputRedirected (or by wrapping Console.ReadKey in a try/catch for InvalidOperationException) so the app doesn't throw when stdin is redirected; if input is redirected, log a clear message via logger.LogInformation/LogWarning that interactive confirmation is unavailable and instruct the user to use --yes (or cancel) and then exit with non-zero (or return 0 if you prefer cancellation behavior). Modify the existing block where Console.ReadKey() is called (the confirmation before proceeding) to perform the redirect check or exception handling and handle the flow accordingly.
♻️ Duplicate comments (4)
tools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sql (1)
48-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd Status 6 variants before using this script’s output for join decisions.
All checks currently exercise only
stats."Status" = 4, but the decision matrix/action items are framed for exporter behavior more generally. Add mirrored Status 6 runs (withidc... "StatusAction" = '6') or parameterize status/action pairs; otherwise recommendations (including downstream C# null-handling guidance) can be misleading.Also applies to: 70-71, 115-116, 147-148, 176-177, 218-219
🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sql` around lines 48 - 49, The query filters only for stats."Status" = 4; add equivalent variants for Status = 6 (and where applicable idc... "StatusAction" = '6') or parameterize the status/action pair so the script produces mirrored outputs for both statuses before using results for join decisions; update the WHERE clauses referenced (e.g., stats."Status" = 4 at lines with date ranges) and any idc..."StatusAction" checks so they either run twice (one with 4 and one with 6) or accept a status/action parameter to generate consistent outputs for both cases.docs/database/Test_Export_Query.sql (1)
61-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse one shared cursor tuple shape in all cursor examples.
The commented cursor predicates still hardcode branch-specific statuses (
..., 4/..., 6). For merged keyset batching ordered by(CorrespondenceId, Status), examples should use the same last-row tuple shape (lastCorrespondenceId,lastStatus) in every query block to avoid boundary skips.♻️ Minimal doc fix
- -- AND (stats."CorrespondenceId", stats."Status") > ('last-uuid'::uuid, 4) + -- AND (stats."CorrespondenceId", stats."Status") > ('last-correspondence-id'::uuid, <last-status>) - -- AND (stats."CorrespondenceId", stats."Status") > ('last-uuid'::uuid, 6) + -- AND (stats."CorrespondenceId", stats."Status") > ('last-correspondence-id'::uuid, <last-status>)Also applies to: 93-93, 139-139, 171-171
🤖 Prompt for 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. In `@docs/database/Test_Export_Query.sql` at line 61, Replace the branch-specific hardcoded tuple in the commented cursor predicates with a shared last-row tuple shape that matches the ORDER BY (CorrespondenceId, Status): use a placeholder pair like (lastCorrespondenceId, lastStatus) (casting lastCorrespondenceId to uuid where needed) instead of ('last-uuid'::uuid, 4) or other numeric literals; update every commented predicate that compares (stats."CorrespondenceId", stats."Status") to use the same (lastCorrespondenceId, lastStatus) naming so all cursor examples use a consistent keyset tuple shape matching the ORDER BY.docs/database/Performance_Optimization_Summary.sql (1)
59-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign the “production query” section with the actual exporter query shape.
This section documents
UNION ALL+LEFT JOIN IdempotencyKeys, while the export path in this PR cohort is separate status queries merged in app code. Keeping a different shape here makes EXPLAIN/timing guidance non-representative and can lead to wrong optimization decisions.🤖 Prompt for 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. In `@docs/database/Performance_Optimization_Summary.sql` around lines 59 - 124, The documented "production query" uses a UNION ALL with LEFT JOIN on IdempotencyKeys (queries against correspondence."CorrespondenceStatuses", correspondence."Correspondences", correspondence."ExternalReferences", and correspondence."A2Parties"), but the exporter in this PR runs separate status-specific queries merged in app code; update the docs to reflect that actual exporter shape: replace the single UNION ALL example with two separate status-specific queries (one for Status=4 and one for Status=6) without UNION, each joining IdempotencyKeys only as used by the exporter, and add a note that pagination is performed in application code (cursor logic similar to WHERE (stats."CorrespondenceId", stats."Status") > (`@lastCorrespondenceId`, `@lastStatus`)) so EXPLAIN/timing guidance matches the real execution path.tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs (1)
143-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet combined
totalCountto 0 unless both per-issue counts are available.The fix from the past review only updated logging but not the actual
totalCountcalculation. Ifcount1716=1000000andcount1951=0, thentotalCount=1000000. After exporting all of issue 1716, progress shows 100%. When issue 1951 records are exported,TotalProcessedexceedsTotalCount, causing percentage >100% and negative ETA.🐛 Proposed fix
// Use pre-calculated counts if available (for progress percentage) long count1716 = preCalculatedCount1716; long count1951 = preCalculatedCount1951; - long totalCount = count1716 + count1951; // Only consider total known if both individual counts are available - if (count1716 > 0 && count1951 > 0) + long totalCount = (count1716 > 0 && count1951 > 0) + ? count1716 + count1951 + : 0; + + if (totalCount > 0) { _logger.LogInformation("Expected records (from pre-calculated counts): ~{Count:N0} total (1716: {Count1716:N0}, 1951: {Count1951:N0})", totalCount, count1716, count1951);🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs` around lines 143 - 156, The current logic calculates totalCount = count1716 + count1951 even when one of the per-issue counts is missing, causing TotalProcessed to exceed TotalCount; update the block around count1716, count1951 and totalCount so that totalCount is set to 0 unless both count1716 > 0 and count1951 > 0 (i.e., only compute totalCount = count1716 + count1951 inside the if that checks both are > 0), and keep the existing logging paths (_logger.LogInformation) consistent with this change so progress/ETA use the corrected totalCount.
🧹 Nitpick comments (1)
tools/Altinn.Correspondence.DialogActivityExporter/Program.cs (1)
254-264: 💤 Low valueRemove dead code for deprecated
--oldestparameter.The
oldestDatevariable is parsed but never used after the deprecation warning. Consider removing the parameter handling entirely to avoid misleading users, or keep it as a no-op for backwards compatibility but simplify the code.♻️ Option: Simplify to warning-only
- DateTime? oldestDate = null; if (!string.IsNullOrEmpty(oldest)) { - if (!DateTime.TryParse(oldest, out var parsedOldest)) - { - logger.LogError("Invalid oldest date format. Use 'yyyy-MM-dd' or 'yyyy-MM-dd HH:mm:ss'"); - return null; - } - oldestDate = parsedOldest; logger.LogWarning("WARNING: --oldest parameter is deprecated and no longer used (performance optimization)"); }🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs` around lines 254 - 264, Code parses and assigns oldestDate from the deprecated --oldest parameter (variables/methods: oldest, oldestDate, DateTime.TryParse, logger.LogWarning, logger.LogError) but never uses it; remove the dead parsing logic and related variable to avoid misleading behavior. Replace the block that attempts to parse oldest and set oldestDate with a single no-op warning: remove declaration of oldestDate and the DateTime.TryParse branch, keep only logger.LogWarning("WARNING: --oldest parameter is deprecated and no longer used (performance optimization)") (or remove the warning entirely if you prefer complete removal of the parameter handling). Ensure no other code references oldestDate after this change and remove any now-unused usings or variables introduced solely for this parsing.
🤖 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 `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs`:
- Around line 93-141: The Progress<ExportProgress> callback computes rate =
p.TotalProcessed / p.ElapsedTime.TotalSeconds (in the delegate assigned to
variable progress) without guarding against zero/near-zero elapsed seconds;
update both occurrences (the rate calculations at the branches using
p.ElapsedTime.TotalSeconds) to check for p.ElapsedTime.TotalSeconds > 0 (or use
Math.Max(p.ElapsedTime.TotalSeconds, 1e-6)) and set rate to 0 (or the safe
computed value) when elapsed time is zero to avoid Infinity/NaN.
---
Outside diff comments:
In
`@tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs`:
- Around line 463-482: The file has a stray closing brace after the EscapeCSV
method that prematurely closes the class and leaves DialogActivityRecord (and
ExportProgress) orphaned; remove the extra `}` that follows EscapeCSV so the
class remains open and DialogActivityRecord and ExportProgress are defined
inside the intended class/namespace, ensuring methods like EscapeCSV and the
internal record DialogActivityRecord compile together.
In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs`:
- Around line 68-81: Guard the interactive confirmation block around
options.SkipConfirmation by checking Console.IsInputRedirected (or by wrapping
Console.ReadKey in a try/catch for InvalidOperationException) so the app doesn't
throw when stdin is redirected; if input is redirected, log a clear message via
logger.LogInformation/LogWarning that interactive confirmation is unavailable
and instruct the user to use --yes (or cancel) and then exit with non-zero (or
return 0 if you prefer cancellation behavior). Modify the existing block where
Console.ReadKey() is called (the confirmation before proceeding) to perform the
redirect check or exception handling and handle the flow accordingly.
---
Duplicate comments:
In `@docs/database/Performance_Optimization_Summary.sql`:
- Around line 59-124: The documented "production query" uses a UNION ALL with
LEFT JOIN on IdempotencyKeys (queries against
correspondence."CorrespondenceStatuses", correspondence."Correspondences",
correspondence."ExternalReferences", and correspondence."A2Parties"), but the
exporter in this PR runs separate status-specific queries merged in app code;
update the docs to reflect that actual exporter shape: replace the single UNION
ALL example with two separate status-specific queries (one for Status=4 and one
for Status=6) without UNION, each joining IdempotencyKeys only as used by the
exporter, and add a note that pagination is performed in application code
(cursor logic similar to WHERE (stats."CorrespondenceId", stats."Status") >
(`@lastCorrespondenceId`, `@lastStatus`)) so EXPLAIN/timing guidance matches the
real execution path.
In `@docs/database/Test_Export_Query.sql`:
- Line 61: Replace the branch-specific hardcoded tuple in the commented cursor
predicates with a shared last-row tuple shape that matches the ORDER BY
(CorrespondenceId, Status): use a placeholder pair like (lastCorrespondenceId,
lastStatus) (casting lastCorrespondenceId to uuid where needed) instead of
('last-uuid'::uuid, 4) or other numeric literals; update every commented
predicate that compares (stats."CorrespondenceId", stats."Status") to use the
same (lastCorrespondenceId, lastStatus) naming so all cursor examples use a
consistent keyset tuple shape matching the ORDER BY.
In
`@tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs`:
- Around line 143-156: The current logic calculates totalCount = count1716 +
count1951 even when one of the per-issue counts is missing, causing
TotalProcessed to exceed TotalCount; update the block around count1716,
count1951 and totalCount so that totalCount is set to 0 unless both count1716 >
0 and count1951 > 0 (i.e., only compute totalCount = count1716 + count1951
inside the if that checks both are > 0), and keep the existing logging paths
(_logger.LogInformation) consistent with this change so progress/ETA use the
corrected totalCount.
In
`@tools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sql`:
- Around line 48-49: The query filters only for stats."Status" = 4; add
equivalent variants for Status = 6 (and where applicable idc... "StatusAction" =
'6') or parameterize the status/action pair so the script produces mirrored
outputs for both statuses before using results for join decisions; update the
WHERE clauses referenced (e.g., stats."Status" = 4 at lines with date ranges)
and any idc..."StatusAction" checks so they either run twice (one with 4 and one
with 6) or accept a status/action parameter to generate consistent outputs for
both cases.
---
Nitpick comments:
In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs`:
- Around line 254-264: Code parses and assigns oldestDate from the deprecated
--oldest parameter (variables/methods: oldest, oldestDate, DateTime.TryParse,
logger.LogWarning, logger.LogError) but never uses it; remove the dead parsing
logic and related variable to avoid misleading behavior. Replace the block that
attempts to parse oldest and set oldestDate with a single no-op warning: remove
declaration of oldestDate and the DateTime.TryParse branch, keep only
logger.LogWarning("WARNING: --oldest parameter is deprecated and no longer used
(performance optimization)") (or remove the warning entirely if you prefer
complete removal of the parameter handling). Ensure no other code references
oldestDate after this change and remove any now-unused usings or variables
introduced solely for this parsing.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c8c7ea17-81fd-40dc-a0c2-33ebf211562b
📒 Files selected for processing (29)
docs/DialogActivityExport_CombinedMode.mddocs/DialogActivityExport_Summary.mddocs/database/Azure_Identity_Migration_Summary.mddocs/database/Check_Disk_Space_And_Table_Stats.sqldocs/database/Configure_PostgreSQL_For_Index_Creation.sqldocs/database/Fix_A2Parties_Recipient_Filter_Index.sqldocs/database/Fix_A2Parties_Recipient_Filter_Schema.sqldocs/database/Index_Creation_Production_Summary.mddocs/database/Index_Creation_Scripts.sqldocs/database/Max_Batches_Feature_Summary.mddocs/database/Performance_Optimization_Summary.sqldocs/database/Query_Logging_Feature_Summary.mddocs/database/Quick_Test_Reference.mddocs/database/README.mddocs/database/Test_Export_Query.sqldocs/database/Testing_Documentation_Summary.mddocs/database/Testing_Guide.mdtools/Altinn.Correspondence.DialogActivityExporter/Altinn.Correspondence.DialogActivityExporter.csprojtools/Altinn.Correspondence.DialogActivityExporter/CHANGELOG.mdtools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cstools/Altinn.Correspondence.DialogActivityExporter/Pre_Calculated_Counts_Feature.mdtools/Altinn.Correspondence.DialogActivityExporter/Program.cstools/Altinn.Correspondence.DialogActivityExporter/README.mdtools/Altinn.Correspondence.DialogActivityExporter/Simplified_Progress_Tracking.mdtools/Altinn.Correspondence.DialogActivityExporter/appsettings.jsontools/Altinn.Correspondence.DialogActivityExporter/appsettings.local.example.jsontools/Altinn.Correspondence.DialogActivityExporter/calculate-counts.sqltools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sqltools/Altinn.Correspondence.DialogActivityExporter/test-export.ps1
✅ Files skipped from review due to trivial changes (19)
- tools/Altinn.Correspondence.DialogActivityExporter/appsettings.local.example.json
- docs/database/Check_Disk_Space_And_Table_Stats.sql
- docs/database/Azure_Identity_Migration_Summary.md
- tools/Altinn.Correspondence.DialogActivityExporter/calculate-counts.sql
- tools/Altinn.Correspondence.DialogActivityExporter/Pre_Calculated_Counts_Feature.md
- docs/database/Query_Logging_Feature_Summary.md
- docs/database/Testing_Documentation_Summary.md
- tools/Altinn.Correspondence.DialogActivityExporter/CHANGELOG.md
- tools/Altinn.Correspondence.DialogActivityExporter/appsettings.json
- docs/DialogActivityExport_CombinedMode.md
- docs/database/Quick_Test_Reference.md
- docs/database/Testing_Guide.md
- docs/DialogActivityExport_Summary.md
- tools/Altinn.Correspondence.DialogActivityExporter/Simplified_Progress_Tracking.md
- tools/Altinn.Correspondence.DialogActivityExporter/Altinn.Correspondence.DialogActivityExporter.csproj
- docs/database/README.md
- docs/database/Max_Batches_Feature_Summary.md
- docs/database/Index_Creation_Production_Summary.md
- tools/Altinn.Correspondence.DialogActivityExporter/README.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tools/Altinn.Correspondence.DialogActivityExporter/README.md (1)
16-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAuthentication section is outdated vs current Azure.Identity flow.
The README still states Azure CLI is required and describes token retrieval via
az account get-access-token, which conflicts with the currentDefaultAzureCredential-based authentication model documented elsewhere in this PR context. Please update prerequisites/how-it-works text so operators don’t configure the wrong auth path.Also applies to: 38-39
🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/README.md` at line 16, Update the README authentication text to remove the outdated Azure CLI/token instructions and instead document the current DefaultAzureCredential flow: replace references to "Azure CLI installed and logged in (`az account get-access-token`)" with guidance on using DefaultAzureCredential (developer sign-in, environment variables, or Managed Identity), list the required environment variables or role assignments for Managed Identity, and show how to test locally (e.g., VS Code/az login fallback) and in Azure (system-assigned/user-assigned MI). Make this change for both occurrences of the Azure AD auth guidance (the earlier Azure CLI mention and the duplicate at lines noted in the review) and remove any example commands that instruct obtaining tokens via az.tools/Altinn.Correspondence.DialogActivityExporter/Program.cs (3)
9-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLoad
appsettings.local.jsonin the configuration chain.The PR adds a local-example config file, but this builder never loads the matching override file. As-is, copying
appsettings.local.example.jsontoappsettings.local.jsonwill not affect the tool, so the local-config workflow in this PR is broken.💡 Proposed fix
var config = new ConfigurationBuilder() .SetBasePath(Directory.GetCurrentDirectory()) .AddJsonFile("appsettings.json", optional: true) + .AddJsonFile("appsettings.local.json", optional: true) .AddEnvironmentVariables() .Build();🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs` around lines 9 - 13, The configuration builder in Program.cs never loads appsettings.local.json so local overrides are ignored; update the ConfigurationBuilder sequence (the block creating var config) to include AddJsonFile("appsettings.local.json", optional: true) after AddJsonFile("appsettings.json") so the local file overrides values from appsettings.json; keep AddEnvironmentVariables() and Build() as-is to preserve env var overrides.
255-258:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winParse
--cutoffwith an exact invariant format.
DateTime.TryParsemakes acceptance depend on the machine locale even though the CLI contract and help text document a singleyyyy-MM-dd HH:mm:ssformat.TryParseExactkeeps validation deterministic.💡 Proposed fix
- if (!DateTime.TryParse(cutoff, out var cutoffDate)) + if (!DateTime.TryParseExact( + cutoff, + "yyyy-MM-dd HH:mm:ss", + System.Globalization.CultureInfo.InvariantCulture, + System.Globalization.DateTimeStyles.AssumeLocal, + out var cutoffDate)) { logger.LogError("Invalid cutoff date format. Use 'yyyy-MM-dd HH:mm:ss'"); return null; }🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs` around lines 255 - 258, The cutoff parsing uses DateTime.TryParse which is locale-dependent; change the code that parses the cutoff variable (in Program.cs where cutoff is parsed into cutoffDate) to use DateTime.TryParseExact with the format "yyyy-MM-dd HH:mm:ss", specify CultureInfo.InvariantCulture and appropriate DateTimeStyles (e.g., DateTimeStyles.None), and keep the same error logging and null return path; reference the cutoff variable and cutoffDate local and the surrounding parsing block so you update the TryParse -> TryParseExact call and import/use System.Globalization.CultureInfo.InvariantCulture.
418-427:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix Entra/AD PostgreSQL “Username” construction for
--azure-adFile:
tools/Altinn.Correspondence.DialogActivityExporter/Program.cs(lines 418-427)
--azure-adbuilds the connection string usingEnvironment.UserNameplus a hardcoded@ai-dev.nosuffix (Username={username}@ai-dev.no``). For Azure Database for PostgreSQL Entra token auth, the connection stringUsernamemust match the exact Entra identity/database role you created (UPN for Entra users; managed identity/service principal name when using those identities). Fabricating it from the local Windows username breaks for different UPN suffixes and for non-user credentials that `DefaultAzureCredential` may return (managed identity/service principal). Make the database username explicit/configurable (or derive it via the official PostgreSQL Entra auth helper) instead of constructing it here.
https://learn.microsoft.com/en-us/azure/postgresql/security/security-entra-configure
https://learn.microsoft.com/en-us/azure/postgresql/security/security-connect-with-managed-identity🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs` around lines 418 - 427, The code currently builds the PostgreSQL connection Username by combining Environment.UserName with a hardcoded "`@ai-dev.no`" (variable: username -> used in connectionString), which is incorrect for Azure/Entra auth; change this to accept an explicit, configurable DB principal (e.g., --azure-ad-db-username or config key) and use that value when building connectionString for the --azure-ad flow (instead of fabricating UPN), or detect and use the correct identity type returned by your credential flow (DefaultAzureCredential / managed identity / service principal) and map it to the appropriate DB role; update the logic that constructs connectionString to reference the new configurable db username and ensure callers set the correct UPN or managed identity name rather than relying on Environment.UserName.
♻️ Duplicate comments (11)
docs/database/Testing_Guide.md (2)
150-150:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
--oldestexamples are stale and should be removed or explicitly marked ignored.These examples imply
--oldestis an active filter, which is no longer true in the current exporter flow; this can mislead test validation.Also applies to: 199-199, 427-427
🤖 Prompt for 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. In `@docs/database/Testing_Guide.md` at line 150, The `--oldest` examples in the Testing_Guide.md are stale and misleading; locate occurrences of the flag (e.g., the example line with `--oldest "2019-03-23"`) and either remove those example lines or precede them with an explicit ignore/Deprecated note stating they are no longer applicable to the current exporter flow; ensure you update all referenced locations (the lines around the shown diff and the other instances noted at lines ~199 and ~427) so the guide no longer suggests `--oldest` is an active filter.
137-137:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFeature claim doesn’t match
test-export.ps1behavior.The guide says the script offers to open the CSV automatically, but the script only prints output file stats/path.
🤖 Prompt for 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. In `@docs/database/Testing_Guide.md` at line 137, The docs claim that the script "offers to open CSV file after completion" but test-export.ps1 does not; either update docs or change the script: either edit docs/Testing_Guide.md to remove or reword the checklist item to accurately state that test-export.ps1 only prints the output path and stats, or modify test-export.ps1 to prompt the user and open the CSV (e.g., add an interactive prompt and call Start-Process or equivalent) so the behavior matches the guide; reference test-export.ps1 when making the change.docs/database/Max_Batches_Feature_Summary.md (1)
41-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis summary still documents obsolete API/CLI behavior.
References to
skipTotalCount,oldestCorrespondenceDate, and--oldestare stale and conflict with the current exporter flow. Please refresh these sections so this feature summary matches actual implementation.Also applies to: 92-93, 307-323
🤖 Prompt for 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. In `@docs/database/Max_Batches_Feature_Summary.md` around lines 41 - 47, The doc section references obsolete symbols and CLI flags (skipTotalCount, oldestCorrespondenceDate, --oldest and the options.MaxBatches usage) that no longer match the exporter implementation; update the prose and examples to remove those stale names and instead describe the current behavior and API/CLI names used by the exporter (replace mentions of skipTotalCount/options.MaxBatches with the real MaxBatches handling in the current exporter, and replace oldestCorrespondenceDate/--oldest with the exporter’s current date-selection/config option or flag), ensuring code examples and summary text use the actual current identifiers and control flow of the exporter.tools/Altinn.Correspondence.DialogActivityExporter/README.md (1)
208-210:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDatabase documentation links still point to stale
DBA_*filenames.These paths should be aligned with the actual docs introduced in this PR (e.g.,
Index_Creation_Scripts.sql, related current markdown files), otherwise readers hit broken links.🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/README.md` around lines 208 - 210, Update the README links that reference stale DBA_* filenames: replace the occurrences of "docs/database/DBA_Index_Creation_Scripts.sql" and "docs/database/DBA_Index_Request_Executive_Summary.md" with the new filenames introduced in this PR (e.g., "docs/database/Index_Creation_Scripts.sql" and the corresponding current markdown name for the executive summary), and verify the link targets match the actual files added in this change so readers don't hit broken links.docs/database/Index_Creation_Production_Summary.md (1)
92-107:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFence the dead-tuple SQL block as
sql.This snippet is still malformed as markdown code fencing, so rendering/linting remains broken.
🤖 Prompt for 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. In `@docs/database/Index_Creation_Production_Summary.md` around lines 92 - 107, The SQL snippet for checking dead tuples and running VACUUM is not fenced as SQL code and breaks Markdown rendering; update the block that starts with the comment "-- Check dead tuples and bloat:" and ends after "VACUUM ANALYZE correspondence.\"CorrespondenceStatuses\";" by wrapping it with proper Markdown fences (use ```sql before the SELECT and ``` after the VACUUM line) so the entire SELECT ... FROM pg_stat_user_tables ... AND tablename = 'CorrespondenceStatuses'; and the VACUUM ANALYZE statement are inside a single ```sql fenced code block.docs/database/Index_Creation_Scripts.sql (1)
25-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale A2Parties script references.
Line 25 and Line 386 still point to old filenames, which can send operators to the wrong scripts during maintenance.
Also applies to: 384-386
🤖 Prompt for 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. In `@docs/database/Index_Creation_Scripts.sql` around lines 25 - 26, Update the stale comment references that point to the old A2Parties script and the obsolete migration filter: search for the comment lines mentioning "Fix_A2Parties_Indexes.sql" and "IX_Correspondences_Id_Created_MigrationFilter" and replace them with the current, correct script names or remove the misleading references; ensure any note text (e.g., the A2Parties index optimization comment and the Phase 3 migration-filter note) accurately points to the existing script filenames or tickets used in the repo so operators are not directed to outdated files.docs/database/Fix_A2Parties_Recipient_Filter_Schema.sql (1)
149-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact the sample
RecipientUrnvalues before logging.This block still writes full self-identified, person, and organization URNs to migration logs. The verification only needs to show that each category is present, not the raw identifiers.
Suggested hardening
- v_selfidentified_sample TEXT; - v_person_sample TEXT; - v_org_sample TEXT; + v_has_selfidentified BOOLEAN; + v_has_person BOOLEAN; + v_has_org BOOLEAN; @@ - SELECT "RecipientUrn" INTO v_selfidentified_sample - FROM correspondence."A2Parties" - WHERE "OutputActorId" LIKE 'urn:altinn:person:legacy-selfidentified:%' - LIMIT 1; - - SELECT "RecipientUrn" INTO v_person_sample - FROM correspondence."A2Parties" - WHERE "OutputActorId" LIKE 'urn:altinn:person:identifier-no:%' - LIMIT 1; - - SELECT "RecipientUrn" INTO v_org_sample - FROM correspondence."A2Parties" - WHERE "OutputActorId" LIKE 'urn:altinn:organization:identifier-no:%' - LIMIT 1; + SELECT EXISTS( + SELECT 1 + FROM correspondence."A2Parties" + WHERE "OutputActorId" LIKE 'urn:altinn:person:legacy-selfidentified:%' + ) INTO v_has_selfidentified; + + SELECT EXISTS( + SELECT 1 + FROM correspondence."A2Parties" + WHERE "OutputActorId" LIKE 'urn:altinn:person:identifier-no:%' + ) INTO v_has_person; + + SELECT EXISTS( + SELECT 1 + FROM correspondence."A2Parties" + WHERE "OutputActorId" LIKE 'urn:altinn:organization:identifier-no:%' + ) INTO v_has_org; @@ - IF v_selfidentified_sample IS NOT NULL THEN - RAISE NOTICE ' Self-identified: %', v_selfidentified_sample; + IF v_has_selfidentified THEN + RAISE NOTICE ' Self-identified: [present]'; END IF; - IF v_person_sample IS NOT NULL THEN - RAISE NOTICE ' Person: %', v_person_sample; + IF v_has_person THEN + RAISE NOTICE ' Person: [present]'; END IF; - IF v_org_sample IS NOT NULL THEN - RAISE NOTICE ' Organization: %', v_org_sample; + IF v_has_org THEN + RAISE NOTICE ' Organization: [present]'; END IF;🤖 Prompt for 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. In `@docs/database/Fix_A2Parties_Recipient_Filter_Schema.sql` around lines 149 - 175, The sample RecipientUrn values (v_selfidentified_sample, v_person_sample, v_org_sample) are logged in full; change the code so you do not output raw identifiers—create masked versions (e.g., using regexp_replace/substring to remove or replace the sensitive part after the last colon or show only the URN prefix/type) and use those masked variables in the RAISE NOTICE calls for 'Self-identified', 'Person', and 'Organization' so the migration verifies presence of each category without exposing full identifiers.docs/database/Performance_Optimization_Summary.sql (2)
49-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t describe cursor pagination as a replacement for
ORDER BY.In
FetchStatusRecordsAsync, the cursor predicate is paired withORDER BY stats."CorrespondenceId", stats."Status"beforeLIMIT. Dropping theORDER BYfrom the per-status query makes the batch contents arbitrary, which breaks deterministic keyset pagination. Reword this to distinguish “noORDER BYafterUNION ALL” from “noORDER BYat all.”Also applies to: 120-123, 159-161
🤖 Prompt for 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. In `@docs/database/Performance_Optimization_Summary.sql` around lines 49 - 52, The note incorrectly implies removing ORDER BY entirely as a replacement for cursor/keyset pagination; update the wording to clarify that only removing ORDER BY after a UNION ALL (which forces a global sort) is recommended for performance, while preserving the per-query ORDER BY used for deterministic keyset pagination (specifically the ORDER BY stats."CorrespondenceId", stats."Status" used by FetchStatusRecordsAsync before LIMIT). Change the phrasing to distinguish "no ORDER BY after UNION ALL" from "don't remove per-status ORDER BY that enforces keyset/cursor ordering" and apply the same clarification to the other referenced sections (the passages around lines 120-123 and 159-161).
59-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the “production” SQL aligned with the exporter.
DialogActivityExportServicecurrently runs two separate status queries,INNER JOINscorrespondence."IdempotencyKeys", and orders each query beforeLIMIT(tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs:278-305and:330-430). ThisUNION ALL+LEFT JOINexample has a different plan shape and can return different rows, so the EXPLAIN/timing guidance here is no longer representative of the real export path.🤖 Prompt for 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. In `@docs/database/Performance_Optimization_Summary.sql` around lines 59 - 123, The documentation's "production" SQL must match the exporter behavior: change the example back to the two separate queries used by DialogActivityExportService (the two status queries it runs), use INNER JOIN to correspondence."IdempotencyKeys" (not LEFT JOIN) and keep the same ORDER BY + LIMIT/pagination pattern (cursor pagination predicate or explicit ORDER BY before LIMIT) so the query plan and returned rows match the real export path implemented in DialogActivityExportService (the queries around the methods shown at tools/Altinn.Correspondence.DialogActivity.DialogActivityExportService.cs:278-305 and :330-430).docs/database/Test_Export_Query.sql (1)
60-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the merged batch cursor tuple in these examples.
DialogActivityExportService.FetchStatusRecordsAsyncapplies the same(@lastid,@lastStatus)tuple to both status queries before the in-memory merge. Hardcoding(..., 4)in the status-4 example and(..., 6)in the status-6 example makes the manual docs skip the(lastId, 6)boundary row when the previous merged batch ended on(lastId, 4)).Also applies to: 92-93, 138-139, 170-171
🤖 Prompt for 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. In `@docs/database/Test_Export_Query.sql` around lines 60 - 61, The SQL examples use hardcoded status values in the cursor tuple which mismatches the merged-batch behavior of DialogActivityExportService.FetchStatusRecordsAsync; update each commented cursor line (e.g., the lines showing AND (stats."CorrespondenceId", stats."Status") > ...) to use the merged batch cursor tuple variables instead of fixed numbers—apply the same (`@lastId`, `@lastStatus`) tuple used by FetchStatusRecordsAsync so the docs reflect the merged boundary handling (update all occurrences noted in the comment: the examples around the status-4/status-6 lines and the other locations described).tools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sql (1)
24-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStatus 6 is still unverified, but the conclusions are written as if both statuses were tested.
Every probe in this file is pinned to
stats."Status" = 4, while both the exporter andcalculate-counts.sqloperate on Status 4 and Status 6. Using these results to justify droppingIdempotencyKeysfrom both pre-calculated totals leaves the Status 6 path unvalidated.Also applies to: 91-149, 160-219, 223-275
🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sql` around lines 24 - 76, The probes only test stats."Status" = 4 but the exporter and calculate-counts.sql use Status 4 and 6, so update the EXISTS checks to validate the Status 6 path as well (either change WHERE stats."Status" = 4 to WHERE stats."Status" IN (4,6) or add a parallel check with stats."Status" = 6) for the IdempotencyKeys and ExternalReferences probes; apply the same update to the other probe blocks referenced (lines ~91-149, ~160-219, ~223-275) so both statuses are verified before removing joins or precomputed totals, using the same table/column identifiers shown (correspondence."CorrespondenceStatuses" stats, correspondence."IdempotencyKeys" idcFetch, correspondence."ExternalReferences" er, etc.).
🤖 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 `@docs/database/Check_Disk_Space_And_Table_Stats.sql`:
- Around line 82-83: Update the timing recommendations in the two comment lines
that read "Index `#1` (Issue `#1716`): 1-2 hours" and "Index `#2` (Issue `#1951`): 6-10
hours" to match the recorded production results reported in this PR
(approximately 2h 15m for Index `#1` and ~3h 9m for Index `#2`), or replace them
with a single consistent baseline (e.g., "≈2h 15m" and "≈3h 9m" or rounded
values like "≈2h 15m" and "≈3h") so the doc and production summary are aligned;
modify the literal text in the two comment lines accordingly.
In `@docs/database/Configure_PostgreSQL_For_Index_Creation.sql`:
- Around line 171-173: The guidance incorrectly suggests using SELECT
pg_size_pretty(pg_total_relation_size('pg_class')) to check "free RAM" before
raising maintenance_work_mem to 4GB; replace that line so it either (A) directs
operators to check actual instance memory/capacity (e.g., VM/DB instance total
RAM and current free/headroom via OS tools like free -m / vmstat or cloud
provider instance metrics) and expected concurrent maintenance jobs, or (B) at
minimum reword the SQL note to clarify that
pg_size_pretty(pg_total_relation_size(...)) reports on-disk relation size not
RAM. Update the runbook text referencing maintenance_work_mem and remove or
rephrase the example SQL to reference appropriate checks (OS/cloud metrics or
instance memory) rather than pg_total_relation_size.
In `@docs/database/Fix_A2Parties_Recipient_Filter_Index.sql`:
- Around line 49-58: The DO $$ block currently uses
pg_current_xact_id_if_assigned() to detect a transaction, which can be NULL even
inside an explicit BEGIN and thus miss cases where CREATE INDEX CONCURRENTLY
will fail; update the guard in the DO $$ block (or remove the IF ...
pg_current_xact_id_if_assigned() check) and instead emit an unconditional
warning/reworded message that reminds users to run CREATE INDEX CONCURRENTLY in
autocommit mode / not inside a transaction block (reference the DO $$ block and
pg_current_xact_id_if_assigned() and the CREATE INDEX CONCURRENTLY note).
In `@docs/database/Index_Creation_Production_Summary.md`:
- Around line 277-279: Update the final summary line for "Index `#2`" so its
status matches the rest of the document: replace "Index `#2`: ⏳ Estimated 8-12
hours" with a completed status line like "Index `#2`: ✅ Complete in <actual
duration>" using the actual duration recorded earlier in the document for Index
`#2`; ensure the phrasing matches the existing "Index `#1`: ✅ Complete in 2h 15m"
style.
In `@docs/database/Index_Creation_Scripts.sql`:
- Around line 380-385: The runbook shows inconsistent total index space:
individual index estimates for
IX_CorrespondenceStatuses_Status_SyncedTimestamp_Synced and
IX_CorrespondenceStatuses_Status_StatusChanged_Migrated (and
IX_Correspondences_Id_Created_MigrationFilter) sum to ~27 GB per the file’s
production results, but the TOTAL NEW INDEX SPACE comment currently states ~15
GB; update the TOTAL NEW INDEX SPACE comment to reflect the correct summed value
(or recalc and replace the per-index estimates if those are wrong) so the values
are consistent, and ensure the three index comment lines
(IX_CorrespondenceStatuses_Status_SyncedTimestamp_Synced,
IX_CorrespondenceStatuses_Status_StatusChanged_Migrated,
IX_Correspondences_Id_Created_MigrationFilter) and the TOTAL NEW INDEX SPACE
line are aligned.
In `@docs/database/Max_Batches_Feature_Summary.md`:
- Around line 134-139: The docs PowerShell snippet uses the old reserved
variable $args but the current script uses $commandArgs; update the snippet to
build and pass parameters into $commandArgs (e.g., replace occurrences of $args
= @(...) with $commandArgs = @(...) and ensure "--max-batches", $MaxBatches
remains included) so the example matches the live script and avoids
reintroducing the reserved-variable pattern.
- Line 360: The statement "File opens automatically when done." in
Max_Batches_Feature_Summary.md is incorrect; either remove that sentence or
correct it to reflect current behavior (e.g., "You must open the output files
manually") or explicitly document steps to open the exported file, or
alternatively implement auto-open behavior in the referenced test-export.ps1
script; update the text to reference the actual behavior and, if choosing to
change the script, mention test-export.ps1 by name so reviewers can find and
verify the change.
In `@docs/database/Quick_Test_Reference.md`:
- Line 118: Remove the stale --oldest "2019-03-23" flag from the production
command example so the quick reference no longer implies that the --oldest
option still affects filtering; locate the example containing the literal
--oldest token (and the quoted date) and delete that flag and its value from the
command string, adjusting spacing/punctuation around the removed token so the
command remains valid and well-formatted.
- Line 9: Update the Quick Test Reference to remove the incorrect "Auto-opens
CSV file for verification (1000 rows)" claim: change the text to accurately
describe test-export.ps1 behavior (it reports the output path, file size, and
approximate row count) or, if you prefer to keep "auto-open" behavior, modify
test-export.ps1 to launch the CSV after generation (e.g., invoke the system
default CSV opener) and then update the doc accordingly; reference the
test-export.ps1 script and the "Auto-opens CSV file for verification (1000
rows)" line when making the change.
In `@docs/database/Testing_Guide.md`:
- Around line 296-297: The docs use inconsistent ActivityType values:
Testing_Guide.md lists "Read"/"Confirmed" while the README uses
"CorrespondenceOpened"/"CorrespondenceConfirmed"; pick the canonical names (use
"CorrespondenceOpened" for Status 4 and "CorrespondenceConfirmed" for Status 6)
and update the Testing_Guide section text and any examples referencing
ActivityType to match the README contract (also ensure any
status-to-ActivityType mapping comments or tables mention the same symbols).
In `@tools/Altinn.Correspondence.DialogActivityExporter/README.md`:
- Around line 78-80: The fenced code blocks showing example output (e.g., the
block containing "Processed: 1,234,567 | 12,450 rows/sec | Elapsed: 00:01:39")
lack language identifiers; update those fenced blocks in README.md (including
the other occurrences around the indicated ranges) to use a language tag such as
```text so markdownlint MD040 is satisfied and formatting remains consistent.
---
Outside diff comments:
In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs`:
- Around line 9-13: The configuration builder in Program.cs never loads
appsettings.local.json so local overrides are ignored; update the
ConfigurationBuilder sequence (the block creating var config) to include
AddJsonFile("appsettings.local.json", optional: true) after
AddJsonFile("appsettings.json") so the local file overrides values from
appsettings.json; keep AddEnvironmentVariables() and Build() as-is to preserve
env var overrides.
- Around line 255-258: The cutoff parsing uses DateTime.TryParse which is
locale-dependent; change the code that parses the cutoff variable (in Program.cs
where cutoff is parsed into cutoffDate) to use DateTime.TryParseExact with the
format "yyyy-MM-dd HH:mm:ss", specify CultureInfo.InvariantCulture and
appropriate DateTimeStyles (e.g., DateTimeStyles.None), and keep the same error
logging and null return path; reference the cutoff variable and cutoffDate local
and the surrounding parsing block so you update the TryParse -> TryParseExact
call and import/use System.Globalization.CultureInfo.InvariantCulture.
- Around line 418-427: The code currently builds the PostgreSQL connection
Username by combining Environment.UserName with a hardcoded "`@ai-dev.no`"
(variable: username -> used in connectionString), which is incorrect for
Azure/Entra auth; change this to accept an explicit, configurable DB principal
(e.g., --azure-ad-db-username or config key) and use that value when building
connectionString for the --azure-ad flow (instead of fabricating UPN), or detect
and use the correct identity type returned by your credential flow
(DefaultAzureCredential / managed identity / service principal) and map it to
the appropriate DB role; update the logic that constructs connectionString to
reference the new configurable db username and ensure callers set the correct
UPN or managed identity name rather than relying on Environment.UserName.
In `@tools/Altinn.Correspondence.DialogActivityExporter/README.md`:
- Line 16: Update the README authentication text to remove the outdated Azure
CLI/token instructions and instead document the current DefaultAzureCredential
flow: replace references to "Azure CLI installed and logged in (`az account
get-access-token`)" with guidance on using DefaultAzureCredential (developer
sign-in, environment variables, or Managed Identity), list the required
environment variables or role assignments for Managed Identity, and show how to
test locally (e.g., VS Code/az login fallback) and in Azure
(system-assigned/user-assigned MI). Make this change for both occurrences of the
Azure AD auth guidance (the earlier Azure CLI mention and the duplicate at lines
noted in the review) and remove any example commands that instruct obtaining
tokens via az.
---
Duplicate comments:
In `@docs/database/Fix_A2Parties_Recipient_Filter_Schema.sql`:
- Around line 149-175: The sample RecipientUrn values (v_selfidentified_sample,
v_person_sample, v_org_sample) are logged in full; change the code so you do not
output raw identifiers—create masked versions (e.g., using
regexp_replace/substring to remove or replace the sensitive part after the last
colon or show only the URN prefix/type) and use those masked variables in the
RAISE NOTICE calls for 'Self-identified', 'Person', and 'Organization' so the
migration verifies presence of each category without exposing full identifiers.
In `@docs/database/Index_Creation_Production_Summary.md`:
- Around line 92-107: The SQL snippet for checking dead tuples and running
VACUUM is not fenced as SQL code and breaks Markdown rendering; update the block
that starts with the comment "-- Check dead tuples and bloat:" and ends after
"VACUUM ANALYZE correspondence.\"CorrespondenceStatuses\";" by wrapping it with
proper Markdown fences (use ```sql before the SELECT and ``` after the VACUUM
line) so the entire SELECT ... FROM pg_stat_user_tables ... AND tablename =
'CorrespondenceStatuses'; and the VACUUM ANALYZE statement are inside a single
```sql fenced code block.
In `@docs/database/Index_Creation_Scripts.sql`:
- Around line 25-26: Update the stale comment references that point to the old
A2Parties script and the obsolete migration filter: search for the comment lines
mentioning "Fix_A2Parties_Indexes.sql" and
"IX_Correspondences_Id_Created_MigrationFilter" and replace them with the
current, correct script names or remove the misleading references; ensure any
note text (e.g., the A2Parties index optimization comment and the Phase 3
migration-filter note) accurately points to the existing script filenames or
tickets used in the repo so operators are not directed to outdated files.
In `@docs/database/Max_Batches_Feature_Summary.md`:
- Around line 41-47: The doc section references obsolete symbols and CLI flags
(skipTotalCount, oldestCorrespondenceDate, --oldest and the options.MaxBatches
usage) that no longer match the exporter implementation; update the prose and
examples to remove those stale names and instead describe the current behavior
and API/CLI names used by the exporter (replace mentions of
skipTotalCount/options.MaxBatches with the real MaxBatches handling in the
current exporter, and replace oldestCorrespondenceDate/--oldest with the
exporter’s current date-selection/config option or flag), ensuring code examples
and summary text use the actual current identifiers and control flow of the
exporter.
In `@docs/database/Performance_Optimization_Summary.sql`:
- Around line 49-52: The note incorrectly implies removing ORDER BY entirely as
a replacement for cursor/keyset pagination; update the wording to clarify that
only removing ORDER BY after a UNION ALL (which forces a global sort) is
recommended for performance, while preserving the per-query ORDER BY used for
deterministic keyset pagination (specifically the ORDER BY
stats."CorrespondenceId", stats."Status" used by FetchStatusRecordsAsync before
LIMIT). Change the phrasing to distinguish "no ORDER BY after UNION ALL" from
"don't remove per-status ORDER BY that enforces keyset/cursor ordering" and
apply the same clarification to the other referenced sections (the passages
around lines 120-123 and 159-161).
- Around line 59-123: The documentation's "production" SQL must match the
exporter behavior: change the example back to the two separate queries used by
DialogActivityExportService (the two status queries it runs), use INNER JOIN to
correspondence."IdempotencyKeys" (not LEFT JOIN) and keep the same ORDER BY +
LIMIT/pagination pattern (cursor pagination predicate or explicit ORDER BY
before LIMIT) so the query plan and returned rows match the real export path
implemented in DialogActivityExportService (the queries around the methods shown
at
tools/Altinn.Correspondence.DialogActivity.DialogActivityExportService.cs:278-305
and :330-430).
In `@docs/database/Test_Export_Query.sql`:
- Around line 60-61: The SQL examples use hardcoded status values in the cursor
tuple which mismatches the merged-batch behavior of
DialogActivityExportService.FetchStatusRecordsAsync; update each commented
cursor line (e.g., the lines showing AND (stats."CorrespondenceId",
stats."Status") > ...) to use the merged batch cursor tuple variables instead of
fixed numbers—apply the same (`@lastId`, `@lastStatus`) tuple used by
FetchStatusRecordsAsync so the docs reflect the merged boundary handling (update
all occurrences noted in the comment: the examples around the status-4/status-6
lines and the other locations described).
In `@docs/database/Testing_Guide.md`:
- Line 150: The `--oldest` examples in the Testing_Guide.md are stale and
misleading; locate occurrences of the flag (e.g., the example line with
`--oldest "2019-03-23"`) and either remove those example lines or precede them
with an explicit ignore/Deprecated note stating they are no longer applicable to
the current exporter flow; ensure you update all referenced locations (the lines
around the shown diff and the other instances noted at lines ~199 and ~427) so
the guide no longer suggests `--oldest` is an active filter.
- Line 137: The docs claim that the script "offers to open CSV file after
completion" but test-export.ps1 does not; either update docs or change the
script: either edit docs/Testing_Guide.md to remove or reword the checklist item
to accurately state that test-export.ps1 only prints the output path and stats,
or modify test-export.ps1 to prompt the user and open the CSV (e.g., add an
interactive prompt and call Start-Process or equivalent) so the behavior matches
the guide; reference test-export.ps1 when making the change.
In `@tools/Altinn.Correspondence.DialogActivityExporter/README.md`:
- Around line 208-210: Update the README links that reference stale DBA_*
filenames: replace the occurrences of
"docs/database/DBA_Index_Creation_Scripts.sql" and
"docs/database/DBA_Index_Request_Executive_Summary.md" with the new filenames
introduced in this PR (e.g., "docs/database/Index_Creation_Scripts.sql" and the
corresponding current markdown name for the executive summary), and verify the
link targets match the actual files added in this change so readers don't hit
broken links.
In
`@tools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sql`:
- Around line 24-76: The probes only test stats."Status" = 4 but the exporter
and calculate-counts.sql use Status 4 and 6, so update the EXISTS checks to
validate the Status 6 path as well (either change WHERE stats."Status" = 4 to
WHERE stats."Status" IN (4,6) or add a parallel check with stats."Status" = 6)
for the IdempotencyKeys and ExternalReferences probes; apply the same update to
the other probe blocks referenced (lines ~91-149, ~160-219, ~223-275) so both
statuses are verified before removing joins or precomputed totals, using the
same table/column identifiers shown (correspondence."CorrespondenceStatuses"
stats, correspondence."IdempotencyKeys" idcFetch,
correspondence."ExternalReferences" er, etc.).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19cbdcdf-be12-4bcd-a562-9a3b61d45331
📒 Files selected for processing (29)
docs/DialogActivityExport_CombinedMode.mddocs/DialogActivityExport_Summary.mddocs/database/Azure_Identity_Migration_Summary.mddocs/database/Check_Disk_Space_And_Table_Stats.sqldocs/database/Configure_PostgreSQL_For_Index_Creation.sqldocs/database/Fix_A2Parties_Recipient_Filter_Index.sqldocs/database/Fix_A2Parties_Recipient_Filter_Schema.sqldocs/database/Index_Creation_Production_Summary.mddocs/database/Index_Creation_Scripts.sqldocs/database/Max_Batches_Feature_Summary.mddocs/database/Performance_Optimization_Summary.sqldocs/database/Query_Logging_Feature_Summary.mddocs/database/Quick_Test_Reference.mddocs/database/README.mddocs/database/Test_Export_Query.sqldocs/database/Testing_Documentation_Summary.mddocs/database/Testing_Guide.mdtools/Altinn.Correspondence.DialogActivityExporter/Altinn.Correspondence.DialogActivityExporter.csprojtools/Altinn.Correspondence.DialogActivityExporter/CHANGELOG.mdtools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cstools/Altinn.Correspondence.DialogActivityExporter/Pre_Calculated_Counts_Feature.mdtools/Altinn.Correspondence.DialogActivityExporter/Program.cstools/Altinn.Correspondence.DialogActivityExporter/README.mdtools/Altinn.Correspondence.DialogActivityExporter/Simplified_Progress_Tracking.mdtools/Altinn.Correspondence.DialogActivityExporter/appsettings.jsontools/Altinn.Correspondence.DialogActivityExporter/appsettings.local.example.jsontools/Altinn.Correspondence.DialogActivityExporter/calculate-counts.sqltools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sqltools/Altinn.Correspondence.DialogActivityExporter/test-export.ps1
✅ Files skipped from review due to trivial changes (9)
- tools/Altinn.Correspondence.DialogActivityExporter/CHANGELOG.md
- tools/Altinn.Correspondence.DialogActivityExporter/Pre_Calculated_Counts_Feature.md
- docs/DialogActivityExport_Summary.md
- docs/database/Testing_Documentation_Summary.md
- docs/database/Azure_Identity_Migration_Summary.md
- tools/Altinn.Correspondence.DialogActivityExporter/Altinn.Correspondence.DialogActivityExporter.csproj
- docs/database/README.md
- tools/Altinn.Correspondence.DialogActivityExporter/appsettings.local.example.json
- docs/DialogActivityExport_CombinedMode.md
added 2 commits
June 3, 2026 16:16
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/Altinn.Correspondence.DialogActivityExporter/README.md (2)
16-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate Azure AD authentication documentation to match current SDK-based implementation.
The README states Azure CLI is required (line 16) and describes calling
az account get-access-tokenas a subprocess (lines 38-44), butProgram.cs(lines 388-414) now uses theAzure.IdentitySDK'sDefaultAzureCredential, which supports multiple authentication methods (environment variables, managed identity, Visual Studio, VS Code, Azure CLI, Azure PowerShell, etc.) without subprocess calls.Update the documentation to:
- Remove the Azure CLI prerequisite (it's one option, not required)
- Replace the subprocess description with an explanation of
DefaultAzureCredential's credential chain- Mention that Azure CLI (
az login) is one of several supported authentication methods📝 Suggested documentation update
## Prerequisites - .NET 10 SDK - Access to Correspondence production database - Required PostgreSQL indexes (see `docs/database/` folder) -- **For Azure AD auth**: Azure CLI installed and logged in (`az login`) + **For Azure AD auth**: Azure credentials (Azure CLI, Visual Studio, VS Code, or other Azure Identity methods)**How it works:** - Detects your Windows username (e.g., `rfatl`) -- Generates access token using Azure CLI: `az account get-access-token --resource-type oss-rdbms` +- Automatically retrieves Azure AD access token using Azure Identity SDK +- Tries multiple authentication methods in order: environment variables, managed identity, Visual Studio, VS Code, Azure CLI (`az login`), Azure PowerShell, etc. - Builds connection string automatically: - Host: `altinn-corr-prod-dbserver.postgres.database.azure.com`Also applies to: 38-44
🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/README.md` at line 16, The README currently mandates Azure CLI and describes using `az account get-access-token` as a subprocess, but the code (Program.cs) uses Azure.Identity's `DefaultAzureCredential`; remove the absolute Azure CLI prerequisite, replace the subprocess instructions (lines ~38-44) with a short explanation that `DefaultAzureCredential` is used and enumerates its credential chain (environment variables, managed identity, Visual Studio, VS Code, Azure CLI/az login, Azure PowerShell, etc.), and explicitly note that `az login` is one supported option rather than a requirement so readers understand multiple auth methods are supported.
209-210:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix broken database documentation links in exporter README.
tools/Altinn.Correspondence.DialogActivityExporter/README.mdcurrently links todocs/database/DBA_Index_Creation_Scripts.sqlanddocs/database/DBA_Index_Request_Executive_Summary.md, but neither file exists underdocs/database/. Update the links to:
docs/database/Index_Creation_Scripts.sqldocs/database/Index_Creation_Production_Summary.md(Also,
docs/DialogActivityExport_Summary.mdreferences the same missingDBA_*.sql/.mdfilenames.)🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/README.md` around lines 209 - 210, Update the broken documentation links that point to non-existent DBA_* files: in tools/Altinn.Correspondence.DialogActivityExporter/README.md replace `docs/database/DBA_Index_Creation_Scripts.sql` and `docs/database/DBA_Index_Request_Executive_Summary.md` with `docs/database/Index_Creation_Scripts.sql` and `docs/database/Index_Creation_Production_Summary.md` respectively, and make the same replacements in docs/DialogActivityExport_Summary.md so both files reference the correct `Index_Creation_Scripts.sql` and `Index_Creation_Production_Summary.md` filenames.
♻️ Duplicate comments (1)
docs/database/Max_Batches_Feature_Summary.md (1)
360-360:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove inaccurate auto-open claim.
The statement "File opens automatically when done" is not supported by the code. Neither
Program.csnor the PowerShell test scripts contain logic to automatically open the output CSV file.📝 Suggested fix
-# That's it! File opens automatically when done. +# That's it! Check the output file at the path you specified.🤖 Prompt for 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. In `@docs/database/Max_Batches_Feature_Summary.md` at line 360, Remove or reword the inaccurate sentence "File opens automatically when done" in docs/database/Max_Batches_Feature_Summary.md because no code in Program.cs or the PowerShell test scripts implements auto-opening; replace it with a correct statement such as "The output CSV must be opened manually" or add a short note describing how to open the output manually so the documentation matches the actual behavior.
🧹 Nitpick comments (1)
docs/database/Status_6_Performance_Issue_Quick_Ref.md (1)
79-86: ⚡ Quick winAdd a pre-drop safety check before recommending index removal.
Before
DROP INDEX CONCURRENTLY, include a usage verification step (e.g., recentpg_stat_user_indexesusage + known workload check) to reduce regression risk for unrelated query paths.Also applies to: 142-153
🤖 Prompt for 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. In `@docs/database/Status_6_Performance_Issue_Quick_Ref.md` around lines 79 - 86, Before issuing DROP INDEX CONCURRENTLY for correspondence."IX_A2Iss1716A2Events_Status_CorrId", add a pre-drop safety verification: check pg_stat_user_indexes (filter by indexrelname = 'IX_A2Iss1716A2Events_Status_CorrId') for recent idx_scan/last_usage and validate against known workload or representative EXPLAINs to ensure the index is unused or low-impact; only proceed with DROP INDEX CONCURRENTLY if usage is below threshold and document the verification steps and recreation command so the index can be restored quickly if regressions occur.
🤖 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 `@docs/CRITICAL_FIX_Cursor_Pagination_Index_Issue.md`:
- Around line 35-37: The AFTER SQL example is inconsistent: it uses ORDER BY
stats."CorrespondenceId" whereas the corrected fix uses ORDER BY
a2Events."CorrespondenceId"; update the AFTER snippet (and the duplicate
occurrence around lines 113-120) to use ORDER BY a2Events."CorrespondenceId" and
ensure the ORDER BY and the AND predicate reference the same table alias
(a2Events."CorrespondenceId", Status) so the query remains consistent and
index-friendly.
In `@docs/database/A2Iss1716A2Events_Production_Verification.md`:
- Around line 5-8: The top-level readiness statement is inconsistent with the
"Status 6 needs attention" section; update the document so there is a single
clear go/no-go decision: either change the top-level banner ("READY FOR
DEPLOYMENT" / "Production verification COMPLETE") to a conditional state (e.g.,
"Pending remediation - NOT READY FOR DEPLOYMENT") if the Status 6 remediation
steps are outstanding, or remove/resolve the Status 6 block (and its remediation
checklist) if those actions are complete; ensure references to "Status 6" and
the readiness banner are reconciled and that any remaining required actions are
explicitly listed under a single final decision statement (also apply the same
change to the repeated block around lines 206-247).
In `@docs/database/Helper_Table_Optimization.md`:
- Around line 183-189: The cursor predicate using the UUID-based composite
(stats."CorrespondenceId", stats."Status") is inconsistent with the helper-table
A2 INT ordering; replace the guidance so the cursor predicate and ORDER BY use
the deterministic helper-table key (the A2 INT column and any tie-breaker used
by the helper table) rather than UUIDs — update references to
stats."CorrespondenceId" and stats."Status" in the Cursor section to show the
same A2 INT ordering and equivalent tie-breaker column(s) used by the helper
table to prevent duplicates/skips and ensure batch consistency.
- Around line 133-143: The fenced EXPLAIN output block containing the lines
starting with "Execution Time: 847.773 ms" and the index names
"IX_A2iss1716corrs_A2_A3" / "IX_Correspondence_CorrespondenceId_Status" needs a
language tag on the opening fence (e.g. change ``` to ```text) so markdownlint
passes and rendering is consistent; update that specific fenced block in
Helper_Table_Optimization.md to use a tagged fence like ```text.
In `@docs/database/Max_Batches_Feature_Summary.md`:
- Around line 14-23: Update the documented API signatures and examples to match
the current implementation: replace the old ExportToCSVAsync signature with the
one that includes preCalculatedCount, maxBatches and freshStart (use
ExportToCSVAsync, preCalculatedCount, maxBatches, freshStart as the referenced
symbols), remove any mention of skipTotalCount and the deprecated --oldest flag,
and update CLI examples to use --fresh instead; ensure examples and descriptive
text (where lines reference skipTotalCount/oldest) reflect that
preCalculatedCount is used directly and that --fresh toggles freshStart.
In `@docs/database/Optimize_A2Iss1716A2Events_Indexes.sql`:
- Around line 217-227: The query against pg_stat_user_indexes uses non-existent
columns tablename/indexname; replace them with relname and indexrelname
respectively and update the WHERE clause to filter relname = 'A2Iss1716A2Events'
(and keep schemaname = 'correspondence') and ORDER BY indexrelname so the SELECT
(relname, indexrelname, idx_scan, idx_tup_read, idx_tup_fetch) returns the
correct stats for the table/indexes using the pg_stat_user_indexes view.
- Around line 60-69: The query uses non-existent columns tablename/indexname
from pg_stat_user_indexes; replace those with the correct column names relname
and indexrelname (e.g., select relname as tablename and indexrelname as
indexname, filter WHERE relname = 'A2Iss1716A2Events' and schemaname =
'correspondence', and ORDER BY indexrelname) while keeping
pg_relation_size(indexrelid) and idx_scan unchanged so the view returns the
expected rows.
In `@docs/database/Query_Logging_Feature_Summary.md`:
- Around line 30-88: The sample SQL in the docs is out of sync with
FetchStatusRecordsAsync: remove the removed oldestCorrespondenceDate/@oldestDate
replacement block, change the displayed "LEFT JOIN
correspondence.\"IdempotencyKeys\"" to "INNER JOIN" to match the current join in
FetchStatusRecordsAsync, and update the timestamp WHERE clause to reflect the
actual implementation (use "BETWEEN '2019-03-23 00:00:00' AND `@cutoffTimestamp`"
for the Issue 1951 example or use the "< `@cutoffTimestamp`" variant for the Issue
1716 example) so the logged-query examples match the code paths in
FetchStatusRecordsAsync.
In `@docs/Export_Scripts_Quick_Reference.md`:
- Line 37: Fix the markdown lint issues by adding a language identifier to the
three fenced code blocks and ensuring blank lines surround the two tables:
update the unlabeled fences at the sample output blocks to use ```text (the
blocks shown around "Batch 1: Fetch=150ms..." and "[1/3] Azure AD
authentication... ✓" and the final "Need to validate query/format?" block) so
they are valid fenced code blocks, and add a blank line above and below each
table under the headings "Test Export (20,000 rows)" and "Production Export
(9.97M rows)" so the tables have surrounding blank lines.
In `@docs/Network_Read_Performance_Issue.md`:
- Around line 140-143: The document is internally inconsistent about the default
BatchSize (1,000 vs 5,000); pick a single operational default and update all
occurrences accordingly (including the example command
".\export-production-1716.ps1 -BatchSize 1000" and the action-item text that
mentions 5,000), and mark the alternate value explicitly as "experimental" or
"for testing only" wherever it appears (also update the section covering lines
172–176) so the guidance is consistent and each non-default value is clearly
labeled.
In `@docs/Performance_Timing_Analysis_Guide.md`:
- Around line 188-199: The docs currently recommend setting private const int
_batchSize = 10000 (in Program.cs / DialogActivityExportService.cs) but your
performance tests show severe degradation at that size; update the guidance to
either specify the validated safe batch-size range (e.g., X–Y rows) or make the
10,000 recommendation conditional on same-region execution and sufficient
network throughput, and mention the exact symbol _batchSize so readers know
where to change it.
In
`@tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs`:
- Line 36: DialogActivityExportService currently builds the checkpoint filename
as checkpointPath = outputFilePath + ".checkpoint" which mismatches the external
script expecting ".checkpoint.json"; update the service to use the same
extension (e.g., set checkpointPath = outputFilePath + ".checkpoint.json") so
all read/write/delete operations in DialogActivityExportService (search for
checkpointPath, and the class/method in DialogActivityExportService) produce the
file the export script expects; ensure any other references in the class that
assume the old name are updated accordingly.
In
`@tools/Altinn.Correspondence.DialogActivityExporter/export-production-1716.ps1`:
- Around line 26-29: The declared BatchSize runtime default in the script is
inconsistent with the documented production default: update the BatchSize
parameter and any hardcoded default assignment to 10000 so the runtime behavior
matches the documentation; specifically, change the default value for the
BatchSize parameter/variable (the BatchSize PARAMETER declaration and the
assignment where $BatchSize is set to 5000) to 10000 and ensure any other
references/use-sites (e.g., where $BatchSize is used later) are unaffected by
this change.
In `@tools/Altinn.Correspondence.DialogActivityExporter/run-export.ps1`:
- Around line 39-57: The help examples currently call .\test-export.ps1 which is
incorrect for this script; update all example lines that reference
.\test-export.ps1 to reference .\run-export.ps1 (the script's actual entrypoint)
so the EXAMPLE blocks and any usage snippets match the script name used by the
run-export.ps1 entrypoint and parameters like -Issue, -BatchSize, -MaxBatches,
-OutputPath and -ConnectionString remain unchanged.
In `@tools/Altinn.Correspondence.DialogActivityExporter/test-export-2.ps1`:
- Around line 39-57: Update the help/examples that reference ".\test-export.ps1"
to use the correct script name ".\test-export-2.ps1"; search for occurrences in
the file (examples and any usage/help text) and replace them so commands like
".\test-export-2.ps1 -Issue 1716 -BatchSize 5000 -MaxBatches 1" and others
correctly reference the current script name, ensuring all example invocations
and comment blocks consistently use "test-export-2.ps1".
---
Outside diff comments:
In `@tools/Altinn.Correspondence.DialogActivityExporter/README.md`:
- Line 16: The README currently mandates Azure CLI and describes using `az
account get-access-token` as a subprocess, but the code (Program.cs) uses
Azure.Identity's `DefaultAzureCredential`; remove the absolute Azure CLI
prerequisite, replace the subprocess instructions (lines ~38-44) with a short
explanation that `DefaultAzureCredential` is used and enumerates its credential
chain (environment variables, managed identity, Visual Studio, VS Code, Azure
CLI/az login, Azure PowerShell, etc.), and explicitly note that `az login` is
one supported option rather than a requirement so readers understand multiple
auth methods are supported.
- Around line 209-210: Update the broken documentation links that point to
non-existent DBA_* files: in
tools/Altinn.Correspondence.DialogActivityExporter/README.md replace
`docs/database/DBA_Index_Creation_Scripts.sql` and
`docs/database/DBA_Index_Request_Executive_Summary.md` with
`docs/database/Index_Creation_Scripts.sql` and
`docs/database/Index_Creation_Production_Summary.md` respectively, and make the
same replacements in docs/DialogActivityExport_Summary.md so both files
reference the correct `Index_Creation_Scripts.sql` and
`Index_Creation_Production_Summary.md` filenames.
---
Duplicate comments:
In `@docs/database/Max_Batches_Feature_Summary.md`:
- Line 360: Remove or reword the inaccurate sentence "File opens automatically
when done" in docs/database/Max_Batches_Feature_Summary.md because no code in
Program.cs or the PowerShell test scripts implements auto-opening; replace it
with a correct statement such as "The output CSV must be opened manually" or add
a short note describing how to open the output manually so the documentation
matches the actual behavior.
---
Nitpick comments:
In `@docs/database/Status_6_Performance_Issue_Quick_Ref.md`:
- Around line 79-86: Before issuing DROP INDEX CONCURRENTLY for
correspondence."IX_A2Iss1716A2Events_Status_CorrId", add a pre-drop safety
verification: check pg_stat_user_indexes (filter by indexrelname =
'IX_A2Iss1716A2Events_Status_CorrId') for recent idx_scan/last_usage and
validate against known workload or representative EXPLAINs to ensure the index
is unused or low-impact; only proceed with DROP INDEX CONCURRENTLY if usage is
below threshold and document the verification steps and recreation command so
the index can be restored quickly if regressions occur.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2edb592-d79b-45fd-90ce-f4f81e19c668
📒 Files selected for processing (43)
docs/CRITICAL_FIX_Cursor_Pagination_Index_Issue.mddocs/DialogActivityExport_CombinedMode.mddocs/DialogActivityExport_Summary.mddocs/Export_Scripts_Quick_Reference.mddocs/Network_Read_Performance_Issue.mddocs/Performance_Timing_Analysis_Guide.mddocs/database/A2Iss1716A2Events_Helper_Table_Migration.mddocs/database/A2Iss1716A2Events_Production_Verification.mddocs/database/Azure_Identity_Migration_Summary.mddocs/database/Check_Disk_Space_And_Table_Stats.sqldocs/database/Configure_PostgreSQL_For_Index_Creation.sqldocs/database/Fix_A2Parties_Recipient_Filter_Index.sqldocs/database/Fix_A2Parties_Recipient_Filter_Schema.sqldocs/database/Helper_Table_Optimization.mddocs/database/Index_Creation_Production_Summary.mddocs/database/Index_Creation_Scripts.sqldocs/database/Max_Batches_Feature_Summary.mddocs/database/Optimize_A2Iss1716A2Events_Indexes.sqldocs/database/Performance_Optimization_Summary.sqldocs/database/Query_Logging_Feature_Summary.mddocs/database/Query_Performance_Notes.mddocs/database/Quick_Test_Reference.mddocs/database/README.mddocs/database/Resumable_Export_Feature.mddocs/database/Status_6_Performance_Issue_Quick_Ref.mddocs/database/Test_Export_Query.sqldocs/database/Testing_Documentation_Summary.mddocs/database/Testing_Guide.mdtools/Altinn.Correspondence.DialogActivityExporter/Altinn.Correspondence.DialogActivityExporter.csprojtools/Altinn.Correspondence.DialogActivityExporter/CHANGELOG.mdtools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cstools/Altinn.Correspondence.DialogActivityExporter/Pre_Calculated_Counts_Feature.mdtools/Altinn.Correspondence.DialogActivityExporter/Program.cstools/Altinn.Correspondence.DialogActivityExporter/README.mdtools/Altinn.Correspondence.DialogActivityExporter/Simplified_Progress_Tracking.mdtools/Altinn.Correspondence.DialogActivityExporter/appsettings.jsontools/Altinn.Correspondence.DialogActivityExporter/appsettings.local.example.jsontools/Altinn.Correspondence.DialogActivityExporter/calculate-counts.sqltools/Altinn.Correspondence.DialogActivityExporter/export-production-1716.ps1tools/Altinn.Correspondence.DialogActivityExporter/run-export.ps1tools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sqltools/Altinn.Correspondence.DialogActivityExporter/test-export-2.ps1tools/Altinn.Correspondence.DialogActivityExporter/test-export.ps1
✅ Files skipped from review due to trivial changes (12)
- tools/Altinn.Correspondence.DialogActivityExporter/appsettings.local.example.json
- docs/database/Azure_Identity_Migration_Summary.md
- docs/database/Query_Performance_Notes.md
- tools/Altinn.Correspondence.DialogActivityExporter/CHANGELOG.md
- tools/Altinn.Correspondence.DialogActivityExporter/appsettings.json
- docs/DialogActivityExport_Summary.md
- docs/database/Check_Disk_Space_And_Table_Stats.sql
- docs/database/Testing_Guide.md
- docs/database/Testing_Documentation_Summary.md
- docs/database/A2Iss1716A2Events_Helper_Table_Migration.md
- docs/DialogActivityExport_CombinedMode.md
- docs/database/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/Altinn.Correspondence.DialogActivityExporter/Altinn.Correspondence.DialogActivityExporter.csproj
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (14)
docs/database/Max_Batches_Feature_Summary.md (2)
134-139:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate PowerShell snippet to use correct variable name.
Lines 134-139 show building
$args, but past review indicated the actual script uses$commandArgsto avoid conflicts with PowerShell's automatic$argsvariable. Update the documentation snippet to match the script implementation.🤖 Prompt for 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. In `@docs/database/Max_Batches_Feature_Summary.md` around lines 134 - 139, The docs snippet uses PowerShell's $args variable but the script uses $commandArgs; update the example to build and pass "--max-batches", $MaxBatches into the $commandArgs array (replace $args with $commandArgs) so it matches the implementation and avoids clobbering PowerShell's automatic $args variable.
360-360:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove inaccurate statement about auto-opening files.
Line 360 states "File opens automatically when done," but the test export script does not implement this behavior. Remove this statement or clarify that users must open files manually.
🤖 Prompt for 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. In `@docs/database/Max_Batches_Feature_Summary.md` at line 360, Remove or reword the inaccurate sentence "File opens automatically when done." in the docs (search for the exact phrase "File opens automatically when done.") and either delete it or replace it with a clarification that the test export script does not auto-open files and users must open the exported file manually; ensure the updated line clearly states the actual behavior.tools/Altinn.Correspondence.DialogActivityExporter/run-export.ps1 (1)
39-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCorrect example script name in help text.
Lines 39–57 reference
.\test-export.ps1, but this file is namedrun-export.ps1. Users copying examples will encounter file-not-found errors.📝 Proposed fix
.EXAMPLE - .\test-export.ps1 + .\run-export.ps1 # Quick test with 2 batches (2000 rows) for Issue `#1951` .EXAMPLE - .\test-export.ps1 -Issue 1716 -BatchSize 5000 -MaxBatches 1 + .\run-export.ps1 -Issue 1716 -BatchSize 5000 -MaxBatches 1 # Test Issue `#1716` with 1 batch of 5000 rows .EXAMPLE - .\test-export.ps1 -MaxBatches 5 + .\run-export.ps1 -MaxBatches 5 # Test with 5 batches to get more data for verification .EXAMPLE - .\test-export.ps1 -Issue all -OutputPath C:\temp\my_test.csv + .\run-export.ps1 -Issue all -OutputPath C:\temp\my_test.csv # Test both issues combined (fast startup, no COUNT queries) .EXAMPLE - .\test-export.ps1 -ConnectionString "Host=localhost;Database=correspondence_dev;Username=dev;Password=dev" + .\run-export.ps1 -ConnectionString "Host=localhost;Database=correspondence_dev;Username=dev;Password=dev" # Test against local dev database🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/run-export.ps1` around lines 39 - 57, Update the help/examples block to reference the correct script name: replace all occurrences of ".\test-export.ps1" with ".\run-export.ps1" in the run-export.ps1 file so the documented EXAMPLE lines match the actual script name; ensure each example (the four occurrences shown) is updated to ".\run-export.ps1" so users won't get file-not-found errors when following examples in the run-export.ps1 help text.tools/Altinn.Correspondence.DialogActivityExporter/test-export-2.ps1 (1)
39-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCorrect example script name in help text.
Lines 39–57 reference
.\test-export.ps1, but this file is namedtest-export-2.ps1. Users copying examples will encounter file-not-found errors.📝 Proposed fix
.EXAMPLE - .\test-export.ps1 + .\test-export-2.ps1 # Quick test with 2 batches (2000 rows) for Issue `#1951` .EXAMPLE - .\test-export.ps1 -Issue 1716 -BatchSize 5000 -MaxBatches 1 + .\test-export-2.ps1 -Issue 1716 -BatchSize 5000 -MaxBatches 1 # Test Issue `#1716` with 1 batch of 5000 rows .EXAMPLE - .\test-export.ps1 -MaxBatches 5 + .\test-export-2.ps1 -MaxBatches 5 # Test with 5 batches to get more data for verification .EXAMPLE - .\test-export.ps1 -Issue all -OutputPath C:\temp\my_test.csv + .\test-export-2.ps1 -Issue all -OutputPath C:\temp\my_test.csv # Test both issues combined (fast startup, no COUNT queries) .EXAMPLE - .\test-export.ps1 -ConnectionString "Host=localhost;Database=correspondence_dev;Username=dev;Password=dev" + .\test-export-2.ps1 -ConnectionString "Host=localhost;Database=correspondence_dev;Username=dev;Password=dev" # Test against local dev database🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/test-export-2.ps1` around lines 39 - 57, Update the example help text in test-export-2.ps1 so the script name referenced matches the actual file: replace occurrences of ".\test-export.ps1" in the EXAMPLE blocks with ".\test-export-2.ps1" (or use a dynamic reference like $PSCommandPath if you prefer) to prevent users from copying an incorrect filename; ensure all example lines (the four EXAMPLE blocks) are updated accordingly.tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs (1)
36-36:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCRITICAL: Checkpoint file extension must match production script expectation.
Line 36 creates checkpoint at
outputFilePath + ".checkpoint", butexport-production-1716.ps1line 164 reads"$OutputPath.checkpoint.json". The resume feature will fail because the script cannot find the checkpoint file written by the service.🐛 Proposed fix
- var checkpointPath = outputFilePath + ".checkpoint"; + var checkpointPath = outputFilePath + ".checkpoint.json";🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs` at line 36, The checkpoint filename currently created in DialogActivityExportService (variable checkpointPath) uses outputFilePath + ".checkpoint" which doesn't match the production script expectation; change the code that sets checkpointPath to use outputFilePath + ".checkpoint.json" so the service writes a .checkpoint.json file (update any references inside DialogActivityExportService.cs that read/write the checkpoint using checkpointPath accordingly).tools/Altinn.Correspondence.DialogActivityExporter/export-production-1716.ps1 (1)
26-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix BatchSize parameter help to match actual default.
Line 27 states the default batch size is
10000, but line 129 sets the actual default to5000. The performance estimates (lines 86-91) also describe 5000-batch behavior, confirming the code default is correct.📝 Proposed fix
.PARAMETER BatchSize - Number of rows per batch (default: 10000) + Number of rows per batch (default: 5000) Larger batch sizes reduce network overhead - Recommended: 10000-20000 for optimal performance + Recommended: 5000-20000 for optimal performance🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/export-production-1716.ps1` around lines 26 - 29, Update the BatchSize parameter help text to match the actual default of 5000 (the value set for the BatchSize parameter in the script) and align the performance guidance with 5000-batch behavior; specifically modify the .PARAMETER BatchSize description to state "default: 5000" and adjust the recommended batch-size guidance/phrasing to reflect the script's 5000 baseline and appropriate recommendation range.docs/database/Optimize_A2Iss1716A2Events_Indexes.sql (2)
60-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
pg_stat_user_indexescolumn names.The view exposes
relnameandindexrelname, nottablenameandindexname. This query will fail or return no rows.🔧 Corrected query
SELECT schemaname, - tablename, - indexname, + relname AS tablename, + indexrelname AS indexname, pg_size_pretty(pg_relation_size(indexrelid)) as size, idx_scan as times_used FROM pg_stat_user_indexes WHERE schemaname = 'correspondence' - AND tablename = 'A2Iss1716A2Events' -ORDER BY indexname; + AND relname = 'A2Iss1716A2Events' +ORDER BY indexrelname;🤖 Prompt for 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. In `@docs/database/Optimize_A2Iss1716A2Events_Indexes.sql` around lines 60 - 69, The query against pg_stat_user_indexes uses nonexistent column names tablename and indexname; update the SELECT and WHERE to use the view's actual column names relname and indexrelname (e.g., select schemaname, relname, indexrelname, pg_size_pretty(pg_relation_size(indexrelid)) as size, idx_scan as times_used and filter WHERE schemaname = 'correspondence' AND relname = 'A2Iss1716A2Events') so the query returns the expected rows; keep the existing pg_relation_size(indexrelid) and idx_scan references.
217-227:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
pg_stat_user_indexescolumn names (same issue as above).Same correction needed: use
relnameandindexrelnameinstead oftablenameandindexname.🔧 Corrected query
SELECT schemaname, - tablename, - indexname, + relname AS tablename, + indexrelname AS indexname, idx_scan as times_used, idx_tup_read as tuples_read, idx_tup_fetch as tuples_fetched FROM pg_stat_user_indexes WHERE schemaname = 'correspondence' - AND tablename = 'A2Iss1716A2Events' -ORDER BY indexname; + AND relname = 'A2Iss1716A2Events' +ORDER BY indexrelname;🤖 Prompt for 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. In `@docs/database/Optimize_A2Iss1716A2Events_Indexes.sql` around lines 217 - 227, The SELECT against pg_stat_user_indexes uses incorrect column names; update the query to use relname and indexrelname instead of tablename and indexname when querying the 'correspondence' schema for table A2Iss1716A2Events so the columns selected (schemaname, relname, indexrelname, idx_scan, idx_tup_read, idx_tup_fetch) are valid—locate the SQL block querying pg_stat_user_indexes for schemaname = 'correspondence' AND tablename = 'A2Iss1716A2Events' and replace tablename→relname and indexname→indexrelname accordingly.docs/database/Helper_Table_Optimization.md (1)
133-143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language tag to fenced code block.
The fenced EXPLAIN output block is missing a language specifier, triggering a markdownlint warning and potentially inconsistent rendering.
🔧 Quick fix
-``` +```text Execution Time: 847.773 ms Buffers: shared hit=11795 read=290 I/O Timings: shared read=816.488🤖 Prompt for 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. In `@docs/database/Helper_Table_Optimization.md` around lines 133 - 143, The fenced EXPLAIN output block starting with "Execution Time: 847.773 ms" and containing index names like "IX_A2iss1716corrs_A2_A3" and "IX_Correspondence_CorrespondenceId_Status" is missing a language tag; update that fenced code block to include a language specifier (e.g. ```text) so markdownlint warnings are resolved and rendering is consistent.docs/database/Fix_A2Parties_Recipient_Filter_Index.sql (1)
49-58:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTransaction detection warning is unreliable.
pg_current_xact_id_if_assigned()returnsNULLinside an explicitBEGINblock until a transaction ID is assigned (usually on the first data-modifying statement). This means the warning won't fire for read-only transactions or transactions that haven't written yet, even thoughCREATE INDEX CONCURRENTLYwill still fail.Since
CREATE INDEX CONCURRENTLYproduces a clear error message when run inside any transaction block, consider either removing this conditional check entirely or rewording it as an unconditional reminder that the script requires autocommit mode.💡 Alternative: unconditional reminder
-DO $$ -BEGIN - IF pg_current_xact_id_if_assigned() IS NOT NULL THEN - RAISE WARNING 'You are inside a transaction block!'; - RAISE WARNING 'CREATE INDEX CONCURRENTLY requires autocommit mode.'; - RAISE WARNING 'If the next command fails, exit transaction and run again.'; - END IF; -END $$; +DO $$ +BEGIN + RAISE NOTICE '⚠️ Reminder: CREATE INDEX CONCURRENTLY requires autocommit mode.'; + RAISE NOTICE ' Do not run this script inside BEGIN/COMMIT or via migration tools that wrap scripts in transactions.'; +END $$;🤖 Prompt for 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. In `@docs/database/Fix_A2Parties_Recipient_Filter_Index.sql` around lines 49 - 58, The DO $$ block uses pg_current_xact_id_if_assigned() to detect transactions which is unreliable; remove the conditional check and instead replace the guarded warnings with an unconditional reminder that the script must be run in autocommit mode (no transaction block) before running CREATE INDEX CONCURRENTLY, or simply remove the DO $$ block entirely so the user relies on the database's own error; locate the DO $$ ... END $$; block and the reference to pg_current_xact_id_if_assigned() and update it to a plain advisory message about autocommit/avoid BEGIN blocks for CREATE INDEX CONCURRENTLY.docs/database/Index_Creation_Scripts.sql (1)
380-384:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate disk space estimates to match production actuals.
The estimates here show
~15 GBtotal, but production results documented at the top of this file (lines 16, 23) show actual disk usage is~27 GB(Index#1: 3 GB, Index#2: 24 GB). The individual index estimates at lines 380-382 are also outdated (showing ~1.5 GB and ~12 GB vs actual 3 GB and 24 GB). Update these values to reflect the actual production deployment results to avoid underestimating disk requirements.📝 Suggested update
-- Estimated Index Sizes: -- ---------------------- --- IX_CorrespondenceStatuses_Status_SyncedTimestamp_Synced: ~1.5 GB (7-9M rows) --- IX_CorrespondenceStatuses_Status_StatusChanged_Migrated: ~12.0 GB (150M rows) **LARGEST** +-- IX_CorrespondenceStatuses_Status_SyncedTimestamp_Synced: ~3 GB (actual production) +-- IX_CorrespondenceStatuses_Status_StatusChanged_Migrated: ~24 GB (actual production) **LARGEST** -- IX_Correspondences_Id_Created_MigrationFilter: ~1.5 GB (partial index) -- ----------------------------------------------------------------------------- --- TOTAL NEW INDEX SPACE: ~15 GB (export optimization) +-- TOTAL NEW INDEX SPACE: ~27 GB (actual production deployment)🤖 Prompt for 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. In `@docs/database/Index_Creation_Scripts.sql` around lines 380 - 384, Update the index size comments to match production measurements: change the individual index estimates for IX_CorrespondenceStatuses_Status_SyncedTimestamp_Synced from "~1.5 GB" to "3 GB" and IX_CorrespondenceStatuses_Status_StatusChanged_Migrated from "~12.0 GB" to "24 GB" (keep the partial index note for IX_Correspondences_Id_Created_MigrationFilter if unchanged), and update the "TOTAL NEW INDEX SPACE" comment from "~15 GB" to "~27 GB" so the header and per-index lines reflect actual production usage recorded earlier in the file.docs/database/Configure_PostgreSQL_For_Index_Creation.sql (1)
170-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace misleading "free RAM" check with actual memory capacity verification.
pg_total_relation_size('pg_class')reports the on-disk size of thepg_classrelation (table + indexes + TOAST), not available server RAM. This guidance cannot determine whether settingmaintenance_work_mem = 4GBis safe. Operators need to check actual instance/VM memory capacity and current utilization (via OS tools likefree -m/vmstator cloud provider metrics) and account for expected concurrent maintenance operations.🔧 Suggested rewrite
-- 2. Memory availability: -– - Ensure server has 4+ GB free RAM before setting maintenance_work_mem = 4GB -– - Check with: SELECT pg_size_pretty(pg_total_relation_size('pg_class')); +– - Ensure server has 4+ GB free RAM before setting maintenance_work_mem = 4GB +– - Check instance/VM memory capacity via cloud provider metrics or OS tools (e.g., free -m) +– - Account for concurrent maintenance operations and existing memory usage🤖 Prompt for 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. In `@docs/database/Configure_PostgreSQL_For_Index_Creation.sql` around lines 170 - 173, The note under maintenance_work_mem incorrectly suggests using pg_total_relation_size('pg_class') to check "free RAM"; update the guidance around maintenance_work_mem to remove that command and instead instruct operators to verify actual system memory capacity and current utilization using OS tools or cloud metrics (e.g., free -m, vmstat, top, or cloud provider instance metrics) and to account for concurrent maintenance operations before setting maintenance_work_mem = 4GB; reference the existing maintenance_work_mem guidance and remove or replace the pg_total_relation_size('pg_class') check with the OS/cloud-based checks.docs/Network_Read_Performance_Issue.md (1)
138-176:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConflicting batch-size recommendations create deployment ambiguity.
Line 140 recommends "Use batch size 1,000" as the proven safe choice, but line 172 states "Reverted default batch size to 5,000" as an action item. The guidance doesn't clearly state which value is the current operational default or whether users need to explicitly override it.
Users following this guide won't know whether to:
- Trust the 5,000 default (line 172) and skip overrides
- Explicitly set 1,000 (line 140) to ensure proven performance
- Test 5,000 first (lines 147-152) before using in production
Please clarify the final operational recommendation and align all references. For example: "Default is set to 1,000 (proven). You may test 5,000 as experimental optimization, but revert to 1,000 if testing shows degradation."
🤖 Prompt for 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. In `@docs/Network_Read_Performance_Issue.md` around lines 138 - 176, The document has conflicting default batch-size guidance ("Use batch size 1,000", "Test 5,000", and "Reverted default batch size to 5,000"); pick and state a single operational default and make the other suggestions explicitly experimental. Update the text so the default is clearly declared (e.g., "Default batch size: 1,000 (proven stable)") and change the action item referencing "Reverted default batch size to 5,000" to an experimental note (e.g., "Default remains 1,000; you may test 5,000 using the 'Test 5,000' steps and only switch to 5,000 in production if tests show acceptable latency"), and ensure the "For Your Current Setup" instructions and the "Action Items" section both reflect that same single default policy so all references to "Use batch size 1,000", "Test 5,000", and "Reverted default batch size to 5,000" are aligned.docs/Performance_Timing_Analysis_Guide.md (1)
186-199:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCRITICAL: Batch size recommendation contradicts measured performance data.
Lines 190 and 196 recommend setting
_batchSize = 10000to "amortize network overhead," butNetwork_Read_Performance_Issue.mdin this same PR demonstrates that batch size 10,000 causes catastrophic performance degradation for local/cross-region scenarios:
- Batch size 1,000: 37 minutes (proven)
- Batch size 10,000: 38.7 hours (63x slower, line 167 of Network_Read_Performance_Issue.md)
Users following this guide will unknowingly configure a 38-hour export instead of a 37-minute export. The recommendation is only valid when running from an Azure VM in the same region (per line 154-159 of Network_Read_Performance_Issue.md).
Please either:
- Change the recommendation to align with the validated safe batch size (1,000-5,000), OR
- Add an explicit conditional: "Only increase to 10,000 if running from Azure VM in same region. For local execution, use 1,000-5,000 max."
🤖 Prompt for 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. In `@docs/Performance_Timing_Analysis_Guide.md` around lines 186 - 199, Update the Recommendations section so the _batchSize guidance matches measured results: either replace the blanket "_batchSize = 10000" recommendation with a safe suggested range (e.g., 1,000–5,000) and note the original test numbers, or add an explicit conditional that "_batchSize = 10000" is only recommended when running from an Azure VM in the same region; reference the variable name (_batchSize) and the example locations (Program.cs / DialogActivityExportService.cs) so readers know where to change it, and cite Network_Read_Performance_Issue.md as the supporting evidence for the change.
🧹 Nitpick comments (8)
docs/database/A2Iss1716A2Events_Production_Verification.md (2)
285-288: ⚡ Quick winIncomplete "Updated" verification checklist section.
This section header promises an updated checklist but only includes three items copied from the Status 4 checklist above (Memoize Caching, DISTINCT Working, Performance Target). Either complete this section with all verification items or remove it to avoid confusion.
🤖 Prompt for 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. In `@docs/database/A2Iss1716A2Events_Production_Verification.md` around lines 285 - 288, The "## Verification Checklist (Updated)" section is incomplete and duplicates only three items (Memoize Caching, DISTINCT Working, Performance Target); either expand this section to include the full set of verification items expected in the updated checklist or remove the "## Verification Checklist (Updated)" header and its duplicated list to avoid confusion. Locate the header "## Verification Checklist (Updated)" and either (a) replace the three bullet items with the complete verification checklist (same items as the Status 4 checklist plus any truly updated results), ensuring each item is annotated (✅/❌) and updated metrics are included, or (b) delete the header and the three bullets so only the original Status 4 checklist remains.
21-25: 💤 Low valueConsider adding language tags to fenced code blocks.
Multiple code blocks throughout the document lack language specifiers. While these are PostgreSQL EXPLAIN ANALYZE outputs (not SQL), adding
textorplaintexttags would satisfy markdown linters and improve rendering consistency.📝 Example fix
-``` +```text Execution Time: 17.258 ms Planning Time: 5.416 ms Total: 22.674 msAlso applies to: 41-47, 56-61, 69-74, 81-89, 98-104, 113-120, 127-131, 209-214, 293-299, 302-308
🤖 Prompt for 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. In `@docs/database/A2Iss1716A2Events_Production_Verification.md` around lines 21 - 25, The fenced code blocks in docs/database/A2Iss1716A2Events_Production_Verification.md (for example the block containing "Execution Time: 17.258 ms") are missing language specifiers; update each triple-backtick fence to include a language tag like text or plaintext (e.g., change ``` to ```text) for the shown block and the other occurrences called out (lines around 41-47, 56-61, 69-74, 81-89, 98-104, 113-120, 127-131, 209-214, 293-299, 302-308) so markdown linters render them consistently.docs/Code_Review_Findings_Resolution.md (1)
1-196: ⚡ Quick winConsider adding batch-size inconsistency to unresolved issues.
The resolution doc doesn't mention the critical batch-size inconsistency between
Export_Scripts_Quick_Reference.md(documents 10,000) andIssue_1716_Export_Final_Specifications.md(documents 5,000 as optimal with detailed performance justification). This should be tracked as an unresolved documentation issue requiring alignment.🤖 Prompt for 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. In `@docs/Code_Review_Findings_Resolution.md` around lines 1 - 196, Add a new unresolved item to the resolution summary noting the batch-size inconsistency between Export_Scripts_Quick_Reference.md (documents 10,000) and Issue_1716_Export_Final_Specifications.md (documents 5,000 as optimal): state the conflict, mark it UNRESOLVED/Needs Alignment, and recommend a follow-up to reconcile the documented default (Export_Scripts_Quick_Reference.md) with the performance-backed recommendation in Issue_1716_Export_Final_Specifications.md so owners can decide which value to standardize on.tools/Altinn.Correspondence.DialogActivityExporter/Program.cs (1)
427-432: 💤 Low valueConsider parameterizing the Azure PostgreSQL connection details.
The Azure PostgreSQL host, database, and username domain are currently hard-coded. While acceptable for a production-only tool, this prevents reuse against staging/test environments without code modification.
♻️ Potential improvement
Read host/database from configuration or add optional CLI flags (
--host,--database) so the tool can target non-production environments during testing.🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs` around lines 427 - 432, The connection string assembly in Program.cs (variable connectionString) hard-codes Azure host, database and the username domain; change it to accept the host, database and username domain from configuration or CLI flags (e.g., add optional parameters like --host, --database, --user-domain or read from environment/appsettings) and default to the current production values when not supplied, then build connectionString using those variables so the tool can target staging/test environments without code changes.tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs (1)
551-551: 💤 Low valueRemove unused parameter binding for issue 1716.
Line 551 unconditionally adds
cutoffTimestampparameter, but the issue 1716 query (lines 468-494) never references@cutoffTimestamp. Only issue 1951 uses it (line 518). While Npgsql tolerates unused parameters, this creates maintenance confusion.♻️ Proposed fix
await using var cmd = new NpgsqlCommand(query, connection); - cmd.Parameters.AddWithValue("cutoffTimestamp", cutoffTimestamp); + + // Only issue 1951 query uses cutoffTimestamp parameter + if (issueNumber == 1951) + { + cmd.Parameters.AddWithValue("cutoffTimestamp", cutoffTimestamp); + } // Only add cursor parameters if we have a cursor value🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs` at line 551, The code unconditionally calls cmd.Parameters.AddWithValue("cutoffTimestamp", cutoffTimestamp) even though the SQL for issue 1716 doesn't reference `@cutoffTimestamp`; remove that unconditional binding and only add the cutoffTimestamp parameter where the SQL actually uses it (e.g., in the branch that builds the issue 1951 query). Locate the parameter addition in DialogActivityExportService.cs (the cmd variable and cutoffTimestamp local) and move or guard the AddWithValue call so it runs only when the constructed command text includes `@cutoffTimestamp` or when handling issue 1951.tools/Altinn.Correspondence.DialogActivityExporter/calculate-counts.sql (1)
15-21: ⚡ Quick winCentralize the cutoff timestamp to avoid drift between queries.
Line 15 requires manual updates, but the same cutoff is repeated in four places (Line 59, Line 76, Line 104, Line 121). Missing one edit will silently produce inconsistent totals for
PreCalculatedCounts.Also applies to: 59-59, 76-76, 104-104, 121-121
🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/calculate-counts.sql` around lines 15 - 21, Centralize the cutoff timestamp by declaring a single variable/CTE (e.g., cutoff_timestamp) at the top of calculate-counts.sql and reference that identifier in all four queries that feed PreCalculatedCounts instead of repeating the literal timestamp; update the WHERE clauses in each query to use cutoff_timestamp and remove the duplicate hard-coded timestamps so a single edit updates all counts consistently.docs/database/Performance_Optimization_Summary.sql (1)
62-123: ⚡ Quick winAlign documented query with actual production implementation.
This "optimized query" uses
UNION ALLandLEFT JOINtoIdempotencyKeys, butTest_Export_Query.sql(the production reference, line 32-33) andDialogActivityExportServicerun separate queries withINNER JOINtoIdempotencyKeys, then merge results in C#. The query shape here diverges from production, so EXPLAIN plans and timing guidance are not representative of the actual export path.💡 Recommendation
Update this section to show two separate
SELECTstatements (one for Status=4, one for Status=6) withINNER JOINtoIdempotencyKeys(matching the production service), or add a note clarifying that this is a simplified/alternative representation and refer readers toTest_Export_Query.sqlfor the exact production query structure.🤖 Prompt for 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. In `@docs/database/Performance_Optimization_Summary.sql` around lines 62 - 123, The documented "optimized query" diverges from production: it uses UNION ALL plus LEFT JOIN to correspondence."IdempotencyKeys" while Test_Export_Query.sql and DialogActivityExportService run two separate SELECTs (Status=4 and Status=6) each using an INNER JOIN to correspondence."IdempotencyKeys" and then merge results in C#; update the docs to match production by replacing the UNION ALL/LEFT JOIN shape with two separate SELECT statements (one filtering stats."Status" = 4 and one for stats."Status" = 6) that INNER JOIN to correspondence."IdempotencyKeys" (idcFetch/idcConfirm) or alternatively add a clear note that this is a simplified variant and point readers to Test_Export_Query.sql and DialogActivityExportService for the exact production query shape.docs/database/A2Iss1716A2Events_Helper_Table_Migration.md (1)
154-218: ⚡ Quick winStatus 6 index issue should be flagged earlier in the migration workflow.
The document marks
ANALYZEas "CRITICAL" (line 154) and explains it prevents Status 6 from being "200x slower" (line 162), but the actual solution (dropping the competing index) is buried in the Troubleshooting section starting at line 248. Users following the migration steps linearly might deploy, start the export, then discover Status 6 queries take 3-4 seconds instead of <100ms.Consider adding a callout or decision point in the main migration flow (around step 3-4) that explicitly warns: "Status 6 may use the wrong index even after ANALYZE; test both Status 4 and Status 6 before deploying, and be prepared to drop
IX_A2Iss1716A2Events_Status_CorrIdif Status 6 remains slow after cache warmup."🤖 Prompt for 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. In `@docs/database/A2Iss1716A2Events_Helper_Table_Migration.md` around lines 154 - 218, Add an explicit warning and decision step into the main migration flow near steps 3–4: explain that even after running ANALYZE on correspondence."A2Iss1716A2Events" Status 6 queries may still use the wrong index and remain slow, instruct users to run the provided EXPLAIN (ANALYZE, BUFFERS) tests for both Status = 4 and Status = 6 before deploying, and if Status 6 remains slow after a second (cached) run, proactively drop the competing index IX_A2Iss1716A2Events_Status_CorrId (or follow the Troubleshooting drop steps) and re-ANALYZE to ensure the planner picks the intended index.
🤖 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 `@docs/Code_Review_Findings_Resolution.md`:
- Around line 83-86: Update the review note that incorrectly states
Max_Batches_Feature_Summary.md is missing: open the
Code_Review_Findings_Resolution.md entry that references
Max_Batches_Feature_Summary.md, change the “file not found” text to acknowledge
that Max_Batches_Feature_Summary.md exists in this PR, and add a follow-up
action to inspect the Max_Batches_Feature_Summary.md API docs for the
preCalculatedCount parameter and update the documentation if the parameter
description is outdated or missing.
In `@docs/CRITICAL_FIX_Cursor_Pagination_Index_Issue.md`:
- Around line 28-43: Clarify and align the documentation: state that the ORDER
BY must reference the same columns used by the DISTINCT and the indexed table
used for the cursor seek (i.e., use a2Events."CorrespondenceId" in ORDER BY when
the cursor predicate is on a2Events), and update the explanatory line that
currently says "References `stats` columns (in SELECT list) → satisfies DISTINCT
requirement" to instead explain that equivalence via the join
(a2Events."CorrespondenceId" = stats."CorrespondenceId") allows the optimizer to
use the index on a2Events so ORDER BY can reference either table's column only
if the SELECT and DISTINCT list include the same (or equivalent) column;
explicitly note why the example chooses a2Events (to match the indexed table
used by the cursor).
In `@docs/database/Max_Batches_Feature_Summary.md`:
- Around line 84-93: Update the test-mode example for the DialogActivityExporter
command to remove the deprecated flag `--oldest "2019-03-23"` so the sample
matches the current CLI interface; edit the example block that contains
`DialogActivityExporter --issue 1951 ... --max-batches 2` and delete the
`--oldest` token (keeping the other flags like `--cutoff`, `--azure-ad`, and
`--max-batches`) so the docs no longer show the deprecated parameter.
- Around line 40-46: Update the docs to remove the obsolete skipTotalCount
snippet and instead describe the current behavior: delete references to
skipTotalCount and note that the code now uses preCalculatedCount directly (with
options.MaxBatches / MaxBatches influencing preCalculatedCount), where a value
of 0 means "no count available"; reference the symbols skipTotalCount,
preCalculatedCount, and options.MaxBatches.HasValue (and align with the note in
Simplified_Progress_Tracking.md about skipTotalCount being removed).
In `@docs/Export_Scripts_Quick_Reference.md`:
- Line 5: The Export_Scripts_Quick_Reference.md currently documents a 10,000 row
batch size in the "Purpose" line (and at the other occurrences), conflicting
with Issue_1716_Export_Final_Specifications.md which specifies 5,000 as the
optimal batch size; verify the actual runtime/configured batch size used by the
export process (check the export job config or constant) and then update
Export_Scripts_Quick_Reference.md to match the verified value (likely change the
"Purpose" and the other three occurrences from 10,000 to 5,000) so both
documents are consistent with Issue_1716_Export_Final_Specifications.md and the
real implementation.
In
`@tools/Altinn.Correspondence.DialogActivityExporter/export-production-1716.ps1`:
- Line 197: The script passes the wrong CLI flag "--fresh-start" which
Program.cs only recognizes as "--fresh" or "-f"; update the PowerShell export
script where $commandArgs is built (the line adding "--fresh-start") to use the
exact supported flag "--fresh" (or "-f") so the exporter actually triggers a
fresh start; ensure any related invocation or documentation in the script uses
the same "--fresh" token to keep behavior consistent with the Program.cs
parsing.
In
`@tools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sql`:
- Around line 9-11: Update the explanatory comments that claim LEFT JOIN on
IdempotencyKeys and use of IsDBNull(1) are the current baseline: change them to
state the exporter already uses INNER JOIN on IdempotencyKeys and that
calculate-counts.sql already removed that LEFT JOIN; remove or rewrite the
recommendation that exporter behavior should be changed (the change is already
applied), and adjust the references at the other spots that mention LEFT
JOIN/IsDBNull (currently lines referencing LEFT JOIN IdempotencyKeys,
IsDBNull(1), calculate-counts.sql) so the script accurately reflects that the
exporter queries use INNER JOIN and no defensive IsDBNull handling is required.
---
Duplicate comments:
In `@docs/database/Configure_PostgreSQL_For_Index_Creation.sql`:
- Around line 170-173: The note under maintenance_work_mem incorrectly suggests
using pg_total_relation_size('pg_class') to check "free RAM"; update the
guidance around maintenance_work_mem to remove that command and instead instruct
operators to verify actual system memory capacity and current utilization using
OS tools or cloud metrics (e.g., free -m, vmstat, top, or cloud provider
instance metrics) and to account for concurrent maintenance operations before
setting maintenance_work_mem = 4GB; reference the existing maintenance_work_mem
guidance and remove or replace the pg_total_relation_size('pg_class') check with
the OS/cloud-based checks.
In `@docs/database/Fix_A2Parties_Recipient_Filter_Index.sql`:
- Around line 49-58: The DO $$ block uses pg_current_xact_id_if_assigned() to
detect transactions which is unreliable; remove the conditional check and
instead replace the guarded warnings with an unconditional reminder that the
script must be run in autocommit mode (no transaction block) before running
CREATE INDEX CONCURRENTLY, or simply remove the DO $$ block entirely so the user
relies on the database's own error; locate the DO $$ ... END $$; block and the
reference to pg_current_xact_id_if_assigned() and update it to a plain advisory
message about autocommit/avoid BEGIN blocks for CREATE INDEX CONCURRENTLY.
In `@docs/database/Helper_Table_Optimization.md`:
- Around line 133-143: The fenced EXPLAIN output block starting with "Execution
Time: 847.773 ms" and containing index names like "IX_A2iss1716corrs_A2_A3" and
"IX_Correspondence_CorrespondenceId_Status" is missing a language tag; update
that fenced code block to include a language specifier (e.g. ```text) so
markdownlint warnings are resolved and rendering is consistent.
In `@docs/database/Index_Creation_Scripts.sql`:
- Around line 380-384: Update the index size comments to match production
measurements: change the individual index estimates for
IX_CorrespondenceStatuses_Status_SyncedTimestamp_Synced from "~1.5 GB" to "3 GB"
and IX_CorrespondenceStatuses_Status_StatusChanged_Migrated from "~12.0 GB" to
"24 GB" (keep the partial index note for
IX_Correspondences_Id_Created_MigrationFilter if unchanged), and update the
"TOTAL NEW INDEX SPACE" comment from "~15 GB" to "~27 GB" so the header and
per-index lines reflect actual production usage recorded earlier in the file.
In `@docs/database/Max_Batches_Feature_Summary.md`:
- Around line 134-139: The docs snippet uses PowerShell's $args variable but the
script uses $commandArgs; update the example to build and pass "--max-batches",
$MaxBatches into the $commandArgs array (replace $args with $commandArgs) so it
matches the implementation and avoids clobbering PowerShell's automatic $args
variable.
- Line 360: Remove or reword the inaccurate sentence "File opens automatically
when done." in the docs (search for the exact phrase "File opens automatically
when done.") and either delete it or replace it with a clarification that the
test export script does not auto-open files and users must open the exported
file manually; ensure the updated line clearly states the actual behavior.
In `@docs/database/Optimize_A2Iss1716A2Events_Indexes.sql`:
- Around line 60-69: The query against pg_stat_user_indexes uses nonexistent
column names tablename and indexname; update the SELECT and WHERE to use the
view's actual column names relname and indexrelname (e.g., select schemaname,
relname, indexrelname, pg_size_pretty(pg_relation_size(indexrelid)) as size,
idx_scan as times_used and filter WHERE schemaname = 'correspondence' AND
relname = 'A2Iss1716A2Events') so the query returns the expected rows; keep the
existing pg_relation_size(indexrelid) and idx_scan references.
- Around line 217-227: The SELECT against pg_stat_user_indexes uses incorrect
column names; update the query to use relname and indexrelname instead of
tablename and indexname when querying the 'correspondence' schema for table
A2Iss1716A2Events so the columns selected (schemaname, relname, indexrelname,
idx_scan, idx_tup_read, idx_tup_fetch) are valid—locate the SQL block querying
pg_stat_user_indexes for schemaname = 'correspondence' AND tablename =
'A2Iss1716A2Events' and replace tablename→relname and indexname→indexrelname
accordingly.
In `@docs/Network_Read_Performance_Issue.md`:
- Around line 138-176: The document has conflicting default batch-size guidance
("Use batch size 1,000", "Test 5,000", and "Reverted default batch size to
5,000"); pick and state a single operational default and make the other
suggestions explicitly experimental. Update the text so the default is clearly
declared (e.g., "Default batch size: 1,000 (proven stable)") and change the
action item referencing "Reverted default batch size to 5,000" to an
experimental note (e.g., "Default remains 1,000; you may test 5,000 using the
'Test 5,000' steps and only switch to 5,000 in production if tests show
acceptable latency"), and ensure the "For Your Current Setup" instructions and
the "Action Items" section both reflect that same single default policy so all
references to "Use batch size 1,000", "Test 5,000", and "Reverted default batch
size to 5,000" are aligned.
In `@docs/Performance_Timing_Analysis_Guide.md`:
- Around line 186-199: Update the Recommendations section so the _batchSize
guidance matches measured results: either replace the blanket "_batchSize =
10000" recommendation with a safe suggested range (e.g., 1,000–5,000) and note
the original test numbers, or add an explicit conditional that "_batchSize =
10000" is only recommended when running from an Azure VM in the same region;
reference the variable name (_batchSize) and the example locations (Program.cs /
DialogActivityExportService.cs) so readers know where to change it, and cite
Network_Read_Performance_Issue.md as the supporting evidence for the change.
In
`@tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs`:
- Line 36: The checkpoint filename currently created in
DialogActivityExportService (variable checkpointPath) uses outputFilePath +
".checkpoint" which doesn't match the production script expectation; change the
code that sets checkpointPath to use outputFilePath + ".checkpoint.json" so the
service writes a .checkpoint.json file (update any references inside
DialogActivityExportService.cs that read/write the checkpoint using
checkpointPath accordingly).
In
`@tools/Altinn.Correspondence.DialogActivityExporter/export-production-1716.ps1`:
- Around line 26-29: Update the BatchSize parameter help text to match the
actual default of 5000 (the value set for the BatchSize parameter in the script)
and align the performance guidance with 5000-batch behavior; specifically modify
the .PARAMETER BatchSize description to state "default: 5000" and adjust the
recommended batch-size guidance/phrasing to reflect the script's 5000 baseline
and appropriate recommendation range.
In `@tools/Altinn.Correspondence.DialogActivityExporter/run-export.ps1`:
- Around line 39-57: Update the help/examples block to reference the correct
script name: replace all occurrences of ".\test-export.ps1" with
".\run-export.ps1" in the run-export.ps1 file so the documented EXAMPLE lines
match the actual script name; ensure each example (the four occurrences shown)
is updated to ".\run-export.ps1" so users won't get file-not-found errors when
following examples in the run-export.ps1 help text.
In `@tools/Altinn.Correspondence.DialogActivityExporter/test-export-2.ps1`:
- Around line 39-57: Update the example help text in test-export-2.ps1 so the
script name referenced matches the actual file: replace occurrences of
".\test-export.ps1" in the EXAMPLE blocks with ".\test-export-2.ps1" (or use a
dynamic reference like $PSCommandPath if you prefer) to prevent users from
copying an incorrect filename; ensure all example lines (the four EXAMPLE
blocks) are updated accordingly.
---
Nitpick comments:
In `@docs/Code_Review_Findings_Resolution.md`:
- Around line 1-196: Add a new unresolved item to the resolution summary noting
the batch-size inconsistency between Export_Scripts_Quick_Reference.md
(documents 10,000) and Issue_1716_Export_Final_Specifications.md (documents
5,000 as optimal): state the conflict, mark it UNRESOLVED/Needs Alignment, and
recommend a follow-up to reconcile the documented default
(Export_Scripts_Quick_Reference.md) with the performance-backed recommendation
in Issue_1716_Export_Final_Specifications.md so owners can decide which value to
standardize on.
In `@docs/database/A2Iss1716A2Events_Helper_Table_Migration.md`:
- Around line 154-218: Add an explicit warning and decision step into the main
migration flow near steps 3–4: explain that even after running ANALYZE on
correspondence."A2Iss1716A2Events" Status 6 queries may still use the wrong
index and remain slow, instruct users to run the provided EXPLAIN (ANALYZE,
BUFFERS) tests for both Status = 4 and Status = 6 before deploying, and if
Status 6 remains slow after a second (cached) run, proactively drop the
competing index IX_A2Iss1716A2Events_Status_CorrId (or follow the
Troubleshooting drop steps) and re-ANALYZE to ensure the planner picks the
intended index.
In `@docs/database/A2Iss1716A2Events_Production_Verification.md`:
- Around line 285-288: The "## Verification Checklist (Updated)" section is
incomplete and duplicates only three items (Memoize Caching, DISTINCT Working,
Performance Target); either expand this section to include the full set of
verification items expected in the updated checklist or remove the "##
Verification Checklist (Updated)" header and its duplicated list to avoid
confusion. Locate the header "## Verification Checklist (Updated)" and either
(a) replace the three bullet items with the complete verification checklist
(same items as the Status 4 checklist plus any truly updated results), ensuring
each item is annotated (✅/❌) and updated metrics are included, or (b) delete the
header and the three bullets so only the original Status 4 checklist remains.
- Around line 21-25: The fenced code blocks in
docs/database/A2Iss1716A2Events_Production_Verification.md (for example the
block containing "Execution Time: 17.258 ms") are missing language specifiers;
update each triple-backtick fence to include a language tag like text or
plaintext (e.g., change ``` to ```text) for the shown block and the other
occurrences called out (lines around 41-47, 56-61, 69-74, 81-89, 98-104,
113-120, 127-131, 209-214, 293-299, 302-308) so markdown linters render them
consistently.
In `@docs/database/Performance_Optimization_Summary.sql`:
- Around line 62-123: The documented "optimized query" diverges from production:
it uses UNION ALL plus LEFT JOIN to correspondence."IdempotencyKeys" while
Test_Export_Query.sql and DialogActivityExportService run two separate SELECTs
(Status=4 and Status=6) each using an INNER JOIN to
correspondence."IdempotencyKeys" and then merge results in C#; update the docs
to match production by replacing the UNION ALL/LEFT JOIN shape with two separate
SELECT statements (one filtering stats."Status" = 4 and one for stats."Status" =
6) that INNER JOIN to correspondence."IdempotencyKeys" (idcFetch/idcConfirm) or
alternatively add a clear note that this is a simplified variant and point
readers to Test_Export_Query.sql and DialogActivityExportService for the exact
production query shape.
In `@tools/Altinn.Correspondence.DialogActivityExporter/calculate-counts.sql`:
- Around line 15-21: Centralize the cutoff timestamp by declaring a single
variable/CTE (e.g., cutoff_timestamp) at the top of calculate-counts.sql and
reference that identifier in all four queries that feed PreCalculatedCounts
instead of repeating the literal timestamp; update the WHERE clauses in each
query to use cutoff_timestamp and remove the duplicate hard-coded timestamps so
a single edit updates all counts consistently.
In
`@tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs`:
- Line 551: The code unconditionally calls
cmd.Parameters.AddWithValue("cutoffTimestamp", cutoffTimestamp) even though the
SQL for issue 1716 doesn't reference `@cutoffTimestamp`; remove that unconditional
binding and only add the cutoffTimestamp parameter where the SQL actually uses
it (e.g., in the branch that builds the issue 1951 query). Locate the parameter
addition in DialogActivityExportService.cs (the cmd variable and cutoffTimestamp
local) and move or guard the AddWithValue call so it runs only when the
constructed command text includes `@cutoffTimestamp` or when handling issue 1951.
In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs`:
- Around line 427-432: The connection string assembly in Program.cs (variable
connectionString) hard-codes Azure host, database and the username domain;
change it to accept the host, database and username domain from configuration or
CLI flags (e.g., add optional parameters like --host, --database, --user-domain
or read from environment/appsettings) and default to the current production
values when not supplied, then build connectionString using those variables so
the tool can target staging/test environments without code changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e467bac-9a14-4cc8-878a-91d4dd691470
📒 Files selected for processing (45)
docs/CRITICAL_FIX_Cursor_Pagination_Index_Issue.mddocs/Code_Review_Findings_Resolution.mddocs/DialogActivityExport_CombinedMode.mddocs/DialogActivityExport_Summary.mddocs/Export_Scripts_Quick_Reference.mddocs/Issue_1716_Export_Final_Specifications.mddocs/Network_Read_Performance_Issue.mddocs/Performance_Timing_Analysis_Guide.mddocs/database/A2Iss1716A2Events_Helper_Table_Migration.mddocs/database/A2Iss1716A2Events_Production_Verification.mddocs/database/Azure_Identity_Migration_Summary.mddocs/database/Check_Disk_Space_And_Table_Stats.sqldocs/database/Configure_PostgreSQL_For_Index_Creation.sqldocs/database/Fix_A2Parties_Recipient_Filter_Index.sqldocs/database/Fix_A2Parties_Recipient_Filter_Schema.sqldocs/database/Helper_Table_Optimization.mddocs/database/Index_Creation_Production_Summary.mddocs/database/Index_Creation_Scripts.sqldocs/database/Max_Batches_Feature_Summary.mddocs/database/Optimize_A2Iss1716A2Events_Indexes.sqldocs/database/Performance_Optimization_Summary.sqldocs/database/Query_Logging_Feature_Summary.mddocs/database/Query_Performance_Notes.mddocs/database/Quick_Test_Reference.mddocs/database/README.mddocs/database/Resumable_Export_Feature.mddocs/database/Status_6_Performance_Issue_Quick_Ref.mddocs/database/Test_Export_Query.sqldocs/database/Testing_Documentation_Summary.mddocs/database/Testing_Guide.mdtools/Altinn.Correspondence.DialogActivityExporter/Altinn.Correspondence.DialogActivityExporter.csprojtools/Altinn.Correspondence.DialogActivityExporter/CHANGELOG.mdtools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cstools/Altinn.Correspondence.DialogActivityExporter/Pre_Calculated_Counts_Feature.mdtools/Altinn.Correspondence.DialogActivityExporter/Program.cstools/Altinn.Correspondence.DialogActivityExporter/README.mdtools/Altinn.Correspondence.DialogActivityExporter/Simplified_Progress_Tracking.mdtools/Altinn.Correspondence.DialogActivityExporter/appsettings.jsontools/Altinn.Correspondence.DialogActivityExporter/appsettings.local.example.jsontools/Altinn.Correspondence.DialogActivityExporter/calculate-counts.sqltools/Altinn.Correspondence.DialogActivityExporter/export-production-1716.ps1tools/Altinn.Correspondence.DialogActivityExporter/run-export.ps1tools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sqltools/Altinn.Correspondence.DialogActivityExporter/test-export-2.ps1tools/Altinn.Correspondence.DialogActivityExporter/test-export.ps1
✅ Files skipped from review due to trivial changes (12)
- tools/Altinn.Correspondence.DialogActivityExporter/appsettings.local.example.json
- docs/database/Check_Disk_Space_And_Table_Stats.sql
- docs/database/Azure_Identity_Migration_Summary.md
- docs/database/Query_Performance_Notes.md
- docs/database/Quick_Test_Reference.md
- tools/Altinn.Correspondence.DialogActivityExporter/Pre_Calculated_Counts_Feature.md
- docs/database/Testing_Guide.md
- tools/Altinn.Correspondence.DialogActivityExporter/CHANGELOG.md
- docs/database/Testing_Documentation_Summary.md
- docs/DialogActivityExport_Summary.md
- docs/DialogActivityExport_CombinedMode.md
- docs/database/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/Altinn.Correspondence.DialogActivityExporter/appsettings.json
- tools/Altinn.Correspondence.DialogActivityExporter/Altinn.Correspondence.DialogActivityExporter.csproj
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/Altinn.Correspondence.DialogActivityExporter/Program.cs (2)
384-455: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftHardcoded production database hostname and domain suffix.
Lines 429-432 contain hardcoded production infrastructure details (
altinn-corr-prod-dbserver.postgres.database.azure.comand@ai-dev.no). This couples the tool to a specific environment, prevents use in staging/dev, and embeds potentially sensitive infrastructure names.Consider reading these from configuration (
appsettings.json) or environment variables.🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs` around lines 384 - 455, The TryBuildAzureConnectionAsync method currently hardcodes production values in the connectionString (Host "altinn-corr-prod-dbserver.postgres.database.azure.com" and username suffix "`@ai-dev.no`"); change it to read host, port, database, and username domain suffix from configuration or environment variables (e.g., via IConfiguration or Environment.GetEnvironmentVariable) and use those values when building connectionString and Username so the function can target dev/staging/prod without embedding sensitive infra names; update references to connectionString construction and the Username usage in TryBuildAzureConnectionAsync to use the configurable variables and provide sensible defaults or fail fast with a clear log if required values are missing.
150-179:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
FreshStartoption is ignored when exporting both issues.
ExportBothToCSVAsync(line 157) doesn't receive or propagateoptions.FreshStart, so users cannot force a fresh start when using--issue all. This inconsistency may confuse users expecting--freshto work with combined exports.🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs` around lines 150 - 179, ExportBothToCSVAsync is currently called without the options.FreshStart flag so the --fresh behavior is ignored; update the call to exportService.ExportBothToCSVAsync(...) to include options.FreshStart (matching the ExportToCSVAsync parameter list) and then update the ExportBothToCSVAsync method signature and its internal logic to accept and honor the FreshStart boolean (same handling as ExportToCSVAsync) so combined exports respect a fresh start.
♻️ Duplicate comments (12)
tools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sql (2)
262-277:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAction items still recommend already-applied join/null-handling changes.
Line 262-277 still instruct changing to
INNER JOINand removing null handling, while the same script states those changes are already in place (Line 9-14). This is stale operational guidance and can trigger unnecessary churn.🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sql` around lines 262 - 277, The commented action items lines recommending switching to INNER JOIN and removing null handling are stale because those changes are already applied earlier in the script; remove or update the block that mentions Test_Export_Query.sql, DialogActivityExportService.cs, calculate-counts.sql, and the IsDBNull(1) check (the lines instructing INNER JOIN and removal of null checks) so the file no longer advises reapplying already-implemented changes and instead documents the current recommended state or a single, accurate decision tree for join/null handling.
51-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStatus-6 path is still untested while conclusions are applied globally.
Line 235 onward drives decisions for exporter/count query structure, but all measured queries are constrained to
Status = 4. That can mislead production tuning for theStatus = 6path.Suggested adjustment
-WHERE stats."Status" = 4 +WHERE stats."Status" IN (4, 6)Or duplicate each block with explicit
Status = 6labels so timings remain comparable per status.Also applies to: 73-74, 118-119, 150-151, 179-180, 221-222, 235-256
🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sql` around lines 51 - 52, Multiple measured queries in the test-count-performance.sql only use WHERE stats."Status" = 4, so the Status = 6 path is untested and comparisons are misleading; update each measured block (the query blocks currently constrained by WHERE stats."Status" = 4 at occurrences around the noted ranges) to also run the equivalent query for WHERE stats."Status" = 6 — either duplicate each block and label the duplicated block clearly as Status = 6 or parametrize the query to iterate statuses [4,6] so timings are captured separately for each status; ensure the duplicated/parametrized blocks preserve identical filters and timing instrumentation so results between Status = 4 and Status = 6 remain directly comparable.docs/database/Configure_PostgreSQL_For_Index_Creation.sql (1)
171-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncorrect RAM check guidance persists.
Line 172 suggests using
SELECT pg_size_pretty(pg_total_relation_size('pg_class'));to check "free RAM," butpg_total_relation_sizereports on-disk relation size (table/index/TOAST footprint), not available server memory. This cannot be used to determine whethermaintenance_work_mem = 4GBis safe. Please replace with guidance to check actual instance memory capacity (e.g., Azure Database for PostgreSQL instance tier/memory allocation, or OS tools likefree -mif shell access is available), or at minimum clarify that this SQL reports disk usage, not RAM availability.🤖 Prompt for 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. In `@docs/database/Configure_PostgreSQL_For_Index_Creation.sql` around lines 171 - 173, Replace the incorrect guidance that suggests using SELECT pg_size_pretty(pg_total_relation_size('pg_class')); to check "free RAM" before setting maintenance_work_mem = 4GB; instead clarify that pg_total_relation_size reports on-disk relation size (not available memory) and instruct readers to check instance memory capacity via their cloud DB instance configuration (e.g., Azure Database for PostgreSQL tier/memory allocation) or OS tools like free -m/top if they have shell access, and make a short, explicit note next to maintenance_work_mem = 4GB advising to confirm actual RAM before increasing the setting.docs/Performance_Timing_Analysis_Guide.md (1)
188-199:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBatch size recommendation conflicts with validated safe range.
Lines 188-199 recommend
_batchSize = 10000, butNetwork_Read_Performance_Issue.mddocuments that 10,000-row batches cause catastrophic performance degradation (38.7 hours vs 37 minutes with 1,000 rows), andIssue_1716_Export_Final_Specifications.mdvalidates 5,000 as the optimal size. Please update this guidance to align with the validated batch size (5,000) or add conditional language explaining that 10,000 is only suitable for same-region execution with sufficient network throughput.🤖 Prompt for 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. In `@docs/Performance_Timing_Analysis_Guide.md` around lines 188 - 199, Update the guidance that sets private const int _batchSize in Program.cs or DialogActivityExportService.cs: don’t recommend 10000 unconditionally—change the recommended value to 5000 (the validated optimal) or make the recommendation conditional (state that _batchSize=10000 is only for same-region execution with verified network throughput and add a note referencing Network_Read_Performance_Issue.md and Issue_1716_Export_Final_Specifications.md). Ensure the doc text around the _batchSize example explicitly states which environment/throughput constraints must be met if suggesting 10000, and replace the unconditional claim about 10x round-trip reduction with the caveat or the 5000-based estimate.docs/Network_Read_Performance_Issue.md (1)
140-143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConflicting default batch size guidance remains unresolved.
Line 140 recommends "Use batch size 1,000 (proven 37 minutes)" as the safe choice, but line 172 states "Reverted default batch size to 5,000". This creates confusion about which value is the actual operational default. Please choose one as the default recommendation and explicitly label the other as "experimental" or "for testing only" to avoid ambiguity.
Also applies to: 172-176
🤖 Prompt for 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. In `@docs/Network_Read_Performance_Issue.md` around lines 140 - 143, Choose a single operational default batch size and update the doc text so it is unambiguous: decide whether "Use batch size 1,000 (proven 37 minutes)" or "Reverted default batch size to 5,000" is the official default, change that statement to read explicitly as the default (e.g., "Default batch size: 1,000"), and mark the other value clearly as "Experimental — for testing only" or "Alternative (not default)" wherever it appears so readers aren’t confused; ensure both occurrences referencing 1,000 and 5,000 are updated to follow this convention.docs/database/Fix_A2Parties_Recipient_Filter_Index.sql (1)
53-57:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
pg_current_xact_id_if_assigned()is not a reliable transaction-block detector here.Line 53 can still be
NULLinside an explicitBEGINuntil an XID is assigned, so this warning can be skipped even thoughCREATE INDEX CONCURRENTLYwill still fail. Make this an unconditional autocommit reminder (or remove the conditional guard).🤖 Prompt for 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. In `@docs/database/Fix_A2Parties_Recipient_Filter_Index.sql` around lines 53 - 57, The conditional check using pg_current_xact_id_if_assigned() is unreliable as a transaction-block detector; remove the IF ... END IF guard around the warnings and make the autocommit reminder unconditional (or simply delete the conditional and always RAISE WARNING lines) so the messages about "You are inside a transaction block!", "CREATE INDEX CONCURRENTLY requires autocommit mode." and "If the next command fails, exit transaction and run again." are always emitted before running CREATE INDEX CONCURRENTLY.docs/database/Performance_Optimization_Summary.sql (1)
62-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis “production” query shape still diverges from exporter behavior.
Using
UNION ALLplusLEFT JOINonIdempotencyKeyschanges result semantics and plan shape versus the exporter’s documented path, so the timing guidance here is not representative.🤖 Prompt for 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. In `@docs/database/Performance_Optimization_Summary.sql` around lines 62 - 124, The current SQL uses UNION ALL plus LEFT JOIN to correspondence."IdempotencyKeys", which changes result semantics and execution plan compared to the exporter path; update the query to match exporter behavior by removing the UNION ALL+LEFT JOIN pattern and instead implement the exporter-aligned logic (either run the two status queries separately or rewrite each SELECT to join idempotency keys the same way the exporter does—e.g., use an INNER JOIN or an EXISTS/INNER JOIN on correspondence."IdempotencyKeys" with the same "StatusAction" filters) for the blocks that reference correspondence."CorrespondenceStatuses", correspondence."ExternalReferences" (er), and correspondence."IdempotencyKeys" (idcFetch/idcConfirm) so the plan shape and semantics mirror the exporter.docs/database/A2Iss1716A2Events_Production_Verification.md (1)
5-8:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeployment readiness is contradictory in the same document.
The doc says “Ready for full export deployment” while also documenting unresolved Status 6 remediation steps. Please provide one explicit final go/no-go state tied to required actions.
Also applies to: 206-249, 366-368
🤖 Prompt for 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. In `@docs/database/A2Iss1716A2Events_Production_Verification.md` around lines 5 - 8, The document currently contains conflicting deployment signals: the checklist states “Ready for full export deployment” while there are unresolved “Status 6 remediation” steps; reconcile by replacing the ambiguous final statement with a single explicit go/no-go decision and required actions—locate the checklist entries (the lines containing “Ready for full export deployment” and the bullet items about performance/EXPLAIN ANALYZE) and the section titled or containing “Status 6 remediation,” then (1) set a definitive status label (“GO — proceed to export” or “NO-GO — do not export”), (2) enumerate the blocking remediation tasks that must be completed before switching to GO (include owners and acceptance criteria), and (3) update any other instances that repeat the contradictory message so the document consistently reflects the chosen go/no-go and required actions.docs/database/Helper_Table_Optimization.md (2)
181-189:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCursor predicate is incompatible with ORDER BY columns.
The query orders by
helper."Altinn2CorrespondenceId"(INT) andstats."Status"(line 116), but the cursor predicate at line 185 compares(stats."CorrespondenceId", stats."Status")whereCorrespondenceIdis a UUID. These are different columns with different sort orders.This mismatch will cause incorrect pagination: the cursor tries to resume using UUID ordering, but the result set is ordered by A2 INT. This can lead to duplicate or skipped rows across batches.
Solution: Align the cursor predicate with the actual ORDER BY columns. If ordering by
Altinn2CorrespondenceId, the cursor must also compare using that column:AND (helper."Altinn2CorrespondenceId", stats."Status") > (`@lastA2Id`, `@lastStatus`)Or document that the implementation must project
Altinn2CorrespondenceIdinto the cursor state instead of using the UUID-basedCorrespondenceId.🤖 Prompt for 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. In `@docs/database/Helper_Table_Optimization.md` around lines 181 - 189, The cursor predicate is using the wrong column type: replace the tuple comparison that uses stats."CorrespondenceId" (UUID) with the INT helper column used in ORDER BY so the cursor aligns with ordering; update the predicate to compare (helper."Altinn2CorrespondenceId", stats."Status") and ensure the cursor state stores lastA2Id (Altinn2CorrespondenceId) and lastStatus instead of CorrespondenceId, or alternatively document and implement projecting helper."Altinn2CorrespondenceId" into the cursor state wherever the cursor is created/consumed.
133-143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language tag to the EXPLAIN output fence.
The fenced code block containing EXPLAIN ANALYZE results should specify a language (e.g.,
text) for proper rendering and to satisfy markdownlint.📝 Suggested fix
-``` +```text Execution Time: 847.773 ms Buffers: shared hit=11795 read=290🤖 Prompt for 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. In `@docs/database/Helper_Table_Optimization.md` around lines 133 - 143, The fenced code block showing EXPLAIN output in Helper_Table_Optimization.md lacks a language tag; update the triple-backtick fence that wraps the EXPLAIN/ANALYZE output to include a language tag (e.g., change ``` to ```text) so the EXPLAIN results (lines like "Execution Time: 847.773 ms", "Buffers: shared hit=11795 read=290", "-> Parallel Index Only Scan using \"IX_A2iss1716corrs_A2_A3\"", etc.) render correctly and satisfy markdownlint.docs/database/Testing_Guide.md (2)
294-294:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStandardize ActivityType values across documentation.
Line 294 uses
"Read"and"Confirmed"for ActivityType values, but other documentation files (e.g.,Query_Logging_Feature_Summary.mdline 76,Helper_Table_Optimization.md) use"CorrespondenceOpened"and"CorrespondenceConfirmed". Align to the canonical values used in the actual export output for consistency.🤖 Prompt for 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. In `@docs/database/Testing_Guide.md` at line 294, Update the ActivityType strings used in the docs to match the canonical export values: replace "Read" (Status 4) with "CorrespondenceOpened" and "Confirmed" (Status 6) with "CorrespondenceConfirmed" so the ActivityType and status mapping in Testing_Guide.md matches the actual export and other docs (e.g., Query_Logging_Feature_Summary.md and Helper_Table_Optimization.md); ensure the text around the ActivityType and status mentions (ActivityType, Status 4, Status 6) is updated accordingly for consistency.
137-137:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove inaccurate "offers to open CSV file" claim.
Line 137 states the script "Offers to open CSV file after completion," but
test-export.ps1does not implement this feature. It only displays file size and row count. Remove this line or update it to reflect actual behavior (e.g., "Displays output file path for manual access").🤖 Prompt for 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. In `@docs/database/Testing_Guide.md` at line 137, The documentation line claiming the script "Offers to open CSV file after completion" is inaccurate; update docs/database/Testing_Guide.md to remove or replace that bullet with the actual behavior of test-export.ps1 (for example, "Displays output file path, file size, and row count for manual access") and ensure the text references the test-export.ps1 script by name so readers aren't misled.
🧹 Nitpick comments (6)
tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs (3)
177-180: 💤 Low valueRemove unreachable dead code.
The
batchCount == 0check at lines 179-180 is unreachable becausebatchCount == 0already causes abreakat line 125. This dead code block will never execute.Proposed fix
- // Continue until we get no more rows (both queries return 0) - // Note: batch count can be up to 2x batchSize (batchSize from each status) - if (batchCount == 0) - break; - // Check if we've reached the max batch limit (test mode)🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs` around lines 177 - 180, In DialogActivityExportService (method in DialogActivityExportService.cs) remove the redundant final check "if (batchCount == 0) break;" because batchCount == 0 is already handled earlier in the loop (causing an immediate break), making this block unreachable; simply delete that unreachable if-statement so the loop contains only meaningful logic.
353-356: 💤 Low valueRemove unreachable dead code.
Same issue:
batchCount == 0check at lines 354-355 is unreachable since the loop already breaks at line 321 whenbatchCount == 0.🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs` around lines 353 - 356, In DialogActivityExportService.cs inside the export loop in the DialogActivityExportService class, remove the redundant "if (batchCount == 0) break;" check that follows the loop body (the unreachable dead code referencing batchCount) because the loop already breaks when batchCount == 0 earlier; delete that conditional and any comments relying on it so the code is clean and has no unreachable branches.
629-640: ⚡ Quick winUnused parameter added to command for Issue 1716 query.
The
cutoffTimestampparameter is unconditionally added to the command at line 632, but the Issue 1716 query (lines 548-574) doesn't reference@cutoffTimestamp. While Npgsql tolerates unused parameters, this is misleading and wasteful.Proposed fix
await using var cmd = new NpgsqlCommand(query, connection); - cmd.Parameters.AddWithValue("cutoffTimestamp", cutoffTimestamp); + + // Only add cutoffTimestamp for Issue 1951 (Issue 1716 uses helper table without timestamp filter) + if (issueNumber == 1951) + { + cmd.Parameters.AddWithValue("cutoffTimestamp", cutoffTimestamp); + } // Only add cursor parameter if we have a cursor value🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs` around lines 629 - 640, The NpgsqlCommand in DialogActivityExportService.cs is adding a cutoffTimestamp parameter unconditionally even though the Issue 1716 SQL (used in the same method around the query construction) does not reference `@cutoffTimestamp`; remove the misleading unused parameter by only adding cmd.Parameters.AddWithValue("cutoffTimestamp", cutoffTimestamp) when the active query includes `@cutoffTimestamp` (or when the branch that builds the Issue1716 query requires it), leaving the existing conditional addition of "lastId" and the unconditional "fetchLimit" untouched so parameters match the SQL being executed.tools/Altinn.Correspondence.DialogActivityExporter/test-export.ps1 (1)
129-184: 💤 Low valueRow estimate may be inaccurate.
Line 171-172 estimates rows as
MaxBatches * BatchSize, but each batch can fetch up to2 × BatchSizerows (one batch per status). The displayed estimate could be half the actual exported count.This is minor for a test script but could confuse users verifying output.
🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/test-export.ps1` around lines 129 - 184, The row estimate currently computed in the test script as $estimatedRows = $MaxBatches * $BatchSize underestimates the possible output because each batch can return up to two times the batch size (one per status); update the displayed estimate logic in test-export.ps1 to reflect this by using a max estimate of 2 * $MaxBatches * $BatchSize (or show a range like "$MaxBatches*$BatchSize - 2*$MaxBatches*$BatchSize") and update the message that prints $estimatedRows so it clearly communicates it’s an approximate/min‑max estimate; modify the code that builds $estimatedRows and the Write-Host line referencing it (search for $estimatedRows, $MaxBatches, $BatchSize).tools/Altinn.Correspondence.DialogActivityExporter/run-export.ps1 (1)
169-169: ⚡ Quick winAvoid loading the entire CSV just to count lines.
Line 169 uses
Get-Contentdirectly, which can consume substantial memory for bigger test files.Suggested fix
-Write-Host "Rows (approx): ~$((Get-Content $OutputPath | Measure-Object -Line).Lines - 1)" -ForegroundColor White +$lineCount = [System.IO.File]::ReadLines($OutputPath).Count +Write-Host "Rows (approx): ~$($lineCount - 1)" -ForegroundColor White🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/run-export.ps1` at line 169, The current Write-Host call uses Get-Content $OutputPath which loads the whole file into memory; change it to stream-count lines instead (e.g. use [System.IO.File]::ReadLines($OutputPath).Count - 1 or iterate the stream) so you compute the approximate row count without allocating the entire CSV; update the Write-Host expression that references $OutputPath and the Get-Content call accordingly (keep the same output message format).docs/database/Testing_Documentation_Summary.md (1)
3-3: 💤 Low valueReplace date placeholder with actual date.
Line 3 contains a placeholder date "2024-01-XX". Either fill in the actual completion date or remove the "XX" suffix if the day is not significant.
🤖 Prompt for 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. In `@docs/database/Testing_Documentation_Summary.md` at line 3, Update the date header "## Date: 2024-01-XX" by replacing the placeholder "XX" with the actual day of completion (e.g., "## Date: 2024-01-15") or remove the day portion entirely if the exact day is not significant (e.g., "## Date: 2024-01") so the document no longer contains a placeholder; edit the header line that currently reads "## Date: 2024-01-XX".
🤖 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 `@docs/Code_Review_Second_Pass_Resolution.md`:
- Line 8: Update the section header text "## ✅ Issues Fixed (11 total)" so the
count matches the listed items (15); change the "(11 total)" to "(15 total)" in
the header string to reflect the actual number of items in the list and keep the
section consistent with the summary that reports 15.
In `@docs/CRITICAL_FIX_Cursor_Pagination_Index_Issue.md`:
- Line 36: The two "AFTER (Fixed)" ORDER BY examples conflict: replace the ORDER
BY that uses stats."CorrespondenceId", Status with the same alias used elsewhere
(a2Events."CorrespondenceId", Status) so both fixed examples and the explanatory
text consistently reference a2Events (the table used in the cursor predicate and
the indexed table); update any explanatory lines referring to stats columns to
refer to a2Events to avoid ambiguity.
In `@docs/database/A2Iss1716A2Events_Helper_Table_Migration.md`:
- Around line 330-336: The performance figures in the table rows "Query Time
(per 100 rows)" and "Total Export Time (estimated)" are inconsistent (one shows
~300–350x and another shows 16–24x / 6–10x); update the table so metrics are
aligned and clearly labeled as either per-query vs end-to-end/export-level
improvements (or adjust numbers to be consistent). Locate the two entries "Query
Time (per 100 rows)" and "Total Export Time (estimated)" (and the similar block
referenced at lines 369–373) and either convert the per-query speedup into the
corresponding end-to-end/export-level multiplier or add parenthetical clarifiers
(e.g., "per-query: 300–350x; end-to-end: 16–24x") so the document shows a single
coherent comparison.
In `@docs/database/A2Iss1716A2Events_Production_Verification.md`:
- Around line 21-25: Add explicit fenced code block languages to each
triple-backtick block showing timings (e.g., change ``` to ```text or ```sql) so
Markdown lint (MD040) stops flagging them; update the blocks containing
"Execution Time: 17.258 ms" and all other timing/result blocks with the same
content pattern to use a language specifier (choose "text" for plain output or
"sql" if it’s query output) and keep the block content unchanged.
In `@docs/database/Index_Creation_Scripts.sql`:
- Around line 134-157: The runbook contains conflicting deployment guidance
about the IX_Correspondences_Id_Created_MigrationFilter index vs later sections
that still budget/deploy it; reconcile by choosing one authoritative action
(either "do not create index" or "create index") and update all references
accordingly: remove or convert any executable deployment steps and budget
estimates about that index into a separate "historical analysis" section, or
conversely move the explanatory notes (the performance reasoning referencing
DialogActivityExportService.cs, stats."CorrespondenceId",
corr.Id/Correspondences.Id) into a non-executable appendix while keeping a
single clear, actionable deployment plan; ensure every mention of
IX_Correspondences_Id_Created_MigrationFilter and related savings/risks is
consistent and that any remaining deployment checklist items reflect the chosen
authoritative scope.
In `@docs/database/Max_Batches_Feature_Summary.md`:
- Line 81: Update the fenced code blocks in Max_Batches_Feature_Summary.md to
include language identifiers for consistent rendering: add `text` to the
help-output block at the block currently on line 81, add `bash` or `shell` to
the command example block at line 87, add `text` to the parameter-help block at
line 118, add `text` to the console-output block at line 179, and add `text` or
`pseudocode` to the flow-description block at line 282 so each fenced block
begins with the appropriate language tag.
In `@docs/database/Query_Logging_Feature_Summary.md`:
- Line 53: The two fenced code blocks that show console output (the untagged
triple-backtick blocks near the top and the second example) should be updated to
include a language tag (e.g., change ``` to ```text) so markdownlint and
renderers treat them as plain text; locate the untagged console-output blocks in
Query_Logging_Feature_Summary.md (the console output examples) and add the
language tag to each opening fence.
In `@docs/database/Resumable_Export_Feature.md`:
- Line 110: Update the fenced code block that shows the resume logging messages
in Resumable_Export_Feature.md to include a language tag (e.g., change the
opening fence from ``` to ```text) so the console output renders consistently;
locate the block that contains the resume logging messages and add the `text`
language identifier to the opening fence.
In `@docs/database/Status_6_Performance_Issue_Quick_Ref.md`:
- Line 98: Add a blank line immediately before the table that starts with "|
Metric | Target | Your Result |" so the markdown renderer recognizes it as a
table, and add a language tag (for example, ```text) to the fenced code block
that contains the decision tree referenced around line 184 so the fence is
```text instead of just ```, ensuring consistent rendering.
- Line 42: The ORDER BY clause uses an unqualified column name Status which is
inconsistent with other aliased columns and may be ambiguous; update the ORDER
BY that currently reads ORDER BY stats."CorrespondenceId", Status to qualify the
Status column as stats."Status" so both sort expressions use the stats alias and
consistent quoted identifiers.
In `@docs/database/Testing_Documentation_Summary.md`:
- Line 129: The example includes the deprecated parameter --oldest "2019-03-23";
remove that flag from the production export example or, if you want to keep it
for historical context, replace it with an explicit deprecation note next to
--oldest explaining it is deprecated and should not be used (e.g., mark as
[DEPRECATED] and point to the current recommended alternative). Ensure the
change updates the example line containing --oldest "2019-03-23" and the
adjacent explanatory text so readers are not misled.
In `@docs/Export_Scripts_Quick_Reference.md`:
- Line 138: The expected row count is incorrect: the document currently states
"Expected: 100,000 rows in < 30 seconds" but with the documented 5,000-row batch
size and 10 batches that should be 50,000 rows; update the expected value to
"50,000 rows" (or adjust the batch count text to 20 batches) so the math matches
the documented 5,000-row batch size and 10 batches.
In `@docs/Issue_1716_Export_Final_Specifications.md`:
- Around line 306-309: The current docs show using (Get-Content
"C:\temp\dialog_activity_export_*.csv").Count which loads the entire file into
memory and will fail for ~2GB CSVs; change the example to a non-memory-intensive
approach: either use Get-Item ... .Length to estimate rows (mentioning the
bytes-per-row heuristic and subtracting the header) or show a streaming accurate
count using Get-Content with -ReadCount and aggregating $_.Count, or suggest
using an external line-count tool (e.g., wc -l) for performance; update the
snippet text to replace the direct (Get-Content ...).Count call and reference
these safer alternatives instead.
In
`@tools/Altinn.Correspondence.DialogActivityExporter/CONNECTION_STRING_OPTIMIZATION.md`:
- Around line 13-27: Update
tools/Altinn.Correspondence.DialogActivityExporter/NETWORK_BOTTLENECK_ANALYSIS.md
so the connection-string examples use the correct Npgsql parameter names that
match
tools/Altinn.Correspondence.DialogActivityExporter/CONNECTION_STRING_OPTIMIZATION.md
and the official docs: replace "Max Pool Size" → "MaxPoolSize", "TCP KeepAlive"
→ "Keepalive", "Internal Command Timeout" → "Command Timeout" (or "Command
Timeout" key exactly), and remove the unsupported "No Reset On Close" entry;
alternatively add a short mapping table in NETWORK_BOTTLENECK_ANALYSIS.md that
maps the invalid keys to the valid Npgsql keys (MaxPoolSize, Keepalive, Command
Timeout, Timeout, Read Buffer Size, Write Buffer Size) so users copying examples
get correct parameters.
In
`@tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs`:
- Around line 771-788: The EstimatedTimeRemaining getter in the ExportProgress
record can return a negative TimeSpan when TotalProcessed > TotalCount; update
the EstimatedTimeRemaining logic in ExportProgress to guard against negative
remaining and zero/near-zero rate by computing remaining as Math.Max(0,
TotalCount - TotalProcessed) and returning TimeSpan.Zero if rate is <= 0,
otherwise compute seconds = remaining / rate and return
TimeSpan.FromSeconds(seconds); make the change inside the EstimatedTimeRemaining
property to ensure it never yields a negative TimeSpan.
In
`@tools/Altinn.Correspondence.DialogActivityExporter/export-production-1716.ps1`:
- Around line 147-190: The checkpoint display is referencing non-existent fields
($checkpoint.LastCursorId and $checkpoint.Timestamp); update the resume UI to
read the actual ExportCheckpoint properties (use
$checkpoint.LastStatus4CorrespondenceId or
$checkpoint.LastStatus6CorrespondenceId as appropriate for the checkpointing
strategy, and $checkpoint.CheckpointTime for the timestamp) so the printed "Last
Cursor" and "Timestamp" values show real data; locate the resume block around
the checkpoint handling (variables: $checkpointPath, $isResume, $checkpoint) and
replace the referenced fields accordingly.
- Around line 1-124: The docblock defaults are inconsistent with the script:
change all mentions of "5000" and "5,000" in the header/description to the
actual default BatchSize (2500) and replace the hardcoded static timestamp
example "20260609_125929" in the OutputPath examples with a note that the script
generates a dynamic timestamp when OutputPath is empty (the dynamic timestamp
logic is already implemented where OutputPath is constructed / handled). Update
the parameter descriptions for -BatchSize, the examples that reference "Batch
size: 5,000 rows" and the OutputPath example to reflect Default: 2500 and
dynamic timestamp generation so docs match the behavior of the $BatchSize
default and the OutputPath generation logic.
In `@tools/Altinn.Correspondence.DialogActivityExporter/run-export.ps1`:
- Around line 98-101: The default OutputPath currently hardcodes a Windows-only
path ("C:\temp\..."); update the fallback to build a cross-platform temp file
path using the system temp directory (e.g., use [System.IO.Path]::GetTempPath()
or $env:TEMP) and combine it with your filename instead of a hardcoded drive
letter; construct the filename using $Issue and $timestamp and join via
[System.IO.Path]::Combine or Join-Path so the OutputPath assignment (the block
that sets $timestamp and $OutputPath) works on Windows, Linux and macOS.
In
`@tools/Altinn.Correspondence.DialogActivityExporter/test-distinct-overhead.sql`:
- Around line 7-29: The two COUNTs are not comparable because the DISTINCT query
applies LIMIT 10000 inside its subquery (subq) while the non-DISTINCT query
places LIMIT after COUNT(*) which does nothing; fix by making both sides use
identical row-limiting semantics — e.g., wrap the non-DISTINCT join (the query
using correspondence."A2Iss1716A2Events" a2Events and
correspondence."CorrespondenceStatuses" stats with WHERE a2Events."Status" = 4)
in a subquery and apply LIMIT 10000 before the outer COUNT, or remove the LIMIT
from the DISTINCT path and compare full counts; ensure both queries use the same
aliasing and where/join conditions so COUNT(*) on subq and COUNT(*) on the
non-distinct subquery are computed over the same pre-limit set.
In `@tools/Altinn.Correspondence.DialogActivityExporter/test-export-2.ps1`:
- Around line 1-65: The help comment's .PARAMETER Issue default ("1951") doesn't
match the script's actual default ("1716"); update either the comment or the
actual default so they match—specifically, adjust the Issue parameter default in
the Param block or change the .PARAMETER Issue text to "1716" so the documented
default and the runtime default for the Issue parameter are consistent (look for
the Issue parameter name in the Param() declaration and the .PARAMETER Issue
help section).
---
Outside diff comments:
In `@tools/Altinn.Correspondence.DialogActivityExporter/Program.cs`:
- Around line 384-455: The TryBuildAzureConnectionAsync method currently
hardcodes production values in the connectionString (Host
"altinn-corr-prod-dbserver.postgres.database.azure.com" and username suffix
"`@ai-dev.no`"); change it to read host, port, database, and username domain
suffix from configuration or environment variables (e.g., via IConfiguration or
Environment.GetEnvironmentVariable) and use those values when building
connectionString and Username so the function can target dev/staging/prod
without embedding sensitive infra names; update references to connectionString
construction and the Username usage in TryBuildAzureConnectionAsync to use the
configurable variables and provide sensible defaults or fail fast with a clear
log if required values are missing.
- Around line 150-179: ExportBothToCSVAsync is currently called without the
options.FreshStart flag so the --fresh behavior is ignored; update the call to
exportService.ExportBothToCSVAsync(...) to include options.FreshStart (matching
the ExportToCSVAsync parameter list) and then update the ExportBothToCSVAsync
method signature and its internal logic to accept and honor the FreshStart
boolean (same handling as ExportToCSVAsync) so combined exports respect a fresh
start.
---
Duplicate comments:
In `@docs/database/A2Iss1716A2Events_Production_Verification.md`:
- Around line 5-8: The document currently contains conflicting deployment
signals: the checklist states “Ready for full export deployment” while there are
unresolved “Status 6 remediation” steps; reconcile by replacing the ambiguous
final statement with a single explicit go/no-go decision and required
actions—locate the checklist entries (the lines containing “Ready for full
export deployment” and the bullet items about performance/EXPLAIN ANALYZE) and
the section titled or containing “Status 6 remediation,” then (1) set a
definitive status label (“GO — proceed to export” or “NO-GO — do not export”),
(2) enumerate the blocking remediation tasks that must be completed before
switching to GO (include owners and acceptance criteria), and (3) update any
other instances that repeat the contradictory message so the document
consistently reflects the chosen go/no-go and required actions.
In `@docs/database/Configure_PostgreSQL_For_Index_Creation.sql`:
- Around line 171-173: Replace the incorrect guidance that suggests using SELECT
pg_size_pretty(pg_total_relation_size('pg_class')); to check "free RAM" before
setting maintenance_work_mem = 4GB; instead clarify that pg_total_relation_size
reports on-disk relation size (not available memory) and instruct readers to
check instance memory capacity via their cloud DB instance configuration (e.g.,
Azure Database for PostgreSQL tier/memory allocation) or OS tools like free
-m/top if they have shell access, and make a short, explicit note next to
maintenance_work_mem = 4GB advising to confirm actual RAM before increasing the
setting.
In `@docs/database/Fix_A2Parties_Recipient_Filter_Index.sql`:
- Around line 53-57: The conditional check using
pg_current_xact_id_if_assigned() is unreliable as a transaction-block detector;
remove the IF ... END IF guard around the warnings and make the autocommit
reminder unconditional (or simply delete the conditional and always RAISE
WARNING lines) so the messages about "You are inside a transaction block!",
"CREATE INDEX CONCURRENTLY requires autocommit mode." and "If the next command
fails, exit transaction and run again." are always emitted before running CREATE
INDEX CONCURRENTLY.
In `@docs/database/Helper_Table_Optimization.md`:
- Around line 181-189: The cursor predicate is using the wrong column type:
replace the tuple comparison that uses stats."CorrespondenceId" (UUID) with the
INT helper column used in ORDER BY so the cursor aligns with ordering; update
the predicate to compare (helper."Altinn2CorrespondenceId", stats."Status") and
ensure the cursor state stores lastA2Id (Altinn2CorrespondenceId) and lastStatus
instead of CorrespondenceId, or alternatively document and implement projecting
helper."Altinn2CorrespondenceId" into the cursor state wherever the cursor is
created/consumed.
- Around line 133-143: The fenced code block showing EXPLAIN output in
Helper_Table_Optimization.md lacks a language tag; update the triple-backtick
fence that wraps the EXPLAIN/ANALYZE output to include a language tag (e.g.,
change ``` to ```text) so the EXPLAIN results (lines like "Execution Time:
847.773 ms", "Buffers: shared hit=11795 read=290", "-> Parallel Index Only Scan
using \"IX_A2iss1716corrs_A2_A3\"", etc.) render correctly and satisfy
markdownlint.
In `@docs/database/Performance_Optimization_Summary.sql`:
- Around line 62-124: The current SQL uses UNION ALL plus LEFT JOIN to
correspondence."IdempotencyKeys", which changes result semantics and execution
plan compared to the exporter path; update the query to match exporter behavior
by removing the UNION ALL+LEFT JOIN pattern and instead implement the
exporter-aligned logic (either run the two status queries separately or rewrite
each SELECT to join idempotency keys the same way the exporter does—e.g., use an
INNER JOIN or an EXISTS/INNER JOIN on correspondence."IdempotencyKeys" with the
same "StatusAction" filters) for the blocks that reference
correspondence."CorrespondenceStatuses", correspondence."ExternalReferences"
(er), and correspondence."IdempotencyKeys" (idcFetch/idcConfirm) so the plan
shape and semantics mirror the exporter.
In `@docs/database/Testing_Guide.md`:
- Line 294: Update the ActivityType strings used in the docs to match the
canonical export values: replace "Read" (Status 4) with "CorrespondenceOpened"
and "Confirmed" (Status 6) with "CorrespondenceConfirmed" so the ActivityType
and status mapping in Testing_Guide.md matches the actual export and other docs
(e.g., Query_Logging_Feature_Summary.md and Helper_Table_Optimization.md);
ensure the text around the ActivityType and status mentions (ActivityType,
Status 4, Status 6) is updated accordingly for consistency.
- Line 137: The documentation line claiming the script "Offers to open CSV file
after completion" is inaccurate; update docs/database/Testing_Guide.md to remove
or replace that bullet with the actual behavior of test-export.ps1 (for example,
"Displays output file path, file size, and row count for manual access") and
ensure the text references the test-export.ps1 script by name so readers aren't
misled.
In `@docs/Network_Read_Performance_Issue.md`:
- Around line 140-143: Choose a single operational default batch size and update
the doc text so it is unambiguous: decide whether "Use batch size 1,000 (proven
37 minutes)" or "Reverted default batch size to 5,000" is the official default,
change that statement to read explicitly as the default (e.g., "Default batch
size: 1,000"), and mark the other value clearly as "Experimental — for testing
only" or "Alternative (not default)" wherever it appears so readers aren’t
confused; ensure both occurrences referencing 1,000 and 5,000 are updated to
follow this convention.
In `@docs/Performance_Timing_Analysis_Guide.md`:
- Around line 188-199: Update the guidance that sets private const int
_batchSize in Program.cs or DialogActivityExportService.cs: don’t recommend
10000 unconditionally—change the recommended value to 5000 (the validated
optimal) or make the recommendation conditional (state that _batchSize=10000 is
only for same-region execution with verified network throughput and add a note
referencing Network_Read_Performance_Issue.md and
Issue_1716_Export_Final_Specifications.md). Ensure the doc text around the
_batchSize example explicitly states which environment/throughput constraints
must be met if suggesting 10000, and replace the unconditional claim about 10x
round-trip reduction with the caveat or the 5000-based estimate.
In
`@tools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sql`:
- Around line 262-277: The commented action items lines recommending switching
to INNER JOIN and removing null handling are stale because those changes are
already applied earlier in the script; remove or update the block that mentions
Test_Export_Query.sql, DialogActivityExportService.cs, calculate-counts.sql, and
the IsDBNull(1) check (the lines instructing INNER JOIN and removal of null
checks) so the file no longer advises reapplying already-implemented changes and
instead documents the current recommended state or a single, accurate decision
tree for join/null handling.
- Around line 51-52: Multiple measured queries in the test-count-performance.sql
only use WHERE stats."Status" = 4, so the Status = 6 path is untested and
comparisons are misleading; update each measured block (the query blocks
currently constrained by WHERE stats."Status" = 4 at occurrences around the
noted ranges) to also run the equivalent query for WHERE stats."Status" = 6 —
either duplicate each block and label the duplicated block clearly as Status = 6
or parametrize the query to iterate statuses [4,6] so timings are captured
separately for each status; ensure the duplicated/parametrized blocks preserve
identical filters and timing instrumentation so results between Status = 4 and
Status = 6 remain directly comparable.
---
Nitpick comments:
In `@docs/database/Testing_Documentation_Summary.md`:
- Line 3: Update the date header "## Date: 2024-01-XX" by replacing the
placeholder "XX" with the actual day of completion (e.g., "## Date: 2024-01-15")
or remove the day portion entirely if the exact day is not significant (e.g.,
"## Date: 2024-01") so the document no longer contains a placeholder; edit the
header line that currently reads "## Date: 2024-01-XX".
In
`@tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs`:
- Around line 177-180: In DialogActivityExportService (method in
DialogActivityExportService.cs) remove the redundant final check "if (batchCount
== 0) break;" because batchCount == 0 is already handled earlier in the loop
(causing an immediate break), making this block unreachable; simply delete that
unreachable if-statement so the loop contains only meaningful logic.
- Around line 353-356: In DialogActivityExportService.cs inside the export loop
in the DialogActivityExportService class, remove the redundant "if (batchCount
== 0) break;" check that follows the loop body (the unreachable dead code
referencing batchCount) because the loop already breaks when batchCount == 0
earlier; delete that conditional and any comments relying on it so the code is
clean and has no unreachable branches.
- Around line 629-640: The NpgsqlCommand in DialogActivityExportService.cs is
adding a cutoffTimestamp parameter unconditionally even though the Issue 1716
SQL (used in the same method around the query construction) does not reference
`@cutoffTimestamp`; remove the misleading unused parameter by only adding
cmd.Parameters.AddWithValue("cutoffTimestamp", cutoffTimestamp) when the active
query includes `@cutoffTimestamp` (or when the branch that builds the Issue1716
query requires it), leaving the existing conditional addition of "lastId" and
the unconditional "fetchLimit" untouched so parameters match the SQL being
executed.
In `@tools/Altinn.Correspondence.DialogActivityExporter/run-export.ps1`:
- Line 169: The current Write-Host call uses Get-Content $OutputPath which loads
the whole file into memory; change it to stream-count lines instead (e.g. use
[System.IO.File]::ReadLines($OutputPath).Count - 1 or iterate the stream) so you
compute the approximate row count without allocating the entire CSV; update the
Write-Host expression that references $OutputPath and the Get-Content call
accordingly (keep the same output message format).
In `@tools/Altinn.Correspondence.DialogActivityExporter/test-export.ps1`:
- Around line 129-184: The row estimate currently computed in the test script as
$estimatedRows = $MaxBatches * $BatchSize underestimates the possible output
because each batch can return up to two times the batch size (one per status);
update the displayed estimate logic in test-export.ps1 to reflect this by using
a max estimate of 2 * $MaxBatches * $BatchSize (or show a range like
"$MaxBatches*$BatchSize - 2*$MaxBatches*$BatchSize") and update the message that
prints $estimatedRows so it clearly communicates it’s an approximate/min‑max
estimate; modify the code that builds $estimatedRows and the Write-Host line
referencing it (search for $estimatedRows, $MaxBatches, $BatchSize).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0abd2564-edb1-4925-a7e2-42b6f33ae13f
📒 Files selected for processing (53)
docs/CRITICAL_FIX_Cursor_Pagination_Index_Issue.mddocs/Code_Review_Findings_Resolution.mddocs/Code_Review_Second_Pass_Resolution.mddocs/DialogActivityExport_CombinedMode.mddocs/DialogActivityExport_Summary.mddocs/Export_Scripts_Quick_Reference.mddocs/Issue_1716_Export_Final_Specifications.mddocs/Network_Read_Performance_Issue.mddocs/Performance_Timing_Analysis_Guide.mddocs/database/A2Iss1716A2Events_Helper_Table_Migration.mddocs/database/A2Iss1716A2Events_Production_Verification.mddocs/database/Azure_Identity_Migration_Summary.mddocs/database/Check_Disk_Space_And_Table_Stats.sqldocs/database/Configure_PostgreSQL_For_Index_Creation.sqldocs/database/Fix_A2Parties_Recipient_Filter_Index.sqldocs/database/Fix_A2Parties_Recipient_Filter_Schema.sqldocs/database/Helper_Table_Optimization.mddocs/database/Index_Creation_Production_Summary.mddocs/database/Index_Creation_Scripts.sqldocs/database/Max_Batches_Feature_Summary.mddocs/database/Optimize_A2Iss1716A2Events_Indexes.sqldocs/database/Performance_Optimization_Summary.sqldocs/database/Query_Logging_Feature_Summary.mddocs/database/Query_Performance_Notes.mddocs/database/Quick_Test_Reference.mddocs/database/README.mddocs/database/Resumable_Export_Feature.mddocs/database/Status_6_Performance_Issue_Quick_Ref.mddocs/database/Test_Export_Query.sqldocs/database/Testing_Documentation_Summary.mddocs/database/Testing_Guide.mdtools/Altinn.Correspondence.DialogActivityExporter/Altinn.Correspondence.DialogActivityExporter.csprojtools/Altinn.Correspondence.DialogActivityExporter/CHANGELOG.mdtools/Altinn.Correspondence.DialogActivityExporter/CONNECTION_STRING_OPTIMIZATION.mdtools/Altinn.Correspondence.DialogActivityExporter/CURSOR_MECHANISM_COMPARISON.mdtools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cstools/Altinn.Correspondence.DialogActivityExporter/NETWORK_BOTTLENECK_ANALYSIS.mdtools/Altinn.Correspondence.DialogActivityExporter/Pre_Calculated_Counts_Feature.mdtools/Altinn.Correspondence.DialogActivityExporter/Program.cstools/Altinn.Correspondence.DialogActivityExporter/README.mdtools/Altinn.Correspondence.DialogActivityExporter/SEPARATE_CURSORS_IMPLEMENTATION.mdtools/Altinn.Correspondence.DialogActivityExporter/Simplified_Progress_Tracking.mdtools/Altinn.Correspondence.DialogActivityExporter/appsettings.jsontools/Altinn.Correspondence.DialogActivityExporter/appsettings.local.example.jsontools/Altinn.Correspondence.DialogActivityExporter/calculate-counts.sqltools/Altinn.Correspondence.DialogActivityExporter/diagnose-query-performance.sqltools/Altinn.Correspondence.DialogActivityExporter/export-production-1716.ps1tools/Altinn.Correspondence.DialogActivityExporter/find-duplicate-source.sqltools/Altinn.Correspondence.DialogActivityExporter/run-export.ps1tools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sqltools/Altinn.Correspondence.DialogActivityExporter/test-distinct-overhead.sqltools/Altinn.Correspondence.DialogActivityExporter/test-export-2.ps1tools/Altinn.Correspondence.DialogActivityExporter/test-export.ps1
✅ Files skipped from review due to trivial changes (12)
- tools/Altinn.Correspondence.DialogActivityExporter/appsettings.local.example.json
- docs/database/Check_Disk_Space_And_Table_Stats.sql
- docs/database/Query_Performance_Notes.md
- docs/database/Azure_Identity_Migration_Summary.md
- tools/Altinn.Correspondence.DialogActivityExporter/diagnose-query-performance.sql
- tools/Altinn.Correspondence.DialogActivityExporter/Pre_Calculated_Counts_Feature.md
- docs/database/Index_Creation_Production_Summary.md
- docs/DialogActivityExport_Summary.md
- tools/Altinn.Correspondence.DialogActivityExporter/CHANGELOG.md
- docs/database/Quick_Test_Reference.md
- docs/DialogActivityExport_CombinedMode.md
- docs/database/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/Altinn.Correspondence.DialogActivityExporter/Altinn.Correspondence.DialogActivityExporter.csproj
- tools/Altinn.Correspondence.DialogActivityExporter/appsettings.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
♻️ Duplicate comments (22)
docs/database/Helper_Table_Optimization.md (2)
133-143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language tag to the EXPLAIN block.
This fenced output block still triggers markdownlint;
textis enough here.🤖 Prompt for 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. In `@docs/database/Helper_Table_Optimization.md` around lines 133 - 143, The fenced code block containing the EXPLAIN output is missing a language identifier tag after the opening triple backticks. Add the language tag `text` immediately after the opening triple backticks (change the opening from ``` to ```text) to properly format the markdown block and satisfy the markdownlint requirements.Source: Linters/SAST tools
183-188:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRewrite the cursor paragraph to match the actual batch key.
The exporter’s helper-table path advances with
CorrespondenceId >@lastId``; it does not use a UUID tuple cursor. Please describe the real scan key here so readers do not copy the wrong pagination rule.🤖 Prompt for 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. In `@docs/database/Helper_Table_Optimization.md` around lines 183 - 188, In the cursor paragraph around lines 183-188, replace the incorrect description of the UUID-based tuple cursor with an accurate explanation of the actual pagination mechanism. The exporter uses a simple CorrespondenceId-based cursor advancement with CorrespondenceId > `@lastId`, not a composite tuple comparison with Status. Rewrite this section to clearly document the real batch key used for scanning the helper table so readers understand the correct pagination approach.docs/database/Testing_Guide.md (2)
132-138:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the auto-open claim.
test-export.ps1still does not open the CSV automatically; it only reports the output details after completion.🤖 Prompt for 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. In `@docs/database/Testing_Guide.md` around lines 132 - 138, The Script Features list in the documentation contains a claim that the script "Offers to open CSV file after completion" but this functionality does not actually exist in test-export.ps1. Remove the bullet point about offering to open the CSV file after completion from the documentation, as the script only reports output details without automatically opening the file.
97-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the output examples to the exporter contract.
This file still mixes
Read/Confirmedwith aSyncedFromAltinn2timestamp, but the exporter writesStatusChangedfor the timestamp and uses the canonical ActivityType names elsewhere. Please update the sample row, manual SQL CASE, and column description together.Also applies to: 247-256, 281-295
🤖 Prompt for 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. In `@docs/database/Testing_Guide.md` around lines 97 - 101, The expected output examples in the Testing_Guide.md file are inconsistent with the actual exporter contract. Update the sample output row (showing ActivityType as "Read") and all related documentation to use the canonical ActivityType names that the exporter actually produces instead of mixing Read/Confirmed values. Additionally, ensure the timestamp column description and any manual SQL CASE statements in the sections referenced at lines 247-256 and 281-295 accurately reflect what the exporter writes, using the canonical ActivityType values and correct timestamp field names throughout all three sections for consistency.docs/Code_Review_Second_Pass_Resolution.md (1)
8-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the section count.
The heading says 11, but the list contains 15 items.
🤖 Prompt for 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. In `@docs/Code_Review_Second_Pass_Resolution.md` at line 8, Update the count in the heading "Issues Fixed (11 total)" to reflect the actual number of items listed below. Count all the items in the Issues Fixed list and replace the number 11 with the correct total count of 15 to ensure the heading accurately represents the list size.docs/CRITICAL_FIX_Cursor_Pagination_Index_Issue.md (1)
28-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the “AFTER” example with the cursor fix.
The fixed snippet still orders by
stats."CorrespondenceId"even though the cursor predicate now usesa2Events. That keeps the “fixed” example contradictory and reintroduces the same confusion as the previous review.🤖 Prompt for 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. In `@docs/CRITICAL_FIX_Cursor_Pagination_Index_Issue.md` around lines 28 - 43, The "AFTER" example demonstrates a cursor predicate that correctly references a2Events columns for index usage, but the ORDER BY clause still references stats columns, creating an internal contradiction. Update the ORDER BY clause to reference a2Events."CorrespondenceId" and a2Events."Status" instead of stats columns to make the fixed example consistent with the cursor predicate. This alignment will eliminate the confusion and clearly show that both the cursor predicate and ORDER BY clause reference the indexed table a2Events.docs/Performance_Timing_Analysis_Guide.md (1)
188-199:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t recommend
_batchSize = 10000on the slow path.This section still tells readers to move to 10,000 rows even though the other docs in this PR show that 10k is the pathological case over the current cross-region path. If this is meant for same-region/Azure VM runs, say so explicitly; otherwise keep it in the validated 1k–5k range.
🤖 Prompt for 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. In `@docs/Performance_Timing_Analysis_Guide.md` around lines 188 - 199, The batch size recommendation in the "Immediate (Before Next Test)" section recommends increasing _batchSize to 10000, but this configuration is only appropriate for same-region or Azure VM deployments and is actually pathological for cross-region scenarios. Add an explicit conditional note clarifying that the 10000 batch size is only recommended when running from an Azure VM in the same region as the database, and for cross-region deployments the batch size should remain in the validated 1k-5k range. Alternatively, if this guidance is meant to be universally applied, reduce the recommended batch size back to the validated 1k-5k range.docs/database/Max_Batches_Feature_Summary.md (1)
307-323:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the stale COUNT-skip example.
The technical-details block still shows
skipTotalCountandoldestCorrespondenceDate, but the summary above says both were removed in favor of directpreCalculatedCounthandling. This reintroduces the old flow into the docs and will confuse anyone following the current implementation.🤖 Prompt for 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. In `@docs/database/Max_Batches_Feature_Summary.md` around lines 307 - 323, The COUNT(*) Skip Logic code example showing skipTotalCount and oldestCorrespondenceDate is stale documentation that contradicts the summary statement above it which indicates both were removed in favor of direct preCalculatedCount handling. Remove the entire COUNT(*) Skip Logic section with the csharp code block containing skipTotalCount logic and GetTotalCountAsync calls to eliminate the discrepancy between the documented approach and the actual current implementation.docs/Network_Read_Performance_Issue.md (1)
90-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winChoose one default batch size and keep the estimates aligned.
This doc still flips between
1,000and5,000as the recommended default, and the5,000-row estimate also jumps from1 hour?to16.6 minutes. Pick one target for the current setup, mark the other as experimental, and update the projections so the guidance is actionable.Also applies to: 138-176
🤖 Prompt for 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. In `@docs/Network_Read_Performance_Issue.md` around lines 90 - 115, The documentation inconsistently recommends multiple batch sizes with conflicting performance estimates. The table shows a "1 hour?" estimate for 5,000 rows while the testing recommendations later calculate "16.6 minutes" for the same batch size. Select a single batch size as the recommended default for the current setup (either 1,000 or 5,000), clearly mark alternative batch sizes as experimental or future testing, and update all performance estimates in both the comparison table and the testing recommendations section to be internally consistent and aligned with the chosen default. Ensure the projections throughout the document match the selected batch size so the guidance is actionable.docs/Export_Scripts_Quick_Reference.md (1)
5-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the row-count math in the quick-reference.
Several examples still disagree on the same calculation:
2 batches × 5,000 rowsis10,000total, not5,000, and the later 10-batch example should be50,000, not100,000. Please align the purpose text, sample output, workflow example, and summary table to one consistent total.Also applies to: 26-26, 38-40, 138-140, 306-309
🤖 Prompt for 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. In `@docs/Export_Scripts_Quick_Reference.md` at line 5, The row-count calculations in the quick-reference document contain mathematical errors that are inconsistent across multiple sections. Correct the calculation for the default scenario from "2 batches × 5,000 rows = 5,000 rows" to "2 batches × 5,000 rows = 10,000 rows" on line 5, and similarly correct the 10-batch example to show "10 batches × 5,000 rows = 50,000 rows" instead of 100,000 rows. Apply these same corrections consistently to all related sections mentioned at lines 26-26, 38-40, 138-140, and 306-309 to ensure the purpose text, sample output, workflow example, and summary table all reflect the same accurate totals.docs/database/Status_6_Performance_Issue_Quick_Ref.md (3)
97-104:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore the blank line before the table.
The markdownlint MD058 warning still applies here.
📝 Suggested fix
**Expected Results**: + | Metric | Target | Your Result | |--------|--------|-------------|🤖 Prompt for 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. In `@docs/database/Status_6_Performance_Issue_Quick_Ref.md` around lines 97 - 104, Add a blank line between the "Expected Results" heading and the table that contains the Metric, Target, and Your Result columns. This will resolve the MD058 markdownlint warning which requires a blank line before tables in markdown documents.Source: Linters/SAST tools
42-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winQualify
Statusin theORDER BY.This is the same ambiguity called out earlier; use
stats."Status"so the sort stays tied to the joined alias.📝 Suggested fix
-ORDER BY stats."CorrespondenceId", Status +ORDER BY stats."CorrespondenceId", stats."Status"🤖 Prompt for 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. In `@docs/database/Status_6_Performance_Issue_Quick_Ref.md` at line 42, In the ORDER BY clause, the Status column reference is unqualified while CorrespondenceId is qualified with the stats alias. Qualify the Status column reference by prefixing it with stats to make it stats."Status", ensuring consistency with the CorrespondenceId reference and eliminating ambiguity about which table the Status column comes from in the sort operation.
184-204:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMark the decision tree fence as
text.Add a language tag so markdownlint stops flagging this block.
📝 Suggested fix
-``` +```text Is Status 6 query slow (>500ms)?🤖 Prompt for 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. In `@docs/database/Status_6_Performance_Issue_Quick_Ref.md` around lines 184 - 204, The code block containing the Status 6 query performance decision tree is missing a language tag specification, which causes markdownlint to flag it. Change the opening fence from triple backticks to triple backticks followed by the word "text" (```text) to specify the block as plain text. This language tag addition will satisfy markdownlint requirements without changing the decision tree content or functionality.Source: Linters/SAST tools
tools/Altinn.Correspondence.DialogActivityExporter/README.md (1)
245-247:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAnnotate the sample-output fence.
Same MD040 issue as before; add
texthere too.📝 Suggested fix
-``` +```text Processed: 50,000,000 | 25,000 rows/sec | Elapsed: 00:33:20🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/README.md` around lines 245 - 247, The code fence displaying the sample output "Processed: 50,000,000 | 25,000 rows/sec | Elapsed: 00:33:20" is missing a language identifier, causing an MD040 markdown linting violation. Add the language identifier `text` after the opening triple backticks (change ``` to ```text) for this code fence to comply with the markdown linting rules.Source: Linters/SAST tools
tools/Altinn.Correspondence.DialogActivityExporter/test-export-Issue1716.ps1 (1)
21-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a cross-platform temp path for the default output file.
The default path is Windows-only (
C:\temp\...), so default runs fail on non-Windows PowerShell hosts.Proposed fix
- Path to output CSV file (default: C:\temp\test_export_1716_{timestamp}.csv) + Path to output CSV file (default: <temp>/test_export_1716_{timestamp}.csv) ... if ([string]::IsNullOrEmpty($OutputPath)) { $timestamp = Get-Date -Format "yyyyMMdd_HHmmss" - $OutputPath = "C:\temp\test_export_1716_$($timestamp).csv" + $tempRoot = [System.IO.Path]::GetTempPath() + $OutputPath = Join-Path $tempRoot "test_export_1716_$($timestamp).csv" }Also applies to: 87-90
🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/test-export-Issue1716.ps1` around lines 21 - 22, The default output file path uses a Windows-specific hardcoded directory C:\temp\ which fails on non-Windows systems. Replace the hardcoded C:\temp\ path references (appearing at lines 21-22 and 87-90 in the default parameter values) with a cross-platform solution using PowerShell's [System.IO.Path]::GetTempPath() method to dynamically determine the appropriate temp directory for any operating system, then construct the filename with the timestamp accordingly.tools/Altinn.Correspondence.DialogActivityExporter/test-export-Issue1951.ps1 (1)
21-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a cross-platform temp path for the default output file.
The default path is Windows-only (
C:\temp\...), so default runs fail on non-Windows PowerShell hosts.Proposed fix
- Path to output CSV file (default: C:\temp\test_export_1951_{timestamp}.csv) + Path to output CSV file (default: <temp>/test_export_1951_{timestamp}.csv) ... if ([string]::IsNullOrEmpty($OutputPath)) { $timestamp = Get-Date -Format "yyyyMMdd_HHmmss" - $OutputPath = "C:\temp\test_export_1951_$($timestamp).csv" + $tempRoot = [System.IO.Path]::GetTempPath() + $OutputPath = Join-Path $tempRoot "test_export_1951_$($timestamp).csv" }Also applies to: 87-90
🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/test-export-Issue1951.ps1` around lines 21 - 22, Replace the hardcoded Windows-only default path `C:\temp\test_export_1951_{timestamp}.csv` with a cross-platform temp path by using PowerShell environment variables or system methods like `$env:TEMP` or `[System.IO.Path]::GetTempPath()` to dynamically construct the default file path. This change should be applied to all occurrences of this default path definition throughout the script (including the locations around lines 21-22 and 87-90).tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs (1)
445-456:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSingle-column cursor paging can skip rows at batch boundaries.
Paging is
CorrespondenceId >@lastId`` withORDER BY "CorrespondenceId"and cursor update from the last row’s `CorrespondenceId`. If one `CorrespondenceId` has multiple rows, remaining rows with the same id can be skipped in the next batch.Suggested direction
- var a2EventsCursorPredicate = lastCorrespondenceId.HasValue - ? "AND \"CorrespondenceId\" > `@lastId`" + var a2EventsCursorPredicate = hasCursor + ? "AND (\"CorrespondenceId\", \"Timestamp\", \"PartyUuid\") > (`@lastId`, `@lastTimestamp`, `@lastPartyUuid`)" : ""; ... - ORDER BY "CorrespondenceId" + ORDER BY "CorrespondenceId", "Timestamp", "PartyUuid"Also persist/read the full composite cursor in
ExportCheckpointand update cursor assignment fromstatusXResults[^1].Also applies to: 491-546
🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs` around lines 445 - 456, The current cursor paging implementation using only CorrespondenceId can skip rows when multiple rows share the same CorrespondenceId value at batch boundaries. The cursor update logic in the assignments for newStatus4Cursor and newStatus6Cursor only captures the CorrespondenceId from the last result (statusXResults[^1].CorrespondenceId), but this needs to be changed to use a composite cursor that includes additional identifying properties. Update the ExportCheckpoint to persist and retrieve the full composite cursor instead of just the single CorrespondenceId value, and modify the cursor assignments to capture all necessary properties from the last result row to ensure no rows are skipped when the same CorrespondenceId appears across multiple batches.tools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sql (2)
9-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove stale action guidance that conflicts with the script’s own “current state”.
The header says exporter/count queries already moved to INNER JOIN semantics, but action items still instruct changing exporter query and removing
IsDBNullhandling. That contradiction makes this runbook unsafe to apply.Also applies to: 160-161, 262-267
🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sql` around lines 9 - 14, The comment header in the test-count-performance.sql file contains outdated action items that contradict the established current state. Remove the stale guidance that instructs changing the exporter query and removing IsDBNull handling, since the header already states that calculate-counts.sql and the export code have already moved to INNER JOIN semantics. Keep only the accurate descriptive text that explains the current state and purpose of the test script. Apply the same cleanup to the other instances of this contradictory guidance at the referenced line ranges (160-161 and 262-267).
51-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd Status 6 coverage to match exporter workload assumptions.
All measurement queries only exercise
Status = 4; conclusions can be incomplete for Issue#1951where Status 6 is also exported.Suggested direction
-WHERE stats."Status" = 4 +WHERE stats."Status" IN (4, 6)(or duplicate each block with explicit
Status = 6variants to preserve comparability).Also applies to: 73-75, 118-121, 150-152, 179-180, 197-199, 221-223
🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sql` around lines 51 - 53, The performance test query in test-count-performance.sql only filters on Status = 4, but the exporter also processes Status = 6 per Issue `#1951`, which makes performance measurements incomplete. Update all WHERE clauses that currently filter exclusively on stats."Status" = 4 to also include Status = 6 by modifying the condition to allow both statuses. Apply this change to all occurrences mentioned in the "Also applies to" section (lines 73-75, 118-121, 150-152, 179-180, 197-199, 221-223) to ensure the performance tests measure the full exporter workload.tools/Altinn.Correspondence.DialogActivityExporter/export-1716-production.ps1 (1)
25-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign help text with the actual default batch size (2500).
The docblock/examples still describe
5000, but the runtime default is2500, which makes runbook usage error-prone.Suggested edit
- Number of rows per batch (default: 5000) - Recommended: 5000-10000 for optimal performance + Number of rows per batch (default: 2500) + Recommended: 2500-10000 for optimal performance ... - # - Batch size: 5,000 rows + # - Batch size: 2,500 rows ... - - Batch size 5,000 (default): Processes ~10,000 rows per batch + - Batch size 2,500 (default): Processes ~5,000 rows per batchAlso applies to: 49-52, 94-95, 134-134
🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/export-1716-production.ps1` around lines 25 - 27, The .PARAMETER BatchSize help documentation states the default is 5000 and recommends 5000-10000, but the actual runtime default is 2500. Update the BatchSize parameter documentation comment to accurately reflect the actual default value of 2500 and adjust the recommended range accordingly. Also apply the same correction to all other occurrences mentioned in the comment (lines 49-52, 94-95, and 134) to ensure consistency across the entire script.tools/Altinn.Correspondence.DialogActivityExporter/test-distinct-overhead.sql (1)
7-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake DISTINCT vs non-DISTINCT counts comparable.
LIMIT 10000is applied before count in the DISTINCT path, but afterCOUNT(*)in the non-DISTINCT path (no effect), so the comparison is invalid.Suggested fix
SELECT COUNT(*) as count_without_distinct -FROM correspondence."A2Iss1716A2Events" a2Events -INNER JOIN correspondence."CorrespondenceStatuses" stats - ON a2Events."CorrespondenceId" = stats."CorrespondenceId" - AND a2Events."Status" = stats."Status" - AND a2Events."PartyUuid" = stats."PartyUuid" - AND a2Events."StatusChanged" = stats."StatusChanged" -WHERE a2Events."Status" = 4 -LIMIT 10000; +FROM ( + SELECT + a2Events."CorrespondenceId", + stats."PartyUuid" + FROM correspondence."A2Iss1716A2Events" a2Events + INNER JOIN correspondence."CorrespondenceStatuses" stats + ON a2Events."CorrespondenceId" = stats."CorrespondenceId" + AND a2Events."Status" = stats."Status" + AND a2Events."PartyUuid" = stats."PartyUuid" + AND a2Events."StatusChanged" = stats."StatusChanged" + WHERE a2Events."Status" = 4 + LIMIT 10000 +) subq;Also applies to: 22-30
🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/test-distinct-overhead.sql` around lines 7 - 20, The LIMIT 10000 clause is applied inconsistently between the DISTINCT and non-DISTINCT query paths, making the COUNT(*) comparison invalid. In the DISTINCT path shown, LIMIT is applied inside the subquery before counting, but the non-DISTINCT path (lines 22-30) does not have the same LIMIT placement. Move the LIMIT 10000 to the same position in both query paths so that the COUNT(*) operations count equivalent result sets. This ensures both the DISTINCT and non-DISTINCT counts are comparable.docs/database/Fix_A2Parties_Recipient_Filter_Index.sql (1)
82-93:⚠️ Potential issue | 🟡 MinorSchema-qualify the index existence check to avoid misidentifying indexes in other schemas.
Line 83 checks only
pg_class.relnamewithout schema qualification. If an index with the same name exists in a different schema,v_index_existswould be set to true whilev_is_validreturns false (since the schema-qualified validity check on lines 85–92 finds nothing). This would incorrectly trigger the INVALID condition at line 113, producing misleading output about an invalid index when the actual issue is a missing index in thecorrespondenceschema.Proposed fix
- EXISTS(SELECT 1 FROM pg_class WHERE relname = 'IX_A2Parties_PartyUuid_Covering'), + EXISTS( + SELECT 1 + FROM pg_class c + JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE c.relname = 'IX_A2Parties_PartyUuid_Covering' + AND n.nspname = 'correspondence' + ),🤖 Prompt for 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. In `@docs/database/Fix_A2Parties_Recipient_Filter_Index.sql` around lines 82 - 93, The EXISTS check on line 83 for the index 'IX_A2Parties_PartyUuid_Covering' only queries pg_class.relname without schema qualification, which could match an index with the same name in a different schema and cause v_index_exists to be true while v_is_valid is false. Schema-qualify the EXISTS subquery by joining pg_class to pg_namespace and adding a filter for n.nspname = 'correspondence', matching the pattern already used in the COALESCE subquery on lines 85-92. This ensures the existence check specifically targets the index in the correspondence schema only.
🟡 Minor comments (7)
docs/database/Testing_Guide.md-438-445 (1)
438-445:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLoosen the Azure AD note.
The note says Azure AD auth requires Azure CLI, but the guide above already documents managed identity, Visual Studio, VS Code, and environment-variable auth. Reword this so it describes one supported option, not a hard requirement.
🤖 Prompt for 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. In `@docs/database/Testing_Guide.md` around lines 438 - 445, In the Notes section, revise the Azure AD Authentication bullet point to describe Azure CLI as one supported authentication option rather than as a hard requirement. The current wording "Requires Azure CLI (`az login`) to be authenticated" implies it is mandatory, but the guide documents multiple authentication methods (managed identity, Visual Studio, VS Code, and environment variables). Reword this note to acknowledge Azure CLI as a valid option without suggesting it is the only way to authenticate for Azure AD.docs/database/Max_Batches_Feature_Summary.md-243-243 (1)
243-243:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the production command typo.
dotbase runis not a valid command here. It looks like a typo fordotnet runor the real exporter invocation.🤖 Prompt for 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. In `@docs/database/Max_Batches_Feature_Summary.md` at line 243, In the production row of the table in the Max_Batches_Feature_Summary.md file, the command `dotbase run` is a typo and should be corrected to the actual valid command. Replace `dotbase run` with `dotnet run` (or verify against the actual exporter command used elsewhere in the documentation) to ensure consistency and accuracy in the production command example shown in the table entry.tools/Altinn.Correspondence.DialogActivityExporter/ISSUE-1951-PRODUCTION-EXPORT.md-112-115 (1)
112-115:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAnnotate the progress example fence.
This fence still trips MD040;
textis enough.📝 Suggested fix
-``` +```text Processed: 50,000,000 | 25,000 rows/sec | Elapsed: 00:33:20🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/ISSUE-1951-PRODUCTION-EXPORT.md` around lines 112 - 115, The code fence containing the batch timing and progress example output is missing a language annotation, which causes MD040 linting violations. Add the language identifier `text` to the opening backticks of the code fence that displays the progress example with the "Processed: 6,170,000 / 190,846,000" output to properly annotate the fence and satisfy the markdown linting requirements.Source: Linters/SAST tools
tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs-185-214 (1)
185-214:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest-mode checkpoint is saved and then always deleted.
Line 189 saves a final checkpoint for test mode, but Lines 210-214 delete it unconditionally at method end. That makes the test-mode checkpoint ineffective.
Proposed fix
- if (File.Exists(checkpointPath)) + if (!maxBatches.HasValue && File.Exists(checkpointPath)) { File.Delete(checkpointPath); _logger.LogInformation("Checkpoint file deleted after successful completion"); }🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cs` around lines 185 - 214, The checkpoint file is being saved for test mode recovery when maxBatches.HasValue is true and the batch limit is reached, but it is then unconditionally deleted at the end of the method regardless of whether test mode was active. To fix this, wrap the checkpoint file deletion logic (the File.Delete call) in a condition that only deletes the checkpoint when NOT in test mode, meaning only when maxBatches does not have a value or when the export completes normally without hitting the batch limit in test mode.tools/Altinn.Correspondence.DialogActivityExporter/test-export-Issue1951.ps1-156-158 (1)
156-158:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winApproximate row count underestimates test output.
$MaxBatches * $BatchSizeassumes one status stream, but the exporter can write rows from both status 4 and status 6 per batch. The displayed estimate can be significantly low.Suggested fix
- $estimatedRows = $MaxBatches * $BatchSize - Write-Host "Rows (approx): ~$estimatedRows (test mode: $MaxBatches batches x $BatchSize)" -ForegroundColor White + $estimatedUpperBound = $MaxBatches * $BatchSize * 2 + Write-Host "Rows (approx): <=$estimatedUpperBound (up to 2 status streams per batch)" -ForegroundColor White🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/test-export-Issue1951.ps1` around lines 156 - 158, The estimated row count calculation using $MaxBatches * $BatchSize does not account for the fact that the exporter generates rows from both status 4 and status 6 per batch. To fix this, multiply the $estimatedRows calculation by 2 to reflect that both status streams contribute rows during each batch iteration, resulting in an estimate that more accurately represents the actual output volume in test mode.tools/Altinn.Correspondence.DialogActivityExporter/find-duplicate-source.sql-22-36 (1)
22-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestrict Query 2 to actual duplicates.
The section is labeled as duplicate distribution, but it currently includes
duplicate_count = 1rows too. Add aHAVING COUNT(*) > 1filter in the CTE.Suggested fix
WITH duplicate_counts AS ( SELECT "CorrespondenceId", COUNT(*) as duplicate_count FROM correspondence."IdempotencyKeys" WHERE "StatusAction" = '3' GROUP BY "CorrespondenceId" + HAVING COUNT(*) > 1 )🤖 Prompt for 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. In `@tools/Altinn.Correspondence.DialogActivityExporter/find-duplicate-source.sql` around lines 22 - 36, The duplicate_counts CTE is currently including all CorrespondenceIds including those with only a single row (duplicate_count = 1), which should not be considered duplicates. Add a HAVING clause after the GROUP BY statement in the duplicate_counts CTE to filter and only include rows where the count is greater than 1, ensuring the query truly reflects only actual duplicate records.docs/database/Execute_A2Iss1951A2Events_Cleanup.sql-76-81 (1)
76-81:⚠️ Potential issue | 🟡 MinorAvoid hard-coded baseline row counts in verification math.
Using literal
195499279at lines 78 and 80 can report wrongdeleted_count/percent if the table changes or the script is rerun, which weakens the COMMIT decision gate. The backup table already exists (created at line 31), so replace with dynamic subqueries:Proposed fix
SELECT 'Rows deleted' AS check_name, - 195499279 - COUNT(*) AS deleted_count, + (SELECT COUNT(*) FROM correspondence."A2Iss1951A2Events_backup") - COUNT(*) AS deleted_count, COUNT(*) AS remaining_rows, - ROUND(100.0 * (195499279 - COUNT(*)) / 195499279, 2) AS pct_deleted + ROUND( + 100.0 * ((SELECT COUNT(*) FROM correspondence."A2Iss1951A2Events_backup") - COUNT(*)) / + NULLIF((SELECT COUNT(*) FROM correspondence."A2Iss1951A2Events_backup"), 0), 2 + ) AS pct_deleted FROM correspondence."A2Iss1951A2Events";🤖 Prompt for 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. In `@docs/database/Execute_A2Iss1951A2Events_Cleanup.sql` around lines 76 - 81, Replace the hard-coded literal value 195499279 in the verification query (used in the deleted_count and pct_deleted calculations) with a dynamic subquery that retrieves the original row count from the backup table created at line 31. This ensures the deleted_count and pct_deleted calculations reflect the actual baseline, making the verification logic robust for reruns and table changes. Update both occurrences of the literal in the SELECT statement that queries correspondence."A2Iss1951A2Events" to use the backup table count dynamically instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a5f147d5-af34-4d09-a1b3-551a7bde2645
📒 Files selected for processing (52)
docs/CRITICAL_FIX_Cursor_Pagination_Index_Issue.mddocs/Code_Review_Findings_Resolution.mddocs/Code_Review_Second_Pass_Resolution.mddocs/Export_Scripts_Quick_Reference.mddocs/Issue_1716_Export_Final_Specifications.mddocs/Network_Read_Performance_Issue.mddocs/Performance_Timing_Analysis_Guide.mddocs/database/Azure_Identity_Migration_Summary.mddocs/database/Check_Disk_Space_And_Table_Stats.sqldocs/database/Clean_A2Iss1951A2Events_Duplicates.sqldocs/database/Complete_Cleanup_A2Iss1951A2Events.sqldocs/database/Configure_PostgreSQL_For_Index_Creation.sqldocs/database/Diagnose_Slow_Query_A2Iss1951A2Events.sqldocs/database/Execute_A2Iss1951A2Events_Cleanup.sqldocs/database/Fast_Duplicate_Analysis_A2Iss1951A2Events.sqldocs/database/Fix_A2Parties_Recipient_Filter_Index.sqldocs/database/Fix_A2Parties_Recipient_Filter_Schema.sqldocs/database/Helper_Table_Optimization.mddocs/database/Index_Creation_Production_Summary.mddocs/database/Index_Creation_Scripts.sqldocs/database/Max_Batches_Feature_Summary.mddocs/database/Monitor_Index_Creation_A2Iss1951A2Events.sqldocs/database/Optimize_A2Iss1716A2Events_Indexes.sqldocs/database/Performance_Optimization_Summary.sqldocs/database/Query_Logging_Feature_Summary.mddocs/database/Query_Performance_Notes.mddocs/database/Quick_Test_Reference.mddocs/database/README.mddocs/database/Status_6_Performance_Issue_Quick_Ref.mddocs/database/Test_Export_Query.sqldocs/database/Testing_Documentation_Summary.mddocs/database/Testing_Guide.mdtools/Altinn.Correspondence.DialogActivityExporter/Altinn.Correspondence.DialogActivityExporter.csprojtools/Altinn.Correspondence.DialogActivityExporter/Analyze_A2Iss1951_Helper_Table_Creation.sqltools/Altinn.Correspondence.DialogActivityExporter/Create_A2Iss1951MigratedEvents_Helper_Table.sqltools/Altinn.Correspondence.DialogActivityExporter/DialogActivityExportService.cstools/Altinn.Correspondence.DialogActivityExporter/ISSUE-1951-PRODUCTION-EXPORT.mdtools/Altinn.Correspondence.DialogActivityExporter/Issue_1951_Query_Template_With_Timestamp_Fix.sqltools/Altinn.Correspondence.DialogActivityExporter/Program.cstools/Altinn.Correspondence.DialogActivityExporter/README.mdtools/Altinn.Correspondence.DialogActivityExporter/appsettings.jsontools/Altinn.Correspondence.DialogActivityExporter/appsettings.local.example.jsontools/Altinn.Correspondence.DialogActivityExporter/calculate-counts.sqltools/Altinn.Correspondence.DialogActivityExporter/diagnose-query-performance.sqltools/Altinn.Correspondence.DialogActivityExporter/export-1716-production.ps1tools/Altinn.Correspondence.DialogActivityExporter/export-1951-production.ps1tools/Altinn.Correspondence.DialogActivityExporter/find-duplicate-source.sqltools/Altinn.Correspondence.DialogActivityExporter/test-count-performance.sqltools/Altinn.Correspondence.DialogActivityExporter/test-distinct-overhead.sqltools/Altinn.Correspondence.DialogActivityExporter/test-export-Issue1716.ps1tools/Altinn.Correspondence.DialogActivityExporter/test-export-Issue1951.ps1tools/Altinn.Correspondence.DialogActivityExporter/verify-indexes-timestamp-fix.sql
✅ Files skipped from review due to trivial changes (8)
- tools/Altinn.Correspondence.DialogActivityExporter/appsettings.local.example.json
- docs/database/Testing_Documentation_Summary.md
- docs/database/Quick_Test_Reference.md
- docs/database/Azure_Identity_Migration_Summary.md
- tools/Altinn.Correspondence.DialogActivityExporter/Altinn.Correspondence.DialogActivityExporter.csproj
- docs/database/Query_Performance_Notes.md
- docs/database/Index_Creation_Production_Summary.md
- docs/database/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/Altinn.Correspondence.DialogActivityExporter/appsettings.json
Contributor
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 `@tools/Altinn.Correspondence.DialogActivityExporter/README.md`:
- Around line 288-301: The markdown tables in the README.md file are not
surrounded by blank lines as required by markdownlint rule MD058. Add a blank
line before the Summary table (before the header row starting with "| Metric |
Value |") and add a blank line after the last row of the Summary table (before
the "#### Helper Table Structure" heading). Apply the same fix to the other
table mentioned at lines 355-368 by ensuring each table has blank lines both
before and after it, separating it from adjacent headings and content.
- Around line 77-82: The current documentation for the Vacuum and Analyze
cleanup step does not warn operators about the blocking nature of VACUUM FULL on
the CorrespondenceStatuses table. Add a warning note in the README that clearly
explains that VACUUM FULL takes an exclusive lock and can block production
traffic for extended periods, then provide guidance on safe alternatives such as
using regular VACUUM instead (which is non-blocking) or scheduling VACUUM FULL
during maintenance windows when production impact is acceptable. Place this
warning prominently before or immediately after the VACUUM FULL SQL command
block.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b2bbc002-83c3-4352-b145-c1bdd0f74cd6
📒 Files selected for processing (3)
docs/database/Index_Creation_Scripts.sqldocs/database/Performance_Optimization_Summary.sqltools/Altinn.Correspondence.DialogActivityExporter/README.md
✅ Files skipped from review due to trivial changes (1)
- docs/database/Performance_Optimization_Summary.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/database/Index_Creation_Scripts.sql
Contributor
Author
|
All the cleanup-activities related to this codebase have been completed, but the changes do not need to be merged into the solution. |
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
1 participant
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.
Generated documentation, scipt and console app to export data neccesry for cleanup of two issues.
The perform this in a reasonable time frame I suggest adding two indexes. See documentation for justification.
This PR is not intended to be merged, it is only created for convenience and review to visualize.
EDIT: 30.06.2026: All the cleanup-activities related to this codebase have been completed, but the changes do not need to be merged into the solution. But this PR / Branch might be useful "just in case"
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Chores