Skip to content

Fix/crm issues 48 49 51#45

Merged
Bor4brn merged 3 commits intomainfrom
fix/crm-issues-48-49-51
Mar 12, 2026
Merged

Fix/crm issues 48 49 51#45
Bor4brn merged 3 commits intomainfrom
fix/crm-issues-48-49-51

Conversation

@Bor4brn
Copy link
Contributor

@Bor4brn Bor4brn commented Mar 12, 2026

No description provided.

@Bor4brn Bor4brn requested a review from anilguleroglu March 12, 2026 11:59
@anilguleroglu
Copy link
Contributor

Review summary (requesting changes):

  1. Provider key trimming now happens on create, but there’s no server-side validation that the trimmed key is non-empty. A whitespace-only key can be persisted or trigger DB errors. Please validate trimmedKey before insert in create endpoints (providerService + API).

  2. scripts/fix-provider-key-whitespace.mjs trims keys in-place without collision detection. If two keys differ only by whitespace, trimming can collapse them and corrupt references/uniqueness. Recommend pre-checking for collisions and aborting or requiring manual resolution; ideally run updates in a transaction per DB.

  3. decodeURIComponent on path segments in the vector UI can throw on malformed encodings and crash the page. Consider a safe decode helper with try/catch fallback.

Also: ensure getConfig().app.url is always set so loginUrl doesn’t become undefined/login in invite emails.

Full details in the review report: /tasks/7aeae198-4231-4550-b57e-e74c5af3aded/run_1/pr_review_report.md

@anilguleroglu
Copy link
Contributor

Review summary (requesting changes):

  1. decodeURIComponent on vector route params can throw URIError for malformed % encoding, which would crash the vector detail page/breadcrumbs. Consider try/catch with fallback to raw param (files: src/app/dashboard/vector/[providerKey]/[externalId]/page.tsx, src/components/layout/DashboardBreadcrumbs.tsx).
  2. The new scripts/fix-provider-key-whitespace.mjs blindly trims keys across tables without checking for collisions. If two keys differ only by whitespace, trimming can violate uniqueness constraints or create ambiguous references. Recommend preflight detection/reporting or conflict handling.
  3. Runtime trimming only normalizes new provider keys; existing tenants still depend on running the script. Please add explicit deployment/migration step or runtime warning to avoid lingering 404s.

Report saved at: /tasks/b6eea21f-4f43-4aee-9917-d147d29cbd8a/run_1/pr_review_report.md

@anilguleroglu
Copy link
Contributor

PR review complete. Summary:

  • No high-risk regressions found in provider assignment, license gating, or invite email updates.
  • Residual risk: ensure getConfig().app.url is set consistently so loginUrl in invitation emails is valid across environments.
  • Testing gap: consider coverage for invitation template fields + license-exempt auth endpoints still requiring session auth.

Suggested validation: invite a user and verify email content; verify /api/auth/session, /api/auth/change-password, /api/auth/logout behave for authenticated users without license access.

Full report: /tasks/d64be944-e44d-4e21-86bb-25974914b5e4/run_1/pr_review_report.md

@Bor4brn Bor4brn merged commit 65871eb into main Mar 12, 2026
@anilguleroglu
Copy link
Contributor

Review summary:

  • No blocking correctness issues found in provider assignment sync, auth license exemption, or invite email payload updates.
  • Residual risk: loginUrl depends on getConfig().app.url; if unset, invitation emails may contain undefined/login. Ensure config validation/defaults and template handling.
  • Tests not run here; recommend smoke test invite email rendering and auth endpoints under restricted license tiers.

Details in pr_review_report.md (run artifacts).

@anilguleroglu anilguleroglu deleted the fix/crm-issues-48-49-51 branch March 25, 2026 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants