Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRemoves legacy SQL analysis/optimization/binlog/schema-sync tooling and Bootstrap templates; prunes related permissions and init SQL; adds SPA-backed Audit, Data Dictionary, and Instance Operations features with new backend APIs/serializers/views/urls, frontend feature modules/pages, API clients, tests, and multiple frontend permission-check safety fixes. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (SPA)
participant AuditPage as AuditPage (Vue)
participant FrontAPI as Frontend API (lib/api)
participant Backend as Django API
participant DB as Metadata DB
Browser->>AuditPage: navigate /audit
AuditPage->>FrontAPI: fetchGeneralAuditLogs(filters, token)
FrontAPI->>Backend: GET /api/v1/audit/general/?...
Backend->>DB: query Audit models with filters and date range
DB-->>Backend: rows + count
Backend-->>FrontAPI: JSON {count, results}
FrontAPI-->>AuditPage: parsed response
AuditPage->>Browser: render audit table
sequenceDiagram
participant Browser as Browser (SPA)
participant OpsPage as Instance Ops Page
participant FrontAPI as Frontend API (lib/api)
participant Backend as Django API
participant Engine as Remote DB Engine
participant MetaDB as Metadata DB
Browser->>OpsPage: open Account Management
OpsPage->>FrontAPI: fetchInstanceOperationAccounts(instanceId, token)
FrontAPI->>Backend: GET /api/v1/instance-operations/account/?instance_id=...
Backend->>MetaDB: read saved account metadata
MetaDB-->>Backend: account records
Backend-->>FrontAPI: JSON account list
FrontAPI-->>OpsPage: show accounts
Browser->>OpsPage: submit create account form
OpsPage->>FrontAPI: createInstanceOperationAccount(payload, token)
FrontAPI->>Backend: POST /api/v1/instance-operations/account/
Backend->>Engine: connect & EXECUTE CREATE/GRANT SQL
Engine-->>Backend: success
Backend->>MetaDB: persist metadata
MetaDB-->>Backend: saved record
Backend-->>FrontAPI: success response
FrontAPI-->>OpsPage: refresh accounts, show success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
![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: 11
🧹 Nitpick comments (19)
sql/migrations/0015_alter_permission_options.py (1)
95-95: Consider a non-noop reverse for the data migration.
migrations.RunPython.noopas the reverse means rolling back this migration to0014_mailboxitemwill leave the retired permissions deleted (theAlterModelOptionsreverse won't recreateauth.Permissionrows either, since those are populated by the post-migrate signal only for permissions present in the currentMeta.permissions). That's likely fine given the feature removal is intentional, but worth confirming you don't rely on a clean rollback restoring them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/migrations/0015_alter_permission_options.py` at line 95, The data migration currently uses migrations.RunPython(remove_retired_sql_permissions, migrations.RunPython.noop) which leaves deleted auth.Permission rows unrecoverable on rollback; replace the noop with a proper reverse function (e.g., a recreate_retired_sql_permissions undo) that recreates the removed Permission rows (using the same codename, content_type and name used by remove_retired_sql_permissions) or explicitly document/confirm that rollbacks must not rely on restoring those rows; update the RunPython call to migrations.RunPython(remove_retired_sql_permissions, recreate_retired_sql_permissions) so rollbacks return the DB to the pre-migration state.frontend/src/lib/api.ts (1)
1457-1475: Falsy-zero guard onworkflow_type.
if (options.workflow_type)will skipworkflow_type === 0. If 0 is ever a meaningful workflow type for the audit endpoint (e.g. "unknown"/"other"), this filter would silently drop it. Same goes foraudit_id/workflow_idif they can ever legitimately be0. Switching to!= null(or explicit!== undefined) is safer.♻️ Defensive refinement
- if (options.audit_id) { + if (options.audit_id != null) { params.set('audit_id', `${options.audit_id}`) } - if (options.workflow_id) { + if (options.workflow_id != null) { params.set('workflow_id', `${options.workflow_id}`) } - if (options.workflow_type) { + if (options.workflow_type != null) { params.set('workflow_type', `${options.workflow_type}`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/api.ts` around lines 1457 - 1475, The guard in fetchWorkflowOperationAuditLogs currently uses truthy checks (if (options.audit_id), if (options.workflow_id), if (options.workflow_type)) which will skip legitimate zero values; update each guard to check for presence rather than truthiness (e.g., use options.audit_id != null or options.audit_id !== undefined, and the same for workflow_id and workflow_type) so 0 is included in the URLSearchParams when provided; keep the rest of the function (building params and calling apiGet/extractData) unchanged.sql/tests.py (1)
190-190: Optional: dropfprefix on placeholder-less strings (Ruff F541).These GET URLs are pre-existing but flagged by Ruff in the touched test methods. Cleaning them up while you’re here keeps lint clean.
♻️ Suggested cleanup
- r = self.client.get(f"/database/", data=data) + r = self.client.get("/database/", data=data) @@ - r = self.client.get(f"/dbdiagnostic/", data=data) + r = self.client.get("/dbdiagnostic/", data=data) @@ - r = self.client.get(f"/instanceparam/", data=data) + r = self.client.get("/instanceparam/", data=data) @@ - r = self.client.get(f"/audit_sqlquery/", data=data) + r = self.client.get("/audit_sqlquery/", data=data) @@ - r = self.client.get(f"/audit_sqlworkflow/", data=data) + r = self.client.get("/audit_sqlworkflow/", data=data)Also applies to: 198-198, 206-206, 238-238, 244-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/tests.py` at line 190, Remove unnecessary f-string prefixes from literal URLs in the tests: replace calls like self.client.get(f"/database/", data=data) with self.client.get("/database/", data=data) (and similarly for the other occurrences flagged by Ruff). Locate the self.client.get calls in sql/tests.py that use f-strings without any interpolation and convert them to normal string literals so Ruff F541 is resolved.api_instances/serializers.py (1)
218-219: Markinstance_nameas read-only.
instance_name = serializers.CharField(source="instance.instance_name")is only used for output in theInstanceOperationParamHistoryGET endpoint. Withoutread_only=True, DRF treats it as a writable field on deserialization, which is misleading and violates DRF best practices for dotted-source fields.♻️ Proposed fix
class InstanceParamHistorySerializer(serializers.ModelSerializer): - instance_name = serializers.CharField(source="instance.instance_name") + instance_name = serializers.CharField( + source="instance.instance_name", read_only=True + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api_instances/serializers.py` around lines 218 - 219, The InstanceParamHistorySerializer defines instance_name = serializers.CharField(source="instance.instance_name") but it's an output-only dotted-source field; modify InstanceParamHistorySerializer to mark instance_name as read-only by setting read_only=True on that field (e.g., instance_name = serializers.CharField(source="instance.instance_name", read_only=True)) so DRF won't treat it as writable during deserialization.api_audit/urls.py (1)
5-10: Optional: Consider naming the URL patterns.None of the routes specify a
name=kwarg. If any code (or templates/tests) needsreverse()to resolve these audit endpoints, you'll need names. Adding them now is cheap insurance and aids discoverability.♻️ Suggested addition
- path("v1/audit/general/", views.GeneralAuditLogList.as_view()), - path("v1/audit/query/", views.QueryAuditLogList.as_view()), - path("v1/audit/sql-workflow/", views.SqlWorkflowAuditLogList.as_view()), - path("v1/audit/workflow-log/", views.WorkflowOperationLogList.as_view()), + path("v1/audit/general/", views.GeneralAuditLogList.as_view(), name="audit-general"), + path("v1/audit/query/", views.QueryAuditLogList.as_view(), name="audit-query"), + path( + "v1/audit/sql-workflow/", + views.SqlWorkflowAuditLogList.as_view(), + name="audit-sql-workflow", + ), + path( + "v1/audit/workflow-log/", + views.WorkflowOperationLogList.as_view(), + name="audit-workflow-log", + ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api_audit/urls.py` around lines 5 - 10, Add explicit name= kwargs to each URL pattern so reverse()-based lookups will work; update the urlpatterns entries that reference GeneralAuditLogList, QueryAuditLogList, SqlWorkflowAuditLogList, and WorkflowOperationLogList to include unique, descriptive names (e.g., "audit-general", "audit-query", "audit-sql-workflow", "audit-workflow-log") ensuring each path(...) call includes the name parameter.sql/views.py (1)
51-52:spa_redirectis a redundant wrapper.The helper just forwards to
HttpResponseRedirect(path)with no added behavior (no logging, no logged-out handling, no querystring forwarding). At call sites,return HttpResponseRedirect("/audit")is just as clear. Either inline it or make it carry its weight (e.g., preserve querystrings, emit aDeprecationheader, or log redirect telemetry to track legacy hits during the transition).♻️ Suggested enrichment if you want to keep the helper
-def spa_redirect(path): - return HttpResponseRedirect(path) +def spa_redirect(path): + """Redirect a legacy server-rendered route to its SPA equivalent. + + Centralized so legacy hits can be observed during the SPA migration. + """ + logger.info("Legacy route redirected to SPA path: %s", path) + response = HttpResponseRedirect(path) + response["Deprecation"] = "true" + return response🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/views.py` around lines 51 - 52, The spa_redirect helper is redundant—remove the spa_redirect function and replace its usages (calls to spa_redirect(...)) with direct returns of HttpResponseRedirect(path); search for the spa_redirect symbol and update call sites to use HttpResponseRedirect(path) so behavior remains identical, then delete the spa_redirect definition from the module to avoid dead code.frontend/src/features/instance-operations/manifest.ts (1)
17-50: Differentiate the navigation icons.All four sidebar items reuse the same
Wrenchicon, which makes the menu harder to scan and weakens visual differentiation between Databases, Accounts, Parameters, and Diagnostics. The corresponding pages already import distinct lucide icons (e.g.,Database,UserRound/KeyRound,SlidersHorizontal,SquareActivity); reusing those here would keep the navigation consistent with each page's identity.♻️ Suggested change
-import { Wrench } from 'lucide-vue-next' +import { Database, SlidersHorizontal, SquareActivity, UserRound } from 'lucide-vue-next' @@ - icon: Wrench, - order: 22, + icon: Database, + order: 22, @@ - icon: Wrench, - order: 23, + icon: UserRound, + order: 23, @@ - icon: Wrench, - order: 24, + icon: SlidersHorizontal, + order: 24, @@ - icon: Wrench, - order: 25, + icon: SquareActivity, + order: 25,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/instance-operations/manifest.ts` around lines 17 - 50, The navigation items in the manifest reuse the same Wrench icon making items hard to distinguish; update the navigation array so each route uses its page-specific lucide icon: use Database for '/instance-operations/databases', UserRound or KeyRound for '/instance-operations/accounts' (pick the one matching the account page import), SlidersHorizontal for '/instance-operations/parameters', and SquareActivity for '/instance-operations/diagnostics' instead of Wrench; ensure the corresponding icon identifiers (Database, UserRound/KeyRound, SlidersHorizontal, SquareActivity) are imported at the top of the file and referenced in the navigation entries.api_instances/urls.py (1)
17-120: Routes look good; consider addingname=kwargs.The new
data-dictionary/*andinstance-operations/*routes are consistent and won't clash with the earlierv1/instance/<int:pk>/(theintconverter only matches digits). Same nit as the audit URLs: none of the new paths declare aname=, which makesreverse()and named-URL referencing impossible. Adding names per route would future-proof the API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api_instances/urls.py` around lines 17 - 120, The routes lack name= kwargs which prevents reverse() and named URL lookup; update each path(...) entry (e.g., the ones referencing DataDictionaryInstanceList, DataDictionaryDatabaseList, DataDictionaryTableList, DataDictionaryTableDetail, DataDictionaryExport, InstanceOperationDatabaseListCreate, InstanceOperationDatabaseInstanceList, InstanceOperationDatabaseDetail, InstanceOperationAccountListCreate, InstanceOperationAccountInstanceList, InstanceOperationAccountMetadata, InstanceOperationAccountPassword, InstanceOperationAccountLock, InstanceOperationAccountDelete, InstanceOperationAccountGrant, InstanceOperationParamList, InstanceOperationParamInstanceList, InstanceOperationParamHistory, InstanceOperationParamEdit, InstanceOperationDiagnosticInstanceList, InstanceOperationDiagnosticProcessList, InstanceOperationDiagnosticKillPreview, InstanceOperationDiagnosticKill, InstanceOperationDiagnosticTablespace, InstanceOperationDiagnosticTransactions, InstanceOperationDiagnosticLocks) to include a unique name="..." kwarg; pick consistent, descriptive names (e.g., "data_dictionary.instances.list", "instance_operations.database.create" or similar) and ensure names are unique across the module so reverse("name") can be used reliably.frontend/src/features/audit/pages/AuditPage.vue (1)
158-186: Double-fetch on view switch.
setView()mutatesfilters.action,filters.status,filters.syntaxTypeand immediately callsloadAuditLogs(). Those same fields are watched at lines 181–186, so the watcher additionally schedulesdebouncedLoadAuditLogs()(~250 ms later), producing a second request per tab switch — wasteful and easy to mistake for a flicker/pagination bug. Either reset the filters before the watcher is registered, suppress the debounce when switching views, or skip the explicitloadAuditLogs()call insetView()and rely on the watcher.♻️ One way to dedupe
function setView(view: AuditView) { activeView.value = view filters.page = 1 filters.action = '' filters.status = '' filters.syntaxType = '' - void loadAuditLogs() + // The watcher will pick up the filter resets and trigger a debounced load. + // Force an immediate load only when nothing changed (i.e., no other watcher fire). + void loadAuditLogs() + // ...or, alternatively, drop this explicit call and let the watcher run. }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/audit/pages/AuditPage.vue` around lines 158 - 186, setView currently mutates filters.action/status/syntaxType and calls loadAuditLogs(), but those same filter fields are watched by the watcher that triggers debouncedLoadAuditLogs(), causing a duplicate fetch; to fix, remove the explicit loadAuditLogs() call from setView and rely on the watcher (or alternatively cancel/suppress the debounce when switching views), i.e. update the setView function (referencing setView, filters, loadAuditLogs, debouncedLoadAuditLogs, and the watch callback) so only one request is scheduled when switching views.frontend/src/features/instance-operations/pages/DatabaseManagementPage.vue (1)
247-251: Refresh button only checksloadingDatabases.
refreshDatabases()awaits bothloadInstances()andloadDatabases(), but the button is only disabled whenloadingDatabasesis true. A user clicking during theloadInstancesphase can stack overlapping refresh calls.- <Button variant="outline" type="button" class="gap-2" :disabled="loadingDatabases" `@click`="void refreshDatabases()"> + <Button variant="outline" type="button" class="gap-2" :disabled="loadingInstances || loadingDatabases" `@click`="void refreshDatabases()"> <RefreshCw class="h-4 w-4" /> Refresh </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/instance-operations/pages/DatabaseManagementPage.vue` around lines 247 - 251, The Refresh button only checks loadingDatabases but refreshDatabases() awaits both loadInstances() and loadDatabases(), so update the button disabling logic to prevent clicks while either phase is running by using both flags (e.g., change the :disabled binding on the Refresh Button to combine loadingDatabases and loadingInstances), and ensure the component exposes/sets loadingInstances around the loadInstances() call so the combined check is reliable.api_instances/tests.py (1)
24-32:FakeDictionaryResult.to_dict()fails silently when columns and rows aren't column-aligned.For
get_all_databases()you setrows=["appdb", "mysql", "hidden"]andcolumn_list=[], soto_dict()would iterate strings and produce[{}, {}, {}]. Not exercised by current tests, but if a future caller invokesto_dict()on that result it'll quietly return empty dicts instead of raising. Consider asserting or normalizing the row shape so the fake fails loudly when misused.def to_dict(self): - return [dict(zip(self.column_list, row)) for row in self.rows] + return [ + dict(zip(self.column_list, row, strict=True)) + for row in self.rows + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api_instances/tests.py` around lines 24 - 32, FakeDictionaryResult.to_dict currently returns empty dicts when column_list is empty or when row lengths don't match columns; update FakeDictionaryResult.to_dict to validate and fail loudly: if column_list is empty raise ValueError (or if column_list has length 1 allow scalar rows by normalizing to single-element tuples), and for each row assert that its length equals len(column_list) (raising ValueError with a clear message if not). Reference: class FakeDictionaryResult and its to_dict method—add these checks/normalization so tests or callers get an explicit error instead of silent empty dicts.frontend/src/features/inventory/pages/DataDictionaryPage.vue (1)
261-269: SynchronousrevokeObjectURLafterclick()can race in some browsers.A handful of older browsers may not have started the download by the time
revokeObjectURLfires synchronously afterclick(). Modern Chrome/Firefox handle this fine, but defensive coding (a microtask/setTimeout(..., 0)deferred revoke) eliminates the rare interrupted-download case.- anchor.click() - anchor.remove() - window.URL.revokeObjectURL(objectUrl) + anchor.click() + anchor.remove() + setTimeout(() => window.URL.revokeObjectURL(objectUrl), 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/inventory/pages/DataDictionaryPage.vue` around lines 261 - 269, The synchronous call to window.URL.revokeObjectURL(objectUrl) may race with the download start after anchor.click(); defer revocation to ensure the browser has time to begin the download by replacing the immediate revoke with a microtask or macrotask (e.g., queueMicrotask or setTimeout(..., 0)) so revokeObjectURL(objectUrl) runs shortly after click; update the block that creates objectUrl/anchor (the objectUrl variable and anchor.click() usage in DataDictionaryPage.vue) to revoke the URL asynchronously instead of immediately.frontend/src/features/instance-operations/pages/SessionDiagnosticsPage.vue (2)
102-105: Silent column truncation at 10.
Object.keys(firstRow).slice(0, 10)quietly drops anything beyond the 10th key, so wider diagnostic payloads (some MySQL processlist/locks views have >10 columns) won't render any cue that more data is available. Either drop the slice or surface a "+N more columns" indicator so operators don't make decisions on partial data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/instance-operations/pages/SessionDiagnosticsPage.vue` around lines 102 - 105, The current columns computed uses Object.keys(firstRow).slice(0, 10) which silently drops columns; change it to keep the full key list (const columns = computed(() => { const firstRow = rows.value[0]; return firstRow ? Object.keys(firstRow) : [] })) or, if you must limit for UI, create two computeds: displayColumns (first 10) and extraColumnsCount (computed from Object.keys(firstRow).length - displayColumns.length) and update the template to render displayColumns plus a "+{extraColumnsCount} more columns" indicator; update references to columns in the component accordingly (look for the columns computed and any template usages).
115-129: Checkbox treats process ID0as "no id" and re-evaluatesprocessId(row)three times.
processId(row) ? selectedThreadIds.includes(processId(row) as number) : falseand:disabled="!processId(row)"use truthiness, so a (theoretical) thread id of0would render unchecked and disabled even when selected. MySQL thread IDs are positive in practice, but the sameprocessId(row)is also called up to three times per row per render — clearer and safer to compute once and compare againstnull.♻️ Suggested change
- <td v-if="activeTab === 'processes' && canKillProcesses" class="px-4 py-3"> - <input - type="checkbox" - class="h-4 w-4 rounded border-slate-300" - :aria-label="`Select process ${processId(row) ?? 'unknown'}`" - :checked="processId(row) ? selectedThreadIds.includes(processId(row) as number) : false" - :disabled="!processId(row)" - `@change`="toggleThread(row)" - > - </td> + <td v-if="activeTab === 'processes' && canKillProcesses" class="px-4 py-3"> + <input + type="checkbox" + class="h-4 w-4 rounded border-slate-300" + :aria-label="`Select process ${processId(row) ?? 'unknown'}`" + :checked="processId(row) !== null && selectedThreadIds.includes(processId(row) as number)" + :disabled="processId(row) === null" + `@change`="toggleThread(row)" + > + </td>Also applies to: 363-373
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/instance-operations/pages/SessionDiagnosticsPage.vue` around lines 115 - 129, processId(row) is being called multiple times and its truthiness treats a valid numeric id of 0 as "no id"; compute the id once and use explicit null checks instead of truthy checks. Update the code paths that render/disable/compare (calls around processId(row), toggleThread, and any template bindings using processId) to call processId(row) a single time into a const (e.g., const id = processId(row)), reuse that id in selectedThreadIds checks and in :disabled, and change conditions to id !== null (or id == null) rather than relying on truthiness so an id of 0 is handled correctly; also ensure toggleThread already uses the single computed id and does not re-call processId.api_audit/views.py (1)
20-30: Optional:_date_rangereturns naive datetimes.With Django's default
USE_TZ=True, comparing a naive datetime against a timezone-awareDateTimeFieldraises aRuntimeWarningand (depending on the DB backend) can drop the date by a few hours near boundaries. If this project runsUSE_TZ=True, wrap the parsed values withtimezone.make_aware(matching the existingfrom django.utils import timezonealready used elsewhere in the codebase). IfUSE_TZ=False, this is fine to leave.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api_audit/views.py` around lines 20 - 30, _date_range currently returns naive datetimes which will cause warnings/incorrect comparisons when Django is running with USE_TZ=True; update _date_range to make the parsed start and end datetimes timezone-aware using django.utils.timezone.make_aware (preserving the added day on end), e.g., call timezone.make_aware on the parsed start and end before returning (keep the existing ValueError handling and return signature).frontend/src/features/instance-operations/pages/ParameterSettingsPage.vue (1)
166-224: Avoid the redundant initial load triggered by bothrefreshAlland the watcher.
refreshAll()runsloadInstances,loadParams, andloadHistoryin parallel. On a fresh mountselectedInstanceIdis stillnull, soloadParamsandloadHistoryearly-return. As soon asloadInstancesassignsselectedInstanceId.value, the[selectedInstanceId, editableFilter]watcher fires and re-issuesloadParams+loadHistory. The net effect is two extra no-op invocations on every refresh. AwaitingloadInstancesfirst and relying on the watcher (or only fetching inrefreshAllwhen an id is already set) keeps behavior identical with one fewer round of wasted calls.♻️ Proposed refactor
async function refreshAll() { feedback.value = '' - await Promise.all([loadInstances(), loadParams(), loadHistory()]) + const previousId = selectedInstanceId.value + await loadInstances() + // If loadInstances did not change selectedInstanceId, the watcher won't fire, + // so trigger the dependent loads explicitly. + if (selectedInstanceId.value === previousId) { + await Promise.all([loadParams(), loadHistory()]) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/instance-operations/pages/ParameterSettingsPage.vue` around lines 166 - 224, The initial mount triggers duplicate calls because refreshAll() runs loadInstances(), loadParams(), and loadHistory() in parallel while selectedInstanceId is still null, then the watcher on [selectedInstanceId, editableFilter] fires when loadInstances sets selectedInstanceId; to fix, change refreshAll() to await loadInstances() first and only then call loadParams() and loadHistory() (e.g., await loadInstances(); await Promise.all([loadParams(), loadHistory()])) so the watcher handles subsequent loads, or alternatively conditionally skip calling loadParams()/loadHistory() in refreshAll() when selectedInstanceId.value is falsy; update the refreshAll function (and keep the watcher and onMounted flow) accordingly to eliminate the redundant early no-op calls.frontend/src/features/instance-operations/pages/AccountManagementPage.vue (2)
237-240: Same redundant-load pattern asParameterSettingsPage— see that comment.
refreshAccounts()parallel-firesloadInstancesandloadAccounts; the latter early-returns whenselectedInstanceIdis null, and then the[selectedInstanceId, savedOnly]watcher firesloadAccountsagain onceloadInstancesassigns the id. AwaitingloadInstancesfirst and only callingloadAccountsif the id didn't change avoids the extra no-op round trip.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/instance-operations/pages/AccountManagementPage.vue` around lines 237 - 240, refreshAccounts currently calls loadInstances and loadAccounts in parallel which causes a redundant no-op because loadAccounts early-returns when selectedInstanceId is null and the [selectedInstanceId, savedOnly] watcher then calls loadAccounts again after loadInstances sets the id; fix by making refreshAccounts call await loadInstances() first, capture the prior selectedInstanceId before the call, and only call loadAccounts() afterwards if selectedInstanceId is non-null and equal to the prior value (i.e., the instance id did not change), so the watcher handles the case where loadInstances sets a new id.
518-705: Reclaim the right-hand column when no form is open.The two-column grid
xl:grid-cols-[minmax(0,1fr)_minmax(21rem,0.34fr)]keeps the second track sized (≥21rem) even though the form Card is gated byv-if="activeFormMode". As a result the accounts table is squeezed by ~21rem of empty space whenever a user is just browsing/filtering. Either collapse to a single column when there's no active form, or move the form into a side drawer so the table can use the full width by default — this keeps the open-on-selection behavior the layout already implements while giving back useful list space.♻️ Sketch of dynamic grid template
- <div class="grid gap-6 xl:grid-cols-[minmax(0,1fr)_minmax(21rem,0.34fr)]"> + <div + class="grid gap-6" + :class="activeFormMode ? 'xl:grid-cols-[minmax(0,1fr)_minmax(21rem,0.34fr)]' : 'xl:grid-cols-1'" + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/instance-operations/pages/AccountManagementPage.vue` around lines 518 - 705, The grid's xl columns are fixed causing wasted space when no form is open; change the container div's static class "xl:grid-cols-[minmax(0,1fr)_minmax(21rem,0.34fr)]" to a dynamic class bound to a computed or method (e.g. computed name gridColsClass or getGridColsClass) that returns 'xl:grid-cols-[minmax(0,1fr)_minmax(21rem,0.34fr)]' when activeFormMode is truthy and 'xl:grid-cols-1' (or equivalent single-column class) when falsy, then update the div to use :class="['grid gap-6', gridColsClass]" so the right-hand Card collapses away when no form is open (refer to the container div, activeFormMode, and the new computed gridColsClass).api_instances/views.py (1)
143-150: Optional: chain the re-raisedValidationErrorto silence Ruff B904.Across this file (this helper plus many
except Exception as exc: ... raise serializers.ValidationError(...)blocks at L729/792/888/976/1066/1128/1252/1303/1409/1464/1508/1578/1671/1816/1915/1960/2005/2053/2099/2143), Ruff flags the bareraiseform. None of these are bugs, but usingraise ... from exc(orfrom Nonewhere the engine error message is already surfaced) preserves causal chains in tracebacks and clears the lint. Worth a sweep in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api_instances/views.py` around lines 143 - 150, The ValidationError being re-raised in helpers like _data_dictionary_instance should chain the original exception to satisfy Ruff B904; change each except block (e.g., except Instance.DoesNotExist:) to capture the exception (except Instance.DoesNotExist as exc:) and re-raise the serializers.ValidationError(...) using "raise serializers.ValidationError(... ) from exc" (or "from None" if you intentionally want to suppress the original traceback where the lower-level error message is already surfaced), and apply the same pattern for the other except Exception handlers listed (lines referenced in the review) to preserve causal chains and silence the linter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api_audit/tests.py`:
- Around line 58-78: The test test_general_audit_ignores_invalid_date_filter
uses a brittle hardcoded end_date "2026-04-25"; change that to a far-future
sentinel like "9999-12-31" so the test still verifies that _date_range (or the
audit filtering logic used by the general audit view) drops the date filter when
start_date is unparseable rather than accidentally relying on the current date;
update the end_date value in the request payload used in this test to
"9999-12-31".
In `@api_audit/views.py`:
- Around line 200-223: The GET handler currently passes the raw audit_id string
into WorkflowLog.objects.filter(audit_id=audit_id) which can cause a 500 for
non-numeric input; before the filter (in the method decorated get),
validate/coerce audit_id to an integer when it's provided (e.g., attempt
int(audit_id) and on ValueError raise serializers.ValidationError with a clear
message) and then use the integer value in
WorkflowLog.objects.filter(audit_id=audit_id); keep the existing WorkflowAudit
lookup behavior when audit_id is not provided.
In `@api_instances/serializers.py`:
- Around line 143-144: The validate_password method currently mutates input by
returning value.strip(); instead, preserve the original password and only
validate: in validate_password(value) do not call strip(), but check if value is
empty or only whitespace and raise a serializers.ValidationError (or appropriate
DRF error) for blank passwords; keep the original value unchanged and return
value when valid so stored passwords match exactly what the user provided.
In `@api_instances/views.py`:
- Around line 1789-1808: The code assumes engine.get_variables returns rows with
at least two columns when assigning runtime_value =
current_variables.rows[0][1], which can raise IndexError/TypeError for
single-element or None-padded rows; update the block handling current_variables
(after current_variables.rows check in the same try) to validate that
current_variables.rows[0] is a sequence with length >= 2 and that the second
element is present, and if not raise serializers.ValidationError({"errors":
"Parameter returned unexpected row shape from instance."}) so the error flows
through the existing ValidationError handling; reference engine.get_variables,
current_variables.rows and the runtime_value assignment to locate where to add
this guard.
- Around line 1395-1428: The password-reset path currently calls
InstanceAccount.objects.update_or_create(..., defaults={"remark": ""}) which
overwrites an existing account's remark; change it to avoid clobbering remark by
using InstanceAccount.objects.get_or_create(...) (or call update_or_create
without the remark default) so existing rows are left untouched and only
newly-created accounts get an initial remark; locate the call to
InstanceAccount.objects.update_or_create in the
InstanceOperationAccountPassword.post handler and replace it with get_or_create
(and set remark for the created case) or remove the remark from defaults to
preserve existing metadata.
In `@frontend/src/features/audit/pages/AuditPage.vue`:
- Around line 188-191: Wrap the onMounted initial load in the same
toUserFacingMessage error-handling flow so rejections from
authStore.loadCurrentUser() are surfaced; specifically, replace the bare await
authStore.loadCurrentUser() call inside the onMounted(async () => { ... }) with
an awaited toUserFacingMessage wrapper (or a try/catch that calls
toUserFacingMessage on failure) referencing authStore.loadCurrentUser(), then
proceed to call loadAuditLogs() only after successful authentication; use the
existing toUserFacingMessage helper and the existing loadAuditLogs function
names so failures are displayed to the user.
In `@frontend/src/features/instance-operations/pages/SessionDiagnosticsPage.vue`:
- Around line 232-247: The onMounted block currently awaits
authStore.loadCurrentUser() with no error handling; wrap the call in the same
toUserFacingMessage flow used elsewhere (e.g., DatabaseManagementPage.vue) so
any rejection becomes a user-facing string and is assigned to error.value, then
return early to stop the permission checks and loadInstances call. Specifically,
catch errors from authStore.loadCurrentUser() (or call it through
toUserFacingMessage), set error.value to the returned message, and exit the
onMounted handler before checking canOpenDiagnostics, firstVisibleTab, setting
activeTab, or calling loadInstances.
In `@frontend/src/features/inventory/pages/DataDictionaryPage.vue`:
- Around line 277-286: Wrap the authStore.loadCurrentUser() call inside the
onMounted handler with the same error-to-user-message flow used elsewhere: call
toUserFacingMessage(...) around await authStore.loadCurrentUser() (or catch its
rejection and pass the error into toUserFacingMessage) and assign the result to
error when present, before returning; keep subsequent permission check for
canViewDataDictionary and the call to refreshDataDictionary unchanged. This
mirrors the pattern used in DatabaseManagementPage (use toUserFacingMessage to
convert loadCurrentUser errors into a user-facing string and set error.value).
In `@sql/migrations/0015_alter_permission_options.py`:
- Around line 5-24: The migration remove_retired_sql_permissions deletes
permissions listed in RETIRED_SQL_PERMISSION_CODENAMES but seed SQL files still
INSERT those same codenames, causing them to be re-created then removed; fix by
updating the seed SQL files (src/init_sql/v*.sql) to remove or conditionally
skip INSERTs for the listed codenames (menu_sqlanalyze, menu_sqloptimize,
menu_sqladvisor, menu_slowquery, menu_my2sql, menu_schemasync, menu_document,
sql_analyze, optimize_sqladvisor, optimize_sqltuning, optimize_soar), or change
initialization ordering so seeds run after this migration, and add a short note
in docs/tests indicating these codenames are retired (referencing
RETIRED_SQL_PERMISSION_CODENAMES and remove_retired_sql_permissions to locate
the migration).
In `@sql/plugins/tests.py`:
- Line 145: The test unpacks stderr from
plugin.execute_cmd(cmd_args).communicate() but never uses it, triggering Ruff
RUF059; change the unpack to ignore stderr by renaming it to _stderr or dropping
it (e.g., stdout, _stderr = plugin.execute_cmd(cmd_args).communicate() or just
stdout = plugin.execute_cmd(cmd_args).communicate()[0]) so update the call site
in tests.py where execute_cmd and communicate are used to remove the unused
variable.
In `@sql/views.py`:
- Around line 633-648: The three view functions audit, audit_sqlquery, and
audit_sqlworkflow currently all redirect to "/audit", which loses tab context;
update each to append a tab query parameter (e.g., "/audit?tab=general",
"/audit?tab=query", "/audit?tab=sql-workflow") so bookmarks retain intent, and
then update the frontend AuditPage.vue to read the route query in its
onMounted() hook: have onMounted() inspect route.query.tab (falling back to
'general'), set activeView accordingly, and only then trigger data loading so
the correct tab content is displayed on initial load.
---
Nitpick comments:
In `@api_audit/urls.py`:
- Around line 5-10: Add explicit name= kwargs to each URL pattern so
reverse()-based lookups will work; update the urlpatterns entries that reference
GeneralAuditLogList, QueryAuditLogList, SqlWorkflowAuditLogList, and
WorkflowOperationLogList to include unique, descriptive names (e.g.,
"audit-general", "audit-query", "audit-sql-workflow", "audit-workflow-log")
ensuring each path(...) call includes the name parameter.
In `@api_audit/views.py`:
- Around line 20-30: _date_range currently returns naive datetimes which will
cause warnings/incorrect comparisons when Django is running with USE_TZ=True;
update _date_range to make the parsed start and end datetimes timezone-aware
using django.utils.timezone.make_aware (preserving the added day on end), e.g.,
call timezone.make_aware on the parsed start and end before returning (keep the
existing ValueError handling and return signature).
In `@api_instances/serializers.py`:
- Around line 218-219: The InstanceParamHistorySerializer defines instance_name
= serializers.CharField(source="instance.instance_name") but it's an output-only
dotted-source field; modify InstanceParamHistorySerializer to mark instance_name
as read-only by setting read_only=True on that field (e.g., instance_name =
serializers.CharField(source="instance.instance_name", read_only=True)) so DRF
won't treat it as writable during deserialization.
In `@api_instances/tests.py`:
- Around line 24-32: FakeDictionaryResult.to_dict currently returns empty dicts
when column_list is empty or when row lengths don't match columns; update
FakeDictionaryResult.to_dict to validate and fail loudly: if column_list is
empty raise ValueError (or if column_list has length 1 allow scalar rows by
normalizing to single-element tuples), and for each row assert that its length
equals len(column_list) (raising ValueError with a clear message if not).
Reference: class FakeDictionaryResult and its to_dict method—add these
checks/normalization so tests or callers get an explicit error instead of silent
empty dicts.
In `@api_instances/urls.py`:
- Around line 17-120: The routes lack name= kwargs which prevents reverse() and
named URL lookup; update each path(...) entry (e.g., the ones referencing
DataDictionaryInstanceList, DataDictionaryDatabaseList, DataDictionaryTableList,
DataDictionaryTableDetail, DataDictionaryExport,
InstanceOperationDatabaseListCreate, InstanceOperationDatabaseInstanceList,
InstanceOperationDatabaseDetail, InstanceOperationAccountListCreate,
InstanceOperationAccountInstanceList, InstanceOperationAccountMetadata,
InstanceOperationAccountPassword, InstanceOperationAccountLock,
InstanceOperationAccountDelete, InstanceOperationAccountGrant,
InstanceOperationParamList, InstanceOperationParamInstanceList,
InstanceOperationParamHistory, InstanceOperationParamEdit,
InstanceOperationDiagnosticInstanceList, InstanceOperationDiagnosticProcessList,
InstanceOperationDiagnosticKillPreview, InstanceOperationDiagnosticKill,
InstanceOperationDiagnosticTablespace, InstanceOperationDiagnosticTransactions,
InstanceOperationDiagnosticLocks) to include a unique name="..." kwarg; pick
consistent, descriptive names (e.g., "data_dictionary.instances.list",
"instance_operations.database.create" or similar) and ensure names are unique
across the module so reverse("name") can be used reliably.
In `@api_instances/views.py`:
- Around line 143-150: The ValidationError being re-raised in helpers like
_data_dictionary_instance should chain the original exception to satisfy Ruff
B904; change each except block (e.g., except Instance.DoesNotExist:) to capture
the exception (except Instance.DoesNotExist as exc:) and re-raise the
serializers.ValidationError(...) using "raise serializers.ValidationError(... )
from exc" (or "from None" if you intentionally want to suppress the original
traceback where the lower-level error message is already surfaced), and apply
the same pattern for the other except Exception handlers listed (lines
referenced in the review) to preserve causal chains and silence the linter.
In `@frontend/src/features/audit/pages/AuditPage.vue`:
- Around line 158-186: setView currently mutates
filters.action/status/syntaxType and calls loadAuditLogs(), but those same
filter fields are watched by the watcher that triggers debouncedLoadAuditLogs(),
causing a duplicate fetch; to fix, remove the explicit loadAuditLogs() call from
setView and rely on the watcher (or alternatively cancel/suppress the debounce
when switching views), i.e. update the setView function (referencing setView,
filters, loadAuditLogs, debouncedLoadAuditLogs, and the watch callback) so only
one request is scheduled when switching views.
In `@frontend/src/features/instance-operations/manifest.ts`:
- Around line 17-50: The navigation items in the manifest reuse the same Wrench
icon making items hard to distinguish; update the navigation array so each route
uses its page-specific lucide icon: use Database for
'/instance-operations/databases', UserRound or KeyRound for
'/instance-operations/accounts' (pick the one matching the account page import),
SlidersHorizontal for '/instance-operations/parameters', and SquareActivity for
'/instance-operations/diagnostics' instead of Wrench; ensure the corresponding
icon identifiers (Database, UserRound/KeyRound, SlidersHorizontal,
SquareActivity) are imported at the top of the file and referenced in the
navigation entries.
In `@frontend/src/features/instance-operations/pages/AccountManagementPage.vue`:
- Around line 237-240: refreshAccounts currently calls loadInstances and
loadAccounts in parallel which causes a redundant no-op because loadAccounts
early-returns when selectedInstanceId is null and the [selectedInstanceId,
savedOnly] watcher then calls loadAccounts again after loadInstances sets the
id; fix by making refreshAccounts call await loadInstances() first, capture the
prior selectedInstanceId before the call, and only call loadAccounts()
afterwards if selectedInstanceId is non-null and equal to the prior value (i.e.,
the instance id did not change), so the watcher handles the case where
loadInstances sets a new id.
- Around line 518-705: The grid's xl columns are fixed causing wasted space when
no form is open; change the container div's static class
"xl:grid-cols-[minmax(0,1fr)_minmax(21rem,0.34fr)]" to a dynamic class bound to
a computed or method (e.g. computed name gridColsClass or getGridColsClass) that
returns 'xl:grid-cols-[minmax(0,1fr)_minmax(21rem,0.34fr)]' when activeFormMode
is truthy and 'xl:grid-cols-1' (or equivalent single-column class) when falsy,
then update the div to use :class="['grid gap-6', gridColsClass]" so the
right-hand Card collapses away when no form is open (refer to the container div,
activeFormMode, and the new computed gridColsClass).
In `@frontend/src/features/instance-operations/pages/DatabaseManagementPage.vue`:
- Around line 247-251: The Refresh button only checks loadingDatabases but
refreshDatabases() awaits both loadInstances() and loadDatabases(), so update
the button disabling logic to prevent clicks while either phase is running by
using both flags (e.g., change the :disabled binding on the Refresh Button to
combine loadingDatabases and loadingInstances), and ensure the component
exposes/sets loadingInstances around the loadInstances() call so the combined
check is reliable.
In `@frontend/src/features/instance-operations/pages/ParameterSettingsPage.vue`:
- Around line 166-224: The initial mount triggers duplicate calls because
refreshAll() runs loadInstances(), loadParams(), and loadHistory() in parallel
while selectedInstanceId is still null, then the watcher on [selectedInstanceId,
editableFilter] fires when loadInstances sets selectedInstanceId; to fix, change
refreshAll() to await loadInstances() first and only then call loadParams() and
loadHistory() (e.g., await loadInstances(); await Promise.all([loadParams(),
loadHistory()])) so the watcher handles subsequent loads, or alternatively
conditionally skip calling loadParams()/loadHistory() in refreshAll() when
selectedInstanceId.value is falsy; update the refreshAll function (and keep the
watcher and onMounted flow) accordingly to eliminate the redundant early no-op
calls.
In `@frontend/src/features/instance-operations/pages/SessionDiagnosticsPage.vue`:
- Around line 102-105: The current columns computed uses
Object.keys(firstRow).slice(0, 10) which silently drops columns; change it to
keep the full key list (const columns = computed(() => { const firstRow =
rows.value[0]; return firstRow ? Object.keys(firstRow) : [] })) or, if you must
limit for UI, create two computeds: displayColumns (first 10) and
extraColumnsCount (computed from Object.keys(firstRow).length -
displayColumns.length) and update the template to render displayColumns plus a
"+{extraColumnsCount} more columns" indicator; update references to columns in
the component accordingly (look for the columns computed and any template
usages).
- Around line 115-129: processId(row) is being called multiple times and its
truthiness treats a valid numeric id of 0 as "no id"; compute the id once and
use explicit null checks instead of truthy checks. Update the code paths that
render/disable/compare (calls around processId(row), toggleThread, and any
template bindings using processId) to call processId(row) a single time into a
const (e.g., const id = processId(row)), reuse that id in selectedThreadIds
checks and in :disabled, and change conditions to id !== null (or id == null)
rather than relying on truthiness so an id of 0 is handled correctly; also
ensure toggleThread already uses the single computed id and does not re-call
processId.
In `@frontend/src/features/inventory/pages/DataDictionaryPage.vue`:
- Around line 261-269: The synchronous call to
window.URL.revokeObjectURL(objectUrl) may race with the download start after
anchor.click(); defer revocation to ensure the browser has time to begin the
download by replacing the immediate revoke with a microtask or macrotask (e.g.,
queueMicrotask or setTimeout(..., 0)) so revokeObjectURL(objectUrl) runs shortly
after click; update the block that creates objectUrl/anchor (the objectUrl
variable and anchor.click() usage in DataDictionaryPage.vue) to revoke the URL
asynchronously instead of immediately.
In `@frontend/src/lib/api.ts`:
- Around line 1457-1475: The guard in fetchWorkflowOperationAuditLogs currently
uses truthy checks (if (options.audit_id), if (options.workflow_id), if
(options.workflow_type)) which will skip legitimate zero values; update each
guard to check for presence rather than truthiness (e.g., use options.audit_id
!= null or options.audit_id !== undefined, and the same for workflow_id and
workflow_type) so 0 is included in the URLSearchParams when provided; keep the
rest of the function (building params and calling apiGet/extractData) unchanged.
In `@sql/migrations/0015_alter_permission_options.py`:
- Line 95: The data migration currently uses
migrations.RunPython(remove_retired_sql_permissions, migrations.RunPython.noop)
which leaves deleted auth.Permission rows unrecoverable on rollback; replace the
noop with a proper reverse function (e.g., a recreate_retired_sql_permissions
undo) that recreates the removed Permission rows (using the same codename,
content_type and name used by remove_retired_sql_permissions) or explicitly
document/confirm that rollbacks must not rely on restoring those rows; update
the RunPython call to migrations.RunPython(remove_retired_sql_permissions,
recreate_retired_sql_permissions) so rollbacks return the DB to the
pre-migration state.
In `@sql/tests.py`:
- Line 190: Remove unnecessary f-string prefixes from literal URLs in the tests:
replace calls like self.client.get(f"/database/", data=data) with
self.client.get("/database/", data=data) (and similarly for the other
occurrences flagged by Ruff). Locate the self.client.get calls in sql/tests.py
that use f-strings without any interpolation and convert them to normal string
literals so Ruff F541 is resolved.
In `@sql/views.py`:
- Around line 51-52: The spa_redirect helper is redundant—remove the
spa_redirect function and replace its usages (calls to spa_redirect(...)) with
direct returns of HttpResponseRedirect(path); search for the spa_redirect symbol
and update call sites to use HttpResponseRedirect(path) so behavior remains
identical, then delete the spa_redirect definition from the module to avoid dead
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 356589c9-69b2-4149-88fb-34edac17c76d
📒 Files selected for processing (91)
README.mdapi_admin/settings.pyapi_audit/__init__.pyapi_audit/serializers.pyapi_audit/tests.pyapi_audit/urls.pyapi_audit/views.pyapi_instances/serializers.pyapi_instances/tests.pyapi_instances/urls.pyapi_instances/views.pycommon/templates/base.htmlcommon/templates/config.htmldocs/spa-bootstrap-parity-checklist.mdfrontend/src/app/feature-registry.tsfrontend/src/features/archives/pages/ArchiveCreatePage.vuefrontend/src/features/archives/pages/ArchiveDetailPage.vuefrontend/src/features/archives/pages/ArchivesPage.vuefrontend/src/features/audit/api.tsfrontend/src/features/audit/manifest.tsfrontend/src/features/audit/pages/AuditPage.vuefrontend/src/features/instance-operations/api.tsfrontend/src/features/instance-operations/manifest.tsfrontend/src/features/instance-operations/pages/AccountManagementPage.vuefrontend/src/features/instance-operations/pages/DatabaseManagementPage.vuefrontend/src/features/instance-operations/pages/ParameterSettingsPage.vuefrontend/src/features/instance-operations/pages/SessionDiagnosticsPage.vuefrontend/src/features/inventory/api.tsfrontend/src/features/inventory/manifest.tsfrontend/src/features/inventory/pages/DataDictionaryPage.vuefrontend/src/features/inventory/pages/InventoryEditorPage.vuefrontend/src/features/inventory/pages/InventoryListPage.vuefrontend/src/features/permissions/pages/PermissionManagementPage.vuefrontend/src/features/queries/pages/QueriesPage.vuefrontend/src/features/settings/pages/SettingsGroupDetailPage.vuefrontend/src/features/settings/pages/SettingsGroupsPage.vuefrontend/src/features/settings/pages/SettingsInstanceTagDetailPage.vuefrontend/src/features/settings/pages/SettingsInstanceTagsPage.vuefrontend/src/features/settings/pages/SettingsResourceGroupDetailPage.vuefrontend/src/features/settings/pages/SettingsResourceGroupsPage.vuefrontend/src/features/settings/system-settings.tsfrontend/src/features/workflows/pages/WorkflowDetailPage.vuefrontend/src/features/workflows/pages/WorkflowSqlEditorPage.vuefrontend/src/features/workflows/pages/WorkflowsPage.vuefrontend/src/lib/api.tsfrontend/tests/e2e/spa-parity.spec.tsrequirements.txtsql/binlog.pysql/fixtures/auth_group.sqlsql/instance.pysql/local_demo.pysql/migrations/0015_alter_permission_options.pysql/models.pysql/notify.pysql/plugins/my2sql.pysql/plugins/schemasync.pysql/plugins/soar.pysql/plugins/sqladvisor.pysql/plugins/tests.pysql/slowlog.pysql/sql_analyze.pysql/sql_optimize.pysql/templates/audit.htmlsql/templates/audit_sqlquery.htmlsql/templates/audit_sqlworkflow.htmlsql/templates/data_dictionary.htmlsql/templates/database.htmlsql/templates/dbaprinciples.htmlsql/templates/dbdiagnostic.htmlsql/templates/instanceaccount.htmlsql/templates/my2sql.htmlsql/templates/param.htmlsql/templates/schemasync.htmlsql/templates/slowquery.htmlsql/templates/sqladvisor.htmlsql/templates/sqlanalyze.htmlsql/test_notify.pysql/tests.pysql/urls.pysql/utils/sql_utils.pysql/views.pysql_api/urls.pysrc/charts/templates/configMap.yamlsrc/charts/values.yamlsrc/docker-compose/datamingle/soar.yamlsrc/docker-compose/docker-compose.local-arm.ymlsrc/docker-compose/docker-compose.ymlsrc/docker/Dockerfilesrc/docker/Dockerfile-basesrc/docker/setup.shsrc/plugins/soar.yaml
💤 Files with no reviewable changes (38)
- requirements.txt
- src/docker-compose/docker-compose.yml
- src/docker-compose/docker-compose.local-arm.yml
- src/docker/Dockerfile
- src/plugins/soar.yaml
- common/templates/config.html
- src/docker/Dockerfile-base
- sql/templates/audit.html
- sql/templates/audit_sqlworkflow.html
- sql/plugins/schemasync.py
- README.md
- sql/plugins/sqladvisor.py
- sql/templates/dbaprinciples.html
- sql/plugins/my2sql.py
- api_admin/settings.py
- src/docker/setup.sh
- sql/plugins/soar.py
- sql/templates/dbdiagnostic.html
- src/charts/values.yaml
- sql/sql_analyze.py
- sql/templates/param.html
- sql/templates/audit_sqlquery.html
- sql/templates/my2sql.html
- sql/test_notify.py
- sql/templates/sqladvisor.html
- sql/binlog.py
- src/docker-compose/datamingle/soar.yaml
- sql/sql_optimize.py
- sql/instance.py
- sql/templates/schemasync.html
- sql/templates/data_dictionary.html
- sql/slowlog.py
- sql/templates/sqlanalyze.html
- sql/models.py
- sql/templates/slowquery.html
- sql/local_demo.py
- sql/urls.py
- sql/templates/database.html
Comment on lines
+5
to
+24
| RETIRED_SQL_PERMISSION_CODENAMES = ( | ||
| "menu_sqlanalyze", | ||
| "menu_sqloptimize", | ||
| "menu_sqladvisor", | ||
| "menu_slowquery", | ||
| "menu_my2sql", | ||
| "menu_schemasync", | ||
| "menu_document", | ||
| "sql_analyze", | ||
| "optimize_sqladvisor", | ||
| "optimize_sqltuning", | ||
| "optimize_soar", | ||
| ) | ||
|
|
||
|
|
||
| def remove_retired_sql_permissions(apps, schema_editor): | ||
| Permission = apps.get_model("auth", "Permission") | ||
| Permission.objects.filter( | ||
| content_type__app_label="sql", codename__in=RETIRED_SQL_PERMISSION_CODENAMES | ||
| ).delete() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any remaining references to retired permission codenames across the repo.
codenames=(
menu_sqlanalyze menu_sqloptimize menu_sqladvisor menu_slowquery
menu_my2sql menu_schemasync menu_document
sql_analyze optimize_sqladvisor optimize_sqltuning optimize_soar
)
for cn in "${codenames[@]}"; do
echo "=== $cn ==="
rg -nP "\b${cn}\b" -g '!**/migrations/**' -g '!**/*.pyc'
doneRepository: jruszo/Datamingle
Length of output: 2362
Coordinate seed data with migration: seed SQL files reinsert deleted permissions.
Seed SQL files (src/init_sql/v*.sql) contain INSERT statements for the exact codenames this migration deletes:
menu_sqlanalyze,menu_sqloptimize,menu_sqladvisor,menu_slowquery(v1.2.0.sql)menu_my2sql(v1.8.3.sql)menu_schemasync(v1.3.2.sql)menu_document,sql_analyze,optimize_sqladvisor,optimize_sqltuning(v1.2.0.sql, v1.4.5.sql)
If seed data runs before migrations, these permissions are inserted then immediately deleted. Remove these INSERT statements from the seed SQL files, or clarify the intended initialization order.
Test and documentation references to these codenames (e.g., /sql_analyze/ endpoints in api_instances/tests.py) are URL paths, not permission checks, so they are not affected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sql/migrations/0015_alter_permission_options.py` around lines 5 - 24, The
migration remove_retired_sql_permissions deletes permissions listed in
RETIRED_SQL_PERMISSION_CODENAMES but seed SQL files still INSERT those same
codenames, causing them to be re-created then removed; fix by updating the seed
SQL files (src/init_sql/v*.sql) to remove or conditionally skip INSERTs for the
listed codenames (menu_sqlanalyze, menu_sqloptimize, menu_sqladvisor,
menu_slowquery, menu_my2sql, menu_schemasync, menu_document, sql_analyze,
optimize_sqladvisor, optimize_sqltuning, optimize_soar), or change
initialization ordering so seeds run after this migration, and add a short note
in docs/tests indicating these codenames are retired (referencing
RETIRED_SQL_PERMISSION_CODENAMES and remove_retired_sql_permissions to locate
the migration).
Comment on lines
+633
to
+648
| @permission_required("sql.audit_user", raise_exception=True) | ||
| def audit(request): | ||
| """General audit log page.""" | ||
| _action_types = AuditEntry.objects.values_list("action").distinct() | ||
| action_types = [i[0] for i in _action_types] | ||
| return render(request, "audit.html", {"action_types": action_types}) | ||
| return spa_redirect("/audit") | ||
|
|
||
|
|
||
| @permission_required("sql.audit_user", raise_exception=True) | ||
| def audit_sqlquery(request): | ||
| """Online SQL query audit page.""" | ||
| user = request.user | ||
| favorites = QueryLog.objects.filter(username=user.username, favorite=True).values( | ||
| "id", "alias" | ||
| ) | ||
| return render(request, "audit_sqlquery.html", {"favorites": favorites}) | ||
| return spa_redirect("/audit") | ||
|
|
||
|
|
||
| @permission_required("sql.audit_user", raise_exception=True) | ||
| def audit_sqlworkflow(request): | ||
| """SQL review workflow list page.""" | ||
| user = request.user | ||
| # Data for filter options | ||
| filter_dict = dict() | ||
| # Admin users can view all workflows | ||
| if user.is_superuser or user.has_perm("sql.audit_user"): | ||
| pass | ||
| # Non-admin users with review or resource-group execution permission | ||
| # can view all workflows in their groups. | ||
| elif user.has_perm("sql.sql_review") or user.has_perm( | ||
| "sql.sql_execute_for_resource_group" | ||
| ): | ||
| # Get the user's resource groups first | ||
| group_list = user_groups(user) | ||
| group_ids = [group.group_id for group in group_list] | ||
| filter_dict["group_id__in"] = group_ids | ||
| # Everyone else can only view their own workflows | ||
| else: | ||
| filter_dict["engineer"] = user.username | ||
| instance_id = ( | ||
| SqlWorkflow.objects.filter(**filter_dict).values("instance_id").distinct() | ||
| ) | ||
| instance = Instance.objects.filter(pk__in=instance_id) | ||
| resource_group_id = ( | ||
| SqlWorkflow.objects.filter(**filter_dict).values("group_id").distinct() | ||
| ) | ||
| resource_group = ResourceGroup.objects.filter(group_id__in=resource_group_id) | ||
|
|
||
| return render( | ||
| request, | ||
| "audit_sqlworkflow.html", | ||
| { | ||
| "status_list": SQL_WORKFLOW_CHOICES, | ||
| "instance": instance, | ||
| "resource_group": resource_group, | ||
| }, | ||
| ) | ||
| return spa_redirect("/audit") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm AuditPage.vue handles a query param for tab/category selection.
fd -t f 'AuditPage.vue' frontend
fd -t f 'AuditPage.vue' frontend --exec rg -nP -C3 '\b(useRoute|route\.query|tab)\b' {}Repository: jruszo/Datamingle
Length of output: 109
🏁 Script executed:
cat frontend/src/features/audit/pages/AuditPage.vueRepository: jruszo/Datamingle
Length of output: 14186
🏁 Script executed:
rg -n 'def spa_redirect' sql/Repository: jruszo/Datamingle
Length of output: 101
🏁 Script executed:
sed -n '51,65p' sql/views.pyRepository: jruszo/Datamingle
Length of output: 441
Three audit endpoints collapse to the same /audit URL — bookmarks/deep links lose context.
audit, audit_sqlquery, and audit_sqlworkflow all redirect to /audit, losing the distinction between general, query, and workflow audit pages. An existing bookmark to the SQL-workflow audit will land on the general tab instead of the intended category.
The suggested approach of passing tab parameters (e.g., /audit?tab=general, /audit?tab=query, /audit?tab=sql-workflow) is valid but requires updates to AuditPage.vue. Currently, the component initializes activeView to 'general' on mount and does not read query parameters from the route. Add query parameter handling to onMounted() to read and select the correct tab before loading data.
♻️ Suggested changes
Backend (sql/views.py):
`@permission_required`("sql.audit_user", raise_exception=True)
def audit(request):
"""General audit log page."""
- return spa_redirect("/audit")
+ return spa_redirect("/audit?tab=general")
`@permission_required`("sql.audit_user", raise_exception=True)
def audit_sqlquery(request):
"""Online SQL query audit page."""
- return spa_redirect("/audit")
+ return spa_redirect("/audit?tab=query")
`@permission_required`("sql.audit_user", raise_exception=True)
def audit_sqlworkflow(request):
"""SQL review workflow list page."""
- return spa_redirect("/audit")
+ return spa_redirect("/audit?tab=sql-workflow")Frontend (frontend/src/features/audit/pages/AuditPage.vue):
onMounted(async () => {
await authStore.loadCurrentUser()
+ const route = useRoute()
+ const tabParam = route.query.tab
+ if (tabParam === 'query') activeView.value = 'query'
+ else if (tabParam === 'sql-workflow') activeView.value = 'workflow'
await loadAuditLogs()
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sql/views.py` around lines 633 - 648, The three view functions audit,
audit_sqlquery, and audit_sqlworkflow currently all redirect to "/audit", which
loses tab context; update each to append a tab query parameter (e.g.,
"/audit?tab=general", "/audit?tab=query", "/audit?tab=sql-workflow") so
bookmarks retain intent, and then update the frontend AuditPage.vue to read the
route query in its onMounted() hook: have onMounted() inspect route.query.tab
(falling back to 'general'), set activeView accordingly, and only then trigger
data loading so the correct tab content is displayed on initial load.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
frontend/src/features/instance-operations/pages/AccountManagementPage.vue (1)
441-449:⚠️ Potential issue | 🟡 MinorHandle
loadCurrentUser()failures before checking permissions.If the session refresh rejects, this page exits through an unhandled promise rejection and never surfaces a user-facing error.
DatabaseManagementPage.vuealready wraps the same call intoUserFacingMessage; this page needs the same guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/instance-operations/pages/AccountManagementPage.vue` around lines 441 - 449, The onMounted hook calls authStore.loadCurrentUser() without handling rejections, causing unhandled promise rejections before the permission check; wrap the call in toUserFacingMessage (like DatabaseManagementPage.vue) and set error.value with the returned user-facing message if it fails, then return early before checking canViewAccounts and before calling refreshAccounts; modify the onMounted block around authStore.loadCurrentUser(), canViewAccounts, error.value, and refreshAccounts to use this guarded pattern.
🧹 Nitpick comments (5)
sql/plugins/tests.py (1)
107-114: Strengthentest_plugin_generate_args2cmdassertion.
assertIsInstance(cmd_args, list)only checks the return type and would pass even ifgenerate_args2cmdreturned[]or had a regression in argument formatting. Consider asserting the actual command-arg structure produced byPlugin.generate_args2cmd.♻️ Proposed refinement
def test_plugin_generate_args2cmd(self): """ Test argument conversion. :return: """ plugin = Plugin(path="/usr/bin/example") cmd_args = plugin.generate_args2cmd({"query": "select 1;"}) - self.assertIsInstance(cmd_args, list) + self.assertEqual(cmd_args, ["/usr/bin/example", "-query", "select 1;"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/plugins/tests.py` around lines 107 - 114, The test test_plugin_generate_args2cmd is too weak—replace the lone assertIsInstance with concrete assertions that validate the produced command argument structure from Plugin.generate_args2cmd; call Plugin(path="/usr/bin/example").generate_args2cmd({"query":"select 1;"}) and assert the list is non-empty, that it includes the query string ("select 1;") in one of the elements, and that the command/binary (plugin.path or "/usr/bin/example") appears in the args (or assert equality against the exact expected list if the expected format is known) so regressions in formatting or missing args are caught.frontend/src/features/instance-operations/pages/ParameterSettingsPage.vue (1)
222-228: Watcher races withrefreshAllon first instance selection.
refreshAll()(called inonMounted) callsloadInstances(), which setsselectedInstanceId.valueto the first instance when one is loaded. That triggers thiswatchcallback synchronously and kicks offloadParams()/loadHistory()concurrently with the awaitedPromise.all([loadParams(), loadHistory()])already running insiderefreshAll(well, insideifblock guarded bypriorInstanceId). On a fresh mountpriorInstanceId === null, sorefreshAllskips the innerPromise.all, but the watcher will still fire and load. That part works out, but it's fragile — any future change torefreshAll's guard could double-load.Consider gating the watcher on a
refreshing-style flag (asDataDictionaryPage.vuedoes) to make the intent explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/instance-operations/pages/ParameterSettingsPage.vue` around lines 222 - 228, The watcher on [selectedInstanceId, editableFilter] races with refreshAll; add a reactive flag (e.g., refreshing or isRefreshing) used by refreshAll() and the watcher: set refreshing.value = true at the start of refreshAll() and clear it in a finally block after its await-ed Promise.all finishes, then update the watcher (the callback that currently calls closeEdit() and void Promise.all([loadParams(), loadHistory()])) to early-return when refreshing.value is true so it won’t trigger concurrent loads; keep calls to closeEdit() only when not refreshing and keep using loadParams() and loadHistory() as before.frontend/src/features/audit/pages/AuditPage.vue (1)
208-215: Tab is parsed only on mount; SPA navigation between?tab=values won't re-sync.
route.query.tabis read insideonMountedonce. If a user navigates to/audit?tab=querywhile already on/audit?tab=generalvia<RouterLink>or programmatic push, the component is reused (same route name) andactiveViewwon't update. If that flow exists (e.g., sidebar nav or deep links cross-tab), consider awatch(() => route.query.tab, ...)to keep them in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/audit/pages/AuditPage.vue` around lines 208 - 215, The current logic that sets activeView (in the AuditPage.vue setup, where route and activeView are defined and currently initialized onMounted) only runs once and won't react to SPA navigation; add a watcher on route.query.tab (e.g., watch(() => route.query.tab, ...)) that applies the same parsing logic (use Array.isArray(route.query.tab) ? route.query.tab[0] : route.query.tab and map 'query' -> activeView.value='query', 'workflow' or 'sql-workflow' -> 'workflow', otherwise 'general') so activeView updates whenever the query param changes; keep the existing onMounted initialization but move the shared parsing logic into a small helper or reuse it from the watcher to avoid duplication.sql/views.py (1)
463-490: Optional: consider hardcoded path reuse and query-string preservation.The five redirects are straightforward and correctly preserve the
permission_requireddecorators. Two minor observations you may want to consider:
- The SPA target paths are duplicated as string literals here and again in
frontend/src/features/*/manifest.ts. If these legacy entry points stay long-term, a small constant module (e.g.,SPA_ROUTES) could prevent drift.HttpResponseRedirectdrops the original request's query string. If any external link/bookmark passes filters (e.g.,/database/?instance=foo), they're silently lost. If that's intentional for these legacy entries, ignore.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/views.py` around lines 463 - 490, The redirect handlers instanceaccount, database, dbdiagnostic, data_dictionary, and instance_param duplicate hardcoded SPA target strings and drop any incoming query string; refactor by extracting those target paths into a shared constant (e.g., SPA_ROUTES with keys for accounts, databases, diagnostics, data_dictionary, parameters) and update each view to read the appropriate SPA_ROUTES entry, then preserve the original request query string when redirecting (append request.META.get('QUERY_STRING') when non-empty) before returning HttpResponseRedirect so query parameters like ?instance=... are not lost.frontend/src/features/inventory/pages/DataDictionaryPage.vue (1)
98-113:normalizeRowsfallback for non-arrayrowsis suspect.When
rowsis a non-array, non-null primitive, the function returns[[rows]]— a 1×1 row matrix wrapping a scalar. This is unlikely to be useful and will silently render that scalar in the column-zero cell of a single row, regardless of column count. If a backend ever sends a malformed payload, you'll see a one-row table with a cryptic value rather than an explicit error. Consider returning[](and surfacing a console warning) for that branch instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/inventory/pages/DataDictionaryPage.vue` around lines 98 - 113, The normalizeRows function currently turns a non-array, non-null primitive rows into [[rows]] which can silently render invalid data; change normalizeRows so that when rows is neither an array nor null/undefined it returns [] and logs a console.warn (or use the existing logger) describing the unexpected rows payload; keep existing behavior for Array and empty array branches and ensure you update the branch that now returns [[rows]] (inside normalizeRows) to instead warn and return [].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api_audit/views.py`:
- Around line 203-223: The lookup passes raw strings to
WorkflowAudit.objects.get which can raise a 500 if workflow_id or workflow_type
are non-numeric; validate/convert both workflow_id and workflow_type before
calling WorkflowAudit.objects.get (similar to the audit_id handling): attempt to
cast workflow_id and workflow_type to int, raise serializers.ValidationError
with clear messages on ValueError, and only call
WorkflowAudit.objects.get(workflow_id=workflow_id, workflow_type=workflow_type)
after successful conversions.
In `@api_instances/serializers.py`:
- Around line 152-162: InstanceAccountPasswordSerializer currently accepts
whitespace-only passwords while InstanceAccountPayloadSerializer blocks them;
update InstanceAccountPasswordSerializer.password to use the same non-blank
validator used by InstanceAccountPayloadSerializer (e.g. add
validators=[validate_not_blank] or the project’s equivalent validator) so that
InstanceOperationAccountPassword.post cannot accept whitespace-only reset
passwords—modify the password field declaration in
InstanceAccountPasswordSerializer to include that validator.
In `@api_instances/views.py`:
- Around line 345-360: _account_metadata_fields currently only extracts host
from the quoted form "`user`@`host`", causing missing host when callers pass raw
"user@host"; update the MySQL branch in _account_metadata_fields to handle both
forms: read user_host = data.get("user_host",""), if user_host contains "@`"
keep existing rsplit logic, else if user_host contains "@" split on the last "@"
(user_host.rsplit("@",1)), strip any surrounding backticks or quotes from both
parts, and assign host (and user if missing) so metadata lookups match
_mysql_user_host and avoid duplicate/stale rows.
- Around line 1138-1145: The code uses InstanceDatabase.objects.create(...)
after creating the live DB which can raise IntegrityError if a metadata-only row
already exists; replace the create() call with an idempotent upsert such as
InstanceDatabase.objects.update_or_create((instance=instance, db_name=db_name),
defaults={'owner': owner, 'owner_display': owner_display, 'remark': remark}) or
equivalent get-or-create + update flow to avoid IntegrityError and ensure
existing metadata is updated; keep the subsequent
_clear_instance_resource_cache() call. Reference InstanceDatabase,
InstanceOperationDatabaseDetail.put, create(), and
_clear_instance_resource_cache().
In `@frontend/src/features/audit/pages/AuditPage.vue`:
- Around line 168-180: The pagination can be overwritten by a pending debounced
filter load; modify movePage to cancel or flush the debounced call before
changing page so a pending debounced handler doesn't reset filters.page to 1.
Specifically, call the cancel/flush method returned by useDebounceFn (refer to
debouncedLoadAuditLogs) or use the rejectOnCancel option and invoke
debouncedLoadAuditLogs.cancel()/flush() prior to setting filters.page and
calling loadAuditLogs(), ensuring loadAuditLogs() is the authoritative immediate
fetch after pagination.
In `@frontend/src/features/instance-operations/pages/AccountManagementPage.vue`:
- Around line 250-263: accountPayload() currently returns snake_case keys
instance_id and user but updateInstanceOperationAccount expects
payload.instanceId and payload.username when building the edit URL; change
accountPayload() (and the similar payload at lines 286-297) to return instanceId
(from selectedInstanceId.value) and username (from accountForm.user.trim())
along with the other fields (dbName, host, password, remark) so the API function
receives the correctly shaped payload and the edit URL is built without
undefined segments.
In `@frontend/src/features/instance-operations/pages/DatabaseManagementPage.vue`:
- Around line 157-162: The list call is using the wrong argument order and
payload keys for the frontend API helper: fetchInstanceOperationDatabases
expects (instanceId, token) and the API payloads use camelCase fields
instanceId/dbName, but the page passes (token, { instance_id, db_name, saved })
which makes the token treated as the instanceId and leaves path params undefined
for edits. Fix by calling fetchInstanceOperationDatabases with
selectedInstanceId.value as the first arg and requireToken() as the second, and
update any edit/patch payloads (the create/edit calls on this page) to use
camelCase keys instanceId and dbName (and pass the instanceId value from
selectedInstanceId.value) so the helper can build the correct URLs.
In `@frontend/src/features/instance-operations/pages/ParameterSettingsPage.vue`:
- Around line 207-220: The onMounted block awaits authStore.loadCurrentUser()
without error handling which can leave the page blank on network/token errors;
wrap the await in a try/catch, convert the caught error via toUserFacingMessage
and set error.value accordingly (then return) before proceeding to permission
checks and refreshAll; update the onMounted handler that calls
authStore.loadCurrentUser(), uses toUserFacingMessage, and ensures refreshAll is
only called on success.
In `@frontend/src/features/instance-operations/pages/SessionDiagnosticsPage.vue`:
- Around line 170-180: The diagnostic fetch calls are passing arguments in the
wrong order causing the token to be used as payload; update the calls where
activeTab === 'processes' and 'tablespace' to call
fetchInstanceOperationDiagnosticProcesses(payload, token) and
fetchInstanceOperationDiagnosticTablespace(payload, token) respectively (build
the payload using selectedInstanceId.value, commandType.value or size as before)
and pass requireToken() as the second argument so the payload (object) is the
first parameter and the auth token is the second; adjust the two call sites in
SessionDiagnosticsPage.vue accordingly.
---
Duplicate comments:
In `@frontend/src/features/instance-operations/pages/AccountManagementPage.vue`:
- Around line 441-449: The onMounted hook calls authStore.loadCurrentUser()
without handling rejections, causing unhandled promise rejections before the
permission check; wrap the call in toUserFacingMessage (like
DatabaseManagementPage.vue) and set error.value with the returned user-facing
message if it fails, then return early before checking canViewAccounts and
before calling refreshAccounts; modify the onMounted block around
authStore.loadCurrentUser(), canViewAccounts, error.value, and refreshAccounts
to use this guarded pattern.
---
Nitpick comments:
In `@frontend/src/features/audit/pages/AuditPage.vue`:
- Around line 208-215: The current logic that sets activeView (in the
AuditPage.vue setup, where route and activeView are defined and currently
initialized onMounted) only runs once and won't react to SPA navigation; add a
watcher on route.query.tab (e.g., watch(() => route.query.tab, ...)) that
applies the same parsing logic (use Array.isArray(route.query.tab) ?
route.query.tab[0] : route.query.tab and map 'query' ->
activeView.value='query', 'workflow' or 'sql-workflow' -> 'workflow', otherwise
'general') so activeView updates whenever the query param changes; keep the
existing onMounted initialization but move the shared parsing logic into a small
helper or reuse it from the watcher to avoid duplication.
In `@frontend/src/features/instance-operations/pages/ParameterSettingsPage.vue`:
- Around line 222-228: The watcher on [selectedInstanceId, editableFilter] races
with refreshAll; add a reactive flag (e.g., refreshing or isRefreshing) used by
refreshAll() and the watcher: set refreshing.value = true at the start of
refreshAll() and clear it in a finally block after its await-ed Promise.all
finishes, then update the watcher (the callback that currently calls closeEdit()
and void Promise.all([loadParams(), loadHistory()])) to early-return when
refreshing.value is true so it won’t trigger concurrent loads; keep calls to
closeEdit() only when not refreshing and keep using loadParams() and
loadHistory() as before.
In `@frontend/src/features/inventory/pages/DataDictionaryPage.vue`:
- Around line 98-113: The normalizeRows function currently turns a non-array,
non-null primitive rows into [[rows]] which can silently render invalid data;
change normalizeRows so that when rows is neither an array nor null/undefined it
returns [] and logs a console.warn (or use the existing logger) describing the
unexpected rows payload; keep existing behavior for Array and empty array
branches and ensure you update the branch that now returns [[rows]] (inside
normalizeRows) to instead warn and return [].
In `@sql/plugins/tests.py`:
- Around line 107-114: The test test_plugin_generate_args2cmd is too
weak—replace the lone assertIsInstance with concrete assertions that validate
the produced command argument structure from Plugin.generate_args2cmd; call
Plugin(path="/usr/bin/example").generate_args2cmd({"query":"select 1;"}) and
assert the list is non-empty, that it includes the query string ("select 1;") in
one of the elements, and that the command/binary (plugin.path or
"/usr/bin/example") appears in the args (or assert equality against the exact
expected list if the expected format is known) so regressions in formatting or
missing args are caught.
In `@sql/views.py`:
- Around line 463-490: The redirect handlers instanceaccount, database,
dbdiagnostic, data_dictionary, and instance_param duplicate hardcoded SPA target
strings and drop any incoming query string; refactor by extracting those target
paths into a shared constant (e.g., SPA_ROUTES with keys for accounts,
databases, diagnostics, data_dictionary, parameters) and update each view to
read the appropriate SPA_ROUTES entry, then preserve the original request query
string when redirecting (append request.META.get('QUERY_STRING') when non-empty)
before returning HttpResponseRedirect so query parameters like ?instance=... are
not lost.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e44f2d4a-ed7f-4cf9-b957-9d5b0227f9ee
📒 Files selected for processing (67)
README.mdadmin.shapi_admin/dashboard.pyapi_audit/tests.pyapi_audit/urls.pyapi_audit/views.pyapi_instances/serializers.pyapi_instances/tests.pyapi_instances/urls.pyapi_instances/views.pycommon/dashboard.pycommon/templates/dashboard.htmlcommon/utils/chart_dao.pydocs/spa-bootstrap-parity-checklist.mdfrontend/src/features/audit/pages/AuditPage.vuefrontend/src/features/dashboard/pages/HomePage.vuefrontend/src/features/instance-operations/manifest.tsfrontend/src/features/instance-operations/pages/AccountManagementPage.vuefrontend/src/features/instance-operations/pages/DatabaseManagementPage.vuefrontend/src/features/instance-operations/pages/ParameterSettingsPage.vuefrontend/src/features/instance-operations/pages/SessionDiagnosticsPage.vuefrontend/src/features/inventory/pages/DataDictionaryPage.vuefrontend/src/lib/api.tssql/migrations/0015_alter_permission_options.pysql/plugins/tests.pysql/tests.pysql/views.pysrc/charts/templates/configMap.yamlsrc/charts/values.yamlsrc/docker-compose/mysql/my.cnfsrc/init_sql/del_permissions.sqlsrc/init_sql/goinception_param_template.sqlsrc/init_sql/mysql_slow_query_review.sqlsrc/init_sql/rds_param_template.sqlsrc/init_sql/v1.0_init.sqlsrc/init_sql/v1.1.0.sqlsrc/init_sql/v1.10.0.sqlsrc/init_sql/v1.12.0.sqlsrc/init_sql/v1.13.0.sqlsrc/init_sql/v1.2.0.sqlsrc/init_sql/v1.3.0.sqlsrc/init_sql/v1.3.2.sqlsrc/init_sql/v1.3.7.sqlsrc/init_sql/v1.4.0.sqlsrc/init_sql/v1.4.3.sqlsrc/init_sql/v1.4.5.sqlsrc/init_sql/v1.5.0.sqlsrc/init_sql/v1.5.3_comment.sqlsrc/init_sql/v1.6.0.sqlsrc/init_sql/v1.6.1.sqlsrc/init_sql/v1.6.2.sqlsrc/init_sql/v1.6.3.sqlsrc/init_sql/v1.6.6.sqlsrc/init_sql/v1.6.7.sqlsrc/init_sql/v1.7.0.sqlsrc/init_sql/v1.7.1.sqlsrc/init_sql/v1.7.11.sqlsrc/init_sql/v1.7.12.sqlsrc/init_sql/v1.7.2.sqlsrc/init_sql/v1.7.3.sqlsrc/init_sql/v1.7.5.sqlsrc/init_sql/v1.7.7.sqlsrc/init_sql/v1.7.8.sqlsrc/init_sql/v1.8.3.sqlsrc/init_sql/v1.8.4.sqlsrc/init_sql/v1.9.0.sqlsrc/script/analysis_slow_query.sh
💤 Files with no reviewable changes (25)
- admin.sh
- src/docker-compose/mysql/my.cnf
- src/init_sql/v1.3.7.sql
- src/init_sql/v1.3.2.sql
- src/init_sql/v1.10.0.sql
- src/init_sql/v1.3.0.sql
- src/init_sql/v1.13.0.sql
- src/init_sql/v1.4.0.sql
- src/init_sql/v1.12.0.sql
- src/init_sql/del_permissions.sql
- api_admin/dashboard.py
- src/init_sql/goinception_param_template.sql
- src/init_sql/v1.4.3.sql
- common/templates/dashboard.html
- src/init_sql/rds_param_template.sql
- src/init_sql/v1.1.0.sql
- src/init_sql/v1.2.0.sql
- src/charts/values.yaml
- common/dashboard.py
- src/init_sql/mysql_slow_query_review.sql
- src/init_sql/v1.5.0.sql
- frontend/src/features/dashboard/pages/HomePage.vue
- src/init_sql/v1.0_init.sql
- common/utils/chart_dao.py
- src/init_sql/v1.4.5.sql
✅ Files skipped from review due to trivial changes (1)
- docs/spa-bootstrap-parity-checklist.md
🚧 Files skipped from review as they are similar to previous changes (5)
- api_audit/urls.py
- frontend/src/features/instance-operations/manifest.ts
- src/charts/templates/configMap.yaml
- README.md
- frontend/src/lib/api.ts
Comment on lines
+203
to
+223
| audit_id = request.query_params.get("audit_id", "").strip() | ||
| workflow_id = request.query_params.get("workflow_id", "").strip() | ||
| workflow_type = request.query_params.get("workflow_type", "").strip() or "2" | ||
|
|
||
| if not audit_id and not workflow_id: | ||
| raise serializers.ValidationError( | ||
| {"errors": "audit_id or workflow_id is required."} | ||
| ) | ||
|
|
||
| if audit_id: | ||
| try: | ||
| audit_id = int(audit_id) | ||
| except ValueError as exc: | ||
| raise serializers.ValidationError( | ||
| {"audit_id": "A valid integer is required."} | ||
| ) from exc | ||
| else: | ||
| try: | ||
| audit_id = WorkflowAudit.objects.get( | ||
| workflow_id=workflow_id, workflow_type=workflow_type | ||
| ).audit_id |
Contributor
There was a problem hiding this comment.
Validate workflow_id and workflow_type before the lookup.
The new audit_id guard fixes one 500 path, but WorkflowAudit.objects.get(workflow_id=workflow_id, workflow_type=workflow_type) still receives raw strings. A non-numeric workflow_id or workflow_type will raise during queryset prep and bypass the API's 400 validation flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api_audit/views.py` around lines 203 - 223, The lookup passes raw strings to
WorkflowAudit.objects.get which can raise a 500 if workflow_id or workflow_type
are non-numeric; validate/convert both workflow_id and workflow_type before
calling WorkflowAudit.objects.get (similar to the audit_id handling): attempt to
cast workflow_id and workflow_type to int, raise serializers.ValidationError
with clear messages on ValueError, and only call
WorkflowAudit.objects.get(workflow_id=workflow_id, workflow_type=workflow_type)
after successful conversions.
Comment on lines
+152
to
+162
| class InstanceAccountPasswordSerializer(serializers.Serializer): | ||
| instance_id = serializers.IntegerField() | ||
| db_name = serializers.CharField(max_length=128, allow_blank=True, required=False) | ||
| db_name_user = serializers.CharField( | ||
| max_length=260, allow_blank=True, required=False | ||
| ) | ||
| user_host = serializers.CharField(max_length=260, allow_blank=True, required=False) | ||
| user = serializers.CharField(max_length=128) | ||
| host = serializers.CharField(max_length=64, allow_blank=True, required=False) | ||
| password = serializers.CharField(max_length=128, write_only=True) | ||
|
|
Contributor
There was a problem hiding this comment.
Apply the same non-blank password validation to reset requests.
InstanceAccountPayloadSerializer rejects whitespace-only passwords, but InstanceAccountPasswordSerializer doesn't. In InstanceOperationAccountPassword.post, a value like " " is truthy, so it bypasses the required-field check and can be accepted as a reset password.
Proposed fix
class InstanceAccountPasswordSerializer(serializers.Serializer):
instance_id = serializers.IntegerField()
db_name = serializers.CharField(max_length=128, allow_blank=True, required=False)
db_name_user = serializers.CharField(
max_length=260, allow_blank=True, required=False
)
user_host = serializers.CharField(max_length=260, allow_blank=True, required=False)
user = serializers.CharField(max_length=128)
host = serializers.CharField(max_length=64, allow_blank=True, required=False)
password = serializers.CharField(max_length=128, write_only=True)
+
+ def validate_password(self, value):
+ if not value.strip():
+ raise serializers.ValidationError("Password cannot be blank.")
+ return value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api_instances/serializers.py` around lines 152 - 162,
InstanceAccountPasswordSerializer currently accepts whitespace-only passwords
while InstanceAccountPayloadSerializer blocks them; update
InstanceAccountPasswordSerializer.password to use the same non-blank validator
used by InstanceAccountPayloadSerializer (e.g. add
validators=[validate_not_blank] or the project’s equivalent validator) so that
InstanceOperationAccountPassword.post cannot accept whitespace-only reset
passwords—modify the password field declaration in
InstanceAccountPasswordSerializer to include that validator.
Comment on lines
+345
to
+360
| def _account_metadata_fields(instance, data): | ||
| user = data.get("user", "") | ||
| host = data.get("host", "") | ||
| db_name = data.get("db_name", "") | ||
| if instance.db_type == "mysql" and not host: | ||
| user_host = data.get("user_host", "") | ||
| if user_host and "@`" in user_host: | ||
| try: | ||
| host = user_host.rsplit("@`", 1)[1].rstrip("`") | ||
| except IndexError: | ||
| host = "" | ||
| elif instance.db_type == "mongo" and not db_name: | ||
| db_name_user = data.get("db_name_user", "") | ||
| if "." in db_name_user: | ||
| db_name, user = db_name_user.split(".", 1) | ||
| return user, host, db_name |
Contributor
There was a problem hiding this comment.
Parse raw user@host values here too.
_mysql_user_host() accepts both `user`@`host` and raw user@host, but _account_metadata_fields() only back-fills host from the quoted form. For password-reset and delete flows that supply user_host without host, the live engine call succeeds while the metadata lookup uses host="", which can create a duplicate row on reset or leave stale metadata on delete.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api_instances/views.py` around lines 345 - 360, _account_metadata_fields
currently only extracts host from the quoted form "`user`@`host`", causing
missing host when callers pass raw "user@host"; update the MySQL branch in
_account_metadata_fields to handle both forms: read user_host =
data.get("user_host",""), if user_host contains "@`" keep existing rsplit logic,
else if user_host contains "@" split on the last "@" (user_host.rsplit("@",1)),
strip any surrounding backticks or quotes from both parts, and assign host (and
user if missing) so metadata lookups match _mysql_user_host and avoid
duplicate/stale rows.
Comment on lines
+1138
to
+1145
| database_record = InstanceDatabase.objects.create( | ||
| instance=instance, | ||
| db_name=db_name, | ||
| owner=owner, | ||
| owner_display=owner_display, | ||
| remark=remark, | ||
| ) | ||
| _clear_instance_resource_cache() |
Contributor
There was a problem hiding this comment.
Don't use create() after the live database operation.
InstanceDatabase is unique on (instance, db_name), and InstanceOperationDatabaseDetail.put already allows metadata-only upserts for the same key. If a saved metadata row exists before the live create runs, this create() raises IntegrityError after the database has already been created on the instance, leaving the API in a partial-success state.
Proposed fix
- database_record = InstanceDatabase.objects.create(
- instance=instance,
- db_name=db_name,
- owner=owner,
- owner_display=owner_display,
- remark=remark,
- )
+ database_record, _ = InstanceDatabase.objects.update_or_create(
+ instance=instance,
+ db_name=db_name,
+ defaults={
+ "owner": owner,
+ "owner_display": owner_display,
+ "remark": remark,
+ },
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| database_record = InstanceDatabase.objects.create( | |
| instance=instance, | |
| db_name=db_name, | |
| owner=owner, | |
| owner_display=owner_display, | |
| remark=remark, | |
| ) | |
| _clear_instance_resource_cache() | |
| database_record, _ = InstanceDatabase.objects.update_or_create( | |
| instance=instance, | |
| db_name=db_name, | |
| defaults={ | |
| "owner": owner, | |
| "owner_display": owner_display, | |
| "remark": remark, | |
| }, | |
| ) | |
| _clear_instance_resource_cache() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api_instances/views.py` around lines 1138 - 1145, The code uses
InstanceDatabase.objects.create(...) after creating the live DB which can raise
IntegrityError if a metadata-only row already exists; replace the create() call
with an idempotent upsert such as
InstanceDatabase.objects.update_or_create((instance=instance, db_name=db_name),
defaults={'owner': owner, 'owner_display': owner_display, 'remark': remark}) or
equivalent get-or-create + update flow to avoid IntegrityError and ensure
existing metadata is updated; keep the subsequent
_clear_instance_resource_cache() call. Reference InstanceDatabase,
InstanceOperationDatabaseDetail.put, create(), and
_clear_instance_resource_cache().
Comment on lines
+168
to
+180
| function movePage(direction: -1 | 1) { | ||
| const nextPage = filters.page + direction | ||
| if (nextPage < 1) { | ||
| return | ||
| } | ||
| filters.page = nextPage | ||
| void loadAuditLogs() | ||
| } | ||
|
|
||
| const debouncedLoadAuditLogs = useDebounceFn(() => { | ||
| filters.page = 1 | ||
| void loadAuditLogs() | ||
| }, 250) |
Contributor
There was a problem hiding this comment.
Potential pagination/debounce race.
movePage calls loadAuditLogs() synchronously while debouncedLoadAuditLogs (250ms) may have a pending call from a recent filter change. The pending debounced call will fire after the page change and reset to page = 1, overwriting the user's pagination. Consider cancelling the debounce before paginating, e.g., with useDebounceFn's returned cancel handle, or have movePage also flush/cancel pending loads.
♻️ Suggested approach
-const debouncedLoadAuditLogs = useDebounceFn(() => {
+const debouncedLoadAuditLogs = useDebounceFn(() => {
filters.page = 1
void loadAuditLogs()
}, 250)
function movePage(direction: -1 | 1) {
const nextPage = filters.page + direction
if (nextPage < 1) {
return
}
+ // Drop any pending debounced reset to avoid clobbering the new page.
filters.page = nextPage
void loadAuditLogs()
}useDebounceFn exposes a cancellable form via useDebounceFn(fn, ms, { rejectOnCancel: false }) plus .cancel(); alternatively, switch to a manually managed timer and clear it in movePage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/audit/pages/AuditPage.vue` around lines 168 - 180, The
pagination can be overwritten by a pending debounced filter load; modify
movePage to cancel or flush the debounced call before changing page so a pending
debounced handler doesn't reset filters.page to 1. Specifically, call the
cancel/flush method returned by useDebounceFn (refer to debouncedLoadAuditLogs)
or use the rejectOnCancel option and invoke
debouncedLoadAuditLogs.cancel()/flush() prior to setting filters.page and
calling loadAuditLogs(), ensuring loadAuditLogs() is the authoritative immediate
fetch after pagination.
Comment on lines
+250
to
+263
| function accountPayload() { | ||
| if (!selectedInstanceId.value) { | ||
| throw new Error('Select an instance first.') | ||
| } | ||
|
|
||
| return { | ||
| instance_id: selectedInstanceId.value, | ||
| db_name: accountForm.dbName.trim(), | ||
| user: accountForm.user.trim(), | ||
| host: accountForm.host.trim(), | ||
| password: accountForm.password, | ||
| remark: accountForm.remark.trim(), | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Use the payload shape expected by updateInstanceOperationAccount.
frontend/src/lib/api.ts:1527-1560 builds the edit URL from payload.instanceId and payload.username, but accountPayload() returns instance_id and user. As written, edit submissions generate a URL with undefined path segments.
Also applies to: 286-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/instance-operations/pages/AccountManagementPage.vue`
around lines 250 - 263, accountPayload() currently returns snake_case keys
instance_id and user but updateInstanceOperationAccount expects
payload.instanceId and payload.username when building the edit URL; change
accountPayload() (and the similar payload at lines 286-297) to return instanceId
(from selectedInstanceId.value) and username (from accountForm.user.trim())
along with the other fields (dbName, host, password, remark) so the API function
receives the correctly shaped payload and the edit URL is built without
undefined segments.
Comment on lines
+157
to
+162
| try { | ||
| const response = await fetchInstanceOperationDatabases(requireToken(), { | ||
| instance_id: selectedInstanceId.value, | ||
| saved: savedOnly.value, | ||
| }) | ||
| databases.value = response.results |
Contributor
There was a problem hiding this comment.
Match these calls to the frontend API helper contract.
frontend/src/lib/api.ts:1495-1510 expects fetchInstanceOperationDatabases(instanceId, token) and builds the update URL from payload.instanceId / payload.dbName. Here the load call passes (token, queryObject), and the edit payload only contains instance_id / db_name, so the list request uses the token as the instance ID and the edit request resolves its patch URL with undefined path params.
Also applies to: 189-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/instance-operations/pages/DatabaseManagementPage.vue`
around lines 157 - 162, The list call is using the wrong argument order and
payload keys for the frontend API helper: fetchInstanceOperationDatabases
expects (instanceId, token) and the API payloads use camelCase fields
instanceId/dbName, but the page passes (token, { instance_id, db_name, saved })
which makes the token treated as the instanceId and leaves path params undefined
for edits. Fix by calling fetchInstanceOperationDatabases with
selectedInstanceId.value as the first arg and requireToken() as the second, and
update any edit/patch payloads (the create/edit calls on this page) to use
camelCase keys instanceId and dbName (and pass the instanceId value from
selectedInstanceId.value) so the helper can build the correct URLs.
Comment on lines
+207
to
+220
| onMounted(async () => { | ||
| await authStore.loadCurrentUser() | ||
|
|
||
| if (!canOpenParamMenu.value) { | ||
| error.value = 'You do not have permission to open parameter settings.' | ||
| return | ||
| } | ||
| if (!canViewParams.value) { | ||
| error.value = 'You do not have permission to view instance parameters.' | ||
| return | ||
| } | ||
|
|
||
| await refreshAll() | ||
| }) |
Contributor
There was a problem hiding this comment.
Unhandled rejection on initial load (same pattern previously flagged in AuditPage.vue/DataDictionaryPage.vue).
authStore.loadCurrentUser() is awaited bare, so a network blip or expired refresh token leaves the user on an empty page with no surfaced error. The other pages in this PR wrap this in try/catch + toUserFacingMessage; do the same here for consistency.
🛡️ Suggested fix
onMounted(async () => {
- await authStore.loadCurrentUser()
+ try {
+ await authStore.loadCurrentUser()
+ } catch (errorValue) {
+ error.value = toUserFacingMessage(errorValue, 'Failed to load the current user.')
+ return
+ }
if (!canOpenParamMenu.value) {
error.value = 'You do not have permission to open parameter settings.'
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onMounted(async () => { | |
| await authStore.loadCurrentUser() | |
| if (!canOpenParamMenu.value) { | |
| error.value = 'You do not have permission to open parameter settings.' | |
| return | |
| } | |
| if (!canViewParams.value) { | |
| error.value = 'You do not have permission to view instance parameters.' | |
| return | |
| } | |
| await refreshAll() | |
| }) | |
| onMounted(async () => { | |
| try { | |
| await authStore.loadCurrentUser() | |
| } catch (errorValue) { | |
| error.value = toUserFacingMessage(errorValue, 'Failed to load the current user.') | |
| return | |
| } | |
| if (!canOpenParamMenu.value) { | |
| error.value = 'You do not have permission to open parameter settings.' | |
| return | |
| } | |
| if (!canViewParams.value) { | |
| error.value = 'You do not have permission to view instance parameters.' | |
| return | |
| } | |
| await refreshAll() | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/instance-operations/pages/ParameterSettingsPage.vue`
around lines 207 - 220, The onMounted block awaits authStore.loadCurrentUser()
without error handling which can leave the page blank on network/token errors;
wrap the await in a try/catch, convert the caught error via toUserFacingMessage
and set error.value accordingly (then return) before proceeding to permission
checks and refreshAll; update the onMounted handler that calls
authStore.loadCurrentUser(), uses toUserFacingMessage, and ensures refreshAll is
only called on success.
Comment on lines
+170
to
+180
| try { | ||
| if (activeTab.value === 'processes') { | ||
| rows.value = (await fetchInstanceOperationDiagnosticProcesses(requireToken(), { | ||
| instance_id: selectedInstanceId.value, | ||
| command_type: commandType.value, | ||
| })).results | ||
| } else if (activeTab.value === 'tablespace') { | ||
| rows.value = (await fetchInstanceOperationDiagnosticTablespace(requireToken(), { | ||
| instance_id: selectedInstanceId.value, | ||
| size: 25, | ||
| })).results |
Contributor
There was a problem hiding this comment.
Flip the diagnostic fetch arguments.
frontend/src/lib/api.ts:1612-1620 and 1643-1675 define fetchInstanceOperationDiagnosticProcesses(payload, token) and fetchInstanceOperationDiagnosticTablespace(payload, token). These calls are reversed, so the access token is sent as the payload and the auth token becomes [object Object]; the Processes and Tablespace tabs will fail to load.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/instance-operations/pages/SessionDiagnosticsPage.vue`
around lines 170 - 180, The diagnostic fetch calls are passing arguments in the
wrong order causing the token to be used as payload; update the calls where
activeTab === 'processes' and 'tablespace' to call
fetchInstanceOperationDiagnosticProcesses(payload, token) and
fetchInstanceOperationDiagnosticTablespace(payload, token) respectively (build
the payload using selectedInstanceId.value, commandType.value or size as before)
and pass requireToken() as the second argument so the payload (object) is the
first parameter and the auth token is the second; adjust the two call sites in
SessionDiagnosticsPage.vue accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api_instances/views.py (1)
1118-1122: Legacyarchery-collection name in Mongo placeholder.The placeholder collection used to materialize a new Mongo database is hardcoded as
f"archery-{db_name}", which leaks the legacy product name into freshly-created Mongo instances even though this PR is rebranding to Datamingle. If you do change it, note that any existing deployments depending on the old name will break — so a migration/compat plan is needed before flipping it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api_instances/views.py` around lines 1118 - 1122, Replace the hardcoded legacy collection name "archery-{db_name}" with the new brand-safe name (e.g., "datamingle-{db_name}") where the Mongo placeholder is created in the block checking instance.db_type == "mongo" (look for the variables/identifiers exec_result, conn = engine.get_connection(), database = conn[db_name], and database.create_collection). Update the string used in database.create_collection to use the new prefix and ensure any surrounding comments/logs that reference "archery" are renamed; also add a TODO or comment near this change noting that a migration/compatibility plan is required for deployments relying on the old collection name so it can be rolled out safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sql/migrations/0015_alter_permission_options.py`:
- Around line 19-31: RETIRED_SQL_PERMISSION_NAMES contains two values that don't
match the originals from 0001_initial.py: update the entries for keys
"menu_document" and "sql_analyze" in RETIRED_SQL_PERMISSION_NAMES so they
exactly match the original display names ("Menu Related Documentation" and
"Execute SQL Analysis" respectively) so reverse migration (migrate sql 0014)
restores the exact prior name values; adjust the mapping in
sql/migrations/0015_alter_permission_options.py and re-run/verify the migration
rollback to ensure byte-equality with 0001_initial.
---
Nitpick comments:
In `@api_instances/views.py`:
- Around line 1118-1122: Replace the hardcoded legacy collection name
"archery-{db_name}" with the new brand-safe name (e.g., "datamingle-{db_name}")
where the Mongo placeholder is created in the block checking instance.db_type ==
"mongo" (look for the variables/identifiers exec_result, conn =
engine.get_connection(), database = conn[db_name], and
database.create_collection). Update the string used in
database.create_collection to use the new prefix and ensure any surrounding
comments/logs that reference "archery" are renamed; also add a TODO or comment
near this change noting that a migration/compatibility plan is required for
deployments relying on the old collection name so it can be rolled out safely.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3fe0f42-f628-4f82-b9f7-bbb226d9d24e
📒 Files selected for processing (7)
api_instances/serializers.pyapi_instances/views.pyfrontend/src/features/instance-operations/pages/DatabaseManagementPage.vuefrontend/src/features/instance-operations/pages/ParameterSettingsPage.vuefrontend/src/features/inventory/pages/DataDictionaryPage.vuesql/migrations/0015_alter_permission_options.pysql/test_query_privileges.py
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/features/instance-operations/pages/DatabaseManagementPage.vue
Comment on lines
+19
to
+31
| RETIRED_SQL_PERMISSION_NAMES = { | ||
| "menu_sqlanalyze": "Menu SQL Analysis", | ||
| "menu_sqloptimize": "Menu SQL Optimization", | ||
| "menu_sqladvisor": "Menu Optimization Tools", | ||
| "menu_slowquery": "Menu Slow Query Log", | ||
| "menu_my2sql": "Menu My2SQL", | ||
| "menu_schemasync": "Menu SchemaSync", | ||
| "menu_document": "Menu Documentation", | ||
| "sql_analyze": "Execute SQL analysis", | ||
| "optimize_sqladvisor": "Execute SQLAdvisor", | ||
| "optimize_sqltuning": "Execute SQLTuning", | ||
| "optimize_soar": "Execute SOAR", | ||
| } |
Contributor
There was a problem hiding this comment.
Reverse migration restores wrong display names for two permissions.
Compared with the originals defined in sql/migrations/0001_initial.py, two entries in RETIRED_SQL_PERMISSION_NAMES don't match what the reverse path should restore:
menu_document: originally"Menu Related Documentation", here"Menu Documentation".sql_analyze: originally"Execute SQL Analysis", here"Execute SQL analysis"(lowercasea).
A rollback (migrate sql 0014) will recreate the permissions with diverged name values, so the DB won't be byte-equal to pre-migration state. Codenames still match, so permission checks aren't affected — purely a fidelity issue for reverse migrations / admin display.
📝 Proposed fix to align names with 0001_initial
- "menu_document": "Menu Documentation",
+ "menu_document": "Menu Related Documentation",
- "sql_analyze": "Execute SQL analysis",
+ "sql_analyze": "Execute SQL Analysis",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| RETIRED_SQL_PERMISSION_NAMES = { | |
| "menu_sqlanalyze": "Menu SQL Analysis", | |
| "menu_sqloptimize": "Menu SQL Optimization", | |
| "menu_sqladvisor": "Menu Optimization Tools", | |
| "menu_slowquery": "Menu Slow Query Log", | |
| "menu_my2sql": "Menu My2SQL", | |
| "menu_schemasync": "Menu SchemaSync", | |
| "menu_document": "Menu Documentation", | |
| "sql_analyze": "Execute SQL analysis", | |
| "optimize_sqladvisor": "Execute SQLAdvisor", | |
| "optimize_sqltuning": "Execute SQLTuning", | |
| "optimize_soar": "Execute SOAR", | |
| } | |
| RETIRED_SQL_PERMISSION_NAMES = { | |
| "menu_sqlanalyze": "Menu SQL Analysis", | |
| "menu_sqloptimize": "Menu SQL Optimization", | |
| "menu_sqladvisor": "Menu Optimization Tools", | |
| "menu_slowquery": "Menu Slow Query Log", | |
| "menu_my2sql": "Menu My2SQL", | |
| "menu_schemasync": "Menu SchemaSync", | |
| "menu_document": "Menu Related Documentation", | |
| "sql_analyze": "Execute SQL Analysis", | |
| "optimize_sqladvisor": "Execute SQLAdvisor", | |
| "optimize_sqltuning": "Execute SQLTuning", | |
| "optimize_soar": "Execute SOAR", | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sql/migrations/0015_alter_permission_options.py` around lines 19 - 31,
RETIRED_SQL_PERMISSION_NAMES contains two values that don't match the originals
from 0001_initial.py: update the entries for keys "menu_document" and
"sql_analyze" in RETIRED_SQL_PERMISSION_NAMES so they exactly match the original
display names ("Menu Related Documentation" and "Execute SQL Analysis"
respectively) so reverse migration (migrate sql 0014) restores the exact prior
name values; adjust the mapping in
sql/migrations/0015_alter_permission_options.py and re-run/verify the migration
rollback to ensure byte-equality with 0001_initial.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
sql/migrations/0015_alter_permission_options.py (2)
26-27:⚠️ Potential issue | 🟡 MinorReverse migration display names still diverge from
0001_initial.py.The two values flagged previously remain unchanged. A
migrate sql 0014rollback will recreatemenu_documentandsql_analyzewith names that don't byte-match the originals. Codename-based permission checks are unaffected, but admin display / fidelity is.📝 Proposed fix
- "menu_document": "Menu Documentation", + "menu_document": "Menu Related Documentation", - "sql_analyze": "Execute SQL analysis", + "sql_analyze": "Execute SQL Analysis",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/migrations/0015_alter_permission_options.py` around lines 26 - 27, The reverse migration in 0015_alter_permission_options.py still uses altered display strings for the "menu_document" and "sql_analyze" permission entries; update the reverse mapping so that the display names for permission codenames "menu_document" and "sql_analyze" exactly match the originals in 0001_initial.py (use the identical byte-for-byte strings from 0001_initial.py) to ensure a rollback (`migrate sql 0014`) recreates the same display names; locate the reverse mapping logic in 0015_alter_permission_options.py that sets these keys and replace their values with the original strings from 0001_initial.py.
5-17:⚠️ Potential issue | 🟠 MajorSeed SQL still reinserts retired codenames — past finding not addressed.
The seed files under
src/init_sql/v*.sqlcontinue toINSERTthe same codenames being removed here (menu_sqlanalyze,menu_sqloptimize,menu_sqladvisor,menu_slowquery,menu_my2sql,menu_schemasync,menu_document,sql_analyze,optimize_sqladvisor,optimize_sqltuning). On a fresh install where seeds run after migrations, the retired permissions get reintroduced and resurface in roles/UIs. Please drop thoseINSERTs from the seed SQL (or pin seed ordering to run before this migration).#!/bin/bash # Re-confirm the codenames are still present in seed SQL files. fd -e sql . src/init_sql 2>/dev/null | xargs rg -nP '\b(menu_sqlanalyze|menu_sqloptimize|menu_sqladvisor|menu_slowquery|menu_my2sql|menu_schemasync|menu_document|sql_analyze|optimize_sqladvisor|optimize_sqltuning|optimize_soar)\b'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/migrations/0015_alter_permission_options.py` around lines 5 - 17, The migration defines RETIRED_SQL_PERMISSION_CODENAMES that should not be reintroduced by seed data; remove any INSERT statements in the seed SQL files under src/init_sql/v*.sql that create permissions with the codenames listed in RETIRED_SQL_PERMISSION_CODENAMES (menu_sqlanalyze, menu_sqloptimize, menu_sqladvisor, menu_slowquery, menu_my2sql, menu_schemasync, menu_document, sql_analyze, optimize_sqladvisor, optimize_sqltuning, optimize_soar), or alternatively ensure seed execution is pinned to run before this migration; update the seed SQL files to drop those INSERTs (or guard them) so the migration’s retire list in RETIRED_SQL_PERMISSION_CODENAMES isn’t undone on fresh installs.frontend/src/features/audit/pages/AuditPage.vue (1)
168-180:⚠️ Potential issue | 🟡 MinorPending debounced filter load can still clobber pagination.
movePageupdatesfilters.pageand callsloadAuditLogs()synchronously, but a recent filter edit may have queueddebouncedLoadAuditLogs(250 ms). When that pending callback fires after the page change, it resetsfilters.page = 1and reloads, undoing the navigation. Cancel/flush the debounced handle inmovePage(e.g. switch touseDebounceFn(fn, 250, { rejectOnCancel: false })and call.cancel()before paginating, or use a manually managed timer).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/audit/pages/AuditPage.vue` around lines 168 - 180, The pagination can be clobbered by a pending debounced filter reload (debouncedLoadAuditLogs) because movePage sets filters.page and immediately calls loadAuditLogs while a queued debounce may later reset filters.page = 1; fix this by cancelling or flushing the pending debounce inside movePage before changing page—e.g., switch useDebounceFn to one that returns a cancel/flush handle (or use useDebounceFn(fn, 250, { rejectOnCancel: false })) and call debouncedLoadAuditLogs.cancel() (or .flush()) at the start of movePage so the pending callback cannot reset filters.page, then proceed to set filters.page and call loadAuditLogs.api_instances/views.py (2)
1165-1171:⚠️ Potential issue | 🟠 MajorUse an upsert after the live database create.
This still does a plain
create()after the database has already been created on the instance. If a metadata row already exists for(instance, db_name), the API fails locally withIntegrityErroreven though the live database creation succeeded.Suggested fix
- database_record = InstanceDatabase.objects.create( - instance=instance, - db_name=db_name, - owner=owner, - owner_display=owner_display, - remark=remark, - ) + database_record, _ = InstanceDatabase.objects.update_or_create( + instance=instance, + db_name=db_name, + defaults={ + "owner": owner, + "owner_display": owner_display, + "remark": remark, + }, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api_instances/views.py` around lines 1165 - 1171, Replace the plain create() call on InstanceDatabase with an upsert using InstanceDatabase.objects.update_or_create so a preexisting metadata row for (instance, db_name) is updated instead of raising IntegrityError; call update_or_create with lookup kwargs instance=instance, db_name=db_name and pass owner, owner_display, remark in the defaults mapping, and assign the returned object to database_record.
366-381:⚠️ Potential issue | 🟠 MajorParse raw
user@hostvalues here as well.
_mysql_user_host()already accepts both quoted and raw MySQL account identifiers, but_account_metadata_fields()only extractshostfrom the quoted form. Reset/delete requests that send rawuser@hostwill operate on the live account correctly while missing the saved metadata row.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api_instances/views.py` around lines 366 - 381, The function _account_metadata_fields only parses quoted MySQL identifiers ("@`") but misses raw user@host forms; update the mysql branch (where user_host is read) to also handle the plain "@" case: if user_host contains "@`" keep the existing rsplit("@`",1) logic, else if it contains "@" rsplit("@",1) to set host and user (strip any surrounding backticks or quotes from both parts), and fall back to empty on errors; reference _account_metadata_fields and the local user_host variable when making the change.api_instances/serializers.py (1)
154-164:⚠️ Potential issue | 🟠 MajorApply the non-blank password validator to reset requests too.
InstanceAccountPasswordSerializerstill accepts whitespace-only passwords, so the reset endpoint can pass" "through as a “valid” password. This is the same gap that was fixed for account creation, but not for password resets.Suggested fix
class InstanceAccountPasswordSerializer(serializers.Serializer): instance_id = serializers.IntegerField() db_name = serializers.CharField(max_length=128, allow_blank=True, required=False) db_name_user = serializers.CharField( max_length=260, allow_blank=True, required=False ) user_host = serializers.CharField(max_length=260, allow_blank=True, required=False) user = serializers.CharField(max_length=128) host = serializers.CharField(max_length=64, allow_blank=True, required=False) password = serializers.CharField(max_length=128, write_only=True) + + def validate_password(self, value): + if not value or not value.strip(): + raise serializers.ValidationError("Password cannot be blank.") + return value🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api_instances/serializers.py` around lines 154 - 164, The password field on InstanceAccountPasswordSerializer currently allows whitespace-only strings; change its validation to reject blank/whitespace-only passwords by removing allow_blank=True for the password field and adding a validator that enforces non-whitespace content (i.e., ensure the password CharField "password" in InstanceAccountPasswordSerializer trims whitespace and fails if empty or only whitespace). Update the serializer's password declaration to use allow_blank=False (or add an explicit validator that checks str.strip() != "") so reset requests are validated the same as account creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api_instances/views.py`:
- Around line 801-808: Requests currently trust any db_name after instance
lookup; enforce the same visibility filtering used by DataDictionaryDatabaseList
before querying metadata. After calling _data_dictionary_instance(request.user,
instance_id) and before using get_engine/get_group_tables_by_db, validate the
requested db_name against the instance's show_db_name_regex and
denied_db_name_regex rules (the same logic used by DataDictionaryDatabaseList);
if the db_name is not allowed, raise the same not-found/permission response.
Apply this check around the handlers that use get_engine and
query_engine.get_group_tables_by_db (the block with escape_string and
get_group_tables_by_db and the other locations noted at 879-898 and 954-959).
In `@frontend/src/features/audit/pages/AuditPage.vue`:
- Around line 160-198: Tab changes are being debounced because setView mutates
watched filters and the watcher routes through debouncedLoadAuditLogs; fix by
making setView perform an immediate fetch and suppress the debounced watcher:
after updating activeView, filters.page and clearing filters in setView, call
void loadAuditLogs() directly and set a short-lived flag (e.g.,
suppressDebouncedLoad = true) that the existing watcher checks and returns early
if true; reset suppressDebouncedLoad (via nextTick or a timeout) so subsequent
filter edits still go through debouncedLoadAuditLogs. Ensure you reference
setView, loadAuditLogs, debouncedLoadAuditLogs and the watcher when applying the
change.
In `@frontend/src/features/instance-operations/pages/SessionDiagnosticsPage.vue`:
- Around line 164-188: The diagnostics loader leaves previous rows visible
during fetch; clear the stale UI state by resetting rows before starting the
network call. In the function that contains this block (e.g., loadRows / the
function manipulating loadingRows.value, error.value, selectedThreadIds.value),
set rows.value = [] (or an appropriate empty state) immediately after setting
loadingRows.value = true and before the try/await calls so old rows are not
shown or used (and ensure selectedThreadIds.value is still cleared as shown).
In `@frontend/src/features/inventory/pages/DataDictionaryPage.vue`:
- Around line 161-176: The page keeps showing stale databases/tableGroups while
awaiting the new fetch; before calling fetchDataDictionaryDatabases (and
likewise before the table-fetch block around lines 185-200), immediately clear
the rendered data so the UI doesn't show the old instance: set databases.value =
[] and tableGroups.value = [] (and clear tableDetail.value,
selectedDbName.value/selectedTableName.value as you already do) before the
await, then on success assign databases.value = response.result and restore
selectedDbName; do the same pattern in the table loader (clear
tables/tableGroups/tableDetail before calling the fetch function) so failed
requests won't leave prior schema visible.
---
Duplicate comments:
In `@api_instances/serializers.py`:
- Around line 154-164: The password field on InstanceAccountPasswordSerializer
currently allows whitespace-only strings; change its validation to reject
blank/whitespace-only passwords by removing allow_blank=True for the password
field and adding a validator that enforces non-whitespace content (i.e., ensure
the password CharField "password" in InstanceAccountPasswordSerializer trims
whitespace and fails if empty or only whitespace). Update the serializer's
password declaration to use allow_blank=False (or add an explicit validator that
checks str.strip() != "") so reset requests are validated the same as account
creation.
In `@api_instances/views.py`:
- Around line 1165-1171: Replace the plain create() call on InstanceDatabase
with an upsert using InstanceDatabase.objects.update_or_create so a preexisting
metadata row for (instance, db_name) is updated instead of raising
IntegrityError; call update_or_create with lookup kwargs instance=instance,
db_name=db_name and pass owner, owner_display, remark in the defaults mapping,
and assign the returned object to database_record.
- Around line 366-381: The function _account_metadata_fields only parses quoted
MySQL identifiers ("@`") but misses raw user@host forms; update the mysql branch
(where user_host is read) to also handle the plain "@" case: if user_host
contains "@`" keep the existing rsplit("@`",1) logic, else if it contains "@"
rsplit("@",1) to set host and user (strip any surrounding backticks or quotes
from both parts), and fall back to empty on errors; reference
_account_metadata_fields and the local user_host variable when making the
change.
In `@frontend/src/features/audit/pages/AuditPage.vue`:
- Around line 168-180: The pagination can be clobbered by a pending debounced
filter reload (debouncedLoadAuditLogs) because movePage sets filters.page and
immediately calls loadAuditLogs while a queued debounce may later reset
filters.page = 1; fix this by cancelling or flushing the pending debounce inside
movePage before changing page—e.g., switch useDebounceFn to one that returns a
cancel/flush handle (or use useDebounceFn(fn, 250, { rejectOnCancel: false }))
and call debouncedLoadAuditLogs.cancel() (or .flush()) at the start of movePage
so the pending callback cannot reset filters.page, then proceed to set
filters.page and call loadAuditLogs.
In `@sql/migrations/0015_alter_permission_options.py`:
- Around line 26-27: The reverse migration in 0015_alter_permission_options.py
still uses altered display strings for the "menu_document" and "sql_analyze"
permission entries; update the reverse mapping so that the display names for
permission codenames "menu_document" and "sql_analyze" exactly match the
originals in 0001_initial.py (use the identical byte-for-byte strings from
0001_initial.py) to ensure a rollback (`migrate sql 0014`) recreates the same
display names; locate the reverse mapping logic in
0015_alter_permission_options.py that sets these keys and replace their values
with the original strings from 0001_initial.py.
- Around line 5-17: The migration defines RETIRED_SQL_PERMISSION_CODENAMES that
should not be reintroduced by seed data; remove any INSERT statements in the
seed SQL files under src/init_sql/v*.sql that create permissions with the
codenames listed in RETIRED_SQL_PERMISSION_CODENAMES (menu_sqlanalyze,
menu_sqloptimize, menu_sqladvisor, menu_slowquery, menu_my2sql, menu_schemasync,
menu_document, sql_analyze, optimize_sqladvisor, optimize_sqltuning,
optimize_soar), or alternatively ensure seed execution is pinned to run before
this migration; update the seed SQL files to drop those INSERTs (or guard them)
so the migration’s retire list in RETIRED_SQL_PERMISSION_CODENAMES isn’t undone
on fresh installs.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 725772f6-2fbe-4d8d-8faf-b8e8dd996a1d
📒 Files selected for processing (8)
api_instances/serializers.pyapi_instances/tests.pyapi_instances/views.pydocs/spa-bootstrap-parity-checklist.mdfrontend/src/features/audit/pages/AuditPage.vuefrontend/src/features/instance-operations/pages/SessionDiagnosticsPage.vuefrontend/src/features/inventory/pages/DataDictionaryPage.vuesql/migrations/0015_alter_permission_options.py
✅ Files skipped from review due to trivial changes (1)
- docs/spa-bootstrap-parity-checklist.md
Comment on lines
+801
to
+808
| instance = _data_dictionary_instance(request.user, instance_id) | ||
|
|
||
| try: | ||
| query_engine = get_engine(instance=instance) | ||
| escaped_db_name = query_engine.escape_string(db_name) | ||
| grouped_tables = query_engine.get_group_tables_by_db( | ||
| db_name=escaped_db_name | ||
| ) |
Contributor
There was a problem hiding this comment.
Enforce database visibility here, not just in the listing endpoint.
DataDictionaryDatabaseList filters databases through show_db_name_regex / denied_db_name_regex, but these handlers trust any requested db_name once the instance is visible. That lets a user query table metadata or export a schema that was intentionally hidden from the database list just by guessing its name.
Also applies to: 879-898, 954-959
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api_instances/views.py` around lines 801 - 808, Requests currently trust any
db_name after instance lookup; enforce the same visibility filtering used by
DataDictionaryDatabaseList before querying metadata. After calling
_data_dictionary_instance(request.user, instance_id) and before using
get_engine/get_group_tables_by_db, validate the requested db_name against the
instance's show_db_name_regex and denied_db_name_regex rules (the same logic
used by DataDictionaryDatabaseList); if the db_name is not allowed, raise the
same not-found/permission response. Apply this check around the handlers that
use get_engine and query_engine.get_group_tables_by_db (the block with
escape_string and get_group_tables_by_db and the other locations noted at
879-898 and 954-959).
Comment on lines
+160
to
+198
| function setView(view: AuditView) { | ||
| activeView.value = view | ||
| filters.page = 1 | ||
| filters.action = '' | ||
| filters.status = '' | ||
| filters.syntaxType = '' | ||
| } | ||
|
|
||
| function movePage(direction: -1 | 1) { | ||
| const nextPage = filters.page + direction | ||
| if (nextPage < 1 || (direction === 1 && activePage.value.next === null)) { | ||
| return | ||
| } | ||
| filters.page = nextPage | ||
| void loadAuditLogs() | ||
| } | ||
|
|
||
| const debouncedLoadAuditLogs = useDebounceFn(() => { | ||
| filters.page = 1 | ||
| void loadAuditLogs() | ||
| }, 250) | ||
|
|
||
| watch( | ||
| () => [ | ||
| activeView.value, | ||
| filters.search, | ||
| filters.action, | ||
| filters.status, | ||
| filters.syntaxType, | ||
| filters.startDate, | ||
| filters.endDate, | ||
| ], | ||
| () => { | ||
| if (initializing) { | ||
| return | ||
| } | ||
| debouncedLoadAuditLogs() | ||
| }, | ||
| ) |
Contributor
There was a problem hiding this comment.
Tab switches incur a 250 ms debounce delay.
setView mutates activeView and resets filters.action/status/syntaxType, all of which are watched. The watcher then routes through debouncedLoadAuditLogs, so clicking a tab waits ~250 ms before the fetch starts (and any in-flight pending debounced filter edit can also race with it). Per the AI summary, tab/view changes are supposed to refetch immediately while only filter edits should be debounced. Consider invoking loadAuditLogs() directly from setView (and skipping the watch when only activeView changed), or splitting the watch into a tab watcher (immediate) and a filter watcher (debounced).
♻️ Possible split
-watch(
- () => [
- activeView.value,
- filters.search,
- filters.action,
- filters.status,
- filters.syntaxType,
- filters.startDate,
- filters.endDate,
- ],
- () => {
- if (initializing) {
- return
- }
- debouncedLoadAuditLogs()
- },
-)
+watch(
+ () => activeView.value,
+ () => {
+ if (initializing) return
+ filters.page = 1
+ void loadAuditLogs()
+ },
+)
+watch(
+ () => [
+ filters.search,
+ filters.action,
+ filters.status,
+ filters.syntaxType,
+ filters.startDate,
+ filters.endDate,
+ ],
+ () => {
+ if (initializing) return
+ debouncedLoadAuditLogs()
+ },
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function setView(view: AuditView) { | |
| activeView.value = view | |
| filters.page = 1 | |
| filters.action = '' | |
| filters.status = '' | |
| filters.syntaxType = '' | |
| } | |
| function movePage(direction: -1 | 1) { | |
| const nextPage = filters.page + direction | |
| if (nextPage < 1 || (direction === 1 && activePage.value.next === null)) { | |
| return | |
| } | |
| filters.page = nextPage | |
| void loadAuditLogs() | |
| } | |
| const debouncedLoadAuditLogs = useDebounceFn(() => { | |
| filters.page = 1 | |
| void loadAuditLogs() | |
| }, 250) | |
| watch( | |
| () => [ | |
| activeView.value, | |
| filters.search, | |
| filters.action, | |
| filters.status, | |
| filters.syntaxType, | |
| filters.startDate, | |
| filters.endDate, | |
| ], | |
| () => { | |
| if (initializing) { | |
| return | |
| } | |
| debouncedLoadAuditLogs() | |
| }, | |
| ) | |
| function setView(view: AuditView) { | |
| activeView.value = view | |
| filters.page = 1 | |
| filters.action = '' | |
| filters.status = '' | |
| filters.syntaxType = '' | |
| } | |
| function movePage(direction: -1 | 1) { | |
| const nextPage = filters.page + direction | |
| if (nextPage < 1 || (direction === 1 && activePage.value.next === null)) { | |
| return | |
| } | |
| filters.page = nextPage | |
| void loadAuditLogs() | |
| } | |
| const debouncedLoadAuditLogs = useDebounceFn(() => { | |
| filters.page = 1 | |
| void loadAuditLogs() | |
| }, 250) | |
| watch( | |
| () => activeView.value, | |
| () => { | |
| if (initializing) return | |
| filters.page = 1 | |
| void loadAuditLogs() | |
| }, | |
| ) | |
| watch( | |
| () => [ | |
| filters.search, | |
| filters.action, | |
| filters.status, | |
| filters.syntaxType, | |
| filters.startDate, | |
| filters.endDate, | |
| ], | |
| () => { | |
| if (initializing) return | |
| debouncedLoadAuditLogs() | |
| }, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/audit/pages/AuditPage.vue` around lines 160 - 198, Tab
changes are being debounced because setView mutates watched filters and the
watcher routes through debouncedLoadAuditLogs; fix by making setView perform an
immediate fetch and suppress the debounced watcher: after updating activeView,
filters.page and clearing filters in setView, call void loadAuditLogs() directly
and set a short-lived flag (e.g., suppressDebouncedLoad = true) that the
existing watcher checks and returns early if true; reset suppressDebouncedLoad
(via nextTick or a timeout) so subsequent filter edits still go through
debouncedLoadAuditLogs. Ensure you reference setView, loadAuditLogs,
debouncedLoadAuditLogs and the watcher when applying the change.
Comment on lines
+164
to
+188
| loadingRows.value = true | ||
| error.value = '' | ||
| feedback.value = '' | ||
| selectedThreadIds.value = [] | ||
| killSql.value = '' | ||
|
|
||
| try { | ||
| if (activeTab.value === 'processes') { | ||
| rows.value = (await fetchInstanceOperationDiagnosticProcesses(requireToken(), { | ||
| instance_id: selectedInstanceId.value, | ||
| command_type: commandType.value, | ||
| })).results | ||
| } else if (activeTab.value === 'tablespace') { | ||
| rows.value = (await fetchInstanceOperationDiagnosticTablespace(requireToken(), { | ||
| instance_id: selectedInstanceId.value, | ||
| size: 25, | ||
| })).results | ||
| } else if (activeTab.value === 'transactions') { | ||
| rows.value = (await fetchInstanceOperationDiagnosticTransactions(requireToken(), selectedInstanceId.value)).results | ||
| } else { | ||
| rows.value = (await fetchInstanceOperationDiagnosticLocks(requireToken(), selectedInstanceId.value)).results | ||
| } | ||
| } catch (errorValue) { | ||
| error.value = toUserFacingMessage(errorValue, 'Failed to load diagnostics.') | ||
| } finally { |
Contributor
There was a problem hiding this comment.
Clear stale diagnostics rows before refetching.
loadRows() resets the selected thread IDs, but it leaves the previous rows visible until the new request succeeds. After switching instances, a failed or slow fetch can leave old process rows on screen under the new instance context, and the kill flow will then submit those IDs against selectedInstanceId.value.
Suggested fix
async function loadRows() {
if (!selectedInstanceId.value) {
rows.value = []
return
}
loadingRows.value = true
error.value = ''
feedback.value = ''
+ rows.value = []
selectedThreadIds.value = []
killSql.value = ''
try {
if (activeTab.value === 'processes') {
rows.value = (await fetchInstanceOperationDiagnosticProcesses(requireToken(), {
@@
rows.value = (await fetchInstanceOperationDiagnosticLocks(requireToken(), selectedInstanceId.value)).results
}
} catch (errorValue) {
+ rows.value = []
error.value = toUserFacingMessage(errorValue, 'Failed to load diagnostics.')
} finally {
loadingRows.value = false
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/instance-operations/pages/SessionDiagnosticsPage.vue`
around lines 164 - 188, The diagnostics loader leaves previous rows visible
during fetch; clear the stale UI state by resetting rows before starting the
network call. In the function that contains this block (e.g., loadRows / the
function manipulating loadingRows.value, error.value, selectedThreadIds.value),
set rows.value = [] (or an appropriate empty state) immediately after setting
loadingRows.value = true and before the try/await calls so old rows are not
shown or used (and ensure selectedThreadIds.value is still cleared as shown).
Comment on lines
+161
to
+176
| loadingDatabases.value = true | ||
| error.value = '' | ||
| selectedDbName.value = '' | ||
| selectedTableName.value = '' | ||
| tableGroups.value = [] | ||
| tableDetail.value = null | ||
|
|
||
| try { | ||
| const response = await fetchDataDictionaryDatabases(selectedInstanceId.value, requireToken()) | ||
| databases.value = response.result | ||
| selectedDbName.value = databases.value[0] ?? '' | ||
| } catch (errorValue) { | ||
| error.value = toUserFacingMessage(errorValue, 'Failed to load databases.') | ||
| } finally { | ||
| loadingDatabases.value = false | ||
| } |
Contributor
There was a problem hiding this comment.
Drop stale instance/database data before loading the next scope.
These loaders reset the selection, but they keep the previous databases / tableGroups rendered until the request completes. If the new request fails, the page keeps showing the old instance's schema, which is pretty misleading.
Suggested fix
async function loadDatabases() {
if (!selectedInstanceId.value) {
databases.value = []
return
}
loadingDatabases.value = true
error.value = ''
+ databases.value = []
selectedDbName.value = ''
selectedTableName.value = ''
tableGroups.value = []
tableDetail.value = null
@@
async function loadTables() {
if (!selectedInstanceId.value || !selectedDbName.value) {
tableGroups.value = []
return
}
loadingTables.value = true
error.value = ''
+ tableGroups.value = []
selectedTableName.value = ''
tableDetail.value = nullAlso applies to: 185-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/inventory/pages/DataDictionaryPage.vue` around lines
161 - 176, The page keeps showing stale databases/tableGroups while awaiting the
new fetch; before calling fetchDataDictionaryDatabases (and likewise before the
table-fetch block around lines 185-200), immediately clear the rendered data so
the UI doesn't show the old instance: set databases.value = [] and
tableGroups.value = [] (and clear tableDetail.value,
selectedDbName.value/selectedTableName.value as you already do) before the
await, then on success assign databases.value = response.result and restore
selectedDbName; do the same pattern in the table loader (clear
tables/tableGroups/tableDetail before calling the fetch function) so failed
requests won't leave prior schema visible.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Removals
Documentation
Tests
Refactor