Fix pr version title notes #844
Conversation
📝 WalkthroughWalkthroughThis pull request adds version awareness to the BasePage and breadcrumbs functionality. The changes extend the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
daras_ai_v2/base.py (1)
955-962: Consider adding a comment explaining theversion_idlogic.The intent of
or version_idis that viewing any specific version should allow publishing, but this isn't immediately obvious to readers.version_id = self.request.query_params.get("version_id", None) return ( published_run.title != title or published_run.notes != notes or published_run.saved_run != saved_run or published_run.photo_url != photo_url + # Always allow saving when viewing a specific version or version_id )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
daras_ai_v2/base.py(5 hunks)daras_ai_v2/breadcrumbs.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Format code in reverse topological order: place the main() function at the top and dependencies below it
Files:
daras_ai_v2/base.pydaras_ai_v2/breadcrumbs.py
🧬 Code graph analysis (1)
daras_ai_v2/breadcrumbs.py (3)
bots/models/published_run.py (1)
is_root(282-283)bots/models/saved_run.py (1)
get_app_url(172-178)handles/models.py (1)
get_app_url(172-173)
🪛 Ruff (0.14.6)
daras_ai_v2/base.py
240-240: Unused class method argument: version_id
(ARG003)
508-508: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
daras_ai_v2/breadcrumbs.py
59-59: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
117-117: Undefined name pr_version
(F821)
119-119: Undefined name pr_version
(F821)
120-120: Undefined name pr_version
(F821)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (1)
daras_ai_v2/base.py (1)
1292-1297: LGTM!The version row correctly propagates
version_idvia query parameters and conditionally renders the version title.
| example_id: str | None = None, | ||
| run_id: str | None = None, | ||
| uid: str | None = None, | ||
| version_id: str | None = None, |
There was a problem hiding this comment.
Unused version_id parameter should be removed or implemented.
The version_id parameter is declared but never used in the method body. Looking at line 1292, version_id is already being passed via query_params, making this parameter redundant.
uid: str | None = None,
- version_id: str | None = None,
query_params: dict[str, str] | None = None,📝 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.
| version_id: str | None = None, | |
| uid: str | None = None, | |
| query_params: dict[str, str] | None = None, |
🧰 Tools
🪛 Ruff (0.14.6)
240-240: Unused class method argument: version_id
(ARG003)
🤖 Prompt for AI Agents
In daras_ai_v2/base.py at line 240, the parameter "version_id: str | None =
None" is declared but unused (it's redundant because version_id is already
injected into query_params at line 1292); remove the unused parameter from the
method signature and update any callers or tests that pass it, or if the
original intent was to accept an explicit version_id, instead wire this
parameter into the existing query_params construction (set
query_params["version_id"] = version_id when provided) and remove the redundant
hard-coded passing; ensure function docstring/type hints and all call sites are
adjusted accordingly.
| version_id = self.request.query_params.get("version_id", None) | ||
| pr_version = None | ||
| if version_id: | ||
| pr_version = pr.versions.get(version_id=version_id) | ||
|
|
||
| if pr_version and self.current_pr: | ||
| self.current_pr.title = pr_version.title | ||
| self.current_pr.notes = pr_version.notes | ||
| self.current_pr.photo_url = pr_version.photo_url | ||
|
|
There was a problem hiding this comment.
Missing exception handling for invalid version_id.
If version_id is provided but doesn't exist, pr.versions.get(version_id=version_id) will raise PublishedRunVersion.DoesNotExist, causing an unhandled 500 error.
version_id = self.request.query_params.get("version_id", None)
pr_version = None
if version_id:
- pr_version = pr.versions.get(version_id=version_id)
-
- if pr_version and self.current_pr:
- self.current_pr.title = pr_version.title
- self.current_pr.notes = pr_version.notes
- self.current_pr.photo_url = pr_version.photo_url
+ try:
+ pr_version = pr.versions.get(version_id=version_id)
+ except PublishedRunVersion.DoesNotExist:
+ pr_version = None
+ else:
+ self.current_pr.title = pr_version.title
+ self.current_pr.notes = pr_version.notes
+ self.current_pr.photo_url = pr_version.photo_urlNote: PublishedRunVersion needs to be imported from bots.models.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In daras_ai_v2/base.py around lines 434 to 443, calling
pr.versions.get(version_id=version_id) can raise
PublishedRunVersion.DoesNotExist for an invalid version_id; wrap the lookup in a
try/except catching PublishedRunVersion.DoesNotExist, set pr_version = None (or
handle gracefully by logging/returning an appropriate response) instead of
letting the exception bubble up, and add the import PublishedRunVersion from
bots.models at the top of the file.
| ) | ||
|
|
||
| if self.tab == RecipeTabs.run and is_example: | ||
| if self.tab == RecipeTabs.run and is_example or pr_version: |
There was a problem hiding this comment.
Ambiguous operator precedence - add parentheses for clarity.
The expression self.tab == RecipeTabs.run and is_example or pr_version has unclear precedence. Python evaluates this as (self.tab == RecipeTabs.run and is_example) or pr_version.
- if self.tab == RecipeTabs.run and is_example or pr_version:
+ if (self.tab == RecipeTabs.run and is_example) or pr_version:🧰 Tools
🪛 Ruff (0.14.6)
508-508: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
🤖 Prompt for AI Agents
In daras_ai_v2/base.py around line 508, the conditional "self.tab ==
RecipeTabs.run and is_example or pr_version" has ambiguous operator precedence;
modify it to make the intended grouping explicit by adding parentheses so the
comparison to RecipeTabs.run is grouped and the is_example/pr_version operands
are grouped together, e.g. treat it as "(self.tab == RecipeTabs.run) and
(is_example or pr_version)".
|
|
||
| is_root = pr and pr.saved_run == sr and pr.is_root() | ||
| is_example = not is_root and pr and pr.saved_run == sr | ||
| is_example = not is_root and pr and pr.saved_run == sr or sr.parent_version |
There was a problem hiding this comment.
Ambiguous operator precedence - add parentheses.
The expression evaluates as (not is_root and pr and pr.saved_run == sr) or sr.parent_version due to operator precedence. If sr.parent_version exists, is_example will be truthy (the version object itself), not a boolean.
- is_example = not is_root and pr and pr.saved_run == sr or sr.parent_version
+ is_example = (not is_root and pr and pr.saved_run == sr) or bool(sr.parent_version)📝 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.
| is_example = not is_root and pr and pr.saved_run == sr or sr.parent_version | |
| is_example = (not is_root and pr and pr.saved_run == sr) or bool(sr.parent_version) |
🧰 Tools
🪛 Ruff (0.14.6)
59-59: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
🤖 Prompt for AI Agents
In daras_ai_v2/breadcrumbs.py around line 59, the expression `is_example = not
is_root and pr and pr.saved_run == sr or sr.parent_version` has ambiguous
precedence and can yield a non-boolean when sr.parent_version is truthy; add
parentheses to make the intended grouping explicit (e.g. `(not is_root and pr
and pr.saved_run == sr)`) and ensure the result is boolean by wrapping the whole
expression with bool(...) or by explicitly comparing `sr.parent_version` (e.g.
`sr.parent_version is not None`) so `is_example` is always a boolean.
anish-work
force-pushed
the
fix_pr_version_title_notes
branch
from
2fa39e0 to
d64baa1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
daras_ai_v2/breadcrumbs.py (2)
59-59: Ambiguous operator precedence - add parentheses.The expression evaluates as
(not is_root and pr and pr.saved_run == sr) or sr.parent_version. Whensr.parent_versionis truthy,is_examplebecomes the version object rather than a boolean.- is_example = not is_root and pr and pr.saved_run == sr or sr.parent_version + is_example = (not is_root and pr and pr.saved_run == sr) or bool(sr.parent_version)
113-122: Critical:pr_versionis undefined - will causeNameErrorat runtime.The variable
pr_versionis referenced on lines 113, 115, and 116 but is never defined in this function. This will crash at runtime when execution reaches this branch.Based on the AI summary mentioning that
base.pyintroducespr_versionhandling, this variable likely needs to be passed as a parameter toget_title_breadcrumbs.Option 1 - Pass
pr_versionas parameter (recommended):def get_title_breadcrumbs( page_cls: typing.Union["BasePage", typing.Type["BasePage"]], sr: SavedRun, pr: PublishedRun | None, tab: typing.Optional["RecipeTabs"] = None, + pr_version: "PublishedRunVersion | None" = None, ) -> TitleBreadCrumbs:Option 2 - Derive from
sr.parent_versionif that's the intended source:- if pr_version: + if sr.parent_version: h1_title = TitleUrl( - title=pr_version.title, - url=pr_version.saved_run.get_app_url(), + title=sr.parent_version.title, + url=sr.parent_version.saved_run.get_app_url(), )Verify whether
pr_versionshould be passed from the caller or derived fromsr.parent_version:#!/bin/bash # Check how get_title_breadcrumbs is called in base.py rg -n -A5 'get_title_breadcrumbs' --type=pydaras_ai_v2/base.py (3)
240-240: Unusedversion_idparameter—remove it or wire it intoquery_params.The
version_idparameter is declared but never used in the method body. At line 1292,version_idis passed via thequery_paramsdictionary, making this parameter redundant.Remove the unused parameter:
uid: str | None = None, - version_id: str | None = None, query_params: dict[str, str] | None = None,Or, if you intend to support an explicit
version_idparameter, wire it intoquery_params:query_params: dict[str, str] | None = None, path_params: dict | None = None, ) -> str: if not tab: tab = RecipeTabs.run if query_params is None: query_params = {} + if version_id: + query_params["version_id"] = version_id
434-443: Missing exception handling for invalidversion_id.If
version_idis provided but doesn't exist,pr.versions.get(version_id=version_id)will raisePublishedRunVersion.DoesNotExist, causing an unhandled 500 error.Wrap the lookup in a try/except block:
version_id = self.request.query_params.get("version_id", None) pr_version = None if version_id: - pr_version = pr.versions.get(version_id=version_id) - - if pr_version and self.current_pr: - self.current_pr.title = pr_version.title - self.current_pr.notes = pr_version.notes - self.current_pr.photo_url = pr_version.photo_url + try: + pr_version = pr.versions.get(version_id=version_id) + except PublishedRunVersion.DoesNotExist: + pr_version = None + else: + self.current_pr.title = pr_version.title + self.current_pr.notes = pr_version.notes + self.current_pr.photo_url = pr_version.photo_url
508-508: Add parentheses to clarify operator precedence.The expression
self.tab == RecipeTabs.run and is_example or pr_versionhas ambiguous precedence. Python evaluates this as(self.tab == RecipeTabs.run and is_example) or pr_version.Make the precedence explicit:
- if self.tab == RecipeTabs.run and is_example or pr_version: + if (self.tab == RecipeTabs.run and is_example) or pr_version:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
daras_ai_v2/base.py(5 hunks)daras_ai_v2/breadcrumbs.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Format code in reverse topological order: place the main() function at the top and dependencies below it
Files:
daras_ai_v2/breadcrumbs.pydaras_ai_v2/base.py
🧬 Code graph analysis (1)
daras_ai_v2/breadcrumbs.py (8)
bots/models/published_run.py (1)
is_root(282-283)bots/models/saved_run.py (1)
get_app_url(172-178)handles/models.py (1)
get_app_url(172-173)bots/models/workflow.py (1)
page_cls(242-245)daras_ai_v2/base.py (1)
get_run_title(1170-1171)recipes/VideoBots.py (1)
get_run_title(818-831)recipes/asr_page.py (1)
get_run_title(100-113)recipes/ModelTrainer.py (1)
get_run_title(249-253)
🪛 Ruff (0.14.6)
daras_ai_v2/breadcrumbs.py
59-59: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
113-113: Undefined name pr_version
(F821)
115-115: Undefined name pr_version
(F821)
116-116: Undefined name pr_version
(F821)
daras_ai_v2/base.py
240-240: Unused class method argument: version_id
(ARG003)
508-508: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (python)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (1)
daras_ai_v2/base.py (1)
1288-1297: LGTM!The version row correctly propagates
version_idviaquery_paramsand conditionally renders the version title when available.
daras_ai_v2/base.py
Outdated
| version_id = self.request.query_params.get("version_id", None) | ||
| return ( | ||
| published_run.title != title | ||
| or published_run.notes != notes | ||
| or published_run.saved_run != saved_run | ||
| or published_run.photo_url != photo_url | ||
| or version_id | ||
| ) |
There was a problem hiding this comment.
Questionable change detection logic—version_id presence always returns True.
The method returns True whenever version_id is present in query params, regardless of whether anything actually changed. This means viewing any historical version will trigger the "Unpublished changes" indicator (line 490-491), which may confuse users.
Consider whether viewing a historical version should be treated as "changed":
- If the intent is to indicate "you're viewing an old version," consider a separate indicator rather than conflating it with "unpublished changes."
- If
version_idshould only triggerTruewhen it differs from the current published version, add that check:
version_id = self.request.query_params.get("version_id", None)
return (
published_run.title != title
or published_run.notes != notes
or published_run.saved_run != saved_run
or published_run.photo_url != photo_url
- or version_id
+ or (version_id and version_id != str(published_run.versions.latest().version_id))
)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In daras_ai_v2/base.py around lines 955 to 962, the current change-detection
returns True whenever a version_id query param exists, which always marks the
run as "changed"; instead, only treat it as changed if the requested version_id
actually differs from the published run's version identifier (e.g., compare
version_id to published_run.version_id or published_run.version if that field
exists), or remove version_id from this boolean expression and surface a
separate "viewing historical version" flag/indicator if the intent is to notify
users they are viewing an older revision; update the conditional accordingly so
version_id alone does not force a True result.
d64baa1 to
3e78a74
Compare
- remove print statements - remove unsused pr_version code
anish-work
force-pushed
the
fix_pr_version_title_notes
branch
from
3e78a74 to
83e1ec7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
daras_ai_v2/breadcrumbs.py (1)
58-61: Clarifyis_exampleprecedence and force a boolean result
is_example = not is_root and pr and pr.saved_run == sr or sr.parent_versionmixesand/orwithout parentheses and returns asr.parent_versionobject rather than a strict bool when present. That’s harder to read and potentially surprising.Consider parenthesizing and wrapping the
parent_versionleg inbool:- is_example = not is_root and pr and pr.saved_run == sr or sr.parent_version + is_example = (not is_root and pr and pr.saved_run == sr) or bool(sr.parent_version)This keeps the current semantics (example when it’s the published run or when there’s a parent_version) while making intent and type explicit.
daras_ai_v2/base.py (4)
233-272:app_url’sversion_idparameter is unused—either wire it in or drop it
version_idis accepted in theapp_urlsignature but never affects the generated URL, while callers currently passversion_idthroughquery_paramsinstead. This is confusing and flagged by Ruff as an unused argument.Either remove
version_idfrom the signature, or (preferably) integrate it intoquery_paramsso callers can use the dedicated parameter:def app_url( cls, @@ example_id: str | None = None, run_id: str | None = None, uid: str | None = None, - version_id: str | None = None, + version_id: str | None = None, query_params: dict[str, str] | None = None, path_params: dict | None = None, ) -> str: if not tab: tab = RecipeTabs.run if query_params is None: query_params = {} + + if version_id is not None: + query_params |= {"version_id": str(version_id)}You can then gradually update call sites like
_render_version_rowto passversion_id=version.version_idinstead of building a one-offquery_paramsdict.
508-513: Disambiguateand/orprecedence in notes visibility condition
if self.tab == RecipeTabs.run and is_example or pr_version:is evaluated as(self.tab == RecipeTabs.run and is_example) or pr_version, but that’s not obvious at a glance and is flagged by Ruff.Add parentheses to make the grouping explicit and keep the current behavior:
- if self.tab == RecipeTabs.run and is_example or pr_version: + if (self.tab == RecipeTabs.run and is_example) or pr_version:This still shows notes when you’re on the run tab with an example, or whenever a
pr_versionis selected, but removes the ambiguity for readers and linters.
955-962: Confirm whetherversion_idshould always imply “published run changed”
_has_published_run_changednow returnsTruewhenever aversion_idquery param is present:version_id = self.request.query_params.get("version_id", None) return ( published_run.title != title or published_run.notes != notes or published_run.saved_run != saved_run or published_run.photo_url != photo_url or version_id )This means that viewing a historical version (with no actual field diffs) is always treated as “changed”. If the intent is:
- to always allow saving when editing from a historical version (effectively “revert to this version”), this behavior is fine but should probably be documented; or
- to indicate changes only when there’s a real diff from the current published run,
then
version_idshould either be removed from this expression or compared against the latest version’s identifier instead.Please double‑check the intended UX here and adjust the predicate (and/or add a brief comment) so future readers don’t misinterpret this as a bug.
432-443: Handle invalidversion_idwhen resolvingpr_versionin_render_header
pr_version = pr.versions.get(version_id=version_id)will raisePublishedRunVersion.DoesNotExistfor an invalid or staleversion_id, resulting in a 500 instead of a graceful fallback.Wrap this lookup in a
try/exceptand only apply overrides when a version is actually found:- sr, pr = self.current_sr_pr - is_example = pr.saved_run == sr - version_id = self.request.query_params.get("version_id", None) - pr_version = None - if version_id: - pr_version = pr.versions.get(version_id=version_id) - - if pr_version and self.current_pr: - self.current_pr.title = pr_version.title - self.current_pr.notes = pr_version.notes - self.current_pr.photo_url = pr_version.photo_url + sr, pr = self.current_sr_pr + is_example = pr.saved_run == sr + version_id = self.request.query_params.get("version_id") + pr_version: PublishedRunVersion | None = None + if version_id: + try: + pr_version = pr.versions.get(version_id=version_id) + except PublishedRunVersion.DoesNotExist: + pr_version = None + else: + if self.current_pr: + self.current_pr.title = pr_version.title + self.current_pr.notes = pr_version.notes + self.current_pr.photo_url = pr_version.photo_urlThis keeps the per-version header behavior but avoids crashing on bad input.
🧹 Nitpick comments (1)
daras_ai_v2/base.py (1)
1288-1297: Version history links and titles look correct; consider using theversion_idparameterThe version history row now:
- Links to a specific saved run and
version_idviaquery_params=dict(version_id=version.version_id), and- Renders
version.titlewhen present.This matches the per-version UX being introduced and looks good.
Once
app_url’sversion_idparameter is wired up, you might simplify this call to:- url = self.app_url( - example_id=version.published_run.published_run_id, - run_id=version.saved_run.run_id, - uid=version.saved_run.uid, - query_params=dict(version_id=version.version_id), - ) + url = self.app_url( + example_id=version.published_run.published_run_id, + run_id=version.saved_run.run_id, + uid=version.saved_run.uid, + version_id=version.version_id, + )Purely a readability/consistency tweak; existing behavior is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
daras_ai_v2/base.py(5 hunks)daras_ai_v2/breadcrumbs.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Format code in reverse topological order: place the main() function at the top and dependencies below it
Files:
daras_ai_v2/base.pydaras_ai_v2/breadcrumbs.py
🧬 Code graph analysis (1)
daras_ai_v2/breadcrumbs.py (1)
bots/models/published_run.py (1)
is_root(282-283)
🪛 Ruff (0.14.6)
daras_ai_v2/base.py
240-240: Unused class method argument: version_id
(ARG003)
508-508: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
daras_ai_v2/breadcrumbs.py
59-59: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (3.10.12, 1.8.3)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
daras_ai_v2/base.py (3)
240-240: Unusedversion_idparameter.This parameter is declared but never used in the method body. The
version_idis already being passed viaquery_paramsat line 1300, making this parameter redundant.
434-442: Missing exception handling for invalidversion_id.If
version_idis provided but doesn't exist,pr.versions.get(version_id=version_id)will raisePublishedRunVersion.DoesNotExist, causing an unhandled 500 error.
512-512: Ambiguous operator precedence.The expression
self.tab == RecipeTabs.run and is_example or pr_versionhas unclear precedence. Add parentheses for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
daras_ai_v2/base.py(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Format code in reverse topological order: place the main() function at the top and dependencies below it
Files:
daras_ai_v2/base.py
🧠 Learnings (2)
📚 Learning: 2025-09-24T07:06:56.185Z
Learnt from: nikochiko
Repo: GooeyAI/gooey-server PR: 792
File: widgets/saved_workflow.py:0-0
Timestamp: 2025-09-24T07:06:56.185Z
Learning: In `daras_ai_v2/base.py`, the `_examples_tab()` method QuerySet at line 2164 needs `.select_related('saved_run', 'workspace', 'last_edited_by').prefetch_related('tags', 'versions')` to optimize database queries for published run rendering.
Applied to files:
daras_ai_v2/base.py
📚 Learning: 2025-09-24T07:06:56.185Z
Learnt from: nikochiko
Repo: GooeyAI/gooey-server PR: 792
File: widgets/saved_workflow.py:0-0
Timestamp: 2025-09-24T07:06:56.185Z
Learning: In `daras_ai_v2/base.py`, the `_saved_tab()` method QuerySet at line 2199 needs `.select_related('saved_run', 'workspace', 'last_edited_by').prefetch_related('tags', 'versions')` to avoid N+1 queries when rendering published runs.
Applied to files:
daras_ai_v2/base.py
🧬 Code graph analysis (1)
daras_ai_v2/base.py (1)
routers/root.py (1)
RecipeTabs(893-937)
🪛 Ruff (0.14.6)
daras_ai_v2/base.py
240-240: Unused class method argument: version_id
(ARG003)
512-512: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (python)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (3)
daras_ai_v2/base.py (3)
490-494: LGTM!The condition correctly incorporates the version check using
self.is_published_run_version()to display the unpublished changes indicator when viewing a historical version.
953-954: LGTM!Clean helper method that encapsulates the version ID query parameter check.
1300-1335: Well-structured version row rendering.The implementation properly:
- Passes
version_idthrough query params for deep linking- Adds hover styling for better UX
- Uses
html.escapefor change notes (good XSS prevention)- Shows contextual information (FIRST VERSION badge, change notes, or rename info)
| if ( | ||
| not self._has_published_run_changed(published_run=pr, **updates) | ||
| and self.is_published_run_version | ||
| ): |
There was a problem hiding this comment.
Critical bug: Method not called - missing parentheses.
self.is_published_run_version is a method reference, not a method call. Without (), it evaluates to the method object itself (always truthy), causing the condition to incorrectly block publishing even when not viewing a version.
if (
not self._has_published_run_changed(published_run=pr, **updates)
- and self.is_published_run_version
+ and self.is_published_run_version()
):🤖 Prompt for AI Agents
In daras_ai_v2/base.py around lines 882 to 885, the condition uses
self.is_published_run_version as a method reference instead of calling it, so it
always evaluates truthy and blocks publishing; change it to call the method
(self.is_published_run_version()) so the boolean result is used, and optionally
wrap with bool(...) if explicit casting is desired.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
daras_ai_v2/base.py (4)
240-240: Unusedversion_idparameter.The
version_idparameter is declared but never used in the method body. It's already passed viaquery_paramsat the call site in_render_version_row(line 1300).
434-442: Missing exception handling for invalidversion_id.If an invalid
version_idis provided,pr.versions.get(version_id=version_id)will raisePublishedRunVersion.DoesNotExist, causing an unhandled 500 error.version_id = self.request.query_params.get("version_id", None) pr_version = None if version_id: - pr_version = pr.versions.get(version_id=version_id) - - if pr_version and self.current_pr: + try: + pr_version = pr.versions.get(version_id=version_id) + except PublishedRunVersion.DoesNotExist: + pr_version = None + else: self.current_pr.title = pr_version.title self.current_pr.notes = pr_version.notes self.current_pr.photo_url = pr_version.photo_url
512-512: Ambiguous operator precedence - add parentheses for clarity.Python evaluates
self.tab == RecipeTabs.run and is_example or pr_versionas(self.tab == RecipeTabs.run and is_example) or pr_version. Add explicit parentheses to clarify intent:- if self.tab == RecipeTabs.run and is_example or pr_version: + if (self.tab == RecipeTabs.run and is_example) or pr_version:
882-885: Critical bug: Method not called - missing parentheses.
self.is_published_run_versionis a method reference, not a method call. Without(), it evaluates to the method object itself (always truthy), causing the condition to incorrectly trigger "No changes to publish" error even when not viewing a version.if ( not self._has_published_run_changed(published_run=pr, **updates) - and self.is_published_run_version + and self.is_published_run_version() ):
🧹 Nitpick comments (1)
widgets/saved_workflow.py (1)
38-42: Mutatingpublished_runin-place may cause unexpected side effects.This function modifies the passed
published_runobject directly. If the caller retains a reference to this object (e.g.,version.published_runin_render_version_row), subsequent code may see the mutated values unexpectedly.Consider working with a shallow copy or documenting this side effect explicitly:
+ # Note: This function mutates published_run fields when version is provided if version: published_run.title = version.title published_run.notes = version.notes published_run.photo_url = version.photo_url published_run.last_edited_by = version.changed_byAlternatively, if isolation is needed:
if version: published_run = copy(published_run) published_run.title = version.title # ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
daras_ai_v2/base.py(7 hunks)widgets/saved_workflow.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Format code in reverse topological order: place the main() function at the top and dependencies below it
Files:
widgets/saved_workflow.pydaras_ai_v2/base.py
🧠 Learnings (3)
📚 Learning: 2025-09-24T07:23:39.522Z
Learnt from: nikochiko
Repo: GooeyAI/gooey-server PR: 792
File: widgets/saved_workflow.py:0-0
Timestamp: 2025-09-24T07:23:39.522Z
Learning: In `widgets/workflow_search.py`, the `render_search_results()` function QuerySet at line 375 needs `.select_related("workspace", "saved_run", "last_edited_by").prefetch_related("tags", "versions")` to avoid N+1 queries when rendering search results with published run previews. Note: `created_by` should not be included as it's not used in workflow rendering.
Applied to files:
widgets/saved_workflow.py
📚 Learning: 2025-09-24T07:06:56.185Z
Learnt from: nikochiko
Repo: GooeyAI/gooey-server PR: 792
File: widgets/saved_workflow.py:0-0
Timestamp: 2025-09-24T07:06:56.185Z
Learning: In `daras_ai_v2/base.py`, the `_saved_tab()` method QuerySet at line 2199 needs `.select_related('saved_run', 'workspace', 'last_edited_by').prefetch_related('tags', 'versions')` to avoid N+1 queries when rendering published runs.
Applied to files:
daras_ai_v2/base.py
📚 Learning: 2025-09-24T07:06:56.185Z
Learnt from: nikochiko
Repo: GooeyAI/gooey-server PR: 792
File: widgets/saved_workflow.py:0-0
Timestamp: 2025-09-24T07:06:56.185Z
Learning: In `daras_ai_v2/base.py`, the `_examples_tab()` method QuerySet at line 2164 needs `.select_related('saved_run', 'workspace', 'last_edited_by').prefetch_related('tags', 'versions')` to optimize database queries for published run rendering.
Applied to files:
daras_ai_v2/base.py
🧬 Code graph analysis (2)
widgets/saved_workflow.py (3)
bots/models/published_run.py (2)
PublishedRunVersion(349-392)PublishedRun(118-346)widgets/demo_button.py (1)
get_demo_bots(41-50)bots/models/bot_integration.py (4)
Platform(28-75)get_icon(36-51)get_title(53-60)get_demo_button_color(62-75)
daras_ai_v2/base.py (1)
widgets/saved_workflow.py (1)
render_saved_workflow_preview(25-102)
🪛 Ruff (0.14.6)
daras_ai_v2/base.py
240-240: Unused class method argument: version_id
(ARG003)
512-512: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (4)
widgets/saved_workflow.py (1)
105-139: LGTM!The
hide_demo_pillsparameter is cleanly threaded through to conditionally skip demo pill rendering. The styling and pill construction logic is well-structured.daras_ai_v2/base.py (3)
490-494: LGTM!The unpublished changes indicator logic correctly uses
self.is_published_run_version()with parentheses.
953-954: LGTM!The
is_published_run_version()method correctly checks for presence ofversion_idin query params.
1296-1332: LGTM!The version row rendering is well-structured:
- Properly passes
version_idviaquery_params- Uses the new
versionparameter inrender_saved_workflow_preview- Clean version card styling with hover effect
- Appropriate "FIRST VERSION" badge and change notes display
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
daras_ai_v2/breadcrumbs.py (1)
77-83: Empty H1 title whenversion.titleis blank—add fallback topage_cls.get_recipe_title().Lines 80–83:
PublishedRunVersion.titledefaults to an empty string and can legitimately be left blank. The current logic:version.title if version else page_cls.get_recipe_title(),will render an empty H1 even though a recipe title exists. The
title_with_prefix()method (lines 29–31) explicitly guards against empty titles, confirming this is a UI bug.Apply the fallback pattern already used in the
is_examplecase (lines 91–94):(version.title or page_cls.get_recipe_title()) if version else page_cls.get_recipe_title(),This ensures version-specific titles display when provided, but always guarantees a non-empty H1.
♻️ Duplicate comments (1)
daras_ai_v2/breadcrumbs.py (1)
60-62: Fix ambiguousis_exampleexpression and ensure it is boolean.As Ruff noted and a previous review mentioned, the current expression:
is_example = not is_root and pr and pr.saved_run == sr or sr.parent_versionrelies on
and/orprecedence and can assignis_exampleto a non-boolean value (e.g.,sr.parent_versionorpr). This is harder to read and may confuse future changes.You can keep the existing truthiness while making the intent explicit and forcing a boolean:
- is_example = not is_root and pr and pr.saved_run == sr or sr.parent_version + is_example = (not is_root and pr and pr.saved_run == sr) or bool(sr.parent_version)This preserves the logic (“example if a non-root PR for this
sr, or ifsr.parent_versionexists”) while makingis_examplealways abooland clarifying precedence.
🧹 Nitpick comments (1)
daras_ai_v2/breadcrumbs.py (1)
51-57: Confirm intended usage surface for the newversionparameter.The
version: PublishedRunVersion | None = Noneparameter is currently only used in the root (is_root) branch for the h1 title, and not in the example/run branches. If the goal is strictly to override the root breadcrumb title when a version is active, this is fine; if you also expect version-specific titles when viewing non-root runs, you may want to threadversioninto theis_example/is_runbranches as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
daras_ai_v2/breadcrumbs.py(4 hunks)widgets/saved_workflow.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- widgets/saved_workflow.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Format code in reverse topological order: place the main() function at the top and dependencies below it
Files:
daras_ai_v2/breadcrumbs.py
🧠 Learnings (2)
📚 Learning: 2025-09-24T07:06:56.185Z
Learnt from: nikochiko
Repo: GooeyAI/gooey-server PR: 792
File: widgets/saved_workflow.py:0-0
Timestamp: 2025-09-24T07:06:56.185Z
Learning: In `daras_ai_v2/base.py`, the `_saved_tab()` method QuerySet at line 2199 needs `.select_related('saved_run', 'workspace', 'last_edited_by').prefetch_related('tags', 'versions')` to avoid N+1 queries when rendering published runs.
Applied to files:
daras_ai_v2/breadcrumbs.py
📚 Learning: 2025-09-24T07:06:56.185Z
Learnt from: nikochiko
Repo: GooeyAI/gooey-server PR: 792
File: widgets/saved_workflow.py:0-0
Timestamp: 2025-09-24T07:06:56.185Z
Learning: In `daras_ai_v2/base.py`, the `_examples_tab()` method QuerySet at line 2164 needs `.select_related('saved_run', 'workspace', 'last_edited_by').prefetch_related('tags', 'versions')` to optimize database queries for published run rendering.
Applied to files:
daras_ai_v2/breadcrumbs.py
🧬 Code graph analysis (1)
daras_ai_v2/breadcrumbs.py (3)
bots/models/published_run.py (2)
PublishedRunVersion(349-392)is_root(282-283)routers/root.py (1)
RecipeTabs(893-937)daras_ai_v2/base.py (1)
get_recipe_title(1182-1183)
🪛 Ruff (0.14.6)
daras_ai_v2/breadcrumbs.py
61-61: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (3.10.12, 1.8.3)
🔇 Additional comments (2)
daras_ai_v2/breadcrumbs.py (2)
5-5: Import ofPublishedRunVersionis appropriate and consistent.The explicit import matches the new
version: PublishedRunVersion | Noneparameter type and keeps the dependency clear; no further changes needed here.
107-109: Spacing change is harmless.The added blank line after
prompt_title = ...is purely cosmetic and does not affect behavior; no change needed.
anish-work
force-pushed
the
fix_pr_version_title_notes
branch
from
4d196c8 to
9e57851
Compare
anish-work
force-pushed
the
fix_pr_version_title_notes
branch
from
452c08a to
7116a44
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
daras_ai_v2/base.py (4)
232-242: Unusedversion_idarg inapp_urland missing propagation fromcurrent_app_url.
version_idis accepted byapp_urlbut never used, andcurrent_app_urldoes not carryversion_idfromself.request.query_params, so version-aware views lose context in generated URLs and static analysis flags the arg as unused.You can wire this through and satisfy Ruff like this:
@@ def current_app_url( self, tab: RecipeTabs = RecipeTabs.run, *, query_params: dict[str, str] | None = None, path_params: dict | None = None, ) -> str: if query_params is None: query_params = {} - example_id, run_id, uid = extract_query_params(self.request.query_params) + example_id, run_id, uid = extract_query_params(self.request.query_params) + version_id = self.request.query_params.get("version_id") + if version_id is not None: + query_params.setdefault("version_id", version_id) return self.app_url( tab=tab, example_id=example_id, run_id=run_id, uid=uid, + version_id=version_id, query_params=query_params, path_params=path_params, ) @@ def app_url( cls, @@ - version_id: str | None = None, + version_id: str | None = None, query_params: dict[str, str] | None = None, path_params: dict | None = None, ) -> str: @@ - if query_params is None: - query_params = {} + if query_params is None: + query_params = {} + + if version_id is not None: + query_params.setdefault("version_id", str(version_id))
490-495: Clarify header conditions for “Unpublished changes” and notes display.Two small issues here:
is_published_run_version()makes the “Unpublished changes” pill show for any historical version, even when nothing actually changed; if that’s meant as “you’re viewing an old version”, consider a separate indicator instead of overloading this pill.if self.tab == RecipeTabs.run and is_example or pr_version:is hard to read; theand/orprecedence is non-obvious and Ruff flags it.Minimal, behavior-preserving tweak:
@@ - if ( - request_changed - or (can_save and not is_example) - or self.is_published_run_version() - ): + if ( + request_changed + or (can_save and not is_example) + or self.is_published_run_version() + ): @@ - if self.tab == RecipeTabs.run and is_example or pr_version: + if (self.tab == RecipeTabs.run and is_example) or pr_version: with gui.div(className="container-margin-reset"): if self.current_pr and self.current_pr.notes: gui.write(self.current_pr.notes, line_clamp=3)(If the intent is “only show notes on the Run tab”, you may instead want
if self.tab == RecipeTabs.run and (is_example or pr_version):.)Also applies to: 512-515
432-443: Protectpr.versions.get(version_id=...)from invalid/unknownversion_id.A bad or stale
version_idin the URL will raisePublishedRunVersion.DoesNotExist(orValueErroron bad types) and 500 the page.Consider making the lookup tolerant and only overriding
current_prwhen a version is actually found:@@ - sr, pr = self.current_sr_pr - is_example = pr.saved_run == sr - version_id = self.request.query_params.get("version_id", None) - pr_version = None - if version_id: - pr_version = pr.versions.get(version_id=version_id) - - if pr_version and self.current_pr: - self.current_pr.title = pr_version.title - self.current_pr.notes = pr_version.notes - self.current_pr.photo_url = pr_version.photo_url + sr, pr = self.current_sr_pr + is_example = pr.saved_run == sr + version_id = self.request.query_params.get("version_id") + pr_version: PublishedRunVersion | None = None + if version_id: + try: + pr_version = pr.versions.get(version_id=version_id) + except (PublishedRunVersion.DoesNotExist, ValueError): + pr_version = None + + if pr_version and self.current_pr: + self.current_pr.title = pr_version.title + self.current_pr.notes = pr_version.notes + self.current_pr.photo_url = pr_version.photo_url
882-885:is_published_run_versionused as a method reference, not called.In the publish form, this condition currently always uses a truthy method object, so it reduces to just “no changes to publish”, regardless of whether you’re viewing a historical version:
if ( not self._has_published_run_changed(published_run=pr, **updates) and self.is_published_run_version ):If the goal is to gate this branch only when viewing a version-specific URL, you need to call the method:
@@ - if ( - not self._has_published_run_changed(published_run=pr, **updates) - and self.is_published_run_version - ): + if ( + not self._has_published_run_changed(published_run=pr, **updates) + and self.is_published_run_version() + ): gui.error("No changes to publish", icon="⚠️") return @@ - def is_published_run_version(self) -> bool: - return self.request.query_params.get("version_id", None) is not None + def is_published_run_version(self) -> bool: + return self.request.query_params.get("version_id") is not NoneAlso applies to: 953-955
🧹 Nitpick comments (2)
daras_ai_v2/base.py (2)
1283-1290: Version history rendering looks good; consider prefetchingchanged_byfor performance.The new card UI for versions is a nice UX improvement. However,
_render_version_historyfetchesself.current_pr.versions.all()and_render_version_rowaccessesversion.changed_by, which can cause N+1 queries on large histories.You can cheaply avoid that:
@@ def _render_version_history(self): - versions = self.current_pr.versions.all() + versions = self.current_pr.versions.select_related("changed_by").all() first_version = versions[0] for version, older_version in pairwise(versions): first_version = older_version self._render_version_row(version, older_version) self._render_version_row(first_version, None)Also applies to: 1291-1339
2257-2277: Addselect_related/prefetch_relatedon examples/saved tabs to avoid N+1 queries.Both
_examples_taband_saved_tabrenderPublishedRuncards (viarender_saved_workflow_preview) but buildqswithout anyselect_related/prefetch_related, which can be expensive when you hit relatedsaved_run,workspace,tags, orversions.Based on prior perf learnings for this file, something like this would help:
@@ def _examples_tab(self): - qs = PublishedRun.objects.filter( - PublishedRun.approved_example_q(), - workflow=self.workflow, - ) + qs = ( + PublishedRun.objects.filter( + PublishedRun.approved_example_q(), + workflow=self.workflow, + ) + .select_related("saved_run", "workspace", "last_edited_by") + .prefetch_related("tags", "versions") + ) @@ def _saved_tab(self): @@ - qs = PublishedRun.objects.filter(Q(workflow=self.workflow) & pr_filter) + qs = ( + PublishedRun.objects.filter(Q(workflow=self.workflow) & pr_filter) + .select_related("saved_run", "workspace", "last_edited_by") + .prefetch_related("tags", "versions") + )Based on learnings, this should eliminate N+1s when rendering these lists.
Also applies to: 2282-2298
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
daras_ai_v2/base.py(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,js,ts,tsx,java,cs,cpp,c,go,rb,php}
📄 CodeRabbit inference engine (.cursor/rules/devs-rules.mdc)
Format code in reverse topological order: place the main() function at the top and dependencies below it
Files:
daras_ai_v2/base.py
🧠 Learnings (2)
📚 Learning: 2025-09-24T07:06:56.185Z
Learnt from: nikochiko
Repo: GooeyAI/gooey-server PR: 792
File: widgets/saved_workflow.py:0-0
Timestamp: 2025-09-24T07:06:56.185Z
Learning: In `daras_ai_v2/base.py`, the `_saved_tab()` method QuerySet at line 2199 needs `.select_related('saved_run', 'workspace', 'last_edited_by').prefetch_related('tags', 'versions')` to avoid N+1 queries when rendering published runs.
Applied to files:
daras_ai_v2/base.py
📚 Learning: 2025-09-24T07:06:56.185Z
Learnt from: nikochiko
Repo: GooeyAI/gooey-server PR: 792
File: widgets/saved_workflow.py:0-0
Timestamp: 2025-09-24T07:06:56.185Z
Learning: In `daras_ai_v2/base.py`, the `_examples_tab()` method QuerySet at line 2164 needs `.select_related('saved_run', 'workspace', 'last_edited_by').prefetch_related('tags', 'versions')` to optimize database queries for published run rendering.
Applied to files:
daras_ai_v2/base.py
🪛 Ruff (0.14.6)
daras_ai_v2/base.py
240-240: Unused class method argument: version_id
(ARG003)
512-512: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (3.10.12, 1.8.3)
- GitHub Check: test (3.10.12, 1.8.3)
Q/A checklist
How to check import time?
You can visualize this using tuna:
To measure import time for a specific library:
To reduce import times, import libraries that take a long time inside the functions that use them instead of at the top of the file:
Legal Boilerplate
Look, I get it. The entity doing business as “Gooey.AI” and/or “Dara.network” was incorporated in the State of Delaware in 2020 as Dara Network Inc. and is gonna need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Dara Network Inc can use, modify, copy, and redistribute my contributions, under its choice of terms.