-
Notifications
You must be signed in to change notification settings - Fork 7.7k
feat: sort metadata by date #10083
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: sort metadata by date #10083
Conversation
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de1e384b6e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rollout_paths.sort_by_key(|(path, _archived)| { | ||
| let parsed = path | ||
| .file_name() | ||
| .and_then(|name| name.to_str()) | ||
| .and_then(parse_timestamp_uuid_from_filename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep archived copies from being overwritten on backfill
This sort ignores the archived flag and sort_by_key is not stable, so if the same rollout file exists in both sessions and archived_sessions (e.g., an interrupted archive move or manual copy), the unarchived copy can be processed last and upsert_thread will overwrite the archived record, clearing archived_at. Previously the loop order guaranteed archived files were applied after sessions. Consider including archived in the sort key (or preserving the original root ordering) so archived copies always win when duplicates exist.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not relevant
gt-oai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these are just going to go into SQLite then it's not clear to me why this matters...?
Because we do this backfill only once so if people close codex directly after starting it, we still one the most recent files to be in |
Oh okay, would make sense as a comment then. Or fixing the underlying problem which is that backfill can be interrupted and not restarted. Otherwise we'll get bug reports of people not knowing this backfill was happening, killing Codex, and then missing sessions? |
No description provided.