Revert SSR to disabled and fix PO status update parsing#343
Merged
Revert SSR to disabled and fix PO status update parsing#343
Conversation
📝 WalkthroughWalkthroughThis PR refactors analysis status handling to prioritize PodOrc pod status data over hub-provided statuses, renames the analysis update event from Changes
Possibly related PRs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/analysis/AnalysesTable.vue`:
- Around line 138-152: The helper useCommonLanguage (and PodStatus enum mapping)
is unused; either remove the dead function or apply it where analysis statuses
are normalized before rendering or comparisons. Locate where statuses are read
for the table (e.g., the status cell renderer, the column definition or any
compute/display function in AnalysesTable.vue that uses PodStatus or reads
hub-sourced status values) and call useCommonLanguage(status) (or replace direct
uses of PodStatus.Executing/Executed with the mapped values) so the
"running"/"finished" vocabulary is actually returned for hub statuses; if you
opt to remove it, delete useCommonLanguage and any unused PodStatus synonyms to
avoid dead code.
- Around line 242-246: The code only copies
executionStatuses[analysisId]!.status into analysisEntry.execution_status when a
PO status exists, so any newer progress from the PO
(executionStatuses[analysisId]!.progress) is ignored and the progress bar
remains stale; update the same branch that checks `if (executionStatuses &&
analysisId in executionStatuses)` to also copy
`executionStatuses[analysisId]!.progress` into
`analysisEntry.execution_progress` (or set it to null/undefined if not present),
and ensure you still apply the existing fallback using `acceptableHubStatuses`
for execution_status in the else branch; locate and update the block that
references `executionStatuses`, `analysisEntry.execution_status`,
`analysisEntry.execution_progress`, and `acceptableHubStatuses`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4da217ad-f065-4985-8309-6eca6f8e7e05
📒 Files selected for processing (7)
app/components/analysis/AnalysesTable.vueapp/components/analysis/AnalysisControlButtons.vueapp/components/analysis/AnalysisUpdateButton.vuenuxt.config.tsserver/routes/flame/api/auth/[...].tstest/components/analysis/AnalysisUpdateButton.spec.tstest/mockapi/handlers.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/routes/flame/api/auth/[...].ts (1)
108-139:⚠️ Potential issue | 🟠 MajorNormalize
expires_atunits before the JWT expiry check.The refresh path at line 137 writes
expires_atin milliseconds, but the initial login at line 189 storesaccount.expires_atfrom the OAuth provider in seconds (per RFC 6749 and OpenID Connect). The guard at line 193 comparesDate.now()(milliseconds) withtoken.expires_at, causing freshly issued tokens to be immediately treated as expired. This triggers an unnecessary refresh on every session bootstrap and breaks authentication if the refresh endpoint is unavailable.Fix: Store
expires_atin seconds consistently. Update line 137 toexpires_at: Math.floor(Date.now() / 1000) + refreshedTokens.expires_inand update the guard at line 193 toMath.floor(Date.now() / 1000) < (token.expires_at as number).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/flame/api/auth/`[...].ts around lines 108 - 139, The refreshAccessToken function writes expires_at in milliseconds while the initial login stores account.expires_at in seconds, causing the expiry check to misfire; update the return in refreshAccessToken (function refreshAccessToken) to set expires_at to seconds by using Math.floor(Date.now()/1000) + refreshedTokens.expires_in, and change the JWT expiry guard that compares Date.now() with token.expires_at to compare Math.floor(Date.now()/1000) < (token.expires_at as number) so both sides use seconds consistently.
🧹 Nitpick comments (1)
nuxt.config.ts (1)
7-7: Keep the auth mode aligned withssr: false.
app/plugins/api.tsstill gets and refreshes sessions entirely from the client, sodisableServerSideAuth: falsedoes not restore real server-side route protection in the current SPA setup. Keeping those flags split makes the auth model harder to reason about and will silently change behavior the next time SSR is turned back on.Also applies to: 29-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nuxt.config.ts` at line 7, The project is configured as an SPA (ssr: false) but auth is still set up for server-side auth; update the auth mode to match by setting disableServerSideAuth to true in the Nuxt config (ensure the same auth-related option that is currently false is flipped), and audit app/plugins/api.ts (which currently fetches and refreshes sessions entirely from the client) to ensure it remains client-only session handling; this keeps the auth model consistent and prevents silent behavior changes when SSR is toggled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/analysis/AnalysesTable.vue`:
- Around line 237-246: When executionStatuses contains an entry for analysisId,
don't preserve a stale hub progress value when podStatus.progress is
null/undefined; instead set analysisEntry.execution_progress to
podStatus.progress if present, otherwise clear it (e.g. set to null or
undefined) so PO becomes the source of truth. Update the block that reads
executionStatuses[analysisId] (using variables podStatus, analysisEntry,
execution_progress, acceptableHubStatuses) to explicitly assign a cleared value
when podStatus.progress is missing rather than using "??
analysisEntry.execution_progress".
---
Outside diff comments:
In `@server/routes/flame/api/auth/`[...].ts:
- Around line 108-139: The refreshAccessToken function writes expires_at in
milliseconds while the initial login stores account.expires_at in seconds,
causing the expiry check to misfire; update the return in refreshAccessToken
(function refreshAccessToken) to set expires_at to seconds by using
Math.floor(Date.now()/1000) + refreshedTokens.expires_in, and change the JWT
expiry guard that compares Date.now() with token.expires_at to compare
Math.floor(Date.now()/1000) < (token.expires_at as number) so both sides use
seconds consistently.
---
Nitpick comments:
In `@nuxt.config.ts`:
- Line 7: The project is configured as an SPA (ssr: false) but auth is still set
up for server-side auth; update the auth mode to match by setting
disableServerSideAuth to true in the Nuxt config (ensure the same auth-related
option that is currently false is flipped), and audit app/plugins/api.ts (which
currently fetches and refreshes sessions entirely from the client) to ensure it
remains client-only session handling; this keeps the auth model consistent and
prevents silent behavior changes when SSR is toggled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9a366e0-27cf-4e7b-933e-b74457f1ceae
📒 Files selected for processing (7)
app/components/analysis/AnalysesTable.vueapp/components/analysis/AnalysisControlButtons.vueapp/components/analysis/AnalysisUpdateButton.vuenuxt.config.tsserver/routes/flame/api/auth/[...].tstest/components/analysis/AnalysisUpdateButton.spec.tstest/mockapi/handlers.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Infrastructure Changes