Skip to content

Commit b9a021a

Browse files
committed
fix(release): address v1.6.0 code review findings
Backend: - export job admission now synchronous (returns 429 at create-time instead of accepting then async-failing) - export pruner unlinks files before deleting DB rows; idempotent on crash, no orphaned artifacts - query share TTL rejects > max with 400 instead of silently clamping - handleDownloadExportJob logs non-ErrNotExist stat errors instead of swallowing them - OIDC startup warning when skip_email_verified_check=true so the bypass is visible in boot logs - drop unreachable empty-format checks in export handlers - remove dead TLSCACert field; system-roots TLS only (proper TLS with custom CA / skip_verify is a follow-up PR) Frontend: - useQuery: surface translate API errors instead of swallowing and proceeding to SQL mode with stale rawSql - LogExplorer: AbortController on export poll, abort on unmount so navigation away does not leak polling - useUrlState: readiness timeout now shows a non-blocking toast and returns boolean - context store: clearSource() action; replace 4 direct contextStore.sourceId = null mutations in SavedQueriesView - DataTable 🔑 fallback for undefined queryId so key stays stable - move planning doc out of repo root (now in .plans/)
1 parent f0e4813 commit b9a021a

12 files changed

Lines changed: 120 additions & 960 deletions

File tree

frontend/src/composables/useQuery.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,10 +177,13 @@ export function useQuery() {
177177
if (response.data?.full_sql) {
178178
exploreStore.setRawSql(response.data.full_sql);
179179
} else {
180-
console.warn("useQuery: Backend did not return full_sql, response:", response.data);
180+
queryError.value = "Could not generate SQL from your query. Please refine and try again.";
181+
return;
181182
}
182183
} catch (error: any) {
183184
console.error("useQuery: Failed to get full SQL from backend:", error);
185+
queryError.value = error?.message || "Failed to translate query to SQL. Please try again.";
186+
return;
184187
}
185188
}
186189
}

frontend/src/composables/useUrlState.ts

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import { useExploreStore } from '@/stores/explore';
44
import { useTeamsStore } from '@/stores/teams';
55
import { useSourcesStore } from '@/stores/sources';
66
import { useContextStore } from '@/stores/context';
7+
import { useToast } from '@/composables/useToast';
8+
import { TOAST_DURATION } from '@/lib/constants';
79
import { savedQueriesApi } from '@/api/savedQueries';
810
import { exploreApi } from '@/api/explore';
911

@@ -24,6 +26,7 @@ export function useUrlState(): UrlStateReturn {
2426
const teamsStore = useTeamsStore();
2527
const sourcesStore = useSourcesStore();
2628
const contextStore = useContextStore();
29+
const { toast } = useToast();
2730

2831
const state = ref<UrlSyncState>('idle');
2932
const error = ref<string | null>(null);
@@ -262,16 +265,21 @@ export function useUrlState(): UrlStateReturn {
262265
}
263266
}
264267

265-
function waitForReadiness(maxWaitMs = 5000): Promise<void> {
268+
function waitForReadiness(maxWaitMs = 5000): Promise<boolean> {
266269
if (checkReadiness()) {
267-
return Promise.resolve();
270+
return Promise.resolve(true);
268271
}
269272

270273
return new Promise((resolve) => {
271274
const timeout = setTimeout(() => {
272275
stopWatch();
273276
console.warn('useUrlState: Readiness timeout, proceeding anyway');
274-
resolve();
277+
toast({
278+
title: "Loading is taking longer than expected",
279+
description: "Some teams or sources may not be ready yet — results could be incomplete.",
280+
duration: TOAST_DURATION.WARNING,
281+
});
282+
resolve(false);
275283
}, maxWaitMs);
276284

277285
const stopWatch = watch(
@@ -286,7 +294,7 @@ export function useUrlState(): UrlStateReturn {
286294
if (checkReadiness()) {
287295
clearTimeout(timeout);
288296
stopWatch();
289-
resolve();
297+
resolve(true);
290298
}
291299
},
292300
{ immediate: true }

frontend/src/stores/context.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,10 @@ export const useContextStore = defineStore('context', () => {
7373
saveToStorage(persisted)
7474
}
7575

76+
function clearSource() {
77+
sourceId.value = null
78+
}
79+
7680
function clear() {
7781
console.log('ContextStore: Clearing all context')
7882
teamId.value = null
@@ -127,6 +131,7 @@ export const useContextStore = defineStore('context', () => {
127131
hasValidContext,
128132
selectTeam,
129133
selectSource,
134+
clearSource,
130135
clear,
131136
clearStorage,
132137
setFromRoute,

frontend/src/views/collections/SavedQueriesView.vue

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ onMounted(async () => {
166166
167167
// If no source was specified in URL, enforce All Sources mode
168168
if (!initialSourceParam && contextStore.sourceId) {
169-
contextStore.sourceId = null;
169+
contextStore.clearSource();
170170
const q = { ...route.query };
171171
delete q.source;
172172
router.replace({ query: q });
@@ -214,7 +214,7 @@ async function handleTeamChange(teamId: string) {
214214
if (teamId === "all") {
215215
isAllTeamsMode.value = true;
216216
// Clear stale team/source context so permission checks don't use the previous team
217-
contextStore.sourceId = null;
217+
contextStore.clearSource();
218218
localTeamQueries.value = [];
219219
const result = await savedQueriesStore.fetchMyCollections();
220220
if (result.success) {
@@ -230,7 +230,7 @@ async function handleTeamChange(teamId: string) {
230230
await contextHandleTeamChange(teamIdNum);
231231
232232
// Default to All Sources when switching teams
233-
contextStore.sourceId = null;
233+
contextStore.clearSource();
234234
const query = { ...route.query };
235235
delete query.source;
236236
router.replace({ query });
@@ -253,7 +253,7 @@ async function handleSourceChange(sourceId: string) {
253253
try {
254254
// Handle All Sources selection
255255
if (!sourceId || sourceId === "all") {
256-
contextStore.sourceId = null;
256+
contextStore.clearSource();
257257
const query = { ...route.query };
258258
delete query.source;
259259
await router.replace({ query });

frontend/src/views/explore/LogExplorer.vue

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
computed,
55
watch,
66
onMounted,
7+
onBeforeUnmount,
78
nextTick,
89
} from "vue";
910
import { storeToRefs } from "pinia";
@@ -116,6 +117,7 @@ const isChangingContext = computed(() => {
116117
return teamLoading || sourceLoading;
117118
});
118119
const isExporting = ref(false);
120+
let exportAbortController: AbortController | null = null;
119121
120122
const getQueryParamValue = (key: string) => {
121123
const value = route.query[key];
@@ -701,14 +703,23 @@ const triggerBrowserDownload = (downloadUrl: string, fileName?: string) => {
701703
link.remove();
702704
};
703705
704-
const waitForExportCompletion = async (exportId: string, timeoutSeconds: number) => {
706+
class ExportAbortedError extends Error {
707+
constructor() {
708+
super("Export polling aborted");
709+
this.name = "ExportAbortedError";
710+
}
711+
}
712+
713+
const waitForExportCompletion = async (exportId: string, timeoutSeconds: number, signal: AbortSignal) => {
705714
if (!currentTeamId.value || !currentSourceId.value) {
706715
throw new Error("Team and source are required for export");
707716
}
708717
709718
const deadline = Date.now() + (timeoutSeconds + 60) * 1000;
710719
while (Date.now() < deadline) {
720+
if (signal.aborted) throw new ExportAbortedError();
711721
const response = await exploreApi.getExportJob(currentSourceId.value, exportId, currentTeamId.value);
722+
if (signal.aborted) throw new ExportAbortedError();
712723
const job = response.data;
713724
if (!job) {
714725
throw new Error("Export status is unavailable");
@@ -740,6 +751,10 @@ const handleExport = async () => {
740751
return;
741752
}
742753
754+
exportAbortController?.abort();
755+
exportAbortController = new AbortController();
756+
const signal = exportAbortController.signal;
757+
743758
try {
744759
isExporting.value = true;
745760
const queryTimeout = Math.max(exploreStore.queryTimeout, 120);
@@ -754,24 +769,33 @@ const handleExport = async () => {
754769
throw new Error("Export job was not created");
755770
}
756771
757-
const completedJob = await waitForExportCompletion(job.id, queryTimeout);
772+
const completedJob = await waitForExportCompletion(job.id, queryTimeout, signal);
758773
if (!completedJob.download_url) {
759774
throw new Error("Export download is unavailable");
760775
}
761776
762777
triggerBrowserDownload(completedJob.download_url, completedJob.file_name);
763778
} catch (error: any) {
779+
if (error instanceof ExportAbortedError) return;
764780
toast({
765781
title: "Download Failed",
766782
description: getErrorMessage(error),
767783
variant: "destructive",
768784
duration: TOAST_DURATION.ERROR,
769785
});
770786
} finally {
787+
if (exportAbortController?.signal === signal) {
788+
exportAbortController = null;
789+
}
771790
isExporting.value = false;
772791
}
773792
};
774793
794+
onBeforeUnmount(() => {
795+
exportAbortController?.abort();
796+
exportAbortController = null;
797+
});
798+
775799
const calendarDatePartToDate = (value: any) => {
776800
if (!value) return undefined;
777801
@@ -1487,7 +1511,7 @@ onMounted(async () => {
14871511
<component
14881512
v-if="exploreStore.columns?.length > 0"
14891513
:is="displayMode === 'table' ? DataTable : CompactLogList"
1490-
:key="`${exploreStore.sourceId}-${exploreStore.activeMode}-${exploreStore.queryId}-${displayMode}`"
1514+
:key="`${exploreStore.sourceId}-${exploreStore.activeMode}-${exploreStore.queryId ?? 'noquery'}-${displayMode}`"
14911515
:columns="exploreStore.columns as any"
14921516
:data="exploreStore.logs"
14931517
:stats="exploreStore.queryStats"

internal/auth/oidc.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,10 @@ func NewOIDCProvider(ctx context.Context, oidcCfg *config.OIDCConfig, log *slog.
120120
SkipIssuerCheck: true,
121121
})
122122

123+
if oidcCfg.SkipEmailVerifiedCheck {
124+
log.Warn("OIDC skip_email_verified_check is enabled — logins will succeed when the email_verified claim is missing")
125+
}
126+
123127
return &OIDCProvider{
124128
provider: provider,
125129
verifier: verifier,

internal/clickhouse/client.go

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import (
99
"fmt"
1010
"log/slog"
1111
"math"
12-
"os"
1312
"reflect"
1413
"regexp"
1514
"strings"
@@ -72,7 +71,6 @@ type ClientOptions struct {
7271
SourceID string // Source ID for metrics tracking.
7372
Source *models.Source // Source model for enhanced metrics.
7473
TLSEnable bool // Enable TLS for the connection.
75-
TLSCACert string // Optional path to a CA certificate file for TLS verification.
7674
}
7775

7876
// ExtendedColumnInfo provides detailed column metadata, including nullability,
@@ -114,22 +112,15 @@ func NewClient(opts ClientOptions, logger *slog.Logger) (*Client, error) {
114112
}
115113
}
116114

117-
// Build TLS config if enabled.
115+
// Build TLS config if enabled. Uses the system root CA pool; operators
116+
// who need a custom CA bundle should install it into the OS trust store.
118117
var tlsCfg *tls.Config
119118
if opts.TLSEnable {
120119
rootCAs, err := x509.SystemCertPool()
121120
if err != nil {
121+
logger.Warn("failed to load system cert pool, falling back to empty pool", "error", err)
122122
rootCAs = x509.NewCertPool()
123123
}
124-
if opts.TLSCACert != "" {
125-
caCert, err := os.ReadFile(opts.TLSCACert)
126-
if err != nil {
127-
return nil, fmt.Errorf("reading TLS CA certificate: %w", err)
128-
}
129-
if !rootCAs.AppendCertsFromPEM(caCert) {
130-
return nil, fmt.Errorf("failed to append CA certificate from %s", opts.TLSCACert)
131-
}
132-
}
133124
tlsCfg = &tls.Config{
134125
RootCAs: rootCAs,
135126
MinVersion: tls.VersionTLS12,

internal/server/export_handlers.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ func (s *Server) handleExportLogs(c *fiber.Ctx) error {
6161
}
6262
format = normalized
6363
}
64-
if format == "" || !isExportFormatAllowed(format, s.config.Export.Formats) {
64+
if !isExportFormatAllowed(format, s.config.Export.Formats) {
6565
return SendErrorWithType(c, fiber.StatusBadRequest, "Unsupported export format. Use csv or ndjson.", models.ValidationErrorType)
6666
}
6767
if format == "csv" {

0 commit comments

Comments
 (0)